Mapping improvements

[jomosh]

No description provided.

[github-actions]

🤖 DeepSeek Code Review

Pull Request Review

Summary

This PR introduces significant enhancements to the ADS-B and radar visualization system:

• Increased rate limits for the API proxy (30→120 requests, with updated environment variable comments)
• Added altitude-based color coding for aircraft and detection points
• Implemented target labels with callsign and altitude information
• Added altitude unit toggle (metres/feet) with localStorage persistence
• Added altitude legend bar to the map UI
• Added settings popup with unit toggle
• Improved label cleanup for disappeared targets
• Reduced ADS-B polling interval from 1s to 2s to reduce API pressure

Code Quality and Best Practices

Issues Found:

1. Inconsistent variable naming (adsb.js): The variable icao was renamed to hex, but the comment in radar.js still refers to "ICAO hex" inconsistently.

2. Hardcoded values (main.js): The altitude color stops array contains magic numbers that should be extracted as constants for maintainability.

3. Missing error handling (adsb.js, radar.js): The _targetLabelFeatures operations assume the object exists without null checks.

4. Inline script in HTML (index.html): The settings popup button uses <script>document.write(...)</script> which is outdated and can cause issues with some browsers. Should use JavaScript to set the text after DOM load.

5. No trailing newline: Multiple files are missing trailing newlines at EOF.

Potential Bugs and Security Concerns

Issues Found:

1. Race condition in label updates (adsb.js, radar.js): Both scripts modify _targetLabelFeatures concurrently without synchronization. If event_adsb() and event_radar() run simultaneously, they could overwrite each other's labels.

2. Memory leak potential (adsb.js, radar.js): The seenHex tracking only removes labels for the current poll cycle. If a target disappears and reappears, its old label might persist if the timing is off.

3. XSS via flight data (adsb.js, radar.js): The flight variable from external data is used directly in label text without sanitization. If flight contains HTML/JavaScript, it could be rendered by the map library.

4. localStorage access without try-catch (main.js): While there's a try-catch for getItem, the setItem in toggleAltitudeUnit() also needs protection against private browsing mode restrictions.

5. Missing input validation (radar.js): The truth[hex].alt_baro value is assumed to be a number without validation before multiplication.

Suggestions for Improvement

1. Add input sanitization for flight/callsign data before using in labels:

function sanitizeLabel(text) {
  return String(text).replace(/[<>&"']/g, '');
}

2. Extract color stops to a constant:

const ALTITUDE_COLOR_STOPS = [
  { alt: 0, hue: 30 },
  // ... rest of stops
];

3. Add synchronization for label updates or use a single update cycle:

// Consider using a single update function that both scripts call
function updateLabels(sourceKey, hex, lat, lon, label, color) {
  // Use requestAnimationFrame or a queue
}

4. Fix the inline script in index.html by moving button text update to JavaScript:

// In main.js, after DOM ready
document.getElementById('btn-alt-unit').textContent = 'Unit: ' + (altUnit === 'm' ? 'metres' : 'feet');

5. Add validation for altitude values:

if (typeof alt_baro_ft !== 'number' || isNaN(alt_baro_ft)) {
  alt_m = 0;
}

Overall Verdict

Request Changes

The PR adds valuable functionality but has several issues that need addressing:

• Critical: XSS vulnerability via unsanitized flight data in labels
• High: Race condition potential between concurrent label updates
• Medium: Missing input validation for altitude values
• Low: Inline script usage, missing newlines, hardcoded values

Please address the security concerns (especially XSS) and the race condition before merging. The functional improvements are well-designed and the code structure is clean overall.

---

Powered by DeepSeek API

[github-actions]

🤖 DeepSeek Code Review

Summary

This pull request introduces significant enhancements to the ADS-B and radar visualization components of the map interface. Key changes include:

1. Rate limiting adjustments - Increased default rate limits in the API
2. ADS-B enhancements - Added target labels with callsign/altitude display, improved polling interval (2s), altitude color coding, and stale label cleanup
3. Radar visualization improvements - Similar label system with truth data integration, altitude-based coloring, and label pruning
4. Altitude legend - New color gradient legend bar at bottom of map
5. Settings UI - Added settings cog button with altitude unit toggle (meters/feet) with localStorage persistence
6. Label management system - New global label registry with source-keyed tracking and MapLibre integration

Code Quality and Best Practices

Issues Found:

1. Variable reassignment of const (adsb.js line 56):

   const alt_baro_ft = aircraftData.alt_baro;
   // Later:
   if (typeof alt_baro_ft !== 'number' || isNaN(alt_baro_ft)) {
     alt_baro_ft = 0;  // ERROR: Cannot reassign const
   }

   This will throw a runtime error. Should use let instead.

2. Missing newline at end of files - Both index.html and main.js are missing trailing newlines, which is a minor style inconsistency.

3. Inconsistent variable declarations - Mix of var, let, and const throughout the JavaScript files. Consider standardizing on const/let.

4. Magic numbers - The altitude color stops array contains many hardcoded values without explanatory comments about why these specific breakpoints were chosen.

Potential Bugs and Security Concerns

Issues Found:

1. Critical bug: const reassignment (mentioned above) - Will cause runtime errors when tar1090 returns non-numeric altitude values.

2. XSS vulnerability in label text - While sanitizeLabel() strips HTML special characters, it doesn't handle all potential injection vectors. The function only removes <>&"' but doesn't handle Unicode escapes or other encoding tricks. Consider using a more robust sanitization approach.

3. Race condition in label flushing - _flushTargetLabels() is called from multiple event loops without synchronization. If both ADS-B and radar polls complete simultaneously, one could overwrite the other's labels.

4. Potential infinite loop - If _flushTargetLabels() throws an error (e.g., source not found), the error isn't caught, which could break the polling loop.

5. localStorage access without fallback - While there's a try/catch for reading, the toggleAltitudeUnit() function writes to localStorage without error handling for private browsing modes that may throw.

Suggestions for Improvement

1. Fix the const bug in adsb.js by changing to let:

   let alt_baro_ft = aircraftData.alt_baro;

2. Add error boundaries around label operations:

   function _flushTargetLabels() {
     try {
       if (!mapLoaded) return;
       // ... rest of function
     } catch(e) {
       console.warn('Label flush failed:', e);
     }
   }

3. Improve sanitization - Consider using a whitelist approach or DOM-based sanitization:

   function sanitizeLabel(text) {
     const el = document.createElement('div');
     el.textContent = text;
     return el.textContent;
   }

4. Add loading state - The settings popup button text is initialized in map.on('load'), but the button exists in HTML before that. Consider initializing with a default value.

5. Document altitude color stops - Add comments explaining the rationale for the specific altitude breakpoints.

6. Consider debouncing for label updates to prevent excessive re-renders during rapid polling.

Overall Verdict

Request Changes

The critical const reassignment bug in adsb.js will cause runtime errors and must be fixed before merging. Additionally, the XSS sanitization should be strengthened. The rest of the changes are well-structured and add valuable functionality, but these issues need resolution first.

---

Powered by DeepSeek API

[github-actions]

🤖 DeepSeek Code Review

Pull Request Review

Summary

This PR introduces significant enhancements to the ADS-B and radar visualization components:

• Updated rate limiter defaults (increased limits)
• Added altitude-based color coding for aircraft/detection points
• Implemented target labels with callsign and altitude display
• Added settings UI for toggling between metric/imperial altitude units
• Added altitude legend bar
• Improved label management with pruning of stale labels
• Increased ADS-B polling interval from 1s to 2s

Code Quality and Best Practices

Issues Found:

1. Inconsistent variable naming (adsb.js): The variable icao was renamed to hex, but the comment still references dump1090/tar1090 - this is fine, but the variable name change should be consistent throughout.

2. Magic numbers (main.js): The altitude color stops array contains hardcoded values that are duplicated in the CSS legend gradient. If one is updated, the other must be updated too.

3. Missing error handling (adsb.js, radar.js): The sanitizeLabel function is used for display text, but there's no validation that the label text doesn't exceed reasonable length limits.

4. Global state management: The _targetLabelFeatures global object could grow unbounded if aircraft continuously appear and disappear without proper cleanup.

5. Inconsistent altitude handling: radar.js uses alt_baro from truth data (feet) while adsb.js uses it directly - the conversion to meters is done in both places but could be centralized.

Potential Bugs and Security Concerns

Issues Found:

1. XSS vulnerability (main.js - sanitizeLabel function):

function sanitizeLabel(text) {
  if (typeof text !== 'string') return '';
  return text.replace(/[<>&"']/g, '');
}

This regex only removes <, >, &, ", ' characters but doesn't handle other potentially dangerous characters. Since this data is used in MapLibre text-field expressions (not HTML), the risk is lower, but the function name implies HTML sanitization which is misleading.

2. Race condition (adsb.js, radar.js): The _targetLabelFeatures object is modified from multiple polling loops without any synchronization. If event_adsb() and event_radar() run concurrently, they could corrupt each other's label data.

3. Memory leak potential: The _targetLabelFeatures object stores features indefinitely. While removeTargetLabel is called for stale entries, there's no upper bound on the number of stored features.

4. Type confusion (adsb.js):

if (typeof alt_baro_ft !== 'number' || isNaN(alt_baro_ft)) {
  alt_baro_ft = 0;
}

This silently converts invalid altitude to 0, which could mask data quality issues.

Suggestions for Improvement

1. Centralize altitude conversion: Create a shared utility function for feet-to-meters conversion to avoid duplication between adsb.js and radar.js.

2. Improve sanitization: Either rename sanitizeLabel to something more accurate (like stripHtmlChars) or implement proper HTML sanitization if it's truly needed.

3. Add maximum label length: Implement a maximum character limit for label text to prevent UI overflow:

function sanitizeLabel(text, maxLength = 30) {
  if (typeof text !== 'string') return '';
  return text.replace(/[<>&"']/g, '').substring(0, maxLength);
}

4. Add memory management: Implement a maximum number of stored labels and evict oldest entries when exceeded.

5. Consider using a single source of truth: Instead of having both adsb.js and radar.js manage their own labels independently, consider a unified approach where altitude data is processed once and labels are generated centrally.

6. Add validation for altitude data: Log warnings when invalid altitude data is received rather than silently converting to 0.

7. Document the rate limiter change: The comment says RATE_LIMIT_MAX=240 but the code sets default to 120 - update the comment to match.

Overall Verdict

Approve with suggestions

The PR adds valuable functionality (altitude-based coloring, labels, unit switching) and improves the user experience. The code is generally well-structured with good comments. However, the XSS sanitization function should be improved, and the potential race condition between polling loops should be addressed in a follow-up PR. The memory management concerns are minor but worth noting for future maintenance.

---

Powered by DeepSeek API

[github-actions]

🤖 DeepSeek Code Review

Review Analysis

Summary

This pull request introduces significant enhancements to a real-time aircraft/radar tracking map interface:

• ADS-B tracking: Improved altitude handling (feet→metres conversion), label management, and reduced polling frequency (1s→2s)
• Radar tracking: Added altitude-based color coding, truth data integration, and label management
• UI improvements: Settings cog with altitude unit toggle (metres/feet), altitude color legend bar
• Rate limiting: Increased default rate limit from 30 to 120 requests per window
• Code quality: New utility functions (sanitizeLabel, formatAltitude, getAltitudeColor) and label management system

Code Quality and Best Practices

Issues Found:

1. Inconsistent variable naming (adsb.js):

   • alt_baro_ft is reassigned from const to let but still uses let with reassignment - should use let consistently
   • namePart is declared with var while modern code uses let/const

2. Magic numbers (main.js):

   • The altitude color stops array and legend breakpoints are duplicated (in getAltitudeColor() and updateLegendLabels())
   • 12000 appears as a magic number multiple times

3. Missing error handling (radar.js):

   • truth[hex].alt_baro access assumes truth[hex] exists without checking

4. Global namespace pollution (main.js):

   • _targetLabelFeatures, _flushTargetLabels, _rebuildAllLabels are global - could conflict with other scripts

5. Inconsistent comment style:

   • Mix of // and /* */ comments
   • Some functions have JSDoc-style comments, others don't

Potential Bugs and Security Concerns

Issues Found:

1. XSS vulnerability (main.js, sanitizeLabel):

   • The function only strips <>&"' characters but doesn't handle other HTML entities or Unicode-based attacks
   • Label text from external data (flight callsigns) is rendered via MapLibre's text-field which may interpret HTML

2. Race condition (adsb.js, radar.js):

   • Both scripts modify _targetLabelFeatures concurrently via setTimeout callbacks
   • No synchronization mechanism for shared state

3. Data validation gap (radar.js):

   • truth[hex].alt_baro is accessed without null-checking truth[hex]
   • Could throw TypeError if truth data is malformed

4. LocalStorage error handling (main.js):

   • localStorage.getItem and setItem are wrapped in try-catch, but removeItem is not used - could fail in private browsing mode

5. Infinite recursion risk (adsb.js):

   • event_adsb() calls itself via setTimeout - if an error occurs in updateAircraftPoints, the error is caught but the recursion continues, potentially causing memory leaks

Suggestions for Improvement

1. Extract altitude color configuration:

const ALTITUDE_STOPS = [
  { alt: 0,     hue: 30  },
  { alt: 150,   hue: 30  },
  // ... rest of stops
];
const MAX_ALTITUDE = 12000;

2. Add synchronization for shared state:

// Use a simple mutex pattern or queue for label updates
const labelUpdateQueue = [];
let labelUpdateInProgress = false;

3. Improve sanitizeLabel:

function sanitizeLabel(text, maxLength = 40) {
  if (typeof text !== 'string') return '';
  // Use DOMParser or textContent for proper sanitization
  const div = document.createElement('div');
  div.textContent = text;
  return div.textContent.substring(0, maxLength);
}

4. Add defensive checks in radar.js:

if (truth[hex] && truth[hex].alt_baro !== undefined && truth[hex].alt_baro !== null) {
  alt_m = truth[hex].alt_baro * 0.3048;
}

5. Consider using requestAnimationFrame instead of setTimeout for smoother UI updates

6. Add unit tests for getAltitudeColor, formatAltitude, and sanitizeLabel

Overall Verdict

Request Changes

The PR introduces valuable features but has several issues that need addressing:

• Critical: XSS vulnerability in sanitizeLabel function
• High: Race condition in shared label state management
• Medium: Missing null checks in radar.js, magic numbers, global namespace pollution
• Low: Inconsistent variable declarations, comment style

The changes should be approved after:

1. Fixing the XSS vulnerability
2. Adding synchronization for shared state
3. Adding proper null checks in radar.js
4. Extracting magic numbers into constants
5. Improving error handling for localStorage operations

---

Powered by DeepSeek API

[github-actions]

🤖 DeepSeek Code Review

Review Analysis

Summary

This pull request introduces significant enhancements to the ADS-B and radar visualization system:

• Rate limiting: Increased default rate limit from 30 to 120 requests per window
• ADS-B polling: Reduced from 1s to 2s interval with improved data processing
• Altitude color coding: New color gradient system for altitude visualization
• Target labels: Added call sign and altitude labels for both ADS-B and radar targets
• Settings UI: Added altitude unit toggle (meters/feet) with persistence
• Legend: Added altitude color legend bar
• Code quality: Added sanitization, label management, and cleanup functions

Code Quality and Best Practices

Issues Found:

1. Inconsistent variable naming (adsb.js):

   • Changed icao to hex but processAircraftData still uses icao in function signature comment
   • Mixed use of var and let throughout JavaScript files

2. Magic numbers (main.js):

   • Hardcoded altitude breakpoints array appears in both getAltitudeColor() and updateLegendLabels() without shared constant
   • Risk of divergence if one array is updated without the other

3. Missing error handling (radar.js):

   • truth[hex].alt_baro access assumes truth[hex] exists without null check
   • truth[hex].flight access has same issue

4. Redundant code (adsb.js):

   • seenHex tracking and label pruning logic is duplicated between adsb.js and radar.js

5. CSS specificity (index.html):

   • Inline styles in <span> elements within legend labels override CSS classes

Potential Bugs and Security Concerns

Issues Found:

1. XSS vulnerability (main.js):

   • sanitizeLabel() only strips <>&"' characters but doesn't handle other HTML entities or Unicode injection vectors
   • Label text is used in text-field property which MapLibre renders as text, but still risky if data contains special characters

2. Race condition (main.js):

   • _targetLabelFeatures is modified by both adsb.js and radar.js without synchronization
   • Concurrent poll loops could cause inconsistent state

3. Memory leak (adsb.js, radar.js):

   • _targetLabelFeatures grows unbounded if removeTargetLabel() fails to clean up stale entries
   • No maximum size limit on the registry

4. Type confusion (adsb.js):

   • alt_baro from tar1090 can be string "ground" - handled with warning, but conversion to 0 may mask real issues
   • seen_pos < 10 comparison works but seen_pos could be undefined

5. localStorage exception (main.js):

   • localStorage.getItem() wrapped in try/catch but setItem() also needs protection (partially done)

Suggestions for Improvement

1. Extract shared constants:

// main.js
const ALTITUDE_BREAKPOINTS = [0, 150, 300, 600, 1200, 1800, 2400, 3000, 6000, 9000, 12000];

2. Improve sanitization:

function sanitizeLabel(text, maxLength) {
  if (typeof text !== 'string') return '';
  // Use DOMParser or textContent for proper sanitization
  const div = document.createElement('div');
  div.textContent = text;
  return div.textContent.substring(0, maxLength || 40);
}

3. Add null checks in radar.js:

var truthEntry = truth[hex];
if (truthEntry) {
  if (truthEntry.alt_baro !== undefined && truthEntry.alt_baro !== null) {
    alt_m = truthEntry.alt_baro * 0.3048;
  }
  flight = truthEntry.flight || null;
}

4. Add registry size limit:

const MAX_LABEL_FEATURES = 500;
function updateTargetLabel(sourceKey, hex, lat, lon, label, color) {
  // ... existing code ...
  // Prune oldest entries if over limit
  var keys = Object.keys(_targetLabelFeatures);
  if (keys.length > MAX_LABEL_FEATURES) {
    delete _targetLabelFeatures[keys[0]];
  }
}

5. Consider using const for immutable variables in modern JavaScript code

Overall Verdict

Request Changes

The PR adds valuable functionality but has several issues that need addressing:

• Critical: XSS vulnerability in sanitizeLabel() should use proper DOM-based sanitization
• High: Race condition potential with shared state between concurrent poll loops
• Medium: Duplicated logic between adsb.js and radar.js for label management
• Medium: Missing null checks in radar.js for truth data access
• Low: Magic numbers and inconsistent variable declarations

The core architecture is sound, but these issues should be resolved before merging to ensure production reliability and security.

---

Powered by DeepSeek API

[github-actions]

🤖 DeepSeek Code Review

Review Analysis

Summary

This pull request introduces significant enhancements to the ADS-B and radar visualization components:

• Rate limiter tuning: Increases default rate limit from 30 to 120 requests per window
• ADS-B improvements: Adds altitude-based color coding, target labels, stale label cleanup, and proper feet-to-meters conversion
• Radar enhancements: Similar altitude coloring and labeling, plus truth data integration for flight/altitude lookup
• UI additions: Settings cog with altitude unit toggle (m/ft), altitude legend bar, and persistent unit preference via localStorage
• Code quality: New utility functions for sanitization, formatting, and label management

Code Quality and Best Practices

Issues Found:

1. Inconsistent variable naming: Mix of var, let, and const throughout the JavaScript files. The new code uses var while existing code uses const and let. Should maintain consistency.

2. Missing error handling for localStorage: While there's a try-catch for reading, the toggleAltitudeUnit() function's localStorage.setItem call lacks error handling for private browsing modes or storage quota issues.

3. Hardcoded magic numbers: The altitude color stops array and legend breakpoints are duplicated in getAltitudeColor() and updateLegendLabels(). This creates a maintenance burden if altitude ranges change.

4. Global namespace pollution: _targetLabelFeatures, _flushTargetLabels, _rebuildAllLabels are global. Consider namespacing or module pattern.

5. Missing JSDoc types: Functions like getAltitudeColor() lack parameter type annotations.

Potential Bugs and Security Concerns

Issues Found:

1. XSS vulnerability in label text: The sanitizeLabel() function only strips <>&"' characters but doesn't handle all HTML entities or Unicode-based injection vectors. Since labels are rendered via MapLibre's text-field property (not innerHTML), the risk is low, but the sanitization is incomplete.

2. Race condition in label flushing: _flushTargetLabels() is called synchronously after each updateTargetLabel() or removeTargetLabel() call. If multiple updates happen in rapid succession (e.g., many aircraft in one poll), this could cause performance issues or race conditions with the map source.

3. Missing null check for data.aircraft: In updateAircraftPoints(), the code accesses data.aircraft || [], but if data itself is null/undefined, this will throw. The existing code had the same issue, but it's worth noting.

4. Potential infinite loop: If event_adsb() or event_radar() throws before the .finally() block, the setTimeout might not execute, stopping polling. The existing code has this issue too.

Suggestions for Improvement

1. Extract altitude color configuration: Create a shared configuration object for altitude breakpoints and colors to avoid duplication between getAltitudeColor() and updateLegendLabels().

2. Batch label updates: Instead of calling _flushTargetLabels() after each individual label update, collect all changes and flush once per poll cycle.

3. Add input validation: Validate that data exists before accessing data.aircraft or data.detections_localised.

4. Improve sanitization: Use a more robust sanitization approach or rely on MapLibre's built-in text escaping rather than custom string manipulation.

5. Consider performance: The for...in loops with hasOwnProperty checks could be optimized with Object.keys() or Object.entries().

6. Add loading state: Consider showing a loading indicator during initial data fetch.

Overall Verdict

Request Changes

The PR introduces valuable features but has several issues that should be addressed:

• The XSS sanitization is incomplete and should be improved or removed (rely on MapLibre's built-in escaping)
• The race condition in label flushing could cause performance issues
• The duplicated altitude configuration creates maintenance risk
• Variable declaration inconsistency should be fixed

These are moderate issues that require changes before merging, but the overall direction and functionality are sound.

---

Powered by DeepSeek API

[github-actions]

🤖 DeepSeek Code Review

Review Analysis

Summary

This pull request introduces significant enhancements to the ADS-B and radar visualization system:

• Rate limiter tuning: Increased default rate limit from 30 to 120 requests per window
• ADS-B polling optimization: Reduced polling interval from 1s to 2s with improved documentation
• Altitude-based coloring: New color mapping system for altitude visualization
• Target labeling: Added call sign and altitude labels for both ADS-B and radar targets
• Settings UI: Added altitude unit toggle (meters/feet) with persistence
• Legend bar: Added altitude color legend with proportional positioning
• Code quality improvements: Better variable naming, data validation, and cleanup of stale labels

Code Quality and Best Practices

Issues Found:

1. Inconsistent comment style (adsb.js, radar.js):

   • Mix of // and /* */ comment styles
   • Some comments are overly verbose for simple operations

2. Magic numbers (main.js):

   • The 12000 max altitude is hardcoded in multiple places (legend, color stops, label positioning)
   • Should be a named constant

3. Redundant null checks (radar.js):

   if ((alt_m === null || alt_m === undefined) && points.length > 0) {

   Could be simplified to if (alt_m == null) which catches both null and undefined

4. Missing semicolons (adsb.js, radar.js):

   • Several statements lack semicolons (e.g., var seenHex = {})

5. Global namespace pollution (main.js):

   • _targetLabelFeatures is a global variable without clear ownership

Potential Bugs and Security Concerns

Issues Found:

1. XSS vulnerability (main.js):

   • sanitizeLabel() only strips <>&"' characters but doesn't handle other HTML entities or Unicode-based attacks
   • The label text is used in text-field which could potentially be exploited if MapLibre processes HTML

2. Race condition (adsb.js, radar.js):

   • Both event_adsb() and event_radar() modify _targetLabelFeatures concurrently
   • No locking mechanism for the shared global state

3. Memory leak (adsb.js):

   • seenHex object grows unbounded if aircraft data contains many unique hex values
   • Should be cleared or limited

4. Type coercion issue (adsb.js):

   if (typeof alt_baro_ft !== 'number' || isNaN(alt_baro_ft)) {

   • isNaN() coerces non-numeric strings to NaN, but typeof check already handles this
   • Redundant check

Suggestions for Improvement

1. Extract constants (main.js):

   const MAX_ALTITUDE_M = 12000;
   const ALTITUDE_STOPS = [...];

2. Add input validation (main.js):

   • Validate altUnit from localStorage against expected values
   • Add bounds checking for altitude values

3. Improve sanitization (main.js):

   function sanitizeLabel(text, maxLength) {
     if (typeof text !== 'string') return '';
     // Use DOMParser or textContent for proper sanitization
     const div = document.createElement('div');
     div.textContent = text;
     return div.textContent.substring(0, maxLength || 40);
   }

4. Add error boundaries (adsb.js, radar.js):

   • Wrap processAircraftData in try-catch to prevent one bad entry from breaking the entire poll

5. Optimize label updates (main.js):

   • Consider debouncing _flushTargetLabels() to avoid excessive re-renders

6. Documentation:

   • Add JSDoc comments to all new functions
   • Document the altitude color mapping algorithm

Overall Verdict

Request Changes

The PR introduces valuable features but has several issues that need addressing:

• Critical: XSS vulnerability in sanitizeLabel() - must be fixed before deployment
• High: Race condition in shared state between polling loops
• Medium: Magic numbers, inconsistent coding style, and potential memory leak

The core functionality is sound, but the security and concurrency issues require attention before this can be approved.

---

Powered by DeepSeek API

[github-actions]

🤖 DeepSeek Code Review

Review Analysis

Summary

This pull request introduces significant enhancements to the ADS-B and radar visualization system:

• Rate limiting: Increased default rate limit from 30 to 120 requests per window
• ADS-B polling: Reduced polling frequency from 1s to 2s with improved data processing
• Altitude color coding: Added color gradient mapping based on altitude (metres)
• Target labels: New label system showing callsign/hex and altitude for both ADS-B and radar targets
• Settings UI: Added settings cog with altitude unit toggle (metres/feet) with localStorage persistence
• Altitude legend: New visual legend bar showing the color-to-altitude mapping
• Label cleanup: Automatic removal of stale labels when targets disappear

Code Quality and Best Practices

Issues Found:

1. Inconsistent variable naming: Mix of var, let, and const throughout the JavaScript code. The new code uses var while existing code uses const/let. Should maintain consistency.

2. Missing semicolons: Several statements lack semicolons (e.g., _flushTargetLabels() calls, variable declarations). While JavaScript has ASI, explicit semicolons are a best practice.

3. Hardcoded magic numbers: The altitude color stops array contains hardcoded values (150, 300, 600, etc.) without explanation of why these specific breakpoints were chosen.

4. Global namespace pollution: _targetLabelFeatures, _flushTargetLabels, _rebuildAllLabels are global functions/variables. Consider namespacing or module pattern.

5. Error handling: The try/catch for localStorage is good, but the sanitizeLabel function silently removes HTML characters without logging - could mask data issues.

Potential Bugs and Security Concerns

Issues Found:

1. XSS vulnerability in label text: The sanitizeLabel function only removes <>&"' characters but doesn't prevent other injection vectors. The label text is used in text-field property which MapLibre renders as text, not HTML - this is safe in this context, but the function name suggests sanitization that isn't comprehensive.

2. Race condition in label updates: Both event_adsb() and event_radar() call _flushTargetLabels() independently. If both run simultaneously, one could overwrite the other's labels. The _targetLabelFeatures object is shared but not protected by a mutex.

3. Memory leak potential: _targetLabelFeatures grows unboundedly if removeTargetLabel isn't called for every stale target. The cleanup logic in both event handlers looks correct, but edge cases (e.g., browser tab backgrounded) could cause accumulation.

4. Altitude conversion precision: Using 0.3048 for feet-to-metres conversion is correct, but 3.28084 for metres-to-feet introduces slight rounding errors. Consider using a single constant: const FEET_PER_METRE = 3.280839895;

5. Missing null check in _flushTargetLabels: If map.getSource('target-labels') returns null (source not yet added), the function silently exits. This is fine during initialization but could mask configuration errors.

Suggestions for Improvement

1. Add altitude validation: The alt_baro field from tar1090 can be "ground" (string) - the code handles this with a warning, but consider also handling null and undefined explicitly.

2. Improve label positioning: Labels currently use text-offset: [0, -1.1] which may overlap with points at certain zoom levels. Consider using text-allow-overlap: false or dynamic offset based on zoom.

3. Add loading state: The settings popup and legend appear immediately, but data may not be loaded yet. Consider adding a loading indicator.

4. Optimize label updates: Instead of rebuilding all labels on every poll, consider incremental updates (add/remove only changed labels) for better performance with many targets.

5. Add unit tests: The altitude color mapping and label formatting functions are good candidates for unit tests.

6. Document the rate limit change: The comment says RATE_LIMIT_MAX=120 but the default is now 120. The comment should explain why this change was made (the 2s polling interval halves the request rate).

Overall Verdict

Approve with suggestions

The changes are well-structured and add significant value:

• ✅ Improved visual feedback with altitude-based coloring
• ✅ Better information density with target labels
• ✅ User-configurable altitude units with persistence
• ✅ Proper cleanup of stale data
• ✅ Reduced API pressure through optimized polling

The identified issues are minor and don't block the PR. The XSS concern is mitigated by MapLibre's text rendering, and the race condition is unlikely in practice given the single-threaded nature of JavaScript. I recommend addressing the suggestions in follow-up PRs rather than blocking this one.

---

Powered by DeepSeek API

[github-actions]

🤖 DeepSeek Code Review

Review: Pull Request Analysis

Summary

This PR introduces significant enhancements to the ADS-B and radar visualization system:

• Rate limiting: Increased default rate limit from 30 to 120 requests per window
• ADS-B polling: Reduced from 1s to 2s interval with improved data processing
• Altitude color coding: New color gradient system based on altitude (metres)
• Target labels: Added call sign and altitude labels for both ADS-B and radar targets
• Settings UI: Added altitude unit toggle (metres/feet) with localStorage persistence
• Legend: Added altitude color legend bar with proportional positioning
• Label management: Added pruning of stale labels for disappeared targets

Code Quality and Best Practices

Issues Found:

1. Inconsistent variable naming (adsb.js):

   • Changed icao to hex but processAircraftData still uses aircraftData parameter name inconsistently with the rest of the codebase

2. Magic numbers (main.js):

   • The altitude color stops array contains hardcoded values that should be constants
   • 12000 appears multiple times as a magic number for max altitude

3. Redundant null checks (radar.js):

   if ((alt_m === null || alt_m === undefined) && points.length > 0) {

   Could be simplified to if (alt_m == null && points.length > 0)

4. Missing error handling (adsb.js):

   • seenHex object is used without checking if ac.hex exists before adding to the set

5. CSS specificity (index.html):

   • Inline styles in HTML for legend labels could be moved to CSS classes

Potential Bugs and Security Concerns

Issues Found:

1. XSS vulnerability (main.js):

   function sanitizeLabel(text, maxLength) {
     var cleaned = text.replace(/[<>&"']/g, '');
     return cleaned.substring(0, maxLength);
   }

   This regex-based sanitization is insufficient. It doesn't handle all HTML entities or encoded characters. Consider using textContent or a proper sanitization library.

2. Race condition (adsb.js):

   • seenHex is built in updateAircraftPoints but labels are removed based on this set. If processAircraftData throws an error for one aircraft, the hex might not be added to seenHex, causing premature label removal.

3. Potential infinite loop (radar.js):

   • The finally block schedules setTimeout(event_radar, 1000) regardless of whether the fetch succeeded or failed. If the API consistently fails, this creates a tight error loop.

4. Type coercion issue (adsb.js):

   if (typeof alt_baro_ft !== 'number' || isNaN(alt_baro_ft)) {

   This check is good, but the subsequent assignment alt_baro_ft = 0 could mask real data issues.

Suggestions for Improvement

1. Security: Replace the custom sanitizeLabel function with proper DOM-based escaping:

   function sanitizeLabel(text) {
     const div = document.createElement('div');
     div.textContent = text;
     return div.textContent.substring(0, 40);
   }

2. Performance: Consider debouncing the _flushTargetLabels calls when multiple labels are updated in rapid succession.

3. Maintainability: Extract altitude color stops and max altitude to constants:

   const ALTITUDE_STOPS = [...];
   const MAX_ALTITUDE = 12000;

4. Error resilience: Add exponential backoff for failed API calls:

   let retryDelay = 1000;
   function event_adsb() {
     fetch(url)
       .then(...)
       .catch(error => {
         retryDelay = Math.min(retryDelay * 2, 30000);
       })
       .finally(() => {
         setTimeout(event_adsb, retryDelay);
         retryDelay = 1000; // reset on success
       });
   }

5. Documentation: Add JSDoc comments to the new functions in adsb.js and radar.js for consistency with main.js.

Overall Verdict

Request Changes

The PR introduces valuable features but has a security vulnerability (XSS via insufficient sanitization) and several code quality issues that should be addressed before merging. The core functionality is sound, but the sanitization function needs to be replaced with a proper implementation, and the race condition in label management should be fixed.

---

Powered by DeepSeek API