Skip to content

tui: Fix URL clicks #2823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dgageot merged 1 commit into from
May 20, 2026
+20 −0
Merged

tui: Fix URL clicks #2823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/tui/components/messages/messages.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,10 @@ func (m *model) handleMouseClick(msg tea.MouseClickMsg) (layout.Model, tea.Cmd)
}
}

if url := m.urlAt(line, col); url != "" {
Copy link
Copy Markdown

@docker-agent docker-agent May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown

@docker-agent docker-agent May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

return m, core.CmdHandler(messages.OpenURLMsg{URL: url})
}

clickCount := m.selection.detectClickType(line, col)

switch clickCount {
Expand Down
16 changes: 16 additions & 0 deletions pkg/tui/components/messages/messages_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,22 @@ func TestViewDoesNotWrapWideLines(t *testing.T) {
}
}

func TestMouseClickOnURLOpensURL(t *testing.T) {
t.Parallel()

m := NewScrollableView(80, 24, &service.SessionState{}).(*model)
m.renderedLines = []string{"visit https://example.com for more"}
m.totalHeight = len(m.renderedLines)
m.renderDirty = false

_, cmd := m.handleMouseClick(tea.MouseClickMsg{X: 10, Y: 0, Button: tea.MouseLeft})
require.NotNil(t, cmd)

msg, ok := cmd().(tuimessages.OpenURLMsg)
require.True(t, ok)
assert.Equal(t, "https://example.com", msg.URL)
}

func TestLoadFromSessionIncludesReasoningContent(t *testing.T) {
t.Parallel()

Expand Down
Loading