Conversation
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 24a59da in 14 seconds. Click for details.
- Reviewed
736lines of code in9files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_NMbcbrdfwbsZUCBq
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/stateMachine/query/surql2/buildSurql.ts">
<violation number="1" location="src/stateMachine/query/surql2/buildSurql.ts:96">
P2: FutureRefField with fieldCardinality === 'ONE' still uses `$this.<path>[*]`, which treats single-valued link fields as arrays and changes the query shape. This should be keyed only on fieldCardinality, not on `field.type`.</violation>
<violation number="2" location="src/stateMachine/query/surql2/buildSurql.ts:114">
P2: NestedFutureRefField with fieldCardinality === 'ONE' still uses `$this.<path>[*]`, treating single-valued nested link fields as arrays. Align this condition with fieldCardinality so ONE uses `$this.<path>` regardless of nested_ref vs nested_future_ref.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| lines.push(buildProjection(field.projection, level + 1, mutParams)); | ||
| const filter = field.filter ? buildFilter(field.filter, mutParams) : undefined; | ||
| lines.push(indent(`FROM $this.${esc(field.path)}[*]`, level + 1)); | ||
| if (field.fieldCardinality === 'ONE' && field.type === 'nested_ref') { |
There was a problem hiding this comment.
P2: NestedFutureRefField with fieldCardinality === 'ONE' still uses $this.<path>[*], treating single-valued nested link fields as arrays. Align this condition with fieldCardinality so ONE uses $this.<path> regardless of nested_ref vs nested_future_ref.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/stateMachine/query/surql2/buildSurql.ts, line 114:
<comment>NestedFutureRefField with fieldCardinality === 'ONE' still uses `$this.<path>[*]`, treating single-valued nested link fields as arrays. Align this condition with fieldCardinality so ONE uses `$this.<path>` regardless of nested_ref vs nested_future_ref.</comment>
<file context>
@@ -87,26 +89,33 @@ const buildDataFieldProjection = (field: DataField, level: number) => {
lines.push(buildProjection(field.projection, level + 1, mutParams));
const filter = field.filter ? buildFilter(field.filter, mutParams) : undefined;
- lines.push(indent(`FROM $this.${esc(field.path)}[*]`, level + 1));
+ if (field.fieldCardinality === 'ONE' && field.type === 'nested_ref') {
+ lines.push(indent(`FROM $this.${esc(field.path)}`, level + 1));
+ } else {
</file context>
| if (field.fieldCardinality === 'ONE' && field.type === 'nested_ref') { | |
| if (field.fieldCardinality === 'ONE') { |
| const subQuery = field.fieldCardinality === 'ONE' && field.type === 'ref' | ||
| ? `SELECT VALUE record::id(id) FROM $this.${escapedPath}` | ||
| : `SELECT VALUE record::id(id) FROM $this.${escapedPath}[*]`; |
There was a problem hiding this comment.
P2: FutureRefField with fieldCardinality === 'ONE' still uses $this.<path>[*], which treats single-valued link fields as arrays and changes the query shape. This should be keyed only on fieldCardinality, not on field.type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/stateMachine/query/surql2/buildSurql.ts, line 96:
<comment>FutureRefField with fieldCardinality === 'ONE' still uses `$this.<path>[*]`, which treats single-valued link fields as arrays and changes the query shape. This should be keyed only on fieldCardinality, not on `field.type`.</comment>
<file context>
@@ -87,26 +89,33 @@ const buildDataFieldProjection = (field: DataField, level: number) => {
const escapedAlias = esc(alias || path);
- if (cardinality === 'ONE') {
- return indent(`array::first(SELECT VALUE record::id(id) FROM $this.${escapedPath}[*]) AS ${escapedAlias}`, level);
+ const subQuery = field.fieldCardinality === 'ONE' && field.type === 'ref'
+ ? `SELECT VALUE record::id(id) FROM $this.${escapedPath}`
+ : `SELECT VALUE record::id(id) FROM $this.${escapedPath}[*]`;
</file context>
| const subQuery = field.fieldCardinality === 'ONE' && field.type === 'ref' | |
| ? `SELECT VALUE record::id(id) FROM $this.${escapedPath}` | |
| : `SELECT VALUE record::id(id) FROM $this.${escapedPath}[*]`; | |
| const subQuery = field.fieldCardinality === 'ONE' | |
| ? `SELECT VALUE record::id(id) FROM $this.${escapedPath}` | |
| : `SELECT VALUE record::id(id) FROM $this.${escapedPath}[*]`; |
Important
Optimize SurQL builder by refining projections, filters, and logging, and update tests accordingly.
batchedMutationand errors inrunSURQLMutationinrun.ts.batchedQueryinquery.ts.buildProjectioninbuildLogical.tsto handleconstantandcomputedfields by skipping them.buildSimpleFieldProjectionto returnundefinedforconstantandcomputedfields.resultCardinalityandfieldCardinalitytoProjectionFieldtypes inlogical.ts.cardinalityandoppositeCardinalityto filters inbuildLogical.tsandoptimize.ts.future_bi_refandbi_reffilters inoptimizeSourceinoptimize.ts.processResults.tsto convertDateTimetoDateintransformResultObject.basic.tsanderrors.tsto match new error messages.This description was created by
for 24a59da. You can customize this summary. It will automatically update as commits are pushed.
Summary by cubic
Optimized the SURQL query builder by introducing precise ref types and cardinality handling to generate faster, more accurate queries. Added helpful logging and result normalization.
Performance
Bug Fixes
Written for commit 24a59da. Summary will update on new commits.