-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
Fix repl cursor multiline overflow #60171
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
38d53d6 to
2508b06
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60171 +/- ##
==========================================
+ Coverage 88.55% 88.56% +0.01%
==========================================
Files 704 704
Lines 208087 208120 +33
Branches 40019 40022 +3
==========================================
+ Hits 184266 184318 +52
+ Misses 15818 15808 -10
+ Partials 8003 7994 -9
🚀 New features to boost your workflow:
|
Add test for cursor position consistency when REPL multiline input exceeds terminal height. Tests navigation through a 27-line input on a 20-row terminal to ensure cursor moves correctly across viewport boundaries
When multiline REPL input exceeds the terminal height, navigating the cursor would move the cursor position correctly but fail to refresh the screen properly, leaving stale content visible. This implements viewport-based rendering in kRefreshLine() that displays only the visible portion of input centered around the cursor position when content exceeds terminal height. Fixes: nodejs#59938
2508b06 to
315a793
Compare
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.
Appreciate you tackling this @Han5991.
Screen.Recording.2025-10-11.at.8.05.55.PM.mov
There are some issues in this implementation. the "viewport" based rendering scrolls the content before the cursor reaches the top. I don't think that's inline with other REPLs out there and users' expectations.
For example, Python's REPL:
Screen.Recording.2025-10-11.at.7.41.39.PM.mov
| } | ||
| } | ||
|
|
||
| this[kWriteToOutput](this[kPrompt] + lines[startLine]); |
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.
We should only show the prompt on the actual first line of the content. Not the first line of the visible content.
| r.input.run([{ name: 'down' }]); | ||
| } | ||
|
|
||
| assert.strictEqual(r.cursor, endCursor); |
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.
I am not sure what are we testing here. As I see, the cursor position always updated, but it's the visible content which did not.
Description
Fixes #59938
When multiline REPL input exceeds the terminal height, navigating the cursor with arrow
keys would move the cursor position correctly but fail to refresh the screen properly,
leaving stale content visible on the screen.
Problem
In the REPL, when a multiline input has more lines than the terminal height:
Solution
This PR implements viewport-based rendering in
kRefreshLine()that displays only thevisible portion of input centered around the cursor position when content exceeds
terminal height:
Changes
rowsgetter toInterfaceclass for terminal height detectionMathMinprimitive for viewport calculationskRefreshLine()to calculate visible line range based on:this.rows)prevRowscalculation to account for viewport positionDemo
Before (Bug):
[Your video showing the bug - cursor moves but screen doesn't refresh]
After (Fixed):
[Your video showing the fix - screen properly refreshes during navigation]
Testing
2025-10-09.10.55.16.mov
Added regression test in
test/parallel/test-repl-multiline-navigation.jsthat:Run tests: