✨ (dashboards) ability to save filters & timeframes on spending widgets #3432
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
MatissJanis
changed the title
/ed/ removed incorrect info (see Carkom's comment below) |
|
@Teprifer you are not using the most up-to-date version of edge. |
|
@carkom Doh! I'd updated but not refreshed enough -.- Thanks for the heads up. |
|
Fatal Error found:
|
| options={[ | ||
| ...(reportMode === 'single-month' | ||
| ? [] | ||
| : [['current-month', t('Current Month')] as const]), |
There was a problem hiding this comment.
I'd suggest we remove the actual current month from the list so it's not duplicated. Could we also offer a "last month" option in the "to" selection and create a "live" graph when "single month" button is chosen?
There was a problem hiding this comment.
I actually removed the notion of "current month" and instead aligned the functionality with the other reports to have a more consistent interface. It did seem like a good idea in the beginning to use the "current month" label, but it actually has a fair few complexities that I hadn't considered. Maybe we will bring it back in a future revision. But for now - no.
--
Added the "live/static" button.
|
@MatissJanis - the calculateTimeRange is not handling reversed dates. Not sure how it should handle them but current functionality is not very intuitive. I guess just an absolute value for the "month difference"?
It's also making the "CompareTo" (which is saved as "end" in the code and the dotted gray line in the graph) equal to the current month which is not the intended function. Current month should be green in a live graph. Seems like the calculateTimeRange is giving you "start" and "end" in reverse order. |
WalkthroughThe pull request introduces a series of modifications across multiple components in the desktop client. Key updates include the addition of new properties for customization, adjustments to rendering logic based on specific conditions, and improvements to type safety. The refactoring of components aims to streamline data handling and enhance the overall functionality of the application, particularly in relation to spending reports and widget management. Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/reports/reports/Spending.tsx (1)
Line range hint
63-584: Consider breaking down the UI rendering logic into smaller components.The function is using the
useFilters,useReport,useNavigate, anduseResponsivehooks for state management, data fetching, navigation, and responsive design, which are good practices. However, the UI rendering logic is quite complex and could be broken down into smaller components for better readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
upcoming-release-notes/3432.mdis excluded by!**/*.md
Files selected for processing (12)
- packages/desktop-client/src/components/common/Select.tsx (3 hunks)
- packages/desktop-client/src/components/reports/DateRange.tsx (1 hunks)
- packages/desktop-client/src/components/reports/Header.tsx (1 hunks)
- packages/desktop-client/src/components/reports/Overview.tsx (1 hunks)
- packages/desktop-client/src/components/reports/ReportRouter.tsx (1 hunks)
- packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx (8 hunks)
- packages/desktop-client/src/components/reports/reports/Spending.tsx (8 hunks)
- packages/desktop-client/src/components/reports/reports/SpendingCard.tsx (4 hunks)
- packages/desktop-client/src/components/reports/spreadsheets/spending-spreadsheet.ts (0 hunks)
- packages/loot-core/src/types/models/dashboard.d.ts (1 hunks)
- packages/loot-core/src/types/models/reports.d.ts (0 hunks)
- packages/loot-core/src/types/prefs.d.ts (0 hunks)
Files not reviewed due to no reviewable changes (3)
- packages/desktop-client/src/components/reports/spreadsheets/spending-spreadsheet.ts
- packages/loot-core/src/types/models/reports.d.ts
- packages/loot-core/src/types/prefs.d.ts
Additional comments not posted (20)
packages/desktop-client/src/components/reports/ReportRouter.tsx (1)
20-20: LGTM! The new route aligns with the PR objective.The addition of the
/spending/:idroute with a dynamic:idparameter enables rendering theSpendingcomponent for specific spending reports. This change supports the PR objective of allowing users to save individual spending widgets within dashboards, as each saved widget would have a unique identifier.The implementation looks good and does not appear to negatively impact existing functionality.
packages/desktop-client/src/components/reports/DateRange.tsx (1)
65-70: LGTM!The new
else ifblock correctly handles the rendering logic for 'budget' and 'average' types. It formats thestartDateand displays the appropriate comparison text, enhancing the component's functionality and user experience.packages/loot-core/src/types/models/dashboard.d.ts (1)
44-50: LGTM! The changes enhance the functionality and versatility of theSpendingWidget.The addition of the new optional properties (
conditions,conditionsOp,timeFrame, andmode) allows for more detailed and customizable representations of spending data:
conditionsandconditionsOpenable more complex filtering or criteria to be applied to the spending data.timeFrameintroduces a temporal aspect to the widget, allowing it to be configured for different periods.modeprovides the ability for the widget to operate in different contexts, such as 'single-month', 'budget', or 'average'.The breaking change is justified given the experimental nature of the feature, as mentioned in the PR objectives. The refactoring effort to transition from
LocalPrefsto a widget-based approach seems to be a step towards a more modular and maintainable codebase.packages/desktop-client/src/components/common/Select.tsx (2)
29-29: LGTM!The new optional
popoverStyleproperty enhances the flexibility of theSelectcomponent by allowing custom styles for the popover. The typeCSSPropertiesis correctly imported and used. This change doesn't introduce any breaking changes or issues.
55-55: LGTM!The
popoverStyleprop is correctly destructured with a default value of an empty object, ensuring that existing code will continue to work without any issues. Applying thepopoverStyleprop to thestyleattribute of thePopovercomponent allows custom styles to be applied when the popover is rendered. These changes are consistent with the addition of thepopoverStyleproperty to theSelectPropstype and don't introduce any breaking changes or issues.Also applies to: 116-116
packages/desktop-client/src/components/reports/reports/SpendingCard.tsx (5)
26-26: LGTM!The
widgetIdprop is a valid addition to theSpendingCardPropstype.
34-36: LGTM!The destructuring of the
widgetIdprop and the default value assignment for themetaprop are valid changes.
40-46: LGTM!The variable assignments for
isDashboardsFeatureEnabled,start,end, andspendingReportModeare valid and consistent with the provided context.
51-59: LGTM!The variable assignments for
selectionandgetGraphDataare valid and consistent with the provided context. The use ofuseMemofor memoizing thegetGraphDatafunction is a good optimization.
Line range hint
63-169: LGTM!The variable assignments, conditional rendering, and the use of the
DateRange,SpendingGraph, andLoadingIndicatorcomponents are all valid and consistent with the provided context.packages/desktop-client/src/components/reports/Header.tsx (1)
76-76: Verify the impact of removing the'/reports/spending'condition.The change in the conditional rendering logic removes the check for
'/reports/spending'path. This means that theViewcomponent will now be rendered for any path other than'/reports/custom'.Please clarify the intention behind this change and verify if it aligns with the expected user interface behavior for different report paths. Ensure that removing the
'/reports/spending'condition does not lead to any unintended consequences.To verify the impact, run the following script:
Verification successful
The removal of '/reports/spending' condition appears intentional and beneficial.
After verifying the codebase, it's clear that the change in
Header.tsxallows the View component to be rendered for all report paths except '/reports/custom'. This includes '/reports/spending', '/reports/net-worth', and '/reports/cash-flow'.The impact of this change is that it simplifies the conditional logic and provides consistent behavior across different report types. It doesn't introduce any issues with existing functionality.
To make the code more explicit and easier to maintain, you might consider using a constant for the excluded path:
const CUSTOM_REPORT_PATH = '/reports/custom'; // ... {!path.includes(CUSTOM_REPORT_PATH) && ( // View component )}This change enhances readability and allows for easier updates if more paths need to be excluded in the future.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the rendering behavior for different report paths. # Test: Search for the usage of `'/reports/spending'` path. # Expect: Occurrences in other components that might be impacted by this change. rg --type typescript $'/reports/spending' # Test: Search for the usage of other report paths (excluding custom and spending). # Expect: Occurrences of other report paths to verify the rendering behavior. rg --type typescript $'/reports/(?!custom|spending)\w+'Length of output: 376
Script:
#!/bin/bash # Description: Verify the rendering behavior for different report paths. # Search for the usage of '/reports/spending' path rg --type ts '/reports/spending' # Search for the usage of '/reports/custom' path rg --type ts '/reports/custom' # Search for other report paths rg --type ts '/reports/\w+' # Search for conditional rendering related to report paths ast-grep --lang typescript --pattern 'path.includes("/reports/")' # Search for Switch or conditional statements related to report paths ast-grep --lang typescript --pattern 'switch ($path) { $$$ case "/reports/$_": $$$ }'Length of output: 3398
packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx (5)
35-41: LGTM!The change to the
monthsproperty in thePayloadItemtype provides more flexibility by allowing a dynamic set of month entries. This is a good improvement to the data structure.
48-49: LGTM!The changes to the
balanceTypeOpandselectionproperties in theCustomTooltipPropstype enforce stricter typing. This improves the clarity of the expected values and reduces the risk of incorrect usage. Good job on enhancing the type safety!
65-65: LGTM!The addition of optional chaining (
?.) in several places improves the robustness of the code by safely accessing properties that may not exist in themonthsobject. This prevents potential runtime errors. Nice work on handling the optional properties gracefully!Also applies to: 88-88, 92-92, 108-108, 112-112
128-128: LGTM!The change to the
modeproperty in theSpendingGraphPropstype enforces stricter typing and improves clarity. The adjustments to thethisMonthMaxandselectionMaxlogic ensure that the code remains robust against undefined values. These changes enhance the type safety and reliability of the code. Well done!Also applies to: 144-158
161-167: LGTM!The explicit type definition for the tick formatter enhances type safety for the tick formatting function. This is a good practice to ensure that the function receives and returns the expected types. Nice work on improving the type safety!
packages/desktop-client/src/components/reports/Overview.tsx (1)
548-548: Excellent addition of thewidgetIdprop to support saving spending widgets!The
widgetIdprop, set to the unique identifieritem.i, is a crucial change that enables theSpendingCardcomponent to manage its state and settings based on the widget instance. This aligns perfectly with the PR objective of transitioning fromLocalPrefsto a widget-based approach for saving spending widgets within dashboards.packages/desktop-client/src/components/reports/reports/Spending.tsx (3)
45-57: LGTM!The function is using the
useParamshook to retrieve the widget ID from the URL and theuseWidgethook to fetch the spending widget data, which are good practices for dynamic routing and data fetching. It also handles the loading state by displaying aLoadingIndicatorwhile the widget data is being fetched, which is a good practice for user experience.
136-157: LGTM!The
onSaveWidgetfunction is saving the widget settings, which is a good practice for user experience.
37-40: Skipping the review of thecalculateTimeRangefunction.The
calculateTimeRangefunction is imported from an external file and is being used correctly to calculate the start and end dates based on the time frame and mode. Since the function is not defined in this file, it cannot be reviewed in detail.
|
Thanks @carkom ! This has now been fixed: I aligned the date logic between the spending report and the cashflow/networth reports. Also sligthly updated the paddings to better align with the other reports. |
I'm not sure it works like this. There's a couple issues with aligning to network/cashflow.
|
|
@MatissJanis |
hmm.. yea, I see your point. The spending report doesn't actually have a "start" & "end" date as the others do. Those are just two dates we compare. I updated the PR to reflect that (removed start/end variable usages and instead using the previous - compare/compareTo). Also removed Sorry for so many rounds of review. The dashboard interface is the part that worries me the most - we need to get that right the first time. Other UX improvements can be patched later, but not the interface so much. LMK if you have any more feedback/ideas what we should change. Happy to do more as part of this PR too :) |
| variant={isLive ? 'primary' : 'normal'} | ||
| onPress={() => setIsLive(state => !state)} | ||
| > | ||
| {isLive ? 'Live' : 'Static'} |
There was a problem hiding this comment.
Looks like you've translated everything but this one, just highlighting in case you missed it.
Allowing to save spending widgets in dashboards. This required a bit of refactoring to move from
LocalPrefsto widgets.This is a breaking change sadly (i.e. the saved local-pref values will be lost). But IMO that's fine since this is an experimental feature that is expected to have breaking changes.