fix(transcript): persist the scroll offset alongside the at-bottom flag - #1348
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe terminal scroll hook now saves ChangesTerminal scroll persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR synchronously persists a transcript’s scroll offset whenever its at-bottom state changes, keeping the two saved values coherent across fast agent swaps.
Confidence Score: 4/5The zero-offset restoration defect should be fixed before merging because users who deliberately scroll to the transcript’s absolute top can still be snapped to the live bottom after a fast swap. The new synchronous write correctly preserves positive offsets, but it can persist zero with Files Needing Attention: src/renderer/components/TerminalOutput/hooks/useTerminalOutputScroll.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[User scrolls] --> B{At-bottom state changed?}
B -- Yes --> C[Persist flag and offset synchronously]
B -- No --> D[Debounce offset refinement]
C --> E[Agent swap and unmount]
D --> E
E --> F{Saved offset greater than zero?}
F -- Yes --> G[Restore saved position]
F -- No --> H[Skip restore and jump to bottom]
Reviews (1): Last reviewed commit: "MAESTRO: persist the scroll offset along..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // mount-time bottom jump snaps the user back down. Writing both | ||
| // halves in the same tick, in both directions, keeps the saved pair | ||
| // coherent no matter when the component goes away. (Y1) | ||
| onScrollPositionChange?.(scrollTop); |
There was a problem hiding this comment.
When a user scrolls to the absolute top of an overflowing transcript, this call persists scrollTop = 0 with isAtBottom = false, but the remount path requires initialScrollTop > 0; restoration is therefore skipped and the transcript snaps to the live bottom.
…eview) Greptile P1 on PR RunMaestro#1348: the restore gate required a POSITIVE saved offset (`initialScrollTop > 0`), so the one position a user reaches by scrolling all the way up - offset 0 with `isAtBottom: false` - was unrestorable. Returning to such a tab skipped the restore and the mount-time bottom jump snapped the view to the live bottom, which is the exact Y1 symptom this PR set out to fix. Zero is a real saved position, not a missing one. `initialIsAtBottom !== false` is the actual gate; the offset only needs to exist, so widen the check to `>= 0`. Two tests: offset 0 with the flag false restores to the top and pauses auto-scroll; offset 0 with the flag absent (a legacy tab) still falls through to the bottom jump, so the other half of the gate cannot silently regress. Verified the first test fails against the old `> 0` gate.
|
Good catch - fixed in Scrolling to the absolute top of an overflowing transcript persists
Two tests added: offset 0 with the flag |
A deliberate scroll-up persists its two halves on different schedules: the at-bottom flag goes out synchronously on the transition, the absolute offset is debounced 200ms and DROPPED (not flushed) on unmount for the RunMaestro#1323 wrong-tab reason. Swapping agents inside that window saves isAtBottom: false with no matching scrollTop, so the remount restore gate (initialScrollTop > 0) skips and the mount-time bottom jump wins. Adds a 'scrolled-up persistence across unmount (Y1)' describe block pinning that current broken behaviour as documentation of the race. No product code touched; it is inverted when the fix lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A deliberately scrolled-up transcript position snapped back to the live bottom after swapping agents away and back. The two halves of the saved scroll state were written on different schedules: the at-bottom flag went out synchronously on the boundary transition, the absolute offset only after a 200ms debounce. The unmount cleanup drops that pending save on purpose (RunMaestro#1323: onScrollPositionChange resolves its target tab at call time, so flushing during a swap would write the outgoing agent's offset into the incoming agent's tab), so a swap inside the debounce window persisted isAtBottom: false with no matching scrollTop. The remount restore requires initialScrollTop > 0, skipped, and the mount-time bottom jump won. Write both halves in the same tick instead, inside the existing transition block and in both directions, so the saved pair can never disagree. The debounced save stays as the refinement path, so a dropped flush now costs at most ~200ms of scrolling precision, never the whole position. No change to the cleanup behaviour, the 200ms debounce, the 16ms throttle, the 50px at-bottom threshold or the 50px restore epsilon. Inverts the Task 1 pin test and adds return-to-bottom pair coherence, debounce refinement, no-scroll-no-save and end-to-end restore coverage. 17/17 in the scoped file; the new tests go red when the hook change is reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eview) Greptile P1 on PR RunMaestro#1348: the restore gate required a POSITIVE saved offset (`initialScrollTop > 0`), so the one position a user reaches by scrolling all the way up - offset 0 with `isAtBottom: false` - was unrestorable. Returning to such a tab skipped the restore and the mount-time bottom jump snapped the view to the live bottom, which is the exact Y1 symptom this PR set out to fix. Zero is a real saved position, not a missing one. `initialIsAtBottom !== false` is the actual gate; the offset only needs to exist, so widen the check to `>= 0`. Two tests: offset 0 with the flag false restores to the top and pauses auto-scroll; offset 0 with the flag absent (a legacy tab) still falls through to the bottom jump, so the other half of the gate cannot silently regress. Verified the first test fails against the old `> 0` gate.
2f08827 to
cd50b83
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Closes finding Y1.
Problem
A deliberately scrolled-up transcript snapped back to the live bottom after an agent swap, instead of holding the position the user chose. This is the inverse of J1/O1 (#1323), which were about failing to reach the bottom.
useTerminalOutputScrollrestores a saved offset only when bothinitialScrollTop > 0andinitialIsAtBottom === falseare persisted. The two halves were written on different schedules:onAtBottomChangefires synchronously on a transitiononScrollPositionChangeis debounced 200ms, and Re-pin transcript to live bottom after agent swap #1323 deliberately drops the pending save on unmountSo a user who scrolled up and swapped within ~200ms persisted
isAtBottom: falsewith no matchingscrollTop. The restore was skipped and the mount-time bottom jump won.Fix
Persist the offset alongside the synchronous at-bottom transition, so both halves are written together and can never disagree. This deliberately does not flush the debounce on unmount - #1323 rejected that explicitly, and its reasoning still holds.
Testing
Scoped tests only. Adds 208 lines to
useTerminalOutputScroll.test.ts, including a test that pins the race by unmounting before the 200ms debounce fires. Existing #1323 follow-the-bottom coverage and the #1140 scroll-event guard bookkeeping are untouched.Human verification
Confirmed on a local RC build: following the bottom, swapping away and back, still lands at the live bottom (the O1 behaviour must not regress).
🤖 Generated with Claude Code
Summary by CodeRabbit