From 93d4941c9fe8ed66c098a952e8e5c4121d42db9f Mon Sep 17 00:00:00 2001 From: Darren Cheng Date: Wed, 24 Jun 2026 00:35:05 -0700 Subject: [PATCH 1/3] Open Link picker: mark PR links in TUI + webapp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pull-request URLs are usually the link the user is hunting for, but in the Open Link picker they look identical to CI links, docs, and references. Mark them so the most actionable link is scannable at a glance. - internal/links: add IsPR(url) (GitHub /pull/, GitLab /merge_requests/, matched by path so GHE / self-hosted GitLab count too) and an IsPR field on Link, stamped by Extract — single source of truth, clients never re-derive. - TUI: both pickers (simple + ctrl+l fuzzy) draw a git-pull-request glyph before PR rows; every row reserves a 2-col gutter so rows stay aligned. - Webapp: Open Link modal renders a purple "PR" badge before PR rows from the server's isPR field (DOM API, no innerHTML). Bump SW_VERSION v61->v62. - Routed through OpenSpec (add-pr-link-indicator), archived with base specs merged. Tests + gotcha notes added. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- context/knowledge/gotchas/misc.md | 2 + context/knowledge/index.md | 2 +- internal/api/handlers_test.go | 4 ++ internal/api/static/index.html | 29 ++++++++-- internal/api/static/sw.js | 2 +- internal/links/links.go | 24 ++++++++- internal/links/links_test.go | 34 ++++++++++++ internal/tui/fuzzylinkpicker.go | 20 ++++--- internal/tui/links.go | 26 ++++++--- internal/tui/links_test.go | 53 +++++++++++++++++++ internal/tui/theme/theme.go | 10 ++++ .../proposal.md | 27 ++++++++++ .../specs/forms-and-modals/spec.md | 25 +++++++++ .../specs/mobile-pwa/spec.md | 22 ++++++++ .../2026-06-24-add-pr-link-indicator/tasks.md | 22 ++++++++ openspec/specs/forms-and-modals/spec.md | 24 +++++++++ openspec/specs/mobile-pwa/spec.md | 21 ++++++++ 17 files changed, 328 insertions(+), 19 deletions(-) create mode 100644 openspec/changes/archive/2026-06-24-add-pr-link-indicator/proposal.md create mode 100644 openspec/changes/archive/2026-06-24-add-pr-link-indicator/specs/forms-and-modals/spec.md create mode 100644 openspec/changes/archive/2026-06-24-add-pr-link-indicator/specs/mobile-pwa/spec.md create mode 100644 openspec/changes/archive/2026-06-24-add-pr-link-indicator/tasks.md diff --git a/context/knowledge/gotchas/misc.md b/context/knowledge/gotchas/misc.md index af4c303f..37253a78 100644 --- a/context/knowledge/gotchas/misc.md +++ b/context/knowledge/gotchas/misc.md @@ -169,6 +169,8 @@ - **SGR sequences must strip to empty; cursor movement must strip to space.** Raw PTY logs contain cursor movement codes (`\x1b[5C`, `\x1b[1B`) that position text on different screen locations. Stripping these to empty merges unrelated text into URLs. `stripANSI` uses `ReplaceAllStringFunc` to distinguish: CSI ending in `m` → empty (preserves mid-URL colors), all else → space (prevents merging). - **`bareLinkRe` must exclude `"`, backtick, `{`, `}`, `<`.** These are never valid in URLs per RFC 3986. Without exclusion, the regex matches through quoted/JSON URL data producing garbage entries in the link picker. - **`hasValidHost` filters in-progress URLs (`https://gi`) by requiring a host with a real TLD.** The regex matches greedily as the agent streams output, so partial typing like `https://gi` would otherwise leak into the picker. Acceptance rule: IP literal, the literal `localhost`, or a DNS name whose final label is ≥2 chars starting with a letter (covers `.co`, `.museum`, and `xn--*` punycode). Don't tighten further with a curated IANA list — keeping the check syntactic avoids shipping (and updating) a 1500-entry blob. +- **`links.IsPR` is the single source of truth for the PR indicator; `Extract` stamps `Link.IsPR` so clients never re-derive it.** Both TUI pickers read `link.IsPR` and the web client reads the JSON `isPR` field — the JS must NOT pattern-match URLs itself, or TUI and web could disagree. `IsPR` matches on the path segment (`/pull/` or `/merge_requests/`), not the host, so it also catches GitHub Enterprise / self-hosted GitLab. +- **Both link pickers reserve a fixed `prLinkGutter` (2-col) left margin on every item row, and the modal-width calc must add it.** The PR glyph is painted into the gutter only for PR rows, but the gutter is reserved for all rows so PR and non-PR links stay column-aligned. If you compute `modalW` from the link display width without adding `prLinkGutter`, the widest PR link truncates a char early. ## Settings UI diff --git a/context/knowledge/index.md b/context/knowledge/index.md index 5b603722..ad00ea65 100644 --- a/context/knowledge/index.md +++ b/context/knowledge/index.md @@ -13,7 +13,7 @@ Non-obvious invariants and gotchas, split by topic. Read the relevant file when | [gotchas/keybindings.md](gotchas/keybindings.md) | Key routing, ctrl sequences, tcell modifier quirks, agent view nav, plugin-view full surrender + double-Ctrl+Q failsafe, plugin help overlay, keyenc single-source + mod-7 Cmd+arrow round-trip, control-frame read-pump threading, plugin resize-envelope reconciliation, ctrl+r Claude session switcher, ctrl+k task switcher (grouped folder mode), plugin-view reconnect (backoff, laidOut survives resume) | 35 | | [gotchas/tasklist-ui.md](gotchas/tasklist-ui.md) | Task list cursor, modals, focus guards, filter, archive, pinned, spinner, row rendering, form done-flag reset, needs-input icon, agent attention bar, sticky needs-input across idle gate, ❯+NBSP prompt anchor, decoration-line skip, PR indicator cell, filter-picker narrow-terminal width clamp (shared taskswitcher/sessionpicker/fuzzylinkpicker), shared TaskStatusIcon classifier (task list + switcher) | 53 | | [gotchas/web-remote.md](gotchas/web-remote.md) | SPA + REST API + service worker, Tailscale-only binding, EventSource auth, xterm.js, HTML escaping, virtual keys, detail/files views, prompt modal, input history, link picker, new-task defaults, file uploads, offline view, Web Push, per-device tokens, settings endpoints, share target + iOS Shortcut, test harness, idle status, daemon-side status flip on exit, static-output replay, mobile compose + key bar, skill autocomplete, task-list search, diff wrap + stacked panels, full-history scrollback, global hotkeys, session artifacts (manifest-scoped serving, X-Frame-Options, blob+sandbox isolation), PR badge from cached pr_state, compose split text→CR send, artifact Open overlay vs paneled, Hera tab (read-only /api/hera roster), Settings System panel (sysmetrics cached snapshot, visible-only poller, *_avail "—" fallback, collector started in ListenAndServe) | 191 | -| [gotchas/misc.md](gotchas/misc.md) | DB patterns, Go idioms, Codex, Pi/ollama prelaunch, MCP, PRs, file explorer, quick-add, scheduled tasks, links, task auto-naming, settings two-pane UI, plugin-scoped token tagging, task_meta sidecar, plugin settings registry, Claude session discovery (folder encoding, JSONL parse), config.toml override layer (precedence, partial-map zeroing, mtime live-reload), hera plugin contract (POST/DELETE /notify), model selection (--model injection, codex-resume re-append), per-schedule model override (TaskCreator taskModel → HeadlessInput.Model), wall-clock duration clamp, openspec archive workflow (rc0-on-abort, authoring-order fold, orphaned-superseded-requirement audit, authoring-drift fixes) | 148 | +| [gotchas/misc.md](gotchas/misc.md) | DB patterns, Go idioms, Codex, Pi/ollama prelaunch, MCP, PRs, file explorer, quick-add, scheduled tasks, links, task auto-naming, settings two-pane UI, plugin-scoped token tagging, task_meta sidecar, plugin settings registry, Claude session discovery (folder encoding, JSONL parse), config.toml override layer (precedence, partial-map zeroing, mtime live-reload), hera plugin contract (POST/DELETE /notify), model selection (--model injection, codex-resume re-append), per-schedule model override (TaskCreator taskModel → HeadlessInput.Model), wall-clock duration clamp, link PR indicator (IsPR single-source + prLinkGutter), openspec archive workflow (rc0-on-abort, authoring-order fold, orphaned-superseded-requirement audit, authoring-drift fixes) | 150 | | [gotchas/orchestration.md](gotchas/orchestration.md) | depends_on DAG / depswatcher / link-unlink-halt / plan_slug all RETIRED (Hera is the single model; base_branch kept; tasks start immediately), task_set_result opaque-to-daemon, ARGUS_TASK_ID env export, (name,project) idempotency, schema column ordering, hera schema/store M1 (FK-cascade, NULL partial-index, per-(task,orchestrator) multi-binding + ErrHeraAmbiguous, transactional role+binding), hera M4 (born-bound spawn + AfterPersist hook; auto-adopt removed; ReconcileBindings keyed on task-row), hera M5 (subtree TLDR roll-up: SubtreeOrchIDs BFS + cycle guard, tree_read_cursors cascade, advisory per-role cursor), hera plan-DAG substrate (planned node = role w/ no binding, gate on role-status done, failed-blocker → HOLD + ping coordinator, in-tx DFS cycle check, check-in pulled via inbox) | 41 | | [gotchas/dag-rendering.md](gotchas/dag-rendering.md) | dagview is now a LAYOUT LIBRARY (dagview.Compute longest-path stage placement) consumed by internal/tui/planview — standalone widget no longer mounted; PLAN-DAG widget (planview) replaced the retired orchestration-tree graph, heraTreeNodes/tree.go DELETED. Sugiyama-lite layer math, rune-vs-byte truncation, single-line edges, branch-change log-only. planview node State/colour from RoleView.TaskStatus/TaskResult ({"failed":true}→red ✕); planview surfaces OnEnter/OnDrillIn/OnDrillOut/OnBranchChange (read-only nav, no edit callbacks); branchShape folds cursor+fanned-group+orch-title | 18 | | [gotchas/hera-view.md](gotchas/hera-view.md) | Native Hera view (`internal/tui/hera`): 2nd tab always native (cfg.Hera.Enabled gates only daemon MCP tools, not tick refresh); structural multi-binding fan-out; freelance section; ready_to_close from task_meta; goroutine-free debounced Refresher on UI thread; rail nav j/k/Up/Down + Space (tab nav 1/2/3 only); no Sync + full-rect coverage. Panes fed from in-process runner ring (poll, not SSE); coord-vs-agent session rule; multi-binding disambiguated by role.OrchID; PTY align via ForceResyncPTY + off-thread SyncPanes. Thin mutation layer (ops.go over M1; shared agent.SpawnHeraWorker); rail keyset via OnXxx callbacks; multi-binding isolation; s/S step hera ROLE status; modal.ConfirmModal + NewInputForm; remote=nil ⇒ inert. Details 2 modes (worker→terminal, coordinator→stacked roster-over-PLAN — same geometry as roster-over-tree); embedded PLAN-DAG graph via heraPlanNodesWithBridge(orch, bridgeIndex) — coordinators not plan nodes, Drillable needs WithBridge form; planview↔hera import one-way (hera imports planview); OnDrillIn page-owned (drillIntoChild→bridgeIndex→PushOrch), OnEnter App-owned; handleDetailsKey Esc-at-root escapes pane; node colour from TaskStatus/TaskResult. Ctrl+Z→fullscreen (closes SIGTSTP footgun); Enter revives dead (startSession) or suspended worker (reviveHeraWorker→KickRerender, idle+not-blocked gated; live coordinator navigate-only); jumpToLeaf expands ancestor coordinators (EnsureAncestorsExpanded via canonicalParents + OrchIDsForTask) before SelectByTaskID so a folded coord doesn't swallow the join (BUG-007) | 39 | diff --git a/internal/api/handlers_test.go b/internal/api/handlers_test.go index f45f033f..71a0d550 100644 --- a/internal/api/handlers_test.go +++ b/internal/api/handlers_test.go @@ -734,12 +734,16 @@ func TestHandleGetLinks(t *testing.T) { Links []struct { Label string `json:"label"` URL string `json:"url"` + IsPR bool `json:"isPR"` } `json:"links"` } testutil.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) testutil.Equal(t, len(resp.Links), 2) testutil.Equal(t, resp.Links[0].URL, "https://example.com/page") + testutil.Equal(t, resp.Links[0].IsPR, false) testutil.Equal(t, resp.Links[1].URL, "https://github.com/org/repo/pull/42") + // PR URLs are flagged so the web client can mark them without re-deriving. + testutil.Equal(t, resp.Links[1].IsPR, true) }) } diff --git a/internal/api/static/index.html b/internal/api/static/index.html index b7b39ffa..7bf8d5db 100644 --- a/internal/api/static/index.html +++ b/internal/api/static/index.html @@ -851,13 +851,23 @@ } .links-empty { padding: 12px; color: var(--text-dim); font-size: 13px; text-align: center; } .links-item { - display: flex; flex-direction: column; gap: 2px; + display: flex; flex-direction: row; align-items: center; gap: 8px; width: 100%; padding: 8px 10px; min-height: 44px; background: transparent; border: 0; border-radius: 0; color: var(--text); text-align: left; cursor: pointer; font: inherit; } .links-item:hover, .links-item:focus { background: var(--surface); outline: none; } +/* Label/URL stack. min-width:0 lets the children's ellipsis kick in inside + the flex row instead of overflowing it. */ +.links-item .links-content { display: flex; flex-direction: column; gap: 2px; flex: 1; min-width: 0; } +/* PR indicator pill. Purple matches the TUI's StylePRLink (ColorPRAwaiting) + and the task list's awaiting-review badge, so "this link is a PR" reads + consistently across surfaces. */ +.links-item .links-pr-badge { + flex: none; font-size: 11px; font-weight: 600; padding: 2px 7px; + border-radius: 10px; background: #251b3a; color: #b294fa; white-space: nowrap; +} .links-item .links-label { font-size: 13px; line-height: 1.3; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; @@ -3931,17 +3941,30 @@

Keyboard shortcuts

const url = l.url || ''; const label = l.label || url; if (label === url) btn.classList.add('url-only'); + // PR indicator. l.isPR is server-classified by the shared links package + // (single source of truth), so the client never re-derives it. Built via + // the DOM API — never innerHTML — like everything else in this row. + if (l.isPR) { + const badge = document.createElement('span'); + badge.className = 'links-pr-badge'; + badge.textContent = 'PR'; + badge.title = 'Pull request'; + btn.appendChild(badge); + } // textContent (not innerHTML) is required — labels and URLs are // attacker-controlled (they came out of the agent's stdout). textContent // escapes HTML, so a label of `` renders as text. + const content = document.createElement('div'); + content.className = 'links-content'; const labelEl = document.createElement('span'); labelEl.className = 'links-label'; labelEl.textContent = label; const urlEl = document.createElement('span'); urlEl.className = 'links-url'; urlEl.textContent = url; - btn.appendChild(labelEl); - btn.appendChild(urlEl); + content.appendChild(labelEl); + content.appendChild(urlEl); + btn.appendChild(content); btn.addEventListener('click', () => { closeLinksModal(); openExternalURL(url); diff --git a/internal/api/static/sw.js b/internal/api/static/sw.js index 54ea3a5b..705b7e71 100644 --- a/internal/api/static/sw.js +++ b/internal/api/static/sw.js @@ -7,7 +7,7 @@ // // Increment SW_VERSION on any shell change to bust the cache. -const SW_VERSION = 'argus-shell-v62'; +const SW_VERSION = 'argus-shell-v63'; const SHELL_ASSETS = [ '/', '/manifest.webmanifest', diff --git a/internal/links/links.go b/internal/links/links.go index 70347ed6..afbe4f17 100644 --- a/internal/links/links.go +++ b/internal/links/links.go @@ -16,6 +16,26 @@ import ( type Link struct { Label string `json:"label"` // markdown link text, or the URL itself for bare URLs URL string `json:"url"` + IsPR bool `json:"isPR"` // URL points at a GitHub PR / GitLab merge request +} + +// prPathRe matches the path of a pull-request / merge-request URL on a +// code-forge host: GitHub's "///pull/" or GitLab's +// "///-/merge_requests/". Matching on the path segment (rather +// than the host) means it also recognizes GitHub Enterprise and self-hosted +// GitLab instances. The trailing "(?:/|$)" keeps "/pull/123" and +// "/pull/123/files" matching while a stray "/pull/abc" does not. +var prPathRe = regexp.MustCompile(`/(?:pull|merge_requests)/\d+(?:/|$)`) + +// IsPR reports whether the URL points at a pull request or merge request. It is +// the single source of truth for the PR indicator shown by the TUI link pickers +// and the web client's Open Link modal. +func IsPR(raw string) bool { + u, err := url.Parse(raw) + if err != nil || u.Host == "" { + return false + } + return prPathRe.MatchString(u.Path) } // mdLinkRe matches markdown links: [text](url) @@ -160,7 +180,7 @@ func Extract(content string) []Link { continue } seen[u] = true - out = append(out, Link{Label: m[1], URL: u}) + out = append(out, Link{Label: m[1], URL: u, IsPR: IsPR(u)}) } // Second pass: bare URLs not already captured @@ -173,7 +193,7 @@ func Extract(content string) []Link { continue } seen[u] = true - out = append(out, Link{Label: u, URL: u}) + out = append(out, Link{Label: u, URL: u, IsPR: IsPR(u)}) } return out diff --git a/internal/links/links_test.go b/internal/links/links_test.go index 8654a9cd..7b20064f 100644 --- a/internal/links/links_test.go +++ b/internal/links/links_test.go @@ -246,3 +246,37 @@ func TestExtract(t *testing.T) { }) } } + +func TestIsPR(t *testing.T) { + tests := []struct { + name string + url string + want bool + }{ + {"github pr", "https://github.com/org/repo/pull/123", true}, + {"github pr with files suffix", "https://github.com/org/repo/pull/123/files", true}, + {"github enterprise pr", "https://github.example.com/org/repo/pull/9", true}, + {"gitlab merge request", "https://gitlab.com/group/repo/-/merge_requests/42", true}, + {"github repo root not a pr", "https://github.com/org/repo", false}, + {"github issue not a pr", "https://github.com/org/repo/issues/5", false}, + {"github compare range not a pr", "https://github.com/org/repo/compare/v1.0...v1.1", false}, + {"pull without number not a pr", "https://github.com/org/repo/pull/abc", false}, + {"unrelated url", "https://example.com/page", false}, + {"empty", "", false}, + {"no host", "https:///pull/1", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testutil.Equal(t, IsPR(tt.url), tt.want) + }) + } +} + +func TestExtractStampsIsPR(t *testing.T) { + got := Extract("PR: https://github.com/org/repo/pull/7 and docs https://example.com/guide") + testutil.Equal(t, len(got), 2) + testutil.Equal(t, got[0].URL, "https://github.com/org/repo/pull/7") + testutil.Equal(t, got[0].IsPR, true) + testutil.Equal(t, got[1].URL, "https://example.com/guide") + testutil.Equal(t, got[1].IsPR, false) +} diff --git a/internal/tui/fuzzylinkpicker.go b/internal/tui/fuzzylinkpicker.go index 34e920ea..d725f511 100644 --- a/internal/tui/fuzzylinkpicker.go +++ b/internal/tui/fuzzylinkpicker.go @@ -196,8 +196,9 @@ func (m *FuzzyLinkPickerModal) Draw(screen tcell.Screen) { } } - // modal width: label + padding + border - modalW := max(maxDisplayW+6, 40) + // modal width: label + padding + border + the PR-indicator gutter every + // item row reserves (so the widest link still fits beside the glyph). + modalW := max(maxDisplayW+6+prLinkGutter, 40) modalW = min(modalW, width-4) innerW := modalW - 4 @@ -284,10 +285,14 @@ func (m *FuzzyLinkPickerModal) Draw(screen tcell.Screen) { display = link.Label + " " + link.URL } - if utf8.RuneCountInString(display) > innerW { + // Every row reserves a left gutter so PR and non-PR links stay + // column-aligned; PR rows fill it with the indicator glyph. + textX := innerX + prLinkGutter + textW := innerW - prLinkGutter + if utf8.RuneCountInString(display) > textW { runes := []rune(display) - if innerW > 3 { - display = string(runes[:innerW-1]) + "…" + if textW > 3 { + display = string(runes[:textW-1]) + "…" } } @@ -295,7 +300,10 @@ func (m *FuzzyLinkPickerModal) Draw(screen tcell.Screen) { if isCursor { style = theme.StyleSelected } - widget.DrawText(screen, innerX, itemsY+i, innerW, display, style) + if link.IsPR { + screen.SetContent(innerX, itemsY+i, theme.IconPRLink, nil, theme.StylePRLink) + } + widget.DrawText(screen, textX, itemsY+i, textW, display, style) } } diff --git a/internal/tui/links.go b/internal/tui/links.go index 172a987b..22ddeca3 100644 --- a/internal/tui/links.go +++ b/internal/tui/links.go @@ -46,6 +46,12 @@ func openURL(url string) { uxlog.Log("[links] opened URL in browser: %s", url) } +// prLinkGutter is the number of columns every link-picker row reserves at its +// left edge for the PR indicator glyph (theme.IconPRLink). The glyph occupies +// one cell; the extra column is breathing room before the link text. Non-PR +// rows leave the gutter blank so PR and non-PR links stay column-aligned. +const prLinkGutter = 2 + // --------------------------------------------------------------------------- // LinkPickerModal — selection dialog for multiple links // --------------------------------------------------------------------------- @@ -135,8 +141,9 @@ func (m *LinkPickerModal) Draw(screen tcell.Screen) { } } - // modal width: label + padding + border - modalW := max(maxLabelW+6, 30) + // modal width: label + padding + border + the PR-indicator gutter every + // item row reserves. + modalW := max(maxLabelW+6+prLinkGutter, 30) modalW = min(modalW, width-4) innerW := modalW - 4 @@ -185,12 +192,16 @@ func (m *LinkPickerModal) Draw(screen tcell.Screen) { link := m.links[idx] isCursor := idx == m.cursor + // Every row reserves a left gutter so PR and non-PR links stay + // column-aligned; PR rows fill it with the indicator glyph. + textX := innerX + prLinkGutter + textW := innerW - prLinkGutter label := link.Label - if utf8.RuneCountInString(label) > innerW { + if utf8.RuneCountInString(label) > textW { // Truncate runes := []rune(label) - if innerW > 3 { - label = string(runes[:innerW-1]) + "…" + if textW > 3 { + label = string(runes[:textW-1]) + "…" } } @@ -198,7 +209,10 @@ func (m *LinkPickerModal) Draw(screen tcell.Screen) { if isCursor { style = theme.StyleSelected } - widget.DrawText(screen, innerX, row+i, innerW, label, style) + if link.IsPR { + screen.SetContent(innerX, row+i, theme.IconPRLink, nil, theme.StylePRLink) + } + widget.DrawText(screen, textX, row+i, textW, label, style) } // Help text diff --git a/internal/tui/links_test.go b/internal/tui/links_test.go index 0fe77f42..d4c9d2e8 100644 --- a/internal/tui/links_test.go +++ b/internal/tui/links_test.go @@ -1,6 +1,7 @@ package tui import ( + "slices" "strings" "testing" @@ -8,8 +9,22 @@ import ( "github.com/rivo/tview" "github.com/drn/argus/internal/testutil" + "github.com/drn/argus/internal/tui/theme" ) +// screenHasRune reports whether any cell on the simulation screen renders r. +// Used to assert the PR indicator glyph is (or isn't) painted by a picker. +// Callers must Show() the screen after Draw so the front buffer is populated. +func screenHasRune(sim tcell.SimulationScreen, r rune) bool { + cells, w, h := sim.GetContents() + for i := 0; i < w*h; i++ { + if slices.Contains(cells[i].Runes, r) { + return true + } + } + return false +} + // Pure ExtractLinks coverage lives in internal/links/links_test.go. This file // keeps tests for the TUI-specific picker modal and openURL helper. @@ -187,3 +202,41 @@ func TestLinkPickerModal_SelectedLink_OutOfBounds(t *testing.T) { link := m.SelectedLink() testutil.Equal(t, link.URL, "") } + +func TestLinkPickerModal_Draw_PRIndicator(t *testing.T) { + t.Run("PR link draws the glyph", func(t *testing.T) { + m := NewLinkPickerModal([]Link{{Label: "PR", URL: "https://github.com/o/r/pull/1", IsPR: true}}) + m.SetRect(0, 0, 80, 24) + sim := drawSim(t) + m.Draw(sim) + sim.Show() + testutil.Equal(t, screenHasRune(sim, theme.IconPRLink), true) + }) + t.Run("non-PR link omits the glyph", func(t *testing.T) { + m := NewLinkPickerModal([]Link{{Label: "Docs", URL: "https://example.com/guide"}}) + m.SetRect(0, 0, 80, 24) + sim := drawSim(t) + m.Draw(sim) + sim.Show() + testutil.Equal(t, screenHasRune(sim, theme.IconPRLink), false) + }) +} + +func TestFuzzyLinkPickerModal_Draw_PRIndicator(t *testing.T) { + t.Run("PR link draws the glyph", func(t *testing.T) { + m := NewFuzzyLinkPickerModal([]Link{{Label: "PR", URL: "https://github.com/o/r/pull/1", IsPR: true}}) + m.SetRect(0, 0, 80, 24) + sim := drawSim(t) + m.Draw(sim) + sim.Show() + testutil.Equal(t, screenHasRune(sim, theme.IconPRLink), true) + }) + t.Run("non-PR link omits the glyph", func(t *testing.T) { + m := NewFuzzyLinkPickerModal([]Link{{Label: "Docs", URL: "https://example.com/guide"}}) + m.SetRect(0, 0, 80, 24) + sim := drawSim(t) + m.Draw(sim) + sim.Show() + testutil.Equal(t, screenHasRune(sim, theme.IconPRLink), false) + }) +} diff --git a/internal/tui/theme/theme.go b/internal/tui/theme/theme.go index 2acc82eb..be64297b 100644 --- a/internal/tui/theme/theme.go +++ b/internal/tui/theme/theme.go @@ -52,6 +52,11 @@ const ( IconPRChanges = rune(0xF09D8) // 󰧘 nf-md-source_pull (changes requested overlay) IconPRApproved = rune(0xF0DDF) // 󰷟 nf-md-source_branch_check (approved overlay) + // IconPRLink marks a pull-request / merge-request URL in the Open Link + // pickers. Reuses the neutral git-pull-request glyph (no review-state + // overlay) so it reads as "this link is a PR" without implying a state. + IconPRLink = rune(0xF407) // nf-oct-git_pull_request + // IconCoordinator marks a task that holds a Hera coordinator role // (meta:hera.role=coordinator). Shares the orchestrator glyph used by the // native Hera rail so the two surfaces read consistently. @@ -82,6 +87,11 @@ var ( StylePRChanges = tcell.StyleDefault.Foreground(ColorPRChanges).Bold(true) StylePRApproved = tcell.StyleDefault.Foreground(ColorPRApproved).Bold(true) + // StylePRLink colors the PR indicator in the Open Link pickers. Purple is + // the app's PR family color, so a PR link reads as a PR at a glance without + // implying any particular review state. + StylePRLink = tcell.StyleDefault.Foreground(ColorPRAwaiting).Bold(true) + // StyleCoordinator renders the task-row coordinator indicator. Cyan (the // project color) reads as "structural/orchestration" rather than a status. StyleCoordinator = tcell.StyleDefault.Foreground(ColorProject).Bold(true) diff --git a/openspec/changes/archive/2026-06-24-add-pr-link-indicator/proposal.md b/openspec/changes/archive/2026-06-24-add-pr-link-indicator/proposal.md new file mode 100644 index 00000000..fb5c0c3a --- /dev/null +++ b/openspec/changes/archive/2026-06-24-add-pr-link-indicator/proposal.md @@ -0,0 +1,27 @@ +# Add a PR indicator to the Open Link picker (TUI + webapp) + +## Why + +The Open Link picker lists every http/https URL an agent emitted, flat and +undifferentiated. Pull-request URLs are usually the link a user is hunting for, +but they look identical to CI links, docs, and random references. Marking PR +links makes the most actionable link scannable at a glance. + +## What Changes + +- Add a shared `links.IsPR(url)` classifier and an `IsPR` field on + `links.Link`, populated by `links.Extract`, so both the TUI and the REST API + expose the same PR judgement (single source of truth — JS never re-derives it). +- TUI: both link pickers (the simple `LinkPickerModal` and the agent-view + `FuzzyLinkPickerModal`) prepend a git-pull-request glyph before PR rows. +- Webapp: the Open Link modal prepends a small "PR" badge before PR rows. + +No keybindings change. The picker's selection/filter behavior is unchanged. + +## Impact + +- Specs: `forms-and-modals` (TUI pickers + shared classification), + `mobile-pwa` (webapp Open Link list). +- Code: `internal/links`, `internal/tui/links.go`, + `internal/tui/fuzzylinkpicker.go`, `internal/tui/theme/theme.go`, + `internal/api/static/index.html` (+ `SW_VERSION` bump). diff --git a/openspec/changes/archive/2026-06-24-add-pr-link-indicator/specs/forms-and-modals/spec.md b/openspec/changes/archive/2026-06-24-add-pr-link-indicator/specs/forms-and-modals/spec.md new file mode 100644 index 00000000..8267770d --- /dev/null +++ b/openspec/changes/archive/2026-06-24-add-pr-link-indicator/specs/forms-and-modals/spec.md @@ -0,0 +1,25 @@ +## ADDED Requirements + +### Requirement: Pull-request links are visually marked in the link pickers + +Both TUI link pickers (the simple link picker and the fuzzy link picker) SHALL render a distinct pull-request indicator glyph before any link row whose URL points at a pull request or merge request, and SHALL render no such glyph for any other link. Whether a URL is a pull request SHALL be decided by the +shared `links` classifier (a GitHub `///pull/` path or a GitLab +`/merge_requests/` path), so the TUI and the web client agree. The indicator +SHALL be purely decorative: it SHALL NOT change which links match the filter, +SHALL NOT be part of the value returned on selection, and SHALL NOT consume the +URL/label text that the row already displays. + +#### Scenario: PR link shows the indicator + +- **WHEN** the link list contains a URL whose path is a GitHub pull request or GitLab merge request +- **THEN** that row SHALL be drawn with the pull-request indicator glyph preceding its text + +#### Scenario: Non-PR link shows no indicator + +- **WHEN** the link list contains a URL that is not a pull request (e.g. a docs page, a CI build, or a GitHub compare range) +- **THEN** that row SHALL be drawn without the pull-request indicator glyph + +#### Scenario: Indicator does not affect selection or filtering + +- **WHEN** the user filters or selects a PR-marked link +- **THEN** filtering SHALL match against the URL and label only, and the selected value SHALL be the link's URL and label, unaffected by the indicator diff --git a/openspec/changes/archive/2026-06-24-add-pr-link-indicator/specs/mobile-pwa/spec.md b/openspec/changes/archive/2026-06-24-add-pr-link-indicator/specs/mobile-pwa/spec.md new file mode 100644 index 00000000..edcc4c31 --- /dev/null +++ b/openspec/changes/archive/2026-06-24-add-pr-link-indicator/specs/mobile-pwa/spec.md @@ -0,0 +1,22 @@ +## ADDED Requirements + +### Requirement: Open Link modal marks pull-request links + +The web client's Open Link modal SHALL render a "PR" badge before any link row +whose URL is a pull request or merge request, and SHALL render no badge for any +other link. The PR judgement SHALL come from the `isPR` field on each link +returned by `GET /api/tasks/{id}/links` (server-classified by the shared `links` +package), so the client does not re-derive it. The badge SHALL be inserted using +the DOM API as a separate element, never via `innerHTML`, preserving the +existing invariant that agent-controlled label/URL text is only ever set through +`textContent`. + +#### Scenario: PR link shows a badge + +- **WHEN** the links payload contains a link with `isPR: true` +- **THEN** its row in the Open Link modal SHALL show a "PR" badge before the link text + +#### Scenario: Non-PR link shows no badge + +- **WHEN** the links payload contains a link with a falsy `isPR` +- **THEN** its row SHALL show no PR badge diff --git a/openspec/changes/archive/2026-06-24-add-pr-link-indicator/tasks.md b/openspec/changes/archive/2026-06-24-add-pr-link-indicator/tasks.md new file mode 100644 index 00000000..cc64a803 --- /dev/null +++ b/openspec/changes/archive/2026-06-24-add-pr-link-indicator/tasks.md @@ -0,0 +1,22 @@ +# Tasks + +## 1. Shared classification +- [x] 1.1 Add `IsPR(url string) bool` to `internal/links` (GitHub `/pull/`, GitLab `/merge_requests/`). +- [x] 1.2 Add `IsPR bool` (`json:"isPR"`) to `links.Link`; populate it in `Extract`. +- [x] 1.3 Unit-test `IsPR` (PR vs non-PR vs compare-range vs malformed) and that `Extract` stamps the field. + +## 2. TUI rendering +- [x] 2.1 Add an `IconPRLink` glyph + `StylePRLink` style to `theme`. +- [x] 2.2 Render the glyph before PR rows in `LinkPickerModal.Draw`. +- [x] 2.3 Render the glyph before PR rows in `FuzzyLinkPickerModal.Draw`. +- [x] 2.4 Tests assert the marker is drawn for PR rows and absent for non-PR rows. + +## 3. Webapp rendering +- [x] 3.1 Prepend a "PR" badge before PR rows in `renderLinksList` (DOM API, no innerHTML). +- [x] 3.2 Add `.links-pr-badge` CSS. +- [x] 3.3 Bump `SW_VERSION` in `sw.js`. + +## 4. Docs + gate +- [x] 4.1 Add gotcha note(s) where the invariant lives. +- [x] 4.2 `make pre-pr` green. +- [x] 4.3 Archive this change folder into `openspec/changes/archive/` in the same PR. diff --git a/openspec/specs/forms-and-modals/spec.md b/openspec/specs/forms-and-modals/spec.md index 611c74cb..a34546b9 100644 --- a/openspec/specs/forms-and-modals/spec.md +++ b/openspec/specs/forms-and-modals/spec.md @@ -352,3 +352,27 @@ The copy-choice modal SHALL present a list of clipboard targets for a task and r - **WHEN** the user presses Esc - **THEN** the modal reports canceled and no choice selected +### Requirement: Pull-request links are visually marked in the link pickers + +Both TUI link pickers (the simple link picker and the fuzzy link picker) SHALL render a distinct pull-request indicator glyph before any link row whose URL points at a pull request or merge request, and SHALL render no such glyph for any other link. Whether a URL is a pull request SHALL be decided by the +shared `links` classifier (a GitHub `///pull/` path or a GitLab +`/merge_requests/` path), so the TUI and the web client agree. The indicator +SHALL be purely decorative: it SHALL NOT change which links match the filter, +SHALL NOT be part of the value returned on selection, and SHALL NOT consume the +URL/label text that the row already displays. + +#### Scenario: PR link shows the indicator + +- **WHEN** the link list contains a URL whose path is a GitHub pull request or GitLab merge request +- **THEN** that row SHALL be drawn with the pull-request indicator glyph preceding its text + +#### Scenario: Non-PR link shows no indicator + +- **WHEN** the link list contains a URL that is not a pull request (e.g. a docs page, a CI build, or a GitHub compare range) +- **THEN** that row SHALL be drawn without the pull-request indicator glyph + +#### Scenario: Indicator does not affect selection or filtering + +- **WHEN** the user filters or selects a PR-marked link +- **THEN** filtering SHALL match against the URL and label only, and the selected value SHALL be the link's URL and label, unaffected by the indicator + diff --git a/openspec/specs/mobile-pwa/spec.md b/openspec/specs/mobile-pwa/spec.md index e4b7f817..2407c0fe 100644 --- a/openspec/specs/mobile-pwa/spec.md +++ b/openspec/specs/mobile-pwa/spec.md @@ -260,3 +260,24 @@ misleading zero. - **WHEN** a metric is reported unavailable by the API - **THEN** that row renders a placeholder instead of a numeric value +### Requirement: Open Link modal marks pull-request links + +The web client's Open Link modal SHALL render a "PR" badge before any link row +whose URL is a pull request or merge request, and SHALL render no badge for any +other link. The PR judgement SHALL come from the `isPR` field on each link +returned by `GET /api/tasks/{id}/links` (server-classified by the shared `links` +package), so the client does not re-derive it. The badge SHALL be inserted using +the DOM API as a separate element, never via `innerHTML`, preserving the +existing invariant that agent-controlled label/URL text is only ever set through +`textContent`. + +#### Scenario: PR link shows a badge + +- **WHEN** the links payload contains a link with `isPR: true` +- **THEN** its row in the Open Link modal SHALL show a "PR" badge before the link text + +#### Scenario: Non-PR link shows no badge + +- **WHEN** the links payload contains a link with a falsy `isPR` +- **THEN** its row SHALL show no PR badge + From 3c239d6bc3739283a1a4e1b8689cea81d8fd67e3 Mon Sep 17 00:00:00 2001 From: Darren Cheng Date: Wed, 24 Jun 2026 13:54:41 -0700 Subject: [PATCH 2/3] Address review: structural StylePRLink alias, clamp-rationale comments, honest IsPR test table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - theme: StylePRLink = StylePRAwaiting (structural identity, not prose) so a future restyle of the PR purple moves both surfaces together. - pickers: comment why textW needs no clamp on narrow terminals (DrawText early-returns on non-positive width, SetContent clips) — closes the asymmetry with the filter row's fieldW clamp. - links_test: assert IsPR in the TestExtract loop and stamp IsPR:true on the PR-URL want entries so the table matches real output. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/links/links_test.go | 9 +++++---- internal/tui/fuzzylinkpicker.go | 3 +++ internal/tui/links.go | 4 +++- internal/tui/theme/theme.go | 6 ++++-- 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/internal/links/links_test.go b/internal/links/links_test.go index 7b20064f..28ff13ee 100644 --- a/internal/links/links_test.go +++ b/internal/links/links_test.go @@ -61,7 +61,7 @@ func TestExtract(t *testing.T) { { name: "github PR URL", content: "PR: https://github.com/org/repo/pull/123", - want: []Link{{Label: "https://github.com/org/repo/pull/123", URL: "https://github.com/org/repo/pull/123"}}, + want: []Link{{Label: "https://github.com/org/repo/pull/123", URL: "https://github.com/org/repo/pull/123", IsPR: true}}, }, { name: "URL with ANSI escape sequences", @@ -105,7 +105,7 @@ func TestExtract(t *testing.T) { name: "multiple OSC 8 hyperlinks", content: "\x1b]8;;https://github.com/org/repo/pull/1\x1b\\PR #1\x1b]8;;\x1b\\ and \x1b]8;;https://circleci.com/gh/org/repo/99\x1b\\CI\x1b]8;;\x1b\\", want: []Link{ - {Label: "https://github.com/org/repo/pull/1", URL: "https://github.com/org/repo/pull/1"}, + {Label: "https://github.com/org/repo/pull/1", URL: "https://github.com/org/repo/pull/1", IsPR: true}, {Label: "https://circleci.com/gh/org/repo/99", URL: "https://circleci.com/gh/org/repo/99"}, }, }, @@ -117,12 +117,12 @@ func TestExtract(t *testing.T) { { name: "cursor movement prevents text merging", content: "https://github.com/org/repo/pull/123\x1b[5C\x1b[1Bpublished", - want: []Link{{Label: "https://github.com/org/repo/pull/123", URL: "https://github.com/org/repo/pull/123"}}, + want: []Link{{Label: "https://github.com/org/repo/pull/123", URL: "https://github.com/org/repo/pull/123", IsPR: true}}, }, { name: "quoted URL stops at double quote", content: `"https://github.com/org/repo/pull/123",URL,"https://github.com/org/repo/pull/123")`, - want: []Link{{Label: "https://github.com/org/repo/pull/123", URL: "https://github.com/org/repo/pull/123"}}, + want: []Link{{Label: "https://github.com/org/repo/pull/123", URL: "https://github.com/org/repo/pull/123", IsPR: true}}, }, { name: "backtick-wrapped URL cleaned", @@ -242,6 +242,7 @@ func TestExtract(t *testing.T) { for i := range tt.want { testutil.Equal(t, got[i].Label, tt.want[i].Label) testutil.Equal(t, got[i].URL, tt.want[i].URL) + testutil.Equal(t, got[i].IsPR, tt.want[i].IsPR) } }) } diff --git a/internal/tui/fuzzylinkpicker.go b/internal/tui/fuzzylinkpicker.go index d725f511..09508fc5 100644 --- a/internal/tui/fuzzylinkpicker.go +++ b/internal/tui/fuzzylinkpicker.go @@ -287,6 +287,9 @@ func (m *FuzzyLinkPickerModal) Draw(screen tcell.Screen) { // Every row reserves a left gutter so PR and non-PR links stay // column-aligned; PR rows fill it with the indicator glyph. + // textW may go ≤ 0 on a very narrow terminal; unlike the filter + // row's fieldW it needs no clamp — DrawText returns early for a + // non-positive width and SetContent clips offscreen cols in tcell. textX := innerX + prLinkGutter textW := innerW - prLinkGutter if utf8.RuneCountInString(display) > textW { diff --git a/internal/tui/links.go b/internal/tui/links.go index 22ddeca3..3f32a32f 100644 --- a/internal/tui/links.go +++ b/internal/tui/links.go @@ -193,7 +193,9 @@ func (m *LinkPickerModal) Draw(screen tcell.Screen) { isCursor := idx == m.cursor // Every row reserves a left gutter so PR and non-PR links stay - // column-aligned; PR rows fill it with the indicator glyph. + // column-aligned; PR rows fill it with the indicator glyph. textW may + // go ≤ 0 on a very narrow terminal but needs no clamp — DrawText + // returns early for a non-positive width and SetContent clips in tcell. textX := innerX + prLinkGutter textW := innerW - prLinkGutter label := link.Label diff --git a/internal/tui/theme/theme.go b/internal/tui/theme/theme.go index be64297b..670772d5 100644 --- a/internal/tui/theme/theme.go +++ b/internal/tui/theme/theme.go @@ -89,8 +89,10 @@ var ( // StylePRLink colors the PR indicator in the Open Link pickers. Purple is // the app's PR family color, so a PR link reads as a PR at a glance without - // implying any particular review state. - StylePRLink = tcell.StyleDefault.Foreground(ColorPRAwaiting).Bold(true) + // implying any particular review state. Aliased to StylePRAwaiting so the + // shared purple stays structurally identical — restyle the family once and + // both surfaces move together rather than silently diverging. + StylePRLink = StylePRAwaiting // StyleCoordinator renders the task-row coordinator indicator. Cyan (the // project color) reads as "structural/orchestration" rather than a status. From 5ee37adba093812f2dc4140d758214be07abc8ef Mon Sep 17 00:00:00 2001 From: Darren Cheng Date: Wed, 24 Jun 2026 13:57:05 -0700 Subject: [PATCH 3/3] docs(gotchas): note OpenSpec requirement first-line SHALL parsing rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Captured from this session: openspec validate parses the requirement statement as text up to the first blank line and requires SHALL/MUST on the first physical line, so wrapping SHALL onto line 2 fails validation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- context/knowledge/gotchas/misc.md | 1 + 1 file changed, 1 insertion(+) diff --git a/context/knowledge/gotchas/misc.md b/context/knowledge/gotchas/misc.md index 37253a78..28d87c4e 100644 --- a/context/knowledge/gotchas/misc.md +++ b/context/knowledge/gotchas/misc.md @@ -219,4 +219,5 @@ - **`openspec archive --yes` ABORTS but still exits 0 on a delta-vs-base mismatch.** It prints `… MODIFIED failed for header "…" - not found` / `… ADDED … - already exists` then `Aborted. No files were changed.` and returns rc=0 — a scripted `for` loop checking `$?` will silently skip the change. Detect failure by re-checking whether the folder actually moved (`test -d openspec/changes/`) or grepping the output for `Aborted`, not by exit code. The abort is atomic (nothing partial), so just fix the delta and re-run. - **Archive a backlog of un-archived changes in AUTHORING/merge order, not alphabetically.** `MODIFIED` replaces the whole requirement block and `REMOVED` deletes it, so the last-applied delta wins and every `REMOVED`/`MODIFIED` needs its target already present. Out-of-order folding either errors (target missing) or silently leaves the wrong generation in the base spec. Derive order from when each change first landed: `git log --diff-filter=A --format=%cI -- openspec/changes//proposal.md | tail -1`, sorted ascending. - **Folding a backlog can yield an internally-contradictory base spec — audit for orphaned superseded requirements after archiving.** When a later change supersedes an earlier one by `MODIFY`-ing a *different* requirement (e.g. re-defining "Rail keybindings") instead of writing a `REMOVE` for the now-stale standalone requirement the earlier change `ADDED`, both survive the fold and contradict each other. `openspec validate --strict` does NOT catch this (it checks structure, not cross-requirement consistency). After folding, diff the base spec against current code behavior and delete the orphaned requirements by hand (this is what reconciled the stale `R`/`C`/`Ctrl+R`/`Ctrl+D`-bridging hera-view requirements vs the EOL-redesign nuke model). +- **A requirement's statement is parsed as the text up to the first blank line, and `validate` requires SHALL/MUST on that *first physical line*.** If you hard-wrap the statement so "SHALL" lands on line 2 (e.g. a lead-in clause ending in an em-dash on line 1), `openspec validate` fails with `ADDED "" must contain SHALL or MUST` even though SHALL is present in the paragraph. Keep the SHALL on the first line — don't wrap before it. - **Authoring drift to expect when folding old deltas:** a `MODIFIED` whose target was renamed/retired by later-merged work → drop or rewrite it; a `MODIFIED` for a requirement that is actually new (absent from base) → it's really an `ADDED`; an `ADDED` whose requirement a later change already put in base → drop the duplicate (keep base's newer version). `openspec archive` auto-creates new-capability specs with a `## Purpose\nTBD - created by archiving change . Update Purpose after archive.` stub — fill it in the same PR.