feat(map): Site Planner outbound coverage estimate — WKWebView bridge (#2058)#2072
Closed
garthvh wants to merge 14 commits into
Closed
feat(map): Site Planner outbound coverage estimate — WKWebView bridge (#2058)#2072garthvh wants to merge 14 commits into
garthvh wants to merge 14 commits into
Conversation
Feature spec for #2058 — the outbound half of the Site Planner round-trip (design#119). Mirrors the shipped Android reference implementation (Meshtastic-Android#6136).
Phase 0/1 planning: research.md resolves the native-bridge contract and query contract against actual upstream source (not the design issue's paraphrase, which was wrong about a bridge=1 flag). data-model.md, contracts/, quickstart.md, plan.md. Flags two open risks for tasks.md: Mac Catalyst headless-WKWebView JS execution, and live design-standards fetch per screen (Constitution VIII).
29 tasks across Setup, Foundational (shared params/RF math/URL builder/bridge/coordinator), and 3 user-story phases matching spec.md's P1/P2/P3. Flags the Mac Catalyst headless-WKWebView spike (T012) as the highest-uncertainty item — sequenced early in Foundational, not left for polish.
T001/T002. The v1.4 design standards doc was fetchable all along; the earlier empty-response confusion during planning was because _latest.md is a symlink whose raw GitHub content is just the target filename string, not the document. Recorded real, actionable findings in research.md #6 (Measurement-based unit display for height/range fields, plain-language subtext for advanced RF params, null-data suppression, touch-target/tooltip rules for the map control).
…romData T003/T004: CoverageEstimateParameters + validation rules (data-model.md), including the RadioClimate/Polarization/ColorScale enums with query-string values sourced from meshtastic-site-planner's own CLIMATE_CODES / POLARIZATION_CODES / COLORMAP_NAMES constants. T005/T006: LoRaRFHelpers — receiver sensitivity per modem preset, sourced from Meshtastic's published radio-settings link-budget table (the six 2.8-only presets map onto config.proto's own "comparable link budget to X" documentation, not guessed); region max transmit power for the txPower==0 sentinel, using real FCC/ETSI figures for the common regions and falling back to the Site Planner's own factory default (0.1W) elsewhere rather than inventing unverified regional limits. T009/T010: MapDataManager.importFromData(_:suggestedName:), mirroring importFromRemote's temp-file pattern exactly; tests added to the existing MapDataManagerTests.swift alongside the importFromRemote suite (matching existing file organization, not a new file as tasks.md originally assumed). All new files registered in project.pbxproj (verified via .o object files in derived data, not just build exit code). 24 tests passing, full build green. Remaining in Phase 2: T007/T008 (query URL builder), T011 (WKWebView bridge), T012 (Mac Catalyst spike), T013 (coordinator) — all UI-adjacent or requiring a real device/simulator spike, not pure logic like this batch.
Builds the flat query URL per contracts/query-contract.md: all 16 required keys always present, 6 optional keys omitted when unset (letting the planner's own defaults apply), run=1 always appended, no bridge=1 (confirmed absent from the real shipped contract). Locale-independent number formatting for query values, distinct from the locale-aware display formatting the form itself will need (Design Standards §10, research.md §6) -- these are two different concerns. 12 tests passing.
Hidden WKWebView + WKUserScript shim (defines window.__meshtasticNative .onCoverage, forwards to a WKScriptMessageHandler) + the handler decoding the received GeoJSON string, exactly per contracts/native-bridge-contract.md. Attached-but-invisible to the key window from the start (1x1, alpha 0), not fully detached -- the safer default per research.md's Catalyst risk note, not the version T012 was meant to falsify. T012 spike: added CoverageEstimateBridgeLiveSpikeTests, a genuine integration test against the live site.meshtastic.org (not a mock). CONFIRMED on iOS Simulator: real end-to-end success in 6.3s (navigation, JS execution, simulation, callback, valid FeatureCollection parsed). Mac Catalyst could not be verified on this machine -- two environment issues (test target's 26.5 deployment target vs this host's 26.4.1; a manual open-launched spike never presented a window) blocked the attempt, neither of which is evidence the WebKit mechanism itself is broken. Recorded honestly in research.md #5 as inconclusive, not as either a pass or a fail, with a concrete recommendation (re-run via Xcode's Test navigator) rather than guessing.
Singleton state machine (idle/running/succeeded/failed/canceled) enforcing the one-in-flight rule (FR-007) and cancellation (FR-008) at the source, routing a successful bridge result into MapDataManager.importFromData. Added a run-token guard: cancel() resolves asynchronously (the task must unwind before state settles to .canceled), so without a token a stale completion from a superseded run could clobber whatever runs next. finish() only applies if its captured token still matches current -- a real correctness fix, not just test scaffolding, surfaced while writing the test suite's reset() helper. 6 new tests (all pure state-machine, no network dependency -- start() transitions to .running synchronously before the bridge's async work begins). Foundation phase (Phase 2) is now complete: 46 tests across 6 suites, full build green.
T014: "Estimate Coverage" action added to NodeDetail's Actions section, gated by the same `if let latestPosition` block as NavigateToButton (FR-001) -- position-only nodes don't show a confusing disabled button, they just don't show the action at all. T015: CoverageEstimateForm -- Site/Transmitter, Receiver, Simulation, and Display sections (Environment/advanced deferred to US3 per plan). Prefill correctness fix: uses the *connected* radio's LoRaConfig for transmit characteristics (frequency via the existing LoRaChannelCalculator, power/sensitivity via LoRaRFHelpers), not the *viewed* node's own self-reported config -- FR-003 asks "if my radio broadcast from this site," not "if a radio like the remote one did." Design Standards Sec.10 (research.md Sec.6): antenna/receiver height and max range are edited via a locale-aware Measurement conversion (metric stored, feet/miles or meters/km displayed per Locale.measurementSystem) -- verified visually via snapshot, not just claimed: 2m prefill renders as "6.6 ft", 30km as "18.6 mi" on this (US-locale) test run. Measurement<UnitLength>.FormatStyle isn't ParseableFormatStyle, so this required a manual Binding<Double> conversion, not the format-style shortcut, discovered via a real build failure. T016/T017: form submission wired through CoverageEstimateCoordinator with in-progress/cancel/success/failure UI states. T018: two new snapshot suites, visually reviewed before committing, deleted and re-recorded on a clean run per the constitution's snapshot rule. T019: checked against the live Design Standards doc (research.md Sec.6) -- Sec.10 units (done above), Sec.6 plain-language subtext (added to the Receiver section's sensitivity footer), Sec.3 null-data suppression (the button itself is gated on a real position; the no-radio fallback uses the Site Planner's own defaults, never a raw 0). 45 tests passing across 8 suites, full build green.
T020: MapEstimateCoverageButton added to MeshMapMK's toolbar, reusing CoverageEstimateForm from US1 unchanged (FR-002) -- not tied to any node. Position prefill falls back through three tiers: current map view center, then the connected radio's own position, then a fixed default -- so the form never opens with an unset (0,0) position, unlike what a naive single-fallback would risk. T021: the no-radio-connected fallback (FR-003) reuses the exact same optional-chaining pattern already covered for US1 -- connectedNode?. loRaConfig being nil falls through to LoRaRFHelpers' sourced defaults, verified by code inspection rather than a duplicate test, since it's the identical code path US1 already exercises. T022: snapshot test added, visually reviewed (plain icon-only button, matches the map-toolbar's icon style), deleted and re-recorded clean before committing. 46 tests passing across 9 suites, full build green. Both US1 and US2 are independently functional now.
T023: Environment fields (radio_climate, polarization, clutter_height,
ground_dielectric, ground_conductivity, atmosphere_bending) and the
Simulation section's advanced statistical params (situation_fraction,
time_fraction) added behind a single "Advanced" DisclosureGroup, so
basic use (US1/US2) is visually unaffected. Enum pickers use the
RadioClimate/Polarization types already defined in
CoverageEstimateParameters.swift (sourced from meshtastic-site-planner's
CLIMATE_CODES/POLARIZATION_CODES, not invented). Each field carries
plain-language subtext per Design Standards Sec.6 -- this is denser RF/ITM
jargon than anything in the base sections.
T024: the color_scale picker + min/max dBm + overlay transparency were
already built as part of T015's Display section (that section's scope
always included Display in full) -- nothing new to add; verified via
the existing coverageEstimateForm_default snapshot, which already shows
the palette picker.
T025: added a snapshot test with the advanced section expanded. Caught
and fixed a real UX bug via the visual check itself: optional fields
(Ground Dielectric, etc.) were showing their own row label as a
redundant placeholder when empty ("Ground Dielectric" / "Ground
Dielectric"). Fixed with an explicit `prompt: Text("Default")` on each
optional TextField, distinct from its accessibility-label title --
re-verified visually before committing.
Also added an `initiallyShowAdvanced` init parameter to
CoverageEstimateForm purely for this test's benefit -- a small,
justified addition, not a broader testability refactor.
47 tests passing across 9 suites, full build green. All three user
stories (US1/US2/US3) are now independently functional.
One-line entry per the constitution's documentation workflow (no RELEASENOTES.md). Rebuilt in-app HTML via scripts/build-docs.sh, which also caught up settings.html/.md to a pre-existing, unrelated drift (a "Mesh Beacon" module row already in docs/user/settings.md but not yet propagated to the generated bundle) -- legitimate sync, not introduced by this change. T028 (full suite): ran the complete MeshtasticTests suite (2237 tests). 20 pre-existing failures (NodeListItemCompactSnapshotTests, NodeListItemSnapshotTests, IntervalConfigurationDetailedTests) -- verified none of this feature's commits ever touch those files, and their reference snapshots date to a 2024/2025-era commit (#1798), unrelated to spec 015. Reverted the working tree's incidental re-recordings of those (and other) unrelated snapshots rather than silently overwriting baselines for tests this feature doesn't own. The T012 live bridge spike passed again during this run (real network round trip, 193KB received) -- consistent, repeatable confirmation on iOS Simulator.
Updated plan.md's Constitution Check table to reflect verified (not assumed) status: VI (lint) and VIII (design standards) upgraded from "will be enforced"/"deferred" to "verified" with concrete evidence. VII (Mac Catalyst) stays honestly open -- environment-blocked, not a known defect, with a clear next step. T026 marked partial, not done: automated tests cover happy-path, concurrency, and no-radio-fallback, but cancellation, forced failure, end-to-end palette verification, and the Mac Catalyst repeat all need an actual interactive pass that no test suite substitutes for.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds an in-app Site Planner RF coverage estimator with parameter validation, radio-based prefilling, hidden WebKit execution, GeoJSON overlay import, coordinator state management, node/map entry points, tests, project wiring, specifications, and documentation. ChangesSite Planner outbound coverage estimate
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CoverageEstimateForm
participant CoverageEstimateCoordinator
participant HiddenWKWebView
participant SitePlanner
participant MapDataManager
User->>CoverageEstimateForm: Enter parameters and tap Run
CoverageEstimateForm->>CoverageEstimateCoordinator: Start estimate
CoverageEstimateCoordinator->>HiddenWKWebView: Load generated query URL
HiddenWKWebView->>SitePlanner: Execute coverage simulation
SitePlanner->>HiddenWKWebView: Send FeatureCollection JSON
HiddenWKWebView-->>CoverageEstimateCoordinator: Return JSON Data
CoverageEstimateCoordinator->>MapDataManager: Import GeoJSON Data
MapDataManager-->>CoverageEstimateForm: Publish overlay result
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
Meshtastic/Helpers/MapDataManager.swift-150-153 (1)
150-153: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the temporary directory, not only the file.
Each import creates a UUID directory, but the
deferremoves onlytempURL, leaving an empty directory behind. If the write fails beforedeferis registered, cleanup is also skipped. Register cleanup before directory creation and remove the UUID directory itself.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Meshtastic/Helpers/MapDataManager.swift` around lines 150 - 153, Update the temporary-file cleanup in the import flow around tempURL so cleanup is registered before directory creation, ensuring it also runs when directory creation or writing fails. Track the UUID directory separately and have the defer remove that directory rather than only the file at tempURL.Meshtastic/Helpers/SitePlanner/LoRaRFHelpers.swift-1-11 (1)
1-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required file-level marker.
This header is neither
// MARK: LoRaRFHelpersnor a copyright comment.As per coding guidelines, Swift files must include
// MARK: FileNameor a file-level copyright comment at the top.Suggested fix
+// MARK: LoRaRFHelpers // // LoRaRFHelpers.swift🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Meshtastic/Helpers/SitePlanner/LoRaRFHelpers.swift` around lines 1 - 11, Add a file-level marker at the top of LoRaRFHelpers.swift using the required `// MARK: LoRaRFHelpers` form, while preserving the existing header comments and imports.Source: Coding guidelines
Meshtastic/Helpers/SitePlanner/CoverageEstimateParameters.swift-1-12 (1)
1-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required file-level marker.
This header is neither
// MARK: CoverageEstimateParametersnor a copyright comment.As per coding guidelines, Swift files must include
// MARK: FileNameor a file-level copyright comment at the top.Suggested fix
+// MARK: CoverageEstimateParameters // // CoverageEstimateParameters.swift🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Meshtastic/Helpers/SitePlanner/CoverageEstimateParameters.swift` around lines 1 - 12, Add the required file-level marker to CoverageEstimateParameters.swift, using the exact filename-based form “// MARK: CoverageEstimateParameters” near the top of the file before the existing imports.Source: Coding guidelines
Meshtastic/Helpers/SitePlanner/CoverageQueryURLBuilder.swift-1-10 (1)
1-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required file-level marker.
This header is neither
// MARK: CoverageQueryURLBuildernor a copyright comment.As per coding guidelines, Swift files must include
// MARK: FileNameor a file-level copyright comment at the top.Suggested fix
+// MARK: CoverageQueryURLBuilder // // CoverageQueryURLBuilder.swift🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Meshtastic/Helpers/SitePlanner/CoverageQueryURLBuilder.swift` around lines 1 - 10, Add the required file-level marker at the top of CoverageQueryURLBuilder.swift, using the filename-based form “// MARK: CoverageQueryURLBuilder” before the existing header comments or imports.Source: Coding guidelines
specs/015-site-planner-outbound/spec.md-56-64 (1)
56-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSpecify the overlay name-collision behavior.
The edge case is listed, but the specification does not say whether the app should append a suffix, prompt the user, replace the existing overlay, or reject the import. Define the expected behavior before treating the requirements as complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/015-site-planner-outbound/spec.md` around lines 56 - 64, Specify the expected behavior when a new estimate’s site name matches an existing overlay name in the Edge Cases section, choosing and documenting whether to suffix, prompt, replace, or reject the import. Ensure the requirement clearly defines the user-visible outcome and preserves existing overlays as appropriate.specs/015-site-planner-outbound/quickstart.md-13-16 (1)
13-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the test-suite selectors.
The command selects
LoRaSensitivityTests, but the planned test file isLoRaRFHelpersTests.swift; it also omitsCoverageQueryURLBuilderTests. As written, this validation command can fail or leave URL-builder coverage untested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/015-site-planner-outbound/quickstart.md` around lines 13 - 16, Update the xcodebuild test selectors in the quickstart command: replace LoRaSensitivityTests with LoRaRFHelpersTests and add CoverageQueryURLBuilderTests, while preserving the existing CoverageEstimateParametersTests and MapDataManagerImportFromDataTests selectors.specs/015-site-planner-outbound/checklists/requirements.md-9-12 (1)
9-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the checklist’s implementation-detail assertions.
spec.mdcontains implementation details in its assumptions, including the embedded web engine, absence of a server API, and result-retrieval mechanism. The two checked “no implementation details” items are therefore inaccurate; either remove those details from the specification or uncheck/update these checklist entries.Also applies to: 25-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/015-site-planner-outbound/checklists/requirements.md` around lines 9 - 12, Correct the checked “No implementation details” assertions in the requirements checklist to reflect the implementation details documented in spec.md, including the embedded web engine, server API absence, and result-retrieval mechanism. Either remove those details from the specification or uncheck/update the affected checklist entries, while preserving accurate checklist items.specs/015-site-planner-outbound/data-model.md-5-15 (1)
5-15: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCorrect the persistence classification.
MapDataMetadatais stored through the existing file-basedMapDataManagerpipeline, not SwiftData. The heading currently conflicts withplan.mdLine 14 andresearch.mdLines 74-78 and could cause future work to introduce an incorrect persistence layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/015-site-planner-outbound/data-model.md` around lines 5 - 15, Correct the persistence heading for MapDataMetadata to distinguish it from SwiftData and identify it as persisted through the existing file-based MapDataManager pipeline. Keep the existing MapDataMetadata description and its importFromData behavior unchanged, and align the classification with the persistence model documented in plan.md and research.md.specs/015-site-planner-outbound/quickstart.md-5-5 (1)
5-5: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a resolvable design-standards URL.
research.mdconfirms that the raw_latest.mdpath returns the symlink target name instead of the document contents. Point this prerequisite at the resolved versioned document or document the required symlink-resolution step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/015-site-planner-outbound/quickstart.md` at line 5, Update the Meshtastic Client Design Standards prerequisite link in the quickstart so it points directly to a resolvable versioned document containing the standards, or explicitly require resolving the current symlink before review. Preserve the requirement to fetch and review the standards freshly before UI work.specs/015-site-planner-outbound/data-model.md-22-26 (1)
22-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the parameter contract with the implementation.
radioClimateandpolarizationare documented as optional here, butCoverageEstimateParameters.swiftdefines them as nonoptional fields with default values. Document the concrete model accurately, including the default behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/015-site-planner-outbound/data-model.md` around lines 22 - 26, Update the Environment parameter contract to mark radioClimate and polarization as required values rather than optional, and document their concrete default behavior from CoverageEstimateParameters.swift. Preserve the existing advanced grouping and all other field definitions.MeshtasticTests/MapDataManagerTests.swift-119-136 (1)
119-136: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAwait and verify imported-file cleanup.
defer { Task { ... } }lets the test finish before deletion completes, whiletry?hides cleanup failures. This can leave stale files in shared MapDataManager storage and contaminate later runs.Based on learnings, estimate results use the existing file-based MapDataManager storage, so cleanup must be awaited and verified.
Proposed fix
- defer { Task { await cleanUp(manager, metadata) } } `#expect`(metadata.overlayCount == 1) `#expect`(metadata.format == "geojson") `#expect`(metadata.originalName == "Test Site") + try await manager.deleteFile(metadata)Apply the same change to both import-success tests and remove the asynchronous cleanup helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MeshtasticTests/MapDataManagerTests.swift` around lines 119 - 136, Update both import-success tests, importsInMemoryGeoJSONBySuggestedName and appendsGeojsonExtensionWhenSuggestedNameHasNone, to await manager.deleteFile(metadata) directly during teardown and verify it succeeds rather than launching an unawaited Task with try?. Remove the cleanUp helper and preserve the existing assertions.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Meshtastic/Helpers/MapDataManager.swift`:
- Around line 147-152: Sanitize the user/node-derived suggestedName in the
filename construction within the map export flow before appending it to tempURL.
Reduce it to a safe lastPathComponent or replace it with a UUID-only filename,
while preserving the existing GeoJSON/JSON extension behavior and ensuring the
resulting path remains inside the per-run temporary directory.
In `@Meshtastic/Helpers/SitePlanner/CoverageEstimateCoordinator.swift`:
- Around line 102-105: Update the success path in CoverageEstimateCoordinator so
it checks Task cancellation immediately after
MapDataManager.shared.importFromData completes and before calling
finish(.succeeded(...)). Ensure a canceled task cannot report success, while
preserving the existing successful import behavior when the task remains active.
In `@Meshtastic/Helpers/SitePlanner/CoverageEstimateParameters.swift`:
- Around line 162-174: Update CoverageEstimateParameters.validationErrors() to
validate every emitted numeric field for finite, domain-valid values before URL
construction, including requiring maxRangeKm to be finite and positive rather
than only checking maxRangeKm > cap. Ensure NaN, infinity, negative, and zero
values produce appropriate validation errors, and preserve the existing cap and
coordinate checks.
In `@Meshtastic/Helpers/SitePlanner/LoRaRFHelpers.swift`:
- Around line 63-72: Normalize all return values in regionMaxPowerWatts(for:) to
conducted-power units, including the .eu868/.eu433 regional caps and the .anz433
case, rather than mixing conducted power with ERP. Preserve the existing region
groupings and fallback behavior, applying the appropriate conversion before each
regional limit is returned.
In `@Meshtastic/Views/Helpers/CoverageEstimateForm.swift`:
- Around line 62-70: Update the success-handling task in the coordinator.state
onChange callback to capture the succeeded overlay’s id and verify it still
matches the current succeeded state after the delay, before calling
acknowledge() and dismiss(). Leave the delayed success display behavior
unchanged, but prevent a stale task from dismissing a later run.
In `@Meshtastic/Views/Nodes/Helpers/Map/MapEstimateCoverageButton.swift`:
- Around line 50-56: Update prefilledParameters and the toolbar action flow so
no geographic default is used when visibleCenter and
connectedNode?.latestPosition?.nodeCoordinate are unavailable. Disable the
coverage estimate action until either a map center or radio coordinate exists,
and only construct or submit parameters with an available coordinate.
In `@Meshtastic/Views/Nodes/Helpers/NodeDetail.swift`:
- Line 681: Move EstimateCoverageButton outside the if connectedNode.num !=
node.num remote-node branch so it is available for the connected node’s own
detail screen, while preserving the existing known-position gate and button
parameters.
In `@MeshtasticTests/CoverageEstimateBridgeLiveSpikeTests.swift`:
- Around line 23-27: Gate the CoverageEstimateBridgeLiveSpikeTests suite or its
bridgeReceivesRealCoverageResult test with a Swift Testing enabled-if trait
controlled by the RUN_SITE_PLANNER_LIVE_TESTS environment variable, so it runs
only when explicitly opted in while preserving the existing live test behavior.
In `@MeshtasticTests/CoverageEstimateCoordinatorTests.swift`:
- Around line 26-31: Update CoverageEstimateCoordinator and the
startTransitionsToRunning test to inject a fake bridge or runner through a
protocol/closure, preventing start(_:) from constructing CoverageEstimateBridge
or triggering WebKit during state-machine tests. Keep the production live
implementation intact and leave real network coverage to
CoverageEstimateBridgeLiveSpikeTests.
In `@MeshtasticTests/SwiftUIViewSnapshotTests.swift`:
- Around line 1643-1721: Update assertViewSnapshot to compare rendered pixel
contents, not only dimensions, and fail when an expected baseline is missing
instead of creating one silently. Generate and check in baselines for
defaultPrefilledState, advancedSectionExpanded,
EstimateCoverageButtonSnapshotTests.defaultState, and
MapEstimateCoverageButtonSnapshotTests.defaultState so these tests validate
deterministic UI output on a clean checkout.
In `@specs/015-site-planner-outbound/plan.md`:
- Around line 31-39: Keep the Platform Parity gate explicitly incomplete while
Mac Catalyst remains unverified: update the T029 summary and final compliance
statement so they do not claim that no Constitution principle is violated or
otherwise imply Principle VII/FR-010 passes. Preserve the existing open-item
wording and mark the overall verification as pending Catalyst confirmation.
In `@specs/015-site-planner-outbound/tasks.md`:
- Around line 31-35: The task tracker incorrectly marks dependent work complete
while blocking task T012 remains unresolved. Update T013, the subsequent
user-story tasks, and the “Foundation ready” checkpoint to incomplete until
CoverageEstimateBridgeLiveSpikeTests verifies Catalyst support; alternatively,
resolve T012 and retain the completed statuses. Ensure the dependency status
consistently reflects FR-010 as unverified.
---
Minor comments:
In `@Meshtastic/Helpers/MapDataManager.swift`:
- Around line 150-153: Update the temporary-file cleanup in the import flow
around tempURL so cleanup is registered before directory creation, ensuring it
also runs when directory creation or writing fails. Track the UUID directory
separately and have the defer remove that directory rather than only the file at
tempURL.
In `@Meshtastic/Helpers/SitePlanner/CoverageEstimateParameters.swift`:
- Around line 1-12: Add the required file-level marker to
CoverageEstimateParameters.swift, using the exact filename-based form “// MARK:
CoverageEstimateParameters” near the top of the file before the existing
imports.
In `@Meshtastic/Helpers/SitePlanner/CoverageQueryURLBuilder.swift`:
- Around line 1-10: Add the required file-level marker at the top of
CoverageQueryURLBuilder.swift, using the filename-based form “// MARK:
CoverageQueryURLBuilder” before the existing header comments or imports.
In `@Meshtastic/Helpers/SitePlanner/LoRaRFHelpers.swift`:
- Around line 1-11: Add a file-level marker at the top of LoRaRFHelpers.swift
using the required `// MARK: LoRaRFHelpers` form, while preserving the existing
header comments and imports.
In `@MeshtasticTests/MapDataManagerTests.swift`:
- Around line 119-136: Update both import-success tests,
importsInMemoryGeoJSONBySuggestedName and
appendsGeojsonExtensionWhenSuggestedNameHasNone, to await
manager.deleteFile(metadata) directly during teardown and verify it succeeds
rather than launching an unawaited Task with try?. Remove the cleanUp helper and
preserve the existing assertions.
In `@specs/015-site-planner-outbound/checklists/requirements.md`:
- Around line 9-12: Correct the checked “No implementation details” assertions
in the requirements checklist to reflect the implementation details documented
in spec.md, including the embedded web engine, server API absence, and
result-retrieval mechanism. Either remove those details from the specification
or uncheck/update the affected checklist entries, while preserving accurate
checklist items.
In `@specs/015-site-planner-outbound/data-model.md`:
- Around line 5-15: Correct the persistence heading for MapDataMetadata to
distinguish it from SwiftData and identify it as persisted through the existing
file-based MapDataManager pipeline. Keep the existing MapDataMetadata
description and its importFromData behavior unchanged, and align the
classification with the persistence model documented in plan.md and research.md.
- Around line 22-26: Update the Environment parameter contract to mark
radioClimate and polarization as required values rather than optional, and
document their concrete default behavior from CoverageEstimateParameters.swift.
Preserve the existing advanced grouping and all other field definitions.
In `@specs/015-site-planner-outbound/quickstart.md`:
- Around line 13-16: Update the xcodebuild test selectors in the quickstart
command: replace LoRaSensitivityTests with LoRaRFHelpersTests and add
CoverageQueryURLBuilderTests, while preserving the existing
CoverageEstimateParametersTests and MapDataManagerImportFromDataTests selectors.
- Line 5: Update the Meshtastic Client Design Standards prerequisite link in the
quickstart so it points directly to a resolvable versioned document containing
the standards, or explicitly require resolving the current symlink before
review. Preserve the requirement to fetch and review the standards freshly
before UI work.
In `@specs/015-site-planner-outbound/spec.md`:
- Around line 56-64: Specify the expected behavior when a new estimate’s site
name matches an existing overlay name in the Edge Cases section, choosing and
documenting whether to suffix, prompt, replace, or reject the import. Ensure the
requirement clearly defines the user-visible outcome and preserves existing
overlays as appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a455895-7d99-47e7-9381-cd3b0a895d74
⛔ Files ignored due to path filters (4)
MeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/coverageEstimateForm_advancedExpanded.pngis excluded by!**/*.pngMeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/coverageEstimateForm_default.pngis excluded by!**/*.pngMeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/estimateCoverageButton_default.pngis excluded by!**/*.pngMeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/mapEstimateCoverageButton_default.pngis excluded by!**/*.png
📒 Files selected for processing (36)
.specify/feature.jsonCLAUDE.mdMeshtastic.xcodeproj/project.pbxprojMeshtastic/Helpers/MapDataManager.swiftMeshtastic/Helpers/SitePlanner/CoverageEstimateBridge.swiftMeshtastic/Helpers/SitePlanner/CoverageEstimateCoordinator.swiftMeshtastic/Helpers/SitePlanner/CoverageEstimateParameters.swiftMeshtastic/Helpers/SitePlanner/CoverageQueryURLBuilder.swiftMeshtastic/Helpers/SitePlanner/LoRaRFHelpers.swiftMeshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/user/settings.mdMeshtastic/Resources/docs/markdown/user/whats-new.mdMeshtastic/Resources/docs/user/settings.htmlMeshtastic/Resources/docs/user/whats-new.htmlMeshtastic/Views/Helpers/CoverageEstimateForm.swiftMeshtastic/Views/Nodes/Helpers/Actions/EstimateCoverageButton.swiftMeshtastic/Views/Nodes/Helpers/Map/MapEstimateCoverageButton.swiftMeshtastic/Views/Nodes/Helpers/NodeDetail.swiftMeshtastic/Views/Nodes/MeshMapMK.swiftMeshtasticTests/CoverageEstimateBridgeLiveSpikeTests.swiftMeshtasticTests/CoverageEstimateCoordinatorTests.swiftMeshtasticTests/CoverageEstimateParametersTests.swiftMeshtasticTests/CoverageQueryURLBuilderTests.swiftMeshtasticTests/LoRaRFHelpersTests.swiftMeshtasticTests/MapDataManagerTests.swiftMeshtasticTests/SwiftUIViewSnapshotTests.swiftdocs/user/whats-new.mdspecs/015-site-planner-outbound/checklists/requirements.mdspecs/015-site-planner-outbound/contracts/native-bridge-contract.mdspecs/015-site-planner-outbound/contracts/query-contract.mdspecs/015-site-planner-outbound/data-model.mdspecs/015-site-planner-outbound/plan.mdspecs/015-site-planner-outbound/quickstart.mdspecs/015-site-planner-outbound/research.mdspecs/015-site-planner-outbound/spec.mdspecs/015-site-planner-outbound/tasks.md
Comment on lines
+147
to
+152
| let filename = suggestedName.lowercased().hasSuffix(".geojson") || suggestedName.lowercased().hasSuffix(".json") | ||
| ? suggestedName | ||
| : "\(suggestedName).geojson" | ||
| let tempURL = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString).appendingPathComponent(filename) | ||
| try FileManager.default.createDirectory(at: tempURL.deletingLastPathComponent(), withIntermediateDirectories: true) | ||
| try data.write(to: tempURL) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Sanitize suggestedName before using it as a filesystem path.
The coordinator passes the user/node-derived name directly here. Names containing / or .. can escape the per-run temporary directory and potentially overwrite another file in the app container. Reduce it to lastPathComponent or use a UUID-only temporary filename.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meshtastic/Helpers/MapDataManager.swift` around lines 147 - 152, Sanitize the
user/node-derived suggestedName in the filename construction within the map
export flow before appending it to tempURL. Reduce it to a safe
lastPathComponent or replace it with a UUID-only filename, while preserving the
existing GeoJSON/JSON extension behavior and ensuring the resulting path remains
inside the per-run temporary directory.
Comment on lines
+102
to
+105
| let data = try await bridge.run(params) | ||
| try Task.checkCancellation() | ||
| let overlay = try await MapDataManager.shared.importFromData(data, suggestedName: params.name) | ||
| self?.finish(.succeeded(overlay: overlay), token: token) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'Meshtastic/Helpers/SitePlanner/CoverageEstimateCoordinator.swift' 'Meshtastic/**/MapDataManager*.swift' 'Meshtastic/**/*.swift' | sed -n '1,120p'
echo '--- CoverageEstimateCoordinator.swift ---'
wc -l Meshtastic/Helpers/SitePlanner/CoverageEstimateCoordinator.swift
sed -n '1,180p' Meshtastic/Helpers/SitePlanner/CoverageEstimateCoordinator.swift
echo '--- importFromData search ---'
rg -n "importFromData\s*\(" Meshtastic -g '*.swift' -A 8 -B 8Repository: meshtastic/Meshtastic-Apple
Length of output: 15637
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- MapDataManager.swift (import pipeline) ---'
wc -l Meshtastic/Helpers/MapDataManager.swift
sed -n '120,240p' Meshtastic/Helpers/MapDataManager.swift
echo '--- Cancellation checks in MapDataManager.swift ---'
rg -n "checkCancellation|CancellationError|cancel\(" Meshtastic/Helpers/MapDataManager.swift -A 4 -B 4
echo '--- CoverageEstimateBridge.swift ---'
wc -l Meshtastic/Helpers/SitePlanner/CoverageEstimateBridge.swift
sed -n '1,220p' Meshtastic/Helpers/SitePlanner/CoverageEstimateBridge.swiftRepository: meshtastic/Meshtastic-Apple
Length of output: 5339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- processUploadedFile / persistence path ---'
sed -n '1,140p' Meshtastic/Helpers/MapDataManager.swift
sed -n '240,420p' Meshtastic/Helpers/MapDataManager.swift
echo '--- cancellation-related APIs in MapDataManager.swift ---'
rg -n "Task\.checkCancellation|CancellationError|isCancelled|cancel\(" Meshtastic/Helpers/MapDataManager.swift -A 3 -B 3Repository: meshtastic/Meshtastic-Apple
Length of output: 11135
Avoid reporting success after a canceled import
CoverageEstimateCoordinator.swift:102-105 only checks cancellation before MapDataManager.shared.importFromData(...). If the task is canceled while that import is running, the import can still complete and then call finish(.succeeded...). Check cancellation again immediately before finishing, or make the import path cancel/rollback its writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meshtastic/Helpers/SitePlanner/CoverageEstimateCoordinator.swift` around
lines 102 - 105, Update the success path in CoverageEstimateCoordinator so it
checks Task cancellation immediately after MapDataManager.shared.importFromData
completes and before calling finish(.succeeded(...)). Ensure a canceled task
cannot report success, while preserving the existing successful import behavior
when the task remains active.
Source: Learnings
Comment on lines
+162
to
+174
| func validationErrors() -> [CoverageEstimateValidationError] { | ||
| var errors: [CoverageEstimateValidationError] = [] | ||
|
|
||
| if !CLLocationCoordinate2DIsValid(latitude: latitude, longitude: longitude) { | ||
| errors.append(.missingPosition) | ||
| } | ||
| if !Self.receiverSensitivityRange.contains(receiverSensitivityDBm) { | ||
| errors.append(.receiverSensitivityOutOfRange) | ||
| } | ||
| let cap = highResolutionTerrain ? Self.highResMaxRangeKm : Self.standardMaxRangeKm | ||
| if maxRangeKm > cap { | ||
| errors.append(.maxRangeExceedsCap(highResCapKm: Self.highResMaxRangeKm, standardCapKm: Self.standardMaxRangeKm)) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject non-finite and non-positive values before building the URL.
maxRangeKm = -1 and Double.nan bypass maxRangeKm > cap; other emitted Double values also have no finiteness checks. Because the coordinator gates bridge execution on params.isValid, malformed values can reach the planner. Add domain and finite-value validation for every emitted numeric field, at minimum requiring a positive finite range.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meshtastic/Helpers/SitePlanner/CoverageEstimateParameters.swift` around lines
162 - 174, Update CoverageEstimateParameters.validationErrors() to validate
every emitted numeric field for finite, domain-valid values before URL
construction, including requiring maxRangeKm to be finite and positive rather
than only checking maxRangeKm > cap. Ensure NaN, infinity, negative, and zero
values produce appropriate validation errors, and preserve the existing cap and
coordinate checks.
Comment on lines
+63
to
+72
| static func regionMaxPowerWatts(for region: RegionCodes) -> Double { | ||
| switch region { | ||
| case .us, .anz, .anz433: | ||
| return 1.0 // FCC Part 15.247 (US) / equivalent ISM rules: 1 W conducted in the 902-928 MHz band. | ||
| case .eu868, .eu866, .euN868, .ua868, .ru, .np865, .nz865: | ||
| return 0.025 // ETSI EN 300 220 SRD band, 868 MHz: 25 mW ERP. | ||
| case .eu433, .ua433, .my433, .ph433, .kz433: | ||
| return 0.01 // ETSI EN 300 220 SRD band, 433 MHz: 10 mW ERP. | ||
| default: | ||
| return 0.1 // No verified regional figure — Site Planner's own factory default, not a guess. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files mentioning regionMaxPowerWatts / tx_power / antenna gain ==\n'
rg -n "regionMaxPowerWatts|tx_power|antenna gain|antennaGain|gain" Meshtastic -g '*.swift'
printf '\n== Outline LoRaRFHelpers.swift ==\n'
ast-grep outline Meshtastic/Helpers/SitePlanner/LoRaRFHelpers.swift --view expanded
printf '\n== Outline other Site Planner files likely involved ==\n'
fd -a -t f 'SitePlanner' Meshtastic | sed 's#^\./##'Repository: meshtastic/Meshtastic-Apple
Length of output: 9566
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== LoRaRFHelpers.swift relevant section ==\n'
sed -n '1,140p' Meshtastic/Helpers/SitePlanner/LoRaRFHelpers.swift | cat -n
printf '\n== CoverageEstimateParameters.swift relevant section ==\n'
sed -n '1,180p' Meshtastic/Helpers/SitePlanner/CoverageEstimateParameters.swift | cat -n
printf '\n== CoverageQueryURLBuilder.swift relevant section ==\n'
sed -n '1,120p' Meshtastic/Helpers/SitePlanner/CoverageQueryURLBuilder.swift | cat -n
printf '\n== CoverageEstimateForm.swift relevant section ==\n'
sed -n '1,180p' Meshtastic/Views/Helpers/CoverageEstimateForm.swift | cat -nRepository: meshtastic/Meshtastic-Apple
Length of output: 25110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Repo references to tx_power / tx_gain semantics ==\n'
rg -n "tx_power|tx_gain|ERP|EIRP|conducted power|radiated power|antenna gain" Meshtastic specs README.md -g '*.md' -g '*.swift'
printf '\n== Site Planner docs / contract references ==\n'
fd -a -t f 'query-contract.md|data-model.md|research.md|specs' Meshtastic specs | sed 's#^\./##'
printf '\n== Search in site planner docs ==\n'
rg -n "tx_power|tx_gain|ERP|EIRP|antenna gain|conducted|radiated" specs Meshtastic/Helpers/SitePlanner Meshtastic/Views/Helpers -g '*.md' -g '*.swift'Repository: meshtastic/Meshtastic-Apple
Length of output: 5850
Normalize regional tx_power limits to one conducted-power basis. tx_power is sent in watts and tx_gain is already separate, so mixing conducted power for US/ANZ with ERP for EU/433 can overstate effective radiated power. Convert every regional cap to the same basis before returning it, including .anz433.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meshtastic/Helpers/SitePlanner/LoRaRFHelpers.swift` around lines 63 - 72,
Normalize all return values in regionMaxPowerWatts(for:) to conducted-power
units, including the .eu868/.eu433 regional caps and the .anz433 case, rather
than mixing conducted power with ERP. Preserve the existing region groupings and
fallback behavior, applying the appropriate conversion before each regional
limit is returned.
Comment on lines
+62
to
+70
| .onChange(of: coordinator.state) { _, newState in | ||
| if case .succeeded = newState { | ||
| // Give the user a moment to see the success row before auto-dismissing. | ||
| Task { | ||
| try? await Task.sleep(for: .seconds(1)) | ||
| coordinator.acknowledge() | ||
| dismiss() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== outline CoverageEstimateForm.swift ==\n'
ast-grep outline Meshtastic/Views/Helpers/CoverageEstimateForm.swift --view expanded || true
printf '\n== search related symbols ==\n'
rg -n "enum .*State|case succeeded|acknowledge\\(|dismiss\\(|CoverageEstimate|coordinator" Meshtastic -g '!**/build/**' -g '!**/dist/**' || trueRepository: meshtastic/Meshtastic-Apple
Length of output: 22231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== outline CoverageEstimateForm.swift ==\n'
ast-grep outline Meshtastic/Views/Helpers/CoverageEstimateForm.swift --view expanded || true
printf '\n== read relevant lines ==\n'
sed -n '1,180p' Meshtastic/Views/Helpers/CoverageEstimateForm.swift
printf '\n== search related symbols ==\n'
rg -n "case succeeded|acknowledge\\(|dismiss\\(|CoverageEstimate|overlay\\.id|currentOverlay|state" Meshtastic -g '!**/build/**' -g '!**/dist/**'Repository: meshtastic/Meshtastic-Apple
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== CoverageEstimateCoordinator.swift =='
sed -n '1,240p' Meshtastic/Helpers/SitePlanner/CoverageEstimateCoordinator.swift
echo
echo '== CoverageEstimateForm.swift state/submit sections =='
sed -n '240,340p' Meshtastic/Views/Helpers/CoverageEstimateForm.swift
echo
echo '== MapDataMetadata definition / id field =='
rg -n "struct MapDataMetadata|var id:|let id:" Meshtastic/Helpers Meshtastic/Model Meshtastic -g '!**/build/**' -g '!**/dist/**'Repository: meshtastic/Meshtastic-Apple
Length of output: 18237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '420,560p' Meshtastic/Helpers/MapDataManager.swift
printf '\n== importFromData references ==\n'
rg -n "importFromData\\(" Meshtastic/Helpers Meshtastic/Views -g '!**/build/**' -g '!**/dist/**'Repository: meshtastic/Meshtastic-Apple
Length of output: 3283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,220p' Meshtastic/Helpers/MapDataManager.swiftRepository: meshtastic/Meshtastic-Apple
Length of output: 4473
Scope the auto-dismiss to the finished run.
A second Run can start during the 1s delay, and the stale success task will still call dismiss() even though a new estimate is running. Guard the task with the succeeded overlay’s id before dismissing.
Proposed fix
- if case .succeeded = newState {
+ if case let .succeeded(overlay) = newState {
Task {
try? await Task.sleep(for: .seconds(1))
+ guard case let .succeeded(currentOverlay) = coordinator.state,
+ currentOverlay.id == overlay.id else { return }
coordinator.acknowledge()
dismiss()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .onChange(of: coordinator.state) { _, newState in | |
| if case .succeeded = newState { | |
| // Give the user a moment to see the success row before auto-dismissing. | |
| Task { | |
| try? await Task.sleep(for: .seconds(1)) | |
| coordinator.acknowledge() | |
| dismiss() | |
| } | |
| } | |
| .onChange(of: coordinator.state) { _, newState in | |
| if case let .succeeded(overlay) = newState { | |
| // Give the user a moment to see the success row before auto-dismissing. | |
| Task { | |
| try? await Task.sleep(for: .seconds(1)) | |
| guard case let .succeeded(currentOverlay) = coordinator.state, | |
| currentOverlay.id == overlay.id else { return } | |
| coordinator.acknowledge() | |
| dismiss() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meshtastic/Views/Helpers/CoverageEstimateForm.swift` around lines 62 - 70,
Update the success-handling task in the coordinator.state onChange callback to
capture the succeeded overlay’s id and verify it still matches the current
succeeded state after the delay, before calling acknowledge() and dismiss().
Leave the delayed success display behavior unchanged, but prevent a stale task
from dismissing a later run.
Comment on lines
+23
to
+27
| @Suite("CoverageEstimateBridge live spike (T012)") | ||
| @MainActor | ||
| struct CoverageEstimateBridgeLiveSpikeTests { | ||
|
|
||
| @Test func bridgeReceivesRealCoverageResult() async throws { |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== target file ==\n'
sed -n '1,220p' MeshtasticTests/CoverageEstimateBridgeLiveSpikeTests.swift
printf '\n== live-test / gating patterns ==\n'
rg -n "RUN_.*LIVE|enabled\\(|disabled\\(|Trait|`#require`|ProcessInfo\\.processInfo\\.environment|live spike|key window|CoverageEstimateBridge" MeshtasticTests . -g '!**/DerivedData/**' -g '!**/.build/**' || true
printf '\n== Swift Testing suites in tests ==\n'
rg -n "^`@Suite`|^\\s*`@Suite`|import Testing" MeshtasticTests -g '*.swift' || trueRepository: meshtastic/Meshtastic-Apple
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== bridge failure path ==\n'
sed -n '1,240p' Meshtastic/Helpers/SitePlanner/CoverageEstimateBridge.swift
printf '\n== any explicit live-test gating in docs/specs ==\n'
rg -n "enabled\\(if:|disabled\\(|RUN_SITE_PLANNER_LIVE_TESTS|live spike|Test navigator|only-testing:MeshtasticTests/CoverageEstimateBridgeLiveSpikeTests|key window" specs MeshtasticTests Meshtastic -g '*.md' -g '*.swift' || trueRepository: meshtastic/Meshtastic-Apple
Length of output: 24277
🌐 Web query:
Swift Testing Suite trait enabled(if:) documentation
💡 Result:
The enabled(if:) trait in the Swift Testing framework is a condition trait used to conditionally enable a test or test suite [1][2]. It allows you to specify a boolean condition that must be true for the test to run [3][4]. If the condition evaluates to false, the test is skipped [5][3]. Key details regarding the enabled(if:) trait include: Functionality: When you apply.enabled(if: condition) to a test function or suite, the framework evaluates the condition at runtime [3][2]. If the condition is true, the test proceeds; if false, the test is skipped [3]. Usage Example: You can pass the trait as an argument to the @Test attribute: @Test("Ice cream is cold",.enabled(if: Season.current ==.summer)) func isCold async throws { // Test logic } Multiple Conditions: You can apply multiple enabled(if:) traits to a single test or suite [3][6]. If multiple conditions are applied, all of them must evaluate to true for the test to run [3]. If any condition fails, the test will be skipped, and the testing framework will note the first failed condition as the reason [3]. Complex Logic: For complex conditions, it is recommended to factor the logic into a helper function to maintain readability [3]. Relationship with Other Traits: The enabled(if:) trait works in conjunction with other traits, such as.disabled(...) [3]. You can combine them to create granular control over test execution [3]. Official Documentation: For further technical details, you can refer to the official Apple Developer documentation for ConditionTrait [1] or the dedicated guide on Enabling and disabling tests [5].
Citations:
- 1: https://developer.apple.com/documentation/testing/conditiontrait
- 2: https://www.avanderlee.com/swift-testing/using-traits-to-annotate-and-customize-test-behavior/
- 3: https://github.com/apple/swift-testing/blob/main/Sources/Testing/Testing.docc/EnablingAndDisabling.md
- 4: https://fatbobman.com/en/posts/mastering-the-swift-testing-framework/
- 5: https://developer.apple.com/documentation/testing/enablinganddisabling
- 6: https://swiftwithmajid.com/2024/11/05/introducing-swift-testing-traits/
Gate this live spike behind an opt-in trait. The suite always runs a live network/WebKit path and can fail on offline or windowless test runs, so it shouldn’t be part of ordinary test execution. Use a Swift Testing condition trait, for example .enabled(if: ProcessInfo.processInfo.environment["RUN_SITE_PLANNER_LIVE_TESTS"] == "1"), or move it to a dedicated live-test plan.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MeshtasticTests/CoverageEstimateBridgeLiveSpikeTests.swift` around lines 23 -
27, Gate the CoverageEstimateBridgeLiveSpikeTests suite or its
bridgeReceivesRealCoverageResult test with a Swift Testing enabled-if trait
controlled by the RUN_SITE_PLANNER_LIVE_TESTS environment variable, so it runs
only when explicitly opted in while preserving the existing live test behavior.
Comment on lines
+26
to
+31
| @Test func startTransitionsToRunning() { | ||
| let coordinator = CoverageEstimateCoordinator.shared | ||
| coordinator.reset() | ||
|
|
||
| coordinator.start(validParams()) | ||
| defer { coordinator.reset() } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant test and implementation files.
git ls-files 'MeshtasticTests/**/*.swift' 'Meshtastic/**/*.swift' | sed -n '1,200p'
echo '--- CoverageEstimateCoordinatorTests outline ---'
ast-grep outline MeshtasticTests/CoverageEstimateCoordinatorTests.swift --view expanded || true
echo '--- CoverageEstimateCoordinator outline ---'
ast-grep outline Meshtastic/CoverageEstimateCoordinator.swift --view expanded || true
echo '--- CoverageEstimateBridge outline ---'
ast-grep outline Meshtastic/CoverageEstimateBridge.swift --view expanded || trueRepository: meshtastic/Meshtastic-Apple
Length of output: 10479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant slices with line numbers.
for f in MeshtasticTests/CoverageEstimateCoordinatorTests.swift Meshtastic/CoverageEstimateCoordinator.swift Meshtastic/CoverageEstimateBridge.swift; do
echo "===== $f ====="
wc -l "$f"
done
echo '--- Tests file ---'
sed -n '1,220p' MeshtasticTests/CoverageEstimateCoordinatorTests.swift | cat -n
echo '--- Coordinator file ---'
sed -n '1,260p' Meshtastic/CoverageEstimateCoordinator.swift | cat -n
echo '--- Bridge file ---'
sed -n '1,260p' Meshtastic/CoverageEstimateBridge.swift | cat -nRepository: meshtastic/Meshtastic-Apple
Length of output: 424
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find start/reset/run usage and any injected dependencies.
rg -n "CoverageEstimateCoordinator|CoverageEstimateBridge|bridge\.run|start\(" MeshtasticTests Meshtastic -g '*.swift'Repository: meshtastic/Meshtastic-Apple
Length of output: 8564
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- MeshtasticTests/CoverageEstimateCoordinatorTests.swift ---'
sed -n '1,220p' MeshtasticTests/CoverageEstimateCoordinatorTests.swift | cat -n
echo '--- Meshtastic/Helpers/SitePlanner/CoverageEstimateCoordinator.swift ---'
sed -n '1,240p' Meshtastic/Helpers/SitePlanner/CoverageEstimateCoordinator.swift | cat -n
echo '--- Meshtastic/Helpers/SitePlanner/CoverageEstimateBridge.swift ---'
sed -n '1,260p' Meshtastic/Helpers/SitePlanner/CoverageEstimateBridge.swift | cat -n
echo '--- MeshtasticTests/CoverageEstimateBridgeLiveSpikeTests.swift ---'
sed -n '1,220p' MeshtasticTests/CoverageEstimateBridgeLiveSpikeTests.swift | cat -nRepository: meshtastic/Meshtastic-Apple
Length of output: 21266
Use a fake bridge here
CoverageEstimateCoordinator.start(_:) still creates a real CoverageEstimateBridge and can kick off the WebKit load, so these state-machine tests aren’t fully isolated from the live planner. Inject a bridge/runner protocol or closure and keep the real network path in CoverageEstimateBridgeLiveSpikeTests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MeshtasticTests/CoverageEstimateCoordinatorTests.swift` around lines 26 - 31,
Update CoverageEstimateCoordinator and the startTransitionsToRunning test to
inject a fake bridge or runner through a protocol/closure, preventing start(_:)
from constructing CoverageEstimateBridge or triggering WebKit during
state-machine tests. Keep the production live implementation intact and leave
real network coverage to CoverageEstimateBridgeLiveSpikeTests.
Comment on lines
+1643
to
+1721
| @Test("Default prefilled state") | ||
| @MainActor | ||
| func defaultPrefilledState() async { | ||
| let params = CoverageEstimateParameters( | ||
| name: "U-District Solar", | ||
| latitude: 47.6602, | ||
| longitude: -122.3132, | ||
| transmitPowerWatts: 0.1, | ||
| transmitFrequencyMHz: 906.875 | ||
| ) | ||
| let view = CoverageEstimateForm(initialParameters: params) | ||
| await assertViewSnapshot(of: view, width: 390, height: 900, named: "coverageEstimateForm_default") | ||
| } | ||
|
|
||
| @Test("Advanced section expanded (US3, T025)") | ||
| @MainActor | ||
| func advancedSectionExpanded() async { | ||
| var params = CoverageEstimateParameters( | ||
| name: "U-District Solar", | ||
| latitude: 47.6602, | ||
| longitude: -122.3132, | ||
| transmitPowerWatts: 0.1, | ||
| transmitFrequencyMHz: 906.875 | ||
| ) | ||
| params.situationFraction = 0.5 | ||
| params.timeFraction = 0.9 | ||
| params.clutterHeightMeters = 3 | ||
| let view = CoverageEstimateForm(initialParameters: params, initiallyShowAdvanced: true) | ||
| await assertViewSnapshot(of: view, width: 390, height: 1400, named: "coverageEstimateForm_advancedExpanded") | ||
| } | ||
| } | ||
|
|
||
| @Suite("EstimateCoverageButton Snapshots") | ||
| struct EstimateCoverageButtonSnapshotTests { | ||
|
|
||
| @Test("Default state in an Actions-style list") | ||
| @MainActor | ||
| func defaultState() async { | ||
| let container = try! ModelContainer( | ||
| for: Schema(MeshtasticSchema.allModels), | ||
| configurations: ModelConfiguration(isStoredInMemoryOnly: true) | ||
| ) | ||
| let context = container.mainContext | ||
|
|
||
| let node = NodeInfoEntity() | ||
| node.num = 0xE75432 | ||
| let user = UserEntity() | ||
| user.longName = "U-District Solar" | ||
| user.shortName = "UDS" | ||
| node.user = user | ||
| let position = PositionEntity() | ||
| position.latitudeI = 476602000 | ||
| position.longitudeI = -1223132000 | ||
| node.positions = [position] | ||
| context.insert(node) | ||
|
|
||
| let view = List { | ||
| EstimateCoverageButton(node: node, connectedNode: node) | ||
| } | ||
| .modelContainer(container) | ||
|
|
||
| await assertViewSnapshot(of: view, width: 390, height: 100, named: "estimateCoverageButton_default") | ||
| } | ||
| } | ||
|
|
||
| @Suite("MapEstimateCoverageButton Snapshots") | ||
| struct MapEstimateCoverageButtonSnapshotTests { | ||
|
|
||
| @Test("Default state (no radio connected, map-center prefill)") | ||
| @MainActor | ||
| func defaultState() async { | ||
| let container = try! ModelContainer( | ||
| for: Schema(MeshtasticSchema.allModels), | ||
| configurations: ModelConfiguration(isStoredInMemoryOnly: true) | ||
| ) | ||
| let view = MapEstimateCoverageButton(visibleCenter: CLLocationCoordinate2D(latitude: 47.6062, longitude: -122.3321)) | ||
| .environmentObject(AccessoryManager.shared) | ||
| .modelContainer(container) | ||
| await assertViewSnapshot(of: view, width: 60, height: 60, transparent: true, named: "mapEstimateCoverageButton_default") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make these real, deterministic snapshot tests.
The new tests call assertViewSnapshot, but the supplied helper only compares pixel dimensions and silently creates a baseline when one is missing. A same-sized visual regression—or a clean checkout without the four new PNG baselines—can therefore pass without validating the UI. Compare pixel contents and check in the generated baselines (or fail when a baseline is absent).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MeshtasticTests/SwiftUIViewSnapshotTests.swift` around lines 1643 - 1721,
Update assertViewSnapshot to compare rendered pixel contents, not only
dimensions, and fail when an expected baseline is missing instead of creating
one silently. Generate and check in baselines for defaultPrefilledState,
advancedSectionExpanded, EstimateCoverageButtonSnapshotTests.defaultState, and
MapEstimateCoverageButtonSnapshotTests.defaultState so these tests validate
deterministic UI output on a clean checkout.
Comment on lines
+31
to
+39
| | IV. Structured Logging | `CoverageEstimateBridge`/`CoverageEstimateCoordinator`/`MapDataManager.importFromData` all use `Logger.services`, no `print()` | ✅ Pass | | ||
| | V. Protobuf Contract Fidelity | No protobuf changes — reads existing generated `LoRaConfigEntity`/`ModemPresets` fields only | ✅ Pass | | ||
| | VI. Lint-Clean Commits | **Verified, not assumed**: full-suite SwiftLint run (T028) reports 0 violations attributed to any of this feature's files | ✅ Pass — verified | | ||
| | VII. Platform Parity | Must work on iPhone, iPad, and Mac Catalyst equally. **iOS Simulator: fully verified** — `CoverageEstimateBridgeLiveSpikeTests` passed twice, independently, against the live site.meshtastic.org (6.3s and 20.7s real round trips). **Mac Catalyst: still genuinely open** (research.md §5) — blocked by environment issues on this dev machine (test target's deployment target vs. installed OS; an ad-hoc `open`-launched spike never presented a window), not by a confirmed WebKit failure. The bridge's design already commits to the safer attached-but-invisible approach as a hedge. | ⚠️ **Open** — re-run `CoverageEstimateBridgeLiveSpikeTests` via Xcode's Test navigator on a Catalyst destination before calling this fully verified | | ||
| | VIII. Design Standards Compliance | Live v1.4 doc fetched successfully (the "failure" during planning was a symlink-resolution red herring, not a real block — research.md §6). Findings applied and **visually verified** via snapshot, not just claimed: locale-aware `Measurement` conversion for height/range fields (2m → "6.6 ft", 30km → "18.6 mi"), plain-language subtext for RF jargon, a real placeholder-text bug caught and fixed by the visual check itself (an empty optional field showed its own label as a redundant placeholder) | ✅ Pass — verified, not deferred | | ||
|
|
||
| **Note on II**: `MapDataManager` storing overlays as flat files rather than SwiftData predates this feature (it already exists on `main`) and is out of scope to "fix" here — this plan extends it consistently rather than introducing a second, inconsistent storage mechanism. Not a new violation. | ||
|
|
||
| **T029 summary**: 6 of 8 principles fully pass with direct verification (not just process-compliance claims). One (VII) has a genuine, honestly-tracked open item — Mac Catalyst runtime behavior — that is an environment-access limitation on this specific machine, not a known defect. No principle is violated. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not mark the platform-parity gate as effectively passing while Catalyst remains unverified.
This section correctly records Mac Catalyst as open, but then concludes that no Constitution principle is violated. Since Principle VII and FR-010 require platform equivalence, keep the gate explicitly incomplete until the Catalyst run is verified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/015-site-planner-outbound/plan.md` around lines 31 - 39, Keep the
Platform Parity gate explicitly incomplete while Mac Catalyst remains
unverified: update the T029 summary and final compliance statement so they do
not claim that no Constitution principle is violated or otherwise imply
Principle VII/FR-010 passes. Preserve the existing open-item wording and mark
the overall verification as pending Catalyst confirmation.
Comment on lines
+31
to
+35
| - [X] T011 Implement `CoverageEstimateBridge`: a hidden `WKWebView`, the `WKUserScript` shim that defines `window.__meshtasticNative.onCoverage` and forwards to a `WKScriptMessageHandler`, and the handler decoding `WKScriptMessage.body` into the raw GeoJSON string — exactly per contracts/native-bridge-contract.md — in `Meshtastic/Helpers/SitePlanner/CoverageEstimateBridge.swift` (register in project.pbxproj) — depends on T007. **Built attached-but-invisible (1×1, alpha 0, added to the key window) from the start, not fully detached — see T012.** | ||
| - [ ] T012 SPIKE (blocking, partially resolved — see research.md §5): a real integration test (`CoverageEstimateBridgeLiveSpikeTests`, hits the live site.meshtastic.org) confirms the bridge works end-to-end on **iOS Simulator** (6.3s round trip, real navigation/JS/callback). **Mac Catalyst is still unverified** — not because of a WebKit failure, but because of two environment issues on this dev machine: the test target's Catalyst deployment target (26.5) exceeds this machine's installed macOS (26.4.1), and a manual `open`-launched spike never presented a window at all (unrelated ad-hoc-launch/signing issue, not conclusively diagnosed). Re-run `CoverageEstimateBridgeLiveSpikeTests` via Xcode's own Test navigator on a Catalyst destination before considering this closed — depends on T011 | ||
| - [X] T013 Implement `CoverageEstimateCoordinator`: a singleton owning `CoverageEstimateState`, enforcing exactly one in-flight run app-wide (FR-007), cancellation (FR-008), a timeout policy for "the bridge never called back" (contracts/native-bridge-contract.md's Timeout Policy note), routing a successful result into `MapDataManager.importFromData`, and logging every lifecycle transition via `Logger` (never `print`, per Constitution IV) — in `Meshtastic/Helpers/SitePlanner/CoverageEstimateCoordinator.swift` (register in project.pbxproj) — depends on T009, T012 | ||
|
|
||
| **Checkpoint**: Foundation ready — every user story from here is UI-only, wiring existing pieces together. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep dependent tasks incomplete until the blocking Catalyst spike is resolved.
T012 is unchecked and explicitly blocking, yet T013 and the subsequent user-story tasks are marked complete. The dependency section also says T012 blocks all stories. Either resolve T012 or mark dependent tasks/checkpoints as incomplete; otherwise the task tracker falsely reports a releasable feature despite FR-010 remaining unverified.
Also applies to: 95-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/015-site-planner-outbound/tasks.md` around lines 31 - 35, The task
tracker incorrectly marks dependent work complete while blocking task T012
remains unresolved. Update T013, the subsequent user-story tasks, and the
“Foundation ready” checkpoint to incomplete until
CoverageEstimateBridgeLiveSpikeTests verifies Catalyst support; alternatively,
resolve T012 and retain the completed statuses. Ensure the dependency status
consistently reflects FR-010 as unverified.
The imported overlay/file was previously named after the site's friendly name; now it's `<nodeID> <yyyy-MM-dd>` (e.g. `!a1b2c3d4 2026-07-11`) so stored coverage maps are identifiable per-node and by day. Decoupled the overlay name from the planner site name: params.name still carries the friendly node long name (sent to the planner → properties.name metadata), while a new optional siteIdentifier (the node hex id, nil for the map entry point which has no node) drives overlayName(date:). Date is yyyy-MM-dd in a fixed POSIX locale — this is a stored file identifier (sortable/stable), deliberately not the OS-locale display format. Map entry point (no node) falls back to the site name + date. 3 unit tests for overlayName (node-id, name-fallback, empty-identifier); date is injected so the assertion is deterministic. Full params suite green; built and installed to device.
Member
Author
|
Superseded by #2081, closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lets a user run a Meshtastic Site Planner RF coverage estimate from inside the app — from a node's detail screen or an arbitrary map location — without a browser or share sheet. Drives the planner in a hidden
WKWebViewat the flat query URL contract; aWKUserScriptshim bridges the planner's native-bridge callback (window.__meshtasticNative.onCoverage) to aWKScriptMessageHandler; the result feeds into the existing GeoJSON overlay pipeline via a newMapDataManager.importFromData. Mirrors the shipped Android reference implementation (Meshtastic-Android#6136) for parameter scope, grouping, and entry points.What's included
LoRaChannelCalculator, transmit power/receiver sensitivity via RF math sourced from Meshtastic's published radio-settings docs andconfig.proto's own documentation (not guessed)Measurement-based, per the Meshtastic Client Design Standards §10) — metric stored, feet/miles or meters/km displayed per the user's localeVerification
MeshtasticTestssuite run; SwiftLint clean on every file this PR touches.Known gap — please read before merging
Related
specs/015-site-planner-outbound/Summary by CodeRabbit
New Features
Documentation
Tests