Conversation
|
📝 WalkthroughWalkthroughThis PR refactors the database layer to support a multi-store architecture with generic, store-parameterized operations. It introduces email encryption abstraction (CryptoEmail), a repository pattern for email CRUD and search (EmailRepository), a provider singleton for lifecycle management (DatabaseProvider), new custom error classes, and restructured database configuration. The DatabaseService API is generalized from email-specific methods to generic store-scoped operations, and all dependent tests are refactored accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/database/index.ts (1)
77-133:⚠️ Potential issue | 🟠 MajorCursor pagination drops rows when index keys are not unique.
Both methods store only
cursor.keyas the resume token, but when resuming across transactions with non-unique index keys, you must also storecursor.primaryKey. After reopening the cursor with the range, you need to skip records until you pass the previously-processed position. Without this, duplicate index values are skipped entirely.For example, if three emails share the same
receivedAttimestamp, the first batch retrieves two of them and returns that timestamp asnextCursor. The next batch usesIDBKeyRange.upperBound(timestamp, true), which in reverse direction skips all remaining records at that timestamp and continues to the next lower value, dropping the third email.Resume with both the index key and primary key to avoid gaps:
- Store
cursor.keyandcursor.primaryKeyfrom the last processed record- Open the next cursor with
IDBKeyRange.lowerBound(lastIndexKey, true)(or equivalent)- Skip records until you advance past the stored
lastPrimaryKey🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/database/index.ts` around lines 77 - 133, getBatch and getBatchByFolder currently resumecursors using only cursor.key which drops rows when index keys are non-unique; change the resume token to capture both cursor.key and cursor.primaryKey (e.g., lastIndexKey and lastPrimaryKey), reopen the cursor using an appropriate bound (upperBound/lowerBound) on the index key as you already do, then when iterating the reopened cursor skip records until you encounter a record whose primaryKey is strictly past the saved lastPrimaryKey (i.e., advance past duplicates with the same index key) before collecting items; update the returned nextCursor to include both index key and primaryKey so subsequent calls can resume correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/database/emails/crypto/crypto.test.ts`:
- Around line 6-12: The tests only cover the happy path for CryptoEmail.encrypt;
add edge-case tests that exercise CryptoEmail.decrypt failure modes: create a
CryptoEmail instance A with a key and encrypt a mail, then attempt to decrypt
with a different CryptoEmail instance B (wrong key) and assert it throws or
returns an invalid result; also tamper with the encrypted payload (e.g., flip
bytes in the ciphertext) and assert CryptoEmail.decrypt raises an error or fails
validation; include tests for empty/undefined inputs to
CryptoEmail.encrypt/decrypt to verify proper error handling in those methods.
- Around line 14-21: The test that calls new CryptoEmail(...),
crypto.encrypt(...) and crypto.decrypt(...) is asserting decryption but is
currently nested in the "Encrypt emails" describe; move this test out of that
describe block into a new or existing "Decrypt emails" describe (or rename the
test block) so tests are organized by behavior — locate the test that uses
CryptoEmail, encrypt and decrypt and relocate it under a describe('Decrypt
emails', ...) grouping without changing the test implementation.
In `@src/services/database/emails/repository/index.ts`:
- Around line 96-97: The byFrom and byTo index lookups
(EMAIL_DB_INDEXES_KEYS.byFrom / byTo used in the if branches) will never match
because params.from/params.to are arrays of User objects but the indexes were
not defined with multiEntry and/or a flattened string key; fix by either (A) add
a new index key on a flattened string array (e.g., store sender/recipient email
strings in a separate field like params.fromEmails/params.toEmails and define
the index with multiEntry: true) and query that index instead of byFrom/byTo, or
(B) remove these early index-based prefilters and fall back to using
matchesFilters(...) to filter sender/recipient in-memory; update the code paths
using STORE and EMAIL_DB_INDEXES_KEYS.byFrom/byTo (and any creation of
params.from/params.to) accordingly.
- Around line 93-95: The code is querying the byRead index with numeric 1/0
which won't match the boolean index; update the call to
this.db.getByIndex<StoredEmail>(STORE, EMAIL_DB_INDEXES_KEYS.byRead, ...) to
pass the boolean value directly (use filters.isRead) instead of converting to
1/0 so the IndexedDB index on params.isRead matches correctly.
- Around line 101-103: The date-range query in getBaseResults uses
IDBKeyRange.bound(filters.dateRange.after, filters.dateRange.before) which
creates numeric bounds that won't match the string ISO dates stored under
TIME_INDEX; update getBaseResults so the bounds match the stored type (either
convert filters.dateRange.after/ before to ISO strings before calling
IDBKeyRange.bound, or change the index to use numeric timestamps) and then call
this.db.getByRange<StoredEmail>(STORE, TIME_INDEX, range) with those corrected
bounds; refer to getBaseResults, filters.dateRange, IDBKeyRange.bound,
TIME_INDEX, StoredEmail and the existing dateInRange helper for guidance on
conversion.
In `@src/services/database/index.ts`:
- Around line 22-31: The upgrade currently only creates indexes when a store is
newly created, so new indexes added in later versions are skipped for existing
stores; modify the openDB upgrade callback (the upgrade function passed to
openDB) to accept the upgrade transaction (e.g., upgrade(db, oldVersion,
newVersion, tx)) and for each store in stores check: if the store already
exists, obtain the existing object store from the upgrade transaction
(tx.objectStore(store.name)) and iterate store.indexes to create any missing
indexes (check index presence via objectStore.indexNames.contains(index.name) or
similar) instead of only creating indexes on newly created object stores.
In `@src/services/database/provider/database.provider.test.ts`:
- Around line 44-49: The existing test only asserts sequential calls to
DatabaseProvider.init() return the same instance but misses the race-condition
scenario; add a new test that calls DatabaseProvider.init() concurrently (e.g.,
via Promise.all) and asserts both returned values are identical to ensure init()
handles concurrent callers and only creates a single instance; reference the
DatabaseProvider.init() method and add a test named like "When initializing
concurrently, then it should return the same instance".
In `@src/services/database/provider/index.ts`:
- Around line 26-38: The DatabaseService is opened before validating
prerequisites, so if LocalStorageService.instance.getMnemonic(),
deriveDatabaseKey(), or subsequent setup throws the db remains open; update the
init flow in the provider to either (A) gather mnemonic and compute indexKey
(using deriveDatabaseKey and mnemonicToBytes) and construct CryptoEmail and
EmailRepository before calling db.open(), or (B) wrap the post-open
initialization (derivation, new CryptoEmail, new EmailRepository, and setting
DatabaseProvider.instance) in a try/catch/finally and call db.close() in the
failure path; ensure you reference DatabaseService (db.open()/db.close()),
LocalStorageService.instance.getMnemonic(), MnemonicNotFoundError,
deriveDatabaseKey, CryptoEmail, EmailRepository, and DatabaseProvider.instance
when making the change so no open connection is leaked on error.
- Around line 20-38: DatabaseProvider.init can race when called concurrently;
make initialization atomic by introducing a static initPromise (e.g.,
DatabaseProvider.initPromise) and use it as a lock: at the top of init, if
DatabaseProvider.instance return it; if DatabaseProvider.initPromise exists
await and return it; otherwise set DatabaseProvider.initPromise to an async
initializer that performs the current creation steps (get user, open
DatabaseService, derive key, create CryptoEmail and EmailRepository), assigns
DatabaseProvider.instance, then clears initPromise (or leaves it resolved) and
returns the instance; ensure errors reject and clear initPromise so subsequent
calls can retry.
In `@src/services/database/types/index.ts`:
- Around line 77-101: The Database type must match the actual DatabaseService
surface: add the missing get<T>(storeName: string, key: IDBValidKey) =>
Promise<T | undefined> method and widen any parameters currently typed as string
| string[] to use IDBValidKey (e.g. the key parameter on getByIndex, remove, and
the startCursor on getBatch/getBatchByFolder) so callers (like EmailRepository)
can use the abstraction; keep getByRange as-is (IDBKeyRange) and ensure return
types (e.g. nextCursor) remain IDBValidKey.
- Around line 23-25: EmailParams currently defines folderId as string[] which
breaks queries against the byFolderIdAndTime compound index used by
getBatchByFolder because the index needs multiEntry semantics for array-valued
keys; either change EmailParams.folderId from string[] to string if emails only
ever belong to a single folder, or update the byFolderIdAndTime index definition
to include multiEntry: true (in the index config where keyPath is
['params.folderId','params.receivedAt']) so array folder IDs are indexed
per-entry and queries using scalar folderId strings will match.
---
Outside diff comments:
In `@src/services/database/index.ts`:
- Around line 77-133: getBatch and getBatchByFolder currently resumecursors
using only cursor.key which drops rows when index keys are non-unique; change
the resume token to capture both cursor.key and cursor.primaryKey (e.g.,
lastIndexKey and lastPrimaryKey), reopen the cursor using an appropriate bound
(upperBound/lowerBound) on the index key as you already do, then when iterating
the reopened cursor skip records until you encounter a record whose primaryKey
is strictly past the saved lastPrimaryKey (i.e., advance past duplicates with
the same index key) before collecting items; update the returned nextCursor to
include both index key and primaryKey so subsequent calls can resume correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad638d81-e9ec-4a6f-ab73-689facb91b05
📒 Files selected for processing (16)
sonar-project.propertiessrc/errors/database/index.tssrc/errors/index.tssrc/services/database/config.tssrc/services/database/config/index.tssrc/services/database/config/utils.tssrc/services/database/database.service.test.tssrc/services/database/emails/crypto/crypto.test.tssrc/services/database/emails/crypto/index.tssrc/services/database/emails/repository/email.repository.test.tssrc/services/database/emails/repository/index.tssrc/services/database/index.tssrc/services/database/provider/database.provider.test.tssrc/services/database/provider/index.tssrc/services/database/types/index.tstsconfig.app.json
💤 Files with no reviewable changes (2)
- tsconfig.app.json
- src/services/database/config.ts
| if (filters.isRead !== undefined) { | ||
| return this.db.getByIndex<StoredEmail>(STORE, EMAIL_DB_INDEXES_KEYS.byRead, filters.isRead ? 1 : 0); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In IndexedDB, if an index keyPath resolves to a boolean property, does IDBIndex.getAll need true/false, or can numeric 1/0 match?
💡 Result:
In IndexedDB, if an index keyPath resolves to a boolean property (true or false), IDBIndex.getAll requires the boolean values true or false for matching. Numeric 1/0 cannot match because booleans are not valid IndexedDB keys. Attempting to create an index on a boolean property or use a boolean as a query key will fail with a DataError DOMException, as the key is invalid. To query records where the boolean property is true/false, you cannot use the index directly. Instead, use a cursor on the object store without a key range (full scan) and filter in JavaScript, or store a numeric equivalent (1/0) in a separate indexed property.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/IDBFactory/cmp
- 2: https://developer.mozilla.org/en-US/docs/IndexedDB/Basic_Concepts_Behind_IndexedDB
- 3: https://github.com/mdn/content/blob/main/files/en-us/web/api/idbfactory/cmp/index.md
- 4: https://stackoverflow.com/questions/74846944/what-types-of-keys-are-allowed-in-indexeddb-and-how-are-they-ordered
- 5: boolean should be a valid key w3c/IndexedDB#76
🏁 Script executed:
# Find the repository index file and examine it
cat -n src/services/database/emails/repository/index.ts | head -150Repository: internxt/mail-web
Length of output: 6541
🏁 Script executed:
# Search for IndexedDB configuration, schema, or setup files
find . -type f \( -name "*.ts" -o -name "*.js" \) | xargs rg -l "byRead|EMAIL_DB_INDEXES" | head -20Repository: internxt/mail-web
Length of output: 257
🏁 Script executed:
# Look for index key definitions
rg "EMAIL_DB_INDEXES_KEYS|byRead" -A 3 -B 3Repository: internxt/mail-web
Length of output: 11766
🏁 Script executed:
# Check the StoredEmail type definition
rg "type StoredEmail|interface StoredEmail" -A 15 src/services/database/types/Repository: internxt/mail-web
Length of output: 919
🏁 Script executed:
# Check generateEmailParamPath implementation
cat -n src/services/database/config/utils.ts 2>/dev/null || rg "generateEmailParamPath" -B 5 -A 10Repository: internxt/mail-web
Length of output: 262
🏁 Script executed:
# Find DatabaseService implementation
rg "class DatabaseService|getByIndex" src/services/database/ -B 2 -A 10Repository: internxt/mail-web
Length of output: 13783
Query byRead with booleans, not 1/0.
The byRead index is created on params.isRead which is a boolean field. IndexedDB requires exact type matching for keys—querying with numeric 1 or 0 will never match boolean true or false values in the index. Pass the boolean directly: filters.isRead instead of converting to 1/0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/database/emails/repository/index.ts` around lines 93 - 95, The
code is querying the byRead index with numeric 1/0 which won't match the boolean
index; update the call to this.db.getByIndex<StoredEmail>(STORE,
EMAIL_DB_INDEXES_KEYS.byRead, ...) to pass the boolean value directly (use
filters.isRead) instead of converting to 1/0 so the IndexedDB index on
params.isRead matches correctly.
There was a problem hiding this comment.
Seems that IDBValidKey does not return a boolean field:
type IDBValidKey = number | string | Date | BufferSource | IDBValidKey[];
So we need to use numbers as we do, right?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the EmailParams interface definition
rg "EmailParams|isRead" src/services/database/types/ -A 2 -B 2 -n
# Also check the StoredEmail type
rg "StoredEmail|EmailParams" src/services/database/types/ -A 20 -n
Length of output: 2904
@xabg2, you're absolutely right on the TypeScript side — IDBValidKey excludes boolean, so passing 1/0 is indeed the only valid approach for the query argument, and my original suggestion to pass filters.isRead directly would have been a TypeScript type error. I apologize for the incorrect suggestion.
However, there is still a real issue on the storage side. EmailParams.isRead is typed as boolean (Line 25 in src/services/database/types/index.ts):
export interface EmailParams {
isRead: boolean; // <-- boolean stored in IDB
...
}Since boolean is not a valid IndexedDB key type, IndexedDB silently skips records whose index keyPath resolves to a boolean — they are simply not added to the byRead index. This means even though your query getByIndex(STORE, byRead, 1) is TypeScript-valid, it will always return an empty array at runtime because no records ever populate that index.
The fix would be to change the isRead field in EmailParams to a numeric type and ensure it's stored as 1/0:
export interface EmailParams {
isRead: 0 | 1; // valid IDB key type
receivedAt: string;
...
}And update all writes accordingly (e.g. isRead: email.isRead ? 1 : 0). The same concern applies to hasAttachment: boolean (Line 29) if it is used as an index key.
✏️ Learnings added
Learnt from: xabg2
Repo: internxt/mail-web PR: 37
File: src/services/database/emails/repository/index.ts:93-95
Timestamp: 2026-04-13T14:02:25.315Z
Learning: In the internxt/mail-web repository, `EmailParams.isRead` (in `src/services/database/types/index.ts`) is typed as `boolean`. Since boolean is not a valid IndexedDB key type (`IDBValidKey = number | string | Date | BufferSource | IDBValidKey[]`), IndexedDB silently skips records where the index keyPath resolves to a boolean, making the `byRead` index effectively empty. The correct fix is to store `isRead` as `0 | 1` (number) rather than boolean, and convert at write time. Querying with numeric `1/0` in `getByIndex` is the correct TypeScript approach since `IDBValidKey` excludes boolean.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| static async init(): Promise<DatabaseProvider> { | ||
| if (DatabaseProvider.instance) return DatabaseProvider.instance; | ||
|
|
||
| const user = LocalStorageService.instance.getUser(); | ||
| if (!user) throw new UserNotFoundError(); | ||
|
|
||
| const databaseName = `DB:${user.uuid}`; | ||
| const db = new DatabaseService(databaseName, MAIL_DB_CONFIG); | ||
| await db.open(); | ||
|
|
||
| const mnemonic = LocalStorageService.instance.getMnemonic(); | ||
| if (!mnemonic) throw new MnemonicNotFoundError(); | ||
| const indexKey = await deriveDatabaseKey(mnemonicToBytes(mnemonic)); | ||
| const crypto = new CryptoEmail(indexKey); | ||
|
|
||
| const emails = new EmailRepository(db, crypto); | ||
|
|
||
| DatabaseProvider.instance = new DatabaseProvider(db, emails); | ||
| return DatabaseProvider.instance; |
There was a problem hiding this comment.
Make singleton initialization atomic.
Two parallel init() calls can both pass the instance check, open separate databases, and then race to overwrite the singleton. Please guard initialization with a cached initPromise or equivalent lock.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/database/provider/index.ts` around lines 20 - 38,
DatabaseProvider.init can race when called concurrently; make initialization
atomic by introducing a static initPromise (e.g., DatabaseProvider.initPromise)
and use it as a lock: at the top of init, if DatabaseProvider.instance return
it; if DatabaseProvider.initPromise exists await and return it; otherwise set
DatabaseProvider.initPromise to an async initializer that performs the current
creation steps (get user, open DatabaseService, derive key, create CryptoEmail
and EmailRepository), assigns DatabaseProvider.instance, then clears initPromise
(or leaves it resolved) and returns the instance; ensure errors reject and clear
initPromise so subsequent calls can retry.
| export type Database = { | ||
| open: () => Promise<void>; | ||
| close: () => void; | ||
| getAll: <T>(storeName: string) => Promise<T[]>; | ||
| getByIndex: <T>(storeName: string, indexName: string, key: string | string[]) => Promise<T[]>; | ||
| getByRange: <T>(storeName: string, indexName: string, range: IDBKeyRange) => Promise<T[]>; | ||
| put: <T>(storeName: string, record: T) => Promise<IDBValidKey>; | ||
| putMany: <T>(storeName: string, records: T[]) => Promise<void>; | ||
| remove: (storeName: string, key: string | string[]) => Promise<void>; | ||
| count: (storeName: string) => Promise<number>; | ||
| deleteOldest: (storeName: string, indexName: string, count: number) => Promise<void>; | ||
| getBatch: <T>( | ||
| storeName: string, | ||
| indexName: string, | ||
| batchSize: number, | ||
| startCursor?: string | string[], | ||
| ) => Promise<{ items: T[]; nextCursor?: IDBValidKey }>; | ||
| getBatchByFolder: <T>( | ||
| storeName: string, | ||
| indexName: string, | ||
| folderId: string, | ||
| batchSize: number, | ||
| startCursor?: IDBValidKey, | ||
| ) => Promise<{ items: T[]; nextCursor?: IDBValidKey }>; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
The public Database contract no longer matches the service surface.
DatabaseService exposes get() and accepts general IDBValidKey keys/cursors, but this type omits get() and narrows several parameters to string | string[]. That forces EmailRepository and its tests to depend on the concrete class instead of the abstraction. Please mirror the actual service signatures here and inject Database downstream.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/database/types/index.ts` around lines 77 - 101, The Database
type must match the actual DatabaseService surface: add the missing
get<T>(storeName: string, key: IDBValidKey) => Promise<T | undefined> method and
widen any parameters currently typed as string | string[] to use IDBValidKey
(e.g. the key parameter on getByIndex, remove, and the startCursor on
getBatch/getBatchByFolder) so callers (like EmailRepository) can use the
abstraction; keep getByRange as-is (IDBKeyRange) and ensure return types (e.g.
nextCursor) remain IDBValidKey.
|
I'm going to close this PR as we are doing to do the search on the server side. We only need to handle the encrypted mails, but they will not need all this indexes. I will only refactor the database service and that's it. |
1 participant
Implementing the Emails Storage in the local Database. This PR contains: