Fix mobile balance modal colors #3492
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe pull request introduces changes primarily focused on the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. |
joel-jeremy
changed the title
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: 2
Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/modals/TrackingBalanceMenuModal.tsx (1)
Line range hint
1-105: Work in Progress: Incomplete implementationThis PR is marked as WIP (Work in Progress), which explains the discrepancy between the current changes and the stated objectives. The prop rename is a good improvement, but it doesn't address the color issues mentioned in the PR title.
Next steps:
- Implement the color-related changes for the mobile balance modal.
- Update the PR description to accurately reflect all changes made.
- Remove the WIP status once the implementation is complete.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
upcoming-release-notes/3492.mdis excluded by!**/*.md
Files selected for processing (4)
- 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/modals/EnvelopeBalanceMenuModal.tsx (1 hunks)
- packages/desktop-client/src/components/modals/TrackingBalanceMenuModal.tsx (1 hunks)
Additional comments not posted (5)
packages/desktop-client/src/components/modals/TrackingBalanceMenuModal.tsx (1)
67-67: Prop name change fromdisabledtoisDisabledThe prop name change from
disabledtoisDisabledimproves the naming convention by making it more explicit. This change aligns with common React practices where boolean props often start with "is" or "has".To ensure consistency across the codebase, let's check for other occurrences of the
disabledprop inBalanceWithCarryovercomponent:packages/desktop-client/src/components/modals/EnvelopeBalanceMenuModal.tsx (1)
69-69: Prop name change fromdisabledtoisDisabledThe change from
disabledtoisDisabledaligns with React's naming convention for boolean props. This improves code readability and consistency.To ensure this change is consistent across the codebase and doesn't introduce any issues, let's verify the usage of the
BalanceWithCarryovercomponent:Verification successful
Prop name change from
disabledtoisDisabledverified successfullyAll instances of the
disabledprop have been successfully replaced withisDisabledin theBalanceWithCarryovercomponent across the codebase. The component definitions are correctly updated, ensuring consistency and adherence to React naming conventions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of 'disabled' prop in BalanceWithCarryover # and verify the correct usage of 'isDisabled' # Search for BalanceWithCarryover usage echo "Searching for BalanceWithCarryover usage:" rg --type typescript --type tsx "<BalanceWithCarryover" -A 10 -B 2 # Search specifically for 'disabled' prop echo "\nChecking for any remaining 'disabled' prop usage:" rg --type typescript --type tsx "<BalanceWithCarryover[^>]*disabled\s*[={\[]" -A 2 -B 2 # Search for the new 'isDisabled' prop usage echo "\nVerifying 'isDisabled' prop usage:" rg --type typescript --type tsx "<BalanceWithCarryover[^>]*isDisabled\s*[={\[]" -A 2 -B 2 # Check BalanceWithCarryover component definition echo "\nChecking BalanceWithCarryover component definition:" rg --type typescript --type tsx "interface BalanceWithCarryoverProps|type BalanceWithCarryoverProps" -A 10Length of output: 917
Script:
#!/bin/bash # Description: Check for any remaining usage of 'disabled' prop in BalanceWithCarryover # and verify the correct usage of 'isDisabled' # Search for BalanceWithCarryover usage echo "Searching for BalanceWithCarryover usage:" rg --type ts --type tsx "<BalanceWithCarryover" -A 10 -B 2 # Search specifically for 'disabled' prop echo "\nChecking for any remaining 'disabled' prop usage:" rg --type ts --type tsx "<BalanceWithCarryover[^>]*disabled\s*[={\[]" -A 2 -B 2 # Search for the new 'isDisabled' prop usage echo "\nVerifying 'isDisabled' prop usage:" rg --type ts --type tsx "<BalanceWithCarryover[^>]*isDisabled\s*[={\[]" -A 2 -B 2 # Check BalanceWithCarryover component definition echo "\nChecking BalanceWithCarryover component definition:" rg --type ts --type tsx "interface BalanceWithCarryoverProps|type BalanceWithCarryoverProps" -A 10Length of output: 857
Script:
#!/bin/bash # Description: Check for any remaining usage of 'disabled' prop in BalanceWithCarryover # and verify the correct usage of 'isDisabled' # Search for BalanceWithCarryover usage echo "Searching for BalanceWithCarryover usage:" rg "<BalanceWithCarryover" -g '*.ts' -g '*.tsx' -A 10 -B 2 # Search specifically for 'disabled' prop echo -e "\nChecking for any remaining 'disabled' prop usage:" rg "<BalanceWithCarryover[^>]*disabled\s*[={\[]" -g '*.ts' -g '*.tsx' -A 2 -B 2 # Search for the new 'isDisabled' prop usage echo -e "\nVerifying 'isDisabled' prop usage:" rg "<BalanceWithCarryover[^>]*isDisabled\s*[={\[]" -g '*.ts' -g '*.tsx' -A 2 -B 2 # Check BalanceWithCarryover component definition echo -e "\nChecking BalanceWithCarryover component definition:" rg "interface BalanceWithCarryoverProps|type BalanceWithCarryoverProps" -g '*.ts' -g '*.tsx' -A 10Length of output: 8249
packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (1)
401-401: Improved prop naming conventionThe change from
disabledtoisDisabledaligns better with React's boolean prop naming convention. This improves code readability and maintainability.To ensure this change is consistent across the codebase, please run the following script:
This will help verify that:
- All other usages of
BalanceWithCarryoverhave been updated to useisDisabledinstead ofdisabled.- The
BalanceWithCarryovercomponent definition has been updated to acceptisDisabledprop instead ofdisabled.Verification successful
Shell Script Correction Needed
The previous shell script encountered errors due to incorrect
--typevalues. Please use the updated script below to verify the changes:This corrected script uses the appropriate
--typearguments (tsfor TypeScript andtsxfor TSX files) to ensure Ripgrep correctly identifies and searches the desired file types.
Adjusted Shell Script for Accurate Searching
The previous shell scripts encountered errors due to unrecognized
--typearguments. Please use the updated script below, which employs glob patterns to target.tsand.tsxfiles directly:This revised script uses the
-gflag with glob patterns to explicitly search within.tsand.tsxfiles, bypassing the issues related to unrecognized file types. Please execute this script to continue the verification process.
Verify All Usages of
BalanceWithCarryoverUseisDisabledPropTo ensure that the
disabledprop has been consistently renamed toisDisabledacross all instances of theBalanceWithCarryovercomponent, please execute the following script:This script searches for any occurrences of the
BalanceWithCarryovercomponent that still use thedisabledprop within.tsand.tsxfiles. If any results are returned, those instances need to be updated to useisDisabledinstead.
Consistent Renaming of
disabledtoisDisabledConfirmedAll instances of the
BalanceWithCarryovercomponent have been updated to use theisDisabledprop. No remaining usages of the olddisabledprop were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of BalanceWithCarryover component rg "BalanceWithCarryover" --type typescript --type tsx # Search for the definition of BalanceWithCarryover component rg "function BalanceWithCarryover" --type typescript --type tsxLength of output: 190
Script:
# #!/bin/bash # Search for other occurrences of BalanceWithCarryover component rg "BalanceWithCarryover" --type ts --type tsx # Search for the definition of BalanceWithCarryover component rg "function BalanceWithCarryover" --type ts --type tsxLength of output: 160
Script:
# #!/bin/bash # Search for other occurrences of the BalanceWithCarryover component in .ts and .tsx files rg "BalanceWithCarryover" -g "*.ts" -g "*.tsx" # Search for the definition of the BalanceWithCarryover component in .ts and .tsx files rg "function BalanceWithCarryover" -g "*.ts" -g "*.tsx"Length of output: 1937
Script:
# #!/bin/bash # Search for BalanceWithCarryover components using the old 'disabled' prop in .ts and .tsx files rg 'BalanceWithCarryover\s+[^>]*disabled=' -g "*.ts" -g "*.tsx"Length of output: 65
packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx (2)
64-71: Type definitions are well-structured and correct.The new type definitions for
CellValueChildrenandChildrenWithClassNameenhance type safety and improve code maintainability.
74-74: Props updated appropriately to reflect new 'children' type.The
BalanceWithCarryoverPropsare correctly updated to include the newchildren?: ChildrenWithClassNameprop and omit'children' | 'binding'from the original component props.Also applies to: 76-76
| (balanceValue: number) => | ||
| String( | ||
| css({ | ||
| ...getBalanceAmountStyle(balanceValue), | ||
| overflow: 'hidden', | ||
| textOverflow: 'ellipsis', | ||
| textAlign: 'right', | ||
| ...(!isDisabled && { | ||
| cursor: 'pointer', | ||
| }), | ||
| ':hover': { textDecoration: 'underline' }, | ||
| }), | ||
| ), | ||
| [getBalanceAmountStyle, isDisabled], |
There was a problem hiding this comment.
Remove unnecessary String() wrapping around css().
The css() function already returns a string representing the class name. Wrapping it with String() is redundant.
Apply this diff to simplify the code:
- (balanceValue: number) =>
- String(
- css({
- ...getBalanceAmountStyle(balanceValue),
- overflow: 'hidden',
- textOverflow: 'ellipsis',
- textAlign: 'right',
- ...(!isDisabled && {
- cursor: 'pointer',
- }),
- ':hover': { textDecoration: 'underline' },
- }),
- ),
+ (balanceValue: number) =>
+ css({
+ ...getBalanceAmountStyle(balanceValue),
+ overflow: 'hidden',
+ textOverflow: 'ellipsis',
+ textAlign: 'right',
+ ...(!isDisabled && {
+ cursor: 'pointer',
+ }),
+ ':hover': { textDecoration: 'underline' },
+ }),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.
| (balanceValue: number) => | |
| String( | |
| css({ | |
| ...getBalanceAmountStyle(balanceValue), | |
| overflow: 'hidden', | |
| textOverflow: 'ellipsis', | |
| textAlign: 'right', | |
| ...(!isDisabled && { | |
| cursor: 'pointer', | |
| }), | |
| ':hover': { textDecoration: 'underline' }, | |
| }), | |
| ), | |
| [getBalanceAmountStyle, isDisabled], | |
| (balanceValue: number) => | |
| css({ | |
| ...getBalanceAmountStyle(balanceValue), | |
| overflow: 'hidden', | |
| textOverflow: 'ellipsis', | |
| textAlign: 'right', | |
| ...(!isDisabled && { | |
| cursor: 'pointer', | |
| }), | |
| ':hover': { textDecoration: 'underline' }, | |
| }), | |
| [getBalanceAmountStyle, isDisabled], |
There was a problem hiding this comment.
glamor's css function returns a StyleAttribute, not a string
There was a problem hiding this comment.
@joel-jeremy, thank you for the clarification. You're absolutely right—glamor's css() function returns a StyleAttribute, not a string. My apologies for the oversight.
(·•·)?
✏️ Learnings added
Learnt from: joel-jeremy
PR: actualbudget/actual#3492
File: packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx:123-136
Timestamp: 2024-09-25T21:00:54.812Z
Learning: glamor's `css()` function returns a `StyleAttribute`, not a string.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx
Outdated
Show resolved
Hide resolved
|
The colors all look fixed! But found a different issue. This also exists in edge, but maybe you could fix it here. The carryover arrow is no longer in line. |
|
Adding new mobile VRTs as part of this PR. Hopefully this stabilizes some of the mobile VRTs a bit. |
I'll just handle this as part of a separate PR :) |
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range and nitpick comments (11)
packages/desktop-client/e2e/page-models/settings-page.js (2)
10-18: LGTM! Consider adding error handling and success verification.The
useBudgetTypemethod is well-implemented and follows good practices. It uses async/await correctly and employs role-based selectors for robustness. However, consider the following improvements:
- Add a check to see if the 'Budget mode toggle' feature is already enabled before enabling it.
- Implement error handling in case the switch button is not found.
- Add verification to ensure the budget type switch was successful.
Here's a suggested implementation with these improvements:
async useBudgetType(budgetType) { // Check if the feature is already enabled if (!(await this.isExperimentalFeatureEnabled('Budget mode toggle'))) { await this.enableExperimentalFeature('Budget mode toggle'); } const switchBudgetTypeButton = this.page.getByRole('button', { name: `Switch to ${budgetType} budgeting`, }); // Error handling if (!(await switchBudgetTypeButton.isVisible())) { throw new Error(`Button to switch to ${budgetType} budgeting not found`); } await switchBudgetTypeButton.click(); // Verify the switch was successful // This is a placeholder - replace with actual verification logic const newBudgetTypeIndicator = this.page.getByText(`Current: ${budgetType} budgeting`); await expect(newBudgetTypeIndicator).toBeVisible(); } // Add this helper method to the class async isExperimentalFeatureEnabled(featureName) { // Implementation depends on how you can check if a feature is enabled // This is a placeholder const featureCheckbox = this.page.getByRole('checkbox', { name: featureName }); return await featureCheckbox.isChecked(); }This implementation adds robustness and error checking to the method.
20-32: LGTM! Consider adding error handling and success verification.The changes to
enableExperimentalFeaturemethod improve its robustness by using role-based selectors. This is a good practice for accessibility and reliability in e2e tests. However, consider the following improvements:
- Implement error handling in case the buttons or checkbox are not found.
- Add verification to ensure the experimental feature was successfully enabled.
Here's a suggested implementation with these improvements:
async enableExperimentalFeature(featureName) { const advancedSettingsButton = this.page.getByTestId('advanced-settings'); const experimentalSettingsButton = this.page.getByTestId('experimental-settings'); const featureCheckbox = this.page.getByRole('checkbox', { name: featureName }); // Error handling for (const element of [advancedSettingsButton, experimentalSettingsButton, featureCheckbox]) { if (!(await element.isVisible())) { throw new Error(`Element not found: ${element}`); } } await advancedSettingsButton.click(); await experimentalSettingsButton.click(); // Check if the feature is already enabled if (!(await featureCheckbox.isChecked())) { await featureCheckbox.click(); } // Verify the feature was successfully enabled await expect(featureCheckbox).toBeChecked(); }This implementation adds error handling and verifies that the feature was successfully enabled. It also checks if the feature is already enabled before clicking, which can prevent unnecessary actions and potential test failures.
packages/desktop-client/e2e/page-models/mobile-account-page.js (1)
Verification Result:
The changes in
packages/desktop-client/e2e/page-models/mobile-account-page.jsdo not include any color-related modifications. Therefore, the main PR objective of fixing mobile balance modal colors is not addressed.Analysis chain
Line range hint
1-40: Verify if these changes address the PR objectivesThe changes in this file improve naming and consistency, which is good. However, they don't seem to directly address the main objective of fixing mobile balance modal colors as mentioned in the PR description and linked issue #3489.
Could you please clarify if these changes are sufficient to resolve the issue with balance colors in the mobile balance modal? If not, are there additional files or changes that need to be reviewed to address the main PR objective?
To help verify this, let's search for relevant files:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for files that might be related to mobile balance modal colors echo "Files potentially related to mobile balance modal:" fd -e js -e jsx -e css | rg -i 'mobile.*balance|balance.*modal' echo "\nRelevant color-related changes:" git diff origin/main | rg -i 'color|rgb|rgba|hex|#[0-9a-f]{3,6}'Length of output: 603
Script:
#!/bin/bash # Identify the current branch current_branch=$(git rev-parse --abbrev-ref HEAD) echo "Current branch: $current_branch" # Fetch the latest changes from the remote git fetch origin # Attempt to diff against origin/main echo "Attempting to diff against origin/main..." git diff origin/main -- color | rg -i 'color|rgb|rgba|hex|#[0-9a-f]{3,6}' # If origin/main does not exist, fallback to diff against main if [ $? -ne 0 ]; then echo "origin/main not found. Attempting to diff against main..." git diff main -- color | rg -i 'color|rgb|rgba|hex|#[0-9a-f]{3,6}' fiLength of output: 601
Script:
#!/bin/bash # Identify all remote branches to find the main branch echo "Fetching all remote branches..." git fetch --all echo "Listing remote branches:" git branch -r # Attempt to identify the main branch main_branch=$(git branch -r | grep -E 'origin/main|origin/master|origin/develop' | head -n 1 | awk -F'/' '{print $2}') if [ -z "$main_branch" ]; then echo "No known main branch (main, master, develop) found. Please verify the main branch name." exit 1 fi echo "Identified main branch: origin/$main_branch" # Perform git diff against the identified main branch for color-related changes echo "Performing git diff against origin/$main_branch for color-related changes..." git diff origin/$main_branch -- color | rg -i 'color|rgb|rgba|hex|#[0-9a-f]{3,6}' # If no output, inform the user if [ $? -ne 0 ]; then echo "No color-related changes found between current branch and origin/$main_branch." fiLength of output: 2514
Script:
#!/bin/bash # Resolve the main branch using git symbolic-ref main_branch_ref=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null) if [ $? -ne 0 ]; then echo "Unable to resolve origin/HEAD. Please verify the main branch name manually." exit 1 fi # Extract the branch name from the reference main_branch=$(echo "$main_branch_ref" | sed 's@^refs/remotes/origin/@@') echo "Identified main branch: origin/$main_branch" # Perform git diff against the identified main branch for color-related changes echo "Performing git diff against origin/$main_branch for color-related changes..." git diff origin/"$main_branch" -- color | rg -i 'color|rgb|rgba|hex|#[0-9a-f]{3,6}' # Check if rg found any matches if [ $? -ne 0 ]; then echo "No color-related changes found between current branch and origin/$main_branch." fiLength of output: 685
packages/desktop-client/e2e/settings.mobile.test.js (1)
11-22: LGTM: beforeEach hook is well-implemented with a minor suggestion.The setup ensures a clean state for each test, which is a good practice. The use of mobile dimensions and creation of a test file before each test ensures consistent starting conditions.
Consider extracting the viewport dimensions into constants at the top of the file for easier maintenance:
const MOBILE_VIEWPORT = { width: 350, height: 600 }; // Then in the beforeEach hook: await page.setViewportSize(MOBILE_VIEWPORT);packages/desktop-client/e2e/accounts.mobile.test.js (1)
1-57: Great overall structure, consider expanding test coverage.The test file is well-structured and follows best practices for e2e testing:
- Use of page object models enhances maintainability and reusability.
- Visual regression testing with
toMatchThemeScreenshots()is valuable for catching unexpected UI changes.- The two main test cases cover important aspects of mobile account functionality.
To further improve the test suite, consider adding the following test cases:
- Test navigation between multiple accounts.
- Verify the behavior of the balance modal mentioned in the PR objectives.
- Test edge cases such as accounts with zero or negative balances.
- Verify the carryover arrow alignment issue mentioned in the PR comments.
Example additional test case:
test('displays correct colors in balance modal', async () => { const accountsPage = await navigation.goToAccountsPage(); const accountPage = await accountsPage.openAccountByName('Test Account'); await accountPage.openBalanceModal(); // Assert on the colors in the balance modal await expect(accountPage.balanceModalValue).toHaveCSS('color', expectedColor); // Check carryover arrow alignment await expect(accountPage.carryoverArrow).toBeVisible(); // Add more specific alignment checks here await expect(page).toMatchThemeScreenshots(); });These additional tests would provide more comprehensive coverage of the mobile account features and address the specific issues mentioned in the PR objectives and comments.
packages/desktop-client/e2e/transactions.mobile.test.js (1)
Line range hint
1-95: Consider expanding test coverage for mobile transactionsWhile the remaining tests cover the basic functionality of creating transactions, the AI summary indicates that several tests have been removed. This significant reduction in test coverage might leave important mobile functionalities untested. Consider the following suggestions:
Reinstate or create new tests for critical mobile functionalities, such as:
- Loading the budget page with budgeted amounts
- Opening the accounts page with balance assertions
- Individual account page functionality (transaction filtering, search)
- Settings page accessibility and data export functionality
Add tests for edge cases and error scenarios in transaction creation.
Include tests for different viewport sizes within the mobile range to ensure responsiveness.
To improve the maintainability of these tests, consider:
- Creating shared utilities for common actions like creating and verifying transactions.
- Implementing data-driven tests to cover various transaction scenarios efficiently.
- Adding comments to explain the purpose and expectations of each test case.
packages/desktop-client/src/components/modals/TrackingBudgetMenuModal.tsx (1)
Line range hint
1-118: Consider addressing the carryover arrow alignment issueIn the PR comments, user youngcw mentioned a new issue regarding the misalignment of the carryover arrow in the mobile balance modal. While this wasn't part of the original PR objectives, it might be beneficial to address this issue in the same PR if it's related to the same component.
Could you please:
- Confirm if the carryover arrow is part of this
TrackingBudgetMenuModalcomponent?- If so, consider adding changes to fix the alignment issue in this PR.
- If not, it might be worth creating a separate issue to track and address this alignment problem.
packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx (1)
111-111: Approved: Improved consistency in aria-labelThe update of the
aria-labelfrom "transaction list" to "Transaction list" improves consistency and clarity. This change contributes to a more polished user interface for screen reader users.For even better consistency, consider using title case for all aria-labels throughout the application. For example, "Transaction List" instead of "Transaction list". This would ensure a uniform style across all accessibility labels.
packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (1)
Line range hint
1-1974: Consider further accessibility enhancementsThe changes made to this file have significantly improved testability and partially addressed accessibility concerns. To further enhance accessibility, consider the following suggestions:
- Add descriptive
aria-labelattributes to all interactive elements that lack clear text labels.- Ensure proper heading hierarchy using
h1,h2, etc., for screen reader navigation.- Implement
aria-liveregions for dynamic content updates.- Add
roleattributes where appropriate to clarify the purpose of elements.- Ensure sufficient color contrast for all text and interactive elements.
Implementing these suggestions would further improve the overall accessibility of the budget table component.
packages/desktop-client/e2e/page-models/mobile-budget-page.js (1)
80-85: Add error handling foropenBalanceMenu.In the
openBalanceMenumethod, there is no check to see if thebalanceButtonexists or is visible before attempting to click it. This could throw an error if the button is not present.Consider adding a check similar to other methods:
async openBalanceMenu(categoryName) { const balanceButton = this.page.getByTestId( `balance-${categoryName}-button`, ); + if (!(await balanceButton.isVisible())) { + throw new Error(`Balance button for category '${categoryName}' is not visible`); + } await balanceButton.click(); }packages/desktop-client/e2e/budget.mobile.test.js (1)
241-241: Typo in test description: 'Overspensing' should be 'Overspending'There's a typographical error in the test description. The word 'Overspensing' should be corrected to 'Overspending'.
Apply this diff to fix the typo:
-test('checks that clicking the Saved/Projected Savings/Overspensing amount opens the budget summary menu modal', async () => { +test('checks that clicking the Saved/Projected Savings/Overspending amount opens the budget summary menu modal', async () => {
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-via-footer-button-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-creates-a-transaction-via-footer-button-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!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (12)
- 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/mobile/budget/BudgetTable.jsx (8 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx (2 hunks)
- packages/desktop-client/src/components/modals/EnvelopeBudgetMenuModal.tsx (1 hunks)
- packages/desktop-client/src/components/modals/TrackingBudgetMenuModal.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx
Additional comments not posted (22)
packages/desktop-client/e2e/page-models/mobile-account-page.js (1)
9-9: LGTM: Improved naming and consistencyThe changes in these lines improve the code:
- Renaming
noTransactionsFoundErrortonoTransactionsMessagebetter reflects the purpose of the element.- Updating the label for
transactionListto 'Transaction list' improves consistency with proper capitalization conventions.These modifications enhance code readability and maintain consistency.
Also applies to: 11-11
packages/desktop-client/e2e/settings.mobile.test.js (4)
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 of test code.
6-10: LGTM: Test suite setup is well-structured.The test suite is properly described, and variables are declared at the suite level, allowing for efficient reuse across multiple tests.
24-26: LGTM: afterEach hook properly cleans up resources.Closing the page after each test is a good practice. It ensures that each test starts with a fresh page and helps in cleaning up resources.
28-42: LGTM with suggestions: Test case covers important functionality, but doesn't address the PR objectives directly.The test case is well-structured and covers important aspects of the settings page functionality, including visual regression testing and data export. However, it doesn't directly address the color issue mentioned in the PR objectives.
Consider adding specific tests to verify the color of the balance value in the mobile balance modal. This could involve:
- Opening the mobile balance modal
- Checking the color of the balance value
- Verifying that it matches the expected color based on the budget table
To assist in implementing these tests, I can help locate the relevant components. Let's search for files that might contain the mobile balance modal implementation:
Once we identify the relevant files, we can create more specific tests to address the PR objectives.
packages/desktop-client/e2e/accounts.mobile.test.js (1)
1-26: LGTM: Imports and test setup are well-structured.The imports, test suite setup, and hooks are correctly implemented. The viewport size is appropriately set to simulate a mobile device, which aligns with the test objectives for mobile account functionalities.
packages/desktop-client/e2e/transactions.mobile.test.js (2)
6-6: Minor update to test suite descriptionThe test suite description has been updated from "Mobile" to "Mobile Transactions", which provides a more specific focus for the test suite.
Line range hint
1-95: Reconsider the removal of critical mobile functionality testsThe AI summary indicates that several important tests have been removed, including those for the budget page, accounts page, and settings page. This significant reduction in test coverage poses the following risks:
- Reduced confidence in the overall functionality of the mobile application.
- Increased likelihood of undetected regression issues in untested areas.
- Difficulty in maintaining and refactoring code without comprehensive test coverage.
To mitigate these risks, consider the following strategies:
If the removed tests are no longer relevant due to changes in the application, document the reasons for their removal and ensure that the functionality they covered is either deprecated or tested through other means.
If the functionality is still present in the application, reinstate the removed tests or create new, more efficient tests to cover these areas.
Implement a comprehensive mobile testing strategy that includes both automated tests and manual testing procedures to ensure all critical functionalities are covered.
If the tests were removed to improve performance or reduce maintenance overhead, consider refactoring them to be more efficient rather than removing them entirely.
To help assess the impact of the removed tests, you can run the following script to check for any remaining references to the removed test functionalities in the codebase:
This script will help identify any remaining references to the removed test functionalities, which can guide decisions on whether to reinstate or update these tests.
packages/desktop-client/src/components/modals/EnvelopeBudgetMenuModal.tsx (3)
102-102: LGTM: Added data-testid attribute for testing purposes.The addition of the
data-testid="budget-amount"attribute to theFocusableAmountInputcomponent is a good practice for facilitating easier and more reliable automated testing. This change will help in identifying and interacting with this specific input element during test execution.
Line range hint
1-115: Verify if this change addresses the reported color issue.The PR objectives mention fixing the mobile balance modal colors, but the change in this file doesn't seem to directly address any color-related issues. The only modification is the addition of a
data-testidattribute, which is unrelated to styling or colors.Could you please clarify how this change relates to the reported issue of balance values not displaying colors corresponding to the budget table? If there are other files or components involved in fixing the color issue, please ensure they are included in the PR.
#!/bin/bash # Description: Search for color-related changes in the codebase # Test: Look for color-related changes in CSS or styled-components rg --type css --type typescript --type javascript -i '(color|background|style)' # Test: Check for any changes to theme or style files fd -e ts -e js -e css | xargs rg -i '(theme|style|color)'
Line range hint
1-115: Consider addressing the carryover arrow alignment issue.The comments summary mentions a new issue with the alignment of the carryover arrow in the mobile balance modal. This file doesn't contain any changes related to that issue.
Could you please verify if the carryover arrow alignment issue should be addressed in this PR? If so, consider making the necessary adjustments to fix the misalignment.
Verification successful
To properly verify the carryover arrow alignment issue in the mobile balance modal, we need to gather more specific information. Let's search for relevant mobile components and recent changes.
The recent commit "Fix rollover arrow display for mobile and desktop (#2943)" seems directly related to the issue mentioned in the review comment. Let's examine this commit and look for any mobile-specific changes to verify if the alignment issue has been addressed.
Based on the information gathered, it appears that the carryover arrow alignment issue in the mobile balance modal has been addressed. Here's a summary of the findings:
Carryover arrow alignment issue has been resolved
The alignment issue mentioned in the review comment has been fixed in commit 58e6c6f (PR #2943). This commit introduced changes to the mobile BudgetTable.jsx file, adding a carryover indicator with specific positioning and styling. The new code uses an SvgArrowThickRight component with carefully defined positioning, which should resolve the alignment problem.
Key points:
- A carryover indicator was added with proper alignment and styling.
- The fix applies to both mobile and desktop versions.
- No remaining TODOs or FIXMEs related to arrow alignment were found in the mobile components.
While the issue appears to be resolved, it's worth noting that there's an open PR (#3492) titled "Fix mobile balance modal colors". Although it doesn't explicitly mention arrow alignment, it might include further improvements to the mobile balance modal's appearance.
Recommendation: The carryover arrow alignment issue can be considered resolved. However, it may be worthwhile to keep an eye on PR #3492 for any additional enhancements to the mobile balance modal.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for carryover arrow related code # Test: Look for carryover arrow related components or styles rg -i '(carryover|arrow)' # Test: Check for any alignment related styles rg -i '(align|position|display|flex)'Length of output: 324502
Script:
#!/bin/bash # Search for mobile balance modal files echo "Searching for mobile balance modal files:" fd -e tsx -e jsx | grep -i 'mobile.*balance.*modal' # Search for recent changes related to carryover arrows or alignment in mobile components echo "Searching for recent changes in mobile components:" git log -n 5 --pretty=format:"%h %s" --grep="carryover\|arrow\|align" -- "packages/desktop-client/src/components/mobile" # Look for mobile-specific styling related to arrows or alignment echo "Searching for mobile-specific arrow or alignment styles:" rg -i '(arrow|align|carryover).*style' packages/desktop-client/src/components/mobileLength of output: 445
Script:
#!/bin/bash # Show the changes introduced by the commit echo "Changes introduced by commit 58e6c6f2:" git show 58e6c6f2 --name-only # Show the diff for mobile-specific files in this commit echo "Diff for mobile-specific files:" git show 58e6c6f2 -- packages/desktop-client/src/components/mobile # Look for any remaining TODOs or FIXMEs related to arrow alignment in mobile components echo "Searching for TODOs or FIXMEs related to arrow alignment:" rg -i '(todo|fixme).*arrow.*align' packages/desktop-client/src/components/mobile # Check for any recent issues or pull requests related to arrow alignment echo "Recent issues or pull requests related to arrow alignment:" gh issue list --search "arrow align" --limit 5 gh pr list --search "arrow align" --limit 5Length of output: 3679
packages/desktop-client/src/components/modals/TrackingBudgetMenuModal.tsx (1)
102-102: Approve addition of data-testid, but query relevance to PR objectivesThe addition of the
data-testid="budget-amount"attribute is a good practice for improving the testability of the component. This will make it easier to select and interact with the budget amount input in automated tests.However, this change doesn't appear to address the color issue mentioned in the PR objectives. Could you please clarify how this change relates to fixing the mobile balance modal colors?
To verify if there are any color-related changes that might have been missed, let's search for color-related properties in this file:
packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx (2)
94-94: Excellent accessibility improvement!The addition of the
aria-label="Loading..."to theViewcomponent is a great enhancement. This change improves accessibility by providing clear feedback to users relying on screen readers when the content is in a loading state.
Line range hint
94-111: Overall positive impact, but color issue not addressedThe changes in this file improve accessibility and consistency, which is commendable. However, the main objective of the PR, as stated in the PR description, was to fix the issue where "the balance value in the mobile balance modal no longer displayed colors corresponding to the budget table."
Could you please confirm if the color issue has been addressed in other files not included in this review? If not, consider adding the necessary changes to restore the color functionality for the balance values in the mobile balance modal.
To verify this, you can run the following command to search for relevant color-related changes:
This will help ensure that the primary objective of the PR is met.
packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (6)
282-287: Improved testability with dynamic data-testidThe addition of a dynamic
data-testidattribute to the CellValue component enhances the testability of the budgeted cells. This change allows for more precise and reliable test selectors.
303-303: Enhanced button testability with dynamic data-testidThe addition of a dynamic
data-testidattribute to the Button component improves the testability of individual budgeted buttons. This change is consistent with the previous modifications and adheres to best practices for testing React components.
604-608: Improved spent cell testability with dynamic data-testidThe addition of a dynamic
data-testidattribute to the CellValue component for spent amounts enhances the testability of these cells. This change is consistent with previous modifications and follows best practices for testing React components.
616-616: Enhanced spent button testability with dynamic data-testidThe addition of a dynamic
data-testidattribute to the Button component for spent amounts improves the testability of individual spent buttons. This change is consistent with previous modifications and adheres to best practices for testing React components.
Line range hint
648-681: Improved balance component testability with dynamic data-testid attributesThe addition of dynamic
data-testidattributes to both the BalanceWithCarryover component and its associated Button component enhances the testability of balance-related elements. These changes are consistent with previous modifications and adhere to best practices for testing React components.
Line range hint
1711-1974: Improved testability and accessibility for budget table header and month navigationThe following enhancements have been made:
- Addition of a
data-testidattribute to the BudgetTableHeader component.- Implementation of
aria-labelattributes for the "Previous month" and "Next month" buttons.- Addition of a
data-testidattribute to the month button.These changes improve both the testability and accessibility of the budget table header and month navigation components. They adhere to best practices for testing React components and enhance the user experience for those using assistive technologies.
packages/desktop-client/e2e/page-models/mobile-budget-page.js (2)
54-60: Ensure input is sanitized when setting budget amount.In the
setBudgetmethod, user input is directly converted to a string and typed. Ensure thatnewAmountis properly sanitized to prevent any injection attacks or unexpected behavior.Please run a check to confirm that
newAmountis always a valid number before proceeding.
1-136: Overall code quality is good.The methods are well-structured, and the use of async/await is appropriate. Once the above issues are addressed, the code will be more robust and maintainable.
joel-jeremy
force-pushed
the
fix-mobile-balance-modal-colors
branch
from
7905fab to
287b917
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
packages/desktop-client/e2e/page-models/settings-page.js (3)
10-18: LGTM! Consider adding error handling and verification.The new
useBudgetTypemethod is well-structured and follows good practices. It enables the experimental feature and switches to the specified budget type as intended.Consider the following improvements:
- Add error handling if the button is not found.
- Verify if the switch was successful after clicking the button.
- Check if the experimental feature is already enabled before enabling it.
Here's a suggested implementation:
async useBudgetType(budgetType) { 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 (adjust the selector as needed) const newBudgetTypeIndicator = this.page.getByText(`Current: ${budgetType} budgeting`); await expect(newBudgetTypeIndicator).toBeVisible(); } else { throw new Error(`Button to switch to ${budgetType} budgeting not found`); } }This implementation includes error handling, verification of the switch, and assumes
enableExperimentalFeaturemethod handles the case where the feature is already enabled.
21-32: LGTM! Consider adding error handling and verification.The changes to the
enableExperimentalFeaturemethod improve its robustness by using role-based selectors. This enhances accessibility and maintainability of the test code.Consider the following improvements:
- Add error handling if elements are not found.
- Verify if the feature was successfully enabled after clicking the checkbox.
- Check if the feature is already enabled before attempting to enable it.
Here's a suggested 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 }); if (await featureCheckbox.isVisible()) { if (!(await featureCheckbox.isChecked())) { await featureCheckbox.click(); // Verify the feature was enabled await expect(featureCheckbox).toBeChecked(); } else { console.log(`Feature "${featureName}" is already enabled.`); } } else { throw new Error(`Checkbox for feature "${featureName}" not found`); } }This implementation includes error handling, verification of the feature enablement, and checks if the feature is already enabled before attempting to enable it.
Line range hint
1-33: Overall, the changes improve test robustness and add necessary functionality.The modifications to this file align well with the PR objectives. The new
useBudgetTypemethod enables testing of the budget mode toggle feature, which is crucial for addressing the color display issue in the mobile balance modal. The improvements to theenableExperimentalFeaturemethod enhance the reliability of enabling experimental features during testing.While the changes are generally good, implementing the suggested improvements for error handling and verification would further enhance the robustness of these e2e tests. This would help catch potential issues earlier in the development process and make the tests more resilient to changes in the UI.
Consider creating helper functions for common operations like clicking buttons and verifying element states. This would reduce code duplication and make the tests easier to maintain as the application evolves.
packages/desktop-client/e2e/transactions.mobile.test.js (1)
Multiple Tests Removed Reduces Overall Test Coverage
The comprehensive search confirms that the tests for budget page loading, settings page accessibility, and data export functionality are no longer present in the codebase. Removing these tests significantly decreases the test coverage for mobile functionality, which may lead to undetected regressions in these areas.
Please address the following:
- Provide the rationale behind removing these tests.
- Ensure that equivalent test coverage is maintained, either by relocating these tests to other files or implementing alternative testing strategies.
Analysis chain
Line range hint
1-93: Clarify the rationale for removing multiple testsThe AI summary indicates that several tests have been removed from this file, including tests for:
- Loading the budget page with budgeted amounts
- Opening the accounts page with balance assertions
- Individual account page functionality (transaction filtering and search)
- Settings page accessibility
- Data export functionality
While the remaining test for transaction creation aligns with the PR objectives, the removal of these other tests significantly reduces the overall test coverage for mobile functionality. This could potentially lead to undetected regressions in the removed areas.
Could you please clarify:
- The rationale behind removing these tests?
- Are these tests being moved to another file or replaced by different testing strategies?
- How do we plan to ensure continued coverage for the functionality that was previously tested?
To verify the extent of test removal, let's check for the presence of these tests in other files:
This will help us understand if these tests have been moved to other files or if they have been completely removed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for potentially moved tests in other files # Test: Search for budget page loading test echo "Searching for budget page loading test:" rg --type js "test\(.*budget.*page.*loading" ./packages/desktop-client/e2e # Test: Search for accounts page opening test echo "Searching for accounts page opening test:" rg --type js "test\(.*open.*accounts.*page" ./packages/desktop-client/e2e # Test: Search for individual account page functionality test echo "Searching for individual account page functionality test:" rg --type js "test\(.*account.*page.*filter" ./packages/desktop-client/e2e # Test: Search for settings page accessibility test echo "Searching for settings page accessibility test:" rg --type js "test\(.*settings.*page.*accessibility" ./packages/desktop-client/e2e # Test: Search for data export functionality test echo "Searching for data export functionality test:" rg --type js "test\(.*data.*export" ./packages/desktop-client/e2eLength of output: 1166
Script:
#!/bin/bash # Description: Perform a comprehensive search for removed tests across the entire repository # Define search patterns patterns=( "test\(.*budget.*page.*loading" "test\(.*open.*accounts.*page" "test\(.*account.*page.*filter" "test\(.*settings.*page.*accessibility" "test\(.*data.*export" ) # Iterate over each pattern and search in all .js files for pattern in "${patterns[@]}"; do echo "Searching for pattern: $pattern" rg --type js "$pattern" . echo "" doneLength of output: 1480
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (104)
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-via-footer-button-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-creates-a-transaction-via-footer-button-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 as they are similar to previous changes (13)
- packages/desktop-client/e2e/accounts.mobile.test.js
- packages/desktop-client/e2e/budget.mobile.test.js
- packages/desktop-client/e2e/page-models/mobile-account-page.js
- packages/desktop-client/e2e/page-models/mobile-budget-page.js
- packages/desktop-client/e2e/settings.mobile.test.js
- packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx
- packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx
- packages/desktop-client/src/components/modals/EnvelopeBalanceMenuModal.tsx
- packages/desktop-client/src/components/modals/EnvelopeBudgetMenuModal.tsx
- packages/desktop-client/src/components/modals/TrackingBalanceMenuModal.tsx
- packages/desktop-client/src/components/modals/TrackingBudgetMenuModal.tsx
Additional comments not posted (1)
packages/desktop-client/e2e/transactions.mobile.test.js (1)
6-6: LGTM: Improved test suite descriptionThe change from 'Mobile' to 'Mobile Transactions' provides a more accurate and specific description of the test suite's focus. This improvement in clarity is beneficial for maintainability and understanding of the test suite's purpose.
joel-jeremy
force-pushed
the
fix-mobile-balance-modal-colors
branch
from
5d18c19 to
80725bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
packages/desktop-client/e2e/transactions.mobile.test.js (2)
Line range hint
28-62: Well-structured test case with good coverageThe remaining test case for creating a transaction via the footer button is well-structured and comprehensive. It covers important steps such as navigation, data entry, and verification. The use of theme screenshots for visual regression testing is a good practice to catch unexpected visual changes.
However, considering the removal of other tests mentioned in the AI summary, there might be a loss of coverage for other important mobile functionalities.
Consider adding more test cases to maintain comprehensive coverage of mobile functionalities, such as:
- Verifying the budget page loading with budgeted amounts
- Testing the accounts page functionality
- Checking the accessibility of the settings page
- Validating the data export functionality
This will help ensure that all critical mobile features are adequately tested.
Line range hint
1-93: Summary of review findings and recommendations
- The change in the test suite description improves clarity and focus.
- There's a discrepancy between the AI-generated summary and the visible code changes, which needs to be verified.
- The remaining test case for creating a transaction is well-structured and uses good practices like visual regression testing.
- However, if other tests have indeed been removed as mentioned in the AI summary, there might be a significant reduction in test coverage for mobile functionalities.
Recommendations:
- Verify the actual changes made to this file, ensuring consistency with the AI summary.
- If tests have been removed, consider adding new test cases to maintain comprehensive coverage of mobile functionalities.
- Document the rationale behind any test removals to help future maintainers understand the decisions made.
- Consider implementing more granular test cases to cover various aspects of mobile transactions and related features.
These steps will help ensure the continued reliability and effectiveness of the mobile transaction testing suite.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (105)
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-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-creates-a-transaction-via-footer-button-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 as they are similar to previous changes (14)
- packages/desktop-client/e2e/accounts.mobile.test.js
- packages/desktop-client/e2e/budget.mobile.test.js
- packages/desktop-client/e2e/page-models/mobile-account-page.js
- packages/desktop-client/e2e/page-models/mobile-budget-page.js
- packages/desktop-client/e2e/page-models/settings-page.js
- packages/desktop-client/e2e/settings.mobile.test.js
- packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx
- packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx
- packages/desktop-client/src/components/modals/EnvelopeBalanceMenuModal.tsx
- packages/desktop-client/src/components/modals/EnvelopeBudgetMenuModal.tsx
- packages/desktop-client/src/components/modals/TrackingBalanceMenuModal.tsx
- packages/desktop-client/src/components/modals/TrackingBudgetMenuModal.tsx
Additional comments not posted (2)
packages/desktop-client/e2e/transactions.mobile.test.js (2)
6-6: Improved test suite descriptionThe change from 'Mobile' to 'Mobile Transactions' provides a more specific and focused description of the test suite. This improvement in clarity is beneficial for maintaining and understanding the purpose of these tests.
Line range hint
1-93: Verify consistency between AI summary and code changesThe AI-generated summary mentions the removal of multiple tests, including those for budget page loading, accounts page functionality, and settings page accessibility. However, these changes are not visible in the provided code snippet. It's important to ensure that the summary accurately reflects the actual changes made to the file.
To verify the consistency between the AI summary and the actual code changes, please run the following script:
This script will help us confirm whether the tests mentioned in the AI summary have indeed been removed and if there are any discrepancies between the summary and the actual code.
joel-jeremy
force-pushed
the
fix-mobile-balance-modal-colors
branch
from
e9a0d38 to
71ba2d8
Compare
joel-jeremy
changed the title
joel-jeremy
force-pushed
the
fix-mobile-balance-modal-colors
branch
from
86a4f1e to
a1b1a0c
Compare
|
How hard would it be to split out the new vrt to its own PR? I would like to get this fix in soon, but the vrt changes probably would take a bit to get working and reviewed. |
joel-jeremy
force-pushed
the
fix-mobile-balance-modal-colors
branch
from
760988f to
ff1d28f
Compare
joel-jeremy
changed the title
|
Separated mobile VRTs to #3505 |
joel-jeremy
force-pushed
the
fix-mobile-balance-modal-colors
branch
from
ff1d28f to
a5fbd5a
Compare
…over.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Found a bug. Recreate:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx (1)
200-200: Address linting warning: Insert line break.There's a linting warning suggesting the insertion of a line break on line 200.
Apply this diff to address the linting issue:
triggerProps={{ delay: 750, - isDisabled: !isGoalTemplatesEnabled || goalValue == null || isNarrowWidth, + isDisabled: + !isGoalTemplatesEnabled || goalValue == null || isNarrowWidth, }}This change improves code formatting and adheres to the project's linting rules.
🧰 Tools
GitHub Check: lint
[warning] 200-200:
Insert⏎···············
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (1)
upcoming-release-notes/3492.mdis excluded by!**/*.md
📒 Files selected for processing (4)
- 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/modals/EnvelopeBalanceMenuModal.tsx (1 hunks)
- packages/desktop-client/src/components/modals/TrackingBalanceMenuModal.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx
- packages/desktop-client/src/components/modals/EnvelopeBalanceMenuModal.tsx
- packages/desktop-client/src/components/modals/TrackingBalanceMenuModal.tsx
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx (1)
Learnt from: joel-jeremy PR: actualbudget/actual#3492 File: packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx:123-136 Timestamp: 2024-09-25T21:00:54.979Z Learning: glamor's `css()` function returns a `StyleAttribute`, not a string.
GitHub Check: lint
packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx
[warning] 200-200:
Insert⏎···············
🔇 Additional comments not posted (4)
packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx (4)
13-13: LGTM: Added responsive design support.The addition of
useResponsivehook is a good improvement, allowing the component to adapt to different screen sizes and enhance the user experience across devices.
65-71: LGTM: Improved type safety for children prop.The addition of
CellValueChildrenandChildrenWithClassNametypes, along with the new optionalchildrenprop inBalanceWithCarryoverProps, enhances type safety and flexibility. This change allows for better customization of the component while maintaining strong typing.Also applies to: 77-77
75-75: LGTM: Improved prop definitions.The updates to
BalanceWithCarryoverPropsare well-considered:
- Removing 'children' and 'binding' from inherited props aligns with the new children prop definition.
- Renaming 'disabled' to 'isDisabled' follows a common naming convention for boolean props, improving code readability.
These changes enhance the component's API clarity and maintainability.
Also applies to: 83-83
145-218: LGTM: Enhanced render logic with improved user feedback.The updates to the component's render logic are well-implemented:
- The use of
getDefaultClassNameimproves code organization and reusability.- The new
Tooltipcomponent provides detailed information about the balance and goal status, enhancing the user experience.- The component now handles various scenarios (fully funded, overfunded, underfunded) and different goal types, making it more versatile and informative.
These changes significantly improve the component's functionality and user feedback.
🧰 Tools
GitHub Check: lint
[warning] 200-200:
Insert⏎···············
| const getDefaultClassName = useCallback( | ||
| (balanceValue: number) => | ||
| String( | ||
| css({ | ||
| ...getBalanceAmountStyle(balanceValue), | ||
| overflow: 'hidden', | ||
| textOverflow: 'ellipsis', | ||
| textAlign: 'right', | ||
| ...(!isDisabled && { | ||
| cursor: 'pointer', | ||
| }), | ||
| ':hover': { textDecoration: 'underline' }, | ||
| }), | ||
| ), | ||
| [getBalanceAmountStyle, isDisabled], | ||
| ); |
There was a problem hiding this comment.
Remove unnecessary String() wrapping around css().
The getDefaultClassName function looks good overall, but there's an unnecessary String() wrapping around the css() call. According to the learning note, css() returns a StyleAttribute, not a string. The String() wrapping is redundant and potentially incorrect.
Apply this diff to fix the issue:
const getDefaultClassName = useCallback(
(balanceValue: number) =>
- String(
- css({
- ...getBalanceAmountStyle(balanceValue),
- overflow: 'hidden',
- textOverflow: 'ellipsis',
- textAlign: 'right',
- ...(!isDisabled && {
- cursor: 'pointer',
- }),
- ':hover': { textDecoration: 'underline' },
- }),
- ),
+ css({
+ ...getBalanceAmountStyle(balanceValue),
+ overflow: 'hidden',
+ textOverflow: 'ellipsis',
+ textAlign: 'right',
+ ...(!isDisabled && {
+ cursor: 'pointer',
+ }),
+ ':hover': { textDecoration: 'underline' },
+ }),
[getBalanceAmountStyle, isDisabled],
);This change ensures that the StyleAttribute returned by css() is used directly, which is the correct approach when working with glamor's css() function.
📝 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.
| const getDefaultClassName = useCallback( | |
| (balanceValue: number) => | |
| String( | |
| css({ | |
| ...getBalanceAmountStyle(balanceValue), | |
| overflow: 'hidden', | |
| textOverflow: 'ellipsis', | |
| textAlign: 'right', | |
| ...(!isDisabled && { | |
| cursor: 'pointer', | |
| }), | |
| ':hover': { textDecoration: 'underline' }, | |
| }), | |
| ), | |
| [getBalanceAmountStyle, isDisabled], | |
| ); | |
| const getDefaultClassName = useCallback( | |
| (balanceValue: number) => | |
| css({ | |
| ...getBalanceAmountStyle(balanceValue), | |
| overflow: 'hidden', | |
| textOverflow: 'ellipsis', | |
| textAlign: 'right', | |
| ...(!isDisabled && { | |
| cursor: 'pointer', | |
| }), | |
| ':hover': { textDecoration: 'underline' }, | |
| }), | |
| [getBalanceAmountStyle, isDisabled], | |
| ); |
Fixes #3489