feat: configurable startup view with navigation improvements

[yimsk]

Summary

Merge feature/117: configurable startup view with navigation improvements

Changes

• 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
• Update documentation (README, configuration.md, keybindings.md)

Test Coverage

• All tests pass
• 95 new test lines for ParseServiceResource()
• 86 new test lines for command behaviors

Breaking Changes

None - backward compatible

🤖 Generated with Claude Code

[claude]

Code Review: PR #119 - Configurable Startup View with Navigation Improvements

Summary

This PR successfully implements configurable startup views with improved navigation. The implementation is well-structured, properly tested, and maintains backward compatibility. Overall, this is a high-quality contribution.

✅ Strengths

Code Quality & Architecture

• Excellent refactoring: The new ParseServiceResource() method in registry.go consolidates duplicate logic from main.go, improving maintainability
• Clean separation of concerns: The resolveStartupView() method in app.go follows the single responsibility principle
• Consistent error handling: Proper fallback to ServiceBrowser when parsing fails
• Good documentation: Clear comments explaining behavior and parameter meanings

Test Coverage

• 95 new test lines for ParseServiceResource() with comprehensive edge cases:
  • Service-only (default resource)
  • Explicit service/resource
  • Alias resolution
  • Invalid inputs (multiple slashes, unknown services)
• 86 new test lines for command behaviors covering new commands (:clear-history, :pulse, :dashboard)
• All tests passing ✅

Backward Compatibility

• No breaking changes
• Configuration is optional (empty string defaults to "services")
• Graceful fallback when invalid startup view is configured

🔍 Issues & Recommendations

1. Navigation Stack Behavior Change (Breaking UX)

Location: internal/view/command_input.go:213-225, 230

The PR changes navigation stack behavior in a way that might confuse users:

// Before (implicit): ClearStack=true for home navigation
// After: ClearStack=false for all navigation
if input == "" || input == "home" {
    browser := NewServiceBrowser(c.ctx, c.registry)
    return nil, &NavigateMsg{View: browser, ClearStack: false}  // ⚠️ Changed
}

if input == "pulse" {
    dashboard := NewDashboardView(c.ctx, c.registry)
    return nil, &NavigateMsg{View: dashboard, ClearStack: false}  // ⚠️ Changed
}

Impact: Users can no longer truly "go home" by clearing their navigation stack. The back button (esc) will still traverse the full history.

Recommendation: Consider these options:

1. Keep :home with ClearStack=true for true home navigation
2. Add a separate command like :reset that clears the stack
3. Document this behavior change in the PR description as it affects UX

2. Max Stack Size Enforcement Logic

Location: internal/app/app.go:684-686

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

Issue: This creates a new slice on every navigation when the stack is full, which causes:

• Unnecessary allocations
• The stack never actually shrinks below maxSize+1

Recommendation: Use a more efficient approach:

if len(a.viewStack) > maxSize {
    copy(a.viewStack, a.viewStack[1:])
    a.viewStack = a.viewStack[:maxSize]
}

Or use a circular buffer/ring structure for better performance.

3. Missing Error Logging

Location: internal/app/app.resolveStartupView(), line 850

service, resourceType, err := a.registry.ParseServiceResource(viewName)
if err != nil {
    // Fallback to ServiceBrowser on error
    return view.NewServiceBrowser(a.ctx, a.registry)
}

Issue: Silent failures make debugging difficult. If a user configures startup.view: "ec2xyz" (typo), they won't know why it didn't work.

Recommendation: Add logging:

if err != nil {
    log.Warn("invalid startup view, using default", "view", viewName, "error", err)
    return view.NewServiceBrowser(a.ctx, a.registry)
}

4. Duplicate Code in Command Input

Location: internal/view/command_input.go:344-369

The fallback prefix matching logic duplicates parts of ParseServiceResource() logic. This could lead to maintenance issues.

Recommendation: Consider moving prefix matching into ParseServiceResource() with a "fuzzy" flag, or create a separate ParseServiceResourceFuzzy() method to reduce duplication.

5. Style Optimization

Location: internal/view/table_style.go:49

func TableBorderStyle() lipgloss.Style {
    return ui.BorderStyle()  // Creates new style on every call
}

Performance: This function is called frequently during rendering. Based on the architecture doc's performance recommendations about style caching, this could cause allocations.

Recommendation: Cache the style as a package-level variable if it doesn't change:

var tableBorderStyle = ui.BorderStyle()

func TableBorderStyle() lipgloss.Style {
    return tableBorderStyle
}

However, verify this doesn't break theme switching first.

🔒 Security

✅ No security concerns identified. The PR:

• Properly validates user input in ParseServiceResource()
• Rejects invalid input (multiple slashes, unknown services)
• No injection vulnerabilities

⚡ Performance

✅ Generally good, with one minor concern noted above (stack slice handling)

Positive impacts:

• Registry method reduces code duplication
• Tests don't show performance regressions

📝 Documentation

✅ Excellent documentation updates:

• Updated README.md with new startup options
• Updated docs/configuration.md with startup.view and navigation.max_stack_size
• Updated docs/keybindings.md with new commands and corrected behaviors
• Updated help view with new commands

🎯 Recommendations Summary

High Priority:

1. Reconsider ClearStack=false behavior - may break user expectations for "home" navigation
2. Add error logging in resolveStartupView() for invalid configurations

Medium Priority:
3. Optimize stack size enforcement to avoid repeated slice allocations
4. Consider refactoring duplicate prefix-matching logic

Low Priority:
5. Consider caching TableBorderStyle() for performance (verify theme switching compatibility)

✨ Conclusion

This is a well-implemented feature with strong test coverage and good architecture. The main concern is the navigation stack behavior change which might impact UX. With the logging improvement, users will have better visibility when configurations fail.

Verdict: Approve with minor recommendations for improvement.

Great work! 🚀

---

Reviewed by Claude Code