Skip to content

fix: clean up spinner on Commander errors#41

Open
jacobo-doist wants to merge 2 commits into
mainfrom
jacobo/fix-stuck-spinner
Open

fix: clean up spinner on Commander errors#41
jacobo-doist wants to merge 2 commits into
mainfrom
jacobo/fix-stuck-spinner

Conversation

@jacobo-doist

Copy link
Copy Markdown

Summary

  • Stop the early loading spinner before Commander writes stdout or stderr.
  • Use Commander exitOverride() so parse/help/version exits go through normal cleanup instead of direct process.exit().
  • Add an entrypoint regression test for Commander parse errors.

Failure scenario

Anonymized reproduction from the original report:

tdc conversation view https://comms.todoist.com/<workspace-id>/msg/<message-id>/ --from 2026-06-26

Before this change, Commander printed:

error: unknown option '--from'

but the early Loading... spinner could remain active/corrupt the terminal until reset because Commander exited before the spinner cleanup path ran.

Testing

  • npm test
  • pre-commit hook: oxfmt, oxlint, npm run type-check
  • pre-push hook: npm test

@doistbot doistbot requested a review from nats12 July 10, 2026 10:46
@jacobo-doist jacobo-doist changed the title Fix stuck spinner on Commander parse errors fix: stuck spinner on Commander parse errors Jul 10, 2026

@doistbot doistbot 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.

This PR fixes the stuck early-loading spinner on Commander parse errors by adding exitOverride() and configureOutput hooks that stop the spinner before Commander writes to stdout/stderr, plus a regression test.

Few things worth tightening:

  • The configureOutput setup is applied to the root program, but the routed-command parser created in runRoutedCommand() (e.g. tdc view <conversation-url>) gets only exitOverride() without the spinner-stopping output hooks — that proxy parser's unknown-option errors can still leave the spinner running. Consider extracting a shared command-output setup helper and applying it to both.
  • The test assertion pins Commander's exact error string ("error: unknown option '--from'\n"), which the codebase doesn't control. A partial match like expect.stringContaining("--from") would be more resilient to Commander wording changes while still proving the error reached stderr.

I also included a few optional follow-up notes in the details below.

Optional follow-up note (1)
  • P3 src/index.test.ts:65: This assertion pins Commander's exact error message format ("error: unknown option '--from'\n"), which the codebase doesn't control. A Commander minor/patch that tweaks wording or punctuation would break the test even though the behavior under test (spinner stops before error output, clean exit with code 1) is still correct. The ordering assertion on line 66–68 already proves the spinner stopped before stderr was written. For the content check, a partial match would be more resilient, e.g.: ts expect(stderrWriteSpy).toHaveBeenCalledWith(expect.stringContaining("--from")) This still confirms the parse error reached stderr without coupling to Commander's exact phrasing.

Share FeedbackReview Logs

Comment thread src/index.ts Outdated
@jacobo-doist jacobo-doist changed the title fix: stuck spinner on Commander parse errors fix: clean up spinner on Commander errors Jul 10, 2026
@jacobo-doist

jacobo-doist commented Jul 10, 2026

Copy link
Copy Markdown
Author

Also addressed the optional test assertion note in c205b0f. The test now uses expect.stringContaining("--from") instead of pinning Commander’s exact error text, while keeping the ordering assertion that proves the spinner is stopped before stderr is written.

-- Replied by Clanker 🤖

@jacobo-doist

Copy link
Copy Markdown
Author

@doistbot /review

@doistbot doistbot 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.

This PR updates fix: clean up spinner on Commander errors. No issues were flagged in the reviewed diff.

Share FeedbackReview Logs

nats12

This comment was marked as duplicate.

@nats12 nats12 self-requested a review July 13, 2026 08:57

@nats12 nats12 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.

🚀 works as expected, the loader is removed.

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.

3 participants