Skip to content

fix: infinite loop on error#8269

Merged
hrishikesh-k merged 3 commits into
mainfrom
hk/skip-system-info
May 21, 2026
Merged

fix: infinite loop on error#8269
hrishikesh-k merged 3 commits into
mainfrom
hk/skip-system-info

Conversation

@hrishikesh-k
Copy link
Copy Markdown
Contributor

🎉 Thanks for submitting a pull request! 🎉

Summary

Next iteration of: #8268. This PR should fix the original issue of the CLI ending in a loop. It should exit as soon as the error is triggered.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 20, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 38c71a09-94f6-4ced-b498-0f5f02927045

📥 Commits

Reviewing files that changed from the base of the PR and between fb840f5 and c5caf74.

📒 Files selected for processing (1)
  • src/commands/main.ts
💤 Files with no reviewable changes (1)
  • src/commands/main.ts

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Prevented duplicate processing of uncaught exceptions to avoid repeated error handling and notifications.
    • Added timeout-protected diagnostics collection and improved error logging to avoid hangs and ensure the application exits cleanly after a fatal error.

Walkthrough

This PR refactors the uncaught-exception handler in src/commands/main.ts to improve robustness. It adds a re-entrancy guard to prevent double-processing, introduces a timeout for system diagnostics collection to avoid hanging, reorders logging to show the stack trace before system info, conditionally logs diagnostics only if gathered within the timeout window, and moves the process exit into a finally block to guarantee cleanup. The existing EADDRINUSE port-in-use messaging is retained, but the reportError call is removed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: infinite loop on error' directly corresponds to the main change: adding a re-entrancy guard to prevent repeated uncaught exceptions from triggering multiple handlers, which fixes the infinite loop issue.
Description check ✅ Passed The description clearly relates to the changeset by referencing the original issue (CLI ending in a loop) and stating the fix should make the CLI exit immediately, which aligns with the error handler modifications.
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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch hk/skip-system-info

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 20, 2026

📊 Benchmark results

Comparing with 1134321

  • Dependency count: 1,134 (no change)
  • Package size: 379 MB (no change)
  • Number of ts-expect-error directives: 355 (no change)

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/commands/main.ts (1)

108-112: ⚡ Quick win

Remove behavior-narrating comments in the exception handler.

These inline comments explain what the code already shows; please drop them to match repo style.

Suggested cleanup
-      // print the error regardless of systemInfo
       console.log(chalk.dim(err.stack || err))

-      // systemInfo sometimes could get stuck resulting in exit() never called
-      // we now prevent that by bailing early so the CLI quits in time
       const systemInfo = await Promise.race([
         getSystemInfo().catch(() => ''),
         new Promise<string>((resolve) =>

As per coding guidelines, "**/*.{ts,tsx,js,jsx}: Never write comments on what the code does; make the code clean and self-explanatory instead".

🤖 Prompt for 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.

In `@src/commands/main.ts` around lines 108 - 112, Remove the behavior-narrating
comments inside the exception handler in main.ts: delete the two comment lines
above and below the console.log call (the lines explaining printing the error
and systemInfo/exit behavior) so the handler only contains the executable
statements (e.g., the console.log(chalk.dim(err.stack || err)) line and
surrounding logic) and no comments describing what the code does.
🤖 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.

Nitpick comments:
In `@src/commands/main.ts`:
- Around line 108-112: Remove the behavior-narrating comments inside the
exception handler in main.ts: delete the two comment lines above and below the
console.log call (the lines explaining printing the error and systemInfo/exit
behavior) so the handler only contains the executable statements (e.g., the
console.log(chalk.dim(err.stack || err)) line and surrounding logic) and no
comments describing what the code does.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 52ad2bef-6dcf-4847-9f33-7584a72812e4

📥 Commits

Reviewing files that changed from the base of the PR and between dfcc0a2 and fb840f5.

📒 Files selected for processing (1)
  • src/commands/main.ts

Copy link
Copy Markdown
Contributor

@jaredm563 jaredm563 left a comment

Choose a reason for hiding this comment

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

Great job!

@hrishikesh-k hrishikesh-k enabled auto-merge (squash) May 21, 2026 08:30
@hrishikesh-k hrishikesh-k merged commit a3549b8 into main May 21, 2026
40 checks passed
@hrishikesh-k hrishikesh-k deleted the hk/skip-system-info branch May 21, 2026 08:38
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