Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds xterm addons and link styling; introduces a FilePathLinkProvider to detect/click file paths in terminal output; adds an External TRPC router to open URLs/files in editors with fallbacks; threads workspace CWD into terminal initialization and exposes a terminal query to resolve workspace CWD. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Terminal
participant FilePathLinkProvider
participant ExternalRouter
participant ShellAPI as Electron Shell
participant Editor
User->>Terminal: Cmd/Ctrl-click file path
Terminal->>FilePathLinkProvider: handleActivation(event, text)
FilePathLinkProvider->>FilePathLinkProvider: parseLineColumnPath(text)
FilePathLinkProvider->>ExternalRouter: openFileInEditor(path, line?, column?, cwd?)
ExternalRouter->>ExternalRouter: normalize path (expand ~, resolve relative to cwd)
ExternalRouter->>ExternalRouter: attempt Cursor --goto
alt Cursor succeeds
ExternalRouter->>Editor: spawnAsync cursor --goto file:line:col
else Cursor fails
ExternalRouter->>ExternalRouter: attempt Code --goto
alt Code succeeds
ExternalRouter->>Editor: spawnAsync code --goto file:line:col
else both fail
ExternalRouter->>ShellAPI: shell.openPath(file)
end
end
sequenceDiagram
participant TerminalComponent
participant TerminalRouter
participant TerminalInstance
participant Addons
participant FilePathLinkProvider
TerminalComponent->>TerminalRouter: getWorkspaceCwd(workspaceId)
TerminalRouter-->>TerminalComponent: cwd (or undefined)
TerminalComponent->>TerminalInstance: createTerminalInstance(container, cwd)
TerminalInstance->>TerminalInstance: open terminal
TerminalInstance->>Addons: load fit, webLinks, clipboard, unicode11, serialize
TerminalInstance->>FilePathLinkProvider: register provider (uses cwd)
TerminalInstance-->>TerminalComponent: ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/index.ts (1)
48-51: Add error handling to prevent unhandled Promise rejection inbefore-quithandlerThe concern is valid. The
cleanup()method is async and returnsPromise<void>, and can reject if operations inside fail (e.g., when awaitinghistoryReader.cleanup()). Electron does not wait for a Promise returned from abefore-quitlistener; the listener is invoked synchronously and Electron continues quitting regardless of whether the async work completes. Without error handling, a rejection will become an unhandled Promise rejection on the main process.Add try/catch to prevent unhandled rejections:
- app.on("before-quit", async () => { - await terminalManager.cleanup(); - }); + app.on("before-quit", async () => { + try { + await terminalManager.cleanup(); + } catch (error) { + console.error("[main] Failed to clean up terminals:", error); + } + });This prevents unhandled rejections. Note: If deterministic cleanup is critical, consider the more robust pattern using
event.preventDefault()andapp.quit()after cleanup completes, though that would require additional refactoring.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/globals.css (1)
163-176: Use CSS custom properties for link colors instead of hard-coded hex values.The new terminal link styling uses hard-coded hex colors (#3b8eea, #5ca8ff) that break the design system defined in this file. Since the file establishes a comprehensive theming system with CSS custom properties for light and dark modes using oklch colors, the xterm link colors should be derived from these theme variables for consistency and proper dark mode support.
Consider extracting these colors into custom properties (e.g.,
--xterm-link-colorand--xterm-link-hover-color) and defining them in both the:rootand.darkselectors, or map them to existing theme variables like--primaryor--accent.Example refactor:
:root { --chart-5: oklch(0.769 0.188 70.08); + --xterm-link-color: oklch(0.488 0.243 264.376); + --xterm-link-hover-color: oklch(0.562 0.265 264.376); --radius: 0.625rem;.dark { --chart-5: oklch(0.645 0.246 16.439); + --xterm-link-color: oklch(0.488 0.243 264.376); + --xterm-link-hover-color: oklch(0.562 0.265 264.376); --sidebar-ring: oklch(0.708 0 0);.xterm .xterm-rows a.xterm-underline-5 { - text-decoration-color: #3b8eea !important; + text-decoration-color: var(--xterm-link-color) !important; } .xterm .xterm-rows a:hover.xterm-underline-5 { - text-decoration-color: #5ca8ff !important; + text-decoration-color: var(--xterm-link-hover-color) !important; }Alternatively, if these colors are intentionally fixed (independent of theme), add a comment explaining why they're not part of the theme system.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
20-63: Handle TRPC mutation promises to avoid unhandled rejectionsBoth the URL and file-path click handlers kick off TRPC mutations without handling the returned Promises:
trpc.external.openUrl.mutate(uri);trpc.external.openFileInEditor.mutate({ path, line, column });If the main-process router or the transport errors, these may surface as unhandled Promise rejections in the renderer. Since these are fire-and-forget side effects, it’s enough to explicitly discard and optionally log failures:
- const webLinksAddon = new WebLinksAddon((event, uri) => { - event.preventDefault(); - trpc.external.openUrl.mutate(uri); - }); + const webLinksAddon = new WebLinksAddon((event, uri) => { + event.preventDefault(); + void trpc.external.openUrl.mutate(uri).catch((error) => { + console.error("[terminal] Failed to open URL:", error); + }); + }); @@ - const filePathLinkProvider = new FilePathLinkProvider( - xterm, - (_event, path, line, column) => { - trpc.external.openFileInEditor.mutate({ - path, - line, - column, - }); - }, - ); + const filePathLinkProvider = new FilePathLinkProvider( + xterm, + (_event, path, line, column) => { + void trpc.external.openFileInEditor + .mutate({ path, line, column }) + .catch((error) => { + console.error("[terminal] Failed to open file in editor:", error); + }); + }, + );The rest of the addon setup (open first, then load addons, then
fit()andunicode.activeVersion = "11") looks solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/package.json(2 hunks)apps/desktop/src/lib/trpc/routers/external/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/index.ts(2 hunks)apps/desktop/src/main/index.ts(3 hunks)apps/desktop/src/renderer/globals.css(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(1 hunks)apps/desktop/src/shared/ipc-channels/external.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/shared/ipc-channels/external.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)
apps/desktop/src/main/index.ts (1)
apps/desktop/src/main/lib/agent-setup.ts (1)
setupAgentHooks(103-116)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/config.ts (1)
TERMINAL_OPTIONS(25-31)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (1)
FilePathLinkProvider(16-116)
apps/desktop/src/lib/trpc/routers/index.ts (1)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
createExternalRouter(14-78)
🔇 Additional comments (9)
apps/desktop/src/main/index.ts (1)
30-44: IPC handler comment + guarded agent hook setup look goodRenaming the comment to “Register IPC handlers” and wrapping
setupAgentHooks()in atry/catchafterinitDb()is sane: failures in hook setup are logged but don’t block app startup. No changes requested here.apps/desktop/src/lib/trpc/routers/external/index.ts (1)
19-21: URL opening procedure is straightforward
openUrldirectly delegates toshell.openExternalwith a validated string input, which is appropriate given this is called from your own renderer code. Just ensure all callers treat the input as a URL and not arbitrary untrusted data.apps/desktop/src/lib/trpc/routers/index.ts (1)
3-22: External router integration intocreateAppRouterlooks correctWiring
external: createExternalRouter()into the main router cleanly exposes the new namespace to the client, and theAppRoutertype will naturally include it. No issues from the server-side wiring perspective.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
1-12: New imports align with terminal featuresClipboard/search/serialize/unicode11 addons,
trpc, andFilePathLinkProviderimports all correspond to usage in this file and to the new dependencies in package.json. Nothing to change here.apps/desktop/package.json (1)
40-45: New terminal-related dependencies match the code changesThe added
@xterm/*addons andline-column-pathline up with the new terminal helper andFilePathLinkProviderusage. As usual, ensure lockfile and build pipeline are updated and that these versions are compatible with@xterm/xterm@^5.5.0in your target environments.Also applies to: 58-58
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (4)
1-15: LGTM!The imports are appropriate, and the documentation clearly explains the class purpose with helpful examples of supported patterns.
85-97: LGTM!The cursor handling is implemented correctly with proper null checks for
terminal.element.
99-115: LGTM!The activation logic is well-implemented:
- Requires modifier key (Cmd/Ctrl) to prevent accidental clicks
- Uses
preventDefault()appropriately- Leverages
line-column-pathlibrary for robust parsing- Includes proper validation before invoking the callback
59-65: No issues found with URL filtering logic.The regex pattern structurally prevents matching URLs with protocols. It requires either absolute paths (starting with
/) or relative paths with directory structure (e.g.,src/file.ts). URLs likehttp://example.com/pathwon't match the regex at all, so thestartsWith()check is defensive but harmless. The code works correctly.
| private readonly FILE_PATH_REGEX = | ||
| /((?:~|\.{1,2})?\/[^\s:()]+|(?:\.?[a-zA-Z0-9_-]+\/)+[a-zA-Z0-9_\-.]+)(?::(\d+))?(?::(\d+))?/g; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Windows path support requirement.
The regex pattern only matches Unix-style paths (starting with /, ~, or relative like ./ or src/). It will not detect Windows-style paths like C:\Users\path\to\file.ts.
If this app runs on Windows, consider whether Windows path support is needed.
🏁 Script executed:
#!/bin/bash
# Check if there are any Windows-specific configurations or path handling in the codebase
rg -n --type=ts --type=tsx -C3 "win32|Windows|platform.*win|process\.platform"Length of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if there are any Windows-specific configurations or path handling in the codebase
rg -n --type=ts -C3 "win32|Windows|platform.*win|process\.platform"
Length of output: 21335
🏁 Script executed:
#!/bin/bash
# Also check package.json or build config for platform targeting
fd -e json | head -20 | xargs grep -l "platform\|windows\|electron" 2>/dev/null | head -5Length of output: 180
🏁 Script executed:
#!/bin/bash
# Check the file itself and surrounding context for platform information
cat apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts | head -40Length of output: 1307
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for line-column-path library usage and tests
rg -n --type=ts "line-column-path|parseLineColumnPath" -C3
Length of output: 3833
🏁 Script executed:
#!/bin/bash
# Check for test files related to FilePathLinkProvider
fd -e test.ts -e spec.ts -e ".test.ts" | xargs grep -l "FilePathLinkProvider\|FILE_PATH_REGEX" 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check entire FilePathLinkProvider file to understand full implementation
cat apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.tsLength of output: 3200
---
🌐 Web query:
```web
line-column-path npm library Windows path support
💡 Result:
Yes — the npm package "line-column-path" (by sindresorhus) parses and stringifies paths with optional :line:column (e.g. "file.js:8:14"). It accepts either a string or an object as input, so for Windows paths that include a drive-letter colon (e.g. "C:\path\file.js") you should avoid the ambiguous string form. Use an object ({file: 'C:\path\file.js', line: 8, column: 14}) or a file:// URL (or otherwise normalize/escape the path) to keep the drive-colon from being misinterpreted as line/column. [1]
Sources:
- line-column-path (npm) — package README and API (parse accepts string | object). [1]
FILE_PATH_REGEX does not match Windows-style paths.
The app explicitly supports Windows (see platform checks, build targets, and comment at line 100: "Ctrl+Click (Windows/Linux)"). However, the regex at lines 19-20 only matches Unix-style paths starting with /, ~, or relative paths with /. It will not detect Windows paths like C:\Users\path\to\file.ts, preventing users on Windows from clicking file paths in terminal output.
Update the regex to also match Windows-style paths with drive letters (e.g., C:\path\file.ts, D:\project\src\main.ts).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/external/index.test.ts(1 hunks)apps/desktop/src/lib/trpc/routers/external/index.ts(1 hunks)apps/desktop/src/main/index.ts(2 hunks)apps/desktop/src/main/lib/terminal-manager.ts(0 hunks)apps/desktop/src/main/windows/main.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(1 hunks)apps/desktop/src/shared/ipc-channels/external.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/lib/terminal-manager.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/main/windows/main.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/lib/trpc/routers/external/index.ts
- apps/desktop/src/main/index.ts
🔇 Additional comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
155-158: LGTM! Formatting improvement.The multiline formatting of the span tag improves readability without affecting functionality.
apps/desktop/src/shared/ipc-channels/external.ts (1)
1-7: Migration approach looks good.The empty interface with explanatory comments is a clean way to handle the migration from IPC channels to TRPC while maintaining backward compatibility with the type system.
| import { describe, expect, it, mock } from "bun:test"; | ||
| import { spawn } from "node:child_process"; | ||
|
|
||
| describe("spawnAsync security", () => { | ||
| it("should handle file paths with shell metacharacters safely", async () => { | ||
| // This test verifies that file paths with shell metacharacters | ||
| // are properly escaped when passed to spawn | ||
|
|
||
| const dangerousPath = "/tmp/test file; rm -rf /"; | ||
| const locationWithMeta = `${dangerousPath}:10:5`; | ||
|
|
||
| // Mock spawn to verify arguments | ||
| const mockSpawn = mock(spawn); | ||
|
|
||
| // The key security fix: spawn receives arguments as an array, | ||
| // not as a concatenated string, so shell metacharacters are treated literally | ||
| // and won't be executed by the shell | ||
|
|
||
| // Example of what we fixed: | ||
| // BEFORE (vulnerable): | ||
| // execAsync(`cursor --goto "${location}"`) | ||
| // This would execute: cursor --goto "/tmp/test file; rm -rf /:10:5" | ||
| // The semicolon could break out and execute rm -rf / | ||
| // | ||
| // AFTER (secure): | ||
| // spawnAsync("cursor", ["--goto", location]) | ||
| // spawn receives: ["--goto", "/tmp/test file; rm -rf /:10:5"] | ||
| // The semicolon is treated as part of the filename, not shell syntax | ||
|
|
||
| expect(true).toBe(true); // Placeholder assertion | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Incomplete test implementation requires completion.
The test provides excellent documentation of the security concern but lacks actual verification:
- The
mockSpawnvariable (line 13) is created but never used - No code from the external router is imported or invoked
- The placeholder assertion (line 30) will pass regardless of implementation correctness
To complete this test, you should:
+import { createExternalRouter } from "./index";
+import { initTRPC } from "@trpc/server";
+
describe("spawnAsync security", () => {
it("should handle file paths with shell metacharacters safely", async () => {
- // This test verifies that file paths with shell metacharacters
- // are properly escaped when passed to spawn
-
const dangerousPath = "/tmp/test file; rm -rf /";
const locationWithMeta = `${dangerousPath}:10:5`;
- // Mock spawn to verify arguments
- const mockSpawn = mock(spawn);
+ // Mock spawn to capture and verify arguments
+ const spawnCalls: unknown[][] = [];
+ mock.module("node:child_process", () => ({
+ spawn: (...args: unknown[]) => {
+ spawnCalls.push(args);
+ return { on: () => {}, stdout: { on: () => {} }, stderr: { on: () => {} } };
+ }
+ }));
- // The key security fix: spawn receives arguments as an array,
- // not as a concatenated string, so shell metacharacters are treated literally
- // and won't be executed by the shell
+ const t = initTRPC.create();
+ const router = createExternalRouter(t.router, t.procedure);
+ const caller = router.createCaller({});
- // Example of what we fixed:
- // BEFORE (vulnerable):
- // execAsync(`cursor --goto "${location}"`)
- // This would execute: cursor --goto "/tmp/test file; rm -rf /:10:5"
- // The semicolon could break out and execute rm -rf /
- //
- // AFTER (secure):
- // spawnAsync("cursor", ["--goto", location])
- // spawn receives: ["--goto", "/tmp/test file; rm -rf /:10:5"]
- // The semicolon is treated as part of the filename, not shell syntax
+ await caller.openFileInEditor({ location: locationWithMeta });
- expect(true).toBe(true); // Placeholder assertion
+ // Verify spawn was called with arguments as array (secure)
+ expect(spawnCalls.length).toBeGreaterThan(0);
+ const [command, args] = spawnCalls[0];
+ expect(Array.isArray(args)).toBe(true);
+ expect(args).toContain(locationWithMeta); // Full path with metacharacters passed as single argument
});
});Would you like me to open an issue to track completing this test implementation?
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/external/index.test.ts lines 1-32, the test
documents the security fix but never imports or calls the function under test
and never asserts anything about the mocked spawn; replace the placeholder by
importing the module function (e.g., spawnAsync or execAsync wrapper from the
external router), call it with locationWithMeta, use the mockSpawn to assert
spawn was called once with the binary name (e.g., "cursor") and args array
["--goto", locationWithMeta] (and that options.shell is false or undefined), and
clean up the mock after the test so the test actually verifies that arguments
are passed as an array and shell metacharacters are not executed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (1)
17-22: Regex still excludes Windows-style paths; Cmd/Ctrl-click won’t work on typical Windows stack traces.The current
FILE_PATH_PATTERNonly matches Unix-like/-separated paths and relative forms (e.g.,src/App.tsx,./foo/bar.ts). It won’t match common Windows formats like:
C:\Users\foo\bar.tsD:\project\src\main.ts:10:5Given the comment about Ctrl+Click on Windows/Linux, this means file links on Windows output will silently fail to become clickable. If Windows is in-scope, consider extending the pattern to cover drive-letter + backslash paths and verifying how
parseLineColumnPathbehaves with them (you may need to normalize before parsing or pass{ file, line, column }explicitly).
🧹 Nitpick comments (4)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
134-150: Avoid duplicating workspace → CWD lookup logic.The implementation is correct, but it duplicates the workspace/worktree resolution done in
createOrAttach. Consider extracting a small shared helper (e.g.,getWorkspaceAndWorktree(workspaceId)) or a DB method so both call sites stay in sync if the workspace/worktree model changes.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
26-29: Gate terminal initialization onworkspaceCwdto avoid double setup and wrong CWD.Right now the effect runs once with
workspaceCwdinitiallyundefinedand then reruns when the query resolves, which:
- creates/detaches/recreates the xterm instance, and
- allows early Cmd/Ctrl-clicks to call
openFileInEditorwithcwd: undefinedbefore the real CWD is known.Consider waiting for the query to settle before initializing (e.g., include
isLoadingfromuseQueryand early-return from the effect while loading, or otherwise defer running the effect until you have a stable CWD or an explicit fallback). That should give you a single initialization with the intendedcwd.Also applies to: 80-80, 197-197
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
13-16: Handle TRPC mutation errors (and consider UX around external actions).The addon wiring looks solid, but both external calls here are fire-and-forget:
trpcClient.external.openUrl.mutate(uri)trpcClient.external.openFileInEditor.mutate({ path, line, column, cwd })If either rejects (e.g., no handler, IPC error), you’ll get an unhandled rejection. Consider explicitly discarding and/or logging the promise, for example:
void trpcClient.external.openUrl.mutate(uri).catch(() => { // optional: log or surface a toast });and similarly for
openFileInEditor.You may also want to decide whether URLs should require a modifier key (like file links do) for consistency and to avoid accidental clicks on untrusted terminal output.
Also applies to: 23-27, 28-35, 38-45, 46-59
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (1)
46-51: Cursor indicates clickability even when modifier isn’t pressed.
handleHoveralways sets the cursor to"pointer", buthandleActivationimmediately returns unless Cmd/Ctrl is held. This can feel broken to users who click without a modifier.If you want to keep the modifier requirement, consider aligning the UX by e.g.:
- showing a hint in the terminal about Cmd/Ctrl+click, or
- only switching to
pointerwhen the relevant modifier is down.Otherwise, dropping the modifier requirement would make the pointer cursor behavior intuitive.
Also applies to: 63-66, 91-101, 103-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/external/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/terminal/terminal.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/lib/trpc/routers/external/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/config.ts (1)
TERMINAL_OPTIONS(25-31)apps/desktop/src/renderer/lib/trpc-client.ts (1)
trpcClient(9-11)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (1)
FilePathLinkProvider(16-120)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
Terminal(14-204)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
createTerminalInstance(13-67)
🪛 ast-grep (0.40.0)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts
[warning] 46-46: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(this.FILE_PATH_PATTERN, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (1)
17-22: Unix-style path detection only; WindowsC:\paths still won’t be clickableThe
FILE_PATH_PATTERNcovers Unix/relative paths well but still doesn’t match Windows-style drive-letter paths such asC:\Users\me\project\file.ts. Given the explicit mention of Windows in the comments (Cmd/Ctrl-click behavior) and broader app context, this remains a functional gap for Windows users trying to click stack traces or paths printed by tools.If Windows support is in scope, consider extending the pattern (or adding a separate one) to capture drive-letter paths and pass them through the same
parseLineColumnPathpipeline.Also applies to: 53-79
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
1-69: createTerminalInstance addon wiring and TRPC integration look solidThe new addon setup (fit → web links → clipboard → Unicode 11) and the change to open the terminal before loading addons are correct for xterm. Routing both URL clicks and file-path Cmd/Ctrl-clicks through TRPC with explicit
.catchlogging is also good, and threadingcwdthrough toopenFileInEditorgives you flexibility for relative paths without complicating the terminal API surface.If you anticipate needing to dynamically enable/disable file-path links later, you might consider returning the registered
FilePathLinkProvideror its disposable so callers can opt into more granular lifecycle control, but that’s purely optional given thatxterm.dispose()already tears everything down.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (1)
33-106: Link provider implementation and regex usage look correct; ReDoS warning is a false positive hereThe
provideLinksimplementation is clean: you guard against missing lines, usematchAllwith a freshRegExpper call to avoid state bleed, filter out common non-file patterns (URLs, versions, timestamps), and compute 1-based coordinates correctly.handleActivation’s Cmd/Ctrl-click guard plusparseLineColumnPathkeeps activation behavior precise and offloads parsing complexity appropriately.Regarding the static-analysis warning on
new RegExp(this.FILE_PATH_PATTERN, "g"): sinceFILE_PATH_PATTERNis a fixed, class-local literal without user input and has no pathological nested quantifiers, the ReDoS concern doesn’t really apply here; I wouldn’t change the implementation just to satisfy that warning.Also applies to: 108-136
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
Terminal(14-204)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/config.ts (1)
TERMINAL_OPTIONS(25-31)apps/desktop/src/renderer/lib/trpc-client.ts (1)
trpcClient(9-11)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (1)
FilePathLinkProvider(16-137)
🪛 ast-grep (0.40.0)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts
[warning] 46-46: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(this.FILE_PATH_PATTERN, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
1 participant
Summary by CodeRabbit
New Features
Improvements
Style
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.