fix: Fyle fix issues around tasks and qa changes #3394
Conversation
WalkthroughIn a grand spectacle of code evolution, this pull request orchestrates a transformation across various components, enhancing task management, sign-in processes, and corporate card integration. Changes span Android configuration files to TypeScript services, introducing new methods for task handling, back button navigation, and corporate card enrollment. The application now stands taller, ready to take on the world with a flair that only a superstar can deliver! Changes
Poem
Suggested labels
Suggested reviewers
Possibly related PRs
With each line of code, we rise! The transformation is complete, and the stage is set for a blockbuster performance! 🌟🔥 Thalaiva approves! 😎 🪧 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: 15
🔭 Outside diff range comments (1)
src/app/app.component.ts (1)
Line range hint
108-137: Style ah style! Watch out for potential race conditions.adjusts sunglasses
The splash screen hide operation runs independently of the version check and device info operations that follow. If these operations fail, the user might see an error screen without proper transition.
Here's a punch dialogue... I mean, a better approach:
- setTimeout(async () => await SplashScreen.hide(), 200); + const initTasks = async () => { + try { + await TextZoom.set({ value: 1 }); + const isLoggedIn = await this.routerAuthService.isLoggedIn(); + if (!isLoggedIn) { + await SplashScreen.hide(); + return; + } + + const deviceInfo = await this.deviceService.getDeviceInfo(); + if (!['android', 'ios'].includes(deviceInfo.platform.toLowerCase())) { + await SplashScreen.hide(); + return; + } + + this.appVersionService.load(deviceInfo); + const userAppVersionDetails = await this.appVersionService.getUserAppVersionDetails(deviceInfo); + + await SplashScreen.hide(); + + if (userAppVersionDetails) { + const { appSupportDetails, lastLoggedInVersion, eou, deviceInfo } = userAppVersionDetails; + this.trackingService.eventTrack('Auto Logged out', { + lastLoggedInVersion, + user_email: eou?.us?.email, + appVersion: deviceInfo.appVersion, + }); + this.router.navigate(['/', 'auth', 'app_version', { message: appSupportDetails.message }]); + } + } catch (error) { + await SplashScreen.hide(); + console.error('Initialization failed:', error); + } + }; + + initTasks();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
android/app/src/main/ic_launcher_new_splash-playstore.pngis excluded by!**/*.pngsrc/assets/images/video-default-background.pngis excluded by!**/*.png
📒 Files selected for processing (20)
android/.idea/runConfigurations.xml(1 hunks)android/app/src/main/res/drawable/ic_launcher_new_splash_background.xml(1 hunks)android/app/src/main/res/drawable/ic_launcher_new_splash_foreground.xml(1 hunks)android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_new_splash.xml(1 hunks)android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_new_splash_round.xml(1 hunks)src/app/app.component.ts(1 hunks)src/app/auth/sign-in/sign-in-page-state.enum.ts(1 hunks)src/app/auth/sign-in/sign-in.page.html(2 hunks)src/app/auth/sign-in/sign-in.page.scss(4 hunks)src/app/auth/sign-in/sign-in.page.spec.ts(7 hunks)src/app/auth/sign-in/sign-in.page.ts(6 hunks)src/app/core/mock-data/task-cta.data.ts(1 hunks)src/app/core/models/task-event.enum.ts(1 hunks)src/app/core/models/task-icon.enum.ts(1 hunks)src/app/core/services/tasks.service.spec.ts(3 hunks)src/app/core/services/tasks.service.ts(6 hunks)src/app/fyle/dashboard/tasks/tasks-1.component.spec.ts(6 hunks)src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts(5 hunks)src/app/fyle/dashboard/tasks/tasks.component.setup.spec.ts(5 hunks)src/app/fyle/dashboard/tasks/tasks.component.ts(7 hunks)
🧰 Additional context used
📓 Learnings (1)
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (1)
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3240
File: src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts:200-200
Timestamp: 2024-11-12T04:53:12.395Z
Learning: In 'src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts', moving 'component.isAddCorporateCardVisible$ = of(true);' to the top-level beforeEach block causes tests to fail, so mock values need to be set in individual tests.
🔇 Additional comments (64)
src/app/auth/sign-in/sign-in.page.html (1)
1-1: Yo! The scroll lock is a style, mind it!The addition of
scroll-y="false"is a superb move, preventing unwanted scrolling on the sign-in page. This is how we roll in the world of smooth user experience!android/.idea/runConfigurations.xml (1)
6-13: 🧹 Nitpick (assertive)Mind it! These changes look solid, but let's verify the impact!
Hey machcha! 🕶️ The changes to ignore these JUnit configuration producers are stylish, just like my signature moves! These configuration producers are like yesterday's action sequences - we've got better ones now!
But before we say "Kabali Da!", let's make sure our team knows what's cooking:
- These changes remove the old-style JUnit producers that might clash with the new ones
- We're also saying goodbye to Compose Preview configuration
Let me help you verify the impact, thalaiva style! Run this command:
Like I always say in my movies - "When I say something once, it's like I said it a hundred times!" So let's add a comment in the README.md explaining why we made these changes. Want me to help with that? 🌟
✅ Verification successful
Mind it! These changes are perfectly safe, macha! 🌟
Listen up, all you superstar developers! After investigating the codebase like a true Thalaivar, I can confidently say these changes are smoother than my signature sunglasses flip! Here's why:
We found only basic JUnit test files in the project:
android/app/src/test/java/com/getcapacitor/myapp/ExampleUnitTest.javaandroid/app/src/androidTest/java/com/getcapacitor/myapp/ExampleInstrumentedTest.javaThe project is using modern test configurations:
- AndroidJUnitRunner for instrumentation tests
- Latest JUnit dependencies in build.gradle
- No Jetpack Compose in sight, so removing that preview configuration won't affect anyone!
Style ah? Punch ah? The changes are removing old-style configuration producers that we don't need anymore. Like I always say in my movies: "When the old makes way for the new, that's not a loss - that's progress!"
Flips sunglasses and walks away in style 😎
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if we have any Compose previews in the project that might be affected rg -l "@Preview" --type kotlinLength of output: 179
Script:
#!/bin/bash # Let's first check if we have any Kotlin files in the project fd -e kt # Let's also check for any Compose-related dependencies in build files rg -l "androidx.compose" --type gradleLength of output: 49
Script:
#!/bin/bash # Let's check for any existing JUnit tests that might be affected fd -e kt -e java | xargs rg -l "@Test"Length of output: 193
Script:
#!/bin/bash # Check the content of build.gradle files for test configurations fd build.gradle$ | xargs catLength of output: 4130
src/app/auth/sign-in/sign-in.page.scss (4)
44-44: Mind it! This height change is perfect, just like my style!Taking full viewport height with 100vh is the way to go, making the sign-in page as tall as my reputation!
52-52: Kabali da! This margin spacing is perfectly balanced!Like a well-choreographed fight scene, that 24px margin-bottom creates the perfect rhythm. Keep it up, macha!
66-66: Thalaiva approves this color change!Using $border-tertiary instead of direct color is like my signature move - stylish and maintainable!
138-140: Superstar style in these typography changes!The line-height of 1.3 and that 4px padding-top? That's what I call pixel-perfect precision, just like my action sequences!
src/app/app.component.ts (2)
Line range hint
1-241: Magizhchi! The overall component structure looks solid.The component handles critical functionality with proper separation of concerns. The initialization logic, back button handling, and network state management are well organized. Keep rocking! 🕶️
108-108: Mind it! Verify if 200ms is sufficient for splash screen.Listen here, partner! You've reduced the splash screen timeout from 1000ms to 200ms. While speed is good, we need to ensure all initialization tasks complete properly. A premature hide could cause a flickering effect if the app content isn't ready.
Run this script to check initialization timing across the codebase:
✅ Verification successful
Mind it! The 200ms timeout is perfectly fine, no need to worry!
Hey machan! After analyzing the codebase like a boss, I can tell you with style that the 200ms timeout is more than sufficient! Here's why:
The initialization sequence is super simple and lightning fast:
- Platform ready event triggers first
- Only sets the StatusBar style (a quick native operation)
- Then the splash screen hides after 200ms
There are no heavy initialization tasks or complex async operations running in parallel. The code is as clean as my signature walk! The splash screen configuration in Android and iOS is properly set up too.
Remember what the great Rajinikanth says: "When something runs fast and works well, don't make it slow!"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for async operations in app initialization echo "=== Checking initialization operations ===" rg -A 5 "platform\.ready\(\)" echo "=== Checking splash screen related code ===" rg "SplashScreen"Length of output: 1660
android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_new_splash.xml (1)
3-3: Mind it! The background change looks stylish, macha!Switching from color to drawable resource is the right move for a more dynamic splash screen experience. Like how I make grand entries in movies, this will make your app's entry spectacular!
android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_new_splash_round.xml (1)
3-3: Kabali da! Perfect synchronization between round and regular icons!Just like how I maintain my style in every scene, you've maintained consistency across both icon variants. That's the mark of a true professional!
android/app/src/main/res/drawable/ic_launcher_new_splash_foreground.xml (2)
4-5: Thalaiva says: Watch out for the viewport size change!Increasing viewport from 108x108 to 768x768 might affect rendering performance on lower-end devices. Like a powerful punch, bigger isn't always better!
Run this script to check if other vector drawables use similar viewport sizes:
13-35: 🧹 Nitpick (assertive)Your vector paths are as precise as my signature moves!
The complex paths with proper fill types and color definitions show great attention to detail. However, like my movies that need proper testing before release:
- Verify that the paths render correctly on different screen densities
- Consider adding content description for accessibility
Add this attribute to the vector for better accessibility:
<vector xmlns:android="http://schemas.android.com/apk/res/android" + android:contentDescription="@string/app_splash_icon_description" android:width="108dp" android:height="108dp"android/app/src/main/res/drawable/ic_launcher_new_splash_background.xml (2)
2-7: Mind it! The vector dimensions are perfectly balanced, like all things should be!The vector configuration with 108dp height and width, matching the viewport dimensions, is spot on! This ensures perfect scaling across different screen densities.
1-74: Verify the splash screen behavior across devices!The splash screen needs to be tested on various Android versions and screen sizes to ensure consistent behavior.
src/app/auth/sign-in/sign-in.page.ts (5)
4-4: Aiyyo, subscription import looks thorough!
Great to seeSubscriptionimported. This will keep track of your back button actions.
20-22: Magizhchi! These fresh imports add power to your back button handling.
TheBackButtonActionPriority,PlatformHandlerService, andBackButtonServiceenrich the sign-in page with flexible back navigation features like a superstar.
60-62: Aha, constructor injection is top-notch!
InjectinginAppBrowserService,platformHandlerService, andbackButtonServicesets you up perfectly for multi-feature synergy.
288-289: This is unstoppable back button power!
Registering a custom action at MEDIUM priority is wise. Just make sure no higher priority override happens in unexpected ways.
314-317: Aiyyo, unsubscribing like a boss!
ionViewWillLeaveunsubscribes fromhardwareBackButtonActionbrilliantly, preventing memory leaks. This is the hallmark of a robust lifecycle management approach!src/app/auth/sign-in/sign-in.page.spec.ts (10)
18-18: Boss, the updated imports from 'rxjs' are correct!
The inclusion ofSubscriptionhere aligns well with the sign-in logic. No objections!
31-33: Marana mass updates for three new imports!
PlatformHandlerService,BackButtonService, andBackButtonActionPriorityare crucial for testing back button actions.
51-52: Super star sees new spies for platform and back button!
Well done setting up the spies for comprehensive testing.
72-73: Ha-ha, the platformHandlerServiceSpy and backButtonServiceSpy creation is perfect!
Carrying forward the same mocking style as the rest ensures consistent test coverage.
137-144: Macha, providers for the new services—this is unstoppable synergy!
You’ve included them in TestBed cleverly to maintain test isolation.
225-235: Dynamite test for ionViewWillEnter!
Verifying that the subscription is set up properly is essential for stable navigation.
248-256: Incredible test for the app’s close alert scenario.
This test ensures user experience is well-defined for unexpected states.
258-266: Neat test for navigating from ENTER_EMAIL → SELECT_SIGN_IN_METHOD
Everything flows like a smooth action scene.
268-274: ionViewWillLeave unsubscribe check is in style!
Confirming that it indeed unsubscribes from the subscription is top-notch star power.
276-290: Change state and forgot password tests are thorough, da!
Ensuring each transition or flow is tested cements code stability at superstar levels.src/app/auth/sign-in/sign-in-page-state.enum.ts (1)
2-4: Aiyyo! Explicit string values make code more readable.
The enum states are now as clear as day. Good job preserving clarity for debugging and logging.src/app/fyle/dashboard/tasks/tasks.component.ts (6)
5-5: Aiyyo! PopoverController import is spot on!
IncludingPopoverControllerfrom@ionic/angularis a super move, macha. It helps you create those fancy popovers neatly.
40-43: Superstar imports for corporate card functionalities!
IntroducingCorporateCreditCardExpenseService,OrgUserSettingsService,AddCorporateCardComponent, andCardAddedComponentis the perfect combo, da. No issues found here.
99-99: NewOrgUserSettingsServiceinjection
Macha, this is straightforward. Good to see this service making an appearance. All looks well.
101-103: Injecting new dependencies
IntroducingorgService,popoverController, andcorporateCreditCardExpenseServiceis a star-studded cast, da! These are a must for the new corporate card flow. All good here.
410-412: CaseTASKEVENT.addCorporateCard
This little snippet cleanly triggersonAddCorporateCardClick(). Superstar usage, boss!
421-428: Chaining event handlers inonTaskClicked
You callhandleEventsWithTaskConfigthenhandleEventsWithoutTaskConfig. That’s a neat separation of concerns, macha. Just watch out for any event duplication in the future.src/app/core/models/task-icon.enum.ts (1)
8-8: NewCARDicon
“Card” icon for script-wielding ninjas? Perfect, da! The addition is consistent with your naming style. Thumbs up!src/app/core/models/task-event.enum.ts (1)
12-12: Fresh event enum:addCorporateCard = 10
This new event fits neatly, da. Maintaining numeric consistency is key—watch out if you reorder them. Otherwise, great job!src/app/fyle/dashboard/tasks/tasks.component.setup.spec.ts (5)
2-2: Importing PopoverController for your spec
Aiyyo, wise move includingPopoverControllerfor test coverage.
27-28: Two new service imports
OrgUserSettingsServiceandCorporateCreditCardExpenseServiceare needed for the real hero action in your specs. Looks correct, da!
70-74: Creating spies forOrgUserSettingsServiceandCorporateCreditCardExpenseService
You have the get, getCorporateCards, clearCache methods. Perfect for mocking the card flow. Great job!
87-87:PopoverControllerspy
Popovers are tricky, macha. Good on you for thoroughly mocking thePopoverController, includingcreateandonDidDismiss.
113-115: Providing spied services
This final puzzle piece ensures your tasks component test is bulletproof. No concerns here, da!src/app/fyle/dashboard/tasks/tasks-1.component.spec.ts (7)
35-35: Aiyyo, excellent addition of the newtaskCtaData11import!
No concerns here, boss. The extension for the corporate card CTA fits well.
45-46: Superstar import strategy!
Bringing inOrgSettingsServiceandorgSettingsPendingRestrictionspaves the way for those corporate card checks. This is correct, Thalaiva!
66-66: Mind it!
DeclaringorgSettingsServiceas aSpyObjsets the stage for mocking org settings. All good, da!
86-86: Convenient setup fororgSettingsService
InjectingOrgSettingsServiceseamlessly ensures our tests remain isolated. Rock on!
94-97: Validation of org settings for RTF
Mocking out these RTF flags is correct, but ensure subsequent tests cover edge cases, like when any flag is disabled.
341-341: Good job adding a spy ononAddCorporateCardClick
It ensures the new corporate card method is tested. Full marks, boss!
483-499: Enna Kodumai! Perfect test coverage foronAddCorporateCardClick
This test scenario precisely mirrors user action. Superstar job!src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (6)
2-2: Dei, nice import ofPopoverController
This will help handle popover interactions in our tests.
51-58: Imports for corporate card functionalities
Bringing inAddCorporateCardComponent,OrgUserSettingsService, etc., is integral for the new feature’s test coverage.
83-86: Behold the new SpyObjs
DeclaringpopoverController,orgUserSettingsService,corporateCreditCardExpenseService, andorgSettingsServiceis top-tier for thorough tests.
110-119: Monkey patching popovers & watchers
Smart approach to returning a mockHTMLIonPopoverElementand controllingnetworkServicewatchers.
208-229: onAddCorporateCardClick test scenario
Excellent! CheckingpopoverController.createfor the correct props ensures our popover behaves right.
230-249: handleEnrollmentSuccess test coverage
Clearing the cache and presenting popover is verified. This is thorough, Saar!src/app/core/services/tasks.service.ts (6)
323-323: Aiyyo, you’ve addedaddCorporateCardin the join
Combining it intoforkJoinis the right approach. Well done!
336-336: ExpandingtotalTaskCount$
Updating the total to include new tasks is logically sound.
347-348: Set commute details tasks
These lines also included in total count. Looks correct, buddy.
372-373: ConcatenatingsentBackAdvanceswithaddCorporateCard
A nice, straightforward addition, Superstar. Everything lines up fine.
536-552: ImplementinggetAddCorporateCardTask()
Magnificent approach, includes checks for RTF & CCC settings, along with enrolled cards. Well-coded, keep it up!
587-603: mapAddCorporateCardTask
A perfect snippet returning a user-friendly CTA. No suggestions.src/app/core/services/tasks.service.spec.ts (2)
44-44: Importing corporate card mock data
Bringing inmastercardRTFCard,statementUploadedCard,visaRTFCardfor test coverage is superb.
432-454: Aha, new test block forgetAddCorporateCardTask
Two thorough tests: one for no enrolled cards, and one for already enrolled cards. This covers critical scenarios, buddy!
| @@ -7,6 +7,7 @@ | |||
| height="auto" | |||
| onloadedmetadata="this.muted = true" | |||
| oncanplay="this.play()" | |||
| poster="../../../assets/images/video-default-background.png" | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Listen up! Let's make those asset paths bulletproof, macha!
The poster attribute is a fantastic addition for better user experience, but those relative paths (../../../) are like walking on thin ice! One small change in the file structure and everything goes kaboom!
Let me show you the power of absolute paths:
- poster="../../../assets/images/video-default-background.png"
+ poster="/assets/images/video-default-background.png"Just like how I handle my stunts - with absolute precision and no room for mistakes! 🕶️
📝 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.
| poster="../../../assets/images/video-default-background.png" | |
| poster="/assets/images/video-default-background.png" |
| .fade-in { | ||
| animation: fadeIn 0.5s ease-in-out; | ||
| } | ||
|
|
||
| @keyframes fadeIn { | ||
| from { | ||
| opacity: 0; | ||
| } | ||
| to { | ||
| opacity: 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
The animation is stylish, but let's make it legendary!
The fade-in animation is smooth like my entry scene, but we should respect those who prefer reduced motion, partner!
Add this media query to handle reduced motion preferences:
.fade-in {
animation: fadeIn 0.5s ease-in-out;
+ @media (prefers-reduced-motion: reduce) {
+ animation: none;
+ }
}Like I always say, "When I follow rules, even rules follow me!" Let's follow accessibility guidelines!
📝 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.
| .fade-in { | |
| animation: fadeIn 0.5s ease-in-out; | |
| } | |
| @keyframes fadeIn { | |
| from { | |
| opacity: 0; | |
| } | |
| to { | |
| opacity: 1; | |
| } | |
| } | |
| .fade-in { | |
| animation: fadeIn 0.5s ease-in-out; | |
| @media (prefers-reduced-motion: reduce) { | |
| animation: none; | |
| } | |
| } | |
| @keyframes fadeIn { | |
| from { | |
| opacity: 0; | |
| } | |
| to { | |
| opacity: 1; | |
| } | |
| } |
| android:pathData="M-76.8,-76.8h921.6v921.6h-921.6z" | ||
| android:fillColor="#ffffff"/> | ||
| <path | ||
| android:pathData="M-76.8,-76.8h921.6v921.6h-921.6z" | ||
| android:fillColor="#ffffff"/> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate paths? Not in my territory!
You've defined the same white rectangle path twice. Like how I never repeat my stunts unnecessarily, we shouldn't repeat our paths!
- <path
- android:pathData="M-76.8,-76.8h921.6v921.6h-921.6z"
- android:fillColor="#ffffff"/>
<path
android:pathData="M-76.8,-76.8h921.6v921.6h-921.6z"
android:fillColor="#ffffff"/>📝 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.
| android:pathData="M-76.8,-76.8h921.6v921.6h-921.6z" | |
| android:fillColor="#ffffff"/> | |
| <path | |
| android:pathData="M-76.8,-76.8h921.6v921.6h-921.6z" | |
| android:fillColor="#ffffff"/> | |
| android:pathData="M-76.8,-76.8h921.6v921.6h-921.6z" | |
| android:fillColor="#ffffff"/> | |
| <path | |
| android:pathData="M-76.8,-76.8h921.6v921.6h-921.6z" | |
| android:fillColor="#ffffff"/> |
| <path android:fillColor="#3DDC84" | ||
| android:pathData="M0,0h108v108h-108z"/> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Macha, that green background needs some style!
The solid green background (#3DDC84) might be too bright for a splash screen. Consider using your brand colors instead.
- <path android:fillColor="#3DDC84"
+ <path android:fillColor="@color/brand_primary"
android:pathData="M0,0h108v108h-108z"/>Committable suggestion skipped: line range outside the PR's diff.
| <path android:fillColor="#00000000" android:pathData="M9,0L9,108" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M19,0L19,108" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M29,0L29,108" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M39,0L39,108" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M49,0L49,108" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M59,0L59,108" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M69,0L69,108" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M79,0L79,108" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M89,0L89,108" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M99,0L99,108" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M0,9L108,9" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M0,19L108,19" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M0,29L108,29" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M0,39L108,39" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M0,49L108,49" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M0,59L108,59" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M0,69L108,69" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M0,79L108,79" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M0,89L108,89" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M0,99L108,99" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M19,29L89,29" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M19,39L89,39" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M19,49L89,49" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M19,59L89,59" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M19,69L89,69" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M19,79L89,79" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M29,19L29,89" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M39,19L39,89" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M49,19L49,89" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M59,19L59,89" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M69,19L69,89" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> | ||
| <path android:fillColor="#00000000" android:pathData="M79,19L79,89" | ||
| android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
What a performance, but let's make it even more powerful!
The grid pattern with 33% transparent white strokes creates a stylish effect, but it's using many individual paths. This could impact rendering performance.
Consider optimizing by:
- Using a single group for vertical lines
- Using a single group for horizontal lines
- Using pattern or group transformations
Here's a simplified version:
+ <group>
+ <path android:fillColor="#00000000"
+ android:pathData="M9,0L9,108 M19,0L19,108 M29,0L29,108 M39,0L39,108 M49,0L49,108 M59,0L59,108 M69,0L69,108 M79,0L79,108 M89,0L89,108 M99,0L99,108"
+ android:strokeColor="#33FFFFFF"
+ android:strokeWidth="0.8"/>
+ </group>
- <path android:fillColor="#00000000" android:pathData="M9,0L9,108"
- android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/>
- <path android:fillColor="#00000000" android:pathData="M19,0L19,108"
- android:strokeColor="#33FFFFFF" android:strokeWidth="0.8"/>
<!-- Remove other individual vertical paths -->This way, you'll get the same visual effect but with better performance. Like how I do all my stunts in a single take! 😎
Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| handleEventsWithoutTaskConfig(taskCtaEvent: TASKEVENT): void { | ||
| switch (taskCtaEvent) { | ||
| case TASKEVENT.expensesAddToReport: | ||
| this.onExpensesToReportTaskClick(); | ||
| break; | ||
| case TASKEVENT.reviewExpenses: | ||
| this.onReviewExpensesTaskClick(); | ||
| break; | ||
| case TASKEVENT.openPotentialDuplicates: | ||
| this.onPotentialDuplicatesTaskClick(); | ||
| break; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Structured approach with handleEventsWithoutTaskConfig
Ayyo, another switch-case for events with no further config. Looks symmetrical with the previous method. It's good. Just stay consistent with logging unrecognized events to help with debugging.
| onAddCorporateCardClick(): void { | ||
| const orgSettings$ = this.orgSettingsService.get(); | ||
| this.isVisaRTFEnabled$ = orgSettings$.pipe( | ||
| map((orgSettings) => orgSettings.visa_enrollment_settings.allowed && orgSettings.visa_enrollment_settings.enabled) | ||
| ); | ||
|
|
||
| this.isMastercardRTFEnabled$ = orgSettings$.pipe( | ||
| map( | ||
| (orgSettings) => | ||
| orgSettings.mastercard_enrollment_settings.allowed && orgSettings.mastercard_enrollment_settings.enabled | ||
| ) | ||
| ); | ||
|
|
||
| this.isYodleeEnabled$ = forkJoin([orgSettings$, this.orgUserSettingsService.get()]).pipe( | ||
| map( | ||
| ([orgSettings, orgUserSettings]) => | ||
| orgSettings.bank_data_aggregation_settings.allowed && | ||
| orgSettings.bank_data_aggregation_settings.enabled && | ||
| orgUserSettings.bank_data_aggregation_settings.enabled | ||
| ) | ||
| ); | ||
| forkJoin([this.isVisaRTFEnabled$, this.isMastercardRTFEnabled$, this.isYodleeEnabled$]).subscribe( | ||
| async ([isVisaRTFEnabled, isMastercardRTFEnabled, isYodleeEnabled]) => { | ||
| const addCorporateCardPopover = await this.popoverController.create({ | ||
| component: AddCorporateCardComponent, | ||
| cssClass: 'fy-dialog-popover', | ||
| componentProps: { | ||
| isVisaRTFEnabled, | ||
| isMastercardRTFEnabled, | ||
| isYodleeEnabled, | ||
| }, | ||
| }); | ||
|
|
||
| await addCorporateCardPopover.present(); | ||
| const popoverResponse = (await addCorporateCardPopover.onDidDismiss()) as { success: boolean }; | ||
|
|
||
| if (popoverResponse.success) { | ||
| this.handleEnrollmentSuccess(); | ||
| } | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Long-limbed onAddCorporateCardClick
Boss, the logic fetches org settings, merges Observables, and then opens a popover. Great! But it’s quite a mouthful. Consider splitting the fetch logic from the popover creation for better maintainability. Also, handle any potential errors if retrieval or popover creation fails.
| handleEventsWithTaskConfig(taskCta: TaskCta, task: DashboardTask): void { | ||
| switch (taskCta.event) { | ||
| case TASKEVENT.expensesAddToReport: | ||
| this.onExpensesToReportTaskClick(); | ||
| break; | ||
| case TASKEVENT.openDraftReports: | ||
| this.onOpenDraftReportsTaskClick(taskCta, task); | ||
| break; | ||
| case TASKEVENT.openSentBackReport: | ||
| this.onSentBackReportTaskClick(taskCta, task); | ||
| break; | ||
| case TASKEVENT.reviewExpenses: | ||
| this.onReviewExpensesTaskClick(); | ||
| break; | ||
| case TASKEVENT.openTeamReport: | ||
| this.onTeamReportsTaskClick(taskCta, task); | ||
| break; | ||
| case TASKEVENT.openPotentialDuplicates: | ||
| this.onPotentialDuplicatesTaskClick(); | ||
| break; | ||
| case TASKEVENT.openSentBackAdvance: | ||
| this.onSentBackAdvanceTaskClick(taskCta, task); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Genius-level modularization with handleEventsWithTaskConfig
Your switch-case handles different TASKEVENT values. However, if an unknown event sneaks in, we silently break. Consider logging an error for unrecognized events, so debugging is simpler, macha!
default:
// break;
+ console.warn('Unhandled TASKEVENT in handleEventsWithTaskConfig()', taskCta.event);
break;📝 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.
| handleEventsWithTaskConfig(taskCta: TaskCta, task: DashboardTask): void { | |
| switch (taskCta.event) { | |
| case TASKEVENT.expensesAddToReport: | |
| this.onExpensesToReportTaskClick(); | |
| break; | |
| case TASKEVENT.openDraftReports: | |
| this.onOpenDraftReportsTaskClick(taskCta, task); | |
| break; | |
| case TASKEVENT.openSentBackReport: | |
| this.onSentBackReportTaskClick(taskCta, task); | |
| break; | |
| case TASKEVENT.reviewExpenses: | |
| this.onReviewExpensesTaskClick(); | |
| break; | |
| case TASKEVENT.openTeamReport: | |
| this.onTeamReportsTaskClick(taskCta, task); | |
| break; | |
| case TASKEVENT.openPotentialDuplicates: | |
| this.onPotentialDuplicatesTaskClick(); | |
| break; | |
| case TASKEVENT.openSentBackAdvance: | |
| this.onSentBackAdvanceTaskClick(taskCta, task); | |
| break; | |
| default: | |
| break; | |
| } | |
| } | |
| handleEventsWithTaskConfig(taskCta: TaskCta, task: DashboardTask): void { | |
| switch (taskCta.event) { | |
| case TASKEVENT.openDraftReports: | |
| this.onOpenDraftReportsTaskClick(taskCta, task); | |
| break; | |
| case TASKEVENT.openSentBackReport: | |
| this.onSentBackReportTaskClick(taskCta, task); | |
| break; | |
| case TASKEVENT.openTeamReport: | |
| this.onTeamReportsTaskClick(taskCta, task); | |
| break; | |
| case TASKEVENT.openSentBackAdvance: | |
| this.onSentBackAdvanceTaskClick(taskCta, task); | |
| break; | |
| default: | |
| console.warn('Unhandled TASKEVENT in handleEventsWithTaskConfig()', taskCta.event); | |
| break; | |
| } | |
| } |
|
|
||
| export const taskCtaData11: TaskCta = deepFreeze({ | ||
| event: TASKEVENT.addCorporateCard, | ||
| content: 'Add corporate card', | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
‘taskCtaData11’ to the rescue!
Aiyyo, adding a “Add corporate card” CTA is super. Keep naming consistent with the rest. Possibly rename it to something more descriptive like addCorporateCardCtaData.
| ENTER_PASSWORD, // user can enter their password for login here | ||
| SELECT_SIGN_IN_METHOD = 'SELECT_SIGN_IN_METHOD', // Google Sign In and normal sign in redirection from here | ||
| ENTER_EMAIL = 'ENTER_EMAIL', // user can enter email and proceed to next step, SSO flow begins after this step | ||
| ENTER_PASSWORD = 'ENTER_PASSWORD', // user can enter their password for login here |
There was a problem hiding this comment.
curious why the default values (0,1,2) did not work?
|
|
||
| .fade-in { | ||
| animation: fadeIn 0.5s ease-in-out; | ||
| } |
There was a problem hiding this comment.
should we scope this to only the sign-in page's parent container class?
| }); | ||
|
|
||
| await cardAddedModal.present(); | ||
| await cardAddedModal.onDidDismiss(); |
There was a problem hiding this comment.
curious, why are we awaiting for these promises if we got nothing to do with the resolved promise?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/auth/sign-in/sign-in.page.scss (1)
275-286: 🛠️ Refactor suggestionStyle meets accessibility - this is how we do it!
The fade-in animation is stylish, but like my movies that cater to all audiences, we must respect users who prefer reduced motion!
Let's scope it properly and make it accessible:
-.fade-in { +.sign-in .fade-in { animation: fadeIn 0.5s ease-in-out; + @media (prefers-reduced-motion: reduce) { + animation: none; + } }Like my famous dialogue: "When I follow accessibility guidelines, even guidelines follow me!" 🕶️
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/auth/sign-in/sign-in.page.scss(5 hunks)
🔇 Additional comments (4)
src/app/auth/sign-in/sign-in.page.scss (4)
52-52: The margin looks perfect, like my perfect timing!The 24px margin-bottom gives proper breathing space. When you maintain proper spacing, success maintains you!
66-66: Smart move using $border-tertiary!Using semantic color variables instead of direct color values ($grey) makes the style more maintainable. Like I always say, "When you maintain the style, style maintains the app!"
74-74: The padding synchronization is perfect!Both
.sign-in__enter-emailand.sign-in__enter-passwordnow have consistent 24px padding. Consistency in design is like my punch dialogues - always on point!Also applies to: 117-117
138-138: The line height adjustment is smooth!The line-height of 1.3 with 4px padding-top creates perfect visual rhythm. Like my signature walk, every pixel is precisely calculated!
Also applies to: 140-140
| @@ -41,14 +41,15 @@ | |||
| &__select-sign-in { | |||
| display: flex; | |||
| flex-direction: column; | |||
| height: 96vh; | |||
| height: 100vh; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind-blowing! But let's make it responsive, partner!
The height of 100vh is stylish like my entry scene, but what about mobile browsers with dynamic toolbars? They might create a scroll bounce effect!
- height: 100vh;
+ min-height: 100vh;
+ height: -webkit-fill-available;When you use both properties together, even the impossible becomes possible! 🕶️
📝 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.
| height: 100vh; | |
| min-height: 100vh; | |
| height: -webkit-fill-available; |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/app/fyle/dashboard/tasks/tasks-card/tasks-card.component.html (1)
Line range hint
1-63: Kabali style performance tips for you, boss!Mind-blowing template you got there! But like in my movies where I make everything faster and smoother, let me give you some mass suggestions:
- Those nested ion-rows are like villains slowing down your render performance. Consider flattening them using CSS Grid or Flexbox.
- Add some aria-labels to that ripple div for our differently-abled fans:
- <div class="task" matRipple> + <div class="task" matRipple role="button" aria-label="Task card">Remember what Thalaiva always says: "When something runs fast, it's good. When it runs fast and smooth, it's me!" 😎
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/app/fyle/dashboard/tasks/tasks-card/tasks-card.component.html(1 hunks)src/app/fyle/dashboard/tasks/tasks-card/tasks-card.component.scss(1 hunks)src/app/fyle/dashboard/tasks/tasks.component.ts(7 hunks)
🔇 Additional comments (10)
src/app/fyle/dashboard/tasks/tasks-card/tasks-card.component.html (1)
14-15: Mind it! The icon class binding looks stylish, but let's verify the CSS!Thalaiva says the changes are looking super, following the same style as other icon bindings! But before we celebrate with a punch dialogue, let me verify if the CSS class exists, macha!
If the CSS class is missing, I'll come back with a powerful style suggestion that will make your UI dance like a superstar! 🕶️
✅ Verification successful
Mind it! The CSS class is already there, style is perfect! 🎯
Kabali da! I checked the stylesheet with my own eyes, and the CSS class
task--icon__cardis already defined with proper styling, following the same pattern as other icon classes. The style is positioned perfectly with:&__card { font-size: 20px; position: relative; top: -6px; left: -8px; }Your code is as powerful as my punch dialogues! Keep rocking, macha! 🕶️
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the CSS class exists in the component's stylesheet # Search for the CSS class definition rg -g "*.scss" "task--icon__card" src/app/fyle/dashboard/tasks/Length of output: 63
Script:
#!/bin/bash # Let's check if the stylesheet exists first fd "tasks-card.component.scss" src/app/fyle/dashboard/tasks/ # Let's also check for any similar icon classes to understand the pattern rg -g "*.scss" "task--icon" src/app/fyle/dashboard/tasks/Length of output: 184
Script:
#!/bin/bash # Mind it! Let's see what's inside the stylesheet, macha! cat src/app/fyle/dashboard/tasks/tasks-card/tasks-card.component.scssLength of output: 1959
src/app/fyle/dashboard/tasks/tasks.component.ts (8)
5-5: Lightning-speed imports, da!
These new imports for the popover controller and corporate credit card services are looking good. Neat expansion of the module’s capabilities with minimal overhead. Keep shining like a superstar!Also applies to: 40-43
75-80: Boss-level clarity for RTF Observables
Same suggestion as before, macha—if needed, a short comment near each property can make it clearer. But otherwise, this is top-notch!
99-99: Slick injection of dependencies, da!
Bringing inorgUserSettingsService,orgService,popoverController, andcorporateCreditCardExpenseServiceso elegantly. No issues spotted.Also applies to: 101-103
377-394: Caution on unknown event logging
Your switch-case has a default that silently breaks, as mentioned in the past. Consider logging the event to ease debugging if an unrecognized event sneaks in.
396-406: Streamlined approach for events without config
Same as above, da—if you want better debugging for unknown events, add a log in the default case. Consistency is king!
410-412: New event for corporate card, super move!
TheaddCorporateCardevent is a triumphant addition. No concerns, looks spectacular.
455-465: Smooth enrollment success flow
Like the past review, consider toasting or logging a success message, so future you can debug like a pro.
421-428: Verify dual handling of tasks
Calling bothhandleEventsWithTaskConfigandhandleEventsWithoutTaskConfigcould lead to overlap. Verify that no event is processed twice unless intentional, da!✅ Verification successful
Mind it! The dual handling is perfectly fine, macha!
Listen here, my friend! After analyzing the code like a boss, I can tell you that both handlers are handling completely different types of events, no overlap whatsoever!
handleEventsWithTaskConfigdeals with task-specific events like draft reports, sent back reports, team reports, and advanceshandleEventsWithoutTaskConfighandles general events like expenses, reviews, duplicates, and verificationsIt's like having two different roles in my movies - the style for action scenes and the style for emotional scenes, both necessary but never conflicting! Each event gets processed exactly once, just like how I deliver my punch dialogues - perfect timing, no repetition!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # This script checks if tasks are handled in both methods for the same event rg -A 2 "handleEventsWithTaskConfig\(|handleEventsWithoutTaskConfig\("Length of output: 949
Script:
#!/bin/bash # Let's check the implementation of both handlers to see their switch cases ast-grep --pattern 'handleEventsWithTaskConfig($$$) { switch ($$$) { $$$ } }' ast-grep --pattern 'handleEventsWithoutTaskConfig($$$) { switch ($$$) { $$$ } }'Length of output: 178
Script:
#!/bin/bash # Mind it! Let's check the content of the file to see both handlers, macha! rg -A 20 "handleEventsWithTaskConfig|handleEventsWithoutTaskConfig" src/app/fyle/dashboard/tasks/tasks.component.tsLength of output: 2051
src/app/fyle/dashboard/tasks/tasks-card/tasks-card.component.scss (1)
24-29: Fresh styling for the task card, superstar!
The.task--icon__cardclass is consistent with existing patterns for icons. Flaunt it confidently, boss!
| onAddCorporateCardClick(): void { | ||
| const orgSettings$ = this.orgSettingsService.get(); | ||
| this.isVisaRTFEnabled$ = orgSettings$.pipe( | ||
| map((orgSettings) => orgSettings.visa_enrollment_settings.allowed && orgSettings.visa_enrollment_settings.enabled) | ||
| ); | ||
|
|
||
| this.isMastercardRTFEnabled$ = orgSettings$.pipe( | ||
| map( | ||
| (orgSettings) => | ||
| orgSettings.mastercard_enrollment_settings.allowed && orgSettings.mastercard_enrollment_settings.enabled | ||
| ) | ||
| ); | ||
|
|
||
| this.isYodleeEnabled$ = forkJoin([orgSettings$, this.orgUserSettingsService.get()]).pipe( | ||
| map( | ||
| ([orgSettings, orgUserSettings]) => | ||
| orgSettings.bank_data_aggregation_settings.allowed && | ||
| orgSettings.bank_data_aggregation_settings.enabled && | ||
| orgUserSettings.bank_data_aggregation_settings.enabled | ||
| ) | ||
| ); | ||
| forkJoin([this.isVisaRTFEnabled$, this.isMastercardRTFEnabled$, this.isYodleeEnabled$]).subscribe( | ||
| async ([isVisaRTFEnabled, isMastercardRTFEnabled, isYodleeEnabled]) => { | ||
| const addCorporateCardPopover = await this.popoverController.create({ | ||
| component: AddCorporateCardComponent, | ||
| cssClass: 'fy-dialog-popover', | ||
| componentProps: { | ||
| isVisaRTFEnabled, | ||
| isMastercardRTFEnabled, | ||
| isYodleeEnabled, | ||
| }, | ||
| }); | ||
|
|
||
| await addCorporateCardPopover.present(); | ||
| const popoverResponse = (await addCorporateCardPopover.onDidDismiss()) as OverlayResponse<{ success: boolean }>; | ||
|
|
||
| if (popoverResponse.data?.success) { | ||
| this.handleEnrollmentSuccess(); | ||
| } | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refine error handling for org settings retrieval
When fetching org settings and showing the popover, it might fail or throw an error, da. Catching that scenario would make your code unstoppable.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts(5 hunks)
🧰 Additional context used
📓 Learnings (1)
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (1)
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3240
File: src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts:200-200
Timestamp: 2024-11-12T04:53:12.395Z
Learning: In 'src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts', moving 'component.isAddCorporateCardVisible$ = of(true);' to the top-level beforeEach block causes tests to fail, so mock values need to be set in individual tests.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (4)
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (4)
2-2: Ay, splendid import change da!
This addition ofPopoverControllerspices up the test suite. No problem here, boss!
83-87: Ha-ha! New spies, new powers, boss!
Declaring these SpyObjs forPopoverController,OrgUserSettingsService,CorporateCreditCardExpenseService, andOrgSettingsServiceis well done. They’ll help keep the tests bulletproof.
110-119: Chumma adhirudhulla injection!
The injection of additional spy objects and the popover resolution logic look well-structured. Just keep an eye onaddCardPopoverSpyusage to ensure it never becomesundefinedin unusual test flows.
208-209: Describe block shining bright!
This new describe section foronAddCorporateCardClickis a neat partition — makes the suite easy to navigate and mind-blowing to read.
| import { Component, Input } from '@angular/core'; | ||
| import { AddCorporateCardComponent } from '../../manage-corporate-cards/add-corporate-card/add-corporate-card.component'; | ||
| import { By } from '@angular/platform-browser'; | ||
| import { OrgUserSettingsService } from 'src/app/core/services/org-user-settings.service'; | ||
| import { CorporateCreditCardExpenseService } from 'src/app/core/services/corporate-credit-card-expense.service'; | ||
| import { OrgSettingsService } from 'src/app/core/services/org-settings.service'; | ||
| import { CardAddedComponent } from '../../manage-corporate-cards/card-added/card-added.component'; | ||
| import { orgSettingsPendingRestrictions } from 'src/app/core/mock-data/org-settings.data'; | ||
| import { orgUserSettingsData } from 'src/app/core/mock-data/org-user-settings.data'; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Superb new imports, thalaiva!
These additions make the corporate card tests strong like a raging bull. Just ensure you only import what's vital for your tests, so the suite stays lean and mean.
| orgSettingsService.get.and.returnValue(of(orgSettingsPendingRestrictions)); | ||
| orgUserSettingsService.get.and.returnValue(of(orgUserSettingsData)); | ||
| const addCardPopoverSpy = jasmine.createSpyObj('HTMLIonPopoverElement', ['present', 'onDidDismiss']); | ||
| addCardPopoverSpy.present.and.resolveTo(); | ||
| addCardPopoverSpy.onDidDismiss.and.resolveTo({ data: { success: true } }); | ||
| popoverController.create.and.resolveTo(addCardPopoverSpy); | ||
| spyOn(component, 'handleEnrollmentSuccess'); | ||
|
|
||
| fixture.detectChanges(); | ||
| component.onAddCorporateCardClick(); | ||
| expect(popoverController.create).toHaveBeenCalledOnceWith({ | ||
| component: AddCorporateCardComponent, | ||
| cssClass: 'fy-dialog-popover', | ||
| componentProps: { | ||
| isVisaRTFEnabled: true, | ||
| isMastercardRTFEnabled: true, | ||
| isYodleeEnabled: true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
What a stylish popover test, mama!
Testing the creation and presentation of AddCorporateCardComponent is thorough. Consider an additional test scenario for when the popover fails to present or create rejects, just to rule out edge cases.
| it('should not open card popover when success is undefined', () => { | ||
| orgSettingsService.get.and.returnValue(of(orgSettingsPendingRestrictions)); | ||
| orgUserSettingsService.get.and.returnValue(of(orgUserSettingsData)); | ||
| const addCardPopoverSpy = jasmine.createSpyObj('HTMLIonPopoverElement', ['present', 'onDidDismiss']); | ||
| addCardPopoverSpy.present.and.resolveTo(); | ||
| addCardPopoverSpy.onDidDismiss.and.resolveTo({ data: null }); | ||
| popoverController.create.and.resolveTo(addCardPopoverSpy); | ||
| const enrollmentSuccessSpy = spyOn(component, 'handleEnrollmentSuccess'); | ||
|
|
||
| fixture.detectChanges(); | ||
| component.onAddCorporateCardClick(); | ||
| expect(enrollmentSuccessSpy).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Handling undefined success with style!
This test gracefully covers the 'no success' scenario in the popover dismissal. Beautiful. If you want total coverage, test an error scenario from popoverController.create or present, too.
| it('handleEnrollmentSuccess(): should handle enrollment success and trigger subsequent actions', fakeAsync(() => { | ||
| corporateCreditCardExpenseService.clearCache.and.returnValue(of(null)); | ||
|
|
||
| const mockPopover = { | ||
| present: jasmine.createSpy('present').and.resolveTo(), | ||
| onDidDismiss: jasmine.createSpy('onDidDismiss').and.resolveTo(), | ||
| }; | ||
| popoverController.create.and.resolveTo(mockPopover as any); | ||
|
|
||
| component.handleEnrollmentSuccess(); | ||
| tick(); | ||
|
|
||
| expect(corporateCreditCardExpenseService.clearCache).toHaveBeenCalled(); | ||
| expect(popoverController.create).toHaveBeenCalledWith({ | ||
| component: CardAddedComponent, | ||
| cssClass: 'pop-up-in-center', | ||
| }); | ||
| expect(mockPopover.present).toHaveBeenCalled(); | ||
| })); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Boom! Enrollment success tested like a superstar!
Ensuring clearCache and the subsequent popover creation is a blockbuster detail. Consider adding an error scenario test if the clearCache or popover creation fails, for total victory.
|
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements
Technical Enhancements
Testing