Feat/increase coverage test screens - #63
Merged
Merged
Conversation
Contributor
WalkthroughChangesThe PR adds shared RF dialogs, snackbars, and text fields; updates related UI and Health Connect behavior; expands CI triggers; and adds reusable Flutter test utilities with broad screen, widget, service, and user-flow coverage. UI changes
Health Connect integration
Flutter widget test coverage
CI and toolchain configuration
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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 |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![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: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workout-logger/lib/screens/widgets/rf_dialogs.dart`:
- Around line 44-60: Update the semantic snackbar styling in the surrounding
dialog builder so success, warning, and error variants use a foreground color
meeting 4.5:1 contrast against their backgrounds for the 14px message text and
icon. Adjust the Icon and Text styling together, while preserving the existing
neutral styling and semantic colors as accents where appropriate.
In `@workout-logger/lib/screens/widgets/rf_widgets.dart`:
- Around line 945-970: Update the TextField wrapper in the RF widget to track
focus using a FocusNode or equivalent focus state, and change the outer
Container border to a high-contrast AppColors.primary border while focused.
Preserve the existing glass border for unfocused fields and keep the TextField’s
InputBorder.none configuration unchanged.
In `@workout-logger/test/screens/ai_coach_screen_test.dart`:
- Around line 12-31: Strengthen the state-specific assertions in
workout-logger/test/screens/ai_coach_screen_test.dart lines 12-31 by asserting
the missing-API-key prompt/banner in addition to the shared “AI Coach” title;
also update
workout-logger/test/screens/programs/program_designer_screen_test.dart lines
40-59 to assert its validation error message/control,
workout-logger/test/screens/widgets/targets_tab_test.dart lines 29-53 to assert
a rendered active-target item or value, and
workout-logger/test/screens/workout_flow_screen_test.dart lines 27-37 to assert
quick-start-specific content or controls.
In `@workout-logger/test/screens/edit_workout_session_screen_test.dart`:
- Around line 73-90: Update the test named “Edits session notes and saves
session” to verify persistence through storage, not only the in-memory provider.
After tapping Save, retrieve the session with await
storage.getWorkoutSession(session.id) and assert its notes equal “Updated
workout session note”; retain the existing provider assertion only if useful.
- Around line 42-56: Strengthen the edit-action tests in
workout-logger/test/screens/edit_workout_session_screen_test.dart: in the “Adds
a set to an existing exercise” test (lines 42-56), assert the target exercise’s
set count increases; in the selected-exercise removal test (lines 58-71), assert
its set count decreases; and in the discard test (lines 92-109), assert both the
discard dialog and EditWorkoutSessionScreen are dismissed after tapping Discard.
In `@workout-logger/test/screens/heart_rate_detail_screen_test.dart`:
- Line 14: Stop discarding widget-test exceptions: in
workout-logger/test/screens/heart_rate_detail_screen_test.dart:14,
history_screen_test.dart:30 and :55, home_screen_test.dart:88-93,
settings_screen_test.dart:29, and sleep_detail_screen_test.dart:14, assert each
tester.takeException() result is null. In
workout-logger/test/screens/home_screen_test.dart:66, fix the underlying
overflow/layout issue rather than clearing the exception.
- Line 16: Update the HeartRateDetailScreen test in
workout-logger/test/screens/heart_rate_detail_screen_test.dart at lines 16-16 to
assert the displayed title and granularity controls/content instead of only
finding the explicitly pumped widget. Update the history screen test in
workout-logger/test/screens/history_screen_test.dart at lines 32-32 to verify
the empty-history UI and that no session items are present.
In `@workout-logger/test/screens/onboarding_screen_test.dart`:
- Around line 23-28: Update the shared TestRobot helper methods, including
pumpScreen and the helpers for tap, fill, and pop operations, to call
tester.takeException() and assert that the result is null after each operation.
Ensure framework exceptions fail the test instead of being silently ignored.
In `@workout-logger/test/screens/widgets/health_widgets_test.dart`:
- Line 91: Update the testWidgets case around tester.takeException() to capture
its returned exception and assert that it is null, rather than discarding it.
Preserve the existing test flow while ensuring unexpected framework rendering
failures cannot be masked by the skipOnError wrapper.
In `@workout-logger/test/test_utils/test_harness.dart`:
- Around line 76-82: Remove the global FlutterError.onError override that
filters messages containing “RenderFlex” or “overflowed” from the test harness.
Update the owning test to fix or explicitly assert the expected layout failure
instead, allowing framework errors to propagate through FlutterError.onError
normally.
- Around line 24-32: Make the required widget parameters explicit in both helper
methods: update TestHarness.wrap in
workout-logger/test/test_utils/test_harness.dart at lines 24-32 to use an empty
required-parameter list followed by required Widget child, and update the
corresponding method in workout-logger/test/test_utils/test_robot.dart at lines
18-24 to use required Widget widget in the same ordering.
In `@workout-logger/test/test_utils/test_robot.dart`:
- Line 34: Replace every discarded tester.takeException() call with an assertion
that the result is null, unless that test intentionally expects a specific
exception. Apply this in workout-logger/test/test_utils/test_robot.dart lines
34-34, 43-43, 52-52, and 59-59; workout-logger/test/test_utils/test_sweep.dart
line 24-24; workout-logger/test/screens/ai_coach_screen_test.dart line 29-29;
workout-logger/test/screens/ai_program_generator_screen_test.dart lines 24-24
and 33-33; workout-logger/test/screens/profile_screen_test.dart line 33-33;
workout-logger/test/screens/programs/program_designer_screen_test.dart lines
32-32, 50-50, 55-55, 71-71, and 81-81; and
workout-logger/test/screens/workout_flow_screen_test.dart lines 21-21 and 34-34.
🪄 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 Plus
Run ID: 2cbbe829-6fb3-4ee3-8c53-9dfdb8c0563a
📒 Files selected for processing (24)
.github/workflows/test.ymlworkout-logger/lib/screens/edit_workout_session_screen.dartworkout-logger/lib/screens/widgets/floating_nav_bar.dartworkout-logger/lib/screens/widgets/rf_dialogs.dartworkout-logger/lib/screens/widgets/rf_widgets.dartworkout-logger/test/screens/ai_coach_screen_test.dartworkout-logger/test/screens/ai_program_generator_screen_test.dartworkout-logger/test/screens/edit_workout_session_screen_test.dartworkout-logger/test/screens/heart_rate_detail_screen_test.dartworkout-logger/test/screens/history_screen_test.dartworkout-logger/test/screens/home_screen_test.dartworkout-logger/test/screens/onboarding_screen_test.dartworkout-logger/test/screens/profile_screen_test.dartworkout-logger/test/screens/programs/program_designer_screen_test.dartworkout-logger/test/screens/settings_screen_test.dartworkout-logger/test/screens/sleep_detail_screen_test.dartworkout-logger/test/screens/widgets/health_widgets_test.dartworkout-logger/test/screens/widgets/rf_cards_test.dartworkout-logger/test/screens/widgets/targets_tab_test.dartworkout-logger/test/screens/workout_flow_screen_test.dartworkout-logger/test/test_utils/test_fixtures.dartworkout-logger/test/test_utils/test_harness.dartworkout-logger/test/test_utils/test_robot.dartworkout-logger/test/test_utils/test_sweep.dart
Comment on lines
+23
to
+28
| await robot.pumpScreen( | ||
| WelcomePage(onComplete: () => completed = true), | ||
| storage: storage, | ||
| settingsProvider: settings, | ||
| workoutProvider: workout, | ||
| ); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make TestRobot fail on framework exceptions.
TestRobot.pumpScreen calls tester.takeException() without asserting it is null, so this test can silently pass after a rendering failure. Change the shared helper to assert no exception after pump, tap, fill, and pop operations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/screens/onboarding_screen_test.dart` around lines 23 -
28, Update the shared TestRobot helper methods, including pumpScreen and the
helpers for tap, fill, and pop operations, to call tester.takeException() and
assert that the result is null after each operation. Ensure framework exceptions
fail the test instead of being silently ignored.
Comment on lines
+24
to
+32
| static Widget wrap( | ||
| Widget child, { | ||
| MockStorageService? storage, | ||
| WorkoutProvider? workoutProvider, | ||
| SettingsProvider? settingsProvider, | ||
| HistoryManager? historyManager, | ||
| HealthHistoryManager? healthHistoryManager, | ||
| Size viewportSize = const Size(1080, 2400), | ||
| }) { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files of interest:"
git ls-files | rg '(^|/)(test_harness|test_robot)\.dart$|analysis_options\.ya?ml' || true
echo
echo "test_harness outline:"
ast-grep outline workout-logger/test/test_utils/test_harness.dart --view expanded || true
echo
echo "test_robot outline:"
ast-grep outline workout-logger/test/test_utils/test_robot.dart --view expanded || true
echo
echo "Relevant snippets:"
cat -n workout-logger/test/test_utils/test_harness.dart | sed -n '1,120p'
echo
cat -n workout-logger/test/test_utils/test_robot.dart | sed -n '1,140p'
echo
echo "Call sites named args:"
rg -n "\bwrap\(|testRobot\(|pumpScreen\(" workout-logger/test -SRepository: Devasy/RepForge
Length of output: 12149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "analysis_options.yaml:"
cat -n workout-logger/analysis_options.yaml | sed -n '1,160p'
echo
echo "Full call-site snippets with formatting context:"
for f in \
workout-logger/test/screens/settings_screen_test.dart \
workout-logger/test/screens/profile_screen_test.dart \
workout-logger/test/screens/ai_program_generator_screen_test.dart \
workout-logger/test/screens/workout_flow_screen_test.dart \
workout-logger/test/screens/onboarding_screen_test.dart \
workout-logger/test/screens/widgets/targets_tab_test.dart \
workout-logger/test/screens/rf_cards_test.dart \
workout-logger/test/screens/history_screen_test.dart \
workout-logger/test/screens/widgets/health_widgets_test.dart \
workout-logger/test/screens/widgets/health_widgets_test.dart
do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f" | sed -n '1,90p'
fi
done
echo
echo "Precise call formats for TestHarness.wrap and pumpScreen:"
python3 - <<'PY'
from pathlib import Path
import re
def read(f):
return Path(f).read_text()
for path, pattern in [
("workout-logger/test", r"\bTestHarness\.wrap\s*\("),
("workout-logger/test/test_utils/test_robot.dart", r"\bpumpScreen\s*\("),
]:
files = list(Path("workout-logger/test").glob("**/*.dart"))
for f in files:
text = f.read_text(errors="replace")
if re.search(pattern, text):
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if re.search(pattern, line):
start = max(1, i-1)
end = min(len(lines), i+8)
block = "\n".join(f"{j}: {lines[j-1]}" for j in range(start, end+1))
print(path+f.name, f"line {i}", block, sep="\n-")
PYRepository: Devasy/RepForge
Length of output: 34969
🌐 Web query:
Dart style guide use named required for required named parameters
💡 Result:
In Dart, named parameters are optional by default [1][2][3]. To make a named parameter mandatory, you must explicitly use the 'required' keyword in the parameter declaration [1][2][4]. There is no specific rule in the official Effective Dart style guide that mandates the use of 'required' for all named parameters; rather, it is a functional language feature used when a caller must provide a value [1][2]. However, the Dart ecosystem and linter enforce specific stylistic conventions regarding their placement: 1. Mandatory Usage: You use the 'required' modifier when you want to ensure that callers explicitly provide a value for a named parameter [1][4]. This is often used for non-nullable types that do not have a default value, as it satisfies sound null safety requirements without forcing the parameter to be nullable [5][4]. 2. Placement Convention: The Dart linter includes a rule (always_put_required_named_parameters_first) that recommends placing all 'required' named parameters before any optional named parameters in the parameter list [6]. This improves readability and API clarity [6]. Example of correct usage and ordering: void exampleFunction({required int requiredParam, int? optionalParam}) { // Function body } While 'required' is a necessary tool for defining mandatory named arguments, its usage should be dictated by your API design requirements—specifically, whether a parameter is essential for the function's execution—rather than a blanket style guide mandate [1][4].
Citations:
- 1: https://dart.dev/language/functions
- 2: https://github.com/dart-lang/site-www/blob/main/src/content/language/functions.md
- 3: https://github.com/dart-lang/language/blob/main/working/simpler-parameters/feature-specification.md
- 4: https://docs.syntblaze.com/dart/parameters/required-named-parameter
- 5: https://dart.dev/tools/diagnostics/missing_default_value_for_parameter
- 6: https://dart.googlesource.com/sdk/+/40323dfaafee4fe1b1008d85155232da462f01c9/pkg/linter/messages.yaml
🌐 Web query:
flutter_lints flutter.yaml rules prefer_named_parameters required named parameters
💡 Result:
There is no official lint rule named prefer_named_parameters in the standard Dart SDK or the flutter_lints package [1][2][3]. The flutter_lints package provides a curated, recommended set of lints for Flutter projects but does not include rules that enforce the use of named parameters over positional ones by default [1][4][5]. However, there are official Dart lint rules related to named parameters that you can enable manually in your analysis_options.yaml file [2][3]: 1. always_put_required_named_parameters_first: This rule enforces that required named parameters must appear before optional named parameters in a function definition [6][7]. 2. avoid_positional_boolean_parameters: While not strictly a "prefer named parameters" rule, this encourages better API design by warning against positional boolean arguments, which are often ambiguous, and suggesting named parameters instead [8]. If you specifically require a rule that warns against using positional parameters when named parameters would be more appropriate (e.g., for functions with many parameters), this is considered an opinionated style choice [9]. Such rules are generally not included in standard, community-wide lint packages like flutter_lints [1][10][9]. If you wish to use such a rule, you would need to look into third-party packages (e.g., saropa_lints) or implement a custom lint rule [10][9]. To enable any of the standard rules mentioned above, add them to your analysis_options.yaml file: linter: rules: - always_put_required_named_parameters_first - avoid_positional_boolean_parameters
Citations:
- 1: https://pub.dev/packages/flutter_lints
- 2: https://dart.dev/tools/linter-rules
- 3: https://dart.dev/tools/linter-rules/all
- 4: https://docs.flutter.dev/release/breaking-changes/flutter-lints-package
- 5: https://github.com/flutter/packages/tree/main/packages/flutter_lints
- 6: https://dart.dev/tools/linter-rules/always_put_required_named_parameters_first
- 7: https://dart.dev/tools/diagnostics/always_put_required_named_parameters_first
- 8: https://dart.dev/tools/linter-rules/avoid_positional_boolean_parameters
- 9: https://pub.dev/documentation/saropa_lints/latest/saropa_lints/PreferAllNamedParametersRule-class.html
- 10: https://pub.dev/documentation/saropa_lints/latest/saropa_lints/PreferNamedParametersRule-class.html
Make the required test-wrapping widgets explicit.
These helpers always need a widget, so mark it in the parameter ordering suggested by the flutter_lints config:
workout-logger/test/test_utils/test_harness.dart#L24-L26: userequired Widget childafter the empty required parameter list.workout-logger/test/test_utils/test_robot.dart#L18-L20: userequired Widget widgetafter the empty required parameter list.
📍 Affects 2 files
workout-logger/test/test_utils/test_harness.dart#L24-L32(this comment)workout-logger/test/test_utils/test_robot.dart#L18-L24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/test_utils/test_harness.dart` around lines 24 - 32, Make
the required widget parameters explicit in both helper methods: update
TestHarness.wrap in workout-logger/test/test_utils/test_harness.dart at lines
24-32 to use an empty required-parameter list followed by required Widget child,
and update the corresponding method in
workout-logger/test/test_utils/test_robot.dart at lines 18-24 to use required
Widget widget in the same ordering.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## r2.1.0 #63 +/- ##
===========================================
+ Coverage 48.76% 74.90% +26.14%
===========================================
Files 87 88 +1
Lines 14432 14491 +59
===========================================
+ Hits 7038 10855 +3817
+ Misses 7394 3636 -3758 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
workout-logger/lib/services/health_connect_service.dart (1)
334-349: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the sync path idempotent around timed-out writes.
Future.timeoutdrops the result but does not stopwriteRecords; Health Connect may still accept the workout before the timeout fires. If this catch returnsfalseand callers retry without a stableclientRecordIdin the record metadata, a late completion can create a duplicate workout. Add idempotent metadata for the session and avoid treating a write timeout/readback failure as a non-synced workout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workout-logger/lib/services/health_connect_service.dart` around lines 334 - 349, Update the session write flow around writeRecords and the subsequent readback so each workout record includes a stable clientRecordId derived from the session identity, allowing retries to be idempotent. Treat write timeouts and readback failures as an unknown or potentially synced outcome rather than returning false, and remove the temporary debug readback logging.workout-logger/test/test_utils/test_sweep.dart (1)
8-19: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMake
finderimmutable.Every reachable branch assigns it once; declare it
final.Proposed fix
- Finder finder; + final Finder finder;As per coding guidelines, “Prefer
finalfor local variables in Dart code.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workout-logger/test/test_utils/test_sweep.dart` around lines 8 - 19, Update the local finder variable in tapAll to be declared final, preserving the existing branch assignments and unsupported-target continue behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workout-logger/lib/screens/widgets/routine_creator.dart`:
- Around line 162-170: Update the onReorderItem callback to allow next ==
_selectedIds.length while still rejecting larger destinations; keep the
old-index validation unchanged. Add a reorder test covering moving an exercise
into that final position before the Add Exercises slot.
In `@workout-logger/lib/services/health_connect_service.dart`:
- Around line 61-65: Replace the universal 100 ms timeout in _getConnector and
the related Health Connect operations with configurable, operation-specific
deadlines: use realistic limits for bounded calls, remove the deadline from
consent/permission requests, and allow the 5,000-record heart-rate query
sufficient time to complete. Apply this across
workout-logger/lib/services/health_connect_service.dart at lines 61-65, 76-78,
90-95, 106-110, 128-135, 149-154, 171-178, 239-246, 261-268, and 286-296,
preserving existing unavailable, denied, and empty-result handling. Add
delayed-success tests covering platform responses that exceed 100 ms but remain
within the configured operation deadline.
In `@workout-logger/test/screens/history_screen_test.dart`:
- Around line 44-48: Update the test setup around TestHarness.prepareTester to
define and pass the intended viewportSize instead of allowing the harness
default to override the configured 800×1800 viewport. Pass viewportSize to both
TestHarness.prepareTester and TestHarness.wrap, preserving the existing viewport
cleanup.
In `@workout-logger/test/screens/programs/programs_screens_test.dart`:
- Around line 25-31: Make both workflows mandatory in
programs_screens_test.dart: at lines 25-31, assert the expected empty/list UI
and replace the optional FloatingActionButton interaction with a required
interaction; at lines 69-77, require the JSON field and Validate button, tap
Validate, and assert the successful validation feedback.
In `@workout-logger/test/screens/widgets/health_bar_chart_test.dart`:
- Line 47: Replace each unasserted tester.takeException() call with an assertion
that the captured exception is null. Apply this at
workout-logger/test/screens/widgets/health_bar_chart_test.dart lines 47, 54, 97,
and 104; workout-logger/test/screens/widgets/health_cards_test.dart lines 88,
131, 138, 168, and 175; and
workout-logger/test/screens/widgets/workout_hr_section_test.dart line 76,
preserving the existing render and interaction test flows.
In `@workout-logger/test/screens/widgets/rf_dialogs_test.dart`:
- Around line 53-85: Add coverage alongside showRFConfirmDialog for the default
non-danger styling path and cancellation behavior: open a dialog without
isDanger, dismiss it via the cancel action, and assert the awaited result is
false. Keep the existing danger-confirmation assertions intact.
- Around line 40-50: Update the snackbar interaction test around the Warning,
Error, and Info taps to wait for each snackbar hide/show animation to settle
before asserting the next snackbar. Replace the single-frame pumps with the test
framework’s settling wait, preserving the existing tap order and snackbar text
assertions.
In `@workout-logger/test/screens/widgets/rf_widgets_test.dart`:
- Around line 146-175: The RFTextField test should verify its focus-dependent
border colors, not only text entry. In the test covering RFTextField, assert the
rendered border uses AppColors.glassBorder before interaction, tap the
TextField, then assert it uses AppColors.primary while focused; retain the
existing controller wiring assertions.
In `@workout-logger/test/services/health_connect_service_test.dart`:
- Around line 65-120: The tests around HealthConnectService.syncWorkoutSession
currently stop at connector initialization and never validate record
construction. Replace the failing connector setup with a successful
connector/platform fake, then assert the emitted record for each scenario:
zero-rep and custom-exercise handling, fallback spacing for identical
timestamps, and empty-session behavior. Keep the existing session inputs and
verify the records passed through the connector rather than only asserting a
false return value.
---
Outside diff comments:
In `@workout-logger/lib/services/health_connect_service.dart`:
- Around line 334-349: Update the session write flow around writeRecords and the
subsequent readback so each workout record includes a stable clientRecordId
derived from the session identity, allowing retries to be idempotent. Treat
write timeouts and readback failures as an unknown or potentially synced outcome
rather than returning false, and remove the temporary debug readback logging.
In `@workout-logger/test/test_utils/test_sweep.dart`:
- Around line 8-19: Update the local finder variable in tapAll to be declared
final, preserving the existing branch assignments and unsupported-target
continue behavior.
🪄 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 Plus
Run ID: beeab699-bc57-4dac-a8e4-f077d6552f82
📒 Files selected for processing (32)
workout-logger/lib/screens/programs/import_program_screen.dartworkout-logger/lib/screens/widgets/health_detail_shell.dartworkout-logger/lib/screens/widgets/rf_dialogs.dartworkout-logger/lib/screens/widgets/rf_widgets.dartworkout-logger/lib/screens/widgets/routine_creator.dartworkout-logger/lib/services/health_connect_service.dartworkout-logger/test/screens/ai_coach_screen_test.dartworkout-logger/test/screens/ai_program_generator_screen_test.dartworkout-logger/test/screens/edit_workout_session_screen_test.dartworkout-logger/test/screens/heart_rate_detail_screen_test.dartworkout-logger/test/screens/history_screen_test.dartworkout-logger/test/screens/home_screen_test.dartworkout-logger/test/screens/onboarding_screen_test.dartworkout-logger/test/screens/profile_screen_test.dartworkout-logger/test/screens/programs/program_designer_screen_test.dartworkout-logger/test/screens/programs/programs_screens_test.dartworkout-logger/test/screens/settings_screen_test.dartworkout-logger/test/screens/sleep_detail_screen_test.dartworkout-logger/test/screens/widgets/health_bar_chart_test.dartworkout-logger/test/screens/widgets/health_cards_test.dartworkout-logger/test/screens/widgets/health_widgets_test.dartworkout-logger/test/screens/widgets/rf_dialogs_test.dartworkout-logger/test/screens/widgets/rf_widgets_test.dartworkout-logger/test/screens/widgets/targets_tab_test.dartworkout-logger/test/screens/widgets/workout_hr_section_test.dartworkout-logger/test/screens/workout_flow_screen_test.dartworkout-logger/test/services/health_connect_service_test.dartworkout-logger/test/sleep_hr_builder_test.dartworkout-logger/test/test_utils/test_harness.dartworkout-logger/test/test_utils/test_robot.dartworkout-logger/test/test_utils/test_sweep.dartworkout-logger/test/userflow_routine_creation_test.dart
💤 Files with no reviewable changes (2)
- workout-logger/lib/screens/programs/import_program_screen.dart
- workout-logger/test/sleep_hr_builder_test.dart
Comment on lines
+65
to
+120
| testWidgets('HealthConnectService handles sessions with zero reps and custom exercises', (WidgetTester tester) async { | ||
| final service = HealthConnectService(); | ||
| final session = WorkoutSession( | ||
| id: 'sess_custom', | ||
| date: DateTime.now(), | ||
| duration: 30, | ||
| notes: 'Custom notes', | ||
| exercises: [ | ||
| ExerciseLog( | ||
| exerciseId: 'custom_exercise_999', | ||
| sets: [ | ||
| WorkoutSet(weight: 0.0, reps: 0, timestamp: DateTime.now()), | ||
| WorkoutSet(weight: 50.0, reps: 10, timestamp: DateTime.now().add(const Duration(minutes: 5))), | ||
| ], | ||
| ), | ||
| ], | ||
| ); | ||
|
|
||
| final success = await service.syncWorkoutSession(session); | ||
| expect(success, isFalse); | ||
| }); | ||
|
|
||
| testWidgets('HealthConnectService handles sessions with identical timestamps (fallback spacing)', (WidgetTester tester) async { | ||
| final service = HealthConnectService(); | ||
| final now = DateTime.now(); | ||
| final session = WorkoutSession( | ||
| id: 'sess_identical_ts', | ||
| date: now, | ||
| duration: 45, | ||
| exercises: [ | ||
| ExerciseLog( | ||
| exerciseId: 'bench_press', | ||
| sets: [ | ||
| WorkoutSet(weight: 60.0, reps: 10, timestamp: now), | ||
| WorkoutSet(weight: 70.0, reps: 8, timestamp: now), | ||
| ], | ||
| ), | ||
| ], | ||
| ); | ||
|
|
||
| final success = await service.syncWorkoutSession(session, title: ''); | ||
| expect(success, isFalse); | ||
| }); | ||
|
|
||
| testWidgets('HealthConnectService handles empty sessions without exercises', (WidgetTester tester) async { | ||
| final service = HealthConnectService(); | ||
| final session = WorkoutSession( | ||
| id: 'sess_empty', | ||
| date: DateTime.now(), | ||
| duration: 20, | ||
| exercises: [], | ||
| ); | ||
|
|
||
| final success = await service.syncWorkoutSession(session); | ||
| expect(success, isFalse); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
These tests never exercise the session-segment cases named.
The mock makes _getConnector() fail, and syncWorkoutSession returns before record construction at workout-logger/lib/services/health_connect_service.dart Lines 316-317. Use a successful connector/platform fake and assert the emitted record for zero reps, custom exercises, identical timestamps, and empty sessions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/services/health_connect_service_test.dart` around lines
65 - 120, The tests around HealthConnectService.syncWorkoutSession currently
stop at connector initialization and never validate record construction. Replace
the failing connector setup with a successful connector/platform fake, then
assert the emitted record for each scenario: zero-rep and custom-exercise
handling, fallback spacing for identical timestamps, and empty-session behavior.
Keep the existing session inputs and verify the records passed through the
connector rather than only asserting a false return value.
Owner
Author
|
@coderabbiai review |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workout-logger/test/test_utils/test_harness.dart (1)
82-85: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake teardown wait for surface reset completion.
setSurfaceSizeupdates the tester’s surface metrics and drains microtasks; calling it without awaiting in teardown can let the final metrics flush run after the test starts.Proposed fix
- addTearDown(() { + addTearDown(() async { tester.view.resetPhysicalSize(); tester.view.resetDevicePixelRatio(); - tester.binding.setSurfaceSize(null); + await tester.binding.setSurfaceSize(null); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workout-logger/test/test_utils/test_harness.dart` around lines 82 - 85, Update the teardown callback containing tester.view resets and tester.binding.setSurfaceSize(null) to be asynchronous, and await setSurfaceSize(null) so surface metric updates and queued microtasks complete before teardown finishes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workout-logger/test/screens/ai_coach_screen_full_test.dart`:
- Line 42: Update the test locals in the relevant sections of the full-screen AI
coach test, including robot and sendIcon, to use final declarations because they
are never reassigned; preserve their existing initialization and usage.
- Around line 59-78: The configured-state test around AiCoachScreen must be
deterministic: keep it offline, assert the seed prompt and suggestion UI
explicitly, and require the send control with findsOneWidget instead of
conditionally tapping it. Remove submission behavior from this render test;
cover message submission separately using a deterministic fake AI service and
await the asynchronous stream before asserting results or errors.
In `@workout-logger/test/screens/widgets/editable_exercise_card_full_test.dart`:
- Around line 63-68: Strengthen the affected tests so missing widgets cannot be
skipped: in
workout-logger/test/screens/widgets/editable_exercise_card_full_test.dart#L63-L68,
assert the “+ Add Set” finder is findsOneWidget before tapping; in
workout-logger/test/screens/workout_flow_screen_full_test.dart#L34-L119, add
assertions that routine exercises render, dropset UI toggles, and
deload-adjusted sets/intensity are shown; in
workout-logger/test/screens/profile_screen_full_test.dart#L32-L58, assert the
“kg” and “lbs” chips exist and verify unit state changes after tapping; in
workout-logger/test/userflow_program_design_and_generator_test.dart#L109-L136,
assert the prompt chip and “Generate Program” button exist and verify either the
expected error state or generated program.
In `@workout-logger/test/screens/widgets/health_cards_test.dart`:
- Around line 85-87: Update the affected widget tests around the ReadinessCard
assertions to verify visible card output rather than merely finding the root
widget type. Assert the ReadinessCard semantics label “Readiness 85 out of 100”
and confirm the expected sheet opens after each tap; apply the same
rendered-output checks to the corresponding assertions at the other referenced
test sections.
In `@workout-logger/test/screens/widgets/routine_creator_test.dart`:
- Around line 26-36: Complete the routine creation test after the optional
CheckboxListTile selection: seed an exercise deterministically, confirm the
picker with “Add 1,” tap “Save,” and assert that the routine provider contains
“Upper Body Push.” Preserve the existing routine-name entry and
exercise-selection steps.
In `@workout-logger/test/services/health_connect_service_test.dart`:
- Around line 140-156: Update the delayed-response test for
HealthConnectService.isAvailable to return a Pigeon-encoded
HealthPlatformStatus.available after the 200 ms delay, then assert the result is
true. Add or retain a separate unavailable-response test that returns the
unavailable status and asserts false, so the delayed test specifically covers a
slow successful platform response.
In `@workout-logger/test/userflow_program_design_and_generator_test.dart`:
- Around line 32-107: Update
workout-logger/test/userflow_program_design_and_generator_test.dart#L32-L107 so
the flow reads the program created by the Save Program interaction from
programManager or storage and opens/asserts ProgramDetailScreen with that
result, removing the manually constructed sampleProgram path; update
workout-logger/test/userflow_screens_sweep_test.dart#L166-L212 to tap the
finish/complete action on WorkoutFlowScreen and assert against the resulting
session, rather than pumping WorkoutSummaryScreen with a manually constructed
WorkoutSession.
In `@workout-logger/test/userflow_programs_screen_deep_test.dart`:
- Around line 91-98: Update the FAB sweep in the test to capture the initial
FloatingActionButton finder results or count before tapping, then iterate over
that stable collection while ensuring each tap still targets the intended
ProgramsScreen FABs rather than widgets introduced by navigation. Keep the
existing tap and pumpAndSettle behavior.
In `@workout-logger/test/userflow_routine_creation_test.dart`:
- Around line 155-166: Update the test CreateRoutineScreen supports reordering
exercise into final position before Add Exercises to seed at least two selected
exercises in the providers or test state before building the screen. Invoke
onReorderItem with a destination index representing the final position, then
assert the displayed exercise order changes; retain the no-exception assertion
so the test exercises the actual final-position reorder path.
In `@workout-logger/test/userflow_targets_and_muscle_sheets_full_test.dart`:
- Around line 96-103: Strengthen the workflow tests so advertised outcomes are
required rather than skipped: in
workout-logger/test/userflow_targets_and_muscle_sheets_full_test.dart:96-103,
require the FloatingActionButton and assert the add-target dialog appears; at
:138-154, require the target field and action buttons, then assert phase/week
creation and save feedback or persistence. In
workout-logger/test/screens/programs/programs_screens_test.dart:25-29, assert
the FAB reaches its expected destination, and at :73-76, assert successful
validation feedback after submitting valid JSON.
In `@workout-logger/test/userflow_targets_and_muscle_sheets_test.dart`:
- Around line 120-127: Update the ReadinessCard test setup to inject a readiness
manager containing a ready snapshot, then assert the card’s visible content and
drill-down interaction before checking for exceptions. Use the existing
harness/provider symbols and preserve the no-exception assertion after
validating the rendered state.
- Around line 59-66: Strengthen the tests around TargetsTab: after persisting
populated targets, assert the rendered target-card content rather than only
TargetsTab visibility; in the editable-card test, interact with the card’s edit
control and verify the captured callback arguments. Update the relevant test
cases and their existing callback capture setup, preserving the current
persistence and screen setup.
---
Outside diff comments:
In `@workout-logger/test/test_utils/test_harness.dart`:
- Around line 82-85: Update the teardown callback containing tester.view resets
and tester.binding.setSurfaceSize(null) to be asynchronous, and await
setSurfaceSize(null) so surface metric updates and queued microtasks complete
before teardown finishes.
🪄 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 Plus
Run ID: 2aa89bc6-031a-4767-9657-ff513dff0177
⛔ Files ignored due to path filters (1)
workout-logger/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
workout-logger/lib/screens/widgets/readiness_card.dartworkout-logger/lib/screens/widgets/routine_creator.dartworkout-logger/lib/services/health_connect_service.dartworkout-logger/pubspec.yamlworkout-logger/test/screens/ai_coach_screen_full_test.dartworkout-logger/test/screens/history_screen_test.dartworkout-logger/test/screens/profile_screen_full_test.dartworkout-logger/test/screens/programs/programs_screens_test.dartworkout-logger/test/screens/widgets/editable_exercise_card_full_test.dartworkout-logger/test/screens/widgets/health_bar_chart_test.dartworkout-logger/test/screens/widgets/health_cards_test.dartworkout-logger/test/screens/widgets/rf_dialogs_test.dartworkout-logger/test/screens/widgets/rf_widgets_test.dartworkout-logger/test/screens/widgets/routine_creator_test.dartworkout-logger/test/screens/widgets/workout_hr_section_test.dartworkout-logger/test/screens/workout_flow_screen_full_test.dartworkout-logger/test/services/health_connect_service_test.dartworkout-logger/test/test_utils/test_harness.dartworkout-logger/test/test_utils/test_robot.dartworkout-logger/test/test_utils/test_sweep.dartworkout-logger/test/userflow_ai_coach_and_gemini_service_test.dartworkout-logger/test/userflow_health_and_profile_screen_test.dartworkout-logger/test/userflow_program_design_and_generator_test.dartworkout-logger/test/userflow_programs_screen_deep_test.dartworkout-logger/test/userflow_routine_creation_test.dartworkout-logger/test/userflow_screens_sweep_test.dartworkout-logger/test/userflow_services_and_ai_sweep_test.dartworkout-logger/test/userflow_targets_and_muscle_sheets_full_test.dartworkout-logger/test/userflow_targets_and_muscle_sheets_test.dart
Comment on lines
+59
to
+78
| aiService.init('valid_mock_api_key'); | ||
|
|
||
| await robot.pumpScreen( | ||
| const AiCoachScreen(seedPrompt: 'How can I improve my Bench Press?'), | ||
| storage: storage, | ||
| workoutProvider: workoutProvider, | ||
| geminiAiService: aiService, | ||
| settingsProvider: settingsProvider, | ||
| ); | ||
|
|
||
| robot.expectVisible(AiCoachScreen); | ||
| expect(find.byType(TextField), findsOneWidget); | ||
|
|
||
| final sendIcon = find.byIcon(Icons.arrow_upward_rounded); | ||
| if (sendIcon.evaluate().isNotEmpty) { | ||
| await tester.tap(sendIcon); | ||
| await tester.pump(); | ||
| } | ||
|
|
||
| expect(tester.takeException(), isNull); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make this configured-state test deterministic and assert the behavior it names.
This uses concrete GeminiAiService with a fake key, then conditionally starts message submission. The single pump() does not await the streamed reply/error, and a missing send icon makes the test pass without the intended interaction. Keep this render test offline and assert the seed/suggestions explicitly; cover submission separately with a deterministic fake service and a required findsOneWidget send control.
Based on supplied context, workout-logger/lib/viewmodels/ai_coach_view_model.dart:77-124 streams through the supplied AI service asynchronously.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/screens/ai_coach_screen_full_test.dart` around lines 59 -
78, The configured-state test around AiCoachScreen must be deterministic: keep
it offline, assert the seed prompt and suggestion UI explicitly, and require the
send control with findsOneWidget instead of conditionally tapping it. Remove
submission behavior from this render test; cover message submission separately
using a deterministic fake AI service and await the asynchronous stream before
asserting results or errors.
Comment on lines
+63
to
+68
| // Tap + Add Set | ||
| final addSetBtn = find.text('+ Add Set'); | ||
| if (addSetBtn.evaluate().isNotEmpty) { | ||
| await tester.tap(addSetBtn); | ||
| expect(setAdded, isTrue); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Conditional evaluate().isNotEmpty guards let regressions pass silently across several new test files. In each site below, an interaction (and sometimes the only functional assertion) only runs if a Finder happens to match; if the targeted widget fails to render, the guard is skipped and the test still passes, undermining the coverage this PR intends to add.
workout-logger/test/screens/widgets/editable_exercise_card_full_test.dart#L63-L68: assertfind.text('+ Add Set')isfindsOneWidgetunconditionally before tapping, so a missing button fails the test instead of skippingexpect(setAdded, isTrue).workout-logger/test/screens/workout_flow_screen_full_test.dart#L34-L119: add concrete assertions matching each test's stated intent (routine exercises rendered, dropset UI toggled, deload-adjusted sets/intensity shown) instead of relying on visibility + no-exception checks alone.workout-logger/test/screens/profile_screen_full_test.dart#L32-L58: assert the 'kg'/'lbs' chips exist unconditionally and verify the resulting unit-selection state changes after tapping, rather than only checking for absence of exceptions.workout-logger/test/userflow_program_design_and_generator_test.dart#L109-L136: assert the prompt chip and "Generate Program" button exist unconditionally, and check a concrete outcome (error state or generated program) instead of onlytester.takeException().
📍 Affects 4 files
workout-logger/test/screens/widgets/editable_exercise_card_full_test.dart#L63-L68(this comment)workout-logger/test/screens/workout_flow_screen_full_test.dart#L34-L119workout-logger/test/screens/profile_screen_full_test.dart#L32-L58workout-logger/test/userflow_program_design_and_generator_test.dart#L109-L136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/screens/widgets/editable_exercise_card_full_test.dart`
around lines 63 - 68, Strengthen the affected tests so missing widgets cannot be
skipped: in
workout-logger/test/screens/widgets/editable_exercise_card_full_test.dart#L63-L68,
assert the “+ Add Set” finder is findsOneWidget before tapping; in
workout-logger/test/screens/workout_flow_screen_full_test.dart#L34-L119, add
assertions that routine exercises render, dropset UI toggles, and
deload-adjusted sets/intensity are shown; in
workout-logger/test/screens/profile_screen_full_test.dart#L32-L58, assert the
“kg” and “lbs” chips exist and verify unit state changes after tapping; in
workout-logger/test/userflow_program_design_and_generator_test.dart#L109-L136,
assert the prompt chip and “Generate Program” button exist and verify either the
expected error state or generated program.
Comment on lines
+85
to
+87
| expect(tester.takeException(), isNull); | ||
|
|
||
| expect(find.byType(ReadinessCard), findsOneWidget); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert visible card output, not the root widget instance.
find.byType(...) always finds the widget supplied to pumpWidget, even if its build returns SizedBox.shrink(). Assert rendered metrics/semantics and the sheet opened after each tap; for ReadinessCard, its Readiness 85 out of 100 semantics label is a suitable assertion.
Also applies to: 128-135, 165-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/screens/widgets/health_cards_test.dart` around lines 85 -
87, Update the affected widget tests around the ReadinessCard assertions to
verify visible card output rather than merely finding the root widget type.
Assert the ReadinessCard semantics label “Readiness 85 out of 100” and confirm
the expected sheet opens after each tap; apply the same rendered-output checks
to the corresponding assertions at the other referenced test sections.
Comment on lines
+26
to
+36
| // Enter routine name via RFTextField | ||
| await robot.fill(find.byType(TextField).first, 'Upper Body Push'); | ||
|
|
||
| // Tap Add Exercises button | ||
| await robot.tap('Add Exercises'); | ||
|
|
||
| // Select exercise in sheet | ||
| final checks = find.byType(CheckboxListTile); | ||
| if (checks.evaluate().isNotEmpty) { | ||
| await robot.tap(checks.first); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Complete the create-and-save flow.
This test stops after an optional checkbox tap; it never confirms the picker, saves, or asserts that a routine was created. Seed an exercise deterministically, tap Add 1 and Save, then verify the provider contains Upper Body Push.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/screens/widgets/routine_creator_test.dart` around lines
26 - 36, Complete the routine creation test after the optional CheckboxListTile
selection: seed an exercise deterministically, confirm the picker with “Add 1,”
tap “Save,” and assert that the routine provider contains “Upper Body Push.”
Preserve the existing routine-name entry and exercise-selection steps.
Comment on lines
+91
to
+98
| // Tap FABs | ||
| final fabs = find.byType(FloatingActionButton); | ||
| expect(fabs, findsWidgets); | ||
|
|
||
| for (int i = 0; i < fabs.evaluate().length; i++) { | ||
| await tester.tap(fabs.at(i)); | ||
| await tester.pumpAndSettle(); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
FAB sweep re-evaluates against a possibly-navigated screen.
Both fabs.at(i) and the loop bound fabs.evaluate().length are recomputed live each iteration. If a tapped FAB pushes a new route, later iterations tap FABs belonging to the new screen using stale indices instead of continuing the intended sweep of ProgramsScreen, making coverage nondeterministic across runs.
🧪 Proposed fix
- final fabs = find.byType(FloatingActionButton);
- expect(fabs, findsWidgets);
-
- for (int i = 0; i < fabs.evaluate().length; i++) {
- await tester.tap(fabs.at(i));
- await tester.pumpAndSettle();
- }
+ final fabs = find.byType(FloatingActionButton);
+ final fabCount = fabs.evaluate().length;
+ expect(fabCount, greaterThan(0));
+
+ for (int i = 0; i < fabCount; i++) {
+ await tester.tap(find.byType(FloatingActionButton).at(i));
+ await tester.pumpAndSettle();
+ // Return to ProgramsScreen if the tap navigated away.
+ if (find.byType(ProgramsScreen).evaluate().isEmpty) {
+ await tester.pageBack();
+ await tester.pumpAndSettle();
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Tap FABs | |
| final fabs = find.byType(FloatingActionButton); | |
| expect(fabs, findsWidgets); | |
| for (int i = 0; i < fabs.evaluate().length; i++) { | |
| await tester.tap(fabs.at(i)); | |
| await tester.pumpAndSettle(); | |
| } | |
| // Tap FABs | |
| final fabs = find.byType(FloatingActionButton); | |
| final fabCount = fabs.evaluate().length; | |
| expect(fabCount, greaterThan(0)); | |
| for (int i = 0; i < fabCount; i++) { | |
| await tester.tap(find.byType(FloatingActionButton).at(i)); | |
| await tester.pumpAndSettle(); | |
| // Return to ProgramsScreen if the tap navigated away. | |
| if (find.byType(ProgramsScreen).evaluate().isEmpty) { | |
| await tester.pageBack(); | |
| await tester.pumpAndSettle(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/userflow_programs_screen_deep_test.dart` around lines 91
- 98, Update the FAB sweep in the test to capture the initial
FloatingActionButton finder results or count before tapping, then iterate over
that stable collection while ensuring each tap still targets the intended
ProgramsScreen FABs rather than widgets introduced by navigation. Keep the
existing tap and pumpAndSettle behavior.
Comment on lines
+155
to
+166
| testWidgets('CreateRoutineScreen supports reordering exercise into final position before Add Exercises', (tester) async { | ||
| await tester.pumpWidget(_buildTestApp( | ||
| workoutProvider: workoutProvider, | ||
| settingsProvider: settingsProvider, | ||
| child: const CreateRoutineScreen(), | ||
| )); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| final reorderableList = tester.widget<ReorderableListView>(find.byType(ReorderableListView)); | ||
| reorderableList.onReorderItem!(0, 1); | ||
| await tester.pumpAndSettle(); | ||
| expect(tester.takeException(), isNull); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise a real final-position reorder.
The screen has no selected exercises, so onReorderItem!(0, 1) returns immediately at routine_creator.dart Line 149. Seed at least two selected exercises, invoke the final destination index, and assert their displayed order changes; otherwise this passes even if the final-position fix regresses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/userflow_routine_creation_test.dart` around lines 155 -
166, Update the test CreateRoutineScreen supports reordering exercise into final
position before Add Exercises to seed at least two selected exercises in the
providers or test state before building the screen. Invoke onReorderItem with a
destination index representing the final position, then assert the displayed
exercise order changes; retain the no-exception assertion so the test exercises
the actual final-position reorder path.
Comment on lines
+96
to
+103
| // Tap FAB to add new target | ||
| final fab = find.byType(FloatingActionButton); | ||
| if (fab.evaluate().isNotEmpty) { | ||
| await tester.tap(fab.first); | ||
| await tester.pumpAndSettle(); | ||
| } | ||
|
|
||
| expect(tester.takeException(), isNull); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require and assert each advertised workflow outcome.
A missing or no-op control currently still passes these coverage tests.
workout-logger/test/userflow_targets_and_muscle_sheets_full_test.dart#L96-L103: require the FAB and assert the add-target dialog appears.workout-logger/test/userflow_targets_and_muscle_sheets_full_test.dart#L138-L154: require the field and buttons; assert phase/week creation and save feedback or persistence.workout-logger/test/screens/programs/programs_screens_test.dart#L25-L29: assert the FAB interaction reaches its expected destination.workout-logger/test/screens/programs/programs_screens_test.dart#L73-L76: assert successful validation feedback after submitting valid JSON.
📍 Affects 2 files
workout-logger/test/userflow_targets_and_muscle_sheets_full_test.dart#L96-L103(this comment)workout-logger/test/userflow_targets_and_muscle_sheets_full_test.dart#L138-L154workout-logger/test/screens/programs/programs_screens_test.dart#L25-L29workout-logger/test/screens/programs/programs_screens_test.dart#L73-L76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/userflow_targets_and_muscle_sheets_full_test.dart` around
lines 96 - 103, Strengthen the workflow tests so advertised outcomes are
required rather than skipped: in
workout-logger/test/userflow_targets_and_muscle_sheets_full_test.dart:96-103,
require the FloatingActionButton and assert the add-target dialog appears; at
:138-154, require the target field and action buttons, then assert phase/week
creation and save feedback or persistence. In
workout-logger/test/screens/programs/programs_screens_test.dart:25-29, assert
the FAB reaches its expected destination, and at :73-76, assert successful
validation feedback after submitting valid JSON.
Comment on lines
+59
to
+66
| await robot.pumpScreen( | ||
| const TargetsTab(), | ||
| storage: storage, | ||
| workoutProvider: workoutProvider, | ||
| settingsProvider: settingsProvider, | ||
| ); | ||
|
|
||
| robot.expectVisible(TargetsTab); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the behaviors named by these tests.
The populated-target test only verifies the screen shell, and the editable-card test never interacts with the card or validates a callback. Assert target-card content after persistence, then trigger an edit and verify captured callback arguments.
Also applies to: 86-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/userflow_targets_and_muscle_sheets_test.dart` around
lines 59 - 66, Strengthen the tests around TargetsTab: after persisting
populated targets, assert the rendered target-card content rather than only
TargetsTab visibility; in the editable-card test, interact with the card’s edit
control and verify the captured callback arguments. Update the relevant test
cases and their existing callback capture setup, preserving the current
persistence and screen setup.
Comment on lines
+120
to
+127
| await robot.pumpScreen( | ||
| const ReadinessCard(), | ||
| storage: storage, | ||
| workoutProvider: workoutProvider, | ||
| settingsProvider: settingsProvider, | ||
| ); | ||
|
|
||
| expect(tester.takeException(), isNull); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Provide a ready snapshot before claiming ReadinessCard renders.
No readiness manager is injected, so the harness supplies one with no snapshot. ReadinessCard therefore builds its empty state; assert a ready snapshot, visible card content, and the drill-down interaction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/userflow_targets_and_muscle_sheets_test.dart` around
lines 120 - 127, Update the ReadinessCard test setup to inject a readiness
manager containing a ready snapshot, then assert the card’s visible content and
drill-down interaction before checking for exceptions. Use the existing
harness/provider symbols and preserve the no-exception assertion after
validating the rendered state.
Devasy
added a commit
that referenced
this pull request
Sep 1, 2026
* Feat/android 17 (#59)
* feat: add localized summaries, app metadata, and signing block information to F-Droid repository data
* chore: upgrade Android SDK 16→17, Java 11→17, Gradle/AGP/Kotlin toolchain
- compileSdk + targetSdk: 36 → 37 (Android 17 / API 37)
- Removed compileSdkExtension (not needed for base API 37)
- Java source/target compatibility: VERSION_11 → VERSION_17
- Kotlin jvmTarget: 11 → 17
- Gradle wrapper: 8.12 → 8.14.1
- AGP: 8.9.1 → 8.11.1
- Kotlin Gradle Plugin: 2.1.0 → 2.2.20
- Enable android.builtInKotlin=true + android.newDsl=true
- Remove explicit id(kotlin-android) plugin (now injected by Flutter)
* chore: update pubspec.lock (transitive dependency bumps)
* chore: update repo name and username references to RepForge and Devasy
* upadtes the build gradle kts file to match the review comment
* Adds pubspec yaml
---------
Co-authored-by: Devasy Patel <110348311+Devasy23@users.noreply.github.com>
* Upgrades bottom nav bar Upgrade Bottom Navigation Bar (#61)
* feat: add localized summaries, app metadata, and signing block information to F-Droid repository data
* chore: upgrade Android SDK 16→17, Java 11→17, Gradle/AGP/Kotlin toolchain
- compileSdk + targetSdk: 36 → 37 (Android 17 / API 37)
- Removed compileSdkExtension (not needed for base API 37)
- Java source/target compatibility: VERSION_11 → VERSION_17
- Kotlin jvmTarget: 11 → 17
- Gradle wrapper: 8.12 → 8.14.1
- AGP: 8.9.1 → 8.11.1
- Kotlin Gradle Plugin: 2.1.0 → 2.2.20
- Enable android.builtInKotlin=true + android.newDsl=true
- Remove explicit id(kotlin-android) plugin (now injected by Flutter)
* chore: update pubspec.lock (transitive dependency bumps)
* chore: update repo name and username references to RepForge and Devasy
* upadtes the build gradle kts file to match the review comment
* Adds pubspec yaml
* Enhances the bottom nav bar
* fixes out bulging issue
* Updates the bottom navbar UI, and then adds build size reuction params
* Adds build script and upgrades the release workflow
* Adds tests
* updates acc to review comments
* Adds gitignore and updates codecov yaml
* updated comments according to review comments
---------
Co-authored-by: Devasy Patel <110348311+Devasy23@users.noreply.github.com>
* Feat/increase coverage test screens (#63)
* Adds tests for screens
* Adds tests
* Adds comprehensive tests
* Adds new tests
* Updates test.yml to run on release branches
* Adds test and resolved the warnings and issues
* Updates tests and minor bug fixes
* Adds fixes for failing testsm and adds connection timeout safety for health connector
* Adds missing lines patch
* Updates the tests with analyse failures
* Updates tests and routine creator to use the common component
* Updates flutter version and adds tests
---------
Co-authored-by: Devasy Patel <110348311+Devasy23@users.noreply.github.com>
* Feat/genui (#64)
* Adds tests for screens
* Adds tests
* Adds comprehensive tests
* Adds new tests
* Updates test.yml to run on release branches
* Adds test and resolved the warnings and issues
* Updates tests and minor bug fixes
* Adds fixes for failing testsm and adds connection timeout safety for health connector
* Adds missing lines patch
* Updates the tests with analyse failures
* Updates tests and routine creator to use the common component
* Updates flutter version and adds tests
* Adds major genui Feature and renderer
* chore: remove patch_so script
* build: add --build-id=none for jni package in F-Droid metadata
* ci: add jni build-id sed step for future reproducible releases
* feat: assisted pullups, deload-aware ML, handle-scoped PRs, sleeping HR tool
Batches several in-flight features that were sitting uncommitted:
- Bodyweight/assisted pullup volume: (BW - assist + extra) * reps
- MLService reads the past 3 sessions and recovers from a deload week
using the pre-deload baseline instead of the deload trough
- PRManager scopes records per handle variation (Rope vs Bar)
- CoachToolService.get_sleeping_hr_analytics: p5/p25/mean, stdev,
variance and linear trend over the last N nights
- GenUI parser tolerates numeric StatCard values, loose trend words and
Markdown code fences
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add A2UiProps alias-aware coercing property reader
Foundation for the genui refactor: a never-throwing view over raw
component prop maps that resolves keys by exact match, normalized
match (case/underscore/hyphen/space-insensitive), then semantic
alias, and coerces values to typed accessors with documented
fallbacks instead of throwing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add A2UiSpec contract, A2UiNode and A2UiRegistry
Adds the four-in-one component contract (A2UiSpec) that lets each UI
component name itself, parse its own props, build its own widget and
document itself for the LLM prompt on one object, plus the
A2UiRegistry lookup table that replaces the old allowedA2UiComponents
set and two parallel switch statements. Includes an A2UiTheme skeleton
(filled in by Task 4) and A2UiNode, the parsed-tree node type.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): make A2UiRegistry throw on name/alias collisions
Code review found that A2UiRegistry's constructor loop silently
resolved canonical-name/alias collisions (last-writer-wins for names,
first-writer-wins for aliases), which would produce unreachable specs
or dropped aliases with no signal as more components are registered in
later tasks. The constructor now throws a StateError identifying both
colliding specs for any of: two specs sharing a canonical name, an
alias colliding with another spec's canonical name, or two specs
sharing an alias. Adds three regression tests using a new configurable
_NamedFakeSpec fake.
Also documents (doc-comment only, no behavior change) that
A2UiNode.children is not defensively copied, per the review's Minor
finding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add A2UiParser with fence, envelope and alias repair
Adds the single gate that decides whether an LLM reply is a UI payload
or ordinary prose, and turns UI payloads into an A2UiNode tree. Handles
markdown fences, prose-wrapped JSON, flat vs props-wrapped shapes,
bare-array/envelope auto-wrapping into GridContainer, and recursive
children, without ever throwing.
Also promotes A2UiProps._asStringKeyed to a public static
A2UiProps.stringKeyed so the parser can re-key decoded JSON maps
without an awkward part-of coupling between the two libraries.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): balanced-bracket JSON extraction and envelope singleton fix
_extractJson previously sliced from the first { to the last }, which
broke on any stray brace in surrounding prose (e.g. "add reps
{optional}"). Replace with a scan that tries jsonDecode on every
balanced {..}/[..] span found via a depth counter that correctly skips
brackets inside string literals, preferring the longest successful
decode as the actual payload.
Also fix _wrap's unconditional single-child collapse: an explicit
envelope key ({"components":[...]}) is a deliberate container request
and must still produce a GridContainer with one child, while a bare
top-level array with one item keeps collapsing since it's ambiguous
between "a list of one" and "just one component."
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): inject A2UiTheme and extract shared panel chrome
Adds A2UiThemeProvider (InheritedWidget, falls back to A2UiTheme.dark)
and the panel/title/empty-state/legend widgets every component spec
will share, plus lib/theme/a2ui_app_theme.dart mapping RepForge's real
design tokens onto A2UiTheme. This is the only file where the two
systems meet - lib/genui/ still imports nothing app-specific.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(genui): strengthen theme-injection and add A2UiPanel coverage
The injection test compared against repforgeA2UiTheme, which is
field-for-field identical to the A2UiThemeProvider.of fallback
(A2UiTheme.dark), so it passed even if the InheritedWidget lookup were
broken. Inject a fixture with distinct values instead, and assert a
sibling context still falls back to the default. Also add direct
coverage for A2UiPanel's padding, decoration, and child rendering,
previously only exercised indirectly via A2UiEmptyPanel.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add A2UiSeries as the shared categorical data shape
A2UiSeries.extract() and maxValue() give line/bar/pie and radar chart
components one common {name, values} shape to consume, so a model that
learns {labels, series} once can drive all four components.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): cover fallback path in A2UiSeries.extract, fix negative-max bug
Address code review findings on A2UiSeries:
- Add tests pinning down the series->values fallback when every series
entry drops to empty/unparseable values, and when series is an empty
list — the risky path the brief called out but left untested.
- Rename the misleading 'reads the axes alias' test; it only exercised
stringified-number coercion inside series values, not alias resolution.
- Fix maxValue() to track whether any value has been seen instead of
seeding with 0.0, so all-negative series report their true max
instead of silently clamping to 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add StatCardSpec with typed props and trend synonyms
Establishes the pattern for Tasks 7-13: a typed props record, an
A2UiSpec bundling name/aliases/doc/parseProps/buildWidget, and
never-throwing parsing that degrades to documented fallbacks.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add MetricGaugeSpec with safe progress and null value
Fixes the validator/renderer contradiction where a String value was
accepted but cast to num, and the min == max NaN sweep angle bug.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add DynamicChartSpec for line, bar and pie
Adds the most-used and most complex A2UI component so far, covering
line/bar/pie rendering over the shared {labels, series} shape with
never-throwing prop parsing and label padding to prevent out-of-range
axis lookups.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add ScatterPlotSpec with point repair and safe bounds
Adds paired x/y observation plotting with an optional correlation badge,
following the Task 6-8 A2UiSpec pattern. Malformed points are dropped
rather than throwing, and bounds widen degenerate axes so fl_chart never
sees a zero-span range.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add RadarChartSpec sharing the labels/series shape
Task 10 of the a2ui/genui refactor: RadarChart consumes the same
{labels, series} shape as DynamicChart, with `axes` kept as a
backward-compatible alias for `labels`. Every series is truncated
or zero-padded to labels.length at parse time so fl_chart's radar
never sees a mismatched entry count.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add DataListGroupSpec with row repair and optional title
Adds a titled list-of-rows component with a defensive row-extraction
fallback chain: named fields, bare scalars, first-stringifiable-value
fallback, and silent drop of rows with nothing displayable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add FilterChipsSpec with nullable active option
Renders a decorative, non-interactive row of scope chips (e.g. "7d /
30d / 90d") and fixes the old renderer's `activeOption as String`
crash by matching case-insensitively and falling back to null instead
of throwing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add GridContainerSpec, default registry and renderer
Task 13: assembles all eight leaf components into defaultA2UiRegistry,
adds the GridContainerSpec layout wrapper, the public A2UiRenderer
widget, and the lib/genui/a2ui.dart barrel file that will be the only
import path the rest of the app uses going forward.
fix(genui): make structural children lookup exact, not alias-resolved
Cross-task fix to a2ui_parser.dart (a Task 3 file), discovered during
Task 13 registry integration. A2UiParser._parseChildren and
_declaresChildren resolved the structural `children` key through
A2UiProps' alias-aware lookup(), which treats `items` as an alias for
`children`. That collided with DataListGroupSpec, whose own canonical
data-row key is also `items`: a DataListGroup node's `items` list of
{primaryText, ...} maps was mistaken for child components, none of
them parsed as one, and the whole node was then discarded as an
emptied-out container. Reading the literal `children` key only fixes
this and matches the precision _envelopeKeys already had (it does not
include `items` as a synonym for `children` either).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): generate the A2UI prompt section from the registry
Replaces hand-written component-schema prose in the coach system
prompt with a section generated from defaultA2UiRegistry, so the
vocabulary advertised to the model can never drift from what the
parser/renderer actually support.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(genui): wire coach screen to the A2UI package, drop legacy renderer
Replaces private _CoachMessageContent with a public, stateful
CoachMessageContent that memoizes parsing per text value and shows a
"Building dashboard..." placeholder for partial JSON while streaming,
instead of letting raw braces scroll past or losing prose on a mixed
reply. Wraps the app root in A2UiThemeProvider(theme: repforgeA2UiTheme)
so the renderer picks up RepForge's design tokens. Deletes the
superseded lib/genui/a2ui_component.dart and lib/genui/a2ui_renderer.dart,
and drops test/new_features_test.dart's GenUI Component Resilience Tests
group, whose two cases are already covered more thoroughly by
test/genui/a2ui_parser_test.dart.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): bracket negative-value ranges in DynamicChart line/bar axes
minY was hardcoded to 0 while maxY derived from the true series max, so an
all-negative dataset (e.g. [-10, -5, -3]) produced a visible axis range of
[0, 1] with every real data point falling outside it — a silent blank
chart despite valid, non-empty data. Adds A2UiSeries.minValue mirroring
the existing maxValue, and a shared _yBounds helper used by both _line and
_bar so the two renderers can't diverge on axis math. Also covers
multi-series label padding, which was previously only exercised through
series[0].
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(genui): cover all-negative bounds and malformed point entries
Task 9 review flagged that ScatterPlotProps.bounds had no regression pin
for all-negative-coordinate spreads (same failure class as Task 8's
DynamicChartSpec axis bug) and that point-parsing had no test for
structurally invalid entries (nested objects, raw lists). Adds both.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): widen per-node children lookup back to components/elements/content
Follow-up to the Task 13 a2ui_parser.dart fix: restricting the per-node
_parseChildren/_declaresChildren lookup to the literal 'children' key
was narrower than intended. It regressed 'components'/'elements'/
'content' as per-node child-list keys, which never collided with
anything (only 'items' did, via DataListGroup's own canonical data key).
A payload like {"component":"GridContainer","props":{"columns":1,
"components":[...]}} resolved fine before the original bug and silently
rendered blank (zero children, no null fallback) after the first fix,
since _declaresChildren no longer recognized 'components' as a
children-declaring key either.
Adds a _childKeys constant (children/components/elements/content,
still excluding items) mirroring _envelopeKeys' existing tolerance, and
routes both _parseChildren and _declaresChildren through a shared
_firstChildList literal (non-alias) lookup over that key set.
Adds regression tests in a2ui_renderer_test.dart: per-node
components/elements/content resolve to real children, and items stays
excluded at the per-node level.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): widen looksLikeUi to catch prose-prefixed fences, fix vacuous memoization test
looksLikeUi only checked whether the text, after stripping a *leading*
fence, started with `{`/`[`. A model that writes a sentence before
opening a fenced payload (e.g. "Here is your data:\n```json\n{...")
fell through undetected, so CoachMessageContent showed the raw partial
JSON instead of the streaming placeholder -- the exact symptom this
task exists to fix. Now also treats an unclosed ``` fence found
anywhere in the streamed-so-far text as a UI signal, while plain prose
with no JSON or fence anywhere still returns false.
Also fixes the memoization regression test in
test/screens/ai_coach_genui_test.dart: the second observation was
taken after a bare `tester.pump()`, which doesn't mark the element
dirty and never actually calls build() again, so the test could not
distinguish memoized parsing from a widget that never rebuilds at all.
It now pumps a second CoachMessageContent instance with identical text
at the same tree location, which reuses the existing State and
genuinely triggers didUpdateWidget/build.
Adds regression tests for both the prose-prefixed-fence case and the
plain-prose-no-json case in test/genui/a2ui_parser_test.dart, plus a
widget-level test in test/screens/ai_coach_genui_test.dart confirming
the placeholder (not raw JSON) renders end-to-end.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(genui): drop presentation payload from tools, add purity and fuzz suites
The sleeping-HR analytics tool was hand-constructing an A2UI DynamicChart
payload directly, leaking presentation decisions into the data layer.
Replace `genui_chart_props` with neutral `labels`/`series` keys so the
prompt — not the tool — decides how to present the data.
Add two permanent guard suites: a2ui_purity_test.dart proves lib/genui/
never imports app-specific code (theme/models/services/screens) and its
component renderers never cast raw model data; a2ui_robustness_test.dart
fuzzes the parser and renderer against ~26 hostile/malformed LLM payloads
to confirm nothing throws.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): depth-agnostic purity regex, pin two silent-visual regressions
Review of the previous commit found the purity test's forbidden-import
check was depth-blind: its literal needle list only covered one and two
../ hops, but components live three levels below lib/, so a real
../../../theme/... import passed undetected. Replace it with a regex
that matches any number of ../ hops (or a package:repforge/ prefix),
covering import and export directives alike, and add a self-test that
proves the regex catches every relevant depth/form without touching real
source files.
Also widen the no-raw-casts check to include bool/Object/dynamic, make
the components-directory scan recursive, and pin down the two historical
silent-visual regressions (Task 8's chart axis-bounds clamp, Task 13's
GridContainer child-key aliasing) with positive assertions in the fuzz
suite, since neither throws and the existing no-throw checks structurally
can't catch either.
Reword analyze_health_workout_correlation's tool declaration to drop
direct component names, closing the same presentation-leak class this
task already fixed for the sleeping-HR tool.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): propagate registry through recursion, pin prompt drift, close review findings
Final whole-branch review fix wave for the A2UI genui refactor:
- A2UiRenderer's registry override used to be silently dropped past one
level of nesting because GridContainerSpec recurses via bare
A2UiRenderer(node: ...) calls. Mirror the existing theme-injection
pattern with a new A2UiRegistryProvider InheritedWidget so an explicit
registry override at any level propagates ambiently to everything below
it (explicit param > inherited provider > defaultA2UiRegistry fallback).
- Pin the hand-written "WHICH COMPONENT TO REACH FOR" prose in
gemini_context_builder.dart against silent drift: every component name
it mentions must resolve in defaultA2UiRegistry, and the registry's
spec count is asserted directly.
- Delete A2UiProps.object()/has() — confirmed zero call sites.
- Repurpose the orphaned Task 3 scaffolding test
(a2ui_parser_stub_test.dart, redundant with a2ui_parser_test.dart) into
a2ui_custom_registry_test.dart, the regression coverage the registry-
propagation fix needed.
- Add scanned-file-count floors to the purity test's two directory scans
so an empty/unreachable directory can't produce a vacuous pass.
- Document FilterChips' SizedBox.shrink() as a deliberate exception to
the plan's "always A2UiEmptyPanel" rule (decorative chrome, not data).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: add design spec for Hive->SQLite migration + coach SQL query tool
* fix: persist assisted-load volume correctly, tighten exercise-handle scoping
- WorkoutSet now snapshots bodyweight/assist/extra at logging time instead
of recomputing effective load from the CURRENT profile bodyweight on every
read, which was silently corrupting historical volume whenever a user
updated their weight. ExerciseLog.totalVolume and the workout_flow_screen
logging path thread the snapshot through.
- Exercise-handle matching (workout_provider) now requires an exact handle
match whenever a handle is set, falling back to legacy behavior only when
no exact match exists — a null-handle log was previously matching ANY
requested handle, surfacing the wrong variation's "last session" data.
- Handle selector no longer visually pre-selects an unpersisted handle, and
setExerciseHandle no longer retroactively relabels already-logged sets.
- Assisted-load display values now respect the user's unit preference; the
assisted-exercise classification is computed once and shared instead of
drifting between two separate predicates.
- Body-weight input (settings_provider) now rejects non-finite/non-positive
values on both the load and set paths, falling back to 70.0 when invalid.
- ml_service: deload-recovery reasoning no longer hardcodes "kg" regardless
of unit settings; recovery detection now requires the comparison session
to be recent and uses effective (not raw) load for assisted exercises.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: bound sleep-analytics window, drop fabricated data, resolve muscle groups by id
- get_sleeping_hr_analytics clamps the model-provided days window instead of
looping unbounded; get_health_metrics now honors the requested days window
instead of always querying one week, and both its and the correlation
tool's declarations no longer advertise fields (resting HR, readiness)
that aren't actually backed by implementation.
- analyze_health_workout_correlation no longer fabricates synthetic sleep
data points to pad out insufficient real pairs — returns the existing
insufficient-data error instead, so correlation/regression/chart output is
never partly made up.
- get_muscle_group_volume now resolves requested names to ids via
_resolveMuscleGroup and compares ids (also aggregating secondary muscle
activations) instead of raw display-name substring matching.
- CoachToolService's optional HealthHistoryManager is now a named parameter.
- gemini_ai_service: daily-quota classification narrowed to actual
daily-limit identifiers so minute-scale rate limits go through normal
retry-delay handling instead of being misclassified as daily exhaustion;
function-call ids are now preserved and matched into their responses;
the fallback path now builds a thinkingConfig compatible with whichever
model was actually selected. Mirrored in scripts/test_gemini_api.py.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): pie negative-value filtering, overflow guard, stat-card unit match
- DynamicChart's pie mode now filters to positive values before computing
percentages/sections (preserving original index alignment with labels and
series colors), falling back to an empty panel when nothing positive
remains, instead of rendering a nonsense chart from negative/zero data.
- A2UiPanelTitle's trailing label is now Flexible with maxLines/ellipsis so
a long model-provided string can't overflow the row.
- StatCard's unit-already-present check now requires a trailing-suffix
match instead of any substring, fixing a false positive like unit "s"
matching inside value "10 reps".
- MetricGauge's arc painter now also compares `track` in shouldRepaint, so
a background-color-only change still triggers a repaint.
- A2UiTheme.seriesColor asserts a non-empty palette before the modulo index
that would otherwise throw on one.
- A2UiParser: props/outer-children now merge (props wins on conflict) so a
model writing children as a sibling of props isn't silently dropped; adds
a whole-text jsonDecode fast path ahead of the balanced-span scan.
- A2UiRenderer logs the unresolved component name via the app's existing
debugPrint/kDebugMode convention before falling back to an empty widget.
- a2ui_app_theme now imports A2UiTheme via the public genui barrel instead
of an internal src path.
- CI: the release workflow's linker-patch step now requires and quotes
PUB_CACHE, restricts the patch to resolved jni-*/src/CMakeLists.txt
targets, is idempotent against re-runs, and fails the build instead of
silently continuing when no target is found or patching fails.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test: close vacuous-test gaps and pin already-fixed regressions
Fixes tests that would pass identically whether the behavior they claim to
verify was correct or broken:
- stat_card_test's pump() helper now actually threads its props argument
into the rendered node (it previously always rendered empty props).
- new_features_test's assisted-pullups case now uses distinguishable
weight/assistWeight values, so the test fails if the wrong field is used.
Tightens two guardrail-class tests to actually detect what they claim to:
- a2ui_prompt_test's worked-example extraction is now bounded to the region
after the "WORKED EXAMPLE:" marker via balanced-brace matching, instead of
the last '}' anywhere in the whole prompt.
- a2ui_purity_test's forbidden-import regex now also guards lib/data/.
- a2ui_robustness_test's negative-axis assertion now requires minY to
actually bracket the dataset's true minimum, not just be below -10.
- a2ui_theme_test's panel-decoration finders are scoped to the panel under
test rather than the first Container anywhere in the tree.
Adds regression coverage pinning fixes already shipped in prior commits:
DynamicChart pie's negative-value filtering, StatCard's unit-suffix match,
and CoachToolService's days-window/insufficient-data/muscle-id fixes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: address CodeRabbit review findings on PR #64 (feat/genui)
Fixes real findings from PR #64's own review, ahead of merging into
r2.1.0, so the sqflite-migration branch (which currently carries these
genui files unmerged) won't reintroduce them as merge conflicts.
- a2ui_theme: seriesColor() now falls back to accent on an empty
seriesPalette instead of only asserting (release builds strip
asserts, so this was still a release-mode divide-by-zero)
- coach_tool_service: removed the synthetic "readiness_score" metric
from analyze_health_workout_correlation — it was a made-up
70-100 formula derived from sleep duration, presented as if it were
an independent measured health signal in statistical output
- coach_tool_service, main.dart: CoachToolService constructor now uses
named parameters (3+ args); updated every call site
- workout_provider: getRecommendations no longer passes the
exercise-wide growth model into a handle-scoped recommendation,
since _growthModels isn't trained per-handle and would mix
variations (e.g. "Rope pushdown" trend bleeding into "Bar pushdown")
- test_gemini_api.py: post_generate_content_with_retry could fall off
the end returning None after a quota-fallback on the final attempt,
despite its dict return type; restructured so every path returns or
raises
- test coverage: legend-absence assertions for single-series/pie
charts, NaN/Infinity scatter-point coordinates, stable payload-based
test names in the robustness suite, hoisted regex in the purity
test, const constructor, and a corrected self-contradictory comment
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: Devasy Patel <110348311+Devasy23@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* test: drop coverage for ApiService/SettingsScreen removed by main merge
Merging main brought in the telemetry removal (ApiService and the
orphaned settings_screen.dart are gone). r2.1.0 had its own test
coverage for both that main never had - api_service_test.dart,
screens/settings_screen_test.dart, and the SettingsScreen-only half
of userflow_settings_and_storage_test.dart all targeted code that no
longer exists, so they're deleted. test_harness.dart drops its
ApiService provider registration, which nothing consumes anymore.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Migrate storage from Hive to SQLite + add coach SQL query tool (#66)
* Adds tests for screens
* Adds tests
* Adds comprehensive tests
* Adds new tests
* Updates test.yml to run on release branches
* Adds test and resolved the warnings and issues
* Updates tests and minor bug fixes
* Adds fixes for failing testsm and adds connection timeout safety for health connector
* Adds missing lines patch
* Updates the tests with analyse failures
* Updates tests and routine creator to use the common component
* Updates flutter version and adds tests
* Adds major genui Feature and renderer
* chore: remove patch_so script
* build: add --build-id=none for jni package in F-Droid metadata
* ci: add jni build-id sed step for future reproducible releases
* feat: assisted pullups, deload-aware ML, handle-scoped PRs, sleeping HR tool
Batches several in-flight features that were sitting uncommitted:
- Bodyweight/assisted pullup volume: (BW - assist + extra) * reps
- MLService reads the past 3 sessions and recovers from a deload week
using the pre-deload baseline instead of the deload trough
- PRManager scopes records per handle variation (Rope vs Bar)
- CoachToolService.get_sleeping_hr_analytics: p5/p25/mean, stdev,
variance and linear trend over the last N nights
- GenUI parser tolerates numeric StatCard values, loose trend words and
Markdown code fences
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add A2UiProps alias-aware coercing property reader
Foundation for the genui refactor: a never-throwing view over raw
component prop maps that resolves keys by exact match, normalized
match (case/underscore/hyphen/space-insensitive), then semantic
alias, and coerces values to typed accessors with documented
fallbacks instead of throwing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add A2UiSpec contract, A2UiNode and A2UiRegistry
Adds the four-in-one component contract (A2UiSpec) that lets each UI
component name itself, parse its own props, build its own widget and
document itself for the LLM prompt on one object, plus the
A2UiRegistry lookup table that replaces the old allowedA2UiComponents
set and two parallel switch statements. Includes an A2UiTheme skeleton
(filled in by Task 4) and A2UiNode, the parsed-tree node type.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): make A2UiRegistry throw on name/alias collisions
Code review found that A2UiRegistry's constructor loop silently
resolved canonical-name/alias collisions (last-writer-wins for names,
first-writer-wins for aliases), which would produce unreachable specs
or dropped aliases with no signal as more components are registered in
later tasks. The constructor now throws a StateError identifying both
colliding specs for any of: two specs sharing a canonical name, an
alias colliding with another spec's canonical name, or two specs
sharing an alias. Adds three regression tests using a new configurable
_NamedFakeSpec fake.
Also documents (doc-comment only, no behavior change) that
A2UiNode.children is not defensively copied, per the review's Minor
finding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add A2UiParser with fence, envelope and alias repair
Adds the single gate that decides whether an LLM reply is a UI payload
or ordinary prose, and turns UI payloads into an A2UiNode tree. Handles
markdown fences, prose-wrapped JSON, flat vs props-wrapped shapes,
bare-array/envelope auto-wrapping into GridContainer, and recursive
children, without ever throwing.
Also promotes A2UiProps._asStringKeyed to a public static
A2UiProps.stringKeyed so the parser can re-key decoded JSON maps
without an awkward part-of coupling between the two libraries.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): balanced-bracket JSON extraction and envelope singleton fix
_extractJson previously sliced from the first { to the last }, which
broke on any stray brace in surrounding prose (e.g. "add reps
{optional}"). Replace with a scan that tries jsonDecode on every
balanced {..}/[..] span found via a depth counter that correctly skips
brackets inside string literals, preferring the longest successful
decode as the actual payload.
Also fix _wrap's unconditional single-child collapse: an explicit
envelope key ({"components":[...]}) is a deliberate container request
and must still produce a GridContainer with one child, while a bare
top-level array with one item keeps collapsing since it's ambiguous
between "a list of one" and "just one component."
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): inject A2UiTheme and extract shared panel chrome
Adds A2UiThemeProvider (InheritedWidget, falls back to A2UiTheme.dark)
and the panel/title/empty-state/legend widgets every component spec
will share, plus lib/theme/a2ui_app_theme.dart mapping RepForge's real
design tokens onto A2UiTheme. This is the only file where the two
systems meet - lib/genui/ still imports nothing app-specific.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(genui): strengthen theme-injection and add A2UiPanel coverage
The injection test compared against repforgeA2UiTheme, which is
field-for-field identical to the A2UiThemeProvider.of fallback
(A2UiTheme.dark), so it passed even if the InheritedWidget lookup were
broken. Inject a fixture with distinct values instead, and assert a
sibling context still falls back to the default. Also add direct
coverage for A2UiPanel's padding, decoration, and child rendering,
previously only exercised indirectly via A2UiEmptyPanel.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add A2UiSeries as the shared categorical data shape
A2UiSeries.extract() and maxValue() give line/bar/pie and radar chart
components one common {name, values} shape to consume, so a model that
learns {labels, series} once can drive all four components.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): cover fallback path in A2UiSeries.extract, fix negative-max bug
Address code review findings on A2UiSeries:
- Add tests pinning down the series->values fallback when every series
entry drops to empty/unparseable values, and when series is an empty
list — the risky path the brief called out but left untested.
- Rename the misleading 'reads the axes alias' test; it only exercised
stringified-number coercion inside series values, not alias resolution.
- Fix maxValue() to track whether any value has been seen instead of
seeding with 0.0, so all-negative series report their true max
instead of silently clamping to 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add StatCardSpec with typed props and trend synonyms
Establishes the pattern for Tasks 7-13: a typed props record, an
A2UiSpec bundling name/aliases/doc/parseProps/buildWidget, and
never-throwing parsing that degrades to documented fallbacks.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add MetricGaugeSpec with safe progress and null value
Fixes the validator/renderer contradiction where a String value was
accepted but cast to num, and the min == max NaN sweep angle bug.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add DynamicChartSpec for line, bar and pie
Adds the most-used and most complex A2UI component so far, covering
line/bar/pie rendering over the shared {labels, series} shape with
never-throwing prop parsing and label padding to prevent out-of-range
axis lookups.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add ScatterPlotSpec with point repair and safe bounds
Adds paired x/y observation plotting with an optional correlation badge,
following the Task 6-8 A2UiSpec pattern. Malformed points are dropped
rather than throwing, and bounds widen degenerate axes so fl_chart never
sees a zero-span range.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add RadarChartSpec sharing the labels/series shape
Task 10 of the a2ui/genui refactor: RadarChart consumes the same
{labels, series} shape as DynamicChart, with `axes` kept as a
backward-compatible alias for `labels`. Every series is truncated
or zero-padded to labels.length at parse time so fl_chart's radar
never sees a mismatched entry count.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add DataListGroupSpec with row repair and optional title
Adds a titled list-of-rows component with a defensive row-extraction
fallback chain: named fields, bare scalars, first-stringifiable-value
fallback, and silent drop of rows with nothing displayable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add FilterChipsSpec with nullable active option
Renders a decorative, non-interactive row of scope chips (e.g. "7d /
30d / 90d") and fixes the old renderer's `activeOption as String`
crash by matching case-insensitively and falling back to null instead
of throwing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): add GridContainerSpec, default registry and renderer
Task 13: assembles all eight leaf components into defaultA2UiRegistry,
adds the GridContainerSpec layout wrapper, the public A2UiRenderer
widget, and the lib/genui/a2ui.dart barrel file that will be the only
import path the rest of the app uses going forward.
fix(genui): make structural children lookup exact, not alias-resolved
Cross-task fix to a2ui_parser.dart (a Task 3 file), discovered during
Task 13 registry integration. A2UiParser._parseChildren and
_declaresChildren resolved the structural `children` key through
A2UiProps' alias-aware lookup(), which treats `items` as an alias for
`children`. That collided with DataListGroupSpec, whose own canonical
data-row key is also `items`: a DataListGroup node's `items` list of
{primaryText, ...} maps was mistaken for child components, none of
them parsed as one, and the whole node was then discarded as an
emptied-out container. Reading the literal `children` key only fixes
this and matches the precision _envelopeKeys already had (it does not
include `items` as a synonym for `children` either).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(genui): generate the A2UI prompt section from the registry
Replaces hand-written component-schema prose in the coach system
prompt with a section generated from defaultA2UiRegistry, so the
vocabulary advertised to the model can never drift from what the
parser/renderer actually support.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(genui): wire coach screen to the A2UI package, drop legacy renderer
Replaces private _CoachMessageContent with a public, stateful
CoachMessageContent that memoizes parsing per text value and shows a
"Building dashboard..." placeholder for partial JSON while streaming,
instead of letting raw braces scroll past or losing prose on a mixed
reply. Wraps the app root in A2UiThemeProvider(theme: repforgeA2UiTheme)
so the renderer picks up RepForge's design tokens. Deletes the
superseded lib/genui/a2ui_component.dart and lib/genui/a2ui_renderer.dart,
and drops test/new_features_test.dart's GenUI Component Resilience Tests
group, whose two cases are already covered more thoroughly by
test/genui/a2ui_parser_test.dart.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): bracket negative-value ranges in DynamicChart line/bar axes
minY was hardcoded to 0 while maxY derived from the true series max, so an
all-negative dataset (e.g. [-10, -5, -3]) produced a visible axis range of
[0, 1] with every real data point falling outside it — a silent blank
chart despite valid, non-empty data. Adds A2UiSeries.minValue mirroring
the existing maxValue, and a shared _yBounds helper used by both _line and
_bar so the two renderers can't diverge on axis math. Also covers
multi-series label padding, which was previously only exercised through
series[0].
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(genui): cover all-negative bounds and malformed point entries
Task 9 review flagged that ScatterPlotProps.bounds had no regression pin
for all-negative-coordinate spreads (same failure class as Task 8's
DynamicChartSpec axis bug) and that point-parsing had no test for
structurally invalid entries (nested objects, raw lists). Adds both.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): widen per-node children lookup back to components/elements/content
Follow-up to the Task 13 a2ui_parser.dart fix: restricting the per-node
_parseChildren/_declaresChildren lookup to the literal 'children' key
was narrower than intended. It regressed 'components'/'elements'/
'content' as per-node child-list keys, which never collided with
anything (only 'items' did, via DataListGroup's own canonical data key).
A payload like {"component":"GridContainer","props":{"columns":1,
"components":[...]}} resolved fine before the original bug and silently
rendered blank (zero children, no null fallback) after the first fix,
since _declaresChildren no longer recognized 'components' as a
children-declaring key either.
Adds a _childKeys constant (children/components/elements/content,
still excluding items) mirroring _envelopeKeys' existing tolerance, and
routes both _parseChildren and _declaresChildren through a shared
_firstChildList literal (non-alias) lookup over that key set.
Adds regression tests in a2ui_renderer_test.dart: per-node
components/elements/content resolve to real children, and items stays
excluded at the per-node level.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): widen looksLikeUi to catch prose-prefixed fences, fix vacuous memoization test
looksLikeUi only checked whether the text, after stripping a *leading*
fence, started with `{`/`[`. A model that writes a sentence before
opening a fenced payload (e.g. "Here is your data:\n```json\n{...")
fell through undetected, so CoachMessageContent showed the raw partial
JSON instead of the streaming placeholder -- the exact symptom this
task exists to fix. Now also treats an unclosed ``` fence found
anywhere in the streamed-so-far text as a UI signal, while plain prose
with no JSON or fence anywhere still returns false.
Also fixes the memoization regression test in
test/screens/ai_coach_genui_test.dart: the second observation was
taken after a bare `tester.pump()`, which doesn't mark the element
dirty and never actually calls build() again, so the test could not
distinguish memoized parsing from a widget that never rebuilds at all.
It now pumps a second CoachMessageContent instance with identical text
at the same tree location, which reuses the existing State and
genuinely triggers didUpdateWidget/build.
Adds regression tests for both the prose-prefixed-fence case and the
plain-prose-no-json case in test/genui/a2ui_parser_test.dart, plus a
widget-level test in test/screens/ai_coach_genui_test.dart confirming
the placeholder (not raw JSON) renders end-to-end.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(genui): drop presentation payload from tools, add purity and fuzz suites
The sleeping-HR analytics tool was hand-constructing an A2UI DynamicChart
payload directly, leaking presentation decisions into the data layer.
Replace `genui_chart_props` with neutral `labels`/`series` keys so the
prompt — not the tool — decides how to present the data.
Add two permanent guard suites: a2ui_purity_test.dart proves lib/genui/
never imports app-specific code (theme/models/services/screens) and its
component renderers never cast raw model data; a2ui_robustness_test.dart
fuzzes the parser and renderer against ~26 hostile/malformed LLM payloads
to confirm nothing throws.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): depth-agnostic purity regex, pin two silent-visual regressions
Review of the previous commit found the purity test's forbidden-import
check was depth-blind: its literal needle list only covered one and two
../ hops, but components live three levels below lib/, so a real
../../../theme/... import passed undetected. Replace it with a regex
that matches any number of ../ hops (or a package:repforge/ prefix),
covering import and export directives alike, and add a self-test that
proves the regex catches every relevant depth/form without touching real
source files.
Also widen the no-raw-casts check to include bool/Object/dynamic, make
the components-directory scan recursive, and pin down the two historical
silent-visual regressions (Task 8's chart axis-bounds clamp, Task 13's
GridContainer child-key aliasing) with positive assertions in the fuzz
suite, since neither throws and the existing no-throw checks structurally
can't catch either.
Reword analyze_health_workout_correlation's tool declaration to drop
direct component names, closing the same presentation-leak class this
task already fixed for the sleeping-HR tool.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): propagate registry through recursion, pin prompt drift, close review findings
Final whole-branch review fix wave for the A2UI genui refactor:
- A2UiRenderer's registry override used to be silently dropped past one
level of nesting because GridContainerSpec recurses via bare
A2UiRenderer(node: ...) calls. Mirror the existing theme-injection
pattern with a new A2UiRegistryProvider InheritedWidget so an explicit
registry override at any level propagates ambiently to everything below
it (explicit param > inherited provider > defaultA2UiRegistry fallback).
- Pin the hand-written "WHICH COMPONENT TO REACH FOR" prose in
gemini_context_builder.dart against silent drift: every component name
it mentions must resolve in defaultA2UiRegistry, and the registry's
spec count is asserted directly.
- Delete A2UiProps.object()/has() — confirmed zero call sites.
- Repurpose the orphaned Task 3 scaffolding test
(a2ui_parser_stub_test.dart, redundant with a2ui_parser_test.dart) into
a2ui_custom_registry_test.dart, the regression coverage the registry-
propagation fix needed.
- Add scanned-file-count floors to the purity test's two directory scans
so an empty/unreachable directory can't produce a vacuous pass.
- Document FilterChips' SizedBox.shrink() as a deliberate exception to
the plan's "always A2UiEmptyPanel" rule (decorative chrome, not data).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: add design spec for Hive->SQLite migration + coach SQL query tool
* fix: persist assisted-load volume correctly, tighten exercise-handle scoping
- WorkoutSet now snapshots bodyweight/assist/extra at logging time instead
of recomputing effective load from the CURRENT profile bodyweight on every
read, which was silently corrupting historical volume whenever a user
updated their weight. ExerciseLog.totalVolume and the workout_flow_screen
logging path thread the snapshot through.
- Exercise-handle matching (workout_provider) now requires an exact handle
match whenever a handle is set, falling back to legacy behavior only when
no exact match exists — a null-handle log was previously matching ANY
requested handle, surfacing the wrong variation's "last session" data.
- Handle selector no longer visually pre-selects an unpersisted handle, and
setExerciseHandle no longer retroactively relabels already-logged sets.
- Assisted-load display values now respect the user's unit preference; the
assisted-exercise classification is computed once and shared instead of
drifting between two separate predicates.
- Body-weight input (settings_provider) now rejects non-finite/non-positive
values on both the load and set paths, falling back to 70.0 when invalid.
- ml_service: deload-recovery reasoning no longer hardcodes "kg" regardless
of unit settings; recovery detection now requires the comparison session
to be recent and uses effective (not raw) load for assisted exercises.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: bound sleep-analytics window, drop fabricated data, resolve muscle groups by id
- get_sleeping_hr_analytics clamps the model-provided days window instead of
looping unbounded; get_health_metrics now honors the requested days window
instead of always querying one week, and both its and the correlation
tool's declarations no longer advertise fields (resting HR, readiness)
that aren't actually backed by implementation.
- analyze_health_workout_correlation no longer fabricates synthetic sleep
data points to pad out insufficient real pairs — returns the existing
insufficient-data error instead, so correlation/regression/chart output is
never partly made up.
- get_muscle_group_volume now resolves requested names to ids via
_resolveMuscleGroup and compares ids (also aggregating secondary muscle
activations) instead of raw display-name substring matching.
- CoachToolService's optional HealthHistoryManager is now a named parameter.
- gemini_ai_service: daily-quota classification narrowed to actual
daily-limit identifiers so minute-scale rate limits go through normal
retry-delay handling instead of being misclassified as daily exhaustion;
function-call ids are now preserved and matched into their responses;
the fallback path now builds a thinkingConfig compatible with whichever
model was actually selected. Mirrored in scripts/test_gemini_api.py.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(genui): pie negative-value filtering, overflow guard, stat-card unit match
- DynamicChart's pie mode now filters to positive values before computing
percentages/sections (preserving original index alignment with labels and
series colors), falling back to an empty panel when nothing positive
remains, instead of rendering a nonsense chart from negative/zero data.
- A2UiPanelTitle's trailing label is now Flexible with maxLines/ellipsis so
a long model-provided string can't overflow the row.
- StatCard's unit-already-present check now requires a trailing-suffix
match instead of any substring, fixing a false positive like unit "s"
matching inside value "10 reps".
- MetricGauge's arc painter now also compares `track` in shouldRepaint, so
a background-color-only change still triggers a repaint.
- A2UiTheme.seriesColor asserts a non-empty palette before the modulo index
that would otherwise throw on one.
- A2UiParser: props/outer-children now merge (props wins on conflict) so a
model writing children as a sibling of props isn't silently dropped; adds
a whole-text jsonDecode fast path ahead of the balanced-span scan.
- A2UiRenderer logs the unresolved component name via the app's existing
debugPrint/kDebugMode convention before falling back to an empty widget.
- a2ui_app_theme now imports A2UiTheme via the public genui barrel instead
of an internal src path.
- CI: the release workflow's linker-patch step now requires and quotes
PUB_CACHE, restricts the patch to resolved jni-*/src/CMakeLists.txt
targets, is idempotent against re-runs, and fails the build instead of
silently continuing when no target is found or patching fails.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test: close vacuous-test gaps and pin already-fixed regressions
Fixes tests that would pass identically whether the behavior they claim to
verify was correct or broken:
- stat_card_test's pump() helper now actually threads its props argument
into the rendered node (it previously always rendered empty props).
- new_features_test's assisted-pullups case now uses distinguishable
weight/assistWeight values, so the test fails if the wrong field is used.
Tightens two guardrail-class tests to actually detect what they claim to:
- a2ui_prompt_test's worked-example extraction is now bounded to the region
after the "WORKED EXAMPLE:" marker via balanced-brace matching, instead of
the last '}' anywhere in the whole prompt.
- a2ui_purity_test's forbidden-import regex now also guards lib/data/.
- a2ui_robustness_test's negative-axis assertion now requires minY to
actually bracket the dataset's true minimum, not just be below -10.
- a2ui_theme_test's panel-decoration finders are scoped to the panel under
test rather than the first Container anywhere in the tree.
Adds regression coverage pinning fixes already shipped in prior commits:
DynamicChart pie's negative-value filtering, StatCard's unit-suffix match,
and CoachToolService's days-window/insufficient-data/muscle-id fixes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: add implementation plan for Hive->SQLite migration + coach SQL tool
* chore: add sqflite dependencies for SQLite storage migration
* feat: add SqliteStorageService with schema and workout session CRUD
* fix: persist bodyWeightAtLog in SqliteStorageService sets table
* feat: implement routine and target CRUD in SqliteStorageService
* feat: implement muscle group and custom exercise CRUD in SqliteStorageService
* feat: implement settings, PR, training program, and conversation CRUD in SqliteStorageService
* feat: implement export/import in SqliteStorageService, completing IStorageService
* feat: add settings enumeration helper to StorageService for migration
* feat: add StorageMigrationService for one-time Hive-to-SQLite migration
* feat: resolve Hive-vs-SQLite storage backend in main() before runApp
* feat: add SqlQueryService for read-only SQL execution
* feat: wire run_sql_query tool into CoachToolService
* fix: fall back to fresh StorageService when app is constructed without going through main()
* fix: block run_sql_query from reading settings/sqlite_master (credential exposure)
SELECT * FROM settings or sqlite_master passed all existing run_sql_query
validation and would leak the migrated Gemini API key into model context
and persisted chat history. Add a second denylist of restricted table/
schema identifiers, checked the same way as the existing forbidden-keyword
list, plus a substring guard against SQLite's pragma_* table-valued
functions.
* fix: prevent trailing SQL comment from breaking LIMIT wrapper
A model-submitted query ending in a `--` line comment swallowed the
wrapper's closing paren when concatenated onto one line, producing an
avoidable syntax error. Put the closing `) LIMIT ?` on its own line.
Also finishes staging test/sql_query_service_test.dart, which now covers
both this fix (trailing-comment query succeeds) and the settings/
sqlite_master restricted-table rejections from the previous commit.
* docs: warn model against SELECT * across joins in run_sql_query
sqflite's row maps are keyed by column name, so a natural join query like
"SELECT * FROM sessions s JOIN exercise_logs l ON ..." silently drops
duplicate columns (e.g. id, notes) from one side with no error. Steer the
model's generated SQL toward explicit aliased columns instead.
* refactor: extract testable storage backend resolution logic; guard sqliteStorage.init()
- lib/main.dart: sqliteStorage.init() was outside the try/catch on the
path every existing user hits on first launch after this update —
disk-space/sandbox/SQLite-build failures propagated out of main()
before runApp(), so the app never booted even though the working Hive
storage right above it was fine. Now guarded with its own fallback to
Hive. Also documents why Hive.initFlutter() stays unconditional post-
cutover: ApiService reads/writes an installation id directly against
this settings box, independent of IStorageService.
- lib/services/storage_backend_resolver.dart (new): extracts the
Hive-vs-SQLite decision (migrate-or-fallback, flag write) out of
main.dart's untestable _resolveStorageBackend into a pure, directly
testable top-level function.
- test/storage_backend_resolver_test.dart (new): covers the two
real-world paths every user takes — already-migrated relaunch, and
fresh-install migration success. The forced-migration-failure case is
intentionally omitted; there's no way to make
StorageMigrationService.migrate() throw with SqliteStorageService's
current public API without adding production surface purely for
testability, and that path is exercised indirectly by
storage_migration_service_test.dart.
* docs: add design spec for syncing sleep/HR data into SQLite for coach SQL joins
Lets run_sql_query join workout data against sleep/HR history instead of
requiring separate live Health Connect tool calls per question.
* docs: add implementation plan for syncing sleep/HR data into SQLite
Five-task TDD plan: schema + upsert methods, HealthDataSyncService,
launch-time wiring, manual sync button, and the coach's schema description.
* feat: add health_samples/sleep_sessions tables + upsert methods to SqliteStorageService
- Add schema v2 with three new tables: health_samples, sleep_sessions, sleep_stage_intervals
- Add upsertHealthSamples() and upsertSleepSessions() methods for health data sync
- Add onUpgrade callback for v1->v2 schema migration
- Use temporary files for in-memory test databases to support read-only connections
- All tests passing (35/35)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: prevent run_sql_query from closing the app's shared database connection
openReadOnlyDatabase(path) with the default singleInstance:true returns the
app's existing shared connection when called against the same path as
SqliteStorageService's live database, so the coach's per-query
finally { db.close() } was tearing down the app's only connection after
the first query. Pass singleInstance:false to force a genuinely separate
connection.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: address Task 1 review findings
- Remove _isTestDatabase path-substring flag; production init() no longer
branches on test-fixture path content
- Remove the unconditional health-schema fallback loop that made onUpgrade
untested/redundant; onCreate and onUpgrade are now the only paths that
create the health tables
- Revert IF NOT EXISTS back to plain CREATE TABLE/CREATE INDEX, matching
the existing schema statement convention
- Use a const list spread (..._healthSchemaStatements) instead of a
duplicated inline copy in _schemaStatements
- :memory: overrides still resolve to temp files (needed for read-only
secondary connections in tests), but now via an explicit Finalizer-based
cleanup keyed on the constructor's _databasePathOverride parameter
rather than sniffing the resulting path string
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: replace Finalizer with deterministic tearDown cleanup
- Remove Finalizer mechanism and unused imports (dart:async)
- Remove _tempDatabasePath and _generatedTempPath fields
- Simplify init() to convert :memory: to temp files without tracking
- Add deterministic tearDown() in test to close database and delete temp files
- Verified: no temp file leaks, all 35 tests passing
Closes: finding #5 from previous review
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* feat: add HealthDataSyncService to pull sleep/HR data into SQLite
* feat: sync health data into SQLite once per app launch
Wires HealthDataSyncService into the composition root, guarded to
only exist post-SQLite-cutover (mirrors the CoachToolService sqlQuery
guard). Fired fire-and-forget from AppInitializer._initializeApp()
alongside readiness.refresh() so it never blocks app startup.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* feat: add manual 'Sync coach data now' action to Profile screen
Lets the user force a Health Connect -> coach SQLite sync on demand
from the Health Connect section, instead of waiting for the next
app launch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* feat: teach run_sql_query about the new health_samples/sleep_sessions tables
Extends the schema description in CoachToolService's run_sql_query
declaration with health_samples, sleep_sessions, and
sleep_stage_intervals so the coach LLM knows these tables exist and
can join against them. Adds a test asserting the description text
mentions the new tables (nothing else would catch a typo/omission
there), plus a regression test for the join shape the coach will run.
* fix: remove overly broad auto-close from init, add explicit close to upgrade test
- Remove auto-close block from init() that was closing database for any
explicit file path, breaking coach_tool_service_test and other callers
- Add explicit await upgraded.close() in upgrade test before file deletion
- Regression: coach_tool_service_test now passes again
- All related tests verified: sqlite_storage_service (35), coach_tool_service (11),
health_data_sync_service (6), sql_query_service (10)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: address final review findings for health sync + coach SQL tool
- Skip syncing a health stream entirely when its HealthReadType isn't
granted, and leave its watermark untouched — prevents watermarks
from silently advancing to `now` on first launch before the user
has opted into Health Connect, which was breaking the 90-day
backfill for essentially every user.
- Store health_samples/sleep_sessions timestamps as local time
(.toLocal() before .toIso8601String()) to match the local-naive
convention used by `sessions.date`, fixing day-bucketing joins for
non-UTC users.
- Wrap the already-migrated SQLite init() branch in main.dart with a
Hive fallback, mirroring the fresh-migration branch, so a partial
upgrade failure can't crash app startup.
- Add IF NOT EXISTS to the health-schema DDL so a retried onUpgrade
after a partial failure doesn't blow up on already-created tables.
- Add missing tearDown to health_data_sync_service_test.dart to stop
leaking temp db files, guard a profile_screen snackbar with mounted
for consistency, and reset _initialized on close() so a
close()+init() cycle actually reopens the connection.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: address migration/SQL-tool review findings from PR #66
Fixes CodeRabbit findings scoped to the hive->sqflite migration and
coach SQL tool work on this branch (genui and docs findings deferred
to their own branches):
- gemini_ai_service: rebuild generationConfig.thinkingConfig after a
daily-quota model fallback, so the retried request matches whichever
model it's about to hit instead of the previous model's shape
- health_data_sync_service: named constructor/_syncSamples params;
guard grantedReadTypes() so a Health Connect failure doesn't abort
the whole sync instead of degrading per-stream
- ml_service: recommendSets now falls back to the first non-empty
pastSessions entry when lastSession is empty, instead of returning
no recommendations
- sqlite_storage_service: guard close() against a never-initialized
db; filter getCustomExercises() by is_custom; order exercise_logs/
sets by rowid instead of the synthetic text id, which sorted "_10"
before "_2" and silently misordered sets/exercises past 9 per group
- workout_provider: removeLastSet preserves the exercise log's handle;
handle-fallback lookups only match legacy handle-less logs instead
of any handle
- test_gemini_api.py: clamp the parsed retry delay to match the Dart
implementation's bounds
- add coverage: 11+ set/exercise ordering, migration-failure fallback
path, training-program/growth-rate migration, and the id/type-only
storage-service call sites
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: address second round of CodeRabbit findings on PR #66
Fixes real findings from the fresh review CodeRabbit ran after…
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.
Summary by CodeRabbit