feat: add --service and --resource-id flags for startup navigation (#97)

[yimsk]

Summary

• Add -s/--service flag to start directly on a service/resource (e.g., claws -s ec2, claws -s rds/snapshots)
• Add -i/--resource-id flag to open detail view for specific resource (requires --service)
• Consolidate default resource selection via registry.DefaultResource() across codebase
• Extract ReadOnlyBadgeStyle() to ui/theme.go following style helper pattern

Changes

File	Change
cmd/claws/main.go	New flags, resolveStartupService() with alias support
cmd/claws/startup_test.go	Tests for service resolution
internal/app/app.go	StartupPath struct, startup navigation handling
internal/registry/registry.go	DefaultResource(), defaultResources map (31 services)
internal/view/command_input.go	Use DefaultResource() instead of ListResources()[0]
internal/view/resource_browser.go	Use DefaultResource() instead of ListResources()[0]
internal/ui/theme.go	Extract ReadOnlyBadgeStyle()

Usage

claws -s ec2              # Open EC2 instances browser
claws -s rds/snapshots    # Open RDS snapshots browser  
claws -s cfn              # Open CloudFormation stacks (alias)
claws -s ec2 -i i-12345   # Open detail view for instance i-12345

Closes #97

[yimsk]

Review Response (4th round)

✅ #2 Leading slash validation

• /instances テストケース追加（空serviceでエラーになることを確認）

✅ #3 Error message UX

• --resource-id のみ指定時のエラーに使用例を追加

Race condition について

前回説明の通り、ResourceBrowser.Init() も AWS 初期化完了前にリソースロードを開始する既存パターン。AWS SDK v2 が内部でクレデンシャル解決をハンドル。コードベース全体で一貫した設計のため変更不要。

Commit: 7be143a

[claude]

Pull Request Review: feat: add --service and --resource-id flags for startup navigation (#97)

Overall, this is a well-implemented feature with excellent test coverage and thoughtful design. The code quality is high, and the implementation follows good practices. Below are my detailed findings:

---

✅ Strengths

1. Code Quality & Best Practices

• Clean separation of concerns: resolveStartupService() handles validation/resolution, fetchStartupResource() handles async fetching
• Excellent use of dependency injection: StartupPath passed to App.New() rather than using globals
• Good error handling with user-friendly messages and examples (cmd/claws/main.go:218-219)
• Consistent naming conventions and clear function documentation
• Thread-safe DefaultResource() implementation with proper mutex usage (internal/registry/registry.go:535-543)

2. Test Coverage

• Comprehensive edge case testing in cmd/claws/startup_test.go:
  • Empty strings, trailing slashes, multiple slashes, leading slashes
  • Alias resolution, default resource selection
  • Unknown services and resources
• Registry tests cover concurrency scenarios (registry_test.go:342-358)
• 13 test cases for resolveStartupService() covering all code paths

3. User Experience

• Helpful error messages with examples (e.g., "Example: claws -s ec2 -i i-1234567890abcdef0")
• Input trimming with strings.TrimSpace() for robustness (cmd/claws/main.go:57, 66)
• Graceful degradation: startup resource errors show as flash messages rather than crashing
• Good documentation in README with clear examples

---

🔍 Areas for Improvement

1. Potential Race Condition (Minor)

Location: internal/app/app.go:152-153

if a.startupPath != nil && a.startupPath.ResourceID != "" {
    cmds = append(cmds, a.fetchStartupResource)
}

Issue: The fetchStartupResource function is called as a background command, but if the resource fetch completes before AWS initialization (initAWSCmd), the DAO creation might fail or use stale credentials.

Recommendation: Consider chaining the resource fetch after AWS init completes, or add defensive error handling in fetchStartupResource for the case where AWS context isn't ready.

---

2. DAO Error Handling Could Be Clearer

Location: internal/app/app.go:375-378

d, err := a.registry.GetDAO(a.ctx, a.startupPath.Service, a.startupPath.ResourceType)
if err != nil {
    log.Warn("failed to get DAO for startup resource", "error", err)
}

Issue: The code logs a warning but continues to pass a potentially nil DAO to NewDetailView(). The comment on line 373 mentions "DAO is optional - DetailView handles nil gracefully", which is good, but this creates an inconsistency:

• Line 369-371: Requires renderer (fails if not found)
• Line 375-378: DAO is optional (continues on error)

Recommendation: Add a comment explaining why DAO failure is non-fatal, or verify that DetailView actually handles nil DAO correctly. Consider checking if this behavior is consistent with the rest of the codebase.

---

3. Validation Gap: Resource ID Without Valid DAO

Location: cmd/claws/main.go:57-71

if opts.service != "" {
    service, resourceType, err := resolveStartupService(strings.TrimSpace(opts.service))
    // ...
    startupPath = &app.StartupPath{
        Service:      service,
        ResourceType: resourceType,
        ResourceID:   strings.TrimSpace(opts.resourceID),
    }
}

Issue: resolveStartupService() validates that the service/resource exists in the registry, but it doesn't verify that the resource has a DAO with a Get() method. If a resource only has a renderer (no DAO), the startup will succeed but fail later when trying to fetch the resource by ID.

Recommendation: Add early validation in main.go when opts.resourceID != "" to ensure the resource actually supports Get() operations. Example:

if opts.resourceID != "" {
    if _, err := registry.Global.GetDAO(ctx, service, resourceType); err != nil {
        fmt.Fprintf(os.Stderr, "Error: resource %s/%s does not support --resource-id\n", service, resourceType)
        os.Exit(1)
    }
}

---

4. Input Validation Edge Case

Location: cmd/claws/main.go:242-251

parts := strings.SplitN(input, "/", 2)
service := parts[0]
if len(parts) > 1 {
    resourceType = parts[1]
}

if strings.Contains(resourceType, "/") {
    return "", "", fmt.Errorf("invalid resource type: %s", resourceType)
}

Issue: The validation on line 248-250 is redundant because SplitN(input, "/", 2) only splits into 2 parts maximum. If input is "ec2/volumes/extra", resourceType would be "volumes/extra", and the check catches it. However, the test case at startup_test.go:85-87 tests this scenario.

Minor note: This is working correctly but could be simplified. The current implementation is defensive and clear, so this is optional.

---

5. Concurrency Safety in DefaultResource (Addressed, but worth noting)

Location: internal/registry/registry.go:535-543

func (r *Registry) DefaultResource(service string) string {
    r.mu.RLock()
    userDefault := r.userDefaults[service]
    r.mu.RUnlock()
    // ...
}

Good: The implementation correctly uses RLock() for reading user defaults.

Potential issue: The defaultResources map is a package-level variable (line 490) that's accessed without locking. This is safe in this codebase because defaultResources is never modified after init, but it's worth documenting this assumption.

Recommendation: Add a comment above defaultResources explaining that it's read-only after initialization.

---

🔒 Security Considerations

No significant security issues found. The implementation:

• ✅ Properly validates user input (service names, resource IDs)
• ✅ Uses strings.TrimSpace() to prevent whitespace injection
• ✅ Checks for invalid characters (multiple slashes, leading slashes)
• ✅ No command injection risks (inputs are not passed to shell)
• ✅ No SQL injection risks (inputs validated against registry)
• ✅ No path traversal risks (inputs are service/resource names, not file paths)

---

⚡ Performance Considerations

Generally good, with one observation:

Parallel Command Execution: The Init() function batches commands (tea.Batch(cmds...) at app.go:156), which is correct for parallel execution. However, the fetchStartupResource may retry or timeout if AWS isn't ready yet.

Recommendation: Monitor startup time with -i flag to ensure resource fetching doesn't block the UI from appearing.

---

🧪 Test Coverage Assessment

Excellent coverage overall:

• ✅ Unit tests for resolveStartupService() with 13 scenarios
• ✅ Registry tests for DefaultResource()
• ✅ Concurrency tests for registry operations
• ✅ Edge cases well covered (empty strings, malformed input)

Missing tests:

• ❌ Integration test for --service and --resource-id flags end-to-end
• ❌ Test for startup resource fetch failure scenarios
• ❌ Test for the race condition between AWS init and resource fetch

Recommendation: Add an integration test in internal/app/app_test.go that verifies:

1. Starting with --service ec2 navigates to EC2 instances
2. Starting with invalid --resource-id shows error flash message
3. Starting with valid --resource-id opens detail view

---

📝 Summary & Recommendations

Priority Fixes:

1. Medium: Add validation in main.go to ensure resources with --resource-id actually have a DAO
2. Low: Document the race condition between AWS init and resource fetch, or add synchronization
3. Low: Add integration tests for the new flags

Optional Improvements:

1. Add comment to defaultResources explaining it's immutable
2. Consider simplifying the multi-slash validation (current code works fine)

---

✅ Approval Status

This PR is ready to merge with the above considerations noted. The implementation is solid, well-tested, and follows the repository's conventions. The identified issues are minor and can be addressed in follow-up PRs if needed.

Great work on this feature! 🎉

---

Reviewed by: Claude Code (Automated Review)
Review Date: 2026-01-04