fix(desktop): improve terminal link handling with CMD+click and wrapp…#197
Conversation
…ed line support - Require CMD+click (Mac) or Ctrl+click (Windows/Linux) to open URLs and file paths - Handle file paths that span wrapped terminal lines by checking both previous and next lines - Add comprehensive tests for FilePathLinkProvider covering all scenarios 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds wrap-aware file-path link detection in the terminal (multi-line matching and precise multi-line ranges), requires modifier key for URL activation, and introduces comprehensive tests covering detection, wrapping, filtering, activation, and edge cases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used📓 Path-based instructions (4)apps/desktop/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
apps/desktop/**/*.{ts,tsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/desktop/src/renderer/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-11-28T01:03:47.963ZApplied to files:
🪛 ast-grep (0.40.0)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts[warning] 51-51: 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. (regexp-from-variable) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (1)
46-49: Consider documenting the 3-line wrap limitation.The implementation combines at most 3 lines (prev + current + next) for path detection. While this covers virtually all realistic scenarios, paths spanning 4+ wrapped lines won't be fully detected. Consider adding a brief comment documenting this design choice.
- // Combined text for matching paths that may span wrap points - // Format: [prevLine] + currentLine + [nextLine] + // Combined text for matching paths that may span wrap points (up to 3 lines) + // Format: [prevLine] + currentLine + [nextLine] + // Note: Paths spanning 4+ wrapped lines are not supported (extremely rare edge case)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts
apps/desktop/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.test.{ts,tsx,js,jsx}: Tests should have one assert per test
Tests should be readable
Tests should be fast
Tests should be independent
Tests should be repeatable
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts
🧠 Learnings (5)
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be readable
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be independent
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be repeatable
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to **/components/**/*.tsx : Co-locate tests next to the component file they test (e.g., `ComponentName.test.tsx`)
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts
🧬 Code graph analysis (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.ts (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (1)
FilePathLinkProvider(4-209)
🪛 ast-grep (0.40.0)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts
[warning] 51-51: 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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (10)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
65-74: LGTM!The modifier key gate correctly ensures URLs only open on CMD+click (macOS) or Ctrl+click (Windows/Linux), which aligns with the FilePathLinkProvider's activation behavior. This provides a consistent UX across both URL and file path links.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.ts (3)
51-52: Static analysis false positive - pattern is a class constant, not user input.The ast-grep warning about ReDoS is a false positive here.
FILE_PATH_PATTERNis areadonlyclass constant defined at line 5-6, not constructed from user-provided input. The pattern is static and controlled by the developer.
130-180: LGTM - calculateLinkRange correctly handles multi-line coordinates.The coordinate calculations properly account for xterm's 1-based indexing and handle all cases where paths span previous, current, and/or next lines. The defensive branch at lines 166-169 is appropriately guarded with a comment.
18-128: LGTM - Well-structured wrap-aware link detection.The
provideLinksmethod correctly:
- Handles 1-based to 0-based line number conversion
- Detects wrapped lines in both directions (prev/next)
- Filters false positives (URLs, versions, npm refs, pure numbers)
- Only processes matches that overlap the current line
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.ts (6)
1-46: LGTM - Well-designed test helpers.The mock helpers (
createMockLine,createMockTerminal,getLinks) are clean and provide good abstractions for testing theFilePathLinkProvider. Each test creates its own isolated mocks, ensuring independence and repeatability as per coding guidelines.
49-124: Good coverage of basic path detection scenarios.Tests cover all expected path formats (absolute, relative, home directory, with line/column). While some tests have 2 assertions, they're testing closely related properties of the same result, which is acceptable for readability. As per coding guidelines, tests are independent and repeatable.
126-182: LGTM - Thorough false positive filtering tests.Each filtering test has a single assertion (
expect(links.length).toBe(0)), following the "one assert per test" guideline. Good coverage of URLs, versions, npm references, and pure numbers.
184-305: Excellent coverage of wrapped line scenarios.Tests cover forward-looking, backward-looking, and three-line wrapping cases. The range coordinate assertions (lines 202-217) test interconnected properties that logically belong together. Non-wrapped line tests correctly verify lines aren't incorrectly combined.
360-378: Multiple assertions test a single logical outcome.Lines 374-377 have 4 assertions, but they're verifying the arguments of a single
onOpencall. Per coding guidelines, splitting these into 4 separate tests would reduce readability without improving test quality. This approach is acceptable.
381-442: LGTM - Good edge case coverage.Cursor styling tests have single assertions each. Edge cases cover empty lines, non-existent lines, and paths without leading directory markers. Tests are readable and follow the established patterns.
…kProvider Remove hover/leave handlers that were setting cursor style - the CSS already handles cursor:pointer for links. This simplifies the code by keeping only the CMD+click activation logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Kitenite
force-pushed
the
azure-valley-11
branch
from
e58c8df to
0914756
Compare
1 participant
…ed line support
🤖 Generated with Claude Code
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.