feat: implement patient activities backend sync on navigation#213
feat: implement patient activities backend sync on navigation#213nthsneha wants to merge 4 commits into
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 38 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWrapped DailyActivitiesScreen's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen as DailyActivitiesScreen
participant Provider as TaskProvider
participant API as Backend API
User->>Screen: Press back (AppBar or system)
Screen->>Provider: updateActivityInBackground()
activate Provider
Provider->>Provider: if _allTasks.isEmpty -> return
Provider->>Provider: _syncStatus = loading\nnotifyListeners
Provider->>API: updateActivityCompletion(_allTasks)
activate API
API-->>Provider: response (success / error)
deactivate API
Provider->>Provider: _syncStatus = resultStatus\nnotifyListeners
deactivate Provider
Screen->>Screen: Navigator.pop(context)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
patient/lib/provider/task_provider.dart (1)
43-45: Unusedtasksparameter — method always uses_allTasks.The
tasksparameter is never used; line 45 always passes_allTasksto the repository. This is pre-existing code, but sinceupdateActivityInBackgroundalso passes_allTasks, the parameter is redundant and could cause confusion.Consider either using the parameter or removing it to clarify the intent.
♻️ Suggested fix: Use the parameter
Future<void> updateActivityCompletion(List<PatientTaskModel> tasks) async { try { - final result = await _patientRepository.updateActivityCompletion(tasks: _allTasks, activityId: _activityId, activitySetId: _activitySetId); + final result = await _patientRepository.updateActivityCompletion(tasks: tasks, activityId: _activityId, activitySetId: _activitySetId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patient/lib/provider/task_provider.dart` around lines 43 - 45, The method updateActivityCompletion has an unused tasks parameter (it always passes _allTasks into _patientRepository.updateActivityCompletion); either remove the tasks parameter from updateActivityCompletion and all callers (e.g., updateActivityInBackground) or, preferably, use the passed-in tasks: change the repository call in updateActivityCompletion to pass the tasks parameter instead of _allTasks (ensure calls that expect to pass _allTasks remain correct), keeping references to updateActivityCompletion and _patientRepository.updateActivityCompletion to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patient/lib/presentation/activities/daily_activities_screen.dart`:
- Around line 64-67: Wrap the existing Scaffold in a PopScope so all back
navigation (system back, swipe-back, predictive gestures) triggers the sync;
inside the PopScope onWillPop callback call
context.read<TaskProvider>().updateActivityInBackground() and then return true
(or await the sync if you need to block) instead of relying only on the AppBar
onPressed/Navigator.pop; update references to Navigator.pop(...) in the action
button to still call Navigator.pop after triggering updateActivityInBackground
if you want immediate pop behavior.
In `@patient/lib/provider/task_provider.dart`:
- Around line 86-93: The try/catch around await
updateActivityCompletion(_allTasks) is ineffective because
updateActivityCompletion swallows errors; instead, after awaiting
updateActivityCompletion(_allTasks) check the provider's result state (e.g.
inspect _apiStatus or a returned success flag set by updateActivityCompletion)
and set _syncStatus = ApiStatus.success only when that result indicates success,
otherwise set _syncStatus = ApiStatus.failure; keep the notifyListeners() calls
around the status changes and reference updateActivityCompletion, _allTasks,
_apiStatus (or the chosen result flag), and _syncStatus when implementing this
conditional status update.
---
Nitpick comments:
In `@patient/lib/provider/task_provider.dart`:
- Around line 43-45: The method updateActivityCompletion has an unused tasks
parameter (it always passes _allTasks into
_patientRepository.updateActivityCompletion); either remove the tasks parameter
from updateActivityCompletion and all callers (e.g., updateActivityInBackground)
or, preferably, use the passed-in tasks: change the repository call in
updateActivityCompletion to pass the tasks parameter instead of _allTasks
(ensure calls that expect to pass _allTasks remain correct), keeping references
to updateActivityCompletion and _patientRepository.updateActivityCompletion to
locate and update the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e34721f4-c134-4cb0-81ae-e30bbbd93c2d
📒 Files selected for processing (2)
patient/lib/presentation/activities/daily_activities_screen.dartpatient/lib/provider/task_provider.dart
There was a problem hiding this comment.
♻️ Duplicate comments (1)
patient/lib/presentation/activities/daily_activities_screen.dart (1)
58-75:⚠️ Potential issue | 🟠 MajorDouble sync triggered when using AppBar back button.
When the user taps the AppBar back button,
updateActivityInBackground()is called twice:
- First in
IconButton.onPressed(line 72)- Then again in
onPopInvokedWithResultwhendidPopis true (line 62)Since
updateActivityInBackground()has no concurrency guards (seetask_provider.dart:82-92), this results in two simultaneous network requests to the backend.Choose one trigger point. Given that
PopScopewithcanPop: falseproperly intercepts all navigation methods (system back, swipe gestures, AppBar button), that approach is cleaner:🛠️ Proposed fix using canPop: false pattern
return PopScope( - canPop: true, + canPop: false, onPopInvokedWithResult: (didPop, result) { - if (didPop) { - context.read<TaskProvider>().updateActivityInBackground(); - } + if (didPop) return; + context.read<TaskProvider>().updateActivityInBackground(); + Navigator.pop(context); }, child: Scaffold( appBar: AppBar( title: const Text('Daily Activities'), centerTitle: true, leading: IconButton( icon: const Icon(Icons.arrow_back_ios_new_rounded), - onPressed: () { - context.read<TaskProvider>().updateActivityInBackground(); - Navigator.pop(context); - }, + onPressed: () => Navigator.maybePop(context), ), ),With this pattern:
canPop: falseintercepts all pop attemptsonPopInvokedWithResulthandles sync for all navigation methods (system back, swipe, AppBar)IconButtondelegates toNavigator.maybePop()which triggers the PopScope callback- Single sync call regardless of how the user navigates back
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patient/lib/presentation/activities/daily_activities_screen.dart` around lines 58 - 75, The AppBar back button currently triggers updateActivityInBackground() twice because it calls it directly in IconButton.onPressed while PopScope.onPopInvokedWithResult also calls it when didPop is true; remove the direct call from IconButton.onPressed and change the button to call Navigator.maybePop() so PopScope (with canPop: false) handles all navigation and a single call to updateActivityInBackground() happens in onPopInvokedWithResult; update references to PopScope, canPop, onPopInvokedWithResult, IconButton.onPressed, Navigator.maybePop(), and updateActivityInBackground() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patient/lib/presentation/activities/daily_activities_screen.dart`:
- Around line 58-75: The AppBar back button currently triggers
updateActivityInBackground() twice because it calls it directly in
IconButton.onPressed while PopScope.onPopInvokedWithResult also calls it when
didPop is true; remove the direct call from IconButton.onPressed and change the
button to call Navigator.maybePop() so PopScope (with canPop: false) handles all
navigation and a single call to updateActivityInBackground() happens in
onPopInvokedWithResult; update references to PopScope, canPop,
onPopInvokedWithResult, IconButton.onPressed, Navigator.maybePop(), and
updateActivityInBackground() accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e83d56d3-5c94-4ea7-ab2f-97c728a18544
📒 Files selected for processing (2)
patient/lib/presentation/activities/daily_activities_screen.dartpatient/lib/provider/task_provider.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- patient/lib/provider/task_provider.dart
|
Hey @nthsneha, went through the diff. The overall approach is reasonable but there are a few things to fix before this merges. What's working
Blockers
return PopScope(
canPop: false,
onPopInvokedWithResult: (didPop, result) {
if (didPop) return;
context.read<TaskProvider>().updateActivityInBackground();
Navigator.pop(context);
},
child: Scaffold(
appBar: AppBar(
leading: IconButton(
icon: const Icon(Icons.arrow_back_ios_new_rounded),
onPressed: () => Navigator.maybePop(context),
),
),
Future<void> updateActivityInBackground() async {
if (_allTasks.isEmpty || _syncStatus == ApiStatus.loading) return;
...
}
Minor
Fix the double sync and add the concurrency guard and this is good to merge. |
|
@Varadraj75 |
|
@nthsneha, the fixes from the last round look good. One thing still needs addressing before this merges. BlockerThere are two await context.read<TaskProvider>().updateActivityInBackground(); // fine
final taskProvider = context.read<TaskProvider>(); // unsafe, this is after the awaitCalling onPopInvokedWithResult: (didPop, result) async {
if (didPop) return;
final taskProvider = context.read<TaskProvider>();
await taskProvider.updateActivityInBackground();
if (taskProvider.syncStatus == ApiStatus.failure) {
if (mounted) {
ScaffoldMessenger.of(context).showSnackBar(
const SnackBar(
content: Text('Failed to save your progress'),
backgroundColor: Colors.red,
duration: Duration(seconds: 2),
),
);
}
}
if (mounted) Navigator.pop(context);
},Everything else looks good
One line change and this is ready. |
2 participants
📝 Description
Implements automatic activity completion tracking by syncing task state to the backend when users leave the Daily Activities screen.
🔧 Changes Made
updateActivityInBackground()method to TaskProvider for backend sync✅ Checklist
Summary by CodeRabbit