ci(web): Phase P6 — vite build + headless E2E gate on every trunk merge (#1445) #1454

2026-08-27T14:31:51Z

fred commented

2026-08-27 14:31:51 +00:00

Closes #1445 (S4 Phase P6).

What this delivers

PR CI builds the SPA. ci.yml gains a build step (turbo vite build) after test, so a PR that breaks the production bundle fails before merge.
Headless E2E gate on every trunk publish. publish.yml gains an e2e step: it boots the gateway from the built dist on an embedded throwaway PGlite database, then runs the Playwright suite headless against the SPA bundle served by the gateway itself — the same artifact shape production serves (#1407 parity). Both kaniko image publishes now depends_on the e2e step, so a red suite blocks the image.
SPA serving fixes (found while building the gate): serve-spa.ts strips query strings before asset resolution, and hashed assets get cache-control: public, max-age=31536000, immutable via an onSend hook. New serve-spa.e2e.spec.ts (12 tests) covers both, plus the missing-asset 404 below.

Design decisions

Pinned Playwright image mcr.microsoft.com/playwright:v1.58.2-noble: matches the repo's @playwright/test version; browsers preinstalled, no download in CI. Fedora hosts render headless text at zero size (unsupported distro), so the Ubuntu image is also the only reproducible runner.
node_modules reuse, no install in the e2e step: the workspace's install from the earlier step is reused (both glibc). Known risk: better-sqlite3 (native, in the better-auth peer graph) would fail if the install had happened under musl — not the case in this pipeline, but flagging it for reviewers.
e2e placement follows #1411: depends_on: [build, verify, publish-next-npm], and only the kaniko steps gate on it.
Gateway DB isolation: the local-tier PGlite dir is hardcoded to $HOME/.config/mosaic/gateway/pglite (database.module.ts), not cwd. The e2e step therefore launches the gateway with HOME pointed at a fresh mktemp -d so each pipeline run gets a virgin database and the globalSetup bootstrap path is deterministic.
BETTER_AUTH_SECRET is generated per run from /dev/urandom inside the step — the gateway refuses to boot without one; no usable literal lives in the tree.

E2E suite hardening

The pre-existing suite (from #152) was effectively a no-op against the current UI:

loginAs race: callers' test.skip(!url.includes('/chat')) guards read page.url() before the post-login redirect, so 27 of 41 specs silently skipped even on successful login. loginAs now waitForURL(/\/chat/) (swallowed on timeout so the skip guards still work for unseeded envs).
globalSetup seeds an admin and a member through the real /api/bootstrap/* and better-auth admin APIs. better-auth's CSRF check rejects server-side fetches without an Origin header (403 MISSING_OR_NULL_ORIGIN), so setup sends the gateway's own origin.
Stale assertions rewritten to the current command-driven UI (no "new conversation" button; commands panel with /new; brand block is a logo image, not a link; active nav uses font-medium). One test asserting a removed "Active Mission" section was deleted.
Strict-mode/race fixes: level: 1 heading queries (empty-state h2s also matched loose patterns) and .or(...).first() auto-retrying locators instead of non-retrying isVisible().catch().

Verification

Full CI-image rehearsal: the publish.yml e2e step body run verbatim inside playwright:v1.58.2-noble via podman (workspace-mounted, same env). Rounds 5 and 6: 40 passed, 0 failed/skipped/flaky, rc=0 both times.
verify-release mirror test 11/11, test:checkout 41/41, typecheck, lint, format, gateway vitest 829 passed, pnpm build clean — all local.
Playwright run artifacts (test-results/, playwright-report/) added to .gitignore.

Review rounds (heads `54adbd2b`, `f3e1a761`)

Commit 54adbd2b answers the gate-16 model review (REQUEST_CHANGES):

E2E_REQUIRE_SEEDED_AUTH=1 in CI: the suite can no longer pass by skipping — loginAs hard-fails on a missed redirect, skip-guards become live-env-only, globalSetup refuses a pre-populated DB (HOME-isolation regression detector), and the non-admin /admin test is a real authorization assertion. Rehearsal signature: 40 passed, 0 skipped.
ci.yml build now depends_on: [test]: turbo test carries ^build; two concurrent turbo builds on the shared workspace would race (same invariant publish.yml documents for #1411).
Missing /assets/* paths 404 instead of falling through to the SPA page — which the onSend hook would have stamped with a year-long immutable cache-control (real cache-poisoning hole; spec arm added).
Minors: when: *image_build_when on the e2e step, $GATEWAY_PORT health poll with AbortSignal.timeout, generated secret, artifact-path echo on failure, dev-guide "E2E Gate" section.

Commit f3e1a761 is out of P6 scope on its face and is called out deliberately: CI 2891 on 54adbd2b redded @mosaicstack/web#test on a pre-existing flake — the settings tabs' uncleaned 2s setSaveState('idle') timers firing after jsdom teardown (all 292 tests passed; vitest fails the run on the unhandled error). A pipeline re-run is not available to this seat, and the root-cause fix is right regardless: one useSavedBadgeReset effect hook with clearTimeout cleanup replaces the three raw timers in apps/web/src/spa/pages/settings.tsx.

Deferred follow-ups (agreed non-blocking across both reviews): e2e tree outside typecheck/lint coverage; corepack network fetch in the e2e step; @playwright/test caret vs pinned image tag; percent-encoded /assets variants; retries: 2 flake masking; publish-gate structure assertion for the e2e when/depends_on coupling; vitest teardown guard for the post-teardown-timer failure class.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XnH8KTL4PNTpEnpubcmiRn

Closes #1445 (S4 Phase P6). ## What this delivers 1. **PR CI builds the SPA.** `ci.yml` gains a `build` step (turbo `vite build`) after `test`, so a PR that breaks the production bundle fails before merge. 2. **Headless E2E gate on every trunk publish.** `publish.yml` gains an `e2e` step: it boots the gateway from the built `dist` on an embedded throwaway PGlite database, then runs the Playwright suite headless against the SPA bundle served by the gateway itself — the same artifact shape production serves (#1407 parity). Both kaniko image publishes now `depends_on` the e2e step, so a red suite blocks the image. 3. **SPA serving fixes** (found while building the gate): `serve-spa.ts` strips query strings before asset resolution, and hashed assets get `cache-control: public, max-age=31536000, immutable` via an onSend hook. New `serve-spa.e2e.spec.ts` (12 tests) covers both, plus the missing-asset 404 below. ## Design decisions - **Pinned Playwright image** `mcr.microsoft.com/playwright:v1.58.2-noble`: matches the repo's `@playwright/test` version; browsers preinstalled, no download in CI. Fedora hosts render headless text at zero size (unsupported distro), so the Ubuntu image is also the only reproducible runner. - **node_modules reuse, no install in the e2e step**: the workspace's install from the earlier step is reused (both glibc). Known risk: `better-sqlite3` (native, in the better-auth peer graph) would fail if the install had happened under musl — not the case in this pipeline, but flagging it for reviewers. - **e2e placement** follows #1411: `depends_on: [build, verify, publish-next-npm]`, and only the kaniko steps gate on it. - **Gateway DB isolation**: the local-tier PGlite dir is hardcoded to `$HOME/.config/mosaic/gateway/pglite` (database.module.ts), not cwd. The e2e step therefore launches the gateway with `HOME` pointed at a fresh `mktemp -d` so each pipeline run gets a virgin database and the globalSetup bootstrap path is deterministic. - `BETTER_AUTH_SECRET` is generated per run from `/dev/urandom` inside the step — the gateway refuses to boot without one; no usable literal lives in the tree. ## E2E suite hardening The pre-existing suite (from #152) was effectively a no-op against the current UI: - **loginAs race**: callers' `test.skip(!url.includes('/chat'))` guards read `page.url()` before the post-login redirect, so 27 of 41 specs silently skipped even on successful login. `loginAs` now `waitForURL(/\/chat/)` (swallowed on timeout so the skip guards still work for unseeded envs). - **globalSetup** seeds an admin and a member through the real `/api/bootstrap/*` and better-auth admin APIs. better-auth's CSRF check rejects server-side fetches without an `Origin` header (403 `MISSING_OR_NULL_ORIGIN`), so setup sends the gateway's own origin. - **Stale assertions rewritten** to the current command-driven UI (no "new conversation" button; commands panel with `/new`; brand block is a logo image, not a link; active nav uses `font-medium`). One test asserting a removed "Active Mission" section was deleted. - **Strict-mode/race fixes**: `level: 1` heading queries (empty-state h2s also matched loose patterns) and `.or(...).first()` auto-retrying locators instead of non-retrying `isVisible().catch()`. ## Verification - Full CI-image rehearsal: the publish.yml e2e step body run verbatim inside `playwright:v1.58.2-noble` via podman (workspace-mounted, same env). Rounds 5 and 6: **40 passed, 0 failed/skipped/flaky, rc=0 both times**. - verify-release mirror test 11/11, `test:checkout` 41/41, typecheck, lint, format, gateway vitest 829 passed, `pnpm build` clean — all local. - Playwright run artifacts (`test-results/`, `playwright-report/`) added to `.gitignore`. ## Review rounds (heads 54adbd2b, f3e1a761) Commit 54adbd2b answers the gate-16 model review (REQUEST_CHANGES): - **E2E_REQUIRE_SEEDED_AUTH=1 in CI**: the suite can no longer pass by skipping — `loginAs` hard-fails on a missed redirect, skip-guards become live-env-only, `globalSetup` refuses a pre-populated DB (HOME-isolation regression detector), and the non-admin `/admin` test is a real authorization assertion. Rehearsal signature: 40 passed, 0 skipped. - **ci.yml `build` now `depends_on: [test]`**: turbo `test` carries `^build`; two concurrent turbo builds on the shared workspace would race (same invariant publish.yml documents for #1411). - **Missing `/assets/*` paths 404** instead of falling through to the SPA page — which the onSend hook would have stamped with a year-long immutable cache-control (real cache-poisoning hole; spec arm added). - Minors: `when: *image_build_when` on the e2e step, `$GATEWAY_PORT` health poll with `AbortSignal.timeout`, generated secret, artifact-path echo on failure, dev-guide "E2E Gate" section. Commit f3e1a761 is **out of P6 scope on its face and is called out deliberately**: CI 2891 on 54adbd2b redded `@mosaicstack/web#test` on a pre-existing flake — the settings tabs' uncleaned 2s `setSaveState('idle')` timers firing after jsdom teardown (all 292 tests passed; vitest fails the run on the unhandled error). A pipeline re-run is not available to this seat, and the root-cause fix is right regardless: one `useSavedBadgeReset` effect hook with `clearTimeout` cleanup replaces the three raw timers in `apps/web/src/spa/pages/settings.tsx`. Deferred follow-ups (agreed non-blocking across both reviews): e2e tree outside typecheck/lint coverage; corepack network fetch in the e2e step; `@playwright/test` caret vs pinned image tag; percent-encoded `/assets` variants; `retries: 2` flake masking; publish-gate structure assertion for the e2e `when`/`depends_on` coupling; vitest teardown guard for the post-teardown-timer failure class. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01XnH8KTL4PNTpEnpubcmiRn

fred added 1 commit 2026-08-27 14:31:51 +00:00

ci(web): Phase P6 — vite build in PR CI + headless E2E gate on trunk publishes (#1445 )

ci/woodpecker/pr/ci Pipeline was successful

Details

311b4dda59

- ci.yml: build step (vite build via turbo) runs on every PR pipeline after test
- publish.yml: e2e step boots the gateway from built dist (HOME/cwd-isolated
  throwaway PGlite) and runs the Playwright suite headless inside
  mcr.microsoft.com/playwright:v1.58.2-noble against the SPA bundle served
  exactly as production serves it; both kaniko publishes now gate on e2e
- serve-spa.ts: strip query strings before asset resolution; immutable
  cache-control for hashed assets (onSend hook); e2e spec covers both
- e2e suite hardened: globalSetup seeds admin+member through real
  bootstrap/better-auth APIs (Origin header for CSRF), loginAs waits for the
  post-login redirect (fixes 27-skipped race), stale #152-era assertions
  rewritten to the current command-driven UI, strict-mode violations fixed
  with level-1 heading queries and .or() auto-retrying locators
- verify-release mirrors the new build stage; playwright artifacts ignored

rev-code-01 approved these changes 2026-08-27 14:35:57 +00:00

Dismissed

rev-code-01 left a comment

APPROVED — rev-code-01, pinned to head 311b4dda599e6d9133169f6339ce7adbe6ef2e39 (re-verified unmoved immediately before posting).

Scope: 15 files, +451/−77 — the P6 CI gate. Verified independently:

serve-spa.e2e.spec.ts is exactly my #1453 SF1 ask, closed: 11 tests pinning all six serving behaviors I listed (deep-link fallback, declared-route precedence, backend JSON 404s, exact static serving, unset-disables, bad-dir-fails-boot) plus my #1453 N1 query-string edge and the new cache semantics. Run by me: 11/11 (embedded in the gateway suite's 829 pass — 818 + these 11). The spec builds a real Nest+Fastify app mirroring main.ts mount order against a fixture dist dir — the right harness.
serve-spa fixes correct: isBackendPath now splits the query string before matching (closes /api?x=1); immutable caching for /assets/ via an onSend hook with the documented reason (@fastify/static applies its own computed cache-control after setHeaders, which would override); non-asset paths keep revalidation semantics — asserted both directions in the spec.
CI wiring sound: ci.yml gains the canonical build mirror after typecheck; publish.yml's e2e step boots the real gateway dist on the embedded PGlite path with HOME and cwd isolation (matching database.module.ts's HOME-keyed PGlite dir), a 90s readiness loop that also detects gateway death and dumps the log, depends_on: [build, verify, publish-next-npm] honoring the #1411 serialization invariant, and both kaniko publishes gate on e2e — a red bundle never ships an image. Playwright image pinned v1.58.2-noble matching @playwright/test ^1.58.2 in the lockfile (bump-together note in the comment). The BETTER_AUTH_SECRET literal is a documented throwaway for the per-pipeline ephemeral local DB — not a credential (verified: value is inert prose, DB is mktemp-HOME-scoped PGlite).
E2E hardening verified by read: globalSetup seeds through the real /api/bootstrap/* + better-auth admin APIs and fails loud on any seeding failure ("a gate whose suites silently skip would pass while proving nothing") while staying no-op against populated environments; the Origin-header CSRF workaround matches better-auth's documented check; loginAs waitForURL-swallow preserves the skip guards; playwright.config flips to the gateway-served bundle (P5 parity) with CI list reporter and single worker.
verify-release mirror updated correctly: build moved into the enforced pnpm-stage mirror set (read the assertion — it now discriminates a ci.yml missing the build step); checkout suite 41/41.
Housekeeping: supertest + @types/supertest added as devDeps; eslint's stale **/.next/** ignore dropped (my #1453 delta N1); Playwright artifacts ignored.
Gates run by me at this head (fresh worktree, build-first): build rc=0; gateway suite 829 pass / 45 skip, incl. serve-spa 11/11; typecheck 45/45; lint 25/25; format rc=0; test:checkout 41/41. (First suite run failed 66 files with Failed to resolve entry for @mosaicstack/db — the known fresh-worktree missing-build state, not the PR; resolved by building workspace deps first, consistent with my #1434 note.)

Non-blocking:

[N1] retries: 2 in CI can mask a first-run flake as a green gate. The rehearsal (two rounds, 0 flaky) is good evidence today; if the gate ever passes only on retry, treat the retry count as consumed and fix the flake rather than absorbing it.
[N2] The image-tag ↔ lockfile @playwright/test coupling is comment-enforced; a structure-test assertion (same style as the build-mirror check) would make the drift mechanically caught. Future nicety.

CI note: 2890 running on this head at review time — its build step and (post-merge) the trunk e2e step are this PR's own verification. Merge waits on 2890 terminal green.

**APPROVED — rev-code-01, pinned to head `311b4dda599e6d9133169f6339ce7adbe6ef2e39`** (re-verified unmoved immediately before posting). Scope: 15 files, +451/−77 — the P6 CI gate. Verified independently: 1. **`serve-spa.e2e.spec.ts` is exactly my #1453 SF1 ask, closed:** 11 tests pinning all six serving behaviors I listed (deep-link fallback, declared-route precedence, backend JSON 404s, exact static serving, unset-disables, bad-dir-fails-boot) plus my #1453 N1 query-string edge and the new cache semantics. **Run by me: 11/11** (embedded in the gateway suite's 829 pass — 818 + these 11). The spec builds a real Nest+Fastify app mirroring main.ts mount order against a fixture dist dir — the right harness. 2. **serve-spa fixes correct:** `isBackendPath` now splits the query string before matching (closes `/api?x=1`); immutable caching for `/assets/` via an `onSend` hook with the documented reason (`@fastify/static` applies its own computed cache-control after `setHeaders`, which would override); non-asset paths keep revalidation semantics — asserted both directions in the spec. 3. **CI wiring sound:** ci.yml gains the canonical `build` mirror after typecheck; publish.yml's `e2e` step boots the real gateway `dist` on the embedded PGlite path with HOME **and** cwd isolation (matching `database.module.ts`'s HOME-keyed PGlite dir), a 90s readiness loop that also detects gateway death and dumps the log, `depends_on: [build, verify, publish-next-npm]` honoring the #1411 serialization invariant, and **both kaniko publishes gate on `e2e`** — a red bundle never ships an image. Playwright image pinned `v1.58.2-noble` matching `@playwright/test ^1.58.2` in the lockfile (bump-together note in the comment). The `BETTER_AUTH_SECRET` literal is a documented throwaway for the per-pipeline ephemeral local DB — not a credential (verified: value is inert prose, DB is `mktemp`-HOME-scoped PGlite). 4. **E2E hardening verified by read:** globalSetup seeds through the real `/api/bootstrap/*` + better-auth admin APIs and **fails loud on any seeding failure** ("a gate whose suites silently skip would pass while proving nothing") while staying no-op against populated environments; the Origin-header CSRF workaround matches better-auth's documented check; `loginAs` waitForURL-swallow preserves the skip guards; playwright.config flips to the gateway-served bundle (P5 parity) with CI `list` reporter and single worker. 5. **verify-release mirror updated correctly:** `build` moved into the enforced pnpm-stage mirror set (read the assertion — it now discriminates a ci.yml missing the build step); checkout suite 41/41. 6. **Housekeeping:** supertest + @types/supertest added as devDeps; eslint's stale `**/.next/**` ignore dropped (my #1453 delta N1); Playwright artifacts ignored. 7. **Gates run by me at this head (fresh worktree, build-first):** build rc=0; gateway suite **829 pass / 45 skip, incl. serve-spa 11/11**; typecheck 45/45; lint 25/25; format rc=0; test:checkout 41/41. (First suite run failed 66 files with `Failed to resolve entry for @mosaicstack/db` — the known fresh-worktree missing-build state, not the PR; resolved by building workspace deps first, consistent with my #1434 note.) Non-blocking: - **[N1]** `retries: 2` in CI can mask a first-run flake as a green gate. The rehearsal (two rounds, 0 flaky) is good evidence today; if the gate ever passes only on retry, treat the retry count as consumed and fix the flake rather than absorbing it. - **[N2]** The image-tag ↔ lockfile `@playwright/test` coupling is comment-enforced; a structure-test assertion (same style as the build-mirror check) would make the drift mechanically caught. Future nicety. CI note: 2890 running on this head at review time — its `build` step and (post-merge) the trunk `e2e` step are this PR's own verification. Merge waits on 2890 terminal green.

fred added 1 commit 2026-08-27 14:53:03 +00:00

review fixes: seeded-auth hard gate, ci.yml build serialization, assets 404

ci/woodpecker/pr/ci Pipeline failed

Details

54adbd2b3d

- M1: E2E_REQUIRE_SEEDED_AUTH=1 in the CI e2e step makes login failures hard
  failures (loginAs throws, guards disabled, globalSetup refuses a pre-populated
  DB); auth.spec redirect test asserts outright under the flag; non-admin
  /admin test is now a real authorization assertion
- M2: ci.yml build step depends_on test — never two concurrent turbo builds
  on the shared workspace
- M3: unknown /assets/* paths 404 from the SPA catch-all instead of serving
  index.html with an immutable cache header; spec arm added
- minors: e2e step gets when: *image_build_when, health poll uses
  GATEWAY_PORT + AbortSignal.timeout, BETTER_AUTH_SECRET generated per run
  (no literal in tree), failure echoes artifact path, dev-guide documents the
  gate, stale verify-release comment fixed

fred dismissed rev-code-01's review 2026-08-27 14:53:04 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

rev-code-01 approved these changes 2026-08-27 15:00:22 +00:00

Dismissed

rev-code-01 left a comment

APPROVED (delta re-review) — rev-code-01, pinned to head 54adbd2b3d78c6b85e346e48b988e6ca1901ec98 (sole parent = my reviewed 311b4dda, review id 338; head re-verified unmoved immediately before posting).

Delta commit read in full; every fix verified:

M1 (seeded-auth hard gate) — correct and complete. E2E_REQUIRE_SEEDED_AUTH=1 set in the CI e2e step; loginAs throws on missed redirect under the flag (wait not swallowed); globalSetup REFUSES a pre-populated DB under the flag (isolation-regression detection, not skip-and-pass); I swept all nine test.skip guard sites — every one is now !REQUIRE_SEEDED_AUTH && … (admin/chat/navigation×2/projects/settings/auth-redirect), so the flag disables every skip path; auth.spec asserts outright under the flag. The live-env affordance survives with the flag unset. This closes the strongest version of my N1 concern (a login regression could previously go green via universal skips — now structurally impossible in CI).
M2 (ci.yml serialization) — correct. build now depends_on: test (not typecheck): turbo gives test a ^build dependency, so concurrent step runs would race two turbo builds on the shared workspace dist/cache with no cross-process locking — the same invariant publish.yml documents for #1411. Right fix, right comment.
M3 (assets 404) — correct, real cache-poisoning hole closed. The catch-all now 404s /assets* paths instead of serving index.html — the stale-index.html browser would otherwise have cached an HTML body under a year-long immutable header. Spec arm added asserting 404 + no-immutable + no-SPA-body, including the query-string variant. Run by me: serve-spa spec now 12/12 (was 11), gateway suite 830 pass / 45 skip (829+1).
Minors all verified: e2e step carries when: *image_build_when (same filter as the steps it gates; skipped-dependency semantics keep docs-only main merges free of the browser suite); health poll uses $GATEWAY_PORT + AbortSignal.timeout(2000); BETTER_AUTH_SECRET generated per run from /dev/urandom — no literal in the tree at all now (better than the disclosed-placeholder state I previously accepted); failure output names the artifact path; dev-guide documents the gate with a local repro recipe (Fedora-headless caveat included); the stale verify-release comment corrected.
Gates at this head, run by me (fresh worktree, build-first): build rc=0; gateway 830/45 incl. serve-spa 12/12; typecheck 45/45; lint 25/25; format rc=0; checkout 41/41.

N2 from my id-338 review (image-tag ↔ lockfile coupling as a structure assertion) remains the acknowledged follow-up, unchanged by this delta.

CI note: 2890 (previous head) terminal success; 2891 running on this head — merge waits on 2891 terminal green.

**APPROVED (delta re-review) — rev-code-01, pinned to head `54adbd2b3d78c6b85e346e48b988e6ca1901ec98`** (sole parent = my reviewed `311b4dda`, review id 338; head re-verified unmoved immediately before posting). Delta commit read in full; every fix verified: 1. **M1 (seeded-auth hard gate) — correct and complete.** `E2E_REQUIRE_SEEDED_AUTH=1` set in the CI e2e step; `loginAs` throws on missed redirect under the flag (wait not swallowed); globalSetup REFUSES a pre-populated DB under the flag (isolation-regression detection, not skip-and-pass); I swept all nine `test.skip` guard sites — every one is now `!REQUIRE_SEEDED_AUTH && …` (admin/chat/navigation×2/projects/settings/auth-redirect), so the flag disables every skip path; auth.spec asserts outright under the flag. The live-env affordance survives with the flag unset. This closes the strongest version of my N1 concern (a login regression could previously go green via universal skips — now structurally impossible in CI). 2. **M2 (ci.yml serialization) — correct.** `build` now `depends_on: test` (not typecheck): turbo gives `test` a `^build` dependency, so concurrent step runs would race two turbo builds on the shared workspace dist/cache with no cross-process locking — the same invariant publish.yml documents for #1411. Right fix, right comment. 3. **M3 (assets 404) — correct, real cache-poisoning hole closed.** The catch-all now 404s `/assets*` paths instead of serving index.html — the stale-index.html browser would otherwise have cached an HTML body under a year-long immutable header. Spec arm added asserting 404 + no-immutable + no-SPA-body, including the query-string variant. **Run by me: serve-spa spec now 12/12** (was 11), gateway suite **830 pass / 45 skip** (829+1). 4. **Minors all verified:** e2e step carries `when: *image_build_when` (same filter as the steps it gates; skipped-dependency semantics keep docs-only main merges free of the browser suite); health poll uses `$GATEWAY_PORT` + `AbortSignal.timeout(2000)`; `BETTER_AUTH_SECRET` generated per run from `/dev/urandom` — no literal in the tree at all now (better than the disclosed-placeholder state I previously accepted); failure output names the artifact path; dev-guide documents the gate with a local repro recipe (Fedora-headless caveat included); the stale verify-release comment corrected. 5. **Gates at this head, run by me (fresh worktree, build-first):** build rc=0; gateway **830/45 incl. serve-spa 12/12**; typecheck 45/45; lint 25/25; format rc=0; checkout 41/41. N2 from my id-338 review (image-tag ↔ lockfile coupling as a structure assertion) remains the acknowledged follow-up, unchanged by this delta. CI note: 2890 (previous head) terminal **success**; 2891 running on this head — merge waits on 2891 terminal green.

fred added 1 commit 2026-08-27 15:07:36 +00:00

fix(web): clear the saved-badge reset timer on unmount

ci/woodpecker/pr/ci Pipeline was successful

Details

f3e1a761aa

CI 2891 failed on an unhandled ReferenceError: the 2s
setTimeout(() => setSaveState('idle')) in the settings tabs fired after
jsdom teardown. Replace the three uncleaned timers with one
useSavedBadgeReset effect hook that clears on unmount.

fred dismissed rev-code-01's review 2026-08-27 15:07:37 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

rev-code-01 approved these changes 2026-08-27 15:09:44 +00:00

rev-code-01 left a comment

APPROVED (delta re-review) — rev-code-01, pinned to head f3e1a761aafdb0c8995ec60ad2b60f7b33909628 (sole parent = my reviewed 54adbd2b, review id 339; head re-verified unmoved immediately before posting).

Single-file delta (+13/−3, settings.tsx) read in full — the flake root-cause fix:

The hook is the correct React pattern. useSavedBadgeReset arms the 2s reset only when saveState === 'saved', returns a clearTimeout cleanup, and re-runs on state change — so the timer never outlives the tab (unmount clears it; a re-save replaces it). All three raw setTimeout call sites (Profile/Appearance/Notifications tabs) removed, hook mounted at each; zero raw timers remain in the file (grep); useEffect already imported.
This is the root-cause class my N1 stance asks for: the CI 2891 failure is itself the red control — the raw timer fired into torn-down jsdom (window is not defined, unhandled) while all 292 tests passed; the fix removes the timer at teardown by construction (the spec's afterEach unmount triggers the cleanup). The suite carries no explicit timer assertion — the discriminating evidence is 2891-red → suite-green-with-no-banner, plus 2892 (running) as the CI-side confirmation.
Gates at this head, run by me: web build rc=0; web suite 292/292 with zero unhandled-banner hits (settings spec 4/4); web tsc --noEmit clean; repo format rc=0.

CI note: 2891 failed at the test step exactly as described; 2892 running on this head — merge waits on 2892 terminal green.

**APPROVED (delta re-review) — rev-code-01, pinned to head `f3e1a761aafdb0c8995ec60ad2b60f7b33909628`** (sole parent = my reviewed `54adbd2b`, review id 339; head re-verified unmoved immediately before posting). Single-file delta (+13/−3, `settings.tsx`) read in full — the flake root-cause fix: 1. **The hook is the correct React pattern.** `useSavedBadgeReset` arms the 2s reset only when `saveState === 'saved'`, returns a `clearTimeout` cleanup, and re-runs on state change — so the timer never outlives the tab (unmount clears it; a re-save replaces it). All three raw `setTimeout` call sites (Profile/Appearance/Notifications tabs) removed, hook mounted at each; zero raw timers remain in the file (grep); `useEffect` already imported. 2. **This is the root-cause class my N1 stance asks for:** the CI 2891 failure is itself the red control — the raw timer fired into torn-down jsdom (`window is not defined`, unhandled) while all 292 tests passed; the fix removes the timer at teardown by construction (the spec's `afterEach` unmount triggers the cleanup). The suite carries no explicit timer assertion — the discriminating evidence is 2891-red → suite-green-with-no-banner, plus 2892 (running) as the CI-side confirmation. 3. **Gates at this head, run by me:** web build rc=0; web suite **292/292 with zero unhandled-banner hits** (settings spec 4/4); web `tsc --noEmit` clean; repo format rc=0. CI note: 2891 failed at the test step exactly as described; 2892 running on this head — merge waits on 2892 terminal green.

fred merged commit e605c83b27 into next

2026-08-27 15:26:11 +00:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mosaicstack/stack#1454