Conversation
Signed-off-by: David Gageot <david.gageot@docker.com>
There was a problem hiding this comment.
Review Summary
Found 1 medium severity issue in the changed code. The clipboard operation now runs first but has its error silently ignored, which can lead to false success feedback when the system clipboard write fails.
Note: While the ordering change achieves dual-clipboard support, consider whether the success notification should be conditional on at least one clipboard operation succeeding.
| return tea.Sequence( | ||
| tea.SetClipboard(text), | ||
| func() tea.Msg { | ||
| _ = clipboard.WriteAll(text) |
There was a problem hiding this comment.
Silent error handling creates false success feedback
The clipboard.WriteAll(text) error is explicitly ignored with _. Since this now runs first in tea.Sequence(), if it fails (e.g., pbcopy unavailable, permission denied, no display server), the error is silently dropped. The success notification always shows "Text copied to clipboard" even when the system clipboard write failed.
Impact: Users see a success message but find their clipboard is empty or has stale content when they try to paste.
Suggestion: Consider checking the error and conditionally showing the success notification, or at minimum logging clipboard failures for debugging. Something like:
func() tea.Msg {
if err := clipboard.WriteAll(text); err != nil {
// Log or handle error
}
return nil
}This would allow you to track whether clipboard operations are actually succeeding.
Instead of copying to the clipboard with
pbcopy(and friends) then OSC52, we do it the reverse order.That makes sure that terminals that support OSC52 get a chance to copy emojis and stuff to the clipboard.
Those that don't support will fallback on what was copied by
pbcopy.