Skip to content

fix: make sure cancel actually aborts push #1831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
charlesvien merged 1 commit into from
Apr 23, 2026
Merged

fix: make sure cancel actually aborts push #1831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions apps/code/src/main/services/git/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,13 +435,15 @@ export class GitService extends TypedEventEmitter<GitServiceEvents> {
remote = "origin",
branch?: string,
setUpstream = false,
signal?: AbortSignal,
): Promise<PushOutput> {
const saga = new PushSaga();
const result = await saga.run({
baseDir: directoryPath,
remote,
branch: branch || undefined,
setUpstream,
signal,
});
if (!result.success) {
return { success: false, message: result.error };
Expand All @@ -464,12 +466,14 @@ export class GitService extends TypedEventEmitter<GitServiceEvents> {
directoryPath: string,
remote = "origin",
branch?: string,
signal?: AbortSignal,
): Promise<PullOutput> {
const saga = new PullSaga();
const result = await saga.run({
baseDir: directoryPath,
remote,
branch: branch || undefined,
signal,
});
if (!result.success) {
return { success: false, message: result.error };
Expand All @@ -488,6 +492,7 @@ export class GitService extends TypedEventEmitter<GitServiceEvents> {
public async publish(
directoryPath: string,
remote = "origin",
signal?: AbortSignal,
): Promise<PublishOutput> {
const currentBranch = await getCurrentBranch(directoryPath);
if (!currentBranch) {
Expand All @@ -499,6 +504,7 @@ export class GitService extends TypedEventEmitter<GitServiceEvents> {
remote,
currentBranch,
true,
signal,
);
return {
success: pushResult.success,
Expand All @@ -511,8 +517,14 @@ export class GitService extends TypedEventEmitter<GitServiceEvents> {
public async sync(
directoryPath: string,
remote = "origin",
signal?: AbortSignal,
): Promise<SyncOutput> {
const pullResult = await this.pull(directoryPath, remote);
const pullResult = await this.pull(
directoryPath,
remote,
undefined,
signal,
);
if (!pullResult.success) {
return {
success: false,
Expand All @@ -521,7 +533,13 @@ export class GitService extends TypedEventEmitter<GitServiceEvents> {
};
}

const pushResult = await this.push(directoryPath, remote);
const pushResult = await this.push(
directoryPath,
remote,
undefined,
false,
signal,
);

const state = await this.getStateSnapshot(directoryPath);

Expand Down
20 changes: 13 additions & 7 deletions apps/code/src/main/trpc/routers/git.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,34 +240,40 @@ export const gitRouter = router({
push: publicProcedure
.input(pushInput)
.output(pushOutput)
.mutation(({ input }) =>
.mutation(({ input, signal }) =>
getService().push(
input.directoryPath,
input.remote,
input.branch,
input.setUpstream,
signal,
),
),

pull: publicProcedure
.input(pullInput)
.output(pullOutput)
.mutation(({ input }) =>
getService().pull(input.directoryPath, input.remote, input.branch),
.mutation(({ input, signal }) =>
getService().pull(
input.directoryPath,
input.remote,
input.branch,
signal,
),
),

publish: publicProcedure
.input(publishInput)
.output(publishOutput)
.mutation(({ input }) =>
getService().publish(input.directoryPath, input.remote),
.mutation(({ input, signal }) =>
getService().publish(input.directoryPath, input.remote, signal),
),

sync: publicProcedure
.input(syncInput)
.output(syncOutput)
.mutation(({ input }) =>
getService().sync(input.directoryPath, input.remote),
.mutation(({ input, signal }) =>
getService().sync(input.directoryPath, input.remote, signal),
),

getGitStatus: publicProcedure
Expand Down
36 changes: 33 additions & 3 deletions apps/code/src/renderer/features/git-interaction/hooks/useGitInteraction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { ANALYTICS_EVENTS } from "@shared/types/analytics";
import { useQueryClient } from "@tanstack/react-query";
import { track } from "@utils/analytics";
import { logger } from "@utils/logger";
import { useMemo } from "react";
import { useMemo, useRef } from "react";

const log = logger.scope("git-interaction");

Expand Down Expand Up @@ -151,6 +151,7 @@ export function useGitInteraction(
const queryClient = useQueryClient();
const store = useGitInteractionStore();
const { actions: modal } = store;
const pushAbortRef = useRef<AbortController | null>(null);

const git = useGitQueries(repoPath);

Expand Down Expand Up @@ -427,6 +428,10 @@ export function useGitInteraction(

const pushMode = mode ?? useGitInteractionStore.getState().pushMode;

pushAbortRef.current?.abort();
const controller = new AbortController();
pushAbortRef.current = controller;

modal.setIsSubmitting(true);
modal.setPushError(null);

Expand All @@ -438,7 +443,10 @@ export function useGitInteraction(
? trpcClient.git.publish
: trpcClient.git.push;

const result = await pushFn.mutate({ directoryPath: repoPath });
const result = await pushFn.mutate(
{ directoryPath: repoPath },
{ signal: controller.signal },
);

if (!result.success) {
const message =
Expand All @@ -458,11 +466,33 @@ export function useGitInteraction(
}

modal.setPushState("success");
} catch (error) {
trackGitAction(taskId, pushMode, false);
if (controller.signal.aborted) {
return;
}
log.error("Push failed", error);
const message = error instanceof Error ? error.message : "Push failed.";
modal.setPushError(message);
modal.setPushState("error");
} finally {
if (pushAbortRef.current === controller) {
Comment on lines 467 to +479
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 trackGitAction duplicated across both catch branches

trackGitAction(taskId, store.pushMode, false) appears identically in both the aborted path and the error path. Per the OnceAndOnlyOnce principle, it can be hoisted before the branch:

Suggested change
modal.setPushState("success");
} catch (error) {
if (controller.signal.aborted) {
trackGitAction(taskId, store.pushMode, false);
return;
}
log.error("Push failed", error);
const message = error instanceof Error ? error.message : "Push failed.";
trackGitAction(taskId, store.pushMode, false);
modal.setPushError(message);
modal.setPushState("error");
} finally {
if (pushAbortRef.current === controller) {
} catch (error) {
trackGitAction(taskId, store.pushMode, false);
if (controller.signal.aborted) {
return;
}
log.error("Push failed", error);
const message = error instanceof Error ? error.message : "Push failed.";
modal.setPushError(message);
modal.setPushState("error");
Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/code/src/renderer/features/git-interaction/hooks/useGitInteraction.ts
Line: 463-476

Comment:
**`trackGitAction` duplicated across both catch branches**

`trackGitAction(taskId, store.pushMode, false)` appears identically in both the aborted path and the error path. Per the OnceAndOnlyOnce principle, it can be hoisted before the branch:

```suggestion
    } catch (error) {
      trackGitAction(taskId, store.pushMode, false);
      if (controller.signal.aborted) {
        return;
      }
      log.error("Push failed", error);
      const message = error instanceof Error ? error.message : "Push failed.";
      modal.setPushError(message);
      modal.setPushState("error");
```

How can I resolve this? If you propose a fix, please make it concise.

pushAbortRef.current = null;
}
modal.setIsSubmitting(false);
}
};

const abortPush = () => {
pushAbortRef.current?.abort();
pushAbortRef.current = null;
};

const closePush = () => {
abortPush();
modal.closePush();
};

const generateCommitMessage = async () => {
if (!repoPath) return;

Expand Down Expand Up @@ -590,7 +620,7 @@ export function useGitInteraction(
actions: {
openAction,
closeCommit: modal.closeCommit,
closePush: modal.closePush,
closePush,
closeBranch: modal.closeBranch,
setCommitMessage: modal.setCommitMessage,
setCommitNextStep: modal.setCommitNextStep,
Expand Down
120 changes: 112 additions & 8 deletions packages/electron-trpc/src/main/__tests__/handleIPCMessage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ describe("api", () => {
},
},
router: testRouter,
subscriptions: new Map(),
operations: new Map(),
});

expect(event.reply).toHaveBeenCalledOnce();
Expand Down Expand Up @@ -96,14 +96,14 @@ describe("api", () => {
},
},
router: testRouter,
subscriptions: new Map(),
operations: new Map(),
});

expect(event.reply).not.toHaveBeenCalled();
});

test("handles subscriptions using observables", async () => {
const subscriptions = new Map();
const operations = new Map();
const ee = new EventEmitter();
const t = trpc.initTRPC.create();
const testRouter = t.router({
Expand Down Expand Up @@ -143,7 +143,7 @@ describe("api", () => {
},
},
internalId: "1-1:1",
subscriptions,
operations,
router: testRouter,
event,
});
Expand Down Expand Up @@ -176,7 +176,7 @@ describe("api", () => {
id: 1,
},
internalId: "1-1:1",
subscriptions,
operations,
router: testRouter,
event,
});
Expand All @@ -194,7 +194,7 @@ describe("api", () => {
});

test("handles subscriptions using async generators", async () => {
const subscriptions = new Map();
const operations = new Map();
const t = trpc.initTRPC.create();

// Simple async generator that yields a single value
Expand Down Expand Up @@ -226,7 +226,7 @@ describe("api", () => {
},
},
internalId: "1-1:1",
subscriptions,
operations,
router: testRouter,
event,
});
Expand Down Expand Up @@ -254,6 +254,110 @@ describe("api", () => {
});
});

test("operation.cancel aborts in-flight mutation signal", async () => {
const event = makeEvent({
reply: vi.fn(),
sender: {
isDestroyed: () => false,
on: () => {},
},
});

const signalCaptured = vi.fn();
const t = trpc.initTRPC.create();
const cancelRouter = t.router({
slowMutation: t.procedure.mutation(async ({ signal }) => {
signalCaptured(signal);
await new Promise<void>((_resolve, reject) => {
signal?.addEventListener("abort", () => {
reject(new Error("aborted"));
});
});
return "should not reach";
}),
});

const operations = new Map();

const mutationCall = handleIPCMessage({
createContext: async () => ({}),
event,
internalId: "1-1:1",
message: {
method: "request",
operation: {
context: {},
id: 1,
input: undefined,
path: "slowMutation",
type: "mutation",
signal: undefined,
},
},
router: cancelRouter,
operations,
});

await vi.waitFor(() => {
expect(signalCaptured).toHaveBeenCalled();
});

expect(operations.has("1-1:1")).toBe(true);

await handleIPCMessage({
createContext: async () => ({}),
event,
internalId: "1-1:1",
message: { method: "operation.cancel", id: 1 },
router: cancelRouter,
operations,
});

await mutationCall;

expect(signalCaptured.mock.calls[0][0].aborted).toBe(true);
expect(event.reply).toHaveBeenCalled();
const lastResponse = event.reply.mock.lastCall?.[1] as {
id: number;
error?: unknown;
};
expect(lastResponse).toMatchObject({ id: 1, error: expect.anything() });
expect(operations.has("1-1:1")).toBe(false);
});

test("query removes itself from operations map on success", async () => {
const event = makeEvent({
reply: vi.fn(),
sender: {
isDestroyed: () => false,
on: () => {},
},
});

const operations = new Map();

await handleIPCMessage({
createContext: async () => ({}),
event,
internalId: "1-1:9",
message: {
method: "request",
operation: {
context: {},
id: 9,
input: { id: "test" },
path: "testQuery",
type: "query",
signal: undefined,
},
},
router: testRouter,
operations,
});

expect(operations.has("1-1:9")).toBe(false);
});

test("subscription responds using custom serializer", async () => {
const event = makeEvent({
reply: vi.fn(),
Expand Down Expand Up @@ -304,7 +408,7 @@ describe("api", () => {
},
},
internalId: "1-1:1",
subscriptions: new Map(),
operations: new Map(),
router: testRouter,
event,
});
Expand Down
Loading
Loading