terminal links - #17
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRemoves CodexTextGenerator tests, tweaks Codex integration defaults (model and reasoning effort), and adds terminal link support to the web client with path/URL extraction, resolution, activation rules, editor integration, and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Terminal as ThreadTerminalDrawer
participant LinkModule as terminal-links
participant Editor
participant External
User->>Terminal: Click terminal output
Terminal->>LinkModule: isTerminalLinkActivation(event, platform)
LinkModule-->>Terminal: allowed / denied
alt allowed
Terminal->>LinkModule: extractTerminalLinks(line)
LinkModule-->>Terminal: TerminalLinkMatch[]
Terminal->>Terminal: locate match at click position
alt kind == "path"
Terminal->>LinkModule: resolvePathLinkTarget(rawPath, cwd)
LinkModule-->>Terminal: absolutePath[:line[:col]]
Terminal->>Editor: openFile(absolutePath, line, col)
Editor-->>User: file opened
else kind == "url"
Terminal->>External: openExternal(URL)
External-->>User: URL opened
end
else denied
Terminal-->>User: no action (modifier key mismatch)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Add clickable URL and file path links to the thread terminal in
|
Greptile OverviewGreptile SummaryAdded clickable link support in terminal output, allowing Cmd/Ctrl+Click to open URLs in browser and file paths (with line:column notation) in the preferred editor. Key changes:
Minor issue:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Terminal as xterm.js Terminal
participant LinkProvider as Terminal Link Provider
participant TerminalLinks as terminal-links.ts
participant API as NativeApi
participant Shell as Shell Service
User->>Terminal: Output contains URLs/paths
Terminal->>LinkProvider: provideLinks(bufferLineNumber, callback)
LinkProvider->>Terminal: buffer.active.getLine(lineNumber)
Terminal-->>LinkProvider: line object
LinkProvider->>LinkProvider: line.translateToString(true)
LinkProvider->>TerminalLinks: extractTerminalLinks(lineText)
TerminalLinks->>TerminalLinks: collectMatches for URLs
TerminalLinks->>TerminalLinks: collectMatches for file paths
TerminalLinks->>TerminalLinks: trimClosingDelimiters
TerminalLinks-->>LinkProvider: TerminalLinkMatch[]
LinkProvider-->>Terminal: callback(links)
Terminal->>User: Display clickable links
User->>Terminal: Cmd/Ctrl+Click on link
Terminal->>LinkProvider: activate(event)
LinkProvider->>TerminalLinks: isTerminalLinkActivation(event)
TerminalLinks-->>LinkProvider: true/false
alt URL link
LinkProvider->>API: shell.openExternal(url)
API->>Shell: Open URL in browser
Shell-->>User: Browser opens URL
else File path link
LinkProvider->>TerminalLinks: resolvePathLinkTarget(path, cwd)
TerminalLinks->>TerminalLinks: splitPathAndPosition
TerminalLinks->>TerminalLinks: joinPath with cwd
TerminalLinks-->>LinkProvider: resolvedPath
LinkProvider->>TerminalLinks: preferredTerminalEditor()
TerminalLinks->>TerminalLinks: localStorage.getItem(LAST_EDITOR_KEY)
TerminalLinks-->>LinkProvider: EditorId
LinkProvider->>API: shell.openInEditor(target, editor)
API->>Shell: Open file in editor
Shell-->>User: Editor opens file
end
Last reviewed commit: 3327659 |
| const FILE_PATH_PATTERN = | ||
| /(?:~\/|\.{1,2}\/|\/|[A-Za-z]:\\|\\\\)[^\s"'`<>]+|[A-Za-z0-9._-]+(?:\/[A-Za-z0-9._-]+)+(?::\d+){0,2}/g; | ||
| const TRAILING_PUNCTUATION_PATTERN = /[.,;!?]+$/; | ||
| const LAST_EDITOR_KEY = "t3code:last-editor"; |
There was a problem hiding this comment.
LAST_EDITOR_KEY constant duplicated in ChatView.tsx:366. Consider extracting to shared constant.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| end: number; | ||
| } | ||
|
|
||
| const URL_PATTERN = /https?:\/\/[^\s"'`<>]+/g; |
There was a problem hiding this comment.
🟢 Low
src/terminal-links.ts:12 URL_PATTERN allows )]} characters, causing O(N²) complexity in trimClosingDelimiters when a URL ends with many closing delimiters. Consider excluding )]} from the pattern, or adding a max-iteration limit in the trim loop.
| const URL_PATTERN = /https?:\/\/[^\s"'`<>]+/g; | |
| const URL_PATTERN = /https?:\/\/[^\s"'`<>()\[\]{}]+/g; |
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/web/src/terminal-links.ts around line 12:
`URL_PATTERN` allows `)]}` characters, causing O(N²) complexity in `trimClosingDelimiters` when a URL ends with many closing delimiters. Consider excluding `)]}` from the pattern, or adding a max-iteration limit in the trim loop.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/server/src/codexTextGenerator.ts`:
- Around line 21-22: Add verification for the changed constants by creating a
unit test for CodexTextGenerator that asserts CODEX_MODEL and
CODEX_REASONING_EFFORT are used when invoking the text-generation client: mock
the underlying API call (or inject a fake client) inside CodexTextGenerator and
assert the model param equals CODEX_MODEL and the effort/metadata equals
CODEX_REASONING_EFFORT; alternatively, if tests are not desired, add a clear
code comment or README note in the CodexTextGenerator module documenting why the
model changed from "gpt-5.3-codex-spark" to "gpt-5.3-codex" and why reasoning
effort was lowered to "low" and what behavioral impact to expect.
In `@apps/web/src/terminal-links.ts`:
- Around line 118-141: splitPathAndPosition currently always returns an object
with path, line, and column keys which can include explicit undefined values;
update it so the return object only includes the optional keys when they are
defined (i.e., return { path } or { path, line } or { path, line, column }
depending on which variables are set). Modify the return construction in
function splitPathAndPosition to build the result conditionally (use a local
result object and add line/column properties only if line !== undefined / column
!== undefined) so it matches the declared signature { path: string; line?:
string; column?: string } under exactOptionalPropertyTypes.
🧹 Nitpick comments (3)
apps/web/src/components/ThreadTerminalDrawer.tsx (1)
11-16: Use the@/alias import instead of a relative import.This keeps imports consistent with the repo convention and avoids brittle
../paths.Proposed change
import { extractTerminalLinks, isTerminalLinkActivation, preferredTerminalEditor, resolvePathLinkTarget, -} from "../terminal-links"; +} from "@/terminal-links";Based on learnings: “Use path aliases
@/for src/ … in imports” and “Prefer importing modules using path aliases … instead of relative imports.”apps/web/src/terminal-links.test.ts (1)
9-110: Good baseline coverage; consider adding Windows +~/resolution cases.What you have covers the key behaviors well. The next high-value additions would be:
- Windows absolute:
C:\repo\src\main.ts:12(and\\server\share\file.ts:12)- Windows-ish relative (if you decide to support it):
.\src\main.ts:12~/expansion whencwdindicates a home dir (/Users/...orC:\Users\...)Based on learnings: “Prefer deterministic inputs and explicit state checks…”
apps/web/src/terminal-links.ts (1)
149-157: Consider consistency in platform detection fallback handling.The empty
platform.length === 0check in this function is unique in the codebase—other platform detection code (terminal-shortcuts.ts,ChatView.tsx) usesnavigator.platformdirectly without a fallback. Either align this function with the existing pattern, or document why the fallback is necessary here. Note that the codebase does not currently usenavigator.userAgentfor platform detection, suggesting the team intentionally avoids that approach.
Queue item pingdotgg#17. A ticket's history was scattered across four drawer views -- the agent conversation, the step list, routing history and discussion -- with no single ordered answer to what happened and why. Built as a pure event-to-entry mapper over the journal, with no new RPC: the timeline read added for time-travel replay already returns exactly these events, so the two features share one read path rather than each growing their own. The History section now renders readable entries instead of raw event type names. The mapper is total by construction. An event this build does not recognize becomes an 'unknown' entry rather than disappearing, because a client is routinely older than the server that wrote the event and a silently missing row is invisible in a way an unfamiliar one is not. Actor is coarse -- user, agent, system, external -- and derived from event semantics, since the journal has no actor column. A manual move is attributed to a person and a routed one to the engine, which is the distinction that matters when reading why a ticket went somewhere. contracts 0 errors / 7 mapper tests, web 0 errors / 2043 tests.
…foreground-accessibility fix(desktop): avoid window capture accessibility timeout
Summary by CodeRabbit
New Features
Tests
Chores