feat: async loading UX with spinners

[yimsk]

Summary

• Improve perceived performance by making AWS API calls non-blocking
• Add spinner animations during loading states for visual feedback

Changes

• Startup: STS GetCallerIdentity runs in background; service list shows immediately
• ResourceBrowser: Spinner animation (⠋ Loading...) during list loading
• DetailView: Async refresh for extended details; immediate navigation with background fetch

Closes #7

[claude]

Code Review: Async Loading UX with Spinners

Summary

This PR successfully improves perceived performance by making AWS API calls non-blocking and adding visual loading indicators. The implementation is well-structured with good test coverage.

✅ Strengths

Architecture & Design

• Excellent separation of concerns: AWS initialization moved to background without blocking UI rendering
• Consistent pattern: Spinner usage is uniform across ResourceBrowser, DetailView, and app initialization
• Smart timeout handling: 5-second timeout on AWS init prevents indefinite hangs (internal/app/app.go:21)
• Centralized spinner factory: ui.NewSpinner() provides consistent styling across the app (internal/ui/theme.go:109)
• Non-breaking changes: DetailView API extended with optional DAO parameter, maintaining backward compatibility

Code Quality

• Strong test coverage: Added 38 new tests covering success/error paths for async operations
• Proper error handling: Graceful degradation when AWS init fails (shows warning, app remains functional)
• Clean state management: Boolean flags (awsInitializing, refreshing) clearly track async state
• Helper function extraction: IsEscKey() consolidates escape key detection logic (internal/view/view.go:81-83)

🔍 Issues & Recommendations

1. Race Condition in DetailView Initialization (Medium Priority)

Location: internal/view/detail_view.go:81-86

func (d *DetailView) Init() tea.Cmd {
    if d.dao != nil && d.dao.Supports(dao.OpGet) {
        d.refreshing = true
        return tea.Batch(d.spinner.Tick, d.refreshResource)
    }
    return nil
}

Issue: d.refreshing is set before refreshResource() runs, but there's no guarantee the message will be processed. If refreshResource() executes immediately (synchronous path), there could be a race condition.

Recommendation: This is likely safe in practice since Bubbletea processes messages sequentially, but consider documenting this assumption or ensuring refreshing is only set within the message handler.

2. Spinner Continues After Error (Low Priority)

Location: internal/view/detail_view.go:97-111

case detailRefreshMsg:
    d.refreshing = false
    if msg.err != nil {
        log.Warn("failed to refresh resource details", "error", msg.err)
        d.refreshErr = msg.err
    } else {
        d.refreshErr = nil
        d.resource = msg.resource
        // Re-render content with refreshed data
        if d.ready {
            content := d.renderContent()
            d.viewport.SetContent(content)
        }
    }
    return d, nil

Issue: When refresh fails, spinner stops but no visual indication except "⚠ refresh failed" in status line. Users might miss this.

Recommendation: Consider adding a transient error message or more prominent error indication.

3. Context Timeout Not Propagated Consistently (Medium Priority)

Location: internal/app/app.go:106-110

initAWSCmd := func() tea.Msg {
    ctx, cancel := context.WithTimeout(a.ctx, awsInitTimeout)
    defer cancel()
    err := aws.InitContext(ctx)
    return awsContextReadyMsg{err: err}
}

Good: Timeout added for AWS init.

Issue: The timeout is only applied to AWS initialization. What about resource loading in ResourceBrowser.loadResources() and DetailView.refreshResource()? Long-running API calls could still hang indefinitely.

Recommendation: Consider adding timeouts to other AWS operations or document why they're not needed (e.g., AWS SDK has built-in timeouts).

4. Missing Nil Check in DetailView (Low Priority)

Location: internal/view/detail_view.go:91-96

func (d *DetailView) refreshResource() tea.Msg {
    if d.dao == nil || d.resource == nil {
        return detailRefreshMsg{resource: d.resource}
    }
    refreshed, err := d.dao.Get(d.ctx, d.resource.GetID())
    if err != nil {
        return detailRefreshMsg{resource: d.resource, err: err}
    }
    return detailRefreshMsg{resource: refreshed}
}

Issue: Check for d.resource == nil but still return detailRefreshMsg{resource: d.resource} which could be nil. This might cause issues downstream.

Recommendation: Return an error or handle the nil case more explicitly.

5. Test Coverage Gap (Low Priority)

Location: internal/view/view_test.go:621-638

Observation: Tests cover DAO with/without OpGet support, but don't test:

• What happens when Get() is called but returns partial data
• Concurrent spinner tick messages during refresh
• Behavior when DetailView is navigated away from while refreshing

Recommendation: Add integration tests for these edge cases.

6. Comment Cleanup (Cosmetic)

Location: internal/view/resource_browser.go:537

-// handleNavigation checks if a key matches a navigation shortcut
+// buildTable rebuilds the table with current filtered resources
 func (r *ResourceBrowser) buildTable() {

Issue: Comment update doesn't match function. Looks like incorrect comment replacement.

Recommendation: Remove or correct the comment.

7. Potential Memory Leak (Low Priority)

Location: Multiple spinner.Tick commands

Observation: Spinners start ticking in Init() and continue until loading completes. If loading never completes (hung API call), ticker keeps running.

Recommendation: Ensure ticker stops on all error paths. Current implementation appears correct but worth verifying.

🎯 Performance Considerations

Positive

• ✅ Non-blocking startup significantly improves perceived performance
• ✅ Spinner provides visual feedback preventing user confusion
• ✅ Async refresh in DetailView allows instant navigation
• ✅ ResourceBrowser batches spinner tick with reload commands efficiently

Potential Concerns

• ⚠️ Multiple concurrent spinners (app init + resource loading) could cause excessive redraws
• ⚠️ No debouncing on rapid navigation - could queue many refresh operations

Recommendation: Monitor performance with slow networks or rate-limited AWS accounts.

🔒 Security Considerations

✅ No security issues identified

• AWS credentials handling unchanged
• No new user input validation required
• Timeout prevents potential DoS from hung connections
• Error messages don't leak sensitive information

📋 Test Coverage Assessment

Excellent coverage with 38 new test cases:

✅ app_test.go: Tests AWS init success/failure paths
✅ theme_test.go: Validates spinner initialization
✅ view_test.go: Comprehensive tests for:

• IsEscKey() with various key types
• DetailView refresh success/error handling
• DAO support detection
• Spinner state management

Missing: Integration tests for concurrent operations, but unit test coverage is solid.

🎨 Code Style & Conventions

✅ Follows repository patterns:

• Consistent with existing Bubbletea message patterns
• Proper use of tea.Batch() for multiple commands
• Style caching pattern maintained
• Comment style matches codebase

💡 Suggestions for Future Enhancement

1. Progressive loading: Show cached/stale data immediately while refreshing in background
2. Retry mechanism: Auto-retry failed refreshes with exponential backoff
3. Cancellation: Allow users to cancel long-running operations with Ctrl+C
4. Metrics: Track how often timeouts/failures occur to tune timeout values

✅ Recommendation

APPROVE with minor suggestions

This PR delivers meaningful UX improvements with solid engineering. The identified issues are minor and don't block merging. Consider addressing the race condition comment and context timeout propagation in a follow-up if needed.

Great work on the comprehensive test coverage and maintaining backward compatibility! 🎉

---

Review completed by Claude Code