Skip to content

Commit 393c06f

Browse files
RomneyDaRomneyDa
committed
feat: unify edit tool apply handling
1 parent 832b2d6 commit 393c06f

9 files changed

+924
-264
lines changed

‎gui/src/redux/thunks/handleApplyStateUpdate.test.ts‎

Lines changed: 762 additions & 0 deletions
Large diffs are not rendered by default.

‎gui/src/redux/thunks/handleApplyStateUpdate.ts‎

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { createAsyncThunk } from "@reduxjs/toolkit";
2-
import { ApplyState } from "core";
2+
import { ApplyState, ApplyToFilePayload } from "core";
33
import { EDIT_MODE_STREAM_ID } from "core/edit/constants";
44
import { logAgentModeEditOutcome } from "../../util/editOutcomeLogger";
55
import {
@@ -105,13 +105,30 @@ export const handleApplyStateUpdate = createAsyncThunk<
105105
},
106106
);
107107

108-
export const handleEditToolApplyError = createAsyncThunk<
108+
export const applyForEditTool = createAsyncThunk<
109109
void,
110-
{ toolCallId: string },
110+
ApplyToFilePayload & { toolCallId: string },
111111
ThunkApiType
112-
>(
113-
"apply/handleEditError",
114-
async ({ toolCallId }, { dispatch, getState, extra }) => {
112+
>("apply/editTool", async (payload, { dispatch, getState, extra }) => {
113+
const { toolCallId, streamId } = payload;
114+
dispatch(
115+
updateApplyState({
116+
streamId,
117+
toolCallId,
118+
status: "not-started",
119+
}),
120+
);
121+
122+
let didError = false;
123+
try {
124+
const response = await extra.ideMessenger.request("applyToFile", payload);
125+
if (response.status === "error") {
126+
didError = true;
127+
}
128+
} catch (e) {
129+
didError = true;
130+
}
131+
if (didError) {
115132
const state = getState();
116133

117134
const toolCallState = selectToolCallById(state, toolCallId);
@@ -149,5 +166,5 @@ export const handleEditToolApplyError = createAsyncThunk<
149166
}),
150167
);
151168
}
152-
},
153-
);
169+
}
170+
});

‎gui/src/util/clientTools/editImpl.ts‎

Lines changed: 6 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { resolveRelativePathInDir } from "core/util/ideUtils";
22
import { v4 as uuid } from "uuid";
3-
import { updateApplyState } from "../../redux/slices/sessionSlice";
4-
import { handleEditToolApplyError } from "../../redux/thunks/handleApplyStateUpdate";
3+
import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate";
54
import { ClientToolImpl } from "./callClientTool";
65
export const editToolImpl: ClientToolImpl = async (
76
args,
@@ -21,29 +20,15 @@ export const editToolImpl: ClientToolImpl = async (
2120
throw new Error(`${args.filepath} does not exist`);
2221
}
2322
const streamId = uuid();
24-
extras.dispatch(
25-
updateApplyState({
26-
streamId,
27-
toolCallId,
28-
status: "not-started",
29-
}),
30-
);
31-
void extras.ideMessenger
32-
.request("applyToFile", {
23+
void extras.dispatch(
24+
applyForEditTool({
3325
streamId,
3426
text: args.changes,
3527
toolCallId,
3628
filepath: firstUriMatch,
37-
})
38-
.then((res) => {
39-
if (res.status === "error") {
40-
void extras.dispatch(
41-
handleEditToolApplyError({
42-
toolCallId,
43-
}),
44-
);
45-
}
46-
});
29+
}),
30+
);
31+
4732
return {
4833
respondImmediately: false,
4934
output: undefined, // No immediate output.

‎gui/src/util/clientTools/multiEditImpl.test.ts‎

Lines changed: 56 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import * as ideUtils from "core/util/ideUtils";
22
import { beforeEach, describe, expect, it, Mock, vi } from "vitest";
3+
import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate";
34
import { ClientToolExtras } from "./callClientTool";
45
import { multiEditImpl } from "./multiEditImpl";
56

@@ -11,14 +12,20 @@ vi.mock("core/util/ideUtils", () => ({
1112
resolveRelativePathInDir: vi.fn(),
1213
}));
1314

15+
vi.mock("../../redux/thunks/handleApplyStateUpdate", () => ({
16+
applyForEditTool: vi.fn(),
17+
}));
18+
1419
describe("multiEditImpl", () => {
1520
let mockExtras: ClientToolExtras;
1621
let mockResolveRelativePathInDir: Mock;
22+
let mockApplyForEditTool: Mock;
1723

1824
beforeEach(() => {
1925
vi.clearAllMocks();
2026

2127
mockResolveRelativePathInDir = vi.mocked(ideUtils.resolveRelativePathInDir);
28+
mockApplyForEditTool = vi.mocked(applyForEditTool);
2229

2330
mockExtras = {
2431
getState: vi.fn(() => ({
@@ -106,16 +113,13 @@ describe("multiEditImpl", () => {
106113
mockExtras,
107114
);
108115

109-
expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith(
110-
"applyToFile",
111-
{
112-
streamId: "test-uuid",
113-
toolCallId: "id",
114-
text: "Hi world",
115-
filepath: "/test/file.txt",
116-
isSearchAndReplace: true,
117-
},
118-
);
116+
expect(mockApplyForEditTool).toHaveBeenCalledWith({
117+
streamId: "test-uuid",
118+
toolCallId: "id",
119+
text: "Hi world",
120+
filepath: "/test/file.txt",
121+
isSearchAndReplace: true,
122+
});
119123
});
120124

121125
it("should apply multiple edits sequentially", async () => {
@@ -135,16 +139,13 @@ describe("multiEditImpl", () => {
135139
mockExtras,
136140
);
137141

138-
expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith(
139-
"applyToFile",
140-
{
141-
streamId: "test-uuid",
142-
toolCallId: "id",
143-
text: "Hi universe\nGoodbye universe",
144-
filepath: "/test/file.txt",
145-
isSearchAndReplace: true,
146-
},
147-
);
142+
expect(mockApplyForEditTool).toHaveBeenCalledWith({
143+
streamId: "test-uuid",
144+
toolCallId: "id",
145+
text: "Hi universe\nGoodbye universe",
146+
filepath: "/test/file.txt",
147+
isSearchAndReplace: true,
148+
});
148149
});
149150

150151
it("should handle edits that depend on previous edits", async () => {
@@ -164,16 +165,13 @@ describe("multiEditImpl", () => {
164165
mockExtras,
165166
);
166167

167-
expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith(
168-
"applyToFile",
169-
{
170-
streamId: "test-uuid",
171-
toolCallId: "id",
172-
text: "let x = 2;",
173-
filepath: "/test/file.txt",
174-
isSearchAndReplace: true,
175-
},
176-
);
168+
expect(mockApplyForEditTool).toHaveBeenCalledWith({
169+
streamId: "test-uuid",
170+
toolCallId: "id",
171+
text: "let x = 2;",
172+
filepath: "/test/file.txt",
173+
isSearchAndReplace: true,
174+
});
177175
});
178176

179177
it("should throw if string not found in edit sequence", async () => {
@@ -227,16 +225,13 @@ describe("multiEditImpl", () => {
227225
mockExtras,
228226
);
229227

230-
expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith(
231-
"applyToFile",
232-
{
233-
streamId: "test-uuid",
234-
toolCallId: "id",
235-
text: "New content\nLine 2",
236-
filepath: "new.txt",
237-
isSearchAndReplace: true,
238-
},
239-
);
228+
expect(mockApplyForEditTool).toHaveBeenCalledWith({
229+
streamId: "test-uuid",
230+
toolCallId: "id",
231+
text: "New content\nLine 2",
232+
filepath: "new.txt",
233+
isSearchAndReplace: true,
234+
});
240235
});
241236

242237
it("should create and edit new file", async () => {
@@ -254,16 +249,13 @@ describe("multiEditImpl", () => {
254249
mockExtras,
255250
);
256251

257-
expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith(
258-
"applyToFile",
259-
{
260-
streamId: "test-uuid",
261-
toolCallId: "id",
262-
text: "Hello universe",
263-
filepath: "new.txt",
264-
isSearchAndReplace: true,
265-
},
266-
);
252+
expect(mockApplyForEditTool).toHaveBeenCalledWith({
253+
streamId: "test-uuid",
254+
toolCallId: "id",
255+
text: "Hello universe",
256+
filepath: "new.txt",
257+
isSearchAndReplace: true,
258+
});
267259
});
268260
});
269261

@@ -286,16 +278,13 @@ describe("multiEditImpl", () => {
286278
mockExtras,
287279
);
288280

289-
expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith(
290-
"applyToFile",
291-
{
292-
streamId: "test-uuid",
293-
toolCallId: "id",
294-
text: "qux bar qux baz qux",
295-
filepath: "/test/file.txt",
296-
isSearchAndReplace: true,
297-
},
298-
);
281+
expect(mockApplyForEditTool).toHaveBeenCalledWith({
282+
streamId: "test-uuid",
283+
toolCallId: "id",
284+
text: "qux bar qux baz qux",
285+
filepath: "/test/file.txt",
286+
isSearchAndReplace: true,
287+
});
299288
});
300289

301290
it("should handle mixed replace_all settings", async () => {
@@ -315,16 +304,13 @@ describe("multiEditImpl", () => {
315304
mockExtras,
316305
);
317306

318-
expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith(
319-
"applyToFile",
320-
{
321-
streamId: "test-uuid",
322-
toolCallId: "id",
323-
text: "a b a z a",
324-
filepath: "/test/file.txt",
325-
isSearchAndReplace: true,
326-
},
327-
);
307+
expect(mockApplyForEditTool).toHaveBeenCalledWith({
308+
streamId: "test-uuid",
309+
toolCallId: "id",
310+
text: "a b a z a",
311+
filepath: "/test/file.txt",
312+
isSearchAndReplace: true,
313+
});
328314
});
329315
});
330316

@@ -346,25 +332,6 @@ describe("multiEditImpl", () => {
346332
),
347333
).rejects.toThrow("Failed to apply multi edit: Read failed");
348334
});
349-
350-
it("should wrap applyToFile errors", async () => {
351-
mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt");
352-
mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("test");
353-
mockExtras.ideMessenger.request = vi
354-
.fn()
355-
.mockRejectedValue(new Error("Write failed"));
356-
357-
await expect(
358-
multiEditImpl(
359-
{
360-
filepath: "file.txt",
361-
edits: [{ old_string: "test", new_string: "new" }],
362-
},
363-
"id",
364-
mockExtras,
365-
),
366-
).rejects.toThrow("Failed to apply multi edit: Write failed");
367-
});
368335
});
369336

370337
describe("return value", () => {

‎gui/src/util/clientTools/multiEditImpl.ts‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { resolveRelativePathInDir } from "core/util/ideUtils";
22
import { v4 as uuid } from "uuid";
3-
import { handleEditToolApplyError } from "../../redux/thunks/handleApplyStateUpdate";
3+
import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate";
44
import { ClientToolImpl } from "./callClientTool";
55

66
interface EditOperation {
@@ -118,23 +118,15 @@ export const multiEditImpl: ClientToolImpl = async (
118118
}
119119

120120
// Apply the changes to the file
121-
void extras.ideMessenger
122-
.request("applyToFile", {
121+
void extras.dispatch(
122+
applyForEditTool({
123123
streamId,
124124
toolCallId,
125125
text: newContent,
126126
filepath: targetFilepath,
127127
isSearchAndReplace: true,
128-
})
129-
.then((res) => {
130-
if (res.status === "error") {
131-
void extras.dispatch(
132-
handleEditToolApplyError({
133-
toolCallId,
134-
}),
135-
);
136-
}
137-
});
128+
}),
129+
);
138130

139131
// Return success - applyToFile will handle the completion state
140132
return {

0 commit comments

Comments
 (0)