v0.6.4: Clipboard copy (y/Y keys)

[yimsk]

Summary

• Add y key to copy resource ID to clipboard
• Add Y key to copy resource ARN to clipboard
• OSC52 + native clipboard support (tmux/screen compatible)

Changes

• New internal/clipboard package
• Flash message feedback with auto-clear
• Comprehensive test coverage

Closes #59

[claude]

Code Review - PR #94: Clipboard Copy Feature (y/Y keys)

✅ Overall Assessment

This is a high-quality PR that implements clipboard functionality with excellent attention to detail. The implementation is clean, well-tested, and follows good software engineering practices.

---

🎯 Strengths

1. Excellent Documentation

   • Clear package-level godoc explaining the clipboard package purpose
   • Good inline comments explaining the OSC52 sequence format
   • README.md updated with the new keybindings
   • Help view updated

2. Robust Terminal Support

   • OSC52 escape sequences for SSH/remote terminal sessions
   • Native clipboard via atotto/clipboard for local sessions
   • Proper tmux/screen multiplexer detection and wrapping
   • Dual-write strategy maximizes compatibility across environments

3. Great Test Coverage

   • Comprehensive unit tests for clipboard package (all functions tested)
   • Integration tests for both ResourceBrowser and DetailView
   • Edge cases covered: empty lists, missing ARNs, cursor bounds
   • Tests verify message types and return values

4. User Experience

   • Flash messages with auto-clear provide clear feedback
   • Warning style for "No ARN available" vs success style for copy
   • Consistent keybindings across list and detail views
   • y/Y mnemonic is intuitive (lowercase=ID, uppercase=ARN)

5. Code Quality

   • Clean separation of concerns (clipboard package is independent)
   • Consistent error handling (debug logging, no panics)
   • Cursor bounds checking prevents potential index out of range errors
   • Follows Bubble Tea patterns correctly

---

🔍 Observations & Suggestions

1. Test Coverage Enhancement (Minor)

The tests verify that commands are returned but don't verify the actual message content in some cases:

internal/view/resource_browser_test.go:629-632

msg := cmd()
if msg == nil {
    t.Fatal("Expected message from clipboard command")
}

Suggestion: Verify the actual message type and content:

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 same pattern applies to:

• TestDetailViewCopyID (detail_view_test.go:291-294)
• TestResourceBrowserCopyARN (resource_browser_test.go:650-653)
• TestDetailViewCopyARN (detail_view_test.go:304-307)

2. OSC52 Edge Case (Very Minor)

internal/clipboard/clipboard.go:52

if _, err := os.Stdout.WriteString(seq); err != nil {
    log.Debug("OSC52 clipboard write failed", "error", err)
}

This is fine, but in very rare cases where stdout is closed/redirected, this could fail silently. Current approach is good (debug logging), but you might consider:

• Whether to surface this to the user in the flash message (probably not - it's too technical)
• Whether clipboard success should depend on at least one method succeeding

Current behavior is acceptable - the dual-write approach means if OSC52 fails, native clipboard likely succeeds and vice versa.

3. Empty String Edge Case (Theoretical)

internal/view/resource_browser_input.go:289-290

resource := dao.UnwrapResource(r.filtered[cursor])
return r, clipboard.CopyID(resource.GetID())

If GetID() returns an empty string, it will still copy (and show "Copied ID"). This is likely fine since AWS resources should always have IDs, but you could add validation:

if id := resource.GetID(); id != "" {
    return r, clipboard.CopyID(id)
}
return r, nil // or show error message

This is probably unnecessary given AWS resource constraints, but worth considering if other resource types are added.

4. Message Field Usage

internal/clipboard/clipboard.go:20

Value string // The copied value (retained for future use: logging, undo)

The Value field is currently unused in the app handler. The comment suggests future use for logging/undo, which is great forward-thinking design. Consider:

• Adding a debug log in app.go when handling CopiedMsg
• Or removing the field if there's no near-term plan to use it (YAGNI principle)

Current approach is fine - it's low cost and enables future features.

---

🔒 Security Review

✅ No security concerns identified

• No user input injection risks (resource IDs/ARNs come from AWS SDK)
• OSC52 sequences are properly base64-encoded
• No shell command execution
• No sensitive data exposure (ARNs/IDs are already visible in the UI)
• Debug logging is appropriate (no sensitive data logged)

---

⚡ Performance Review

✅ No performance concerns

• Clipboard operations are async (tea.Cmd pattern)
• Flash message cleanup uses timer, not polling
• Base64 encoding is O(n) on string length (negligible for ARNs)
• No blocking operations on main thread

---

📦 Dependencies

✅ Dependency management is correct

The go.mod changes properly move github.com/atotto/clipboard from indirect to direct dependency and reclassify github.com/mattn/go-runewidth as indirect. This is the correct change.

---

🎨 Code Style

✅ Consistent with codebase patterns

• Follows existing handler pattern (handleMark, handleEnter, etc.)
• Message types follow conventions (CopiedMsg, NoARNMsg)
• Error handling matches project style (debug logging)
• Naming is clear and consistent

---

📝 Minor Nitpicks (Optional)

1. Consistency in handler pattern: The cursor bounds check pattern is now unified across handlers (great!), but you could extract it:

   func (r *ResourceBrowser) getSelectedResource() dao.Resource {
       cursor := r.table.Cursor()
       if len(r.filtered) > 0 && cursor >= 0 && cursor < len(r.filtered) {
           return dao.UnwrapResource(r.filtered[cursor])
       }
       return nil
   }

   This reduces repetition, but current approach is perfectly fine.

2. Magic number: flashDuration = 2 * time.Second is already extracted as a constant - excellent!

---

✨ Recommendations

1. Merge as-is - This PR is production-ready
2. Consider enhancing test assertions to verify message content (low priority)
3. Consider using the CopiedMsg.Value field for debug logging (optional)

---

🏆 Summary

Approve with minor suggestions for future improvement.

This PR demonstrates:

• Strong understanding of the Bubble Tea framework
• Excellent test coverage and edge case handling
• Good user experience design
• Clean, maintainable code

Great work on this feature! 🎉

---

Review Score: 9/10

• Code Quality: ✅ Excellent
• Test Coverage: ✅ Excellent
• Security: ✅ No concerns
• Performance: ✅ No concerns
• Documentation: ✅ Excellent