-
Notifications
You must be signed in to change notification settings - Fork 156
fix: support off-screen lines when re-rendering #414
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.
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
Conversation
This fixes a deep, difficult bug 👀 Let's try explain by example... If you have an `autocomplete` which has 2 options: - First option is multiple lines (let's say 6 lines) - Second option is one line In a terminal which is 10 rows tall, we are likely to exceed the terminal height since `6 (row 1) + 1 (row 2) + 4 (message and UI) = 11`. When this happens, the first render will be fine and the top row will be off screen. Now when we navigate (e.g. press `<down>`), the current rendering logic will assume that the _entire_ frame is on screen. It'll diff the previous frame and this frame, then update the changed rows. Because it assumes the entire frame is on screen, it'll try move down to row `N` (where `N` is the changed row). Here lies the problem: row `N` will be off by `1` (because `1` row is off screen right now). That means row `4` of the raw text is visually row `3` in your terminal. This fix basically accounts for that offset, which fixes a whole bunch of weird behaviours in smaller terminals, or just long prompts. One notable thing we also account for in this fix: if the new frame fits on screen, but the last frame didn't, we erase the entire frame and render it all to regain screen space.
🦋 Changeset detectedLatest commit: ca8d03d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@example/basic • @example/changesets commit: |
|
@yannbf this may fix some of the weirdness you've seen in the SB CLI but i'm not certain @natemoo-re this will one day be moot since the new renderer I have on a branch will replace the logic in this method anyway FYI. but that will need to account for this behaviour too. |
dreyfus92
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks good overall, but i would recommend adding some tests for short terminal window tests e.g. (just to verify it works properly).
describe('short terminal rendering', () => {
test('renders correctly when terminal height is less than content', () => {
// Mock terminal with only 5 rows
// Render content with 10+ lines
// Verify only visible lines are updated
});
test('handles diff when changed line is off-screen above viewport', () => {
// Terminal: 5 rows
// Content: 10 lines, currently showing lines 5-9
// Update line 2 (off-screen above)
// Verify: should update _prevFrame but not write to output
});
test('handles diff when changed line is off-screen below viewport', () => {
// Terminal: 5 rows
// Content: 10 lines, currently showing lines 0-4
// Update line 8 (off-screen below)
// Verify correct cursor positioning and content rendering
});
});maybe later we should consider some edge cases i don't mind adding something like these later:
test('handles case where all changed lines are off-screen', () => {
// All diff lines < diffOffsetAfter
// Should early return without rendering
});
test('handles single line change at viewport boundary', () => {
// Changed line exactly at diffOffsetAfter
// Should render that single line
});
test('handles multiple line changes spanning visible and off-screen areas', () => {
// Some diffs visible, some off-screen
// Should only render visible portion
});|
its not as simple as what you're describing this is used under the hood by all prompts so we don't currently have a way to test it in isolation the test cases you listed don't really work when we approach this from the point of view of the existing prompts. for example:
this depends on the prompt. lets say we test the select prompt - the initial render will be the same as it is now even if the terminal height is shorter than the content. because we'll still output the entire frame so we'd have to render the select, then somehow cause a re-render which updates the top of the frame. this isn't going to happen in the select since we only re-render the options, not the message. similar, it won't be encountered in text, confirm, etc. i'll try come up with a test but it can't really be the ones you listed for the reasons above |
|
i've added one basic test but i'm going to leave adding a full test suite for this for now we don't have any tests in isolation for rendering prompts right now so its out of scope of this PR. similarly, the rendering code is going in the bin once my rework lands (unpublished branch). once the new rendering code comes along, then we can add tests for this stuff in isolation |
This fixes a deep, difficult bug 👀
Let's try explain by example...
If you have an
autocompletewhich has 2 options:In a terminal which is 10 rows tall, we are likely to exceed the
terminal height since
6 (row 1) + 1 (row 2) + 4 (message and UI) = 11.When this happens, the first render will be fine and the top row will be
off screen.
Now when we navigate (e.g. press
<down>), the current rendering logicwill assume that the entire frame is on screen. It'll diff the
previous frame and this frame, then update the changed rows.
Because it assumes the entire frame is on screen, it'll try move down to
row
N(whereNis the changed row). Here lies the problem: rowNwill be off by
1(because1row is off screen right now). That meansrow
4of the raw text is visually row3in your terminal.This fix basically accounts for that offset, which fixes a whole bunch
of weird behaviours in smaller terminals, or just long prompts.
One notable thing we also account for in this fix: if the new frame fits
on screen, but the last frame didn't, we erase the entire frame and
render it all to regain screen space.