Mission Planning: Prevent removing the primary NAV_WAYPOINT command

[ArturoManzoli]

• A waypoint's sole MAV_CMD_NAV_WAYPOINT could be deleted (or edited into a different command), so that waypoint vanished on upload and every later mission item ID shifted.
• The delete button is now disabled when it would remove the last MAV_CMD_NAV_WAYPOINT, with a tooltip explaining why.
• Editing that protected command locks the type/command selectors so only its parameters can change; a short hint explains the lock.
• The mission store also refuses remove/update mutations that would leave a waypoint without a MAV_CMD_NAV_WAYPOINT.

Image 1 - The trash button next to the sole MAV_CMD_NAV_WAYPOINT (1) is now disabled.

Closes #2642

[github-actions]

Automated PR Review (Claude)

0. Summary

Verdict: MINOR SUGGESTIONS

Minor items to consider: 1.1, 1.2, 5.1.

This PR prevents users from removing or editing away the sole MAV_CMD_NAV_WAYPOINT command on a waypoint — a bug that caused the waypoint to silently vanish on mission upload and shifted all subsequent sequence IDs. The fix is well-layered: the UI disables the delete button and locks the command/type selectors when editing a protected command, while the store mutations (removeCommandFromWaypoint, updateWaypointCommand) add defense-in-depth guards. Two small pure helpers (isNavWaypointCommand, countNavWaypointCommands) are correctly placed in src/types/mission.ts.

1. Correctness & Implementation Bugs

1.1 (minor) — Silent no-op in store mutations. removeCommandFromWaypoint and updateWaypointCommand in src/stores/mission.ts silently return when the invariant would be violated. The UI already prevents these calls, so the guard is defense-in-depth — but a silent return makes it hard to detect programming errors elsewhere. Consider logging a warning (e.g. console.warn(...)) or throwing, so future callers know the operation was rejected rather than wondering why nothing happened.

1.2 (minor) — isNavWaypointCommand return type could be a type predicate. In src/types/mission.ts:163, the function returns boolean. Changing the signature to (command: MissionCommand): command is MavlinkNavCommand would narrow the type at call sites and is the idiomatic TypeScript approach for discriminator checks. Not a bug, but worth considering since callers may eventually need the narrowed type.

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX

5.1 (minor) — No logUserAction for the new disabled-state interactions. The edit button (editCommand) does not currently log a user action (pre-existing), but the new lock behavior and the disabled delete button introduce a visible change in the user's interaction flow. Per AGENTS.md, new user-interaction features should be logged. At minimum, consider logging when the user attempts to edit a protected command (the editCommand call site) so the action log shows they opened the form with the lock active. This is a minor gap — the existing editCommand already lacked logging.

6. Code Quality & Style — ✅

7. Commit Hygiene — ✅

Single commit (mission-planning: keep at least one MAV_CMD_NAV_WAYPOINT) with a clear subject and body. Clean history.

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional

10.1 (nit) — The hint text in CommandInputForm.vue ("The primary MAV_CMD_NAV_WAYPOINT command can't be changed.") uses a typographic contraction. Consider "cannot" for consistency with the tooltip text in WaypointConfigPanel.vue which uses the longer phrasing "A waypoint must keep a MAV_CMD_NAV_WAYPOINT command".

10.2 (nit) — isNavWaypointCommand and countNavWaypointCommands live in src/types/mission.ts, which is otherwise a pure type/interface file. Since these are runtime functions (not types), src/libs/ might be a more conventional home per the project's separation-of-concerns guidelines. That said, the file already contains instanceOfCockpitMission (a runtime function), so this is consistent with the current state of the codebase.

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

[ArturoManzoli]

Done:

• (1.1) store guards now throw instead of silently returning, matching the sibling not-found/invalid-index checks in the same functions so a rejected mutation surfaces instead of vanishing.
• (1.2) isNavWaypointCommand is now a command is MavlinkNavCommand type predicate.
• (10.1) locked-form hint now reads "cannot", matching the delete tooltip wording.

Not addressed:

• (5.1) this change adds no new discrete user action; the disabled delete button and locked selectors are passive states, and editCommand logging is a pre-existing gap outside this fix's scope.
• (10.2) isNavWaypointCommand/countNavWaypointCommands stay in types/mission.ts next to the existing instanceOfCockpitMission runtime helper, consistent with the file.

[ArturoManzoli]

/review

[github-actions]

Automated PR Re-review 2 (Claude)

Comparing 785c61ebb495bc2cd56e88e7914e787731e328eb → 43c6b48a1eb9c1b46904d31583dd260a5b2a8585. The previous commit was amended/force-pushed, so the incremental diff mirrors the full diff; the re-check is based on the full pr.diff.

Previous findings status

#	Finding	Severity	Status
1.1	Silent no-op in store mutations — should throw or warn	minor	✅ Addressed
1.2	isNavWaypointCommand return type should be a type predicate	minor	✅ Addressed
5.1	No logUserAction for new disabled-state interactions	minor	⚪ No longer applicable
10.1	Inconsistent "can't" vs "cannot" wording	nit	✅ Addressed
10.2	Runtime helpers in types/mission.ts vs libs/	nit	⚪ No longer applicable

Discussion since last review

ArturoManzoli (comment):

• Confirmed 1.1, 1.2, and 10.1 are addressed in the amended commit.
• Declined 5.1 ("this change adds no new discrete user action; the disabled delete button and locked selectors are passive states, and editCommand logging is a pre-existing gap outside this fix's scope") — this is a fair argument; the new UI elements are passive guards, not new interactive actions, so the finding is no longer applicable.
• Declined 10.2, noting types/mission.ts already holds the runtime helper instanceOfCockpitMission — consistent with the current codebase; accepted.

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

Minor item to consider: 6.1.

The PR prevents deleting or editing away the sole MAV_CMD_NAV_WAYPOINT command on a waypoint. All three previously-actionable findings (1.1, 1.2, 10.1) have been addressed: the store guards now throw, isNavWaypointCommand uses a type predicate, and the hint wording is consistent. The implementation is clean and well-layered. One small JSDoc accuracy nit remains.

1. Correctness & Implementation Bugs — ✅

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX — ✅

6. Code Quality & Style

6.1 (nit) — JSDoc @returns type mismatch on isNavWaypointCommand. The JSDoc reads @returns {boolean} but the actual TypeScript return type is the type predicate command is MavlinkNavCommand. The {boolean} tag is not wrong (a type predicate is a boolean at runtime), but updating it to @returns {command is MavlinkNavCommand} (or simply removing the explicit type from the tag and letting the TS signature speak) would keep the documentation aligned with the code and avoid confusing readers who see a type predicate in the signature but boolean in the doc.

src/types/mission.ts:161–163

7. Commit Hygiene — ✅

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional — ✅

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

[ArturoManzoli]

Done:

• (6.1) isNavWaypointCommand's JSDoc @returns now uses {command is MavlinkNavCommand} to match the type-predicate signature.

[ArturoManzoli]

/review

[github-actions]

Automated PR Re-review 3 (Claude)

Comparing 43c6b48a1eb9c1b46904d31583dd260a5b2a8585 → 195598442989511e243fb72dbb89909b1dc4e568. The commit was amended/force-pushed; incremental diff mirrors the full diff so the re-check is based on pr.diff.

Previous findings status

#	Finding	Severity	Status
6.1	JSDoc @returns type mismatch on isNavWaypointCommand ({boolean} vs type predicate)	nit	✅ Addressed

Discussion since last review

ArturoManzoli (comment): confirmed 6.1 is addressed — @returns now reads {command is MavlinkNavCommand} matching the type-predicate signature.

New findings

0. Summary

Verdict: READY TO MERGE

The PR prevents deleting or editing away the sole MAV_CMD_NAV_WAYPOINT command on a mission waypoint, guarding the UI (disabled button + locked selectors) and the store mutations (defensive throws). The single remaining nit from the previous review (6.1 — JSDoc @returns type) has been addressed. Single clean commit, well-structured implementation, no new issues found.

1. Correctness & Implementation Bugs — ✅

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX — ✅

6. Code Quality & Style — ✅

7. Commit Hygiene — ✅

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional — ✅

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

[ArturoManzoli]

Ready to go.