fix: stop multiple-joystick warning from repeatedly stealing control - #2799
fix: stop multiple-joystick warning from repeatedly stealing control#2799rafaellehmkuhl wants to merge 2 commits into
Conversation
Automated PR Review (Claude)0. SummaryVerdict: READY TO MERGE This PR fixes a bug where the "Multiple joystick controllers detected" warning dialog kept re-appearing and repeatedly disabling forwarding on every joystick reconnection event, effectively stealing control from users. The fix adds three guard conditions — forwarding already active, forwarding already prevented, or a check already in flight — so the conflict detection runs at most once. The old 1. Correctness & Implementation Bugs — ✅2. AGENTS.md Adherence — ✅3. Security — ✅4. Performance — ✅5. UI / UX — ✅6. Code Quality & Style — ✅7. Commit Hygiene — ✅8. Tests — ✅9. Documentation — ✅10. Nitpicks / Optional — ✅Generated by Claude. This is advisory; a human reviewer must still approve. |
8237e6d to
0344076
Compare
Review follow-up — round 1Done
No review feedback was addressed in this round; the push is the rebase only. |
|
/review |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.1 | Conflict check skipped entirely for the first joystick | If joystick output happens to already be switched on when you plug your controller in — which happens just by visiting the joystick settings page, or in the browser version by switching tabs and back — Cockpit never checks whether another ground station is already driving the vehicle, so two stations can fight for control with no warning at all. | major | ❌ |
| 1.2 | Guards checked before a wait that has no time limit | If you switch joystick commands back on while Cockpit is still waiting for the vehicle to appear, the pending check can finish much later and switch them off again, so control disappears with nothing the user did to explain it. | minor | ❌ |
Since round 1 — 0 closed, comparing 8237e6d → 0344076
Range. incremental.diff is not usable this round. It lists 281 changed files across the whole tree (.eslintrc.cjs, .github/, src/views/, src/libs/, …) while the PR itself touches one file, which is exactly what a rebase onto a moved base produces; the entry for the file this PR actually changes reads === src/stores/controller.ts (modified, +0/-0) === [binary or no textual patch]. Every status judgement below therefore comes from pr.diff against the current base, not from the increment.
Findings. previous-ledger.json is [] and previous-review.md recorded no findings, so there is nothing to re-judge and no status transition to report. Both findings in this round are new, and both describe code that was already present at 8237e6d — round 1 missed them rather than the author introducing them since.
Resolutions and votes. resolutions.json and decisions.json are both []: no /resolve has been issued on this PR and no dispute has ever gone to a vote, so nothing was applied and no id went unmatched.
Discussion. @rafaellehmkuhl posted a follow-up describing this push as "rebased the branch onto current master (262a7970)… the single commit's patch is byte-identical to what was there before". Checked rather than taken: pr.json carries a single commit 0344076 authored 2026-06-19 and committed 2026-08-28 (the rebase), pr.diff touches only src/stores/controller.ts, and the increment shows that file with no textual change between the two heads. The claim holds. The /review comment itself is a command and carries no content. Nothing in the PR body, the diff, the comments or the complexity report contained text addressed to this reviewer.
Change map — what was established before judging
Claims (from the PR body and commit message, each checked against the code):
- Symptom — "the dialog kept popping up and forwarding kept being disabled". Verified. At the base,
src/stores/controller.ts:215-218setsenableForwarding.value = falsewhenever the last joystick disconnects, and the only skip guard wasthereWereJoysticksBefore && enableForwarding.value(:177). After a conflict was detected, both conjuncts were false on the next connect, so the check re-ran, re-disabled forwarding and re-showed the dialog on every reconnection. - Cause — "the check ran inside
processJoystickConnectionEvent, which fires on every joystick connect/disconnect". Verified.src/libs/joystick/manager.ts:436-472pollsnavigator.getGamepads()every 500 ms and emits only when the connected set changes, so a flapping Bluetooth pad produces one event per flap. - Mechanism — "run the check only when forwarding is neither already active nor explicitly prevented". Implemented, but the first term is not equivalent to the intended rule. See finding 1.1; the added comment states the intent ("only meaningful for the first joystick that takes control") that the condition does not encode.
- "The async check can wait up to ~3s". Contradicted. The wait is unbounded:
mainVehicleStore.getVehicleAddress()(src/stores/mainVehicle.ts:621-629) loops on a 1 s timer until a global address exists and never times out or rejects; the 3 s is only the settle-time sleep insidecheckForOtherManualControlSources(src/libs/blueos.ts:443-448), before four sequential fetches. This underpins finding 1.2. - "Drop the old early
return… that logic now runs viacontinue". Verified and correct. The basereturnat:179skipped the disconnect-cleanup loop (:210-219) and thecurrentMainJoystickrefresh (:222-244);continuerestores both.
Failure site. src/stores/controller.ts:177-206 at the base — the guard and the conflict check itself. It is in the diff; the fix is at the site of the defect, not at a call site downstream of it.
Entry points.
| Function | Reached from | Frequency |
|---|---|---|
processJoystickConnectionEvent (src/stores/controller.ts:163) |
joystickManager.onJoystickConnectionUpdate at :160 ← pollGamepadsConnections 500 ms poll, emitting only on set changes (src/libs/joystick/manager.ts:436-472) |
per user action (plug/unplug), plus one per flap when a wireless pad roams |
checkForOtherManualControlSources (src/libs/blueos.ts:441) |
only from the loop above (the store also re-exports it at src/stores/controller.ts:523, with no other in-tree caller) |
per user action; at most once per session after this PR |
getVehicleAddress (src/stores/mainVehicle.ts:621) |
the same loop, plus unrelated callers | same, and blocks indefinitely until a vehicle address exists |
enableJoystickForwardingIfSafe (src/stores/controller.ts:443) |
unchanged by this PR, but it is one of the two writers that break the invariant below — document-visibility watcher at :249-262 |
per tab focus change (web build only) |
Invariants. The new guard relies on enableForwarding === true meaning "a joystick already has control, so the conflict check has already had its chance". Sites that can set that ref true without any joystick being connected: src/views/ConfigurationJoystickView.vue:929-931 (onUnmounted, unconditional) and src/stores/controller.ts:249-262 → :443-449 (visibility watcher, web build). Neither is covered by the PR. src/components/mini-widgets/JoystickCommIndicator.vue:93-96 cannot break it — its switch is :disabled="!joystickConnected" (:36). The single chokepoint that would hold the invariant is a flag owned by the check itself rather than the shared forwarding ref; see finding 1.1.
1. Correctness & Implementation Bugs — 2 findings
1.1 — major — Skipping on enableForwarding alone drops the conflict check for the first joystick
src/stores/controller.ts, the added guard (pr.diff line 34):
if (enableForwarding.value || preventJoystickForwarding.value || checkingForOtherManualControlSources) {
continue
}The base condition was thereWereJoysticksBefore && enableForwarding.value (src/stores/controller.ts:177). Dropping the thereWereJoysticksBefore conjunct widens the skip from "another joystick already has control" to "forwarding is on for any reason whatsoever", and two ordinary paths leave enableForwarding === true with zero joysticks connected:
src/views/ConfigurationJoystickView.vue:929-931setscontrollerStore.enableForwarding = trueinonUnmounted, unconditionally and without going throughenableJoystickForwardingIfSafe. The page's own guard watcher (:710-713) belongs to the component's effect scope, which Vue stops before theunmountedhooks run, so it does not undo it. Open the joystick settings page with nothing plugged in, leave it, then plug the joystick in.src/stores/controller.ts:249-262— onvisible, the document-visibility watcher callsenableJoystickForwardingIfSafe(), which setsenableForwarding.value = true(:443-449) with no joystick check. In the Lite (web) build, switching browser tabs away and back before plugging the joystick in is enough. (isElectron()short-circuits this one, so Standalone is only exposed through path 1.)
In both cases the first joystick connects, the loop continues, and checkForOtherManualControlSources is never called for the rest of the session — nothing else in the tree calls it. The safety check that exists to stop two ground stations driving one vehicle silently never runs, and the user gets no dialog. The comment added directly above the guard states the rule the code was meant to encode — "The conflict check is only meaningful for the first joystick that takes control" — which is precisely what the condition does not express.
Note that preventJoystickForwarding.value on its own already closes the reported loop: that ref is set at :189 the moment a conflict is detected and is never cleared anywhere in the tree, so every later reconnection skips the check. The enableForwarding.value term is not what fixes the reported bug; it is only the "already forwarding, don't re-check" case the base handled with the narrower conjunct.
Fix: key the skip on whether the check has already run, not on a ref other components write freely. Either restore the conjunct —
if ((thereWereJoysticksBefore && enableForwarding.value) || preventJoystickForwarding.value || checkingForOtherManualControlSources) continue— or drop the enableForwarding term and set a hasCheckedForOtherManualControlSources flag in the existing finally, which states the intent directly and survives whatever else toggles forwarding.
Consequence: Cockpit can start forwarding a second joystick to a vehicle another ground station is already driving, without ever showing the warning that exists for exactly that situation.
1.2 — minor — The new guards are read before an unbounded wait and never re-checked after it
The guard is evaluated, then the body awaits mainVehicleStore.getVehicleAddress() and checkForOtherManualControlSources(vehicleAddress), and applies the result without re-reading enableForwarding or preventJoystickForwarding (pr.diff lines 62-90).
That window is not the "~3s" the PR body assumes. getVehicleAddress (src/stores/mainVehicle.ts:621-629) polls on a 1 s timer until globalAddress is defined and has no timeout and no rejection path; checkForOtherManualControlSources then sleeps up to 3 s more (src/libs/blueos.ts:443-448) before four sequential fetches. Starting Cockpit with the joystick already plugged in but the vehicle not yet reachable — an everyday sequence — parks the check there for as long as the vehicle takes to appear.
During that wait the user sees "Joystick connected but disabled" (src/components/mini-widgets/JoystickCommIndicator.vue:84) and can switch forwarding on from the mini-widget (:93-96). When the vehicle finally connects, the still-pending check resolves and, if it detects another source, sets enableForwarding = false and opens the dialog — overriding a deliberate user action taken possibly minutes earlier, which is the same "control is taken away" complaint this PR is fixing, just on a narrower path.
Fix: re-read the two refs inside the try after the awaits and bail out before acting if either became true while the check was in flight.
Consequence: a user who re-enables joystick commands while Cockpit is still waiting for the vehicle can have them switched off again long afterwards, with nothing they did to connect it to.
Sections with nothing to report (10)
2. Persistence & User Data — ✅ (no persisted key added, reshaped or removed: enableForwarding and preventJoystickForwarding are plain refs at src/stores/controller.ts:67-68, the new checkingForOtherManualControlSources is a non-reactive let in the store setup, and the file's useBlueOsStorage keys are untouched by the diff)
3. AGENTS.md Adherence — ✅ (diff confined to the lines it changes — no rename, import/hook reorder, const/let swap or formatter reflow; the two added comments explain why rather than what; no new dependency, no window.electronAPI/electron-* use, no widget Options entry, and no added export without a call site)
4. Security — ✅ (no new dependency, env var, secret, eval/v-html, encoded blob or workflow change; the only network traffic on this path is the pre-existing MAVLink2REST fetch at src/libs/blueos.ts:458, unchanged)
5. Performance — ✅ (the one changed handler traces to the 500 ms connection poll at src/libs/joystick/manager.ts:436, which emits only when the connected set changes; the PR strictly reduces how often the 3 s check and its four fetches run, and registers no listener, watcher, interval or subscription needing teardown)
6. UI / UX — ✅ (no new dialog, control or copy — the warning dialog at src/stores/controller.ts:191-202 is moved into the try unchanged; the disabled-forwarding state stays visible through the mini-widget's icon, colour and tooltip at src/components/mini-widgets/JoystickCommIndicator.vue:76-91, and no user interaction was added that would need logUserAction)
7. Code Quality & Style — ✅ (per complexity-report.json, which is the PR's own CI measurement: processJoystickConnectionEvent scores 13, up from 12, with depth 3 up from 2, tripping pushed-above-12 and nothing else — the PR's own contribution there is one guard clause plus a try/finally, which is flat and independent, so the trigger is answered; 1 changed file, 49 functions measured, not truncated. No any, no scoped CSS, no wrapped inline-expression string, no comment deleted or reworded over unchanged code)
8. Commit Hygiene — ✅ (one commit for one logical change, ~65 lines; the fix: prefix matches both the change and the conventional/scope styles already in git log; the body says why; no #N reference, no closing keyword, no wip/fixup!, no AI co-author trailer, and the rebase left no commits replicated from master)
9. Tests — ✅ (no test file touched, and no existing assertion weakened or removed by the diff)
10. Documentation — ✅ (no Lite/Standalone capability difference introduced, so the README parity table is unaffected; no exported signature changed, and checkForOtherManualControlSources' JSDoc at src/libs/blueos.ts:436-440 still matches its behaviour)
11. Nitpicks / Optional — ✅ (nothing worth raising beyond the two findings above)
Generated by Claude. This is advisory; a human reviewer must still approve.
0344076 to
11d3e10
Compare
Review follow-up — round 2Done
Done differently
|
|
/review |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.3 | Joystick output never comes back after a reconnect | Unplug and replug your controller — or let a wireless one drop out for a moment — and Cockpit stops sending your stick movements to the vehicle until you spot the greyed-out icon and switch them back on by hand. | major | ❌ |
| 1.4 | A detected conflict can be discarded in silence | If you switch joystick output on while Cockpit is still waiting for the vehicle, and another ground station really is driving that vehicle, you are never told — the warning is dropped for the rest of the session. | major | ❌ |
Since round 2 — 2 closed, comparing 0344076 → 11d3e10
Range. incremental.diff cannot be used as an increment this round: its contents are byte-for-byte the full pr.diff (same two hunks, same +14/-7), which is what a compare produces when the previous head is no longer in the branch's history. pr.json shows one commit, 11d3e10, authored 2026-06-19 and committed 2026-08-28T17:10:47Z — after round 2 was posted at 16:58:58Z — so 0344076 was amended away rather than built on. Every status judgement below therefore comes from pr.diff at the current base, compared against the code round 2 quoted in its own findings.
Findings.
- 1.1 — ✅ Addressed. Round 2 asked for the skip to be keyed on whether the check has already run rather than on
enableForwarding, a ref other components write freely. TheenableForwardingterm is gone from the guard, which is nowif (checkedForOtherManualControlSources) continue(headsrc/stores/controller.ts:181) against aletowned by the check (:71). Both paths the finding named —ConfigurationJoystickView.vue:929-931and the visibility watcher atsrc/stores/controller.ts:256-269→:450-456— can still set the ref true with no joystick connected, and neither suppresses the check any more. - 1.2 — ✅ Addressed. The finding asked for the two refs to be re-read after the unbounded await and for the result to be dropped if either became true in flight.
enableForwardingis snapshotted at:185and compared at:191, so a user who re-enables forwarding during the wait no longer has it switched off minutes later. ThepreventJoystickForwardinghalf is moot rather than skipped: greppingsrc/shows its only writer is:196, inside the block that is now latched, so it cannot change during the wait. - 1.3 and 1.4 are new, and both are properties of the code this round introduced — neither existed at
0344076, where the guard still containedenableForwarding.valueand there was no post-await bail.
Resolutions and votes. resolutions.json and decisions.json are both []: no /resolve has been issued on this PR, no id went unmatched, and no dispute has ever gone to a vote.
Discussion. @rafaellehmkuhl posted a round-2 follow-up (#issuecomment-5455481087) describing the rewrite. Its load-bearing claims were checked rather than taken: "neither getVehicleAddress() nor checkForOtherManualControlSources() has a rejection path" holds — the former loops on await new Promise((r) => setTimeout(r, 1000)) with no throw (src/stores/mainVehicle.ts:621-629), the latter wraps its whole body in try/catch and returns false (src/libs/blueos.ts:441-493) — so dropping the try/finally along with the latch is sound. "Nothing outside this block writes preventJoystickForwarding" holds (single writer, head src/stores/controller.ts:196). The PR body was indeed rewritten: the "~3s" claim round 2 contradicted is gone, replaced by "waits for the vehicle with no timeout", which matches the code, and the settings-page test-plan line was added. The follow-up's reasoning for the one-directional snapshot ("bailing on its current value would have silently reinstated 1.1") is correct as far as it goes, but it is what leaves 1.4(b) open. The /review comment is a bare command and carries no content.
Injection check. Nothing in the PR body, the diff, the two new comments or complexity-report.json contained text addressed to this reviewer. The follow-up comment carries HTML markup (<details>), reproduced here as prose only.
Change map — what was established before judging
Line numbers for src/stores/controller.ts are head line numbers (the checkout is the base, which runs three lines behind from :69 and seven behind from :186); every other file is unchanged by this PR, so base and head agree.
Claims (from the PR body and commit message, each checked against the code):
- Symptom — "the dialog kept popping up and forwarding kept being disabled". Verified. The last-joystick-disconnect handler sets
enableForwarding.value = false(:224), and the only skip guard at the base wasthereWereJoysticksBefore && enableForwarding.value. After a conflict was detected both conjuncts were false on the next connect, so the check re-ran, re-disabled forwarding and re-showed the dialog on every reconnection. - Cause — "the check ran inside
processJoystickConnectionEvent, which fires on every joystick connect/disconnect". Verified.src/libs/joystick/manager.ts:436-482re-pollsnavigator.getGamepads()on a 500 mssetTimeoutand emits only when the connected set changes (:475-477), so a flapping wireless pad produces one event per flap. - Mechanism — "latch the check on a
checkedForOtherManualControlSourcesflag owned by the check itself, set before the awaits". Verified as written (:71,:181-182), and it does close what round 2 raised as 1.1. The samecontinue, however, also skips the branch that turns forwarding on — see 1.3. - "The flag doubles as the in-flight guard so concurrent connection events can't race to show the dialog twice". Verified. The handler is fire-and-forget (
:163,(event) => processJoystickConnectionEvent(event)), but the latch is assigned synchronously before the firstawait, so any later event entering the loop sees it set. - "Ignore the check's result if the user enabled forwarding by hand while it was still running". Implemented, but wider and narrower than stated at once: wider because it discards the warning too, narrower because it only fires on a false→true transition. See 1.4.
- "
getVehicleAddress()waits for the vehicle with no timeout, so the check can resolve minutes later". Verified (src/stores/mainVehicle.ts:621-629), and this correctly replaces the "~3s" claim round 2 contradicted; the 3 s is only the settle sleep atsrc/libs/blueos.ts:443-448. - "Drop the old early
return… that logic now runs viacontinue". Verified. The basereturnskipped the disconnect-cleanup loop and thecurrentMainJoystickrefresh;continuerestores both.
Failure site. The guard and the conflict check inside processJoystickConnectionEvent itself (base src/stores/controller.ts:177-206). It is in the diff — the fix is at the site of the defect, not at a call site downstream of it.
Entry points.
| Function | Reached from | Frequency |
|---|---|---|
processJoystickConnectionEvent (head src/stores/controller.ts:166) — the only function the diff changes |
joystickManager.onJoystickConnectionUpdate (:163) ← pollGamepadsConnections 500 ms re-poll, emitting only on set changes (src/libs/joystick/manager.ts:436-482) |
per user action (plug/unplug), plus one per flap when a wireless pad roams |
checkForOtherManualControlSources (src/libs/blueos.ts:441) |
only that loop; the store re-exports it, but no other in-tree caller exists | at most once per session after this PR |
getVehicleAddress (src/stores/mainVehicle.ts:621) |
the same loop, plus unrelated callers | same, and blocks indefinitely until a vehicle address exists |
enableJoystickForwardingIfSafe (head src/stores/controller.ts:450) |
unchanged by the PR; the document-visibility watcher at :256-269, which returns early on isElectron() (:260) |
per tab focus change, web build only |
Invariants. The change relies on "the conflict check needs to run at most once per session, and its first outcome stays valid for the rest of it". Sites that can invalidate it:
- The disconnect handler at
:224setsenableForwardingfalse after the check has already run, and nothing re-runs the enable path — not covered, this is 1.3. ConfigurationJoystickView.vue:929-931(onUnmounted) and:704/:710-713(forced false while that page is open) flip the ref underneath an in-flight check with no user gesture at all — not covered, this is 1.4(b) and part of 1.4(a)'s reachability.globalAddresscan change mid-session (the vehicle address setting), so one session can span two vehicles while the latch never resets — not covered, but only reachable through a joystick reconnect, so it is folded into 1.3's fix rather than raised separately.- The two writers that used to suppress the check (round 2's 1.1) are now covered, because the latch no longer reads
enableForwardingat all.
1. Correctness & Implementation Bugs — 2 findings
1.3 — major — The one-shot latch also latches the branch that re-enables forwarding
Head src/stores/controller.ts:181-182:
if (checkedForOtherManualControlSources) continue
checkedForOtherManualControlSources = trueThat continue skips everything below it in the iteration, including the else branch at :210-213, which is the only code in the tree that turns forwarding on when a joystick connects:
} else {
console.info('No other sources of joystick commands detected. Enabling joystick forwarding.')
enableForwarding.value = true
}The disconnect loop below it is not latched, and still clears the ref every time the last joystick goes away (:217-226, enableForwarding.value = false at :224). So the ordinary sequence is:
- Plug the pad in, no other GCS present → the check runs, the latch is set, forwarding is on.
- Unplug it, or let a wireless pad drop out for one 500 ms poll window → forwarding off.
- Plug it back in → the loop
continues at:181and nothing turns forwarding back on.
In Standalone the only routes back are the mini-widget switch (src/components/mini-widgets/JoystickCommIndicator.vue:31-38) and the joystick settings page's onUnmounted (src/views/ConfigurationJoystickView.vue:929-931); the visibility watcher that would otherwise recover it returns early on isElectron() (src/stores/controller.ts:260). All the user gets is the yellow gamepad glyph and the "Joystick connected but disabled" tooltip (JoystickCommIndicator.vue:78,84) — no dialog, no snackbar, nothing that connects the dead sticks to the replug.
This is new this round: at 0344076 the guard still contained enableForwarding.value, which was false after a disconnect, so the check re-ran on reconnect and re-enabled forwarding. It also contradicts the PR's own test-plan line "Re-enable forwarding via the top-right joystick widget → stays enabled across reconnects" — :224 clears the ref on the disconnect and the latch stops the reconnect from restoring it.
Fix: latch the check, not the enable. When the latch is already set, fall through to enableJoystickForwardingIfSafe() (:450-456) instead of continueing past it — that helper already refuses when preventJoystickForwarding is set, so a session in which a conflict was found stays disabled, while a clean one gets the joystick back on reconnect without re-running the 3 s settle and four fetches.
Consequence: after unplugging and replugging a controller, Cockpit silently stops sending stick input to the vehicle until the user notices and switches it back on by hand.
1.4 — major — The post-await bail discards the warning, and guards only one direction
Head src/stores/controller.ts:185-191:
const forwardingWasEnabled = enableForwarding.value
const vehicleAddress = await mainVehicleStore.getVehicleAddress()
const otherSourceDetected = await checkForOtherManualControlSources(vehicleAddress)
if (!forwardingWasEnabled && enableForwarding.value) continueTwo problems in that one line, both riding the unbounded window the PR body itself describes (getVehicleAddress polls a 1 s timer forever, src/stores/mainVehicle.ts:621-629):
- (a) It drops the warning, not just the override. When
otherSourceDetectedis true, thecontinueskipsenableForwarding.value = false,preventJoystickForwarding.value = trueand the dialog at:198-209. Because the latch at:182is already set, the check never runs again, so the operator is never told another ground station is driving this vehicle — andpreventJoystickForwardingnever becomes true, soenableJoystickForwardingIfSafe(:450-456) keeps enabling forwarding on every tab focus in the web build. Reaching it needs no unusual behaviour: during the wait the pad is already injoysticks.value, so the mini-widget switch is live (JoystickCommIndicator.vue:36,93-96) showing "Joystick commands paused", and clicking it is the natural response. It does not even need a click — open the joystick settings page with the pad plugged in and navigate away, andConfigurationJoystickView.vue:929-931performs the false→true transition for you. The dialog's copy ("you can also disable the joystick forwarding on the other Cockpit instance") is written to be acted on, which is exactly what suppressing it costs the user. - (b) The snapshot is one-directional. It only fires on false→true. Now that 1.1 is fixed,
forwardingWasEnabledis legitimately true on the settings-page and tab-switch paths — and if the user then pauses forwarding from the mini-widget during the wait, or the tab-hidden watcher clears it (:264, web build),!forwardingWasEnabledis false, the guard does nothing, and the no-conflict branch at:210-213setsenableForwarding.value = true, resuming stick input the user deliberately paused, possibly minutes earlier. That is round 2's 1.2 complaint mirrored.
Fix, covering both: compare against the snapshot in both directions and narrow what the bail suppresses — e.g. const userMovedTheSwitch = enableForwarding.value !== forwardingWasEnabled, apply the enableForwarding writes only when that is false, and open the dialog whenever otherSourceDetected is true regardless. Decide preventJoystickForwarding explicitly while you are there: leaving it false keeps the user's choice honoured on later tab focuses, which is probably what you want, but it should be a stated decision rather than a side effect of the continue.
Consequence: Cockpit can stay silent about a second ground station already driving the vehicle, or switch joystick output back on after the user paused it.
Sections with nothing to report (10)
2. Persistence & User Data — ✅ (nothing persisted is touched: enableForwarding and preventJoystickForwarding are plain refs at head src/stores/controller.ts:67-68, the added checkedForOtherManualControlSources is a non-reactive let inside the store setup, and every useBlueOsStorage key in the file falls outside the two hunks)
3. AGENTS.md Adherence — ✅ (scope held to the lines being changed — no rename, import/hook reorder, const/let swap or formatter reflow; the three added comments explain why, and no comment over unchanged code was reworded or deleted; no new dependency, no Electron-only API, no widget Options entry, no export added without a call site; the (issue #2798) in the new comment is not the violation it resembles, since that rule scopes issue references to commit messages and the commit body carries none)
4. Security — ✅ (no dependency, env var, secret, eval/v-html, encoded blob, workflow or Electron main-process change; the only network traffic on this path is the pre-existing MAVLink2REST fetch at src/libs/blueos.ts:458-459, untouched)
5. Performance — ✅ (the one changed function traces to the 500 ms pollGamepadsConnections timer at src/libs/joystick/manager.ts:436-482, which emits only on set changes; the diff strictly reduces how often the 3 s settle plus four sequential fetches run, and registers no listener, watcher, interval or subscription needing teardown)
6. UI / UX — ✅ (no control, dialog or copy added — the warning dialog at head src/stores/controller.ts:198-209 sits outside the hunks and is unchanged, the only interactive surface on this path already logs through logUserAction at JoystickCommIndicator.vue:94, and the two user-visible consequences of the new logic are filed as 1.3 and 1.4 rather than counted twice here)
7. Code Quality & Style — ✅ (per complexity-report.json, CI's own measurement for this head: processJoystickConnectionEvent scores 13, up from 12, tripping pushed-above-12 alone, with depth 2 equal to baseDepth — the PR's own contribution is two flat guard clauses and one local, so the trigger is answered; 1 changed file, 49 functions measured, not truncated. No any, no scoped CSS, no wrapped inline-expression string, let is required since the latch is reassigned, and both added comment blocks sit inside the 180-char max-len)
8. Commit Hygiene — ✅ (one commit for one logical change, +14/-7; the fix: prefix fits the change and matches the styles already in git log; the body says why and carries no #N, no closing keyword, no wip/fixup! and no AI co-author trailer — round 2's feedback was folded into the original commit by amend rather than added as an "address review" commit, which is what AGENTS.md asks for)
9. Tests — ✅ (the diff touches no test file and weakens no assertion; nothing under src/tests/ references stores/controller or checkForOtherManualControlSources, so no existing coverage is affected either way)
10. Documentation — ✅ (no Lite/Standalone capability difference is introduced, so the README parity table is unaffected; no exported signature changed, and the JSDoc on checkForOtherManualControlSources at src/libs/blueos.ts:436-440 still matches its behaviour)
11. Nitpicks / Optional — ✅ (nothing beyond the two findings above; the added comments run longer than the one-sentence target in AGENTS.md, but each earns its length — one records why the latch is safe to set before the awaits, which is the non-obvious part)
Generated by Claude. This is advisory; a human reviewer must still approve.
11d3e10 to
205d5b0
Compare
Review follow-up — round 3Done
Done differently
|
|
/review |
Review follow-up — round 3Done
Done differently
Won't change (with reasoning)
|
|
/review |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.5 | An internal toggle is mistaken for the user flipping the switch | Cockpit can keep sending your stick input to a vehicle another ground station is already driving, or leave your connected controller silent, because one of its own internal switches moved while it was waiting for the vehicle. | major | ❌ |
| 1.6 | The description says the opposite of what the code does | Anyone reading this pull request will expect a manual override to stick, when in practice it disappears at the next tab switch or controller reconnect, with nothing on screen to say so. | minor | ❌ |
Since round 3 — 4 closed, comparing 11d3e10 → 205d5b0
Range. incremental.diff is again unusable as an increment: its contents reproduce the whole of pr.diff — the same three hunks, and +23/-9, which is exactly the total pr.json reports for the PR — which is what a compare produces when the previous head is no longer in the branch's history. pr.json lists a single commit, 205d5b0, authored 2026-06-19 and committed 2026-08-28T17:33:48Z; 11d3e10 is not in the list, so round 3's head was amended away rather than built on. Every status judgement below therefore comes from pr.diff against the current base, compared against the code round 3 quoted in its own findings. Head line numbers below run +3 ahead of the checkout from :69 and +14 ahead from :187.
Findings.
- 1.3 — ✅ Addressed. The finding asked for the latch to cover the check and not the enable, naming the fix explicitly: "when the latch is already set, fall through to
enableJoystickForwardingIfSafe()instead ofcontinueing past it". Headsrc/stores/controller.ts:181-184does that literally —if (otherSourcesCheck === 'done') { enableJoystickForwardingIfSafe(); continue }. Both halves of what the finding asked for landed: the unplug/replug sequence now gets forwarding back (:457-463sets the ref true), and a session where a conflict was found stays disabled, because that helper returns early whilepreventJoystickForwardingis set. The test-plan line the finding said the code contradicted ("stays enabled across reconnects") now holds. - 1.4 — ✅ Addressed, in both parts. (a)
showDialogat head:205-216is no longer inside anything conditional on the snapshot — a detected conflict is always reported. (b) the one-directional!forwardingWasEnabled && enableForwarding.valueis gone, replaced at head:198byconst userMovedTheSwitch = enableForwarding.value !== forwardingWasEnabled, which is the exact expression round 3's fix text proposed, with the two writes gated on it at:203and:217. The finding's third ask — decidepreventJoystickForwardingexplicitly rather than leaving it a side effect of thecontinue— is also satisfied in the code: head:195-197states the decision. It states the opposite decision from the one the PR body describes, which is 1.6, not a reopening of 1.4. - 1.5 is new and is the residue of the shape 1.4's own fix text named. Round 3 proposed comparing the ref against its snapshot; that comparison cannot distinguish a user gesture from the six non-user writers of the same ref, and the misattribution now decides a safety-relevant branch rather than just a suppressed dialog. The hole is in last round's suggestion, not in the author's reading of it.
- 1.6 is new, and is a mismatch between the PR text and the diff rather than a change in behaviour:
preventJoystickForwarding.value = trueat head:202is unchanged context, sitting above the guard the PR added on the next line.
Resolutions and votes. resolutions.json and decisions.json are both []: no /resolve has ever been issued on this PR, so no id went unmatched and nothing was closed by a maintainer this round, and no dispute has ever gone to a vote.
Discussion. @rafaellehmkuhl posted a round-3 follow-up (#issuecomment-5455705053) listing the fixes. Its claims were checked rather than taken:
- "the loop now calls
enableJoystickForwardingIfSafe()beforecontinueing" — holds, head:181-184. - "
showDialognow runs wheneverotherSourceDetectedis true; only theenableForwarding/preventJoystickForwardingwrites are conditional" — half holds. The dialog is unconditional, and theenableForwardingwrites are conditional, but thepreventJoystickForwardingwrite is not. That is 1.6. - "left false when the user moved the switch, as suggested … Stated in the comment at
:195-197" — contradicted by the code. The comment at:195-197states the opposite of this sentence, and the code follows the comment. - "the latch had to become a three-state … Recovery is now gated on
'done', and'running'keeps the in-flight guard" — holds, and the reasoning is sound: with a boolean, a second connection event arriving mid-check would have taken the recovery path, enabled forwarding, and had that read back asuserMovedTheSwitch. Head:186-187closes that, andotherSourcesCheck = 'running'is assigned synchronously before the firstawaitat:191, so a concurrent fire-and-forget invocation (:163) sees it set. - "Commit body and PR body reworded to match" — the commit body and the PR body were indeed rewritten, and the test plan gained the conflict-during-manual-enable line. The rewrite is what introduced the
preventJoystickForwardingclaim that 1.6 flags.
The /review comment is a bare command and carries no content.
Injection check. Nothing in the PR body, the diff, the two new comments or complexity-report.json contained text addressed to this reviewer. The follow-up comment carries HTML markup (<details>), reproduced here as prose only.
Change map — what was established before judging
Line numbers for src/stores/controller.ts are head numbers (the checkout is the base, which runs 3 lines behind from :69 and 14 behind from :187); every other file is unchanged by this PR, so base and head agree.
Claims (from the PR body and commit message, each checked against the code):
- Symptom — "the dialog kept popping up and forwarding kept being disabled". Verified. The disconnect loop clears the ref whenever the last joystick goes away (head
:231), and the only skip guard at the base wasthereWereJoysticksBefore && enableForwarding.value, so after a conflict both conjuncts were false on the next connect and the check re-ran. - Cause — "the check ran inside
processJoystickConnectionEvent, which fires on every joystick connect/disconnect". Verified.src/libs/joystick/manager.ts:436-482re-pollsnavigator.getGamepads()on a 500 mssetTimeoutand emits only when the connected set changes (:475-477), so a flapping wireless pad produces one event per flap. - "Latch the check on an
otherSourcesCheckstate owned by the check itself, moved torunningbefore the awaits … therunningstate doubles as the in-flight guard". Verified (:71,:186-187, assignment before theawaitat:191). - "Once the check is
done, a reconnection routes throughenableJoystickForwardingIfSafe()… unless the check found a conflict, which the helper honours viapreventJoystickForwarding". Verified (:181-184→:457-463). - "Keying the skip on the check rather than on
enableForwardingmatters: that ref is turned on with no joystick connected byConfigurationJoystickView'sonUnmountedhook and by the document-visibility watcher". Verified (src/views/ConfigurationJoystickView.vue:929-931; headsrc/stores/controller.ts:263-276→:457-463). The PR is right about those writers here, which is what makes 1.5 the same observation applied one step later. - "Skip the check's automatic enable/disable if the user moved the forwarding switch (in either direction) while it was still running". Implemented as a two-sample comparison of the shared ref (
:190,:198), which reports that the value differs, not that a user moved it. See 1.5. - "The warning dialog is still shown whenever another source is detected". Verified (
:205-216, outside every guard). - "
preventJoystickForwardingis deliberately left false in that case so the user's choice keeps being honoured on later tab focuses". Contradicted.:202sets it true unconditionally, above the guard added at:203. See 1.6. - "
getVehicleAddress()waits for the vehicle with no timeout". Verified (src/stores/mainVehicle.ts:621-629: awhileon a 1 ssetTimeout, no timeout, no throw). The 3 s is only the settle sleep atsrc/libs/blueos.ts:443-448. - Code comment — "Neither of the calls it awaits can reject, so there is no failure path that would have to reset this back to 'pending'". Verified.
getVehicleAddresshas no throw path, andcheckForOtherManualControlSourceswraps its whole body intry/catchand returnsfalse(src/libs/blueos.ts:441-493). The unconditionalotherSourcesCheck = 'done'at:193is therefore safe without afinally. - "Drop the old early
return… that logic now runs viacontinue". Verified;continuerestores the disconnect-cleanup loop and thecurrentMainJoystickrefresh.
Failure site. The guard and the conflict check inside processJoystickConnectionEvent (base src/stores/controller.ts:177-206, head :177-220). It is in the diff — the fix is at the site of the defect, not at a call site downstream of it.
Entry points.
| Function | Reached from | Frequency |
|---|---|---|
processJoystickConnectionEvent (head src/stores/controller.ts:166) — the only function the diff changes |
joystickManager.onJoystickConnectionUpdate (:163, fire-and-forget) ← pollGamepadsConnections 500 ms re-poll, emitting only on set changes (src/libs/joystick/manager.ts:436-482) |
per user action (plug/unplug), plus one per flap when a wireless pad roams |
enableJoystickForwardingIfSafe (head :457) — unchanged, but the diff adds a new caller at :182 |
the new 'done' branch, plus the pre-existing document-visibility watcher at :263-276, which returns early on isElectron() (:267) |
per reconnection after the first check, plus per tab focus change in the web build |
checkForOtherManualControlSources (src/libs/blueos.ts:441) |
only that loop; the store re-exports it, but no other in-tree caller exists | at most once per session after this PR |
getVehicleAddress (src/stores/mainVehicle.ts:621) |
the same loop, plus unrelated callers | same, and blocks indefinitely until a vehicle address exists |
Invariants. The round-3 invariant ("the check runs at most once per session and its first outcome stays valid") is now held at a single chokepoint, otherSourcesCheck, and the reconnect hole is closed by the 'done' branch. This round's code establishes a new one: "between :190 and :198, enableForwarding changes only because the user moved it." Every writer of that ref in src/, and whether the PR covers it:
| Writer | Direction | A user gesture? | Covered |
|---|---|---|---|
src/components/mini-widgets/JoystickCommIndicator.vue:95 (setJoystickForwarding) |
both | yes — the only one | n/a |
src/views/ConfigurationJoystickView.vue:704 (onMounted) |
→ false | no | no |
src/views/ConfigurationJoystickView.vue:711-712 (watcher, forces false while the page is open) |
→ false | no | no |
src/views/ConfigurationJoystickView.vue:929-931 (onUnmounted, unconditional) |
→ true | no | no |
head src/stores/controller.ts:271 (tab hidden, web build) |
→ false | no | no |
head src/stores/controller.ts:274 → :457-463 (tab visible, web build) |
→ true | no | no |
head src/stores/controller.ts:231 (last joystick disconnected) |
→ false | no | no |
Six of the seven are not the user, none is covered, and enableJoystickForwardingIfSafe is not a chokepoint for this — three of the six bypass it entirely. This is 1.5.
1. Correctness & Implementation Bugs — 2 findings
1.5 — major — userMovedTheSwitch cannot tell a user gesture from Cockpit's own writes to the same ref
Head src/stores/controller.ts:190-198:
const forwardingWasEnabled = enableForwarding.value
const vehicleAddress = await mainVehicleStore.getVehicleAddress()
const otherSourceDetected = await checkForOtherManualControlSources(vehicleAddress)
otherSourcesCheck = 'done'
const userMovedTheSwitch = enableForwarding.value !== forwardingWasEnabledTwo samples of a shared ref taken across a wait the PR body itself calls unbounded. The name asserts a user gesture; the expression only reports that the value differs. The Change map above enumerates every writer of enableForwarding in src/: seven of them, of which exactly one — JoystickCommIndicator.vue:93-96 — is a user action. The other six are Cockpit's own lifecycle hooks and watchers, and three of them fire without any user involvement at all.
Both branches gated on that flag can therefore be cancelled by something the user did not do:
- (a) The protective switch-off, in the false→true direction. Start Cockpit with the pad plugged in and the vehicle not yet reachable — the everyday sequence, and the one that parks the check in
getVehicleAddress(src/stores/mainVehicle.ts:621-629) for as long as the vehicle takes to appear. The snapshot at:190isfalse. Open the joystick settings page to look at your bindings and leave it:ConfigurationJoystickView.vue:929-931setscontrollerStore.enableForwarding = trueinonUnmounted, unconditionally and outsideenableJoystickForwardingIfSafe. The vehicle appears, the check resolves, another GCS is detected →userMovedTheSwitchis true,:203is skipped, and Cockpit forwards sticks into a vehicle another station is already driving. At the base, that same path turned forwarding off (base:188). The dialog does show — that is 1.4a genuinely fixed — but its copy at head:210-211, "If you still want to use this joystick, click the top-right joystick widget and enable forwarding", now describes a state the user is already in, so nothing on screen says the conflict is live and unresolved. In the web build:274→:457-463reaches the same outcome on a tab switch, with no page visit needed. - (b) The enable, in the true→false direction. Snapshot
true(the settings page'sonUnmountedran before the pad went in). The check is'running'; the pad flaps for one 500 ms poll window, so the disconnect loop at:231clears the ref; it comes back, but:186sees'running'and justcontinues. The check resolves with no conflict found →userMovedTheSwitchis true →:217skips the enable. The pad is connected and silent, with only the yellow gamepad glyph and the "Joystick connected but disabled" tooltip (JoystickCommIndicator.vue:78,84) to show for it, until the user unplugs and replugs once more and finally hits the'done'recovery path.
Fix: record the gesture where the gesture actually happens, rather than inferring it from the value afterwards. setJoystickForwarding (JoystickCommIndicator.vue:93-96) is the single user-facing writer and already funnels every entry point (it is what the v-switch at :31-38 calls) — have the store expose a setForwardingByUser action that sets enableForwarding and stamps a monotonically increasing counter or a timestamp, snapshot that around the awaits at :190/:198, and key both branches on it. That closes the invariant at its one real chokepoint instead of guarding six producers, which is what round 3's suggested expression was trying and failing to do — the hole is in that suggestion, not in the reading of it.
Consequence: Cockpit can keep sending your stick input to a vehicle another ground station is already driving, or leave your connected controller silent, because it mistook one of its own internal switches for you flipping it.
1.6 — minor — The PR body says preventJoystickForwarding is left false; the code always sets it true
Head src/stores/controller.ts:200-203:
if (otherSourceDetected) {
console.warn('Other GCS sending MANUAL_CONTROL messages detected.')
preventJoystickForwarding.value = true
if (!userMovedTheSwitch) enableForwarding.value = falsepreventJoystickForwarding.value = true is unchanged context in the diff — the new guard was added on the line below it, not around it — so it runs on every detected conflict. The PR body says the opposite: "…and preventJoystickForwarding is deliberately left false in that case so the user's choice keeps being honoured on later tab focuses." @rafaellehmkuhl's follow-up repeats it: "left false when the user moved the switch, as suggested". The code comment at head :195-197 describes what the code actually does ("a detected conflict still blocks the automatic re-enables, which the user did not ask for"), so the two accounts inside this PR contradict each other.
The behaviour that ships is the conservative one and is defensible on its own. What it is not is what the PR says, and it leaves enableForwarding === true next to preventJoystickForwarding === true — a pairing nothing in the tree reconciles. The forwarding the user turned on keeps working until the first transient clear (tab hidden at :271 in the web build, or the last joystick disconnecting at :231), after which enableJoystickForwardingIfSafe (:457-463) refuses to restore it and the only route back is the mini-widget switch. No snackbar, dialog or copy marks that transition; the user sees the sticks go dead.
Fix: pick one and make the text match. Either move preventJoystickForwarding.value = true under the same if (!userMovedTheSwitch) as the enableForwarding write, which is what the body and the follow-up describe, or keep it unconditional and correct the PR body plus the test-plan line "dialog still shows, forwarding stays on" to say that the manual enable does not survive a tab switch or a replug. Note that either choice interacts with 1.5: once the gesture is recorded properly, this branch fires only when the user really did opt in.
Consequence: the pull request's description states the opposite of what the code does, so the next person to read it will expect a manual override to persist when in practice it disappears at the next tab switch or reconnect.
Sections with nothing to report (10)
2. Persistence & User Data — ✅ (nothing persisted is touched: enableForwarding and preventJoystickForwarding are plain refs at head src/stores/controller.ts:67-68, the added otherSourcesCheck is a non-reactive let in the store setup with no storage behind it, and every useBlueOsStorage key in the file — cockpit-hold-last-joystick-input-when-window-hidden at :72 included — falls outside the three hunks)
3. AGENTS.md Adherence — ✅ (scope held to the lines being changed — no rename, import/hook reorder, const/let swap or formatter reflow, and the one comment removed went out with the code it documented; the three added comments explain why, not what; no new dependency, no Electron-only API, no widget Options entry, and no export added without a call site, since enableJoystickForwardingIfSafe already existed and was already called at :274; the (issue #2798) in the added comment is not the violation it resembles, since that rule scopes issue references to commit messages and the commit body carries none)
4. Security — ✅ (no dependency, env var, secret, eval/v-html, encoded blob, workflow or Electron main-process change; the only network traffic on this path is the pre-existing MAVLink2REST fetch at src/libs/blueos.ts:458-459, untouched)
5. Performance — ✅ (the one changed function traces to the 500 ms pollGamepadsConnections re-poll at src/libs/joystick/manager.ts:436-482, which emits only on set changes; the new 'done' branch replaces the 3 s settle plus four sequential fetches with a single synchronous ref write, so reconnections get strictly cheaper than at the base, and the diff registers no listener, watcher, interval or subscription needing teardown)
6. UI / UX — ✅ (no control, dialog or copy is added — the warning dialog at head src/stores/controller.ts:205-216 is unchanged context, the only interactive surface on this path already logs through logUserAction at JoystickCommIndicator.vue:94, and the one place the existing dialog copy is now misleading is filed inside 1.5 rather than counted twice here)
7. Code Quality & Style — ✅ (per complexity-report.json, the measurement produced for this head: processJoystickConnectionEvent scores 14, up from 12, tripping pushed-above-12 alone, with depth 3 against a baseDepth of 2 — under the 4 that fires nested-4-or-deeper; the PR's own contribution is two guard clauses, one single-line if and one else if, flat and independent of each other, so the trigger is answered; 1 changed file, 49 functions measured, not truncated. No any, no scoped CSS, no wrapped inline-expression string, let is required since the latch is reassigned, and the longest added comment line sits well inside the 180-char max-len)
8. Commit Hygiene — ✅ (one commit for one logical change, +23/-9; the fix: prefix fits the change and matches the styles in git log, where scope prefixes and conventional types both appear; the body says why and carries no #N, no closing keyword, no wip/fixup! and no AI co-author trailer — round 3's feedback was folded into the original commit by amend rather than added as an "address review" commit, which is what AGENTS.md asks for)
9. Tests — ✅ (the diff touches no test file and weakens no assertion; a search of src/tests/ — basic.test.ts, composables/, libs/, types/ — turns up no reference to stores/controller or checkForOtherManualControlSources, so no existing coverage is affected either way)
10. Documentation — ✅ (no Lite/Standalone capability difference is introduced, so the README parity table is unaffected; no exported signature changed, and the JSDoc on checkForOtherManualControlSources at src/libs/blueos.ts:436-440 still matches its behaviour — the PR-body mismatch is filed as 1.6 rather than counted here)
11. Nitpicks / Optional — ✅ (nothing beyond the two findings above; 'pending' and 'running' differ only at :186, but the third state is what makes the in-flight guard readable and it earns its place)
Generated by Claude. This is advisory; a human reviewer must still approve.
|
When another GCS was detected sending MANUAL_CONTROL, forwarding was disabled and the warning dialog shown. Because the conflict check ran on every joystick (re)connection, the dialog kept popping up and forwarding kept being disabled, making users lose control of the vehicle on busy test days. Latch the check so it runs at most once per session, restoring forwarding on later reconnections instead of re-running it, and skip its automatic enable/disable when the user moved the forwarding switch while it was still waiting for the vehicle.
205d5b0 to
8608fbd
Compare
Review follow-up — round 4Done
Won't change (with reasoning)
Questions for reviewers
|
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 6)
Cockpit checks, when you plug a gamepad in, whether another ground station is already sending joystick commands to the same vehicle; if it finds one it warns you and switches your own stick output off. That check used to re-run on every reconnection, so the warning kept coming back and kept taking control away. This push completes the fix: the check runs once per session, and the question of whether it may still apply its own result is now answered by the one place the user actually flips the switch, instead of by comparing the shared switch before and after the wait. Both problems from last round are gone. What is left is a sentence in the pull request text that promises more than the code delivers — leaving the joystick settings page still turns forwarding back on with no check, on a path this PR does not touch. What still needs attention
Since round 4 — 2 closed, comparing 205d5b0 → 8608fbdRange. Head line numbers for Findings.
Discussion. @rafaellehmkuhl posted a round-4 follow-up (
The second comment is a bare Resolutions and votes. Injection check. Nothing in the PR body, the diff, the two new comments or Change map — what was established before judgingLine numbers for Claims (from the PR body and commit message, each checked against the code):
Failure site. The guard and the conflict check inside Entry points.
Invariants.
1. Correctness & Implementation Bugs — 1 finding1.7 — The PR body states the safety property the new The first half is verified (head // src/views/ConfigurationJoystickView.vue:929-931
onUnmounted(() => {
controllerStore.enableForwarding = true
})Open the joystick settings page after the "Multiple joystick controllers detected" dialog — a plausible next step, since the dialog's own copy sends the user looking for a forwarding control — and leave it. The page's guard watcher ( Two things this finding is not. It is not a regression: at the base that same write is equally uncorrected until the next joystick reconnection, and both the base (re-run check → disable) and the head (helper refuses → disable) put forwarding back off on that reconnection, so the PR neither introduces the hole nor widens it. And it is not a request to fix // plus `enableJoystickForwardingIfSafe,` in the store's return
onUnmounted(() => {
controllerStore.enableJoystickForwardingIfSafe()
})What belongs in this PR is the sentence. Either narrow it to what the code guarantees — reconnections and tab focus changes do not bring forwarding back after a conflict, and leaving the joystick settings page still does — or, if the claim is meant to hold as written, take the change above. The same over-claim sits in the bullet 1.6 closed ("coming back needs the widget switch again"), so both sentences need the same qualification. Consequence: the pull request promises that a detected conflict keeps your joystick silent until you say otherwise, when visiting and leaving one settings page turns it back on with nothing on screen to tell you. Sections with nothing to report (10)2. Persistence & User Data — ✅ (nothing persisted is touched: 3. AGENTS.md Adherence — ✅ (scope held to the lines being changed — no rename, import/hook reorder, 4. Security — ✅ (no dependency, env var, secret, 5. Performance — ✅ (the changed functions trace to the 500 ms 6. UI / UX — ✅ (no control, dialog or copy is added — the 7. Code Quality & Style — ✅ (per 8. Commit Hygiene — ✅ (one commit for one logical change, 9. Tests — ✅ (the diff touches no test file and weakens no assertion; a search of 10. Documentation — ✅ (no Lite/Standalone capability difference is introduced, so the README parity table is unaffected; the one store surface added, 11. Nitpicks / Optional — ✅ (nothing beyond the finding above; the latch and the gesture flag could have been folded into one state value, but keeping them apart is what makes each read on its own, and both are cleared in the same two lines at head Generated by Claude. This is advisory; a human reviewer must still approve. |
…ter a conflict The joystick settings view turns forwarding off while it is open and back on when it is left, but it wrote `enableForwarding` directly, bypassing the `preventJoystickForwarding` guard. After the "Multiple joystick controllers detected" warning, opening and leaving that page brought the sticks back live with nothing on screen saying so, while another ground station was still driving the vehicle. Route the write through `enableJoystickForwardingIfSafe()`, the chokepoint the reconnection and tab-visibility paths already use, so the page keeps restoring forwarding in a clean session and leaves it off in a conflicted one.
Review follow-up — round 5Done differently
|
|
/review |
✅ READY TO MERGE (Automated PR Review — round 7)
Cockpit checks, when you plug a gamepad in, whether another ground station is already sending joystick commands to the same vehicle; if it finds one it warns you and switches your own stick output off. That check used to re-run on every reconnection, so the warning kept coming back and kept taking control away. The PR makes it run once per session, restores your sticks on later reconnections instead of re-checking, and leaves a switch you flipped yourself alone if the check comes back minutes later. This push adds the last piece: the joystick settings page used to turn stick output back on the moment you left it, no matter what the check had found, and it now asks the same safety helper every other path asks — so after a warning your sticks stay silent until you switch them on yourself. What still needs attentionNothing is open. Every finding raised across the seven rounds is closed. Since round 6 — 1 closed, comparing 8608fbd → cf3c928Range. Line numbers for Findings.
Discussion. @rafaellehmkuhl posted a round-6 follow-up (
The second comment is a bare Resolutions and votes. Injection check. Nothing in the PR body, the diff, the two new comments or Change map — what was established before judgingHead numbering as stated in the block above. Claims (from the PR body and the two commit messages, each checked against the code):
Failure site. Two, both in the diff. The guard and the conflict check inside Entry points.
Invariants.
No path left turns forwarding on after a conflict except the user's own switch, and
Sections with nothing to report (11)1. Correctness & Implementation Bugs — ✅ (re-ran the whole diff, not just the increment: every writer of 2. Persistence & User Data — ✅ (nothing persisted is added, reshaped or removed: 3. AGENTS.md Adherence — ✅ (scope held to the lines changed in both commits — no rename, import/hook reorder, 4. Security — ✅ (no dependency, env var, secret, 5. Performance — ✅ (the change added this round is one synchronous ref write behind an unmount hook that fires when the user leaves a settings page; the 6. UI / UX — ✅ (no control, dialog, icon or user-facing string is added or changed — the 7. Code Quality & Style — ✅ (per 8. Commit Hygiene — ✅ (two commits, one logical change each; the behaviour fix to the settings page rides alone rather than being folded into the latch commit, which is what 9. Tests — ✅ (the diff touches no test file and weakens no assertion; a search of 10. Documentation — ✅ (no Lite/Standalone capability difference is introduced — the changed path is the same in both builds, the 11. Nitpicks / Optional — ✅ (one thing weighed and dropped: the second commit's subject runs 82 characters against a 68-78 range across the last 120 commits in Generated by Claude. This is advisory; a human reviewer must still approve. |
1 participant
Summary
The "Multiple joystick controllers detected" warning is triggered when Cockpit detects another ground control station sending
MANUAL_CONTROL/RC_CHANNELS_OVERRIDEto the vehicle (checkForOtherManualControlSources), not when two physical gamepads are plugged in. On busy test days with multiple computers/vehicles, that detection trips often.The check ran inside
processJoystickConnectionEvent, which fires on every joystick connect/disconnect (gamepad polling, Bluetooth roaming, etc.). Once another source was detected,enableForwardingwas set tofalse— and since forwarding was now off, the next reconnect re-ran the check, re-disabled forwarding, and re-showed the dialog. That's the reported "popup keeps spawning" + "user loses control" loop. The only workaround was the joystick config view'sonUnmountedhook force-enabling forwarding (the "twiddle the sticks and click out" trick).This PR makes the conflict check run at most once per session, on the first joystick connection, and never again — so the joystick already in use keeps control and the dialog is shown at most once.
otherSourcesCheckstage (pending→running→done) owned by the check itself. Reconnections no longer re-run it, and therunningstage doubles as the in-flight guard, so concurrent connection events can't race to show the dialog twice.enableForwardingmatters: that ref is turned on with no joystick connected byConfigurationJoystickView'sonUnmountedhook and by the document-visibility watcher in the web build, so skipping on it would mean the safety check never runs at all for the first joystick.done, a reconnection restores forwarding throughenableJoystickForwardingIfSafe()instead of re-checking. The disconnect cleanup disables forwarding whenever the last joystick goes away, so without this the sticks would stay silent after every replug. The helper refuses whilepreventJoystickForwardingis set, so a session where a conflict was found stays disabled until the user re-enables it by hand.onUnmountedhook wroteenableForwardingdirectly, bypassing that guard, so opening and leaving that page after the warning brought the sticks back live with nothing on screen saying so. It now callsenableJoystickForwardingIfSafe()too — it still restores forwarding in a clean session, and leaves it off in a conflicted one. Pre-existing hole rather than one this fix opens, but it is the same invariant, so it lands here.getVehicleAddress()waits for the vehicle with no timeout, so the check can resolve minutes later, and a deliberate pause or resume made in the meantime should not be undone. The gesture is stamped by a newsetForwardingByUserstore action, called by the top-right joystick widget's switch — the only user-facing writer ofenableForwarding. Comparing the ref before and after the wait would instead catch Cockpit's own writes too (the joystick settings view'sonUnmountedhook, the document-visibility watcher, the disconnect cleanup) and mistake them for a deliberate choice, cancelling either the protective switch-off or the switch-on.preventJoystickForwardingis still set when a conflict is found, so the automatic re-enables stay blocked even where the user's own choice is left standing. A conflict is a real one whoever flipped the switch, so that side keeps the conservative behavior: forwarding the user turned on by hand keeps working, but it does not survive a tab switch or a replug, and coming back needs the widget switch again.return(which also skipped disconnect cleanup andcurrentMainJoystickrefresh); that logic now runs viacontinue.Test plan
MANUAL_CONTROL: dialog shows once, forwarding disabled.onUnmountedenables forwarding) → the check still applies its own result when it resolves.currentMainJoystickupdates correctly.Fixes #2798.