feat: configurable startup view with navigation improvements

[yimsk]

Summary

• Add startup.view config (dashboard/services/service/resource)
• Consolidate view resolution logic in registry.ParseServiceResource()
• Add navigation.max_stack_size config
• Add :clear-history, :pulse, :dashboard commands

Changes

• internal/registry: Add ParseServiceResource() for centralized parsing
• internal/config: Add startup.view and navigation.max_stack_size
• internal/app: Use resolveStartupView() helper
• internal/view: Update command semantics (: → services, :pulse → dashboard)
• cmd/claws: Simplify resolveStartupService()

Test plan

• All tests pass
• Config defaults work
• CLI -s flag precedence
• Command navigation behavior

🤖 Generated with Claude Code

[claude]

Pull Request Review: Configurable Startup View with Navigation Improvements

Summary

This PR adds flexible startup configuration, consolidates service/resource parsing logic, and improves navigation semantics. Overall, this is a well-structured PR with good test coverage and clear intent. Below are my findings organized by category.

---

✅ Strengths

1. Excellent Consolidation: Moving service/resource parsing logic into registry.ParseServiceResource() is a great architectural improvement. This eliminates code duplication between cmd/claws/main.go and internal/view/command_input.go.

2. Comprehensive Test Coverage: The new tests in registry_test.go and command_input_test.go cover edge cases well (aliases, invalid inputs, multiple slashes, etc.).

3. Backward Compatibility: The changes maintain backward compatibility with existing CLI usage patterns while adding new functionality.

4. Good Documentation: Updates to keybindings.md and configuration.md clearly explain the new features.

5. Consistent Navigation Semantics: Changing empty command (:) to go to services instead of dashboard makes sense as a "home" navigation, with :pulse as the explicit dashboard command.

---

🐛 Potential Issues

1. Inconsistent Stack Clearing Semantics (internal/view/command_input.go:219-243)

The PR changes navigation to use ClearStack: false for all commands, but this creates inconsistency:

// Line 219 - services browser
return nil, &NavigateMsg{View: browser, ClearStack: false}

// Line 233 - pulse/dashboard  
return nil, &NavigateMsg{View: dashboard, ClearStack: false}

// Line 248 - services/browse
return nil, &NavigateMsg{View: browser, ClearStack: false}

Issue: Previously, dashboard navigation cleared the stack (treated as "home"). Now nothing clears the stack except explicit :clear-history. This could lead to unbounded stack growth for users who frequently toggle between views.

Recommendation: Consider whether dashboard (:pulse) should clear the stack as a "reset to home" action, while services browser preserves history. The ~ toggle makes sense to preserve history, but explicit :pulse might be intended as a reset.

2. Max Stack Size Could Lose Important Navigation History (internal/app/app.go:686-691)

if len(a.viewStack) > maxSize {
    // Remove oldest entries
    a.viewStack = a.viewStack[len(a.viewStack)-maxSize:]
}

Issue: This removes the oldest entries when the stack is full. If a user has a deep navigation path (Services → EC2 → Instance → VPC → Security Group → ...), they'll lose the ability to navigate back to the beginning.

Recommendation: Consider whether this is the desired behavior, or if there should be a warning/message when the stack is truncated. Alternatively, consider whether a circular buffer or different strategy might be better.

3. Missing Error Handling in resolveStartupView (internal/app/app.go:847-851)

default:
    // Try to parse as AWS service/resource (e.g., "ec2", "rds/snapshots")
    service, resourceType, err := a.registry.ParseServiceResource(viewName)
    if err != nil {
        // Fallback to ServiceBrowser on error
        return view.NewServiceBrowser(a.ctx, a.registry)
    }

Issue: Silent fallback on error could mask configuration mistakes. If a user configures startup.view: "ecs" but meant "ecs/services", they won't know the parsing failed.

Recommendation: Log the error at WARN level before falling back:

if err != nil {
    log.Warn("invalid startup view, falling back to service browser", "view", viewName, "error", err)
    return view.NewServiceBrowser(a.ctx, a.registry)
}

4. Redundant Code Pattern in command_input.go (internal/view/command_input.go:337-370)

After calling ParseServiceResource(), there's a fallback to manual prefix matching:

service, resourceType, err := c.registry.ParseServiceResource(input)
if err == nil {
    browser := NewResourceBrowserWithType(c.ctx, c.registry, service, resourceType)
    return nil, &NavigateMsg{View: browser}
}

// Fallback: prefix matching for partial input
parts := strings.SplitN(input, "/", 2)
service = parts[0]
// ... more prefix matching logic

Concern: This fallback re-implements parsing logic that partially duplicates ParseServiceResource(). The comment says "prefix matching for partial input" but ParseServiceResource should already handle this via ResolveAlias().

Recommendation: Verify if this fallback is actually needed. If it is, add a comment explaining when and why ParseServiceResource fails but prefix matching succeeds.

---

⚡ Performance Considerations

1. Stack Size Enforcement: The max stack size check happens on every navigation push. With DefaultMaxStackSize = 100, the slice truncation is rare but could cause a small allocation when it happens. This is acceptable for the benefit provided.

2. Style Function Changes: Replacing lipgloss.NewStyle() with ui.NoStyle() and ui.BorderStyle() is good for consistency and readability, but has no performance impact (both create new style instances).

---

🔒 Security Concerns

None identified. This PR deals with UI navigation and configuration parsing - no security-sensitive operations.

---

🧪 Test Coverage

Excellent. The PR adds:

• 95 lines of tests for ParseServiceResource() covering aliases, defaults, invalid input, etc.
• 86 lines of tests for new command behaviors (:clear-history, :dashboard, :pulse)
• Tests verify both success and error cases

Suggestion: Consider adding a test for the max stack size enforcement in app_test.go to verify stack truncation behavior.

---

📝 Code Quality & Best Practices

1. Good: Uses existing error patterns (fmt.Errorf) consistently
2. Good: Follows the project's comment style and formatting
3. Good: New config fields have appropriate defaults and validation
4. Minor: The ClearHistoryMsg struct is empty - this is fine but could have a comment explaining it's a signal-only message

---

💡 Recommendations

High Priority:

1. Add warning log when resolveStartupView() falls back due to invalid config
2. Clarify whether :pulse should clear stack or preserve history (document the decision)
3. Verify the fallback prefix matching in command_input.go:337-370 is necessary

Medium Priority:

4. Add test coverage for stack size enforcement
5. Consider adding user feedback when navigation history is truncated

Low Priority:

6. Document the ClearHistoryMsg struct with a comment
7. Consider adding examples to configuration.md showing startup.view options

---

✅ Verdict

LGTM with minor suggestions. This is a solid PR that improves code organization and adds useful features. The issues identified are minor and don't block merging, but addressing them would improve robustness.

The core functionality is sound, tests are comprehensive, and the architectural changes (especially ParseServiceResource() consolidation) are excellent improvements.

Suggested Action: Address the logging in resolveStartupView() and clarify the stack-clearing semantics, then merge.