Feat/new session store#406
Conversation
|
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:
WalkthroughAdds a new ChangesCookie Storage Session Manager
Sequence DiagramsequenceDiagram
participant RequestHandler
participant CookieStorage
participant CookieStore
participant ClientCookies
RequestHandler->>CookieStorage: new CookieStorage(req, resp, options)
RequestHandler->>CookieStorage: setSessionItem(key, value)
CookieStorage->>CookieStorage: ensureCookieStore() - select middleware or server
CookieStorage->>CookieStorage: serialize and chunk value
CookieStorage->>CookieStore: set prefixed chunk cookies
CookieStore->>ClientCookies: write via req/resp or cookies()
RequestHandler->>CookieStorage: getSessionItem(key)
CookieStorage->>CookieStore: read base cookie + chunks
CookieStore-->>CookieStorage: concatenated chunk data
CookieStorage->>CookieStorage: parse via destr
CookieStorage-->>RequestHandler: value or null
RequestHandler->>CookieStorage: destroySession()
CookieStorage->>CookieStore: enumerate & delete matching cookies
CookieStore->>ClientCookies: clear via maxAge: 0
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/session/sessionManager/cookieManager.ts(1 hunks)src/session/sessionManager/index.ts(1 hunks)src/session/sessionManager/settings.ts(1 hunks)src/session/sessionManager/types.ts(1 hunks)tests/frontend/cookie-manager.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-13T10:45:31.961Z
Learnt from: DanielRivers
PR: kinde-oss/kinde-auth-nextjs#229
File: src/session/sessionManager.js:165-188
Timestamp: 2024-11-13T10:45:31.961Z
Learning: There's a new implementation coming soon for `src/session/sessionManager.js`, so refactoring suggestions related to cookie parsing logic may not be necessary at this time.
Applied to files:
src/session/sessionManager/cookieManager.ts
🧬 Code graph analysis (4)
src/session/sessionManager/types.ts (1)
src/session/sessionManager/index.ts (2)
StorageKeys(8-8)SessionManager(9-9)
tests/frontend/cookie-manager.test.ts (3)
src/session/sessionManager.js (1)
options(27-27)src/session/sessionManager/cookieManager.ts (1)
CookieStorage(24-146)src/utils/constants.ts (2)
MAX_COOKIE_LENGTH(23-23)COOKIE_LIST(12-21)
src/session/sessionManager/settings.ts (1)
src/session/sessionManager/types.ts (1)
StorageSettingsType(15-27)
src/session/sessionManager/cookieManager.ts (4)
src/session/sessionManager/settings.ts (1)
CookieStorageSettings(44-49)src/utils/constants.ts (4)
MAX_COOKIE_LENGTH(23-23)COOKIE_LIST(12-21)GLOBAL_COOKIE_OPTIONS(5-10)TWENTY_NINE_DAYS(3-3)src/session/sessionManager/types.ts (1)
SessionManager(61-105)src/session/sessionManager.js (1)
options(27-27)
🪛 GitHub Actions: Build and test
src/session/sessionManager/index.ts
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
tests/frontend/cookie-manager.test.ts
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
src/session/sessionManager/settings.ts
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
src/session/sessionManager/cookieManager.ts
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/session/sessionManager/cookieManager.ts (1)
33-42: Fix constructor typing to allow session cookies.The constructor's
options: { persistent: true }type prevents callers from passingpersistent: false, breaking session cookie functionality.Apply this diff:
constructor( - req: NextApiRequest, - resp: NextApiResponse, - options: { persistent: true }, + req: NextApiRequest | undefined, + resp: NextApiResponse | undefined, + options: { persistent: boolean } = { persistent: true }, ) { super(); this.req = req; this.resp = resp; this.sessionState = options; }
🧹 Nitpick comments (4)
tests/frontend/cookie-manager.test.ts (1)
42-50: Consider testing session cookie behavior.The test setup always uses
persistent: true. Consider adding tests forpersistent: falseto verify session cookie behavior (nomaxAgeset).For example:
it("creates session cookies when persistent is false", async () => { const sessionStorage = new CookieStorage<any>(undefined as any, undefined as any, { persistent: false, }); // @ts-ignore sessionStorage._cookieStore = fake as any; await sessionStorage.setSessionItem(StorageKeys.state, "test"); // Verify cookie was set without maxAge (session cookie) const cookie = fake.get(StorageKeys.state); expect(cookie).toBeDefined(); // In real implementation, session cookies have undefined maxAge });src/session/sessionManager/cookieManager.ts (3)
69-83: Refactor cookie name extraction for clarity.The current pattern chains
.map()and.forEach()on separate lines. Consider combining into a single iteration for better readability.Apply this diff:
async destroySession(): Promise<void> { const cookieStore = await this.ensureCookieStore(); - cookieStore - .getAll() - .map((c) => c.name) - .forEach((key) => { - if (COOKIE_LIST.some((substr) => key.startsWith(substr))) { - cookieStore.set(key, "", { - domain: config.cookieDomain ? config.cookieDomain : undefined, - maxAge: 0, - ...GLOBAL_COOKIE_OPTIONS, - }); - } - }); + for (const { name } of cookieStore.getAll()) { + if (COOKIE_LIST.some((substr) => name.startsWith(substr))) { + cookieStore.set(name, "", { + domain: config.cookieDomain ? config.cookieDomain : undefined, + maxAge: 0, + ...GLOBAL_COOKIE_OPTIONS, + }); + } + } }
88-116: LGTM with optional refactor.The chunking and serialization logic correctly handles objects, primitives, and undefined values. Cookie naming and expiry settings are appropriate.
Consider the same iteration refactor as suggested for
destroySession:async setSessionItem( itemKey: V | StorageKeys, itemValue: unknown, ): Promise<void> { const cookieStore = await this.ensureCookieStore(); - cookieStore - .getAll() - .map((c) => c.name) - .forEach((key) => { - if (key.startsWith(`${String(itemKey)}`)) { - cookieStore.delete(key); - } - }); + for (const { name } of cookieStore.getAll()) { + if (name.startsWith(`${String(itemKey)}`)) { + cookieStore.delete(name); + } + } if (itemValue !== undefined) { // ... rest remains the same } }
145-155: LGTM with optional refactor.The removal logic correctly deletes all cookie chunks for the given key.
Apply the same iteration refactor as suggested for
destroySessionandsetSessionItem:async removeSessionItem(itemKey: V | StorageKeys): Promise<void> { const cookieStore = await this.ensureCookieStore(); - cookieStore - .getAll() - .map((c) => c.name) - .forEach((key) => { - if (key.startsWith(`${String(itemKey)}`)) { - cookieStore.delete(key); - } - }); + for (const { name } of cookieStore.getAll()) { + if (name.startsWith(`${String(itemKey)}`)) { + cookieStore.delete(name); + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/session/sessionManager/cookieManager.ts(1 hunks)src/session/sessionManager/index.ts(1 hunks)src/session/sessionManager/settings.ts(1 hunks)tests/frontend/cookie-manager.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/session/sessionManager/settings.ts
- src/session/sessionManager/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-13T10:45:31.961Z
Learnt from: DanielRivers
PR: kinde-oss/kinde-auth-nextjs#229
File: src/session/sessionManager.js:165-188
Timestamp: 2024-11-13T10:45:31.961Z
Learning: There's a new implementation coming soon for `src/session/sessionManager.js`, so refactoring suggestions related to cookie parsing logic may not be necessary at this time.
Applied to files:
src/session/sessionManager/cookieManager.ts
🧬 Code graph analysis (2)
tests/frontend/cookie-manager.test.ts (3)
src/session/sessionManager.js (1)
options(27-27)src/session/sessionManager/cookieManager.ts (1)
CookieStorage(23-156)src/utils/constants.ts (2)
MAX_COOKIE_LENGTH(23-23)COOKIE_LIST(12-21)
src/session/sessionManager/cookieManager.ts (3)
src/session/sessionManager/settings.ts (1)
CookieStorageSettings(44-49)src/utils/constants.ts (4)
MAX_COOKIE_LENGTH(23-23)COOKIE_LIST(12-21)GLOBAL_COOKIE_OPTIONS(5-10)TWENTY_NINE_DAYS(3-3)src/session/sessionManager/types.ts (1)
SessionManager(61-105)
🔇 Additional comments (4)
tests/frontend/cookie-manager.test.ts (3)
1-5: LGTM!Clean imports using the public API surface and test utilities.
6-36: LGTM!The
FakeCookieStoremock implementation accurately mirrors the cookie store API behavior, including correct handling of expired cookies viamaxAge <= 0.
52-154: LGTM!Comprehensive test coverage of core functionality:
- Chunking and reassembly with correct naming conventions
- Serialization/deserialization via
destr- Removal of chunked cookies
- Prefix-based destruction
- Edge cases (undefined, primitives, objects)
src/session/sessionManager/cookieManager.ts (1)
47-63: Verify CookieManager constructor usage
Check allnew CookieManager(...)instantiations to ensurereqis only omitted (undefined) in App Router contexts so thecookies()fallback isn’t ever reached in Pages Router or middleware.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/session/sessionManager/cookieManager.ts (1)
123-145: Reconsider fallback toitem.valueon parse failure.Line 143 falls back to
item.value(first chunk only) whenitemValueis falsy. If parsing fails but multiple chunks were successfully reassembled into an empty or falsyitemValue, returning only the first chunk loses data. Consider:- return itemValue || item.value; + return itemValue;This ensures the full reassembled string is returned even when parsing fails, maintaining data integrity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/session/sessionManager/cookieManager.ts(1 hunks)tests/frontend/cookie-manager.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/frontend/cookie-manager.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-13T10:45:31.961Z
Learnt from: DanielRivers
PR: kinde-oss/kinde-auth-nextjs#229
File: src/session/sessionManager.js:165-188
Timestamp: 2024-11-13T10:45:31.961Z
Learning: There's a new implementation coming soon for `src/session/sessionManager.js`, so refactoring suggestions related to cookie parsing logic may not be necessary at this time.
Applied to files:
src/session/sessionManager/cookieManager.ts
🧬 Code graph analysis (1)
src/session/sessionManager/cookieManager.ts (3)
src/session/sessionManager/settings.ts (1)
CookieStorageSettings(44-49)src/utils/constants.ts (4)
MAX_COOKIE_LENGTH(23-23)COOKIE_LIST(12-21)GLOBAL_COOKIE_OPTIONS(5-10)TWENTY_NINE_DAYS(3-3)src/session/sessionManager/types.ts (1)
SessionManager(61-105)
🔇 Additional comments (9)
src/session/sessionManager/cookieManager.ts (9)
1-15: LGTM!All imports are appropriate for the cookie-based session storage implementation. The use of
destrfor safe JSON parsing aligns with best practices for handling untrusted data.
17-21: LGTM!The storage settings are correctly configured with the unified
"kinde-"prefix and appropriate security defaults.
23-26: LGTM!Class declaration correctly extends
SessionBaseand implementsSessionManagerwith appropriate generic constraints.
27-31: LGTM!Class properties are correctly typed with appropriate visibility modifiers and sensible defaults.
33-42: LGTM!Constructor signature correctly allows both persistent and session cookies, addressing previous feedback.
88-118: LGTM!The method correctly:
- Applies the unified prefix
- Clears stale chunks before writing new ones
- Handles serialization for both objects and primitives
- Chunks large values appropriately
- Respects the
persistentflag formaxAge
150-161: LGTM!The method correctly removes all chunked cookies associated with the given key by matching the prefix.
69-83: Use mutable RequestCookies for cookie mutations
EnsureensureCookieStorereturnsRequestCookies(fromnext/headers) instead ofReadonlyRequestCookies, sinceset()/delete()require a mutable store.
47-63: Asymmetric App Router enforcement is correct. Whenreqis undefined you’re already in App Router context; when provided,isAppRouter(req)accurately guards for webRequest. No changes needed.
| * Clears all tracked items from the cookie store. | ||
| * @returns {void} | ||
| */ | ||
| async destroySession(): Promise<void> { |
There was a problem hiding this comment.
This unfortunately won't work in middleware in Next versions <15, it'll throw Error: Cookies can only be modified in a Server Action or Route Handler. - this applies to all mutations on cookies. We'd need to mutate the cookies on the req/res objects. See https://github.com/kinde-oss/kinde-auth-nextjs/blob/c8599cc4d0a8b6ef76e71ebac823dcc6dc526b10/src/authMiddleware/authMiddleware.ts
The reason we'd need to also mutate the cookies on the request and not just the response is during SSR in Next versions prior to 14.2.8, Cookies are read from the Set-Cookie from the request, not the response. See this part of the middleware specifically: https://github.com/kinde-oss/kinde-auth-nextjs/blob/c8599cc4d0a8b6ef76e71ebac823dcc6dc526b10/src/authMiddleware/authMiddleware.ts#L166C7-L170C39
It's a big gotcha that they resolved in 15 and above thankfully, but ideally we should continue supporting 14 as a good portion of the Next userbase is still on there.
There was a problem hiding this comment.
hey @Yoshify, I'd love a chance to discuss this with you further so I can better understand the desired flow within NextJS.
| /** | ||
| * Sets the provided key-value store into cookies (split into chunks if needed). | ||
| */ | ||
| async setSessionItem( |
| /** | ||
| * Removes all cookie parts associated with the provided key. | ||
| */ | ||
| async removeSessionItem(itemKey: V | StorageKeys): Promise<void> { |
| this._cookieStore = await cookies(); | ||
| return this._cookieStore; | ||
| } else { | ||
| throw new Error("This store is to be used for App Router."); |
There was a problem hiding this comment.
Instead of throwing here we should probably use the request cookies to support middleware in v14> <v15, that will also potentially allow us to use the same store for pages router. This shouldn't be too difficult given we have the req/res objects already. https://github.com/kinde-oss/kinde-auth-nextjs/blob/c8599cc4d0a8b6ef76e71ebac823dcc6dc526b10/src/authMiddleware/authMiddleware.ts has some good examples of this.
There was a problem hiding this comment.
Hey @Yoshify I've made some changes to the way this works. Can you re-review please?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/session/sessionManager/cookieManager.ts (3)
110-176: Consider extracting the serialization logicThe cookie serialization and chunking logic is duplicated between the middleware and non-middleware branches (lines 132-150 and 158-174). While the current implementation is correct, extracting this into a helper function would improve maintainability.
Example refactor:
private prepareChunks(itemValue: unknown): string[] { if (itemValue === undefined) return []; const itemValueString = typeof itemValue === "object" ? JSON.stringify(itemValue) : String(itemValue); return splitString(itemValueString, MAX_COOKIE_LENGTH); }Then use it in both branches:
const chunks = this.prepareChunks(itemValue); chunks.forEach((value, index) => { // set cookie with appropriate API });
232-236: Consider parallelizing batch operationsThe method sequentially awaits each
setSessionItemcall. If ordering is not critical, you could improve performance by parallelizing withPromise.all.async setItems(items: Partial<Record<V, unknown>>): Promise<void> { await Promise.all( Object.entries(items).map(([key, value]) => this.setSessionItem(key as V, value) ) ); }
241-245: Consider parallelizing batch operationsSimilar to
setItems, this method could benefit from parallelization if ordering is not critical.async removeItems(...items: V[]): Promise<void> { await Promise.all(items.map((item) => this.removeSessionItem(item))); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (3)
src/session/sessionManager/cookieManager.ts(1 hunks)src/session/sessionManager/index.ts(1 hunks)src/session/sessionManager/settings.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/session/sessionManager/settings.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-13T10:45:31.961Z
Learnt from: DanielRivers
PR: kinde-oss/kinde-auth-nextjs#229
File: src/session/sessionManager.js:165-188
Timestamp: 2024-11-13T10:45:31.961Z
Learning: There's a new implementation coming soon for `src/session/sessionManager.js`, so refactoring suggestions related to cookie parsing logic may not be necessary at this time.
Applied to files:
src/session/sessionManager/cookieManager.ts
📚 Learning: 2024-12-17T00:41:07.608Z
Learnt from: Yoshify
PR: kinde-oss/kinde-auth-nextjs#254
File: src/session/isAuthenticated.js:14-17
Timestamp: 2024-12-17T00:41:07.608Z
Learning: In `src/session/isAuthenticated.js` of this Next.js application, cookies cannot be modified in React Server Components (RSC). Therefore, to prevent accessing stale data outside of middleware, the application redirects on token expiry.
Applied to files:
src/session/sessionManager/cookieManager.ts
🧬 Code graph analysis (1)
src/session/sessionManager/cookieManager.ts (3)
src/session/sessionManager/settings.ts (1)
CookieStorageSettings(3-8)src/utils/constants.ts (4)
MAX_COOKIE_LENGTH(23-23)COOKIE_LIST(12-21)GLOBAL_COOKIE_OPTIONS(5-10)TWENTY_NINE_DAYS(3-3)src/session/sessionManager.js (1)
options(27-27)
🔇 Additional comments (8)
src/session/sessionManager/index.ts (1)
1-4: LGTM!The re-exports are clean and follow proper TypeScript conventions. The previous issue with file extensions has been correctly addressed.
src/session/sessionManager/cookieManager.ts (7)
1-15: LGTM!The imports correctly use Next.js App Router types (
NextRequest,NextResponse) rather than Pages Router types, addressing previous feedback.
17-21: LGTM!The settings are well-defined, and the
keyPrefixis consistently applied across all cookie operations in the class methods.
31-42: LGTM!The constructor correctly accepts both persistent and session-scoped cookie configurations, addressing the previous type constraint issue.
48-50: LGTM!Simple and correct helper for detecting middleware context.
79-105: LGTM!The method correctly differentiates between middleware and non-middleware contexts for cookie deletion. The middleware path properly uses
resp.cookiesfor mutations to support Next.js < 15.
181-203: LGTM!The method correctly reassembles chunked cookies with proper null-checking to prevent race conditions, and safely parses values using
destr.
208-227: LGTM!The method correctly removes all cookie chunks with proper context differentiation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/session/sessionManager/cookieManager.ts (2)
232-236: Consider parallel execution for better performance.The sequential
awaitcalls may be slower when setting multiple items. While the current implementation is correct and easier to reason about, parallel execution could improve performance.If you'd like to optimize, apply this diff:
async setItems(items: Partial<Record<V, unknown>>): Promise<void> { - for (const [key, value] of Object.entries(items)) { - await this.setSessionItem(key as V, value); - } + await Promise.all( + Object.entries(items).map(([key, value]) => + this.setSessionItem(key as V, value) + ) + ); }Note: Only apply this optimization if profiling shows it's beneficial, as sequential execution may be preferred for debugging or when order matters.
241-245: Consider parallel execution for better performance.Similar to
setItems, the sequentialawaitcalls may be slower when removing multiple items.If you'd like to optimize:
async removeItems(...items: V[]): Promise<void> { - for (const item of items) { - await this.removeSessionItem(item); - } + await Promise.all(items.map((item) => this.removeSessionItem(item))); }Note: Only apply if profiling shows benefit, as sequential execution may be preferred for debugging or when order matters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (1)
src/session/sessionManager/cookieManager.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-13T10:45:31.961Z
Learnt from: DanielRivers
PR: kinde-oss/kinde-auth-nextjs#229
File: src/session/sessionManager.js:165-188
Timestamp: 2024-11-13T10:45:31.961Z
Learning: There's a new implementation coming soon for `src/session/sessionManager.js`, so refactoring suggestions related to cookie parsing logic may not be necessary at this time.
Applied to files:
src/session/sessionManager/cookieManager.ts
📚 Learning: 2024-12-17T00:41:07.608Z
Learnt from: Yoshify
PR: kinde-oss/kinde-auth-nextjs#254
File: src/session/isAuthenticated.js:14-17
Timestamp: 2024-12-17T00:41:07.608Z
Learning: In `src/session/isAuthenticated.js` of this Next.js application, cookies cannot be modified in React Server Components (RSC). Therefore, to prevent accessing stale data outside of middleware, the application redirects on token expiry.
Applied to files:
src/session/sessionManager/cookieManager.ts
🧬 Code graph analysis (1)
src/session/sessionManager/cookieManager.ts (3)
src/session/sessionManager/settings.ts (1)
CookieStorageSettings(3-8)src/utils/constants.ts (4)
MAX_COOKIE_LENGTH(23-23)COOKIE_LIST(12-21)GLOBAL_COOKIE_OPTIONS(5-10)TWENTY_NINE_DAYS(3-3)src/session/sessionManager.js (1)
options(27-27)
🔇 Additional comments (8)
src/session/sessionManager/cookieManager.ts (8)
17-21: LGTM!The cookie storage settings are properly configured with sensible defaults. The keyPrefix alignment and maxLength constraint prevent cookie size issues.
23-42: LGTM!The class declaration and constructor are well-structured. The constructor properly accepts both persistent modes, resolving the previous type constraint issue.
44-50: LGTM!The middleware context detection is correct and well-documented. The comment clearly explains the Next.js < 15 requirement.
75-105: LGTM!The session destruction logic correctly handles both middleware and server contexts. The filtering by
COOKIE_LISTprefixes and settingmaxAge: 0is the proper approach for cookie deletion.
110-176: LGTM with a minor note on serialization.The chunking logic and dual-path handling for middleware/server contexts are well-implemented. The keyPrefix is now correctly applied to all operations.
One minor consideration: Line 135 uses
String(itemValue)for non-object primitives. This works for numbers and booleans but convertsnullto"null"string. Ensure upstream code doesn't passnullwhenundefinedis intended, or add explicit null handling if needed.
181-203: LGTM!The reassembly logic with null guard (line 191-192) properly handles edge cases. The use of
destrfor safe parsing and the debug logging for parse failures follow best practices.Based on learnings.
208-227: LGTM!The removal logic correctly handles all chunks and maintains consistency with the dual-path approach used throughout the class.
55-73: Add middleware context branch for Next.js < 14.2.8
Current code always callsawait cookies()in middleware, so mutations viaresp.cookiesaren’t visible in Next.js < 14.2.8. Introduce before the existing branches:private async ensureCookieStore(): Promise<ReadonlyRequestCookies> { if (this._cookieStore) return this._cookieStore; + if (this.isMiddlewareContext() && this.req) { + this._cookieStore = this.req.cookies; + return this._cookieStore; + } if (!this.req) { this._cookieStore = await cookies();Confirm that
NextRequest.cookiesis compatible withReadonlyRequestCookies.
…ionBase and sessionManager with tests
…kinde-auth-nextjs into feat/new-session-store
dtoxvanilla1991
left a comment
dtoxvanilla1991
left a comment
There was a problem hiding this comment.
Good work! Some feedback 💭
| .map((c) => c.name) | ||
| .filter((key) => COOKIE_LIST.some((substr) => key.startsWith(substr))); | ||
|
|
||
| // In middleware context, use resp.cookies (Next.js < 15 requirement) |
There was a problem hiding this comment.
destroySession() currently filters only by COOKIE_LIST prefixes, but this class writes session keys using kinde- prefixed names. That mismatch can leave prefixed auth cookies behind after logout/session destruction. Please align delete matching with the same namespace strategy used by setSessionItem/getSessionItem/removeSessionItem (and preserve legacy cleanup if needed).
| this._cookieStore = cookies(); | ||
| return this._cookieStore; | ||
| } | ||
|
|
||
| if (isAppRouter(this.req)) { | ||
| this._cookieStore = cookies(); |
There was a problem hiding this comment.
cookies() in current Next.js versions is async and returns a promise-backed cookie store (Promise<ReadonlyRequestCookies>). Assigning it directly here risks runtime/type drift in non-middleware paths, and subsequent .set/.delete calls can fail depending on context. Please resolve this branch to await and normalize store type consistently with the rest of ensureCookieStore.
There was a problem hiding this comment.
@dtoxvanilla1991 Can you please verify this is done to your satisfaction?
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/session/sessionManager/cookieManager.ts (1)
10-14:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix Next.js 12 compatibility: unguarded
next/headersimports
src/session/sessionManager/cookieManager.tsimportscookiesfromnext/headers.js(and App Router types) at module scope.next/headerswas introduced in Next 13, so Next12.3.5installs (allowed by this package’speerDependencies.next) will fail when bundling. Also,src/session/sessionManager.jshas a top-levelimport { cookies } from "next/headers";, so the incompatibility isn’t limited tocookieManager.ts.🤖 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/session/sessionManager/cookieManager.ts` around lines 10 - 14, The module imports the runtime symbol cookies from "next/headers" at top-level (in cookieManager.ts and sessionManager.js), which breaks on Next 12; move the runtime import into the functions that actually call cookies (i.e., replace the top-level import of the cookies symbol with a dynamic/delayed require or import inside the function that uses cookies), wrap that import in a try/catch to provide a clear fallback/error for Next 12 users, and remove or convert any non-runtime-only imports to type-only imports if necessary; specifically, eliminate the top-level `import { cookies } from "next/headers"` and instead perform a guarded dynamic import within the functions in cookieManager.ts (and the analogous location in sessionManager.js) so bundlers on Next 12 won’t attempt to resolve next/headers at module-evaluation time.
♻️ Duplicate comments (1)
src/session/sessionManager/cookieManager.ts (1)
82-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror middleware mutations into the request cookie store too.
ensureCookieStore()cachesthis.req.cookies, but the middleware branches only mutatethis.resp.cookies. After the first set/delete, later reads and cleanup in the same request still operate on stale request data, sogetSessionItem()/destroySession()can miss cookies written earlier in that request. Updatereq.cookiesalongsideresp.cookies, or replace_cookieStorewith a mutable shadow store after the first mutation.#!/bin/bash set -euo pipefail echo "Current CookieStorage middleware branches:" sed -n '82,235p' src/session/sessionManager/cookieManager.ts echo echo "Existing middleware cookie-sync patterns in the repo:" fd -p 'authMiddleware.ts' src | while read -r file; do echo "FILE: $file" rg -n "copyCookiesToRequest|req\.cookies\.(set|delete)|resp\.cookies\.(set|delete)" "$file" -C2 doneAlso applies to: 117-135, 162-185, 219-234
🤖 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/session/sessionManager/cookieManager.ts` around lines 82 - 89, ensureCookieStore() currently caches this.req.cookies but middleware branches only mutate this.resp.cookies, leaving this._cookieStore stale and causing getSessionItem()/destroySession() to miss in-request changes; update the middleware branches in cookieManager.ts (methods using isMiddlewareContext(), ensureCookieStore(), and any code that calls resp.cookies.set/delete) to mirror mutations into this.req.cookies as they occur or, after the first mutation, replace this._cookieStore with a mutable shadow store and route subsequent reads/writes to that shadow so both reads (getSessionItem) and cleanup (destroySession) see up-to-date cookie state. Ensure changes touch the same unique symbols: this._cookieStore, this.req.cookies, this.resp.cookies, ensureCookieStore(), isMiddlewareContext(), getSessionItem(), and destroySession().
🤖 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.
Outside diff comments:
In `@src/session/sessionManager/cookieManager.ts`:
- Around line 10-14: The module imports the runtime symbol cookies from
"next/headers" at top-level (in cookieManager.ts and sessionManager.js), which
breaks on Next 12; move the runtime import into the functions that actually call
cookies (i.e., replace the top-level import of the cookies symbol with a
dynamic/delayed require or import inside the function that uses cookies), wrap
that import in a try/catch to provide a clear fallback/error for Next 12 users,
and remove or convert any non-runtime-only imports to type-only imports if
necessary; specifically, eliminate the top-level `import { cookies } from
"next/headers"` and instead perform a guarded dynamic import within the
functions in cookieManager.ts (and the analogous location in sessionManager.js)
so bundlers on Next 12 won’t attempt to resolve next/headers at
module-evaluation time.
---
Duplicate comments:
In `@src/session/sessionManager/cookieManager.ts`:
- Around line 82-89: ensureCookieStore() currently caches this.req.cookies but
middleware branches only mutate this.resp.cookies, leaving this._cookieStore
stale and causing getSessionItem()/destroySession() to miss in-request changes;
update the middleware branches in cookieManager.ts (methods using
isMiddlewareContext(), ensureCookieStore(), and any code that calls
resp.cookies.set/delete) to mirror mutations into this.req.cookies as they occur
or, after the first mutation, replace this._cookieStore with a mutable shadow
store and route subsequent reads/writes to that shadow so both reads
(getSessionItem) and cleanup (destroySession) see up-to-date cookie state.
Ensure changes touch the same unique symbols: this._cookieStore,
this.req.cookies, this.resp.cookies, ensureCookieStore(), isMiddlewareContext(),
getSessionItem(), and destroySession().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 468b1009-cf5b-47ba-8ade-823da8f0909d
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (2)
src/session/sessionManager/cookieManager.tstests/frontend/cookie-manager.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/frontend/cookie-manager.test.ts
@DanielRivers @Yoshify Should I implement this nitpick? Or is it not necessary? |
4 participants
Explain your changes
Create new NextJs cookie store for the app router.
Ensure new cookie store is testable.
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.