Re-login modal #717
Re-login modal #717
Conversation
- Add QuickRegister component with improved modal management - Integrate quick login/register with app store state - Implement custom event handling for login/registration flow - Update OAuth callback to support quick login in popup windows - Refactor authentication-related components to use global events
- Create new useAuth composable to centralize login, registration, and social login logic - Simplify authentication methods in LoginForm and RegisterForm - Add event-based login/registration flow with quick login support - Remove redundant API calls and consolidate authentication processes - Improve error handling and analytics tracking for authentication events
WalkthroughThis update refactors several authentication and registration components to streamline the login flow. The changes include adding global event handling via a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant QR as QuickRegister Component
participant LF as LoginForm/RegisterForm
participant GP as Guest Page
participant PS as AppStore (Pinia)
U->>QR: Initiate login action
QR->>QR: Execute handleAfterLogin()
QR->>window: Emit "after-quick-login" event
window->>GP: Trigger afterLogin() via event listener
GP->>PS: (Optional) Update central modal state
LF->>PS: Manage lifecycle listeners (mounted/unmounted)
sequenceDiagram
participant O as OAuth Provider
participant CB as Callback Page
participant AU as useAuth Composable
participant U as User
O->>CB: Redirect with OAuth code & UTM data
CB->>AU: Call handleSocialCallback(provider, code, UTM)
AU-->>CB: Return authentication result
alt New User
CB->>U: Redirect to form creation page, display success alert
else Existing User
CB->>window: Dispatch event to opener window
CB->>window: Close callback window
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (21)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
client/composables/useAuth.js (1)
31-38: Consider more robust error handling for analytics tracking.The try-catch block for GTM tracking is good, but the error is only logged to the console. Consider adding more comprehensive error handling or fallback strategies for analytics failures.
try { useGtm().trackEvent({ event: eventName, source }) } catch (error) { console.error(error) + // Consider adding a fallback analytics method or retry logic }client/pages/oauth/callback.vue (1)
53-59: Add fallback for window closing behavior.The code currently attempts to close the window unconditionally after dispatching the event. This may not work in all browsers if the window wasn't opened via JavaScript. Consider adding a fallback.
if (!isNewUser) { // Handle existing user login if (window.opener) { window.opener.document.dispatchEvent(new CustomEvent('quick-login-complete')) + window.close() + } else { + // Fallback if window.opener is not available + router.push({ name: "home" }) } - window.close() }client/pages/forms/create/guest.vue (1)
96-104: Consider removing the setTimeout for form saving.Using setTimeout for operations after login could be fragile. Consider using a more robust approach like watching for state changes or implementing a callback pattern.
const afterLogin = () => { isGuest.value = false fetchAllWorkspaces() - setTimeout(() => { + // Wait for workspace loading to complete before saving + watch(() => workspacesStore.loading, (loading) => { + if (!loading && editor.value) { + editor.value.saveFormCreate() + } + }, { immediate: true }) - if (editor) { - editor.value.saveFormCreate() - } - }, 500) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
client/app.vue(2 hunks)client/components/pages/auth/components/LoginForm.vue(1 hunks)client/components/pages/auth/components/QuickRegister.vue(3 hunks)client/components/pages/auth/components/RegisterForm.vue(3 hunks)client/composables/useAuth.js(1 hunks)client/composables/useOpnApi.js(1 hunks)client/pages/forms/create/guest.vue(2 hunks)client/pages/oauth/callback.vue(1 hunks)client/stores/app.js(1 hunks)client/stores/oauth_providers.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (22)
client/stores/app.js (1)
12-13: Good addition of modal state properties.Adding
quickLoginModalandquickRegisterModalstate properties to the Pinia store centralizes the modal visibility management, which aligns with the PR objective of streamlining the login flow.client/stores/oauth_providers.js (1)
79-79: Improved user experience by opening OAuth in a new tab.Changing from
window.location.hreftowindow.open(data.url, '_blank')keeps users on the current page while authentication happens in a new tab, preventing disruption to the user's current context.client/composables/useOpnApi.js (1)
66-66: Enhanced UX by showing login modal instead of redirecting.Replacing the redirect to login page with a modal dialog (
useAppStore().quickLoginModal = true) when encountering a 401 error creates a smoother user experience, allowing users to re-authenticate without losing their current context.client/app.vue (3)
53-53: Good implementation of the QuickRegister component.Adding the
<QuickRegister>component with the@after-loginevent handler creates a global registration mechanism that's accessible throughout the application.
108-112: Well-implemented global event propagation.The
handleAfterLoginmethod effectively creates a global event system for login events, allowing any component to listen for successful authentication without tight coupling.
117-117: Properly exposed the handler in the template.Exposing the
handleAfterLoginmethod in the return object ensures it's accessible from the template.client/composables/useAuth.js (2)
1-108: Well-structured authentication composable with comprehensive functionality.This new
useAuthcomposable effectively centralizes authentication logic into a reusable module with clear separation of concerns. The code is well-structured with async/await patterns and proper error handling for the authentication flow.A few observations:
- The
authenticateUserfunction handles both the authentication process and analytics tracking- Social login callbacks are properly managed with the isNewUser flag
- AppSumo license validation is handled within the registration flow
74-101:❓ Verification inconclusive
Verify login credential persistence after registration.
The registration flow submits the form twice - once for registration and once for login. This could potentially lead to issues if the server has validation rules that differ between the two endpoints.
🏁 Script executed:
#!/bin/bash # Find all places where the login and registration endpoints are used echo "Checking login and registration API usage:" rg -A 3 "post.*'/login'" --glob "*.{js,vue}" rg -A 3 "post.*'/register'" --glob "*.{js,vue}" # Check for different validation rules between login and register echo "Checking for validation differences:" rg -A 5 "validate|validation" --glob "app/Http/Requests/*Request.php"Length of output: 1241
Attention: Manual Verification of Endpoint Validation Required
The current authentication flow uses two form submissions—one for registration (
/register) and one for login (/login). While our search confirmed the expected API calls inclient/composables/useAuth.js, the automated query for validation logic differences (using the glob pattern forapp/Http/Requests/*Request.php) did not return any files. This lack of output may be due to filtering issues rather than a definitive absence of discrepancies.Action Points:
- Verify Login Credential Persistence: Ensure that the token returned from the
/logincall is correctly persisted and that the user session remains authenticated.- Manual Validation Check: Manually review the server-side validation rules for both
/registerand/loginendpoints to confirm they maintain consistent expectations regarding login credentials and handle validation similarly.Please confirm these aspects so that we can be confident that the dual submission process does not introduce any edge-case issues.
client/pages/oauth/callback.vue (1)
42-68: Improved OAuth callback handling with the new auth composable.The refactored callback handling is more concise and maintainable. Using the auth composable centralizes the authentication logic while properly handling different user scenarios.
client/pages/forms/create/guest.vue (2)
14-14: Improved state management with centralized modal control.Using the app store for modal state management is a good improvement over local component state, providing better consistency across the application.
87-94: 🛠️ Refactor suggestionFix event listener reference for proper cleanup.
The event listener setup and cleanup should use the same function reference to ensure proper removal.
onMounted(() => { // Add event listener for after quick login - window.addEventListener('after-quick-login', afterLogin) + window.addEventListener('after-quick-login', afterLogin) }) onUnmounted(() => { // Clean up the event listener - window.removeEventListener('after-quick-login', afterLogin) + window.removeEventListener('after-quick-login', afterLogin) })The current implementation looks correct, but I wanted to emphasize the importance of using the same function reference when adding and removing event listeners.
Likely an incorrect or invalid review comment.
client/components/pages/auth/components/LoginForm.vue (2)
149-163: Cleaner login implementation with async/await.The refactored login method using async/await and the auth composable is more readable and maintainable.
157-159:❓ Verification inconclusive
Verify self-host mode error handling.
The specific error message check for self-host mode seems unusual. Verify that this is the intended behavior and consider adding a comment explaining why this error is handled differently.
🏁 Script executed:
#!/bin/bash # Look for other instances of this error message and how it's handled echo "Checking for self-host mode error handling:" rg "You must change your credentials when in self host mode" --glob "*.{js,vue,php}" # Check related authentication logic for self-host mode echo "Checking for self-host related code:" rg "self.?host" --glob "app/Http/Controllers/Auth*.php"Length of output: 634
Attention: Clarify Self-Host Error Handling
- In
client/components/pages/auth/components/LoginForm.vue(lines 157–159), the error message"You must change your credentials when in self host mode"is uniquely checked in this component.- Our search confirmed that this message only appears here, and no additional self-host handling logic was located in the expected backend files.
- Please verify that using this specific error message to trigger a redirect is the intended behavior for self-host mode.
- Consider adding a comment in the code to explain why errors in self-host mode are handled differently, especially since there’s no similar logic elsewhere.
client/components/pages/auth/components/QuickRegister.vue (5)
5-7: Good refactoring to centralized state management!Using the appStore to manage the login modal state improves maintainability by centralizing state management rather than using local component state.
39-41: Consistent use of centralized state managementThe register modal now follows the same pattern as the login modal, using appStore for state management.
73-78: Well-implemented Composition API conversionGood transition from Options API to Composition API using
<script setup>. The proper defineEmits usage ensures type safety for event handling.
80-90: Proper event listener cleanupExcellent implementation of lifecycle hooks to add and remove event listeners. This prevents memory leaks by ensuring event listeners are properly cleaned up when the component is unmounted.
92-109: Clean method implementationsThe refactored methods are well-implemented with clear responsibilities:
openLoginandopenRegistertoggle the appropriate modalsafterQuickLoginhandles post-login behavior with proper timingThe use of emits for communication with parent components follows Vue best practices.
client/components/pages/auth/components/RegisterForm.vue (4)
77-94: Improved terms and conditions formattingThe terms and conditions section has been nicely reformatted for better readability and structure, while maintaining the same content.
209-211: Event listener for quick login addedGood addition of the event listener for the 'quick-login-complete' event, ensuring this component reacts appropriately when a quick login occurs elsewhere in the application.
237-253: Simplified registration logicThe register method has been nicely refactored to use the useAuth composable, which encapsulates authentication logic. This improves code maintainability and follows the principle of separation of concerns.
The error handling is now more streamlined with useAlert().
254-266: Good extraction of redirect logicExtracting the navigation logic into a separate method improves code organization and reusability, especially since it's now used in both the register method and the event listener.
| mounted() { | ||
| document.addEventListener('quick-login-complete', () => { | ||
| this.redirect() | ||
| }) | ||
| }, | ||
| unmounted() { | ||
| document.removeEventListener('quick-login-complete', () => { | ||
| this.redirect() | ||
| }) | ||
| }, |
There was a problem hiding this comment.
Fix event listener removal implementation.
The current event listener removal doesn't work properly because it uses an inline function that won't match the original listener. Use a named function or store the handler in a variable.
+ data: () => ({
+ form: useForm({
+ email: "",
+ password: "",
+ }),
+ loading: false,
+ remember: false,
+ showForgotModal: false,
+ handleQuickLoginComplete: null,
+ }),
mounted() {
+ this.handleQuickLoginComplete = () => {
+ this.redirect()
+ }
- document.addEventListener('quick-login-complete', () => {
- this.redirect()
- })
+ document.addEventListener('quick-login-complete', this.handleQuickLoginComplete)
},
unmounted() {
- document.removeEventListener('quick-login-complete', () => {
- this.redirect()
- })
+ document.removeEventListener('quick-login-complete', this.handleQuickLoginComplete)
},📝 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.
| mounted() { | |
| document.addEventListener('quick-login-complete', () => { | |
| this.redirect() | |
| }) | |
| }, | |
| unmounted() { | |
| document.removeEventListener('quick-login-complete', () => { | |
| this.redirect() | |
| }) | |
| }, | |
| export default { | |
| // ... other component options | |
| + data: () => ({ | |
| + form: useForm({ | |
| + email: "", | |
| + password: "", | |
| + }), | |
| + loading: false, | |
| + remember: false, | |
| + showForgotModal: false, | |
| + handleQuickLoginComplete: null, | |
| + }), | |
| mounted() { | |
| + this.handleQuickLoginComplete = () => { | |
| + this.redirect() | |
| + } | |
| - document.addEventListener('quick-login-complete', () => { | |
| - this.redirect() | |
| - }) | |
| + document.addEventListener('quick-login-complete', this.handleQuickLoginComplete) | |
| }, | |
| unmounted() { | |
| - document.removeEventListener('quick-login-complete', () => { | |
| - this.redirect() | |
| - }) | |
| + document.removeEventListener('quick-login-complete', this.handleQuickLoginComplete) | |
| }, | |
| // ... other component options | |
| } |
| onUnmounted(() => { | ||
| document.removeEventListener('quick-login-complete', () => { | ||
| afterQuickLogin() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Event listener callback mismatch
The callback function for removeEventListener should reference the same function instance as addEventListener, but you're creating a new anonymous function each time.
-onUnmounted(() => {
- document.removeEventListener('quick-login-complete', () => {
- afterQuickLogin()
- })
-})
+onUnmounted(() => {
+ document.removeEventListener('quick-login-complete', afterQuickLogin)
+})Similarly, update the addEventListener call:
-onMounted(() => {
- document.addEventListener('quick-login-complete', () => {
- afterQuickLogin()
- })
-})
+onMounted(() => {
+ document.addEventListener('quick-login-complete', afterQuickLogin)
+})📝 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.
| onUnmounted(() => { | |
| document.removeEventListener('quick-login-complete', () => { | |
| afterQuickLogin() | |
| }) | |
| }) | |
| onMounted(() => { | |
| document.addEventListener('quick-login-complete', afterQuickLogin) | |
| }) | |
| onUnmounted(() => { | |
| document.removeEventListener('quick-login-complete', afterQuickLogin) | |
| }) |
| unmounted() { | ||
| document.removeEventListener('quick-login-complete', () => { | ||
| this.redirect() | ||
| }) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Event listener cleanup issue
The event listener removal has the same issue as in QuickRegister.vue. You're creating a new anonymous function instead of referencing the same function that was added.
-unmounted() {
- document.removeEventListener('quick-login-complete', () => {
- this.redirect()
- })
-},
+unmounted() {
+ document.removeEventListener('quick-login-complete', this.redirect)
+},Similarly, update the addEventListener call:
-mounted() {
- document.addEventListener('quick-login-complete', () => {
- this.redirect()
- })
+mounted() {
+ document.addEventListener('quick-login-complete', this.redirect)📝 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.
| unmounted() { | |
| document.removeEventListener('quick-login-complete', () => { | |
| this.redirect() | |
| }) | |
| }, | |
| mounted() { | |
| document.addEventListener('quick-login-complete', this.redirect) | |
| }, | |
| unmounted() { | |
| document.removeEventListener('quick-login-complete', this.redirect) | |
| }, |
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
♻️ Duplicate comments (1)
client/components/pages/auth/components/RegisterForm.vue (1)
230-234:⚠️ Potential issueFunction reference inconsistency in event listener cleanup
The event listener removal won't work because you're using a new anonymous function rather than referencing the same function that was added.
- unmounted() { - document.removeEventListener('quick-login-complete', () => { - this.redirect() - }) - }, + unmounted() { + document.removeEventListener('quick-login-complete', this.handleQuickLogin) + },And update the addEventListener call:
- mounted() { - document.addEventListener('quick-login-complete', () => { - this.redirect() - }) + mounted() { + this.handleQuickLogin = () => this.redirect() + document.addEventListener('quick-login-complete', this.handleQuickLogin)
🧹 Nitpick comments (9)
client/composables/useAuth.js (4)
1-6: Consider adding error handling and default initial states for stores.Although the store references appear valid, if
useAuthStore,useWorkspacesStore, oruseFormsStorethrow an error or are uninitialized, the composable might fail silently. Adding a try/catch block or default object handling would help ensure robustness.
46-53: Allow passing a “remember me” or expiration parameter explicitly.
loginWithCredentialscurrently assumes a simple login scenario without specifying cookie expiration or long-lived tokens. If you intend to support “Remember Me” functionality, consider adding a parameter to this function, passing it into the call toauthenticateUserorauthStore.setToken.const loginWithCredentials = async (form, remember = false) => { const { token } = await form.submit('post', '/login') return authenticateUser({ token, source: 'credentials', + expiresIn: remember ? 60 * 60 * 24 * 7 : undefined // example 7 days }) }
58-69: Handle potential callback errors more gracefully.When calling
/oauth/{provider}/callback, if the server returns an error (e.g. invalid code), the current code may unhandledly throw. Consider catching these errors to provide a better user experience or fallback.
103-108: Ensure usage clarifies composable responsibilities.Exposing
loginWithCredentials,handleSocialCallback, andregisterUserfromuseAuthis helpful. For maintainability, consider documenting these signature expectations (e.g., required form structure, error handling approach) so that other components know precisely how to consume them.client/stores/auth.js (1)
62-63: Confirm logout logic for no-user scenario.When
setUseris called withnull, the code forcibly sets the token tonullwithmaxAge = 0. Ensure the rest of the code (like analytics or reliant services) gracefully handles a user no longer being present.api/app/Console/Commands/Tax/GenerateTaxExport.php (3)
67-69: Offer concurrency or scheduling advice.Although starting a timer is helpful, consider scheduling or chunked parallel execution if invoice volumes grow large. This could reduce runtime and memory usage.
192-207: Validate potential rate-limit exposures.Though Stripe’s API can handle the request rate, consider a retry logic pattern for improved resiliency if Stripe temporarily throttles or encounters network hiccups. Relying on a single catch might omit certain transient errors.
220-246: Improve final output for clarity.The displayed counters are useful. Consider adding:
- A ratio of processed vs. total.
- A separate line item for partial refunds (if tracked).
- Potential generation of a summary CSV or JSON.
client/pages/oauth/callback.vue (1)
64-66: Consider adding fallback error handlingThe current error handling assumes
error.response._data.messagewill always be available. If the error structure is different (network error, unexpected response format), this might throw another exception.- useAlert().error(error.response._data.message) + const errorMessage = error.response?._data?.message || 'An error occurred during authentication' + useAlert().error(errorMessage)
🛑 Comments failed to post (4)
client/composables/useAuth.js (2)
7-41: 🛠️ Refactor suggestion
Wrap parallel data fetching with error handling.
In
authenticateUser, the code simultaneously fetches user data and workspaces withPromise.all. If either request fails, the function may throw unhandled errors. Consider:
- Wrapping the concurrent calls in a
try/catchto manage partial failures or offline scenarios.- Providing user feedback or fallback states (e.g., redirect to login if fetching user data fails).
const authenticateUser = async ({ token, source, isNewUser = false }) => { + try { authStore.setToken(token) const [userData, workspaces] = await Promise.all([ opnFetch("user"), fetchAllWorkspaces() ]) ... + } catch (error) { + console.error("Failed to authenticate user:", error) + // Possibly redirect or reset the state + throw error + } }Committable suggestion skipped: line range outside the PR's diff.
74-101: 🛠️ Refactor suggestion
Validate registration response and potential partial failures.
After registration and login, the code assumes both requests succeed. Network or server errors in either step may leave the user in an inconsistent state. It might be beneficial to short-circuit if the initial
/registercall fails.const registerUser = async (form) => { try { const data = await form.submit('post', '/register') const { token } = await form.submit('post', '/login') ... } catch (error) { useAlert().error("Failed to complete registration or login. " + error.message) throw error } ... }Committable suggestion skipped: line range outside the PR's diff.
client/stores/auth.js (1)
30-41: 🛠️ Refactor suggestion
Review cookie security settings.
setToken(token, expiresIn)sets no additional cookie flags likesecureorsameSite. This can be a security risk if the site is accessed over HTTP or from multiple subdomains. Consider setting secure cookie options if your environment strictly uses HTTPS.setToken(token, expiresIn) { const cookieOptions = {} if (expiresIn) { cookieOptions.maxAge = expiresIn } + cookieOptions.secure = true + cookieOptions.sameSite = 'strict' this.setCookie("token", token, cookieOptions) this.token = token }📝 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.setToken(token, expiresIn) { // Set cookie with expiration if provided const cookieOptions = {} if (expiresIn) { // expiresIn is in seconds, maxAge also needs to be in seconds cookieOptions.maxAge = expiresIn } cookieOptions.secure = true cookieOptions.sameSite = 'strict' this.setCookie("token", token, cookieOptions) this.token = token },api/app/Console/Commands/Tax/GenerateTaxExport.php (1)
125-146: 🛠️ Refactor suggestion
Consider memory usage when accumulating invoices.
Storing all invoices in
$processedInvoicescan be memory-intensive if there are thousands of invoices. Consider streaming the results to a file or database incrementally instead of storing them all in an array.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Refactor