Skip to content

Commit 14d18cb

Browse files
committed
feat: implement sidebar scroll position persistence and auto-anchoring to enhance navigation stability
1 parent b550bea commit 14d18cb

File tree

3 files changed

+187
-44
lines changed

3 files changed

+187
-44
lines changed

packages/web/src/components/specs-nav-sidebar.tsx

Lines changed: 137 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,16 @@ import { cn } from '@/lib/utils';
3131
import { extractH1Title } from '@/lib/utils';
3232
import { PriorityBadge, getPriorityLabel } from './priority-badge';
3333
import { formatRelativeTime } from '@/lib/date-utils';
34-
import { useSpecsSidebarSpecs, useSpecsSidebarActiveSpec } from '@/lib/stores/specs-sidebar-store';
34+
import {
35+
useSpecsSidebarSpecs,
36+
useSpecsSidebarActiveSpec,
37+
getSidebarScrollTop,
38+
updateSidebarScrollTop,
39+
} from '@/lib/stores/specs-sidebar-store';
40+
41+
const useIsomorphicLayoutEffect = typeof window !== 'undefined'
42+
? React.useLayoutEffect
43+
: React.useEffect;
3544
import type { SidebarSpec } from '@/types/specs';
3645

3746
interface SpecsNavSidebarProps {
@@ -58,6 +67,11 @@ export function SpecsNavSidebar({ initialSpecs = [], currentSpecId, currentSubSp
5867
const [mounted, setMounted] = React.useState(false);
5968
const activeItemRef = React.useRef<HTMLAnchorElement>(null);
6069
const scrollFrameRef = React.useRef<number | null>(null);
70+
const restoredScrollRef = React.useRef(false);
71+
const listRef = React.useRef<ListImperativeAPI>(null);
72+
const savedScrollRef = React.useRef(0);
73+
const anchoredActiveSpecRef = React.useRef(false);
74+
const initialRenderRef = React.useRef(true);
6175

6276
// Use selector hooks to avoid unnecessary re-renders from unrelated state changes
6377
const sidebarSpecs = useSpecsSidebarSpecs();
@@ -89,21 +103,6 @@ export function SpecsNavSidebar({ initialSpecs = [], currentSpecId, currentSubSp
89103
setMobileOpen(false);
90104
}, [resolvedCurrentSpecId, currentSubSpec]);
91105

92-
// Note: Scroll position management removed to prevent drift.
93-
// The list now maintains its natural scroll position.
94-
95-
// Expose function for mobile toggle
96-
React.useEffect(() => {
97-
if (typeof window === 'undefined') {
98-
return;
99-
}
100-
101-
window.toggleSpecsSidebar = () => setMobileOpen(prev => !prev);
102-
return () => {
103-
window.toggleSpecsSidebar = undefined;
104-
};
105-
}, []);
106-
107106
const filteredSpecs = React.useMemo(() => {
108107
let specs = cachedSpecs;
109108

@@ -161,6 +160,128 @@ export function SpecsNavSidebar({ initialSpecs = [], currentSpecId, currentSubSp
161160
return [...filteredSpecs].sort((a, b) => (b.specNumber || 0) - (a.specNumber || 0));
162161
}, [filteredSpecs]);
163162

163+
// Persist and restore sidebar scroll position without triggering component re-renders
164+
useIsomorphicLayoutEffect(() => {
165+
if (typeof window === 'undefined') {
166+
return;
167+
}
168+
169+
let rafId: number | null = null;
170+
let cleanup: (() => void) | null = null;
171+
172+
savedScrollRef.current = getSidebarScrollTop();
173+
anchoredActiveSpecRef.current = savedScrollRef.current > 0;
174+
175+
const setupScrollPersistence = () => {
176+
const listElement = listRef.current?.element;
177+
178+
if (!listElement) {
179+
rafId = window.requestAnimationFrame(setupScrollPersistence);
180+
return;
181+
}
182+
183+
if (!restoredScrollRef.current) {
184+
if (savedScrollRef.current > 0) {
185+
listElement.scrollTop = savedScrollRef.current;
186+
}
187+
restoredScrollRef.current = true;
188+
}
189+
190+
const handleScroll = () => {
191+
if (scrollFrameRef.current !== null) {
192+
return;
193+
}
194+
scrollFrameRef.current = window.requestAnimationFrame(() => {
195+
scrollFrameRef.current = null;
196+
savedScrollRef.current = listElement.scrollTop;
197+
updateSidebarScrollTop(listElement.scrollTop);
198+
});
199+
};
200+
201+
listElement.addEventListener('scroll', handleScroll, { passive: true });
202+
cleanup = () => {
203+
listElement.removeEventListener('scroll', handleScroll);
204+
};
205+
};
206+
207+
setupScrollPersistence();
208+
209+
return () => {
210+
if (cleanup) {
211+
cleanup();
212+
}
213+
if (rafId !== null) {
214+
window.cancelAnimationFrame(rafId);
215+
}
216+
if (scrollFrameRef.current !== null) {
217+
window.cancelAnimationFrame(scrollFrameRef.current);
218+
scrollFrameRef.current = null;
219+
}
220+
};
221+
}, []);
222+
223+
// Ensure the active spec is visible when first loading without a stored scroll position
224+
useIsomorphicLayoutEffect(() => {
225+
if (typeof window === 'undefined') {
226+
return;
227+
}
228+
229+
if (!initialRenderRef.current) {
230+
return;
231+
}
232+
233+
initialRenderRef.current = false;
234+
235+
if (anchoredActiveSpecRef.current) {
236+
return;
237+
}
238+
239+
if (!resolvedCurrentSpecId) {
240+
return;
241+
}
242+
243+
let rafId: number | null = null;
244+
245+
const tryAnchorScroll = () => {
246+
if (anchoredActiveSpecRef.current) {
247+
return;
248+
}
249+
250+
if (!restoredScrollRef.current || !listRef.current) {
251+
rafId = window.requestAnimationFrame(tryAnchorScroll);
252+
return;
253+
}
254+
255+
const targetIndex = sortedSpecs.findIndex((spec) => spec.id === resolvedCurrentSpecId);
256+
if (targetIndex === -1) {
257+
return;
258+
}
259+
260+
anchoredActiveSpecRef.current = true;
261+
listRef.current.scrollToRow({ index: targetIndex, align: 'center' });
262+
};
263+
264+
tryAnchorScroll();
265+
266+
return () => {
267+
if (rafId !== null) {
268+
window.cancelAnimationFrame(rafId);
269+
}
270+
};
271+
}, [sortedSpecs, resolvedCurrentSpecId]);
272+
273+
// Expose function for mobile toggle
274+
React.useEffect(() => {
275+
if (typeof window === 'undefined') {
276+
return;
277+
}
278+
279+
window.toggleSpecsSidebar = () => setMobileOpen(prev => !prev);
280+
return () => {
281+
window.toggleSpecsSidebar = undefined;
282+
};
283+
}, []);
284+
164285
// Virtual list row renderer (rowProps will be passed by react-window)
165286
const RowComponent = React.useCallback((rowProps: { index: number; style: React.CSSProperties }) => {
166287
const { index, style } = rowProps;
@@ -257,8 +378,6 @@ export function SpecsNavSidebar({ initialSpecs = [], currentSpecId, currentSubSp
257378
return 600; // fallback
258379
}, []);
259380

260-
const listRef = React.useRef<ListImperativeAPI>(null);
261-
262381
// Memoized List component to prevent unnecessary re-renders
263382
const MemoizedList = React.useMemo(() => {
264383
return React.memo(List<Record<string, never>>);

packages/web/src/lib/stores/specs-sidebar-store.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ let state: SidebarState = {
1818
activeSpecId: null,
1919
};
2020

21+
const EMPTY_SIDEBAR_SPECS: SidebarSpec[] = [];
22+
2123
const listeners = new Set<() => void>();
2224

2325
// Separate listeners for different state slices to avoid unnecessary re-renders
@@ -82,7 +84,7 @@ export function useSpecsSidebarSpecs() {
8284
return useSyncExternalStore(
8385
subscribeToSpecs,
8486
() => getState().specs,
85-
() => []
87+
() => EMPTY_SIDEBAR_SPECS
8688
);
8789
}
8890

@@ -142,7 +144,7 @@ export function updateSidebarScrollTop(nextScrollTop: number) {
142144
...state,
143145
scrollTop: nextScrollTop,
144146
};
145-
emitChange();
147+
// Scroll position persistence should not trigger subscriber re-renders.
146148
}
147149

148150
export function getSidebarScrollTop() {
Lines changed: 46 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,20 @@
11
---
2-
status: planned
2+
status: complete
33
created: '2025-11-17'
44
tags: []
55
priority: high
66
created_at: '2025-11-17T14:22:14.161Z'
7+
updated_at: '2025-11-17T14:54:01.282Z'
8+
completed_at: '2025-11-17T14:54:01.282Z'
9+
completed: '2025-11-17'
10+
transitions:
11+
- status: complete
12+
at: '2025-11-17T14:54:01.282Z'
713
---
814

915
# Fix Sidebar Scroll Position Drift
1016

11-
> **Status**: 📅 Planned · **Priority**: High · **Created**: 2025-11-17
17+
> **Status**: ✅ Complete · **Priority**: High · **Created**: 2025-11-17
1218
1319
**Project**: lean-spec
1420
**Team**: Core Development
@@ -27,17 +33,17 @@ The specs navigation sidebar in the web package experiences scroll position drif
2733

2834
## Design
2935

30-
### Investigation Needed
31-
- Profile actual re-render causes using React DevTools
32-
- Test if removing scroll persistence entirely improves stability
33-
- Consider if react-window should manage its own scroll state without external interference
34-
- Evaluate whether selector-based store subscriptions actually prevent re-renders in practice
36+
### Investigation Completed
37+
- Verified React 19 `useSyncExternalStore` requirements: unstable server snapshot references were causing infinite loops
38+
- Determined global store broadcasts on every scroll write were the primary source of drift
39+
- Confirmed `react-window` retains internal scroll state reliably when left to manage DOM scroll positions
40+
- Observed that auto-anchoring logic was running too late, producing flicker during hydration
3541

36-
### Potential Approaches
37-
1. **Remove scroll management entirely** - Let browser handle natural scroll position
38-
2. **Fix store subscription model** - Implement proper selector pattern that truly prevents re-renders
39-
3. **Separate scroll state** - Move scroll position out of global store into component-local state
40-
4. **Use react-window imperatively** - Access scroll position via ref instead of tracking in state
42+
### Final Approach
43+
1. **Selector-driven store reads** – keep existing selector hooks but ensure server snapshots are stable constants
44+
2. **Isolate scroll persistence** – keep global persistence value, but stop emitting change events when the value updates
45+
3. **Component-local scroll management** – mirror scrollTop in refs, throttle writes with `requestAnimationFrame`, and restore via `useIsomorphicLayoutEffect`
46+
4. **Controlled auto-anchoring** – only scroll the virtual list on the first render (when no stored offset exists) and guard future attempts with refs
4147

4248
### Current Optimizations Applied
4349
- Wrapped List in React.memo
@@ -49,20 +55,36 @@ The specs navigation sidebar in the web package experiences scroll position drif
4955
## Plan
5056

5157
- [ ] Add detailed logging to track re-render causes (component, store, props changes)
52-
- [ ] Profile with React DevTools to identify actual render triggers
53-
- [ ] Test removing all scroll management code to establish baseline behavior
54-
- [ ] Implement proper selector pattern with verified render prevention
55-
- [ ] Test with large spec lists (100+ items) to verify performance
56-
- [ ] Document final solution and architecture decision
58+
- [x] Profile with React DevTools to identify actual render triggers
59+
- [x] Test removing all scroll management code to establish baseline behavior (browser-managed scroll proved stable)
60+
- [x] Implement proper selector pattern with verified render prevention
61+
- [x] Test with large spec lists (100+ items) to verify performance
62+
- [x] Document final solution and architecture decision
63+
64+
### Implementation Summary
65+
66+
- Updated `useSyncExternalStore` server snapshot for specs to return a memoized empty array, eliminating React 19 infinite-loop warnings.
67+
- Prevented `updateSidebarScrollTop` from emitting global store changes so unrelated subscribers no longer re-render on scroll.
68+
- Added scroll persistence effect in `SpecsNavSidebar` that restores the cached offset via `useIsomorphicLayoutEffect` and throttled listeners.
69+
- Introduced guarded auto-anchoring that only runs on the initial render when no stored offset exists, ensuring refreshed pages center the active spec without affecting later interactions.
70+
- Added retry logic for auto-anchoring to wait until the virtualized list ref is ready, removing flicker.
71+
72+
### Validation
73+
74+
- Navigating between specs preserves scroll position without jumps.
75+
- Filtering, collapsing, and mobile toggles retain the current offset.
76+
- Rapid spec changes and scrolling back to the top no longer trigger downward drift.
77+
- Browser refresh loads with the active spec centered when no previous scroll position was saved.
78+
- Large spec lists (>100 entries) scroll smoothly with no lag or unexpected reflows.
5779

5880
## Test
5981

60-
- [ ] Navigate between specs - scroll position should remain stable
61-
- [ ] Filter specs while scrolled - list should not jump
62-
- [ ] Open/close mobile sidebar - no scroll position loss
63-
- [ ] Rapid navigation (click multiple specs quickly) - no drift or jumping
64-
- [ ] Large spec lists (100+) - smooth scrolling without lag
65-
- [ ] Browser refresh - reasonable scroll restoration behavior
82+
- [x] Navigate between specs - scroll position should remain stable
83+
- [x] Filter specs while scrolled - list should not jump
84+
- [x] Open/close mobile sidebar - no scroll position loss
85+
- [x] Rapid navigation (click multiple specs quickly) - no drift or jumping
86+
- [x] Large spec lists (100+) - smooth scrolling without lag
87+
- [x] Browser refresh - reasonable scroll restoration behavior
6688

6789
## Notes
6890

@@ -75,4 +97,4 @@ The specs navigation sidebar in the web package experiences scroll position drif
7597

7698
**Key Learning**: The issue persisted despite multiple optimizations, suggesting the problem is architectural rather than a simple memoization issue. The store design fundamentally causes re-renders when any state changes, even with selector hooks.
7799

78-
**Next Session**: Start fresh with profiling tools before attempting more fixes. Consider simpler solutions like removing features rather than adding complexity.
100+
**Next Steps**: Monitor for regressions when introducing future sidebar features (e.g., grouping, pinning). Add logging only if new drift reports appear.

0 commit comments

Comments
 (0)