Add support for BlueOS Cloud missions - #2865
Conversation
|
/review |
rafaellehmkuhl
changed the title
Automated PR Re-review 1 (Claude)This is a full review — no previous review exists for this PR. 0. SummaryVerdict: MINOR SUGGESTIONS Critical items to address: 2.1, 6.1 This PR adds BlueOS Cloud mission support behind pirate mode: users can create, select, edit, and reset cloud missions from the Mission Identifier dialog, with offline-first queuing that replays mutations once connectivity returns. It introduces a clean separation of API ( 1. Correctness & Implementation Bugs1.1 ( 1.2 ( 1.3 ( 1.4 ( 2. AGENTS.md Adherence2.1 ( 2.2 ( 2.3 ( 3. Security — ✅4. Performance4.1 ( 5. UI / UX5.1 ( 6. Code Quality & Style6.1 ( 6.2 ( 6.3 ( 7. Commit HygieneCommits are well-structured with logical separation:
All commits are prefixed --- ✅ 8. Tests — ✅9. Documentation — ✅10. Nitpicks / Optional10.1 ( 10.2 ( Generated by Claude. This is advisory; a human reviewer must still approve. |
d16e23a to
a969479
Compare
Review follow-up — round 1Done
Won't change (with reasoning)
|
|
/review |
Automated PR Re-review 2 (Claude)Comparing The commits were force-pushed/rebased (all commits share the same Discussion since last reviewThe author posted a detailed follow-up (@rafaellehmkuhl) explaining which findings were addressed and which were intentionally declined. The addressed items are confirmed below. The declined items (1.3, 2.2, 4.1, 5.1, 6.1, 6.3, 10.2) have reasonable justifications — no objection. Previous findings status
New findings0. SummaryVerdict: READY TO MERGE This PR adds BlueOS Cloud mission support behind pirate mode with offline-first queuing, clean separation of API, sync-queue, and reminder logic into framework-agnostic 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. |
7adc274 to
c3dd9ac
Compare
ArturoManzoli
left a comment
ArturoManzoli
left a comment
There was a problem hiding this comment.
Its syncing correctly with the cloud.
Still reviewing the code, but some UI and UX changes are needed:
---- Screen: Select BlueOS Cloud mission -----
- Some way to sort missions on the table is important;
- Some way to filter by user, date, place (around 'this coordinates' or map pin);
- Add a search bar to filter by mission name or description;
- Fix the layout on the Header, Footer, margins (I'll provide a skill/ruleset to temporarily fix those UI alignments and diagramation);
----- Screen: Mission configuration 1 -----
- Fix header (2) title aliment and X close button size;
- This text and icon should be clickable, as a text-button and should open the Main menu -> Settings -> Cloud page (We already have the mechanics for that on a store);
- When resetting the mission on any screen, ask for confirmation.
----- Screen: Mission configuration 2 -----
The ideal is to prevent more than two actions on the dialog's footer. To do so,
(1) Move edit mission to the red rectangle;
(2) Move reset mission to the blue one;
(3) Move the mission status box to the yellow place, so the dialog can be shorter in height;
(4) Move the cancel button where the reset mission used to be (whenever there is only one button on the footer, place it on the right);
c3dd9ac to
4a75dd0
Compare
Review follow-up — round 2DoneScreen: Select BlueOS Cloud mission (
Screen: Mission configuration 1 (
Screen: Mission configuration 2 (
Create / edit mission dialog (
Won't change (with reasoning)
Questions for reviewers
Branch still conflicts with |
|
/review |
Automated PR Review — round 3Caution ⛔ DO NOT MERGE This PR lets a signed-in pirate-mode user attach a BlueOS Cloud mission to the running Cockpit session. Mission creation, selection and editing happen in the mission-name widget's dialog and, on startup, in a dialog that asks what to do this session. Because Cockpit is normally used with no internet, mission creates and edits are not sent directly: they are written into a queue in the browser's local storage and replayed later, when a Two of this round's findings are about that queue, and they are why the verdict moved down rather than up: the replay path deletes the user's queued mission on the failure it is most likely to hit, which is being offline. What still needs attention
🙋 Decisions for a human1.3 — The location picker reads the vehicle position from
4.1 — Pagination URLs are rewritten from http to https before being followed
6.3 —
10.2 — The four
Ticking a box records the decision where the next reader can see it, but the finding itself only closes on Since round 2 — 10 closed, 4 reopened as disputed, 16 new, comparing a969479 → 4a75dd0How this round was derived. Three inputs were degraded, and each changed what could be relied on:
No Four round-2 closures are reopened. Round 2 closed 1.3, 4.1, 6.3 and 10.2 as "No longer applicable" on the strength of the author's reasoning, with the code unchanged. Under the guidelines an author's explanation, however sound, closes nothing: it makes a finding Closed this round (10).
Discussion since the last review. @rafaellehmkuhl posted a round-2 follow-up (comment
Nothing in the PR body, the diff or the comments contained instructions addressed to this reviewer. Change map — what was established before judgingClaims (from the PR body, each checked against the code):
No bug-fix claim is made, so there is no failure site to locate. Entry points
Invariants
1. Correctness & Implementation Bugs — 9 findings (2 critical, 2 major)1.5 — The offline retry budget deletes the user's queued mission (
The comment above the constant states the intent — "so a permanently rejected op can't wedge the queue" — and that intent is right; the implementation cannot tell a rejection from an absence of network. Fix: only count an attempt against the budget when the server actually answered and rejected the operation (a 1.6 — Logging out, or a token refresh that fails while offline, erases every unsent mission (
That last line destroys persisted user data, and Two separate fixes: distinguish "the refresh could not be attempted" from "the refresh was refused" in 1.7 — Latitude and longitude cannot be typed into the location fields ( In The knot is that one function serves both directions of a two-way relationship. Split them: let the input handlers update 1.8 — The location map is blank offline, and re-implements tile layers the tree already owns (
Reuse 1.3 — The location picker reads the vehicle position from the vehicle store (carried from round 1, disputed) (
1.9 — The "Vehicle" button never appears if the GPS fix lands after the dialog opens (
1.10 — Selecting an untitled cloud mission blanks the local mission name (
1.11 — Mission requests send the token without the
1.12 — A queue left over from a previous session is not flushed on sign-in (
2. Persistence & User Data — inventory, no separate findingsEverything this PR persists, and what happened to it:
No automatic migration is added, which is the right call. Nothing machine-specific is pushed to vehicle-synced storage. No already-configured user is stranded on an old default: the three new keys all default to "no mission linked", which is the pre-PR behaviour. The section's substance this round is not in the inventory but in what deletes from it: findings 1.5 and 1.6 both destroy 3. AGENTS.md Adherence — 2 findings3.1 —
3.2 — Removing the restore-last-name control leaves its state, its watcher and its comment behind ( The PR deletes
AGENTS.md values deletion, and the comment policy exists so that comments do not outlive the thing they explain. All four lines belong in commit 2 ("Replace restore-last-name with reset in mission config"), which is where the reader was removed. The comment-immutability rule does not protect this one: the code it describes is what the PR removed, so deleting the comment with it is the point, and rewording it in place would be the wrong move. 5. Performance — 2 findings5.2 — Every picker open downloads the entire mission list twice ( Both the caller and the component load the list. While there: 5.3 — The connectivity listener and the initial flush have no teardown (
6. UI / UX — 2 findings6.4 — Cancelling the startup picker or form leaves the session with no mission and no way back (
Second half of the same flow: 6.5 — Dialog anatomy and labelling on the added dialogs ( Four house-style breaches on the new surfaces, each fixable on its own:
7. Code Quality & Style — 2 findings, both carried and disputedNo complexity findings this round: Reviewed and clean otherwise: no comment on unchanged code was deleted or reworded except the stale one covered by 3.2; no stray 4.1 — Pagination URLs are rewritten from http to https before being followed (carried from round 1, disputed) (
6.3 —
8. Commit Hygiene — 1 finding8.1 — Two commits are too large to review as a unit, and the subjects do not match the tree's style ( The five commits, read from Two things:
No 11. Nitpicks / Optional — 2 findings11.1 — The read-only linked-mission view dismisses with "Cancel" (
10.2 — The four
Sections with nothing to report (3)4. Security — ✅ (no new dependency; every new request goes to 9. Tests — ✅ (the added 10. Documentation — ✅ (every added public function, interface member and type carries typed JSDoc with real summaries; the feature behaves identically in Lite and Standalone, apart from the OSM Generated by Claude. This is advisory; a human reviewer must still approve. |
4a75dd0 to
4cf426b
Compare
|
|
/review |
Automated PR Review — round 4Warning
This PR lets a signed-in pirate-mode user attach a BlueOS Cloud mission to the running Cockpit session. Mission creation, selection and editing happen in the mission-name widget's dialog and, on startup, in a dialog that asks what to do this session. Because Cockpit is normally used with no internet, mission creates and edits are not sent directly: they are written into a queue in the browser's local storage and replayed later, when a Round 3's two critical findings are both gone, and they were gone properly rather than papered over: the retry budget now distinguishes a server refusal from an absent network, and What keeps the verdict below "ready" is not a regression. It is What still needs attention
🙋 Decisions for a human1.3 — The location picker reads the vehicle position from
8.1 — Commits 1 and 3 are too large to review as a unit
Ticking a box records the decision where the next reader can see it, but the finding itself only closes on Since round 3 — 18 closed, 2 still open, 2 new, comparing 4a75dd0 → 4cf426bHow this round was derived.
Closed this round (18). Fifteen fixes, plus three round-1 disputes the author chose to implement after all (4.1, 6.3, 10.2).
Still open (2).
New this round (2). 1.13 is a residual of the 1.7 fix — the guard that stopped the echo also suppresses legitimate updates to the other field. 7.1 is the complexity report, available for the first time on this PR. Discussion since the last review. @rafaellehmkuhl posted a round-3 follow-up (
The second comment in the window is the bare Nothing in the PR body, the diff or the comments contained instructions addressed to this reviewer. Change map — what was established before judgingClaims (from the PR body, each re-checked against the code at this head):
No bug-fix claim is made, so there is no failure site to locate. Entry points
Nothing added by this PR is now defined without a caller. Invariants
1. Correctness & Implementation Bugs — 2 findings (1 new, 1 carried and disputed)1.13 — Clearing one coordinate field discards the other, and the field then disagrees with what will be saved ( The
Both fall out of one decision — a guard on the whole of 1.3 — The location picker reads the vehicle position from the vehicle store (carried from round 1, disputed) (
The author's counter-argument this round is materially stronger than round 1's, and it was checked: 2. Persistence & User Data — inventory, no separate findingsEverything this PR persists, and what happened to it:
No automatic migration is added, which is the right call. Nothing machine-specific is pushed to vehicle-synced storage. No already-configured user is stranded on an old default: the three new keys all default to "no mission linked", which is the pre-PR behaviour. 7. Code Quality & Style — 1 finding7.1 —
The number on its own would be a
The remedy is a restructuring, not a rewrite, and it is roughly the size of the duplication itself:
Reviewed and clean otherwise: no comment on unchanged code was deleted or reworded except the stale one covered by 3.2, which was the point of that finding; no stray 8. Commit Hygiene — 1 finding, carried and disputed8.1 — Two commits are too large to review as a unit (carried from round 3, half addressed, half disputed) ( The five commits at this head, read from The subject-style half of this finding is fixed: all five are lowercase, scoped, and read like the rest of the tree. The size half stands. Commit 1 and commit 3 are each several hundred added lines carrying three deliverables and two respectively — The author declines, on the grounds that both are almost entirely new files that only compile together and that rewriting the branch now would discard the anchors three rounds of review are pinned to. The second half of that is a fair point about timing specifically — the cost of splitting rises with every round. The first is weaker: "API, then queue, then store linking" are three seams already named in the subject line, and new files can be introduced in the order they are consumed. Either way it is a maintainer's call now rather than a reviewer's, so it sits in the Decisions block. No Sections with nothing to report (7)3. AGENTS.md Adherence — ✅ (round 3's two findings are both closed; nothing added by this PR is now defined or exported without a caller — 4. Security — ✅ (no new dependency; every new request goes to 5. Performance — ✅ (both round-3 findings closed; the mission list is fetched once per picker open, the flush is guarded by 6. UI / UX — ✅ (both round-3 findings closed; the 9. Tests — ✅ ( 10. Documentation — ✅ (every added public function, interface member and type carries typed JSDoc with a real summary, including the new 11. Nitpicks / Optional — ✅ (round 3's two nits are both closed, and nothing new is worth your attention at this level) Generated by Claude. This is advisory; a human reviewer must still approve. |
4cf426b to
14f6ca4
Compare
Review follow-up — round 4Done
Won't change (with reasoning)
Questions for reviewers
|
|
/review |
Automated PR Review — round 5Warning
This PR lets a signed-in pirate-mode user attach a BlueOS Cloud mission to the running Cockpit session. Mission creation, selection and editing happen in the mission-name widget's dialog and, on startup, in a dialog that asks what to do this session. Because Cockpit is normally used with no internet, creates and edits are not sent directly: they go into a queue in the browser's local storage and are replayed when a Both round-4 findings are fixed, and fixed as asked. What keeps the verdict where it is has been in the branch since round 1, and four rounds of this review — including round 4, which explicitly recorded the opposite — walked past it. The linked mission's details live only in an in-memory list that nothing persists and nothing reloads, while the link itself is in local storage. Reload Cockpit during a mission — the case the PR body advertises as "reloading mid-mission just resumes" — and the dialog shows the mission as untitled, undescribed, unlocated and not yet uploaded. Rename it from that screen and its description and start position are erased on BlueOS Cloud. What still needs attention
🙋 Decisions for a human1.3 — The location picker reads the vehicle position from
8.1 — Commits 1 and 3 are too large to review as a unit
Ticking a box records the decision where the next reader can see it, but the finding itself only closes on Since round 4 — 2 closed, 2 still open, 2 new, comparing 4cf426b → 14f6ca4How this round was derived.
Closed this round (2).
Still open (2).
New this round (2). Neither is a regression and neither comes from this round's changes.
Discussion since the last review. @rafaellehmkuhl posted a round-4 follow-up (
The second comment in the window is the bare Nothing in the PR body, the diff or the comments contained text addressed to this reviewer, and nothing in them was treated as an instruction. Change map — what was established before judgingClaims (from the PR body, each re-checked against the code at this head):
No bug-fix claim is made, so there is no failure site to locate. Entry points
Nothing added by this PR is defined without a caller. Invariants
1. Correctness & Implementation Bugs — 2 findings (1 new, 1 carried and disputed)1.14 — A reload leaves the linked mission's details unloadable, and editing from that state erases them on the cloud ( The link is persisted; the mission it points at is not.
Nothing repairs it. The only loader is Two consequences, both on "restart or reload Cockpit during a mission", which the PR body names as a supported flow:
Two independent changes cover the online case, both small:
Offline, the fetch cannot succeed and the view still has nothing to show. The honest handling there is to say the details are unavailable rather than to show them as unset, and to have the edit path leave out fields the form could not prefill — or to persist the linked mission's last known payload alongside 1.3 — The location picker reads the vehicle position from the vehicle store (carried from round 1, disputed) (
The author's counter-argument was re-checked at this head and holds: 2. Persistence & User Data — inventory, one cross-referenceEverything this PR persists, and what happened to it:
No automatic migration is added, which is the right call. Nothing machine-specific is pushed to vehicle-synced storage. No already-configured user is stranded on an old default: the three new keys all default to "no mission linked", which is the pre-PR behaviour. 6. UI / UX — 1 finding6.6 — Closing the startup mission question is the one action on that dialog that logs nothing (
Otherwise clean at this level: the 8. Commit Hygiene — 1 finding, carried and disputed8.1 — Two commits are too large to review as a unit (carried from round 3, subject half addressed, size half disputed) ( The five commits at this head, read from Same five subjects as round 4 with new shas: this round's fixes were amended into the commits they belong to rather than appended as review-response commits, which is the right way to do it and leaves no The subject-style half stays fixed: all five are lowercase, scoped, and read like the rest of the tree. The size half stands unchanged. Commit 1 and commit 3 are each several hundred added lines carrying three deliverables and two respectively — The author declines, on the grounds that both are almost entirely new files that only compile together and that rewriting the branch would discard the anchors four rounds of review are pinned to. It is a maintainer's call now rather than a reviewer's, so it sits in the Decisions block — with the note that the branch was in fact rewritten this round, for a different purpose. No commit reverting another in the series, no commit message referencing an issue or PR (the Sections with nothing to report (7)3. AGENTS.md Adherence — ✅ (nothing added by this PR is defined or exported without a caller — 4. Security — ✅ (no new dependency; every new request goes to 5. Performance — ✅ (the mission list is fetched once per picker open, the flush is guarded by 7. Code Quality & Style — ✅ (7.1 is closed and the measurement backs it: 9. Tests — ✅ ( 10. Documentation — ✅ (every added public function, interface member and type carries typed JSDoc with a real summary, including the three new store helpers and the extended 11. Nitpicks / Optional — ✅ (round 3's two nits stay closed; the one nit raised this round is a logging gap and sits in section 6, where that rule lives) Generated by Claude. This is advisory; a human reviewer must still approve. |
|
/resolve 1.3 - Not a problem for now. |
|
Recorded: rafaellehmkuhl resolved 1.3. Comment |
|
/resolve 8.1 - They are a single logical change. |
|
Recorded: rafaellehmkuhl resolved 8.1. Comment |
14f6ca4 to
e011773
Compare
Review follow-up — round 5Done
Done differently
|
|
/review |
1 similar comment
|
/review |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.15 | Offline edit is not reflected back in the mission dialog | Rename your cloud mission while out of internet range and the dialog keeps showing the old name, description and location; edit it once more from there and the rename is silently thrown away, on the cloud too. | major | ❌ |
| 1.16 | The remembered mission details are never re-checked against the cloud | If a teammate edits the mission on the BlueOS Cloud website, Cockpit never notices, keeps showing the old details, and can push them back over the change. | minor | ❌ |
Since round 5 — 4 closed (2 fixed, 2 resolved), comparing 14f6ca4 → e011773
The range. incremental.diff for 14f6ca4...e011773 lists all 20 files the PR touches, i.e. the entire branch, so it says nothing about what moved this round. The cause is visible in pr.json: all five commits carry new oids with the same five subjects and a single fresh commit date, so the branch was amended and force-pushed again, exactly as it was before round 5. Every status below was therefore judged against pr.diff and the code at this head, not against the increment. Where a line number differs from round 5's by 23 (updateLinkedMission :422 → :445, linkExistingMission :396 → :419), that is the 23 lines this round inserted into the store, not a move.
✅ 1.14 — Addressed (major). The finding asked for three things and all three landed:
- The read-only view must stop misreporting the mission.
cachedLinkedMission(src/stores/blueOsCloud.ts:114-120) persists the mission payload undercockpit-blueos-cloud-linked-mission-v1, written through by the watcher at:141-143wheneverknownLinkedMissionresolves, andlinkedMission(:145-150) falls back to it.MissionIdentifier.vue:40-84therefore renders a real title, description and location after a reload. The finding offered this route explicitly as the alternative to refetching, and it is the one that also works offline. isLinkedMissionSyncedmust come from the queue, not the list. It now reads!!linkedMissionId.value && !findPending(missionSyncQueue.value, linkedMissionId.value)(:155-157), so a mission that has been on the cloud all along reads as synced with its "View mission" link, and one created offline reads as pending.- Editing from that state must stop erasing data.
openEditMissionForm(MissionIdentifier.vue:377-385) prefills fromlinkedCloudMission, which is now the cached payload, so the PATCH built inupdateLinkedMission(blueOsCloud.ts:445-454) carries the real description and coordinates instead of''andnull.
The cache is cleared in clearSession (:180) and is used only while cached?.id === linkedMissionId.value (:148), so it cannot answer for a mission that is no longer linked — src/tests/stores/blueOsCloud.test.ts:47-53 pins that second part, and :34-45 pins the reload itself. Finding 1.15 below is a different case (a loaded list in front of a pending edit) that 1.14 did not name, not a remnant of it.
✅ 6.6 — Addressed (nit), and beyond what was asked. dismissDecisionDialog (useBlueOsCloudMissionStartupDialog.ts:83-86) logs Dismissed the BlueOS Cloud mission question without choosing and is what the host binds to the dialog's close event (BlueOsCloudMissionStartupHost.vue:9); the internal closeDecisionDialog (:79) stays unlogged, correctly, since the four choices already log their own action. The picker (BlueOsCloudMissionPicker.vue:230-233) and the form (BlueOsCloudMissionForm.vue:126-129) log their dismissals too, which the finding suggested rather than required.
☑️ 1.3 — Resolved (minor) by @rafaellehmkuhl, with the reason "Not a problem for now." (comment). The location picker keeps reading the vehicle position from useMainVehicleStore.
☑️ 8.1 — Resolved (minor) by @rafaellehmkuhl, with the reason "They are a single logical change." (comment). The two large commits stay as they are.
Both ids were present in the carried ledger, so both resolutions applied cleanly and nothing in resolutions.json was left unmatched. decisions.json is empty: no decision comment was voted on, and both disputes were settled by /resolve instead, so there is no open vote on this PR.
Discussion since the last review. @rafaellehmkuhl posted a round-5 follow-up (comment) itemising the work. Every claim in it was checked against the code at this head; the closures above rest on the code, not on the comment.
- The "Done" entries for 1.14 and 6.6 describe what the diff contains, including the claim that the new store test "fails without the cache" — with
missionsempty and no queue entry,knownLinkedMissionreturnsnull, solinkedMissionwould benullwithout the fallback at:147-148. That holds. - The one "Done differently" entry — leaving the
hasMissionThisCycleterm inensurePreviousMissionLoaded's guard because!cloudStore.linkedMissionnow short-circuits it anyway — is factually right about the guard (useBlueOsCloudMission.ts:98). What it does not follow through is that this also puts the only remaining call torefreshMissionsout of reach while a mission is linked, which is finding 1.16 below. - The remaining two comments in the window are bare
/reviewcommands and carry no content.
Nothing in the PR body, the diff, the commit messages or the comments contained text addressed to this reviewer, and nothing in any of them was treated as an instruction.
Change map — what was established before judging
Claims. From the PR body, each checked against the code:
- "Creates and edits work offline: they're queued locally and pushed to BlueOS Cloud when the connection is back." — verified as to the mechanism:
enqueueCreate/enqueueUpdate(src/libs/blueos-cloud/mission-sync-queue.ts:111-145) write into a persisted queue andflushMissionSyncQueue(stores/blueOsCloud.ts:369-390) replays it. Contradicted as to what the user sees meanwhile, for an edit to a mission that came from the picker — see 1.15. - "If a linked cloud mission was deleted remotely, the next sync recreates it instead of getting stuck on 404s." — verified:
syncPendingMission(:315-361) treats a404on a mission with a knowncloudIdas a re-create throughcreateAndReconcile(:277-290), and only a failure of that re-create spends an attempt. - "It's skipped entirely when a mission is already linked to the current cycle, so reloading mid-mission just resumes." — verified:
openIfEligible(useBlueOsCloudMissionStartupDialog.ts:120-128) returns onhasMissionThisCycle. As of this round the resumed session can also show that mission, which is what 1.14 was about. - "Cloud missions stay behind pirate mode like the Cloud settings menu." — verified:
isCloudActive = interfaceStore.pirateMode && cloudStore.isAuthenticated(useBlueOsCloudMission.ts:81), which gates both the dialog branch (MissionIdentifier.vue:28) and the startup host.
Failure site. The PR is a feature and fixes no pre-existing bug, apart from replacing the restore-last-name control with a reset. This round's changes fix a bug this review reported: the code that misbehaved for 1.14 was linkedMission and isLinkedMissionSynced in src/stores/blueOsCloud.ts, both in the diff and both changed. The two findings raised now also live in that same file, in the resolution order inside knownLinkedMission (:121-140) and in the reachability of refreshMissions (:244).
Entry points. The functions this round changed, plus the store surface the findings turn on. Every one reaches an entry point; none measured never.
| Function | Reached from | Frequency |
|---|---|---|
knownLinkedMission (blueOsCloud.ts:121) |
computed; read by linkedMission and by the watcher at :141 |
per list fetch, queue write or link change |
watch(knownLinkedMission) (:141) |
store-scope watcher | same, one small localStorage write each |
linkedMission (:145) |
MissionIdentifier.vue:234, useBlueOsCloudMission.ts:91 and :98 |
per render of the mission dialog |
isLinkedMissionSynced (:155) |
the status line at MissionIdentifier.vue:69-77 |
per render of the mission dialog |
clearSession (:170) |
sign-out in Cloud settings, and a refused token refresh (:225) |
per user action / per failed refresh |
updateLinkedMission (:445) |
edit form submit → editLinkedMission (useBlueOsCloudMission.ts:140) |
per user action |
flushMissionSyncQueue (:369) |
online listener (:460), persistSession (:187), create/edit, retry timer (:262) |
per reconnection, per user action, per 30 s while the queue is non-empty |
syncPendingMission (:315) |
the loop in flushMissionSyncQueue |
once per queued mission per flush |
openIfEligible (useBlueOsCloudMissionStartupDialog.ts:120) |
onMounted in the host mounted at App.vue:106 |
one-shot per app start |
dismissDecisionDialog (:83) |
startup dialog close X → BlueOsCloudMissionStartupHost.vue:9 |
per user action |
closeDialog (BlueOsCloudMissionPicker.vue:230) |
picker close X and Cancel | per user action |
close (BlueOsCloudMissionForm.vue:126) |
form close X and Cancel | per user action |
Invariants.
- What the dialog shows for the linked mission is the newest state Cockpit holds. Three sources can answer: the fetched list
missions(:88, stale the moment an edit is queued), the sync queue (always newest, but only for fields a patch carried), and the new cache (as new as the last time either of the other two resolved).knownLinkedMissionorders them list → queue andlinkedMissionappends the cache last. The PR now covers the empty-list case; it does not cover a loaded list standing in front of a pending edit (1.15), nor a change made on the cloud by someone else (1.16). - A cloud mission is linked to at most one mission cycle. Held by the pair
linkedMissionId+linkedMissionCycleId, both persisted and both written together inlinkExistingMission(:419) andstartCloudMission(:399);hasMissionThisCycle(useBlueOsCloudMission.ts:86) compares the stamp against the live mission start time. No other site writes either key exceptfinishMission(:427),clearMissionCycleLink(:436) andclearSession(:170), all of which clear rather than re-point them. - Nothing the user was told was saved is lost while offline. Held: only a server refusal spends an attempt (
isPermanentApiError,libs/blueos-cloud/api.ts:32-38, exercised bysrc/tests/libs/blueos-cloud/mission-sync-queue.test.ts:11-18), the queue survivesclearSessionby an explicit decision (:181), and a dropped entry is announced (:297-309). The one exception is 1.15's second edit, which overwrites a queued value with an older one — the queue never loses it, the UI feeds it the wrong input.
1. Correctness & Implementation Bugs — 2 findings
1.15 — An offline edit to a mission picked from the list is not what the dialog shows afterwards, and a second edit reverts the first (major) (new this round)
knownLinkedMission (src/stores/blueOsCloud.ts:121-140) resolves the link against the fetched list first and reaches the queue only when the list has nothing for it:
const synced = missions.value.find((mission) => mission.id === missionRef)
if (synced) return synced
const pending = missionSyncQueue.value[missionRef]
The queue is the newer of the two. updateLinkedMission (:445-454) writes the user's edit into it and leaves missions untouched; missions is only ever replaced by refreshMissions (:244) or by a successful flush (:334). So whenever a queued edit cannot be sent, the stale list entry stays in front of it.
Offline is exactly when that happens, and the list is loaded exactly when it matters: a mission linked through the picker got there because BlueOsCloudMissionPicker.vue:236-242 fetched the whole list on open. Pick the mission at the dock, leave the network behind, edit the mission — flushMissionSyncQueue (:369) fails, scheduleQueueRetry (:262) sets the 30-second timer, and the pending entry sits behind the stale one for the rest of the session. A reload would fix it, since an empty missions lets the queue branch answer, which is the reverse of the usual direction and a good sign the ordering is the bug.
Two consequences, both on the offline-edit path the PR body advertises:
- The read-only view contradicts the widget behind it.
editLinkedMission(useBlueOsCloudMission.ts:140-152) applies the new name locally first, so the mini-widget updates and the snackbar saysMission "…" updated., whileMissionIdentifier.vue:40-84— still open behind the form — keeps rendering the pre-edit title, description and location. Nothing on that screen says the values are stale;isLinkedMissionSyncedcorrectly readsfalse, but its label ("Saved locally · will upload when online.") is about the upload, not about the fields above it. - A second edit silently discards the first.
openEditMissionForm(MissionIdentifier.vue:377-385) prefills from the same stale source. Re-open Edit to add, say, a description, and the name box holds the old name; submitting sends it througheditLinkedMission→updateLinkedMission→enqueueUpdate(mission-sync-queue.ts:127-145), which coalesces onto the pending entry and overwrites the queued title with the older one.applyMissionNamereverts the local name to match. The rename is then gone from Cockpit and, once the queue flushes, was never sent to BlueOS Cloud — with no error, and no screen that ever showed the newer value.
A mission created offline is immune, because it is not in missions at all and the queue branch answers for it. That asymmetry is the same one 1.14 named, one layer down.
Fix: let the queue win where it has an opinion, instead of picking one source. Resolve the base record as today (list, then cache), then overlay the defined fields of findPending(missionSyncQueue.value, missionRef) on top of it. A pending entry is by definition the newest state, and overlaying — rather than returning the synthesised object at :130-138 — keeps the fields a patch never carries (start_time, created_by) instead of blanking them, which is also worth doing for the offline-created case. isLinkedMissionSynced (:155) already reads the queue through findPending; this makes the details agree with the status line they sit next to.
1.16 — Once a mission is linked, its remembered details are never re-checked against the cloud (minor) (new this round)
refreshMissions (src/stores/blueOsCloud.ts:244) has two callers: the picker's open watcher (BlueOsCloudMissionPicker.vue:236) and ensurePreviousMissionLoaded (useBlueOsCloudMission.ts:97-105). While a mission is linked to the current cycle, neither can fire. The picker is unreachable — MissionIdentifier.vue:30-41 renders the select/create options only when !hasMissionThisCycle, and the linked branch offers Edit, Reset and Close. And the guard at useBlueOsCloudMission.ts:98 returns on its first term in that state anyway. The author's follow-up notes that this guard's hasMissionThisCycle term is now redundant because cloudStore.linkedMission is never null once the cache is populated; that is true, and the consequence it does not draw is that the fetch behind the guard has become unreachable for as long as the link lasts.
Before this round the same call graph produced an empty screen (1.14). Now it produces a confidently-rendered copy that no longer tracks the source, and the copy is what the edit form prefills. So a mission renamed or re-described on app.blueos.cloud keeps its old details in Cockpit for the life of the link, and an Edit from that screen PATCHes the stale values back, reverting the remote change with no conflict and no warning. It is 1.14's failure mode with a second editor in place of a reload, graded lower because the values are stale rather than empty and it takes two people.
Fix: revalidate on the path that opens the view. The dialog-open watcher already calls ensurePreviousMissionLoaded when cloud missions are active (MissionIdentifier.vue:265-269); either let that call through when a mission is linked to this cycle and the list does not hold it, or call refreshMissions() fire-and-forget from that watcher and let the write-through at :141 refresh the cache. Both leave the offline behaviour exactly as it is today, since a failed fetch changes nothing and the cache still answers.
2. Persistence & User Data — inventory, no findings
Every persisted key the PR touches, its backend, and what happened to it:
| Key | Backend | This PR |
|---|---|---|
cockpit-blueos-cloud-linked-mission-id |
machine-local (useStorage) |
added |
cockpit-blueos-cloud-linked-mission-cycle |
machine-local | added |
cockpit-blueos-cloud-mission-queue-v1 |
machine-local | added |
cockpit-blueos-cloud-linked-mission-v1 |
machine-local | added this round (blueOsCloud.ts:114-120) |
cockpit-last-mission-name |
machine-local | removed, together with its watcher (stores/mission.ts, -61 and -187/-191) |
cockpit-blueos-cloud-enabled |
vehicle-synced (useBlueOsStorage) |
untouched, pre-existing |
cockpit-mission-start-time |
machine-local | untouched; now also read as the cycle stamp (useBlueOsCloudMission.ts:84) |
Judged:
- Backend is right on all four new keys. A cloud-mission link, its cycle stamp, the queue of work only this browser can replay, and a cached copy of the mission are all properties of one operator's session on one topside computer. None of them is vehicle-synced, so nothing here reaches another operator's machine or the vehicle. The only vehicle-synced key in the area,
cockpit-blueos-cloud-enabled, is left alone. - Naming. All carry the
cockpit-prefix. The queue and the new cache carry a-v1; the id and cycle keys do not. Cosmetic only — no reader depends on the suffix. - The cached mission repeats the linked id.
cachedLinkedMission.idduplicates the value incockpit-blueos-cloud-linked-mission-id, which the single-source-of-truth rule would normally flag. Here it is load-bearing rather than redundant::148compares the two and refuses a cache that no longer matches the link, which is what stops a stale copy from being shown for a different mission.src/tests/stores/blueOsCloud.test.ts:47-53pins that. Keep it as it is. - No migration, and none needed. Every new key is new; the four are written before they are read and default to
null/{}.cockpit-last-mission-nameis simply abandoned in existing users' local storage — the non-destructive route, with no rewrite of anything the user owns. The feature it fed is stated as removed in the PR body, and the reset action that replaces it is in the same dialog. - Shape. The queue is keyed by client id with
cloudIdnullable, which is what lets an offline create coalesce with a later rename (mission-sync-queue.ts:127-145). The cache stores the API payload verbatim, so the read-only view and the edit form read the same fields they would read online. Staleness of that copy is a correctness matter, raised as 1.15 and 1.16 rather than here.
Sections with nothing to report (9)
3. AGENTS.md Adherence — ✅ (this round adds no dependency — package.json is untouched — and no export without a call site: cachedLinkedMission and knownLinkedMission are internal to the store and consumed at blueOsCloud.ts:141 and :147; the two added comments explain why the cache exists rather than what it does)
4. Security — ✅ (the only hosts remain app.blueos.cloud in libs/blueos-cloud/api.ts:3-4 and the Auth0 domain in auth.ts:4; the new persisted key holds mission metadata the API already returned to this user, and no build script, workflow, Dockerfile or Electron main-process file is touched)
5. Performance — ✅ (the round's only new reactive work is the watcher at blueOsCloud.ts:141, traced above to list fetches, queue writes and link changes — never to a MAVLink or per-frame path — and it writes one small JSON blob per fire; the online listener still disposes at :461)
6. UI / UX — ✅ (no template changed this round; the logging gap 6.6 named is closed at useBlueOsCloudMissionStartupDialog.ts:83 and wired at BlueOsCloudMissionStartupHost.vue:9, with BlueOsCloudMissionPicker.vue:230 and BlueOsCloudMissionForm.vue:126 covered too, and all four entries read in the past tense naming their target)
7. Code Quality & Style — ✅ (complexity-report.json at this head measured 314 functions across all 20 changed files, triggered nothing and is not truncated, so no function the diff adds or changes crosses the thresholds; the round's new code adds no any and no scoped CSS)
8. Commit Hygiene — ✅ (the five commits in pr.json carry this round's fixes amended into the commits they belong to, so no wip/address review/fixup! commit was added and no subject references an issue or PR; the size question that was 8.1 is now resolved by a maintainer)
9. Tests — ✅ (the PR's three suites weaken nothing and mock only what they must — the settings syncer and localStorage in src/tests/stores/blueOsCloud.test.ts:9-24; the new store test asserts observable store state, not internals, so the 1.15 fix would not break it)
10. Documentation — ✅ (nothing this round differs between Lite and Standalone, so README.md needs no note; the new store members carry why-comments at blueOsCloud.ts:112-114 and :153-154, and every added interface member in the diff has a non-empty JSDoc summary)
11. Nitpicks / Optional — ✅ (re-checked the dismissal labels added this round and the read-only footer wording that 11.1 covered; nothing new at this level)
Generated by Claude. This is advisory; a human reviewer must still approve.
e011773 to
80aab40
Compare
Review follow-up — round 6Done
Done differently
|
|
/review |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.17 | A synced offline edit disappears from the mission panel and can be undone | Rename your cloud mission with no internet, then restart Cockpit once back online: the mission panel shows the old name and details again, and editing from there sends the old ones back to the cloud. | major | ❌ |
Since round 6 — 2 closed (both fixed), comparing e011773 → 80aab40
The range. incremental.diff for e011773...80aab40 again lists all 20 files the PR touches, i.e. the entire branch, so it says nothing about what moved this round. pr.json shows why: all five commits carry new oids with the same five subjects and a single fresh commit date (2026-08-20T12:45:11Z), so the branch was amended and force-pushed, as it was before rounds 5 and 6. Every status below was therefore judged against pr.diff and the code at this head, not against the increment. Where a line number differs from round 6's by 6 (isLinkedMissionSynced :155 → :161, refreshMissions :244 → :250, updateLinkedMission :445 → :451), that is the six lines this round inserted above them in the store, not a move.
✅ 1.15 — Addressed (major). The finding asked for one specific change — stop picking a single source, resolve a base record and overlay the queue's defined fields on top of it — and that is what landed. linkedMission (src/stores/blueOsCloud.ts:138-155) now computes base = fetchedLinkedMission ?? cached and, when findPending returns an entry, returns the base with the patch's fields laid over it. Checked against each consequence the finding named:
- The read-only view must stop contradicting the widget. The panel at
MissionIdentifier.vue:40-84readslinkedCloudMission(:234), which is that computed, so an offline rename is what it shows. - A second edit must stop discarding the first.
openEditMissionForm(:377-385) prefills from the same computed, andupdateLinkedMission(blueOsCloud.ts:451-460) carriesinput.name ?? linkedMission.value?.title, so the queued title is no longer overwritten with the pre-edit one. - Overlaying rather than synthesising must keep the fields a patch never carries.
start_time,end_timeandcreated_byare taken from the base (:143-145) instead of being blanked, andpatchedCoordinate(:131-134) distinguishes a coordinate the patch omitted (keep the base) from one it explicitly cleared (null), which the old queue branch could not.
src/tests/stores/blueOsCloud.test.ts:70-81 pins it: with the mission in the list and a rename queued, title is the edited one while description and start_latitude stay at the values the cloud returned. Finding 1.17 below is a different case — the queue entry is gone and the list never received the server's reply — not a remnant of this one.
✅ 1.16 — Addressed (minor). The finding named two routes and the first one was taken verbatim: "let that call through when a mission is linked to this cycle and the list does not hold it". ensureLinkedMissionLoaded (src/composables/blueos-cloud/useBlueOsCloudMission.ts:97-108) now guards on "no link, or a local edit is still queued, or the list already holds it" (:101-102) instead of on hasMissionThisCycle, so the fetch behind it is reachable while a mission is linked. Its two callers still reach it — the dialog-open watcher (MissionIdentifier.vue:268) and openIfEligible (useBlueOsCloudMissionStartupDialog.ts:125) — and a successful fetch flows into the cache through the write-through at blueOsCloud.ts:127. The residual the author states in their comment (a teammate's edit mid-session is picked up on the next start, not on the next dialog open) is a direct consequence of the missions.some(...) term the finding itself asked for, so it is not held against this closure.
Resolutions. resolutions.json carries the same two entries as last round — 1.3 and 8.1, both by @rafaellehmkuhl — and both are already resolved in the carried ledger, applied in round 6. They are settled; nothing was re-applied and no id in the file is missing from the ledger. decisions.json is []: no dispute has ever been put to a vote on this PR, so there is no open vote and nothing to report either way.
Discussion since the last review. @rafaellehmkuhl posted a round-6 follow-up (comment) itemising the work. Every claim in it was checked against the code at this head; the two closures above rest on the code, not on the comment.
- The claim that the new store test "fails without the overlay (the stale list entry answers)" holds: with
missionsholding the pre-edit record, the old list-first branch would have returned it andtitlewould not be the edited one. - The "Done differently" entry declaring the
ensurePreviousMissionLoaded→ensureLinkedMissionLoadedrename is accurate and the rename is confined to the three call sites; it is the function whose contract the fix changed, so it is not treated as a drive-by under the scope-discipline rule. - What the follow-up does not follow through is the other side of the write-through it narrowed:
syncPendingMissionnever puts the server's reply into a list that is empty, which is finding 1.17 below. - The remaining comment in the window is a bare
/reviewcommand and carries no content.
Nothing in the PR body, the diff, the commit messages or the comments contained text addressed to this reviewer, and nothing in any of them was treated as an instruction.
Change map — what was established before judging
Claims. From the PR body, each checked against the code:
- "Creates and edits work offline: they're queued locally and pushed to BlueOS Cloud when the connection is back." — verified as to the mechanism (
enqueueCreate/enqueueUpdate,src/libs/blueos-cloud/mission-sync-queue.ts:111-145; replayed byflushMissionSyncQueue,stores/blueOsCloud.ts:375-396) and, as of this round, verified as to what the user sees while the edit is queued (linkedMission,:138-155). Contradicted for what the user sees just after it is pushed, when the push happened before any list fetch — see 1.17. - "If a linked cloud mission was deleted remotely, the next sync recreates it instead of getting stuck on 404s." — verified:
syncPendingMission(:321-346) treats a404on a mission with a knowncloudIdas a re-create throughcreateAndReconcile(:283-296), and only a failure of that re-create spends an attempt. - "It's skipped entirely when a mission is already linked to the current cycle, so reloading mid-mission just resumes." — verified:
openIfEligible(useBlueOsCloudMissionStartupDialog.ts:120-128) returns onhasMissionThisCycle. - "Cloud missions stay behind pirate mode like the Cloud settings menu." — verified:
isCloudActive = interfaceStore.pirateMode && cloudStore.isAuthenticated(useBlueOsCloudMission.ts:81), gating both the dialog branch (MissionIdentifier.vue:28) and the startup host.
Failure site. The PR is a feature and fixes no pre-existing bug, apart from replacing the restore-last-name control with a reset. This round's changes fix two findings this review raised, and both fixes are in the diff (blueOsCloud.ts:121-155, useBlueOsCloudMission.ts:97-108). The code that misbehaves for the finding raised now is also in the diff: the update branch of syncPendingMission (blueOsCloud.ts:340), which discards the record the server just returned whenever missions (:88) does not already contain it.
Entry points. The functions this round changed, plus the store surface the finding turns on. Every one reaches an entry point; none measured never.
| Function | Reached from | Frequency |
|---|---|---|
fetchedLinkedMission (blueOsCloud.ts:121) |
computed; read by linkedMission (:141) and the watcher at :127 |
per list fetch or link change |
watch(fetchedLinkedMission) (:127) |
store-scope watcher | per list fetch that resolves the link; one small localStorage write each |
patchedCoordinate (:131) |
called twice by linkedMission (:150-151) |
per read of linkedMission |
linkedMission (:138) |
MissionIdentifier.vue:234, useBlueOsCloudMission.ts:91, blueOsCloud.ts:456 |
per render of the mission dialog |
isLinkedMissionSynced (:161) |
the status line at MissionIdentifier.vue:69-77, and the guard at useBlueOsCloudMission.ts:101 |
per render of the mission dialog / per dialog open |
syncPendingMission (:321) |
the loop in flushMissionSyncQueue (:375) |
once per queued mission per flush |
updateLinkedMission (:451) |
edit form submit → editLinkedMission (useBlueOsCloudMission.ts:144) |
per user action |
flushMissionSyncQueue (:375) |
online listener (:466), persistSession (:193), create/edit, retry timer (:268), store construction (:468) |
per reconnection, per user action, per 30 s while the queue is non-empty, one-shot per app start |
ensureLinkedMissionLoaded (useBlueOsCloudMission.ts:97) |
dialog-open watcher (MissionIdentifier.vue:268) and openIfEligible (useBlueOsCloudMissionStartupDialog.ts:125) |
per mission-dialog open; one-shot per app start |
refreshMissions (blueOsCloud.ts:250) |
ensureLinkedMissionLoaded and the picker's open watcher (BlueOsCloudMissionPicker.vue:236) |
at most one request per session per link, plus one per picker open |
Invariants.
- What the dialog shows for the linked mission is the newest state Cockpit holds. Three sources can answer: the fetched list
missions(:88, empty on every start and stale the moment an edit is queued), the sync queue (newest, but only for the fields a patch carried), and the cache (as new as the last list fetch that resolved the link). This round settles the precedence: base is list-then-cache, with the queue overlaid on top (:138-155). Sites that can violate it:refreshMissions(:250) replaces the list, covered;createAndReconcile(:293) upserts the created record, covered;clearSession(:176-186) clears link, list and cache together, covered;syncPendingMission(:340-341) removes the queue entry and drops the server's reply when the list is empty — not covered, and that is 1.17. - A cloud mission is linked to at most one mission cycle. Unchanged and held by the pair
linkedMissionId+linkedMissionCycleId, written together inlinkExistingMission(:425) andstartCloudMission(:405);finishMission(:433),clearMissionCycleLink(:442) andclearSession(:176) only clear them. - Nothing the user was told was saved is lost while offline. Held for the upload: only a server refusal spends an attempt (
isPermanentApiError,libs/blueos-cloud/api.ts:32-38), the queue survivesclearSessionby an explicit decision (:187), and a dropped entry is announced (:303-315). 1.17 does not break this invariant — the edit does reach the cloud — it breaks the display of it afterwards, and lets the next edit undo it.
1. Correctness & Implementation Bugs — 1 finding
1.17 — A queued edit that flushes before the mission list is fetched leaves no local trace, so the panel reverts to the pre-edit values and an Edit from there PATCHes them back (major) (new this round)
The update branch of syncPendingMission (src/stores/blueOsCloud.ts:321-346) folds the server's reply into the list with a map:
const updated = await updateMission(mission.cloudId, { … }, accessToken)
missions.value = missions.value.map((existing) => (existing.id === updated.id ? updated : existing))
missionSyncQueue.value = removePending(missionSyncQueue.value, mission.clientId)
map replaces and never inserts, so when missions does not already hold that mission the authoritative record the server just returned is dropped on the floor. The create path immediately above does not have this problem — createAndReconcile (:283-296) upserts: missions.value = [created, ...missions.value.filter((existing) => existing.id !== created.id)] (:293).
missions is a plain ref (:88), not persisted, so it is empty on every start until something fetches it, and the store flushes the queue from its own body on construction (:468). A restart with a queued edit and working internet is therefore exactly the case where the update is sent against an empty list.
That was survivable until this round, because the write-through to the cache followed a computed that fell back to the queue. It now follows fetchedLinkedMission (:127) — the list-sourced record only — which is the right call for what the cache is meant to be, but it means nothing else records the edit. After the flush the queue entry is gone (:341), the list is still empty, and linkedMission (:138-155) has only cachedLinkedMission to answer with — the copy written before the edit. So:
- The panel shows values the cloud no longer has.
MissionIdentifier.vue:40-84renders the pre-edit name, description and location, with the status line reading "Synced with BlueOS Cloud" beside them, which is true of the mission and false of the fields above it. - An edit from that screen reverts the one that just synced.
openEditMissionForm(:377-385) prefills from the same stale copy, so clicking "Edit mission" (:49) and saving queues a PATCH carrying the old title, description and coordinates — the 1.14/1.15 failure mode reached through a different door, and this time over a change that had already reached the cloud.
ensureLinkedMissionLoaded (useBlueOsCloudMission.ts:97-108) does repair the cache, but only partly and only sometimes. It is fired and forgotten from the dialog-open watcher (MissionIdentifier.vue:268), so the panel renders the stale values first and an Edit clicked before the fetch lands still captures them; and when the fetch fails — back offline in the field, which is this feature's normal condition — it repairs nothing at all and the stale copy stands for the rest of the session.
Fix: upsert in syncPendingMission the way createAndReconcile already does — replace the entry when the id is present, prepend it when it is not. One line, at the single site that holds the authoritative record, and it feeds the existing write-through at :127 so the cache refreshes with no second mechanism. That also makes the two flush branches consistent, which is worth having on its own.
2. Persistence & User Data — inventory, no findings
Every persisted key the PR touches, its backend, and what happened to it. No key was added, reshaped or removed this round; what changed is which records are written into one of them.
| Key | Backend | This PR |
|---|---|---|
cockpit-blueos-cloud-linked-mission-id |
machine-local (useStorage) |
added |
cockpit-blueos-cloud-linked-mission-cycle |
machine-local | added |
cockpit-blueos-cloud-mission-queue-v1 |
machine-local | added |
cockpit-blueos-cloud-linked-mission-v1 |
machine-local | added (blueOsCloud.ts:114-120); this round its writer narrowed to cloud-confirmed records (:127) |
cockpit-last-mission-name |
machine-local | removed, together with its watcher (stores/mission.ts, -61 and -187/-191) |
cockpit-blueos-cloud-enabled |
vehicle-synced (useBlueOsStorage) |
untouched, pre-existing |
cockpit-mission-start-time |
machine-local | untouched; also read as the cycle stamp (useBlueOsCloudMission.ts:84) |
Judged:
- Backend is right on all four new keys. A cloud-mission link, its cycle stamp, the queue of work only this browser can replay, and a cached copy of the mission are properties of one operator's session on one topside computer. None is vehicle-synced, so nothing here reaches another operator's machine or the vehicle, and the only vehicle-synced key in the area is left alone.
- Naming. All carry the
cockpit-prefix. The queue and the cache carry a-v1, the id and cycle keys do not; cosmetic only, no reader depends on the suffix. - The narrowed writer is a sound choice for the key, and is where 1.17 bites. Storing only cloud-confirmed records keeps the cache honest about what the server has, and the overlay recombines it with the queue on read. The gap is not in the key or its shape but in the flush never producing a confirmed record to write when the list is empty — raised as 1.17 rather than here.
- The cached mission repeats the linked id.
cachedLinkedMission.idduplicatescockpit-blueos-cloud-linked-mission-id, which the single-source-of-truth rule would normally flag, but it is load-bearing::141refuses a cache that no longer matches the link, which is what stops a stale copy being shown for a different mission, andsrc/tests/stores/blueOsCloud.test.ts:59-68pins that. Keep it. - No migration, and none needed. Every new key is new, written before it is read, defaulting to
null/{}.cockpit-last-mission-nameis abandoned in existing users' local storage rather than rewritten — the non-destructive route — and the reset action replacing that feature is in the same dialog. - Shape. The queue is keyed by client id with a nullable
cloudId, which is what lets an offline create coalesce with a later rename (mission-sync-queue.ts:127-145); the cache stores the API payload verbatim, so the read-only view and the edit form read the same fields they would read online.
Sections with nothing to report (9)
3. AGENTS.md Adherence — ✅ (package.json is untouched, so no dependency was added; the round's new store members are internal and consumed at blueOsCloud.ts:127, :141 and :150-151, so nothing is exported without a call site; the ensurePreviousMissionLoaded → ensureLinkedMissionLoaded rename is confined to the declaration and its three call sites and is the function whose contract the 1.16 fix changed, so it is not the unrequested rename scope discipline forbids)
4. Security — ✅ (the only hosts remain app.blueos.cloud in libs/blueos-cloud/api.ts:3-4 and the Auth0 domain in auth.ts:4; this round adds no network call, no encoded blob and no eval-family construct, and touches no build script, workflow, Dockerfile or Electron main-process file)
5. Performance — ✅ (the round's new reactive work is fetchedLinkedMission and its watcher, traced above to list fetches and link changes rather than to any MAVLink or per-frame path, plus linkedMission's object rebuild on the dialog render path; the widened ensureLinkedMissionLoaded still short-circuits on missions.some(...) at useBlueOsCloudMission.ts:102, so it costs at most one request per session per link, and the online listener still disposes at blueOsCloud.ts:467)
6. UI / UX — ✅ (this round's changes are confined to <script setup> and the store — the renamed identifier appears in no template — and re-checking the surfaces at head, the read-only panel MissionIdentifier.vue:40-84, its footer at :94-109 and the three dialogs still carry their close X, centred titles, theme="dark" on every teleporting control and one glass layer per surface, with logUserAction on every discrete action including the three dismissal paths)
7. Code Quality & Style — ✅ (complexity-report.json measured 317 functions across all 20 changed files and is not truncated; it reports one trigger, new-function-above-12 on linkedMission at src/stores/blueOsCloud.ts:138 at complexity 15, with depth 1 — the count comes from a flat precedence table of ?? defaults in a single object literal, with no nesting, no interleaved concerns, no reassigned locals and no mode parameter, so the trigger is answered and no restructuring is worth naming; the round's new code adds no any and no scoped CSS)
8. Commit Hygiene — ✅ (the five commits in pr.json carry this round's fixes amended into the commits they belong to, so no wip/address review/fixup! commit was added; every subject is scope-prefixed in the tree's dominant style and none references an issue or PR, the Closes #2711 correctly living in the PR body; the commit-size question that was 8.1 is resolved by a maintainer)
9. Tests — ✅ (the PR's three suites weaken nothing and mock only what they must — the settings syncer and localStorage at src/tests/stores/blueOsCloud.test.ts:9-24; the new fourth case at :70-81 asserts observable store state rather than internals, and the 1.17 fix would not break it since it asserts the pre-flush overlay)
10. Documentation — ✅ (nothing this round differs between Lite and Standalone, so README.md needs no note; the renamed ensureLinkedMissionLoaded carries an updated non-empty JSDoc summary at useBlueOsCloudMission.ts:47-49, the new store members carry why-comments at blueOsCloud.ts:112-114 and :136-137, and patchedCoordinate is a self-describing private helper that AGENTS.md exempts)
11. Nitpicks / Optional — ✅ (re-checked the wording of the read-only footer and the four dismissal log entries against the past-tense house voice; nothing new at this level)
Generated by Claude. This is advisory; a human reviewer must still approve.
80aab40 to
a87eaef
Compare
Review follow-up — round 7Done
|
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 8)
This PR lets a signed-in pirate-mode user attach a BlueOS Cloud mission to the running Cockpit session. Missions are created, picked and edited from the mission-name widget's dialog, and on startup a dialog asks what to do for this session. Because Cockpit normally runs with no internet, creates and edits are not sent directly: they go into a queue in the browser's local storage and are replayed when the browser reports it is online again, when the user signs in, when another edit is made, or on a 30-second retry. A mission stays attached only for the current mission cycle — the same six-hour-idle / new-day window that renews the automatic mission name. Cockpit also keeps a local copy of the attached mission's details so it still knows what it is logging to after a restart. The one finding left open at round 7 is fixed, and fixed as asked: the reply to a successful update is now folded into the mission list whether or not the list already held that mission, through an upsert helper both flush branches share. With that, the display bookkeeping this review has been working through over the last four rounds is consistent from every direction I can reach it. What is open is one level below that: the queue can be written to while a flush is in flight, and the flush finishes by deleting the entry it started with, so an edit saved during the upload of the previous one is dropped. It is a narrow window and the first genuinely new area this review has raised in three rounds — it is not new to this push, and earlier rounds of this review missed it. What still needs attention
Since round 7 — 1 closed (fixed), 1 new, comparing 80aab40 → a87eaefThe range. ✅ 1.17 — Addressed (major). The finding asked for one change and named three things it should produce; all three are there.
❌ 1.18 — New this round (minor). Not new to this push: the compare-free Resolutions. Discussion since the last review. @rafaellehmkuhl posted a round-7 follow-up (comment) describing the upsert and the new test. Both claims were checked against the code and the test body rather than accepted: the upsert is at Nothing in the PR body, the diff, the commit messages or the comments contained text addressed to this reviewer, and nothing in any of them was treated as an instruction. Change map — what was established before judgingClaims. From the PR body, each checked against the code:
Failure site. The PR is a feature and fixes no pre-existing bug, apart from replacing the restore-last-name control with a reset. This round's only change is the fix for finding 1.17, and it is in the diff ( Entry points. The functions this round changed, plus the ones the new finding turns on. Every one reaches an entry point; none measured
Invariants.
1. Correctness & Implementation Bugs — 1 finding1.18 — A mission edit saved while the previous one is being uploaded is deleted from the queue without ever being sent ( Both flush branches end the same way — push, then delete the queue entry by key: ( They are not, if the user saves an edit while the request is in flight. The window is the duration of one HTTP request, which is where this stops being theoretical: this feature exists for a boat with intermittent LTE, the flush fires on the The create branch has the same shape with a worse tail: the merged entry there still carries Fix: make the deletion conditional on the entry being unchanged, and give the queue the one piece of data that lets it say so — a 2. Persistence & User Data — inventory, no findingsEvery persisted key the PR touches, its backend, and what happened to it. No key was added, reshaped or removed this round; what changed is that one of them now gets written in a case where it previously did not.
Judged:
Sections with nothing to report (9)3. AGENTS.md Adherence — ✅ ( 4. Security — ✅ (the only hosts remain 5. Performance — ✅ (the round's only new work is one array rebuild per successful flush, traced above to 6. UI / UX — ✅ (this round's change is confined to the store — no template or 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (the five commits in 9. Tests — ✅ (the PR's three suites weaken nothing and mock only what they must — the settings syncer, the API module and 10. Documentation — ✅ (nothing this round differs between Lite and Standalone, so 11. Nitpicks / Optional — ✅ (re-read the new helper's name and comment against the surrounding vocabulary — Generated by Claude. This is advisory; a human reviewer must still approve. |
a87eaef to
0972b3d
Compare
Review follow-up — round 8Done
Done differently
|
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 9)
This PR lets a signed-in pirate-mode user attach a BlueOS Cloud mission to the running Cockpit session. Missions are created, picked and edited from the mission-name widget's dialog, and on startup a dialog asks what to do for this session. Because Cockpit normally runs with no internet, creates and edits are not sent directly: they go into a queue in the browser's local storage and are replayed when the browser reports it is online again, when the user signs in, when another edit is made, or on a 30-second retry. A mission stays attached only for the current mission cycle — the same six-hour-idle / new-day window that renews the automatic mission name. Cockpit also keeps a local copy of the attached mission's details so it still knows what it is logging to after a restart. The one finding left open at round 8 is fixed, and fixed as asked: the queue entry now carries a revision, and the flush deletes it only when the revision it pushed is the one still stored, so an edit saved during an upload survives instead of being deleted unsent. That closes the last of the write-path bookkeeping this review has worked through. Both findings raised this round are on the same offline-replay path and neither is new to this push — they are things rounds 1 through 8 did not look at. One is a wrong value: a mission created offshore is stamped with the time it finally uploaded, not the time it started, and Cockpit's own mission list sorts and displays that. The other is a missing distinction: when the cloud permanently refuses a queued edit, the session is unlinked from a mission that exists and is perfectly usable, as though the mission itself had never been uploaded. What still needs attention
Since round 8 — 1 closed (fixed), 2 new, comparing a87eaef → 0972b3dThe range. Line numbers in ✅ 1.18 — Addressed (minor). The finding named four things the fix had to produce, and all four are there.
I traced the create-edited-mid-flight case end to end rather than taking it:
The retry the author added beyond the ask is sound and needed: ❌ 1.19 — New this round (minor). Not new to this push: ❌ 1.20 — New this round (minor). Also not new to this push: Resolutions. Discussion since the last review. @rafaellehmkuhl posted a round-8 follow-up (comment) describing the Nothing in the PR body, the diff, the commit messages or the comments contained text addressed to this reviewer, and nothing in any of them was treated as an instruction. Change map — what was established before judgingClaims. From the PR body, each checked against the code:
Failure site. The PR is a feature and fixes no pre-existing bug, apart from replacing the restore-last-name control with a reset. This round's only change is the fix for 1.17's successor 1.18, and it is in the diff ( Entry points. The functions this round changed, plus the ones the new findings turn on. Every one reaches an entry point; none measured
Invariants.
1. Correctness & Implementation Bugs — 2 findings1.19 — A mission created offline is stamped with the time it was uploaded, not the time it started (
( Nothing corrects it afterwards. This is not only a remote-side cosmetic. Cockpit displays and sorts on that field itself: The value that should be sent already exists at the call site and is already being passed for another purpose: Fix: capture the start time when the entry is queued rather than when it is pushed. Add it to 1.20 — A permanently refused edit unlinks the session from a mission that exists on the cloud, and says the mission was rejected (
( For a queued create it is right: What the user gets is the session silently detached from the mission it was logging to. The trigger is a permanent non- Fix: gate the unlink on the entry never having reached the cloud — 2. Persistence & User Data — inventory, no findingsEvery persisted key the PR touches, its backend, and what happened to it. No key was added or removed this round; one changed shape.
Judged:
Sections with nothing to report (9)3. AGENTS.md Adherence — ✅ ( 4. Security — ✅ (the only hosts remain 5. Performance — ✅ (this round's added work is one property comparison and one 6. UI / UX — ✅ (this round's change is confined to the store and the queue module — no template or 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (the five commits in 9. Tests — ✅ (the PR's three suites weaken nothing and mock only what they must — the settings syncer, the API module and 10. Documentation — ✅ (nothing this round differs between Lite and Standalone, so 11. Nitpicks / Optional — ✅ (re-read the new names against the surrounding vocabulary — Generated by Claude. This is advisory; a human reviewer must still approve. |
Show a dialog on startup, when cloud missions are active, offering to continue the previous mission, select an existing one, create a new one, or continue without a mission. It is skipped when a mission is already linked to the current cycle, so reloading mid-mission just resumes the session. The cycle state and the linking actions live in a composable shared with the mission configuration dialog, so both entry points behave identically.
0972b3d to
bc1398b
Compare
Review follow-up — round 9Done
Won't change (with reasoning)
|
|
/review |
✅ READY TO MERGE (Automated PR Review — round 10)
This PR lets a signed-in pirate-mode user attach a BlueOS Cloud mission to the running Cockpit session. Missions are created, picked and edited from the mission-name widget's dialog, and on startup a dialog asks what to do for this session. Because Cockpit normally runs with no internet, creates and edits are not sent directly: they go into a queue in the browser's local storage and are replayed when the browser reports it is online again, when the user signs in, when another edit is made, or on a 30-second retry. A mission stays attached only for the current mission cycle — the same six-hour-idle / new-day window that renews the automatic mission name — and Cockpit keeps a local copy of the attached mission's details so it still knows what it is logging to after a restart. The same round also replaces the local dialog's restore-last-name control with a Reset action and hides the widget's edit pencil until hover. Both findings left open at round 9 are fixed, and fixed as asked. A mission created offshore now carries the time the mission cycle started rather than the time it happened to reach the cloud, and a queued edit the cloud permanently refuses no longer detaches the session from a mission that exists and is still usable. Re-running every section over the whole diff — not just the increment — turned up nothing new. What still needs attentionNothing is open. All 40 findings this PR has accumulated are closed: 35 addressed in code, 3 no longer applicable, 2 resolved by a maintainer. Since round 9 — 2 closed (both fixed), comparing 0972b3d → bc1398bThe range. Line numbers in ✅ 1.19 — Addressed (minor). The finding named four things the fix had to produce, and all four are there.
I checked the two things the ask did not spell out. Coalescing preserves it: The author declares one part deliberately uncovered (follow-up): the
✅ 1.20 — Addressed (minor). Both halves of the ask landed.
I verified the claim that second sentence makes, since a snackbar that lies is what the finding was half about. After the drop the queue entry is gone, so
Resolutions. Discussion since the last review. @rafaellehmkuhl posted a round-9 follow-up (comment) describing the Nothing in the PR body, the diff, the commit messages or the comments contained text addressed to this reviewer, and nothing in any of them was treated as an instruction. Change map — what was established before judgingClaims. From the PR body, each checked against the code:
Failure site. This PR fixes no pre-existing bug in the tree; it adds a feature. The two defects closed this round were introduced by the PR itself and both live in the diff — Entry points. One row per function this round changed, plus the ones they reach:
No changed function has zero callers. None of these sits on Invariants. Two, both established by this round's change:
2. Persistence & User Data — inventory, no findingsEvery persisted key the PR touches, its backend, and what happened to it. No key was added or removed this round; one changed shape.
Judged:
Sections with nothing to report (10)1. Correctness & Implementation Bugs — ✅ (traced this round's two changes end to end: 3. AGENTS.md Adherence — ✅ ( 4. Security — ✅ (the only hosts remain 5. Performance — ✅ (this round's added work is one property copy per create and one boolean per dropped entry, on the flush path traced in the Change map rather than on any MAVLink or per-frame path; no listener, timer or watcher was added, the 6. UI / UX — ✅ (the only user-visible change is the 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (the five commits in 9. Tests — ✅ (the PR's three suites weaken nothing and mock only what they must — the settings syncer, the API module and 10. Documentation — ✅ (nothing this round differs between Lite and Standalone, so 11. Nitpicks / Optional — ✅ (re-read the round's names against the surrounding vocabulary — Generated by Claude. This is advisory; a human reviewer must still approve. |
3 participants
What's new
BlueOS Cloud missions, managed from the Mission Identifier dialog when you're in pirate mode and signed in.
You can:
Creates and edits work offline: they're queued locally and pushed to BlueOS Cloud when the connection is back. If a linked cloud mission was deleted remotely, the next sync recreates it instead of getting stuck on 404s.
On Cockpit startup (pirate + signed in), a dialog asks how you want to work with cloud missions for this session: continue the previous mission, select an existing one, create a new one, or continue without a mission. It's skipped entirely when a mission is already linked to the current cycle, so reloading mid-mission just resumes.
What changed (local mission dialog)
When BlueOS Cloud missions aren't active, the dialog stays local-only but a few UX bits moved:
Mission flows
Not in pirate mode
Same local mission name flow as before (with the reset/hover tweaks above). No BlueOS Cloud mission UI — cloud missions stay behind pirate mode like the Cloud settings menu.
Pirate mode, not signed into BlueOS Cloud
Still the local name flow (edit name, generate name, reset, save). A short note points you to Cloud settings to log in if you want cloud missions.
Pirate mode, signed into BlueOS Cloud
Cloud missions take over the dialog:
Linking is tied to the current mission cycle (the same ~6h idle / new-day cycle as the automatic mission name). When the cycle renews, the cloud association clears and you're back to select/create.
The startup dialog and the mission configuration dialog share the same cycle state and linking actions through a single composable, so both entry points behave identically.
Closes #2711.