Add camp boundary and label map layers

[chrisballinger]

Summary

Adds two new toggleable map layers for camp visualization with zoom-based visibility controls.

Features

New Map Layers

• Camp Boundaries: Polyline outlines showing camp perimeters (visible at zoom 15+)
• Camp Labels (Big): Polyline-drawn letter shapes for camp names (visible at zoom 17+)

UI Controls

• New "Camp Display" section in Map Filter settings
• Toggles for both camp boundaries and camp names
• "Always" option to override zoom restrictions for boundaries
• Consistent with existing Art/Camps zoom toggle patterns

Technical Implementation

• Created MapLayerManager class to control runtime layer visibility
• Uses GeoJSON data from camp_outlines.geojson and camp_labels.geojson
• Dynamic minzoom control based on user settings
• Fixed delegate conflict that was breaking annotations

Changes

Core Implementation

• Added MapLayerManager for layer visibility control
• Updated MapFilterView with new Camp Display section
• Added UserSettings properties for layer preferences
• Integrated with MapLibre's style system

Bug Fixes

• Fixed critical annotation delegate conflict in BaseMapViewController
• Used callback pattern to avoid MapViewAdapter delegate override

Style Updates

• Added camp-boundaries and camp-labels-big layers to map styles
• Set appropriate colors for light/dark themes
• Configured zoom restrictions (minzoom: 15 for boundaries, 17 for labels)

Testing

• Build succeeds without errors
• Map filter UI displays new toggles
• Settings persist to UserDefaults
• Layers toggle visibility correctly
• Zoom restrictions work as expected
• Annotations still function properly

Files Modified

• iBurn/MapLayerManager.swift (new)
• iBurn/MapFilterView.swift
• iBurn/UserSettings.swift
• iBurn/BaseMapViewController.swift
• iBurn/MainMapViewController.swift
• iBurn/MapViewAdapter.swift
• Map style JSONs and GeoJSON data files

🤖 Generated with Claude Code

[claude]

Claude finished @chrisballinger's task —— View job

---

Code Review Complete

I've thoroughly reviewed this pull request that adds camp boundary and label map layers. Here's my comprehensive assessment:

Review Tasks

• Read repository guidelines (CLAUDE.md)
• Analyze new MapLayerManager architecture
• Review UI integration and settings changes
• Check delegate pattern implementation and potential conflicts
• Assess zoom-based visibility logic
• Review memory management and performance
• Check for security concerns
• Evaluate test coverage
• Provide comprehensive feedback

---

Overall Assessment: ✅ Good Implementation with Minor Suggestions

This PR successfully adds two new map layers with a clean, well-architected approach that follows the existing codebase patterns.

Code Quality Analysis

✅ Strengths

1. Clean Architecture: The new MapLayerManager class follows single responsibility principle and integrates well with the existing MapViewAdapter pattern.

2. Proper Delegate Pattern: The implementation correctly uses the callback pattern (onStyleLoaded) to avoid delegate conflicts, addressing the critical issue mentioned in the PR description at BaseMapViewController.swift:62-68.

3. Consistent UI Integration: The MapFilterView.swift changes follow existing patterns with proper SwiftUI bindings and state management at MapFilterView.swift:151-156.

4. Settings Persistence: UserDefaults integration follows established patterns with default values properly handled at UserSettings.swift:322-325 and UserSettings.swift:347-350.

⚠️ Areas for Improvement

1. Memory Management Concern:

   • Issue: MapLayerManager uses weak var mapView: MLNMapView? but there's potential for strong reference cycles
   • Location: MapLayerManager.swift:14
   • Suggestion: Consider making MapLayerManager a protocol to improve testability and reduce coupling

2. Error Handling:

   • Missing: No error handling if map layers don't exist in style
   • Location: MapLayerManager.swift:30, MapLayerManager.swift:46
   • Suggestion: Add logging when expected layers are missing to help with debugging

3. Zoom Logic Simplification:

   • Observation: The "Always" toggle logic could be clearer
   • Location: MapLayerManager.swift:34-42
   • Suggestion: Consider extracting zoom level constants

Performance Considerations

1. ✅ Good: Layers are only updated when needed through the callback pattern
2. ✅ Good: Settings changes trigger single update call (MainMapViewController.swift:117)
3. ⚠️ Monitor: Large GeoJSON files (48MB for labels) - performance should be monitored in production

Security Assessment

1. ✅ No Security Concerns: All user inputs go through proper UserDefaults, no external data sources
2. ✅ Good: Follows principle of least privilege with weak references

Test Coverage Gap

Major Gap: No unit tests for the new functionality

• No tests for MapLayerManager
• No tests for new UserSettings properties
• No integration tests for layer visibility logic

Recommendation: Add unit tests, particularly for:

// Example test structure needed
func testMapLayerManagerUpdatesCampBoundaries() { ... }
func testUserSettingsDefaultValues() { ... }
func testMapFilterViewModelSyncsWithSettings() { ... }

Technical Deep Dive

Zoom-Based Visibility Implementation

The implementation correctly modifies minimumZoomLevel at runtime (MapLayerManager.swift:37-40), which is the proper MapLibre approach. However, I notice the PR description mentions "visible at zoom 15+" but I don't see this implemented for the "Big Camp Names" layer - only boundaries get zoom restrictions.

Delegate Conflict Resolution

The callback approach (BaseMapViewController.swift:62-68) is excellent and avoids the annotation delegate issues mentioned in the PR. This is much cleaner than trying to chain delegates.

UI State Management

The SwiftUI integration properly uses @Published properties and didSet observers to maintain consistency between toggles (MapFilterView.swift:32-37).

Minor Code Style Notes

1. Good: Follows Swift naming conventions
2. Good: Proper documentation comments
3. Good: Consistent with existing codebase style
4. Minor: Consider extracting magic numbers (zoom levels 15, 17) to constants

Recommendations

1. High Priority: Add unit tests for new functionality
2. Medium Priority: Add error logging for missing map layers
3. Low Priority: Extract zoom level constants for maintainability
4. Consider: Monitor performance with large GeoJSON files in production

Final Verdict

This is a solid implementation that adds valuable functionality while maintaining code quality. The architecture decisions are sound, and the integration follows established patterns. The main gap is test coverage, which should be addressed before or shortly after merge.

Recommended Action: ✅ Approve after adding basic unit tests

---

[claude]

Claude finished @chrisballinger's task —— View job

---

Claude Code Review Complete

Code Review Tasks

• Read repository guidelines (CLAUDE.md)
• Analyze new MapLayerManager architecture
• Review UI integration and settings changes
• Check delegate pattern implementation and potential conflicts
• Assess zoom-based visibility logic
• Review memory management and performance
• Check for security concerns
• Evaluate test coverage
• Provide comprehensive feedback

---

Overall Assessment: ✅ Good Implementation with Minor Suggestions

This PR successfully adds two new map layers with a clean, well-architected approach that follows the existing codebase patterns.

Code Quality Analysis

✅ Strengths

1. Clean Architecture: The new MapLayerManager class follows single responsibility principle and integrates well with the existing MapViewAdapter pattern.

2. Proper Delegate Pattern: The implementation correctly uses the callback pattern (onStyleLoaded) to avoid delegate conflicts, addressing the critical issue mentioned in the PR description at BaseMapViewController.swift:62-68.

3. Consistent UI Integration: The MapFilterView.swift changes follow existing patterns with proper SwiftUI bindings and state management at MapFilterView.swift:151-156.

4. Settings Persistence: UserDefaults integration follows established patterns with default values properly handled at UserSettings.swift:322-325 and UserSettings.swift:347-350.

⚠️ Areas for Improvement

1. Memory Management Concern:

   • Issue: MapLayerManager uses weak var mapView: MLNMapView? but there's potential for strong reference cycles
   • Location: MapLayerManager.swift:14
   • Suggestion: Consider making MapLayerManager a protocol to improve testability and reduce coupling

2. Error Handling:

   • Missing: No error handling if map layers don't exist in style
   • Location: MapLayerManager.swift:30, MapLayerManager.swift:46
   • Suggestion: Add logging when expected layers are missing to help with debugging

3. Zoom Logic Simplification:

   • Observation: The "Always" toggle logic could be clearer
   • Location: MapLayerManager.swift:34-42
   • Suggestion: Consider extracting zoom level constants

Performance Considerations

1. ✅ Good: Layers are only updated when needed through the callback pattern
2. ✅ Good: Settings changes trigger single update call (MainMapViewController.swift:117)
3. ⚠️ Monitor: Large GeoJSON files (48MB for labels) - performance should be monitored in production

Security Assessment

1. ✅ No Security Concerns: All user inputs go through proper UserDefaults, no external data sources
2. ✅ Good: Follows principle of least privilege with weak references

Test Coverage Gap

Major Gap: No unit tests for the new functionality

• No tests for MapLayerManager
• No tests for new UserSettings properties
• No integration tests for layer visibility logic

Recommendation: Add unit tests, particularly for:

// Example test structure needed
func testMapLayerManagerUpdatesCampBoundaries() { ... }
func testUserSettingsDefaultValues() { ... }
func testMapFilterViewModelSyncsWithSettings() { ... }

Technical Deep Dive

Zoom-Based Visibility Implementation

The implementation correctly modifies minimumZoomLevel at runtime (MapLayerManager.swift:37-40), which is the proper MapLibre approach. However, I notice the PR description mentions "visible at zoom 15+" but I don't see this implemented for the "Big Camp Names" layer - only boundaries get zoom restrictions.

Delegate Conflict Resolution

The callback approach (BaseMapViewController.swift:62-68) is excellent and avoids the annotation delegate issues mentioned in the PR. This is much cleaner than trying to chain delegates.

UI State Management

The SwiftUI integration properly uses @Published properties and didSet observers to maintain consistency between toggles (MapFilterView.swift:32-37).

Minor Code Style Notes

1. Good: Follows Swift naming conventions
2. Good: Proper documentation comments
3. Good: Consistent with existing codebase style
4. Minor: Consider extracting magic numbers (zoom levels 15, 17) to constants

Recommendations

1. High Priority: Add unit tests for new functionality
2. Medium Priority: Add error logging for missing map layers
3. Low Priority: Extract zoom level constants for maintainability
4. Consider: Monitor performance with large GeoJSON files in production

Final Verdict

This is a solid implementation that adds valuable functionality while maintaining code quality. The architecture decisions are sound, and the integration follows established patterns. The main gap is test coverage, which should be addressed before or shortly after merge.

Recommended Action: ✅ Approve after adding basic unit tests

---

[claude]

Claude finished @chrisballinger's task —— View job

---

Code Review Complete

I've thoroughly reviewed this pull request that adds camp boundary and label map layers. Here's my comprehensive assessment:

Review Tasks

• Read repository guidelines (CLAUDE.md)
• Analyze new MapLayerManager architecture
• Review UI integration and settings changes
• Check delegate pattern implementation and potential conflicts
• Assess zoom-based visibility logic
• Review memory management and performance
• Check for security concerns
• Evaluate test coverage
• Provide comprehensive feedback

---

Overall Assessment: ✅ Good Implementation with Minor Suggestions

This PR successfully adds two new map layers with a clean, well-architected approach that follows the existing codebase patterns.

Code Quality Analysis

✅ Strengths

1. Clean Architecture: The new MapLayerManager class follows single responsibility principle and integrates well with the existing MapViewAdapter pattern.

2. Proper Delegate Pattern: The implementation correctly uses the callback pattern (onStyleLoaded) to avoid delegate conflicts, addressing the critical issue mentioned in the PR description at BaseMapViewController.swift:62-68.

3. Consistent UI Integration: The MapFilterView.swift changes follow existing patterns with proper SwiftUI bindings and state management at MapFilterView.swift:151-156.

4. Settings Persistence: UserDefaults integration follows established patterns with default values properly handled at UserSettings.swift:322-325 and UserSettings.swift:347-350.

⚠️ Areas for Improvement

1. Memory Management Concern:

   • Issue: MapLayerManager uses weak var mapView: MLNMapView? but there's potential for strong reference cycles
   • Location: MapLayerManager.swift:14
   • Suggestion: Consider making MapLayerManager a protocol to improve testability and reduce coupling

2. Error Handling:

   • Missing: No error handling if map layers don't exist in style
   • Location: MapLayerManager.swift:30, MapLayerManager.swift:46
   • Suggestion: Add logging when expected layers are missing to help with debugging

3. Zoom Logic Simplification:

   • Observation: The "Always" toggle logic could be clearer
   • Location: MapLayerManager.swift:34-42
   • Suggestion: Consider extracting zoom level constants

Performance Considerations

1. ✅ Good: Layers are only updated when needed through the callback pattern
2. ✅ Good: Settings changes trigger single update call (MainMapViewController.swift:117)
3. ⚠️ Monitor: Large GeoJSON files (48MB for labels) - performance should be monitored in production

Security Assessment

1. ✅ No Security Concerns: All user inputs go through proper UserDefaults, no external data sources
2. ✅ Good: Follows principle of least privilege with weak references

Test Coverage Gap

Major Gap: No unit tests for the new functionality

• No tests for MapLayerManager
• No tests for new UserSettings properties
• No integration tests for layer visibility logic

Recommendation: Add unit tests, particularly for:

// Example test structure needed
func testMapLayerManagerUpdatesCampBoundaries() { ... }
func testUserSettingsDefaultValues() { ... }
func testMapFilterViewModelSyncsWithSettings() { ... }

Technical Deep Dive

Zoom-Based Visibility Implementation

The implementation correctly modifies minimumZoomLevel at runtime (MapLayerManager.swift:37-40), which is the proper MapLibre approach. However, I notice the PR description mentions "visible at zoom 15+" but I don't see this implemented for the "Big Camp Names" layer - only boundaries get zoom restrictions.

Delegate Conflict Resolution

The callback approach (BaseMapViewController.swift:62-68) is excellent and avoids the annotation delegate issues mentioned in the PR. This is much cleaner than trying to chain delegates.

UI State Management

The SwiftUI integration properly uses @Published properties and didSet observers to maintain consistency between toggles (MapFilterView.swift:32-37).

Minor Code Style Notes

1. Good: Follows Swift naming conventions
2. Good: Proper documentation comments
3. Good: Consistent with existing codebase style
4. Minor: Consider extracting magic numbers (zoom levels 15, 17) to constants

Recommendations

1. High Priority: Add unit tests for new functionality
2. Medium Priority: Add error logging for missing map layers
3. Low Priority: Extract zoom level constants for maintainability
4. Consider: Monitor performance with large GeoJSON files in production

Final Verdict

This is a solid implementation that adds valuable functionality while maintaining code quality. The architecture decisions are sound, and the integration follows established patterns. The main gap is test coverage, which should be addressed before or shortly after merge.

Recommended Action: ✅ Approve after adding basic unit tests

---