fix(desktop): use full PATH for open in app in production builds#185
fix(desktop): use full PATH for open in app in production builds#185saddlepaddle merged 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@saddlepaddle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 21 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 (1)
WalkthroughAdds a cached login-shell PATH resolver used for locating binaries and injecting PATH into spawned processes; refactors external editor launch to prefer editor URL schemes (vscode/cursor) with shell.openExternal and falls back to shell.openPath, and improves spawn error messages. No public signatures changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
saddlepaddle
force-pushed
the
fix/open-in-app-path-production
branch
from
ccffe0e to
df8942f
Compare
![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: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/lib/trpc/routers/external/index.ts(2 hunks)apps/desktop/src/main/lib/agent-setup.ts(2 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/lib/trpc/routers/external/index.tsapps/desktop/src/main/lib/agent-setup.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/lib/trpc/routers/external/index.tsapps/desktop/src/main/lib/agent-setup.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/lib/trpc/routers/external/index.tsapps/desktop/src/main/lib/agent-setup.ts
apps/desktop/src/lib/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in shared code like
src/lib/electron-router-dom.ts- this code runs in both main and renderer processes
Files:
apps/desktop/src/lib/trpc/routers/external/index.ts
apps/desktop/src/main/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Node.js modules (fs, path, os, net, etc.) can be used in main process code only
Files:
apps/desktop/src/main/lib/agent-setup.ts
🧠 Learnings (2)
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
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
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to apps/desktop/src/main/**/*.{ts,tsx} : Node.js modules (fs, path, os, net, etc.) can be used in main process code only
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/agent-setup.ts (1)
getSupersetPath(168-170)
⏰ 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). (2)
- GitHub Check: Build - macOS (arm64)
- GitHub Check: Build
🔇 Additional comments (6)
apps/desktop/src/main/lib/agent-setup.ts (3)
10-15: LGTM! Good caching strategy.The cached variable with clear documentation effectively addresses the Finder/Dock PATH inheritance issue. Caching this expensive shell operation improves performance.
47-65: LGTM! Binary discovery now uses the full shell PATH.The modification to use
getShellPath()ensures that wrapper scripts can find real binaries even when the app is launched from Finder/Dock. The logic correctly filters out the wrapper directory.
163-170: LGTM! Core fix for production build PATH issue.The change to use
getShellPath()ensures that spawned processes (likecodeandcursor) can be found even when the Electron app doesn't inherit the user's shell PATH. This directly addresses the PR objective.apps/desktop/src/lib/trpc/routers/external/index.ts (3)
4-4: LGTM! Appropriate import for main process code.The import of
getSupersetPathis correct for this tRPC router which runs in the main process.
48-51: LGTM! Correct PATH injection for spawned processes.Setting
PATHtogetSupersetPath()ensures that CLI tools likecodeandcursorcan be located in spawned processes, even when the Electron app lacks the full shell PATH. This is the critical fix for the production build issue.
54-72: LGTM! Much more actionable error messages.The enhanced error messages provide clear context about which command failed and actionable guidance for users. This significantly improves the debugging experience when CLI tools are missing or misconfigured.
| /** | ||
| * Gets the user's shell PATH by running a login shell. | ||
| * Caches the result for subsequent calls. | ||
| */ | ||
| function getShellPath(): string { | ||
| if (cachedShellPath !== null) { | ||
| return cachedShellPath; | ||
| } | ||
|
|
||
| const shell = process.env.SHELL || "/bin/zsh"; | ||
| try { | ||
| // Run a login shell to get the full PATH | ||
| const result = execSync(`${shell} -l -c 'echo $PATH'`, { | ||
| encoding: "utf-8", | ||
| timeout: 5000, | ||
| }); | ||
| cachedShellPath = result.trim(); | ||
| console.log("[agent-setup] Retrieved shell PATH"); | ||
| } catch (error) { | ||
| console.warn("[agent-setup] Failed to get shell PATH, using process.env.PATH:", error); | ||
| cachedShellPath = process.env.PATH || "/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin"; | ||
| } | ||
|
|
||
| return cachedShellPath; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the agent-setup.ts file to verify the code snippet
cat -n apps/desktop/src/main/lib/agent-setup.ts | head -60Repository: superset-sh/superset
Length of output: 2272
🏁 Script executed:
#!/bin/bash
# Check for platform-specific configuration in package.json or build files
fd -e "package.json" -e "electron-builder.config.*" -e "tsconfig.json" | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Find package.json files and check for platform configuration
find . -name "package.json" -type f 2>/dev/null | grep -E "(apps/desktop|root)" | head -5
cat ./package.json 2>/dev/null | grep -A 5 -B 5 "target\|platform" | head -30Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Read the desktop package.json to check build targets
cat apps/desktop/package.json | head -100Repository: superset-sh/superset
Length of output: 3247
🏁 Script executed:
#!/bin/bash
# Check electron-builder configuration
cat apps/desktop/electron-builder.ts 2>/dev/null | head -150Repository: superset-sh/superset
Length of output: 2472
🏁 Script executed:
#!/bin/bash
# Check for platform detection in agent-setup.ts
cat -n apps/desktop/src/main/lib/agent-setup.ts | wc -l
echo "---"
cat apps/desktop/src/main/lib/agent-setup.tsRepository: superset-sh/superset
Length of output: 5287
🏁 Script executed:
#!/bin/bash
# Search for setupAgentHooks calls and any platform detection
rg -n "setupAgentHooks" apps/desktop/src/
echo "---"
echo "Checking for platform detection patterns:"
rg -n "process.platform\|darwin\|win32\|linux" apps/desktop/src/main/ | head -20Repository: superset-sh/superset
Length of output: 337
🏁 Script executed:
#!/bin/bash
# Check the context around setupAgentHooks call in index.ts
cat -n apps/desktop/src/main/index.ts | sed -n '1,50p'Repository: superset-sh/superset
Length of output: 1823
Based on my verification, I've identified a critical issue with the review comment and the underlying code:
Key Findings:
-
✅ The concern about
/bin/zshfallback is valid - it doesn't exist on Windows -
✅ The hard-coded PATH fallback (
/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin) is macOS/Linux-only -
🔴 Larger scope: The ENTIRE
agent-setup.tsfile is UNIX-specific, not just the shell fallback:- Uses
whichcommand (line 50) - unavailable on Windows - All wrapper scripts are bash scripts - don't work on Windows
setupAgentHooks()is called unconditionally on all platforms (line 39 of index.ts) with no platform guards
- Uses
-
The electron-builder config confirms the app targets Windows, Linux, and macOS
Critical Gap: While the review comment correctly identifies the /bin/zsh issue, it understates the severity—the entire file will fail on Windows without platform detection.
Verify the shell fallback and broader platform compatibility of agent-setup.ts.
The code assumes a UNIX-like environment but the app is built for Windows, Linux, and macOS. Line 26 hard-codes /bin/zsh as a fallback shell (doesn't exist on Windows), and the entire file uses UNIX-specific commands (which, bash scripts) that won't work on Windows. Additionally, setupAgentHooks() is called unconditionally in apps/desktop/src/main/index.ts:39 with no platform guards.
Recommend:
- Add platform detection using
process.platformto skip agent setup on Windows or provide Windows-compatible alternatives - Replace UNIX-specific shell logic with cross-platform approach (e.g., use
process.env.PATHdirectly on Windows instead of spawning a login shell) - Ensure
whichcommand fallback for Windows (usewhereor alternative approach)
saddlepaddle
force-pushed
the
fix/open-in-app-path-production
branch
from
df8942f to
e9f1627
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/agent-setup.ts (1)
10-41: Platform compatibility issue remains unresolved.The past review comment about UNIX-specific assumptions still applies to this cached PATH implementation:
- Line 26:
/bin/zshfallback doesn't exist on Windows- Line 29: Shell execution with
-l -cflags is UNIX-specific- Line 37: Hardcoded PATH fallback is UNIX-specific
- Line 50:
whichcommand unavailable on WindowsThe caching mechanism introduced here inherits these platform limitations. As noted in the previous review, the entire file requires platform detection (e.g.,
process.platformguards) or Windows-compatible alternatives to work cross-platform.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
154-169: URL scheme approach effectively avoids PATH issues.The refactor to use editor URL schemes (
cursor://fileandvscode://file) is a clean solution that works reliably without depending on PATH environment variables. The fallback toshell.openPath()provides reasonable last-resort behavior.Optional improvement: Consider logging when falling back to
shell.openPath(), as it loses line/column information. This would help users understand why line positioning didn't work.// Fall back to system default + console.warn('[external] Editor URL schemes failed, falling back to system default (line/column info will be lost)'); await shell.openPath(filePath);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/lib/trpc/routers/external/index.ts(5 hunks)apps/desktop/src/main/lib/agent-setup.ts(2 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/main/lib/agent-setup.tsapps/desktop/src/lib/trpc/routers/external/index.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/main/lib/agent-setup.tsapps/desktop/src/lib/trpc/routers/external/index.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/main/lib/agent-setup.tsapps/desktop/src/lib/trpc/routers/external/index.ts
apps/desktop/src/main/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Node.js modules (fs, path, os, net, etc.) can be used in main process code only
Files:
apps/desktop/src/main/lib/agent-setup.ts
apps/desktop/src/lib/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in shared code like
src/lib/electron-router-dom.ts- this code runs in both main and renderer processes
Files:
apps/desktop/src/lib/trpc/routers/external/index.ts
🧠 Learnings (4)
📚 Learning: 2025-11-24T21:32:21.713Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/CLAUDE.md:0-0
Timestamp: 2025-11-24T21:32:21.713Z
Learning: Applies to apps/desktop/**/AGENTS.md : Document agent responsibilities, capabilities, and interaction patterns in AGENTS.md
Applied to files:
apps/desktop/src/main/lib/agent-setup.ts
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Applied to files:
apps/desktop/src/lib/trpc/routers/external/index.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/lib/trpc/routers/external/index.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
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
⏰ 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). (2)
- GitHub Check: Build - macOS (arm64)
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/main/lib/agent-setup.ts (1)
21-41: Shell PATH caching approach looks good.The caching mechanism effectively solves the PATH inheritance issue for production builds launched from Finder/Dock. The fallback chain (login shell → process.env.PATH → hardcoded PATH) provides reasonable resilience.
Minor note: Line 29 uses string interpolation with
process.env.SHELLinexecSync. While command injection risk is minimal in the main process context, this is worth noting for future maintainers.apps/desktop/src/lib/trpc/routers/external/index.ts (1)
50-67: Improved error messages enhance debuggability.The enhanced error handling now provides actionable feedback including the command name and installation hints. This aligns well with the PR's goal of improving error messages.
| * Get the command and args to open a path in the specified app. | ||
| * Uses `open -a` for macOS apps to avoid PATH issues in production builds. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the structure of the external router file and look for platform detection
head -50 apps/desktop/src/lib/trpc/routers/external/index.tsRepository: superset-sh/superset
Length of output: 1687
🏁 Script executed:
#!/bin/bash
# Search for platform-specific checks in the trpc routers directory
rg -n "process\.platform|darwin|win32|linux" apps/desktop/src/lib/trpc/ -A 2 -B 2Repository: superset-sh/superset
Length of output: 383
🏁 Script executed:
#!/bin/bash
# Check if there are other platform-specific router files or variants
fd -t f "router" apps/desktop/src/lib/trpc/ | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Look for how the external router is used/exported
rg -n "external" apps/desktop/src/lib/trpc/ -A 2 -B 2 | head -50Repository: superset-sh/superset
Length of output: 1913
🏁 Script executed:
#!/bin/bash
# Check electron-builder config to understand target platforms
cat electron-builder.json 2>/dev/null || cat electron-builder.js 2>/dev/null || find . -maxdepth 2 -name "*electron-builder*" -type fRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Get more context from the external router, especially the createExternalRouter function
sed -n '70,200p' apps/desktop/src/lib/trpc/routers/external/index.tsRepository: superset-sh/superset
Length of output: 2742
🏁 Script executed:
#!/bin/bash
# Search for calls to openFileInEditor to see if platform detection happens at call site
rg -n "openFileInEditor|\.external\." apps/desktop/src/ -A 2 -B 2Repository: superset-sh/superset
Length of output: 5474
🏁 Script executed:
#!/antml/bash
# Check how the routers are conditionally mounted or used in main process
rg -n "createExternalRouter|process\.platform" apps/desktop/src/lib/trpc/ -A 3 -B 3Repository: superset-sh/superset
Length of output: 2537
🏁 Script executed:
#!/bin/bash
# Look for any main process entry points to understand platform handling
find apps/desktop/src -name "*main*" -o -name "*index.ts" | grep -E "(main|index)" | head -10Repository: superset-sh/superset
Length of output: 521
🏁 Script executed:
#!/bin/bash
# Check the electron-builder config more broadly
find . -maxdepth 3 -type f \( -name "*.json" -o -name "*.js" \) | xargs grep -l "electron-builder\|build.*electron" 2>/dev/null | head -5Repository: superset-sh/superset
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Check if there's any platform detection in the main process setup or app initialization
cat apps/desktop/src/main/index.ts | head -100Repository: superset-sh/superset
Length of output: 1479
🏁 Script executed:
#!/bin/bash
# Look for any build-time or runtime configuration that might disable certain features on non-macOS
rg -n "darwin|platform.*darwin" apps/desktop/src/ -B 2 -A 2Repository: superset-sh/superset
Length of output: 1333
🏁 Script executed:
#!/bin/bash
# Check package.json to understand target platforms for the electron build
cat apps/desktop/package.json | grep -A 20 '"build"'Repository: superset-sh/superset
Length of output: 840
Add platform detection to openInApp and getAppCommand procedures to prevent runtime failures on Windows/Linux builds.
The concern is valid: open -a commands (for VSCode, Cursor, Xcode, iTerm, Warp, Terminal) are macOS-only and will fail on Windows/Linux. The external router is unconditionally mounted in the main router with no platform guards, and callers (e.g., WorkspaceView, Terminal helpers) invoke openInApp without checking the platform despite getPlatform being available in the renderer.
The codebase has platform detection utilities available (e.g., process.platform === "darwin" checks in menu.ts, PLATFORM constants in shared/constants.ts, and renderer-side platform queries), but they are not applied to the external router procedures.
Recommendation: Add platform detection in getAppCommand to return appropriate fallbacks (e.g., null or error) on non-macOS platforms, or add guards in the openInApp procedure to handle platform-specific behavior.
🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/external/index.ts around lines 12-13, the
getAppCommand/openInApp logic uses macOS-only "open -a" commands unguarded,
which will fail on Windows/Linux; update getAppCommand to detect platform (use
PLATFORM constant or process.platform === "darwin") and return null or a clear
error for non-macOS, and update openInApp to check the returned command and
either throw a descriptive RPC error or perform a platform-specific fallback
(e.g., use shell commands for Windows/Linux) so callers don't attempt to execute
macOS-only commands on other platforms.
e9f1627 to
ade2cb6
Compare
Use macOS-native approaches instead of CLI commands that require PATH: - Use `open -a "App Name"` for opening workspaces in editors - Use URL schemes (cursor://file, vscode://file) for opening files at specific line/column positions This avoids PATH issues in production builds where Electron apps launched from Finder/Dock don't inherit the user's shell PATH. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
saddlepaddle
force-pushed
the
fix/open-in-app-path-production
branch
from
ade2cb6 to
e1f5c43
Compare
drizzle's better-sqlite3 migrator can record a migration as applied while its ALTER statement silently rolls back, leaving the schema missing a column. When the upstream 1.5.8 merge brought migration 0040_agent_preset_permissions_migrated_at, some users booted Superset into a state where that column was absent, causing every settings- touching tRPC query to throw 'no such column' and cascading into a React superset-sh#185 loop on the new-tab/presets UI. After migrate(), walk every drizzle table exported from @superset/ local-db and compare its declared columns against PRAGMA table_info. Any simple missing column is backfilled with ALTER TABLE ADD COLUMN (type from column.getSQLType(), default from column.default). Primary keys and NOT NULL without default are skipped; failures per column are logged and do not abort the rest.
1 participant
Summary
codeandcursorto not be foundgetSupersetPath()which includes the user's full PATH (same approach as terminal-manager)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.