Skip to content

fix(renderer): don't trim final newline (because it can be intentional) - #1798

Open
meowgorithm wants to merge 1 commit into
mainfrom
fix/preserve-trailing-blank-lines
Open

fix(renderer): don't trim final newline (because it can be intentional)#1798
meowgorithm wants to merge 1 commit into
mainfrom
fix/preserve-trailing-blank-lines

Conversation

@meowgorithm

Copy link
Copy Markdown
Member

This revision maintains trailing newlines, because trailing blank lines can be a design choice. Prior to this change it was nearly impossible to keep them.

Worth testing this one thoroughly, per the removed comment in the code.

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.91%. Comparing base (73b6d91) to head (b56fb2e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1798      +/-   ##
==========================================
+ Coverage   57.81%   57.91%   +0.09%     
==========================================
  Files          25       25              
  Lines        1337     1340       +3     
==========================================
+ Hits          773      776       +3     
  Misses        474      474              
  Partials       90       90              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@andrinoff andrinoff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's also a pre-existing bug that a view without a trailing newline loses its last line on exit. close moves to the last buffer row and erases below it, so the shell prompt overwrites that line. Maybe worth fixing here too

Comment thread cursed_renderer.go
// than on its last visible line. Frames taller than the screen
// have their top rows dropped, so their last visible row is the
// screen's.
y := frameArea.Dy() - 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

parking at the frame's last row is correct, but for frames taller than the screen this now aligns the frame with the top of the terminal, and the existing per-frame full redraw then floods scrollback in tmux

Comment thread cursed_renderer_test.go

// Frames taller than the screen drop their top rows, so the parked cursor
// must land on the screen's last visible row, not beyond it.
func TestCursedRenderer_trailingBlankLinesTallFrame(t *testing.T) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this checks the parked position but not that a second unchanged flush is a no-op. that's why the regression above was missed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants