Conversation
* remove the CLLocationCoordinate2D extension that added Equatable Conformance and replace it with a .isNear(to:) method that behaves in exactly the same way but with a name that is more descriptive of its purpose
This also avoids undefined behavior in the future if CLLocationCoordinate2D ever gains Equatable conformance.
* fix localisation string in VoiceSettings.storyboard That had no affect on the app but was causing a warning
* add dummy experiment to git rid of warning about empty enum
There was a problem hiding this comment.
Pull Request Overview
This PR refactors coordinate equality comparisons by replacing the custom Equatable extension on CLLocationCoordinate2D with a more explicit isNear(to:) method. This prevents potential undefined behavior if Apple ever adds native Equatable conformance to CLLocationCoordinate2D and makes the fuzzy comparison behavior more obvious to developers.
Key changes include:
- Replaced all
==coordinate comparisons with.isNear(to:)method calls - Added a placeholder experiment to resolve empty enum warning
- Fixed a localization string in the VoiceSettings storyboard
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| CoreLocation+Extensions.swift | Removed Equatable extension and added explicit isNear(to:) methods |
| Multiple test files | Updated test assertions to use isNear(to:) for coordinate comparisons |
| Multiple production files | Replaced == with .isNear(to:) for all coordinate comparisons |
| GeometryUtilsTest.swift | Added helper function assertNear for testing coordinate arrays |
| VoiceSettings.storyboard | Fixed incorrect localization string key |
| ExperimentManager.swift | Added placeholder experiment case to resolve empty enum warning |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // If the selected location on the underlying entity has changed, reconfigure the view | ||
| // to show the new distance | ||
| let beaconCoordinateDidChange = newValue?.locationDetail.location.coordinate != oldValue?.locationDetail.location.coordinate | ||
| let beaconCoordinateDidChange = !(newValue?.locationDetail.location.coordinate.isNear(to: oldValue?.locationDetail.location.coordinate) ?? false) |
There was a problem hiding this comment.
This line is overly complex and hard to read. Consider extracting the coordinate comparison logic into a separate variable or method for better readability.
|
|
||
| // Check if the coordinates are the same | ||
| guard self != coordinate else { | ||
| guard !self .isNear(to: coordinate )else { |
There was a problem hiding this comment.
Inconsistent spacing around the method call. There should be no space before the dot operator and after the parentheses.
| // note: We want the first adjacent road coordinate to be the same as the last coordinate of the root road | ||
| guard let lastAdjacentRoadCoordinate = nextRoadSegmentCoordinates.last else { return nil } | ||
| let reversedDirection = (lastCoordinate == lastAdjacentRoadCoordinate) | ||
| let reversedDirection = lastCoordinate .isNear(to: lastAdjacentRoadCoordinate) |
There was a problem hiding this comment.
Inconsistent spacing before the dot operator. There should be no space between the variable name and the dot.
|
|
||
| private func showAnnotationAndCenter(_ annotation: MKAnnotation) { | ||
| guard centerCoordinate != annotation.coordinate else { | ||
| guard !centerCoordinate.isNear(to: annotation.coordinate )else { |
There was a problem hiding this comment.
Inconsistent spacing after the closing parenthesis. There should be a space between the closing parenthesis and 'else'.
| if coordinatesExcludingRoot.contains(where: {coordinate in | ||
| coordinates.contains(where: {coordinate.isNear(to: $0)}) | ||
| }) { | ||
| return true |
There was a problem hiding this comment.
The nested contains(where:) calls create confusing logic. Consider refactoring this into a more readable implementation with better variable naming to distinguish between the outer and inner coordinate variables.
| if coordinatesExcludingRoot.contains(where: {coordinate in | |
| coordinates.contains(where: {coordinate.isNear(to: $0)}) | |
| }) { | |
| return true | |
| for roadCoordinate in coordinatesExcludingRoot { | |
| for trailCoordinate in coordinates { | |
| if roadCoordinate.isNear(to: trailCoordinate) { | |
| return true | |
| } | |
| } |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces tolerance-based coordinate comparison via CLLocationCoordinate2D.isNear, replacing strict equality across geo, map, data, preview, and UI layers. Removes prior Equatable for CLLocationCoordinate2D. Updates related logic and tests accordingly. Adds a placeholder experiment case. Adjusts a storyboard’s layout and metadata. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Source as Location Source
participant Store as BeaconDetailStore
participant VC as BeaconTitleViewController
participant UI as UI
User->>Source: Move device / update location
Source-->>Store: newCoordinate, source
Store->>Store: if oldCoordinate.isNear(to: newCoordinate) and source unchanged
alt Near (within threshold)
Store-->>UI: No update
else Not near or source changed
Store->>VC: notifyBeaconChanged(newCoordinate)
VC->>VC: isNear(to:) check vs prior
alt Significant change
VC->>UI: Update title/config
else Insignificant change
VC-->>UI: No visual change
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/ios/GuideDogs/Code/Visual UI/Views/VoiceSettings.storyboard (1)
55-77: SpeakingRateTableViewCell assigned to VoiceInstructionsCell but its slider outlet is missing — fix or change classSpeakingRateTableViewCell declares
@IBOutlet weak var speakingRateSlider: UISlider!and callsspeakingRateSlider.isContinuous = false; the storyboard VoiceInstructionsCell contains only a label (no slider) — this will crash at runtime.
- Fix options: (A) change the storyboard cell's customClass to
UITableViewCellif it's informational, or (B) add a UISlider to the cell and connectspeakingRateSlider, or (C) make the outlet optional and guard before use (avoid IUO access).- Also remove the fixed
rowHeight="120"for this multiline label and use dynamic sizing (UITableView.automaticDimension) to support large accessibility text.Files: apps/ios/GuideDogs/Code/Visual UI/Views/VoiceSettings.storyboard, apps/ios/GuideDogs/Code/Visual UI/Controls/Voice Settings/SpeakingRateTableViewCell.swift
apps/ios/GuideDogs/Code/App/Feature Flags/Experiment Flighting/ExperimentManager.swift (3)
19-28: uuid must be deterministic per experiment; generating a new UUID each access breaks persistence and matching.
isEnabled(_:)asksFlightwithexperiment.uuid. A freshUUID()per access guarantees mismatches with server configs and any persisted state.If you keep the enum case(s), map each to a fixed UUID (example below shows the debug placeholder only):
- var uuid: UUID { - // Return a UUID for each known experiment - return UUID() - } + var uuid: UUID { + switch self { + #if DEBUG + case .placeholder: + return UUID(uuidString: "00000000-0000-0000-0000-000000000000")! + #endif + } + }
83-86: Dispatch delegate callbacks to the main thread.These callbacks can trigger UI work; they currently run on
services.soundscape.flightmanagerqueue. Dispatch to.mainto avoid UI threading bugs.- if descriptionsDidComplete { - self?.delegate?.onExperimentManagerReady() - } + if descriptionsDidComplete { + DispatchQueue.main.async { self?.delegate?.onExperimentManagerReady() } + }- if configurationDidComplete { - self?.delegate?.onExperimentManagerReady() - } + if configurationDidComplete { + DispatchQueue.main.async { self?.delegate?.onExperimentManagerReady() } + }Also applies to: 109-111
11-31: Prevent placeholder leakage and make experiment IDs stable
- TelemetryHelper enumerates KnownExperiment.allCases and emits keys "experiment..isEnabled" (apps/ios/GuideDogs/Code/App/Logging/TelemetryHelper.swift:206–207) — the current
placeholderwill be recorded.- KnownExperiment currently returns a new UUID() on each access (apps/ios/GuideDogs/Code/App/Feature Flags/Experiment Flighting/ExperimentManager.swift:11–31) — experiment identifiers must be stable.
- Action items: replace
var uuid { return UUID() }with an explicit switch returning fixed UUID literals per case; and either remove theplaceholdercase, make it DEBUG‑only, or implement a customallCasesthat filtersplaceholderout in non‑debug builds. Also update TelemetryHelper to iterate only the sanitized list of experiments.apps/ios/GuideDogs/Code/Data/Models/Protocols/Road.swift (1)
150-152: Bug:road2Directioncomputed fromroad1.Likely copy/paste error; this skews sorting of next segments.
- let road1Direction = road1.direction(at: intersection) - let road2Direction = road1.direction(at: intersection) + let road1Direction = road1.direction(at: intersection) + let road2Direction = road2.direction(at: intersection)apps/ios/GuideDogs/Code/App/Helpers/GeometryUtils.swift (1)
72-77: Close polygon with a line, not a move.Using move(to:) starts a new subpath and doesn’t close the polygon; addLine(to:) properly closes it. Behaviorally safer for CGPath.contains.
- if !coordinates.last!.isNear(to: coordinates.first! ){ - (pixelX, pixelY) = VectorTile.getPixelXY(latitude: coordinates.first!.latitude, longitude: coordinates.first!.longitude, zoom: 16) - } - path.move(to: CGPoint(x: pixelX, y: pixelY)) + if !coordinates.last!.isNear(to: coordinates.first!) { + let (firstX, firstY) = VectorTile.getPixelXY(latitude: coordinates.first!.latitude, + longitude: coordinates.first!.longitude, + zoom: 16) + path.addLine(to: CGPoint(x: firstX, y: firstY)) + }
♻️ Duplicate comments (4)
apps/ios/GuideDogs/Code/App/Framework Extensions/MapKit/MKMapView+Extensions.swift (1)
23-23: Fix spacing around guard/else.Nit: Style issue flagged previously. Add a space before
elseand remove the extra space before).- guard !centerCoordinate.isNear(to: annotation.coordinate )else { + guard !centerCoordinate.isNear(to: annotation.coordinate) else {apps/ios/GuideDogs/Code/App/Framework Extensions/Geo Extensions/CoreLocation+Extensions.swift (1)
168-170: Fix spacing around method call and guard/else.Remove the stray space before
.and add a space beforeelse.- guard !self .isNear(to: coordinate )else { + guard !self.isNear(to: coordinate) else {apps/ios/GuideDogs/Code/Visual UI/Controls/Beacon/BeaconTitleViewController.swift (1)
100-100: Readability improved by extracting the proximity check.This addresses prior feedback about complexity around the inline comparison.
apps/ios/GuideDogs/Code/Data/Preview/IntersectionFinder.swift (1)
415-415: Fix spacing before the dot operator.Remove the stray space before .isNear for consistency.
- let reversedDirection = lastCoordinate .isNear(to: lastAdjacentRoadCoordinate) + let reversedDirection = lastCoordinate.isNear(to: lastAdjacentRoadCoordinate)
🧹 Nitpick comments (17)
apps/ios/GuideDogs/Code/Visual UI/Views/VoiceSettings.storyboard (1)
16-16: Many fractional frames; likely design-time noise.No functional change, but these floaty rects tend to oscillate across editors. If possible, rely fully on constraints and let runtime layout drive frames to reduce diffs.
Also applies to: 25-25, 56-56, 79-79, 82-82, 86-86, 93-93
apps/ios/GuideDogs/Code/App/Feature Flags/Experiment Flighting/ExperimentManager.swift (5)
114-117: Correct the log message for descriptions failure.This path fetches experiment descriptions, not flight controls.
- case .failure(let error): GDLogAppError("Error downloading flight controls: \(error.localizedDescription)") + case .failure(let error): GDLogAppError("Error downloading experiment descriptions: \(error.localizedDescription)")
160-164: Fix load error log message and include error details.Currently logs “save” on load failure and drops the underlying error.
- } catch { - GDLogAppError("Unable to save current flight state") - return nil - } + } catch { + GDLogAppError("Unable to load current flight state: \(error)") + return nil + }
183-185: Use atomic writes when persisting configuration.Prevents partial files on crash/power loss and reduces corruption risk.
- let data = try JSONEncoder().encode(configuration) - try data.write(to: url) + let data = try JSONEncoder().encode(configuration) + try data.write(to: url, options: .atomic)
128-131: Typo in docs: “lunch” → “launch”.Tiny doc polish.
- /// has a chance to select a language in the language selection screen on initial lunch. + /// has a chance to select a language in the language selection screen on initial launch.
34-36: Doc wording: say “experiment manager” instead of “flight manager”.Aligns with the protocol/class name.
- /// Called when the flight manager has finished downloading the experiment controls file + /// Called when the experiment manager has finished downloading the experiment controls fileapps/ios/GuideDogs/Code/Data/Models/Cache Models/Intersection.swift (1)
614-631: Minor readability nit in firstIndex(where:) closure.Consider spacing in the closure and keeping style consistent across the file.
- guard let intersectionCoordinateIndex = roadCoordinates.firstIndex(where: {$0.isNear(to: intersectionCoordinate) }) else { + guard let intersectionCoordinateIndex = roadCoordinates.firstIndex(where: { $0.isNear(to: intersectionCoordinate) }) else {Also applies to: 630-631
apps/ios/GuideDogs/Code/Data/Preview/RoadAdjacentDataView.swift (4)
14-14: Remove stale “fixme Equatable” comment.Equatable is implemented below; the inline comment is outdated.
-struct RoadAdjacentDataView: AdjacentDataView /*, fixme Equatable */ { +struct RoadAdjacentDataView: AdjacentDataView {
433-446: Tighten equality iteration; satisfy SwiftLint “for_where” or use allSatisfy.Current loop trips the
for_whererule and is slightly verbose.Option A (clear + fast-fail):
- for (c1, c2) in zip(lhs.coordinatesToEndpoint, rhs.coordinatesToEndpoint) { - if !c1.isNear(to: c2) { return false } - } + for (c1, c2) in zip(lhs.coordinatesToEndpoint, rhs.coordinatesToEndpoint) where !c1.isNear(to: c2) { + return false + }Option B (idiomatic):
- for (c1, c2) in zip(lhs.coordinatesToEndpoint, rhs.coordinatesToEndpoint) { - if !c1.isNear(to: c2) { return false } - } + guard zip(lhs.coordinatesToEndpoint, rhs.coordinatesToEndpoint) + .allSatisfy({ $0.isNear(to: $1) }) else { return false }
439-445: Dictionary comparison: consider allSatisfy to streamline.Equivalent behavior with less branching.
- for (key, loc1) in lhs.adjacentCalloutLocationsHistory { - guard let loc2 = rhs.adjacentCalloutLocationsHistory[key], - loc1.isNear(to: loc2) - else { - return false - } - } + guard lhs.adjacentCalloutLocationsHistory.allSatisfy({ (key, loc1) in + guard let loc2 = rhs.adjacentCalloutLocationsHistory[key] else { return false } + return loc1.isNear(to: loc2) + }) else { return false }
266-268: Bearing equality likely needs tolerance.Strict
==on bearings risks duplicates slipping through due to FP jitter. Prefer an angular tolerance or wrap-aware difference.- guard !adjacent.contains(where: { $0.endpoint == adjacentIntersection.endpoint && $0.direction.bearing == adjacentIntersection.direction.bearing }) else { + let dup = adjacent.contains { e in + e.endpoint == adjacentIntersection.endpoint && + abs(e.direction.bearing.angularDifference(from: adjacentIntersection.direction.bearing)) <= 1.0 + } + guard !dup else {apps/ios/GuideDogs/Code/App/Framework Extensions/Geo Extensions/CoreLocation+Extensions.swift (1)
226-235: Consider distance-based proximity helper (optional).
isNearuses degree deltas; a meters-based variant can be more uniform across latitudes. Keep current API, but optionally add:public func isNearMeters(to coordinate: CLLocationCoordinate2D, maxDistance: CLLocationDistance = 0.25) -> Bool { CLLocation(self).distance(from: CLLocation(coordinate)) <= maxDistance }apps/ios/GuideDogs/Code/Data/Spatial Data/SpatialDataCache.swift (1)
349-349: Prefer “nearest within threshold” over “first near” for intersection selection.If multiple intersections are within tolerance, picking the first is order‑dependent. Filter by proximity, then choose the nearest.
- return intersections(forRoadKey: roadKey)?.first(where: { $0.coordinate.isNear(to: coordinate) }) + guard let list = intersections(forRoadKey: roadKey) else { return nil } + let near = list.filter { $0.coordinate.isNear(to: coordinate) } + return near.min { lhs, rhs in + lhs.coordinate.distance(from: coordinate) < rhs.coordinate.distance(from: coordinate) + }apps/ios/GuideDogs/Code/Data/Preview/IntersectionFinder.swift (1)
451-454: Refactor nested contains(where:) for clarity and early-exit.Current double-closure is hard to read and reuses the same variable name. Use straightforward loops with early return.
- if coordinatesExcludingRoot.contains(where: {coordinate in - coordinates.contains(where: {coordinate.isNear(to: $0)}) - }) { - return true - } + for roadCoordinate in coordinatesExcludingRoot { + if coordinates.contains(where: { roadCoordinate.isNear(to: $0) }) { + return true + } + }apps/ios/GuideDogs/Code/Data/Authored Activities/AuthoredActivityContent.swift (1)
365-369: Bug: Using activity name instead of POI name in description output.This prints the activity’s name for each POI row; should use wpt.name.
- desc.append("\t\t\(index + 1). \(name) (\(loc)), \(hasDescription)\n") + desc.append("\t\t\(index + 1). \(wpt.name) (\(loc)), \(hasDescription)\n")apps/ios/GuideDogs/Code/App/Helpers/GeometryUtils.swift (1)
329-331: Remove stray semicolon.SwiftLint trailing_semicolon; also idiomatic Swift.
- guard !path.isEmpty else { - return nil; - } + guard !path.isEmpty else { + return nil + }apps/ios/UnitTests/App/Helpers/GeometryUtilsTest.swift (1)
117-117: Remove trailing semicolons flagged by SwiftLint.Minor style cleanups.
- let badKey = GeometryUtils.coordinates(geoJson: "{a: 1}"); + let badKey = GeometryUtils.coordinates(geoJson: "{a: 1}") ... - XCTAssertNotNil(before_closest); + XCTAssertNotNil(before_closest)Also applies to: 349-349
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/ios/GuideDogs/Code/App/Feature Flags/Experiment Flighting/ExperimentManager.swift(1 hunks)apps/ios/GuideDogs/Code/App/Framework Extensions/Geo Extensions/CoreLocation+Extensions.swift(3 hunks)apps/ios/GuideDogs/Code/App/Framework Extensions/MapKit/MKMapView+Extensions.swift(1 hunks)apps/ios/GuideDogs/Code/App/Helpers/GeometryUtils.swift(4 hunks)apps/ios/GuideDogs/Code/Data/Authored Activities/AuthoredActivityContent.swift(1 hunks)apps/ios/GuideDogs/Code/Data/Models/Cache Models/Intersection.swift(4 hunks)apps/ios/GuideDogs/Code/Data/Models/Database Models/ReferenceEntity.swift(1 hunks)apps/ios/GuideDogs/Code/Data/Models/Protocols/Road.swift(1 hunks)apps/ios/GuideDogs/Code/Data/Models/Temp Models/GenericLocation.swift(1 hunks)apps/ios/GuideDogs/Code/Data/Preview/IntersectionFinder.swift(8 hunks)apps/ios/GuideDogs/Code/Data/Preview/RoadAdjacentDataView.swift(2 hunks)apps/ios/GuideDogs/Code/Data/Spatial Data/SpatialDataCache.swift(1 hunks)apps/ios/GuideDogs/Code/Visual UI/Controls/Beacon/BeaconTitleViewController.swift(1 hunks)apps/ios/GuideDogs/Code/Visual UI/Helpers/Location/Location Detail/LocationDetail.swift(1 hunks)apps/ios/GuideDogs/Code/Visual UI/Observable Data Stores/BeaconDetailStore.swift(1 hunks)apps/ios/GuideDogs/Code/Visual UI/View Controllers/POI Table/Location Detail/EditableMapViewController.swift(2 hunks)apps/ios/GuideDogs/Code/Visual UI/Views/VoiceSettings.storyboard(7 hunks)apps/ios/UnitTests/App/Helpers/GeometryUtilsTest.swift(7 hunks)apps/ios/UnitTests/Behaviors/Route Guidance/RouteGuidanceTest.swift(1 hunks)apps/ios/UnitTests/Data/Authored Activities/AuthoredActivityContentTest.swift(3 hunks)
🧰 Additional context used
🪛 SwiftLint (0.57.0)
apps/ios/GuideDogs/Code/App/Feature Flags/Experiment Flighting/ExperimentManager.swift
[Warning] 17-17: TODOs should be resolved (Replace with real experiments)
(todo)
apps/ios/UnitTests/App/Helpers/GeometryUtilsTest.swift
[Warning] 349-349: Lines should not have trailing semicolons
(trailing_semicolon)
apps/ios/GuideDogs/Code/Data/Preview/RoadAdjacentDataView.swift
[Warning] 435-435: where clauses are preferred over a single if inside a for
(for_where)
🔇 Additional comments (31)
apps/ios/GuideDogs/Code/Visual UI/Views/VoiceSettings.storyboard (3)
124-125: System symbol metric updates — safe.SF Symbols dimension metadata changes are benign and expected between Xcode versions.
2-2: Pin Xcode/CI version to avoid storyboard churn — manual verification requiredConsider pinning the team/CI Xcode version (e.g., add .xcode-version or specify Xcode in the CI workflow matrix). The verification script returned "No files were searched" and did not find any .xcode-version or Xcode pins; confirm whether the repo already pins an Xcode version and add one if missing.
70-71: Localization key fix removes IB warning — LGTM.Cannot verify that "voice.apple.additional" exists in all shipped locales because the verification script failed (tee: command not found). Confirm the key is present in every Localizable.strings file.
apps/ios/GuideDogs/Code/Data/Models/Cache Models/Intersection.swift (2)
383-394: Good switch to near-equality for T-intersection vertex checks.Using the optional-aware
isNear(to:)on.first/.lastis correct and avoids nil pitfalls.
523-541: Similarity filter now tolerant to small coordinate drift — LGTM.The
isNear-based clustering should reduce duplicate intersections caused by tiny deltas.apps/ios/GuideDogs/Code/App/Framework Extensions/Geo Extensions/CoreLocation+Extensions.swift (1)
139-140: Origin check via near-equality — LGTM.Treating near-(0,0) as invalid is consistent with the new semantics.
apps/ios/GuideDogs/Code/Data/Models/Temp Models/GenericLocation.swift (1)
94-96: Adoption of isNear — LGTM.Tolerance-based containment aligns with the project-wide change.
apps/ios/GuideDogs/Code/Visual UI/Observable Data Stores/BeaconDetailStore.swift (1)
179-181: Solid debounce of trivial coordinate jitter.The guard correctly avoids unnecessary publishes unless source changes or movement exceeds tolerance.
apps/ios/GuideDogs/Code/Data/Models/Protocols/Road.swift (1)
168-174: Correct tolerance-based comparisons at endpoints and along-path.Makes direction detection robust to minor coordinate drift.
apps/ios/GuideDogs/Code/Visual UI/Helpers/Location/Location Detail/LocationDetail.swift (1)
85-87: Equatable semantics updated to proximity — looks good.Near‑equality on coordinates avoids false negatives while preserving strict equality on names/addresses.
Also applies to: 92-92, 99-99
apps/ios/GuideDogs/Code/Data/Models/Database Models/ReferenceEntity.swift (1)
409-410: Right gate for location changes.Suppressing updates when new coords are “near” prevents noisy writes and unintended route churn.
Consider adding a unit test to assert that tiny deltas do not trigger
Route.updateWaypointInAllRoutesbut larger deltas do.apps/ios/GuideDogs/Code/Visual UI/View Controllers/POI Table/Location Detail/EditableMapViewController.swift (2)
126-129: Avoids spurious delegate updates when center hasn’t meaningfully moved.Matches new coordinate semantics.
135-147: Good UX: immediate edit when already centered; otherwise recenter then transition.Tolerance check prevents unnecessary map animations.
apps/ios/UnitTests/Behaviors/Route Guidance/RouteGuidanceTest.swift (1)
178-178: Good switch to proximity checks in tests.
Aligns tests with runtime semantics and eliminates flaky failures from floating-point jitter. Verification script returned no output; re-run this check locally to confirm there are no remaining direct CLLocationCoordinate2D/.coordinate equality comparisons in tests:
rg -n --type=swift -C1 '(.coordinate|CLLocationCoordinate2D)\s*(==|!=)' apps/ios/UnitTests
Also applies to: 184-184, 191-191, 198-198, 205-205apps/ios/GuideDogs/Code/Data/Preview/IntersectionFinder.swift (6)
37-45: LGTM: Near-equality guard in init is correct.Switching to isNear(to:) avoids fragile exact-equality checks and matches the PR intent.
198-221: LGTM: Endpoint intersection lookup matches new semantics.Using isNear(to:) for the road-end intersection and skipping synthesis when an intersection exists preserves prior behavior with tolerance.
303-304: Direction reversal check reads well.Computing reversedDirection based on whether the adjacent segment ends at the root intersection is sound with isNear(to:).
339-341: LGTM: First-intersection lookup now tolerant.isNear(to:) in firstIntersection aligns with broader updates.
398-401: LGTM: Cycle check against root uses tolerance appropriately.
303-304: Confirm adjacent-road reversal logic in both call sites.Both reversedDirection values compare the root/last coordinate to nextRoadSegment.coordinates.last via isNear(to:). Call sites: apps/ios/GuideDogs/Code/Data/Preview/IntersectionFinder.swift (lines ~302–304 and ~414–415). The only reference to nextRoadSegments found is the Road protocol declaration (apps/ios/GuideDogs/Code/Data/Models/Protocols/Road.swift:124) — no concrete implementations located in the repo, so ordering invariants could not be verified. Verify that all Road implementations / data sources guarantee the returned coordinates are oriented so .last is the shared endpoint; if not, either normalize segment orientation before use or derive reversedDirection by checking both endpoints (first OR last) and choosing orientation accordingly.
apps/ios/GuideDogs/Code/Data/Authored Activities/AuthoredActivityContent.swift (1)
407-409: LGTM: contains(location:) now tolerance-based.This matches app-wide semantics and avoids fragile equality.
apps/ios/UnitTests/Data/Authored Activities/AuthoredActivityContentTest.swift (1)
87-91: LGTM: Tests updated to isNear(to:).Assertions now align with proximity semantics; good coverage retained for v1/v2 parse cases.
Also applies to: 174-179, 186-187
apps/ios/GuideDogs/Code/App/Helpers/GeometryUtils.swift (3)
128-135: LGTM: split(at:) uses tolerant index lookup.
150-152: LGTM: rotate guard updated for tolerance.
180-181: LGTM: Circularity check now tolerance-based.apps/ios/UnitTests/App/Helpers/GeometryUtilsTest.swift (6)
160-168: LGTM: assertNear improves resilience of rotate tests.
276-287: LGTM: referenceCoordinate assertions now tolerant.
299-302: LGTM: Edge-case assertions updated to isNear(to:).Also applies to: 312-323
339-356: LGTM: closestEdge tests now compare with tolerance.
401-406: LGTM: Equidistant closest-edge checks use isNear(to:).
420-451: LGTM: assertNear helper is concise and reusable.Solid helper for sequence-wise proximity checks.
| // Supports client experimentation | ||
| // For each experiment, add a case to `KnownExperiment` | ||
| // | ||
| case placeholder // TODO: Replace with real experiments |
There was a problem hiding this comment.
Avoid shipping a dummy enum case; make it debug‑only or remove via explicit allCases.
Keeping placeholder in a CaseIterable enum risks showing a bogus “experiment” in any UI/logic that iterates KnownExperiment.allCases. Also the // TODO trips SwiftLint. Prefer either: (A) gate the placeholder behind #if DEBUG and give it a stable UUID; or (B) remove the placeholder and implement allCases as [] until real experiments exist.
Option A (debug‑only placeholder, stable UUID, no TODO):
enum KnownExperiment: CaseIterable {
@@
- case placeholder // TODO: Replace with real experiments
+ #if DEBUG
+ /// Debug-only placeholder to keep CaseIterable non-empty during development.
+ case placeholder
+ #endif
@@
- var uuid: UUID {
- /*
- switch self {
- case someExperiment: return "Experiment UUID"
- }
- */
-
- // Return a UUID for each known experiment
- return UUID()
- }
+ var uuid: UUID {
+ switch self {
+ #if DEBUG
+ case .placeholder:
+ // Stable value to avoid churn in persisted state or lookups.
+ return UUID(uuidString: "00000000-0000-0000-0000-000000000000")!
+ #endif
+ }
+ }Option B (no placeholder; explicit empty cases):
-enum KnownExperiment: CaseIterable {
-
- //
- // Supports client experimentation
- // For each experiment, add a case to `KnownExperiment`
- //
- case placeholder // TODO: Replace with real experiments
-
- var uuid: UUID {
- // Return a UUID for each known experiment
- return UUID()
- }
-
- static let configurationVersion = "1.0.0"
-}
+enum KnownExperiment: CaseIterable {
+ static var allCases: [KnownExperiment] { [] }
+ var uuid: UUID {
+ preconditionFailure("No KnownExperiment cases defined yet")
+ }
+ static let configurationVersion = "1.0.0"
+}📝 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.
| case placeholder // TODO: Replace with real experiments | |
| enum KnownExperiment: CaseIterable { | |
| static var allCases: [KnownExperiment] { [] } | |
| var uuid: UUID { | |
| preconditionFailure("No KnownExperiment cases defined yet") | |
| } | |
| static let configurationVersion = "1.0.0" | |
| } |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 17-17: TODOs should be resolved (Replace with real experiments)
(todo)
🤖 Prompt for AI Agents
In apps/ios/GuideDogs/Code/App/Feature Flags/Experiment
Flighting/ExperimentManager.swift around line 17, the CaseIterable enum contains
a shipping placeholder case which can leak into production UIs and trips
SwiftLint; either replace it with a debug‑only case or remove it and provide an
explicit empty allCases. For Option A: wrap the placeholder case in #if DEBUG /
#endif (no TODO comment), give it a stable UUID value if the enum requires
associated IDs, and leave CaseIterable synthesis for debug builds only. For
Option B: delete the placeholder case entirely and implement a manual static var
allCases: [KnownExperiment] { return [] } (or the equivalent explicit array) so
production builds have no dummy experiment. Ensure no TODO comments remain and
the enum compiles in both debug and release.
remove the CLLocationCoordinate2D extension that added Equatable Conformance and replace it with a .isNear(to:) method that behaves in exactly the same way but with a name that is more descriptive of its purpose
This also avoids undefined behavior in the future if CLLocationCoordinate2D ever gains Equatable conformance.
add dummy experiment to git rid of warning about empty enum
Summary by CodeRabbit
Bug Fixes
Style