Improve Battery Usage and Memory Pressure - #181
Conversation
github-project-automation
Bot
moved this to In Progress
in MyHeart Counts Project Planning
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request adds a four-way iOS UI-test pipeline with manifest validation, cached test products, parallel shard execution, and result aggregation. It also redesigns health upload persistence and delivery around typed GRDB records, chunked draining, durable file staging, account-generation cleanup gates, adaptive scheduling, and cancellation-aware uploads. Background task handling, SensorKit processing, HealthKit staging, localization, dependency references, submodules, and repository metadata are updated accordingly. Sequence Diagram(s)sequenceDiagram
participant Account
participant HealthUploadStaging
participant ManagedFileUpload
participant FirebaseStorage
Account->>HealthUploadStaging: add health samples
HealthUploadStaging->>ManagedFileUpload: stage compressed health payload
ManagedFileUpload->>FirebaseStorage: upload captured-account file
FirebaseStorage-->>ManagedFileUpload: report completion or failure
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@lukaskollmer Let me know what you think about the changes. Also cleaned up and improved some of the CI to avoid long re-runs; should especially pay off once we have more macOS runners. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MyHeartCounts/SensorKit/SensorKitDataFetcher.swift (1)
126-141: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDedup check happens after an
await, so two callers can still start competing fetches.
fetchAndUploadNewDatasuspends atawait standard.shouldCollectHealthData(Line 130) before inspectingactiveFetch. Two callers (e.g. the launch fallback and the background task) can both suspend there, both observeactiveFetch == nil, and both create a task; the second assignment overwrites the first, so the first task is never awaited or cancellable viacancelAllActiveCollection()and both fetch/upload runs proceed concurrently — exactly what the comment says must not happen.Hoisting the dedup check above the suspension point (or having callers await a memoized task) closes the window.
🔒 Sketch of the reordering
`@MainActor` private func fetchAndUploadNewData() async { guard SensorKit.isAvailable else { return } + if let activeFetch { + _ = await activeFetch.result + return + } guard await standard.shouldCollectHealthData else { return } - if let activeFetch { - // if we're already performing this task, we simply wait on that task's result, ... - _ = await activeFetch.result - if self.activeFetch == activeFetch { - self.activeFetch = nil - } - return - }🤖 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/SensorKit/SensorKitDataFetcher.swift` around lines 126 - 141, Move the activeFetch deduplication check in fetchAndUploadNewData above the await standard.shouldCollectHealthData suspension point, while preserving the existing wait, identity check, and return behavior. Ensure concurrent callers cannot both pass an activeFetch == nil check and create competing fetch tasks.
🧹 Nitpick comments (7)
MyHeartCountsTests/HealthUploadStagingMigrationTests.swift (2)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCleanup misses the WAL sidecar files.
Now that the on-disk configuration enables WAL, opening this database also creates
-waland-shmfiles next todbUrl; thedeferonly removes the main file, so each run leaks two temp files.🧹 Suggested cleanup
defer { try? FileManager.default.removeItem(at: dbUrl) + try? FileManager.default.removeItem(at: URL(filePath: dbUrl.path() + "-wal")) + try? FileManager.default.removeItem(at: URL(filePath: dbUrl.path() + "-shm")) }🤖 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 `@MyHeartCountsTests/HealthUploadStagingMigrationTests.swift` around lines 31 - 34, Update the deferred cleanup around dbUrl in the staging migration tests to remove the SQLite WAL and shared-memory sidecar files alongside the main database file, using the same temporary-directory cleanup flow and preserving best-effort removal.
43-104: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider covering the unhappy migration path too.
The test only exercises timestamps written the way GRDB's
Datebinding writes them. A row whosetimestampSQLite can't parse currently fails the whole v2 migration (see the note onHealthUploadStaging+Schema.swift); a case asserting the migration still completes would lock in whatever behaviour you decide there.🤖 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 `@MyHeartCountsTests/HealthUploadStagingMigrationTests.swift` around lines 43 - 104, Extend the migration test around HealthUploadStaging.applyMigrations and HealthUploadStaging initialization to insert at least one v1 row with an SQLite-unparseable timestamp, then assert the v2 migration completes successfully and preserves the expected migration behavior for that row. Cover the relevant pendingSamples or pendingDeletions path without weakening the existing valid-timestamp assertions.MyHeartCounts/Health Import/HistoricalHealthSamplesExportManager.swift (1)
95-99: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTimestamp records session start, so an aborted export still suppresses the next resume for a day.
lastHistoricalExportSessionStartis written as soon assession.startreturns. If the app is terminated shortly after (common for a memory-heavy bulk export), the next launch sees a fresh timestamp and defers the resume for up toTimeConstants.dayunless the device happens to be charging. Consider only persisting once the session reports meaningful progress/completion, or using a shorter staleness for this path.🤖 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/Health` Import/HistoricalHealthSamplesExportManager.swift around lines 95 - 99, Change the historical export timestamp handling around managedFileUpload.scheduleForUpload and .lastHistoricalExportSessionStart so an export start is not recorded immediately after session.start. Persist the timestamp only after the session reports meaningful progress or completion, or apply the established shorter staleness policy for interrupted exports, while preserving the existing upload scheduling behavior.MyHeartCounts/Modules/HealthUploadStagingUploader.swift (1)
134-141: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLow Power Mode guard sits after the chunk fetch in both drain helpers. Each helper evaluates
fetchNextDrainChunk— a full bounded read of up todrainChunkSizerows — before checkingisLowPowerModeEnabled, so entering a drain in Low Power Mode still costs one wasted chunk read per helper.
MyHeartCounts/Modules/HealthUploadStagingUploader.swift#L134-L141: hoist theisLowPowerModeEnabledcheck above thewhile let chunk = …loop indrainPendingSamples.MyHeartCounts/Modules/HealthUploadStagingUploader.swift#L165-L172: do the same indrainPendingDeletions, and have_processskip this helper entirely whendidDrainSamplesis alreadyfalse.🤖 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/HealthUploadStagingUploader.swift` around lines 134 - 141, Move the low-power-mode guard in both drain helpers before their respective fetch loops: update drainPendingSamples and drainPendingDeletions so they return before calling fetchNextDrainChunk when low-power mode is enabled. In _process, skip drainPendingDeletions entirely when didDrainSamples is false.MyHeartCounts/Modules/HealthUploadStaging.swift (1)
57-65: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrefer GRDB’s built-in
journalModefor WAL.
Configuration.journalMode = .walis the intended way to makeDatabaseQueueuse WAL without manually executingPRAGMA journal_mode = WAL, and it leaves the concurrent-reader benefit toDatabasePoolwhere it actually comes from.🤖 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/HealthUploadStaging.swift` around lines 57 - 65, Update the DatabaseQueue configuration in the visible initialization block to set GRDB’s built-in journalMode property to .wal, and remove the prepareDatabase closure that manually executes PRAGMA journal_mode = WAL. Preserve the existing DatabaseQueue path and other configuration behavior..github/workflows/build-and-test.yml (2)
52-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCache-key formula is duplicated verbatim across two jobs.
Both jobs independently compute
cache-keywith the same shell expression. Sincemhc_app_ui_test_shardsrestores withfail-on-cache-miss: true, any future edit to one copy without the other would break all 4 shard jobs with a hard cache-miss failure. Consider extracting this into a small composite action or a single reusable step to keep the two computations in sync.Also applies to: 120-125
🤖 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/build-and-test.yml around lines 52 - 57, Deduplicate the cache-key computation used by the two jobs, including the Resolve test products cache key step and its counterpart near the later job. Extract the shared Xcode version and cache-key generation into a reusable composite action or shared workflow step, then have both jobs consume its output while preserving the existing key format and cache behavior.
148-152: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFirebase Functions are rebuilt independently in all 4 UI test shards.
npm ci+npm run buildforMyHeartCounts-Firebase/functionsruns once per matrix shard (4x total on self-hosted macOS runners), even though the output is identical across shards. This duplicated compile work runs counter to the PR's goal of reducing CI battery/CPU usage. Consider building/caching the Functions output once (e.g. inmhc_app_ui_test_products, keyed on a hash offunctions/package-lock.json+ source) and restoring it in each shard instead of rebuilding from scratch 4 times.🤖 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/build-and-test.yml around lines 148 - 152, Update the UI test workflow around the “Prepare Firebase functions” step so Firebase Functions dependencies and build output are produced once and reused by all four matrix shards. Move or add a cache/artifact mechanism keyed by the Functions package lockfile and source inputs, restore the prepared output in each shard, and avoid rerunning npm ci and npm run build per shard.
🤖 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/build-and-test.yml:
- Around line 46-48: Update both checkout steps in the mhc_app_ui_test_products
and mhc_app_ui_test_shards jobs to set persist-credentials to false alongside
submodules: recursive. Leave the existing checkout action and other options
unchanged.
In `@MyHeartCounts-Firebase`:
- Line 1: Update the MyHeartCounts-Firebase submodule pointer to a fetchable
intended commit, or restore the previously referenced repository if that commit
was removed; verify the submodule initializes and updates successfully.
In `@MyHeartCounts/Modules/HealthUploadStaging`+Schema.swift:
- Around line 169-179: Update the migration INSERT around the v1-to-v2
conversion to handle timestamps that SQLite cannot parse, rather than inserting
NULL into the strict table’s non-null timestamp column. Use the existing
migration symbols to default invalid timestamps to a valid value or exclude
those rows, while preserving normal strftime conversion for parseable timestamps
so the migration completes without leaving the database unavailable.
In `@MyHeartCounts/Modules/HealthUploadStagingUploader.swift`:
- Around line 36-48: Update drainChunkSize so its lower bound does not force a
fixed 1000-record chunk under memory pressure. Replace the hardcoded max(1000,
…) floor with a much smaller or availableMemory-derived minimum while preserving
the 10,000-record upper cap and the quarter-of-available-memory sizing behavior.
In `@MyHeartCounts/Modules/ManagedFileUpload.swift`:
- Around line 181-188: Update upload in ManagedFileUpload to delegate staging to
the existing stage method instead of directly constructing the staging URL and
calling moveItem. Preserve the subsequent uploadAndDelete flow by using the URL
returned by stage, ensuring the staging directory guard is consistently applied.
- Around line 168-174: The stage method’s FileManager.moveItem call must handle
an existing destination instead of allowing the upload to be lost. Before moving
in stage(_:category:), detect or catch a destination conflict at stagingUrl and
remove the existing file or generate a unique destination, while preserving
normal move errors and ensuring the current upload is staged successfully.
---
Outside diff comments:
In `@MyHeartCounts/SensorKit/SensorKitDataFetcher.swift`:
- Around line 126-141: Move the activeFetch deduplication check in
fetchAndUploadNewData above the await standard.shouldCollectHealthData
suspension point, while preserving the existing wait, identity check, and return
behavior. Ensure concurrent callers cannot both pass an activeFetch == nil check
and create competing fetch tasks.
---
Nitpick comments:
In @.github/workflows/build-and-test.yml:
- Around line 52-57: Deduplicate the cache-key computation used by the two jobs,
including the Resolve test products cache key step and its counterpart near the
later job. Extract the shared Xcode version and cache-key generation into a
reusable composite action or shared workflow step, then have both jobs consume
its output while preserving the existing key format and cache behavior.
- Around line 148-152: Update the UI test workflow around the “Prepare Firebase
functions” step so Firebase Functions dependencies and build output are produced
once and reused by all four matrix shards. Move or add a cache/artifact
mechanism keyed by the Functions package lockfile and source inputs, restore the
prepared output in each shard, and avoid rerunning npm ci and npm run build per
shard.
In `@MyHeartCounts/Health` Import/HistoricalHealthSamplesExportManager.swift:
- Around line 95-99: Change the historical export timestamp handling around
managedFileUpload.scheduleForUpload and .lastHistoricalExportSessionStart so an
export start is not recorded immediately after session.start. Persist the
timestamp only after the session reports meaningful progress or completion, or
apply the established shorter staleness policy for interrupted exports, while
preserving the existing upload scheduling behavior.
In `@MyHeartCounts/Modules/HealthUploadStaging.swift`:
- Around line 57-65: Update the DatabaseQueue configuration in the visible
initialization block to set GRDB’s built-in journalMode property to .wal, and
remove the prepareDatabase closure that manually executes PRAGMA journal_mode =
WAL. Preserve the existing DatabaseQueue path and other configuration behavior.
In `@MyHeartCounts/Modules/HealthUploadStagingUploader.swift`:
- Around line 134-141: Move the low-power-mode guard in both drain helpers
before their respective fetch loops: update drainPendingSamples and
drainPendingDeletions so they return before calling fetchNextDrainChunk when
low-power mode is enabled. In _process, skip drainPendingDeletions entirely when
didDrainSamples is false.
In `@MyHeartCountsTests/HealthUploadStagingMigrationTests.swift`:
- Around line 31-34: Update the deferred cleanup around dbUrl in the staging
migration tests to remove the SQLite WAL and shared-memory sidecar files
alongside the main database file, using the same temporary-directory cleanup
flow and preserving best-effort removal.
- Around line 43-104: Extend the migration test around
HealthUploadStaging.applyMigrations and HealthUploadStaging initialization to
insert at least one v1 row with an SQLite-unparseable timestamp, then assert the
v2 migration completes successfully and preserves the expected migration
behavior for that row. Cover the relevant pendingSamples or pendingDeletions
path without weakening the existing valid-timestamp 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: CHILL
Plan: Pro Plus
Run ID: bbc26e21-ecbb-445d-83d1-ce2b7373a481
⛔ Files ignored due to path filters (1)
MyHeartCounts.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvedis excluded by!**/Package.resolved
📒 Files selected for processing (23)
.github/scripts/run-ui-test-shard.sh.github/scripts/validate-ui-test-shards.sh.github/ui-test-shards.json.github/ui-test-shards.json.license.github/workflows/build-and-test.ymlMyHeartCounts-FirebaseMyHeartCounts-StudyDefinitionsMyHeartCounts.xcodeproj/project.pbxprojMyHeartCounts.xcodeproj/xcshareddata/xcschemes/MyHeartCountsWatchCompanion.xcschemeMyHeartCounts/Account/Demographics/DemographicsData.swiftMyHeartCounts/Account/VerifyEmailSheet.swiftMyHeartCounts/Health Import/HistoricalHealthSamplesExportManager.swiftMyHeartCounts/Modules/HealthUploadStaging+Schema.swiftMyHeartCounts/Modules/HealthUploadStaging.swiftMyHeartCounts/Modules/HealthUploadStagingUploader.swiftMyHeartCounts/Modules/ManagedFileUpload.swiftMyHeartCounts/MyHeartCountsStandard+HealthKit.swiftMyHeartCounts/Resources/Localizable.xcstringsMyHeartCounts/SensorKit/SensorKitDataFetcher.swiftMyHeartCounts/Utils/DeviceBattery.swiftMyHeartCountsTests/HealthSampleProcessingTests.swiftMyHeartCountsTests/HealthUploadStagingMigrationTests.swiftREADME.md
💤 Files with no reviewable changes (1)
- MyHeartCounts.xcodeproj/xcshareddata/xcschemes/MyHeartCountsWatchCompanion.xcscheme
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
MyHeartCounts/Modules/HealthUploadStaging+Schema.swift (2)
123-163: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
elidePendingUploadsloop has no cancellation checkpoint.This runs from
configure()on every launch via a background-priorityTask, and can loop indefinitely over large backlogs without ever callingTask.checkCancellation(), unlike the sibling drain loops (drainPendingSamples/drainPendingDeletionsinHealthUploadStagingUploader.swift) which check cancellation on every iteration. Given the PR's stated goal of reducing battery/memory pressure, an uncooperative long-running elision pass could keep the app active longer than intended after backgrounding.🔋 Suggested fix
func elidePendingUploads(in dbQueue: DatabaseQueue) throws -> [String: Int] { var summary: [String: Int] = [:] while true { + try Task.checkCancellation() let batch = try dbQueue.write { db -> [PendingRecordKey] in🤖 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/HealthUploadStaging`+Schema.swift around lines 123 - 163, Add a Task.checkCancellation() checkpoint at the start of each iteration in elidePendingUploads, before beginning the database write, so cancellation promptly stops long-running backlog processing while preserving the existing batching and summary behavior.
97-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
createDrainIndexeshardcodesPendingSampleRecord.Columnsfor both tables.
createDrainIndexes(in:for:)is invoked generically for bothpendingSamplesandpendingDeletions, but always resolves column names viatypealias Column = PendingSampleRecord.Columns. This only works today becausePendingDeletionRecord.Columnshappens to use identical raw names fortimestamp/sampleType. A future pending-record type with differently-named columns would silently produce an index on the wrong column name.♻️ Suggested fix: derive column names from the actual record type
- private static func createDrainIndexes(in db: Database) throws { - let recordTypes: [any TableRecord.Type] = [ - PendingSampleRecord.self, - PendingDeletionRecord.self - ] - for recordType in recordTypes { - try createDrainIndexes(in: db, for: recordType.databaseTableName) - } - } - - private static func createDrainIndexes(in db: Database, for table: String) throws { - typealias Column = PendingSampleRecord.Columns - try db.create( - index: "\(table)_on_timestamp_sampleType", - on: table, - columns: [Column.timestamp.name, Column.sampleType.name], - options: .ifNotExists - ) - try db.create( - index: "\(table)_on_sampleType_timestamp", - on: table, - columns: [Column.sampleType.name, Column.timestamp.name], - options: .ifNotExists - ) - } + private static func createDrainIndexes(in db: Database) throws { + let recordTypes: [any _PendingEntityRecord.Type] = [ + PendingSampleRecord.self, + PendingDeletionRecord.self + ] + for recordType in recordTypes { + try createDrainIndexes(in: db, for: recordType) + } + } + + private static func createDrainIndexes(in db: Database, for recordType: some _PendingEntityRecord.Type) throws { + let table = recordType.databaseTableName + try db.create( + index: "\(table)_on_timestamp_sampleType", + on: table, + columns: [recordType.timestampColumn.name, recordType.sampleTypeColumn.name], + options: .ifNotExists + ) + try db.create( + index: "\(table)_on_sampleType_timestamp", + on: table, + columns: [recordType.sampleTypeColumn.name, recordType.timestampColumn.name], + options: .ifNotExists + ) + }🤖 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/HealthUploadStaging`+Schema.swift around lines 97 - 121, Update createDrainIndexes(in:for:) and its caller to derive timestamp and sampleType column names from the actual record type being indexed, rather than hardcoding PendingSampleRecord.Columns. Pass the relevant TableRecord type or column metadata through the generic loop, preserving the existing index names, column order, and ifNotExists behavior for both PendingSampleRecord and PendingDeletionRecord.MyHeartCounts/Modules/ManagedFileUpload+Firebase.swift (1)
52-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider preserving the underlying Firebase error.
String(describing: error)discards theStorageErrorCode, which makes it impossible for callers to distinguish retryable failures (network, quota) from permanent ones later. Rethrowing the original error (and reservingStorageUploadErrorfor the missing-metadata case) keeps that information.🤖 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/ManagedFileUpload`+Firebase.swift around lines 52 - 63, In the upload continuation around putFile, rethrow the original Firebase error directly instead of wrapping it in StorageUploadError.failed(String(describing: error)). Keep StorageUploadError.failed for the missing-metadata branch only, preserving the underlying StorageErrorCode for callers.MyHeartCounts/Modules/ManagedFileUpload.swift (2)
94-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPause ordering can be undone by an in-flight replay.
cancelAndWaitForQuiescencepauses the queue and only then awaits the replay task. If that replay task is already suspended insideresumeQueueIfReplayIsCurrent'sawait uploadQueue.resume(), the resume can land afterpauseAndCancel, flippingacceptsUploadsback totruefor a queue that was just cancelled. The generation re-check afterwards detects the stale replay but does not undo the resume. Awaiting the cancelled replay task before callingpauseAndCancel(or havingresumeQueueIfReplayIsCurrentpass the generation into the queue soresumeis a no-op for stale generations) closes the window.Also applies to: 253-259
🤖 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/ManagedFileUpload.swift` around lines 94 - 103, Update cancelAndWaitForQuiescence so it awaits the captured orphanReplayTask after cancellation before calling uploadQueue.pauseAndCancel, preventing resumeQueueIfReplayIsCurrent from resuming the queue after it is paused. Preserve replayGeneration invalidation, task clearing, and final quiescence behavior.
139-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
Category.stagingDirUrlaligned with the owning directory.
FileUploadInsights.swiftstill readscategory.stagingDirUrl, and nowstage()/replay usestagingDirectory(for:)instead. Derive this URL from the instance owningCategory(for example via aManagedFileUploadhelper) rather than hard-wiring it to the defaultManagedFileUpload.directory.🤖 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/ManagedFileUpload.swift` around lines 139 - 151, Update Category initialization so stagingDirUrl is derived from the owning ManagedFileUpload instance or its stagingDirectory(for:) helper, instead of the static ManagedFileUpload.directory. Keep Category.stagingDirUrl consistent with the directory used by stage() and replay, while preserving FileUploadInsights.swift access.MyHeartCounts/SensorKit/SensorKitDataFetcher.swift (1)
160-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
while truecoalescing loop can iterate more than once.Reusing an in-flight
.limitedfetch and then retrying for.fullis sound, but if another caller registers a fresh.limitedfetch between thedeferclearingactiveFetchand the next iteration, this call can loop again. Each iteration awaits a real task so it can't spin, but bounding it to a single retry would make the intent explicit and rule out pathological chains.🤖 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/SensorKit/SensorKitDataFetcher.swift` around lines 160 - 186, Bound the coalescing loop in the fetch flow around activeFetch to one retry at most. Preserve the existing behavior of retrying when a full allowance reuses a limited fetch, but prevent subsequent iterations from forming a chain if another limited fetch is registered after activeFetch is cleared. Keep awaiting the selected task and returning normally for all other cases.
🤖 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/Health` Import/HistoricalHealthSamplesExportManager.swift:
- Around line 83-90: Update setupAndStartExportSession() to establish a retry
path when charging or low-power guards defer the export: either register a
background processing task requiring external power or observe
UIDevice.batteryStateDidChangeNotification, then retry the historical export
when power conditions become suitable. Preserve the existing cleanup guard
behavior and avoid relying solely on future configure() or enroll(...) calls.
In `@MyHeartCounts/Modules/HealthUploadStaging.swift`:
- Around line 86-97: Update excludeStoreFromBackup to stop silently swallowing
setResourceValues failures: capture the error for each database, -wal, and -shm
URL and surface it through the module’s existing logging or telemetry mechanism,
while preserving the current file-existence checks and backup-exclusion
behavior.
In `@MyHeartCounts/MyHeartCountsStandard.swift`:
- Around line 124-132: Update the startup cleanup flow around
clearPendingAccountData so a thrown cleanup error does not leave health
collection disabled for the remainder of the session. Add a retry mechanism on a
timer or subsequent background task, or surface a user-visible diagnostic, while
preserving the existing logging and pending-upload resume behavior.
In `@MyHeartCounts/SensorKit/SensorKitDataFetcher.swift`:
- Around line 188-208: Update _fetchAndUploadNewData so .limited runs do not
update .lastSensorKitFetch, since they process only one batch per sensor; record
the timestamp only after an unrestricted run completes successfully. Preserve
cancellation and upload behavior, and use the existing allowance value to
distinguish partial from complete drains.
In `@MyHeartCounts/SensorKit/SensorKitDataFetcher`+Uploading+Base.swift:
- Around line 88-94: Update the upload flow around sensorCollection to write the
reference and observation documents through a single Firestore WriteBatch,
committing them atomically. Keep cancellation checking before the batch commit,
and remove the cancellation boundary between the two individual writes so
cancellation cannot leave only the reference document persisted.
In `@MyHeartCountsTests/ManagedFileUploadTests.swift`:
- Around line 30-44: Update the upload coordination in the probe’s upload()
method to stop concurrently consuming the shared releaseStream AsyncStream.
Replace it with a multi-consumer-safe mechanism, such as a shared
CheckedContinuation collection, ensuring each upload waits for and is released
independently while preserving cancellation accounting and active-count cleanup.
---
Nitpick comments:
In `@MyHeartCounts/Modules/HealthUploadStaging`+Schema.swift:
- Around line 123-163: Add a Task.checkCancellation() checkpoint at the start of
each iteration in elidePendingUploads, before beginning the database write, so
cancellation promptly stops long-running backlog processing while preserving the
existing batching and summary behavior.
- Around line 97-121: Update createDrainIndexes(in:for:) and its caller to
derive timestamp and sampleType column names from the actual record type being
indexed, rather than hardcoding PendingSampleRecord.Columns. Pass the relevant
TableRecord type or column metadata through the generic loop, preserving the
existing index names, column order, and ifNotExists behavior for both
PendingSampleRecord and PendingDeletionRecord.
In `@MyHeartCounts/Modules/ManagedFileUpload.swift`:
- Around line 94-103: Update cancelAndWaitForQuiescence so it awaits the
captured orphanReplayTask after cancellation before calling
uploadQueue.pauseAndCancel, preventing resumeQueueIfReplayIsCurrent from
resuming the queue after it is paused. Preserve replayGeneration invalidation,
task clearing, and final quiescence behavior.
- Around line 139-151: Update Category initialization so stagingDirUrl is
derived from the owning ManagedFileUpload instance or its stagingDirectory(for:)
helper, instead of the static ManagedFileUpload.directory. Keep
Category.stagingDirUrl consistent with the directory used by stage() and replay,
while preserving FileUploadInsights.swift access.
In `@MyHeartCounts/Modules/ManagedFileUpload`+Firebase.swift:
- Around line 52-63: In the upload continuation around putFile, rethrow the
original Firebase error directly instead of wrapping it in
StorageUploadError.failed(String(describing: error)). Keep
StorageUploadError.failed for the missing-metadata branch only, preserving the
underlying StorageErrorCode for callers.
In `@MyHeartCounts/SensorKit/SensorKitDataFetcher.swift`:
- Around line 160-186: Bound the coalescing loop in the fetch flow around
activeFetch to one retry at most. Preserve the existing behavior of retrying
when a full allowance reuses a limited fetch, but prevent subsequent iterations
from forming a chain if another limited fetch is registered after activeFetch is
cleared. Keep awaiting the selected task and returning normally for all other
cases.
🪄 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: ccc084b1-ee53-4898-abb4-32a0cf4e87f2
📒 Files selected for processing (23)
.github/workflows/build-and-test.ymlMyHeartCounts/Account/VerifyEmailSheet.swiftMyHeartCounts/Health Import/HealthKitSamplesFHIRUploader.swiftMyHeartCounts/Health Import/HistoricalHealthSamplesExportManager.swiftMyHeartCounts/Modules/HealthUploadStaging+Schema.swiftMyHeartCounts/Modules/HealthUploadStaging.swiftMyHeartCounts/Modules/HealthUploadStagingUploader.swiftMyHeartCounts/Modules/MHCBackgroundTasks.swiftMyHeartCounts/Modules/ManagedFileUpload+Firebase.swiftMyHeartCounts/Modules/ManagedFileUpload+Queue.swiftMyHeartCounts/Modules/ManagedFileUpload.swiftMyHeartCounts/Modules/SetupTestEnvironment.swiftMyHeartCounts/MyHeartCountsStandard+HealthKit.swiftMyHeartCounts/MyHeartCountsStandard.swiftMyHeartCounts/SensorKit/SensorKitDataFetcher+Uploading+Base.swiftMyHeartCounts/SensorKit/SensorKitDataFetcher.swiftMyHeartCounts/SharedContext/Preferences.swiftMyHeartCounts/Utils/DeviceBattery.swiftMyHeartCountsTests/DeviceBatteryTests.swiftMyHeartCountsTests/HealthSampleProcessingTests.swiftMyHeartCountsTests/HealthUploadStagingMigrationTests.swiftMyHeartCountsTests/MHCBackgroundTasksTests.swiftMyHeartCountsTests/ManagedFileUploadTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- MyHeartCounts/Account/VerifyEmailSheet.swift
- .github/workflows/build-and-test.yml
- MyHeartCounts/Modules/HealthUploadStagingUploader.swift
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MyHeartCounts/SensorKit/SensorKitDataFetcher+Uploading+Base.swift (2)
40-42: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRegister temporary-file cleanup before writing the file.
If
write(to:)fails after creating a partial file, thisdeferis never registered and the temporary upload file remains on disk. Install the cleanup immediately after creatingurl, before the write.Proposed fix
let url = URL.temporaryDirectory .appending(component: UUID().uuidString) .appendingPathExtension("\(fileExtension)\(shouldCompress ? ".zstd" : "")") +defer { + try? FileManager.default.removeItem(at: url) +} try (consume data).write(to: url) -defer { - try? FileManager.default.removeItem(at: url) -}🤖 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/SensorKit/SensorKitDataFetcher`+Uploading+Base.swift around lines 40 - 42, Move the defer cleanup in the temporary-file upload flow to immediately after `url` is created and before calling `write(to:)`. Keep the existing `FileManager.default.removeItem` cleanup behavior, ensuring partial files are removed even when writing fails.
44-45: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake SensorKit upload staging and Firestore metadata atomic.
uploadSensorKitFile(...)moves the file into durable staging and queues upload, sostage()returns before Firestore commits. If task cancellation is detected beforebatch.commit(), the caller exits without creating the reference/observation documents and the deferred cleanup removes the temporary copy; the file remains queued in ManagedFileUpload staging with no associated FHIR documents. Defer file staging/enqueueing until after metadata metadata is reserved/committed, or remove the staged upload entry when the Firestore batch is skipped or fails.🤖 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/SensorKit/SensorKitDataFetcher`+Uploading+Base.swift around lines 44 - 45, Make the SensorKit upload flow atomic across file staging and Firestore metadata: update the surrounding upload method so metadata is reserved and successfully committed before calling standard.uploadSensorKitFile, or explicitly remove the staged upload when batch.commit is cancelled or fails. Ensure every exit path avoids leaving a ManagedFileUpload entry without its associated FHIR reference and observation documents.
🤖 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/Task` Handling/Active Tasks/Timed Walk
Test/TimedWalkingTest.swift:
- Around line 198-199: Make finalization in the TimedWalkingTest flow idempotent
by introducing a shared stopping state/task that both automatic and
user-triggered stop paths reuse. Keep session.completeSessionTask set until
stop() finishes, and have concurrent callers await the in-flight finalization
instead of querying or uploading again. Ensure the FHIR resource and
uploadHealthObservation path are immutable/idempotent for the same result so
directFirestore cannot overwrite it with duplicate finalization data.
---
Outside diff comments:
In `@MyHeartCounts/SensorKit/SensorKitDataFetcher`+Uploading+Base.swift:
- Around line 40-42: Move the defer cleanup in the temporary-file upload flow to
immediately after `url` is created and before calling `write(to:)`. Keep the
existing `FileManager.default.removeItem` cleanup behavior, ensuring partial
files are removed even when writing fails.
- Around line 44-45: Make the SensorKit upload flow atomic across file staging
and Firestore metadata: update the surrounding upload method so metadata is
reserved and successfully committed before calling standard.uploadSensorKitFile,
or explicitly remove the staged upload when batch.commit is cancelled or fails.
Ensure every exit path avoids leaving a ManagedFileUpload entry without its
associated FHIR reference and observation documents.
🪄 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: 9f095563-5ad7-4876-b6dc-2670dad51f29
📒 Files selected for processing (8)
MyHeartCounts/Health Import/HistoricalHealthSamplesExportManager.swiftMyHeartCounts/Modules/HealthUploadStaging+Schema.swiftMyHeartCounts/Modules/HealthUploadStaging.swiftMyHeartCounts/Modules/ManagedFileUpload+Firebase.swiftMyHeartCounts/Modules/ManagedFileUpload+Queue.swiftMyHeartCounts/Modules/ManagedFileUpload.swiftMyHeartCounts/SensorKit/SensorKitDataFetcher+Uploading+Base.swiftMyHeartCounts/Task Handling/Active Tasks/Timed Walk Test/TimedWalkingTest.swift
🚧 Files skipped from review as they are similar to previous changes (6)
- MyHeartCounts/Modules/ManagedFileUpload+Firebase.swift
- MyHeartCounts/Modules/HealthUploadStaging+Schema.swift
- MyHeartCounts/Health Import/HistoricalHealthSamplesExportManager.swift
- MyHeartCounts/Modules/ManagedFileUpload+Queue.swift
- MyHeartCounts/Modules/ManagedFileUpload.swift
- MyHeartCounts/Modules/HealthUploadStaging.swift
# Conflicts: # MyHeartCounts.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
| guard DeviceBattery.isCharging, !ProcessInfo.processInfo.isLowPowerModeEnabled else { | ||
| logger.notice("Deferring historical upload until the device is charging") | ||
| waitForSuitablePower() | ||
| return false | ||
| } |
There was a problem hiding this comment.
i'm worried that this might cause us to miss uploads, or end up in a situation where the historical data collection ends up taking way longer than is the case currently (do we know how much potential execution time we will have left if we limit it to "user is actively using the device while it is plugged in and low power mode is disabled", compared to the current much less strict upload timing?)
we might wanna consider instead to register a background task for this (like we already have dedicated tasks for the regular HealthKit uploading and the SensorKit uploading), and then configure that task to have iOS schedule it only if the device is connected to power.
currently the historical health collection+upload actually has no dedicated background task of its own, and instead simply schedules itself when the app is being launched for some other reason (be it a normal user-initiated launch, or any one of the other background tasks we have), so adding a dedicated background task for this action might actually be worthwhile anyway...
| try Self.applyMigrations(to: dbQueue) | ||
| if case .onDisk(let url) = persistence, | ||
| url.standardizedFileURL == Persistence.defaultDatabaseUrl.standardizedFileURL { | ||
| Self.excludeStoreFromBackup(at: url) |
There was a problem hiding this comment.
are we sure we want this?
There was a problem hiding this comment.
It might grow very large with every backup? Ideally we have an easy way to figure out when we wouldd get the past data we didn't capture between a new installation and this? Technically our complete historical data upload would probably capture this anyways?
| let tables = try String.fetchAll(db, sql: """ | ||
| SELECT name FROM sqlite_master WHERE type = 'table' | ||
| AND name NOT LIKE 'sqlite_%' AND name NOT LIKE 'grdb_%' | ||
| """) | ||
| return try tables.allSatisfy { table in | ||
| try Int.fetchOne(db, sql: "SELECT 1 FROM \(table.quotedDatabaseIdentifier) LIMIT 1") == nil | ||
| } | ||
| try PendingSampleRecord.fetchCount(db) == 0 | ||
| && PendingDeletionRecord.fetchCount(db) == 0 |
There was a problem hiding this comment.
it previously was intentionally implemented the way it was, so that the property would be guaranteed to stay correct if we make changes to the schema down the road, without us needing to manually keep the implemenation here in sync...
There was a problem hiding this comment.
what's the overall idea behind these changes? are we sure that the new code matches the semantics and behaviour of the previous implementation?
| do { | ||
| try Task.checkCancellation() | ||
| try await handler() | ||
| try Task.checkCancellation() |
There was a problem hiding this comment.
what if the task gets cancelled inbetween the handler returning and the cancellation check in the next line? why would we want to consider that a failure?
| @concurrent | ||
| private func _fetchAndUploadNewData(_ allowance: DeviceBattery.WorkAllowance) async throws { | ||
| try Task.checkCancellation() | ||
| let maximumBatchesPerSensor = allowance == .limited ? 1 : nil |
There was a problem hiding this comment.
why do we impose this limit? wouldn't that mean that if the allowance is limited, we would fetch only a single batch per sensor per background task run?
| } catch is CancellationError { | ||
| throw CancellationError() | ||
| } catch let error as SensorKitProcessingError { | ||
| throw error | ||
| } catch { | ||
| if Task.isCancelled { | ||
| throw CancellationError() | ||
| } |
There was a problem hiding this comment.
why do we add both a catch is CancellationError as well as a if Task.isCancelled, both of which throw CancellationError()s? the second (generic catch with isCancelled check) sumsumes the first...
| private(set) var inProgressResult: TimedWalkingTestResult | ||
| /// The `Task` that waits for the session's duration to pass, and then ends the session | ||
| fileprivate var completeSessionTask: Task<TimedWalkingTestResult?, any Error>? | ||
| fileprivate var finalizationTask: Task<TimedWalkingTestResult?, any Error>? |
There was a problem hiding this comment.
why do we need (want) this?
| switch context { | ||
| case .explicitUserLogoutEvent: | ||
| LocalPreferencesStore.standard[.pendingAccountDataCleanupRequired] = true | ||
| if case .explicitUserLogoutEvent = context { | ||
| await appState.setIsLoggingOut(true) | ||
| case .onLaunchCleanupBcNoUser: | ||
| break | ||
| } |
There was a problem hiding this comment.
using switch instead of if case .foo = bar is an intentional choice on my end; i always strictly prefer the switch and never use if case in such situations, the reason being that the switch forces us to explicitly enumerate all cases in the enum, meaning that if we add/remove a case down the road (and change the semantics of the enum) the compiler will emit an error in all places in the program where we were using the enum, making it easy for us to validate that these use sites still operate on the correct cases
eg if we were to add a new case to the enum here, with semantics overlapping those of the existing explicitUserLogoutEvent (eg implicitUserLogoutEvent), the switch-based impl would force us to add the case (and make a decision how it should be handled), whereas the if case impl would simply silently ignore the new case, thereby leading to incorrect behaviour
There was a problem hiding this comment.
noticed this in a couple of other places as well; will probably rewrite all of them back to using a switch...
| logger.error("Unable to clear \(description): \(error)") | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
this should not be a type-level function here. it's called attemptAccountDataCleanup, but it really just calls the closure it is provided, swallows the error, and returns true/false to indicate success/failure.
it doesn't enforce that the closure perform cleanup work, or that it operate on account data.
Co-authored-by: Paul Schmiedmayer <PSchmiedmayer@users.noreply.github.com>
Improve Battery Usage and Memory Pressure
Background
Xcode Organizer diagnostics showed avoidable resource pressure in background health and SensorKit processing.
Uploads are bounded to two concurrent files and account cleanup prevents pending data from crossing sign-ins. UI tests now build once and run as four validated shards, reducing the work repeated after a flaky failure.
⚙️ Release Notes
Code of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: