Skip to content

Mission Planning: Add free map placement to mission library items#2802

Open
ArturoManzoli wants to merge 6 commits into
bluerobotics:masterfrom
ArturoManzoli:mission-library-free-placement
Open

Mission Planning: Add free map placement to mission library items#2802
ArturoManzoli wants to merge 6 commits into
bluerobotics:masterfrom
ArturoManzoli:mission-library-free-placement

Conversation

@ArturoManzoli

@ArturoManzoli ArturoManzoli commented Jun 22, 2026

Copy link
Copy Markdown
Contributor
  • 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 from library" entry to the map context menu so a saved mission can be inserted 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 in orange so the endpoints are obvious before inserting near them.

Diff excluding the borrowed [drop] commits: $\color{green}\pmb{+1{,}517}$ $\color{red}\pmb{-34}$

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

Second and final PR splitting #2654:

To be merged after #2795

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

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

Copy link
Copy Markdown

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:1235JSON.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.vuewatchDebounced 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.tssaveMissionToLibrary 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-1438utf8ToBase64 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 ArturoManzoli marked this pull request as draft June 22, 2026 17:59
@ArturoManzoli

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

Copy link
Copy Markdown

Automated PR Re-review 2 (Claude)

Comparing fecb133fc63bd42bfb549ac6 (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.tsJSON.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.tssaveMissionToLibrary missing JSDoc on the function itself minor ❌ Not addressed
6.3 src/libs/mission/library.tsutf8ToBase64 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.vueaddMissionFromLibraryContextMenu 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.vuegetEndpointSplicePositionForLatLng 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.vuegetEndpointInsertIndexForLatLng 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 nullundefined 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

Copy link
Copy Markdown
Contributor Author

1.4 (minor) — MissionPlanningView.vueaddMissionFromLibraryContextMenu 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 ArturoManzoli marked this pull request as ready for review June 22, 2026 23:11
@ArturoManzoli ArturoManzoli force-pushed the mission-library-free-placement branch 2 times, most recently from 4a0eaa5 to 48c8e4c Compare July 1, 2026 20:54
@ArturoManzoli

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

Automated PR Re-review 3 (Claude)

Comparing d42bfb549ac648c8e4c4821e. 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.tsJSON.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.vuefixed 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.vueaddWaypoint 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.vuependingSimplePathInsertIndex 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.tssaveMissionToLibrary 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 ArturoManzoli force-pushed the mission-library-free-placement branch from 48c8e4c to 0e87529 Compare July 1, 2026 22:07
@ArturoManzoli

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

Automated PR Re-review 4 (Claude)

Comparing 48c8e4c4821e0e87529d424b. 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.tsJSON.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.vuefixed 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

ArturoManzoli commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

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 ArturoManzoli force-pushed the mission-library-free-placement branch from 0e87529 to 572da50 Compare July 2, 2026 13:27
@ArturoManzoli

Copy link
Copy Markdown
Contributor Author

/review

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

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

Automated PR Re-review 6 (Claude)

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

Previous findings status

# Finding Severity Status
1.2 useMissionPlacement.tsJSON.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.vuecloseModal / 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.vuefixed 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.vueaddWaypointFromContextMenu 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

ArturoManzoli commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

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 ArturoManzoli force-pushed the mission-library-free-placement branch from 572da50 to 7dad330 Compare July 2, 2026 13:49
@ArturoManzoli

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

Automated PR Re-review 7 (Claude)

Comparing 572da506732c7dad3300ce2e (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.tsJSON.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.vuecloseModal / 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.vuefixed 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.

Add a "Planning for" selector (Surface Boat, Submarine, UAV, Ground
Rover) to the planner sidebar that users can pick while no vehicle is
connected, backed by a new `plannedVehicleType` store field (persisted
under `cockpit-planned-vehicle-type`) and an `effectiveVehicleType`
reader that prefers the connected vehicle's real type and falls back
to the planned one. Mission estimates and vehicle-specific UI cues now
consume `effectiveVehicleType` so they keep working offline, and the
Surface Boat options gear is hidden when no vehicle is online since
its tweaks never reach the generic offline estimator anyway.
Replace the toolbar's separate save/load buttons with a single
bookshelf entry that opens a new MissionLibraryModal — a grid of
preview cards (thumbnail, name, date, vehicle type, stats pills) for
the user's saved missions, with save and detail dialogs and an "Open
in Google Earth" link. Entries are persisted under
`cockpit-mission-library` (BlueOS-synced when available) and captured
at save time with a baked SVG thumbnail and a precomputed
`MissionEstimatesSnapshot`, so summaries render without recomputing
against the currently planned mission. Loads go through a
placement-choice dialog where "Keep original location" replays the
saved settings via loadDraftMission — the repositioning path arrives
in a follow-up PR.
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.
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, and a
restore button resets scale and rotation to defaults.

The interactive state machine (drag, scale-corner handles, rotation
handle, raf-coalesced preview rebuild, snackbar hint) lives in a
leaflet-aware composable (`useMissionPlacement`) so the host view only
owns mission routing and toolbar positioning. The composable manages
its own preview layers, mouse listeners, and zoom-end rebuild, and
tears everything down on scope dispose to keep the view's onUnmounted
free of placement code.

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); or
- replays the saved settings when the planner is empty.

buildCurrentMissionSnapshot, currentMissionSnapshot and its
watchDebounced, and currentMissionEstimatesSnapshot are also
colocated next to drawMissionOnTheMap so the placement section can
grow without bundling unrelated function-definition relocations into
the same diff.
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 via a new `openSaveOnMount` prop / `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, a small getEndpointInsertIndexForLatLng
helper 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
(orange fill via `#ff9800`, scaled up to 1.25x around the center) so
it's visually obvious which waypoints are the mission's start and
end. 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.
@ArturoManzoli ArturoManzoli force-pushed the mission-library-free-placement branch from 7dad330 to 272ffd8 Compare July 2, 2026 14:37
@ArturoManzoli

ArturoManzoli commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

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

@ArturoManzoli

Copy link
Copy Markdown
Contributor Author

/review

1 similar comment
@ArturoManzoli

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

Automated PR Re-review 8 (Claude)

Comparing 7dad3300ce2e272ffd87b914. The incremental diff (4789 lines) is larger than the full PR diff (3536 lines), indicating a rebase/force-push made it unreliable; this re-review is based on the full pr.diff instead.

Previous findings status

# Finding Severity Status
1.2 useMissionPlacement.tsJSON.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
2.2 MissionLibraryModal.vuecloseModal / 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 ⚪ No longer applicable — author confirmed this belongs to the [drop] commit from PR #2795
5.1 ContextMenu.vuefixed right-0 on submenu chevron nit ✅ Addressed — changed to absolute right-1
5.2/5.3 Missing logUserAction on context-menu add helpers major ✅ Addressed
6.1 MissionPlanningView.vue >2000 lines major :large_yellow_circle: Partially addressed — composable extractions are good progress but the file grows from ~4989 to ~5378 lines (+496 / −107)
6.5 saveMissionToLibrary / deleteSavedMission missing JSDoc minor ⚪ No longer applicable — [drop] commit files
6.6 getEndpointInsertIndexForLatLng one-liner wrapper nit ✅ Addressed — removed, conversion inlined at the call site
10.1 Duplicated METERS_PER_DEGREE_LAT constant nit ✅ Addressed — exported from library.ts, imported in useMissionPlacement.ts

Discussion since last review

@ArturoManzoli explained that findings 5.1 (chevron positioning), 6.6 (wrapper removal), and 10.1 (constant duplication) are all addressed in this push. Finding 4.1 (watchDebounced running unconditionally) was clarified as owned by the [drop] commit from PR #2795, same as 2.2 and 6.5.

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

Items worth attention: 6.1 (ongoing).

This push addresses the chevron positioning fix (5.1), removes the one-liner wrapper (6.6), and deduplicates the METERS_PER_DEGREE_LAT constant (10.1). No new correctness, security, or performance issues were introduced. The only remaining concern is the ongoing growth of MissionPlanningView.vue (6.1), which the author has acknowledged as a follow-up.

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 is now ~5378 lines (base 4989 + 496 additions − 107 deletions). The composable extractions (useMissionPlacement, useMissionInsertion) represent good structural progress, but the view itself continues to accumulate placement-toolbar positioning logic, context-menu routing wrappers (addSimplePathFromContextMenu, addSurveyFromContextMenu, addMissionFromLibraryContextMenu, openSegmentRadialMenuFromContextMenu), and endpoint-bias helpers. Acknowledged as 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

Copy link
Copy Markdown
Contributor Author

@rafaellehmkuhl This one is also ready to review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant