fix(desktop): resolve relative file paths using pty process cwd#440
fix(desktop): resolve relative file paths using pty process cwd#440
Conversation
When clicking relative file paths in the terminal, the path was being resolved against a potentially stale cwd tracked via OSC 7 sequences. This caused intermittent failures when opening files after navigating directories. Now queries the actual cwd directly from the pty's process using its PID (via lsof on macOS, /proc on Linux), ensuring reliable path resolution regardless of OSC 7 support or timing issues. 🤖 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 14 minutes and 46 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 (4)
WalkthroughRefactors file-open resolution to use terminal pane IDs. Adds TerminalManager.getCwd(paneId) to query the OS for a pane's process cwd. TRPC router, terminal helpers, and Terminal component now pass paneId instead of cwd and handle missing/failed cwd lookups explicitly. Changes
Sequence DiagramsequenceDiagram
participant Client as Renderer (Terminal UI)
participant Router as TRPC External Router
participant TM as TerminalManager (main)
participant OS as OS (lsof / proc)
participant Editor as Editor Launcher
Client->>Router: openFileInEditor(path, paneId)
Router->>TM: getCwd(paneId)
Note over TM: find session, check alive, get PTY pid
alt macOS
TM->>OS: run lsof to query process cwd
else Linux
TM->>OS: read /proc/[pid]/cwd symlink
end
OS-->>TM: cwd path or error
TM-->>Router: cwd (string) or null
Router->>Router: resolve absolute path using cwd or error
Router->>Editor: launch editor with resolved path (line/column)
Editor-->>Client: file opened (or error)
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: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/lib/trpc/routers/external/index.ts(3 hunks)apps/desktop/src/main/lib/terminal/manager.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
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/index.tsapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/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/index.tsapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/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/index.tsapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
apps/desktop/src/main/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Files:
apps/desktop/src/main/lib/terminal/manager.ts
**/{components,features}/**/[!.]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Organize project structure with one folder per component: ComponentName/ComponentName.tsx with index.ts barrel export
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/{components,features}/**/*.{ts,tsx,test.ts,test.tsx,stories.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - use only in main process (src/main/)
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
**/*.{tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use React + TailwindCSS v4 + shadcn/ui for UI development
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/{components,features}/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/{components,features}/**/*.tsx: Nest components in parent's components/ folder if used only once, promote to highest shared parent's components/ if used 2+ times
Use one component per file - do not combine multiple components in a single file
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🧠 Learnings (3)
📚 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-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} : For Electron interprocess communication, ALWAYS use trpc as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/trpc/routers/external/index.ts
🧬 Code graph analysis (2)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
apps/desktop/src/main/lib/terminal/manager.ts (1)
terminalManager(458-458)
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(91-175)
⏰ 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 (8)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
201-201: LGTM! Signature update is consistent.The call to
createTerminalInstancecorrectly passespaneIdinstead ofworkspaceCwd, aligning with the new signature that enables paneId-based cwd resolution.
396-396: LGTM! Dependency array correctly updated.Removing
workspaceCwdfrom the effect dependencies is correct since it's no longer passed tocreateTerminalInstance. The component still usesworkspaceCwdfor seeding the initial cwd (line 76), but that doesn't require effect re-triggering.apps/desktop/src/lib/trpc/routers/external/index.ts (3)
6-6: LGTM! Import added for new functionality.The
terminalManagerimport is necessary for the new paneId-based cwd resolution logic.
155-155: LGTM! Input schema updated to support paneId-based resolution.Replacing
cwdwithpaneIdin the input schema aligns with the PR's goal of querying the actual terminal process cwd rather than relying on potentially stale OSC 7 sequences. Maintaining optionality allows backward compatibility with direct path resolution when paneId is not provided.
170-176: LGTM! Path resolution correctly uses paneId-based cwd.The logic properly queries
terminalManager.getCwd(paneId)when available and falls back topath.resolve(filePath)when not, ensuring relative paths are resolved correctly. The synchronousgetCwdcall is acceptable here since it's designed as a fast lookup with proper timeout handling in its implementation.apps/desktop/src/main/lib/terminal/manager.ts (1)
1-1: LGTM! Imports added for new cwd query functionality.The
execSync,existsSync, andreadlinkSyncimports are necessary for the platform-specific cwd detection in the newgetCwdmethod.Also applies to: 3-3
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (2)
91-94: LGTM! Signature updated to use paneId.The function signature correctly changes from accepting an optional
cwdparameter to a requiredpaneIdparameter. This is appropriate sincepaneIdis always available in the terminal context and enables more reliable cwd resolution by querying the actual terminal process.
143-161: LGTM! FilePathLinkProvider correctly passes paneId.The file path link handler now passes
paneIdtoopenFileInEditor.mutate, enabling the backend to query the actual terminal process cwd for accurate relative path resolution at click time.
| /** | ||
| * Get the current working directory of a terminal session's process. | ||
| * Queries the actual cwd from the OS using the pty's PID. | ||
| * Falls back to the initial cwd if the query fails. | ||
| */ | ||
| getCwd(paneId: string): string | null { | ||
| const session = this.sessions.get(paneId); | ||
| if (!session || !session.isAlive) { | ||
| return null; | ||
| } | ||
|
|
||
| const pid = session.pty.pid; | ||
|
|
||
| try { | ||
| if (process.platform === "darwin") { | ||
| // macOS: use lsof to get the cwd | ||
| // lsof -d cwd -Fn -p <pid> outputs lines like: | ||
| // p<pid> | ||
| // n<path> | ||
| const output = execSync(`lsof -d cwd -Fn -p ${pid}`, { | ||
| encoding: "utf-8", | ||
| timeout: 1000, | ||
| }); | ||
| const lines = output.trim().split("\n"); | ||
| for (const line of lines) { | ||
| if (line.startsWith("n") && line.length > 1) { | ||
| const cwd = line.slice(1); | ||
| if (existsSync(cwd)) { | ||
| return cwd; | ||
| } | ||
| } | ||
| } | ||
| } else if (process.platform === "linux") { | ||
| // Linux: read /proc/<pid>/cwd symlink | ||
| const cwd = readlinkSync(`/proc/${pid}/cwd`); | ||
| if (existsSync(cwd)) { | ||
| return cwd; | ||
| } | ||
| } | ||
| } catch { | ||
| // Query failed, fall back to initial cwd | ||
| } | ||
|
|
||
| // Fall back to initial cwd | ||
| return session.cwd; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking package.json for build targets ==="
if [ -f "package.json" ]; then
cat package.json | jq '.build' 2>/dev/null || grep -A 20 '"build"' package.json | head -30
fi
echo -e "\n=== Checking for Windows platform references in desktop/src ==="
rg -i "win32|windows" apps/desktop/src --max-count=20
echo -e "\n=== Checking manager.ts file ==="
wc -l apps/desktop/src/main/lib/terminal/manager.ts
echo -e "\n=== Examining getCwd method and surrounding code ==="
sed -n '265,315p' apps/desktop/src/main/lib/terminal/manager.tsRepository: superset-sh/superset
Length of output: 5213
Add Windows support to getCwd method.
The getCwd method correctly implements platform-specific cwd detection for macOS and Linux, but Windows is missing. Since the app targets Windows (evidenced by platform checks throughout the codebase), add a Windows implementation. Windows lacks /proc/[pid]/cwd symlinks and lsof, so consider using Windows-specific APIs or the wmic command as a fallback approach. Ensure the method maintains its current defensive behavior with timeout protection and graceful fallback to session.cwd.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/terminal/manager.ts around lines 268-313, add
Windows handling in getCwd: detect process.platform === "win32" and run a
short-timeout child command (PowerShell or wmic) to retrieve the target process
executable path or command-line, derive the directory (path.dirname) and check
existsSync before returning it; keep the exec in try/catch with the same timeout
protection and fall back to session.cwd on any failure so behavior remains
defensive and consistent with macOS/Linux branches.
- Add detailed logging to getCwd to debug session lookup issues - Throw error instead of silently falling back to Electron's cwd when session not found (was causing paths to resolve against /) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/terminal/manager.ts (1)
268-329: Windows platform support is missing.This method handles macOS and Linux but has no implementation for Windows (
process.platform === "win32"). Since the app targets Windows (as noted in past reviews), users on Windows will always fall back tosession.cwd, which defeats the purpose of this fix.
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
170-184: Consider differentiating error cases for clearer debugging.The current logic conflates two distinct failure scenarios into one error message:
paneIdwas not provided (undefined)paneIdwas provided but the terminal session doesn't exist orgetCwdreturned nullThis could complicate debugging. The warning message at line 177 would also print
"undefined"whenpaneIdis not provided.🔎 Proposed improvement
// Query actual cwd from the terminal's pty process if paneId is provided if (!path.isAbsolute(filePath)) { - const cwd = input.paneId - ? terminalManager.getCwd(input.paneId) - : null; - if (!cwd) { + if (!input.paneId) { console.warn( - `[openFileInEditor] Cannot resolve relative path "${filePath}": no cwd available for paneId "${input.paneId}"`, + `[openFileInEditor] Cannot resolve relative path "${filePath}": no paneId provided`, ); throw new Error( - `Cannot open relative path "${filePath}" - terminal session not found`, + `Cannot open relative path "${filePath}" - no terminal context provided`, ); } + const cwd = terminalManager.getCwd(input.paneId); + if (!cwd) { + console.warn( + `[openFileInEditor] Cannot resolve relative path "${filePath}": terminal session not found for paneId "${input.paneId}"`, + ); + throw new Error( + `Cannot open relative path "${filePath}" - terminal session not found`, + ); + } filePath = path.resolve(cwd, filePath); }apps/desktop/src/main/lib/terminal/manager.ts (2)
300-302: Consider reducing log verbosity for the success path.This
console.logwill fire on every successful Cmd+click file open on macOS. For production builds, this may clutter logs. Consider using a debug-level log or removing it entirely, while keeping the warning logs for failure cases.🔎 Proposed change
if (existsSync(cwd)) { - console.log( - `[TerminalManager.getCwd] Got cwd from lsof for pid ${pid}: ${cwd}`, - ); return cwd; }
324-328: Consider usingconsole.warnfor fallback logging.The fallback path indicates the OS-specific cwd query failed. Using
console.warnwould be consistent with the other warning logs in this method (lines 276-278, 307-309, 318-321) and make it easier to identify degraded behavior in logs.🔎 Proposed change
// Fall back to initial cwd - console.log( + console.warn( `[TerminalManager.getCwd] Falling back to initial cwd: ${session.cwd}`, ); return session.cwd;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/lib/trpc/routers/external/index.ts(3 hunks)apps/desktop/src/main/lib/terminal/manager.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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/main/lib/terminal/manager.tsapps/desktop/src/lib/trpc/routers/external/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
apps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/lib/trpc/routers/external/index.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/main/lib/terminal/manager.tsapps/desktop/src/lib/trpc/routers/external/index.ts
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/main/lib/terminal/manager.ts
apps/desktop/src/main/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Files:
apps/desktop/src/main/lib/terminal/manager.ts
🧠 Learnings (3)
📚 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-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} : For Electron interprocess communication, ALWAYS use trpc as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/trpc/routers/external/index.ts
🧬 Code graph analysis (1)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
apps/desktop/src/main/lib/terminal/manager.ts (1)
terminalManager(474-474)
🔇 Additional comments (4)
apps/desktop/src/lib/trpc/routers/external/index.ts (2)
6-6: LGTM!Import follows project conventions using the defined alias.
149-156: LGTM!The input schema change from
cwdtopaneIdcorrectly implements the PR objective of resolving cwd at click-time rather than relying on potentially stale OSC 7 data captured in a closure.apps/desktop/src/main/lib/terminal/manager.ts (2)
1-3: LGTM!Import additions are appropriate for the new
getCwdmethod functionality.
291-294: Synchronous operation is acceptable for this use case.While
execSyncblocks the main thread, the 1000ms timeout and user-initiated nature of this operation (click-to-open-file) make this trade-off reasonable. Thelsofcommand typically completes in milliseconds.
The pty.pid returns the login wrapper process, not the actual shell. This was causing lsof to return "/" as the cwd. Now walks down the process tree using pgrep -P to find the deepest child process (the shell) and gets its cwd instead. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
lsof returns "/" on macOS due to security restrictions. Instead, track the cwd by parsing OSC 7 escape sequences that shells emit on directory changes. This is the same approach used by terminal emulators. - Add parse-cwd.ts to main process - Track trackedCwd in TerminalSession from pty output - getCwd returns trackedCwd if available, falls back to initial cwd 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
getCwd(paneId)method to TerminalManager that queries the actual cwd from the pty's process via PIDopenFileInEditorto acceptpaneIdand resolve paths using the real process cwdProblem
Clicking relative file paths in the terminal was intermittent - sometimes working, sometimes not. The issue was that the frontend tracked the terminal's cwd via OSC 7 escape sequences, which:
Solution
Query the actual cwd directly from the OS using the pty's process ID:
lsof -d cwd -Fn -p <pid>readlink /proc/<pid>/cwdThis ensures we always get the real current working directory at click time.
Test plan
cdls,grep, compiler errors)🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.