fix(db, db-ivm): support Temporal objects in join hashing and normalization#1370
Open
KyleAMathews wants to merge 5 commits intomainfrom
Open
fix(db, db-ivm): support Temporal objects in join hashing and normalization#1370KyleAMathews wants to merge 5 commits intomainfrom
KyleAMathews wants to merge 5 commits intomainfrom
Conversation
Temporal objects (PlainDate, ZonedDateTime, etc.) have no enumerable own properties, so Object.keys() returns [] and all instances produce identical hashes. This causes the IVM join Index to treat old and new rows as equal, silently swallowing updates when only a Temporal field changed. Hash Temporal objects by their Symbol.toStringTag type and toString() representation to produce correct, value-based hashes. Fixes #1367 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…zation Temporal objects have no enumerable properties, so hashPlainObject() produced identical hashes for all Temporal values. This caused join index updates to be silently swallowed when a Temporal field changed. - Add Temporal-aware hashing via Symbol.toStringTag + toString() - Add Temporal normalization in normalizeValue() for join key matching - Add Temporal handling in ascComparator for correct sort ordering - Add null guard to exported isTemporal() - Convert temporalTypes from Array to Set for consistency Fixes #1367 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: bcf3ef5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds Temporal-aware hashing, normalization, and comparison so Temporal objects are hashed and compared by their tag + string representation, ensuring Temporal field changes produce distinct hashes and propagate through IVM join indexes and live queries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
📝 Coding Plan
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/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Contributor
|
Size Change: +97 B (+0.1%) Total Size: 98.7 kB
ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 4.23 kB ℹ️ View Unchanged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/db-ivm/src/hashing/hash.ts (1)
23-32: Consider centralizing Temporal type tags to avoid cross-package drift.
Line 23-Line 32 duplicates the same Temporal tag registry already present inpackages/db/src/utils.ts; keeping one source of truth reduces future mismatch risk.As per coding guidelines: Extract common logic into reusable utility functions when duplicated across multiple places.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db-ivm/src/hashing/hash.ts` around lines 23 - 32, The local temporalTypes Set duplicates the Temporal tag registry; remove this local declaration and import the single shared constant from the common utils module (export it as a stable symbol such as TEMPORAL_TYPE_TAGS or getTemporalTypeTags) so both packages use the same source of truth; update this file to reference that imported symbol (replacing temporalTypes) and ensure the shared utils exports are typed and tested for consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/db-ivm/src/hashing/hash.ts`:
- Around line 34-37: The current isTemporal uses unsafe "as any" casts; define a
TemporalLike interface/type that includes an optional [Symbol.toStringTag]:
string and any Temporal-specific properties needed, change isTemporal to the
type guard signature "function isTemporal(input: unknown): input is
TemporalLike" (or "input is TemporalLike") and implement it by reading (input as
TemporalLike)[Symbol.toStringTag] and checking temporalTypes, then remove all
uses of "as any" in hashTemporal and elsewhere by relying on the type guard to
narrow input to TemporalLike before accessing Temporal fields; update
hashTemporal to accept the narrowed type rather than casting.
In `@packages/db/src/utils/comparison.ts`:
- Around line 58-65: The Temporal branch currently compares by toString(), which
can misorder values (especially ZonedDateTime across zones) and does not enforce
same-type comparison; change it so when isTemporal(a) && isTemporal(b) and they
are the same Temporal type (e.g., a.constructor === b.constructor or compare
constructor names), call the appropriate Temporal compare method for that type
(e.g., Temporal.ZonedDateTime.compare(a,b), Temporal.PlainDateTime.compare(a,b),
Temporal.PlainDate.compare(a,b), Temporal.PlainTime.compare(a,b), etc.) and
return the compare result normalized to -1/0/1; if the types differ fall back to
the existing non-Temporal logic (or a stable tie-breaker) rather than comparing
toString().
---
Nitpick comments:
In `@packages/db-ivm/src/hashing/hash.ts`:
- Around line 23-32: The local temporalTypes Set duplicates the Temporal tag
registry; remove this local declaration and import the single shared constant
from the common utils module (export it as a stable symbol such as
TEMPORAL_TYPE_TAGS or getTemporalTypeTags) so both packages use the same source
of truth; update this file to reference that imported symbol (replacing
temporalTypes) and ensure the shared utils exports are typed and tested for
consumers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79560193-361d-49f8-ad69-09528e1e8ac9
📒 Files selected for processing (4)
.changeset/fix-temporal-join-hashing.mdpackages/db-ivm/src/hashing/hash.tspackages/db/src/utils.tspackages/db/src/utils/comparison.ts
Comment on lines
+58
to
+65
| // If both are Temporal objects of the same type, compare by string representation | ||
| if (isTemporal(a) && isTemporal(b)) { | ||
| const aStr = a.toString() | ||
| const bStr = b.toString() | ||
| if (aStr < bStr) return -1 | ||
| if (aStr > bStr) return 1 | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Use Temporal compare semantics instead of raw toString() ordering.
Line 58-Line 65 can misorder chronological values (e.g., Temporal.ZonedDateTime across different zones/offsets). It also doesn’t enforce same-type comparison despite the comment.
💡 Proposed fix
// If both are Temporal objects of the same type, compare by string representation
if (isTemporal(a) && isTemporal(b)) {
- const aStr = a.toString()
- const bStr = b.toString()
- if (aStr < bStr) return -1
- if (aStr > bStr) return 1
- return 0
+ const aTag = a[Symbol.toStringTag]
+ const bTag = b[Symbol.toStringTag]
+
+ // Prefer Temporal's native compare when both values share the same constructor/type.
+ if (aTag === bTag && a.constructor === b.constructor) {
+ const compare = (a.constructor as { compare?: (x: unknown, y: unknown) => number }).compare
+ if (typeof compare === `function`) {
+ return compare(a, b)
+ }
+ }
+
+ // Deterministic fallback for mixed Temporal types or missing compare().
+ if (aTag < bTag) return -1
+ if (aTag > bTag) return 1
+ const aStr = a.toString()
+ const bStr = b.toString()
+ if (aStr < bStr) return -1
+ if (aStr > bStr) return 1
+ return 0
}For JavaScript Temporal, can lexicographic ordering of `toString()` differ from `Temporal.ZonedDateTime.compare(a, b)` for values in different time zones/offsets? Provide a concrete example.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/utils/comparison.ts` around lines 58 - 65, The Temporal
branch currently compares by toString(), which can misorder values (especially
ZonedDateTime across zones) and does not enforce same-type comparison; change it
so when isTemporal(a) && isTemporal(b) and they are the same Temporal type
(e.g., a.constructor === b.constructor or compare constructor names), call the
appropriate Temporal compare method for that type (e.g.,
Temporal.ZonedDateTime.compare(a,b), Temporal.PlainDateTime.compare(a,b),
Temporal.PlainDate.compare(a,b), Temporal.PlainTime.compare(a,b), etc.) and
return the compare result normalized to -1/0/1; if the types differ fall back to
the existing non-Temporal logic (or a stable tie-breaker) rather than comparing
toString().
Incorporates Ben's Temporal hash fix with tests: - Unit tests for Temporal hash correctness (mock-based) - Regression test for join live query with Temporal field updates - Removed duplicate changeset (ours covers both packages) - Resolved hash.ts conflict: kept Set-based temporalTypes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/db/tests/query/join.test.ts (1)
2093-2095: Removeanytype cast from update callback.The
dueDateproperty is defined on theTasktype (line 2035), so theas anycast is unnecessary. Use explicit typing on the callback parameter instead:Suggested fix
- taskCollection.update(1, (draft) => { - ;(draft as any).dueDate = Temporal.PlainDate.from(`2024-06-15`) - }) + taskCollection.update(1, (draft: Task) => { + draft.dueDate = Temporal.PlainDate.from(`2024-06-15`) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/tests/query/join.test.ts` around lines 2093 - 2095, Remove the unnecessary "as any" cast in the update callback for taskCollection.update; instead give the callback a proper typed parameter (e.g., (draft: Task) => { draft.dueDate = Temporal.PlainDate.from('2024-06-15') }) so the compiler knows draft has dueDate; reference the Task type (declared earlier) and the taskCollection.update call to locate and replace the "(draft as any)" usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/db-ivm/tests/utils.test.ts`:
- Around line 7-12: The function createTemporalLike returns an untyped object
from Object.create which becomes any; add an explicit return type such as {
toString(): string; [Symbol.toStringTag]: string } to the function signature so
TypeScript enforces the shape (e.g. change function createTemporalLike(tag:
string, value: string): { toString(): string; [Symbol.toStringTag]: string } {
... }), referencing createTemporalLike, Symbol.toStringTag and the toString
property in the change.
---
Nitpick comments:
In `@packages/db/tests/query/join.test.ts`:
- Around line 2093-2095: Remove the unnecessary "as any" cast in the update
callback for taskCollection.update; instead give the callback a proper typed
parameter (e.g., (draft: Task) => { draft.dueDate =
Temporal.PlainDate.from('2024-06-15') }) so the compiler knows draft has
dueDate; reference the Task type (declared earlier) and the
taskCollection.update call to locate and replace the "(draft as any)" usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e94ee10-c120-4dde-8965-f4adc304ef4f
📒 Files selected for processing (2)
packages/db-ivm/tests/utils.test.tspackages/db/tests/query/join.test.ts
samwillis
requested changes
Mar 13, 2026
Collaborator
samwillis
left a comment
samwillis
left a comment
There was a problem hiding this comment.
Looks like we don't have the types for Temporal in the typescript setup. We should add them rather than cast as any.
Address review feedback from Sam Willis and CodeRabbit: - Add TemporalLike interface in db-ivm for type-safe Temporal detection - Make isTemporal a proper type guard (returns input is TemporalLike) - Remove as-any casts in hashTemporal - Add return type to createTemporalLike test helper - Remove unnecessary casts in join regression test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/tests/query/join.test.ts (1)
2094-2097: Strengthen post-update assertions to catch stale/duplicate rows.After the update, the test validates only the first row’s date. Add explicit cardinality and stale-value checks so duplicate/stale join rows can’t pass silently.
✅ Suggested test hardening
await flushPromises() - - expect(String(liveQuery.toArray[0]!.task.dueDate)).toBe(`2024-06-15`) + const rowsAfterUpdate = liveQuery.toArray + expect(rowsAfterUpdate).toHaveLength(1) + expect(String(rowsAfterUpdate[0]!.task.dueDate)).toBe(`2024-06-15`) + expect( + rowsAfterUpdate.some( + (row) => String(row.task.dueDate) === `2024-01-15`, + ), + ).toBe(false)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 the current code and only fix it if needed. In `@packages/db/tests/query/join.test.ts` around lines 2094 - 2097, The assertion after await flushPromises() only checks the first element and can miss stale/duplicate join rows; update the test that references liveQuery and its toArray result to assert the expected cardinality (e.g., exact length), verify each element’s task.dueDate equals the expected value (`2024-06-15`), and ensure there are no duplicate task ids or stale values (compare task.id or serialize each row to confirm uniqueness) so stale/duplicate join rows cannot pass silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/db/tests/query/join.test.ts`:
- Around line 2094-2097: The assertion after await flushPromises() only checks
the first element and can miss stale/duplicate join rows; update the test that
references liveQuery and its toArray result to assert the expected cardinality
(e.g., exact length), verify each element’s task.dueDate equals the expected
value (`2024-06-15`), and ensure there are no duplicate task ids or stale values
(compare task.id or serialize each row to confirm uniqueness) so stale/duplicate
join rows cannot pass silently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4b4060a-9f27-4542-a67a-8e7a1a85772a
📒 Files selected for processing (4)
packages/db-ivm/src/hashing/hash.tspackages/db-ivm/tests/utils.test.tspackages/db/src/utils.tspackages/db/tests/query/join.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db-ivm/src/hashing/hash.ts
- packages/db-ivm/tests/utils.test.ts
Add temporal-polyfill as devDependency to db-ivm and replace createTemporalLike mocks with real Temporal.PlainDate, PlainTime, PlainDateTime, and Instant objects in hash tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db-ivm/tests/utils.test.ts (1)
174-207: Thorough test coverage for Temporal hashing invariants.The tests correctly validate:
- Same Temporal values produce identical hashes (lines 184, 198, 205)
- Different Temporal values produce different hashes (lines 185, 199, 206)
- Different Temporal types with overlapping ISO strings produce distinct hashes (line 191) — critical for preventing collisions between
PlainDateandPlainDateTimeConsider adding an edge case test to verify Temporal objects hash differently from plain strings with identical representations:
🧪 Optional: Add Temporal vs string collision test
expect(hash(instant1)).not.toBe(hash(instant3)) + + // Temporal objects should hash differently from plain strings with same representation + const temporalDate = Temporal.PlainDate.from(`2024-01-15`) + const plainString = `2024-01-15` + expect(hash(temporalDate)).not.toBe(hash(plainString)) })Based on learnings: "Test corner cases including: empty collections, single elements, undefined vs null..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db-ivm/tests/utils.test.ts` around lines 174 - 207, Add a test asserting Temporal objects do not collide with plain strings that share their ISO representation: in the same test block that exercises hash(Temporal.PlainDate), hash(Temporal.PlainDateTime), hash(Temporal.Instant), add assertions that hash(Temporal.PlainDate.from("2024-01-15")) !== hash("2024-01-15") and similarly for Temporal.PlainDateTime/Temporal.Instant compared to their ISO string forms using the existing hash function to ensure Temporal types are distinguished from raw strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/db-ivm/tests/utils.test.ts`:
- Around line 174-207: Add a test asserting Temporal objects do not collide with
plain strings that share their ISO representation: in the same test block that
exercises hash(Temporal.PlainDate), hash(Temporal.PlainDateTime),
hash(Temporal.Instant), add assertions that
hash(Temporal.PlainDate.from("2024-01-15")) !== hash("2024-01-15") and similarly
for Temporal.PlainDateTime/Temporal.Instant compared to their ISO string forms
using the existing hash function to ensure Temporal types are distinguished from
raw strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93ce6a03-8f7e-46cc-bf89-fbefeb20d8fc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/db-ivm/package.jsonpackages/db-ivm/tests/utils.test.ts
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
3 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.
Temporal objects (e.g.
Temporal.PlainDate) break live query updates when joins are involved. Simple queries without joins work fine.Incorporates #1368 by @goatrenterguy (Ben Guericke) which provides the core hash fix and tests.
Root Cause
The structural hash function in
db-ivmusesObject.keys()to hash objects. Temporal objects store data in non-enumerable internal slots, soObject.keys()returns[]— every Temporal value hashes identically. When a Temporal field changes on a row in a join index,hash(oldRow) === hash(newRow), the-1and+1multiplicities cancel out, and the update is silently swallowed.Approach
Three related gaps in Temporal support along the join/query pipeline:
1. Hashing (
db-ivm) — The primary bug (from #1368). Detect Temporal objects viaSymbol.toStringTagand hash byTEMPORAL_MARKER + typeTag + toString()instead of falling through tohashPlainObject. Follows the same pattern used forDate(marker + value representation).2. Value normalization (
db) —normalizeValue()converts special types to primitives forMapkey equality in join key matching. Added Temporal → string conversion (__temporal__${tag}__${toString}), following the existingDate → getTime()andUint8Array → __u8__patterns.3. Comparator (
db) —ascComparator()had no Temporal handling, soORDER BYon Temporal fields would sort by arbitrary object reference IDs. Added string-based comparison viatoString(), which produces lexicographically sortable ISO 8601 strings for date/time types.Also made
isTemporal()defensive (null guard on the exported API) and converted thetemporalTypeslist fromArraytoSetfor consistency with thedb-ivmcopy.Key Invariants
MaplookupNon-goals
temporalTypes/isTemporaldefinitions acrossdbanddb-ivmpackages (would require cross-package dependency changes)Temporal.Durationsort ordering (durations aren't lexicographically sortable viatoString()— a separate concern)Verification
Files Changed
packages/db-ivm/src/hashing/hash.tsTEMPORAL_MARKER,temporalTypes,isTemporal(),hashTemporal()packages/db-ivm/tests/utils.test.tspackages/db/tests/query/join.test.tspackages/db/src/utils/comparison.tsnormalizeValue()andascComparator()packages/db/src/utils.tsisTemporal(), converttemporalTypesto Set.changeset/fix-temporal-join-hashing.mdFixes #1367
Incorporates #1368
🤖 Generated with Claude Code
Summary by CodeRabbit