iOS 26.5 fixes#171
Conversation
attempts to work around FB23536382 and FB23551259
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
📝 WalkthroughWalkthroughAdds root-anchored adaptive sheet routing, flushes pending demographics writes on dismissal, refactors prompted-actions presentation, and replaces boolean test-login flags with structured credentials across setup, app startup, and UI tests. ChangesAdaptive Sheet Routing and Presentation Updates
Demographics Dismissal and Prompted Actions
Launch Options and UI Test Credentials
Accessibility Tree Dump Utility
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MyHeartCounts/Modules/SetupTestEnvironment.swift (2)
154-168: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSignup fallback ignores the
credentialsparameter.
loginAndEnroll(_:)now takes acredentialsparameter and uses it for the login attempt (lines 159-160), but theinvalidCredentialssignup fallback still hardcodes"leland@stanford.edu"/"StanfordRocks!"instead ofcredentials.username/credentials.password. If this is ever called with non-default credentials, login fails, then signup silently creates the default account instead of the requested one — defeating the purpose of accepting structured credentials.🐛 Proposed fix
} catch FirebaseAccountError.invalidCredentials { // account doesn't exist yet, signup var details = AccountDetails() - details.userId = "leland@stanford.edu" - details.password = "StanfordRocks!" + details.userId = credentials.username + details.password = credentials.password details.name = PersonNameComponents(givenName: "Leland", familyName: "Stanford")🤖 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/Modules/SetupTestEnvironment.swift` around lines 154 - 168, The signup fallback in loginAndEnroll(_:) still hardcodes the default account values instead of using the provided credentials parameter. Update the FirebaseAccountError.invalidCredentials branch to populate AccountDetails from credentials.username and credentials.password so the same requested account is created after a failed login. Keep the existing login check and fallback structure, but ensure all account creation fields in SetupTestEnvironment.loginAndEnroll(_:) are driven by the passed-in credentials rather than fixed literals.
75-78: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFirebase-load wait shortened from 4s to 1s.
Reducing the fixed sleep after
Spezi.loadFirebaseby 75% risks proceeding tosetUp()before Firebase has finished initializing on slower CI runners, potentially causing intermittent failures in the login/enroll flow that immediately follows.🤖 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/Modules/SetupTestEnvironment.swift` around lines 75 - 78, The Firebase initialization pause in SetupTestEnvironment is too short and can let setUp() proceed before Spezi.loadFirebase has finished on slower runners. Update the wait in the Firebase-loading block to use a safer synchronization approach in place of the fixed 1-second sleep, or restore a longer delay if no readiness signal exists, so the subsequent login/enroll flow starts only after Firebase is ready.
🧹 Nitpick comments (1)
MyHeartCounts/Utils/SwiftUI/TabBarAccessibilityFix.swift (1)
52-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging when the retry budget is exhausted without success.
If
apply()never returnstruewithin the 40 attempts (10s), the task silently gives up and identifiers are never applied — since this workaround exists specifically to fix flaky UI-test tab lookups, a silent failure here could reintroduce the same flakiness this PR is meant to resolve, without any diagnostic trail.♻️ Proposed diagnostic on exhaustion
initialApplication = Task { `@MainActor` [weak self] in for _ in 0..<40 { // the item views usually exist within a few hundred ms guard let self, !Task.isCancelled else { return } if self.apply() { return } try? await Task.sleep(for: .milliseconds(250)) } + Logger.debugStuff.warning("TabBarAccessibilityFixer failed to resolve tab bar item views within retry budget") }🤖 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/Utils/SwiftUI/TabBarAccessibilityFix.swift` around lines 52 - 62, The retry loop in TabBarAccessibilityFix’s initialApplication task currently exits silently after exhausting all 40 attempts, so add a diagnostic when apply() never succeeds. Update the Task block that repeatedly calls apply() to detect the exhausted-retries case and log a clear failure message before returning, keeping the existing cancellation and success paths unchanged. Use the existing identifiers initialApplication and apply() to place the logging in the right spot.
🤖 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/Utils/Debug` Stuff/AXDump.swift:
- Around line 45-47: The AXDump classification logic in the button-traits branch
is too broad because .selected is causing non-switch controls to be labeled as
Switch. Update the conditional in AXDump.swift so only actual switch controls
identified by obj is UISwitch return "Switch", and keep all selected
buttons/tabs classified as "Button" in the traits.contains(.button) path.
- Around line 14-23: Gate the private AX runtime hook path in AXDump so it only
runs for debug/simulator builds; the current enableAXRuntime() call chain is
reachable from dumpAll() and can ship _AXSSetAutomationEnabled /
_AXSApplicationAccessibilitySetEnabled in Release. Add a DEBUG + simulator-only
compile guard around the AXDump helpers, or otherwise exclude AXDump from the
release app target, keeping the change localized to AXDump and dumpAll().
In `@MyHeartCountsShared/Sources/MyHeartCountsShared/Launch`
Options/SetupTestEnvironmentConfig.swift:
- Around line 104-109: The login-and-enroll launch argument in
SetupTestEnvironmentConfig is emitting plaintext credentials that later get
printed by MHCTestCase.launchAppAndEnrollIntoStudy, so adjust the
launch-argument construction to avoid exposing the password (and ideally the
username too) in any logged value. Update the handling in the loginAndEnroll
switch so the generated argument is sanitized or replaced with a non-sensitive
token, and keep the actual credentials only in memory for the code path that
needs them.
- Around line 104-109: The `loginAndEnroll` switch in
`SetupTestEnvironmentConfig` contains a redundant `let _ = ()` in the `.skip`
case that triggers SwiftLint. Remove the no-op binding and leave the `.skip`
branch empty, while keeping the `.enable(let credentials)` branch unchanged so
the switch remains valid and lint-clean.
- Around line 69-72: The validation in SetupTestEnvironmentConfig is checking
for the allowed reset and login-and-enroll tokens instead of invalid ones,
causing valid launch options to be rejected and ignored. Update the rawArgs
filtering logic in SetupTestEnvironmentConfig to detect non-allowed inputs by
comparing against allowedOptions, and keep the LaunchOptionDecodingError.other
path only for truly invalid values so _value(for:) can decode the intended
config.
---
Outside diff comments:
In `@MyHeartCounts/Modules/SetupTestEnvironment.swift`:
- Around line 154-168: The signup fallback in loginAndEnroll(_:) still hardcodes
the default account values instead of using the provided credentials parameter.
Update the FirebaseAccountError.invalidCredentials branch to populate
AccountDetails from credentials.username and credentials.password so the same
requested account is created after a failed login. Keep the existing login check
and fallback structure, but ensure all account creation fields in
SetupTestEnvironment.loginAndEnroll(_:) are driven by the passed-in credentials
rather than fixed literals.
- Around line 75-78: The Firebase initialization pause in SetupTestEnvironment
is too short and can let setUp() proceed before Spezi.loadFirebase has finished
on slower runners. Update the wait in the Firebase-loading block to use a safer
synchronization approach in place of the fixed 1-second sleep, or restore a
longer delay if no readiness signal exists, so the subsequent login/enroll flow
starts only after Firebase is ready.
---
Nitpick comments:
In `@MyHeartCounts/Utils/SwiftUI/TabBarAccessibilityFix.swift`:
- Around line 52-62: The retry loop in TabBarAccessibilityFix’s
initialApplication task currently exits silently after exhausting all 40
attempts, so add a diagnostic when apply() never succeeds. Update the Task block
that repeatedly calls apply() to detect the exhausted-retries case and log a
clear failure message before returning, keeping the existing cancellation and
success paths unchanged. Use the existing identifiers initialApplication and
apply() to place the logging in the right spot.
🪄 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: 6c22beca-b156-4742-9f88-4aa1458cdb32
📒 Files selected for processing (15)
MyHeartCounts/Modules/SetupTestEnvironment.swiftMyHeartCounts/MyHeartCounts.swiftMyHeartCounts/Root View/RootView.swiftMyHeartCounts/Utils/Debug Stuff/AXDump.swiftMyHeartCounts/Utils/SwiftUI/TabBarAccessibilityFix.swiftMyHeartCountsShared/Sources/MyHeartCountsShared/Launch Options/SetupTestEnvironmentConfig.swiftMyHeartCountsShared/Sources/MyHeartCountsShared/UITesting.swiftMyHeartCountsUITests/AOnboardingTests.swiftMyHeartCountsUITests/BasicAppUsage.swiftMyHeartCountsUITests/DemographicsNavigator.swiftMyHeartCountsUITests/MHCTestCase.swiftMyHeartCountsUITests/MyHeartCounts UI Tests.xctestplanMyHeartCountsUITests/OtherTests.swiftMyHeartCountsUITests/PromptedActionsTests.swiftMyHeartCountsUITests/TabBarAXStatsTests.swift
💤 Files with no reviewable changes (1)
- MyHeartCountsShared/Sources/MyHeartCountsShared/UITesting.swift
✅ Files skipped from review due to trivial changes (1)
- MyHeartCountsUITests/MyHeartCounts UI Tests.xctestplan
🚧 Files skipped from review as they are similar to previous changes (1)
- MyHeartCountsUITests/PromptedActionsTests.swift
| nonisolated static func enableAXRuntime() { | ||
| guard let lib = dlopen("/usr/lib/libAccessibility.dylib", RTLD_NOW) else { | ||
| return | ||
| } | ||
| typealias SetEnabled = @convention(c) (Bool) -> Void | ||
| for sym in ["_AXSSetAutomationEnabled", "_AXSApplicationAccessibilitySetEnabled"] { | ||
| if let ptr = dlsym(lib, sym) { | ||
| unsafeBitCast(ptr, to: SetEnabled.self)(true) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether AXDump.swift is compiled into release app targets.
fd -a 'project.pbxproj$' . | xargs -r sed -n '/AXDump.swift/,+6p'Repository: SchmiedmayerLab/MyHeartCounts-iOS
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and any project references to it.
git ls-files | rg -n '(^|/)AXDump\.swift$|project\.pbxproj$|\.xcodeproj/project\.pbxproj$'
# Show the file and surrounding context.
ax_file=$(git ls-files | rg '(^|/)AXDump\.swift$' | head -n1)
if [ -n "${ax_file:-}" ]; then
printf '\n### %s\n' "$ax_file"
cat -n "$ax_file" | sed -n '1,260p'
fi
# Show project references to the file and build-configuration clues.
for pbx in $(git ls-files | rg '(^|/)project\.pbxproj$'); do
printf '\n### %s references\n' "$pbx"
rg -n -C 3 'AXDump\.swift|DEBUG|targetEnvironment\(simulator\)|SWIFT_ACTIVE_COMPILATION_CONDITIONS|EXCLUDED_SOURCE_FILE_NAMES' "$pbx"
doneRepository: SchmiedmayerLab/MyHeartCounts-iOS
Length of output: 11151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pbx="MyHeartCounts.xcodeproj/project.pbxproj"
# Show the file reference, build-file entry, and the app target's sources phase.
rg -n -C 4 'AXDump\.swift|PBXSourcesBuildPhase|Sources \*/ = \{|Files = \(|MyHeartCounts \*/ = \{|Release|DEBUG' "$pbx"
# Narrow to the source build phase entries that include AXDump.swift.
rg -n -C 2 'AXDump\.swift' "$pbx"
# Print the surrounding section where AXDump.swift is listed in the sources phase.
line=$(rg -n 'AXDump\.swift' "$pbx" | head -n1 | cut -d: -f1)
if [ -n "${line:-}" ]; then
start=$((line-20)); [ "$start" -lt 1 ] && start=1
end=$((line+20))
sed -n "${start},${end}p" "$pbx"
fiRepository: SchmiedmayerLab/MyHeartCounts-iOS
Length of output: 10319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pbx="MyHeartCounts.xcodeproj/project.pbxproj"
# Inspect the file-system-synchronized sections and any exception sets tied to the MyHeartCounts root group.
rg -n -C 4 'PBXFileSystemSynchronized(BuildFileExceptionSet|RootGroup)|80F72B(D7|E3)|AXDump\.swift|exceptions = \(' "$pbx"
# Print the section around the MyHeartCounts root group and its exception set.
start=$(rg -n '80F72BA52D7E008E00150FE3 /\* MyHeartCounts \*/' "$pbx" | head -n1 | cut -d: -f1)
if [ -n "${start:-}" ]; then
sed -n "$((start-10)),$((start+40))p" "$pbx"
fi
# Print the exception set section near the referenced exception UUID from the root group.
ex=$(rg -n '80F72BD72D7E00A100150FE3 /\* PBXFileSystemSynchronizedBuildFileExceptionSet \*/' "$pbx" | head -n1 | cut -d: -f1 || true)
if [ -n "${ex:-}" ]; then
sed -n "$((ex-10)),$((ex+40))p" "$pbx"
fiRepository: SchmiedmayerLab/MyHeartCounts-iOS
Length of output: 7269
Gate these private AX runtime hooks to debug/simulator builds. MyHeartCounts/Utils/Debug Stuff/AXDump.swift is part of the app target, and dumpAll() always reaches enableAXRuntime(), so _AXSSetAutomationEnabled / _AXSApplicationAccessibilitySetEnabled can still ship in Release. Wrap this path in #if DEBUG && targetEnvironment(simulator) or exclude the file from the release target.
🤖 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/Utils/Debug` Stuff/AXDump.swift around lines 14 - 23, Gate the
private AX runtime hook path in AXDump so it only runs for debug/simulator
builds; the current enableAXRuntime() call chain is reachable from dumpAll() and
can ship _AXSSetAutomationEnabled / _AXSApplicationAccessibilitySetEnabled in
Release. Add a DEBUG + simulator-only compile guard around the AXDump helpers,
or otherwise exclude AXDump from the release app target, keeping the change
localized to AXDump and dumpAll().
There was a problem hiding this comment.
Probably actually a good idea 👍
There was a problem hiding this comment.
@PSchmiedmayer Glad that helps! Here's a concrete way to gate it — wrap the risky entry points in AXDump.swift with a compile-time guard so the AX automation hooks never make it into Release/device builds:
`#if` DEBUG && targetEnvironment(simulator)
`@MainActor`
enum AXDump {
nonisolated static func enableAXRuntime() {
...
}
...
}
`#endif`And guard the call site in dumpAll() (or wherever it's invoked) similarly, so callers don't fail to compile in Release:
`#if` DEBUG && targetEnvironment(simulator)
AXDump.dumpAll()
`#endif`Alternatively, if you'd rather keep it usable on-device for internal debug builds too, #if DEBUG alone (without the simulator check) would still strip it from Release/TestFlight/App Store builds, which is the main risk here.
Let me know once it's in and I'll take another look.
There was a problem hiding this comment.
@PSchmiedmayer no worries; that's the plan anyway. this is a debugging tool which either will be removed entirely, or will become a noop for non–debug builds
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if traits.contains(.button) { | ||
| return traits.contains(.selected) || obj is UISwitch ? "Switch" : "Button" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don’t classify selected buttons as switches.
.selected is not switch semantics; selected tabs/buttons can be reported as Switch here. Restrict Switch to actual switch controls and keep selected buttons as Button.
Proposed fix
let traits = obj.accessibilityTraits
if traits.contains(.button) {
- return traits.contains(.selected) || obj is UISwitch ? "Switch" : "Button"
+ return obj is UISwitch ? "Switch" : "Button"
}📝 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 traits.contains(.button) { | |
| return traits.contains(.selected) || obj is UISwitch ? "Switch" : "Button" | |
| } | |
| if traits.contains(.button) { | |
| return obj is UISwitch ? "Switch" : "Button" | |
| } |
🤖 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/Utils/Debug` Stuff/AXDump.swift around lines 45 - 47, The
AXDump classification logic in the button-traits branch is too broad because
.selected is causing non-switch controls to be labeled as Switch. Update the
conditional in AXDump.swift so only actual switch controls identified by obj is
UISwitch return "Switch", and keep all selected buttons/tabs classified as
"Button" in the traits.contains(.button) path.
| switch loginAndEnroll { | ||
| case .skip: | ||
| let _ = () | ||
| case .enable(let credentials): | ||
| "\(Self.loginAndEnrollOptionName):\(credentials.username);\(credentials.password)" | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Plaintext credentials embedded in launch args get printed to test logs.
login-and-enroll:<username>;<password> is emitted verbatim as a launch argument, and MHCTestCase.launchAppAndEnrollIntoStudy prints the full launchArguments list (including this value) to stdout/CI logs. For the hardcoded default test account this is low risk, but any custom Credentials passed to this API would leak in plaintext in CI logs.
🧰 Tools
🪛 GitHub Check: SwiftLint / Run SwiftLint
[failure] 106-106:
Redundant Discardable Let Violation: Prefer _ = foo() over let _ = foo() when discarding a result from a function (redundant_discardable_let)
🤖 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 `@MyHeartCountsShared/Sources/MyHeartCountsShared/Launch`
Options/SetupTestEnvironmentConfig.swift around lines 104 - 109, The
login-and-enroll launch argument in SetupTestEnvironmentConfig is emitting
plaintext credentials that later get printed by
MHCTestCase.launchAppAndEnrollIntoStudy, so adjust the launch-argument
construction to avoid exposing the password (and ideally the username too) in
any logged value. Update the handling in the loginAndEnroll switch so the
generated argument is sanitized or replaced with a non-sensitive token, and keep
the actual credentials only in memory for the code path that needs them.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
SwiftLint: redundant let _ = ().
Static analysis flags this as a failing check.
🔧 Proposed fix
switch loginAndEnroll {
case .skip:
- let _ = ()
+ _ = ()
case .enable(let credentials):
"\(Self.loginAndEnrollOptionName):\(credentials.username);\(credentials.password)"
}📝 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.
| switch loginAndEnroll { | |
| case .skip: | |
| let _ = () | |
| case .enable(let credentials): | |
| "\(Self.loginAndEnrollOptionName):\(credentials.username);\(credentials.password)" | |
| } | |
| switch loginAndEnroll { | |
| case .skip: | |
| _ = () | |
| case .enable(let credentials): | |
| "\(Self.loginAndEnrollOptionName):\(credentials.username);\(credentials.password)" | |
| } |
🧰 Tools
🪛 GitHub Check: SwiftLint / Run SwiftLint
[failure] 106-106:
Redundant Discardable Let Violation: Prefer _ = foo() over let _ = foo() when discarding a result from a function (redundant_discardable_let)
🤖 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 `@MyHeartCountsShared/Sources/MyHeartCountsShared/Launch`
Options/SetupTestEnvironmentConfig.swift around lines 104 - 109, The
`loginAndEnroll` switch in `SetupTestEnvironmentConfig` contains a redundant
`let _ = ()` in the `.skip` case that triggers SwiftLint. Remove the no-op
binding and leave the `.skip` branch empty, while keeping the `.enable(let
credentials)` branch unchanged so the switch remains valid and lint-clean.
Source: Linters/SAST tools
PSchmiedmayer
left a comment
PSchmiedmayer
left a comment
There was a problem hiding this comment.
Thank you for the improvements here, good to also see some more stability for the tests 👍
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
MyHeartCountsUITests/DemographicsNavigator.swift (1)
99-106: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo existence wait before interacting with
optionButton.
optionButton.isSelectedis read immediately after the query is built, without awaitForExistenceguard. If the sub-screen hasn't finished rendering,.isSelecteddefaults tofalseand triggers an unguarded.tap()on a possibly-missing element, risking flakiness. Other elements in this file gate interaction onwaitForExistencefirst (e.g. Lines 35-36).♻️ Suggested robustness fix
app.staticTexts["Referral Source"].tap() let optionButton = app.buttons.matching("label CONTAINS[c] %@", title).firstMatch + XCTAssert(optionButton.waitForExistence(timeout: 2)) if !optionButton.isSelected { optionButton.tap() }🤖 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/DemographicsNavigator.swift` around lines 99 - 106, The fillInReferralSourceOption helper in DemographicsNavigator is interacting with optionButton before confirming it exists. Add a waitForExistence guard on the queried button before checking isSelected or tapping it, and only proceed once the option is present, matching the existing wait-first pattern used elsewhere in this file.MyHeartCounts/Utils/AccountDetails+Diffing.swift (3)
23-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant nested
try.
try decoder.decode(JSONObject.self, from: try encoder.encode(other))— the innertryis unnecessary since the outertryalready propagates the error; only onetryis needed per throwing expression chain.♻️ Cleanup
- let jsonOld = try decoder.decode(JSONObject.self, from: try encoder.encode(other)) - let jsonNew = try decoder.decode(JSONObject.self, from: try encoder.encode(self)) + let jsonOld = try decoder.decode(JSONObject.self, from: encoder.encode(other)) + let jsonNew = try decoder.decode(JSONObject.self, from: encoder.encode(self))🤖 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/Utils/AccountDetails`+Diffing.swift around lines 23 - 24, Remove the redundant nested try in the AccountDetails diffing logic by simplifying the JSONObject decoding expressions in the AccountDetails+Diffing.swift implementation. Update the code around the jsonOld and jsonNew assignments so each throwing chain uses only the necessary try once, while keeping the same encoder and decoder behavior in the diffing method.
19-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent failure defeats the purpose of a debug helper.
The
catchblock swallows the error and returns"", so any encoding/decoding failure (e.g. anAccountKeyvalue that fails to encode) produces an empty, indistinguishable-from-"no diff" result. Since this method exists purely for debugging, at minimum include the error in the returned string.🐛 Proposed fix
} catch { - return "" + return "Failed to compute diff: \(error)" }🤖 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/Utils/AccountDetails`+Diffing.swift around lines 19 - 38, The `AccountDetails.diff(from:)` debug helper is swallowing encoding/decoding failures by returning an empty string from the `catch` block, which hides real errors and makes failures indistinguishable from “no diff.” Update the `catch` in `AccountDetails+Diffing` to return a string that includes the underlying error details instead of `""`, so failures in the JSONEncoder/JSONDecoder path are visible when diffing `AccountDetails` instances.
26-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTuple interpolation produces raw, unlabeled debug output.
entryforremoved/insertedis a(Key, Value)tuple andmutatedis(key:, old:, new:); default string interpolation of tuples yields output like("email", JSONValue.string("x")), which is usable but not very readable for a debug diff. Consider formatting explicitly (e.g."\(entry.0): \(entry.1)"and"\(mutated.key): \(mutated.old) -> \(mutated.new)").🤖 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/Utils/AccountDetails`+Diffing.swift around lines 26 - 34, The debug diff formatting in AccountDetails+Diffing should not rely on default tuple interpolation, since it prints unreadable raw tuple/debug text. Update the loop in the diff-building logic to format each `removed` and `inserted` `(Key, Value)` entry explicitly using its key and value, and format each `mutated` item from the diff with its `key`, `old`, and `new` fields in a readable transition form. Use the existing diff handling code around `removed`, `inserted`, and `mutated` to make the output clearer.
🤖 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 `@MyHeartCountsUITests/DemographicsNavigator.swift`:
- Around line 99-106: `fillInReferralSourceOption(_:)` in
`DemographicsNavigator` is missing the same post-selection assertion used by the
other helper flows in `fillInDemographics()`. After tapping the referral source
option and navigating back, verify that the expected summary label/value is
shown again before returning, using the same pattern as the existing helper
methods so a failed tap is caught immediately.
---
Nitpick comments:
In `@MyHeartCounts/Utils/AccountDetails`+Diffing.swift:
- Around line 23-24: Remove the redundant nested try in the AccountDetails
diffing logic by simplifying the JSONObject decoding expressions in the
AccountDetails+Diffing.swift implementation. Update the code around the jsonOld
and jsonNew assignments so each throwing chain uses only the necessary try once,
while keeping the same encoder and decoder behavior in the diffing method.
- Around line 19-38: The `AccountDetails.diff(from:)` debug helper is swallowing
encoding/decoding failures by returning an empty string from the `catch` block,
which hides real errors and makes failures indistinguishable from “no diff.”
Update the `catch` in `AccountDetails+Diffing` to return a string that includes
the underlying error details instead of `""`, so failures in the
JSONEncoder/JSONDecoder path are visible when diffing `AccountDetails`
instances.
- Around line 26-34: The debug diff formatting in AccountDetails+Diffing should
not rely on default tuple interpolation, since it prints unreadable raw
tuple/debug text. Update the loop in the diff-building logic to format each
`removed` and `inserted` `(Key, Value)` entry explicitly using its key and
value, and format each `mutated` item from the diff with its `key`, `old`, and
`new` fields in a readable transition form. Use the existing diff handling code
around `removed`, `inserted`, and `mutated` to make the output clearer.
In `@MyHeartCountsUITests/DemographicsNavigator.swift`:
- Around line 99-106: The fillInReferralSourceOption helper in
DemographicsNavigator is interacting with optionButton before confirming it
exists. Add a waitForExistence guard on the queried button before checking
isSelected or tapping it, and only proceed once the option is present, matching
the existing wait-first pattern used elsewhere in this file.
🪄 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: aaba5ae1-cff7-4c6c-8a83-ec45e26c8053
📒 Files selected for processing (6)
MyHeartCounts/Home Tab/Prompted Actions/MHCPromptedActions.swiftMyHeartCounts/Utils/AccountDetails+Diffing.swiftMyHeartCountsShared/Sources/MyHeartCountsShared/Launch Options/SetupTestEnvironmentConfig.swiftMyHeartCountsShared/Sources/MyHeartCountsShared/Utils/JSONValue.swiftMyHeartCountsUITests/DemographicsNavigator.swiftMyHeartCountsUITests/PromptedActionsTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- MyHeartCountsShared/Sources/MyHeartCountsShared/Launch Options/SetupTestEnvironmentConfig.swift
- MyHeartCountsUITests/PromptedActionsTests.swift
| func fillInReferralSourceOption(_ title: String) { | ||
| app.staticTexts["Referral Source"].tap() | ||
| let optionButton = app.buttons.matching("label CONTAINS[c] %@", title).firstMatch | ||
| if !optionButton.isSelected { | ||
| optionButton.tap() | ||
| } | ||
| app.navigationBars.buttons["BackButton"].tap() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing post-selection verification, unlike the rest of the navigator's helpers.
Every other selection flow in fillInDemographics() re-asserts the resulting summary label after returning from the sub-screen (e.g. Lines 63, 68, 74, 79, 85), but fillInReferralSourceOption does not verify that the option was actually selected before returning. If the tap silently fails to register, this test would not catch it.
✅ Proposed fix to verify selection after navigating back
func fillInReferralSourceOption(_ title: String) {
app.staticTexts["Referral Source"].tap()
let optionButton = app.buttons.matching("label CONTAINS[c] %@", title).firstMatch
if !optionButton.isSelected {
optionButton.tap()
}
app.navigationBars.buttons["BackButton"].tap()
+ XCTAssert(app.buttons["Referral Source, \(title)"].waitForExistence(timeout: 1))
}📝 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.
| func fillInReferralSourceOption(_ title: String) { | |
| app.staticTexts["Referral Source"].tap() | |
| let optionButton = app.buttons.matching("label CONTAINS[c] %@", title).firstMatch | |
| if !optionButton.isSelected { | |
| optionButton.tap() | |
| } | |
| app.navigationBars.buttons["BackButton"].tap() | |
| } | |
| func fillInReferralSourceOption(_ title: String) { | |
| app.staticTexts["Referral Source"].tap() | |
| let optionButton = app.buttons.matching("label CONTAINS[c] %@", title).firstMatch | |
| if !optionButton.isSelected { | |
| optionButton.tap() | |
| } | |
| app.navigationBars.buttons["BackButton"].tap() | |
| XCTAssert(app.buttons["Referral Source, \(title)"].waitForExistence(timeout: 1)) | |
| } |
🤖 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/DemographicsNavigator.swift` around lines 99 - 106,
`fillInReferralSourceOption(_:)` in `DemographicsNavigator` is missing the same
post-selection assertion used by the other helper flows in
`fillInDemographics()`. After tapping the referral source option and navigating
back, verify that the expected summary label/value is shown again before
returning, using the same pattern as the existing helper methods so a failed tap
is caught immediately.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
2 participants
iOS 26.5 fixes
♻️ Current situation & Problem
there are several SwiftUI issues/bugs in iOS 26.5, which cause certain sheets in MHC to get invalidated when they shouldn't (and consequently also dismissed when they shouldn't); this PR attempts to work around these issues.
⚙️ Release Notes
📚 Documentation
n/a
✅ Testing
the CI is currently is still using Xcode 26.5, i think, but i verified the tests locally using Xcode 26.6, and it is working fine there now, while it was consistently failing (due to the missing Tab accessibility identifiers) before these changes.
Code of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit