feat(offline-transactions): confirm writes off the serial drain path (confirmWrite hook)#1603
Open
TomasGonzalez wants to merge 1 commit into
Open
Conversation
Add an opt-in `OfflineConfig.confirmWrite` hook that holds a just-committed write's optimistic state through the post-commit confirmation window, off the serial drain path, so waiting for an async sync stream (e.g. Electric's awaitTxId) no longer throttles drain throughput and no longer flickers rows. - Factor the hold create/release primitive into executor/OptimisticHold.ts and reuse it in restoreOptimisticState. - Add maxConfirmationHolds cap + getActiveConfirmationHoldCount() diagnostic; release holds on clear()/dispose(). - Thread the mutationFn result through to the completion promise and the hook (previously awaited and discarded). - Fully opt-in: with no confirmWrite, behavior is unchanged. Closes TanStack#1602 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds an opt-in ChangesconfirmWrite hook: contracts, primitives, executor wiring, and tests
Sequence DiagramsequenceDiagram
participant App
participant OfflineExecutor
participant TransactionExecutor
participant OptimisticHold
participant confirmWrite hook
rect rgba(100, 149, 237, 0.5)
Note over OfflineExecutor, TransactionExecutor: Serial drain path
App->>OfflineExecutor: mutate(data)
OfflineExecutor->>TransactionExecutor: runMutationFn(tx)
TransactionExecutor-->>OfflineExecutor: result (mutationFn return value)
TransactionExecutor->>TransactionExecutor: resolveWithOptionalConfirmation(tx, result)
TransactionExecutor->>OptimisticHold: createOptimisticHold(mutations)
OptimisticHold-->>TransactionExecutor: hold (overlay painted)
TransactionExecutor->>OfflineExecutor: resolveTransaction (outbox entry removed)
end
rect rgba(144, 238, 144, 0.5)
Note over TransactionExecutor, confirmWrite hook: Off-drain confirmation path
TransactionExecutor->>confirmWrite hook: confirmWrite({ id, mutations, result })
confirmWrite hook-->>TransactionExecutor: settles (resolve or reject)
TransactionExecutor->>OptimisticHold: release() / release({ rollback: true })
OptimisticHold-->>App: overlay dropped, collection updates
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 1
🧹 Nitpick comments (2)
packages/offline-transactions/src/types.ts (1)
103-103: ⚡ Quick winUse
unknownforConfirmWriteContext.metadatainstead ofany.Line 103 introduces a new public API field typed as
Record<string, any>, which drops type safety at the boundary and propagates unchecked values downstream.Suggested patch
- metadata?: Record<string, any> + metadata?: Record<string, unknown>As per coding guidelines, "Avoid using
anytypes; 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/offline-transactions/src/types.ts` at line 103, Replace the `any` type with `unknown` in the ConfirmWriteContext.metadata field definition. Change the metadata field from Record<string, any> to Record<string, unknown> to maintain type safety at the public API boundary and align with coding guidelines that avoid using any types.Source: Coding guidelines
packages/offline-transactions/tests/confirm-write.test.ts (1)
149-171: ⚡ Quick winAdd a regression assertion that
confirmWritestill executes when hold creation is skipped.This test currently validates only the hold count/overlay behavior. It should also assert that the hook is invoked under
maxConfirmationHolds: 0, so a contract regression (hook suppressed) is caught.Suggested patch
it(`skips the hold past maxConfirmationHolds (O(n^2) safety valve)`, async () => { const gate = deferred() + let confirmCalls = 0 const config: Partial<OfflineConfig> = { - confirmWrite: () => gate.promise, + confirmWrite: () => { + confirmCalls += 1 + return gate.promise + }, maxConfirmationHolds: 0, } @@ expect(env.executor.getActiveConfirmationHoldCount()).toBe(0) expect(env.collection.get(`item-1`)).toBeUndefined() + expect(confirmCalls).toBe(1)As per coding guidelines, "
**/*.test.{ts,tsx,js}: Always add unit tests that reproduce a bug before fixing it to ensure the bug is fixed and prevent regression."🤖 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/confirm-write.test.ts` around lines 149 - 171, The test currently only validates that no hold is created when maxConfirmationHolds is 0, but does not verify that the confirmWrite hook is still invoked. Add a spy or flag to track whether the confirmWrite callback executes, then add an assertion before gate.resolve() to verify that confirmWrite was indeed called despite the hold being skipped. This ensures the hook contract is maintained even when hold creation is bypassed due to the maxConfirmationHolds cap.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/executor/TransactionExecutor.ts`:
- Around line 200-219: The confirmWrite callback is being silently skipped due
to early returns at lines 202 and 213 when holds are capped or hold creation
fails, which violates the post-commit hook contract. Restructure the logic in
the method body to ensure confirmWrite is always invoked, regardless of hold
availability. Additionally, the maxConfirmationHolds assignment does not
normalize NaN or negative config values, which can unexpectedly bypass or
disable the cap. Add validation to ensure maxConfirmationHolds is a positive
integer, using something like Math.max to ensure proper normalization before the
size check is performed.
---
Nitpick comments:
In `@packages/offline-transactions/src/types.ts`:
- Line 103: Replace the `any` type with `unknown` in the
ConfirmWriteContext.metadata field definition. Change the metadata field from
Record<string, any> to Record<string, unknown> to maintain type safety at the
public API boundary and align with coding guidelines that avoid using any types.
In `@packages/offline-transactions/tests/confirm-write.test.ts`:
- Around line 149-171: The test currently only validates that no hold is created
when maxConfirmationHolds is 0, but does not verify that the confirmWrite hook
is still invoked. Add a spy or flag to track whether the confirmWrite callback
executes, then add an assertion before gate.resolve() to verify that
confirmWrite was indeed called despite the hold being skipped. This ensures the
hook contract is maintained even when hold creation is bypassed due to the
maxConfirmationHolds cap.
🪄 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: a618729c-330c-4d17-ba1b-7fa34d594a50
📒 Files selected for processing (7)
.changeset/confirm-write-hook.mdpackages/offline-transactions/src/OfflineExecutor.tspackages/offline-transactions/src/executor/OptimisticHold.tspackages/offline-transactions/src/executor/TransactionExecutor.tspackages/offline-transactions/src/types.tspackages/offline-transactions/tests/confirm-write.test.tspackages/offline-transactions/tests/offline-e2e.test.ts
Comment on lines
+200
to
+219
| const maxHolds = | ||
| this.config.maxConfirmationHolds ?? DEFAULT_MAX_CONFIRMATION_HOLDS | ||
| if (this.confirmationHolds.size >= maxHolds) { | ||
| // Safety valve: too many concurrent holds. Skip the hold — the optimistic | ||
| // overlay drops at resolve as it would without the hook. The write is | ||
| // already durably committed, so correctness is unaffected. | ||
| this.offlineExecutor.resolveTransaction(transaction.id, result) | ||
| return | ||
| } | ||
|
|
||
| const hold = this.createConfirmationHold(transaction.mutations) | ||
| this.offlineExecutor.resolveTransaction(transaction.id, result) | ||
|
|
||
| if (!hold) { | ||
| // Hold creation failed (already logged). The write is committed; just let | ||
| // the optimistic state drop as it did before the hook existed. | ||
| return | ||
| } | ||
|
|
||
| this.runConfirmation(confirmWrite, transaction, result, hold) |
Contributor
There was a problem hiding this comment.
confirmWrite is silently skipped when holds are capped or hold creation fails.
At Line 202 and Line 213, the function returns before invoking confirmWrite. That breaks the post-commit hook contract and makes behavior depend on hold availability instead of hook configuration. Also, Line 201 uses maxConfirmationHolds without normalizing NaN/negative values, which can bypass or disable the cap unexpectedly.
Suggested patch
- const maxHolds =
- this.config.maxConfirmationHolds ?? DEFAULT_MAX_CONFIRMATION_HOLDS
- if (this.confirmationHolds.size >= maxHolds) {
- // Safety valve: too many concurrent holds. Skip the hold — the optimistic
- // overlay drops at resolve as it would without the hook. The write is
- // already durably committed, so correctness is unaffected.
- this.offlineExecutor.resolveTransaction(transaction.id, result)
- return
- }
-
- const hold = this.createConfirmationHold(transaction.mutations)
+ const configuredMax = this.config.maxConfirmationHolds
+ const maxHolds =
+ Number.isFinite(configuredMax) && (configuredMax as number) >= 0
+ ? Math.floor(configuredMax as number)
+ : DEFAULT_MAX_CONFIRMATION_HOLDS
+ const hold =
+ this.confirmationHolds.size < maxHolds
+ ? this.createConfirmationHold(transaction.mutations)
+ : null
this.offlineExecutor.resolveTransaction(transaction.id, result)
-
- if (!hold) {
- // Hold creation failed (already logged). The write is committed; just let
- // the optimistic state drop as it did before the hook existed.
- return
- }
-
this.runConfirmation(confirmWrite, transaction, result, hold)
}
@@
- hold: OptimisticHold,
+ hold: OptimisticHold | null,
): void {
const release = (): void => {
- this.confirmationHolds.delete(hold)
- try {
- hold.release()
- } catch (error) {
- console.warn(`Failed to release confirmation hold:`, error)
+ if (hold) {
+ this.confirmationHolds.delete(hold)
+ try {
+ hold.release()
+ } catch (error) {
+ console.warn(`Failed to release confirmation hold:`, error)
+ }
}
}Also applies to: 237-270
🤖 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/src/executor/TransactionExecutor.ts` around
lines 200 - 219, The confirmWrite callback is being silently skipped due to
early returns at lines 202 and 213 when holds are capped or hold creation fails,
which violates the post-commit hook contract. Restructure the logic in the
method body to ensure confirmWrite is always invoked, regardless of hold
availability. Additionally, the maxConfirmationHolds assignment does not
normalize NaN or negative config values, which can unexpectedly bypass or
disable the cap. Add validation to ensure maxConfirmationHolds is a positive
integer, using something like Math.max to ensure proper normalization before the
size check is performed.
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.
Closes #1602
🎯 Changes
Adds an opt-in
OfflineConfig.confirmWritehook to@tanstack/offline-transactionsthat keeps a just-committed write's optimistic state painted across the post-commit confirmation window — running off the serial drain path.Motivation
The executor drains the outbox serially (one
mutationFnat a time), which is what preserves create-then-update / FK ordering. But there's no built-in way to keep a row painted between "the server committed the write" and "the sync stream echoed it back". The only option today is toawaitthat confirmation inside themutationFn— e.g.:Because the drain is serial, that
awaitblocks the next write. With an ElectricSQL shape stream whoseawaitTxIdbudget is ~10s, throughput collapses to ~1 write / 10s and each settled write tends to spawn a duplicate "already-applied" resend. The alternative — dropping theawait— makes rows flicker (disappear then reappear) in the commit→sync gap, because the executor drops the transaction's optimistic overlay the instantmutationFnresolves.What this adds
confirmWriteruns after the write commits and its outbox entry is removed, but off the serial path — it never blocks the nextmutationFn:While the returned promise is pending, the library keeps the committed mutations' optimistic overlay painted via an internal hold transaction — the same primitive
restoreOptimisticStatealready uses to re-show pending writes after a reload — then releases it when the hook settles. The serial chain still serializes the POSTs (ordering preserved); only the confirmation moved off it.Design points:
confirmWritejust releases the overlay early (a possible brief flicker), never data loss. Any timeout / verify-by-state logic lives inside the hook.resolveTransactiondrops the original overlay, so the overlay is owned continuously.maxConfirmationHolds(default1000) caps concurrent holds to avoid O(n²) optimistic-recompute churn on a large, fast drain; beyond the cap the hold is skipped (degrades to today's behavior).getActiveConfirmationHoldCount()exposes the live count.clear()anddispose().Internals: the hold create/release pattern is factored into a new
executor/OptimisticHold.tsand reused byrestoreOptimisticState, so there's a single primitive. The hook is fully opt-in — with noconfirmWriteconfigured, behavior is byte-for-byte unchanged.Drive-by fix
runMutationFnpreviouslyawaited themutationFnand discarded its return value, sowaitForTransactionCompletionalways resolvedundefined. The result is now threaded through to the completion promise and toconfirmWrite(so the hook can read e.g. a server txid). One existing e2e assertion was updated to reflect this.Context
This generalizes a pattern we shipped app-side at Autarc (an external
offlineConfirmationHolds.tsthat reaches intocollection._stateand re-implements the library's private hold/teardown). With this hook, that ~360-line module collapses to the smallconfirmWritecallback above. Happy to bikeshed the name (confirmWrite/awaitSync/confirmTransaction) and the default cap.✅ Checklist
pnpm test. (@tanstack/offline-transactions: 71 passing + 6 new intests/confirm-write.test.ts, 1 pre-existing skip; full suite green.eslint srcandprettier --checkclean.tscintroduces zero new errors vsmain.)🚀 Release Impact
.changeset/confirm-write-hook.md,minor).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
confirmWritehook for post-commit confirmation workflowsmaxConfirmationHoldsconfiguration to limit concurrent confirmation overlaysgetActiveConfirmationHoldCount()for monitoring active holdsTests