Skip to content

Commit 68bf1ff

Browse files
dcramerclaude
andcommitted
fix(sync): Fix metadata merge bug, improve type safety, add tests
- Fix metadata replacement bug in importSubtasksFromIssueBody that would wipe integration metadata (github/shortcut keys) when updating existing subtasks — now merges with existing metadata - Change createData type from Record<string, unknown> to CreateTaskInput for compile-time type safety, removing unsafe cast in saveMetadata - Add end-to-end test for sync pull creating subtasks locally - Add test for --update updating existing subtasks (not just creating) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 1fb1cd2 commit 68bf1ff

5 files changed

Lines changed: 134 additions & 6 deletions

File tree

src/cli/import.test.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -492,6 +492,65 @@ describe("import command", () => {
492492
expect(subtask!.parent_id).toBe("task600");
493493
});
494494

495+
it("updates existing subtasks during --update", async () => {
496+
// First import: task with a subtask
497+
githubMock.getIssue(
498+
"test-owner",
499+
"test-repo",
500+
700,
501+
createIssueFixture({
502+
number: 700,
503+
title: "Task With Sub",
504+
body: createFullDexIssueBody({
505+
context: "Has subtask",
506+
rootMetadata: { id: "task700" },
507+
subtasks: [{ id: "existsub1", name: "Original Name", priority: 1 }],
508+
}),
509+
}),
510+
);
511+
await runCli(["import", "#700"], { storage });
512+
513+
let tasks = await storage.readAsync();
514+
expect(tasks.tasks).toHaveLength(2);
515+
const originalSub = tasks.tasks.find((t) => t.id === "existsub1");
516+
expect(originalSub).toBeDefined();
517+
expect(originalSub!.name).toBe("Original Name");
518+
519+
// Second import with --update: subtask has changed
520+
githubMock.getIssue(
521+
"test-owner",
522+
"test-repo",
523+
700,
524+
createIssueFixture({
525+
number: 700,
526+
title: "Task With Sub",
527+
body: createFullDexIssueBody({
528+
context: "Has subtask",
529+
rootMetadata: { id: "task700" },
530+
subtasks: [
531+
{
532+
id: "existsub1",
533+
name: "Updated Name",
534+
priority: 3,
535+
completed: true,
536+
},
537+
],
538+
}),
539+
}),
540+
);
541+
await runCli(["import", "#700", "--update"], { storage });
542+
543+
tasks = await storage.readAsync();
544+
// Should still have 2 tasks, not 3 (no duplicate)
545+
expect(tasks.tasks).toHaveLength(2);
546+
const updatedSub = tasks.tasks.find((t) => t.id === "existsub1");
547+
expect(updatedSub).toBeDefined();
548+
expect(updatedSub!.name).toBe("Updated Name");
549+
expect(updatedSub!.priority).toBe(3);
550+
expect(updatedSub!.completed).toBe(true);
551+
expect(updatedSub!.parent_id).toBe("task700");
552+
});
553+
495554
it("shows subtask counts in --all --dry-run", async () => {
496555
githubMock.listIssues("test-owner", "test-repo", [
497556
createIssueFixture({

src/cli/import.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -498,7 +498,9 @@ async function importSubtasksFromIssueBody(
498498
completed: subtask.completed,
499499
started_at: subtask.started_at,
500500
result: subtask.result,
501-
metadata: subtask.metadata,
501+
metadata: subtask.metadata
502+
? { ...existing.metadata, ...subtask.metadata }
503+
: existing.metadata,
502504
});
503505
idMapping.set(subtask.id, existing.id);
504506
updated++;

src/cli/sync.test.ts

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
setupGitHubMock,
1212
cleanupGitHubMock,
1313
createIssueFixture,
14+
createFullDexIssueBody,
1415
setupShortcutMock,
1516
cleanupShortcutMock,
1617
createStoryFixture,
@@ -298,6 +299,74 @@ describe("sync command", () => {
298299
expect(out).toContain("1 task(s)");
299300
expect(out).toContain("1 updated");
300301
});
302+
303+
it("creates subtasks locally when pulling from remote", async () => {
304+
const taskId = await createTask("Parent task", {
305+
description: "context",
306+
});
307+
308+
// First sync to create the issue
309+
githubMock.listIssues("test-owner", "test-repo", []);
310+
githubMock.createIssue(
311+
"test-owner",
312+
"test-repo",
313+
createIssueFixture({ number: 500, title: "Parent task" }),
314+
);
315+
await run(["sync", taskId]);
316+
fixture.output.stdout.length = 0;
317+
318+
// Read the task to get its updated_at
319+
const store = await fixture.storage.readAsync();
320+
const parentTask = store.tasks.find((t) => t.id === taskId)!;
321+
322+
// Remote is newer and has a subtask that doesn't exist locally
323+
const futureTime = new Date(
324+
new Date(parentTask.updated_at).getTime() + 3600000,
325+
).toISOString();
326+
327+
const remoteBody = createFullDexIssueBody({
328+
context: "context",
329+
rootMetadata: {
330+
id: taskId,
331+
updated_at: futureTime,
332+
},
333+
subtasks: [
334+
{
335+
id: "remotesub1",
336+
name: "Remote Subtask",
337+
priority: 2,
338+
created_at: futureTime,
339+
updated_at: futureTime,
340+
},
341+
],
342+
});
343+
344+
// Page 1: issue with newer updated_at and subtask
345+
githubMock.listIssues("test-owner", "test-repo", [
346+
createIssueFixture({
347+
number: 500,
348+
title: "Parent task",
349+
body: remoteBody,
350+
labels: [{ name: "dex" }, { name: "dex:pending" }],
351+
}),
352+
]);
353+
// Page 2: empty (end of pagination)
354+
githubMock.listIssues("test-owner", "test-repo", []);
355+
356+
await run(["sync"]);
357+
358+
// Verify the subtask was created locally
359+
const updatedStore = await fixture.storage.readAsync();
360+
const subtask = updatedStore.tasks.find((t) => t.id === "remotesub1");
361+
expect(subtask).toBeDefined();
362+
expect(subtask!.name).toBe("Remote Subtask");
363+
expect(subtask!.parent_id).toBe(taskId);
364+
expect(subtask!.priority).toBe(2);
365+
366+
// Verify summary includes pulled from remote
367+
const out = fixture.output.stdout.join("\n");
368+
expect(out).toContain("pulled from remote");
369+
});
301370
});
302371

303372
describe("error handling", () => {

src/cli/sync.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -456,9 +456,7 @@ async function saveMetadata(
456456
for (const subtaskResult of result.subtaskResults) {
457457
if (subtaskResult.needsCreation && subtaskResult.createData) {
458458
// Create subtask that exists in remote but not locally
459-
await service.create(
460-
subtaskResult.createData as Parameters<typeof service.create>[0],
461-
);
459+
await service.create(subtaskResult.createData);
462460
} else {
463461
await saveMetadata(service, integrationId, subtaskResult);
464462
}

src/core/sync/registry.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { Task, TaskStore } from "../../types.js";
1+
import type { Task, TaskStore, CreateTaskInput } from "../../types.js";
22
import type { IntegrationId, SyncAllOptions } from "./interface.js";
33

44
/**
@@ -20,7 +20,7 @@ export interface SyncResult {
2020
/** True if the task doesn't exist locally and needs to be created */
2121
needsCreation?: boolean;
2222
/** Full task data for creation (used when needsCreation is true) */
23-
createData?: Record<string, unknown>;
23+
createData?: CreateTaskInput;
2424
}
2525

2626
/**

0 commit comments

Comments
 (0)