Experiment 1: Explicitly commit IndexedDB write transactions - #2853
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesIndexedDB transaction commits
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
c14e0c0 to
647e3aa
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/packages/core/__tests__/src/utils/PersistedObject.test.ts (1)
432-439: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert one commit per write helper.
The aggregate assertion at Line 439 does not prove that
setItem,multiSet, andremoveItemeach callcommit()once. For example, two calls in one helper and no call in another would still pass. Clear the spy and assert one call after each awaited helper.Proposed test adjustment
await idb.setItem('key1', 'value1'); + expect(commitSpy).toHaveBeenCalledTimes(1); + commitSpy.mockClear(); await idb.multiSet([ ['key2', 'value2'], ['key3', 'value3'], ]); + expect(commitSpy).toHaveBeenCalledTimes(1); + commitSpy.mockClear(); await idb.removeItem('key3'); - expect(commitSpy).toHaveBeenCalledTimes(3); + expect(commitSpy).toHaveBeenCalledTimes(1);🤖 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 `@client/packages/core/__tests__/src/utils/PersistedObject.test.ts` around lines 432 - 439, Update the test around setItem, multiSet, and removeItem to clear commitSpy before each awaited helper, then assert it was called exactly once immediately afterward. Remove the aggregate commitSpy assertion so each write helper’s individual commit behavior is verified.
🤖 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.
Nitpick comments:
In `@client/packages/core/__tests__/src/utils/PersistedObject.test.ts`:
- Around line 432-439: Update the test around setItem, multiSet, and removeItem
to clear commitSpy before each awaited helper, then assert it was called exactly
once immediately afterward. Remove the aggregate commitSpy assertion so each
write helper’s individual commit behavior is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 132c8cb6-2b2c-4189-9f50-8f1f37bc9699
📒 Files selected for processing (2)
client/packages/core/__tests__/src/utils/PersistedObject.test.tsclient/packages/core/src/IndexedDBStorage.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- client/packages/core/src/IndexedDBStorage.ts
|
View Vercel preview at instant-www-js-codex-indexeddb-explicit-commit-jsv.vercel.app. |
Problem
If you background a tab in android, sometimes it causes active tabs to freeze
Reason
We open a transaction on IndexedDB when we save pending mutations. If Chrome decides to freeze while the tx is open, all other transactions to IndexedDB will be blocked, waiting for that transaction
Solution
Unfortunately there is no easy way to tell IndexedDB to time out when we are sending a tx.
But one thing we can do is to commit the transaction faster:
We call
IDBTransaction.commit()aftersetItem,multiSet, andremoveItemright away.This lets IDB commit the transaction, before having to send back a "done" result to our callback.
When running tests, this reduced the number of noticed blocks from 4/6 to 1/6
Detailed explainer: https://instant-frozen-tab-idb.stopa.chatgpt.site
Future Work
This still may not be enough. It's still possible that we try to do a very large write (with lots of pending mutations), and just at that moment Android freezes us.
To avoid this, we would have to start saving each transaction as a separate row, so we could start to bound writes to pendingMutations.
This may be a bit more involved, so I wanted to first ship this version, before experimenting with a full on rewrite.
@dwwoelfel @nezaj