feat: add setItems to storage #16
Conversation
WalkthroughThe pull request includes modifications to the session management system in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (9)
lib/sessionManager/stores/chromeStore.ts (1)
21-21: Document bulk operation behaviorThe class handles Chrome storage limitations carefully. Consider adding JSDoc comments explaining how bulk operations interact with the existing chunking mechanism.
Add documentation like:
/** * Provides a chrome.store.local based session manager implementation for the browser. + * Supports bulk operations through setItems while maintaining chunking for large values. + * @see setItems for bulk storage operations * @class ChromeStore */lib/sessionManager/stores/expoSecureStore.ts (1)
Line range hint
24-31: Enhance error handling for secure storage operations.The error handling in
loadExpoStoreonly logs the error. For a secure storage implementation, we should provide more robust error handling and potentially a way to verify the store's availability.Consider implementing a store status check:
private storeAvailable = false; private async loadExpoStore() { try { expoSecureStore = await import("expo-secure-store"); this.storeAvailable = true; } catch (error) { this.storeAvailable = false; console.error("Error loading dependency expo storage:", error); throw new Error("Secure storage is not available"); } } private ensureStoreAvailable() { if (!this.storeAvailable) { throw new Error("Secure storage is not initialized"); } }Then add this check at the beginning of each storage operation method.
lib/sessionManager/stores/memory.test.ts (1)
47-58: Consider adding test cases for edge cases.While the happy path is covered, consider adding tests for:
- Empty object:
setItems({})- Invalid keys
- Null/undefined values
- Overwriting existing values
Example test cases:
it("should handle empty object gracefully", async () => { await sessionManager.setItems({}); // Assert storage state remains unchanged }); it("should handle null/undefined values", async () => { await sessionManager.setItems({ [StorageKeys.accessToken]: null, [StorageKeys.idToken]: undefined }); expect(await sessionManager.getSessionItem(StorageKeys.accessToken)).toBeNull(); expect(await sessionManager.getSessionItem(StorageKeys.idToken)).toBeNull(); }); it("should overwrite existing values", async () => { await sessionManager.setSessionItem(StorageKeys.accessToken, "oldValue"); await sessionManager.setItems({ [StorageKeys.accessToken]: "newValue" }); expect(await sessionManager.getSessionItem(StorageKeys.accessToken)).toBe("newValue"); });lib/sessionManager/stores/expoSecureStore.test.ts (2)
Line range hint
8-8: Remove.skipfrom the test suite.The test suite is currently being skipped. This prevents these tests from being executed as part of the test suite, which could lead to undetected regressions. If there's a specific reason for skipping, it should be documented; otherwise, the
.skipshould be removed.-describe.skip("ExpoSecureStore standard keys", () => { +describe("ExpoSecureStore standard keys", () => {
47-58: Enhance test coverage for setItems functionality.While the current test case verifies the basic functionality, consider adding the following test cases to ensure robust error handling and edge cases:
- Negative test cases:
- Invalid values
- Null/undefined values
- Edge cases:
- Empty object
- Single item
- Error handling scenarios:
- Storage errors
- Invalid key types
Here's a suggested additional test case for handling empty input:
it("should handle empty object in setItems", async () => { await sessionManager.setItems({}); expect(await sessionManager.getSessionItem(StorageKeys.accessToken)).toBeNull(); expect(await sessionManager.getSessionItem(StorageKeys.idToken)).toBeNull(); });lib/sessionManager/stores/chromeStore.test.ts (3)
48-59: Enhance test coverage for setItems functionality.While the basic functionality is tested, consider adding the following test cases:
- Verify atomicity - all items should be set or none
- Test error scenarios (e.g., invalid values)
- Verify that existing storage items not included in setItems remain unchanged
Consider this enhanced test structure:
it("should set many items", async () => { // Setup existing item await sessionManager.setSessionItem(StorageKeys.refreshToken, "existingValue"); // Test multiple items await sessionManager.setItems({ [StorageKeys.accessToken]: "accessTokenValue", [StorageKeys.idToken]: "idTokenValue", }); // Verify new items expect(await sessionManager.getSessionItem(StorageKeys.accessToken)).toBe("accessTokenValue"); expect(await sessionManager.getSessionItem(StorageKeys.idToken)).toBe("idTokenValue"); // Verify existing item remains unchanged expect(await sessionManager.getSessionItem(StorageKeys.refreshToken)).toBe("existingValue"); }); it("should handle errors when setting items", async () => { await expect(sessionManager.setItems({ [StorageKeys.accessToken]: null, [StorageKeys.idToken]: undefined, })).rejects.toThrow(); });
Line range hint
63-120: Add setItems test to the extra keys suite.The new setItems functionality is only tested with standard keys. To ensure consistent behavior, add a corresponding test to the "ChromeStore keys: storageKeys" suite.
Add this test to the extra keys suite:
it("should set many items with extra storage", async () => { await sessionManager.setItems({ [StorageKeys.accessToken]: "accessTokenValue", [ExtraKeys.testKey]: "extraKeyValue", }); expect(await sessionManager.getSessionItem(StorageKeys.accessToken)).toBe("accessTokenValue"); expect(await sessionManager.getSessionItem(ExtraKeys.testKey)).toBe("extraKeyValue"); });
Implement chrome.storage mock similar to localStorage mock implementation
Based on the codebase analysis, particularly the localStorage.test.ts implementation, here's how to address the skipped tests:
- Create a mock implementation for chrome.storage.local similar to the localStorage mock:
const chromeStorageMock = { local: { store: {}, get: vi.fn((keys, callback) => { callback(keys.reduce((acc, key) => ({ ...acc, [key]: store[key] }), {})); }), set: vi.fn((items, callback) => { Object.assign(store, items); callback?.(); }), remove: vi.fn((key, callback) => { delete store[key]; callback?.(); }), clear: vi.fn((callback) => { store = {}; callback?.(); }) } }; vi.stubGlobal('chrome', { storage: chromeStorageMock });
- Test cases should mirror the pattern in localStorage.test.ts, covering:
- Setting and getting items
- Removing items
- Clearing storage
- Handling string splitting for large values
🔗 Analysis chain
Line range hint
8-8: Address skipped tests and TODO comments.The test suite is currently skipped due to missing chrome storage mocks. This could lead to unreliable test coverage, especially for the new setItems functionality.
Let's check if other test files have implemented chrome storage mocking:
Would you like me to:
- Generate a mock implementation for chrome.storage?
- Create a GitHub issue to track this technical debt?
Also applies to: 63-63
Let me search for existing test mocks and patterns to help provide specific guidance.
Let me try to find test files and mock implementations with a more specific search:
Let me check the implementation of other store test files to see how they handle similar storage mocking:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for chrome storage mock implementations rg -l "chrome\.storage" --type tsLength of output: 76
Script:
#!/bin/bash # Look for mock implementations in test files rg -l "mock" --type test # Check the chrome store implementation to understand what needs to be mocked cat lib/sessionManager/stores/chromeStore.tsLength of output: 2909
Script:
#!/bin/bash # Search for test files fd -e test.ts # Look for jest mock implementations rg "jest.mock|jest.spyOn" -l # Check if there's a test setup file fd -e setup.tsLength of output: 847
Script:
#!/bin/bash # Check other store test implementations cat lib/sessionManager/stores/localStorage.test.ts cat lib/sessionManager/stores/memory.test.tsLength of output: 8688
lib/sessionManager/types.ts (1)
23-26: Add JSDoc comments to abstract methods inSessionBaseFor better maintainability and to align with the existing documentation practices, consider adding JSDoc comments to the abstract methods in
SessionBase, similar to those in theSessionManagerinterface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- lib/sessionManager/index.ts (1 hunks)
- lib/sessionManager/stores/chromeStore.test.ts (1 hunks)
- lib/sessionManager/stores/chromeStore.ts (2 hunks)
- lib/sessionManager/stores/expoSecureStore.test.ts (1 hunks)
- lib/sessionManager/stores/expoSecureStore.ts (2 hunks)
- lib/sessionManager/stores/memory.test.ts (1 hunks)
- lib/sessionManager/stores/memory.ts (1 hunks)
- lib/sessionManager/types.ts (2 hunks)
🔇 Additional comments (10)
lib/sessionManager/index.ts (2)
20-23: LGTM! Improved type exports.The change from wildcard exports to specific type exports (
SessionManagerandStorageKeys) improves the API documentation and prevents accidental exposure of internal types.
16-18: Verify the inconsistent file extensions across store implementations.While
.jsextensions are used forMemoryStorage,ChromeStore, andExpoSecureStore, theLocalStorageimport still uses.ts. This inconsistency might cause issues during compilation or bundling.Let's check the file extensions across the codebase:
lib/sessionManager/stores/memory.ts (2)
9-9: LGTM! Type safety improvements.The class signature changes enhance type safety by constraining
Vtostringand extendingSessionBase<V>while maintaining backward compatibility.
1-2: Verify the JavaScript import extensions.The file has a
.tsextension but imports from.jsfiles. This mixed usage could cause module resolution issues depending on your build setup.Let's check the project's module resolution configuration and other similar imports:
✅ Verification successful
The verification results show that:
- The project's
tsconfig.jsonhasmoduleResolutionset to "Node" andallowImportingTsExtensionsis enabled- The
.jsextension in imports is consistently used across all TypeScript files in thesessionManagermodule- This appears to be an intentional pattern used throughout the codebase
The
.jsextension usage in imports is correct and follows the project's configurationThe import statements with
.jsextensions are consistent with the TypeScript configuration and the module's import pattern. This is a valid setup that's commonly used in projects configured for ESM compatibility, where the.jsextension is required in imports even for TypeScript files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for TypeScript configuration echo "Checking for tsconfig.json settings:" cat tsconfig.json | grep -A 5 "moduleResolution" # Check for mixed .js/.ts import patterns echo -e "\nChecking for other .js imports in .ts files:" rg "from.*\.js[\"']" -g "*.ts"Length of output: 1804
lib/sessionManager/stores/chromeStore.ts (2)
2-2: LGTM: Import statement correctly updatedThe addition of
SessionBaseto the imports is appropriate for the new inheritance structure.
21-21: Verify compatibility with Chrome storage implementationWhile the inheritance structure is correct, we should verify that the inherited
setItemsimplementation fromSessionBaseis compatible with Chrome's storage limitations and chunking mechanism.Let's check the implementation of
setItemsin SessionBase:Consider the following points:
- Chrome storage has per-item size limitations
- The current
setSessionItemimplements chunking for large values- Ensure the inherited
setItemshandles these constraints correctly✅ Verification successful
Let me gather more information about the SessionBase implementation and storage settings.
Let me check the SessionBase implementation and storage settings configuration one final time.
Chrome storage implementation is compatible with inherited bulk operations
The implementation is safe because:
- ChromeStore correctly handles storage limitations through the
splitStringutility that chunks data atstorageSettings.maxLength(2000 bytes)- Each storage implementation, including ChromeStore, properly implements the chunking mechanism with unique indexed keys
- The inherited bulk operations from SessionBase will work correctly as they rely on the individual
setSessionItemimplementation that handles Chrome storage constraints🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the setItems implementation in SessionBase # and its compatibility with Chrome storage # Check SessionBase implementation ast-grep --pattern 'abstract class SessionBase { $$$ setItems($$$) { $$$ } $$$ }' # Look for any Chrome storage size limitations in comments or documentation rg -i "chrome.storage" -A 5 -B 5Length of output: 5766
Script:
#!/bin/bash # Check SessionBase implementation and storage settings # Look for setItems implementation in SessionBase ast-grep --pattern 'class SessionBase { $$$ setItems($$$) { $$$ } }' # Check storage settings rg "storageSettings" -B 2 -A 2 # Check for any chunking or size-related utilities rg "splitString" -B 2 -A 5Length of output: 19940
Script:
#!/bin/bash # Check SessionBase implementation and storage settings configuration ast-grep --pattern 'abstract class SessionBase { $$$ }' # Check storage settings configuration rg "maxLength" -B 2 -A 2Length of output: 2407
lib/sessionManager/stores/expoSecureStore.ts (2)
1-2: Verify the .js file extension usage in imports.The use of
.jsextensions in imports within a TypeScript file is unusual. While this might be intentional for your build setup, please confirm if this aligns with your project's module resolution strategy.#!/bin/bash # Check if other TypeScript files in the project use .js extensions in imports rg -t ts '\.js"' --glob '!*.test.ts' # Check if there's any documentation about this convention fd -t f "README.md" -X grep -l "typescript.*import.*\.js"
19-19:⚠️ Potential issueImplementation missing for PR's primary objective.
The PR's main objective is to add
setItemsfunctionality for bulk operations, but I don't see the implementation in this class. IfSessionBaseprovides this functionality, we should verify its implementation.If
SessionBasedoesn't provide this functionality, please implement thesetItemsmethod. Here's a suggested implementation:async setItems(items: Record<V, string>): Promise<void> { for (const [key, value] of Object.entries(items)) { await this.setSessionItem(key as V, value); } }lib/sessionManager/stores/memory.test.ts (1)
47-58: LGTM! The test case effectively validates the basic functionality.The test follows good practices with proper async/await usage and clear assertions.
lib/sessionManager/stores/expoSecureStore.test.ts (1)
47-58: Verify consistent implementation across storage types.Since this is part of a broader change adding
setItemsfunctionality, let's ensure consistent implementation across all storage types.✅ Verification successful
Let me gather more specific information about the implementation consistency.
Let me check the actual implementations with a corrected regex.
Let me try one final search with different patterns to locate the implementations.
Let me do one final check to find the actual setItems method implementations.
Implementation is consistent across storage types
The verification shows:
- All three stores (memory, chrome, and expoSecureStore) have identical test implementations for the
setItemsfunctionality- The interface in
types.tsproperly defines thesetItemsmethod signature- The test cases consistently verify setting multiple items and retrieving them individually
- The implementation pattern matches across all storage types, using the same parameter structure and return types
The implementation is uniform and follows a consistent pattern across all storage types, which is exactly what we want to see in this kind of abstraction.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent test implementation across storage types echo "Checking for setItems test implementation across storage types..." rg -l "should set many items" lib/sessionManager/stores/ # Verify the setItems method signature is consistent echo "\nChecking setItems method signature across storage implementations..." ast-grep --pattern 'setItems($items) { $$$ }'Length of output: 537
Script:
#!/bin/bash # Check the actual test implementations echo "=== Test implementations across stores ===" rg "should set many items" -A 10 lib/sessionManager/stores/ # Check the actual setItems implementations echo -e "\n=== setItems implementations across stores ===" rg "setItems.*items.*{" -A 5 lib/sessionManager/stores/Length of output: 3142
Script:
#!/bin/bash # Check the actual setItems implementations with fixed regex echo "=== setItems implementations across stores ===" rg "async setItems" -A 5 lib/sessionManager/stores/ # Also check the interface definition echo -e "\n=== Storage interface definition ===" rg "interface.*Storage" -A 10 lib/sessionManager/stores/Length of output: 304
Script:
#!/bin/bash # Search for setItems method with broader pattern echo "=== setItems implementations ===" rg "setItems" -B 1 -A 5 lib/sessionManager/stores/ # Search for Storage interface in the entire lib directory echo -e "\n=== Storage interface definition ===" rg "interface.*Storage" -B 1 -A 10 lib/Length of output: 4325
Script:
#!/bin/bash # Search for setItems method implementation in store files echo "=== setItems method implementations ===" rg "async.*setItems.*items.*" -B 1 -A 8 lib/sessionManager/stores/*.ts # Also check the method signature in the interface echo -e "\n=== Interface method signature ===" rg "setItems.*:" -A 3 lib/sessionManager/types.tsLength of output: 876
…s-utils into feat/storageSetItems # Conflicts: # lib/sessionManager/index.ts
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16 +/- ##
==========================================
+ Coverage 78.96% 79.58% +0.61%
==========================================
Files 28 28
Lines 599 622 +23
Branches 95 97 +2
==========================================
+ Hits 473 495 +22
- Misses 126 127 +1
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
lib/sessionManager/stores/chromeStore.ts (1)
Line range hint
5-15: Consider adding storage quota and performance optimizations.The
getStorageValuefunction and storage operations could benefit from:
- Storage quota checks before writing data
- Batch operations for better performance when dealing with multiple items
- Type safety improvements by replacing
unknownwith a more specific typeConsider implementing these improvements:
function getStorageValue<T>(key: string): Promise<T | undefined> { return new Promise((resolve, reject) => { try { chrome.storage.local.getBytesInUse(null, (bytesInUse) => { if (bytesInUse >= chrome.storage.local.QUOTA_BYTES) { reject(new Error('Storage quota exceeded')); return; } chrome.storage.local.get([key], function (result) { if (chrome.runtime.lastError) { reject(undefined); } else { resolve(result[key] as T); } }); }); } catch (error) { reject(undefined); } }); }lib/sessionManager/stores/localStorage.ts (4)
14-16: Enhance the warning message with specific reasons.The warning message could be more informative by explaining why localStorage shouldn't be used in production (e.g., limited storage space, synchronous API blocking the main thread, security considerations).
- console.warn("LocalStorage store should not be used in production"); + console.warn( + "LocalStorage store should not be used in production due to limited storage space, " + + "synchronous API blocking the main thread, and potential security risks" + );
Line range hint
41-57: Add error handling for storage quota exceeded.The
setSessionItemmethod should handle the QuotaExceededError that can occur when localStorage is full.async setSessionItem( itemKey: V | StorageKeys, itemValue: unknown, ): Promise<void> { // clear items first await this.removeSessionItem(itemKey); this.internalItems.add(itemKey); + try { if (typeof itemValue === "string") { splitString(itemValue, storageSettings.maxLength).forEach( (splitValue, index) => { localStorage.setItem( `${storageSettings.keyPrefix}${itemKey}${index}`, splitValue, ); }, ); return; } localStorage.setItem( `${storageSettings.keyPrefix}${itemKey}0`, itemValue as string, ); + } catch (error) { + this.internalItems.delete(itemKey); + if (error instanceof Error && error.name === 'QuotaExceededError') { + throw new Error('Storage quota exceeded. Unable to store item.'); + } + throw error; + }
25-28: Consider using a transaction-like pattern for atomic operations.The
destroySessionandremoveSessionItemmethods modify both localStorage and the internal Set. Consider implementing a transaction-like pattern to ensure consistency, especially in multi-tab scenarios.async destroySession(): Promise<void> { + const keysToRemove = Array.from(this.internalItems); + try { this.internalItems.forEach((key) => { this.removeSessionItem(key); }); + } catch (error) { + // Rollback on error + keysToRemove.forEach(key => this.internalItems.add(key)); + throw error; + } }Also applies to: 104-104
Based on the evidence gathered, I can now provide the final response:
Proper value serialization is needed before storage
The review comment is valid as the code needs to handle various types of values:
- Complex objects (like tokens created by
createMockAccessTokenwhich returns stringified JWT)- Primitive values (like
truein localStorage tests)- Potentially null/undefined values
The suggested fix properly handles all these cases by:
- Keeping strings as-is
- Converting null/undefined to empty string
- Using JSON.stringify for objects
- Converting other primitives to string
🔗 Analysis chain
Line range hint
41-57: Validate itemValue type before storage.The
setSessionItemmethod should validate thatitemValuecan be properly serialized to string before storage.async setSessionItem( itemKey: V | StorageKeys, itemValue: unknown, ): Promise<void> { // clear items first await this.removeSessionItem(itemKey); this.internalItems.add(itemKey); + let valueToStore: string; if (typeof itemValue === "string") { - splitString(itemValue, storageSettings.maxLength).forEach( + valueToStore = itemValue; + } else if (itemValue === null || itemValue === undefined) { + valueToStore = ''; + } else if (typeof itemValue === 'object') { + try { + valueToStore = JSON.stringify(itemValue); + } catch { + throw new Error('Unable to serialize item value'); + } + } else { + valueToStore = String(itemValue); + } + + splitString(valueToStore, storageSettings.maxLength).forEach( (splitValue, index) => { localStorage.setItem( `${storageSettings.keyPrefix}${itemKey}${index}`, splitValue, ); }, ); - return; - } - localStorage.setItem( - `${storageSettings.keyPrefix}${itemKey}0`, - itemValue as string, - );
Let me search for more context about the types of values being stored and how they are handled.
Let me check the implementation of
createMockAccessTokento understand the token structure and how it's serialized.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for setSessionItem usage to understand what types of values are being stored rg "setSessionItem" -A 2Length of output: 19903
Script:
#!/bin/bash # Search for createMockAccessToken to understand token structure rg "createMockAccessToken" -B 2 # Search for test files that store non-string values rg "setSessionItem.*\(" -A 1 "*.test.ts" | grep -v "\".*\""Length of output: 7964
Script:
#!/bin/bash # Look at the implementation of createMockAccessToken cat lib/utils/token/testUtils/index.ts # Check localStorage test cases for non-string values rg "setSessionItem.*\(" -A 1 "lib/sessionManager/stores/localStorage.test.ts"Length of output: 2779
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- lib/sessionManager/index.ts (1 hunks)
- lib/sessionManager/stores/chromeStore.test.ts (1 hunks)
- lib/sessionManager/stores/chromeStore.ts (2 hunks)
- lib/sessionManager/stores/expoSecureStore.ts (2 hunks)
- lib/sessionManager/stores/localStorage.ts (3 hunks)
- lib/sessionManager/stores/memory.ts (1 hunks)
- lib/sessionManager/types.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/sessionManager/index.ts
- lib/sessionManager/stores/chromeStore.test.ts
- lib/sessionManager/stores/memory.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
lib/sessionManager/stores/expoSecureStore.ts
[warning] 23-23: lib/sessionManager/stores/expoSecureStore.ts#L23
Added line #L23 was not covered by tests
🔇 Additional comments (9)
lib/sessionManager/types.ts (1)
75-80: LGTM! Well-documented interface update.The
setItemsmethod is well-documented with clear JSDoc comments and proper type definitions. The signature correctly usesPartial<Record<V, unknown>>andAwaitable<void>return type, maintaining consistency with the abstract class implementation.lib/sessionManager/stores/chromeStore.ts (3)
21-24: Verify setItems implementation for ChromeStore.The PR objective mentions adding functionality to set multiple storage items, but there's no explicit
setItemsimplementation in this class. IfSessionBaseprovides a default implementation, please verify that it:
- Correctly handles Chrome's storage limitations
- Properly manages the
storageSettings.maxLengthsplitting for string values- Maintains the same error handling pattern as other methods
#!/bin/bash # Check SessionBase for setItems implementation ast-grep --pattern 'abstract class SessionBase<$_> { $$$ setItems($$$) { $$$ } $$$ }' # Look for tests verifying setItems behavior rg -A 5 "should set many items.*ChromeStore"
21-24: Verify the dual inheritance pattern necessity.The class both extends
SessionBase<V>and implementsSessionManager<V>. This suggests thatSessionBasemight not fully implementSessionManager. Consider:
- If
SessionBaseimplements allSessionManagermethods, theimplements SessionManager<V>is redundant.- If
SessionBaseis meant to be a partial implementation, document this design decision.
1-2: Consider using.tsextension in imports for TypeScript files.The imports use
.jsextensions despite this being a TypeScript file. While this might work with specific build configurations, it's generally recommended to use.tsextensions in TypeScript source files for better IDE support and type checking.lib/sessionManager/stores/localStorage.ts (2)
2-2: LGTM! Type constraints and inheritance structure look good.The addition of
V extends stringtype constraint is appropriate for localStorage keys, and the class properly extendsSessionBase<V>while implementingSessionManager<V>.Also applies to: 9-11
18-18: LGTM! Good encapsulation of internal state.The private modifier and name change to
internalItemsbetter reflect the purpose of this property and improve encapsulation.lib/sessionManager/stores/expoSecureStore.ts (3)
1-2: LGTM: Import changes align with architectural updates.The updates to import statements correctly reflect the move to .js extensions and the new inheritance structure.
22-25:⚠️ Potential issueAdd test coverage for constructor initialization.
The
super()call in the constructor lacks test coverage. Additionally, the async initialization concern from the previous review remains valid.Consider adding the following test:
describe('ExpoSecureStore', () => { it('should properly initialize the store', async () => { const store = await ExpoSecureStore.create(); expect(store).toBeInstanceOf(ExpoSecureStore); }); });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-23: lib/sessionManager/stores/expoSecureStore.ts#L23
Added line #L23 was not covered by tests
19-21: LGTM: Enhanced type safety with proper inheritance.The class now properly extends
SessionBase<V>with improved type constraints.✅ Verification successful
The previous command might have failed due to pattern syntax. Let's try with a simpler pattern and multiple approaches to verify the inheritance structure.
I can see both ExpoSecureStore and ChromeStore extending SessionBase, but we need to verify the SessionBase definition from types.ts to fully understand the inheritance structure.
LGTM: Inheritance structure is correctly implemented
The verification confirms that:
SessionBaseis an abstract class that implementsSessionManagerinterface- Both
ExpoSecureStoreandChromeStoreproperly extendSessionBase<V>with the correct type parameter- The inheritance provides proper type constraints and shared functionality through
setItemsmethod🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the inheritance structure across storage implementations ast-grep --pattern 'class $_ extends SessionBase<$_> { $$$ }'Length of output: 65
Script:
#!/bin/bash # Let's first check if SessionBase exists and its definition rg -l "SessionBase" --type typescript # Then check for any class extending SessionBase with a different pattern ast-grep --pattern 'class $_ extends SessionBase' # Also search for class implementations in storage-related files fd "store" -e ts -x cat {}Length of output: 14528
Script:
#!/bin/bash # Let's check for the SessionBase definition to understand the inheritance structure fd "types.ts" -x cat {}Length of output: 4964
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
lib/sessionManager/types.ts (1)
34-40: Fix inconsistent indentation in the map callback.The indentation in the map callback is inconsistent with the surrounding code.
Apply this diff to fix the indentation:
async setItems(items: Partial<Record<V, unknown>>): Awaitable<void> { await Promise.all( (Object.entries(items) as [V | StorageKeys, unknown][]).map(([key, value]) => { - return this.setSessionItem(key, value); + return this.setSessionItem(key, value); } )); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lib/sessionManager/stores/localStorage.test.ts (0 hunks)
- lib/sessionManager/types.ts (2 hunks)
💤 Files with no reviewable changes (1)
- lib/sessionManager/stores/localStorage.test.ts
🔇 Additional comments (1)
lib/sessionManager/types.ts (1)
21-32: LGTM! Well-structured abstract class declaration.The abstract class is well-designed with proper type constraints and method signatures.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
lib/sessionManager/types.ts (1)
77-82: Enhance JSDoc documentation for setItems.While the basic documentation is present, consider adding:
- Usage examples
- Error scenarios and handling
- Performance considerations for large batches
/** * Sets multiple items simultaneously. + * @description + * Allows batch setting of multiple key-value pairs in storage. + * For large batches, items are processed in chunks to optimize performance. + * * @param {Record<V | StorageKeys, unknown>} items - Object containing key-value pairs to store + * @throws {Error} When any individual item fails to be set + * @example + * ```typescript + * await sessionManager.setItems({ + * [StorageKeys.accessToken]: 'token123', + * [StorageKeys.idToken]: 'id456' + * }); + * ``` * @returns {Promise<void>} */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- lib/sessionManager/types.ts (2 hunks)
🧰 Additional context used
📓 Learnings (1)
lib/sessionManager/types.ts (1)
Learnt from: DanielRivers PR: kinde-oss/js-utils#16 File: lib/sessionManager/types.ts:75-80 Timestamp: 2024-10-25T14:24:05.260Z Learning: All storage classes (`MemoryStorage`, `LocalStorage`, `ChromeStore`, `ExpoSecureStore`) extend `SessionBase`, inheriting the `setItems` method, so they do not need to implement `setItems` explicitly.
🔇 Additional comments (1)
lib/sessionManager/types.ts (1)
21-32: LGTM! Well-structured abstract class implementation.The abstract class
SessionBaseis well-designed with proper type constraints and abstract method definitions that align perfectly with theSessionManagerinterface requirements.
Explain your changes
Add option to set multiple storage items at once
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.