fix: don't return null on server routes.#343
Conversation
DanielRivers
requested review from
Yoshify
and removed request for
DaveOrDead and
peterphanouvong
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a new portal route and handler, adds a corresponding React component, and expands the routes configuration. It refactors authentication middleware and several handlers to improve error handling, token refresh logic, and prefetch request management. Additional utility functions and types are updated or introduced, and the changelog and version are incremented. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant PortalHandler
participant Session
participant Storage
participant KindeClient
Client->>Router: Request /portal
Router->>PortalHandler: Invoke portal()
PortalHandler->>Session: Get access token
alt No access token
PortalHandler->>Router: Redirect to login
else Access token exists
PortalHandler->>Storage: Store access token
PortalHandler->>KindeClient: generatePortalUrl(params)
alt Success
PortalHandler->>Router: Redirect to portal URL
else Failure
PortalHandler->>Router: Redirect to default URL
end
end
sequenceDiagram
participant Client
participant Middleware
participant KindeClient
participant Response
Client->>Middleware: Request protected route
Middleware->>KindeClient: Validate tokens
alt Tokens expired
Middleware->>KindeClient: Refresh tokens
alt Success
Middleware->>Response: Set refreshed token cookies
Middleware->>Client: Continue
else Failure
Middleware->>Client: Redirect to login
end
else Tokens valid
Middleware->>Client: Continue
end
Possibly related PRs
Suggested reviewers
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/utils/cookies/getStandardCookieOptions.ts (1)
1-14: LGTM! Consider simplifying the domain assignment.The utility function effectively centralizes cookie configuration and improves maintainability. The typing is correct and the logic is sound.
Consider simplifying the domain assignment since the ternary operator is redundant:
- domain: config.cookieDomain ? config.cookieDomain : undefined, + domain: config.cookieDomain,When
config.cookieDomainis falsy, it will already beundefinedor falsy, achieving the same result.src/utils/workQueue.ts (1)
1-5: Consider improving type safety.The
QueueItemtype usesanytypes which reduces type safety. Consider making it generic to preserve type information.-type QueueItem = { - execute: () => Promise<any>; - resolve: (value: any) => void; - reject: (error: any) => void; +type QueueItem<T = unknown> = { + execute: () => Promise<T>; + resolve: (value: T) => void; + reject: (error: any) => void; };Then update the queue property and related methods to use the generic type.
🧰 Tools
🪛 GitHub Actions: Build and test
[warning] 1-1: Prettier formatting check warning. Code style issues found. Run Prettier with --write to fix.
src/components/PortalLink.tsx (2)
17-30: Consider URL encoding for query parameters.The current implementation doesn't encode the
subNavandreturnUrlparameters, which could lead to issues if these values contain special characters.if (subNav !== undefined) { - params.append("subNav", subNav); + params.append("subNav", encodeURIComponent(subNav)); } if (returnUrl !== undefined) { - params.append("returnUrl", returnUrl); + params.append("returnUrl", encodeURIComponent(returnUrl)); }Note:
URLSearchParams.append()actually handles encoding automatically, so this change may not be necessary. Consider verifying the encoding behavior.
32-38: Simplify conditional rendering logic.The
portalUrl &&check is unnecessary sinceportalUrlis always a non-empty string based on the construction logic above.return ( - portalUrl && ( - <a href={portalUrl} {...props}> - {children} - </a> - ) + <a href={portalUrl} {...props}> + {children} + </a> );src/handlers/portal.ts (1)
36-48: Enhance error handling specificity.The current error handling catches all errors and redirects to the default URL. Consider logging more specific error information or handling different error types differently.
} catch (error) { - console.error("Portal URL generation failed:", error); + console.error("Portal URL generation failed:", { + error: error.message, + subNav: routerClient.searchParams.get("subNav"), + returnUrl, + domain: config.issuerURL + }); return routerClient.redirect(config.redirectURL); }
🛑 Comments failed to post (4)
src/utils/workQueue.ts (1)
23-23:
⚠️ Potential issueFix indentation to resolve Prettier formatting warning.
The indentation on this line is inconsistent with the surrounding code and is causing the pipeline failure.
- console.debug("enqueue: task added to queue"); + console.debug("enqueue: task added to queue");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.console.debug("enqueue: task added to queue");🤖 Prompt for AI Agents
In src/utils/workQueue.ts at line 23, the console.debug statement has inconsistent indentation causing a Prettier formatting warning. Adjust the indentation of this line to match the surrounding code block, ensuring it aligns properly with other statements at the same scope level.src/handlers/createOrg.ts (1)
1-1: 💡 Verification agent
🧩 Analysis chain
Verify import path consistency.
The import path uses
"src/utils/getHeaders"while line 4 uses"src/utils/isPreFetch". Ensure these paths are consistent with the project's import conventions.
🏁 Script executed:
#!/bin/bash # Check if the import paths follow consistent patterns across the codebase echo "Checking import path patterns for utils..." rg "from [\"']src/utils/" --type ts --type js -A 1 -B 1 echo -e "\nChecking relative vs absolute import patterns..." rg "from [\"']\.\./utils/" --type ts --type js -A 1 -B 1Length of output: 6984
Align import paths for utils to use relative imports
In
src/handlers/createOrg.ts, thegetHeadersandisPreFetchimports are using absolute paths, while the rest of the handlers import from../utils/.... To match the project convention, please update them to:-import { getHeaders } from "src/utils/getHeaders"; +import { getHeaders } from "../utils/getHeaders"; -import { isPreFetch } from "src/utils/isPreFetch"; +import { isPreFetch } from "../utils/isPreFetch";This keeps all utility imports consistent across handlers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.-import { getHeaders } from "src/utils/getHeaders"; +import { getHeaders } from "../utils/getHeaders"; -import { isPreFetch } from "src/utils/isPreFetch"; +import { isPreFetch } from "../utils/isPreFetch";🤖 Prompt for AI Agents
In src/handlers/createOrg.ts at line 1, the import path for getHeaders uses an absolute path "src/utils/getHeaders" which is inconsistent with the project's convention of using relative imports for utilities. Change the import path to a relative path like "../utils/getHeaders" to align with other handlers and maintain consistency in import statements.src/handlers/portal.ts (1)
23-24: 💡 Verification agent
🧩 Analysis chain
Consider memory storage cleanup.
The memory storage is created and set as active but there's no cleanup mechanism. Consider if this could lead to memory leaks in long-running processes.
🏁 Script executed:
#!/bin/bash # Check how memory storage is used across the codebase and if cleanup is handled echo "Checking MemoryStorage usage patterns..." rg "MemoryStorage|setActiveStorage" --type ts --type js -A 3 -B 1 echo -e "\nChecking for any cleanup patterns..." rg "cleanup|clear|reset.*storage" --type ts --type js -A 2 -B 2Length of output: 1692
Add explicit cleanup for MemoryStorage to prevent memory leaks
Currently, we instantiate and activate an in-memory store without ever clearing or disposing it, which can lead to unbounded memory growth in long-running processes.• File:
src/handlers/portal.ts, lines 23–24const storage = new MemoryStorage(); setActiveStorage(storage);Recommendations:
- If
MemoryStoragesupports a clear or dispose method, invoke it once the storage is no longer needed:await doYourWork(); storage.clear(); // or storage.dispose(); setActiveStorage(null);- Otherwise, extend
MemoryStoragewith a cleanup API and ensure it’s called at the end of the request/handler lifecycle.🤖 Prompt for AI Agents
In src/handlers/portal.ts at lines 23-24, the MemoryStorage instance is created and set active without any cleanup, risking memory leaks in long-running processes. To fix this, check if MemoryStorage has a clear or dispose method and call it when the storage is no longer needed, such as at the end of the handler or request lifecycle. Also, reset the active storage by calling setActiveStorage(null) after cleanup. If no cleanup method exists, extend MemoryStorage to add one and ensure it is invoked appropriately.src/handlers/callback.ts (1)
8-14: 🛠️ Refactor suggestion
Fix variable naming conflict to avoid confusion.
The variable
reauthStateis used both for the search parameter (line 8) and the parsed JSON object (line 12), which creates confusion and potential for bugs.Apply this diff to use distinct variable names:
- const reauthState = routerClient.getSearchParam("reauth_state"); - if (reauthState) { - const decodedAuthState = atob(reauthState); + const reauthStateParam = routerClient.getSearchParam("reauth_state"); + if (reauthStateParam) { + const decodedAuthState = atob(reauthStateParam); try { const reauthState = JSON.parse(decodedAuthState);🤖 Prompt for AI Agents
In src/handlers/callback.ts lines 8 to 14, the variable name reauthState is used twice for different purposes, causing a naming conflict. Rename the parsed JSON object variable on line 12 to a distinct name like parsedReauthState to avoid confusion and potential bugs while keeping the original reauthState for the search parameter.
2 participants
Explain your changes
Replaced null result on prefetch requests
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.