-
Notifications
You must be signed in to change notification settings - Fork 7
fix: prevent terminal cursor from disappearing after breaking change … #373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…error by returning error instead of os.Exit(1) in checkForBreakingChanges
WalkthroughThe function Changes
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/bundler/upgrade.go (2)
75-75: Fix trailing space in version constraint.There's a trailing space in the version constraint that could potentially cause semver parsing issues.
- Version: "<0.0.115 ", + Version: "<0.0.115",
77-77: Fix typo in migration message.There's a typo "aand" in the migration message.
- Message: "The environment variable and code reference for your Agentuity API key has changed from AGENTUITY_API_KEY to AGENTUITY_SDK_KEY. Update all occurrences in your .env files and codebase. See the v0.0.115 Changelog for details.\n\n" + tui.Link("https://agentuity.dev/Changelog/sdk-js#v00115") + "\n\nAfter migrated, please run npm upgrade @agentuity/sdk aand then re-run this command again.", + Message: "The environment variable and code reference for your Agentuity API key has changed from AGENTUITY_API_KEY to AGENTUITY_SDK_KEY. Update all occurrences in your .env files and codebase. See the v0.0.115 Changelog for details.\n\n" + tui.Link("https://agentuity.dev/Changelog/sdk-js#v00115") + "\n\nAfter migrated, please run npm upgrade @agentuity/sdk and then re-run this command again.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/bundler/upgrade.go(1 hunks)
🔇 Additional comments (3)
internal/bundler/upgrade.go (3)
243-243: LGTM! Good improvement for graceful error handling.The change from
os.Exit(1)to returning an error after successful migration allows the caller to handle cleanup properly, which aligns perfectly with the PR objective to prevent terminal cursor disappearance.
250-250: LGTM! Consistent error handling approach.Replacing the immediate exit with error return maintains consistency with the function's error handling pattern and allows proper UI cleanup.
252-252: LGTM! Proper replacement of logger.Fatal.Converting the fatal log call to error return is the correct approach, ensuring the caller can handle the error gracefully instead of abruptly terminating.
…error by returning error instead of os.Exit(1) in checkForBreakingChanges
We can't just quit in the middle of a UI process, we need to gracefully quit after cleaning up. Please dont call panic or os.Exit when we're in a UI session
Summary by CodeRabbit