Issue2228

[dfahlander]

Repro and fix #2228 (discussion #2226)

Summary by CodeRabbit

• Bug Fixes

  • Resolved issue with update operations containing property modifications on non-existing items.

• Tests

  • Added comprehensive test coverage for cloud sync operations.
  • Enhanced test infrastructure with additional testing tools.

• Chores

  • Updated demo cloud environment configuration.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Fixed handling of PropModifications (add/remove operations) in cloud sync mutations by adding guards in listClientChanges.ts to prevent invalid optimization, added test case for issue #2228, updated test configuration, and added demo user entry.

Changes

Cohort / File(s)	Summary
Cloud sync mutation handling addons/dexie-cloud/src/sync/listClientChanges.ts	Added PropModification guards to skip optimization of mutations containing add/remove operations; changed key coverage retrieval to safely handle missing coverage.
Test infrastructure addons/dexie-cloud/test/unit/karma.conf.cjs, addons/dexie-cloud/package.json	Added explicit plugins array to Karma config for local testing; added karma-webdriver-launcher devDependency.
Test case and data addons/dexie-cloud/test/unit/tests-github-issues.ts, addons/dexie-cloud/dexie-cloud-import.json	Added new test for issue #2228 covering cloud sync with PropModifications; added [email protected] demo user.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• listClientChanges.ts: Logic change affecting mutation optimization requires verification that PropModification guards are correctly placed and don't over-restrict optimization
• tests-github-issues.ts: New test case should be verified for correct cloud sync flow and adequate coverage of the issue scenario
• karma.conf.cjs: Verify that explicit plugins list matches original implicit behavior

Poem

🐰 PropMods were causing sync mutations to break,
Now guards protect them for safety's sake,
Tests confirm the fix runs clean and bright,
Issue 2228 dances in the light! 🌟

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Issue2228' is vague and lacks specificity about the actual change being made.	Use a more descriptive title that explains the fix, such as 'Fix: Prevent invalid sync mutations with PropModifications in update()' or 'Fix PropModification handling in dexie-cloud sync mutations'.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	The PR addresses the core objectives from issue #2228: reproducing and fixing invalid sync mutations when using PropModifications (add/remove) in Table.update().
Out of Scope Changes check	✅ Passed	All changes are scoped to the issue: test reproduction, sync logic fix in listClientChanges.ts, Karma configuration updates for testing, and demo data setup.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch issue2228

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

addons/dexie-cloud/package.json (1)

89-89: Consider pinning the version for reproducible builds.

Using "*" for karma-webdriver-launcher may lead to inconsistent test behavior across environments. Given that this dependency is explicitly excluded in the local karma config (as seen in karma.conf.cjs), this appears intentional for CI/remote testing scenarios, but a pinned version would ensure consistency.

addons/dexie-cloud/test/unit/tests-github-issues.ts (2)

16-17: Remove duplicate DBURL declaration.

The DBURL constant is now declared at the top level here, but line 73 in the issue-2185 test still has a local declaration of the same value. Consider removing the local declaration in issue-2185 to avoid redundancy.

🔎 Proposed fix in the issue-2185 test

 promisedTest('https://github.com/dexie/Dexie.js/issues/2185', async () => {
   const DBNAME = 'issue2185';
-  const DBURL = 'https://zv8n7bwcs.dexie.cloud'; // Shall exist in cloud.
   const DEMOUSER1 = '[email protected]'; // This user is imported into the cloud database using `npx dexie-cloud import dexie-cloud-import.json`

---

43-51: Consider verifying the final item state after sync.

The test confirms sync operations complete, but doesn't verify the item's final state contains the expected drinks array. Given that issue #2228 was about invalid sync mutations, asserting the final data would strengthen the test.

🔎 Suggested verification

  await db.cloud.sync({purpose: 'push', wait: true });

+ const finalItem = await db.items2228.get(itemId);
+ deepEqual(finalItem?.drinks, ['coffee', 'tea'], 'Item drinks array is correct after add/remove operations');
+
  ok(true, 'Test completed successfully');

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c4d820 and f185bdb.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (5)

• addons/dexie-cloud/dexie-cloud-import.json (1 hunks)
• addons/dexie-cloud/package.json (1 hunks)
• addons/dexie-cloud/src/sync/listClientChanges.ts (1 hunks)
• addons/dexie-cloud/test/unit/karma.conf.cjs (1 hunks)
• addons/dexie-cloud/test/unit/tests-github-issues.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

addons/dexie-cloud/test/unit/tests-github-issues.ts (1)

addons/dexie-cloud/src/dexie-cloud-client.ts (1)

• dexieCloud (76-501)

🔇 Additional comments (5)

addons/dexie-cloud/dexie-cloud-import.json (1)

4-5: LGTM!

The new demo user entry follows the existing naming convention and aligns with the test case in tests-github-issues.ts that uses [email protected] for authentication.

addons/dexie-cloud/src/sync/listClientChanges.ts (2)

80-82: LGTM! Good defensive check for PropModification.

This correctly prevents PropModification entries from being added to the coverage map, ensuring that add/remove operations are not considered for redundancy optimization.

---

95-106: LGTM! Proper guards ensure PropModification mutations are preserved.

The duplicate PropModification check (lines 98-100) in the filter pass is necessary because mutations not added to the coverage map in the first pass still need to be explicitly retained here. The added null check on line 106 is a good defensive addition that handles edge cases where keyCoverage may not exist.

addons/dexie-cloud/test/unit/tests-github-issues.ts (1)

37-41: Good edge case coverage for non-existing items.

Testing add/remove operations on a non-existing item is valuable - this ensures the sync logic handles orphaned mutations gracefully without crashing.

addons/dexie-cloud/test/unit/karma.conf.cjs (1)

24-31: No action needed. The explicit plugins array correctly references karma-mocha-reporter, which is available as a devDependency in the root workspace package.json of this monorepo.