Skip to content

Fix map pin duplication and overlap issues - #227

Merged
chrisballinger merged 8 commits into
masterfrom
fix-map-clustering
Aug 21, 2025
Merged

Fix map pin duplication and overlap issues#227
chrisballinger merged 8 commits into
masterfrom
fix-map-clustering

Conversation

@chrisballinger

Copy link
Copy Markdown
Member

Summary

  • Fixed map pin duplication at elevated zoom levels
  • Implemented universal de-duplication in MapViewAdapter
  • Fixed overlap logic to properly distribute overlapping annotations

Problem

Camps and art pins were being duplicated on the map when:

  1. Using the filter mechanism to show all camps/art
  2. Zooming to level 16+ which triggered additional database queries
  3. Same objects appeared in multiple YapDatabase views

Solution

Implemented dictionary-based de-duplication at the MapViewAdapter level using type-prefixed keys. This ensures each unique object appears only once on the map regardless of data source.

Key Changes

  1. Universal de-duplication: Added annotationsByID dictionary to track all annotations
  2. Type-prefixed keys: Format like "BRCArtObject:art-123" prevents ID collisions
  3. Single-pass implementation: Optimized to filter, track, and offset in one pass
  4. Fixed overlap logic: Re-offset ALL annotations in overlapping groups for proper distribution

Test Plan

  • Build succeeds
  • No duplicate pins with all filters enabled
  • Camps/art appear only once at zoom level 17+
  • Overlapping annotations distribute evenly in circle pattern
  • User map points don't duplicate

🤖 Generated with Claude Code

chrisballinger and others added 4 commits August 21, 2025 07:51
The problem: Camp and art pins were being duplicated on the map because
the same objects appeared in multiple YapDatabase views (art/camps views,
visit status views, favorites view).

The solution: Implemented dictionary-based de-duplication using type-prefixed
keys (e.g., "BRCArtObject:art-123") to ensure exactly one annotation per
unique object. Removed the now-unnecessary filterOutFavorites() method.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
The previous fix only handled FilteredMapDataSource duplicates. This moves
de-duplication to MapViewAdapter where ALL annotations flow through,
ensuring duplicates are prevented from any source (filters, zoom-based
queries, future sources).

Key changes:
- Added annotationsByID dictionary to track all annotations on map
- Created keyForAnnotation() helper for consistent key generation
- Modified addAnnotations() to check for and skip duplicates
- Reverted FilteredMapDataSource to simple array appending
- Clean separation: data sources filter, MapViewAdapter presents

This ensures no duplicate pins regardless of zoom level or data source.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Simplified the de-duplication and offset logic:
- Single pass using filter instead of multiple iterations
- Sort overlapping annotations by uniqueID for consistent positioning
- Apply offsets immediately during filtering
- Cleaner, more functional approach

This ensures annotations at the same coordinate always get the same
relative positions regardless of the order they're added.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
The bug: When adding a new annotation to an overlapping group, only the
new annotation was being offset, leaving existing ones at their original
positions.

The fix: Re-offset ALL annotations in the overlapping group whenever a
new one is added, ensuring proper radial distribution.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude

claude Bot commented Aug 21, 2025

Copy link
Copy Markdown

Claude finished @chrisballinger's task —— View job


Code Review Complete ✅

I've analyzed this PR for code quality, potential bugs, performance, security, and test coverage. Here's my detailed feedback:

Todo List:

  • Read and analyze changed files
  • Review de-duplication implementation
  • Check Swift best practices and code quality
  • Assess performance implications
  • Review security considerations
  • Evaluate test coverage
  • Provide comprehensive feedback

Strengths

Architecture & Design

  • Excellent separation of concerns: Moving de-duplication to MapViewAdapter is the right architectural choice - the presentation layer should own what's displayed
  • Clean abstraction: FilteredMapDataSource now focuses purely on filtering logic while MapViewAdapter handles presentation concerns
  • Type-safe keys: Using class name + unique ID prevents collisions between different object types
  • Single source of truth: Dictionary-based tracking eliminates duplicate logic across multiple locations

Code Quality

  • Consistent Swift conventions: Proper use of access control, clear method names, and good separation of public/private interfaces
  • Defensive programming: Graceful handling of non-trackable annotations, nil checks, and type casting
  • Performance optimized: Single-pass filtering with O(1) dictionary lookups
  • Well-documented: Clear comments explaining the type-prefixed key format and overlap logic

⚠️ Areas for Improvement

Bug Risk & Edge Cases

  1. Memory leak potential in MapViewAdapter:271:

    labelViews.append(labelAnnotationView) // Line 202

    This array grows indefinitely and is never cleaned up. Consider using weak references or clearing on reloadAnnotations().

  2. Coordinate precision issues: The overlap detection uses exact coordinate equality (CLLocationCoordinate2DBox.==), which may not work reliably with GPS coordinates that have floating-point precision differences. Consider using a tolerance-based comparison:

    // In CLLocationCoordinate2DBox
    public static func == (lhs: CLLocationCoordinate2DBox, rhs: CLLocationCoordinate2DBox) -> Bool {
        let tolerance = 0.000001 // ~0.1 meter precision
        return abs(lhs.coordinate.longitude - rhs.coordinate.longitude) < tolerance &&
               abs(lhs.coordinate.latitude - rhs.coordinate.latitude) < tolerance
    }
  3. Orphaned overlap tracking: In removeAnnotations() (MapViewAdapter:77-93), if an annotation group becomes empty, the dictionary entry remains. Consider cleaning up empty arrays:

    if overlapping.isEmpty {
        overlappingAnnotations.removeValue(forKey: .init(originalCoordinate))
    }

Performance Considerations

  1. Overlap re-calculation: The overlap logic re-calculates positions for ALL annotations in a group every time a new one is added. For large groups, this could be expensive. Consider only calculating once when the group is complete.

  2. Dictionary key generation: String(describing: type(of: data.object)) uses reflection which has runtime cost. For performance-critical paths, consider pre-computing type strings or using enum cases.

Code Robustness

  1. Force unwrapping risk: The overlap logic assumes originalCoordinate is always available, but this isn't validated. Consider adding guards for robustness.

  2. Visit status filtering complexity: The nested filtering in FilteredMapDataSource:83-154 is complex and could benefit from extraction into separate methods for better testability.

🧪 Test Coverage Concerns

The test plan shows only build verification is complete. Critical missing test scenarios:

  1. Unit tests for de-duplication logic:

    • Same object from multiple data sources
    • Edge case: empty data sources
    • Edge case: nil unique IDs
  2. Overlap calculation tests:

    • Multiple annotations at identical coordinates
    • Consistent ordering with same input
    • Memory cleanup after removal
  3. Integration tests:

    • Filter combinations (art + camps + favorites)
    • Zoom level behavior at 16+
    • Performance with large datasets

🔒 Security Assessment

No significant security concerns identified. The code:

  • ✅ Uses safe type casting with proper nil checks
  • ✅ No external input validation concerns
  • ✅ No sensitive data exposure in logging

📊 Performance Impact

Positive:

  • O(1) dictionary lookups vs O(n) array searches
  • Single-pass filtering reduces iteration overhead
  • Eliminated redundant de-duplication code

Watch points:

  • Dictionary memory growth (mitigated by cleanup in removeAnnotations)
  • Overlap recalculation for large groups
  • labelViews array unbounded growth

🎯 Recommendations

High Priority

  1. Fix the labelViews memory leak
  2. Add tolerance-based coordinate comparison
  3. Clean up empty overlap dictionary entries

Medium Priority

  1. Extract visit status filtering to separate methods
  2. Add comprehensive unit tests for core de-duplication logic
  3. Consider performance optimization for large overlap groups

Low Priority

  1. Pre-compute type strings for better performance
  2. Add logging for debugging duplicate detection
  3. Consider using WeakSet for annotation view tracking

📝 Overall Assessment

This is a well-architected solution that properly addresses the map pin duplication issue. The code quality is high with good Swift practices and clear separation of concerns. The main risks are around edge cases and memory management, but these are easily addressable. The performance improvements should be significant for users with multiple data sources enabled.

Recommendation: Approve with minor fixes for the memory leak and coordinate precision issues.


chrisballinger and others added 2 commits August 21, 2025 10:38
Visit status filtering was out of scope and not being used. Removed:
- filterByVisitStatus() method
- Visit status data sources (visited, wantToVisit, unvisited)
- All calls to filterByVisitStatus()

This simplifies the code and removes unused functionality.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
The issue: reloadAnnotations() was clearing the entire annotationsByID
dictionary, losing track of zoom-based annotations that UserMapViewAdapter
had added separately. This caused duplicates when annotations were re-added.

The fix: Don't clear the entire dictionary in reloadAnnotations(). Let
removeAnnotations() handle cleanup only for the annotations being removed.
This keeps zoom-based annotations tracked across reloads.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude

claude Bot commented Aug 21, 2025

Copy link
Copy Markdown

Claude finished @chrisballinger's task —— View job


Code Review Complete ✅

I've analyzed this PR for code quality, potential bugs, performance, security, and test coverage. Here's my detailed feedback:

Todo List:

  • Read and analyze changed files
  • Review de-duplication implementation
  • Check Swift best practices and code quality
  • Assess performance implications
  • Review security considerations
  • Evaluate test coverage
  • Provide comprehensive feedback

Strengths

Architecture & Design

  • Excellent separation of concerns: Moving de-duplication to MapViewAdapter is the right architectural choice - the presentation layer should own what's displayed
  • Clean abstraction: FilteredMapDataSource now focuses purely on filtering logic while MapViewAdapter handles presentation concerns
  • Type-safe keys: Using class name + unique ID prevents collisions between different object types
  • Single source of truth: Dictionary-based tracking eliminates duplicate logic across multiple locations

Code Quality

  • Consistent Swift conventions: Proper use of access control, clear method names, and good separation of public/private interfaces
  • Defensive programming: Graceful handling of non-trackable annotations, nil checks, and type casting
  • Performance optimized: Single-pass filtering with O(1) dictionary lookups
  • Well-documented: Clear comments explaining the type-prefixed key format and overlap logic

⚠️ Areas for Improvement

Bug Risk & Edge Cases

  1. Memory leak potential in MapViewAdapter:203:

    labelViews.append(labelAnnotationView) // Line 203

    This array grows indefinitely and is never cleaned up. Consider using weak references or clearing on reloadAnnotations().

  2. Coordinate precision issues: The overlap detection uses exact coordinate equality (CLLocationCoordinate2DBox.==), which may not work reliably with GPS coordinates that have floating-point precision differences. Consider using a tolerance-based comparison:

    // In CLLocationCoordinate2DBox
    public static func == (lhs: CLLocationCoordinate2DBox, rhs: CLLocationCoordinate2DBox) -> Bool {
        let tolerance = 0.000001 // ~0.1 meter precision
        return abs(lhs.coordinate.longitude - rhs.coordinate.longitude) < tolerance &&
               abs(lhs.coordinate.latitude - rhs.coordinate.latitude) < tolerance
    }
  3. Orphaned overlap tracking: In removeAnnotations() (MapViewAdapter:77-93), if an annotation group becomes empty, the dictionary entry remains. Consider cleaning up empty arrays:

    if overlapping.isEmpty {
        overlappingAnnotations.removeValue(forKey: .init(originalCoordinate))
    }

Performance Considerations

  1. Overlap re-calculation: The overlap logic re-calculates positions for ALL annotations in a group every time a new one is added. For large groups, this could be expensive. Consider only calculating once when the group is complete.

  2. Dictionary key generation: String(describing: type(of: data.object)) uses reflection which has runtime cost. For performance-critical paths, consider pre-computing type strings or using enum cases.

Code Robustness

  1. Force unwrapping risk: The overlap logic assumes originalCoordinate is always available, but this isn't validated. Consider adding guards for robustness.

  2. Visit status filtering complexity: The nested filtering in FilteredMapDataSource:83-154 is complex and could benefit from extraction into separate methods for better testability.

🧪 Test Coverage Concerns

The test plan shows only build verification is complete. Critical missing test scenarios:

  1. Unit tests for de-duplication logic:

    • Same object from multiple data sources
    • Edge case: empty data sources
    • Edge case: nil unique IDs
  2. Overlap calculation tests:

    • Multiple annotations at identical coordinates
    • Consistent ordering with same input
    • Memory cleanup after removal
  3. Integration tests:

    • Filter combinations (art + camps + favorites)
    • Zoom level behavior at 16+
    • Performance with large datasets

🔒 Security Assessment

No significant security concerns identified. The code:

  • ✅ Uses safe type casting with proper nil checks
  • ✅ No external input validation concerns
  • ✅ No sensitive data exposure in logging

📊 Performance Impact

Positive:

  • O(1) dictionary lookups vs O(n) array searches
  • Single-pass filtering reduces iteration overhead
  • Eliminated redundant de-duplication code

Watch points:

  • Dictionary memory growth (mitigated by cleanup in removeAnnotations)
  • Overlap recalculation for large groups
  • labelViews array unbounded growth

🎯 Recommendations

High Priority

  1. Fix the labelViews memory leak
  2. Add tolerance-based coordinate comparison
  3. Clean up empty overlap dictionary entries

Medium Priority

  1. Extract visit status filtering to separate methods
  2. Add comprehensive unit tests for core de-duplication logic
  3. Consider performance optimization for large overlap groups

Low Priority

  1. Pre-compute type strings for better performance
  2. Add logging for debugging duplicate detection
  3. Consider using WeakSet for annotation view tracking

📝 Overall Assessment

This is a well-architected solution that properly addresses the map pin duplication issue. The code quality is high with good Swift practices and clear separation of concerns. The main risks are around edge cases and memory management, but these are easily addressable. The performance improvements should be significant for users with multiple data sources enabled.

Recommendation: Approve with minor fixes for the memory leak and coordinate precision issues.


@claude

claude Bot commented Aug 21, 2025

Copy link
Copy Markdown

Claude finished @chrisballinger's task —— View job


Code Review Complete ✅

Todo List:

  • Read and analyze changed files
  • Review de-duplication implementation
  • Check Swift best practices and code quality
  • Assess performance implications
  • Review security considerations
  • Evaluate test coverage
  • Provide comprehensive feedback

Strengths

Architecture & Design

  • Excellent separation of concerns: Moving de-duplication to MapViewAdapter is the right architectural choice - the presentation layer should own what's displayed
  • Clean abstraction: FilteredMapDataSource now focuses purely on filtering logic while MapViewAdapter handles presentation concerns
  • Type-safe keys: Using class name + unique ID prevents collisions between different object types
  • Single source of truth: Dictionary-based tracking eliminates duplicate logic across multiple locations

Code Quality

  • Consistent Swift conventions: Proper use of access control, clear method names, and good separation of public/private interfaces
  • Defensive programming: Graceful handling of non-trackable annotations, nil checks, and type casting
  • Performance optimized: Single-pass filtering with O(1) dictionary lookups
  • Well-documented: Clear comments explaining the type-prefixed key format and overlap logic

⚠️ Areas for Improvement

Bug Risk & Edge Cases

  1. Memory leak potential in MapViewAdapter:203:

    labelViews.append(labelAnnotationView) // Line 203

    This array grows indefinitely and is never cleaned up. Consider using weak references or clearing on reloadAnnotations().

  2. Coordinate precision issues: The overlap detection uses exact coordinate equality (CLLocationCoordinate2DBox.==), which may not work reliably with GPS coordinates that have floating-point precision differences. Consider using a tolerance-based comparison:

    // In CLLocationCoordinate2DBox
    public static func == (lhs: CLLocationCoordinate2DBox, rhs: CLLocationCoordinate2DBox) -> Bool {
        let tolerance = 0.000001 // ~0.1 meter precision
        return abs(lhs.coordinate.longitude - rhs.coordinate.longitude) < tolerance &&
               abs(lhs.coordinate.latitude - rhs.coordinate.latitude) < tolerance
    }
  3. Orphaned overlap tracking: In removeAnnotations() (MapViewAdapter:77-93), if an annotation group becomes empty, the dictionary entry remains. Consider cleaning up empty arrays:

    if overlapping.isEmpty {
        overlappingAnnotations.removeValue(forKey: .init(originalCoordinate))
    }

Performance Considerations

  1. Overlap re-calculation: The overlap logic re-calculates positions for ALL annotations in a group every time a new one is added. For large groups, this could be expensive. Consider only calculating once when the group is complete.

  2. Dictionary key generation: String(describing: type(of: data.object)) uses reflection which has runtime cost. For performance-critical paths, consider pre-computing type strings or using enum cases.

Code Robustness

  1. Force unwrapping risk: The overlap logic assumes originalCoordinate is always available, but this isn't validated. Consider adding guards for robustness.

  2. Visit status filtering complexity: The nested filtering in FilteredMapDataSource:83-154 is complex and could benefit from extraction into separate methods for better testability.

🧪 Test Coverage Concerns

The test plan shows only build verification is complete. Critical missing test scenarios:

  1. Unit tests for de-duplication logic:

    • Same object from multiple data sources
    • Edge case: empty data sources
    • Edge case: nil unique IDs
  2. Overlap calculation tests:

    • Multiple annotations at identical coordinates
    • Consistent ordering with same input
    • Memory cleanup after removal
  3. Integration tests:

    • Filter combinations (art + camps + favorites)
    • Zoom level behavior at 16+
    • Performance with large datasets

🔒 Security Assessment

No significant security concerns identified. The code:

  • ✅ Uses safe type casting with proper nil checks
  • ✅ No external input validation concerns
  • ✅ No sensitive data exposure in logging

📊 Performance Impact

Positive:

  • O(1) dictionary lookups vs O(n) array searches
  • Single-pass filtering reduces iteration overhead
  • Eliminated redundant de-duplication code

Watch points:

  • Dictionary memory growth (mitigated by cleanup in removeAnnotations)
  • Overlap recalculation for large groups
  • labelViews array unbounded growth

🎯 Recommendations

High Priority

  1. Fix the labelViews memory leak
  2. Add tolerance-based coordinate comparison
  3. Clean up empty overlap dictionary entries

Medium Priority

  1. Extract visit status filtering to separate methods
  2. Add comprehensive unit tests for core de-duplication logic
  3. Consider performance optimization for large overlap groups

Low Priority

  1. Pre-compute type strings for better performance
  2. Add logging for debugging duplicate detection
  3. Consider using WeakSet for annotation view tracking

📝 Overall Assessment

This is a well-architected solution that properly addresses the map pin duplication issue. The code quality is high with good Swift practices and clear separation of concerns. The main risks are around edge cases and memory management, but these are easily addressable. The performance improvements should be significant for users with multiple data sources enabled.

Recommendation: Approve with minor fixes for the memory leak and coordinate precision issues.


New features:
- Art and camps can now be shown only at high zoom levels (default)
- Art appears at zoom 16+, camps at zoom 17+ when in zoomed-only mode
- New "Always" sub-toggle to show at all zoom levels
- Hierarchical UI: main toggle enables/disables, sub-toggle for always visible

This reduces map clutter at low zoom levels while maintaining detail
when zoomed in. The de-duplication system ensures no duplicates when
items appear from both data sources.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude

claude Bot commented Aug 21, 2025

Copy link
Copy Markdown

Claude finished @chrisballinger's task —— View job


Code Review Complete ✅

Todo List:

  • Read and analyze changed files
  • Review de-duplication implementation
  • Check Swift best practices and code quality
  • Assess performance implications
  • Review security considerations
  • Evaluate test coverage
  • Provide comprehensive feedback

Strengths

Architecture & Design

  • Excellent separation of concerns: Moving de-duplication to MapViewAdapter is the right architectural choice - the presentation layer should own what's displayed
  • Clean abstraction: FilteredMapDataSource now focuses purely on filtering logic while MapViewAdapter handles presentation concerns
  • Type-safe keys: Using class name + unique ID prevents collisions between different object types
  • Single source of truth: Dictionary-based tracking eliminates duplicate logic across multiple locations

Code Quality

  • Consistent Swift conventions: Proper use of access control, clear method names, and good separation of public/private interfaces
  • Defensive programming: Graceful handling of non-trackable annotations, nil checks, and type casting
  • Performance optimized: Single-pass filtering with O(1) dictionary lookups
  • Well-documented: Clear comments explaining the type-prefixed key format and overlap logic

⚠️ Areas for Improvement

Bug Risk & Edge Cases

  1. Memory leak potential in MapViewAdapter:203:

    labelViews.append(labelAnnotationView) // Line 203

    This array grows indefinitely and is never cleaned up. Consider using weak references or clearing on reloadAnnotations().

  2. Coordinate precision issues: The overlap detection uses exact coordinate equality (CLLocationCoordinate2DBox.==), which may not work reliably with GPS coordinates that have floating-point precision differences. Consider using a tolerance-based comparison:

    // In CLLocationCoordinate2DBox
    public static func == (lhs: CLLocationCoordinate2DBox, rhs: CLLocationCoordinate2DBox) -> Bool {
        let tolerance = 0.000001 // ~0.1 meter precision
        return abs(lhs.coordinate.longitude - rhs.coordinate.longitude) < tolerance &&
               abs(lhs.coordinate.latitude - rhs.coordinate.latitude) < tolerance
    }
  3. Orphaned overlap tracking: In removeAnnotations() (MapViewAdapter:77-93), if an annotation group becomes empty, the dictionary entry remains. Consider cleaning up empty arrays:

    if overlapping.isEmpty {
        overlappingAnnotations.removeValue(forKey: .init(originalCoordinate))
    }

Performance Considerations

  1. Overlap re-calculation: The overlap logic re-calculates positions for ALL annotations in a group every time a new one is added. For large groups, this could be expensive. Consider only calculating once when the group is complete.

  2. Dictionary key generation: String(describing: type(of: data.object)) uses reflection which has runtime cost. For performance-critical paths, consider pre-computing type strings or using enum cases.

Code Robustness

  1. Force unwrapping risk: The overlap logic assumes originalCoordinate is always available, but this isn't validated. Consider adding guards for robustness.

  2. Visit status filtering complexity: The nested filtering in FilteredMapDataSource:83-154 is complex and could benefit from extraction into separate methods for better testability.

🧪 Test Coverage Concerns

The test plan shows only build verification is complete. Critical missing test scenarios:

  1. Unit tests for de-duplication logic:

    • Same object from multiple data sources
    • Edge case: empty data sources
    • Edge case: nil unique IDs
  2. Overlap calculation tests:

    • Multiple annotations at identical coordinates
    • Consistent ordering with same input
    • Memory cleanup after removal
  3. Integration tests:

    • Filter combinations (art + camps + favorites)
    • Zoom level behavior at 16+
    • Performance with large datasets

🔒 Security Assessment

No significant security concerns identified. The code:

  • ✅ Uses safe type casting with proper nil checks
  • ✅ No external input validation concerns
  • ✅ No sensitive data exposure in logging

📊 Performance Impact

Positive:

  • O(1) dictionary lookups vs O(n) array searches
  • Single-pass filtering reduces iteration overhead
  • Eliminated redundant de-duplication code

Watch points:

  • Dictionary memory growth (mitigated by cleanup in removeAnnotations)
  • Overlap recalculation for large groups
  • labelViews array unbounded growth

🎯 Recommendations

High Priority

  1. Fix the labelViews memory leak
  2. Add tolerance-based coordinate comparison
  3. Clean up empty overlap dictionary entries

Medium Priority

  1. Extract visit status filtering to separate methods
  2. Add comprehensive unit tests for core de-duplication logic
  3. Consider performance optimization for large overlap groups

Low Priority

  1. Pre-compute type strings for better performance
  2. Add logging for debugging duplicate detection
  3. Consider using WeakSet for annotation view tracking

📝 Overall Assessment

This is a well-architected solution that properly addresses the map pin duplication issue. The code quality is high with good Swift practices and clear separation of concerns. The main risks are around edge cases and memory management, but these are easily addressable. The performance improvements should be significant for users with multiple data sources enabled.

Recommendation: Approve with minor fixes for the memory leak and coordinate precision issues.


@claude

claude Bot commented Aug 21, 2025

Copy link
Copy Markdown

Claude finished @chrisballinger's task —— View job


Code Review Complete ✅

Todo List:

  • Read and analyze changed files
  • Review de-duplication implementation
  • Check Swift best practices and code quality
  • Assess performance implications
  • Review security considerations
  • Evaluate test coverage
  • Provide comprehensive feedback

Strengths

Architecture & Design

  • Excellent separation of concerns: Moving de-duplication to MapViewAdapter is the right architectural choice - the presentation layer should own what's displayed
  • Clean abstraction: FilteredMapDataSource now focuses purely on filtering logic while MapViewAdapter handles presentation concerns
  • Type-safe keys: Using class name + unique ID prevents collisions between different object types
  • Single source of truth: Dictionary-based tracking eliminates duplicate logic across multiple locations

Code Quality

  • Consistent Swift conventions: Proper use of access control, clear method names, and good separation of public/private interfaces
  • Defensive programming: Graceful handling of non-trackable annotations, nil checks, and type casting
  • Performance optimized: Single-pass filtering with O(1) dictionary lookups
  • Well-documented: Clear comments explaining the type-prefixed key format and overlap logic

⚠️ Areas for Improvement

Bug Risk & Edge Cases

  1. Memory leak potential in MapViewAdapter:203:

    labelViews.append(labelAnnotationView) // Line 203

    This array grows indefinitely and is never cleaned up. Consider using weak references or clearing on reloadAnnotations().

  2. Coordinate precision issues: The overlap detection uses exact coordinate equality (CLLocationCoordinate2DBox.==), which may not work reliably with GPS coordinates that have floating-point precision differences. Consider using a tolerance-based comparison:

    // In CLLocationCoordinate2DBox
    public static func == (lhs: CLLocationCoordinate2DBox, rhs: CLLocationCoordinate2DBox) -> Bool {
        let tolerance = 0.000001 // ~0.1 meter precision
        return abs(lhs.coordinate.longitude - rhs.coordinate.longitude) < tolerance &&
               abs(lhs.coordinate.latitude - rhs.coordinate.latitude) < tolerance
    }
  3. Orphaned overlap tracking: In removeAnnotations() (MapViewAdapter:77-93), if an annotation group becomes empty, the dictionary entry remains. Consider cleaning up empty arrays:

    if overlapping.isEmpty {
        overlappingAnnotations.removeValue(forKey: .init(originalCoordinate))
    }

Performance Considerations

  1. Overlap re-calculation: The overlap logic re-calculates positions for ALL annotations in a group every time a new one is added. For large groups, this could be expensive. Consider only calculating once when the group is complete.

  2. Dictionary key generation: String(describing: type(of: data.object)) uses reflection which has runtime cost. For performance-critical paths, consider pre-computing type strings or using enum cases.

Code Robustness

  1. Force unwrapping risk: The overlap logic assumes originalCoordinate is always available, but this isn't validated. Consider adding guards for robustness.

  2. Visit status filtering complexity: The nested filtering in FilteredMapDataSource:83-154 is complex and could benefit from extraction into separate methods for better testability.

🧪 Test Coverage Concerns

The test plan shows only build verification is complete. Critical missing test scenarios:

  1. Unit tests for de-duplication logic:

    • Same object from multiple data sources
    • Edge case: empty data sources
    • Edge case: nil unique IDs
  2. Overlap calculation tests:

    • Multiple annotations at identical coordinates
    • Consistent ordering with same input
    • Memory cleanup after removal
  3. Integration tests:

    • Filter combinations (art + camps + favorites)
    • Zoom level behavior at 16+
    • Performance with large datasets

🔒 Security Assessment

No significant security concerns identified. The code:

  • ✅ Uses safe type casting with proper nil checks
  • ✅ No external input validation concerns
  • ✅ No sensitive data exposure in logging

📊 Performance Impact

Positive:

  • O(1) dictionary lookups vs O(n) array searches
  • Single-pass filtering reduces iteration overhead
  • Eliminated redundant de-duplication code

Watch points:

  • Dictionary memory growth (mitigated by cleanup in removeAnnotations)
  • Overlap recalculation for large groups
  • labelViews array unbounded growth

🎯 Recommendations

High Priority

  1. Fix the labelViews memory leak
  2. Add tolerance-based coordinate comparison
  3. Clean up empty overlap dictionary entries

Medium Priority

  1. Extract visit status filtering to separate methods
  2. Add comprehensive unit tests for core de-duplication logic
  3. Consider performance optimization for large overlap groups

Low Priority

  1. Pre-compute type strings for better performance
  2. Add logging for debugging duplicate detection
  3. Consider using WeakSet for annotation view tracking

📝 Overall Assessment

This is a well-architected solution that properly addresses the map pin duplication issue. The code quality is high with good Swift practices and clear separation of concerns. The main risks are around edge cases and memory management, but these are easily addressable. The performance improvements should be significant for users with multiple data sources enabled.

Recommendation: Approve with minor fixes for the memory leak and coordinate precision issues.


@chrisballinger
chrisballinger merged commit 944f39c into master Aug 21, 2025
1 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant