Upgrades bottom nav bar Upgrade Bottom Navigation Bar - #61
Conversation
…ation to F-Droid repository data
…hain - 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)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe pull request updates Android release tooling and CI workflows, replaces the home navigation bar with a floating implementation, changes workout completion navigation, and adds service, storage, model, and widget-flow tests. ChangesAndroid release and CI
Floating navigation
Application behavior and validation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 @.github/workflows/release.yml:
- Line 140: Update the release workflow step containing the Flutter APK build to
retain both Dart symbols from build/app/outputs/symbols and the R8 mapping file
at android/app/build/outputs/mapping/release/mapping.txt. Add a private artifact
upload with retention configured so these deobfuscation files are preserved
alongside the published APKs.
- Around line 127-130: Ensure release signing fails when any credential is
missing: in .github/workflows/release.yml lines 127-130, validate
KEYSTORE_BASE64, KEY_STORE_PASSWORD, KEY_ALIAS, and KEY_PASSWORD before
building; in workout-logger/android/app/build.gradle.kts lines 44-48, treat
blank environment values as absent before falling back to key.properties,
preventing incomplete credentials from selecting debug signing.
- Line 25: Pin every listed GitHub Action to a reviewed immutable commit SHA
instead of a mutable tag or branch. Update actions/checkout, actions/setup-java,
gradle/actions/setup-gradle, subosito/flutter-action, actions/upload-artifact,
softprops/action-gh-release, and codecov/codecov-action at all affected sites:
.github/workflows/release.yml lines 25-25, 31-31, 37-37, and 164-164, plus
.github/workflows/test.yml line 25-25.
In @.github/workflows/test.yml:
- Line 64: Remove the trailing blank line at the end of the workflow file so the
YAML lint check passes.
- Around line 37-40: Update the dependency installation step in the workflow to
run flutter pub get on every checkout by removing the CACHE-HIT condition from
the step using the flutter-action output. Preserve its working-directory and
command.
In `@workout-logger/lib/screens/widgets/floating_nav_bar.dart`:
- Around line 515-535: Update the chip decoration in the floating navigation bar
to use FloatingNavBarTheme.defaultChipBorderOpacity for the border alpha and
defaultChipShadowOpacity for the shadow alpha instead of the hardcoded 0.28 and
0.18 values. Preserve the existing colorP scaling and conditional rendering.
- Around line 584-602: Wrap the label Text in the showLabels branch of the
floating navigation bar’s chip content with Flexible, preserving its existing
opacity, styling, single-line, and clipping behavior so long labels remain
within the fixed-width Container.
🪄 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: 5cc2e9ba-cbed-47fe-9db7-2bf5de8b69a1
⛔ Files ignored due to path filters (1)
workout-logger/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/release.yml.github/workflows/test.ymlREADME.mdworkout-logger/.gitignoreworkout-logger/android/app/build.gradle.ktsworkout-logger/android/app/proguard-rules.proworkout-logger/android/gradle.propertiesworkout-logger/android/gradle/wrapper/gradle-wrapper.propertiesworkout-logger/android/key.properties.exampleworkout-logger/android/settings.gradle.ktsworkout-logger/fastlane/metadata/android/en-US/full_description.txtworkout-logger/lib/screens/home_screen.dartworkout-logger/lib/screens/widgets/floating_nav_bar.dartworkout-logger/lib/screens/widgets/rf_widgets.dartworkout-logger/pubspec.yamlworkout-logger/scripts/build_release.py
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| uses: actions/checkout@v7 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow pins =="
grep -nE 'uses: [^[:space:]]+@' .github/workflows/release.yml .github/workflows/test.yml 2>/dev/null || true
echo
echo "== tracked workflow files =="
git ls-files .github/workflows || trueRepository: Devasy/RepForge
Length of output: 907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
for path in [Path(".github/workflows/release.yml"), Path(".github/workflows/test.yml")]:
print(f"\n== {path} ==")
for i, line in enumerate(path.read_text().splitlines(), 1):
m = re.search(r'uses:\s*([^\s#]+)@([^\s#]+)', line)
if m:
ref = m.group(2)
status = "SHA-like" if re.fullmatch(r'[0-9a-fA-F]{40}', ref) else "ref-like"
print(f"{i}: {m.group(1)}@{m.group(2)} :: {status}")
PYRepository: Devasy/RepForge
Length of output: 601
Pin GitHub Actions to immutable commit SHAs. Mutable tags/branches let upstream changes alter release or test workflow execution. Pin the checked-in action references to reviewed commit SHAs, including actions/checkout, actions/setup-java, gradle/actions/setup-gradle, subosito/flutter-action, actions/upload-artifact, softprops/action-gh-release, and codecov/codecov-action.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 25-25: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
📍 Affects 2 files
.github/workflows/release.yml#L25-L25(this comment).github/workflows/release.yml#L31-L31.github/workflows/release.yml#L37-L37.github/workflows/release.yml#L164-L164.github/workflows/test.yml#L25-L25
🤖 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 @.github/workflows/release.yml at line 25, Pin every listed GitHub Action to
a reviewed immutable commit SHA instead of a mutable tag or branch. Update
actions/checkout, actions/setup-java, gradle/actions/setup-gradle,
subosito/flutter-action, actions/upload-artifact, softprops/action-gh-release,
and codecov/codecov-action at all affected sites: .github/workflows/release.yml
lines 25-25, 31-31, 37-37, and 164-164, plus .github/workflows/test.yml line
25-25.
Source: Linters/SAST tools
| if [ -z "${{ secrets.KEYSTORE_BASE64 }}" ]; then | ||
| echo "Error: KEYSTORE_BASE64 secret is not configured in repository secrets." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail releases when any signing credential is absent. The workflow validates only KEYSTORE_BASE64, while Gradle leaves release signing unset when any password or alias is blank and silently signs with the debug key.
.github/workflows/release.yml#L127-L130: validateKEYSTORE_BASE64,KEY_STORE_PASSWORD,KEY_ALIAS, andKEY_PASSWORDbefore building.workout-logger/android/app/build.gradle.kts#L44-L48: treat blank environment values as absent before falling back tokey.properties.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 127-127: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
📍 Affects 2 files
.github/workflows/release.yml#L127-L130(this comment)workout-logger/android/app/build.gradle.kts#L44-L48
🤖 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 @.github/workflows/release.yml around lines 127 - 130, Ensure release signing
fails when any credential is missing: in .github/workflows/release.yml lines
127-130, validate KEYSTORE_BASE64, KEY_STORE_PASSWORD, KEY_ALIAS, and
KEY_PASSWORD before building; in workout-logger/android/app/build.gradle.kts
lines 44-48, treat blank environment values as absent before falling back to
key.properties, preventing incomplete credentials from selecting debug signing.
| KEY_ALIAS: ${{ secrets.KEY_ALIAS }} | ||
| KEY_PASSWORD: ${{ secrets.KEY_PASSWORD }} | ||
| run: flutter build apk --release --split-per-abi | ||
| run: flutter build apk --release --split-per-abi --obfuscate --split-debug-info=build/app/outputs/symbols |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Retain Dart and R8 deobfuscation artifacts.
This runner is ephemeral and later steps only publish APKs, so production stack traces cannot be decoded. Upload build/app/outputs/symbols and android/app/build/outputs/mapping/release/mapping.txt as a private retained artifact.
🤖 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 @.github/workflows/release.yml at line 140, Update the release workflow step
containing the Flutter APK build to retain both Dart symbols from
build/app/outputs/symbols and the R8 mapping file at
android/app/build/outputs/mapping/release/mapping.txt. Add a private artifact
upload with retention configured so these deobfuscation files are preserved
alongside the published APKs.
| - name: Install dependencies | ||
| if: steps.flutter-action.outputs.PUB-CACHE-HIT != 'true' | ||
| if: steps.flutter-action.outputs.CACHE-HIT != 'true' | ||
| working-directory: ./workout-logger | ||
| run: flutter pub get |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow files =="
git ls-files .github/workflows || true
echo "== test workflow excerpt =="
if [ -f .github/workflows/test.yml ]; then
nl -ba .github/workflows/test.yml | sed -n '1,140p'
fi
echo "== flutter-action/cache references =="
rg -n "flutter-action|CACHE-HIT|flutter pub get|pub get|dart_tool/package_config|flutter analyze|flutter test" .github/workflows workout-logger 2>/dev/null || true
echo "== flutter action usage context in tracked files =="
for f in $(git ls-files .github/workflows); do
echo "--- $f"
nl -ba "$f" | sed -n '1,220p'
doneRepository: Devasy/RepForge
Length of output: 299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpts =="
for f in .github/workflows/release.yml .github/workflows/test.yml; do
if [ -f "$f" ]; then
echo "--- $f"
awk '{printf "%6d %s\n", NR, $0}' "$f" | sed -n '1,220p'
fi
done
echo "== search relevant references =="
rg -n "flutter-action|CACHE-HIT|flutter pub get|pub get|dart_tool/package_config|flutter analyze|flutter test|subosito" .github/workflows workout-logger 2>/dev/null || trueRepository: Devasy/RepForge
Length of output: 11942
🌐 Web query:
subosito flutter-action v2 cache true CACHE-HIT output generated files .dart_tool/package_config.json
💡 Result:
The subosito/flutter-action action (v2) with cache: true enabled does not automatically cache the.dart_tool/package_config.json file or your project's generated files [1][2][3]. When you enable cache: true, the action uses actions/cache internally to cache two specific areas [4]: 1. The Flutter SDK installation [1][4]. 2. The Dart/Flutter pub package cache (commonly located in ~/.pub-cache) [1][4][5]. The CACHE-HIT and PUB-CACHE-HIT outputs provided by the action reflect whether an exact match was found for these specific SDK and pub caches, respectively [2][6][3]. They do not indicate the presence or validity of your project's local build artifacts like.dart_tool/package_config.json [2][7]. If you need to persist generated files (such as those in.dart_tool,.g.dart files, etc.) across workflow runs, you must explicitly add a separate actions/cache step to your workflow configuration [8]. Example of how to manually cache these files: - name: Cache generated files uses: actions/cache@v4 with: path: |.dart_tool/package_config.json /.dart_tool key: build-artifacts-${{ hashFiles('/pubspec.lock') }} [8] The CACHE-HIT output from subosito/flutter-action is solely for the action's own internal caching logic [2][9]. You should not rely on it as a proxy for the state of your application's generated build artifacts [2][7].
Citations:
- 1: https://github.com/subosito/flutter-action/blob/main/action.yaml
- 2: https://github.com/subosito/flutter-action/blob/main/README.md
- 3: https://github.com/subosito/flutter-action
- 4: How to integrate with caching subosito/flutter-action#283
- 5: Add separate pub-cache boolean flag subosito/flutter-action#377
- 6: Add cache hit outputs subosito/flutter-action#364
- 7: feat: impl cache hit outputs subosito/flutter-action#346
- 8: https://gist.github.com/sezabass/73c178512bff7589b45b820bf1bd37b2
- 9: https://github.com/marketplace/actions/flutter-action
Run flutter pub get on every checkout.
A cache hit from subosito/flutter-action restores the Flutter package cache/SDK, not the checkout’s generated .dart_tool/package_config.json; skipping flutter pub get on cache hits can make analyze or test fail.
Proposed fix
- name: Install dependencies
- if: steps.flutter-action.outputs.CACHE-HIT != 'true'
working-directory: ./workout-logger
run: flutter pub get📝 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.
| - name: Install dependencies | |
| if: steps.flutter-action.outputs.PUB-CACHE-HIT != 'true' | |
| if: steps.flutter-action.outputs.CACHE-HIT != 'true' | |
| working-directory: ./workout-logger | |
| run: flutter pub get | |
| - name: Install dependencies | |
| working-directory: ./workout-logger | |
| run: flutter pub get |
🤖 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 @.github/workflows/test.yml around lines 37 - 40, Update the dependency
installation step in the workflow to run flutter pub get on every checkout by
removing the CACHE-HIT condition from the step using the flutter-action output.
Preserve its working-directory and command.
| with: | ||
| files: workout-logger/coverage/lcov.info | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the trailing blank line.
YAMLlint reports this as an error, so the workflow lint check will fail.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 64-64: too many blank lines (1 > 0)
(empty-lines)
🤖 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 @.github/workflows/test.yml at line 64, Remove the trailing blank line at the
end of the workflow file so the YAML lint check passes.
Source: Linters/SAST tools
| decoration: BoxDecoration( | ||
| color: Color.lerp(Colors.transparent, widget.chipBg, colorP), | ||
| borderRadius: BorderRadius.circular(9999), | ||
| border: colorP > 0.05 | ||
| ? Border.all( | ||
| color: (t.selectedChipBorderColor ?? widget.chipContent) | ||
| .withValues(alpha: 0.28 * colorP), | ||
| width: 1.0, | ||
| ) | ||
| : null, | ||
| boxShadow: colorP > 0.05 | ||
| ? [ | ||
| BoxShadow( | ||
| color: | ||
| (t.selectedChipShadowColor ?? widget.chipContent) | ||
| .withValues(alpha: 0.18 * colorP), | ||
| blurRadius: 14, | ||
| spreadRadius: -2, | ||
| ), | ||
| ] | ||
| : null, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Chip border/shadow opacities are hardcoded, ignoring the theme fields.
defaultChipBorderOpacity (0.25) and defaultChipShadowOpacity (0.15) are declared on FloatingNavBarTheme (Lines 209-210) but never read — the chip decoration uses literals 0.28 and 0.18 instead, so these knobs are dead. Wire them through for a consistent, configurable API.
♻️ Use theme-configured opacities
border: colorP > 0.05
? Border.all(
color: (t.selectedChipBorderColor ?? widget.chipContent)
- .withValues(alpha: 0.28 * colorP),
+ .withValues(alpha: t.defaultChipBorderOpacity * colorP),
width: 1.0,
)
: null,
boxShadow: colorP > 0.05
? [
BoxShadow(
color:
(t.selectedChipShadowColor ?? widget.chipContent)
- .withValues(alpha: 0.18 * colorP),
+ .withValues(alpha: t.defaultChipShadowOpacity * colorP),
blurRadius: 14,
spreadRadius: -2,
),
]
: null,📝 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.
| decoration: BoxDecoration( | |
| color: Color.lerp(Colors.transparent, widget.chipBg, colorP), | |
| borderRadius: BorderRadius.circular(9999), | |
| border: colorP > 0.05 | |
| ? Border.all( | |
| color: (t.selectedChipBorderColor ?? widget.chipContent) | |
| .withValues(alpha: 0.28 * colorP), | |
| width: 1.0, | |
| ) | |
| : null, | |
| boxShadow: colorP > 0.05 | |
| ? [ | |
| BoxShadow( | |
| color: | |
| (t.selectedChipShadowColor ?? widget.chipContent) | |
| .withValues(alpha: 0.18 * colorP), | |
| blurRadius: 14, | |
| spreadRadius: -2, | |
| ), | |
| ] | |
| : null, | |
| decoration: BoxDecoration( | |
| color: Color.lerp(Colors.transparent, widget.chipBg, colorP), | |
| borderRadius: BorderRadius.circular(9999), | |
| border: colorP > 0.05 | |
| ? Border.all( | |
| color: (t.selectedChipBorderColor ?? widget.chipContent) | |
| .withValues(alpha: t.defaultChipBorderOpacity * colorP), | |
| width: 1.0, | |
| ) | |
| : null, | |
| boxShadow: colorP > 0.05 | |
| ? [ | |
| BoxShadow( | |
| color: | |
| (t.selectedChipShadowColor ?? widget.chipContent) | |
| .withValues(alpha: t.defaultChipShadowOpacity * colorP), | |
| blurRadius: 14, | |
| spreadRadius: -2, | |
| ), | |
| ] | |
| : 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/lib/screens/widgets/floating_nav_bar.dart` around lines 515 -
535, Update the chip decoration in the floating navigation bar to use
FloatingNavBarTheme.defaultChipBorderOpacity for the border alpha and
defaultChipShadowOpacity for the shadow alpha instead of the hardcoded 0.28 and
0.18 values. Preserve the existing colorP scaling and conditional rendering.
| if (t.showLabels && extraW > 1.0) ...[ | ||
| Opacity( | ||
| opacity: labelOpacity, | ||
| child: Text( | ||
| widget.item.label, | ||
| style: (t.labelStyle ?? | ||
| const TextStyle( | ||
| fontSize: 13, | ||
| fontWeight: FontWeight.w600, | ||
| letterSpacing: 0.1, | ||
| )) | ||
| .copyWith(color: widget.chipContent), | ||
| maxLines: 1, | ||
| softWrap: false, | ||
| overflow: TextOverflow.clip, | ||
| ), | ||
| ), | ||
| SizedBox(width: rightPad), | ||
| ], |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unconstrained label can overflow the fixed-width chip.
The chip Container width is clamped using the constant labelWidth (Line 511-514), but the label Text has an intrinsic width driven by the actual string. For longer labels the Row (mainAxisSize.min) can exceed the container width and trip a RenderFlex overflow. Since this is a reusable widget, wrap the label in Flexible so it clips within the available space instead of overflowing.
🛡️ Constrain the label
- Opacity(
- opacity: labelOpacity,
- child: Text(
- widget.item.label,
- style: (t.labelStyle ??
- const TextStyle(
- fontSize: 13,
- fontWeight: FontWeight.w600,
- letterSpacing: 0.1,
- ))
- .copyWith(color: widget.chipContent),
- maxLines: 1,
- softWrap: false,
- overflow: TextOverflow.clip,
- ),
- ),
+ Flexible(
+ child: Opacity(
+ opacity: labelOpacity,
+ child: Text(
+ widget.item.label,
+ style: (t.labelStyle ??
+ const TextStyle(
+ fontSize: 13,
+ fontWeight: FontWeight.w600,
+ letterSpacing: 0.1,
+ ))
+ .copyWith(color: widget.chipContent),
+ maxLines: 1,
+ softWrap: false,
+ overflow: TextOverflow.clip,
+ ),
+ ),
+ ),📝 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.
| if (t.showLabels && extraW > 1.0) ...[ | |
| Opacity( | |
| opacity: labelOpacity, | |
| child: Text( | |
| widget.item.label, | |
| style: (t.labelStyle ?? | |
| const TextStyle( | |
| fontSize: 13, | |
| fontWeight: FontWeight.w600, | |
| letterSpacing: 0.1, | |
| )) | |
| .copyWith(color: widget.chipContent), | |
| maxLines: 1, | |
| softWrap: false, | |
| overflow: TextOverflow.clip, | |
| ), | |
| ), | |
| SizedBox(width: rightPad), | |
| ], | |
| Flexible( | |
| child: Opacity( | |
| opacity: labelOpacity, | |
| child: Text( | |
| widget.item.label, | |
| style: (t.labelStyle ?? | |
| const TextStyle( | |
| fontSize: 13, | |
| fontWeight: FontWeight.w600, | |
| letterSpacing: 0.1, | |
| )) | |
| .copyWith(color: widget.chipContent), | |
| maxLines: 1, | |
| softWrap: false, | |
| overflow: TextOverflow.clip, | |
| ), | |
| ), | |
| ), | |
| SizedBox(width: rightPad), |
🤖 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/screens/widgets/floating_nav_bar.dart` around lines 584 -
602, Wrap the label Text in the showLabels branch of the floating navigation
bar’s chip content with Flexible, preserving its existing opacity, styling,
single-line, and clipping behavior so long labels remain within the fixed-width
Container.
…ades-bottom-nav-bar
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/sleep_hr_builder_test.dart`:
- Around line 7-38: Extract the duplicated _StubHcService implementation from
workout-logger/test/sleep_hr_builder_test.dart lines 7-38 into
workout-logger/test/test_utils/stub_health_connect_service.dart, preserving its
constructor defaults and IHealthConnectService behavior. Update
workout-logger/test/sleep_hr_builder_test.dart lines 7-38 to import and use the
shared helper, and replace the duplicate definition in
workout-logger/test/userflow_history_and_session_details_test.dart lines 15-36
with the same shared helper.
In `@workout-logger/test/sleep_hr_models_test.dart`:
- Around line 24-32: Make the SleepStageStats construction assigned to stats
const, matching the existing const construction later in the test and the
literal-only arguments.
In `@workout-logger/test/userflow_history_and_session_details_test.dart`:
- Around line 124-151: The tests directly construct sub-widgets instead of
exercising their real userflow transitions. In
workout-logger/test/userflow_history_and_session_details_test.dart:124-151,
update the test around SessionDetailsSheet to pump HistoryScreen and tap the
relevant history list item; in
workout-logger/test/userflow_workout_logging_test.dart:79-129, drive
WorkoutFlowScreen through an actual rest-timer countdown and workout completion
so RestTimerView and WorkoutSummaryScreen appear through the production flow,
preserving the existing assertions.
In `@workout-logger/test/userflow_settings_and_storage_test.dart`:
- Around line 64-76: The test “Toggling weight unit in SettingsProvider persists
to storage and updates display label” only checks provider state, not rendered
UI. Update this test to render SettingsScreen with the test harness, toggle the
weight unit through the screen, and assert the updated label using the widget
finder pattern established by the first test in the file, while retaining the
existing persistence and provider assertions.
🪄 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: 3d79a405-f0a6-4cca-b623-5f24324ee083
📒 Files selected for processing (11)
workout-logger/test/api_service_test.dartworkout-logger/test/debug_log_buffer_test.dartworkout-logger/test/gemini_context_builder_test.dartworkout-logger/test/settings_provider_test.dartworkout-logger/test/sleep_hr_builder_test.dartworkout-logger/test/sleep_hr_models_test.dartworkout-logger/test/storage_service_test.dartworkout-logger/test/userflow_history_and_session_details_test.dartworkout-logger/test/userflow_routine_creation_test.dartworkout-logger/test/userflow_settings_and_storage_test.dartworkout-logger/test/userflow_workout_logging_test.dart
| testWidgets('Toggling weight unit in SettingsProvider persists to storage and updates display label', (tester) async { | ||
| expect(settingsProvider.weightUnit, equals(WeightUnit.kg)); | ||
| expect(settingsProvider.unitLabel, equals('kg')); | ||
|
|
||
| await settingsProvider.setWeightUnit(WeightUnit.lbs); | ||
| expect(settingsProvider.weightUnit, equals(WeightUnit.lbs)); | ||
| expect(settingsProvider.unitLabel, equals('lbs')); | ||
| expect(mockStorage.settings['weightUnit'], equals('lbs')); | ||
|
|
||
| await settingsProvider.setWeightIncrement(5.0); | ||
| expect(settingsProvider.weightIncrement, equals(5.0)); | ||
| expect(mockStorage.settings['weightIncrement'], equals('5.0')); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Test doesn't verify the "display label" update it claims to.
This test only exercises SettingsProvider directly (no pumpWidget call), so the described UI-label-update behavior is never actually checked against rendered output. Consider rendering SettingsScreen and asserting the updated unit label appears (e.g. via find.text), similar to the first test in this file.
🤖 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_settings_and_storage_test.dart` around lines 64
- 76, The test “Toggling weight unit in SettingsProvider persists to storage and
updates display label” only checks provider state, not rendered UI. Update this
test to render SettingsScreen with the test harness, toggle the weight unit
through the screen, and assert the updated label using the widget finder pattern
established by the first test in the file, while retaining the existing
persistence and provider assertions.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
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/sleep_hr_builder_test.dart (1)
80-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the calculated sleep values, not only non-emptiness.
This test is named as a calculation test but would pass with incorrect values. Assert representative segment and deep-stage statistics such as min/max BPM, stage, and sample count.
🤖 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/sleep_hr_builder_test.dart` around lines 80 - 82, Strengthen the calculation test assertions after the existing snapshot checks by verifying representative calculated values in snapshot.segments and snapshot.stageStats, including expected minimum and maximum BPM, sleep stage, and sample count. Use the fixture’s known expected values so the test fails when calculations are incorrect rather than merely empty.
🤖 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/sleep_hr_builder_test.dart`:
- Line 9: Update the granted HealthReadType set in the test to include
HealthReadType.restingHeartRate, then assert that the resulting
buildHrDaySnapshot value has restingBpm equal to 58. Apply the same fixture and
assertion adjustment to the related test ranges so the resting-HR branch is
exercised consistently.
In `@workout-logger/test/test_utils/stub_health_connect_service.dart`:
- Around line 16-20: Update the read methods in the health-connect stub,
including readSleepSessions, readHeartRateSamples, and readRestingHeartRate, to
return fresh list copies rather than the backing fixture lists. Preserve the
existing fixture contents while preventing callers such as buildHrDaySnapshot
from mutating shared test state.
- Around line 9-13: Update the StubHcService constructor to be const, preserving
its existing parameters and compile-time default values.
---
Outside diff comments:
In `@workout-logger/test/sleep_hr_builder_test.dart`:
- Around line 80-82: Strengthen the calculation test assertions after the
existing snapshot checks by verifying representative calculated values in
snapshot.segments and snapshot.stageStats, including expected minimum and
maximum BPM, sleep stage, and sample count. Use the fixture’s known expected
values so the test fails when calculations are incorrect rather than merely
empty.
🪄 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: 621e3f68-e3d6-4464-94da-9fa1b10efecc
📒 Files selected for processing (7)
workout-logger/lib/screens/workout_flow_screen.dartworkout-logger/test/sleep_hr_builder_test.dartworkout-logger/test/sleep_hr_models_test.dartworkout-logger/test/test_utils/stub_health_connect_service.dartworkout-logger/test/userflow_history_and_session_details_test.dartworkout-logger/test/userflow_settings_and_storage_test.dartworkout-logger/test/userflow_workout_logging_test.dart
| import 'test_utils/stub_health_connect_service.dart'; | ||
|
|
||
| void main() { | ||
| final granted = <HealthReadType>{HealthReadType.heartRate, HealthReadType.sleep}; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the resting-HR branch instead of supplying dead fixture data.
Line 9 omits HealthReadType.restingHeartRate, so buildHrDaySnapshot never reads the resting fixture and falls back to the ordinary HR samples. Add that permission and assert snapshot.restingBpm == 58 so this test verifies the behavior named in its description.
Proposed test adjustment
- final granted = <HealthReadType>{HealthReadType.heartRate, HealthReadType.sleep};
+ final granted = <HealthReadType>{
+ HealthReadType.heartRate,
+ HealthReadType.sleep,
+ HealthReadType.restingHeartRate,
+ };
...
expect(snapshot!.minBpm, equals(70));
expect(snapshot.maxBpm, equals(120));
+ expect(snapshot.restingBpm, equals(58));
expect(snapshot.buckets, isNotEmpty);Also applies to: 29-37, 41-44
🤖 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/sleep_hr_builder_test.dart` at line 9, Update the granted
HealthReadType set in the test to include HealthReadType.restingHeartRate, then
assert that the resulting buildHrDaySnapshot value has restingBpm equal to 58.
Apply the same fixture and assertion adjustment to the related test ranges so
the resting-HR branch is exercised consistently.
| StubHcService({ | ||
| this.sleepPeriods = const [], | ||
| this.hrSamples = const [], | ||
| this.restingHrSamples = const [], | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make the test stub constructor const.
All fields are final and the default values are compile-time constants, so this constructor can be declared const, as required by the Dart guidelines.
Proposed fix
- StubHcService({
+ const StubHcService({📝 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.
| StubHcService({ | |
| this.sleepPeriods = const [], | |
| this.hrSamples = const [], | |
| this.restingHrSamples = const [], | |
| }); | |
| const StubHcService({ | |
| this.sleepPeriods = const [], | |
| this.hrSamples = const [], | |
| this.restingHrSamples = const [], | |
| }); |
🤖 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/stub_health_connect_service.dart` around lines
9 - 13, Update the StubHcService constructor to be const, preserving its
existing parameters and compile-time default values.
Source: Coding guidelines
| Future<List<SleepPeriod>> readSleepSessions(DateTime start, DateTime end) async => sleepPeriods; | ||
| @override | ||
| Future<List<HealthSample>> readHeartRateSamples(DateTime start, DateTime end) async => hrSamples; | ||
| @override | ||
| Future<List<HealthSample>> readRestingHeartRate(DateTime start, DateTime end) async => restingHrSamples; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Return copies of fixture lists from the stub.
buildHrDaySnapshot sorts the list returned by readRestingHeartRate in place. Returning restingHrSamples directly mutates the caller’s fixture and can leak state between tests; return fresh lists for all read methods.
Proposed fix
- Future<List<SleepPeriod>> readSleepSessions(DateTime start, DateTime end) async => sleepPeriods;
+ Future<List<SleepPeriod>> readSleepSessions(DateTime start, DateTime end) async => List.of(sleepPeriods);
...
- Future<List<HealthSample>> readHeartRateSamples(DateTime start, DateTime end) async => hrSamples;
+ Future<List<HealthSample>> readHeartRateSamples(DateTime start, DateTime end) async => List.of(hrSamples);
...
- Future<List<HealthSample>> readRestingHeartRate(DateTime start, DateTime end) async => restingHrSamples;
+ Future<List<HealthSample>> readRestingHeartRate(DateTime start, DateTime end) async => List.of(restingHrSamples);📝 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.
| Future<List<SleepPeriod>> readSleepSessions(DateTime start, DateTime end) async => sleepPeriods; | |
| @override | |
| Future<List<HealthSample>> readHeartRateSamples(DateTime start, DateTime end) async => hrSamples; | |
| @override | |
| Future<List<HealthSample>> readRestingHeartRate(DateTime start, DateTime end) async => restingHrSamples; | |
| Future<List<SleepPeriod>> readSleepSessions(DateTime start, DateTime end) async => List.of(sleepPeriods); | |
| `@override` | |
| Future<List<HealthSample>> readHeartRateSamples(DateTime start, DateTime end) async => List.of(hrSamples); | |
| `@override` | |
| Future<List<HealthSample>> readRestingHeartRate(DateTime start, DateTime end) async => List.of(restingHrSamples); |
🤖 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/stub_health_connect_service.dart` around lines
16 - 20, Update the read methods in the health-connect stub, including
readSleepSessions, readHeartRateSamples, and readRestingHeartRate, to return
fresh list copies rather than the backing fixture lists. Preserve the existing
fixture contents while preventing callers such as buildHrDaySnapshot from
mutating shared test state.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Quality