[WIP] Mobile VRTs #3505
[WIP] Mobile VRTs #3505
Conversation
|
Warning Rate limit exceeded@joel-jeremy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces a comprehensive suite of end-to-end tests for mobile account and budgeting functionalities using Playwright. It includes new test files for accounts, budgeting, and settings, along with modifications to existing page models and components. Key changes involve the addition of new methods for managing budgets, improvements in accessibility through Changes
Possibly related PRs
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 (
|
✅ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (22)
packages/desktop-client/e2e/page-models/settings-page.js (2)
10-18: LGTM! Consider adding error handling and success verification.The
useBudgetTypemethod is well-structured and follows good practices. It effectively enables the experimental feature and switches to the specified budget type.Consider the following improvements:
- Add a check to see if the 'Budget mode toggle' is already enabled before calling
enableExperimentalFeature.- Implement error handling in case the switch button is not found.
- Add a verification step to ensure the budget type was successfully switched.
Here's a potential implementation:
async useBudgetType(budgetType) { if (!(await this.isExperimentalFeatureEnabled('Budget mode toggle'))) { await this.enableExperimentalFeature('Budget mode toggle'); } const switchBudgetTypeButton = this.page.getByRole('button', { name: `Switch to ${budgetType} budgeting`, }); if (await switchBudgetTypeButton.isVisible()) { await switchBudgetTypeButton.click(); // Verify the switch was successful const newBudgetTypeIndicator = this.page.getByText(`Using ${budgetType} budgeting`); await expect(newBudgetTypeIndicator).toBeVisible(); } else { throw new Error(`Button to switch to ${budgetType} budgeting not found`); } }This implementation assumes the existence of an
isExperimentalFeatureEnabledmethod, which you might need to add to the class.
20-32: LGTM! Consider adding error handling and improving toggle logic.The modifications to
enableExperimentalFeatureimprove its reliability by using more specific selectors. The use ofgetByTestIdfor buttons andgetByRolefor the checkbox is a good practice.Consider the following improvements:
- Add error handling in case elements are not found.
- Verify that the feature was successfully enabled after clicking the checkbox.
- Check the current state of the checkbox before clicking to ensure we're enabling (not disabling) the feature.
Here's a potential implementation:
async enableExperimentalFeature(featureName) { const advancedSettingsButton = this.page.getByTestId('advanced-settings'); await advancedSettingsButton.click(); const experimentalSettingsButton = this.page.getByTestId('experimental-settings'); await experimentalSettingsButton.click(); const featureCheckbox = this.page.getByRole('checkbox', { name: featureName }); // Check if the checkbox is already checked const isChecked = await featureCheckbox.isChecked(); if (!isChecked) { await featureCheckbox.click(); // Verify the checkbox is now checked await expect(featureCheckbox).toBeChecked(); } // Additional verification could be added here to ensure the feature is actually enabled }This implementation adds checks to ensure we're enabling the feature and verifies the checkbox state after clicking. You might also want to add try-catch blocks for error handling if necessary.
packages/desktop-client/e2e/settings.mobile.test.js (2)
6-22: Consider adjusting the viewport size for better mobile coverage.The test setup is well-structured and appropriate for mobile testing. However, the viewport size (350x600) seems quite small, even for mobile devices. Consider using a slightly larger size to cover more modern mobile devices, or implement multiple viewport sizes for better coverage.
For example, you could use:
await page.setViewportSize({ width: 375, height: 667, });This size corresponds to iPhone 6/7/8, which is a common reference point for mobile designs.
28-42: LGTM: Comprehensive test case with a suggestion for improvement.The test case effectively checks navigation to the settings page and the data export functionality. The use of visual regression testing and verification of the exported file name are good practices.
Suggestion for improvement: Consider adding a check for the contents of the exported file, if possible. This would ensure not just that a file is exported, but that it contains the expected data.
Example pseudo-code:
const fileContents = await readExportedFile(download); expect(fileContents).toContain(expectedData);This would provide an additional layer of validation for the export functionality.
packages/desktop-client/e2e/accounts.mobile.test.js (4)
1-26: LGTM! Consider extracting viewport size to a constant.The test setup is well-structured and follows best practices. The use of page models promotes code reusability, and the beforeEach and afterEach hooks ensure a clean state for each test.
Consider extracting the viewport size to a constant at the top of the file for easier maintenance:
const MOBILE_VIEWPORT = { width: 350, height: 600 }; // Then in beforeEach: await page.setViewportSize(MOBILE_VIEWPORT);
28-36: LGTM! Consider using a more robust method to select the account.The test effectively verifies the basic functionality of the accounts page, including both text content and visual appearance.
To improve test robustness, consider selecting the account by name instead of index:
const account = await accountsPage.getAccountByName('Ally Savings');This change would make the test less susceptible to failures if the order of accounts changes. You'll need to implement the
getAccountByNamemethod in youraccountsPagemodel.
38-46: LGTM! Consider improvements for robustness and specificity.The test setup is thorough and covers important aspects of the individual account page.
- Similar to the previous test, consider selecting the account by name for improved robustness:
const accountPage = await accountsPage.openAccountByName('Bank of America');
- For the balance check, consider using a more specific assertion if possible:
await expect(await accountPage.getBalance()).toEqual(expectedBalance);Where
expectedBalanceis a known value for this test account. This would make the test more sensitive to unexpected changes in the account balance.
53-56: LGTM! Consider adding more specific assertions.This part of the test effectively verifies that transactions are found when searching for 'Kroger'.
To make the test more robust and informative, consider adding more specific assertions:
- Check for a minimum number of expected transactions:
await expect(accountPage.transactions).toHaveCount(atLeast(1));
- Verify that the displayed transactions contain 'Kroger':
const transactions = await accountPage.getTransactions(); await expect(transactions.some(t => t.includes('Kroger'))).toBeTruthy();These additions would provide more confidence that the search is working as expected and returning relevant results.
packages/desktop-client/e2e/transactions.mobile.test.js (2)
6-6: Approve the updated test suite description with a minor suggestion.The change from "Mobile" to "Mobile Transactions" provides a more accurate description of the test suite's current focus. This aligns well with the streamlined set of tests.
Consider adding more specificity to the description, such as "Mobile Transaction Creation" to precisely reflect the current test coverage.
Line range hint
28-89: Approve the remaining transaction creation tests with a suggestion for enhancement.The two remaining tests for transaction creation are comprehensive and cover different entry points (via footer button and from an account page). They include appropriate checks for UI elements, data entry, and result verification.
To further enhance these tests, consider adding negative test cases or edge cases. For example:
- Attempting to create a transaction with invalid data (e.g., negative amounts, extremely large numbers).
- Testing the behavior when required fields are left empty.
- Verifying error messages for invalid inputs.
These additions would improve the robustness of the transaction creation tests.
packages/desktop-client/src/components/modals/EnvelopeBudgetMenuModal.tsx (2)
102-102: Approve the addition of data-testid attributeThe addition of the
data-testid="budget-amount"attribute to theFocusableAmountInputcomponent is a good practice. It enhances the testability of the component by providing a stable selector for end-to-end or integration tests.Consider using a more specific test ID to avoid potential conflicts in larger test suites. For example:
-data-testid="budget-amount" +data-testid="envelope-budget-amount-input"This change would make the test ID more unique and descriptive, potentially reducing the risk of selector conflicts in your test suite.
Line range hint
86-102: Enhance accessibility of the budget inputWhile the component is well-structured, we can improve its accessibility by adding an
aria-labelto theFocusableAmountInput. This will provide better context for screen readers.Consider adding an
aria-labelto theFocusableAmountInput:<FocusableAmountInput value={integerToAmount(budgeted || 0)} focused={amountFocused} onFocus={() => setAmountFocused(true)} onBlur={() => setAmountFocused(false)} onEnter={close} zeroSign="+" focusedStyle={{ width: 'auto', padding: '5px', paddingLeft: '20px', paddingRight: '20px', minWidth: '100%', }} textStyle={{ ...styles.veryLargeText, textAlign: 'center' }} onUpdateAmount={_onUpdateBudget} data-testid="budget-amount" + aria-label={`Budget amount for ${category.name}`} />This change will improve the user experience for those using screen readers by providing clear context about the input's purpose.
packages/desktop-client/src/components/modals/TrackingBudgetMenuModal.tsx (3)
102-102: Approve the addition of data-testid attributeThe addition of the
data-testid="budget-amount"attribute to theFocusableAmountInputcomponent is a good practice for improving testability. This will make it easier to select and interact with this specific element in automated tests.Consider using a more specific test ID to avoid potential conflicts in larger test suites. For example:
-data-testid="budget-amount" +data-testid="tracking-budget-modal-amount-input"This change would make the test ID more unique and descriptive of its location within the component hierarchy.
Line range hint
52-54: Consider adding type safety to _onUpdateBudget functionThe
_onUpdateBudgetfunction uses theanytype implicitly for itsamountparameter. To improve type safety, consider explicitly typing this parameter.Here's a suggested improvement:
- const _onUpdateBudget = (amount: number) => { + const _onUpdateBudget = (amount: number) => { onUpdateBudget?.(amountToInteger(amount)); };This change ensures that only number values can be passed to the function, preventing potential type-related bugs.
Line range hint
61-63: Add error handling for null categoryThe component returns null if the category is not found, but it doesn't provide any feedback to the user. Consider adding an error message or fallback UI for this case.
Here's a suggested improvement:
if (!category) { - return null; + return <Text>Error: Category not found</Text>; }This change provides feedback to the user when the category is not available, improving the user experience.
packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx (1)
111-111: Approved: Consistent capitalization in aria-labelThe capitalization of "Transaction list" in the
aria-labelattribute improves consistency in the labeling convention. This change, while minor, contributes to a more polished user interface.Consider using a more descriptive label that includes the number of transactions, such as "List of X transactions". This would provide users with more context about the content they're interacting with.
packages/desktop-client/e2e/page-models/mobile-budget-page.js (2)
17-35: Inconsistent selector usage intoggleVisibleColumnsmethod.In the
toggleVisibleColumnsmethod, theBudgetedbutton is selected usingthis.budgetTableHeader.getByRole(...), while theSpentbutton is selected usingthis.page.getByRole(...). For consistency and to ensure accurate scoping, consider usingthis.budgetTableHeader.getByRole(...)for theSpentbutton as well, unless there is a specific reason for the different scopes.Apply this change for consistency:
- const spentHeaderButton = this.page.getByRole('button', { name: 'Spent' }); + const spentHeaderButton = this.budgetTableHeader.getByRole('button', { name: 'Spent' });
66-72: Ensure input field focus before typing insetBudgetmethod.After clicking the budgeted button in
setBudget, ensure the input field is focused before typingnewAmount. This can prevent potential issues if the input does not automatically receive focus.You can explicitly focus the input field:
await budgetedButton.click(); + const inputField = this.page.getByRole('textbox'); + await inputField.focus(); await this.page.keyboard.type(String(newAmount)); await this.page.keyboard.press('Enter');packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx (2)
Line range hint
1-1: Enable TypeScript strict mode for better type safetyThe file begins with
// @ts-strict-ignore, which disables TypeScript's strict mode. Enabling strict mode can help catch type errors at compile time, enhancing code reliability and maintainability. It is advisable to address any existing type issues and remove this directive to benefit from stricter type checking.
123-137: Simplify thegetDefaultClassNamefunctionThe
getDefaultClassNamefunction wraps the result ofcss()withString(). Ifcss()already returns a string representing the class name, theString()conversion may be redundant. Consider removingString()to streamline the function unless there is a specific reason for this casting.packages/desktop-client/e2e/budget.mobile.test.js (2)
99-100: Ensure consistency between code and commentsThe comment
// Set to 100.00suggests setting the budget to100.00, but the code sets it to10000. To improve clarity, consider updating the comment or the code so they match. If10000represents100.00, consider using a helper function or constant to make this clear.Also applies to: 231-232
100-104: Clarify amount representation in budget settingWhen setting the budgeted amount using
await budgetPage.setBudget('Food', 10000);, it's not immediately clear that10000represents100.00. To enhance readability, consider defining a constant or using a helper function to represent monetary values, making the code more understandable.Also applies to: 232-236
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (103)
packages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-individual-account-page-and-checks-that-filtering-is-working-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-individual-account-page-and-checks-that-filtering-is-working-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-individual-account-page-and-checks-that-filtering-is-working-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-individual-account-page-and-checks-that-filtering-is-working-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-individual-account-page-and-checks-that-filtering-is-working-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-individual-account-page-and-checks-that-filtering-is-working-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-individual-account-page-and-checks-that-filtering-is-working-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-individual-account-page-and-checks-that-filtering-is-working-8-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-individual-account-page-and-checks-that-filtering-is-working-9-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-the-accounts-page-and-asserts-on-balances-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-the-accounts-page-and-asserts-on-balances-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-the-accounts-page-and-asserts-on-balances-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--14404-in-the-page-header-opens-the-month-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--321fd-ed-amount-opens-the-budget-summary-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--4bb70-ed-amount-opens-the-budget-summary-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--94a85-ed-amount-opens-the-budget-summary-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--bed18-in-the-page-header-opens-the-month-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--c18ad-l-redirects-to-the-category-transactions-page-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--ceb3a-in-the-page-header-opens-the-month-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--e995e-l-redirects-to-the-category-transactions-page-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--ff568-l-redirects-to-the-category-transactions-page-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-balance-button-opens-the-balance-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-balance-button-opens-the-balance-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-balance-button-opens-the-balance-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-loads-the-budget-page-with-budgeted-amounts-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-loads-the-budget-page-with-budgeted-amounts-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-loads-the-budget-page-with-budgeted-amounts-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-checks-that-clicking--11290-l-redirects-to-the-category-transactions-page-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--42062-in-the-page-header-opens-the-month-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--49fb6-in-the-page-header-opens-the-month-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--57d88-l-redirects-to-the-category-transactions-page-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--5d90c-l-redirects-to-the-category-transactions-page-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--730ef-ng-amount-opens-the-budget-summary-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--b1562-in-the-page-header-opens-the-month-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--c3fb8-ng-amount-opens-the-budget-summary-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--d9705-ng-amount-opens-the-budget-summary-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-balance-button-opens-the-balance-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-balance-button-opens-the-balance-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-balance-button-opens-the-balance-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-loads-the-budget-page-with-budgeted-amounts-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-loads-the-budget-page-with-budgeted-amounts-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-loads-the-budget-page-with-budgeted-amounts-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/budget.test.js-snapshots/Budget-renders-the-summary-information-available-funds-overspent-budgeted-and-for-next-month-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.test.js-snapshots/Budget-renders-the-summary-information-available-funds-overspent-budgeted-and-for-next-month-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-checks-that-settings-page-can-be-opened-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-checks-that-settings-page-can-be-opened-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-creates-a-transaction-from-accounts-id-page-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-creates-a-transaction-from-accounts-id-page-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-creates-a-transaction-from-accounts-id-page-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-creates-a-transaction-via-footer-button-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-opens-the-accounts-page-and-asserts-on-balances-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-opens-the-accounts-page-and-asserts-on-balances-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-opens-the-accounts-page-and-asserts-on-balances-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/onboarding.test.js-snapshots/Onboarding-checks-the-page-visuals-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-8-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!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-from-accounts-id-page-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-from-accounts-id-page-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-from-accounts-id-page-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-8-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-9-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-6-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3492.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
- packages/desktop-client/e2e/accounts.mobile.test.js (1 hunks)
- packages/desktop-client/e2e/budget.mobile.test.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-account-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-budget-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/settings-page.js (1 hunks)
- packages/desktop-client/e2e/settings.mobile.test.js (1 hunks)
- packages/desktop-client/e2e/transactions.mobile.test.js (1 hunks)
- packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx (4 hunks)
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (8 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx (2 hunks)
- packages/desktop-client/src/components/modals/EnvelopeBalanceMenuModal.tsx (1 hunks)
- packages/desktop-client/src/components/modals/EnvelopeBudgetMenuModal.tsx (1 hunks)
- packages/desktop-client/src/components/modals/TrackingBalanceMenuModal.tsx (1 hunks)
- packages/desktop-client/src/components/modals/TrackingBudgetMenuModal.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-client/e2e/page-models/mobile-account-page.js
🔇 Additional comments not posted (15)
packages/desktop-client/e2e/settings.mobile.test.js (2)
1-5: LGTM: Imports and setup look good.The necessary modules are imported from Playwright, and custom page models are used, which is a good practice for maintainability and reusability in end-to-end tests.
24-26: LGTM: Proper test teardown.Closing the page after each test is a good practice. It ensures proper cleanup of resources and helps maintain test isolation.
packages/desktop-client/e2e/accounts.mobile.test.js (2)
48-51: LGTM! Comprehensive check for the "no results" scenario.This part of the test effectively verifies the behavior when no transactions are found. It checks both UI elements (no transactions message) and data (transaction count), and includes a visual regression test.
1-57: Overall, well-structured and comprehensive test suite for mobile accounts.This test file effectively covers key functionality of the mobile accounts feature, including account balance display and transaction filtering. The use of page models, before/after hooks, and visual regression tests are commendable practices.
The suggested improvements, such as using more robust account selection methods and more specific assertions, will further enhance the reliability and effectiveness of these tests.
Great job on implementing these end-to-end tests for the mobile experience!
packages/desktop-client/e2e/transactions.mobile.test.js (1)
Line range hint
1-89: Clarify the rationale behind removing multiple tests.I've noticed that several tests have been removed from this suite, including those for budget page loading, accounts page opening, and settings page accessibility. While this focuses the suite on transaction creation, it also significantly reduces the overall test coverage.
Could you please clarify the reasoning behind these removals? Are these functionalities being tested elsewhere, or is there a specific strategy that led to this decision?
To help verify the current state of testing, please run the following script:
This will help us understand if the removed tests are covered in other files or if additional test coverage might be needed.
✅ Verification successful
Test coverage remains comprehensive with existing tests covering budget, account, and settings functionalities. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other test files that might cover the removed functionalities echo "Searching for other mobile test files:" fd -e js -e ts --full-path '.*mobile.*test' packages/desktop-client/e2e echo "\nSearching for budget-related tests:" rg -i 'test.*budget' packages/desktop-client/e2e echo "\nSearching for account-related tests:" rg -i 'test.*account' packages/desktop-client/e2e echo "\nSearching for settings-related tests:" rg -i 'test.*settings' packages/desktop-client/e2eLength of output: 7389
packages/desktop-client/src/components/modals/TrackingBalanceMenuModal.tsx (1)
67-67: Prop renamed for improved clarityThe prop
disabledhas been renamed toisDisabledin theBalanceWithCarryovercomponent. This change aligns with React's best practices for boolean prop naming, which suggests using "is" or "has" prefixes for boolean props to improve readability and self-documentation.To ensure consistency across the codebase, let's check if this change has been applied uniformly:
This script will help us identify if there are any inconsistencies in the usage of this prop across the project.
packages/desktop-client/src/components/modals/EnvelopeBalanceMenuModal.tsx (1)
69-69: Improved prop naming conventionThe change from
disabledtoisDisabledimproves readability and follows React best practices for boolean prop naming. This is a good improvement.To ensure consistency across the codebase, please run the following script to check for other occurrences of the
BalanceWithCarryovercomponent and verify if they need similar updates:Additionally, consider reviewing the hardcoded
truevalue forisDisabled. If this prop should be dynamic, you might want to pass a variable or a condition instead.✅ Verification successful
Prop renaming verified successfully
All instances of the
BalanceWithCarryovercomponent have been updated to use theisDisabledprop consistently. No occurrences of the olddisabledprop were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of BalanceWithCarryover component # and check for 'disabled' prop usage # Search for BalanceWithCarryover usage echo "Searching for BalanceWithCarryover usage:" rg --type typescript --type tsx "BalanceWithCarryover" -A 10 -B 2 # Search for the BalanceWithCarryover component definition echo "Searching for BalanceWithCarryover component definition:" rg --type typescript --type tsx "function BalanceWithCarryover" -A 20Length of output: 423
Script:
#!/bin/bash # Description: Find all occurrences of BalanceWithCarryover component # and check for 'isDisabled' prop usage with corrected file types # Search for BalanceWithCarryover usage in .ts and .tsx files echo "Searching for BalanceWithCarryover usage in TypeScript files:" rg --type ts --type tsx "BalanceWithCarryover" -A 10 -B 2 # Search for the BalanceWithCarryover component definition in .ts and .tsx files echo "Searching for BalanceWithCarryover component definition in TypeScript files:" rg --type ts --type tsx "function BalanceWithCarryover" -A 20Length of output: 473
Script:
#!/bin/bash # Description: Find all occurrences of BalanceWithCarryover component # and check for 'isDisabled' prop usage in .ts and .tsx files # Search for BalanceWithCarryover usage in .ts and .tsx files echo "Searching for BalanceWithCarryover usage in TypeScript files:" rg "BalanceWithCarryover" -g "*.ts" -g "*.tsx" -A 10 -B 2 # Search for the BalanceWithCarryover component definition in .ts and .tsx files echo "Searching for BalanceWithCarryover component definition in TypeScript files:" rg "function BalanceWithCarryover" -g "*.ts" -g "*.tsx" -A 20Length of output: 21496
packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (1)
401-401: LGTM! Verify BalanceWithCarryover component for consistency.The change from
disabledtoisDisabledaligns with React naming conventions for boolean props. This improves code readability and consistency.To ensure this change doesn't introduce any issues, please verify that the
BalanceWithCarryovercomponent has been updated to expectisDisabledinstead ofdisabled. Run the following script to check for any remaining usage of thedisabledprop in theBalanceWithCarryovercomponent:✅ Verification successful
Verification Successful: The
disabledprop has been fully replaced withisDisabledin theBalanceWithCarryovercomponent. No remaining instances ofdisabledwere found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for 'disabled' prop usage in BalanceWithCarryover component # Search for the BalanceWithCarryover component definition echo "Searching for BalanceWithCarryover component definition:" rg --type typescript "export (const|function) BalanceWithCarryover" -A 10 # Search for any remaining usage of 'disabled' prop within the component echo "\nSearching for 'disabled' prop usage in BalanceWithCarryover:" rg --type typescript "disabled(\s*):(\s*)" -g "*BalanceWithCarryover*"Length of output: 482
Script:
#!/bin/bash # Description: Check for 'disabled' prop usage in BalanceWithCarryover component # Search for the BalanceWithCarryover component definition echo "Searching for BalanceWithCarryover component definition in .tsx files:" rg "export (const|function) BalanceWithCarryover" -g "*.tsx" -A 10 # Search for any remaining usage of 'disabled' prop within the component echo "\nSearching for 'disabled' prop usage in BalanceWithCarryover within .tsx files:" rg "disabled\s*=" -g "*.tsx" | grep "BalanceWithCarryover"Length of output: 1472
packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx (2)
94-94: Excellent addition for accessibility!The
aria-label="Loading..."attribute enhances the user experience for screen reader users by providing clear feedback when the content is loading. This change aligns with web accessibility best practices.
Line range hint
1-489: Overall assessment: Well-structured component with improved accessibilityThe
TransactionListcomponent is well-organized and follows React best practices. The recent changes focus on enhancing accessibility, which is commendable. The component handles complex logic for rendering transactions and user interactions effectively.Consider adding more accessibility features in the future, such as keyboard navigation support and ARIA attributes for interactive elements within the transaction list.
packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (4)
282-286: Improved testability with data-testid attributesThe addition of
data-testidattributes to the CellValue components and buttons in the ExpenseCategory component is a positive change. These unique identifiers will greatly enhance the ability to select specific elements during testing, making the tests more robust and easier to maintain.Also applies to: 303-303, 604-608, 616-616
1924-1924: Enhanced accessibility and testability in MonthSelectorThe addition of
aria-labelattributes to the previous and next month buttons in the MonthSelector component is a great improvement for accessibility. This change allows screen readers to properly announce the purpose of these buttons. Additionally, thedata-testidattribute added to the month button enhances testability. These modifications contribute to a more inclusive and testable user interface.Also applies to: 1946-1946, 1953-1953
1702-1702: Improved testability for BudgetTableHeaderThe addition of a
data-testidattribute to the main View component of the BudgetTableHeader is a positive change. This unique identifier will make it easier to select and interact with the BudgetTableHeader component during testing, enhancing the overall testability of the component.
Line range hint
1-1965: Overall improvements in testability and accessibilityThe changes made to this file consistently enhance both testability and accessibility across multiple components. The addition of
data-testidattributes improves the ability to select specific elements during testing, while the newaria-labelattributes in the MonthSelector component improve screen reader support. These modifications contribute to a more robust, maintainable, and inclusive codebase without altering the core functionality of the components.packages/desktop-client/e2e/page-models/mobile-budget-page.js (1)
12-14: Initialization of new properties is appropriate.The properties
budgetTableHeader,budgetTable, andnavigationare correctly initialized in the constructor, enhancing the page model's capabilities for budget interactions.
| @@ -99,6 +99,7 @@ export function TrackingBudgetMenuModal({ | |||
| }} | |||
| textStyle={{ ...styles.veryLargeText, textAlign: 'center' }} | |||
| onUpdateAmount={_onUpdateBudget} | |||
| data-testid="budget-amount" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility with aria-label
While adding the data-testid attribute is great for testing, consider also adding an aria-label to improve accessibility for screen readers.
Here's a suggested improvement:
data-testid="budget-amount"
+ aria-label={`Budget amount for ${category.name}`}This change provides more context to screen reader users about the purpose of this input field.
📝 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.
| data-testid="budget-amount" | |
| data-testid="budget-amount" | |
| aria-label={`Budget amount for ${category.name}`} |
| async openEnvelopeBudgetSummaryMenu() { | ||
| const toBudgetButton = this.page.getByRole('button', { name: 'To Budget' }); | ||
|
|
||
| if ((await toBudgetButton.count()) > 0) { | ||
| await toBudgetButton.click(); | ||
| return; | ||
| } | ||
|
|
||
| const overbudgetedButton = this.page.getByRole('button', { | ||
| name: 'Overbudgeted', | ||
| }); | ||
|
|
||
| if ((await overbudgetedButton.count()) > 0) { | ||
| await overbudgetedButton.click(); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle the case when no summary buttons are found in openEnvelopeBudgetSummaryMenu.
If neither the 'To Budget' nor 'Overbudgeted' buttons are found, the method exits silently without any action. Adding error handling will assist in identifying unexpected UI changes.
Consider adding an error throw:
...
if ((await overbudgetedButton.count()) > 0) {
await overbudgetedButton.click();
return;
}
+ throw new Error('No summary button found in openEnvelopeBudgetSummaryMenu');
}📝 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 openEnvelopeBudgetSummaryMenu() { | |
| const toBudgetButton = this.page.getByRole('button', { name: 'To Budget' }); | |
| if ((await toBudgetButton.count()) > 0) { | |
| await toBudgetButton.click(); | |
| return; | |
| } | |
| const overbudgetedButton = this.page.getByRole('button', { | |
| name: 'Overbudgeted', | |
| }); | |
| if ((await overbudgetedButton.count()) > 0) { | |
| await overbudgetedButton.click(); | |
| return; | |
| } | |
| } | |
| async openEnvelopeBudgetSummaryMenu() { | |
| const toBudgetButton = this.page.getByRole('button', { name: 'To Budget' }); | |
| if ((await toBudgetButton.count()) > 0) { | |
| await toBudgetButton.click(); | |
| return; | |
| } | |
| const overbudgetedButton = this.page.getByRole('button', { | |
| name: 'Overbudgeted', | |
| }); | |
| if ((await overbudgetedButton.count()) > 0) { | |
| await overbudgetedButton.click(); | |
| return; | |
| } | |
| throw new Error('No summary button found in openEnvelopeBudgetSummaryMenu'); | |
| } |
| async openTrackingBudgetSummaryMenu() { | ||
| const savedButton = this.page.getByRole('button', { name: 'Saved' }); | ||
|
|
||
| if ((await savedButton.count()) > 0) { | ||
| await savedButton.click(); | ||
| return; | ||
| } | ||
|
|
||
| const projectedSavingsButton = this.page.getByRole('button', { | ||
| name: 'Projected Savings', | ||
| }); | ||
|
|
||
| if ((await projectedSavingsButton.count()) > 0) { | ||
| await projectedSavingsButton.click(); | ||
| return; | ||
| } | ||
|
|
||
| const overspentButton = this.page.getByRole('button', { | ||
| name: 'Overspent', | ||
| }); | ||
|
|
||
| if ((await overspentButton.count()) > 0) { | ||
| await overspentButton.click(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Handle the case when no summary buttons are found in openTrackingBudgetSummaryMenu.
Similarly, in openTrackingBudgetSummaryMenu, if none of the buttons ('Saved', 'Projected Savings', 'Overspent') are found, the method does nothing. Adding error handling will help in debugging and ensuring the method behaves as expected.
Consider adding an error throw:
...
if ((await overspentButton.count()) > 0) {
await overspentButton.click();
return;
}
+ throw new Error('No summary button found in openTrackingBudgetSummaryMenu');
}📝 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 openTrackingBudgetSummaryMenu() { | |
| const savedButton = this.page.getByRole('button', { name: 'Saved' }); | |
| if ((await savedButton.count()) > 0) { | |
| await savedButton.click(); | |
| return; | |
| } | |
| const projectedSavingsButton = this.page.getByRole('button', { | |
| name: 'Projected Savings', | |
| }); | |
| if ((await projectedSavingsButton.count()) > 0) { | |
| await projectedSavingsButton.click(); | |
| return; | |
| } | |
| const overspentButton = this.page.getByRole('button', { | |
| name: 'Overspent', | |
| }); | |
| if ((await overspentButton.count()) > 0) { | |
| await overspentButton.click(); | |
| return; | |
| } | |
| async openTrackingBudgetSummaryMenu() { | |
| const savedButton = this.page.getByRole('button', { name: 'Saved' }); | |
| if ((await savedButton.count()) > 0) { | |
| await savedButton.click(); | |
| return; | |
| } | |
| const projectedSavingsButton = this.page.getByRole('button', { | |
| name: 'Projected Savings', | |
| }); | |
| if ((await projectedSavingsButton.count()) > 0) { | |
| await projectedSavingsButton.click(); | |
| return; | |
| } | |
| const overspentButton = this.page.getByRole('button', { | |
| name: 'Overspent', | |
| }); | |
| if ((await overspentButton.count()) > 0) { | |
| await overspentButton.click(); | |
| return; | |
| } | |
| throw new Error('No summary button found in openTrackingBudgetSummaryMenu'); | |
| } |
| async getBudgetedButton(categoryName) { | ||
| let budgetedButton = this.page.getByTestId( | ||
| `budgeted-${categoryName}-button`, | ||
| ); | ||
|
|
||
| if ((await budgetedButton.count()) > 0) { | ||
| return budgetedButton; | ||
| } | ||
|
|
||
| await this.toggleVisibleColumns(); | ||
| budgetedButton = await this.getBudgetedButton(categoryName); | ||
|
|
||
| if ( | ||
| (await budgetedButton.count()) > 0 && | ||
| (await budgetedButton.isVisible()) | ||
| ) { | ||
| return budgetedButton; | ||
| } | ||
|
|
||
| throw new Error( | ||
| `Budget button for category ${categoryName} could not be located`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor recursion in getBudgetedButton method to an iterative approach.
The getBudgetedButton method uses recursion after toggling columns if the button is not found initially. Refactoring to an iterative approach can enhance readability and prevent potential stack overflow issues.
Apply this refactor:
async getBudgetedButton(categoryName) {
let budgetedButton = this.page.getByTestId(`budgeted-${categoryName}-button`);
+ for (let attempts = 0; attempts < 2; attempts++) {
+ if ((await budgetedButton.count()) > 0 && (await budgetedButton.isVisible())) {
+ return budgetedButton;
+ }
+ await this.toggleVisibleColumns();
+ budgetedButton = this.page.getByTestId(`budgeted-${categoryName}-button`);
+ }
throw new Error(
`Budget button for category ${categoryName} could not be located`,
);
}📝 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 getBudgetedButton(categoryName) { | |
| let budgetedButton = this.page.getByTestId( | |
| `budgeted-${categoryName}-button`, | |
| ); | |
| if ((await budgetedButton.count()) > 0) { | |
| return budgetedButton; | |
| } | |
| await this.toggleVisibleColumns(); | |
| budgetedButton = await this.getBudgetedButton(categoryName); | |
| if ( | |
| (await budgetedButton.count()) > 0 && | |
| (await budgetedButton.isVisible()) | |
| ) { | |
| return budgetedButton; | |
| } | |
| throw new Error( | |
| `Budget button for category ${categoryName} could not be located`, | |
| ); | |
| } | |
| async getBudgetedButton(categoryName) { | |
| let budgetedButton = this.page.getByTestId(`budgeted-${categoryName}-button`); | |
| for (let attempts = 0; attempts < 2; attempts++) { | |
| if ((await budgetedButton.count()) > 0 && (await budgetedButton.isVisible())) { | |
| return budgetedButton; | |
| } | |
| await this.toggleVisibleColumns(); | |
| budgetedButton = this.page.getByTestId(`budgeted-${categoryName}-button`); | |
| } | |
| throw new Error( | |
| `Budget button for category ${categoryName} could not be located`, | |
| ); | |
| } |
| const getDifferenceToGoal = useCallback( | ||
| (balanceValue: number) => | ||
| longGoalValue === 1 | ||
| ? balanceValue - goalValue | ||
| : budgetedValue - goalValue, |
There was a problem hiding this comment.
Ensure safe handling of goalValue when it may be null or undefined
In the getDifferenceToGoal function, calculations involve goalValue, which might be null or undefined. If goalValue is null, operations like subtraction could result in NaN, leading to unexpected behavior in comparisons and rendering. Consider adding checks to ensure goalValue is a valid number before performing arithmetic operations.
| test.beforeEach(async ({ browser }) => { | ||
| page = await browser.newPage(); | ||
| navigation = new MobileNavigation(page); | ||
| configurationPage = new ConfigurationPage(page); | ||
|
|
||
| await page.setViewportSize({ | ||
| width: 350, | ||
| height: 600, | ||
| }); | ||
| await page.goto('/'); | ||
| await configurationPage.createTestFile(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor shared setup code to eliminate duplication
The beforeEach functions in both test suites are nearly identical. Consider abstracting the common setup steps into a shared function or utilizing test hooks to reduce code duplication and improve maintainability.
Also applies to: 131-142
| test.afterEach(async () => { | ||
| await page.close(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor shared teardown code to eliminate duplication
Similar to the setup functions, the afterEach functions in both test suites perform the same actions. Abstracting this into a shared teardown function can further reduce duplication.
Also applies to: 144-146
| await expect(budgetPage.categoryNames).toHaveText([ | ||
| 'Food', | ||
| 'Restaurants', | ||
| 'Entertainment', | ||
| 'Clothing', | ||
| 'General', | ||
| 'Gift', | ||
| 'Medical', | ||
| 'Savings', | ||
| 'Cell', | ||
| 'Internet', | ||
| 'Mortgage', | ||
| 'Water', | ||
| 'Power', | ||
| 'Starting Balances', | ||
| 'Misc', | ||
| 'Income', | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding category names in tests
Hardcoding category names makes the tests fragile to changes in the category list. Consider retrieving category names dynamically or using a fixture or test data file. This will make the tests more resilient to changes and easier to maintain.
Also applies to: 153-170
| }); | ||
| await page.goto('/'); | ||
| await configurationPage.createTestFile(); | ||
| }); | ||
|
|
||
| test.afterEach(async () => { | ||
| await page.close(); | ||
| }); | ||
|
|
||
| test('loads the budget page with budgeted amounts', async () => { | ||
| const budgetPage = await navigation.goToBudgetPage(); | ||
|
|
||
| await expect(budgetPage.categoryNames).toHaveText([ | ||
| 'Food', | ||
| 'Restaurants', | ||
| 'Entertainment', | ||
| 'Clothing', | ||
| 'General', | ||
| 'Gift', | ||
| 'Medical', | ||
| 'Savings', | ||
| 'Cell', | ||
| 'Internet', | ||
| 'Mortgage', | ||
| 'Water', | ||
| 'Power', | ||
| 'Starting Balances', | ||
| 'Misc', | ||
| 'Income', | ||
| ]); | ||
| await expect(page).toMatchThemeScreenshots(); | ||
| }); | ||
|
|
||
| test('checks that clicking the budgeted cell opens the budget menu modal', async () => { | ||
| const budgetPage = await navigation.goToBudgetPage(); | ||
| await expect(budgetPage.budgetTable).toBeVisible(); | ||
|
|
||
| await budgetPage.openBudgetMenu('Food'); | ||
|
|
||
| const budgetMenuModal = page.getByTestId('envelope-budget-menu-modal'); | ||
| await expect(budgetMenuModal).toBeVisible(); | ||
| await expect(page).toMatchThemeScreenshots(); | ||
| }); | ||
|
|
||
| test('checks that clicking spent cell redirects to the category transactions page', async () => { | ||
| const budgetPage = await navigation.goToBudgetPage(); | ||
| await expect(budgetPage.budgetTable).toBeVisible(); | ||
|
|
||
| const accountPage = await budgetPage.openSpentPage('Food'); | ||
|
|
||
| await expect(accountPage.transactionList).toBeVisible(); | ||
| await expect(page).toMatchThemeScreenshots(); | ||
| }); | ||
|
|
||
| test('checks that clicking the balance button opens the balance menu modal', async () => { | ||
| const budgetPage = await navigation.goToBudgetPage(); | ||
| await expect(budgetPage.budgetTable).toBeVisible(); | ||
|
|
||
| await budgetPage.openBalanceMenu('Food'); | ||
|
|
||
| const balanceMenuModal = page.getByTestId('envelope-balance-menu-modal'); | ||
| await expect(balanceMenuModal).toBeVisible(); | ||
| await expect(page).toMatchThemeScreenshots(); | ||
| }); | ||
|
|
||
| test('checks that clicking the month in the page header opens the month menu modal', async () => { | ||
| const budgetPage = await navigation.goToBudgetPage(); | ||
| await expect(budgetPage.budgetTable).toBeVisible(); | ||
|
|
||
| await budgetPage.openMonthMenu(); | ||
|
|
||
| const monthMenuModal = page.getByTestId('envelope-budget-month-menu-modal'); | ||
| await expect(monthMenuModal).toBeVisible(); | ||
| await expect(page).toMatchThemeScreenshots(); | ||
| }); | ||
|
|
||
| test('updates the budgeted amount', async () => { | ||
| const budgetPage = await navigation.goToBudgetPage(); | ||
| await expect(budgetPage.budgetTable).toBeVisible(); | ||
|
|
||
| // Set to 100.00 | ||
| await budgetPage.setBudget('Food', 10000); | ||
|
|
||
| const budgetedButton = await budgetPage.getBudgetedButton('Food'); | ||
|
|
||
| await expect(budgetedButton).toHaveText('100.00'); | ||
|
|
||
| await expect(page).toMatchThemeScreenshots(); | ||
| }); | ||
|
|
||
| test('checks that clicking the To Budget/Overbudgeted amount opens the budget summary menu modal', async () => { | ||
| const budgetPage = await navigation.goToBudgetPage(); | ||
|
|
||
| await budgetPage.openEnvelopeBudgetSummaryMenu(); | ||
|
|
||
| const summaryModal = page.getByTestId('envelope-budget-summary-modal'); | ||
| await expect(summaryModal).toBeVisible(); | ||
| await expect(page).toMatchThemeScreenshots(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize tests to handle multiple budget types
The test suites for 'Mobile Budget [Envelope]' and 'Mobile Budget [Tracking]' are very similar, with differences primarily in the budget type. Consider parameterizing the tests to handle both budget types within a single suite. This will reduce code duplication and simplify future test maintenance.
Also applies to: 120-253
| const setBudgetType = async budgetType => { | ||
| // Set budget type. | ||
| const settingsPage = await navigation.goToSettingsPage(); | ||
| await settingsPage.useBudgetType(budgetType); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving utility functions outside the test suite
The setBudgetType function is defined within the 'Mobile Budget [Tracking]' test suite but may be useful elsewhere. Consider moving it outside the suite or into a utility module if it's needed in other tests, promoting reusability.
…over.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
joel-jeremy
force-pushed
the
mobile-vrts
branch
from
760988f to
1bc77b5
Compare
No description provided.