fix(dexie-cloud): decode private key in changeSpecs for 'update' mutations#2277
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change modifies the 'update' mutation type handling in applyServerChanges.ts to remove primary key field values from each change spec before executing bulk updates. When applying inbound updates with a keyPath present, the code now iterates through change specs and deletes the primary-key field at the specified keyPath using Dexie.delByKeyPath before the bulkUpdate call. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@addons/dexie-cloud/src/sync/connectWebSocket.ts`:
- Around line 64-69: The current switchMap waits on
db.cloud.persistedSyncState.take(1) which can replay a stale BehaviorSubject
value; change it to first attempt a fresh read via liveQuery(() =>
db.getPersistedSyncState()) and only if that fresh value has no serverRevision
fall back to subscribing to db.cloud.persistedSyncState as a backup waiter. In
other words, in the switchMap replace the single
persistedSyncState.pipe(filter(...), take(1)) with logic that (a) uses
liveQuery(() => db.getPersistedSyncState()).pipe(filter(sync => !!(sync &&
sync.serverRevision)), take(1)) and (b) if that completes without a valid
revision, subscribes to db.cloud.persistedSyncState.pipe(filter(...), take(1))
so the fresh DB read is preferred and the BehaviorSubject is only a fallback.
In `@addons/dexie-cloud/src/sync/LocalSyncWorker.ts`:
- Around line 22-24: The code clears pullSignalled and pushSignalled before
scheduling a backoff, which loses the original purpose and causes a failed pull
to be retried as a push; update the retry logic in syncAndRetry() so the
original purpose (derived from pullSignalled/pushSignalled or the local variable
purpose) is captured and passed into the backoff callback (or stored on the
retry context) instead of relying on the flags, and ensure syncIfPossible() is
invoked with options.purpose set to that captured purpose; apply the same change
to the other occurrence that clears flags (the block around the second instance
mentioned, lines 58–61) so pull retries remain pull retries until success or a
new signal overrides them.
🪄 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: 374b873d-af53-4155-8ca7-1aa2d81ec6e5
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsamples/dexie-cloud-todo-app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
addons/dexie-cloud/src/sync/LocalSyncWorker.tsaddons/dexie-cloud/src/sync/applyServerChanges.tsaddons/dexie-cloud/src/sync/connectWebSocket.tssamples/dexie-cloud-todo-app/package.json
…tions The primary key should never be part of an updateSpec — it cannot change and is already communicated via the operation's keys array. For private singleton IDs (e.g. '#key:userId' on server, '#key' on client), the encoded server-side key may leak into the changeSpec via getObjectDiff(). This causes bulkUpdate() to throw 'Cannot change primary key' since the changeSpec key mismatches the decoded key from mut.keys. Strip the primary key unconditionally from all changeSpecs as a defensive measure, mirroring how insert/upsert already use Dexie.setByKeyPath to keep inbound keys consistent. Fixes #2279
8b682b1 to
2d0eacd
Compare
Problem
When the server sends an
update-type mutation for a record with an inbound private ID primary key (e.g.#ical-calendars-data), sync fails permanently withError: Cannot change primary key.Root cause: In
applyServerChanges.ts, theupdatecase decodesmut.keysviakeyDecoder(#key:userId→#key) but does not update the primary key field insidemut.changeSpecs. WhenbulkUpdate()compares them, they mismatch:The
insertandupsertcases already handle this correctly viaDexie.setByKeyPath.Fix
Before calling
bulkUpdate(), check if the primary key field is present in each changeSpec (usingDexie.getByKeyPathto correctly handle compound/nested keyPaths) and overwrite it with the decoded key.Testing
Reproduction:
settingtable withid: "#ical-calendars-data")update-type mutation (observed with large blob references)Reported by: @bennie.forss
Summary by CodeRabbit