Implement session management with express-session and MemoryStorage #136
Conversation
|
Warning Rate limit exceeded@dtoxvanilla1991 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 17 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
WalkthroughAdds a new Express-backed session store class ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressApp
participant ExpressStore
participant reqSession as req.session
Client->>ExpressApp: HTTP request (contains req)
ExpressApp->>ExpressStore: new ExpressStore(req)
ExpressStore->>reqSession: read/write/delete keys (get/set/remove)
ExpressStore-->>ExpressApp: return item / confirm write/delete
Client->>ExpressApp: request to destroy session
ExpressApp->>ExpressStore: call destroySession()
ExpressStore->>reqSession: req.session.destroy(callback)
reqSession-->>ExpressStore: callback (success/error)
ExpressStore-->>ExpressApp: resolve/reject promise
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
lib/sessionManager/stores/expressSessionManager.test.ts (1)
1-126: Critical mismatch: Test file tests wrong class.The filename
expressSessionManager.test.tssuggests this file should test theExpressSessionManagerclass, but the actual code imports and testsMemoryStorageinstead. This is a significant inconsistency that needs to be resolved.The test file should either:
- Be renamed to
memory.test.tsif it's intended to test MemoryStorage- Be rewritten to test ExpressSessionManager functionality
#!/bin/bash # Check if there's already a test file for MemoryStorage fd -e test.ts -e spec.ts | xargs grep -l "MemoryStorage" # Check if ExpressSessionManager has any other test files fd -e test.ts -e spec.ts | xargs grep -l "ExpressSessionManager"Based on the PR objectives to implement session management with express-session, this file should test the ExpressSessionManager class with Express request mocks, not MemoryStorage.
🧹 Nitpick comments (4)
lib/sessionManager/stores/expressSessionManager.ts (4)
1-2: Consider removing the @ts-expect-error suppression.The
@ts-expect-errordirective suppresses TypeScript errors, but this might mask legitimate type issues. Since express is listed as a peer dependency, consider using proper TypeScript configuration orimport typesyntax instead of suppressing the error.-// @ts-expect-error express is not in dev deps but in peer deps -import type { Request } from "express"; +import type { Request } from "express";
47-51: Consistent null handling for missing keys.The use of
?? nullensures consistent return values for missing keys, which aligns with the interface contract. However, wrapping a synchronous operation inPromise.resolve()is unnecessary overhead.- async getSessionItem(itemKey: V | StorageKeys): Promise<unknown | null> { - // ?? null ensures we consistently return null for missing keys - const itemValue = this.req.session![itemKey as string] ?? null; - return Promise.resolve(itemValue); - } + async getSessionItem(itemKey: V | StorageKeys): Promise<unknown | null> { + // ?? null ensures we consistently return null for missing keys + return this.req.session![itemKey as string] ?? null; + }
59-65: Unnecessary Promise wrapping in setSessionItem.Similar to getSessionItem, wrapping a synchronous operation in
Promise.resolve()adds unnecessary overhead.- async setSessionItem( - itemKey: V | StorageKeys, - itemValue: unknown - ): Promise<void> { - this.req.session![itemKey as string] = itemValue; - return Promise.resolve(); - } + async setSessionItem( + itemKey: V | StorageKeys, + itemValue: unknown + ): Promise<void> { + this.req.session![itemKey as string] = itemValue; + }
72-75: Unnecessary Promise wrapping in removeSessionItem.The same optimization applies here - no need to wrap synchronous operations.
- async removeSessionItem(itemKey: V | StorageKeys): Promise<void> { - delete this.req.session![itemKey as string]; - return Promise.resolve(); - } + async removeSessionItem(itemKey: V | StorageKeys): Promise<void> { + delete this.req.session![itemKey as string]; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
lib/main.ts(1 hunks)lib/sessionManager/stores/expressSessionManager.test.ts(1 hunks)lib/sessionManager/stores/expressSessionManager.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: GitGuardian Security Checks
🔇 Additional comments (2)
lib/sessionManager/stores/expressSessionManager.ts (2)
5-15: Global namespace extension is implemented correctly.The global declaration properly extends the Express Request interface to include typed session properties. This follows TypeScript best practices for module augmentation.
32-40: Constructor validation provides clear error messaging.The constructor correctly validates that the session middleware is configured before usage. The error message is descriptive and guides developers on proper setup.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/sessionManager/stores/expressStore.ts (1)
77-78: Consider adding a comment explaining the removal logicThe removal of existing items before setting is important to prevent orphaned split keys. Consider adding a comment to clarify this design decision.
- // Remove any existing split items first + // Remove any existing split items first to prevent orphaned keys + // when the new value has fewer chunks than the previous value await this.removeSessionItem(itemKey);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/main.test.ts(1 hunks)lib/main.ts(1 hunks)lib/sessionManager/index.ts(1 hunks)lib/sessionManager/stores/expressStore.test.ts(1 hunks)lib/sessionManager/stores/expressStore.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/main.test.ts
- lib/sessionManager/index.ts
- lib/main.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: DanielRivers
PR: kinde-oss/js-utils#61
File: lib/sessionManager/types.ts:20-20
Timestamp: 2025-01-16T21:47:40.307Z
Learning: Security documentation for configuration options in this codebase should be placed in the implementation file (e.g., index.ts) rather than the types file, as demonstrated with `useInsecureForRefreshToken` in `lib/sessionManager/index.ts`.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#130
File: lib/types.ts:62-66
Timestamp: 2025-06-02T08:18:51.057Z
Learning: In the kinde-oss/js-utils project, DanielRivers is comfortable with breaking changes to manual typing systems during refactoring, preferring clean migration to new naming conventions over backward compatibility aliases for types.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#69
File: lib/utils/token/refreshToken.ts:54-57
Timestamp: 2025-01-20T20:01:21.460Z
Learning: In the Kinde JS utils library, when not using a custom domain, local storage is used by default for storing refresh tokens. This is an intentional design decision that will be documented. Users can explicitly opt out of this behavior if needed.
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.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#12
File: lib/main.ts:0-0
Timestamp: 2024-09-23T22:08:18.788Z
Learning: When exporting modules in `lib/main.ts`, ensure that paths to modules like `./utils/token` are explicitly set to include `index.ts` (e.g., `./utils/token/index.ts`) to prevent missing export issues.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#12
File: lib/main.ts:0-0
Timestamp: 2024-10-08T23:57:58.113Z
Learning: When exporting modules in `lib/main.ts`, ensure that paths to modules like `./utils/token` are explicitly set to include `index.ts` (e.g., `./utils/token/index.ts`) to prevent missing export issues.
lib/sessionManager/stores/expressStore.test.ts (9)
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/token/index.test.ts:59-62
Timestamp: 2024-11-19T20:31:59.197Z
Learning: In `lib/utils/token/index.test.ts`, the test "getInsecureStorage returns active storage when no insecure storage is set" is intentional. It verifies that when insecure storage is not set, `getInsecureStorage()` returns the active storage as a fallback, preferring secure storage but allowing for insecure storage when needed by consumers of the library.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#8
File: lib/utils/token/testUtils/index.ts:3-39
Timestamp: 2024-09-19T22:17:02.939Z
Learning: The file `lib/utils/token/testUtils/index.ts` is a test utility, not production code.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#8
File: lib/utils/token/testUtils/index.ts:3-39
Timestamp: 2024-10-08T23:57:58.113Z
Learning: The file `lib/utils/token/testUtils/index.ts` is a test utility, not production code.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#61
File: lib/sessionManager/types.ts:20-20
Timestamp: 2025-01-16T21:47:40.307Z
Learning: Security documentation for configuration options in this codebase should be placed in the implementation file (e.g., index.ts) rather than the types file, as demonstrated with `useInsecureForRefreshToken` in `lib/sessionManager/index.ts`.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#12
File: lib/main.ts:0-0
Timestamp: 2024-09-23T22:08:18.788Z
Learning: When exporting modules in `lib/main.ts`, ensure that paths to modules like `./utils/token` are explicitly set to include `index.ts` (e.g., `./utils/token/index.ts`) to prevent missing export issues.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#12
File: lib/main.ts:0-0
Timestamp: 2024-10-08T23:57:58.113Z
Learning: When exporting modules in `lib/main.ts`, ensure that paths to modules like `./utils/token` are explicitly set to include `index.ts` (e.g., `./utils/token/index.ts`) to prevent missing export issues.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#15
File: lib/utils/generateAuthUrl.test.ts:104-104
Timestamp: 2024-10-25T23:53:26.124Z
Learning: In `lib/utils/generateAuthUrl.test.ts`, the code is test code and may not require strict adherence to PKCE specifications.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#16
File: lib/sessionManager/types.ts:34-42
Timestamp: 2024-10-25T15:15:13.928Z
Learning: In `lib/sessionManager/types.ts`, the `setItems` method is unlikely to be used with large batches of items, so chunking is not necessary.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#15
File: lib/utils/generateAuthUrl.ts:27-30
Timestamp: 2024-10-25T23:50:56.599Z
Learning: In `lib/utils/generateAuthUrl.ts`, `StorageKeys` is imported from `../main` and includes the `state` key.
lib/sessionManager/stores/expressStore.ts (5)
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.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#16
File: lib/sessionManager/types.ts:34-42
Timestamp: 2024-10-25T15:15:13.928Z
Learning: In `lib/sessionManager/types.ts`, the `setItems` method is unlikely to be used with large batches of items, so chunking is not necessary.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#61
File: lib/sessionManager/types.ts:20-20
Timestamp: 2025-01-16T21:47:40.307Z
Learning: Security documentation for configuration options in this codebase should be placed in the implementation file (e.g., index.ts) rather than the types file, as demonstrated with `useInsecureForRefreshToken` in `lib/sessionManager/index.ts`.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/exchangeAuthCode.ts:51-51
Timestamp: 2024-11-10T23:29:36.293Z
Learning: In `lib/utils/exchangeAuthCode.ts`, the `exchangeAuthCode` function intentionally uses `getInsecureStorage()` to store temporary authentication flow values locally between sessions. This is necessary for storing data needed for the return trip. The `getInsecureStorage()` function returns the secure storage if no active insecure storage is defined.
Learnt from: DanielRivers
PR: kinde-oss/js-utils#15
File: lib/utils/generateAuthUrl.ts:27-30
Timestamp: 2024-10-25T23:50:56.599Z
Learning: In `lib/utils/generateAuthUrl.ts`, `StorageKeys` is imported from `../main` and includes the `state` key.
🧬 Code Graph Analysis (2)
lib/sessionManager/stores/expressStore.test.ts (2)
lib/sessionManager/index.ts (3)
ExpressStore(28-28)storageSettings(3-21)StorageKeys(31-31)lib/sessionManager/stores/expressStore.ts (1)
ExpressStore(26-120)
lib/sessionManager/stores/expressStore.ts (2)
lib/main.ts (5)
ExpressStore(77-77)StorageKeys(59-59)SessionManager(76-76)storageSettings(55-55)splitString(18-18)lib/sessionManager/index.ts (4)
ExpressStore(28-28)StorageKeys(31-31)SessionManager(32-32)storageSettings(3-21)
🔇 Additional comments (1)
lib/sessionManager/stores/expressStore.ts (1)
8-18: No express-session type conflicts detectedSearched your
package.jsonand codebase—there’s no@types/express-sessiondependency and no othernamespace Expressaugmentations. The global declaration inlib/sessionManager/stores/expressStore.tsis safe as is. If you add express-session types in the future, consider moving this into a module augmentation (e.g. in a.d.tsfile withdeclare module 'express-session'or extendingExpress.Session) to avoid merging conflicts.
dtoxvanilla1991
force-pushed
the
feat/add-express-manager-storage
branch
2 times, most recently
from
ca96815 to
9158ab7
Compare
dtoxvanilla1991
force-pushed
the
feat/add-express-manager-storage
branch
from
9050e70 to
e93980f
Compare
dtoxvanilla1991
force-pushed
the
feat/add-express-manager-storage
branch
3 times, most recently
from
418f97e to
0f87128
Compare
dtoxvanilla1991
force-pushed
the
feat/add-express-manager-storage
branch
from
0f87128 to
1b80932
Compare
2 participants
Introduce session management capabilities using the
express-sessionpackage and a customMemoryStorageimplementation. Enhance TypeScript support and refactor session handling to improve type definitions and interface management.