fix(settings): unfreeze the desktop settings panel and keep its rail reachable - #1641
Conversation
…reachable On desktop, opening settings left the panel unscrollable, and clicking any section in the rail scrolled the rail, the title bar and the close control out of the dialog with no way to bring them back. Three compounding defects, all at `lg` and up: - The two-column grid used `lg:h-auto` + `lg:max-h-`. An auto-height grid sizes its single row to max-content — the full ~2800px of settings — which overflows the 792px-capped container and is clipped by `overflow-hidden`. The scroll column inside therefore never overflowed its own box, so `overflow-y-auto` never engaged (measured at 1440x900: scrollHeight === clientHeight === 2800). A definite `lg:h-[min(88dvh,840px)]` bounds the row, which bounds the column. Same visual cap as before, since the content always reached it. - `scrollToSection` called `target.scrollIntoView()`, which walks every scrollable ancestor — and an `overflow: hidden` ancestor is still programmatically scrollable. With the real scroller inert it scrolled the clipped grid instead (after clicking Privacy: grid.scrollTop 2008, rail at top -1954). It now scrolls the settings scroll port explicitly, offset by the sticky header, and asks for "instant" under reduced motion — "auto" would have deferred to the container's own `scroll-smooth`. - The desktop title bar was `lg:static`, so reaching a later section scrolled the only pointer-driven way out of settings off the top. It stays sticky at `lg` now, with an opaque panel-surface fill so content passes behind it. Also replaces the IntersectionObserver scroll-spy with a geometry read on scroll. An observer callback receives only the entries whose intersection changed in that batch, so "topmost visible entry" was the topmost of a partial set — which is why selecting the last rail item highlighted its neighbour. A rail click now pins its own selection until the reader scrolls, and the spy is gated behind the `lg` media query so phone scrolling pays nothing for it. Verified in Chromium at 1440x900, 1280x720, 1024x640 and under reduced motion: the grid never clips, the content column scrolls, and all eight rail items land their heading below the sticky bar with the rail and close control in-panel and hit-testable throughout. The new ui-smoke journey fails against the previous `lg:h-auto` layout and passes on this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013vs5TgqziktquHWaseoRp2
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:59 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 (3)
📝 WalkthroughWalkthroughThe settings dialog now uses a definite desktop content height, sticky title bar, geometry-based scroll tracking, direct container navigation, reduced-motion handling, and rail-click pinning. A desktop smoke test covers scrolling, navigation, controls, final-section selection, and reopening behavior. ChangesDesktop settings scrolling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsDialog
participant SettingsContainer
User->>SettingsDialog: click navigation rail section
SettingsDialog->>SettingsContainer: scroll to compensated offset
SettingsContainer->>SettingsDialog: report scroll position
SettingsDialog->>User: update active section
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Aug 6, 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 claude/settings-nav-freeze-desktop-tdzh7z at starting commit 9e6255f; 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:claude/settings-nav-freeze-desktop-tdzh7z, 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. |
A rail click pinned its section so the geometry scroll-spy could not override it — the last sections are shorter than the scroll port and can never reach the marker line. But the pin was only released by wheel, touchmove and keydown on the scroll port. Dragging the native scrollbar changes scrollTop and emits `scroll` alone, so the pin survived and the rail kept pointing at the clicked section while the reader was somewhere else. That is the one interaction this branch just gave the dialog back. Release on position instead: hold while the scroll closes the gap to the clicked section, hold once it arrives, and let go as soon as the position moves again. Input-agnostic, so scrollbar drags, wheel, touch, keyboard and an interrupted animation all behave the same, and the over-broad keydown release (which fired on Tab mid-animation) is gone with it. Also clear the pin when the dialog opens: the Sheet unmounts its children while closed, so the port returns at offset 0, but this component stays mounted and a stale pin would hold the spy inert on the fresh surface. Covers a scrollbar-drag journey and a close/reopen journey in ui-smoke. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013vs5TgqziktquHWaseoRp2
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Codex Review: Didn't find any major issues. 🎉 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". |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/clinical-dashboard/settings-dialog.tsx (1)
378-385: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the panel height to a
@themetoken.The definite height fixes the overflow chain correctly. The value
min(88dvh,840px)is a hardcoded design constant in a component class. If another dialog needs the same bounded panel height, define the value once as a@themetoken insrc/app/globals.cssand reference it here.♻️ Sketch
/* src/app/globals.css */ `@theme` { --size-dialog-panel:min(88dvh,840px); }- <div className="relative grid h-full max-h-full min-h-0 overflow-hidden lg:h-[min(88dvh,840px)] lg:grid-cols-[248px_minmax(0,1fr)]">+ <div className="relative grid h-full max-h-full min-h-0 overflow-hidden lg:h-(--size-dialog-panel) lg:grid-cols-[248px_minmax(0,1fr)]">As per coding guidelines: "Use Tailwind 4
@themetokens in src/app/globals.css and the repository's intentionally unlayered component CSS rather than introducing hardcoded design values."🤖 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/settings-dialog.tsx` around lines 378 - 385, Define the shared bounded dialog height as a Tailwind 4 `@theme` token in globals.css, then replace the hardcoded lg:h-[min(88dvh,840px)] value on the settings dialog panel with the corresponding theme-token utility. Preserve the existing responsive height behavior and overflow layout.Source: Coding guidelines
tests/ui-smoke.spec.ts (1)
1389-1400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the Tailwind class selector with a stable test hook.
closest("[class*='overflow-y-auto']")couples the test to a utility class string. If the class list changes,closestreturnsnull,portScrollablebecomesfalse, and the failure points at scrolling instead of at the selector. The same expression is repeated three times.Add a
data-testidto the scroll container insettings-dialog.tsxand resolve the port once.♻️ Proposed change
<div ref={scrollRef} onScroll={handleScroll} + data-testid="settings-scroll-port" className="relative min-h-0 w-full overflow-y-auto scroll-smooth bg-[color:var(--background)] polished-scroll lg:bg-transparent lg:px-7" >+ const port = settings.getByTestId("settings-scroll-port");+ const scrollState = async () => - settings- .locator("[data-settings-section]")- .first()- .evaluate((section) => {- const port = section.closest<HTMLElement>("[class*='overflow-y-auto']");- const panel = port?.parentElement;+ port.evaluate((element) => {+ const panel = element.parentElement; return { - portScrollable: port ? port.scrollHeight > port.clientHeight : false,+ portScrollable: element.scrollHeight > element.clientHeight, panelClipped: panel ? panel.scrollHeight > panel.clientHeight : true, }; });- await settings- .locator("[data-settings-section]")- .first()- .evaluate((section) => {- const port = section.closest<HTMLElement>("[class*='overflow-y-auto']");- if (port) port.scrollTop = 0;- });+ await port.evaluate((element) => {+ element.scrollTop = 0;+ });Also applies to: 1427-1433, 1446-1452
🤖 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 `@tests/ui-smoke.spec.ts` around lines 1389 - 1400, Replace the repeated Tailwind-based closest("[class*='overflow-y-auto']") lookups in scrollState and the related checks with a stable data-testid on the scroll container rendered by settings-dialog.tsx. Resolve the container once and reuse it for portScrollable, panelClipped, and the corresponding assertions.
🤖 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.
Nitpick comments:
In `@src/components/clinical-dashboard/settings-dialog.tsx`:
- Around line 378-385: Define the shared bounded dialog height as a Tailwind 4
`@theme` token in globals.css, then replace the hardcoded lg:h-[min(88dvh,840px)]
value on the settings dialog panel with the corresponding theme-token utility.
Preserve the existing responsive height behavior and overflow layout.
In `@tests/ui-smoke.spec.ts`:
- Around line 1389-1400: Replace the repeated Tailwind-based
closest("[class*='overflow-y-auto']") lookups in scrollState and the related
checks with a stable data-testid on the scroll container rendered by
settings-dialog.tsx. Resolve the container once and reuse it for portScrollable,
panelClipped, and the corresponding assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0d101e0-b302-4e49-9d75-c0d32825d5b7
📒 Files selected for processing (2)
src/components/clinical-dashboard/settings-dialog.tsxtests/ui-smoke.spec.ts
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #8427 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
…g-review-fixes-db59 Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Re-run the geometry spy on resize/content growth, align marker slack with settle tolerance, and drop important utilities that overrode a11y header fallbacks. Also stabilize the smoke journey with a scroll-port test id and instant scrollTop writes under scroll-smooth. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…op-tdzh7z' into claude/settings-nav-freeze-desktop-tdzh7z Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
BigSimmo
commented
Aug 6, 2026
Review feedback disposition (PR #1641)Pushed on Fixed
Not changed (with reason)
CILatest BugbotNo |
A coalesced rAF can run after a rail click arms the pin but before smooth-scroll moves scrollTop. Strict distance < pin.distance treated that no-progress frame as reader takeover and dropped the pin, so a rapid second click (or an in-flight scroll listener) lost the hold before the animation started. Use <= so equal distance still counts as the click's own scroll. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
BigSimmo
commented
Aug 6, 2026
Review + CI follow-up ( |
| Check | Cause |
|---|---|
| Static PR checks / Safety and config / Production UI (1–3) / Visual baselines | GitHub Actions Failed to resolve action download info → Service Unavailable / Bad Gateway before the job body ran |
Unit coverage (91dbf2cb) | npm error network read ECONNRESET during install |
Earlier CI on 5d664b1 was green for the same settings surface. This push re-triggers CI on e7f380f3.
When content or viewport growth made a pinned section offset unreachable, the ResizeObserver path clamped the offset but left the pre-clamp distance in place. The next scroll frame then saw a larger gap than pin.distance and treated it as reader takeover, dropping the highlight mid-animation. Reset distance (and settled) from the live scrollTop whenever the offset is clamped. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
BigSimmo
commented
Aug 6, 2026
Babysit follow-up ( |
Capture the babysit outcome at d4ecd2b: review threads clear, pin-clamp fix pushed, hosted CI blocked on the GitHub Actions outage rather than a product failure. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Point the sweep record at c31827e so ledger:lookup matches HEAD. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Cancel the coalesced spy rAF when the dialog closes, and ignore a frame whose port is no longer the live scroll ref. Sheet unmounts children on close while SettingsDialog stays mounted, so a frame armed mid-scroll could overwrite the reopen Account reset. Smoke journey now asserts Account is current immediately after reopen. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Babysit / review-and-fix summary (PR #1641)Tip: Fixed
Review notes
Required CIGitHub Actions = major_outage. Checks on this tip are queued/pending (PR policy, mergeability, Semgrep, Gitleaks, CI) — not green, and missing checks while dirty/outaged are not treated as green. No product CI failure to fix locally. No re-run attempted during outage (would not clear the blocker). Local gates (decisive lines)
Residual risks
Merge left to you. |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Record squash-merge content verification after the settings nav freeze PR landed on main. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Summary
Reported: on desktop, opening settings freezes the settings navigation and leaves no way to click back. Reproduced at 1440x900 — the panel does not scroll, and clicking any section in the rail scrolls the rail, the title bar and the close control out of the dialog with no scrollbar to bring them back.
Three compounding defects, all at
lgand up:The panel could not scroll at all. The two-column grid used
lg:h-auto+lg:max-h-[min(88dvh,840px)]. An auto-height grid sizes its single row to max-content — the full ~2800px of settings — which overflows the 792px-capped container and gets clipped byoverflow-hidden. The scroll column inside therefore never overflowed its own box, so itsoverflow-y-autonever engaged (measured at 1440x900:scrollHeight === clientHeight === 2800, no scrollbar, wheel does nothing). Fixed with a definitelg:h-[min(88dvh,840px)], which bounds the row and so bounds the column. Same visual cap as before, since the content always reached it.A rail click scrolled the whole panel away.
scrollToSectioncalledtarget.scrollIntoView(), which walks every scrollable ancestor — and anoverflow: hiddenancestor is still programmatically scrollable. With the real scroller inert it scrolled the clipped grid instead (after clicking Privacy:grid.scrollTop2008, rail attop: -1954). It now scrolls the settings scroll port explicitly viacontainer.scrollTo, offset by the sticky header, and asks for"instant"under reduced motion —"auto"would have deferred to the container's ownscroll-smooth.The desktop title bar was
lg:static. The close control lives in that bar, so reaching a later section scrolled the only pointer-driven way out of settings off the top. It staysstickyatlgnow, with an opaque panel-surface fill so content passes behind it rather than through it. Scroll-hide stays phone-only vialg:translate-y-0.Scroll-spy rewritten from geometry instead of
IntersectionObserver. An observer callback receives only the entries whose intersection changed in that batch, so "topmost visible entry" was the topmost of a partial set — which is why selecting the last rail item highlighted its neighbour. A rail click now pins its own selection until the reader scrolls for themselves, and the spy is gated behind thelgmedia query so phone scrolling pays nothing for its geometry reads.Regression coverage added to
tests/ui-smoke.spec.ts: asserts the settings column owns the overflow and the panel never clips, then walks the last three rail sections checking the rail and close control stay in the viewport.Mobile/phone behaviour is untouched — every layout change is
lg:-scoped, and the phone scroll path takes the same work it did before.Verification
npm run typecheck— cleannpm run lint— cleannpm run test—Test Files 509 passed (509),Tests 5374 passed | 4 skipped (5378)npm run build— passed,Client bundle secret surface check passed.npm run check:rag:fixtures—Offline RAG fixture and manifest validation passed (36 golden cases, 23 suites).New journey via
npm run test:e2e -- tests/ui-smoke.spec.ts --project=chromium -g "desktop settings scrolls its own column"—1 passed (5.1s)against a production build. Confirmed it fails whenlg:h-autois temporarily restored, so it pins the defect rather than the fix.Direct Chromium proof of the reported surface at 1440x900, 1280x720, 1024x640 and under reduced motion: grid never clips, content column scrollable, and all eight rail items land their heading below the sticky bar with the rail and close button in-panel and hit-testable throughout.
npm run test:e2e:pr(theverify:uijourney set) —347 passed,4 failed. The new settings journey passed. Each of the four failures was checked against the pre-fix tree in the same container and is not attributable to this diff:ui-pwa.spec.ts:143(browser-valid manifest / installable icons / root worker) — fails identically at baseline.ui-smoke.spec.ts"document viewer puts the PDF preview first with pinned evidence after it on mobile" — fails identically at baseline, same assertion (expect(pdfScroller.locator("canvas")).toBeVisible()→element(s) not found).ui-smoke.spec.ts"document frame stretches canvas and native owners at phone and desktop" — passes in isolation with this diff applied; same PDF.js-canvas family as the above, failing only under full-sweep parallel load.ui-formulation.spec.ts:234— passes in isolation both at baseline and with this diff; unreachable from a settings-dialog change.The PDF.js canvas failures are very likely this container's Chromium substitution (see below) rather than a product regression — CI's own
Production UIshards are the authority on that, not this local run.Verification not run: npm run verify:pr-local— it fails closed atcheck:installed-lock-parityon pre-existing container drift (playwright: installed 1.62.0 does not match locked 1.62.1), unrelated to this diff. Its remaining steps (check:runtime, format, lint, typecheck, test, build,check:rag:fixtures) were each run directly and are listed above. For the same drift, Playwright was driven withPLAYWRIGHT_CHROMIUM_EXECUTABLE_PATHpointed at the preinstalled Chromium 1194 instead of the locked build.No provider-backed gate was run: no OpenAI, Supabase, or live eval calls.
Risk and rollout
Sheetis unchanged. The main thing to watch is thelgtitle bar now being sticky, which changes what a reader sees behind it when scrolled.Notes
classifyPullRequestFilesreturnsclinicalRisk: false,operationalRisk: false,ragRanking: false,ui: truefor this diff, so no Clinical Governance Preflight and noRAG impact:line are required. No retrieval, ranking, ingestion, source-rendering, document-access, or privacy-behaviour code is touched — the Privacy section of the settings dialog is rendered by this file, but its clear/reset handlers are unmodified.Summary by CodeRabbit
Bug Fixes
Tests