Skip to content

fix: map tileset selection not applying on dashboard#2948

Merged
Yeraze merged 1 commit intomainfrom
fix/map-tileset-not-applying
May 10, 2026
Merged

fix: map tileset selection not applying on dashboard#2948
Yeraze merged 1 commit intomainfrom
fix/map-tileset-not-applying

Conversation

@Yeraze
Copy link
Copy Markdown
Owner

@Yeraze Yeraze commented May 10, 2026

Summary

  • DashboardMap TileLayer missing key: react-leaflet kept the same Leaflet instance when the tile URL prop changed, so picking a new tileset in Settings never refreshed the dashboard map. Added key={tilesetId} to force a remount.
  • SettingsProvider baseUrl default '' broke save under BASE_URL deployments: GlobalSettingsPage, ReportsPage, MapAnalysisPage, PacketMonitorPage, and DashboardPage instantiate <SettingsProvider> without passing baseUrl, so POST /api/user/map-preferences hit the unprefixed path and 404'd. SettingsProvider now auto-detects the base from window.location when no prop is supplied; explicit prop callers (AppWithToast) are unaffected.

Test plan

  • Verified locally on dev container behind /meshmonitor BASE_URL — switching Map Tileset in Settings now succeeds (200) and the dashboard map updates immediately on next load.
  • CI build + tests
  • Smoke-test other pages mounted via <SettingsProvider> (Reports, MapAnalysis, PacketMonitor) for any regression in baseUrl detection.

🤖 Generated with Claude Code

Two bugs prevented the global Map Tileset setting from taking effect:

1. DashboardMap's <TileLayer> had no `key` prop tied to tilesetId, so
   react-leaflet kept the same Leaflet tile layer instance when the URL
   prop changed and never refreshed the tile source.

2. SettingsProvider defaulted baseUrl to '' when no prop was passed.
   Four pages (GlobalSettingsPage, ReportsPage, MapAnalysisPage,
   PacketMonitorPage, DashboardPage) instantiate the provider without
   threading baseUrl through, so POST /api/user/map-preferences hit
   the unprefixed path and 404'd under BASE_URL deployments. Now
   SettingsProvider auto-detects the base from window.location when
   no prop is supplied.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 10, 2026

Claude finished @Yeraze's task —— View job


Pull Request Review

Excellent work! This PR addresses two critical issues in the dashboard map functionality and BASE_URL deployments. The fixes are minimal, targeted, and well-implemented.

✅ Code Quality & Implementation

DashboardMap.tsx (Line 108):

  • Perfect fix: Adding key={tilesetId} to the TileLayer component is the correct React solution for forcing re-renders when props change
  • Clean approach: Leverages React's reconciliation algorithm rather than manual DOM manipulation
  • Follows React best practices: Using the key prop to control component lifecycle

SettingsContext.tsx (Lines 165-177, 180):

  • Smart auto-detection: The detectBaseUrlFromLocation() function correctly mirrors the logic in App.tsx and DashboardPage
  • Robust path parsing: Properly handles edge cases (empty path, single segment, etc.)
  • Maintains backward compatibility: Explicit baseUrl prop takes precedence over auto-detection
  • Comprehensive route mapping: Covers all app routes for accurate base path detection

✅ Bug Fixes & Correctness

Map Tileset Issue:

  • Root cause correctly identified: react-leaflet reuses Leaflet instances when only props change
  • Solution directly addresses the problem without side effects

BASE_URL Deployment Issue:

  • Correctly identified that SettingsProvider instantiated without baseUrl defaults to empty string
  • The fix ensures API calls hit the correct prefixed endpoints (/meshmonitor/api/...)

✅ Performance Considerations

  • Minimal performance impact: Key prop change only triggers re-render when tileset actually changes
  • No unnecessary re-renders: Auto-detection only runs during SettingsProvider initialization
  • Efficient path parsing: Simple string operations with no expensive computations

✅ Security Assessment

  • No security concerns: Changes are purely UI-related with no authentication, input validation, or data exposure issues
  • Safe string manipulation: Path parsing uses basic string methods without eval or dynamic code execution

⚠️ Minor Considerations

Test Coverage:

  • Consider adding a test to verify TileLayer re-renders when tilesetId changes
  • Could add a test for detectBaseUrlFromLocation() with various path scenarios
  • Integration test for BASE_URL deployments would catch future regressions

Code Suggestions:

  • The appRoutes array in detectBaseUrlFromLocation() could be extracted to a shared constant to avoid duplication across files
  • Consider adding JSDoc comments to document the auto-detection behavior

📋 Todo List:

  • Read and analyze DashboardMap.tsx changes
  • Read and analyze SettingsContext.tsx changes
  • Review code quality and implementation approach
  • Check for potential bugs and security concerns
  • Assess performance implications
  • Evaluate test coverage needs
  • Provide comprehensive review feedback

🎯 Overall Assessment

Excellent PR! Both fixes are:

  • ✅ Targeted and minimal
  • ✅ Address root causes effectively
  • ✅ Follow React/JavaScript best practices
  • ✅ Maintain backward compatibility
  • ✅ Have no negative performance or security implications

The implementation quality is high and the changes are ready for merge. The test plan verification confirms the fixes work as intended.


@Yeraze Yeraze merged commit c16b5a2 into main May 10, 2026
19 checks passed
@Yeraze Yeraze deleted the fix/map-tileset-not-applying branch May 10, 2026 02:27
Yeraze added a commit that referenced this pull request May 10, 2026
* chore(release): bump version to 4.3.0

Headline feature: Waypoints — per-source storage, map rendering,
and in-place authoring UI for Meshtastic WAYPOINT_APP pins.

Includes since 4.2.3:
- #2936/#2938 feat(waypoints): basic waypoint support
- #2942 feat(waypoints): authoring UI for create/edit/delete
- #2937 fix(auth): bootstrap first OIDC user as admin
- #2935 fix(traceroute): stop cascading IP-style across radio segments
- #2945 feat: Tile Selection / Legend visibility toggles in Map Features
- #2944 fix: correct channel encryption label for unencrypted/shorthand PSKs
- #2946 fix(mobile): mobile browser interface fixes
- #2948 fix: map tileset selection not applying on dashboard
- docs: new Waypoints feature page; 4.3 Highlights nav entry

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

* docs: list Waypoints under regular Features instead of 4.3 Highlights

Per Randall: Waypoints should be documented like any other feature, not
called out in the nav as a "🆕 4.3 Highlights" section.

- Drop the "🆕 4.3 Highlights" entry from the top nav and the /features/
  sidebar; restore "🆕" on 4.2 Highlights as the most-recent callout
- Add Waypoints to the regular Features sidebar list (next to Embed Maps)

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

* docs: drop version-Highlights nav sections, fold entries into Features

Removes the "🆕 4.2 / 4.1 / 4.0 Highlights" callouts from both the top
nav and the /features/ sidebar. Their pages — Analysis & Reports, Map
Analysis, Multi-Source, Per-Source Permissions, Global Settings, Store
& Forward, Geofence Triggers — now live in the regular Features list,
slotted near topically-related entries (Multi-Source / permissions
near Settings; Geofence Triggers next to Automation; Map Analysis next
to Embed Maps; Analysis & Reports next to Analytics; etc.).

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

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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