e2e/marker-factory.spec.js (1)

32-32: Same hard-coded date issue as other test files.

Consider using dynamic dates for better test reliability across different environments and time periods.

app/javascript/controllers/trips_controller.js (1)

10-11: Import path update LGTM, but keep quote style consistent

The new imports are correct and necessary for the refactor.
Minor style nit: surrounding imports in this file use double-quotes, while the two new lines use single-quotes. Aligning on one quote style keeps the file tidy.

-import { showFlashMessage } from '../maps/helpers';
-import { fetchAndDisplayPhotos } from '../maps/photos';
+import { showFlashMessage } from "../maps/helpers";
+import { fetchAndDisplayPhotos } from "../maps/photos";

e2e/live-map-handler.spec.js (1)

31-31: Consider using dynamic date ranges for better test reliability.

The hard-coded date range 2025-06-04 may become stale over time and could cause tests to fail when no data exists for that specific date.

Consider using dynamic date ranges based on the current date:

-    await page.goto('/map?start_at=2025-06-04T00:00&end_at=2025-06-04T23:59');
+    const today = new Date();
+    const startDate = today.toISOString().split('T')[0];
+    await page.goto(`/map?start_at=${startDate}T00:00&end_at=${startDate}T23:59`);

e2e/memory-leak-fix.spec.js (1)

33-33: Use dynamic date ranges for test reliability.

Same issue as the previous test file - hard-coded dates may cause test failures when no data exists for that specific date.

Apply the same dynamic date solution:

-    await page.goto('/map?start_at=2025-06-04T00:00&end_at=2025-06-04T23:59');
+    const today = new Date();
+    const startDate = today.toISOString().split('T')[0];
+    await page.goto(`/map?start_at=${startDate}T00:00&end_at=${startDate}T23:59`);

app/javascript/maps/markers.js (1)

32-36: Distance calculation is acceptable but could be improved.

The simplified distance calculation using Euclidean distance is a reasonable performance optimization for marker simplification. However, the TODO comment suggests haversine distance was considered.

The current implementation is adequate for marker filtering, but if more accurate distance calculation is needed in the future, consider implementing a simple haversine approximation:

-    // Note: haversineDistance function would need to be imported or implemented
-    // For now, using simple distance calculation
-    const latDiff = currLat - prevLat;
-    const lngDiff = currLon - prevLon;
-    const distance = Math.sqrt(latDiff * latDiff + lngDiff * lngDiff) * 111000; // Rough conversion to meters
+    // Simple haversine approximation for better accuracy
+    const toRad = (deg) => deg * (Math.PI / 180);
+    const dLat = toRad(currLat - prevLat);
+    const dLng = toRad(currLon - prevLon);
+    const a = Math.sin(dLat/2) * Math.sin(dLat/2) + 
+              Math.cos(toRad(prevLat)) * Math.cos(toRad(currLat)) * 
+              Math.sin(dLng/2) * Math.sin(dLng/2);
+    const distance = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1-a)) * 6371000; // Earth radius in meters

app/javascript/maps/live_map_handler.js (3)

36-36: Fix typo in property name

The property name fogLinethreshold appears to be a typo.

-    this.fogLinethreshold = options.fogLinethreshold || 10;
+    this.fogLineThreshold = options.fogLineThreshold || 10;

---

206-210: Make polyline styling configurable

The polyline color and weight are hardcoded, limiting customization options.

Add configuration options and use them:

+    this.polylineColor = options.polylineColor || '#3388ff';
+    this.polylineWeight = options.polylineWeight || 3;

       const newSegment = L.polyline([
         [prevPoint[0], prevPoint[1]],
         [newPoint[0], newPoint[1]]
       ], {
-        color: this.routeOpacity > 0 ? '#3388ff' : 'transparent',
-        weight: 3,
+        color: this.routeOpacity > 0 ? this.polylineColor : 'transparent',
+        weight: this.polylineWeight,
         opacity: this.routeOpacity
       });

---

221-227: Implement fog of war update logic

The fog of war update is currently just a placeholder with no actual implementation.

The method needs to implement the actual fog clearing logic. Would you like me to help implement this feature or create an issue to track it?

   _updateFogOfWar() {
     if (this.map.hasLayer(this.fogOverlay)) {
-      // This would need to be implemented based on the existing fog logic
-      // For now, we'll just log that it needs updating
-      console.log('LiveMapHandler: Fog of war update needed');
+      // TODO: Implement fog of war clearing logic
+      // - Clear fog around the new point using this.clearFogRadius
+      // - Update fog overlay visualization
+      console.warn('LiveMapHandler: Fog of war update not yet implemented');
     }
   }

app/javascript/maps/visits.js (3)

494-499: Improve user messaging and remove debug logging

The placeholder message could be more helpful, and debug console.log should be removed.

-      console.log('Drawer opened - showing placeholder message');
       // Just show a placeholder message in the drawer, don't fetch visits
       const container = document.getElementById('visits-list');
       if (container) {
-        container.innerHTML = '<p class="text-gray-500">Enable "Suggested Visits" or "Confirmed Visits" layers to see visits data</p>';
+        container.innerHTML = `
+          <div class="text-gray-500 text-center p-4">
+            <p class="mb-2">No visits data loaded</p>
+            <p class="text-sm">Enable "Suggested Visits" or "Confirmed Visits" layers from the map controls to view visits.</p>
+          </div>
+        `;
       }

---

540-541: Remove debug logging statements

Multiple console.log statements should be removed before merging to production.

Remove all the console.log statements or replace with a proper logging solution that can be toggled via configuration.

Also applies to: 556-557, 574-575, 578-581, 597-597

---

613-614: Remove excessive debug logging

The createMapCircles method has too many console.log statements that should be removed.

Remove all console.log statements or wrap them in a development-only check:

-      console.log('No visits to create circles for');
+      if (window.DEBUG) console.debug('No visits to create circles for');

Also applies to: 618-619, 622-624, 633-641, 658-659, 662-663, 668-668, 672-673

app/javascript/maps/marker_factory.js (2)

20-27: Add parameter validation for robustness

The function should validate inputs to prevent invalid CSS or negative sizes.

 export function createStandardIcon(color = 'blue', size = 8) {
+  // Validate size
+  if (size <= 0 || !Number.isFinite(size)) {
+    console.warn('Invalid marker size, using default');
+    size = 8;
+  }
+  
   return L.divIcon({
     className: 'custom-div-icon',
     html: `<div style='background-color: ${color}; width: ${size}px; height: ${size}px; border-radius: 50%;'></div>`,
     iconSize: [size, size],
     iconAnchor: [size / 2, size / 2]
   });
 }

---

231-240: Consider using constants for array indices

The marker data array uses hardcoded indices which reduces maintainability.

Define constants at the module level:

// At the top of the file
const MARKER_DATA_INDICES = {
  LATITUDE: 0,
  LONGITUDE: 1,
  BATTERY: 2,
  ALTITUDE: 3,
  TIMESTAMP: 4,
  VELOCITY: 5,
  ID: 6,
  COUNTRY: 7
};

// Then use them in the code:
const updatedMarkerData = [
  parseFloat(data.latitude),
  parseFloat(data.longitude),
  originalMarkerData[MARKER_DATA_INDICES.BATTERY],
  originalMarkerData[MARKER_DATA_INDICES.ALTITUDE],
  // etc...
];

e2e/map.spec.js (5)

191-191: Remove console.log statements from tests

Console logs should be avoided in test files. Use Playwright's test reporter features instead if debugging information is needed.

-        console.log(`Stats control displays: ${distance} ${unit} | ${points} points`);

---

591-591: Remove debugging console.log statements

Multiple console.log statements are present throughout the settings panel tests. These should be removed for production test suites.

Remove all console.log statements:

-      console.log(`Settings panel closed after submission: ${isPanelClosed}`);
-      console.log(`Fog settings panel closed after submission: ${isPanelClosed}`);
-      console.log('Panel stayed open after submission - testing persistence directly');
-      console.log('Fog panel stayed open after submission - testing persistence directly');
-      console.log(`Points rendering panel closed after submission: ${isPanelClosed}`);
-      console.log('Points panel stayed open after submission - testing persistence directly');

Also applies to: 675-675, 679-679, 865-865, 869-869

---

1432-1439: Performance test might be too lenient

A 15-second timeout for page load might be too generous and could hide performance regressions.

Consider:

1. Setting a more aggressive target (e.g., 5 seconds)
2. Adding performance metrics collection
3. Testing under different network conditions

     test('should load within reasonable time', async () => {
       const startTime = Date.now();
+      const metrics = [];

       await page.goto('/map');
       await page.waitForSelector('.leaflet-container', { timeout: 15000 });

       const loadTime = Date.now() - startTime;
-      expect(loadTime).toBeLessThan(15000); // Should load within 15 seconds
+      expect(loadTime).toBeLessThan(5000); // Should load within 5 seconds
+      
+      // Collect additional metrics
+      const performanceMetrics = await page.evaluate(() => 
+        JSON.stringify(performance.getEntriesByType('navigation')[0])
+      );
+      console.info('Performance metrics:', performanceMetrics);
     });

---

1667-1667: Remove console.log from error handling test

-      console.log(`Console errors detected: ${consoleErrors.length}`);
+      // Verify errors were detected and handled
+      expect(consoleErrors.length).toBeGreaterThan(0);

---

1-1671: Consider splitting this large test file for better maintainability

This test file is 1671 lines long, which makes it difficult to maintain and navigate. Consider splitting it into smaller, focused test files.

Split the test file by functionality:

• e2e/map/core-display.spec.js - Core map display tests
• e2e/map/navigation.spec.js - Date and time navigation tests
• e2e/map/layer-controls.spec.js - Layer control tests
• e2e/map/settings.spec.js - Settings panel tests
• e2e/map/calendar.spec.js - Calendar panel tests
• e2e/map/interactive-elements.spec.js - Interactive map elements
• e2e/map/performance.spec.js - Performance and loading tests
• e2e/map/accessibility.spec.js - Accessibility tests
• e2e/map/error-handling.spec.js - Error handling tests

Each file can share common setup through a base configuration or page object model pattern.

app/javascript/controllers/maps_controller.js (2)

122-124: Remove commented-out dead code

These commented lines are duplicates of the active code above and serve no purpose. Dead code should be removed to improve maintainability.

         let distance = parseInt(this.element.dataset.distance) || 0;
         const pointsNumber = this.element.dataset.points_number || '0';
-        // Original stats data loading disabled:
-        // let distance = parseInt(this.element.dataset.distance) || 0;
-        // const pointsNumber = this.element.dataset.points_number || '0';

---

422-476: Consider refactoring to reduce code duplication

The world borders fetching logic (lines 445-458) duplicates the same logic from setupScratchLayer (lines 372-385). Consider extracting this into a separate method to follow DRY principles.

+  async fetchWorldBordersData() {
+    if (this.worldBordersData) {
+      return this.worldBordersData;
+    }
+
+    const response = await fetch('/api/v1/countries/borders.json', {
+      headers: {
+        'Accept': 'application/geo+json,application/json'
+      }
+    });
+
+    if (!response.ok) {
+      throw new Error(`HTTP error! status: ${response.status}`);
+    }
+
+    this.worldBordersData = await response.json();
+    return this.worldBordersData;
+  }
+
   async refreshScratchLayer() {
     console.log('Refreshing scratch layer with current data');

     if (!this.scratchLayer) {
       console.log('Scratch layer not initialized, setting up');
       await this.setupScratchLayer(this.countryCodesMap);
       return;
     }

     try {
       // Clear existing data
       this.scratchLayer.clearLayers();

       // Get current visited countries based on current markers
       const visitedCountries = this.getVisitedCountries(this.countryCodesMap);
       console.log('Current visited countries:', visitedCountries);

       if (visitedCountries.length === 0) {
         console.log('No visited countries found');
         return;
       }

-      // Fetch country borders data (reuse if already loaded)
-      if (!this.worldBordersData) {
-        console.log('Loading world borders data');
-        const response = await fetch('/api/v1/countries/borders.json', {
-          headers: {
-            'Accept': 'application/geo+json,application/json'
-          }
-        });
-
-        if (!response.ok) {
-          throw new Error(`HTTP error! status: ${response.status}`);
-        }
-
-        this.worldBordersData = await response.json();
-      }
+      // Fetch country borders data
+      console.log('Loading world borders data');
+      const worldData = await this.fetchWorldBordersData();

       // Filter for visited countries
-      const filteredFeatures = this.worldBordersData.features.filter(feature =>
+      const filteredFeatures = worldData.features.filter(feature =>
         visitedCountries.includes(feature.properties["ISO3166-1-Alpha-2"])
       );

Then update setupScratchLayer to use the same method.

e2e/live-mode.spec.js (2)

407-415: Document network simulation requirements

The network simulation might fail in certain test environments. Consider adding a comment about the requirements or making this test conditional.

       // Test connection resilience by simulating various network conditions
+      // Note: Network simulation requires Playwright to be running with appropriate permissions
+      // and may not work in all CI environments (e.g., Docker containers without network capabilities)
       try {
         // Simulate brief network interruption
         await page.context().setOffline(true);

---

36-37: Consider parameterizing test dates

The tests use hardcoded future dates (2025-06-04) which will eventually become past dates. Consider using relative dates or parameterizing these values.

+  // Use a date far enough in the future to avoid conflicts with real data
+  const TEST_DATE = new Date();
+  TEST_DATE.setFullYear(TEST_DATE.getFullYear() + 10);
+  const TEST_DATE_STRING = TEST_DATE.toISOString().split('T')[0];
+
   test.beforeEach(async () => {
-    // Navigate to June 4, 2025 where we have test data
-    await page.goto('/map?start_at=2025-06-04T00:00&end_at=2025-06-04T23:59');
+    // Navigate to test date where we have test data
+    await page.goto(`/map?start_at=${TEST_DATE_STRING}T00:00&end_at=${TEST_DATE_STRING}T23:59`);

Also applies to: 226-226, 472-476

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

app/javascript/maps/visits.js (1)

585-600: Same layer control state checking issue remains unaddressed.

This implementation still accesses the private _layers property and uses fragile string comparisons, which was flagged in previous reviews. The suggested solution was to use map.hasLayer(this.visitCircles) and map.hasLayer(this.confirmedVisitCircles) instead of iterating through internal layer properties.

app/javascript/maps/live_map_handler.js (4)

17-23: Parameter validation still missing.

The constructor lacks validation for required parameters that was suggested in previous reviews. The recommended validation should check that map and layers parameters are provided and valid.

---

83-84: Point data validation and cloning still missing.

The point data is still assigned directly without validation or cloning, which was flagged in previous reviews. The recommended approach was to validate the data structure and clone the array to prevent mutations.

---

104-104: Zoom level still hardcoded.

The map zoom level remains hardcoded at 16, which was flagged in previous reviews. The suggested solution was to make this configurable via options.

---

233-245: Last marker should use factory and distinctive styling.

The _updateLastMarker method still creates markers directly instead of using the marker factory, and lacks distinctive styling to indicate it's the current position, both of which were flagged in previous reviews.

app/javascript/maps/marker_factory.js (2)

162-198: Polyline update performance issue remains unaddressed.

The nested iteration through all map layers during drag events still impacts performance, which was flagged in previous reviews. The suggested optimization was to cache polyline layers and use a more reasonable tolerance value.

---

267-270: Alert-based error notification still needs improvement.

The use of alert() for error notification still blocks the UI thread, which was flagged in previous reviews. The recommended approach was to use a non-blocking notification system.

app/javascript/maps/visits.js (1)

614-676: Good separation of concerns, but optimize logging and performance.

The extraction of map circle creation is excellent for maintainability. However, consider these improvements:

1. Conditional logging: Wrap console.log statements in development checks to avoid performance impact in production
2. Incremental updates: Consider whether clearing and recreating all circles is necessary, or if incremental updates would be more efficient for large datasets

-    console.log('fetchAndDisplayVisits called');
+    if (window.DEBUG || process.env.NODE_ENV === 'development') {
+      console.log('fetchAndDisplayVisits called');
+    }

Apply similar changes to other console.log statements throughout the method.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro