feat: copy resource ID/ARN to clipboard (y/Y keys)

[yimsk]

Summary

• Add y key to copy resource ID to clipboard
• Add Y key to copy resource ARN to clipboard (shows warning if no ARN)
• Works in both ResourceBrowser and DetailView

Implementation

• New internal/clipboard package with OSC52 + native clipboard support
• Flash message feedback (2s auto-clear) with success/warning styling
• Follows existing handler patterns (cursor bounds check, dao.UnwrapResource())

Changes

• internal/clipboard/clipboard.go - OSC52 + atotto/clipboard dual-write
• internal/app/app.go - Flash message handling
• internal/view/resource_browser_input.go - y/Y handlers
• internal/view/detail_view.go - y/Y handlers
• Updated README, help_view, StatusLine

Closes #59

[claude]

Code Review for PR #93: Copy Resource ID/ARN to Clipboard

Summary

This PR adds clipboard functionality (y for ID, Y for ARN) with OSC52 and native clipboard support. Overall, this is a well-implemented feature with good test coverage and proper integration with the existing codebase. The implementation follows established patterns and includes thoughtful user feedback.

✅ Strengths

1. Excellent Test Coverage: Comprehensive unit tests for both the clipboard package and view handlers, including edge cases (empty lists, missing ARNs)
2. Good Architecture: Clean separation of concerns with a dedicated internal/clipboard package
3. User Feedback: Flash messages with auto-clear provide clear feedback on copy operations
4. Multi-environment Support: OSC52 with tmux/screen detection plus native clipboard fallback is excellent for TUI apps
5. Consistent Patterns: Follows existing handler patterns with cursor bounds checking and dao.UnwrapResource()
6. Documentation: Good package-level and function-level comments

🔍 Code Quality & Best Practices

Positive

• Proper use of Bubbletea message pattern
• Consistent error handling with debug logging (non-intrusive)
• Extracted flashDuration constant for maintainability
• Unified handler patterns across all key handlers (Pattern A with cursor >= 0 check)

Minor Observations

1. OSC52 Implementation (clipboard.go:40-54): The OSC52 implementation is solid, but writing directly to os.Stdout in a Bubbletea app is unconventional. Bubbletea typically manages all terminal output. However, I acknowledge this is a pragmatic approach for OSC52 sequences and likely the only way to achieve this functionality.

   Impact: Low - Works correctly, but could theoretically interfere with Bubbletea's terminal management in edge cases.

2. Flash Message Styling (app.go:394-398): Good use of theme system with SuccessStyle() and WarningStyle(). The Unicode symbols (✓ and ⚠) are a nice touch.

3. Cursor Bounds Checking (resource_browser_input.go:117,143,161,288,297): The pattern cursor >= 0 && cursor < len(r.filtered) is now consistently applied across all handlers. Great refactoring!

🐛 Potential Bugs / Issues

None identified. The code properly handles edge cases:

• Empty resource lists (returns nil cmd)
• Missing ARNs (shows warning message)
• Resource unwrapping for multi-region/multi-profile contexts

⚡ Performance Considerations

1. Base64 Encoding: base64.StdEncoding.EncodeToString() is called synchronously, but this is fine since resource IDs/ARNs are small strings.
2. Flash Message Timer: tea.Tick(flashDuration, ...) creates a goroutine, but only one at a time per copy operation. No performance concerns.
3. Clipboard Library: The atotto/clipboard package is a lightweight dependency already in go.mod as indirect - good choice.

Verdict: No performance concerns. Operations are lightweight and appropriately scoped.

🔒 Security Concerns

1. ✅ No XSS/Injection: Resource IDs and ARNs are copied as-is without modification - correct approach
2. ✅ No Sensitive Data Leakage: The implementation doesn't log the actual values being copied (only debug logs for errors)
3. ✅ Clipboard Contents: ARNs may contain account IDs, but this is expected behavior - users are intentionally copying this data
4. ✅ OSC52 Escaping: Proper base64 encoding prevents injection attacks via malicious resource IDs

Verdict: No security concerns identified.

🧪 Test Coverage

Clipboard Package

• ✅ Tests for all public functions
• ✅ Tests verify correct message types and values
• ✅ Tests execute commands and validate results

View Tests

• ✅ ResourceBrowser: Copy ID, Copy ARN, No ARN case, Empty list
• ✅ DetailView: Copy ID, Copy ARN, No ARN case
• ✅ Tests verify commands are returned (or nil for edge cases)

Suggestions

1. Integration Test: Consider adding a test that verifies the full flow (key press → CopiedMsg → app flash message → clearFlashMsg). Currently, tests only verify that commands are returned, not the full message flow.

   Example:

   // In resource_browser_test.go or app_test.go
   func TestClipboardFlashMessageFlow(t *testing.T) {
       // Setup browser with resource
       // Press 'y' key
       // Verify CopiedMsg is sent
       // Verify flash message is set in app
       // Fast-forward timer
       // Verify flash message is cleared
   }

   Priority: Low - Current tests are sufficient, but this would increase confidence in the full UX flow.

📋 Suggestions for Improvement

1. Message Type Assertion Safety (Low Priority)

In tests like TestResourceBrowserCopyID (resource_browser_test.go:624-632), you could verify the message type and content more thoroughly:

msg := cmd()
copiedMsg, ok := msg.(clipboard.CopiedMsg)
if !ok {
    t.Fatalf("Expected CopiedMsg, got %T", msg)
}
if copiedMsg.Label != "ID" {
    t.Errorf("Expected Label 'ID', got %q", copiedMsg.Label)
}
if copiedMsg.Value != "i-1234567890abcdef0" {
    t.Errorf("Expected Value 'i-1234567890abcdef0', got %q", copiedMsg.Value)
}

This would catch regressions where the wrong message type is returned.

2. Error Handling Documentation (Very Low Priority)

The Copy() function always returns success even if both clipboard writes fail. This is correct for a TUI app (you don't want to crash on clipboard errors), but it might be worth a comment:

// Copy copies the given value to the clipboard and returns a tea.Cmd that sends a CopiedMsg.
// It writes to both OSC52 (terminal clipboard) and native system clipboard for maximum compatibility.
// Always returns CopiedMsg even if writes fail - clipboard operations are best-effort.
func Copy(label, value string) tea.Cmd {

3. Potential Future Enhancement

Consider adding the copied value to the debug log on success (not just failures):

func Copy(label, value string) tea.Cmd {
    return func() tea.Msg {
        writeOSC52(value)
        if err := clipboard.WriteAll(value); err != nil {
            log.Debug("native clipboard write failed", "error", err)
        } else {
            log.Debug("copied to clipboard", "label", label) // Don't log value for privacy
        }
        return CopiedMsg{Label: label, Value: value}
    }
}

This would help with debugging clipboard issues without exposing sensitive data.

📖 Documentation

✅ README.md updated with keybindings
✅ help_view.go updated with descriptions
✅ Package-level documentation in clipboard.go
✅ Function-level comments for exported functions

Suggestion: Consider adding a note in docs/architecture.md about the clipboard package under the "Directory Structure" section, since this is a new internal package.

🎯 Final Verdict

✅ APPROVE - This is a high-quality implementation that I recommend merging.

Checklist

• ✅ Code quality and best practices: Excellent
• ✅ Potential bugs: None identified
• ✅ Performance: No concerns
• ✅ Security: No concerns
• ✅ Test coverage: Comprehensive
• ✅ Documentation: Complete

Optional Follow-ups (Non-blocking)

1. Add integration test for full clipboard flash message flow
2. Enhance test assertions to verify message content
3. Add debug logging for successful copy operations
4. Update docs/architecture.md with clipboard package

Great work! This feature will be very useful for users working with AWS resources. 🎉