feat: optionally unload views that are not on screen - #3037
feat: optionally unload views that are not on screen#3037rafaellehmkuhl wants to merge 7 commits into
Conversation
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.1 | Frozen gauge readings keep being recorded | With the option on, the mission log and the telemetry text burned into recorded video keep writing the last value of gauges from views you are not looking at, as if those readings were still live. | major | ❌ |
| 5.1 | Repeating screenshot capture cannot be stopped | With the option on, switching away from a view while a repeating screenshot capture is running leaves it taking pictures forever, with no button left to stop it. | major | ❌ |
| 1.2 | Two parts of the app can disagree on which view is current | If the stored "current view" number ever points past the end of the profile, turning the option on shows an empty screen instead of a view. | minor | ❌ |
Change map — what was established before judging
Claims
- "Every visible view is stacked and left running (maps, video, render loops)" — partly contradicted. Verified for video, maps and iframes:
src/views/WidgetsView.vue:3mounts one.widget-viewper entry ofviewsToShow,VideoPlayer.vueregisters a stream consumer unconditionally (its only visibility-aware code is the teardown atsrc/components/widgets/VideoPlayer.vue:316-320), andsrc/components/widgets/IFrame.vue:727-729only appendsdisplay: nonefor a widget that is not on the current view, so the embedded page keeps running. Contradicted for render loops: the canvas widgets already skip rendering when their view is not the current one —Attitude.vue:354,Compass.vue:241,CompassHUD.vue:743,DepthHUD.vue:302,VirtualHorizon.vue:258,Plotter.vue:550, all gating onwidgetStore.isWidgetVisible(src/stores/widgetManager.ts:344-346). The saving the feature actually delivers is video sessions, leaflet instances and iframes, not idle canvas work. - "
viewsToShownow mounts only the current view when the setting is on" — verified.src/stores/widgetManager.ts:308now delegates toselectViewsToShow(src/types/widgets.ts:1016), whoseunmountHiddenbranch returns[current]or[]. Both consumers arev-fors keyed per view (src/views/WidgetsView.vue:3,src/App.vue:60), so shrinking the array is a real unmount, not a hide. - "It stays off by default" and "the value is stored only on this computer" — verified.
src/stores/widgetManager.ts:65uses vueuseuseStoragewithfalse, notuseBlueOsStorage. - "Widgets on the view you left should start again when you come back" — verified as the intent; findings 1.1 and 5.1 are the two places where the leaving half of that is not clean.
Entry points
| Function | Reached from | Frequency |
|---|---|---|
selectViewsToShow (src/types/widgets.ts:1016) |
viewsToShow computed (src/stores/widgetManager.ts:308) → v-for at src/views/WidgetsView.vue:3 and src/App.vue:60; invalidated by currentViewIndex, currentProfile.views and the new setting |
per user action |
viewsToShow computed (src/stores/widgetManager.ts:308) |
the same two v-fors; no other reader in the tree |
per user action |
setUnmountHiddenViews (src/views/ConfigurationUIView.vue:211) |
@update:model-value on the v-switch at src/views/ConfigurationUIView.vue:161 |
per user action |
Invariants
The change relies on: a view dropping out of viewsToShow unmounts every widget and mini-widget under it, so anything that must outlive the view has to be owned outside the component. Every site that can violate it, and how it fares:
- Holds —
src/components/widgets/VideoPlayer.vue:316-320andsrc/components/mini-widgets/MiniVideoRecorder.vue:449-453release their stream consumer on unmount, anddeactivateStreamIfUnused(src/stores/video.ts:690-700) explicitly refuses to tear a stream down while a recorder is attached, so a recording started on the view you leave keeps running. - Holds —
src/components/widgets/Map.vue:1123-1125flushes the live map position before unmounting;useDataLakeVariableunlistens inonUnmounted(src/composables/useDataLakeVariable.ts:47-51). - Violated —
src/components/mini-widgets/VeryGenericIndicator.vueregisters into the datalogger and intoCurrentlyLoggedVariables(:479-487,:496-513) with no unmount teardown → finding 1.1. - Violated —
src/components/mini-widgets/SnapshotTool.vue:399-442owns twosetIntervalhandles and the file has no unmount hook → finding 5.1.
A second invariant the PR introduces: viewsToShow and currentView must agree on which view is current. currentView clamps the index (src/stores/widgetManager.ts:295-304); the new branch does not → finding 1.2.
1. Correctness & Implementation Bugs — 2 findings
1.1 — Unmounting a view freezes its indicator values in the recorded telemetry instead of dropping them · major
registerVeryGenericData (src/libs/sensors-logging.ts:494-502) appends to datalogger.veryGenericIndicators (:231) and there is no removal API anywhere on DataLogger. collectVeryGenericData (:381-391) then copies every entry into each log point and stamps it with the current timestamp, and that runs from the logging interval at :353 inside logRoutine (:324-372), independently of what is mounted. The only thing keeping an entry truthful is the mounted indicator's watcher on parsedState (src/components/mini-widgets/VeryGenericIndicator.vue:479-487) — and that component has no onBeforeUnmount.
Today this is nearly unreachable: src/App.vue:60 mounts the bottom bar of every visible view, so an indicator only unmounts when the user deletes it. With this setting on, every view switch unmounts the bars of the views left behind. The shipped ROV default profile puts four of these on view bottom bars — "Pilot Gain", "Lights (1)" and "Cam Tilt" (src/assets/defaults.ts:219-252) plus "Water Temp" (:266-277) — so switching from the video view to the map leaves the sensor log, and the telemetry overlay burned into recorded video, writing those four at their last-seen value with a fresh timestamp for the rest of the dive.
Fix it at the shared site rather than in the new code: give DataLogger a deregisterVeryGenericData(displayName) beside registerVeryGenericData (src/libs/sensors-logging.ts:494) and call it from an onBeforeUnmount in VeryGenericIndicator.vue. That also closes the deletion path, which today drops the variable from CurrentlyLoggedVariables (src/components/MiniWidgetContainer.vue:159) but leaves the datalogger entry behind.
The same onBeforeUnmount covers a second leak with the same cause: VeryGenericIndicator.vue:509 calls CurrentlyLoggedVariables.addVariable on every mount, and the instance counter it increments (src/libs/sensors-logging.ts:182-191) is only decremented on explicit deletion (src/components/MiniWidgetContainer.vue:159). With the setting on, each return to a view adds one more instance, so after a few switches deleting the widget can no longer remove its variable from the list in Settings → Logs.
1.2 — The new branch indexes views without the clamp currentView applies · minor
currentView (src/stores/widgetManager.ts:295-304) deliberately clamps currentViewIndex into range before indexing, because cockpit-current-view-index-v1 is vehicle-synced (:64) and the views group is a separate synced key (:63) — the two do not arrive atomically, so the index can transiently point past the end of the local profile. selectViewsToShow's new branch (src/types/widgets.ts:1017-1019) indexes raw: views[currentIndex] is undefined, current?.visible is falsy, and it returns []. The app then mounts no view at all while currentBottomBarHeightPixels (:282-284) and miniWidgetContainersInCurrentView (:313-321) still describe the clamped view.
The pre-existing path is not better — with the setting off, views[5] pushed into the array makes .filter((v) => v.visible) throw inside the computed — but one line covers both branches: derive const index = Math.max(0, Math.min(currentIndex, views.length - 1)) at the top of selectViewsToShow and use it throughout, matching what currentView already does.
2. Persistence & User Data — inventory, no findings
| Key | Backend | Change |
|---|---|---|
cockpit-unmount-hidden-views |
machine-local — vueuse useStorage (src/stores/widgetManager.ts:65) |
added |
Judged:
- Backend is right. A performance trade is a property of the topside computer, not of the vehicle, so it must not travel to other operators through
useBlueOsStorage. The same call shape is the established machine-local pattern here —cockpit-prevent-auto-vehicle-discovery-dialog(src/App.vue:378),cockpit-base-station-track-by-gps(src/composables/baseStation/useBaseStation.ts:35),cockpit-vehicle-address(src/stores/mainVehicle.ts:90). The key is still carried by the settings backup, which copies every localStorage key bar four (src/libs/settings-management.ts:991-992). - Key naming starts with
cockpit-, as AGENTS.md requires. - Shape is a bare boolean with no field duplicating the key, no nested state, no reshape of an existing key and no automatic migration.
- Default is
false, so no already-configured user is moved onto new behaviour and nothing needs to carry them over.
5. Performance — 1 finding
5.1 — Timed snapshot capture keeps firing after its widget is unmounted · major
src/components/mini-widgets/SnapshotTool.vue:399-400 holds shotInterval and progressInterval in module-closure lets, and both are created and destroyed only by the watcher on the component-local isTakingTimedSnapshot ref (:243, watcher at :407-442). The file registers no onBeforeUnmount at all — its last hook is onMounted at :465-469 — which is the missing teardown the Cleanup rule asks for.
Until now the widget only unmounted on deletion, so the gap was latent. With this setting on, switching away from the view holding it unmounts it mid-capture: the watcher's stop branch never runs, both intervals survive, and fireTimedSnapshot (:402-405) keeps calling captureSnapshot (:274-) every N seconds — canvas capture plus a write per source — with the stop button gone from the screen. Returning to the view remounts the widget with isTakingTimedSnapshot back at false (:243), so the UI offers "start" while a loop is already running, and pressing it allocates a second pair of intervals. Nothing short of reloading Cockpit stops it.
This is also the "automatic heavy work" case: after the unmount the capture is no longer attached to anything the user did or can undo.
Fix at the real site, not in this PR's new code:
onBeforeUnmount(() => {
if (shotInterval) clearInterval(shotInterval)
if (progressInterval) clearInterval(progressInterval)
})Sections with nothing to report (8)
3. AGENTS.md Adherence — ✅ (JSDoc on selectViewsToShow carries a real summary and typed, non-empty @param/@returns; the helper sits beside fillMissingBarContainers and the validate* functions already living in src/types/widgets.ts and has its call site in this same PR; the no-bottom-divider move at src/views/ConfigurationUIView.vue:122 is forced by the new panel becoming the last one, not a drive-by edit; no dependency added, package.json untouched)
4. Security — ✅ (all nine sub-checks run across the four changed files: no encoded blobs or binary-like strings, no hidden or bidirectional Unicode, no network call, no change under scripts/, .github/ or src/electron/, no new package, no eval/Function()/v-html, no secret or environment variable; pr.json, pr.diff and complexity-report.json contain no text addressed to the reviewer)
6. UI / UX — ✅ (the switch reuses the exact token set already in the tree at src/views/ConfigurationMissionView.vue:10-17 — color="white", base-color="#FFFFFF33", hide-details, mt-2 -mb-2 ml-3 inside the same px-4 pt-5 column the sibling panels use, so no new inset is stacked; panel title and label are sentence case and the label does not restate the panel; the setting is grouped in an ExpansiblePanel per the space-economy rule; no overlay-teleporting control is added, so theme="dark" does not apply; logUserAction is called from the handler in past tense naming the target, not from a watcher a BlueOS sync could retrigger; the #info slot renders through the existing info affordance at src/components/ExpansiblePanel.vue:51-55,81 and warns that widgets restart)
7. Code Quality & Style — ✅ (complexity-report.json reports 157 functions measured over the 4 changed files with triggered empty and truncated false, so no function's complexity or depth was raised past the thresholds; no any, no new scoped CSS duplicating a Tailwind utility, no existing comment deleted or reworded, setUnmountHiddenViews(value: unknown) matches the setDistanceUnit shape two functions above it, and the largest recipient — src/types/widgets.ts — gains 18 lines)
8. Commit Hygiene — ✅ (one commit, feat: matching a feature addition and consistent with the feat:/scope-prefixed mix in recent history; the body states the why; no #N, owner/repo#N or closing keyword in the message — Closes #3036 stays in the PR body; nothing to squash, split or revert)
9. Tests — ✅ (four cases added to src/tests/types/widgets.test.ts, none removed or weakened; the view() factory supplies all six required fields of the View type at src/types/widgets.ts:823-849, and the cases cover both branches plus the hidden-current-view edge)
10. Documentation — ✅ (nothing in the changed code touches window.electronAPI, isElectron() or any Electron-only module, so the setting behaves identically in Lite and Standalone and the README's parity table needs no entry)
11. Nitpicks / Optional — ✅ (nothing worth raising)
Generated by Claude. This is advisory; a human reviewer must still approve.
6862d80 to
8e4e4c4
Compare
Review follow-up — round 1Done
|
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 2)
Cockpit keeps every visible view of a profile built and running at once, stacking the one you are looking at on top. This PR adds an opt-in switch, stored only on the computer you set it on and off by default, that builds only the view on screen; leaving a view then destroys everything on it and coming back builds it again. This round the author also fixed the two teardown bugs the last review found — a repeating screenshot capture that could not be stopped, and gauge readings that kept being written to the dive log after their gauge was gone — and made the view-picking rule tolerate an out-of-range view number. What is left is smaller: one more widget that leaves something behind when it is destroyed, one gauge case the new clean-up keys wrongly, a capture that now ends without telling anyone, and three bug fixes buried inside the feature's single commit. What still needs attention
Since round 1 — 3 closed, comparing 6862d80 → 8e4e4c4The range. ✅ 1.1 — Addressed. ✅ 1.2 — Addressed. ✅ 5.1 — Addressed. Discussion. @rafaellehmkuhl posted a follow-up listing the three fixes as done. Treating it as a claim rather than evidence, I checked each against the diff: all three hold, as recorded above. The second comment is a bare Change map — what was established before judgingClaims
Failure site The feature itself fixes no bug, but the three changes folded in with it do, and all three are fixed at the site that misbehaves rather than around it: Entry points
Every changed function fires per user action, so none of them is on a hot path. The multiplier is on the other side of the registry the two new hooks edit: Invariants
1. Correctness & Implementation Bugs — 1 finding1.3 — The new deregistration is keyed on the display name, so it drops an entry a second indicator still owns ·
That is a shape the codebase already expects: The guard Fix: make the two sides agree on the key. Either count instances in 2. Persistence & User Data — inventory, no findings
Judged:
5. Performance — 1 finding5.2 —
Today a custom widget unmounts only when it is deleted, its view is hidden, or the profile changes, so one orphan handler is the practical ceiling. With the setting on, every switch away from a view holding a custom widget leaks another listener and, through its closure, the whole setup scope of the discarded component — so the toggle a user reaches for to lower memory pressure raises it a little on each switch, which is the opposite of the trade the panel offers them. The handler is inert once unmounted ( Fix at the real site, in the form onBeforeUnmount(() => window.removeEventListener('resize', updateWrapDirection))6. UI / UX — 1 finding6.1 — A running timed snapshot capture is now cancelled by a view switch without telling anyone · The 5.1 fix is right about the intervals, but it makes the capture end in silence. Every other way that loop ends announces itself: the watcher's stop branch opens "Timed snapshot stopped." ( Either of the first two closes it; the third goes with whichever is chosen:
8. Commit Hygiene — 1 finding8.1 — Three independent bug fixes squashed into the feature commit · Per
Sections with nothing to report (6)3. AGENTS.md Adherence — ✅ (JSDoc on both added public functions carries a real summary and typed, non-empty 4. Security — ✅ (all nine sub-checks run over the seven changed files: no encoded blob or binary-like string, no hidden or bidirectional Unicode, no network call, nothing under 7. Code Quality & Style — ✅ (the complexity report for this head states 281 functions measured across 7 changed files with 9. Tests — ✅ (five cases now in 10. Documentation — ✅ (nothing in the changed code touches 11. Nitpicks / Optional — ✅ (nothing worth raising beyond the findings above) Generated by Claude. This is advisory; a human reviewer must still approve. |
8e4e4c4 to
8fabf57
Compare
Review follow-up — round 2Done
|
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 3)
Cockpit keeps every visible view of a profile built and running at once, stacking the one you are looking at on top. This PR adds an opt-in switch, stored only on the computer you set it on and off by default, that builds only the view on screen; leaving a view then destroys everything on it and coming back builds it again. Because that turns every view switch into a teardown, the branch also fixes three things that were never released when a widget went away: a repeating screenshot capture, gauge readings written into the dive log, and a window listener on custom widget containers. This round the author fixed the container listener, made the cancelled screenshot capture announce itself, and split the three fixes into their own commits. What is left is one gauge case the clean-up still keys wrongly and one more widget that leaves a data feed behind. What still needs attention
Since round 2 — 3 closed, 1 partly, comparing 8e4e4c4 → 8fabf57The range. 🟡 1.3 — Partially addressed. The finding asked for the two sides of the registry to agree on their key, and named two ways to do it. The author took the first: ✅ 5.2 — Addressed. ✅ 6.1 — Addressed. All three sub-items landed. ✅ 8.1 — Addressed. Discussion. @rafaellehmkuhl left a follow-up ( Change map — what was established before judgingClaims
Failure site The feature fixes no bug; the three commits folded around it do, and all three are fixed at the site that misbehaves rather than around it: Entry points
Every changed function fires per user action, so none of them is on a hot path. The multipliers are on the far side of the two registries the new hooks edit: Invariants
1. Correctness & Implementation Bugs — 1 finding1.3 — The owner count is retained under the mount-time display name and released under the unmount-time one · The retain count is the right mechanism and closes the case round 2 led with: two gauges that both carry What remains is that the two calls read the name at different times, and the name is not stable.
Fix: release the key that was retained, rather than the name that happens to be current. The component already keeps 2. Persistence & User Data — inventory, no findings
Judged:
5. Performance — 1 finding5.3 — The iframe widget's external-API data-lake listeners are never released, so each view switch leaks one per subscribed variable ·
This is not a hypothetical path: it is the shipped extension API, whose client half posts that exact message ( Fix at the real site, in the shape Sections with nothing to report (8)3. AGENTS.md Adherence — ✅ (both new 4. Security — ✅ (all nine sub-checks run over the eight changed files: no encoded blob or binary-like string, no hidden or bidirectional Unicode, no network call, nothing under 6. UI / UX — ✅ (the cancelled capture now announces itself through the existing snackbar, closing 6.1; the 7. Code Quality & Style — ✅ (the complexity report for this head states 320 functions measured across 8 changed files with 8. Commit Hygiene — ✅ (the four commits in 9. Tests — ✅ (the five 10. Documentation — ✅ (nothing in the changed code touches 11. Nitpicks / Optional — ✅ (the only taste point is the Generated by Claude. This is advisory; a human reviewer must still approve. |
8fabf57 to
85ce744
Compare
Review follow-up — round 3Done
Done differently
Won't change (with reasoning)
|
|
/review |
✅ READY TO MERGE (Automated PR Review — round 4)
Cockpit keeps every visible view of a profile built and running at once, stacking the one you are looking at on top. This PR adds an opt-in switch, stored only on the computer you set it on and off by default, that builds only the view on screen; leaving a view then destroys everything on it and coming back builds it again. Because that turns every view switch into a teardown, the branch also fixes four things that were never released when a widget went away: a repeating screenshot capture, gauge readings written into the dive log, a window listener on custom widget containers, and the data feeds an embedded extension page subscribes to. This round the author made the gauge clean-up track the name it actually claimed, so renaming a gauge no longer disturbs another one, and released the extension feeds on unmount. What still needs attentionNothing. Both findings carried into this round are addressed, and re-running the sections over the whole diff raised nothing new. Since round 3 — 2 closed, comparing 8fabf57 → 85ce744The range. ✅ 1.3 — Addressed. The finding asked for one thing with two halves: release the key that was actually retained, and keep the two sides in step while the name changes. Both landed. ✅ 5.3 — Addressed. Discussion. @rafaellehmkuhl left a follow-up ( Change map — what was established before judgingClaims
Failure site The feature fixes no bug; the four commits folded around it do, and all four are fixed at the site that misbehaves rather than around it: Entry points
Only Invariants
2. Persistence & User Data — inventory, no findings
Judged:
Sections with nothing to report (10)1. Correctness & Implementation Bugs — ✅ (traced the retain/release pair through mount, rename, repeated renames while the name field is typed into, and unmount, and through all three writers of 3. AGENTS.md Adherence — ✅ (both 4. Security — ✅ (all nine sub-checks run over the nine changed files: no encoded blob or binary-like string, no hidden or bidirectional Unicode, no network call, nothing under 5. Performance — ✅ (every registration the diff adds or touches has a matching teardown, checked per call site under invariant 1 rather than per file; 6. UI / UX — ✅ (unchanged this round; the 7. Code Quality & Style — ✅ (the complexity report for this head — 8. Commit Hygiene — ✅ (the five commits in 9. Tests — ✅ (the five 10. Documentation — ✅ (nothing in the changed code touches 11. Nitpicks / Optional — ✅ (nothing left; Generated by Claude. This is advisory; a human reviewer must still approve. |
The capture intervals lived only in the start/stop watcher, so deleting the widget or leaving its view left them firing with no way to stop.
A VeryGenericIndicator left its last reading in every later log point after it was removed. Keep a retain count so two gauges that share a name stay in the log until the last one is gone.
The window resize handler was never removed, so each discarded custom widget left a listener and its setup scope behind.
The external API answers a subscription message by keeping only the callback, so every unmount left one listener per subscribed variable running.
85ce744 to
7e6cb39
Compare
There was a problem hiding this comment.
Some UI fixes needed and one UX suggestion:
(1) Reduce the top margin in 10px. It should be 50px in total, as the other content inside collapsible panels;
(2) Add a 'Options' menu item to the View selector mini widget with one of these two behaviors:
*1- A regular config modal that has the same 'Unload hidden views' switch;
*2- A shortcut to the 'Interface configuration menu page', that will allow the user to find and change that option. (My suggestion is *1).
This is a very useful feature and its hidden inside the Main menu -> Settings -> Interface -> Performance. I think many people would not be able to find it.
|
Two more things, all when 'unload views mode' is enabled': : 1- When switching views, it most of the times quickly switches back to last view. Probably a settings sync issue 2- Is the map loading normally to you? |
This is a problem that happens to me from time to time, not specifically from this PR. |
7e6cb39 to
25a742c
Compare
Review follow-up — round 4Done
|
|
/review |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 5.4 | Which view is loaded follows a setting the vehicle can overwrite | With the option on, a stray settings sync can throw you back to the view you just left — and now it destroys and rebuilds both views instead of just re-stacking them, so the video reconnects and the map reloads for nothing. | major | ❌ |
| 2.1 | Leaving the map now writes to the vehicle every time | Every glance away from the map view saves your map position onto the vehicle, where it becomes the position every other operator's map opens at. | minor | ❌ |
| 5.5 | The battery indicator's cleanup never runs | A battery indicator placed on a view leaves a timer running forever each time you switch away from that view, so the app slowly accumulates dead work — the opposite of what this option is for. | minor | ❌ |
| 6.2 | The new pop-up indents its contents twice | The switch and its explanation sit further in from the edge than the pop-up's own title, so the small window looks lopsided. | minor | ❌ |
| 7.1 | The same switch is written out twice | The toggle and its explanation now exist in two files, so a later wording or behaviour change will be made in one and forgotten in the other. | minor | ❌ |
| 11.1 | The description names only one place to find the setting | Anyone reading the pull request will not learn that the setting is also reachable from the view selector. | nit | ❌ |
Since round 4 — 0 closed, comparing 85ce744 → 25a742c
Range. incremental.diff is not usable this round and I did not use it. The branch was rewritten — every commit in pr.json carries a fresh commit date and 85ce744 is no longer reachable — and the file that came back lists 56 files, among them src/components/widgets/MiniMap.vue, src/components/BaseStationGnssSetupDialog.vue and src/libs/sensors/gnss.ts, none of which are among this PR's 10 files. That is a rebase onto a newer master showing through the compare, not the author's edits. Every status and every finding below was judged against pr.diff and against the PR's own base, 72c3cc2 (the revision complexity-report.json names as base); where a finding rests on a file the PR does not touch I re-read that file at 72c3cc2 rather than at the checkout's master tip.
What the push changed. Reading pr.diff against the previous round, three edits: the mt-2 dropped from the switch in src/views/ConfigurationUIView.vue:467; src/types/widgets.ts:404 flipping ViewSelector to configurable; and src/components/mini-widgets/ViewSelector.vue gaining a miniWidget prop and a config dialog carrying the same toggle (+50/−1).
Status changes. None. All eight carried findings were addressed at round 4 and stay closed; nothing this push touched reopens any of them.
Findings that are not new code. 5.4, 5.5 and 2.1 all describe the option's consequences rather than this push. 5.4 and 5.5 were reachable at round 4 and my enumeration then missed them — round 4 checked whether each widget file has a teardown hook and whether the four fixed sites were complete; it did not check whether the hooks that already existed do anything, nor what the mount set follows. 2.1 lives in src/components/widgets/Map.vue at the PR's base; I verified the code is identical at 72c3cc2, but because the branch has been rebased I cannot say whether it was reachable at round 4 and I am not claiming it was.
Resolutions and decisions. resolutions.json and decisions.json are both empty: no /resolve has been issued on this PR and no dispute has ever been put to a vote. Nothing to apply, and no id to report back as unmatched.
Discussion since round 4.
- ArturoManzoli reported two things, both with the option on (comment): that switching views "most of the times quickly switches back to last view. Probably a settings sync issue", and that the map comes back rendered wrong after a trip to the video or HUD view, with a screenshot. rafaellehmkuhl answered that this "is a problem that happens to me from time to time, not specifically from this PR" (comment), and the push that followed addressed neither. I treated both as claims and checked them.
- On the first: the mechanism is real and I found it.
currentViewIndexis vehicle-synced (src/stores/widgetManager.ts:63), local writes to a synced ref are held for 3 s before being pushed (src/composables/settingsSyncer.ts:11,:107), and the listener at:130will write a value it receives in that window straight back into the ref. So for three seconds after you pick a view, any settings-manager notification for that key replays the old index and the view flips back. The author is right that this is not caused by this PR — but it is now the input to what gets mounted, which is finding 5.4. His comment answers the cause; it does not answer the cost, so I have raised 5.4 as open rather than disputed. If he means it is also not worth fixing here, that is a different argument and he should say so. - On the second I found nothing, and I am reporting that rather than inventing a cause. Leaflet teardown on unmount is complete (
Map.vue:1131callsmap.remove()); the container the map initialises into already has its final geometry at mount (.widget-viewisposition: absoluteat 100%/100% insrc/views/WidgetsView.vue:97-106, andWidgetHugger.vue's only transition is on opacity, so there is no size animation racing the leaflet init); and the center/zoom round-trip throughmissionStore.userLastMapZoom/userLastMapCenter(Map.vue:353-354,:1112) restores the same values it saved. Nothing in the diff or in its blast radius explains the screenshot. I cannot run the head code, so this needs a human repro before merge — the author's own test plan expects "the map reloads" on this path, and whether it reloads correctly is exactly what is in question. - rafaellehmkuhl's follow-up (comment) lists two items numbered 1 and 2 that do not correspond to anything in the visible thread; ArturoManzoli's "Two more things" implies an earlier list I do not have. I checked both claims in it against the diff and both hold: the
mt-2is gone fromConfigurationUIView.vue:467, and the Options dialog exists and is bound to the same machine-local key. I am not grading the view-selector dialog as unrequested scope, since the evidence is that it was asked for.
Change map — what was established before judging
Claims.
- Four teardowns "already wrong on delete" — verified, each in the diff:
stopTimedSnapshotIntervalsplus anonBeforeUnmountatsrc/components/mini-widgets/SnapshotTool.vue:407,477; retain/release around the gauge log name atsrc/components/mini-widgets/VeryGenericIndicator.vue:82,92,108,111andsrc/libs/sensors-logging.ts:270,280; the resize listener atsrc/components/widgets/CollapsibleContainer.vue:211; the iframe listener ids atsrc/components/widgets/IFrame.vue:224,236,244. - "A machine-local setting, off by default" — verified.
useStorage('cockpit-unmount-hidden-views', false)atsrc/stores/widgetManager.ts:65is vueuse-over-localStorage, notuseBlueOsStorage, so it does not reach the vehicle; thecockpit-prefix on auseStoragekey matchessrc/stores/mission.ts:54andsrc/stores/mainVehicle.ts:90. - "Mounts only the current view" — verified at
src/types/widgets.ts:419-430, including thevisiblecheck on the current view and the index clamp. - "Toggle: Settings → Interface → Performance" — incomplete. There is now a second toggle, in the view selector's Options dialog (
src/components/mini-widgets/ViewSelector.vue:144-152), which the body does not mention. Finding 11.1. - "
yarn lintandyarn test:unitclean" — not verifiable here; I do not execute head code.complexity-report.json(head25a742c, base72c3cc2) reports 375 functions measured across 10 changed files,triggered: 0, not truncated.
Failure site. For the four fix: commits the misbehaving code is in the diff, at the lines above. For finding 5.5 it is not: the no-op teardown lives at src/components/mini-widgets/BatteryIndicator.vue:364-367, which this PR does not touch, and the smaller fix belongs there. For 5.4 the replay that moves the index lives at src/composables/settingsSyncer.ts:109-144, also outside the diff; what is inside the diff is the decision to mount off that value.
Entry points.
| Function | Reached from | Frequency |
|---|---|---|
stopTimedSnapshotIntervals (SnapshotTool.vue:407) |
watch(isTakingTimedSnapshot) and the new onBeforeUnmount |
per user action |
onBeforeUnmount (SnapshotTool.vue:477) |
Vue unmount — widget delete, profile change, or a view switch with the option on | per user action |
watch(options.displayName) (VeryGenericIndicator.vue:92) |
renaming the gauge in its config | per user action |
onMounted / onBeforeUnmount (VeryGenericIndicator.vue:108,111) |
Vue mount and unmount | one-shot per instance / per user action |
DataLogger.retainVeryGenericData (sensors-logging.ts:270) |
those two hooks only | per user action |
DataLogger.releaseVeryGenericData (sensors-logging.ts:280) |
those two hooks and the rename watcher | per user action |
onBeforeUnmount (CollapsibleContainer.vue:211) |
Vue unmount | per user action |
apiEventCallback (IFrame.vue:226) |
window message listener from embedded content |
per incoming message |
onBeforeUnmount (IFrame.vue:242) |
Vue unmount | per user action |
selectViewsToShow (types/widgets.ts:419) |
the viewsToShow computed (widgetManager.ts:308) → WidgetsView.vue:3 and the bottom bars at App.vue:60 |
per user action, and on any write to the synced cockpit-current-view-index-v1 |
setUnmountHiddenViews (ConfigurationUIView.vue:211) |
the settings switch @update:model-value |
per user action |
setUnmountHiddenViews (ViewSelector.vue:187) |
the new dialog's switch @update:model-value |
per user action |
onSelectView (ViewSelector.vue:182) |
the view dropdown | per user action |
The last row of selectViewsToShow is the multiplier the diff does not show and the reason for 5.4: with the option on, that computed decides what exists, and one of its inputs is a value the vehicle can move.
Invariants.
- A widget releases on unmount everything it acquired. This is the PR's premise, and the four fixes are four sites of it. I enumerated the other side rather than trusting the count: all 12 files under
src/components/widgets/andsrc/components/mini-widgets/that register a teardown hook, read by body. Eleven release cleanly.BatteryIndicator.vue:364-367has a hook whose guard is inverted, so it releases nothing — finding 5.5. The invariant is closed at N producers with nothing enforcing it, which is why a twelfth site was there to find. - The converse, which the PR does not establish: unmount must not itself do expensive or externally-visible work, because unmount is now per view switch rather than per widget deletion. Same enumeration: every other teardown body is a pure release (
clearInterval,removeEventListener,unlisten*,unregisterStreamConsumer).Map.vue:1112is the one that writes — finding 2.1. - With the option on, exactly one view is mounted and it is the one the user chose. This needs
currentViewIndexto be locally authoritative. It is not: it is vehicle-synced and its local writes are replayable for 3 s. Finding 5.4.
2. Persistence & User Data — inventory, 1 finding
Inventory.
| Key | Backend | What happened |
|---|---|---|
cockpit-unmount-hidden-views |
machine-local — vueuse useStorage over localStorage (src/stores/widgetManager.ts:65) |
added; boolean, default false |
cockpit-current-view-index-v1 |
vehicle-synced — useBlueOsStorage (src/stores/widgetManager.ts:63) |
not modified; the PR gives it a new role — it now decides what is mounted, not only what is on top (see 5.4) |
cockpit-user-last-map-center |
vehicle-synced — useBlueOsStorage (src/stores/mission.ts:66) |
not modified; written far more often (see 2.1) |
cockpit-user-last-map-zoom |
vehicle-synced — useBlueOsStorage (src/stores/mission.ts:67) |
not modified; written far more often (see 2.1) |
Judging the added key: machine-local is the right backend — whether a topside computer is slow enough to need this is a property of that computer, and the PR's own test plan checks the value does not appear on a second topside. The cockpit- prefix is present, as AGENTS.md requires, and useStorage with such a prefix is the established form for machine-local values in this tree (mission.ts:54, mainVehicle.ts:90, useBaseStation.ts:35). The stored shape is a bare boolean that does not repeat its key. No migration, no default change stranding anyone: existing users get false, which is today's behaviour. Nothing here needs a cockpit-foo-v2.
2.1 minor — Leaving the Map view now pushes the map position to the vehicle on every view switch.
src/components/widgets/Map.vue:1110-1112 persists the live map view from onBeforeUnmount, into missionStore.saveLastMapPosition (src/stores/mission.ts:531-534), which writes userLastMapCenter and userLastMapZoom — both useBlueOsStorage, so both reach the vehicle and every other operator of it. The comment on that line explains the intent it was written for: a leave that used to mean the widget was going away, or Mission Planning taking over.
With unmountHiddenViews on, that hook fires every time the user switches off the Map view. Two vehicle-synced keys are written per switch, each debounced 3 s and then pushed (src/composables/settingsSyncer.ts:107-125). The values are also the ones another operator's map opens at, so the position churns for everyone on that vehicle at the rate one person flips between views.
Consequence: every glance away from the map view saves your map position onto the vehicle, where it becomes the position every other operator's map opens at.
Remedy: guard the write on the values having actually moved — keep the last persisted center/zoom and skip saveLastMapPosition when neither changed, which costs a couple of lines at the call site and removes the churn for the common case of switching away and back without touching the map. Alternatively, state in the PR body that the extra churn is accepted; what should not happen is it arriving unnoticed with a performance option.
5. Performance — 2 findings
5.4 major — With the option on, what is mounted follows a vehicle-synced value, so a sync bounce destroys and rebuilds views instead of re-stacking them.
selectViewsToShow(currentProfile.value.views, currentViewIndex.value, unmountHiddenViews.value) at src/stores/widgetManager.ts:308-310 reads currentViewIndex, which is useBlueOsStorage<number>('cockpit-current-view-index-v1', 0) (:63) — vehicle-synced. Two properties of that syncer matter:
- a local write is held in a
setTimeoutfordefaultSettingsSyncDebounceMs, 3000 ms (src/composables/settingsSyncer.ts:11,:107-125), before it reaches the settings manager at all; and - the listener at
:130-144writes any value the settings manager notifies for that key straight into the ref, and the settings manager notifies per changed key fromnotifyAllListenersAboutSettingsChange(src/libs/settings-management.ts:393-411, reached on the user/vehicle switch at:1291-1297, i.e. on vehicle connect) and fromhandleStorageChanging(:1313-1329, astorageevent from another window or an external write).
So for three seconds after the user picks a view, the settings manager still holds the old index, and any notification in that window replays it over the user's choice. That is the mechanism behind ArturoManzoli's report, and the author is right that it predates this PR.
What this PR changes is what the bounce costs. With the option off, a bounce reorders viewsToShow and nothing unmounts. With it on, viewsToShow goes from [chosen] to [previous] and back: both views are torn down and rebuilt in under three seconds — the video stream reconnects (VideoPlayer.vue:316-320), the leaflet instance is destroyed and recreated (Map.vue:768, :1131), every iframe reloads, and per 2.1 two vehicle-synced keys are written. None of it was requested by the user, none of it is attributable to anything they did, and none of it can be stopped, which is the shape section 5 escalates.
Consequence: with the option on, a stray settings sync can throw you back to the view you just left, and now it destroys and rebuilds both views instead of just re-stacking them, so the video reconnects and the map reloads for nothing.
Remedy, at the chokepoint rather than at N producers — viewsToShow is one computed:
- In
widgetManager.ts, do not let the mount set shrink on a transient index. Keep the view being left mounted until the index has held still for longer than the syncer's write debounce, so a bounce costs a re-stack exactly as it does today and only a settled switch unmounts anything. Selection stays on the synced key; only mounting waits for it to settle. - The real site, which fixes it for every synced key and not just this one, is
src/composables/settingsSyncer.ts:109-144: the listener accepts a notification for a key whose local write is still pending inwatchUpdaterTimeout. Flushing the pending write before accepting such a notification — or ignoring one whose value equals the value that pending write is replacing — closes the replay window. That is a change to shared code and larger than this PR; naming it is not a request to do it here, but the local remedy above should not be mistaken for a fix of the underlying race.
5.5 minor — BatteryIndicator's unmount guard is inverted, so its toggle interval survives every unmount — including the ones this PR adds.
src/components/mini-widgets/BatteryIndicator.vue:364-367 (identical at the PR's base 72c3cc2):
onUnmounted(() => {
if (toggleIntervaler.value !== undefined) return
clearInterval(toggleIntervaler.value)
})
The guard returns precisely when there is an interval to clear, so clearInterval only ever runs on the handle that is already undefined. The interval is set at :338-340 whenever both showCurrent and showPower are enabled, firing at options.toggleInterval (floor 500 ms, :248) to flip showCurrent. It is never cleared, so it keeps running against a destroyed component and holds that component's whole setup scope alive with it; errorMessageTimeout (:345) is never cleared either.
This is the fifth site of the invariant the PR's four fix: commits close, and it is the one an enumeration by "does this file have a teardown hook" cannot see, because the hook is there. The default profiles put the battery indicator in the global top bar (src/assets/defaults.ts:933-938), which does not unmount on a view switch, so the exposure is a user who has placed one in a view's own bar — at which point, with the option on, every switch away from that view leaks one permanent timer.
Consequence: a battery indicator placed on a view leaves a timer running forever each time you switch away from that view, so the app slowly accumulates dead work — the opposite of what this option is for.
Remedy: at BatteryIndicator.vue:364-367, drop the guard and clear both handles unconditionally (clearInterval and clearTimeout on undefined are no-ops), matching the four fixes already in this branch. It is a one-line change in a file the PR does not otherwise touch; as a fifth fix: commit it sits naturally alongside the other four.
6. UI / UX — 1 finding
6.2 minor — The new config dialog indents its contents past its own title.
src/components/mini-widgets/ViewSelector.vue:128-158. The anatomy is right and follows in-tree precedent closely: centred v-card-title, the close X as a v-btn icon with aria-label="Close" at absolute top-2 right-2 with equal insets, one glass layer on the card surface, no divider under the header and no footer to need a separator. It is the same shell as ModeSelector.vue:12-31, DepthIndicator.vue:16-31 and BatteryIndicator.vue:73-88, down to the class strings.
What differs from those three is the padding. The card owns pa-4 and v-card-text adds its own horizontal inset, and then both children add more on top: class="mt-2 ml-3" on the switch (:150) and class="text-sm opacity-80 mt-2 ml-3" on the paragraph (:153). The three precedent dialogs put no lateral margin on their content for exactly this reason (ModeSelector.vue:28, DepthIndicator.vue:32, BatteryIndicator.vue:89); ml-3 is the settings-page idiom, where it pairs with the px-4 pt-5 wrapper that ConfigurationUIView.vue:460 has and this dialog does not. The result is content sitting further in than the centred title above it, in a 500 px-wide window where that reads immediately.
This is the same class of thing the author fixed one round ago on the sibling instance of this switch, by dropping the mt-2 at ConfigurationUIView.vue:467.
Consequence: the switch and its explanation sit further in from the edge than the pop-up's own title, so the small window looks lopsided.
Remedy: drop mt-2 ml-3 from both children and let the card's pa-4 own the inset, as the three neighbouring dialogs do; if the switch and paragraph need separating, class="flex flex-col gap-y-4" on the v-card-text is the in-tree form (DepthIndicator.vue:19) rather than per-child margins.
7. Code Quality & Style — 1 finding
7.1 minor — The setting's handler and its explanation are now written out twice.
setUnmountHiddenViews is byte-identical in two files — src/views/ConfigurationUIView.vue:211-215 and src/components/mini-widgets/ViewSelector.vue:187-191 — down to the logUserAction string, so the log cannot distinguish which surface the user toggled it from. The user-facing explanation is duplicated too, and has already diverged: "When enabled, only the view you are looking at stays loaded. Widgets on other views start again when you switch to them. Turn this on if this computer slows down with several views." at ConfigurationUIView.vue:455-456 against "Only the view you are looking at stays loaded. Widgets on other views start again when you switch to them." at ViewSelector.vue:154. Two copies of a setting's copy and its handler, added in the same push, are two places a later wording or behaviour change has to be made — and the second is the one that gets forgotten.
AGENTS.md is explicit on this: "when the same logic genuinely lives (or would live) in two or more places, you extract a shared abstraction rather than duplicating it."
Consequence: the toggle and its explanation now exist in two files, so a later wording or behaviour change will be made in one and forgotten in the other.
Remedy: one source for both. Move the setter next to the ref it writes, as an action on the widget-manager store (src/stores/widgetManager.ts, alongside unmountHiddenViews), and have both surfaces call it; keep the description in one exported string, or render the switch plus its copy from a single small component that both the panel and the dialog mount. Either removes the duplicate handler and the second copy of the text at once, without adding an abstraction beyond the two real call sites.
11. Nitpicks / Optional — 1 finding
11.1 nit — The PR body names one place to find the setting; there are two.
The body says "Toggle: Settings → Interface → Performance". Since this push the same toggle is also in the view selector's Options dialog (src/components/mini-widgets/ViewSelector.vue:144-152, reachable via right-click Options through MiniWidgetInstantiator.vue:86-88 and the edit-mode cog, enabled by src/types/widgets.ts:404). The test plan reaches it only through Settings, so the second entry point is untested by the plan as written.
Consequence: anyone reading the pull request will not learn that the setting is also reachable from the view selector.
Remedy: name the second toggle in that bullet and add a test-plan line for it.
Sections with nothing to report (6)
1. Correctness & Implementation Bugs — ✅ (traced both new switch handlers: :model-value plus @update:model-value rather than v-model, so logUserAction fires on the click and not on a settings-sync write, and Boolean(null) from v-switch lands as false; ViewSelector is instantiated only through MiniWidgetInstantiator.vue:3 from the four default profiles, so its new required miniWidget prop is always supplied, and the dialog is keyed per instance by hash, so several view selectors in one profile do not share dialog state)
3. AGENTS.md Adherence — ✅ (useStorage with a cockpit- prefix for a machine-local value matches mission.ts:54, mainVehicle.ts:90 and useBaseStation.ts:35; no dependency added, so no package.json ordering to check; selectViewsToShow and both new DataLogger methods carry non-empty typed JSDoc with @param/@returns; nothing is exported without a call site in this PR; no comment reworded over unchanged code)
4. Security — ✅ (ran all nine sub-checks across the 10 changed files: no new dependency, no fetch/XHR/websocket, no encoded blob or binary-like constant, no hidden Unicode or homoglyph in the added identifiers, no v-html/eval/Function, no env var or credential, and nothing under .github/, scripts/ or src/electron/; the one listener the diff registers is removed at IFrame.vue:244)
8. Commit Hygiene — ✅ (read the five commits from pr.json: four fix: teardowns then the feat:, each fix: genuinely a fix and standing alone, scope prefixes matching git log on master, no issue or PR reference in any message, no wip/fixup!/"address review" noise, and no commit reverting or reimplementing another — the split made for 8.1 survived the rebase, and this round's edits were folded into the feature commit they belong to)
9. Tests — ✅ (src/tests/types/widgets.test.ts adds five cases for selectViewsToShow covering both modes, a hidden non-current view, a hidden current view, and an out-of-range index in both modes; no existing test removed, weakened or made order-dependent)
10. Documentation — ✅ (the setting behaves identically in Lite and Standalone — no isElectron() branch or Electron API anywhere in the diff — so the README's Lite-vs-Standalone notes need nothing; in-code JSDoc covered under section 3, and the PR-body gap is 11.1)
Generated by Claude. This is advisory; a human reviewer must still approve.
Hidden-but-enabled views stay mounted and keep maps, video, and iframes running, which hurts constrained machines. Add a machine-local setting, off by default, that mounts only the current view.
The unmount guard returned when an interval existed, so the toggle interval and error timeout were never cleared.
A local change is held for three seconds before it reaches the settings manager, so a notification in that window used to replay the old value and bounce the UI.
25a742c to
03a687b
Compare
Review follow-up — round 5Done
Done differently
Won't change (with reasoning)
|
|
/review |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 5.6 | The "recording in progress" close warning belongs to a widget | With the new option on, a video recording can be silently lost when you close the app, or the app can refuse to close claiming a recording is running when none is. | major | ❌ |
| 2.1 | Leaving the map writes to the vehicle every time | Every glance away from the map view saves your map position onto the vehicle, where it becomes the position every other operator's map opens at. | minor | 💬 |
| 7.1 | The setting's explanation is written out twice | The wording shown to the user lives in two files, so a later change will be made in one and forgotten in the other. | minor | 💬 |
Since round 5 — 4 closed, comparing 25a742c → 03a687b
The range. incremental.diff for this round is not a 25a742c…03a687b delta and I did not judge anything from it. The feature commit was amended (e0772d0 now carries a commit date of 20:14:34Z, three hours after the four fix: commits below it), so 25a742c is unreachable and the compare fell back to the merge-base 7b9b494 — the file's seven entries have +/- counts identical to the PR-level totals in pr.json, i.e. it re-includes the whole feature commit. Every status below is therefore judged against pr.diff and the base checkout, using round 5's own quoted lines as the before state.
No maintainer settlements. resolutions.json is [] and decisions.json is []: no /resolve has been issued on this PR and no dispute has been put to a vote, so there are no ids to apply and none that failed to match the ledger.
✅ 5.4 — Addressed (major). The remedy landed at the shared site rather than in widgetManager.ts, which is the second of the two options round 5 named. src/composables/settingsSyncer.ts:141-144 adds, after the existing isEqual(newValue, refedValue.value) early return:
// A pending local write has not reached the store yet; applying its old value would bounce the UI back.
if (firstPendingChangeEpoch !== undefined && isEqual(newValue, oldRefedValue)) {
return
}
I checked this closes the window rather than narrowing it. oldRefedValue is captured in the watch before the write (settingsSyncer.ts:86-94) and holds the pre-write value for exactly as long as the write is pending; firstPendingChangeEpoch is set when the watch fires (:100-102) and cleared inside the debounced write (:109-125). So during the 3 s window the listener now rejects precisely the notification that carries the value the user just replaced — which is the replayed stale index — while still accepting a genuinely different remote value. A real remote index change does still tear down and rebuild both views with the option on, but that is a deliberate switch and not the bounce this finding was about.
✅ 5.5 — Addressed (minor). src/components/mini-widgets/BatteryIndicator.vue:362-367 drops the inverted guard and clears both handles, including the errorMessageTimeout the finding named:
onUnmounted(() => {
- if (toggleIntervaler.value !== undefined) return
clearInterval(toggleIntervaler.value)
+ clearTimeout(errorMessageTimeout.value)
})
Landed as its own commit (53a38ef fix: widgets: clear battery indicator timers on unmount), matching the shape of the other four.
✅ 6.2 — Addressed (minor). ViewSelector.vue:14 is now <v-card-text class="flex flex-col gap-y-4"> with no mt-2 ml-3 on either child (:27-34, :35-37), which is the DepthIndicator.vue:19 form the finding pointed at. The dialog is now structurally identical to DepthIndicator.vue:16-31.
✅ 11.1 — Addressed (nit). The PR body now names both locations ("Settings → Interface → Performance, or right-click the view selector → Options") and adds test-plan lines for them.
💬 2.1 — Disputed (minor, was :x:). The author's position is "won't change", on the grounds that useBlueOsStorage skips a write whose serialization matches the last one. Half of that checks out and half does not, so the finding stays open with its severity unchanged — details in section 2, including a correction to my own round-5 wording.
💬 7.1 — Disputed (minor, was :x:). Half the finding is fixed in code: setUnmountHiddenViews now exists once, as a store action at src/stores/widgetManager.ts:312-320, and both surfaces call it. The duplicated user-facing copy is declined on the stated grounds that the two surfaces want different lengths of text. Reprinted in section 7 against what remains.
❌ 5.6 — New (major). Raised this round from the same invariant enumeration as 5.1-5.5, on a site none of the earlier rounds reached: MiniVideoRecorder.vue owns window.onbeforeunload. Full text in section 5.
Discussion. rafaellehmkuhl's round-5 follow-up (#issuecomment-5640119976) is the only substantive comment; I verified each of its claims against the code rather than taking them as the state of the branch, and the two "won't change"/"done differently" items are what the :speech_balloon: rows above record. His note that ArturoManzoli's map-render report predates this PR is consistent with what round 5 already said; 5.4 was never about who introduced the race, only about what the new option makes it cost, and the guard at settingsSyncer.ts:141-144 answers that either way. The /review command itself carried no content.
Change map — what was established before judging
Claims. The PR body asserts (a) that several views mounted at once slow down weak topside computers, (b) that the cause is every view's widgets running whether or not they are visible, and (c) that the mechanism is an opt-in setting narrowing the mount set to the current view. (a) is a premise I cannot verify from code and take as stated. (b) is verified: viewsToShow at src/stores/widgetManager.ts:308-310 previously returned every view, reordered, and the container renders all of them. (c) is verified: unmountHiddenViews is useStorage('cockpit-unmount-hidden-views', false) (widgetManager.ts:65) and feeds selectViewsToShow (src/types/widgets.ts:1018-1037). The body also claims the four fix: commits are prerequisites; verified — each closes a teardown that only this option makes reachable per view switch.
Failure site. For the four prerequisite fixes and for 5.5, the misbehaving code is in the widgets themselves, and the PR does fix it there (SnapshotTool.vue, IFrame.vue, CollapsibleContainer.vue, VeryGenericIndicator.vue + sensors-logging.ts, BatteryIndicator.vue). For the bounce 5.4 described, the failure site is src/composables/settingsSyncer.ts:130-144, and it is now in the diff. For 5.6 the failure site is src/components/mini-widgets/MiniVideoRecorder.vue:469-489 and it is not in the diff.
Entry points.
| Function | Reached from | Frequency |
|---|---|---|
selectViewsToShow (types/widgets.ts:1018) |
viewsToShow computed (widgetManager.ts:308) → view container render |
per user action (view switch), plus once per settings-sync notification for the view index |
setUnmountHiddenViews (widgetManager.ts:312) |
the switch in ConfigurationUIView.vue:161-169 and in ViewSelector.vue:27-34 |
per user action |
retainVeryGenericIndicatorVariable / releaseVeryGenericIndicatorVariable (libs/sensors-logging.ts) |
VeryGenericIndicator.vue onBeforeMount / onUnmounted |
per user action (now once per view switch, per gauge) |
settings-sync listener callback (settingsSyncer.ts:130-144) |
notifyAllListenersAboutSettingsChange (libs/settings-management.ts:393-411) and handleStorageChanging (:1313-1329) |
per incoming message (vehicle connect, cross-window write) |
onUnmounted bodies in SnapshotTool.vue, IFrame.vue, CollapsibleContainer.vue, BatteryIndicator.vue |
Vue teardown of a view's widget tree | per user action (per view switch, with the option on) |
selectViewsToShow test cases (tests/types/widgets.test.ts) |
vitest | one-shot |
Invariants.
- Every widget's teardown releases everything its setup acquired. Enumerated across
src/components/widgets/,src/components/mini-widgets/andsrc/components/custom-widget-elements/. Covered by the PR: the four prerequisite sites plusBatteryIndicator. Verified clean this round: the custom-widget elements allunlistentheir data-lake variables;Dial's document listeners are drag-scoped;CollapsibleContainer,IFrame,MiniMapandMissionControlPanelregister only transientmouseup/dragendhandlers;SnapshotTool's flash overlay self-heals on asetTimeout;VideoPlayerclears its interval. One site is not covered:MiniVideoRecorder.vueassignswindow.onbeforeunloadand never resets it on teardown — finding 5.6. - The converse: unmount must not do externally-visible work, since unmount is now per view switch rather than per widget deletion. Every other teardown body is a pure release.
src/components/widgets/Map.vue:1110-1112is the one that writes, viamissionStore.saveLastMapPosition— finding 2.1. - Nothing transient may decide the mount set. The mount set follows
currentViewIndex, which is vehicle-synced (widgetManager.ts:63). The guard added atsettingsSyncer.ts:141-144closes the one transient source round 5 identified (a sync echo replaying the pre-write value); see 5.4 above.
2. Persistence & User Data — inventory, 1 finding (carried from round 5)
Inventory.
| Key | Backend | What happened |
|---|---|---|
cockpit-unmount-hidden-views |
machine-local — useStorage (src/stores/widgetManager.ts:65) |
added |
cockpit-current-view-index-v1 |
vehicle-synced — useBlueOsStorage (src/stores/widgetManager.ts:63) |
not modified; now also decides what is mounted, not only what is on top |
cockpit-user-last-map-center |
vehicle-synced — useBlueOsStorage (src/stores/mission.ts:66) |
not modified; written on every view switch away from the map (see 2.1) |
cockpit-user-last-map-zoom |
vehicle-synced — useBlueOsStorage (src/stores/mission.ts:67) |
not modified; write attempted on every such switch, normally deduplicated |
per-widget options (VeryGenericIndicator, SnapshotTool, IFrame) |
vehicle-synced profile storage | not reshaped; no Options entries added or removed |
Judging the added key: machine-local is the right backend, since whether a topside computer needs this is a property of that computer, and the PR's test plan checks the value does not appear on a second topside. The cockpit- prefix is present as AGENTS.md requires, and useStorage with that prefix is the established machine-local form in this tree (mission.ts:54, mainVehicle.ts:90, useBaseStation.ts:35). The stored shape is a bare boolean that does not repeat its key. No migration, no default change stranding anyone: existing users get false, which is today's behaviour.
2.1 minor — Leaving the Map view pushes the map centre to the vehicle on every view switch. (carried from round 5, now disputed)
src/components/widgets/Map.vue:1110-1112 persists the live map view from onBeforeUnmount, into missionStore.saveLastMapPosition (src/stores/mission.ts:519-522), which writes userLastMapCenter and userLastMapZoom — both useBlueOsStorage, so both reach the vehicle and every other operator of it. The comment on that line explains the intent it was written for: a leave that used to mean the widget was going away, or Mission Planning taking over. With unmountHiddenViews on, that hook fires every time the user switches off the Map view.
The author's answer is that useBlueOsStorage skips a write whose serialization matches the last one, so switching away without panning pushes nothing. The first half of that is correct, and I checked it: settingsSyncer.ts:86-94 compares prettyFormat(newValue) against prettyFormat(oldRefedValue) and returns before scheduling anything. My round-5 wording was wrong to say two keys are written per switch, and I withdraw that sentence — the zoom key is an integer that matches its stored value, so it is genuinely deduplicated.
The centre is not, and that is why the finding stays open. persistLiveMapView (src/libs/map/utils-map.ts:312-325, unchanged at this PR's base) does not save the stored ref; it saves what leaflet reports:
const { lat, lng } = map.getCenter()
save(map.getZoom(), [lat, lng])
map.getCenter() is derived from the current pixel origin, so it comes back rounded to the rendered pixel grid and differs from the value that was restored into the map in its low-order decimals. The serialization therefore differs, the dedupe does not fire, and one vehicle-synced key churns per switch away from the map even when the user never touched it. That value is also what every other operator's map opens at.
Consequence: every glance away from the map view saves your map position onto the vehicle, where it becomes the position every other operator's map opens at.
Remedy, unchanged and now a smaller change than it was: in Map.vue, keep the last persisted centre and skip saveLastMapPosition when it has not moved beyond a small epsilon — or compare against missionStore.userLastMapCenter before calling, which is the same two lines at the call site. Either makes the dedupe the author is relying on actually cover this path. Alternatively, state in the PR body that the churn is accepted; what should not happen is it arriving unnoticed with a performance option.
5. Performance — 1 finding
5.6 major — The "recording in progress" close guard is owned by a widget, so unloading hidden views leaves window.onbeforeunload either stale or absent.
src/components/mini-widgets/MiniVideoRecorder.vue:469-489 installs and removes the app's only guard against closing during a recording, from a component watcher: when the widget's stream stops recording it sets window.onbeforeunload = null (:472), and when it starts it installs a handler returning the "recording will be lost" warning (:485-488). The teardown hook at :454-458 clears the stream-connection interval, the loading timeout and the stream consumer registration — it does not reset onbeforeunload, and the watcher is not immediate, so it fires on transitions only.
That was harmless while the widget's lifetime matched the app's. It no longer does. The default ROV profile places MiniVideoRecorder in view-scoped bars — the Video view's own bar (src/assets/defaults.ts:294-299) and the HUD view's (:637-642) — not in the global containers that start at :897. With the option on, switching views unmounts it. Recording itself keeps going: deactivateStreamIfUnused (src/stores/video.ts:690-698) explicitly refuses to tear down a stream while a mediaRecorder is attached, which is correct and is what makes both halves of this reachable.
- Stale guard. A recording stops while the widget is unmounted — from
stop_recording_all_streamsortoggle_recording_all_streams, registered as cockpit actions atvideo.ts:1580-1591and bindable to a joystick button or a key, or internally on an error path (video.ts:1150,:1178). No watcher runs, soonbeforeunloadkeeps returning the warning. Returning to the view does not clear it either:onBeforeMount(:205-217) resolvesselectedExternalIdsynchronously, soisRecording(:370-378) evaluates to its settled value with no transition for the watcher to see. The user is now told a recording will be lost every time they close Cockpit, indefinitely, with nothing recording. - Missing guard. The mirror case: a recording is started by those same actions while the widget is unmounted, or is running when the user switches away and closes the app from another view. No handler is installed, so the window closes with no warning and the in-flight recording is lost.
Consequence: with the option on, closing Cockpit can silently discard a video recording that was still being written, or refuse to close claiming a recording is in progress when there is none.
Remedy: the guard is global state about a global fact, so it should not be owned by a widget at all. Move the watcher into src/stores/video.ts, next to the recording state it observes, and have it set and clear window.onbeforeunload there — the store outlives every view, so both transitions are always seen and no widget has to be mounted for the warning to be right. That also deletes the block from MiniVideoRecorder.vue rather than adding to it. Like 5.5, this is outside the diff, so it belongs in a small fix: commit of its own alongside the other five.
7. Code Quality & Style — 1 finding (carried from round 5)
7.1 minor — The setting's user-facing explanation is written out twice. (carried from round 5, partly fixed, now disputed)
The handler half of this is fixed and I confirmed it: setUnmountHiddenViews exists once, as a store action at src/stores/widgetManager.ts:312-320, next to the ref it writes, and both surfaces call it (ViewSelector.vue:27-34 and ConfigurationUIView.vue:161-169 both bind @update:model-value="widgetStore.setUnmountHiddenViews"). That is exactly the first remedy round 5 named, and the logUserAction string now exists once too.
What remains is the copy, which the author declines on the grounds that the panel wants the longer explanation and the dialog the short line. The two strings are still two strings describing one setting, and they already share a sentence verbatim:
ConfigurationUIView.vue:154-157— "When enabled, only the view you are looking at stays loaded. Widgets on other views start again when you switch to them. Turn this on if this computer slows down with several views."ViewSelector.vue:35-37— "Only the view you are looking at stays loaded. Widgets on other views start again when you switch to them."
AGENTS.md: "when the same logic genuinely lives (or would live) in two or more places, you extract a shared abstraction rather than duplicating it." The severity stays minor — nothing reaches a user wrong today.
Consequence: the wording shown to the user lives in two files, so a later change to what this option does will be made in one and forgotten in the other.
Remedy, sized to the objection rather than to the original finding: export the shared sentence once (next to the store action, or in src/types/widgets.ts beside selectViewsToShow) and have the panel render it followed by its extra "Turn this on if…" line, which is the only part that is genuinely panel-specific. That keeps both lengths and leaves one place to edit.
Complexity, for this section. complexity-report.json for this head reports 393 functions measured across the 12 changed files, untruncated, with no function tripping any threshold — those are the report's figures, produced by the PR's own CI run rather than measured here. No complexity finding this round.
Sections with nothing to report (8)
1. Correctness & Implementation Bugs — ✅ (re-checked the clamp in selectViewsToShow at types/widgets.ts:1018-1037, the retain/release pairing in sensors-logging.ts against the three delete paths at widgetManager.ts:576/:581 and MiniWidgetContainer.vue:159, and the Boolean(value) coercion in the new store action against Vuetify's null switch value)
3. AGENTS.md Adherence — ✅ (the three added functions carry typed @param JSDoc; the absent @returns on the void setUnmountHiddenViews matches selectView at widgetManager.ts:463-467 and .eslintrc.cjs does not require one for void; no dependency added, package.json untouched, and nothing exported without a call site in this PR)
4. Security — ✅ (whole diff scanned for non-ASCII and zero-width characters, encoded blobs, eval/Function/v-html, new network hosts, and changes under .github/, scripts/ or src/electron/; none present, and no input file carried text addressed to the reviewer)
6. UI / UX — ✅ (the ViewSelector dialog now matches DepthIndicator.vue:16-31 clause for clause — centred title, keyboard-reachable v-btn icon close X with equal insets, one glass layer, no divider under the header, no footer — the settings switch keeps the in-tree -mb-2 ml-3 idiom of ConfigurationMissionView, and the toggle is logged through logUserAction in the store action)
8. Commit Hygiene — ✅ (seven commits read off pr.json; each fix: closes exactly one teardown site, the two added this round are separate commits rather than folded into the feature, which is what 8.1 asked for, and there are no issue references, no wip/fixup!, and no commit reverting another)
9. Tests — ✅ (src/tests/types/widgets.test.ts adds cases for selectViewsToShow including the out-of-range index, and removes or weakens no existing assertion)
10. Documentation — ✅ (the option behaves identically in Lite and Standalone, so AGENTS.md's README-parity requirement does not apply, and the PR body now names both places the toggle lives)
11. Nitpicks / Optional — ✅ (11.1 closed this round; nothing further raised)
Generated by Claude. This is advisory; a human reviewer must still approve.
🙋 Decision needed — 2.1Leaving the Map view persists the map position to two vehicle-synced keys on every view switch once hidden views are unloaded The author's argument: The vehicle-synced storage layer already skips a write whose serialized value matches the last one, so switching away from the map without panning does not push anything to the vehicle, and saving the position when the map is left is the intended behaviour. How to vote on this disputeReact to this comment and the next
The two reactions already here were left by the bot so that either answer is one click, and neither of them counts. Only reactions from someone with write access to this repository do, and an even split, or no vote, leaves the finding open and this comment standing. Move your reaction to change your mind while the vote is open — once a |
🙋 Decision needed — 7.1setUnmountHiddenViews and the setting's user-facing explanation are duplicated between ConfigurationUIView.vue and ViewSelector.vue The author's argument: The handler now lives once in the widget-manager store and both surfaces call it; the explanation stays duplicated on purpose because the settings panel needs the longer wording and the view-selector dialog the short one. How to vote on this disputeReact to this comment and the next
The two reactions already here were left by the bot so that either answer is one click, and neither of them counts. Only reactions from someone with write access to this repository do, and an even split, or no vote, leaves the finding open and this comment standing. Move your reaction to change your mind while the vote is open — once a |
2 participants
Summary
Five commits: four unmount teardowns that were already wrong on delete, then the opt-in setting. Two more
fix:commits after review: battery-indicator timers were never cleared, and a pending settings-sync write no longer lets a stale echo bounce the current view.fix:). Intervals are cleared when the snapshot tool unmounts, and the existing "Timed snapshot stopped." snackbar fires so a view switch does not end a capture in silence.fix:). Unmounted VeryGenericIndicators leave the sensor log. A retain count keeps a shared display name in the log until the last copy is gone, and the entry is released under the name it was retained with, so renaming a gauge while it is on screen does not drop another gauge's entry or strand its own.fix:). The window resize listener on CollapsibleContainer is removed on unmount.fix:). Data lake variables an embedded page subscribes to through the external API are unlistened on unmount, instead of leaving one listener behind per variable.Test plan
Checks
selectViewsToShowwith the option off still returns every visible view, current last; with it on, only the current view. Out-of-range indexes are clamped. Cases insrc/tests/types/widgets.test.ts.yarn lintandyarn test:unitclean.Closes #3036