fix(domain): add --domain validation and fix spinner leaks in domain create#222
Open
stantheman0128 wants to merge 1 commit intozeabur:mainfrom
Open
Conversation
…create In non-interactive mode, `domain create` proceeds with an empty domain name if --domain is not provided, causing a confusing API error. Add validation to require the --domain flag. Also fix spinner goroutine leaks where s.Stop() is not called before returning on error in both interactive and non-interactive paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughThe changes add explicit spinner cleanup on error paths in domain creation functions and introduce validation to ensure the domain name is provided when running in non-interactive mode. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmd/domain/create/create.go`:
- Around line 167-169: The current non-interactive validation only checks
opts.domainName == "" and allows whitespace-only names; change the check to trim
whitespace (e.g., use strings.TrimSpace on opts.domainName) and treat a trimmed
empty string the same as missing, returning the existing error
fmt.Errorf("--domain is required in non-interactive mode"); update the
validation in the same function where opts.domainName is currently checked so
whitespace-only inputs are rejected early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1eeadaf-a745-4546-b8e1-d7173cb19c6a
📒 Files selected for processing (1)
internal/cmd/domain/create/create.go
Comment on lines
+167
to
+169
| if opts.domainName == "" { | ||
| return fmt.Errorf("--domain is required in non-interactive mode") | ||
| } |
There was a problem hiding this comment.
Harden --domain validation against whitespace-only input.
Line 167 currently rejects only "". Values like " " still pass and can fail later with a confusing API error.
🔧 Suggested patch
import (
"context"
"fmt"
+ "strings"
@@
- if opts.domainName == "" {
+ if strings.TrimSpace(opts.domainName) == "" {
return fmt.Errorf("--domain is required in non-interactive mode")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if opts.domainName == "" { | |
| return fmt.Errorf("--domain is required in non-interactive mode") | |
| } | |
| import ( | |
| "context" | |
| "fmt" | |
| "strings" | |
| ) | |
| // ... existing code ... | |
| if strings.TrimSpace(opts.domainName) == "" { | |
| return fmt.Errorf("--domain is required in non-interactive mode") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmd/domain/create/create.go` around lines 167 - 169, The current
non-interactive validation only checks opts.domainName == "" and allows
whitespace-only names; change the check to trim whitespace (e.g., use
strings.TrimSpace on opts.domainName) and treat a trimmed empty string the same
as missing, returning the existing error fmt.Errorf("--domain is required in
non-interactive mode"); update the validation in the same function where
opts.domainName is currently checked so whitespace-only inputs are rejected
early.
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
1 participant
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
--domainflag validation in non-interactive mode — previously, omitting--domainwould pass an empty string to the API, producing a confusing server errors.Stop()was not called before returningChanges
runCreateDomainNonInteractive--domainempty check with clear error messagerunCreateDomainNonInteractiveline 192s.Stop()before returning onAddDomainerrorrunCreateDomainInteractiveline 111s.Stop()before returning onCheckDomainAvailableerrorrunCreateDomainInteractiveline 128s.Stop()before returning onListDomainserrorReproducing the bug
Test plan
domain create --id <id> -i=falsewithout--domainreturns clear errordomain create --id <id> --domain test -g -i=falsestill works🤖 Generated with Claude Code
Summary by CodeRabbit