feat: store watch-list data in IndexedDB - #78
Conversation
|
@CodeWithMaBot is attempting to deploy a commit to the Ma's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change moves application persistence from ChangesIndexedDB storage migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AngularApplication
participant appConfig
participant StorageService
participant IndexedDB
AngularApplication->>appConfig: Run application initializer
appConfig->>StorageService: Call initialize()
StorageService->>IndexedDB: Open and load storage
IndexedDB-->>StorageService: Return stored or default data
StorageService-->>AngularApplication: Resolve initialization promise
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
README.md (1)
37-39: 🧹 Nitpick | 🔵 TrivialConsider a one-time automatic localStorage detection as a safety net.
The note to export data before upgrading is accurate given the current implementation has no migration path from
localStorageto IndexedDB. Since this depends on the user reading the README before upgrading, consider havingStorageServicedetect a leftover legacylocalStorageentry on first IndexedDB initialization and offer to import it automatically, reducing reliance on users seeing this note.🤖 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 `@README.md` around lines 37 - 39, Update StorageService initialization to perform a one-time check for the legacy localStorage watch-list entry after IndexedDB setup, and offer to import the data when found. Preserve existing IndexedDB behavior, avoid repeating the migration after it succeeds or is declined, and retain the README note as a fallback.
🤖 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.
Inline comments:
In `@src/app/services/storage.service.spec.ts`:
- Around line 34-38: Update the migration and v4-seasons tests around the
service.importData calls to await both asynchronous imports before performing
assertions or allowing the tests to finish, matching the existing awaited
importData usage elsewhere in the spec.
- Line 3: Reset fake IndexedDB before each test by assigning a new IDBFactory to
indexedDB before StorageService.initialize(). Apply this setup in
src/app/services/storage.service.spec.ts (lines 3-3),
src/app/services/round-robin.service.spec.ts (lines 4-14), and
src/app/services/watch-list.service.spec.ts (lines 5-14), using each file’s
existing beforeEach setup.
In `@src/app/services/storage.service.ts`:
- Around line 36-47: Update persistData so a failed write cannot leave this.data
holding an unpersisted value: track the last successfully persisted StorageData
and restore it when writeData(updated) rejects, while retaining the rejection
for callers such as importData. Ensure saveData’s fire-and-forget path also
triggers the rollback rather than only logging the error.
- Around line 21-24: Update StorageService.initialize/loadData to catch
normalizeStorageData, openDatabase, and readData failures, recover with
createDefaultStorageData(), persist the replacement, and resolve initialization
instead of caching a rejected promise. Update saveData/persistData so the signal
is reconciled only after IndexedDB write success, or propagate write failures to
callers rather than leaving unpersisted state in memory.
---
Nitpick comments:
In `@README.md`:
- Around line 37-39: Update StorageService initialization to perform a one-time
check for the legacy localStorage watch-list entry after IndexedDB setup, and
offer to import the data when found. Preserve existing IndexedDB behavior, avoid
repeating the migration after it succeeds or is declined, and retain the README
note as a fallback.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39ddb1bc-d3d7-4f6d-bda0-8f2c7fb764e4
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
README.mdpackage.jsonsrc/app/app.config.tssrc/app/services/import-export.service.tssrc/app/services/round-robin.service.spec.tssrc/app/services/storage.service.spec.tssrc/app/services/storage.service.tssrc/app/services/watch-list.service.spec.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/app/services/storage.service.ts (1)
22-24: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRecovery for
openDatabase()/readData()/writeData()failures is still missing.
loadDatanow recovers from anormalizeStorageDatafailure, but it does not catch failures fromthis.openDatabase()(Line 77),this.readData()(Line 78), orthis.writeData()insidesetPersistedData(Line 98). If any of these reject,loadData()rejects, andinitialize()caches that rejection inthis.initializationforever (Line 23). Every later call toinitialize()returns the same rejected promise. SinceprovideAppInitializerwaits for this promise, the app cannot bootstrap and cannot retry without a full reload.At minimum, reset
this.initializationtonullon failure so a later call can retry:🔧 Proposed fix to allow retrying initialization
initialize(): Promise<void> { - this.initialization ??= this.loadData(); + this.initialization ??= this.loadData().catch((error: unknown) => { + this.initialization = null; + throw error; + }); return this.initialization; }Decide separately whether
openDatabase()/readData()failures should fall back to an in-memory default (degraded mode without persistence) or simply remain a rejection that callers can retry.Also applies to: 76-101
🤖 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 `@src/app/services/storage.service.ts` around lines 22 - 24, Update the initialization flow around initialize() and loadData() so any openDatabase(), readData(), or writeData() failure clears this.initialization back to null before propagating the rejection, allowing subsequent initialize() calls to retry. Preserve the existing error behavior; do not add an in-memory fallback unless already established by the surrounding implementation.
🧹 Nitpick comments (1)
src/app/services/storage.service.ts (1)
103-116: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd an
onblockedhandler to avoid an indefinite hang on database open.
openDatabase()setsonupgradeneeded,onsuccess, andonerror, but notonblocked. Per MDN, theblockedevent fires when another open connection blocks aversionchangetransaction. Without a handler, the request neither resolves nor rejects until the blocking connection closes, and sinceprovideAppInitializerawaitsinitialize(), the app bootstrap hangs silently. This cannot happen at the initialDATABASE_VERSION, but it will become reachable the first timeDATABASE_VERSIONis bumped while another tab holds the app open.🔧 Proposed fix
request.onsuccess = () => resolve(request.result); request.onerror = () => reject(request.error ?? new Error('Failed to open IndexedDB')); + request.onblocked = () => + reject(new Error('IndexedDB open request is blocked by another open connection'));🤖 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 `@src/app/services/storage.service.ts` around lines 103 - 116, Update the openDatabase() IndexedDB request handling to add an onblocked callback that rejects the promise with a descriptive error, while preserving the existing onsuccess, onerror, and upgrade behavior.
🤖 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.
Duplicate comments:
In `@src/app/services/storage.service.ts`:
- Around line 22-24: Update the initialization flow around initialize() and
loadData() so any openDatabase(), readData(), or writeData() failure clears
this.initialization back to null before propagating the rejection, allowing
subsequent initialize() calls to retry. Preserve the existing error behavior; do
not add an in-memory fallback unless already established by the surrounding
implementation.
---
Nitpick comments:
In `@src/app/services/storage.service.ts`:
- Around line 103-116: Update the openDatabase() IndexedDB request handling to
add an onblocked callback that rejects the promise with a descriptive error,
while preserving the existing onsuccess, onerror, and upgrade behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fbbbe22-7378-4d2f-a7b1-50a9d0241ef5
📒 Files selected for processing (4)
src/app/services/round-robin.service.spec.tssrc/app/services/storage.service.spec.tssrc/app/services/storage.service.tssrc/app/services/watch-list.service.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/services/round-robin.service.spec.ts
- src/app/services/watch-list.service.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/app/components/settings/settings.component.spec.ts (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModel the asynchronous service contract in the test double.
exportRecoveryBackupreturnsPromise<void>, but the mock returnsundefined. Usevi.fn().mockResolvedValue(undefined)by default. Add resolved and rejected-path tests that assert both feedback messages.🤖 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 `@src/app/components/settings/settings.component.spec.ts` at line 23, Update the exportRecoveryBackup mock in the settings component tests to use vi.fn().mockResolvedValue(undefined), matching its Promise<void> contract. Add tests covering both resolved and rejected export paths, asserting the expected feedback message in each case.
🤖 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.
Inline comments:
In `@src/app/components/settings/settings.component.ts`:
- Around line 202-213: Update exportRecoveryBackup to clear both successMessage
and errorMessage before entering the try block, ensuring stale status from prior
operations is removed before the export begins.
In `@src/app/services/storage.service.ts`:
- Around line 68-73: Update persistData(), getData(), and loadData() so queued
writes, data exposed to callers, and lastPersistedData use independent deep
snapshots rather than shared nested objects. Clone the private snapshot before
assigning rollback state, preserving failed-write rollback to the last
successfully persisted values. Add a rejected-write test that mutates an item in
place and verifies rollback restores the prior nested value.
- Around line 155-157: The onblocked handler rejects the promise but does not
cancel the underlying open request, so onsuccess can still fire and return an
IDBDatabase object after rejection, leaving it unclosed. Mark the request as
abandoned when onblocked fires by setting a flag. In the onsuccess handler,
check this flag before resolving, and if the request was abandoned, call close()
on the returned IDBDatabase to clean up the resource before proceeding. This
ensures late-arriving success results after a blocked rejection do not leak an
open database connection.
---
Nitpick comments:
In `@src/app/components/settings/settings.component.spec.ts`:
- Line 23: Update the exportRecoveryBackup mock in the settings component tests
to use vi.fn().mockResolvedValue(undefined), matching its Promise<void>
contract. Add tests covering both resolved and rejected export paths, asserting
the expected feedback message in each case.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 47390848-2564-4b85-9aa2-9b489982f69c
📒 Files selected for processing (5)
src/app/components/settings/settings.component.spec.tssrc/app/components/settings/settings.component.tssrc/app/services/import-export.service.tssrc/app/services/storage.service.spec.tssrc/app/services/storage.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/services/storage.service.spec.ts
- Use pristine snapshot for rescue backup instead of mutated data - Fall back to in-memory session on IndexedDB write errors during bootstrap - Add transaction abort handling to read path to prevent hangs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/components/settings/settings.component.spec.ts (1)
107-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the selected backup key in both export tests.
The current assertions only verify the displayed message. They pass if
exportRecoveryBackupis not called or receives the wrong key. Assert the service call in both the success and failure cases.Proposed test update
await fixture.componentInstance.exportRecoveryBackup('watch-list-data-backup-123'); + expect(exportService.exportRecoveryBackup).toHaveBeenCalledWith( + 'watch-list-data-backup-123', + ); expect(fixture.componentInstance.successMessage()).toBe( 'Recovery backup exported successfully', );🤖 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 `@src/app/components/settings/settings.component.spec.ts` around lines 107 - 130, Update both recovery backup export tests around SettingsComponent.exportRecoveryBackup to spy on or assert ImportExportService.exportRecoveryBackup was called with the selected key 'watch-list-data-backup-123'. Keep the existing success and failure message assertions, and verify the call argument in each case.
🤖 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 `@src/app/components/settings/settings.component.spec.ts`:
- Around line 107-130: Update both recovery backup export tests around
SettingsComponent.exportRecoveryBackup to spy on or assert
ImportExportService.exportRecoveryBackup was called with the selected key
'watch-list-data-backup-123'. Keep the existing success and failure message
assertions, and verify the call argument in each case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 11e10985-e528-4df5-b54d-85a16b1fac41
📒 Files selected for processing (7)
src/app/app.htmlsrc/app/app.tssrc/app/components/settings/settings.component.spec.tssrc/app/components/settings/settings.component.tssrc/app/services/import-export.service.tssrc/app/services/storage.service.spec.tssrc/app/services/storage.service.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/app.html
- src/app/services/storage.service.spec.ts
- src/app/app.ts
- src/app/services/storage.service.ts
2 participants
Fixes #66
Summary by CodeRabbit
New Features
Bug Fixes
Documentation