fix(desktop): resolve gh CLI PATH issue when launched from Finder/Dock#591
fix(desktop): resolve gh CLI PATH issue when launched from Finder/Dock#591
Conversation
📝 WalkthroughWalkthroughAdds a macOS-aware command runner Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Desktop App
participant Exec as execWithShellEnv
participant GH as gh CLI (child process)
participant OS as macOS Shell Environment
App->>Exec: run `gh ...` (cmd,args)
Exec->>GH: attempt execFile(cmd,args)
alt success
GH-->>Exec: stdout/stderr
Exec-->>App: return output
else ENOENT on macOS and not pathFixAttempted
Exec->>OS: getShellEnvironment(shell=/bin/zsh)
OS-->>Exec: env (PATH,...)
Exec->>Exec: set process.env.PATH (cache)
Exec->>GH: retry execFile(cmd,args) with updated env
alt retry success
GH-->>Exec: stdout/stderr
Exec-->>App: return output
else retry fails
GH-->>Exec: error
Exec-->>App: throw original error
end
else other error or non-macOS
GH-->>Exec: error
Exec-->>App: throw error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)apps/desktop/**/*.{ts,tsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts (1)
⏰ 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 (3)
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
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts (1)
122-126: Consider using an object parameter for better extensibility.The function has 3 parameters. Per coding guidelines, functions with 2+ parameters should use a single params object for self-documentation and extensibility.
🔎 Suggested refactor
-export async function execWithShellEnv( - cmd: string, - args: string[], - options?: Omit<ExecFileOptionsWithStringEncoding, "encoding">, -): Promise<{ stdout: string; stderr: string }> { +export async function execWithShellEnv(params: { + cmd: string; + args: string[]; + options?: Omit<ExecFileOptionsWithStringEncoding, "encoding">; +}): Promise<{ stdout: string; stderr: string }> { + const { cmd, args, options } = params;Note: This would require updating callers in
github.tsas well. Given the function is currently internal and has limited callers, this refactor could be deferred if preferred.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: 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.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts
🧠 Learnings (3)
📚 Learning: 2026-01-02T06:50:28.662Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts
📚 Learning: 2026-01-02T06:50:28.662Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules like node:path or dotenv in electron-router-dom.ts and similar shared files - they run in both main and renderer processes
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts
🧬 Code graph analysis (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts (1)
execWithShellEnv(122-172)
⏰ 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/lib/trpc/routers/workspaces/utils/shell-env.ts (4)
1-19: LGTM on imports and state tracking setup.The module-level flags
pathFixAttemptedandpathFixSucceededcorrectly implement the lazy fix strategy described in the PR. The separation allows for retry if derivation previously failed while avoiding redundant attempts after success.
39-41: LGTM on zsh fallback.Using
/bin/zshas the macOS default is the right choice since it's been the default shell since Catalina and Homebrew typically configures PATH in zsh profiles.
127-141: LGTM on the retry guard logic.The condition correctly gates retry attempts: only on macOS, only on ENOENT, and only when PATH hasn't been successfully fixed yet. The
pathFixAttemptedguard prevents concurrent fix attempts during in-flight derivation.
146-165: LGTM on environment derivation and retry flow.The logic correctly:
- Persists PATH to
process.envfor subsequent calls- Forces
shellEnv.PATHin the retry environment to override any caller-provided PATH- Maintains caller's other env vars via
options?.envapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts (4)
1-12: LGTM on imports.Both
execFile(forgitwhich is at/usr/bin/git) andexecWithShellEnv(forghwhich may be in Homebrew paths) are appropriately retained.
70-88: LGTM on getRepoUrl update.Correctly routes
ghCLI invocation through the new shell-env wrapper while maintaining existing error handling and schema validation.
90-141: LGTM on getPRForBranch update.The switch to
execWithShellEnvis clean. The comment on line 95 provides good context. Error handling for "no pull requests found" is correctly preserved.
40-44: Verify git command doesn't need the shell-env wrapper.The
gitcommand still usesexecFileAsyncdirectly. This should be fine sincegitis typically at/usr/bin/gitwhich is in the minimal PATH, but worth confirming this works in Finder-launched scenarios.If
gitis user-installed (e.g., via Homebrew for a newer version), this could fail with the same ENOENT issue. Consider whether this should also useexecWithShellEnvfor consistency, or add a note explaining why it's safe to use the direct call.
On macOS, GUI apps launched from Finder/Dock get a minimal PATH that excludes homebrew (/opt/homebrew/bin) and other user-installed tools. This caused GitHub PR status to silently fail when the desktop app wasn't launched from a terminal. - Add execWithShellEnv() helper that retries on ENOENT with shell env - Persist process.env.PATH after fix so all subsequent calls benefit - Use /bin/zsh fallback on darwin (homebrew configures PATH in zsh) - Route gh CLI calls through the new helper
andreasasprou
force-pushed
the
religious-mole-b5cf56
branch
from
a786f8a to
2cda6f8
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
2 participants
Summary
Why / Context
On macOS, GUI apps launched from Finder/Dock get a minimal PATH (
/usr/bin:/bin:/usr/sbin:/sbin) that excludes homebrew (/opt/homebrew/bin) and other user-installed tools. This causedghCLI calls to silently fail with ENOENT.Terminal sessions worked because shell wrappers source user's
.zprofile/.zshrcwhich configure PATH properly.How It Works
execWithShellEnv()wraps command execution with ENOENT retry logicgetShellEnvironment()process.env.PATHso all subsequent calls benefit (critical for multi-call flows likefetchGitHubPRStatuswhich calls bothgh repo viewandgh pr view)pathFixAttemptedvspathFixSucceededseparately - if derivation fails transiently, future ENOENTs can retryDesign Decisions
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.