fix: preserve scroll when clicked on a sidebar link#537
fix: preserve scroll when clicked on a sidebar link#537kaulith wants to merge 5 commits intofrappe:developfrom
Conversation
WalkthroughAdds DOM markers ( 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/NestedDraggable.vue`:
- Around line 201-219: The scroll logic never finds an element because no DOM
node sets the data-wiki-item attribute referenced in the watch callback (the
querySelector(`[data-wiki-item="${selectedId}"]`) always returns null); add a
bound attribute on the row element (e.g. set data-wiki-item to either page id or
draft key in the template where the row <div> is rendered) so the lookup can
succeed, remove the redundant await nextTick() inside the watch since { flush:
'post' } already runs after DOM updates, and replace the fragile
closest('.overflow-auto') lookup with a stable reference (use a scroll container
ref or a dedicated data-scroll-container attribute) so isElementInViewport +
scrollIntoView will reliably find the container.
|
Can you add a video @kaulith ? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
wiki/templates/wiki/includes/sidebar.html (2)
290-303: Dual listeners for route changes are redundant and risk double-firing.Both the
wiki-route-changedevent listener (Lines 290-296) and the$watchon$store.navigation.currentRoute(Lines 298-303) perform the same work: updatethis.currentRouteand callexpandCurrentPageParents(). While thenewRoute !== this.currentRouteguard should prevent the second handler from re-executing in most cases, maintaining two identical code paths for the same logical event is a maintenance hazard.Consider keeping only one mechanism. The
$watchon the store is more idiomatic in Alpine and doesn't require a custom DOM event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/templates/wiki/includes/sidebar.html` around lines 290 - 303, Remove the duplicate route-change handling by keeping only the store watcher: delete the explicit window.addEventListener('wiki-route-changed', ...) block and rely on this.$watch('$store.navigation.currentRoute', ...) to update this.currentRoute and call expandCurrentPageParents(); ensure the $watch handler retains the existing guard (newRoute && newRoute !== this.currentRoute) and that expandCurrentPageParents() and this.currentRoute assignments remain unchanged so behavior is identical.
359-403: Retry logic is reasonable but consider a max-attempt bail-out log.The exponential backoff with 8 retries and a 1-second cap is well-designed for handling sidebar animation timing. However, if all 8 retries exhaust without the container becoming measurable, execution silently falls through to the
if (containerHeight === 0) return;on Line 387 with no indication. Aconsole.debugorconsole.warnat that point would help diagnose scroll issues in the field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/templates/wiki/includes/sidebar.html` around lines 359 - 403, In scrollToSelectedItem's inner function attemptScroll, add a diagnostic log before the final early return that happens when containerHeight === 0 so failures after all retries are visible; specifically, detect when retries has reached the max (8) or when (containerHeight === 0 && retries >= 8) and call console.warn (or console.debug) naming the function (scrollToSelectedItem/attemptScroll), the currentRoute, and the retries value, then return; place this log immediately before the existing "if (containerHeight === 0) return;" branch so maintain behavior but surface the failure for sidebarNav/containerHeight measurement issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wiki/templates/wiki/includes/sidebar.html`:
- Around line 273-275: The current code unconditionally sets
history.scrollRestoration = 'manual' (history.scrollRestoration) which is a
global side effect; update the component to save the previous value before
changing it and restore that original value when the component is torn
down/destroyed (or only set it while the sidebar is active), e.g., capture const
prev = history.scrollRestoration before assigning and restore
history.scrollRestoration = prev in the component teardown/unload handler so
other parts of the app or browser navigation behavior are not broken.
- Around line 389-398: The sidebar always recenters the selected item by calling
sidebarNav.scrollTo regardless of visibility; add a visibility check using the
same isElementInViewport utility (or replicate its logic) to test
currentPageElement within sidebarNav before computing scrollPosition and calling
sidebarNav.scrollTo (keep existing Math.max and behavior:'instant'); if
isElementInViewport(currentPageElement, sidebarNav) returns true, skip the
scroll to avoid the unnecessary jump (see NestedDraggable.vue for the exact
visibility check implementation to reuse).
In `@wiki/templates/wiki/macros/sidebar_tree.html`:
- Line 41: The click handler on the sidebar link now always calls preventDefault
because the .exact modifier was removed, which breaks modifier-key "open in new
tab" behavior; restore the original behavior by either re-adding the .exact
modifier to the template's click directive (i.e., change `@click.prevent` to
`@click.prevent.exact` on the element that calls $store.navigation.navigateTo('{{
node.route }}')) or modify the navigateTo method in the navigation store
($store.navigation.navigateTo) to detect modifier keys (e.g., event.ctrlKey,
event.metaKey, event.shiftKey, event.altKey) and return early so the browser
default handling for opening in a new tab/window occurs. Ensure the chosen fix
references the template's click directive and the navigateTo function so the
behavior is preserved for modifier-key clicks.
---
Duplicate comments:
In `@frontend/src/components/NestedDraggable.vue`:
- Around line 226-244: The double nextTick usage around the scroll-to-selected
logic is fragile; replace it with a MutationObserver on the scroll container
(found via selectedElement.closest('[data-wiki-scroll-container]')) that waits
for child nodes/attributes changes and then runs the same logic (compute
escapedId with CSS.escape(selectedId), find selectedElement, check
isElementInViewport and call selectedElement.scrollIntoView({ behavior:
'smooth', block: 'nearest' })); ensure the observer disconnects after a
successful scroll or after a short timeout to avoid leaks, and also add a short
setTimeout retry fallback (same scroll logic) to handle environments where
mutations don't fire in time—keep error handling around the operation as
currently done.
---
Nitpick comments:
In `@wiki/templates/wiki/includes/sidebar.html`:
- Around line 290-303: Remove the duplicate route-change handling by keeping
only the store watcher: delete the explicit
window.addEventListener('wiki-route-changed', ...) block and rely on
this.$watch('$store.navigation.currentRoute', ...) to update this.currentRoute
and call expandCurrentPageParents(); ensure the $watch handler retains the
existing guard (newRoute && newRoute !== this.currentRoute) and that
expandCurrentPageParents() and this.currentRoute assignments remain unchanged so
behavior is identical.
- Around line 359-403: In scrollToSelectedItem's inner function attemptScroll,
add a diagnostic log before the final early return that happens when
containerHeight === 0 so failures after all retries are visible; specifically,
detect when retries has reached the max (8) or when (containerHeight === 0 &&
retries >= 8) and call console.warn (or console.debug) naming the function
(scrollToSelectedItem/attemptScroll), the currentRoute, and the retries value,
then return; place this log immediately before the existing "if (containerHeight
=== 0) return;" branch so maintain behavior but surface the failure for
sidebarNav/containerHeight measurement issues.
…ore ctrl+click new tab behavior
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wiki/templates/wiki/includes/sidebar.html (1)
300-313: Redundant dual route-change detection —$watchalone is sufficient.Both the
wiki-route-changedevent listener (Lines 300–306) andthis.$watch('$store.navigation.currentRoute', …)(Lines 308–313) callexpandCurrentPageParents()on every navigation. The mutual-exclusion guard (newRoute !== this.currentRoute) prevents actual double-execution, but the dual mechanism is purely redundant for all currentnavigateTopaths (regular fetch, prefetch cache, andpopstate).If the custom event is not intended as a public API for external consumers, the listener can be removed:
♻️ Simplification
- window.addEventListener('wiki-route-changed', (event) => { - const newRoute = event.detail?.route; - if (newRoute && newRoute !== this.currentRoute) { - this.currentRoute = newRoute; - this.expandCurrentPageParents(); - } - }); - this.$watch('$store.navigation.currentRoute', (newRoute) => { if (newRoute && newRoute !== this.currentRoute) { this.currentRoute = newRoute; this.expandCurrentPageParents(); } });If
wiki-route-changedis retained as an intentional public event (for future consumers), the dispatch should use the correctroutevalue as described in the previous comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/templates/wiki/includes/sidebar.html` around lines 300 - 313, Remove the redundant window.addEventListener('wiki-route-changed', ...) handler and keep the existing this.$watch('$store.navigation.currentRoute', ...) mechanism to drive updates; specifically delete the listener that reads event.detail?.route and sets this.currentRoute/ calls expandCurrentPageParents(), relying on the $watch to update this.currentRoute and call expandCurrentPageParents() instead, and only retain a separate 'wiki-route-changed' event if you intend it as a public API (in which case ensure any dispatch supplies the correct route value).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wiki/templates/wiki/includes/sidebar.html`:
- Around line 399-404: The retry timers started in scrollToSelectedItem can
leave pending setTimeout/requestAnimationFrame callbacks after component
teardown; modify scrollToSelectedItem to store the timeoutId and rafId in
outer-scope variables (e.g., timeoutHandle and rafHandle) whenever calling
setTimeout/requestAnimationFrame, and update any retry recursion to overwrite
those handles; then update destroy() to clear the scheduled tasks by calling
clearTimeout(timeoutHandle) and cancelAnimationFrame(rafHandle) (and reset the
handles) before restoring scroll restoration so no timers fire after destroy;
ensure attempts still early-return if DOM is gone.
---
Nitpick comments:
In `@wiki/templates/wiki/includes/sidebar.html`:
- Around line 300-313: Remove the redundant
window.addEventListener('wiki-route-changed', ...) handler and keep the existing
this.$watch('$store.navigation.currentRoute', ...) mechanism to drive updates;
specifically delete the listener that reads event.detail?.route and sets
this.currentRoute/ calls expandCurrentPageParents(), relying on the $watch to
update this.currentRoute and call expandCurrentPageParents() instead, and only
retain a separate 'wiki-route-changed' event if you intend it as a public API
(in which case ensure any dispatch supplies the correct route value).
| if ((containerHeight === 0 || containerWidth === 0) && retries < 8) { | ||
| const delay = Math.min(100 * Math.pow(1.5, retries), 1000); | ||
| setTimeout(() => { | ||
| requestAnimationFrame(() => attemptScroll(retries + 1)); | ||
| }, delay); | ||
| return; |
There was a problem hiding this comment.
Retry timers are not cancelled on destroy().
scrollToSelectedItem may schedule up to 8 setTimeout/requestAnimationFrame callbacks (Lines 399–404) with a maximum combined window of several seconds. destroy() (Lines 430–434) only restores scroll restoration but never cancels these pending handles.
While each callback guards against a missing DOM and exits early, the handles still fire in the destroyed component's closure. Track the active timer and cancel it on teardown:
🛠️ Proposed fix
+ _scrollRetryTimer: null,
+
scrollToSelectedItem() {
const attemptScroll = (retries = 0) => {
...
if ((containerHeight === 0 || containerWidth === 0) && retries < 8) {
const delay = Math.min(100 * Math.pow(1.5, retries), 1000);
- setTimeout(() => {
+ this._scrollRetryTimer = setTimeout(() => {
requestAnimationFrame(() => attemptScroll(retries + 1));
}, delay);
return;
} destroy() {
+ if (this._scrollRetryTimer !== null) {
+ clearTimeout(this._scrollRetryTimer);
+ this._scrollRetryTimer = null;
+ }
if ('scrollRestoration' in history && this.previousScrollRestoration !== null) {
history.scrollRestoration = this.previousScrollRestoration;
}
}Also applies to: 430-434
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wiki/templates/wiki/includes/sidebar.html` around lines 399 - 404, The retry
timers started in scrollToSelectedItem can leave pending
setTimeout/requestAnimationFrame callbacks after component teardown; modify
scrollToSelectedItem to store the timeoutId and rafId in outer-scope variables
(e.g., timeoutHandle and rafHandle) whenever calling
setTimeout/requestAnimationFrame, and update any retry recursion to overwrite
those handles; then update destroy() to clear the scheduled tasks by calling
clearTimeout(timeoutHandle) and cancelAnimationFrame(rafHandle) (and reset the
handles) before restoring scroll restoration so no timers fire after destroy;
ensure attempts still early-return if DOM is gone.
3 participants
Closes #521
Screen.Recording.2026-02-18.at.4.09.39.PM.mov
Changes:
querySelector()calls now search within.wiki-sidebaronly@click.exact.preventto@click.preventwatch()watcher usingflush: 'post'nextTick()for deterministic DOM timing.isElementInViewport()helper to check element visibility before scrollingSummary by CodeRabbit
New Features
Bug Fixes / Improvements