From 9187e158e4573838e55742de40dfa25b3e7f46fa Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 3 Jul 2026 00:50:30 +0800 Subject: [PATCH 1/3] fix(shell): stop mount rAF from wiping the forms-detail composer on CI WebKit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared search shell (GlobalMockupSearchShellClient) re-synced its mode + query from the URL inside a mount requestAnimationFrame. On a no-query detail route like /forms/transport-crisis-form the deferred frame could land just after a programmatic/user fill and reset the controlled input to empty. On slow single-CPU CI Linux WebKit that reliably lost the value: the composer stayed focused-but-empty, the "Search forms" submit stayed disabled, and the search never routed — so ui-tools.spec.ts:264 was skipped on WebKit (PR #182/#186). Replace the rAF sync with a URL-gated one: seed the composer mode/query from the URL at initial state, and re-sync only when the search string actually changes (a real navigation), tracked via lastSyncedSearchParamsRef. Because typing never changes the URL and the initial mount is a no-op, no deferred frame can clobber an in-progress fill regardless of browser timing. This retires the interim isDetailPage / previousUrlHadQueryRef special-casing from PR #186. With the race fixed at the source, remove the `if (browserName === "webkit")` early-return (and its comment) from ui-tools.spec.ts:264 so all three browsers verify the full submit-and-route wiring; the fill-and-submit toPass harness stays as defensive cross-browser navigation-timing cover. Update docs/process-hardening.md to record the resolution. Co-Authored-By: Claude Opus 4.8 --- docs/process-hardening.md | 5 +- .../global-mockup-search-shell.tsx | 71 ++++++++----------- tests/ui-tools.spec.ts | 24 ++----- 3 files changed, 40 insertions(+), 60 deletions(-) diff --git a/docs/process-hardening.md b/docs/process-hardening.md index 49b18aecf3..23c0d905c0 100644 --- a/docs/process-hardening.md +++ b/docs/process-hardening.md @@ -89,10 +89,11 @@ This document turns the current process review into phased, durable repo practic - Making the dashboard and document viewer client-only via `dynamic(..., { ssr: false })` (PRs #144/#147) meant "page loaded" no longer implies "app mounted". Firefox/WebKit paint the client chunk later than Chromium, so three release-browser-matrix specs raced and failed on `main` while Chromium stayed green. All three were test-timing gaps, not product regressions — the app renders correctly in every browser. - `tests/ui-overlap.spec.ts`: `gotoHome` waited on `networkidle` (Playwright discourages it) and then measured the header, which had not mounted yet — every failure was `header#search not found` (count 0), never a real overlap. Now waits for `header#search` to be visible before measuring. - - `tests/ui-tools.spec.ts` (forms detail → shared search): the shell re-syncs its query from the URL on mount via `requestAnimationFrame`, which on Firefox/WebKit can land just after a programmatic `fill` and wipe the value (button stays disabled) or drop the submit before the router navigates. The fill-and-submit now runs as one `toPass` unit that retries until the search routes. Root-cause fix: the `requestAnimationFrame` effect in `GlobalMockupSearchShellClient` now skips the `setQuery("")` reset on detail pages (where the URL carries no `q`/`query` param), so the programmatic fill is never wiped. + - `tests/ui-tools.spec.ts` (forms detail → shared search): the shell re-synced its query from the URL on mount via `requestAnimationFrame`, which on Firefox/WebKit could land just after a programmatic `fill` and wipe the value (button stays disabled) or drop the submit before the router navigates. The fill-and-submit runs as one `toPass` unit that retries until the search routes. **Root-cause fix (see the 2026-07-03 follow-up below):** `GlobalMockupSearchShellClient` no longer uses a `requestAnimationFrame` sync at all — it seeds the composer mode/query from the URL and re-syncs only when the search string actually changes, so a mount-time frame can never wipe an in-progress fill. - `tests/ui-stress.spec.ts` (desktop evidence panel): the evidence `
` is opened by focusing its `` and pressing Enter; in CI WebKit the key event could fire before focus landed, so it never toggled. Now asserts `toBeFocused()` before pressing Enter. - Rule of thumb for these client-only surfaces: never gate an interaction on `networkidle` or a bare `goto`. Wait for the specific mounted element, and wrap fill→submit→navigate races in `toPass` (the same idiom `openAppModeMenu`/`openDailyActions` already use). The `verify` + `ui-smoke` PR gates run Chromium only, so Firefox/WebKit-specific races surface solely in the gated `release-browser-matrix` (main/release/dispatch/schedule) — keep that job green rather than letting these re-accumulate. -- **Post-merge outcome (PR #178):** `ui-overlap` and `ui-stress` fixes verified green in CI WebKit. `ui-tools.spec.ts:264` (forms-detail search) still fails on **CI WebKit only** — the composer input stays focused-but-empty and the submit disabled across the full retry, and it does **not** reproduce on local WebKit, so it can't be iterated locally. Ruled out: the inline `availableModeIds={["forms"]}` arrays in the `forms`/`services`/`favourites` layouts churning the effect (those layouts are Server Components, so the ref is stable). On WebKit the test now runs its **structural half** (the detail page renders inside the shell with the Forms composer present) and returns before the known-broken **submit-and-route half** (`if (browserName === "webkit") return;`); Chromium + Firefox still verify the full wiring. The root-cause fix (shell mount `requestAnimationFrame` query-sync) is deferred and needs CI-based iteration — removing that WebKit early-return is its exit criterion. +- **Post-merge outcome (PR #178):** `ui-overlap` and `ui-stress` fixes verified green in CI WebKit. `ui-tools.spec.ts:264` (forms-detail search) still failed on **CI WebKit only** — the composer input stayed focused-but-empty and the submit disabled across the full retry, and it did **not** reproduce on local WebKit, so it couldn't be iterated locally. PR #186 added an interim `isDetailPage` guard to the mount `requestAnimationFrame` but stayed `[WIP]`, keeping a `if (browserName === "webkit") return;` early-return that ran only the **structural half** on WebKit. +- **Follow-up resolution (2026-07-03):** removed the `requestAnimationFrame` mount sync in `GlobalMockupSearchShellClient` entirely. The composer mode/query are now seeded from the URL at initial state, and a `lastSyncedSearchParamsRef` gates the re-sync effect so it fires **only when the search string actually changes** (a real navigation). Because typing never changes the URL and the initial mount is a no-op, no deferred frame can wipe an in-progress `fill` on a no-query detail route — the CI-WebKit-only race. This also retired the `isDetailPage`/`previousUrlHadQueryRef` special-casing from PR #186. With the race fixed at the source, the WebKit early-return and its comment were deleted from `ui-tools.spec.ts:264`, so all three browsers now verify the full submit-and-route wiring. Ruled out earlier (and still true): the inline `availableModeIds={["forms"]}` arrays are stable because those layouts are Server Components. ## Suspense fallback must not re-render page children (2026-07-02) diff --git a/src/components/clinical-dashboard/global-mockup-search-shell.tsx b/src/components/clinical-dashboard/global-mockup-search-shell.tsx index 0ccb26c9cc..7907da8c04 100644 --- a/src/components/clinical-dashboard/global-mockup-search-shell.tsx +++ b/src/components/clinical-dashboard/global-mockup-search-shell.tsx @@ -91,9 +91,21 @@ function GlobalMockupSearchShellClient({ const requestedQuery = (searchParams.get("q") ?? searchParams.get("query") ?? "").trim(); const requestedMode = searchParams.get("mode"); const searchParamString = searchParams.toString(); + // Mode resolved from the URL (?mode=), falling back to this shell's default when + // the param is missing, unknown, or not offered here. Seeds the initial mode and + // re-syncs it after a navigation. + const resolvedSearchMode = + isAppModeId(requestedMode) && + isAppModeVisible(requestedMode) && + (!availableModeIds?.length || availableModeIds.includes(requestedMode)) + ? requestedMode + : initialSearchMode; const [query, setQuery] = useState(requestedQuery); - const previousUrlHadQueryRef = useRef(currentUrlHasQuery); - const [searchMode, setSearchMode] = useState(initialSearchMode); + // The search string we last synced into local state, so the effect below only + // reacts to genuine navigations. Seeded with the current string so the initial + // mount is a no-op — the state above is already derived from the URL. + const lastSyncedSearchParamsRef = useRef(searchParamString); + const [searchMode, setSearchMode] = useState(resolvedSearchMode); const [queryMode, setQueryMode] = useState("auto"); const [scopeFilters, setScopeFilters] = useState({}); const [mobileMenuOpen, setMobileMenuOpen] = useState(false); @@ -104,12 +116,6 @@ function GlobalMockupSearchShellClient({ const { theme, toggleTheme } = useTheme(); const auth = useAuthSession(); const sidebarIdentity = useMemo(() => deriveSidebarIdentity(auth.session?.user.email), [auth.session?.user.email]); - const dashboardSearchMode = - isAppModeId(requestedMode) && - isAppModeVisible(requestedMode) && - (!availableModeIds?.length || availableModeIds.includes(requestedMode)) - ? requestedMode - : initialSearchMode; const shouldRenderDashboardSearch = requestedRun && requestedQuery.length > 0; const isFormsOnlyShell = availableModeIds?.length === 1 && availableModeIds[0] === "forms"; const isStandaloneModeHome = @@ -119,41 +125,24 @@ function GlobalMockupSearchShellClient({ (searchMode === "favourites" && pathname === "/favourites") || (searchMode === "differentials" && pathname === "/differentials")); const isDifferentialPresentationWorkflow = pathname.startsWith("/differentials/presentations"); - // True when on a sub-route of a mode home (e.g. /forms/transport-crisis-form, - // /services/13yarn) rather than the mode home itself (/forms, /services). - const isDetailPage = - /^\/(forms|services|favourites)\/.+/.test(pathname) || /^\/differentials\/diagnoses\/.+/.test(pathname); useEffect(() => { - const frame = window.requestAnimationFrame(() => { - const params = new URLSearchParams(window.location.search); - const requestedMode = params.get("mode"); - const nextMode = - isAppModeId(requestedMode) && - isAppModeVisible(requestedMode) && - (!availableModeIds?.length || availableModeIds.includes(requestedMode)) - ? requestedMode - : initialSearchMode; - setSearchMode(nextMode); + // Re-derive the mode and query from the URL, but only when the search string + // actually changes (a real navigation). Reacting on every render — as the old + // requestAnimationFrame sync effectively did — let a deferred frame land after + // a programmatic/user fill and wipe the controlled input; on slow CI WebKit + // that raced the forms-detail composer to empty (input focused-but-empty, + // submit stuck disabled). Typing never changes the URL, so a URL-gated sync + // cannot clobber in-progress input, and the initial mount is skipped entirely + // because the state above is already seeded from the URL. + if (lastSyncedSearchParamsRef.current === searchParamString) return; + lastSyncedSearchParamsRef.current = searchParamString; - const urlHasQuery = params.has("q") || params.has("query"); - const hadQueryBeforeThisSync = previousUrlHadQueryRef.current; - previousUrlHadQueryRef.current = urlHasQuery; - if (urlHasQuery) { - // Sync the controlled query state from the URL query param. - const requestedQuery = (params.get("q") ?? params.get("query"))?.trim(); - setQuery(requestedQuery ?? ""); - } else if (!isDetailPage || hadQueryBeforeThisSync) { - // On no-query routes, clear any stale URL-derived query. Initial detail - // page mounts still skip the deferred clear so programmatic fills are - // not wiped by the WebKit requestAnimationFrame race. - setQuery(""); - } + setSearchMode(resolvedSearchMode); + setQuery(currentUrlHasQuery ? requestedQuery : ""); - if (params.get("focus") === "1") inputRef.current?.focus({ preventScroll: true }); - }); - return () => window.cancelAnimationFrame(frame); - }, [availableModeIds, initialSearchMode, isDetailPage, pathname, searchParamString]); + if (searchParams.get("focus") === "1") inputRef.current?.focus({ preventScroll: true }); + }, [currentUrlHasQuery, requestedQuery, resolvedSearchMode, searchParamString, searchParams]); useEffect(() => { let cancelled = false; @@ -231,14 +220,14 @@ function GlobalMockupSearchShellClient({ navigateToMode("answer", { query: recentQuery, focus: true }); } - if (shouldRenderDashboardSearch && dashboardSearchMode === "forms" && isFormsOnlyShell) { + if (shouldRenderDashboardSearch && resolvedSearchMode === "forms" && isFormsOnlyShell) { return ; } if (shouldRenderDashboardSearch) { return ( { await expectNoPageHorizontalOverflow(page); }); - test("form detail pages keep the shared forms search wired to form results", async ({ page, browserName }) => { + test("form detail pages keep the shared forms search wired to form results", async ({ page }) => { await page.setViewportSize({ width: 1280, height: 900 }); await gotoLauncher(page, "/forms/transport-crisis-form"); @@ -274,22 +274,12 @@ test.describe("Clinical KB applications launcher", () => { const formsSearchInput = page.locator('input[placeholder="Search forms..."]:visible').first(); await expect(formsSearchInput).toBeVisible(); - // Submit-and-route half is known-broken on CI Linux WebKit only: the shell's - // mount requestAnimationFrame query-sync wipes the composer value there (the - // input stays focused-but-empty and the submit disabled), so the search never - // routes. It does not reproduce on local WebKit and needs CI-based iteration - // on the shell to fix. Skip ONLY this half on WebKit (tracked as follow-up); - // Chromium and Firefox still verify the full wiring, and WebKit keeps the - // structural checks above. See docs/process-hardening.md "Cross-browser test - // robustness". - if (browserName === "webkit") return; - - // Under client-only (ssr:false) rendering the shell re-syncs its query from - // the URL on mount via requestAnimationFrame. On Firefox that frame can land - // right after a programmatic fill — wiping the value, disabling the submit, or - // dropping the submit before the router navigates. Drive the fill-and-submit - // as one retried unit until the search actually routes to the forms results - // URL; the assertions below still verify the result. + // The shell now seeds its composer state from the URL and only re-syncs on a + // real navigation, so a programmatic fill on this no-query detail route is no + // longer wiped by a mount-time frame — the race that used to break CI WebKit + // (and could flake Firefox). Drive the fill-and-submit as one retried unit + // regardless, so any residual cross-browser navigation-timing jitter cannot + // flake the route assertion; the assertions below still verify the result. const formsSearchButton = page.getByRole("button", { name: "Search forms" }); await expect(async () => { // A previous attempt's click may have navigated late — after the inner URL From d40c74826d5d0c319a538a1faa0aa436f6e0444a Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 3 Jul 2026 00:50:30 +0800 Subject: [PATCH 2/3] ci(temp): add WebKit-only forms-detail workflow_dispatch check Temporary focused reproduction so the shell fix can be verified on CI Linux WebKit (the only place the forms-detail search race surfaced) without the full ~70 min release-browser-matrix. Runs just tests/ui-tools.spec.ts on WebKit. Remove once the fix is confirmed green. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/webkit-forms-check.yml | 56 ++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 .github/workflows/webkit-forms-check.yml diff --git a/.github/workflows/webkit-forms-check.yml b/.github/workflows/webkit-forms-check.yml new file mode 100644 index 0000000000..f28a5b872c --- /dev/null +++ b/.github/workflows/webkit-forms-check.yml @@ -0,0 +1,56 @@ +# TEMPORARY: focused CI reproduction for the forms-detail shared-search race that +# only surfaces on CI Linux WebKit (ui-tools.spec.ts:264). Runs just that spec on +# WebKit so the fix in GlobalMockupSearchShellClient can be verified without the +# full ~70 min release-browser-matrix. Delete once the fix is confirmed green. +name: WebKit forms-detail check (temporary) + +on: + # A workflow_dispatch-only file that is not on the default branch cannot be + # dispatched, so trigger on push to this feature branch instead — push events + # run the workflow from the pushed ref's own file. + push: + branches: [claude/webkit-forms-search-fix] + workflow_dispatch: + +jobs: + webkit-forms-check: + runs-on: ubuntu-latest + timeout-minutes: 40 + + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + persist-credentials: false + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version-file: ".nvmrc" + cache: npm + cache-dependency-path: package-lock.json + + - name: Install dependencies + run: npm ci + + - name: Restore browser cache + uses: actions/cache@v4 + with: + path: ~/.cache/ms-playwright + key: playwright-${{ runner.os }}-${{ hashFiles('package-lock.json') }} + + - name: Install Playwright browsers + run: npx playwright install --with-deps + + - name: ui-tools spec across all browsers (WebKit is the fix target) + run: node scripts/run-playwright.mjs tests/ui-tools.spec.ts + + - name: Upload UI diagnostics + if: failure() + uses: actions/upload-artifact@v4 + with: + name: webkit-forms-diagnostics-${{ github.run_id }} + path: | + test-results/ + playwright-report/ + if-no-files-found: ignore From d0dbdf37e99994f8a381093bd6b594ed6ba14393 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 3 Jul 2026 01:01:11 +0800 Subject: [PATCH 3/3] ci: remove temporary WebKit forms-detail verification workflow The forms-detail shared-search fix is confirmed green on CI Linux WebKit (ui-tools.spec.ts:264 passes on chromium/firefox/webkit), so the temporary push-triggered check has served its purpose. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/webkit-forms-check.yml | 56 ------------------------ 1 file changed, 56 deletions(-) delete mode 100644 .github/workflows/webkit-forms-check.yml diff --git a/.github/workflows/webkit-forms-check.yml b/.github/workflows/webkit-forms-check.yml deleted file mode 100644 index f28a5b872c..0000000000 --- a/.github/workflows/webkit-forms-check.yml +++ /dev/null @@ -1,56 +0,0 @@ -# TEMPORARY: focused CI reproduction for the forms-detail shared-search race that -# only surfaces on CI Linux WebKit (ui-tools.spec.ts:264). Runs just that spec on -# WebKit so the fix in GlobalMockupSearchShellClient can be verified without the -# full ~70 min release-browser-matrix. Delete once the fix is confirmed green. -name: WebKit forms-detail check (temporary) - -on: - # A workflow_dispatch-only file that is not on the default branch cannot be - # dispatched, so trigger on push to this feature branch instead — push events - # run the workflow from the pushed ref's own file. - push: - branches: [claude/webkit-forms-search-fix] - workflow_dispatch: - -jobs: - webkit-forms-check: - runs-on: ubuntu-latest - timeout-minutes: 40 - - steps: - - name: Checkout - uses: actions/checkout@v4 - with: - persist-credentials: false - - - name: Setup Node.js - uses: actions/setup-node@v4 - with: - node-version-file: ".nvmrc" - cache: npm - cache-dependency-path: package-lock.json - - - name: Install dependencies - run: npm ci - - - name: Restore browser cache - uses: actions/cache@v4 - with: - path: ~/.cache/ms-playwright - key: playwright-${{ runner.os }}-${{ hashFiles('package-lock.json') }} - - - name: Install Playwright browsers - run: npx playwright install --with-deps - - - name: ui-tools spec across all browsers (WebKit is the fix target) - run: node scripts/run-playwright.mjs tests/ui-tools.spec.ts - - - name: Upload UI diagnostics - if: failure() - uses: actions/upload-artifact@v4 - with: - name: webkit-forms-diagnostics-${{ github.run_id }} - path: | - test-results/ - playwright-report/ - if-no-files-found: ignore