Improve bounded iBeacon ranging and region reconciliation - 3 - #5735
Draft
Pixelobserver wants to merge 7 commits into
Draft
Improve bounded iBeacon ranging and region reconciliation - 3#5735Pixelobserver wants to merge 7 commits into
Pixelobserver wants to merge 7 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Five unresolved moderate findings affect background-task coverage and collector lifecycle, state, and reconciliation behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves iBeacon verification and region reconciliation with bounded ranging, retry handling, background execution, and region-limit enforcement.
Changes:
- Adds ranging-based entry verification and exit reconciliation.
- Shares scan constraints and bounds retries, cancellation, and execution time.
- Adds lifecycle, freshness, cancellation, and region-limit regression coverage.
Five moderate findings remain regarding background-task protection and collector scan, state, and reconciliation handling.
File summaries
| File | Description |
|---|---|
Tests/Shared/AppZone.test.swift |
Updates region assertion formatting. |
Tests/App/ZoneManager/ZoneManagerRegionFilter.test.swift |
Tests combined region limits. |
Tests/App/ZoneManager/ZoneManagerProcessor.test.swift |
Tests beacon-exit processing. |
Tests/App/ZoneManager/ZoneManagerCollector.test.swift |
Adds ranging and lifecycle regression coverage. |
Tests/App/ZoneManager/ZoneManager.test.swift |
Updates collector test doubles. |
Tests/App/ZoneManager/FakeCLLocationManager.swift |
Adds ranging and authorization test hooks. |
Sources/Shared/API/Models/AppZone.swift |
Clarifies beacon monitoring behavior. |
Sources/App/ZoneManager/ZoneManagerState.swift |
Adds ranging failure state. |
Sources/App/ZoneManager/ZoneManagerRegionFilter.swift |
Enforces combined region-limit filtering. |
Sources/App/ZoneManager/ZoneManagerProcessor.swift |
Processes beacon exits. |
Sources/App/ZoneManager/ZoneManagerIgnoreReason.swift |
Updates verification ignore reason. |
Sources/App/ZoneManager/ZoneManagerEvent.swift |
Adds ranging diagnostics. |
Sources/App/ZoneManager/ZoneManagerCollector.swift |
Implements bounded ranging and reconciliation. |
Sources/App/ZoneManager/UIApplicationBeaconScanBackgroundExecution.swift |
Implements background-task leasing. |
Sources/App/ZoneManager/RangedBeaconSample.swift |
Defines ranged sample data. |
Sources/App/ZoneManager/BeaconScanBackgroundExecution.swift |
Defines the execution abstraction. |
Sources/App/ZoneManager/BeaconDiagnostic.swift |
Defines ranging diagnostic metadata. |
Review details
Suppressed comments (3)
Sources/App/ZoneManager/ZoneManagerCollector.swift:166
- This guard is global to the whole opportunistic scan: if any foreground entry (for example beacon A) exists, regions that are only opportunistic (beacon B) are never added or ranged. Let the constraint-sharing logic below reuse overlapping foreground ranges while still registering and starting new constraints.
// Foreground ranging is already continuous, so an additional timed scan
// would only duplicate work.
guard foregroundBeaconEntries.isEmpty else { return }
Sources/App/ZoneManager/ZoneManagerCollector.swift:347
- Every location update calls
startOpportunisticBeaconScanning, whose first action cancels the current timeout and replaces it. Repeated location callbacks during one scan can therefore keep ranging and the background work alive past the intended 25-second bound and continually restart unchanged scans. Route this through the idempotent background reconciliation path or avoid replacing an active window when the regions are unchanged.
startOpportunisticBeaconScanning(in: manager.monitoredRegions, manager: manager)
Sources/App/ZoneManager/ZoneManagerCollector.swift:98
- When an existing identifier changes its beacon constraint, this removes the foreground entry but leaves any pending verification keyed only by that identifier. A callback for the old constraint can then be accepted and emitted as an event for the current zone even though its beacon definition changed. Invalidate pending work when the constraint changes, while preserving it only for an intentional lifecycle handoff to another consumer.
let identifiersToRemove = foregroundBeaconEntries.compactMap { identifier, entry in
desiredEntries[identifier]?.constraint == entry.constraint ? nil : identifier
}
let removedConstraints = identifiersToRemove.compactMap { identifier in
foregroundBeaconEntries.removeValue(forKey: identifier)?.constraint
}
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Pixelobserver
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Invalid background-task acquisition and non-empty samples being treated as empty can undermine bounded ranging correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Balanced
Comment on lines
+9
to
+15
| identifier = UIApplication.shared.beginBackgroundTask( | ||
| withName: "ZoneManagerBeaconScan", | ||
| expirationHandler: { [weak self] in | ||
| expirationHandler() | ||
| self?.end() | ||
| } | ||
| ) |
Comment on lines
+320
to
+322
| guard let detectedBeacon = samples.first(where: isBeaconInsideRange) else { | ||
| reconcileEmptyBeaconSample(identifiers: Array(identifiers), manager: manager) | ||
| return |
4 tasks
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5735 +/- ##
==========================================
+ Coverage 42.87% 42.93% +0.06%
==========================================
Files 1124 1125 +1
Lines 78186 78286 +100
==========================================
+ Hits 33523 33614 +91
- Misses 44663 44672 +9
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bgoncal
marked this pull request as draft
September 11, 2026 13:45
Member
|
Keeping as draft as we review the PRs that this one depends on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI Policy
Select exactly one option that describes AI usage in this contribution:
Summary
Improve iBeacon transition verification with bounded ranging. This is part 3 of the split requested for #5629.
This collector/filter foundation is independent of #5703 and #5730. Existing Core Location delegate paths use the ranging logic; the remaining lifecycle integration and durable event delivery follow separately in part 4.
Screenshots
N/A — no visual changes or new settings.
Link to pull request in Documentation repository
No documentation PR yet. This change improves verification of existing beacon behavior without adding configuration options.
Any other notes
Completed local validation:
Pending CI verification:
Twelve additional regression test cases were added during preparation. This describes available tests, not a completed test run. Real-device ranging and battery behavior have not been verified for this revision.