-
Notifications
You must be signed in to change notification settings - Fork 21
feat: implement buildAuthUrl helper for consistent authorizationEndpoint handling #188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdded optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/state/KindeProvider.tsx (1)
197-212: Consider adding input validation forauthorizationEndpoint.The implementation correctly preserves the domain and query parameters by only modifying the pathname. However, consider validating that
authorizationEndpointis a reasonable path value (e.g., doesn't contain unexpected characters, protocols, or path traversal sequences like..). While settingURL.pathnameshould sanitize most issues, defensive validation would make the intent clearer and prevent potential misuse.Additionally, consider adding JSDoc to document the expected format of
authorizationEndpoint(e.g., "should be a path like/oauth/authorize").Example validation:
const buildAuthUrl = useCallback((authUrl: { url: URL }): string => { if (!authorizationEndpoint) { return authUrl.url.toString(); } + + // Validate that authorizationEndpoint is a simple path + if (authorizationEndpoint.includes('://') || authorizationEndpoint.includes('?')) { + console.warn('authorizationEndpoint should be a path, not a full URL or query string'); + } const customUrl = new URL(authUrl.url.toString());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/state/KindeProvider.tsx(8 hunks)
🔇 Additional comments (5)
src/state/KindeProvider.tsx (5)
97-97: LGTM: Clean addition of the optional prop.The
authorizationEndpointprop is correctly typed and integrated into the component signature.Also applies to: 143-143
243-243: LGTM: Correct integration ofbuildAuthUrlin login flow.The helper is properly invoked and the result is used consistently. The dependency array correctly includes
buildAuthUrl.Also applies to: 247-247, 262-262
294-299: LGTM: Consistent implementation in register flow.The register function now correctly applies the same
buildAuthUrllogic as login, addressing the inconsistency mentioned in the PR objectives. The dependency array is properly updated.Also applies to: 325-325
631-631: Clarify the necessity of optional chaining here.In a browser environment,
window.location.hrefshould always be defined. Is this change addressing a TypeScript strict mode requirement, or is there a specific scenario (e.g., SSR, test environment) wherewindow.location.hrefcould be undefined? While the defensive programming doesn't cause issues, it seems unrelated to the main feature and might indicate an underlying concern worth documenting.
197-212: Implementation verified as correct and consistent.The
buildAuthUrlhelper correctly:
- Preserves domain and query parameters by using
new URL()constructor- Handles custom
authorizationEndpointby modifying only the pathname- Normalizes leading slashes in the endpoint path
- Is used consistently in both login (line 243) and register (line 294-296) flows
- Has proper dependency tracking in the useCallback dependency array
Summary
Implements a reusable
buildAuthUrlhelper function to provide consistent custom authorization endpoint support across bothloginandregisterfunctions.Problem
The
authorizationEndpointprop was only implemented in theloginfunction with inline logic, while theregisterfunction completely ignored this prop, causing inconsistent behavior.Solution
Created a centralized
buildAuthUrlhelper function that:/if missing)Implementation Details