Add circular MiniMap widget - #2852
Conversation
ArturoManzoli
changed the title
Automated PR Review (Claude)0. SummaryVerdict: MINOR SUGGESTIONS Minor items to address: 1.1, 2.1, 4.1 This PR adds a new circular MiniMap widget with heading-up rotation (via the 1. Correctness & Implementation Bugs1.1 ( /**
*
*/
L?: typeof LAGENTS.md explicitly forbids JSDoc blocks whose summary is empty or whitespace-only ("Never write a JSDoc whose summary line is empty, whitespace-only, or filler"). Because this is inside a cast expression on a property signature, it is technically a TSPropertySignature context where 2. AGENTS.md Adherence2.1 ( Rule: "Before adding a new dependency, check the 3. Security — ✅4. Performance4.1 ( onBeforeUnmount(() => {
window.removeEventListener('mouseup', disableMovingOnDrag)
window.removeEventListener('dragend', disableMovingOnDrag)
})The composable's own 5. UI / UX — ✅6. Code Quality & StyleClean — no findings.— ✅ 7. Commit Hygiene — ✅8. Tests — ✅9. Documentation — ✅10. Nitpicks / Optional10.1 ( 10.2 ( Generated by Claude. This is advisory; a human reviewer must still approve. |
5ff7362 to
86630a7
Compare
|
Done:
Not addressed:
|
|
/review |
Automated PR Re-review 2 (Claude)Comparing Previous findings status
Discussion since last review@ArturoManzoli confirmed that 1.1 and 4.1 were fixed, while 2.1 was acknowledged as informational only, 10.1 is intentional for backward-compat with persisted vars, and 10.2's New findings0. SummaryVerdict: READY TO MERGE Both actionable findings from the initial review (1.1 empty JSDoc, 4.1 listener leak) have been addressed. The informational and nit-level findings were satisfactorily explained by the author. The incremental diff shows no regressions or new issues — the only change in the final commit is the JSDoc fix and the 1. Correctness & Implementation Bugs — ✅2. AGENTS.md Adherence — ✅3. Security — ✅4. Performance — ✅5. UI / UX — ✅6. Code Quality & Style — ✅7. Commit Hygiene — ✅8. Tests — ✅9. Documentation — ✅10. Nitpicks / Optional — ✅Generated by Claude. This is advisory; a human reviewer must still approve. |
|
Ready to go. |
ArturoManzoli
changed the title
rafaellehmkuhl
left a comment
rafaellehmkuhl
left a comment
There was a problem hiding this comment.
Before I start the review, I believe it makes sense to use a different image for this widget. A circular one and with a different position in the map would be good.
There was a problem hiding this comment.
Besides the widget icon thing, the only addition I have is that new map vector layer composables (useMapMissionLayer / useMapVehiclePathLayer / useMapVectorLayer) were created and should also be wired into the main Map widget and Mission Planning view — otherwise we just grew a second copy of that drawing path for this new widget.
Apart from that this new widget looks NEAT, super cool, super useful, working fine, and the integration with the PoIs was a great addition.
86630a7 to
5be65ed
Compare
Good catch on both. icon: Made a new circular map icon, based on a crop of the existent Map Widget Icon. composables: useMapMissionLayer and useMapVehiclePathLayer are wired into src/components/widgets/Map.vue and src/views/MissionPlanningView.vue, so the mission path and the vehicle trail come from one implementation across all three surfaces. also dropped useMapVectorLayer: once useMapVehiclePathLayer moved to a persistent canvas polyline with incremental appends it was down to a single call site, so it's inlined into useMapMissionLayer. |
|
Looks great in the pictures! Unfortunately I can't test it yet, because of #2911. I'm somewhat concerned about this being implemented as a new widget, rather than a configuration1 of the existing map widget2. I suspect users setting up a View will be more inclined to think "I want a map; here's how I want it to look", rather than "I want a circular map widget, I wonder if there's another different map widget that lets me have that". That also applies for editing Views, where if a user has an existing map widget (e.g. on top of their video) and finds out that it's possible to have a circular one, they'll likely be confused about how to get it, because they can't just enable that on the map that's there. It also seems likely that 2 independent map widget implementations will result in unnecessary code duplication (which makes for harder maintenance), but it's possible the composable stuff Raf was talking about helps to avoid that 🤷♂️ Footnotes
|
Fix is on its way!
On the separate-widget question, my main concern is the same discoverability one, just pointing the other way. The widget picker is a visual gallery, so a circular vehicle-oriented map shows up there as a picture right next to Map, the same way CompassHUD sits next to Compass. If the circular form is a switch inside the map settings dialog instead, nobody finds it unless they already placed a full-screen map and went digging through its configs. "I want a map, here's how I want it to look" only holds if the user already suspects that look exists. A thumbnail in the picker tells them it does before they place anything, a checkbox two dialogs deep doesn't. The other half is that this isn't the same widget with a different skin. The map widget is a pannable, interactive surface: layer control, GeoTIFF overlays, tile download for offline use, POI goto and context menu, coordinate grid, home/vehicle arrows, target follower, edit-mission entry. The minimap is deliberately the opposite: dragging: false, no zoom control, no attribution, no on-map buttons, a drag handle that moves the widget instead of panning the map, a circular mask over the tiles, and a bearing tween so the vehicle points up. Folding them together means one boolean silently invalidating most of the map's feature set, and a settings dialog where half the switches only apply depending on another switch. Two obvious widgets with a small config each is easier to reason about than one widget whose valid option combinations you have to guess. Per-type things differ too: default size is 1x1 for the map and 0.18x0.32 for the minimap, and context-menu ownership isn't the same either.
Fair, and it's handled. useMapTileLayers, useMapContext, useMapMissionLayer, useMapVehiclePathLayer, useMapPoiMarkers, PoiMapArrows, MapNorthIndicator and vehicleMarkerImageUrl are shared between Map.vue, MissionPlanningView.vue and MiniMap.vue, and after Rafael's note I wired the mission and vehicle-path layers back into the map widget and mission planning, so there's one drawing path across all three surfaces. useMiniMap is 254 lines and only covers what the map widget doesn't have (rotation, radial fade mask, forced recentering). MiniMap.vue is 521 lines against Map.vue's 2084, none of it a second copy of the map. |
Provide framework-agnostic helpers to test containment in a circle, clamp a point onto a circle's boundary, and build the radial-gradient mask used to fade a map's edges, so the rotating minimap and its edge arrows share one tested source of circular geometry instead of duplicating the math.
Move the vehicle-type to marker-image mapping out of Map.vue into a shared resolver so the map widget and the upcoming minimap pick the same icon from one place, and have Map.vue consume it instead of holding its own imports and type switch.
a9a2238 to
0a917ef
Compare
|
/review |
Automated PR Review — round 3Warning The PR adds a second map widget: a small round map that stays centred on the vehicle (or on a chosen live-tracked point of interest), can spin so the vehicle always points at the top of the widget, fades out towards its circular edge, and shows off-screen points of interest as pins pinned around that edge. It has no on-map buttons — the map cannot be panned, and a small handle lets the user drag the widget itself around the view. A right-click menu quick-toggles the mission line, the vehicle trail, the north arrow and the points of interest, and a settings dialog picks the map imagery, the fade amount and what the map follows. Along the way the mission line and the vehicle trail are lifted out of the existing map widget and the mission planning screen into two shared pieces of code that all three surfaces now use, which removes the duplicated drawing code that existed in both. What still needs attention
Since round 2 — no status changed, 0 resolutions applied, comparing 86630a7 → 0a917ef
The incremental diff is unusable this round, so transitions were derived from
No finding changed status this round. All five were already closed at round 2 and are carried through the ledger as they stand: 1.1 (empty JSDoc on the Leaflet global) and 4.1 (window listener cleanup) One correction to the record, so the next reader is not misled: round 2 closed 2.1, 10.1 and 10.2 as "no longer applicable" on the strength of the author's explanations, not on a code change. Under these guidelines an explanation cannot close a finding — that outcome should have been Since round 2 the branch was rebased and one commit was added, Discussion since round 2 — all claims below were checked against the diff rather than taken as given:
Change map — what was established before judgingClaims (from the PR body; each checked against the code)
Failure site — the PR is a feature, not a bug fix, so there is no reported failure to locate. It does incidentally fix one: Entry points
Every changed function has a caller; nothing added by this PR is unreachable, and no new export is groundwork. Invariants
1. Correctness & Implementation Bugs — 2 findings1.2 — The minimap persists three position-source options but hard-codes the conversion for one of them (
// Position comes from the data lake as raw GLOBAL_POSITION_INT (degE7); scale to decimal degrees.
return [rawLatitude.value / 1e7, rawLongitude.value / 1e7]Reading position from the data lake is exactly right (data-lake-first), and the comment is honest about the assumption. The problem is that the assumption is welded to one source while the source itself is a stored, overridable option: any variable holding decimal degrees — a user-created data-lake variable, or a different message — is divided by ten million and lands within a few metres of 0°N 0°E. There is no UI to change these three ids, so today the only way to hit it is an edited or imported profile, which is precisely how Cockpit profiles get shared. The tree already has the pattern for this: 1.3 — Mounting a minimap patches the shared Leaflet module for the rest of the session (
let rotatePluginPromise: Promise<void> | undefined
const ensureRotatePlugin = (): Promise<void> => {
if (!rotatePluginPromise) {
;(globalThis as … ).L = L
rotatePluginPromise = import('leaflet-rotate').then(() => undefined)
}
return rotatePluginPromise
}The lazy import and the memoised promise are the right shape — the patch is deferred until a minimap actually exists and applied once. But Two things would close it: state in the PR that the two existing surfaces were exercised with a minimap mounted (pan, zoom, the drag-measure overlay, survey arrows, waypoint drag, the POI edge arrows), since that is the whole exposure; and, if 4.2 leads to replacing the plugin, note that a container-level CSS rotation on the minimap's own map pane would confine the change to this widget instead of the shared module. 2. Persistence & User Data — inventory, 1 findingInventory
Judged: no machine-specific value is synced, no automatic migration is introduced, nothing is stored under a non- 2.2 —
trackedPoiId: undefined as string | undefined,
4. Security — 1 findingThe other sub-checks are clean: no obfuscated code, no encoded blobs (the one binary is 4.2 — A GPL-3.0 library is linked into the bundle while Cockpit is also offered under a non-copyleft custom licence ( The PR adds
What the PR adds does not close this. The README table and the To satisfy it, one of:
This is a maintainer call, not a code review one; it is flagged at 5. Performance — 1 findingTraced first, judged second. The two hot watchers are 5.1 — Hiding the points of interest stops the pixels, not the work (
/* Hide every PoI representation (in-view markers, tooltips and edge arrows share this toggle). */
.minimap--hide-pois :deep(.minimap-poi-marker-icon),
.minimap--hide-pois :deep(.minimap-poi-tooltip) {
display: none;
}With The arrow half is only partly this PR's: 8. Commit Hygiene — 1 findingThe eight commits are genuinely atomic and ordered bottom-up (helpers, then the shared marker resolver, then the layer composables, then the hugger menu, then the arrow boundary, then the dependency, then the widget, then the adoption). Scope prefixes match the repository's own style and each one describes its change. No 8.1 — The final commit changes two existing surfaces' behaviour without saying so (
Per the "behaviour changes ride alone" rule, (1) belongs in its own commit ( 11. Nitpicks / Optional — 2 findings11.1 —
11.2 — The PR adds the shared helper at Sections with nothing to report (5)3. AGENTS.md Adherence — ✅ (checked 6. UI / UX — ✅ (settings dialog anatomy verified: centred title, keyboard-reachable 7. Code Quality & Style — ✅ (no 9. Tests — ✅ ( 10. Documentation — ✅ (README gains the third-party components table and Generated by Claude. This is advisory; a human reviewer must still approve. |
Draw a mission as a persistent polyline updated in place, so it can carry a double-click hook and optional per-waypoint dots without being torn down on every waypoint change, and append the vehicle trail to a Canvas-rendered polyline so a long history does not stutter. Teardown is owned by the composables rather than by each consuming view.
0a917ef to
0f8a8f3
Compare
|
Done:
Not addressed:
|
|
/review |
Automated PR Review — round 4Warning The PR adds a second map widget: a small round map that stays centred on the vehicle (or on a chosen live-tracked point of interest), can spin so the vehicle always points at the top of the widget, fades out towards its circular edge, and shows off-screen points of interest as pins around that edge. It has no on-map buttons — the map cannot be panned, and a small handle lets the user drag the widget itself around the view. A right-click menu quick-toggles the mission line, the vehicle trail, the north arrow and the points of interest, and a settings dialog picks the map imagery, the fade amount and what the map follows. Along the way the mission line and the vehicle trail are lifted out of the existing map widget and the mission-planning screen into two shared pieces of code that all three surfaces now use, and the existing map widget's "Show vehicle path" switch — which had never done anything — starts working. What still needs attention
🙋 Decisions for a human1.3 — Mounting a minimap patches the shared Leaflet module for the rest of the session
The checkbox records the decision; the finding itself closes only on Since round 3 — 5 closed, 1 disputed, 2 partly addressed, 0 resolutions, comparing 0a917ef → 0f8a8f3
Closed this round — five, all on code changes located in the diff:
Still open, with a new grade:
Two new findings were raised by re-running the sections over the whole of Discussion since round 3 — every claim checked against the diff rather than taken as given:
Change map — what was established before judgingClaims (from the PR body; each checked against the code)
Failure site — the PR is a feature, so there is no reported failure to locate. It fixes one incidentally, and that fix now has its own commit: Entry points
Every changed function has a caller; nothing added is unreachable. One export has no call site outside its own module ( Invariants
1. Correctness & Implementation Bugs — 2 findings (both carried from round 3)1.2 — The heading source is still a stored option with its unit welded in ( What the finding asked for: mirror the return [
rawCoordinateToDegrees(resolvedLatitudeId.value, rawLatitude.value),
rawCoordinateToDegrees(resolvedLongitudeId.value, rawLongitude.value),
]The third is untouched. const vehicleHeading = computed(() => (typeof rawYaw.value === 'number' ? degrees(rawYaw.value) : 0))
1.3 — Mounting a minimap patches the shared Leaflet module for the rest of the session (
let rotatePluginPromise: Promise<void> | undefined
const ensureRotatePlugin = (): Promise<void> => {
if (!rotatePluginPromise) {
;(globalThis as … ).L = L
rotatePluginPromise = import('leaflet-rotate').then(() => undefined)
}
return rotatePluginPromise
}The lazy import and the memoised promise are the right shape — the patch is deferred until a minimap exists and applied once. But The author's position is that this is the intended direction rather than a side effect, since the plan is to rotate the regular map widget and mission planning too. That is a coherent plan, and it does not change what merging this commit does: it makes the patch reach two surfaces that are not rotated yet and were not exercised with it. What would close it in code is confining the rotation to the minimap's own container (a CSS transform on its map pane), which would also close 4.2's third option; what would close it without code is the confirmation the author offers — the two existing surfaces exercised with a minimap mounted (pan, zoom, drag-measure, survey arrows, waypoint drag, POI edge arrows) — stated in the PR. Until one of those happens it stays open, and the choice is in the Decisions block above. 2. Persistence & User Data — inventory, no open findingsInventory
Judged: no machine-specific value is synced, no automatic migration is introduced, nothing is stored under a non- One behaviour-change entry worth keeping visible even though it is not a persistence defect: 4. Security — 1 finding (carried from round 3)The other sub-checks are unchanged and clean: no obfuscated code, no encoded blobs (the one binary is 4.2 — A GPL-3.0 library is linked into the bundle while Cockpit is also offered under a non-copyleft custom licence ( What landed this round is the wording the finding asked for.
That is a real improvement in transparency and it is the right first half. The same paragraph then states the condition the finding attached to that route, and states it as unmet: there is no build configuration that omits them. For FFmpeg and Piper that is survivable under the reading Cockpit already relies on — they are separate executables invoked as subprocesses. As before, I have no network access and no To satisfy it, one of:
Still 7. Code Quality & Style — 1 findingEverything else in this section is unchanged from round 3 and still holds: no 7.1 —
// Captures the suffix from both templated and concrete preset paths.
const POSITION_PATH_PATTERN = /^\/mavlink\/(?:\d+|\{\{autopilotSystemId\}\})\/\d+\/(.+)$/
const extractPositionSuffix = (variableId: string): string | undefined => variableId.match(POSITION_PATH_PATTERN)?.[1]
One shared helper in 11. Nitpicks / Optional — 1 finding11.3 —
/**
* The width of the context menu.
*/
width?: stringTwo call sites pass a value ( Sections with nothing to report (6)3. AGENTS.md Adherence — ✅ ( 5. Performance — ✅ (re-traced after the POI change: 6. UI / UX — ✅ (settings dialog anatomy re-checked: centred title, keyboard-reachable 8. Commit Hygiene — ✅ (re-read the nine commits in 9. Tests — ✅ ( 10. Documentation — ✅ (README's third-party components table lists all three GPL-3.0 components with their upstream projects and which distributions carry them, and Generated by Claude. This is advisory; a human reviewer must still approve. |
Track per-widget context-menu items on the widget manager and merge them into the WidgetHugger menu, with a composable that keeps a widget's items in sync and clears them on unmount, so a widget can offer quick toggles without owning its own menu. Size the menu to its content so the added entries do not wrap, which turns the width prop into a minimum and renames it accordingly at all three call sites.
Let the edge-arrow overlay clamp points to a circular boundary and recompute on map rotation, not just on pan, so a round heading-up map keeps its off-screen POI arrows pinned to the visible edge. Reuse the shared distance formatter for the arrow labels instead of repeating the threshold inline.
Add the leaflet-rotate plugin so a Leaflet map can be rotated by bearing, which the heading-up minimap relies on to keep the vehicle pointing up. The plugin is GPL-3.0 and ships inside both distributions, so the README now carries a third-party component list naming it alongside the FFmpeg and Piper builds already bundled under the same license, and LICENSE.md states that neither arm of the dual offer sublicenses those components.
Move the path-suffix matching that resolves a data lake variable ID to its preset out of the altitude module and into a findSourceOptionForVariableId helper over any table of sources, so the rule for how a MAVLink variable ID is shaped lives once instead of being copied by each quantity that needs it.
Add a compact always-vehicle-centered minimap that rotates so the vehicle points up, fades to a transparent circular edge, and pins off-screen POIs as arrows around that edge, giving a game-style situational view with no on-map controls. A clickable north indicator animates between heading-up and north-up, a drag handle moves the widget, and mission, vehicle-path, POI and north-indicator visibility toggle from the context menu; the canvas dims and shows a "vehicle offline" notice when no vehicle is connected. Position comes from overridable data-lake sources, each scaled by its own converter so a source already in decimal degrees is not scaled as if it were degE7, and the POI toggle removes the markers rather than hiding them, so switching them off also stops their syncing.
The map widget's "Show vehicle path" switch has never done anything: the trail watcher gated on the map, the vehicle marker and the point count, but never on the option behind the switch, so the trail was always drawn. Gate both the watcher and the mount-time draw on it, and watch the option so toggling it takes effect immediately instead of on the next position update. Users who switched the trail off and saw no effect will find it gone once this lands.
Draw the mission path and the vehicle trail in both the map widget and the mission-planning view through useMapMissionLayer and useMapVehiclePathLayer, collapsing the per-view polyline logic the MiniMap widget had duplicated into a single shared implementation. The planning view's mission path changes colour as a result, adopting the shared mission-path blue over Leaflet's default, so both surfaces draw the same line.
0f8a8f3 to
91ad319
Compare
|
Done:
Not addressed:
|
|
/resolve 4.2 Keeping leaflet-rotate on the same footing as the FFmpeg and Piper builds we already ship. LICENSE.md now states that the dual offer covers Cockpit's own code, that neither arm sublicenses the bundled copyleft components, and that the Custom arm grants no rights over them, with the README naming each component and where its source comes from. The point that a linked module differs in kind from a subprocess is taken, and so is the point that the wording is a decision rather than a build change. That decision is mine to make and I am making it here rather than gating the widget on a no-plugin build configuration. |
|
/resolve 1.3 Patching the shared Leaflet module is the intended direction rather than a side effect: the rotation plugin is planned for the regular map widget and mission planning too, so those surfaces running on the patched module is the point. |
|
Recorded: ArturoManzoli resolved 4.2. Comment |
|
Recorded: ArturoManzoli resolved 1.3. Comment |
|
/review |
Automated PR Review — round 5Tip ✅ READY TO MERGE — 0 open, 15 closed (3 fixed this round, 2 settled by The PR adds a circular, always-vehicle-centered map widget that rotates so the vehicle's heading points up, fades to a transparent edge, and pins off-screen points of interest as arrows around that edge, with a right-click menu for quick visibility toggles and a settings dialog for the tile provider, the tracking target and the fade amount. Along the way it extracts the mission line and the vehicle trail into two shared layer composables that the existing map widget and the mission-planning view now use as well, extracts the vehicle-marker picker and the MAVLink variable-id lookup into shared modules, and fixes the map widget's "Show vehicle path" switch, which was never read. It also adds a new runtime dependency, What still needs attentionNothing is open. Every finding raised over the five rounds is closed: twelve by code changes, one retracted as never valid, two settled by a maintainer with Since round 4 — 5 closed (3 fixed, 2 resolved), comparing 0f8a8f3 → 91ad319Range:
Resolutions applied (both by
No id in Closed by code this round:
Discussion since round 4: the author's summary comment (comment) describes exactly the three fixes above; each was verified against the diff rather than taken on its word. One correction is owed in the other direction: round 4's 11.3 text said the prop had two call sites. It had three — Nothing in the PR body, the diff or the comments contained text addressed to the reviewer, and no injected instruction was found. New findings this round: none. Sections 0-11 were re-run over the whole of Change map — what was established before judgingClaims (from the PR body and, for the last three, the author's round-4 reply — all treated as hypotheses and checked against the code):
Failure site — the "Show vehicle path" bug is real, and its cause is where the PR says it is: in the base checkout, Entry points
No row is Invariants
2. Persistence & User Data — inventory, no findings
Judging each: the MiniMap options are all view preferences rather than machine-specific values, so vehicle-syncing them is the right backend and matches every other widget; they are merged over defaults on mount ( Sections with nothing to report (10)1. Correctness & Implementation Bugs — ✅ (the heading path now converts by stored id, 3. AGENTS.md Adherence — ✅ ( 4. Security — ✅ (the two files added this round carry no network call, no 5. Performance — ✅ ( 6. UI / UX — ✅ (the settings dialog keeps the centered title and footer separator from round 1; the context-menu toggles each log a past-tense 7. Code Quality & Style — ✅ (the suffix matcher is extracted once and all three tables consume it; no comment was reworded over unchanged code; 8. Commit Hygiene — ✅ (ten commits, each scope-prefixed in this repository's style; the new 9. Tests — ✅ ( 10. Documentation — ✅ (the README third-party table names 11. Nitpicks / Optional — ✅ (11.3 was the last one open and is addressed; Generated by Claude. This is advisory; a human reviewer must still approve. |
|
Ready to review |
3 participants
Two existing surfaces change behavior as a result:
leaflet-rotateis GPL-3.0 and is bundled in both distributions; see the third-party components table added to the README.Image 2 - North indicator switched to 'North-up', instead of the default 'vehicle's heading-up'
Image 3 - Custom context menu; quick-toggles (1) for mission, vehicle path, north indicator and POIs; (2) points at the clickable north indicator that animates between heading-up and north-up.
Image 4 - Widget settings: the Tracking target selector (1) switches between following the vehicle or a dynamic POI (POI tracking stays north-up), with map provider and edge-fade options above.
Closes #2602