Make document search on demand - #2199
Conversation
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:7 minutes Limit details: You’ve used the included review currently available. Your 87 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughDocument search now uses a dedicated mobile footer composer. Document actions and overview actions open it. Closing resets search state, handles Escape, restores focus, and updates phone footer layout. ChangesDocument search composer
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk:⚪ Minimal · up to This PR makes a localized document-viewer search interaction change, with focused tests and changed journeys passing; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant User
participant DocumentActionsMenu
participant DocumentViewer
participant PhoneSearchComposer
User->>DocumentActionsMenu: Select Search document
DocumentActionsMenu->>DocumentViewer: Open document search
DocumentViewer->>PhoneSearchComposer: Render and focus search input
User->>PhoneSearchComposer: Press Escape or select close
PhoneSearchComposer->>DocumentViewer: Close document search
DocumentViewer->>DocumentActionsMenu: Restore focus
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #12711 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:f69003e982
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Aug 20, 2026
@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. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch codex/document-search-toggle-pr at starting commit f69003e; never publish fixes to a detached or synthetic work branch and never create a stacked pull request. Use the authenticated GitHub connector to commit each approved fix to BigSimmo/Database:codex/document-search-toggle-pr, then verify that the pull request head contains the published commit before reporting success. 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. For a fixed thread, reply with as the first line and as the second line. For a no-code disposition, use followed by . These result markers authorize the workflow to close that exact thread only after it verifies a fixed commit is the pull request head; a local-only commit is not a fix. If publication or verification fails, do not use either result marker, do not claim success, and leave the thread open with the blocker. If human input or new authorization is required, do the same. 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. |
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…emand The document-viewer baseline target masked `.document-viewer-composer` because the composer was always rendered and, being viewport-pinned inside a ~2900px stitched capture, redrew a band of the golden on any unrelated edit (#278). With search now closed by default the composer is not in the DOM at all in the state this target captures, and `assertMaskSelectors` fails a mask that matches nothing — deliberately, so a rename cannot silently stop masking. `classify-visual-baseline-outcome.mjs` scores that as a non-drift failure, so the advisory Visual baselines job would have gone hard red on every main push for a reason unrelated to pixels. The mask is now the sticky header alone; nothing is lost, since a masked region was never compared, and the closed composer's resting layout is now inside the golden. The committed Linux golden drifts with this change (the overview action is "Search document" and the desktop composer clearance is released while closed). That is ordinary pixel drift: advisory, and adopted post-land from the job artifact with `npm run design-system:baselines:adopt`. Also lands the PR #2199 review-ledger record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HoyoNN7nCUVDLaE84k6e6c
…iting a baseline that never ran the job (#2209) * test(guard-push): make gh availability injectable so the in-flight-CI cases can run `inFlightCiGuard` probes for the `gh` binary and fails open before either injected fetcher is consulted. The message-formatting case injected `prViewer` and `runFetcher` but could not inject that probe, so it only ever exercised the code under test on a machine that happened to have `gh` installed: green in CI, red in any bare container, for no product reason. `ghAvailable` now joins the other two as an injectable, defaulting to the real probe. The formatting case and the script's own `--self-test` pass it, and a new case pins the fail-open branch itself — asserting that neither fetcher is consulted when `gh` is absent, which nothing covered before. No behaviour change: production callers pass no options and get `ghIsAvailable`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HoyoNN7nCUVDLaE84k6e6c * ci: give base-branch pushes their own concurrency group so merges stop evicting each other `cancel-in-progress: ${{ github.event_name != 'push' }}` was added to stop a later merge superseding an in-flight main run, and it does that. It does not stop the other way a run dies: GitHub keeps at most ONE pending run per concurrency group, so when merges land faster than a run completes, each newly queued main run cancels the one already waiting. No supersession is involved, so the exemption never sees it, and main kept landing unverified anyway. Observed 2026-08-20: `a1c2ced`, `d745d15`, `97f6142` and `1cc0d29` were all cancelled while a ~70-minute release-browser-matrix held the shared `CI-refs/heads/main` group. A mobile-`/` CLS regression rode through that gap and surfaced hours later on an unrelated PR (#2199), where it read as that PR's failure. It is the same pattern the existing comment already documents from 2026-08-18 (23 of 30 main pushes cancelled) — the earlier fix simply treated the wrong mechanism. Pushes now key concurrency on `github.run_id`, exactly as workflow_dispatch and schedule already do, so a merged commit is never queued behind or evicted by another. `cancel-in-progress` stays: it is the line that states the intent, and it remains the only protection if the group key ever changes. Cost is the one the existing comment already accepts: concurrent main runs, one per merge, each still scoped by the `changes` job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HoyoNN7nCUVDLaE84k6e6c * ci(triage): stop citing a main run that never ran the failing job as a baseline CI is path-scoped, so a docs-only push to main reports `success` with Lighthouse, Production UI and Build all skipped. The triage comment nevertheless printed "Compared with main CI run #N (success)" and labelled the failure "needs investigation", which reads as "main is green for this job" when main had never measured it. That is how the mobile-`/` CLS regression was waved through on PR #2199 — the bot cited run #12662, a docs-only merge that skipped Lighthouse entirely — and it is the trap ledger #5DYBQQ already recorded from the other side. The baseline's jobs are now read whatever its conclusion, not only when it failed. A failing job the baseline did not execute is classified `unbaselined` and says so in its own words, and the comparison sentence names the jobs that run did not exercise. `classifyFailedJobs` takes the executed-name list as an optional fourth argument: omitted means "could not establish", which keeps the previous classification rather than inventing a verdict — so the `catch` path in the workflow degrades exactly as before. Cost is one extra paginated job listing per triage comment, on a workflow that only runs when a PR's CI has already failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HoyoNN7nCUVDLaE84k6e6c * ci(triage): don't let a cancelled baseline job establish a baseline `executedJobNames` excluded `skipped` by denylist, so every other conclusion counted as evidence — including `cancelled`. A cancelled job completed no verification, so a PR's Build failure would still be reported as `needs-investigation` while citing a main run that never finished that check: the same wrong claim the previous commit set out to remove, arrived at from a different conclusion value. It is also the likeliest one here, since a cancelled main run is this repo's common failure mode rather than a rarity. Inverted to an allowlist — `success`, `failure`, `neutral` — because the two mistakes are not symmetric. Calling a job that ran "unbaselined" only says the comparison is silent there, which costs a reader nothing; calling a job that did NOT run "baselined" implies main covered the failure. An allowlist therefore fails in the harmless direction for `timed_out`, `stale`, `action_required` and any conclusion GitHub adds later, instead of silently treating each new state as evidence. Self-test covers cancelled, timed_out, stale, action_required, an unrecognised future conclusion, and the end-to-end case that a cancelled baseline job yields `unbaselined`. Raised by Codex review on this PR (P2). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HoyoNN7nCUVDLaE84k6e6c --------- Co-authored-by: Claude <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
The prior commit on this branch (7455b14) misdiagnosed the CLS source: it assumed the entrance transform on a freshly-mounted notice card was the cause, but Chromium's Layout Instability API does not score a brand-new element's entrance regardless of which CSS property animates it — confirmed empirically and by review. That fix left the actual mechanism untouched, and Lighthouse budget still failed on this branch's head (3dbf548) after it shipped. Reading the real failing Lighthouse mobile-root report (from CI on PRs #2199/#2204) shows the true source: `.pwa-notice-stack` (`layout-shifts` audit, score 0.223) captured mid-shift displaying the install-prompt card, at the exact box the offline card previously occupied. `.pwa-notice-stack` is `position: fixed`, so when the set of visible cards changes while the stack is already on screen — e.g. the offline card clears the same React commit that reveals the connection-restored/install cards, both driven by one `online` event — the stack's height changes while already painted, which is a real, attributable shift. `useSettledNoticeSignature` forces every transition between two different non-empty card combinations through one fully-unmounted frame, so the stack only ever grows from nothing or shrinks to nothing, never resizes with a sibling card still on screen. Reverts the ineffective opacity-only keyframe change and its source-contract test from the prior commit. Verified with scripts/measure-cls-attribution.mjs (repo tooling from ledger #147) driving real headless Chromium: a synthetic offline→online blip while an install prompt is pending reproduced `.pwa-notice-stack` as a 0.163 shift source pre-fix, matching the CI magnitude; with this fix applied under the identical trigger, `.pwa-notice-stack` no longer appears as a shift source at all. RAG impact: no retrieval behaviour change — this only touches PWA notice-stack rendering and its entrance animation, no rag/, retrieval, or ranking surface.
Summary
Search documentaction across phone and desktop layouts.aria-expanded/aria-controlsstate.Add to scopefrom the document viewer and release phone/desktop footer space whenever document search is closed.Verification
npm run verify:pr-localVerification not run: the current-head attempt was superseded by focused checks; the pre-push static lint/typecheck phase was capacity-blocked by another worktree's exclusive Playwright lease.
npm run verify:uiwhen UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changednpm run verify:uiwas partial before the current-main replay: build passed; 468 passed, 2 skipped, and 6 broad-suite journeys failed under heavy host contention. Every changed document-viewer phone/PWA/search journey passed. The only overlapping failure occurred before the new composer opened and passed 1/1 when rerun alone.npm run verify:releasebefore release or handoff confidence claimsNot run: release confidence was not requested.
Focused evidence:
npm run format— passed on the pushed PR head; no files changed.npm run test -- tests/document-viewer-shell.dom.test.tsx tests/document-detail-performance.test.ts tests/header-scroll-hide-contract.test.ts tests/ui-overlay-css-contract.test.ts— 4 files, 58/58 passed on the current-main PR head.npm run test:e2e -- tests/ui-smoke.spec.ts --project=chromium --grep "search regressions avoid fetch errors and open viewer hits"— 1/1 passed on the pre-replay commit with the same source diff.npm run verify:phone-chrome -- --full=always --files ...— partial before replay: phone contracts 133/133 passed; changed browser selection 174/175 passed. The lone calculator failure passed on untouched baseline and later in the full UI run.npm run typecheck— blocked before execution byDATABASE_HEAVY_RUN_ADMISSION_BUSY; the same source diff typechecked successfully before replay.Risk and rollout
f69003e98.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
origin/main;mainadvanced once more with an unrelated database live-drift commit before push and does not overlap this diff.Summary by CodeRabbit
New Features
Bug Fixes