diff --git a/context/knowledge/gotchas/misc.md b/context/knowledge/gotchas/misc.md index af4c303f..28d87c4e 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 @@ -217,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. 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..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,7 +242,42 @@ 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) } }) } } + +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..09508fc5 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,17 @@ 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. + // 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 { runes := []rune(display) - if innerW > 3 { - display = string(runes[:innerW-1]) + "…" + if textW > 3 { + display = string(runes[:textW-1]) + "…" } } @@ -295,7 +303,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..3f32a32f 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,18 @@ 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. 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 - 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 +211,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..670772d5 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,13 @@ 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. 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. 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 +