Search results band: truthful failure state, approved design, and adoption gate - #1316
Conversation
The shared results band had two states, loading or done, and no failure
path. When a registry call errored or returned unauthorized, the page
handed it matchCount={0} and it rendered a confident "0 matches". On the
services navigator that reads as "there are no crisis services" when the
truth is "we could not check" — the band asserted a clinical fact it had
no basis for. The same held on medications, differentials, documents,
forms and favourites.
Add a `status` union (ready | loading | refetching | error | unauthorized)
so the invariant "a faulted search never asserts a number" lives in one
guard rather than being re-derived per call site. A union rather than a
second boolean because `loading && error` is otherwise representable and
undefined; it also mirrors RegistryRequestStatus, so most callers can pass
a near-identity map of the status they already hold.
The deprecated `loading` prop stays as a shim so the five pages with no
async source keep working untouched and the existing DOM assertions are
undisturbed. No call site changes yet — this commit only adds the
capability.
Accessibility: the spine keeps one unconditional role="status" (Playwright
asserts it is visible on every search route), and the fault panel below
carries role="alert". While faulted the spine's live region is silenced so
the alert makes the single announcement rather than both speaking. Retry
goes through the shared AsyncButton busy contract.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
The services navigator passed `loading={registryLoading}` and left
`registryBlocked` unused by the band, so an errored or unauthorized
registry reached the header as matchCount={0} and rendered "0 matches".
On the crisis-services surface that states there are no crisis services
when the search never ran.
Pass the real status instead, with the existing notice copy moved into the
band's fault panel and `registry.refetch` — previously computed and never
used — wired to Retry. The unauthorized case keeps its distinct "Session
expired" copy and the account-setup link rather than being flattened into
a generic error.
Remove the now-duplicate ModeHomeStatusNotice blocks below the band so the
failure is announced once. The mode-home page keeps its own notice; it has
no results band.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
Every page that can actually fail now tells the band so, instead of
letting an errored fetch arrive as matchCount={0}:
- differentials: catalogFailed was computed and used only for a panel
below the band; the band itself still printed "0 matches".
- prescribing: catalog.error was rendered in a separate danger strip.
- therapy-compass: b.error was never referenced on this screen at all,
and b.retryData now backs Retry.
- documents: the standalone unavailable alert is kept only for routes
that render no ribbon, so the message is never lost.
- forms: structural. The band was mounted only when the registry was
ready, so a failure removed the header entirely. It is now mounted in
every state and RegistryStatusNotice, made redundant, is deleted.
- favourites: useSavedRegistryFavourites discarded both underlying
registry statuses and returned bare items, so the page could not tell
"no favourites" from "could not load favourites". It now returns
{ items, status }, folding only the registries it actually requested —
a disabled hook sits in its initial state forever and must not read as
a failure.
The five pages with no async source are untouched; they are already
correct on the default.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
Wiring every current page is worth little if the next search page can skip the band and go back to reporting "0 matches" for a failed search. Two layers close that: - `AppModeSearchConfig.resultsSurface` is required and non-optional, so a fourteenth mode fails typecheck until its author declares whether it presents a result list or a synthesised answer. - tests/search-results-band-adoption.test.ts holds them to it: every mode declaring "results-band" must have a production component mounting the band for that modeId, and every production search route must reach the band within one import hop. The modeId scan collects literals recursively because document-search computes its mode via a ternary; a naive matcher would read that page as having no mode. The route assertion carries one documented allowlist entry (the documents search stub, which has no result list of its own) and asserts it found routes at all, so it cannot pass vacuously. Verified by negative control: flipping answer to "results-band" fails the gate with a named mode, and restoring it returns to green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
Visual pass to the approved mockup (Option A, Edit 2). Rail: the accent was an absolutely-positioned bar inside an overflow-hidden 12px-radius card, so the corner arc sliced its ends — at y=0 the arc is 12px in, at y=2px it is 5.4px in — and it started short and tapered while the 1px border curved past it. Two lines, two geometries. It is now the card's real border-top, which mitres into the side borders and follows the radius by construction. The desktop left-hand bar goes with it. Type: nothing on this surface is bold any more. The heaviest step is 580 and the query (560) and figure (580) share it, separated by tabular numerals and a hairline rather than by shouting; "matches" drops to 400 and a zero count steps down to 470 and muted. Weights are numeric font-weight on named classes in globals.css rather than Tailwind arbitrary values, because check:type-scale --strict is a zero gate on arbitrary text-[Npx] and that is already the idiom in this file. Breakpoint: the row/stack switch moves from lg to sm so portrait tablets stop getting the phone layout. The rail's overflow, fade mask and trailing spacer stay on lg deliberately — at 640-1023px a page with chips, sort, a mobile filter and utility controls can exceed the width, and containing that in a scrollable rail is what keeps the no-horizontal-overflow assertions green. Forced colors: the rail survives as thickness (3px, 6px double for a fault) because --clinical-accent resolves to LinkText and would otherwise be indistinguishable from the other borders. That is what keeps a failed search distinct from a successful one when colour is gone. The block is appended last in globals.css, since at equal specificity a later rule wins and an earlier block is silently overridden while still reading correctly. --warning-border is not remapped by the token block, so it is pinned here. Formulation adopts the refetch state: its "loading" was useDeferredValue lag over static data, not a network request, so the previous count is still correct and stays visible with a pulse instead of collapsing to a skeleton. Safe there precisely because nothing identity-scoped is held across the gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
A comment inside the null branch of the results ternary was reordered by Prettier on every run, so format:check could never converge. Move it above the expression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe shared ChangesSearch results band
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SearchRoute
participant RegistryOrCatalogue
participant SearchResultsHeaderBand
participant User
SearchRoute->>RegistryOrCatalogue: read loading, error, or unauthorized state
SearchRoute->>SearchResultsHeaderBand: pass resolved status and fault content
SearchResultsHeaderBand->>SearchResultsHeaderBand: suppress count and controls when faulted
SearchResultsHeaderBand->>User: render status region and fault alert
User->>SearchResultsHeaderBand: activate retry
SearchResultsHeaderBand->>RegistryOrCatalogue: invoke retry handler
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #6289 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
Production UI failed twice on this PR with the same signature but a different test each time: a strict-mode violation where a document-wide getByTestId resolved to two identical elements, one of them hidden. First `differentials-search-results`, then `global-search-input`. That is the Next streaming `S:` clone of the page root, which CI load makes observable and which #1294 already documented and fixed the same way on the differentials detail page. It is not caused by this branch: the test that failed first passed untouched on the next run. Scope the affected assertions to the visible element, which is the idiom already used for this exact testid in ui-overlap.spec.ts. Applied to the class rather than the single instance, so the flake does not simply move to whichever spec loses the race next. Deliberately not changed: the `toHaveCount` assertions on this testid. Those count elements on purpose as single-owner guards, and visible-scoping them would change what they assert. Verified locally: ui-chrome-scroll + ui-phone-scroll 65 passed; ui-smoke 91 passed with only the pre-existing PDF-canvas failure, which is a local browser-build artifact and passes in CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
…-w53znc' into claude/top-search-design-mockups-w53znc
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 389250d3f6
ℹ️ 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".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ade17ac880
ℹ️ 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".
Four findings, all real, all mine. P1 — forms/medication/differentials: suppressing the count in the spine is worthless while a page-supplied filter chip still renders "Forms 0" or "All (0)" beside it. The reader still saw a zero asserted about a search that never ran, which is the exact defect this branch exists to remove. Fixed at the band rather than per page: a faulted band drops filter and mobile controls entirely, since a filter over a result set that failed to load is meaningless anyway. Test now asserts no digit anywhere in the ribbon, not just in the status node. P1 — documents: on the services/forms path the registry has its own status and can be healthy while the unrelated document API is down. Deriving the ribbon's status from the document API let that invalidate good registry results — announcing "Couldn't search" and hiding a valid recordMatchCount while SearchRecordResults rendered those very matches below. Derive from whichever source the ribbon is actually counting. P2 — the accent rail was not rendering at all. `@layer components` loses to Tailwind's utilities layer, and the band root carries `border` / `border-[color:var(--border)]`, so the 2px accent silently degraded to a neutral 1px. AGENTS.md already says component classes here are deliberately unlayered; this block was not. Verified by computed style: borderTopWidth 1px/rgb(229,231,235) before, 2px/rgb(11,111,134) after. My own test missed it because it asserted class presence, so ui-accessibility now asserts computed border width and colour, plus the forced-colors thickening to 3px. P2 — differentials: the fault copy told the reader to retry or browse the catalogue directly while the removed error section had taken both actions with it. Restore rerunSearch as onRetry and the catalogue links as faultAction. Verified: 409 files / 4148 unit tests; ui-accessibility 14 passed; ui-tools + ui-formulation + ui-specifiers 99 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbad2a9a9f
ℹ️ 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".
The Retry I added in bbad2a9 called rerunSearch, which only re-runs the parent document-evidence search. useDifferentialSearch keys on query plus auth identity, neither of which changes when the reader asks to try again, so the catalogue request was never reissued and the band stayed faulted. A Retry that cannot retry is worse than no Retry: it promises recovery and silently does nothing. Give the hook a refetch, mirroring useRegistryRecords. A failed request is never written to the cache, so bumping a retry counter in the effect deps is enough to re-request — no eviction needed, and the identity/query clearing guards are untouched. Test asserts the real behaviour Codex asked for: first catalogue request fails, refetch is called, and both kind=diagnosis and kind=presentation are requested again with the band returning to ready. One existing assertion moved from toEqual to toMatchObject, since the hook's return now carries the refetch callback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
src/components/clinical-dashboard/search-results-header-band.tsx (1)
410-435: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the retry against a rejected
onRetry.
await onRetry()has nocatch, so a rejecting recovery callback (several call sites passregistry.refetch/b.retryDatadirectly) escapes as an unhandled rejection while the panel silently returns to the idle "Retry" label with no feedback.finallyonly resets the spinner.🛡️ Proposed fix
onClick={async () => { setRetrying(true); try { await onRetry(); + } catch { + // The fault panel is already the failure surface; a failed + // retry simply leaves it in place rather than crashing. } finally { setRetrying(false); } }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/clinical-dashboard/search-results-header-band.tsx` around lines 410 - 435, Update the retry handler on the AsyncButton in the retry action block to catch rejected onRetry calls, preserving the existing setRetrying(false) cleanup while surfacing the failure through the component’s established error or feedback mechanism instead of allowing an unhandled rejection.src/components/clinical-dashboard/medication-prescribing-workspace.tsx (1)
471-472: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a retry action to the medication catalogue fault panel.
useMedicationCatalogonly returnsdata/loading/error, so this panel has no recovery path. Expose arefetch/retryfrom the hook and pass it intoSearchResultsHeaderBandasonRetry(orfaultAction).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/clinical-dashboard/medication-prescribing-workspace.tsx` around lines 471 - 472, Expose a refetch or retry callback from useMedicationCatalog, then pass it as onRetry or faultAction to the SearchResultsHeaderBand rendering the medication catalogue fault panel. Preserve the existing catalog error, loading, and ready status behavior while enabling users to retry failed requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/search-chrome-behaviour.md`:
- Around line 53-60: Update the typography guidance in the section describing
heading weights to consistently state whether the query and figure both use
weight 580, or only the query is emphasized; align the opening claim with the
explicit weight values and clarify the intended hierarchy for implementers.
In `@src/components/clinical-dashboard/differentials-home.tsx`:
- Around line 919-929: Update the faultBody expression in the catalog fault
panel to branch on catalog.status like faultTitle, omitting retry guidance for
unauthorized responses while retaining the retry and browse guidance for other
failures; keep onRetry behavior unchanged.
- Around line 992-1007: Update the main catalogue-content branching around the
`!best && !catalogFailed` empty-state condition so `catalogFailed` suppresses
the entire results body, preventing the results grid from rendering when `best`
is null. Preserve the existing fault panel as the sole failure UI and leave the
guarded `DifferentialsMobileCompareBar` behavior unchanged.
In `@src/components/clinical-dashboard/document-search-results.tsx`:
- Around line 894-909: Update the record-mode handling in the status/faultBody
logic and the related standalone alert near the corresponding duplicate block so
an unavailableMessage remains surfaced when the ribbon does not carry it.
Preserve recordStatus-based status and valid ribbon counts, but avoid setting
faultBody and alert visibility in a way that drops auth, API, or setup notices
on services and forms record paths.
In `@src/components/clinical-dashboard/favourites-command-library-page.tsx`:
- Around line 1198-1201: Update the empty-results rendering in the favourites
command library page so SearchResultsEmptyState is shown only when
favouritesRegistryStatus is ready. For loading, error, or unauthorized statuses,
suppress the normal “No matches” branch and use the existing recovery action for
faulted states; retain the current empty-state behavior for successfully loaded
results.
In `@src/components/clinical-dashboard/favourites-hub.tsx`:
- Line 50: Update the favourites data handling around useSavedRegistryFavourites
so it retains the returned status alongside items. Render the shared
fault/recovery state for registry errors or unauthorized responses, and only
show “No favourites match” when the registry status is ready and the item list
is empty.
In `@src/components/clinical-dashboard/use-saved-registry-favourites.ts`:
- Around line 27-34: Update useSavedRegistryFavourites to expose and return the
refetch callback from useRegistryRecords, then thread it through the consuming
page into its fault UI so the error state provides the existing retry action.
In `@src/components/therapy-compass/screens/search-screen.tsx`:
- Around line 31-33: Update the search screen body rendering to suppress the
results and “No therapies match those filters” empty state whenever b.error is
set, while preserving the existing loading and successful-results behavior. Use
the existing b.error state and the body rendering logic near the fault panel,
matching the services page’s faulted-state handling.
In `@tests/search-results-band-adoption.test.ts`:
- Around line 151-157: Update the forced-colors validation in the search-results
adoption test to fail when the final `.search-band` selector is absent instead
of returning early. Determine the final forced-colors block’s closing boundary
and assert that the selector’s position falls between its opener and closer,
rather than relying only on opener offset ordering.
In `@tests/ui-accessibility.spec.ts`:
- Around line 620-626: Update the rail style extraction in the band evaluation
to return borderLeftColor instead of borderLeftWidth, then compare rail.color
against that left-border color in the accent-rail assertion. Keep the existing
width and neutral-color checks unchanged.
---
Nitpick comments:
In `@src/components/clinical-dashboard/medication-prescribing-workspace.tsx`:
- Around line 471-472: Expose a refetch or retry callback from
useMedicationCatalog, then pass it as onRetry or faultAction to the
SearchResultsHeaderBand rendering the medication catalogue fault panel. Preserve
the existing catalog error, loading, and ready status behavior while enabling
users to retry failed requests.
In `@src/components/clinical-dashboard/search-results-header-band.tsx`:
- Around line 410-435: Update the retry handler on the AsyncButton in the retry
action block to catch rejected onRetry calls, preserving the existing
setRetrying(false) cleanup while surfacing the failure through the component’s
established error or feedback mechanism instead of allowing an unhandled
rejection.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2e69a80-fdf3-4abf-ae6f-bbef25acf1da
📒 Files selected for processing (24)
docs/search-chrome-behaviour.mdsrc/app/globals.csssrc/components/clinical-dashboard/differentials-home.tsxsrc/components/clinical-dashboard/document-search-results.tsxsrc/components/clinical-dashboard/favourites-command-library-page.tsxsrc/components/clinical-dashboard/favourites-hub.tsxsrc/components/clinical-dashboard/medication-prescribing-workspace.tsxsrc/components/clinical-dashboard/search-results-header-band.tsxsrc/components/clinical-dashboard/universal-search-command-surface.tsxsrc/components/clinical-dashboard/use-saved-registry-favourites.tssrc/components/forms/forms-search-results-page.tsxsrc/components/formulation/formulation-home-page.tsxsrc/components/services/services-navigator-page.tsxsrc/components/therapy-compass/screens/search-screen.tsxsrc/lib/app-modes.tstests/favourites-auth-gate.dom.test.tsxtests/favourites-hub-unavailable-controls.dom.test.tsxtests/mode-menu-prefetch.dom.test.tsxtests/search-results-band-adoption.test.tstests/search-results-header-band.dom.test.tsxtests/ui-accessibility.spec.tstests/ui-chrome-scroll.spec.tstests/ui-phone-scroll.spec.tstests/ui-smoke.spec.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
CodeRabbit found a crash I introduced. `best` is `results[0] ?? null`, and the gate `!best && !catalogFailed` means a faulted catalogue with no results takes neither the loading nor the empty branch — control reaches the results grid, which dereferences `best.id` and `best.kind` unconditionally and throws. That fires on every failed or unauthorized differentials search: exactly the state this branch exists to report truthfully. I had already seen this and misread it. An earlier probe of the failed catalogue path returned zero rendered elements and I recorded that as "the component does not render when the catalogue fails". It was an error boundary swallowing a crashed subtree. `catalogFailed` now short-circuits before the `!best` test. Four more findings in the same family, all confirmed against the code rather than taken on the reviewer's word: - therapy-compass rendered "No therapies match those filters" underneath the fault panel, so the page claimed both failure and emptiness. - favourites (library and hub) treated an empty list as "no favourites" during loading and failure, which reads as data loss rather than an unavailable registry. The hub had also discarded the status entirely. - documents dropped the unavailable-API notice in record mode: the ribbon takes its fault from the registry there, so nothing carried the message. - a rejecting `onRetry` escaped as an unhandled rejection. Also fixed the assertion I added last round to catch the inert rail: it compared `borderTopColor` against `borderLeftWidth` — a colour against a length — so it could never fail. It now compares colour to colour. Verified: 409 files / 4149 unit tests, lint, typecheck. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65ba242837
ℹ️ 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".
…ion gate Two more from Codex, both mine. The count leak I fixed at the band only covered faults. Forms forces `displayedMatches` to [] until the registry is ready, so ResultTabs still asserted "Forms 0" underneath a "Searching…" spine — the same untrue zero, one state over. Loading now suppresses count-bearing controls too. `refetching` deliberately does not: there the prior count is still correct, which is the whole point of that state. The adoption gate matched the bare identifier, so an import, a comment, or a dead reference counted as adoption — a production search route could drop the band while the gate I built to prevent that stayed green. It now requires a rendered `<SearchResultsHeaderBand` element, with a negative fixture asserting that an import, a mention and an alias are all rejected while a mount is accepted. Verified: 409 files / 4150 unit tests, lint, typecheck. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
@cursoragent Inspect the PR and repository state, identify the root cause of every failing CI check, unresolved review comment, merge conflict, and related defect. Apply the smallest robust fix using existing project patterns and best practices, without unrelated changes. Run Bugbot and all relevant lint, type-check, test, build, and CI-equivalent commands, fix any issues found, then resolve only the review comments that are fully addressed. Summarise the root causes, changes made, validation results, and any remaining blockers. |
|
CI/review closeout for #1316 is done on tip Root causes
Changes made
Validation
Remaining blockers
  |
Prettier formatting only, no behaviour change. The unformatted file was failing both `Static PR checks` and the new `ci/circleci: verify` job, which each run `npm run format:check`; `prettier --check .` now reports all matched files clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
…utes Track GET /api/account/favourites failures as loadError so a failed first save cannot mark an already-loaded empty library unavailable. Discover results-band mode homes in the adoption gate (not only /search/ paths), with two-hop import reachability and landing-only allowlist entries. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…hub.com/BigSimmo/Database into claude/top-search-design-mockups-w53znc Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Prettier formatting only, no behaviour change. Second recurrence of the same break: `format:check` is not part of `npm run test`, `typecheck`, or eslint, so a change can report a green suite and still fail both `Static PR checks` and `ci/circleci: verify`, which each run it. Verified on this tip: `prettier --check .` clean, and the full unit suite is 416 files / 4232 passed / 4 skipped — the post-merge run that had not yet been done. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
Heads-up for anyone else pushing to this branch: run The same failure has now broken CI twice — It keeps recurring because
Worth knowing about the CircleCI check specifically: it is new. Verified on the current tip
That last one also closes the gap the previous closeout flagged, which noted the suite was green before the tip merge rather than after it. It is now green after. One red still outstanding and unaddressed: Gitleaks. It is not reporting a secret — the log says Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/therapy-compass/data/use-therapy-data.ts`:
- Around line 68-90: Scope retry waiters to the request key and retry attempt in
retry and settleRetryWaiters, so a pending retry promise is resolved only by the
matching request effect. Prevent a route change from allowing the new request to
settle the old retry, while ensuring the original retry is settled when its
matching request completes; add a test covering navigation during an in-flight
retry.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7995cdbc-521f-4be3-b2b9-de2e58ecbf9e
📒 Files selected for processing (13)
docs/branch-review-ledger.mddocs/outstanding-issues.mdsrc/components/account-data-provider.tsxsrc/components/clinical-dashboard/favourites-hub.tsxsrc/components/clinical-dashboard/saved-registry-favourites-status.tssrc/components/clinical-dashboard/use-saved-registry-favourites.tssrc/components/therapy-compass/bindings.tsxsrc/components/therapy-compass/data/use-therapy-data.tstests/favourites-account-retry.dom.test.tsxtests/favourites-hub-unavailable-controls.dom.test.tsxtests/saved-registry-favourites-status.test.tstests/search-results-band-adoption.test.tstests/therapy-compass-data-recovery.dom.test.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/saved-registry-favourites-status.test.ts
- tests/favourites-account-retry.dom.test.tsx
- tests/search-results-band-adoption.test.ts
- docs/outstanding-issues.md
- src/components/clinical-dashboard/use-saved-registry-favourites.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9662d9b2cc
ℹ️ 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".
…uction UI run ui-overlap.spec.ts:199 passed toHaveCount(1) and then resolved the same header#search locator to 2 elements one statement later, one hidden. A duplicate appearing after a passing count assertion is a stream or hydration artifact by construction, which rules out a static double mount and rules out this branch's CSS and component changes as a cause. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54b302f38d
ℹ️ 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".
The band's fault panel now owns error/unauthorized for the services and forms registry, but RecordRegistryNotice still rendered underneath it, so one failed request produced two adjacent panels saying much the same thing. This is the double-reporting already removed from the standalone services and forms pages, missed on the dashboard path. Loading is deliberately left to the notice: the band only says "Searching…" there and never names the registry, so suppressing it would lose information rather than duplicate it. Checked the test catches the defect rather than merely passing — with the guard removed both fault cases fail on the duplicate copy while the loading case still passes, confirming the suppression is not too broad. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7abc91f19d
ℹ️ 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".
The save and clear paths already call markSessionExpired() on a 401 (lines 177, 200); the load path only stored a generic message. That gap was invisible until this branch added reload and wired Retry to it — Retry re-sends the same rejected authorization header, so an expired token produced a button that could never recover. A control that advertises an action must perform one. markSessionExpired is depended on by identity rather than reached through auth, which would re-run the load effect on every auth-object render; it is a useCallback upstream, so it stays stable. Two tests, both checked against the unfixed code: the 401 case fails without the change, and a 503 case pins that a transient server fault still leaves a signed-in reader signed in rather than bouncing them to a sign-in screen. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
Re: Confirmed the asymmetry before changing anything: The finding is right that this branch created the harm. Before Implementation note: I depend on Two tests, both checked against the unfixed code rather than assumed:
I also had to clear the hoisted Verification on For context on the run before this one: Generated by Claude Code |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Correction: the therapy retry finding is marked addressed but is not fixed
The finding itself stands and I verified it independently earlier: the old effect's Practical consequence: it is now invisible to the Separately:
|
Summary
Testing
|
The row cited seven abbreviated SHAs as the thing to consolidate. All seven are unreachable: git fetch origin <sha> fails for every one, no open PR or branch carries them, and none was in the squash merge. They were authored in a sandbox on a branch named work and never pushed, so the follow-up PRs each commit reported do not exist. A ledger entry whose only pointer is a dead hash is worse than no entry, because it reads as tracked. Rewritten around what is actually true and checkable: the PR #1316 review threads persist and describe each fix with file and line detail, and two of the findings are still live on main — the unscoped Therapy Compass retry waiters at use-therapy-data.ts:68, and the adoption gate returning null for root-path modes at tests/search-results-band-adoption.test.ts:101, which leaves /?mode=prescribing and Documents outside the inventory. Both verified against origin/main rather than inferred. The rest of the batch had already landed independently and is recorded as needing no action. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
Codex pushed back on my own row and was right. retryWaitersRef at
use-therapy-data.ts:68 is genuinely unscoped, but nothing observes the
early settle: useTherapyData lives in the long-lived TcProvider and
requestKey derives only from screen, so it cannot change without the
screen changing. The sole awaiting caller is the band's AsyncButton
inside search-screen.tsx, which unmounts on exactly that transition, and
workspace.tsx uses onClick={b.retryData}, discarding the promise.
I had already rated this below the original Major severity but still
listed it as live with a visible "Retry stops being busy" symptom. That
claim was wrong, and a ledger row asserting a user-visible defect that
cannot occur would have sent follow-up work at unnecessary waiter
keying. The correction is retained rather than deleted, with the
condition that would make it real.
Also links the durable PR #1316 review source, using the repo's pinned
Prettier 3.9.5 rather than a transient npx version.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
* issues: archive five completed rows, delete none Triage pass over all 58 open items found five rows whose work is finished and whose next action is empty. Each moves from Open items to Resolved / archive with its fix evidence and the 2026-07-30 date: - #95 the pr-required aggregate now routes a cancelled result through a shared cancelled_error helper; guarded by seven cases that execute the extracted script, three mutation-proven. The red is deliberately retained, since GitHub counts a skipped required check as passing. - #96 every PR #1316 sub-item is dispositioned: the adoption-gate root-path gap closed on PR #1394, four findings were fixed independently, and the Therapy Compass retry-waiter finding was corrected to not-a-live-defect. - #104 a correction row with no next action - the worker's triple image read is an accepted peak-memory trade-off documented at worker/main.ts:866-869, not debt. Archived so a fourth audit does not re-file it. - #109 the branch sweep refuses on a shallow clone, an indeterminate result is its own failure, and the guard moved into the exported collector so the evidence-pack path fails closed too. - #115 the band adoption gate was redesigned to walk a real reachability graph rather than asking whether a file mentions the band. Nothing is deleted. The ledger contract is archive-only (SKILL.md:44 "Archive, never delete"; this file's own conventions: "rows are archived, not deleted, so the history stays auditable"), so no row qualifies for deletion. Row total is unchanged at 120: 58 -> 53 open, 62 -> 67 archived. Prettier widened the archive Outcome column to fit the new evidence, which repads the other archive rows; git diff --ignore-all-space is 7 insertions / 7 deletions, i.e. the five moved rows plus both separators. Verified: node scripts/check-outstanding-issues.mjs --self-test && node scripts/check-outstanding-issues.mjs -> "Outstanding-issues guard passed: 120 rows (53 open, 67 archived), unique ids, next-id=126 above the highest". npx prettier --check . -> "All matched files use Prettier code style!" Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YdPS2KhKqz2buzsUgmX3c * docs: record PR 1428 review * docs: align issue 109 resolution date --------- Co-authored-by: Claude <noreply@anthropic.com>
3 participants
Summary
main.SearchResultsHeaderBandhad two states — loading or done — and no failure path. When a registry call errored or returned unauthorized, the page handed itmatchCount={0}and it rendered a confident "0 matches". Onservices-navigator-page.tsxthat states there are no crisis services when the search never ran. The same held on medications, differentials, documents, forms and favourites.Option A · Edit 2 — final), which already encoded the fix: a warning rail, "Couldn't search", and no number asserted, held distinct from a genuine zero result which does assert0. The redesign and the safety fix are one change, not two.Six commits, each independently revertible and reviewable in order:
c9c494estatusunion + fault panel on the band. No call site changes, no visual change.54f75e83750784a868e8c26e258a+2Design decisions worth a reviewer's attention
statusunion, not a second boolean.loading && erroris otherwise representable and undefined; the invariant "a faulted search never asserts a number" becomes one guard instead of a per-call-site puzzle. It mirrorsRegistryRequestStatus, so most callers pass a near-identity map. The deprecatedloadingprop stays as a shim so the five pages with no async source need no edit and the nine existing DOM assertions are undisturbed.role="status"—ui-tools.spec.ts:953asserts it is visible on every search route — and the fault panel carriesrole="alert". While faulted the spine goesaria-live="off"so the alert makes the single announcement rather than both speaking.border-top. As an absolutely-positioned bar inside anoverflow-hidden12px-radius card, the corner arc sliced its ends (at y=0 the arc is 12px in, at y=2px it is 5.4px in) so it started short and tapered while the 1px border curved past it. The desktop left-hand vertical bar goes with it.lg→smso portrait tablets stop getting the phone layout, but the rail's overflow, fade mask and trailing spacer stay onlg— at 640–1023px a page with chips, sort, a mobile filter and utility controls can exceed the width, and containing that in a scrollable rail is what keeps the no-horizontal-overflow assertions green.font-weighton named classes inglobals.css, not Tailwind arbitrary values:check:type-scale --strictis a zero gate on arbitrarytext-[Npx], and this is already the idiom in that file. Geist is a variable face, so 470/540/560/580 interpolate rather than snapping to 700.doublefor a fault) because--clinical-accentresolves toLinkTextand would otherwise be indistinguishable from the other borders — that is what keeps a failed search distinct from a successful one when colour is gone. The block is appended last inglobals.css, since at equal specificity a later rule wins and an earlier block is silently overridden while still reading correctly.--warning-borderis not remapped by the token block, so it is pinned explicitly.formswas structural — the band was mounted only when the registry was ready, so a failure removed the header entirely. It is now mounted in every state and the redundantRegistryStatusNoticeis deleted.favouritesneeded a hook change —useSavedRegistryFavouritesdiscarded both underlying registry statuses, so the page could not tell "no favourites" from "could not load favourites". It now returns{ items, status }, folding only the registries it actually requested: a disabled hook sits in its initial state forever and must not read as a failure.Verification
npm run verify:cheap— exit 0. Includescheck:type-scale --strict,check:icon-scale,check:design-system-contract,check:knip,sitemap:check,docs:check-*,lint,typecheck, and the full unit suite (408 files, 4133 passed, 4 skipped).npm run lint,npm run typecheck— exit 0.npm run verify:pr-local— not run;verify:cheapplus the targeted Playwright runs below were used instead.npm run verify:ui— partially run.UI verification not run in full: the pinned Playwright browser build (chromium_headless_shell-1228) is absent from this environment's/opt/pw-browsers, which only has1194.The suite was run against the available build via the config's existingPLAYWRIGHT_CHROMIUM_EXECUTABLE_PATHhook:ui-tools86/87,ui-smoke91/92,ui-formulation+ui-specifiers+ui-accessibility+ui-route-coverage37/37.The two failures are outside this diff and neither file is touched by it:
ui-tools.spec.ts:2362safety-plan — strict-mode violation on duplicate copy inpatient-safety-plan.tsx.ui-smoke.spec.ts:3526document viewer — PDF.js canvas not rendering inpdf-canvas-viewer.tsx; plausibly the browser-build mismatch.A reviewer should re-run
verify:uion the pinned build before merge.npm run eval:retrieval:quality— not applicable. No retrieval, ranking, selection, chunking, or scoring behaviour is touched.npm run check:production-readiness— not run; no clinical workflow, privacy, environment, Supabase, source-governance or deployment behaviour changed.RAG impact: no retrieval behaviour change — this is presentation chrome above the result list. No file under
src/lib/rag/**, clinical-search, retrieval-selection, released-search-order, ranking-config, the eval harness, the golden fixture, or the retrieval RPCs is modified.Risk and rollout
formsgate inversion (the band's mount condition changed) and the breakpoint move (horizontal overflow at 640–1023px). Both are covered by existing Playwright assertions that pass. Thefavouriteshook signature changed, which touched three call sites and three test mocks.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy) — untouched.Notes
One piece of the approved plan is deliberately incomplete, and it is the item to discuss.
The refetch pulse (stale count held visible with a pulsing dot instead of a skeleton) is built in the band and adopted on
formulation, where the "loading" isuseDeferredValuelag over static data — nothing identity-scoped is held across the gap, so it is safe.It is not extended to the auth-backed registries, because every async hook here deliberately clears its data on entering loading:
src/lib/use-registry-records.ts:85—refetch()resets torecords: []use-medication-catalog.ts:76—setState({ data: null, … })use-differential-catalog.ts:133— resets toemptyDifferentialMatchesuse-differential-catalog.ts:122states why: "Auth must clear prior identity's matches immediately (parity with useUniversalSearch)", and:164adds that "a later retype of any prior query cannot resurrect authorized matches." That is a privacy invariant, not an oversight. Holding a stale count across a reload on those surfaces means changing three shared hooks under an identity-and-query guard, with a test pinning that an identity change still clears. That deserves its own commit and its own review rather than riding along here.Also worth flagging: no automated review is currently available on this repository — CodeRabbit is rate-limited and Codex is out of credits, which is how PR #1280 merged with no bot review. Given this diff touches shared chrome rendered by 12 production surfaces, it wants a human reader.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
Generated by Claude Code
Summary by CodeRabbit