soundscape icon indicating copy to clipboard operation
soundscape copied to clipboard

Clean up some compiler warnings

Open RDMurray opened this issue 6 months ago • 3 comments

  • 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

Summary by CodeRabbit

  • Bug Fixes

    • Improved map centering, route guidance, and POI/intersection detection by using proximity-based coordinate comparison, reducing jitter and ignoring minor GPS noise.
    • Prevented unnecessary beacon and detail updates when locations change only slightly.
    • Made geometry operations (path rotations, circular checks, splits) tolerant to tiny coordinate differences for more consistent behavior.
  • Style

    • Refined Voice settings screen layout (spacing, cell sizes, image assets) for better readability.
    • Updated a localization string on the Voice instructions cell.

RDMurray avatar Jun 11 '25 09:06 RDMurray

@coderabbitai full review

RDMurray avatar Sep 18 '25 18:09 RDMurray

✅ Actions performed

Full review triggered.

coderabbitai[bot] avatar Sep 18 '25 18:09 coderabbitai[bot]

[!NOTE]

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Introduces 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

Cohort / File(s) Summary
Geo proximity API
apps/ios/GuideDogs/Code/App/Framework Extensions/Geo Extensions/CoreLocation+Extensions.swift
Adds public isNear(to:threshold:) overloads for CLLocationCoordinate2D (including optional). Replaces origin/zero, bearing, and equality logic with near-equality. Removes Equatable conformance and == operator.
Map interaction
apps/ios/GuideDogs/Code/App/Framework Extensions/MapKit/MKMapView+Extensions.swift
showAnnotationAndCenter now early-returns when map center is near the annotation (uses isNear).
Geometry utilities
apps/ios/GuideDogs/Code/App/Helpers/GeometryUtils.swift
Replaces ==/!= and index lookups with isNear in closing paths, splitting, rotating, and circularity checks.
Data models: intersections & roads
.../Data/Models/Cache Models/Intersection.swift, .../Data/Models/Protocols/Road.swift
Uses isNear for endpoint checks, direction determination, intersection similarity, and coordinate lookups.
Data models: entities & spatial cache
.../Data/Models/Database Models/ReferenceEntity.swift, .../Data/Spatial Data/SpatialDataCache.swift
Location-change detection and intersection lookup now use isNear instead of strict equality.
Generic/location containment
.../Data/Models/Temp Models/GenericLocation.swift, .../Data/Authored Activities/AuthoredActivityContent.swift
contains(location:) now matches by isNear rather than exact coordinate equality.
Preview logic
.../Data/Preview/IntersectionFinder.swift
Applies isNear across initialization, traversal, endpoint handling, and cycle detection. No signature changes.
Preview view model equality
.../Data/Preview/RoadAdjacentDataView.swift
Removes inline Equatable on struct; adds custom Equatable extension using isNear for coordinates and array/keyed comparisons.
Beacon flows
.../Visual UI/Observable Data Stores/BeaconDetailStore.swift, .../Visual UI/Controls/Beacon/BeaconTitleViewController.swift
Change-detection gates now use isNear to ignore minor coordinate movements.
Location detail equality
.../Visual UI/Helpers/Location/Location Detail/LocationDetail.swift
LocationDetail.Source equality compares coordinates with isNear in all relevant cases.
Editable map VC
.../Visual UI/View Controllers/POI Table/Location Detail/EditableMapViewController.swift
Done/edit actions switch from exact to near-equality checks for center/selection.
Feature flags
.../App/Feature Flags/Experiment Flighting/ExperimentManager.swift
Adds placeholder enum case to KnownExperiment; all other logic unchanged.
Storyboard updates
.../Visual UI/Views/VoiceSettings.storyboard
Updates Interface Builder versions, adjusts frames/sizes, updates a localization key, and tweaks asset sizes.
Unit tests: geometry
apps/ios/UnitTests/App/Helpers/GeometryUtilsTest.swift
Introduces assertNear and replaces exact equality assertions with near-equality across tests.
Unit tests: route guidance
apps/ios/UnitTests/Behaviors/Route Guidance/RouteGuidanceTest.swift
Replaces several coordinate equality assertions with isNear.
Unit tests: authored activities
apps/ios/UnitTests/Data/Authored Activities/AuthoredActivityContentTest.swift
Uses isNear for coordinate assertions in GPX parsing tests.

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

Thump-thump go my paws on the map I steer,
Trading “equals” for “almost,” we nuzzle near.
Roads now meet where numbers once lied,
Beacons hush when jitters subside.
With gentle hops through tests we cheer—
A hare’s soft tolerance, crystal-clear. 🐇🗺️

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 65.85% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Clean up some compiler warnings" is concise and directly related to the PR’s stated intent to silence warnings (removing the Equatable extension, adding a dummy enum case, and fixing a storyboard localization). It is somewhat generic and does not highlight the notable behavioral change of switching from strict coordinate equality to tolerance-based isNear comparisons, which reviewers may want flagged. Overall the title is acceptable because it accurately reflects a real and central purpose of the changeset.
✨ Finishing touches
  • [ ] 📝 Generate Docstrings
🧪 Generate unit tests
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch cleanup2

[!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.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

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 help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Sep 18 '25 18:09 coderabbitai[bot]