From ebad5d78ee5d20f85b075be642dc963138b3b309 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Mon, 21 Sep 2026 12:32:48 -0400 Subject: [PATCH 1/7] Add keyboard navigation for TUI links --- docs/tui.md | 2 + internal/markdown/contain.go | 5 - internal/markdown/linked.go | 151 ++++++++++++++++ internal/markdown/render_test.go | 124 +++++++++++++ internal/markdown/safety_test.go | 17 ++ internal/tui/mail.go | 168 +++++++++++++++++- internal/tui/open_url.go | 78 ++++++++ internal/tui/open_url_test.go | 79 +++++++++ internal/tui/section_view.go | 9 + internal/tui/tui.go | 24 ++- internal/tui/tui_test.go | 293 +++++++++++++++++++++++++++++++ 11 files changed, 936 insertions(+), 14 deletions(-) create mode 100644 internal/markdown/linked.go create mode 100644 internal/tui/open_url.go create mode 100644 internal/tui/open_url_test.go diff --git a/docs/tui.md b/docs/tui.md index 4dec4426..d6dfd941 100644 --- a/docs/tui.md +++ b/docs/tui.md @@ -61,6 +61,8 @@ uppercase belongs to Labels: | Ctrl+A | switch linked account | | Ctrl+V | choose an Imbox cover | +While reading a thread, links can be selected without a mouse. Tab selects the next link in document order and Shift+Tab selects the previous one; both wrap at the ends. The selected destination is shown above the thread, and the viewport moves to show the whole link, including wrapped lines. Press Enter to open it, Escape once to clear the selection, and Escape again to leave the thread. A thread with no selectable links keeps the normal global Tab focus behavior. Existing OSC 8 mouse links remain available. + Most of those keep working while you are reading a thread, the way the web app's topic toolbar stays live: `r`, `f`, `v`, `b`, `u`, `i`, `l`, `a`, `d`, `p` and `t` all act on the thread on screen rather than on the list behind it. Filing a thread leaves it open in diff --git a/internal/markdown/contain.go b/internal/markdown/contain.go index b88c39c9..86fd993d 100644 --- a/internal/markdown/contain.go +++ b/internal/markdown/contain.go @@ -118,11 +118,6 @@ func hyperlinkEnd(s string) (end, terminator int) { return -1, 0 } -func allowedHyperlink(uri string) bool { - lower := strings.ToLower(uri) - return strings.HasPrefix(lower, "http://") || strings.HasPrefix(lower, "https://") || strings.HasPrefix(lower, "mailto:") -} - // stripAll removes every escape sequence and control character, newlines and tabs // excepted, from text that contain could not vouch for. func stripAll(out string) string { diff --git a/internal/markdown/linked.go b/internal/markdown/linked.go new file mode 100644 index 00000000..e432e8d3 --- /dev/null +++ b/internal/markdown/linked.go @@ -0,0 +1,151 @@ +package markdown + +import ( + "net/url" + "strings" + + "github.com/charmbracelet/x/ansi" + + "github.com/basecamp/hey-cli/internal/htmlutil" +) + +// LinkOccurrence describes one selectable hyperlink in rendered document order. +// Lines are zero-based and inclusive. A wrapped link therefore has different +// StartLine and EndLine values. +type LinkOccurrence struct { + Destination string + StartLine int + EndLine int +} + +// LinkedRender is the contained terminal rendering of one Markdown body and +// the safe hyperlink occurrences it contains. +type LinkedRender struct { + Text string + Links []LinkOccurrence +} + +// RenderLinked renders one sealed Markdown body and records each selectable +// hyperlink in the order it appears. selected is a zero-based occurrence index; +// a negative value leaves all links unselected. +func RenderLinked(md htmlutil.Markdown, width, selected int) LinkedRender { + out := render(md.String(), width) + return linkedRender(out, selected) +} + +func linkedRender(out string, selected int) LinkedRender { + var b strings.Builder + b.Grow(len(out)) + links := make([]LinkOccurrence, 0) + line := 0 + currentDestination := "" + currentVisible := "" + currentComplete := false + for i := 0; i < len(out); { + if strings.HasPrefix(out[i:], "\x1b]8;") { + openEnd, _, ok := hyperlink(out[i:]) + if ok { + _, destination, found := hyperlinkDestination(openEnd) + if found && destination != "" { + contentStart := i + len(openEnd) + closeRel := strings.Index(out[contentStart:], "\x1b]8;") + if closeRel >= 0 { + closeStart := contentStart + closeRel + closeEnd, _, closeOK := hyperlink(out[closeStart:]) + if closeOK { + if allowedHyperlink(destination) { + startLine := line + endLine := line + strings.Count(out[contentStart:closeStart], "\n") + content := out[contentStart:closeStart] + if destination != currentDestination || currentComplete { + links = append(links, LinkOccurrence{Destination: destination, StartLine: startLine, EndLine: endLine}) + currentDestination = destination + currentVisible = "" + } else { + links[len(links)-1].EndLine = endLine + } + currentVisible += withoutWhitespace(ansi.Strip(content)) + currentComplete = strings.HasSuffix(currentVisible, withoutWhitespace(destination)) + + b.WriteString(openEnd) + if len(links)-1 == selected { + b.WriteString("\x1b[7m") + b.WriteString(selectedContent(content)) + b.WriteString("\x1b[27m") + } else { + b.WriteString(content) + } + b.WriteString(closeEnd) + line = endLine + i = closeStart + len(closeEnd) + continue + } + } + } + } + } + } + if out[i] == '\n' { + line++ + } + b.WriteByte(out[i]) + i++ + } + return LinkedRender{Text: contain(b.String()), Links: links} +} + +func hyperlinkDestination(sequence string) (params, destination string, found bool) { + body := sequence[len("\x1b]8;"):] + body = strings.TrimSuffix(body, "\a") + body = strings.TrimSuffix(body, "\x1b\\") + params, destination, found = strings.Cut(body, ";") + return params, destination, found +} + +func allowedHyperlink(uri string) bool { + if strings.ContainsFunc(uri, isControl) { + return false + } + parsed, err := url.Parse(uri) + if err != nil || parsed.Scheme == "" { + return false + } + switch strings.ToLower(parsed.Scheme) { + case "http", "https": + return parsed.Host != "" && parsed.Hostname() != "" + case "mailto": + return parsed.Opaque != "" + default: + return false + } +} + +func withoutWhitespace(s string) string { + return strings.Map(func(r rune) rune { + if r == ' ' || r == '\t' || r == '\n' || r == '\r' { + return -1 + } + return r + }, s) +} + +func selectedContent(content string) string { + var b strings.Builder + b.Grow(len(content) + 16) + for i := 0; i < len(content); { + if content[i] == '\x1b' && strings.HasPrefix(content[i:], "\x1b[") { + sequence, _, ok := sgr(content[i:]) + if ok { + b.WriteString(sequence) + // A nested reset can turn reverse video off. Reapply it after + // every SGR while the selected label is still visible. + b.WriteString("\x1b[7m") + i += len(sequence) + continue + } + } + b.WriteByte(content[i]) + i++ + } + return b.String() +} diff --git a/internal/markdown/render_test.go b/internal/markdown/render_test.go index 4924716a..cad28aba 100644 --- a/internal/markdown/render_test.go +++ b/internal/markdown/render_test.go @@ -3,6 +3,8 @@ package markdown import ( "strings" "testing" + + "github.com/basecamp/hey-cli/internal/htmlutil" ) func TestRenderEmpty(t *testing.T) { @@ -76,3 +78,125 @@ func TestRenderFallsBackToDefaultWidth(t *testing.T) { t.Error("Render with no width returned nothing") } } + +func TestRenderLinkedReturnsOneOccurrence(t *testing.T) { + linked := RenderLinked(htmlutil.ToMarkdown(`

Read the report.

`), 80, -1) + if len(linked.Links) != 1 { + t.Fatalf("RenderLinked returned %d links, want 1: %#v", len(linked.Links), linked.Links) + } + if linked.Links[0].Destination != "https://example.com/report" { + t.Errorf("destination = %q", linked.Links[0].Destination) + } + if linked.Links[0].StartLine != 0 || linked.Links[0].EndLine != 0 { + t.Errorf("line range = %d-%d, want 0-0", linked.Links[0].StartLine, linked.Links[0].EndLine) + } + if !strings.Contains(linked.Text, "\x1b]8;") { + t.Errorf("linked text = %q, want OSC 8", linked.Text) + } +} + +func TestRenderLinkedPreservesOrderDuplicatesAndSchemes(t *testing.T) { + linked := RenderLinked(htmlutil.ToMarkdown(`

one again mail https://example.org/two

`), 80, -1) + want := []string{"https://example.com/one", "https://example.com/one", "mailto:reader@example.com", "https://example.org/two"} + if len(linked.Links) != len(want) { + t.Fatalf("links = %#v, want %d occurrences", linked.Links, len(want)) + } + for i, destination := range want { + if linked.Links[i].Destination != destination { + t.Errorf("links[%d].Destination = %q, want %q", i, linked.Links[i].Destination, destination) + } + } +} + +func TestRenderLinkedRejectsUnsafeAndRelativeDestinations(t *testing.T) { + linked := RenderLinked(htmlutil.ToMarkdown(`

relative ftp script bad

`), 80, -1) + if len(linked.Links) != 0 { + t.Fatalf("links = %#v, want no selectable links", linked.Links) + } +} + +func TestRenderLinkedStylesOnlySelectedOccurrence(t *testing.T) { + linked := RenderLinked(htmlutil.ToMarkdown(`

one two

`), 80, 1) + if len(linked.Links) != 2 { + t.Fatalf("links = %#v, want 2 occurrences", linked.Links) + } + if strings.Count(linked.Text, "\x1b[7m") < 2 || strings.Count(linked.Text, "\x1b[27m") == 0 { + t.Errorf("selected styling missing from %q", linked.Text) + } + if !sgrOnly(linked.Text) { + t.Errorf("selected output escaped containment: %q", linked.Text) + } +} + +func TestRenderLinkedReportsWrappedLinkRange(t *testing.T) { + linked := RenderLinked(htmlutil.ToMarkdown(`

this is a link label that must wrap over several lines

`), 12, -1) + if len(linked.Links) != 1 { + t.Fatalf("links = %#v, want one occurrence", linked.Links) + } + if linked.Links[0].EndLine <= linked.Links[0].StartLine { + t.Errorf("line range = %d-%d, want a wrapped range", linked.Links[0].StartLine, linked.Links[0].EndLine) + } +} + +func TestRenderLinkedKeepsDuplicateLinksSeparate(t *testing.T) { + for name, source := range map[string]string{ + "labels": `

oneagain

`, + "autolinks": `

https://example.com/one https://example.com/one

`, + "line break": `

https://example.com/one
https://example.com/one

`, + } { + t.Run(name, func(t *testing.T) { + linked := RenderLinked(htmlutil.ToMarkdown(source), 80, -1) + if len(linked.Links) != 2 { + t.Fatalf("links = %#v, want two occurrences", linked.Links) + } + }) + } +} + +func TestRenderLinkedSelectedNestedLabelStaysSelected(t *testing.T) { + linked := RenderLinked(htmlutil.ToMarkdown(`

bold and italic

`), 80, 0) + if len(linked.Links) != 1 { + t.Fatalf("links = %#v, want one occurrence", linked.Links) + } + if !strings.Contains(linked.Text, "\x1b[7m\x1b[94;1m\x1b[7mbold") || !strings.Contains(linked.Text, "\x1b[7m\x1b[94;3m\x1b[7mitalic") || !strings.Contains(linked.Text, "\x1b[m\x1b[7m") { + t.Errorf("nested SGR styling lost selection: %q", linked.Text) + } + if !sgrOnly(linked.Text) { + t.Errorf("selected output escaped containment: %q", linked.Text) + } +} + +func TestRenderLinkedSelectedWrappedLabelStaysSelected(t *testing.T) { + linked := RenderLinked(htmlutil.ToMarkdown(`

this is a link label that must wrap over several lines

`), 12, 0) + if len(linked.Links) != 1 { + t.Fatalf("links = %#v, want one occurrence", linked.Links) + } + if strings.Count(linked.Text, "\x1b[7m") < 6 { + t.Errorf("selected wrapped label lost selection: %q", linked.Text) + } + if !sgrOnly(linked.Text) { + t.Errorf("selected output escaped containment: %q", linked.Text) + } +} + +func TestRenderLinkedSanitizesLinkTextAndDestination(t *testing.T) { + linked := RenderLinked(htmlutil.ToMarkdown("

shown\u200b\x01\u202ename

"), 80, -1) + if len(linked.Links) != 1 || linked.Links[0].Destination != "https://example.com/ab" { + t.Fatalf("links = %#v, want one sanitized link", linked.Links) + } + if strings.Contains(linked.Text, "\u200b") || strings.Contains(linked.Text, "\u202e") { + t.Errorf("sanitized link output = %q", linked.Text) + } +} + +func TestRenderLinkedRejectsMalformedDestinationAndHandlesNoLinks(t *testing.T) { + for _, source := range []string{ + `

malformed

`, + `

malformed mail address

`, + `

There is no link here.

`, + } { + if linked := RenderLinked(htmlutil.ToMarkdown(source), 80, -1); len(linked.Links) != 0 { + t.Errorf("source %q produced links %#v", source, linked.Links) + } + } +} diff --git a/internal/markdown/safety_test.go b/internal/markdown/safety_test.go index b3ca492a..adff5413 100644 --- a/internal/markdown/safety_test.go +++ b/internal/markdown/safety_test.go @@ -8,6 +8,8 @@ import ( "charm.land/glamour/v2" "github.com/charmbracelet/x/ansi" + + "github.com/basecamp/hey-cli/internal/htmlutil" ) // visible is what the terminal shows: the output with every escape sequence removed. @@ -321,6 +323,21 @@ func FuzzContainment(f *testing.F) { t.Skip() } out := render(md, 40) + linked := RenderLinked(htmlutil.ToMarkdown(md), 40, len(md)%8) + if !sgrOnly(linked.Text) { + t.Fatalf("RenderLinked(%q) = %q carries a sequence outside the allow-list", md, linked.Text) + } + for _, link := range linked.Links { + if !allowedHyperlink(link.Destination) { + t.Fatalf("RenderLinked(%q) returned unsafe destination %q", md, link.Destination) + } + if link.StartLine < 0 || link.EndLine < link.StartLine { + t.Fatalf("RenderLinked(%q) returned invalid range %#v", md, link) + } + if !strings.Contains(linked.Text, ";"+link.Destination+"\a") && !strings.Contains(linked.Text, ";"+link.Destination+"\x1b\\") { + t.Fatalf("RenderLinked(%q) returned destination %q without a matching OSC 8 occurrence", md, link.Destination) + } + } if !sgrOnly(out) { t.Fatalf("render(%q) = %q carries a sequence outside the allow-list", md, out) } diff --git a/internal/tui/mail.go b/internal/tui/mail.go index bcac3342..d55fbb50 100644 --- a/internal/tui/mail.go +++ b/internal/tui/mail.go @@ -175,6 +175,11 @@ type attachmentSavedMsg struct { err error } +type linkOpenedMsg struct { + topicID int64 + err error +} + type attachmentOpenedMsg struct { topicID int64 attachmentID string @@ -255,6 +260,13 @@ type collectionActionDoneMsg struct { // --- Mail section view --- +type mailLink struct { + destination string + startLine int + endLine int + key string +} + type mailView struct { vc *viewContext @@ -275,6 +287,9 @@ type mailView struct { attachmentCursor int imageContent string entryOffsets []int // line where each message starts in the thread content + links []mailLink + selectedLink int + selectedLinkKey string inThread bool threadNotice string // what the open thread's read did not get; stays until the thread is left contentHeight int // the rows the section has, which the thread's notices and viewport share @@ -321,6 +336,7 @@ func newMailView(vc *viewContext) *mailView { searchList: contentList{hideSeenState: true}, bundleList: contentList{hideSeenState: true}, seenList: contentList{hideSeenState: true}, + selectedLink: -1, } if vc.loadCover != nil { view.cover = parseCoverPreset(vc.loadCover()) @@ -544,6 +560,8 @@ func (v *mailView) Update(msg tea.Msg) (tea.Cmd, bool) { v.attachments = msg.attachments v.attachmentCursor = 0 v.threadNotice = msg.notice + v.selectedLink = -1 + v.selectedLinkKey = "" v.fitThreadViewport() var imageContent strings.Builder var uploadCmds []tea.Cmd @@ -699,6 +717,17 @@ func (v *mailView) Update(msg tea.Msg) (tea.Cmd, bool) { } return notify("Saved attachment to " + msg.path), true + case linkOpenedMsg: + if msg.topicID != v.topicID || !v.inThread { + return nil, true + } + if msg.err != nil { + v.notice = terminal.SanitizeLine("Could not open link: " + msg.err.Error()) + v.fitThreadViewport() + v.revealLink() + } + return nil, true + case attachmentOpenedMsg: if !v.currentAttachmentAction(msg.topicID, msg.attachmentID) { return nil, true @@ -993,6 +1022,12 @@ func (v *mailView) HelpBindings() []helpBinding { helpBinding{"t", "trash"}, ) } + if v.ClaimsLinkNavigation() { + bindings = append(bindings, helpBinding{"tab/shift+tab", "next/previous link"}) + if v.LinkSelectionActive() { + bindings = append(bindings, helpBinding{"enter", "open link"}) + } + } if len(v.entries) > 1 { bindings = append(bindings, helpBinding{"j/k", "next/previous message"}) } @@ -1293,6 +1328,10 @@ func (v *mailView) HandleContentKey(msg tea.KeyPressMsg) tea.Cmd { } if v.inThread { + if cmd, handled := v.handleLinkKey(msg); handled { + return cmd + } + switch msg.String() { case "r", "R": if v.topicID != 0 { @@ -1464,7 +1503,19 @@ func (v *mailView) InThread() bool { return v.inThread || v.searchActive || v.bundleActive || v.seenActive } +func (v *mailView) ClaimsLinkNavigation() bool { + return v.inThread && v.modal == nil && len(v.links) > 0 +} + +func (v *mailView) LinkSelectionActive() bool { + return v.ClaimsLinkNavigation() && v.selectedLink >= 0 && v.selectedLink < len(v.links) +} + func (v *mailView) ExitDetail(key string) { + if v.inThread && key != "q" && v.selectedLink >= 0 { + v.clearLinkSelection() + return + } if key == "q" && (v.searchActive || v.bundleActive || v.seenActive) && !v.inThread && (v.requests.kind == mailRequestTopic || v.requests.kind == mailRequestSearch) { v.requests.cancel() v.clearSearch() @@ -1485,6 +1536,8 @@ func (v *mailView) ExitThread() { v.threadNotice = "" v.threadPosting = mail.Posting{} v.threadBoxKind = "" + v.clearLinkSelection() + v.links = nil v.modal = nil v.requests.cancel() return @@ -1585,6 +1638,8 @@ func (v *mailView) Restyle() { offset := v.topicViewport.YOffset() v.rebuildTopicContent() v.topicViewport.SetYOffset(offset) + v.fitThreadViewport() + v.revealLink() } if v.modal != nil { v.modal.restyle(v.vc.styles) @@ -1599,9 +1654,14 @@ func (v *mailView) Resize(width, height int) { v.searchList.setSize(width, height) v.bundleList.setSize(width, height) v.seenList.setSize(width, height) + oldWidth := v.topicViewport.Width() v.topicViewport.SetWidth(width) v.contentHeight = height + if v.inThread && oldWidth != width { + v.rebuildTopicContent() + } v.fitThreadViewport() + v.revealLink() } // threadNotices is what is shown above an open thread's viewport: the partial-read @@ -1611,7 +1671,11 @@ func (v *mailView) Resize(width, height int) { // way rather than pushing the viewport out. func (v *mailView) threadNotices() []string { var notices []string - for _, notice := range []string{v.threadNotice, v.notice} { + linkNotice := "" + if v.LinkSelectionActive() { + linkNotice = "Open: " + terminal.SanitizeLine(v.links[v.selectedLink].destination) + } + for _, notice := range []string{linkNotice, v.threadNotice, v.notice} { if notice != "" { notices = append(notices, truncateToWidth(notice, max(v.vc.width, 4))) } @@ -1671,6 +1735,8 @@ func (v *mailView) switchBox(index int) tea.Cmd { v.threadNotice = "" v.threadPosting = mail.Posting{} v.threadBoxKind = "" + v.clearLinkSelection() + v.links = nil v.clearSearch() v.clearBundle() v.clearSeen() @@ -1694,6 +1760,8 @@ func (v *mailView) openPreviouslySeen() tea.Cmd { v.threadNotice = "" v.threadPosting = mail.Posting{} v.threadBoxKind = "" + v.clearLinkSelection() + v.links = nil v.clearSearch() v.clearBundle() v.notice = "" @@ -2088,10 +2156,84 @@ func (v *mailView) jumpEntry(delta int) { v.topicViewport.GotoTop() } +func (v *mailView) clearLinkSelection() { + selected := v.selectedLink >= 0 || v.selectedLinkKey != "" + v.selectedLink = -1 + v.selectedLinkKey = "" + if selected && v.inThread { + offset := v.topicViewport.YOffset() + v.rebuildTopicContent() + v.topicViewport.SetYOffset(offset) + v.fitThreadViewport() + } +} + +func (v *mailView) handleLinkKey(msg tea.KeyPressMsg) (tea.Cmd, bool) { + if !v.inThread || len(v.links) == 0 { + return nil, false + } + if msg.Key().Code == tea.KeyTab { + delta := 1 + if msg.Key().Mod == tea.ModShift { + delta = -1 + } + if v.selectedLink < 0 { + if delta < 0 { + v.selectedLink = len(v.links) + } else { + v.selectedLink = -1 + } + } + v.selectedLink = (v.selectedLink + delta + len(v.links)) % len(v.links) + v.selectedLinkKey = v.links[v.selectedLink].key + v.rebuildTopicContent() + v.fitThreadViewport() + v.revealLink() + return nil, true + } + if msg.Key().Code == tea.KeyEnter && v.selectedLink >= 0 { + link := v.links[v.selectedLink] + if v.vc.openURL == nil { + return nil, true + } + topicID, destination := v.topicID, link.destination + return func() tea.Msg { + return linkOpenedMsg{topicID: topicID, err: v.vc.openURL(destination)} + }, true + } + return nil, false +} + +func (v *mailView) revealLink() { + if v.selectedLink < 0 || v.selectedLink >= len(v.links) { + return + } + link := v.links[v.selectedLink] + height := max(v.topicViewport.Height(), 1) + if link.startLine < v.topicViewport.YOffset() { + v.topicViewport.SetYOffset(link.startLine) + } + if link.endLine >= v.topicViewport.YOffset()+height { + v.topicViewport.SetYOffset(link.endLine - height + 1) + } +} + func (v *mailView) rebuildTopicContent() { - rendered, offsets := v.renderEntries(v.entries) + key := v.selectedLinkKey + rendered, offsets, links := v.renderEntriesWithLinks(v.entries, key) v.topicContent = rendered + v.imageContent v.entryOffsets = offsets + v.links = links + v.selectedLink = -1 + for i := range links { + if links[i].key == key && key != "" { + v.selectedLink = i + break + } + } + if v.selectedLink < 0 { + v.selectedLinkKey = "" + } v.topicViewport.SetContent(v.topicContent) } @@ -3054,9 +3196,15 @@ func (v *mailView) fetchTopic(ctx context.Context, requestID uint64, boxID, topi // renderEntries renders the thread's messages and returns the content along // with the line each message header starts on, for j/k jumps. func (v *mailView) renderEntries(entries []mail.Entry) (string, []int) { + rendered, offsets, _ := v.renderEntriesWithLinks(entries, "") + return rendered, offsets +} + +func (v *mailView) renderEntriesWithLinks(entries []mail.Entry, selectedKey string) (string, []int, []mailLink) { var b strings.Builder offsets := make([]int, 0, len(entries)) lineCount := 0 + links := []mailLink{} sepWidth := max(v.vc.width-4, 40) sep := v.vc.styles.separator.Render(strings.Repeat("─", sepWidth)) @@ -3094,7 +3242,19 @@ func (v *mailView) renderEntries(entries []mail.Entry) (string, []int) { fmt.Fprintf(&b, "%s %s\n", v.vc.styles.entryFrom.Render(terminal.SanitizeLine(from)), v.vc.styles.entryDate.Render(formatDisplayDateTime(e.CreatedAt))) switch { case !e.Body.IsEmpty(): - fmt.Fprintf(&b, "\n%s\n", v.vc.styles.entryBody.Render(markdown.Render(e.Body, sepWidth))) + linked := markdown.RenderLinked(e.Body, sepWidth, -1) + localSelected := -1 + for i, occurrence := range linked.Links { + key := fmt.Sprintf("%d\x00%d\x00%s", e.ID, i, occurrence.Destination) + links = append(links, mailLink{destination: occurrence.Destination, startLine: lineCount + 2 + occurrence.StartLine, endLine: lineCount + 2 + occurrence.EndLine, key: key}) + if key == selectedKey { + localSelected = i + } + } + if localSelected >= 0 { + linked = markdown.RenderLinked(e.Body, sepWidth, localSelected) + } + fmt.Fprintf(&b, "\n%s\n", v.vc.styles.entryBody.Render(linked.Text)) case e.BodyState == string(threadload.StateHydrated): fmt.Fprintf(&b, "\n%s\n", v.vc.styles.entryDate.Render("(empty body)")) case e.BodyState == string(threadload.StateBodyless) && e.Summary != "": @@ -3112,5 +3272,5 @@ func (v *mailView) renderEntries(entries []mail.Entry) (string, []int) { lineCount += strings.Count(b.String()[entryStart:], "\n") } - return b.String(), offsets + return b.String(), offsets, links } diff --git a/internal/tui/open_url.go b/internal/tui/open_url.go new file mode 100644 index 00000000..4712aca6 --- /dev/null +++ b/internal/tui/open_url.go @@ -0,0 +1,78 @@ +package tui + +import ( + "context" + "fmt" + "net/url" + "os/exec" + "runtime" + "strings" + "unicode" +) + +type urlCommandStarter func(string, ...string) error + +func openExternalURL(destination string) error { + return openURLWith(runtime.GOOS, destination, startURLCommand) +} + +func openURLWith(goos, destination string, start urlCommandStarter) error { + name, args, err := openURLCommand(goos, destination) + if err != nil { + return err + } + return start(name, args...) +} + +func openURLCommand(goos, destination string) (string, []string, error) { + if err := validateURLDestination(destination); err != nil { + return "", nil, err + } + + switch goos { + case "darwin": + return "open", []string{destination}, nil + case "linux": + return "xdg-open", []string{destination}, nil + case "windows": + return "rundll32", []string{"url.dll,FileProtocolHandler", destination}, nil + default: + return "", nil, fmt.Errorf("opening URLs is not supported on %s", goos) + } +} + +func validateURLDestination(destination string) error { + if destination == "" || strings.IndexFunc(destination, func(r rune) bool { + return unicode.IsControl(r) + }) >= 0 { + return fmt.Errorf("invalid URL destination") + } + + parsed, err := url.Parse(destination) + if err != nil { + return fmt.Errorf("invalid URL destination: %w", err) + } + + switch strings.ToLower(parsed.Scheme) { + case "http", "https": + if !parsed.IsAbs() || parsed.Host == "" || parsed.Hostname() == "" { + return fmt.Errorf("invalid URL destination") + } + case "mailto": + if parsed.Opaque == "" { + return fmt.Errorf("invalid URL destination") + } + default: + return fmt.Errorf("invalid URL destination") + } + return nil +} + +func startURLCommand(name string, args ...string) error { + command := exec.CommandContext(context.Background(), name, args...) // #nosec G204 -- fixed OS launcher receives the validated destination as one argument + if err := command.Start(); err != nil { + return err + } + go func() { _ = command.Wait() }() + return nil +} diff --git a/internal/tui/open_url_test.go b/internal/tui/open_url_test.go new file mode 100644 index 00000000..5bc0e63b --- /dev/null +++ b/internal/tui/open_url_test.go @@ -0,0 +1,79 @@ +package tui + +import ( + "errors" + "testing" +) + +func TestOpenURLCommand(t *testing.T) { + tests := []struct { + name string + goos string + want string + args []string + }{ + {name: "darwin", goos: "darwin", want: "open", args: []string{"https://example.com/a?x=1&y=2"}}, + {name: "linux", goos: "linux", want: "xdg-open", args: []string{"mailto:alex@example.com?subject=Hello"}}, + {name: "windows", goos: "windows", want: "rundll32", args: []string{"url.dll,FileProtocolHandler", "https://example.com/a"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, args, err := openURLCommand(tt.goos, tt.args[len(tt.args)-1]) + if err != nil { + t.Fatalf("openURLCommand() error = %v", err) + } + if got != tt.want { + t.Errorf("executable = %q, want %q", got, tt.want) + } + if len(args) != len(tt.args) { + t.Fatalf("args = %#v, want %#v", args, tt.args) + } + for i := range args { + if args[i] != tt.args[i] { + t.Errorf("args[%d] = %q, want %q", i, args[i], tt.args[i]) + } + } + }) + } +} + +func TestOpenExternalURLReturnsLauncherStartupFailure(t *testing.T) { + startupErr := errors.New("launcher unavailable") + var executable string + var arguments []string + err := openURLWith("linux", "https://example.com/report", func(name string, args ...string) error { + executable = name + arguments = append(arguments, args...) + return startupErr + }) + if !errors.Is(err, startupErr) { + t.Fatalf("openURLWith error = %v, want startup failure", err) + } + if executable != "xdg-open" || len(arguments) != 1 || arguments[0] != "https://example.com/report" { + t.Errorf("launcher = %q %#v", executable, arguments) + } +} + +func TestOpenURLCommandValidation(t *testing.T) { + valid := []string{"http://example.com", "https://example.com/path", "mailto:alex@example.com"} + invalid := []string{"", "http:/example.com", "http:///path", "http://", "https://?x", "ftp://example.com", "file:///tmp/a", "mailto:", "mailto://", "https://example.com/line\nbreak", "https://example.com/\u0085", "https://example.com/%zz"} + + for _, goos := range []string{"darwin", "linux", "windows", "freebsd", ""} { + t.Run(goos, func(t *testing.T) { + for _, destination := range valid { + if _, _, err := openURLCommand(goos, destination); goos == "freebsd" || goos == "" { + if err == nil { + t.Errorf("openURLCommand(%q, %q) accepted unsupported OS", goos, destination) + } + } else if err != nil { + t.Errorf("openURLCommand(%q, %q) error = %v", goos, destination, err) + } + } + for _, destination := range invalid { + if _, _, err := openURLCommand(goos, destination); err == nil { + t.Errorf("openURLCommand(%q, %q) accepted invalid destination", goos, destination) + } + } + }) + } +} diff --git a/internal/tui/section_view.go b/internal/tui/section_view.go index 5816b7a7..0659dca7 100644 --- a/internal/tui/section_view.go +++ b/internal/tui/section_view.go @@ -11,6 +11,7 @@ import ( // viewContext holds shared dependencies injected into every sectionView. type attachmentSaveFunc func(context.Context, string, string, bool) (int64, error) type attachmentOpenFunc func(string) error +type urlOpenFunc func(string) error // The Imbox's cover crosses the seam as a preset name rather than a coverPreset, // so that what stores it does not have to know how covers are drawn. @@ -26,6 +27,7 @@ type viewContext struct { imageFetcher imageFetcher saveAttachment attachmentSaveFunc openAttachment attachmentOpenFunc + openURL urlOpenFunc newAttachmentTempDir func() (string, error) loadCover coverLoadFunc saveCover coverSaveFunc @@ -85,6 +87,13 @@ type inputCapturer interface { CapturingInput() bool } +// linkNavigator is an optional content-row capability. It keeps link-specific +// keyboard routing out of sectionView, whose other implementations do not need it. +type linkNavigator interface { + ClaimsLinkNavigation() bool + LinkSelectionActive() bool +} + type accountSwitchBlocker interface { AccountSwitchBlocked() bool } diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 0d3dd955..6769f324 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -194,6 +194,7 @@ func newViewContext(ctx context.Context, rootSDK, sdk *hey.Client, styles styles return attachmentfiles.Save(ctx, sdk, destination, sourceURL, force) }, openAttachment: openExternalFile, + openURL: openExternalURL, newAttachmentTempDir: func() (string, error) { return os.MkdirTemp("", "hey-cli-attachment-*") }, @@ -720,11 +721,16 @@ func (m *model) updateHelpBindings() { bindings = append(m.activeView.HelpBindings(), quitHint) } else if m.activeView.InThread() { extra := m.activeView.HelpBindings() - bindings = make([]helpBinding, 0, 3+len(extra)) - bindings = append(bindings, - helpBinding{"↑↓", "scroll"}, - helpBinding{"esc/q", "back"}, - ) + bindings = make([]helpBinding, 0, 4+len(extra)) + bindings = append(bindings, helpBinding{"↑↓", "scroll"}) + if navigator, ok := m.activeView.(linkNavigator); ok && navigator.LinkSelectionActive() { + bindings = append(bindings, + helpBinding{"esc", "clear link"}, + helpBinding{"q", "back"}, + ) + } else { + bindings = append(bindings, helpBinding{"esc/q", "back"}) + } bindings = append(bindings, extra...) bindings = append(bindings, quitHint) } else { @@ -885,6 +891,14 @@ func (m model) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { return m, nil } + if msg.Key().Code == tea.KeyTab && m.focus == rowContent { + if navigator, ok := m.activeView.(linkNavigator); ok && navigator.ClaimsLinkNavigation() { + cmd := m.activeView.HandleContentKey(msg) + m.updateHelpBindings() + return m, m.syncLoading(cmd) + } + } + if msg.Key().Code == tea.KeyTab { if msg.Key().Mod == tea.ModShift { m.focus = (m.focus + 2) % 3 diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go index c4a0dd43..a98f11e6 100644 --- a/internal/tui/tui_test.go +++ b/internal/tui/tui_test.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net/http" + "net/http/httptest" "slices" "strconv" "strings" @@ -68,6 +69,19 @@ func testPostings() []mail.Posting { } } +type linkNavigationTestModal struct { + plainModal + keys []string +} + +func (m *linkNavigationTestModal) handleKey(_ *mailView, msg tea.KeyPressMsg) (tea.Cmd, bool) { + m.keys = append(m.keys, msg.String()) + return nil, true +} + +func (*linkNavigationTestModal) draw(*mailView) string { return "modal" } +func (*linkNavigationTestModal) helpBindings() []helpBinding { return nil } + func keyPress(key string) tea.KeyPressMsg { k := tea.Key{Text: key} switch key { @@ -1277,3 +1291,282 @@ func TestViewShowsBoxNames(t *testing.T) { t.Error("View should contain Imbox") } } + +func openLinkThreadThroughModel(t *testing.T) model { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/topics/100/entries.json": + _, _ = w.Write([]byte(`[ + {"id":503,"kind":"message","summary":"Send a note","created_at":"2026-08-19T11:00:00Z","creator":{"id":30,"name":"Carol"}}, + {"id":502,"kind":"message","summary":"Second report","created_at":"2026-08-19T10:00:00Z","creator":{"id":20,"name":"Bob"}}, + {"id":501,"kind":"message","summary":"First report","created_at":"2026-08-19T09:00:00Z","creator":{"id":10,"name":"Alice"}} + ]`)) + case "/messages/501.json": + _, _ = w.Write([]byte(`{"id":501,"subject":"Three reports","content":"

Read the first report.

","created_at":"2026-08-19T09:00:00Z","creator":{"id":10,"name":"Alice"}}`)) + case "/messages/502.json": + _, _ = w.Write([]byte(`{"id":502,"subject":"Three reports","content":"

Padding padding padding padding padding padding padding padding padding padding padding padding.

Read the second report.

","created_at":"2026-08-19T10:00:00Z","creator":{"id":20,"name":"Bob"}}`)) + case "/messages/503.json": + _, _ = w.Write([]byte(`{"id":503,"subject":"Three reports","content":"

Finally, send a note.

","created_at":"2026-08-19T11:00:00Z","creator":{"id":30,"name":"Carol"}}`)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(server.Close) + + m := modelWithBoxes() + m.mailView.vc.sdk = hey.NewClient( + &hey.Config{BaseURL: server.URL}, + &hey.StaticTokenProvider{Token: "test-token"}, + hey.WithMaxRetries(0), + ) + updated, cmd := m.Update(keyPress("enter")) + m = updated.(model) + if cmd == nil { + t.Fatal("opening the selected posting returned no command") + } + message := runCmd(cmd) + if batch, ok := message.(tea.BatchMsg); ok { + message = runCmd(batch[0]) + } + if _, ok := message.(viewGenerationMsg); !ok { + t.Fatalf("topic command returned %T, want viewGenerationMsg", message) + } + updated, _ = m.Update(message) + m = updated.(model) + if !m.mailView.InThread() || len(m.mailView.links) != 3 { + t.Fatalf("loaded thread links = %d, want 3", len(m.mailView.links)) + } + return m +} + +func TestRootModelKeepsGlobalTabForALinklessThread(t *testing.T) { + m := modelWithBoxes() + m.mailView.inThread = true + m.mailView.entries = []mail.Entry{{ID: 501, Creator: mail.Contact{Name: "Alice"}}} + m.mailView.rebuildTopicContent() + m.updateHelpBindings() + + updated, _ := m.Update(keyPress("tab")) + m = updated.(model) + if m.focus != rowSection { + t.Errorf("linkless thread Tab focus = %d, want rowSection", m.focus) + } +} + +func TestLinkedThreadUsesGlobalTabOutsideContent(t *testing.T) { + m := openLinkThreadThroughModel(t) + m.focus = rowSubnav + updated, _ := m.Update(keyPress("tab")) + m = updated.(model) + if m.focus != rowContent || m.mailView.selectedLink != -1 { + t.Errorf("Tab outside content set focus=%d selectedLink=%d", m.focus, m.mailView.selectedLink) + } +} + +func TestEnterWithoutASelectedLinkDoesNotOpen(t *testing.T) { + m := openLinkThreadThroughModel(t) + opened := false + m.mailView.vc.openURL = func(string) error { + opened = true + return nil + } + updated, cmd := m.Update(keyPress("enter")) + m = updated.(model) + if cmd != nil || opened || m.mailView.selectedLink != -1 { + t.Errorf("Enter without selection returned command=%v opened=%v selected=%d", cmd != nil, opened, m.mailView.selectedLink) + } +} + +func TestThreadLinkNavigationYieldsToAModal(t *testing.T) { + m := openLinkThreadThroughModel(t) + updated, _ := m.Update(keyPress("tab")) + m = updated.(model) + selectedKey := m.mailView.selectedLinkKey + open := &linkNavigationTestModal{} + m.mailView.modal = open + m.updateHelpBindings() + + updated, _ = m.Update(keyPress("tab")) + m = updated.(model) + if len(open.keys) != 1 || open.keys[0] != "tab" { + t.Fatalf("modal keys = %q, want Tab", open.keys) + } + if m.mailView.selectedLinkKey != selectedKey { + t.Error("modal Tab changed the selected thread link") + } +} + +func TestThreadLinkSelectionSurvivesRebuildsAndClearsWhenOccurrenceDisappears(t *testing.T) { + m := openLinkThreadThroughModel(t) + updated, _ := m.Update(keyPress("tab")) + m = updated.(model) + selectedKey := m.mailView.selectedLinkKey + + updated, _ = m.Update(tea.WindowSizeMsg{Width: 48, Height: 40}) + m = updated.(model) + if m.mailView.selectedLinkKey != selectedKey || m.mailView.selectedLink < 0 { + t.Fatal("resize lost the logical link occurrence") + } + selected := m.mailView.links[m.mailView.selectedLink] + if selected.startLine < m.mailView.topicViewport.YOffset() || selected.endLine >= m.mailView.topicViewport.YOffset()+m.mailView.topicViewport.Height() { + t.Errorf("resized selected range %d-%d is outside the viewport", selected.startLine, selected.endLine) + } + + m.mailView.Restyle() + if m.mailView.selectedLinkKey != selectedKey || !strings.Contains(m.mailView.topicContent, "\x1b[7m") { + t.Error("restyle lost the selected occurrence or its styling") + } + m.mailView.attachments = []messageAttachment{{ID: "501:1", MessageID: 501, Filename: "report.pdf"}} + m.mailView.moveAttachmentCursor(1) + if m.mailView.selectedLinkKey != selectedKey { + t.Error("attachment rebuild lost the selected occurrence") + } + + m.mailView.entries = m.mailView.entries[1:] + m.mailView.rebuildTopicContent() + if m.mailView.selectedLink != -1 || m.mailView.selectedLinkKey != "" { + t.Error("removing the selected occurrence did not clear selection") + } +} + +func TestThreadMessageAndAttachmentKeysRemainAvailableWithLinks(t *testing.T) { + m := openLinkThreadThroughModel(t) + updated, _ := m.Update(tea.WindowSizeMsg{Width: 80, Height: 16}) + m = updated.(model) + updated, _ = m.Update(keyPress("tab")) + m = updated.(model) + selectedKey := m.mailView.selectedLinkKey + + updated, _ = m.Update(keyPress("j")) + m = updated.(model) + if m.mailView.topicViewport.YOffset() != m.mailView.entryOffsets[1] { + t.Errorf("j offset = %d, want second message at %d", m.mailView.topicViewport.YOffset(), m.mailView.entryOffsets[1]) + } + + m.mailView.attachments = []messageAttachment{ + {ID: "501:1", MessageID: 501, Filename: "report.pdf"}, + {ID: "501:2", MessageID: 501, Filename: "chart.png"}, + } + m.mailView.rebuildTopicContent() + updated, _ = m.Update(keyPress("]")) + m = updated.(model) + if m.mailView.attachmentCursor != 1 || m.mailView.selectedLinkKey != selectedKey { + t.Errorf("next attachment set cursor=%d selected=%q", m.mailView.attachmentCursor, m.mailView.selectedLinkKey) + } + updated, _ = m.Update(keyPress("[")) + m = updated.(model) + if m.mailView.attachmentCursor != 0 { + t.Errorf("previous attachment cursor = %d, want 0", m.mailView.attachmentCursor) + } +} + +func TestQLeavesAThreadWithASelectedLink(t *testing.T) { + m := openLinkThreadThroughModel(t) + updated, _ := m.Update(keyPress("tab")) + m = updated.(model) + updated, _ = m.Update(keyPress("q")) + m = updated.(model) + if m.mailView.InThread() { + t.Error("q cleared link selection instead of leaving the thread") + } +} + +func TestRootModelNavigatesThreadLinksAndOpensExactDestination(t *testing.T) { + m := openLinkThreadThroughModel(t) + var opened []string + m.mailView.vc.openURL = func(destination string) error { + opened = append(opened, destination) + return nil + } + updated, _ := m.Update(tea.WindowSizeMsg{Width: 80, Height: 16}) + m = updated.(model) + + // These are end-user key messages. In particular, do not call HandleContentKey. + updated, _ = m.Update(keyPress("tab")) + m = updated.(model) + updated, _ = m.Update(keyPress("shift+tab")) + m = updated.(model) + if m.mailView.selectedLink != 2 { + t.Fatalf("reverse wrap selected link = %d, want last occurrence", m.mailView.selectedLink) + } + updated, _ = m.Update(keyPress("tab")) + m = updated.(model) + if m.mailView.selectedLink != 0 { + t.Fatalf("forward wrap selected link = %d, want first occurrence", m.mailView.selectedLink) + } + updated, _ = m.Update(keyPress("tab")) + m = updated.(model) + if m.mailView.selectedLink != 1 { + t.Fatalf("selected link = %d, want second occurrence", m.mailView.selectedLink) + } + if !strings.Contains(stripANSI(m.contentView()), "https://example.org/second?full=destination") { + t.Fatalf("selected destination is not visible: %q", m.contentView()) + } + if m.mailView.topicViewport.YOffset() == 0 { + t.Error("selecting the offscreen second link did not move the viewport") + } + selected := m.mailView.links[m.mailView.selectedLink] + visibleStart := m.mailView.topicViewport.YOffset() + visibleEnd := visibleStart + m.mailView.topicViewport.Height() - 1 + if selected.startLine < visibleStart || selected.endLine > visibleEnd { + t.Errorf("selected range %d-%d is outside viewport %d-%d", selected.startLine, selected.endLine, visibleStart, visibleEnd) + } + if !strings.Contains(m.mailView.topicContent, "\x1b[7m") { + t.Error("selected link has no reverse-video styling") + } + if !hasHelpBinding(m.help.bindings, "enter") || !hasHelpBinding(m.help.bindings, "esc") || !hasHelpBinding(m.help.bindings, "q") || hasHelpBinding(m.help.bindings, "esc/q") { + t.Errorf("selected-link help is inaccurate: %#v", m.help.bindings) + } + + updated, cmd := m.Update(keyPress("enter")) + m = updated.(model) + if cmd == nil { + t.Fatal("enter did not return an opener command") + } + if msg := cmd(); msg == nil { + t.Fatal("opener command returned no result") + } else { + updated, _ = m.Update(msg) + m = updated.(model) + } + if len(opened) != 1 || opened[0] != "https://example.org/second?full=destination" { + t.Fatalf("opened destinations = %q, want exact second destination once", opened) + } + m.mailView.vc.openURL = func(string) error { return errors.New("\x1b[31mblocked\x1b[0m") } + updated, cmd = m.Update(keyPress("enter")) + m = updated.(model) + if cmd == nil { + t.Fatal("enter with a selected link did not retry the opener") + } + updated, _ = m.Update(cmd()) + m = updated.(model) + if !strings.Contains(stripANSI(m.contentView()), "Could not open link: blocked") || strings.Contains(m.contentView(), "\x1b[31m") { + t.Errorf("opener error was not a sanitized visible notice: %q", m.contentView()) + } + + updated, _ = m.Update(keyPress("shift+tab")) + m = updated.(model) + if m.mailView.selectedLink != 0 { + t.Errorf("shift+tab selected %d, want first occurrence", m.mailView.selectedLink) + } + updated, _ = m.Update(keyPress("esc")) + m = updated.(model) + if !m.mailView.InThread() || m.mailView.selectedLink != -1 { + t.Fatal("first escape should clear selection and keep the thread open") + } + if strings.Contains(m.mailView.topicContent, "\x1b[7m") { + t.Error("first escape left selected styling in the thread") + } + for _, notice := range m.mailView.threadNotices() { + if strings.HasPrefix(notice, "Open: ") { + t.Errorf("first escape left destination notice %q", notice) + } + } + updated, _ = m.Update(keyPress("esc")) + m = updated.(model) + if m.mailView.InThread() { + t.Error("second escape should leave the thread") + } +} From 6d965836eb78fc9c05d6da1f5dce4ef6877c63c4 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Mon, 21 Sep 2026 13:06:37 -0400 Subject: [PATCH 2/7] Show complete link targets before opening --- docs/tui.md | 2 +- internal/markdown/linked.go | 2 +- internal/markdown/render_test.go | 2 +- internal/tui/mail.go | 49 ++++++++++++++++++++------- internal/tui/mail_test.go | 58 ++++++++++++++++++++++++++++++++ internal/tui/open_url.go | 2 +- internal/tui/open_url_test.go | 2 +- 7 files changed, 100 insertions(+), 17 deletions(-) diff --git a/docs/tui.md b/docs/tui.md index d6dfd941..f2a90e5e 100644 --- a/docs/tui.md +++ b/docs/tui.md @@ -61,7 +61,7 @@ uppercase belongs to Labels: | Ctrl+A | switch linked account | | Ctrl+V | choose an Imbox cover | -While reading a thread, links can be selected without a mouse. Tab selects the next link in document order and Shift+Tab selects the previous one; both wrap at the ends. The selected destination is shown above the thread, and the viewport moves to show the whole link, including wrapped lines. Press Enter to open it, Escape once to clear the selection, and Escape again to leave the thread. A thread with no selectable links keeps the normal global Tab focus behavior. Existing OSC 8 mouse links remain available. +While reading a thread, links can be selected without a mouse. Tab selects the next link in document order and Shift+Tab selects the previous one; both wrap at the ends. The complete selected destination is shown above the thread, wrapped across rows when necessary, and the viewport moves to show the whole link. Press Enter to open it; opening stays unavailable until the terminal has room to show the complete destination. Press Escape once to clear the selection, and Escape again to leave the thread. A thread with no selectable links keeps the normal global Tab focus behavior. Existing OSC 8 mouse links remain available. Most of those keep working while you are reading a thread, the way the web app's topic toolbar stays live: `r`, `f`, `v`, `b`, `u`, `i`, `l`, `a`, `d`, `p` and `t` all act on diff --git a/internal/markdown/linked.go b/internal/markdown/linked.go index e432e8d3..9c215399 100644 --- a/internal/markdown/linked.go +++ b/internal/markdown/linked.go @@ -112,7 +112,7 @@ func allowedHyperlink(uri string) bool { } switch strings.ToLower(parsed.Scheme) { case "http", "https": - return parsed.Host != "" && parsed.Hostname() != "" + return parsed.User == nil && parsed.Host != "" && parsed.Hostname() != "" case "mailto": return parsed.Opaque != "" default: diff --git a/internal/markdown/render_test.go b/internal/markdown/render_test.go index cad28aba..db441d8b 100644 --- a/internal/markdown/render_test.go +++ b/internal/markdown/render_test.go @@ -109,7 +109,7 @@ func TestRenderLinkedPreservesOrderDuplicatesAndSchemes(t *testing.T) { } func TestRenderLinkedRejectsUnsafeAndRelativeDestinations(t *testing.T) { - linked := RenderLinked(htmlutil.ToMarkdown(`

relative ftp script bad

`), 80, -1) + linked := RenderLinked(htmlutil.ToMarkdown(`

relative ftp script bad userinfo

`), 80, -1) if len(linked.Links) != 0 { t.Fatalf("links = %#v, want no selectable links", linked.Links) } diff --git a/internal/tui/mail.go b/internal/tui/mail.go index d55fbb50..328e1189 100644 --- a/internal/tui/mail.go +++ b/internal/tui/mail.go @@ -9,6 +9,7 @@ import ( "charm.land/bubbles/v2/viewport" tea "charm.land/bubbletea/v2" "charm.land/lipgloss/v2" + "github.com/charmbracelet/x/ansi" "github.com/basecamp/hey-sdk/go/pkg/generated" hey "github.com/basecamp/hey-sdk/go/pkg/hey" @@ -1024,7 +1025,7 @@ func (v *mailView) HelpBindings() []helpBinding { } if v.ClaimsLinkNavigation() { bindings = append(bindings, helpBinding{"tab/shift+tab", "next/previous link"}) - if v.LinkSelectionActive() { + if v.linkDestinationReviewable() { bindings = append(bindings, helpBinding{"enter", "open link"}) } } @@ -1664,18 +1665,14 @@ func (v *mailView) Resize(width, height int) { v.revealLink() } -// threadNotices is what is shown above an open thread's viewport: the partial-read -// notice for as long as the thread is open, and the one-shot notice while it is up. Each -// is one row, truncated to the width, so the rows they take can be counted, and the -// thread itself keeps at least one: in a section too short for both, a notice gives -// way rather than pushing the viewport out. +// threadNotices is what is shown above an open thread's viewport: the selected +// destination, the partial-read notice for as long as the thread is open, and the +// one-shot notice while it is up. A destination wraps in full and can open only when +// every row fits. Other notices stay on one truncated row, and the thread itself keeps +// at least one row. func (v *mailView) threadNotices() []string { - var notices []string - linkNotice := "" - if v.LinkSelectionActive() { - linkNotice = "Open: " + terminal.SanitizeLine(v.links[v.selectedLink].destination) - } - for _, notice := range []string{linkNotice, v.threadNotice, v.notice} { + notices, _ := v.linkNoticeLines() + for _, notice := range []string{v.threadNotice, v.notice} { if notice != "" { notices = append(notices, truncateToWidth(notice, max(v.vc.width, 4))) } @@ -1686,6 +1683,31 @@ func (v *mailView) threadNotices() []string { return notices } +func (v *mailView) linkNoticeLines() ([]string, bool) { + if !v.LinkSelectionActive() { + return nil, false + } + width := v.vc.width + if width <= 0 { + return nil, false + } + notice := "Open: " + terminal.SanitizeLine(v.links[v.selectedLink].destination) + lines := strings.Split(ansi.Hardwrap(notice, width, false), "\n") + room := max(v.contentHeight-1, 0) + if len(lines) <= room { + return lines, true + } + if room == 0 { + return nil, false + } + return []string{truncateToWidth("Enlarge the terminal to inspect this link", width)}, false +} + +func (v *mailView) linkDestinationReviewable() bool { + _, reviewable := v.linkNoticeLines() + return reviewable +} + // fitThreadViewport gives the thread's viewport the rows its notices leave, so the // section never draws more rows than it has. The one-shot notice comes and goes from // dozens of sites, so the fit is also checked where the thread is drawn. @@ -2192,6 +2214,9 @@ func (v *mailView) handleLinkKey(msg tea.KeyPressMsg) (tea.Cmd, bool) { return nil, true } if msg.Key().Code == tea.KeyEnter && v.selectedLink >= 0 { + if !v.linkDestinationReviewable() { + return nil, true + } link := v.links[v.selectedLink] if v.vc.openURL == nil { return nil, true diff --git a/internal/tui/mail_test.go b/internal/tui/mail_test.go index 356f6dc2..90855ca0 100644 --- a/internal/tui/mail_test.go +++ b/internal/tui/mail_test.go @@ -448,6 +448,64 @@ func TestMailViewKeepsAPartialThreadsNoticeAndLeavesItUnseen(t *testing.T) { } } +func TestLinkDestinationMustBeFullyVisibleBeforeOpening(t *testing.T) { + v := newMailView(testVC()) + v.inThread = true + v.topicID = 100 + v.selectedLink = 0 + destination := "https://example.com/" + strings.Repeat("quarterly-report/", 6) + v.links = []mailLink{{destination: destination, key: "501\x000"}} + v.selectedLinkKey = v.links[0].key + v.vc.width = 24 + v.contentHeight = 12 + + lines, reviewable := v.linkNoticeLines() + if !reviewable || len(lines) < 2 { + t.Fatalf("long destination notice = %#v reviewable=%v, want complete wrapped rows", lines, reviewable) + } + if !hasHelpBinding(v.HelpBindings(), "enter") { + t.Error("reviewable destination has no open-link help") + } + if joined := strings.Join(lines, ""); !strings.Contains(joined, destination) || strings.Contains(joined, "...") { + t.Errorf("wrapped destination notice = %q, want the complete destination", joined) + } + + var opened string + v.vc.openURL = func(destination string) error { + opened = destination + return nil + } + cmd, handled := v.handleLinkKey(keyPress("enter")) + if !handled || cmd == nil { + t.Fatal("a fully visible destination was not opened") + } + runCmd(cmd) + if opened != destination { + t.Errorf("opened %q, want %q", opened, destination) + } + + v.contentHeight = 1 + if lines, reviewable := v.linkNoticeLines(); reviewable || len(lines) != 0 { + t.Errorf("one-row destination notice = %#v reviewable=%v", lines, reviewable) + } + if hasHelpBinding(v.HelpBindings(), "enter") { + t.Error("hidden destination still offers open-link help") + } + opened = "" + if cmd, handled := v.handleLinkKey(keyPress("enter")); !handled || cmd != nil || opened != "" { + t.Errorf("hidden destination opened: handled=%v command=%v destination=%q", handled, cmd != nil, opened) + } + + v.vc.width = 3 + v.contentHeight = 12 + if _, reviewable := v.linkNoticeLines(); reviewable { + t.Error("destination wider than a narrow terminal was marked reviewable") + } + if cmd, handled := v.handleLinkKey(keyPress("enter")); !handled || cmd != nil { + t.Errorf("narrow terminal opened destination: handled=%v command=%v", handled, cmd != nil) + } +} + func TestMailViewLeavesBubbledUpThreadAloneWhenOpened(t *testing.T) { v, recorded := mailWithTestServer(t, http.StatusNoContent) v.postingList.postings[0].BubbledUp = true diff --git a/internal/tui/open_url.go b/internal/tui/open_url.go index 4712aca6..963c4e7e 100644 --- a/internal/tui/open_url.go +++ b/internal/tui/open_url.go @@ -55,7 +55,7 @@ func validateURLDestination(destination string) error { switch strings.ToLower(parsed.Scheme) { case "http", "https": - if !parsed.IsAbs() || parsed.Host == "" || parsed.Hostname() == "" { + if !parsed.IsAbs() || parsed.User != nil || parsed.Host == "" || parsed.Hostname() == "" { return fmt.Errorf("invalid URL destination") } case "mailto": diff --git a/internal/tui/open_url_test.go b/internal/tui/open_url_test.go index 5bc0e63b..8760f6e0 100644 --- a/internal/tui/open_url_test.go +++ b/internal/tui/open_url_test.go @@ -56,7 +56,7 @@ func TestOpenExternalURLReturnsLauncherStartupFailure(t *testing.T) { func TestOpenURLCommandValidation(t *testing.T) { valid := []string{"http://example.com", "https://example.com/path", "mailto:alex@example.com"} - invalid := []string{"", "http:/example.com", "http:///path", "http://", "https://?x", "ftp://example.com", "file:///tmp/a", "mailto:", "mailto://", "https://example.com/line\nbreak", "https://example.com/\u0085", "https://example.com/%zz"} + invalid := []string{"", "http:/example.com", "http:///path", "http://", "https://?x", "https://trusted.example@evil.example/path", "ftp://example.com", "file:///tmp/a", "mailto:", "mailto://", "https://example.com/line\nbreak", "https://example.com/\u0085", "https://example.com/%zz"} for _, goos := range []string{"darwin", "linux", "windows", "freebsd", ""} { t.Run(goos, func(t *testing.T) { From 1787e1539d217e65a3d470584ec94158ef3b3489 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Mon, 21 Sep 2026 15:11:59 -0400 Subject: [PATCH 3/7] Stabilize TUI link navigation --- docs/tui.md | 2 +- internal/tui/mail.go | 185 +++++++++++++++++++++++------------ internal/tui/mail_test.go | 53 ++++++---- internal/tui/section_view.go | 7 ++ internal/tui/styles.go | 42 ++++---- internal/tui/tui.go | 53 ++++++++-- internal/tui/tui_test.go | 55 ++++++++--- 7 files changed, 274 insertions(+), 123 deletions(-) diff --git a/docs/tui.md b/docs/tui.md index f2a90e5e..045c2621 100644 --- a/docs/tui.md +++ b/docs/tui.md @@ -61,7 +61,7 @@ uppercase belongs to Labels: | Ctrl+A | switch linked account | | Ctrl+V | choose an Imbox cover | -While reading a thread, links can be selected without a mouse. Tab selects the next link in document order and Shift+Tab selects the previous one; both wrap at the ends. The complete selected destination is shown above the thread, wrapped across rows when necessary, and the viewport moves to show the whole link. Press Enter to open it; opening stays unavailable until the terminal has room to show the complete destination. Press Escape once to clear the selection, and Escape again to leave the thread. A thread with no selectable links keeps the normal global Tab focus behavior. Existing OSC 8 mouse links remain available. +While reading a thread, links can be selected without a mouse. Tab selects the next link in document order and Shift+Tab selects the previous one; both wrap at the ends. A fixed row above the shortcut bar shows the complete selected destination without moving the thread. Press Enter to open it; opening stays unavailable until the terminal is wide enough to show the complete destination. Press Escape once to clear the selection, and Escape again to leave the thread. A thread with no selectable links keeps the normal global Tab focus behavior. Existing OSC 8 mouse links remain available. Most of those keep working while you are reading a thread, the way the web app's topic toolbar stays live: `r`, `f`, `v`, `b`, `u`, `i`, `l`, `a`, `d`, `p` and `t` all act on diff --git a/internal/tui/mail.go b/internal/tui/mail.go index 328e1189..cca6784e 100644 --- a/internal/tui/mail.go +++ b/internal/tui/mail.go @@ -9,7 +9,6 @@ import ( "charm.land/bubbles/v2/viewport" tea "charm.land/bubbletea/v2" "charm.land/lipgloss/v2" - "github.com/charmbracelet/x/ansi" "github.com/basecamp/hey-sdk/go/pkg/generated" hey "github.com/basecamp/hey-sdk/go/pkg/hey" @@ -265,9 +264,17 @@ type mailLink struct { destination string startLine int endLine int + body int + occurrence int key string } +type mailLinkBody struct { + entry int + start int + lines []string +} + type mailView struct { vc *viewContext @@ -278,6 +285,7 @@ type mailView struct { postingList contentList topicViewport viewport.Model topicContent string + topicLines []string topicID int64 threadPosting mail.Posting // snapshot of the posting the open thread was opened from, zero when it has none threadBoxKind string // the box kind the open thread files out of, following it as filings move it @@ -289,6 +297,7 @@ type mailView struct { imageContent string entryOffsets []int // line where each message starts in the thread content links []mailLink + linkBodies []mailLinkBody selectedLink int selectedLinkKey string inThread bool @@ -1025,9 +1034,6 @@ func (v *mailView) HelpBindings() []helpBinding { } if v.ClaimsLinkNavigation() { bindings = append(bindings, helpBinding{"tab/shift+tab", "next/previous link"}) - if v.linkDestinationReviewable() { - bindings = append(bindings, helpBinding{"enter", "open link"}) - } } if len(v.entries) > 1 { bindings = append(bindings, helpBinding{"j/k", "next/previous message"}) @@ -1665,13 +1671,11 @@ func (v *mailView) Resize(width, height int) { v.revealLink() } -// threadNotices is what is shown above an open thread's viewport: the selected -// destination, the partial-read notice for as long as the thread is open, and the -// one-shot notice while it is up. A destination wraps in full and can open only when -// every row fits. Other notices stay on one truncated row, and the thread itself keeps -// at least one row. +// threadNotices is what is shown above an open thread's viewport: the partial-read +// notice for as long as the thread is open, and the one-shot notice while it is up. +// Each stays on one truncated row, and the thread itself keeps at least one row. func (v *mailView) threadNotices() []string { - notices, _ := v.linkNoticeLines() + var notices []string for _, notice := range []string{v.threadNotice, v.notice} { if notice != "" { notices = append(notices, truncateToWidth(notice, max(v.vc.width, 4))) @@ -1683,28 +1687,35 @@ func (v *mailView) threadNotices() []string { return notices } -func (v *mailView) linkNoticeLines() ([]string, bool) { +// LinkFooter reserves one footer row for an open mail thread. Its text stays blank +// until a link is selected, so moving through links never changes the viewport's +// height. A destination can open only when the footer shows it in full. +func (v *mailView) LinkFooter() (text string, visible bool) { + if !v.inThread || v.modal != nil { + return "", false + } + text, _ = v.linkDestinationFooter() + return text, true +} + +func (v *mailView) linkDestinationFooter() (string, bool) { if !v.LinkSelectionActive() { - return nil, false + return "", false } width := v.vc.width if width <= 0 { - return nil, false - } - notice := "Open: " + terminal.SanitizeLine(v.links[v.selectedLink].destination) - lines := strings.Split(ansi.Hardwrap(notice, width, false), "\n") - room := max(v.contentHeight-1, 0) - if len(lines) <= room { - return lines, true + return "", false } - if room == 0 { - return nil, false + destination := terminal.SanitizeLine(v.links[v.selectedLink].destination) + footer := "Open: " + destination + " (press Enter to visit)" + if lipgloss.Width(footer) <= width { + return footer, true } - return []string{truncateToWidth("Enlarge the terminal to inspect this link", width)}, false + return truncateToWidth("Enlarge the terminal to inspect this link", width), false } func (v *mailView) linkDestinationReviewable() bool { - _, reviewable := v.linkNoticeLines() + _, reviewable := v.linkDestinationFooter() return reviewable } @@ -2179,15 +2190,12 @@ func (v *mailView) jumpEntry(delta int) { } func (v *mailView) clearLinkSelection() { - selected := v.selectedLink >= 0 || v.selectedLinkKey != "" + if v.selectedLink < 0 && v.selectedLinkKey == "" { + return + } + v.restoreLinkBody(v.selectedLink) v.selectedLink = -1 v.selectedLinkKey = "" - if selected && v.inThread { - offset := v.topicViewport.YOffset() - v.rebuildTopicContent() - v.topicViewport.SetYOffset(offset) - v.fitThreadViewport() - } } func (v *mailView) handleLinkKey(msg tea.KeyPressMsg) (tea.Cmd, bool) { @@ -2199,17 +2207,17 @@ func (v *mailView) handleLinkKey(msg tea.KeyPressMsg) (tea.Cmd, bool) { if msg.Key().Mod == tea.ModShift { delta = -1 } + previous := v.selectedLink if v.selectedLink < 0 { + v.selectedLink = 0 if delta < 0 { - v.selectedLink = len(v.links) - } else { - v.selectedLink = -1 + v.selectedLink = len(v.links) - 1 } + } else { + v.selectedLink = (v.selectedLink + delta + len(v.links)) % len(v.links) } - v.selectedLink = (v.selectedLink + delta + len(v.links)) % len(v.links) v.selectedLinkKey = v.links[v.selectedLink].key - v.rebuildTopicContent() - v.fitThreadViewport() + v.updateLinkSelection(previous, v.selectedLink) v.revealLink() return nil, true } @@ -2245,10 +2253,12 @@ func (v *mailView) revealLink() { func (v *mailView) rebuildTopicContent() { key := v.selectedLinkKey - rendered, offsets, links := v.renderEntriesWithLinks(v.entries, key) + rendered, offsets, links, bodies := v.renderEntriesWithLinks(v.entries) v.topicContent = rendered + v.imageContent + v.topicLines = strings.Split(v.topicContent, "\n") v.entryOffsets = offsets v.links = links + v.linkBodies = bodies v.selectedLink = -1 for i := range links { if links[i].key == key && key != "" { @@ -2258,8 +2268,52 @@ func (v *mailView) rebuildTopicContent() { } if v.selectedLink < 0 { v.selectedLinkKey = "" + } else { + v.selectLinkBody(v.selectedLink) + } + v.topicViewport.SetContentLines(v.topicLines) +} + +func (v *mailView) updateLinkSelection(previous, selected int) { + if previous >= 0 && v.links[previous].body != v.links[selected].body { + v.restoreLinkBody(previous) + } + v.selectLinkBody(selected) +} + +func (v *mailView) restoreLinkBody(linkIndex int) { + if linkIndex < 0 || linkIndex >= len(v.links) { + return + } + bodyIndex := v.links[linkIndex].body + if bodyIndex < 0 || bodyIndex >= len(v.linkBodies) { + return + } + body := v.linkBodies[bodyIndex] + if body.start < 0 || body.start+len(body.lines) > len(v.topicLines) { + return } - v.topicViewport.SetContent(v.topicContent) + copy(v.topicLines[body.start:body.start+len(body.lines)], body.lines) +} + +func (v *mailView) selectLinkBody(linkIndex int) { + if linkIndex < 0 || linkIndex >= len(v.links) { + return + } + link := v.links[linkIndex] + if link.body < 0 || link.body >= len(v.linkBodies) { + return + } + body := v.linkBodies[link.body] + if body.entry < 0 || body.entry >= len(v.entries) || body.start < 0 || body.start+len(body.lines) > len(v.topicLines) { + return + } + selected := markdown.RenderLinked(v.entries[body.entry].Body, max(v.vc.width-4, 40), link.occurrence) + lines := strings.Split(v.vc.styles.entryBody.Render(selected.Text), "\n") + if len(lines) != len(body.lines) { + return + } + copy(v.topicLines[body.start:body.start+len(lines)], lines) } func (v *mailView) openSelected() tea.Cmd { @@ -3221,15 +3275,20 @@ func (v *mailView) fetchTopic(ctx context.Context, requestID uint64, boxID, topi // renderEntries renders the thread's messages and returns the content along // with the line each message header starts on, for j/k jumps. func (v *mailView) renderEntries(entries []mail.Entry) (string, []int) { - rendered, offsets, _ := v.renderEntriesWithLinks(entries, "") + rendered, offsets, _, _ := v.renderEntriesWithLinks(entries) return rendered, offsets } -func (v *mailView) renderEntriesWithLinks(entries []mail.Entry, selectedKey string) (string, []int, []mailLink) { +func (v *mailView) renderEntriesWithLinks(entries []mail.Entry) (string, []int, []mailLink, []mailLinkBody) { var b strings.Builder offsets := make([]int, 0, len(entries)) lineCount := 0 links := []mailLink{} + bodies := []mailLinkBody{} + write := func(s string) { + b.WriteString(s) + lineCount += strings.Count(s, "\n") + } sepWidth := max(v.vc.width-4, 40) sep := v.vc.styles.separator.Render(strings.Repeat("─", sepWidth)) @@ -3238,17 +3297,14 @@ func (v *mailView) renderEntriesWithLinks(entries []mail.Entry, selectedKey stri if subject := terminal.SanitizeLine(v.topicName); subject != "" { centered := lipgloss.NewStyle().Width(sepWidth).Align(lipgloss.Center).Foreground(colorBright).Bold(true). Render(truncateStr(subject, sepWidth)) - fmt.Fprintf(&b, "%s\n\n", centered) - lineCount += 2 + write(centered + "\n\n") } for i, e := range entries { if i > 0 { - fmt.Fprintf(&b, "%s\n", sep) - lineCount++ + write(sep + "\n") } offsets = append(offsets, lineCount) - entryStart := b.Len() from := e.Creator.Name if from == "" { @@ -3264,38 +3320,41 @@ func (v *mailView) renderEntriesWithLinks(entries []mail.Entry, selectedKey stri // as printThreadStyled in internal/cmd/topic.go. Printed beside a body it repeats // the message's opening line; printed for a body that was read and rendered to // nothing, or for one that was not read, it passes a preview off as the message. - fmt.Fprintf(&b, "%s %s\n", v.vc.styles.entryFrom.Render(terminal.SanitizeLine(from)), v.vc.styles.entryDate.Render(formatDisplayDateTime(e.CreatedAt))) + write(fmt.Sprintf("%s %s\n", v.vc.styles.entryFrom.Render(terminal.SanitizeLine(from)), v.vc.styles.entryDate.Render(formatDisplayDateTime(e.CreatedAt)))) switch { case !e.Body.IsEmpty(): linked := markdown.RenderLinked(e.Body, sepWidth, -1) - localSelected := -1 + bodyStartLine := lineCount + 1 + bodyText := v.vc.styles.entryBody.Render(linked.Text) + bodyIndex := len(bodies) + bodies = append(bodies, mailLinkBody{entry: i, start: bodyStartLine, lines: strings.Split(bodyText, "\n")}) for i, occurrence := range linked.Links { key := fmt.Sprintf("%d\x00%d\x00%s", e.ID, i, occurrence.Destination) - links = append(links, mailLink{destination: occurrence.Destination, startLine: lineCount + 2 + occurrence.StartLine, endLine: lineCount + 2 + occurrence.EndLine, key: key}) - if key == selectedKey { - localSelected = i - } - } - if localSelected >= 0 { - linked = markdown.RenderLinked(e.Body, sepWidth, localSelected) + links = append(links, mailLink{ + destination: occurrence.Destination, + startLine: bodyStartLine + occurrence.StartLine, + endLine: bodyStartLine + occurrence.EndLine, + body: bodyIndex, + occurrence: i, + key: key, + }) } - fmt.Fprintf(&b, "\n%s\n", v.vc.styles.entryBody.Render(linked.Text)) + write("\n" + bodyText + "\n") case e.BodyState == string(threadload.StateHydrated): - fmt.Fprintf(&b, "\n%s\n", v.vc.styles.entryDate.Render("(empty body)")) + write("\n" + v.vc.styles.entryDate.Render("(empty body)") + "\n") case e.BodyState == string(threadload.StateBodyless) && e.Summary != "": - fmt.Fprintf(&b, "\n%s\n", terminal.SanitizeLine(e.Summary)) + write("\n" + terminal.SanitizeLine(e.Summary) + "\n") case e.BodyState == string(threadload.StateBodyless): - fmt.Fprintf(&b, "\n%s\n", v.vc.styles.entryDate.Render("(no body)")) + write("\n" + v.vc.styles.entryDate.Render("(no body)") + "\n") default: - fmt.Fprintf(&b, "\n%s\n", v.vc.styles.entryDate.Render("(body not read: "+e.BodyState+")")) + write("\n" + v.vc.styles.entryDate.Render("(body not read: "+e.BodyState+")") + "\n") } entryAttachments := attachmentsForMessage(v.attachments, e.ID) if panel := renderAttachmentPanel(entryAttachments, selectedAttachmentForMessage(v.attachments, v.attachmentCursor, e.ID)); panel != "" { - fmt.Fprintf(&b, "\n%s\n", panel) + write("\n" + panel + "\n") } - b.WriteString("\n") - lineCount += strings.Count(b.String()[entryStart:], "\n") + write("\n") } - return b.String(), offsets, links + return b.String(), offsets, links, bodies } diff --git a/internal/tui/mail_test.go b/internal/tui/mail_test.go index 90855ca0..fe5cd9bc 100644 --- a/internal/tui/mail_test.go +++ b/internal/tui/mail_test.go @@ -448,26 +448,36 @@ func TestMailViewKeepsAPartialThreadsNoticeAndLeavesItUnseen(t *testing.T) { } } -func TestLinkDestinationMustBeFullyVisibleBeforeOpening(t *testing.T) { +func TestLinkDestinationMustFitFooterBeforeOpening(t *testing.T) { v := newMailView(testVC()) v.inThread = true v.topicID = 100 v.selectedLink = 0 - destination := "https://example.com/" + strings.Repeat("quarterly-report/", 6) + destination := "https://example.com/" + strings.Repeat("quarterly-report/", 3) v.links = []mailLink{{destination: destination, key: "501\x000"}} v.selectedLinkKey = v.links[0].key - v.vc.width = 24 + v.vc.width = 120 v.contentHeight = 12 + v.threadNotice = "Some messages were not read" - lines, reviewable := v.linkNoticeLines() - if !reviewable || len(lines) < 2 { - t.Fatalf("long destination notice = %#v reviewable=%v, want complete wrapped rows", lines, reviewable) + footer, visible := v.LinkFooter() + if !visible || !v.linkDestinationReviewable() { + t.Fatalf("link footer = %q visible=%v, want a reviewable footer", footer, visible) + } + if !strings.Contains(footer, destination) || !strings.Contains(footer, "press Enter to visit") || strings.Contains(footer, "...") { + t.Errorf("link footer = %q, want the complete destination and action", footer) + } + notices := v.threadNotices() + if len(notices) != 1 || notices[0] != v.threadNotice { + t.Errorf("selected link displaced permanent thread notice: %#v", notices) } - if !hasHelpBinding(v.HelpBindings(), "enter") { - t.Error("reviewable destination has no open-link help") + for _, notice := range notices { + if strings.Contains(notice, destination) { + t.Errorf("thread notice still contains destination: %q", notice) + } } - if joined := strings.Join(lines, ""); !strings.Contains(joined, destination) || strings.Contains(joined, "...") { - t.Errorf("wrapped destination notice = %q, want the complete destination", joined) + if hasHelpBinding(v.HelpBindings(), "enter") { + t.Error("footer action is duplicated in help") } var opened string @@ -484,12 +494,10 @@ func TestLinkDestinationMustBeFullyVisibleBeforeOpening(t *testing.T) { t.Errorf("opened %q, want %q", opened, destination) } - v.contentHeight = 1 - if lines, reviewable := v.linkNoticeLines(); reviewable || len(lines) != 0 { - t.Errorf("one-row destination notice = %#v reviewable=%v", lines, reviewable) - } - if hasHelpBinding(v.HelpBindings(), "enter") { - t.Error("hidden destination still offers open-link help") + v.vc.width = 24 + footer, visible = v.LinkFooter() + if !visible || v.linkDestinationReviewable() || strings.Contains(footer, destination) { + t.Errorf("narrow link footer = %q visible=%v reviewable=%v", footer, visible, v.linkDestinationReviewable()) } opened = "" if cmd, handled := v.handleLinkKey(keyPress("enter")); !handled || cmd != nil || opened != "" { @@ -497,12 +505,17 @@ func TestLinkDestinationMustBeFullyVisibleBeforeOpening(t *testing.T) { } v.vc.width = 3 - v.contentHeight = 12 - if _, reviewable := v.linkNoticeLines(); reviewable { - t.Error("destination wider than a narrow terminal was marked reviewable") + if v.linkDestinationReviewable() { + t.Error("destination wider than a tiny terminal was marked reviewable") } if cmd, handled := v.handleLinkKey(keyPress("enter")); !handled || cmd != nil { - t.Errorf("narrow terminal opened destination: handled=%v command=%v", handled, cmd != nil) + t.Errorf("tiny terminal opened destination: handled=%v command=%v", handled, cmd != nil) + } + + v.selectedLink = -1 + footer, visible = v.LinkFooter() + if !visible || footer != "" { + t.Errorf("unselected link footer = %q visible=%v, want one reserved blank row", footer, visible) } } diff --git a/internal/tui/section_view.go b/internal/tui/section_view.go index 0659dca7..a29df454 100644 --- a/internal/tui/section_view.go +++ b/internal/tui/section_view.go @@ -94,6 +94,13 @@ type linkNavigator interface { LinkSelectionActive() bool } +// linkFooterProvider is implemented by a view that reserves a stable footer row +// for contextual detail. The bool reports whether the row belongs on screen even +// when its content is blank. +type linkFooterProvider interface { + LinkFooter() (string, bool) +} + type accountSwitchBlocker interface { AccountSwitchBlocked() bool } diff --git a/internal/tui/styles.go b/internal/tui/styles.go index f717d815..c0ca3d94 100644 --- a/internal/tui/styles.go +++ b/internal/tui/styles.go @@ -144,30 +144,32 @@ func relativeLuminance(c color.Color) float64 { } type styles struct { - app lipgloss.Style - title lipgloss.Style // bold primary for inline titles - pill lipgloss.Style // filled button, for a call to action above a list - entryFrom lipgloss.Style - entryDate lipgloss.Style - entryBody lipgloss.Style - separator lipgloss.Style - helpKey lipgloss.Style - helpDesc lipgloss.Style - helpSep lipgloss.Style + app lipgloss.Style + title lipgloss.Style // bold primary for inline titles + pill lipgloss.Style // filled button, for a call to action above a list + entryFrom lipgloss.Style + entryDate lipgloss.Style + entryBody lipgloss.Style + separator lipgloss.Style + helpKey lipgloss.Style + helpDesc lipgloss.Style + helpSep lipgloss.Style + linkStatus lipgloss.Style } func newStyles() styles { return styles{ - app: lipgloss.NewStyle().Padding(1, 2), - title: lipgloss.NewStyle().Foreground(colorPrimary).Bold(true), - pill: lipgloss.NewStyle().Foreground(colorOnAccent).Background(colorPrimary).Bold(true).Padding(0, 1), - entryFrom: lipgloss.NewStyle().Foreground(colorPrimary).Bold(true), - entryDate: styleMuted, - entryBody: lipgloss.NewStyle(), - separator: lipgloss.NewStyle().Foreground(colorChrome), - helpKey: lipgloss.NewStyle().Foreground(colorChrome).Bold(true), - helpDesc: lipgloss.NewStyle().Foreground(colorChrome), - helpSep: lipgloss.NewStyle().Foreground(colorChrome), + app: lipgloss.NewStyle().Padding(1, 2), + title: lipgloss.NewStyle().Foreground(colorPrimary).Bold(true), + pill: lipgloss.NewStyle().Foreground(colorOnAccent).Background(colorPrimary).Bold(true).Padding(0, 1), + entryFrom: lipgloss.NewStyle().Foreground(colorPrimary).Bold(true), + entryDate: styleMuted, + entryBody: lipgloss.NewStyle(), + separator: lipgloss.NewStyle().Foreground(colorChrome), + helpKey: lipgloss.NewStyle().Foreground(colorChrome).Bold(true), + helpDesc: lipgloss.NewStyle().Foreground(colorChrome), + helpSep: lipgloss.NewStyle().Foreground(colorChrome), + linkStatus: lipgloss.NewStyle().Foreground(colorLink).Bold(true), } } diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 6769f324..31c6586e 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -679,17 +679,27 @@ func (m model) View() tea.View { b.WriteString(content) helpView := m.help.view() - if helpView != "" { + linkFooter, linkFooterVisible := m.linkFooter() + if helpView != "" || linkFooterVisible { contentLines := strings.Count(b.String(), "\n") - helpH := strings.Count(helpView, "\n") + 1 - footerH := 1 + helpH + helpH := 0 + if helpView != "" { + helpH = strings.Count(helpView, "\n") + 1 + } + footerH := helpH + 3 padLines := m.height - contentLines - footerH - 1 for range max(padLines, 0) { b.WriteString("\n") } b.WriteString(renderRule(m.width, "")) - b.WriteString("\n" + helpView) + b.WriteString("\n") + if linkFooter != "" { + b.WriteString(m.styles.linkStatus.Render(linkFooter)) + } + if helpView != "" { + b.WriteString("\n" + helpView) + } } v := tea.NewView(b.String()) @@ -723,9 +733,9 @@ func (m *model) updateHelpBindings() { extra := m.activeView.HelpBindings() bindings = make([]helpBinding, 0, 4+len(extra)) bindings = append(bindings, helpBinding{"↑↓", "scroll"}) - if navigator, ok := m.activeView.(linkNavigator); ok && navigator.LinkSelectionActive() { + if navigator, ok := m.activeView.(linkNavigator); ok && navigator.ClaimsLinkNavigation() { bindings = append(bindings, - helpBinding{"esc", "clear link"}, + helpBinding{"esc", "clear/back"}, helpBinding{"q", "back"}, ) } else { @@ -782,11 +792,33 @@ func (m *model) updateHelpBindings() { } } +func (m model) linkFooter() (string, bool) { + if provider, ok := m.activeView.(linkFooterProvider); ok { + return provider.LinkFooter() + } + return "", false +} + +func (m model) linkFooterFits() bool { + _, visible := m.linkFooter() + if !visible { + return false + } + statusHeight := 0 + if m.mailWatchNotice() != "" { + statusHeight = 1 + } + return m.height >= headerHeight+m.help.height()+3+statusHeight+1 +} + // contentHeight gives the active view every row that is not navigation or a -// visible help footer. The footer carries two clear rows above its divider. +// visible footer. Ordinary help has two clear rows above its divider; a link +// footer uses one of them for its stable status row. func (m model) contentHeight() int { footerHeight := 0 - if helpHeight := m.help.height(); helpHeight > 0 { + helpHeight := m.help.height() + _, linkFooterVisible := m.linkFooter() + if helpHeight > 0 || linkFooterVisible { footerHeight = helpHeight + 3 } statusHeight := 0 @@ -929,6 +961,11 @@ func (m model) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { m.updateHelpBindings() return m, m.syncLoading(cmd) case rowContent: + if msg.Key().Code == tea.KeyEnter { + if navigator, ok := m.activeView.(linkNavigator); ok && navigator.LinkSelectionActive() && !m.linkFooterFits() { + return m, nil + } + } cmd := m.activeView.HandleContentKey(msg) cmd = m.syncLoading(cmd) m.updateHelpBindings() diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go index a98f11e6..3f7e1f77 100644 --- a/internal/tui/tui_test.go +++ b/internal/tui/tui_test.go @@ -1415,7 +1415,7 @@ func TestThreadLinkSelectionSurvivesRebuildsAndClearsWhenOccurrenceDisappears(t } m.mailView.Restyle() - if m.mailView.selectedLinkKey != selectedKey || !strings.Contains(m.mailView.topicContent, "\x1b[7m") { + if m.mailView.selectedLinkKey != selectedKey || !strings.Contains(m.mailView.topicViewport.View(), "\x1b[7m") { t.Error("restyle lost the selected occurrence or its styling") } m.mailView.attachments = []messageAttachment{{ID: "501:1", MessageID: 501, Filename: "report.pdf"}} @@ -1484,6 +1484,15 @@ func TestRootModelNavigatesThreadLinksAndOpensExactDestination(t *testing.T) { m = updated.(model) // These are end-user key messages. In particular, do not call HandleContentKey. + unselectedViewportHeight := m.mailView.topicViewport.Height() + cachedTopicContent := m.mailView.topicContent + if len(m.mailView.topicLines) == 0 { + t.Fatal("thread has no cached lines") + } + cachedFirstLine := &m.mailView.topicLines[0] + if footer, visible := m.mailView.LinkFooter(); !visible || footer != "" { + t.Fatalf("unselected link footer = %q visible=%v, want one reserved blank row", footer, visible) + } updated, _ = m.Update(keyPress("tab")) m = updated.(model) updated, _ = m.Update(keyPress("shift+tab")) @@ -1501,8 +1510,18 @@ func TestRootModelNavigatesThreadLinksAndOpensExactDestination(t *testing.T) { if m.mailView.selectedLink != 1 { t.Fatalf("selected link = %d, want second occurrence", m.mailView.selectedLink) } - if !strings.Contains(stripANSI(m.contentView()), "https://example.org/second?full=destination") { - t.Fatalf("selected destination is not visible: %q", m.contentView()) + if strings.Contains(stripANSI(m.contentView()), "Open: https://example.org/second?full=destination") { + t.Fatalf("selected destination is still above the thread: %q", m.contentView()) + } + footerText := "Open: https://example.org/second?full=destination (press Enter to visit)" + if !strings.Contains(stripANSI(m.View().Content), footerText) { + t.Fatalf("selected destination is not in the footer: %q", m.View().Content) + } + if !strings.Contains(m.View().Content, m.styles.linkStatus.Render(footerText)) { + t.Fatalf("selected destination does not use the link status style: %q", m.View().Content) + } + if m.mailView.topicViewport.Height() != unselectedViewportHeight { + t.Errorf("link selection changed viewport height from %d to %d", unselectedViewportHeight, m.mailView.topicViewport.Height()) } if m.mailView.topicViewport.YOffset() == 0 { t.Error("selecting the offscreen second link did not move the viewport") @@ -1513,15 +1532,28 @@ func TestRootModelNavigatesThreadLinksAndOpensExactDestination(t *testing.T) { if selected.startLine < visibleStart || selected.endLine > visibleEnd { t.Errorf("selected range %d-%d is outside viewport %d-%d", selected.startLine, selected.endLine, visibleStart, visibleEnd) } - if !strings.Contains(m.mailView.topicContent, "\x1b[7m") { - t.Error("selected link has no reverse-video styling") + if m.mailView.topicContent != cachedTopicContent || &m.mailView.topicLines[0] != cachedFirstLine { + t.Error("link navigation rebuilt the cached thread content") + } + if !strings.Contains(m.mailView.topicViewport.View(), "\x1b[7m") { + t.Errorf("selected link has no reverse-video styling: %q", m.mailView.topicViewport.View()) } - if !hasHelpBinding(m.help.bindings, "enter") || !hasHelpBinding(m.help.bindings, "esc") || !hasHelpBinding(m.help.bindings, "q") || hasHelpBinding(m.help.bindings, "esc/q") { + if hasHelpBinding(m.help.bindings, "enter") || !hasHelpBinding(m.help.bindings, "esc") || !hasHelpBinding(m.help.bindings, "q") || hasHelpBinding(m.help.bindings, "esc/q") { t.Errorf("selected-link help is inaccurate: %#v", m.help.bindings) } + updated, _ = m.Update(tea.WindowSizeMsg{Width: 80, Height: 7}) + m = updated.(model) updated, cmd := m.Update(keyPress("enter")) m = updated.(model) + if cmd != nil || len(opened) != 0 { + t.Fatalf("enter opened a destination while the footer was below the terminal: command=%v opened=%q", cmd != nil, opened) + } + updated, _ = m.Update(tea.WindowSizeMsg{Width: 80, Height: 16}) + m = updated.(model) + + updated, cmd = m.Update(keyPress("enter")) + m = updated.(model) if cmd == nil { t.Fatal("enter did not return an opener command") } @@ -1556,13 +1588,14 @@ func TestRootModelNavigatesThreadLinksAndOpensExactDestination(t *testing.T) { if !m.mailView.InThread() || m.mailView.selectedLink != -1 { t.Fatal("first escape should clear selection and keep the thread open") } - if strings.Contains(m.mailView.topicContent, "\x1b[7m") { + if strings.Contains(m.mailView.topicViewport.View(), "\x1b[7m") { t.Error("first escape left selected styling in the thread") } - for _, notice := range m.mailView.threadNotices() { - if strings.HasPrefix(notice, "Open: ") { - t.Errorf("first escape left destination notice %q", notice) - } + if footer, visible := m.mailView.LinkFooter(); !visible || footer != "" { + t.Errorf("first escape left link footer %q visible=%v", footer, visible) + } + if m.mailView.topicViewport.Height() != unselectedViewportHeight { + t.Errorf("clearing link changed viewport height from %d to %d", unselectedViewportHeight, m.mailView.topicViewport.Height()) } updated, _ = m.Update(keyPress("esc")) m = updated.(model) From 4c74a531d26f59bc8ea9e50d1a1cff5ced0303b3 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Mon, 21 Sep 2026 15:35:17 -0400 Subject: [PATCH 4/7] Preserve TUI footer spacing --- internal/tui/tui.go | 13 +++++++++---- internal/tui/tui_test.go | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 31c6586e..c3b5eefe 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -686,16 +686,21 @@ func (m model) View() tea.View { if helpView != "" { helpH = strings.Count(helpView, "\n") + 1 } - footerH := helpH + 3 + footerH := 1 + helpH + if linkFooterVisible { + footerH++ + } padLines := m.height - contentLines - footerH - 1 for range max(padLines, 0) { b.WriteString("\n") } b.WriteString(renderRule(m.width, "")) - b.WriteString("\n") - if linkFooter != "" { - b.WriteString(m.styles.linkStatus.Render(linkFooter)) + if linkFooterVisible { + b.WriteString("\n") + if linkFooter != "" { + b.WriteString(m.styles.linkStatus.Render(linkFooter)) + } } if helpView != "" { b.WriteString("\n" + helpView) diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go index 3f7e1f77..1e225ade 100644 --- a/internal/tui/tui_test.go +++ b/internal/tui/tui_test.go @@ -205,6 +205,20 @@ func TestScreenerRequestWaitsForMailThenOpensTheScreener(t *testing.T) { } } +func TestFooterLeavesOnlyTheTerminalSafetyRow(t *testing.T) { + m := modelWithBoxes() + if rows := strings.Count(m.View().Content, "\n") + 1; rows != m.height-1 { + t.Errorf("ordinary view rows = %d, want %d", rows, m.height-1) + } + + m = openLinkThreadThroughModel(t) + updated, _ := m.Update(tea.WindowSizeMsg{Width: 80, Height: 16}) + m = updated.(model) + if rows := strings.Count(m.View().Content, "\n") + 1; rows != m.height-1 { + t.Errorf("linked thread rows = %d, want %d", rows, m.height-1) + } +} + func TestQuestionMarkTogglesHelpAndResizesContent(t *testing.T) { m := modelWithBoxes() visibleHeight := m.vc.height From 8a5e52b701b91d79314dcb4af1a2c97675b5d9c1 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Mon, 21 Sep 2026 16:46:48 -0400 Subject: [PATCH 5/7] Handle link anchors and launcher failures --- internal/markdown/linked.go | 9 +++++---- internal/markdown/render_test.go | 22 ++++++++++++++++++++++ internal/tui/open_url.go | 16 ++++++---------- internal/tui/open_url_test.go | 21 ++++++++++++++++----- 4 files changed, 49 insertions(+), 19 deletions(-) diff --git a/internal/markdown/linked.go b/internal/markdown/linked.go index 9c215399..3de446cd 100644 --- a/internal/markdown/linked.go +++ b/internal/markdown/linked.go @@ -39,7 +39,6 @@ func linkedRender(out string, selected int) LinkedRender { links := make([]LinkOccurrence, 0) line := 0 currentDestination := "" - currentVisible := "" currentComplete := false for i := 0; i < len(out); { if strings.HasPrefix(out[i:], "\x1b]8;") { @@ -60,12 +59,14 @@ func linkedRender(out string, selected int) LinkedRender { if destination != currentDestination || currentComplete { links = append(links, LinkOccurrence{Destination: destination, StartLine: startLine, EndLine: endLine}) currentDestination = destination - currentVisible = "" } else { links[len(links)-1].EndLine = endLine } - currentVisible += withoutWhitespace(ansi.Strip(content)) - currentComplete = strings.HasSuffix(currentVisible, withoutWhitespace(destination)) + // Glamour renders a named anchor as one OSC 8 span for + // its label and a second span for the shown destination. + // Only the destination span completes the occurrence. A + // label that merely ends with the URL must not do so. + currentComplete = withoutWhitespace(ansi.Strip(content)) == withoutWhitespace(destination) b.WriteString(openEnd) if len(links)-1 == selected { diff --git a/internal/markdown/render_test.go b/internal/markdown/render_test.go index db441d8b..983c33e4 100644 --- a/internal/markdown/render_test.go +++ b/internal/markdown/render_test.go @@ -138,6 +138,28 @@ func TestRenderLinkedReportsWrappedLinkRange(t *testing.T) { } } +func TestRenderLinkedKeepsOneAnchorWhoseLabelContainsItsDestinationTogether(t *testing.T) { + for _, tt := range []struct { + name string + label string + selectedSpans int + }{ + {name: "ends with destination", label: "Read https://example.com", selectedSpans: 2}, + {name: "equals destination", label: "https://example.com", selectedSpans: 1}, + } { + t.Run(tt.name, func(t *testing.T) { + source := `

` + tt.label + `

` + linked := RenderLinked(htmlutil.ToMarkdown(source), 80, 0) + if len(linked.Links) != 1 { + t.Fatalf("links = %#v, want one occurrence; rendered text = %q", linked.Links, linked.Text) + } + if got := strings.Count(linked.Text, "\x1b[27m"); got != tt.selectedSpans { + t.Errorf("selected spans = %d, want %d: %q", got, tt.selectedSpans, linked.Text) + } + }) + } +} + func TestRenderLinkedKeepsDuplicateLinksSeparate(t *testing.T) { for name, source := range map[string]string{ "labels": `

oneagain

`, diff --git a/internal/tui/open_url.go b/internal/tui/open_url.go index 963c4e7e..374b5f9a 100644 --- a/internal/tui/open_url.go +++ b/internal/tui/open_url.go @@ -10,18 +10,18 @@ import ( "unicode" ) -type urlCommandStarter func(string, ...string) error +type urlCommandRunner func(string, ...string) error func openExternalURL(destination string) error { - return openURLWith(runtime.GOOS, destination, startURLCommand) + return openURLWith(runtime.GOOS, destination, runURLCommand) } -func openURLWith(goos, destination string, start urlCommandStarter) error { +func openURLWith(goos, destination string, run urlCommandRunner) error { name, args, err := openURLCommand(goos, destination) if err != nil { return err } - return start(name, args...) + return run(name, args...) } func openURLCommand(goos, destination string) (string, []string, error) { @@ -68,11 +68,7 @@ func validateURLDestination(destination string) error { return nil } -func startURLCommand(name string, args ...string) error { +func runURLCommand(name string, args ...string) error { command := exec.CommandContext(context.Background(), name, args...) // #nosec G204 -- fixed OS launcher receives the validated destination as one argument - if err := command.Start(); err != nil { - return err - } - go func() { _ = command.Wait() }() - return nil + return command.Run() } diff --git a/internal/tui/open_url_test.go b/internal/tui/open_url_test.go index 8760f6e0..db3c8925 100644 --- a/internal/tui/open_url_test.go +++ b/internal/tui/open_url_test.go @@ -2,6 +2,7 @@ package tui import ( "errors" + "os/exec" "testing" ) @@ -37,23 +38,33 @@ func TestOpenURLCommand(t *testing.T) { } } -func TestOpenExternalURLReturnsLauncherStartupFailure(t *testing.T) { - startupErr := errors.New("launcher unavailable") +func TestOpenExternalURLReturnsLauncherFailure(t *testing.T) { + launcherErr := errors.New("launcher failed") var executable string var arguments []string err := openURLWith("linux", "https://example.com/report", func(name string, args ...string) error { executable = name arguments = append(arguments, args...) - return startupErr + return launcherErr }) - if !errors.Is(err, startupErr) { - t.Fatalf("openURLWith error = %v, want startup failure", err) + if !errors.Is(err, launcherErr) { + t.Fatalf("openURLWith error = %v, want launcher failure", err) } if executable != "xdg-open" || len(arguments) != 1 || arguments[0] != "https://example.com/report" { t.Errorf("launcher = %q %#v", executable, arguments) } } +func TestRunURLCommandReturnsExitFailure(t *testing.T) { + command, err := exec.LookPath("false") + if err != nil { + t.Skip("false command is unavailable") + } + if err := runURLCommand(command); err == nil { + t.Fatal("runURLCommand returned success for a failed launcher") + } +} + func TestOpenURLCommandValidation(t *testing.T) { valid := []string{"http://example.com", "https://example.com/path", "mailto:alex@example.com"} invalid := []string{"", "http:/example.com", "http:///path", "http://", "https://?x", "https://trusted.example@evil.example/path", "ftp://example.com", "file:///tmp/a", "mailto:", "mailto://", "https://example.com/line\nbreak", "https://example.com/\u0085", "https://example.com/%zz"} From f3f7df5403929eaa817a34a95b6fbe2a9210b949 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Mon, 21 Sep 2026 22:01:15 -0400 Subject: [PATCH 6/7] Keep link launchers asynchronous --- internal/tui/mail.go | 8 +++--- internal/tui/open_url.go | 25 +++++++++++++---- internal/tui/open_url_test.go | 52 +++++++++++++++++++++++++++-------- internal/tui/tui_test.go | 11 +++++++- 4 files changed, 73 insertions(+), 23 deletions(-) diff --git a/internal/tui/mail.go b/internal/tui/mail.go index cca6784e..d0526068 100644 --- a/internal/tui/mail.go +++ b/internal/tui/mail.go @@ -1687,11 +1687,11 @@ func (v *mailView) threadNotices() []string { return notices } -// LinkFooter reserves one footer row for an open mail thread. Its text stays blank -// until a link is selected, so moving through links never changes the viewport's -// height. A destination can open only when the footer shows it in full. +// LinkFooter reserves one footer row for a thread with selectable links. Its text +// stays blank until a link is selected, so moving through links never changes the +// viewport's height. A destination can open only when the footer shows it in full. func (v *mailView) LinkFooter() (text string, visible bool) { - if !v.inThread || v.modal != nil { + if !v.inThread || v.modal != nil || len(v.links) == 0 { return "", false } text, _ = v.linkDestinationFooter() diff --git a/internal/tui/open_url.go b/internal/tui/open_url.go index 374b5f9a..2936911e 100644 --- a/internal/tui/open_url.go +++ b/internal/tui/open_url.go @@ -10,18 +10,23 @@ import ( "unicode" ) -type urlCommandRunner func(string, ...string) error +type urlCommandStarter func(string, ...string) error + +type urlProcess interface { + Start() error + Wait() error +} func openExternalURL(destination string) error { - return openURLWith(runtime.GOOS, destination, runURLCommand) + return openURLWith(runtime.GOOS, destination, startURLCommand) } -func openURLWith(goos, destination string, run urlCommandRunner) error { +func openURLWith(goos, destination string, start urlCommandStarter) error { name, args, err := openURLCommand(goos, destination) if err != nil { return err } - return run(name, args...) + return start(name, args...) } func openURLCommand(goos, destination string) (string, []string, error) { @@ -68,7 +73,15 @@ func validateURLDestination(destination string) error { return nil } -func runURLCommand(name string, args ...string) error { +func startURLCommand(name string, args ...string) error { command := exec.CommandContext(context.Background(), name, args...) // #nosec G204 -- fixed OS launcher receives the validated destination as one argument - return command.Run() + return startURLProcess(command) +} + +func startURLProcess(process urlProcess) error { + if err := process.Start(); err != nil { + return err + } + go func() { _ = process.Wait() }() + return nil } diff --git a/internal/tui/open_url_test.go b/internal/tui/open_url_test.go index db3c8925..159f1d15 100644 --- a/internal/tui/open_url_test.go +++ b/internal/tui/open_url_test.go @@ -2,8 +2,8 @@ package tui import ( "errors" - "os/exec" "testing" + "time" ) func TestOpenURLCommand(t *testing.T) { @@ -38,33 +38,61 @@ func TestOpenURLCommand(t *testing.T) { } } -func TestOpenExternalURLReturnsLauncherFailure(t *testing.T) { - launcherErr := errors.New("launcher failed") +func TestOpenExternalURLReturnsLauncherStartupFailure(t *testing.T) { + startupErr := errors.New("launcher unavailable") var executable string var arguments []string err := openURLWith("linux", "https://example.com/report", func(name string, args ...string) error { executable = name arguments = append(arguments, args...) - return launcherErr + return startupErr }) - if !errors.Is(err, launcherErr) { - t.Fatalf("openURLWith error = %v, want launcher failure", err) + if !errors.Is(err, startupErr) { + t.Fatalf("openURLWith error = %v, want startup failure", err) } if executable != "xdg-open" || len(arguments) != 1 || arguments[0] != "https://example.com/report" { t.Errorf("launcher = %q %#v", executable, arguments) } } -func TestRunURLCommandReturnsExitFailure(t *testing.T) { - command, err := exec.LookPath("false") - if err != nil { - t.Skip("false command is unavailable") +func TestURLProcessReturnsAfterStartAndReapsInBackground(t *testing.T) { + process := &blockingURLProcess{ + release: make(chan struct{}), + waited: make(chan struct{}), } - if err := runURLCommand(command); err == nil { - t.Fatal("runURLCommand returned success for a failed launcher") + done := make(chan error, 1) + go func() { done <- startURLProcess(process) }() + + select { + case err := <-done: + if err != nil { + t.Fatalf("startURLProcess error = %v", err) + } + case <-time.After(time.Second): + close(process.release) + t.Fatal("startURLProcess waited for the launcher to exit") + } + close(process.release) + select { + case <-process.waited: + case <-time.After(time.Second): + t.Fatal("launcher process was not reaped") } } +type blockingURLProcess struct { + release chan struct{} + waited chan struct{} +} + +func (*blockingURLProcess) Start() error { return nil } + +func (p *blockingURLProcess) Wait() error { + <-p.release + close(p.waited) + return nil +} + func TestOpenURLCommandValidation(t *testing.T) { valid := []string{"http://example.com", "https://example.com/path", "mailto:alex@example.com"} invalid := []string{"", "http:/example.com", "http:///path", "http://", "https://?x", "https://trusted.example@evil.example/path", "ftp://example.com", "file:///tmp/a", "mailto:", "mailto://", "https://example.com/line\nbreak", "https://example.com/\u0085", "https://example.com/%zz"} diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go index 1e225ade..9ec0c5b2 100644 --- a/internal/tui/tui_test.go +++ b/internal/tui/tui_test.go @@ -1362,7 +1362,16 @@ func TestRootModelKeepsGlobalTabForALinklessThread(t *testing.T) { m.mailView.rebuildTopicContent() m.updateHelpBindings() - updated, _ := m.Update(keyPress("tab")) + if footer, visible := m.mailView.LinkFooter(); visible || footer != "" { + t.Errorf("linkless thread footer = %q visible=%v, want no reserved row", footer, visible) + } + updated, _ := m.Update(tea.WindowSizeMsg{Width: m.width, Height: m.height}) + m = updated.(model) + if got, want := m.mailView.topicViewport.Height(), m.contentHeight(); got != want { + t.Errorf("linkless thread viewport height = %d, want all %d content rows", got, want) + } + + updated, _ = m.Update(keyPress("tab")) m = updated.(model) if m.focus != rowSection { t.Errorf("linkless thread Tab focus = %d, want rowSection", m.focus) From 8f23fa3c7a297b678ff90617dbe9e2e63baecc04 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Tue, 22 Sep 2026 00:08:42 -0400 Subject: [PATCH 7/7] Keep rendered link spans together --- internal/markdown/linked.go | 40 ++++++++++++++++++++++++++++---- internal/markdown/render_test.go | 8 +++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/internal/markdown/linked.go b/internal/markdown/linked.go index 3de446cd..ebc3acf9 100644 --- a/internal/markdown/linked.go +++ b/internal/markdown/linked.go @@ -39,6 +39,8 @@ func linkedRender(out string, selected int) LinkedRender { links := make([]LinkOccurrence, 0) line := 0 currentDestination := "" + currentShownDestination := false + currentShownText := "" currentComplete := false for i := 0; i < len(out); { if strings.HasPrefix(out[i:], "\x1b]8;") { @@ -59,14 +61,24 @@ func linkedRender(out string, selected int) LinkedRender { if destination != currentDestination || currentComplete { links = append(links, LinkOccurrence{Destination: destination, StartLine: startLine, EndLine: endLine}) currentDestination = destination + currentShownDestination = false + currentShownText = "" } else { links[len(links)-1].EndLine = endLine } // Glamour renders a named anchor as one OSC 8 span for - // its label and a second span for the shown destination. - // Only the destination span completes the occurrence. A - // label that merely ends with the URL must not do so. - currentComplete = withoutWhitespace(ansi.Strip(content)) == withoutWhitespace(destination) + // its label and one or more underlined spans for the shown + // destination. Only those destination spans complete the + // occurrence. A plain fallback URL has no style prefix. + if !currentShownDestination && (precededByUnderline(out, i) || content == destination) { + currentShownDestination = true + } + if currentShownDestination { + currentShownText += withoutWhitespace(ansi.Strip(content)) + currentComplete = currentShownText == withoutWhitespace(destination) + } else { + currentComplete = false + } b.WriteString(openEnd) if len(links)-1 == selected { @@ -121,6 +133,26 @@ func allowedHyperlink(uri string) bool { } } +func precededByUnderline(out string, position int) bool { + start := strings.LastIndex(out[:position], "\x1b[") + if start < 0 { + return false + } + sequence, _, ok := sgr(out[start:position]) + if !ok || start+len(sequence) != position { + return false + } + parameters := strings.FieldsFunc(sequence[2:len(sequence)-1], func(r rune) bool { + return r == ';' || r == ':' + }) + for _, parameter := range parameters { + if parameter == "4" { + return true + } + } + return false +} + func withoutWhitespace(s string) string { return strings.Map(func(r rune) rune { if r == ' ' || r == '\t' || r == '\n' || r == '\r' { diff --git a/internal/markdown/render_test.go b/internal/markdown/render_test.go index 983c33e4..e6c18e10 100644 --- a/internal/markdown/render_test.go +++ b/internal/markdown/render_test.go @@ -146,6 +146,7 @@ func TestRenderLinkedKeepsOneAnchorWhoseLabelContainsItsDestinationTogether(t *t }{ {name: "ends with destination", label: "Read https://example.com", selectedSpans: 2}, {name: "equals destination", label: "https://example.com", selectedSpans: 1}, + {name: "equals after whitespace is removed", label: "https://example. com", selectedSpans: 2}, } { t.Run(tt.name, func(t *testing.T) { source := `

` + tt.label + `

` @@ -160,6 +161,13 @@ func TestRenderLinkedKeepsOneAnchorWhoseLabelContainsItsDestinationTogether(t *t } } +func TestRenderLinkedKeepsWrappedDuplicateLinksSeparate(t *testing.T) { + linked := RenderLinked(htmlutil.ToMarkdown(`

one again

`), 12, -1) + if len(linked.Links) != 2 { + t.Fatalf("links = %#v, want two occurrences; rendered text = %q", linked.Links, linked.Text) + } +} + func TestRenderLinkedKeepsDuplicateLinksSeparate(t *testing.T) { for name, source := range map[string]string{ "labels": `

oneagain

`,