diff --git a/README.md b/README.md index c1e284e..c5be2f5 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ The glamour style is built into the binary (`style.go`) and tuned for a narrow p - Compact, with at most one blank line between blocks, no margins, and no trailing-space padding. - Hex (truecolor) colors throughout, never 256-palette indexes, which Ghostty remaps. -- Teal, underlined links. Bare URLs stay on their own line, so Ghostty can detect them and make them clickable. +- Teal, underlined links. Bare URLs (on their own line, per convention) render as real OSC 8 hyperlinks — clickable in any terminal that supports them, Ghostty included — with the visible text shortened to fit the pane when the URL itself is longer, though the link always opens the full, untruncated address. In a terminal without OSC 8 support, the shortened text is just plain text. - An H1 badge in black on lavender, H2 headings in muted amber with a `▍ ` prefix, and bold text in the default foreground. To adjust the colors, edit the `color…` constants at the top of `style.go` — links use `colorLink` — and rebuild. diff --git a/diff_test.go b/diff_test.go index 3b54ae8..060e95c 100644 --- a/diff_test.go +++ b/diff_test.go @@ -160,7 +160,7 @@ func TestChangedLinesDeletionNoSpuriousMark(t *testing.T) { func TestComposeMarkedNeverWiderThanWidth(t *testing.T) { for _, w := range []int{20, 40, 80} { lines, _ := func() ([]string, error) { - out, err := renderMarkdown("# Title\n\n- a fairly long bullet item that will wrap\n- short\n\nsome prose here too\n", w) + out, err := renderMarkdown("# Title\n\n- a fairly long bullet item that will wrap\n- short\n\nsome prose here too\n", w, true) return strings.Split(out, "\n"), err }() all := map[int]bool{} diff --git a/go.mod b/go.mod index 0147d99..b5430b8 100644 --- a/go.mod +++ b/go.mod @@ -7,8 +7,8 @@ require ( github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/glamour v1.0.0 github.com/charmbracelet/lipgloss v1.1.1-0.20250404203927-76690c660834 + github.com/charmbracelet/x/ansi v0.11.6 github.com/fsnotify/fsnotify v1.10.1 - github.com/muesli/reflow v0.3.0 github.com/muesli/termenv v0.16.0 golang.org/x/term v0.43.0 ) @@ -19,7 +19,6 @@ require ( github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect github.com/aymerick/douceur v0.2.0 // indirect github.com/charmbracelet/colorprofile v0.4.1 // indirect - github.com/charmbracelet/x/ansi v0.11.6 // indirect github.com/charmbracelet/x/cellbuf v0.0.15 // indirect github.com/charmbracelet/x/exp/slice v0.0.0-20250327172914-2fdc97757edf // indirect github.com/charmbracelet/x/term v0.2.2 // indirect @@ -36,6 +35,7 @@ require ( github.com/microcosm-cc/bluemonday v1.0.27 // indirect github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect github.com/muesli/cancelreader v0.2.2 // indirect + github.com/muesli/reflow v0.3.0 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect github.com/yuin/goldmark v1.7.13 // indirect diff --git a/main.go b/main.go index a7059d1..c7e0266 100644 --- a/main.go +++ b/main.go @@ -97,7 +97,14 @@ func main() { // runStatic renders the file once to stdout and exits — no watching, no // alt-screen. Handy for piping, CI, and quick inline checks. Width is the -// terminal width (minus 2) when stdout is a TTY, else 80. +// terminal width (minus 2) when stdout is a TTY, else 80. Bare-URL OSC 8 +// hyperlinking (and the display-text elision that comes with it) is skipped +// whenever stdout isn't a TTY: whatever's on the other end of the pipe — +// grep, an editor, a CI log — can't render the escape sequence anyway. A +// URL short enough to fit the fallback width comes through plain and +// intact; one long enough to still need wrapping hits glamour's original +// hard break instead (see renderMarkdown's linkify doc comment) — out of +// scope for this fix, just not silently hidden behind an OSC 8 escape. func runStatic(args []string) int { path := defaultBoardPath() if len(args) > 0 { @@ -113,11 +120,12 @@ func runStatic(args []string) int { fmt.Fprintln(os.Stderr, "sidecar:", err) return 1 } + isTTY := term.IsTerminal(int(os.Stdout.Fd())) width := 80 if w, _, err := term.GetSize(int(os.Stdout.Fd())); err == nil && w > 0 { width = w } - out, err := renderMarkdown(string(data), width-2) + out, err := renderMarkdown(string(data), width-2, isTTY) if err != nil { fmt.Fprintln(os.Stderr, "sidecar:", err) return 1 diff --git a/render_test.go b/render_test.go index 794ca68..1d6697b 100644 --- a/render_test.go +++ b/render_test.go @@ -1,10 +1,13 @@ package main import ( + "fmt" "os" "regexp" "strings" "testing" + + xansi "github.com/charmbracelet/x/ansi" ) func renderFixture(t *testing.T, width int) string { @@ -13,7 +16,7 @@ func renderFixture(t *testing.T, width int) string { if err != nil { t.Fatal(err) } - out, err := renderMarkdown(string(raw), width) + out, err := renderMarkdown(string(raw), width, true) if err != nil { t.Fatal(err) } @@ -38,7 +41,9 @@ func TestCompactSpacing(t *testing.T) { } // NEVER render wider than the requested width — padded/overwide lines wrap -// in the pane and fake double-spacing. +// in the pane and fake double-spacing. This holds even for bare-URL lines +// wider than the width: their OSC 8 target carries the full URL, but the +// visible display text is elided to fit (see TestBareURLIntact, issue #15). func TestNeverWiderThanWidth(t *testing.T) { for _, width := range []int{40, 60, 78} { out := renderFixture(t, width) @@ -51,6 +56,29 @@ func TestNeverWiderThanWidth(t *testing.T) { } } +// The hyperlinked-line path has width-sensitive elision math the rest of +// the renderer doesn't (restoreBareURLs' budget calculation), so it gets +// its own sweep across every width from the render-width clamp floor up — +// scoped to lines that actually contain a hyperlink, so it isn't tripped +// up by glamour's own pre-existing, unrelated wrap imprecision on ordinary +// text at odd widths (a real but out-of-scope issue: e.g. at width 15 a +// plain non-URL blockquote line measures one cell over, independent of +// anything in this file). +func TestHyperlinkedLineNeverWiderThanWidth(t *testing.T) { + for w := 10; w <= 100; w++ { + out := renderFixture(t, w) + for i, line := range strings.Split(out, "\n") { + if !oscLinkRE.MatchString(line) { + continue + } + if got := visibleWidth(line); got > w { + t.Errorf("width %d, line %d: visible width %d: %q", + w, i, got, stripANSI(line)) + } + } + } +} + // No trailing-space padding on any line (glow's -w padding bug). func TestNoTrailingSpacePadding(t *testing.T) { out := renderFixture(t, 78) @@ -61,32 +89,447 @@ func TestNoTrailingSpacePadding(t *testing.T) { } } -// Bare URLs must survive intact on a single line so Ghostty's link -// detection can make them clickable. +// oscLinkRE finds an OSC 8 hyperlink's target and display text. The display +// text carries its own SGR styling, so it isn't ANSI-escape-free — match +// non-greedily up to the closing OSC 8 rather than excluding ESC outright. +var oscLinkRE = regexp.MustCompile(`(?s)\x1b]8;;([^\x07\n]*)\x07(.*?)\x1b]8;;\x07`) + +// Bare URLs must be reachable via a single, complete OSC 8 hyperlink on one +// line — even at pane widths narrower than the URL itself, which is the +// common case and is what used to hard-split mid-URL (issue #15), and later +// what a naive fix let the viewport silently truncate instead. func TestBareURLIntact(t *testing.T) { - out := stripANSI(renderFixture(t, 78)) + out := renderFixture(t, 40) for _, url := range []string{ "https://github.com/example/app/pull/412", "https://qa.example.dev/checkout-race", } { found := false for _, line := range strings.Split(out, "\n") { - if n := strings.Count(line, url); n > 0 { + for _, m := range oscLinkRE.FindAllStringSubmatch(line, -1) { + if m[1] != url { + continue + } found = true - if strings.Count(line, "http") > 1 { - t.Errorf("URL duplicated on line: %q", line) + if n := strings.Count(line, "\x1b]8;;"+url+"\x07"); n > 1 { + t.Errorf("target %s duplicated on line: %q", url, line) + } + // The display text should use most of the available + // width, not just technically "fit" — a single bare URL + // alone on its own line, at a width comfortably close to + // its own length, has no reason to elide down to almost + // nothing. Root-cause regression guard: the elision + // budget briefly measured glamour's own trailing + // pad-to-width filler as if it were real content, which + // starved every hyperlinked line's display text down to a + // cell or two while still passing every other check here + // (target intact, one line, width never exceeded). + if w := visibleWidth(m[2]); w < 20 { + t.Errorf("display text for %s suspiciously narrow (%d cells) given ~38 available: %q", url, w, stripANSI(m[2])) } } } if !found { - t.Errorf("URL %s not intact on a single line:\n%s", url, out) + t.Errorf("URL %s has no complete OSC 8 hyperlink on one line:\n%s", url, stripANSI(out)) } } } +// linkify=false (the non-TTY path runStatic uses when stdout isn't a +// terminal) must skip the stash/hyperlink machinery entirely — a plain, +// complete URL for grep/CI/an editor, not an OSC 8 escape it can't render +// and won't see through. At runStatic's real non-TTY width (80), the +// fixture's URLs fit without wrapping at all — the common case linkify=false +// is meant to help. (At a narrower width a long URL still hits the +// original pre-#15 wrap-split via glamour's own autolink path; skipping the +// stash doesn't and isn't meant to fix that — it only avoids hiding an +// already-short URL behind an unreadable escape sequence.) +func TestLinkifyFalseSkipsHyperlinking(t *testing.T) { + raw, err := os.ReadFile("testdata/REVIEW.md") + if err != nil { + t.Fatal(err) + } + out, err := renderMarkdown(string(raw), 78, false) + if err != nil { + t.Fatal(err) + } + if oscLinkRE.MatchString(out) { + t.Errorf("linkify=false still produced an OSC 8 hyperlink:\n%s", stripANSI(out)) + } + if strings.Contains(out, "\x1fU") { + t.Errorf("a stash placeholder leaked with linkify=false:\n%s", stripANSI(out)) + } + if !strings.Contains(stripANSI(out), "https://github.com/example/app/pull/412") { + t.Errorf("plain URL missing with linkify=false:\n%s", stripANSI(out)) + } +} + +// An ordered-list URL (`1. https://…`) is a real board shape, not just a +// bulleted one — it must get the same fix, not fall through to the +// original wrap-split bug. +func TestBareURLOrderedListMarker(t *testing.T) { + raw := "1. https://example.test/ordered-list-item-long-enough-to-need-eliding\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil || m[1] != "https://example.test/ordered-list-item-long-enough-to-need-eliding" { + t.Errorf("ordered-list URL wasn't hyperlinked: %v\n%s", m, stripANSI(out)) + } +} + +// Two bare URLs that elide to identical visible text must still each get +// their own correct hyperlink target — no cross-linking. (A global text +// search that re-finds the first occurrence is the failure mode this +// guards against; index-keyed placeholders avoid it structurally.) +func TestBareURLCollisionSafe(t *testing.T) { + raw := "- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1\n" + + "- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + links := oscLinkRE.FindAllStringSubmatch(out, -1) + if len(links) != 2 { + t.Fatalf("want 2 hyperlinks, got %d:\n%s", len(links), stripANSI(out)) + } + if links[0][1] != "https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1" { + t.Errorf("first link target wrong: %q", links[0][1]) + } + if links[1][1] != "https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2" { + t.Errorf("second link target wrong: %q", links[1][1]) + } +} + +// A board line that already contains \x1f (a pasted-tool-output edge case) +// must not prefix-match a real placeholder and substitute the wrong URL. +func TestBareURLSourceControlCharNeutralized(t *testing.T) { + raw := "- weird\x1fbytes here, not a url\n- https://example.test/the-real-target-url\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + if strings.Contains(out, "\x1f") { + t.Errorf("raw \\x1f from the source survived into output: %q", out) + } + links := oscLinkRE.FindAllStringSubmatch(out, -1) + if len(links) != 1 || links[0][1] != "https://example.test/the-real-target-url" { + t.Errorf("wrong or missing link, want exactly the real URL: %v", links) + } +} + +// Two bare URLs as consecutive continuation lines of one paragraph reflow +// onto a single physical rendered line — the same reflow +// TestBareURLKeepsTrailingContent already relies on, just with a second +// placeholder instead of trailing prose. Both must keep a usable display +// width sharing that line's budget, not have the first URL claim nearly +// all of it and starve the second down to a cell or two. +func TestBareURLsSharingLineSplitBudgetFairly(t *testing.T) { + raw := "See docs:\nhttps://a.example.test/some/long/path\nhttps://b.example.test/some/long/path\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + links := oscLinkRE.FindAllStringSubmatch(out, -1) + if len(links) != 2 { + t.Fatalf("want 2 hyperlinks, got %d:\n%s", len(links), stripANSI(out)) + } + const wantTarget0 = "https://a.example.test/some/long/path" + const wantTarget1 = "https://b.example.test/some/long/path" + if links[0][1] != wantTarget0 { + t.Errorf("first target wrong: %q", links[0][1]) + } + if links[1][1] != wantTarget1 { + t.Errorf("second target wrong: %q", links[1][1]) + } + // A "usable" floor, not a precise one: enough to recognize the domain, + // not just "h…". The old, per-URL-not-per-line budget calculation + // gave the second URL 1-2 cells here. + const minUsableDisplay = 8 + for i, m := range links { + if w := visibleWidth(m[2]); w < minUsableDisplay { + t.Errorf("link %d display text too narrow to be usable: %d cells (%q)", i, w, stripANSI(m[2])) + } + } +} + +// A bare URL inside a fenced code block is content the user typed verbatim +// — stashBareURLs must leave it alone entirely (no OSC 8 hyperlink, no +// leaked placeholder). Whatever glamour itself does with fenced content +// (it word-wraps long hyphenated lines same as any other text — pre-existing, +// unrelated to bare-URL handling) is out of scope here. +func TestBareURLInFenceUntouched(t *testing.T) { + raw := "```\nhttps://example.test/verbatim-in-a-fence-that-is-long-enough-to-elide\n```\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + if oscLinkRE.MatchString(out) { + t.Errorf("URL inside a fence got hyperlinked:\n%s", stripANSI(out)) + } + if strings.Contains(out, "\x1fU") { + t.Errorf("a stash placeholder leaked into fenced output:\n%s", stripANSI(out)) + } +} + +// A control byte or non-ASCII byte in a URL must not break out of the OSC 8 +// escape (injection) or get silently dropped (lossy percent-encoding) \u2014 in +// EITHER half of the hyperlink. The target (m[1]) percent-encodes so the +// link stays meaningful; the display text (m[2]) just needs the control +// bytes gone outright, since a raw ESC/BEL there reaches the terminal as a +// live escape sequence (glamour never sees the stashed URL to neutralize +// it, and neither xansi.Truncate nor termenv sanitize). +func TestBareURLUnsafeBytesEncoded(t *testing.T) { + raw := "- https://example.test/caf\u00e9-and-a-bell-\x07-in-the-middle\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil { + t.Fatalf("no hyperlink found:\n%s", stripANSI(out)) + } + if strings.ContainsAny(m[1], "\x07\x1b") { + t.Fatalf("target still contains a raw control byte: %q", m[1]) + } + if !strings.Contains(m[1], "%C3%A9") || !strings.Contains(m[1], "%07") { + t.Errorf("target wasn't percent-encoded correctly: %q", m[1]) + } + // The display text legitimately carries our own SGR styling (teal, + // underline), which is itself ESC bytes \u2014 strip that (SGR-only; OSC 8 + // isn't SGR and survives) before checking for anything else. + if plain := stripANSI(m[2]); strings.ContainsAny(plain, "\x07\x1b\x00") { + t.Fatalf("display text still contains a raw control byte \u2014 escape injection: %q", plain) + } +} + +// An ESC byte in a URL must not let a pasted-in escape sequence \u2014 most +// pointedly a second, attacker-controlled OSC 8 open \u2014 reach the terminal +// inside the display text. Board files are agent-written from pasted tool +// output, so this isn't hypothetical. +func TestBareURLDisplayTextNoEscapeInjection(t *testing.T) { + raw := "- https://example.test/x\x1b]8;;https://evil.test\x07pwned\n" + out, err := renderMarkdown(raw, 60, true) + if err != nil { + t.Fatal(err) + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil { + t.Fatalf("no hyperlink found:\n%s", stripANSI(out)) + } + plain := stripANSI(m[2]) // strip our own legitimate SGR styling first + if strings.Contains(plain, "\x1b]8;;") { + t.Fatalf("display text carries an injected OSC 8 open: %q", plain) + } + if strings.Contains(plain, "\x1b") { + t.Fatalf("display text contains a raw ESC byte: %q", plain) + } +} + +// A nested list item's budget must account for its actual indent, not a +// guessed constant — a hard-coded top-level-bullet reserve would overshoot +// on nested items and re-break the line it's meant to fix. +func TestBareURLNestedIndentBudget(t *testing.T) { + raw := "- Parent\n - https://example.test/nested-item-url-thats-long-enough-to-need-eliding\n" + out, err := renderMarkdown(raw, 30, true) + if err != nil { + t.Fatal(err) + } + for i, line := range strings.Split(out, "\n") { + if w := visibleWidth(line); w > 30 { + t.Errorf("line %d: visible width %d > 30: %q", i, w, stripANSI(line)) + } + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil || m[1] != "https://example.test/nested-item-url-thats-long-enough-to-need-eliding" { + t.Errorf("nested URL lost its hyperlink target: %v", m) + } +} + +// A URL as its own continuation line under a 2-levels-deep nested bullet +// reaches the same 4-space indent as an indented code block, but it's a +// CommonMark lazy list continuation, not code — preceded by a non-blank +// line (the child item itself), not a blank one. It must still be +// hyperlinked, not skipped by the indented-code-block guard. +func TestBareURLNestedListContinuationNotMistakenForCode(t *testing.T) { + raw := "- Parent\n - Child item\n https://example.test/nested-continuation-line-long-enough-to-elide\n" + out, err := renderMarkdown(raw, 30, true) + if err != nil { + t.Fatal(err) + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil || m[1] != "https://example.test/nested-continuation-line-long-enough-to-elide" { + t.Errorf("nested list continuation URL wasn't hyperlinked (mistaken for indented code?): %v\n%s", m, stripANSI(out)) + } +} + +// changedLines must still detect a change when only a URL's target differs +// but its elided display text happens to be identical — the diff key has to +// retain the OSC 8 target, not just the visible text. +func TestChangedLinesSeesURLTargetChange(t *testing.T) { + before, err := renderMarkdown("- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1\n", 40, true) + if err != nil { + t.Fatal(err) + } + after, err := renderMarkdown("- https://example.test/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2\n", 40, true) + if err != nil { + t.Fatal(err) + } + changed := changedLines(strings.Split(before, "\n"), strings.Split(after, "\n")) + if len(changed) == 0 { + t.Errorf("target-only URL change went undetected") + } +} + +// The load-bearing assumption behind this whole approach: bubbletea's +// renderer and lipgloss's MaxWidth both truncate the final frame to the +// pane width using charmbracelet/x/ansi, which understands OSC 8 and only +// cuts visible cells. If that ever regresses to an OSC-blind truncator, a +// hyperlinked line gets cut inside the escape and the display text (which +// comes after it) is dropped outright — this pins the assumption directly. +func TestHyperlinkSurvivesDownstreamTruncation(t *testing.T) { + target := "https://example.test/downstream-truncation-gate" + line := "prefix " + hyperlink(target, "short") + got := xansi.Truncate(line, 20, "") + m := oscLinkRE.FindStringSubmatch(got) + if m == nil || m[1] != target { + t.Fatalf("x/ansi.Truncate dropped or corrupted the hyperlink: %q", got) + } + if !strings.Contains(stripANSI(got), "short") { + t.Errorf("x/ansi.Truncate dropped the display text: %q", got) + } +} + +// A bare-URL line that's a soft-wrapped continuation inside a multi-line +// paragraph can have real content after the URL on the same rendered line +// (e.g. "See docs at\nhttps://…\nfor more." reflows to one paragraph). +// That trailing content must survive, and the elision budget must leave it +// room — restoreBareURLs used to discard everything after the placeholder. +func TestBareURLKeepsTrailingContent(t *testing.T) { + raw := "See the docs at\nhttps://example.test/reasonably-long-path-name\nfor more information, seriously.\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + plain := stripANSI(out) + if !strings.Contains(plain, "for more") { + t.Errorf("trailing paragraph content after the URL was dropped:\n%s", plain) + } + for i, line := range strings.Split(out, "\n") { + if w := visibleWidth(line); w > 40 { + t.Errorf("line %d: visible width %d > 40: %q", i, w, stripANSI(line)) + } + } +} + +// budget < 1 (indent alone fills the pane) must drop the URL rather than +// render a line wider than the pane — the one invariant every other line +// in this renderer holds. Exercised directly against restoreBareURLs: a +// markdown fixture that reliably produces a zero-budget nested indent +// through the real glamour pipeline is brittle to construct, and this is +// the exact boundary the fix claims to hold. +func TestBareURLNoRoomDropsRatherThanOverflows(t *testing.T) { + prefix := strings.Repeat("x", 10) // consumes the entire width on its own + rendered := prefix + urlPlaceholder(0) + out := restoreBareURLs(rendered, []string{"https://example.test/no-room-left"}, 10) + if w := visibleWidth(out); w > 10 { + t.Errorf("visible width %d > 10: %q", w, out) + } + if strings.Contains(out, "\x1f") { + t.Errorf("placeholder leaked into output: %q", out) + } + if strings.Contains(out, "http") { + t.Errorf("URL text should have been dropped, not shown partially: %q", out) + } +} + +// A CRLF board must still get the fix — bareURLLine's trailing-whitespace +// class has to include \r or a CRLF board silently keeps the pre-fix +// wrap-split bug. +func TestBareURLCRLFStillStashed(t *testing.T) { + raw := "- https://example.test/crlf-board-long-enough-to-need-eliding\r\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + m := oscLinkRE.FindStringSubmatch(out) + if m == nil || m[1] != "https://example.test/crlf-board-long-enough-to-need-eliding" { + t.Errorf("CRLF board line wasn't hyperlinked: %v\n%s", m, stripANSI(out)) + } +} + +// A URL inside a 4-space indented code block is CommonMark verbatim +// content, same as a fenced block — it must not be truncated or +// hyperlinked. +func TestBareURLInIndentedCodeBlockUntouched(t *testing.T) { + raw := "Some paragraph.\n\n https://example.test/indented-code-block-verbatim-text\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + if oscLinkRE.MatchString(out) { + t.Errorf("URL inside an indented code block got hyperlinked:\n%s", stripANSI(out)) + } + if strings.Contains(out, "\x1fU") { + t.Errorf("a stash placeholder leaked into indented-code-block output:\n%s", stripANSI(out)) + } +} + +// An indented code block is a block, not a per-line property: a second URL +// deeper in the same block (whose own immediate predecessor isn't blank) +// must be protected exactly like the first one, whose predecessor is. A +// per-line wasPrevBlank check alone catches the first URL and misses the +// second, so two URLs in the same verbatim block would render differently +// from each other. +func TestBareURLIndentedCodeBlockMultiLine(t *testing.T) { + raw := "Run these:\n\n https://example.test/first-verbatim-command\n https://example.test/second-verbatim-command\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + if oscLinkRE.MatchString(out) { + t.Errorf("a URL deeper in a multi-line indented code block got hyperlinked:\n%s", stripANSI(out)) + } + if strings.Contains(out, "\x1fU") { + t.Errorf("a stash placeholder leaked into multi-line indented-code-block output:\n%s", stripANSI(out)) + } +} + +// The line that CLOSES an indented code block must still be checked as a +// possible fence opener itself — an else-branch skip there would let a +// fence right after an indented block never open, hyperlinking the URL +// inside it (exactly what fence protection exists to prevent) and, worse, +// leave the line meant to CLOSE that fence opening a phantom one instead — +// silently disabling the whole fix for every real bare URL for the rest of +// the document. +func TestBareURLFenceAfterIndentedCodeBlock(t *testing.T) { + raw := "intro\n\n indented code\n\n```\nhttps://example.test/long-url-inside-a-fence-after-code\n```\n\n" + + "- https://example.test/real-url-after-the-fence-must-still-be-fixed\n" + out, err := renderMarkdown(raw, 40, true) + if err != nil { + t.Fatal(err) + } + links := oscLinkRE.FindAllStringSubmatch(out, -1) + for _, m := range links { + if m[1] == "https://example.test/long-url-inside-a-fence-after-code" { + t.Errorf("URL inside the fence got hyperlinked — fence never opened:\n%s", stripANSI(out)) + } + } + found := false + for _, m := range links { + if m[1] == "https://example.test/real-url-after-the-fence-must-still-be-fixed" { + found = true + } + } + if !found { + t.Errorf("real bare URL after the fence wasn't hyperlinked — phantom fence never closed:\n%s", stripANSI(out)) + } +} + // Emoji section markers are double-width; wrapping must account for that. func TestEmojiHeadingWidth(t *testing.T) { - out, err := renderMarkdown("## 🔴 Needs action right now with a long heading tail end", 40) + out, err := renderMarkdown("## 🔴 Needs action right now with a long heading tail end", 40, true) if err != nil { t.Fatal(err) } @@ -97,6 +540,25 @@ func TestEmojiHeadingWidth(t *testing.T) { } } +// A hyperlink's display text must carry raw truecolor SGR, not termenv's +// auto-detected-profile styling — every test here runs with stdout not a +// terminal, which is exactly the condition under which termenv.String +// would silently drop the styling and this would be the one link on +// screen that isn't truecolor while everything else (forced via +// glamour.WithColorProfile / diff.go's hand-written 38;2;r;g;b) stays so. +func TestHyperlinkDisplayIsRawTruecolor(t *testing.T) { + out := renderFixture(t, 40) + m := oscLinkRE.FindStringSubmatch(out) + if m == nil { + t.Fatal("no hyperlink found in fixture at width 40") + } + lr, lg, lb := hexToRGB(colorLink) + want := fmt.Sprintf("\x1b[4;38;2;%d;%d;%dm", lr, lg, lb) + if !strings.Contains(m[2], want) { + t.Errorf("display text missing raw truecolor SGR %q: %q", want, m[2]) + } +} + // Colors must be hex, not 256-palette indexes (Ghostty remaps the palette). func TestTrueColorOutput(t *testing.T) { out := renderFixture(t, 78) diff --git a/style.go b/style.go index e5db43d..0649dd5 100644 --- a/style.go +++ b/style.go @@ -1,11 +1,14 @@ package main import ( + "fmt" + "regexp" + "strconv" "strings" "github.com/charmbracelet/glamour" "github.com/charmbracelet/glamour/ansi" - reflowansi "github.com/muesli/reflow/ansi" + xansi "github.com/charmbracelet/x/ansi" "github.com/muesli/termenv" ) @@ -117,14 +120,334 @@ func styleConfig() ansi.StyleConfig { } } +// bareURLLine matches a markdown line that is nothing but a bare URL — the +// board convention for links (see README.md's rendering-style section: +// "Bare URLs ... render as real OSC 8 hyperlinks") — whether it's an +// indented continuation line under a bullet, a bulleted top-level item, or +// an ordered-list item (`1.`/`1)`). Groups: 1 = leading indent, 2 = +// optional list marker (with its trailing space), 3 = the URL, 4 = trailing +// whitespace, \r included so a CRLF board doesn't +// silently skip the fix. +var bareURLLine = regexp.MustCompile(`^([ \t]*)((?:(?:[-*+]|\d+[.)])\s+)?)(https?://\S+)([ \t\r]*)$`) + +// fenceLine matches a fenced-code-block delimiter (``` or ~~~, 3+ of the +// same character). Bare URLs inside a fence are content the user typed +// verbatim and are never word-wrapped by glamour in the first place, so +// stashBareURLs leaves them alone. +var fenceLine = regexp.MustCompile("^[ \t]*(```+|~~~+)") + +// urlPlaceholder is a short, markdown-inert stand-in for a stashed URL. It +// uses the ASCII unit separator as a delimiter so it can never collide with +// real board text, and stays well under any realistic wrap width — short +// enough that glamour's word-wrap never has a reason to touch it. +func urlPlaceholder(i int) string { + return fmt.Sprintf("\x1fU%d\x1f", i) +} + +// residualPlaceholder matches any urlPlaceholder token — a fallback sweep +// for the case restoreBareURLs' own placeholder search doesn't find one. +var residualPlaceholder = regexp.MustCompile(`\x1fU\d+\x1f`) + +// listMarkerPrefix matches a line beginning with a list marker, regardless +// of what follows — used to tell an indented code block's opening line +// apart from a list item that merely reaches the same 4-space indent. +var listMarkerPrefix = regexp.MustCompile(`^[ \t]*(?:[-*+]|\d+[.)])\s+`) + +// leadingIndent returns line's leading run of spaces/tabs. +func leadingIndent(line string) string { + i := 0 + for i < len(line) && (line[i] == ' ' || line[i] == '\t') { + i++ + } + return line[:i] +} + +// hasCodeIndent reports whether indent (spaces/tabs only) is deep enough to +// open or continue a CommonMark indented code block: 4+ spaces, or any tab. +func hasCodeIndent(indent string) bool { + return strings.Contains(indent, "\t") || len(indent) >= 4 +} + +// stashBareURLs replaces every bare-URL-only line with a short placeholder, +// returning the stashed URLs in order (skipping lines inside fenced or +// indented code blocks). Glamour's word-wrap hard-splits a long link token +// mid-URL once it exceeds the wrap width — it treats the autolink's +// rendered ANSI run differently from plain text and force-breaks it instead +// of pushing the whole word to the next line — so the placeholder keeps +// such lines out of that path entirely. restoreBareURLs puts the real, +// styled, hyperlinked URL back after rendering. +func stashBareURLs(raw string) (string, []string) { + // A board line already containing \x1f — a pasted-tool-output edge + // case, the same threat model stripControlBytes exists for — would + // otherwise prefix-match a real placeholder in restoreBareURLs' search + // and substitute the wrong URL. \x1f has no legitimate use in board + // text, so it's always safe to drop from the input outright. + raw = strings.ReplaceAll(raw, "\x1f", "") + lines := strings.Split(raw, "\n") + var urls []string + var fenceChar byte + var fenceLen int + inIndentedCode := false + prevBlank := true // document start counts as a boundary, same as CommonMark + for i, line := range lines { + wasPrevBlank := prevBlank + blank := strings.TrimSpace(line) == "" + prevBlank = blank + + if fenceChar != 0 { + // Already inside a fence: only a run of the same character, + // at least as long as the one that opened it, closes it — + // anything else (including a run of the other fence + // character) is just content. + if m := fenceLine.FindStringSubmatch(line); m != nil { + c, n := m[1][0], len(m[1]) + if c == fenceChar && n >= fenceLen { + fenceChar, fenceLen = 0, 0 + } + } + continue + } + + // Indented code is a *block*, not a per-line property: once a + // blank line followed by a 4-space (or tab) indented, unmarked + // line opens one, every subsequent indented line belongs to it — + // regardless of whether that later line's own predecessor was + // blank — until a non-blank, non-indented line closes it. Tracked + // as running state across every line (URL or not), symmetric with + // the fence tracking above, so a second URL deeper in the same + // block doesn't fall through the guard the first URL was caught by. + indent := leadingIndent(line) + indented := hasCodeIndent(indent) + if inIndentedCode { + if blank || indented { + continue + } + inIndentedCode = false + // Falls through to the fence/open checks below rather than + // an else-branch skip: the line that closes an indented code + // block (non-blank, non-indented — the case that just fell + // through above) still needs to be checked as a possible + // fence opener itself. Missing that let a ``` line closing + // an indented block skip fence-recognition entirely, so the + // fence it should have opened never did, the bare URL right + // after it went unprotected, and the *next* fence-looking + // line (meant to close that fence) opened a phantom one + // instead — silently reverting every real bare URL for the + // rest of the document. `indented` is false here by + // construction (that's what let this branch fall through at + // all), so the indented-code-open check below cannot misfire + // on this same line. + } + // A fence delimiter only opens a fence outside an indented code + // block — a ``` or ~~~ line that's itself part of one is just + // indented content, same as any other line in it, so this check + // has to come after the indented-code state above or an indented + // fence-looking line would open a fence that never finds its + // close and silently reverts every bare URL for the rest of the + // document. + if m := fenceLine.FindStringSubmatch(line); m != nil { + fenceChar, fenceLen = m[1][0], len(m[1]) + continue + } + if wasPrevBlank && !blank && indented && !listMarkerPrefix.MatchString(line) { + inIndentedCode = true + continue + } + + m := bareURLLine.FindStringSubmatch(line) + if m == nil { + continue + } + // Known boundary, not a bug: a *loose* list's second paragraph + // (blank line, then a 4-space-indented URL under a bullet) opens + // this same indented-code state and falls through to the old + // wrap-split. Distinguishing it needs real list-context tracking; + // the board convention doesn't produce loose lists, so this hasn't + // been worth the complexity. + urls = append(urls, m[3]) + // m[4] (trailing whitespace) is dropped, not reinserted: tidy() + // strips it from the final output anyway, and keeping it here + // would shrink restoreBareURLs' elision budget for spaces nobody + // sees. + lines[i] = m[1] + m[2] + urlPlaceholder(len(urls)-1) + } + return strings.Join(lines, "\n"), urls +} + +// oscOpen/oscClose delimit an OSC 8 hyperlink. BEL-terminated rather than +// ST (ESC \\): this codebase's other ANSI helpers only recognize a bare +// ESC[0m-style reset, and BEL is a single unambiguous byte to bound a +// terminator scan on. +const oscOpen = "\x1b]8;;" +const oscBEL = "\x07" +const oscClose = oscOpen + oscBEL + +// oscTarget encodes url for safe use as an OSC 8 target: bytes that could +// terminate the escape early (control bytes, DEL) or aren't valid in a URI +// (anything non-ASCII) are percent-encoded rather than rejected, so a +// pathological or non-ASCII URL still gets a working, complete link instead +// of silently losing its hyperlink. +func oscTarget(url string) string { + var b strings.Builder + for i := 0; i < len(url); i++ { + c := url[i] + if c < 0x20 || c == 0x7f || c >= 0x80 { + fmt.Fprintf(&b, "%%%02X", c) + continue + } + b.WriteByte(c) + } + return b.String() +} + +// hyperlink wraps display in an OSC 8 hyperlink pointing at target. display +// carries its own SGR styling; the OSC 8 escapes only add the link. +func hyperlink(target, display string) string { + return oscOpen + oscTarget(target) + oscBEL + display + oscClose +} + +// stripControlBytes removes ASCII control bytes and DEL from s. The stashed +// URL is never seen by glamour (it's replaced with a placeholder before +// rendering), so nothing else in the pipeline neutralizes an embedded ESC +// or BEL before it becomes on-screen display text — xansi.Truncate passes +// escapes through unconditionally (that's the whole premise this PR relies +// on for the hyperlink itself to survive truncation) and termenv only +// styles text, it doesn't sanitize it. oscTarget percent-encodes the same +// bytes for the link target, where they need to stay meaningful; the +// visible text just needs them gone, since a board line is written by an +// agent pasting arbitrary tool output and an ESC there would otherwise +// reach the terminal as a real escape sequence — a nested OSC 8 pointing +// somewhere the board never named, or worse. +func stripControlBytes(s string) string { + var b strings.Builder + for i := 0; i < len(s); i++ { + if c := s[i]; c >= 0x20 && c != 0x7f { + b.WriteByte(c) + } + } + return b.String() +} + +// placeholderFind locates every placeholder on a line at once, capturing +// its index — used by restoreBareURLs to size the shared budget when more +// than one bare URL reflowed onto the same physical line. +var placeholderFind = regexp.MustCompile(`\x1fU(\d+)\x1f`) + +// restoreBareURLs swaps each placeholder back for its real URL, wrapped in +// an OSC 8 hyperlink and styled like glamour's own Link (colorLink, +// underlined). The unit of work is the rendered LINE, not the individual +// URL: every placeholder on a line shares one width budget, computed once +// from the line with all placeholders removed and then split evenly across +// however many there are. Processing URLs independently (each measuring +// its budget against a line that already had earlier URLs' full-size +// hyperlinks substituted in) let the first URL on a shared line claim +// nearly the whole width and starve the rest down to a cell or two — width +// invariant intact, but a real bug: two bare URLs as consecutive +// continuation lines of one board item reflow onto a single physical line +// exactly like this. The hyperlink target always carries the full, +// untruncated URL regardless of what its display text elides to. Each URL +// has its own index-keyed placeholder, so two URLs that happen to elide to +// identical visible text can never cross-link — there's nothing to search +// for and collide on. If a URL's share of the line's budget is under 1 +// cell, it's dropped rather than drawn — an over-width line would break +// the one invariant every other line in this renderer holds. +func restoreBareURLs(rendered string, urls []string, width int) string { + if len(urls) == 0 { + return rendered + } + lines := strings.Split(rendered, "\n") + lr, lg, lb := hexToRGB(colorLink) // loop-invariant + for li, line := range lines { + matches := placeholderFind.FindAllStringSubmatchIndex(line, -1) + if matches == nil { + continue + } + + // glamour's MarginWriter pads every short block line with + // trailing spaces out to the full block width (see styleConfig's + // doc comment). Real trailing content on the line only ever needs + // the width AFTER that padding, never the padding itself — left + // uncorrected, the padding reads as content already filling the + // line and starves the elision budget to almost nothing, + // regardless of how much real room there actually is. The + // hyperlink target and the width invariant both stay technically + // correct at any budget, including a useless one, so this needs + // its own check rather than relying on those. Strip the padding + // by measuring the plain-text line with real trailing spaces + // trimmed, then cutting the ANSI-styled line to that same visible + // width — x/ansi.Truncate is escape-aware, so this keeps every + // SGR code and every placeholder intact and only drops the + // trailing filler. + realWidth := visibleWidth(strings.TrimRight(stripANSI(line), " ")) + line = xansi.Truncate(line, realWidth, "") + matches = placeholderFind.FindAllStringSubmatchIndex(line, -1) + if matches == nil { + continue // placeholders are never whitespace; stay safe anyway + } + + available := width - visibleWidth(placeholderFind.ReplaceAllString(line, "")) + share := available / len(matches) + + var out strings.Builder + cursor := 0 + for _, m := range matches { + start, end := m[0], m[1] + out.WriteString(line[cursor:start]) + cursor = end + idx, err := strconv.Atoi(line[m[2]:m[3]]) + if err != nil || idx < 0 || idx >= len(urls) || share < 1 { + continue // drop this URL: no room, or a malformed index + } + url := urls[idx] + // xansi.Truncate, not go-runewidth: it's grapheme-cluster + // aware (a VS16 emoji presentation sequence is one cluster + // but two runes) and it's the same measurement visibleWidth + // uses to enforce the pane-width invariant. Measuring the + // budget with one metric and building the display text with + // a different one is exactly how that invariant would + // quietly break again. + display := xansi.Truncate(stripControlBytes(url), share, "…") + // Raw ANSI, not termenv.String: termenv.String binds to + // termenv's auto-detected package-global Output profile, + // which this codebase deliberately overrides everywhere else + // — the glamour renderer is forced to termenv.TrueColor + // below, and diff.go writes 38;2;r;g;b by hand for the same + // reason ("piped/degraded profiles were how glow washed + // out"). Under a degraded profile termenv.String would + // silently drop the styling and this would be the one link + // on screen that isn't truecolor. + styled := fmt.Sprintf("\x1b[4;38;2;%d;%d;%dm%s\x1b[0m", lr, lg, lb, display) + out.WriteString(hyperlink(url, styled)) + } + out.WriteString(line[cursor:]) + lines[li] = out.String() + } + return strings.Join(lines, "\n") +} + // renderMarkdown renders raw markdown at the given width (already reduced // from the pane width by the caller). Output is post-processed to guarantee // the hard requirements: no trailing-space padding, at most one blank line // between blocks, no leading/trailing blank runs. -func renderMarkdown(raw string, width int) (string, error) { +// +// linkify controls the bare-URL OSC 8 hyperlink path: true for the +// interactive TUI, where the escape sequence is invisible to the terminal +// and only the (possibly elided) display text is shown. false skips +// stashBareURLs entirely — for a non-TTY consumer (runStatic piped to a +// file, grep, a CI log) whose stdout is not a terminal that would render +// the hyperlink at all, an OSC 8 escape it can't see is worse than plain +// text. It isn't a full guarantee, though: a URL long enough to still hit +// glamour's own hard word-wrap on that path renders un-elided but still +// split mid-URL, same as before this fix — this only helps the common +// case, a URL short enough to need no wrap at the non-TTY fallback width. +func renderMarkdown(raw string, width int, linkify bool) (string, error) { if width < 10 { width = 10 } + var urls []string + if linkify { + raw, urls = stashBareURLs(raw) + } r, err := glamour.NewTermRenderer( glamour.WithStyles(styleConfig()), glamour.WithWordWrap(width), @@ -139,6 +462,26 @@ func renderMarkdown(raw string, width int) (string, error) { if err != nil { return "", err } + if len(urls) > 0 { + out = restoreBareURLs(out, urls, width) + // Defensive: if a placeholder somehow didn't survive glamour intact + // (an unanticipated reflow edge case), strip the residual bytes + // rather than let a raw \x1f-delimited token land on screen — the + // URL is lost either way at that point; better a bare, mostly + // inert failure than one carrying our own control bytes. The plain + // \x1f pass after it also catches a placeholder that got hard- + // broken across a wrap (the paired-delimiter regex above can't + // match half a token) — \x1f itself is unambiguously our own byte + // (nothing else in this pipeline emits it), so it's always safe to + // drop. It's not a fully invisible failure in that split case, + // though: only the delimiter bytes are removed, so a naked "U0" + // can be left as plain visible text. Narrowing that further would + // mean matching digits without a \x1f anchor, which risks eating + // real board text (e.g. "U2", "Update") instead — worse than the + // cosmetic residue it would prevent. + out = residualPlaceholder.ReplaceAllString(out, "") + out = strings.ReplaceAll(out, "\x1f", "") + } return tidy(out), nil } @@ -171,7 +514,11 @@ func tidy(s string) string { return strings.Join(out, "\n") } -// visibleWidth is the printable cell width of a line, ignoring ANSI codes. +// visibleWidth is the printable cell width of a line, ignoring ANSI codes — +// SGR and OSC 8 hyperlinks alike. x/ansi's parser understands OSC; the +// muesli/reflow width counter this used to call does not (it only +// recognizes CSI's `[0-9;]*[A-Za-z]` terminator), so an OSC 8-wrapped line +// would otherwise measure with the link target counted as visible text. func visibleWidth(line string) int { - return reflowansi.PrintableRuneWidth(line) + return xansi.StringWidth(line) } diff --git a/ui.go b/ui.go index 0539fb2..4f9485a 100644 --- a/ui.go +++ b/ui.go @@ -283,7 +283,7 @@ func (m *model) reload(force bool) (changed bool) { displayRaw = applyCollapse(raw, board, m.collapsed) } - rendered, err := renderMarkdown(displayRaw, m.renderWidth()) + rendered, err := renderMarkdown(displayRaw, m.renderWidth(), true) if err != nil { m.loadErr = err m.vp.SetContent(fmt.Sprintf("\n Render error: %v", err)) @@ -307,7 +307,7 @@ func (m *model) reload(force bool) (changed bool) { if baseBoard, ok := parseBoard(m.prevBaseline); ok { baseDisplay = applyCollapse(m.prevBaseline, baseBoard, m.collapsed) } - if base, berr := renderMarkdown(baseDisplay, m.renderWidth()); berr == nil { + if base, berr := renderMarkdown(baseDisplay, m.renderWidth(), true); berr == nil { changedMap = changedLines(strings.Split(base, "\n"), lines) } } @@ -351,7 +351,7 @@ func (m *model) rerenderCollapse() { return } displayRaw := applyCollapse(m.raw, m.board, m.collapsed) - rendered, err := renderMarkdown(displayRaw, m.renderWidth()) + rendered, err := renderMarkdown(displayRaw, m.renderWidth(), true) if err != nil { return // m.raw already rendered fine on the last successful reload }