refactor: modal-based selectors with stack support (#62)

[yimsk]

Summary

• Display Help, Region Selector, Profile Selector, Action Menu as modals instead of full-screen views
• Add modal stack for nested modals (e.g., Profile Selector → Profile Detail)
• Extract helper functions to eliminate duplicate message handling

Changes

• modalStack field for nested modal support
• showModal(), popModal(), clearModalState() helpers
• handleNavigate(), handleRegionChanged(), handleProfilesChanged(), popToRefreshableView() to reduce duplication
• Fix: AddRegion instead of SetRegion to preserve existing regions on profile refresh
• Modal width constants centralized in internal/view/modal.go
• Updated docs/architecture.md with modal system documentation

Testing

• 25 tests pass including new modal stack tests
• Lint clean, build success

Closes #62

[claude]

Pull Request Review: Modal-based selectors with stack support (#62)

Overview

This PR refactors the Help, Region Selector, Profile Selector, and Action Menu views from full-screen views to modals with nested modal stack support. The implementation is well-architected and includes comprehensive test coverage.

---

✅ Strengths

1. Code Quality & Architecture

• Excellent refactoring: Helper functions (showModal(), popModal(), handleNavigate(), handleRegionChanged(), handleProfilesChanged(), popToRefreshableView()) effectively eliminate code duplication
• Clean separation of concerns: Modal stack logic is isolated and easy to understand
• Consistent patterns: Modal width constants are centralized in internal/view/modal.go
• Good documentation: Updated docs/architecture.md with clear modal system documentation

2. Test Coverage

• 25 tests pass including new comprehensive modal stack tests
• Excellent test coverage for edge cases:
  • Modal stack push/pop behavior
  • Modal clearing on region/profile changes
  • Multiple escape key variations (q, esc, backspace)
  • State preservation across nested modals
• Good use of test helper (newTestApp()) to reduce duplication

3. User Experience

• Modals preserve underlying view state (no view stack manipulation)
• Nested modals work correctly (Profile Selector → Profile Detail)
• Proper cleanup on context changes (region/profile switches)

---

🔍 Issues & Concerns

1. Critical Bug: Region Persistence ⚠️

Location: internal/app/app.go:303

if msg.region != "" {
    config.Global().AddRegion(msg.region)  // ✅ Correct: preserves existing regions
}

vs. internal/app/app.go:509 in handleRegionChanged():

_, existingProfile := config.File().GetStartup()
config.File().SetStartup(msg.Regions, existingProfile)  // ⚠️ Inconsistency

Issue: The PR description states "Fix: AddRegion instead of SetRegion to preserve existing regions on profile refresh", but in handleRegionChanged(), the code still uses SetStartup() which replaces regions entirely (line 510). This appears inconsistent with the stated goal.

Question: Should handleRegionChanged() preserve existing profile settings similarly to how handleProfilesChanged() preserves existing regions (line 525)?

Current behavior in handleProfilesChanged():

existingRegions := config.Global().Regions()  // Preserve regions
config.File().SetStartup(existingRegions, profile)

Suggested fix for consistency (if this is the intent):

func (a *App) handleRegionChanged(msg navmsg.RegionChangedMsg) (tea.Model, tea.Cmd) {
    log.Info("regions changed", "regions", msg.Regions)
    if config.File().PersistenceEnabled() {
        existingRegions, existingProfile := config.File().GetStartup()
        // Merge new regions with existing ones, or replace? Needs clarification
        config.File().SetStartup(msg.Regions, existingProfile)
        if err := config.File().Save(); err != nil {
            log.Warn("failed to persist config", "error", err)
        }
    }
    return a.popToRefreshableView()
}

Actually, looking more closely, the current implementation may be correct - msg.Regions already contains the full list of selected regions from the RegionSelector, so replacing is appropriate. The AddRegion() fix at line 303 is for a different code path (profileRefreshDoneMsg). This needs clarification - the commit message may be misleading about what was fixed.

2. Minor: Removed Comment

Location: internal/app/app.go:22

The comment // clearErrorMsg is sent to clear transient errors after a timeout was removed. While the type name is self-documenting, retaining comments for message types is generally helpful for documentation purposes.

3. Code Consistency: Profile Refresh Logic

Location: internal/app/app_test.go:372

The test TestProfileRefresh_RapidChangesOnlyLatestHonored was modified to remove a check for skipping ProfileSelector views:

Before (implied by surrounding code):

if _, ok := a.currentView.(*view.ProfileSelector); ok {
    continue
}

After: This check was removed from handleProfilesChanged().

Question: Is this intentional? Should we document why the ProfileSelector check was removed? Was it causing issues with the modal-based approach?

---

🛡️ Security & Performance

Security

✅ No security concerns identified

• No new external inputs
• No credential handling changes
• Proper mutex usage in thread-safe operations

Performance

✅ No performance concerns

• Modal operations are O(1) stack operations
• No new blocking operations introduced
• Existing timeout configurations remain unchanged

---

📋 Suggestions

1. Consider adding modal stack depth limit

While unlikely to be an issue in practice, consider adding a safeguard:

const maxModalStackDepth = 10

func (a *App) showModal(modal *view.Modal) (tea.Model, tea.Cmd) {
    if len(a.modalStack) >= maxModalStackDepth {
        log.Warn("modal stack depth exceeded", "depth", len(a.modalStack))
        a.clearModalState()  // Prevent potential stack overflow from bugs
    }
    if a.modal != nil {
        a.modalStack = append(a.modalStack, a.modal)
    }
    a.modal = modal
    return a, a.modal.SetSize(a.width, a.height)
}

2. Add godoc comments for exported constants

The new modal width constants in internal/view/modal.go:20-25 are exported but lack documentation:

// Modal width constants define the preferred widths for specific modal types.
// These values balance content visibility with screen real estate.
const (
    ModalWidthHelp          = 70  // Help view with key bindings
    ModalWidthRegion        = 45  // Region selector list
    ModalWidthProfile       = 55  // Profile selector list
    ModalWidthProfileDetail = 65  // Detailed profile information
    ModalWidthActionMenu    = 60  // Available resource actions
)

3. Test coverage gap: Modal stack overflow

Consider adding a test for deeply nested modals to ensure graceful handling:

func TestModalStackDeepNesting(t *testing.T) {
    app := newTestApp(t)
    // Push multiple modals and ensure state remains consistent
}

---

📊 Summary

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐⭐	Excellent refactoring with helper functions
Test Coverage	⭐⭐⭐⭐⭐	Comprehensive tests including edge cases
Documentation	⭐⭐⭐⭐	Good architecture docs, minor godoc gaps
Performance	⭐⭐⭐⭐⭐	No concerns
Security	⭐⭐⭐⭐⭐	No concerns

---

✅ Recommendation

APPROVE with minor clarifications requested

This is a well-implemented refactoring that improves code maintainability and UX. The main concern is clarifying the region persistence behavior mentioned in the PR description vs. the actual implementation.

Before merging, please clarify:

1. Is the AddRegion vs SetRegion fix only for the profileRefreshDoneMsg path (line 303)?
2. Is the removal of the ProfileSelector check from handleProfilesChanged() intentional?

Great work overall! The modal stack implementation is clean and well-tested. 🎉

---

Generated by Claude Code PR Review