Skip to content

Fix teleport when exiting out of alt buffer. v2 #5411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix teleport when exiting out of alt buffer. v2 #5411

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32c3cb0
Make sure this._latestYDisp is up to date
anthonykim1 Oct 1, 2025
2a6e22e
set this._latestYDisp = undefined onBufferActivate
anthonykim1 Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/browser/Viewport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,10 @@ export class Viewport extends Disposable {
});
this._suppressOnScrollHandler = false;

// If ydisp has been changed by some other copmonent (input/buffer), then stop animating smooth
// If ydisp has been changed by some other component (input/buffer), then stop animating smooth
// scroll and scroll there immediately.
if (ydisp !== this._latestYDisp) {
this._latestYDisp = ydisp;
Copy link
Member

Choose a reason for hiding this comment

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

Does it work if we set this only when this._latestYDisp is undefined? Normally _latestYDisp is set when handling scroll events, not syncing.

Copy link
Contributor Author

@anthonykim1 anthonykim1 Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I just tested this with conditional this._latestYDisp is undefined , and it's not working..

this._latestYDisp switches from 0 to 1000 in queueSync, and this this._latestYDisp passes down and becomes ydisp in sync where

 if (ydisp !== this._latestYDisp) {    // 1. this would be 0 !== 1000
      this._scrollableElement.setScrollPosition({
        scrollTop: ydisp * this._renderService.dimensions.css.cell.height   // 2. ydisp is 0.
      });
    }

Afterwards, when latestYDisp is 1000, and ydisp is also 1000, we don't get to call

  scrollTop: ydisp * this._renderService.dimensions.css.cell.height

because if (ydisp !== this._latestYDisp) is false, and this would leave the scrollTop to remain 0, instead updating to correct value (end of buffer)
---> Hence you get stuck in 0 scrollTop position.

Also, _sync 's ydisp cannot actually be undefined since it has default value, from:

 private _sync(ydisp: number = this._bufferService.buffer.ydisp): void {

Long story short,
In the "teleport" scenario, I think the the problem appears because we don't get to update scrollTop to the correct place when we go from having 0 to 1000 ydisp when calling _sync

Maybe this is also some race condition, as I can only repro sometime

Copy link
Member

Choose a reason for hiding this comment

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

Does this._bufferService.buffers.onBufferActivate need to be handled specially? It seems like it's related to buffer switching during a smooth scroll happening. You could try increasing smooth scroll duration significantly (10000ms?) to see if it's easier to repro?

Copy link
Contributor Author

@anthonykim1 anthonykim1 Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, maybe more proper/intuitive fix is that we set latestYDisp to undefined onBufferActivate.
This way we prevent from any contamination when switching between buffers.

Setting the smooth scroll to 50000 and also aggressively scrolling and letting the mouse scroll run right before exiting out via q! seemed to help me repro more consistently.

this._scrollableElement.setScrollPosition({
scrollTop: ydisp * this._renderService.dimensions.css.cell.height
});
Expand Down
Loading