-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: Goal widget improvements #3947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1. Fix progress scale: 1/5 now shows correctly as 20% (removed minValue from calculation) 2. Daily reflection sends FROM Omi AI, not from user 3. Pre-load widget with cached data for instant display 4. Advice limited to 50 words (3-4 lines) and uses GPT-4.1 for better quality 5. Advice text never clips (full text always visible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces several valuable improvements to the Goal widget, including fixing the progress calculation, enhancing the AI-generated advice, and implementing a caching mechanism for a faster, smoother user experience. The changes are well-aligned with the goals described. My review focuses on two critical issues: a logic error in the progress percentage calculation for goals with negative targets, and a race condition in the new caching implementation that could lead to displaying stale data, which violates the rule of re-validating cached objects. Addressing these will ensure the reliability and correctness of the new features.
| if (targetValue <= 0) return currentValue > 0 ? 1.0 : 0.0; | ||
| return (currentValue / targetValue).clamp(0.0, 1.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for handling targetValue <= 0 is incorrect and will lead to wrong progress calculations for goals with negative targets. For example, if the target is -10 and the current value is -5, the progress should be 50%, but this code will return 0%. A division-by-zero error only occurs when targetValue is exactly 0. The clamp(0.0, 1.0) call correctly handles cases with negative targets if the division is allowed to happen.
| if (targetValue <= 0) return currentValue > 0 ? 1.0 : 0.0; | |
| return (currentValue / targetValue).clamp(0.0, 1.0); | |
| if (targetValue == 0) return currentValue > 0 ? 1.0 : 0.0; | |
| return (currentValue / targetValue).clamp(0.0, 1.0); |
| // Load cached data immediately (sync), then refresh in background | ||
| _loadCachedDataSync(); | ||
| _loadGoal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling two async methods, _loadCachedDataSync and _loadGoal, without await from initState creates a critical race condition. The _loadCachedDataSync method (which is misleadingly named as it's async) might finish after _loadGoal, causing stale cached data to overwrite fresh data from the network. This can lead to unpredictable behavior and bugs.
To fix this, the loading logic should be orchestrated in a single async method to ensure sequential execution. For example:
@override
void initState() {
super.initState();
WidgetsBinding.instance.addObserver(this);
_loadInitialData();
}
void _loadInitialData() async {
// Await cached data loading to prevent race conditions.
await _loadCachedDataSync();
// Then await the network refresh.
await _loadGoal();
}This ensures that cached data is loaded first, and then the network refresh happens, preventing any data conflicts.
References
- When using a cached object as a fallback, re-validate it against the current filters (e.g.,
selectedDate) before returning to prevent serving stale data.
Fixes
1. Progress scale calculation fixed
minValuefrom calculation. NowprogressPercentage = currentValue / targetValue2. Daily reflection message sender
3. Widget pre-loading
4. Advice improvements
llm_medium(GPT-4.1) instead ofllm_miniFiles Changed
app/lib/backend/http/api/goals.dart- Fixed progressPercentage calculationapp/lib/pages/chat/page.dart- Fixed auto-message to send as AIapp/lib/pages/conversations/widgets/goal_tracker_widget.dart- Added caching, pre-loadingbackend/utils/llm/goals.py- Use better model, word limit