feat: add store listener/subscription mechanism #174
Conversation
…rage` and `MemoryStorage`
WalkthroughAdds a publish-subscribe API to SessionBase/SessionManager (subscribe + notifyListeners with microtask batching) and updates store implementations (chrome, expoSecure, localStorage, memory) to call notifyListeners after mutations. New tests validate listener invocation, batching, deduplication, and unsubscribe behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Store as Store (chrome/expo/local/memory)
participant Session as SessionBase / SessionManager
participant L1 as Listener A
participant L2 as Listener B
Client->>Session: subscribe(listener)
Session-->>Client: unsubscribe()
rect rgba(200,230,255,0.12)
Client->>Store: set/remove/destroy
Store->>Session: notifyListeners()
alt notification not yet scheduled
Session->>Session: schedule microtask (coalesce)
else already scheduled
Session-->>Session: no-op (coalesced)
end
end
Note over Session: microtask flush triggers batched notifications
Session->>L1: invoke listener()
par
L1-->>Session: resolve (sync/async)
and
Session->>L2: invoke listener()
L2-->>Session: resolve (sync/async)
end
Session->>Session: reset batching flag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
lib/sessionManager/stores/expoSecureStore.ts (2)
41-48: Fix async iteration in destroySession.The
forEachwith an async callback doesn't await theremoveSessionItemoperations, sonotifyListeners()is called before all items are removed. This creates a race condition where listeners are notified while deletion is still in progress.Apply this diff to properly await all deletions:
async destroySession(): Promise<void> { const keys = Object.values(StorageKeys); - keys.forEach(async (key) => { - await this.removeSessionItem(key); - }); + await Promise.all( + keys.map((key) => this.removeSessionItem(key)) + ); this.notifyListeners(); }
56-79: Fix unreachable notifyListeners call and missing notification for string values.The
notifyListeners()call on line 78 is unreachable because:
- If
itemValueis a string, the function returns at line 73- If not a string, an error is thrown at line 75
Additionally, string values (the common case) don't trigger listener notifications due to the early return. This is inconsistent with the subscription mechanism's purpose.
Apply this diff to fix both issues:
if (typeof itemValue === "string") { splitString(itemValue, Math.min(storageSettings.maxLength, 2048)).forEach( async (splitValue, index) => { await expoSecureStore!.setItemAsync( `${storageSettings.keyPrefix}${itemKey}${index}`, splitValue, ); }, ); + this.notifyListeners(); return; } else { throw new Error("Item value must be a string"); } - - this.notifyListeners(); }lib/sessionManager/stores/memory.ts (1)
30-50: Missing notification for string values.String values don't trigger listener notifications because the function returns early at line 44 before reaching
notifyListeners()on line 49. This is inconsistent—string values are the primary use case for session storage.Apply this diff to notify listeners for all value types:
if (typeof itemValue === "string") { splitString(itemValue, storageSettings.maxLength).forEach( (splitValue, index) => { this.memCache[`${storageSettings.keyPrefix}${itemKey}${index}`] = splitValue; }, ); + this.notifyListeners(); return; } this.memCache[`${storageSettings.keyPrefix}${String(itemKey)}0`] = itemValue; this.notifyListeners(); }lib/sessionManager/stores/chromeStore.ts (1)
41-63: Two critical issues: missing notification for string values and async forEach.
- String values don't trigger notifications due to the early return at line 56
- The
forEachwith async callback doesn't await the storage operations, so the function may return before all chunks are writtenApply this diff to fix both issues:
if (typeof itemValue === "string") { - splitString(itemValue, storageSettings.maxLength).forEach( - async (splitValue, index) => { - await chrome.storage.local.set({ - [`${storageSettings.keyPrefix}${itemKey}${index}`]: splitValue, - }); - }, - ); + await Promise.all( + splitString(itemValue, storageSettings.maxLength).map( + async (splitValue, index) => { + await chrome.storage.local.set({ + [`${storageSettings.keyPrefix}${itemKey}${index}`]: splitValue, + }); + }, + ), + ); + this.notifyListeners(); return; } await chrome.storage.local.set({ [`${storageSettings.keyPrefix}${itemKey}0`]: itemValue, }); this.notifyListeners(); }lib/sessionManager/stores/localStorage.ts (2)
26-32: Fix async iteration in destroySession.The
forEachdoesn't awaitremoveSessionItem(which returns a Promise), sonotifyListeners()is called before deletions complete.Apply this diff:
async destroySession(): Promise<void> { - this.internalItems.forEach((key) => { - this.removeSessionItem(key); - }); + await Promise.all( + Array.from(this.internalItems).map((key) => this.removeSessionItem(key)) + ); this.notifyListeners(); }
40-65: Missing notification for string values.String values don't trigger notifications because the function returns at line 57 before reaching
notifyListeners()on line 64.Apply this diff:
if (typeof itemValue === "string") { splitString(itemValue, storageSettings.maxLength).forEach( (splitValue, index) => { localStorage.setItem( `${storageSettings.keyPrefix}${itemKey}${index}`, splitValue, ); }, ); + this.notifyListeners(); return; } localStorage.setItem( `${storageSettings.keyPrefix}${itemKey}0`, itemValue as string, ); this.notifyListeners(); }
🧹 Nitpick comments (2)
lib/sessionManager/stores/memory.test.ts (1)
332-335: Clarify batching expectations in comment.The comment states "listener should be called more than once but less than 3 times due to batching" but this is vague about the exact expected behavior. The assertion
expect(callCount).toBeLessThanOrEqual(3)allows for 3 calls (no batching).Consider clarifying the batching semantics:
- // Even though we made 3 changes, listener should be called more than once - // but less than 3 times due to batching + // Due to batching within the same microtask, the listener may be called + // fewer times than the number of operations (1-3 calls for 3 operations) expect(callCount).toBeGreaterThan(0); expect(callCount).toBeLessThanOrEqual(3);lib/sessionManager/stores/localStorage.test.ts (1)
355-358: Clarify batching expectations in comment.Same vague comment as in
memory.test.ts. Consider using the same clarification suggested there for consistency.- // Even though we made 3 changes, listener should be called more than once - // but less than 3 times due to batching + // Due to batching within the same microtask, the listener may be called + // fewer times than the number of operations (1-3 calls for 3 operations) expect(callCount).toBeGreaterThan(0); expect(callCount).toBeLessThanOrEqual(3);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/sessionManager/stores/chromeStore.ts(3 hunks)lib/sessionManager/stores/expoSecureStore.ts(3 hunks)lib/sessionManager/stores/localStorage.test.ts(1 hunks)lib/sessionManager/stores/localStorage.ts(3 hunks)lib/sessionManager/stores/memory.test.ts(1 hunks)lib/sessionManager/stores/memory.ts(3 hunks)lib/sessionManager/types.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/sessionManager/stores/localStorage.test.ts (1)
lib/sessionManager/stores/localStorage.ts (1)
LocalStorage(9-114)
lib/sessionManager/stores/memory.test.ts (1)
lib/sessionManager/stores/memory.ts (1)
MemoryStorage(9-92)
🪛 Biome (2.1.2)
lib/sessionManager/stores/expoSecureStore.ts
[error] 78-78: This code will never be reached ...
... because either this statement will return from the function ...
... or this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
🔇 Additional comments (11)
lib/sessionManager/stores/expoSecureStore.ts (1)
113-134: LGTM!The
notifyListeners()call is correctly placed after the deletion loop completes.lib/sessionManager/stores/memory.ts (2)
19-22: LGTM!The synchronous cache clear followed by
notifyListeners()is correct.
82-91: LGTM!The
notifyListeners()call is correctly placed after the deletion loop.lib/sessionManager/stores/chromeStore.ts (2)
29-33: LGTM!The
awaitensures all items are cleared before notifying listeners.
92-107: LGTM!The deletion loop completes before
notifyListeners()is called.lib/sessionManager/stores/localStorage.ts (1)
96-113: LGTM!The deletion loop completes before
notifyListeners()is invoked.lib/sessionManager/stores/memory.test.ts (1)
142-431: Comprehensive test coverage for subscription mechanism.The test suite thoroughly validates:
- Listener invocation on set/remove/destroy operations
- Multiple listeners and async listener support
- Unsubscribe behavior
- Batching semantics and microtask boundaries
- Deduplication of duplicate listener subscriptions
Note: These tests will fail until the missing
notifyListeners()call for string values inmemory.tssetSessionItem is added (flagged in separate comment).lib/sessionManager/stores/localStorage.test.ts (1)
164-454: Comprehensive test coverage mirroring memory store tests.The test suite validates the same subscription semantics as
memory.test.ts, ensuring consistent behavior across storage implementations.Note: These tests will fail until the missing
notifyListeners()call for string values inlocalStorage.tssetSessionItem is added (flagged in separate comment).lib/sessionManager/types.ts (3)
8-8: LGTM!The
StoreListenertype correctly supports both synchronous and asynchronous listeners.
41-82: Well-designed batching and subscription mechanism.The implementation provides:
- Deduplication via
Set<StoreListener>preventing accidental double-subscription- Batching via
notificationScheduledflag to coalesce multiple rapid calls within the same execution context- Microtask-based notification scheduling for proper async handling
Promise.allto await all listeners (both sync and async)- Clean unsubscribe pattern returning a cleanup function
The
voidoperator on line 55 makesnotifyListeners()synchronous while still scheduling the async notification properly.
163-173: LGTM!The public API additions to
SessionManagerare well-documented and provide a clear contract for the subscription mechanism.
… after the mutation completes
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/sessionManager/stores/expoSecureStore.ts (1)
63-71: Critical: Async operations in forEach not awaited, causing race condition.The
forEachloop spawns multiple async operations without awaiting them, sonotifyListeners()on line 71 executes before the storage writes complete. Listeners will be notified while chunks are still being written, potentially reading incomplete/stale data.Apply this diff to await all write operations:
if (typeof itemValue === "string") { - splitString(itemValue, Math.min(storageSettings.maxLength, 2048)).forEach( - async (splitValue, index) => { - await expoSecureStore!.setItemAsync( - `${storageSettings.keyPrefix}${itemKey}${index}`, - splitValue, - ); - }, - ); + const chunks = splitString(itemValue, Math.min(storageSettings.maxLength, 2048)); + await Promise.all( + chunks.map((splitValue, index) => + expoSecureStore!.setItemAsync( + `${storageSettings.keyPrefix}${itemKey}${index}`, + splitValue, + ) + ) + ); this.notifyListeners(); return;lib/sessionManager/stores/chromeStore.ts (1)
48-56: Critical: Async operations in forEach not awaited, causing race condition.The
forEachloop spawns multiple async chrome.storage.local.set operations without awaiting them, sonotifyListeners()on line 56 executes before the writes complete. Listeners will be notified while chunks are still being written, potentially reading incomplete data.Apply this diff to await all write operations:
if (typeof itemValue === "string") { - splitString(itemValue, storageSettings.maxLength).forEach( - async (splitValue, index) => { - await chrome.storage.local.set({ - [`${storageSettings.keyPrefix}${itemKey}${index}`]: splitValue, - }); - }, - ); + const chunks = splitString(itemValue, storageSettings.maxLength); + await Promise.all( + chunks.map((splitValue, index) => + chrome.storage.local.set({ + [`${storageSettings.keyPrefix}${itemKey}${index}`]: splitValue, + }) + ) + ); this.notifyListeners(); return;lib/sessionManager/stores/localStorage.ts (1)
48-64: Critical: Missing notifyListeners() call for string values.The string code path (lines 48-57) returns early on line 57 without calling
notifyListeners(), while the non-string path (lines 59-64) does call it. Since string values are the primary use case for session storage, listeners won't be notified for most write operations, breaking the subscription mechanism.Apply this diff to notify listeners for string values:
if (typeof itemValue === "string") { splitString(itemValue, storageSettings.maxLength).forEach( (splitValue, index) => { localStorage.setItem( `${storageSettings.keyPrefix}${itemKey}${index}`, splitValue, ); }, ); + this.notifyListeners(); return; }
🧹 Nitpick comments (1)
lib/sessionManager/stores/expoSecureStore.ts (1)
43-45: Remove redundant notifyListeners() in expoSecureStore.destroySessionSessionBase.notifyListeners() uses scheduleNotification() with a notificationScheduled guard, so multiple calls coalesce — the final this.notifyListeners() in lib/sessionManager/stores/expoSecureStore.ts (destroySession) is redundant; remove it for consistency with other stores.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/sessionManager/stores/chromeStore.ts(3 hunks)lib/sessionManager/stores/expoSecureStore.ts(3 hunks)lib/sessionManager/stores/localStorage.ts(3 hunks)lib/sessionManager/stores/memory.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/sessionManager/stores/chromeStore.ts (1)
lib/sessionManager/index.ts (1)
storageSettings(3-42)
lib/sessionManager/stores/memory.ts (1)
lib/sessionManager/index.ts (1)
storageSettings(3-42)
🔇 Additional comments (8)
lib/sessionManager/stores/memory.ts (3)
19-21: LGTM!The
destroySessionimplementation correctly clears the cache and notifies listeners synchronously.
30-51: LGTM!Both code paths (string and non-string) correctly notify listeners after completing the write operations. The synchronous nature of memory storage ensures listeners see consistent state.
83-92: LGTM!The
removeSessionItemimplementation correctly removes all matching keys and notifies listeners.lib/sessionManager/stores/chromeStore.ts (3)
29-33: LGTM!The
destroySessionimplementation correctly clears storage and notifies listeners.
59-64: LGTM!The non-string path correctly awaits the write operation before notifying listeners.
93-108: LGTM!The
removeSessionItemimplementation correctly removes all chunks sequentially and notifies listeners.lib/sessionManager/stores/localStorage.ts (2)
27-31: Verify if duplicate notifications in destroySession are intentional.
destroySessioncallsremoveSessionItemon each key viaPromise.all(line 27-29), and eachremoveSessionIteminternally callsnotifyListeners(). Then line 31 callsnotifyListeners()again. This pattern differs frommemory.tsandchromeStore.tswhich avoid the duplication. The microtask batching might coalesce these, but the intent is unclear.See the verification script in the review comment for
expoSecureStore.tsat lines 43-45 to check SessionBase's batching implementation.
96-113: LGTM!The
removeSessionItemimplementation correctly removes all chunks and notifies listeners.
…is called after values have been set
Added a subscription pattern to
SessionBaseso you can listen for changes. You can now subscribe to storage updates and do something when things change. Also added some smart batching using microtasks so we don't spam listeners when multiple operations happen at once.Tests have been updated to comprehensively test:
Chrome Storage and Expo Secure Store tests have been omitted for the time being as they're skipped entirely.
How to Use It
Node/TS
React