"Update Health Conditions" button; Vo2Max#179
Conversation
github-project-automation
Bot
moved this to In Progress
in MyHeart Counts Project Planning
github-project-automation
Bot
moved this to In Progress
in MyHeart Counts Project Planning
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds account controls for editing and persisting comorbidities. Timed walk and run results now use a shared view with a running-specific VO₂ max estimate and custom measurement unit. UI tests cover timed walk workflows, scheduled tasks, permission prompts, recovery after relaunch, and comorbidity persistence. Spezi is pinned to version 0.1.6, while accessibility, localization, onboarding setup, and consent test timing are updated. Sequence Diagram(s)sequenceDiagram
participant AccountSheet
participant UpdateComorbiditiesButton
participant DemographicsData
participant ComorbiditiesPicker
AccountSheet->>UpdateComorbiditiesButton: display update control
UpdateComorbiditiesButton->>DemographicsData: populateIfEmpty(from: account)
UpdateComorbiditiesButton->>ComorbiditiesPicker: present picker
ComorbiditiesPicker->>DemographicsData: update comorbidities
UpdateComorbiditiesButton->>DemographicsData: flush changes
sequenceDiagram
participant TimedWalkTestTests
participant MyHeartCountsApp
participant MotionFitnessAlert
participant TimedWalkingTestView
TimedWalkTestTests->>MyHeartCountsApp: launch and start test
MyHeartCountsApp->>MotionFitnessAlert: request permission
TimedWalkTestTests->>MotionFitnessAlert: allow access
TimedWalkTestTests->>TimedWalkingTestView: complete test
TimedWalkingTestView->>TimedWalkTestTests: return result fields
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
MyHeartCountsUITests/TimedWalkTestTests.swift (1)
84-84: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUse substitution variables to construct the
NSPredicate.Using dynamic string interpolation inside
NSPredicateformats is considered bad practice because it can lead to predicate injection, and it triggers static analysis warnings. It is safer to use substitution variables (%@) instead.♻️ Proposed refactor
- let testCellPred = NSPredicate(format: "label LIKE '\(kind.rawValue), *'") + let testCellPred = NSPredicate(format: "label LIKE %@", "\(kind.rawValue), *")🤖 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 `@MyHeartCountsUITests/TimedWalkTestTests.swift` at line 84, Update the testCellPred NSPredicate construction to use a %@ substitution placeholder and pass kind.rawValue as the predicate argument, preserving the existing label matching behavior without interpolating dynamic values into the format string.Source: Linters/SAST tools
MyHeartCounts/Heart Health Dashboard/PastTimedWalkTestResultInfo.swift (1)
25-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
EmptyView()for idiomatic SwiftUI.Instead of declaring an empty tuple and disabling SwiftLint, you can return an
EmptyView()in the.walkingcase. This is the idiomatic way to handle intentional "no-op" cases inside a@ViewBuilder.♻️ Proposed refactor
switch result.test.kind { case .walking: - let _ = () // swiftlint:disable:this redundant_discardable_let + EmptyView() case .running:🤖 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 `@MyHeartCounts/Heart` Health Dashboard/PastTimedWalkTestResultInfo.swift around lines 25 - 28, In the switch over result.test.kind, replace the discarded empty tuple in the .walking case with an EmptyView() view. Remove the redundant SwiftLint suppression while preserving the existing .running behavior and `@ViewBuilder` output.
🤖 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 `@MyHeartCounts/Account/UpdateComorbiditiesButton.swift`:
- Around line 50-61: Update closeSheetButton to remove the iOS availability
check and invalid ButtonRole.confirm usage. Replace both branches with a single
standard SwiftUI Button that closes the sheet, preserving the existing
confirmationAction toolbar placement and “Done” label behavior.
---
Nitpick comments:
In `@MyHeartCounts/Heart` Health Dashboard/PastTimedWalkTestResultInfo.swift:
- Around line 25-28: In the switch over result.test.kind, replace the discarded
empty tuple in the .walking case with an EmptyView() view. Remove the redundant
SwiftLint suppression while preserving the existing .running behavior and
`@ViewBuilder` output.
In `@MyHeartCountsUITests/TimedWalkTestTests.swift`:
- Line 84: Update the testCellPred NSPredicate construction to use a %@
substitution placeholder and pass kind.rawValue as the predicate argument,
preserving the existing label matching behavior without interpolating dynamic
values into the format string.
🪄 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: CHILL
Plan: Pro Plus
Run ID: b8742753-f686-4b6e-9028-f4ebbb49feb0
📒 Files selected for processing (17)
MyHeartCounts.xcodeproj/project.pbxprojMyHeartCounts/Account/AccountSheet.swiftMyHeartCounts/Account/Demographics/DemographicsData.swiftMyHeartCounts/Account/UpdateComorbiditiesButton.swiftMyHeartCounts/Heart Health Dashboard/PastTimedWalkTestResultInfo.swiftMyHeartCounts/Heart Health Dashboard/PastTimedWalkTestResults.swiftMyHeartCounts/Modules/SetupTestEnvironment.swiftMyHeartCounts/Resources/Localizable.xcstringsMyHeartCounts/Task Handling/Active Tasks/Timed Walk Test/TimedWalkingTestView.swiftMyHeartCounts/Utils/DisclosureIndicator.swiftMyHeartCounts/Utils/UnitOxygenConsumption.swiftMyHeartCountsShared/Package.swiftMyHeartCountsUITests/MHCTestCase.swiftMyHeartCountsUITests/OnboardingNavigator.swiftMyHeartCountsUITests/OtherTests.swiftMyHeartCountsUITests/TaskHandlingTests.swiftMyHeartCountsUITests/TimedWalkTestTests.swift
💤 Files with no reviewable changes (2)
- MyHeartCountsUITests/OnboardingNavigator.swift
- MyHeartCountsUITests/TaskHandlingTests.swift
| @ViewBuilder private var closeSheetButton: some View { | ||
| // look into using an AsyncButton here | ||
| if #available(iOS 26, *) { | ||
| Button(role: .confirm) { | ||
| closeSheet() | ||
| } | ||
| } else { | ||
| Button("Done") { | ||
| closeSheet() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the impossible version check and invalid button role.
The #available(iOS 26, *) check contains a typo and will evaluate to false for the next decade. Additionally, standard SwiftUI ButtonRole does not have a .confirm member, so this code will likely fail to compile when that hypothetical version is reached.
Because the parent ToolbarItem(placement: .confirmationAction) (Line 42) already applies the correct semantic styling (such as bolding the text) for a confirmation button, you can safely remove the version check entirely and use a standard button.
♻️ Proposed fix
- `@ViewBuilder` private var closeSheetButton: some View {
- // look into using an AsyncButton here
- if `#available`(iOS 26, *) {
- Button(role: .confirm) {
- closeSheet()
- }
- } else {
- Button("Done") {
- closeSheet()
- }
- }
- }
+ private var closeSheetButton: some View {
+ // look into using an AsyncButton here
+ Button("Done") {
+ closeSheet()
+ }
+ }📝 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.
| @ViewBuilder private var closeSheetButton: some View { | |
| // look into using an AsyncButton here | |
| if #available(iOS 26, *) { | |
| Button(role: .confirm) { | |
| closeSheet() | |
| } | |
| } else { | |
| Button("Done") { | |
| closeSheet() | |
| } | |
| } | |
| } | |
| private var closeSheetButton: some View { | |
| // look into using an AsyncButton here | |
| Button("Done") { | |
| closeSheet() | |
| } | |
| } |
🤖 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 `@MyHeartCounts/Account/UpdateComorbiditiesButton.swift` around lines 50 - 61,
Update closeSheetButton to remove the iOS availability check and invalid
ButtonRole.confirm usage. Replace both branches with a single standard SwiftUI
Button that closes the sheet, preserving the existing confirmationAction toolbar
placement and “Done” label behavior.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
PSchmiedmayer
left a comment
PSchmiedmayer
left a comment
There was a problem hiding this comment.
Thank you @lukaskollmer 🚀
3 participants
♻️ Current situation & Problem
⚙️ Release Notes
📚 Documentation
n/a
✅ Testing
yes
Code of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: