fix: Run GitHub sync in worker thread to prevent UI blocking#2602
fix: Run GitHub sync in worker thread to prevent UI blocking#2602wwwillchen-bot wants to merge 1 commit intodyad-sh:mainfrom
Conversation
When syncing to GitHub, the chat input was blocked - commands issued during the sync wouldn't appear until the sync finished. This was caused by git operations blocking the main Electron process event loop. Solution: Move git push/pull operations to a dedicated worker thread that runs independently from the main process. This keeps the main event loop free to handle IPC messages (like chat input) during sync. Key changes: - Add git_worker.ts that runs git operations in a separate thread - Add git_worker_utils.ts wrapper for spawning the worker - Update handlePushToGithub to use worker-based git operations - Add vite config for building the git worker - Add git_worker_types.ts for type-safe worker communication The worker uses child_process.execFile directly (instead of dugite) to avoid module resolution issues in packaged Electron apps. Fixes dyad-sh#2592 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
|
@BugBot run |
Summary of ChangesHello @wwwillchen-bot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical UI responsiveness issue where the application's chat input would become blocked during GitHub synchronization. The problem stemmed from git operations executing on the main Electron process's event loop, which led to UI freezes. The solution involves offloading these potentially long-running git tasks to a dedicated Node.js worker thread, thereby isolating them from the main UI thread and ensuring a consistently smooth and responsive user experience during repository interactions. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile OverviewGreptile SummaryThis PR moves GitHub sync (push/pull/remote URL updates) out of the Electron main process event loop into a dedicated Node worker thread, aiming to keep IPC (e.g., chat input) responsive during long-running git operations. It introduces a new git worker entrypoint ( The overall direction fits the existing pattern used for the TypeScript worker ( Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer
participant M as Main (github_handlers)
participant W as Git Worker Thread
participant G as git (execFile)
R->>M: IPC invoke github:push {appId, force?}
M->>M: readSettings() / db lookup app
M->>W: runGitOperation(setRemoteUrl)
W->>G: git remote set-url/add origin <remoteUrl>
G-->>W: exit code + stderr/stdout
W-->>M: {success|error}
alt not force push
M->>W: runGitOperation(pull)
W->>G: git pull origin <branch>
G-->>W: exit code + stderr/stdout
W-->>M: {success|error,name?}
M->>M: map conflict errors to GitConflictError
end
M->>W: runGitOperation(push)
W->>G: git push origin <branch>:<branch> [--force*]
G-->>W: exit code + stderr/stdout
W-->>M: {success|error}
M-->>R: IPC resolve/reject
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const workerPath = path.join(__dirname, "git_worker.js"); | ||
|
|
||
| logger.info(`Starting git worker for operation: ${input.type}`); |
There was a problem hiding this comment.
Worker path likely wrong
runGitOperation constructs the worker script path as path.join(__dirname, "git_worker.js"), but the compiled/bundled entry for this worker comes from workers/git/git_worker.ts (built via vite.git-worker.config.mts). Unless the build output is also copied into src/ipc/utils/ at runtime (which doesn’t appear to be the case), this will fail at runtime with “cannot find module …/git_worker.js”. The existing TSC worker (src/ipc/processors/tsc.ts:26) works because tsc_worker.js is co-located with that module; the git worker isn’t.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ipc/utils/git_worker_utils.ts
Line: 70:72
Comment:
**Worker path likely wrong**
`runGitOperation` constructs the worker script path as `path.join(__dirname, "git_worker.js")`, but the compiled/bundled entry for this worker comes from `workers/git/git_worker.ts` (built via `vite.git-worker.config.mts`). Unless the build output is also copied into `src/ipc/utils/` at runtime (which doesn’t appear to be the case), this will fail at runtime with “cannot find module …/git_worker.js”. The existing TSC worker (`src/ipc/processors/tsc.ts:26`) works because `tsc_worker.js` is co-located with that module; the git worker isn’t.
How can I resolve this? If you propose a fix, please make it concise.| const result = await execGit(args, appPath, sanitizedEnv); | ||
| if (result.exitCode !== 0) { | ||
| const errorMsg = result.stderr || result.stdout; | ||
| return { success: false, error: `Git push failed: ${errorMsg}` }; |
There was a problem hiding this comment.
Pull conflict detection broken
execGit always returns { exitCode: 0 } in the success path, but in the error path it sets exitCode: error.code || 1. For execFile, error.code is often a string like "ENOENT", so exitCode can become a string. In handlePull/handlePush you compare result.exitCode !== 0, which will be true for every successful command that returns exitCode: 0? (fine) but also for string codes in failures; more importantly, the type mismatch means conflict detection/error handling becomes inconsistent and can misclassify failures. This needs to ensure exitCode is always numeric (and keep the original error code separately if needed).
Prompt To Fix With AI
This is a comment left during a code review.
Path: workers/git/git_worker.ts
Line: 165:168
Comment:
**Pull conflict detection broken**
`execGit` always returns `{ exitCode: 0 }` in the success path, but in the error path it sets `exitCode: error.code || 1`. For `execFile`, `error.code` is often a string like `"ENOENT"`, so `exitCode` can become a string. In `handlePull`/`handlePush` you compare `result.exitCode !== 0`, which will be `true` for *every successful command that returns `exitCode: 0`?* (fine) but also for string codes in failures; more importantly, the type mismatch means conflict detection/error handling becomes inconsistent and can misclassify failures. This needs to ensure `exitCode` is always numeric (and keep the original error code separately if needed).
How can I resolve this? If you propose a fix, please make it concise.| await gitSetRemoteUrlWorker({ | ||
| appPath, | ||
| remoteUrl, | ||
| }); |
There was a problem hiding this comment.
Missing repo lock around git ops
handlePushToGithub used to run git operations in-process; after moving to the worker, the gitPullWorker/gitPushWorker calls are no longer wrapped in withLock(appId, ...) (unlike other operations such as prepareLocalBranch). This allows concurrent sync/push operations for the same appId to overlap and mutate the same repo simultaneously, which will produce nondeterministic failures (e.g., index.lock errors, partial pulls, or push after a conflicting pull). Wrap the pull/push (and remote URL update) in the same per-app lock.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ipc/handlers/github_handlers.ts
Line: 863:866
Comment:
**Missing repo lock around git ops**
`handlePushToGithub` used to run git operations in-process; after moving to the worker, the `gitPullWorker`/`gitPushWorker` calls are no longer wrapped in `withLock(appId, ...)` (unlike other operations such as `prepareLocalBranch`). This allows concurrent sync/push operations for the same `appId` to overlap and mutate the same repo simultaneously, which will produce nondeterministic failures (e.g., index.lock errors, partial pulls, or push after a conflicting pull). Wrap the pull/push (and remote URL update) in the same per-app lock.
How can I resolve this? If you propose a fix, please make it concise.![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const args = ["pull", remote, branch]; | ||
| const result = await execGit(args, appPath, sanitizedEnv); |
There was a problem hiding this comment.
🔴 Worker git pull missing --rebase=false, author config, and credential.helper= flags present in the original implementation
The worker's handlePull constructs the git pull command as ["pull", remote, branch], but the original gitPull in src/ipc/utils/git_utils.ts:1174-1181 uses a much more complete command:
["-c", "user.name=...", "-c", "user.email=...", "-c", "credential.helper=", "pull", "--rebase=false", remote, branch]
Missing flags and their impact
-
--rebase=false: Without this flag, if the user haspull.rebase=truein their global or local git config, the pull will rebase instead of merge. The original code explicitly forces merge behavior. This changes the git history shape and can lead to unexpected conflicts or behavior. -
-c user.name=... -c user.email=...(viawithGitAuthor): The original code sets the committer/author identity for merge commits created during pull. Without this, merge commits will use whatever is in the user's global git config, or fail/use empty identity if none is configured. Since this runs in a worker thread with a copiedprocess.env, the git author must be explicitly set. -
-c credential.helper=: The original code disables credential helpers to prevent git from launching interactive prompts. In a worker thread, an interactive credential prompt would silently hang the operation indefinitely since there's no terminal attached.
Impact: Any git pull that creates a merge commit will have incorrect author info. Users with pull.rebase=true will get rebases instead of merges. Credential helper invocations could hang the worker.
Prompt for agents
In workers/git/git_worker.ts, the handlePull function at line 192 needs to replicate the flags used by the original gitPull in src/ipc/utils/git_utils.ts lines 1174-1181. Specifically:
1. Add --rebase=false to force merge strategy.
2. Add -c credential.helper= to disable credential helpers (which would hang the worker).
3. Add -c user.name=<name> -c user.email=<email> to set the author identity for any merge commits.
Since the worker can't call withGitAuthor (it relies on getGitAuthor which reads settings), you'll need to either:
- Pass the git author name/email as part of the GitPullWorkerInput type (add fields to shared/git_worker_types.ts), populate them in gitPullWorker in src/ipc/utils/git_worker_utils.ts from getGitAuthor(), and use them in the worker.
- Or find another way to make the author info available in the worker.
The final args should look like: ["-c", "user.name=<name>", "-c", "user.email=<email>", "-c", "credential.helper=", "pull", "--rebase=false", remote, branch].
Was this helpful? React with 👍 or 👎 to provide feedback.
| worker.on("exit", (code) => { | ||
| if (code !== 0) { | ||
| logger.error(`Git worker exited with code ${code}`); | ||
| reject(new Error(`Worker exited with code ${code}`)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🟡 worker.terminate() in message handler causes spurious error log and reject on every successful operation
In runGitOperation, when the message event fires, worker.terminate() is called at line 83 before resolving the promise. worker.terminate() causes the worker to exit with code 1, which triggers the exit event handler at line 98-103. Since code is 1 (not 0), the exit handler logs Git worker exited with code 1 as an error and calls reject().
Detailed explanation of the race
The sequence on every successful git operation is:
- Worker sends message →
messagehandler fires worker.terminate()is called at line 83resolve(output)is called at line 89 — promise is settled- Worker exits with code 1 (due to
terminate()) →exithandler fires logger.error("Git worker exited with code 1")logs a spurious error at line 100reject(new Error(...))at line 101 is a no-op since promise is already resolved
Impact: Every successful git worker operation produces a misleading error log entry (Git worker exited with code 1), which pollutes the logs and makes real errors harder to find.
| worker.on("exit", (code) => { | |
| if (code !== 0) { | |
| logger.error(`Git worker exited with code ${code}`); | |
| reject(new Error(`Worker exited with code ${code}`)); | |
| } | |
| }); | |
| let settled = false; | |
| worker.on("message", (output: GitWorkerOutput) => { | |
| settled = true; | |
| worker.terminate(); | |
| if (output.success) { | |
| logger.info(`Git worker completed successfully: ${input.type}`); | |
| } else { | |
| logger.error(`Git worker failed: ${input.type} - ${output.error}`); | |
| } | |
| resolve(output); | |
| }); | |
| worker.on("error", (error) => { | |
| logger.error(`Git worker error: ${input.type}`, error); | |
| settled = true; | |
| worker.terminate(); | |
| reject(error); | |
| }); | |
| worker.on("exit", (code) => { | |
| if (code !== 0 && !settled) { | |
| logger.error(`Git worker exited with code ${code}`); | |
| reject(new Error(`Worker exited with code ${code}`)); | |
| } | |
| }); | |
Was this helpful? React with 👍 or 👎 to provide feedback.
🔍 Dyadbot Code Review SummaryFound 6 issue(s) flagged by consensus of 3 independent reviewers. Summary
Issues to Address
See inline comments for details. Generated by Dyadbot multi-agent code review (3 agents, consensus voting) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }; | ||
| } | ||
|
|
||
| const args = ["pull", remote, branch]; |
There was a problem hiding this comment.
🔴 HIGH: Worker pull missing --rebase=false, -c credential.helper=, and git author config
The original gitPull in git_utils.ts (line 1174) uses withGitAuthor and passes -c credential.helper= and --rebase=false. This worker's handlePull uses a bare ["pull", remote, branch] with none of these flags, causing three behavioral regressions:
-
Missing
--rebase=false: If the user's global git config haspull.rebase=true, the pull will rebase instead of merge. This could produce unexpected history rewrites and break the conflict detection logic (which looks for merge-conflict keywords). -
Missing
-c credential.helper=: Without explicitly disabling credential helpers, git may invoke an interactive credential prompt which will hang the worker thread indefinitely since there's no TTY. -
Missing git author config (
-c user.name=... -c user.email=...): If a pull results in a merge commit, git needs author/committer identity. Without it, the operation will fail with "Author identity unknown" if the user hasn't configured git globally.
| const args = ["pull", remote, branch]; | |
| const args = ["-c", "credential.helper=", "pull", "--rebase=false", remote, branch]; |
Note: The git author config (-c user.name=... -c user.email=...) also needs to be added. This requires either passing author info through the worker input message or reading it from git config inside the worker, similar to what withGitAuthor does in git_utils.ts.
(Flagged by 3/3 reviewers)
| worker.on("message", (output: GitWorkerOutput) => { | ||
| worker.terminate(); | ||
| if (output.success) { | ||
| logger.info(`Git worker completed successfully: ${input.type}`); | ||
| } else { | ||
| logger.error(`Git worker failed: ${input.type} - ${output.error}`); | ||
| } | ||
| resolve(output); | ||
| }); | ||
|
|
||
| worker.on("error", (error) => { | ||
| logger.error(`Git worker error: ${input.type}`, error); | ||
| worker.terminate(); | ||
| reject(error); | ||
| }); | ||
|
|
||
| worker.on("exit", (code) => { | ||
| if (code !== 0) { | ||
| logger.error(`Git worker exited with code ${code}`); | ||
| reject(new Error(`Worker exited with code ${code}`)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🔴 HIGH: Promise double-settlement race between message/exit handlers
When the message handler fires, it calls worker.terminate() then resolve(). The terminate() call triggers the exit event with a non-zero code (typically 1), which then calls reject(). This causes:
- Spurious error logs on every successful operation:
"Git worker exited with code 1"is logged even when the operation succeeded. - Potential unhandled rejection: If
errorfires and thenexitfires (both beforemessage),rejectis called twice. - No timeout: If the worker crashes before sending a message, the Promise will never resolve.
Fix: Add a settled flag to guard all resolve/reject calls:
| worker.on("message", (output: GitWorkerOutput) => { | |
| worker.terminate(); | |
| if (output.success) { | |
| logger.info(`Git worker completed successfully: ${input.type}`); | |
| } else { | |
| logger.error(`Git worker failed: ${input.type} - ${output.error}`); | |
| } | |
| resolve(output); | |
| }); | |
| worker.on("error", (error) => { | |
| logger.error(`Git worker error: ${input.type}`, error); | |
| worker.terminate(); | |
| reject(error); | |
| }); | |
| worker.on("exit", (code) => { | |
| if (code !== 0) { | |
| logger.error(`Git worker exited with code ${code}`); | |
| reject(new Error(`Worker exited with code ${code}`)); | |
| } | |
| }); | |
| let settled = false; | |
| worker.on("message", (output: GitWorkerOutput) => { | |
| if (settled) return; | |
| settled = true; | |
| worker.terminate(); | |
| if (output.success) { | |
| logger.info(`Git worker completed successfully: ${input.type}`); | |
| } else { | |
| logger.error(`Git worker failed: ${input.type} - ${output.error}`); | |
| } | |
| resolve(output); | |
| }); | |
| worker.on("error", (error) => { | |
| if (settled) return; | |
| settled = true; | |
| logger.error(`Git worker error: ${input.type}`, error); | |
| worker.terminate(); | |
| reject(error); | |
| }); | |
| worker.on("exit", (code) => { | |
| if (settled) return; | |
| if (code !== 0) { | |
| settled = true; | |
| logger.error(`Git worker exited with code ${code}`); | |
| reject(new Error(`Worker exited with code ${code}`)); | |
| } | |
| }); |
(Flagged by 3/3 reviewers)
| return { | ||
| stdout: error.stdout || "", | ||
| stderr: error.stderr || error.message || "", | ||
| exitCode: error.code || 1, |
There was a problem hiding this comment.
🟡 MEDIUM: error.code may be a string, not a numeric exit code
When Node's child_process.execFile throws, error.code can be a string like 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER' or 'ENOENT' (when git binary is not found), not a numeric exit code. The actual numeric exit code is available on error.status.
Using error.code means exitCode could be set to a string, which would still pass the !== 0 check (strings are truthy), but the ExecResult.exitCode type declares number. Downstream callers in github_handlers.ts also check pullError?.code === "MissingRefError" / "NotFoundError" which are isomorphic-git codes the worker will never produce.
| exitCode: error.code || 1, | |
| exitCode: typeof error.code === 'number' ? error.code : (error.status ?? 1), |
(Flagged by 3/3 reviewers)
| type: "push"; | ||
| appPath: string; | ||
| branch: string; | ||
| accessToken?: string; |
There was a problem hiding this comment.
🟡 MEDIUM: accessToken field accepted in types but silently ignored by the worker
Both GitPushWorkerInput.accessToken and GitPullWorkerInput.accessToken are defined here and the main-process wrappers pass them through. However, the worker (git_worker.ts) never reads or uses accessToken in any handler -- the destructuring simply ignores it.
Authentication works because the token is already embedded in the remote URL (set via gitSetRemoteUrlWorker). However, having an unused accessToken field is misleading -- it suggests the token is used for auth in the worker, which it isn't. If someone removes the token from the URL thinking "it's passed separately," pull/push would fail silently.
Consider either removing accessToken from the worker types entirely, or adding a comment explaining it's present for API compatibility but unused since auth is via the remote URL.
(Flagged by 3/3 reviewers)
| const { stdout, stderr } = await execFileAsync(GIT_PATH, args, { | ||
| cwd, | ||
| env, | ||
| maxBuffer: 100 * 1024 * 1024, // 100MB |
There was a problem hiding this comment.
🟡 MEDIUM: maxBuffer of 100MB is excessive for push/pull operations
For git push/pull/remote operations, stdout/stderr should be small (a few KB at most). A 100MB buffer means if git produces unexpectedly large output (e.g., a corrupted repository), the worker will buffer up to 100MB in memory. Since a new worker is spawned per operation, concurrent operations could consume significant memory.
A more reasonable buffer like 10MB would be sufficient:
| maxBuffer: 100 * 1024 * 1024, // 100MB | |
| maxBuffer: 10 * 1024 * 1024, // 10MB |
(Flagged by 2/3 reviewers)
|
🟡 MEDIUM: Inconsistent use of worker vs direct call for
While (Flagged by 2/3 reviewers) |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 9 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="workers/git/git_worker.ts">
<violation number="1" location="workers/git/git_worker.ts:128">
P2: `error.code` can be a string (e.g., `'ENOENT'`) when the process fails to spawn, causing a runtime type mismatch with the `exitCode: number` field. Use a type guard to ensure a numeric exit code.</violation>
<violation number="2" location="workers/git/git_worker.ts:192">
P1: The git pull command is missing critical flags from the original implementation: `--rebase=false` (to force merge behavior regardless of user's git config), and `-c credential.helper=` (to prevent credential helper prompts from hanging the worker). Without `--rebase=false`, users with `pull.rebase=true` in their git config will get rebases instead of merges, changing the expected git history.</violation>
</file>
<file name="src/ipc/handlers/github_handlers.ts">
<violation number="1" location="src/ipc/handlers/github_handlers.ts:862">
P1: The git worker operations (pull, push, setRemoteUrl) are not wrapped in a per-app lock. If multiple sync operations run concurrently for the same app, they could produce index.lock errors, partial pulls, or race conditions. The original implementation likely used `withLock(appId, ...)` to serialize these operations. Consider wrapping these calls in a lock to prevent concurrent repo mutations.</violation>
</file>
<file name="src/ipc/utils/git_worker_utils.ts">
<violation number="1" location="src/ipc/utils/git_worker_utils.ts:98">
P2: Bug: `worker.terminate()` causes exit with code 1, so the `exit` handler will log a spurious error and attempt to reject an already-settled promise on every successful operation. Add a `settled` flag to guard against this race condition between the `message`/`error` handlers and the `exit` handler.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }; | ||
| } | ||
|
|
||
| const args = ["pull", remote, branch]; |
There was a problem hiding this comment.
P1: The git pull command is missing critical flags from the original implementation: --rebase=false (to force merge behavior regardless of user's git config), and -c credential.helper= (to prevent credential helper prompts from hanging the worker). Without --rebase=false, users with pull.rebase=true in their git config will get rebases instead of merges, changing the expected git history.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At workers/git/git_worker.ts, line 192:
<comment>The git pull command is missing critical flags from the original implementation: `--rebase=false` (to force merge behavior regardless of user's git config), and `-c credential.helper=` (to prevent credential helper prompts from hanging the worker). Without `--rebase=false`, users with `pull.rebase=true` in their git config will get rebases instead of merges, changing the expected git history.</comment>
<file context>
@@ -0,0 +1,288 @@
+ };
+ }
+
+ const args = ["pull", remote, branch];
+ const result = await execGit(args, appPath, sanitizedEnv);
+ if (result.exitCode !== 0) {
</file context>
| await gitSetRemoteUrl({ | ||
| path: appPath, | ||
|
|
||
| // Set or update remote URL using git config (in worker thread) |
There was a problem hiding this comment.
P1: The git worker operations (pull, push, setRemoteUrl) are not wrapped in a per-app lock. If multiple sync operations run concurrently for the same app, they could produce index.lock errors, partial pulls, or race conditions. The original implementation likely used withLock(appId, ...) to serialize these operations. Consider wrapping these calls in a lock to prevent concurrent repo mutations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/handlers/github_handlers.ts, line 862:
<comment>The git worker operations (pull, push, setRemoteUrl) are not wrapped in a per-app lock. If multiple sync operations run concurrently for the same app, they could produce index.lock errors, partial pulls, or race conditions. The original implementation likely used `withLock(appId, ...)` to serialize these operations. Consider wrapping these calls in a lock to prevent concurrent repo mutations.</comment>
<file context>
@@ -852,17 +858,18 @@ async function handlePushToGithub(
- await gitSetRemoteUrl({
- path: appPath,
+
+ // Set or update remote URL using git config (in worker thread)
+ await gitSetRemoteUrlWorker({
+ appPath,
</file context>
| reject(error); | ||
| }); | ||
|
|
||
| worker.on("exit", (code) => { |
There was a problem hiding this comment.
P2: Bug: worker.terminate() causes exit with code 1, so the exit handler will log a spurious error and attempt to reject an already-settled promise on every successful operation. Add a settled flag to guard against this race condition between the message/error handlers and the exit handler.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/utils/git_worker_utils.ts, line 98:
<comment>Bug: `worker.terminate()` causes exit with code 1, so the `exit` handler will log a spurious error and attempt to reject an already-settled promise on every successful operation. Add a `settled` flag to guard against this race condition between the `message`/`error` handlers and the `exit` handler.</comment>
<file context>
@@ -0,0 +1,209 @@
+ reject(error);
+ });
+
+ worker.on("exit", (code) => {
+ if (code !== 0) {
+ logger.error(`Git worker exited with code ${code}`);
</file context>
| return { | ||
| stdout: error.stdout || "", | ||
| stderr: error.stderr || error.message || "", | ||
| exitCode: error.code || 1, |
There was a problem hiding this comment.
P2: error.code can be a string (e.g., 'ENOENT') when the process fails to spawn, causing a runtime type mismatch with the exitCode: number field. Use a type guard to ensure a numeric exit code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At workers/git/git_worker.ts, line 128:
<comment>`error.code` can be a string (e.g., `'ENOENT'`) when the process fails to spawn, causing a runtime type mismatch with the `exitCode: number` field. Use a type guard to ensure a numeric exit code.</comment>
<file context>
@@ -0,0 +1,288 @@
+ return {
+ stdout: error.stdout || "",
+ stderr: error.stderr || error.message || "",
+ exitCode: error.code || 1,
+ };
+ }
</file context>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the UI blocking issue during GitHub sync operations by moving them to a dedicated worker thread. The implementation is solid, with suggestions provided to improve robustness and maintainability through reduced code duplication and refined error handling. From a security perspective, while no vulnerabilities were found in the processed files, several empty files prevented a complete SAST Reconnaissance scan.
| if (setResult.exitCode !== 0) { | ||
| // Remote might not exist, try adding it | ||
| const addResult = await execGit( | ||
| ["remote", "add", "origin", remoteUrl], | ||
| appPath, | ||
| sanitizedEnv, | ||
| ); | ||
|
|
||
| if (addResult.exitCode !== 0) { | ||
| return { | ||
| success: false, | ||
| error: `Failed to set remote URL: ${addResult.stderr || addResult.stdout}`, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation attempts to git remote add if git remote set-url fails for any reason. This could mask the original error. For example, if set-url fails due to an invalid URL, the code will then try to add the remote, which might also fail (e.g., if the remote already exists), leading to a confusing error message.
It would be more robust to specifically check if set-url failed because the remote does not exist before attempting to add it.
if (setResult.exitCode !== 0) {
// If `set-url` failed because the remote doesn't exist, try adding it.
if (setResult.stderr.includes("No such remote")) {
const addResult = await execGit(
["remote", "add", "origin", remoteUrl],
appPath,
sanitizedEnv,
);
if (addResult.exitCode !== 0) {
return {
success: false,
error: `Failed to add remote 'origin': ${addResult.stderr || addResult.stdout}`,
};
}
} else {
// `set-url` failed for a different reason.
return {
success: false,
error: `Failed to set remote URL: ${setResult.stderr || setResult.stdout}`,
};
}
}| if (!result.success) { | ||
| const error = new Error(result.error); | ||
| if (result.name) { | ||
| (error as any).name = result.name; | ||
| } | ||
| if (result.code) { | ||
| (error as any).code = result.code; | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
The logic for checking the worker result and re-throwing a structured error is duplicated in gitPushWorker, gitPullWorker, and gitSetRemoteUrlWorker. This can be extracted into a helper function to avoid repetition and make the code more maintainable.
For example, you could add a helper function:
function handleWorkerResult(result: GitWorkerOutput): void {
if (!result.success) {
const error = new Error(result.error);
if (result.name) {
(error as any).name = result.name;
}
if (result.code) {
(error as any).code = result.code;
}
throw error;
}
}Then you can simplify this function to just handleWorkerResult(result);.
| async function processGitOperation( | ||
| input: GitWorkerInput, | ||
| ): Promise<GitWorkerOutput> { | ||
| switch (input.type) { | ||
| case "push": | ||
| return handlePush(input); | ||
| case "pull": | ||
| return handlePull(input); | ||
| case "setRemoteUrl": | ||
| return handleSetRemoteUrl(input); | ||
| default: | ||
| return { success: false, error: `Unknown operation type` }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The check for enableNativeGit is repeated in handlePush, handlePull, and handleSetRemoteUrl. To reduce code duplication and improve maintainability, this check can be moved to the processGitOperation function, as it's a common precondition for all operations handled by this worker.
async function processGitOperation(
input: GitWorkerInput,
): Promise<GitWorkerOutput> {
if (!input.enableNativeGit) {
return {
success: false,
error:
'Git worker requires native git to be enabled. Please enable native git in settings.',
};
}
switch (input.type) {
case "push":
return handlePush(input);
case "pull":
return handlePull(input);
case "setRemoteUrl":
return handleSetRemoteUrl(input);
default:
return { success: false, error: `Unknown operation type` };
}
}
github-actions
bot
added
cc:pending
cc:done
needs-human:review-issue
2 participants
Summary
Key Changes
workers/git/git_worker.ts- Worker thread that handles git push/pull/setRemoteUrl operationssrc/ipc/utils/git_worker_utils.ts- Wrapper utilities for spawning and communicating with the workershared/git_worker_types.ts- Type-safe worker communication typeshandlePushToGithubto use worker-based git operations instead of direct callsTechnical Details
The worker uses
child_process.execFiledirectly instead ofdugiteto avoid module resolution issues in packaged Electron apps. It replicates dugite's environment setup (GIT_EXEC_PATH, templates, etc.) to ensure the bundled git binary works correctly.Test plan
Fixes #2592
🤖 Generated with Claude Code
Summary by cubic
Run GitHub sync in a worker thread to keep the UI responsive during long git operations. Fixes #2592 where chat input froze until sync finished.
Written for commit c033417. Summary will update on new commits.