fix: wrap and indent spinner messages#584
Conversation
🦋 Changeset detectedLatest commit: a259bea The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
dreyfus92
left a comment
dreyfus92
left a comment
There was a problem hiding this comment.
awesome job @beeequeue left a few comments, it would be nice that @43081j also jumps into this ✌🏻
| trim: false, | ||
| }); | ||
| output.write(wrapped); | ||
| output.write(outputMessage); |
There was a problem hiding this comment.
looks like message() slipped thru, it still sets _message raw in line 203 but the per frame wrap that used to catch it is gone now. so anything long/multiline set mid-spin renders unwrapped until stop. should probable mirror what start() does on line 136 and also a test calling .message() with a columns + 10 string would cover the gap.
| return min > 0 ? `[${min}m ${secs}s]` : `[${secs}s]`; | ||
| }; | ||
|
|
||
| const prefix = `${styleText('gray', S_BAR)} `; |
There was a problem hiding this comment.
head up that wrapTextWithPrefix does column - prefix.length and this prefix has ansi codes in it, so it's ~13 chars instead of 3 visible cols. messages wrap ~ 10 columns early. not this PR's fault necessarily but passing a styled prefix here surfaces it, might be worth stripping ansi (or using visible width) in core as follow up.
| _origin = performance.now(); | ||
| if (hasGuide) { | ||
| output.write(`${styleText('gray', S_BAR)}\n`); | ||
| output.write(`${prefix}\n`); |
There was a problem hiding this comment.
could we keep this as just the bar? the 2 trailing spaces from prefix are why basically every snapshot in both files changed. writing trailing whitespace doesn't buy anything and would shrink the diff a ton.
| hard: true, | ||
| trim: false, | ||
| }); | ||
| output.write(wrapped); |
There was a problem hiding this comment.
pre-existing rather than introduced here, but this diff changes its failure mode: clearPrevMessage counts erase lines from _prevMessage alone, but what's on screen is ${frame} ${_message}${loadingDots} which is 3 cols wider. with withGuide: false and a near-full-width message, the output soft-wraps one row more than counted and a stale line survives the erase. repro (fake 40-col output, start('x'.repeat(40))):
main: "• " <- stale stub
"o done"
this branch: "• xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" <- stale full line
"o done"so same bug, but without the write-time wrap the leftover is now a full line of content instead of a 3-char stub. proper fix is probably counting from the actually-written string (needs its own var, the isCI dedup compares _message === _prevMessage and the frame changes every tick). fine as a follow-up, just wanted it on record
|
there's been a few attempts at this same change but i've always held off on it since the real fix is to make the spinner use the prompt API. we're a bit blocked there though since we'd end up reintroducing the windows spinner bug (main has one too, just a different bug, including in this PR). i think lets hold off on merging this until i can catch myself up on the state of the prompt PR. if we can land that anytime soon then it'd make this one redundant |
3 participants
What does this PR do?
makes sure the printed messages in
spinnerand its dependent prompts are correctly wrapped and indented.Screenshots
Before:
After:
Type of change
Checklist
pnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
i spotted #479 which might also fix this issue, but it seems to have lost momentum and i think this is a reasonable patch until it's done