docs/design/conditional-format-multi-range.md (1)

1-156: Well-structured design document covering the multi-range migration comprehensively.

The document clearly outlines goals, data model changes, affected modules, migration strategy with backward compatibility, and risk mitigation. This provides excellent context for the implementation.

Minor nit at Line 115: "CLI interface" is redundant since CLI already stands for "Command Line Interface". Consider using just "CLI" or "Command-line interface".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/design/conditional-format-multi-range.md` around lines 1 - 156, Change
the redundant heading "CLI interface" to either "CLI" or "Command-line
interface" in the design doc near the migration script section (the block that
shows usage for migrate-yorkie-cf-ranges.ts and the pnpm commands); update the
heading text only (leave the example commands and script name
migrate-yorkie-cf-ranges.ts unchanged).

packages/backend/scripts/migrate-yorkie-cf-ranges.ts (1)

140-165: Consider checking for changes before calling doc.update().

The current pattern calls doc.update() unconditionally and determines changed inside the callback. This differs from the reference migration in migrate-yorkie-worksheet-shape.ts, which parses the snapshot first and only calls doc.update() when changes are needed.

While functionally correct (the sync is skipped when changed is false), calling doc.update() without changes may still create an empty checkpoint in Yorkie's history. Consider restructuring to check for migration necessity before mutating:

♻️ Suggested restructure to avoid unnecessary update calls

 async function migrateDocument(
   client: Client,
   documentId: string,
 ): Promise<{ changed: boolean }> {
   const doc = new yorkie.Document<Record<string, unknown>>(
     `sheet-${documentId}`,
   );
   await client.attach(doc, { syncMode: SyncMode.Manual });
 
   try {
-    let changed = false;
-    doc.update((root) => {
-      changed = migrateConditionalFormatRanges(
-        root as Record<string, unknown>,
-      );
-    }, 'Migrate conditional format range to ranges array');
+    // Check if migration is needed by inspecting current state
+    const root = doc.getRoot() as Record<string, unknown>;
+    const needsMigration = checkNeedsMigration(root);
+    
+    if (!needsMigration) {
+      return { changed: false };
+    }
 
-    if (changed) {
-      await client.sync(doc);
-    }
+    doc.update((root) => {
+      migrateConditionalFormatRanges(root as Record<string, unknown>);
+    }, 'Migrate conditional format range to ranges array');
+    await client.sync(doc);
 
-    return { changed };
+    return { changed: true };
   } finally {
     await client.detach(doc);
   }
 }
+
+function checkNeedsMigration(root: Record<string, unknown>): boolean {
+  const sheets = root.sheets as Record<string, Record<string, unknown>> | undefined;
+  if (!sheets) return false;
+  
+  for (const worksheet of Object.values(sheets)) {
+    const rules = worksheet.conditionalFormats as Array<Record<string, unknown>> | undefined;
+    if (!rules) continue;
+    for (const rule of rules) {
+      if (!rule.ranges && rule.range) return true;
+    }
+  }
+  return false;
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/backend/scripts/migrate-yorkie-cf-ranges.ts` around lines 140 - 165,
The migrateDocument function currently calls doc.update() unconditionally which
can create empty Yorkie checkpoints; instead, read the document state first
(e.g., via doc.getSnapshot() or equivalent) and run
migrateConditionalFormatRanges on the parsed root to determine if changes are
necessary, then only call doc.update(...) and client.sync(doc) when that
pre-check returns true; keep the existing client.attach/doc.detach and error
flow but move the migration check outside the doc.update callback and invoke
migrateConditionalFormatRanges again inside doc.update only when needed.

packages/backend/scripts/migrate-yorkie-cf-ranges.ts (2)

81-89: --limit is silently ignored when used with --document.

The --limit option is only effective with --all (used at line 119), but the parser accepts --limit with --document without warning. Users may expect --limit to cap the number of --document arguments processed.

Consider adding validation to reject --limit without --all, or document this behavior more clearly.

💡 Optional: Add validation for --limit usage

   return options;
 }
+
+function validateOptions(options: CliOptions): void {
+  if (options.limit !== undefined && !options.processAll) {
+    throw new Error('--limit can only be used with --all');
+  }
+}

Then call validateOptions(options) before returning from parseArgs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/backend/scripts/migrate-yorkie-cf-ranges.ts` around lines 81 - 89,
The parser currently accepts --limit into options.limit even when --document
mode is used (only --all honors limit), so update parseArgs to validate
combinations: after parsing arguments (before returning) call
validateOptions(options) or explicitly check if options.limit is set while
options.all is false and throw a clear Error (e.g. " --limit can only be used
with --all") so users are rejected when using --limit with --document; reference
options.limit, --all, --document, parseArgs, and validateOptions when making the
change.

---

226-247: Consider deduplicating Yorkie keys.

If a user provides both --yorkie-key sheet-xxx and --document xxx, the same key would be processed twice. While harmless (second pass finds ranges already set), it's redundant and could confuse the operator reviewing logs.

💡 Optional: Use a Set to deduplicate keys

-    const yorkieKeys: string[] = [...options.yorkieKeys];
+    const yorkieKeySet = new Set<string>(options.yorkieKeys);

     if (options.processAll || options.documentIds.length > 0) {
       // ... existing code ...
       for (const document of documents) {
-        yorkieKeys.push(`sheet-${document.id}`);
+        yorkieKeySet.add(`sheet-${document.id}`);
       }
     }

+    const yorkieKeys = [...yorkieKeySet];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/backend/scripts/migrate-yorkie-cf-ranges.ts` around lines 226 - 247,
The yorkieKeys array can contain duplicates when both --yorkie-key and
--document are used; update the logic that builds yorkieKeys (the yorkieKeys
variable populated from options.yorkieKeys and the loop that pushes
`sheet-${document.id}` for each document returned by loadDocuments) to
deduplicate entries before further processing (e.g., convert to a Set and back
or use a Set throughout) so each yorkie key is processed only once and logs
won't show redundant work.