From 7721e530fbe0eec578ca3bb8dbff6b64e54f59f1 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Mon, 6 Jul 2026 04:36:13 +0000 Subject: [PATCH] fix(#5657): preserve workspace file-tree scroll position across re-render MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit renderFileTree() clears its scroll container with box.innerHTML='' and rebuilds every row. Detaching the rows collapses scrollHeight, so the browser clamps scrollTop to 0 — every folder expand/collapse, breadcrumb nav, refresh, and hidden-files toggle that re-runs the renderer teleported the reader to the top of a long tree, losing their place (the dominant interaction in the panel). Fix: capture box.scrollTop before the innerHTML='' wipe and restore it after the normal render tail (after _renderTreeItems repaints and the container is tall again). A plain scrollTop restore is sufficient and correct here — expand/collapse insert or remove rows BELOW the clicked disclosure, so the clicked row keeps its offset from the top; no getBoundingClientRect anchor delta is needed (that's only for the prepend-above case in the chat messages list). The reporter's data-path anchor sketch is deliberately NOT used: _renderTreeItems rows carry no data-path, so that selector would match nothing. The two early-return paths (no-workspace hides the box; empty-dir has nothing to scroll) are left untouched — they legitimately reset. The restore guards only the normal render tail. loadDir's two-render sequence (initial + post-expanded-dirs prefetch) is handled correctly: each renderFileTree() call independently captures then restores its own pre-wipe scrollTop across the await. Test: tests/test_issue5657_filetree_scroll_preserve.py — static guards that the capture precedes the wipe and the restore follows _renderTreeItems (per the maintainer test plan; drops the getBoundingClientRect anchor assertion since the fix deliberately doesn't use it). Fail-without-fix verified; 355 workspace-tree regression tests green. Closes #5657. --- static/ui.js | 15 ++++- ...test_issue5657_filetree_scroll_preserve.py | 64 +++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 tests/test_issue5657_filetree_scroll_preserve.py diff --git a/static/ui.js b/static/ui.js index 68c2c523..c5089ef0 100644 --- a/static/ui.js +++ b/static/ui.js @@ -17212,7 +17212,18 @@ if(!S._expandedDirs) S._expandedDirs=new Set(); if(!S._dirCache) S._dirCache={}; function renderFileTree(){ - const box=$('fileTree');box.innerHTML=''; + const box=$('fileTree'); + // #5657: capture the scroll position before wiping the container. box.innerHTML='' + // detaches every row, collapsing scrollHeight so the browser clamps scrollTop to 0; + // without this, every expand/collapse, breadcrumb nav, refresh, and hidden-files + // toggle that re-runs renderFileTree() teleports the reader back to the top of a + // long tree. Restored only after the normal render tail below — the two early-return + // paths (no-workspace hides the box; empty-dir has nothing to scroll) legitimately + // reset. A plain scrollTop restore suffices here: expand/collapse insert/remove rows + // BELOW the clicked disclosure, so the clicked row keeps its offset from the top (no + // getBoundingClientRect anchor delta needed — that's only for prepend-above cases). + const prevScrollTop=box?box.scrollTop:0; + box.innerHTML=''; // Cache current dir entries S._dirCache[S.currentDir||'.']=S.entries; // Show empty-state when no workspace is set or the directory is empty (#703) @@ -17231,6 +17242,8 @@ function renderFileTree(){ return; } _renderTreeItems(box, visibleEntries, 0); + // #5657: restore the pre-wipe scroll position now that the tree is tall again. + if(box) box.scrollTop=prevScrollTop; } let _wsActiveDragPath=null; diff --git a/tests/test_issue5657_filetree_scroll_preserve.py b/tests/test_issue5657_filetree_scroll_preserve.py new file mode 100644 index 00000000..fb21fd46 --- /dev/null +++ b/tests/test_issue5657_filetree_scroll_preserve.py @@ -0,0 +1,64 @@ +"""Regression coverage for #5657 — workspace file-tree preserves scroll on re-render. + +`renderFileTree()` clears its scroll container with ``box.innerHTML=''`` and +rebuilds every row. That detaches the rows, collapses ``scrollHeight``, and the +browser clamps ``scrollTop`` to 0 — so every folder expand/collapse, breadcrumb +nav, refresh, and hidden-files toggle that re-runs the renderer teleported the +reader to the top of a long tree. + +Fix: capture ``scrollTop`` before the wipe and restore it after the normal +render tail. A plain scrollTop restore is sufficient (expand/collapse insert or +remove rows BELOW the clicked disclosure, so the clicked row keeps its offset) — +the reporter's getBoundingClientRect anchor sketch is deliberately NOT used, and +the ``.file-item`` rows carry no ``data-path`` for it anyway. +""" + +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parents[1] +UI_JS = (REPO_ROOT / "static" / "ui.js").read_text(encoding="utf-8") + + +def _render_file_tree_body() -> str: + start = UI_JS.index("function renderFileTree()") + end = UI_JS.index("\nfunction ", start + 1) + return UI_JS[start:end] + + +def _render_file_tree_code() -> str: + """Body with // comment lines stripped, so assertions anchor on real code.""" + lines = [ln for ln in _render_file_tree_body().splitlines() if not ln.strip().startswith("//")] + return "\n".join(lines) + + +def test_render_file_tree_captures_and_restores_scrolltop(): + body = _render_file_tree_code() + # It must still wipe the container (the behavior that loses scroll)... + assert "box.innerHTML=''" in body or 'box.innerHTML = ""' in body + # ...and it must capture scrollTop BEFORE the wipe and restore it after. + assert "scrollTop" in body, ( + "renderFileTree must capture + restore scrollTop around the innerHTML wipe (#5657)" + ) + capture_idx = body.index("prevScrollTop=box") + wipe_idx = body.index("box.innerHTML=''") + assert capture_idx < wipe_idx, ( + "scrollTop must be captured BEFORE box.innerHTML='' so it isn't already clamped to 0" + ) + render_idx = body.index("_renderTreeItems(box") + restore_idx = body.rindex("box.scrollTop=") + assert restore_idx > render_idx, ( + "scrollTop must be restored AFTER _renderTreeItems repaints the tree" + ) + + +def test_early_return_paths_still_reset_scroll(): + # The no-workspace and empty-dir early returns legitimately want scroll reset + # (box hidden / nothing to scroll) — the restore must live on the normal tail + # only, i.e. AFTER _renderTreeItems, not before the early returns. + body = _render_file_tree_code() + render_idx = body.index("_renderTreeItems(box") + tail = body[render_idx:] + assert "box.scrollTop=" in tail or "box.scrollTop =" in tail, ( + "the scrollTop restore must be on the normal render tail, after _renderTreeItems" + )