From e6e20219e9067847349d558a80a25c2970ca95a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 09:52:08 +0000 Subject: [PATCH 1/8] fix(settings): unfreeze the desktop settings panel and keep its rail reachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_013vs5TgqziktquHWaseoRp2 --- .../clinical-dashboard/settings-dialog.tsx | 118 ++++++++++++++---- tests/ui-smoke.spec.ts | 52 ++++++++ 2 files changed, 143 insertions(+), 27 deletions(-) diff --git a/src/components/clinical-dashboard/settings-dialog.tsx b/src/components/clinical-dashboard/settings-dialog.tsx index 0a04dd2c91..38581ca222 100644 --- a/src/components/clinical-dashboard/settings-dialog.tsx +++ b/src/components/clinical-dashboard/settings-dialog.tsx @@ -81,6 +81,11 @@ const APPEARANCE_OPTIONS: ReadonlyArray<{ value: ThemePreference; label: string; { value: "system", label: "System", icon: Monitor }, ]; +// The section rail is `hidden lg:flex`, so the scroll-spy has nothing to drive +// below this seam — and phone scroll is a hot path that should not pay for its +// geometry reads. +const settingsRailMediaQuery = "(min-width: 1024px)"; + function sectionDomId(id: SettingsSectionId) { return `settings-section-${id}`; } @@ -116,13 +121,20 @@ export function SettingsDialog({ const closeButtonRef = useRef(null); const guideButtonRef = useRef(null); const scrollRef = useRef(null); + // The title bar is sticky inside the scroll region on every breakpoint, so its + // height is the amount of the scroll port a section would otherwise land + // underneath — both the scroll-spy root and the click-to-scroll offset have to + // subtract it. + const headerRef = useRef(null); + // Section chosen from the rail, held until the reader scrolls for themselves. + const pinnedSectionRef = useRef(null); const settingsEmailInputRef = useRef(null); const { theme, preference: themePreference, setPreference: setThemePreference } = useTheme(); const { preferences, setPreference, resetPreferences } = useAppPreferences(); // Hide-on-scroll for the mobile glass header (phone-gated inside the hook), so // the top goes fully edge-to-edge while scrolling — the same behaviour as the - // app's search bar. Desktop keeps a static in-panel header. + // app's search bar. Desktop keeps its title bar pinned and never hides it. const { hidden: headerHidden, reportScroll } = useScrollHideReporter(); const auth = useAuthSession(); @@ -169,39 +181,66 @@ export function SettingsDialog({ setDataCounts(readDataCounts()); }, []); - // Desktop scroll-spy: highlight the section nearest the top of the scroll - // region so the rail mirrors what the reader is looking at. + /** + * Desktop scroll-spy: the rail highlights the last section whose heading has + * passed under the sticky title bar. + * + * Read from geometry on scroll rather than from an IntersectionObserver. An + * observer callback receives only the entries whose intersection *changed* in + * that batch, so picking "the topmost visible entry" from it picks the topmost + * of a partial set — which is how selecting the last rail item could leave the + * rail highlighting the one above it. + */ + const syncActiveSection = useCallback((container: HTMLDivElement) => { + // A rail click pins its own selection. The last sections are shorter than + // the scroll port, so they physically cannot be scrolled to the marker line + // — geometry alone would answer a click on "Shortcuts" by highlighting + // whichever neighbour happens to sit at the top of the runway's end. + if (pinnedSectionRef.current) return; + if (typeof window !== "undefined" && !window.matchMedia(settingsRailMediaQuery).matches) return; + const maxOffset = container.scrollHeight - container.clientHeight; + // The final section is shorter than the scroll port, so its heading can + // never reach the marker line. The end of the runway belongs to it. + if (maxOffset > 0 && maxOffset - container.scrollTop <= 2) { + setActiveSection(SETTINGS_SECTIONS[SETTINGS_SECTIONS.length - 1].id); + return; + } + const marker = container.getBoundingClientRect().top + (headerRef.current?.offsetHeight ?? 0) + 1; + let next: SettingsSectionId = SETTINGS_SECTIONS[0].id; + for (const element of container.querySelectorAll("[data-settings-section]")) { + if (element.getBoundingClientRect().top > marker) break; + next = element.getAttribute("data-settings-section") as SettingsSectionId; + } + setActiveSection(next); + }, []); + useEffect(() => { - if (!open || typeof IntersectionObserver === "undefined") return; + if (!open) return; const container = scrollRef.current; - if (!container) return; - const sectionEls = Array.from(container.querySelectorAll("[data-settings-section]")); - if (sectionEls.length === 0) return; - - const observer = new IntersectionObserver( - (entries) => { - const visible = entries - .filter((entry) => entry.isIntersecting) - .sort((a, b) => a.boundingClientRect.top - b.boundingClientRect.top); - const next = visible[0]?.target.getAttribute("data-settings-section"); - if (next) setActiveSection(next as SettingsSectionId); - }, - { root: container, rootMargin: "0px 0px -62% 0px", threshold: [0, 0.35] }, - ); - sectionEls.forEach((element) => observer.observe(element)); - return () => observer.disconnect(); - }, [open]); + if (container) syncActiveSection(container); + }, [open, syncActiveSection]); + // Scroll the settings scroll port itself rather than calling + // `target.scrollIntoView()`. `scrollIntoView` walks every scrollable ancestor, + // and an ancestor with `overflow: hidden` is still programmatically + // scrollable — so it used to drag the whole two-column panel up, taking the + // section rail and the close control out of the dialog with it. const scrollToSection = useCallback( (id: SettingsSectionId) => { setActiveSection(id); + pinnedSectionRef.current = id; const container = scrollRef.current; const target = container?.querySelector(`[data-settings-section="${id}"]`); - if (!target) return; + if (!container || !target) return; const prefersReducedMotion = preferences.motion === "reduced" || (typeof window !== "undefined" && window.matchMedia("(prefers-reduced-motion: reduce)").matches); - target.scrollIntoView({ behavior: prefersReducedMotion ? "auto" : "smooth", block: "start" }); + const headerOffset = headerRef.current?.offsetHeight ?? 0; + const top = + container.scrollTop + target.getBoundingClientRect().top - container.getBoundingClientRect().top - headerOffset; + // `behavior: "auto"` defers to the container's `scroll-smooth`, which is + // exactly what reduced motion must not do — ask for "instant" explicitly. + container.scrollTo({ top: Math.max(0, top), behavior: prefersReducedMotion ? "instant" : "smooth" }); }, [preferences.motion], ); @@ -210,10 +249,17 @@ export function SettingsDialog({ (event: UIEvent) => { const el = event.currentTarget; reportScroll({ offset: el.scrollTop, maxOffset: el.scrollHeight - el.clientHeight, source: el }); + syncActiveSection(el); }, - [reportScroll], + [reportScroll, syncActiveSection], ); + // Any scroll the reader drives themselves releases the rail's pinned choice, + // so the highlight tracks reading again from the next scroll event onwards. + const releaseSectionPin = useCallback(() => { + pinnedSectionRef.current = null; + }, []); + async function submitSettingsEmail(event: FormEvent) { event.preventDefault(); if (!settingsEmail.trim()) return; @@ -283,7 +329,14 @@ export function SettingsDialog({ contentClassName="w-full max-w-none border-[color:var(--border-lux)] bg-[color:var(--background)] font-sans shadow-none max-lg:!pb-0 lg:max-w-[940px] lg:bg-[color:var(--surface-lux)] lg:shadow-[var(--shadow-lux)]" bodyClassName="p-0" > -
+ {/* The desktop height must be definite, not `h-auto` + `max-h-`. With an + auto height the single grid row sizes to max-content (the full ~2800px + of settings), overflows the capped container, and is clipped by + `overflow-hidden` — so the scroll column below never overflows its own + box and `overflow-y-auto` never engages. That left desktop settings + unscrollable, with the section rail stretched off the bottom of the + panel. A definite height bounds the row, which bounds the column. */} +