fix(db): preserve discriminated union types through .select() (#1511)#1597
fix(db): preserve discriminated union types through .select() (#1511)#1597kevin-dp wants to merge 3 commits into
Conversation
…ck#1511) Distribute Ref<T> over its T parameter so a discriminated union field no longer has its keys collapsed by the mapped-type keyof. Reshape ExtractRef<T> to distinguish a real branded Ref (return the underlying user type U directly) from a spread-produced inline object (still projected via ResultTypeFromSelect). Add DeepNullable<U> so the Nullable=true flag (from left/right/full joins) keeps propagating | undefined into every leaf, preserving prior join-test behavior. Fixes the issue both at the top level and when the union field is nested inside another selected object.
📝 WalkthroughWalkthroughRefactors ChangesSelect discriminated union type preservation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/src/query/builder/types.ts (1)
513-515: ⚡ Quick winPrefer
unknownoveranyinDeepNullableconstraint.Using
Record<string, unknown>keeps this helper type-safe while preserving intent. Please re-run the existing type-level tests after the swap.♻️ Proposed change
type DeepNullable<T> = - T extends Record<string, any> + T extends Record<string, unknown> ? IsPlainObject<T> extends true ? { [K in keyof T]: DeepNullable<T[K]> } : T | undefined : T | undefinedAs per coding guidelines, "
**/*.{ts,tsx}: Avoid usinganytypes; useunknowninstead when the type is truly unknown`."🤖 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/types.ts` around lines 513 - 515, The DeepNullable type definition uses Record<string, any> in its constraint which violates the type-safety coding guidelines. Replace the any keyword with unknown in the Record constraint within the DeepNullable type to maintain type-safety while preserving the intent of the helper type, then run the existing type-level tests to ensure the change does not break any type definitions.Source: Coding guidelines
🤖 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/src/query/builder/types.ts`:
- Around line 513-515: The DeepNullable type definition uses Record<string, any>
in its constraint which violates the type-safety coding guidelines. Replace the
any keyword with unknown in the Record constraint within the DeepNullable type
to maintain type-safety while preserving the intent of the helper type, then run
the existing type-level tests to ensure the change does not break any type
definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 775011a2-7dad-4cac-a956-a65a2f2e1430
📒 Files selected for processing (3)
.changeset/fix-ref-union-collapse-b.mdpackages/db/src/query/builder/types.tspackages/db/tests/query/select.test-d.ts
|
Thanks for the fix here — the distributive
type IsTrueRef<T> =
T extends RefLeaf<infer U>
? [
Exclude<
keyof T,
| typeof RefBrand
| typeof NullableBrand
| keyof VirtualRowProps
| keyof U
>,
] extends [never]
? true
: false
: falseThat proves “no extra keys”, but it does not prove that I reproduced two failing type cases against this PR. 1. Same-key compatible/narrower overridetype User = {
id: number
code: string | number
slug: string
nickname?: string
}
const col = createLiveQueryCollection((q) =>
q.from({ u: users }).select(({ u }) => ({
narrowed: { ...u, code: u.slug },
})),
)
const result = col.toArray[0]!
expectTypeOf(result.narrowed.code).toEqualTypeOf<string>()
2. Omitted optional property is reintroducedtype User = {
id: number
code: string | number
slug: string
nickname?: string
}
const col = createLiveQueryCollection((q) =>
q.from({ u: users }).select(({ u }) => {
const { nickname, ...withoutNickname } = u
return { trimmed: withoutNickname }
}),
)
const result = col.toArray[0]!
type HasNickname = `nickname` extends keyof typeof result.trimmed ? true : false
expectTypeOf<HasNickname>().toEqualTypeOf<false>()Since The existing spread regression test for I also verified locally that tightening Could you add regression tests for the two cases above and update |
The new `IsTrueRef` fast path only checked that a ref had no keys beyond
`keyof U` (plus the brand/virtual props). That key-subset check let
spread-derived objects that keep the same key set but change a field's
type (`{ ...u, code: u.slug }`) or drop an optional key
(`const { nickname, ...rest } = u`) be classified as a true `Ref` and
collapsed back to `U`, discarding the projection.
Require strict structural equivalence against the canonical `Ref<U>`
shape instead, so only genuine refs take the fast path and any
spread-derived object falls through to `ResultTypeFromSelect`. Adds
regression tests for both cases.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tests/query/select.test-d.ts`:
- Line 218: The destructuring in the select type test is triggering the unused
binding lint because `nickname` is bound but never used, while the rest omission
behavior must still exclude the `nickname` property. Update the destructuring in
the `u` assignment to use an aliasing rename so the property key remains
`nickname` but the local variable name satisfies the `/^_/` rule, and keep the
rest object check in `select.test-d.ts` unchanged semantically.
- Around line 223-229: The `result` binding in `select.test-d.ts` is only used
for a type query, so remove the unused runtime variable and reference the
projected element type directly from `col.toArray` instead. Update the
`HasNickname` check to use the `trimmed` property type from the collection’s
return type rather than `typeof result.trimmed`, keeping the same assertion with
`expectTypeOf` while avoiding the lint warning.
🪄 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: 56f04438-ac1f-4293-a106-c10bfc701be2
📒 Files selected for processing (3)
.changeset/fix-ref-union-collapse-b.mdpackages/db/src/query/builder/types.tspackages/db/tests/query/select.test-d.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-ref-union-collapse-b.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/query/builder/types.ts
|
|
||
| const col = createLiveQueryCollection((q) => | ||
| q.from({ u: spreadUsers }).select(({ u }) => { | ||
| const { nickname, ...withoutNickname } = u |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Lint failure: unused nickname binding.
ESLint flags nickname as an unused variable (must match /^_/). You can't just rename the binding to _nickname because that would change which property is destructured-out and break the omission semantics this test relies on. Use a rename binding so the key nickname is still removed from the rest while the local name satisfies the rule.
🛠️ Proposed fix
- const { nickname, ...withoutNickname } = u
+ const { nickname: _nickname, ...withoutNickname } = u📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { nickname, ...withoutNickname } = u | |
| const { nickname: _nickname, ...withoutNickname } = u |
🧰 Tools
🪛 ESLint
[error] 218-218: 'nickname' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
🤖 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/select.test-d.ts` at line 218, The destructuring in
the select type test is triggering the unused binding lint because `nickname` is
bound but never used, while the rest omission behavior must still exclude the
`nickname` property. Update the destructuring in the `u` assignment to use an
aliasing rename so the property key remains `nickname` but the local variable
name satisfies the `/^_/` rule, and keep the rest object check in
`select.test-d.ts` unchanged semantically.
Source: Linters/SAST tools
| const result = col.toArray[0]! | ||
| // `nickname` was destructured out, so the projected object must | ||
| // not reintroduce the key. | ||
| type HasNickname = `nickname` extends keyof typeof result.trimmed | ||
| ? true | ||
| : false | ||
| expectTypeOf<HasNickname>().toEqualTypeOf<false>() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
Remove unused runtime binding to fix type-only lint warning
The variable result is only consumed in a type position (typeof result.trimmed), triggering an ESLint warning. Access the projected type directly from the collection's return type instead of creating an unused runtime binding.
🛠️ Proposed fix
- const result = col.toArray[0]!
- // `nickname` was destructured out, so the projected object must
- // not reintroduce the key.
- type HasNickname = `nickname` extends keyof typeof result.trimmed
+ // `nickname` was destructured out, so the projected object must
+ // not reintroduce the key.
+ type HasNickname = `nickname` extends keyof (typeof col.toArray)[number][`trimmed`]
? true
: false
expectTypeOf<HasNickname>().toEqualTypeOf<false>()The expression (typeof col.toArray)[number][trimmed] correctly resolves to the trimmed property type of the array elements, eliminating the need for the intermediate result variable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = col.toArray[0]! | |
| // `nickname` was destructured out, so the projected object must | |
| // not reintroduce the key. | |
| type HasNickname = `nickname` extends keyof typeof result.trimmed | |
| ? true | |
| : false | |
| expectTypeOf<HasNickname>().toEqualTypeOf<false>() | |
| // `nickname` was destructured out, so the projected object must | |
| // not reintroduce the key. | |
| type HasNickname = `nickname` extends keyof (typeof col.toArray)[number][`trimmed`] | |
| ? true | |
| : false | |
| expectTypeOf<HasNickname>().toEqualTypeOf<false>() |
🧰 Tools
🪛 ESLint
[error] 223-223: 'result' is assigned a value but only used as a type. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
🤖 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/select.test-d.ts` around lines 223 - 229, The
`result` binding in `select.test-d.ts` is only used for a type query, so remove
the unused runtime variable and reference the projected element type directly
from `col.toArray` instead. Update the `HasNickname` check to use the `trimmed`
property type from the collection’s return type rather than `typeof
result.trimmed`, keeping the same assertion with `expectTypeOf` while avoiding
the lint warning.
|
@KyleAMathews I've pushed a fix that replaces the subset/no-extra-keys check in type IsTrueRef<T> =
T extends RefLeaf<infer U>
? RefShapeMatches<T, Ref<U, IsNullableRef<T>>> extends true
? true
: false
: false
// Strict structural equivalence (Equals idiom)
type RefShapeMatches<A, B> =
(<G>() => G extends A ? 1 : 2) extends <G>() => G extends B ? 1 : 2
? true
: falseA couple of notes on the choice of check:
I also added the two regression tests you provided to |
3 participants
Fixes #1511.
Problem
.select()collapses a discriminated-union field (e.g.{ type: 'pdf'; url: string } | { type: 'image'; width: number }) to the intersection of common keys. Two failing-on-main type tests inpackages/db/tests/query/select.test-d.tsdemonstrate both the shallow case and the nested case (union insidepayload: { inner: Payload }).Root cause
Two compounding issues in
packages/db/src/query/builder/types.ts:Ref<T, Nullable>is a non-distributive mapped type. WhenTis a discriminated union,[K in keyof T]only iterates the common keys — the union is lost before per-key processing ever runs.ExtractRef<T>projects the branded shape back throughResultTypeFromSelect, which drags theVirtualPropsRefintersection ($synced,$origin,$key,$collectionId) into the user-facing result whenever a Ref to a sub-object is selected. This is a pre-existing leak that the nested-union test makes visible.Fix
Ref<T, Nullable>distribute overTvia aT extends unknown ? RefBranch<T, Nullable> : neverwrapper.Ref<A | B>becomesRef<A> | Ref<B>, sokeyofnever sees the union.ExtractRef<T>to distinguish a trueRef(brand carrier whose own keys are exactly the brand symbols + virtual props +keyof U) from a spread-produced inline object (which inherits the brand symbol from the spread but also carries extra keys). True Refs return the underlying user typeUdirectly; spreads fall through to the originalPrettify<ResultTypeFromSelect<WithoutRefBrand<T>>>projection (preserving virtual props as the spread tests require).DeepNullable<U>so theNullable=trueflag (left/right/full joins) keeps propagating| undefinedinto every leaf, matching what the priorResultTypeFromSelectrecursion produced for nullable joins. This preserves all existing join-test behavior.No changes outside
packages/db/src/query/builder/types.ts. ~36 LOC.Why not approach (a) (PR #1512)
PR #1512 adds an
IsUnion-gated shortcut in the middle of theResultTypeFromSelectcascade. It makes the shallow test pass under vitest's typecheck runner, buttsc --noEmitstill flags both new tests as failing because virtual props leak into the projected union, and the nested case still collapses. This PR fixes both cases undertsc.Verification
Both new union tests pass under both
tscand vitest. No regressions in the existing select / select-spread / join type-tests.IDE display
Hovering a discriminated-union Ref field now shows
Ref<A> | Ref<B> | Ref<C>instead of the previous collapsed form. This is an improvement (it exposes the union the user wrote) but is a visible change for anyone who was reading the collapsed display.Summary by CodeRabbit
.select()type inference so discriminated-union selections no longer collapse into the intersection of shared keys.