Scroll the table of contents to the active page on client-side navigation - #4534
Open
Cypher-Aura-19 wants to merge 1 commit into
Open
Scroll the table of contents to the active page on client-side navigation#4534Cypher-Aura-19 wants to merge 1 commit into
Cypher-Aura-19 wants to merge 1 commit into
Conversation
…tion ScrollContainer's auto-scroll effect depended on the `active` prop, which the table of contents passes as the constant selector "[data-active=true]". The dependency therefore never changed and the effect only ran on mount: a full page load scrolled the active page into view, but a client-side navigation left the sidebar at its previous offset, hiding the page being viewed. Track the resolved active element in state and key the scroll effect on that element instead, so the existing scroll behavior retriggers whenever the selector starts matching a different item. A MutationObserver scoped to the container keeps it in sync, because the items set their own `data-active` and the group holding the new active item may expand only afterwards — the container itself does not re-render on navigation. Relying on the element's identity means React bails out while it is unchanged, so repeated observer callbacks do not re-scroll and the visitor's own scroll position is kept. Also skip scrolling when the active item is already fully visible, so moving between neighbouring pages no longer shifts the list for no reason. The geometry now lives in scrollIntoContainer.ts so it can be unit-tested without pulling in the component's dependencies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 2b1638c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Cypher-Aura-19
requested a deployment
to
preview-approval
August 22, 2026 19:18 — with
GitHub Actions
Waiting
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
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.
Fixes #4461
What
The sidebar did not scroll the current page into view when navigating client-side. On a full page load the active page was scrolled into view correctly, but after clicking through to a page further down the table of contents the sidebar stayed at its previous offset, so the page you were reading was off-screen.
How
ScrollContainer's auto-scroll effect depended on theactiveprop:TableOfContentspassesactive="[data-active=true]"— a constant selector string. The dependency was therefore identical on every render, so the effect only ever ran on mount. A full page load mounts the container (scroll worked); a client-side navigation keeps it mounted while the items update their owndata-activeattributes, so the effect never re-fired.SiteSectionListpasses a dynamicactive={#${currentSection.id}}and was not affected, which is why the bug was specific to the table of contents.Rather than add a second scrolling mechanism, this retriggers the existing one: the resolved active element is tracked in state and the scroll effect is keyed on that element, so it re-runs exactly when the selector starts matching a different item.
A
MutationObserverscoped to the container keeps that state in sync. It is needed because there is no React signal available here — the active item is identified by CSS selector, the items set their owndata-active, and the container itself does not re-render on navigation. It also covers the case where the group holding the new active item expands (and renders it) only after the navigation. This follows existing usage inuseScrollOverflow.tsandTableOfContentsScript.tsx, and avoids anysetTimeoutor arbitrary delay — the timing comes from React's own lifecycle.Because the effect is keyed on the element's identity, React's
Object.isbailout means repeated observer callbacks for the same active item do not re-scroll, so the visitor's own scroll position is left alone and there are no scroll loops.Two further points:
behavior: 'auto'; it moved toscrollIntoContainer.tsso it can be unit-tested without pulling in the component's dependency graph (the same split ascategorizeVariants.ts). Only the sidebar's own scroll container is moved — the window is never scrolled.No dependencies were added and group expansion state is untouched, so the fixes from #4391 and #4404 are unaffected.
Visual verification
Before
Client-side navigation changes the active page, but the sidebar keeps its previous scroll position. The newly active page remains outside the visible sidebar area.
gitbook-4461-before.webm
After
With this change, the same client-side navigation automatically brings the newly active page into view inside the sidebar.
gitbook-4461-after.webm
Already-visible item
When navigating to an item that is already visible, the sidebar keeps its current position instead of scrolling unnecessarily.
gitbook-4461-visible-item.webm
Testing
Tests actually run:
bun test src/components/primitives/scrollIntoContainer.test.tsinpackages/gitbook— 13 tests pass. They cover the visibility check (fully visible, above, below, clipped by the bottom edge, flush with the edges, measured against the container box rather than the viewport), the centering offsets, andresolveActiveItem— including the regression itself, that a navigation resolves a different element, plus items outside the container being ignored.oxlinton the changed files — no warnings.oxfmt --checkon the changed files — correctly formatted.tsc --noEmitinpackages/gitbook— no new errors (two errors inAdaptiveVisitorContextProvider.tsxandimageFonts.tsare present on an unmodified tree too).These were focused checks, not a full CI-equivalent run of the workspace.
Manual verification was done in Chromium (Playwright) against a harness importing the real
scrollIntoContainer.tshelpers and mounting both the old and new effect, rather than throughlocalhost:3000/url/...— I do not have GitBook API credentials to run the dev server against a published site, so I could not verify against real content. All measurements below are read back from live layout:scrollTop0 → 0, active item at 1200px, not visible (reproduces the bug)scrollTop1121, item at 79–119px of a 200px viewport, visiblescrollTop721, then 1400)scrollTopunchanged, 0scrollTo()callsscrollTop241)window.scrollY0 → 0 while the container scrolled 0 → 1321Worth a maintainer's eye on real content: the
MutationObserverscope, in case there are table-of-contents attribute updates I have not anticipated.