Fix OPFS sync metadata recovery#570
Conversation
📝 WalkthroughWalkthroughThe PR improves sync collection replication resilience by introducing recovery logic for corrupted local metadata. A new recovery function conditionally removes affected collections and reloads the app when storage errors occur, integrated into the remote count stream to allow graceful degradation when queries fail. ChangesSync Collection Recovery
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e371f5b8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const sessionKey = `${SYNC_RECOVERY_SESSION_KEY_PREFIX}${collection.name}`; | ||
| const sessionStorage = getSessionStorage(); | ||
| if (sessionStorage?.getItem(sessionKey) === '1') { |
There was a problem hiding this comment.
When the same collection name is recovered once, this session key blocks recovery for every other store database in the tab because it only includes collection.name; createFastStoreDB() creates distinct fast_store_v5_${store.localID} databases, so switching stores after recovering (for example) templates in one store will make a later templates OPFS corruption in another store return false and surface the count error instead of resetting. Include the collection's database name (or another store/database identifier) in the guard key so the one-shot reload protection is per corrupted DB, not global per collection name.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/query/tests/logs-recovery.test.ts (1)
67-86: ⚡ Quick winConsider adding a test for sync collection removal failure.
The test suite includes a case for logs collection removal failure (line 115), which verifies the session flag is not set when
remove()throws. Adding an equivalent test forrecoverSyncCollectionStoragewould ensure consistent error handling and prevent regressions.Suggested test case
it('does not set the session flag when sync collection removal fails', async () => { const removeError = new Error('remove failed'); const remove = jest.fn(async () => { throw removeError; }); const syncCollection = { name: 'templates', remove }; const reload = jest.fn(); await expect( recoverSyncCollectionStorage( syncCollection, new Error( 'could not requestRemote: {"methodName":"count","error":{"name":"SyntaxError","message":"Unexpected token"}}' ), { reload } ) ).rejects.toThrow(removeError); expect(remove).toHaveBeenCalledTimes(1); expect( globalThis.sessionStorage.getItem('wcpos_sync_storage_recovery_attempted_templates') ).toBeNull(); expect(reload).not.toHaveBeenCalled(); });🤖 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/query/tests/logs-recovery.test.ts` around lines 67 - 86, Add a new test in packages/query/tests/logs-recovery.test.ts that mirrors the logs removal-failure case but targets recoverSyncCollectionStorage: create a remove mock that throws a specific Error (e.g., removeError), construct syncCollection = { name: 'templates', remove } and a jest fn reload, then call recoverSyncCollectionStorage(...) wrapped in await expect(...).rejects.toThrow(removeError); assert remove was called once, that globalThis.sessionStorage.getItem('wcpos_sync_storage_recovery_attempted_templates') is null (i.e., no session flag set), and that reload was not called; name the test like "does not set the session flag when sync collection removal fails".
🤖 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 `@packages/query/tests/logs-recovery.test.ts`:
- Around line 67-86: Add a new test in
packages/query/tests/logs-recovery.test.ts that mirrors the logs removal-failure
case but targets recoverSyncCollectionStorage: create a remove mock that throws
a specific Error (e.g., removeError), construct syncCollection = { name:
'templates', remove } and a jest fn reload, then call
recoverSyncCollectionStorage(...) wrapped in await
expect(...).rejects.toThrow(removeError); assert remove was called once, that
globalThis.sessionStorage.getItem('wcpos_sync_storage_recovery_attempted_templates')
is null (i.e., no session flag set), and that reload was not called; name the
test like "does not set the session flag when sync collection removal fails".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2edc6c7b-9778-4216-b196-bace2ae7f455
📒 Files selected for processing (3)
packages/query/src/collection-replication-state.tspackages/query/src/logs-storage-recovery.tspackages/query/tests/logs-recovery.test.ts
📊 Test Coverage Report
Coverage Legend
|
🚀 Deployment Summary
🔗 Quick Links🤖 Updated by GitHub Actions |
1 participant
Summary
templatescount failure path.requestRemote/SyntaxErrorcount error.Design decisions (preserve through rebases)
Test plan
could not requestRemoteerror.pnpm --filter @wcpos/query test --runInBandpnpm --filter @wcpos/query typecheckpnpm --filter @wcpos/query lint(0 errors; existing hook dependency warnings remain)🤖 Generated with Codex
Summary by CodeRabbit
Bug Fixes
Tests