fix(review-diff): page over collapsed files instead of through them - #3030
Merged
Merged
Conversation
In Review Diff, PageDown/PageUp near a collapsed file wedged the view: the frame stopped changing, the cursor left the screen, and `Ln` kept climbing a page per press until it had crawled past every hidden row of the collapsed file. On a large collapsed file that is dozens of presses of nothing. `handle_page_motion` scrolls the viewport a page of *drawn* rows — a fold-aware walk that steps over a collapsed body for free — and then lands the cursor at the new top. It resolved the leaf to move correctly (`effective_active_split`), but delegated the scroll to `handle_scroll_event`, which targets the split manager's active leaf instead. For a grouped buffer those differ: the review panel is an inner leaf of the group, so the scroll moved the group host's viewport while the panel's stayed put. Seeing no movement, the page motion concluded the buffer was already at its edge and fell back to the plain logical-line cursor page-down, which counts lines whether or not the renderer draws them. Scroll the leaf the caller resolved, and let `handle_scroll_event` reach the same leaf for its own callers (Ctrl+Up / Ctrl+Down in a panel had the same split mismatch). Inner group leaves aren't nodes of the split tree, so `buffer_for_split` doesn't know them; `buffer_for_leaf` falls back to the leaf's own view state, which `effective_active_pair` already treats as authoritative. Fixes #3029 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EM6V5VanwxUpmnhqtxjy8R
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3029.
Symptom
In Review Diff, paging down towards a collapsed file wedges the view: the same frame is redrawn press after press, the cursor is nowhere on screen, and
Lnkeeps climbing by a page each time. The stall lasts for as many presses as the collapsed file has hidden rows — on aHEAD~100..HEADreview with a ~3000-row file collapsed, ~80 presses of nothing.Reproduced by hand in tmux against
HEAD~100..HEADbefore touching any code, then again after the fix: the same key sequence that used to freeze now steps over all six collapsed files in a single press, in both directions.Cause
handle_page_motionscrolls the viewport a page of drawn rows — a fold-aware walk that steps over a collapsed body for free — and then lands the cursor at the new viewport top. It resolves the leaf to move correctly (effective_active_split), but delegates the actual scroll tohandle_scroll_event, which targets the split manager's active leaf instead.For a grouped buffer those two differ. The review panel is an inner leaf of the group, not a node of the split tree, so the scroll moved the group host's viewport while the panel's stayed exactly where it was. Seeing no movement,
handle_page_motionconcluded the buffer was already at its edge and returnedNone, falling back to the plain logical-line cursor page-down — which counts lines whether or not the renderer draws them. The cursor walks into the collapsed body, the viewport has nowhere to follow it, and the screen stops.Without folds the two motions look identical (every logical line is a drawn row), which is why this only shows up once something is collapsed.
Change
handle_page_motionscrolls the leaf it already resolved, via a newhandle_scroll_event_for_split.handle_scroll_eventkeeps its old signature but now resolveseffective_active_split()rather than the tree's active leaf, soCtrl+Up/Ctrl+Downin a panel hit the same fix (they had the identical mismatch). Its early return for a window with no split layout is preserved —effective_active_pairasserts one.buffer_for_leafresolves the buffer for a leaf:SplitManager::buffer_for_splitonly knows tree nodes, so it answersNonefor an inner panel; the fallback reads the leaf's ownSplitViewState::active_buffer, whicheffective_active_pairalready treats as authoritative.Left alone deliberately:
handle_set_viewport_eventandhandle_recenter_eventresolve their leaf the same old way and would have the same blind spot for inner panels. Neither is reachable from a review panel by a keybinding I could find, so there's no reproducer to write a test against and no fix worth making without one.Tests
crates/fresh-editor/tests/e2e/plugins/review_diff_collapsed_paging.rsdrives keys and asserts only on rendered output: it opens a review whosesrc/file02.rsis forty times longer than its neighbours, collapses it withEnteron the file header, returns to the top of the stream, and requires PageDown to reachsrc/file03.rs's content — and PageUp to get back above the fold — within 30 presses. The fold-blind motion needs ~60 to cross that file's 1600 hidden rows; the fixed motion needs about four.Verified failing on the parent commit (
PageDown never reached src/file03.rs past the collapsed src/file02.rs — 30 presses were spent inside its hidden body) and passing with the change.Also run:
cargo fmt,cargo clippy --all-targets(no new warnings),cargo check --all-targets --features gui,cargo test -p fresh-editor --lib(3375 passed), and the e2e suites covering paging, scrolling, viewports, review diff / audit mode, splits, composites and folds (697 tests, 0 failures).🤖 Generated with Claude Code
https://claude.ai/code/session_01EM6V5VanwxUpmnhqtxjy8R
Generated by Claude Code