[e2e] Mobile budget menu modal VRT #3583
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
WalkthroughThis pull request introduces several enhancements to the mobile testing framework, particularly in budget management and navigation functionalities. Key changes include the addition of five new asynchronous utility functions: Modifications to the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 5
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/e2e/budget.mobile.test.js (1)
261-271: Clarify the budget amount input formatIn the test case, you're setting the budget amount using the string
'12300':await budgetMenuModal.setBudgetAmount('12300');And then expecting the budgeted button to display
'123.00':await expect(budgetedButton).toHaveText('123.00');This might cause confusion regarding the input format for
setBudgetAmount. If the amount is expected in cents, consider adding a comment to clarify this. Alternatively, input the amount as a decimal string to make the code clearer:- await budgetMenuModal.setBudgetAmount('12300'); + await budgetMenuModal.setBudgetAmount('123.00');This makes the intention explicit and improves code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (24)
packages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-updates-the-budgeted-amount-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-updates-the-budgeted-amount-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-updates-the-budgeted-amount-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (6)
- packages/desktop-client/e2e/budget.mobile.test.js (2 hunks)
- packages/desktop-client/e2e/page-models/account-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/close-account-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-page.js (2 hunks)
- packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-client/e2e/page-models/account-page.js
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/e2e/page-models/close-account-modal.js (1)
Learnt from: UnderKoen PR: actualbudget/actual#3499 File: packages/desktop-client/e2e/page-models/close-account-modal.js:16-18 Timestamp: 2024-09-24T20:20:59.061Z Learning: In the `CloseAccountModal` class, methods are implemented without additional error handling, return values, or additional comments to maintain consistency with existing codebase practices.
🔇 Additional comments (11)
packages/desktop-client/e2e/page-models/close-account-modal.js (4)
2-4: LGTM! Constructor update improves element interaction.The change from
rootPagetolocatorin the constructor signature and assignment is a good improvement. This update likely allows for more precise and flexible element selection, which is beneficial for robust end-to-end testing.
8-9: Great improvement in element selection and user simulation!The changes in this method enhance the robustness of the test:
- Using
getByPlaceholderfor element selection is more reliable and readable.- Adding the
Enterkey press simulates real user behavior more accurately.These updates will likely result in more stable and realistic tests.
13-13: Excellent use of accessibility-aware selector!The change to use
getByRolewith a specific button name is a significant improvement. This approach:
- Makes the test more resilient to UI changes.
- Aligns with accessibility best practices.
- Improves the test's readability and maintainability.
This update demonstrates a commitment to both robust testing and accessible UI design.
1-15: Overall excellent improvements to the CloseAccountModal class!The changes made to this class consistently enhance the quality and reliability of the end-to-end tests:
- The shift from
rootPagetolocatorallows for more precise element selection.- The use of semantic selectors like
getByPlaceholderandgetByRoleimproves test robustness and aligns with accessibility best practices.- The addition of keyboard interactions in
selectTransferAccountmore accurately simulates user behavior.These updates maintain the class's simplicity while significantly improving its effectiveness in testing. Great work on modernizing and strengthening these tests!
packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.js (4)
1-23: LGTM: Well-structured class following Page Object Model patternThe
BudgetMenuModalclass is well-structured and follows the Page Object Model pattern, which is excellent for e2e testing. The constructor properly initializes the page and locator, and the UI elements are well-defined using appropriate locator strategies.
25-27: LGTM: Concise and accessible close() methodThe
close()method is well-implemented. It uses a role-based selector to find the close button, which is good for accessibility and test resilience.
29-33: LGTM: Comprehensive setBudgetAmount() methodThe
setBudgetAmount()method is well-implemented. It properly handles type conversion, triggers blur events, and follows up with closing the modal, which represents a complete interaction flow.
35-53: LGTM: Consistent and clear action methodsThe action methods (
copyLastMonthBudget(),setTo3MonthAverage(),setTo6MonthAverage(),setToYearlyAverage(), andapplyBudgetTemplate()) are consistently implemented. They are concise, use pre-defined locators, and have clear, descriptive names.packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (1)
153-153: Approve change and update related testsThe change from
"amount-fake-input"to"amount-input-text"improves the clarity of the test identifier. This new identifier more accurately describes the purpose of the Text component, which displays the amount either as editing text or formatted currency.Please ensure that any existing tests using the previous test identifier
"amount-fake-input"are updated to use"amount-input-text". Run the following script to check for any occurrences of the old test identifier:If any occurrences are found, update them to use the new test identifier.
packages/desktop-client/e2e/page-models/mobile-budget-page.js (1)
2-2: LGTM: New import for BudgetMenuModal.The new import statement for
BudgetMenuModalis correctly placed and follows the existing import style in the file. This import is likely related to the changes in theopenBudgetMenumethod, indicating a refactoring of the budget menu functionality.packages/desktop-client/e2e/budget.mobile.test.js (1)
293-320: Ensure correct usage ofsetBudgetAverageFnin test casesWhen iterating over the array of
[numberOfMonths, setBudgetAverageFn], you're passingsetBudgetAverageFnto thesetBudgetAveragefunction:const averageSpent = await setBudgetAverage( budgetPage, categoryName, numberOfMonths, setBudgetAverageFn, );Given the earlier issue with naming conflicts in
setBudgetAverage, ensure thatsetBudgetAverageFnis correctly defined and passed after renaming the parameter in thesetBudgetAveragefunction.Double-check that
setBudgetAverageFncorresponds to the appropriate function (setTo3MonthAverage,setTo6MonthAverage,setToYearlyAverage) and that these functions are correctly refactored if you implement the earlier refactoring suggestion.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/e2e/budget.mobile.test.js (2)
33-59: LGTM:setBudgetAveragefunction implementationThe
setBudgetAveragefunction correctly calculates the average spending over a specified number of months. It properly navigates through previous months, retrieves spent amounts, and returns to the current month.One potential improvement to consider:
- The function modifies the current month during its execution. While it does return to the original month, this approach might lead to unexpected behavior if there's an error during execution. Consider using a separate function to retrieve historical data without changing the current month.
292-318: LGTM: New test cases for setting budget to average amountsThese new test cases effectively verify the functionality of setting budgets to 3, 6, and 12 month averages. The use of a loop to generate multiple similar test cases is efficient and reduces code duplication. Each test correctly uses the
setBudgetAveragefunction to calculate the expected average and verify the result.One point to consider:
- The use of
Math.abs()in the expectation suggests that negative averages are being handled. It might be worth adding a comment or a separate test case to explicitly cover the scenario of negative averages, ensuring that this behavior is intentional and correctly implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3583.mdis excluded by!**/*.md
📒 Files selected for processing (1)
- packages/desktop-client/e2e/budget.mobile.test.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/desktop-client/e2e/budget.mobile.test.js (3)
3-3: LGTM: New import statement for currency conversion utilitiesThe addition of
amountToCurrencyandcurrencyToAmountimports is appropriate for the new budget average calculation functionality.
260-270: LGTM: Updated test case for setting budgeted amountThe test case has been successfully updated to use the new
budgetMenuModalobject and itssetBudgetAmountmethod. This change aligns with the new budget menu modal functionality and maintains the correct verification of the budgeted amount update.
272-290: LGTM: New test case for copying last month's budgetThis new test case effectively verifies the 'copy last month's budget' functionality. It correctly:
- Navigates to the previous month
- Stores the previous month's budget
- Returns to the current month
- Applies the copy action
- Verifies that the current month's budget matches the previous month's budget
The use of the
copyLastMonthBudgetutility function is appropriate and aligns with the new functionality introduced earlier in the file.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.js (3)
2-9: Constructor looks good, consider adding input validation.The constructor is well-structured and uses roles for locators, which is a good practice. However, consider adding type checking or validation for the input parameters to improve robustness.
You could add simple checks like:
if (!page || !locator) { throw new Error('Page and locator are required parameters'); }
11-13:closemethod looks good, consider adding error handling.The
closemethod is concise and uses roles for element selection, which is good. However, consider adding error handling to make the method more robust.You could improve error handling like this:
async close() { try { await this.heading.getByRole('button', { name: 'Close' }).click(); } catch (error) { console.error('Failed to close the modal:', error); throw new Error('Failed to close the modal'); } }
15-18:updateNotesmethod is functional, consider adding input validation and error handling.The
updateNotesmethod performs the expected actions for updating notes. However, it could benefit from input validation and error handling to improve robustness.Consider improving the method like this:
async updateNotes(notes) { if (typeof notes !== 'string') { throw new TypeError('Notes must be a string'); } try { await this.textArea.fill(notes); await this.saveNotesButton.click(); } catch (error) { console.error('Failed to update notes:', error); throw new Error('Failed to update notes'); } }packages/desktop-client/e2e/page-models/mobile-category-menu-modal.js (2)
3-11: LGTM: Well-structured class with clear property definitions.The
CategoryMenuModalclass is well-designed with a clear constructor and descriptive property names. The use ofgetByRoleandgetByTestIddemonstrates good practices for accessibility and testability.Consider adding JSDoc comments to describe the purpose of the class and the parameters of the constructor. This would enhance code readability and maintainability.
17-21: LGTM: Well-implementededitNotesmethod with good separation of concerns.The
editNotesmethod is clear and follows good practices by creating a newEditNotesModalinstance for further interactions.Consider adding error handling to catch potential issues when clicking the button or creating the new modal. For example:
async editNotes() { try { await this.editNotesButton.click(); return new EditNotesModal(this.page, this.page.getByRole('dialog')); } catch (error) { console.error('Error opening edit notes modal:', error); throw error; } }packages/desktop-client/e2e/page-models/settings-page.js (1)
22-24: Approve changes with a minor suggestion for improvement.The added conditional checks for element visibility and state before interaction are excellent improvements. They enhance the robustness of the
enableExperimentalFeaturemethod by preventing potential errors when elements are not visible or already in the desired state.To further improve code readability and maintainability, consider extracting these checks into separate helper methods.
Here's a suggested refactoring:
async clickIfVisible(element) { if (await element.isVisible()) { await element.click(); } } async checkIfUnchecked(checkbox) { if (!(await checkbox.isChecked())) { await checkbox.click(); } } async enableExperimentalFeature(featureName) { await this.clickIfVisible(this.page.getByTestId('advanced-settings')); await this.clickIfVisible(this.page.getByTestId('experimental-settings')); await this.checkIfUnchecked(this.page.getByRole('checkbox', { name: featureName })); }This refactoring improves code reusability and makes the
enableExperimentalFeaturemethod more concise and easier to read.Also applies to: 29-31, 36-38
packages/desktop-client/e2e/budget.mobile.test.js (1)
9-31: LGTM with suggestion: Consider refactoring to reduce duplicationThe new utility functions (
copyLastMonthBudget,setTo3MonthAverage,setTo6MonthAverage, andsetToYearlyAverage) are correctly implemented and serve their intended purposes. However, as previously suggested, there's an opportunity to reduce code duplication by refactoring these functions into a single higher-order function.Consider implementing a higher-order function to handle these actions:
const performBudgetMenuAction = async (budgetPage, categoryName, action) => { const budgetMenuModal = await budgetPage.openBudgetMenu(categoryName); await action(budgetMenuModal); await budgetMenuModal.close(); }; const copyLastMonthBudget = (budgetPage, categoryName) => performBudgetMenuAction(budgetPage, categoryName, menu => menu.copyLastMonthBudget()); const setTo3MonthAverage = (budgetPage, categoryName) => performBudgetMenuAction(budgetPage, categoryName, menu => menu.setTo3MonthAverage()); // ... similar for setTo6MonthAverage and setToYearlyAverageThis refactoring will make the code more maintainable and easier to extend in the future.
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (1)
Line range hint
150-160: Include all dependencies inuseEffectdependency arrayThe
useEffecthook depends onscrollY,previousScrollY,hide, andopenDefault, but onlyscrollYis included in the dependency array. Omitting dependencies can lead to stale closures and unexpected behavior. Include all external variables used within theuseEffectto ensure it runs correctly when any of them change.Apply this diff to include missing dependencies:
useEffect(() => { if ( scrollY && previousScrollY && scrollY > previousScrollY && previousScrollY !== 0 ) { hide(); } else { openDefault(); } - }, [scrollY]); + }, [scrollY, previousScrollY, hide, openDefault]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (16)
packages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-4-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (7)
- packages/desktop-client/e2e/budget.mobile.test.js (2 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-page.js (3 hunks)
- packages/desktop-client/e2e/page-models/mobile-category-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-navigation.js (1 hunks)
- packages/desktop-client/e2e/page-models/settings-page.js (1 hunks)
- packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (7 hunks)
🧰 Additional context used
🔇 Additional comments (19)
packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.js (1)
1-19: LGTM: Well-structured class following Page Object Model pattern.The
EditNotesModalclass is well-structured and follows the Page Object Model pattern, which is a best practice for e2e testing. The class and method names are descriptive and follow appropriate naming conventions.packages/desktop-client/e2e/page-models/mobile-category-menu-modal.js (3)
1-1: LGTM: Import statement is appropriate.The import of
EditNotesModalis correctly used in theeditNotesmethod, maintaining good modularity.
13-15: LGTM: Concise and well-implementedclosemethod.The
closemethod is clear, concise, and follows good practices by using role-based element selection.
1-22: Overall, excellent implementation of theCategoryMenuModalclass.This new file introduces a well-structured and clearly implemented
CategoryMenuModalclass for e2e testing. The code demonstrates good practices in terms of:
- Modularity and encapsulation
- Accessibility considerations (use of
getByRole)- Testability (use of
getByTestId)- Clear and descriptive naming conventions
The suggestions for improvement are minor and include:
- Adding JSDoc comments for better documentation
- Implementing error handling in the
editNotesmethodGreat job on this implementation!
packages/desktop-client/e2e/page-models/mobile-budget-page.js (4)
2-3: LGTM: New imports added correctly.The new import statements for
BudgetMenuModalandCategoryMenuModalare correctly formatted and align with the existing code style. These imports support the changes made to theopenBudgetMenuandopenCategoryMenumethods.
Line range hint
1-300: Overall: Good improvements to modularity, verify impacts on calling code.The changes in this file improve modularity and separation of concerns by introducing
BudgetMenuModalandCategoryMenuModal. This is a positive step towards more maintainable code.However, since both
openBudgetMenuandopenCategoryMenunow return values instead of being void methods, it's crucial to ensure that all calling code is updated accordingly. This includes both the application code and the test suite.Please run the verification scripts provided in the previous comments to identify all occurrences where these methods are called, and update them as necessary to handle the returned modal instances.
184-184: LGTM: Improved modularity inopenBudgetMenu.The changes to the
openBudgetMenumethod improve modularity by returning aBudgetMenuModalinstance. This encapsulation of budget menu interactions is a good practice.As noted in a previous review comment, ensure that all existing tests using this method have been updated to work with the new return value (BudgetMenuModal instance instead of void).
To verify the impact of this change, please run the following script:
#!/bin/bash # Search for usages of openBudgetMenu in test files echo "Searching for openBudgetMenu usages in test files:" rg "openBudgetMenu" --type js --type ts -g "*test*" -g "*spec*"
144-145: LGTM: Improved modularity inopenCategoryMenu.The changes to the
openCategoryMenumethod improve modularity by returning aCategoryMenuModalinstance. This encapsulation of category menu interactions is a good practice.However, since the method now returns a value, ensure that any code calling this method is updated to handle the returned
CategoryMenuModalinstance correctly.To verify the impact of this change, please run the following script:
packages/desktop-client/e2e/budget.mobile.test.js (6)
3-3: LGTM: Import statement added correctlyThe new import statement for
amountToCurrencyandcurrencyToAmountfunctions from theloot-core/shared/utilmodule is appropriate and necessary for the new functionality introduced in this file.
33-59: LGTM: Issues resolved insetBudgetAveragefunctionThe
setBudgetAveragefunction has been implemented correctly, addressing the previously identified issues:
- The infinite recursion problem has been resolved by using the
setBudgetAverageFnparameter instead of calling the function recursively.- The spent amounts are now correctly retrieved after navigating to each month, ensuring accurate data collection.
The function effectively calculates the average spent amount over the specified number of months and returns to the current month after the calculation.
260-272: LGTM: Test case updated to use new modal interactionThe "updates the budgeted amount" test case has been successfully updated to use the new
BudgetMenuModalclass. The changes align with the new modal-based interaction model and correctly verify the expected behavior of setting and displaying the budgeted amount.
274-292: LGTM: New test case for copying last month's budgetThe new test case "copies last month's budget" effectively verifies the functionality of copying the budget from the previous month. It correctly uses the
copyLastMonthBudgetutility function and ensures that the budgeted amount matches the previous month's value after the operation.
294-320: LGTM: Efficient implementation of average budget testsThe new test cases for setting the budget to 3, 6, and 12-month averages are well-implemented. The use of a loop to generate tests for different month ranges is an efficient approach that reduces code duplication. Each test case correctly uses the
setBudgetAveragefunction along with the corresponding utility function to verify the expected behavior.
322-347: LGTM: Test case updated to use new modal interactionsThe "applies budget template" test case has been successfully updated to use the new
CategoryMenuModalandEditNotesModalclasses. The changes align with the new modal-based interaction model and correctly verify the expected behavior of applying a budget template. The test effectively covers the process of setting up a template and verifying its application.packages/desktop-client/e2e/page-models/mobile-navigation.js (1)
104-106: Verify the inclusion of 'default' state in navbar state checkIn
goToSettingsPage(), the navbar state is checked for both'default'and'hidden'before callingdragNavbarUp(). In other navigation methods, only'hidden'is checked. Please verify if including'default'is necessary for this method.packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (4)
1-6: Imports are correctly structuredThe imported modules and hooks, including the
type ComponentType, are properly organized.
217-217: EnsurenavbarStateis defined before usageWhen adding
data-navbar-state={navbarState}, make sure thatnavbarStateis notundefined. SincenavbarStatemay initially beundefined, this could lead to an attribute with an undefined value, which might cause unexpected behavior.Ensure that
navbarStateis initialized properly. Refer to the previous comment about initializingnavbarState.
181-183: Verify the logic in the drag handlerIn the
useDraghook, the logic determining whether to callopenDefaultoropenFullafter a drag action depends on the vertical offset and velocity. Verify that the thresholds and conditions correctly reflect the desired user experience, ensuring smooth and intuitive interactions.Consider testing different drag scenarios to confirm that the navbar opens and closes as expected.
270-270:userSelect: 'none'is correctly appliedThe addition of
userSelect: 'none'prevents text selection on the navigation tabs, enhancing the user experience during interactions.
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/e2e/budget.mobile.test.js (4)
33-59: LGTM: setBudgetAverage function implementationThe
setBudgetAveragefunction correctly calculates the average spending over a specified number of months. The issues mentioned in past review comments have been addressed:
- The potential infinite recursion has been resolved by correctly calling
setBudgetAverageFninstead of recursively callingsetBudgetAverage.- The retrieval of spent amounts is now correctly placed inside the loop, ensuring accurate data for each month.
To further improve readability, consider extracting the month navigation logic into separate helper functions:
const navigateToPreviousMonths = async (budgetPage, numberOfMonths) => { for (let i = 0; i < numberOfMonths; i++) { await budgetPage.goToPreviousMonth(); } }; const navigateToCurrentMonth = async (budgetPage, numberOfMonths) => { for (let i = 0; i < numberOfMonths; i++) { await budgetPage.goToNextMonth(); } };Then use these helper functions in
setBudgetAverage:async function setBudgetAverage( budgetPage, categoryName, numberOfMonths, setBudgetAverageFn, ) { await navigateToPreviousMonths(budgetPage, numberOfMonths); // ... (calculate average spent) await navigateToCurrentMonth(budgetPage, numberOfMonths); await setBudgetAverageFn(budgetPage, categoryName, numberOfMonths); return averageSpent; }This change would make the main function more concise and easier to understand.
266-284: LGTM: New test for copying last month's budgetThis new test effectively verifies the functionality of copying the previous month's budget using the
copyLastMonthBudgetutility function. It correctly navigates between months and checks that the budget amount is copied accurately.To improve the test's robustness, consider adding an initial step to set a specific budget amount in the previous month before copying. This would ensure that the test is not dependent on pre-existing data:
// Set a specific budget amount in the previous month await budgetPage.goToPreviousMonth(); const budgetMenuModal = await budgetPage.openBudgetMenu(categoryName); await budgetMenuModal.setBudgetAmount('15000'); // Set to 150.00 await budgetMenuModal.close(); // Rest of the test remains the sameThis addition would make the test more deterministic and less likely to fail due to changes in test data.
286-312: LGTM: New tests for setting budget to month averagesThese new tests effectively verify the functionality of setting the budget to 3, 6, and 12 month averages using the
setBudgetAverageutility function. The use of a loop to generate tests for different month ranges is an efficient approach.To improve the tests' clarity, consider adding a descriptive comment before the loop and using a more descriptive name for the test cases:
// Test budget averaging for 3, 6, and 12 month periods [ [3, setTo3MonthAverage, 'quarterly'], [6, setTo6MonthAverage, 'semi-annual'], [12, setToYearlyAverage, 'annual'], ].forEach(([numberOfMonths, setBudgetAverageFn, periodName]) => { test(`set budget to ${periodName} average (${numberOfMonths} months)`, async () => { // ... (rest of the test implementation) }); });This change would make the purpose of each test case more immediately clear to readers.
314-339: LGTM: New test for applying budget templateThis new test effectively verifies the functionality of applying a budget template. It correctly sets up the environment by enabling the experimental feature, creates a template, and verifies its application.
To improve the test's clarity and maintainability, consider extracting the template setup into a separate helper function:
const setupBudgetTemplate = async (budgetPage, categoryName, amount) => { const categoryMenuModal = await budgetPage.openCategoryMenu(categoryName); const editNotesModal = await categoryMenuModal.editNotes(); await editNotesModal.updateNotes(`#template ${amount}`); await editNotesModal.close(); }; test(`applies budget template`, async () => { const settingsPage = await navigation.goToSettingsPage(); await settingsPage.enableExperimentalFeature('Goal templates'); const budgetPage = await navigation.goToBudgetPage(); await budgetPage.waitForBudgetTable(); const categoryName = await budgetPage.getCategoryNameForRow(1); const amountToTemplate = 123; await setupBudgetTemplate(budgetPage, categoryName, amountToTemplate); // ... (rest of the test implementation) });This refactoring would make the test more readable and easier to maintain, especially if you need to set up templates in other tests in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/e2e/budget.mobile.test.js (4 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/desktop-client/e2e/budget.mobile.test.js (3)
3-3: LGTM: New import for currency conversion utilitiesThe addition of
amountToCurrencyandcurrencyToAmountimports is appropriate for the new currency conversion operations in the file.
252-263: LGTM: Updated test for budget amount changesThe test has been successfully updated to use the new
budgetMenuModalapproach for setting the budget amount. This change aligns with the overall shift towards using modals for budget operations, maintaining consistency across the codebase.
Line range hint
1-424: Overall assessment: Good additions with room for improvementThe changes in this file significantly enhance the testing coverage for budget-related features in the mobile application. The new utility functions and tests are well-implemented and cover important use cases. However, there are opportunities for improvement:
- Refactor the budget utility functions to reduce code duplication.
- Improve the readability of the
setBudgetAveragefunction by extracting helper methods.- Enhance the robustness and clarity of the new tests by adding setup steps and using more descriptive names.
These suggested improvements will make the code more maintainable and the tests more reliable. Overall, good job on expanding the test suite!
joel-jeremy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.js (3)
2-20: LGTM: Well-structured constructor with comprehensive UI element locators.The constructor is well-designed, initializing necessary instance variables and UI element locators. The use of
getByRoleandgetByTestIdsuggests good accessibility practices, which is crucial for robust end-to-end tests.Consider adding JSDoc comments to describe the
pageandlocatorparameters for better code documentation. For example:/** * @param {Page} page - The Playwright Page object * @param {Locator} locator - The base locator for this modal */ constructor(page, locator) { // ... existing code ... }
6-19: LGTM: Comprehensive and well-defined UI element properties.The class properties cover all necessary UI elements for interacting with the modal. The consistent use of semantic roles and test IDs follows best practices for robust and maintainable end-to-end tests.
Consider grouping related buttons into an object for better organization and easier maintenance. For example:
this.buttons = { transferToAnotherCategory: locator.getByRole('button', { name: 'Transfer to another category' }), coverOverspending: locator.getByRole('button', { name: 'Cover overspending' }), rolloverOverspending: locator.getByRole('button', { name: 'Rollover overspending' }), removeOverspendingRollover: locator.getByRole('button', { name: 'Remove overspending rollover' }) };This structure could make it easier to add or modify buttons in the future and provide a cleaner API for using these buttons in tests.
22-24: LGTM: Simple and focusedclosemethod.The
closemethod is well-implemented, usingasync/awaitcorrectly for the asynchronous click operation. Its single responsibility is clear and appropriate.To improve robustness, consider adding error handling and a fallback mechanism. For example:
async close() { try { await this.heading.getByRole('button', { name: 'Close' }).click(); } catch (error) { console.warn('Failed to find Close button in heading, trying modal-wide search'); await this.locator.getByRole('button', { name: 'Close' }).click(); } }This approach would first try to find the Close button within the heading, and if that fails, it would search for it within the entire modal. This could help prevent test failures if the button's location changes in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/desktop-client/e2e/budget.mobile.test.js (5 hunks)
- packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-page.js (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/e2e/budget.mobile.test.js
🧰 Additional context used
🔇 Additional comments (5)
packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.js (1)
1-1: LGTM: Class structure and export.The
BalanceMenuModalclass is well-named and appropriately exported as a named export. This follows good practices for modular code organization in JavaScript/TypeScript projects.packages/desktop-client/e2e/page-models/mobile-budget-page.js (4)
2-4: LGTM: New modal imports added.The new imports for
BalanceMenuModal,BudgetMenuModal, andCategoryMenuModalare correctly placed and follow the existing naming convention. These additions suggest a modular approach to handling different budget-related actions.
145-146: LGTM: NewopenCategoryMenumethod added.The new
openCategoryMenumethod is well-implemented. It follows the existing pattern of other methods in the class, uses the private#getButtonForCategorymethod to locate the button, and returns a newCategoryMenuModalinstance. This addition enhances the functionality of theMobileBudgetPageclass.
202-202: LGTM:openBalanceMenuupdated to useBalanceMenuModal.The modification to return a new
BalanceMenuModalinstance is consistent with the modal-based approach seen in other methods. The existing error handling for when the balance button is not visible has been retained, which is good for maintaining robustness.
Line range hint
1-324: Summary: Consistent implementation of modal-based approach for budget actions.The changes in this file consistently implement a modal-based approach for various budget-related actions, improving modularity and consistency. New modal classes (
BalanceMenuModal,BudgetMenuModal, andCategoryMenuModal) have been introduced and integrated into the existing methods.Key points:
- New imports added for modal classes.
openCategoryMenumethod added, following the existing pattern.openBudgetMenuandopenBalanceMenumethods updated to return modal instances.setBudgetmethod removed (as per AI summary, not visible in the diff).These changes align well with the PR objectives of enhancing the mobile testing framework for budget management. However, the removal of the
setBudgetmethod may have implications for other parts of the codebase or existing tests.To ensure a smooth transition, please:
- Verify that all tests using
MobileBudgetPagehave been updated to work with the new modal-based approach.- Check for any remaining references to the removed
setBudgetmethod in other parts of the codebase.- Update the documentation for
MobileBudgetPageto reflect these changes, especially noting the new return types for methods likeopenBudgetMenuandopenBalanceMenu.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.js (2)
1-7: LGTM! Consider adding JSDoc comments.The class declaration and constructor are well-structured and follow good practices. The use of role-based selectors (
getByRole('heading')) is excellent for maintaining test stability.Consider adding JSDoc comments to describe the purpose of the class and the expected types of the constructor parameters. This would improve code documentation and maintainability.
1-12: Great implementation! Consider expanding functionality.The
EnvelopeBudgetSummaryModalclass is well-structured, focused, and follows good testing practices. It provides a solid foundation for interacting with the envelope budget summary modal in your e2e tests.As the testing suite evolves, consider expanding this class to include additional methods for interacting with other elements within the modal, such as retrieving budget information or interacting with form elements if present. This would further enhance the reusability and maintainability of your test code.
packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.js (2)
2-7: LGTM: Constructor implementation with a suggestionThe constructor is well-implemented, properly initializing instance variables with the provided
pageandlocatorparameters. Theheadingproperty initialization is a good optimization for frequently accessed elements.Consider adding JSDoc comments to describe the parameters and their types for better code documentation:
/** * @param {import('@playwright/test').Page} page - The Playwright Page object * @param {import('@playwright/test').Locator} locator - The Locator for this modal */ constructor(page, locator) { // ... existing code ... }
9-11: LGTM: Well-implemented close method with a suggestionThe
closemethod is well-implemented, using asynchronous operations and chained locators to find and click the close button. The method name clearly describes its action.Consider adding error handling to make the method more robust:
async close() { try { await this.heading.getByRole('button', { name: 'Close' }).click(); } catch (error) { console.error('Failed to close the TrackingBudgetSummaryModal:', error); throw error; // Re-throw the error for the test to catch } }This will provide more informative error messages if the close action fails during testing.
packages/desktop-client/e2e/page-models/mobile-budget-page.js (2)
276-283: LGTM:openEnvelopeBudgetSummaryupdated to use modal approach.The renaming and modification to return an
EnvelopeBudgetSummaryModalinstance align well with the overall refactoring. This change is consistent with the modal-based approach implemented in other methods.Consider using object property shorthand for improved readability:
return new EnvelopeBudgetSummaryModal( - this.page, + this.page, this.page.getByRole('dialog'), );
308-315: LGTM:openTrackingBudgetSummaryupdated to use modal approach.The renaming and modification to return a
TrackingBudgetSummaryModalinstance are consistent with the overall refactoring and the modal-based approach implemented in other methods.Consider using object property shorthand for improved readability:
return new TrackingBudgetSummaryModal( - this.page, + this.page, this.page.getByRole('dialog'), );packages/desktop-client/e2e/budget.mobile.test.js (2)
286-312: Efficient test cases for setting budget to averageThe approach of using a loop to test multiple average periods (3, 6, and 12 months) is efficient and reduces code duplication. These tests effectively cover the functionality of setting budgets to different average periods.
To improve clarity, consider adding a comment explaining the structure of the test data:
// Test data structure: [number of months, corresponding setBudgetAverage function] [ [3, setTo3MonthAverage], [6, setTo6MonthAverage], [12, setToYearlyAverage], ].forEach(([numberOfMonths, setBudgetAverageFn]) => { // ... (rest of the code remains the same) });This addition would make the purpose of each array element more immediately clear to other developers.
314-340: Comprehensive test for applying budget templateThis test case effectively covers the budget template feature:
- It enables the experimental 'Goal templates' feature.
- It creates a template by updating category notes.
- It applies the template and verifies the budgeted amount.
To improve robustness, consider adding a cleanup step to disable the experimental feature after the test:
test.afterEach(async () => { const settingsPage = await navigation.goToSettingsPage(); await settingsPage.disableExperimentalFeature('Goal templates'); });This ensures that the experimental feature doesn't affect other tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/desktop-client/e2e/budget.mobile.test.js (7 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-page.js (6 hunks)
- packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.js (1)
9-11: LGTM! Well-implemented close method.The
closemethod is concise, uses async/await correctly, and employs accessibility-friendly selectors. The implementation is robust and should reliably close the modal.packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.js (1)
1-12: LGTM: Well-structured class for e2e testingThe
TrackingBudgetSummaryModalclass is well-structured and follows the Page Object Model pattern, which is excellent for maintainability and readability in e2e tests. The class name clearly indicates its purpose, and the exported structure allows for easy reuse in other test files.packages/desktop-client/e2e/page-models/mobile-budget-page.js (4)
2-6: LGTM: New modal imports align with updated class methods.The addition of these import statements is consistent with the changes made in the class methods, indicating a shift towards modal-based interactions for budget-related actions.
147-148: LGTM: NewopenCategoryMenumethod aligns with modal-based approach.The new method correctly clicks on the category button before returning a
CategoryMenuModalinstance, which is consistent with the shift towards modal-based interactions for budget management.
204-204: LGTM:openBalanceMenuupdated to useBalanceMenuModal.The modification to return a new
BalanceMenuModalinstance is consistent with the modal-based approach implemented in other methods. This change aligns well with the overall refactoring of the class.
Line range hint
1-316: Overall: Consistent implementation of modal-based approach for budget interactions.The changes in this file demonstrate a systematic shift towards using modal-based interactions for budget-related actions. This refactoring improves the overall structure and consistency of the
MobileBudgetPageclass. The removal of thesetBudgetmethod and the introduction of various modal classes suggest a significant change in how budget operations are handled.While these changes enhance the modularity of the code, they may require updates in other parts of the codebase that previously relied on the removed
setBudgetmethod. Ensure that all affected components are updated to use the new modal-based approach consistently.To ensure a smooth transition, please run comprehensive tests covering all budget-related functionalities, paying special attention to areas that previously used the
setBudgetmethod.packages/desktop-client/e2e/budget.mobile.test.js (4)
33-59: Improvements implemented insetBudgetAveragefunctionGreat job addressing the issues mentioned in the past review comments:
- The infinite recursion issue has been resolved by renaming the parameter to
setBudgetAverageFn.- The
spentButtonis now correctly retrieved inside the loop, ensuring accurate data for each month.These changes improve the function's reliability and correctness.
266-284: Well-structured test case for copying last month's budgetThis test case effectively verifies the
copyLastMonthBudgetfunctionality:
- It navigates to the previous month to get the last month's budget.
- It returns to the current month and applies the
copyLastMonthBudgetfunction.- It verifies that the current month's budget matches the previous month's budget.
The test is comprehensive and well-structured.
Line range hint
371-397: Effective tests for budget summary modalsThese test cases comprehensively cover the budget summary functionality:
- They test both Envelope and Tracking budget types separately.
- They verify that clicking the appropriate elements opens the correct budget summary modal.
- They check that the modal headings are correct.
The conditional tests based on budget type ensure that the appropriate functionality is tested for each type. This approach is well-structured and thorough.
Line range hint
1-397: Overall assessment of budget.mobile.test.js changesThe changes in this file significantly enhance the budget management functionality and its testing:
- New utility functions have been added to support various budget operations.
- Comprehensive test cases cover new features like copying last month's budget, setting budget averages, and applying budget templates.
- Previous issues identified in past reviews have been addressed, improving code reliability.
- The tests are well-structured and cover both Envelope and Tracking budget types.
While there are some opportunities for minor improvements (as noted in previous comments), the overall quality of the changes is high. The new functionality is well-tested and should provide a solid foundation for mobile budget management features.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
packages/desktop-client/e2e/page-models/mobile-reports-page.js (2)
2-6: Constructor looks good, consider adding error handling.The constructor correctly initializes the necessary properties and uses a test ID selector for the overview element, which is a good practice for test stability.
Consider adding error handling in case the overview element is not found. Here's a suggested improvement:
constructor(page) { this.page = page; - this.overview = page.getByTestId('reports-overview'); + this.overview = page.getByTestId('reports-overview'); + if (!this.overview) { + throw new Error("Failed to locate the reports overview element"); + } }This change will make the test fail early if the crucial overview element is missing, making debugging easier.
8-10:waitFormethod is good, but could use documentation and error handling.The
waitFormethod provides a useful way to synchronize tests with the page state. The use of async/await is correct.Consider the following improvements:
- Add JSDoc comments to explain the purpose of the method and the
optionsparameter.- Implement error handling.
Here's a suggested improvement:
+/** + * Waits for the reports overview element to be ready. + * @param {Object} options - Options to pass to the underlying Playwright `waitFor` method. + * @throws {Error} If waiting for the overview element times out. + */ async waitFor(options) { - await this.overview.waitFor(options); + try { + await this.overview.waitFor(options); + } catch (error) { + throw new Error(`Timed out waiting for the reports overview: ${error.message}`); + } }These changes will improve the method's documentation and make it more robust by providing meaningful error messages.
packages/desktop-client/e2e/page-models/mobile-accounts-page.js (2)
11-13: LGTM! Good addition for handling asynchronous UI updates.The
waitFormethod is a valuable addition that will help improve the reliability of tests by ensuring the account list is ready before interacting with it.Consider adding a default timeout to the
optionsparameter to make the method more robust:- async waitFor(options) { + async waitFor(options = { timeout: 5000 }) { await this.accountList.waitFor(options); }
31-31: LGTM! Consistent update to use the new element selection strategy.The change to use
this.accountListItemsis consistent with the updates made elsewhere in the class.Consider adding error handling to improve robustness:
async openNthAccount(idx) { - await this.accountListItems.nth(idx).click(); + const accountItem = this.accountListItems.nth(idx); + if (await accountItem.count() === 0) { + throw new Error(`Account at index ${idx} not found`); + } + await accountItem.click(); return new MobileAccountPage(this.page); }packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.js (2)
15-16: Good addition of waitFor method to improve test reliability.The new
waitFormethod enhances the reliability of tests by ensuring the transaction form is ready before interactions. This is a good practice in E2E testing.Consider adding a default timeout to the
waitFormethod for consistency across tests:async waitFor(options = { timeout: 5000 }) { await this.transactionForm.waitFor(options); }
26-26: Consistent update to createTransaction method.The change to use
addTransactionButtonaligns well with the restructured element access introduced in the constructor.Consider adding error handling to improve robustness:
async createTransaction() { try { await this.addTransactionButton.click(); return new MobileAccountPage(this.page); } catch (error) { console.error('Failed to create transaction:', error); throw error; } }packages/desktop-client/src/components/settings/index.tsx (3)
150-150: LGTM! Consider extending test coverage.The addition of the
data-testid="settings"attribute to theViewcomponent is a good practice. It enhances the testability of the Settings component by providing a reliable selector for automated tests.To further improve test coverage, consider adding more specific
data-testidattributes to child components within the Settings page. This would allow for more granular testing of individual settings sections.
Line range hint
76-83: Consider addressing commented-out code.There's a section of commented-out code related to displaying additional IDs. It's generally a good practice to remove commented-out code or add a clear TODO comment explaining why it's kept.
If these IDs are no longer needed, consider removing the commented code. If they might be used in the future, add a clear TODO comment explaining the plan for this code.
Line range hint
93-93: Consider using React.memo for performance optimization.The
Settingscomponent renders multiple child components and uses several hooks. To potentially improve performance, especially if this component re-renders frequently, consider wrapping it withReact.memo.Here's how you could apply
React.memo:export const Settings = React.memo(function Settings() { // ... existing component code ... });This optimization would prevent unnecessary re-renders if the component's props (in this case, there are none) haven't changed.
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1)
187-187: Approve the addition ofdata-testidand suggest test enhancements.The addition of
data-testid="account-list"to theViewcomponent enhances the testability of the account list container. This change allows for more precise targeting in tests, potentially improving test reliability and readability.Consider updating or adding tests that utilize this new
data-testidto improve test coverage and specificity. For example, you could add tests to verify the structure and content of the account list container.packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (1)
Line range hint
1062-1079: Good addition: New TransactionEdit component with hooks.The new TransactionEdit component is a good addition that separates concerns and uses React hooks for data fetching. This improves code organization and reusability.
Consider adding a prop-types definition for this component to improve type checking and documentation.
You could add prop-types like this:
import PropTypes from 'prop-types'; // ... component code ... TransactionEdit.propTypes = { // Add any props that might be passed to this component };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- packages/desktop-client/e2e/budget.mobile.test.js (4 hunks)
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js (2 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-page.js (7 hunks)
- packages/desktop-client/e2e/page-models/mobile-navigation.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-reports-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.js (2 hunks)
- packages/desktop-client/e2e/page-models/settings-page.js (1 hunks)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (2 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (2 hunks)
- packages/desktop-client/src/components/reports/Overview.tsx (1 hunks)
- packages/desktop-client/src/components/settings/index.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/components/reports/Overview.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/e2e/budget.mobile.test.js
- packages/desktop-client/e2e/page-models/settings-page.js
🧰 Additional context used
🔇 Additional comments (20)
packages/desktop-client/e2e/page-models/mobile-reports-page.js (1)
1-11: LGTM: Well-structured Page Object Model class.The
MobileReportsPageclass follows the Page Object Model pattern, which is a best practice for organizing end-to-end tests. The class name accurately describes its purpose, and the structure with a constructor and awaitFormethod provides a good foundation for interacting with the mobile reports page in tests.packages/desktop-client/e2e/page-models/mobile-accounts-page.js (3)
7-8: LGTM! Improved element selection strategy.The new
accountListandaccountListItemsproperties provide a more structured and flexible approach to selecting account elements. Using test IDs for element selection is a good practice in E2E testing, as it makes the tests more resilient to UI changes.
19-19: LGTM! Consistent use of the new element selection strategy.The update to use
this.accountListItemsaligns well with the changes made in the constructor, maintaining consistency throughout the class.
Line range hint
1-35: Overall, great improvements to theMobileAccountsPageclass!The changes made to this class enhance its flexibility and maintainability:
- The new element selection strategy using test IDs improves test resilience.
- The addition of the
waitFormethod helps handle asynchronous UI updates.- Consistent updates across methods maintain the class's coherence.
These improvements will likely lead to more robust and reliable E2E tests for the mobile accounts functionality.
packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.js (2)
7-12: Improved structure and testability of the MobileTransactionEntryPage class.The new properties (
transactionForm,amountField,footer, andaddTransactionButton) enhance the organization of the class and improve its testability. The use ofgetByTestIdandgetByRolemethods aligns with best practices for robust test selectors.
Line range hint
1-29: Overall improvements to MobileTransactionEntryPage class structure and testability.The changes in this file significantly enhance the
MobileTransactionEntryPageclass by:
- Restructuring element access for better organization.
- Improving testability with the use of test-id and role-based selectors.
- Adding a
waitFormethod to increase test reliability.- Updating the
createTransactionmethod for consistency.These modifications align well with best practices in E2E testing and should lead to more robust and maintainable tests.
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1)
74-74: Approve thedata-testidchange and verify existing tests.The change from
data-testid="account"todata-testid="account-list-item"improves the specificity and clarity of the test identifier. This modification better represents the component's role as an item within the account list.Please ensure that all existing tests referencing this component are updated to use the new
data-testidvalue. Run the following script to check for any occurrences of the olddata-testid:✅ Verification successful
Verification successful: No tests reference the old
data-testid="account"value.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for occurrences of the old data-testid in test files # Test: Look for "data-testid="account"" in test files rg --type test 'data-testid="account"'Length of output: 69
Script:
#!/bin/bash # Description: Search for occurrences of the old data-testid in test files with common test file extensions # Search in files ending with .test.js, .spec.js, .test.jsx, or .spec.jsx rg 'data-testid="account"' --glob '*.test.js' --glob '*.spec.js' --glob '*.test.jsx' --glob '*.spec.jsx'Length of output: 106
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (3)
191-191: Minor enhancement: Added data-testid attribute.A
data-testidattribute has been added to the footer view. This is a good practice for improving testability of the component.
745-748: Improved testability: Added data-testid to transaction form.The addition of a
data-testidattribute to the main transaction form view enhances the component's testability. This is consistent with best practices for creating easily identifiable elements in automated tests.
Line range hint
1-1079: Overall: Improved testability and code organization.The changes in this file focus on two main areas:
- Adding
data-testidattributes to key elements, enhancing the testability of the components.- Introducing a new
TransactionEditcomponent that uses React hooks for data fetching and wraps the existingTransactionEditUnconnectedcomponent.These changes align with best practices in React development, improving both the testability and organization of the code. The additions are consistent and do not introduce any apparent issues or significant logic changes.
packages/desktop-client/e2e/page-models/mobile-navigation.js (3)
14-14: Consider declaringROW_HEIGHTas a static class property
ROW_HEIGHTis currently defined as an instance property. IfROW_HEIGHTis intended to be a constant value shared across all instances ofMobileNavigation, consider making it a static class property using thestatickeyword.
44-45: Usethis.navbar.getByRolefor link selection ingoToBudgetPageIn
goToBudgetPage(), you're usingthis.page.getByRoleto select the 'Budget' link. To maintain consistency with other navigation methods, consider usingthis.navbar.getByRoleinstead.
69-70: Usethis.navbar.getByRolefor link selection ingoToAccountsPageIn
goToAccountsPage(), you're usingthis.page.getByRoleto select the 'Accounts' link. It would be more consistent to usethis.navbar.getByRole, as done in other methods.packages/desktop-client/e2e/page-models/mobile-budget-page.js (7)
2-6: LGTM: New modal classes imported correctlyThe imports for the new modal classes are appropriately added and correspond with their usage within the file.
86-87: Verify updates to method calls for 'waitFor'The method
waitForBudgetTable()has been renamed towaitFor(options). Please ensure that all references towaitForBudgetTable()have been updated to use the new method signature to prevent any broken references.Run the following script to find any lingering references to the old method:
#!/bin/bash # Description: Search for references to 'waitForBudgetTable' # Expected Result: No occurrences of 'waitForBudgetTable' should be found rg "waitForBudgetTable" --type js --type ts
204-204: Ensure callers of 'openBalanceMenu' handle the new return valueThe method
openBalanceMenunow returns a new instance ofBalanceMenuModal. Please verify that all callers of this method are updated to handle the returned modal appropriately.Run the following script to find usages of
openBalanceMenuand check for handling of the return value:#!/bin/bash # Description: Find usages of 'openBalanceMenu' and review usage # Expected Result: Callers should handle the returned 'BalanceMenuModal' instance rg "openBalanceMenu\(" --type js --type ts -A 2
276-283: Ensure callers of 'openEnvelopeBudgetSummary' handle the new return valueThe method
openEnvelopeBudgetSummarynow returns a new instance ofEnvelopeBudgetSummaryModal. Please verify that all callers of this method are updated to handle the returned modal appropriately.Run the following script to find usages of
openEnvelopeBudgetSummaryand check for handling of the return value:#!/bin/bash # Description: Find usages of 'openEnvelopeBudgetSummary' and review usage # Expected Result: Callers should handle the returned 'EnvelopeBudgetSummaryModal' instance rg "openEnvelopeBudgetSummary\(" --type js --type ts -A 2Additionally, ensure all references to the old method name
openEnvelopeBudgetSummaryMenuhave been updated:#!/bin/bash # Description: Search for references to 'openEnvelopeBudgetSummaryMenu' # Expected Result: No occurrences of 'openEnvelopeBudgetSummaryMenu' should be found rg "openEnvelopeBudgetSummaryMenu\(" --type js --type ts
147-148: Ensure callers of 'openCategoryMenu' handle the new return valueThe method
openCategoryMenunow returns a new instance ofCategoryMenuModal. Please verify that all callers of this method are updated to handle the returned modal appropriately.Run the following script to find usages of
openCategoryMenuand check for handling of the return value:#!/bin/bash # Description: Find usages of 'openCategoryMenu' and review usage # Expected Result: Callers should handle the returned 'CategoryMenuModal' instance rg "openCategoryMenu\(" --type js --type ts -A 2
308-315: Ensure callers of 'openTrackingBudgetSummary' handle the new return valueThe method
openTrackingBudgetSummarynow returns a new instance ofTrackingBudgetSummaryModal. Please verify that all callers of this method are updated to handle the returned modal appropriately.Run the following script to find usages of
openTrackingBudgetSummaryand check for handling of the return value:#!/bin/bash # Description: Find usages of 'openTrackingBudgetSummary' and review usage # Expected Result: Callers should handle the returned 'TrackingBudgetSummaryModal' instance rg "openTrackingBudgetSummary\(" --type js --type ts -A 2Also, ensure all references to the old method name
openTrackingBudgetSummaryMenuhave been updated:#!/bin/bash # Description: Search for references to 'openTrackingBudgetSummaryMenu' # Expected Result: No occurrences of 'openTrackingBudgetSummaryMenu' should be found rg "openTrackingBudgetSummaryMenu\(" --type js --type ts
187-187: Ensure callers of 'openBudgetMenu' handle the new return valueThe method
openBudgetMenunow returns a new instance ofBudgetMenuModal. Please verify that all callers of this method are updated to handle the returned modal appropriately.Run the following script to find usages of
openBudgetMenuand check for handling of the return value:✅ Verification successful
No callers of
openBudgetMenufound. No action required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of 'openBudgetMenu' and review usage # Expected Result: Callers should handle the returned 'BudgetMenuModal' instance rg "openBudgetMenu\(" --type js --type ts -A 2Length of output: 682
Script:
#!/bin/bash # Description: Find all calls to 'openBudgetMenu' and check if the return value is handled # Expected Result: All calls to 'openBudgetMenu' should handle the returned 'BudgetMenuModal' instance ast-grep --lang javascript --pattern "call_expression_expression: call_expression: identifier: \"openBudgetMenu\"" --actions 'print @file ":" @line ":" @column " - " @text'Length of output: 375
Script:
#!/bin/bash # Description: Find all calls to 'openBudgetMenu' and check if the return value is handled # Expected Result: All calls to 'openBudgetMenu' should handle the returned 'BudgetMenuModal' instance ast-grep --lang javascript --pattern 'call_expression: identifier: "openBudgetMenu"'Length of output: 86
Script:
#!/bin/bash # Description: Find all calls to 'openBudgetMenu' and check if the return value is handled # Expected Result: All calls to 'openBudgetMenu' should handle the returned 'BudgetMenuModal' instance rg -n -A2 "(const|let|let|await)\s+\w+\s*=\s*await\s+openBudgetMenu\(" --type js --type tsLength of output: 92
joel-jeremy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/e2e/budget.mobile.test.js (3)
273-298: LGTM: Test cases for setting budget to average are well-implemented.The parameterized approach for testing 3, 6, and 12-month averages is efficient and reduces code duplication.
Consider adding a brief comment explaining the structure of these tests for better readability:
// Test cases for setting budget to 3, 6, and 12-month averages [ [3, setTo3MonthAverage], [6, setTo6MonthAverage], [12, setToYearlyAverage], ].forEach(([numberOfMonths, setBudgetAverageFn]) => { // ... (rest of the code) });
300-328: LGTM: Test case for applying budget template is well-implemented with improved accessibility.The test case thoroughly verifies the budget template functionality, including setup and application.
Great job on improving accessibility by adding the
role="alert"attribute to the notification element. This enhances the user experience for screen reader users.
218-220: LGTM: Existing test cases updated consistently with new modal objects.The updates to existing test cases improve code consistency and align with the new utility functions and modal implementations.
For better consistency, consider using the same naming convention for all modal variables. For example:
const categoryMenuModal = await budgetPage.openCategoryMenu(categoryName); const budgetMenuModal = await budgetPage.openBudgetMenu(categoryName); const balanceMenuModal = await budgetPage.openBalanceMenu(categoryName); const budgetSummaryModal = await budgetPage.openEnvelopeBudgetSummary(); // or openTrackingBudgetSummary()Also applies to: 230-232, 240-252, 350-352, 360-365, 374-379
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/desktop-client/e2e/budget.mobile.test.js (4 hunks)
- packages/desktop-client/e2e/page-models/mobile-navigation.js (1 hunks)
- packages/desktop-client/e2e/page-models/settings-page.js (1 hunks)
- packages/desktop-client/src/components/Notifications.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/components/Notifications.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/e2e/page-models/settings-page.js
🧰 Additional context used
🔇 Additional comments (16)
packages/desktop-client/e2e/page-models/mobile-navigation.js (10)
3-3: LGTM: Import statement for MobileReportsPageThe import statement for
MobileReportsPageis consistent with the naming convention of other imported classes and aligns with the newgoToReportsPagemethod added to theMobileNavigationclass.
10-11: LGTM: New properties for improved element selectionThe addition of
headingandnavbarproperties usinggetByRoleis a good practice for accessibility and provides a centralized way to reference these elements throughout the class.
16-18: LGTM: dragNavbarUp method implementationThe
dragNavbarUpmethod provides a clear and concise way to drag the navbar to the heading position. The use of thedragToAPI is appropriate for this interaction.
27-29: LGTM: hasNavbarState method implementationThe
hasNavbarStatemethod is well-implemented. It efficiently checks the navbar's state against multiple possible values using the spread operator andincludesmethod. This approach provides flexibility and readability.
33-54: LGTM: Improved goToBudgetPage methodThe updates to
goToBudgetPagemethod enhance its functionality by:
- Handling different navbar states.
- Ensuring the navbar is visible before navigation.
- Returning the
budgetPageinstance consistently.These changes improve the robustness and reliability of the navigation process.
58-79: LGTM: Consistent improvements in goToAccountsPage methodThe
goToAccountsPagemethod has been updated consistently with thegoToBudgetPagemethod. It now includes:
- Proper handling of navbar states.
- Ensuring navbar visibility before navigation.
- Returning the
accountsPageinstance.These changes maintain consistency across navigation methods and improve the overall reliability of the navigation process.
83-100: LGTM: Enhanced goToTransactionEntryPage methodThe
goToTransactionEntryPagemethod has been improved consistently with other navigation methods. Notable enhancements include:
- Proper handling of navbar states.
- Use of
this.navbarfor link selection, which is more specific and consistent with the class structure.- Returning the
transactionEntryPageinstance.These changes contribute to a more robust and consistent navigation implementation across the class.
103-124: LGTM: Well-implemented goToReportsPage methodThe new
goToReportsPagemethod is a welcome addition to theMobileNavigationclass. It follows the same pattern as other navigation methods, ensuring:
- Proper handling of navbar states.
- Consistent navigation process.
- Return of the
reportsPageinstance.This implementation maintains the consistency and reliability of the navigation system.
127-148: LGTM: Consistent implementation of goToSettingsPage methodThe new
goToSettingsPagemethod is a solid addition to theMobileNavigationclass. It maintains consistency with other navigation methods by:
- Handling navbar states appropriately.
- Following the established navigation process.
- Returning the
settingsPageinstance.This implementation contributes to the overall consistency and reliability of the navigation system.
20-25: Review dragNavbarDown implementationThe
dragNavbarDownmethod usesthis.navbaras both the source and target of the drag operation. This might not produce the intended effect. Consider adjusting the implementation to ensure the navbar is dragged down correctly.To verify the correct implementation, you can run the following script:
Based on the results, you may need to adjust the
dragNavbarDownmethod. Consider usingthis.page.mousefor a more precise drag operation or adjusting the target element.packages/desktop-client/e2e/budget.mobile.test.js (6)
9-13: LGTM: Utility function implementation is correct.The
copyLastMonthBudgetfunction correctly implements the budget menu interaction pattern.
15-19: LGTM: Utility function implementation is correct.The
setTo3MonthAveragefunction correctly implements the budget menu interaction pattern.
21-31: LGTM: Utility functions implementation is correct.The
setTo6MonthAverageandsetToYearlyAveragefunctions correctly implement the budget menu interaction pattern.
254-271: LGTM: Test case for copying last month's budget is well-implemented.The test case correctly verifies the functionality of the
copyLastMonthBudgetfunction by comparing the budgeted amount with the previous month's value.
Line range hint
1-383: Overall, excellent improvements to the budget management test suite.The changes in this file significantly enhance the budget management functionality and test coverage. New utility functions and test cases are well-implemented, and existing tests have been updated consistently. The code demonstrates good practices in test structure and maintainability.
Key improvements:
- Addition of utility functions for various budget operations.
- New test cases for copying last month's budget and setting budget averages.
- Implementation of budget template functionality with accessibility considerations.
- Consistent updates to existing test cases using new modal objects.
Great job on these enhancements!
33-59: LGTM: Budget average calculation implemented correctly.The
setBudgetAveragefunction correctly calculates the average spent amount and applies it to the budget. It addresses the past review comments about infinite recursion and potential stale elements.To ensure the function correctly returns to the current month, we can run the following verification:
This script will help confirm that the function properly returns to the current month after calculating the average.
✅ Verification successful
Action Required: Correct the shell script to use the proper file type for ripgrep.
Please run the following script to verify that the function correctly navigates back to the current month:
Action Required: Verify the presence of
goToNextMonthfunction call within the test file.Please run the following script to check if the
goToNextMonthfunction is being called withinbudget.mobile.test.js:
LGTM: Budget average calculation implemented correctly.
The
setBudgetAveragefunction correctly calculates the average spent amount and ensures navigation back to the current month after computation. It addresses past review comments regarding infinite recursion and potential stale elements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the function navigates back to the current month # Search for the navigation logic rg --type javascript 'for \(let i = 0; i < numberOfMonths; i\+\+\) \{.*?await budgetPage\.goToNextMonth\(\);.*?\}' packages/desktop-client/e2e/budget.mobile.test.jsLength of output: 201
Script:
#!/bin/bash # Verify that the function navigates back to the current month # Search for the navigation logic rg --type js 'for \(let i = 0; i < numberOfMonths; i\+\+\) \{.*?await budgetPage\.goToNextMonth\(\);.*?\}' packages/desktop-client/e2e/budget.mobile.test.jsLength of output: 158
Script:
#!/bin/bash # Verify the presence of goToNextMonth function call rg 'await budgetPage\.goToNextMonth\(\)' packages/desktop-client/e2e/budget.mobile.test.jsLength of output: 228
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/desktop-client/src/components/Page.tsx (3)
Line range hint
67-67: Consider addingaria-levelto the heading roleIn the
MobilePageHeadercomponent, you've correctly usedrole="heading". To further improve accessibility, consider adding anaria-levelattribute to specify the heading level. For example:<View role="heading" aria-level={1} // ... other props > {title} </View>This helps screen reader users understand the document structure better.
Line range hint
106-106: Enhance type safety with explicit prop typesIn the
Pagecomponent, consider using more specific TypeScript types for some props. For example:type PageProps = { header: ReactNode; style?: CSSProperties; padding?: number; children: ReactNode; footer?: ReactNode; isNarrowWidth?: boolean; // Add this if it's a prop };This improves type safety and makes the component's API more clear.
Line range hint
108-150: Consider extracting responsive logicThe
Pagecomponent contains logic for responsive design based onisNarrowWidth. To improve maintainability and reusability, consider extracting this logic into a custom hook or a higher-order component. This separation of concerns could make the component easier to test and maintain.For example, you could create a
usePageLayouthook:function usePageLayout(header: ReactNode, isNarrowWidth: boolean) { const headerToRender = typeof header === 'string' ? isNarrowWidth ? <MobilePageHeader title={header} /> : <PageHeader title={header} /> : header; const style = { ...(isNarrowWidth ? {} : styles.page), flex: 1, backgroundColor: isNarrowWidth ? theme.mobilePageBackground : theme.pageBackground, }; return { headerToRender, style }; }Then use it in your
Pagecomponent:export function Page({ header, style, padding, children, footer }: PageProps) { const { isNarrowWidth } = useResponsive(); const { headerToRender, pageStyle } = usePageLayout(header, isNarrowWidth); // ... rest of the component }This refactoring would make the
Pagecomponent more focused on composition and less on logic.packages/desktop-client/e2e/page-models/mobile-navigation.js (3)
18-26: LGTM: Added dragNavbarUp method with a suggestionThe
dragNavbarUpmethod effectively simulates dragging the navbar up. It uses the bounding box of the main content area to calculate the drag distance, which is a good approach.Consider extracting the magic number
1used in the x-coordinates to a named constant for better readability:const DRAG_X_OFFSET = 1; // ... sourcePosition: { x: DRAG_X_OFFSET, y: 0 }, targetPosition: { x: DRAG_X_OFFSET, y: boundingBox.height / 2 },
29-41: LGTM: Added dragNavbarDown method with suggestionsThe
dragNavbarDownmethod effectively simulates dragging the navbar down. It uses the bounding box of the navbar and theNAVBAR_ROWSconstant to calculate the drag distance, which is a good approach.
Consider extracting the magic number
1used in the x-coordinates to a named constant for better readability, similar to the suggestion fordragNavbarUp.The method uses the same element (
this.navbarSelector) as both source and target for the drag operation. While this might work as intended, it's worth verifying that this produces the desired behavior in all scenarios.const DRAG_X_OFFSET = 1; // ... sourcePosition: { x: DRAG_X_OFFSET, y: 0 }, targetPosition: { x: DRAG_X_OFFSET, y: boundingBox.height / this.NAVBAR_ROWS, },
70-97: LGTM: Added navigateToPage method with a suggestionThe
navigateToPagemethod is well-implemented and effectively centralizes the navigation logic for all pages. It handles various scenarios and uses the constants and helper methods defined earlier in the class.Consider adding a timeout to the
waitForcalls to handle potential loading issues:await pageInstance.waitFor({ timeout: 5000 });This ensures that the test doesn't hang indefinitely if there's an issue with page loading.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/desktop-client/e2e/page-models/mobile-navigation.js (1 hunks)
- packages/desktop-client/src/components/Page.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
packages/desktop-client/src/components/Page.tsx (1)
143-143: Excellent accessibility enhancement!Adding the
role="main"attribute to the<View>component that wraps the main content is a great improvement. This change enhances the semantic structure of the page, making it more accessible to users of assistive technologies. It aligns well with Web Content Accessibility Guidelines (WCAG) and helps screen readers identify the primary content area of the page.packages/desktop-client/e2e/page-models/mobile-navigation.js (8)
3-3: LGTM: New import for MobileReportsPageThe addition of the
MobileReportsPageimport is consistent with the existing import pattern and supports the newgoToReportsPagemethod.
10-13: LGTM: Enhanced constructor with key element selectorsThe additions to the constructor provide convenient access to important page elements and selectors. The use of roles for element selection is a good practice for accessibility.
16-16: LGTM: Added NAVBAR_ROWS constantThe addition of the
NAVBAR_ROWSconstant is a good practice. It makes the code more maintainable by centralizing the definition of the number of navbar rows.
44-51: LGTM: Added hasNavbarState methodThe
hasNavbarStatemethod is well-implemented. It efficiently checks for the presence of the navbar and its state. The use of the rest parameter syntax to accept multiple states provides flexibility in usage.
54-60: LGTM: Added NAV_LINKS_HIDDEN_BY_DEFAULT constantThe addition of the
NAV_LINKS_HIDDEN_BY_DEFAULTconstant is a good practice. It centralizes the definition of which navigation links are hidden by default, making the code more maintainable and easier to update.
62-68: LGTM: Added ROUTES_BY_PAGE constantThe addition of the
ROUTES_BY_PAGEconstant is a good practice. It centralizes the mapping between page names and their routes, making the code more maintainable and easier to update if routes change in the future.
100-126: LGTM: Updated navigation methodsThe refactoring of the navigation methods to use the new
navigateToPagemethod is a significant improvement. It reduces code duplication and centralizes the navigation logic, making the code more maintainable. The addition of thegoToReportsPagemethod is consistent with the other navigation methods.
1-126: Overall assessment: Significant improvements to mobile navigationThe changes to the
MobileNavigationclass represent a substantial improvement in the mobile navigation functionality and code quality. Key improvements include:
- Introduction of new helper methods (
dragNavbarUp,dragNavbarDown,hasNavbarState) for managing the navbar state.- Addition of constants (
NAVBAR_ROWS,NAV_LINKS_HIDDEN_BY_DEFAULT,ROUTES_BY_PAGE) that centralize important configuration data.- Implementation of a generic
navigateToPagemethod that centralizes navigation logic and reduces code duplication.- Refactoring of existing navigation methods to use the new
navigateToPagemethod.- Addition of a new
goToReportsPagemethod, consistent with other navigation methods.These changes enhance the maintainability, readability, and flexibility of the mobile navigation code. The use of constants and helper methods makes the code more robust and easier to update in the future.
Great job on these improvements! The code is now more organized and easier to maintain.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (3)
109-121: LGTM: Well-implementedopenFullfunction with a suggestionThe
openFullfunction is well-implemented, usinguseCallbackfor optimization and providing different spring configurations for a better user experience.Consider adding a comment explaining the purpose of the
canceledparameter and how it affects the animation. This would improve code readability for future maintainers.
123-145: LGTM: Well-implementedopenDefaultandhidefunctions with a suggestionThe
openDefaultandhidefunctions are well-implemented, consistent withopenFull, and useuseCallbackfor optimization.Consider refactoring the common logic in
openFull,openDefault, andhideinto a single function to reduce code duplication. For example:const animateNavbar = useCallback( (state: 'open' | 'default' | 'hidden', y: number, velocity = 0, config = config.stiff) => { setNavbarState(state); api.start({ y, immediate: false, config: { ...config, velocity }, }); }, [api] );Then you can call this function from
openFull,openDefault, andhidewith the appropriate parameters.
217-217: LGTM: Addeddata-navbar-stateattribute with a suggestionThe addition of the
data-navbar-stateattribute is a good practice. It improves the component's testability, accessibility, and debuggability by exposing the current state of the navbar.Consider adding appropriate ARIA attributes to further enhance accessibility. For example, you could add
aria-expanded={navbarState !== 'hidden'}to indicate when the navbar is expanded or collapsed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (7 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (2)
36-38: LGTM: Improved state management for navbar visibilityThe introduction of
navbarStatewith clear, descriptive values ('default', 'open', 'hidden') enhances the control over the navbar's visibility. Setting the initial state to 'default' resolves the previous issue of potential undefined state.
158-158: LGTM: UpdateduseEffectand drag handling logicThe changes in the
useEffecthook and drag handling logic align well with the new state management and callback functions. The behavior remains consistent while providing more granular control over the navbar's state and animations.Also applies to: 181-181, 183-183
joel-jeremy
force-pushed
the
mobile-budget-menu-modal-vrt
branch
from
2589558
to
5dcc22d
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
packages/desktop-client/e2e/page-models/settings-page.js (3)
4-16: LGTM! Consider adding JSDoc comments for better documentation.The addition of these properties in the constructor improves code readability and maintainability. Using
data-testidattributes and ARIA roles for element selection is a good practice for E2E testing.Consider adding JSDoc comments for each property to provide more context about their purpose and usage. For example:
/** * @type {Locator} * @description Locator for the export data button */ this.exportDataButton = this.settings.getByRole('button', { name: 'Export data', });
29-34: LGTM! Consider adding error handling for unexpected button text.The changes to the
useBudgetTypemethod are good improvements:
- Using the
switchBudgetTypeButtonproperty improves consistency.- Checking the button text before clicking prevents unnecessary actions.
Consider adding error handling for cases where the button text doesn't match either expected value. For example:
const buttonText = await this.switchBudgetTypeButton.textContent(); if (buttonText.toLowerCase().includes(budgetType.toLowerCase())) { await this.switchBudgetTypeButton.click(); } else if (!buttonText.toLowerCase().includes('envelope') && !buttonText.toLowerCase().includes('tracking')) { throw new Error(`Unexpected budget type button text: ${buttonText}`); }
38-51: LGTM! Consider adding a timeout for visibility checks.The changes to the
enableExperimentalFeaturemethod significantly improve its robustness:
- Checking visibility before clicking prevents errors with hidden elements.
- Verifying the checkbox state before clicking prevents unnecessary toggling.
Consider adding a timeout to the visibility checks to prevent potential hangs if an element never becomes visible. For example:
const timeout = 5000; // 5 seconds if (await this.advancedSettingsButton.isVisible({ timeout })) { await this.advancedSettingsButton.click(); } if (await this.experimentalSettingsButton.isVisible({ timeout })) { await this.experimentalSettingsButton.click(); }packages/desktop-client/e2e/page-models/mobile-budget-page.js (1)
86-87: Consider adding a comment to clarify the method's purpose.The
waitFormethod has been made more generic, which is good for flexibility. However, it might be helpful to add a comment explaining its specific use in the context of the budget table.packages/desktop-client/e2e/budget.mobile.test.js (4)
33-59: Consider adding comments and handling state changesThe
setBudgetAveragefunction is well-implemented, but consider the following improvements:
- Add a comment explaining the purpose of the
setBudgetAverageFnparameter for clarity.- Consider wrapping the month navigation in a try-finally block to ensure the budget page always returns to the original month, even if an error occurs:
const originalMonth = await budgetPage.getSelectedMonth(); try { // Existing logic for calculating average } finally { // Navigate back to the original month while (await budgetPage.getSelectedMonth() !== originalMonth) { await budgetPage.goToNextMonth(); } }This ensures that the test state is preserved regardless of the function's outcome.
254-271: Enhance assertion clarityThe test case for copying last month's budget is well-implemented. To improve clarity, consider using a more explicit assertion:
const expectedBudget = currencyToAmount(lastMonthBudget); const actualBudget = currencyToAmount(await budgetedButton.textContent()); expect(actualBudget).toBe(expectedBudget);This approach converts the currency strings to numbers for comparison, making the assertion more robust against formatting differences.
273-298: Enhance test case clarity and assertionsThe test cases for setting budget to average are well-implemented. Consider the following improvements:
- Add a comment explaining the test structure:
// Parameterized test cases for 3, 6, and 12-month budget averages
- Enhance the assertion for better clarity:
const expectedBudget = Math.abs(averageSpent); const actualBudget = currencyToAmount(await budgetedButton.textContent()); expect(actualBudget).toBeCloseTo(expectedBudget, 2); // Compare with 2 decimal places precisionThis approach provides more precise comparisons and better handles potential rounding issues.
300-328: Enhance test robustness and documentationThe test case for applying a budget template is well-implemented. Consider the following improvements:
- Add a comment explaining the purpose and format of the budget template:
// Test applying a budget template. Format: #template <amount>
- Add error handling for the experimental feature:
test.fail(({ browserName }) => browserName !== 'chromium', 'This test requires the experimental feature to be available');
- Verify that the experimental feature was successfully enabled:
await expect(settingsPage.isExperimentalFeatureEnabled('Goal templates')).resolves.toBe(true);These changes will improve the test's robustness and clarity.
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (1)
112-112: Typo in comment: 'passed' should be 'past'In the comment, replace "passed" with "past" for grammatical correctness.
Apply this diff to fix the typo:
- // when cancel is true, it means that the user passed the upwards threshold + // when cancel is true, it means that the user past the upwards threshold
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (41)
packages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-updates-the-budgeted-amount-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-updates-the-budgeted-amount-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-updates-the-budgeted-amount-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-4-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3583.mdis excluded by!**/*.md
📒 Files selected for processing (23)
- packages/desktop-client/e2e/budget.mobile.test.js (4 hunks)
- packages/desktop-client/e2e/page-models/account-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/close-account-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js (2 hunks)
- packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-page.js (7 hunks)
- packages/desktop-client/e2e/page-models/mobile-category-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-navigation.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-reports-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.js (2 hunks)
- packages/desktop-client/e2e/page-models/settings-page.js (1 hunks)
- packages/desktop-client/src/components/Notifications.tsx (1 hunks)
- packages/desktop-client/src/components/Page.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (7 hunks)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (2 hunks)
- packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (2 hunks)
- packages/desktop-client/src/components/reports/Overview.tsx (1 hunks)
- packages/desktop-client/src/components/settings/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/desktop-client/e2e/page-models/account-page.js
- packages/desktop-client/e2e/page-models/close-account-modal.js
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js
- packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.js
- packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.js
- packages/desktop-client/e2e/page-models/mobile-category-menu-modal.js
- packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.js
- packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.js
- packages/desktop-client/e2e/page-models/mobile-reports-page.js
- packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.js
- packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.js
- packages/desktop-client/src/components/Notifications.tsx
- packages/desktop-client/src/components/Page.tsx
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx
- packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/desktop-client/src/components/settings/index.tsx
🧰 Additional context used
🔇 Additional comments (18)
packages/desktop-client/e2e/page-models/settings-page.js (2)
18-20: LGTM! Good addition for managing asynchronous behavior.The
waitFormethod is a useful addition that provides a way to wait for the settings element to be ready. It's correctly implemented as an asynchronous method, which is appropriate for Playwright operations.
23-23: LGTM! Good refactoring for consistency.The
exportDatamethod has been simplified to use theexportDataButtonproperty defined in the constructor. This change improves code consistency and maintainability.packages/desktop-client/e2e/page-models/mobile-navigation.js (7)
3-3: LGTM: New import for MobileReportsPageThe addition of the import statement for
MobileReportsPageis correct and consistent with the existing import style. This import is likely used in the newgoToReportsPagemethod, enhancing the navigation capabilities of the class.
10-13: LGTM: Enhanced constructor with new propertiesThe additions to the constructor are well-structured and use role-based selectors, which is good for accessibility and testing. These new properties (heading, navbar, mainContentSelector, navbarSelector) are likely used in the new navigation methods, improving the overall functionality of the class.
18-26: LGTM: Well-implemented dragNavbarUp methodThe
dragNavbarUpmethod is well-implemented. It effectively uses thepage.dragAndDropmethod to simulate dragging the navbar up, and calculates the target position based on the main content's bounding box. The use of class properties (mainContentSelector, navbarSelector) makes the code clean and maintainable.
44-51: LGTM: Well-implemented hasNavbarState methodThe
hasNavbarStatemethod is well-implemented. It effectively checks the navbar's state based on itsdata-navbar-stateattribute and handles the case where the navbar might not exist on the page. The use of the rest parameter syntax (...states) allows for flexible checking of multiple states, which is a good practice.
70-97: LGTM: Well-implemented navigateToPage methodThe
navigateToPagemethod is well-implemented and effectively centralizes the logic for navigating to different pages. It handles checking the current URL, managing the navbar state, and performing the navigation in a clear and organized manner. The use of class properties and methods (likehasNavbarState,dragNavbarUp, anddragNavbarDown) makes the code clean and maintainable.The method also takes into account the
NAV_LINKS_HIDDEN_BY_DEFAULTto determine the appropriate navbar states, which is a good practice for handling different types of navigation links.
100-123: LGTM: Well-refactored navigation methodsThe navigation methods (
goToBudgetPage,goToAccountsPage,goToTransactionEntryPage,goToReportsPage) have been effectively refactored to use the newnavigateToPagemethod. This refactoring simplifies the methods, reduces code duplication, and improves maintainability by using a consistent pattern across all navigation methods.The use of arrow functions for creating page instances is a good practice, as it ensures that the page instance is created only when needed. This approach is both efficient and clean.
125-127: LGTM: Consistent refactoring of goToSettingsPageThe
goToSettingsPagemethod has been refactored consistently with the other navigation methods. It now uses thenavigateToPagemethod, passing the page name and a factory function for creating theSettingsPageinstance. This change maintains consistency across all navigation methods and improves the overall structure of the class.packages/desktop-client/e2e/page-models/mobile-budget-page.js (7)
2-6: LGTM: New modal imports enhance modularity.The addition of these modal imports indicates a shift towards a more modular architecture, which should improve code organization and maintainability.
147-148: LGTM: NewopenCategoryMenumethod enhances functionality.This new method follows the established pattern and provides a clean interface for opening the category menu modal.
187-187: LGTM:openBudgetMenuupdated to useBudgetMenuModal.This change aligns with the modal-based approach seen in other methods, providing a consistent interface for budget-related actions.
204-204: LGTM:openBalanceMenuupdated to useBalanceMenuModal.This change is consistent with the updates to other methods and enhances the functionality by providing access to the balance menu modal.
276-283: LGTM:openEnvelopeBudgetSummaryupdated for consistency.The method has been renamed for clarity and now returns an
EnvelopeBudgetSummaryModalinstance, consistent with other modal-based methods.
308-315: LGTM:openTrackingBudgetSummaryupdated for consistency.The method has been renamed for clarity and now returns a
TrackingBudgetSummaryModalinstance, consistent with other modal-based methods.
Line range hint
1-316: Overall: Excellent refactoring to a modal-based approach.The changes in this file consistently implement a modal-based approach for various budget-related actions. This refactoring improves modularity, consistency, and should enhance maintainability of the codebase. The new structure allows for better separation of concerns between the main page and specific modal interactions.
Great job on this refactoring effort!
packages/desktop-client/e2e/budget.mobile.test.js (1)
Line range hint
1-383: Overall assessment: Good additions with room for improvementThe changes introduce valuable new functionality for budget management and comprehensive test coverage. The new utility functions and test cases are well-implemented and provide good coverage of the new features. However, there are opportunities for improvement:
- Refactor utility functions to reduce code duplication.
- Enhance test assertions for more precise comparisons.
- Improve error handling and documentation in test cases.
- Consider adding more inline comments to explain complex logic or test structures.
These improvements will enhance the maintainability and robustness of the codebase. Great work on expanding the functionality and test coverage!
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (1)
36-38: Initialization ofnavbarStateensures consistent stateThe
navbarStateis now correctly initialized to'default', which prevents it from beingundefinedon the initial render. This ensures consistent behavior across renders.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/e2e/page-models/mobile-navigation.js (3)
16-30: LGTM: Well-structured constants with a minor suggestionThe addition of static private properties (
#NAVBAR_ROWS,#NAV_LINKS_HIDDEN_BY_DEFAULT, and#ROUTES_BY_PAGE) is a good practice for encapsulating constants within the class. The use of the#prefix for private fields is a nice touch, leveraging modern JavaScript features.Consider adding JSDoc comments for these static properties to provide more context about their purpose and usage. For example:
/** * The number of rows in the navbar. * @type {number} */ static #NAVBAR_ROWS = 3;
50-62: LGTM with a suggestion: dragNavbarDown implementationThe
dragNavbarDown()method is well-implemented, usingpage.dragAndDrop()to simulate dragging the navbar down. The calculation of the target position based on the navbar's height and#NAVBAR_ROWSis correct.Consider modifying the drag target to be a point just below the navbar instead of using the navbar itself as both source and target. This might produce a more reliable drag effect across different testing environments:
- await this.page.dragAndDrop(this.navbarSelector, this.navbarSelector, { + await this.page.dragAndDrop(this.navbarSelector, this.mainContentSelector, { sourcePosition: { x: 1, y: 0 }, targetPosition: { x: 1, - y: boundingBox.height / MobileNavigation.#NAVBAR_ROWS, + y: boundingBox.height + 1, // Just below the navbar }, });
75-104: LGTM: Well-structured navigateToPage method with a minor suggestionThe
navigateToPage()method is well-implemented, centralizing the navigation logic for different pages. It correctly checks the current page, handles navbar states, and uses the static properties defined earlier. The use of a factory function for creating page instances is a good practice.Consider adding error handling for cases where the page name doesn't exist in
#ROUTES_BY_PAGE. For example:if (!MobileNavigation.#ROUTES_BY_PAGE[pageName]) { throw new Error(`Invalid page name: ${pageName}`); }This would help catch potential typos or invalid page names early in the navigation process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/e2e/page-models/mobile-navigation.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
packages/desktop-client/e2e/page-models/mobile-navigation.js (6)
3-3: LGTM: Improved organization and maintainabilityThe addition of the
MobileReportsPageimport and the new properties in the constructor enhance the organization of the code. The use of role-based selectors (heading,navbar) and the introduction ofmainContentSelectorandnavbarSelectorimprove maintainability and make the code more semantic.Also applies to: 10-13
32-47: LGTM: Well-implemented dragNavbarUp methodThe
dragNavbarUp()method is well-implemented, usingpage.dragAndDrop()to simulate user interaction. The calculation of drag positions based on bounding boxes is correct and should accurately simulate dragging the navbar up into the main content area.
65-72: LGTM: Well-implemented hasNavbarState methodThe
hasNavbarState()method is well-implemented. It correctly handles the case where the navbar doesn't exist on the page and uses thedata-navbar-stateattribute to determine the navbar's state. The use of rest parameters (...states) provides flexibility in checking multiple states.
107-130: LGTM: Excellent refactoring of navigation methodsThe refactoring of
goToBudgetPage(),goToAccountsPage(),goToTransactionEntryPage(), andgoToReportsPage()to use the newnavigateToPage()method is excellent. This change significantly reduces code duplication and improves maintainability. The consistent use of arrow functions for creating page instances is clean and concise.
132-133: LGTM: Consistent refactoring of goToSettingsPageThe refactoring of
goToSettingsPage()to use the newnavigateToPage()method is consistent with the changes made to other navigation methods. This change maintains the uniformity of the navigation logic across all pages.
1-134: Excellent refactoring and enhancement of MobileNavigation classThe changes made to the
MobileNavigationclass significantly improve its functionality, maintainability, and extensibility. Key improvements include:
- Centralized navigation logic in the
navigateToPage()method, reducing code duplication.- Addition of methods for controlling navbar behavior (
dragNavbarUp(),dragNavbarDown(),hasNavbarState()).- Use of static private fields for constants, improving encapsulation.
- Consistent refactoring of all navigation methods to use the new centralized logic.
- Improved organization of selectors and properties in the constructor.
These changes make the code more robust and easier to maintain. The use of modern JavaScript features like private fields and arrow functions is commendable. This refactoring will make it easier to add new pages or modify navigation behavior in the future.
Great job on this refactoring! The code is now more organized, efficient, and easier to understand.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
joel-jeremy
force-pushed
the
mobile-budget-menu-modal-vrt
branch
from
4c58904
to
9746c67
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
packages/desktop-client/e2e/page-models/mobile-navigation.js (4)
16-30: LGTM: Well-structured static propertiesThe addition of static properties
#NAVBAR_ROWS,#NAV_LINKS_HIDDEN_BY_DEFAULT, and#ROUTES_BY_PAGEis well-implemented. The use of private fields (#) ensures encapsulation.Consider adding JSDoc comments to describe the purpose of each static property for better code documentation.
32-47: LGTM: Well-implementeddragNavbarUpmethodThe
dragNavbarUpmethod effectively simulates dragging the navbar into the main content area. The calculation of bounding boxes and use ofpage.dragAndDropare appropriate.Consider adding error handling for cases where the main content or navbar elements might not be found:
async dragNavbarUp() { const mainContent = this.page.locator(this.mainContentSelector); const navbar = this.page.locator(this.navbarSelector); await Promise.all([mainContent.waitFor(), navbar.waitFor()]); const mainContentBoundingBox = await mainContent.boundingBox(); const navbarBoundingBox = await navbar.boundingBox(); if (!mainContentBoundingBox || !navbarBoundingBox) { throw new Error('Failed to get bounding boxes for main content or navbar'); } // ... rest of the method }
50-62: LGTM: Well-implementeddragNavbarDownmethodThe
dragNavbarDownmethod effectively simulates dragging the navbar down. The use of#NAVBAR_ROWSfor calculating the drag distance is appropriate.Consider adding error handling similar to the suggestion for
dragNavbarUp:async dragNavbarDown() { const navbar = this.page.locator(this.navbarSelector); await navbar.waitFor(); const boundingBox = await navbar.boundingBox(); if (!boundingBox) { throw new Error('Failed to get bounding box for navbar'); } // ... rest of the method }
75-104: LGTM: Well-implementednavigateToPagemethodThe
navigateToPagemethod effectively centralizes navigation logic, reducing code duplication and ensuring consistent behavior across different pages. The handling of navbar states and use of static properties is well-implemented.Consider adding a timeout to the
waitForcalls to handle potential navigation issues:const timeout = 5000; // 5 seconds await this.navbar.waitFor({ timeout }); // ... await pageInstance.waitFor({ timeout });This ensures that the navigation doesn't hang indefinitely if there's an unexpected issue.
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (1)
109-145: LGTM: Well-structured callback functions for navbar managementThe new callback functions (
openFull,openDefault, andhide) are well-implemented and provide clear management of the navbar's state and animations. The use ofuseCallbackfor memoization is appropriate.One minor suggestion:
In the
openFullfunction, thecanceledproperty is destructured but not used. Consider removing it if it's not needed:-const openFull = useCallback( - ({ canceled }: { canceled: boolean }) => { +const openFull = useCallback( + () => { setNavbarState('open'); - // when cancel is true, it means that the user passed the upwards threshold - // so we change the spring config to create a nice wobbly effect api.start({ y: openFullY, immediate: false, - config: canceled ? config.wobbly : config.stiff, + config: config.stiff, }); }, [api, openFullY], );If the
canceledproperty is indeed needed for the wobbly effect, please add a comment explaining its purpose and how it's determined.packages/desktop-client/e2e/budget.mobile.test.js (2)
33-59: Ensure correct retrieval of spent amounts after month navigationIn the
setBudgetAveragefunction, you're correctly retrieving thespentButtonafter navigating to the previous month within the loop. This ensures that you're accessing the correct element corresponding to the current month in each iteration. Good job on implementing this correctly!However, to improve readability and make the intent clearer, consider adding a comment explaining why you're retrieving the
spentButtoninside the loop. For example:for (let i = 0; i < numberOfMonths; i++) { await budgetPage.goToPreviousMonth(); // Retrieve the spent button after navigation to ensure we're getting the correct element for the current month const spentButton = await budgetPage.getButtonForSpent(categoryName); const spent = await spentButton.textContent(); totalSpent += currencyToAmount(spent); }This comment will help future maintainers understand the reasoning behind this implementation.
273-298: LGTM: New test cases for setting budget to averageThese new test cases effectively verify the functionality of setting the budget to 3, 6, and 12 month averages. The use of a parameterized approach is excellent for reducing code duplication and ensuring consistent testing across different average periods.
Suggestion for improvement:
Consider adding a descriptive comment before the test cases to explain the purpose of the array and how it's used in the parameterized tests. This will enhance readability for other developers. For example:// Test cases for setting budget to average over different periods // Each entry is [number of months, corresponding setter function] [ [3, setTo3MonthAverage], [6, setTo6MonthAverage], [12, setToYearlyAverage], ].forEach(([numberOfMonths, setBudgetAverageFn]) => { // ... (rest of the code remains the same) });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (38)
packages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-4-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3583.mdis excluded by!**/*.md
📒 Files selected for processing (23)
- packages/desktop-client/e2e/budget.mobile.test.js (4 hunks)
- packages/desktop-client/e2e/page-models/account-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/close-account-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js (2 hunks)
- packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-page.js (7 hunks)
- packages/desktop-client/e2e/page-models/mobile-category-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-navigation.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-reports-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.js (2 hunks)
- packages/desktop-client/e2e/page-models/settings-page.js (1 hunks)
- packages/desktop-client/src/components/Notifications.tsx (1 hunks)
- packages/desktop-client/src/components/Page.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (7 hunks)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (2 hunks)
- packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (2 hunks)
- packages/desktop-client/src/components/reports/Overview.tsx (1 hunks)
- packages/desktop-client/src/components/settings/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/desktop-client/e2e/page-models/account-page.js
- packages/desktop-client/e2e/page-models/close-account-modal.js
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js
- packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.js
- packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.js
- packages/desktop-client/e2e/page-models/mobile-category-menu-modal.js
- packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.js
- packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.js
- packages/desktop-client/e2e/page-models/mobile-reports-page.js
- packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.js
- packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.js
- packages/desktop-client/e2e/page-models/settings-page.js
- packages/desktop-client/src/components/Notifications.tsx
- packages/desktop-client/src/components/Page.tsx
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx
- packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/desktop-client/src/components/settings/index.tsx
🧰 Additional context used
🔇 Additional comments (13)
packages/desktop-client/e2e/page-models/mobile-navigation.js (4)
3-3: LGTM: Enhanced constructor and importsThe addition of the
MobileReportsPageimport and the new constructor properties (heading,navbar,mainContentSelector, andnavbarSelector) are well-implemented. The use of role-based selectors is a good practice for accessibility and testing.Also applies to: 10-13
65-72: LGTM: Well-implementedhasNavbarStatemethodThe
hasNavbarStatemethod is well-implemented. It correctly handles cases where the navbar is not present and uses thedata-navbar-stateattribute to determine the state. The flexibility to check for multiple states is a good design choice.
107-130: LGTM: Consistent updates to navigation methodsThe updates to the navigation methods (
goToBudgetPage,goToAccountsPage,goToTransactionEntryPage,goToReportsPage) are well-implemented. The consistent use of the newnavigateToPagemethod reduces code duplication and ensures uniform navigation behavior across different pages.
132-133: LGTM: Consistent update togoToSettingsPageThe update to the
goToSettingsPagemethod is well-implemented and consistent with the changes made to other navigation methods. It correctly uses the newnavigateToPagemethod and theSettingsPageclass.packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (5)
1-6: LGTM: Necessary imports addedThe new imports (
useCallback,useEffect, anduseState) are correctly added and are essential for the refactored component's functionality.
36-38: LGTM: State initialization addressedThe
navbarStateis now properly initialized with a default value, addressing the concern raised in a previous review. The state values are well-defined, providing clear representation of the navbar's possible states.
103-104: LGTM: Clear constants for navbar positionsThe introduction of
openFullYandopenDefaultYconstants improves code readability and maintainability. These values clearly represent different positions of the navbar.
181-183: LGTM: Updated drag handler aligns with new state managementThe changes in the drag handler correctly use the new
openDefaultandopenFullfunctions, aligning with the refactored navbar state management. The use of velocity inopenDefaultis a good touch for smooth animations.Note: As mentioned in a previous comment, consider reviewing the use of the
canceledparameter in theopenFullfunction, as it's passed here but not utilized in the function body.
217-217: LGTM: Helpful data attribute for navbar stateThe addition of the
data-navbar-stateattribute is a good practice. It provides a clear indicator of the navbar's current state, which can be useful for testing and potentially for CSS styling based on the navbar state.packages/desktop-client/e2e/budget.mobile.test.js (2)
240-252: LGTM: Updated test case for setting budget amountThe changes to this test case are consistent with the new implementation of budget management using modals. The test now correctly opens the budget menu modal, sets the budget amount, and verifies the updated value. This ensures that the new modal-based interaction is properly tested.
254-271: LGTM: New test case for copying last month's budgetThis new test case effectively verifies the 'copy last month's budget' functionality. It covers the following key aspects:
- Navigating to the previous month
- Retrieving the budget amount from the previous month
- Returning to the current month
- Applying the 'copy last month's budget' action
- Verifying that the current month's budget matches the previous month's budget
The test is well-structured and provides good coverage for this feature.
packages/desktop-client/e2e/page-models/mobile-budget-page.js (2)
147-148:openCategoryMenumethod implementation looks goodThe
openCategoryMenumethod is correctly implemented and follows the existing patterns for opening modals.
86-87: Ensure all references towaitForBudgetTableare updatedThe method
waitForBudgetTable()has been renamed towaitFor(options). Please verify that all references towaitForBudgetTablein the codebase have been updated to prevent any broken functionality.Run the following script to check for any remaining references:
✅ Verification successful
All references to
waitForBudgetTablehave been successfully updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any references to waitForBudgetTable in the codebase. # Test: Search for occurrences of 'waitForBudgetTable'. Expect: No matches. rg --type js --type ts 'waitForBudgetTable'Length of output: 43
| openDefault(); | ||
| } | ||
| }, [scrollY]); | ||
| }, [scrollY, hide, openDefault]); |
There was a problem hiding this comment.
Add missing dependency to useEffect
The change from close() to openDefault() aligns well with the new navbar state management. However, the useEffect dependency array is still incomplete.
Add previousScrollY to the dependency array to ensure the effect runs correctly when this value changes:
useEffect(() => {
if (
scrollY &&
previousScrollY &&
scrollY > previousScrollY &&
previousScrollY !== 0
) {
hide();
} else {
openDefault();
}
-}, [scrollY, hide, openDefault]);
+}, [scrollY, previousScrollY, hide, openDefault]);📝 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.
| openDefault(); | |
| } | |
| }, [scrollY]); | |
| }, [scrollY, hide, openDefault]); | |
| openDefault(); | |
| } | |
| }, [scrollY, previousScrollY, hide, openDefault]); |
| const copyLastMonthBudget = async (budgetPage, categoryName) => { | ||
| const budgetMenuModal = await budgetPage.openBudgetMenu(categoryName); | ||
| await budgetMenuModal.copyLastMonthBudget(); | ||
| await budgetMenuModal.close(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication
The copyLastMonthBudget function follows a pattern that's repeated in other utility functions. To improve maintainability and reduce duplication, consider implementing a higher-order function as suggested in the past review comments:
const performBudgetMenuAction = async (budgetPage, categoryName, action) => {
const budgetMenuModal = await budgetPage.openBudgetMenu(categoryName);
await action(budgetMenuModal);
await budgetMenuModal.close();
};
const copyLastMonthBudget = (budgetPage, categoryName) =>
performBudgetMenuAction(budgetPage, categoryName, menu => menu.copyLastMonthBudget());This refactoring can be applied to all similar functions (setTo3MonthAverage, setTo6MonthAverage, setToYearlyAverage).
|
|
||
| await this.page.keyboard.type(String(newAmount)); | ||
| await this.page.keyboard.press('Enter'); | ||
| return new BudgetMenuModal(this.page, this.page.getByRole('dialog')); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor modal instantiation to reduce code duplication
The instantiation of modal classes with this.page and this.page.getByRole('dialog') is repeated across multiple methods (openBudgetMenu, openBalanceMenu, openEnvelopeBudgetSummary, openTrackingBudgetSummary). Consider creating a helper method to streamline modal creation and improve maintainability.
You can implement a private helper method like this:
+ #createModal(ModalClass) {
+ return new ModalClass(this.page, this.page.getByRole('dialog'));
+ }Then update the methods to use the helper:
// In openBudgetMenu
- return new BudgetMenuModal(this.page, this.page.getByRole('dialog'));
+ return this.#createModal(BudgetMenuModal);
// In openBalanceMenu
- return new BalanceMenuModal(this.page, this.page.getByRole('dialog'));
+ return this.#createModal(BalanceMenuModal);
// In openEnvelopeBudgetSummary
- return new EnvelopeBudgetSummaryModal(this.page, this.page.getByRole('dialog'));
+ return this.#createModal(EnvelopeBudgetSummaryModal);
// In openTrackingBudgetSummary
- return new TrackingBudgetSummaryModal(this.page, this.page.getByRole('dialog'));
+ return this.#createModal(TrackingBudgetSummaryModal);Also applies to: 204-204, 280-283, 312-315
|
Looks like the test is flaky:
Would you mind taking a peak at it to make it more stable? Apart from that LGTM |
joel-jeremy
force-pushed
the
mobile-budget-menu-modal-vrt
branch
from
88d5375
to
557a4f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
packages/desktop-client/e2e/page-models/settings-page.js (3)
18-20: Good addition of the waitFor method.This method enhances test stability by providing a consistent way to wait for the settings element. To further improve its flexibility, consider adding a default timeout value:
async waitFor(options = { timeout: 5000 }) { await this.settings.waitFor(options); }This allows users to override the default timeout if needed.
29-34: Good improvements to the useBudgetType method.The use of the pre-defined
switchBudgetTypeButtonproperty and the text check before clicking are excellent improvements. They enhance consistency and prevent unnecessary actions.To make the method more robust, consider using a case-insensitive comparison:
if (buttonText.toLowerCase().includes(budgetType.toLowerCase())) { await this.switchBudgetTypeButton.click(); }This change would make the method more resilient to potential variations in button text capitalization.
38-51: Good enhancements to the enableExperimentalFeature method.The added visibility checks and the conditional click on the checkbox improve the method's robustness and efficiency. These changes will help prevent errors and unnecessary actions during test execution.
To further improve error handling, consider adding timeouts and error messages:
async enableExperimentalFeature(featureName, timeout = 5000) { try { await this.advancedSettingsButton.waitFor({ state: 'visible', timeout }); await this.advancedSettingsButton.click(); await this.experimentalSettingsButton.waitFor({ state: 'visible', timeout }); await this.experimentalSettingsButton.click(); const featureCheckbox = this.page.getByRole('checkbox', { name: featureName }); await featureCheckbox.waitFor({ state: 'visible', timeout }); if (!(await featureCheckbox.isChecked())) { await featureCheckbox.click(); } } catch (error) { throw new Error(`Failed to enable experimental feature "${featureName}": ${error.message}`); } }This implementation adds timeouts to the visibility checks and wraps the entire method in a try-catch block for better error reporting.
packages/desktop-client/e2e/page-models/mobile-navigation.js (1)
1-141: Overall assessment: Excellent improvements to mobile navigationThe changes made to the
MobileNavigationclass significantly enhance its functionality and maintainability:
- The addition of static constants improves configuration management.
- New methods for navbar manipulation (dragNavbarUp, dragNavbarDown, hasNavbarState) provide fine-grained control over navigation behavior.
- The centralized
navigateToPagemethod and refactored navigation methods reduce code duplication and improve consistency.- The use of role-based selectors enhances accessibility and testability.
These improvements will likely lead to more robust and maintainable mobile navigation testing.
Consider documenting the expected navbar states and their implications in a comment at the class level. This would provide valuable context for future maintainers and help ensure consistent usage of the navigation methods.
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (1)
217-217: LGTM: Added data-navbar-state attributeThe addition of the
data-navbar-stateattribute is a good improvement. It provides a clear indication of the navbar's current state in the DOM, which can be useful for styling or testing purposes.Consider using this attribute in your CSS or test scripts to ensure the navbar is in the correct state during different interactions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (41)
packages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-applies-budget-template-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-copies-last-month-s-budget-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-12-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-3-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-set-budget-to-6-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-updates-the-budgeted-amount-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-updates-the-budgeted-amount-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-updates-the-budgeted-amount-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-applies-budget-template-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-copies-last-month-s-budget-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-12-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-3-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-set-budget-to-6-month-average-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-updates-the-budgeted-amount-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-4-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3583.mdis excluded by!**/*.md
📒 Files selected for processing (24)
- packages/desktop-client/e2e/budget.mobile.test.js (4 hunks)
- packages/desktop-client/e2e/budget.test.js (1 hunks)
- packages/desktop-client/e2e/page-models/account-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/close-account-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js (2 hunks)
- packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-page.js (7 hunks)
- packages/desktop-client/e2e/page-models/mobile-category-menu-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-navigation.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-reports-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.js (2 hunks)
- packages/desktop-client/e2e/page-models/settings-page.js (1 hunks)
- packages/desktop-client/src/components/Notifications.tsx (1 hunks)
- packages/desktop-client/src/components/Page.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (7 hunks)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (2 hunks)
- packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (2 hunks)
- packages/desktop-client/src/components/reports/Overview.tsx (1 hunks)
- packages/desktop-client/src/components/settings/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/desktop-client/e2e/page-models/account-page.js
- packages/desktop-client/e2e/page-models/close-account-modal.js
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js
- packages/desktop-client/e2e/page-models/mobile-balance-menu-modal.js
- packages/desktop-client/e2e/page-models/mobile-budget-menu-modal.js
- packages/desktop-client/e2e/page-models/mobile-category-menu-modal.js
- packages/desktop-client/e2e/page-models/mobile-edit-notes-modal.js
- packages/desktop-client/e2e/page-models/mobile-envelope-budget-summary-modal.js
- packages/desktop-client/e2e/page-models/mobile-reports-page.js
- packages/desktop-client/e2e/page-models/mobile-tracking-budget-summary-modal.js
- packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.js
- packages/desktop-client/src/components/Notifications.tsx
- packages/desktop-client/src/components/Page.tsx
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx
- packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/desktop-client/src/components/settings/index.tsx
🧰 Additional context used
🔇 Additional comments (28)
packages/desktop-client/e2e/page-models/settings-page.js (2)
4-15: Excellent improvements to the constructor!The addition of these properties enhances code readability, maintainability, and test stability. Using
data-testidattributes andgetByRole()methods also improves accessibility testing. This refactoring will make future updates easier and more consistent.
23-23: Great simplification of the exportData method.Using the pre-defined
exportDataButtonproperty improves consistency and reduces duplication. This change makes the code more maintainable and less prone to errors if selectors need to be updated in the future.packages/desktop-client/e2e/page-models/mobile-navigation.js (7)
3-3: LGTM: New import for MobileReportsPageThe addition of the import statement for
MobileReportsPageis correct and consistent with the existing import style. This import is necessary for the newgoToReportsPagemethod implemented later in the class.
10-13: LGTM: Enhanced constructor with new propertiesThe additions to the constructor are well-implemented:
- Using
getByRoleforheadingandnavbarimproves accessibility and testability.- The
mainContentSelectorandnavbarSelectorare consistently defined using role attributes.These new properties lay the groundwork for the enhanced navigation functionality implemented in the class.
16-30: LGTM: Well-defined static constants for navigation configurationThe addition of static private constants is a good practice:
#NAVBAR_ROWSdefines the number of rows in the navbar.#NAV_LINKS_HIDDEN_BY_DEFAULTlists the navigation links that are initially hidden.#ROUTES_BY_PAGEmaps page names to their respective routes.These constants improve code maintainability and readability by centralizing configuration values.
32-47: LGTM: Well-implemented dragNavbarUp methodThe
dragNavbarUpmethod is well-implemented:
- It correctly calculates the drag distance using bounding boxes.
- The use of
page.dragAndDropaccurately simulates user interaction.- The navbar is positioned correctly at the bottom of the main content area.
This method enhances the navigation functionality by allowing the navbar to be revealed when hidden.
65-72: LGTM: Well-implemented hasNavbarState methodThe
hasNavbarStatemethod is well-designed:
- It first checks for the existence of the navbar, preventing potential errors.
- The use of the 'data-navbar-state' attribute is a good way to track the navbar's state.
- The method's flexibility in checking multiple states enhances its utility.
This method provides a robust way to determine the navbar's current state, which is crucial for the navigation logic.
75-104: LGTM: Excellent implementation of centralized navigation logicThe
navigateToPagemethod is very well-implemented:
- It efficiently checks if the user is already on the requested page, avoiding unnecessary navigation.
- The method handles the navbar state appropriately, revealing it when needed.
- Waiting for the new page to load and adjusting the navbar state afterwards ensures a smooth navigation experience.
- The use of a
pageModelFactoryprovides flexibility in instantiating page objects.This centralized navigation method significantly improves code maintainability and reduces duplication in individual navigation methods.
107-139: LGTM: Excellent refactoring of navigation methodsThe refactoring of all navigation methods (goToBudgetPage, goToAccountsPage, goToTransactionEntryPage, goToReportsPage, goToSettingsPage) is excellent:
- Each method now uses the centralized
navigateToPagefunction, significantly reducing code duplication.- The consistent pattern across all navigation methods improves readability and maintainability.
- The use of arrow functions for page model instantiation is concise and appropriate.
This refactoring greatly improves the overall structure and maintainability of the navigation logic in the
MobileNavigationclass.packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (6)
36-38: LGTM: Improved state management for navbar visibilityThe introduction of
navbarStatewith a defined initial state of 'default' is a good improvement. This addresses the previous concern about potential undefined state and provides more granular control over the navbar's visibility.
109-121: LGTM: Well-implementedopenFullcallbackThe
openFullcallback is well-implemented, providing a smooth animation for fully opening the navbar. The use ofuseCallbackfor optimization and the conditional wobbly effect add nice touches to the user experience.
123-133: LGTM: Well-implementedopenDefaultcallbackThe
openDefaultcallback is well-implemented, providing a smooth animation for partially opening the navbar. The inclusion of a velocity parameter allows for more natural animations based on user interactions.
135-145: LGTM: Well-implementedhidecallbackThe
hidecallback is well-implemented, providing a smooth animation for hiding the navbar. The inclusion of a velocity parameter allows for more natural animations based on user interactions.
181-183: LGTM: Improved drag handling logicThe updates to the drag handling logic align well with the new navbar state management. The use of velocity in
openDefaultand thecanceledparameter inopenFullshould provide smoother and more engaging animations for the user.
158-160:⚠️ Potential issueAdd missing dependency to useEffect
The change from
close()toopenDefault()aligns well with the new navbar state management. However, theuseEffectdependency array is still incomplete.Add
previousScrollYto the dependency array to ensure the effect runs correctly when this value changes:useEffect(() => { if ( scrollY && previousScrollY && scrollY > previousScrollY && previousScrollY !== 0 ) { hide(); } else { openDefault(); } -}, [scrollY, hide, openDefault]); +}, [scrollY, previousScrollY, hide, openDefault]);packages/desktop-client/e2e/budget.mobile.test.js (10)
3-7: LGTM: Appropriate imports addedThe new imports for currency conversion and month utilities are relevant to the added functionality and correctly placed.
93-94: LGTM: Proper test setup for budget typeThe addition of navigation to the settings page and setting the budget type ensures that the tests are run with the correct configuration.
218-220: LGTM: Improved category menu modal interactionThe updates to opening the category menu modal and checking its heading enhance the readability and maintainability of the test.
230-232: LGTM: Improved budget menu modal interactionThe updates to opening the budget menu modal and checking its heading enhance the readability and maintainability of the test.
240-252: LGTM: Well-structured test for updating budgeted amountThe test for updating the budgeted amount is well-structured and correctly uses the new budget menu modal interaction. It properly sets the budget amount and verifies the result.
254-271: LGTM: Comprehensive test for copying last month's budgetThis new test thoroughly verifies the functionality of copying the previous month's budget. It correctly navigates between months, uses the new
copyLastMonthBudgetutility function, and checks the expected outcome.
273-298: LGTM: Comprehensive tests for setting budget to averagesThese new tests efficiently cover setting the budget to 3, 6, and 12 month averages. The use of a loop to generate tests for different periods is a good practice, reducing code duplication. The tests correctly use the
setBudgetAveragefunction and verify the expected outcomes.
300-327: LGTM: Thorough test for applying budget templateThis new test comprehensively covers the functionality of applying a budget template. It correctly enables the experimental 'Goal templates' feature, sets up a template, applies it, and verifies the result. The test is well-structured and includes all necessary steps to validate the feature.
350-379: LGTM: Well-structured tests for balance and budget summary modalsThese updated tests for opening balance menu modal and budget summary modals are well-structured and correctly differentiate between Envelope and Tracking budget types. They use the new modal interaction methods appropriately and verify the expected outcomes.
Line range hint
1-382: Overall assessment: High-quality changes with minor improvement suggestionsThe changes in this file significantly enhance the test coverage for mobile budget functionality. New utility functions and tests have been added to cover various budget management scenarios. The code is generally well-structured and maintainable.
Two minor suggestions for improvement were made:
- Refactoring the utility functions to reduce code duplication.
- Improving the retrieval of spent amounts in the
setBudgetAveragefunction.These suggestions aim to further enhance code maintainability and reliability. Overall, the changes are of high quality and ready for merging after addressing the minor suggestions.
packages/desktop-client/e2e/page-models/mobile-budget-page.js (3)
276-283: Verify method name change fromopenEnvelopeBudgetSummaryMenutoopenEnvelopeBudgetSummaryThe method
openEnvelopeBudgetSummaryMenuhas been renamed toopenEnvelopeBudgetSummary. Ensure that all places where this method is called have been updated accordingly to avoid broken links or method not found errors.Run the following script to find any remaining references to
openEnvelopeBudgetSummaryMenu:✅ Verification successful
All references to
openEnvelopeBudgetSummaryMenuhave been successfully updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any remaining references to 'openEnvelopeBudgetSummaryMenu' rg --type js --fixed-strings 'openEnvelopeBudgetSummaryMenu'Length of output: 60
308-315: Verify method name change fromopenTrackingBudgetSummaryMenutoopenTrackingBudgetSummaryThe method
openTrackingBudgetSummaryMenuhas been renamed toopenTrackingBudgetSummary. Please verify that all existing calls to this method have been updated to reflect the new name to prevent any runtime errors.Run the following script to check for any remaining references to
openTrackingBudgetSummaryMenu:✅ Verification successful
Method Name Change Verified
All references to
openTrackingBudgetSummaryMenuhave been successfully updated toopenTrackingBudgetSummary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any remaining references to 'openTrackingBudgetSummaryMenu' rg --type js --fixed-strings 'openTrackingBudgetSummaryMenu'Length of output: 60
Script:
#!/bin/bash # Description: Find any remaining references to 'openTrackingBudgetSummaryMenu' rg --type js --fixed-strings 'openTrackingBudgetSummaryMenu'Length of output: 60
86-87: Verify method renaming for consistency across the codebaseThe method
waitForBudgetTablehas been renamed towaitFor. Please ensure that all references towaitForBudgetTablehave been updated towaitForto prevent any potential issues due to unresolved references.Run the following script to check for any remaining references to
waitForBudgetTable:✅ Verification successful
Method renaming verified: All references to
waitForBudgetTablehave been updated towaitFor.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any remaining references to 'waitForBudgetTable' rg --type js --fixed-strings 'waitForBudgetTable'Length of output: 49
Script:
#!/bin/bash # Description: Verify any remaining references to 'waitForBudgetTable' with context rg --type js --fixed-strings 'waitForBudgetTable' -A 2 -B 2Length of output: 59
| await expect(summary.getByText('Available Funds')).toBeVisible({ | ||
| timeout: 10000, | ||
| }); | ||
| await expect(summary.getByText('Available Funds')).toBeVisible(); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Timeout Configuration Across Test Files
The removal of the timeout: 10000 parameter from the toBeVisible() expectation in packages/desktop-client/e2e/budget.test.js is inconsistent with other test files that utilize custom timeouts:
packages/desktop-client/e2e/onboarding.test.jscontains multipletoBeVisible()calls with custom timeout values.
This inconsistency could lead to unexpected test behaviors and reduced stability. It's recommended to standardize timeout configurations across all end-to-end tests or ensure that each timeout adjustment is appropriately justified and documented.
🔗 Analysis chain
Removal of explicit timeout: Potential impact on test stability
The removal of the timeout: 10000 parameter from the toBeVisible() expectation could affect the test's behavior:
- It may make the test faster if the "Available Funds" text typically appears quickly.
- However, it could potentially make the test more brittle if the element sometimes takes longer to appear than the default timeout.
Given the concerns about flaky tests mentioned in the PR comments, this change warrants careful consideration.
Could you please clarify the reasoning behind removing this timeout? Was it causing issues, or is it part of a broader strategy to standardize timeouts?
To ensure this change doesn't negatively impact test stability, I recommend:
- Monitoring the test results closely after this change, especially in CI environments.
- If the change is intentional and based on specific insights, consider adding a comment explaining the rationale.
- If you haven't already, consider implementing retry logic for flaky tests to improve overall stability.
This will help us understand if this is an isolated change or part of a broader refactoring effort.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other custom timeouts in the test files
echo "Checking for other custom timeouts in test files:"
rg --type js 'toBeVisible\(\s*\{.*timeout:' packages/desktop-client/e2e
Length of output: 546
| async dragNavbarDown() { | ||
| const boundingBox = await this.page | ||
| .locator(this.navbarSelector) | ||
| .boundingBox(); | ||
|
|
||
| return new MobileAccountsPage(this.page); | ||
| await this.page.dragAndDrop(this.navbarSelector, this.navbarSelector, { | ||
| sourcePosition: { x: 1, y: 0 }, | ||
| targetPosition: { | ||
| x: 1, | ||
| // Only scroll until bottom of screen i.e. bottom of first navbar row. | ||
| y: boundingBox.height / MobileNavigation.#NAVBAR_ROWS, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM with suggestion: Well-implemented dragNavbarDown method
The dragNavbarDown method is well-implemented overall:
- It correctly calculates the drag distance based on the navbar's height and
#NAVBAR_ROWS. - The use of
page.dragAndDropaccurately simulates user interaction.
However, there's a potential improvement:
Consider adjusting the drag target to be a point just below the navbar instead of using the navbar itself as both source and target. This might produce a more reliable drag effect across different environments.
- await this.page.dragAndDrop(this.navbarSelector, this.navbarSelector, {
+ await this.page.dragAndDrop(this.navbarSelector, this.mainContentSelector, {
sourcePosition: { x: 1, y: 0 },
targetPosition: {
x: 1,
- y: boundingBox.height / MobileNavigation.#NAVBAR_ROWS,
+ y: boundingBox.height + 1, // Just below the navbar
},
});This change ensures that the drag operation moves the navbar to a position just below its current location.
Committable suggestion was skipped due to low confidence.
| const copyLastMonthBudget = async (budgetPage, categoryName) => { | ||
| const budgetMenuModal = await budgetPage.openBudgetMenu(categoryName); | ||
| await budgetMenuModal.copyLastMonthBudget(); | ||
| await budgetMenuModal.close(); | ||
| }; | ||
|
|
||
| const setTo3MonthAverage = async (budgetPage, categoryName) => { | ||
| const budgetMenuModal = await budgetPage.openBudgetMenu(categoryName); | ||
| await budgetMenuModal.setTo3MonthAverage(); | ||
| await budgetMenuModal.close(); | ||
| }; | ||
|
|
||
| const setTo6MonthAverage = async (budgetPage, categoryName) => { | ||
| const budgetMenuModal = await budgetPage.openBudgetMenu(categoryName); | ||
| await budgetMenuModal.setTo6MonthAverage(); | ||
| await budgetMenuModal.close(); | ||
| }; | ||
|
|
||
| const setToYearlyAverage = async (budgetPage, categoryName) => { | ||
| const budgetMenuModal = await budgetPage.openBudgetMenu(categoryName); | ||
| await budgetMenuModal.setToYearlyAverage(); | ||
| await budgetMenuModal.close(); | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication
The utility functions copyLastMonthBudget, setTo3MonthAverage, setTo6MonthAverage, and setToYearlyAverage share identical structures, leading to code duplication. To improve maintainability and reduce duplication, consider implementing a higher-order function:
const performBudgetMenuAction = async (budgetPage, categoryName, action) => {
const budgetMenuModal = await budgetPage.openBudgetMenu(categoryName);
await action(budgetMenuModal);
await budgetMenuModal.close();
};
const copyLastMonthBudget = (budgetPage, categoryName) =>
performBudgetMenuAction(budgetPage, categoryName, menu => menu.copyLastMonthBudget());
const setTo3MonthAverage = (budgetPage, categoryName) =>
performBudgetMenuAction(budgetPage, categoryName, menu => menu.setTo3MonthAverage());
// Similar for setTo6MonthAverage and setToYearlyAverageThis refactoring will make the code more maintainable and easier to extend in the future.
| async function setBudgetAverage( | ||
| budgetPage, | ||
| categoryName, | ||
| numberOfMonths, | ||
| setBudgetAverageFn, | ||
| ) { | ||
| let totalSpent = 0; | ||
|
|
||
| for (let i = 0; i < numberOfMonths; i++) { | ||
| await budgetPage.goToPreviousMonth(); | ||
| const spentButton = await budgetPage.getButtonForSpent(categoryName); | ||
| const spent = await spentButton.textContent(); | ||
| totalSpent += currencyToAmount(spent); | ||
| } | ||
|
|
||
| // Calculate average amount | ||
| const averageSpent = totalSpent / numberOfMonths; | ||
|
|
||
| // Go back to the current month | ||
| for (let i = 0; i < numberOfMonths; i++) { | ||
| await budgetPage.goToNextMonth(); | ||
| } | ||
|
|
||
| await setBudgetAverageFn(budgetPage, categoryName, numberOfMonths); | ||
|
|
||
| return averageSpent; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve retrieval of spent amounts
The setBudgetAverage function correctly calculates the average spent amount over the specified number of months. However, to ensure accurate data retrieval across month changes, consider moving the spentButton retrieval inside the loop:
async function setBudgetAverage(
budgetPage,
categoryName,
numberOfMonths,
setBudgetAverageFn,
) {
let totalSpent = 0;
for (let i = 0; i < numberOfMonths; i++) {
await budgetPage.goToPreviousMonth();
+ const spentButton = await budgetPage.getButtonForSpent(categoryName);
const spent = await spentButton.textContent();
totalSpent += currencyToAmount(spent);
}
// ... (rest of the function remains the same)
}This change ensures that you're accessing the correct spent amount for each month, even after the DOM has been updated due to month navigation.
📝 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.
| async function setBudgetAverage( | |
| budgetPage, | |
| categoryName, | |
| numberOfMonths, | |
| setBudgetAverageFn, | |
| ) { | |
| let totalSpent = 0; | |
| for (let i = 0; i < numberOfMonths; i++) { | |
| await budgetPage.goToPreviousMonth(); | |
| const spentButton = await budgetPage.getButtonForSpent(categoryName); | |
| const spent = await spentButton.textContent(); | |
| totalSpent += currencyToAmount(spent); | |
| } | |
| // Calculate average amount | |
| const averageSpent = totalSpent / numberOfMonths; | |
| // Go back to the current month | |
| for (let i = 0; i < numberOfMonths; i++) { | |
| await budgetPage.goToNextMonth(); | |
| } | |
| await setBudgetAverageFn(budgetPage, categoryName, numberOfMonths); | |
| return averageSpent; | |
| } | |
| async function setBudgetAverage( | |
| budgetPage, | |
| categoryName, | |
| numberOfMonths, | |
| setBudgetAverageFn, | |
| ) { | |
| let totalSpent = 0; | |
| for (let i = 0; i < numberOfMonths; i++) { | |
| await budgetPage.goToPreviousMonth(); | |
| const spentButton = await budgetPage.getButtonForSpent(categoryName); | |
| const spent = await spentButton.textContent(); | |
| totalSpent += currencyToAmount(spent); | |
| } | |
| // Calculate average amount | |
| const averageSpent = totalSpent / numberOfMonths; | |
| // Go back to the current month | |
| for (let i = 0; i < numberOfMonths; i++) { | |
| await budgetPage.goToNextMonth(); | |
| } | |
| await setBudgetAverageFn(budgetPage, categoryName, numberOfMonths); | |
| return averageSpent; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/e2e/page-models/mobile-navigation.js (4)
18-32: LGTM: Good use of static constants with a minor suggestionThe addition of static constants improves maintainability and readability. The use of private fields (#) is a good practice for encapsulation.
Consider adding JSDoc comments for these constants to provide more context about their usage, especially for
#NAVBAR_ROWSand#NAV_LINKS_HIDDEN_BY_DEFAULT.
52-64: LGTM: Well-implemented dragNavbarDown method with a suggestionThe
dragNavbarDownmethod is well-implemented:
- It correctly calculates the drag distance based on the navbar's height and
#NAVBAR_ROWS.- The use of
page.dragAndDropis consistent with thedragNavbarUpmethod.Consider adding a comment explaining the calculation of the
yvalue in thetargetPosition. While the current comment is helpful, a more detailed explanation of why dividing by#NAVBAR_ROWSachieves the desired effect would improve maintainability.
67-76: LGTM: Well-implemented hasNavbarState method with a suggestionThe
hasNavbarStatemethod is well-implemented:
- It handles the case when the navbar is not present.
- The use of
data-navbar-stateattribute is a good practice for state management.- The
expectassertion ensures the attribute exists.Consider adding error handling for the case where the
data-navbar-stateattribute exists but is empty. This could be done by checking ifdataNavbarStateis truthy before callingincludes.
79-108: LGTM: Well-implemented navigateToPage method with a suggestionThe
navigateToPagemethod is well-implemented:
- It checks the current URL to avoid unnecessary navigation.
- The handling of navbar visibility during navigation is comprehensive.
- The use of a factory function for creating page instances is a good practice for flexibility.
Consider adding a timeout or retry mechanism for the
link.click()operation, as network conditions or animations might sometimes delay the navigation. This would improve the robustness of the navigation process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/e2e/page-models/mobile-navigation.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
packages/desktop-client/e2e/page-models/mobile-navigation.js (6)
12-15: LGTM: Good use of roles for element selectionThe addition of
headingandnavbarproperties usinggetByRoleis a good practice for accessibility and maintainability. ThemainContentSelectorandnavbarSelectorprovide clear references to important page elements.
34-49: LGTM: Well-implemented dragNavbarUp methodThe
dragNavbarUpmethod is well-implemented:
- It correctly calculates drag positions based on bounding boxes.
- The use of
page.dragAndDropaccurately simulates user interaction.
111-116: LGTM: Well-refactored goToBudgetPage methodThe
goToBudgetPagemethod has been effectively refactored to use the newnavigateToPagemethod. This simplifies the code and reduces duplication across navigation methods.
118-130: LGTM: Consistent refactoring of navigation methodsThe
goToAccountsPageandgoToTransactionEntryPagemethods have been refactored consistently withgoToBudgetPage. This refactoring improves code consistency and maintainability across the class.
132-143: LGTM: Consistent refactoring of remaining navigation methodsThe
goToReportsPageandgoToSettingsPagemethods have been refactored consistently with the other navigation methods. This completes the refactoring of all navigation methods in the class, resulting in a more maintainable and consistent codebase.
1-143: Overall: Well-implemented refactoring and enhancementsThe changes to the
MobileNavigationclass significantly improve its structure, maintainability, and functionality. The introduction of thenavigateToPagemethod and the consistent refactoring of all navigation methods reduce code duplication and enhance readability. The new utility methods for navbar manipulation are well-implemented and provide robust functionality for mobile navigation testing.While some minor suggestions were made for further improvements, the overall quality of the changes is high. Great job on this refactoring effort!
| }); | ||
|
|
||
| test(`applies budget template`, async () => { | ||
| const settingsPage = await navigation.goToSettingsPage(); |
There was a problem hiding this comment.
The pipeline's saying this is flaky:
https://github.com/actualbudget/actual/actions/runs/11389100433/job/31687676786#step:5:397
No description provided.