fix(runner): ensure IDs are added to array elements and default values #1908
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed changes from recent commits (found 1 issue).
1 issue found across 3 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/runner/src/query-result-proxy.ts">
<violation number="1" location="packages/runner/src/query-result-proxy.ts:126">
The new Symbol.iterator handler never reads the array length through the runtime transaction, so iterating an empty array records no dependency; reactive consumers will miss later additions. Please mirror the other array read paths by logging the length before iterating.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| return () => createCell(runtime, link, tx, true); | ||
| } else if (prop === toOpaqueRef) { | ||
| return () => makeOpaqueRef(link); | ||
| } else if (prop === Symbol.iterator && Array.isArray(target)) { |
Contributor
There was a problem hiding this comment.
The new Symbol.iterator handler never reads the array length through the runtime transaction, so iterating an empty array records no dependency; reactive consumers will miss later additions. Please mirror the other array read paths by logging the length before iterating.
Prompt for AI agents
Address the following comment on packages/runner/src/query-result-proxy.ts at line 126:
<comment>The new Symbol.iterator handler never reads the array length through the runtime transaction, so iterating an empty array records no dependency; reactive consumers will miss later additions. Please mirror the other array read paths by logging the length before iterating.</comment>
<file context>
@@ -123,6 +123,26 @@ 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;
</file context>
✅ Addressed in 842f1da
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.
…identally points at the old object
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.
…xies 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.
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)
seefeldb
force-pushed
the
berni/ct-982-fix-defaults-and-create-cells-on-array-writes
branch
from
766ed5f to
42b9ec0
Compare
seefeldb
deleted the
berni/ct-982-fix-defaults-and-create-cells-on-array-writes
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Modified the
setmethod to calladdIDIfNeededon each element when writing an array, ensuring all objects get IDs before being processed bydiffAndUpdate.Enhanced the
pushmethod to:processDefaultValueto ensure they're properly materialized with IDsaddIDIfNeededto all elements (both existing defaults and newly pushed values) to ensure consistent ID assignmentImproved the
updatemethod's schema validation to:resolveSchemato properly handle schema referencesAdded new
addIDIfNeededhelper function that:New tests in cell.test.ts:
"set operations with arrays" suite:
asSchemawithasCell: trueto read back as cells and verify each element has a distinct document ID"push operations with default values" suite:
These changes ensure that array operations consistently create separate documents for each object, maintaining proper referential structure in the storage layer.
Summary by cubic
Ensures array writes and pushes create separate docs for each object by auto-assigning IDs, including for schema defaults. Addresses Linear CT-982 by fixing default materialization and creating cells on array writes.