Integrate Health Connect for workout syncing and permissions - #40
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughAdds Health Connect integration: new service/interface and sync manager, HistoryManager/WorkoutProvider wiring, UI toggle with permission flows and lifecycle reconciliation, Android manifest and minSdk updates, model field for sync timestamp, tests, DI/exports, and a new dependency. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 54 minutes and 53 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workout-logger/android/app/build.gradle.kts`:
- Line 27: The change raising minSdk to 26 in the Android Gradle config (the
minSdk property in the build script) is a breaking change for older devices;
either restore the previous minSdk value to retain compatibility or, if the bump
is intentional (e.g., for Health Connect), add explicit release/migration notes
documenting that support for API <26 is dropped and include guidance for users
and CI (upgrade guidance, impact, and rationale). Update the build.gradle.kts
minSdk property handling (the minSdk assignment) accordingly and ensure the
release notes/migration docs reference this exact change.
In `@workout-logger/lib/screens/profile_screen.dart`:
- Line 352: The persisted flag settings.healthConnectEnabled can become stale;
update ProfileScreen to reconcile the UI with runtime state by calling the
Health Connect runtime checks (e.g., HealthConnectApi.isAvailable() and
HealthConnectApi.hasPermissions()) when the screen loads and when the app
resumes (initState and onResume/app lifecycle handler). If either check fails,
programmatically set settings.healthConnectEnabled = false and update the
toggle/status UI (the code around the enabled variable and the toggle/status
block) so the UI reflects actual availability/permissions; if checks pass ensure
the toggle matches the real permission state.
In `@workout-logger/lib/services/health_connect_service.dart`:
- Around line 104-105: The call to _buildSegments(session, sessionStart,
sessionEnd) uses positional arguments but must use named parameters; update the
_buildSegments function signature to accept named parameters (e.g.,
{_buildSegments({Session session, DateTime sessionStart, DateTime sessionEnd})}
or with required modifiers) and then change this invocation to
_buildSegments(session: session, sessionStart: sessionStart, sessionEnd:
sessionEnd); ensure any other callers and the function implementation are
updated to match the new named parameter names and maintain required annotations
if needed.
- Around line 14-57: The _segmentTypeMap is missing mappings for two built-in
exercise IDs causing them to default to otherWorkout; add entries
'cable_crunch': ExerciseSegmentType.crunch and 'russian_twist':
ExerciseSegmentType.crunch to the static const _segmentTypeMap so those segments
map correctly to ExerciseSegmentType.crunch (refer to _segmentTypeMap and
ExerciseSegmentType in health_connect_service.dart).
In `@workout-logger/lib/services/workout_provider.dart`:
- Around line 593-600: The Health Connect sync logic inside
WorkoutProvider.finishWorkout (the hcEnabled check,
_storage.getSetting('healthConnectEnabled') and the
_healthConnect.syncWorkoutSession(session).catchError(...) fire-and-forget)
should be extracted to a dedicated history/sync boundary (e.g., HistoryManager
or HealthSyncManager): remove the direct sync orchestration from
WorkoutProvider.finishWorkout, add a dependency (constructor-injected service
like HistoryManager/HealthSyncManager or an IHealthSyncService) and instead call
that service or emit a history-synced event from finishWorkout; implement the
sync orchestration (checking setting, invoking _healthConnect.syncWorkoutSession
and error handling) inside the new manager so WorkoutProvider only handles
finishing the session and delegating persistence/events to the new component.
- Around line 593-600: The Health Connect check and sync in finishWorkout must
be made best-effort so exceptions don’t abort the rest of finishWorkout; wrap
the await _storage.getSetting('healthConnectEnabled') and the subsequent
_healthConnect.syncWorkoutSession(session) call in a try/catch (or otherwise
handle Promise errors) so any thrown error is caught and logged but not
rethrown, and ensure the code that clears active-workout state (the existing
lines after finishWorkout that run after persistence) always runs; specifically
update the block referencing _storage.getSetting and
_healthConnect.syncWorkoutSession to catch both sync and setting-read errors and
return/continue without affecting finishWorkout completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17c401cb-822d-4efb-941d-7f7d97f5b5d0
📒 Files selected for processing (10)
workout-logger/android/app/build.gradle.ktsworkout-logger/android/app/src/main/AndroidManifest.xmlworkout-logger/lib/main.dartworkout-logger/lib/screens/profile_screen.dartworkout-logger/lib/services/health_connect_service.dartworkout-logger/lib/services/interfaces/health_connect_service_interface.dartworkout-logger/lib/services/interfaces/interfaces.dartworkout-logger/lib/services/settings_provider.dartworkout-logger/lib/services/workout_provider.dartworkout-logger/pubspec.yaml
… workout session syncing
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
…e project configuration
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workout-logger/android/app/src/main/AndroidManifest.xml`:
- Line 3: Remove the unnecessary READ_EXERCISE permission entry from the
AndroidManifest (the <uses-permission
android:name="android.permission.health.READ_EXERCISE" /> line) so the app
doesn't declare read access it doesn't use; locate the permission in the
AndroidManifest.xml and delete that <uses-permission> element, and double-check
there are no code paths invoking Health Connect read APIs that would require
READ_EXERCISE (the app only uses HealthDataType.exerciseSession.writePermission
and writeRecords()).
In `@workout-logger/lib/screens/profile_screen.dart`:
- Around line 36-128: Add unit tests covering the HC permission fallback and
lifecycle reconciliation: write tests that mock IHealthConnectService and
SettingsProvider (using Mockito) to assert _reconcileHealthConnectState clears
SettingsProvider.healthConnectEnabled when hc.isAvailable() returns false or
hc.hasPermissions() returns false, and that
didChangeAppLifecycleState(AppLifecycleState.resumed) triggers the same
reconciliation; also add tests for _requestHealthConnectPermission covering the
branch where requestPermissions() throws so you re-check hasPermissions() and
the branch where permissions are granted or denied, following Arrange–Act–Assert
(AAA) and placing tests under test/** with mocks for IHealthConnectService
methods (isAvailable, hasPermissions, requestPermissions) and verifying
SettingsProvider.setHealthConnectEnabled and snack UI side-effects; run flutter
test locally before committing.
In `@workout-logger/lib/services/health_connect_service.dart`:
- Around line 124-159: _buildSegments currently discards each
WorkoutSet.timestamp and fabricates evenly spaced timings; change it to use
per-set timestamps from WorkoutSet (models.WorkoutSet.timestamp) to build
segments: collect tuples of (type, reps, timestamp) for sets with reps>0, sort
them by timestamp, then generate ExerciseSessionSegmentEvent entries where each
segment's startTime is the set's timestamp (clamped to sessionStart) and endTime
is the next set's timestamp (or sessionEnd for the last), ensuring non-overlap
by clamping endTime to be >= startTime and <= sessionEnd; keep fallback behavior
for missing/nullable timestamps by placing those sets at the nearest neighbor or
as a final evenly spaced fallback so _buildSegments still returns valid
segments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9960b2d6-4394-4e7e-84f8-6eb548d28d8c
📒 Files selected for processing (7)
CLAUDE.mdworkout-logger/android/app/build.gradle.ktsworkout-logger/android/app/src/main/AndroidManifest.xmlworkout-logger/lib/screens/profile_screen.dartworkout-logger/lib/services/health_connect_service.dartworkout-logger/lib/services/interfaces/health_connect_service_interface.dartworkout-logger/lib/services/workout_provider.dart
…t with new History and HealthSync managers
10 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workout-logger/lib/screens/history_screen.dart`:
- Around line 428-436: The _SessionDetailsSheet currently declares a required
historyManager field that is never used; either remove the unused parameter from
the _SessionDetailsSheet class and its constructor and update the call site in
_SessionCard._showSessionDetails to stop passing historyManager, or implement a
minimal "Sync" action that uses the historyManager (e.g., add a Sync button in
_SessionDetailsSheet that calls historyManager.syncSession or the appropriate
method) and wire that into the existing UI/handlers (also update helper methods
like _editSession/_confirmDelete if they should reference historyManager).
Reference _SessionDetailsSheet, historyManager,
_SessionCard._showSessionDetails, _editSession, and _confirmDelete when making
the change.
In `@workout-logger/test/health_sync_manager_test.dart`:
- Around line 121-134: The test currently uses returnsNormally against an async
lambda so exceptions from the Future aren't observed; change the assertion to
await/assert the Future returned by HealthSyncManager.syncSession instead—e.g.,
call manager.syncSession(_makeSession()) and assert it completes without
throwing by using expect(..., completes) or await expectLater(..., completes);
keep the setup (hc = _MockHcService(shouldThrow: true),
settings.setHealthConnectEnabled(true)) and ensure you await the Future so the
test actually verifies that syncSession does not rethrow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 990dcd11-fe71-4540-8d0b-1c4e203b0cfd
📒 Files selected for processing (11)
workout-logger/lib/main.dartworkout-logger/lib/models/models.dartworkout-logger/lib/screens/history_screen.dartworkout-logger/lib/services/health_connect_service.dartworkout-logger/lib/services/interfaces/health_sync_manager_interface.dartworkout-logger/lib/services/interfaces/interfaces.dartworkout-logger/lib/services/managers/health_sync_manager.dartworkout-logger/lib/services/managers/history_manager.dartworkout-logger/lib/services/managers/managers.dartworkout-logger/lib/services/workout_provider.dartworkout-logger/test/health_sync_manager_test.dart
Comment on lines
+428
to
436
| final HistoryManager historyManager; | ||
| final ScrollController scrollController; | ||
|
|
||
| const _SessionDetailsSheet({ | ||
| required this.session, | ||
| required this.provider, | ||
| required this.historyManager, | ||
| required this.scrollController, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Unused historyManager parameter in _SessionDetailsSheet.
The historyManager field is declared and required in the constructor but never used in the widget's build method or any helper methods (_editSession, _confirmDelete). The details sheet has Edit and Delete buttons but no Sync action that would require historyManager.
Either remove the unused parameter or add the sync functionality that justifies it.
♻️ Option A: Remove unused parameter
class _SessionDetailsSheet extends StatelessWidget {
final WorkoutSession session;
final WorkoutProvider provider;
- final HistoryManager historyManager;
final ScrollController scrollController;
const _SessionDetailsSheet({
required this.session,
required this.provider,
- required this.historyManager,
required this.scrollController,
});Also update the call site in _SessionCard._showSessionDetails:
builder: (context, scrollController) => _SessionDetailsSheet(
session: session,
provider: provider,
- historyManager: historyManager,
scrollController: scrollController,
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/screens/history_screen.dart` around lines 428 - 436, The
_SessionDetailsSheet currently declares a required historyManager field that is
never used; either remove the unused parameter from the _SessionDetailsSheet
class and its constructor and update the call site in
_SessionCard._showSessionDetails to stop passing historyManager, or implement a
minimal "Sync" action that uses the historyManager (e.g., add a Sync button in
_SessionDetailsSheet that calls historyManager.syncSession or the appropriate
method) and wire that into the existing UI/handlers (also update helper methods
like _editSession/_confirmDelete if they should reference historyManager).
Reference _SessionDetailsSheet, historyManager,
_SessionCard._showSessionDetails, _editSession, and _confirmDelete when making
the change.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Owner
Author
|
@coderabbitai the edit and delete workouts are not working, from history page. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me investigate the history screen code to find what's causing the edit and delete issues. 🏁 Script executed: Length of output: 25348 Now let me check 🏁 Script executed: Length of output: 47929
Root CauseThe history screen was migrated to watch Bug 1 — DeleteBoth await provider.deleteWorkoutSession(session.id); // ❌ bypasses HistoryManager
Fix in // Before:
await provider.deleteWorkoutSession(session.id);
// After:
await historyManager.deleteSession(session.id);
Bug 2 — Edit
final provider = context.read<WorkoutProvider>();
await provider.updateWorkoutSession(updatedSession); // ❌ bypasses HistoryManager
Fix in // Before:
final provider = context.read<WorkoutProvider>();
await provider.updateWorkoutSession(updatedSession);
// After:
// HistoryManager.updateSession handles storage + notifyListeners
final historyManager = context.read<HistoryManager>();
await historyManager.updateSession(updatedSession);You'll also need to add the import at the top of import '../services/managers/history_manager.dart';Why
|
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workout-logger/lib/services/managers/history_manager.dart`:
- Around line 87-94: _onHcSynced currently replaces the entire session object in
_sessions with the incoming updated payload which can clobber newer local edits;
change the implementation to locate the existing session (using the same index
lookup), merge only the synchronization-specific fields (e.g., hcSyncedAt and
any other sync metadata) from the incoming WorkoutSession into the existing
session rather than assigning the whole object, then persist the merged session
via _storage.saveWorkoutSession(…) and handle errors as before; reference the
_onHcSynced method, the _sessions list, and _storage.saveWorkoutSession when
making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d572ec3-2e24-4954-9ec7-d16558bac92d
📒 Files selected for processing (6)
.github/workflows/test.ymlworkout-logger/lib/screens/workout_flow_screen.dartworkout-logger/lib/services/managers/history_manager.dartworkout-logger/lib/services/workout_provider.dartworkout-logger/test/health_sync_manager_test.dartworkout-logger/test/history_manager_test.dart
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
workout-logger/lib/services/health_connect_service.dart (1)
106-106: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUse named parameters for
_buildSegments(3 arguments).The function signature and call site still use positional parameters despite having 3 arguments.
💡 Proposed fix
- final segments = _buildSegments(session, sessionStart, sessionEnd); + final segments = _buildSegments( + session: session, + sessionStart: sessionStart, + sessionEnd: sessionEnd, + ); @@ - List<ExerciseSessionSegmentEvent> _buildSegments( - WorkoutSession session, - DateTime sessionStart, - DateTime sessionEnd, - ) { + List<ExerciseSessionSegmentEvent> _buildSegments({ + required WorkoutSession session, + required DateTime sessionStart, + required DateTime sessionEnd, + }) {As per coding guidelines: "Use named parameters for functions with 3 or more arguments in Dart".
Also applies to: 133-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/services/health_connect_service.dart` at line 106, Change the _buildSegments function to accept named parameters instead of positional ones and update all call sites (the call at _buildSegments(session, sessionStart, sessionEnd) and the ones around lines 133-137) to use those named arguments; specifically modify the _buildSegments signature to something like _buildSegments({required Session session, required DateTime sessionStart, required DateTime sessionEnd}) and then call it with _buildSegments(session: session, sessionStart: sessionStart, sessionEnd: sessionEnd) (and similarly for the other calls) so the function and all callers use named parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 51-55: Update the Codecov upload step ("Upload coverage to
Codecov") that uses codecov/codecov-action@v5 to include the fail_ci_if_error:
true input so upload failures (bad token, network errors) cause the job to fail;
modify the step definition where files and token are set to add
fail_ci_if_error: true alongside those inputs.
In `@workout-logger/lib/services/managers/history_manager.dart`:
- Around line 196-210: Add a short clarifying comment above the cache-only
mutation methods evictSession and patchSession stating that these methods
intentionally do not call onSessionsChanged because they only mutate the
in-memory _sessions and WorkoutProvider (or the caller) is responsible for
invoking onSessionsChanged when storage-backed changes occur; reference the
methods by name (evictSession, patchSession) and place the comment directly
above each method so future maintainers understand the design decision.
In `@workout-logger/lib/services/workout_provider.dart`:
- Around line 591-602: The current logic duplicates session state by letting
HistoryManager.addSession persist and cache the session
(HistoryManager._sessions) while WorkoutProvider also inserts it into its own
_sessions; to fix, avoid dual caching: when _historyManager != null, remove the
_sessions.insert(0, session) path and rely on HistoryManager._sessions (or,
alternatively, have HistoryManager.addSession return the canonical session and
replace WorkoutProvider._sessions insertion with a single authoritative update
using that returned session), ensure _clearDraft() still runs and that
evictSession/patchSession delegate to HistoryManager to keep a single source of
truth (references: _historyManager, addSession, HistoryManager._sessions,
_sessions, evictSession, patchSession, _storage.saveWorkoutSession,
_clearDraft).
---
Duplicate comments:
In `@workout-logger/lib/services/health_connect_service.dart`:
- Line 106: Change the _buildSegments function to accept named parameters
instead of positional ones and update all call sites (the call at
_buildSegments(session, sessionStart, sessionEnd) and the ones around lines
133-137) to use those named arguments; specifically modify the _buildSegments
signature to something like _buildSegments({required Session session, required
DateTime sessionStart, required DateTime sessionEnd}) and then call it with
_buildSegments(session: session, sessionStart: sessionStart, sessionEnd:
sessionEnd) (and similarly for the other calls) so the function and all callers
use named parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dadc3580-c003-4f98-8ca5-129d9362dfb2
📒 Files selected for processing (5)
.github/workflows/test.ymlworkout-logger/lib/screens/profile_screen.dartworkout-logger/lib/services/health_connect_service.dartworkout-logger/lib/services/managers/history_manager.dartworkout-logger/lib/services/workout_provider.dart
Comment on lines
+51
to
+55
| - name: Upload coverage to Codecov | ||
| uses: codecov/codecov-action@v5 | ||
| with: | ||
| files: workout-logger/coverage/lcov.info | ||
| token: ${{ secrets.CODECOV_TOKEN }} |
Contributor
There was a problem hiding this comment.
Consider adding fail_ci_if_error: true to surface upload failures.
Without this, a failed upload (bad token, network error) is silently swallowed and the step still shows green. The official Codecov action example explicitly includes fail_ci_if_error: true for exactly this reason.
⚙️ Proposed addition
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v5
with:
files: workout-logger/coverage/lcov.info
token: ${{ secrets.CODECOV_TOKEN }}
+ fail_ci_if_error: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yml around lines 51 - 55, Update the Codecov upload
step ("Upload coverage to Codecov") that uses codecov/codecov-action@v5 to
include the fail_ci_if_error: true input so upload failures (bad token, network
errors) cause the job to fail; modify the step definition where files and token
are set to add fail_ci_if_error: true alongside those inputs.
Comment on lines
+196
to
+210
| void evictSession(String sessionId) { | ||
| final index = _sessions.indexWhere((s) => s.id == sessionId); | ||
| if (index == -1) return; | ||
| _sessions = List.from(_sessions)..removeAt(index); | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| /// Replace a session in the in-memory list without touching storage. | ||
| void patchSession(WorkoutSession updated) { | ||
| final index = _sessions.indexWhere((s) => s.id == updated.id); | ||
| if (index == -1) return; | ||
| _sessions = List.from(_sessions)..[index] = updated; | ||
| _sessions.sort((a, b) => b.date.compareTo(a.date)); | ||
| notifyListeners(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Document the intentional omission of onSessionsChanged in cache-only mutations.
evictSession and patchSession don't call onSessionsChanged, unlike their storage-backed counterparts (deleteSession, updateSession). This is likely intentional since WorkoutProvider already handles the callback in its own flow, but adding a brief comment would clarify this for future maintainers.
💡 Suggested documentation
// ── Cache-only mutations (no storage I/O) ─────────────────────────────────
// Called by WorkoutProvider after it has already handled storage, so that
// HistoryManager's in-memory list stays in sync without a double-write.
+ // Note: These do NOT call onSessionsChanged because WorkoutProvider already
+ // handles the callback and growth model retraining in its own flow.
/// Remove a session from the in-memory list without touching storage.
void evictSession(String sessionId) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/services/managers/history_manager.dart` around lines 196 -
210, Add a short clarifying comment above the cache-only mutation methods
evictSession and patchSession stating that these methods intentionally do not
call onSessionsChanged because they only mutate the in-memory _sessions and
WorkoutProvider (or the caller) is responsible for invoking onSessionsChanged
when storage-backed changes occur; reference the methods by name (evictSession,
patchSession) and place the comment directly above each method so future
maintainers understand the design decision.
Comment on lines
+591
to
602
| if (_historyManager != null) { | ||
| // Delegate persistence + HC sync to HistoryManager. | ||
| await _historyManager.addSession( | ||
| session, | ||
| routineName: _activeRoutine?.name ?? _activeProgramDay?.name, | ||
| ); | ||
| } else { | ||
| // Fallback: persist directly (no HC sync) when historyManager is absent. | ||
| await _storage.saveWorkoutSession(session); | ||
| } | ||
| await _clearDraft(); | ||
| _sessions.insert(0, session); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Dual session state may cause inconsistency.
When _historyManager is present, addSession persists and caches the session in HistoryManager._sessions. Then line 602 also inserts into WorkoutProvider._sessions. Both collections now hold the same session, but they can drift apart:
- If something updates
WorkoutProvider._sessionsdirectly without going throughHistoryManager, they desync. - The code does call
evictSession/patchSessionon delete/update, but any other direct mutations to_sessionswould miss the sync.
This is likely acceptable during the migration phase (per the file header noting backward compatibility), but consider documenting this dual-state invariant or consolidating to a single source of truth.
Based on learnings: "In the workout-logger Flutter application, WorkoutProvider is identified as a God Class... Future changes should move toward splitting this into smaller providers: WorkoutManager (active session), HistoryManager (past data)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/services/workout_provider.dart` around lines 591 - 602,
The current logic duplicates session state by letting HistoryManager.addSession
persist and cache the session (HistoryManager._sessions) while WorkoutProvider
also inserts it into its own _sessions; to fix, avoid dual caching: when
_historyManager != null, remove the _sessions.insert(0, session) path and rely
on HistoryManager._sessions (or, alternatively, have HistoryManager.addSession
return the canonical session and replace WorkoutProvider._sessions insertion
with a single authoritative update using that returned session), ensure
_clearDraft() still runs and that evictSession/patchSession delegate to
HistoryManager to keep a single source of truth (references: _historyManager,
addSession, HistoryManager._sessions, _sessions, evictSession, patchSession,
_storage.saveWorkoutSession, _clearDraft).
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo Devasy/Workout-logger \
--title "Consolidate dual session state between WorkoutProvider and HistoryManager" \
--body "## Summary
When \`_historyManager\` is present in \`WorkoutProvider.finishWorkout()\`, \`HistoryManager.addSession()\` persists and caches the session in \`HistoryManager._sessions\`. However, \`WorkoutProvider._sessions\` also inserts the same session immediately after (line 602 of \`workout-logger/lib/services/workout_provider.dart\`). This dual-state ownership can cause the two collections to drift apart:
- Direct mutations to \`WorkoutProvider._sessions\` that bypass \`HistoryManager\` will desync the two caches.
- While \`evictSession\`/\`patchSession\` are called on delete/update to propagate changes, any other direct mutations to \`_sessions\` would miss the sync.
## Suggested Fix
When \`_historyManager != null\`, remove the \`_sessions.insert(0, session)\` path in \`WorkoutProvider.finishWorkout()\` and rely solely on \`HistoryManager._sessions\` as the authoritative source of truth for persisted sessions. Ensure \`_clearDraft()\` still runs and that all delete/update paths continue to delegate to \`HistoryManager\`.
This aligns with the broader refactoring roadmap to split \`WorkoutProvider\` into smaller, focused managers (\`WorkoutManager\`, \`HistoryManager\`, \`RoutineManager\`).
## References
- PR: https://github.com/Devasy23/Workout-logger/pull/40
- Review comment: https://github.com/Devasy/Workout-logger/pull/40#discussion_r3174919100
Requested by `@Devasy`." \
--assignee Devasy
Length of output: 51
@Devasy, done! I've created the issue to track this: #44 - Consolidate dual session state between WorkoutProvider and HistoryManager.
It captures the dual-state problem between WorkoutProvider._sessions and HistoryManager._sessions, the suggested fix (removing _sessions.insert(0, session) when _historyManager != null), and links back to this PR and review comment.
🧠 Learnings used
Learnt from: Devasy
Repo: Devasy/Workout-logger PR: 0
File: :0-0
Timestamp: 2026-01-29T08:37:13.808Z
Learning: In the workout-logger Flutter application, `WorkoutProvider` is identified as a God Class that violates SRP by handling multiple responsibilities: state management (sessions, routines, targets, exercises), data persistence (direct StorageService calls), business logic (statistics calculation, growth models, recommendations), and workout execution (timers, current index). Future changes should move toward splitting this into smaller providers: `WorkoutManager` (active session), `HistoryManager` (past data), and `RoutineManager`.
Learnt from: Devasy
Repo: Devasy/Workout-logger PR: 0
File: :0-0
Timestamp: 2026-01-29T08:37:13.808Z
Learning: In the workout-logger Flutter application, `StorageService` acts as a monolithic repository for all data types (sessions, routines, targets, muscle groups, custom exercises). The codebase has tight coupling to this concrete implementation, violating DIP. The recommended approach is to extract interfaces like `IStorageService` and `IExerciseRepository` to allow for different storage backends without modifying consumers.
Learnt from: Devasy
Repo: Devasy/Workout-logger PR: 0
File: :0-0
Timestamp: 2026-01-29T08:37:13.808Z
Learning: In the workout-logger Flutter application, the refactoring roadmap prioritizes: (1) Extract interfaces like `IStorageService` and `IExerciseRepository`, (2) Split `WorkoutProvider` into `WorkoutManager`, `HistoryManager`, and `RoutineManager`, (3) Make `MLService` injectable via dependency injection. New code should align with these architectural goals.
Learnt from: Devasy
Repo: Devasy/Workout-logger PR: 0
File: :0-0
Timestamp: 2026-01-29T08:37:13.808Z
Learning: In the workout-logger Flutter application, there are interface segregation violations where components like `HomeScreen` depend on the entire `WorkoutProvider` even though they only need specific functionality like `getQuickStats()` and routines list. This causes unnecessary rebuilds and coupling. Future changes should ensure components depend only on the specific interfaces or providers they actually need.
Learnt from: Devasy
Repo: Devasy/Workout-logger PR: 0
File: :0-0
Timestamp: 2026-01-29T08:37:13.808Z
Learning: In the workout-logger Flutter application, `MLService` uses static methods which prevents dependency injection, testing with mocks, and runtime algorithm swapping. It should be refactored to an instance-based service implementing `IMLService` and injected into `WorkoutProvider`.
Learnt from: CR
Repo: Devasy/Workout-logger PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T11:23:47.548Z
Learning: Applies to lib/screens/**/*.dart : Prefer watching the smallest scoped manager/provider in widgets; avoid broad `context.watch<WorkoutProvider>()` in leaf widgets to reduce coupling and rebuilds
Learnt from: Devasy
Repo: Devasy/Workout-logger PR: 40
File: workout-logger/android/app/src/main/AndroidManifest.xml:3-3
Timestamp: 2026-05-01T16:12:59.944Z
Learning: In the workout-logger Flutter app (`workout-logger/android/app/src/main/AndroidManifest.xml`), both `android.permission.health.READ_EXERCISE` and `android.permission.health.WRITE_EXERCISE` Health Connect permissions are intentionally declared, even if only write operations are currently visible in the codebase. Do NOT flag READ_EXERCISE as unnecessary or suggest removing it.
This was referenced May 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement integration with Health Connect to enable workout syncing and manage permissions. This includes adding necessary permissions, creating an interface for Health Connect services, and updating the UI to allow users to enable or disable syncing.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Chores
Tests