Fix the main-CI browser matrix timeout, improve the DSM differential sidebar, and record the findings - #2713
Conversation
The DSM diagnosis page rendered `documentation_template`, one fixed paragraph
per record that asserts every criterion as met and enumerates every symptom the
disorder can present with. For panic disorder that is all thirteen. Copying it
documented findings that may never have been elicited, and it could not be
edited, tailored or even copied from the page.
It is replaced by a builder that records each criterion as met, not met or not
assessed and emits only what was marked. A criterion left alone is written out
as not assessed rather than quietly dropped, so the note cannot claim more than
the assessment supports. Differentials are ticked only when actively excluded.
Output is sanitised to characters a record system can display. The export uses
216 instances of a greater-than-or-equal sign plus arrows, dashes and curly
quotes, and a threshold silently losing its sign reverses the meaning.
Semicolons are folded to commas, and a criterion line is sentence-cased because
spelling out a leading threshold otherwise opens the line mid-word.
The builder is handed a projection rather than the whole record. It is a Client
Component, so its props cross into the RSC payload, and passing the diagnosis
shipped `documentation_template` to the browser on all 146 pages. `dsm.ts`
records the same trap for the category export.
Also on this page:
- A specifier row that states the disorder HAS no specifiers was being counted
as one. Ten records reported "1 specifier" when the true answer is none.
`dsmSpecifierSplit` separates the two, keeping the six such rows that carry a
real description as prose.
- The Record summary card repeated criteria, specifier and severity counts from
the at-a-glance strip and the ICD-10 code from the header chip. It now carries
provenance, which was stranded in unlabelled footer pills.
- The differentials sidebar silently truncated at six. Nine records carry more,
and the count is now shown.
- The severity tile said "Not listed", which reads as a gap in the catalogue
rather than a fact about DSM-5-TR.
- Added the cross-mode rail, which resolves a related page for 137 of the 146
records. The page was otherwise a dead end.
Specifier menu names ("Mild / Moderate / Severe") are deliberately not parsed
into options. Every splitting rule tried against the corpus produced wrong text
for at least six of the seventeen, and wrong text here is pasted into a record.
Those rows stay visible as reference and are typed into the free-text field.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
…n-8ek1fi # Conflicts: # docs/design-system/COMPONENTS.md
…Lighthouse baseline sequencing Three immutable inbox requests from the DSM diagnosis page session: - release-browser-matrix fails on every main run with 15 Firefox/WebKit failures and is not in PR required, so it never blocks a merge - main CI runs are cancelled during high merge churn despite the per-run concurrency fix, so most main commits land unverified - do not refresh the Lighthouse baseline until main has one genuinely green run, otherwise unattributable drift becomes the new normal No canonical ledger edit; reconciliation runs separately on its own branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
…Lighthouse baseline sequencing Three immutable inbox requests from the DSM diagnosis page session: - release-browser-matrix fails on every main run with 15 Firefox/WebKit failures and is not in PR required, so it never blocks a merge - main CI runs are cancelled during high merge churn despite the per-run concurrency fix, so most main commits land unverified - do not refresh the Lighthouse baseline until main has one genuinely green run, otherwise unattributable drift becomes the new normal No canonical ledger edit; reconciliation runs separately on its own branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
The DSM note-builder commits on this branch were squash-merged into main as #2689, so git still treats them as unmerged. Record them as superseded rather than discarding them, keeping the pull request diff to the three outstanding-issues inbox files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b0329401-2973-4fb8-ad23-832bb619f873) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 607945a484
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rix request Codex review on #2713 was right. The queued request proposed deciding whether release-browser-matrix should join PR required, but that remedy cannot work as written: - ci.yml gates release-browser-matrix on workflow_dispatch, schedule, refs/heads/release/*, or refs/heads/main, so it never runs on a pull_request event. - pr-required's needs list does not include it, and the comment above the job records that it must deliberately not wait on pr-required (#23). A needs edit alone would aggregate a skipped job. The corrected record points at fixing the post-merge signal instead, and states that making the matrix genuinely merge-blocking is a separate, much larger workflow change that must be costed on its own. The request has not landed on main, so this correction is a change to unmerged content rather than a mutation of a queued record in the base. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
…ut not a quota cap The queued request blamed a GitHub Actions spending or concurrency cap for main runs concluding cancelled. Job-level timing disproves that. release-browser-matrix carries timeout-minutes: 70. Measured on two consecutive main runs: - run 34100540973 (f3ea7cb) job 101675121681: 08:30:56Z to 09:41:19Z = 70m23s - run 34104496596 (c8cc72f) job 101689976803: 09:22:47Z to 10:33:07Z = 70m20s Both land exactly on the configured timeout, and their "Full browser UI matrix" step ends in conclusion cancelled. GitHub reports a timed-out job as cancelled, and one cancelled job makes the whole run cancelled. Every other job succeeded in both runs, PR required included. The consequence is worse than first recorded: the matrix now produces no result rather than a red one, so main's Firefox and WebKit coverage is zero rather than failing. The sibling request's 15 failures describe the last state in which the job still finished, at 48.3m, and are cross-linked as superseded in part. The next action is to find why the suite went from roughly 48 to over 70 minutes, not to raise the timeout, which would buy a longer run without restoring a verdict. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_96b8f295-9607-4e73-84cb-704b5d61a74f) |
… a verdict The job stopped finishing. Measured at job level on two consecutive main runs: run 34100540973 job 101675121681 ran 70m23s, run 34104496596 job 101689976803 ran 70m20s, both landing exactly on the old timeout-minutes: 70. GitHub reports a timed-out job as cancelled, and one cancelled job makes the whole run cancelled, which is why every main run concluded cancelled while every other job passed. The consequence was worse than a red suite: the matrix produced NO verdict, so main had zero Firefox and WebKit coverage rather than failing coverage. The 15 Firefox/WebKit failures last seen on edbd29f at 48.3m have not been reported since. Cause: playwright.config.ts pins workers: 1 and fullyParallel: false for determinism, so chromium-mockups + firefox + webkit ran strictly end to end in one job. With retries: 0 and a 60s per-test timeout, each of the 15 failures spends its full timeout, which is roughly the 48-to-70 minute gap on its own. Fix: run the engines as sibling matrix jobs, the same answer Production UI already uses for its Chromium shards. Wall-clock becomes the slowest engine rather than the sum, and determinism inside each job is untouched. The timeout is LOWERED to 45 rather than raised: a single-worker suite is not made faster by a longer cap, and raising it would restore the defect. The split introduces one hazard a single job did not have. The fail-safe path ran bare `npm run test:e2e`, meaning every project in playwright.config.ts; spread across engines that set is now written out, so a project added later could silently never run. tests/ci-browser-matrix-coverage.test.ts re-derives both branch unions from ci.yml and the project list from playwright.config.ts and fails closed on any drift, duplicate, or omission. No test is skipped, disabled or quarantined by this change, and the 15 known failures will now be reported again rather than hidden behind a timeout. The ci-cache-safety case that pinned the single-job command is updated, not removed: it still owns the property it always owned, that the primary path does not re-run production Chromium which ui-critical already proved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
…able, copy the ICD-10 code Four changes to the diagnosis page, all in the differential sidebar and header. 1. DISCRIMINATORS. 534 of the 688 differential rows the sidebar shows (78%) end in a parenthetical that is the clinical reason the differential is raised - "Social anxiety disorder (expected attacks in social situations)". Rendering the whole string on one line buried that behind the name, so the list read as labels rather than something that helps separate two candidates. The name is now the row and the reason is a second line under it. NOT sourced from cross-mode-differentials-index.json, which was the obvious candidate and is wrong for this. Its clinicalHinge is per PRESENTATION GROUP, not per differential: 201 entries share 31 distinct hinge strings, so social-anxiety-disorder carries "Abrupt peak over minutes, recurrent unexpected attacks, anticipatory anxiety or avoidance" - which describes panic disorder, the presentation, not social anxiety. Rendering that under a differential's name would state something clinically false about that diagnosis. The parenthetical is authored on the record against that exact differential, so it cannot be mismatched. Measured coverage would also have been 16% rather than 78%. 2. EVERY ROW IS ACTIONABLE. Rows that resolve to a record open it; the rest - "Medical cause (cardiac, respiratory, endocrine)" and its kind, which name a category rather than a DSM record - search for the name. Previously those rendered as inert text beside linked siblings with nothing to explain the difference, so the list looked half-broken rather than deliberate. 3. COMPARE PER ROW. Compare was one page-level button. When two candidates are in play the natural action is to compare from the row being read, so each resolving row carries a compare control to this diagnosis. 4. COPY THE ICD-10 CODE. It is the one string on the page that gets retyped into a coding field, and it was plain text in a chip sized for reading. The header takes an opt-in copyCode flag so the comparison and differential- considerations pages, which pass a summary string rather than one diagnosis's code, keep rendering text and cannot copy the wrong thing. Both new controls are min-h-tap: they are phone controls in a dense list where the neighbouring rows are other diagnoses. Measured 48x48 and 72x48 at 390px. Verified in the browser at 1280px and 390px: no console errors, zero horizontal overflow, and the copy control puts "F41.0" on the clipboard with its accessible name updating to "Copied ICD-10 code F41.0". tests/dsm-differential-parts.test.ts proves the split against the whole corpus: every differential must rebuild exactly from its parts, and any name that resolved to a diagnosis whole must still resolve after splitting, so the change cannot rewrite clinical text or cost a link. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
check:design-system-contract rejected stacking min-h-tap onto metadataPill: "Chip/metadata density recipes have competing text or height utilities". That is correct - two recipes fighting over one element's height is how chip sizing drifts across the app. The button now owns the 48px tap area and the chip recipe stays on an inner span, so both keep their own job. Hover moves to group-hover so the visual still responds to the whole control. Re-measured at 390px: 72x48 tap target, clipboard receives "F41.0", zero horizontal overflow, no console errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_99446719-b364-434f-b4fb-7ad8433c1b70) |
Saving a diagnosis is the one page-level capability the diagnosis-page review found that cannot be built without a schema change. The content type is constrained in two places at once - favouriteContentTypeSchema and the user_favourites_content_type_check constraint - and adding a value reaches the live clinical database the moment the migration merges, with no deploy step in between. That is worth deciding on a page before it is decided in a migration. The mockup puts three decisions in front of the reader rather than in a doc beside the pictures: 1. Where the save control lives: page header versus the ellipsis menu, with the recommendation and its reason stated inline. 2. What metadata a saved diagnosis carries, shown beside the four types that already exist so the row can be compared rather than imagined. Every other favourite type shows something source-backed; a diagnosis has no source, so code + category + criteria count is proposed as the equivalent. 3. What it costs to build, including the irreversible step called out explicitly. There is a precedent to copy exactly, and the mockup says so: PR 20260814150000 added 'therapy' by dropping and re-adding the check constraint, and 20260814151000 validated it as a separate migration. Same two-step. NOTHING IS WIRED. Every control is local state over fixed data, so the page renders with no account, no database and no network. Built on the project's design tokens rather than Tailwind palette classes. The first draft used bg-sky-700 / bg-slate-* and rendered invisible: this project's @theme replaces the default colour palette, so those classes generate nothing and the badge computed to rgba(0,0,0,0). Caught by reading the rendered page, not by typecheck or lint, neither of which can see a colour that does not exist. Registered in mockups/README.md, which check:mockups gates on, and the route map regenerated. check:mockups, sitemap:check, knip, lint and typecheck all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
…nt size check:type-scale --strict failed on the mockup: three text-[11px] utilities bypass the scale. Mockups are exempt from several design rules - hardcoded hex, the z-index ladder, lucide icon aria, and check:design-system-contract - but the type scale is not one of them, and the exemption list is the kind of thing that is easy to assume rather than check. text-2xs computes to exactly 11px, so the rendering is byte-identical and this is purely the named step replacing the arbitrary one. My process error, not a surprise from the repo: after adding the mockup I ran a targeted subset (lint, knip, check:mockups, sitemap:check, design-system contract) instead of the gate that covers the change, so a static check that had passed on the previous commit was never re-run on this one. The full verify:pr-local run follows this commit rather than preceding the push. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
check:repo-awareness-snapshot failed: adding src/app/mockups/dsm-favourites left the committed snapshot behind the repository's route list. Regenerated with npm run snapshot:repo-awareness. The diff is additive and contains only the new route entry, nothing else. Second thing the full gate caught that a targeted subset had not. Adding a route touches more generated records than the ones I thought to check by hand - sitemap and the mockup index were regenerated, this one was not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #16306 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d7227d84-ecdc-4346-a3ba-8848f4ad3be5) |
1. CI:
release-browser-matrixnever finished, somainhad no browser verdictRoot cause, measured at job level. The job ran
chromium-mockups+firefox+webkitsequentially in ONE job underplaywright.config.ts'sworkers: 1/fullyParallel: false. Run34100540973job101675121681ran 70m23s; run34104496596job101689976803ran 70m20s — both landing exactly on the oldtimeout-minutes: 70. GitHub reports a timed-out job ascancelled, and one cancelled job makes the whole run cancelled, which is why everymainrun concluded cancelled while every other job passed,PR requiredincluded.Worse than a red suite: the matrix produced NO verdict, so
mainhas had zero Firefox and WebKit coverage rather than failing coverage. The 15 Firefox/WebKit failures last seen onedbd29f(48.3m) have not been reported since. The job is also not inPR required, so none of this ever blocked a merge.Why it crossed the line: with
retries: 0and a 60s per-test timeout, each of those 15 failures spends its full timeout instead of passing fast — roughly the 48-to-70 minute gap on its own, and self-accelerating.Fix: run the engines as sibling matrix jobs, the same answer
Production UIalready uses for its Chromium shards. Wall-clock becomes the slowest engine rather than their sum; determinism inside each job is untouched.The timeout is LOWERED to 45, not raised. A single-worker suite is not made faster by a longer cap.
tests/ci-browser-matrix-coverage.test.tspins it at ≤45 so it cannot drift back.No test is skipped, disabled, or quarantined. Expect the matrix to go RED on the first
mainrun after this lands — that is the change working. Triaging those 15 is the follow-up this unblocks.The hazard the split introduces, and its guard
The fail-safe path ran bare
npm run test:e2e, meaning every project inplaywright.config.ts. Spread across engines that set is now written out by hand, so a project added later could silently never run.tests/ci-browser-matrix-coverage.test.tsre-derives both branch unions fromci.ymland the project list fromplaywright.config.ts. Proved by mutation, each reverted:mobile-pwa-standalonefrom the fail-safewebkitgrouptimeout-minutesback to 70playwright.config.tswithout assigning ittests/ci-cache-safety.test.tspinned the old single-job command. Updated, not removed: it still owns the property it always owned, that the primary path does not re-run production Chromiumui-criticalalready proved.2. UI: the DSM differential sidebar
Discriminators. 534 of the 688 rows the sidebar shows (78%) end in a parenthetical that is the clinical reason the differential is raised — "Social anxiety disorder (expected attacks in social situations)". Rendering the whole string on one line buried that behind the name. The name is now the row; the reason is a second line under it.
NOT sourced from
cross-mode-differentials-index.json, which was the obvious candidate and is wrong for this. ItsclinicalHingeis per PRESENTATION GROUP, not per differential: 201 entries share 31 distinct hinge strings, sosocial-anxiety-disordercarries "Abrupt peak over minutes, recurrent unexpected attacks, anticipatory anxiety or avoidance" — which describes panic disorder, the presentation, not social anxiety. Rendering that under a differential's name would state something clinically false about that diagnosis. Coverage would also have been 16% rather than 78%. The parenthetical is authored on the record against that exact differential, so it cannot be mismatched.Every row is actionable. Rows resolving to a record open it; the rest — "Medical cause (cardiac, respiratory, endocrine)" and its kind, naming a category rather than a DSM record — search for the name. Previously those were inert text beside linked siblings with nothing explaining the difference.
Compare per row, against the diagnosis being read. The ICD-10 code copies — the one string on the page that gets retyped into a coding field, previously plain text. The header takes an opt-in
copyCodeflag so the comparison and differential-considerations pages, which pass a summary string rather than one diagnosis's code, cannot copy the wrong thing.Both new controls are
min-h-tap. Measured at 390px: 48×48 and 72×48, zero horizontal overflow, no console errors, and the copy control putsF41.0on the clipboard with its accessible name updating to "Copied ICD-10 code F41.0".Two gates caught real defects in my first attempt, both fixed: a 28px compare target, and stacking
min-h-taponto themetadataPillchip recipe (check:design-system-contract: "Chip/metadata density recipes have competing text or height utilities"). The button now owns the tap area and the chip recipe stays on an inner span.tests/dsm-differential-parts.test.tsproves the split against the whole corpus: every differential must rebuild exactly from its parts, and any name that resolved to a diagnosis whole must still resolve after splitting — so the change cannot rewrite clinical text or cost a link.3. Three ledger records
Immutable inbox requests under
docs/outstanding-issues-inbox/; no canonical ledger edit, reconciliation runs separately on its own fresh-base branch. The matrix redness and its 15 test identities; the cancellation mechanism, corrected to the timeout finding; and do-not-refresh-the-Lighthouse-baseline untilmainhas one genuinely green run.Corrections made on this branch
#discussion_r3948230590),6e871f1. The record proposed deciding whether the matrix should joinPR required. That cannot work —ci.ymlgates the job onworkflow_dispatch,schedule,refs/heads/release/*, orrefs/heads/main, so nopull_requestevent satisfies it, and its omission frompr-requiredis deliberate (#023).eeecbbe. The record blamed an Actions spending or concurrency cap. Job-level timing disproves it; the per-run concurrency group is explicitly not implicated.Verification
npm run verify:pr-local— risk-routed to heavy scope. All static gates,lint,typecheck, and the full unit suite pass.npm run test:ci-workflows— 568 passed (24 files)npm run build— run clean after stopping the dev server (the gate correctly refuses a build while it is up):check:rag:fixtures(36 golden cases, 26 suites),check:medication-interactions,check:medication-lexicon-report— the three the earlier build failure had blockedTwo warnings, neither from this change.
bundle-budgetreports +5.6% vs baseline (drift warning >5%, tolerance 10%, so it passes) — accumulated growth already tracked as#QSHHGK, and this diff adds one small client component.check:medication-lexicon-reportwarns that a sign-off recorded 2026-08-22 predates the current mappings hash; nothing here touches medication data.mainafter merge.Risk and rollout
/dsm/diagnoses/[slug]. No data, schema, or provider effect. The clinical-text risk is the one addressed above by refusing the mismatched hinge source.npm run issues:reconcile.Notes
main, socheck:ledger-write-disciplinesees unmerged content rather than a mutated base record.key_featuresis present on all 146 records and is what the main criteria panel renders; the section is gated oncriteria_display, which only Bipolar II carries, so the gate is correct duplicate-avoidance rather than a fault. No change made.🤖 Generated with Claude Code
https://claude.ai/code/session_017fBy41i2ynVpA51b5CuYSD
Note
Medium Risk
The CI workflow change directly affects post-merge browser coverage on main; a mis-assigned Playwright project would silently drop tests without the new guard test. DSM changes are read-path UI and parsed clinical strings, with tests but no schema migration in this PR.
Overview
CI:
release-browser-matrixno longer runs Chromium mockups, Firefox, and WebKit in one sequential job that was hitting the old 70-minute cap with no test verdict. It now uses aengine: [chromium, firefox, webkit]matrix withfail-fast: false, a 45-minute per-job timeout, and shellcaseblocks that assign Playwright--project=flags per engine (primary vs fail-safe paths). Failure artifacts are named-${{ matrix.engine }}.tests/ci-browser-matrix-coverage.test.ts(wired intotest:ci-workflows) fails closed if engine groups drift fromplaywright.config.tsor the timeout creeps back up;tests/ci-cache-safety.test.tswas updated for the new shape.DSM diagnosis UI: Adds
dsmDifferentialPartsto split trailing parenthetical discriminators from names; the sidebar shows name + subtitle, links unresolved rows to/dsm/search?…&run=1, and adds per-row compare plus an opt-incopyCodeICD-10 control viaDsmCodeCopy. Vitest/DOM tests cover the split and rendering.Also: Static
/mockups/dsm-favouritesstudy page and doc/index updates; three outstanding-issues inbox JSON records (matrix timeout, Firefox/WebKit failures, Lighthouse baseline refresh guidance) plus snapshot/sitemap entries.Reviewed by Cursor Bugbot for commit dcaf127. Configure here.