feat: warn when heavy traffic goes through the vehicle's wireless link by rafaellehmkuhl · Pull Request #2955 · bluerobotics/cockpit

rafaellehmkuhl · 2026-08-19T17:53:25Z

Summary

A vehicle reached over WiFi streams its video over WiFi, even with a cable attached. Cockpit cannot move the media to the cable, so it now tells the user about it.

Two things have to hold before the warning shows up:

the vehicle is currently reached over a wireless address while a cabled one is also available, as reported by the beacon. This is the same source the video store already uses to pick the preferred stream routes, so an operator already on the cable, and a vehicle with no cable at all, stay quiet.
a wireless interface has carried at least 5 Mbps of upload across every ten-second stretch of the last forty seconds. The stretches are taken from the readings themselves rather than from fixed timestamps, so a link that loses readings still produces a verdict, as long as the readings still inside the forty seconds span thirty of them, which any reading cadence up to a third of the history guarantees. The rate comes from how far the vehicle's transmitted-byte counter moved across each stretch, not from what each poll reported: the vehicle refreshes those counters slower than Cockpit polls them at 1 Hz, so the per-poll rate reads zero most of the time and spikes on the polls that catch a refresh, which no per-sample statistic tells apart from an idle link. Every stretch rather than the history as a whole because Cockpit pulls map tile archives and files from the vehicle's file storage over this same link, and a single large transfer moves enough bytes to clear the threshold over a long span while lasting only seconds — it would spend the session's one warning on itself. Any transfer shorter than a stretch leaves an idle stretch for the verdict to fail on, so it has to last longer than ten seconds to pass. That only holds once the history spans three stretches, which is what the first verdict waits for, so sustained traffic warns around 31 s in.

The beacon is only asked once the traffic condition holds, and at most once every 30 s from there, so a cable plugged in or pulled mid-session changes the answer.

The warning is a 15 s snackbar, given once per session — acting on it means changing the vehicle address, which reloads Cockpit anyway. No UI was added — it uses the existing snackbar.

An address the beacon does not report, such as the default blueos-avahi.local host name, leaves the current link kind undetermined, and in that case nothing is warned.

The plotted network speeds, fixed in the second commit

The upload speed (Mbps) and download speed (Mbps) data-lake variables were computed from a single poll-to-poll delta, which is the very thing the counter-refresh diagnosis above says is meaningless: they read as zeros and spikes on a busy link just as much as on an idle one, which is exactly what the plots earlier in this thread show.

That defect is pre-existing, but it is the number an operator reaches for to see what the link is doing, and the one anybody verifying this feature would plot, so it is fixed here rather than left for later — in its own commit, since it changes existing behaviour. Both speeds are now measured across the readings of the last ten seconds, which spans several counter refreshes, and the counter-delta-to-Mbps conversion lives in one helper shared with the warning's own history. The minimum span lives in that helper too, so the polls right after connecting publish no traffic instead of the sawtooth they have too little history to look past, and a gap in the readings is measured against the newest reading available instead of leaving the last rate published as if it were still current.

Test plan

Connect to a vehicle over WiFi, by IP, with a stream running, and confirm the warning shows up once and is not repeated for the rest of the session.
Connect to the same vehicle over the cabled network and confirm no warning shows up.
Connect to a vehicle with no cable attached and confirm no warning shows up.
With no cable attached and a stream running, plug the cable in and confirm the warning shows up shortly after.
Pull a large file off the vehicle over WiFi with no stream running — a custom map tile archive, which is multi-MB by design — and confirm no warning shows up.
Plot the wireless and cabled upload speed (Mbps) variables with a stream running and confirm they now hold a steady rate instead of alternating between zero and a spike.

Checks

yarn lint:fix clean, and vitest run passes 40 tests. cosmos.test.ts and connection.test.ts fail to collect the same way on master.
yarn typecheck is broken on my machine, on master as well — vue-tsc bails on src/App.vue and exits 0 even with a deliberate type error — so I checked the changed files with tsc directly and diffed the errors against the same run without this branch's changes. No new ones.
src/tests/libs/wireless-traffic-warning.test.ts covers the trigger, the once-per-session latch, traffic below the threshold, readings arriving five seconds apart, a busy link whose counter only refreshes every few polls, a single large transfer spread over several readings on an idle wireless link, and the four link-kind cases of the beacon check.
src/tests/libs/blueos.test.ts covers the shared rate helper: a counter that only refreshes every few seconds, a span too short to cover a refresh — both when it reads no progress and when it catches one — a counter reset by a vehicle reboot, and an empty span.

Closes #2953
Closes #2971

github-actions · 2026-08-19T18:04:35Z

Automated PR Review — round 1

Warning

⚠️ IMPORTANT FIXES REQUIRED — 6 open findings: 1 major (1.1), 3 minor and 2 nits.

The existing once-per-second poll of the vehicle's network counters now also feeds a small in-memory history of per-interface upload rates. When the busiest wlan* interface has held a median of at least 5 Mbps for a full ten-second window, and the busiest eth* interface is not carrying ten times as much, a 15-second warning snackbar tells the user to connect over the cable instead. It stays quiet for five minutes afterwards. Nothing is persisted and no UI surface is added.

What still needs attention

#	Problem	What it means	Severity	Status
1.1	Warning fires without checking who is on the WiFi, or whether a cable exists	Users already plugged into the cable, and users whose vehicle has no cable at all, can be told every five minutes to plug in a cable.	major	❌
1.2	Warning needs 8 of the last 10 polls to succeed	On the badly congested links the feature exists to catch, the checks that detect congestion get dropped and the user is never warned.	minor	❌
7.1	Third hand-written median in the tree	Extra code to maintain that an already-installed library and an existing copy in the app both already provide.	minor	❌
7.2	Interface-kind naming rule duplicated	Two files independently decide what counts as WiFi versus cable, so a future change to one silently breaks the other.	minor	❌
11.1	Redundant ten-second gate	A second timing check that can never change the outcome, which the next reader has to work out.	nit	❌
11.2	Test assertion in the wrong test	A reader looking for the WiFi-threshold test will not find it where its name says it is.	nit	❌

Change map — what was established before judging

Claims

"The per-interface upload rates that already feed the data lake … now also feed a watcher" — verified. uploadMbpsPerInterface is filled from the same Math.max(0, uploadSpeedMbps) value that goes into the data lake (src/stores/mainVehicle.ts:899-907 in base, rewritten by the diff) and handed to shouldWarn at the end of the same forEach pass.
"refreshed at 1 Hz" — verified with a qualification. The producer is setInterval(async …, 1000) at src/stores/mainVehicle.ts:836-921. Each round awaits getStatus, getCpuTempCelsius, getCpusInfo and getNetworkInfo in sequence, and a failed getStatus (3 s timeout, src/libs/blueos.ts:21) returns from the round before any network reading is taken (src/stores/mainVehicle.ts:839-845). So 1 Hz is a ceiling, not a floor — this is what finding 1.2 rests on.
"warns when the wireless interface holds a median of at least 5 Mbps over a 10 s window while no cabled interface carries ten times that" — verified, src/libs/wireless-traffic-warning.ts:78-83.
"a full window of readings is required before any verdict, so a stalled BlueOS request cannot warn on thin data" — verified (minSamplesPerWindow = 8, plus the wall-clock gate at line 67, which is redundant — nit 11.1). The same rule is what silences the feature under congestion (1.2).
"a vehicle with no cabled interface" stays quiet — contradicted on real hardware. The guard is cabledMedian === undefined, which requires no eth* interface to appear in the readings at all. getNetworkInfo filters only on the name (src/libs/blueos.ts:336-338, dropping nothing but v*), and RawNetworkInfo carries is_up and ips (src/types/blueos.ts:29-48) precisely because interfaces that are down are reported too. A Pi with nothing plugged into eth0 still reports eth0, at 0 Mbps, which passes the dominance check rather than suppressing the warning. The unit test's { wlan0: 6 } shape is not what the endpoint returns.
"there is nothing Cockpit can do to move the media … binding go2rtc to a specific address needs OS-level access on the vehicle" — not checkable from this checkout; it is about a vehicle-side service. Nothing in the diff depends on it being true beyond the decision to warn rather than act.
Unstated premise: that the traffic seen on wlan* is this Cockpit's traffic. The code never establishes it — see 1.1.

Entry points

Function	Reached from	Frequency
`createWirelessTrafficWatcher` (`src/libs/wireless-traffic-warning.ts:46`)	`VehicleFactory.onVehicles.once` handler inside `useMainVehicleStore` (`src/stores/mainVehicle.ts:632`, new line at 784)	one-shot
`shouldWarn` closure (`src/libs/wireless-traffic-warning.ts:52`)	the 1 Hz `setInterval` in that same handler (`src/stores/mainVehicle.ts:836-921`)	per incoming message (one BlueOS network poll response per second)
`busiestMedian` (`:71`), `median` (`:36`), `isWireless`/`isCabled` (`:33-34`)	`shouldWarn` only	per incoming message
the interval callback (`src/stores/mainVehicle.ts:836`)	timer created once from the `.once` handler	per incoming message
`feedSeconds` (`src/tests/libs/wireless-traffic-warning.test.ts:164`)	vitest	one-shot (test only)

Cost is negligible at that frequency: two or three interfaces × ten samples, one sort per interface per second. The openSnackbar on that path is behind the five-minute cooldown, so the timer cannot spam it.

Invariants

Readings arrive at ~1 Hz. minSamplesPerWindow = 8 over a 10 s window encodes it. Violators: an early return on a failed BlueOS status check (src/stores/mainVehicle.ts:839-845), a throwing getNetworkInfo (:918-920), and the first round of each interface, which has no previous reading so contributes no rate (:885-909). None are covered; all fail towards silence (1.2).
Interface kind is derivable from the name. wlan/eth substrings, src/libs/wireless-traffic-warning.ts:33-34. The only other site holding that convention is src/libs/blueos.ts:337; nothing keeps the two in sync, and src/stores/video.ts:1140 expresses the same idea a third way, as beacon interface types ['WIRED', 'USB'] (7.2).
Watcher state may live for the app's lifetime. Checked: it can. The handler is .once (src/stores/mainVehicle.ts:632), so exactly one watcher and one interval exist. That is safe across vehicle switches because changing the address reloads Cockpit (src/views/ConfigurationGeneralView.vue:639-643, src/components/VehicleDiscoveryDialog.vue:210), so no stale samples or stale cooldown can carry from one vehicle to the next.

1. Correctness & Implementation Bugs — 2 findings

1.1 — The warning fires without establishing either of the two things it asserts — major
Consequence: an operator already connected through the cable, or one whose vehicle has no cable at all, can be told every five minutes to plug in a cable.

src/libs/wireless-traffic-warning.ts:78-83 decides from vehicle-side byte counters alone. Two premises of the message ("your connection is the WiFi one" and "a cabled network is available") are never checked:

Whose traffic it is. Vehicle wlan0 upload above 5 Mbps means something is pulling data over the vehicle's WiFi — a second operator on a tablet, a phone on the vehicle's hotspot, a BlueOS extension, an upload to shore. A Cockpit sitting on eth0 at 4 Mbps while another client streams 6 Mbps over WiFi satisfies every condition (4 >= 10 * 6 is false) and gets the warning, advising a cable it is already using.
Whether a cable exists. cabledMedian === undefined only holds when no eth* interface is reported at all. As established in the Change map, an unplugged eth0 is still reported, at 0 Mbps, which passes the dominance test. So the WiFi-only case the PR body says is excluded is in fact the case that warns.

Cockpit already determines the wired/wireless question directly, from the beacon: getIpsInformationFromVehicle (src/libs/blueos.ts:238-248) returns each reachable address with its interfaceType, and src/stores/video.ts:1133-1144 uses exactly that to set currentlyOnWirelessConnection and to decide which addresses are tethered (['WIRED', 'USB']). Beacon only lists interfaces BlueOS is actually reachable on, so the same call answers both premises: warn only when the address in globalAddress maps to a non-tethered interface and a tethered address exists. That determination is currently inline in the video store; lifting it into a small helper (next to getIpsInformationFromVehicle, or in src/libs/) and calling it from both places is smaller than the traffic heuristic it would gate, and removes the guesswork rather than tuning it. Note the known limit of that comparison, which the video store shares: it matches globalAddress against an IPv4 address, so an mDNS host name (blueos-avahi.local, src/assets/defaults.ts:145) resolves to nothing it can compare — decide explicitly what the gate does in that case, and prefer silence over a warning Cockpit cannot substantiate.

1.2 — Requiring 8 of the last 10 polls makes the feature quietest exactly when it should speak — minor
Consequence: on the badly congested links this feature exists to catch, the warning may never appear.

minSamplesPerWindow = 8 (src/libs/wireless-traffic-warning.ts:31) is checked per interface per window (:73). The samples come from a round that returns early whenever the BlueOS status check fails on its 3 s timeout (src/stores/mainVehicle.ts:839-845) or getNetworkInfo throws (:918-920) — losses that get more likely as the link saturates, since the poll shares that link. Three lost rounds in ten seconds and the median is discarded, indefinitely, with no output at all. Prefer a rule that degrades instead of switching off: require a minimum span of readings (say first and last sample at least 6 s apart) with a lower count floor, so a lossy link still produces a verdict from the samples that did arrive.

7. Code Quality & Style — 2 findings

Complexity: the report for this head lists 116 measured functions across the 3 changed files, truncated: false, and no triggered entries, so there is no complexity finding to raise. Lint: checked the added lines against .eslintrc.cjs — arrow bodies satisfy func-style with allowArrowFunctions, every added function carries an explicit return type for @typescript-eslint/explicit-function-return-type, no any, no semicolons, no line over 180 characters, and the one jsdoc/require-jsdoc disable on the local RateSample type follows the in-tree precedent at src/libs/blueos.ts:184.

7.1 — median is the third copy of the same function — minor
Consequence: one more small piece of arithmetic to keep correct in a codebase that already has it twice.

src/libs/wireless-traffic-warning.ts:36-40 re-implements calculateMedian from src/components/widgets/Plotter.vue:507-518, and mathjs — a dependency already in package.json and already imported in src/libs/vehicle/mavlink/vehicle.ts:2 and elsewhere — exports median. Per the AGENTS.md ladder (rung 2 then rung 5), either use mathjs's median, or put one median in src/libs/utils.ts next to round/constrain and have both call sites use it. The local version is correct for its inputs (samples.length >= minSamplesPerWindow guarantees a non-empty array), so this is duplication only.

7.2 — The "what is WiFi, what is a cable" rule now lives in three shapes — minor
Consequence: a vehicle whose interfaces are named differently, or a change to one of the three places, silently turns the feature off or makes it disagree with the rest of the app.

isWireless/isCabled (src/libs/wireless-traffic-warning.ts:33-34) restate the substring convention already in src/libs/blueos.ts:337, while src/stores/video.ts:1140 classifies the same interfaces by beacon type and counts USB as cabled. The practical consequences: a USB-tethered operator is "not cabled" here but tethered there, and a BlueOS host with predictable interface names (enp3s0, wlp2s0) is filtered out upstream so the watcher never sees a reading. Put the classification in one place — the natural one being alongside the beacon interface-type helper that finding 1.1 asks for — and have getNetworkInfo's filter and this module share it.

11. Nitpicks / Optional — 2 findings

11.1 — The wall-clock warm-up gate cannot change any outcome — nit
firstSampleTimestamp and the timestamp - firstSampleTimestamp < analysisWindowMs return (src/libs/wireless-traffic-warning.ts:48, 66-67) are subsumed by the per-interface samples.length >= minSamplesPerWindow check at :73: eight samples cannot exist inside a 10 s window before ~7 s of readings, and after the first window the gate is permanently true. Dropping both lines removes a piece of state and a branch without changing behaviour.

11.2 — Wireless-threshold assertion sits inside the cabled-link test — nit
src/tests/libs/wireless-traffic-warning.test.ts:187 ({ eth0: 0, wlan0: 4.9 }) tests the 5 Mbps floor, not the cable dominance the test name describes. It belongs in its own test('stays quiet below the busy threshold').

Sections with nothing to report (8)

2. Persistence & User Data — ✅ (no persisted key is added, reshaped or removed; the watcher's history is closure-local, the data-lake variables it reads are registered persistent: false, persistValue: false at src/stores/mainVehicle.ts:829-834, and no useBlueOsStorage/settings-management call appears in the diff, so the inventory is empty)

3. AGENTS.md Adherence — ✅ (package.json untouched, so no dependency-ordering question; both exports have call sites in this PR — the store at mainVehicle.ts:785 and the test — so no groundwork; the added JSDoc on WirelessTrafficWatcher.shouldWarn and createWirelessTrafficWatcher carries typed @param/@returns with non-empty bodies per jsdoc/require-returns; the pure heuristic went to src/libs/ rather than into the store, per Separation of concerns; the only reflow, mainVehicle.ts:899-907, is the line the extracted nonNegativeUploadMbps required, so no scope creep)

4. Security — ✅ (no new dependency, no new network call — the watcher consumes the existing getNetworkInfo response — no encoded blobs, no hidden Unicode in the added identifiers, no eval/Function/v-html, no window.electronAPI or Electron-only module so the Lite build is unaffected, and no build script, workflow or src/electron/ file is touched; the PR body and diff contain no text addressed to the reviewer)

5. Performance — ✅ (shouldWarn traces only to the pre-existing 1 Hz BlueOS poll, per the entry-point table, and does one sort of ~10 numbers per interface per second; no listener, watcher, interval or timeout is registered by the diff, so there is no new teardown to require — the un-cleared interval at mainVehicle.ts:836 is inherited — and the snackbar's dismissal timer lives in src/composables/snackbar.ts:54-56)

6. UI / UX — ✅ (no dialog, overlay, footer or Vuetify control is added, so the dialog-anatomy, theme="dark", button-token and padding rules do not apply; the snackbar goes through the shared openSnackbar with a valid variant/duration/closeButton combination for SnackbarOptions, the five-minute cooldown is the guard against repeat-opening from a timed loop, the copy names no protocol or internal id and says what to do, and logUserAction is not owed since the diff adds no user interaction)

8. Commit Hygiene — ✅ (one commit, 7ecce51d, whose feat: prefix matches the mix of conventional types and scope prefixes in recent history and does describe this change; 142 added lines is reviewable as a unit; the body explains the go2rtc reasoning rather than restating the diff; no #N reference or closing keyword in the message, with Closes #2953 correctly confined to the PR body; nothing to squash, no stacked-PR commits)

9. Tests — ✅ (no existing test is removed or weakened; the added file only adds cases, and the heuristic was put in a framework-agnostic module which is what makes it testable at all — the one mis-homed assertion is nit 11.2, and the wrong model of the no-cable case is part of finding 1.1)

10. Documentation — ✅ (nothing here differs between Lite and Standalone — no Electron-only API is reached — so the README.md parity table needs no row; the added module's exported symbols both carry JSDoc)

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

rafaellehmkuhl · 2026-08-19T19:39:53Z

Review follow-up — round 1

Done

src/libs/wireless-traffic-warning.ts, src/stores/mainVehicle.ts (1.1 — warning fires without checking who is on the WiFi, or whether a cable exists): the traffic heuristic no longer decides on its own. canSuggestCabledLink reads the beacon addresses and only allows the warning when globalAddress maps to a non-tethered interface and a tethered address is also reported. You were right about the no-cable case being inverted: cabledMedian === undefined never held on real hardware, so the one case the body claimed was excluded was the one that warned.
src/libs/wireless-traffic-warning.ts (1.2 — 8 of the last 10 polls): replaced with a span rule, minAnalysisSpanMs = 6000 and a floor of 4 samples, so a lossy window still produces a verdict from what did arrive. New test still warns when part of the readings never arrive feeds one reading every 2 s and expects a warning.
src/libs/wireless-traffic-warning.ts (7.1 — third hand-written median): now median from mathjs, which was already a dependency.
src/libs/blueos.ts, src/stores/video.ts (7.2 — interface-kind naming rule duplicated): isWirelessInterfaceName and isCabledInterfaceName now live next to getNetworkInfo and its filter uses them, so the watcher and the filter cannot disagree. The beacon side is now isTetheredInterfaceType, shared with the video store, replacing its inline theteredInterfaceTypes array.
src/libs/wireless-traffic-warning.ts (11.1 — redundant ten-second gate): firstSampleTimestamp and its branch are gone.
src/tests/libs/wireless-traffic-warning.test.ts (11.2 — assertion in the wrong test): the 4.9 Mbps case is now its own stays quiet below the busy threshold.

Done differently

src/libs/wireless-traffic-warning.ts (1.1): the cable-dominance check and isCabled are deleted rather than fixed. Once the beacon tells us our own link is the wireless one, "is the cable busier" cannot change the answer, so it was a weaker proxy for something now measured directly. This dropped the stays quiet when the cabled link is the one carrying the traffic and stays quiet when there is no cabled link tests, both of which tested behaviour that moved to the beacon check and is covered by the new only suggests the cable when the vehicle is reached wirelessly and a cabled address exists.
src/libs/wireless-traffic-warning.ts (1.1): the shared beacon logic is a pure canSuggestCabledLink(ipsInfo, currentAddress) in the warning module, not a new module next to getIpsInformationFromVehicle. Only the two classification predicates went into blueos.ts, which keeps the fetching there and the decision unit-testable without mocking ky.
src/stores/mainVehicle.ts (1.1): the beacon is only consulted after the traffic condition is met, since it is a request of its own — at most one per cooldown. The consequence, noted in a comment there, is that the cooldown is consumed even when the link check rejects, so a warning can be delayed by one cooldown. It cannot repeat, because reaching the vehicle on another address reloads Cockpit.
mDNS: blueos-avahi.local matches no reported address, so the link kind is undetermined and nothing is warned. Silence over an unsubstantiated warning, as you suggested — see the question below.

Won't change (with reasoning)

7.1 (the other copy): calculateMedian in src/components/widgets/Plotter.vue is left alone. Deduplicating it means touching an unrelated widget in a review round, and it is not on any path this PR adds.
7.2 (predictable interface names): enp3s0/wlp2s0 are still filtered out upstream by getNetworkInfo, so the watcher never sees them. That is pre-existing and affects the four data-lake network variables the same way, so it belongs in its own change rather than here.

Questions for reviewers

src/libs/wireless-traffic-warning.ts (1.1): the mDNS gap is bigger than it looks, since blueos-avahi.local is the default address, so a large share of users will never see this warning. Silence is the safe call and it is what is implemented, but if we would rather reach those users, the alternative is to accept the traffic heuristic alone when the address cannot be classified, and eat the false positive of warning someone who is already on the cable. Happy to switch if you prefer reach over certainty.

rafaellehmkuhl · 2026-08-19T19:39:59Z

/review

github-actions · 2026-08-19T19:56:13Z

Automated PR Review — round 2

Note

📝 MINOR SUGGESTIONS — 1 open finding (1 minor); 6 closed since round 1.

Cockpit's once-a-second poll of the vehicle's network counters now also keeps a short history of how fast each interface is uploading. When a wireless interface has held a median of at least 5 Mbps for long enough, Cockpit asks the vehicle which addresses it can be reached on, and only if the address in use is the wireless one while a cabled one also exists does it show a 15-second warning suggesting the cable. It then stays quiet for five minutes. When the configured address is a host name the vehicle does not report — the default in the desktop build — the link cannot be classified and nothing is said. Nothing is persisted and no new UI is added.

What still needs attention

#	Problem	What it means	Severity	Status
1.3	The warning is lost when the check that gates it fails	If the vehicle does not answer the extra question Cockpit asks just before warning — most likely on the very congested connection this feature is about — the advice is silently postponed for another five minutes, and the failure is logged as if something else broke.	minor	❌

Since round 1 — 6 closed, comparing 7ecce51 → 913ebf7

Range. The branch was force-pushed: pr.json now lists a single commit (913ebf7) and 7ecce51 is no longer in its history, so incremental.diff came back as the whole PR instead of as an increment. It is therefore unusable for judging what moved, and every status below was worked out from pr.diff against the round-1 findings.

Resolutions. resolutions.json is empty — no /resolve has been banked on this PR, so nothing was closed by a maintainer this round and there are no unrecognised ids to report back.

Previous findings

1.1 — Warning fires without establishing that Cockpit is on the wireless link or that a cable exists — ✅ Addressed. The finding asked for three things and all three landed: the beacon now answers both premises (canSuggestCabledLink, src/libs/wireless-traffic-warning.ts:45-49, requires the current address to map to a non-tethered interface and a tethered address to exist), it is gated in the store before the snackbar (src/stores/mainVehicle.ts:927-938), and the unclassifiable-address case is decided explicitly towards silence (currentType === undefined returns false, tested at src/tests/libs/wireless-traffic-warning.test.ts:59). The inverted no-cable guard the finding was built on (cabledMedian === undefined) is deleted rather than patched, along with the cable-dominance comparison.
1.2 — Requiring 8 of the last 10 polls silences the warning on congested links — ✅ Addressed. minSamplesPerWindow is down to 4 and coversAnalysisSpan now judges a window by the span its readings cover (src/libs/wireless-traffic-warning.ts:29-34), which is the degrading rule the finding named; a link delivering one reading every 2 s still produces a verdict, per src/tests/libs/wireless-traffic-warning.test.ts:34.
7.1 — median re-implemented a third time — ✅ Addressed via the first of the two options the finding offered: median now comes from mathjs (src/libs/wireless-traffic-warning.ts:1), already a dependency and already imported by name elsewhere in the tree.
7.2 — Wireless/cabled classification duplicated across three sites — ✅ Addressed. Name-based classification is single-sourced at src/libs/blueos.ts:344 and :346 and consumed by getNetworkInfo's own filter, so the filter and the watcher can no longer disagree; beacon-type classification is single-sourced at src/libs/blueos.ts:255 and now used by both src/stores/video.ts:1145 and the watcher.
11.1 — Redundant wall-clock warm-up gate — ✅ Addressed; firstSampleTimestamp and its branch no longer exist.
11.2 — Wireless-threshold assertion in the cabled-link test — ✅ Addressed; it is now its own stays quiet below the busy threshold (src/tests/libs/wireless-traffic-warning.test.ts:30).

Discussion since round 1

rafaellehmkuhl's follow-up (comment) lists six fixes; each was checked against pr.diff rather than taken as given, and all six hold, which is what the statuses above rest on. The two deferrals it states are reasonable on this branch: calculateMedian in src/components/widgets/Plotter.vue is off every path this PR adds, and the enp3s0/wlp2s0 names dropped by getNetworkInfo's filter are pre-existing behaviour that affects the four network data-lake variables identically.
The same comment asks a genuine product question: because the default desktop address is the mDNS host name (src/assets/defaults.ts:145-146), the beacon gate leaves those users unwarned, and the alternative would be to warn on traffic alone when the address cannot be classified. Read as advice rather than instruction, keeping the silence is the better call — warning on traffic alone re-creates exactly the case finding 1.1 was about, telling an operator already on the cable to plug in a cable, and nothing currently in the tree can classify a host name (the video store's own wired-route selection shares the limitation, src/stores/video.ts:1145-1153). If reach matters more than certainty later, the signal that does not depend on the configured address is the ICE candidate pair actually selected for the stream, and that is its own change rather than a loosened gate here.
The second new comment is the bare /review that triggered this round; treated as a command, not as review input.

Change map — what was established before judging

Claims (from the PR body and commit message, each checked against the code)

"the vehicle is currently reached over a wireless address while a cabled one is also available, as reported by the beacon … the same source the video store already uses" — verified. canSuggestCabledLink (src/libs/wireless-traffic-warning.ts:45-49) reads the IpInfo[] from getIpsInformationFromVehicle (src/libs/blueos.ts:238-248) and classifies with isTetheredInterfaceType (src/libs/blueos.ts:255), the same predicate src/stores/video.ts:1145 now uses.
"a wireless interface has held a median upload of at least 5 Mbps over a 10 s window" — verified with a qualification. 10 s is the retention window (analysisWindowMs, src/libs/wireless-traffic-warning.ts:22); a verdict is admitted from as few as 4 samples spanning 6 s (:29-34). That is the intended loosening from finding 1.2, but the body's "10 s window" overstates what is required.
"Cockpit already reads those rates at 1 Hz" — verified as a ceiling, not a floor. The producer is setInterval(async …, 1000) (src/stores/mainVehicle.ts:840-942), whose round awaits getStatus (3 s timeout, src/libs/blueos.ts:21, :289), getCpuTempCelsius, getCpusInfo and getNetworkInfo in sequence and returns early when the status check fails (src/stores/mainVehicle.ts:843-849).
"The warning is a 15 s snackbar and repeats at most once every 5 min" — verified for the ceiling, contradicted for the floor. The cooldown clock is started when the traffic condition passes (src/libs/wireless-traffic-warning.ts:80-82), before the beacon is consulted, so a shown warning can be rarer than one per five minutes. This is finding 1.3.
"An address the beacon does not report … leaves the current link kind undetermined, and in that case nothing is warned" — verified (src/libs/wireless-traffic-warning.ts:46), and tested.
"No UI was added" — verified; the only output is openSnackbar (src/stores/mainVehicle.ts:930-936).
"go2rtc serves the video over whatever interface it was reached on … forcing the route from Cockpit is not possible" — not checkable from this checkout (vehicle-side service), and qualified in-tree: when the beacon reports a wired address that is also an ICE candidate, src/stores/video.ts:1136-1153 does steer WebRTC media to it. That path moves only the media, not the reached address, so it does not undermine the decision to warn.

Failure site. The misbehaving component is vehicle-side routing, outside this repository, and the PR deliberately advises rather than fixes. The two premises Cockpit can establish are now established at the two sites named above, which is what closed 1.1.

Entry points

Function	Reached from	Frequency
`createWirelessTrafficWatcher` (`src/libs/wireless-traffic-warning.ts:55`)	`VehicleFactory.onVehicles.once` handler in `useMainVehicleStore` (`src/stores/mainVehicle.ts:786`)	one-shot
`shouldWarn` closure (`src/libs/wireless-traffic-warning.ts:60`)	the 1 Hz poll's network block (`src/stores/mainVehicle.ts:927`)	per incoming message (one BlueOS network response per second)
`coversAnalysisSpan` (`src/libs/wireless-traffic-warning.ts:32`)	`shouldWarn` only	per incoming message
`canSuggestCabledLink` (`src/libs/wireless-traffic-warning.ts:45`)	same poll, behind the traffic gate and the cooldown (`src/stores/mainVehicle.ts:928-929`)	per incoming message, throttled to at most one per 5 min
`isTetheredInterfaceType` (`src/libs/blueos.ts:255`)	`canSuggestCabledLink`, and the video store's ICE check interval (`src/stores/video.ts:1118-1174`, which clears itself)	per incoming message / one-shot
`isWirelessInterfaceName`, `isCabledInterfaceName` (`src/libs/blueos.ts:344`, `:346`)	`getNetworkInfo`'s filter — so also the startup variable registration (`src/stores/mainVehicle.ts:807`)	per incoming message
`feedSeconds` (`src/tests/libs/wireless-traffic-warning.test.ts:9`)	vitest	one-shot (test only)

Cost at that frequency stays negligible: a few interfaces × ten samples and one median per interface per second. The one new network request is behind both gates, so the timer cannot spam it — the stall risk it carries is finding 1.3.

Invariants

Samples are appended in timestamp order. coversAnalysisSpan reads first and last rather than min and max (src/libs/wireless-traffic-warning.ts:32-34). Violators: the un-awaited setInterval (src/stores/mainVehicle.ts:840) lets rounds overlap, and two getNetworkInfo responses can resolve out of order on a lossy link. Checked, no finding: an out-of-order push can only shrink or negate the computed span, so it fails towards silence, and the error is about one poll interval against the 4 s of slack between the 6 s span rule and the 10 s window.
Interface kind is derivable from the name. Now held in one place (src/libs/blueos.ts:344, :346) and consumed by the only two sites that need it. Remaining gap, pre-existing and explicitly deferred by the author: hosts with predictable interface names are dropped by that same filter.
A rejected link check cannot change later. The store's comment (src/stores/mainVehicle.ts:923-926) rests on an address change reloading Cockpit, which is verified (src/views/ConfigurationGeneralView.vue:639-643). It holds for a beacon that answers and not for one that fails — the uncovered violator behind 1.3.
One watcher and one interval for the app's lifetime. Unchanged: the creating handler is .once, so no stale samples or cooldown can carry across vehicles.

1. Correctness & Implementation Bugs — 1 finding

1.3 — A beacon failure burns the five-minute cooldown and is reported as a data-lake failure — minor
Consequence: on the congested link this feature exists to catch, the one request that has to succeed for the warning to appear is the one most likely to fail, and each failure pushes the advice five minutes further out while blaming something else in the log.

shouldWarn writes lastWarningTimestamp = timestamp and returns true (src/libs/wireless-traffic-warning.ts:80-82) before anything asks the beacon; the store then awaits getIpsInformationFromVehicle and only warns if canSuggestCabledLink agrees (src/stores/mainVehicle.ts:927-938). The comment at src/stores/mainVehicle.ts:923-926 argues the spent cooldown is harmless because the link in use cannot change without a reload — that part is verified (src/views/ConfigurationGeneralView.vue:639-643) — but it only covers a beacon that answers. getIpsInformationFromVehicle throws on any failure (src/libs/blueos.ts:245-247), and that failure is correlated with the saturated wireless link the traffic gate has just detected. Two consequences follow:

The throw propagates to the round's catch (src/stores/mainVehicle.ts:939-941) with the cooldown already spent, so the next opportunity is five minutes away; while the link stays bad enough for the beacon to keep failing, the user is never told the thing the feature exists to tell them.
That catch logs Failed to update network information in data lake: Could not get information about IPs on BlueOS. …, pointing a future debugging session at the data-lake update, which in fact completed before the beacon was called.

A third, smaller edge sits in the same place: unlike the calls at src/libs/blueos.ts:32 and :309, getIpsInformationFromVehicle passes no retry: 0 and uses the 10 s defaultTimeout (src/libs/blueos.ts:20, :241), so a failing beacon can occupy the awaited round for tens of seconds while the 1 Hz timer keeps starting new ones.

Fix: give the beacon call its own try/catch at the new call site, and consume the cooldown only once the snackbar has actually been shown — either by having the watcher expose the traffic verdict separately from a registerWarningShown(timestamp), or by passing the link check into shouldWarn as a predicate it evaluates before touching lastWarningTimestamp. Either shape also makes the field's name true: today lastWarningTimestamp (src/libs/wireless-traffic-warning.ts:57) records the last evaluation that passed the traffic gate, not the last warning.

Sections with nothing to report (10)

2. Persistence & User Data — ✅ (searched the diff for useBlueOsStorage, useStorage and settings-management: none; the rate history is closure-local to createWirelessTrafficWatcher (src/libs/wireless-traffic-warning.ts:56-57) and the network variables it reads are still registered persistent: false, persistValue: false (src/stores/mainVehicle.ts:833-838), so the PR's persistence footprint is empty and no key is added, reshaped or removed)

3. AGENTS.md Adherence — ✅ (package.json untouched — median comes from the already-installed mathjs, rung 5 of the minimalism ladder, with in-tree precedent for named imports from it at src/libs/vehicle/mavlink/vehicle.ts:2; every new export has a call site in this PR — isTetheredInterfaceType at src/stores/video.ts:1145, isWirelessInterfaceName in getNetworkInfo's own filter, IpInfo in the watcher's signature — so no groundwork; each added JSDoc block has a non-empty summary with typed @param/@returns; the decision logic stayed in src/libs/ with the store only wiring it; and the comment above the reflowed speed lines (src/stores/mainVehicle.ts:904) was kept rather than reworded)

4. Security — ✅ (no dependency added; the single new request goes to the same vehicle beacon endpoint the video store already calls; no encoded blob, no hidden or bidi Unicode in the added identifiers and strings, no eval/Function/v-html, no Electron-only API so the Lite build reaches nothing new, and no build script, workflow, Dockerfile or src/electron/ file is touched — pr.json, pr.diff and both new comments were read as data, and none contains text addressed to the reviewer or an instruction to this workflow)

5. Performance — ✅ (per the entry-point table the watcher rides the pre-existing 1 Hz poll and does one median over at most ten numbers per interface; the only new I/O is one beacon GET behind both the traffic condition and the five-minute cooldown, so at most one per five minutes, and the case where that request stalls the round is covered as finding 1.3 rather than raised twice; the diff registers no listener, watcher, interval or timeout, so there is no new teardown to require)

6. UI / UX — ✅ (no dialog, overlay, menu or Vuetify control is added, so the dialog-anatomy, theme="dark", button-token, padding and stacking rules do not apply; the message goes through the shared openSnackbar with a variant/duration/closeButton combination valid for SnackbarOptions (src/composables/snackbar.ts:6-29), the cooldown is the guard against a timed loop re-opening it, the copy names no protocol or internal id and says what the user should do, and the diff adds no user interaction that would owe logUserAction)

7. Code Quality & Style — ✅ (complexity-report.json for this head reports 256 functions measured across the 5 changed files, truncated: false and triggeredCount: 0, so by the report's own figures nothing the diff added or changed crossed the complexity or depth thresholds and there is no complexity finding to raise; against .eslintrc.cjs, simple-import-sort order holds in all three added import lists — datalogger, DatalogVariable at src/stores/mainVehicle.ts:31 being the in-tree precedent for the lowercase-before-capital order the test file uses — every added arrow carries an explicit return type, there is no any, no line exceeds 180 characters, jsdoc/require-jsdoc is satisfied for the added interface and its method signature, and the RateSample disable follows the precedent at src/libs/blueos.ts:184; +25 lines on mainVehicle.ts is well under the file-growth threshold, and round 1's two duplication findings are gone with no new copy introduced)

8. Commit Hygiene — ✅ (pr.json lists one commit, 913ebf7: the round-1 fixes were amended into it instead of being left as "address review" or fixup! commits, which is what the AGENTS.md rule asks for, and nothing self-correcting or stacked remains; the feat: prefix fits this change and the body was updated to explain the beacon gate; the shared-predicate extraction that rides along is the dedup the feature itself consumes, nine behaviour-preserving lines in src/stores/video.ts, small enough not to owe a commit of its own; no #N or closing keyword in the message, with Closes #2953 confined to the PR body)

9. Tests — ✅ (nothing outside this PR is touched; the two cases dropped in this round tested the deleted cable-dominance branch and had been added by this same PR, so no test on master was removed or weakened, and only suggests the cable when the vehicle is reached wirelessly and a cabled address exists (src/tests/libs/wireless-traffic-warning.test.ts:50) covers all four link-kind cases including the mDNS one; the new suite sits with the others under src/tests/libs/)

10. Documentation — ✅ (nothing added behaves differently between Lite and Standalone — no Electron-only API is reached and both builds run the same poll — so the README.md parity table needs no row, and all four added exports carry JSDoc)

11. Nitpicks / Optional — ✅ (both round-1 nits are gone; re-read the added lines for naming and shape, and the one remaining wording point — lastWarningTimestamp recording an evaluation rather than a warning — is part of finding 1.3's remedy rather than a separate nit)

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

rafaellehmkuhl · 2026-08-19T20:03:51Z

Review follow-up — round 2

Done

src/libs/wireless-traffic-warning.ts, src/stores/mainVehicle.ts (1.3 — a beacon failure burns the cooldown and is logged as a data-lake failure): the cooldown is now started by registerWarningShown(timestamp), called only after openSnackbar, so lastWarningTimestamp records what its name says. The beacon call moved into suggestCabledLinkIfItMakesSense with its own try/catch and its own message, so a failure no longer surfaces as Failed to update network information in data lake and no longer costs five minutes — the next round retries.
src/tests/libs/wireless-traffic-warning.test.ts (1.3): warns only once the wireless link has been busy for the whole window now asserts the verdict keeps being offered while no warning has been registered, and only goes quiet after registerWarningShown.

Done differently

src/stores/mainVehicle.ts (1.3 — retries on a failing beacon): rather than retrying once per second for as long as the traffic condition holds, the verdict is asked at most once successfully and cached, since the link in use cannot change without reloading Cockpit. false is cached too, so the common mDNS case now costs one request for the session instead of one per cooldown. A checkingCabledLinkSuggestion flag keeps a slow first answer from being awaited by several rounds at once and warning several times over.
src/stores/mainVehicle.ts (1.3, third edge — the beacon occupying the awaited round): the call is not awaited by the poll at all, so a slow beacon cannot stretch a round, and the in-flight flag bounds the feature to one outstanding request regardless of how long it takes. I left getIpsInformationFromVehicle's ky options alone on purpose: adding retry: 0 and the shorter beaconTimeout there would also make the video store's ICE route selection give up sooner, which is not this PR's call to make.

Won't change (with reasoning)

body wording, "10 s window": agreed it overstated the requirement, so the PR body now says at least 4 readings spanning 6 s of a 10 s history. No code change — the loosening itself is what 1.2 asked for.
mDNS reach question (from round 1): taking the answer as given, the gate stays as it is, and the ICE-candidate-pair signal is left for its own change.

rafaellehmkuhl · 2026-08-19T20:03:52Z

/review

github-actions · 2026-08-19T20:25:10Z

Automated PR Review — round 3

Warning

⚠️ IMPORTANT FIXES REQUIRED — 3 open (1 major, 1 minor, 1 nit); 1 closed since round 2, 7 closed in total.

Cockpit's once-a-second poll of the vehicle's network counters now also keeps a ten-second history of how fast each interface is uploading. Once a wireless interface has held a median of at least 5 Mbps across enough of that history, Cockpit asks the vehicle which addresses it can be reached on; if the address in use is the wireless one and a cabled one also exists, it shows a 15-second snackbar suggesting the cable and then stays quiet for five minutes. That question is now asked at most once per session and the answer kept, the request is no longer awaited by the poll, and a failure to answer it no longer costs the user a warning. Nothing is persisted and no new UI is added.

What still needs attention

#	Problem	What it means	Severity	Status
1.4	The cable decision is made once and never revisited	Cockpit makes up its mind seconds into a session about whether the cable advice applies, so a cable plugged in later never produces the advice, and a cable removed later keeps producing advice the operator can no longer follow.	major	❌
6.1	The same advice repeats every five minutes, forever	An operator who cannot switch to the cable right now gets the same interruption every five minutes for the whole session, with no way to stop it.	minor	❌
11.3	Code description no longer matches the rule it describes	The documentation and a test name still say the link must be busy for the full ten seconds, while the code settles for less.	nit	❌

Since round 2 — 1 closed, comparing 913ebf7 → da98bff

Range. The branch was force-pushed again: pr.json lists a single commit (da98bff) and 913ebf7 is not in its history. incremental.diff came back holding all five files with src/libs/wireless-traffic-warning.ts as +91/-0, i.e. the whole PR rather than an increment, so it is unusable for judging what moved. The transition below was worked out from pr.diff against the round-2 finding text and the line references it quoted.

Resolutions. resolutions.json is [] — no /resolve has been banked on this PR, so nothing was closed by a maintainer this round and there are no unrecognised ids to report back.

Previous findings

1.3 — A beacon failure burns the five-minute cooldown and is reported as a data-lake failure — ✅ Addressed. The finding asked for three things plus a smaller third edge, and each is answered in code:
- its own try/catch at the new call site: the beacon call moved into suggestCabledLinkIfItMakesSense (src/stores/mainVehicle.ts:794-807) with its own handler and its own message, Failed to get the links the vehicle can be reached on (:801-803). The round's outer catch (:959-961) can no longer report it as a data-lake failure, and the call at :957 is not awaited, so the throw never reaches that catch at all.
- the cooldown consumed only once the snackbar has been shown: shouldWarn no longer writes the timestamp; the new registerWarningShown (src/libs/wireless-traffic-warning.ts:87-89) is the only writer of lastWarningTimestamp (:62), and its one call site is src/stores/mainVehicle.ts:810, past the if (!cabledLinkSuggestionMakesSense) return gate and immediately before openSnackbar (:811). A failed or negative check now costs nothing.
- the field's name made true: it records a shown warning, per the above.
- the third edge (the beacon occupying the awaited round): answered differently and adequately. Not awaiting the call means the 10 s defaultTimeout and ky's default retries on getIpsInformationFromVehicle (src/libs/blueos.ts:238-248) can no longer stretch a poll round, and checkingCabledLinkSuggestion (src/stores/mainVehicle.ts:788, set before the await, cleared in finally at :804-806) bounds the feature to one outstanding request. Leaving that call's ky options alone is the right call for the reason given — src/stores/video.ts:1140 shares it.
- The tests moved with it: :27 asserts the verdict keeps being offered while nothing has been registered, :29-32 that it goes quiet only after registerWarningShown (src/tests/libs/wireless-traffic-warning.test.ts).
- The mechanism chosen to stop the retries from repeating once a second — caching the composite verdict for the session — is new behaviour and is raised as finding 1.4 below. Closing 1.3 does not cover it.
The six findings closed in round 2 (1.1, 1.2, 7.1, 7.2, 11.1, 11.2) stay closed: the code each was closed on is still in place — the beacon gate at src/libs/wireless-traffic-warning.ts:50-54 still gates the snackbar (src/stores/mainVehicle.ts:808), the 4-sample/6-second span rule is unchanged (:34-39), median still comes from mathjs (:1), and the shared predicates still live at src/libs/blueos.ts:255, :344, :346.

Discussion since round 2

rafaellehmkuhl's follow-up (comment) lists what was done, done differently, and left alone. Each item was checked against pr.diff rather than taken as given; the "Done" items and both "Done differently" items hold as described, which is what 1.3's closure rests on.
The argument offered for the new caching — that the verdict can be asked once because "the link in use cannot change without reloading Cockpit" — is verified for the address (src/views/ConfigurationGeneralView.vue:639-643 reloads when it changes) but covers only one of the two premises the cached boolean combines. That gap is finding 1.4; it is a new finding of this round, not a dispute of the author's fix.
The PR body's "10 s window" wording was corrected to "at least 4 of them, spanning 6 s of a 10 s history", which matches minSamplesPerWindow, minAnalysisSpanMs and analysisWindowMs (src/libs/wireless-traffic-warning.ts:27, :34-35). The round-2 qualification on that claim is therefore gone; the in-code descriptions kept the old phrasing, which is nit 11.3.
The other new comment is the bare /review that triggered this round; treated as a command, not as review input.

Change map — what was established before judging

Claims (from the PR body and commit message, each checked against the code)

"the vehicle is currently reached over a wireless address while a cabled one is also available, as reported by the beacon … the same source the video store already uses" — verified. canSuggestCabledLink (src/libs/wireless-traffic-warning.ts:50-54) reads the IpInfo[] from getIpsInformationFromVehicle (src/libs/blueos.ts:238-248) and classifies with isTetheredInterfaceType (src/libs/blueos.ts:255), the same predicate src/stores/video.ts:1145 now uses.
"a wireless interface has held a median upload of at least 5 Mbps over the recent readings — at least 4 of them, spanning 6 s of a 10 s history" — verified. analysisWindowMs = 10000, busyWirelessThresholdMbps = 5 (:27-28), minAnalysisSpanMs = 6000, minSamplesPerWindow = 4 (:34-35), applied at :37-39 and :82-83.
"Cockpit already reads those rates at 1 Hz" — verified as a ceiling, not a floor. The producer is setInterval(async …, 1000) (src/stores/mainVehicle.ts:872), whose round awaits getStatus, getCpuTempCelsius, getCpusInfo and getNetworkInfo in sequence and returns early when the status check fails (:874-881).
"The warning is a 15 s snackbar and repeats at most once every 5 min" — verified, and this is the claim that changed this round. duration: 15000 at src/stores/mainVehicle.ts:815; the cooldown is now started by registerWarningShown next to the snackbar (:810-811) rather than by the traffic verdict, so the interval is a real floor. The recorded moment is the reading's timestamp rather than the snackbar's, which differs by the beacon latency on the first warning only.
"An address the beacon does not report, such as the default blueos-avahi.local host name, leaves the current link kind undetermined, and in that case nothing is warned" — verified (src/libs/wireless-traffic-warning.ts:51-52), tested at src/tests/libs/wireless-traffic-warning.test.ts:64.
"No UI was added — it uses the existing snackbar" — verified; the only output is openSnackbar (src/stores/mainVehicle.ts:811-817), whose options match SnackbarOptions (src/composables/snackbar.ts:6-29).
"go2rtc serves the video over whatever interface it was reached on … forcing the route from Cockpit is not possible" — not checkable from this checkout (vehicle-side service), and qualified in-tree: when the beacon reports a wired address that is also an ICE candidate, src/stores/video.ts:1136-1160 does steer WebRTC media to it. That path moves the media, not the reached address, so it does not undermine the decision to warn.

Failure site. The misbehaving component is vehicle-side routing, outside this repository; the PR deliberately advises rather than fixes. The two premises Cockpit can establish are established in canSuggestCabledLink; what this round changed is how long that answer is trusted, which is finding 1.4.

Entry points

Function	Reached from	Frequency
`createWirelessTrafficWatcher` (`src/libs/wireless-traffic-warning.ts:60`)	`VehicleFactory.onVehicles.once` handler (`src/stores/mainVehicle.ts:634`), watcher created at `:786`	one-shot
`shouldWarn` closure (`src/libs/wireless-traffic-warning.ts:65`)	the 1 Hz poll's network block (`src/stores/mainVehicle.ts:956`)	per incoming message (one BlueOS network response per second)
`coversAnalysisSpan` (`:37`)	`shouldWarn` only	per incoming message
`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:794`)	same poll, un-awaited, behind the traffic gate (`:956-957`)	per incoming message
`canSuggestCabledLink` (`src/libs/wireless-traffic-warning.ts:50`)	`suggestCabledLinkIfItMakesSense` (`:800`) — at most once successfully per app session, since the result is cached at `:787`	one-shot
`registerWarningShown` (`:87`)	`suggestCabledLinkIfItMakesSense` (`:810`), after the gate	per incoming message, throttled to one per 5 min
`isTetheredInterfaceType` (`src/libs/blueos.ts:255`)	`canSuggestCabledLink`; the video store's ICE-selection interval (`src/stores/video.ts:1118-1174`, which clears itself)	per incoming message / one-shot
`isWirelessInterfaceName` (`src/libs/blueos.ts:344`)	`getNetworkInfo`'s own filter (`:352-356`) and `shouldWarn`	per incoming message
`isCabledInterfaceName` (`src/libs/blueos.ts:346`)	`getNetworkInfo`'s filter only	per incoming message
`getNetworkInfo` (`src/libs/blueos.ts:348`, filter body only)	startup variable registration (`src/stores/mainVehicle.ts:839`) and the 1 Hz poll (`:908`)	per incoming message
ICE `forEach` callback (`src/stores/video.ts:1142-1152`, refactored)	the 5 s ICE-check interval	per incoming message until the interval clears
`feedSeconds` (`src/tests/libs/wireless-traffic-warning.test.ts:9-17`)	vitest	one-shot (test only)

Cost at that frequency stays negligible: a few interfaces × ten samples and one median per interface per second. The new I/O is one beacon GET; while the answer is still unknown it is retried once per poll round instead of once per five minutes, which is one small request per second at worst, alongside the four the poll already issues.

Invariants

The link in use cannot change without reloading Cockpit (stated at src/stores/mainVehicle.ts:790-793). Verified for the address (src/views/ConfigurationGeneralView.vue:639-643). It is used to justify caching the whole of canSuggestCabledLink, which also asserts that a cabled address exists — a property with its own violators: a cable plugged in or pulled mid-session, and a wired interface acquiring its address after Cockpit's first check. Neither is covered → finding 1.4.
At most one beacon request in flight. checkingCabledLinkSuggestion (:788) is set before the only await and cleared in finally (:804-806); the poll is the sole caller, and the continuation from finally through registerWarningShown to openSnackbar is synchronous, so no second round can interleave and warn twice. Covered.
Samples are appended in timestamp order. coversAnalysisSpan reads first and last rather than min and max (src/libs/wireless-traffic-warning.ts:37-39). Violators: the un-awaited setInterval (src/stores/mainVehicle.ts:872) lets rounds overlap, and two getNetworkInfo responses can resolve out of order. Checked, no finding: an out-of-order push can only shrink the computed span, so it fails towards silence, against 4 s of slack between the 6 s span rule and the 10 s window.
Interface kind is derivable from the name. Single-sourced at src/libs/blueos.ts:344, :346, consumed by the filter and the watcher, so they cannot disagree. Remaining gap, pre-existing and explicitly deferred by the author: hosts with predictable interface names (wlp2s0) are dropped by that filter.
One watcher for the app's lifetime. The creating handler is .once (src/stores/mainVehicle.ts:634), so no stale samples or cooldown carry across vehicles — and, as 1.4 notes, no cached verdict is dropped either.

1. Correctness & Implementation Bugs — 1 finding

1.4 — The cable suggestion is decided once per session and cached, including the half of the decision that can change — major
Consequence: Cockpit decides seconds into a session whether the cable advice makes sense and never revisits it, so a cable attached later never produces the advice, and a cable removed later keeps producing advice the operator can no longer follow.

suggestCabledLinkIfItMakesSense consults the beacon only while cabledLinkSuggestionMakesSense === undefined (src/stores/mainVehicle.ts:795-807) and then keeps that boolean for the life of the app — it lives in the onVehicles.once handler scope (:634, :787) and is the gate at :808. The comment at :790-793 justifies this with "the link in use cannot change without reloading Cockpit", which is true of the address (src/views/ConfigurationGeneralView.vue:639-643). But canSuggestCabledLink (src/libs/wireless-traffic-warning.ts:50-54) returns the conjunction of two premises, and the reload argument covers only the first:

the address in use is one the beacon reports, and reports as non-tethered (:51-52) — immutable without a reload;
some reported address is tethered (:53) — a fact about the vehicle right now, which changes whenever a cable is plugged in or pulled, or when a wired interface acquires its address after the first check.

Both directions reach the user:

Cached false. The beacon is first consulted the moment a wireless interface has been busy for 6 s — typically seconds into a session with video running. A vehicle whose cable is attached (or whose wired address appears) after that is silent for the rest of the session, which is exactly the situation the PR exists to report.
Cached true. Once the cable is gone, every subsequent five-minute window still fires the snackbar (:808-817), telling the operator to connect through a cabled network that no longer exists. That is the unsubstantiated warning the module's own JSDoc calls "worse than none" (src/libs/wireless-traffic-warning.ts:44-45) and that closed finding 1.1 was about.

Fix: cache only the premise the reload argument covers. Split the decision into the two predicates canSuggestCabledLink already computes internally — for example isReachedOverWirelessLink(ipsInfo, currentAddress) and hasCabledAddress(ipsInfo), both staying in src/libs/wireless-traffic-warning.ts so the store keeps only the wiring. Cache the first, including its negative: a false there (the address is tethered, or unknown as with the mDNS default) is stable for the session and still costs one request per session, which is what the caching was introduced for. Re-evaluate the second on the response of the round that is about to warn — since the beacon is only consulted when a warning is due, that is at most one request per five-minute cooldown, the rate the round-2 code paid and not something finding 1.3 asked to remove.

6. UI / UX — 1 finding

6.1 — The warning repeats every five minutes for the whole session, where the tree warns once — minor
Consequence: an operator who cannot switch to the cable right now is interrupted by the same advice every five minutes for as long as they fly, with no way to stop it.

rewarnIntervalMs (src/libs/wireless-traffic-warning.ts:29) plus the gate at :85 means the snackbar reappears indefinitely while the traffic condition holds. Acting on the advice means editing the vehicle address in settings, which reloads Cockpit (src/views/ConfigurationGeneralView.vue:639-643) — not something an operator does mid-dive — so the second and later showings repeat advice the user has already declined and cannot act on now. Every sibling warning of this class in the tree is once per session instead: losingChunksWarningIssued (src/stores/video.ts:889, head numbering) for degraded video, and noIpSelectedWarningIssued / selectedIpNotAvailableWarningIssued (:1116-1117, used at :1167-1170) for the wired-route advice this feature sits directly next to.

Fix: latch it — drop rewarnIntervalMs and let registerWarningShown silence the watcher for the session, matching the three flags above; or, if repeating is wanted, stop after the second showing. Graded minor rather than major: the repeat is deliberate and stated in the PR body, nothing breaks, and the cost is interruption plus inconsistency with the surrounding pattern.

11. Nitpicks / Optional — 1 finding

11.3 — shouldWarn's doc and the first test name still describe the pre-1.2 rule — nit
Consequence: the next reader is told the link must be busy for the full ten seconds, and will be surprised by a warning that fires after six.

@returns {boolean} True when a wireless interface has been busy for the whole window … (src/libs/wireless-traffic-warning.ts:14) and the test named warns only once the wireless link has been busy for the whole window (src/tests/libs/wireless-traffic-warning.test.ts:19) both predate the loosening finding 1.2 asked for: the rule is 4 readings spanning 6 s of a 10 s history (src/libs/wireless-traffic-warning.ts:34-39). The PR body was corrected this round for exactly this wording; these two were not.

Sections with nothing to report (8)

2. Persistence & User Data — ✅ (searched the diff for useBlueOsStorage, useStorage and settings-management: none; the rate history and both new flags are closure-local (src/libs/wireless-traffic-warning.ts:61-62, src/stores/mainVehicle.ts:787-788) and the network variables the poll feeds are still registered persistent: false, persistValue: false (src/stores/mainVehicle.ts:865-870), so the PR adds, reshapes and removes no persisted key)

3. AGENTS.md Adherence — ✅ (package.json untouched — median still comes from the installed mathjs, rung 5 of the minimalism ladder; every new export has a call site in this PR (isTetheredInterfaceType at src/stores/video.ts:1145, isWirelessInterfaceName in getNetworkInfo's filter, registerWarningShown at src/stores/mainVehicle.ts:810, IpInfo in the watcher's signature), so no groundwork; the added JSDoc blocks have non-empty summaries with typed @param/@returns; the decision logic stayed in src/libs/ with the store wiring it; the // Set speeds (ensure they're not negative …) comment at :936 was kept while its lines changed; and the one reflow that rides along, nonNegativeUploadMbps (:937, :942-943), is required by the new per-interface map rather than gratuitous)

4. Security — ✅ (no dependency added; the single new request goes to the same beacon/v1.0/services endpoint the video store already calls (src/libs/blueos.ts:240); no encoded blob, no hidden or bidirectional Unicode in the added identifiers or the snackbar string, no eval/Function/v-html, no Electron-only API so the Lite build reaches nothing new, and no build script, workflow, Dockerfile or src/electron/ file is touched — pr.json, pr.diff, complexity-report.json and both new comments were read as data, and none contains text addressed to the reviewer or an instruction to this workflow)

5. Performance — ✅ (per the entry-point table the watcher rides the pre-existing 1 Hz poll and does one median over at most ten numbers per interface; the beacon call is no longer awaited by the round (src/stores/mainVehicle.ts:957) and is bounded to one outstanding request by checkingCabledLinkSuggestion, so the worst case while the answer is unknown is one extra small GET per second beside the four the poll already makes at :874-908, with its failure logged at the same 1 Hz as the pre-existing console.error at :879; the diff registers no listener, watcher, interval or timeout, so no teardown is owed)

7. Code Quality & Style — ✅ (complexity-report.json for this head reports, by its own figures, base ce3a8d4 and head da98bff with 258 functions measured across the 5 changed files, truncated: false and triggeredCount: 0, so nothing the diff added or changed crossed the complexity or depth thresholds and no complexity finding is raised; against .eslintrc.cjs, simple-import-sort order holds in all four added or edited import lists — canByPassCategory, EventCategory, slideToConfirm at src/stores/mainVehicle.ts:31 being the in-tree precedent for the lowercase-before-capital specifier order the test file uses — every added arrow carries an explicit return type, there is no any, the longest added code line (the snackbar message, src/stores/mainVehicle.ts:813) is well under 180 characters, jsdoc/require-jsdoc is satisfied for the added interface and its two method signatures while ArrowFunctionExpression: false covers suggestCabledLinkIfItMakesSense, and the RateSample disable follows the precedent at src/libs/blueos.ts:184; +41 net lines on a 1109-line mainVehicle.ts is far from the file-growth threshold, and no comment whose code is unchanged was reworded)

8. Commit Hygiene — ✅ (pr.json lists one commit, da98bff: this round's fixes were amended into it rather than left as an "address review" or fixup! commit, which is what the AGENTS.md rule asks, and nothing self-correcting or stacked remains; the feat: prefix fits and the body explains both the beacon gate and the median-over-instant choice; 230 additions in one commit is reviewable as a unit; the nine behaviour-preserving lines in src/stores/video.ts are the dedup the feature itself consumes, small enough not to owe a commit of their own; no #N or closing keyword in the message, with Closes #2953 confined to the PR body)

9. Tests — ✅ (no test outside this PR is touched; the round-2 edits to warns only once the wireless link has been busy for the whole window strengthened it — :27 now pins that an unshown verdict keeps being offered and :29-32 that only registerWarningShown silences it — and the four link-kind cases including the mDNS one remain at :55-65; the new suite sits with the others under src/tests/libs/)

10. Documentation — ✅ (nothing added behaves differently between Lite and Standalone — no Electron-only API is reached and both builds run the same poll and the same snackbar — so the README.md parity table needs no row; the five new function and interface exports all carry JSDoc, the accuracy of one being nit 11.3 rather than an omission, while the newly exported IpInfo type sits inside the pre-existing jsdoc/require-jsdoc disable region at src/libs/blueos.ts:184)

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

rafaellehmkuhl · 2026-08-19T20:40:41Z

Review follow-up — round 3

Done

src/stores/mainVehicle.ts (1.4 — the cable verdict is cached including the half that can change): the cache is gone. suggestCabledLinkIfItMakesSense now consults the beacon fresh and evaluates canSuggestCabledLink on that response, so a cable plugged in later starts producing the advice and a cable pulled stops it. You were right that the reload argument only covers the address, not the existence of a cabled one.
src/libs/wireless-traffic-warning.ts, src/stores/mainVehicle.ts, src/tests/libs/wireless-traffic-warning.test.ts (6.1 — repeats every five minutes forever): latched. rewarnIntervalMs and lastWarningTimestamp are gone; registerWarningShown() takes no timestamp and silences the watcher for the session, matching losingChunksWarningIssued and noIpSelectedWarningIssued. The test now feeds 400 s after the warning and expects no second verdict, which fails under the old cooldown.
src/libs/wireless-traffic-warning.ts, src/tests/libs/wireless-traffic-warning.test.ts (11.3 — stale description of the rule): shouldWarn's @returns and the test name now say "over enough of the recent readings"/"over enough of the window". The commit message carried the same "for ten seconds" wording and was reworded in the same amend.

Done differently

src/stores/mainVehicle.ts (1.4 — how the request rate is kept down): instead of splitting canSuggestCabledLink into two predicates and caching the wireless one, the whole check is throttled to once every 30 s (cabledLinkCheckIntervalMs), fresh each time. The split does not actually bound the request rate: the beacon is consulted whenever a warning is due, and after 1.3 a verdict stays due every second until the snackbar is shown — so a vehicle that is on WiFi with no cable attached would fetch once per second, forever, which is what the caching was added to stop. A throttle bounds it in every combination without splitting anything, and it costs one small GET per 30 s while heavy traffic is flowing, against the four per second the poll already makes. The failure path rides the same throttle now — the timestamp is written before the request, so a failing beacon retries in 30 s instead of logging at 1 Hz.
PR body: the "repeats at most once every 5 min" claim and the test-plan step that checked it were updated for the latch, plus a step for plugging the cable in mid-session.

rafaellehmkuhl · 2026-08-19T20:40:42Z

/review

github-actions · 2026-08-19T20:55:38Z

Automated PR Review — round 4

Tip

✅ READY TO MERGE — 0 open; 3 closed since round 3, 10 closed in total.

Cockpit's once-a-second poll of the vehicle's network counters now also keeps a ten-second history of how fast each interface is uploading. Once a wireless interface has held a median of at least 5 Mbps across enough of that history, Cockpit asks the vehicle which addresses it can be reached on — freshly each time, at most once every thirty seconds — and if the address in use is the wireless one while a cabled one also exists, it shows a fifteen-second snackbar suggesting the cable. That advice is given once and then the watcher goes quiet for the rest of the session, matching how the video store warns about degraded streams beside it. Nothing is persisted and no new UI is added.

What still needs attention

Nothing is open: all ten findings raised over the four rounds are closed.

Since round 3 — 3 closed, comparing da98bff → a078722

Range. The branch was force-pushed again: pr.json lists a single commit (a078722) and da98bff is not in its history. incremental.diff came back holding all five files, with src/libs/wireless-traffic-warning.ts as +87/-0 — the whole PR rather than an increment — so it is unusable for judging what moved. The three transitions below were worked out from pr.diff against the round-3 finding text and the code it quoted.

Resolutions. resolutions.json is [] — no /resolve has been banked on this PR, so nothing was closed by a maintainer this round, and there are no unrecognised ids to report back.

Previous findings

1.4 — The cable suggestion is decided once per session and cached, including the half of the decision that can change — ✅ Addressed. The finding asked for the mutable premise ("a cabled address exists") to stop being cached and to be re-established on the round that is about to warn. Both parts landed, by a different mechanism:
- the cache is gone entirely. cabledLinkSuggestionMakesSense no longer exists anywhere in the onVehicles.once scope (src/stores/mainVehicle.ts:634); suggestCabledLinkIfItMakesSense (:795-819) fetches the beacon on every check and evaluates canSuggestCabledLink on that response (:802-803), keeping no verdict.
- cached false — a cable attached later never warns: closed. While the traffic condition holds, shouldWarn stays true until a snackbar is registered (src/libs/wireless-traffic-warning.ts:81), so the throttle (src/stores/mainVehicle.ts:797, cabledLinkCheckIntervalMs = 30000 at :787) re-asks the beacon every 30 s and a cable that appears mid-session produces the advice within that window.
- cached true — a cable removed later keeps advising: closed twice over. There is no cache to go stale, and after the single warning the watcher is silent for the session, so there is no later window in which stale advice could fire.
- the mechanism differs from the fix I named (splitting canSuggestCabledLink and caching only the immutable half). The author's reason, in their follow-up, is that the split does not bound the request rate because a verdict stays due every second until the snackbar is shown. Checked against the round-3 code rather than taken as given: correct — with the beacon consulted on every due verdict, a WiFi-reached vehicle with no cable would have fetched once per second for the session. The throttle bounds it at one small GET per 30 s in every combination, which is the same order as the five-minute rate the round-2 code paid, so nothing 1.4 asked for is traded away.
6.1 — The warning repeats every five minutes for the whole session, where the tree warns once — ✅ Addressed. The finding asked for the latch, matching the three sibling flags. rewarnIntervalMs and lastWarningTimestamp are gone; the watcher now holds warningShown (src/libs/wireless-traffic-warning.ts:60), whose only writer is registerWarningShown() — now argument-less (:83-84) — and shouldWarn returns !warningShown && … (:81). Its one call site is still src/stores/mainVehicle.ts:811, past the beacon gate and immediately before openSnackbar. That is the shape of losingChunksWarningIssued (src/stores/video.ts:889, head numbering) and noIpSelectedWarningIssued (:1116) the finding pointed at. The test moved with it: src/tests/libs/wireless-traffic-warning.test.ts:32 feeds 400 s of busy readings after the latch and expects no verdict, which the old five-minute rewarn would fail.
11.3 — shouldWarn's doc and the first test name still describe the pre-1.2 rule — ✅ Addressed. The @returns now reads "busy over enough of the recent readings" (src/libs/wireless-traffic-warning.ts:14) and the test is named warns only once the wireless link has been busy over enough of the window (src/tests/libs/wireless-traffic-warning.test.ts:19). The commit body carried the same stale "for ten seconds" phrasing and now reads "across enough of a ten-second history".
The seven findings closed in earlier rounds stay closed. The code each rested on is still in place: the beacon gate still gates the snackbar (src/libs/wireless-traffic-warning.ts:48-52 via src/stores/mainVehicle.ts:803), the 4-sample / 6-second span rule is unchanged (:32-37), median still comes from mathjs (:1), the shared predicates still live at src/libs/blueos.ts:255, :344, :346, and 1.3's separation of the beacon failure from the data-lake catch is intact (src/stores/mainVehicle.ts:804-806 against :960-961). One behavioural change rides on top of 1.3: the throttle timestamp is written before the request (:799), so a failing beacon now costs 30 s of quiet instead of nothing. That is not a regression of 1.3 — the cooldown 1.3 was about is the warning latch, which only a shown snackbar sets — and it drops the failure console.error (:805) from 1 Hz to once per 30 s.

Discussion since round 3

rafaellehmkuhl's follow-up (comment) lists three items as done and two as done differently. Each was checked against pr.diff rather than accepted as stated; all five hold as described, which is what the three closures above rest on. The argument offered for the throttle is the one verified in 1.4.
The other new comment is the bare /review that triggered this round; treated as a command, not as review input.
Nothing in pr.json, pr.diff, incremental.diff, complexity-report.json, resolutions.json or new-comments.json contains text addressed to this reviewer or an instruction to this workflow.

Change map — what was established before judging

Claims (from the PR body and commit message, each checked against the code)

"The beacon is only asked once the traffic condition holds, and at most once every 30 s from there, so a cable plugged in or pulled mid-session changes the answer" — verified, with one half qualified. The gate is if (wirelessTrafficWatcher.shouldWarn(…)) at src/stores/mainVehicle.ts:957-958; the throttle is :797 against cabledLinkCheckIntervalMs = 30000 (:787). "Plugged in" holds: the verdict stays due every second until a snackbar is shown (src/libs/wireless-traffic-warning.ts:81), so the beacon is re-asked every 30 s and a cable appearing mid-session produces the advice. "Pulled" holds only until the one warning is shown; after that the watcher is silent for the session, so stale advice is impossible rather than re-checked.
"The warning is a 15 s snackbar, given once per session" — verified. duration: 15000 at src/stores/mainVehicle.ts:816; the latch is warningShown (src/libs/wireless-traffic-warning.ts:60, read at :81, written only by registerWarningShown at :83-84, called at src/stores/mainVehicle.ts:811).
"a wireless interface has held a median upload of at least 5 Mbps over the recent readings — at least 4 of them, spanning 6 s of a 10 s history" — verified. analysisWindowMs = 10000, busyWirelessThresholdMbps = 5 (src/libs/wireless-traffic-warning.ts:26-27), minAnalysisSpanMs = 6000, minSamplesPerWindow = 4 (:32-33), applied at :35-37 and :78-81.
"Cockpit already reads those rates at 1 Hz" — verified as a ceiling, not a floor. The producer is setInterval(async …, 1000) (src/stores/mainVehicle.ts:873), whose round awaits getStatus, getCpuTempCelsius, getCpusInfo and getNetworkInfo in sequence and returns early when the status check fails (:874-882). This is what the span-based window rule exists for.
"the same source the video store already uses" — verified. canSuggestCabledLink reads the IpInfo[] from getIpsInformationFromVehicle (src/libs/blueos.ts:238-248) and classifies with isTetheredInterfaceType (:255), the predicate src/stores/video.ts:1145 now shares.
"An address the beacon does not report, such as the default blueos-avahi.local host name, leaves the current link kind undetermined, and in that case nothing is warned" — verified (src/libs/wireless-traffic-warning.ts:49-50), tested at src/tests/libs/wireless-traffic-warning.test.ts:64.
"No UI was added — it uses the existing snackbar" — verified; the only output is openSnackbar (src/stores/mainVehicle.ts:812-818), whose options match SnackbarOptions (src/composables/snackbar.ts:6-29).
"go2rtc serves the video over whatever interface it was reached on … forcing the route from Cockpit is not possible" — not checkable from this checkout (vehicle-side service), and qualified in-tree: when the beacon reports a wired address that is also an ICE candidate, src/stores/video.ts:1142-1152 does steer WebRTC media to it. That moves the media, not the reached address, so it does not undermine the decision to warn.
Unverified premise, recorded as such. The gate treats any beacon-reported WIRED/USB address as proof that a cable is attached (src/libs/wireless-traffic-warning.ts:51). Whether BlueOS's beacon advertises a carrier-less eth0 that still holds a static address is vehicle-side behaviour, and nothing in this checkout settles it — getIpsInformationFromVehicle reports whatever the endpoint returns (src/libs/blueos.ts:238-248), and the is_up flag that would settle it belongs to a different endpoint's payload (src/types/blueos.ts:34). If the beacon does advertise one, the no-cable case would warn anyway. Not raised as a finding because it cannot be established here; the PR's test plan covers it on hardware (the no-cable and plug-in-mid-session steps).

Failure site. The misbehaving component is vehicle-side routing, outside this repository; the PR deliberately advises rather than fixes. The two premises Cockpit can establish are established in canSuggestCabledLink, and what changed this round is that they are re-established on live data instead of being decided once.

Entry points

Function	Reached from	Frequency
`createWirelessTrafficWatcher` (`src/libs/wireless-traffic-warning.ts:58`)	`src/stores/mainVehicle.ts:786`, inside the `VehicleFactory.onVehicles.once` handler (`:634`)	one-shot
`shouldWarn` closure (`src/libs/wireless-traffic-warning.ts:63`)	the 1 Hz poll's network block (`src/stores/mainVehicle.ts:957`)	per incoming message (one BlueOS network response per second)
`coversAnalysisSpan` (`:35`)	`shouldWarn` only	per incoming message
`registerWarningShown` (`:83`)	`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:811`), past both guards	one-shot (once per session, by the latch it sets)
`canSuggestCabledLink` (`:48`)	`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:803`)	per incoming message, throttled to one per 30 s
`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:795`)	the same poll, un-awaited, behind the traffic gate (`:957-958`)	per incoming message (body throttled to one per 30 s)
`isTetheredInterfaceType` (`src/libs/blueos.ts:255`)	`canSuggestCabledLink`; the video store's ICE selection (`src/stores/video.ts:1145`)	per incoming message / per incoming message until the 5 s interval clears
`isWirelessInterfaceName` (`src/libs/blueos.ts:344`)	`getNetworkInfo`'s own filter (`:350-354`) and `shouldWarn`	per incoming message
`isCabledInterfaceName` (`:346`)	`getNetworkInfo`'s filter only	per incoming message
`getNetworkInfo` (`:348`, filter body only)	startup variable registration (`src/stores/mainVehicle.ts:840`) and the 1 Hz poll (`:909`)	per incoming message
ICE `forEach` callback (`src/stores/video.ts:1142-1152`, refactored)	the 5 s ICE-check interval (`:1118`), which clears itself	per incoming message until the interval clears
`feedSeconds` (`src/tests/libs/wireless-traffic-warning.test.ts:7-15`)	vitest	one-shot (test only)

Cost at that frequency stays negligible: a few interfaces × ten samples and one median per interface per second. The new I/O is one beacon GET, now bounded to one per 30 s and one outstanding at a time, beside the four requests the poll already issues each second.

Invariants

The link in use cannot change without reloading Cockpit (the reason the advice is given once, stated at src/tests/libs/wireless-traffic-warning.test.ts:31). Verified for the address: changing it reloads (src/views/ConfigurationGeneralView.vue:639-643). Nothing else now depends on this rule — the round-3 cache that leaned on it for the cabled-address premise as well is gone, which is what closed 1.4.
A cabled address existing is a fact about the vehicle right now. No longer assumed: it is re-read from the beacon on every check (src/stores/mainVehicle.ts:802-803). Violators — a cable plugged in or pulled, a wired interface acquiring its address late — are covered up to a 30 s lag before the warning, and moot after it, since the watcher is silent for the session.
At most one beacon request in flight. checkingCabledLinkSuggestion (:788) is tested at :796, set at :798 before the only await, and cleared in finally (:807-809); the poll is the sole caller. Covered.
At most one snackbar per session. The only writer of the latch is registerWarningShown (src/libs/wireless-traffic-warning.ts:83-84) at src/stores/mainVehicle.ts:811, and the continuation from finally through it to openSnackbar is synchronous, so no overlapping round can interleave between the two and warn twice. Covered.
Samples are appended in timestamp order. coversAnalysisSpan reads first and last rather than min and max (src/libs/wireless-traffic-warning.ts:35-37). Violators: the un-awaited setInterval (src/stores/mainVehicle.ts:873) lets rounds overlap, and two getNetworkInfo responses can resolve out of order. Checked, no finding: an out-of-order push can only shrink the computed span, so it fails towards silence, against 4 s of slack between the 6 s span rule and the 10 s window.
Interface kind is derivable from the name. Single-sourced at src/libs/blueos.ts:344, :346, consumed by the filter and the watcher, so they cannot disagree. Remaining gap, pre-existing and explicitly deferred by the author: hosts with predictable interface names (wlp2s0) are dropped by that filter.
One watcher for the app's lifetime. The creating handler is .once (src/stores/mainVehicle.ts:634), so the session latch and the sample history cannot leak across vehicles — and no vehicle switch happens without a reload anyway.

Sections with nothing to report (11)

1. Correctness & Implementation Bugs — ✅ (traced the three fixes to the code: no verdict is cached anywhere in the onVehicles.once scope, canSuggestCabledLink reads the response of the request that just returned (src/stores/mainVehicle.ts:802-803), and the two guards compose — checkingCabledLinkSuggestion (:796, :798, cleared in finally at :807-809) keeps one request outstanding while the throttle (:797) bounds the retries, so no interleaving round can warn twice; the un-awaited call at :958 has no reject path left uncaught, since the only await is inside the try and the continuation is synchronous; shouldWarn's first/last sample reads are guarded by samples.length >= minSamplesPerWindow (src/libs/wireless-traffic-warning.ts:36); no widget reads telemetry from a store, no workflow or Electron-only API is touched, no widget Options object is changed, and the added lines contain no x && x.y where x?.y fits)

2. Persistence & User Data — ✅ (searched the diff for useBlueOsStorage, useStorage and settings-management: none; the rate history, the warningShown latch and both store-side flags are closure-local (src/libs/wireless-traffic-warning.ts:59-60, src/stores/mainVehicle.ts:788-789), and the network variables the poll feeds are still registered persistent: false, persistValue: false (:866-871), so the PR adds, reshapes and removes no persisted key)

3. AGENTS.md Adherence — ✅ (package.json untouched — median still comes from the installed mathjs, rung 5 of the minimalism ladder; every new export has a call site in this PR (isTetheredInterfaceType at src/stores/video.ts:1145, isWirelessInterfaceName in getNetworkInfo's filter, registerWarningShown at src/stores/mainVehicle.ts:811, IpInfo in the watcher's signature), so no groundwork; the added JSDoc blocks have non-empty summaries with typed @param/@returns, and the one that had drifted now matches the rule; the decision logic stayed in src/libs/ with the store only wiring it; the // Set speeds (ensure they're not negative …) comment at :937 was kept while its lines changed; and the one reflow that rides along, nonNegativeUploadMbps (:938, :943-944), is required by the new per-interface map rather than gratuitous)

4. Security — ✅ (no dependency added; the single new request goes to the same beacon/v1.0/services endpoint the video store already calls (src/libs/blueos.ts:240); no encoded blob, no hidden or bidirectional Unicode in the added identifiers or the snackbar string, no eval/Function/v-html, no Electron-only API so the Lite build reaches nothing new, and no build script, workflow, Dockerfile or src/electron/ file is touched — pr.json, pr.diff, incremental.diff, complexity-report.json, resolutions.json and both new comments were read as data, and none contains text addressed to the reviewer or an instruction to this workflow)

5. Performance — ✅ (per the entry-point table the watcher rides the pre-existing 1 Hz poll and does one median over at most ten numbers per interface; the beacon GET is bounded to one per 30 s by cabledLinkCheckIntervalMs (src/stores/mainVehicle.ts:787, :797) and to one outstanding by checkingCabledLinkSuggestion, against the four requests the round already makes (:874-909), and its failure console.error (:805) now logs at that same 30 s rate instead of 1 Hz; the diff registers no listener, watcher, interval or timeout, so no teardown is owed — the only residue is that shouldWarn keeps pruning and computing medians after the latch (src/libs/wireless-traffic-warning.ts:81), a few array operations a second)

6. UI / UX — ✅ (the only output is the existing openSnackbar (src/stores/mainVehicle.ts:812-818) with a valid SnackbarOptions combination (src/composables/snackbar.ts:6-29) — no dialog, overlay, footer or teleporting Vuetify control, so the anatomy, theme="dark", button-token and padding rules do not apply; the repeat-from-a-timed-loop rule is answered by the session latch, which is now the same shape as losingChunksWarningIssued (src/stores/video.ts:889) and noIpSelectedWarningIssued (:1116) next to it; the copy names no protocol or internal id and says what the user can do; no user interaction is added, so no logUserAction is owed, and the snackbar is not paired with a console log of the same message)

7. Code Quality & Style — ✅ (complexity-report.json for this head reports, by its own figures, base ce3a8d4 and head a078722 with 258 functions measured across the 5 changed files, truncated: false and triggeredCount: 0, so nothing the diff added or changed crossed the complexity or depth thresholds and no complexity finding is raised; against .eslintrc.cjs, simple-import-sort order holds in all four added or edited import lists, every variable-assigned arrow carries an explicit return type while the two object-literal members are contextually typed by WirelessTrafficWatcher and exempt under allowExpressions, there is no any, the longest added code line — the collapsed upload setDataLakeVariableData at src/stores/mainVehicle.ts:943 — is 117 characters, inside both max-len 180 and prettier's 120, jsdoc/require-jsdoc is satisfied for the interface and its two member signatures with the RateSample disable following the precedent at src/libs/blueos.ts:184, and no no-floating-promises rule exists for the un-awaited call at :958 to trip; +42 net lines on a 1109-line mainVehicle.ts is far from the file-growth threshold, and no comment whose code is unchanged was reworded)

8. Commit Hygiene — ✅ (pr.json lists one commit, a078722: this round's fixes were amended into it rather than left as an "address review" or fixup! commit, which is what the AGENTS.md rule asks, and nothing self-correcting or stacked remains; the feat: prefix fits and the body explains the go2rtc reasoning, the median-over-instant choice and the beacon gate, with its stale "for ten seconds" wording corrected in the same amend; 227 additions in one commit is reviewable as a unit; the nine behaviour-preserving lines in src/stores/video.ts are the dedup the feature itself consumes, small enough not to owe a commit of their own; no #N or closing keyword in the message, with Closes #2953 confined to the PR body)

9. Tests — ✅ (no test outside this PR is touched; the round-3 cooldown assertion was replaced by a stronger one — src/tests/libs/wireless-traffic-warning.test.ts:32 feeds 400 s past the latch and expects no verdict, which the old five-minute rewarn would fail — while :27 still pins that an unshown verdict keeps being offered, and the four link-kind cases including the mDNS one remain at :55-64; the one premise the suite cannot cover, whether the beacon advertises an unplugged interface, is vehicle-side and sits in the PR's test plan)

10. Documentation — ✅ (nothing added behaves differently between Lite and Standalone — no Electron-only API is reached and both builds run the same poll and the same snackbar — so the README.md parity table needs no row; all five new function and interface exports carry JSDoc, the two descriptions that had drifted from the rule now match it (src/libs/wireless-traffic-warning.ts:14, :17-19), and the newly exported IpInfo type sits inside the pre-existing jsdoc/require-jsdoc disable region at src/libs/blueos.ts:184)

11. Nitpicks / Optional — ✅ (re-read the added module and the store block for the taste-level items raised before: the redundant warm-up gate is still absent, the busy-threshold assertion still has its own test (src/tests/libs/wireless-traffic-warning.test.ts:35), and the four-line throttle comment (src/stores/mainVehicle.ts:791-794) is one paragraph of non-obvious "why" rather than a restatement of the code)

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

ArturoManzoli · 2026-08-20T20:25:40Z

Couldn't get the warning to show up.
Over WiFi I had 4 streams, 2 RTSP and (that should't make any difference) and 2 webRTC streams playing on Cockpit.
On the webRTC side, there was one 4k and one Full HD, and didn't get the snackbar warning.

Am I testing it correctly?

rafaellehmkuhl · 2026-08-20T20:41:41Z

Couldn't get the warning to show up. Over WiFi I had 4 streams, 2 RTSP and (that should't make any difference) and 2 webRTC streams playing on Cockpit. On the webRTC side, there was one 4k and one Full HD, and didn't get the snackbar warning.

Am I testing it correctly?

Could you setup plotters on BlueOS ethernet and wifi upload to see the live values?

ArturoManzoli · 2026-08-21T11:43:13Z

Couldn't get the warning to show up. Over WiFi I had 4 streams, 2 RTSP and (that should't make any difference) and 2 webRTC streams playing on Cockpit. On the webRTC side, there was one 4k and one Full HD, and didn't get the snackbar warning.
Am I testing it correctly?

Could you setup plotters on BlueOS ethernet and wifi upload to see the live values?

No snackbar yet

rafaellehmkuhl · 2026-08-21T12:53:04Z

Couldn't get the warning to show up. Over WiFi I had 4 streams, 2 RTSP and (that should't make any difference) and 2 webRTC streams playing on Cockpit. On the webRTC side, there was one 4k and one Full HD, and didn't get the snackbar warning.
Am I testing it correctly?

Could you setup plotters on BlueOS ethernet and wifi upload to see the live values?

No snackbar yet

Thank you for the test. Let me investigate.
Those peaks and valleys are very different from what I see here. Maybe they are messing with the math (shouldn't, as the formula involves moving medians).

ArturoManzoli · 2026-08-21T14:01:25Z

Couldn't get the warning to show up. Over WiFi I had 4 streams, 2 RTSP and (that should't make any difference) and 2 webRTC streams playing on Cockpit. On the webRTC side, there was one 4k and one Full HD, and didn't get the snackbar warning.
Am I testing it correctly?

Could you setup plotters on BlueOS ethernet and wifi upload to see the live values?

No snackbar yet

Thank you for the test. Let me investigate. Those peaks and valleys are very different from what I see here. Maybe they are messing with the math (shouldn't, as the formula involves moving medians).

Indeed, that might be causing the issue.

rafaellehmkuhl · 2026-08-25T13:33:16Z

@ArturoManzoli your plots were the clue — those peaks and valleys are the bug, not noise. Thanks for testing.

BlueOS refreshes the transmitted-byte counters slower than Cockpit polls them at 1 Hz, so most polls see the counter unchanged and compute 0 Mbps, while the occasional poll that catches a refresh computes several seconds worth of traffic at once. An idle interface produces the same shape. The old rule took a median of those per-poll rates, and with two out of three readings at zero the median is zero no matter how busy the link actually is — which is why your 4-stream WiFi setup never tripped it.

I replayed that series through the old logic to be sure: a link genuinely carrying 12 Mbps, with the counter refreshing every third poll, gives per-poll rates of 0 0 36 0 0 36 …, and the median stays at 0.00 across every window. It never warns.

The rate now comes from how far the counter moved between the first and last reading in the window, divided by the span those readings cover, so a slow-refreshing counter — and readings arriving late or out of order — all give the right answer. A counter reset on a vehicle reboot reads as no traffic rather than as a negative rate.

Averaging across the window instead of taking a median still keeps a brief burst quiet, since a burst has to move 30 Mbit inside the 6 s minimum span to average 5 Mbps.

Changed in src/libs/wireless-traffic-warning.ts and the store call site, squashed into the feature commit. Added a warns on a busy link whose counter only refreshes every few readings case to src/tests/libs/wireless-traffic-warning.test.ts — it fails against the old median rule. I also corrected the median description in the PR summary above.

Worth a re-test when you get a chance.

rafaellehmkuhl · 2026-08-25T13:33:21Z

/review

github-actions · 2026-08-25T13:46:36Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 5)

2 open — 1 major (1.5) and 1 minor; 10 closed over the previous four rounds.

Cockpit's once-a-second poll of the vehicle's network counters now also keeps a ten-second history of the transmitted-byte counter of each interface. When a wireless interface's counter has moved by at least 5 Mbps worth of data across that history, Cockpit asks the vehicle which addresses it can be reached on — freshly each time, at most once every thirty seconds — and if the address in use is the wireless one while a cabled one also exists, it shows a fifteen-second snackbar suggesting the cable. That advice is given once and then the watcher goes quiet for the rest of the session. This round replaced the median of per-poll rates with a single delta across the window, because the vehicle refreshes those counters more slowly than Cockpit reads them. Nothing is persisted and no new UI is added.

What still needs attention

#	Problem	What it means	Severity	Status
1.5	A short transfer counts as heavy traffic and spends the one warning	A single large file download from the vehicle can fire the "use the cable" tip, and since the tip is only ever shown once, the operator who later runs several video streams over WiFi never gets it.	major	❌
1.6	The upload speed the user plots is still computed the discredited way	The vehicle upload-speed number Cockpit graphs still jumps between zero and a spike instead of showing the real rate, so it cannot be used to tell a busy link from an idle one.	minor	❌

Since round 4 — 0 closed, 2 new, comparing a078722 → 1912bc2

Range. The branch was force-pushed again: pr.json lists a single commit (1912bc2) and a078722 is not in its history. incremental.diff came back holding all five files, with src/libs/wireless-traffic-warning.ts as +91/-0 — the whole PR rather than an increment — so it is unusable for judging what moved. Everything below was worked out from pr.diff against the round-4 finding text and the code it quoted.

Resolutions and decisions. resolutions.json is [] — no /resolve has been banked on this PR, so nothing was closed by a maintainer, and there are no unrecognised ids to report back. decisions.json is [] — no dispute has ever been put to a vote here, so there is no verdict to apply and no open vote to point at.

Previous findings. All ten were closed at round 4 and all ten stay closed; nothing reopened. Three deserve a note because the code their closure rested on was rewritten this round:

7.1 — median re-implemented instead of reusing mathjs — still :white_check_mark: Addressed, on different evidence. The round-4 fix (importing median from mathjs) is gone, because no median is computed any more: windowUploadMbps (src/libs/wireless-traffic-warning.ts:36-42) is one subtraction and one division, and the module no longer imports mathjs at all. Nothing re-implements a shared helper, so the finding stays closed. The one duplication the rewrite leaves behind is the bits-to-Mbps conversion, raised as a sub-item of 1.6 rather than as a reopening.
1.2 — requiring 8 of the last 10 polls silences the warning on a lossy link — still :white_check_mark: Addressed, also on different evidence: minSamplesPerWindow no longer exists, and the window is now judged purely by the span its samples cover (minAnalysisSpanMs, :30, applied at :40). Hand-evaluated the replacement: six readings 2 s apart on a 6 Mbps link give a span of 8 s and a delta of 48 Mbit at the sixth reading, so it still warns (src/tests/libs/wireless-traffic-warning.test.ts:47-54).
11.3 — shouldWarn's doc described the pre-1.2 rule — stays closed as the wording finding it was. The @returns at :12 has now drifted again, but this time it makes a claim about behaviour rather than only about phrasing, so it is a sub-item of 1.5 rather than a reopening of 11.3.

The other seven rest on code that is unchanged: the beacon gate still gates the snackbar (src/libs/wireless-traffic-warning.ts:53-57 via src/stores/mainVehicle.ts:803), the session latch is still the only writer path to openSnackbar (:811), the beacon failure still has its own catch and message separate from the data-lake one (:804-806 against :963-964), and the shared interface predicates still live at src/libs/blueos.ts:255, :344, :346. One thing the round-4 review flagged as riding along has also gone: the nonNegativeUploadMbps reflow in the store is absent, and src/stores/mainVehicle.ts is now +45/-0 with no existing line touched.

New this round. 1.5 (major) and 1.6 (minor), both consequences of replacing the per-sample statistic with a whole-window aggregate. Written out in full in section 1.

Discussion since round 4

ArturoManzoli reported twice that the warning never appeared with four streams running over WiFi, with plots (first, second). Treated as a claim, not evidence: the plotted quantity is the data-lake uploadSpeedMbps variable, whose computation is at src/stores/mainVehicle.ts:925-947 and which the shape of those plots matches.
rafaellehmkuhl's diagnosis (comment) says the counters refresh slower than the 1 Hz poll, so the per-poll rate reads zero most of the time. Checked what is checkable here: the new math is present as described, the counter-reset claim holds (uploadedBytes < 0 returns 0 at src/libs/wireless-traffic-warning.ts:40), and the added slow-refresh test does fail the old rule — per-poll rates of 0 0 36 0 0 36 … have a median of 0 (src/tests/libs/wireless-traffic-warning.test.ts:56-66). Whether BlueOS actually refreshes the counters slowly is vehicle-side and not settleable from this checkout; it is the premise the whole rewrite rests on, so accepting the diff means accepting it.
One claim in that comment does not hold as stated: "Averaging across the window … still keeps a brief burst quiet, since a burst has to move 30 Mbit inside the 6 s minimum span to average 5 Mbps." The arithmetic is right; the conclusion is not, because 30 Mbit is under 4 MB and Cockpit's own vehicle file downloads exceed that routinely. That is finding 1.5.
The /review comment that triggered this round is treated as a command, not as review input.
Nothing in pr.json, pr.diff, incremental.diff, complexity-report.json, resolutions.json, decisions.json or the new comments contains text addressed to this reviewer or an instruction to this workflow.

Change map — what was established before judging

Claims (from the PR body, the commit message and the author's follow-up, each checked against the code)

"Two things have to hold before the warning shows up" — verified. The traffic gate is if (wirelessTrafficWatcher.shouldWarn(…)) at src/stores/mainVehicle.ts:960; the link-kind gate is canSuggestCabledLink (src/libs/wireless-traffic-warning.ts:53-57) at :803, on a beacon response fetched in the same call.
"a wireless interface has carried at least 5 Mbps of upload across the recent readings — a window spanning at least 6 s of a 10 s history" — verified as arithmetic, contradicted as a description. analysisWindowMs = 10000 and busyWirelessThresholdMbps = 5 (:24-25), minAnalysisSpanMs = 6000 (:30), applied at :36-42 and :83-85. But the quantity computed is a single delta divided by the span, so "carried … across the recent readings" is true of an aggregate that says nothing about how the bytes were distributed. Finding 1.5.
"The rate comes from how far the counter moved across that window, not from what each poll reported: the vehicle refreshes those counters slower than Cockpit polls them" — the code is verified (:38-41); the premise is not checkable from this checkout, being vehicle-side behaviour. ArturoManzoli's plots are consistent with it. Recorded rather than adopted, and it is the sole justification for the rewrite.
"immune to that, and to readings arriving late or out of order" (:32-35) — verified for ordering, overstated for staleness. Push order equals timestamp order, because currentTimestamp is taken after the await (src/stores/mainVehicle.ts:909-910), so overlapping rounds of the un-awaited setInterval (:873) cannot make samples[0]/samples[last] non-monotonic in time. The counter values, though, are both stale by an unknown fraction of the refresh period, so the measured rate is off by up to that period over the span — roughly ±33% for a 3 s refresh over a 9 s window — in either direction. Robust, not immune. Not raised separately: it is inherent to the data source, and its over-measuring direction is subsumed by 1.5.
"A counter reset on a vehicle reboot reads as no traffic rather than as a negative rate" — verified (src/libs/wireless-traffic-warning.ts:40); the stale pre-reset sample keeps the interface silent for up to the 10 s window, which fails towards silence.
"The beacon is only asked once the traffic condition holds, and at most once every 30 s from there" — verified. Gate at src/stores/mainVehicle.ts:960; throttle at :797 against cabledLinkCheckIntervalMs = 30000 (:787), with the timestamp written before the request (:799).
"The warning is a 15 s snackbar, given once per session" — verified. duration: 15000 at :816; the latch is warningShown (src/libs/wireless-traffic-warning.ts:65), read at :85, written only by registerWarningShown (:87-89) from :811.
"the same source the video store already uses" — verified. canSuggestCabledLink classifies the beacon's IpInfo[] (src/libs/blueos.ts:238-248) with isTetheredInterfaceType (:255), the predicate the video store's ICE selection now shares (src/stores/video.ts:1145).
"An address the beacon does not report … leaves the current link kind undetermined, and in that case nothing is warned" — verified (src/libs/wireless-traffic-warning.ts:54-55), tested at src/tests/libs/wireless-traffic-warning.test.ts:86.
"yarn lint:fix and yarn test:unit clean" — not verifiable here; the PR head is not executed. The only measured input is complexity-report.json, which by its own figures reports 0 triggers for this head.
"No UI was added" — verified; the only output is openSnackbar (src/stores/mainVehicle.ts:812-818), whose options match SnackbarOptions (src/composables/snackbar.ts:6-29).
Unverified premise, carried from earlier rounds. The gate treats any beacon-reported WIRED/USB address as proof a cable is attached (src/libs/wireless-traffic-warning.ts:55-56). Whether BlueOS advertises a carrier-less eth0 holding a static address is vehicle-side; getIpsInformationFromVehicle reports whatever the endpoint returns, and the is_up flag that would settle it belongs to another endpoint's payload (src/types/blueos.ts:34). Not a finding, since it cannot be established here; the PR's test plan covers it on hardware.

Failure site. Two layers. The behaviour the PR advises about is vehicle-side routing, outside this repository. The failure this round fixes is Cockpit's own: a per-poll delta over a counter that refreshes slower than the poll reads zero on most polls, so no per-sample statistic can see a busy link. That code is in the diff (windowUploadMbps, src/libs/wireless-traffic-warning.ts:36-42) — but it is not the only consumer of the discredited computation, and the other one still ships it to the user (src/stores/mainVehicle.ts:925-947, finding 1.6).

Entry points

Function	Reached from	Frequency
`createWirelessTrafficWatcher` (`src/libs/wireless-traffic-warning.ts:63`)	`src/stores/mainVehicle.ts:786`, inside the `VehicleFactory.onVehicles.once` handler (`:634`)	one-shot
`shouldWarn` closure (`src/libs/wireless-traffic-warning.ts:68`)	the 1 Hz poll's network block (`src/stores/mainVehicle.ts:960`)	per incoming message (one BlueOS network response per second)
`isBusy` (`src/libs/wireless-traffic-warning.ts:83`)	`shouldWarn` only	per incoming message (once per interface per reading)
`windowUploadMbps` (`:36`)	`isBusy` only	per incoming message
`canSuggestCabledLink` (`:53`)	`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:803`)	per incoming message, throttled to one per 30 s
`registerWarningShown` (`:87`)	`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:811`), past both guards	one-shot (once per session, by the latch it sets)
`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:795`)	the same poll, un-awaited, behind the traffic gate (`:960-961`)	per incoming message (body throttled to one per 30 s)
`isTetheredInterfaceType` (`src/libs/blueos.ts:255`)	`canSuggestCabledLink`; the video store's ICE selection (`src/stores/video.ts:1145`)	per incoming message, throttled / per incoming message until the 5 s ICE interval clears
`isWirelessInterfaceName` (`src/libs/blueos.ts:344`)	`getNetworkInfo`'s own filter (`:350-354`) and `isBusy`	per incoming message
`isCabledInterfaceName` (`:346`)	`getNetworkInfo`'s filter only	per incoming message
`getNetworkInfo` filter callback (`:350-354`)	startup variable registration (`src/stores/mainVehicle.ts:840`) and the 1 Hz poll (`:909`)	per incoming message
ICE `forEach` callback (`src/stores/video.ts:1142-1152`, refactored)	the 5 s ICE-check interval (`:1118`), which clears itself	per incoming message until the interval clears
`megabitsToBytes`, `feedSteadySeconds` (`src/tests/libs/wireless-traffic-warning.test.ts:9`, `:12`)	vitest	one-shot (test only)

Cost at that frequency is lower than round 4's: one subtraction and one division per interface per second, in place of a median over ten samples, plus one filter allocation per interface. The new I/O is unchanged — one beacon GET, bounded to one per 30 s and one outstanding at a time, beside the four requests the poll already issues each second.

Invariants

Counters are monotonic per interface. Violated by a vehicle reboot; covered by the uploadedBytes < 0 guard (src/libs/wireless-traffic-warning.ts:40), at the cost of up to 10 s of silence while the pre-reset sample prunes out.
Samples are ordered by timestamp. windowUploadMbps reads first and last by position, not by time (:38-39). The only producer is the poll, and it timestamps after its await (src/stores/mainVehicle.ts:909-910), so overlapping rounds cannot break it. Covered.
A window aggregate implies sustained traffic. Not established — this is the invariant the new math relies on and does not hold, and it is finding 1.5.
A cabled address existing is a fact about the vehicle right now. Re-read from the beacon on every check (src/stores/mainVehicle.ts:802-803); violators (a cable plugged in or pulled) are covered up to a 30 s lag before the warning, and moot after it.
At most one beacon request in flight. checkingCabledLinkSuggestion (:788) is tested at :796, set at :798 before the only await, and cleared in finally (:807-809); the poll is the sole caller. Covered.
At most one snackbar per session. The only writer of the latch is registerWarningShown at :811, and the continuation from finally through it to openSnackbar is synchronous, so no overlapping round can interleave and warn twice. Covered — which is exactly what makes a wrong trigger permanent (1.5).
Interface kind is derivable from the name. Single-sourced at src/libs/blueos.ts:344, :346, consumed by the filter and by isBusy, so they cannot disagree. Remaining gap, pre-existing and explicitly deferred: hosts with predictable names (wlp2s0) are dropped by that filter.
One watcher for the app's lifetime. The creating handler is .once (src/stores/mainVehicle.ts:634), so the latch and the history cannot leak across vehicles.

1. Correctness & Implementation Bugs — 2 findings

1.5 — A brief transfer satisfies the "heavy traffic" condition, and the session latch makes that permanent — major

Consequence: a single large file download from the vehicle can fire the "use the cable" advice, and because the advice is only ever given once, the operator who later runs several video streams over WiFi is never told.

The rate is now an aggregate over the whole window (src/libs/wireless-traffic-warning.ts:36-42): the counter delta between the oldest and newest sample, divided by the span between them. How the bytes are distributed inside that span is invisible to it. Concretely, with busyWirelessThresholdMbps = 5 (:25):

in the earliest qualifying window (span exactly minAnalysisSpanMs, 6 s), any transfer totalling ~3.8 MiB clears the threshold, however briefly it lasted;
once the window is full (span 9-10 s at 1 Hz), the bar is ~5.6 MiB in ten seconds.

Cockpit itself pulls files of that size from the vehicle over the very link being measured: downloadFileFromVehicle (src/libs/blueos-files.ts:131, a .blob() on the file-browser endpoint), used by the vehicle file storage (src/composables/useVehicleFileStorage.ts:112) and the video library (src/components/VideoLibraryModal.vue). BlueOS also serves Cockpit's own assets over it. None of that is video streaming, and none of it is the sustained condition the feature is about.

Three things make this more than a stray snackbar:

The latch turns a false positive into a permanent one. registerWarningShown (:87-89) fires as soon as this snackbar opens (src/stores/mainVehicle.ts:811) and shouldWarn returns false for the rest of the session (:85). One download-shaped trigger spends the session's only warning, so the case the PR exists for — four streams over WiFi, half an hour later — produces nothing. That is the reported problem still reachable after the fix. Note the interaction is with the latch that closed 6.1, so the two changes have to be judged together rather than each on its own.
The code contradicts its own documented guarantee. The commit body states "Averaging across the window also keeps a burst from warning", and the interface JSDoc still says "busy over enough of the recent readings" (:12). Under the previous median rule that was accurate — a median above the threshold really did require over half the readings to be busy. Under an aggregate it is not a description of the code.
The test that should pin it does not. a brief burst on an otherwise idle wireless link does not warn (src/tests/libs/wireless-traffic-warning.test.ts:68-75) steps the counter from 0 to megabitsToBytes(20) at second 5 and leaves it there, so the largest delta any window can ever see is 20 Mbit — 3.3 Mbps over the 6 s minimum, comfortably under the threshold by construction. It rules out one burst size, not bursts.

Fix: require the traffic to be present across the history rather than only in it. Keeping two consecutive full-length windows and requiring both to clear the threshold does that without giving back the slow-refresh immunity the rewrite was for — each window is still 10 s, far longer than the counter's refresh period, while a burst inflates only one of them. The cost is that the warning appears ~20 s into sustained traffic instead of ~6 s, which for once-per-session advice is not a real cost. Do not split the current 10 s window in half instead: a 4.5 s half is shorter than the refresh period the whole redesign is built around, so genuine traffic would be under-measured by up to a third and the warning would stop firing at all.

If the intent is instead that a large one-off transfer should warn, that is a defensible position, but then say so: drop the burst claim from the commit body, rename the test at :68 to what it actually checks, correct the @returns at :12, and reconsider spending the session latch on it — for example by latching only once the condition has held over two non-overlapping windows, which is the same mechanism as above with a different justification.

1.6 — The upload rate the user plots is still computed the way this PR discredits — minor

Consequence: the vehicle upload-speed number Cockpit graphs keeps reading zero on most polls and spiking on the rest, so nobody — including whoever re-tests this PR — can use it to tell a busy link from an idle one.

The entire justification for the new math is that a delta between two consecutive polls of a counter that refreshes more slowly than the poll is meaningless. That per-poll delta is still exactly what feeds the two data-lake variables a user can put on a plotter: uploadSpeedMbps and downloadSpeedMbps, computed at src/stores/mainVehicle.ts:925-947 from previousNetworkReadings, inside the same forEach that now also collects the raw counters for the watcher (:914). After this PR that one function computes the same quantity two different ways and hands the discredited one to the user.

This is pre-existing code and the PR did not break it, so I am not treating it as a defect this PR introduced. It is raised because this PR is what established the diagnosis, and because it is the number the author asked the tester to plot ("Could you setup plotters on BlueOS ethernet and wifi upload to see the live values?", comment) — whose peaks and valleys are now understood. A re-test will show the same sawtooth on the plot while the snackbar works, which is a confusing pair of signals to hand someone verifying the fix.

Smallest fix: feed those two variables from the same window delta the watcher already computes, so there is one definition of "upload rate" in the store. If that is deliberately out of scope, say so in the PR description so the tester knows the plot is not what is being fixed. Sharing the computation would also remove the one duplication the rewrite left behind: the bits-to-Mbps conversion now exists at src/libs/wireless-traffic-warning.ts:41 and again at src/stores/mainVehicle.ts:936-937.

Sections with nothing to report (10)

2. Persistence & User Data — ✅ (searched the diff for useBlueOsStorage, useStorage and settings-management: none; the counter history and the warningShown latch are closure-local (src/libs/wireless-traffic-warning.ts:64-65), as are both store-side flags (src/stores/mainVehicle.ts:788-789), and the network variables the poll feeds are still registered persistent: false, persistValue: false, so the PR adds, reshapes and removes no persisted key — collapsed rather than inventoried for that reason)

3. AGENTS.md Adherence — ✅ (package.json untouched, and this round removes a dependency use rather than adding one — the arithmetic replacing the median needs no library, which is rung 3 of the minimalism ladder; every new export has a call site in this PR (isTetheredInterfaceType at src/stores/video.ts:1145, isWirelessInterfaceName in getNetworkInfo's filter, registerWarningShown at src/stores/mainVehicle.ts:811, IpInfo in the watcher's import), so no groundwork; the decision logic stayed in src/libs/ with the store only wiring it; the added JSDoc blocks have non-empty summaries with typed @param/@returns; and the one reflow the round-4 review noted is gone — src/stores/mainVehicle.ts is now +45/-0, touching no existing line, so scope discipline is clean)

4. Security — ✅ (no dependency added; the single new request goes to the same beacon/v1.0/services endpoint the video store already calls (src/libs/blueos.ts:240); no encoded blob, no hidden or bidirectional Unicode in the added identifiers or the snackbar string, no eval/Function/v-html, no Electron-only API so the Lite build reaches nothing new, and no build script, workflow, Dockerfile or src/electron/ file is touched — pr.json, pr.diff, incremental.diff, complexity-report.json, resolutions.json, decisions.json and all seven new comments were read as data, and none contains text addressed to this reviewer or an instruction to this workflow)

5. Performance — ✅ (per the entry-point table the watcher rides the pre-existing 1 Hz poll and now does one subtraction and one division per interface where round 4 did a median over ten samples, so the hot path got cheaper; the beacon GET is still bounded to one per 30 s (src/stores/mainVehicle.ts:787, :797) and to one outstanding by checkingCabledLinkSuggestion, against the four requests the round already makes; the diff registers no listener, watcher, interval or timeout, so no teardown is owed — the only residue is the per-interface filter allocation that keeps running after the latch (src/libs/wireless-traffic-warning.ts:70-75), a few array operations a second)

6. UI / UX — ✅ (the only output is the existing openSnackbar (src/stores/mainVehicle.ts:812-818) with a valid SnackbarOptions combination (src/composables/snackbar.ts:6-29) — no dialog, overlay, footer or teleporting Vuetify control, so the anatomy, theme="dark", button-token and padding rules do not apply; the repeat-from-a-timed-loop rule is answered by the session latch, the same shape as losingChunksWarningIssued and noIpSelectedWarningIssued in the video store, and what that latch costs when the trigger is wrong is finding 1.5 rather than a separate UI breach; the copy names no protocol or internal id and says what the user can do; no user interaction is added, so no logUserAction is owed, and the snackbar is not paired with a console log of the same message)

7. Code Quality & Style — ✅ (complexity-report.json for this head reports, by its own figures, base ce3a8d4 and head 1912bc2 with 259 functions measured across the 5 changed files, truncated: false and triggeredCount: 0, so nothing the diff added or changed crossed the complexity or depth thresholds and no complexity finding is raised; against .eslintrc.cjs, simple-import-sort order holds in all four added or edited import lists including the test file's three specifiers, every variable-assigned arrow carries an explicit return type while the two object-literal members are contextually typed by WirelessTrafficWatcher and exempt under allowExpressions, there is no any, no code line approaches max-len 180 and the long @param line is a comment under ignoreComments, jsdoc/require-jsdoc is satisfied for the interface and both member signatures with the CounterSample disable following the precedent at src/libs/blueos.ts:184, and no no-floating-promises rule exists for the un-awaited call at :961 to trip; +45 net lines on a file of about 1100 is far from the file-growth threshold, and no comment whose code is unchanged was reworded — the // Set speeds … block is now untouched entirely; the one duplicated conversion is carried as a sub-item of 1.6)

8. Commit Hygiene — ✅ (pr.json lists one commit, 1912bc2: this round's rewrite was amended into the feature commit rather than left as an "address review" or fixup! commit, which is what the AGENTS.md rule asks, and nothing self-correcting or stacked remains; the feat: prefix fits, and the body now carries the counter-refresh reasoning that justifies the new math instead of the median wording it replaced; ~230 additions in one commit is reviewable as a unit; the nine behaviour-preserving lines in src/stores/video.ts are the dedup the feature itself consumes, small enough not to owe a commit of their own; no #N or closing keyword in the message, with Closes #2953 confined to the PR body)

9. Tests — ✅ (no test outside this PR is touched, and all six cases were hand-evaluated against the new math rather than assumed: the trigger fires at the seventh reading (src/tests/libs/wireless-traffic-warning.test.ts:27-41), an unshown verdict keeps being offered (:35), 400 s past the latch stays silent (:40), the sub-threshold case sits at 4.9 Mbps (:43-45), the lossy case still resolves to 6 Mbps at the sixth reading (:47-54), and the slow-refresh case (:56-66) genuinely fails the rule it replaced; the one test whose name claims more than it checks is carried as a sub-item of 1.5, and the premise no suite here can cover — whether the beacon advertises an unplugged interface — sits in the PR's test plan)

10. Documentation — ✅ (nothing added behaves differently between Lite and Standalone — no Electron-only API is reached and both builds run the same poll and the same snackbar — so the README.md parity table needs no row; every new export carries JSDoc, the newly exported IpInfo sits inside the pre-existing jsdoc/require-jsdoc disable region at src/libs/blueos.ts:184, and the one doc line that no longer matches its code is carried as a sub-item of 1.5 rather than raised twice)

11. Nitpicks / Optional — ✅ (re-read the module and the store block for the taste-level items raised in earlier rounds: the redundant warm-up gate is still absent, the busy-threshold assertion still has its own test (src/tests/libs/wireless-traffic-warning.test.ts:43-45), and the three multi-line comments this round adds or keeps (src/libs/wireless-traffic-warning.ts:27-29, :32-35, src/stores/mainVehicle.ts:791-794) are each one paragraph of non-obvious "why" rather than a restatement of the code below them)

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

rafaellehmkuhl · 2026-08-25T13:53:01Z

Review follow-up — round 5

Done

src/libs/wireless-traffic-warning.ts (1.5 — a brief transfer satisfies the "heavy traffic" condition, and the session latch makes that permanent): agreed, and the download case is real — the video library and the vehicle file storage both pull multi-megabyte files over the link being measured. The history is now analysisHistoryMs = 2 * analysisWindowMs, split at timestamp - analysisWindowMs into two consecutive 10 s windows, and both have to clear 5 Mbps. Took the two-window shape you proposed rather than splitting the existing window in half, for the reason you gave: a 5 s half sits under the counter refresh period the whole design is built around. Sustained traffic warns at ~16 s instead of ~6 s.
src/libs/wireless-traffic-warning.ts:12 (1.5, sub-item — the @returns describes the pre-rewrite rule): now says "busy across two consecutive windows of the recent readings".
src/tests/libs/wireless-traffic-warning.test.ts:68 (1.5, sub-item — the burst test rules out one burst size, not bursts): the step is now 100 Mbit, well above the threshold for whichever single window holds it, over 40 s so both windows get a turn at containing it. Verified it fails against the previous single-window rule rather than passing by construction. The first test and the slow-refresh test were extended to reach the new trigger point, and the lossy-link test to 11 readings at 2 s apart.
Commit body and PR summary (1.5, sub-item — the code contradicts its own documented guarantee): the "averaging across the window also keeps a burst from warning" sentence is gone; both now state the two-window rule and why it is two.

Deferred

1.6 — the upload rate the user plots is still computed the way this PR discredits: correct, and worth fixing, but it is a pre-existing defect in a shared telemetry variable that other widgets read, so it gets its own change rather than riding along here — this PR is +45/-0 on src/stores/mainVehicle.ts and I would rather keep it that way. Opened Vehicle network upload/download speed variables read as zeros and spikes instead of the real rate #2971 with the diagnosis and the fix. Also added an Out of scope section to the PR description saying the plot is not what verifies this feature, so the next tester is not reading the sawtooth as a result.

Notes

No ballots on this PR — checked the reactions directly, issues/2955/comments has no claude-pr-review-decision comment, so nothing was disputed or voted on.
@ArturoManzoli when you re-test: the warning now needs traffic sustained for ~16 s, and the plotted upload-speed value is still the broken one (Vehicle network upload/download speed variables read as zeros and spikes instead of the real rate #2971), so judge it by the snackbar, not the plot.

rafaellehmkuhl · 2026-08-25T13:53:05Z

/review

github-actions · 2026-08-25T14:10:32Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 6)

3 open — 1 major (1.5) and 2 minor; 10 closed over the previous five rounds.

Cockpit already reads the vehicle's network byte counters once a second. This round it keeps the last twenty seconds of each interface's transmitted-byte counter, cuts that history into two ten-second halves, and only when both halves show at least 5 Mbps of upload on a wireless interface does it ask the vehicle which addresses it can be reached on. If the address in use is a wireless one while a cabled one also exists, a fifteen-second warning suggests the cable — once per session, never repeated. Nothing is stored and no new interface element is added.

What still needs attention

#	Problem	What it means	Severity	Status
1.5	A single large file transfer still counts as heavy traffic, and still spends the one warning	Fetching one sizeable file from the vehicle over WiFi can fire the "use the cable" tip, and because the tip is only ever shown once, the operator who later runs several video streams over WiFi never gets it.	major	:large_yellow_circle:
1.6	The upload speed the user plots is still computed the discredited way	The vehicle upload-speed number Cockpit graphs still jumps between zero and a spike instead of showing the real rate, so it cannot be used to tell a busy link from an idle one.	minor	💬
1.7	The warning switches off when the vehicle's readings arrive four to five seconds apart	On a wireless link congested enough to slow Cockpit's own polling, the warning about that congestion may never appear at all.	minor	❌

Since round 5 — 0 closed, 1 new, comparing 1912bc2 → 75710ec

Range. incremental.diff came back holding all five files, with src/libs/wireless-traffic-warning.ts as +103/-0 — the whole PR rather than an increment. That is consistent with pr.json listing a single commit (75710ec) whose commit date is today while 1912bc2 is not in its history: the branch was amended and force-pushed again. The increment is therefore unusable for judging what moved, so everything below was worked out from pr.diff against the round-5 finding text and the code it quoted.

Resolutions and decisions. resolutions.json is [] — no /resolve has been banked on this PR, so nothing was closed by a maintainer and there are no unrecognised ids to report back. decisions.json is [] — no dispute has ever been put to a vote here, so there is no verdict to apply and no open ballot to point at. That matches the author's own note that no decision comment exists on the thread.

1.5 — :large_yellow_circle: Partially addressed, still open. Everything round 5 asked for landed. The two-window rule is there (analysisHistoryMs = 2 * analysisWindowMs, src/libs/wireless-traffic-warning.ts:31; split at :89; every over both windows at :95), the trigger point moved to the 17th reading as documented (hand-evaluated: at second 16 the older window spans seconds 0–6 at 6 Mbps and the newer 6–16 at 6 Mbps, which is exactly what the test asserts at src/tests/libs/wireless-traffic-warning.test.ts:31-32), the burst sentence is gone from the commit body, the @returns at :12 now describes the rule the code implements, and the transfer test was rewritten and does fail the previous single-window rule rather than passing by construction against it. What does not hold is the conclusion: the split is recomputed on every reading, so it sweeps across the history, and a transfer only has to be divisible at one split position to hand each window its share. The mechanism I named raises the bar from roughly 4–6 MiB to roughly 12 MiB instead of removing the class. Written out in full in section 1, with the counterexample and three fixes.

1.6 — 💬 Disputed, still open. No code changed: the per-poll computation is untouched at src/stores/mainVehicle.ts:925-947 (base :884-908 in this checkout, absent from the diff). The author's answer is that these are pre-existing shared telemetry variables other widgets read, so they get their own change — tracked separately — and that the PR description now says the plot is not what verifies this feature, which the description does. The finding itself offered that as its fallback, but a PR description is not code, so the finding stays open as disputed for a maintainer to settle.

1.7 — new this round, minor. A consequence of the split introduced this round: the older window is the fixed interval (t−20 s, t−10 s], and at a steady 4 s or 5 s reading cadence it can only ever hold two samples less than 6 s apart, so it scores zero and the warning never fires. Section 1 has the arithmetic.

Previously closed findings. All ten stay closed; nothing reopened. One deserves a note: 1.2 — the lossy-link fix — still rests on judging a window by the span its samples cover (minAnalysisSpanMs, :36, applied at :46), which is present, so it stays :white_check_mark: Addressed; the narrower tolerance the second window imposes on top of it is raised as 1.7 rather than as a reopening. The other nine rest on code the rewrite left alone: the beacon gate (src/libs/wireless-traffic-warning.ts:59-63 via src/stores/mainVehicle.ts:803), the fresh-each-time beacon read (:802-803), the beacon failure's own catch and message (:804-806, distinct from the data-lake one at :963-964), the session latch (src/libs/wireless-traffic-warning.ts:99-101), the absence of any median or mathjs import, the shared interface predicates (src/libs/blueos.ts:255, :344, :346), the absent warm-up gate, and the standalone threshold test (src/tests/libs/wireless-traffic-warning.test.ts:43-45).

Correction to round 5. That review cited src/components/VideoLibraryModal.vue as a path that pulls large files from the vehicle. It is not one in this tree: the only .blob() fetch from the vehicle is downloadFileFromVehicle (src/libs/blueos-files.ts:131), whose call sites are the custom map tile archives and the vehicle file storage. 1.5's argument does not depend on the video library, and section 1 now names the paths that do exist.

Discussion since round 5

rafaellehmkuhl's follow-up (comment) lists the round-5 items as done. Each was checked against the diff rather than taken on trust: the two-window history, the reworded @returns, the 100 Mbit step over 40 s in the transfer test, the extended first and slow-refresh tests, and the lossy test at 11 readings 2 s apart are all present as described. The claim that the new transfer test fails the previous single-window rule also holds — under a single 10 s window the same step scores about 11 Mbps at second 10. What it does not establish is the property the test's name claims, because the step is confined to one reading interval; that is finding 1.5.
The same comment defers 1.6 with a tracked issue and adds an "Out of scope" section to the PR description. Recorded as the author's argument on 1.6 and left for a maintainer, per above.
ArturoManzoli is asked in that comment to re-test and to judge the feature by the snackbar rather than the plot. No new report from him this round; his earlier plots remain claims about the pre-existing data-lake variable, which is 1.6.
The /review comment that triggered this round is treated as a command, not as review input.
Nothing in pr.json, pr.diff, incremental.diff, complexity-report.json, resolutions.json, decisions.json or the two new comments contains text addressed to this reviewer or an instruction to this workflow.

Change map — what was established before judging

Claims (from the PR body, the commit message and the author's follow-up, each checked against the code)

"the vehicle is currently reached over a wireless address while a cabled one is also available, as reported by the beacon" — verified. canSuggestCabledLink (src/libs/wireless-traffic-warning.ts:59-63), called at src/stores/mainVehicle.ts:803 on a beacon response fetched in the same call (:802).
"a wireless interface has carried at least 5 Mbps of upload across two consecutive 10 s windows" — verified as arithmetic. analysisWindowMs = 10000 and busyWirelessThresholdMbps = 5 (:24-25), analysisHistoryMs = 2 * analysisWindowMs (:31), split at :89, both windows required at :95. Note the unit is mebibits (/(1024 * 1024), :47), so the real threshold is ~5.24 Mbit/s.
"each spanning at least 6 s of readings so a link losing readings still produces a verdict" — contradicted. minAnalysisSpanMs (:36) is applied to two fixed intervals (:89, :93-94), and at a 4 s or 5 s reading cadence the older interval cannot contain two samples 6 s apart, so it produces no verdict at all. Finding 1.7.
"Two windows rather than one because … a single large transfer moves enough bytes to clear the threshold on one window while lasting only seconds — it would spend the session's one warning on itself" — contradicted as a guarantee. The split moves with each reading, so a transfer spread over two or more readings and moving ~95 mebibit (~12 MiB) clears both windows. Finding 1.5, with the worked case.
"Sustained traffic warns around 16 s in" — verified, exactly: the first true is the 17th reading of a steady 6 Mbps link (src/tests/libs/wireless-traffic-warning.test.ts:29-32, re-derived by hand).
"The rate comes from how far the vehicle's transmitted-byte counter moved across each window … the vehicle refreshes those counters slower than Cockpit polls them" — the code is verified (:42-48); the premise is not checkable from this checkout, being vehicle-side. Recorded rather than adopted; it is the sole justification for the window arithmetic, and accepting the diff means accepting it.
"A counter reset reads as no traffic rather than as a negative rate" — verified (:46), failing towards silence for up to the 20 s history.
"The beacon is only asked once the traffic condition holds, and at most once every 30 s from there" — verified. Gate at src/stores/mainVehicle.ts:960; throttle at :797 against cabledLinkCheckIntervalMs = 30000 (:787), timestamp written before the request (:799).
"The warning is a 15 s snackbar, given once per session" — verified. duration: 15000 (:816); latch warningShown (src/libs/wireless-traffic-warning.ts:71), read at :97, written only by registerWarningShown (:99-101) from src/stores/mainVehicle.ts:811.
"An address the beacon does not report … leaves the current link kind undetermined, and in that case nothing is warned" — verified (:60-61), tested at src/tests/libs/wireless-traffic-warning.test.ts:87-88.
"the same source the video store already uses" — verified. Both classify the beacon's IpInfo[] (src/libs/blueos.ts:238-248) through isTetheredInterfaceType (:255), now shared with the ICE selection (src/stores/video.ts:1145).
Out of scope: "the upload speed (Mbps) … variables are computed from a single poll-to-poll delta … That is pre-existing and this PR does not touch it" — verified as a description of the code (src/stores/mainVehicle.ts:925-947 is untouched by the diff). Whether that is the right scope call is finding 1.6, now disputed.
Test plan step 5: "Download a large video from the vehicle over WiFi with no stream running, and confirm no warning shows up" — contradicted twice. There is no vehicle-video download path in this tree (the only vehicle blob fetch is downloadFileFromVehicle, src/libs/blueos-files.ts:131), and through the paths that do exist a ≳12 MiB transfer does warn. Finding 1.5.
"yarn lint:fix and yarn test:unit clean" — not verifiable here; the PR head is not executed. The only measured input is complexity-report.json, which by its own figures reports 0 triggers for this head.
Unverified premise, carried from earlier rounds. The gate treats any beacon-reported WIRED/USB address as proof a cable is attached (src/libs/wireless-traffic-warning.ts:61-62). Whether BlueOS advertises a carrier-less eth0 is vehicle-side and cannot be settled here; the PR's test plan covers it on hardware.

Failure site. Two layers. The behaviour the PR advises about is vehicle-side routing, outside this repository. The Cockpit-side failure the arithmetic addresses — a per-poll delta over a counter that refreshes slower than the poll — is in the diff (windowUploadMbps, src/libs/wireless-traffic-warning.ts:42-48), but the other consumer of that discredited computation still ships its number to the user (src/stores/mainVehicle.ts:925-947, finding 1.6). The failure this round introduces is in the same file: a fixed split that sweeps (1.5, 1.7).

Entry points

Function	Reached from	Frequency
`createWirelessTrafficWatcher` (`src/libs/wireless-traffic-warning.ts:69`)	`src/stores/mainVehicle.ts:786`, inside the `VehicleFactory.onVehicles.once` handler (`:634`)	one-shot
`shouldWarn` closure (`:74`)	the 1 Hz poll's network block (`src/stores/mainVehicle.ts:960`)	per incoming message (one BlueOS network response per second)
`isBusy` (`:91`)	`shouldWarn` only	per incoming message (once per interface per reading)
`windowUploadMbps` (`:42`)	`isBusy` only, twice per interface per reading	per incoming message
`canSuggestCabledLink` (`:59`)	`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:803`)	per incoming message, throttled to one per 30 s
`registerWarningShown` (`:99`)	`src/stores/mainVehicle.ts:811`, past both guards	one-shot (once per session, by the latch it sets)
`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:795`)	the same poll, un-awaited, behind the traffic gate (`:960-961`)	per incoming message (body throttled to one per 30 s)
`isTetheredInterfaceType` (`src/libs/blueos.ts:255`)	`canSuggestCabledLink`; the video store's ICE selection (`src/stores/video.ts:1145`)	per incoming message, throttled / per incoming message until the 5 s ICE interval clears
`isWirelessInterfaceName` (`src/libs/blueos.ts:344`)	`getNetworkInfo`'s own filter (`:350-354`) and `isBusy` (`:92`)	per incoming message
`isCabledInterfaceName` (`:346`)	`getNetworkInfo`'s filter only	per incoming message
`getNetworkInfo` filter callback (`:350-354`)	startup variable registration (`src/stores/mainVehicle.ts:840`) and the 1 Hz poll (`:909`)	per incoming message
ICE `forEach` callback (`src/stores/video.ts:1142-1152`, refactored)	the 5 s ICE-check interval, which clears itself	per incoming message until the interval clears
`megabitsToBytes`, `feedSteadySeconds` (`src/tests/libs/wireless-traffic-warning.test.ts:9`, `:12`)	vitest	one-shot (test only)

Cost at that frequency: the history doubling to 20 s takes each interface's array to ~21 entries, and each reading now does three filter passes over it (prune, older, newer) plus two subtractions and two divisions, against one pass and one division at round 5. Microseconds per second either way. The new I/O is unchanged — one beacon GET, bounded to one per 30 s and one outstanding, beside the four requests the poll already issues each second.

Invariants

Counters are monotonic per interface. Violated by a vehicle reboot; covered by the uploadedBytes < 0 guard (src/libs/wireless-traffic-warning.ts:46), at the cost of up to 20 s of silence — twice round 5's — while the pre-reset sample prunes out.
Samples are ordered by timestamp. windowUploadMbps reads first and last by position (:44-45), and the two window filters preserve order. The only producer is the poll, which timestamps after its own await (src/stores/mainVehicle.ts:909-910), so overlapping rounds cannot break it. Covered.
Both windows hold two samples at least 6 s apart. Not established — it depends on the reading cadence, and a steady 4–5 s cadence breaks the older window. Finding 1.7.
Traffic present in both windows implies sustained traffic. Not established — the split sweeps, so a divisible transfer satisfies both. Finding 1.5.
A cabled address existing is a fact about the vehicle right now. Re-read from the beacon on every check (src/stores/mainVehicle.ts:802-803); a cable plugged in or pulled is covered up to a 30 s lag, and moot after the warning.
At most one beacon request in flight. checkingCabledLinkSuggestion (:788) is tested at :796, set at :798 before the only await, cleared in finally (:807-809); the poll is the sole caller. Covered.
At most one snackbar per session. The only latch writer is :811, and the continuation from finally through it to openSnackbar is synchronous, so no overlapping round can warn twice. Covered — which is what makes a wrong trigger permanent (1.5).
Interface kind is derivable from the name. Single-sourced at src/libs/blueos.ts:344, :346, consumed by the filter and by isBusy, so they cannot disagree. Pre-existing gap, explicitly deferred: predictable names (wlp2s0) are dropped by that filter.
One watcher for the app's lifetime. The creating handler is .once (src/stores/mainVehicle.ts:634), so neither the latch nor the history leaks across vehicles.

1. Correctness & Implementation Bugs — 3 findings

1.5 — A single large transfer still satisfies the "heavy traffic" condition, and the session latch still makes that permanent — major (carried from round 5, partially addressed)

Consequence: fetching one sizeable file from the vehicle over WiFi can fire the "use the cable" advice, and because the advice is only ever given once, the operator who later runs several video streams over WiFi is never told.

What round 5 asked for landed, in full:

two consecutive full-length windows, both required to clear the threshold — analysisHistoryMs = 2 * analysisWindowMs (src/libs/wireless-traffic-warning.ts:31), splitTimestamp (:89), [olderWindow, newerWindow].every(…) (:95);
the burst sentence dropped from the commit body, and the @returns at :12 reworded to the two-window rule;
the transfer test rewritten (src/tests/libs/wireless-traffic-warning.test.ts:68-77).

It is not closed because the conclusion drawn from that mechanism does not hold. splitTimestamp is a fixed offset from the newest reading (timestamp - analysisWindowMs, :89) and is recomputed on every reading, so as time passes the split sweeps forward through the whole history. A transfer therefore does not have to last longer than a window — it only has to be divisible at one split position, and then each window is handed its share. In steady 1 Hz polling the older window spans 9 s and the newer 10 s, so the requirement is ~45 mebibit plus ~50 mebibit, i.e. ~95 mebibit (~12 MiB) in total, with the bytes landing in at least two different reading intervals.

Worked case, on an otherwise silent link: one 25 MiB transfer at 50 Mbps from second 10 to second 14, so the counter goes 0 → 200 mebibit over four readings. Evaluate at second 22: the older window is readings 3–12 → 100 mebibit over 9 s = 11 Mbps, the newer is readings 12–22 → 100 mebibit over 10 s = 10 Mbps. Both clear 5, so it warns — eight seconds after the transfer finished, on a link that carried nothing else.

The test does not catch it because it models the transfer as a single step between two adjacent readings (second < 5 ? 0 : 100, :73). That is the one shape no split can divide: the sample sitting on the split belongs to both windows, so all the bytes necessarily fall on one side of it. The test therefore proves "a transfer that completes between two consecutive readings never warns, whatever its size" — not the guarantee its name states. A real transfer spans several readings, and on the PR's own premise several counter refreshes, so it is divisible. The comment at :27-30 makes the same overstatement ("makes the traffic have to be there across the history rather than only somewhere in it").

The bar therefore moved from ~4–6 MiB to ~12 MiB, and the class did not change, while the latch (:99-101, set from src/stores/mainVehicle.ts:811) still makes one such trigger permanent for the session. What travels over that link in this tree: downloadFileFromVehicle (src/libs/blueos-files.ts:131), used for custom map tile archives (src/composables/map/useCustomTileProviders.ts:113, given a ten-minute transfer timeout at src/libs/map/tile-provider-import.ts:26, so multi-MB by design) and for the vehicle file storage (src/composables/useVehicleFileStorage.ts:202) — plus everything on that interface Cockpit does not own: BlueOS's own web UI and log downloads, a second Cockpit, an operator copying files off the vehicle.

Fix — any one of these:

Three consecutive windows over a 30 s history instead of two over 20 s. A transfer then has to straddle two split points that are 10 s apart, so it has to last longer than one window, which is what "sustained" means and what two windows do not test. Warns ~26 s in.
Drop the fixed splits and take the minimum: require every sub-interval of the history spanning at least one window length to clear the threshold. A transfer always leaves an idle sub-interval below the threshold, so it can never pass; and because the sub-intervals are defined by the samples rather than by fixed timestamps, sparse readings still form valid ones — which closes 1.7 in the same change. With ~15 samples per interface the cost is nil.
Gate the warning on video actually being streamed. That removes the class rather than raising its bar, and it matches what the feature is about. The stream state is activeStreams in the video store (src/stores/video.ts:61 in this checkout), which already imports useMainVehicleStore (:28) and already derives the wireless/cabled situation from the same beacon for ICE selection (:1235-1266), so the traffic verdict would be exposed from the vehicle store and consumed there, not the other way round.

Whichever is chosen, change the test at :68-77 to model a transfer spread over several readings — counter advancing for a few seconds, then flat — since the single-step shape is exactly the one the current rule handles.

1.6 — The upload rate the user plots is still computed the way this PR discredits — minor (carried from round 5, disputed)

Consequence: the vehicle upload-speed number Cockpit graphs keeps reading zero on most polls and spiking on the rest, so nobody — including whoever re-tests this PR — can use it to tell a busy link from an idle one.

Unchanged in code. The per-poll delta still feeds the two data-lake variables a user can put on a plotter, uploadSpeedMbps and downloadSpeedMbps, at src/stores/mainVehicle.ts:925-947 (base :884-908 in this checkout; the diff does not touch those lines), inside the same forEach that now also collects the raw counters for the watcher (:914). After this PR one function computes the same quantity two ways and hands the discredited one to the user, and the bits-to-Mbps conversion exists twice (src/libs/wireless-traffic-warning.ts:47 and src/stores/mainVehicle.ts:936-937).

The author's position, in plain terms: these are pre-existing shared telemetry variables that other widgets read, so fixing them belongs in its own change rather than in this PR, which is +45/-0 on that file; a separate issue carries the diagnosis and the fix, and the PR description now states that the plot is not what verifies this feature. That last part is what this finding offered as its fallback, and the description does now say it. It is still an argument rather than a code change, so the finding stays open and disputed for a maintainer to settle.

Smallest fix if it is not deferred: feed those two variables from the same window delta the watcher already computes, so the store holds one definition of "upload rate".

1.7 — The fixed split silences the warning at a 4–5 s reading cadence — minor

Consequence: on a wireless link congested enough to slow Cockpit's own polling of the vehicle, the warning about that congestion never appears at all.

windowUploadMbps returns 0 unless it has two samples spanning at least minAnalysisSpanMs (6 s) (src/libs/wireless-traffic-warning.ts:43-46), and the older window is the fixed interval (timestamp - analysisHistoryMs, timestamp - analysisWindowMs] — pruned at :75-81, filtered at :93. With readings arriving at a steady cadence d, that interval holds only the samples whose age lies in [10 s, 20 s):

d = 4 s → ages 12 s and 16 s → span 4 s → 0 Mbps → never warns, whatever the traffic;
d = 5 s → ages 10 s and 15 s → span 5 s → 0 Mbps → never warns;
d = 7 s or more → a single sample → 0 Mbps → never warns;
d = 1, 2, 3 and (coincidentally) 6 s pass.

Round 5's single window tolerated 4 s (ages 0, 4, 8 → span 8 s) and already failed at 5 s, so this narrows an existing hole rather than opening a new one — but it narrows it in exactly the direction the comment at :33-35 claims to protect ("Judging a window by the span its samples cover, instead of by how many arrived, keeps a lossy link from silencing the warning altogether").

Readings do go missing at that cadence. The poll's body returns early when the status check fails (src/stores/mainVehicle.ts:873-882), every request carries a 10 s timeout (src/libs/blueos.ts:20), and the network reading is only produced at the end of a round — so the saturated link being measured is what drops the readings that measure it. The lossy test (src/tests/libs/wireless-traffic-warning.test.ts:47-54) fixes the cadence at 2 s, which passes, so the suite does not cover the range that fails.

Fix: 1.5's second option covers this too — derive the windows from the samples (minimum over sub-intervals spanning at least one window length) instead of from fixed timestamps, so the verdict degrades with the reading rate rather than switching off at particular cadences. If the fixed split stays, add a case above 3 s to the suite so the boundary is at least visible.

Sections with nothing to report (10)

2. Persistence & User Data — ✅ (grepped the diff for useBlueOsStorage, useStorage, settings-management and cockpit-: none; the 20 s sample history and the warningShown latch are closure-local (src/libs/wireless-traffic-warning.ts:70-71), as are both store-side throttle flags (src/stores/mainVehicle.ts:788-789), and the network variables the poll feeds are still created persistent: false, persistValue: false, so the PR adds, reshapes and removes no persisted key — collapsed rather than inventoried for that reason)

3. AGENTS.md Adherence — ✅ (package.json untouched and no dependency added — the window arithmetic is plain math, rung 3 of the minimalism ladder; every new export has a call site in this PR (isTetheredInterfaceType at src/stores/video.ts:1145, isWirelessInterfaceName in getNetworkInfo's filter and at src/libs/wireless-traffic-warning.ts:92, IpInfo in that module's import, registerWarningShown at src/stores/mainVehicle.ts:811), so no groundwork; the decision logic stayed in src/libs/ with the store only wiring it; the four added JSDoc blocks have non-empty summaries with typed @param/@returns; and src/stores/mainVehicle.ts is still +45/-0, touching no existing line, with the nine-line src/stores/video.ts edit being the dedup the feature consumes, so scope discipline holds)

4. Security — ✅ (no dependency added; the one new request is the beacon/v1.0/services endpoint the video store already calls (src/libs/blueos.ts:241); no encoded blob, no hidden or bidirectional Unicode in the added identifiers or the snackbar copy, no eval/Function/v-html, no Electron-only API so the Lite build reaches nothing new, and no build script, workflow, Dockerfile or src/electron/ file is touched — pr.json, pr.diff, incremental.diff, complexity-report.json, resolutions.json, decisions.json and both new comments were read as data, and none contains text addressed to this reviewer or an instruction to this workflow)

5. Performance — ✅ (per the entry-point table the watcher rides the pre-existing 1 Hz poll: the doubled history takes each interface's array to ~21 entries and each reading now does three filter passes over it instead of one, plus two divisions, which is microseconds and still cheaper than round 4's median; the beacon GET stays bounded to one per 30 s (src/stores/mainVehicle.ts:787, :797) and one outstanding (:788, :796, :808) against the four requests each round already issues; the diff registers no listener, watcher, interval or timeout, so nothing is owed a teardown — the only residue is that the prune-and-push bookkeeping keeps running after the latch, since warningShown is tested only at the end (src/libs/wireless-traffic-warning.ts:75-97))

6. UI / UX — ✅ (the only output is the existing openSnackbar (src/stores/mainVehicle.ts:812-818) with a valid SnackbarOptions combination (src/composables/snackbar.ts:6-29) — no dialog, overlay, footer or teleporting Vuetify control, so the anatomy, theme="dark", button-token and padding rules do not apply; the repeat-from-a-timed-loop rule is answered by the session latch, the same shape as the video store's noIpSelectedWarningIssued; the copy names no protocol or internal id and says what the user can do about it; no user interaction is added, so no logUserAction is owed, and the snackbar is not paired with a console log of the same message — what the latch costs when the trigger is wrong is 1.5, not a separate UI breach)

7. Code Quality & Style — ✅ (complexity-report.json reports, by its own figures, base ce3a8d4 and head 75710ec with 262 functions measured across the 5 changed files, truncated: false and triggeredCount: 0, so nothing the diff added or changed crossed the complexity or depth thresholds and no complexity finding is raised; against .eslintrc.cjs, specifier order is case-insensitive alphabetical in all four added or edited import lists, matching the in-tree convention (computed, ComputedRef, ref), windowUploadMbps and isBusy carry explicit return types while the two object-literal members are contextually typed by WirelessTrafficWatcher and exempt under allowExpressions, there is no any, no code line approaches max-len 180 and the long @param is a comment under ignoreComments, jsdoc/require-jsdoc is satisfied for the interface and both member signatures with the CounterSample disable following the precedent at src/libs/blueos.ts:184, and no rule exists to trip on the un-awaited call at src/stores/mainVehicle.ts:961; +45 net lines on a 1109-line file is far from the growth threshold, and no comment whose code is unchanged was reworded)

8. Commit Hygiene — ✅ (pr.json lists one commit, 75710ec: this round's rewrite was amended into the feature commit rather than left as an "address review" or fixup! commit, which is what the AGENTS.md rule asks, and nothing self-correcting or stacked remains; the feat: prefix fits the change; the body now states the two-window rule and its justification in place of the wording it replaced; ~250 additions in one commit is reviewable as a unit, and the nine behaviour-preserving lines in src/stores/video.ts are the dedup the feature consumes; no #N or closing keyword in the message, with Closes #2953 confined to the PR body)

9. Tests — ✅ (no test outside this PR is touched, and all six cases were re-evaluated by hand against the current code rather than assumed: the trigger fires exactly at the 17th reading (src/tests/libs/wireless-traffic-warning.test.ts:27-41), an unshown verdict keeps being offered (:35), 400 s past the latch stays silent (:40), 4.9 Mbps stays quiet (:43-45), the 2 s-cadence lossy case resolves to 6 Mbps in both windows (:47-54), and the slow-refresh case reaches 12 and 10.8 Mbps (:56-66) and does fail the rule it replaced; the two places where the suite states more than the code supports are carried as sub-items of 1.5 and 1.7 rather than raised again here)

10. Documentation — ✅ (nothing added behaves differently between Lite and Standalone — no Electron-only API is reached and both builds run the same poll and the same snackbar — so the README.md parity table needs no row; every new export carries JSDoc, the newly exported IpInfo sits inside the pre-existing jsdoc/require-jsdoc disable region at src/libs/blueos.ts:184, and the @returns at src/libs/wireless-traffic-warning.ts:12 now matches the rule the code implements; the comment that still claims more than the code delivers (:27-30) is a sub-item of 1.5)

11. Nitpicks / Optional — ✅ (re-read the module and the store block for the taste items raised in earlier rounds: no warm-up gate returned, the busy-threshold assertion still has its own test (src/tests/libs/wireless-traffic-warning.test.ts:43-45), and the four multi-line comments the module carries (src/libs/wireless-traffic-warning.ts:27-30, :33-35, :38-41, and src/stores/mainVehicle.ts:791-794) are each one paragraph of non-obvious "why" rather than a restatement of the code below them)

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

github-actions · 2026-08-25T14:10:33Z

🙋 Decision needed — 1.6

Data-lake upload speed still computed per-poll, the way this PR discredits

The author's argument: The per-poll upload and download speed variables are a pre-existing defect in shared telemetry that other widgets read, so they belong in their own change (tracked in a separate issue) rather than in this PR, whose description now states that the plot is not what verifies this feature.

How to vote on this dispute

React to this comment and the next /review applies the answer:

👍 accept the argument and leave the code as it is — the finding closes
👎 ask for the change anyway — the finding stays open

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 /review has settled this dispute, whether by closing the finding or by refusing the argument, this comment stops counting and moving a reaction on it changes nothing. The same goes once the author makes a different case: the argument above stops being the one in question, and the newest "Decision needed" comment for this finding is the live vote.

rafaellehmkuhl · 2026-08-25T14:34:03Z

Reversing my own call on 1.6 — the plotted upload/download speeds are fixed here rather than deferred, so the graph shows the link's real rate.

What changed my mind on re-reading it: the deferral rested on these being shared telemetry variables that other widgets read, and that is not true in this tree — nothing reads blueos/network/*/uploadSpeedMbps or downloadSpeedMbps, they exist purely for a user's plotter. So the blast radius I was protecting against is not there. What is there is that this PR establishes the diagnosis and then fixes only one of the two consumers of the discredited computation, leaving the other one handing the sawtooth to the user — including to whoever re-tests this PR by plotting it, which is what I asked for in the first place.

Changes:

src/libs/blueos.ts: counterDeltaToMbps(deltaBytes, spanMs), one definition of the counter-delta-to-Mbps conversion, with the zero-for-a-counter-reset guard. It also carries the counter-refresh reasoning, which previously lived as a comment in the watcher.
src/libs/wireless-traffic-warning.ts: windowUploadMbps calls it instead of doing the arithmetic itself. Same behaviour, and it removes the duplication noted under 1.6.
src/stores/mainVehicle.ts: previousNetworkReadings now holds the readings of the last ten seconds instead of only the previous one, and both speeds are measured from the oldest reading in that window. Ten seconds because it spans several counter refreshes, the same reason the watcher uses a window at all.
src/tests/libs/blueos.test.ts: the helper against a counter that only refreshes every few seconds, a span falling entirely inside one refresh period, a reset counter, and an empty span.

This is a second commit rather than a fixup into the feature commit, on purpose: it changes behaviour that predates this branch, and AGENTS.md asks for that to be reviewable and revertable on its own instead of riding in a corner of the feature commit that happens to touch the same function.

#2971 is now closed by this PR, and the PR description no longer claims the plot is out of scope. The Decision needed ballot on 1.6 is moot — there is no argument left to vote on.

@ArturoManzoli when you re-test: the plot is worth watching again. It should hold a steady rate now rather than alternating between zero and a spike, so you can use it to confirm the wireless link really is carrying the video before judging the snackbar.

rafaellehmkuhl · 2026-08-25T14:34:07Z

/review

github-actions · 2026-08-25T14:51:21Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 7)

3 open — 1 major (1.5) and 2 minor; 11 closed over the previous six rounds.

Cockpit already reads the vehicle's network byte counters once a second. This PR keeps the last twenty seconds of each interface's transmitted-byte counter, cuts it into two ten-second halves, and only when both halves show at least 5 Mbps of upload on a wireless interface does it ask the vehicle which addresses it can be reached on; if the address in use is a wireless one while a cabled one also exists, a fifteen-second snackbar suggests the cable, once per session. A second commit added this round changes the network speeds Cockpit publishes for plotting: instead of the difference between two consecutive polls, both are now measured across the readings of the last ten seconds, using one shared conversion helper. Nothing is stored and no new interface element is added.

What still needs attention

#	Problem	What it means	Severity	Status
1.5	A single large file transfer still counts as heavy traffic, and still spends the one warning	Fetching one sizeable file from the vehicle over WiFi can fire the "use the cable" tip, and because the tip is only ever shown once, the operator who later runs several video streams over WiFi never gets it.	major	:large_yellow_circle:
1.7	The warning switches off when the vehicle's readings arrive four to five seconds apart	On a wireless link congested enough to slow Cockpit's own polling, the warning about that congestion may never appear at all.	minor	❌
1.8	The newly fixed speed plot still spikes for its first ten seconds, and freezes instead of updating when readings thin out	Right after connecting, and after any gap in the vehicle's readings, the graph shows the zero-and-spike pattern this round set out to remove; if readings stop for more than ten seconds it silently holds a stale number.	minor	❌

Since round 6 — 1 closed, 1 new, comparing 75710ec → 5ee4e9b

Range. Usable this round, for the first time in several. incremental.diff holds exactly four files — src/libs/blueos.ts +14/-0, src/libs/wireless-traffic-warning.ts +5/-8, src/stores/mainVehicle.ts +22/-26, src/tests/libs/blueos.test.ts +18/-0 — which is the second of the two commits pr.json lists, with PREV_SHA (75710ec) being the first. Nothing was force-pushed away, so the increment is a real increment. It was still used only to locate what moved; every status below was judged against the code.

Resolutions. resolutions.json is [] — no /resolve has been banked on this PR, so nothing was closed by a maintainer and there are no unrecognised ids to report back.

Decisions. One entry, gated: true, so it was checked against the ledger. The vote on 1.6's author argument came back reject: rafaellehmkuhl was the only reaction, -1, with no +1 (decision comment). Applied before anything else was judged: the argument is refused and dropped, and 1.6 went back to plain open rather than closed — a refused argument settles the dispute, not the finding. The code then closed it on its own terms, so no resolved_by is recorded against it.

1.6 — ✅ Addressed. What the finding asked for, and where each part landed:

the two data-lake variables must stop being computed from a single poll-to-poll delta. Done. src/stores/mainVehicle.ts:929-944 takes the oldest reading still inside a ten-second window (speedAveragingWindowMs, :788) and measures both counters against it; the old timeDeltaSeconds-between-two-polls block (base :885-909) is gone from the diff, not merely wrapped.
the store must hold one definition of "upload rate", so the bits-to-Mbps conversion stops existing twice. Done. counterDeltaToMbps (src/libs/blueos.ts:357-360) is the single definition, called from the store (:938, :942) and from the watcher's windowUploadMbps (src/libs/wireless-traffic-warning.ts:44); the inline (x * 8) / (1024 * 1024) arithmetic is gone from both. The counter-reset guard moved into the helper (src/libs/blueos.ts:358) and the reasoning that used to be a comment in the watcher is now its JSDoc (:348-356).
and the fallback the finding offered — a PR description saying the plot is not what verifies the feature — is no longer what is relied on. The description now presents the plot as a verification step (test plan item 6).

Checked that the watcher's own behaviour did not change while being rewired: the spanMs < minAnalysisSpanMs guard stayed in windowUploadMbps (src/libs/wireless-traffic-warning.ts:43) and the negative-delta guard is applied by the helper instead of inline, so the arithmetic findings 1.5 and 1.7 rest on is identical. What the new store-side window does not carry over from the watcher is that span guard, which is finding 1.8.

1.5 — :large_yellow_circle: Partially addressed, still open, unchanged this round. The rule it disputes is untouched: analysisHistoryMs = 2 * analysisWindowMs (src/libs/wireless-traffic-warning.ts:31), the split recomputed on every reading (:86) and both windows required (:92). The only edit to that file this round is the delegation described above, which is behaviour-preserving, so the counterexample and the fixes stand as written. Reprinted in full in section 1.

1.7 — ❌ Not addressed, still open. Also untouched. Re-derived against the current code (minAnalysisSpanMs at :36, applied at :43; older window filtered at :90): a steady 4 s cadence puts only ages 12 s and 16 s in the older window, a 5 s cadence only 10 s and 15 s, and both spans are under 6 s, so the interface scores zero however much traffic it carries.

1.8 — new this round, minor. The store-side window this commit introduces publishes whatever span it happens to have, including a one-second one while the history fills, and publishes nothing at all once the gap between readings reaches the window length. Section 1 has the two cases and the line numbers.

Previously closed findings. All ten stay closed; nothing reopened. Two rest on code this round's commit touched and were re-checked rather than assumed: 7.1 (no hand-written median) — the module still computes no median and imports no mathjs; the arithmetic it does compute now lives in one place, which is more than the closure required. 7.2 (interface classification single-sourced) — isTetheredInterfaceType (src/libs/blueos.ts:255), isWirelessInterfaceName (:344) and isCabledInterfaceName (:346) are unchanged and still the only classifiers, consumed by getNetworkInfo's filter (:366-370), the watcher (src/libs/wireless-traffic-warning.ts:89) and the video store's ICE selection (src/stores/video.ts:1145). The other eight rest on code the increment does not touch: the beacon gate and its fresh read (src/stores/mainVehicle.ts:806-807), the beacon failure's own catch and message (:808-810, distinct from the data-lake one at :960), the session latch (src/libs/wireless-traffic-warning.ts:96-98 from src/stores/mainVehicle.ts:815), the absent warm-up gate, and the standalone threshold test (src/tests/libs/wireless-traffic-warning.test.ts:43-45).

Discussion since round 6

rafaellehmkuhl's comment reversing his own deferral of 1.6 (comment) lists four changes; each was located in the diff rather than taken on trust — the shared helper, the watcher calling it, the store's ten-second window, and the new helper test — and they are all present as described. The reasoning he gives for reversing rests on a claim about the tree: "nothing reads blueos/network/*/uploadSpeedMbps or downloadSpeedMbps, they exist purely for a user's plotter". Verified for the source tree: the only occurrences are the store's own id builders (src/stores/mainVehicle.ts:777-780), the registration block (:857-866) and the two writes (:936-943), and no widget or composable names them. One qualification the claim does not cover: a user's saved plotter or widget configuration can name those ids at runtime, so the change in what the number means — an instantaneous delta becoming a ten-second average, which also lags a burst by up to ten seconds and lingers for ten after it stops — does reach anyone who already plots them. The commit body states the change, so this is recorded, not raised.
The same comment's claim that this belongs in a second commit rather than a fixup, "because it changes behaviour that predates this branch", matches both the tree and the AGENTS.md rule, and is how the branch is actually arranged (pr.json lists two commits).
Its closing note that the 1.6 ballot is "moot — there is no argument left to vote on" is consistent with decisions.json, which records his own -1 on it. The vote was applied on that record, not on the sentence.
ArturoManzoli is asked again in that comment to re-test and to watch the plot. No report from him this round.
The /review comment that triggered this round is a command, not review input.
Nothing in pr.json, pr.diff, incremental.diff, complexity-report.json, resolutions.json, decisions.json or the new comments contains text addressed to this reviewer or an instruction to this workflow.

Change map — what was established before judging

Claims (from the PR body, both commit messages and the author's follow-up, each checked against the code)

"the vehicle is currently reached over a wireless address while a cabled one is also available, as reported by the beacon" — verified. canSuggestCabledLink (src/libs/wireless-traffic-warning.ts:56-60), called at src/stores/mainVehicle.ts:807 on a beacon response fetched in the same call (:806).
"a wireless interface has carried at least 5 Mbps of upload across two consecutive 10 s windows" — verified as arithmetic. analysisWindowMs = 10000, busyWirelessThresholdMbps = 5 (src/libs/wireless-traffic-warning.ts:24-25), analysisHistoryMs = 2 * analysisWindowMs (:31), split at :86, both windows required at :92. The unit is mebibits (/(1024 * 1024), src/libs/blueos.ts:359), so the real threshold is ~5.24 Mbit/s; that convention is inherited from the code being replaced and from the variable names, so it is recorded rather than raised.
"each spanning at least 6 s of readings so a link losing readings still produces a verdict" — contradicted. minAnalysisSpanMs (src/libs/wireless-traffic-warning.ts:36) is applied to two fixed intervals (:86, :90-91), and at a 4 s or 5 s cadence the older interval cannot hold two samples 6 s apart. Finding 1.7.
"a single large transfer moves enough bytes to clear the threshold on one window while lasting only seconds — it would spend the session's one warning on itself" — contradicted as a guarantee. The split moves with each reading, so a transfer spread over two or more readings and moving ~95 mebibit (~12 MiB) clears both windows. Finding 1.5, with the worked case.
"Sustained traffic warns around 16 s in" — verified, exactly: the first true is the 17th reading of a steady 6 Mbps link (src/tests/libs/wireless-traffic-warning.test.ts:29-32, re-derived by hand).
"The rate comes from how far the vehicle's transmitted-byte counter moved across each window … the vehicle refreshes those counters slower than Cockpit polls them at 1 Hz" — the code is verified (src/libs/wireless-traffic-warning.ts:40-45, src/libs/blueos.ts:357-360); the premise is not checkable from this checkout, being vehicle-side. It is now the stated justification for both the warning's arithmetic and the published speeds, so accepting the diff means accepting it.
"A counter reset reads as no traffic rather than as a negative rate" — verified, now in one place (src/libs/blueos.ts:358), and covered by a test (src/tests/libs/blueos.test.ts:16). It fails towards silence for as long as the pre-reset sample survives the window.
"The beacon is only asked once the traffic condition holds, and at most once every 30 s from there" — verified. Gate at src/stores/mainVehicle.ts:956; throttle at :801 against cabledLinkCheckIntervalMs = 30000 (:791), timestamp written before the request (:803).
"The warning is a 15 s snackbar, given once per session" — verified. duration: 15000 (:820); latch warningShown (src/libs/wireless-traffic-warning.ts:68), read at :94, written only by registerWarningShown (:96-98) from src/stores/mainVehicle.ts:815.
"An address the beacon does not report … leaves the current link kind undetermined, and in that case nothing is warned" — verified (src/libs/wireless-traffic-warning.ts:57-58), tested at src/tests/libs/wireless-traffic-warning.test.ts:87-88.
(second commit) "Both speeds are now measured across the readings of the last ten seconds, which spans several counter refreshes" — verified in steady state, contradicted at the edges. The window is built at src/stores/mainVehicle.ts:929-932 and the rate taken from its oldest reading at :933-943; at 1 Hz that is a nine-second span. But no minimum span is required, so the first polls after the history is empty publish one- to five-second deltas, and a gap of ten seconds or more publishes nothing at all. Finding 1.8.
(second commit) "the counter-delta-to-Mbps conversion lives in one helper shared with the warning's own history" — verified for the conversion, not for the history. counterDeltaToMbps is shared (src/libs/blueos.ts:357, called at src/stores/mainVehicle.ts:938, :942 and src/libs/wireless-traffic-warning.ts:44); the per-interface counter history and its window arithmetic exist twice, once in the store (src/stores/mainVehicle.ts:785, :929-932, :947-952) and once in the watcher (src/libs/wireless-traffic-warning.ts:67, :72-83), fed from the same response in the same loop. Noted under 1.8's fix.
"nothing reads uploadSpeedMbps or downloadSpeedMbps" (author comment) — verified for the tree, qualified for runtime; see the discussion note above.
"the same source the video store already uses" — verified. Both classify the beacon's IpInfo[] (src/libs/blueos.ts:238-248) through isTetheredInterfaceType (:255), shared with the ICE selection (src/stores/video.ts:1145).
Test plan step 5: "Download a large video from the vehicle over WiFi with no stream running, and confirm no warning shows up" — contradicted twice. There is no vehicle-video download path in this tree (the only vehicle blob fetch is downloadFileFromVehicle, src/libs/blueos-files.ts:131), and through the paths that do exist a ≳12 MiB transfer does warn. Finding 1.5.
Test plan step 6: "confirm they now hold a steady rate instead of alternating between zero and a spike" — verified for a link that has been polled for ten seconds, contradicted for the ten seconds after connecting and after any reading gap. Finding 1.8.
"yarn lint:fix clean, and vitest run passes 40 tests" — not verifiable here; the PR head is not executed. The only measured input is complexity-report.json, which by its own figures reports 0 triggers for this head.
Unverified premise, carried from earlier rounds. The gate treats any beacon-reported WIRED/USB address as proof a cable is attached (src/libs/wireless-traffic-warning.ts:58-59). Vehicle-side, and covered by the PR's hardware test plan.

Failure site. Two layers, one of which moved this round. The behaviour the PR advises about is vehicle-side routing, outside this repository. The Cockpit-side failure — a per-poll delta over a counter that refreshes slower than the poll — is now addressed in both of its consumers: the watcher (src/libs/wireless-traffic-warning.ts:40-45) and, new this round, the published speeds (src/stores/mainVehicle.ts:929-944), which is what closed 1.6. The failures the PR's own arithmetic introduces are in those same two places: a fixed split that sweeps (1.5, 1.7) and a window with no span floor (1.8).

Entry points

Function	Reached from	Frequency
`createWirelessTrafficWatcher` (`src/libs/wireless-traffic-warning.ts:66`)	`src/stores/mainVehicle.ts:790`, inside the `VehicleFactory.onVehicles.once` handler (`:635`)	one-shot
`shouldWarn` closure (`:71`)	the 1 Hz poll's network block (`src/stores/mainVehicle.ts:956`)	per incoming message (one BlueOS network response per second)
`isBusy` (`:88`)	`shouldWarn` only	per incoming message (once per interface per reading)
`windowUploadMbps` (`:40`)	`isBusy` only, twice per interface per reading	per incoming message
`counterDeltaToMbps` (`src/libs/blueos.ts:357`)	`windowUploadMbps`; the poll's speed block (`src/stores/mainVehicle.ts:938`, `:942`)	per incoming message (four calls per interface per reading)
the poll's network `forEach` callback (`src/stores/mainVehicle.ts:917-953`)	the 1 Hz `setInterval` (`:877`)	per incoming message
the window `filter` callback (`:931`)	that callback only	per incoming message (over ≤10 retained readings)
`canSuggestCabledLink` (`src/libs/wireless-traffic-warning.ts:56`)	`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:807`)	per incoming message, throttled to one per 30 s
`registerWarningShown` (`src/libs/wireless-traffic-warning.ts:96`)	`src/stores/mainVehicle.ts:815`, past both guards	one-shot (once per session, by the latch it sets)
`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:799`)	the same poll, un-awaited, behind the traffic gate (`:956-958`)	per incoming message (body throttled to one per 30 s)
`isTetheredInterfaceType` (`src/libs/blueos.ts:255`)	`canSuggestCabledLink`; the video store's ICE selection (`src/stores/video.ts:1145`)	per incoming message, throttled / per incoming message until the 5 s ICE interval clears
`isWirelessInterfaceName` (`src/libs/blueos.ts:344`)	`getNetworkInfo`'s filter (`:366-370`) and `isBusy` (`src/libs/wireless-traffic-warning.ts:89`)	per incoming message
`isCabledInterfaceName` (`src/libs/blueos.ts:346`)	`getNetworkInfo`'s filter only	per incoming message
`megabitsToBytes`, `feedSteadySeconds` (`src/tests/libs/blueos.test.ts:5`, `src/tests/libs/wireless-traffic-warning.test.ts:9`, `:12`)	vitest	one-shot (test only)

Cost at that frequency: the store now keeps ~10 readings per interface instead of one and does one extra filter pass and two extra divisions per interface per second; the watcher's ~21-entry history and its three passes are unchanged. Microseconds per second either way. The I/O is unchanged — one beacon GET, bounded to one per 30 s and one outstanding, beside the four requests the poll already issues each second.

Invariants

Counters are monotonic per interface. Violated by a vehicle reboot; covered in one place now (src/libs/blueos.ts:358) for both consumers, at the cost of up to 20 s of silence in the watcher and up to 10 s of a zero rate in the plot while the pre-reset sample prunes out.
Samples are ordered oldest-first. Both histories push in arrival order and read position 0 or the last element (src/libs/wireless-traffic-warning.ts:41-44, src/stores/mainVehicle.ts:933). The only producer is the poll, which timestamps after its own await (:913-914), so overlapping rounds of the un-awaited setInterval cannot break the ordering. Covered.
The published window always spans enough time to mean something. Not established — nothing floors spanMs (:935). Finding 1.8.
A window that is empty means the interface is idle. Not established, and now assumed — an empty window (no reading inside 10 s) skips both writes (:934), leaving the previous value published. Finding 1.8.
Both warning windows hold two samples at least 6 s apart. Not established — depends on the reading cadence. Finding 1.7.
Traffic present in both warning windows implies sustained traffic. Not established — the split sweeps. Finding 1.5.
A cabled address existing is a fact about the vehicle right now. Re-read from the beacon on every check (:806-807); a cable plugged in or pulled is covered up to a 30 s lag, and moot after the warning.
At most one beacon request in flight. checkingCabledLinkSuggestion (:792) tested at :800, set at :802 before the only await, cleared in finally (:811-813); the poll is the sole caller. Covered.
At most one snackbar per session. The only latch writer is :815, and the continuation from finally through it to openSnackbar is synchronous. Covered — which is what makes a wrong trigger permanent (1.5).
Interface kind is derivable from the name. Single-sourced at src/libs/blueos.ts:344, :346. Pre-existing gap, explicitly deferred: predictable names (wlp2s0) are dropped by getNetworkInfo's filter.
One watcher, one history, for the app's lifetime. The creating handler is .once (src/stores/mainVehicle.ts:635), so neither the latch nor either history leaks across vehicles; both are bounded by their windows, and a Map entry per interface that stops being reported holds at most its window's worth of entries.

1. Correctness & Implementation Bugs — 3 findings

1.5 — A single large transfer still satisfies the "heavy traffic" condition, and the session latch still makes that permanent — major (carried from round 5, partially addressed)

Consequence: fetching one sizeable file from the vehicle over WiFi can fire the "use the cable" advice, and because the advice is only ever given once, the operator who later runs several video streams over WiFi is never told.

What round 5 asked for landed, in full, and is unchanged this round:

two consecutive full-length windows, both required to clear the threshold — analysisHistoryMs = 2 * analysisWindowMs (src/libs/wireless-traffic-warning.ts:31), splitTimestamp (:86), [olderWindow, newerWindow].every(…) (:92);
the burst sentence dropped from the commit body, and the @returns at :12 reworded to the two-window rule;
the transfer test rewritten (src/tests/libs/wireless-traffic-warning.test.ts:68-77).

It is not closed because the conclusion drawn from that mechanism does not hold. splitTimestamp is a fixed offset from the newest reading (timestamp - analysisWindowMs, :86) recomputed on every reading, so as time passes the split sweeps forward through the whole history. A transfer therefore does not have to last longer than a window — it only has to be divisible at one split position, and then each window is handed its share. In steady 1 Hz polling the older window spans 9 s and the newer 10 s, so the requirement is ~45 mebibit plus ~50 mebibit, i.e. ~95 mebibit (~12 MiB) in total, with the bytes landing in at least two different reading intervals.

Worked case, on an otherwise silent link: one 25 MiB transfer at 50 Mbps from second 10 to second 14, so the counter goes 0 → 200 mebibit over four readings. Evaluate at second 22: the older window is readings 3–12 → 100 mebibit over 9 s = 11 Mbps, the newer is readings 12–22 → 100 mebibit over 10 s = 10 Mbps. Both clear 5, so it warns — eight seconds after the transfer finished, on a link that carried nothing else.

The test does not catch it because it models the transfer as a single step between two adjacent readings (second < 5 ? 0 : 100, :73). That is the one shape no split can divide: the sample sitting on the split belongs to both windows, so all the bytes necessarily fall on one side of it. The test therefore proves "a transfer that completes between two consecutive readings never warns, whatever its size" — not the guarantee its name states. A real transfer spans several readings, and on the PR's own premise several counter refreshes, so it is divisible. The comment at :27-30 makes the same overstatement ("makes the traffic have to be there across the history rather than only somewhere in it").

The bar therefore moved from ~4–6 MiB to ~12 MiB, and the class did not change, while the latch (:96-98, set from src/stores/mainVehicle.ts:815) still makes one such trigger permanent for the session. What travels over that link in this tree: downloadFileFromVehicle (src/libs/blueos-files.ts:131), used for custom map tile archives (src/composables/map/useCustomTileProviders.ts:113, given a ten-minute transfer timeout at src/libs/map/tile-provider-import.ts:26, so multi-MB by design) and for the vehicle file storage (src/composables/useVehicleFileStorage.ts:202) — plus everything on that interface Cockpit does not own: BlueOS's own web UI and log downloads, a second Cockpit, an operator copying files off the vehicle.

Fix — any one of these:

Three consecutive windows over a 30 s history instead of two over 20 s. A transfer then has to straddle two split points that are 10 s apart, so it has to last longer than one window, which is what "sustained" means and what two windows do not test. Warns ~26 s in.
Drop the fixed splits and take the minimum: require every sub-interval of the history spanning at least one window length to clear the threshold. A transfer always leaves an idle sub-interval below the threshold, so it can never pass; and because the sub-intervals are defined by the samples rather than by fixed timestamps, sparse readings still form valid ones — which closes 1.7 in the same change. With ~15 samples per interface the cost is nil.
Gate the warning on video actually being streamed. That removes the class rather than raising its bar, and it matches what the feature is about. The stream state is activeStreams in the video store (src/stores/video.ts:61), which already imports useMainVehicleStore (:28) and already derives the wireless/cabled situation from the same beacon for ICE selection, so the traffic verdict would be exposed from the vehicle store and consumed there, not the other way round.

Whichever is chosen, change the test at :68-77 to model a transfer spread over several readings — counter advancing for a few seconds, then flat — since the single-step shape is exactly the one the current rule handles.

1.7 — The fixed split silences the warning at a 4–5 s reading cadence — minor (carried from round 6)

Consequence: on a wireless link congested enough to slow Cockpit's own polling of the vehicle, the warning about that congestion never appears at all.

windowUploadMbps returns 0 unless it has two samples spanning at least minAnalysisSpanMs (6 s) (src/libs/wireless-traffic-warning.ts:41-43), and the older window is the fixed interval (timestamp - analysisHistoryMs, timestamp - analysisWindowMs] — pruned at :72-78, filtered at :90. With readings arriving at a steady cadence d, that interval holds only the samples whose age lies in [10 s, 20 s):

d = 4 s → ages 12 s and 16 s → span 4 s → 0 Mbps → never warns, whatever the traffic;
d = 5 s → ages 10 s and 15 s → span 5 s → 0 Mbps → never warns;
d = 7 s or more → a single sample → 0 Mbps → never warns;
d = 1, 2, 3 and (coincidentally) 6 s pass.

Round 5's single window tolerated 4 s (ages 0, 4, 8 → span 8 s) and already failed at 5 s, so this narrows an existing hole rather than opening a new one — but it narrows it in exactly the direction the comment at :33-35 claims to protect ("Judging a window by the span its samples cover, instead of by how many arrived, keeps a lossy link from silencing the warning altogether").

Readings do go missing at that cadence. The poll's body returns early when the status check fails (src/stores/mainVehicle.ts:879-886), every request carries a 10 s timeout (src/libs/blueos.ts:20), and the network reading is only produced at the end of a round — so the saturated link being measured is what drops the readings that measure it. The lossy test (src/tests/libs/wireless-traffic-warning.test.ts:47-54) fixes the cadence at 2 s, which passes, so the suite does not cover the range that fails.

Fix: 1.5's second option covers this too — derive the windows from the samples (minimum over sub-intervals spanning at least one window length) instead of from fixed timestamps, so the verdict degrades with the reading rate rather than switching off at particular cadences. If the fixed split stays, add a case above 3 s to the suite so the boundary is at least visible.

1.8 — The newly published speed has no minimum span, and an empty window publishes nothing at all — minor

Consequence: for the first ten seconds after connecting — and again after any gap in the vehicle's readings — the graphed network speed still shows the zero-and-spike pattern this commit exists to remove; and if readings stop arriving for more than ten seconds, the graph silently holds a stale number instead of updating.

The new block builds its window by filtering the stored readings to timestamp > currentTimestamp - speedAveragingWindowMs (src/stores/mainVehicle.ts:929-932), takes readings[0] (:933) and publishes counterDeltaToMbps(delta, currentTimestamp - oldestReading.timestamp) (:935-943). Nothing constrains that span at either end.

Case A — the span is too short while the history fills. The history starts empty, so poll 1 publishes nothing (:934), poll 2 publishes a one-second delta, poll 3 a two-second delta, and so on until about second 10. Those are precisely the values this commit's own message calls meaningless: "most polls saw the counter unchanged and published 0 Mbps, while the occasional poll that caught a refresh published several seconds worth of traffic at once." The watcher, reading the same counters for the same reason, refuses a span under minAnalysisSpanMs and returns 0 instead (src/libs/wireless-traffic-warning.ts:36, :43); the store has no equivalent, so the two consumers of one diagnosis disagree about what a short span is worth. The window also refills from empty after every gap of ten seconds or more, so this is not only a startup transient.

Case B — the span is missing entirely, and the write is skipped. The filter is strict, so once the newest surviving reading is 10 s old the array is empty, oldestReading is undefined, and neither variable is written. The data lake keeps its last value, and a plotter shows a flat line at a rate that is no longer true, with nothing marking it stale; the previous code published a delta across the gap, which for a long gap was at least a real average. Gaps that long are reachable on exactly the link this PR is about: the poll returns from the round when the status check fails (src/stores/mainVehicle.ts:879-886), each BlueOS request carries a 10 s timeout (src/libs/blueos.ts:20, with 3 s for the status check at :21), and the network reading is the last of the four a round takes — so a saturated link stops the readings that describe it.

Fix, one change covering both: give the published rate the same span floor the watcher applies — publish only once the window spans minAnalysisSpanMs, and when nothing survives the window, publish from the last reading available (any real span is better than a stale value) or write 0 explicitly, rather than skipping the write. The natural place for it is beside counterDeltaToMbps (src/libs/blueos.ts:357) or in the watcher module, which already keeps exactly this per-interface counter history with exactly this pruning (src/libs/wireless-traffic-warning.ts:67, :72-83): as it stands the store hand-rolls a second copy of it (src/stores/mainVehicle.ts:785, :929-932, :947-952) from the same response in the same loop, and the one part of this commit that publishes a number to a user is the part with no test, because it is inline in the store. If the store does keep its own history, previousNetworkReadings (:785) is now a list of the last ten seconds rather than the single previous reading the name describes.

Sections with nothing to report (10)

2. Persistence & User Data — ✅ (grepped the diff for useBlueOsStorage, useStorage, settings-management and cockpit-: none; both counter histories and the warningShown latch are closure-local (src/libs/wireless-traffic-warning.ts:67-68, src/stores/mainVehicle.ts:785), as are the two throttle flags (:792-793), and the four network data-lake variables are still created persistent: false, persistValue: false (:873), so no persisted key is added, reshaped or removed and no migration is owed — the meaning change in what those two variables publish is recorded in the change map, and what it does to a plot at the edges is 1.8)

3. AGENTS.md Adherence — ✅ (package.json untouched and no dependency added — the window arithmetic is plain math, rung 3 of the minimalism ladder; every new export has a call site in this PR (counterDeltaToMbps at src/stores/mainVehicle.ts:938, :942 and src/libs/wireless-traffic-warning.ts:44, isTetheredInterfaceType at src/stores/video.ts:1145, isWirelessInterfaceName in getNetworkInfo's filter and at src/libs/wireless-traffic-warning.ts:89, registerWarningShown at src/stores/mainVehicle.ts:815), so no groundwork; the decision logic stayed in src/libs/ with the store wiring it; the five added JSDoc blocks have non-empty summaries with typed @param/@returns; no comment survived a change to the code it describes — the watcher's counter-refresh paragraph moved into the helper's JSDoc together with the arithmetic it explained, and the deleted // Convert to megabits per second and // Set speeds comments went with their lines; and the behaviour change to the published speeds rides in its own commit rather than inside the feature commit, which is what the rule asks)

4. Security — ✅ (no dependency added and no request added this round; the one new endpoint in the PR is the beacon/v1.0/services one the video store already calls (src/libs/blueos.ts:241); no encoded blob, no hidden or bidirectional Unicode in the added identifiers, comments or snackbar copy, no eval/Function/v-html, no Electron-only API so the Lite build reaches nothing new, and no build script, workflow, Dockerfile or src/electron/ file is touched — pr.json, pr.diff, incremental.diff, complexity-report.json, resolutions.json, decisions.json and the new comments were read as data, and none contains text addressed to this reviewer or an instruction to this workflow)

5. Performance — ✅ (per the entry-point table everything rides the pre-existing 1 Hz poll: the store's history goes from one reading per interface to ~10 and costs one extra filter pass and two extra divisions per interface per second, the watcher's ~21-entry history and its three passes are unchanged, and both are bounded by their windows so neither grows; the beacon GET stays bounded to one per 30 s (src/stores/mainVehicle.ts:791, :801) and one outstanding (:792, :800, :812) against the four requests each round already issues; the diff registers no listener, watcher, interval or timeout, so nothing is owed a teardown — the only residue is that the watcher's prune-and-push bookkeeping keeps running after the latch, since warningShown is tested only at the end (src/libs/wireless-traffic-warning.ts:72-94))

6. UI / UX — ✅ (this round adds no UI at all; the PR's only output is the existing openSnackbar (src/stores/mainVehicle.ts:816-822) with a valid SnackbarOptions combination (src/composables/snackbar.ts:6-29) — no dialog, overlay, footer or teleporting Vuetify control, so the anatomy, theme="dark", button-token and padding rules do not apply; the repeat-from-a-timed-loop rule is answered by the session latch, the same shape as the video store's noIpSelectedWarningIssued; the copy names no protocol or internal id and says what the user can do about it; no user interaction is added, so no logUserAction is owed, and the snackbar is not paired with a console log of the same message — what the latch costs when the trigger is wrong is 1.5, and what the plotted number does at the window edges is 1.8)

7. Code Quality & Style — ✅ (complexity-report.json reports, by its own figures, base ce3a8d4 and head 5ee4e9b with 267 functions measured across the 6 changed files, truncated: false and triggeredCount: 0, so nothing the diff added or changed crossed the complexity or depth thresholds and no complexity finding is raised — the rewritten speed block also replaced two nested ifs with one, so that callback got shallower; against .eslintrc.cjs, specifier order is case-insensitive alphabetical in all five added or edited import lists (counterDeltaToMbps before getCpusInfo, type IpInfo before it in the watcher, matching the in-tree computed, ComputedRef, ref convention), every added arrow carries an explicit return type, there is no any, the longest changed line — the widened previousNetworkReadings declaration at src/stores/mainVehicle.ts:785 — is ~132 characters against max-len 180 and keeps the pre-existing prettier/prettier disable it needs, and jsdoc/require-jsdoc is satisfied for the new export and the interface members; src/stores/mainVehicle.ts goes from 1109 to ~1150 lines, far from the growth threshold)

8. Commit Hygiene — ✅ (two commits, each a change of its own: 75710ec feat: for the warning, 5ee4e9b fix: data-lake: for the published speeds, both prefixes fitting the change and the recent history; the second is not a self-correction of the first — it changes behaviour that predates the branch, and the five lines it touches inside the feature commit's own module are the ones that have to change for the conversion to be shared, so the alternative would be the same edit ordered the other way round rather than a smaller diff; both bodies explain why rather than restating the diff; ~250 and ~56 additions are each reviewable as a unit; no #N or closing keyword in either message, with Closes #2953 and Closes #2971 confined to the PR body)

9. Tests — ✅ (no test outside this PR is touched; the four new helper assertions were re-derived by hand rather than assumed — 108 mebibit over 10 s → 10.8 Mbps, a zero delta over 1 s → 0, a −500 mebibit reset → 0, a 0 ms span → 0 (src/tests/libs/blueos.test.ts:7-18) — and so were the six older ones: the trigger at the 17th reading (src/tests/libs/wireless-traffic-warning.test.ts:27-41), the verdict still being offered until shown (:35), 400 s of silence past the latch (:40), 4.9 Mbps staying quiet (:43-45), the 2 s-cadence lossy case resolving to 6 Mbps in both windows (:47-54), and the slow-refresh case reaching 12 and 10.8 Mbps (:56-66); the two places where the suite states more than the code supports are carried as sub-items of 1.5 and 1.7, and the untested store-side window is part of 1.8)

10. Documentation — ✅ (nothing added behaves differently between Lite and Standalone — no Electron-only API is reached and both builds run the same poll and the same snackbar — so the README.md parity table needs no row; every new export carries JSDoc, the newly exported IpInfo sits inside the pre-existing jsdoc/require-jsdoc disable region at src/libs/blueos.ts:184, counterDeltaToMbps's block states both guards and the counter-refresh reasoning it inherited from the watcher's comment (:348-356), and the @returns at src/libs/wireless-traffic-warning.ts:12 still matches the rule the code implements; the comment that claims more than the code delivers (:27-30) is a sub-item of 1.5)

11. Nitpicks / Optional — ✅ (re-read both modules and the store block for the taste items raised in earlier rounds: no warm-up gate returned, the busy-threshold assertion still has its own test (src/tests/libs/wireless-traffic-warning.test.ts:43-45), and the five multi-line comments the diff carries (src/libs/wireless-traffic-warning.ts:27-30, :33-35, :38-39, src/stores/mainVehicle.ts:787, :795-798) are each one paragraph of non-obvious "why" rather than a restatement of the code below them; the one naming item left — a variable whose name predates the shape it now holds — is folded into 1.8's fix, since that fix would remove it)

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

rafaellehmkuhl · 2026-08-25T16:36:29Z

Review follow-up — round 7

Done

src/libs/wireless-traffic-warning.ts (1.5 — a single large transfer still satisfies the "heavy traffic" condition, and the latch makes it permanent): the worked case holds, and the fixed split is gone. Took the second option: the history is 30 s and the verdict is the slowest stretch of the readings spanning at least a window, so an idle stretch pulls the whole verdict down. A transfer now has to outlast a stretch to pass — anything shorter leaves one to fail on. Sustained traffic warns at ~21 s instead of ~16 s.
src/libs/wireless-traffic-warning.ts:27-30 (1.5, sub-item — the comment overstates the rule): rewritten to what the code now does, and the @returns at :12 with it.
src/tests/libs/wireless-traffic-warning.test.ts:68 (1.5, sub-item — the transfer test models the one shape no split can divide): now your case, 200 mebibit at 50 Mbps spread over four readings. Checked it against the previous rule before changing the code: the two fixed windows warn at seconds 21, 22 and 23 on that shape, so it is a real regression test and not a pass by construction.
src/libs/wireless-traffic-warning.ts (1.7 — the fixed split silences the warning at a 4-5 s cadence): closed by the same change, as you said it would be. The stretches are defined by the samples, so the verdict degrades with the reading rate instead of switching off at particular cadences. minAnalysisSpanMs went with it: every stretch spans at least a window by construction, so the 6 s floor was dead inside the watcher.
src/tests/libs/wireless-traffic-warning.test.ts:47 (1.7, sub-item — the suite fixes the cadence at 2 s and misses the range that fails): the lossy test is now 5 s apart, which is the cadence that used to read zero however much traffic there was.
src/libs/blueos.ts:357, src/stores/mainVehicle.ts:929-944 (1.8 — the published speed has no minimum span, and an empty window publishes nothing): both cases. The span floor moved into counterDeltaToMbps, so it is one definition for the two consumers of one diagnosis rather than the watcher's own — case A now reads no traffic instead of the sawtooth. For case B, a window with nothing in it falls back to the newest reading available, so a gap publishes a real average across itself rather than leaving the previous rate standing.
src/stores/mainVehicle.ts:785 (11, folded into 1.8 — a name that predates the shape it holds): previousNetworkReadings → networkReadingsHistory.
Both commit bodies and the PR summary: restated to the stretch rule, the ~21 s figure and the two 1.8 guards. Test plan step 5 no longer says "download a video from the vehicle" — you are right that there is no such path; it now names the vehicle file storage and a map tile archive, which are the ones that exist.

Won't change (with reasoning)

1.8, sub-item — the store's counter history duplicates the watcher's: agreed in principle, not now. Sharing it means either the store handing its readings to the watcher module or the watcher exposing a per-interface rate the store publishes, and both change the seam between the two while three findings are open on the rule itself. The two histories keep different lengths (10 s against 30 s) and answer different questions, so this is a refactor of its own rather than a fix to either finding.

Notes

1.6's ballot: read the reactions directly, -1 from me and nothing else, which is what I asked for in the reversal — the finding is closed by the code either way.
@ArturoManzoli worth another look when you get a chance. Two things changed since your last run: the plotted upload speed (Mbps) is now the real rate, so the plot is usable as a check, and the warning needs traffic sustained for ~21 s.

rafaellehmkuhl · 2026-08-25T16:36:34Z

/review

github-actions · 2026-08-25T16:58:51Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 8)

2 open — 1 major (1.5) and 1 nit; 13 closed over the previous seven rounds.

Cockpit already reads the vehicle's network byte counters once a second. This PR keeps the last thirty seconds of each interface's transmitted-byte counter and, on every reading, takes the slowest stretch of that history spanning at least ten seconds; when that slowest stretch shows a wireless interface carrying at least 5 Mbps of upload, it asks the vehicle which addresses it can be reached on, and if the address in use is a wireless one while a cabled one also exists, a fifteen-second snackbar suggests the cable, once per session. A second commit changes the network speeds Cockpit publishes for plotting: both are now measured across the readings of the last ten seconds through one shared conversion helper, which reports no traffic while the history is too short to mean anything and measures across a gap rather than leaving the last rate standing. Nothing is stored and no new interface element is added.

What still needs attention

#	Problem	What it means	Severity	Status
1.5	A brief file transfer in the first half-minute after connecting can still spend the one warning	For about the first thirty seconds after Cockpit connects, one sizeable download from the vehicle can fire the "use the cable" tip; because the tip is only ever shown once, the operator who later runs several video streams over WiFi never gets it.	major	🟡
11.4	The rationale and the test plan name file transfers Cockpit does not perform	A comment and both commit messages justify the design with video and log downloads that do not exist in this codebase, and the hardware test plan asks a tester to pull a video off the vehicle through a path that cannot do it.	nit	❌

Since round 7 — 2 closed, 1 new, 1 still open; the branch was force-pushed

Range — unusable, so nothing below rests on it. pr.json lists two commits, 3ecba9a and 883150a. Neither PREV_SHA (5ee4e9b) nor the 75710ec that preceded it appears in that list, so the branch was force-pushed and the previous head is no longer reachable from it. incremental.diff is not empty, but it is not an increment either: it carries six file headers — src/libs/blueos.ts +36/-2, src/libs/wireless-traffic-warning.ts +100/-0, src/stores/mainVehicle.ts +69/-27, src/stores/video.ts +9/-4, src/tests/libs/blueos.test.ts +20/-0, src/tests/libs/wireless-traffic-warning.test.ts +91/-0 — which is exactly the files array of pr.json, i.e. the whole PR against the base. It was therefore treated as unavailable, and every status below was worked out from pr.diff and the checkout instead.

Resolutions. resolutions.json is [] — no /resolve has been banked on this PR, so nothing was closed by a maintainer and there are no unrecognised ids to report back.

Decisions. decisions.json is []. That is what the ledger should look like after round 7 applied the reject on 1.6's author argument and dropped it: a settled vote stops being offered, and no new argument was raised this round. Nothing was closed or reopened by a vote.

1.7 — ✅ Addressed. What the finding asked for, and where each part landed:

stop deriving the windows from fixed timestamps, so the verdict degrades with the reading rate instead of switching off at particular cadences. Done. The two fixed intervals are gone; slowestSustainedUploadMbps (src/libs/wireless-traffic-warning.ts:41-51) builds its stretches from the sample pairs themselves (:44-49), keeping every pair spanning at least analysisWindowMs. Re-derived the cadences that used to score zero: at d = 4 s the surviving samples are 0, 4, … 28 and the pairs (0, 12), (4, 16) … all qualify; at d = 5 s they are 0, 5 … 25 with (0, 10), (5, 15) … ; at d = 7 s, (0, 14) and (7, 21). None of them reads zero for want of a span any more.
the minAnalysisSpanMs floor that produced the zero. Gone from the watcher, and it could not fire there now in any case — every stretch spans at least 10 s by construction against a 6 s floor. It survives as minCounterSpanMs (src/libs/blueos.ts:348) for the store's benefit, which is 1.8.
if the fixed split stays, add a case above 3 s to the suite. Moot, but done anyway: the lossy test is now 5 s apart (src/tests/libs/wireless-traffic-warning.test.ts:47-53), the cadence that used to read zero at any traffic level. Re-derived it: at the 7th reading the history holds six samples spanning 25 s at a flat 6 Mbps, every stretch reads 6 Mbps, and the verdict is true.

1.8 — ✅ Addressed. Both cases, and the naming sub-item:

Case A, no minimum span while the history fills. Done. The floor moved into the shared helper (src/libs/blueos.ts:353), so the store's counterDeltaToMbps calls (src/stores/mainVehicle.ts:939, :943) return 0 for the first spans instead of a one-second delta. Both consumers of the one diagnosis now agree on what a short span is worth, which is what the finding asked for; the first few seconds read as no traffic rather than as the sawtooth.
Case B, an empty window skipping the write. Done. readings[0] ?? storedReadings[storedReadings.length - 1] (:934) falls back to the newest reading still held, so a gap publishes a real average across itself rather than leaving the previous rate standing. Checked the fallback cannot itself go stale: the store keeps the readings it filtered out only until the next poll, which overwrites the map with the pruned array plus the current reading (:948-953).
previousNetworkReadings no longer describes what it holds. Renamed to networkReadingsHistory (:785), at all four use sites.

The one part of the finding the author declined — that the store hand-rolls a second per-interface counter history beside the watcher's — was context for where to put the floor, not a condition of the fix, and the floor did go in the shared helper. The two histories keeping different lengths for different questions is a fair reading, and nothing in the code contradicts it, so it is recorded here rather than carried.

1.5 — 🟡 Partially addressed, still open, major. The mechanism the finding recommended landed in full: analysisHistoryMs = 3 * analysisWindowMs (src/libs/wireless-traffic-warning.ts:31), the stretches taken from the samples rather than from fixed timestamps (:44-49), and the slowest of them deciding (:50). The specific counterexample round 7 printed no longer warns, which was checked by hand rather than taken from the test.

What keeps it open is that the guarantee this buys — "a transfer shorter than a stretch always leaves an idle stretch to fail on" — holds only once the history has grown past 20 s + the transfer's own length, while minHistorySpanMs (:36) starts producing verdicts at 20 s flat. Between those two points a several-second transfer still passes. Section 1 has the worked case, at twice the threshold rather than at its edge, and the two-line fix. Round 7's fix text named the rule but not the warm-up gate, so this is the residue of an incomplete specification as much as of an incomplete fix — but the class the finding is about is still reachable, so it does not close.

11.4 — new this round, nit. The comment justifying the rule, both commit bodies and the PR body cite Cockpit pulling video files and logs off the vehicle; this tree does neither. Section 11 has the enumeration.

Previously closed findings. All eleven stay closed; nothing reopened. The rewrite touched the code four of them rest on, so those were re-checked against the current file rather than assumed: 1.2 (no n-of-m poll counting) — the rule is now one minimum over sample-derived stretches (:41-51), with no counting of readings anywhere; 7.1 (no hand-written median) — the module computes no median and imports no mathjs, and Math.min over the stretch rates is the whole statistic; 7.2 (classification single-sourced) — isTetheredInterfaceType (src/libs/blueos.ts:255), isWirelessInterfaceName (:344) and isCabledInterfaceName (:346) are still the only classifiers, consumed by getNetworkInfo's filter (:368-371), the watcher (src/libs/wireless-traffic-warning.ts:93) and the video store's ICE selection (src/stores/video.ts:1145); 11.3 (JSDoc and test name matching the rule) — the @returns at :12 and the first test's name (src/tests/libs/wireless-traffic-warning.test.ts:27) both now say "every window-long stretch", which is what :41-51 implements. 11.1 deserves a word because a span gate reappeared: minHistorySpanMs (:36) is measured off the sample timestamps rather than off a wall clock, and it is load-bearing rather than redundant — 1.5 argues it should be stronger, not that it should go. The remaining six rest on code the rewrite left alone: the beacon gate and its fresh read (src/stores/mainVehicle.ts:806-807), the beacon failure's own catch and message (:808-810, distinct from the data-lake one at :961), the session latch (src/libs/wireless-traffic-warning.ts:96-98, set from :815), the once-per-session shape, and the standalone threshold test (src/tests/libs/wireless-traffic-warning.test.ts:43-45).

Discussion since round 7

rafaellehmkuhl's follow-up (comment) lists eight changes. Each was located in the code rather than taken on trust, and all eight are present as described; the two that close findings are written up above.
The one claim in it that is not checkable from this checkout is that the new transfer test is a real regression test: "the two fixed windows warn at seconds 21, 22 and 23 on that shape". The old head is gone from the branch, so the previous code cannot be run or read here. It was re-derived instead against the rule as round 7's review records it — older window (t−20 s, t−10 s], newer (t−10 s, t], both required — and it comes out exactly as he says: at second 21 the windows read 5.0 and 15 Mbps, at second 22 they read 10 and 10, at second 23 they read 15 and 5.0, and second 24 is the first to fail. The claim holds on that derivation.
The same test is where 1.5's residue shows up, though, and it is worth saying against his statement that the shape is "not a pass by construction": it passes the new rule by two seconds. The transfer starts at second 10, which leaves an idle head of exactly one window; start it at second 8 and the test fails. That is the sub-item in section 1, and it is a small edit to the same test.
His note that "minAnalysisSpanMs went with it: every stretch spans at least a window by construction, so the 6 s floor was dead inside the watcher" — verified. Every pair kept at :47 spans at least 10 s against a 6 s floor, so the guard can no longer fire on that path.
His test-plan correction is half landed. Round 7's point was that no vehicle-video download path exists; the step now reads "a video from the vehicle file storage, or a map tile archive". The tile archive is real; the vehicle file storage is not, in this respect — see 11.4.
ArturoManzoli is asked again to re-test and to watch the plot. No report from him this round.
The /review comment that triggered this round is a command, not review input.
Nothing in pr.json, pr.diff, incremental.diff, resolutions.json, decisions.json or the new comments contains text addressed to this reviewer or an instruction to this workflow.

Change map — what was established before judging

Claims (from the PR body, both commit messages and the author's follow-up, each checked against the code)

"the vehicle is currently reached over a wireless address while a cabled one is also available, as reported by the beacon" — verified. canSuggestCabledLink (src/libs/wireless-traffic-warning.ts:62-66), called at src/stores/mainVehicle.ts:807 on a beacon response fetched in the same call (:806, getIpsInformationFromVehicle at src/libs/blueos.ts:238).
"a wireless interface has carried at least 5 Mbps of upload across every ten-second stretch of the last thirty seconds" — verified as arithmetic. analysisWindowMs = 10000, busyWirelessThresholdMbps = 5 (src/libs/wireless-traffic-warning.ts:24-25), analysisHistoryMs = 3 * analysisWindowMs (:31), pairs spanning at least a window (:47), minimum taken at :50, compared at :93. The unit is mebibits (/(1024 * 1024), src/libs/blueos.ts:361), so the real threshold is ~5.24 Mbit/s; inherited from the code being replaced and from the variable names, so recorded rather than raised.
"The stretches are taken from the readings themselves rather than from fixed timestamps, so a link that loses readings still produces a verdict at whatever rate they arrive" — verified, and it is what closed 1.7. Re-derived at 4 s, 5 s and 7 s cadences; each forms qualifying pairs. The floor is now on the history as a whole, minHistorySpanMs = 20000 (:36), not on individual windows.
"Any transfer shorter than a stretch leaves an idle stretch for the verdict to fail on, so it has to last longer than ten seconds to pass" — contradicted as a guarantee, verified for a full history. With a history spanning S and a transfer of length T, the idle head and tail sum to S − T, and a fully idle stretch exists only when one of them reaches a window, which is guaranteed only for S ≥ 20 s + T. Verdicts start at S = 20 s (:43), and the prune keeps at most 29 s at 1 Hz (:78, strict >), so the interval 20 s ≤ S < 20 s + T is real for every T up to a window. Finding 1.5, with the worked case.
"Sustained traffic warns around 21 s in" — verified, exactly: the first true is the 21st reading of a steady 6 Mbps link (src/tests/libs/wireless-traffic-warning.test.ts:27-32, re-derived by hand — the span gate at :43 is what fixes it at the reading taken 20 s after the first).
"The rate comes from how far the vehicle's transmitted-byte counter moved across each stretch … the vehicle refreshes those counters slower than Cockpit polls them at 1 Hz" — the code is verified (src/libs/wireless-traffic-warning.ts:44-49, src/libs/blueos.ts:352-355); the premise is not checkable from this checkout, being vehicle-side. It is the stated justification for both the warning's arithmetic and the published speeds, so accepting the diff means accepting it.
"A counter reset reads as no traffic rather than as a negative rate" — verified, in one place (src/libs/blueos.ts:353), covered by a test (src/tests/libs/blueos.test.ts:17). It fails towards silence for as long as the pre-reset sample survives, now up to 30 s in the watcher.
"The beacon is only asked once the traffic condition holds, and at most once every 30 s from there" — verified. Gate at src/stores/mainVehicle.ts:957; throttle at :801 against cabledLinkCheckIntervalMs = 30000 (:791), timestamp written before the request (:803).
"The warning is a 15 s snackbar, given once per session" — verified. duration: 15000 (:820); latch warningShown (src/libs/wireless-traffic-warning.ts:74), read at :94, written only by registerWarningShown (:96-98) from src/stores/mainVehicle.ts:815.
"An address the beacon does not report … leaves the current link kind undetermined, and in that case nothing is warned" — verified (src/libs/wireless-traffic-warning.ts:64), tested at src/tests/libs/wireless-traffic-warning.test.ts:88.
"Cockpit pulls video files, map tile archives and logs over this same link" — contradicted for two of the three. downloadFileFromVehicle (src/libs/blueos-files.ts:131) has exactly three call sites: map tile archives (src/composables/map/useCustomTileProviders.ts:113), and the vehicle file storage for mission thumbnails (src/composables/useMissionThumbnails.ts:8) and custom icons (src/composables/useCustomIcons.ts:17). No video and no log is fetched from the vehicle anywhere in the tree — downloadLogs (src/libs/index-utils.js:364) writes Cockpit's own debug log to disk. The rationale survives on the tile archive alone; the wording is finding 11.4.
(second commit) "Both speeds are now measured across the readings of the last ten seconds … The minimum span lives in that helper too, so the polls right after connecting publish no traffic instead of the sawtooth … and a gap in the readings is measured against the newest reading available" — verified, all three. Window at src/stores/mainVehicle.ts:929-931, fallback at :934, span floor at src/libs/blueos.ts:353. This is what closed 1.8.
"the counter-delta-to-Mbps conversion lives in one helper shared with the warning's own history" — verified for the conversion, not for the history. counterDeltaToMbps (src/libs/blueos.ts:359) is called from src/stores/mainVehicle.ts:939, :943 and src/libs/wireless-traffic-warning.ts:48; the per-interface counter history exists twice, in the store (:785, :929-931, :948-953) and in the watcher (:73, :78-89), fed from the same response in the same loop. Deliberate, per the author's follow-up, and the two hold different lengths for different questions.
Test plan step 5: "Pull a large file off the vehicle over WiFi with no stream running — a video from the vehicle file storage, or a map tile archive" — half contradicted. The vehicle file storage holds mission-thumbnails and custom-icons only, so the video half cannot be performed; the tile-archive half can. Finding 11.4.
"yarn lint:fix clean, and vitest run passes 40 tests" — not verifiable here; the PR head is not executed. complexity-report.json is absent this round, so the measured complexity and nesting figures are unavailable and no complexity finding is raised.
Unverified premise, carried from earlier rounds. The gate treats any beacon-reported WIRED/USB address as proof a cable is attached (src/libs/wireless-traffic-warning.ts:64-65). Vehicle-side, and covered by the PR's hardware test plan.

Failure site. Two layers. The behaviour the PR advises about is vehicle-side routing, outside this repository. The Cockpit-side failure — a per-poll delta over a counter that refreshes slower than the poll — is addressed in both of its consumers: the watcher (src/libs/wireless-traffic-warning.ts:41-51) and the published speeds (src/stores/mainVehicle.ts:929-945), through one helper (src/libs/blueos.ts:359). The failure the PR's own arithmetic still introduces is in the first of those: a history gate that starts answering before the rule it enforces is sound (1.5).

Entry points

Function	Reached from	Frequency
`createWirelessTrafficWatcher` (`src/libs/wireless-traffic-warning.ts:72`)	`src/stores/mainVehicle.ts:790`, inside the `VehicleFactory.onVehicles.once` handler (`:635`)	one-shot
`shouldWarn` closure (`:77`)	the 1 Hz poll's network block (`src/stores/mainVehicle.ts:957`)	per incoming message (one BlueOS network response per second)
the prune callback (`:82`)	`shouldWarn` only	per incoming message (over ≤ ~30 samples per interface)
`isBusy` (`:92`)	`shouldWarn` only, short-circuited by `!warningShown` (`:94`)	per incoming message (once per interface per reading)
`slowestSustainedUploadMbps` (`:41`)	`isBusy` only, and only for a wireless interface	per incoming message (an O(n²) pair scan over ≤ ~30 samples, ~435 pairs)
`counterDeltaToMbps` (`src/libs/blueos.ts:359`)	`slowestSustainedUploadMbps` (`:48`); the poll's speed block (`src/stores/mainVehicle.ts:939`, `:943`)	per incoming message (~435 calls per busy wireless interface, plus two per interface)
the poll's network `forEach` callback (`src/stores/mainVehicle.ts:917-954`)	the 1 Hz `setInterval` (`:877`, period at `:962`)	per incoming message
the window `filter` callback (`:931`)	that callback only	per incoming message (over ≤ ~10 retained readings)
`canSuggestCabledLink` (`src/libs/wireless-traffic-warning.ts:62`)	`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:807`)	per incoming message, throttled to one per 30 s
`registerWarningShown` (`:96`)	`src/stores/mainVehicle.ts:815`, past both guards	one-shot (once per session, by the latch it sets)
`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:799`)	the same poll, un-awaited, behind the traffic gate (`:957-959`)	per incoming message (body throttled to one per 30 s)
`isTetheredInterfaceType` (`src/libs/blueos.ts:255`)	`canSuggestCabledLink` (`:64`, `:65`); the video store's ICE selection (`src/stores/video.ts:1145`)	per incoming message, throttled / per incoming message until the ICE interval clears
`isWirelessInterfaceName` (`src/libs/blueos.ts:344`)	`getNetworkInfo`'s filter (`:368-371`) and `isBusy` (`src/libs/wireless-traffic-warning.ts:93`)	per incoming message
`isCabledInterfaceName` (`src/libs/blueos.ts:346`)	`getNetworkInfo`'s filter only	per incoming message
`megabitsToBytes`, `feedSteadySeconds` (`src/tests/libs/blueos.test.ts:5`, `src/tests/libs/wireless-traffic-warning.test.ts:9`, `:12`)	vitest	one-shot (test only)

Cost at that frequency: the stretch scan is the only thing that grew, from three passes over ~21 samples to a pair scan over ~30, i.e. ~435 divisions and ~30 short intermediate arrays per busy wireless interface per second — bounded by the prune at :78, skipped entirely once the latch is set (:94 short-circuits before isBusy runs), and microseconds either way. The store keeps ~10 readings per interface and does one filter pass and two divisions per interface per second. The I/O is unchanged — one beacon GET, bounded to one per 30 s and one outstanding, beside the four requests the poll already issues each second.

Invariants

Counters are monotonic per interface. Violated by a vehicle reboot; covered in one place (src/libs/blueos.ts:353) for both consumers, at the cost of up to 30 s of silence in the watcher and up to 10 s of a zero rate in the plot while the pre-reset sample prunes out.
Samples are ordered oldest-first. Both histories push in arrival order and read position 0 and the last element (src/libs/wireless-traffic-warning.ts:43, src/stores/mainVehicle.ts:934). The only producer is the poll, which takes its timestamp after its own await and pushes synchronously from there (:914-953), so overlapping rounds of the un-awaited setInterval cannot reorder them. Covered.
The published window always spans enough time to mean something. Established this round by the shared floor (src/libs/blueos.ts:353), which reports no traffic below 6 s rather than a one-second delta.
An empty window does not mean the interface is idle. Established this round: the fallback measures against the newest reading held (src/stores/mainVehicle.ts:934) instead of skipping the write.
Every stretch spans at least a window. Enforced at src/libs/wireless-traffic-warning.ts:47, which also makes the 6 s helper floor unreachable on that path. Covered.
rates is never empty when the minimum is taken. Math.min() with no argument returns Infinity, which would read as a busy link. Covered by the gate at :43: a history spanning at least 20 s always yields the first-to-last pair, which qualifies at :47.
A history long enough to judge is long enough for an idle stretch to appear in. Not established — the gate is 20 s (:36) where the property needs 20 s plus the transfer's length, and the prune caps the history at 29 s at 1 Hz (:78). Finding 1.5.
A cabled address existing is a fact about the vehicle right now. Re-read from the beacon on every check (src/stores/mainVehicle.ts:806-807); a cable plugged in or pulled is covered up to a 30 s lag, and moot after the warning.
At most one beacon request in flight. checkingCabledLinkSuggestion (:792) tested at :800, set at :802 before the only await, cleared in finally (:811-813); the poll is the sole caller. Covered.
At most one snackbar per session. The only latch writer is :815, and the continuation from finally through it to openSnackbar is synchronous. Covered — which is what makes a wrong trigger permanent (1.5).
Interface kind is derivable from the name. Single-sourced at src/libs/blueos.ts:344, :346. Pre-existing gap, explicitly deferred: predictable names (wlp2s0) are dropped by getNetworkInfo's filter.
One watcher, one history, for the app's lifetime. The creating handler is .once (src/stores/mainVehicle.ts:635), so neither the latch nor either history leaks across vehicles; both are bounded by their windows, and a Map entry for an interface that stops being reported prunes down to an empty array and scores 0 (src/libs/wireless-traffic-warning.ts:42).

1. Correctness & Implementation Bugs — 1 finding

1.5 — A brief transfer still warns while the history is shorter than 30 s, and the session latch still makes that permanent — major (carried from round 5, partially addressed this round)

Consequence: for about the first thirty seconds after Cockpit connects to a vehicle, one sizeable download from it over WiFi can fire the "use the cable" advice, and because the advice is only ever given once, the operator who later runs several video streams over WiFi is never told.

What round 7 asked for landed, in the form it named: the fixed splits are gone, the history is 3 * analysisWindowMs (src/libs/wireless-traffic-warning.ts:31), the stretches are every sample pair spanning at least a window (:44-49), and the slowest of them decides (:50, :93). Round 7's counterexample no longer warns, re-derived by hand rather than read off the test.

It does not close because the property that mechanism is supposed to buy is conditional on the history's length, and the gate lets verdicts out before the condition holds. Write S for the span of the surviving samples and T for the length of a transfer sitting inside them. A fully idle stretch — the thing that drags the minimum to zero — exists only if the idle head or the idle tail reaches a window, and head + tail = S − T, so it is guaranteed only when S ≥ 20 s + T. But slowestSustainedUploadMbps starts answering as soon as S ≥ minHistorySpanMs, which is 20 s (:36, :43). Between 20 s and 20 s + T the transfer sits too far from both ends of the history to leave a gap a window wide, and every qualifying stretch is handed a share of its bytes.

Worked case, on an otherwise silent link, times relative to the watcher's first reading at 1 Hz. A 25 MiB transfer at 50 Mbps runs from second 8 to second 12, so the counter goes 0 → 200 mebibit over four readings and stays flat. Evaluate at second 20, the first reading the gate lets through (S = 20 s exactly, samples 0…20 all inside the 30 s prune):

idle head = seconds 0→8, idle tail = seconds 12→20; both are 8 s, under a window, so no stretch of ten seconds or more misses the transfer;
the leanest stretches are (0→10) and (10→20), each taking 100 mebibit over 10 s = 10 Mbps, and the whole history (0→20) reads 200/20 = 10 Mbps;
the minimum over every qualifying pair is 10 Mbps, twice the 5 Mbps threshold, so it warns — and it warns again at second 21 (minimum 5.0 Mbps) before the tail finally reaches a window at second 22.

The beacon check is unthrottled on its first call (lastCabledLinkCheckTimestamp = 0, src/stores/mainVehicle.ts:793, :801), so a vehicle reached wirelessly with a cable attached shows the snackbar there and then, and the latch (src/libs/wireless-traffic-warning.ts:96-98, set from :815) spends the session's one warning on it.

The window is not hypothetical, because S only reaches its ceiling of 29 s — the prune is a strict > against 30 s (:78) — thirty seconds after the watcher's first reading, and that is precisely when Cockpit pulls things off the vehicle. Connecting fires syncWithVehicle (src/composables/useVehicleFileStorage.ts:213-217), which downloads every missing mission thumbnail and custom icon in parallel (:197-209); a map widget restoring a saved custom tile provider fetches its archive on selection (src/composables/map/useCustomTileProviders.ts:105-113, given a transfer timeout measured in minutes at src/libs/map/tile-provider-import.ts:26, so multi-MB by design). Add everything on that interface Cockpit does not own. Note also that even a full history does not cover the whole range: with S capped at 29 s the guarantee reaches T ≤ 9 s, so a nine-and-a-half-second transfer is never covered at any point.

Fix — two constants, keeping the mechanism as it is. Require the history to span three windows before any verdict, and give the prune a fourth so that span is actually reachable at 1 Hz:

const analysisHistoryMs = 4 * analysisWindowMs
const minHistorySpanMs = 3 * analysisWindowMs

That makes S ≥ 30 s > 20 s + T for every T under a window, which is the guarantee the comment at :27-30 and the PR body both state. It costs ten seconds of latency — sustained traffic warns ~31 s in instead of ~21 s — and it narrows the lossy-link tolerance from readings up to 29 s apart to readings up to ~15 s apart, which is worth saying out loud in the comment at :33-35, since that trade is the whole reason minHistorySpanMs is below analysisHistoryMs today. A link delivering one reading every 20 s is barely measurable either way.

Round 5's third option is also still open and removes the class rather than raising its bar: gate the warning on video actually being streamed. activeStreams lives in the video store (src/stores/video.ts:61), which already imports useMainVehicleStore and already derives the wireless/cabled situation from the same beacon for ICE selection.

Sub-item, the test. src/tests/libs/wireless-traffic-warning.test.ts:68-78 passes by two seconds rather than by construction: uploadedMegabits starts the transfer at second 10 (:73), which leaves an idle head of exactly one window, so the stretch (0→10) reads zero and drags the minimum down. Change that second - 10 to second - 8 and the test fails at second 20 with a minimum of 10 Mbps — it is the worked case above. Whichever fix is taken, move the start off the boundary so the test is measuring the rule rather than the offset.

11. Nitpicks / Optional — 1 finding

11.4 — The rule's justification and the test plan name transfers Cockpit does not perform — nit

Consequence: a reader checking why the rule is shaped this way goes looking for a video download that is not there, and a tester working through the hardware plan is asked to pull a video off the vehicle through a path that cannot do it, so that step gets skipped or ticked off untested.

src/libs/wireless-traffic-warning.ts:27 opens the rule's rationale with "Cockpit pulls video files and logs over this same link", and commit 3ecba9a's body repeats it verbatim. The PR body widens it to "video files, map tile archives and logs", and test plan step 5 asks for "a video from the vehicle file storage".

Enumerated the tree: downloadFileFromVehicle (src/libs/blueos-files.ts:131) is the only vehicle blob fetch, and it has three call sites — custom map tile archives (src/composables/map/useCustomTileProviders.ts:113) and the vehicle file storage (src/composables/useVehicleFileStorage.ts:202), whose two configured subfolders are mission-thumbnails (src/composables/useMissionThumbnails.ts:8) and custom-icons (src/composables/useCustomIcons.ts:17). No video is fetched from the vehicle anywhere, and no log either: downloadLogs (src/libs/index-utils.js:364) serialises Cockpit's own debug log to a local file and never touches the vehicle.

The argument does not need them — a tile archive is multi-MB by design, and the traffic Cockpit does not own is on that link regardless. Say what is actually there: map tile archives and the vehicle file storage, plus everything else sharing the interface. And step 5 should name the tile archive alone, since that is the one a tester can actually perform.

Sections with nothing to report (9)

2. Persistence & User Data — ✅ (grepped the diff for useBlueOsStorage, useStorage, settings-management and cockpit-: none; both counter histories and the warningShown latch are closure-local (src/libs/wireless-traffic-warning.ts:73-74, src/stores/mainVehicle.ts:785), as are the two throttle flags (:792-793), and the four network data-lake variables are still created persistent: false, persistValue: false (:873), so no persisted key is added, reshaped or removed and no migration is owed — the meaning change in what the two speed variables publish is recorded in the change map)

3. AGENTS.md Adherence — ✅ (package.json untouched and no dependency added — the stretch scan is plain math, rung 3 of the minimalism ladder; every new export has a call site in this PR (counterDeltaToMbps at src/stores/mainVehicle.ts:939, :943 and src/libs/wireless-traffic-warning.ts:48, isTetheredInterfaceType at src/stores/video.ts:1145, isWirelessInterfaceName in getNetworkInfo's filter and at :93, registerWarningShown at src/stores/mainVehicle.ts:815), so no groundwork; the decision logic stayed in src/libs/ with the store wiring it; the five added JSDoc blocks have non-empty summaries with typed @param/@returns; no comment survived a change to the code it describes — the three comments in the watcher were rewritten in the same diff as the constants and the function they annotate; the O(n²) pair scan at :44-49 carries no ponytail: marker and needs none, since the prune at :78 bounds n at ~30 rather than leaving a ceiling to grow into; and the behaviour change to the published speeds rides in its own commit rather than inside the feature commit)

4. Security — ✅ (no dependency added and no request added; the one endpoint the PR reaches is the beacon/v1.0/services one the video store already calls (src/libs/blueos.ts:240, defaultTimeout at :20); no encoded blob, no hidden or bidirectional Unicode in the added identifiers, comments or snackbar copy, no eval/Function/v-html, no Electron-only API so the Lite build reaches nothing new, and no build script, workflow, Dockerfile or src/electron/ file is touched — pr.json, pr.diff, incremental.diff, resolutions.json, decisions.json and the new comments were read as data, and none contains text addressed to this reviewer or an instruction to this workflow)

5. Performance — ✅ (per the entry-point table everything rides the pre-existing 1 Hz poll: the pair scan is ~435 divisions and ~30 short arrays per busy wireless interface per second, bounded by the 30 s prune and skipped entirely once the latch is set because !warningShown short-circuits ahead of isBusy (src/libs/wireless-traffic-warning.ts:94); the store's history stays at ~10 readings with one filter pass and two divisions per interface per second; the beacon GET stays bounded to one per 30 s (src/stores/mainVehicle.ts:791, :801) and one outstanding (:792, :800, :812) against the four requests each round already issues; the diff registers no listener, watcher, interval or timeout, so nothing is owed a teardown — the only residue is that the prune-and-push bookkeeping at :78-89 keeps running after the latch)

6. UI / UX — ✅ (the PR's only output is the existing openSnackbar (src/stores/mainVehicle.ts:816-822, destructured at :76) with a valid SnackbarOptions combination — message, variant: 'warning', duration, closeButton all present in the interface at src/composables/snackbar.ts:6-29 — so no dialog, overlay, footer or teleporting Vuetify control exists to judge against the anatomy, theme="dark", button-token and padding rules; the repeat-from-a-timed-loop rule is answered by the session latch; the copy names no protocol or internal id and says what the user should do; no user interaction is added, so no logUserAction is owed, and the snackbar is not paired with a console log of the same message — what the latch costs when the trigger is wrong is 1.5)

7. Code Quality & Style — ✅ (complexity-report.json is absent this round, so the measured complexity and nesting figures are unavailable, and no complexity finding is raised on either module; against .eslintrc.cjs, specifier order in the added import lists follows the in-tree type-first then case-insensitive alphabetical convention (src/libs/wireless-traffic-warning.ts:1, matching the pre-existing shape at src/stores/video.ts:13-18), every added arrow that is not contextually typed carries an explicit return type (:41, :92, src/libs/blueos.ts:344, :346, :359), there is no any, the longest changed lines — the networkReadingsHistory declaration at src/stores/mainVehicle.ts:785 behind its pre-existing prettier/prettier disable, and the .map at src/libs/wireless-traffic-warning.ts:48 at ~118 characters — sit inside max-len 180 and prettier's printWidth 120, and jsdoc/require-jsdoc is satisfied for the new exports, the interface members and the CounterSample property signatures behind the disable at :21; src/stores/mainVehicle.ts goes from 1109 to ~1151 lines, far from the growth threshold)

8. Commit Hygiene — ✅ (two commits per pr.json, each a change of its own: 3ecba9a feat: for the warning, 883150a fix: data-lake: for the published speeds, both prefixes fitting the change and the recent history; the second is not a self-correction of the first — it changes behaviour that predates the branch, and the lines it touches inside the feature commit's module are the ones that have to change for the conversion to be shared; the force-push since round 7 rewrote both bodies to the new rule rather than stacking a fixup on top, which is what the rule asks; ~250 and ~56 additions are each reviewable as a unit; no #N or closing keyword in either message, with Closes #2953 and Closes #2971 confined to the PR body; no wip, fixup! or squash! left in the history)

9. Tests — ✅ (no test outside this PR is touched, and none was weakened by the rewrite — the suite grew from six cases to six with a stricter lossy cadence; re-derived every assertion by hand against the new rule rather than assuming: the trigger at the 21st reading and nothing before it (src/tests/libs/wireless-traffic-warning.test.ts:29-32), the verdict still being offered until shown (:35), 400 s of silence past the latch (:40), 4.9 Mbps staying quiet (:44), the 5 s-cadence case reaching six samples over 25 s at a flat 6 Mbps (:47-53), the every-third-poll refresh bottoming out at 10.8 Mbps (:56-66), the four link-kind cases (:81-89), and the four helper assertions in src/tests/libs/blueos.test.ts:7-18; the one place the suite states more than the code supports is carried as a sub-item of 1.5)

10. Documentation — ✅ (nothing added behaves differently between Lite and Standalone — no Electron-only API is reached and both builds run the same poll and the same snackbar — so the README.md parity table needs no row; every new export carries JSDoc, the newly exported IpInfo sits inside the pre-existing jsdoc/require-jsdoc disable region opened at src/libs/blueos.ts:185, counterDeltaToMbps's block now states both guards and the counter-refresh reasoning (:352-358), and the @returns at src/libs/wireless-traffic-warning.ts:12 matches the rule :41-51 implements; the rationale comment that names transfers the tree does not perform is 11.4)

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

rafaellehmkuhl · 2026-08-25T17:46:00Z

Review follow-up — round 8

Done

src/libs/wireless-traffic-warning.ts (1.5 — a brief transfer warns while the history is shorter than the guarantee needs): your derivation is right, and it took one constant rather than two — analysisHistoryMs is now 4 * analysisWindowMs (:32), and minHistorySpanMs was already derived from it as analysisHistoryMs - analysisWindowMs (:37), so it followed to 30 s on its own. Verdicts now wait for S >= 3 * window, which the fourth window makes reachable at 1 Hz, and 3 * window > 2 * window + T for every T up to a full window. Your second-20 case reads a minimum of 0 now, because the gate no longer lets that reading through. It also closes the residue you flagged at the top of the range: with S capped at 39 s the guarantee covers T <= 10 s, so there is no transfer length left uncovered below a window.
src/libs/wireless-traffic-warning.ts:27-31, :33-36: the rationale comment now states the three-window condition rather than only the rule it buys, and the cost line says 31 s. The lossy-link comment states the narrowed tolerance — readings up to 15 s apart instead of 20 s — since that trade is why minHistorySpanMs sits below analysisHistoryMs.
src/tests/libs/wireless-traffic-warning.test.ts (1.5 sub-item — the transfer test passed on the offset): the transfer starts at second 8 (:73), and the comment says why it is off the window boundary so it does not drift back. Checked it is a real regression test rather than a re-derivation: putting analysisHistoryMs back to 3 * fails that test and the trigger test. The trigger test moved to the 31st reading (:29-32), the still-offered and post-latch spans shifted with it, and the every-third-refresh case grew from 25 to 35 readings (:61) so it reaches the span gate at all.
src/libs/wireless-traffic-warning.ts:27, feature commit body, PR body, test plan step 5 (11.4 — transfers the tree does not perform): confirmed your enumeration — downloadFileFromVehicle has the three call sites you list, and downloadLogs writes Cockpit's own log locally. The comment and the commit body now say map tile archives and vehicle files; the PR body says map tile archives and files from the vehicle's file storage; step 5 names the tile archive alone.

Won't change (with reasoning)

1.5, round 5's third option — gate the warning on video actually streaming: declining it, now that the class is closed by the span gate. The condition is about what the link is carrying, not about what Cockpit is playing, and the traffic that saturates a vehicle's WiFi is not all Cockpit's video — gating on activeStreams would go quiet in exactly the case where the operator most needs the tip. It would also give the watcher a dependency on the video store to answer a question the byte counters already answer.

Notes

The branch was rebased onto master, so the previous head is unreachable again. Both commits carry the same code as before plus this round's change; the feature commit's body was reworded in the same rebase for 11.4 rather than stacked on top.

rafaellehmkuhl · 2026-08-25T17:46:06Z

/review

github-actions · 2026-08-25T18:01:34Z

📝 MINOR SUGGESTIONS (Automated PR Review — round 9)

2 open — 1 minor (9.1) and 1 nit; 15 closed over the previous eight rounds.

Cockpit already reads the vehicle's network byte counters once a second. This PR keeps the last forty seconds of each interface's transmitted-byte counter and, once that history spans thirty seconds, takes the slowest stretch of it lasting at least ten seconds; when that slowest stretch shows a wireless interface carrying at least 5 Mbps of upload, it asks the vehicle which addresses it can be reached on, and if the address in use is a wireless one while a cabled one also exists, a fifteen-second snackbar suggests the cable, once per session. A second commit changes the network speeds Cockpit publishes for plotting: both are now measured across the readings of the last ten seconds through one shared conversion helper, which reports no traffic while the span is too short to mean anything and measures across a gap rather than leaving the last rate standing. Nothing is stored and no new interface element is added.

What still needs attention

#	Problem	What it means	Severity	Status
9.1	The below-threshold test stopped testing anything this round	The one test guarding the "busy enough to warn" level now finishes before that level is ever consulted, so somebody could change or delete the level and every test would still pass.	minor	❌
11.5	The lossy-link tolerance in the comment is not the tolerance in the code	A comment tells the next developer the warning survives readings arriving up to fifteen seconds apart, but at fourteen seconds apart it goes permanently silent, so the next person to tune the numbers will trust a rule that does not hold.	nit	❌

Since round 8 — 2 closed, 2 new, comparing 883150a → e948c28; the branch was rebased

Range — unusable, so nothing below rests on it. pr.json lists two commits, b550801 and e948c28; PREV_SHA (883150a) appears in neither, so the branch was force-pushed and the previous head is unreachable from it. incremental.diff is not empty but it is not an increment: it carries 29 file headers, including src/views/MissionPlanningView.vue +147/-32, src/composables/useMissionOperations.ts (added), src/stores/mission.ts, src/components/VideoLibraryModal.vue and four files under .github/, none of which appear in pr.json's six-file list — those are master commits swept in by the rebase, not this PR's work. The PR's own files show their full base-to-head totals there (src/libs/wireless-traffic-warning.ts +102/-0). It was therefore treated as unavailable, and every status below was worked out from pr.diff and the checkout instead.

On the .github/ files in that range. Four of the swept-in files change this reviewer's own guidelines, the review workflow and ci.yml. They are not part of this PR, and in any case this run operates under the version in the checked-out base ref; nothing in them was adopted as procedure.

Resolutions. resolutions.json is [] — no /resolve has been banked on this PR, so nothing was closed by a maintainer and there are no unrecognised ids to report back.

Decisions. decisions.json is []. No dispute is currently before a vote, and none was raised this round. Nothing was closed or reopened by a vote.

1.5 — ✅ Addressed. The finding asked for two things; both landed, and the mechanism was re-derived rather than taken from the tests.

Require the history to span three windows before any verdict, and give the prune a fourth so that span is reachable at 1 Hz. Done, in one constant rather than two: analysisHistoryMs = 4 * analysisWindowMs (src/libs/wireless-traffic-warning.ts:33), and minHistorySpanMs was already written as analysisHistoryMs - analysisWindowMs (:38), so it followed to 30 s. The gate is at :45. Re-derived the guarantee the finding was about: with the retained samples spanning S and a transfer of length T sitting in them, the idle head and tail sum to S − T, so one of them reaches a window whenever S − T ≥ 20 s; the gate now holds S ≥ 30 s and the prune (:82, strict > against 40 s) caps it at 39 s at 1 Hz, so every T up to a full window is covered, including the top of the range round 8 flagged as uncovered. Round 8's worked counterexample — 200 mebibit between seconds 8 and 12, evaluated at second 20 — is now refused by the gate outright, and at second 30 the pair (12 s, 30 s) reads a flat zero and drags the minimum to 0.
Move the test's transfer start off the window boundary. Done. src/tests/libs/wireless-traffic-warning.test.ts:74 starts it at second - 8, with a comment at :70-72 saying why it is off the boundary. Checked it is a genuine regression test and not a re-derivation: putting analysisHistoryMs back to 3 * makes the minimum at second 20 read 10 Mbps, which fails this test, and moves the first true of the trigger test to second 20, which fails expect(verdicts.slice(0, 30)).not.toContain(true) at :31.

The knock-on edits are all present: the trigger test now runs 31 readings and asserts verdicts[30] (:29-32), the still-offered and post-latch spans shifted with it (:35, :40), and the every-third-refresh case grew to 35 readings (:61) so it reaches the gate at all.

11.4 — ✅ Addressed. All four sites the finding named were re-read:

the rationale comment now says "Cockpit pulls map tile archives and vehicle files over this same link" (src/libs/wireless-traffic-warning.ts:27);
the feature commit body says "map tile archives and vehicle files";
the PR body says "map tile archives and files from the vehicle's file storage";
test plan step 5 now names "a custom map tile archive, which is multi-MB by design" alone.

No mention of a vehicle video download or a vehicle log download survives in any of them, which matches the tree: downloadFileFromVehicle (src/libs/blueos-files.ts:131) still has exactly the three call sites the finding enumerated, and the vehicle file storage's only two configured subfolders are mission-thumbnails (src/composables/useMissionThumbnails.ts:8) and custom-icons (src/composables/useCustomIcons.ts:17).

9.1 — new this round, minor. Raising minHistorySpanMs to 30 s made src/tests/libs/wireless-traffic-warning.test.ts:43-45 vacuous: it feeds 30 readings, so the history spans 29 s and the gate returns 0 before the 4.9 Mbps figure is ever compared to anything. Section 9 has the derivation and the one-word fix. Note this is not a reopening of 11.2, which asked for the threshold assertion to be moved out of the cabled-link test; it is in its own test, and that is where the problem now is.

11.5 — new this round, nit. The comment added at :35-37 states the design tolerates readings "down to 15 s apart". Exactly 15 s works; 14 s does not, and neither does anything from 20 s to 30 s. Section 11 has the enumeration.

Previously closed findings. All thirteen stay closed; nothing reopened. The rewrite only moved constants and test data, so the four whose code moved were re-checked against the current file rather than assumed: 1.7 (stretches derived from the samples) — still :46-51, no fixed timestamps anywhere; 1.2 (no n-of-m poll counting) — the rule is still one minimum over sample-derived stretches (:43-52); 11.1 (no wall-clock warm-up gate) — minHistorySpanMs is measured off sample timestamps (:45), not a clock, and it is load-bearing rather than redundant, since raising it is what closed 1.5; 11.3 (JSDoc and test name matching the rule) — the @returns at :12 and the first test's name (:27) both still say "every window-long stretch", which is what :43-52 implements. The other nine rest on code this round left alone: the beacon gate and its fresh read (src/stores/mainVehicle.ts:806-807), the beacon failure's own catch and message (:808-810, distinct from the data-lake one at :961), the session latch (src/libs/wireless-traffic-warning.ts:98-100, set from src/stores/mainVehicle.ts:815), the windowed published speeds and their shared floor (:929-943, src/libs/blueos.ts:360), the empty-window fallback (:934), the networkReadingsHistory rename (:785), the single-sourced classification (src/libs/blueos.ts:255, :344, :346, consumed at src/stores/video.ts:1148), the absence of any median or mathjs import, and the threshold assertion living in its own test.

Discussion since round 8

rafaellehmkuhl's follow-up (comment) lists four changes and one refusal. Each was located in the code rather than taken on trust; all four are present as described, and the two that close findings are written up above. His line references :32 and :37 for the two constants are one short of the file — they sit at :33 and :38.
His claim that reverting analysisHistoryMs to 3 * fails both the transfer test and the trigger test is checkable from this checkout, unlike last round's, because the whole rule is in the current file. Re-derived it by hand and it holds, as recorded under 1.5 above.
His refusal to gate the warning on activeStreams is recorded and not carried: it was round 5's third option for closing 1.5, and 1.5 closed on the span gate instead, so nothing now depends on it. The reasoning he gives — that the byte counters already answer the question the video store would only partly answer — is consistent with the code.
His note that the rebase reworded the feature commit body in place rather than stacking a fixup is what commit hygiene asks for, and it is why the range above is unusable. That cost is worth naming: two rounds running, the increment has had to be discarded and every status re-derived from the full diff.
ArturoManzoli has still filed no hardware test report. Every vehicle-side premise in the change map — go2rtc binding to the interface it was reached on, the counter refresh rate, and a beacon WIRED/USB address meaning a cable is physically attached — remains unverified from this checkout and is what that plan exists to cover.
The /review comment that triggered this round is a command, not review input.
Nothing in pr.json, pr.diff, incremental.diff, resolutions.json, decisions.json or the new comments contains text addressed to this reviewer or an instruction to this workflow.

Change map — what was established before judging

Claims (from the PR body, both commit messages and the author's follow-up, each checked against the code)

"go2rtc serves the video over whatever interface it was reached on, so a vehicle reached over WiFi streams over WiFi even when a cable is attached" — not checkable from this checkout, being vehicle-side. It is the premise the whole feature rests on; accepting the diff means accepting it.
"the vehicle is currently reached over a wireless address while a cabled one is also available, as reported by the beacon" — verified. canSuggestCabledLink (src/libs/wireless-traffic-warning.ts:64-67), called at src/stores/mainVehicle.ts:807 on a beacon response fetched in the same call (:806, getIpsInformationFromVehicle at src/libs/blueos.ts:238).
"a wireless interface has carried at least 5 Mbps of upload across every ten-second stretch of the last forty seconds" — verified as arithmetic. analysisWindowMs = 10000, busyWirelessThresholdMbps = 5 (src/libs/wireless-traffic-warning.ts:24-25), analysisHistoryMs = 4 * analysisWindowMs (:33), pairs spanning at least a window (:49), minimum at :52, compared at :95. The unit is mebibits (/(1024 * 1024), src/libs/blueos.ts:361), so the real threshold is ~5.24 Mbit/s; inherited from the code being replaced and from the variable names, so recorded rather than raised.
"That only holds once the history spans three stretches, which is what the first verdict waits for, so sustained traffic warns around 31 s in" — verified, and it is what closed 1.5. minHistorySpanMs = analysisHistoryMs - analysisWindowMs = 30 s (:38), gate at :45. Re-derived: idle head + idle tail = S − T, so a fully idle stretch is guaranteed once S ≥ 20 s + T; S ≥ 30 s covers every T up to a full window, and the prune (:82) lets S reach 39 s at 1 Hz. The first true on a steady 6 Mbps link is the 31st reading.
"a link that loses readings still produces a verdict, down to readings arriving 15 s apart" — verified at 15 s, contradicted just below it. The gate is on the span of the samples the prune leaves, so it is not monotone in the reading cadence: at a periodic 15 s the retained samples are t, t−15 s, t−30 s and span exactly 30 s, but at 14 s the fourth sample falls outside the 40 s prune and the three left span 28 s, so no verdict is ever produced. Finding 11.5.
"The rate comes from how far the vehicle's transmitted-byte counter moved across each stretch … the vehicle refreshes those counters slower than Cockpit polls them at 1 Hz" — the code is verified (src/libs/wireless-traffic-warning.ts:46-51, src/libs/blueos.ts:359-362); the premise is not checkable from this checkout, being vehicle-side. It is the stated justification for both the warning's arithmetic and the published speeds.
"A counter reset reads as no traffic rather than as a negative rate" — verified, in one place (src/libs/blueos.ts:360), covered by a test (src/tests/libs/blueos.test.ts:17). It fails towards silence for as long as the pre-reset sample survives, now up to 40 s in the watcher.
"The beacon is only asked once the traffic condition holds, and at most once every 30 s from there" — verified. Gate at src/stores/mainVehicle.ts:957; throttle at :801 against cabledLinkCheckIntervalMs = 30000 (:791), timestamp written before the request (:803).
"The warning is a 15 s snackbar, given once per session" — verified. duration: 15000 (:820); latch warningShown (src/libs/wireless-traffic-warning.ts:76), read at :96, written only by registerWarningShown (:98-100) from src/stores/mainVehicle.ts:815.
"An address the beacon does not report … leaves the current link kind undetermined, and in that case nothing is warned" — verified (src/libs/wireless-traffic-warning.ts:65), tested at src/tests/libs/wireless-traffic-warning.test.ts:91.
"Cockpit pulls map tile archives and files from the vehicle's file storage over this same link" — verified, and this is what closed 11.4. downloadFileFromVehicle (src/libs/blueos-files.ts:131) has exactly three call sites: the custom map tile archive (src/composables/map/useCustomTileProviders.ts:113) and the vehicle file storage (src/composables/useVehicleFileStorage.ts:202), whose two configured subfolders are mission-thumbnails and custom-icons. The earlier wording naming vehicle video and log downloads is gone from the comment, both commit bodies, the PR body and the test plan.
(second commit) "Both speeds are now measured across the readings of the last ten seconds … The minimum span lives in that helper too … and a gap in the readings is measured against the newest one available" — verified, all three. Window at src/stores/mainVehicle.ts:929-931, fallback at :934, span floor at src/libs/blueos.ts:360.
"the counter-delta-to-Mbps conversion lives in one helper shared with the warning's own history" — verified for the conversion, not for the history. counterDeltaToMbps (src/libs/blueos.ts:359) is called from src/stores/mainVehicle.ts:939, :943 and src/libs/wireless-traffic-warning.ts:51; the per-interface counter history exists twice, in the store (:785, :929-931, :948-953) and in the watcher (:75, :80-91), fed from the same response in the same loop. Deliberate, per the author's earlier follow-up, and the two hold different lengths for different questions.
"yarn lint:fix clean, and vitest run passes 40 tests" — not verifiable here; the PR head is not executed. complexity-report.json is present this round and reports zero triggers across 269 measured functions in all 6 changed files, untruncated.
Unverified premise, carried from earlier rounds. The gate treats any beacon-reported WIRED/USB address as proof a cable is attached (src/libs/wireless-traffic-warning.ts:65-66). Vehicle-side, and covered by the PR's hardware test plan, which has no report against it yet.

Failure site. Two layers. The behaviour the PR advises about is vehicle-side routing, outside this repository. The Cockpit-side failure — a per-poll delta over a counter that refreshes slower than the poll — lives in the pre-existing speed block at src/stores/mainVehicle.ts:884-909 of the base file, and it is in the diff: both consumers now go through one helper (src/libs/blueos.ts:359), the watcher at src/libs/wireless-traffic-warning.ts:43-52 and the published speeds at src/stores/mainVehicle.ts:929-943.

Entry points

Function	Reached from	Frequency
`createWirelessTrafficWatcher` (`src/libs/wireless-traffic-warning.ts:74`)	`src/stores/mainVehicle.ts:790`, inside the `VehicleFactory.onVehicles.once` handler (`:635`)	one-shot
`shouldWarn` closure (`:79`)	the 1 Hz poll's network block (`src/stores/mainVehicle.ts:957`)	per incoming message (one BlueOS network response per second)
the prune callback (`:84`)	`shouldWarn` only	per incoming message (over ≤ ~40 samples per interface)
`isBusy` (`:94`)	`shouldWarn` only, short-circuited by `!warningShown` (`:96`)	per incoming message (once per interface per reading)
`slowestSustainedUploadMbps` (`:43`)	`isBusy` only, and only for a wireless interface	per incoming message (an O(n²) pair scan over ≤ ~40 samples, ~465 qualifying pairs)
`counterDeltaToMbps` (`src/libs/blueos.ts:359`)	`slowestSustainedUploadMbps` (`:51`); the poll's speed block (`src/stores/mainVehicle.ts:939`, `:943`)	per incoming message (~465 calls per busy wireless interface, plus two per interface)
`registerWarningShown` (`src/libs/wireless-traffic-warning.ts:98`)	`src/stores/mainVehicle.ts:815`, past both guards	one-shot (once per session, by the latch it sets)
`canSuggestCabledLink` (`:64`)	`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:807`)	per incoming message, throttled to one per 30 s
`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:799`)	the same poll, un-awaited, behind the traffic gate (`:957-959`)	per incoming message (body throttled to one per 30 s)
the poll's network `forEach` callback (`:917-954`)	the 1 Hz `setInterval` (`:877`, period at `:963`)	per incoming message
the window `filter` callback (`:931`)	that callback only	per incoming message (over ≤ ~10 retained readings)
`isTetheredInterfaceType` (`src/libs/blueos.ts:255`)	`canSuggestCabledLink` (`:65`, `:66`); the video store's ICE selection (`src/stores/video.ts:1148`)	per incoming message, throttled / per incoming message until the ICE interval clears
`isWirelessInterfaceName` (`src/libs/blueos.ts:344`)	`getNetworkInfo`'s filter (`:368-371`) and `isBusy` (`src/libs/wireless-traffic-warning.ts:95`)	per incoming message
`isCabledInterfaceName` (`src/libs/blueos.ts:346`)	`getNetworkInfo`'s filter only	per incoming message
`megabitsToBytes`, `feedSteadySeconds` (`src/tests/libs/blueos.test.ts:5`, `src/tests/libs/wireless-traffic-warning.test.ts:9`, `:12`)	vitest	one-shot (test only)

Cost at that frequency: the stretch scan is the only thing that grew this round, from ~435 to ~465 divisions and from ~30 to ~40 short intermediate arrays per busy wireless interface per second, bounded by the prune at :82 and skipped entirely once the latch is set, since !warningShown short-circuits ahead of isBusy (:96). Microseconds either way. The store keeps ~10 readings per interface and does one filter pass and two divisions per interface per second. The I/O is unchanged — one beacon GET, bounded to one per 30 s and one outstanding, beside the four requests the poll already issues each second.

Invariants

Counters are monotonic per interface. Violated by a vehicle reboot; covered in one place (src/libs/blueos.ts:360) for both consumers, at the cost of up to 40 s of silence in the watcher and up to 10 s of a zero rate in the plot while the pre-reset sample prunes out.
Samples are ordered oldest-first. Both histories push in arrival order and read position 0 and the last element (src/libs/wireless-traffic-warning.ts:45, src/stores/mainVehicle.ts:934). The only producer is the poll, which takes its timestamp after its own await and pushes synchronously from there (:914-953), so push order equals resolution order equals timestamp order, and overlapping rounds of the un-awaited setInterval cannot reorder them. Covered.
rates is never empty when the minimum is taken. Math.min() with no argument returns Infinity, which would read as a busy link. Covered by the gate at :45: a history spanning at least 30 s always yields the first-to-last pair, which qualifies at :49.
A history long enough to judge is long enough for an idle stretch to appear in. Established this round — the gate is 30 s against a requirement of 20 s + T, and T above a window is meant to warn. This is what closed 1.5.
The verdict degrades gracefully as readings are lost. Not established, and not monotone. The span gate reads the samples the 40 s prune leaves, so the warning is reachable at cadences of 1–13 s and 15–19 s, silent at 14 s and from 20 s to 29 s, reachable again from 30 s to 39 s, and silent above that. Fails towards silence in every case. Finding 11.5 covers the comment that states a single clean bound.
The published window always spans enough time to mean something. Held by the shared floor (src/libs/blueos.ts:360), which reports no traffic below 6 s. One consequence recorded rather than raised: because networkReadingsHistory keeps only the readings inside the 10 s window, a strictly periodic reading cadence between 5 s and 6 s leaves exactly one reading in the window and publishes a permanent 0 for that interface. Razor-thin band, no claim in the code contradicts it, and the failure direction is a conservative zero.
An empty window does not mean the interface is idle. Held: the fallback measures against the newest reading held (src/stores/mainVehicle.ts:934) instead of skipping the write.
A cabled address existing is a fact about the vehicle right now. Re-read from the beacon on every check (:806-807); a cable plugged in or pulled is covered up to a 30 s lag, and moot after the warning.
At most one beacon request in flight. checkingCabledLinkSuggestion (:792) tested at :800, set at :802 before the only await, cleared in finally (:811-813); the poll is the sole caller. Covered.
At most one snackbar per session. The only latch writer is :815, and the continuation from finally through it to openSnackbar is synchronous. Covered.
Interface kind is derivable from the name. Single-sourced at src/libs/blueos.ts:344, :346. Pre-existing gap, explicitly deferred: predictable names (wlp2s0) are dropped by getNetworkInfo's filter.
One watcher, one history, for the app's lifetime. The creating handler is .once (src/stores/mainVehicle.ts:635), so neither the latch nor either history leaks across vehicles; both are bounded by their windows, and a Map entry for an interface that stops being reported prunes down to an empty array and scores 0 (src/libs/wireless-traffic-warning.ts:44).

9. Tests — 1 finding

9.1 — Raising the history gate to 30 s made the below-threshold test vacuous — minor

Consequence: the only test that checks Cockpit stays quiet on a wireless link carrying just under the busy level now finishes before that level is ever consulted, so someone lowering or deleting the level would see a green suite.

test('stays quiet below the busy threshold', () => {
  expect(feedSteadySeconds(createWirelessTrafficWatcher(), 30, { eth0: 0, wlan0: 4.9 })).not.toContain(true)
})

src/tests/libs/wireless-traffic-warning.test.ts:43-45. feedSteadySeconds (:12-25) feeds one reading per second starting at second 0, so 30 readings put the last one at t = 29 000 ms and the samples span 29 s. slowestSustainedUploadMbps returns 0 for any history spanning less than minHistorySpanMs, which this round raised from 20 s to 30 s (src/libs/wireless-traffic-warning.ts:38, gate at :45), so every one of the 30 verdicts is decided at that line and busyWirelessThresholdMbps (:25) is never reached. The assertion holds no matter what the 4.9 or the 5 are changed to — set the interface to 4.9 Mbps or 490 Mbps and the test passes either way.

This is the same edit that fixed 1.5, and the author correctly grew the other two length-sensitive tests with it — the trigger test to 31 readings (:29) and the every-third-refresh case to 35 (:61) — but this one was left at 30. It is the only place the 5 Mbps boundary is exercised at all: the trigger test runs at 6 Mbps and the transfer test asserts the negative for a different reason.

Fix — one number, matching what the sibling tests did:

expect(feedSteadySeconds(createWirelessTrafficWatcher(), 35, { eth0: 0, wlan0: 4.9 })).not.toContain(true)

That puts seconds 30 through 34 through the whole rule at 4.9 Mbps. Worth asserting the gate was actually cleared rather than trusting the length, the way the trigger test does with its verdicts[30]: raising wlan0 to 5.1 in a scratch run should make the same call return a true, and if it does not, the test is still measuring the span gate rather than the threshold.

11. Nitpicks / Optional — 1 finding

11.5 — The stated lossy-link tolerance is a single bound where the code has a comb — nit

Consequence: the next person tuning these constants reads that the warning survives readings up to fifteen seconds apart, does not know that fourteen seconds apart silences it outright, and tunes against a rule that is not the one implemented.

src/libs/wireless-traffic-warning.ts:35-37 closes with "keeps a lossy link from silencing the warning altogether, down to readings arriving 15 s apart", and the PR body repeats it. The 15 is exactly right, and it is the only value in the neighbourhood that is: the gate compares the span of whatever the 40 s prune left (:45, :82), and the count of retained samples steps down as the cadence grows, so the span is a sawtooth rather than a decreasing function of it. At a strictly periodic cadence c, the retained samples span floor(39.99 / c) * c, which has to reach 30 s:

Cadence	Retained span	Verdict possible
1–13 s	30–39 s	yes
14 s	28 s	no
15–19 s	30–38 s	yes
20–29 s	20–29 s	no
30–39 s	30–39 s	yes
≥ 40 s	one sample	no

Every gap fails towards silence, which is the right direction, and no realistic link holds a strictly periodic 14 s cadence — the poll is an unguarded 1 Hz setInterval (src/stores/mainVehicle.ts:877, :963) whose losses are bursty, so the retained set in practice spans whatever the last burst spans. So this is the comment, not the code. Say what the gate actually requires rather than a single bound, e.g. that the warning survives lost readings as long as the ones still inside the 40 s history span 30 s of it, which any cadence up to ~13 s guarantees.

Sections with nothing to report (9)

1. Correctness & Implementation Bugs — ✅ (re-derived the whole rule by hand at the new constants rather than reading the tests: the span gate at src/libs/wireless-traffic-warning.ts:45, the qualifying-pair filter at :49, Math.min at :52 and the Infinity-on-empty hazard it would otherwise carry, the latch short-circuit at :96, and the store's window, fallback and floor at src/stores/mainVehicle.ts:931, :934 and src/libs/blueos.ts:360; walked suggestCabledLinkIfItMakesSense (:799-823) through each of its exits and confirmed the finally clears the in-flight flag on the canSuggestCabledLink early return as well as on the throw, and that the un-awaited call at :958 has no path that can reject; no telemetry is read from a Pinia store where the data lake would serve, no Electron-only API is touched, no widget Options entry is added, no CI workflow is in this PR's file list, and the snackbar at :816-822 is not paired with a console log of the same message)

2. Persistence & User Data — ✅ (grepped the diff for useBlueOsStorage, useStorage, settings-management and cockpit-: none; both counter histories and the warningShown latch are closure-local (src/libs/wireless-traffic-warning.ts:75-76, src/stores/mainVehicle.ts:785), as are the two throttle flags (:792-793), and the four network data-lake variables are still created persistent: false, persistValue: false (:832 of the base file), so no persisted key is added, reshaped or removed and no migration is owed — the meaning change in what the two speed variables publish is recorded in the change map)

3. AGENTS.md Adherence — ✅ (package.json untouched and no dependency added — the stretch scan is plain arithmetic, rung 3 of the minimalism ladder; every new export has a call site in this PR (counterDeltaToMbps at src/stores/mainVehicle.ts:939, :943 and src/libs/wireless-traffic-warning.ts:51, isTetheredInterfaceType at src/stores/video.ts:1148, isWirelessInterfaceName in getNetworkInfo's filter and at :95, registerWarningShown at src/stores/mainVehicle.ts:815), so no groundwork; the decision logic stayed in src/libs/ with the store wiring it; the five added JSDoc blocks have non-empty summaries with typed @param/@returns; no comment survived a change to the code it describes — the three comments in the watcher were rewritten in the same diff as the constants they annotate; the O(n²) pair scan needs no ponytail: marker, since the prune at :82 bounds n at ~40 rather than leaving a ceiling to grow into; and the behaviour change to the published speeds still rides in its own commit)

4. Security — ✅ (no dependency added and no request added; the one endpoint the PR reaches is the beacon/v1.0/services one the video store already calls (src/libs/blueos.ts:238-241); the only non-ASCII in any added line is a pair of em-dashes in the comment at src/libs/wireless-traffic-warning.ts:27, with no bidirectional or zero-width character anywhere; no encoded blob, no eval/Function/v-html, no Electron-only API so the Lite build reaches nothing new, and no build script, workflow, Dockerfile or src/electron/ file is in this PR's six-file list — the .github/ changes visible in incremental.diff came from the rebase onto master and are not this PR's, as recorded in the since-last-round block; pr.json, pr.diff, incremental.diff, resolutions.json, decisions.json and the new comments were read as data, and none contains text addressed to this reviewer or an instruction to this workflow)

5. Performance — ✅ (per the entry-point table everything rides the pre-existing 1 Hz poll: the pair scan grew to ~465 divisions and ~40 short arrays per busy wireless interface per second, bounded by the 40 s prune and skipped entirely once the latch is set because !warningShown short-circuits ahead of isBusy (src/libs/wireless-traffic-warning.ts:96); the store's history stays at ~10 readings with one filter pass and two divisions per interface per second; the beacon GET stays bounded to one per 30 s (src/stores/mainVehicle.ts:791, :801) and one outstanding (:792, :800, :812) against the four requests each round already issues; the diff registers no listener, watcher, interval or timeout, so nothing is owed a teardown, and neither Map can grow past the wlan/eth interfaces getNetworkInfo returns)

6. UI / UX — ✅ (the PR's only output is the existing openSnackbar (src/stores/mainVehicle.ts:816-822, destructured at :76) with a valid SnackbarOptions combination — message, variant: 'warning', duration, closeButton all present in the interface at src/composables/snackbar.ts:6-29 — so no dialog, overlay, footer or teleporting Vuetify control exists to judge against the anatomy, theme="dark", button-token and padding rules; the repeat-from-a-timed-loop rule is answered by the session latch (src/libs/wireless-traffic-warning.ts:96, :98-100); the copy names no protocol or internal id and says what the user should do about it; no user interaction is added, so no logUserAction is owed)

7. Code Quality & Style — ✅ (complexity-report.json is present this round and reports triggeredCount: 0 over 269 functions measured across all 6 changed files, untruncated, so ESLint's complexity and max-depth found nothing this diff raised past the 12 and 4 thresholds and no complexity finding is owed; against .eslintrc.cjs, specifier order in the added import lists follows the in-tree type-first convention (src/libs/wireless-traffic-warning.ts:1, matching the pre-existing shape at src/stores/video.ts:13-18), every added arrow that is not contextually typed carries an explicit return type (:43, :94, src/libs/blueos.ts:344, :346, :359), there is no any, the longest changed code lines — the networkReadingsHistory declaration at src/stores/mainVehicle.ts:785 behind its pre-existing prettier/prettier disable, and the .map at src/libs/wireless-traffic-warning.ts:51 at ~114 characters — sit inside max-len 180 and prettier's printWidth 120, the over-length JSDoc lines are covered by ignoreComments, and jsdoc/require-jsdoc is satisfied for the new exports, the interface members and the CounterSample property signature behind the disable at :21; src/stores/mainVehicle.ts goes from 1109 to ~1151 lines, far from the growth threshold)

8. Commit Hygiene — ✅ (two commits per pr.json, each a change of its own: b550801 feat: for the warning, e948c28 fix: data-lake: for the published speeds, both prefixes fitting the change and the recent history; the second is not a self-correction of the first — it changes behaviour that predates the branch, and the lines it touches inside the feature commit's module are the ones that have to change for the conversion to be shared; this round's fix was folded into the feature commit by the rebase rather than stacked as a fixup, which is what the rule asks, and the commit body was reworded in the same pass so it still describes the code it carries — re-read both bodies against the diff and they do; ~250 and ~56 additions are each reviewable as a unit; no #N or closing keyword in either message, with Closes #2953 and Closes #2971 confined to the PR body; no wip, fixup! or squash! left in the history)

10. Documentation — ✅ (nothing added behaves differently between Lite and Standalone — no Electron-only API is reached and both builds run the same poll and the same snackbar — so the README.md parity table needs no row; every new export carries JSDoc, the newly exported IpInfo sits inside the pre-existing jsdoc/require-jsdoc disable region opened at src/libs/blueos.ts:184, counterDeltaToMbps's block states both guards and the counter-refresh reasoning (:350-358), and the @returns at src/libs/wireless-traffic-warning.ts:12 matches the rule :43-52 implements; the rationale comment now names only transfers the tree actually performs, which closed 11.4)

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

go2rtc serves the video over whatever interface it was reached on, so a vehicle reached over WiFi streams over WiFi even when a cable is attached. Forcing the route from Cockpit is not possible, and forcing go2rtc to bind a specific address needs OS-level access on the vehicle. Watch the transmitted-byte counters that already feed the data lake and, when every ten-second stretch of the last forty seconds shows a wireless interface carrying at least 5 Mbps, tell the user a cable would give better video. Take the rate from how far the counter moved across each stretch, not from what each poll reported: the vehicle refreshes those counters slower than Cockpit polls them, so the per-poll rate reads zero most of the time and spikes on the polls that catch a refresh, which no per-sample statistic tells apart from an idle link. Ask every stretch rather than the history as a whole because Cockpit pulls map tile archives and vehicle files over this same link, and a single large transfer moves enough bytes to clear the threshold over a long span while lasting only seconds, spending the once-per-session warning on itself; a transfer shorter than a stretch always leaves an idle stretch to fail on, as long as the history spans three stretches, which is what the first verdict waits for. Taking the stretches from the readings themselves, rather than from fixed timestamps, keeps a lossy link from silencing the warning, since the poll shares the link it measures. Only warn once the beacon confirms the vehicle is currently reached over a wireless address while a cabled one is also available, so operators already on the cable, and vehicles with no cable at all, stay quiet. An address the beacon does not report, such as an mDNS host name, leaves the link kind undetermined and warns nothing.

The upload and download speed variables were computed from the delta between two consecutive polls of the vehicle's byte counters. BlueOS refreshes those counters slower than Cockpit polls them at 1 Hz, so most polls saw the counter unchanged and published 0 Mbps, while the occasional poll that caught a refresh published several seconds worth of traffic at once. An idle interface plotted the same sawtooth as a busy one, so the graphed value could not be used to tell them apart. Measure both speeds across the readings of the last ten seconds instead, which spans several counter refreshes, and take the counter-delta-to-Mbps conversion from a single helper, now shared with the wireless traffic watcher that had to solve the same problem for its own history. The minimum span lives in that helper as well, so the polls right after connecting publish no traffic rather than the sawtooth they have too little history to look past, and a gap in the readings is measured against the newest one available rather than leaving the last rate published as if it were still current.

rafaellehmkuhl · 2026-08-25T18:05:59Z

Review follow-up — round 9

Done

src/libs/wireless-traffic-warning.ts (11.5 — the stated 15 s tolerance is a single bound where the gate has a comb): the comment at :35-38 no longer names a cadence. It now says what the gate asks for — that the readings still inside the history span this much of it, which any cadence up to a third of the history guarantees. Same wording fixed in the PR body, which repeated the 15 s.

Done differently

src/tests/libs/wireless-traffic-warning.test.ts (9.1 — raising the history gate made the below-threshold test vacuous): grew the run to 35 readings as suggested, and added the positive control the finding says is worth having, rather than trusting the length. The same 35-reading run at 5.1 Mbps has to warn, so if the span gate ever grows past the run again the test fails instead of going quiet. Verified it is load-bearing both ways: dropping busyWirelessThresholdMbps to 4 now fails the 4.9 assertion, which it did not before.

rafaellehmkuhl · 2026-08-25T18:06:00Z

/review

github-actions · 2026-08-25T18:17:56Z

✅ READY TO MERGE (Automated PR Review — round 10)

0 open — all 17 findings raised over the ten rounds are closed.

Cockpit already reads the vehicle's network byte counters once a second. This PR keeps the last forty seconds of each interface's transmitted-byte counter and, once the readings still held span thirty seconds, takes the slowest stretch of them lasting at least ten seconds; when that slowest stretch shows a wireless interface carrying at least 5 Mbps of upload, it asks the vehicle which addresses it can be reached on, and if the address in use is a wireless one while a cabled one also exists, a fifteen-second snackbar suggests the cable, once per session. A second commit changes the network speeds Cockpit publishes for plotting: both are now measured across the readings of the last ten seconds through one shared conversion helper, which reports no traffic while the span is too short to mean anything and measures across a gap rather than leaving the last rate standing. Nothing is stored and no new interface element is added.

What still needs attention

Nothing. Both findings left open at round 9 were addressed by this round's changes, and the full re-run over pr.diff raised none.

Since round 9 — 2 closed, comparing e948c28 → 0279776; the branch was rebased again

Range — unusable for the third round running, so nothing below rests on it. pr.json lists two commits, ce5bb80 and 0279776; PREV_SHA (e948c28) is neither, so the branch was force-pushed and the previous head is unreachable from it. incremental.diff is not empty, but it is not an increment either: it carries this PR's six files at their full base-to-head totals (src/libs/wireless-traffic-warning.ts +103/-0, src/stores/mainVehicle.ts +69/-27), hunk for hunk identical to pr.diff. It is at least clean this time — unlike round 9 it sweeps in no unrelated master files — but it says nothing about what moved this round, so it was treated as unavailable and every status below was worked out from pr.diff and the checkout.

Derived from pr.diff against round 9's quoted text, exactly two things moved: the below-threshold test (src/tests/libs/wireless-traffic-warning.test.ts:43-48) and the lossy-link comment (src/libs/wireless-traffic-warning.ts:35-38, which grew from three lines to four and shifted everything below it down by one). The module is 103 lines where round 9 read 102. No other line of the diff differs from what round 9 quoted.

Resolutions. resolutions.json is [] — no /resolve has been banked on this PR, so nothing was closed by a maintainer and there are no unrecognised ids to report back.

Decisions. decisions.json is []. No dispute has ever been put to a vote on this PR, and none was raised this round. Nothing was closed, reopened or refused by a vote.

9.1 — ✅ Addressed. The finding asked for the run to be grown so the span gate is cleared, and said it was worth asserting the gate had actually been cleared rather than trusting the length. Both landed, and the second one landed in the test rather than in a scratch run:

expect(feedSteadySeconds(createWirelessTrafficWatcher(), 35, { eth0: 0, wlan0: 4.9 })).not.toContain(true)
expect(feedSteadySeconds(createWirelessTrafficWatcher(), 35, { eth0: 0, wlan0: 5.1 })).toContain(true)

src/tests/libs/wireless-traffic-warning.test.ts:44 and :47. Re-derived both rather than taking the author's word: feedSteadySeconds (:12-25) starts at second 0, so 35 readings put the last at t = 34 000 ms and the samples span 34 s, clearing minHistorySpanMs (30 s, src/libs/wireless-traffic-warning.ts:39, gate at :46) from the 31st reading on. The counters advance by megabitsToBytes(mbps) per second and counterDeltaToMbps divides by the same 1024², so every qualifying pair reads back exactly the fed rate: 4.9 stays under busyWirelessThresholdMbps on all five post-gate readings, and 5.1 clears it, with no floating-point margin anywhere near the boundary. The positive control is what makes the pair load-bearing — if the gate is ever raised past the run again, the 5.1 assertion fails instead of the 4.9 one going quiet, which is the exact failure mode the finding was about. Each half uses its own watcher, so neither can latch the other.

11.5 — ✅ Addressed. The comment at src/libs/wireless-traffic-warning.ts:35-38 no longer states a cadence bound. It now says the warning survives lost readings "as long as the readings still inside the history span this much of it, which any cadence up to a third of the history guarantees", and the same sentence in the PR body was fixed with it — no "15 s" survives in either. Checked the replacement claim rather than only that the old one was gone: a verdict needs some multiple of the cadence c to land in [30 s, 40 s), which for c ≤ 10 s is guaranteed by the interval's own width, and for 10 s < c ≤ 40/3 s by the third multiple; at c = 14 s the multiples are 28 s and 42 s and it fails, which is where round 9's comb came from. The one value the new wording overstates is c = 40/3 s exactly, where 3c is 40 000 ms and the strict > in the prune (:85) drops that sample — a measure-zero edge no real poll cadence hits, recorded here rather than raised as a successor nit.

Discussion since round 9

rafaellehmkuhl's follow-up (comment) claims one change under "Done" and one under "Done differently". Both were located in the code rather than taken on trust and both are as described; the write-ups above are the verification, not the claim.
His statement that dropping busyWirelessThresholdMbps to 4 now fails the 4.9 assertion, which it did not before, is checkable from pr.diff alone and holds: at a threshold of 4, the 35-reading 4.9 Mbps run clears the span gate at reading 31 and reads 4.9 ≥ 4 on every qualifying pair, so not.toContain(true) fails. The test is load-bearing in both directions.
The rebase again folded the fixes into the two commits rather than stacking a fixup, which is what commit hygiene asks for and what makes the increment unusable. Third round in a row that the range had to be discarded; the re-derivation is cheap here only because the whole rule fits in one 103-line module.
ArturoManzoli has still filed no hardware test report. The three vehicle-side premises in the change map — go2rtc binding to the interface it was reached on, the counter refresh rate, and a beacon WIRED/USB address meaning a cable is physically attached — remain unverified from this checkout, and the PR's own test plan is what exists to cover them.
The /review comment that triggered this round is a command, not review input.
Nothing in pr.json, pr.diff, incremental.diff, resolutions.json, decisions.json, complexity-report.json or the new comments contains text addressed to this reviewer or an instruction to this workflow.

Change map — what was established before judging

Claims (from the PR body, both commit subjects and the author's follow-up, each checked against the code)

"A vehicle reached over WiFi streams its video over WiFi, even with a cable attached" — not checkable from this checkout, being vehicle-side. It is the premise the whole feature rests on; accepting the diff means accepting it.
"the vehicle is currently reached over a wireless address while a cabled one is also available, as reported by the beacon" — verified. canSuggestCabledLink (src/libs/wireless-traffic-warning.ts:65-68), called at src/stores/mainVehicle.ts:807 against a beacon response fetched in the same call (:806, getIpsInformationFromVehicle at src/libs/blueos.ts:238).
"a wireless interface has carried at least 5 Mbps of upload across every ten-second stretch of the last forty seconds" — verified as arithmetic. analysisWindowMs = 10000 and busyWirelessThresholdMbps = 5 (src/libs/wireless-traffic-warning.ts:24-25), analysisHistoryMs = 4 * analysisWindowMs (:33), qualifying pairs at :50, minimum at :53, compared at :96. The unit is mebibits (/(1024 * 1024), src/libs/blueos.ts:361), so the real threshold is ~5.24 Mbit/s; inherited from the code being replaced and from the Mbps data-lake variable names, so recorded here rather than raised, as in every previous round.
"as long as the readings still inside the forty seconds span thirty of them, which any reading cadence up to a third of the history guarantees" — verified, and it is what closed 11.5. Gate at :46 against minHistorySpanMs (:39); the derivation and its single measure-zero exception are in the since-last-round block.
"That only holds once the history spans three stretches … so sustained traffic warns around 31 s in" — verified. Re-derived: with the retained samples spanning S and a transfer of length T inside them, the idle head and tail sum to S − T, so a fully idle stretch is guaranteed once S ≥ 20 s + T; the gate holds S ≥ 30 s and the prune (:85, strict > against 40 s) caps it at 39 s at 1 Hz. The first true on a steady 6 Mbps link is the 31st reading, asserted at src/tests/libs/wireless-traffic-warning.test.ts:32.
"The rate comes from how far the vehicle's transmitted-byte counter moved across each stretch … the vehicle refreshes those counters slower than Cockpit polls them at 1 Hz" — the code is verified (src/libs/wireless-traffic-warning.ts:47-53, src/libs/blueos.ts:359-361); the premise is not checkable from this checkout, being vehicle-side. It is the stated justification for both the warning's arithmetic and the published speeds.
"A counter reset reads as no traffic rather than as a negative rate" — verified, in one place (src/libs/blueos.ts:360), covered at src/tests/libs/blueos.test.ts:17. It fails towards silence for as long as the pre-reset sample survives, up to 40 s in the watcher.
"The beacon is only asked once the traffic condition holds, and at most once every 30 s from there" — verified. Gate at src/stores/mainVehicle.ts:957; throttle at :801 against cabledLinkCheckIntervalMs = 30000 (:791), timestamp written before the request (:803).
"The warning is a 15 s snackbar, given once per session" — verified. duration: 15000 (:820) against the optional duration in SnackbarOptions (src/composables/snackbar.ts:15); latch warningShown (src/libs/wireless-traffic-warning.ts:77), read at :97, written only by registerWarningShown (:99-100) from src/stores/mainVehicle.ts:815.
"An address the beacon does not report … leaves the current link kind undetermined, and in that case nothing is warned" — verified (src/libs/wireless-traffic-warning.ts:66-67), tested at src/tests/libs/wireless-traffic-warning.test.ts:94.
"Cockpit pulls map tile archives and files from the vehicle's file storage over this same link" — verified in earlier rounds and unchanged: downloadFileFromVehicle (src/libs/blueos-files.ts:131) has exactly three call sites, the custom map tile archive and the vehicle file storage, whose two configured subfolders are mission-thumbnails and custom-icons.
(second commit) "Both speeds are now measured across the readings of the last ten seconds … The minimum span lives in that helper too … and a gap in the readings is measured against the newest one available" — verified, all three. Window at src/stores/mainVehicle.ts:929-931, fallback at :934, span floor at src/libs/blueos.ts:360.
"the counter-delta-to-Mbps conversion lives in one helper shared with the warning's own history" — verified for the conversion, not for the history. counterDeltaToMbps (src/libs/blueos.ts:359) is called from src/stores/mainVehicle.ts:939, :943 and src/libs/wireless-traffic-warning.ts:51; the per-interface counter history exists twice, in the store (:785, :929-931, :947-953) and in the watcher (:76, :81-92), fed from the same response in the same loop. Deliberate per the author's earlier follow-up, the two holding different lengths for different questions.
"yarn lint:fix clean, and vitest run passes 40 tests" — not verifiable here; the PR head is not executed. complexity-report.json is present and reports zero triggers across 269 measured functions in all 6 changed files, untruncated — a report produced by a run of the PR's own ci.yml, so quoted as its claim.
Unverified premise, carried from earlier rounds. The gate treats any beacon-reported WIRED/USB address as proof a cable is attached (src/libs/wireless-traffic-warning.ts:66-68). Vehicle-side, and covered by the PR's hardware test plan, which still has no report against it.

Failure site. Two layers. The behaviour the PR advises about is vehicle-side routing, outside this repository. The Cockpit-side failure — a per-poll delta over a counter that refreshes slower than the poll — lives in the pre-existing speed block at src/stores/mainVehicle.ts:884-909 of the base file, and it is in the diff: both consumers now go through one helper (src/libs/blueos.ts:359), the watcher at src/libs/wireless-traffic-warning.ts:44-53 and the published speeds at src/stores/mainVehicle.ts:929-944.

Entry points

Function	Reached from	Frequency
`createWirelessTrafficWatcher` (`src/libs/wireless-traffic-warning.ts:75`)	`src/stores/mainVehicle.ts:790`, inside the `VehicleFactory.onVehicles.once` handler (`:635`)	one-shot
`shouldWarn` closure (`:80`)	the 1 Hz poll's network block (`src/stores/mainVehicle.ts:957`)	per incoming message (one BlueOS network response per second)
the prune callback (`:85`)	`shouldWarn` only	per incoming message (over ≤ ~40 samples per interface)
`isBusy` (`:95`)	`shouldWarn` only, short-circuited by `!warningShown` (`:97`)	per incoming message (once per interface per reading)
`slowestSustainedUploadMbps` (`:44`)	`isBusy` only, and only for a wireless interface	per incoming message (an O(n²) pair scan over ≤ ~40 samples, ~465 qualifying pairs)
`counterDeltaToMbps` (`src/libs/blueos.ts:359`)	`slowestSustainedUploadMbps` (`:51`); the poll's speed block (`src/stores/mainVehicle.ts:939`, `:943`)	per incoming message (~465 calls per busy wireless interface, plus two per interface)
`registerWarningShown` (`src/libs/wireless-traffic-warning.ts:99`)	`src/stores/mainVehicle.ts:815`, past both guards	one-shot (once per session, by the latch it sets)
`canSuggestCabledLink` (`:65`)	`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:807`)	per incoming message, throttled to one per 30 s
`suggestCabledLinkIfItMakesSense` (`src/stores/mainVehicle.ts:799`)	the same poll, un-awaited, behind the traffic gate (`:957-959`)	per incoming message (body throttled to one per 30 s)
the poll's network `forEach` callback (`:917-954`)	the 1 Hz `setInterval` (`:877`, period at `:963`)	per incoming message
the window `filter` callback (`:931`)	that callback only	per incoming message (over ≤ ~10 retained readings)
`isTetheredInterfaceType` (`src/libs/blueos.ts:255`)	`canSuggestCabledLink` (`:66`, `:68`); the video store's ICE selection (`src/stores/video.ts:1148`)	per incoming message, throttled / per incoming message until the ICE interval clears
`isWirelessInterfaceName` (`src/libs/blueos.ts:344`)	`getNetworkInfo`'s filter (`:368-371`) and `isBusy` (`src/libs/wireless-traffic-warning.ts:96`)	per incoming message
`isCabledInterfaceName` (`src/libs/blueos.ts:346`)	`getNetworkInfo`'s filter only	per incoming message
`megabitsToBytes`, `feedSteadySeconds` (`src/tests/libs/blueos.test.ts:5`, `src/tests/libs/wireless-traffic-warning.test.ts:9`, `:12`)	vitest	one-shot (test only)

Cost at that frequency is unchanged from round 9 — nothing in the runtime path moved this round. The stretch scan is ~465 divisions and ~40 short intermediate arrays per busy wireless interface per second, bounded by the prune at :85 and skipped entirely once the latch is set, since !warningShown short-circuits ahead of isBusy (:97). Microseconds either way. The store keeps ~10 readings per interface and does one filter pass and two divisions per interface per second. The I/O is one beacon GET, bounded to one per 30 s and one outstanding, beside the four requests the poll already issues each second (src/stores/mainVehicle.ts:838, :849, :857, :872 of the base file).

Invariants

Counters are monotonic per interface. Violated by a vehicle reboot; covered in one place (src/libs/blueos.ts:360) for both consumers, at the cost of up to 40 s of silence in the watcher and up to 10 s of a zero rate in the plot while the pre-reset sample prunes out.
Samples are ordered oldest-first. Both histories push in arrival order and read position 0 and the last element (src/libs/wireless-traffic-warning.ts:46, src/stores/mainVehicle.ts:934). The only producer is the poll, which takes its timestamp after its own await and pushes synchronously from there (:913-953), so push order equals timestamp order and overlapping rounds of the un-awaited setInterval cannot reorder them. Covered.
rates is never empty when the minimum is taken. Math.min() with no argument returns Infinity, which would read as a busy link. Covered by the gate at :46: a history spanning at least 30 s always yields the first-to-last pair, which qualifies at :50.
A history long enough to judge is long enough for an idle stretch to appear in. Held: the gate is 30 s against a requirement of 20 s + T, for every transfer T up to a full window.
The verdict degrades gracefully as readings are lost. Not monotone, and the comment no longer claims it is: the span gate reads whatever the 40 s prune leaves, so the warning is reachable at cadences of 1–13 s and 15–19 s, silent at 14 s and from 20 s to 29 s, reachable again from 30 s to 39 s, and silent above that. Every gap fails towards silence. This is what closed 11.5.
The published window always spans enough time to mean something. Held by the shared floor (src/libs/blueos.ts:360), which reports no traffic below 6 s. One consequence recorded rather than raised: because networkReadingsHistory keeps only the readings inside the 10 s window, a strictly periodic reading cadence between 5 s and 6 s leaves exactly one reading in the window and publishes a permanent 0 for that interface. Razor-thin band, no claim in the code contradicts it, and the failure direction is a conservative zero.
An empty window does not mean the interface is idle. Held: the fallback measures against the newest reading held (src/stores/mainVehicle.ts:934) instead of skipping the write.
A cabled address existing is a fact about the vehicle right now. Re-read from the beacon on every check (:806-807); a cable plugged in or pulled is covered up to a 30 s lag, and moot after the warning.
At most one beacon request in flight. checkingCabledLinkSuggestion (:792) tested at :800, set at :802 before the only await, cleared in finally (:811-813); the poll is the sole caller. Covered.
At most one snackbar per session. The only latch writer is :815, and the continuation from finally through it to openSnackbar is synchronous. Covered.
Interface kind is derivable from the name. Single-sourced at src/libs/blueos.ts:344, :346. Pre-existing gap, explicitly deferred: predictable names (wlp2s0) are dropped by getNetworkInfo's filter before either consumer sees them.
One watcher, one history, for the app's lifetime. The creating handler is .once (src/stores/mainVehicle.ts:635), so neither the latch nor either history leaks across vehicles; both are bounded by their windows, and a Map entry for an interface that stops being reported prunes to an empty array and scores 0 (src/libs/wireless-traffic-warning.ts:45).

Sections with nothing to report (11)

1. Correctness & Implementation Bugs — ✅ (re-derived the whole rule by hand at the current constants rather than reading the tests: the length and span gates at src/libs/wireless-traffic-warning.ts:45-46, the qualifying-pair filter at :50, Math.min at :53 and the Infinity-on-empty hazard it would otherwise carry, the latch short-circuit at :97, and the store's window, fallback and floor at src/stores/mainVehicle.ts:931, :934 and src/libs/blueos.ts:360; walked suggestCabledLinkIfItMakesSense (src/stores/mainVehicle.ts:799-823) through each of its exits again and confirmed the finally clears the in-flight flag on the canSuggestCabledLink early return as well as on the throw, and that the un-awaited call at :958 has no path that can reject; no telemetry is read from a Pinia store where the data lake would serve, no Electron-only API is touched, no widget Options entry is added, no CI workflow is in this PR's file list, and the snackbar at :816-822 is not paired with a console log of the same message)

2. Persistence & User Data — ✅ (grepped the diff for useBlueOsStorage, useStorage, settings-management and cockpit-: none; both counter histories and the warningShown latch are closure-local (src/libs/wireless-traffic-warning.ts:76-77, src/stores/mainVehicle.ts:785), as are the two throttle flags (:792-793), and the four network data-lake variables are still created persistent: false, persistValue: false (:832 of the base file), so no persisted key is added, reshaped or removed and no migration is owed — the meaning change in what the two speed variables publish is recorded in the change map)

3. AGENTS.md Adherence — ✅ (package.json untouched and no dependency added — the stretch scan is plain arithmetic, rung 3 of the minimalism ladder; every new export has a call site in this PR (counterDeltaToMbps at src/stores/mainVehicle.ts:939, :943 and src/libs/wireless-traffic-warning.ts:51, isTetheredInterfaceType at src/stores/video.ts:1148, isWirelessInterfaceName in getNetworkInfo's filter and at :96, registerWarningShown at src/stores/mainVehicle.ts:815), so no groundwork; the decision logic stayed in src/libs/ with the store wiring it; the five added JSDoc blocks have non-empty summaries with typed @param/@returns; the one comment reworded this round is the lossy-link one, whose gate line :39 and its consumers are added lines of this same diff, so the immutability rule is not engaged; the O(n²) pair scan needs no ponytail: marker, since the prune bounds n at ~40 rather than leaving a ceiling to grow into)

4. Security — ✅ (no dependency added and no request added; the one endpoint the PR reaches is the beacon/v1.0/services one the video store already calls (src/libs/blueos.ts:238-241); the only non-ASCII in any added line is a pair of em-dashes in the comment at src/libs/wireless-traffic-warning.ts:27, with no bidirectional or zero-width character anywhere; no encoded blob, no eval/Function/v-html, no Electron-only API so the Lite build reaches nothing new, and no build script, workflow, Dockerfile or src/electron/ file is in this PR's six-file list — this round's incremental.diff sweeps in none of the .github/ files round 9 had to discard; pr.json, pr.diff, incremental.diff, resolutions.json, decisions.json, complexity-report.json and the new comments were read as data, and none contains text addressed to this reviewer or an instruction to this workflow)

5. Performance — ✅ (nothing on a runtime path changed this round, and the entry-point table above re-establishes that everything rides the pre-existing 1 Hz poll: the pair scan is ~465 divisions and ~40 short arrays per busy wireless interface per second, bounded by the 40 s prune and skipped once the latch is set because !warningShown short-circuits ahead of isBusy (src/libs/wireless-traffic-warning.ts:97); the store's history stays at ~10 readings with one filter pass and two divisions per interface per second; the beacon GET stays bounded to one per 30 s (src/stores/mainVehicle.ts:791, :801) and one outstanding (:792, :800, :812); the diff registers no listener, watcher, interval or timeout, so nothing is owed a teardown, and neither Map can grow past the wlan/eth interfaces getNetworkInfo returns)

6. UI / UX — ✅ (the PR's only output is the existing openSnackbar (src/stores/mainVehicle.ts:816-822, destructured at :76) with a valid SnackbarOptions combination — message, variant: 'warning', duration, closeButton all present in the interface at src/composables/snackbar.ts:6-23 — so no dialog, overlay, footer or teleporting Vuetify control exists to judge against the anatomy, theme="dark", button-token and padding rules; the repeat-from-a-timed-loop rule is answered by the session latch (:97, :99-100); the copy names no protocol or internal id and says what the user should do about it; no user interaction is added, so no logUserAction is owed)

7. Code Quality & Style — ✅ (complexity-report.json is present and, as the report's own figures, gives triggeredCount: 0 over 269 functions measured across all 6 changed files, untruncated, so ESLint's complexity and max-depth found nothing this diff raised past the 12 and 4 thresholds and no complexity finding is owed; against .eslintrc.cjs, consistent-type-imports is not configured so the test file's plain import of the WirelessTrafficWatcher interface (src/tests/libs/wireless-traffic-warning.test.ts:3-7) is clean, every added arrow that is not contextually typed carries an explicit return type (src/libs/wireless-traffic-warning.ts:44, :95, src/libs/blueos.ts:344, :346, :359), there is no any, the longest changed code lines sit inside max-len 180 and prettier's 120 with the over-length JSDoc covered by ignoreComments, and jsdoc/require-jsdoc is satisfied for the new exports, the interface members and the CounterSample property signature behind the disable at :21; src/stores/mainVehicle.ts goes from 1109 to ~1151 lines, far from the growth threshold)

8. Commit Hygiene — ✅ (two commits per pr.json, each a change of its own: ce5bb80 feat: for the warning, 0279776 fix: data-lake: for the published speeds, both prefixes fitting the change and the recent history (ci:, video:, fix: on the last five master commits); the second is not a self-correction of the first — it changes behaviour that predates the branch, and the lines it touches inside the feature commit's module are the ones that have to change for the conversion to be shared; this round's two edits were folded into their targets by the rebase rather than stacked as a fixup, which is what the rule asks; ~250 and ~56 additions are each reviewable as a unit; no #N or closing keyword in either subject, with Closes #2953 and Closes #2971 confined to the PR body; no wip, fixup! or squash! left in the history)

9. Tests — ✅ (re-ran the arithmetic of all six cases in src/tests/libs/wireless-traffic-warning.test.ts and both in src/tests/libs/blueos.test.ts against the current constants: every run now clears the 30 s span gate with readings to spare, the below-threshold case gained the positive control that keeps it from going vacuous again (:44, :47), the trigger test still pins the exact first warning at verdicts[30] rather than asserting mere presence (:31-32), and no assertion depends on a floating-point margin narrower than 0.1 Mbps; nothing was removed or weakened, and the two files sit alongside the existing src/tests/libs/ suite)

10. Documentation — ✅ (nothing added behaves differently between Lite and Standalone — no Electron-only API is reached and both builds run the same poll and the same snackbar — so the README.md parity table needs no row; every new export carries JSDoc, the newly exported IpInfo sits inside the pre-existing jsdoc/require-jsdoc disable region opened at src/libs/blueos.ts:184, counterDeltaToMbps's block states both guards and the counter-refresh reasoning (:350-357), and the @returns at src/libs/wireless-traffic-warning.ts:14 still matches the rule :44-53 implements)

11. Nitpicks / Optional — ✅ (re-read the four comments in the watcher against the code beneath them, including the one rewritten this round at :35-38, and each now states what the constant below it actually enforces; the only overstatement left is the measure-zero cadence of exactly 40/3 s noted in the since-last-round block, which no real poll hits)

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

rafaellehmkuhl · 2026-08-25T18:22:09Z

@ArturoManzoli ready again.

rafaellehmkuhl assigned rafaellehmkuhl and unassigned rafaellehmkuhl Aug 19, 2026

rafaellehmkuhl force-pushed the issue-2953-warn-wireless-video-traffic branch from 7ecce51 to 913ebf7 Compare August 19, 2026 19:38

rafaellehmkuhl force-pushed the issue-2953-warn-wireless-video-traffic branch from 913ebf7 to da98bff Compare August 19, 2026 20:03

rafaellehmkuhl force-pushed the issue-2953-warn-wireless-video-traffic branch 2 times, most recently from 37c79e1 to a078722 Compare August 19, 2026 20:39

rafaellehmkuhl requested a review from ArturoManzoli August 19, 2026 21:26

rafaellehmkuhl force-pushed the issue-2953-warn-wireless-video-traffic branch from a078722 to 1912bc2 Compare August 25, 2026 13:32

rafaellehmkuhl mentioned this pull request Aug 25, 2026

Vehicle network upload/download speed variables read as zeros and spikes instead of the real rate #2971

Open

rafaellehmkuhl force-pushed the issue-2953-warn-wireless-video-traffic branch from 1912bc2 to 75710ec Compare August 25, 2026 13:52

rafaellehmkuhl force-pushed the issue-2953-warn-wireless-video-traffic branch from 5ee4e9b to 883150a Compare August 25, 2026 16:35

rafaellehmkuhl force-pushed the issue-2953-warn-wireless-video-traffic branch from 883150a to e948c28 Compare August 25, 2026 17:44

rafaellehmkuhl added 2 commits August 25, 2026 15:05

rafaellehmkuhl force-pushed the issue-2953-warn-wireless-video-traffic branch from e948c28 to 0279776 Compare August 25, 2026 18:05

Uh oh!

Conversation

rafaellehmkuhl commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

The plotted network speeds, fixed in the second commit

Test plan

Checks

Uh oh!

github-actions Bot commented Aug 19, 2026

Automated PR Review — round 1

What still needs attention

Uh oh!

rafaellehmkuhl commented Aug 19, 2026

Review follow-up — round 1

Done

Done differently

Won't change (with reasoning)

Questions for reviewers

Uh oh!

rafaellehmkuhl commented Aug 19, 2026

Uh oh!

github-actions Bot commented Aug 19, 2026

Automated PR Review — round 2

What still needs attention

Uh oh!

rafaellehmkuhl commented Aug 19, 2026

Review follow-up — round 2

Done

Done differently

Won't change (with reasoning)

Uh oh!

rafaellehmkuhl commented Aug 19, 2026

Uh oh!

github-actions Bot commented Aug 19, 2026

Automated PR Review — round 3

What still needs attention

Uh oh!

rafaellehmkuhl commented Aug 19, 2026

Review follow-up — round 3

Done

Done differently

Uh oh!

rafaellehmkuhl commented Aug 19, 2026

Uh oh!

github-actions Bot commented Aug 19, 2026

Automated PR Review — round 4

What still needs attention

Uh oh!

ArturoManzoli commented Aug 20, 2026

Uh oh!

rafaellehmkuhl commented Aug 20, 2026

Uh oh!

ArturoManzoli commented Aug 21, 2026

Uh oh!

rafaellehmkuhl commented Aug 21, 2026

Uh oh!

ArturoManzoli commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafaellehmkuhl commented Aug 25, 2026

Uh oh!

rafaellehmkuhl commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

What still needs attention

Uh oh!

rafaellehmkuhl commented Aug 25, 2026

Done

Deferred

Notes

Uh oh!

rafaellehmkuhl commented Aug 25, 2026

Uh oh!

github-actions Bot commented Aug 25, 2026

What still needs attention

Uh oh!

github-actions Bot commented Aug 25, 2026

🙋 Decision needed — 1.6

Uh oh!

rafaellehmkuhl commented Aug 19, 2026 •

edited

Loading

ArturoManzoli commented Aug 21, 2026 •

edited

Loading