247 dexiejs migration#248
Conversation
…wman/corates into 247-dexiejs-migration
…wman/corates into 247-dexiejs-migration
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR consolidates multiple IndexedDB databases into a unified Dexie-based storage layer (db.js), migrating from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 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. 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 |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
corates-workers-prod | cf44c44 | Jan 08 2026, 03:10 AM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/web/src/primitives/__tests__/db.test.js:
- Around line 14-25: The comment claiming "Reset modules to get fresh database
instance" is misleading because dynamic import() does not clear module cache;
update the beforeEach comment to state that test isolation is achieved via the
afterEach cleanup (which uses deleteProjectData/clearAllData) OR add
vi.resetModules() before the dynamic import to truly reset the module cache and
get a fresh db instance; adjust the comment accordingly and keep references to
the beforeEach block, db, deleteProjectData, and clearAllData so reviewers can
locate the change.
In @packages/web/src/primitives/db.js:
- Around line 131-148: The formStates store needs a projectId index and the
deleteProjectData routine must remove project-scoped form states; update the
schema in the DB versioning (the formStates definition used when calling
this.version(...).stores) to include projectId as an indexed field (e.g., add
projectId to the formStates key list), and inside deleteProjectData call the
formStates deletion path using
db.formStates.where('projectId').equals(projectId).delete() to remove all
formStates tied to the deleted project.
🧹 Nitpick comments (6)
packages/web/src/api/better-auth-store.js (1)
45-51: Remove unused import.The
clearAvatarCacheimport on line 48 is no longer used after migrating toclearAllData(). The other avatar-related functions are still needed for caching during authentication.♻️ Proposed fix to remove unused import
import { fetchAndCacheAvatar, getCachedAvatar, - clearAvatarCache, pruneExpiredAvatars, } from '@/primitives/avatarCache.js';packages/web/src/primitives/db.js (2)
163-168: Consider expanding project cleanup to include ops and formStates.The
deleteProjectDatafunction currently removes the project and its PDFs, but doesn't clean up:
opstable entries that may reference this projectformStatesentries associated with this project (for 'addStudies' forms)While these may not cause correctness issues, they represent orphaned data that consumes storage space.
♻️ Expand cleanup to include ops and formStates
export async function deleteProjectData(projectId) { - await db.transaction('rw', [db.projects, db.pdfs], async () => { + await db.transaction('rw', [db.projects, db.pdfs, db.ops, db.formStates], async () => { await db.projects.delete(projectId); await db.pdfs.where('projectId').equals(projectId).delete(); + // Clean up orphaned operation queue entries + // Note: ops table doesn't have projectId index, so we need to scan + const projectOps = await db.ops.filter(op => + op.endpoint?.includes(projectId) || op.payload?.projectId === projectId + ).toArray(); + await db.ops.bulkDelete(projectOps.map(op => op.id)); + + // Clean up form states for addStudies forms + await db.formStates.where('projectId').equals(projectId).delete(); }); }Note: This assumes
formStateshas aprojectIdfield as indicated in the FormStateRow typedef. If ops cleanup is too expensive without an index, consider adding aprojectIdindex to the ops table in a future version.
163-188: Consider adding error handling for data operations.Both
deleteProjectDataandclearAllDataperform critical data operations but don't include error handling. If these operations fail (e.g., due to quota exceeded, IndexedDB corruption), the caller won't be notified.Consider wrapping these in try-catch blocks or documenting that callers should handle potential Dexie exceptions.
♻️ Add error handling example
export async function deleteProjectData(projectId) { + try { await db.transaction('rw', [db.projects, db.pdfs], async () => { await db.projects.delete(projectId); await db.pdfs.where('projectId').equals(projectId).delete(); }); + } catch (error) { + console.error(`Failed to delete project data for ${projectId}:`, error); + throw error; // Re-throw to let caller handle + } }Alternatively, document that these functions may throw Dexie errors and callers should handle them appropriately.
packages/web/src/primitives/__tests__/pdfCache.test.js (1)
133-152: LRU eviction test relies on timing delay.The 10ms
setTimeoutfor timestamp differentiation may be flaky in slow CI environments. Consider using explicitcachedAtvalues by directly inserting records viadb.pdfs.put()instead of relying oncachePdftiming.♻️ Proposed fix for deterministic timestamp ordering
describe('LRU eviction', () => { it('evicts oldest entries when cache limit exceeded', async () => { - // Cache limit is 200MB, so we test with smaller files but verify ordering - const data = new ArrayBuffer(1000); - - // Cache multiple entries with different timestamps - await cachePdf('project-1', 'study-1', 'old.pdf', data); - await new Promise(r => setTimeout(r, 10)); // Small delay for cachedAt difference - await cachePdf('project-1', 'study-2', 'newer.pdf', data); + // Insert directly with explicit cachedAt values for deterministic ordering + await db.pdfs.bulkPut([ + { + id: 'project-1:study-1:old.pdf', + projectId: 'project-1', + studyId: 'study-1', + fileName: 'old.pdf', + data: new ArrayBuffer(1000), + size: 1000, + cachedAt: 1000, + }, + { + id: 'project-1:study-2:newer.pdf', + projectId: 'project-1', + studyId: 'study-2', + fileName: 'newer.pdf', + data: new ArrayBuffer(1000), + size: 1000, + cachedAt: 2000, + }, + ]); // Verify both are cached expect(await getCachedPdf('project-1', 'study-1', 'old.pdf')).not.toBeNull();packages/web/src/primitives/pdfCache.js (1)
51-54: Consider performance for large caches.
getTotalCacheSize()loads all entries viatoArray()to sum sizes. For a 200MB cache with many small files, this could load significant data into memory. Consider using Dexie'seach()to iterate without loading all data at once, or store a running total in a separate metadata record.♻️ Alternative using each() for memory efficiency
async function getTotalCacheSize() { - const all = await db.pdfs.toArray(); - return all.reduce((sum, entry) => sum + (entry.size || 0), 0); + let total = 0; + await db.pdfs.each(entry => { + total += entry.size || 0; + }); + return total; }packages/web/src/lib/queryPersister.js (1)
1-55: Clean migration to Dexie, with one minor suggestion.The migration from IndexedDB to Dexie is well-executed with proper API usage and updated documentation. All persister methods correctly interact with
db.queryCache.♻️ Consider using nullish coalescing operator for consistency
Line 37 uses the logical OR operator (
||) which could theoretically returnnullif the cached data is a falsy value (0, false, empty string). While TanStack Query cache data should always be an object, using the nullish coalescing operator (??) would be more explicit and consistent with the pattern used inavatarCache.jsline 47.- return record?.data || null; + return record?.data ?? null;This only checks for
nullorundefinedrather than all falsy values.
| beforeEach(async () => { | ||
| // Reset modules to get fresh database instance | ||
| const module = await import('../db.js'); | ||
| db = module.db; | ||
| deleteProjectData = module.deleteProjectData; | ||
| clearAllData = module.clearAllData; | ||
|
|
||
| // Ensure database is open | ||
| if (!db.isOpen()) { | ||
| await db.open(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the full test file to understand the implementation
cat -n packages/web/src/primitives/__tests__/db.test.js | head -50Repository: InfinityBowman/corates
Length of output: 1874
🏁 Script executed:
#!/bin/bash
# Find vitest configuration
fd -t f "vitest.config" packages/web -o packages/Repository: InfinityBowman/corates
Length of output: 203
🏁 Script executed:
#!/bin/bash
# Look for vitest.config in common locations
find . -name "vitest.config.*" -o -name "vite.config.*" | grep -E "(vitest|vite)" | head -5Repository: InfinityBowman/corates
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Read the vite.config.js for packages/web
cat -n packages/web/vite.config.jsRepository: InfinityBowman/corates
Length of output: 3299
🏁 Script executed:
#!/bin/bash
# Also check if there's a vitest.config in packages/web root
ls -la packages/web/ | grep -E "vitest|vite"Repository: InfinityBowman/corates
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Read the db.js file to understand how the database instance is created
cat -n packages/web/src/primitives/db.js | head -60Repository: InfinityBowman/corates
Length of output: 2452
🏁 Script executed:
#!/bin/bash
# Read the rest of db.js to see how the database instance is created
cat -n packages/web/src/primitives/db.js | tail -n +60Repository: InfinityBowman/corates
Length of output: 4893
🏁 Script executed:
#!/bin/bash
# Check if there are other test files in the codebase using similar patterns
rg "beforeEach.*import\(" packages/ --type js -B 2 -A 3Repository: InfinityBowman/corates
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if there are other references to resetModules or module reset in tests
rg "resetModules|resetModuleRegistry" packages/web --type jsRepository: InfinityBowman/corates
Length of output: 311
🏁 Script executed:
#!/bin/bash
# Check the setup.js to see if there's any module isolation configuration
cat packages/web/src/__tests__/setup.js 2>/dev/null || echo "File not found"Repository: InfinityBowman/corates
Length of output: 2074
🏁 Script executed:
#!/bin/bash
# Check projectStore.test.js to see how it uses vi.resetModules()
cat -n packages/web/src/primitives/__tests__/projectStore.test.js | head -40Repository: InfinityBowman/corates
Length of output: 1580
Remove misleading comment or add vi.resetModules() before import.
The comment at line 15 ("Reset modules to get fresh database instance") is misleading. Dynamic import() alone does not reset the module cache; the db singleton is shared across all tests. Test isolation currently works through the comprehensive afterEach cleanup (lines 27-39), not the dynamic import.
Either:
- Update the comment to reflect that cleanup provides isolation, not the import
- Add
vi.resetModules()before the import (like inprojectStore.test.js) if you want truly fresh instances per test
The current approach works, but the code comment should accurately describe what's happening.
🤖 Prompt for AI Agents
In @packages/web/src/primitives/__tests__/db.test.js around lines 14 - 25, The
comment claiming "Reset modules to get fresh database instance" is misleading
because dynamic import() does not clear module cache; update the beforeEach
comment to state that test isolation is achieved via the afterEach cleanup
(which uses deleteProjectData/clearAllData) OR add vi.resetModules() before the
dynamic import to truly reset the module cache and get a fresh db instance;
adjust the comment accordingly and keep references to the beforeEach block, db,
deleteProjectData, and clearAllData so reviewers can locate the change.
| this.version(1).stores({ | ||
| // Y.Doc stored as 'ydoc' property via y-dexie | ||
| projects: 'id, orgId, updatedAt, ydoc: Y.Doc', | ||
| // PDF cache with LRU eviction by cachedAt | ||
| pdfs: 'id, projectId, studyId, cachedAt', | ||
| // Operation queue with compound index for efficient pending queries | ||
| ops: '++id, idempotencyKey, status, createdAt, [status+createdAt]', | ||
| // Avatar cache with expiry by cachedAt | ||
| avatars: 'userId, cachedAt', | ||
| // Form state persistence for OAuth redirects | ||
| formStates: 'key, type, timestamp', | ||
| // Local checklists for offline practice | ||
| localChecklists: 'id, createdAt, updatedAt', | ||
| // PDFs associated with local checklists | ||
| localChecklistPdfs: 'checklistId, updatedAt', | ||
| // TanStack Query cache persistence | ||
| queryCache: 'key', | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
y-dexie v4 Y.Doc schema syntax Dexie storage
💡 Result:
Summary — y-dexie (v4) + Y.Doc schema + how Dexie stores Y.Doc
-
What y-dexie is: y-dexie is the Dexie add‑on that provides Y.js (Y.Doc) storage and a Dexie Y provider for loading/releasing Y.Doc instances in Dexie-managed objects. Use it by adding the addon when creating the Dexie instance. [1]
-
Schema syntax (how to declare Y.Doc fields): declare a property in your stores() schema with the type Y.Doc (example):
db.version(1).stores({
friends:++id, name, notes: Y.Doc, age
})
This tells Dexie/y-dexie that the property (notes) is a Y.Doc property (virtual on objects; persisted in its own dedicated table). [1][2] -
Behavior / rules (key points):
- Y.Doc properties exist on all returned objects (as non-enumerable prototype properties) and are never null/undefined. [2]
- Physical Y.Doc update data are stored in a separate IndexedDB table(s) managed by y-dexie (not inline with the object row). [2]
- Y.Doc props are readonly on query results (you cannot replace them with table.update()); to edit use Y.js APIs on the document instance. [2]
- You must load the document before using it via the provided provider/hook:
- Imperative: const provider = DexieYProvider.load(doc); await provider.whenLoaded; ... DexieYProvider.release(doc). [2]
- React: useDocument(doc) / useLiveQuery helpers available in dexie-react-hooks integration. [3]
-
Migration / packaging note: Dexie moved built-in Y.js support into the separate y-dexie add-on (from earlier dexie@4.1.x experimental support). Install/import y-dexie and pass it as an addon when creating the Dexie instance. Also update schema declarations to use Y.Doc. [2]
Selected sources
- Dexie docs — y-dexie overview & examples. [1]
- Dexie GitHub release notes / migration details showing Y.Doc schema syntax and DexieYProvider usage. [2]
- dexie-react-hooks useDocument sample (React hook for consuming Y.Doc). [3]
If you want, I can paste a minimal code example (Dexie + y-dexie + Yjs) and show how to add/get a record and load its Y.Doc.
🏁 Script executed:
cd packages/web/src/primitives && head -200 db.js | tail -100Repository: InfinityBowman/corates
Length of output: 2898
🏁 Script executed:
cd packages/web/src && rg -A 3 "db\.ops\." | head -50Repository: InfinityBowman/corates
Length of output: 2768
🏁 Script executed:
cd packages/web/src && rg -A 3 "db\.formStates\." | head -50Repository: InfinityBowman/corates
Length of output: 2033
🏁 Script executed:
cd packages/web/src/primitives && grep -A 15 "typedef.*OpQueueRow" db.jsRepository: InfinityBowman/corates
Length of output: 733
🏁 Script executed:
cd packages/web/src/lib && head -60 formStatePersistence.jsRepository: InfinityBowman/corates
Length of output: 1656
Remove ops cleanup suggestion; add formStates cleanup for project deletion.
The ydoc: Y.Doc schema syntax is correct for y-dexie v4—y-dexie automatically manages Y.Doc persistence in separate IndexedDB tables and handles cleanup when the parent object is deleted. The ops table does not store projectId (operations are app-wide), so they don't require per-project cleanup.
However, deleteProjectData should also delete form states associated with the project:
await db.formStates.where('projectId').equals(projectId).delete();Form state entries can be project-specific (when saved during "add studies" workflows) and should be cleaned up to avoid orphaned data, even though the 24-hour expiry provides eventual cleanup.
🤖 Prompt for AI Agents
In @packages/web/src/primitives/db.js around lines 131 - 148, The formStates
store needs a projectId index and the deleteProjectData routine must remove
project-scoped form states; update the schema in the DB versioning (the
formStates definition used when calling this.version(...).stores) to include
projectId as an indexed field (e.g., add projectId to the formStates key list),
and inside deleteProjectData call the formStates deletion path using
db.formStates.where('projectId').equals(projectId).delete() to remove all
formStates tied to the deleted project.
2 participants
Summary by CodeRabbit
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.