fix(desktop): bundle git binary to avoid system git dependency #356
Conversation
|
Warning Rate limit exceeded@AviPeltz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 55 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)
WalkthroughReplace system Git usage with a bundled Dugite Git: add Dugite dependency and packaging rules, introduce a git-binary helper (getGitBinaryPath/getGitExecPath/createBundledGit), switch code/tests to use the bundled Git, remove shell-env, and update packaging/copy logic to include Dugite. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Router as Router / Procedure
participant GitHelper as main/lib/git-binary
participant SimpleGit as simple-git wrapper
participant Dugite as dugite (bundled)
participant GitExec as Git CLI
Router->>GitHelper: createBundledGit(baseDir?)
GitHelper->>Dugite: resolve paths (dev: resolveGitBinary/exec via require OR packaged: compute app.asar.unpacked path)
Dugite-->>GitHelper: binaryPath / execPath
GitHelper->>SimpleGit: instantiate configured client (binary: binaryPath, libexec: execPath)
SimpleGit-->>Router: configured git client
Router->>SimpleGit: perform git operation (clone/fetch/ls-files/etc)
SimpleGit->>GitExec: execute command using bundled git binary
GitExec-->>SimpleGit: command result / error
SimpleGit-->>Router: operation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
saddlepaddle
force-pushed
the
extended-pheasant-7faf6d
branch
from
0dafd72 to
3588dc3
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/package.json (1)
33-99: Pindugiteversion and add GPL compliance notices for bundled Git.The dependency
"dugite": "^3.0.0"bundles portable Git binaries that are unpacked into the Electron app (seeelectron-builder.ts). The caret range allows silent version changes across builds, potentially altering bundled Git payload and behavior. More critically, since dugite's embedded Git is GPLv2-licensed, redistributing these binaries in the packaged app requires adding GPL compliance notices (license text and copyright) to the app. The repository LICENSE.md (Apache 2.0) does not currently include these notices; add Git and git-lfs license text to comply with GPLv2 redistribution obligations, and consider pinning to a specific version for deterministic releases.
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
8-22:createGit()should be shared + set exec/LFS environment (and avoid the one-liner).This repeats the same
createGitpattern found in other routers; consider extracting a single helper (e.g.,src/main/lib/create-git.ts) to ensure consistent config. Also, similar to other callsites, consider setting env (GIT_EXEC_PATH/PATH) so bundled Git can reliably find helpers andgit-lfs. The current ternary one-liner is also hard to read/maintain.apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
25-84:repoUsesLfs()is a good robustness improvement; watchgit ls-filescost on huge repos.The layered checks make sense. Minor consideration:
git ls-filescan be very large; if this ends up on a hot path, consider a cheaper/limited query.apps/desktop/src/lib/trpc/routers/changes/changes.ts (1)
1-25: Import and wiregetGitExecPath()increateGit()for robust Git helper/LFS support.The
createGit()consolidation is good, butgetGitExecPath()is documented as "required for some git operations to find helper binaries" and is currently unused. ExtendcreateGit()to set the exec path:import { getGitBinaryPath, getGitExecPath } from "main/lib/git-binary"; function createGit(baseDir: string) { return simpleGit({ baseDir, binary: getGitBinaryPath() }).env({ ...process.env, GIT_EXEC_PATH: getGitExecPath(), }); }This ensures bundled git can locate helper executables and git-lfs across platforms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
apps/desktop/electron-builder.ts(2 hunks)apps/desktop/package.json(1 hunks)apps/desktop/src/lib/trpc/routers/changes/changes.ts(10 hunks)apps/desktop/src/lib/trpc/routers/projects/projects.ts(4 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts(0 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts(9 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts(0 hunks)apps/desktop/src/main/lib/git-binary.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/package.jsonapps/desktop/src/main/lib/git-binary.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/electron-builder.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
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/git-binary.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/electron-builder.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.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/git-binary.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/electron-builder.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/main/lib/git-binary.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/electron-builder.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
apps/desktop/src/main/lib/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement IPC handlers accepting object parameters (not positional parameters) in apps/desktop/src/main/lib/*.ts files
Files:
apps/desktop/src/main/lib/git-binary.ts
🧬 Code graph analysis (3)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
apps/desktop/src/main/lib/git-binary.ts (1)
getGitBinaryPath(11-16)
apps/desktop/src/lib/trpc/routers/changes/changes.ts (1)
apps/desktop/src/main/lib/git-binary.ts (1)
getGitBinaryPath(11-16)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
apps/desktop/src/main/lib/git-binary.ts (1)
getGitBinaryPath(11-16)
⏰ 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). (6)
- GitHub Check: Deploy API
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Web
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Build
🔇 Additional comments (3)
apps/desktop/electron-builder.ts (1)
35-41: Verify macOS code-signing/notarization for unpacked Dugite binaries.Unpacking/copying Dugite’s
git/**/*is the right direction, but please verify on macOS (hardened runtime + notarization) that the embeddedgit/git-lfsexecutables are actually runnable post-install (code signing can be finicky for nested binaries underapp.asar.unpacked). Optionally consider narrowing the copy filter to only whatdugiteneeds at runtime if size becomes an issue.Also applies to: 59-64
apps/desktop/src/lib/trpc/routers/changes/changes.ts (1)
39-412: Callsite updates tocreateGit(input.worktreePath)are consistent.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
199-377: UsingcreateGit()ininitGitAndOpenandcloneRepois the right integration point.
| /** | ||
| * Builds the merged environment for git operations. | ||
| * Takes process.env as base, then overrides only PATH from shell environment. | ||
| * This preserves runtime vars (git credentials, proxy, ELECTRON_*, etc.) | ||
| * while picking up PATH modifications from shell profiles (e.g., homebrew git-lfs). | ||
| * Creates a simpleGit instance configured to use the bundled git binary. | ||
| * This avoids dependency on system git (no Xcode license issues on macOS, etc.) | ||
| */ | ||
| async function getGitEnv(): Promise<Record<string, string>> { | ||
| const shellEnv = await getShellEnvironment(); | ||
| const result: Record<string, string> = {}; | ||
|
|
||
| // Start with process.env as base | ||
| for (const [key, value] of Object.entries(process.env)) { | ||
| if (typeof value === "string") { | ||
| result[key] = value; | ||
| } | ||
| } | ||
|
|
||
| // Only override PATH from shell env (use platform-appropriate key) | ||
| const pathKey = process.platform === "win32" ? "Path" : "PATH"; | ||
| if (shellEnv[pathKey]) { | ||
| result[pathKey] = shellEnv[pathKey]; | ||
| } | ||
|
|
||
| return result; | ||
| function createGit(baseDir: string) { | ||
| return simpleGit({ baseDir, binary: getGitBinaryPath() }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/desktop && find . -type f \( -name "*.ts" -o -name "*.tsx" \) | grep -E "(git|trpc)" | head -20Repository: superset-sh/superset
Length of output: 929
🏁 Script executed:
rg "getGitBinaryPath|getGitExecPath" apps/desktop --type ts -A 2Repository: superset-sh/superset
Length of output: 2723
🏁 Script executed:
rg "createGit" apps/desktop --type ts -B 2 -A 2Repository: superset-sh/superset
Length of output: 11051
🏁 Script executed:
rg "execFileAsync" apps/desktop --type ts -B 3 -A 3Repository: superset-sh/superset
Length of output: 4540
🏁 Script executed:
wc -l apps/desktop/src/main/lib/git-binary.tsRepository: superset-sh/superset
Length of output: 106
🏁 Script executed:
cat -n apps/desktop/src/main/lib/git-binary.tsRepository: superset-sh/superset
Length of output: 993
🏁 Script executed:
cat apps/desktop/package.json | grep -E "simple-git|dugite" -A 2 -B 2Repository: superset-sh/superset
Length of output: 344
🏁 Script executed:
cd apps/desktop && cat -n src/lib/trpc/routers/workspaces/utils/git.ts | sed -n '110,130p'Repository: superset-sh/superset
Length of output: 868
🏁 Script executed:
cd apps/desktop && grep -n "execFileAsync" src/lib/trpc/routers/workspaces/utils/git.tsRepository: superset-sh/superset
Length of output: 163
🏁 Script executed:
cd apps/desktop && cat -n src/lib/trpc/routers/workspaces/utils/git.ts | sed -n '160,175p'Repository: superset-sh/superset
Length of output: 512
🏁 Script executed:
rg "env.*GIT_EXEC_PATH|GIT_EXEC_PATH.*env" apps/desktop --type tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
rg "new SimpleGit|simpleGit(" apps/desktop --type ts -A 3 | head -40Repository: superset-sh/superset
Length of output: 162
🏁 Script executed:
rg "env:" apps/desktop --type ts | head -20Repository: superset-sh/superset
Length of output: 414
🏁 Script executed:
cd apps/desktop && cat src/lib/trpc/routers/workspaces/utils/teardown.tsRepository: superset-sh/superset
Length of output: 1881
🏁 Script executed:
cd apps/desktop && node -e "const SimpleGit = require('simple-git').default; const instance = new SimpleGit(); console.log(Object.keys(instance).slice(0, 20))"Repository: superset-sh/superset
Length of output: 927
🏁 Script executed:
rg "SimpleGitOptions|type.*GitOptions" apps/desktop --type tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
find apps/desktop -name "*.d.ts" | xargs grep -l "simple-git" 2>/dev/null | head -5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
cd apps/desktop && cat -n src/lib/trpc/routers/workspaces/utils/git.ts | sed -n '25,100p'Repository: superset-sh/superset
Length of output: 2496
🏁 Script executed:
cd apps/desktop && cat -n src/lib/trpc/routers/workspaces/utils/git.ts | tail -20Repository: superset-sh/superset
Length of output: 543
Pass environment variables (GIT_EXEC_PATH) to git spawning so git-lfs and helper binaries are discoverable.
Both execFileAsync calls (worktree add/remove at lines 119–123, 168–172) and all simpleGit instantiations via createGit need to include environment variables. Without GIT_EXEC_PATH, git-lfs and other bundled helper binaries won't be found, causing operations to fail in LFS-enabled repositories. Use getGitExecPath() (already exported from main/lib/git-binary.ts) and pass it as GIT_EXEC_PATH in the options, following the pattern in teardown.ts: { ...process.env, GIT_EXEC_PATH: getGitExecPath() }.
🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts around lines 16–23
(and also for the execFileAsync calls at lines 119–123 and 168–172), the git
invocations do not pass GIT_EXEC_PATH so git-lfs and helper binaries aren’t
found; import and call getGitExecPath() and merge it into the environment when
spawning git by passing options like { env: { ...process.env, GIT_EXEC_PATH:
getGitExecPath() } } to execFileAsync, and update createGit to instantiate
simpleGit with the same merged env (e.g., simpleGit({ baseDir, binary:
getGitBinaryPath(), env: { ...process.env, GIT_EXEC_PATH: getGitExecPath() } }))
so all git operations can discover bundled helper binaries.
| import { resolveGitBinary, resolveGitExecPath } from "dugite"; | ||
|
|
||
| let cachedGitPath: string | null = null; | ||
| let cachedGitExecPath: string | null = null; | ||
|
|
||
| /** | ||
| * Returns the path to the bundled git binary. | ||
| * Uses dugite's embedded git so we don't depend on system git | ||
| * (avoids Xcode license issues on macOS, missing git on Windows, etc.) | ||
| */ | ||
| export function getGitBinaryPath(): string { | ||
| if (!cachedGitPath) { | ||
| cachedGitPath = resolveGitBinary(); | ||
| } | ||
| return cachedGitPath; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the git exec path for the bundled git. | ||
| * Required for some git operations to find helper binaries. | ||
| */ | ||
| export function getGitExecPath(): string { | ||
| if (!cachedGitExecPath) { | ||
| cachedGitExecPath = resolveGitExecPath(); | ||
| } | ||
| return cachedGitExecPath; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for usages of getGitBinaryPath and getGitExecPath
rg "getGitBinaryPath|getGitExecPath" --type ts --type tsx -B 2 -A 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check if there's a pattern of setting GIT_EXEC_PATH or bundled git env elsewhere
rg "GIT_EXEC_PATH|git.*env|execFile|spawn.*git" apps/desktop/src --type ts --type tsx -lRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Look for child process spawning patterns in the desktop app
fd "\.ts$" apps/desktop/src | head -20 | xargs grep -l "spawn\|execFile" 2>/dev/nullRepository: superset-sh/superset
Length of output: 115
🌐 Web query:
dugite npm package GIT_EXEC_PATH environment variable requirements git-lfs helper binaries
💡 Result:
Short answer: dugite runs the git CLI binary (packaged or system) so helper programs (git-lfs, credential helpers, git-*-core helpers) must be discoverable by Git itself — either on PATH or in Git’s exec-path (GIT_EXEC_PATH). If you need git-lfs support when using dugite, install git-lfs (or a Git distribution that includes it) and make sure git can find the helper (via PATH or by setting GIT_EXEC_PATH to the git libexec/git-core directory). Sources: dugite package docs and Git/Git LFS docs.
Key references:
- dugite (npm / repo): dugite is a thin binding that invokes the git CLI. [1]
- git --exec-path / GIT_EXEC_PATH: Git uses --exec-path or the GIT_EXEC_PATH env var to locate core helper programs. Set it to the directory containing git's helper binaries (typically .../libexec/git-core). [2]
- Git LFS: git-lfs must be installed and on PATH (or otherwise available to Git) for LFS-enabled repos; otherwise Git operations will error that 'git-lfs' was not found. [3]
If you want, tell me your OS and whether you're using the packaged Git that came with GitHub Desktop or a system git, and I’ll give exact commands to verify/set GIT_EXEC_PATH or install/locate git-lfs.
References:
[1] dugite npm / repo pages.
[2] git documentation: --exec-path / GIT_EXEC_PATH.
[3] Git LFS / Atlassian guidance about 'git-lfs' needing to be on PATH.
🏁 Script executed:
# Search for usages of getGitBinaryPath and getGitExecPath
rg "getGitBinaryPath|getGitExecPath" apps/desktop/src --type ts -B 2 -A 2Repository: superset-sh/superset
Length of output: 4343
🏁 Script executed:
# Look for child process spawning with git
rg "spawn|execFile|exec\(" apps/desktop/src --type ts -l | head -10Repository: superset-sh/superset
Length of output: 548
🏁 Script executed:
# Check if GIT_EXEC_PATH is being set anywhere
rg "GIT_EXEC_PATH" apps/desktop/src --type tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look at the structure of apps/desktop/src to understand the architecture
fd "\.ts$" apps/desktop/src | grep -E "(git|spawn|process)" | head -20Repository: superset-sh/superset
Length of output: 426
🏁 Script executed:
# Check for any usage of getGitExecPath
rg "getGitExecPath" apps/desktop/src --type tsRepository: superset-sh/superset
Length of output: 146
🏁 Script executed:
# Look at the git.ts file to see how simpleGit is configured and if env is set
cat apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts | head -100Repository: superset-sh/superset
Length of output: 2678
🏁 Script executed:
# Check execFileAsync calls to see if env parameter is passed
rg "execFileAsync" apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts -A 3 -B 1Repository: superset-sh/superset
Length of output: 497
🏁 Script executed:
# Look at simpleGit usage more carefully in projects.ts
rg "simpleGit|createGit" apps/desktop/src/lib/trpc/routers/projects/projects.ts -B 2 -A 5Repository: superset-sh/superset
Length of output: 1325
🏁 Script executed:
# Check how simple-git handles environment variables
rg "simple-git" apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts -A 20 | head -40Repository: superset-sh/superset
Length of output: 605
🌐 Web query:
simple-git npm options environment variable GIT_EXEC_PATH configuration
💡 Result:
Short answer
- simple-git can pass GIT_EXEC_PATH to the spawned git child process using .env(...) or by passing a full env object. Use .customBinary(...) if you want a different git executable. (npmjs.com)
Examples
-
Set GIT_EXEC_PATH only for the child process:
const git = require('simple-git')();
git.env('GIT_EXEC_PATH', '/path/to/git/libexec').status(); -
Merge with existing process.env:
git.env({ ...process.env, GIT_EXEC_PATH: '/path/to/git/libexec' }).status(); -
Use a different git binary instead of changing exec path:
const git = require('simple-git')('/repo/path', { binary: '/usr/local/bin/git' });
References
- simple-git env and customBinary docs. (npmjs.com)
- GIT_EXEC_PATH purpose (where Git looks for its sub-programs / git --exec-path). (git-scm.com)
Citations:
- 1: https://www.npmjs.com/package/simple-git?utm_source=openai
- 2: https://www.npmjs.com/package/simple-git?utm_source=openai
- 3: https://git-scm.com/book/en/v2/Git-Internals-Environment-Variables.html?utm_source=openai
Export a getBundledGitEnv() helper to properly set GIT_EXEC_PATH for git-lfs and helper binaries.
The getGitExecPath() function is defined but never used, and GIT_EXEC_PATH is not being set anywhere in the codebase. Without it, git-lfs and helper binaries won't work reliably. Since simpleGit supports .env() and execFileAsync accepts environment configuration, create a centralized helper like:
export function getBundledGitEnv(): NodeJS.ProcessEnv {
return {
...process.env,
GIT_EXEC_PATH: getGitExecPath(),
};
}Then use it consistently across all simpleGit() and execFileAsync() calls (in workspaces/utils/git.ts, projects/projects.ts, changes/changes.ts). This ensures git-lfs and other helpers are discoverable.
Minor: also prefer explicit null checks (cachedGitPath === null) instead of falsy checks to avoid treating empty strings as cache misses.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/git-binary.ts around lines 1 to 27, add and export
a getBundledGitEnv() helper that returns process.env merged with GIT_EXEC_PATH
set to getGitExecPath(), update the module to use strict null checks (check
cachedGitPath === null and cachedGitExecPath === null) when populating caches,
and export getBundledGitEnv so callers (simpleGit().env(...) and execFileAsync
calls in workspaces/utils/git.ts, projects/projects.ts, changes/changes.ts) can
pass the returned env to ensure git-lfs and helper binaries find the bundled git
exec path.
b8b8cdb to
31dacbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/desktop/src/main/lib/git-binary.ts (1)
12-13: WireGIT_EXEC_PATHcentrally (git-lfs/helpers) + use strict null checks.
You already exposegetGitExecPath(), but nothing here (or in call sites) setsGIT_EXEC_PATH. That’s a common reason bundled-git + git-lfs/other helpers fail to run reliably. Also, prefercachedGitPath === null/cachedGitExecPath === nullover falsy checks.let cachedGitPath: string | null = null; let cachedGitExecPath: string | null = null; +export function getBundledGitEnv(): NodeJS.ProcessEnv { + return { + ...process.env, + GIT_EXEC_PATH: getGitExecPath(), + }; +} export function getGitBinaryPath(): string { - if (!cachedGitPath) { + if (cachedGitPath === null) { const { resolveGitBinary } = dynamicRequire( "dugite", ) as typeof import("dugite"); cachedGitPath = resolveGitBinary(); } return cachedGitPath; } export function getGitExecPath(): string { - if (!cachedGitExecPath) { + if (cachedGitExecPath === null) { const { resolveGitExecPath } = dynamicRequire( "dugite", ) as typeof import("dugite"); cachedGitExecPath = resolveGitExecPath(); } return cachedGitExecPath; }Also applies to: 24-46
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
6-6: Pass bundled-git env toexecFileAsync(git-lfs/helpers) or guarantee it centrally.
Right now the worktreeexecFileAsync(getGitBinaryPath(), ...)calls don’t passenv, so if the bundled Git needsGIT_EXEC_PATHfor helpers (incl. git-lfs), these operations can still fail on some setups. If you addgetBundledGitEnv()inmain/lib/git-binary.ts, use it here.- await execFileAsync( + await execFileAsync( getGitBinaryPath(), [ "-C", mainRepoPath, "worktree", "add", worktreePath, "-b", branch, startPoint, ], - { timeout: 120_000 }, + { + timeout: 120_000, + // env: getBundledGitEnv(), // import from main/lib/git-binary if added + }, );- await execFileAsync( + await execFileAsync( getGitBinaryPath(), ["-C", mainRepoPath, "worktree", "remove", worktreePath, "--force"], - { timeout: 60_000 }, + { + timeout: 60_000, + // env: getBundledGitEnv(), + }, );Also applies to: 55-55, 110-123, 168-172
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts (1)
4-4: Add a timeout (and consider passing bundled-git env) for therev-parseexec.
If git hangs (network FS / locked repo), this can block status refresh. Also, if you add a centralgetBundledGitEnv()(seegit-binary.ts), passing it here keeps git invocations consistent.- const { stdout: branchOutput } = await execFileAsync( - getGitBinaryPath(), - ["rev-parse", "--abbrev-ref", "HEAD"], - { cwd: worktreePath }, - ); + const { stdout: branchOutput } = await execFileAsync( + getGitBinaryPath(), + ["rev-parse", "--abbrev-ref", "HEAD"], + { + cwd: worktreePath, + timeout: 10_000, + // env: getBundledGitEnv(), // if introduced in main/lib/git-binary + }, + );Also applies to: 40-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/changes/changes.ts(9 hunks)apps/desktop/src/lib/trpc/routers/projects/projects.ts(3 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts(10 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts(2 hunks)apps/desktop/src/main/lib/git-binary.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/lib/trpc/routers/projects/projects.ts
🧰 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/changes/changes.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/main/lib/git-binary.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/changes/changes.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/main/lib/git-binary.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/main/lib/git-binary.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/main/lib/git-binary.ts
apps/desktop/src/main/lib/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement IPC handlers accepting object parameters (not positional parameters) in apps/desktop/src/main/lib/*.ts files
Files:
apps/desktop/src/main/lib/git-binary.ts
🧬 Code graph analysis (3)
apps/desktop/src/lib/trpc/routers/changes/changes.ts (1)
apps/desktop/src/main/lib/git-binary.ts (1)
createBundledGit(55-76)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
apps/desktop/src/main/lib/git-binary.ts (2)
createBundledGit(55-76)getGitBinaryPath(24-32)
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts (1)
apps/desktop/src/main/lib/git-binary.ts (1)
getGitBinaryPath(24-32)
⏰ 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). (6)
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy API
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/lib/trpc/routers/changes/changes.ts (1)
3-3: Good consolidation tocreateBundledGit(...); validate bundled-git env supports helper binaries (git-lfs).
This router looks fine, but its correctness for LFS-enabled repos now depends oncreateBundledGitensuring Git can find bundled helpers (typically viaGIT_EXEC_PATH). Please confirm that’s handled centrally inapps/desktop/src/main/lib/git-binary.ts.Also applies to: 31-31, 80-80, 196-196, 238-238, 333-333, 346-346, 359-359, 373-373, 381-381
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
183-183:createBundledGit(...)usage is consistent; prefer centralizing env/timeouts there.
These call sites look good; if you implementgit.env(getBundledGitEnv())insidecreateBundledGit, you won’t need to remember env wiring across all these helpers.Also applies to: 196-196, 209-209, 218-218, 250-250, 260-260, 272-272, 280-280, 308-308
| export function createBundledGit(baseDir?: string): SimpleGit { | ||
| const simpleGit = dynamicRequire("simple-git").default as ( | ||
| opts?: Partial<SimpleGitOptions>, | ||
| ) => SimpleGit; | ||
|
|
||
| // Temporarily suppress the specific warning from simple-git | ||
| const originalWarn = console.warn; | ||
| console.warn = (...args: unknown[]) => { | ||
| if (args[0] === SIMPLE_GIT_BINARY_WARNING) return; | ||
| originalWarn.apply(console, args); | ||
| }; | ||
|
|
||
| try { | ||
| return simpleGit({ | ||
| ...(baseDir && { baseDir }), | ||
| binary: getGitBinaryPath(), | ||
| unsafe: { allowUnsafeCustomBinary: true }, | ||
| }); | ||
| } finally { | ||
| console.warn = originalWarn; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.ts" -path "*/desktop/src/main/lib/*" | head -20Repository: superset-sh/superset
Length of output: 996
🏁 Script executed:
find . -type f -name "*.ts" -path "*/desktop/src/main/lib/*" | head -20Repository: superset-sh/superset
Length of output: 996
🏁 Script executed:
# Search for getBundledGitEnv function
rg "getBundledGitEnv" --type ts --type tsx -A 3Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check simple-git type definitions and usage patterns
rg "SimpleGit" --type ts -A 2 | head -50Repository: superset-sh/superset
Length of output: 4010
🏁 Script executed:
# Look for console.warn patching or similar patterns
rg "console\.warn\s*=" --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check if .env() method is used elsewhere with SimpleGit
rg "\.env\(" --type ts --type tsx -B 2 -A 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Search for getBundledGitEnv without type filters
rg "getBundledGitEnv" -A 5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for .env( method call without type filters
rg "\.env\(" -B 2 -A 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look at simple-git node_modules to check API
find node_modules/simple-git -name "*.d.ts" | head -5Repository: superset-sh/superset
Length of output: 122
🏁 Script executed:
head -100 apps/desktop/src/main/lib/git-binary.tsRepository: superset-sh/superset
Length of output: 2642
🏁 Script executed:
# Search entire codebase for getBundledGitEnv
rg "getBundledGitEnv"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for bundled env related functions
rg "BundledGitEnv|bundledGitEnv" -A 3Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look for env-related exports in terminal or git modules
rg "export.*env|export.*Env" apps/desktop/src/main/lib/terminal/Repository: superset-sh/superset
Length of output: 330
🏁 Script executed:
# Search for how SimpleGit is used in routers
rg "createBundledGit|SimpleGit" -A 5 apps/desktop/src/lib/trpc/Repository: superset-sh/superset
Length of output: 22200
🏁 Script executed:
# Check if simple-git has logger/silent options in any comments or docs
rg "silent|logger|log" apps/desktop/src/main/lib/git-binary.tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search package.json for simple-git version
rg "simple-git" package.jsonRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's a terminal/env.ts that might have buildTerminalEnv
head -80 apps/desktop/src/main/lib/terminal/env.tsRepository: superset-sh/superset
Length of output: 1773
🏁 Script executed:
# Search for any existing bundled env patterns elsewhere
rg "buildTerminalEnv|getTerminalEnv|terminalEnv" -A 3Repository: superset-sh/superset
Length of output: 4063
🏁 Script executed:
# Look for simple-git usage with additional options or patterns
rg "simpleGit\(" -B 2 -A 4Repository: superset-sh/superset
Length of output: 487
🏁 Script executed:
# Get the entire package.json to find simple-git version
cat package.json | head -100Repository: superset-sh/superset
Length of output: 1464
🌐 Web query:
simple-git SimpleGit .env method type definitions
💡 Result:
The SimpleGit .env TypeScript signatures are:
- (name: string, value: string) => this
- (env: object) => this
i.e. in the SimpleGit interface:
env(name: string, value: string): this;
env(env: object): this;
Usage examples: git.env('GIT_SSH_COMMAND', '...') or git.env({ ...process.env, GIT_SSH_COMMAND }). [1][2]
Sources:
[1] simple-git API docs (env method).
[2] simple-git distributed typings / promise.d.ts (package typings).
🌐 Web query:
simple-git logger silent option configuration
💡 Result:
Short answer:
- simple-git historically exposes git().silent(true|false) to turn console logging off/on, but that method was deprecated in v2.7.0 and will be removed in v3.0.0 — prefer the debug library instead. [1][2]
How to silence / enable logging
-
Legacy (still works but deprecated):
- git().silent(true) // silence
- git().silent(false) // enable
- Note: .silent() defaults to true when NODE_ENV contains "prod". [1][2]
-
Recommended (use debug):
- Disable (no DEBUG set): do nothing — debug is off by default.
- Enable logging for simple-git:
- Environment variable: DEBUG=simple-git node app.js
- Or in code: require('debug').enable('simple-git')
- For verbose output you can use namespaces like simple-git:task:* or simple-git:output:* . [3]
Sources
- simple-git API / .silent docs and notes. [1]
- simple-git v2.7.0 changelog deprecating .silent in favour of debug. [2]
- Readme / Troubleshooting: how to enable logging via DEBUG and namespace examples. [3]
🏁 Script executed:
# Check if there's any environment setup for git in terminal or other modules
rg "GIT_" --max-count=20Repository: superset-sh/superset
Length of output: 230
🏁 Script executed:
# Look at the full terminal/env.ts to see if it includes GIT_ variables
cat apps/desktop/src/main/lib/terminal/env.ts | tail -100Repository: superset-sh/superset
Length of output: 2136
🏁 Script executed:
# Search for any comments about env vars or bundled git
rg "bundled|dugite|env|GIT_LFS" apps/desktop/src/main/lib/git-binary.ts -B 2 -A 2Repository: superset-sh/superset
Length of output: 1411
Avoid global console.warn patching; consider simpler suppression alternatives if available.
The current approach temporarily replaces the global console.warn which is process-wide and could conflict with other code performing similar patching. Consider whether simple-git offers a per-instance configuration option (e.g., via the constructor options or a dedicated logger/silent method) to avoid this mutation. If simple-git provides no scoped alternative, this pattern is acceptable but should remain minimal.
Note: The suggestion to apply bundled environment via git.env(getBundledGitEnv()) requires creating a new helper function and lacks evidence that git operations currently fail without it. Before implementing, verify whether the bundled git binary already works correctly for all operations as-is.
🤖 Prompt for AI Agents
apps/desktop/src/main/lib/git-binary.ts lines 55-75: avoid mutating the global
console.warn; instead create the simple-git instance with a per-instance
suppression or no-op logger (e.g., pass a logger option or call
instance.silent(true) or similar per-instance API) so the specific
SIMPLE_GIT_BINARY_WARNING is not emitted globally; if simple-git exposes a
logger option, pass a logger object with a noop warn method, or after creating
the instance call the instance-silent API, and only fall back to the temporary
console.warn replacement if no per-instance suppression exists (in that
fallback, keep the try/finally and add a guard so you only patch if console.warn
is exactly the original function to avoid clobbering other patches).
AviPeltz
force-pushed
the
extended-pheasant-7faf6d
branch
from
8185d2a to
9bdcfcd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/desktop/src/main/lib/git-binary.ts (2)
71-98: Reminder:getGitExecPath()needs to be integrated into git operations.This function is exported but the past review identified that
GIT_EXEC_PATHis not being set in the environment for git operations. Without this environment variable, git-lfs and helper binaries may not be discoverable, potentially breaking LFS repository operations (a key PR objective).The past review recommendation to create a
getBundledGitEnv()helper remains valid. This would centralize environment setup for all git operations.Based on the past review, consider implementing:
export function getBundledGitEnv(): NodeJS.ProcessEnv { return { ...process.env, GIT_EXEC_PATH: getGitExecPath(), }; }Then ensure all
simpleGitinstances andexecFileAsynccalls use this environment.
107-136: Acknowledge globalconsole.warnpatching limitation.The past review raised concerns about patching the global
console.warn, which is process-wide. While this is not ideal, the warning originates from simple-git's binary path validation during instantiation, before any instance-level configuration can be applied.The current try/finally pattern ensures the patch is minimal and restored. If simple-git offers no pre-instantiation option to suppress this specific validation warning, this approach is acceptable.
Minor improvement: you could add a guard to verify
console.warnhasn't been replaced by other code:// Temporarily suppress the specific warning from simple-git const originalWarn = console.warn; + if (originalWarn !== console.warn) { + // Already patched by other code, skip to avoid conflicts + console.log("[git-binary] console.warn already patched, skipping suppression"); + } console.warn = (...args: unknown[]) => { if (args[0] === SIMPLE_GIT_BINARY_WARNING) return; originalWarn.apply(console, args); };
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/git-binary.ts (1)
30-65: Consider validating binary existence and throwing early.The function logs whether the binary exists but doesn't throw if it's missing. This defers errors to later git operations, potentially causing confusing error messages. Consider adding validation:
} else { // In development, use dugite's resolver const { resolveGitBinary } = dynamicRequire( "dugite", ) as typeof import("dugite"); cachedGitPath = resolveGitBinary(); console.log("[git-binary] Development mode"); console.log("[git-binary] Git binary path:", cachedGitPath); console.log("[git-binary] Git binary exists:", existsSync(cachedGitPath)); } + + if (!existsSync(cachedGitPath)) { + throw new Error( + `Git binary not found at expected path: ${cachedGitPath}. ` + + `This indicates a packaging or installation issue.` + ); + } } return cachedGitPath; }This provides clearer feedback if there's a packaging issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/electron.vite.config.ts(1 hunks)apps/desktop/src/main/lib/git-binary.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/electron.vite.config.ts
🧰 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/git-binary.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/git-binary.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/main/lib/git-binary.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/main/lib/git-binary.ts
apps/desktop/src/main/lib/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement IPC handlers accepting object parameters (not positional parameters) in apps/desktop/src/main/lib/*.ts files
Files:
apps/desktop/src/main/lib/git-binary.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). (6)
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Deploy API
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Build
Use dugite to bundle a portable git binary with the app, eliminating the need for system git. This fixes the "Xcode license agreements" error that macOS users encounter when they haven't accepted the Xcode license. Changes: - Add dugite package for bundled git binaries - Create git-binary.ts helper to resolve bundled git path - Update all simple-git usages to use bundled binary - Update electron-builder to include dugite in app bundle - Remove shell-env.ts (no longer needed - was for finding system git) - Bundled git includes git-lfs, so LFS repos work out of the box 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Use dugite's bundled git binary to avoid Xcode license requirement on macOS - Add createBundledGit() helper that suppresses spurious simple-git warnings (dugite's path contains '@' which fails simple-git's overly strict regex) - Centralize all git binary logic in git-binary.ts - Remove shell-env.ts since we no longer need to detect system git 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The workspace tests previously mocked simple-git directly, but now that we use createBundledGit from git-binary.ts, we need to mock at the function level. Updated tests to use configurable mock state for git utility functions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove unused mock return values and unnecessary comments from workspace tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add dugite to copy-native-modules script to replace symlinks with real files before electron-builder runs - Update asarUnpack pattern to unpack entire dugite module to avoid "pattern is too long" error from minimatch 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
The dugite package contains internal symlinks (e.g., git-apply -> git) inside git/libexec/git-core/. When copying the module, these symlinks would still point to the original Bun cache location, causing electron-builder to fail with "symlinked outside the package" errors. This fix adds `dereference: true` to cpSync calls to resolve all internal symlinks into actual file copies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
The diff viewer was failing in packaged builds because simple-git was loaded via dynamicRequire, preventing Rollup from bundling it. Changed to a static import so simple-git is included in the bundle. Also manually construct paths to app.asar.unpacked for dugite's git binary in packaged apps, since dugite's __dirname points inside the asar archive where binaries can't execute. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
AviPeltz
force-pushed
the
extended-pheasant-7faf6d
branch
from
9bdcfcd to
f347715
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (1)
103-107: Improve mock to return a minimal SimpleGit shape.The current mock returns an empty object which will throw if any code path calls methods like
raw(),status(), etc. While the tests use./utils/gitmocks, this could cause issues if the import chain changes.mock.module("main/lib/git-binary", () => ({ - createBundledGit: mock(() => ({})), + createBundledGit: mock(() => ({ + raw: mock(() => Promise.resolve("")), + status: mock(() => Promise.resolve({ files: [] })), + branch: mock(() => Promise.resolve({ current: "main", branches: {} })), + })), getGitBinaryPath: mock(() => "/mock/git"), }));apps/desktop/src/main/lib/git-binary.ts (2)
71-98:getGitExecPath()is defined but never used; git-lfs operations may fail.This function is exported but not called anywhere. For git-lfs and helper binaries to work,
GIT_EXEC_PATHshould be set when invoking git. Consider either:
- Using it in
createBundledGitvia simple-git's.env()method:const git = simpleGit({ ...(baseDir && { baseDir }), binary: gitPath, unsafe: { allowUnsafeCustomBinary: true }, }).env('GIT_EXEC_PATH', getGitExecPath());
- Or exporting a helper for direct
execFileAsynccalls:export function getBundledGitEnv(): NodeJS.ProcessEnv { return { ...process.env, GIT_EXEC_PATH: getGitExecPath() }; }Verify git-lfs works in the packaged app by cloning a repo with LFS files (mentioned in PR test plan as pending).
114-131: Globalconsole.warnpatching is acceptable but document the reason.The try/finally ensures proper restoration, but this pattern patches a global. The comment explains the "why" but consider adding a note about thread-safety implications in Node.js (single-threaded so safe, but could interleave with async code).
The approach is reasonable given simple-git doesn't expose per-instance warning suppression.
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (3)
110-123: PassGIT_EXEC_PATHenvironment variable to git spawning.The
execFileAsynccall doesn't include theGIT_EXEC_PATHenvironment variable, which means git-lfs and other bundled helper binaries won't be discoverable. This will cause operations to fail on LFS-enabled repositories.Apply this diff to pass the required environment:
+ const env = { ...process.env, GIT_EXEC_PATH: getGitExecPath() }; + await execFileAsync( getGitBinaryPath(), [ "-C", mainRepoPath, "worktree", "add", worktreePath, "-b", branch, startPoint, ], - { timeout: 120_000 }, + { timeout: 120_000, env }, );
6-6: ImportgetGitExecPathto passGIT_EXEC_PATHto git operations.The import is missing
getGitExecPath, which is needed to set theGIT_EXEC_PATHenvironment variable for git operations. Without this, git-lfs and other bundled helper binaries won't be discoverable, causing LFS operations to fail.Apply this diff to add the missing import:
-import { createBundledGit, getGitBinaryPath } from "main/lib/git-binary"; +import { createBundledGit, getGitBinaryPath, getGitExecPath } from "main/lib/git-binary";
168-172: PassGIT_EXEC_PATHenvironment variable to git spawning.Similar to
createWorktree, thisexecFileAsynccall is missing theGIT_EXEC_PATHenvironment variable.Apply this diff:
+ const env = { ...process.env, GIT_EXEC_PATH: getGitExecPath() }; + await execFileAsync( getGitBinaryPath(), ["-C", mainRepoPath, "worktree", "remove", worktreePath, "--force"], - { timeout: 60_000 }, + { timeout: 60_000, env }, );
🧹 Nitpick comments (4)
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts (1)
40-44: Bundled git binary usage looks correct, but consider addingGIT_EXEC_PATHfor consistency.The switch to
getGitBinaryPath()correctly uses the bundled binary. However, for full compatibility with git-lfs and helper binaries, consider passingGIT_EXEC_PATHin the environment options:const { stdout: branchOutput } = await execFileAsync( getGitBinaryPath(), ["rev-parse", "--abbrev-ref", "HEAD"], - { cwd: worktreePath }, + { cwd: worktreePath, env: { ...process.env, GIT_EXEC_PATH: getGitExecPath() } }, );This requires importing
getGitExecPathfrommain/lib/git-binary. Whilerev-parselikely works without it, setting the environment consistently across all bundled git invocations prevents subtle failures with other git operations.apps/desktop/src/main/lib/git-binary.ts (2)
14-14: Consider removing or reducing debug logging for production.Multiple
console.logstatements throughout this module (lines 14, 48-51, 59-61, 86, 94, 108, 112, 127) will add noise to production logs. Consider:
- Using a conditional debug flag:
if (process.env.DEBUG_GIT_BINARY)- Or removing logs after the bundling issue is resolved
30-65: Path resolution logic is sound; minor improvement for robustness.The logic correctly handles packaged vs development modes. Consider adding a fail-fast check when the binary doesn't exist in packaged mode:
console.log("[git-binary] Git binary exists:", existsSync(cachedGitPath)); +if (!existsSync(cachedGitPath)) { + throw new Error(`Bundled git binary not found at: ${cachedGitPath}`); +}This provides a clear error message instead of a cryptic failure later during git operations.
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
77-83: Consider a more concise implementation.The helper can be simplified slightly without losing clarity.
Apply this diff for a more concise version:
function isEnoent(error: unknown): boolean { - return ( - error instanceof Error && - "code" in error && - (error as NodeJS.ErrnoException).code === "ENOENT" - ); + return (error as NodeJS.ErrnoException)?.code === "ENOENT"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
apps/desktop/electron-builder.ts(2 hunks)apps/desktop/electron.vite.config.ts(1 hunks)apps/desktop/package.json(1 hunks)apps/desktop/scripts/copy-native-modules.ts(2 hunks)apps/desktop/src/lib/trpc/routers/changes/changes.ts(9 hunks)apps/desktop/src/lib/trpc/routers/projects/projects.ts(3 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts(0 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts(10 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts(2 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts(0 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts(4 hunks)apps/desktop/src/main/lib/git-binary.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/electron.vite.config.ts
- apps/desktop/scripts/copy-native-modules.ts
- apps/desktop/electron-builder.ts
- apps/desktop/package.json
🧰 Additional context used
📓 Path-based instructions (6)
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/workspaces/workspaces.test.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/src/main/lib/git-binary.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/workspaces/workspaces.test.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/src/main/lib/git-binary.ts
apps/desktop/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.test.{ts,tsx,js,jsx}: Tests should have one assert per test
Tests should be readable
Tests should be fast
Tests should be independent
Tests should be repeatable
Files:
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/src/main/lib/git-binary.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/src/main/lib/git-binary.ts
apps/desktop/src/main/lib/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement IPC handlers accepting object parameters (not positional parameters) in apps/desktop/src/main/lib/*.ts files
Files:
apps/desktop/src/main/lib/git-binary.ts
🧠 Learnings (2)
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be independent
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be repeatable
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts
🧬 Code graph analysis (3)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
apps/desktop/src/main/lib/git-binary.ts (1)
createBundledGit(107-136)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
apps/desktop/src/main/lib/git-binary.ts (2)
createBundledGit(107-136)getGitBinaryPath(30-65)
apps/desktop/src/lib/trpc/routers/changes/changes.ts (1)
apps/desktop/src/main/lib/git-binary.ts (1)
createBundledGit(107-136)
⏰ 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). (6)
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy API
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (9)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (3)
8-8: Import change looks good.The centralized
createBundledGitimport aligns with the PR's goal of using bundled git consistently.
194-194: Correct usage for git init operation.Passing
input.pathasbaseDircorrectly scopes the git instance to the target directory for initialization.
334-335: Correct usage for clone operation.Calling
createBundledGit()withoutbaseDiris appropriate for clone since the target directory doesn't exist yet. The subsequentgit.clone(input.url, clonePath)handles path specification.apps/desktop/src/lib/trpc/routers/changes/changes.ts (2)
3-3: Import change is consistent with codebase migration.
31-31: Consistent migration to bundled git across all router methods.All
simpleGit(input.worktreePath)calls have been uniformly replaced withcreateBundledGit(input.worktreePath). The pattern is applied consistently to:getBranches,getStatus,getCommitFiles,getFileContents,stageFile,unstageFile,discardChanges,stageAll, andunstageAll.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (2)
62-101: Well-structured configurable mock state for test isolation.The
gitMockStatepattern withbeforeEachreset provides good test isolation and configurability. The exact path matching logic inworktreeExistsmock (lines 88-100) correctly tests the parsing behavior.
137-143: Proper test isolation with state reset.Resetting all
gitMockStatefields inbeforeEachensures tests are independent and repeatable. Based on learnings, this follows best practices for test independence.apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (2)
19-75: LGTM! Robust LFS detection with appropriate fallbacks.The three-stage detection strategy (checking .git/lfs directory, attribute files, and sampling with git check-attr) provides reliable LFS detection while handling missing files and command failures gracefully.
133-157: LGTM! Comprehensive error handling with informative messages.The lock file and LFS error detection logic is thorough, and the error messages provide clear guidance to users on how to resolve issues.
| try { | ||
| const git = simpleGit(repoPath); | ||
| // Get a small sample of tracked files (limit to 20 for performance) | ||
| const git = createBundledGit(repoPath); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and read the git-binary.ts file
find . -name "git-binary.ts" -type f | head -5Repository: superset-sh/superset
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Read the git-binary.ts file to examine createBundledGit implementation
cat -n ./apps/desktop/src/main/lib/git-binary.tsRepository: superset-sh/superset
Length of output: 5595
Pass GIT_EXEC_PATH environment variable to all createBundledGit instances.
The createBundledGit function computes the correct git exec path via getGitExecPath() but never passes it to simpleGit. This breaks git-lfs operations and any git commands relying on helper binaries. Use .env("GIT_EXEC_PATH", getGitExecPath()) when creating the simpleGit instance at line 122-126 in apps/desktop/src/main/lib/git-binary.ts.
Also applies to: 183-183, 196-196, 209-209, 218-218, 250-250, 260-260, 272-272, 280-280, 308-308
Dugite's bundled git doesn't include git-lfs, so we need to find it from the user's shell PATH. Restored shell-env.ts and getGitEnv() to merge PATH from shell profiles, and added proactive LFS availability check before worktree creation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
i think this should bundle but ill check out the above coderabbit error |
Summary
Problem
Users on macOS were seeing this error when trying to use the app:
This happened because the app relied on the system git, which on macOS comes from Xcode Command Line Tools and requires accepting the license.
Solution
Bundle git with the app using dugite, the same solution GitHub Desktop uses. The app now ships with its own git binary (~40-50MB added to app size) and works out of the box without any system dependencies.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.