Skip to content

Open email links from the TUI keyboard - #475

Open
robzolkos wants to merge 7 commits into
mainfrom
rob/issue-445-open-links
Open

robzolkos wants to merge 7 commits into
mainfrom
rob/issue-445-open-links

Conversation

@robzolkos

@robzolkos robzolkos commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add Tab and Shift-Tab navigation for safe links in open mail threads, with wrapping, viewport reveal, selected styling, and the full destination shown before opening
  • open selected HTTP, HTTPS, and mailto destinations through a validated platform launcher without a shell
  • preserve terminal-native OSC 8 links, existing global Tab behavior outside linked thread content, message and attachment keys, and Escape/q behavior
  • document the thread link controls

This is the link-opening half of the issue. In-message search remains separate.

Refs #445

Automated validation

  • go test ./internal/markdown -run '^TestRenderLinked' -count=1
  • go test ./internal/tui -run '^(TestRootModelNavigatesThreadLinksAndOpensExactDestination|TestRootModelKeepsGlobalTabForALinklessThread|TestLinkedThreadUsesGlobalTabOutsideContent|TestEnterWithoutASelectedLinkDoesNotOpen|TestThreadLinkNavigationYieldsToAModal|TestThreadLinkSelectionSurvivesRebuildsAndClearsWhenOccurrenceDisappears|TestThreadMessageAndAttachmentKeysRemainAvailableWithLinks|TestQLeavesAThreadWithASelectedLink|TestOpenURLCommand|TestOpenExternalURLReturnsLauncherStartupFailure)$' -count=1
  • go test ./internal/markdown ./internal/tui
  • make test
  • make lint
  • make build
  • go test ./internal/markdown -run=^$ -fuzz=FuzzContainment -fuzztime=10s
  • git diff --check main...HEAD

All passed. The root-model flow opens a realistic three-message thread from a fake HEY server and verifies navigation, wrapping, destination display, off-screen reveal, exact opener delivery, errors, and Escape behavior.

The TUI suite also passed with the default environment, a dark environment, and NO_COLOR=1.

Real-terminal validation

HEY authentication is available on the validation machine, but interactive terminal validation was blocked:

  • Kitty is not installed and no Kitty session is running.
  • Ghostty is installed at /usr/bin/ghostty, but no Ghostty session is running and the agent session has no TTY.

No browser was opened and no account state was changed. Therefore, mouse opening, live theme switching, and real default-handler opening could not be exercised in Kitty or Ghostty in this environment.


Summary by cubic

Adds keyboard navigation for links in open thread content, so links can be selected and opened without a mouse.

  • Tab and Shift+Tab select the next/previous link (wrapping), with reverse-video styling and viewport scrolling to keep the whole link visible.
  • Selection is tracked per message and occurrence, so it survives resize, restyle, and attachment rebuilds.
  • A stable reserved footer row shows the complete selected destination without changing the thread's viewport height; Enter opens it only when the terminal fits the whole footer.
  • Enter starts the platform launcher (open, xdg-open, or rundll32) without a shell or blocking the TUI; only HTTP, HTTPS, and mailto destinations are accepted.
  • Launcher startup failures surface as a sanitized "Could not open link" notice in the thread.
  • Escape clears the selection first, then exits the thread as before; q still exits directly, and threads with no selectable links keep the existing global Tab focus behavior.
  • Adds RenderLinked in internal/markdown to render links while recording their line ranges; named anchors that Glamour splits into two OSC 8 spans stay one selectable link, and existing OSC 8 mouse links are preserved.

Refs #445.

Written for commit 8f23fa3. Summary will update on new commits.

Review in cubic

@robzolkos
robzolkos requested a review from a team as a code owner September 21, 2026 16:46
Copilot AI balanced review requested due to automatic review settings September 21, 2026 16:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Long or hidden destinations can be opened without displaying their complete target, creating a security-sensitive mismatch with the stated behavior.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds keyboard link navigation and safe URL launching to TUI mail threads.

Changes:

  • Adds wrapped Tab/Shift+Tab link selection, highlighting, and viewport reveal.
  • Opens validated HTTP(S) and mailto URLs through platform launchers.
  • Adds documentation and comprehensive tests.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File Description
internal/​tui/​tui.go Routes link navigation keys and updates help.
internal/​tui/​tui_test.go Tests end-to-end thread link behavior.
internal/​tui/​section_view.go Adds link navigation and URL opener interfaces.
internal/​tui/​open_url.go Validates and launches URLs.
internal/​tui/​open_url_test.go Tests launchers and URL validation.
internal/​tui/​mail.go Implements link selection, rendering, and opening.
internal/​markdown/​safety_test.go Extends containment fuzz coverage.
internal/​markdown/​render_test.go Tests linked rendering behavior.
internal/​markdown/​linked.go Extracts and highlights rendered links.
internal/​markdown/​contain.go Centralizes stricter hyperlink validation.
docs/​tui.md Documents thread link controls.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/tui/mail.go Outdated
@robzolkos

Copy link
Copy Markdown
Collaborator Author

Fixed - complete destinations now wrap across available rows, opening stays disabled until every row is visible, and HTTP(S) userinfo is rejected.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale comment

Not approved: Cursor Security Agent was present but still pending after the wait window, so the required security-review signal did not complete. Human review is needed; no reviewers were assigned (assignment is not available in this run). Bugbot was not present after the first poll and was skipped.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Link notices can hide permanent partial-thread warnings, and each navigation key synchronously rerenders the entire bounded-but-potentially-large thread.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Cache rendered entry links to prevent Tab navigation stalls

internal/​tui/​mail.go:2211

Every Tab/Shift-Tab synchronously rerenders the entire thread through Glamour, and the selected entry is rendered a second time in renderEntriesWithLinks. TUI threads allow up to 2,000 entries and 64 MiB, so link traversal can block the event loop on each keystroke. Cache the per-entry rendered text/link metadata and invalidate it on body, width, or style changes, then update only the previously/currently selected occurrences.

Comment thread internal/tui/mail.go Outdated
@robzolkos

Copy link
Copy Markdown
Collaborator Author

Fixed - link selection now uses a fixed footer without displacing thread notices, and navigation updates only the affected cached entry body instead of rerendering the full thread.

@robzolkos

Copy link
Copy Markdown
Collaborator Author

Flagged for human review - automated approval requested a human security review rather than an in-scope code change.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved: Cursor Security Agent completed successfully with no findings comment (treated as clean), and Bugbot was not present after the first poll so that signal was skipped. No reviewers were assigned (assignment is not available in this run). No approval-policy files applied.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Footer height accounting regresses spacing and shifts the footer across TUI screens.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

Comment thread internal/tui/tui.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Link occurrence detection can split one anchor into two stops, and launcher exit failures are discarded.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Avoid splitting anchors when label ends with target URL

internal/​markdown/​linked.go:68

The suffix heuristic splits one anchor into two selectable links when its label ends with its target, for example [Read https://example.com](https://example.com). Glamour renders the label and visible href as separate OSC 8 pairs; the label prematurely sets currentComplete, so the href starts another LinkOccurrence. Tab then stops twice on one anchor and styles only half each time. Determine completion from the renderer's href phase rather than label text, and add this case as a regression test.

Medium severity Propagate launcher exit errors instead of reporting startup success

internal/​tui/​open_url.go:78

This reports success as soon as the launcher process starts and discards its exit status. Launchers such as xdg-open can start normally and then exit non-zero when no default handler exists, so the TUI never receives the failure and silently leaves the link unopened. Since this function already runs inside a Bubble Tea command, wait for the launcher and return its exit error instead.

@robzolkos

Copy link
Copy Markdown
Collaborator Author

Fixed - link labels that contain their destination now remain one selectable occurrence, and URL launchers now report non-zero exit failures to the TUI.

@robzolkos
robzolkos requested a balanced review from Copilot and removed request for a team September 21, 2026 22:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Linkless threads unnecessarily lose viewport space, and URL launch commands can remain attached until the opened application exits.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reserve the link footer row only when selectable links exist

internal/​tui/​mail.go:1696

LinkFooter reserves this extra row for every open thread, even when v.links is empty. Linkless threads therefore lose one viewport row and render a blank footer despite having no link-selection state; this is an unrelated regression for ordinary thread reading. Only reserve the row when the thread actually has selectable links.

Comment thread internal/tui/open_url.go Outdated
@robzolkos

Copy link
Copy Markdown
Collaborator Author

Fixed - linkless threads now keep the full viewport, while URL launchers return after startup and are reaped asynchronously.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Whitespace-normalized labels can make one anchor appear as two keyboard-selectable links.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Avoid duplicate links for whitespace-normalized matching labels

internal/​markdown/​linked.go:69

This can split one anchor into two selectable occurrences when its label differs from the destination only by whitespace. For example, a label such as https://example. com is emitted as a named link, so Glamour produces a label span and a shown-destination span; withoutWhitespace marks the label span complete, causing the destination span to append a second link instead of merging. Detect Glamour’s generated destination span from its surrounding link rendering (or track anchors before rendering) rather than treating any whitespace-normalized matching label as completion.

@robzolkos

Copy link
Copy Markdown
Collaborator Author

Fixed - rendered destination spans are now identified separately from link labels, including whitespace-normalized labels and wrapped duplicate destinations.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants