Merge develop to main

[yimsk]

Summary

• feat: add --service and --resource-id flags for startup navigation (Feature Proposal: Start TUI Focusing On A Service/Resource #97) (feat: add --service and --resource-id flags for startup navigation (#97) #100)
• fix: stay on current view after profile/region change (fix: stay on current view after profile/region change #98)

Merge Strategy

MERGE COMMIT (not squash - per AGENTS.md)

[claude]

Pull Request Review - PR #101: Merge develop to main

Summary

This PR introduces two key features:

1. Startup navigation flags (--service and --resource-id) for direct navigation to services/resources
2. UX improvement to stay on the current view after profile/region changes instead of popping back

Code Quality & Best Practices ✅

Strengths:

• Excellent test coverage: New functionality has comprehensive tests (120 lines in startup_test.go, 130+ lines in app_test.go)
• Well-structured separation of concerns: resolveStartupService() is cleanly separated and testable
• Good error handling: Proper validation with helpful error messages (e.g., requiring --service when using --resource-id)
• Follows Go conventions: Clear naming, proper use of pointers for optional values
• Documentation: README updated with examples, good inline comments

Minor Suggestions:

1. Registry default resources map location (cmd/claws/main.go:243-292)
The defaultResources map in internal/registry/registry.go:489-531 is quite comprehensive. Consider:

• Moving this map to a separate file like internal/registry/defaults.go for better organization
• Adding a comment explaining the criteria for choosing defaults (appears to be most commonly used resource type)

2. Error message consistency (cmd/claws/main.go:237-271)
Consider making error messages more consistent and actionable with suggestions for the user.

Potential Bugs/Issues ⚠️

1. Minor: Resource ID trimming inconsistency (cmd/claws/main.go:67)
While service is trimmed before passing to resolveStartupService() (line 59), resourceID is only trimmed when creating the struct. This is fine but consider trimming both at parse time for consistency.

2. Edge case: Empty service after trim (cmd/claws/main.go:59)
If user provides --service " " (only spaces), the trimmed result is empty string, which will fail in resolveStartupService. The error message "unknown service:" might be confusing. Consider adding explicit validation for empty strings.

3. Potential nil pointer dereference (internal/app/app.go:362-374)
In the startupResourceMsg handler, if a.startupPath becomes nil after initialization (edge case), the code could panic when accessing a.startupPath.ResourceID and a.startupPath.Service. The nil check at line 362 protects against this.

Performance Considerations 🚀

1. Startup resource fetch is non-blocking ✅
The fetchStartupResource() is called asynchronously via tea.Batch() (app.go:152), which is excellent - it won't block app initialization.

2. Navigation stack management is efficient ✅
The refactor from popToRefreshableView() to refreshCurrentView() (app.go:681-691) is a significant UX improvement and actually reduces unnecessary view popping/initialization overhead.

Security Concerns 🔒

1. Input validation looks good ✅

• Profile name validation: config.IsValidProfileName() (main.go:43)
• Region validation: config.IsValidRegion() (main.go:48)
• Service/resource path validation: checks for multiple slashes (main.go:249)

2. No injection vulnerabilities detected ✅

• User input (service, resourceID) is validated before use
• Error messages properly use %s formatting instead of string concatenation

Test Coverage 📊

Excellent Coverage:

• startup_test.go: 16 test cases covering edge cases like default resource resolution, alias handling, multiple slashes rejection, empty strings, and trailing slashes
• app_test.go: 7 new test cases for navigation behavior covering profile/region changes on refreshable and non-refreshable views, plus edge cases

Missing Tests:

1. Integration test for startup path flow: While unit tests cover parsing, consider adding an integration test that verifies the full flow from CLI flag to DetailView
2. Error cases in fetchStartupResource(): Test what happens when DAO.Get() returns various error types
3. Concurrent startup scenarios: Test behavior when startupResourceMsg arrives before/after awsContextReadyMsg

Additional Observations

Good Practices Found:

• Backward compatibility: All existing tests pass (no breaking changes)
• Progressive enhancement: Features work independently (--service without --resource-id)
• User-friendly errors: Clear examples in error messages (main.go:71)
• Consistent naming: StartupPath struct mirrors the concept well

Recommendations

Priority: High

None - code is production-ready

Priority: Medium

1. Add explicit validation for empty service string after trimming
2. Add integration test for complete startup navigation flow

Priority: Low

1. Consider organizing defaultResources map into a separate file
2. Make error messages more actionable with suggestions
3. Minor: Consider inlining popView() or using it in more places for consistency

Conclusion

This is high-quality code that is ready to merge. The implementation is clean, well-tested, and follows Go best practices. The new features are valuable additions that improve user experience significantly. The test coverage is exceptional, and the code maintains backward compatibility.

Recommendation: Approve and Merge ✅

Great work on:

• Comprehensive test coverage
• Clean separation of concerns
• Thoughtful UX improvements (staying on current view)
• Good error handling and validation

---

Review conducted by Claude Code