diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 3cd1a6d0de..ff8cdf6cff 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -171,3 +171,4 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-07-30 | claude/outstanding-issues-triage-24c8ow | 8d2710fd6cbdc84e8c50a6c9bc0a1e1a0cd612c8 | open PR changed-scope review | APPROVE: completed items 095, 096, 104, 109, and 115 move to archive with no deletion, duplicate ID, or stale next-id. | check:outstanding-issues PASS; check:branch-review-ledger PASS; diff review; no unresolved threads | | 2026-07-30 | claude/latency-findings-impl-s8g01v | e7ff5e933ba1f34d5adbd46dd77c38aced11ed44 | open PR changed-scope review | APPROVE: ordering-risk documentation is accurate and the near-bottom refusal guard now proves its geometry is non-vacuous before asserting no hide. | diff check PASS; focused test review; no unresolved threads; exact-head Production UI required | | 2026-07-30 | claude/ci-testing-review-2l8klp | 2e2160bc8b9d2d824209c217c67cb9cac1be3a8d | open PR changed-scope review | APPROVE: three-way UI sharding, critical-first gating, measured drag travel, and gate-manifest updates preserve required-check aggregation and deterministic Playwright settings. | check:github-actions PASS; check:ci-scope PASS; check:gate-manifest PASS; ledger guards PASS; exact-head sharded Production UI required | +| 2026-07-30 | PR #1430 | a9ae22ac4915e86d51ee05787059382a39bd8ba8 | phone chrome diagnostics and merge repair | fixed and ready for CI | issues guard; ledger guard; 37 focused tests; phone-chrome dry-run | diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index a8684d1ae3..ef5f14f169 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -82,7 +82,7 @@ removed after current-main verification; it is not missing recommended work. | 34 | `#105` | Optional | High — browser/UI verification | When the heavy-run lock is free | 20–40 minutes | Run `verify:ui` over the ten `LoadingPanel` fallbacks and confirm the Supabase `preconnect` reaches `` on a live page. Implementation already shipped; this row is the outstanding verification only. | | 35 | `#126` | Optional | Standard — repository hygiene | Once per UTC calendar quarter, or when the live ledger grows large | 5–15 minutes | Run `npm run ledger:rotate -- --dry-run`, then `npm run ledger:rotate` if the preview looks right; commit the live+archive diff. Stop if dry-run shows unexpected mass moves or archive path collisions. | - + ## Open items @@ -151,7 +151,7 @@ removed after current-main verification; it is not missing recommended work. | #117 | P2 | rec | Therapy Compass catalogue payload is the mobile LCP outlier | **Outcome:** `/therapy-compass` mobile LCP lands near the other mobile routes instead of double them. **Measured 2026-07-30** by the new pre-merge Lighthouse budget: mobile LCP 5229 ms, TBT 612 ms, CLS 0.142, against 2123-2460 ms on every other mobile route and 826 ms on desktop — so it is client-side work under mobile CPU/network throttling, not server latency. **Cause:** `useTherapyData` fetches `public/therapy-compass-data/therapies-index.json` (690 KB raw, 139 KB gzipped, 205 records x 16 fields) for the home/search/pathways screens, so the download plus JSON parse sits on the critical path before content paints. 90% of that weight is long-form clinical prose — indications 159 KB (26%), contraindicationsOrCautions 139 KB (23%), bestUsedFor 73 KB (12%), clinicalSummary 67 KB (11%), patientPopulation 59 KB (10%), targetSymptoms 48 KB (8%) — while name, slug, category, tags and setting together are 54 KB (7%). **Blocked on one decision per field group: rendered on the card, matched by search, or neither.** `therapy-card.tsx` references five of those prose fields and the same index feeds the search screen, so stripping fields could silently change clinical display or search recall. **Next:** settle that per-field question, then either pre-truncate prose that only feeds card display, or move search matching server-side / load prose on first keystroke. **Gate:** `check:therapy-data-index` plus the therapy Playwright journeys; re-measure with `npm run verify:lighthouse`. **Stop:** do not drop a field from the catalogue payload without confirming no card renders it and no search path matches on it. Same class as #013 (route-chunk / catalogue JSON weight), different route and now measured. | session 2026-07-30 Lighthouse budget first run; PR #1404 | 2026-07-30 | | #118 | P2 | task | Adopt the visual and Lighthouse baselines so the two new gates actually gate | **Outcome:** `visual-baseline` and `lighthouse-budget` stop reporting and start blocking. **Detail:** PR #1404 added both as `continue-on-error` jobs outside `pr-required`, deliberately. `tests/ui-visual-baseline.spec.ts` has no committed baselines, so all six targets fail with a missing-snapshot error by design; the job uploads them on every run (run 30513537912, artifact 8748062487, 31 files). `lighthouse-budget.json` ships `enforce: false` with `baseline: null`, so the grader warns rather than grades. **Next:** (1) download that artifact, review the six PNGs and commit them under the platform-scoped screenshots directory that `playwright.visual.config.ts` names in its `snapshotPathTemplate` — from CI, never a developer machine, because font hinting differs between them; (2) run `npm run check:lighthouse-budget -- --update` against a known-good CI build and flip `enforce`, but not before #117 or the baseline pins a known-slow route; (3) then add each job to `pr-required` and drop `continue-on-error` in the same edit. **Also:** PR #1404 added the first rendered-effect contract for #094, but 37 of the 38 unlayered visual classes still carry exemptions in `tests/helpers/style-contracts.ts` rather than contracts; and `scripts/run-lighthouse-budget.mjs` duplicates about 50 lines of the isolated-server boot in `scripts/run-playwright.mjs`, deferred to avoid destabilising the required UI gate in the same change. **Stop:** do not make a missing baseline skip instead of fail — that is the soft-skip-green pattern `AGENTS.md` forbids. | session 2026-07-30; PR #1404 | 2026-07-30 | | #119 | P2 | issue | `ci/circleci: verify` is failing repo-wide | **Outcome:** the CircleCI status is trustworthy again. **Detail:** it failed on every head of PR #1404 (builds 646, 654, 658, 668) and also on PR #1400, which changes two lines of markdown — so it is not any one diff. Every command `.circleci/config.yml` runs is green under GitHub Actions on the same commits: `static-pr` runs the same `format:check`, `lint` and `typecheck`, the `coverage` job runs the same unit suite, and CircleCI's own pre-step `node scripts/ci-change-scope.mjs --base origin/main --head HEAD` exits 0 locally. That leaves the steps unique to CircleCI — the `cimg/node:24.18` executor and its engine asserts, `npm ci` inside that image, and the `apt-get` plus `python3 -m venv` plus pinned `PyMuPDF==1.28.0` bootstrap that exports `PYTHON_BIN` for the test run. Checked 2026-07-30: the `PyMuPDF==1.28.0` pin is valid on PyPI — latest, not yanked, wheels plus an sdist — so a bad pin is NOT the cause; if that step is still implicated it would be a build-from-sdist failure under the image's Python, not a resolution failure. A docs-only commit (`a6f5fa6`, one markdown file) also failed, so the result is invariant to what is pushed. **Next:** someone with CircleCI log access must read build 672; no session here holds those credentials. Overlaps the CI-health review in PR #1406. **Stop:** do not treat a red CircleCI status as evidence about a branch's own diff until this is resolved. | session 2026-07-30 PR #1404 CI triage; PR #1400; PR #1406 | 2026-07-30 | -| #127 | P2 | issue | `ui-phone-scroll` document-detail hide sticks visible under CI load | **Outcome:** the required `Production UI` job stops failing on a test nothing in the PR touched. **Evidence 2026-07-30 (PR #1404 head `233b358`, run `30521269873`, job `90802044795`):** `tests/ui-phone-scroll.spec.ts:428` "document detail header overlay and footer follow browser document scrolling together" (the `browser document` variant) failed at line 586 — the **second** hide cycle, the one after `page.emulateMedia({ reducedMotion: "reduce" })`. `data-scroll-hidden` never appeared on `universal-header-collapse` across the full 10 s poll (24 locator resolutions, `unexpected value "null"`), and the failing snapshot carries it on none of the three chrome edges. Everything else in that job passed: `1 failed, 340 passed (11.9m)`. **Not caused by that PR:** #1404 changes no file under `src/`, does not touch `ui-phone-scroll.spec.ts`, and `main` passed this exact test at `90b3e34` (run `30520838851`, Production UI success 06:50-07:06) — the very commit #1404 merged. **The gesture landed:** the trace records `documentElement.scrollTop` = 1272, exactly the 552 + 720 the drag asks for, and the same test's earlier hide measured `maxScrollTop` 2753 visible / 2575 hidden, so the page sat ~1300 px clear of the bottom band where `computeScrollHideUpdate` legitimately refuses to hide. **It is stuck, not slow:** a 10 s non-flip is a latched state, not a race that resolves, which points at `sharedChromePinned` (`headerFocusPinsChrome` carries no still-the-active-owner guard, unlike `composerFocusPinsChrome` beside it) or at a detached scroll listener, rather than at CI slowness. The trace cannot separate those two — both suppress all three edges identically. **Does not reproduce locally:** the single test passes in 6.3 s and the whole file `56 passed (4.3m)` against the same isolated production build. **Next:** capture which of `scrollHidden` / `sharedChromePinned` is wrong at the failing assertion, from a CI-load run rather than a laptop; re-running the job is a mask, not a fix. **Stop:** do not move it into `tests/flake-ledger.json` — that ledger takes reproduced `@quarantine` flakes only, and this one has not been reproduced. | `tests/ui-phone-scroll.spec.ts:586`; `src/components/clinical-dashboard/use-hide-on-scroll.ts`; `master-search-header.tsx:403` | 2026-07-30 | +| #127 | P2 | issue | Document-route phone header did not hide; likely a short drag, fixed by #1427 | **Outcome:** the document-route phone header hides when asked, and a failure names its own cause. **Observed:** `Production UI` failed on `30521269873` (1 variant) and `30524711173` (both variants, `2 failed, 339 passed`), always at the reduced-motion hide after the section-sheet round-trip, then **passed** on `30530393684` — intermittent, 2 of 3 completed runs. **Cause most likely already fixed on main by PR #1427 (2026-07-30), not by anything in this row's earlier diagnoses.** #1427 found that `addPhoneScrollRunway` slept 50 ms and merely hoped the appended 1600 px runway had reached layout, and that `dragScrollBy` clamped silently at the end of the range while reporting nothing — so under CI load the drag delivered less than it asked for and the chrome was right to stay visible. It now polls for the runway, returns the distance actually travelled, and `dragScrollUntilHidden` refuses to expect a hide until both the remaining runway and the delivered travel clear 160 px. **Two earlier diagnoses in this row were wrong and are withdrawn:** (1) `sharedChromePinned` is stuck — rested on reading DocumentViewer's composer as a proxy for the header's `scrollHidden`, which it is not, since the header runs the shell's `chromeScrollHide` and DocumentViewer runs its own reporters; (2) a short/clamped drag was ruled out early using a `maxOffset` of 2753 measured at a different moment in the trace than the failing drag — the pre-runway reading in the same trace was 1153, and a runway that had not fully landed puts the offset inside the near-bottom band where `computeScrollHideUpdate` legitimately refuses. That is the mechanism #1427 fixes. **Still open here:** #1427's own comment notes the remaining gap — `data-scroll-hidden` is the composite `scrollHidden && !sharedChromePinned`, so a reporter that never fired and a latched pin are indistinguishable. `data-scroll-signal` now publishes the raw signal and `expectChromeHidden` reports it, so if this recurs after #1427 the failure says which. **Next:** watch for a recurrence on a post-#1427 head; if none appears, close this as fixed by #1427. **Stop:** do not infer the header's scroll state from any page-owned composer, and do not read a `maxOffset` from one trace moment as the value at another. | `tests/ui-phone-scroll.spec.ts`; PR #1427; `master-search-header.tsx`; `global-search-shell.tsx:332,345,876` | 2026-07-30 | | #128 | P2 | issue | Draft-to-ready alone does not retrigger required CI | **Outcome:** un-drafting a PR is not by itself enough to get its full required-check suite running. **Detail:** on 2026-07-30 PR #1406 sat with only 3-4 minimal checks (`PR policy`, `GitGuardian`, `Supabase Preview`) for 30+ minutes after being marked ready for review, with `mergeable_state` clean (not conflicted — distinct from #116). `.github/workflows/ci.yml`'s `on: pull_request` block has no explicit `types:`, which defaults to GitHub's `[opened, synchronize, reopened]`; `ready_for_review` is not in that list, so converting a draft to ready fires no workflow run on its own. The full suite only started once an actual new commit landed (a `synchronize` event), e.g. syncing the branch with `main`. **Next:** if drafts routinely go ready without an accompanying commit, add `ready_for_review` to `ci.yml`'s `pull_request.types` list ALONGSIDE the current implicit defaults — `types: [opened, synchronize, reopened, ready_for_review]`, not a bare `[ready_for_review]`, which would replace the defaults and stop CI firing on ordinary `opened`/`synchronize`/`reopened` events. Otherwise document that un-drafting alone is insufficient and a session should push a no-op/sync commit to actually kick off CI. **Stop:** do not conclude a draft's CI is "just slow" from elapsed time alone — check whether it actually has the full check set (16-19 checks, not 3-4) before waiting further. | PR #1406; session 2026-07-30 PR babysit | 2026-07-30 | | #129 | P2 | issue | GitHub's `update-branch` API doesn't honor this repo's `merge=ledger` driver | **Outcome:** `update-branch` can report a 422 "merge conflict between base and head" on a branch that a local `git merge origin/main` resolves cleanly. **Detail:** on 2026-07-30 PR #1406's branch was several commits behind `main` and touched `docs/branch-review-ledger.md`, which carries `merge=ledger` in `.gitattributes` specifically so parallel ledger appends resolve without conflict (see #088/#112). GitHub's own server-side merge/update-branch implementation does not read `.gitattributes` custom merge drivers, so it computed a real textual conflict at the same hunk the local `ledger` driver resolves. `git merge-tree --write-tree origin/main ` confirmed clean; the API call still 422'd. **Next:** when `update-branch` fails on a branch touching `docs/branch-review-ledger.md` (or any other `merge=ledger` path) and `git merge-tree` shows no real conflict, treat it as staleness rather than a genuine conflict needing manual resolution and fall back to a local `git merge origin/main` + push (per the existing "Open PR branch sync" guidance) — same as any other push, this still needs the explicit user confirmation AGENTS.md's "API and provider confirmation boundary" requires outside an authorized sweep (`Run PR`/`upload`), not a standing exemption for `merge=ledger` paths. **Stop:** do not conclude a real content conflict from `update-branch`'s response alone on a custom-merge-driven file; verify with `git merge-tree` first — same discipline as the existing GitHub `dirty`/`CONFLICTING` staleness guidance. | PR #1406; session 2026-07-30 PR babysit | 2026-07-30 | | #130 | P2 | issue | PR #1396 merged shared phone-chrome behaviour without its own declared physical-device gate | **Outcome:** a shared-chrome PR does not merge with a self-declared merge prerequisite left undone, or the ledger records that it did. **Detail:** PR #1396 ("overlay the phone header so hiding it never moves content") repeatedly stated in its own body and PR comments that `docs/phone-chrome-physical-acceptance.md` "genuinely applies before merge" because local Chromium cannot certify Safari chrome-minimisation or cold-launch PWA paint (invariant 23) — restated at least three times across the review thread, including after the final `a638b66e`/`f7347144` fix. It merged at 06:49:55 anyway. Checked 2026-07-30: `docs/phone-chrome-physical-acceptance.md` on `main` is still the blank checklist template — every "Result / evidence" cell is empty, no PR comment attaches a filled-in copy or device evidence, and no existing ledger row (`#120`, `#122`) covers this gap. Related but distinct: one Codex thread on this PR also names a still-missing guard — a pre-paint/cold-load hydration test comparing content position before and after hydration, which the author explicitly said they would "rather file it than ship a test that looks like it covers the window and does not" — and that filing never happened either. **Next:** run the physical-device matrix in `docs/phone-chrome-physical-acceptance.md` against `main`'s current tip on a real iPhone (Safari tab + cold-launch PWA, light/dark, portrait/landscape) and commit the filled-in evidence; separately, add the pre-paint/cold-load Playwright pattern this PR's own review identified as missing. **Stop:** do not treat this PR's extensive Codex/CI remediation (13 findings fixed, 9 threads resolved) as a substitute for the physical-device proof — headless Chromium was explicitly stated as unable to certify the two things this checklist exists for. | PR #1396 (merged 2026-07-30); session 2026-07-30 PR babysit | 2026-07-30 | @@ -164,6 +164,7 @@ removed after current-main verification; it is not missing recommended work. | #137 | P3 | rec | Advisory UI spends ~3 min per UI PR on five mockup tests | **Outcome:** the advisory lane costs what its signal is worth. **Detail:** `ui-advisory` runs on every UI PR (3m14 measured 2026-07-30) to cover `@quarantine` plus `@mockup` journeys — but `tests/flake-ledger.json` is empty and there are **zero** `@quarantine` tests in the suite, so it executes 5 `@mockup` tests. It is `continue-on-error` and outside `pr-required`, so it can also rot unnoticed. **Next:** gate it on mockup-file scope, or accept the cost as the price of keeping the lane warm for future quarantines. Either is defensible; the current state is just unmeasured. | `.github/workflows/ci.yml` `ui-advisory`; `tests/flake-ledger.json`; session 2026-07-30 | 2026-07-30 | | #138 | P3 | task | CI Triage ships inert pending a repo variable | **Outcome:** a PR red that is really a main-side regression is labelled as such instead of costing an author a debugging session. **Detail:** `.github/workflows/ci-triage.yml` is complete and self-tested (`check:ci-triage` runs in `verify:cheap` and `static-pr`) but every run short-circuits on `vars.CI_TRIAGE_ENABLED == 'true'`, which is unset. Its purpose is the failure mode this doc records repeatedly: CI merges the PR branch with current `main`, so a main regression surfaces on every open PR. **Next:** operator sets the repository variable, then confirm one triage comment posts. Reads job metadata from a trusted default-branch checkout only; never runs PR code. | `.github/workflows/ci-triage.yml`; session 2026-07-30 | 2026-07-30 | | #139 | P3 | rec | `changes` job computes outputs nothing consumes, and over-triggers coverage | **Outcome:** the change-scope contract says what it means. **Detail:** `changes` exports `source_changed`, `workflow_changed`, `changed_files` and `rag_eval_changed` and **no job reads any of them** — only `rag_eval_changed` is documented as intentionally advisory (`ci-change-scope.mjs`); the other three read as live wiring. Separately `coverage_changed` is derived as _any non-doc file_, so a workflow-only edit runs the ~4 min coverage job. **Next:** delete or document the dead outputs; decide whether coverage should narrow. Low value alone — bundle with the next `ci.yml` change rather than minting a PR. | `.github/workflows/ci.yml`; `scripts/ci-change-scope.mjs`; session 2026-07-30 | 2026-07-30 | +| #140 | P2 | issue | `merge=union` on the issues ledger duplicates the whole table instead of merging it | **Outcome:** a concurrent edit to `docs/outstanding-issues.md` merges, conflicts honestly, or is deduped — but never silently doubles the file. **Evidence 2026-07-30 (PR #1430):** `.gitattributes:19` sets `docs/outstanding-issues.md merge=union`. That is git's **built-in** union driver, which concatenates both sides of every conflicting hunk with **no dedupe** — unlike `docs/branch-review-ledger.md merge=ledger`, whose `scripts/merge-branch-review-ledger.mjs` does dedupe exact rows. The open-items table is not append-only: rows get rewritten, reordered and archived. So when this branch had edited `#127` while `main` rewrote and reordered overlapping rows, `git merge origin/main` reported success and produced **every row twice** — `#059 appears 2 times (lines 101, 166)` and so on for the entire table. `npm run check:outstanding-issues` caught it, which is the gate working; but the driver converted a resolvable conflict into a guaranteed guard failure needing a manual rebuild of the file from `origin/main` plus the one changed row. Union also makes `git merge-tree` report a clean tree, so the pre-merge conflict check cannot warn about it. **Next:** either give this file a dedupe driver like the branch-review ledger's, or drop `merge=union` and let it conflict honestly — the guard already blocks duplicates either way. **Stop:** do not resolve a duplicated table by deleting rows by eye; rebuild from `origin/main` and re-apply only the rows the branch actually changed. | `.gitattributes:19`; `scripts/check-outstanding-issues.mjs`; `scripts/merge-branch-review-ledger.mjs`; PR #1430 | 2026-07-30 | ## Resolved / archive diff --git a/src/components/clinical-dashboard/master-search-header.tsx b/src/components/clinical-dashboard/master-search-header.tsx index 068f6a66db..c8118c8e7c 100644 --- a/src/components/clinical-dashboard/master-search-header.tsx +++ b/src/components/clinical-dashboard/master-search-header.tsx @@ -2109,6 +2109,17 @@ export function MasterSearchHeader({ const collapsingTopBar = (
{ + const attribute = (selector: string, name: string) => + document.querySelector(selector)?.getAttribute(name) ?? null; + const describe = (element: Element | null) => { + if (!element) return "none"; + const testId = element.getAttribute("data-testid"); + return `${element.tagName.toLowerCase()}${testId ? `[data-testid="${testId}"]` : ""}`; + }; + const active = document.activeElement; + const addonHost = document.getElementById(addonSlotId); + const collapseSelector = '[data-testid="universal-header-collapse"]'; + return { + headerScrollSignal: attribute(collapseSelector, "data-scroll-signal"), + headerCollapse: attribute(collapseSelector, "data-scroll-hidden"), + documentComposer: attribute("form.document-viewer-composer", "data-scroll-hidden"), + documentContent: attribute('[data-testid="document-viewer-content"]', "data-scroll-hidden"), + activeElement: describe(active), + focusInsideAddonHost: Boolean(addonHost && active && addonHost.contains(active)), + addonHostPresent: Boolean(addonHost), + expandedMenuTrigger: document.querySelector('[aria-haspopup="menu"][aria-expanded="true"]') !== null, + expandedCombobox: document.querySelector('[role="combobox"][aria-expanded="true"]') !== null, + scopePopover: document.querySelector('[data-testid="scope-command-popover"]') !== null, + openDialogs: document.querySelectorAll('[role="dialog"]').length, + openMenus: document.querySelectorAll('[role="menu"]').length, + }; + }, phoneHeaderCollapseAddonSlotId); + + const documentViewerHidden = state.documentComposer === "true" || state.documentContent === "true"; + const verdict = + state.headerScrollSignal === "hidden" + ? "the header's own scrollHidden is TRUE, so sharedChromePinned swallowed it — see the pin tells below" + : state.headerScrollSignal === "visible" + ? `the header's own scrollHidden is FALSE — the shell reporter never saw the proven gesture${ + documentViewerHidden + ? ", while DocumentViewer's independent reporters DID: the two scroll feeds diverged" + : "" + }` + : "data-scroll-signal is absent, so the two causes cannot be separated"; + + throw new Error( + `${phase}: the phone chrome never hid.\n${verdict}\nchrome state: ${JSON.stringify(state, null, 2)}\n\n` + + `Original assertion failure:\n${error instanceof Error ? error.message : String(error)}`, + ); + } +} + interface PageOwnedFooterGeometry { footerOpacity: number; footerPosition: string; @@ -558,7 +626,7 @@ for (const phoneOwner of ["browser document", "standalone PWA main"] as const) { await sectionTrigger.evaluate((element) => element.blur()); await dragScrollUntilHidden(page, 720, 24); - await expect(collapse).toHaveAttribute("data-scroll-hidden", "true"); + await expectChromeHidden(page, collapse, "first hide with motion enabled"); await expect(overlayStack).toHaveAttribute("data-scroll-hidden", "true"); await expect(composer).toHaveAttribute("data-scroll-hidden", "true"); @@ -658,7 +726,7 @@ for (const phoneOwner of ["browser document", "standalone PWA main"] as const) { await dragScrollBy(page, -480, 16); await expect(collapse).not.toHaveAttribute("data-scroll-hidden", "true"); await dragScrollUntilHidden(page, 720, 24); - await expect(collapse).toHaveAttribute("data-scroll-hidden", "true"); + await expectChromeHidden(page, collapse, "reduced-motion hide after the section-sheet round-trip"); const reducedHidden = await readPrimaryScrollAndDomGeometry(page, { stack: '.phone-sticky-header-stack[data-phone-motion="overlay"]', content: '[data-testid="document-viewer-content"]',