fix(cli): withLoading no-ops when stream is not a TTY#14
Merged
Conversation
isTTY=false is now an unconditional short-circuit, regardless of FORCE_COLOR. Painting spinner frames into a pipe corrupts downstream consumers (captured stdout, CI buffers, log files), so the spinner must never animate when the stream is not a real terminal. isSpinnerEnabled still honors FORCE_COLOR for callers that want to ask the question, but withLoading's stronger contract — documented in its own header comment — wins on the no-op path.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
withLoadingnow short-circuits unconditionally whenstream.isTTY !== true, restoring the no-op contract documented in its own header comment. No writes, no spinner setup, justawait fn().Root cause
withLoadinggated the no-op path onisSpinnerEnabled(stream), which returnstruewhenFORCE_COLOR=1is set in the env (intentional behavior — that's what test S5 asserts). In environments whereFORCE_COLORis exported (e.g. the local dev shell here hasFORCE_COLOR=3),isSpinnerEnabled(stream)returnstrueeven withisTTY=false, sowithLoadingpainted a spinner frame and a cleanup erase into the fake stream — two writes where the contract requires zero.The doc comment on
withLoadingalready stated the stronger contract: "TTY-false path: zero writes, zero overhead". The implementation just didn't honor it.Fix
Add an explicit
if (stream.isTTY !== true) return fn();guard before theisSpinnerEnabledcheck insidewithLoading.isSpinnerEnabledis left untouched — itsFORCE_COLORsemantics are still useful for callers that want to ask the question — butwithLoading's stronger "never animate into a pipe" rule wins on its own path.Why the stronger rule is correct: animated cursor-return frames written to a non-TTY corrupt downstream consumers (captured stdout, CI log buffers, file redirects).
FORCE_COLORis meant to opt INTO color in pipes where the producer knows the consumer can render ANSI — it should not opt into an animated spinner, which assumes a live terminal that handles\r.Testing checklist
bun test src/cli/loading.test.ts— 15/15 pass (was 13/15)bun testfull suite — 237/237 pass, no regressionsisSpinnerEnabledtruth-table cases still pass