Skip to content

fix: keep a zz centering when opening a comment near EOF - #595

Merged
agavra merged 1 commit into
agavra:mainfrom
davinkevin:fix/keep-eof-centering-when-opening-comment
Aug 13, 2026
Merged

agavra merged 1 commit into
agavra:mainfrom
davinkevin:fix/keep-eof-centering-when-opening-comment

Conversation

@davinkevin

Copy link
Copy Markdown
Contributor

Problem

zz on a line near the end of a file centers the cursor and leaves blank rows below the content. Opening a comment on that line snaps the view back, so the line being reviewed jumps to the bottom of the viewport. Esc does not restore the centering.

Repro

  1. Open any diff and move the cursor near the end of a file — far enough that zz leaves blank rows below the content.
  2. Press ZZ to center the line.
  3. Press c to open a comment on it.

The line drops from the middle of the viewport to near the bottom. It happens in both the unified and side-by-side views.

Cause

Two places disagree on the maximum scroll offset.

App::max_scroll_offset (src/app/navigation.rs) explicitly allows blank space below the content, which is what makes zz work at EOF:

/// This permits empty space below content (e.g. when centering the cursor near EOF)
pub fn max_scroll_offset(&self) -> usize {
    self.total_lines().saturating_sub(1)
}

scroll_comment_input_into_view (src/ui/diff_view.rs) clamped with a stricter "no blank space below" rule:

let max_scroll = total_lines.saturating_sub(viewport_height);

That clamp runs on every frame an input box is rendered (diff_unified.rs, diff_side_by_side.rs). The early return only checks whether a box exists, so the clamp fired even when the box was already fully visible and no adjustment was needed.

Worked example — viewport 40 rows, 100-line document, zz on line 98:

step scroll_offset screen row of line 98
after zz 78 (18 blank rows below) 20 — centered
comment opened (4-line box, 104 lines total) clamped to 104 − 40 = 64 34 — near the bottom

Fix

Clamp to total_lines - 1 so the render path and the app agree. The branches that pull the box into view when it is above, below, or taller than the viewport are untouched, so the box still follows the text cursor while typing.

Tests

  • should_keep_cursor_centered_when_opening_comment_near_eof — the scenario above; scroll_offset stays at 78.
  • should_clamp_to_last_line_when_box_sits_past_the_content — covers the changed clamp directly.
  • should_not_scroll_past_end_of_content asserted 90, but that value came from the box-above-viewport branch, not the clamp — the clamp never ran in that case. It now asserts the 95 that branch produces, and the new test above pins the clamp.

cargo test (1363 passed), cargo clippy --all-targets -D warnings, and cargo fmt --check are clean. Also verified interactively with a release build: centering survives at EOF, and the input box still auto-scrolls into view at the top and middle of a file, in both diff layouts.

No issue filed for this — I could not find one open that tracks it (closest is #76, different code path; #11 is likely where the strict clamp came from).

`zz` near the end of a file centers the cursor and leaves blank rows
below the content, which `App::max_scroll_offset` explicitly allows
(`total_lines - 1`, "permits empty space below content"). Opening a
comment there snapped the view back so the reviewed line jumped to the
bottom of the viewport, and Esc did not restore it.

`scroll_comment_input_into_view` clamped to `total_lines -
viewport_height` instead, a stricter "no blank space below" rule. That
clamp runs on every frame an input box is rendered, in both the unified
and side-by-side renderers, so it overrode the centering even when the
box was already fully visible and no adjustment was needed.

Clamp to `total_lines - 1` so the render path and the app agree on the
maximum scroll. The branches that pull the box into view are untouched,
so the box still follows the text cursor while typing.

`should_not_scroll_past_end_of_content` asserted 90, but the value it
pinned came from the box-above-viewport branch rather than the clamp; it
now asserts the 95 that branch produces, and a separate test covers the
clamp itself.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

@agavra agavra left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, this is subtly improved.

@agavra
agavra merged commit 7c9632d into agavra:main Aug 13, 2026
4 checks passed
martintrojer pushed a commit that referenced this pull request Oct 7, 2026
`wrap_lines` defaults to true, and with wrapping on a logical line can
cover several visual rows. `scroll_comment_input_into_view` measured the
input box in logical lines but compared it against `viewport_height`, a
row count, so `box_end + 1 - viewport_height` under-scrolled and left the
box below the fold. On the last line of a file there is nothing further
to scroll into, so the box never came back and the user typed blind.

Take a `row_height` accessor and walk up from the bottom of the box
accumulating rows until the viewport is full, instead of subtracting line
counts. The walk is bounded by `viewport_height`, and both renderers
build the accessor on the `wrap_spans` they already use, so only the
lines it touches get measured. With wrapping off every line is one row
and the result is identical to the previous arithmetic; the clamp added
in #595 is untouched.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants