Fix the results-band shelf Clear deleting the search query - #1611
Merged
Conversation
The shelf is labelled "Filtered by" and its trailing Clear was wired to `clearSearch`, which resets EMPTY_SEARCH — including `query: ""`. So Clear silently deleted the search term the user was reading, while the comment beside it claimed the query was deliberately excluded. Adds a filter-only `clearSearchFilters` binding; the sheet's own "Clear all" still resets everything on purpose. Also drops a false justification: the `appliedFilters` memo suppressed `exhaustive-deps` on the claim that `toggleTagFacet` was a stable closure. It was a plain inline function with a new identity every render. Now `useCallback`-wrapped and listed as a real dependency, suppression removed. Restores two tests wrongly deleted alongside the inert scope system, and pins the Clear behaviour with a guard that fails when the fix is reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:28 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BigSimmo
marked this pull request as ready for review
August 4, 2026 13:11
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Uh oh!
There was an error while loading. Please reload this page.
Merged
3 tasks
9 tasks
BigSimmo pushed a commit
that referenced
this pull request
Aug 5, 2026
Resolves the four expected conflicts. #1616's contribution to the three component files is a token migration plus a rename, so each resolution takes this branch's structure and re-applies #1616's change onto it: - search-screen.tsx: keep clearSearchFilters. Both sides had already converged on --text-muted for the className, so only the handler differed, and clearSearch also wipes the query - the defect #1611 fixed on the shelf's Clear. This branch's comment anticipated the merge and pre-matched the className so exactly this one decision was left. - search-results-header-band.tsx: keep this branch's shelf scroll-track and tap-target work; migrate the Funnel glyph this branch introduced off the deprecated --text-soft to --decoration-soft, matching what #1616 did to the equally decorative ChevronsUpDown. - document-search-results.tsx: keep SearchResultsEmptyState; adopt #1616's metadataPill -> metadataPillDensity rename. The import still said metadataPill while the auto-merged bodies already called metadataPillDensity, so taking either side wholesale would not have compiled. Drops the now-unused EmptyState import and migrates three further decorative --text-soft glyphs. - outstanding-issues.md: take main's next-id=246. Ours said 239, which is below main's highest allocated id and fails check:outstanding-issues. docs/design-system/adoption-manifest.json is regenerated, not hand-edited. document-search-results.tsx drops off EmptyState's adoption lists because this branch replaces EmptyState with the shared SearchResultsEmptyState there - a reviewed change with its own announcement rationale, not a fall back to a local panel. The manifest describes the tree, so it now describes the merged one. Verified on the merged result: tsc --noEmit exit 0 (the import break would have failed here), full unit suite 503 files / 5325 tests after the manifest regeneration, decoration-on-text contract 6 passed, lint clean on all three components. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T97Kqdj9Xh1Cubv5ms3KVy
BigSimmo pushed a commit
that referenced
this pull request
Aug 5, 2026
…sign Four conflicts, resolved as the trial merge recorded in the PR body predicted. `search-screen.tsx` reduced to the handler alone, exactly as forecast: both sides had independently arrived at `--text-muted`, so the only disagreement left was `clearSearchFilters` (here) versus `clearSearch` (main). Kept this branch's — #1616 branched before defect #1611's sibling was fixed, and `clearSearch` on a control labelled `Clear` inside a filter row deletes the query the reader is looking at. `search-results-header-band.tsx` (2 hunks) took this branch: main's side is the pre-restructure shelf carrying only the token migration this branch had already applied. `document-search-results.tsx` (3 hunks): the two empty-state hunks took this branch's shared `SearchResultsEmptyState`. The import hunk is a genuine merge — v2 renamed `metadataPill` to `metadataPillDensity`, and the auto-merged body now calls `metadataPillDensity.roomyCompact`, so the import must follow. `EmptyState` is dropped from it because both of v2's call sites are the ones this branch replaced. `docs/outstanding-issues.md`: #1616 minted its own #237 and #238 from the same `next-id`, so both sides landed rows under both numbers. Renumbered *this* branch's to #246 and #247 and bumped the marker to 248, rather than taking one side wholesale — main's rows are already landed and referenced. This is the collision `#156` and `#168` predict; the guard caught it. Two v2 gates then failed on the merged tree and are now satisfied: - `check:design-system-contract` ratchets `textSoftConsumers` per file, and v2 had driven both of these to zero. The four remaining usages here are icons and glyphs, which this branch had deliberately left on `--text-soft` because v1 defined no decoration token. v2 defines `--decoration-soft`, so that constraint is gone and they move there — the tier v2 itself uses for the same nodes. - The generated adoption manifest and COMPONENTS.md maturity section went stale as a result; regenerated with `design-system:adoption:update`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a
BigSimmo added a commit
that referenced
this pull request
Aug 5, 2026
* fix(ui): make the applied-filter shelf reachable and its dead ends readable Four review findings land on one row and interact. F4 — the shelf was a single `overflow-x-auto` row: label, chips, a `flex-1` spacer, then `Clear`. With four or five chips on a phone the spacer collapses and `Clear` sits past the right edge, reachable only by swiping a row whose scrollbar is hidden. That is the same defect the shelf was built to avoid for the chips themselves. The chips now scroll in an inner track and `Clear` is pinned outside it. F5 — the rail fades its edge on overflow and the shelf did not, so a sixth chip simply stopped existing visually. The chip track now carries the same mask on the same overflow condition, via a second instance of the existing `useRailOverflow` hook. F3 — `Clear` was `px-2 py-1`, about 26px beside 48px chips: the row's only global action was also its hardest target. Matched to the chips instead, staying quiet through weight and an underline. 48px rather than the 44px generic tap guidance suggests, because `min-h-11` reintroduces a fixed `ui-smoke` sub-pixel flake and `--spacing-tap` is this repo's floor. F10 — zero-count facets in the documents sheet were dimmed with `opacity-50`, which multiplies against an already-muted foreground and lands at 2.34:1. The disabled state was least readable exactly when it most needed explaining. Replaced with a real muted pair plus a dashed border: 4.72:1, and it survives forced colors, where border-style is preserved and opacity is not. The three facet states are now mutually exclusive branches rather than a base plus an override — `cn` is a plain join, so competing `border-[color:…]` utilities would both reach the DOM and stylesheet order, not intent, would pick the winner. Also rewires the therapy-compass quick-filter row's `Clear` to `clearSearchFilters`. It sits among the filter chips and is labelled `Clear`, but it called `clearSearch`, which wipes the query with them — the sibling of the defect #1611 fixed on the shelf, missed when that was reviewed. The sheet's `Clear all` is deliberately a full reset and is untouched. Shelf label becomes a funnel glyph below `sm` and the wordmark from `sm`: a prefixed chip costs ~215px of a 350px bar, so every character the label spends is a chip the reader cannot see. The group keeps `aria-label="Applied filters"` either way, so both forms are decorative. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * feat(ui): give the filtered-to-zero empty state its relax-the-filter route back F11. The release that made filters real (#1555) is also the one that removed the only copy pointing at relaxing them. The old empty state read "Relax the scope, try an example, or jump to another mode" with a `Clear scope filters (N)` button; both were dead — they keyed off the inert scope system and never rendered — so the change deleted them and left "Try an example, or jump to another mode." A reader who has filtered to zero is offered an unrelated example query and a different mode, never the chips sitting directly above that caused it. `SearchResultsEmptyState` now takes the same `appliedFilters` the shelf renders, plus `onClearFilters` and `onBrowseAll`. When the set is non-empty it leads with `Remove "X"` and `Clear all filters`, and demotes the example and cross-mode routes below a rule — an example query is a different search, and the reader has not finished this one. With nothing applied the current copy is already correct and is untouched. Two things it deliberately does not claim. The heading counts the filters rather than quoting the query, because the query is not what emptied the set and saying so sends the reader to rewrite the one thing that was working. And `Remove "X"` names the last chip without calling it the most recent: `appliedFilters` arrives in group order, not application order, so that would be a claim the data cannot support. Documents and therapy-compass rendered their own bare `EmptyState` for this case — naming the problem while offering no route out of it — so both now use the shared surface. Documents also passes Browse, because when narrowing this result set is not the answer, reaching the whole corpus is. This retires the last `clearSearch` mislabel on therapy-compass. Its empty state had one button labelled `Clear filters` wired to `clearSearch`, which wiped the query too; `Remove "X"` and `Clear all filters` are now separate controls, so each label matches its own action. The guard added in the previous commit tightens to assert exactly one full reset survives that screen — the sheet's `Clear all`, the only control whose label says it clears everything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * feat(ui): rebuild the documents filter sheet around eleven groups F7 — facet buttons were `min-h-7`: 28px targets, packed at `gap-1.5`, on the surface that exists for phones and whose only interactive elements they are. Raised to the tap token, relaxing to 36px from `sm` and 32px from `lg` where a pointer is likely. 48px rather than the 44px the design called for, because `min-h-11` reintroduces a fixed `ui-smoke` sub-pixel flake and `--spacing-tap` is this repo's floor; the sheet has the vertical room. F8 — source type is a `radiogroup` and the facets below are `aria-pressed` toggles, but both rendered as chips of near-identical size, colour and radius, directly adjacent. Nothing said one row replaces and the next accumulates, so the OR-within-group, AND-across-groups model had to be found by experiment. Source type is now a joined segmented control, which reads as one-of on sight, with a `one only` hint for the first time it is seen. The ARIA is unchanged — this is presentation. F9 — eleven groups stacked in one phone column with no collapsing and no search, so reaching Document type meant scrolling past ten sections. Adds a find-a-filter field and collapses groups by default, each carrying its selected count. Both are gated on the same density threshold: a sheet showing two groups that are both shut is a scroll saved that never existed and two taps added that did, so below four groups everything stays open and the heading is not a disclosure control at all. A group holding a selection cannot be collapsed — a closed section silently narrowing the list is worse than the scroll it saves. F12 — Library leaves the utility rail. It sat adjacent to Filter while answering a different question, and it occupied the rail space the pinned Filter needs; it is also the reason the phone rail could overflow at all. It is moved, not removed: the requirement the old comment protected still holds, since the documents action menu routes through `onSearchModeChange`, which clears the query. Both new homes preserve it — the sheet footer under a rule, phrased as reach with the corpus count beside it, and the zero-result state. Also drops the footer's duplicate count. It printed "12 documents" beside "Show 12 documents", spending the sheet's most prominent slot on a number two centimetres to its left; the button carries it, the new meter and readout carry the proportion, and the live announcement moves to `sr-only` so it still speaks as the number changes underneath. The header's `Clear all` becomes `Clear filters`, matching a handler that was already filter-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * feat(ui): collapse the results bar to one line and pin what must not scroll away The band was 123px on a phone to say "12 documents". The utility rail dropped to its own row and that row was ~85% empty, so the height was spent on the layout rather than on anything the reader needed. It is now 58px, 60px from `sm`. F1 — the rationale for moving Filter last was that the right edge is where a thumb already rests, but the spacer that pushed the rail right was `hidden lg:block lg:flex-1`, so it did nothing below 1024px. On every phone the rail was left-packed and Filter sat mid-rail: the change delivered its stated benefit only at the width where thumbs are least relevant. The query now takes the flexible space and the control group is edge-aligned at every width, with no conditional spacer. `mr-auto` does it rather than moving `lg:flex-1` down — auto margins only absorb what is left after flexible lengths resolve, so the wide layout is byte-identical and no breakpoint had to move. F2 — the rail was one `overflow-x-auto` region with Filter as its last child, so the only control carrying filter state was the first to fall off the right edge once a Retry button or a longer sort label appeared. The code's own comment explained that applied-filter chips had been moved out of that rail for exactly this reason. Only the optional controls scroll now; Filter and Retry are pinned siblings outside the track. Retry too, because it is the recovery action in a degraded state — the one control that must never need a horizontal swipe. The state tile is deleted and the full-width accent border becomes a 2 x 18px lead rule inside the padding: at bar height a line across the whole width read as a divider between the composer and the results rather than as the band's accent. That deletion is where the design as drawn had to be extended. The tile carried state as SHAPE — alert when faulted, spinner while running, funnel once filtered — and the mockup replaced it with colour alone, which makes a failed search identical to a successful one for a reader who cannot separate the hues, and contradicts a recorded decision. The tile turned out to carry three jobs and only one needed a tile. Narrowed is now carried by the shelf, which grows the band by a whole labelled row. Running was already inline. Faulted keeps three independent non-chromatic channels: the lead rule doubles from one stroke to two, a CircleAlert renders before the count for non-ready states only, and a faulted band still renders no digit at all. The mark is a `border-left` on a zero-width box rather than a background precisely so that forced colors, which drops backgrounds but maps border colour to CanvasText and preserves border-style, keeps all of it. F12 — Library leaves the rail (moved in the previous commit), which is what makes the arithmetic work: at 390px count + query + Library + Filter does not fit, and without Library it fits comfortably. One line is not safe for every mode, and the mockup was drawn for documents. Six modes pass `MobileResultFilterControl` into `mobileControls` — a `w-full` native select, and formulation and specifiers pass two in a two-column grid — which is unreadable pinned into a 58px line at 320px. `mobileControlsPlacement` therefore defaults to `row` whenever a page passes a phone control and to `inline` when it passes none; documents and therapy-compass opt in explicitly. The default is the safe one so a new mode that forgets the prop degrades to today's layout rather than to an unusable one. Verified at 320/390/430/768/1024/1440 in light, dark and forced-colors with no page horizontal overflow at any of them. `tests/ui-accessibility.spec.ts` is rewritten, not deleted: it caught the accent degrading to a neutral border when Tailwind's utilities layer outranked the component layer, which has actually happened here, and the same failure now shows up as a zero-width box. It gains the fault assertion the border-top version could not make — a border has no style to change — and a guard that its own probe returned real values, because a silently-null measurement reads as a pass. The style-effect contract in `tests/helpers/style-contracts.ts` is retargeted from `search-band` to `search-band-lead` for the same reason. `docs/search-results-bar-decisions.md` is amended in this commit, since two of its records go stale here: the state tile, and Library staying in the bar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * docs(ui): correct two comments the redesign left describing the old band `search-band` no longer paints the accent, and the placement note referenced a variable name that never shipped. Also drops the claim that `refetching` is dimmed "via CSS `data-status`": no such rule exists in globals.css and there is no evidence one ever did, so the sentence described an intention rather than the code. Adding the rule is a visual change across twelve modes and belongs to whoever decides a background refresh should look different — asserting it here while it does not exist is what let it go unnoticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * docs(issues): capture the two follow-ups the results-bar redesign surfaced #237 — the band claimed a `refetching` dimming rule that globals.css has never defined, so a background refresh is signalled by a pulsing dot alone against text that is deliberately identical to `ready`. Corrected the comment in the redesign rather than adding the rule, because adding it is a visual change across twelve modes. #238 — the one-line bar currently reaches two modes plus every mode that passes no phone control. Six pass a `w-full` native select (two of them pass a pair), which is unreadable pinned into a 58px line at 320px, so they keep their own row by default. Widening them is per-mode work, not a shared-band change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * fix(ui): reach the corpus from the sheet, and re-point the test that pinned it Fallout from moving Library off the utility rail, caught by `verify:ui` (`348 passed, 1 failed`) rather than by anything offline. `ui-smoke.spec.ts` asserted the ribbon still carried an "Open source library" button, then used that same button further down to open the Sources drawer and check focus returned to it on Escape. Both are re-pointed at Library's new home in the filter sheet's footer rather than deleted — the ribbon assertion is inverted into an absence, so putting Library back on the rail re-creates the overflow F12 removed and fails loudly instead of passing quietly. The behaviour fix the test exposed: the footer control called `onOpenLibrary` without dismissing the sheet, so the Sources drawer opened underneath a filter panel still covering the results both of them describe. Browsing the corpus is leaving the filter surface, not another thing to do on it, so it now closes the sheet on the way out. That in turn moves where focus lands when the drawer closes: the opener has unmounted with the sheet, so the app falls back to the documents options button. Asserted explicitly, with the reasoning, rather than loosened to "not body" — it is a visible related control in the same workspace and the app's existing restore target, but it is a fallback and the comment says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * docs(ledger): record the results-bar redesign review at 7eb723b Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * fix(ui): keep document filter sheet chrome scoped to the active query Reset the find-a-filter needle and expand set when the search query changes, and keep selected facets visible while the find field narrows the list so an active constraint cannot become unreachable inside the sheet. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix: restore empty-state recovery and rail observation * fix: restore therapy zero-result recovery * fix: keep document filter recovery paths reachable * test: cover announced empty-state recovery * test: cover document filter recovery paths * test: intercept the differential search endpoint * fix: close filter disclosure click handler * fix: use a valid exact search route regex * style: format empty-state regression test * style: format document-search-results.tsx * fix(ui): observe overflow without effect state writes * chore(ui): remove obsolete empty-state import * fix(ui): stop the empty state claiming a second status region The auto-fix for Devin's announcement finding gave `SearchResultsEmptyState` a `role="status"` root. The band already renders one unconditionally on every search route, so `getByRole("status")` became ambiguous everywhere the empty state can appear — which is every mode. Devin's own prompt flagged the risk ("checking it does not collide with the band's own single-status-region assertions"); the collision landed anyway and broke three suites. A bare `aria-live="polite"` announces identically — `role="status"` is just implicit polite + atomic — without adding a node to the role query. `aria-atomic` is deliberately left off: heading and body change together, and re-reading the whole panel on every keystroke is worse than reading what changed. Also repairs three tests that were asserting the wrong things: - The therapy guard counted `b.clearSearch` call sites and required exactly one. An agent then added a correctly-labelled `Clear search` recovery, and the count read it as a regression. The rule was never about head-count — it is that a control wired to `clearSearch` must be *labelled* for clearing the search. Rewritten to assert that, so `onClear`/`onClearSearch` pass and an `onClearFilters` or bare `onClick` still fails. A duplicate copy of the same test carrying the old assertion is removed. - `states the proportion once…` expected `Show 0 documents` from selecting Clozapine + Suicide. The panel deliberately prevents that: once Clozapine narrows the set, Suicide re-counts to 0, becomes a dead end, and dead ends are click-guarded. The test read the feature as a bug. It now asserts the guard — `aria-disabled`, the description, and that the guarded click leaves the count alone. - Two `getByRole("status")` queries in the empty-state tests are now by text, matching the role change above. Two failures in `document-search-record-fault.dom.test.tsx` are untouched here and are not from this commit — verified failing with these changes stashed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * test: follow the zero-match state onto the shared empty state `document-search-record-fault.dom.test.tsx` still expected "No matching documents". That branch adopted `SearchResultsEmptyState` so Library stays reachable from a search that returned nothing — the gap Codex and Devin both raised — which changed the heading to the shared "No matches for <query>". Re-pointed rather than loosened: the assertions still name the exact copy and still distinguish the loading state from the settled one, so a state that stops rendering its heading fails here the way it did before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * fix(ui): keep the documents zero-result title a heading Adopting the shared empty state demoted the main document-search zero-result title from `h3` back to a paragraph, silently undoing #1612 — the release that gave `EmptyState` an optional heading level precisely so the two states owning their region could keep one. Nothing offline noticed; the only signal was `ui-smoke`'s `@critical` journey going red on `getByRole("heading", { name: "No matching documents" })`. `SearchResultsEmptyState` now takes the same opt-in, un-defaulted `headingLevel` as `EmptyState`, for the same reason: most of the twelve modes rendering it sit inside a region whose heading the band already owns, so promoting every title would insert an outline level the page never declared. Documents passes `3` at the call site that owns its region; the inline filtered-to-zero state inside the results grid stays a paragraph. The Playwright assertion follows the new copy but keeps asserting the role and level, and `document-search-record-fault` now pins the same level so the fast gate catches this next time instead of a Chromium journey. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * fix(test): restore the differentials fault interception, and match empty-state copy to its controls Three review findings, one of which was a live CI failure caused by acting on a fourth without running it. `ui-accessibility`'s two fault tests were switched from intercepting `/api/differentials` to `/api/search` on review advice. That page's catalog hook fetches `/api/differentials?kind=diagnosis|presentation` and never touches `/api/search`, so the interception faulted nothing, the band stayed healthy, and both tests hung waiting for a fault panel that could not render — two red `Production UI` shards. Restored, with the endpoint named in a comment so the swap is not made again. The empty state's body told every reader to "try an example, or jump to another mode". `searchCommandSurfaceByMode` is a `Partial<Record<…>>` with no therapy-compass entry, so on that mode the panel renders neither control and the copy named two affordances that were not there. The body is now derived from what the panel actually offers. Also drops a hardcoded neutral colour literal from the lead-rule assertion — the line above it already compares against the probed neutral — and scopes the style-effect contract's selector to `[data-tone="accent"]`, so a degraded live result cannot fail a contract that exists to catch cascade regressions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * fix(ui): move the new search-surface text nodes off the deprecated decoration tier Readying this change for the v2 design system (#1616), which activates the canonical token layer across 224 files and touches four of the same files. `--text-soft` is a deprecated alias of `--decoration-soft`, and `ckb-v2-token-contract` pins it *below* 4.5:1 deliberately, "so the tier cannot be fixed away". TOKENS.md §7 forbids it on any text node. This branch added eight new text-node usages, which under the v1 palette measure a comfortable 4.72:1 and look correct — and under the v2 palette measure 2.99:1. The dead-end facet is the one that matters. Its entire reason to exist is that the previous `opacity-50` treatment measured 2.34:1; putting the replacement on the decoration tier meant the fix held only until the v2 layer activated, at which point the same markup would have been worse than the AA floor again with nothing reporting it. dead-end facet copy on --surface-subtle opacity-50, before this branch 2.34:1 --text-soft v1 / v2 4.72:1 / 2.99:1 --text-muted v1 / v2 7.29:1 / 5.99:1 (7.54:1 v2 dark) Counts, the shelf label, the shelf Clear and the disabled-facet copy move to `--text-muted`; the find-a-filter placeholder moves to `--text-placeholder`, which is the role the existing recipe contract already requires. Icons and glyphs keep `--text-soft` — that is what the tier is for, and v1 defines no `--decoration-soft` to move them to. Two of these lines are the exact lines #1616 migrates, so both sides now make the same change and those hunks merge instead of conflicting. `decoration-on-text.contract.test.ts` gains four cases pinning the tier per surface, verified to fail on the regression and pass when restored. A DOM assertion cannot do this job: jsdom sees the class, not the resolved colour, so it would pass on either token. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * fix(ui): move the therapy quick-filter Clear label off the decoration tier The last text node on this branch still using `--text-soft`. It is a button label, so under the v2 palette it measures 2.99:1; `--text-muted` gives 5.99:1 light and 7.54:1 dark. Found by trial-merging the v2 branch rather than by reading the diff — the conflict hunk showed v2 migrating this exact line while this branch kept the old tier. Matching v2's token choice also shrinks that conflict to the handler alone, which is the part the two branches genuinely disagree about (`clearSearchFilters` here versus `clearSearch` on v2's base). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * fix(ui): restore Library on the record-match path, and stop the phantom collapse Two defects raised in review, both verified against the code and both introduced by this branch. Neither was covered — the existing suites passed before these fixes as well as after, which is why each gets a guard proven by inversion. **Library was unreachable from a services or forms search that matched records but no documents.** Moving Library off the utility rail left it three homes: the sheet footer, the zero-result empty state, and the inline fallback. The footer needs `matches.length > 0`; the empty state needs `recordMatchCount === 0`. The record-match render satisfies neither and returned `null` outright, so on that path the only route to the corpus was the documents action menu — which calls `setQuery("")` and destroys the search being read. That is the precise thing `docs/search-results-bar-decisions.md` requires an in-context route for. The control is now a shared const rendered from both paths, so a fourth branch cannot be added without one. **A group heading reported a collapse it did not perform.** `isOpen` is forced true while the find-a-filter field has text, but the disclosure button stayed mounted and its handler still branched on that forced-true value: tapping it left `aria-expanded="true"`, rotated no chevron, hid nothing — and wrote the group into `collapsed`, so the collapse ambushed the reader later, once the field was cleared and the tap forgotten. While searching, the needle owns what is open, so the heading now renders in its static form. This is the rule the comment three lines above it already stated; the needle case was simply missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a * fix(ui): stabilize results-band review findings on #1615 Keep phone-control row geometry across loading, stop wide selects sharing a shrinkable flex line with Sort, announce empty states without double-speaking filtered zeros, and clear the stale docs/comments the redesign left behind. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(ui): keep the deferred empty-state announce out of the effect body `788b664` added a deferred screen-reader announcement for the query-only empty state — a live region that mounts already populated is silent in most screen readers, so the message has to arrive on a later frame. The mechanism is right; the implementation called `setLiveMessage("")` synchronously in the effect body twice, which `react-hooks/set-state-in-effect` blocks. `Static PR checks` went red on that lint error, and nothing offline caught it: lint is in neither the unit suite nor `tsc`. Both cleared calls were dead anyway. The region renders only when `!filtered`, so the filtered branch had nothing mounted to clear, and that same gate unmounts and remounts the region across the transition, so it starts empty on its own. Only the `requestAnimationFrame` callback now sets state, which is the form the rule allows and the one the deferral needs. CodeRabbit reached the same fix independently on the same head. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qm3i3eLCDcwytzge1sKM4a --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
BigSimmo pushed a commit
that referenced
this pull request
Aug 9, 2026
…lready require jsdom@30 requires Node ^24.15.0, but engines.node said "24.x" and every gate compared majors only. A container on 24.13.0 therefore passed check:runtime and the preinstall hook, then failed `npm ci` with an opaque EBADENGINE for a transitive package. That blocked local verification on PRs #1611, #1697, #1705 and #1740, each worked around by hand. - package.json: engines.node "24.x" -> ">=24.15.0 <25", declaring the floor the dependency tree already imposes. This cannot newly break any environment that currently installs, since jsdom enforces the same floor during `npm ci`. - check-node-engine.cjs (npm preinstall): compare the full version against that range instead of the major, so the failure names the real requirement. Kept import-free — the Dockerfile COPYs this file alone before `npm ci` — with the range restated as a literal and pinned to package.json by test. - check-runtime.ts: add NODE_MINIMUM_VERSION and reject a matching major below it. This gate fronts verify:cheap, :ui, :release and :pr-local. - session-start.sh: provision 24.19.0, and make the install guard floor-aware. The old major-only test meant a cached container already on 24.13 skipped the install entirely, so bumping the pin alone would not have upgraded it. - check-codex-cloud-setup.mjs: accept the range form, still requiring its major to track .node-version. Closes#285. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XZHZjz6kX8Ly1qBvvCg29C
BigSimmo added a commit
that referenced
this pull request
Aug 18, 2026
…e claiming "0 commits ahead" when it isn't (#2117) * fix(claude): make the session-start hook runnable and give the agent config teeth `.claude/hooks/session-start.sh` was committed as mode 100644 while both its siblings were 100755, and it is the only hook registered by bare path rather than through `bash`. Its whole body is gated on CLAUDE_CODE_REMOTE=true, so the one environment it does any work in is a Linux web container — exactly where a non-executable checkout cannot run. It is also the script that provisions the Node 24 the engine floor requires, after npm ci EBADENGINE blocked PRs #1611, #1697, #1705 and #1740. The defect was invisible locally: the primary workstation is a Windows ReFS Dev Drive with core.fileMode=false, so git ignores filesystem permission bits and a local `chmod +x` is a silent no-op. Only `git update-index --chmod=+x` can fix it. Fixed three ways so it cannot recur: the index mode, a `bash "..."` registration that stops the mode being load-bearing, and a contract test. Also in this change: - .claude/settings.json gains a permissions block. AGENTS.md's provider confirmation boundary was prose-only; this encodes it as deny/ask rules, including deny on reading .env* (a staging key leaked on 2026-08-18) and on the Supabase MCP write tools. The repo already learned that prose does not hold here — see the comment in pr-handoff-stop.sh. - check-base-freshness.mjs now emits its stale-base warning on stdout as hook JSON. Every human-readable branch used console.error, and Claude Code injects only stdout into context, so the tripwire never reached the agent. The origin/main fetch also gains a 10s timeout so a hung remote cannot burn the whole SessionStart budget. - clean-worktree.mjs gains list-only `--merged` and `--squashed`. Nothing reclaimed merged worktrees, so 49 accumulated, ~19 GB of duplicated node_modules on a 50 GB Dev Drive. Ancestor detection alone finds 2 of 49 because this repo squash-merges; the patch-id test finds 9. A `confidence:` line distinguishes proven from inferred, because the two are not the same claim and one candidate had 2 of 21 files still differing. - Explicit hook timeouts, a PreCompact hook that asks for /issues capture while the context still exists, and a push format guard that only fires where the .githooks pre-push guard is not wired. Removal stays a separate opt-in throughout; `runWorktreeCleanup()` is byte identical, so verify:preflight is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(tests): stop the session-start hook test failing on every Windows run `tests/session-start-hook.test.ts` asserted that the written env file contains `join(home, ".node24", …)`. On Windows `home` comes from mkdtempSync(tmpdir()) as `C:\Users\…\AppData\Local\Temp\session-start-home-XXXX`, while the hook runs under Git Bash and writes the POSIX view of the same directory, `/tmp/ session-start-home-XXXX`. The assertion therefore failed on every Windows run regardless of the diff under test. That is worse than a red test: it is a red test everyone learns to ignore. It fails inside `npm run test`, which is the last step of `verify:pr-local`, so the whole gate goes red locally for every change and the only way to use it is to decide which failures do not count. Compare the path tail instead. The unique mkdtemp basename still pins the assertion to this test's own HOME, so it loses no strength, and it now holds on both platforms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ledger): record the Claude Code environment review for PR #2113 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(claude): pin the permission boundary and make the PreCompact hook answerable Closes the four items left open when PR #2113 was opened, each with the smallest fix that actually resolves it rather than restating it. 1. The claim that no `allow` rule can reach a provider-backed script was asserted in review and never measured. It is now a test: 36 provider-backed scripts, derived from script names rather than hand-listed, each asserted unreachable through `allow` and covered by an explicit `ask`. Mutation-checked — injecting a broad `Bash(npm run check:*)` allow rule turns 4 of them red. 2. Hook registrations are pinned to invoke through an interpreter rather than a bare path, and to carry an explicit timeout. Bare-path registration is what made the `session-start.sh` mode bug reachable; the mutation check confirms both guards bite. 3. The PreCompact hook's known limit could not be closed by reading code — the installed CLI ships a compiled binary with no inspectable bundle, so whether the platform injects its stdout into model context is not determinable here. Instead of leaving that permanently unverified, the hook now appends one line per firing to a log under the git dir. After the next compaction the log distinguishes "hook never ran" from "hook ran but its output went nowhere", and both answers are actionable. The log lives outside the worktree so it can never be staged. 4. The `newtask` skill said "~40 worktrees". It was 48 on 2026-08-18 and reached 50 during one session, so the number now carries the reason it drifts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(issues): queue the five follow-ups this session could not close Each of these is blocked on something outside the repo, so capturing them is the smallest fix that resolves them — the alternative is that they die with the session context. - P1 rec: PR churn has exhausted both review bots' budgets, so PR #2113 landed with zero automated review and subsequent PRs will too. AGENTS.md already measured the CI half of this cost; this is the second bill and the worse one. Needs a decision on whether the bundling rule gets a gate. - P2 task: nine landed worktrees, ~4.5 GB on a 51%-full Dev Drive, deliberately not removed — re-verification immediately before deletion showed two held unmerged commits despite the scan reporting none minutes earlier, and a third had been switched branches mid-scan by a live session. - P3 task: confirm D:\.npm-cache is a registered Dev Drive trusted cache; fsutil needs elevation and the non-elevated registry fallback reads empty. - P3 task: read the PreCompact hook's own log after the next compaction to settle whether its output reaches model context. The log lives under the worktree's git dir, so check it before cleaning that worktree up. - P3 task: confirm on a real web session that session-start.sh now runs. The mode bug was proven; the failure it would cause on Linux was not, because no container was available. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(clean-worktree): stop the candidate listing claiming "0 commits ahead" when it isn't Every `--squashed` candidate printed "0 commits ahead" while being genuinely ahead of origin/main — by 11, 2, 1 commits on the real fleet. A squash-merged branch keeps its original commits forever, so it stays ahead permanently; what is zero is the count of UNLANDED commits, which is a different claim. The line was therefore stating something a reader could disprove with one `git rev-list`, on a tool whose entire job is to be trusted before a deletion. It now reports both numbers: "11 ahead of origin/main, 0 unlanded commits". Also corrects the comment above the ahead check, which described it as belt-and-braces against the merge test. That is true in ancestor mode, where it is the real gate. In squash mode it is not a second opinion at all — gitAheadUnlandedCount returns 0 for any branch the squash test just accepted, so the check is satisfied by construction and can only fire on a candidate that was already skipped. Keeping it is correct; describing it as independent evidence was not. The raw count is reporting only and never gates, so no candidate set changes. Verified against the live fleet: same 9 candidates before and after, worktree count unchanged, --remove not run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ledger): record the follow-up review for PR #2117 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two review findings from #1555 that merged before they were addressed, plus two tests that were wrongly deleted in that PR and are restored here.
Clearwas deleting the search term. On therapy-compass the shelf's trailingClearwas wired toclearSearch, which callssetSearch(EMPTY_SEARCH)— andEMPTY_SEARCHcontainsquery: "". So a row labelledFiltered bysilently removed the query as well as the filters, while the comment directly above it claimed the query was deliberately excluded because "removing it is not a filter operation". The code asserted the opposite of what it did. This adds a filter-onlyclearSearchFiltersbinding that preservesqueryand rewires the shelf to it. The filter sheet's ownClear allstill resets everything including the query, which is deliberate and documented infilter-sheet.tsx— only the shelf changes.appliedFiltersmemo indocument-search-results.tsxsuppressedreact-hooks/exhaustive-depson the stated grounds thattoggleTagFacetwas "a stable closure overquery". It was a plain inline function declaration with a new identity on every render. The behaviour happened to be correct because the memo's other dependencies change whenever the facet set does, but the justification was wrong, which is worse than no comment.toggleTagFacetis nowuseCallback-wrapped on[query]and listed as a real dependency; the suppression is gone.exposes differential red-flag search termsandkeeps one sm max-height cap per command dropdown placementintests/search-command-surface.test.tswere unrelated to the inert command-scope system that PR deleted, but were cut alongside it. Lint then flagged their now-unused imports, and removing those confirmed the deletion rather than revealing it.Verification
npx tsc --noEmit— exit 0.npx eslinton the three changed source files — exit 0. This is the meaningful lint result for the second finding: the suppression was removed, so the rule now actually evaluates the memo's dependencies and passes.Test Files 4 passed (4),Tests 59 passed (59).onClearFilterstob.clearSearchfails the new guard withAssertionError: expected '"use client";\n\nimport { useId, useS…' to contain 'onClearFilters={b.clearSearchFilters}', and the file returns to1 failed | 14 passed. The guard is real rather than vacuous.npm run verify:pr-localwas not completed. It exits atcheck:installed-lock-parity(playwright: installed 1.62.0 does not match locked 1.62.1), andnpm ci --include=devcannot repair that here: currentmainlocksjsdom@30.0.1, which requires Node^22.22.2 || ^24.15.0 || >=26.0.0, and this container is pinned at Node 24.13.0. CI resolves.nvmrc(24) to the latest 24.x and is unaffected. Flagging rather than reporting a gate that did not run — the wrapper exited 0 while having done nothing past the parity check.npm run verify:uineeds the Playwright browser matched to the lockfile, and this container cannot reach a lockfile-matching install for the Node reason above (Chromium 1194 against the pinned 1228). The change alters no rendered markup, class, or layout — only which callback one existing control invokes — so the Chromium journeys have nothing new to observe. CI runs the full gate against a correct install.eval:rag,eval:quality,eval:retrieval:quality,verify:release,check:supabase-project— not run and not applicable. No retrieval, ranking, ingestion, or provider-backed surface is touched.Risk and rollout
Clearno longer removes the query. Every other clear path is unchanged, including the sheet'sClear alland the empty-state action. TheuseCallbackchange is identity-only;toggleTagFacetcloses over the samequeryit always did.classifyPullRequestFilesagainst the fullorigin/main...HEADdiff returnsclinicalRisk: false,operationalRisk: false,ragRanking: false,ui: true, so no Clinical Governance Preflight and noRAG impact:line are required for this file set.Clearbehaviour and the suppression together. Nothing else depends onclearSearchFilters; it is additive to the bindings type.Notes
origin/mainafter Search results bar: delete the inert scope system, rebuild the filter shelf, and land the bar anatomy #1555 squash-merged and GitHub auto-deleted the head. This PR carries only the follow-up commit; none of Search results bar: delete the inert scope system, rebuild the filter shelf, and land the bar anatomy #1555's history is re-proposed.Cleardefect is the same class as the badge/Clear allsplit fixed earlier on this component — a control doing more than it advertises — which is why the guard added here asserts the wiring rather than only the rendered output.Filter and sort. It is per-page work, since only documents and therapy-compass have sheets.docs/handoff-search-bar.mdonmainrecords this.Generated by Claude Code