fix(desktop): resolve shell environment for macOS GUI apps#720
Conversation
On macOS, GUI apps launched via Finder/Spotlight/Dock don't inherit environment variables from the user's shell configuration. This causes issues with custom ZDOTDIR configurations not being detected and PATH missing user-installed tools. This change uses the shell-env package to spawn a login shell at startup and capture the user's full environment, persisting ZDOTDIR and PATH to process.env before terminal initialization. Fixes #701 🤖 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 that can be reviewed per hour. Please wait 16 minutes and 33 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 ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new dependency and a startup utility that captures the user shell environment (ZDOTDIR and PATH) on macOS/Linux and injects it into the desktop process before agent/terminal hooks are initialized, with defensive error handling and tests for PATH merge behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Desktop Main
participant Auth as AuthService / init
participant ShellUtil as ensureShellEnvVars
participant OS as User Shell (macOS/Linux)
participant Agent as setupAgentHooks
App->>Auth: initAppState() / authService.initialize()
App->>ShellUtil: ensureShellEnvVars()
ShellUtil->>OS: shellEnv() (capture login shell env)
OS-->>ShellUtil: env (PATH, ZDOTDIR, ...)
ShellUtil-->>App: set process.env (PATH, ZDOTDIR)
App->>Agent: setupAgentHooks() (uses PATH/ZDOTDIR)
Agent-->>App: agent hooks initialized
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
- Add 5-second timeout to prevent app hang on slow/broken shell configs - Merge PATH instead of replacing to preserve Electron runtime paths - Use TTY detection instead of ZDOTDIR check for terminal launch detection 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Kitenite
force-pushed
the
debug-shell-config
branch
from
64a08cf to
ea25013
Compare
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![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/main/lib/shell-env.test.ts (1)
72-78: Consider adding a clarifying comment for the asymmetric filtering behavior.This test documents that empty segments are filtered from shell input (for deduplication logic) but preserved in the original
PATH. The expected result/new/path:/usr/bin::/binretains the::from the original.This is reasonable behavior (avoid modifying user's PATH structure), but the test name "should filter empty path segments" might be slightly misleading since it only filters shell input, not the original PATH. A brief inline comment would improve clarity.
📝 Suggested clarifying comment
it("should filter empty path segments", () => { + // Empty segments are filtered from shell input for deduplication, + // but original PATH structure (including empty segments) is preserved process.env.PATH = "/usr/bin::/bin"; const result = mergePathFromShell("/new/path:::/usr/bin"); expect(result).toBe(true); expect(process.env.PATH).toBe("/new/path:/usr/bin::/bin"); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/main/lib/shell-env.test.tsapps/desktop/src/main/lib/shell-env.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/lib/shell-env.ts
🧰 Additional context used
📓 Path-based instructions (5)
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/main/lib/shell-env.test.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/main/lib/shell-env.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate tests with implementation files using .test.ts or .test.tsx suffix
Files:
apps/desktop/src/main/lib/shell-env.test.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/main/lib/shell-env.test.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/main/lib/shell-env.test.ts
🧠 Learnings (2)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
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/main/lib/shell-env.test.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
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/main/lib/shell-env.test.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/shell-env.test.ts (1)
apps/desktop/src/main/lib/shell-env.ts (1)
mergePathFromShell(9-21)
🔇 Additional comments (3)
apps/desktop/src/main/lib/shell-env.test.ts (3)
1-14: LGTM!Clean test setup with proper environment isolation. The
beforeEach/afterEachpattern correctly saves and restoresprocess.env.PATHto prevent test pollution.
16-70: Good test coverage for core functionality and edge cases.The test cases correctly verify:
- Prepending behavior for new shell paths
- No-op when all paths already exist
- Preservation of existing runtime paths
- Handling of empty/undefined
PATH- Deduplication logic
All expectations align with the implementation logic.
80-86: LGTM!Good test for verifying that the order of new entries from the shell is preserved when prepended to the existing PATH.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
)" This reverts commit 4a41f05.
1 participant
Summary
shell-envpackage to resolve shell environment variables for macOS GUI appsensureShellEnvVars()function that spawns a login shell at startup to captureZDOTDIRandPATHsetupAgentHooks()so terminal wrappers get the correct config pathsProblem
On macOS, GUI apps launched via Finder/Spotlight/Dock don't inherit environment variables from the user's shell configuration (
.zshrc,.zprofile, etc.). This causes:ZDOTDIRconfigurations not being detected$HOMEinstead of the user's actual config directorySolution
Use the battle-tested
shell-envpackage (same approach as VS Code and Hyper) to:-ilcflags at app startupZDOTDIRandPATHtoprocess.envTest plan
ZDOTDIRset in shell profileFixes #701
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.