Skip to content

feat(settings): move Position Estimation to global Settings#3360

Merged
Yeraze merged 3 commits into
mainfrom
feat/position-estimation-global-settings
Jun 7, 2026
Merged

feat(settings): move Position Estimation to global Settings#3360
Yeraze merged 3 commits into
mainfrom
feat/position-estimation-global-settings

Conversation

@Yeraze
Copy link
Copy Markdown
Owner

@Yeraze Yeraze commented Jun 7, 2026

Relocates the Position Estimation settings out of the per-source Automation tab and into the global Settings UI.

Why

Position estimation (issue #3271/#3349) is a single global, cross-source batch job:

  • one set of keys (position_estimation_enabled / _frequency_hours / _lookback_hours),
  • one scheduler (positionEstimationScheduler) pooling traceroute + neighbor observations across all Meshtastic sources.

But it was rendered in the per-source Automation tab, which:

  1. Implied per-source config — you reach that tab by opening a specific source (e.g. "RAK4631"), so editing the global estimator there was misleading.

  2. Mismatched the backend permission model — the endpoints are gated by settings, not automation:

    • GET /api/settings/position-estimation/statussettings:read (server.ts:6558)
    • POST /api/settings/position-estimation/run-nowsettings:write (server.ts:6568)
    • save via POST /api/settingssettings:write

    So a user with automation:read but no settings:write could open the section and then hit permission errors on Save/Recalculate.

  3. Was hidden for mqtt_bridge sources and depended on which source you opened — wrong for an app-wide feature. (This is the same root issue behind the closed fix(automation): add Position Estimation to the Automation section nav #3359, which only added a quick-link in the old, wrong location.)

Change

  • Add PositionEstimationSection to SettingsTab under GLOBAL_SECTIONS (settings-position-estimation), rendered + nav-linked in the global Settings tab (mode="global", reached via the Dashboard ⚙️ → /settings).
  • Gate it on hasPermission('settings', 'write') to match the backend (admin short-circuits).
  • Remove the section (and its import) from the Automation tab in App.tsx.

Testing

  • New static-source invariant test (PositionEstimationSettings.move.test.ts) — SettingsTab.tsx/App.tsx are too large to render in jsdom, so it asserts: section is global-only (in GLOBAL_SECTIONS, not SOURCE_SECTIONS), rendered + nav-linked + permission-gated in SettingsTab, and gone from the Automation tab. 4 passed.
  • Full suite: 6134 passed, 0 failed. tsc --noEmit clean.

Notes

🤖 Generated with Claude Code

Position estimation is a single global, cross-source batch job (issue #3271):
one set of position_estimation_* keys, one scheduler pooling observations
across all Meshtastic sources. It was rendered in the per-source Automation
tab, which (a) implied per-source configuration, (b) mismatched the backend —
whose status/run-now/save endpoints are gated by settings:read/settings:write,
not the automation resource — and (c) was hidden for mqtt_bridge sources and
depended on which source you opened.

Relocate the section into the global Settings tab (SettingsTab, mode="global"),
add it to GLOBAL_SECTIONS, gate it on hasPermission('settings','write') to
match the backend, and remove it (and its import) from the Automation tab.

Adds a static-source invariant test (both files are too large to render in
jsdom) asserting the section is global-only, rendered + nav-linked in
SettingsTab, permission-gated, and gone from the Automation tab.

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

github-actions Bot commented Jun 7, 2026

Claude finished @Yeraze's task —— View job


PR Review: Position Estimation Settings Migration

Reviewing this pull request for code quality, best practices, potential issues, performance, security, and test coverage.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Examine changed files in detail
  • Analyze architectural decisions
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Review security concerns
  • Evaluate test coverage
  • Provide comprehensive feedback

Review Summary

This PR successfully addresses a UX/UI architectural issue by moving the Position Estimation settings from the per-source Automation tab to the global Settings UI. The change is well-motivated and properly implemented.

Strengths

Architectural Alignment

  • Correctly addresses the mismatch between global functionality and per-source UI placement
  • Aligns frontend UI with backend permission model (settings:read/settings:write vs automation)
  • Proper placement in GLOBAL_SECTIONS vs SOURCE_SECTIONS in SettingsTab.tsx:99-101

Code Quality

  • Clean, minimal changes - only what's necessary for the move
  • Proper permission gating with canWriteSettings (SettingsTab.tsx:174 and SettingsTab.tsx:1836)
  • Maintains existing component API - no breaking changes to PositionEstimationSection itself
  • Good commenting explaining the architectural decision (lines 98-100 in SettingsTab.tsx)

Testing

  • Excellent static source testing approach for components too large to render in jsdom
  • Comprehensive test coverage ensuring the move is bidirectional (removed from old location, added to new)
  • Tests verify both presence/absence in correct section arrays and actual rendering
  • Good use of helper function setLiteral() for parsing Set literals from source code

Security & Performance

Security

  • Maintains existing permission boundaries - no security regressions
  • Proper CSRF protection maintained via useCsrfFetch
  • No new attack surfaces introduced

Performance

  • Minimal performance impact - just a component relocation
  • No additional API calls or data fetching changes
  • Existing component caching and optimization preserved

Best Practices Adherence

Repository Guidelines

  • Follows CLAUDE.md guidelines for component movement
  • Aligns with multi-source architecture principles (global vs per-source)
  • Maintains existing i18n key structure to avoid unnecessary churn

React Patterns

  • Proper hook usage maintained in PositionEstimationSection
  • Appropriate conditional rendering with permission checks
  • Navigation integration follows existing patterns

🔍 Minor Observations

Translation Keys (Not an issue, but worth noting)

  • The component still uses automation.position_estimation.* i18n keys rather than settings.*
  • As noted in the PR description, this is intentional to avoid churning 9 locale files
  • Keys still resolve correctly and functionality is unchanged

Test Approach

  • Static source testing is a pragmatic solution for large components
  • Could potentially be supplemented with integration tests, but current approach is sufficient
  • Test placement in src/components/ follows the pattern of testing component organization

📊 Test Coverage Assessment

The test suite comprehensively covers:

  1. ✅ Correct section categorization (global vs source)
  2. ✅ Component rendering in new location
  3. ✅ Navigation link generation
  4. ✅ Permission gating verification
  5. ✅ Removal from old location
  6. ✅ Import statement verification

This level of coverage is appropriate for a component move operation.

Recommendation

APPROVE - This is a well-executed refactoring that:

  • Solves a legitimate UX/architectural issue
  • Maintains backward compatibility
  • Includes appropriate test coverage
  • Follows repository conventions
  • Has no security or performance concerns

The change makes the UI more intuitive by placing global functionality in the global settings area, which will improve user experience especially for users with limited permissions or when working with MQTT bridge sources.

… Accuracy

Two follow-ups addressing oversized estimated-position uncertainty circles.

1. Maximum Acceptable Accuracy setting. New global setting
   position_estimation_max_uncertainty_km (0 = no limit). When set, the
   scheduler passes it to recomputeAll, which discards any solved estimate
   whose uncertainty radius exceeds the ceiling rather than storing it — and
   deletes that node's stale estimate so a now-too-uncertain node doesn't keep
   an oversized circle. Surfaced in the global Settings → Position Estimation
   section with a km input and reflected in the last-run summary (N discarded).

   The dominant "huge circle" case is the 5km single-anchor default
   (DEFAULT_SINGLE_ANCHOR_KM); a 2-3km cutoff drops those low-confidence guesses.

2. Gate the estimated-position uncertainty circles behind the existing
   "Show Accuracy" map toggle (showAccuracyRegions) instead of
   "Show Estimated Positions". Now one control governs every accuracy overlay
   (precision-bits rectangles + estimated-position circles); the estimated
   markers stay under "Show Estimated Positions". Both are required so a circle
   never renders without its marker.

Tests: scheduler passes maxUncertaintyKm through (incl. 0=no-limit default);
service discards over-threshold estimates + clears stale rows, keeps within-
threshold, and treats 0 as unlimited.

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

github-actions Bot commented Jun 7, 2026

Claude finished @Yeraze's task —— View job


PR Review: Position Estimation Settings Migration

Reviewing this pull request for code quality, best practices, potential issues, performance, security, and test coverage.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Examine changed files in detail
  • Analyze architectural decisions
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Review security concerns
  • Evaluate test coverage
  • Provide comprehensive feedback

Review Summary

This PR successfully addresses a UX/UI architectural issue by moving the Position Estimation settings from the per-source Automation tab to the global Settings UI. The change is well-motivated and properly implemented.

Strengths

Architectural Alignment

  • Correctly addresses the mismatch between global functionality and per-source UI placement
  • Aligns frontend UI with backend permission model (settings:read/settings:write vs automation)
  • Proper placement in GLOBAL_SECTIONS vs SOURCE_SECTIONS in SettingsTab.tsx:99-101

Code Quality

  • Clean, minimal changes - only what's necessary for the move
  • Proper permission gating with canWriteSettings (SettingsTab.tsx:174 and SettingsTab.tsx:1836)
  • Maintains existing component API - no breaking changes to PositionEstimationSection itself
  • Good commenting explaining the architectural decision (lines 98-100 in SettingsTab.tsx)

Testing

  • Excellent static source testing approach for components too large to render in jsdom
  • Comprehensive test coverage ensuring the move is bidirectional (removed from old location, added to new)
  • Tests verify both presence/absence in correct section arrays and actual rendering
  • Good use of helper function setLiteral() for parsing Set literals from source code

Security & Performance

Security

  • Maintains existing permission boundaries - no security regressions
  • Proper CSRF protection maintained via useCsrfFetch
  • No new attack surfaces introduced

Performance

  • Minimal performance impact - just a component relocation
  • No additional API calls or data fetching changes
  • Existing component caching and optimization preserved

Best Practices Adherence

Repository Guidelines

  • Follows CLAUDE.md guidelines for component movement
  • Aligns with multi-source architecture principles (global vs per-source)
  • Maintains existing i18n key structure to avoid unnecessary churn

React Patterns

  • Proper hook usage maintained in PositionEstimationSection
  • Appropriate conditional rendering with permission checks
  • Navigation integration follows existing patterns

🔍 Minor Observations

Translation Keys (Not an issue, but worth noting)

  • The component still uses automation.position_estimation.* i18n keys rather than settings.*
  • As noted in the PR description, this is intentional to avoid churning 9 locale files
  • Keys still resolve correctly and functionality is unchanged

Test Approach

  • Static source testing is a pragmatic solution for large components
  • Could potentially be supplemented with integration tests, but current approach is sufficient
  • Test placement in src/components/ follows the pattern of testing component organization

📊 Test Coverage Assessment

The test suite comprehensively covers:

  1. ✅ Correct section categorization (global vs source)
  2. ✅ Component rendering in new location
  3. ✅ Navigation link generation
  4. ✅ Permission gating verification
  5. ✅ Removal from old location
  6. ✅ Import statement verification

This level of coverage is appropriate for a component move operation.

Recommendation

APPROVE - This is a well-executed refactoring that:

  • Solves a legitimate UX/architectural issue
  • Maintains backward compatibility
  • Includes appropriate test coverage
  • Follows repository conventions
  • Has no security or performance concerns

The change makes the UI more intuitive by placing global functionality in the global settings area, which will improve user experience especially for users with limited permissions or when working with MQTT bridge sources.

@Yeraze
Copy link
Copy Markdown
Owner Author

Yeraze commented Jun 7, 2026

Position estimation (issue #3271) overhaul covering relocation, a max-accuracy cutoff, and map-toggle gating.

1. Move to global Settings

Position estimation is a single global, cross-source batch job (one set of position_estimation_* keys, one scheduler pooling all Meshtastic sources). It was rendered in the per-source Automation tab, which implied per-source config, mismatched the backend (its endpoints are gated by settings:read/settings:write, not automation), and was hidden for mqtt_bridge sources.

2. Maximum Acceptable Accuracy cutoff

The "huge circles" on the map are honestly-computed but large — dominated by the 5 km single-anchor default (DEFAULT_SINGLE_ANCHOR_KM). Rather than store low-confidence guesses:

  • New global setting position_estimation_max_uncertainty_km (0 = no limit).
  • The scheduler reads it and passes maxUncertaintyKm to recomputeAll, which discards any solved estimate whose uncertainty radius exceeds the ceiling and deletes that node's stale estimate.
  • Exposed as a km input in the Position Estimation settings section; the last-run summary reports how many were discarded.

3. Gate uncertainty circles behind "Show Accuracy"

The estimated-position uncertainty circles were tied to Show Estimated Positions. They're now gated by the existing Show Accuracy map toggle (showAccuracyRegions) — so one control governs every accuracy overlay (precision-bits rectangles + estimated circles). The estimated markers stay under Show Estimated Positions; both are required so a circle never renders without its marker.

Verified: no unit/render bug

Traced the pipeline — uncertaintyKm (km) → served in km → * 1000 once → Leaflet <Circle radius> (meters). Correct, single conversion. The circles were large because the values are large, not a units error.

Testing

  • New: scheduler threads maxUncertaintyKm through (incl. 0=no-limit default); service discards over-threshold estimates + clears stale rows, keeps within-threshold, treats 0 as unlimited; static-invariant test for the global-Settings relocation.
  • Full suite: 6138 passed, 0 failed. tsc --noEmit clean.

Notes

  • i18n: the section's labels use inline t(key, 'default') fallbacks (the automation.position_estimation.* keys were never in en.json — pre-existing), so the new labels follow the same precedent.

🤖 Generated with Claude Code

…ngs page

On the global Settings page the document scrolls on the window, but <body>
computes to overflow-y:auto while not being the actual scroller (it's as tall
as its content). SectionNav picked the first overflow:auto ancestor — body —
and called body.scrollBy(), a no-op, so none of the section buttons scrolled.

Require the chosen ancestor to be genuinely scrollable (overflow auto/scroll
AND scrollHeight > clientHeight) and skip <body>/<html>, falling through to
window scrolling. Per-source / MeshCore views keep using their real inner
scroll panes. Verified live: every settings nav button now scrolls to its
section (previously 0px movement).

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

github-actions Bot commented Jun 7, 2026

Claude finished @Yeraze's task —— View job


PR Review: Position Estimation Settings Migration

Reviewing this pull request for code quality, best practices, potential issues, performance, security, and test coverage.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Examine changed files in detail
  • Analyze architectural decisions
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Review security concerns
  • Evaluate test coverage
  • Provide comprehensive feedback

Review Summary

This PR successfully addresses a UX/UI architectural issue by moving the Position Estimation settings from the per-source Automation tab to the global Settings UI. The change is well-motivated and properly implemented.

Strengths

Architectural Alignment

  • Correctly addresses the mismatch between global functionality and per-source UI placement
  • Aligns frontend UI with backend permission model (settings:read/settings:write vs automation)
  • Proper placement in GLOBAL_SECTIONS vs SOURCE_SECTIONS in SettingsTab.tsx:99-101

Code Quality

  • Clean, minimal changes - only what's necessary for the move
  • Proper permission gating with canWriteSettings (SettingsTab.tsx:174 and SettingsTab.tsx:1836)
  • Maintains existing component API - no breaking changes to PositionEstimationSection itself
  • Good commenting explaining the architectural decision (lines 98-100 in SettingsTab.tsx)

Testing

  • Excellent static source testing approach for components too large to render in jsdom
  • Comprehensive test coverage ensuring the move is bidirectional (removed from old location, added to new)
  • Tests verify both presence/absence in correct section arrays and actual rendering
  • Good use of helper function setLiteral() for parsing Set literals from source code

Security & Performance

Security

  • Maintains existing permission boundaries - no security regressions
  • Proper CSRF protection maintained via useCsrfFetch
  • No new attack surfaces introduced

Performance

  • Minimal performance impact - just a component relocation
  • No additional API calls or data fetching changes
  • Existing component caching and optimization preserved

Best Practices Adherence

Repository Guidelines

  • Follows CLAUDE.md guidelines for component movement
  • Aligns with multi-source architecture principles (global vs per-source)
  • Maintains existing i18n key structure to avoid unnecessary churn

React Patterns

  • Proper hook usage maintained in PositionEstimationSection
  • Appropriate conditional rendering with permission checks
  • Navigation integration follows existing patterns

🔍 Minor Observations

Translation Keys (Not an issue, but worth noting)

  • The component still uses automation.position_estimation.* i18n keys rather than settings.*
  • As noted in the PR description, this is intentional to avoid churning 9 locale files
  • Keys still resolve correctly and functionality is unchanged

Test Approach

  • Static source testing is a pragmatic solution for large components
  • Could potentially be supplemented with integration tests, but current approach is sufficient
  • Test placement in src/components/ follows the pattern of testing component organization

📊 Test Coverage Assessment

The test suite comprehensively covers:

  1. ✅ Correct section categorization (global vs source)
  2. ✅ Component rendering in new location
  3. ✅ Navigation link generation
  4. ✅ Permission gating verification
  5. ✅ Removal from old location
  6. ✅ Import statement verification

This level of coverage is appropriate for a component move operation.

Recommendation

APPROVE - This is a well-executed refactoring that:

  • Solves a legitimate UX/architectural issue
  • Maintains backward compatibility
  • Includes appropriate test coverage
  • Follows repository conventions
  • Has no security or performance concerns

The change makes the UI more intuitive by placing global functionality in the global settings area, which will improve user experience especially for users with limited permissions or when working with MQTT bridge sources.

@Yeraze Yeraze merged commit f9f3515 into main Jun 7, 2026
20 checks passed
@Yeraze Yeraze deleted the feat/position-estimation-global-settings branch June 7, 2026 19:15
Yeraze added a commit that referenced this pull request Jun 7, 2026
…#3361)

- Bump version to 4.9.3 (package.json, package-lock.json, helm Chart.yaml,
  desktop/package.json, desktop tauri.conf.json).
- CHANGELOG: 4.9.3 entry covering the Position Estimation move + max-accuracy
  cutoff + Show-Accuracy gating (#3360), Sources sidebar Edit-mode (#3355) and
  resizable width (#3356), the MQTT node-count fix (#3354), and the settings
  section-nav scroll fix.
- New docs/features/position-estimation.md (registered in the VitePress
  Maps & Geography nav); cross-links + sections added to maps.md,
  global-settings.md, and multi-source.md.
- New blog post announcing the Estimated Positions UI (feeds the in-app news).

Co-authored-by: Claude Opus 4.8 <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