LF-4746(1) FinancesDateRangeSelector minor refactors and bugfixes #3708

kathyavini · 2025-03-06T22:31:28Z

Description

This PR is a preparation PR for the creation of the <SensorsDateRangeSelector /> filter + hook.

It prepares the existing Finances/DateRangeSelector implementation for re-use in a general way by some renaming and conversion to TypeScript. In looking at this, I found a few bugs in Finances and fixed them. Most of the code changes are just renames! That is the only reason so many files are touched.

Changes in this PR:

I have reanamed the import of the Finances/DateRangeSelector component to FinancesDateRangeSelector throughout the app. It was sometimes imported as such and sometimes as <DateRangeSelector /> which made code search more difficult. We also have a different component components/DateRangeSelector so I wanted to avoid the naming conflict.
the hook useDateRangeSelector.js imports and uses the action and selector for the Finances Redux store. To make this dependency clear I have renamed it to useFinancesDateRange.ts and moved it into Finances/. It was previously co-located with components/DateRangeSelector but that component is redux-store agnostic
both of these files have been converted to TS (hopefully with zero modification of their logic)
The buggy behaviour of custom date range (how it resets and the date picker loads as open each time you navigate to a different page in finances) was due to the .onChange() handler of the base <DateRangeSelector /> being called upon first load by a call to setSelectedDateRangeOption.
- I'll leave a note in the code in case I did not understand the use of this correctly, but I believe the functionality will be correct if setSelectedDateRangeOption is just not called on the first render.
- There are a few Jira tickets that refer to this issue and I have linked two below
Loading the transaction list used to include a reset of the date range back to "Year as Date". I don't know if we have strictly ever classified this as a bug, so I started a thread in #ux_ui about it. To me it felt quite buggy and I have removed it. At some point a proper implementation of resetting when leaving finances can be added.
one more bug fix related to the Redux filter data (see Jira links and conversation below)

Jira links:

Bug tickets related to the custom range behaviour (the reset + open picker)

Bug tickets related to incomplete reset of Redux state as spotted by @SayakaOno 🙏

https://lite-farm.atlassian.net/browse/LF-3958

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Passes test case
UI components visually reviewed on desktop view
UI components visually reviewed on mobile view
Other (please explain)

Checklist:

I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
The precommit and linting ran successfully
I have added or updated language tags for text that's part of the UI
I have added "MISSING" for all new language tags to languages I don't speak
I have added the GNU General Public License to all new files

…tch component

…nnected to the finance redux store

…date picker) when the component first mounts

…to clearOldFarmStateSaga() In fetchAllSaga() it was being called each time the transaction list page was loaded

…nent and hook

kathyavini · 2025-03-06T22:43:22Z

packages/webapp/src/components/DateRangeSelector/index.jsx

@@ -67,11 +67,18 @@ export default function DateRangeSelector({
    }
  }, [isValid, isCustomDatePickerOpen]);

+  const isInitialRender = useRef(true);


This useEffect hook was added in this PR:

LF-3796, LF-3765 fix finances crashes #2963

to solve a crash we used to see in finances, but I think we all (mostly myself as reviewer on that PR 😅🙇 ) missed that it also triggered buggy behaviour on custom date range by clearing and opening the picker on every navigation within Finances.

The bug happens because the ReactSelect's .onChange() is called after this useEffect runs on the initial render, and that opens the date picker and clears the value. I believe that the initial render should be safely covered by the defaultValue prop on the ReactSelect, which is why I think it's okay to skip it and have done so here, but @antsgar please let me know if you remember more context about how this contributed to the crash and if I just reverted something important! 🙏

Update Mar 12: in tech daily we decided to remove this useEffect() entirely.

kathyavini · 2025-03-06T22:46:05Z

packages/webapp/src/containers/Finances/saga.js

@@ -516,7 +514,6 @@ export function* fetchAllDataSaga() {
  ]);
  yield put(setSelectedExpenseTypes([]));
  yield put(resetTransactionsFilter());
-  yield put(setDateRange({ option: dateRangeOptions.YEAR_TO_DATE }));


This is the cause of the Finances date range resetting to Year to Date each time transactions list is loaded. I have moved the same call to clearOldFarmStateSaga() below, since it was switching between farms I believe we were finding problematic.

SayakaOno

This feels amazing! I was expecting it would need more changes to fix the behaviour, but the fixes seem effective and powerful! ❤️

The one thing I couldn't spot just from reviewing the code is that the custom range persists when switching farms. (set a custom range -> switch farms (no need to go to finances, though I did in the recording)-> switch back to the original farm and go to finances) Are you able to reproduce it?

Screen.Recording.2025-03-07.at.10.19.07.AM.mov

kathyavini · 2025-03-07T19:19:38Z

@SayakaOno thank you so much for spotting that!! ❤️ 🙏 It wasn't even just the custom dates, but everything other than the option was maintained (i.e. the string and computed dates were out of alignment too 😬)... I was foiled by old-style Redux and didn't realize the objects were being merged instead of replaced by that reducer.

Edit: actually since that action call was in the original code too there may even be an additional bug ticket I can track down related to this. Found one! https://lite-farm.atlassian.net/browse/LF-3958

kathyavini · 2025-03-07T19:22:56Z

packages/webapp/src/components/DateRangeSelector/types.ts

+  endDate?: string | Moment;
+}
+
+export interface DateRangeSelection {


Unrelated to anything else in the PR, do you have any suggestions for this type? 😅 I'm not loving the name I picked here -- but is DateRange too unspecific? Maybe DateRangeData? It's the shape of the date range data that the hooks and components act on.

It could be stored in Redux or in component state, dependent on use case... maybe DateRangeState?

I think I usually go with data or info, but both data and state sound good to me!

Thank you, I'll update all to DateRangeData in the sensors PR 🙏

kathyavini · 2025-03-07T20:26:13Z

I found another bug in the finance date range filters that seemed an easy fix so I had initially added it as well:

Custom date range filter from finances dashboard isn't correctly carried into export date range filter

Actually logically speaking to me the original useEffect() seemed sound so I'm going to spend a bit more time trying to figure out why the solution works and I'll update my comment here when I do 😆 I thought it was stale component state from the time the <Report /> component first rendered (and thus the solution) but it actually seems to be a resetting to default value even after the useEffect has run. The solution still works but I would like to spend more time figuring out why.

However, I'll just open it as a separate ticket since it's not dependent on the others and it's still a bit mysterious to me. Apologies for the force-pushing 🙇

…sensor-readings

Calling sequentially when closing picker was passing stale state in the second call

kathyavini · 2025-03-12T19:37:03Z

I'm embarrassed to admit how long it took me to debug why the dates were not setting properly within the onClickAway() handler 😅 It turns out two sequential calls to changeDateMethod() don't work as expected since the second call will use stale state for the parameter it's not concerned with. Therefore I have made the following changes:

updated changeDateMethod to set the full range at once, which is appropriate to how it's now being triggered
moved this call to the onClickAway() handler
removed the clearing of the date range upon opening the picker
- this is not essential for the timing fix, but it seems more in the spirit of the timing we are going for (where changes are only committed to the store once dates are selected and the picker closed)
- in the context of charts it will mean that opening the custom picker will not trigger an API call
removed the useEffect() as discussed today in tech daily

Thank you in advance for your re-reviews 🙏

SayakaOno

❤️

antsgar · 2025-03-13T14:10:54Z

Looks good, thank you for all the improvements to the component! ❤️

kathyavini added 7 commits March 5, 2025 13:12

LF-4746 For clarity, rename imports of FinanceDateRangeSelector to ma…

7cd0364

…tch component

LF-4746 Rename and relocate useDateRangeSelector to indicate it is co…

32804ab

…nnected to the finance redux store

LF-4746 Use Ref to avoid triggering the onChange (and so opening the …

fb1b384

…date picker) when the component first mounts

LF-4746 Convert Finances/DateRangeSelector and constants file to TS

985e57c

LF-4746 Move the reset of the Finances dateRange from fetchAllSaga() …

64c6cea

…to clearOldFarmStateSaga() In fetchAllSaga() it was being called each time the transaction list page was loaded

LF-4746 Convert hook to TS and create types file shared between compo…

d633f96

…nent and hook

LF-4746 Some more naming adjusts and undoing some AI suggests

db74062

kathyavini added bug Something isn't working enhancement New feature or request labels Mar 6, 2025

kathyavini self-assigned this Mar 6, 2025

kathyavini requested review from a team as code owners March 6, 2025 22:31

kathyavini requested review from Duncan-Brain and SayakaOno and removed request for a team March 6, 2025 22:31

kathyavini changed the title ~~LF-4746(1) Create date range filter for sensor readings -- FinancesDateRangeSelector minor refactors and bugfixes~~ LF-4746(1) FinancesDateRangeSelector minor refactors and bugfixes Mar 6, 2025

kathyavini requested a review from antsgar March 6, 2025 22:34

kathyavini commented Mar 6, 2025

View reviewed changes

LF-4746 Combine constants and types files

f6cf506

SayakaOno reviewed Mar 7, 2025

View reviewed changes

LF-4746 Create and use action to reset date range

a5f8a30

kathyavini commented Mar 7, 2025

View reviewed changes

kathyavini force-pushed the LF-4746-create-date-range-filter-for-sensor-readings branch from a032055 to 7886133 Compare March 7, 2025 20:02

kathyavini force-pushed the LF-4746-create-date-range-filter-for-sensor-readings branch from 7886133 to a5f8a30 Compare March 7, 2025 20:37

Merge branch 'integration' into LF-4746-create-date-range-filter-for-…

2418b66

…sensor-readings

kathyavini mentioned this pull request Mar 8, 2025

LF-4746 (2) Create date range filter for sensor readings #3709

Merged

16 tasks

kathyavini requested a review from SayakaOno March 10, 2025 15:43

SayakaOno previously approved these changes Mar 10, 2025

View reviewed changes

kathyavini added 3 commits March 12, 2025 10:04

LF-4746 Remove useEffect that was triggering onChange() on mount

0374bb5

LF-4746 Update changeDateMethod to update the whole range at once

3a7e064

Calling sequentially when closing picker was passing stale state in the second call

LF-4746 Don't reset dates when opening picker

a1afaad

kathyavini dismissed SayakaOno’s stale review via a1afaad March 12, 2025 19:28

kathyavini requested a review from SayakaOno March 12, 2025 19:38

SayakaOno approved these changes Mar 12, 2025

View reviewed changes

antsgar approved these changes Mar 13, 2025

View reviewed changes

antsgar added this pull request to the merge queue Mar 13, 2025

Merged via the queue into integration with commit 3d9974f Mar 13, 2025
5 checks passed

antsgar deleted the LF-4746-create-date-range-filter-for-sensor-readings branch March 13, 2025 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LF-4746(1) FinancesDateRangeSelector minor refactors and bugfixes #3708

LF-4746(1) FinancesDateRangeSelector minor refactors and bugfixes #3708

kathyavini commented Mar 6, 2025 •

edited

Loading

kathyavini Mar 6, 2025 •

edited

Loading

kathyavini Mar 12, 2025

kathyavini Mar 6, 2025 •

edited

Loading

SayakaOno left a comment

kathyavini commented Mar 7, 2025 •

edited

Loading

kathyavini Mar 7, 2025 •

edited

Loading

SayakaOno Mar 10, 2025

kathyavini Mar 10, 2025

kathyavini commented Mar 7, 2025 •

edited

Loading

kathyavini commented Mar 12, 2025 •

edited

Loading

SayakaOno left a comment

antsgar commented Mar 13, 2025

LF-4746(1) FinancesDateRangeSelector minor refactors and bugfixes #3708

LF-4746(1) FinancesDateRangeSelector minor refactors and bugfixes #3708

Conversation

kathyavini commented Mar 6, 2025 • edited Loading

kathyavini Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

kathyavini Mar 12, 2025

Choose a reason for hiding this comment

kathyavini Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

SayakaOno left a comment

Choose a reason for hiding this comment

kathyavini commented Mar 7, 2025 • edited Loading

kathyavini Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

SayakaOno Mar 10, 2025

Choose a reason for hiding this comment

kathyavini Mar 10, 2025

Choose a reason for hiding this comment

kathyavini commented Mar 7, 2025 • edited Loading

kathyavini commented Mar 12, 2025 • edited Loading

SayakaOno left a comment

Choose a reason for hiding this comment

antsgar commented Mar 13, 2025

kathyavini commented Mar 6, 2025 •

edited

Loading

kathyavini Mar 6, 2025 •

edited

Loading

kathyavini Mar 6, 2025 •

edited

Loading

kathyavini commented Mar 7, 2025 •

edited

Loading

kathyavini Mar 7, 2025 •

edited

Loading

kathyavini commented Mar 7, 2025 •

edited

Loading

kathyavini commented Mar 12, 2025 •

edited

Loading