Skip to content

fix: prevent progress spinner from cluttering error output#116

Open
ManjunathByadagi wants to merge 1 commit into
shriyanss:mainfrom
ManjunathByadagi:fix-spinner-error-output
Open

fix: prevent progress spinner from cluttering error output#116
ManjunathByadagi wants to merge 1 commit into
shriyanss:mainfrom
ManjunathByadagi:fix-spinner-error-output

Conversation

@ManjunathByadagi

@ManjunathByadagi ManjunathByadagi commented May 27, 2026

Copy link
Copy Markdown

What Changed

Cleared the MCP thinking spinner before printing both success and error output so terminal messages render on clean lines without colliding with the active spinner.

Also centralized spinner cleanup logic to ensure consistent behavior across success, failure, and abort flows.

Root Cause

The spinner interval continued running while error messages were printed, causing the terminal to redraw the animated spinner on the same line as the error output.

How It Was Tested

  • Rebuilt the project using npm run cleanup

  • Ran multiple failing MCP chat attempts using an invalid API key

  • Verified that:

    • errors render on separate clean lines
    • spinner stops correctly
    • terminal prompt restores normally afterward

Summary by CodeRabbit

  • Refactor
    • Improved internal chat session handling and error management for enhanced stability.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

Failed to post review comments

📝 Walkthrough

Walkthrough

The change refactors spinner management in the interactive chat session flow by extracting cleanup logic into a dedicated helper function, improving type safety with explicit interval typing, and ensuring consistent cleanup in both success and error paths.

Changes

Thinking Spinner Cleanup

Layer / File(s) Summary
Spinner cleanup helper and error handling
src/mcp/cli.ts
clearThinkingSpinner helper consolidates interval clearing and console state reset. Spinner cleanup is now called consistently in success and error paths; spinnerInterval is explicitly typed as ReturnType<typeof setInterval>, console clearing width is adjusted, and error handling distinguishes AbortError from other failures while resetting session.currentAbortController.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A spinner spins, then must be tamed,
Cleanup helper keeps it well-framed,
Success or error, it knows the way,
When to clear and reset the display!

🚥 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 accurately summarizes the main change: preventing a progress spinner from cluttering error output by clearing it before printing errors, which matches the core purpose of refactoring the spinner cleanup logic.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

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

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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 and usage tips.

@ManjunathByadagi

Copy link
Copy Markdown
Author

Hi! Just following up on the PR. Please let me know if any additional changes or improvements are needed. Thanks!

@shriyanss

Copy link
Copy Markdown
Owner

Hi @ManjunathByadagi,
Thanks for the PR. Could you please provide the following:

  • Screenshot of the problem (the cluttered display)
  • Screenshot of solution (the code in PR)

Also, please change the target branch to shriyanss:dev

Thanks

@ManjunathByadagi

Copy link
Copy Markdown
Author

Hi @shriyanss,

I checked the PR page, but GitHub isn't showing me the option to change the base branch from main to dev.

Could you please retarget the PR to dev, or let me know if you'd prefer that I open a new PR against dev instead?

I'll upload the requested screenshots shortly.

Thanks!

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