fix: video: stop the recording health monitor from reopening its dialog - #2951
Conversation
Automated PR Review — round 1 —
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.1 | One opt-out button silences two different warnings | A user who dismisses the mild "file is not growing" nag will never be told, for the rest of the session, that the recording file has vanished — so a recording can be lost without any warning. | major | ❌ |
| 1.2 | Repeat popups are matched only by their wording | If two different parts of the app ask for a popup that happens to use the same words, the second one silently never appears and gets the first one's answer. | minor | ❌ |
| 6.1 | The new "don't show again" button is not logged | When a user later reports that Cockpit stopped warning them about a broken recording, the support log has no record of them having switched the warnings off. | minor | ❌ |
| 7.1 | The warning text is written across two source lines | The warning sentence carries a line break and a run of spaces in the middle of it, which will show up literally anywhere the text is not rendered as web page content. | minor | ❌ |
| 8.1 | Two separate changes in one commit | The reworded warning and its new opt-out button cannot be reverted or backported without also reverting the popup fix. | minor | ❌ |
| 11.1 | Debug printouts left in the new test | Continuous-integration logs get three lines of leftover debugging noise on every run. | nit | ❌ |
Change map — what was established before judging
Claims (from the PR body and commit message — the author's hypothesis, checked against the code)
| Claim | Verdict |
|---|---|
"A recording with a missing output file made the 15 s health monitor call showDialog on every tick" |
Verified. src/stores/video.ts:823-828 (base) calls showDialog directly and returns without clearing the interval registered at src/stores/video.ts:813, whose period is 15000 ms (src/stores/video.ts:836). Nothing guards the call. |
"showDialog unmounts and mounts a fresh dialog on each call" |
Verified. showDialog (src/composables/interactionDialog.ts:139-151) unconditionally calls mountDialog, which begins with unmountDialog() (src/composables/interactionDialog.ts:119-122). |
| "the warning kept popping back with no way to dismiss it for good" | Verified. The missing-file branch passed no actions, and defaultDialogState() sets persistent: true (src/composables/interactionDialog.ts:97), so the only control was the fallback Close button at src/components/InteractionDialog.vue:74, which the next tick undid. |
| "The fix lands in the shared dialog composable rather than in the caller" | Verified, and it is the right chokepoint: the remount was in the shared helper, not in the caller. |
| "That covers every caller that notifies from a poll or an interval" | Partly contradicted. useInteractionDialog is a factory, not a singleton — every call gets a fresh closure (src/composables/interactionDialog.ts:101-106 plus the two new lets). The guard therefore dedupes only within one instance, and only when title and message are byte-identical. Callers that build a message containing a changing value (a size, a count, a timestamp) still remount on every tick. See finding 1.2. |
| "it replaces the hand-rolled 'is it already open' flag the sibling not-growing-file check carried" | Verified; notGrowingDialogOpen (src/stores/video.ts:779, :781, :800-801) is removed by the diff. |
Failure site — src/stores/video.ts:823-828 (the unguarded showDialog in the Electron monitor tick) and src/composables/interactionDialog.ts:139-151 (the unconditional remount). Both are in the diff, and the diff fixes the shared one rather than only the caller.
Entry points
| Function | Reached from | Frequency |
|---|---|---|
showDialog (src/composables/interactionDialog.ts:139, head hunk @@ -137,21 +140,35) |
~30 call sites; the one this PR targets is the setInterval at src/stores/video.ts:813 (Electron) and :839 (web) |
per interval tick — every 15 s per active recording stream, plus per user action from every other caller |
closeDialog (src/composables/interactionDialog.ts:153) |
dialog action buttons via handleAction (src/components/InteractionDialog.vue:251-254), and the internalShowDialog watcher at src/components/InteractionDialog.vue:247 |
per user action |
new resolveFn / rejectFn wrappers (added inside showDialog) |
the onConfirmed / onDismissed props passed in mountDialog (src/composables/interactionDialog.ts:127-132), emitted at src/components/InteractionDialog.vue:253 and :246 |
per user action |
suppressRecordingHealthDialog (src/stores/video.ts:784, renamed) |
the "Don't show again during this session" button in recordingHealthDialogConfig.actions |
per user action |
showRecordingHealthDialog (src/stores/video.ts:799, renamed) |
setInterval at src/stores/video.ts:813 and :839 |
per interval tick — every 15 s per active recording stream |
src/tests/composables/interactionDialog.test.ts body |
vitest only | never in production code (test file, not a finding) |
Invariants — the change relies on: openDialogPromise is defined exactly while this composable instance's dialog is on screen. Every site that can take the dialog off screen, and whether the PR covers it:
| Site | Covered? |
|---|---|
closeDialog (src/composables/interactionDialog.ts:153) |
Yes — the diff adds openDialogPromise = undefined. |
user confirm → emit('confirmed') (src/components/InteractionDialog.vue:253) → resolveFn |
Yes — the new resolve wrapper clears it. |
dismiss / timer expiry → emit('dismissed') (src/components/InteractionDialog.vue:246, :226) → rejectFn |
Yes — the new reject wrapper clears it. |
a superseding showDialog with a different key |
Yes — resolveFn?.(…) runs before the reassignment, so the clear cannot clobber the new promise. |
InteractionDialog.vue:247 calling closeDialog from its own composable instance, not the store's |
Yes, indirectly — emit('dismissed') at :246 fires first on the store's instance. |
onUnmounted → unmountDialog (src/composables/interactionDialog.ts:158-160) does not clear the flag |
Moot — the closure dies with its owning component, so that instance can never be asked again. |
a different useInteractionDialog() instance mounting its own dialog over this one |
Not covered, and pre-existing: unmountDialog only knows its own mountPoint, so cross-instance stacking is unchanged by this PR and is not a finding against it. |
Complexity — complexity-report.json reports triggeredCount: 0 over 118 functions measured across all 3 changed files, truncated: false. No complexity findings.
1. Correctness & Implementation Bugs — 2 findings
1.1 — One session flag now silences two warnings of very different severity (major)
src/stores/video.ts:74 and the diff hunk at :775-790.
The diff renames suppressNotGrowingDialogs to suppressRecordingHealthDialogs and routes the missing-file branch through showRecordingHealthDialog, which returns early on that same flag. The flag is:
- store-level, not per recording (
src/stores/video.ts:74), so it spans every stream and every recording started later in the session; - never reset —
grepfinds only the declaration and the single= truewrite, no path back tofalse; - set from a button whose label ("Don't show again during this session") does not say which warnings it covers.
On Standalone both warnings come from the same monitor tick (src/stores/video.ts:813), so the sequence is reachable in one sitting: the user silences the relatively benign "The video output file is not growing" nag on one stream, and from then on the far more serious "Cockpit cannot find the file this recording is being saved to" — the one the new copy itself says means the recording may be lost — never appears, for any stream, for the rest of the session. In the base revision that warning was not suppressible at all (which was the bug), so this is a behaviour the PR introduces, not one it inherits.
Consequence: a user can lose a recording with no warning at all, because they earlier consented to silence a different, milder message.
Smallest fix: keep the opt-out scoped to what the user actually opted out of. Either give showRecordingHealthDialog a per-message suppression set (suppressedRecordingHealthMessages: Set<string>, keyed on the message it was invoked with) and check membership instead of one boolean, or leave the boolean to the not-growing checks and omit the opt-out action from the missing-file config. If the intent really is one switch for all of them, the button has to say so ("Stop warning me about this recording's health") and the finding becomes a copy fix instead.
1.2 — The dedup key covers only title and message, and the guard discards the caller's other options (minor)
Added in showDialog, diff hunk src/composables/interactionDialog.ts @@ -137,21 +140,35:
const key = JSON.stringify([options.title, options.message])
if (openDialogPromise && openDialogKey === key) return openDialogPromise
Two problems follow from the key:
variant,actions,maxWidth,persistentandtimerare outside it, so a second caller with the same wording but different buttons gets an early return:dialogPropsis never updated, theiractionsnever render, and they are handed a promise owned by the first caller — so theirisConfirmedis decided by whichever button the user pressed on the other request. I traced the shared-instance call sites (src/libs/utils-vue.ts:5,src/stores/widgetManager.ts:49,src/stores/mission.ts:127,src/stores/controller.ts:264,src/stores/snapshot.ts:25,src/libs/sensors-logging.ts:39,src/libs/joystick/protocols/mavlink-manual-control.ts:216) and found no collision today, which is why this isminorrather thanmajor— but nothing stops the next one, and the failure is silent.titleis read raw, whiledefaultDialogState()normalises it to''(src/composables/interactionDialog.ts:93). A caller that omitstitleand one that passestitle: ''produce[null,…]and["",…], i.e. two different keys for the same dialog, so the dedup does not fire between them.
Fix, in the same two lines: key on the whole options object (JSON.stringify(options) will not capture the action callbacks, but title/message/variant/persistent/timer are what distinguish dialogs in practice) or, at minimum, use options.title ?? '' and compare message and variant together. Either keeps the diff the same size.
Note for the PR body: the claim that this "covers every caller that notifies from a poll or an interval" holds only for callers whose message text is constant across ticks. useInteractionDialog is a factory (each call gets its own closure), so the dedup is also per-instance rather than app-wide.
6. UI / UX — 1 finding
6.1 — The new dialog actions are not logged via logUserAction (minor)
Diff hunk src/stores/video.ts @@ -775,31 +775,24. The missing-file dialog gains two buttons it did not have before, and suppressRecordingHealthDialog is a new handler; neither it nor the Close action calls logUserAction. AGENTS.md ("Logging user interactions") requires every discrete user interaction in a new feature to produce a logUserAction(…) entry, and src/stores/video.ts currently contains no logUserAction call at all, so this is house direction the file has not converged on yet rather than a local inconsistency — hence minor.
The session opt-out is the one that matters: once it is pressed, Cockpit deliberately stops warning about a recording that may be failing, and finding 1.1 shows how wide that silence is. Without a log entry, a later "Cockpit didn't tell me the recording was broken" report is unexplainable from the logs.
One line in the handler, in the established past-tense voice:
logUserAction('Silenced the recording health warnings for this session')
Consequence: when a user reports missing warnings, support has no record that the user turned them off.
7. Code Quality & Style — 1 finding
7.1 — The new warning text is a two-line template literal, so a newline and 12 spaces end up inside the string (minor)
Diff hunk src/stores/video.ts @@ -821,14 +814,16:
const msg = `Cockpit cannot find the file this recording is being saved to, which means the recording may
be lost. We recommend stopping it and starting a new one.`
The literal contains may\n be lost. It renders acceptably today only because src/components/InteractionDialog.vue:31 interpolates it into a plain <div>, where HTML collapses the run of whitespace — the string itself is wrong, and it will show the break literally the moment it reaches a title attribute, a log line, a snackbar with white-space preserved, or a copy-paste.
max-len is code: 180 with .eslintrc.cjs:47, and the single-line form is ~175 characters at that indentation, so this fits without the bare // eslint-disable-next-line the diff correctly removes. Alternatively DialogOptions.message already accepts string[] (src/composables/interactionDialog.ts:17) and the component renders an array as list items (src/components/InteractionDialog.vue:26-28), which is the in-tree way to split a message across lines deliberately. Both sibling messages in the same function stay on one line (src/stores/video.ts:831, :852-854).
Consequence: the warning sentence carries a stray line break and indentation that will surface as literal text wherever it is not rendered as HTML body content.
8. Commit Hygiene — 1 finding
8.1 — The single commit bundles the shared dialog fix with a separate user-facing behaviour change (minor)
pr.json lists one commit, e6b76d4 "video: stop the recording health monitor from reopening its dialog". Read against git log on this checkout, the scope-prefixed subject is the dominant style here and video: fits, so the subject is fine. The contents are two logical changes:
- the
showDialogdedup insrc/composables/interactionDialog.tsplus the removal of the now-redundantnotGrowingDialogOpenflag and its test — this is the fix, and it alone stops the reopening; - the missing-file branch being rewritten to use
recordingHealthDialogConfig, gaining a two-button footer, the session opt-out and completely new copy — a user-visible behaviour change that is not required for (1) to work.
AGENTS.md "Commit hygiene": "A fix or a modification to existing behavior gets its own commit, never a corner of the feature commit that happens to touch the same code — it has to be reviewable, revertable, and backportable on its own." Finding 1.1 is entirely inside (2); with the split, it could be reverted or reworked without touching the composable fix.
Nothing else here is a problem: no issue or PR reference in the commit message (Closes #2950 correctly sits in the PR body only), no wip/fixup! noise, no over-splitting, and 74/28 lines is well within one sitting.
Consequence: the reworded warning and its new opt-out cannot be reverted or backported without also reverting the popup fix.
11. Nitpicks / Optional — 1 finding
11.1 — Three leftover console.log calls in the new test (nit)
src/tests/composables/interactionDialog.test.ts lines 12, 18 and 23 of the added file print the mounts counter before each expect. no-console is off (.eslintrc.cjs:49) so this is not a lint error, and no existing test under src/tests/ logs progress this way. The expect calls already report the value on failure. Drop them.
Sections with nothing to report (6)
2. Persistence & User Data — ✅ (the only touched declaration near persisted state is src/stores/video.ts:74, a plain ref(false) renamed in place; the useBlueOsStorage keys at :68-69 appear as diff context only, and no cockpit-* key is added, reshaped or removed, so the section collapses per the inventory rule)
3. AGENTS.md Adherence — ✅ (no new dependency and no package.json change; the two renames are justified by the widened meaning of the flag rather than cosmetic, so scope discipline holds; the reworded comment at src/stores/video.ts:790-791 sits above unchanged persistent: true, but it named the deleted notGrowingDialogOpen and was therefore factually wrong, which the comment-immutability rule permits rewriting; the showDialog JSDoc at src/composables/interactionDialog.ts:79-81 was extended alongside the code it documents and keeps its typed @param/@returns; nothing exported is left without a call site)
4. Security — ✅ (checked all sub-checks: no new dependency, no encoded blob, no non-ASCII or bidirectional characters in the three files, no network call, no eval/Function/v-html, no env var or credential, no change under scripts/, .github/ or src/electron/, no licensing impact; pr.json, pr.diff and complexity-report.json were also read for text addressed to the reviewer and contain none)
5. Performance — ✅ (the guard removes an app unmount/createApp/mount cycle per 15 s tick at src/stores/video.ts:813 and :839; no timer, listener, subscription or watch is added, so there is nothing new needing teardown, and none of the hot paths — mavlink:onIncomingMessage, dataLake:setVariable, dataLake:notifyListeners — is touched; the added work per call is one JSON.stringify of two short strings)
9. Tests — ✅ (no existing test weakened or removed; the added test asserts mount counts through a stubbed InteractionDialog.vue and asserts promise identity, which matches the behaviour the diff introduces rather than its implementation, and it restores state via closeDialog() before its last assertion so it is not order-dependent within the file)
10. Documentation — ✅ (no Lite/Standalone parity change to record in README.md — the missing-file check already ran only behind window.electronAPI at src/stores/video.ts:811 and still does; the only public surface touched, the showDialog contract, has its JSDoc updated in the same hunk)
Generated by Claude. This is advisory; a human reviewer must still approve.
e6b76d4 to
b3c5aeb
Compare
Review follow-up — round 1Done
Done differently
Notes
|
|
/review |
Automated PR Review — round 2 — 📝 MINOR SUGGESTIONS
While a video recording runs, Cockpit checks every 15 seconds that the file is still growing and pops a warning up when it is not. This round the shared pop-up helper now recognises a request for a pop-up that is already on screen — matching on the whole request, not just its wording — and hands the caller the existing one back instead of tearing it down and rebuilding it. The "output file is missing" warning now goes through the same pop-up as its siblings, with new wording, a "don't show again this session" button that only silences the one warning the user dismissed, and log entries for both buttons. A unit test counts pop-up rebuilds, and the work is now split into two commits. What still needs attention
Since round 1 — 6 closed, 2 new, comparing e6b76d4 → b3c5aebRange —
No Status of the round 1 findings
New this round — 1.3 ( Discussion since round 1
Change map — what was established before judgingClaims (from the PR body and the two commit messages — the author's hypothesis, checked against the code)
Failure site — base Entry points
Invariants Invariant A —
Invariant B — the key is byte-stable across ticks, or the guard does not fire. Held by every message this PR passes (all three are constant literals). It breaks for any message carrying a changing value, and the guard is in any case only a per-instance, most-recent-dialog one: see 1.3, and note the interaction with 6.2's suggestion to name the stream. Complexity — 1. Correctness & Implementation Bugs — 1 finding1.3 — The no-remount guard only covers the most recently shown dialog, so the 15 s monitor can still rebuild a dialog under the user (
|
b3c5aeb to
e7da178
Compare
Review follow-up — round 2Done
Done differently
Won't change (with reasoning)
Notes
|
|
/review |
Automated PR Review — round 3 — 📝 MINOR SUGGESTIONS
While a video recording runs, Cockpit checks every 15 seconds that the file is still growing and pops a warning up when it is not. The shared pop-up helper now recognises a request for a pop-up that is already on screen and hands the caller the existing one back instead of tearing it down and rebuilding it. On top of that, this round the recording checks take turns on the single pop-up surface: the warning on screen stays put, and the checks for other broken recordings come back on a later tick instead of replacing it. Every warning now names the recording it is about, the "output file is missing" warning goes through the same pop-up with a "don't show again this session" button, and both buttons write a log entry naming the warning the user acted on. What still needs attention
Since round 2 — 1 closed, 1 new, 1 partially addressed, comparing b3c5aeb → e7da178Range —
No Status of the previously open findings
New this round — 11.2 ( Discussion since round 2
Change map — what was established before judgingClaims (from the PR body and the two commit messages — the author's hypothesis, checked against the code)
Failure site — base Entry points
Invariants Invariant A — Invariant B —
Complexity — 1. Correctness & Implementation Bugs — 1 finding (carried from round 2)1.3 — The single-surface owner has no severity order, and the corner-cut comment names a ceiling the code does not have (
|
e7da178 to
40066fa
Compare
Review follow-up — round 3Done
Notes
|
|
/review |
Automated PR Review — round 4 —
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 3.1 | Two empty documentation blocks added | Two explanation notes were added to the code with nothing written inside them, so the next person reading this file finds a blank where the description should be. | major | ❌ |
Since round 3 — 2 closed, 1 new, comparing e7da178 → 40066fa
Range — e7da17811f1c013aa2bd3e2a0f05b4d83d1f88cf → 40066fa4c4c1457b1c3b4437998bc2face2352ec.
incremental.diff is again not usable as this round's delta, though it is closer than round 3's. It reports one file (src/stores/video.ts, +65/-27), but its removed side matches neither of the two revisions it should: it does not match the base checkout, which still declares let notGrowingDialogOpen at src/stores/video.ts:779, and it does not match what round 3 recorded at e7da178, which had the store-level let isRecordingHealthDialogOpen at head :77. What it removes is suppressNotGrowingDialogs.value plus a flag-less showNotGrowingDialog — the state round 3 described at round 2's head. So the range it spans covers round 3's own work as well as this round's, which would credit changes that were already reviewed. Every status below was therefore judged against pr.diff and the base checkout, as in round 3.
No /resolve commands have been issued on this PR (resolutions.json is []), so nothing was closed by a maintainer this round, and there are no unknown ids to report back.
Status of the previously open findings
- 1.3 — The single-surface owner has no severity order, and the corner-cut comment names a ceiling the code does not have (
minor) — ✅ Addressed. The finding offered two routes and the first one landed whole. (a) The store-levelletis now the owning warning rather than a boolean:openRecordingHealthWarning: { message, meansDataLoss } | undefinedat headsrc/stores/video.ts:77-88. (b) The ranking is at head:111—if (openRecordingHealthWarning && (openRecordingHealthWarning.meansDataLoss || !meansDataLoss)) return— so a milder warning yields the surface to a data-loss one, while data-loss holds against everything. Only the missing-file call site passestrue(head:854); the not-growing (:860-862) and web-chunk (:883-885) sites use thefalsedefault. (c) Theponytail:line at head:109-110now names the real ceiling ("a second unhealthy stream can wait as long as the user leaves the first dialog up") instead of round 3's "waits a tick". The release also had to become owner-checked, and it is:releaseRecordingHealthDialogcompares against the current owner at head:91, and the new owner is assigned at:112beforeshowDialogruns at:114, so the superseded warning's.then(release, release)(:128) fires a microtask later, finds a different owner, and no-ops rather than clearing the warning that just took the surface. - 11.2 — The four health-dialog helpers stay nested in
startRecording(nit) — ✅ Addressed. Both parts landed. The four helpers are now store-level at head:90-129, directly under the two declarations they use (:75,:77), so they are built once instead of once per active recording;startRecordingdrops from 325 lines (base:722-1046) to ~300 (head:777-1077). The doubled reasoning is gone too: head:106-108states the ownership-and-preemption rule once and:109-110states the equal-weight queueing ceiling, which are different facts rather than the overlap round 3 quoted.
New this round — 3.1 (major), written out below. It arrived with the object type that replaced the boolean, and the author's follow-up does not mention it.
Discussion since round 3
- @rafaellehmkuhl's follow-up (comment) lists six items; each was checked against
pr.diffrather than taken as evidence, and all six hold. The two claims about text outside the code check out as well: the second commit's body inpr.jsonnow ends "one that means the recording may already be lost takes the surface from a milder one rather than waiting behind it for as long as the user leaves it up", and the PR body's third bullet carries the same corrected wording. - The comment's account of why the release had to become owner-checked ("otherwise the superseded warning's
.thenwould clear the flag the new owner had just set, and the two would ping-pong every 15 s") is the one claim worth re-deriving, since the whole priority feature depends on it. It is correct, for the ordering reason given under 1.3 above. - The Notes item — no unit test for the store-side guard, because the helpers are
defineStoreinternals — is accurate and is not raised: this review does not ask for tests for logic a PR adds. It does mean the new priority rule is covered only by the manual test-plan items. - The other comment since round 3 is the bare
/reviewthat triggered this round, and carries nothing to act on.
Change map — what was established before judging
Claims (from the PR body and the two commit messages — the author's hypothesis, checked against the code)
| Claim | Verdict |
|---|---|
"A recording with a missing output file made the 15 s health monitor call showDialog on every tick" |
Verified. Base src/stores/video.ts:823-827 calls showDialog and returns without touching the interval registered at base :813, whose period is 15000 ms (base :836). Nothing guarded it. |
"showDialog unmounts and mounts a fresh dialog on each call" |
Verified. Base showDialog (src/composables/interactionDialog.ts:139-151) always calls mountDialog, which opens with unmountDialog() (base :119-122). |
| "the warning kept popping back with no way to dismiss it for good" | Verified. The base missing-file branch passed no actions, and defaultDialogState() sets persistent: true (base :97), leaving only the fallback Close at src/components/InteractionDialog.vue:74, which the next tick undid. |
"showDialog no longer remounts a dialog that is already on screen … the pending promise is returned" |
Verified. Head src/composables/interactionDialog.ts:147-149: resolvedOptions / key / if (openDialogPromise && openDialogKey === key) return openDialogPromise, before any mounting; the same promise object is returned at head :169. Unchanged this round. |
"useInteractionDialog is a factory, so this dedupes per composable instance, and it covers only the dialog shown last" |
Verified. The store holds one instance (src/stores/video.ts:49), shared by both monitors and ~16 unrelated showDialog calls in the same file (:382, :417, :727, :1081, …). |
| "It replaces the hand-rolled 'is it already open' flag the sibling not-growing file check carried" | Verified in part, and the body says which part. The per-stream notGrowingDialogOpen (base :779) is gone; what replaces it is store-wide and now richer than a flag (head :77-88), which the body's third bullet states outright. |
| "the store tracks which warning owns the surface … a warning about a recording that may already be lost therefore takes the surface from a milder one instead of queueing behind it" | Verified, and this is round 3's overstated claim now correct. Head :111 yields the surface only when the incoming warning is not a data-loss one, or the owner already is; :112 claims it; release is owner-checked at :91. |
| "the monitors of the other unhealthy streams retry on later ticks … the wait is as long as the user leaves it up" | Verified, including the limitation. Two warnings of equal weight still queue indefinitely behind whichever showed first, which is exactly what the ponytail: line at head :109-110 now says. |
| "Every recording health warning names its stream" | Verified. Head :853, :861, :884, all three through showRecordingHealthDialog. |
| "The session opt-out is keyed by message, so silencing the mild not-growing nag does not silence the serious missing-file warning" | Verified — suppressedRecordingHealthMessages.has(message) at head :105, .add(message) at :95. Since the messages carry the stream name, the opt-out is per stream as well; "during this session" does not promise otherwise, so it is not raised. |
| "The Close action's log entry now names the warning it closed, matching the opt-out's" | Verified — head :100 against :94. |
"yarn lint and yarn test:unit clean" |
Plausible, and that is the problem in 3.1. No PR code is executed here. The longest added code line is the missing-file message at head :853, between 170 and 176 columns against max-len code: 180 (.eslintrc.cjs:47); the added functions are arrow expressions, which @typescript-eslint/explicit-function-return-type allows via allowExpressions (.eslintrc.cjs:84), and each declares : void anyway; logUserAction is an ESLint global (.eslintrc.cjs:18-21). The two empty JSDoc blocks satisfy jsdoc/require-jsdoc, which only requires that a block exist, so a clean lint run is consistent with them being there — see 3.1. |
Failure site — base src/stores/video.ts:823-827 (the unguarded showDialog on the Electron monitor tick) and base src/composables/interactionDialog.ts:139-151 (the unconditional remount). Both are in the diff, and the shared one is fixed rather than only the caller.
Entry points
| Function | Reached from | Frequency |
|---|---|---|
showDialog (head src/composables/interactionDialog.ts:142-170) |
~30 call sites across the tree; the ones this PR targets are the setIntervals at head src/stores/video.ts:841 (Electron, period :867) and :870 (web, period :890), through the single instance at :49 |
per interval tick — every 15 s per active recording — plus per user action from every other caller |
resolveFn / rejectFn wrappers (head src/composables/interactionDialog.ts:159-166) |
the onConfirmed / onDismissed props passed in mountDialog, emitted at src/components/InteractionDialog.vue:253 and :246 |
per user action |
closeDialog (head src/composables/interactionDialog.ts:172-176) |
the two health actions (head src/stores/video.ts:97, :102) — grep confirms they are still the store's only closeDialog() calls; also src/components/InteractionDialog.vue:247, on that component's own instance, which owns no mounted app |
per user action |
showRecordingHealthDialog (head src/stores/video.ts:104-129) |
the two setIntervals above, via :852, :860, :883; one monitor per recording stream |
per interval tick — every 15 s per active recording |
releaseRecordingHealthDialog (head src/stores/video.ts:90-92) |
both dialog actions, and the .then(release, release) at head :128 when the dialog's promise settles |
per user action, or per superseding dialog |
suppressRecordingHealthDialog (head src/stores/video.ts:93-98) |
the "Don't show again during this session" button | per user action |
closeRecordingHealthDialog (head src/stores/video.ts:99-103) |
the "Close" button | per user action |
src/tests/composables/interactionDialog.test.ts body |
vitest only (default **/*.test.ts include; vite.config.ts:70-73); unchanged since round 2 |
never in production code (test file, not a finding) |
Invariants
Invariant A — openDialogPromise is defined exactly while this instance's dialog is on screen. Unchanged since round 2 and re-checked: closeDialog clears it (head interactionDialog.ts:174), both settle wrappers clear it (:160, :164), and a superseding showDialog runs resolveFn?.() at :153 before assigning the new promise at :155, so the stale clear cannot clobber the new one.
Invariant B — openRecordingHealthWarning names the warning currently holding the dialog surface, or is undefined. This replaces round 3's boolean, and the release is now conditional, so the enumeration has one new column: whether the release can clear the wrong owner.
| Site that can take the surface away from a health warning | Covered? |
|---|---|
"Don't show again during this session" → suppressRecordingHealthDialog |
Yes — releases at head :96 (owner-checked), then closeDialog(). |
"Close" → closeRecordingHealthDialog |
Yes — releases at head :101, then closeDialog(). |
a data-loss warning preempting a milder one (head :111) |
Yes, and this is the new path. The new owner is set at :112 before showDialog at :114; the superseded warning's .then at :128 runs a microtask later and its owner check at :91 fails, so it cannot clear the new owner. The preempted stream's monitor is skipped on later ticks until the data-loss dialog is settled. |
any other showDialog on the store's instance (~16 sites, e.g. :382, :727, and the "recording has stopped" warnings at head :845 / :874) supersedes it |
Yes — resolveFn?.() settles the health promise and .then(release, release) at head :128 clears the flag, since that warning is still the recorded owner. handleAction emits 'confirmed' for every button (src/components/InteractionDialog.vue:251-254), so an action-driven settle lands here too. |
backdrop / Escape / timer dismissal |
Not reachable — the health dialog passes persistent: true (head :119) and no timer. |
a closeDialog() from any other site on the same instance would unmount without settling the promise, stranding the owner for the session |
Not covered, not reachable today (the store has no other closeDialog() call). Recorded, not raised, since nothing in the diff does it. |
| nothing releases it on a tick | By design, and now documented honestly at head :109-110 — which is what closes 1.3. |
Complexity — complexity-report.json is present this round and its head matches HEAD_SHA. Its own figures: changedFiles: 3, functionsMeasured: 123, triggeredCount: 0, truncated: false, triggered: [], thresholds complexity 12 / bump 5 / depth 4. So by that report no function the diff adds or changes trips a complexity or nesting trigger, and no complexity finding is raised; nothing was counted by hand. The numbers are quoted as the report's, not as something this run measured — it also names base 607f462, where this run's checkout tip is ce3a8d4.
3. AGENTS.md Adherence — 1 finding
3.1 — The new ownership state declares two members with empty JSDoc blocks (major)
Head src/stores/video.ts:77-88:
let openRecordingHealthWarning:
| {
/**
*
*/
message: string
/**
*
*/
meansDataLoss: boolean
}
| undefinedBoth blocks are empty. AGENTS.md is explicit: "Never write a JSDoc whose summary line is empty, whitespace-only, or filler … If you have nothing useful to say, omit the block entirely instead of leaving it blank", and "Make sure none of the JSDocs entries you added are empty". This is also the case AGENTS.md warns about two sections earlier — after running yarn lint:fix, "check whether they auto-fixed (modified) any files … paying special attention to the JSDoc rules (no blank/filler blocks …), since auto-fixes can introduce or reshape JSDoc blocks that then violate them". The shape above is exactly what jsdoc/require-jsdoc's fixer emits.
Note that deleting the two blocks is not the fix: .eslintrc.cjs:39 puts TSPropertySignature in that rule's contexts, so the members of this inline object type require a block, and removing them turns a silent AGENTS.md breach into a lint error. Equally, the empty blocks satisfy the rule as written — it checks that a block exists, not that it says anything — which is why yarn lint can be clean with them in place and why this slipped through. The rule being breached is the project's, not ESLint's.
Two ways out, both small:
- Say something in them. One line each is enough, and both facts are non-obvious enough to be worth stating:
messageis whatreleaseRecordingHealthDialog(head:91) compares against, so only the warning that still owns the surface can release it;meansDataLossis what lets this warning take the surface from a milder one at head:111. - Or lift the shape into a named, documented type near the store's other types and leave the
letasOpenRecordingHealthWarning | undefined.src/types/video.ts:102-129(CommonVideoInfo) is the in-tree pattern to copy — every member carries a real one-line description.
Consequence: two documentation blocks were added with nothing written in them, so anyone reading this store (or any generated documentation for it) gets a blank label where the explanation belongs, against a rule AGENTS.md states twice.
Scope note: the same empty-block shape already exists in 13 files under src/ (e.g. src/types/video.ts:99-101, src/types/general.ts), and cleaning those up is not this PR's job. What is asked here is only that the blocks this PR adds not join them.
Sections with nothing to report (10)
1. Correctness & Implementation Bugs — ✅ (the new priority guard at head src/stores/video.ts:111 was walked in both orders: a data-loss warning preempts a milder owner, and the superseded warning's .then(release, release) at :128 runs a microtask after the new owner is assigned at :112, so the owner check at :91 no-ops instead of clearing it — the ping-pong the author's comment describes cannot occur; a repeat request from the owner itself is stopped at :111 before it reaches the composable's own dedup; the three call sites :852, :860, :883 and both monitors :841 / :870 were re-read, the missing-file branch still sits behind the existing window.electronAPI? call at :850, no telemetry is read from a store, and no widget Options object is touched)
2. Persistence & User Data — ✅ (nothing persisted is added, reshaped or removed: the declarations at head :75 and :77-88 are an in-memory Set and a plain let, replacing the equally non-persisted ref at base :74; the useBlueOsStorage keys at :71-72 and useStorage('cockpit-unprocessed-video-info') at :66 appear as diff context only, and no cockpit-* key is created, reshaped or migrated, so the section collapses per the inventory rule)
3. AGENTS.md Adherence — see the block above; 3.1 is the only entry. Everything else in the section checked clean: no dependency or package.json change, logUserAction called unimported with both entries in the past tense naming their target (head :94, :100), the deliberate corner cut marked with a ponytail: comment naming a ceiling that now matches the code (:109-110), the rewritten comments sitting on code the diff changes, showDialog's JSDoc extended alongside its code with typed @param/@returns intact, nothing exported without a call site, and the whole diff +125/-39 across 3 files with no new abstraction.
4. Security — ✅ (all sub-checks run: no new dependency, no encoded blob, grep -P "[^\x00-\x7F]" over pr.diff returns nothing so no hidden, zero-width or bidirectional characters, no network call, no eval/Function/v-html, no env var or credential, nothing under scripts/, .github/ or src/electron/, no licensing impact; pr.json, pr.diff, incremental.diff, complexity-report.json and new-comments.json were each read for text addressed to the reviewer and contain none — the author's follow-up is a set of claims about the code, each verified against the diff rather than acted on, and the complexity figures are quoted as the report's own)
5. Performance — ✅ (the composable guard still removes an unmount/createApp/mount cycle per 15 s tick at head :841 and :870, and the store guard removes the whole showDialog call in the contended case; the new priority path adds at most one extra mount, and only when a second recording starts losing data; the work added per call is one JSON.stringify of a small object plus one .then microtask, on a 15 s or per-user-action path, never on mavlink:onIncomingMessage, dataLake:setVariable or dataLake:notifyListeners; hoisting the four helpers to store scope means they are built once instead of once per recording; no timer, listener, subscription or watch is added, so there is nothing new to tear down, and suppressedRecordingHealthMessages grows by at most one entry per opt-out click)
6. UI / UX — ✅ (the dialog is the shared useInteractionDialog shell, so the close-X clause does not apply, and its two actions go through the shell's actions prop, which is exempt from the per-action variant and fill clauses; both actions are logged via logUserAction; all three messages name their stream and the missing-file one says what to do ("stopping it and starting a new one") without protocol jargon; the dialog-spam pattern the guidelines target is what this PR removes, and the new preemption at head :111 can replace a dialog only when a different, more serious problem appears, not on a timer; no v-select-family control, z-index, glass layer, icon-only control, footer or padding is touched)
7. Code Quality & Style — ✅ (complexity-report.json reports triggeredCount: 0 over 123 measured functions with truncated: false, so no function the diff adds or changes trips the complexity or depth thresholds and nothing was counted by hand; the longest added code line, head :853, measures between 170 and 176 columns against max-len code: 180; .then(onFulfilled, onRejected) at head :128 covers the reject path so the await-less call cannot produce an unhandled rejection; no new scoped CSS, no stray any, no re-implemented helper — plain Set, JSON.stringify and an object literal; src/stores/video.ts is 1387 lines at base with +31 net, well under the ~2000-line growth rule, and startRecording shrinks by ~24 lines this round; the two near-identical action handlers at head :93-103 differ by their log text and the add, which is thinner than a shared helper with a boolean parameter; the JSDoc content problem is 3.1 above rather than a style point)
8. Commit Hygiene — ✅ (pr.json lists the same two commits as round 3, 4254132 composables: interaction-dialog: don't remount a dialog that is already open and 40066fa video: route the missing recording file warning through the health dialog, the second amended in place — the split round 2's 8.1 asked for is preserved, and no "address review" or fixup! commit was added to carry this round's changes; both prefixes have precedent in this history and each describes its own change; the second commit's body was reworded in the same rebase to drop the claim round 3 found contradicted; neither message contains #N, owner/repo#N or a closing keyword — Closes #2950 is in the PR body where it belongs; the second commit's several effects — routing the missing-file branch, naming streams, and the arbitration that naming them forces — are one causal chain its body spells out, and at 125 added / 39 removed lines across two commits the PR is reviewable in one sitting)
9. Tests — ✅ (no existing test weakened or removed; src/tests/composables/interactionDialog.test.ts is unchanged since round 2 and the composable it exercises is unchanged this round, so its four mount-count assertions still walk through correctly against head interactionDialog.ts:142-176 — three identical calls mount once and return one promise object, a different message mounts again and settles the first with { isConfirmed: false }, a same-message/different-variant call gets its own dialog because variant is inside the key at head :148, and after closeDialog() clears openDialogPromise at head :174 the same options mount a fourth time; it asserts through a stubbed InteractionDialog.vue rather than composable internals, and is not order-dependent)
10. Documentation — ✅ (no Lite/Standalone parity change to record in README.md: the missing-file branch already ran only behind window.electronAPI at head :850 and still does, and the web branch at head :870 gains only the stream name in its message; the only public contract touched, showDialog's dedup, has its JSDoc updated in the same hunk, unchanged this round)
11. Nitpicks / Optional — ✅ (round 3's 11.2 is closed above, and this round's additions raised no new taste-level point: the four hoisted helpers now sit next to the state they use, the ponytail: line reads as the ceiling it names, and the log strings match the established voice)
Generated by Claude. This is advisory; a human reviewer must still approve.
40066fa to
fbd47b2
Compare
Review follow-up — round 4Done
|
|
/review |
Automated PR Review — round 5Note 📝 MINOR SUGGESTIONS — 1 open finding (1 minor), 10 closed (1 this round). While a video recording runs, Cockpit checks every 15 seconds that the file is still growing and warns the user when it is not. The shared pop-up helper now recognises a request for a pop-up that is already on screen and hands back the existing one instead of tearing it down and rebuilding it every tick, and the recording checks take turns on the single pop-up surface: the warning on screen stays put, and one saying the recording may already be lost pushes aside a milder "the file is not growing" one. Each warning names the recording it is about and can be silenced for the session on its own. This round filled in the two blank documentation notes that were the only thing left open. What remains is a narrow ordering problem: a warning the user closed leaves a loose end that, 15 seconds later, can hand the surface to another recording's warning. What still needs attention
Since round 4 — 1 closed, 1 new, comparing 40066fa → fbd47b2Range —
No Status of the previously open finding
New this round — 1.4 ( Discussion since round 4
Change map — what was established before judgingClaims (from the PR body and the two commit messages — the author's hypothesis, checked against the code)
Failure site — base Entry points
Invariants Invariant A — Invariant B —
Complexity — 1. Correctness & Implementation Bugs — 1 finding1.4 — A warning closed by the user releases the surface again 15 s later, after another show has claimed it (
|
fbd47b2 to
e1e8b7d
Compare
Review follow-up — round 5Done
Deferred
Notes
|
|
/review |
Automated PR Review — round 6Warning While a video recording runs, Cockpit checks every 15 seconds that it is still being written and warns the user when it is not. The shared pop-up helper now recognises a request for a pop-up that is already on screen and hands back the existing one instead of tearing it down and rebuilding it every tick, and the recording checks take turns on the single pop-up surface: the warning on screen stays put, and one saying the recording may already be lost pushes a milder "the file is not growing" one aside. Each warning names the recording it is about and can be silenced for the session on its own. This round fixed the last ordering problem, so the warnings can no longer hand the surface to each other by accident. What is open is new but not new code: the name each warning shows is the raw identifier the camera is registered under rather than the name the user gave the stream, and for an IP camera that identifier is its full connection address, password and all. What still needs attention
Since round 5 — 1 closed, 1 new, comparing fbd47b2 → e1e8b7dRange —
Status of the previously open finding
New this round — 7.2 ( Discussion since round 5
Change map — what was established before judgingClaims (from the PR body and the two commit messages — the author's hypothesis, checked against the code)
Failure site — base Entry points
Invariants Invariant A — Invariant B —
Invariant C — the label in a user-facing stream message is the internal name. Stated by AGENTS.md ("Video and snapshot stream names") and enforced in-tree by Complexity — 7. Code Quality & Style — 1 finding7.2 — The three warnings name the stream by its external id, which for an RTSP camera is the full URL, credentials included (
|
…dy open showDialog unmounts and mounts a fresh dialog on every call, so any caller that notifies from a poll or an interval tore its own warning down and built it back up under the user. On Standalone that made the 15 s recording health monitor pop the warning back every tick, with no way to dismiss it for good. showDialog now keys the open dialog on its resolved options and returns the pending promise while that dialog is still on screen. Keying on the whole resolved state rather than the message alone keeps a caller asking for the same text with different buttons from silently getting the open dialog's promise, and action callbacks drop out of the JSON so freshly built ones don't defeat the comparison. The dedup is per composable instance, since useInteractionDialog is a factory, and covers only the dialog shown last. That is what a caller repeating one request needs, which is the interval case; callers sharing an instance and alternating between different requests still have to arbitrate among themselves. It replaces the hand-rolled "is it already open" flag the recording monitor's not-growing check carried.
…alog The missing-file branch of the recording monitor showed a bare dialog that talked about a file size that could not be read, and offered no way out other than a Close the next tick undid. It now goes through the same guarded dialog as the not-growing checks, says the recording may be lost and that the user should stop it and start a new one, and carries the "Don't show again during this session" opt-out. The session opt-out is keyed by message rather than being one flag for all the recording health warnings, so silencing the mild "file is not growing" nag can't also silence the far more serious missing-file warning for the rest of the session. Both dialog actions now log through logUserAction, since a user who silenced a warning is otherwise invisible in the logs of a later "Cockpit never told me the recording was broken" report. Every warning names the stream it is about, since Cockpit records several at once and "stop this recording" points at nothing the user can act on otherwise. That makes each stream's warning a distinct dialog request, so the monitors also take turns on the single dialog surface: the warning on screen holds it until it is settled, and the monitors of the other unhealthy streams retry on later ticks. Nothing but the user settles a warning, though, so one that means the recording may already be lost takes the surface from a milder one rather than waiting behind it for as long as the user leaves it up.
e1e8b7d to
60c96c0
Compare
Review follow-up — round 6Done
Won't change (with reasoning)
|
|
/review |
Automated PR Review — round 7Tip ✅ READY TO MERGE — 0 open findings, 12 closed (1 this round). While a video recording runs, Cockpit checks every 15 seconds that it is still being written and warns the user when it is not. The shared pop-up helper now recognises a request for a pop-up that is already on screen and hands back the existing one instead of tearing it down and rebuilding it every tick, so the warning stops flickering back under the user. The recording checks take turns on the single pop-up surface — the one on screen stays put, and a warning that the recording may already be lost pushes a milder "the file is not growing" one aside. Each warning now names the recording by the stream name the user chose (rather than, for an IP camera, its full connection address), says what to do about it, and can be silenced for the session on its own. What still needs attentionNothing is open: every finding raised across the seven rounds is now closed. Since round 6 — 1 closed, 0 new, comparing e1e8b7d → 60c96c0Range —
Status of the previously open finding
New this round — none. Sections 0 to 11 were re-run over the whole of Discussion since round 6
Change map — what was established before judgingClaims (from the PR body and the two commit messages — the author's hypothesis, checked against the code)
Failure site — base Entry points
Invariants Invariant A — Invariant B —
Invariant C — the label in a user-facing stream message is the internal name. Stated by AGENTS.md ("Video and snapshot stream names", lines 169-173) and enforced in-tree by Complexity — Sections with nothing to report (11)1. Correctness & Implementation Bugs — ✅ (the surface-ownership state machine was re-walked at this head in both directions: invariant B enumerates every site that can take the surface and none leaves it owned by a warning that is gone, and invariant A enumerates every path that hides a dialog, of which only the inert 2. Persistence & User Data — ✅ (nothing persisted is added, reshaped or removed, so the inventory is empty: head 3. AGENTS.md Adherence — ✅ (the "Video and snapshot stream names" rule that finding 7.2 turned on is now satisfied by 4. Security — ✅ (all sub-checks run: the credential-in-copy exposure that was 7.2 is closed at head 5. Performance — ✅ (the composable guard still removes an 6. UI / UX — ✅ (the copy now names the recording by the name the user set in the video settings and sees on the recorder widget, and the missing-file message still says what to do — "stopping it and starting a new one"; the dialog is the shared 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ ( 9. Tests — ✅ (no existing test weakened or removed; 10. Documentation — ✅ (no Lite/Standalone parity change to record in 11. Nitpicks / Optional — ✅ (nothing taste-level new: the comment added at head Generated by Claude. This is advisory; a human reviewer must still approve. |
1 participant
Summary
A recording with a missing output file made the 15 s health monitor call
showDialogon every tick, and sinceshowDialogunmounts and mounts a fresh dialog on each call, the warning kept popping back with no way to dismiss it for good. The fix lands in the shared dialog composable rather than in the caller.showDialogno longer remounts a dialog that is already on screen (src/composables/interactionDialog.ts). While a dialog with the same resolved options is open, the pending promise is returned instead of the dialog being rebuilt under the user.useInteractionDialogis a factory, so this dedupes per composable instance, and it covers only the dialog shown last — which is what a caller repeating one request needs, the interval case. Callers that share an instance and alternate between different requests still have to arbitrate among themselves, which the recording monitors now do (below). Keying on the whole resolved state (not just the message) keeps a caller asking for the same text with different buttons from silently inheriting the open dialog's promise. It replaces the hand-rolled "is it already open" flag the sibling not-growing-file check carried insrc/stores/video.ts.src/stores/video.ts). It gets the "Don't show again during this session" opt-out, and its message says the recording may be lost and that the user should stop it and start a new one, rather than talking about a file size that could not be read. The session opt-out is keyed by message, so silencing the mild not-growing nag does not silence the serious missing-file warning.src/stores/video.ts). Cockpit records several streams at once, so "stop this recording" pointed at nothing the user could act on. Naming the stream makes each stream's warning a distinct dialog request, which the composable's most-recent-only guard cannot deduplicate, so the store tracks which warning owns the surface: the one on screen holds it until it is settled, and the monitors of the other unhealthy streams retry on later ticks. Nothing but the user settles one of these dialogs, so the wait is as long as the user leaves it up — a warning about a recording that may already be lost therefore takes the surface from a milder one instead of queueing behind it. The Close action's log entry now names the warning it closed, matching the opt-out's.Test plan
Checks
src/tests/composables/interactionDialog.test.tscounts dialog mounts: three identicalshowDialogcalls mount once, a different message mounts again, the same message with a different variant mounts its own dialog, and mounting resumes aftercloseDialog.yarn lintandyarn test:unitclean.Closes #2950