Optimize lookup and caching patterns with performance tests - #38
Conversation
chore: bump Flutter SDK version to ^3.11.4
|
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 32 minutes and 7 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughIntroduces in-memory indexes and fast-paths for exercise and session lookups, refactors target calculation and deletion checks, adjusts a UI dropdown binding and an import removal, bumps the Dart SDK constraint, and adds performance regression tests validating index vs brute-force behavior. 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. Comment |
…ests for workout tracking logic
Devasy
changed the title
…ns and performance-optimized exercise history indexing.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
workout-logger/lib/services/managers/analytics_manager.dart (2)
156-184:⚠️ Potential issue | 🔴 CriticalFast path can be slower than the fallback —
_logDatedefeats the optimization.This loop calls
_logDate(log, sessions)for every indexed log, and_logDate(lines 189–194) does a linear scan over all sessions, withsession.exercises.contains(log)itself running an O(E) equality check per session.Total cost of the "fast path" for an exercise with K logs across N sessions of average size E:
- Fast path:
O(K · N · E)- Brute-force fallback:
O(N log N + N · E)For any frequently-logged exercise (large K), the indexed path is strictly worse than the fallback you replaced. There's also a silent-correctness risk:
List.containsuses==, so ifExerciseLogdoesn't override value equality (or two sessions hold logs that happen to be equal),_logDatereturns the wrong session date — orDateTime.now()from the "should not happen" branch — and the whole progression chart is corrupted.Recommend storing the date alongside the log in the index so the fast path is a true O(K) read with no lookup:
Suggested fix
- // Pre-computed exerciseId → sorted-newest-first ExerciseLog index. + // Pre-computed exerciseId → newest-first list of (date, log) entries. // Rebuilt via [buildSessionIndex] whenever the session list changes. - Map<String, List<ExerciseLog>> _sessionIndex = {}; + Map<String, List<({DateTime date, ExerciseLog log})>> _sessionIndex = {};void buildSessionIndex(List<WorkoutSession> sessions) { - final index = <String, List<ExerciseLog>>{}; + final index = <String, List<({DateTime date, ExerciseLog log})>>{}; final sorted = List<WorkoutSession>.from(sessions) ..sort((a, b) => b.date.compareTo(a.date)); for (final session in sorted) { for (final log in session.exercises) { if (log.sets.isNotEmpty) { - index.putIfAbsent(log.exerciseId, () => []).add(log); + index + .putIfAbsent(log.exerciseId, () => []) + .add((date: session.date, log: log)); } } } _sessionIndex = index; }List<({DateTime date, double volume})> getVolumeProgression( String exerciseId, List<WorkoutSession> sessions, ) { - // Fast path: use pre-built index final logs = _sessionIndex[exerciseId]; if (logs != null) { - // Index is newest-first; progression needs oldest-first. return [ - for (final log in logs.reversed) - (date: _logDate(log, sessions), volume: log.totalVolume), + for (final entry in logs.reversed) + (date: entry.date, volume: entry.log.totalVolume), ]; } ... }
getRecommendationsthen becomeslogs.first.loginstead oflogs.first. After this,_logDate(and its dependence on the caller-providedsessionsmatching the indexing pass) can be deleted entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/services/managers/analytics_manager.dart` around lines 156 - 184, The fast path in getVolumeProgression is defeated by calling _logDate for each indexed log (O(K·N·E)); change the index _sessionIndex to store the session date alongside each log (e.g. store pairs/records like (log, date) when you build the index), then update getVolumeProgression to read the date directly from the indexed entry instead of calling _logDate; remove the _logDate helper and update any other call-sites (the review notes getRecommendations will become logs.first.log vs logs.first) so consumers read the stored date field and you regain an O(K) fast path without any session scanning or reliance on ExerciseLog equality.
29-64:⚠️ Potential issue | 🟠 MajorCache invalidation is fully manual — ensure
buildSessionIndex()is called after every session mutation during integration.
_sessionIndexis only rebuilt by explicitbuildSessionIndex()call. Currently this is safe becauseAnalyticsManageris not yet wired intoWorkoutProvider—buildSessionIndex()is only invoked in tests. However, once integrated, every session mutator (finishWorkout,deleteWorkoutSession,updateWorkoutSession) must rebuild the index, and none currently do so. This is easy to miss during integration.Two options to prevent cache corruption:
- Have
AnalyticsManagerown the canonical session list and exposeaddSession()/removeSession()/updateSession()mutators that incrementally maintain the index.- Make the index lazy: clear it via
invalidate()and rebuild on next read.At minimum, add a doc comment on
buildSessionIndex()stating that callers MUST invoke it after every session mutation. Consider an assertion in debug builds that the cached sessions list identity matches the one passed to reads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/services/managers/analytics_manager.dart` around lines 29 - 64, The session index cache (_sessionIndex) is only rebuilt when buildSessionIndex(...) is called, so callers must not rely on it remaining valid after mutations; update AnalyticsManager to either own and mutate the canonical session list via new methods (addSession, removeSession, updateSession) that incrementally maintain _sessionIndex, or add an invalidate() method and make reads lazy (rebuild on demand) plus document that buildSessionIndex must be called after every session mutation; at minimum add a clear doc comment to buildSessionIndex stating callers MUST call it after any session change and consider adding a debug-only assertion comparing a cached sessions identity to the passed list to detect stale cache usage.workout-logger/lib/services/strategies/target_calculator.dart (1)
41-49: 🧹 Nitpick | 🔵 TrivialLogic LGTM; comment is slightly misleading.
The in-place scan is correct and avoids the intermediate
map().reduce()allocation. However, the inline comment on line 42 says "no.toDouble()needed" but line 44 still callsset.reps.toDouble(). The actual saving is the dropped intermediate iterable +reduce, not thetoDouble()call. Consider tightening the comment so it doesn't mislead future readers.Suggested wording
- // In-place max scan: no intermediate list allocation, no .toDouble() needed. + // In-place max scan: avoids the intermediate mapped iterable + reduce + // allocation that the previous implementation incurred per log. for (final set in log.sets) { if (set.reps > bestValue) bestValue = set.reps.toDouble(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/services/strategies/target_calculator.dart` around lines 41 - 49, The comment above the in-place max scan is misleading about ".toDouble()"—update the comment near the loop in target_calculator.dart (around the _getExerciseLogsForExercise loop and bestValue calculation) to say the savings come from avoiding the intermediate iterable/reduce allocation rather than avoiding .toDouble(); leave the logic as-is (keep set.reps.toDouble() and the in-place scan using bestValue) and replace the comment text to reflect that the benefit is skipping an intermediate map()/reduce() allocation.
🤖 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/analytics_manager.dart`:
- Around line 186-194: The _logDate function currently uses
session.exercises.contains(log) which silently fails because ExerciseLog uses
identity equality; change the lookup to an explicit identity check (e.g.,
iterate session.exercises and use identical(entry, log)) so the identity
assumption is clear, and replace the silent fallback return DateTime.now() with
a failing assertion or throw (e.g., assert(false, ...) or throw StateError(...))
to avoid masking bugs; update any related comments and keep the behavior
predictable when the log is not found (refer to _logDate, ExerciseLog, and
session.exercises to locate the code).
In `@workout-logger/lib/services/workout_provider.dart`:
- Around line 686-691: The code currently makes a sorted copy of _sessions
before iterating, which is unnecessary because this provider maintains a
newest-first invariant (see finishWorkout inserting at index 0 and
updateWorkoutSession which re-sorts on edits); remove the List.from(...).sort()
allocation and iterate _sessions directly in newest-first order, using the
existing early-skip logic (use continue for sessions older than weekAgo when
scanning forward) so you avoid the O(n log n) cost on every render; if you
expect external invariants to be violated (e.g., bulk import) instead add an
explicit, documented sort call in that import path or a boolean flag to trigger
sorting rather than sorting here.
- Around line 211-225: The consolidated debug message loses which collection
prevented deletion; modify the referencedIds construction to also collect
per-collection reasons when adding IDs (e.g., record that an ID came from
_sessions, _routines, _targets, _currentExerciseLogs, or _activeRoutine) so that
after you check referencedIds.contains(exerciseId) you can emit a targeted
debugPrint mentioning the specific source(s) (use the existing referencedIds set
logic but augment it with a lightweight Map or Set of reasons keyed by
exerciseId, then print those reasons for the matching exerciseId rather than the
generic "still referenced").
In `@workout-logger/test/performance_regression_test.dart`:
- Around line 311-333: The test uses a brittle numeric threshold (< 900) tied to
exercise activation weights; instead modify the test to assert behavior
deterministically by either (a) creating and injecting a small controlled
Exercise with known activation weights and calling getWeeklyVolumeByMuscle (or
AnalyticsManager.getWeeklyVolumeByMuscle) so you can assert exact expected
totals for the recent session only, or (b) call provider.getWeeklyVolumeByMuscle
twice with a shifted "now" (advance time by 30+ days) to show that when both
sessions are outside the 7-day window totalVolume becomes zero and thus the
original result must be due to the recent session—replace the magic 900 check
with one of these deterministic comparisons referencing
provider.getWeeklyVolumeByMuscle / AnalyticsManager.getWeeklyVolumeByMuscle.
- Around line 341-392: Replace the locally defined _FakeMLService with the
shared MockMLService from test_utils: remove the _FakeMLService class and import
MockMLService, then instantiate and configure MockMLService in tests to provide
the same behaviors used here (recommendSets, getDefaultRecommendations,
extractExerciseDataPoints, trainGrowthModel, predictTargetCompletion), e.g. set
up its stubbed return values or expectations for recommendSets and
trainGrowthModel so tests remain deterministic; update any references to
_FakeMLService to use MockMLService and ensure the test imports are
added/cleaned accordingly.
---
Outside diff comments:
In `@workout-logger/lib/services/managers/analytics_manager.dart`:
- Around line 156-184: The fast path in getVolumeProgression is defeated by
calling _logDate for each indexed log (O(K·N·E)); change the index _sessionIndex
to store the session date alongside each log (e.g. store pairs/records like
(log, date) when you build the index), then update getVolumeProgression to read
the date directly from the indexed entry instead of calling _logDate; remove the
_logDate helper and update any other call-sites (the review notes
getRecommendations will become logs.first.log vs logs.first) so consumers read
the stored date field and you regain an O(K) fast path without any session
scanning or reliance on ExerciseLog equality.
- Around line 29-64: The session index cache (_sessionIndex) is only rebuilt
when buildSessionIndex(...) is called, so callers must not rely on it remaining
valid after mutations; update AnalyticsManager to either own and mutate the
canonical session list via new methods (addSession, removeSession,
updateSession) that incrementally maintain _sessionIndex, or add an invalidate()
method and make reads lazy (rebuild on demand) plus document that
buildSessionIndex must be called after every session mutation; at minimum add a
clear doc comment to buildSessionIndex stating callers MUST call it after any
session change and consider adding a debug-only assertion comparing a cached
sessions identity to the passed list to detect stale cache usage.
In `@workout-logger/lib/services/strategies/target_calculator.dart`:
- Around line 41-49: The comment above the in-place max scan is misleading about
".toDouble()"—update the comment near the loop in target_calculator.dart (around
the _getExerciseLogsForExercise loop and bestValue calculation) to say the
savings come from avoiding the intermediate iterable/reduce allocation rather
than avoiding .toDouble(); leave the logic as-is (keep set.reps.toDouble() and
the in-place scan using bestValue) and replace the comment text to reflect that
the benefit is skipping an intermediate map()/reduce() allocation.
🪄 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: 89bae7bc-16e7-4028-a5a1-19516125353a
📒 Files selected for processing (8)
workout-logger/lib/screens/programs/program_designer_screen.dartworkout-logger/lib/screens/programs/program_detail_screen.dartworkout-logger/lib/services/managers/analytics_manager.dartworkout-logger/lib/services/managers/exercise_manager.dartworkout-logger/lib/services/strategies/target_calculator.dartworkout-logger/lib/services/workout_provider.dartworkout-logger/pubspec.yamlworkout-logger/test/performance_regression_test.dart
💤 Files with no reviewable changes (1)
- workout-logger/lib/screens/programs/program_detail_screen.dart
|
@coderabbitai how would this changes would affect the userflow, in calc, so I can test it manually? |
|
Tip For best results, initiate chat on the files or code changes.
1. 🐛 Dropdown Initial Value Fix (
|
| Priority | Test | Reason |
|---|---|---|
| 🔴 High | Program Detail screen compiles & renders | Potentially broken import |
| 🔴 High | Dropdown initial value in Add Day dialog | Direct bug fix |
| 🟡 Medium | Delete custom exercise (referenced vs. unreferenced) | Logic rewrite |
| 🟡 Medium | Reps target recommendation | Algorithm change |
| 🟢 Low | Weekly volume stats (last 7 days only) | Performance change, same output |
🧠 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, 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: 37
File: workout-logger/pubspec.yaml:0-0
Timestamp: 2026-04-25T11:09:16.926Z
Learning: In the workout-logger Flutter application (`workout-logger/pubspec.yaml`), the `flutter` SDK constraint under `environment:` is intentionally pinned to an exact version (e.g., `flutter: 3.41.6`) to match the Flutter version used in GitHub Actions CI, ensuring reproducible builds. Do NOT flag this as a nitpick or suggest switching to a range constraint.
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, `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, `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`.
…ce components and regression tests
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
workout-logger/lib/services/workout_provider.dart (1)
698-715:⚠️ Potential issue | 🟡 Minor
continuedefeats the “early break” optimization promised by the PR.The PR description states this method "processes sessions newest-first with early break for 7-day weekly volume calculation," but using
continuehere makes the loop O(N) over the full session list, identical to the pre-optimization scan. With the newest-first invariant maintained byfinishWorkout/updateWorkoutSession, switching tobreakmakes the traversal O(K) where K is the number of in-range sessions (typically ≤ a handful per week) — the actual optimization the PR set out to deliver.If the concern is bulk-import paths violating newest-first, the safer fix is to enforce the invariant once at the import boundary (the existing
updateWorkoutSessionat line 490 already sorts) rather than degrading every analytics-screen render. Otherwise, the localexerciseMapprecompute is the only remaining net win here.🔧 Suggested change
- // Iterate directly since _sessions is maintained newest-first. - // We use continue rather than break in case of external imports - // that might temporarily violate the newest-first invariant. + // _sessions is maintained newest-first by finishWorkout/updateWorkoutSession, + // so we can break as soon as we hit an out-of-range session. for (final session in _sessions) { - if (session.date.isBefore(weekAgo)) continue; + if (session.date.isBefore(weekAgo)) break;🤖 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 698 - 715, The loop over _sessions currently uses continue on session.date.isBefore(weekAgo), which prevents the intended newest-first early break optimization; change the inner check in the for (final session in _sessions) loop to use break instead of continue so traversal stops once an older-than-weekAgo session is encountered, leaving the rest of the code (exerciseMap, muscleVolume, volumeByMuscle) unchanged; ensure the newest-first invariant is enforced at import/update boundaries by relying on finishWorkout/updateWorkoutSession (or call the existing sort in updateWorkoutSession where bulk imports occur) so the break is safe.workout-logger/lib/services/managers/analytics_manager.dart (2)
29-55:⚠️ Potential issue | 🟡 Minor
_lastIndexedSessionsis a write-only field.Line 30 declares it as a "debug-only reference to detect stale usage," but the only assignment is inside the
assert(() { ... }())at line 53 and there is no read site anywhere — noassertcomparing the currentsessionsargument against it on subsequent calls, nodebugPrint, nothing. In release builds the assert is stripped, so the field is effectively dead; in debug builds it's still write-only and consumes memory holding a strong reference to the (potentially large) sessions list.Either complete the staleness check (e.g., assert in
getRecommendations/getVolumeProgressionthatidentical(_lastIndexedSessions, sessions)when the cache is hot) or remove the field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/services/managers/analytics_manager.dart` around lines 29 - 55, Remove the dead debug-only field and its assert assignment: delete the private field _lastIndexedSessions and remove the assert block inside buildSessionIndex that assigns to it (the assert(() { _lastIndexedSessions = sessions; return true;}())). This keeps buildSessionIndex behavior unchanged while not holding a strong reference to the sessions list; if you prefer to retain staleness checks instead, add paired assert reads (e.g., in getRecommendations/getVolumeProgression) that verify identical(_lastIndexedSessions, sessions) rather than leaving _lastIndexedSessions write-only.
29-72:⚠️ Potential issue | 🟠 MajorCache staleness in
AnalyticsManager.buildSessionIndex— pre-integration blocker.
AnalyticsManageris designed to cache session indices for performance, butbuildSessionIndexis never automatically called when sessions change. The fast paths ingetRecommendations(line 144–145) andgetVolumeProgression(line 170–176) will return stale data: a session added after the last index build returns the previous "most recent" log; a deleted session leaves ghost references. Tests pass only because each manually callsbuildSessionIndex.While
AnalyticsManageris not yet instantiated in production code, this cache invalidation design flaw must be fixed before integration.HistoryManageralready exposesonSessionsChangedcallbacks (lines 47, 106, 139) — use them to rebuild the index:🔧 Fix sketch
AnalyticsManager( this._storage, this._mlService, - {this.onGrowthModelUpdated} + {this.onGrowthModelUpdated, void Function(List<WorkoutSession>)? onSessionsChanged} ) { + onSessionsChanged?.call; // Wire cache invalidation }Or simpler: have
WorkoutProvidercallanalytics.buildSessionIndex(_sessions)immediately after anyHistoryManagersession mutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/services/managers/analytics_manager.dart` around lines 29 - 72, The session index can become stale because buildSessionIndex in AnalyticsManager is never invoked when sessions change; update the code so the index is rebuilt on session mutations by subscribing to HistoryManager.onSessionsChanged (or having WorkoutProvider call analytics.buildSessionIndex(_sessions) immediately after any session add/update/delete). Specifically, ensure AnalyticsManager.buildSessionIndex is called whenever HistoryManager emits changes so that callers of AnalyticsManager.getRecommendations and getVolumeProgression always operate on a fresh _sessionIndex and no deleted/ghost WorkoutSession or out-of-date ExerciseLog entries remain.
♻️ Duplicate comments (1)
workout-logger/lib/services/managers/analytics_manager.dart (1)
158-176:⚠️ Potential issue | 🟡 Minor
getVolumeProgressionfast path silently changes semantics for empty histories.When the index has been built but no logs exist for
exerciseId,_sessionIndex[exerciseId]isnull, so the code falls through to the slow path and returns[]. But once any session is added without rebuilding the index, the new exercise's logs will also be missing from the cache while other exercises' progressions are silently truncated. This is the same staleness issue flagged onbuildSessionIndexand is harder to spot here because there's no second branch likegetRecommendationshas.Consider adding an explicit invariant check or only consulting the cache when you can prove it's current (e.g., a build-version counter on the sessions list).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/services/managers/analytics_manager.dart` around lines 158 - 176, The fast path in getVolumeProgression currently treats a missing key and an empty-history the same because it checks _sessionIndex[exerciseId] == null; change it to consult the cache only when you can prove it's current and to distinguish “built but empty” from “not built”: use _sessionIndex.containsKey(exerciseId) (so an existing empty list yields []) and add/verify a freshness check (e.g., an indexVersion or sessionsVersion produced by buildSessionIndex compared against the provided sessions list) before using the cached logs; if the version check fails, fall back to the slow path or rebuild the index so getVolumeProgression (and functions like buildSessionIndex) do not silently return truncated/stale results.
🤖 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/workout_provider.dart`:
- Around line 222-230: The debug labels currently pass private field names
(e.g., '_sessions', '_routines', '_targets', '_currentExerciseLogs',
'_activeRoutine') to addReference; replace those strings with user-facing,
non-private labels (for example "sessions", "routines", "targets", "current
exercise logs", "active routine" or "active workout") so logs no longer expose
implementation details; update the calls around addReference(e.exerciseId,
'_sessions'), addReference(id, '_routines'), addReference(t.exerciseId,
'_targets'), addReference(l.exerciseId, '_currentExerciseLogs'), and
addReference(id, '_activeRoutine') to use the new public labels.
- Around line 221-231: The for-loops that call addReference without braces
violate the curly_braces_in_flow_control_structures lint; update each loop in
workout_provider.dart so their bodies are wrapped in braces — specifically the
loops iterating over _sessions (for final s in _sessions), _routines (for final
r in _routines), _targets (for final t in _targets), _currentExerciseLogs (for
final l in _currentExerciseLogs), and the _activeRoutine exerciseIds loop (for
final id in _activeRoutine!.exerciseIds) — keeping the addReference(...) calls
unchanged but enclosed in { ... } to satisfy the lint.
In `@workout-logger/test/performance_regression_test.dart`:
- Around line 84-112: The test currently compares recommendations from
getRecommendations before and after buildSessionIndex but doesn't verify the
cached "lastLog" used by the fast path; add an assertion that the cached entry
equals the brute-force result from findMostRecentExerciseLog to ensure the index
priming is correct. After calling analytics.buildSessionIndex(sessions) compute
the brute-force lastLog via analytics.findMostRecentExerciseLog('ex_0',
sessions) and assert it equals analytics._sessionIndex['ex_0'].first.log (or the
equivalent cached field used by getRecommendations); this will ensure the cached
lastLog is identical to the cold-path computed value and that
MockMLService.recommendSets will be fed the same true input in both paths.
- Around line 274-345: The test is labeled for
WorkoutProvider.getWeeklyVolumeByMuscle but actually calls
AnalyticsManager.getWeeklyVolumeByMuscle (creating local analytics), so the
provider's early-exit code path is never exercised; either rename the test/group
to reflect AnalyticsManager or change the assertions to call
provider.getWeeklyVolumeByMuscle(...) (use the existing provider variable from
setUp) so the provider implementation (WorkoutProvider.getWeeklyVolumeByMuscle)
runs; if you choose to call provider, remove/invalidate passing a custom now and
ensure sessions are seeded relative to DateTime.now() (as in setUp) so the
provider's internal 7-day window logic is tested.
- Around line 349-354: Remove the stale comment header "// Minimal fake ML
service — returns deterministic recommendations" that refers to the removed
_FakeMLService and replace it with either nothing or a brief note referring to
the current MockMLService; search for the comment text and delete that block so
the file only documents the current MockMLService implementation (no references
to _FakeMLService remain).
---
Outside diff comments:
In `@workout-logger/lib/services/managers/analytics_manager.dart`:
- Around line 29-55: Remove the dead debug-only field and its assert assignment:
delete the private field _lastIndexedSessions and remove the assert block inside
buildSessionIndex that assigns to it (the assert(() { _lastIndexedSessions =
sessions; return true;}())). This keeps buildSessionIndex behavior unchanged
while not holding a strong reference to the sessions list; if you prefer to
retain staleness checks instead, add paired assert reads (e.g., in
getRecommendations/getVolumeProgression) that verify
identical(_lastIndexedSessions, sessions) rather than leaving
_lastIndexedSessions write-only.
- Around line 29-72: The session index can become stale because
buildSessionIndex in AnalyticsManager is never invoked when sessions change;
update the code so the index is rebuilt on session mutations by subscribing to
HistoryManager.onSessionsChanged (or having WorkoutProvider call
analytics.buildSessionIndex(_sessions) immediately after any session
add/update/delete). Specifically, ensure AnalyticsManager.buildSessionIndex is
called whenever HistoryManager emits changes so that callers of
AnalyticsManager.getRecommendations and getVolumeProgression always operate on a
fresh _sessionIndex and no deleted/ghost WorkoutSession or out-of-date
ExerciseLog entries remain.
In `@workout-logger/lib/services/workout_provider.dart`:
- Around line 698-715: The loop over _sessions currently uses continue on
session.date.isBefore(weekAgo), which prevents the intended newest-first early
break optimization; change the inner check in the for (final session in
_sessions) loop to use break instead of continue so traversal stops once an
older-than-weekAgo session is encountered, leaving the rest of the code
(exerciseMap, muscleVolume, volumeByMuscle) unchanged; ensure the newest-first
invariant is enforced at import/update boundaries by relying on
finishWorkout/updateWorkoutSession (or call the existing sort in
updateWorkoutSession where bulk imports occur) so the break is safe.
---
Duplicate comments:
In `@workout-logger/lib/services/managers/analytics_manager.dart`:
- Around line 158-176: The fast path in getVolumeProgression currently treats a
missing key and an empty-history the same because it checks
_sessionIndex[exerciseId] == null; change it to consult the cache only when you
can prove it's current and to distinguish “built but empty” from “not built”:
use _sessionIndex.containsKey(exerciseId) (so an existing empty list yields [])
and add/verify a freshness check (e.g., an indexVersion or sessionsVersion
produced by buildSessionIndex compared against the provided sessions list)
before using the cached logs; if the version check fails, fall back to the slow
path or rebuild the index so getVolumeProgression (and functions like
buildSessionIndex) do not silently return truncated/stale results.
🪄 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: 46b2adcd-eaf3-433d-996b-efc7be5205a3
📒 Files selected for processing (4)
workout-logger/lib/services/managers/analytics_manager.dartworkout-logger/lib/services/strategies/target_calculator.dartworkout-logger/lib/services/workout_provider.dartworkout-logger/test/performance_regression_test.dart
| for (final s in _sessions) { | ||
| for (final e in s.exercises) addReference(e.exerciseId, '_sessions'); | ||
| } | ||
|
|
||
| // Check for references in active workout | ||
| if (_currentExerciseLogs.any((l) => l.exerciseId == exerciseId)) { | ||
| debugPrint('Cannot delete custom exercise: Used in active workout'); | ||
| return false; | ||
| for (final r in _routines) { | ||
| for (final id in r.exerciseIds) addReference(id, '_routines'); | ||
| } | ||
| for (final t in _targets) addReference(t.exerciseId, '_targets'); | ||
| for (final l in _currentExerciseLogs) addReference(l.exerciseId, '_currentExerciseLogs'); | ||
| if (_activeRoutine != null) { | ||
| for (final id in _activeRoutine!.exerciseIds) addReference(id, '_activeRoutine'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify flutter_lints / analysis_options enables curly_braces_in_flow_control_structures
fd -e yaml analysis_options
fd -e yaml analysis_options --exec cat {}
rg -n 'curly_braces_in_flow_control_structures' || trueRepository: Devasy/Workout-logger
Length of output: 1704
🏁 Script executed:
# Read the specific lines in workout_provider.dart to verify the violations
sed -n '220,232p' workout-logger/lib/services/workout_provider.dartRepository: Devasy/Workout-logger
Length of output: 582
Add curly braces around single-statement flow-control bodies.
flutter_lints enforces curly_braces_in_flow_control_structures, which requires braces around the body of flow-control statements even for single statements. Lines 222, 225, 227, 228, and 230 violate this rule and will fail lint analysis.
🔧 Fix
for (final s in _sessions) {
- for (final e in s.exercises) addReference(e.exerciseId, '_sessions');
+ for (final e in s.exercises) {
+ addReference(e.exerciseId, '_sessions');
+ }
}
for (final r in _routines) {
- for (final id in r.exerciseIds) addReference(id, '_routines');
+ for (final id in r.exerciseIds) {
+ addReference(id, '_routines');
+ }
}
- for (final t in _targets) addReference(t.exerciseId, '_targets');
- for (final l in _currentExerciseLogs) addReference(l.exerciseId, '_currentExerciseLogs');
+ for (final t in _targets) {
+ addReference(t.exerciseId, '_targets');
+ }
+ for (final l in _currentExerciseLogs) {
+ addReference(l.exerciseId, '_currentExerciseLogs');
+ }
if (_activeRoutine != null) {
- for (final id in _activeRoutine!.exerciseIds) addReference(id, '_activeRoutine');
+ for (final id in _activeRoutine!.exerciseIds) {
+ addReference(id, '_activeRoutine');
+ }
}🤖 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 221 - 231,
The for-loops that call addReference without braces violate the
curly_braces_in_flow_control_structures lint; update each loop in
workout_provider.dart so their bodies are wrapped in braces — specifically the
loops iterating over _sessions (for final s in _sessions), _routines (for final
r in _routines), _targets (for final t in _targets), _currentExerciseLogs (for
final l in _currentExerciseLogs), and the _activeRoutine exerciseIds loop (for
final id in _activeRoutine!.exerciseIds) — keeping the addReference(...) calls
unchanged but enclosed in { ... } to satisfy the lint.
| for (final e in s.exercises) addReference(e.exerciseId, '_sessions'); | ||
| } | ||
|
|
||
| // Check for references in active workout | ||
| if (_currentExerciseLogs.any((l) => l.exerciseId == exerciseId)) { | ||
| debugPrint('Cannot delete custom exercise: Used in active workout'); | ||
| return false; | ||
| for (final r in _routines) { | ||
| for (final id in r.exerciseIds) addReference(id, '_routines'); | ||
| } | ||
| for (final t in _targets) addReference(t.exerciseId, '_targets'); | ||
| for (final l in _currentExerciseLogs) addReference(l.exerciseId, '_currentExerciseLogs'); | ||
| if (_activeRoutine != null) { | ||
| for (final id in _activeRoutine!.exerciseIds) addReference(id, '_activeRoutine'); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reason labels expose private field names in debug output.
'_sessions', '_routines', '_targets', '_currentExerciseLogs', '_activeRoutine' are internal field names; the prior per-collection messages used user-facing labels (e.g. "sessions", "active workout"). The current strings will surface as still referenced in _sessions, _activeRoutine in logs, which leaks implementation detail and reads worse than the previous variant.
- for (final e in s.exercises) addReference(e.exerciseId, '_sessions');
+ for (final e in s.exercises) addReference(e.exerciseId, 'sessions');
...
- for (final id in r.exerciseIds) addReference(id, '_routines');
+ for (final id in r.exerciseIds) addReference(id, 'routines');
...
- for (final t in _targets) addReference(t.exerciseId, '_targets');
- for (final l in _currentExerciseLogs) addReference(l.exerciseId, '_currentExerciseLogs');
+ for (final t in _targets) addReference(t.exerciseId, 'targets');
+ for (final l in _currentExerciseLogs) addReference(l.exerciseId, 'active workout');
...
- for (final id in _activeRoutine!.exerciseIds) addReference(id, '_activeRoutine');
+ for (final id in _activeRoutine!.exerciseIds) addReference(id, 'active routine');🤖 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 222 - 230,
The debug labels currently pass private field names (e.g., '_sessions',
'_routines', '_targets', '_currentExerciseLogs', '_activeRoutine') to
addReference; replace those strings with user-facing, non-private labels (for
example "sessions", "routines", "targets", "current exercise logs", "active
routine" or "active workout") so logs no longer expose implementation details;
update the calls around addReference(e.exerciseId, '_sessions'),
addReference(id, '_routines'), addReference(t.exerciseId, '_targets'),
addReference(l.exerciseId, '_currentExerciseLogs'), and addReference(id,
'_activeRoutine') to use the new public labels.
| test('getRecommendations with index == getRecommendations without index', | ||
| () { | ||
| // Without index (cold) | ||
| final withoutIndex = analytics.getRecommendations('ex_0', sessions); | ||
|
|
||
| // Build the index | ||
| analytics.buildSessionIndex(sessions); | ||
|
|
||
| // With index (warm) | ||
| final withIndex = analytics.getRecommendations('ex_0', sessions); | ||
|
|
||
| expect( | ||
| withIndex.length, | ||
| equals(withoutIndex.length), | ||
| reason: 'Recommendation count should be identical', | ||
| ); | ||
| for (var i = 0; i < withoutIndex.length; i++) { | ||
| expect( | ||
| withIndex[i].reps, | ||
| equals(withoutIndex[i].reps), | ||
| reason: 'reps mismatch at index $i', | ||
| ); | ||
| expect( | ||
| withIndex[i].weight, | ||
| equals(withoutIndex[i].weight), | ||
| reason: 'weight mismatch at index $i', | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Cold-path invocation also primes nothing — both branches execute the same fallback in getRecommendations.
Before buildSessionIndex is called, _sessionIndex is empty, so getRecommendations at line 87 takes the findMostRecentExerciseLog fallback. After buildSessionIndex, line 93 takes the cache fast path. Both routes feed _mlService.recommendSets(...) with the same lastLog, so this test would still pass even if the cache were grossly miscomputed (e.g., picking the oldest log instead of the newest), because MockMLService.recommendSets is deterministic in its inputs but those inputs are computed by the production code, not validated here.
To make this a real regression test, assert directly that the cached lastLog matches findMostRecentExerciseLog (e.g., compare analytics._sessionIndex['ex_0'].first.log against the brute-force result), or seed multiple logs per session so picking the wrong one would produce visibly different recommendations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/test/performance_regression_test.dart` around lines 84 - 112,
The test currently compares recommendations from getRecommendations before and
after buildSessionIndex but doesn't verify the cached "lastLog" used by the fast
path; add an assertion that the cached entry equals the brute-force result from
findMostRecentExerciseLog to ensure the index priming is correct. After calling
analytics.buildSessionIndex(sessions) compute the brute-force lastLog via
analytics.findMostRecentExerciseLog('ex_0', sessions) and assert it equals
analytics._sessionIndex['ex_0'].first.log (or the equivalent cached field used
by getRecommendations); this will ensure the cached lastLog is identical to the
cold-path computed value and that MockMLService.recommendSets will be fed the
same true input in both paths.
| group('WorkoutProvider.getWeeklyVolumeByMuscle — early exit + map', () { | ||
| late MockStorageService mockStorage; | ||
| late WorkoutProvider provider; | ||
|
|
||
| setUp(() async { | ||
| mockStorage = MockStorageService(); | ||
| // Seed one session from 3 days ago and one from 30 days ago. | ||
| final recent = WorkoutSession( | ||
| id: 'recent', | ||
| date: DateTime.now().subtract(const Duration(days: 3)), | ||
| exercises: [ | ||
| ExerciseLog( | ||
| exerciseId: 'bench_press', // built-in exercise | ||
| sets: [WorkoutSet(weight: 100, reps: 5)], | ||
| ), | ||
| ], | ||
| duration: 30, | ||
| ); | ||
| final old = WorkoutSession( | ||
| id: 'old', | ||
| date: DateTime.now().subtract(const Duration(days: 30)), | ||
| exercises: [ | ||
| ExerciseLog( | ||
| exerciseId: 'bench_press', | ||
| sets: [WorkoutSet(weight: 80, reps: 5)], | ||
| ), | ||
| ], | ||
| duration: 30, | ||
| ); | ||
| mockStorage.addMockSession(recent); | ||
| mockStorage.addMockSession(old); | ||
| provider = WorkoutProvider( | ||
| mockStorage, | ||
| programManager: ProgramManager(mockStorage), | ||
| ); | ||
| await provider.init(); | ||
| }); | ||
|
|
||
| test('only counts sessions within the last 7 days', () { | ||
| // Create AnalyticsManager to use getWeeklyVolumeByMuscle with deterministic 'now' | ||
| final analytics = AnalyticsManager(mockStorage, MockMLService()); | ||
|
|
||
| // Build a controlled exercise with exact 100% activation | ||
| final exercises = [ | ||
| Exercise( | ||
| id: 'bench_press', | ||
| name: 'Bench Press', | ||
| muscleActivations: [ | ||
| MuscleActivation(muscleGroupId: 'chest', activationPercentage: 100), | ||
| ], | ||
| category: 'compound', | ||
| ) | ||
| ]; | ||
| final sessions = provider.sessions; | ||
|
|
||
| // With current date: recent session (100kg x 5 = 500 volume) is included, | ||
| // old session (80kg x 5 = 400 volume) is excluded. | ||
| final resultWithRecent = analytics.getWeeklyVolumeByMuscle( | ||
| sessions, | ||
| exercises, | ||
| now: DateTime.now(), | ||
| ); | ||
| expect(resultWithRecent['chest'], equals(500.0)); | ||
|
|
||
| // With shifted date (30+ days in future): both sessions are excluded. | ||
| final resultFuture = analytics.getWeeklyVolumeByMuscle( | ||
| sessions, | ||
| exercises, | ||
| now: DateTime.now().add(const Duration(days: 30)), | ||
| ); | ||
| expect(resultFuture['chest'], isNull); | ||
| }); |
There was a problem hiding this comment.
Test name claims to cover WorkoutProvider.getWeeklyVolumeByMuscle but actually exercises AnalyticsManager.getWeeklyVolumeByMuscle.
The group at line 274 and the setUp at lines 278–310 build a WorkoutProvider, but the assertions at lines 331 and 339 call analytics.getWeeklyVolumeByMuscle(...) (a fresh AnalyticsManager constructed inside the test). The provider's own implementation — which is the continue/break early-exit code path at workout_provider.dart:698–715 you're trying to regress against — is never executed. The 7-day window logic in AnalyticsManager.getWeeklyVolumeByMuscle is a separate function (no early-exit, takes now), so the test gives a false sense of coverage for the optimization called out in the PR description.
Either rename the group to reflect what's actually under test, or drive the assertions through provider.getWeeklyVolumeByMuscle() directly (and test the time-window behavior by seeding sessions relative to the system clock instead of injecting now).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/test/performance_regression_test.dart` around lines 274 - 345,
The test is labeled for WorkoutProvider.getWeeklyVolumeByMuscle but actually
calls AnalyticsManager.getWeeklyVolumeByMuscle (creating local analytics), so
the provider's early-exit code path is never exercised; either rename the
test/group to reflect AnalyticsManager or change the assertions to call
provider.getWeeklyVolumeByMuscle(...) (use the existing provider variable from
setUp) so the provider implementation (WorkoutProvider.getWeeklyVolumeByMuscle)
runs; if you choose to call provider, remove/invalidate passing a custom now and
ensure sessions are seeded relative to DateTime.now() (as in setUp) so the
provider's internal 7-day window logic is tested.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… optimize data lookups
1 participant
Set the initial value for the dropdown and remove an unused import. Update the Flutter SDK version to ensure compatibility with the latest features.
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Chores
Tests