fix: preserve Temporal values in offline transactions#1579
Open
cdemetriou-prod wants to merge 4 commits into
Open
fix: preserve Temporal values in offline transactions#1579cdemetriou-prod wants to merge 4 commits into
cdemetriou-prod wants to merge 4 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTransactionSerializer now encodes Temporal-like values as markers and reconstructs them via injected or global Temporal constructors; metadata is included in the round-trip. OutboxManager and OfflineExecutor accept optional temporal constructors and propagate MissingTemporalConstructorError. Types, tests, and a devDependency/changeset were added. ChangesTemporal value preservation in offline transactions
Sequence DiagramsequenceDiagram
participant Client
participant OutboxManager
participant TransactionSerializer
participant TemporalConstructors
Client->>OutboxManager: request transaction (get/getAll)
OutboxManager->>TransactionSerializer: deserialize(serializedTransaction)
TransactionSerializer->>TransactionSerializer: detect Temporal marker in value
TransactionSerializer->>TemporalConstructors: resolve constructor for `type` (injected or globalThis.Temporal)
TransactionSerializer->>TemporalConstructors: call constructor.from(value)
TemporalConstructors->>TransactionSerializer: return Temporal instance
TransactionSerializer->>OutboxManager: return reconstructed OfflineTransaction
OutboxManager->>Client: return transaction / re-throw MissingTemporalConstructorError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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 |
![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: 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/offline-transactions/src/outbox/TransactionSerializer.ts`:
- Around line 50-65: TransactionSerializer currently reaches for
globalThis.Temporal via getTemporalConstructor (used by deserializeValue),
causing silent failures in OutboxManager.get; refactor to make the Temporal
dependency explicit by adding an injectable Temporal namespace or a mapping of
TemporalConstructorName->TemporalConstructor to TransactionSerializer (e.g., a
constructor parameter on TransactionSerializer) and update
getTemporalConstructor to use that injected object instead of
globalThis.Temporal; also update OutboxManager.get to either construct
TransactionSerializer with the injected Temporal (propagate the dependency from
wherever OutboxManager is created) or, if no Temporal is supplied, throw a clear
runtime error (fail fast) rather than swallowing the deserialization error and
returning null so persisted outbox items remain readable.
In `@packages/offline-transactions/tests/TransactionSerializer.test.ts`:
- Around line 323-345: The test currently seeds changes.temporalValues but only
asserts deserialized.mutations[0].modified and .original; add assertions that
inspect deserialized.changes.temporalValues (the same path used when creating
the transaction) and verify each entry's Symbol.toStringTag and toString() match
the original temporalValues entries; locate the test around
serializer.serialize/serializer.deserialize and the transaction object creation
and add checks analogous to the modified/original assertions but against
deserialized.changes.temporalValues to prevent regressions in the changes
branch.
🪄 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: 60e28cff-5dc9-4353-a259-c675f299e6c9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
.changeset/quiet-temporal-dragons.mdpackages/offline-transactions/package.jsonpackages/offline-transactions/src/outbox/TransactionSerializer.tspackages/offline-transactions/tests/TransactionSerializer.test.ts
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/offline-transactions/tests/TransactionSerializer.test.ts (1)
281-371: ⚡ Quick winAdd a negative-path test for missing Temporal constructors
Current Temporal tests only verify successful round-trips. Please add one assertion that deserializing a Temporal marker without injected constructors throws
MissingTemporalConstructorError, so this runtime contract doesn’t regress.➕ Minimal test addition
-import { TransactionSerializer } from '../src/outbox/TransactionSerializer' +import { + MissingTemporalConstructorError, + TransactionSerializer, +} from '../src/outbox/TransactionSerializer' @@ describe(`Temporal handling`, () => { + it(`should throw when Temporal constructors are unavailable`, () => { + const serializerWithTemporal = createSerializer() + const serializerWithoutTemporal = new TransactionSerializer({ + 'test-collection': mockCollection as any, + }) + + const transaction = createTransaction({ + modified: { + id: `1`, + dueDate: Temporal.PlainDate.from(`2024-01-15`), + }, + }) + + const serialized = serializerWithTemporal.serialize(transaction) + expect(() => serializerWithoutTemporal.deserialize(serialized)).toThrow( + MissingTemporalConstructorError, + ) + }) + it(`should restore Temporal values in mutation data`, () => {As per coding guidelines, "Always add unit tests that reproduce a bug before fixing it to ensure the bug is fixed and prevent regression" and "Test corner cases including ... undefined vs null values ...".
🤖 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/offline-transactions/tests/TransactionSerializer.test.ts` around lines 281 - 371, Add a negative-path test that ensures deserializing Temporal markers without injected constructors throws MissingTemporalConstructorError: create a serializer (via createSerializer) that does not have Temporal constructors injected, produce or reuse a serialized transaction containing Temporal markers (e.g., from serialize or a minimal prepared payload), and assert that calling serializer.deserialize(...) throws MissingTemporalConstructorError; reference serializer.deserialize, createSerializer, and MissingTemporalConstructorError in the test so it fails when the runtime contract is not met.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.
Inline comments:
In `@packages/offline-transactions/src/OfflineExecutor.ts`:
- Around line 283-287: When replaying persisted outbox entries in
loadAndReplayTransactions() the broad catch currently swallows
MissingTemporalConstructorError thrown by OutboxManager.getAll(), so update
loadAndReplayTransactions() to explicitly catch MissingTemporalConstructorError
(the error class by name) and rethrow it (or propagate it) instead of treating
it as a recoverable warning; keep the existing generic catch for other errors
but ensure MissingTemporalConstructorError is not swallowed so startup can fail
fast when temporals are missing.
---
Nitpick comments:
In `@packages/offline-transactions/tests/TransactionSerializer.test.ts`:
- Around line 281-371: Add a negative-path test that ensures deserializing
Temporal markers without injected constructors throws
MissingTemporalConstructorError: create a serializer (via createSerializer) that
does not have Temporal constructors injected, produce or reuse a serialized
transaction containing Temporal markers (e.g., from serialize or a minimal
prepared payload), and assert that calling serializer.deserialize(...) throws
MissingTemporalConstructorError; reference serializer.deserialize,
createSerializer, and MissingTemporalConstructorError in the test so it fails
when the runtime contract is not met.
🪄 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: ddb558b6-5105-4d68-8fab-0711c3364227
📒 Files selected for processing (5)
packages/offline-transactions/src/OfflineExecutor.tspackages/offline-transactions/src/outbox/OutboxManager.tspackages/offline-transactions/src/outbox/TransactionSerializer.tspackages/offline-transactions/src/types.tspackages/offline-transactions/tests/TransactionSerializer.test.ts
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/offline-transactions/tests/OfflineExecutor.test.ts`:
- Around line 108-135: The test currently calls executor.dispose() only on the
success path, risking leaked resources if expect(...waitForInit()) rejects; move
disposal into the finally block so the executor is always disposed. After
creating the executor via startOfflineExecutor(...) and before restoring
Temporal (originalTemporal/globalWithTemporal), ensure executor.dispose() is
invoked in the finally branch (guarding for executor being defined) so
startOfflineExecutor, executor.dispose, waitForInit, and
MissingTemporalConstructorError are handled safely and no listeners leak between
tests.
🪄 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: 408f1c0f-8b48-46bc-bb16-d31e71c99e97
📒 Files selected for processing (2)
packages/offline-transactions/src/OfflineExecutor.tspackages/offline-transactions/tests/OfflineExecutor.test.ts
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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
1 participant
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.
🎯 Changes
Preserves TC39 Temporal values when offline transactions are serialized to and restored from the outbox.
Previously, Temporal objects such as
Temporal.PlainDateandTemporal.Instantcould round-trip as{}because they do not expose enumerable fields. This updates the offline transaction serializer to store Temporal values with explicit type markers and restore them throughglobalThis.Temporal, covering mutation data and transaction metadata.Adds serializer tests for Temporal values in mutation
modified/original/changesdata and transactionmetadata.✅ Checklist
pnpm test.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores