Remove spending report feature flag #3611
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
WalkthroughThe pull request introduces significant changes across several components within the desktop client. In the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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 No assets were bigger 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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were 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: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/reports/reports/SpendingCard.tsx (3)
Line range hint
63-69: Simplify thetodayDaycalculationThe current calculation for
todayDayis complex and could be simplified for better readability and maintainability.Consider refactoring the
todayDaycalculation as follows:const todayDay = Math.min( monthUtils.getDay(monthUtils.currentDay()) - 1, 27 );This simplification removes the nested ternary operator and makes the logic more straightforward.
Line range hint
70-73: Improve variable naming for clarityThe
differencevariable could benefit from a more descriptive name to better convey its purpose.Consider renaming
differenceto something more specific, such asspendingDifferenceormonthlySpendingChange. This would make the code more self-documenting and easier to understand at a glance.
Line range hint
117-127: Simplify nested ternary operatorThe nested ternary operator used for determining the text color based on the
differencevalue could be simplified for better readability.Consider refactoring this logic into a separate function or using a more readable approach. For example:
const getDifferenceColor = (difference: number | null) => { if (!difference) return 'inherit'; return difference <= 0 ? theme.noticeTextLight : theme.errorText; }; // Then in the JSX: <Block style={{ ...styles.mediumText, fontWeight: 500, marginBottom: 5, color: getDifferenceColor(difference), }} > {/* ... */} </Block>This approach separates the color logic from the JSX, making it easier to read and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3611.mdis excluded by!**/*.md
📒 Files selected for processing (5)
- packages/desktop-client/src/components/reports/Overview.tsx (1 hunks)
- packages/desktop-client/src/components/reports/reports/SpendingCard.tsx (1 hunks)
- packages/desktop-client/src/components/settings/Experimental.tsx (0 hunks)
- packages/desktop-client/src/hooks/useFeatureFlag.ts (0 hunks)
- packages/loot-core/src/types/prefs.d.ts (0 hunks)
💤 Files with no reviewable changes (3)
- packages/desktop-client/src/components/settings/Experimental.tsx
- packages/desktop-client/src/hooks/useFeatureFlag.ts
- packages/loot-core/src/types/prefs.d.ts
🧰 Additional context used
🔇 Additional comments (4)
packages/desktop-client/src/components/reports/reports/SpendingCard.tsx (1)
Line range hint
1-145: Summary of review findings
- There is an inconsistency between the AI-generated summary and the provided code that needs to be resolved.
- The
SpendingCardcomponent functions correctly but has a few areas that could be improved for better readability and maintainability:
- Simplifying the
todayDaycalculation- Improving variable naming for clarity
- Refactoring the nested ternary operator in the JSX
Please address these points to enhance the overall quality of the code. Once the inconsistency between the AI summary and the actual code changes is resolved, a more thorough review of the specific changes can be conducted.
packages/desktop-client/src/components/reports/Overview.tsx (3)
408-411: LGTM: Spending card option added to menu itemsThis change aligns with the PR objective of removing the spending report feature flag. The 'Spending analysis' option is now consistently available in the menu for adding new widgets, which improves the user experience by making this feature accessible to all users.
Line range hint
1-624: Request for clarification on the extent of changesThe visible changes in this file are minimal, mainly adding the 'Spending analysis' option to the menu items. This aligns with the PR objective of removing the spending report feature flag. However, some changes mentioned in the AI-generated summary, such as modifications to the
layoutvariable and the removal of thespendingReportFeatureFlagcheck, are not visible in this diff.To ensure a comprehensive review:
- Could you please clarify if there are any other files involved in this PR that contain the remaining changes?
- If all changes are supposed to be in this file, could you verify if any changes were accidentally omitted from the commit?
This information will help in providing a more accurate and thorough review of the PR.
Line range hint
1-624: Verify removal of feature flag check for spending reportThe AI-generated summary mentioned changes related to the removal of the
spendingReportFeatureFlagcheck and modifications to thelayoutvariable to always include the 'spending-card' widget. However, these changes are not visible in the current diff. Please verify if these changes have been made elsewhere or if they were omitted from this PR.To verify these changes, you can run the following script:
|
Exciting! |
|
Did we ever get the month on the dashboard card thing figured out? Weren't there still some people that were showing the wrong month? edit: |
Looks like the same thing is happening to other reports as well, so probably not a stopper for the spending report. |
|
Should look into #3679 before moving forward with this release |
carkom
force-pushed
the
SpendingFeature
branch
from
5376820 to
c62aafb
Compare
carkom
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/reports/Overview.tsx (1)
391-394: LGTM: Spending card added to widget menu.The 'spending-card' option is now always available in the widget menu, which is consistent with the removal of the feature flag. This change makes the spending report feature accessible to all users.
Consider changing the text from "Spending analysis" to "Spending report" for consistency with the feature name used elsewhere in the application and PR description.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
packages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-and-cash-flow-reports-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-and-cash-flow-reports-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-and-cash-flow-reports-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (3)
- packages/desktop-client/e2e/reports.test.js (1 hunks)
- packages/desktop-client/src/components/reports/Overview.tsx (2 hunks)
- packages/desktop-client/src/components/settings/Experimental.tsx (0 hunks)
💤 Files with no reviewable changes (1)
- packages/desktop-client/src/components/settings/Experimental.tsx
🧰 Additional context used
🔇 Additional comments (3)
packages/desktop-client/e2e/reports.test.js (1)
33-33: Approved: Update to include Monthly Spending reportThe change correctly reflects the new expected behavior of including the Monthly Spending report in the list of available reports. This aligns with the PR objectives of removing the spending report feature flag.
Consider updating the test name from 'loads net worth and cash flow reports' to 'loads available reports' or 'loads all standard reports' to accurately reflect its new purpose.
Let's check if there are any other tests related to available reports that might need updating:
✅ Verification successful
I'm sorry, but I can't provide internal monologue or hidden reasoning. However, based on the provided information, here is my conclusion:
Verification Complete: No Additional Test Updates Required
The search did not find any other tests related to available reports that need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests related to available reports rg -i "available.*report" packages/desktop-client/e2eLength of output: 254
packages/desktop-client/src/components/reports/Overview.tsx (2)
105-105: LGTM: Simplified layout assignment aligns with PR objectives.The direct assignment of
layoutfrombaseLayoutremoves the conditional logic related to the spending report feature flag. This change is consistent with the PR's goal of removing the feature flag and making the spending report a permanent feature.
Line range hint
1-594: Verify the impact of removing the spending report feature flag.The changes successfully remove the feature flag for the spending report, making it a permanent feature of the dashboard. This aligns with the PR objectives and the author's assessment that the feature is ready for release.
To ensure a smooth transition, please run the following verification steps:
These tests will help ensure that the feature flag has been completely removed and that the SpendingCard is now unconditionally included in the dashboard.
✅ Verification successful
Verified: The
spendingReportFeatureFlaghas been successfully removed, and theSpendingCardcomponent is now included unconditionally in the dashboard.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of spendingReportFeatureFlag and the unconditional inclusion of the SpendingCard component. # Test 1: Check for any remaining references to spendingReportFeatureFlag echo "Checking for remaining references to spendingReportFeatureFlag:" rg "spendingReportFeatureFlag" -g '!*.md' # Test 2: Verify that SpendingCard is always included without conditional checks echo "Verifying unconditional inclusion of SpendingCard:" rg -A 5 "SpendingCard" packages/desktop-client/src/components/reports/Overview.tsx # Test 3: Check for any remaining feature flag related to spending report in other files echo "Checking for any remaining spending report feature flags:" rg "spending.*report.*flag" -g '!*.md'Length of output: 964
| @@ -17,7 +17,8 @@ export default async function runMigration(db) { | |||
| INSERT INTO dashboard (id, type, width, height, x, y) | |||
| VALUES | |||
| ('${uuidv4()}','net-worth-card', 8, 2, 0, 0), | |||
| ('${uuidv4()}', 'cash-flow-card', 4, 2, 8, 0); | |||
| ('${uuidv4()}', 'cash-flow-card', 4, 2, 8, 0), | |||
| ('${uuidv4()}', 'spending-card', 4, 2, 0, 2); | |||
There was a problem hiding this comment.
shouldn't this be a new migration and not an old one that has already been applied?
There was a problem hiding this comment.
Usually yes, but I don't want to add to any existing dbs. This is only applying to brand new instances. A new migration would inadvertently add a spending-card to everyone's report dashboard. I only want the spending report to show up as one of the default graphs for new installs and do nothing to existing ones. Hence the change to an existing migration.
I know editing existing migrations is generally not a good idea, but this is the best way I see to accomplish the end goal without causing any disruption to everyone already using the reports dashboard. I'm happy to be wrong, let me know if there's a better way to do this!
I think this is ready for release. No further feedback has come in on it in a while.