Skip to content

Commit 36454b3

Browse files
bborninoclaude
andcommitted
fix(worker): aggregate collection_tags across all locales before insert
Addresses CodeRabbit's review feedback on PR #178: the collectionTags delete+insert was running inside the per-locale loop, so multi-locale collections ended up with only the last-processed locale's tags, silently overwriting earlier locales' tags each iteration. Now the per-locale loop only aggregates a Set<string> union of tags (image processing and collectionData/collectionAuthors handling unchanged), and a single delete+insert runs once after the loop completes, mirroring sync-post's existing tag-union pattern. Also replaces the near-duplicate "multiple tags" test with a tag-replacement test (sync twice with different tag sets, assert old tags are gone), and adds a test asserting the union/single-insert behavior across multiple locales. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent c459bd2 commit 36454b3

2 files changed

Lines changed: 229 additions & 22 deletions

File tree

apps/worker/src/tasks/sync-collection/processor.test.ts

Lines changed: 204 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,7 @@ published: "2023-01-01T00:00:00Z"
391391
]);
392392
});
393393

394-
test("Handles collection with multiple tags", async () => {
394+
test("Replaces tags when synced again with a different tag set", async () => {
395395
const insertCollectionValues = vi.fn().mockReturnValue({
396396
onConflictDoNothing: vi.fn(),
397397
});
@@ -442,6 +442,21 @@ test("Handles collection with multiple tags", async () => {
442442
return Promise.reject();
443443
}) as never);
444444

445+
vi.mocked(github.getContentsRawStream).mockImplementation((params) => {
446+
if (
447+
params.path ===
448+
"/content/example-author/collections/example-collection/cover.png"
449+
) {
450+
const buffer = Buffer.from(mockImage, "base64");
451+
return Promise.resolve({
452+
data: Readable.toWeb(Readable.from(buffer)) as never,
453+
status: 200,
454+
});
455+
}
456+
return Promise.reject();
457+
});
458+
459+
// First sync: original tag set
445460
vi.mocked(github.getContentsRaw).mockImplementation((params) => {
446461
if (
447462
params.path ===
@@ -464,14 +479,44 @@ tags:
464479
return Promise.reject();
465480
});
466481

467-
vi.mocked(github.getContentsRawStream).mockImplementation((params) => {
482+
await processor({
483+
data: {
484+
author: "example-author",
485+
collection: "example-collection",
486+
ref: "main",
487+
},
488+
} as unknown as Job<TaskInputs["sync-collection"]>);
489+
490+
expect(insertTagsValues).toBeCalledWith([
491+
{
492+
collectionSlug: "example-collection",
493+
tag: "javascript",
494+
},
495+
{
496+
collectionSlug: "example-collection",
497+
tag: "tutorial",
498+
},
499+
]);
500+
501+
insertTagsValues.mockClear();
502+
deleteWhere.mockClear();
503+
504+
// Second sync: a different tag set
505+
vi.mocked(github.getContentsRaw).mockImplementation((params) => {
468506
if (
469507
params.path ===
470-
"/content/example-author/collections/example-collection/cover.png"
508+
"/content/example-author/collections/example-collection/index.md"
471509
) {
472-
const buffer = Buffer.from(mockImage, "base64");
473510
return Promise.resolve({
474-
data: Readable.toWeb(Readable.from(buffer)) as never,
511+
data: `---
512+
title: "Example Collection"
513+
description: "A test collection"
514+
coverImg: "./cover.png"
515+
published: "2023-01-01T00:00:00Z"
516+
tags:
517+
- rust
518+
---
519+
`,
475520
status: 200,
476521
});
477522
}
@@ -486,18 +531,170 @@ tags:
486531
},
487532
} as unknown as Job<TaskInputs["sync-collection"]>);
488533

489-
// Both tags should be inserted, and the old association deleted first
534+
// Old tags were deleted and only the new tag set remains
490535
expect(deleteWhere).toBeCalledWith(
491536
eq(collectionTags.collectionSlug, "example-collection"),
492537
);
493538
expect(insertTagsValues).toBeCalledWith([
494539
{
495540
collectionSlug: "example-collection",
541+
tag: "rust",
542+
},
543+
]);
544+
expect(insertTagsValues).not.toBeCalledWith(
545+
expect.arrayContaining([expect.objectContaining({ tag: "javascript" })]),
546+
);
547+
expect(insertTagsValues).not.toBeCalledWith(
548+
expect.arrayContaining([expect.objectContaining({ tag: "tutorial" })]),
549+
);
550+
});
551+
552+
test("Unions tags across all locales", async () => {
553+
const insertCollectionValues = vi.fn().mockReturnValue({
554+
onConflictDoNothing: vi.fn(),
555+
});
556+
const insertCollectionDataValues = vi.fn().mockReturnValue({
557+
onConflictDoUpdate: vi.fn(),
558+
});
559+
const insertAuthorValues = vi.fn();
560+
const insertTagsValues = vi.fn();
561+
vi.mocked(db.insert).mockImplementation((table) => {
562+
if (table === collections) {
563+
return { values: insertCollectionValues } as never;
564+
}
565+
if (table === collectionData) {
566+
return { values: insertCollectionDataValues } as never;
567+
}
568+
if (table === collectionAuthors) {
569+
return { values: insertAuthorValues } as never;
570+
}
571+
if (table === collectionTags) {
572+
return { values: insertTagsValues } as never;
573+
}
574+
throw new Error(`Unexpected table: ${table}`);
575+
});
576+
577+
const deleteCollectionAuthorsWhere = vi.fn();
578+
const deleteCollectionTagsWhere = vi.fn();
579+
vi.mocked(db.delete).mockImplementation((table) => {
580+
if (table === collectionAuthors) {
581+
return { where: deleteCollectionAuthorsWhere } as never;
582+
}
583+
if (table === collectionTags) {
584+
return { where: deleteCollectionTagsWhere } as never;
585+
}
586+
throw new Error(`Unexpected table: ${table}`);
587+
});
588+
589+
// Return folder listing with both index.md and index.es.md
590+
vi.mocked(github.getContents).mockImplementation(((params: {
591+
path: string;
592+
}) => {
593+
if (
594+
params.path ===
595+
"/content/example-author/collections/multilang-tags-collection/"
596+
) {
597+
return Promise.resolve({
598+
data: {
599+
entries: [
600+
{
601+
name: "index.md",
602+
path: "content/example-author/collections/multilang-tags-collection/index.md",
603+
},
604+
{
605+
name: "index.es.md",
606+
path: "content/example-author/collections/multilang-tags-collection/index.es.md",
607+
},
608+
],
609+
},
610+
status: 200,
611+
});
612+
}
613+
return Promise.reject();
614+
}) as never);
615+
616+
vi.mocked(github.getContentsRaw).mockImplementation((params) => {
617+
if (
618+
params.path ===
619+
"/content/example-author/collections/multilang-tags-collection/index.md"
620+
) {
621+
return Promise.resolve({
622+
data: `---
623+
title: "English Collection"
624+
description: "A test collection"
625+
coverImg: "./cover.png"
626+
published: "2023-01-01T00:00:00Z"
627+
tags:
628+
- javascript
629+
- tutorial
630+
---
631+
`,
632+
status: 200,
633+
});
634+
}
635+
if (
636+
params.path ===
637+
"/content/example-author/collections/multilang-tags-collection/index.es.md"
638+
) {
639+
return Promise.resolve({
640+
data: `---
641+
title: "Colección en Español"
642+
description: "A test collection"
643+
coverImg: "./cover.png"
644+
published: "2023-01-01T00:00:00Z"
645+
tags:
646+
- espanol
647+
---
648+
`,
649+
status: 200,
650+
});
651+
}
652+
return Promise.reject();
653+
});
654+
655+
vi.mocked(github.getContentsRawStream).mockImplementation((params) => {
656+
if (
657+
params.path ===
658+
"/content/example-author/collections/multilang-tags-collection/cover.png"
659+
) {
660+
const buffer = Buffer.from(mockImage, "base64");
661+
return Promise.resolve({
662+
data: Readable.toWeb(Readable.from(buffer)) as never,
663+
status: 200,
664+
});
665+
}
666+
return Promise.reject();
667+
});
668+
669+
await processor({
670+
data: {
671+
author: "example-author",
672+
collection: "multilang-tags-collection",
673+
ref: "main",
674+
},
675+
} as unknown as Job<TaskInputs["sync-collection"]>);
676+
677+
// Assert: tags from both locales are unioned and deduped in a single insert
678+
expect(insertTagsValues).toBeCalledTimes(1);
679+
expect(insertTagsValues).toBeCalledWith([
680+
{
681+
collectionSlug: "multilang-tags-collection",
496682
tag: "javascript",
497683
},
498684
{
499-
collectionSlug: "example-collection",
685+
collectionSlug: "multilang-tags-collection",
500686
tag: "tutorial",
501687
},
688+
{
689+
collectionSlug: "multilang-tags-collection",
690+
tag: "espanol",
691+
},
502692
]);
693+
694+
// Assert: the tags delete ran once (not once per locale, unlike author associations)
695+
expect(deleteCollectionTagsWhere).toBeCalledTimes(1);
696+
expect(deleteCollectionTagsWhere).toBeCalledWith(
697+
eq(collectionTags.collectionSlug, "multilang-tags-collection"),
698+
);
699+
expect(deleteCollectionAuthorsWhere).toBeCalledTimes(2);
503700
});

apps/worker/src/tasks/sync-collection/processor.ts

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,8 @@ export default createProcessor(
9797
[] as Array<{ entry: Entry; locale: string }>,
9898
);
9999

100+
const allTags = new Set<string>();
101+
100102
// Check if coverImg or socialImg have changed since last edit, if so upload to S3
101103
for (const { entry, locale } of collectionEntries) {
102104
const contentUrl = new URL(entry.path, "http://localhost");
@@ -118,6 +120,10 @@ export default createProcessor(
118120
const { data } = matter(contentResponse.data);
119121
const collectionParsedData = Value.Parse(CollectionMetaSchema, data);
120122

123+
if (collectionParsedData.tags) {
124+
collectionParsedData.tags.forEach((tag) => allTags.add(tag));
125+
}
126+
121127
let coverImgKey: string | null = null;
122128
let socialImgKey: string | null = null;
123129
if (collectionParsedData.coverImg) {
@@ -216,22 +222,26 @@ export default createProcessor(
216222
})),
217223
);
218224
}
219-
220-
// Delete existing tag associations for this collection
221-
await tx
222-
.delete(collectionTags)
223-
.where(eq(collectionTags.collectionSlug, collectionId));
224-
225-
// Insert new tag associations
226-
if (collectionParsedData.tags && collectionParsedData.tags.length > 0) {
227-
await tx.insert(collectionTags).values(
228-
collectionParsedData.tags.map((tag) => ({
229-
collectionSlug: collectionId,
230-
tag,
231-
})),
232-
);
233-
}
234225
});
235226
}
227+
228+
const tags = [...allTags];
229+
230+
await db.transaction(async (tx) => {
231+
// Delete existing tag associations for this collection
232+
await tx
233+
.delete(collectionTags)
234+
.where(eq(collectionTags.collectionSlug, collectionId));
235+
236+
// Insert new tag associations
237+
if (tags.length > 0) {
238+
await tx.insert(collectionTags).values(
239+
tags.map((tag) => ({
240+
collectionSlug: collectionId,
241+
tag,
242+
})),
243+
);
244+
}
245+
});
236246
},
237247
);

0 commit comments

Comments
 (0)