fix(desktop): use user's preferred app for terminal click-to-open#441
fix(desktop): use user's preferred app for terminal click-to-open#441
Conversation
Terminal file path clicks now open files in the user's preferred app (from OpenInButton selection) instead of hardcoding Cursor/VS Code. Refactored external router to eliminate duplication: - Extract helpers.ts with pure functions (getAppCommand, resolvePath) - Share openPathInApp logic between openInApp and openFileInEditor - Replace switch statement with map lookup - Add 24 tests for helper functions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded@Kitenite has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis PR extracts and modularizes helper utilities for macOS external app interactions, introducing a new dedicated helpers module with comprehensive test coverage for app command generation, path resolution, and async process spawning capabilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
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/lib/trpc/routers/external/index.ts (1)
77-90: Line and column parameters are accepted but never used, breaking navigation functionality.The
openFileInEditormutation acceptslineandcolumnparameters in its input schema (lines 81-82), but the implementation ignores them entirely. The parameters are never passed toopenPathInApp, and even if they were, the current approach using macOS'sopen -acommand cannot forward line:column information to applications.To support line/column navigation, the implementation needs to:
- Pass
lineandcolumntoopenPathInApp- Switch from generic
open -alauncher to app-specific CLIs that support navigation:
- Cursor/VS Code:
cursor file.js:42:10orcode --goto file.js:42:10- Sublime:
subl file:42:10- Others require their respective CLI tools
- Update
getAppCommandto construct these properly formatted commands
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/external/helpers.test.ts (1)
67-107: Consider adding tests for remaining JetBrains IDEs.The test suite covers 5 JetBrains IDEs but
APP_NAMESin helpers.ts defines 12 total. Consider adding tests for the remaining 7: phpstorm, rubymine, clion, rider, datagrip, appcode, and fleet to ensure their display names are mapped correctly.🔎 Proposed additional test cases
test("returns correct command for rustrover", () => { const result = getAppCommand("rustrover", "/path/to/file"); expect(result).toEqual({ command: "open", args: ["-a", "RustRover", "/path/to/file"], }); }); + + test("returns correct command for phpstorm", () => { + const result = getAppCommand("phpstorm", "/path/to/file"); + expect(result).toEqual({ + command: "open", + args: ["-a", "PhpStorm", "/path/to/file"], + }); + }); + + test("returns correct command for rubymine", () => { + const result = getAppCommand("rubymine", "/path/to/file"); + expect(result).toEqual({ + command: "open", + args: ["-a", "RubyMine", "/path/to/file"], + }); + }); + + test("returns correct command for clion", () => { + const result = getAppCommand("clion", "/path/to/file"); + expect(result).toEqual({ + command: "open", + args: ["-a", "CLion", "/path/to/file"], + }); + }); + + test("returns correct command for rider", () => { + const result = getAppCommand("rider", "/path/to/file"); + expect(result).toEqual({ + command: "open", + args: ["-a", "Rider", "/path/to/file"], + }); + }); + + test("returns correct command for datagrip", () => { + const result = getAppCommand("datagrip", "/path/to/file"); + expect(result).toEqual({ + command: "open", + args: ["-a", "DataGrip", "/path/to/file"], + }); + }); + + test("returns correct command for appcode", () => { + const result = getAppCommand("appcode", "/path/to/file"); + expect(result).toEqual({ + command: "open", + args: ["-a", "AppCode", "/path/to/file"], + }); + }); + + test("returns correct command for fleet", () => { + const result = getAppCommand("fleet", "/path/to/file"); + expect(result).toEqual({ + command: "open", + args: ["-a", "Fleet", "/path/to/file"], + }); + }); });apps/desktop/src/lib/trpc/routers/external/helpers.ts (1)
42-64: Consider documenting edge case behavior.If both
HOMEandUSERPROFILEare undefined, tilde expansion silently fails and the path remains unchanged. While this is unlikely on macOS, documenting this behavior would improve code clarity.🔎 Suggested documentation improvement
/** * Resolve a path by expanding ~ and converting relative paths to absolute. + * If ~ expansion fails (no HOME or USERPROFILE env var), the path is used as-is. */ export function resolvePath(filePath: string, cwd?: string): string {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/external/helpers.test.ts(1 hunks)apps/desktop/src/lib/trpc/routers/external/helpers.ts(1 hunks)apps/desktop/src/lib/trpc/routers/external/index.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx,js,jsx}: For Electron interprocess communication, ALWAYS use trpc as defined insrc/lib/trpc
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/lib/trpc/routers/external/helpers.test.tsapps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/lib/trpc/routers/external/helpers.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
apps/desktop/src/lib/trpc/routers/external/helpers.test.tsapps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/lib/trpc/routers/external/helpers.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
apps/desktop/src/lib/trpc/routers/external/helpers.test.tsapps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/lib/trpc/routers/external/helpers.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Place tests co-located with components using .test.ts or .test.tsx naming convention
Files:
apps/desktop/src/lib/trpc/routers/external/helpers.test.ts
🧠 Learnings (5)
📚 Learning: 2025-12-18T23:19:10.405Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.405Z
Learning: Applies to **/{components,features}/**/*.{ts,tsx,test.ts,test.tsx,stories.tsx} : Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Applied to files:
apps/desktop/src/lib/trpc/routers/external/helpers.test.ts
📚 Learning: 2025-12-18T23:19:10.405Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.405Z
Learning: Applies to apps/desktop/src/main/**/*.ts : Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Applied to files:
apps/desktop/src/lib/trpc/routers/external/index.ts
📚 Learning: 2025-12-18T17:26:38.664Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-18T17:26:38.664Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use trpc as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/lib/trpc/routers/external/helpers.ts
📚 Learning: 2025-12-18T23:19:10.405Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.405Z
Learning: Applies to apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx} : Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/lib/trpc/routers/external/index.ts
📚 Learning: 2025-12-18T17:26:38.664Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-18T17:26:38.664Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : Use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/lib/trpc/routers/external/helpers.ts
🧬 Code graph analysis (2)
apps/desktop/src/lib/trpc/routers/external/helpers.test.ts (1)
apps/desktop/src/lib/trpc/routers/external/helpers.ts (2)
getAppCommand(33-40)resolvePath(45-64)
apps/desktop/src/lib/trpc/routers/external/index.ts (2)
apps/desktop/src/lib/trpc/routers/external/helpers.ts (5)
EXTERNAL_APPS(100-100)ExternalApp(100-100)getAppCommand(33-40)spawnAsync(70-97)resolvePath(45-64)apps/desktop/src/main/lib/db/schemas.ts (2)
EXTERNAL_APPS(73-95)ExternalApp(97-97)
⏰ 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 (7)
apps/desktop/src/lib/trpc/routers/external/helpers.test.ts (1)
118-187: LGTM! Comprehensive path resolution tests.The test suite thoroughly covers home directory expansion, absolute/relative path handling, and edge cases. The setup/teardown properly manages environment variables.
apps/desktop/src/lib/trpc/routers/external/helpers.ts (3)
5-27: LGTM! Well-structured app name mapping.The mapping provides clear names for all supported apps with appropriate handling for the special
findercase.
29-40: LGTM! Clean command generation.The function correctly uses macOS
open -ato avoid PATH issues in production builds. Returning null for unrecognized apps allows caller to handle fallback.
66-97: LGTM! Robust async process spawning.The function properly handles both spawn errors and non-zero exit codes with descriptive error messages. Using
stdio: "ignore"anddetached: falseis appropriate for this use case.apps/desktop/src/lib/trpc/routers/external/index.ts (3)
1-11: LGTM! Clean imports.Proper use of type imports and helpers module. Follows the project's tRPC-based IPC communication pattern.
Based on learnings, tRPC usage for Electron IPC is correct.
15-32: LGTM! Well-structured internal helper.The centralized
openPathInAppfunction properly handles the specialfindercase and provides a sensible fallback toshell.openPathwhen app commands aren't available.
50-66: LGTM! Clean preference-saving flow.The mutation correctly saves the user's app preference before opening, ensuring subsequent operations use the selected app.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.