fix(studio-server): an agent turn cut by a claim commits its part before the claim

Author: miguel-heygen

packages/studio-server/src/history/projectHistory.test.ts

@@ -364,7 +364,7 @@ describe("claim: a writer that records after writing", () => {
     expect(read("index.html")).toBe("A");
   });
 
-  it("an agent's turn that ends after Studio's edit to the same file does not jam Cmd+Z", async () => {
+  it("an agent's turn cut by Studio's edit becomes an entry before it and one after, so Cmd+Z walks back in order", async () => {
     const { history, write, read } = await project({ "index.html": "A" });
     const window = await history.beginWindow(agent, "Agent turn");
     write("index.html", "B");
@@ -373,15 +373,87 @@ describe("claim: a writer that records after writing", () => {
     await history.claim(you, "Moved Title", ["index.html"], {
       overwrote: { "index.html": fileContentVersion("B") },
     });
-    await window.close();
-    expect(history.list().map((entry) => entry.label)).toEqual(["Moved Title", "Agent turn"]);
-    expect(history.next("back")?.label, "the button names the step Cmd+Z takes").toBe(
+    write("index.html", "D");
+    expect(await window.close(), "the window still returns the entry it became").toMatchObject({
+      id: window.id,
+    });
+    expect(history.list().map((entry) => entry.label)).toEqual([
+      "Agent turn",
       "Moved Title",
-    );
-    expect(await history.step("back", you)).toMatchObject({ ok: true });
-    expect(read("index.html")).toBe("B");
-    expect(await history.step("back", you)).toMatchObject({ ok: true });
-    expect(read("index.html")).toBe("A");
+      "Agent turn",
+    ]);
+    for (const expected of ["C", "B", "A"]) {
+      expect(await history.step("back", you)).toMatchObject({ ok: true });
+      expect(read("index.html")).toBe(expected);
+    }
+  });
+
+  it("a cut agent turn with nothing after the cut ends as the entry it became at the cut", async () => {
+    const { history, write, read } = await project({ "index.html": "A", "b.js": "1" });
+    const window = await history.beginWindow(agent, "Agent turn");
+    write("index.html", "B");
+    await history.claim(you, "sweep", []);
+    write("index.html", "C");
+    write("b.js", "2");
+    await history.claim(you, "Moved Title", ["index.html", "b.js"], {
+      overwrote: { "index.html": fileContentVersion("B"), "b.js": fileContentVersion("1") },
+    });
+    write("b.js", "3");
+    await history.claim(you, "Recolored", ["b.js"], {
+      overwrote: { "b.js": fileContentVersion("2") },
+    });
+    expect(await window.close()).toMatchObject({ label: "Agent turn" });
+    for (const [index, b] of [
+      ["C", "2"],
+      ["B", "1"],
+      ["A", "1"],
+    ]) {
+      expect(await history.step("back", you)).toMatchObject({ ok: true });
+      expect([read("index.html"), read("b.js")]).toEqual([index, b]);
+    }
+  });
+
+  it("Cmd+Z undoes an agent's later write first when a held drag claim commits after the agent's turn", async () => {
+    const { history, write, read } = await project({ "index.html": "A" });
+    const window = await history.beginWindow(agent, "Agent turn");
+    write("index.html", "B");
+    await history.claim(you, "sweep", []);
+    write("index.html", "C");
+    await history.claim(you, "Dragged Title", ["index.html"], {
+      coalesceKey: "drag",
+      idleMs: 60_000,
+      overwrote: { "index.html": fileContentVersion("B") },
+    });
+    write("index.html", "D");
+    await window.close();
+    await history.flush();
+    expect(history.list().map((entry) => entry.label)).toEqual([
+      "Agent turn",
+      "Agent turn",
+      "Dragged Title",
+    ]);
+    expect(history.next("back")?.label, "the button names the step Cmd+Z takes").toBe("Agent turn");
+    for (const expected of ["C", "B", "A"]) {
+      expect(await history.step("back", you)).toMatchObject({ ok: true });
+      expect(read("index.html")).toBe(expected);
+    }
+  });
+
+  it("logs an entry with only its own fields, not its window's timer", async () => {
+    const { history, write } = await project({ "index.html": "A" });
+    const window = await history.beginWindow(agent, "Agent turn");
+    write("index.html", "B");
+    await window.close();
+    expect(Object.keys(history.list()[0]!).sort()).toEqual([
+      "endedAt",
+      "files",
+      "id",
+      "label",
+      "pinned",
+      "startedAt",
+      "undone",
+      "who",
+    ]);
   });
 
   it("takes Studio's write out of an agent's open window, and leaves the agent's own writes there", async () => {

packages/studio-server/src/history/projectHistory.ts

@@ -335,14 +335,15 @@ class Engine {
     { coalesceKey, idleMs, overwrote = {} }: ClaimOptions,
   ): Promise<{ id: string } | null> {
     await this.sweep();
-    const taken = this.takeClaimed(who, paths, overwrote);
+    const { taken, split } = this.takeClaimed(who, paths, overwrote);
     if (!taken.length) {
       // A claim under another key still ends the held one.
       if (coalesceKey !== this.claimed?.key) await this.commitClaim();
       return null;
     }
-    // What happened outside before this write is older than it, so it is logged first.
+    // What happened outside, or in a window cut by this claim, before this write is older than it: logged first.
     await this.commitOutside();
+    for (const window of split) await this.commitSoFar(window);
     const group = await this.claimGroup(who, label, coalesceKey);
     for (const change of taken) addChange(group, change.path, change.before, change.after);
     if (coalesceKey) return this.holdClaim(group, coalesceKey, idleMs);
@@ -364,13 +365,13 @@ class Engine {
 
   /**
    * Removes and returns the uncommitted changes to `paths` (project-relative or absolute) filed outside or in another
-   * writer's open window, each cut at the version `who` overwrote.
+   * writer's open window, each cut at the version `who` overwrote; `split` names the windows that keep a part.
    */
   takeClaimed(
     who: HistoryWho,
     paths: readonly string[],
     overwrote: Readonly<Record<string, string>>,
-  ): HistoryFileChange[] {
+  ): { taken: HistoryFileChange[]; split: Group[] } {
     const wanted = new Set(paths.map((path) => this.logPath(path)));
     const at = new Map(
       Object.entries(overwrote).map(([path, version]) => [
@@ -381,17 +382,34 @@ class Engine {
     const others = this.windows.filter((open) => !sameWho(open.who, who));
     const groups = this.outside ? [this.outside, ...others] : others;
     const taken: HistoryFileChange[] = [];
+    const split = new Set<Group>();
     for (const group of groups) {
       for (const change of [...group.changes.values()]) {
         if (!wanted.has(change.path)) continue;
-        const cut = at.get(change.path);
-        const [kept, claimed] = splitAt(change, cut, cut !== undefined && this.blobs.has(cut));
-        group.changes.delete(change.path);
-        if (kept) group.changes.set(kept.path, kept);
+        const [kept, claimed] = this.cutOut(group, change, at.get(change.path));
         if (claimed) taken.push(claimed);
+        if (kept && claimed && group !== this.outside) split.add(group);
       }
     }
-    return taken;
+    return { taken, split: [...split] };
+  }
+
+  /** Cuts `change` in `group` at `cut`: the part before stays in the group; both parts are returned. */
+  cutOut(group: Group, change: HistoryFileChange, cut: string | undefined) {
+    const [kept, claimed] = splitAt(change, cut, cut !== undefined && this.blobs.has(cut));
+    group.changes.delete(change.path);
+    if (kept) group.changes.set(kept.path, kept);
+    return [kept, claimed] as const;
+  }
+
+  /**
+   * Commits a window's writes so far as an entry of its own and keeps the window open. A window holds one change per
+   * file, so without this the writer's next write to a file cut by a claim would span the claimer's edit.
+   */
+  async commitSoFar(window: Group): Promise<void> {
+    window.entry = await this.commit({ ...window, id: randomUUID() });
+    window.changes = new Map();
+    window.startedAt = this.now();
   }
 
   holdClaim(
@@ -444,9 +462,9 @@ class Engine {
 
   async commit(group: Group, extra: Partial<HistoryEntry> = {}): Promise<HistoryEntry | null> {
     if (!group.changes.size) return null;
-    const { changes, ...rest } = group;
+    const { id, who, label, startedAt, changes } = group;
     const files = [...changes.values()].sort((a, b) => a.path.localeCompare(b.path));
-    const entry: HistoryEntry = { ...rest, endedAt: this.now(), files, ...extra };
+    const entry: HistoryEntry = { id, who, label, startedAt, endedAt: this.now(), files, ...extra };
     this.log.entries.push(entry);
     try {
       saveRecord(this.logFile, this.log, { type: "entry", entry });
@@ -530,7 +548,8 @@ class Engine {
     if (!this.windows.includes(window)) return window.entry ?? null;
     clearTimeout(window.idleTimer);
     this.windows = this.windows.filter((open) => open !== window);
-    window.entry = await this.commit(window);
+    // A window cut by a claim may have nothing after the cut: it became the entry committed then.
+    window.entry = (await this.commit(window)) ?? window.entry ?? null;
     return window.entry;
   }
 

packages/studio-server/src/routes/history.test.ts

@@ -80,6 +80,26 @@ describe("history routes", () => {
     });
   });
 
+  it("name the step the engine takes when it steps past an entry whose file moved on", async () => {
+    const { projectDir, history, call } = await demoProject();
+    const write = (text: string) => writeFileSync(join(projectDir, "index.html"), text);
+    const agent = { kind: "agent" as const, name: "Agent" };
+    const you = { kind: "person" as const, name: "You" };
+    const window = await history.beginWindow(agent, "Agent turn");
+    write("B");
+    await history.claim(you, "sweep", []);
+    write("C");
+    await history.claim(you, "Dragged Title", ["index.html"], {
+      coalesceKey: "drag",
+      idleMs: 60_000,
+      overwrote: { "index.html": fileContentVersion("B") },
+    });
+    write("D");
+    await window.close();
+    await history.flush();
+    expect((await (await call("")).json()).back).toMatchObject({ label: "Agent turn" });
+  });
+
   it("label an undo's writes with Studio's write token, so their echo reads as Studio's own", async () => {
     const { projectDir, call } = await demoProject();
     writeFileSync(join(projectDir, "index.html"), "B");

packages/studio/src/hooks/usePersistentEditHistory.test.ts

@@ -235,3 +235,16 @@ it("a step whose reply cannot be read says so, instead of throwing", async () =>
     message: "The history's reply was unreadable.",
   });
 });
+
+it("a step that cannot reach the server says so", async () => {
+  const { hook, readFile } = await studio();
+  const real = globalThis.fetch;
+  vi.stubGlobal("fetch", (url: string, init?: RequestInit) =>
+    url.endsWith("/history/step") ? Promise.reject(new TypeError("fetch failed")) : real(url, init),
+  );
+  expect(await act(() => hook().undo({ readFile }))).toEqual({
+    ok: false,
+    reason: "failed",
+    message: "Studio could not reach its server.",
+  });
+});