Keep scroll from jumping on edit #69

New issue

Closed

opened 2026-04-07 18:25:04 +00:00 by maxtkc · 0 comments

maxtkc commented

2026-04-07 18:25:04 +00:00

Owner

Summary

Every time a patch is recorded, patchManager.on('change', ...) in src/index.ts:192 calls browseNavigation.refresh(), which calls render(), which does this.container.innerHTML = .... Replacing the entire container HTML destroys the .content div (which carries the overflow-y-auto scroll state), resetting its scrollTop to 0. The same happens on undo/redo via refreshAfterUndoRedo at src/index.ts:175.

The fix is minimal: in render(), save .content's scrollTop before rebuilding the HTML, then restore it after — but only when the page state hasn't changed (i.e., this is an in-place refresh, not user navigation to a new entity). Navigation should naturally reset scroll to 0.

Relevant Context

src/modules/browse-navigation.ts:300 — render() is the only place this.container.innerHTML is assigned (excluding the loading/error state helpers). This is the only file that needs to change.
src/types/page-state.ts — PageState is a small union of plain objects. JSON.stringify comparison is safe for equality checking.
src/index.ts:188-199 — patchManager.on('change', ...) is the primary trigger for spurious re-renders. src/index.ts:168-186 covers undo/redo; the fix handles both since they all flow through render().
The scroll container is .content (the <div class="content flex-1 overflow-y-auto"> rendered inside render()). There are no other independently-scrolling containers within it that need preservation — they're wiped out with the innerHTML reset anyway.

Phase 1: Scroll Preservation in `render()`

Single file change: src/modules/browse-navigation.ts.

Add private lastRenderedPageState: PageState | null = null to the BrowseNavigation class (near the other private fields around line 87).
In render() (line 300), before fetching page state:
- Capture the current scroll position: const contentDiv = this.container.querySelector<HTMLElement>('.content'); const savedScrollTop = contentDiv?.scrollTop ?? 0;

After computing pageState (before building the HTML), determine if this is a same-page re-render:

const isSamePage =
  this.lastRenderedPageState !== null &&
  JSON.stringify(this.lastRenderedPageState) === JSON.stringify(pageState);

After this.container.innerHTML = ..., update lastRenderedPageState and restore scroll if same page:

this.lastRenderedPageState = pageState;
if (isSamePage) {
  const newContent = this.container.querySelector<HTMLElement>('.content');
  if (newContent) newContent.scrollTop = savedScrollTop;
}

Place the restore after attachEventListeners() so event wiring doesn't interfere.

Gotchas:

render() is async. Concurrent calls (rapid edits) both read savedScrollTop from the same live DOM element before either has written new HTML — this is fine; both will restore to the same position and the last write wins.
renderLoadingState() and renderErrorState() also replace container.innerHTML. These are called only in edge cases (loading/error) where scroll position doesn't matter; leave them unchanged.
On the very first render, lastRenderedPageState is null, so isSamePage is false and scroll starts at 0 — correct.
On navigation (page state changes), isSamePage is false, scroll resets to 0 — correct.

Phase 2: Scroll Preservation in `refreshCurrentTimetable()`

Single file change: src/modules/schedule-controller.ts.

The timetable has a completely separate re-render path from the browse panel. Timetable cell edits call refreshCurrentTimetable() (line 930), which does container.innerHTML = html on #schedule-view (line 951). This wipes out the inner div.flex-1.overflow-x-auto that holds horizontal scroll state. The browse-navigation fix does not apply here at all.

The scroll container to preserve is the div.flex-1.overflow-x-auto rendered inside #schedule-view by timetable-renderer.ts:158. There is no meaningful vertical scroll within #schedule-view itself — the relevant scroll is horizontal (scrollLeft) so columns don't jump back to the left edge after an edit.

In refreshCurrentTimetable() (line 948), before assigning container.innerHTML, capture the horizontal scroll position of the inner scroll div:
```
const scrollDiv = container.querySelector<HTMLElement>('.overflow-x-auto');
const savedScrollLeft = scrollDiv?.scrollLeft ?? 0;
```

After container.innerHTML = html, restore the scroll position:

const newScrollDiv = container.querySelector<HTMLElement>('.overflow-x-auto');
if (newScrollDiv) newScrollDiv.scrollLeft = savedScrollLeft;

Gotchas:

The selector .overflow-x-auto matches the one direct child scroll div reliably; timetable-renderer.ts renders only one such element inside #schedule-view.
Vertical scrolling within the timetable area (if the table is taller than the viewport) is handled by a parent container outside #schedule-view; that container is not rebuilt here, so its scroll is naturally preserved without any extra work.
refreshCurrentTimetable() is also the method called when switching direction tabs — in that case, resetting horizontal scroll to 0 is correct behavior. But since the same timetable is being re-rendered with the same route/service/direction, a blanket restore is fine: if the user just switched directions, savedScrollLeft will be 0 already (the new direction tab starts at 0).

Phase 3: Preserve Timetable Horizontal Scroll in `browseNavigation.render()`

Single file change: src/modules/browse-navigation.ts.

The root cause of the timetable horizontal scroll jump is that browseNavigation.render() (triggered by patchManager.on('change')) runs a full re-render of the timetable via contentRenderer.renderPage() after refreshCurrentTimetable() has already restored the scroll. The browse-nav re-render replaces this.container.innerHTML entirely, recreating #schedule-view from scratch with scrollLeft=0. Phase 2's fix is already undone by the time the user sees the result.

The fix is the same pattern as phase 1 — in render(), before replacing this.container.innerHTML, also save the scrollLeft of any .overflow-x-auto inside the container, then restore it after (only on same-page re-renders).

In render() (browse-navigation.ts), after capturing savedScrollTop, also capture horizontal scroll:

const scrollXDiv = this.container.querySelector<HTMLElement>('.overflow-x-auto');
const savedScrollLeft = scrollXDiv?.scrollLeft ?? 0;

After restoring newContent.scrollTop, also restore scrollLeft if same page:

if (isSamePage && savedScrollLeft > 0) {
  const newScrollXDiv = this.container.querySelector<HTMLElement>('.overflow-x-auto');
  if (newScrollXDiv) newScrollXDiv.scrollLeft = savedScrollLeft;
}

Gotchas:

The .overflow-x-auto selector matches only the timetable's scroll div (the only such element inside the browse-navigation container during timetable view). No collision risk.
The savedScrollLeft > 0 guard avoids a needless querySelector on every non-timetable re-render.
refreshCurrentTimetable() is now redundant for scroll preservation (browse-nav handles it), but leave it in place — it also runs in cases where no patch is recorded (e.g. adding a pending stop) and the direct re-render is still valuable.

Phase 4: Debug and Fix Remaining Timetable Scroll Issue

Phases 2 and 3 were committed but the timetable horizontal scroll still resets on edit. All the logic in render() looks correct on paper — savedScrollLeft should be captured as positive, isSamePage should be true, and the new .overflow-x-auto should be findable after the rebuild. The root cause can only be determined from runtime logs. This phase adds targeted debug logging first, then fixes the root cause based on what the logs reveal.

Add console.log statements in render() (browse-navigation.ts) at: capture time (scrollXDiv found?, savedScrollLeft value, isSamePage), and restore time (newScrollXDiv found?, scrollLeft after set).
Add console.log statements in refreshCurrentTimetable() (schedule-controller.ts) at: savedScrollLeft capture, and after restore.
User tests and reports what the logs show.
Fix the actual root cause identified by the logs.

Root cause discovered: The DOM scrollLeft value was always 0 at the time render() captured it, because the browser resets scroll position during async await calls. The fix was to install a capture-phase scroll event listener on document in ScheduleController that synchronously records scrollLeft/scrollTop into timetableScrollLeft/timetableScrollTop instance fields. Both render() and refreshCurrentTimetable() read from these fields instead of the DOM. On navigation to a new page, resetTimetableScroll() zeroes the fields so the new timetable opens at the top-left.

Debug logs were removed after the fix was confirmed working.

Original Issue

When we make an edit, the browse panel refreshes and if you have scrolled down, the scroll jumps back to the top. Not critical, but very annoying.

## Summary Every time a patch is recorded, `patchManager.on('change', ...)` in `src/index.ts:192` calls `browseNavigation.refresh()`, which calls `render()`, which does `this.container.innerHTML = ...`. Replacing the entire container HTML destroys the `.content` div (which carries the `overflow-y-auto` scroll state), resetting its `scrollTop` to 0. The same happens on undo/redo via `refreshAfterUndoRedo` at `src/index.ts:175`. The fix is minimal: in `render()`, save `.content`'s `scrollTop` before rebuilding the HTML, then restore it after — but only when the page state hasn't changed (i.e., this is an in-place refresh, not user navigation to a new entity). Navigation should naturally reset scroll to 0. ## Relevant Context - **`src/modules/browse-navigation.ts:300`** — `render()` is the only place `this.container.innerHTML` is assigned (excluding the loading/error state helpers). This is the only file that needs to change. - **`src/types/page-state.ts`** — `PageState` is a small union of plain objects. `JSON.stringify` comparison is safe for equality checking. - **`src/index.ts:188-199`** — `patchManager.on('change', ...)` is the primary trigger for spurious re-renders. `src/index.ts:168-186` covers undo/redo; the fix handles both since they all flow through `render()`. - The scroll container is `.content` (the `<div class="content flex-1 overflow-y-auto">` rendered inside `render()`). There are no other independently-scrolling containers within it that need preservation — they're wiped out with the innerHTML reset anyway. --- ## Phase 1: Scroll Preservation in `render()` Single file change: `src/modules/browse-navigation.ts`. - [x] Add `private lastRenderedPageState: PageState | null = null` to the `BrowseNavigation` class (near the other private fields around line 87). - [x] In `render()` (line 300), before fetching page state: - Capture the current scroll position: `const contentDiv = this.container.querySelector<HTMLElement>('.content'); const savedScrollTop = contentDiv?.scrollTop ?? 0;` - [x] After computing `pageState` (before building the HTML), determine if this is a same-page re-render: ```ts const isSamePage = this.lastRenderedPageState !== null && JSON.stringify(this.lastRenderedPageState) === JSON.stringify(pageState); ``` - [x] After `this.container.innerHTML = ...`, update `lastRenderedPageState` and restore scroll if same page: ```ts this.lastRenderedPageState = pageState; if (isSamePage) { const newContent = this.container.querySelector<HTMLElement>('.content'); if (newContent) newContent.scrollTop = savedScrollTop; } ``` Place the restore *after* `attachEventListeners()` so event wiring doesn't interfere. **Gotchas:** - `render()` is async. Concurrent calls (rapid edits) both read `savedScrollTop` from the same live DOM element before either has written new HTML — this is fine; both will restore to the same position and the last write wins. - `renderLoadingState()` and `renderErrorState()` also replace `container.innerHTML`. These are called only in edge cases (loading/error) where scroll position doesn't matter; leave them unchanged. - On the very first render, `lastRenderedPageState` is `null`, so `isSamePage` is `false` and scroll starts at 0 — correct. - On navigation (page state changes), `isSamePage` is `false`, scroll resets to 0 — correct. --- ## Phase 2: Scroll Preservation in `refreshCurrentTimetable()` Single file change: `src/modules/schedule-controller.ts`. The timetable has a completely separate re-render path from the browse panel. Timetable cell edits call `refreshCurrentTimetable()` (line 930), which does `container.innerHTML = html` on `#schedule-view` (line 951). This wipes out the inner `div.flex-1.overflow-x-auto` that holds horizontal scroll state. The browse-navigation fix does not apply here at all. The scroll container to preserve is the `div.flex-1.overflow-x-auto` rendered inside `#schedule-view` by `timetable-renderer.ts:158`. There is no meaningful vertical scroll within `#schedule-view` itself — the relevant scroll is **horizontal** (`scrollLeft`) so columns don't jump back to the left edge after an edit. - [x] In `refreshCurrentTimetable()` (line 948), before assigning `container.innerHTML`, capture the horizontal scroll position of the inner scroll div: ```ts const scrollDiv = container.querySelector<HTMLElement>('.overflow-x-auto'); const savedScrollLeft = scrollDiv?.scrollLeft ?? 0; ``` - [x] After `container.innerHTML = html`, restore the scroll position: ```ts const newScrollDiv = container.querySelector<HTMLElement>('.overflow-x-auto'); if (newScrollDiv) newScrollDiv.scrollLeft = savedScrollLeft; ``` **Gotchas:** - The selector `.overflow-x-auto` matches the one direct child scroll div reliably; `timetable-renderer.ts` renders only one such element inside `#schedule-view`. - Vertical scrolling within the timetable area (if the table is taller than the viewport) is handled by a parent container outside `#schedule-view`; that container is not rebuilt here, so its scroll is naturally preserved without any extra work. - `refreshCurrentTimetable()` is also the method called when switching direction tabs — in that case, resetting horizontal scroll to 0 is correct behavior. But since the same timetable is being re-rendered with the same route/service/direction, a blanket restore is fine: if the user just switched directions, `savedScrollLeft` will be 0 already (the new direction tab starts at 0). --- ## Phase 3: Preserve Timetable Horizontal Scroll in `browseNavigation.render()` Single file change: `src/modules/browse-navigation.ts`. The root cause of the timetable horizontal scroll jump is that `browseNavigation.render()` (triggered by `patchManager.on('change')`) runs a full re-render of the timetable via `contentRenderer.renderPage()` **after** `refreshCurrentTimetable()` has already restored the scroll. The browse-nav re-render replaces `this.container.innerHTML` entirely, recreating `#schedule-view` from scratch with `scrollLeft=0`. Phase 2's fix is already undone by the time the user sees the result. The fix is the same pattern as phase 1 — in `render()`, before replacing `this.container.innerHTML`, also save the `scrollLeft` of any `.overflow-x-auto` inside the container, then restore it after (only on same-page re-renders). - [x] In `render()` (`browse-navigation.ts`), after capturing `savedScrollTop`, also capture horizontal scroll: ```ts const scrollXDiv = this.container.querySelector<HTMLElement>('.overflow-x-auto'); const savedScrollLeft = scrollXDiv?.scrollLeft ?? 0; ``` - [x] After restoring `newContent.scrollTop`, also restore `scrollLeft` if same page: ```ts if (isSamePage && savedScrollLeft > 0) { const newScrollXDiv = this.container.querySelector<HTMLElement>('.overflow-x-auto'); if (newScrollXDiv) newScrollXDiv.scrollLeft = savedScrollLeft; } ``` **Gotchas:** - The `.overflow-x-auto` selector matches only the timetable's scroll div (the only such element inside the browse-navigation container during timetable view). No collision risk. - The `savedScrollLeft > 0` guard avoids a needless querySelector on every non-timetable re-render. - `refreshCurrentTimetable()` is now redundant for scroll preservation (browse-nav handles it), but leave it in place — it also runs in cases where no patch is recorded (e.g. adding a pending stop) and the direct re-render is still valuable. --- ## Phase 4: Debug and Fix Remaining Timetable Scroll Issue Phases 2 and 3 were committed but the timetable horizontal scroll still resets on edit. All the logic in `render()` looks correct on paper — `savedScrollLeft` should be captured as positive, `isSamePage` should be true, and the new `.overflow-x-auto` should be findable after the rebuild. The root cause can only be determined from runtime logs. This phase adds targeted debug logging first, then fixes the root cause based on what the logs reveal. - [x] Add `console.log` statements in `render()` (`browse-navigation.ts`) at: capture time (scrollXDiv found?, savedScrollLeft value, isSamePage), and restore time (newScrollXDiv found?, scrollLeft after set). - [x] Add `console.log` statements in `refreshCurrentTimetable()` (`schedule-controller.ts`) at: savedScrollLeft capture, and after restore. - [x] User tests and reports what the logs show. - [x] Fix the actual root cause identified by the logs. **Root cause discovered:** The DOM `scrollLeft` value was always 0 at the time `render()` captured it, because the browser resets scroll position during async `await` calls. The fix was to install a capture-phase `scroll` event listener on `document` in `ScheduleController` that synchronously records `scrollLeft`/`scrollTop` into `timetableScrollLeft`/`timetableScrollTop` instance fields. Both `render()` and `refreshCurrentTimetable()` read from these fields instead of the DOM. On navigation to a new page, `resetTimetableScroll()` zeroes the fields so the new timetable opens at the top-left. Debug logs were removed after the fix was confirmed working. --- ## Original Issue When we make an edit, the browse panel refreshes and if you have scrolled down, the scroll jumps back to the top. Not critical, but very annoying.