Fix DailyDataGoal not updating when date changes #318
Conversation
WalkthroughThe pull request introduces changes to the Changes
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: 1
🧹 Outside diff range and nitpick comments (3)
src/components/container/DailyDataGoal/DailyDataGoal.stories.tsx (3)
Line range hint
98-117: LGTM: Story renamed for clarityThe renaming of the story from SleepGoalLive to FitbitSleepMinutesGoalLive improves specificity and clarity. This is a good change.
Consider updating the
titleprop in the story args to reflect the new name, e.g., change "Worn to Sleep" to "Fitbit Sleep Minutes Goal" for consistency.
118-122: LGTM: New renderWithDRC function addedThe addition of the
renderWithDRCfunction is a good solution to address the issue of DailyDataGoal not updating when the date changes. Wrapping the component with DateRangeCoordinator should allow it to react to date changes properly.Consider making the interval type configurable by passing it as an argument to the function. This would allow for more flexibility in testing different interval types:
const renderWithDRC = (args: DailyDataGoalProps, intervalType: "Day" | "Week" | "Month" = "Day") => ( <Layout colorScheme='auto'> <DateRangeCoordinator intervalType={intervalType}> <DailyDataGoal {...args} /> </DateRangeCoordinator> </Layout> );
124-153: LGTM: New LiveGarminSleepScore story addedThe addition of the LiveGarminSleepScore story is a good demonstration of using the DailyDataGoal component with the new renderWithDRC function. It provides a clear example of how the component works with a different type of sleep data (Garmin Sleep Score).
For consistency with the FitbitSleepMinutesGoalLive story, consider adding a
subtitleprop to the args object. For example:subtitle: "Track your sleep quality",This would make the story structure more consistent across different data types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/components/container/DailyDataGoal/DailyDataGoal.stories.tsx (3 hunks)
- src/components/container/DailyDataGoal/DailyDataGoal.tsx (2 hunks)
🔇 Additional comments (5)
src/components/container/DailyDataGoal/DailyDataGoal.stories.tsx (2)
7-7: LGTM: DateRangeCoordinator import addedThe addition of the DateRangeCoordinator import is appropriate and aligns with the PR objective of fixing the issue with DailyDataGoal not updating when the date changes.
Line range hint
1-153: Overall assessment: Changes effectively address the PR objectivesThe modifications to this file successfully address the issue of DailyDataGoal not updating when the date changes. The introduction of the
renderWithDRCfunction and its use in the new LiveGarminSleepScore story demonstrate how the DateRangeCoordinator can be used to manage date changes.Key improvements:
- Addition of DateRangeCoordinator import
- Renaming of the FitbitSleepMinutesGoalLive story for clarity
- Introduction of the renderWithDRC function
- Addition of a new LiveGarminSleepScore story
These changes not only fix the reported issue but also improve the overall structure and clarity of the Storybook stories for the DailyDataGoal component. The minor suggestions provided in the review comments can further enhance consistency and flexibility.
src/components/container/DailyDataGoal/DailyDataGoal.tsx (3)
8-8: Import changes align with PR objectivesThe addition of
endOfDayandstartOfDayimports from 'date-fns' is consistent with the PR's goal of limiting the data search to a single day. The removal of theaddfunction import (not visible in the provided code) further supports this change in date handling logic.
36-37: Date handling logic now correctly limits data to a single dayThe updated logic for calculating
startDateandendDateusingstartOfDayandendOfDayfunctions effectively addresses the PR objective. This change ensures that:
- The data is queried only for the current day, from the start to the end of the day.
- The previous issue of searching for data across three days is resolved.
This implementation provides a more precise and standardized way of defining day boundaries, improving the accuracy of the daily data goal display.
Line range hint
1-99: Overall assessment: Changes effectively address PR objectivesThe modifications made to the
DailyDataGoalcomponent successfully address the main objectives of the PR:
- The date handling logic now correctly limits data queries to a single day, resolving the issue of searching across three days.
- The component now updates properly when the date changes, thanks to the updated dependency array in the
useInitializeViewhook.These changes improve the accuracy and responsiveness of the daily data goal display, enhancing the user experience as intended. The code maintains good style and structure, and no security issues are apparent in the changes.
To further improve the implementation, consider addressing the potential issue with the dependency array as mentioned in the previous comment.
|
|
||
| queryDailyData(props.dailyDataType, startDate, endDate, props.previewState === "Default").then((results) => { | ||
| setDailyValue(results[getDayKey(date)] || 0); | ||
| }); | ||
| }, ["externalAccountSyncComplete"], []); | ||
| }, ["externalAccountSyncComplete"], [dateRangeContext?.intervalStart]); |
There was a problem hiding this comment.
Dependency array update improves date change responsiveness, but needs refinement
The addition of dateRangeContext?.intervalStart to the dependency array is a positive change that addresses the PR objective. It ensures that the useInitializeView hook re-runs when the date range changes, fixing the issue of the DailyDataGoal not updating on date changes.
However, there's a potential issue with the current implementation:
The dependency array still includes the string "externalAccountSyncComplete". This is likely a mistake, as strings in dependency arrays are treated as static values and won't trigger re-runs of the effect.
To fix this, consider the following change:
- }, ["externalAccountSyncComplete"], [dateRangeContext?.intervalStart]);
+ }, [dateRangeContext?.intervalStart]);If "externalAccountSyncComplete" is meant to be a variable or prop, make sure it's properly referenced:
- }, ["externalAccountSyncComplete"], [dateRangeContext?.intervalStart]);
+ }, [externalAccountSyncComplete, dateRangeContext?.intervalStart]);This will ensure that the effect runs correctly based on both the date range changes and any external account sync status updates.
📝 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.
| }, ["externalAccountSyncComplete"], [dateRangeContext?.intervalStart]); | |
| }, [dateRangeContext?.intervalStart]); |
Overview
When using the DailyDataGoal within a DateRangeCoordinator (already supported), when the user navigates to a new day
Current Behavior:
The data does not update
The data is searched for across a span of 3 days
Expected Behavior:
The data should reflect the current date
The data is searched for should be limited to one day
Security
REMINDER: All file contents are public.
Describe briefly what security risks you considered, why they don't apply, or how they've been mitigated.
Checklist
Testing
Documentation
Consider "Squash and merge" as needed to keep the commit history reasonable on
main.Reviewers
Assign to the appropriate reviewer(s). Minimally, a second set of eyes is needed ensure no non-public information is published. Consider also including:
Summary by CodeRabbit
New Features
LiveGarminSleepScorewith updated goal settings and messages.Improvements
Refactor
SleepGoalLivetoFitbitSleepMinutesGoalLiveand updated rendering method for consistency.