Refactor authenticator.initiateLogin
#2674
Conversation
WalkthroughThis pull request updates the way login is initiated across multiple components. The changes remove the explicit Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as Component
participant A as AuthenticatorService
participant R as Router
U->>C: User clicks login button
C->>A: initiateLogin()
A->>R: Determine current URL or fallback origin
R-->>A: Provide redirection URL
A-->>C: Redirect user to login page
sequenceDiagram
participant U as User
participant C as Component
participant A as AuthenticatorService
participant R as Router
U->>C: User clicks login (with redirect intent)
C->>A: initiateLoginAndRedirectTo(redirectURL)
A->>R: Construct login URL using redirectURL
R-->>A: Confirm target redirection URL
A-->>C: Redirect user after successful login
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
Bundle ReportChanges will decrease total bundle size by 1.78kB (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-array-pushAssets Changed:
Files in
|
ryan-gang
force-pushed
the
authenticator-ref
branch
from
2f26de8 to
b422f11
Compare
ryan-gang
force-pushed
the
authenticator-ref
branch
from
b422f11 to
152230e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/services/authenticator.ts (1)
83-91: Consider adding JSDoc comments to document the expected format ofredirectPathOrUrl.The method handles three different types of paths (backend URLs, absolute paths, and other URLs). Adding JSDoc comments would help users understand the expected format and behavior.
+/** + * Initiates login with a custom redirect path or URL. + * @param redirectPathOrUrl - Can be one of: + * - A backend URL starting with the backend URL + * - An absolute path starting with '/' + * - Any other URL + */ initiateLoginAndRedirectTo(redirectPathOrUrl: string) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/components/affiliate-link-page/accept-referral-button.ts(1 hunks)app/components/concept-page/concept-completed-modal.ts(1 hunks)app/components/referral-link-page/accept-referral-container.ts(1 hunks)app/components/track-page/course-card/signup-to-preview-button.ts(1 hunks)app/components/track-page/start-track-button.ts(1 hunks)app/components/vote-page/course-extension-idea-card.ts(1 hunks)app/components/vote-page/course-idea-card.gts(1 hunks)app/controllers/pay.ts(1 hunks)app/routes/login.js(1 hunks)app/services/authenticator.ts(1 hunks)app/utils/base-route.ts(1 hunks)
🔇 Additional comments (11)
app/routes/login.js (1)
8-12: LGTM! The changes improve code clarity.The updated method name
initiateLoginAndRedirectTobetter describes its purpose when handling redirection, while maintaining the simplerinitiateLogincall for the default case.app/components/track-page/course-card/signup-to-preview-button.ts (1)
10-12: LGTM! The changes align with the PR objective.Removing the unnecessary
nullargument simplifies the code while maintaining the same functionality.app/components/track-page/start-track-button.ts (1)
30-32: LGTM! The changes align with the PR objective.Removing the unnecessary
nullargument simplifies the code while maintaining proper handling of anonymous users.app/components/concept-page/concept-completed-modal.ts (1)
36-36: LGTM! The changes improve code clarity.The updated method name
initiateLoginAndRedirectTobetter describes its purpose when handling redirection to the concept completion URL.app/components/vote-page/course-extension-idea-card.ts (1)
44-47: LGTM! Simplified login initiation.The removal of the null parameter aligns with the PR objective and maintains the same functionality.
app/utils/base-route.ts (1)
30-35: LGTM! Enhanced login flow with explicit redirection.The change improves clarity by explicitly handling redirection after login, both with and without route parameters. The FastBoot compatibility check ensures proper server-side rendering behavior.
app/controllers/pay.ts (1)
56-63: LGTM! Simplified login initiation in payment flow.The removal of the null parameter aligns with the PR objective while maintaining the same payment flow functionality.
app/components/affiliate-link-page/accept-referral-button.ts (1)
66-68: LGTM! Simplified login initiation in referral flow.The removal of the null parameter aligns with the PR objective while maintaining the same referral flow functionality.
app/components/referral-link-page/accept-referral-container.ts (1)
71-71: LGTM!The change aligns with the PR objective by removing the unnecessary
nullargument from theinitiateLogincall.app/services/authenticator.ts (1)
75-81: LGTM! The simplifiedinitiateLoginmethod.The method now handles the common case of redirecting to the current URL or origin, making the interface cleaner and more intuitive.
app/components/vote-page/course-idea-card.gts (1)
55-55: LGTM!The change aligns with the PR objective by removing the unnecessary
nullargument from theinitiateLogincall.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/services/authenticator.ts(1 hunks)
🔇 Additional comments (1)
app/services/authenticator.ts (1)
75-81: LGTM! Interface simplified as requested.The changes align with the previous review feedback by:
- Removing the optional parameter
- Simplifying the interface for callers
- Properly handling both URL scenarios
Extract login redirection logic into a separate method for better clarity and flexibility. The changes include: - Creating a new `initiateLoginAndRedirect` method - Handling different types of redirect paths and URLs - Simplifying the main `initiateLogin` method
Update multiple components to call initiateLogin without a null parameter, simplifying login initiation across the application
Update authenticator service and related components to use a more descriptive method name for login redirection. Changes include: - Renaming `initiateLoginAndRedirect` to `initiateLoginAndRedirectTo` - Updating method calls across multiple files to use the new method name - Maintaining existing login redirection functionality
Improve the `initiateLoginAndRedirectTo` method by: - Extracting the login base URL into a constant - Simplifying URL handling for different redirect scenarios - Ensuring consistent URL encoding and full redirect URL construction
Simplify the `initiateLoginAndRedirectTo` method by: - Removing conditional URL handling - Always constructing a full redirect URL - Ensuring consistent URL encoding for all redirect scenarios
ryan-gang
force-pushed
the
authenticator-ref
branch
from
97ed0bf to
cc9daa0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/services/authenticator.ts (1)
83-87: Consider handling additional URL cases.While the implementation correctly handles absolute URLs and paths starting with '/', it might not handle all relative URL cases correctly (e.g., 'path/to/resource' without leading slash).
Consider using URL parsing utilities to handle all URL cases:
initiateLoginAndRedirectTo(redirectPathOrUrl: string) { - const fullRedirectUrl = redirectPathOrUrl.startsWith('/') ? `${window.origin}${redirectPathOrUrl}` : redirectPathOrUrl; + const url = new URL(redirectPathOrUrl, window.origin); + const fullRedirectUrl = url.href; window.location.href = `${config.x.backendUrl}/login?next=` + encodeURIComponent(fullRedirectUrl); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/components/affiliate-link-page/accept-referral-button.ts(1 hunks)app/components/concept-page/concept-completed-modal.ts(1 hunks)app/components/referral-link-page/accept-referral-container.ts(1 hunks)app/components/track-page/course-card/signup-to-preview-button.ts(1 hunks)app/components/track-page/start-track-button.ts(1 hunks)app/components/vote-page/course-extension-idea-card.ts(1 hunks)app/components/vote-page/course-idea-card.gts(1 hunks)app/controllers/pay.ts(1 hunks)app/routes/login.js(1 hunks)app/services/authenticator.ts(1 hunks)app/utils/base-route.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- app/controllers/pay.ts
- app/components/vote-page/course-idea-card.gts
- app/routes/login.js
- app/components/affiliate-link-page/accept-referral-button.ts
- app/components/concept-page/concept-completed-modal.ts
- app/components/vote-page/course-extension-idea-card.ts
- app/components/track-page/course-card/signup-to-preview-button.ts
- app/utils/base-route.ts
- app/components/track-page/start-track-button.ts
- app/components/referral-link-page/accept-referral-container.ts
🔇 Additional comments (1)
app/services/authenticator.ts (1)
75-81: LGTM! Clean implementation of the simplified login interface.The removal of the
redirectPathparameter and the use of the current URL from the router provides a cleaner, more intuitive interface. The implementation correctly handles both cases where the current URL is available and when it's not.
Ensure proper URL encoding for the `next` parameter in login redirection test cases to prevent potential URL parsing issues
ryan-gang
changed the title
authenticator.initiateLogin
| window.location.href = `${config.x.backendUrl}/login?next=` + encodeURIComponent(`${window.origin}${redirectPath}`); | ||
| } else if (this.router.currentURL) { | ||
| initiateLogin() { | ||
| if (this.router.currentURL) { | ||
| window.location.href = `${config.x.backendUrl}/login?next=` + encodeURIComponent(`${window.origin}${this.router.currentURL}`); |
There was a problem hiding this comment.
| window.location.href = `${config.x.backendUrl}/login?next=` + encodeURIComponent(`${window.origin}${this.router.currentURL}`); | |
| this.initiateLoginAndRedirectTo(`${window.origin}${this.router.currentURL}`); |
| window.location.href = `${config.x.backendUrl}/login?next=` + encodeURIComponent(`${window.origin}${redirectPath}`); | ||
| } else if (this.router.currentURL) { | ||
| initiateLogin() { | ||
| if (this.router.currentURL) { | ||
| window.location.href = `${config.x.backendUrl}/login?next=` + encodeURIComponent(`${window.origin}${this.router.currentURL}`); | ||
| } else { | ||
| window.location.href = `${config.x.backendUrl}/login?next=` + encodeURIComponent(window.origin); |
There was a problem hiding this comment.
| window.location.href = `${config.x.backendUrl}/login?next=` + encodeURIComponent(window.origin); | |
| this.initiateLoginAndRedirectTo(window.origin); |
| window.location.href = `${config.x.backendUrl}/login?next=` + encodeURIComponent(`${window.origin}${this.router.currentURL}`); | ||
| } else { | ||
| window.location.href = `${config.x.backendUrl}/login?next=` + encodeURIComponent(window.origin); | ||
| } | ||
| } | ||
|
|
||
| initiateLoginAndRedirectTo(redirectPathOrUrl: string) { | ||
| const fullRedirectUrl = redirectPathOrUrl.startsWith('/') ? `${window.origin}${redirectPathOrUrl}` : redirectPathOrUrl; |
There was a problem hiding this comment.
| const fullRedirectUrl = redirectPathOrUrl.startsWith('/') ? `${window.origin}${redirectPathOrUrl}` : redirectPathOrUrl; | |
| const redirectUrl = redirectPathOrUrl.startsWith('/') ? `${window.origin}${redirectPathOrUrl}` : redirectPathOrUrl; |
("url" vs "path" is already clear, so just redirectUrl should do here? Another option is finalRedirectUrl to signify that there's an intermediary redirect followed by another. "Full" doesn't add much meaning)
Update multiple components to call initiateLogin without a null parameter, simplifying login initiation across the application
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit
New Features
Refactor
These updates result in a smoother and more cohesive login process on several user-facing pages, improving overall navigation and reducing friction during authentication.