keep the terminal scrolled to the bottom across resizes if it was at the bottom#2941
keep the terminal scrolled to the bottom across resizes if it was at the bottom#2941
Conversation
WalkthroughThis PR introduces bottom-scroll state tracking to the terminal component. It adds fields to track when the terminal was last at the bottom of the viewport (lastAtBottomTime, lastScrollAtBottom, cachedAtBottomForResize) and implements methods to check if the terminal is currently or recently at bottom (wasRecentlyAtBottom). A scroll event listener is attached to the xterm viewport to update bottom state. The ResizeObserver callback includes a guard to initialize the cached bottom state before handling resize operations, ensuring scroll position preservation during resize events. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Deploying waveterm with
|
| Latest commit: |
0cca7aa
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://50dc6278.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-term-scrollpos2.waveterm.pages.dev |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Review NotesThis PR implements scroll position preservation during terminal resize operations. The implementation:
The logic is sound and follows a reasonable approach for handling the resize/scroll interaction. The 20ms timeout for scroll restoration allows the terminal to complete its resize before scrolling. No blocking issues found. The implementation handles the edge cases appropriately:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/app/view/term/termwrap.ts (1)
232-244: Initialize bottom-state immediately after attaching the scroll listener.Line 107 defaults to
true, so until the first scroll event, resize logic may use a stale assumption. CallscrollHandler()once right after binding to seed accurate state.Suggested patch
if (viewportElem) { const scrollHandler = () => { const atBottom = viewportElem.scrollTop + viewportElem.clientHeight >= viewportElem.scrollHeight - 20; this.setAtBottom(atBottom); }; viewportElem.addEventListener("scroll", scrollHandler); + scrollHandler(); this.toDispose.push({ dispose: () => { viewportElem.removeEventListener("scroll", scrollHandler); }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/termwrap.ts` around lines 232 - 244, The scroll listener sets the "at bottom" state but the initial value remains stale until the first scroll event; after attaching the listener on the viewport element in termwrap.ts (the viewportElem and scrollHandler closure), invoke scrollHandler() once immediately (after viewportElem.addEventListener("scroll", scrollHandler)) to seed the correct state via this.setAtBottom(...) so resize/logic won't rely on the default true value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/view/term/termwrap.ts`:
- Around line 518-524: The scheduled resize timeout in the atBottom branch uses
setTimeout but does not store the timer id, allowing its callback to run after
the terminal is disposed; update the code around cachedAtBottomForResize so you
assign the result of setTimeout to a field (e.g., this.resizeTimeoutId or reuse
cachedAtBottomForResize to hold the timer id), clear any existing timeout before
creating a new one, and ensure the dispose/cleanup path clears that timer
(clearTimeout(this.resizeTimeoutId)) so the callback (which calls
this.terminal.scrollToBottom() and this.setAtBottom(true)) cannot run after
teardown.
---
Nitpick comments:
In `@frontend/app/view/term/termwrap.ts`:
- Around line 232-244: The scroll listener sets the "at bottom" state but the
initial value remains stale until the first scroll event; after attaching the
listener on the viewport element in termwrap.ts (the viewportElem and
scrollHandler closure), invoke scrollHandler() once immediately (after
viewportElem.addEventListener("scroll", scrollHandler)) to seed the correct
state via this.setAtBottom(...) so resize/logic won't rely on the default true
value.
| if (atBottom) { | ||
| setTimeout(() => { | ||
| this.cachedAtBottomForResize = null; | ||
| this.terminal.scrollToBottom(); | ||
| this.setAtBottom(true); | ||
| }, 20); | ||
| } |
There was a problem hiding this comment.
Clear pending resize timeout to prevent post-dispose terminal mutations.
Line 519 schedules async work, but the timer handle is not tracked or canceled. If dispose happens before it fires, callback code can still run and touch terminal state after teardown.
Suggested patch
export class TermWrap {
@@
cachedAtBottomForResize: boolean | null = null;
+ private resizeBottomTimeout: ReturnType<typeof window.setTimeout> | null = null;
@@
dispose() {
+ if (this.resizeBottomTimeout != null) {
+ clearTimeout(this.resizeBottomTimeout);
+ this.resizeBottomTimeout = null;
+ }
this.promptMarkers.forEach((marker) => {
try {
marker.dispose();
@@
if (atBottom) {
- setTimeout(() => {
+ if (this.resizeBottomTimeout != null) {
+ clearTimeout(this.resizeBottomTimeout);
+ }
+ this.resizeBottomTimeout = window.setTimeout(() => {
this.cachedAtBottomForResize = null;
this.terminal.scrollToBottom();
this.setAtBottom(true);
+ this.resizeBottomTimeout = null;
}, 20);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/term/termwrap.ts` around lines 518 - 524, The scheduled
resize timeout in the atBottom branch uses setTimeout but does not store the
timer id, allowing its callback to run after the terminal is disposed; update
the code around cachedAtBottomForResize so you assign the result of setTimeout
to a field (e.g., this.resizeTimeoutId or reuse cachedAtBottomForResize to hold
the timer id), clear any existing timeout before creating a new one, and ensure
the dispose/cleanup path clears that timer (clearTimeout(this.resizeTimeoutId))
so the callback (which calls this.terminal.scrollToBottom() and
this.setAtBottom(true)) cannot run after teardown.
1 participant
No description provided.