feat(db): unionAll operator#1537
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements conditional ChangescaseWhen and unionAll Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
samwillis
force-pushed
the
multi-source-from
branch
from
a79e5eb to
7fa5b53
Compare
More templates
@tanstack/angular-db
@tanstack/browser-db-sqlite-persistence
@tanstack/capacitor-db-sqlite-persistence
@tanstack/cloudflare-durable-objects-db-sqlite-persistence
@tanstack/db
@tanstack/db-ivm
@tanstack/db-sqlite-persistence-core
@tanstack/electric-db-collection
@tanstack/electron-db-sqlite-persistence
@tanstack/expo-db-sqlite-persistence
@tanstack/node-db-sqlite-persistence
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/react-native-db-sqlite-persistence
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Contributor
|
Size Change: +4.34 kB (+3.7%) Total Size: 122 kB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 4.24 kB ℹ️ View Unchanged
|
Contributor
|
@samwillis I find the multi from confusing. There is no SQL equivalent, the closest you can get in SQL is to const feed = createLiveQueryCollection((q) =>
q
.from({
message: messages,
toolCall: toolCalls,
})
.orderBy(({ message, toolCall }) =>
coalesce(message.timestamp, toolCall.timestamp),
),
)I find this query confusing because i initially thought that this is just another notation for q
.from({
message: messages,
})
.orderBy(({ message }) =>
message.timestamp
)
UNION
q
.from({
toolCall: toolCalls,
})
.orderBy(({ toolCall }) =>
toolCall.timestamp
)But it is not equivalent to this because SELECT * FROM (
SELECT m.* AS message, NULL AS toolCall FROM messages m
UNION ALL
SELECT NULL AS message, t.* AS toolCall FROM toolCalls t
) sub
ORDER BY coalesce(sub.message.timestamp, sub.toolCall.timestamp)I find this very confusing. You can't tell that from reading With explicit q.unionAll(
q.from({ message: messages }),
q.from({ toolCall: toolCalls }),
).orderBy(({ message, toolCall }) =>
coalesce(message.timestamp, toolCall.timestamp),
) // obviously post-unionCompare this to the current form where you have to infer scope from the shape of the |
samwillis
force-pushed
the
multi-source-from
branch
from
6bb8cc8 to
084a4a5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (10)
docs/guides/live-queries.md (1)
484-540: ⚡ Quick winConsider adding guidance on when to use
unionAll()vsfrom().The "From Clause" section describes single-source
from()usage, and the following "Source-LevelunionAll" section describes combining multiple sources. However, there's no explicit guidance on when developers should choose one over the other, or whether multi-sourcefrom()is still supported.Given that the UNION_ALL_REFACTOR_PLAN.md indicates multi-source
from()will be deprecated, consider adding a note like:> **Note:** Use `from()` with a single source. To combine multiple independent sources, use `unionAll()` instead (see [Source-Level unionAll](`#source-level-unionall`) below).This helps developers understand the intended usage pattern and prevents confusion about the API design.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/live-queries.md` around lines 484 - 540, Add a short advisory note in the "From Clause" section clarifying that from() should be used with a single source and that combining multiple independent sources should be done with unionAll() (reference the Source-Level unionAll section), and mention that multi-source from() will be deprecated per the UNION_ALL_REFACTOR_PLAN.md; update the text near the Method Signature or Basic Usage (where from({ [alias]: Collection | Query }) and examples appear) to include this guidance and a link/reference to the Source-Level unionAll section.packages/db/src/errors.ts (1)
389-402: ⚡ Quick winConsider using a union type for the
contextparameter.The
contextparameter is checked using a string literal (context === "unionAll clause"). Using a typed union would provide compile-time safety and prevent typos:constructor(context: "from clause" | "unionAll clause" | "join clause", type: string)This ensures callers pass valid context strings and makes the valid values discoverable through IDE autocomplete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/errors.ts` around lines 389 - 402, The constructor in errors.ts currently accepts an untyped string for context and relies on string-equality checks (e.g., context === "unionAll clause"); change the constructor signature to use a string literal union for allowed contexts (e.g., "from clause" | "unionAll clause" | "join clause") so the compiler enforces valid values, update any internal checks that compare context to those literals to remain the same, and update all call sites of this constructor to pass one of the unioned literal values so callers get IDE autocomplete and type safety (identify the constructor in this file to make the signature change and scan for usages of this error class to update callers).UNION_ALL_REFACTOR_PLAN.md (1)
1-235: 💤 Low valueConsider moving this planning document to a more appropriate location.
This refactor plan is currently in the repository root. Planning and design documents are typically better organized in dedicated directories. Consider moving it to:
docs/design/UNION_ALL_REFACTOR_PLAN.md(if you have design docs).github/docs/UNION_ALL_REFACTOR_PLAN.md(for GitHub-specific planning)- Or a dedicated
planning/directoryThis keeps the repo root cleaner and makes it easier to find design documentation alongside other architectural decisions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@UNION_ALL_REFACTOR_PLAN.md` around lines 1 - 235, Move the planning doc "UNION_ALL_REFACTOR_PLAN.md" out of the repo root into a documentation folder (suggested targets: docs/design/UNION_ALL_REFACTOR_PLAN.md, .github/docs/UNION_ALL_REFACTOR_PLAN.md, or planning/UNION_ALL_REFACTOR_PLAN.md), update any references to the file (README, docs index, or issue/PR descriptions) to the new path, and ensure the top-level header "# `unionAll` Query Builder Refactor Plan" remains unchanged so searches still find it; create or update a docs/index or TOC entry if you have one so the plan is discoverable after the move.packages/db/tests/query/case-when.test.ts (3)
79-769: ⚡ Quick winAdd explicit empty/single-element case coverage for
caseWhenqueries.This suite is comprehensive, but it’s missing direct edge-case assertions for empty collections and single-element inputs on conditional projection/include paths. A couple of targeted tests here would harden regressions around null/default materialization and grouped behavior.
As per coding guidelines,
Test corner cases including: empty collections, single elements, undefined vs null, resolved promises, race conditions, limit/offset edge cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/case-when.test.ts` around lines 79 - 769, Add targeted tests covering empty and single-element inputs for caseWhen paths: add at least two tests using createCollection/createLiveQueryCollection + preload that assert behavior when the source collection is empty and when it contains a single item (verify conditional scalar branches, projection objects, includes/toArray branches and null vs undefined handling). Reference the existing patterns in this file (functions/symbols: caseWhen, createLiveQueryCollection, createCollection, createUsersCollection/createPostsCollection, preload, toArray, orderBy, stripVirtualPropsAndSymbols) and follow the existing test structure and expectations to assert correct materialization for empty results (e.g. null/default branches, empty arrays for includes) and single-element results. Ensure tests cover both scalar and object branches, and grouped/aggregated usages where applicable.
59-77: ⚡ Quick winReplace
anyin helper utilities withunknown+ narrowing.Line 59/65/75/76 currently erase type checks with
any. Please switch tounknownand narrow locally to keep helper behavior type-safe.Proposed patch
-function stripVirtualPropsAndSymbols(value: any): any { +function stripVirtualPropsAndSymbols(value: unknown): unknown { if (Array.isArray(value)) { return value.map((entry) => stripVirtualPropsAndSymbols(entry)) } if (value && typeof value === `object`) { - const out: Record<string, any> = {} + const out: Record<string, unknown> = {} for (const [key, entry] of Object.entries(stripVirtualProps(value))) { out[key] = stripVirtualPropsAndSymbols(entry) } return out } return value } -function childRows(collection: any): Array<any> { - return [...collection.toArray].map((row) => stripVirtualPropsAndSymbols(row)) +function childRows(collection: { toArray: ReadonlyArray<unknown> }): Array<unknown> { + return collection.toArray.map((row) => stripVirtualPropsAndSymbols(row)) }As per coding guidelines,
Avoid using any types; use unknown instead when type is truly unknown and provide type guards to narrow safely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/case-when.test.ts` around lines 59 - 77, Replace the use of `any` in the helpers by declaring parameters and intermediate vars as `unknown` and adding local narrowing guards: in stripVirtualPropsAndSymbols change the parameter type to `unknown`, use Array.isArray to narrow arrays, check `value !== null && typeof value === 'object'` and index into Object.entries only after that check (and preserve use of stripVirtualProps by narrowing its input), and in childRows type the `collection` parameter as `unknown` then assert/guard that it has a `toArray` iterable (e.g., check `collection && typeof (collection as any).toArray !== 'undefined'`) before spreading and mapping; update all local variables similarly so no flow uses `any` without narrowing. Ensure references to the functions stripVirtualPropsAndSymbols and childRows remain unchanged.
39-57: ⚡ Quick winAnnotate return types for collection factory helpers.
createUsersCollectionandcreatePostsCollectionshould declare explicit return types to avoid inference drift in tests that depend on these helpers.As per coding guidelines,
Provide proper type annotations for function return values instead of relying on implicit types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/case-when.test.ts` around lines 39 - 57, The two test helper functions createUsersCollection and createPostsCollection lack explicit return type annotations which can cause inference drift; add an explicit return type to each (for example ": ReturnType<typeof createCollection>" or the concrete collection type used in your codebase such as "SyncCollection<User>" / "SyncCollection<Post>") to their declarations, e.g. function createUsersCollection(): ReturnType<typeof createCollection> { ... } and function createPostsCollection(): ReturnType<typeof createCollection> { ... }, and add any necessary imports for the concrete type if you choose not to use ReturnType.packages/db/tests/query/case-when.test-d.ts (1)
31-49: ⚡ Quick winAdd explicit return types to test helper factories.
createUsersandcreatePostscurrently rely on inference. Please annotate return types so helper contracts remain stable ifcreateCollectioninference changes.As per coding guidelines,
Provide proper type annotations for function return values instead of relying on implicit types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/case-when.test-d.ts` around lines 31 - 49, The test helper functions createUsers and createPosts lack explicit return type annotations; update their signatures to declare the concrete return type returned by createCollection (e.g., the collection type produced when calling createCollection with mockSyncCollectionOptions<User> / mockSyncCollectionOptions<Post>) so the helpers' contracts do not depend on inference from createCollection or mockSyncCollectionOptions. Locate the createUsers and createPosts functions and add the appropriate return type annotations matching the collection type they produce.packages/db/tests/query/builder/union-all.test.ts (1)
46-47: ⚡ Quick winAvoid
as anyin IR assertions; narrow by discriminant instead.These casts suppress the exact type guarantees this builder test should enforce.
♻️ Proposed assertion tightening
- expect( - (builtQuery.from as any).sources.map((source: any) => source.alias), - ).toEqual([`employees`, `departments`]) + if (builtQuery.from.type !== `unionFrom`) { + throw new Error(`Expected unionFrom`) + } + expect(builtQuery.from.sources.map((source) => source.alias)).toEqual([ + `employees`, + `departments`, + ]) @@ - expect((builtQuery.from as any).queries).toHaveLength(2) + if (builtQuery.from.type !== `unionAll`) { + throw new Error(`Expected unionAll`) + } + expect(builtQuery.from.queries).toHaveLength(2)As per coding guidelines,
**/*.{ts,tsx}: "Avoid usinganytypes; useunknowninstead when type is truly unknown and provide type guards to narrow safely".Also applies to: 81-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/builder/union-all.test.ts` around lines 46 - 47, The test currently uses (builtQuery.from as any).sources to bypass type checking; instead, narrow the union by checking the discriminant on each source before accessing alias. Update the assertion to iterate builtQuery.from.sources, use a type guard or an inline check like if (source.type === 'table' || source.kind === 'table') to narrow the type, then collect source.alias values and expect them toEqual([`employees`,`departments`]); apply the same pattern for the other occurrence around line 81 to remove `as any` casts and rely on explicit discriminant narrowing.packages/db/tests/query/union-all.test.ts (1)
182-198: ⚡ Quick winReplace
anyin test helpers/callbacks withunknown+ narrowing.These
anyannotations weaken the exact safety guarantees this union-heavy suite is meant to protect.♻️ Proposed typed helper refactor
-function stripVirtualPropsDeep(value: any): any { +function isRecord(value: unknown): value is Record<string, unknown> { + return value !== null && typeof value === `object` +} + +function stripVirtualPropsDeep(value: unknown): unknown { if (Array.isArray(value)) { return value.map((entry) => stripVirtualPropsDeep(entry)) } - if (value && typeof value === `object`) { - const out: Record<string, any> = {} + if (isRecord(value)) { + const out: Record<string, unknown> = {} for (const [key, entry] of Object.entries(stripVirtualProps(value))) { out[key] = stripVirtualPropsDeep(entry) } return out } return value } -function childRows(collection: any): Array<any> { +function childRows<T>(collection: { toArray: Array<T> }): Array<unknown> { return [...collection.toArray].map((row) => stripVirtualPropsDeep(row)) }As per coding guidelines,
**/*.{ts,tsx}: "Avoid usinganytypes; useunknowninstead when type is truly unknown and provide type guards to narrow safely".Also applies to: 595-597, 619-621, 713-715, 1148-1158, 1431-1450
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/union-all.test.ts` around lines 182 - 198, Replace the loose any types in the test helpers by using unknown and explicit narrowing: change function signatures stripVirtualPropsDeep(value: any) -> stripVirtualPropsDeep(value: unknown) and childRows(collection: unknown) -> childRows(collection: { toArray: unknown } | unknown), then implement safe type-guards (Array.isArray, typeof value === "object" && value !== null) and use Record<string, unknown> for object outputs; call stripVirtualProps(value) expecting unknown and narrow its result to Record<string, unknown> before iterating entries, recursively call stripVirtualPropsDeep with narrowed types, and in childRows assert collection has a toArray that is an iterable and map its elements after narrowing—apply the same unknown-to-narrowing pattern to the other helpers mentioned (lines ~595-597, 619-621, 713-715, 1148-1158, 1431-1450).packages/db/src/query/compiler/evaluators.ts (1)
577-595: ⚡ Quick winUse
unknownfor the new exported predicate helper.
anyhere disables checking for every caller of this helper.unknownkeeps the implementation the same while preserving safe narrowing at the call sites.♻️ Proposed fix
-export function isCaseWhenConditionTrue(value: any): boolean { +export function isCaseWhenConditionTrue(value: unknown): boolean { if (value == null || value === false) { return false }As per coding guidelines, "Avoid using
anytypes; useunknowninstead when type is truly unknown and provide type guards to narrow safely".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/query/compiler/evaluators.ts` around lines 577 - 595, Change the exported predicate helper isCaseWhenConditionTrue to accept value: unknown instead of any to preserve caller type-safety; update the function signature to isCaseWhenConditionTrue(value: unknown): boolean and keep the existing runtime checks (null/false, true, typeof number/bigint, Boolean(value)) so behavior is unchanged while callers retain safe narrowing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/src/query/builder/functions.ts`:
- Around line 570-596: The caseWhen overload that accepts a 6th condition/value
pair (the function signature beginning with "export function caseWhen< C1...V5
>( ... condition6: ExpressionLike, value6: CaseWhenValue, ...rest:
Array<CaseWhenValue> ): any") currently returns any and loses type information;
update the overloads so they return a properly typed CaseWhenResult (e.g.,
CaseWhenResult<C1,V1,C2,V2,C3,V3,C4,V4,C5,V5,C6,V6> or the appropriate generic
for the six-pair form), and add the matching overload for the six-pair plus
default value variant (or replace with a variadic-tuple generic that preserves
exact types for all arities). Ensure you reference the existing types
ExpressionLike, CaseWhenValue and CaseWhenResult and keep the runtime
implementation signature unchanged (only adjust the TypeScript overload
signatures) so downstream query types retain full type inference.
- Around line 71-81: The conditional in the CaseWhenResult type is distributing
over union members causing mixed branch unions to be typed as a union of
BasicExpression and CaseWhenWrapper; change the distributive check to a
non-distributive form so the type only yields BasicExpression when all branch
values are ExpressionLike. Concretely, replace "TValues[number] extends
ExpressionLike" with "[TValues[number]] extends [ExpressionLike]" (and keep the
THasDefault logic intact) so CaseWhenResult correctly resolves to
BasicExpression only when every branch is expression-like and to CaseWhenWrapper
otherwise; update the type use-sites (e.g., where CaseWhenResult is returned
from caseWhen) to rely on this corrected behavior.
In `@packages/db/src/query/builder/index.ts`:
- Around line 1126-1129: The fallback that builds childAliases only handles
childQuery.from.type === `unionFrom` and uses sources, but misses the `unionAll`
case so branch aliases (e.g., branchA/branchB) are not included and
extractCorrelation never matches those refs; update the logic around
childAliases to also detect `childQuery.from.type === 'unionAll'` and collect
all branch aliases (e.g., map over childQuery.from.branches or equivalent branch
list to extract each branch.alias) so extractCorrelation will see aliases from
unionAll branches as well.
In `@packages/db/src/query/builder/types.ts`:
- Around line 590-595: The ValueOfUnion helper currently returns never when a
union branch lacks key K which makes branch-exclusive fields appear required;
update ValueOfUnion (the conditional type) so that when K is not a key of a
branch it yields undefined instead of never, preserving undefined for missing
keys and making fields like toolCall optional in unions used by RefsForContext;
locate the ValueOfUnion type in types.ts and change the branch that now produces
never to produce undefined while keeping the existing conditional structure.
In `@packages/db/src/query/compiler/group-by.ts`:
- Around line 919-939: compileGroupedConditionalSelect currently returns
undefined when no branch matches and no defaultValue is provided; change it to
return null to match non-grouped behavior. Locate
compileGroupedConditionalSelect and adjust the final return so that if
defaultValue is undefined it returns null, otherwise it invokes and returns
defaultValue(row); keep branch evaluation logic
(branches.map/isCaseWhenConditionTrue) unchanged. Ensure you reference the
compiled defaultValue (the result of compileGroupedSelectValue) rather than the
original conditional.defaultValue when deciding to return null.
In `@packages/db/src/query/compiler/index.ts`:
- Around line 440-446: The current clone only shallow-copies query.select but
replaceIncludesInSelect() mutates nested branches (including ConditionalSelect),
which modifies the shared IR; change the flow so replacements are done
immutably: update or replace replaceIncludesInSelect to return a new transformed
Select tree (or perform path-copying on any node it touches) instead of mutating
nodes in place, and invoke it to assign the returned select to query.select;
also apply the same immutable transformation approach to the other occurrence
referenced around the 1878-1933 region to ensure no nested objects are mutated.
- Around line 244-245: The union-backed include path is skipping the
parentKeyStream when isUnionFrom is true so processFromClause/processUnionAll
never receive the correlation stream and children never get __correlationKey;
update the logic around parentKeyStream/isUnionFrom (where mainInput is selected
from sources[mainSource]) to always propagate the parentKeyStream (and attach
childCorrelationField) into the per-branch processing: pass the correlation
stream into processFromClause and processUnionAll for each union branch, ensure
__correlationKey is set on union branch inputs (not just non-union paths), and
update any callers that build the union input so that union-backed includes
compile/load the filtered, correlated subset and can be routed back to parent
rows.
In `@packages/db/src/query/compiler/lazy-targets.ts`:
- Around line 122-166: getSourceFromAlias currently only looks at the top-level
from and misses aliases defined on query joins or inside queryRef/union
branches, causing getTargetsFromPropRef to miss valid join-backed aliases;
update getSourceFromAlias (and usages in
getTargetsFromPropRef/getTargetsFromQueryRef) to collect and search aliases
from: the top-level from sources, any nested QueryRef sources' query.from and
query.join arrays (and for unionFrom/unionAll iterate each branch's from and its
query joins), so that when a matching alias exists in a join or inside a
queryRef branch you return the correct CollectionRef or QueryRef instead of
undefined. Ensure you still respect existing branch logic (e.g., unionAll -> [])
and preserve source.type checks like collectionRef vs queryRef.
In `@packages/db/src/query/live/utils.ts`:
- Around line 118-121: extractCollectionFromSource currently returns the first
branch for unionFrom/unionAll which lets CollectionConfigBuilder implicitly
inherit defaultStringCollation from an arbitrary branch; instead, update
extractCollectionFromSource (or CollectionConfigBuilder's use of it) to either
validate that all branches in unionFrom/unionAll share the same
compareOptions/defaultStringCollation and throw if they differ, or require
callers to pass an explicit defaultStringCollation for union queries;
specifically, when encountering from.type === 'unionFrom' or 'unionAll', iterate
all sources/queries and compare their compareOptions/defaultStringCollation
values, and if any mismatch is detected raise an error (or clear the inherited
collation and force an explicit parameter) so collation is not silently taken
from the first branch.
In `@packages/db/src/query/optimizer.ts`:
- Around line 908-909: getFirstFromAlias currently dereferences
getFromSources(...)[0] which throws for unionAll (no sources); update
getFirstFromAlias to guard for an empty array (or query.kind === 'unionAll') and
return a safe sentinel (e.g. empty string) instead of indexing into [0]; also
apply the same guard to the similar code paths around the
referencesAliasWithRemappedSelect call (the occurrence at lines ~1194-1197) so
callers receive a safe value and the optimizer will conservatively avoid
pushdown instead of crashing.
In `@packages/db/tests/query/case-when.test-d.ts`:
- Around line 127-324: Tests and types disagree on whether non-matching caseWhen
branches are represented as undefined or null; fix by making the representation
consistent. Update the caseWhen implementation and its type signature (the
function named caseWhen) to return null for non-matching branches (or, if you
prefer the opposite, update the runtime to return undefined) and then align all
type tests (references: adultProfile, postProfile, profile, maybeProfile, and
any caseWhen usages in these tests) to expect that same nullable value (replace
| undefined with | null across the failing expectTypeOf assertions) so runtime,
types, and docs all match.
---
Nitpick comments:
In `@docs/guides/live-queries.md`:
- Around line 484-540: Add a short advisory note in the "From Clause" section
clarifying that from() should be used with a single source and that combining
multiple independent sources should be done with unionAll() (reference the
Source-Level unionAll section), and mention that multi-source from() will be
deprecated per the UNION_ALL_REFACTOR_PLAN.md; update the text near the Method
Signature or Basic Usage (where from({ [alias]: Collection | Query }) and
examples appear) to include this guidance and a link/reference to the
Source-Level unionAll section.
In `@packages/db/src/errors.ts`:
- Around line 389-402: The constructor in errors.ts currently accepts an untyped
string for context and relies on string-equality checks (e.g., context ===
"unionAll clause"); change the constructor signature to use a string literal
union for allowed contexts (e.g., "from clause" | "unionAll clause" | "join
clause") so the compiler enforces valid values, update any internal checks that
compare context to those literals to remain the same, and update all call sites
of this constructor to pass one of the unioned literal values so callers get IDE
autocomplete and type safety (identify the constructor in this file to make the
signature change and scan for usages of this error class to update callers).
In `@packages/db/src/query/compiler/evaluators.ts`:
- Around line 577-595: Change the exported predicate helper
isCaseWhenConditionTrue to accept value: unknown instead of any to preserve
caller type-safety; update the function signature to
isCaseWhenConditionTrue(value: unknown): boolean and keep the existing runtime
checks (null/false, true, typeof number/bigint, Boolean(value)) so behavior is
unchanged while callers retain safe narrowing.
In `@packages/db/tests/query/builder/union-all.test.ts`:
- Around line 46-47: The test currently uses (builtQuery.from as any).sources to
bypass type checking; instead, narrow the union by checking the discriminant on
each source before accessing alias. Update the assertion to iterate
builtQuery.from.sources, use a type guard or an inline check like if
(source.type === 'table' || source.kind === 'table') to narrow the type, then
collect source.alias values and expect them
toEqual([`employees`,`departments`]); apply the same pattern for the other
occurrence around line 81 to remove `as any` casts and rely on explicit
discriminant narrowing.
In `@packages/db/tests/query/case-when.test-d.ts`:
- Around line 31-49: The test helper functions createUsers and createPosts lack
explicit return type annotations; update their signatures to declare the
concrete return type returned by createCollection (e.g., the collection type
produced when calling createCollection with mockSyncCollectionOptions<User> /
mockSyncCollectionOptions<Post>) so the helpers' contracts do not depend on
inference from createCollection or mockSyncCollectionOptions. Locate the
createUsers and createPosts functions and add the appropriate return type
annotations matching the collection type they produce.
In `@packages/db/tests/query/case-when.test.ts`:
- Around line 79-769: Add targeted tests covering empty and single-element
inputs for caseWhen paths: add at least two tests using
createCollection/createLiveQueryCollection + preload that assert behavior when
the source collection is empty and when it contains a single item (verify
conditional scalar branches, projection objects, includes/toArray branches and
null vs undefined handling). Reference the existing patterns in this file
(functions/symbols: caseWhen, createLiveQueryCollection, createCollection,
createUsersCollection/createPostsCollection, preload, toArray, orderBy,
stripVirtualPropsAndSymbols) and follow the existing test structure and
expectations to assert correct materialization for empty results (e.g.
null/default branches, empty arrays for includes) and single-element results.
Ensure tests cover both scalar and object branches, and grouped/aggregated
usages where applicable.
- Around line 59-77: Replace the use of `any` in the helpers by declaring
parameters and intermediate vars as `unknown` and adding local narrowing guards:
in stripVirtualPropsAndSymbols change the parameter type to `unknown`, use
Array.isArray to narrow arrays, check `value !== null && typeof value ===
'object'` and index into Object.entries only after that check (and preserve use
of stripVirtualProps by narrowing its input), and in childRows type the
`collection` parameter as `unknown` then assert/guard that it has a `toArray`
iterable (e.g., check `collection && typeof (collection as any).toArray !==
'undefined'`) before spreading and mapping; update all local variables similarly
so no flow uses `any` without narrowing. Ensure references to the functions
stripVirtualPropsAndSymbols and childRows remain unchanged.
- Around line 39-57: The two test helper functions createUsersCollection and
createPostsCollection lack explicit return type annotations which can cause
inference drift; add an explicit return type to each (for example ":
ReturnType<typeof createCollection>" or the concrete collection type used in
your codebase such as "SyncCollection<User>" / "SyncCollection<Post>") to their
declarations, e.g. function createUsersCollection(): ReturnType<typeof
createCollection> { ... } and function createPostsCollection():
ReturnType<typeof createCollection> { ... }, and add any necessary imports for
the concrete type if you choose not to use ReturnType.
In `@packages/db/tests/query/union-all.test.ts`:
- Around line 182-198: Replace the loose any types in the test helpers by using
unknown and explicit narrowing: change function signatures
stripVirtualPropsDeep(value: any) -> stripVirtualPropsDeep(value: unknown) and
childRows(collection: unknown) -> childRows(collection: { toArray: unknown } |
unknown), then implement safe type-guards (Array.isArray, typeof value ===
"object" && value !== null) and use Record<string, unknown> for object outputs;
call stripVirtualProps(value) expecting unknown and narrow its result to
Record<string, unknown> before iterating entries, recursively call
stripVirtualPropsDeep with narrowed types, and in childRows assert collection
has a toArray that is an iterable and map its elements after narrowing—apply the
same unknown-to-narrowing pattern to the other helpers mentioned (lines
~595-597, 619-621, 713-715, 1148-1158, 1431-1450).
In `@UNION_ALL_REFACTOR_PLAN.md`:
- Around line 1-235: Move the planning doc "UNION_ALL_REFACTOR_PLAN.md" out of
the repo root into a documentation folder (suggested targets:
docs/design/UNION_ALL_REFACTOR_PLAN.md, .github/docs/UNION_ALL_REFACTOR_PLAN.md,
or planning/UNION_ALL_REFACTOR_PLAN.md), update any references to the file
(README, docs index, or issue/PR descriptions) to the new path, and ensure the
top-level header "# `unionAll` Query Builder Refactor Plan" remains unchanged so
searches still find it; create or update a docs/index or TOC entry if you have
one so the plan is discoverable after the move.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8c3e0df-e8de-477a-9339-ee31fb42af1c
📒 Files selected for processing (31)
.changeset/tender-mugs-hear.mdUNION_ALL_REFACTOR_PLAN.mddocs/guides/live-queries.mdpackages/db/src/errors.tspackages/db/src/query/builder/functions.tspackages/db/src/query/builder/index.tspackages/db/src/query/builder/ref-proxy.tspackages/db/src/query/builder/types.tspackages/db/src/query/compiler/evaluators.tspackages/db/src/query/compiler/group-by.tspackages/db/src/query/compiler/index.tspackages/db/src/query/compiler/joins.tspackages/db/src/query/compiler/lazy-targets.tspackages/db/src/query/compiler/order-by.tspackages/db/src/query/compiler/select.tspackages/db/src/query/effect.tspackages/db/src/query/index.tspackages/db/src/query/ir.tspackages/db/src/query/live/collection-config-builder.tspackages/db/src/query/live/collection-subscriber.tspackages/db/src/query/live/utils.tspackages/db/src/query/optimizer.tspackages/db/tests/query/builder/from.test.tspackages/db/tests/query/builder/union-all.test.tspackages/db/tests/query/case-when.test-d.tspackages/db/tests/query/case-when.test.tspackages/db/tests/query/compiler/subqueries.test.tspackages/db/tests/query/group-by.test.tspackages/db/tests/query/includes.test.tspackages/db/tests/query/union-all.test-d.tspackages/db/tests/query/union-all.test.ts
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
Add union-style from handling across the query builder, compiler, optimizer, live traversal, and tests so independent sources can feed one live collection. Co-authored-by: Cursor <[email protected]>
Tighten union join typing, avoid inactive guarded include evaluation, and document multi-source ordering defaults. Co-authored-by: Cursor <[email protected]>
Add runtime and type coverage for union branches that are joined and projected subqueries, since that is the main pre-filtering pattern. Co-authored-by: Cursor <[email protected]>
Resolve multi-source subquery join targets per branch so safe coalesce projections can load indexed subsets while computed or limited branches deopt to eager loading. Co-authored-by: Cursor <[email protected]>
Share union-aware lazy target resolution between joins and includes so correlated child collections can be built from subquery or multi-source sources without crashing. Co-authored-by: Cursor <[email protected]>
Carry nested include routing through QueryRef sources and preserve keyed incremental updates so child live queries receive streaming materialization changes. Co-authored-by: Cursor <[email protected]>
Tighten branch key encoding, join pushdown nullability, lazy target resolution, and conditional/grouped projection semantics based on review feedback. Co-authored-by: Cursor <[email protected]>
Restore nested update cloning after rebasing onto the latest caseWhen branch and update conditional include expectations. Co-authored-by: Cursor <[email protected]>
Assert subquery-backed join lazy loading marks the concrete subscribed alias rather than the outer QueryRef alias. Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
samwillis
force-pushed
the
multi-source-from
branch
from
084a4a5 to
c09359e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/db/src/query/builder/index.ts (1)
1121-1129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
unionAllchild aliases when extracting include correlations.This code does not handle the
unionAllcase. WhenchildQuery.from.type === 'unionAll', the fallback useschildQuery.from.aliaswhich returns''(empty string) forUnionAll. This causesextractCorrelation()to fail matching refs, resulting in false "must have a WHERE eq()" errors for correlated includes withunionAllchildren.🐛 Proposed fix to handle unionAll branches
// Collect child's own aliases const childAliases: Array<string> = childQuery.from.type === `unionFrom` ? childQuery.from.sources.map((source) => source.alias) + : childQuery.from.type === `unionAll` + ? childQuery.from.queries.flatMap((branch) => + branch.from.type === `unionFrom` + ? branch.from.sources.map((s) => s.alias) + : branch.from.type === `unionAll` + ? [] // Nested unionAll not expected; skip or recurse + : [branch.from.alias] + ) : [childQuery.from.alias]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/query/builder/index.ts` around lines 1121 - 1129, The current child alias extraction in the include-correlation path misses the unionAll case: when childQuery.from.type === 'unionAll' childQuery.from.alias is empty and causes extractCorrelation() to fail; update the logic that builds childAliases (the childAliases const in the block handling childQuery.from.type === 'unionFrom') to also handle 'unionAll' by collecting aliases from childQuery.from.sources (like the unionFrom branch) and include those aliases plus any join.from.alias entries before calling extractCorrelation(); ensure you reference childQuery.from.type, childQuery.from.sources, childQuery.from.alias, childQuery.join and the childAliases array so both unionFrom and unionAll produce the correct list of aliases.
🧹 Nitpick comments (6)
packages/db/tests/query/union-all.test-d.ts (1)
36-64: ⚡ Quick winExtract a shared typed collection factory for test fixtures.
The three helper functions repeat the same construction pattern. Consolidating this into one generic helper will keep fixture setup consistent and reduce drift.
Proposed change
+function createTypedCollection<TRow extends { id: number }>(id: string) { + return createCollection( + mockSyncCollectionOptions<TRow>({ + id, + getKey: (row) => row.id, + initialData: [], + }), + ) +} + function createMessages() { - return createCollection( - mockSyncCollectionOptions<Message>({ - id: `multi-source-type-messages`, - getKey: (message) => message.id, - initialData: [], - }), - ) + return createTypedCollection<Message>(`multi-source-type-messages`) } @@ function createToolCalls() { - return createCollection( - mockSyncCollectionOptions<ToolCall>({ - id: `multi-source-type-tools`, - getKey: (toolCall) => toolCall.id, - initialData: [], - }), - ) + return createTypedCollection<ToolCall>(`multi-source-type-tools`) } @@ function createUsers() { - return createCollection( - mockSyncCollectionOptions<User>({ - id: `multi-source-type-users`, - getKey: (user) => user.id, - initialData: [], - }), - ) + return createTypedCollection<User>(`multi-source-type-users`) }As per coding guidelines, "Extract common logic into reusable utility functions when duplicated across multiple places".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/union-all.test-d.ts` around lines 36 - 64, The three fixtures createMessages, createToolCalls, and createUsers duplicate the same pattern; replace them with a single generic factory (e.g., createTypedCollection<T>) that calls createCollection with mockSyncCollectionOptions<T> and accepts an id and getKey (or infers getKey as (item) => item.id) to produce collections for Message, ToolCall, and User; update tests to call createTypedCollection<Message>(`multi-source-type-messages`), createTypedCollection<ToolCall>(`multi-source-type-tools`), and createTypedCollection<User>(`multi-source-type-users`) and remove the three specific functions (references: createMessages, createToolCalls, createUsers, createCollection, mockSyncCollectionOptions).packages/db/tests/query/builder/union-all.test.ts (1)
45-47: ⚡ Quick winRemove
as anyfrom IR assertions and narrow by discriminant.These casts bypass the exact union typing you’re validating here. Narrow on
from.typebefore accessing branch-specific fields.Proposed change
expect(builtQuery.from).toBeDefined() expect(builtQuery.from.type).toBe(`unionFrom`) expect(builtQuery.from.alias).toBe(`employees`) - expect( - (builtQuery.from as any).sources.map((source: any) => source.alias), - ).toEqual([`employees`, `departments`]) + if (builtQuery.from.type !== `unionFrom`) { + throw new Error(`Expected unionFrom`) + } + expect(builtQuery.from.sources.map((source) => source.alias)).toEqual([ + `employees`, + `departments`, + ]) @@ expect(builtQuery.from).toBeDefined() expect(builtQuery.from.type).toBe(`unionAll`) - expect((builtQuery.from as any).queries).toHaveLength(2) + if (builtQuery.from.type !== `unionAll`) { + throw new Error(`Expected unionAll`) + } + expect(builtQuery.from.queries).toHaveLength(2)As per coding guidelines, "Avoid using
anytypes; useunknowninstead when type is truly unknown and provide type guards to narrow safely".Also applies to: 80-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/builder/union-all.test.ts` around lines 45 - 47, The assertion currently uses "as any" to access builtQuery.from.sources[].alias; replace that with proper narrowing on the discriminant builtQuery.from.type (e.g. if (builtQuery.from.type === '...') { const from = builtQuery.from as /* appropriate interface */; expect(from.sources.map(s => s.alias)).toEqual([...]) }) or implement a small type guard that takes builtQuery.from: unknown and returns the correct branch type, and use unknown instead of any; apply the same change for the other assertions at the later occurrence (lines referenced around the second assertion).packages/db/tests/query/union-all.test.ts (2)
380-434: ⚡ Quick winAdd an assertion for lazy/deopt behavior in this lazy-join test.
This test name claims lazy deoptimization coverage, but currently it only checks result rows. It can pass even if lazy-loading behavior regresses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/union-all.test.ts` around lines 380 - 434, The test currently only asserts result rows but must also assert that the lazy-join branch was deoptimized; after calling collection.preload() add an assertion that the query was forced/materialized (e.g., check the live query or plan deopt flag exposed by your runtime) — reference the created query pieces messageRows and toolCallRows and the collection returned by createLiveQueryCollection (after preload) and assert the deoptimization/materialization indicator (for example assert collection.queryPlan.isDeoptimized or collection._isMaterialized or equivalent internal flag your codebase exposes) so the test fails if lazy-loading behavior regresses.
182-198: ⚡ Quick winReplace
anyin shared helper signatures withunknownand narrow with type guards.These helpers are reused extensively throughout the test file (11+ usages);
anycan silently mask type regressions. The refactor strengthens type safety while maintaining functionality.♻️ Proposed refactor
-function stripVirtualPropsDeep(value: any): any { +function stripVirtualPropsDeep(value: unknown): unknown { if (Array.isArray(value)) { return value.map((entry) => stripVirtualPropsDeep(entry)) } if (value && typeof value === `object`) { - const out: Record<string, any> = {} + const out: Record<string, unknown> = {} for (const [key, entry] of Object.entries(stripVirtualProps(value))) { out[key] = stripVirtualPropsDeep(entry) } return out } return value } -function childRows(collection: any): Array<any> { - return [...collection.toArray].map((row) => stripVirtualPropsDeep(row)) +function childRows<T>(collection: { toArray: Iterable<T> }): Array<unknown> { + return [...collection.toArray].map((row) => stripVirtualPropsDeep(row)) }Per coding guidelines:
**/*.{ts,tsx}— Avoid usinganytypes; useunknowninstead when type is truly unknown and provide type guards to narrow safely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/union-all.test.ts` around lines 182 - 198, Replace the loose any types in stripVirtualPropsDeep and childRows with unknown in their parameter and return signatures, and add narrowings/type guards: keep the Array.isArray branch but type the input as unknown and map with (entry: unknown) => stripVirtualPropsDeep(entry), use typeof value === "object" && value !== null to treat value as Record<string, unknown> and iterate Object.entries, calling stripVirtualProps (or a new type-guard wrapper) to narrow each property before recursing; for childRows accept collection: unknown, assert it has a toArray method or is iterable (via a small type guard) and map its elements through stripVirtualPropsDeep, and update return types to Array<unknown> (or unknown) accordingly so callers get stronger typing without using any.packages/db/tests/query/case-when.test.ts (1)
59-77: 💤 Low valueOptional: Prefer
unknownoveranyin test helper functions.The helper functions use
anytypes, which is pragmatic for test utilities but violates the coding guideline to preferunknownwith type narrowing.♻️ Optional refactor to align with type guidelines
-function stripVirtualPropsAndSymbols(value: any): any { +function stripVirtualPropsAndSymbols(value: unknown): unknown { if (Array.isArray(value)) { return value.map((entry) => stripVirtualPropsAndSymbols(entry)) } - if (value && typeof value === `object`) { - const out: Record<string, any> = {} + if (value !== null && typeof value === `object`) { + const out: Record<string, unknown> = {} for (const [key, entry] of Object.entries(stripVirtualProps(value))) { out[key] = stripVirtualPropsAndSymbols(entry) } return out } return value } -function childRows(collection: any): Array<any> { +function childRows(collection: { toArray: Iterable<unknown> }): Array<unknown> { return [...collection.toArray].map((row) => stripVirtualPropsAndSymbols(row)) }As per coding guidelines: "Avoid using
anytypes; useunknowninstead when type is truly unknown and provide type guards to narrow safely".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/case-when.test.ts` around lines 59 - 77, Update the test helpers to avoid using `any`: change the parameter and return types in stripVirtualPropsAndSymbols and childRows from `any` to `unknown` and add minimal runtime type narrowing/type guards where those helpers currently check Array.isArray, typeof === 'object', and use Object.entries so the compiler knows the narrowed types; keep the same logic but cast only after narrowing (e.g., assert non-null object before iterating) and keep stripVirtualProps referenced as-is but treat its output as unknown until narrowed.packages/db/src/query/builder/index.ts (1)
118-133: 💤 Low valueRedundant empty-object check.
The empty-object validation at lines 118-120 is repeated at lines 129-131 inside the
context !== 'unionAll clause'block. The first check throws before the second can ever be reached.♻️ Remove redundant check
if (keys.length === 0) { throw new InvalidSourceTypeError(context, `empty object`) } // Check if it looks like a string was passed (has numeric keys). // This applies to source-level unionAll as well as single-source clauses. if (keys.every((k) => !isNaN(Number(k)))) { throw new InvalidSourceTypeError(context, `string`) } if (context !== `unionAll clause` && keys.length !== 1) { - if (keys.length === 0) { - throw new InvalidSourceTypeError(context, `empty object`) - } throw new OnlyOneSourceAllowedError(context) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/query/builder/index.ts` around lines 118 - 133, The empty-object validation is duplicated: InvalidSourceTypeError(context, `empty object`) is checked first via keys.length === 0 and again inside the context !== `unionAll clause` block; remove the redundant check inside that block so only the initial keys.length === 0 throws, and keep the subsequent OnlyOneSourceAllowedError(context) logic intact (refer to variables keys and context and the error classes InvalidSourceTypeError and OnlyOneSourceAllowedError in the function in builder/index.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/src/query/compiler/joins.ts`:
- Around line 571-575: getFirstFromAlias currently only handles `unionFrom` and
falls back to `query.from.alias`, which breaks for `unionAll` (its shape has
`queries[]` not `alias`). Fix getFirstFromAlias to handle `unionAll` by
returning the first source/query's alias (e.g. recurse into
`query.from.queries[0]` and call getFirstFromAlias on it) so it returns a valid
alias when possible; also update the call site that compares `from.alias ===
fromInnerAlias` to tolerate an undefined `fromInnerAlias` (explicitly handle the
undefined case or guard the comparison) so subqueries aren't misclassified. Use
the function name getFirstFromAlias and the unionFrom/unionAll variants to
locate the changes.
---
Duplicate comments:
In `@packages/db/src/query/builder/index.ts`:
- Around line 1121-1129: The current child alias extraction in the
include-correlation path misses the unionAll case: when childQuery.from.type ===
'unionAll' childQuery.from.alias is empty and causes extractCorrelation() to
fail; update the logic that builds childAliases (the childAliases const in the
block handling childQuery.from.type === 'unionFrom') to also handle 'unionAll'
by collecting aliases from childQuery.from.sources (like the unionFrom branch)
and include those aliases plus any join.from.alias entries before calling
extractCorrelation(); ensure you reference childQuery.from.type,
childQuery.from.sources, childQuery.from.alias, childQuery.join and the
childAliases array so both unionFrom and unionAll produce the correct list of
aliases.
---
Nitpick comments:
In `@packages/db/src/query/builder/index.ts`:
- Around line 118-133: The empty-object validation is duplicated:
InvalidSourceTypeError(context, `empty object`) is checked first via keys.length
=== 0 and again inside the context !== `unionAll clause` block; remove the
redundant check inside that block so only the initial keys.length === 0 throws,
and keep the subsequent OnlyOneSourceAllowedError(context) logic intact (refer
to variables keys and context and the error classes InvalidSourceTypeError and
OnlyOneSourceAllowedError in the function in builder/index.ts).
In `@packages/db/tests/query/builder/union-all.test.ts`:
- Around line 45-47: The assertion currently uses "as any" to access
builtQuery.from.sources[].alias; replace that with proper narrowing on the
discriminant builtQuery.from.type (e.g. if (builtQuery.from.type === '...') {
const from = builtQuery.from as /* appropriate interface */;
expect(from.sources.map(s => s.alias)).toEqual([...]) }) or implement a small
type guard that takes builtQuery.from: unknown and returns the correct branch
type, and use unknown instead of any; apply the same change for the other
assertions at the later occurrence (lines referenced around the second
assertion).
In `@packages/db/tests/query/case-when.test.ts`:
- Around line 59-77: Update the test helpers to avoid using `any`: change the
parameter and return types in stripVirtualPropsAndSymbols and childRows from
`any` to `unknown` and add minimal runtime type narrowing/type guards where
those helpers currently check Array.isArray, typeof === 'object', and use
Object.entries so the compiler knows the narrowed types; keep the same logic but
cast only after narrowing (e.g., assert non-null object before iterating) and
keep stripVirtualProps referenced as-is but treat its output as unknown until
narrowed.
In `@packages/db/tests/query/union-all.test-d.ts`:
- Around line 36-64: The three fixtures createMessages, createToolCalls, and
createUsers duplicate the same pattern; replace them with a single generic
factory (e.g., createTypedCollection<T>) that calls createCollection with
mockSyncCollectionOptions<T> and accepts an id and getKey (or infers getKey as
(item) => item.id) to produce collections for Message, ToolCall, and User;
update tests to call
createTypedCollection<Message>(`multi-source-type-messages`),
createTypedCollection<ToolCall>(`multi-source-type-tools`), and
createTypedCollection<User>(`multi-source-type-users`) and remove the three
specific functions (references: createMessages, createToolCalls, createUsers,
createCollection, mockSyncCollectionOptions).
In `@packages/db/tests/query/union-all.test.ts`:
- Around line 380-434: The test currently only asserts result rows but must also
assert that the lazy-join branch was deoptimized; after calling
collection.preload() add an assertion that the query was forced/materialized
(e.g., check the live query or plan deopt flag exposed by your runtime) —
reference the created query pieces messageRows and toolCallRows and the
collection returned by createLiveQueryCollection (after preload) and assert the
deoptimization/materialization indicator (for example assert
collection.queryPlan.isDeoptimized or collection._isMaterialized or equivalent
internal flag your codebase exposes) so the test fails if lazy-loading behavior
regresses.
- Around line 182-198: Replace the loose any types in stripVirtualPropsDeep and
childRows with unknown in their parameter and return signatures, and add
narrowings/type guards: keep the Array.isArray branch but type the input as
unknown and map with (entry: unknown) => stripVirtualPropsDeep(entry), use
typeof value === "object" && value !== null to treat value as Record<string,
unknown> and iterate Object.entries, calling stripVirtualProps (or a new
type-guard wrapper) to narrow each property before recursing; for childRows
accept collection: unknown, assert it has a toArray method or is iterable (via a
small type guard) and map its elements through stripVirtualPropsDeep, and update
return types to Array<unknown> (or unknown) accordingly so callers get stronger
typing without using any.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba965af4-978a-44cf-b8fd-cb0540a49c14
📒 Files selected for processing (26)
.changeset/tender-mugs-hear.mddocs/guides/live-queries.mdpackages/db/src/errors.tspackages/db/src/query/builder/index.tspackages/db/src/query/builder/ref-proxy.tspackages/db/src/query/builder/types.tspackages/db/src/query/compiler/group-by.tspackages/db/src/query/compiler/index.tspackages/db/src/query/compiler/joins.tspackages/db/src/query/compiler/lazy-targets.tspackages/db/src/query/compiler/order-by.tspackages/db/src/query/compiler/select.tspackages/db/src/query/effect.tspackages/db/src/query/ir.tspackages/db/src/query/live/collection-config-builder.tspackages/db/src/query/live/collection-subscriber.tspackages/db/src/query/live/utils.tspackages/db/src/query/optimizer.tspackages/db/tests/query/builder/from.test.tspackages/db/tests/query/builder/union-all.test.tspackages/db/tests/query/case-when.test.tspackages/db/tests/query/compiler/subqueries.test.tspackages/db/tests/query/group-by.test.tspackages/db/tests/query/includes.test.tspackages/db/tests/query/union-all.test-d.tspackages/db/tests/query/union-all.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/tender-mugs-hear.md
Co-authored-by: Cursor <[email protected]>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/errors.ts (1)
394-402:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a
.join(...)example forjoin clausevalidation errors.
InvalidSourceTypeError('join clause', ...)still renders the.from(...)example, so a malformed join call gets pointed at the wrong API.💡 Proposed fix
const example = context === `unionAll clause` ? `.unionAll({ todos: todosCollection, events: eventsCollection })` + : context === `join clause` + ? `.join({ todos: todosCollection }, ({ current, todos }) => eq(current.id, todos.id))` : `.from({ todos: todosCollection })`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/errors.ts` around lines 394 - 402, The constructor that builds the validation message (constructor(context: SourceClauseContext, type: string) in errors.ts) currently only distinguishes `unionAll clause` vs other contexts and therefore uses a `.from(...)` example for `join clause`; update the logic to handle `join clause` explicitly so that when context === `join clause` you set expected to indicate the join signature and set example to use `.join({ alias: collection })` (or the appropriate multi-arg join example) instead of `.from(...)`, keeping the existing `unionAll clause` branch unchanged; modify the conditional that assigns expected and example (referencing the constructor and the context variable) to include this new branch so InvalidSourceTypeError('join clause', ...) shows the correct `.join(...)` example.
♻️ Duplicate comments (1)
packages/db/src/query/compiler/index.ts (1)
245-285:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCorrelated includes still skip parent filtering for direct union-backed children.
The
!isUnionFromguard means a child include built directly fromq.unionAll(...)or a multi-source union never gets inner-joined withparentKeyStream.processUnionAll()also returnsalias: '', so the later correlation lookup has no stable place to read from. Those includes will compile/load the full union instead of the parent-matched subset and can’t reliably route rows back to the parent.Also applies to: 1168-1259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/query/compiler/index.ts` around lines 245 - 285, The current guard (if (!isUnionFrom && parentKeyStream && childCorrelationField)) skips the inner-join logic for children that come from a union (isUnionFrom true) and for unionAll which returns alias '', causing union-backed children to bypass parentKeyStream filtering; update the condition to include union-backed children and ensure mainSource resolves even when alias is empty: either remove the !isUnionFrom check or change it to allow isUnionFrom cases, and when mainSource is '' (processUnionAll return), resolve the actual source stream from the union metadata (or make processUnionAll return a stable alias) so sources[mainSource] points at the union stream; then run the existing rekey/join/map sequence (references: isUnionFrom, parentKeyStream, childCorrelationField, mainSource, sources, processUnionAll, wrapInputWithAlias) so union children are inner-joined to the parentKeyStream and wrapped with alias for downstream routing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/db/src/errors.ts`:
- Around line 394-402: The constructor that builds the validation message
(constructor(context: SourceClauseContext, type: string) in errors.ts) currently
only distinguishes `unionAll clause` vs other contexts and therefore uses a
`.from(...)` example for `join clause`; update the logic to handle `join clause`
explicitly so that when context === `join clause` you set expected to indicate
the join signature and set example to use `.join({ alias: collection })` (or the
appropriate multi-arg join example) instead of `.from(...)`, keeping the
existing `unionAll clause` branch unchanged; modify the conditional that assigns
expected and example (referencing the constructor and the context variable) to
include this new branch so InvalidSourceTypeError('join clause', ...) shows the
correct `.join(...)` example.
---
Duplicate comments:
In `@packages/db/src/query/compiler/index.ts`:
- Around line 245-285: The current guard (if (!isUnionFrom && parentKeyStream &&
childCorrelationField)) skips the inner-join logic for children that come from a
union (isUnionFrom true) and for unionAll which returns alias '', causing
union-backed children to bypass parentKeyStream filtering; update the condition
to include union-backed children and ensure mainSource resolves even when alias
is empty: either remove the !isUnionFrom check or change it to allow isUnionFrom
cases, and when mainSource is '' (processUnionAll return), resolve the actual
source stream from the union metadata (or make processUnionAll return a stable
alias) so sources[mainSource] points at the union stream; then run the existing
rekey/join/map sequence (references: isUnionFrom, parentKeyStream,
childCorrelationField, mainSource, sources, processUnionAll, wrapInputWithAlias)
so union children are inner-joined to the parentKeyStream and wrapped with alias
for downstream routing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8faa2f0-ad89-4896-b30f-6b75828d4062
📒 Files selected for processing (9)
docs/guides/live-queries.mdpackages/db/src/errors.tspackages/db/src/query/builder/index.tspackages/db/src/query/compiler/group-by.tspackages/db/src/query/compiler/index.tspackages/db/src/query/compiler/lazy-targets.tspackages/db/src/query/optimizer.tspackages/db/tests/query/builder/union-all.test.tspackages/db/tests/query/union-all.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/guides/live-queries.md
samwillis
changed the title
Guard join compilation against unionAll subqueries and show context-specific examples for invalid join sources. Co-authored-by: Cursor <[email protected]>
kevin-dp
approved these changes
May 21, 2026
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/db/tests/query/builder/union-all.test.ts (1)
96-109: ⚡ Quick winAdd the empty-source regression case.
_createRefsForSource()now has a dedicatedempty objectfailure path, but this suite never exercisesbuilder.unionAll({} as any), so that branch can regress unnoticed.Suggested assertion
expect(() => builder.unionAll([employeesCollection] as any)).toThrow( InvalidSourceTypeError, ) + expect(() => builder.unionAll({} as any)).toThrow( + InvalidSourceTypeError, + ) expect(() => builder.unionAll({ employees: [] } as any)).toThrow( InvalidSourceError, )As per coding guidelines,
**/*.test.{ts,tsx,js,jsx}:Add unit tests that reproduce bugs before fixing them to validate fixes and prevent regressionsandTest corner cases including: empty collections, single elements, undefined vs null, resolved promises, race conditions, limit/offset edge cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/builder/union-all.test.ts` around lines 96 - 109, Add a unit test for the empty-object regression: in the existing union-all.test.ts case that checks invalid inputs, call builder.unionAll({} as any) and assert it throws the same error path used for empty sources (i.e., expect(() => builder.unionAll({} as any)).toThrow(InvalidSourceError)), so the dedicated empty-object branch in _createRefsForSource is exercised and prevented from regressing; locate this near the other builder.unionAll(...) assertions in the same test block.packages/db/tests/query/union-all.test.ts (1)
1453-1537: ⚡ Quick winCover explicit
nulldefaults in guarded union projections.This PR changed the traversal/rewrite paths from truthy checks to
!== undefined, but the new cases here still only exercise omitted defaults. A smallcaseWhen(..., null)regression test would lock in the null-vs-undefined behavior that changed.Suggested regression test
+ it(`preserves explicit null defaults in guarded projections`, async () => { + const messages = createMessagesCollection(`multi-source-messages-null-default`) + const toolCalls = createToolCallsCollection(`multi-source-tools-null-default`) + + const collection = createLiveQueryCollection((q) => + q + .unionAll({ + message: messages, + toolCall: toolCalls, + }) + .select(({ message, toolCall }) => ({ + messageId: caseWhen(message.id, message.id, null), + toolCallId: caseWhen(toolCall.id, toolCall.id, null), + })) + .orderBy(({ $selected }) => + coalesce($selected.messageId, $selected.toolCallId), + ), + ) + + await collection.preload() + + expect(collection.toArray.map(stripVirtualProps)).toEqual([ + { messageId: 1, toolCallId: null }, + { messageId: null, toolCallId: 1 }, + { messageId: 2, toolCallId: null }, + { messageId: null, toolCallId: 3 }, + ]) + })As per coding guidelines,
**/*.test.{ts,tsx,js,jsx}:Add unit tests that reproduce bugs before fixing them to validate fixes and prevent regressionsandTest corner cases including: empty collections, single elements, undefined vs null, resolved promises, race conditions, limit/offset edge cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/union-all.test.ts` around lines 1453 - 1537, The test currently only exercises omitted defaults (undefined) for guarded union projections; add a regression case that uses an explicit null default in the caseWhen call to lock in null-vs-undefined behavior. Locate the select that builds event with caseWhen in this test (the caseWhen call inside the unionAll.select) and add another assertion run or adjust the caseWhen so one branch yields null as the fallback (e.g., change the default/else argument of caseWhen to null for one branch) and update the expected array to include the explicit null-event row(s) so the test verifies null vs undefined handling for message/toolCall projections.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/db/tests/query/builder/union-all.test.ts`:
- Around line 96-109: Add a unit test for the empty-object regression: in the
existing union-all.test.ts case that checks invalid inputs, call
builder.unionAll({} as any) and assert it throws the same error path used for
empty sources (i.e., expect(() => builder.unionAll({} as
any)).toThrow(InvalidSourceError)), so the dedicated empty-object branch in
_createRefsForSource is exercised and prevented from regressing; locate this
near the other builder.unionAll(...) assertions in the same test block.
In `@packages/db/tests/query/union-all.test.ts`:
- Around line 1453-1537: The test currently only exercises omitted defaults
(undefined) for guarded union projections; add a regression case that uses an
explicit null default in the caseWhen call to lock in null-vs-undefined
behavior. Locate the select that builds event with caseWhen in this test (the
caseWhen call inside the unionAll.select) and add another assertion run or
adjust the caseWhen so one branch yields null as the fallback (e.g., change the
default/else argument of caseWhen to null for one branch) and update the
expected array to include the explicit null-event row(s) so the test verifies
null vs undefined handling for message/toolCall projections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9080362b-08cb-485f-83d1-9566a5c4bdb3
📒 Files selected for processing (10)
docs/guides/live-queries.mdpackages/db/src/indexes/auto-index.tspackages/db/src/query/builder/index.tspackages/db/src/query/compiler/index.tspackages/db/src/query/index.tspackages/db/src/query/live/utils.tspackages/db/tests/collection-events.test.tspackages/db/tests/query/builder/union-all.test.tspackages/db/tests/query/union-all.test-d.tspackages/db/tests/query/union-all.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/db/src/indexes/auto-index.ts
- packages/db/tests/collection-events.test.ts
- docs/guides/live-queries.md
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
This PR adds explicit
unionAll()support to the DB query builder. It replaces the earlier multi-sourcefrom()design with a clearer API for combining independent sources or query branches into one live query result without requiring a join between those branches.from()now remains a single-source start method.unionAll()is the explicit start method for union-style queries.What Changed
unionAll({ alias: collectionOrQuery, ... })for combining independent collections/subqueries while preserving source namespaces.unionAll(queryA, queryB, ...)for combining already-shaped query results into a result-level union.unionAlldownstream clauses operate on the union result row shape, not the original branch source aliases.where,select,orderBy,distinct,limit,offset,groupBy,having, joins, and includes across union results.unionAllby preserving an exact ref schema separately from the widened compatibility schema.select()projections and through deoptimizedfn.select()projections.unionAllbranches in compiler, optimizer, live query subscription extraction, alias extraction, and collection extraction.unionAllis the lazy side, while retaining safe per-branch lazy loading where possible.unionAll()forms and ordering/collation behavior in the live queries guide.Examples
Source-level union with namespaces:
Without
select, the source-level result is an exclusive union:Query-branch union with a discriminated result shape:
Join after query-branch
unionAll:Branch includes can be materialized through an outer projection:
Design Notes
unionAll({ ...sources })preserves source aliases as namespaces. This is useful when callers want to keep branch-specific row shapes visible.unionAll(queryA, queryB)unions query results. Downstream callbacks see the result fields from those branch queries.unionAllas a derived result relation unless they are explicitly lowering union branches.orderBycompare options when they need a specific collation.Test Plan
pnpm --filter @tanstack/db test tests/query/union-all.test.ts tests/query/union-all.test-d.ts tests/query/builder/union-all.test.ts tests/query/group-by.test.tspnpm --filter @tanstack/db test tests/query/union-all.test.ts tests/query/union-all.test-d.tspnpm --filter @tanstack/db lintpnpm --filter @tanstack/db buildSummary by CodeRabbit
New Features
Documentation
Behavioral Changes