feat(app): dashboard section navigation aids (TOC, view options, deep links)#2330
Closed
teeohhem wants to merge 19 commits into
Closed
feat(app): dashboard section navigation aids (TOC, view options, deep links)#2330teeohhem wants to merge 19 commits into
teeohhem wants to merge 19 commits into
Conversation
🦋 Changeset detectedLatest commit: 3b4f678 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
Contributor
E2E Test Results✅ All tests passed • 190 passed • 3 skipped • 1201s
Tests ran across 4 shards in parallel. |
Contributor
Deep Review✅ No critical issues found. The diff adds three opt-in navigation aids behind feature flags / per-user preferences ( 🔵 P3 nitpicks (5)
Reviewers (8): correctness, testing, maintainability, project-standards, kieran-typescript, julik-frontend-races, adversarial, agent-native. Testing gaps:
|
Adds three opt-in helpers for navigating dashboards with many sections, all off by default so existing dashboards look unchanged. - Hash anchors per section: each DashboardContainer renders with id="container-<id>". Hovering a section header reveals a "copy link" icon that copies the current URL plus a #container-<id> fragment (using the shared @/utils/clipboard helper). Loading the page with such a hash auto-scrolls to that section, auto-expanding it first. - View-options menu in the dashboard toolbar: "Collapse all sections" / "Expand all sections" / "Show table of contents" toggle. Batch collapse/expand reuse the existing URL-synced collapse state so the resulting view is shareable. - Sticky in-flow TOC rail: when enabled via the view-options toggle, a 180px column renders beside the dashboard content (Flex sibling, not an overlay) with one entry per section. useScrollSpy highlights the active section as the user scrolls; click jumps. Hidden below the md breakpoint. The "tocVisible" preference is per-user in localStorage. Core hook lives in src/hooks/useDashboardSectionNav.ts and exposes scrollToContainer, copySectionLink, collapseAll, and expandAll.
The menu's items (collapse-all / expand-all / show TOC) are all section-scoped and behave as no-ops on dashboards with zero containers. Hide the toolbar entry-point in that case so it does not surface unhelpful UI on sectionless dashboards. The TOC itself already short-circuits when containers is empty.
DBDashboardPage owned a subscription on the 'collapsed' and 'expanded' URL params via useQueryState; useDashboardSectionNav independently subscribed to the same two keys. The hook now receives setters from the caller instead so DBDashboardPage is the sole subscriber per key. Eliminates a potential interaction between concurrent nuqs subscribers during initial hydration on the saved dashboard route, where the expected post-hydration re-render to populate router.query.dashboardId could otherwise be delayed. Behaviour of collapseAll / expandAll / scrollToContainer is unchanged; tests updated to inject the setters.
teeohhem
force-pushed
the
dashboard-toc-navigation
branch
from
89682c4 to
2b6e5ee
Compare
karl-power
previously approved these changes
May 26, 2026
Reverts DBDashboardPage.tsx to origin/main while keeping all other dashboard-toc-navigation changes (new files, DashboardContainer id + copy-link prop, useUserPreferences tocVisible field). If the saved dashboard refresh bug disappears with this commit, the cause is somewhere in the page-level wiring (hooks, Flex wrap, mount points, hash effect); re-introduce piece by piece after confirming.
Smallest possible addition on top of the previous bisect commit (DBDashboardPage at origin/main). If the saved-dashboard refresh bug returns with just this single subscription, confirms the cause is the Jotai atomWithStorage hydration racing with router.isReady transition.
useUserPreferences was confirmed safe in step 1. Adding the only other top-level useEffect my changes introduced: the hash-on-load listener. Body stripped of scrollToContainer dependency so this is the bare effect+listener shape. If the bug returns, the cause is this effect's interaction with router state during hydration.
useUserPreferences (step 1) and hash-on-load useEffect (step 2) both
confirmed safe. Now wrapping the dashboard content area in
<Flex gap='md' align='flex-start' mt='sm'> with a <Box flex={1} miw={0}>
inner wrap — the structural CSS change from my changes, no TOC sibling.
If the bug returns, the cause is this layout structure (likely the
flex+miw interaction with ReactGridLayout or with router-time effects).
useUserPreferences (step 1), hash useEffect (step 2), Flex wrap (step 3) all confirmed safe. Last suspect: the useDashboardSectionNav hook call. It's only useCallback wrappers but it's the only remaining piece of my changes. If the bug returns, the cause is something subtle about the hook (likely useCallback identity churn affecting downstream effects).
…Container dep) Steps 1-4 individually safe. Hypothesis: the bug needs the FULL hash effect with scrollToContainer in both body and dep array. If nuqs v1 setters are unstable across renders, scrollToContainer's identity changes each render and this effect re-runs constantly, thrashing hashchange listener attach/detach during the router hydration window. If the bug returns with this commit, the cause is the effect+ identity-churn interaction.
Reverts the bisect debug commits' page-level changes back to the full feature state (matches b2f5f6d's DBDashboardPage). Bisect inconclusive: none of the individual additions reproduced the original bug in a fresh-session test, and we've concluded the original report may have been state-dependent (browser localStorage / React Query cache from pre-fix builds). If the bug returns in this commit, the cause is in the remaining surface area (tocVisible/toggleToc derivation, tocContainers memo, DashboardViewOptions + DashboardTOC mounts, onCopyLink forwarding).
useScrollSpy's `reinitialize` callback in Mantine v9 changes identity every render. With `reinitialize` in the effect's dep array the effect re-fires each render → reinitialize schedules a state update inside useScrollSpy → component re-renders → reinitialize gets a new identity → effect fires again. Infinite loop, 'Maximum update depth exceeded', and React never reaches the post-hydration commit that would populate `router.query.dashboardId`. The visible symptom on the dashboard page: refreshing a saved-dashboard URL leaves the page stuck rendering the 'Temporary Dashboard' banner, and sidebar nav clicks register but don't navigate. Manifests only when `tocVisible` is true in user preferences (i.e. after the user toggled the TOC on once); fresh sessions never hit it. Fix: drop `reinitialize` from the dep array. The effect still re-runs when the container id signature changes, which is what we actually need.
Toggling the sticky TOC mounts/unmounts a 180px Flex sibling, shrinking or growing the tile-grid column on the left. ReactGridLayout's WidthProvider listens for window resize events to recompute column widths; without one of those, RGL keeps the old pixel widths, so tiles overflow under the TOC when toggling it on and leave a gap on the right when toggling it off. Persists until the next full page refresh. Fix: dispatch a synthetic 'resize' event whenever tocVisible flips so WidthProvider re-measures and re-lays out the grid against the new available width.
The app layout puts page content inside `<div id="app-content-scroll-container" style="overflow-y: scroll">`, so the dashboard scrolls inside that inner div — the window does not scroll at all. Mantine's `useScrollSpy` defaults to listening on `window`, so it never received scroll events from the dashboard and the active-section indicator (the accent left-border on TOC entries) stayed pinned to the first section forever. Click navigation worked because that uses scrollIntoView on a known element id, no scroll listener needed. Fix: resolve #app-content-scroll-container after mount and pass it as useScrollSpy's `scrollHost` so the spy listens on the right element.
useScrollSpy picks the section whose top is closest to the viewport top. That has a dead zone: when the user clicks a section that is already visible but cannot be scroll-pinned at the top (e.g. the last section in a short dashboard where the bottom hits the scroll end before the section's top reaches the viewport top), the spy never updates and the highlight stays on whatever was previously active. Fix: treat clicks as explicit user intent that overrides the spy. On click, set a clickedId; the entry highlights immediately. Wait ~700ms for the smooth-scroll triggered by the click to settle, then arm a one-time scroll listener that clears the override on the next user-initiated scroll. Result: clicks always highlight what was clicked; the spy seamlessly resumes once the user scrolls manually.
Replaces the Mantine useScrollSpy + click-override + scroll-host plumbing
with a single IntersectionObserver effect. Active = section with the
highest intersection ratio with the viewport, computed against the
dashboard's actual scroll container (#app-content-scroll-container).
Why this is better:
- One rule replaces three layers of workarounds:
* useScrollSpy reinitialize identity churn (caused an infinite loop
that suppressed the post-hydration router commit on the dashboard
page; previously fixed by removing reinitialize from deps).
* useScrollSpy listening on window when the layout actually scrolls
inside an inner div (previously fixed by passing scrollHost).
* 'click-overrides-spy' timer + one-shot scroll listener to paper
over the spy's 'closest-to-viewport-top' dead zone when the
clicked section can't be pinned at the top.
All three concerns disappear: IntersectionObserver tracks visibility
directly, the clicked section is naturally most-visible after
scrollIntoView lands, and the observer.root is the scroll container.
- Matches user intuition. 'Highlight what I'm looking at' = the section
with the most pixels on screen, not 'whichever heading top is closest
to viewport top'.
- Fewer hooks (useState x1, useEffect x1) replaces (useState x2,
useEffect x3, useRef x1, useCallback x1, useScrollSpy x1). ~40
fewer lines including the comment block.
Tests updated: jsdom does not implement IntersectionObserver, so the
suite installs a tiny fake that records the most recent observer and
exposes a manual `trigger` for asserting active-state transitions
across simulated visibility changes.
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 free
to 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
Adds three opt-in dashboard navigation aids for dashboards that have grown past comfortable scroll length. All three are off by default — existing dashboards look unchanged for users who never open the new menu.
DashboardContainerrenders withid="container-<id>", and hovering a section header reveals a "copy link" icon that writes<current-url>#container-<id>to the clipboard (via the shared@/utils/clipboardhelper). Loading the page with such a hash auto-scrolls to that section, auto-expanding it first.?collapsed=…&expanded=…), so a compact view is shareable like any other dashboard state.mdbreakpoint. Visibility is a per-user preference inuseUserPreferences.Core logic lives in
src/hooks/useDashboardSectionNav.ts(scrollToContainer,copySectionLink,collapseAll,expandAll). New components areDashboardTOCandDashboardViewOptions.DashboardContainergained an optionalonCopyLinkprop and a stableidattribute on its root.Screenshots or video
Before
After
dashboardtoc.mp4
How to test on Vercel preview
Preview routes: `/dashboards/[id]` (any existing dashboard with 4+ sections)
Steps:
References
Reviewer notes (pre-push fanout findings)
Ran `ce-correctness`, `ce-maintainability`, `ce-adversarial`, `ce-kieran-typescript`, and `ce-testing` reviewers in parallel before pushing. Triaged each finding as introduced/amplified/pre-existing.
Fixed before push (P1 / P2):
Known follow-ups (P2, not fixed in this PR):