Skip to content

Comments

Repro of #1922#2209

Open
dfahlander wants to merge 2 commits intomasterfrom
issue1922
Open

Repro of #1922#2209
dfahlander wants to merge 2 commits intomasterfrom
issue1922

Conversation

@dfahlander
Copy link
Collaborator

@dfahlander dfahlander commented Nov 1, 2025

This PR is currently failing as it reproduces issue #1922. When the bug is solved in dexie.cloud service, please rerun the tests.

Summary by CodeRabbit

  • Tests
    • Expanded Dexie Cloud test suite with new scenarios covering open/sync/modify/validate flows
    • Added scaffolding for deterministic sync tests and improved data normalization before assertions
    • Standardized transaction callbacks and formatting for clearer, more reliable tests
    • Strengthened assertions and synchronization checks across multiple scenarios

@coderabbitai
Copy link

coderabbitai bot commented Nov 1, 2025

Walkthrough

Removed unused public imports from the Dexie Cloud test file and added/expanded test scaffolding that opens, syncs, modifies, and validates data (issue #1922). Also reformatted and adjusted several Dexie Cloud test sequences (issue #2185) and standardized transaction callback signatures.

Changes

Cohort / File(s) Change Summary
Import/API cleanup
addons/dexie-cloud/test/unit/tests-github-issues.ts
Removed DexieCloudOptions and getTiedRealmId from the public import surface; now imports only dexieCloud and DexieCloudTable.
Test scaffolding and formatting
addons/dexie-cloud/test/unit/tests-github-issues.ts
Added DEXIE_CLOUD_PROPS constant and a promised test exercising open→sync→modify→validate flow for issue #1922; expanded/reformatted tests for issue #2185 with consistent (tx) transaction callback parameters, trailing commas, adjusted async handling, minor test-data and assertion updates, and a runtime strip helper usage.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Test as Tests (unit)
  participant Client as dexie-cloud-client
  participant Realm as Local Realm/DB
  participant Server as Dexie Cloud Server

  Note over Test,Client: Test scaffolding opens client and triggers sync
  Test->>Client: open() / start sync
  Client->>Realm: apply local mutations (tx)
  Client->>Server: push local changes / request updates
  Server-->>Client: send remote updates / acks
  Client->>Realm: apply remote updates
  Realm-->>Test: confirm data state
  Note right of Test: Assertions and validations
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas needing extra attention:
    • The new promised test flow (open, sync, modify, validate) — verify correctness of async ordering and assertions.
    • Any implicit assumptions in the strip helper usage and commented alternate implementation.
    • Confirm that removing DexieCloudOptions and getTiedRealmId from the import reflects intended public API surface (tests compile/run without those symbols).

Poem

🐰 I hopped into tests with a curious mind,
Cleared old imports and tidied the bind.
I synced and I hopped from local to sky,
Watching data dance, then winked an eye —
Tiny rabbit cheers for tests refined! 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Repro of #1922" directly and accurately describes the main change in the pull request. According to the raw summary, the primary modification is adding new test scaffolding for Dexie issue #1922, including a new test case that exercises opening, syncing, modifying, and validating data. The title is concise, clear, and specific—it references the issue number in a way that makes the purpose immediately understandable to reviewers. While the changeset includes some additional adjustments (such as test reformatting for issue #2185 and import changes), these are secondary to the main objective of reproducing issue #1922.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ 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 issue1922

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5509f8 and 4d4dd71.

📒 Files selected for processing (1)
  • addons/dexie-cloud/test/unit/tests-github-issues.ts (3 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)

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

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 761a933 and f5509f8.

📒 Files selected for processing (1)
  • addons/dexie-cloud/test/unit/tests-github-issues.ts (3 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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: test (addons/y-dexie/test)
  • GitHub Check: test (libs/dexie-react-hooks/test)
  • GitHub Check: test (addons/dexie-export-import/test)
  • GitHub Check: test (addons/Dexie.Syncable/test)
  • GitHub Check: test (test)
  • GitHub Check: test (addons/Dexie.Observable/test)

Comment on lines +102 to 106
await db.transaction('rw', db.items2185, db.members, db.realms, (tx) => {
tx.items2185.clear();
tx.members.where({ realmId: REALM_ID }).delete();
tx.realms.delete(REALM_ID);
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix transaction table access

Dexie.Transaction doesn’t expose tables as direct properties, so tx.items2185/tx.members/tx.realms are undefined. The first call to tx.items2185.clear() throws, breaking both the initial cleanup and the final cleanup for this test. Use the already-bound table instances from db (or tx.table('items2185')) instead.

-  await db.transaction('rw', db.items2185, db.members, db.realms, (tx) => {
-    tx.items2185.clear();
-    tx.members.where({ realmId: REALM_ID }).delete();
-    tx.realms.delete(REALM_ID);
+  await db.transaction('rw', db.items2185, db.members, db.realms, () => {
+    db.items2185.clear();
+    db.members.where({ realmId: REALM_ID }).delete();
+    db.realms.delete(REALM_ID);
   });
@@
-  await db.transaction('rw', db.items2185, db.members, db.realms, (tx) => {
-    tx.items2185.clear();
-    tx.members.where({ realmId: REALM_ID }).delete();
-    tx.realms.delete(REALM_ID);
+  await db.transaction('rw', db.items2185, db.members, db.realms, () => {
+    db.items2185.clear();
+    db.members.where({ realmId: REALM_ID }).delete();
+    db.realms.delete(REALM_ID);
   });

Also applies to: 169-173

🤖 Prompt for AI Agents
In addons/dexie-cloud/test/unit/tests-github-issues.ts around lines 102-106 (and
similarly at 169-173), the transaction callback incorrectly accesses tables via
tx.items2185/tx.members/tx.realms which are undefined; use the already-bound db
table instances or tx.table('tableName') instead. Update the transaction to call
operations on db.items2185, db.members.where(...).delete(), and
db.realms.delete(REALM_ID) (or replace each tx.X with tx.table('X')) so the
transaction uses valid Table objects and no longer throws on the first access.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant