Skip to content

Fix counting extra lines when lines wrap - #180

Open
mogzol wants to merge 1 commit into
unjs:mainfrom
mogzol:fix-counting-lines
Open

Fix counting extra lines when lines wrap#180
mogzol wants to merge 1 commit into
unjs:mainfrom
mogzol:fix-counting-lines

Conversation

@mogzol

@mogzol mogzol commented Jun 23, 2026

Copy link
Copy Markdown

(note: this PR was originally submitted as #165, but I needed to change the source branch, so I am re-creating it)

Fixes #164

Previously, extra lines were counted only by the number of line breaks in them, so the number of lines cleared would be incorrect if any lines wrapped. Additionally, prevLineCount could be wrong if the size of the terminal changes between renders.

To fix this, the number of lines to clear is calculated immediately before clearing them, and the 'string-width' library is used to determine accurate widths and line counts. This is the same library used by 'wrap-ansi', so it should be reliable, and it is not actually a new dependency.

Summary by CodeRabbit

  • Refactor
    • Reworked terminal log rendering to track previously rendered output more precisely, improving behavior across updates and terminal resizing.
  • Bug Fixes
    • Fixed occasional line misalignment or incorrect clearing/erasing when output contains wide characters (e.g., full-width text) or when the terminal dimensions change.

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c78fbdcb-f6c9-4f21-8302-b20634c17231

📥 Commits

Reviewing files that changed from the base of the PR and between 4a335b0 and 4f7d1de.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • package.json
  • src/utils/log-update.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • package.json
  • src/utils/log-update.ts

📝 Walkthrough

Walkthrough

Adds string-width as a direct dependency and rewrites LogUpdate's internal line-tracking: prevLineCount (a number) is replaced by prevLines (a stored string), and a lineCount getter now computes the true number of terminal rows occupied by computing character widths with stringWidth against the current terminal column count.

Changes

LogUpdate line-wrap fix

Layer / File(s) Summary
Dependency, state shape, and lineCount computation
package.json, src/utils/log-update.ts
Adds string-width to dependencies, imports stringWidth, replaces the prevLineCount: number field with prevLines: string | null, updates render() to store the rendered string into prevLines, and rewrites the lineCount getter to split prevLines on \n and sum wrapped-line counts using stringWidth vs. current columns.
clear, done, and _onData using computed lineCount
src/utils/log-update.ts
clear() erases using this.lineCount (now string-width-aware), done() resets prevLines to null, and _onData appends intercepted output into prevLines instead of incrementing a numeric counter.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A long line once wrapped round the bend,
And the bar would just duplicate—no end!
Now I measure each glyph, count the rows,
Clearing just what the terminal shows.
No more ghost bars haunting the screen! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix counting extra lines when lines wrap' directly summarizes the main change: fixing how extra lines are counted when terminal lines wrap, which is the core issue addressed in the PR.
Linked Issues check ✅ Passed The PR satisfies issue #164 by adding string-width dependency and refactoring log-update.ts to accurately count wrapped lines using character width calculations instead of newline counting.
Out of Scope Changes check ✅ Passed All changes directly address the line wrapping issue: adding string-width dependency and refactoring LogUpdate to use it for accurate line counting.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/utils/log-update.ts`:
- Around line 72-75: The clear() method calls this.done() before erasing lines,
but this.done() sets this.prevLines to null, causing the lineCount getter to
return 0 and eraseLines(0) to produce an empty string. Fix this by reordering
the operations in the clear() method: call
this.write(eraseLines(this.lineCount)) before this.done() so that lineCount is
computed while prevLines is still set, ensuring lines are actually erased before
the state is reset.
- Around line 84-91: In the _onData method, the prevLines property is typed as
string | null and initialized as null, but at line 88 where this.prevLines +=
data is executed, if prevLines is still null, the += operator will concatenate
the string "null" to the data, corrupting the buffer. Add a guard to check if
prevLines is null before performing the concatenation—either initialize it to an
empty string if null, or use conditional logic to ensure the assignment only
happens when prevLines is a valid string.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5242c2be-df6b-4c45-9fab-e8a98633d439

📥 Commits

Reviewing files that changed from the base of the PR and between c7d3c81 and 0f1cb60.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • package.json
  • src/utils/log-update.ts

Comment thread src/utils/log-update.ts
Comment thread src/utils/log-update.ts
@mogzol
mogzol force-pushed the fix-counting-lines branch from 0f1cb60 to 4a335b0 Compare June 24, 2026 00:15
Previously, extra lines were counted only by the number of line breaks
in them, so the number of lines cleared would be incorrect if any lines
wrapped. Additionally, prevLineCount could be wrong if the size of the
terminal changes between renders.

To fix this, the number of lines to clear is calculated immediately
before clearing them, and the 'string-width' library is used to
determine accurate widths. This is the same library used by 'wrap-ansi',
so it should be reliable, and it is not actually a new dependency.
@mogzol
mogzol force-pushed the fix-counting-lines branch from 4a335b0 to 4f7d1de Compare June 24, 2026 00:32
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.

Bar gets duplicated if a long line is printed during build

1 participant