Mission Planning: Add free map placement to mission library items by ArturoManzoli · Pull Request #2802 · bluerobotics/cockpit

ArturoManzoli · 2026-06-22T17:42:10Z

Add a "Reposition on map" option to the library load dialog that drops the saved mission onto the planning map at 1:1 scale with interactive transforms before commit.
Drag, scale and rotate the mission item before merging to map.
Add an "Insert mission from library here" entry to the segment radial menu so a library mission can be spliced between two existing waypoints.
Add a "Mission library" submenu to the map context menu, holding "Add mission from library" and "Save mission to library", so a saved mission can be inserted or the current one saved directly from a right-click.
Bias the context-menu adds (waypoint, survey, simple path, library mission) toward the closer mission endpoint: clicks near the start prepend instead of always appending.
Highlight the first and last waypoints of the current mission with an orange ring so the endpoints are obvious before inserting near them.
Move the floating-panel screen-placement maths to libs/map/screen-placement, shared by the new placement toolbar and the pre-existing survey-confirm strip.

Diff: $\color{green}\pmb{+2{,}041}$ $\color{red}\pmb{-153}$

Screenshare.-.2026-06-22.1_37_36.PM.mp4

Second and final PR splitting #2654.

ArturoManzoli · 2026-06-22T17:56:03Z

/review

github-actions · 2026-06-22T17:59:05Z

Automated PR Re-review 1 (Claude)

No previous review exists — performing a full review of pr.diff against fecb133fc63b.

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

Items worth attention: 6.1, 6.2, 7.1.

This PR adds a Mission Library feature to the mission planner: users can save, load, import/export, and delete missions from a local library. It introduces free-placement mode (drag, scale, rotate on the map) when loading a library mission, a new MissionLibraryModal.vue component, a useMissionPlacement composable, a src/libs/mission/library.ts helper, and extends the mission store and types. The toolbar replaces the old save/load-to-file buttons with a library icon, and the segment radial menu gains an "Insert mission from library here" entry.

1. Correctness & Implementation Bugs

1.1 (minor) — MissionLibraryModal.vue — In onImportFileSelected, when importing a bare CockpitMission (the instanceOfCockpitMission branch), the file is saved to the library but the <input> element's value is only cleared inside the finally block of the outer try. If the saveMissionToLibrary call itself throws (e.g. a storage quota error), the snackbar in the catch will display, but input.value = '' still runs (correctly) because it's in finally. This is fine — just noting the import-error path is handled. No issue here.

1.2 (minor) — useMissionPlacement.ts:1235 — JSON.parse(JSON.stringify(mission)) is used to deep-clone the mission for placement. The comment justifies this by noting structuredClone can fail on Pinia reactivity proxies. However, buildCurrentMissionSnapshot in the view already uses structuredClone(toRaw(...)). Since the mission passed to startFreePlacement comes from a SavedMission (which lives in a useBlueOsStorage ref and is also a Pinia-store reactive), toRaw + structuredClone would be more robust (handles undefined, NaN, Infinity correctly, unlike JSON). Consider aligning with the structuredClone(toRaw(...)) pattern used elsewhere in this PR.

1.3 (minor) — useMissionPlacement.ts — The rotation handle's geographic position is computed from a screen-space pixel offset (PREVIEW.rotationHandleOffsetPx) converted back to lat/lng. This means the rotation handle's geographic position shifts when the user zooms (the zoomend rebuild fixes this), but during a pinch-zoom gesture the handle will be momentarily misaligned until zoomend fires. This is cosmetic and likely acceptable.

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance

4.1 (nit) — MissionPlanningView.vue — watchDebounced on currentPlanningWaypoints and currentPlanningSurveys with { deep: true } fires a structuredClone on every reactive change to any waypoint coordinate, altitude, or survey vertex. For very large missions (hundreds of waypoints) this deep watch + clone can be noticeable. The 100ms debounce mitigates it, but the snapshot is only consumed when the library modal is open. Consider gating the clone behind interfaceStore.isMissionLibraryVisible to avoid unnecessary work.

5. UI / UX — ✅

6. Code Quality & Style

6.1 (major) — MissionPlanningView.vue grows from ~4,417 to ~4,830 lines with this PR (net +413). AGENTS.md and the review guidelines flag Vue files past ~2,000 lines as candidates for extraction. The file was already well past this threshold, and this PR adds substantial new logic (placement toolbar positioning, cloneMissionForPlanning, appendMissionToPlanning, insertMissionIntoSegment, finalizeMissionPlacement, loadDraftMission option extension, snapshot watchers). Consider extracting the library-loading / placement-routing logic (everything from cloneMissionForPlanning through finalizeMissionPlacement) into a dedicated composable (e.g. useMissionLibraryPlacement) so the view only wires the hooks. This is consistent with the composable extraction done for useMissionPlacement.

6.2 (minor) — src/stores/mission.ts — saveMissionToLibrary and deleteSavedMission are arrow-function consts, so jsdoc/require-jsdoc with ArrowFunctionExpression: false does not mandate JSDoc. However, AGENTS.md §4 says "Always create docs for the @returns" for public functions. Both functions are returned from the store (public API). saveMissionToLibrary returns a SavedMission without JSDoc; deleteSavedMission returns void. At minimum saveMissionToLibrary should document its return value. The inline JSDoc on the payload fields is good, but the function itself has none.

6.3 (nit) — src/libs/mission/library.ts:1433-1438 — utf8ToBase64 is a small utility. btoa(binary) on a per-byte-assembled string works, but the project already has @zip.js/zip.js in deps which provides base64 codecs. This is fine as a self-contained helper — just noting the project's base64 landscape.

7. Commit Hygiene

7.1 (minor) — 11 of the 15 commits are tagged [drop], signalling they belong to the parent PR (#2795) and will be removed before merge. This is standard for stacked PRs, but the review tooling (and future git log) should not contain these. Confirm that these commits will be rebased out before merging this PR so the final branch history only contains the 4 new commits (57c92c1e, af306424, 84d2bd69, fecb133f).

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional

10.1 (nit) — useMissionPlacement.ts and src/libs/mission/library.ts both define const METERS_PER_DEGREE_LAT = 111320. Consider exporting the constant from library.ts and importing it in the composable to keep a single source of truth.

10.2 (nit) — MissionLibraryModal.vue:504 — The destructuring const { id: _id, ...exportable } = mission with the eslint-disable for @typescript-eslint/no-unused-vars is fine, but you could use const { id: _, ...exportable } = mission (single underscore) which most TS/eslint configs already ignore without the directive. Minor stylistic preference.

Generated by Claude. This is advisory; a human reviewer must still approve.

ArturoManzoli · 2026-06-22T22:59:44Z

/review

github-actions · 2026-06-22T23:01:44Z

Automated PR Re-review 2 (Claude)

Comparing fecb133fc63b → d42bfb549ac6 (3 new commits: context-menu library submenu, endpoint highlighting, and endpoint-biased insertion).

Previous findings status

#	Finding	Severity	Status
1.1	`MissionLibraryModal.vue` — import error path / input clearing logic	minor	⚪ No longer applicable — no changes to this file in the incremental diff; finding was informational only
1.2	`useMissionPlacement.ts` — `JSON.parse(JSON.stringify())` vs `structuredClone(toRaw())`	minor	❌ Not addressed
1.3	`useMissionPlacement.ts` — rotation handle misalignment during pinch-zoom	minor	⚪ No longer applicable — cosmetic / informational, no code change expected
4.1	`MissionPlanningView.vue` — deep `watchDebounced` snapshot runs even when library is closed	nit	❌ Not addressed
6.1	`MissionPlanningView.vue` >2000 lines, extract library logic to composable	major	:large_yellow_circle: Partially addressed — the new commits add ~170 more lines to the view (+endpoint highlighting, context-menu wiring, insertion helpers), further enlarging it. No extraction happened.
6.2	`src/stores/mission.ts` — `saveMissionToLibrary` missing JSDoc on the function itself	minor	❌ Not addressed
6.3	`src/libs/mission/library.ts` — `utf8ToBase64` overlaps with existing deps	nit	⚪ No longer applicable — informational
7.1	11 `[drop]` commits should be rebased out before merge	minor	❌ Not addressed — still 11 `[drop]` commits in the branch
10.1	Duplicated `METERS_PER_DEGREE_LAT` constant	nit	❌ Not addressed
10.2	`_id` eslint-disable stylistic preference	nit	⚪ No longer applicable — stylistic only

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

Items worth attention: 1.4, 6.1 (ongoing).

The three new commits add a context-menu submenu for the mission library (add from library / save to library), highlight the first and last mission waypoints in orange to signal endpoints, and bias context-menu inserts (waypoint, survey, simple path, library mission) toward the closer mission endpoint — clicks near the start prepend instead of always appending.

1. Correctness & Implementation Bugs

1.4 (minor) — MissionPlanningView.vue — addMissionFromLibraryContextMenu uses pendingSegmentInsertIndex.value = -1 as a sentinel for "insert at the start", but openMissionLibrary() is called first, which sets pendingSegmentInsertIndex.value = null. The code then immediately overwrites it:

openMissionLibrary()
const insertIndex = getContextMenuEndpointSplicePosition()
if (insertIndex === 0) pendingSegmentInsertIndex.value = -1

This works because the assignment on the very next line happens synchronously before any watcher fires. However, it relies on openMissionLibrary being synchronous and not dispatching a microtask or nextTick that reads the value. Currently it is synchronous, so the logic is correct, but it's fragile — a comment or a small guard at the openMissionLibrary call site noting this intentional ordering would prevent a future refactor from breaking it.

1.5 (nit) — MissionPlanningView.vue — getEndpointSplicePositionForLatLng uses geographic distanceTo (haversine meters) to decide whether the cursor is closer to the start or end waypoint. This is correct for the intended purpose. Just noting: when the first and last waypoints are equidistant, < returns 0 (prepend), which silently biases toward prepending. This is fine UX-wise.

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX

5.1 (nit) — ContextMenu.vue — The submenu chevron icon uses class="fixed right-0". Using position: fixed inside a Vuetify menu (which is rendered in a portal/overlay) can cause the chevron to jump to the far right edge of the viewport on some screen sizes or when the context menu opens near the right edge. Consider absolute or relative positioning within the list item instead.

6. Code Quality & Style

6.1 (ongoing, major) — MissionPlanningView.vue — The file continues to grow (now ~5,000+ lines post-PR). This batch of commits adds ~170 net lines of endpoint-biased insertion logic (getEndpointSplicePositionForLatLng, getEndpointInsertIndexForLatLng, getContextMenuEndpointSplicePosition, addSimplePathFromContextMenu, addSurveyFromContextMenu, addMissionFromLibraryContextMenu, addWaypointFromContextMenu expansion, isEndpointWaypoint, createWaypointMarkerHtml changes, etc.). The marker HTML generation and endpoint detection helpers are pure functions that could live in a shared utility or composable. The original finding still applies.

6.4 (nit) — MissionPlanningView.vue — The comment // NEW: hide/show the live pill when hovering survey nodes/add/delete UI was removed from renderMeasureOverlay. Since the code it documented is still present (just refactored with an evt && guard), this is fine and matches AGENTS.md rule that comments on unchanged code should stay — here the code did change.

7. Commit Hygiene

7.2 (nit) — The 3 new commits are well-scoped (context-menu submenu, endpoint highlighting, endpoint-biased insertion). Each is a distinct logical unit. Good.

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional

10.3 (nit) — MissionPlanningView.vue — getEndpointInsertIndexForLatLng is a one-liner wrapper that converts null → undefined via ?? undefined. It's only used once (in addWaypointFromContextMenu). Consider inlining it or at least documenting why the null → undefined translation is needed (answer: addWaypoint's insertIndex param is typed number | undefined). A small JSDoc or inline comment would clarify the intent.

Generated by Claude. This is advisory; a human reviewer must still approve.

ArturoManzoli · 2026-06-22T23:09:32Z

1.4 (minor) — MissionPlanningView.vue — addMissionFromLibraryContextMenu uses pendingSegmentInsertIndex.value = -1 as a sentinel for "insert at the start", but openMissionLibrary() is called first, which sets pendingSegmentInsertIndex.value = null

Skipping this one, since MissionPlanningView.vue at line 2015 already has the "set the sentinel afterwards" comment at the call site, which is exactly the remediation suggested.

ArturoManzoli · 2026-07-01T20:58:16Z

/review

github-actions · 2026-07-01T21:00:59Z

Automated PR Re-review 3 (Claude)

Comparing d42bfb549ac6 → 48c8e4c4821e. The incremental diff included unrelated changes from a rebase (.eslintrc.cjs, AGENTS.md, index.html, package.json changes not part of this PR), so this re-review is based on the full pr.diff (base…head).

Previous findings status

#	Finding	Severity	Status
1.2	`useMissionPlacement.ts` — `JSON.parse(JSON.stringify())` vs `structuredClone(toRaw())`	minor	:large_yellow_circle: Partially addressed — `buildCurrentMissionSnapshot` now uses `structuredClone(toRaw(...))`, but `startFreePlacement` (line ~1338 of the composable) still uses `JSON.parse(JSON.stringify(toRaw(mission)))`
1.4	`addMissionFromLibraryContextMenu` sentinel ordering fragility	minor	✅ Addressed — author confirmed the call-site comment already exists per comment
1.5	Equidistant bias toward prepending	nit	⚪ No longer applicable — informational only
4.1	`watchDebounced` snapshot runs even when library is closed	nit	❌ Not addressed — the `watchDebounced` on `currentMissionSnapshot` still fires continuously regardless of whether the library modal is open
5.1	`ContextMenu.vue` — `fixed right-0` on submenu chevron	nit	❌ Not addressed — still uses `class="fixed right-0"`
6.1	`MissionPlanningView.vue` >2000 lines	major	❌ Not addressed — the file continues to grow significantly with this PR
6.2	`saveMissionToLibrary` in `src/stores/mission.ts` missing function-level JSDoc	minor	❌ Not addressed — the function body has inline param JSDoc but no function-level `/** */` block
7.1	`[drop]` commits should be rebased out before merge	minor	:large_yellow_circle: Partially addressed — reduced from 11 to 2 `[drop]` commits (`26217d40` and `97e74331`), but 2 remain
10.1	Duplicated `METERS_PER_DEGREE_LAT` constant	nit	❌ Not addressed — defined in both `src/libs/mission/library.ts` and `src/composables/map/useMissionPlacement.ts`
10.3	`getEndpointInsertIndexForLatLng` one-liner wrapper	nit	❌ Not addressed

Discussion since last review

ArturoManzoli noted that finding 1.4 (sentinel ordering in addMissionFromLibraryContextMenu) already has a comment at the call site, which is the exact remediation suggested. Marking it addressed.

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

Items worth attention: 5.2 (major), 6.1 (ongoing, major), 1.6.

This PR adds free-placement functionality to the mission library: users can drag, scale, and rotate a saved mission on the planning map before committing it. It also adds endpoint highlighting (first/last waypoints in orange), endpoint-biased insertion from the context menu, a "Planning for" vehicle-type selector for offline planning, and context-menu entries for save/load from the mission library. The code is well-structured with domain logic properly extracted into src/libs/mission/library.ts and the placement composable in src/composables/map/useMissionPlacement.ts.

1. Correctness & Implementation Bugs

1.6 (minor) — src/views/MissionPlanningView.vue — addWaypoint now accepts an optional insertIndex parameter, but addWaypointMarker (called from appendMissionToPlanning and insertMissionIntoSegment) does not pass insertIndex when adding to the leaflet marker map. The marker is added to the waypointMarkers dictionary keyed by id, so leaflet ordering is fine, but the marker is created without considering endpoint status at the time of batch insertion — isEndpointWaypoint(waypoint.id) is called during addWaypointMarker, which queries the array after the full splice, so endpoint status is correct. No bug here after closer inspection, withdrawing.

1.7 (minor) — src/views/MissionPlanningView.vue — pendingSimplePathInsertIndex is set when starting simple-path mode near the start endpoint, and every click in simple-path mode inserts at insertIndex ?? undefined. However, after the first click inserts a waypoint at index 0, subsequent clicks also insert at index 0, effectively building the path in reverse order. If the user clicks A, B, C while near the start, the waypoint order becomes [C, B, A, ...original...] rather than [A, B, C, ...original...]. This may be intentional (extending outward from the start), but if the user expects the clicks to build a path in the order clicked, this is surprising behavior. A brief comment explaining the reversed insertion order would help.

2. AGENTS.md Adherence

2.1 (minor) — Interaction logging: The closeModal function in MissionLibraryModal.vue (line ~396 of the diff) does not log a user action. The "close" button triggers it, and the watch(isVisible) path also fires, but neither calls logUserAction. Per AGENTS.md, dialog close events should be logged. Similarly, triggerImportFile (clicking the import button) is not logged before the file picker opens.

3. Security — ✅

4. Performance — ✅

5. UI / UX

5.2 (major) — Interaction logging: Several new discrete user interactions introduced in this PR are missing logUserAction calls, per the AGENTS.md rule:

MissionLibraryModal.vue: closeModal() (close button), triggerImportFile() (import button click), openSaveDialog() (save dialog open).
ContextMenu.vue: handleAddMissionFromLibrary(), handleSaveMissionToLibrary() — these emit events but do not log the user action at the point of click. The parent handlers (addMissionFromLibraryContextMenu, openMissionLibraryWithSaveDialog) do call openMissionLibrary which logs, so the context-menu variants are partially covered. However, the direct "Place mission on map" button in the library card grid (onLoadClick) already logs.
MissionPlanningView.vue: addWaypointFromContextMenu(), addSimplePathFromContextMenu(), addSurveyFromContextMenu() — these context-menu-triggered actions do not log.

6. Code Quality & Style

6.1 (ongoing, major) — MissionPlanningView.vue continues to grow well past ~2000 lines. The placement toolbar positioning logic (PLACEMENT_TOOLBAR_LAYOUT, updatePlacementConfirmButtonPosition, pickBestPosition, screenBounds), the mission cloning/splicing helpers (cloneMissionForPlanning, appendMissionToPlanning, insertMissionIntoSegment), and the endpoint detection/bias helpers are all pure functions or view-level orchestration that could be extracted into dedicated modules or composables. The placement composable extraction (useMissionPlacement) was a good step, but the view still owns significant non-trivial logic.

6.5 (minor) — src/stores/mission.ts — saveMissionToLibrary and deleteSavedMission are public store methods but lack function-level JSDoc blocks. The saveMissionToLibrary has inline param documentation in the payload type, but per .eslintrc.cjs jsdoc/require-jsdoc with FunctionExpression: false, this may not lint-error since they're arrow functions — but AGENTS.md still recommends JSDoc for public-facing functions.

7. Commit Hygiene

7.1 (updated, minor) — The branch now has 6 commits, of which 2 are marked [drop] (offline vehicle-type planning, mission library save/load). The PR description notes these are "borrowed" from another PR (#2795). This is significantly cleaner than before (was 11 [drop]), but ideally the [drop] commits should be rebased out before merge to keep the main branch history clean.

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional

10.1 (ongoing, nit) — METERS_PER_DEGREE_LAT is defined in both src/libs/mission/library.ts (line ~1746 in the diff) and src/composables/map/useMissionPlacement.ts (line ~807). The composable already imports from the library, so the duplicate could be dropped and the single definition reused.

10.4 (nit) — src/views/MissionPlanningView.vue — The v-select for "Planning for" vehicle type binds directly to missionStore.plannedVehicleType via v-model, and a separate @update:model-value="onPlannedVehicleTypeChange" handler logs the user action. Per AGENTS.md, this is the correct approach (handler-based logging avoids watch-triggered sync noise). Good.

Generated by Claude. This is advisory; a human reviewer must still approve.

ArturoManzoli · 2026-07-01T22:09:17Z

/review

github-actions · 2026-07-01T22:11:52Z

Automated PR Re-review 4 (Claude)

Comparing 48c8e4c4821e → 0e87529d424b. The incremental diff covers the changes since the last review. The previous review was based on a full pr.diff due to a rebase; this re-review cross-checks both the incremental and full diffs.

Previous findings status

#	Finding	Severity	Status
1.2	`useMissionPlacement.ts` — `JSON.parse(JSON.stringify())` vs `structuredClone(toRaw())`	minor	:large_yellow_circle: Partially addressed — `buildCurrentMissionSnapshot` now uses `structuredClone(toRaw(...))`, but `startFreePlacement` still uses `JSON.parse(JSON.stringify(toRaw(mission)))` (composable line ~56 in incremental)
1.7	Simple-path reverse insertion ordering near start endpoint	minor	⚪ No longer applicable — informational / design choice; the measure-line anchor was updated to reflect the intended behavior
2.1	`MissionLibraryModal.vue` missing `logUserAction` on `closeModal`, `triggerImportFile`	minor	❌ Not addressed — `closeModal` and `triggerImportFile` still lack `logUserAction` calls
4.1	`watchDebounced` snapshot runs even when library is closed	nit	❌ Not addressed — the `watchDebounced` on `currentMissionSnapshot` still fires continuously
5.1	`ContextMenu.vue` — `fixed right-0` on submenu chevron	nit	❌ Not addressed — still uses `class="fixed right-0"`
5.2	Missing `logUserAction` on several context-menu and view actions	major	:large_yellow_circle: Partially addressed — `openMissionLibrary` now logs contextually (save / segment-insert / generic open), but `addWaypointFromContextMenu`, `addSimplePathFromContextMenu`, and `addSurveyFromContextMenu` still lack `logUserAction` calls
6.1	`MissionPlanningView.vue` >2000 lines	major	❌ Not addressed — the file continues to grow with this PR
6.5	`saveMissionToLibrary` and `deleteSavedMission` missing function-level JSDoc	minor	❌ Not addressed — these arrow functions still have inline payload docs but no function-level `/** */` block
7.1	`[drop]` commits should be rebased out before merge	minor	✅ Addressed — still 2 `[drop]` commits (`26217d40`, `97e74331`), but these are borrowed from the prerequisite PR #2795 and are expected to be dropped during merge
10.1	Duplicated `METERS_PER_DEGREE_LAT` constant	nit	❌ Not addressed — still defined in both `src/libs/mission/library.ts` and `src/composables/map/useMissionPlacement.ts`
10.3	`getEndpointInsertIndexForLatLng` one-liner wrapper	nit	❌ Not addressed

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

Items worth attention: 6.1 (ongoing, major), 5.2 (partially addressed, major from previous review).

The changes since the last review are primarily structural: the large block of mission-merging logic (clone, append, segment-insert, fresh-load routing) has been extracted from MissionPlanningView.vue into a dedicated useMissionInsertion composable, and the placement flow is now factored into useMissionPlacement. The MissionLibraryModal was also extracted as a standalone component. These extractions are a positive step toward reducing the view's size and improving separation of concerns. The incremental diff also adds the "Reposition on map" dialog option, the context-menu library submenu, segment-insert from the radial menu, and endpoint-biased insertion for waypoints/surveys/simple-paths.

1. Correctness & Implementation Bugs — ✅

2. AGENTS.md Adherence

2.2 (minor) — Interaction logging: closeModal in MissionLibraryModal.vue (line ~394 of the diff) is a discrete user action (clicking the close button) but does not call logUserAction. Per AGENTS.md, dialog close events should be logged. The triggerImportFile handler similarly opens a native file picker — a user action — without logging.

3. Security — ✅

4. Performance — ✅

5. UI / UX

5.3 (minor) — Interaction logging (context-menu actions): The three new context-menu handler wrappers — addWaypointFromContextMenu, addSimplePathFromContextMenu, and addSurveyFromContextMenu — invoke the underlying logic without a logUserAction call. These are discrete user interactions (right-click → "Add waypoint at cursor", etc.) and should be logged per AGENTS.md.

6. Code Quality & Style

6.1 (ongoing, major) — MissionPlanningView.vue remains well past ~2000 lines. The extraction of useMissionInsertion and useMissionPlacement in this PR is good progress, but the view still hosts the placement toolbar positioning logic (PLACEMENT_TOOLBAR_LAYOUT, updatePlacementConfirmButtonPosition), the endpoint detection helpers (getEndpointSplicePositionForLatLng, getEndpointInsertIndexForLatLng, getContextMenuEndpointSplicePosition, isEndpointWaypoint), and the context-menu routing wrappers. These could be further extracted.

6.6 (nit) — getEndpointInsertIndexForLatLng (MissionPlanningView) is a one-liner that converts null → undefined. It is called in exactly one place (addWaypointFromContextMenu). The conversion could be inlined at the call site to avoid the extra indirection.

7. Commit Hygiene — ✅

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional

10.1 (ongoing, nit) — METERS_PER_DEGREE_LAT is still defined in both src/libs/mission/library.ts and src/composables/map/useMissionPlacement.ts. The composable already imports from the library, so the duplicate could be dropped and the canonical definition reused.

Generated by Claude. This is advisory; a human reviewer must still approve.

ArturoManzoli · 2026-07-02T13:26:22Z

Review follow-up — round 4

Done

src/views/MissionPlanningView.vue (5.2 + 5.3 — missing logUserAction on context-menu wrappers): addWaypointFromContextMenu, addSimplePathFromContextMenu, and addSurveyFromContextMenu each call logUserAction at the top of the wrapper now.

Won't change (with reasoning)

1.2 — startFreePlacement uses JSON.parse instead of structuredClone: intentionally used JSON.parse(JSON.stringify(toRaw(mission))) there — it matches the safeClone pattern in src/migration/default-profile-importer.ts and the inline comment in startFreePlacement points to it. both forms are proxy-safe once toRaw is applied first, so this is a consistency preference rather than a correctness fix. leaving as-is.

6.1 — MissionPlanningView.vue > 2000 lines: the previous push already extracted two composables (useMissionInsertion and useMissionPlacement), which pulled roughly 50% of the lines this PR added to the view back out of it. further extractions are a legitimate follow-up, not something to keep expanding this PR with.

Note: 2.1 + 2.2 (closeModal/triggerImportFile logging) and 6.5 (JSDoc on saveMissionToLibrary/deleteSavedMission) touch files owned by PR #2795's [drop] commits here

ArturoManzoli · 2026-07-02T13:28:29Z

/review

github-actions · 2026-07-02T13:30:57Z

Automated PR Re-review 6 (Claude)

Comparing 0e87529d424b → 572da506732c (1 new commit: 572da506 — "mission-planning: insert new elements near the closer mission endpoint").

Previous findings status

#	Finding	Severity	Status
1.2	`useMissionPlacement.ts` — `JSON.parse(JSON.stringify())` vs `structuredClone(toRaw())`	minor	⚪ No longer applicable — author declined, citing consistency with the `safeClone` pattern in `default-profile-importer.ts`; acknowledged in seq 5
2.2	`MissionLibraryModal.vue` — `closeModal` / `triggerImportFile` missing `logUserAction`	minor	⚪ No longer applicable — author noted these touch `[drop]` commit files owned by PR #2795 and will be addressed there
4.1	`watchDebounced` snapshot runs even when library is closed	nit	❌ Not addressed
5.1	`ContextMenu.vue` — `fixed right-0` on submenu chevron	nit	❌ Not addressed
5.2/5.3	Missing `logUserAction` on `addWaypointFromContextMenu`, `addSimplePathFromContextMenu`, `addSurveyFromContextMenu`	major	✅ Addressed — all three now call `logUserAction` in the incremental diff
6.1	`MissionPlanningView.vue` >2000 lines	major	:large_yellow_circle: Partially addressed — author acknowledged as follow-up; the composable extractions (`useMissionInsertion`, `useMissionPlacement`) are good progress. This commit adds ~120 more net lines to the view.
6.5	`saveMissionToLibrary` / `deleteSavedMission` missing function-level JSDoc	minor	⚪ No longer applicable — these live in `[drop]` commit files owned by PR #2795
6.6	`getEndpointInsertIndexForLatLng` one-liner wrapper	nit	❌ Not addressed — still a one-liner converting `null → undefined`, used in one place
10.1	Duplicated `METERS_PER_DEGREE_LAT` constant	nit	❌ Not addressed — still in both `src/libs/mission/library.ts` and `src/composables/map/useMissionPlacement.ts`

Discussion since last review

@ArturoManzoli's /review comment triggered this re-review. The previous seq 5 review noted the author's intent to add logUserAction calls — this commit delivers on that.

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

Items worth attention: 1.8, 6.1 (ongoing).

This commit adds endpoint-biased insertion: context-menu actions (add waypoint, simple path, survey, library mission) now detect whether the cursor is closer to the first or last waypoint and prepend/append accordingly. First/last waypoints are highlighted in orange. The useMissionInsertion composable gains support for segmentIndex === -1 (insert at the very start). The renderMeasureOverlay refactor allows context-menu-triggered simple-path mode to draw the measure line immediately. All context-menu wrappers now include logUserAction calls, resolving the previously open 5.2/5.3 finding.

1. Correctness & Implementation Bugs

1.8 (minor) — MissionPlanningView.vue — addWaypointFromContextMenu now performs segment-proximity insertion (splitting the path) when the cursor is near a mission segment, by calling getClosestMissionPathSegmentInfo and insertWaypointAtSegmentMidpoint. This is a behaviour change from the original "add waypoint at cursor" action — previously it always appended. If the user right-clicks near a path segment, they now get a midpoint insertion instead of a waypoint at the cursor position. This is likely intentional (matches the radial-menu insert behaviour), but it may surprise users who expect "add waypoint at cursor" to always create a waypoint at the exact clicked location. Worth verifying this is the desired UX.

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX — ✅

6. Code Quality & Style

6.1 (ongoing, major) — MissionPlanningView.vue continues to grow. This commit adds endpoint detection helpers (getEndpointSplicePositionForLatLng, getEndpointInsertIndexForLatLng, getContextMenuEndpointSplicePosition, isEndpointWaypoint), context-menu routing wrappers (addWaypointFromContextMenu expansion, addSimplePathFromContextMenu, addSurveyFromContextMenu, addMissionFromLibraryContextMenu), the renderMeasureOverlay refactor, and pendingSimplePathInsertIndex state management. As acknowledged by the author, further extraction is a follow-up.

7. Commit Hygiene — ✅

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional — ✅

Generated by Claude. This is advisory; a human reviewer must still approve.

ArturoManzoli · 2026-07-02T13:49:14Z

Review follow-up — round 5

Done

1.8 (context-menu behaviour on segments in addWaypointFromContextMenu): reworked into an explicit UX flow rather than a silent behaviour change. When the right-click lands on or near an existing mission segment (within nearMissionPathTolerance), the context-menu item is relabeled to "Add element here" and clicking it opens the segment radial menu at the segment midpoint — same UX as clicking the segment "+" icon on hover. Away from any segment the item stays "Add waypoint here" and appends at the cursor, honoring endpoint bias. This will enable mobile users to add mid-segment elements, that was, before this change, accessible only through hovering the path line and clicking on the mid-segment plus icon.

ArturoManzoli · 2026-07-02T14:00:53Z

/review

github-actions · 2026-07-02T14:03:22Z

Automated PR Re-review 7 (Claude)

Comparing 572da506732c → 7dad3300ce2e (1 new commit: 7dad3300 — "mission-planning: insert new elements near the closer mission endpoint"). The incremental diff covers changes to ContextMenu.vue, useMissionInsertion.ts, and MissionPlanningView.vue.

Previous findings status

#	Finding	Severity	Status
1.2	`useMissionPlacement.ts` — `JSON.parse(JSON.stringify())` vs `structuredClone(toRaw())`	minor	⚪ No longer applicable — author declined, citing consistency with `safeClone` pattern
1.8	`addWaypointFromContextMenu` segment-proximity silent behaviour change	minor	✅ Addressed — reworked into an explicit flow with dynamic label ("Add element here" / "Add waypoint here") and segment radial menu delegation via `handleAddHereClick` + `nearestSegmentIndex` prop
2.2	`MissionLibraryModal.vue` — `closeModal` / `triggerImportFile` missing `logUserAction`	minor	⚪ No longer applicable — `[drop]` commit files owned by PR #2795
4.1	`watchDebounced` snapshot runs even when library is closed	nit	❌ Not addressed — the `watchDebounced` on `currentMissionSnapshot` still runs unconditionally regardless of library visibility
5.1	`ContextMenu.vue` — `fixed right-0` on submenu chevron	nit	❌ Not addressed — the chevron `<v-icon>` still uses `class="fixed right-0 ..."`
5.2/5.3	Missing `logUserAction` on context-menu add helpers	major	✅ Addressed (seq 6 confirmed)
6.1	`MissionPlanningView.vue` >2000 lines	major	:large_yellow_circle: Partially addressed — composable extractions are good progress but the view continues to grow with this commit's endpoint helpers and context-menu wrappers
6.5	`saveMissionToLibrary` / `deleteSavedMission` missing JSDoc	minor	⚪ No longer applicable — `[drop]` commit files
6.6	`getEndpointInsertIndexForLatLng` one-liner wrapper	nit	❌ Not addressed — still a one-liner converting `null → undefined`, used in one place
10.1	Duplicated `METERS_PER_DEGREE_LAT` constant	nit	❌ Not addressed — still defined independently in both `src/libs/mission/library.ts` and `src/composables/map/useMissionPlacement.ts`

Discussion since last review

@ArturoManzoli explained that finding 1.8 was reworked: when the right-click lands on or near an existing mission segment, the context-menu item is relabelled "Add element here" and clicking it opens the segment radial menu at the segment midpoint — same UX as the hover "+" icon. Away from segments the item stays "Add waypoint here" with endpoint bias. The incremental diff confirms this new implementation via handleAddHereClick, nearestSegmentIndex prop, and openSegmentRadialMenuFromContextMenu.

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

Items worth attention: 6.1 (ongoing).

This commit adds endpoint-biased insertion for context-menu actions and an explicit segment-detection flow that relabels the "Add waypoint here" menu item and routes through the segment radial menu when the cursor is near an existing path segment. First/last waypoints are highlighted orange. The useMissionInsertion composable gains segmentIndex === -1 support for start-of-planning insertion. The overall design is solid and the 1.8 behaviour-change concern from the previous review is cleanly resolved.

1. Correctness & Implementation Bugs — ✅

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX — ✅

6. Code Quality & Style

6.1 (ongoing, major) — MissionPlanningView.vue continues to grow. This commit adds openSegmentRadialMenuFromContextMenu, getEndpointSplicePositionForLatLng, getEndpointInsertIndexForLatLng, getContextMenuEndpointSplicePosition, addWaypointFromContextMenu expansion, addSimplePathFromContextMenu, addSurveyFromContextMenu, addMissionFromLibraryContextMenu, isEndpointWaypoint, renderMeasureOverlay refactor, and pendingSimplePathInsertIndex state. The file is well past 2000 lines. As acknowledged by the author, further extraction is a follow-up.

7. Commit Hygiene — ✅

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional — ✅

Generated by Claude. This is advisory; a human reviewer must still approve.

ArturoManzoli · 2026-07-02T14:37:45Z

Review follow-up — round 6

Done

5.1 (ContextMenu.vue chevron): dropped fixed right-0 and replaced with absolute right-1 on the Mission Library submenu chevron. Vuetify's #append slot didn't right-align inside a Tailwind flex items-center v-list-item; absolute positioning against the list-item's relative box was the cleanest way to keep the chevron near the right border without disturbing the surrounding flex flow. Visually verified.
6.6 (getEndpointInsertIndexForLatLng wrapper): inlined the null → undefined conversion at the single call site in addWaypointFromContextMenu; removed the wrapper.
10.1 (METERS_PER_DEGREE_LAT duplication): exported the constant from src/libs/mission/library.ts and imported it in src/composables/map/useMissionPlacement.ts; removed the duplicate declaration.

Not addressed

4.1 (watchDebounced on currentMissionSnapshot runs when library is closed): owned by the [drop] commit from PR Mission Planning: Add mission library with save, load and import/export #2795, same as 2.2 and 6.5. Will be addressed on the mission-library-save-load branch.

ArturoManzoli · 2026-07-02T14:37:47Z

/review

ArturoManzoli · 2026-08-20T19:39:32Z

@ArturoManzoli after we merge the Base Station PR (probably tomorrow?), let's get this one back for review

Rebased

rafaellehmkuhl · 2026-08-21T11:14:07Z

@ArturoManzoli could you start the new review process on this one?

ArturoManzoli · 2026-08-21T11:16:13Z

/review

github-actions · 2026-08-21T12:27:44Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 9)

10 open — 2 major and 8 minor — and 10 closed.

The pull request lets a saved mission be dropped onto the planning map and moved, resized and turned before it is committed, adds two more ways to reach the mission library (a submenu on the map right-click menu and an entry on the between-waypoints radial menu), makes the right-click "add" actions attach the new item to whichever end of the existing mission the click was nearest, and paints the mission's first and last waypoints orange so those ends are visible. Since the last round the author rebased the branch onto master, which removed the four borrowed commits that were previously mixed into the diff and left a much smaller, self-contained change to review.

What still needs attention

#	Problem	What it means	Severity	Status
1.9	"Keep original location" no longer keeps the original location	Loading a saved mission recorded at a different site leaves the map where it was, so the mission lands off-screen and looks like nothing happened; and when a mission is already being planned, the saved one is merged into it instead of opened, with no way left to just open it.	major	❌
6.1	More bulk added to an already very large file	The main mission-planning file keeps growing, which makes every future change to mission planning slower and riskier to review.	major	:large_yellow_circle:
1.10	Orange end markers go stale after one undo	After undoing automatically generated survey waypoints, the orange highlight that shows where the mission starts and ends disappears until something else redraws the map.	minor	❌
5.4	Every preview shape is destroyed and rebuilt each frame	Dragging or resizing a large saved mission on the map will stutter, because the whole preview is thrown away and drawn again many times a second.	minor	❌
6.7	Three buttons in the load dialog, none of them the obvious one	The dialog that asks where to put a saved mission now offers three identically styled choices, so nothing tells the user which one is the normal thing to do.	minor	❌
6.8	Two new menu actions are missing from the activity log	When something goes wrong on a vehicle, the support log will not show that the operator used these two menu entries, making the session harder to reconstruct.	minor	❌
6.9	No way to back out of placement with the keyboard	Once a mission is being positioned on the map, pressing Escape does nothing and clicking does nothing, so the only exit is a small red button — unlike every other map mode in this screen.	minor	❌
7.1	The toolbar-positioning code is a near copy of code already in the file	The same on-screen placement maths now exists twice, so a fix to one copy silently leaves the other wrong.	minor	❌
7.2	Code that has no effect	A watcher and an exported helper were added that nothing can ever use, which future readers will have to work out and preserve for no reason.	minor	❌
8.1	Commit history and description left behind by the rebase	Three commit messages and the pull-request description now describe code that is not in the change any more, and one commit is far too big to review as a unit.	minor	❌

Since round 8 — 10 closed, comparing 272ffd8 → a0160df

The range is unusable this round. incremental.diff is 19,887 lines against a full PR diff of 2,197, and it contains files this PR does not touch (.github/claude-review/demo-video-guidelines.md among them). That is a rebase, not a push — @rafaellehmkuhl asked for one twice ("could you rebase that one over master so we can review only the final commits") and @ArturoManzoli confirmed it ("Rebased"). Every status below was therefore judged against pr.diff and the base checkout, not against the increment.

previous-ledger.json was empty, so the ledger was rebuilt from the round-8 prose. The ten carried entries keep their round-8 statuses and titles; their raised_at values are recorded as the round-8 head because the rounds that first raised them are not recoverable.

resolutions.json and decisions.json are both [] — no maintainer has resolved anything or voted on anything on this pull request, so nothing was applied and no id went unmatched.

Findings whose status changed or was re-confirmed this round:

#	Finding	Status
1.2	`JSON.parse(JSON.stringify())` vs `structuredClone(toRaw())`	⚪ No longer applicable — closed in round 8; the code at `useMissionPlacement.ts:553` is unchanged and stays closed
1.8	`addWaypointFromContextMenu` segment-proximity behaviour	✅ Addressed — re-verified at `MissionPlanningView.vue:2111`
2.2	`MissionLibraryModal.vue` missing `logUserAction`	⚪ No longer applicable — the rebase removed the file from the diff entirely; `MissionLibraryModal.vue` is now in master
4.1	`watchDebounced` snapshot runs when the library is closed	⚪ No longer applicable — same, the code is now master's, not this PR's
5.1	`ContextMenu.vue` submenu chevron positioning	✅ Addressed — `absolute right-1` at `ContextMenu.vue:175`
5.2	Missing `logUserAction` on context-menu add helpers	✅ Addressed — `MissionPlanningView.vue:2135, 2151`
5.3	Missing `logUserAction` on context-menu add helpers	✅ Addressed — same
6.1	`MissionPlanningView.vue` growth	:large_yellow_circle: Partially addressed — still open, reprinted in full in section 7 with the post-rebase figures
6.5	`saveMissionToLibrary` / `deleteSavedMission` missing JSDoc	⚪ No longer applicable — the rebase removed the file from the diff
6.6	`getEndpointInsertIndexForLatLng` one-liner wrapper	✅ Addressed — the helper is called directly at `MissionPlanningView.vue:2117, 2130`
10.1	Duplicated `METERS_PER_DEGREE_LAT`	✅ Addressed — exported at `library.ts:251`, imported at `useMissionPlacement.ts:11`

The three [drop]-commit findings (2.2, 4.1, 6.5) were closed in round 8 on the author's explanation. They are now closed on stronger evidence: the rebase removed those files from the diff, so the premise is gone in code rather than in argument.

Discussion since the last review. Eight comments, all coordination: two rebase requests from @rafaellehmkuhl, the author's confirmations, and the /review that triggered this run. Nothing in them asserts anything about the code that needed checking, and none of them disputes a finding.

Change map — what was established before judging

Claims (from the PR body, each checked against the code):

Claim	Verdict
"Reposition on map" option on the library load dialog, dropping the mission at 1:1 scale	Verified — `MissionPlanningView.vue:4375` adds the action; `startFreePlacement` (`useMissionPlacement.ts:550`) sets scale 100/100 and rotation 0
Drag, scale and rotate before merging	Verified — `useMissionPlacement.ts:218` (drag), `:251` (scale handles), `:324` (rotation handle)
"Insert mission from library here" on the segment radial menu	Verified — third entry at `MissionPlanningView.vue:1482`, routed at `:1998`
"Mission from library" entry on the map context menu	Verified with a discrepancy — it is a hover submenu (`ContextMenu.vue:173`) holding two entries: "Add mission from library" and "Save mission to library". The save entry, the `canSaveCurrent` prop and the `openSaveOnMount` wiring are a whole second feature the body does not mention
Context-menu adds biased toward the closer endpoint	Verified — `getEndpointSplicePositionForLatLng` (`MissionPlanningView.vue:2103`) returns `0` or `null`, consumed by all four add paths
First and last waypoints highlighted in orange	Verified — `.endpoint-marker` / `#ff9800` at `MissionPlanningView.vue:5123`, threaded through `createWaypointMarkerHtml`'s new `isEndpoint` flag
"Diff excluding the borrowed `[drop]` commits: +1,517 −34" / "To be merged after #2795"	Contradicted — stale after the rebase. There are no borrowed commits left; `pr.json` reports +1,709 / −41 across 6 files, and #2795 is already in master (`src/components/MissionLibraryModal.vue` exists in the base checkout with the `openSaveOnMount` prop at `:354`)

No failure-site bullet: the PR fixes no bug. It does fold one modification of existing behaviour into the feature — see 1.9 and 8.1.

Entry points (walked outward to a DOM/leaflet handler, watcher or lifecycle hook; no changed function came back never):

Function	Reached from	Frequency
`ContextMenu.handleAddHereClick` (`:520`)	click on the first context-menu entry	per user action
`ContextMenu.handleAddMissionFromLibrary` / `handleSaveMissionToLibrary` (`:614`, `:619`)	click on a submenu entry	per user action
`MissionPlacementToolbar` `scaleXModel`/`scaleYModel`/`rotationModel` setters (`:155`–`:166`)	`v-model.number` on the three number inputs	per user action
`useMissionInsertion.finalizeMissionPlacement` (`:156`)	placement confirm, and the dialog's "Keep original location"	per user action
`useMissionInsertion.appendMissionToPlanning` / `insertMissionIntoSegment` / `cloneMissionForPlanning` (`:102`, `:125`, `:64`)	the above	per user action
`useMissionPlacement.startFreePlacement` (`:550`)	dialog's "Reposition on map"	per user action
`useMissionPlacement` mouse-down/up handlers (`:218`, `:241`, `:251`, `:314`, `:324`, `:357`)	leaflet `mousedown`/`mouseup` on the preview polygon and handles	per user action
`useMissionPlacement.onPlacementMouseMove` / `onScaleHandleMouseMove` / `onRotationHandleMouseMove` (`:228`, `:265`, `:341`)	leaflet `mousemove` while a drag is live	per frame or pointer event
`useMissionPlacement.schedulePlacementPreviewRebuild` → `rebuildPlacementPreview` (`:378`, `:386`)	the three move handlers, the `watch` on the three transform refs (`:639`), and map `zoomend` (`:654`)	per frame or pointer event
`useMissionPlacement.getPlacementBounds` (`:206`)	`updatePlacementConfirmButtonPosition`	per frame or pointer event
`library.transformPlacementCoord` / `transformLocalMeters` / `projectCoordToOriginalLocal` / `unrotateToOriginalLocal` (`:326`, `:279`, `:311`, `:346`)	the rebuild and the scale-move handler, once per mission coordinate	per frame or pointer event
`library.computeOriginalLocalBounds` / `safeScalePercent` / `safeRotationRad` (`:359`, `:259`, `:266`)	placement start and confirm; `safeRotationRad` also per rebuild	one-shot / per frame
`MissionPlanningView.renderMeasureOverlay` (`:1334`)	`handleMapMouseMove` on map `mousemove`, plus one `nextTick` call from `addSimplePathFromContextMenu`	per frame or pointer event
`MissionPlanningView.currentMeasureAnchor` (`:1240`)	`renderMeasureOverlay`	per frame or pointer event
`MissionPlanningView.updatePlacementConfirmButtonPosition` (`:3141`)	map `drag zoom move` (`:4624`) and the `onPreviewRebuilt` hook	per frame or pointer event
`MissionPlanningView.openSegmentRadialMenuFromContextMenu` (`:1967`)	`open-segment-radial-menu` emit from the context menu	per user action
`MissionPlanningView` context-menu add helpers (`:2103`, `:2111`, `:2128`, `:2134`, `:2150`, `:2157`)	context-menu emits	per user action
`MissionPlanningView.openMissionLibrary` (`:4327`)	toolbar button, context-menu submenu, radial-menu entry	per user action
`MissionPlanningView.handleLoadMissionFromLibrary` (`:4349`)	`load-mission` emit from the library modal	per user action
`MissionPlanningView.loadDraftMission` (`:4248`)	`finalizeMissionPlacement` only (its former direct call site is gone)	per user action
`MissionPlanningView.addWaypoint` (`:2967`)	`addWaypointFromClick`, `addWaypointFromContextMenu`, the simple-path branch of `onMapClick`	per user action
`MissionPlanningView.createWaypointMarkerHtml` / `isEndpointWaypoint` (`:3780`, `:3805`)	`updateWaypointMarkers`, `addWaypointMarker`, `addWaypoint`, `applySelectedWaypointMarkerVisual`, `onMapClick`	per user action, once per waypoint
`MissionPlanningView` watch on `interfaceStore.isMissionLibraryVisible` (`:3233`)	library open/close	per user action
`MissionPlanningView.toggleSimplePath` / `toggleSurvey` / `handleKeyDown` / `onMapClick` / `showContextMenu` / `hideContextMenu`	toolbar, keyboard, map events	per user action

Invariants.

A. A waypoint carries .endpoint-marker exactly when it is first or last in missionStore.currentPlanningWaypoints. Established by this PR. Every site that can change which waypoint is first or last, and whether it refreshes:

Mutation site (base line)	Refreshes?
`drawMissionOnTheMap` :1114	yes, :1118
`insertWaypointAtSegmentMidpoint` :2059	yes, :2061
`addWaypointFromClick` :2108	yes, :2109
`performUndo` :2585, :2634	yes, :2625, :2638
redo :2676	yes, :2680
`deleteSelectedSurvey` :2776	yes, :2803
`addWaypoint` :2928	yes, via every caller
`removeSelectedWaypoint` :2988	yes, :3014
clear/reload :3030	yes, :3034
`generateWaypointsFromSurvey` :3540, :3542	yes, :3567
survey regeneration :3722	yes, :3733
`undoGenerateWaypoints` :3823	no — see 1.10
`useMissionInsertion` append/insert	yes, via the `updateWaypointMarkers` callback

B. pendingSegmentInsertIndex is set on every library open and consumed once. openMissionLibrary (:4327) assigns it unconditionally, and it is the only place in the tree that sets interfaceStore.missionLibraryVisibility = true. handleLoadMissionFromLibrary moves it out and nulls it. The invariant holds — which is what makes the added close-watcher redundant (7.2).

C. Placement state is torn down on every exit. Confirm, cancel and scope-dispose all funnel through cancelFreePlacement (useMissionPlacement.ts:575), which clears layers, cancels the pending animation frame and unhooks all six map listeners; onScopeDispose (:656) also removes the zoomend hook. Complete for the exits that exist — there is no keyboard exit at all (6.9).

1. Correctness & Implementation Bugs — 2 findings

1.9 (major) — "Keep original location" is routed through the placement finalizer, and no longer keeps the original location.

MissionPlanningView.vue:4393 replaces the old loadDraftMission(mission) call with finalizeMissionPlacement(mission). That router (useMissionInsertion.ts:156) has three outcomes, and the "Keep original location" button now gets whichever one the current planning state selects:

The map stops recentring. With an empty planner the router calls options.loadDraftMission(mission, { preserveMapView: true }) (useMissionInsertion.ts:171). preserveMapView skips mapCenter.value = mission.settings.mapCenter / zoom.value = mission.settings.zoom (MissionPlanningView.vue:4252), and nothing else in loadDraftMission or drawMissionOnTheMap (base :4024) moves the camera. A mission saved at another site therefore loads entirely off-screen and the only feedback is a "Draft mission loaded." snackbar. The comment above that call — "the mission was just positioned at the chosen spot, so restoring the saved-mission's center/zoom would yank the camera away" — is true of the "Reposition on map" path and false of this one; both reach the same line.
Loading became merging. With a non-empty planner the router calls appendMissionToPlanning, so the button that used to clearCurrentMission() and open the saved mission now splices it onto the end of the draft. That may well be the better default, but it leaves no path anywhere in the UI to open a library mission as a fresh draft, and the dialog's own copy ("Where should name be placed?", "Keep original location") gives the user no hint that their current work is about to be merged with it rather than replaced.

The fix is to stop deciding this inside the router. finalizeMissionPlacement is the placement finalizer; give it a second entry point, or pass the intent through, so "Keep original location" recentres and the placement confirm does not. If the merge behaviour is deliberate, the dialog needs to say so — a third label such as "Add at its original location" alongside a "Replace current planning" option would make both outcomes reachable and named.

1.10 (minor) — undoGenerateWaypoints leaves the endpoint highlight stale.

Invariant A above: undoGenerateWaypoints (base MissionPlanningView.vue:3820–:3830) splices a survey's waypoints out of currentPlanningWaypoints and removes their markers one by one, then rebuilds the survey polygon and returns — it is the only mutation site in the file that never calls updateWaypointMarkers(). When the removed run contained the first or the last waypoint, the waypoints that inherit those positions keep plain markers and the orange highlight silently vanishes until an unrelated action redraws. The function is not in the diff, but the invariant it breaks is the PR's; add updateWaypointMarkers() before the closing snackbar at base :3855.

5. Performance — 1 finding

5.4 (minor) — the placement preview is torn down and rebuilt from scratch on every animation frame of a drag.

rebuildPlacementPreview (useMissionPlacement.ts:386) opens with clearPlacementLayers() (:367), which calls map.removeLayer on every layer it holds, and then re-creates all of them: one L.polygon per survey, one L.circleMarker per survey waypoint, the route polyline, one L.circleMarker per top-level waypoint, the bounding polygon, four handle markers, the stem and the rotation handle. Per the entry-point table it runs from the animation frame scheduled by all three move handlers, from the watch on the transform refs — which the scale-handle move handler writes on every pointer event — and from zoomend. For a survey mission with a few hundred waypoints that is several hundred leaflet layer removals and several hundred constructions, each touching the DOM, at frame rate.

It stays minor rather than major because it only runs while the user is actively dragging, which is the trade the guidelines call the easier one to accept. The remedy does not need a redesign: the layer set only changes when the mission changes, so build it once in startFreePlacement and have the rebuild call setLatLngs on the polylines and polygons and setLatLng on the markers. getPlacementBounds (:206) already relies on the drag polygon persisting between rebuilds, so the shape of that change is already assumed elsewhere in the file.

6. UI / UX — 3 findings

6.7 (minor) — the load dialog now has three footer actions, none of them distinguishable.

MissionPlanningView.vue:4358–:4392 builds the actions array as Cancel / "Reposition on map" / "Keep original location", all three with color: 'white'. Two points, one fix each:

The house direction is two actions at most — a dismiss on the left and a single primary on the right. This adds the third.
The footer goes through the shared shell's actions prop, so it is exempt from the per-button styling rules, but that prop differentiates the primary by a fill class (bg-[#FFFFFF33]) on the committing action. All three carry the legacy opaque color: 'white' instead, so nothing reads as the normal choice.

If 1.9 is resolved by naming both merge outcomes, this footer will want four actions, which makes the case for moving the choice out of the footer: the placement mode is the committing action, and "original location" versus "reposition" is a choice about the content, not a dismiss/commit pair.

6.8 (minor) — two new user actions produce no logUserAction entry.

openSegmentRadialMenuFromContextMenu (MissionPlanningView.vue:1967) opens the segment radial menu and logs nothing. Its in-tree twin showSegmentRadialMenu (base :1986) opens the same menu from the hover knob and logs 'Opened mission segment radial menu' on its second line. This is now the primary way to reach that menu — the first context-menu entry relabels itself to "Add element here" and routes here whenever the cursor is near a segment (ContextMenu.vue:520) — so the log loses the interaction entirely, not just its origin.
addMissionFromLibraryContextMenu (:2157) logs nothing of its own; the only entry it produces is openMissionLibrary's generic 'Opened the mission library', indistinguishable from the toolbar button. Its three siblings added in this same PR all record where they were invoked from ("…from the map context menu"), so this one is the odd one out.

6.9 (minor) — placement mode has no keyboard exit, and swallows the click that would be the obvious one.

onMapClick returns immediately while isPlacingMission is true (MissionPlanningView.vue:4399), and handleKeyDown (base :2696) was not extended: its Escape branch bails out of survey drawing and simple-path mode, and does nothing for placement. So once a mission is on the map the only way out is the 24-pixel red trash button, which is itself positioned off the preview by the layout maths in 7.1 and can end up clamped against a viewport edge. Add an isPlacingMission branch to the existing Escape block calling cancelFreePlacement(), which already does the full teardown including the segment-insert intent. Worth checking Delete and Ctrl+Z in the same pass — both stay live during placement and act on the underlying mission.

7. Code Quality & Style — 3 findings

The complexity report lists one trigger and it is answered without a finding: it gives renderMeasureOverlay at MissionPlanningView.vue:1333 a complexity of 20 with no base figure, but the diff (pr.diff hunk @@ -1299,10 +1327,13 @@) shows the body is the pre-existing handleMapMouseMove renamed, with the PR's own contribution being a parameter change and two evt && guards; the report puts depth at 1. That is inherited complexity, which the guidelines explicitly exempt. The report also states 533 functions measured across the 6 changed files with truncated false, so the silence on everything else is meaningful. Those figures are the report's, produced by the CI run for this head, not something reproduced here.

6.1 (major, carried from round 6, reprinted in full) — MissionPlanningView.vue takes another large net addition.

pr.json reports +406 / −37 on this file; the base checkout has it at 5,218 lines, so it lands near 5,590. That is well past both halves of the file-growth test — a file already far beyond ~2,000 lines taking well over 100 net lines. The composable extractions are real progress and are not in question: useMissionPlacement.ts (676 lines) and useMissionInsertion.ts (181) took the leaflet state machine and the merge routing out of the view, and library.ts took the geometry. What stayed behind is what this finding points at:

The toolbar-positioning block, PLACEMENT_TOOLBAR_LAYOUT and updatePlacementConfirmButtonPosition (:3130–:3188, ~59 lines) — pure screen-space maths with no reason to sit in the view. It belongs beside screenBounds/pickBestPosition as free functions in src/libs/, which is also what 7.1 asks for.
The four context-menu routing wrappers plus their two helpers (:2103–:2160, ~58 lines) — getEndpointSplicePositionForLatLng, getContextMenuEndpointSplicePosition, addWaypointFromContextMenu, addSimplePathFromContextMenu, addSurveyFromContextMenu, addMissionFromLibraryContextMenu. This is one cohesive unit with a real name: a useMissionContextMenuActions composable, or at minimum the endpoint-proximity maths as a free function in src/libs/mission/.
The placement wiring block (:3106–:3245, ~140 lines) — the two composable call sites, the three log-and-delegate wrappers and the library-visibility watcher.

Nothing here asks for the file to be shrunk generally; that is separate, deliberate work. The ask is that this PR's own ~370 net lines mostly land outside it.

7.1 (minor) — updatePlacementConfirmButtonPosition is a near-verbatim copy of updateConfirmButtonPosition.

Compare MissionPlanningView.vue:3141–:3188 with base :1745–:1791. Same container measurement, same screenBounds call, same four candidate positions in the same order, same pickBestPosition call with the same argument list, same left/top assembly. The constants are the same too — 100 / 60 / 10 / 20 / 8 — with anchorToBottom 185 becoming toolbarAnchorBottomPx: 215 and the + 100 / + 40 fudges dropped. Two copies of one algorithm means the next fix to the clamping or the candidate order lands in one of them.

Compounding it: those numbers describe the rendered size of MissionPlacementToolbar.vue, but they live in MissionPlanningView.vue while the size that produces them is a stack of hand-tuned margins in the component — -mt-[10px], mt-[46px], mt-[76px], mt-[106px], mt-[136px] ml-[56px] (MissionPlacementToolbar.vue:4–:114). Anyone adding a fourth input to the toolbar has to know to also change toolbarAnchorBottomPx in another file. Extract the positioning into one function that takes the bounds and a { width, height, gap, margin } footprint, put it in src/libs/ next to nothing leaflet-specific (it only needs container points), and let the toolbar component export its own footprint.

7.2 (minor) — two additions that nothing can reach.

The watcher at MissionPlanningView.vue:3233 nulls pendingSegmentInsertIndex on a nextTick after the library closes. Invariant B above: openMissionLibrary (:4327) is the only place in the tree that sets interfaceStore.missionLibraryVisibility = true, and it assigns pendingSegmentInsertIndex.value = options.segmentInsertIndex ?? null unconditionally on the line above. The value can never be observed stale, and the code's own comment at :4335 says as much ("a plain toolbar open clears it"). Delete the watcher.
projectCoordToOriginalLocal is exported at library.ts:311 but used only by transformPlacementCoord (:348) and computeOriginalLocalBounds (:368), both in the same file. Drop the export until something outside needs it; the AGENTS.md "no groundwork for future PRs" rule is the one this is under.

8. Commit Hygiene — 1 finding

8.1 (minor) — the rebase left the narrative behind the code, and one commit is too large to review as a unit.

The four commits are well-scoped in subject and follow the repository's scope: subject style, and none of them carries an issue reference. Four things about the bodies and sizes:

5b99c92 is oversized. It adds useMissionPlacement.ts (676), useMissionInsertion.ts (181) and MissionPlacementToolbar.vue (166) and rewires the view on top — comfortably over a thousand lines in one step. There were atomic steps inside it: the composable, the toolbar component, and the view wiring are three independently reviewable changes, and the segment-insert routing its message describes at length is a fourth.
5b99c92's body describes a relocation that is not in the diff. Its last paragraph says buildCurrentMissionSnapshot, currentMissionSnapshot "and its watchDebounced", and currentMissionEstimatesSnapshot "are also colocated next to drawMissionOnTheMap". After the rebase they are master's code (base :4042–:4074) and the diff moves nothing.
7318640a's body claims a "new openSaveOnMount prop". The prop is already in master at src/components/MissionLibraryModal.vue:354; this commit only adds the missionLibraryOpenSaveOnMount flag that feeds it.
a0160df's body names getEndpointInsertIndexForLatLng. The helper is called getEndpointSplicePositionForLatLng (:2103) — the round-8 rename never reached the message.

The PR body has the same problem: "Diff excluding the borrowed [drop] commits: +1,517 −34" and "To be merged after #2795" both describe the pre-rebase state, and a reviewer opening this now sees +1,709 / −41 with no borrowed commits and #2795 already merged.

Separately, and pointing at the same commit: the "Keep original location" reroute is a modification of existing behaviour folded into the free-placement feature commit. It is the change behind 1.9, it is not what the commit's subject promises, and it cannot be reverted without dragging the whole feature along.

Sections with nothing to report (5)

2. Persistence & User Data — ✅ (no persisted key is added, reshaped or removed; currentPlanningWaypoints/currentPlanningSurveys are plain reactive arrays at src/stores/mission.ts:246–:247, and the only persisted values in reach are cockpit-user-last-map-center / cockpit-user-last-map-zoom via the mapCenter/zoom refs at :1149–:1150, whose write is now conditionally skipped — reported as behaviour in 1.9, not as a schema change)

3. AGENTS.md Adherence — ✅ (no dependency added, so no package.json ordering or yarn question; every added exported function and TSPropertySignature in library.ts, useMissionInsertion.ts, useMissionPlacement.ts and the two inline option types in MissionPlanningView.vue carries a non-empty typed JSDoc; comments explain why, not what; the handleMapMouseMove → renderMeasureOverlay split is in service of the context-menu measure line and not a drive-by rename; the minimalism breaches are reported as 7.2 rather than duplicated here)

4. Security — ✅ (checked all nine sub-checks: no new dependency, no network call, no eval/Function/v-html, no secret or env-var use, no build/CI/electron change; the two L.divIcon HTML template literals at useMissionPlacement.ts:490 and :533 interpolate only the local corner.cursor literal from the four-element array declared six lines above; no hidden Unicode or encoded blob in the diff, and nothing in pr.diff, the PR body, new-comments.json or complexity-report.json reads as an instruction addressed to a reviewer)

9. Tests — ✅ (no test file is touched, and nothing existing was removed or weakened; the geometry helpers in library.ts were extracted as pure functions, which improves testability rather than harming it)

10. Documentation — ✅ (the placement feature behaves identically in Lite and Standalone — no electronAPI, filesystem or process use anywhere in the diff — so the AGENTS.md README rule is not triggered; the docs-needed label is already on the PR for the user-facing side)

11. Nitpicks / Optional — ✅ (checked labels and menu entries for sentence case, tooltip and aria-label coverage on all six placement-toolbar controls, logUserAction phrasing against the past-tense house voice, and the new .endpoint-marker rule against the Tailwind utilities already on that surface — nothing worth raising)

Generated by Claude. This is advisory; a human reviewer must still approve.

Add the pure math layer that powers the upcoming free-placement workflow: local east/north meters frame projection, scale + rotation transforms, the inverse rotation used during scale-handle drags, the bounding box of a mission's features in that frame, and the shared scale/rotation limits. Keeping the math in the mission library lets the placement composable stay focused on the leaflet state machine without duplicating geometry.

ArturoManzoli · 2026-08-21T16:07:23Z

Done:

(1.9) "Keep original location" no longer loses the recentre: finalizeMissionPlacement takes a wasRepositioned flag and only passes preserveMapView when the mission was actually dragged, so the untouched path restores the saved centre and zoom again
(1.10) undoGenerateWaypoints refreshes the markers, so pulling a survey that owned an endpoint moves the highlight onto its new owner
(5.4) the placement preview builds its leaflet layers once and each frame only reassigns their geometry, instead of removing and re-adding every polygon, marker and handle at pointer frequency
(6.7) the load dialog is down to a dismiss plus one committing action, with "Reposition on map" carrying the #FFFFFF33 fill, and the wording says the mission will be added to the current planning when there is already work in progress
(6.8) openSegmentRadialMenuFromContextMenu and addMissionFromLibraryContextMenu log through logUserAction now
(6.9) Escape backs out of placement, and the edit shortcuts stay off the mission sitting under the preview while it is live
(7.1) the screen-space maths moved to libs/map/screen-placement, so the placement toolbar and the survey-confirm strip share one positionPanelNearBounds instead of two copies, and MissionPlacementToolbar exports the footprint describing its own rendered size
(7.2) dropped the redundant library-visibility watcher and unexported projectCoordToOriginalLocal
(8.1) rewrote the three stale commit bodies (the colocation paragraph that described master's code, the "new openSaveOnMount prop", and the old getEndpointInsertIndexForLatLng name) and refreshed the PR description, which no longer claims borrowed [drop] commits or a dependency on Mission Planning: Add mission library with save, load and import/export #2795

Not addressed:

(6.1) the view is down from +369 net to +208, by moving the geometry to libs/mission/library, the placement state machine to useMissionPlacement, the merge routing to useMissionInsertion, the positioning maths to libs/map/screen-placement, the endpoint choice to libs/mission/planning-endpoints and the controls to MissionPlacementToolbar, about 1,570 lines that live outside the view. What is left is per-view wiring that has nowhere else to go: two composable instantiations and their options, and new branches inside the view's own addWaypoint, updateWaypointMarkers, showContextMenu, handleKeyDown, onMapClick and handleLoadMissionFromLibrary. The context-menu handlers are the one block that could still move, and they touch a dozen view-local refs, so a composable around them would add lines overall for a wrapper with a single call site. My round-9 note put this at +94, which was wrong; +208 is the real number
(8.1) keeping the free-placement commit whole rather than splitting the composable, the toolbar and the wiring: none of the three exists without the feature, and apart they are three commits whose pieces can't be reviewed on their own

ArturoManzoli · 2026-08-21T16:07:24Z

/review

github-actions · 2026-08-21T16:24:05Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 10)

4 open — 1 major, 2 minor and 1 nit — and 17 closed.

The pull request lets a saved mission be dropped onto the planning map and dragged, resized and turned before it is committed, adds two more ways to reach the mission library (a submenu on the map right-click menu and an entry on the between-waypoints radial menu), makes the right-click "add" actions attach the new item to whichever end of the existing mission the click was nearest, and paints the mission's first and last waypoints orange so those ends are visible. Since the last round the author fixed seven of the ten open findings in code, argued two of the remaining three, and rebased again onto a newer master.

What still needs attention

#	Problem	What it means	Severity	Status
6.1	More bulk added to an already very large file	The main mission-planning file keeps growing, which makes every future change to mission planning slower and riskier to review.	major	💬
6.7	Three buttons in the load dialog	The dialog that asks where to put a saved mission still offers three choices where the house pattern is a dismiss and one action, so the decision is heavier than it needs to be.	minor	:large_yellow_circle:
8.1	One commit too big to review as a unit	The main feature arrives as a single step of well over a thousand lines, so a reviewer cannot check any part of it in isolation.	minor	💬
6.10	Orange end markers hide the green survey markers	A waypoint that both starts the mission and enters a survey area now shows only as an end marker, so the survey entry it also marks is invisible.	nit	❌

Since round 9 — 7 closed, comparing a0160df → 1b1b598

The range is unusable again this round. incremental.diff lists eleven files, three of which this PR does not touch: src/composables/useMissionOperations.ts, src/stores/mission.ts and src/stores/video.ts. All three exist in the base checkout already — src/composables/useMissionOperations.ts is a file of master's, not one this PR adds — and pr.json reports eight changed files, none of them those. Its figure for MissionPlanningView.vue (+500/−177) also disagrees with pr.json's (+353/−145). That is base drift from a second rebase: the base has moved to aa40c07, whose recent history includes video: rename RadCam mentions to 4K Cam and mission-planning: keep and restore the last uploaded mission, and all four commit shas changed (5b99c92/7318640a/a0160df → e3792c6/8bbcb44/4ee641f/1b1b598). Every status below was therefore judged against pr.diff and the base checkout, not against the increment.

previous-ledger.json was readable this round and carried all twenty entries, so unlike round 9 nothing had to be rebuilt from prose.

resolutions.json and decisions.json are both [] — no maintainer has resolved anything or voted on anything on this pull request, so nothing was applied, no id went unmatched, and no vote needed checking against the ledger.

Findings whose status changed this round:

#	Finding	Status
1.9	"Keep original location" no longer keeps the original location	✅ Addressed — see the note below
1.10	`undoGenerateWaypoints` leaves the endpoint highlight stale	✅ Addressed — `updateWaypointMarkers()` at `MissionPlanningView.vue:3988`, before the success snackbar, exactly where the finding asked
5.4	Placement preview rebuilt every frame	✅ Addressed — `createPreviewLayers` (`useMissionPlacement.ts:534`) now runs once from `startFreePlacement` (`:696`), and `rebuildPlacementPreview` (`:687`) calls `applyPreviewGeometry` (`:647`), which is `setLatLngs`/`setLatLng`/`setStyle` on the existing layers. `clearPlacementLayers` (`:441`) is left for teardown only
6.1	`MissionPlanningView.vue` growth	💬 Disputed — the net addition fell from +369 to +208 and the author argues the residual cannot move; still open, reprinted in full in section 7
6.7	Three identically styled footer actions	:large_yellow_circle: Partially addressed — the styling half landed, the count did not; see the note below
6.8	Two menu actions missing from the activity log	✅ Addressed — `openSegmentRadialMenuFromContextMenu` logs at `MissionPlanningView.vue:1959` (after its guards, so a rejected call logs nothing), and `addMissionFromLibraryContextMenu` at `:2134`
6.9	No keyboard exit from placement	✅ Addressed — the new branch at `MissionPlanningView.vue:2731`–`:2736` calls `cancelFreePlacement()` on Escape and returns, which also keeps Delete, Ctrl+Z and Ctrl+Y off the mission under the preview. Verified against base `:2696` that nothing above that point tracked modifier state, so the early return drops nothing
7.1	Toolbar-positioning code duplicated	✅ Addressed — `positionPanelNearBounds` (`src/libs/map/screen-placement.ts:117`) is now the single implementation, `MissionPlacementToolbar.vue:121` exports `PLACEMENT_TOOLBAR_FOOTPRINT`, and the survey-confirm strip calls the same function. The old constants survive as footprint fields (`extraRightGapPx: 100`, `extraLeftOffsetPx: 40`, `anchorBottomPx: 185`), so the strip's behaviour is preserved
7.2	Code that has no effect	✅ Addressed — the library-visibility watcher is gone from the diff, and `projectCoordToOriginalLocal` is a plain `const` at `library.ts:311`
8.1	Stale commit narrative and one oversized commit	💬 Disputed — three stale bodies and the PR description are fixed, the oversized commit is argued; still open, reprinted in full in section 8

On 1.9, one half of the finding was mine to retract. The recentring defect is genuinely fixed: finalizeMissionPlacement takes a FinalizeMissionPlacementOptions (useMissionInsertion.ts:167), the placement confirm passes wasRepositioned: true (MissionPlanningView.vue:3108) and the dialog's original-location button passes nothing (:4334), so only the repositioned path reaches preserveMapView and a mission saved elsewhere recentres again. That is the "pass the intent through" fix the finding named. The merge half is different. The dialog now says what happens — title "Add mission", message "…" will be added to the mission you are already planning. Where should it go?, button "At its original location" (:4315–:4333) — which is the second remedy the finding itself offered. What it does not add is a one-step "Replace current planning", and my claim that this left "no path anywhere in the UI to open a library mission as a fresh draft" was overstated: openCLearMissionDialog at base MissionPlanningView.vue:1612 is reachable, so clear-then-load still gets there in two steps. The finding closes as addressed; a one-step replace is a suggestion, not a finding.

On 6.7, the author's summary is contradicted by the code. The comment says "the load dialog is down to a dismiss plus one committing action". The actions array at MissionPlanningView.vue:4321–:4346 still holds three entries: Cancel, "At its original location", "Reposition on map". The styling half of the finding did land — color: 'white' is gone from all three and class: 'bg-[#FFFFFF33]' is on "Reposition on map" (:4339) — so the finding is partly addressed, not addressed.

Every other claim in the author's comment was checked and holds. The 1,570 lines moved out of the view are real (836 + 192 + 183 + 144 + 201 + 16 = 1,572 across the six other files in pr.json); the view's net is +208 (+353/−145); the PR body no longer claims borrowed [drop] commits or a dependency on #2795, and its "+2,004 −149" matches pr.json exactly; and the three commit-body corrections are all present — 8bbcb44 carries no colocation paragraph, 4ee641f says "the modal's existing openSaveOnMount prop", and 1b1b598 names endpointSplicePosition.

Discussion since the last review. Two comments, both from @ArturoManzoli: the round-by-round summary handled above, and the bare /review that triggered this run. Neither disputes a finding beyond the two arguments recorded as 6.1 and 8.1.

Change map — what was established before judging

Claims (from the PR body, each checked against the code):

Claim	Verdict
"Reposition on map" option on the library load dialog, dropping the mission at 1:1 scale	Verified — `MissionPlanningView.vue:4337` adds the action; `startFreePlacement` (`useMissionPlacement.ts:696`) sets scale 100/100 and rotation 0
Drag, scale and rotate before merging	Verified — `useMissionPlacement.ts:292` (drag), `:325` (scale handles), `:398` (rotation handle)
"Insert mission from library here" on the segment radial menu	Verified — third entry in `segmentRadialMenuItems`, routed at `MissionPlanningView.vue:1993`
"Mission from library" entry on the map context menu	Verified with the same discrepancy as round 9 — it is a hover submenu (`ContextMenu.vue:170`–`:228`) holding two entries, "Add mission from library" and "Save mission to library". The save entry, the `canSaveCurrent` prop and the `missionLibraryOpenSaveOnMount` wiring are a second feature the body still does not mention
Context-menu adds biased toward the closer endpoint	Verified — `endpointSplicePosition` (`src/libs/mission/planning-endpoints.ts:11`) returns `0` or `null`, consumed by all four add paths
First and last waypoints highlighted in orange	Verified — `.endpoint-marker` / `#ff9800` at `MissionPlanningView.vue:5077`, threaded through `createWaypointMarkerHtml`'s new `isEndpoint` flag (`:3687`)
"Diff: +2,004 −149"	Verified — `pr.json` reports exactly +2,004 / −149 across 8 files, and the body no longer carries the pre-rebase claims

No failure-site bullet: the PR fixes no bug.

Entry points (walked outward to a DOM/leaflet handler, watcher or lifecycle hook; no changed function came back never):

Function	Reached from	Frequency
`ContextMenu.handleAddHereClick` (`:520`)	click on the first context-menu entry	per user action
`ContextMenu.handleAddMissionFromLibrary` / `handleSaveMissionToLibrary` (`:611`, `:616`)	click on a submenu entry	per user action
`MissionPlacementToolbar` `scaleXModel`/`scaleYModel`/`rotationModel` (`:171`–`:183`)	`v-model.number` on the three number inputs	per user action
`useMissionInsertion.finalizeMissionPlacement` (`:167`)	placement confirm, and the dialog's "At its original location"	per user action
`useMissionInsertion.cloneMissionForPlanning` / `appendMissionToPlanning` / `insertMissionIntoSegment` (`:75`, `:113`, `:136`)	the above	per user action
`useMissionPlacement.startFreePlacement` (`:696`)	dialog's "Reposition on map"	per user action
`useMissionPlacement` mouse-down/up handlers (`:292`, `:315`, `:325`, `:388`, `:398`, `:431`)	leaflet `mousedown`/`mouseup` on the preview polygon and handles	per user action
`useMissionPlacement.onPlacementMouseMove` / `onScaleHandleMouseMove` / `onRotationHandleMouseMove` (`:302`, `:339`, `:415`)	leaflet `mousemove` while a drag is live	per frame or pointer event
`useMissionPlacement.schedulePlacementPreviewRebuild` → `rebuildPlacementPreview` → `applyPreviewGeometry` (`:462`, `:687`, `:647`)	the three move handlers, the `watch` on the transform refs, map `zoomend` (`:790`)	per frame or pointer event
`useMissionPlacement.createPreviewLayers` (`:534`)	`startFreePlacement` only — the change behind 5.4	one-shot per placement
`useMissionPlacement.getPlacementBounds` / `updateToolbarPosition` (`:280`, `:661`)	the rebuild, and map `drag zoom move` via `onMapMove` (`:794`)	per frame or pointer event
`library.transformPlacementCoord` / `transformLocalMeters` / `projectCoordToOriginalLocal` / `unrotateToOriginalLocal` (`:326`, `:279`, `:311`, `:346`)	the rebuild and the scale-move handler, once per mission coordinate	per frame or pointer event
`library.computeOriginalLocalBounds` / `safeScalePercent` / `safeRotationRad` (`:359`, `:259`, `:266`)	placement start and confirm; `safeRotationRad` also per rebuild	one-shot / per frame
`screen-placement.positionPanelNearBounds` → `screenBounds` / `pickBestPosition` (`:117`, `:41`, `:67`)	`updateToolbarPosition`, and the pre-existing survey-confirm strip (`MissionPlanningView.vue:1734`)	per frame or pointer event
`MissionPlanningView.renderMeasureOverlay` (`:1405`)	`handleMapMouseMove` (`:1480`) on map `mousemove`, plus one `nextTick` call from `addSimplePathFromContextMenu`	per frame or pointer event
`MissionPlanningView.currentMeasureAnchor` (base `:1286`)	`renderMeasureOverlay`	per frame or pointer event
`MissionPlanningView.openSegmentRadialMenuFromContextMenu` (`:1959`)	`open-segment-radial-menu` emit from the context menu	per user action
`MissionPlanningView` context-menu add helpers (`:2094`, `:2106`, `:2111`, `:2127`, `:2134`)	context-menu emits	per user action
`MissionPlanningView.showContextMenu` nearest-segment block (`:2279`)	map `contextmenu`	per user action
`MissionPlanningView.handleKeyDown` placement branch (`:2731`)	window `keydown`	per user action
`MissionPlanningView.cancelFreePlacement` / `onConfirmPlacement` / `onResetPlacement` (`:3132`, `:3138`, `:3143`)	placement-toolbar buttons, and Escape for the first	per user action
`MissionPlanningView.openMissionLibrary` (`:4235`)	toolbar button, context-menu submenu, radial-menu entry	per user action
`MissionPlanningView.openMissionLibraryWithSaveDialog` (`:3094`)	context-menu submenu save entry	per user action
`MissionPlanningView.handleLoadMissionFromLibrary` (`:4288`)	`load-mission` emit from the library modal	per user action
`MissionPlanningView.loadDraftMission` (`:4156`)	`finalizeMissionPlacement` only	per user action
`MissionPlanningView.addWaypoint` (`:2950`)	`addWaypointFromClick`, `addWaypointFromContextMenu`, the simple-path branch of `onMapClick` (`:4451`)	per user action
`MissionPlanningView.createWaypointMarkerHtml` / `isEndpointWaypoint` (`:3687`, `:3710`)	`updateWaypointMarkers`, `addWaypointMarker`, `addWaypoint`, `applySelectedWaypointMarkerVisual`, `onMapClick`	per user action, once per waypoint
`MissionPlanningView.undoGenerateWaypoints` (base `:3820`)	survey undo button	per user action
`MissionPlanningView.toggleSimplePath` / `toggleSurvey` / `onMapClick`	toolbar, map click	per user action

Invariants.

A. A waypoint carries .endpoint-marker exactly when it is first or last in missionStore.currentPlanningWaypoints. Established by this PR, and closed this round. All six createWaypointMarkerHtml call sites in the base (:2958, :3606, :3916, :3962, :3982, :4180) now pass the flag, and every mutation site that can change which waypoint is first or last refreshes: drawMissionOnTheMap (base :3034), insertWaypointAtSegmentMidpoint (:2061), addWaypointFromClick (:2109), undo/redo (:2625, :2638, :2680), deleteSelectedSurvey (:2803), removeSelectedWaypoint (:3014), clear/reload (:3034), generateWaypointsFromSurvey (:3567), survey regeneration (:3733), the useMissionInsertion callback, and — new this round — undoGenerateWaypoints (:3988) and the simple-path branch of onMapClick (:4457). No site is left uncovered.

B. pendingSimplePathInsertIndex is null whenever isCreatingSimplePath is false. New this round, and it holds. The base has exactly four sites that clear isCreatingSimplePath, at :2357 (toggleSimplePath), :2366 (toggleSurvey), :2704 (handleKeyDown) and :4716 (the main-menu step watcher), and the diff adds the matching null to all four. Without it a later toolbar-started simple path would silently prepend.

C. endpointSplicePosition's 0 means "at the very start" in each consumer's own index convention. Three different conventions, all three correct: addWaypointFromContextMenu (:2094) and the simple-path branch (:4451) pass it as an array index to addWaypoint's new insertIndex; addSurveyFromContextMenu (:2127) writes it to segmentSurveyInsertIndex, which generateWaypointsFromSurvey consumes as an array splice index (base :3540), not a segment index — so 0 prepends rather than landing between waypoints 0 and 1; and addMissionFromLibraryContextMenu (:2134) maps 0 to segment -1, which insertMissionIntoSegment (useMissionInsertion.ts:136) splices at segmentIndex + 1 = 0. The guards agree too: showContextMenu measures segment proximity from event.latlng (:2279) while the helpers measure from currentCursorGeoCoordinates, and every one of the seven sites that opens the menu assigns that ref from the same event.latlng on the line above showContextMenu(event).

D. Placement state is torn down on every exit. Confirm, cancel, Escape and scope-dispose all funnel through cancelPlacementInternal, which clears the layers, cancels the pending animation frame and unhooks the map listeners; onScopeDispose (useMissionPlacement.ts:816) removes the zoomend hook. The view wraps it (:3132) to also drop placementInsertSegmentIndex. Complete.

6. UI / UX — 2 findings

6.7 (minor, carried from round 9, partly addressed, reprinted in full) — the load dialog still has three footer actions.

MissionPlanningView.vue:4321–:4346 builds the actions array as Cancel / "At its original location" / "Reposition on map". The styling half of this finding is fixed and is not in question: color: 'white' is gone from all three, and the committing action carries the shell's fill class (class: 'bg-[#FFFFFF33]', :4339), which is exactly the form the house direction asks for on a footer passed through useInteractionDialog's actions prop — and DialogActions.class is bound at InteractionDialog.vue:66, so it renders.

What remains is the count. The house direction is two actions at most, a dismiss on the left and a single primary on the right; this is a third. It is minor for the reason the guidelines give — multi-action footers still exist in the tree and this is the form being replaced, not a breach that reaches the user.

The fix that 1.9's resolution now makes natural: "at its original location" versus "reposition on map" is a choice about where the content goes, not a dismiss/commit pair, so it belongs in the dialog body — two radio options or two selectable cards above the footer — leaving Cancel on the left and one "Add" on the right. That also leaves room to name the third outcome (replacing the current planning) if it is ever added, without a four-button footer.

6.10 (nit) — the endpoint highlight overrides the survey entry/exit marker instead of composing with it.

createWaypointMarkerHtml (MissionPlanningView.vue:3687) appends both classes to the same element — entryExitClass at :3698 and the new endpointClass at :3699, producing waypoint-main-marker green-marker endpoint-marker. The new rule at :5077 sets background-color: #ff9800 !important, and .green-marker at :5072 sets background-color: #034103 with no !important, so the endpoint colour always wins. A waypoint that is both the mission's first (or last) and a survey entry/exit — which is the common case, since a mission that begins with a survey has exactly that waypoint at index 0 — loses the green entirely and reads only as an endpoint.

Deciding that the endpoint signal outranks the entry/exit one is defensible; doing it with an !important that no comment explains is what makes it a nit rather than nothing. Either compose the two (keep the green fill and carry the endpoint on the ring — .endpoint-marker { border-color: #ff9800 } alongside the existing 1.25× scale, which already distinguishes it on its own), or drop the !important and say in a comment that the cascade order is deliberate.

7. Code Quality & Style — 1 finding

The complexity report lists one trigger and it is answered without a finding, on the same grounds as last round: it gives renderMeasureOverlay at MissionPlanningView.vue:1405 a complexity of 20 with no base figure, but the diff (pr.diff hunk @@ -1369,10 +1399,13 @@) shows the body is the pre-existing handleMapMouseMove renamed, with the PR's own contribution being a parameter change and two evt && guards; the report puts depth at 1. That is inherited complexity, which the guidelines explicitly exempt. The report also states 548 functions measured across the 8 changed files with truncated false, so the silence on everything else is meaningful. Those figures are the report's, produced by the CI run for this head, not something reproduced here.

6.1 (major, carried from round 6, disputed, reprinted in full) — MissionPlanningView.vue takes another large net addition.

pr.json reports +353 / −145 on this file — +208 net, down from +369 last round. The base checkout has it at 5,218 lines, so it lands near 5,426. That still clears both halves of the file-growth test: a file already far beyond ~2,000 lines taking well over 100 net lines.

The extraction work done since round 9 is real and is not in question. screen-placement.ts (144 lines) took the positioning maths, planning-endpoints.ts (16) took the endpoint choice, and they join useMissionPlacement.ts (836), useMissionInsertion.ts (192), MissionPlacementToolbar.vue (183) and library.ts (+201) — 1,572 lines outside the view, which is the author's figure and it checks out against pr.json. Two of the three blocks this finding named in round 9 are gone from it: the toolbar-positioning block moved to src/libs/map/, and the placement wiring is down from ~140 lines to 63 (:3085–:3147).

What is left that could still move is the third block, and it is smaller than it was:

The context-menu routing wrappers and their one helper, MissionPlanningView.vue:2094–:2140 (~47 lines) — addWaypointFromContextMenu, getContextMenuEndpointSplicePosition, addSimplePathFromContextMenu, addSurveyFromContextMenu, addMissionFromLibraryContextMenu. The author's objection to a composable here is fair on its own terms: they touch currentCursorGeoCoordinates, segmentSurveyInsertIndex, pendingSimplePathInsertIndex, cursorLivePositionX/Y, planningMap and four view functions, and a wrapper taking all of those as options would cost more lines than it saves. But that is an argument against one particular extraction, not against the finding. The part of that block that is not view state is the ~14 lines inside addSimplePathFromContextMenu (:2186–:2194) that convert a client-space cursor position into a LatLng through the container rect — that is screen-placement.ts's subject, it needs only the map container and two numbers, and the same conversion already exists verbatim in refreshLiveMeasureOnMapMove at base :1441–:1443.

The finding stays open because the measurement it is made of has not changed category: +208 onto a 5,426-line file is still the PR piling bulk onto a file already past the threshold, and the guidelines are explicit that a finding is not downgraded because it shrank. Whether +208 of genuinely per-view wiring is an acceptable price for this feature is a judgement about this repository's direction rather than about the code, which is what the dispute is for.

8. Commit Hygiene — 1 finding

8.1 (minor, carried from round 9, disputed, reprinted in full) — one commit is too large to review as a unit.

The four commits are well-scoped in subject, follow the repository's scope: subject style, and none carries an issue reference. Everything this finding raised about the bodies is fixed and re-verified against pr.json: 8bbcb44 no longer claims to colocate buildCurrentMissionSnapshot and its neighbours, 4ee641f now says "the modal's existing openSaveOnMount prop", 1b1b598 names endpointSplicePosition rather than the old getEndpointInsertIndexForLatLng, and the PR body drops both the borrowed-[drop] figure and the #2795 dependency while its "+2,004 −149" matches pr.json exactly.

What remains is the size of 8bbcb44 ("mission-planning: add free-placement workflow for library missions"). It adds useMissionPlacement.ts (836), MissionPlacementToolbar.vue (183), useMissionInsertion.ts (192) and screen-placement.ts (144) and rewires the view on top — well over 1,300 lines in one step, which the guidelines flag on size alone.

The author's case for keeping it whole is that none of the four pieces exists without the feature. That is true of the composable and the toolbar, and it is the standard argument against splitting a feature by file. It is weaker for two of the four:

src/libs/map/screen-placement.ts is a pure extraction of code that already existed in the view, and it changes the survey-confirm strip, which is not part of this feature. That is a refactor of existing behaviour riding inside a feature commit — reviewable on its own, revertable on its own, and the one part of 8bbcb44 a reviewer would most want isolated because a regression there hits a surface the PR is not otherwise touching.
The segment-insert routing (insertMissionIntoSegment plus the radial-menu entry) is a second user-facing feature the commit's own message describes in its own paragraph.

Separately, and pointing at the same commit: the "at its original location" reroute through finalizeMissionPlacement is a modification of existing behaviour folded into the feature commit. 1.9 closed on the fix, but the change itself still cannot be reverted or backported without dragging the whole feature along.

Sections with nothing to report (8)

1. Correctness & Implementation Bugs — ✅ (invariants A–D above: all four isCreatingSimplePath clear sites also clear the new pendingSimplePathInsertIndex; all three consumers of endpointSplicePosition's 0 map it correctly to their own index convention, including the segmentSurveyInsertIndex path that is an array index rather than a segment index; unrotateToOriginalLocal (library.ts:346) is the exact inverse of the rotation in transformLocalMeters (:279); and the surveyLinesAngle + rotationDeg in confirmFreePlacement matches the 90 - surveyLinesAngle compass convention the line generator uses at base MissionPlanningView.vue:3501)

2. Persistence & User Data — ✅ (no persisted key is added, reshaped or removed; currentPlanningWaypoints/currentPlanningSurveys are plain reactive arrays at src/stores/mission.ts:246–:247, and the only persisted values in reach are cockpit-user-last-map-center / cockpit-user-last-map-zoom via the mapCenter/zoom refs, whose write loadDraftMission (:4156) now skips only on the repositioned path — the correction that closed 1.9)

3. AGENTS.md Adherence — ✅ (no dependency added, so no package.json ordering or yarn question; every added exported function and TSPropertySignature across screen-placement.ts, planning-endpoints.ts, library.ts, useMissionInsertion.ts, useMissionPlacement.ts and the inline option types in the view carries a non-empty typed JSDoc; comments explain why; the handleMapMouseMove → renderMeasureOverlay split serves the context-menu measure line rather than being a drive-by rename; and 7.2's two minimalism breaches closed this round with the watcher deleted and the export dropped)

4. Security — ✅ (checked all nine sub-checks: no new dependency, no network call, no eval/Function/v-html, no secret or env-var use, no build/CI/electron change; the L.divIcon HTML template literals in useMissionPlacement.ts interpolate only local literals from the corner array declared alongside them; no hidden Unicode or encoded blob in the diff, and nothing in pr.diff, the PR body, new-comments.json or complexity-report.json reads as an instruction addressed to a reviewer)

5. Performance — ✅ (5.4 closed — the per-frame path is now applyPreviewGeometry at useMissionPlacement.ts:647, which reassigns geometry on layers built once, with clearPlacementLayers reached only from teardown; the nextTick(renderMeasureOverlay) added at MissionPlanningView.vue:2193 fires once per menu click, not per frame; and invariant D confirms every listener, the animation frame and the zoomend hook have a matching teardown)

9. Tests — ✅ (no test file is touched, and nothing existing was removed or weakened; the two new src/libs/ modules are pure functions, which improves testability rather than harming it)

10. Documentation — ✅ (the placement feature behaves identically in Lite and Standalone — no electronAPI, filesystem or process use anywhere in the diff — so the AGENTS.md README rule is not triggered; the docs-needed label is already on the PR for the user-facing side)

11. Nitpicks / Optional — ✅ (checked the new labels and menu entries for sentence case, tooltip and aria-label coverage on all six placement-toolbar controls, the four new logUserAction strings against the past-tense house voice, and the two new src/libs/ module names against what they contain — nothing worth raising beyond 6.10)

Generated by Claude. This is advisory; a human reviewer must still approve.

github-actions · 2026-08-21T16:24:06Z

🙋 Decision needed — 6.1

MissionPlanningView.vue takes another large net addition (+353/-145 onto 5,218 lines)

The author's argument: The view is down to +208 net because roughly 1,570 lines of geometry, placement state machine, merge routing, positioning maths, endpoint choice and toolbar controls were moved out to six other files, and what is left is per-view wiring with nowhere else to go — two composable instantiations and new branches inside the view's own existing functions — with the context-menu handlers the only movable block, and they touch a dozen view-local refs so a composable around them would add lines overall for a wrapper with a single call site.

React to this comment and the next /review applies the answer:

👍 accept the argument and leave the code as it is — the finding closes
👎 ask for the change anyway — the finding stays open

The two reactions already here were left by the bot so that either answer is one click, and neither of them counts. Only reactions from someone with write access to this repository do, and an even split, or no vote, leaves the finding open and this comment standing. Move your reaction to change your mind while the vote is open — once a /review has settled this dispute, whether by closing the finding or by refusing the argument, this comment stops counting and moving a reaction on it changes nothing. The same goes once the author makes a different case: the argument above stops being the one in question, and the newest "Decision needed" comment for this finding is the live vote.

github-actions · 2026-08-21T16:24:07Z

🙋 Decision needed — 8.1

Oversized free-placement commit 8bbcb44

The author's argument: The free-placement commit should stay whole rather than being split into the composable, the toolbar and the wiring, because none of the three exists without the feature and apart they are three commits whose pieces cannot be reviewed on their own.

React to this comment and the next /review applies the answer:

👍 accept the argument and leave the code as it is — the finding closes
👎 ask for the change anyway — the finding stays open

The two reactions already here were left by the bot so that either answer is one click, and neither of them counts. Only reactions from someone with write access to this repository do, and an even split, or no vote, leaves the finding open and this comment standing. Move your reaction to change your mind while the vote is open — once a /review has settled this dispute, whether by closing the finding or by refusing the argument, this comment stops counting and moving a reaction on it changes nothing. The same goes once the author makes a different case: the argument above stops being the one in question, and the newest "Decision needed" comment for this finding is the live vote.

ArturoManzoli · 2026-08-21T16:31:13Z

Done:

(6.10) the endpoint highlight moved to the marker's ring, so a waypoint that is both a mission endpoint and a survey entry/exit keeps the green fill underneath it, and the unexplained !important is gone
(6.1) took the concession in the finding: resolving a cached cursor position against the map is now mapPointerPositionFromClient in libs/map/utils-map, shared with the pre-existing refreshLiveMeasureOnMapMove instead of repeated in addSimplePathFromContextMenu

Not addressed:

(6.7) moving the where-does-it-go choice out of the footer needs body content on useInteractionDialog, which today takes only a message and an actions array, so closing this means widening the shared dialog API for one call site and is worth deciding on its own rather than inside this PR
(6.1) the view is still around +210 net. The remaining context-menu wrappers are the block the finding and I agree cannot move without a wrapper that costs more than it saves, so the rest is per-view wiring and new branches inside the view's own existing functions
(8.1) keeping the free-placement commit whole rather than splitting the composable, the toolbar and the wiring: none of the three exists without the feature, and apart they are three commits whose pieces can't be reviewed on their own

ArturoManzoli · 2026-08-21T18:01:23Z

/review

github-actions · 2026-08-21T18:13:14Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 11)

4 open — 1 major, 2 minor and 1 nit — and 18 closed.

The pull request lets a saved mission be dropped onto the planning map and dragged, resized and turned before it is committed, adds two more ways to reach the mission library (a submenu on the map right-click menu and an entry on the between-waypoints radial menu), makes the right-click "add" actions attach the new item to whichever end of the existing mission the click was nearest, and rings the mission's first and last waypoints in orange so those ends are visible. Since the last round the author amended the final commit twice over: the endpoint highlight moved from the marker's fill to its ring, and the client-to-map cursor conversion was pulled into a shared helper.

What still needs attention

#	Problem	What it means	Severity	Status
6.1	More bulk added to an already very large file	The main mission-planning file keeps growing, which makes every future change to mission planning slower and riskier to review.	major	💬
6.7	Three buttons in the load dialog	The dialog that asks where to put a saved mission still offers three choices where the house pattern is a dismiss and one action, so the decision is heavier than it needs to be.	minor	💬
8.1	One commit too big to review as a unit	The main feature arrives as a single step of well over a thousand lines, so a reviewer cannot check any part of it in isolation.	minor	💬
8.2	Commit message describes the old version of the fix	The last commit says the end markers are filled orange and then says they are not, so anyone reading the history later learns the wrong thing about what the change did.	nit	❌

Since round 10 — 1 closed, comparing 1b1b598 → da069a1

The increment is unusable again, for a new reason. incremental.diff lists four files and reports src/libs/mission/planning-endpoints.ts as added — but round 10 already cited endpointSplicePosition at planning-endpoints.ts:11, so that file existed at 1b1b598 and cannot have appeared since. Its ContextMenu.vue figure (+12/−4) also disagrees with pr.json's (+79/−4). This is not another rebase: the base is still aa40c07 and the first three shas (e3792c6, 8bbcb44, 4ee641f) are unchanged from round 10. Only the fourth moved, 1b1b598 → da069a1, which means it was amended rather than added to, so the three-dot compare bottoms out at their common parent 4ee641f and replays the whole of that fourth commit instead of the delta. Every status below was judged against pr.diff and the base checkout.

What actually changed is recoverable from pr.json's per-file counts against round 10's: src/libs/map/utils-map.ts is new to the PR (+28), MissionPlanningView.vue went from +353/−145 to +362/−149, and the other seven files carry byte-identical counts. That is two edits — the .endpoint-marker rule and the cursor-conversion extraction — plus two paragraphs appended to da069a1's message.

previous-ledger.json was readable and carried all twenty-one entries, so nothing had to be rebuilt from prose.

resolutions.json is []: no maintainer has resolved anything on this pull request, nothing was applied, and no id went unmatched.

decisions.json holds two entries, both with gated true, so both were checked against the ledger. Both are pending with no +1 and no -1: the vote on 6.1 (decision comment) and the vote on 8.1 (decision comment) are still open, nobody entitled to decide has reacted, and neither changed anything in the ledger this round.

Findings whose status changed this round:

#	Finding	Status
6.10	Orange endpoint marker overrode the green survey marker	✅ Addressed — see the note below
6.7	Three identically styled footer actions	💬 Disputed — the author's reason checks out against the shared dialog's API; still open, reprinted in full in section 6

On 6.10, both halves landed. The rule at MissionPlanningView.vue:5082 is now border: 2px solid #ff9800 with transform: scale(1.25), the !important is gone, and a two-line comment above it says why the ring rather than the fill. It composes as the finding asked: .endpoint-marker and .green-marker are both single-class selectors and the endpoint rule is written directly after .green-marker (:5077), so the later border wins while .green-marker's background-color: #034103 survives untouched. A waypoint that is both the mission start and a survey entry now shows green with an orange ring.

One residue is worth recording without turning into a finding. .selected-marker also sets a border, so at the default marker size a selected endpoint takes the orange ring in place of the yellow selection ring (the selection's glow still distinguishes it), and at the xs/sm sizes the two-class .wp-marker-xs .selected-marker outranks .endpoint-marker, so there the orange ring disappears while the marker is selected. In both directions the 1.25× scale still marks the endpoint, and — the reason this is an observation rather than a new finding — the remedy round 10 itself proposed (.endpoint-marker { border-color: #ff9800 }) has exactly the same cascade behaviour. It is worth one line in the same comment that already explains the ordering.

On 6.1, the concession landed but the measurement moved the wrong way. mapPointerPositionFromClient now exists at src/libs/map/utils-map.ts:60, with a typed JSDoc and an exported MapPointerPosition, and both call sites go through it — refreshLiveMeasureOnMapMove (MissionPlanningView.vue:1473) and addSimplePathFromContextMenu (:2118). A grep for getBoundingClientRect() across the view, Map.vue and src/libs/map/ finds no third copy of that conversion. libs/map/utils-map is a better home than the screen-placement.ts the finding suggested, since that module is about panel positioning. But the view's own net went up, not down: +362/−149 is +213 net against round 10's +208, because the explanatory CSS comment and the multi-line helper call cost slightly more than the four inline lines they replaced. 6.1 stays as it was.

On 6.7, the author's reason is correct and the finding is now disputed rather than simply unaddressed. The claim is that moving the choice into the dialog body needs body content on useInteractionDialog. Checked: DialogOptions (src/composables/interactionDialog.ts:12–:51) exposes only message: string | string[], variant, title, actions, maxWidth, persistent and timer; DialogActions (src/types/general.ts:26) is text/action/color/size/class/disabled; and mountDialog (:119) creates the component with createApp(InteractionDialogComponent, props), so a caller cannot pass a slot at all. Closing 6.7 the way round 10 described would mean widening a shared API used across the tree for one call site. That is an argument about how to fix it, not a fix, so the finding stays open and goes to a vote.

Discussion since the last review. Two comments, both from @ArturoManzoli: the "Done / Not addressed" summary handled above, and the bare /review that triggered this run. Every claim in the first was checked against the code and all of them hold, with one number to correct — "the view is still around +210 net" is +213 by pr.json.

Change map — what was established before judging

Claims (from the PR body, each checked against the code):

Claim	Verdict
"Reposition on map" option on the library load dialog, dropping the mission at 1:1 scale	Verified — `MissionPlanningView.vue:4339` adds the action; `startFreePlacement` (`useMissionPlacement.ts:696`) sets scale 100/100 and rotation 0
Drag, scale and rotate before merging	Verified — `useMissionPlacement.ts:292` (drag), `:325` (scale handles), `:398` (rotation handle)
"Insert mission from library here" on the segment radial menu	Verified — third entry in `segmentRadialMenuItems`, routed at `MissionPlanningView.vue:1993`
"Mission from library" entry on the map context menu	Verified with the same discrepancy as rounds 9 and 10 — it is a hover submenu (`ContextMenu.vue:170`–`:228`) holding two entries, "Add mission from library" and "Save mission to library". The save entry, the `canSaveCurrent` prop and the `missionLibraryOpenSaveOnMount` wiring are a second feature the body still does not mention
Context-menu adds biased toward the closer endpoint	Verified — `endpointSplicePosition` (`src/libs/mission/planning-endpoints.ts:11`) returns `0` or `null`, consumed by all four add paths
First and last waypoints highlighted in orange	Verified, and changed this round — it is now an orange ring, `.endpoint-marker` / `border: 2px solid #ff9800` at `MissionPlanningView.vue:5082`, threaded through `createWaypointMarkerHtml`'s `isEndpoint` flag (`:3689`)
"Diff: +2,004 −149"	Contradicted — `pr.json` reports +2,041 / −153 across 9 files. It matched exactly at round 10; this round's two edits moved it. Recorded under 8.2

No failure-site bullet: the PR fixes no bug.

Entry points (walked outward to a DOM/leaflet handler, watcher or lifecycle hook; no changed function came back never):

Function	Reached from	Frequency
`ContextMenu.handleAddHereClick` (`:520`)	click on the first context-menu entry	per user action
`ContextMenu.handleAddMissionFromLibrary` / `handleSaveMissionToLibrary` (`:611`, `:616`)	click on a submenu entry	per user action
`MissionPlacementToolbar` `scaleXModel`/`scaleYModel`/`rotationModel` (`:171`–`:183`)	`v-model.number` on the three number inputs	per user action
`useMissionInsertion.finalizeMissionPlacement` (`:167`)	placement confirm, and the dialog's "At its original location" (`MissionPlanningView.vue:4336`)	per user action
`useMissionInsertion.cloneMissionForPlanning` / `appendMissionToPlanning` / `insertMissionIntoSegment` (`:75`, `:113`, `:136`)	the above	per user action
`useMissionPlacement.startFreePlacement` (`:696`)	dialog's "Reposition on map"	per user action
`useMissionPlacement` mouse-down/up handlers (`:292`, `:315`, `:325`, `:388`, `:398`, `:431`)	leaflet `mousedown`/`mouseup` on the preview polygon and handles	per user action
`useMissionPlacement.onPlacementMouseMove` / `onScaleHandleMouseMove` / `onRotationHandleMouseMove` (`:302`, `:339`, `:415`)	leaflet `mousemove` while a drag is live	per frame or pointer event
`useMissionPlacement.schedulePlacementPreviewRebuild` → `rebuildPlacementPreview` → `applyPreviewGeometry` (`:462`, `:687`, `:647`)	the three move handlers, the `watch` on the transform refs, map `zoomend` (`:790`)	per frame or pointer event
`useMissionPlacement.createPreviewLayers` (`:534`)	`startFreePlacement` only	one-shot per placement
`useMissionPlacement.getPlacementBounds` / `updateToolbarPosition` (`:280`, `:661`)	the rebuild, and map `drag zoom move` via `onMapMove` (`:794`)	per frame or pointer event
`library.transformPlacementCoord` / `transformLocalMeters` / `projectCoordToOriginalLocal` / `unrotateToOriginalLocal` (`:326`, `:279`, `:311`, `:346`)	the rebuild and the scale-move handler, once per mission coordinate	per frame or pointer event
`library.computeOriginalLocalBounds` / `safeScalePercent` / `safeRotationRad` (`:359`, `:259`, `:266`)	placement start and confirm; `safeRotationRad` also per rebuild	one-shot / per frame
`screen-placement.positionPanelNearBounds` → `screenBounds` / `pickBestPosition` (`:117`, `:41`, `:67`)	`updateToolbarPosition`, and the pre-existing survey-confirm strip	per frame or pointer event
`utils-map.mapPointerPositionFromClient` (`:60`) — new this round	`refreshLiveMeasureOnMapMove` (`MissionPlanningView.vue:1473`) on map `drag`/`zoom`, and `addSimplePathFromContextMenu` (`:2118`)	per frame or pointer event
`MissionPlanningView.renderMeasureOverlay` (`:1406`)	`handleMapMouseMove` (`:1482`) on map `mousemove`, plus one `nextTick` call from `addSimplePathFromContextMenu` (`:2123`)	per frame or pointer event
`MissionPlanningView.currentMeasureAnchor` (`:1316`)	`renderMeasureOverlay`	per frame or pointer event
`MissionPlanningView.openSegmentRadialMenuFromContextMenu` (`:1961`)	`open-segment-radial-menu` emit from the context menu	per user action
`MissionPlanningView` context-menu add helpers (`:2096`, `:2106`, `:2110`, `:2127`, `:2134`)	context-menu emits	per user action
`MissionPlanningView.showContextMenu` nearest-segment block (`:2281`)	map `contextmenu`	per user action
`MissionPlanningView.handleKeyDown` placement branch (`:2734`–`:2739`)	window `keydown`	per user action
`MissionPlanningView.cancelFreePlacement` / `onConfirmPlacement` / `onResetPlacement` (`:3134`, `:3140`, `:3145`)	placement-toolbar buttons, and Escape for the first	per user action
`MissionPlanningView.openMissionLibrary` (`:4237`)	toolbar button, context-menu submenu, radial-menu entry	per user action
`MissionPlanningView.openMissionLibraryWithSaveDialog` (`:3096`)	context-menu submenu save entry	per user action
`MissionPlanningView.handleLoadMissionFromLibrary` (`:4290`)	`load-mission` emit from the library modal	per user action
`MissionPlanningView.loadDraftMission` (`:4158`)	`finalizeMissionPlacement` only	per user action
`MissionPlanningView.addWaypoint` (`:2952`)	`addWaypointFromClick`, `addWaypointFromContextMenu`, the simple-path branch of `onMapClick` (`:4454`)	per user action
`MissionPlanningView.createWaypointMarkerHtml` / `isEndpointWaypoint` (`:3689`, `:3712`)	`updateWaypointMarkers`, `addWaypointMarker`, `addWaypoint`, `applySelectedWaypointMarkerVisual`, `onMapClick`	per user action, once per waypoint
`MissionPlanningView.undoGenerateWaypoints` (base `:3820`)	survey undo button	per user action
`MissionPlanningView.toggleSimplePath` / `toggleSurvey` / `onMapClick`	toolbar, map click	per user action

Invariants.

A. A waypoint carries .endpoint-marker exactly when it is first or last in missionStore.currentPlanningWaypoints. Re-verified unchanged: all six createWaypointMarkerHtml call sites pass the flag, and every mutation site that can change which waypoint is first or last refreshes the markers — including addWaypointFromContextMenu (:2103), undoGenerateWaypoints (:3991) and the simple-path branch of onMapClick (:4463). No site is left uncovered.

B. pendingSimplePathInsertIndex is null whenever isCreatingSimplePath is false. Holds. The four sites that clear isCreatingSimplePath all clear it too: toggleSimplePath (:2393), toggleSurvey (:2403), handleKeyDown (:2748) and the main-menu step watcher (:4921).

C. endpointSplicePosition's 0 means "at the very start" in each consumer's own index convention. Three conventions, all three correct: addWaypointFromContextMenu (:2096) and the simple-path branch (:4453) pass it as an array index to addWaypoint's insertIndex; addSurveyFromContextMenu (:2127) writes it to segmentSurveyInsertIndex, consumed by generateWaypointsFromSurvey as an array splice index; and addMissionFromLibraryContextMenu (:2134) maps 0 to segment -1, which insertMissionIntoSegment (useMissionInsertion.ts:136) splices at segmentIndex + 1 = 0. Every one of the seven sites that opens the menu assigns currentCursorGeoCoordinates from the same event.latlng the nearest-segment block at :2281 measures from.

D. Placement state is torn down on every exit. Confirm, cancel, Escape and scope-dispose all funnel through cancelPlacementInternal, which clears the layers, cancels the pending animation frame and unhooks the map listeners; onScopeDispose (useMissionPlacement.ts:816) removes the zoomend hook. The view wraps it (:3134) to also drop placementInsertSegmentIndex.

E. Every cached client-space cursor position is resolved against the map through one helper. New this round and it holds: getBoundingClientRect() on a map container appears nowhere else in MissionPlanningView.vue, Map.vue or src/libs/map/ after the extraction — the one other hit in the view is on a panel element, not the map.

6. UI / UX — 1 finding

6.7 (minor, carried from round 9, now disputed, reprinted in full) — the load dialog still has three footer actions.

MissionPlanningView.vue:4324–:4348 builds the actions array as Cancel / "At its original location" / "Reposition on map". The styling half of this finding is fixed and is not in question: color: 'white' is gone from all three, and the committing action carries the shell's fill class (class: 'bg-[#FFFFFF33]', :4341), which is the form the house direction asks for on a footer passed through useInteractionDialog's actions prop — and DialogActions.class is bound at InteractionDialog.vue:66, so it renders.

What remains is the count. The house direction is two actions at most, a dismiss on the left and a single primary on the right; this is a third. It is minor for the reason the guidelines give — multi-action footers still exist in the tree and this is the form being replaced, not a breach that reaches the user.

The author's objection is verified and is the reason this is now disputed rather than simply unaddressed: DialogOptions (src/composables/interactionDialog.ts:12–:51) carries only message, variant, title, actions, maxWidth, persistent and timer, and mountDialog (:119) instantiates the component through createApp(InteractionDialogComponent, props), so there is no slot a caller could fill. Radio options or selectable cards in the body — round 10's suggested fix — do mean widening a shared API for one call site.

There is a second route that needs no dialog change, and it is worth putting to the same vote: fold "at its original location" into the placement flow instead of the footer. startFreePlacement (useMissionPlacement.ts:696) currently anchors the preview at the map centre; anchoring it at the mission's own coordinates when the planner is empty makes "confirm without moving it" the original-location outcome, which leaves Cancel on the left and one "Add" on the right with no new dialog capability. That trades a footer button for a change in where the preview lands, which is a real cost — but it is a different trade from widening DialogOptions, and the dispute should be decided knowing both exist.

7. Code Quality & Style — 1 finding

The complexity report lists one trigger and it is answered without a finding, on the same grounds as the last two rounds: it gives renderMeasureOverlay at MissionPlanningView.vue:1406 a complexity of 20 with no base figure, but the diff (pr.diff hunk @@ -1369,10 +1400,13 @@) shows the body is the pre-existing handleMapMouseMove renamed, with the PR's own contribution being a parameter change and two evt && guards; the report puts depth at 1. That is inherited complexity, which the guidelines explicitly exempt. The report also states 593 functions measured across the 9 changed files with truncated false — up from 548 across 8, consistent with the new utils-map.ts helper — so the silence on everything else, including that helper, is meaningful. Those figures are the report's, produced by the CI run for this head, not something reproduced here.

6.1 (major, carried from round 6, disputed, reprinted in full) — MissionPlanningView.vue takes another large net addition.

pr.json reports +362 / −149 on this file — +213 net, up from +208 last round. The base checkout has it at 5,218 lines, so it lands near 5,431. That still clears both halves of the file-growth test: a file already far beyond ~2,000 lines taking well over 100 net lines.

The extraction work is real and is not in question. screen-placement.ts (144 lines) took the positioning maths, planning-endpoints.ts (16) took the endpoint choice, utils-map.ts (+28) took the client-to-map cursor conversion this round, and they join useMissionPlacement.ts (836), useMissionInsertion.ts (192), MissionPlacementToolbar.vue (183) and library.ts (+201) — 1,600 lines outside the view. All three blocks this finding named in round 9 are now gone from it: the toolbar-positioning block moved to src/libs/map/, the placement wiring is down to 63 lines (:3087–:3149), and the cursor conversion inside addSimplePathFromContextMenu — the concession the finding itself offered — is now mapPointerPositionFromClient at src/libs/map/utils-map.ts:60, shared with refreshLiveMeasureOnMapMove (:1473).

What is left is the context-menu routing block, MissionPlanningView.vue:2096–:2140 (~45 lines): addWaypointFromContextMenu, getContextMenuEndpointSplicePosition, addSimplePathFromContextMenu, addSurveyFromContextMenu, addMissionFromLibraryContextMenu. The author's objection to a composable here is accepted on its own terms and has been since round 10: they touch currentCursorGeoCoordinates, segmentSurveyInsertIndex, pendingSimplePathInsertIndex, cursorLivePositionX/Y, planningMap and four view functions, and a wrapper taking all of those as options would cost more lines than it saves. There is no longer a specific block this finding can point at.

It stays open because the measurement it is made of has not changed category, and this round it moved the wrong way: extracting 28 lines to utils-map.ts left the view at +213 rather than below +208, because the two-line CSS comment and the multi-line helper call cost more than the four inline lines they replaced. +213 onto a 5,431-line file is still the PR piling bulk onto a file already past the threshold, and the guidelines are explicit that a finding is not downgraded because it shrank. Whether +213 of genuinely per-view wiring is an acceptable price for this feature is a judgement about this repository's direction rather than about the code, which is what the dispute is for.

8. Commit Hygiene — 2 findings

8.1 (minor, carried from round 9, disputed, reprinted in full) — one commit is too large to review as a unit.

The four commits are well-scoped in subject, follow the repository's scope: subject style, and none carries an issue reference. Everything this finding raised about the bodies was fixed in round 10 and still holds: 8bbcb44 no longer claims to colocate buildCurrentMissionSnapshot and its neighbours, and 4ee641f says "the modal's existing openSaveOnMount prop".

What remains is the size of 8bbcb44 ("mission-planning: add free-placement workflow for library missions"). It adds useMissionPlacement.ts (836), MissionPlacementToolbar.vue (183), useMissionInsertion.ts (192) and screen-placement.ts (144) and rewires the view on top — well over 1,300 lines in one step, which the guidelines flag on size alone.

The author's case for keeping it whole is that none of the four pieces exists without the feature. That is true of the composable and the toolbar, and it is the standard argument against splitting a feature by file. It is weaker for two of the four:

src/libs/map/screen-placement.ts is a pure extraction of code that already existed in the view, and it changes the survey-confirm strip, which is not part of this feature. That is a refactor of existing behaviour riding inside a feature commit — reviewable on its own, revertable on its own, and the one part of 8bbcb44 a reviewer would most want isolated because a regression there hits a surface the PR is not otherwise touching.
The segment-insert routing (insertMissionIntoSegment plus the radial-menu entry) is a second user-facing feature the commit's own message describes in its own paragraph.

Separately, and pointing at the same commit: the "at its original location" reroute through finalizeMissionPlacement is a modification of existing behaviour folded into the feature commit. 1.9 closed on the fix, but the change itself still cannot be reverted or backported without dragging the whole feature along.

8.2 (nit, new) — the amended commit describes the version of the fix it replaced, and the PR body's diff figure has gone stale.

da069a1's body says the endpoints are "rendered with a new .endpoint-marker style (orange fill via #ff9800, scaled up to 1.25x around the center)", and three paragraphs later says "The highlight rides on the marker's ring rather than its fill". The code at MissionPlanningView.vue:5082–:5086 sets border: 2px solid #ff9800 and no background-color, so the first description is the wrong one — this round's fix was appended to the message rather than folded into the paragraph it supersedes. Amend that paragraph to say ring instead of fill and drop the trailing correction; the commit gets read once, by someone who has no idea an earlier version existed.
The PR body still reads "Diff: +2,004 −149" while pr.json reports +2,041 / −153 across nine files. It matched exactly at round 10, so this is drift from this round's two edits rather than a figure that was never right.

nit because neither reaches a user or a future bisect, and the second re-stales on every push.

Sections with nothing to report (8)

1. Correctness & Implementation Bugs — ✅ (invariants A–E above; re-checked this round's two edits specifically — mapPointerPositionFromClient (utils-map.ts:60) reproduces the replaced inline arithmetic exactly, rect.left/rect.top subtraction then containerPointToLatLng, and both call sites still guard planningMap.value before entering it, so no new null path opens)

2. Persistence & User Data — ✅ (no persisted key is added, reshaped or removed, and this round's edits add none; currentPlanningWaypoints/currentPlanningSurveys are plain reactive arrays at src/stores/mission.ts:246–:247, and the only persisted values in reach remain cockpit-user-last-map-center / cockpit-user-last-map-zoom via the mapCenter/zoom refs, whose write loadDraftMission skips only on the repositioned path)

3. AGENTS.md Adherence — ✅ (no dependency added; the new mapPointerPositionFromClient and its MapPointerPosition type carry non-empty typed JSDoc including both TSPropertySignature members, which .eslintrc.cjs:39 requires; the new CSS comment explains why the ring rather than what the rule does; the helper lands in libs/map/utils-map.ts, whose stated subject is leaflet map helpers, rather than widening screen-placement.ts's panel-positioning purpose; and the extraction deletes a duplicate rather than adding an abstraction, which is the direction the minimalism ladder asks for)

4. Security — ✅ (re-ran all nine sub-checks over the whole diff: no new dependency, no network call, no eval/Function/v-html, no secret or env-var use, no build/CI/electron change; the new helper reads a DOM rect and does arithmetic; the L.divIcon HTML template literals in useMissionPlacement.ts interpolate only local literals; no hidden Unicode or encoded blob, and nothing in pr.diff, the PR body, new-comments.json, resolutions.json, decisions.json or complexity-report.json reads as an instruction addressed to a reviewer)

5. Performance — ✅ (the extracted helper sits on a per-frame path — refreshLiveMeasureOnMapMove inside a requestAnimationFrame — and adds nothing to it: same one getBoundingClientRect() read per frame as before, one small object allocated where four locals were, no new listener; invariant D confirms every listener, the animation frame and the zoomend hook still have a matching teardown)

9. Tests — ✅ (no test file is touched, and nothing existing was removed or weakened; mapPointerPositionFromClient is the third pure function this PR moves into src/libs/, which improves testability rather than harming it)

10. Documentation — ✅ (this round's edits are a CSS rule and a pure helper, neither of which differs between Lite and Standalone — no electronAPI, filesystem or process use anywhere in the diff — so the AGENTS.md README rule is still not triggered; the docs-needed label is already on the PR)

11. Nitpicks / Optional — ✅ (checked the new helper's name against what it returns, its JSDoc against the @param/@returns house form, the new CSS comment against the comment-immutability rule — it is added, nothing existing was reworded — and re-checked the marker cascade at all four size classes and both selection states, whose one residue is recorded in the since-round-10 block rather than raised, since round 10's own proposed remedy shares it)

Generated by Claude. This is advisory; a human reviewer must still approve.

github-actions · 2026-08-21T18:13:15Z

🙋 Decision needed — 6.7

Load-mission dialog still has three footer actions

The author's argument: Moving the where-does-it-go choice out of the load dialog's footer would need body content on the shared useInteractionDialog, which today accepts only a message and an array of actions, so closing this means widening a shared dialog API for one call site and is better decided on its own than inside this PR.

React to this comment and the next /review applies the answer:

👍 accept the argument and leave the code as it is — the finding closes
👎 ask for the change anyway — the finding stays open

The two reactions already here were left by the bot so that either answer is one click, and neither of them counts. Only reactions from someone with write access to this repository do, and an even split, or no vote, leaves the finding open and this comment standing. Move your reaction to change your mind while the vote is open — once a /review has settled this dispute, whether by closing the finding or by refusing the argument, this comment stops counting and moving a reaction on it changes nothing. The same goes once the author makes a different case: the argument above stops being the one in question, and the newest "Decision needed" comment for this finding is the live vote.

The screen-space maths that keeps a floating panel beside a target lives in `libs/map/screen-placement` as pure functions, so the survey-confirm strip reads its position from a shared implementation instead of an inline copy and a later panel can reuse it. Behaviour is unchanged: the strip declares its own footprint plus the two hand-tuned offsets it has always carried, so it still lands where it did before.

Extend the library load flow with a "Reposition on map" option that drops the saved mission onto the planning map at 1:1 scale and lets the user drag, scale (independent X/Y, hold Shift for uniform), and rotate it via on-map handles plus numeric overlays before committing. A green check confirms the placement, a red trash cancels, Escape backs out, and a restore button resets scale and rotation to defaults. The interactive state machine (drag, scale-corner handles, rotation handle, raf-coalesced preview update, snackbar hint) lives in a leaflet-aware composable (`useMissionPlacement`) so the host view only owns mission routing. The composable manages its own preview layers, mouse listeners, zoom-end and map-move rebuilds, and the toolbar's screen position, and tears everything down on scope dispose to keep the view's onUnmounted free of placement code. Because the layer set is fixed by the mission being placed, the layers are built once and each frame only reassigns their geometry, instead of removing and re-adding every polygon, marker and handle at pointer frequency. The toolbar keeps itself beside the mission through the shared `libs/map/screen-placement` helpers and exports the footprint that describes its own rendered size, so it and the survey-confirm strip share one implementation instead of two copies. Also add the segment-insert third option to the radial menu shown when hovering between two existing waypoints ("Insert mission from library here") and the routing in finalizeMissionPlacement / appendMissionToPlanning / insertMissionIntoSegment so loading a library mission either: - splices into the requested segment (when the load came from the radial menu), preserving waypoint ids consistently between the top-level and survey-internal lists so editing the survey later doesn't leave an orphan polygon on the map; - appends to the current planning when the planner already has work in progress (so a second library load doesn't wipe the first), with the dialog naming that outcome instead of offering to "load"; or - replays the saved settings when the planner is empty, restoring the saved centre and zoom unless the mission was just repositioned by hand.

Replace the single "Mission library" entry plan with a hover submenu in the map context menu that exposes "Save mission to library" (only enabled when there's something to save) and "Add mission from library". The save entry opens the library modal directly on the save form by feeding the modal's existing `openSaveOnMount` prop from a new `missionLibraryOpenSaveOnMount` flag, while the add entry reuses the existing toolbar entry point. Both segment-insert intent and save-on-mount intent are now cleared on every fresh open of the library so a plain toolbar open never inherits stale flags from a previous interaction.

The "Add waypoint", "Add survey", "Add simple path", and "Add mission from library" actions on the map context menu now check which mission endpoint is closer to the click and prepend instead of append when it lands closer to the start. Segment-proximity insertion still wins when the cursor is on or near an existing segment. addWaypoint takes an optional insertIndex, the pure `endpointSplicePosition` helper in `libs/mission/planning-endpoints` drives the choice, and insertMissionIntoSegment accepts -1 for "insert at the very start" with the exclusive upper bound documented so future callers route append-to-end through appendMissionToPlanning. To make the bias UX-discoverable, the first and last waypoints of the current mission are rendered with a new `.endpoint-marker` style (an orange `#ff9800` ring, scaled up to 1.25x around the center) so it's visually obvious which waypoints are the mission's start and end. The highlight rides on the marker's ring rather than its fill, so a waypoint that is both a mission endpoint and a survey entry/exit keeps the green underneath it. The `endpoint-marker` class is added through `createWaypointMarkerHtml`'s new `isEndpoint` flag, computed via the small `isEndpointWaypoint(id)` helper and threaded through every place that builds or refreshes a waypoint icon (updateWaypointMarkers, both marker-creation paths, applySelectedWaypointMarkerVisual, and the click-clear handler) so the highlight stays consistent across selection, drag, zoom, and survey entry/exit re-renders. undoGenerateWaypoints refreshes the markers too, so removing a survey that held an endpoint moves the highlight onto its new owner. Resolving the live cursor against the map moves to `mapPointerPositionFromClient` in `libs/map/utils-map`, shared with the pre-existing refreshLiveMeasureOnMapMove.

ArturoManzoli · 2026-08-25T13:15:57Z

Done:

(8.1) Split the screen-placement extraction out of the free-placement commit into mission-planning: move floating-panel placement maths to libs/map, so the survey-confirm strip's repositioning is reviewable and revertable on its own instead of riding inside the feature. That commit takes 104 lines out of the view and adds 23, and the two commits together reproduce the old tree exactly.
(8.2) Reworded da069a1 so the endpoint style reads as a ring throughout rather than describing the orange fill it replaced, and dropped the trailing correction paragraph. Refreshed the PR body's diff figure to +2,041 −153, and while in there named the "Save mission to library" submenu entry the change map has been flagging as unmentioned since round 9.

Not addressed:

(6.1) All three blocks this named are out of the view now. What is left is per-view wiring with nowhere to go: two composable instantiations whose option objects close over view-local functions, three logging wrappers, and the context-menu handlers, which touch currentCursorGeoCoordinates, segmentSurveyInsertIndex, pendingSimplePathInsertIndex, cursorLivePositionX/Y and planningMap plus four view functions, so a wrapper around them adds lines rather than removing them.
(6.7) Both routes out of the third action cost more than the button saves. Body content means widening useInteractionDialog for a single call site, and anchoring the preview at the mission's own coordinates routes every load through placement, which collapses the wasRepositioned distinction that finalizeMissionPlacement uses to replay the saved centre and zoom into an empty planner.

ArturoManzoli · 2026-08-25T13:16:02Z

/review

github-actions · 2026-08-25T13:31:13Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 12)

3 open — 1 major and 2 minor — and 19 closed.

The pull request lets a saved mission be dropped onto the planning map and dragged, resized and turned before it is committed, adds two more ways to reach the mission library (a submenu on the map right-click menu and an entry on the between-waypoints radial menu), makes the right-click "add" actions attach the new item to whichever end of the existing mission the click was nearest, and rings the mission's first and last waypoints in orange so those ends are visible. Nothing in the code changed since the last round: the author rewrote the branch's history instead, splitting the shared positioning-maths extraction into a commit of its own and rewording the last commit, and refreshed the pull-request description.

What still needs attention

#	Problem	What it means	Severity	Status
6.1	More bulk added to an already very large file	The main mission-planning file keeps growing, which makes every future change to mission planning slower and riskier to review.	major	💬
6.7	Three buttons in the load dialog	The dialog that asks where to put a saved mission still offers three choices where the house pattern is a dismiss and one action, so the decision is heavier than it needs to be.	minor	💬
8.1	One commit still too big to review as a unit	The main feature still arrives as a single step of well over a thousand lines, so a reviewer cannot check most of it in isolation.	minor	:large_yellow_circle:

Since round 11 — 1 closed, comparing da069a1 → 608ceb3

The increment is unusable, and this time because the branch was rewritten. incremental.diff is present and non-empty, but it replays the whole pull request — eight files whose per-file counts are identical to pr.json's, everything except src/libs/mission/library.ts. That is what a three-dot compare produces when PREV_SHA is no longer an ancestor of the head: three of round 11's four shas (8bbcb44, 4ee641f, da069a1) are gone from pr.json's commit list, so da069a1...608ceb3 bottoms out at the one commit the two histories share, e3792c6 — which is also why library.ts, the file that commit owns, is the one file missing from the increment. Every status below was judged against pr.diff and the base checkout, not against the increment.

What actually changed is the history, not the tree. All nine per-file counts in pr.json are identical to round 11's — ContextMenu.vue +79/−4, MissionPlacementToolbar.vue +183, useMissionInsertion.ts +192, useMissionPlacement.ts +836, screen-placement.ts +144, utils-map.ts +28, library.ts +201, planning-endpoints.ts +16, MissionPlanningView.vue +362/−149, totalling +2,041/−153 — and the code round 11 cited is still where it was (the .endpoint-marker ring rule, border: 2px solid #ff9800 with transform: scale(1.25) and no !important, is unchanged). What moved: four commits became five, with the screen-placement extraction lifted into 11ffa4b; the endpoint commit was reworded and is now 608ceb3; and the pull-request body gained the screen-placement bullet, named the "Save mission to library" submenu entry, and refreshed its diff figure.

previous-ledger.json was readable and carried all twenty-two entries, so nothing had to be rebuilt from prose.

resolutions.json is []: no maintainer has resolved anything on this pull request, nothing was applied, and no id went unmatched.

decisions.json holds three entries, all with gated true, so all three were checked against the ledger, and all three are pending with no +1 and no -1 — nobody entitled to decide has reacted, so none of them changed anything in the ledger this round. The vote on 6.1 (decision comment) is still open. The vote on 6.7 (decision comment) is still open, and its question has changed this round — the author now argues a second route as well, so the argument in the ledger was rewritten and that comment's tally is superseded by the one this round posts. The vote on 8.1 (decision comment) no longer asks a live question: the author acted on the finding instead of arguing it, so the argument is dropped from the ledger and 8.1 is carried as plainly open. That comment stays on the thread inviting a reaction that would now decide nothing.

Findings whose status changed this round:

#	Finding	Status
8.2	Commit body described the superseded orange-fill style; stale diff figure in the PR body	✅ Addressed — both bullets, see below
8.1	Oversized free-placement commit	:large_yellow_circle: Partially addressed — the extraction is out, the commit is still ~1,200 lines and still carries two other things; reprinted in full in section 8
6.7	Three footer actions in the load dialog	💬 Still disputed, on a new argument — the second route is now answered too; reprinted in full in section 6

On 8.2, both halves landed. 608ceb3's body now reads "an orange #ff9800 ring, scaled up to 1.25x around the center" where da069a1 said "orange fill via #ff9800", so the description matches MissionPlanningView.vue:5084–:5088. The sentence that follows — the highlight rides on the ring rather than the fill so a survey entry/exit keeps its green — is no longer a correction of an earlier paragraph but the reason for the choice, which is what a commit body should carry. The pull-request body's figure is now "+2,041 −153", matching pr.json exactly, and the change map's long-standing gap is closed: the submenu bullet now names both "Add mission from library" and "Save mission to library".

On 8.1, the concession landed and it is the right one, but two thirds of the finding is untouched. 11ffa4b ("mission-planning: move floating-panel placement maths to libs/map") now carries the extraction alone, and its message states behaviour is unchanged and that the strip keeps its two hand-tuned offsets. As far as these inputs allow, that checks out: the two view hunks that delete the inline positioning maths (@@ -1671,76 +1715,20 @@ and @@ -1748,42 +1736,18 @@) remove about 104 lines and add about 23, which is the author's figure. Per-commit trees are not in the inputs — only the commit list and the aggregate diff — so "the two commits together reproduce the old tree exactly" is confirmed at the level that is checkable here, namely that the head tree is byte-for-byte what round 11 reviewed. What is not addressed is the size of the remaining feature commit and the two other things still riding in it; see section 8.

On 6.7, the author's second claim is verified, and it is why the argument was rewritten rather than carried. Round 11 named a route that needs no dialog change — anchor the placement preview at the mission's own coordinates so "confirm without moving it" becomes the original-location outcome — and asked for the dispute to be decided knowing both routes exist. The author's answer is that this collapses the repositioned-or-not distinction. Checked: finalizeMissionPlacement (useMissionInsertion.ts:167) has three branches (:170 segment insert, :177 append, :182 fresh load), and only the third consults the flag, passing preserveMapView: placement?.wasRepositioned === true into loadDraftMission, whose guard at MissionPlanningView.vue:4168 is what decides whether a saved mission's centre and zoom are restored. Only the confirm path sets it (:3129); the dialog's original-location action calls finalizeMissionPlacement(mission) with no options (:4337). So routing every load through placement would make the flag always true and the saved view would never be replayed — the behaviour finding 1.9 was closed on. The route is not thereby impossible (the camera could be moved to the mission up front, which is the same replay done earlier), but it is a larger behaviour change than round 11 presented it as, and that is a fair cost to weigh. Still an argument about how to fix it rather than a fix, so the finding stays open and the vote is what settles it.

Discussion since the last review. Two comments, both from @ArturoManzoli: the "Done / Not addressed" summary handled above, and the bare /review that triggered this run. Every claim in the first was checked against the code or the commit list; all of them hold. Two notes. The 6.1 paragraph makes the same case as the argument already in the ledger, so that argument was carried forward verbatim and no new vote was opened on it — of the items it lists, the two composable option objects (:3106–:3110 and :3128–:3132) do close over view-local functions, and the three logging wrappers (:3138, :3144, :3149) are three lines each. And the 8.1 paragraph is filed under "Done", so the dispute on it is treated as withdrawn.

Change map — what was established before judging

Claims (from the PR body, each checked against the code):

Claim	Verdict
"Reposition on map" option on the library load dialog, dropping the mission at 1:1 scale	Verified — `MissionPlanningView.vue:4340`–`:4348` adds the action; `startFreePlacement` (`useMissionPlacement.ts:696`) sets scale 100/100 and rotation 0
Drag, scale and rotate before merging	Verified — `useMissionPlacement.ts:292` (drag), `:325` (scale handles), `:398` (rotation handle)
"Insert mission from library here" on the segment radial menu	Verified — third entry in `segmentRadialMenuItems` (`MissionPlanningView.vue:1557`), routed at `:1993`
"Mission library" submenu holding "Add mission from library" and "Save mission to library"	Verified, and the body now says so — the hover submenu is `ContextMenu.vue:173`–`:224`, with `canSaveCurrent` gating the save entry and `missionLibraryOpenSaveOnMount` (`MissionPlanningView.vue:3092`) carrying the intent to the modal. This is the discrepancy rounds 9 to 11 recorded; it is closed
Context-menu adds biased toward the closer endpoint	Verified — `endpointSplicePosition` (`src/libs/mission/planning-endpoints.ts:11`) returns `0` or `null`, consumed by all four add paths
First and last waypoints highlighted with an orange ring	Verified — `.endpoint-marker` / `border: 2px solid #ff9800` at `MissionPlanningView.vue:5084`, threaded through `createWaypointMarkerHtml`'s `isEndpoint` flag (`:3691`)
Screen-placement maths moved to `libs/map/screen-placement`, shared with the survey-confirm strip	Verified — `screenBounds` (`:41`), `pickBestPosition` (`:67`) and `positionPanelNearBounds` (`:117`) are consumed both by `useMissionPlacement.updateToolbarPosition` (`:661`) and by the view's own `updateConfirmButtonPosition`
"Diff: +2,041 −153"	Verified — `pr.json` reports exactly that across 9 files. Stale at round 11; corrected this round

No failure-site bullet: the PR fixes no bug.

Entry points (walked outward to a DOM/leaflet handler, watcher or lifecycle hook; no changed function came back never):

Function	Reached from	Frequency
`ContextMenu.handleAddHereClick` (`:518`)	click on the first context-menu entry	per user action
`ContextMenu.handleAddMissionFromLibrary` / `handleSaveMissionToLibrary` (`:611`, `:616`)	click on a submenu entry	per user action
`MissionPlacementToolbar` `scaleXModel` / `scaleYModel` / `rotationModel` (`:171`, `:175`, `:179`)	`v-model.number` on the three number inputs	per user action
`useMissionInsertion.finalizeMissionPlacement` (`:167`)	placement confirm (`MissionPlanningView.vue:3129`), and the dialog's "At its original location" (`:4337`)	per user action
`useMissionInsertion.cloneMissionForPlanning` / `appendMissionToPlanning` / `insertMissionIntoSegment` (`:75`, `:113`, `:136`)	the above	per user action
`useMissionPlacement.startFreePlacement` (`:696`)	dialog's "Reposition on map" (`MissionPlanningView.vue:4346`)	per user action
`useMissionPlacement` mouse-down/up handlers (`:292`, `:315`, `:325`, `:388`, `:398`, `:431`)	leaflet `mousedown`/`mouseup` on the preview polygon and handles	per user action
`useMissionPlacement.onPlacementMouseMove` / `onScaleHandleMouseMove` / `onRotationHandleMouseMove` (`:302`, `:339`, `:415`)	leaflet `mousemove` while a drag is live	per frame or pointer event
`useMissionPlacement.schedulePlacementPreviewRebuild` → `rebuildPlacementPreview` → `applyPreviewGeometry` (`:462`, `:687`, `:647`)	the three move handlers, the `watch` on the transform refs (`:786`), map `zoomend` (`:790`)	per frame or pointer event
`useMissionPlacement.createPreviewLayers` (`:534`)	`startFreePlacement` only	one-shot per placement
`useMissionPlacement.getPlacementBounds` / `updateToolbarPosition` (`:280`, `:661`)	the rebuild, and map `drag zoom move` via `onMapMove` (`:794`)	per frame or pointer event
`useMissionPlacement.cancelFreePlacement` / `confirmFreePlacement` (`:721`, `:754`)	toolbar buttons, Escape, and `onScopeDispose` (`:816`)	per user action
`library.transformPlacementCoord` / `transformLocalMeters` / `projectCoordToOriginalLocal` / `unrotateToOriginalLocal` (`:326`, `:279`, `:311`, `:346`)	the rebuild and the scale-move handler, once per mission coordinate	per frame or pointer event
`library.computeOriginalLocalBounds` / `safeScalePercent` / `safeRotationRad` (`:359`, `:259`, `:266`)	placement start and confirm; `safeRotationRad` also per rebuild	one-shot / per frame
`screen-placement.positionPanelNearBounds` → `screenBounds` / `pickBestPosition` (`:117`, `:41`, `:67`)	`updateToolbarPosition`, and the pre-existing survey-confirm strip	per frame or pointer event
`utils-map.mapPointerPositionFromClient` (`:60`)	`refreshLiveMeasureOnMapMove` (`MissionPlanningView.vue:1473`) on map `drag`/`zoom`, and `addSimplePathFromContextMenu` (`:2118`)	per frame or pointer event
`MissionPlanningView.renderMeasureOverlay` (`:1406`)	`handleMapMouseMove` (`:1482`) on map `mousemove`, plus one `nextTick` call from `addSimplePathFromContextMenu`	per frame or pointer event
`MissionPlanningView.currentMeasureAnchor` (`:1316`)	`renderMeasureOverlay`	per frame or pointer event
`MissionPlanningView.openSegmentRadialMenuFromContextMenu` (`:1961`)	`open-segment-radial-menu` emit from the context menu	per user action
`MissionPlanningView` context-menu add helpers (`:2096`, `:2106`, `:2110`, `:2127`, `:2134`)	context-menu emits	per user action
`MissionPlanningView.showContextMenu` nearest-segment block (`:2281`)	map `contextmenu`	per user action
`MissionPlanningView.handleKeyDown` placement branch (`:2734`–`:2739`)	window `keydown`	per user action
`MissionPlanningView.cancelFreePlacement` / `onConfirmPlacement` / `onResetPlacement` (`:3138`, `:3144`, `:3149`)	placement-toolbar buttons, and Escape for the first	per user action
`MissionPlanningView.openMissionLibrary` (`:4236`)	toolbar button, context-menu submenu, radial-menu entry	per user action
`MissionPlanningView.openMissionLibraryWithSaveDialog` (`:3098`)	context-menu submenu save entry	per user action
`MissionPlanningView.handleLoadMissionFromLibrary` (`:4290`)	`load-mission` emit from the library modal	per user action
`MissionPlanningView.loadDraftMission` (`:4158`)	`finalizeMissionPlacement` only	per user action
`MissionPlanningView.addWaypoint` (`:2952`)	`addWaypointFromClick`, `addWaypointFromContextMenu`, the simple-path branch of `onMapClick` (`:4454`)	per user action
`MissionPlanningView.createWaypointMarkerHtml` / `isEndpointWaypoint` (`:3691`, `:3714`)	`updateWaypointMarkers`, `addWaypointMarker`, `addWaypoint`, `applySelectedWaypointMarkerVisual`, `onMapClick`	per user action, once per waypoint
`MissionPlanningView.undoGenerateWaypoints` (base `:3820`)	survey undo button	per user action
`MissionPlanningView.toggleSimplePath` / `toggleSurvey` / `onMapClick`	toolbar, map click	per user action

Invariants.

A. A waypoint carries .endpoint-marker exactly when it is first or last in missionStore.currentPlanningWaypoints. Holds, re-checked: all six createWaypointMarkerHtml call sites pass the flag, and every mutation site that can change which waypoint is first or last refreshes the markers afterwards — addWaypointFromContextMenu (:2103), undoGenerateWaypoints (:3991) and the simple-path branch of onMapClick (:4463). No site is left uncovered.

B. pendingSimplePathInsertIndex is null whenever isCreatingSimplePath is false. Holds. The four sites that clear isCreatingSimplePath all clear it too: toggleSimplePath (:2393), toggleSurvey (:2403), handleKeyDown (:2748) and the main-menu step watcher (:4921).

C. endpointSplicePosition's 0 means "at the very start" in each consumer's own index convention. Three conventions, all three correct: addWaypointFromContextMenu (:2096) and the simple-path branch (:4453) pass it as an array index to addWaypoint's insertIndex; addSurveyFromContextMenu (:2127) writes it to segmentSurveyInsertIndex, consumed by generateWaypointsFromSurvey as an array splice index; addMissionFromLibraryContextMenu (:2134) maps 0 to segment -1, which insertMissionIntoSegment (useMissionInsertion.ts:136) splices at segmentIndex + 1 = 0.

D. Placement state is torn down on every exit. Confirm, cancel, Escape and scope-dispose all funnel through cancelFreePlacement in the composable (:721), which clears the layers, cancels the pending animation frame and unhooks the map listeners; onScopeDispose (:816) also removes the zoomend/drag zoom move hooks. The view wraps it (:3138) to drop placementInsertSegmentIndex as well.

E. The segment-insert intent never survives into an unrelated load. Holds through two refs: openMissionLibrary (:4236) assigns pendingSegmentInsertIndex from its own options on every open, so a plain toolbar open clears whatever a dismissed dialog left behind, and handleLoadMissionFromLibrary (:4309–:4310) moves it into the placement-scoped placementInsertSegmentIndex and nulls the pending one. finalizeMissionPlacement (useMissionInsertion.ts:168) reads and immediately nulls it, so no path consumes it twice.

6. UI / UX — 1 finding

6.7 (minor, carried from round 9, disputed on a new argument, reprinted in full) — the load dialog still has three footer actions.

MissionPlanningView.vue:4324–:4349 builds the actions array as Cancel / "At its original location" / "Reposition on map". The styling half of this finding is fixed and is not in question: color: 'white' is gone from all three, and the committing action carries the shell's fill class (class: 'bg-[#FFFFFF33]', :4342), which is the form the house direction asks for on a footer passed through useInteractionDialog's actions prop — and DialogActions.class is bound at InteractionDialog.vue:66, so it renders.

What remains is the count. The house direction is two actions at most, a dismiss on the left and a single primary on the right; this is a third. It is minor for the reason the guidelines give — multi-action footers still exist in the tree and this is the form being replaced, not a breach that reaches the user.

Both routes out of it are now answered by the author, and both answers check out:

Moving the choice into the dialog body needs body content on a shared component. DialogOptions (src/composables/interactionDialog.ts:12–:51) carries only message, variant, title, actions, maxWidth, persistent and timer, and mountDialog (:119) instantiates it through createApp(InteractionDialogComponent, props), so there is no slot a caller could fill. That is widening a shared API used across the tree for one call site.
Folding "at its original location" into the placement flow — anchoring the preview at the mission's own coordinates instead of the map centre (useMissionPlacement.ts:704) so confirming without moving it is the original-location outcome — costs more than it looks. finalizeMissionPlacement's fresh-load branch (useMissionInsertion.ts:182) is the only consumer of wasRepositioned, and it is what decides whether loadDraftMission replays the saved centre and zoom (MissionPlanningView.vue:4168). Route every load through placement and that flag is always true, so the saved view is never restored — the regression finding 1.9 was closed on. The camera could instead be moved to the mission when placement starts, which is the same replay performed earlier, but that is a larger behaviour change than round 11 presented.

So the fix is real work either way, which is what the dispute is about. It stays open because an argument about cost is not a fix; the vote on its decision comment is what settles it.

7. Code Quality & Style — 1 finding

complexity-report.json is absent this round, so the complexity measurement was unavailable — whether CI had not finished, the measurement failed, or none was produced for this head is not something these inputs say. No complexity finding is raised on this round's evidence. Round 11's report is not reused: it was produced for a different head sha, and its one trigger (renderMeasureOverlay, inherited complexity) was answered without a finding then.

6.1 (major, carried from round 6, disputed, reprinted in full) — MissionPlanningView.vue takes another large net addition.

pr.json reports +362 / −149 on this file — +213 net, unchanged from last round. The base checkout has it at 5,218 lines, so it lands near 5,431. That clears both halves of the file-growth test: a file already far beyond ~2,000 lines taking well over 100 net lines.

The extraction work is real and is not in question. useMissionPlacement.ts (836), library.ts (+201), useMissionInsertion.ts (192), MissionPlacementToolbar.vue (183), screen-placement.ts (144), utils-map.ts (+28) and planning-endpoints.ts (16) put roughly 1,600 lines outside the view, and all three blocks this finding named in round 9 are gone from it — the toolbar-positioning maths is in src/libs/map/screen-placement.ts, the endpoint choice is in planning-endpoints.ts, and the client-to-map cursor conversion is mapPointerPositionFromClient (utils-map.ts:60), shared with the pre-existing refreshLiveMeasureOnMapMove (:1473).

What is left in the view is two blocks. The placement wiring, :3091–:3153 (63 lines): the two composable instantiations, whose option objects close over view-local functions (:3106–:3110 passes cloneCommands, addWaypointMarker, updateWaypointMarkers and loadDraftMission; :3128–:3132 passes finalizeMissionPlacement, the ignoreNextClick assignment and the toolbar footprint), plus three logging wrappers of three lines each (:3138, :3144, :3149). And the context-menu routing block, :2096–:2140 (~45 lines): addWaypointFromContextMenu, getContextMenuEndpointSplicePosition, addSimplePathFromContextMenu, addSurveyFromContextMenu, addMissionFromLibraryContextMenu. The author's objection to a composable around the second is accepted on its own terms and has been since round 10: those five touch currentCursorGeoCoordinates, segmentSurveyInsertIndex, pendingSimplePathInsertIndex, cursorLivePositionX/Y, planningMap and four view functions, and a wrapper taking all of them as options would cost more lines than it saves. There is no longer a specific block this finding can point at.

It stays open because the measurement it is made of has not changed: +213 onto a 5,431-line file is the PR piling bulk onto a file already well past the threshold, and the guidelines are explicit that a finding is not downgraded because it shrank. Whether +213 of genuinely per-view wiring is an acceptable price for this feature is a judgement about this repository's direction rather than about the code, which is what the dispute is for.

8. Commit Hygiene — 1 finding

8.1 (minor, carried from round 9, partially addressed, dispute withdrawn, reprinted in full) — one commit is still too large to review as a unit.

The five commits are well-scoped in subject, follow the scope: subject style the repository's own log uses (video:, composables:, fix:), and none carries an issue or pull-request reference. Everything this finding raised about the commit bodies stays fixed.

What landed this round. 11ffa4b ("mission-planning: move floating-panel placement maths to libs/map") now carries the screen-placement.ts extraction on its own, ahead of the feature. That was the strongest of the three sub-points: it is a pure move of code that already existed in the view and it changes the survey-confirm strip, a surface this feature does not otherwise touch, so it is exactly the part a reviewer wants isolated and revertable. The two view hunks it accounts for (@@ -1671,76 +1715,20 @@ and @@ -1748,42 +1736,18 @@) remove about 104 lines of inline positioning maths and add about 23, matching the figure the author gives.

What has not. The feature commit is now 1476b3d ("mission-planning: add free-placement workflow for library missions"), and it still adds useMissionPlacement.ts (836), MissionPlacementToolbar.vue (183) and useMissionInsertion.ts (192) plus the view rewiring on top — around 1,200 new lines in one step, still several hundred past the point the guidelines flag on size alone. Two of the three things riding inside it are untouched, and both are named in its own message:

The segment-insert routing — insertMissionIntoSegment (useMissionInsertion.ts:136), the third radial-menu entry (MissionPlanningView.vue:1557) and its handler (:1993) — is a second user-facing feature, described in its own paragraph of the commit message. It has its own snackbar, its own index convention and its own failure mode; a reviewer should be able to read it, and revert it, without the placement state machine.
The "at its original location" reroute through finalizeMissionPlacement (:4337, replacing the previous direct loadDraftMission call) is a modification of existing behaviour folded into a feature commit. Finding 1.9 closed on the fix, but the change itself still cannot be reverted or backported without dragging the whole feature along, which is what the guideline about behaviour changes riding alone exists to prevent.

Two more splits of the kind that landed this round close it: lift the segment-insert routing and the original-location reroute out of 1476b3d the way the screen-placement extraction was lifted out, leaving the placement composable, its toolbar and their wiring as the feature commit.

Sections with nothing to report (8)

1. Correctness & Implementation Bugs — ✅ (re-read the five added files in full and every hunk of the two modified components; invariants A–E above all hold — the four sites that clear isCreatingSimplePath each clear pendingSimplePathInsertIndex (:2393, :2403, :2748, :4921), all six createWaypointMarkerHtml call sites pass the endpoint flag, and finalizeMissionPlacement's three branches (useMissionInsertion.ts:170, :177, :182) are mutually exclusive with the intent ref nulled before any of them runs)

2. Persistence & User Data — ✅ (no persisted key is added, reshaped or removed and package.json is untouched; currentPlanningWaypoints/currentPlanningSurveys are plain reactive arrays in src/stores/mission.ts, and the only persisted values in reach remain cockpit-user-last-map-center / cockpit-user-last-map-zoom through the mapCenter/zoom refs, whose write loadDraftMission skips only on the repositioned path (:4168))

3. AGENTS.md Adherence — ✅ (no dependency added or reordered; nothing exported without a call site — PLACEMENT_TOOLBAR_FOOTPRINT (MissionPlacementToolbar.vue:121) is consumed at MissionPlanningView.vue:3132, and every export of screen-placement.ts, planning-endpoints.ts and utils-map.ts has a caller in this PR; the added JSDoc blocks all carry non-empty typed @param/@returns, including the TSPropertySignature members .eslintrc.cjs:39 requires; the history rewrite moved no code, so scope discipline is where round 11 left it)

4. Security — ✅ (re-ran all nine sub-checks over the whole of pr.diff: no new dependency, no network call, no eval/Function/v-html, no secret or env-var use, no build/CI/electron change; the L.divIcon template literals in useMissionPlacement.ts:1140 and :1165 interpolate only module-local constants; no hidden Unicode or encoded blob; and nothing in pr.diff, pr.json's body, new-comments.json, resolutions.json or decisions.json reads as an instruction addressed to a reviewer)

5. Performance — ✅ (the per-frame path is unchanged from round 11 and still bounded — schedulePlacementPreviewRebuild (:462) coalesces to one animation frame, applyPreviewGeometry (:647) reassigns geometry on layers created once, and getPlacementBounds (:280) reuses the bounding polygon's own bounds instead of re-projecting every coordinate; invariant D confirms every listener, the pending frame and both map hooks have a matching teardown)

9. Tests — ✅ (no test file is touched and nothing existing was removed or weakened; the three pure modules this PR adds under src/libs/ are the testable half of the feature)

10. Documentation — ✅ (nothing in the diff differs between Lite and Standalone — no electronAPI, electron-* import, filesystem or process use anywhere in it — so the AGENTS.md README rule is not triggered; the docs-needed label is already on the PR, and the body's change map was corrected this round)

11. Nitpicks / Optional — ✅ (checked the one comment the diff deletes, // NEW: hide/show the live pill … in the measure-overlay hunk @@ -1389,13 +1423,12 @@, against the AGENTS.md comment-immutability rule — the line it documented is itself changed in the same diff, so the rule does not bite; and checked the placement toolbar's z-[650] and its hand-tuned mt-[46px]/ml-[10px] offsets against in-tree precedent, which is the survey-confirm strip it sits beside (MissionPlanningView.vue:24) and the map overlays around it at 600, 601 and 670)

Generated by Claude. This is advisory; a human reviewer must still approve.

github-actions · 2026-08-25T13:31:14Z

🙋 Decision needed — 6.7

Load-mission dialog still has three footer actions

The author's argument: Both ways out of the third footer action cost more than the button saves: moving the choice into the dialog body means widening the shared useInteractionDialog API, which today accepts only a message and an array of actions, for a single call site, and anchoring the placement preview at the mission's own coordinates would route every library load through placement, which collapses the repositioned-or-not distinction the merge routing uses to decide whether a saved mission's centre and zoom are restored into an empty planner.

How to vote on this dispute

React to this comment and the next /review applies the answer:

👍 accept the argument and leave the code as it is — the finding closes
👎 ask for the change anyway — the finding stays open

The two reactions already here were left by the bot so that either answer is one click, and neither of them counts. Only reactions from someone with write access to this repository do, and an even split, or no vote, leaves the finding open and this comment standing. Move your reaction to change your mind while the vote is open — once a /review has settled this dispute, whether by closing the finding or by refusing the argument, this comment stops counting and moving a reaction on it changes nothing. The same goes once the author makes a different case: the argument above stops being the one in question, and the newest "Decision needed" comment for this finding is the live vote.

ArturoManzoli · 2026-08-25T15:05:34Z

Not addressed:

(8.1) Leaving the segment-insert routing and the original-location reroute inside 1476b3d. The routing is the same user-facing capability as the placement it rides with, so lifting it out spreads one feature across three commits rather than making it readable. The reroute can only land after the commit that introduces finalizeMissionPlacement, so isolating it means inventing an intermediate handleLoadMissionFromLibrary whose merge-aware title and message contradict the path they describe, which is a worse commit to read than the one it would replace.
(6.1) Standing on the recorded argument. The three blocks this named are out of the view now and what is left is per-view wiring, so there is no longer a target a code change could act on.
(6.7) Standing on the recorded argument, which round 12 confirms on both routes: body content means widening useInteractionDialog for one call site, and folding the choice into the placement flow drops the saved centre and zoom on a fresh load.

ArturoManzoli · 2026-08-25T15:47:57Z

/review

github-actions · 2026-08-25T15:55:18Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 13)

3 open — 1 major and 2 minor — and 19 closed.

The pull request lets a saved mission be dropped onto the planning map and dragged, resized and turned before it is committed, adds two more ways to reach the mission library (a submenu on the map right-click menu and an entry on the between-waypoints radial menu), makes the right-click "add" actions attach the new item to whichever end of the existing mission the click was nearest, and rings the mission's first and last waypoints in orange so those ends are visible. No code changed since the last round — the head is the same commit round 12 reviewed. What arrived instead is discussion: the author has taken up the one finding that was not being argued, so all three open findings are now disputes waiting on a maintainer.

What still needs attention

#	Problem	What it means	Severity	Status
6.1	More bulk added to an already very large file	The main mission-planning file keeps growing, which makes every future change to mission planning slower and riskier to review.	major	💬
6.7	Three buttons in the load dialog	The dialog that asks where to put a saved mission still offers three choices where the house pattern is a dismiss and one action, so the decision is heavier than it needs to be.	minor	💬
8.1	One commit still too big to review as a unit	The main feature still arrives as a single step of well over a thousand lines, so a reviewer cannot check most of it in isolation.	minor	💬

Since round 12 — 0 closed, 1 newly disputed, at the same head 608ceb3

Nothing was pushed, and this round was still run rather than skipped. HEAD_SHA and PREV_SHA are the same commit, 608ceb3cc267; incremental.diff is empty, which for an unchanged head is what it should be rather than a sign of breakage. pr.json confirms it independently: the same five commits in the same order (e3792c6, 11ffa4b, 1476b3d, 7c1e1fc, 608ceb3), the same nine files, and the same per-file counts as round 12 — ContextMenu.vue +79/−4, MissionPlacementToolbar.vue +183, useMissionInsertion.ts +192, useMissionPlacement.ts +836, screen-placement.ts +144, utils-map.ts +28, library.ts +201, planning-endpoints.ts +16, MissionPlanningView.vue +362/−149, totalling +2,041/−153. The tree under review is byte-for-byte round 12's.

An unchanged head with nothing to apply is normally reported and stopped there. This round has something to apply: the author has disputed 8.1, which round 12 carried as plainly open because the dispute on it had been withdrawn. That moves a finding's status and opens a vote, so it is work a short-circuit would have thrown away — the comments that arrived since round 12 are only ever delivered once. Every status below was judged against pr.diff and the base checkout, since there is no increment to judge against.

previous-ledger.json was readable and carried all twenty-two entries, so nothing had to be rebuilt from prose.

resolutions.json is []: no maintainer has resolved anything on this pull request, nothing was applied, and no id went unmatched.

decisions.json holds two entries, both with gated true, so both were checked against the ledger, and both are pending with no +1 and no -1 — nobody entitled to decide has reacted, so neither changed anything. The vote on 6.1 (decision comment) is still open. The vote on 6.7 (decision comment) is still open. Round 12's third entry, on 8.1, is gone from the file because the argument it answered had been dropped from the ledger; the dispute the author raises this round is a new one and gets a decision comment of its own.

Findings whose status changed this round:

#	Finding	Status
8.1	Oversized free-placement commit	💬 Now disputed — the author argues both splits rather than making them; reprinted in full in section 8

On 8.1, the dispute is live again and the finding is unchanged underneath it. Round 12 recorded that the author had filed 8.1 under "Done" and treated the dispute as withdrawn; this round's comment files the same id under "Not addressed" and argues it, so the argument goes back into the ledger and a fresh vote opens. The code is untouched, so what round 12 judged still holds exactly: 11ffa4b carries the screen-placement extraction alone and that concession stands, while 1476b3d still adds useMissionPlacement.ts (836), MissionPlacementToolbar.vue (183) and useMissionInsertion.ts (192) plus the view rewiring in one step. The finding stays 🟡 on the code and 💬 on the record — a dispute leaves a finding open, and this one is carried into every later round until the commits are split or a maintainer settles it.

Both halves of the author's 8.1 argument were checked, and they do not land equally. The first — that the segment-insert routing is the same user-facing capability as the placement it rides with — is fair as far as it goes: insertMissionIntoSegment (useMissionInsertion.ts:136) is only ever reached through finalizeMissionPlacement (:167), which the same commit introduces, so the routing genuinely cannot be lifted after the feature commit. The second — that isolating the original-location reroute would need an intermediate commit whose merge-aware dialog title and message describe a path that does not yet exist — does not survive the code. The merge-aware labels at MissionPlanningView.vue:4313–:4316 are driven by isInserting and canSaveCurrentMissionToLibrary, neither of which has anything to do with free placement, and the two outcomes they name (append when the planner has work, fresh load when it is empty) are both implemented inside finalizeMissionPlacement independently of it. A commit that adds useMissionInsertion, reroutes the dialog's existing action through it, relabels the dialog and adds the radial-menu entry is self-consistent and reviewable on its own, with "Reposition on map" and the placement composable arriving after it. So the split the finding asks for runs the other way round from the one the argument rebuts, which the maintainers deciding this should know. Either way it is an argument about how to split rather than a split, so the finding stays open and the vote settles it.

On 6.1 and 6.7, the author stands on the arguments already recorded, and both were re-verified against the unchanged code. For 6.1: pr.json still reports +362/−149 on the view, the base checkout still has it at 5,218 lines, and the two blocks the finding can still point at are where round 12 left them — the placement wiring at :3091–:3153 and the context-menu routing at :2096–:2140. For 6.7: MissionPlanningView.vue:4319–:4349 still builds three footer actions, color: 'white' is still gone from all three, and the committing action still carries class: 'bg-[#FFFFFF33]'. Both arguments are carried forward verbatim; neither was rewritten, so neither of their open votes is superseded.

Discussion since the last review. Two comments, both from @ArturoManzoli: a "Done / Not addressed" summary, and the bare /review that triggered this run, which is a command and carries no claim. The summary lists 8.1, 6.1 and 6.7 under "Not addressed" and adds nothing under "Done" — consistent with a round in which nothing was pushed. Its 6.1 and 6.7 paragraphs restate the arguments already in the ledger and were carried unchanged; its 8.1 paragraph is the new dispute handled above. Every factual claim in it was checked against pr.diff, pr.json and the base checkout rather than taken as given.

Change map — what was established before judging

Claims (from the PR body, each re-checked against the code this round):

Claim	Verdict
"Reposition on map" option on the library load dialog, dropping the mission at 1:1 scale	Verified — `MissionPlanningView.vue:4341`–`:4349` adds the action; `startFreePlacement` (`useMissionPlacement.ts:696`) sets scale 100/100 and rotation 0
Drag, scale and rotate before merging	Verified — `useMissionPlacement.ts:292` (drag), `:325` (scale handles), `:398` (rotation handle)
"Insert mission from library here" on the segment radial menu	Verified — third entry in `segmentRadialMenuItems` (`MissionPlanningView.vue:1557`), routed at `:1993`
"Mission library" submenu holding "Add mission from library" and "Save mission to library"	Verified — the hover submenu is `ContextMenu.vue:173`–`:224`, with `canSaveCurrent` gating the save entry and `missionLibraryOpenSaveOnMount` (`MissionPlanningView.vue:3092`) carrying the intent to the modal
Context-menu adds biased toward the closer endpoint	Verified — `endpointSplicePosition` (`src/libs/mission/planning-endpoints.ts:11`) returns `0` or `null`, consumed by all four add paths
First and last waypoints highlighted with an orange ring	Verified — `.endpoint-marker` / `border: 2px solid #ff9800` at `MissionPlanningView.vue:5084`, threaded through `createWaypointMarkerHtml`'s `isEndpoint` flag (`:3691`)
Screen-placement maths moved to `libs/map/screen-placement`, shared with the survey-confirm strip	Verified — `screenBounds` (`:41`), `pickBestPosition` (`:67`) and `positionPanelNearBounds` (`:117`) are consumed both by `useMissionPlacement.updateToolbarPosition` (`:661`) and by the view's own `updateConfirmButtonPosition` (`:1736`)
"Diff: +2,041 −153"	Verified — `pr.json` reports exactly that across 9 files

No failure-site bullet: the PR fixes no bug.

Entry points (walked outward to a DOM/leaflet handler, watcher or lifecycle hook; no changed function came back never):

Function	Reached from	Frequency
`ContextMenu.handleAddHereClick` (`:518`)	click on the first context-menu entry	per user action
`ContextMenu.handleAddMissionFromLibrary` / `handleSaveMissionToLibrary` (`:611`, `:616`)	click on a submenu entry	per user action
`MissionPlacementToolbar` `scaleXModel` / `scaleYModel` / `rotationModel` (`:171`, `:175`, `:179`)	`v-model.number` on the three number inputs	per user action
`useMissionInsertion.finalizeMissionPlacement` (`:167`)	placement confirm (`MissionPlanningView.vue:3129`), and the dialog's "At its original location" (`:4338`)	per user action
`useMissionInsertion.cloneMissionForPlanning` / `appendMissionToPlanning` / `insertMissionIntoSegment` (`:75`, `:113`, `:136`)	the above	per user action
`useMissionPlacement.startFreePlacement` (`:696`)	dialog's "Reposition on map" (`MissionPlanningView.vue:4347`)	per user action
`useMissionPlacement` mouse-down/up handlers (`:292`, `:315`, `:325`, `:388`, `:398`, `:431`)	leaflet `mousedown`/`mouseup` on the preview polygon and handles	per user action
`useMissionPlacement.onPlacementMouseMove` / `onScaleHandleMouseMove` / `onRotationHandleMouseMove` (`:302`, `:339`, `:415`)	leaflet `mousemove` while a drag is live	per frame or pointer event
`useMissionPlacement.schedulePlacementPreviewRebuild` → `rebuildPlacementPreview` → `applyPreviewGeometry` (`:462`, `:687`, `:647`)	the three move handlers, the `watch` on the transform refs (`:786`), map `zoomend` (`:790`)	per frame or pointer event
`useMissionPlacement.createPreviewLayers` (`:534`)	`startFreePlacement` only	one-shot per placement
`useMissionPlacement.getPlacementBounds` / `updateToolbarPosition` (`:280`, `:661`)	the rebuild, and map `drag zoom move` via `onMapMove` (`:794`)	per frame or pointer event
`useMissionPlacement.cancelFreePlacement` / `confirmFreePlacement` (`:721`, `:754`)	toolbar buttons, Escape, and `onScopeDispose` (`:816`)	per user action
`library.transformPlacementCoord` / `transformLocalMeters` / `projectCoordToOriginalLocal` / `unrotateToOriginalLocal` (`:326`, `:279`, `:311`, `:346`)	the rebuild and the scale-move handler, once per mission coordinate	per frame or pointer event
`library.computeOriginalLocalBounds` / `safeScalePercent` / `safeRotationRad` (`:359`, `:259`, `:266`)	placement start and confirm; `safeRotationRad` also per rebuild	one-shot / per frame
`screen-placement.positionPanelNearBounds` → `screenBounds` / `pickBestPosition` (`:117`, `:41`, `:67`)	`updateToolbarPosition`, and the pre-existing survey-confirm strip	per frame or pointer event
`utils-map.mapPointerPositionFromClient` (`:60`)	`refreshLiveMeasureOnMapMove` (`MissionPlanningView.vue:1473`) on map `drag`/`zoom`, and `addSimplePathFromContextMenu` (`:2118`)	per frame or pointer event
`MissionPlanningView.renderMeasureOverlay` (`:1406`)	`handleMapMouseMove` (`:1482`) on map `mousemove`, plus one `nextTick` call from `addSimplePathFromContextMenu`	per frame or pointer event
`MissionPlanningView.currentMeasureAnchor` (`:1316`)	`renderMeasureOverlay`	per frame or pointer event
`MissionPlanningView.openSegmentRadialMenuFromContextMenu` (`:1961`)	`open-segment-radial-menu` emit from the context menu	per user action
`MissionPlanningView` context-menu add helpers (`:2096`, `:2106`, `:2110`, `:2127`, `:2134`)	context-menu emits	per user action
`MissionPlanningView.showContextMenu` nearest-segment block (`:2281`)	map `contextmenu`	per user action
`MissionPlanningView.handleKeyDown` placement branch (`:2734`–`:2739`)	window `keydown`	per user action
`MissionPlanningView.cancelFreePlacement` / `onConfirmPlacement` / `onResetPlacement` (`:3138`, `:3144`, `:3149`)	placement-toolbar buttons, and Escape for the first	per user action
`MissionPlanningView.openMissionLibrary` (`:4236`)	toolbar button, context-menu submenu, radial-menu entry	per user action
`MissionPlanningView.openMissionLibraryWithSaveDialog` (`:3098`)	context-menu submenu save entry	per user action
`MissionPlanningView.handleLoadMissionFromLibrary` (`:4290`)	`load-mission` emit from the library modal	per user action
`MissionPlanningView.loadDraftMission` (`:4158`)	`finalizeMissionPlacement` only	per user action
`MissionPlanningView.addWaypoint` (`:2952`)	`addWaypointFromClick`, `addWaypointFromContextMenu`, the simple-path branch of `onMapClick` (`:4454`)	per user action
`MissionPlanningView.createWaypointMarkerHtml` / `isEndpointWaypoint` (`:3691`, `:3714`)	`updateWaypointMarkers`, `addWaypointMarker`, `addWaypoint`, `applySelectedWaypointMarkerVisual`, `onMapClick`	per user action, once per waypoint
`MissionPlanningView.undoGenerateWaypoints` (base `:3820`)	survey undo button	per user action
`MissionPlanningView.toggleSimplePath` / `toggleSurvey` / `onMapClick`	toolbar, map click	per user action

Invariants.

A. A waypoint carries .endpoint-marker exactly when it is first or last in missionStore.currentPlanningWaypoints. Holds, re-checked against every call site: all six createWaypointMarkerHtml calls pass the flag (:2957, :3696, :3728, :4054, :4108, :4376), and every mutation that can change which waypoint is first or last refreshes the markers afterwards — addWaypointFromContextMenu (:2103), undoGenerateWaypoints (:3991) and the simple-path branch of onMapClick (:4463). The insert-at-0 path is the one that matters, since addWaypoint only sets the flag on the waypoint it creates; both of its callers refresh.

B. pendingSimplePathInsertIndex is null whenever isCreatingSimplePath is false. Holds. The four sites that clear isCreatingSimplePath all clear it too: toggleSimplePath (:2393), toggleSurvey (:2403), handleKeyDown (:2748) and the main-menu step watcher (:4921).

C. endpointSplicePosition's 0 means "at the very start" in each consumer's own index convention. Three conventions, all three correct: addWaypointFromContextMenu (:2096) and the simple-path branch (:4453) pass it as an array index to addWaypoint's insertIndex; addSurveyFromContextMenu (:2127) writes it to segmentSurveyInsertIndex, consumed by generateWaypointsFromSurvey as an array splice index; addMissionFromLibraryContextMenu (:2134) maps 0 to segment -1, which insertMissionIntoSegment (useMissionInsertion.ts:136) splices at segmentIndex + 1 = 0.

D. Placement state is torn down on every exit. Confirm, cancel, Escape and scope-dispose all funnel through cancelFreePlacement in the composable (:721), which clears the layers, cancels the pending animation frame and unhooks all six map listeners; onScopeDispose (:816) also removes the zoomend/drag zoom move hooks. The view wraps it (:3138) to drop placementInsertSegmentIndex as well.

E. The segment-insert intent never survives into an unrelated load. Holds through two refs: openMissionLibrary (:4236) assigns pendingSegmentInsertIndex from its own options on every open, so a plain toolbar open clears whatever a dismissed dialog left behind, and handleLoadMissionFromLibrary (:4309–:4310) moves it into the placement-scoped placementInsertSegmentIndex and nulls the pending one. The dialog's Cancel action (:4326) clears the placement-scoped one, and finalizeMissionPlacement (useMissionInsertion.ts:168) reads and immediately nulls it, so no path consumes it twice.

6. UI / UX — 1 finding

6.7 (minor, carried from round 9, disputed, reprinted in full) — the load dialog still has three footer actions.

MissionPlanningView.vue:4319–:4350 builds the actions array as Cancel / "At its original location" / "Reposition on map". The styling half of this finding is fixed and is not in question: color: 'white' is gone from all three, and the committing action carries the shell's fill class (class: 'bg-[#FFFFFF33]', :4343), which is the form the house direction asks for on a footer passed through useInteractionDialog's actions prop — and DialogActions.class is bound at InteractionDialog.vue:66, so it renders.

What remains is the count. The house direction is two actions at most, a dismiss on the left and a single primary on the right; this is a third. It is minor for the reason the guidelines give — multi-action footers still exist in the tree and this is the form being replaced, not a breach that reaches the user.

Both routes out of it are answered by the author, and both answers were re-checked against the unchanged code and still hold:

Moving the choice into the dialog body needs body content on a shared component. DialogOptions (src/composables/interactionDialog.ts:12–:51) carries only message, variant, title, actions, maxWidth, persistent and timer, and mountDialog (:119) instantiates it through createApp(InteractionDialogComponent, props), so there is no slot a caller could fill. That is widening a shared API used across the tree for one call site.
Folding "at its original location" into the placement flow — anchoring the preview at the mission's own coordinates instead of the map centre (useMissionPlacement.ts:704) so confirming without moving it is the original-location outcome — costs more than it looks. finalizeMissionPlacement's fresh-load branch (useMissionInsertion.ts:182) is the only consumer of wasRepositioned, and it is what decides whether loadDraftMission replays the saved centre and zoom (MissionPlanningView.vue:4168). Route every load through placement and that flag is always true, so the saved view is never restored — the regression finding 1.9 was closed on. The camera could instead be moved to the mission when placement starts, which is the same replay performed earlier, but that is a larger behaviour change than it first appears.

So the fix is real work either way, which is what the dispute is about. It stays open because an argument about cost is not a fix; the vote on its decision comment is what settles it.

7. Code Quality & Style — 1 finding

complexity-report.json is absent this round, so the complexity measurement was unavailable — whether CI had not finished, the measurement failed, or none was produced for this head is not something these inputs say. No complexity finding is raised on this round's evidence, and no earlier round's report is reused, since each was produced against a report this run cannot see.

6.1 (major, carried from round 6, disputed, reprinted in full) — MissionPlanningView.vue takes another large net addition.

pr.json reports +362 / −149 on this file — +213 net, unchanged from last round. The base checkout has it at 5,218 lines, so it lands near 5,431. That clears both halves of the file-growth test: a file already far beyond ~2,000 lines taking well over 100 net lines.

The extraction work is real and is not in question. useMissionPlacement.ts (836), library.ts (+201), useMissionInsertion.ts (192), MissionPlacementToolbar.vue (183), screen-placement.ts (144), utils-map.ts (+28) and planning-endpoints.ts (16) put roughly 1,600 lines outside the view, and all three blocks this finding named in round 9 are gone from it — the toolbar-positioning maths is in src/libs/map/screen-placement.ts (the view's updateConfirmButtonPosition now calls into it at :1736 and drops about 80 lines of inline screenBounds/pickBestPosition), the endpoint choice is in planning-endpoints.ts, and the client-to-map cursor conversion is mapPointerPositionFromClient (utils-map.ts:60), shared with the pre-existing refreshLiveMeasureOnMapMove (:1473).

What is left in the view is two blocks. The placement wiring, :3091–:3153 (63 lines): the two composable instantiations, whose option objects close over view-local functions (:3106–:3110 passes cloneCommands, addWaypointMarker, updateWaypointMarkers and loadDraftMission; :3128–:3132 passes finalizeMissionPlacement, the ignoreNextClick assignment and the toolbar footprint), plus three logging wrappers of three lines each (:3138, :3144, :3149). And the context-menu routing block, :2096–:2140 (~45 lines): addWaypointFromContextMenu, getContextMenuEndpointSplicePosition, addSimplePathFromContextMenu, addSurveyFromContextMenu, addMissionFromLibraryContextMenu. The author's objection to a composable around the second is accepted on its own terms and has been since round 10: those five touch currentCursorGeoCoordinates, segmentSurveyInsertIndex, pendingSimplePathInsertIndex, cursorLivePositionX/Y, planningMap and four view functions, and a wrapper taking all of them as options would cost more lines than it saves. There is no longer a specific block this finding can point at.

It stays open because the measurement it is made of has not changed: +213 onto a 5,431-line file is the PR piling bulk onto a file already well past the threshold, and the guidelines are explicit that a finding is not downgraded because it shrank. Whether +213 of genuinely per-view wiring is an acceptable price for this feature is a judgement about this repository's direction rather than about the code, which is what the dispute is for.

8. Commit Hygiene — 1 finding

8.1 (minor, carried from round 9, partially addressed in code and now disputed, reprinted in full) — one commit is still too large to review as a unit.

The five commits are well-scoped in subject, follow the scope: subject style the repository's own log uses (video:, composables:, fix:), and none carries an issue or pull-request reference. Everything this finding raised about the commit bodies stays fixed.

What landed earlier. 11ffa4b ("mission-planning: move floating-panel placement maths to libs/map") carries the screen-placement.ts extraction on its own, ahead of the feature. That was the strongest of the three sub-points: it is a pure move of code that already existed in the view and it changes the survey-confirm strip, a surface this feature does not otherwise touch, so it is exactly the part a reviewer wants isolated and revertable. The two view hunks it accounts for (@@ -1671,76 +1715,20 @@ and @@ -1748,42 +1736,18 @@) remove about 104 lines of inline positioning maths and add about 23, matching the figure the author gives.

What has not. The feature commit 1476b3d ("mission-planning: add free-placement workflow for library missions") still adds useMissionPlacement.ts (836), MissionPlacementToolbar.vue (183) and useMissionInsertion.ts (192) plus the view rewiring on top — around 1,200 new lines in one step, still several hundred past the point the guidelines flag on size alone. Two of the three things riding inside it are untouched, and both are named in its own message:

The segment-insert routing — insertMissionIntoSegment (useMissionInsertion.ts:136), the third radial-menu entry (MissionPlanningView.vue:1557) and its handler (:1993) — is a second user-facing feature, described in its own paragraph of the commit message. It has its own snackbar, its own index convention and its own failure mode; a reviewer should be able to read it, and revert it, without the placement state machine.
The "at its original location" reroute through finalizeMissionPlacement (:4338, replacing the previous direct loadDraftMission call) is a modification of existing behaviour folded into a feature commit. Finding 1.9 closed on the fix, but the change itself still cannot be reverted or backported without dragging the whole feature along, which is what the guideline about behaviour changes riding alone exists to prevent.

The split runs the other way from the one the author's argument rebuts. The argument treats both items as things that would have to be lifted out of 1476b3d into commits that follow it, and on that reading the second is genuinely awkward. But neither has to come after the feature: useMissionInsertion.ts depends on nothing in useMissionPlacement.ts, so a commit that adds the insertion composable, reroutes the dialog's existing "keep original location" action through finalizeMissionPlacement, relabels the dialog for the merge case and adds the radial-menu entry stands entirely on its own before the placement work. Checked: the merge-aware title and message (MissionPlanningView.vue:4313–:4316) are driven by isInserting and canSaveCurrentMissionToLibrary, neither of which touches placement, and the two outcomes they name — append when the planner has work, fresh load when it is empty — are both implemented inside finalizeMissionPlacement (useMissionInsertion.ts:172–:183) with no reference to it. wasRepositioned simply defaults to absent on that path, which is the correct behaviour for a dialog that has not yet grown a reposition button. That leaves 1476b3d as the placement composable, its toolbar, the third footer action and their wiring: still large, but one thing.

The finding stays open on that basis, and the dispute is what a maintainer settles.

Sections with nothing to report (8)

1. Correctness & Implementation Bugs — ✅ (re-read the five added files in full and every hunk of the two modified components; invariants A–E above all hold — the four sites that clear isCreatingSimplePath each clear pendingSimplePathInsertIndex (:2393, :2403, :2748, :4921), all six createWaypointMarkerHtml call sites pass the endpoint flag with a marker refresh behind every insert-at-0 path, and finalizeMissionPlacement's three branches (useMissionInsertion.ts:170, :177, :182) are mutually exclusive with the intent ref nulled before any of them runs)

2. Persistence & User Data — ✅ (no persisted key is added, reshaped or removed and package.json is untouched; currentPlanningWaypoints/currentPlanningSurveys are plain reactive arrays in src/stores/mission.ts, and the only persisted values in reach remain cockpit-user-last-map-center / cockpit-user-last-map-zoom through the mapCenter/zoom refs, whose write loadDraftMission skips only on the repositioned path (:4168))

3. AGENTS.md Adherence — ✅ (no dependency added or reordered; nothing exported without a call site — PLACEMENT_TOOLBAR_FOOTPRINT (MissionPlacementToolbar.vue:121) is consumed at MissionPlanningView.vue:3132, and every export of screen-placement.ts, planning-endpoints.ts and utils-map.ts has a caller in this PR; the added JSDoc blocks all carry non-empty typed @param/@returns, including the TSPropertySignature members .eslintrc.cjs:39 requires; scope discipline is unchanged, since no code moved this round)

4. Security — ✅ (re-ran all nine sub-checks over the whole of pr.diff: no new dependency, no network call, no eval/Function/v-html, no secret or env-var use, no build/CI/electron change; the L.divIcon template literals in useMissionPlacement.ts:1140 and :1165 interpolate only module-local constants; no hidden Unicode or encoded blob; and nothing in pr.diff, pr.json's body, new-comments.json, resolutions.json or decisions.json reads as an instruction addressed to a reviewer)

5. Performance — ✅ (the per-frame path is unchanged and still bounded — schedulePlacementPreviewRebuild (:462) coalesces to one animation frame, applyPreviewGeometry (:647) reassigns geometry on layers created once, and getPlacementBounds (:280) reuses the bounding polygon's own bounds instead of re-projecting every coordinate; invariant D confirms every listener, the pending frame and both map hooks have a matching teardown)

9. Tests — ✅ (no test file is touched and nothing existing was removed or weakened; the three pure modules this PR adds under src/libs/ are the testable half of the feature)

10. Documentation — ✅ (nothing in the diff differs between Lite and Standalone — no electronAPI, electron-* import, filesystem or process use anywhere in it — so the AGENTS.md README rule is not triggered; the docs-needed label is already on the PR, and the body's change map matches the code)

11. Nitpicks / Optional — ✅ (re-checked the one comment the diff deletes, // NEW: hide/show the live pill … in the measure-overlay hunk @@ -1389,13 +1423,12 @@, against the AGENTS.md comment-immutability rule — the line it documented is itself changed in the same diff, so the rule does not bite; and re-checked the placement toolbar's z-[650] and its hand-tuned mt-[46px]/ml-[10px] offsets against in-tree precedent, which is the survey-confirm strip it sits beside (MissionPlanningView.vue:24) and the map overlays around it at 600, 601 and 670)

Generated by Claude. This is advisory; a human reviewer must still approve.

github-actions · 2026-08-25T15:55:20Z

🙋 Decision needed — 8.1

Oversized free-placement commit (1476b3d, ~1,200 lines, still carrying the segment-insert routing and the original-location reroute)

The author's argument: Both splits cost more than they save: the segment-insert routing is the same user-facing capability as the placement it rides with, so lifting it out spreads one feature across three commits instead of making it readable, and the original-location reroute can only land after the commit that introduces the merge-aware finalizer, so isolating it means inventing an intermediate commit whose merge-aware dialog title and message describe a path that does not yet exist.

How to vote on this dispute

React to this comment and the next /review applies the answer:

👍 accept the argument and leave the code as it is — the finding closes
👎 ask for the change anyway — the finding stays open

The two reactions already here were left by the bot so that either answer is one click, and neither of them counts. Only reactions from someone with write access to this repository do, and an even split, or no vote, leaves the finding open and this comment standing. Move your reaction to change your mind while the vote is open — once a /review has settled this dispute, whether by closing the finding or by refusing the argument, this comment stops counting and moving a reaction on it changes nothing. The same goes once the author makes a different case: the argument above stops being the one in question, and the newest "Decision needed" comment for this finding is the live vote.

ArturoManzoli marked this pull request as ready for review June 22, 2026 17:55

ArturoManzoli marked this pull request as draft June 22, 2026 17:59

ArturoManzoli marked this pull request as ready for review June 22, 2026 23:11

ArturoManzoli requested review from ES-Alexander and rafaellehmkuhl June 22, 2026 23:11

ArturoManzoli mentioned this pull request Jun 23, 2026

Mission planning: Add mission library #2654

Open

ArturoManzoli force-pushed the mission-library-free-placement branch 2 times, most recently from 4a0eaa5 to 48c8e4c Compare July 1, 2026 20:54

ArturoManzoli force-pushed the mission-library-free-placement branch from 48c8e4c to 0e87529 Compare July 1, 2026 22:07

ArturoManzoli force-pushed the mission-library-free-placement branch from 0e87529 to 572da50 Compare July 2, 2026 13:27

bluerobotics deleted a comment from github-actions Bot Jul 2, 2026

ArturoManzoli force-pushed the mission-library-free-placement branch from 572da50 to 7dad330 Compare July 2, 2026 13:49

ArturoManzoli force-pushed the mission-library-free-placement branch from 7dad330 to 272ffd8 Compare July 2, 2026 14:37

ArturoManzoli requested a review from rafaellehmkuhl August 20, 2026 19:39

ArturoManzoli force-pushed the mission-library-free-placement branch from a0160df to 1b1b598 Compare August 21, 2026 16:06

ArturoManzoli force-pushed the mission-library-free-placement branch from 1b1b598 to da069a1 Compare August 21, 2026 16:30

ArturoManzoli added 4 commits August 25, 2026 10:12

ArturoManzoli force-pushed the mission-library-free-placement branch from da069a1 to 608ceb3 Compare August 25, 2026 13:15

Uh oh!

Conversation

ArturoManzoli commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArturoManzoli commented Jun 22, 2026

Uh oh!

github-actions Bot commented Jun 22, 2026

Automated PR Re-review 1 (Claude)

New findings

0. Summary

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

Uh oh!

ArturoManzoli commented Jun 22, 2026

Uh oh!

github-actions Bot commented Jun 22, 2026

Automated PR Re-review 2 (Claude)

Previous findings status

New findings

0. Summary

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

Uh oh!

ArturoManzoli commented Jun 22, 2026

Uh oh!

ArturoManzoli commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Automated PR Re-review 3 (Claude)

Previous findings status

Discussion since last review

New findings

0. Summary

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

Uh oh!

ArturoManzoli commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Automated PR Re-review 4 (Claude)

Previous findings status

New findings

0. Summary

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

Uh oh!

ArturoManzoli commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ArturoManzoli commented Jun 22, 2026 •

edited

Loading

ArturoManzoli commented Jul 2, 2026 •

edited

Loading

ArturoManzoli commented Jul 2, 2026 •

edited

Loading

ArturoManzoli commented Jul 2, 2026 •

edited

Loading