test(e2e): isolate preference-mutating specs from the shared admin user

[steilerDev]

Fixes #1957. Entirely within e2e/ — no production code changed (AC7).

Audit (AC1)

Posted as an issue comment and as the header block of e2e/fixtures/isolatedUser.ts. File list is exactly reproducible with grep -rl "users/me/preferences" e2e/tests/ — 5 files, nothing added, nothing dropped:

Spec	Keys written
dashboard.spec.ts	dashboard.hiddenCards, locale
i18n.spec.ts	locale
i18n-categories.spec.ts	locale (already isolated)
diary-uat-fixes.spec.ts	dashboard.hiddenCards
invoices.spec.ts	table.invoices.columns

e2e/pages/DashboardPage.ts and e2e/pages/InvoicesPage.ts also match the string, but only inside waitForResponse() predicates — they observe the app's request and never issue one. Called out as non-entries.

Mechanism

e2e/fixtures/isolatedUser.ts packages the pattern the three reference sites hand-rolled, as two opt-in options that override Playwright's storageState option so the plain page/context fixtures are already the dedicated user:

• isolatedUserPerWorker — one dedicated user per worker (file scope)
• isolatedUserPerTest — fresh user per test (file or describe scope)

Overriding the option rather than hand-building browser.newContext() keeps Playwright's automatic trace/video/screenshot instrumentation, which the existing hand-rolled helpers silently lose, and means no test-body changes. Verified against node_modules/playwright/lib/index.js that _combinedContextOptions depends on the storageState fixture, so the override genuinely re-points page rather than being a silent no-op.

An auto guard fixture asserts once per test that /api/auth/me reports the dedicated user — so a future silent regression to shared-admin writes fails loudly instead of quietly reverting.

Conversions (AC2, AC4, AC5)

File	Change	Isolated tests
dashboard.spec.ts	isolatedUserPerWorker, file scope; Scenario 6/7's inline loginAsIsolatedDashboardUser deleted as redundant	69
i18n.spec.ts	isolatedUserPerWorker, file scope; serial kept as CPU-contention defence in depth, documented as no longer the isolation mechanism (AC4)	10
diary-uat-fixes.spec.ts	isolatedUserPerTest on the Scenario 3 + 7 describes (AC5)	6
invoices.spec.ts	isolatedUserPerTest on the "Effective Amount" column describe	1

AC3: no spec needed it. Checked route-by-route rather than assumed — server-side requireRole('admin') exists only on /api/users mutations and /api/backups/*; client-side gating is only the Settings sub-nav tabs and work-item note edit/delete for others' notes. Nothing the converted tests touch is gated, so the fixture's default role: 'member' is correct throughout. role: 'admin' is supported for a future case that genuinely needs it.

Two corrections to the issue's premises

1. ON DELETE CASCADE does not clean up the preference row. DELETE /api/users/:id is a soft delete (deactivateUser + destroyUserSessions), so the user and its user_preferences rows survive. Isolation still holds — the account can never be logged into again and e-mails are never reused — but users accumulate, which drove the per-worker/per-test scope decision below.
2. POST /api/auth/login is rate-limited to 20 requests / 15 min per IP, and all workers in a shard share the bucket.

Why per-worker rather than per-test

Per-test provisioning for all 69 dashboard tests would add ~85 users per run against a /settings/users page that renders 100 rows and is scanned row-by-row by edit-user.spec.ts / deactivate-user.spec.ts — arming a future failure — and would push logins toward the 20/15-min cap. Per-worker costs ~15 users and ~1 login per worker; worst realistic per-shard login count is ~10–12 including retries.

Verification available in this sandbox

• npx playwright test --config e2e/playwright.config.ts --list → 2673 tests in 108 files, no load or fixture-pool errors. This validates the whole fixture graph (scope violations, cycles) without containers. Independently re-run.
• npx tsc --noEmit -p e2e/tsconfig.json: 123 errors before, 123 after — all pre-existing (e2e is not covered by npm run typecheck); zero in isolatedUser.ts.
• npx eslint and npx prettier --check clean on every touched file.

Playwright cannot run in this sandbox (browser binaries are network-policy-blocked), so CI is the confirmation — AC6 requires all 16 shards green at whatever shard assignment is in effect.

Residual risks, in order

1. The storage-state handoff from APIRequestContext to BrowserContext — a documented Playwright pattern, but first use in this repo. Would fail loudly for all isolated tests at once. (SECURE_COOKIES is false in the E2E container, so the cookie transfer should work.)
2. 34 dashboard tests now authenticate as member rather than admin. No role-dependent rendering was found on any page they touch, but this is the first thing to re-check if dashboard tests go red.
3. Login rate-limit exhaustion in a pathologically co-located shard (~10–12 of 20 expected); would surface as a clear 429 fixture error.

Kept deliberately: the beforeEach/afterEach preference resets, which remain load-bearing against sequential carry-over within a worker; and the explicit preference DELETEs/PATCHes inside converted tests, now belt-and-braces preconditions on the dedicated user.

[steilerDev]

[product-architect] Architecture review of #1961 (issue #1957).

VERDICT: CHANGES_REQUIRED

The design is right and I could not break the mechanism. The single blocker is AC6: E2E Tests (Shard 14/16) is red on head 950f6c45, and I traced it to a side effect of this PR that nobody flagged — the test.use() opt-ins reshuffle Playwright's shard boundaries suite-wide. Detail below; the fix is small and stays inside e2e/.

---

What I verified independently

The storageState override is genuinely load-bearing, not a no-op. Traced end-to-end in node_modules/playwright/lib/index.js:

• :256 — storageState: [({ contextOptions }, use) => use(contextOptions.storageState), { option: true, box: true }]
• :268/:285/:324-325 — _combinedContextOptions destructures the storageState fixture and sets options.storageState
• :357/:386 — _contextFactory depends on _combinedContextOptions and passes it straight into browser.newContext({ ...videoOptions, ...options })
• :80 — _setupArtifacts also depends on _combinedContextOptions, so the trace/video/screenshot claim holds

This is also the documented Playwright "authenticate in each parallel worker" pattern, so it is the right mechanism, and it is strictly better than the three hand-rolled browser.newContext() reference sites. Agreed with the choice.

The ?? testInfo.project.use.storageState fallback is necessary and correct. Config sets storageState on each project (playwright.config.ts:80, :93, :108), never at top level — and once the option is overridden as a fixture the config value is no longer consulted. Without that line, non-opted-in tests in an importing file would have lost admin auth entirely.

AC1 is exact. grep -rl "users/me/preferences" e2e/tests/ → exactly the 5 audited files. Excluding e2e/pages/DashboardPage.ts:135-140/:179-184 and e2e/pages/InvoicesPage.ts:378-383 is right — all three are waitForResponse() predicates.

And the grep-defined audit has no blind spot, which I checked because a grep for API paths cannot see UI-driven preference writes. The only two in the suite are #languageSelect (i18n.spec.ts) and enableColumn() (invoices.spec.ts) — both converted. Nothing else dismisses a dashboard card or toggles a DataTable column as the shared admin.

AC3 holds — no member/admin risk. Swept all four files route-by-route and assertion-by-assertion. No converted test reaches /settings/users or /settings/backups; no sub-nav tab-list or tab-count assertion; the only menu toHaveCount is dashboard.spec.ts:1065 (Add dropdown, ungated). interceptDashboardApis mocks all six calls loadAllData() issues, and DashboardCard.tsx:47-61 renders heading + dismiss button unconditionally, so no card assertion can fail from a fresh user's empty data. i18n.spec.ts:274-275 is safe: SubNav.tsx:26-34 always renders the landmark and only filters visible !== false, which is set only on the two admin tabs. Nothing in the app is user-scoped — createdBy is only ever leftJoined for display, never a WHERE filter — and there is no greeting/avatar/lastLogin/onboarding surface anywhere in client/src. i18n.spec.ts:362 actually gets stronger: the isolated storage state comes from APIRequestContext.storageState() and carries no localStorage origins at all, so the locale-is-null precondition is now structurally guaranteed rather than dependent on admin.json.

AC4 is satisfied, not relabelled. isolatedUserPerWorker makes the locale row unreachable from anywhere else in the suite; serial's remaining job (not co-scheduling two slow German cold-starts on a 2-vCPU runner) is orthogonal to isolation. Retaining it is fine.

Per-worker is the right scope, and the justification is even stronger than stated: each shard runs its own container and DB (containers/setup.ts in globalSetup), so user accumulation is bounded per shard, not per run — the 100-row /settings/users page is never remotely in reach. Nothing in the converted files needs per-test that got per-worker; both files reset the keys they dirty (dashboard.spec.ts beforeEach, i18n.spec.ts afterEach), which is exactly the constraint per-worker imposes.

Both premise corrections check out. userService.ts:373-377 deactivateUser only sets deactivated_at — soft delete confirmed, the ON DELETE CASCADE claim in the issue is wrong. auth.ts:139 is max: 20, timeWindow: '15 minutes', and rateLimitPlugin.ts:11-23 keys on request.ip, so one bucket per shard.

The guard fixture does close the failure mode it claims to. page.request shares the BrowserContext cookie jar, so /api/auth/me cannot report the dedicated user while the context is still the shared admin — it is not possible for the guard to pass on a broken override. See the non-blocking note on its edges below.

npx playwright test --list → 2673 tests / 108 files, re-run in this worktree; I also ran it against base 6644397c for the shard comparison below.

---

Blocking

B1. AC6 unmet — shard 14 is red, and this PR is the proximate cause via shard redistribution

E2E Tests (Shard 14/16) failed on 950f6c45. One hard failure, on both the first attempt and the retry:

[mobile] › tests/household-items/no-area-filter.spec.ts:194:5 ›
  ?areaId=__none__ shows filtered empty state when no unassigned household items exist (Scenario 4)
Error: expect(locator).toBeVisible() failed
Locator: locator('[class*="emptyState"]').first()
  at e2e/tests/household-items/no-area-filter.spec.ts:216:12

Plus 2 flaky (household-item-edit.spec.ts:40, :69) with page.goto: Timeout 10000ms and apiRequestContext.post: Timeout 10000ms — symptoms of a heavier shard. Base 6644397c was 16/16 green, as were the last eight beta commits.

Root cause — this is not a fixture defect, it is a shard-boundary shift. createTestGroups (node_modules/playwright/lib/runner/index.js:2251) buckets tests by test._workerHash first, and emits groups in bucket insertion order; filterForShard (:2321) then walks that list and slices by cumulative test count. test.use({ isolatedUserPerWorker: ... }) changes the worker hash of the affected tests, so group emission order changes and every shard boundary moves — even though the total is 2673 both before and after.

Confirmed empirically by running --list --shard=N/16 on both trees:

	base 6644397c	head 950f6c45
shard 14	…, diary/diary-uat-fixes.spec.ts, household-items/area-filter.spec.ts, …	same minus diary-uat-fixes.spec.ts, plus household-items/no-area-filter.spec.ts
no-area-filter.spec.ts	shard 15	shard 14

no-area-filter.spec.ts:194 asserts a global precondition — that no household item without an area exists anywhere in the shared DB — while area-filter.spec.ts:185 (itemNoAreaName) and :319 (plannedNoAreaName) create exactly such items and hold them for the length of a long scenario. Under fullyParallel: true in the same shard/container, the ?areaId=__none__ list is non-empty and the empty state never renders. That is the same class of cross-file shared-state race as #1957 itself, just on household items instead of preferences, and it was latent until this PR moved the two files together.

Why this blocks rather than becoming a follow-up: main requires E2E Gates (all shards), so merging as-is guarantees promotion PR #1958 stays blocked — the opposite of this PR's purpose. AC6 is also explicit that all shards must be green at the shard assignment in effect when the fix PR runs, and the assignment in effect is the shifted one.

Required: get 16/16 green on this head. The right fix is in e2e/tests/household-items/no-area-filter.spec.ts Scenario 4 — stop asserting a global "no unassigned items exist" precondition and scope the empty-state assertion to the test's own data (e.g. combine ?areaId=__none__ with a testPrefix search filter so a foreign unassigned item cannot satisfy the list). Do not resolve it by pinning or reordering shards — #1957's Notes rule that out explicitly, and it would only re-arm this for the next redistribution. Either fix it on this branch or land it as a stacked PR first; I have no preference on which, as long as this head is 16/16 before merge.

Also required (cheap, same PR): document the redistribution side effect. Opting a file into a worker-scoped option reshuffles shard membership across the whole suite — that is a non-obvious consequence of an apparently file-local change, and the next person converting a file needs to expect an unrelated shard to go red. One line in the isolatedUser.ts header ("WHICH SCOPE TO PICK" is the natural home) and one in .claude/agent-memory/e2e-test-engineer/isolated-user-fixture.md. The memory file already notes that option values participate in the worker hash and cost worker restarts; what it misses is that they also move shard boundaries.

---

Non-blocking

Listing these as follow-ups; none needs to hold the merge once B1 is green.

• N1. invoices.spec.ts:856 still destructures testPrefix inside the converted describe. testPrefix (e2e/fixtures/auth.ts:26) depends on authenticatedPage, which provisions a second, admin-authenticated browser context that the test never touches. Isolation is not defeated — every assertion runs on page, and testPrefix is consumed purely as a string — but it is wasted setup and a live trap for anyone who later reaches for authenticatedPage in that describe. Worth decoupling testPrefix from authenticatedPage (it only reads testInfo); that is a one-line fixture change affecting many files, so better as its own issue.
• N2. Dead-and-slightly-dangerous fallback at isolatedUser.ts:346. ?? ADMIN_STORAGE_STATE can never fire today (all three real projects set storageState). If a future project deliberately runs unauthenticated, this would silently force admin auth on it. Letting the expression resolve to undefined is safer and loses nothing.
• N3. Guard fixture edges. (a) It is auto and depends on page, so a browser context plus one /api/auth/me is created for every test in an importing file — including tests that a beforeEach immediately test.skip()s. Harmless today (i18n.spec.ts is desktop-only via grep: /@responsive/, so those tests do not exist in the tablet/mobile projects), but it is a general property of the design worth a comment. (b) It only validates the page fixture, so a hand-rolled browser.newContext() inside a converted file is outside its reach — correct by construction today (the only such cases, invoices.spec.ts:781/:803, are in the non-converted Dark mode describe), but the header's "a future silent regression fails loudly" is a touch broader than what is actually enforced. One clause noting the limit would keep the doc honest.
• N4. Login rate-limit headroom is thin and fails opaquely. 20 per 15 min, one request.ip bucket per shard, and a shard run fits inside a single window. My worst-case count for a shard is ~6–12 including retries, so it clears — but there is no guarantee, redistribution can co-locate this with UI-login-heavy specs, and exhaustion surfaces as a 429 inside fixture setup. Follow-up issue: make the login rate limit env-configurable so the E2E container can raise it. That is a production change (server/src/routes/auth.ts), so it should not ride along here.
• N5. search-users.spec.ts:37 searches 'e2e-test', which matches every isolated user's @e2e-test.local address, then requires admin@e2e-test.local on the rendered page. listUsers (userService.ts:292-306) has no ORDER BY, so SQLite returns rowid order and the setup admin (rowid 1) stays on page 1 of 100. Low risk, but this PR adds to that pool on top of i18n-categories.spec.ts / change-password.spec.ts. Follow-up issue at most.
• N6. Doc nit in the fixture header. The admin-gating summary lists client-side note edit/delete but not its server-side counterpart — server/src/routes/notes.ts:127/:152 pass request.user.role === 'admin' into work-item note update/delete. Irrelevant to the converted specs (none opens a work-item detail page), but the header reads as an exhaustive inventory, so it should be one.

---

On scope and the audit itself

No production code changed — AC7 satisfied, and Detect Changes correctly skipped Static Analysis/Test/Trailer Check. AC1, AC2, AC3 (vacuous by verified inspection), AC4, AC5 are all met. The audit in the isolatedUser.ts header is the best artifact in this PR: it names keys per file, states the admin-gating verdict per file with the evidence, explains why the two e2e/pages matches are non-entries, and calls out adjacent hazards it deliberately left out of scope. Keep that standard.

AC6 is the only outstanding criterion, and B1 is what it takes to close it.

[github-actions]

🎉 This PR is included in version 2.13.0-beta.51 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 2.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀