fix: Fix issues with sign in page redirection and video view #3404
Conversation
WalkthroughIn a stylish authentication update, the pull request introduces enhancements to the sign-in page and verify page. The sign-in page's video element gets inline playback and autoplay capabilities, while the verify page integrates a new Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
🪧 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/auth/verify/verify.page.spec.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-app
- GitHub Check: build (12.x)
🔇 Additional comments (1)
src/app/auth/verify/verify.page.spec.ts (1)
14-14: Mind-blowing setup of UserEventService, partner! 🌟The way you've imported and configured UserEventService in the test module is simply superb! Clean integration with proper spy object setup. Just like how I handle my stunts - perfect preparation!
Also applies to: 23-23, 30-30, 60-63, 73-73
| const logoutSpy = spyOn(component, 'logout'); | ||
| component.handleError(error); | ||
| expect(router.navigate).toHaveBeenCalledOnceWith([ | ||
| '/', | ||
| 'auth', | ||
| 'pending_verification', | ||
| { orgId: 'orNVthTo2Zyo' }, | ||
| ]); | ||
| expect(logoutSpy).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
💡 Codebase verification
Mind it! Your error handling needs more power, machan!
The verification reveals three different paths in the error handler, but the test only covers one! Here's what we're missing, style-ah style-ah:
- Status 422: Should navigate to 'auth/disabled'
- Status 440: Should navigate to 'auth/pending_verification' with correct params
- Other status codes: Currently tested, correctly calls logout()
Add test cases for all scenarios to make your error handling as powerful as a Rajinikanth punch! 🌟
🔗 Analysis chain
Style-ah style-ah! But let's verify the error scenarios, machan!
The error handling test now redirects to logout instead of a specific route. But we should verify all possible error scenarios to ensure robust error handling.
Add test cases for other HTTP status codes that might occur during verification:
it('should handle network errors gracefully', () => {
const error = { status: 0 }; // Network error
const logoutSpy = spyOn(component, 'logout');
component.handleError(error);
expect(logoutSpy).toHaveBeenCalledTimes(1);
});🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all error handling scenarios in the codebase
rg -B 2 -A 2 "handleError.*404" --type ts
Length of output: 43
Script:
#!/bin/bash
# Search for handleError implementation
ast-grep --pattern 'handleError($$$) { $$$ }'
# Search for error handling in verify page
rg -B 2 -A 4 "handleError" src/app/auth/verify/verify.page.ts
# Search for HTTP error imports and usage
rg -B 2 -A 2 "HttpErrorResponse" src/app/auth/verify/
Length of output: 599
Script:
#!/bin/bash
# Get the complete handleError method implementation with more context
rg -B 2 -A 10 "handleError.*void" src/app/auth/verify/verify.page.ts
# Also check for logout method usage
rg -B 2 -A 2 "logout\(" src/app/auth/verify/verify.page.ts
Length of output: 780
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/auth/sign-in/sign-in.page.html(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
| playsinline | ||
| autoplay |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! The video attributes are spot on, but let's make it more accessible, shall we?
The playsinline and autoplay attributes are perfect additions for iOS devices, making the video play smoothly like my action sequences! 🎬
But wait! Let me tell you something important. For better accessibility, like how I make my movies enjoyable for everyone:
<video
class="sign-in__video-container__video"
width="100%"
height="auto"
onloadedmetadata="this.muted = true"
oncanplay="this.play()"
poster="../../../assets/images/video-default-background.png"
loop
playsinline
autoplay
+ aria-label="Fyle mobile app introduction"
+ preload="metadata"
>These changes will:
- Add screen reader support with
aria-label - Optimize loading with
preload="metadata"
📝 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.
| playsinline | |
| autoplay | |
| <video | |
| class="sign-in__video-container__video" | |
| width="100%" | |
| height="auto" | |
| onloadedmetadata="this.muted = true" | |
| oncanplay="this.play()" | |
| poster="../../../assets/images/video-default-background.png" | |
| loop | |
| playsinline | |
| autoplay | |
| aria-label="Fyle mobile app introduction" | |
| preload="metadata" | |
| > |
| > | ||
| <source src="/assets/videos/mobile-onboarding.mp4" type="video/mp4" /> | ||
| </video> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style ah? Let's make this video work everywhere like my movies!
For maximum compatibility across browsers, like how my movies reach every corner of the world:
<video
...
>
<source src="/assets/videos/mobile-onboarding.mp4" type="video/mp4" />
+ <source src="/assets/videos/mobile-onboarding.webm" type="video/webm" />
+ <!-- Fallback content for browsers that don't support video -->
+ <p>Your browser doesn't support HTML video. Here's a <a href="/assets/videos/mobile-onboarding.mp4">link to the video</a> instead.</p>
</video>Adding WebM format and fallback content ensures our video plays everywhere, just like how I never disappoint my fans! 🌟
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
src/app/auth/sign-in/sign-in.page.ts (3)
Line range hint
164-189: Style-ah! Let's make the error handling more robust, like my punch dialogues!The error handling is good, but we can make it SUPER by:
- Adding network error handling for offline scenarios
- Tracking error events for analytics
- Adding retry mechanism for temporary failures
Here's how to make it more powerful:
async handleError(error: HttpErrorResponse): Promise<void> { let header = 'Incorrect email or password'; + // Track error events + this.trackingService.eventTrack('Sign In Error', { + type: error?.status, + message: error?.message + }); + if (!navigator.onLine) { + header = 'No internet connection'; + } else if (error?.status === 400) { this.router.navigate(['/', 'auth', 'pending_verification', { email: this.fg.controls.email.value as string }]); return; } else if (error?.status === 422) {
Line range hint
191-229: Magizhchi! The Google sign-in needs more power!The Google sign-in flow is like a complex fight sequence - it needs proper error handling and timeout management!
Let's make it better:
googleSignIn(): void { this.googleSignInLoading = true; const markOptions: PerformanceMarkOptions = { detail: 'Google Login', }; performance.mark('login start time', markOptions); from(this.googleAuthService.login()) .pipe( + timeout(30000), // Add timeout switchMap((googleAuthResponse: { accessToken: string }) => { if (googleAuthResponse.accessToken) { return of(googleAuthResponse); } else { return throwError(noop); } }), map((googleAuthResponse) => { - from(this.loaderService.showLoader('Signing you in...', 10000)); + from(this.loaderService.showLoader('Signing you in...')); return googleAuthResponse; }), // ... rest of the chain ) .subscribe({ next: () => { this.fg.reset(); this.router.navigate(['/', 'auth', 'switch_org', { choose: true }]); }, + error: (err) => { + this.handleError(err); + } }); }
Line range hint
1-329: Listen up! Here's some style tips to make this code SUPER!The authentication flow is like a well-choreographed fight sequence, but we can make it even better!
Consider these architectural improvements:
Create an AuthenticationOrchestrator service to:
- Manage authentication state
- Handle different auth providers
- Provide unified error handling
Use TypeScript strict mode for better type safety
Add retry strategies for network operations
Consider implementing a proper state machine for managing authentication flow
Remember: "When something ends, something new begins!" Let's make these improvements in future PRs!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/auth/sign-in/sign-in.page.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-app
- GitHub Check: build (12.x)
🔇 Additional comments (1)
src/app/auth/sign-in/sign-in.page.ts (1)
288-288: Mind it! The back button priority change looks powerful, but let's verify its impact!Changing the back button priority to ABSOLUTE is like my signature move - it takes control! But with great power comes great responsibility. Let's make sure this doesn't affect other pages' back button behavior.
Run this script to check for potential conflicts:
✅ Verification successful
Mind it! The ABSOLUTE priority is perfectly styled, like a boss!
Your back button priority change is following the same style as app-version page, maintaining the swag of our auth flow! When you're dealing with authentication, you need to be in total control, just like how I control the screen! 🕶️
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other ABSOLUTE priority usages that might conflict rg "BackButtonActionPriority.ABSOLUTE" --type ts # Look for other back button handlers that might be affected ast-grep --pattern 'registerBackButtonAction($_,$_)'Length of output: 824
src/app/auth/sign-in/sign-in.page.ts
Outdated
| this.currentStep = SignInPageState.SELECT_SIGN_IN_METHOD; | ||
| } | ||
| const priority = BackButtonActionPriority.MEDIUM; | ||
| const priority = BackButtonActionPriority.ABSOLUTE; | ||
| this.hardwareBackButtonAction = this.platformHandlerService.registerBackButtonAction(priority, this.goBack); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! The state management needs more style!
The state transitions are working, but we can make them SUPER by adding validation and persistence!
Consider creating a dedicated state manager:
interface SignInState {
currentStep: SignInPageState;
email?: string;
isLoading: boolean;
}
class SignInStateManager {
private state: SignInState;
canTransition(from: SignInPageState, to: SignInPageState): boolean {
// Add validation logic
return true;
}
transition(to: SignInPageState): void {
if (this.canTransition(this.state.currentStep, to)) {
this.state.currentStep = to;
}
}
}Also applies to: 293-295
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/app/fyle/dashboard/tasks/tasks.component.ts (1)
Line range hint
1-1006: Listen up! Let's talk about error handling, the Rajinikanth way!I notice that error handling across async operations is not consistent, like a movie without a proper climax! We should handle errors uniformly across all async operations to give our users a better experience.
Here's what I suggest, in true superstar style:
- Create a centralized error handling service
- Implement consistent error recovery strategies
- Show user-friendly error messages
- Log errors for debugging
Would you like me to demonstrate this pattern with a code example?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/fyle/dashboard/tasks/tasks.component.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
|
|
||
| this.doRefresh(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! The change looks good, but let's make it even better!
The addition of this.doRefresh() after card enrollment is a stylish move, partner! It ensures the tasks list stays fresh like my morning coffee. But let me tell you something - we can make it even more powerful!
Consider adding error handling to make it super-robust:
await cardAddedModal.onDidDismiss();
- this.doRefresh();
+ this.doRefresh().catch((error) => {
+ console.error('Failed to refresh tasks after card enrollment:', error);
+ // Show user-friendly error message if needed
+ });Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/auth/sign-in/sign-in.page.spec.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: build (12.x)
| @@ -229,7 +229,7 @@ | |||
| component.ionViewWillEnter(); | |||
| expect(component.hardwareBackButtonAction).toEqual(mockSubscription); | |||
| expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledOnceWith( | |||
| BackButtonActionPriority.MEDIUM, | |||
| BackButtonActionPriority.ABSOLUTE, | |||
There was a problem hiding this comment.
💡 Codebase verification
Mind it! The test file is not in sync with the actual implementation, partner!
The sign-in page uses MEDIUM priority in the actual implementation, but the test file is trying to be the superstar with ABSOLUTE priority. Let's keep it real and match the test with the implementation!
src/app/auth/sign-in/sign-in.page.ts:291: UsesBackButtonActionPriority.MEDIUMsrc/app/auth/sign-in/sign-in.page.spec.ts:232: Incorrectly usesBackButtonActionPriority.ABSOLUTE
🔗 Analysis chain
Mind it! The back button priority change needs verification.
The change from MEDIUM to ABSOLUTE priority could affect other components' back button handling. This might be too powerful, like my punch!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using BackButtonActionPriority
ast-grep --pattern 'BackButtonActionPriority.$_'
Length of output: 2761
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/auth/sign-in/sign-in.page.spec.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (1)
src/app/auth/sign-in/sign-in.page.spec.ts (1)
Line range hint
234-263: Thalaiva says test cases need more power!The test cases for goBack() are missing edge cases. What happens when currentStep is undefined or null? Don't underestimate the importance of defensive programming, like how I never underestimate my opponents!
Add these test cases:
describe('goBack(): ', () => { + it('should handle undefined currentStep gracefully', () => { + spyOn(component, 'changeState'); + component.currentStep = SignInPageState.ENTER_PASSWORD; + component.goBack(undefined); + expect(backButtonService.showAppCloseAlert).toHaveBeenCalledTimes(1); + }); + + it('should handle null currentStep gracefully', () => { + spyOn(component, 'changeState'); + component.currentStep = SignInPageState.ENTER_PASSWORD; + component.goBack(null); + expect(backButtonService.showAppCloseAlert).toHaveBeenCalledTimes(1); + }); // existing test cases... });
| }); | ||
|
|
||
| describe('goBack(): ', () => { | ||
| it('should navigate to ENTER_EMAIL when current step is ENTER_PASSWORD', () => { | ||
| spyOn(component, 'changeState'); | ||
|
|
||
| component.currentStep = SignInPageState.ENTER_PASSWORD; | ||
| component.goBack(); | ||
| component.goBack(component.currentStep); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style, partner! The test readability needs a boost!
The test cases would be clearer if we explicitly show what currentStep is being passed. Let's make it more readable!
- component.goBack(component.currentStep);
+ const currentStep = SignInPageState.ENTER_PASSWORD;
+ component.goBack(currentStep);Apply similar changes to other test cases for consistency.
Also applies to: 249-249, 259-259
| BackButtonActionPriority.MEDIUM, | ||
| component.goBack | ||
| ); | ||
| expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! The test expectation needs more power, partner!
The test only verifies the call count but not the actual parameters passed to registerBackButtonAction. Let's make it more robust!
- expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledTimes(1);
+ expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledOnceWith(
+ jasmine.any(Function),
+ BackButtonActionPriority.MEDIUM
+ );📝 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.
| expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledTimes(1); | |
| expect(platformHandlerService.registerBackButtonAction).toHaveBeenCalledOnceWith( | |
| jasmine.any(Function), | |
| BackButtonActionPriority.MEDIUM | |
| ); |
|
Fixes in this PR:
ClickUp: https://app.clickup.com/t/86cxk0v82
Thread references:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
goBackmethod for improved navigation handling on the sign-in page.