Skip to content

Mission Planning: Add mission library with save, load and import/export#2795

Open
ArturoManzoli wants to merge 2 commits into
bluerobotics:masterfrom
ArturoManzoli:mission-library-save-load
Open

Mission Planning: Add mission library with save, load and import/export#2795
ArturoManzoli wants to merge 2 commits into
bluerobotics:masterfrom
ArturoManzoli:mission-library-save-load

Conversation

@ArturoManzoli

@ArturoManzoli ArturoManzoli commented Jun 19, 2026

Copy link
Copy Markdown
Contributor
  • Add a Mission Library modal listing saved missions with thumbnails, name, date, vehicle type, length, ETA and waypoint/survey counts.
  • Replace the toolbar's separate save/load buttons with a single "Mission library" entry that opens the modal.
  • Save the currently planned mission (waypoints, surveys, settings, estimates) into the library, with name and description.
  • Load a saved mission back into the planner at its original location.
  • Import and export missions as .cmp files.
  • Add a "Planning for" vehicle-type selector, used while no vehicle is connected, so vehicle-specific planning surfaces follow the planned type.
  • Hide vehicle-specific mission-estimate options when no vehicle is online so the offline planner uses the generic estimator.
  • Persist the library and thumbnails under cockpit-mission-library and cockpit-mission-library-thumbnails, keyed by stable mission id.

First of two sequential PRs splitting #2654 — the follow-up lands on top of this one:

  • mission-library-free-placement — free-placement workflow to drop, drag/scale/rotate and append a library mission before commit, plus a context-menu library submenu, endpoint-biased inserts for waypoint/survey/path/library actions, and orange highlight of the mission's first and last waypoints.
image image image image

@github-actions

Copy link
Copy Markdown

Automated PR Review (Claude)

0. Summary

Verdict: MINOR SUGGESTIONS

Critical items to address: 1.1

This PR adds a Mission Library feature to the mission-planning view: a modal to browse, save, load, delete, import, and export missions. Missions are persisted under cockpit-mission-library / cockpit-mission-library-thumbnails using useBlueOsStorage. A new "Planning for" vehicle-type selector is shown when offline, feeding effectiveVehicleType into the estimator and library. The toolbar's separate save/load-to-file buttons are replaced with a single "Mission library" entry. The implementation is clean, well-structured, and follows existing project conventions closely.


1. Correctness & Implementation Bugs

1.1 majorExported .cmp files lose metadata on re-import. onExportClick strips the id field from the exported object. isSavedMission() requires typeof value.id === 'string' (line ~169 of library.ts), so an exported-then-reimported file always fails the isSavedMission guard and falls through to instanceOfCockpitMission, discarding vehicleType, estimates, description, createdAt, and updatedAt. Either relax isSavedMission to make id optional, or check for the saved-mission-specific fields (name, createdAt, etc.) without requiring id.

src/libs/mission/library.ts:~168–180 / src/components/MissionLibraryModal.vue:~525–537

1.2 minorcurrentMissionSnapshot is a computed that calls JSON.parse(JSON.stringify(...)) on every access. Because this is passed as a prop to the modal and the canSaveCurrent computed reads from it, every reactivity trigger on waypoints/surveys re-serialises the entire mission object (including deep survey arrays). Consider replacing the deep clone with structuredClone for correctness (handles more types) or, better, making currentMissionSnapshot a regular ref updated via a watchEffect/debounced watcher so the clone only runs once per change, not on every downstream read.

src/views/MissionPlanningView.vue (added currentMissionSnapshot computed)

1.3 minordetailLocation can crash on empty missions. If a saved mission has zero waypoints and zero surveys, computeMissionLocation returns mission.settings.mapCenter, which works fine. But .toFixed(6) is called on detailLocation[0] and [1] in the template without a guard against the unlikely case where mapCenter is malformed (e.g. a corrupted persisted entry). A defensive ?.toFixed(6) ?? '—' would prevent a render crash.

src/components/MissionLibraryModal.vue:~242


2. AGENTS.md Adherence

2.1 minorScope: duplicate-import merge is unrelated to the PR. The diff merges two separate import lines (MavCmd and MavType from mavlink2rest-enum) into one line in the base MissionPlanningView.vue. While harmless, this is an import-reorder on code not otherwise changed, which AGENTS.md scope discipline forbids unless explicitly requested.

src/views/MissionPlanningView.vue (diff hunk around line 715)

2.2 nitMissing JSDoc on several private-looking helpers that are actually exported. computeBounds and utf8ToBase64 in library.ts are module-private (not exported), so JSDoc is not required by the rules. However, PLANNABLE_VEHICLE_TYPES is exported and has a JSDoc, which is good. Just flagging that computeMissionLocation's JSDoc @param type annotation says {CockpitMission} but the actual type on the overload that receives a SavedMission (which extends CockpitMission) would be more precise as {CockpitMission | SavedMission} — not blocking.


3. Security — ✅

4. Performance

4.1 minorJSON.parse(JSON.stringify(...)) deep-clone in a computed. As noted in 1.2, every reactive access to currentMissionSnapshot triggers a full serialisation round-trip. This is fine while the modal is closed (the computed is not read), but while the modal is mounted, Vue will re-evaluate the computed every time waypoints or surveys change. If planning is active at the same time, this could be noticeable. Converting to a debounced ref would cap the clone to once per change batch.


5. UI / UX — ✅

6. Code Quality & Style

6.1 minorMissionPlanningView.vue is already ~4 824 lines and this PR adds ~60 net. The file is well past the ~2 000-line guideline. The newly added currentMissionSnapshot, currentMissionEstimatesSnapshot, openMissionLibrary, and handleLoadMissionFromLibrary are all good candidates for extraction into a composable (e.g. useMissionLibrary) to stop the file from growing further. Not blocking, but worth tracking.

6.2 nitScoped CSS duplicates Tailwind utilities. Several declarations in MissionLibraryModal's <style scoped> block duplicate available Tailwind classes (e.g. .dialog { position: fixed; top: 0; left: 0; width: 100vw; height: 100vh; display: flex; justify-content: center; align-items: center; }fixed inset-0 w-screen h-screen flex justify-center items-center). Mixing both patterns is fine for the custom properties like --v-overlay-opacity, but the pure layout props could lean on Tailwind for consistency with the rest of the template.


7. Commit Hygiene — ✅

Commits are well-scoped, one logical change each, with clear conventional-commit-style subjects. No noise commits or leftover fixups.


8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional

10.1 nit — The googleEarthUrl helper hard-codes altitude and camera distance (500a,1000d). A brief inline comment explaining why those values were chosen would help future readers.

10.2 nitsaveForm.id is typed string | undefined and initialised to undefined but never set to a string anywhere in the current PR. It's presumably reserved for the update-in-place flow in a follow-up PR. A short // Reserved for future update-in-place support comment would clarify intent.


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

@ArturoManzoli ArturoManzoli force-pushed the mission-library-save-load branch from 3c568f1 to 9640c81 Compare June 19, 2026 13:37
@ArturoManzoli

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

Copy link
Copy Markdown

Automated PR Re-review 2 (Claude)

Comparing 3c568f11e239b5461fc58799247673108290859e9640c8188bd400ef03ea8ff5ca3b15c4f5806846

The incremental diff included the full PR content (consistent with a rebase/force-push that rewrote the branch), so re-checking was performed against the complete pr.diff.

Previous findings status

# Finding Severity Status
1.1 Exported .cmp files lose metadata on re-import (isSavedMission required id) major ✅ Addressed — isSavedMission now discriminates on name/createdAt/updatedAt instead of id, with a clear comment explaining the design choice.
1.2 currentMissionSnapshot as a computed with JSON.parse(JSON.stringify(...)) minor ✅ Addressed — replaced with a ref backed by watchDebounced + structuredClone(toRaw(...)).
1.3 detailLocation crash on empty/malformed missions without ?.toFixed(6) minor ✅ Addressed — template now uses detailLocation[0]?.toFixed(6) ?? '—'.
2.1 Scope: unrelated duplicate-import merge of MavCmd/MavType lines minor ✅ Addressed — no unrelated import merges remain in the current diff.
2.2 JSDoc @param type on computeMissionLocation could be CockpitMission | SavedMission nit ⚪ No longer applicable — CockpitMission is the correct parameter type since SavedMission extends CockpitMission.
4.1 Performance: deep-clone in a computed fires on every reactive read minor ✅ Addressed — same fix as 1.2.
6.1 MissionPlanningView.vue exceeds ~2 000-line guideline minor ❌ Not addressed — the file is still well past the threshold. Not blocking for this PR.
6.2 Scoped CSS in MissionLibraryModal.vue duplicates Tailwind utilities nit :large_yellow_circle: Partially addressed — some custom properties (--v-overlay-opacity, box-shadow, backdrop-filter) genuinely need scoped CSS; the pure layout properties (.dialog's position: fixed; top: 0; left: 0; width: 100vw; ...) could still lean on Tailwind.
10.1 googleEarthUrl hardcodes altitude/camera distance without a comment nit ❌ Not addressed
10.2 saveForm.id unused in the current PR, no clarifying comment nit ❌ Not addressed

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

The three new commits since the last review (be147365, 5fe10d39, 9640c818) are all clean, well-scoped changes: hiding the boat-specific estimate options when offline, renaming the confusingly-shadowing length field to pathLength, and extracting the vehicle-type label list into the shared library.ts helper. All three critical/major findings from the first review have been addressed — isSavedMission no longer requires id, the snapshot is now a debounced ref with structuredClone, and detailLocation has defensive optional chaining. No new critical or major issues found.


1. Correctness & Implementation Bugs — ✅

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX — ✅

6. Code Quality & Style — ✅

7. Commit Hygiene

7.1 nit — Several commit messages include a trailing Made-with: Cursor line. This is a minor hygiene observation — it doesn't affect the code but may be worth stripping before squash-merge if the project doesn't want tool attribution in the permanent history.


8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional — ✅

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

@ArturoManzoli

ArturoManzoli commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

6.1 MissionPlanningView.vue exceeds ~2 000-line guideline minor ❌ Not addressed — the file is still well past the threshold. Not blocking for this PR.

Will be done on the second PR on this sequence. For this one, adding a composable for "open the modal, hand it to a handler, call loadDraftMission" is over-engineering for a single call site.

@rafaellehmkuhl rafaellehmkuhl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will continue the review later, but one point here to have in mind: I believe the automation in the commit creation ended up separating logical changes in more commits than it should. Here it ended up separating a lot of changes that should be together, increasing the review burden as one needs to go back and fourth between commits that are just doing the same logical change.

Comment on lines 47 to +132
@@ -128,6 +129,7 @@ export const useAppInterfaceStore = defineStore('responsive', {
},
isConfigModalVisible: (state) => state.configModalVisibility,
isVideoLibraryVisible: (state) => state.videoLibraryVisibility,
isMissionLibraryVisible: (state) => state.missionLibraryVisibility,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to have this kind of code change in a separate commit. It has no purpose without the mission library itself.

Comment thread src/stores/mission.ts
const homeMarkerPosition = ref<WaypointCoordinates | undefined>(undefined)

// Fallback vehicle type used by vehicle-specific planning features when no vehicle is connected.
const plannedVehicleType = useBlueOsStorage<MavType | undefined>('cockpit-planned-vehicle-type', undefined)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a strange resource. Does it make sense to save the vehicle type inside a vehicle settings?

import { MavType } from '@/libs/connection/m2r/messages/mavlink2rest-enum'
import { useAppInterfaceStore } from '@/stores/appInterface'
import { useMainVehicleStore } from '@/stores/mainVehicle'
import { useMissionStore } from '@/stores/mission'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep the "planned vehicle type" feature, this kind of change should be on the same commit.

@ArturoManzoli ArturoManzoli force-pushed the mission-library-save-load branch from 9640c81 to d304c01 Compare June 29, 2026 19:14
@ArturoManzoli

Copy link
Copy Markdown
Contributor Author

I will continue the review later, but one point here to have in mind: I believe the automation in the commit creation ended up separating logical changes in more commits than it should. Here it ended up separating a lot of changes that should be together, increasing the review burden as one needs to go back and fourth between commits that are just doing the same logical change.

As discussed in private, I have reorganized the commit structure to better reflect the logical changes

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.
@ArturoManzoli ArturoManzoli force-pushed the mission-library-save-load branch from d304c01 to 6c78b01 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 9640c8188bd400ef03ea8ff5ca3b15c4f58068466c78b01e53391452c3831a61198ccf5e61bc6dcc

The branch was force-pushed/rebased since the last review (the incremental diff includes unrelated changes to .eslintrc.cjs, AGENTS.md, index.html, ExpansiblePanel.vue, MainMenu.vue, etc.), so this re-check is based on the full pr.diff.

The commit history was reorganized from 8 commits down to 2 clean logical commits (aa265e2f mission-planning: support offline vehicle-type planning, 6c78b01e mission-planning: add mission library save/load), addressing the maintainer's review feedback about commit structure.

Previous findings status

# Finding Severity Status
1.1 Exported .cmp files lose metadata on re-import (isSavedMission required id) major ✅ Addressed
1.2 currentMissionSnapshot as a computed with JSON.parse(JSON.stringify(...)) minor ✅ Addressed
1.3 detailLocation crash on empty/malformed missions without ?.toFixed(6) minor ✅ Addressed
2.1 Scope: unrelated duplicate-import merge of MavCmd/MavType lines minor ✅ Addressed
2.2 JSDoc @param type on computeMissionLocation could be CockpitMission | SavedMission nit ⚪ No longer applicable
4.1 Performance: deep-clone in a computed fires on every reactive read minor ✅ Addressed
6.1 MissionPlanningView.vue exceeds ~2 000-line guideline minor :large_yellow_circle: Partially addressed — see discussion below
6.2 Scoped CSS in MissionLibraryModal.vue duplicates Tailwind utilities nit :large_yellow_circle: Partially addressed — custom visual properties remain (appropriate), but .dialog and .modal-content still contain pure layout (display: flex; flex-direction: row; position: fixed; width: 100vw; height: 100vh) that could use Tailwind. Not blocking.
7.1 Made-with: Cursor lines in commit messages nit ✅ Addressed — commit messages are now clean.
10.1 googleEarthUrl hardcodes altitude/camera distance without a comment nit ❌ Not addressed
10.2 saveForm.id unused in the current PR nit ⚪ No longer applicable — saveForm.id is passed through to saveMissionToLibrary and enables the update-in-place code path, which is valid infrastructure for the follow-up PR.

Discussion since last review

  • @ArturoManzoli (comment): re finding 6.1 (MissionPlanningView.vue size), noted that extracting a composable for the modal call is over-engineering for a single call site and will be addressed in the second PR. This is a reasonable position — the file is already very large (~4800 lines on base) and this PR only adds ~43 net lines. The problem is pre-existing and not made meaningfully worse here.
  • @rafaellehmkuhl (inline, inline, inline): raised concerns about commit structure (separate commits for tightly coupled changes) and questioned whether plannedVehicleType belongs in the mission store. These have been addressed by reorganizing into 2 logical commits.
  • @ArturoManzoli (comment): confirmed commit restructuring was done.

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

This PR adds a mission library feature with save, load, import/export of .cmp files, and an offline "Planning for" vehicle-type selector. The code is well-structured: domain logic lives in src/libs/mission/library.ts, types in src/types/mission.ts, persistence in the mission store, and the modal is a dedicated component. All prior critical/major findings have been addressed. User interactions are comprehensively logged via logUserAction. The commit history has been cleaned up to 2 logical commits. Minor suggestions remain in sections 5 and 6.

1. Correctness & Implementation Bugs — ✅

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX

5.1 minorMissing logUserAction on several discrete user actions. The PR correctly logs most interactions, but the following user-initiated actions are not logged:

  • Clicking the "Close" button in the left panel of the library modal (closeModal in MissionLibraryModal.vue)
  • Clicking "Import file" (triggerImportFile)
  • Opening the save dialog (openSaveDialog)

Per AGENTS.md, every discrete user interaction should produce a logUserAction(...) entry. These are all user-initiated button clicks.

6. Code Quality & Style

6.1 minorMissionPlanningView.vue exceeds ~2 000 lines (pre-existing, ~4862 post-merge). Carried forward from the previous review. Per author's comment, this will be addressed in the follow-up PR. Not blocking.

7. Commit Hygiene — ✅

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional

10.1 nitgoogleEarthUrl in MissionLibraryModal.vue:405 hardcodes 500a,1000d (altitude/camera distance). A brief inline comment explaining what these Google Earth URL parameters mean would help future readers.

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

@rafaellehmkuhl

Copy link
Copy Markdown
Member

@ArturoManzoli let me know if this is ready for review or you still plan on doing changes.

@ArturoManzoli

Copy link
Copy Markdown
Contributor Author

@ArturoManzoli let me know if this is ready for review or you still plan on doing changes.

It's ready to review, now! Thanks

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.

2 participants