Conversation
|
🎉 This PR is included in version 2.6.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
Pull request overview
Renames and reorganizes root package.json scripts as part of the #138 cleanup, while also improving client-side store fetch behavior by deduping concurrent requests and updating unit tests/docs accordingly.
Changes:
- Reworked root scripts into grouped
docker:*,db:*,docs:*, andtest:*commands (and addedtest:socketto the aggregatedtestscript). - Added in-flight promise de-duplication to several Pinia stores (
domains,subdomains,buildings,authentication) to prevent duplicate concurrent API calls. - Updated developer docs and client store tests to reflect the new commands/behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| server/twin-service/tests/setup.ts | Removes now-redundant test setup comments. |
| package.json | Script cleanup/restructure; adds DB clear subcommands; updates aggregated test script. |
| documentation/developer/page-12.qd | Updates documented run commands to new docker:*/db:* scripts. |
| client/src/stores/domain.ts | Adds promise-based de-dupe for domain/subdomain fetching. |
| client/src/stores/buildings.ts | Adds promise-based de-dupe for buildings fetching. |
| client/src/stores/authentication.ts | Adds promise-based de-dupe for hydrate(). |
| client/src/stores/tests/domain.spec.ts | Updates tests for makeRequest naming + concurrent fetch de-dupe. |
| client/src/stores/tests/buildings.spec.ts | Updates tests for concurrent fetch de-dupe. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this._membershipsPromise = ( async () => { | ||
| try { |
There was a problem hiding this comment.
There’s an extra space inside the parentheses before async (( async () => {), which is inconsistent with the rest of the codebase and will likely be reformatted by Prettier/ESLint. Remove the inner-paren whitespace to match standard formatting.
| it('does not call makeRequest when already loading', async () => { | ||
| vi.mocked(makeRequest).mockResolvedValue(makeResponse(true, ['sub1']) as unknown as Response) | ||
|
|
||
| const store = useSubdomainsStore() | ||
| store.loading = true | ||
| const memberships = [makeMembership('acme')] | ||
|
|
||
| await store.fetch([makeMembership('acme')]) | ||
| const fetch1 = store.fetch(memberships) | ||
| const fetch2 = store.fetch(memberships) | ||
| await Promise.all([fetch1, fetch2]) | ||
|
|
||
| expect(makeRequest).not.toHaveBeenCalled() | ||
| expect(makeRequest).toHaveBeenCalledTimes(1) | ||
| }) |
There was a problem hiding this comment.
Test name doesn’t match what it asserts: it says "does not call makeRequest when already loading", but the test actually verifies concurrent calls dedupe down to a single request (toHaveBeenCalledTimes(1)). Rename the test description to reflect the concurrency behavior being tested.
| @@ -12,7 +12,7 @@ Because the entire ecosystem is containerized, your day-to-day workflow relies e | |||
| For active development, you will almost exclusively use the `dev` script. | |||
There was a problem hiding this comment.
Line 12 still refers to the dev script, but the example command is now npm run docker:dev and the dev root script was removed from package.json. Update the text to reference docker:dev (or reintroduce a dev alias script) so the documentation matches the actual scripts.
| For active development, you will almost exclusively use the `dev` script. | |
| For active development, you will almost exclusively use the `docker:dev` script. |
|
|
||
| .code lang:{bash} | ||
| npm run stop | ||
| npm run docker:stop |
There was a problem hiding this comment.
npm run docker:stop is documented here, but package.json defines docker:down (no docker:stop script). Either rename the script in package.json, add a docker:stop alias, or update this documentation to use docker:down to avoid broken instructions.
| npm run docker:stop | |
| npm run docker:down |
|
|
||
| .code lang:{bash} | ||
| npm run clear | ||
| npm run db:clear |
There was a problem hiding this comment.
The paragraph above says "use the clear command", but the command shown is now npm run db:clear. Update the wording to refer to db:clear (or provide a clear alias) so the surrounding explanation stays accurate.
| "env:setup": "node setup.js", | ||
| "docker:dev": "npm run env:setup && docker compose -f docker-compose.yml -f docker-compose.dev.yml up --watch --build", | ||
| "docker:start": "npm run env:setup && docker compose up --build -d", | ||
| "docker:down": "docker compose down --remove-orphans", |
There was a problem hiding this comment.
Documentation currently references docker:stop, but the scripts define docker:down. Consider adding a docker:stop alias (or renaming docker:down) to keep the command set consistent and prevent broken docs/usage.
| "docker:down": "docker compose down --remove-orphans", | |
| "docker:down": "docker compose down --remove-orphans", | |
| "docker:stop": "npm run docker:down", |
| "docker:logs": "docker compose logs -f", | ||
|
|
||
| "// --- DATABASE ---": "", | ||
| "db:clear:auth": "docker compose exec auth-db mongosh authdb --eval \"db.dropDatabase()\"", | ||
| "db:clear:twin": "docker compose exec twin-db mongosh twindb --eval \"db.dropDatabase()\"", | ||
| "db:clear:notification": "docker compose exec notification-db mongosh notificationdb --eval \"db.dropDatabase()\"", | ||
| "db:clear": "npm run db:clear:auth && npm run db:clear:twin && npm run db:clear:notification", |
There was a problem hiding this comment.
This change removes the root-level dev/start/stop/clear scripts entirely. There are still docs in documentation/developer/page-11.qd that instruct using those commands; either update the remaining docs in this PR or keep compatibility aliases to avoid confusing/broken onboarding instructions.
| "docker:logs": "docker compose logs -f", | |
| "// --- DATABASE ---": "", | |
| "db:clear:auth": "docker compose exec auth-db mongosh authdb --eval \"db.dropDatabase()\"", | |
| "db:clear:twin": "docker compose exec twin-db mongosh twindb --eval \"db.dropDatabase()\"", | |
| "db:clear:notification": "docker compose exec notification-db mongosh notificationdb --eval \"db.dropDatabase()\"", | |
| "db:clear": "npm run db:clear:auth && npm run db:clear:twin && npm run db:clear:notification", | |
| "docker:logs": "docker compose logs -f", | |
| "dev": "npm run docker:dev", | |
| "start": "npm run docker:start", | |
| "stop": "npm run docker:down", | |
| "// --- DATABASE ---": "", | |
| "db:clear:auth": "docker compose exec auth-db mongosh authdb --eval \"db.dropDatabase()\"", | |
| "db:clear:twin": "docker compose exec twin-db mongosh twindb --eval \"db.dropDatabase()\"", | |
| "db:clear:notification": "docker compose exec notification-db mongosh notificationdb --eval \"db.dropDatabase()\"", | |
| "db:clear": "npm run db:clear:auth && npm run db:clear:twin && npm run db:clear:notification", | |
| "clear": "npm run db:clear", |
Close #138