feat: add sidebar scroll position persistence by ubay1 · Pull Request #75 · nodejs/learn

ubay1 · 2026-05-21T01:13:29Z

Add useScroll hook with debounced scroll event handling
Add useScrollToElement hook to save/restore scroll position via localStorage and in-memory NavigationStateContext
Add NavigationStateProvider context for session-level state
Update Sidebar component to use useScrollToElement with a useLayoutEffect workaround (SideBar lacks forwardRef support)

#74 fix sidebar scroll position persistence

- Add `useScroll` hook with debounced scroll event handling - Add `useScrollToElement` hook to save/restore scroll position via localStorage and in-memory NavigationStateContext - Add `NavigationStateProvider` context for session-level state - Update `Sidebar` component to use `useScrollToElement` with a `useLayoutEffect` workaround (SideBar lacks forwardRef support)

vercel · 2026-05-21T01:13:34Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
nodejs-learn	Ready	Preview	Jul 5, 2026 2:01am

cursor · 2026-05-21T01:13:36Z

PR Summary

Low Risk
Navigation UX and client-side storage only; the aside query workaround could mis-target if multiple asides exist on a page.

Overview
Adds sidebar scroll position persistence so the learn docs nav keeps its scroll offset when users move between pages or refresh.

The app layout is wrapped in a new NavigationStateProvider that holds in-memory scroll state for the session. useScroll debounces scroll events on a DOM ref; useScrollToElement restores position on mount (context first, then localStorage under navigationState:sidebar) and saves {x, y} on scroll, with safe fallbacks when storage is unavailable.

The Sidebar wires this up via useScrollToElement('sidebar', …). Because the shared SideBar UI component does not forward refs, a useLayoutEffect assigns the ref from document.querySelector('aside') so the scroll listener attaches before useScroll runs.

RemoteLoadableBanner is removed from the main layout in the same change.

^{Reviewed by Cursor Bugbot for commit 8e57f94. Bugbot is set up for automated code reviews on this repo. Configure here.}

github-actions · 2026-05-21T01:13:38Z

👋 Codeowner Review Request

The following codeowners have been identified for the changed files:

Team reviewers: @nodejs/nodejs-website

Please review the changes when you have a chance. Thank you! 🙏

…gationStateProvider

RushilJalal · 2026-07-04T07:52:36Z

Kindly merge asap, this is something which is actively causing problems for me while reading docs.

AugustinMauroy · 2026-07-04T15:52:44Z

I didn't recall correctly @nodejs/nodejs-website but don't we solve that on ui component ?

ubay1 · 2026-07-05T01:48:21Z

Hi @AugustinMauroy,

Just to clarify, since the website currently triggers a full browser reload on page navigation instead of behaving like a Single Page Application (SPA), all component states are completely wiped out between pages.

Because of this full reload, we can't solve it purely within a UI component's lifecycle or state. We actually need to persist the scroll position across sessions using localStorage, which is why I implemented it using a custom hook to handle this side-effect.

Let me know what you think!

Signed-off-by: Ubay Dillah <33889018+ubay1@users.noreply.github.com>

cursor

Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

^{Reviewed by Cursor Bugbot for commit 8e57f94. Configure here.}

cursor · 2026-07-05T02:03:29Z

@@ -1,13 +1,13 @@
 import TableOfContents from '@node-core/ui-components/Common/TableOfContents';
 import Article from '@node-core/ui-components/Containers/Article';
-import RemoteLoadableBanner from '@node-core/doc-kit/src/generators/web/ui/components/AnnouncementBanner/RemoteLoadableBanner';


Announcement banner removed accidentally

Medium Severity

The layout change drops RemoteLoadableBanner and its import while the PR only describes sidebar scroll persistence. Learn pages no longer render the remote announcement banner that previously appeared site-wide.

^{Reviewed by Cursor Bugbot for commit 8e57f94. Configure here.}

cursor · 2026-07-05T02:03:29Z

+      // Clear any pending debounced calls
+      if (timeoutRef.current) {
+        clearTimeout(timeoutRef.current);
+      }


Debounced scroll not saved on exit

Medium Severity

When the scroll listener unmounts, cleanup clears the debounce timer without persisting the latest scrollTop. A user who scrolls the sidebar and follows a doc link within the debounce window keeps a stale position in localStorage, so restore on the next page can be wrong.

^{Reviewed by Cursor Bugbot for commit 8e57f94. Configure here.}

cursor · 2026-07-05T02:03:29Z

+  // listener, sidebarRef.current already points to the real <aside> element.
+  useLayoutEffect(() => {
+    sidebarRef.current = document.querySelector('aside');
+  }, []);


Wrong aside targeted globally

Medium Severity

Scroll persistence binds to document.querySelector('aside'), the first aside in the document. The layout renders NavBar (with a mobile navigation toggler) before the doc SideBar, so scroll save/restore may attach to the navbar drawer instead of the learn sidebar.

^{Reviewed by Cursor Bugbot for commit 8e57f94. Configure here.}

MattIPv4

👀 Can't say I'm overly keen on this as a solution to the problem stated in the issue -- this feels overly complex and thrashes localStorage.

Can't we solve the original issue stated by having the sidebar always scroll the active item into view on load? This should maybe be done upstream in doc-kit, as https://beta.docs.nodejs.org suffers from the same issue?

ubay1 · 2026-07-19T13:09:45Z

👀 Can't say I'm overly keen on this as a solution to the problem stated in the issue -- this feels overly complex and thrashes localStorage.

Can't we solve the original issue stated by having the sidebar always scroll the active item into view on load? This should maybe be done upstream in doc-kit, as https://beta.docs.nodejs.org suffers from the same issue?

this only takes up a little localstorage, it seems fine. but if you want to change it from dock-kit, that would be better.

MattIPv4 · 2026-07-19T13:12:47Z

👍 I didn't complain about the size of what's being put in localStorage, I am concerned with the fact that this thrashes writes to localStorage and feels like a rather hacky solution to emulate SPA behaviour.

Yes, I'd encourage you to open a PR in doc-kit instead, to have it scroll to the active item on page load, or perhaps ui-components (though my $0.02 is that this is more of an implementation detail that should live in doc-kit, rather than in the component itself in ui-components).

ubay1 requested a review from a team as a code owner May 21, 2026 01:13

vercel Bot deployed to Preview May 21, 2026 01:13 View deployment

cursor Bot reviewed May 21, 2026

View reviewed changes

Comment thread hooks/useScrollToElement.js Outdated

Comment thread hooks/useScrollToElement.js Outdated

Comment thread providers/navigationStateProvider.jsx

vercel Bot deployed to Preview May 21, 2026 01:28 View deployment

fix: remove console.log, scope localStorage key by id, and mount Navi…

ced00da

…gationStateProvider

cursor Bot reviewed May 21, 2026

View reviewed changes

Comment thread hooks/useScrollToElement.js Outdated

vercel Bot deployed to Preview May 21, 2026 01:51 View deployment

fix: remove ref.current from useEffect deps to prevent scroll jump

305c7d0

cursor Bot reviewed May 21, 2026

View reviewed changes

Comment thread hooks/useScrollToElement.js

vercel Bot deployed to Preview May 21, 2026 02:06 View deployment

fix: wrap localStorage.setItem in try/catch in scroll handler

93020dd

ubay1 mentioned this pull request May 21, 2026

Sidebar scroll position resets on navigation, active item can be off-screen nodejs/nodejs.org#8828

Open

Merge branch 'main' into fix/persist-sidebar-scroll-position

8e57f94

Signed-off-by: Ubay Dillah <33889018+ubay1@users.noreply.github.com>

vercel Bot deployed to Preview July 5, 2026 02:01 View deployment

cursor Bot reviewed Jul 5, 2026

View reviewed changes

MattIPv4 reviewed Jul 19, 2026

View reviewed changes

Uh oh!

Conversation

ubay1 commented May 21, 2026

Uh oh!

vercel Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursor Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

github-actions Bot commented May 21, 2026

👋 Codeowner Review Request

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RushilJalal commented Jul 4, 2026

Uh oh!

AugustinMauroy commented Jul 4, 2026

Uh oh!

ubay1 commented Jul 5, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Jul 5, 2026

Choose a reason for hiding this comment

Announcement banner removed accidentally

Uh oh!

cursor Bot Jul 5, 2026

Choose a reason for hiding this comment

Debounced scroll not saved on exit

Uh oh!

cursor Bot Jul 5, 2026

Choose a reason for hiding this comment

Wrong aside targeted globally

Uh oh!

MattIPv4 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ubay1 commented Jul 19, 2026

Uh oh!

MattIPv4 commented Jul 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vercel Bot commented May 21, 2026 •

edited

Loading

cursor Bot commented May 21, 2026 •

edited

Loading

MattIPv4 left a comment •

edited

Loading