Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the
WalkthroughAdds cookie-based session storage (CookieStorage) with chunked cookie read/write/remove, serialization/deserialization via destr, batch ops, session destruction, CookieStorageSettings type, index re-exports, and frontend tests for Next.js App Router contexts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Handler as Server Handler / Middleware
participant CS as CookieStorage
participant NS as Next.js Cookies API
rect rgba(248,250,255,0.5)
note right of CS: Initialization (lazy)
Handler->>CS: new CookieStorage(req, resp, { persistent })
CS->>CS: ensureCookieStore()\n(middleware: resp.cookies,\nserver: cookieStore())
CS-->>Handler: instance
end
rect rgba(235,255,235,0.5)
note over Handler,CS: Set session item (chunk + write)
Handler->>CS: setSessionItem(key, value)
CS->>CS: serialize + chunk by MAX_COOKIE_LENGTH
loop per chunk
CS->>NS: set cookie(key.partN, chunk, opts)
end
CS-->>Handler: done
end
rect rgba(255,245,235,0.5)
note over Handler,CS: Get session item (read + reassemble)
Handler->>CS: getSessionItem(key)
CS->>NS: read cookies(key.part*)
CS->>CS: reassemble + parse (destr)
CS-->>Handler: value | null
end
rect rgba(255,235,240,0.5)
note over Handler,CS: Remove / Destroy
Handler->>CS: removeSessionItem(key)
CS->>NS: delete cookies(key.part*)
Handler->>CS: destroySession()
CS->>NS: delete cookies(COOKIE_LIST prefixes)
CS-->>Handler: done
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
…-20251121-1552
Bumps [next](https://github.com/vercel/next.js) from 16.0.0 to 16.1.6. - [Release notes](https://github.com/vercel/next.js/releases) - [Changelog](https://github.com/vercel/next.js/blob/canary/release.js) - [Commits](vercel/next.js@v16.0.0...v16.1.6) --- updated-dependencies: - dependency-name: next dependency-version: 16.1.6 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
…rn/next-16.1.6 chore(deps): bump next from 16.0.0 to 16.1.6
…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.
5 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.