Skip to content

fix(ui): preserve scroll position across comment add/edit/delete#1

Merged
amix merged 3 commits into
mainfrom
amix/preserve-scroll-on-comment-mutation
May 11, 2026
Merged

fix(ui): preserve scroll position across comment add/edit/delete#1
amix merged 3 commits into
mainfrom
amix/preserve-scroll-on-comment-mutation

Conversation

@amix
Copy link
Copy Markdown
Owner

@amix amix commented May 11, 2026

Context

Adding, editing, or deleting a comment in the review pane currently lets the diff content shift under the user's eye — inserting an inline comment card above the viewport pushes code down by the card's height, and deleting one pulls everything up. Watcher-triggered reloads of .dunk/comments.json have the same problem.

What was changed

  • Generalized the existing layout/wrap anchor restoration useLayoutEffect in DiffPane to also fire on files identity changes (previousFilesRef.current !== files). One source of truth for viewport preservation across every relayout-causing input.
  • For files-only changes, when the natural anchor cannot resolve in the new geometry (e.g. the inline-note row was just deleted), fall back to the nearest surviving diff row before clamping. Inline-note stable keys are inline-note:${annotation.id}, so an in-place comment edit still resolves cleanly and keeps the reader's position inside the card.
  • DiffSectionRowBounds carries a kind ("diff-row" | "inline-note") so anchor pickers can prefer survivable rows.
  • findViewportRowAnchor accepts { preferDiffRows?: boolean }; resolveViewportRowAnchorTop now returns number | null so callers can apply a clamp fallback instead of scrolling to file body top.

Validation

  • bun run typecheck, bun test (334 pass), bun run test:integration (21 pass — full existing PTY suite, including the layout/wrap scroll-preservation tests that exercise the same scaffolding this change extends), bun run lint, bun run format.
  • Manual TTY smoke run on a real diff.
  • 5 new unit tests in viewportAnchor.test.ts covering: preferDiffRows row-picking, inline-note anchor surviving an in-place edit (with explicit height-bump assertion), inline-note anchor returning null after deletion, diff-row fallback resolving cleanly after deletion, and missing-file resolver behavior.

Expert review

Design was validated before implementation via expert-letter to Codex (.expert/letter-20260511T104143Z-*.md). The unified-effect approach, preferDiffRows bias, nullable resolver contract, and clamp fallback are all from that response.

Out of scope

  • Drive-by extraction of the inline VisibleAgentNote id-format mapping from DiffPane. The same string format is built in two places, but the existing site is unrelated to this change.
  • Dedicated comment-mutation request-id signal threaded from App to DiffPane. Codex explicitly advised against App-level plumbing here; the files identity trigger is the intended design and also benefits filter/reload paths.
  • Pane-level PTY integration test exercising comment add + scroll preservation end-to-end. The new helper-level tests cover the algorithmic core, and the DiffPane wiring reuses the layout/wrap restoration scaffolding already exercised by existing PTY coverage. Worth a follow-up.

🤖 Generated with Claude Code

The review pane re-anchors the viewport on a surviving diff row whenever
the file list changes (comment mutations and external `.dunk/comments.json`
reloads), so inserting or removing inline comment cards no longer pushes
the code under the user's eye out of frame.

Reuses the existing layout/wrap anchor restoration scaffolding: a single
useLayoutEffect now triggers on `files` identity change too, falls back to
a survivable diff row when the natural inline-note anchor disappears, and
clamps the previous scrollTop into the new content extent as a last resort.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@amix amix marked this pull request as ready for review May 11, 2026 11:16
amix and others added 2 commits May 11, 2026 13:26
These four tests asserted on the first frame captured after `settleDiffPane`
(two renderOnce calls with a microtask between them). On Windows test
runners the OpenTUI render+settle cycle can take more iterations than that
budget, so the scroll reveal hadn't landed when the frame was captured,
producing flaky `(fail)` runs that re-ran clean. The flake surfaced once the
PR-only Windows workflow began running.

Each test now polls via the existing `waitForFrame` helper until the
scroll-settled predicate matches (8 attempts), keeping the original assertions
intact and the passing-path cost unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Convert "keeps a selected wrapped hunk fully visible" to waitForFrame so
  the reveal poll absorbs Windows OpenTUI settle variance, matching the
  three sibling tests this branch already hardened.
- Bump the default `jj` test timeout to 30s. Windows process spawning runs
  ~70× slower than Linux (5500ms vs 73ms in CI), pushing the invalid-revset
  test past Bun's 5s default. Skips the still-passing variants undisturbed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@amix amix merged commit 3a7792d into main May 11, 2026
2 checks passed
@amix amix deleted the amix/preserve-scroll-on-comment-mutation branch May 11, 2026 11:35
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.

1 participant