fix(runner): ensure IDs are added to array elements and default values (commontoolsinc#1908)

* fix(runner): ensure IDs are added to array elements and default values

This change ensures that when writing arrays to cells or pushing to array
cells, all objects are given IDs so they can be stored as separate documents
with links. This applies to both explicitly written arrays and default values
from schemas.

Changes to cell.ts:

1. Modified the `set` method to call `addIDIfNeeded` on each element when
   writing an array, ensuring all objects get IDs before being processed by
   `diffAndUpdate`.

2. Enhanced the `push` method to:
   - Process default values from schemas using `processDefaultValue` to ensure
     they're properly materialized with IDs
   - Apply `addIDIfNeeded` to all elements (both existing defaults and newly
     pushed values) to ensure consistent ID assignment

3. Improved the `update` method's schema validation to:
   - Use `resolveSchema` to properly handle schema references
   - Check for undefined schemas (which allow objects)
   - Consolidate the schema validation logic to determine if objects are
     allowed

4. Added new `addIDIfNeeded` helper function that:
   - Checks if a value is an object without an ID
   - Generates a new ID from the frame's counter if needed
   - Preserves existing IDs when present

New tests in cell.test.ts:

- "set operations with arrays" suite:
  - Tests that objects written as arrays each get their own document ID
  - Verifies that existing IDs are preserved during array writes
  - Uses `asSchema` with `asCell: true` to read back as cells and verify
    each element has a distinct document ID

- "push operations with default values" suite:
  - Tests that default values from schemas are properly materialized with IDs
  - Verifies all objects (defaults + pushed) get unique IDs
  - Tests push operations both with and without schema defaults

These changes ensure that array operations consistently create separate
documents for each object, maintaining proper referential structure in the
storage layer.

* make adding IDs recursive, since the calendar use-case sets objects with arrays

* make toCell non-enumerable so that {...obj} doesn't copy them and accidentally points at the old object

* fix test that was previously broken, but with the bug hidden by our bug

* feat(runner): implement Symbol.iterator for array query result proxies

Add custom Symbol.iterator implementation for array query result proxies
to support spreading and for...of iteration. When iterating over an array
proxy, each element is now wrapped in its own query result proxy,
maintaining reactivity and cell references throughout the iteration.

This enables patterns like:
- const spread = [...proxy.items]
- for (const item of proxy.items) { ... }

Each iterated element maintains its query result proxy wrapper, allowing
access to toCell() and preserving the link to the underlying cell data.

* test(runner): add tests for Symbol.iterator in array query result proxies

Add comprehensive tests verifying Symbol.iterator behavior for array
query result proxies:

- for...of iteration with object elements
- Spread operator with object elements
- Nested array spreading with proper proxy preservation
- Arrays containing cell references (links to other cells)

Tests confirm that iteration and spreading maintain query result proxy
wrappers for each element, allowing access to toCell() and preserving
reactivity throughout iteration. The cell references test specifically
validates that link resolution works correctly when iterating over arrays
of cell links.

* log .length read in tx for Symbol.iterator

* outliner: clone tree before applying edit

otherwise Cell.set turns this back into cell references and this can create loops

(fwiw, we might want to turn this into a structure where each node is its own document with its own id, in which case we'd actually want this behavior)

Author: seefeldb

packages/generated-patterns/integration/patterns/compliance-checklist.pattern.ts

@@ -576,12 +576,8 @@ export const complianceChecklist = recipe<ComplianceChecklistArgs>(
     const insights = lift(computeInsights)(currentTasks);
 
     const tasksView = lift(cloneTasks)(currentTasks);
-    const categorySummaries = lift((snapshot: ComplianceInsights) =>
-      snapshot.categories.map((entry) => ({ ...entry }))
-    )(insights);
-    const gapDetails = lift((snapshot: ComplianceInsights) =>
-      snapshot.gapList.map((entry) => ({ ...entry }))
-    )(insights);
+    const categorySummaries = insights.categories;
+    const gapDetails = insights.gapList;
 
     const coveragePercent = lift((snapshot: ComplianceInsights) =>
       snapshot.coveragePercent

packages/runner/src/builder/types.ts

@@ -49,6 +49,7 @@ export { AuthSchema } from "./schema-lib.ts";
 export {
   ID,
   ID_FIELD,
+  type IDFields,
   NAME,
   type Schema,
   schema,

packages/runner/src/cell.ts

@@ -5,6 +5,7 @@ import {
   type Cell,
   ID,
   ID_FIELD,
+  type IDFields,
   isStreamValue,
   type JSONSchema,
   type OpaqueRef,
@@ -24,7 +25,11 @@ import { diffAndUpdate } from "./data-updating.ts";
 import { resolveLink } from "./link-resolution.ts";
 import { ignoreReadForScheduling, txToReactivityLog } from "./scheduler.ts";
 import { type Cancel, isCancel, useCancelGroup } from "./cancel.ts";
-import { validateAndTransform } from "./schema.ts";
+import {
+  processDefaultValue,
+  resolveSchema,
+  validateAndTransform,
+} from "./schema.ts";
 import { toURI } from "./uri-utils.ts";
 import {
   type LegacyJSONCellLink,
@@ -448,12 +453,15 @@ export class RegularCell<T> implements Cell<T> {
     // retry on conflict.
     if (!this.synced) this.sync();
 
+    // Looks for arrays and makes sure each object gets its own doc.
+    const transformedValue = recursivelyAddIDIfNeeded(newValue);
+
     // TODO(@ubik2) investigate whether i need to check classified as i walk down my own obj
     diffAndUpdate(
       this.runtime,
       this.tx,
       resolveLink(this.tx, this.link, "writeRedirect"),
-      newValue,
+      transformedValue,
       getTopFrame()?.cause,
     );
 
@@ -486,36 +494,36 @@ export class RegularCell<T> implements Cell<T> {
     const resolvedLink = resolveLink(this.tx, this.link);
     const currentValue = this.tx.readValueOrThrow(resolvedLink);
 
-    // If there's no current value, initialize based on schema
+    // If there's no current value, initialize based on schema, even if there is
+    // no default value.
     if (currentValue === undefined) {
-      if (isObject(this.schema)) {
-        // Check if schema allows objects
-        const allowsObject = ContextualFlowControl.isTrueSchema(this.schema) ||
-          this.schema.type === "object" ||
-          (Array.isArray(this.schema.type) &&
-            this.schema.type.includes("object")) ||
-          (this.schema.anyOf &&
-            this.schema.anyOf.some((s) =>
-              typeof s === "object" && s.type === "object"
-            ));
-
-        if (!allowsObject) {
-          throw new Error(
-            "Cannot update with object value - schema does not allow objects",
-          );
-        }
-      } else if (this.schema === false) {
+      const resolvedSchema = resolveSchema(this.schema, this.rootSchema);
+
+      // TODO(seefeld,ubik2): This should all be moved to schema helpers. This
+      // just wants to know whether the value could be an object.
+      const allowsObject = resolvedSchema === undefined ||
+        ContextualFlowControl.isTrueSchema(resolvedSchema) ||
+        (isObject(resolvedSchema) &&
+          (resolvedSchema.type === "object" ||
+            (Array.isArray(resolvedSchema.type) &&
+              resolvedSchema.type.includes("object")) ||
+            (resolvedSchema.anyOf &&
+              resolvedSchema.anyOf.some((s) =>
+                typeof s === "object" && s.type === "object"
+              ))));
+
+      if (!allowsObject) {
         throw new Error(
           "Cannot update with object value - schema does not allow objects",
         );
       }
+
       this.tx.writeValueOrThrow(resolvedLink, {});
     }
 
     // Now update each property
     for (const [key, value] of Object.entries(values)) {
-      // Workaround for type checking, since T can be Cell<> and that's fine.
-      (this.key as any)(key).set(value);
+      (this as Cell<any>).key(key).set(value);
     }
   }
 
@@ -546,14 +554,6 @@ export class RegularCell<T> implements Cell<T> {
       throw new Error("Can't push into non-array value");
     }
 
-    // If this is an object and it doesn't have an ID, add one.
-    const valuesToWrite = value.map((val: any) =>
-      (!isLink(val) && isObject(val) &&
-          (val as { [ID]?: unknown })[ID] === undefined && getTopFrame())
-        ? { [ID]: getTopFrame()!.generatedIdCounter++, ...val }
-        : val
-    );
-
     // If there is no array yet, create it first. We have to do this as a
     // separate operation, so that in the next steps [ID] is properly anchored
     // in the array.
@@ -565,8 +565,14 @@ export class RegularCell<T> implements Cell<T> {
         [],
         cause,
       );
-      array = isObject(this.schema) && Array.isArray(this.schema?.default)
-        ? this.schema.default
+      const resolvedSchema = resolveSchema(this.schema, this.rootSchema);
+      array = isObject(resolvedSchema) && Array.isArray(resolvedSchema?.default)
+        ? processDefaultValue(
+          this.runtime,
+          this.tx,
+          this.link,
+          resolvedSchema.default,
+        )
         : [];
     }
 
@@ -575,7 +581,7 @@ export class RegularCell<T> implements Cell<T> {
       this.runtime,
       this.tx,
       resolvedLink,
-      [...array, ...valuesToWrite],
+      recursivelyAddIDIfNeeded([...array, ...value]),
       cause,
     );
   }
@@ -877,6 +883,78 @@ function subscribeToReferencedDocs<T>(
   };
 }
 
+/**
+ * Recursively adds IDs elements in arrays, unless they are already a link.
+ *
+ * This ensures that mutable arrays only consist of links to documents, at least
+ * when written to only via .set, .update and .push above.
+ *
+ * TODO(seefeld): When an array has default entries and is rewritten as [...old,
+ * new], this will still break, because the previous entries will point back to
+ * the array itself instead of being new entries.
+ *
+ * @param value - The value to add IDs to.
+ * @returns The value with IDs added.
+ */
+function recursivelyAddIDIfNeeded<T>(
+  value: T,
+  seen: Map<unknown, unknown> = new Map(),
+): T {
+  // Can't add IDs without top frame.
+  if (!getTopFrame()) return value;
+
+  // Not a record, no need to add IDs. Already a link, no need to add IDs.
+  if (!isRecord(value) || isLink(value)) return value;
+
+  // Already seen, return previously annotated result.
+  if (seen.has(value)) return seen.get(value) as T;
+
+  if (Array.isArray(value)) {
+    const result: unknown[] = [];
+
+    // Set before traversing, otherwise we'll infinite recurse.
+    seen.set(value, result);
+
+    result.push(...value.map((v) => {
+      const value = recursivelyAddIDIfNeeded(v, seen);
+      // For objects on arrays only: Add ID if not already present.
+      if (
+        isObject(value) && !isLink(value) && !(ID in value)
+      ) {
+        return { [ID]: getTopFrame()!.generatedIdCounter++, ...value };
+      } else {
+        return value;
+      }
+    }));
+    return result as T;
+  } else {
+    const result: Record<string, unknown> = {};
+
+    // Set before traversing, otherwise we'll infinite recurse.
+    seen.set(value, result);
+
+    Object.entries(value).forEach(([key, v]) => {
+      result[key] = recursivelyAddIDIfNeeded(v, seen);
+    });
+
+    // Copy supported symbols from original value.
+    [ID, ID_FIELD].forEach((symbol) => {
+      if (symbol in value) {
+        (result as IDFields)[symbol as keyof IDFields] =
+          value[symbol as keyof IDFields];
+      }
+    });
+
+    return result as T;
+  }
+}
+
+/**
+ * Converts cells and objects that can be turned to cells to links.
+ *
+ * @param value - The value to convert.
+ * @returns The converted value.
+ */
 export function convertCellsToLinks(
   value: readonly any[] | Record<string, any> | any,
   path: string[] = [],

packages/runner/src/query-result-proxy.ts

@@ -123,6 +123,33 @@ export function createQueryResultProxy<T>(
           return () => createCell(runtime, link, tx, true);
         } else if (prop === toOpaqueRef) {
           return () => makeOpaqueRef(link);
+        } else if (prop === Symbol.iterator && Array.isArray(target)) {
+          return function () {
+            let index = 0;
+            return {
+              next() {
+                const readTx = (tx?.status().status === "ready")
+                  ? tx
+                  : runtime.edit();
+                const length = readTx.readValueOrThrow({
+                  ...link,
+                  path: [...link.path, "length"],
+                }) as number;
+                if (index < length) {
+                  const result = {
+                    value: createQueryResultProxy(runtime, tx, {
+                      ...link,
+                      path: [...link.path, String(index)],
+                    }, depth + 1),
+                    done: false,
+                  };
+                  index++;
+                  return result;
+                }
+                return { done: true };
+              },
+            };
+          };
         }
 
         const readTx = (tx?.status().status === "ready") ? tx : runtime.edit();

packages/runner/src/schema.ts

@@ -102,7 +102,7 @@ export function resolveSchema(
  *
  * For `required` objects and arrays assume {} and [] as default value.
  */
-function processDefaultValue(
+export function processDefaultValue(
   runtime: IRuntime,
   tx: IExtendedStorageTransaction | undefined,
   link: NormalizedFullLink,
@@ -310,8 +310,15 @@ function annotateWithBackToCellSymbols(
     isRecord(value) && !isCell(value) && !isStream(value) &&
     !isQueryResultForDereferencing(value)
   ) {
-    value[toCell] = () => createCell(runtime, link, tx);
-    value[toOpaqueRef] = () => makeOpaqueRef(link);
+    // Non-enumerable, so that {...obj} won't copy these symbols
+    Object.defineProperty(value, toCell, {
+      value: () => createCell(runtime, link, tx),
+      enumerable: false,
+    });
+    Object.defineProperty(value, toOpaqueRef, {
+      value: () => makeOpaqueRef(link),
+      enumerable: false,
+    });
     Object.freeze(value);
   }
   return value;