tui: Fix URL clicks#2823
Conversation
Commit 7a30d0d added URL opening for terminals where mouse tracking prevents native link handling. That worked by treating a mouse release at the same cell as the initial click as a plain click, then opening any URL at that position. That made link activation depend on the terminal and Bubble Tea delivering a matching MouseReleaseMsg after MouseClickMsg. With the current event stream, plain clicks can reach handleMouseClick without a reliable release event, so the URL detector still found links but the OpenURLMsg path was never reached. Open URLs from the left-click handler after higher-priority message controls have been checked, and cover that event path with a regression test. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
The fix correctly moves URL opening from handleMouseRelease to handleMouseClick so that URLs open reliably without depending on a MouseReleaseMsg. The regression test is well-structured. Two behavioural issues in the changed code are noted inline.
| } | ||
| } | ||
|
|
||
| if url := m.urlAt(line, col); url != "" { |
There was a problem hiding this comment.
[MEDIUM] URL may be opened twice on a plain click (double-open risk)
handleMouseClick now opens the URL and returns at line 333 — before m.selection.start(line, col) is reached. This means selection.startLine/startCol are NOT updated for this click.
In handleMouseRelease (line 434) the existing guard if line == m.selection.startLine && col == m.selection.startCol still triggers a second OpenURLMsg. If the release coordinates happen to match the previous click's startLine/startCol (e.g., the user clicks a URL at the same cell twice in a row, or startLine/startCol initialise to 0 and the URL is in the top-left area), the URL will be opened a second time.
The old code in handleMouseRelease that performs the same urlAt lookup was not removed by this PR; the two paths are now additive rather than mutually exclusive.
Suggestion: Remove (or gate) the urlAt branch inside handleMouseRelease now that URL opening is handled in handleMouseClick, so each click triggers at most one OpenURLMsg.
| } | ||
| } | ||
|
|
||
| if url := m.urlAt(line, col); url != "" { |
There was a problem hiding this comment.
[MEDIUM] Drag-select starting on URL text is no longer possible
Because the urlAt check fires on MouseClickMsg (mouse-button-down) and returns immediately, m.selection.start(line, col) is never called when the pointer is over a URL.
The previous design handled this correctly: handleMouseRelease only opened the URL when release == press (no movement). A drag that began on URL text would have different press/release coordinates, so the URL-open branch was skipped and the selection was committed normally.
With the new approach any press over URL text immediately dispatches OpenURLMsg and returns, making it impossible to begin a drag-select from URL text.
Suggestion: Either defer URL opening to MouseReleaseMsg (keeping the same-cell guard), or check handleMouseClick only when the click is definitively a non-drag (i.e., after a short threshold/on release), so drag-selection over URLs remains functional.
3 participants
Commit 7a30d0d added URL opening for terminals where mouse tracking prevents native link handling. That worked by treating a mouse release at the same cell as the initial click as a plain click, then opening any URL at that position.
That made link activation depend on the terminal and Bubble Tea delivering a matching MouseReleaseMsg after MouseClickMsg. With the current event stream, plain clicks can reach handleMouseClick without a reliable release event, so the URL detector still found links but the OpenURLMsg path was never reached.
Open URLs from the left-click handler after higher-priority message controls have been checked, and cover that event path with a regression test.