feat: remote control — Stream Deck support, global shortcuts, token-gated local API#150
Merged
Conversation
…ated local API (issue #143) RC0: OS-global shortcuts (record/stream/mic) via Electron globalShortcut — Stream Deck's native Hotkey action works today; configured in Settings, off by default. RC1: third backend transport role 'remote' with a HARD allowlist (remote.describe + remote.intent only; everything else 403s), rotatable token in the secret store, 0600 same-machine discovery file (written on enable/startup, removed on disable/shutdown), intent validation + 150ms per-family debounce, and a generation watch that closes live remote sockets the moment the token rotates or the surface is disabled. Remote sockets' event filter is LOCKED to remote.state/remote.ack at connection setup and cannot be widened. RC2+RC3: the backend only relays — intents execute in the renderer through the exact same handlers as the on-screen buttons (record/stream via the session flow with all validation, mic via audio processing, scenes via the layout transaction, takeovers via Screens, windows via main IPC), every intent is acked, and a minimal leak-safe state projection (recording/live/ mic/scene/takeover/windows) streams to remote clients for key rendering. RC4: official Stream Deck plugin (apps/streamdeck-plugin, Elgato SDK v2): six actions with state-driven titles, discovery-file pairing, reconnect backoff. Builds in the workspace. RC5: Settings 'Remote control' section (enable/token/copy/regenerate/ connected count) + 'Global shortcuts' section; docs/remote-control.md protocol doc for third-party integrations; smoke:remote-control in local-gates — proves the discovery contract, allowlist, filter lock, intent round trips against backend-CONFIRMED state, debounce, and regenerate-cuts-clients against the real app. Verification: 1314 backend tests, clippy -D warnings, fmt, 1113 desktop tests, typecheck, lint, format, plugin typecheck+build, smoke:remote-control PASS end-to-end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis change adds a token-gated remote-control protocol with backend intent handling, renderer integration, global shortcuts, Stream Deck support, settings UI, discovery-file pairing, and smoke coverage for security, state round trips, debounce, and client invalidation. ChangesRemote control surface
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StreamDeck
participant BackendWebSocket
participant Renderer
participant SettingsUI
StreamDeck->>BackendWebSocket: Connect with discovery token
BackendWebSocket->>StreamDeck: remote.describe and remote.state
StreamDeck->>BackendWebSocket: remote.intent
BackendWebSocket->>Renderer: remote.intent event
Renderer->>BackendWebSocket: remote.intent.ack and remote.surface.publish
BackendWebSocket->>StreamDeck: remote.ack and remote.state
SettingsUI->>BackendWebSocket: remote.control.enable or regenerate
BackendWebSocket->>StreamDeck: Close clients after generation change
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
apps/streamdeck-plugin/package.json (1)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit coverage for
readDiscovery/defaultDiscoveryPath.These are small pure functions in
videorc-client.tswell suited for the existingtestscript; no test files are included in this slice.As per coding guidelines,
**/*.{ts,tsx,rs}: "prefer small pure helpers for behavior that needs tests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/streamdeck-plugin/package.json` around lines 8 - 11, Add unit tests under the existing test script for the pure helpers readDiscovery and defaultDiscoveryPath in videorc-client.ts, covering their expected path and discovery-reading behavior. Use the project’s established test conventions and ensure the tests are included by the current src/*.test.ts pattern.Source: Coding guidelines
apps/streamdeck-plugin/bin/plugin.js (1)
1-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompiled
tscoutput committed alongside sources. Both files underbin/are generated bynpm run build(tsc -p tsconfig.json,outDir: "bin") but are checked into git next to theirsrc/*.tscounterparts. This risks drift if someone editssrc/without rebuilding, and duplicates the source-level issues already flagged onplugin.ts/videorc-client.ts.
apps/streamdeck-plugin/bin/plugin.js#L1-L224: either gitignorebin/and generate it as part of packaging/release, or add a CI check that fails ifbin/plugin.jsis stale relative tosrc/plugin.ts.apps/streamdeck-plugin/bin/videorc-client.js#L1-L149: same treatment — gitignore or add a staleness check relative tosrc/videorc-client.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/streamdeck-plugin/bin/plugin.js` around lines 1 - 224, Stop committing generated TypeScript output under apps/streamdeck-plugin/bin: remove both apps/streamdeck-plugin/bin/plugin.js (lines 1-224) and apps/streamdeck-plugin/bin/videorc-client.js (lines 1-149) from version control, add bin/ to the appropriate gitignore, and ensure the packaging or release workflow runs npm run build to generate them.apps/streamdeck-plugin/src/videorc-client.ts (1)
39-39: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueFixed 2s reconnect delay, no exponential backoff.
RECONNECT_DELAY_MSis a flat 2s retry with no backoff/jitter. For a local same-machine socket this is low risk, but if the backend is down for a long time this will hammerconnect()indefinitely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/streamdeck-plugin/src/videorc-client.ts` at line 39, Update the reconnect scheduling around RECONNECT_DELAY_MS to use exponential backoff, increasing the delay after each failed connection attempt and resetting it after a successful connection. Preserve a bounded maximum delay and apply jitter so repeated retries do not hammer connect() at a fixed interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/src/hooks/use-studio.tsx`:
- Around line 9241-9252: Add rejection handling to the setGlobalShortcuts
promise in the effect containing the shortcut registration flow, reporting the
IPC error through reportError and preserving the existing conflict toast for
partial failures. Include reportError in that effect’s dependency array so the
callback remains correctly tracked.
- Around line 9195-9210: Update the useEffect responsible for
remote.surface.publish to retry failed initial publication with bounded backoff,
ensuring remote.describe is eventually populated even without later state
changes. Preserve the existing connection guard and best-effort behavior, but
replace the silent catch in the publication flow with retry scheduling and
explicit failure diagnostics after the retry limit.
- Around line 9178-9192: Update the state projection containing notes and
preview to include comments: commentsWindow.open, and add commentsWindow.open to
the associated memo dependency array. Add a regression assertion verifying the
published comments-window state reflects whether the configured comments window
is open.
In `@apps/streamdeck-plugin/manifest.json`:
- Around line 39-59: Add the appropriate PropertyInspectorPath to the manifest
entries for scene-apply, takeover-toggle, and window-front, pointing each to the
existing inspector UI used to configure its settings. Ensure the inspector
persists layoutPreset, assetId, and window values so these actions receive their
required settings instead of remaining unset.
- Around line 1-17: Update the top-level manifest object with the plugin UUID
required for packaging, then add settings/property-inspector support for the
scene-apply, takeover-toggle, and window-front actions so users can configure
and persist layoutPreset, assetId, and window values consumed by those actions.
In `@apps/streamdeck-plugin/src/plugin.ts`:
- Around line 36-47: Update onKeyDown to await the boolean result from
client.sendIntent(intent), and call ev.action.showAlert() when it returns false.
Preserve the existing disconnected-client and missing-intent checks, and ensure
rejected backend intents no longer complete silently.
- Around line 16-48: Register the shared client event listeners only once
instead of inside onWillAppear. Add a constructor or equivalent one-time setup
in VideorcAction that subscribes refresh to the state, connected, and
disconnected events, while leaving onWillAppear responsible only for updating
the newly visible action’s title.
In `@apps/streamdeck-plugin/src/videorc-client.ts`:
- Around line 137-146: Update the message parsing in the ws 'message' callback
to reject null and non-object JSON results before accessing message.event or
message.payload. Preserve the existing behavior of returning silently for
malformed or unsupported messages, while continuing to process valid object
messages.
- Around line 108-163: Update sendIntent() to retain the generated request id
and track or otherwise associate it with the dispatched intent, then update
connect()’s message handler to validate the parsed envelope before reading event
and surface matching ok: false responses as Stream Deck feedback using the
existing client feedback mechanism. Preserve current describe/state handling and
ensure JSON null or other non-object payloads are ignored safely.
In `@crates/videorc-backend/src/main.rs`:
- Around line 4344-4348: Replace the manual connected_clients
increment/decrement around the Remote WebSocket connection flow with a
Drop-based RAII guard, following the existing WebSocketQueueMetricsInner/Weak
tracking pattern. Ensure the guard increments on creation and decrements
automatically on every exit path, including failed initial backend.ready sends,
breaks, and panics; remove the redundant manual decrement near the function end.
In `@crates/videorc-backend/src/remote_control.rs`:
- Around line 207-225: The write_discovery function currently creates the
discovery file before applying restrictive permissions, leaving a temporary
exposure window. Replace std::fs::write with an OpenOptions-based handle
configured via Unix OpenOptionsExt::mode(0o600), write the serialized body
through that handle, and retain the existing Unix permission handling while
avoiding any additional Windows ACL changes.
In `@scripts/smoke-remote-control-app.mjs`:
- Around line 16-18: Update the smoke script’s fail/error handling and cleanup
flow: make fail throw instead of calling process.exit, set process.exitCode in
the top-level catch, and ensure the temporary user-data directory is removed
after stopApp() completes. Preserve cleanup through the outer finally for both
success and failure paths, including the referenced import and shutdown
sections.
---
Nitpick comments:
In `@apps/streamdeck-plugin/bin/plugin.js`:
- Around line 1-224: Stop committing generated TypeScript output under
apps/streamdeck-plugin/bin: remove both apps/streamdeck-plugin/bin/plugin.js
(lines 1-224) and apps/streamdeck-plugin/bin/videorc-client.js (lines 1-149)
from version control, add bin/ to the appropriate gitignore, and ensure the
packaging or release workflow runs npm run build to generate them.
In `@apps/streamdeck-plugin/package.json`:
- Around line 8-11: Add unit tests under the existing test script for the pure
helpers readDiscovery and defaultDiscoveryPath in videorc-client.ts, covering
their expected path and discovery-reading behavior. Use the project’s
established test conventions and ensure the tests are included by the current
src/*.test.ts pattern.
In `@apps/streamdeck-plugin/src/videorc-client.ts`:
- Line 39: Update the reconnect scheduling around RECONNECT_DELAY_MS to use
exponential backoff, increasing the delay after each failed connection attempt
and resetting it after a successful connection. Preserve a bounded maximum delay
and apply jitter so repeated retries do not hammer connect() at a fixed
interval.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcfe6d64-a398-4691-acbf-1649fc92d8e8
⛔ Files ignored due to path filters (8)
apps/streamdeck-plugin/imgs/mic.svgis excluded by!**/*.svgapps/streamdeck-plugin/imgs/plugin.svgis excluded by!**/*.svgapps/streamdeck-plugin/imgs/record.svgis excluded by!**/*.svgapps/streamdeck-plugin/imgs/scene.svgis excluded by!**/*.svgapps/streamdeck-plugin/imgs/stream.svgis excluded by!**/*.svgapps/streamdeck-plugin/imgs/takeover.svgis excluded by!**/*.svgapps/streamdeck-plugin/imgs/window.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
AGENTS.mdapps/desktop/src/main/index.tsapps/desktop/src/preload/index.tsapps/desktop/src/renderer/src/components/tabs/settings-tab.tsxapps/desktop/src/renderer/src/hooks/studio-provider.integration.test.tsapps/desktop/src/renderer/src/hooks/use-studio.tsxapps/desktop/src/renderer/src/lib/capture.tsapps/desktop/src/shared/backend.tsapps/desktop/src/shared/electron-ipc-contract.test.tsapps/desktop/src/shared/electron-ipc-contract.tsapps/desktop/src/shared/renderer-security-policy.tsapps/streamdeck-plugin/bin/plugin.jsapps/streamdeck-plugin/bin/videorc-client.jsapps/streamdeck-plugin/manifest.jsonapps/streamdeck-plugin/package.jsonapps/streamdeck-plugin/src/plugin.tsapps/streamdeck-plugin/src/videorc-client.tsapps/streamdeck-plugin/tsconfig.jsoncrates/videorc-backend/src/backend_authority.rscrates/videorc-backend/src/main.rscrates/videorc-backend/src/remote_control.rscrates/videorc-backend/src/state.rsdocs/remote-control.mdpackage.jsonscripts/smoke-remote-control-app.mjs
Comment on lines
+9241
to
+9252
| void window.videorc?.setGlobalShortcuts?.(shortcuts).then((result) => { | ||
| const failed = Object.entries(result?.registered ?? {}) | ||
| .filter(([, ok]) => !ok) | ||
| .map(([action]) => action) | ||
| if (failed.length > 0) { | ||
| toast.error('Some global shortcuts could not be registered', { | ||
| id: 'global-shortcuts-conflict', | ||
| description: | ||
| 'Another app may already use that key combination. Pick a different one in Settings.' | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle shortcut-registration rejection.
If the IPC request rejects, this creates an unhandled promise rejection and leaves users without registration diagnostics. Add .catch(...) reporting and include reportError in the effect dependencies.
As per coding guidelines, “Prefer explicit status and diagnostics over silent fallbacks.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/src/hooks/use-studio.tsx` around lines 9241 - 9252,
Add rejection handling to the setGlobalShortcuts promise in the effect
containing the shortcut registration flow, reporting the IPC error through
reportError and preserving the existing conflict toast for partial failures.
Include reportError in that effect’s dependency array so the callback remains
correctly tracked.
Source: Coding guidelines
franky47
suggested changes
Jul 17, 2026
franky47
left a comment
franky47
left a comment
There was a problem hiding this comment.
So many useEffects 😭
Q: does the takeover screens mute the mic too? Generally my BRB scene is only one splash screen in OBS, no cam no mic.
| : intent.kind === 'micMute' | ||
| } | ||
| })) | ||
| return void (await ack(true)) |
| if (status) setRemoteStatus(status) | ||
| }) | ||
| .finally(() => setRemotePending(false)) | ||
| } |
…r (PR #150 review) franky47's review + 12 CodeRabbit findings: Renderer — the remote-control code now uses NO useEffect: - Intent executor and global-shortcut handler are React 19.2 effect events (stable identity, latest closure), subscribed once in the existing client-setup effect; the intent-handler ref and its sync effect are gone. - The state projection is pushed by a RemoteSurfacePublisher the render body hands the latest snapshot (same latest-value pattern as the hook's render-synced refs); it dedupes, debounces past the commit, republishes on reconnect, and retries failed publishes with bounded backoff — a failed FIRST publish no longer leaves remote.describe empty forever. - Global-shortcut registration is a render-synced registrar that dedupes by value and reports IPC rejection instead of dropping it. - The projection now includes the comments window; describe rides the same snapshot. - Settings tab no longer polls: the backend pushes remote.control.status (enable/disable/regenerate + client connect/disconnect) into studio context; the tab renders it and fires actions. - Nested captureConfig mic mutations use Immer. Backend: - Remote client counting is a Drop guard — the early return on a failed initial ready send leaked connected_clients forever (phantom deck in Settings). Connect/disconnect emit remote.control.status. - Discovery file is born 0600 via OpenOptionsExt (no create-then-chmod umask window). Stream Deck plugin: - manifest gains the top-level UUID; scene/takeover/window actions get a dependency-free property inspector (ui/property-inspector.html) whose options populate live from remote.describe via sendToPlugin — these three actions were unconfigurable before. - Shared-client listeners register once in the action constructor instead of stacking on every onWillAppear. - sendIntent returns end-to-end truth (admission response + renderer's remote.ack, 5s timeout) and key presses showAlert on failure; message envelopes are guarded against non-object JSON. - readDiscovery/defaultDiscoveryPath unit tests. Smoke: fail() throws so the launched app always stops; exitCode instead of process.exit; temp user-data dir removed in finally. Gates: cargo 1314 green + clippy -D warnings, desktop 1113 green, typecheck/lint/format, scripts 647 green, smoke:remote-control PASS end-to-end against the dev app. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
All review changes are in (07730e8). @franky47 — useEffects: all gone. The remote-control code now adds zero
Your question — does a takeover mute the mic? No — takeovers are visual-only today: the camera/screen composition is replaced by the splash, but the mic keeps its live state (deliberate default: "BRB while I answer the door" often still wants you audible until you choose otherwise, and muting is one key away). For your OBS-style BRB workflow I'd add an opt-in "Mute mic while a takeover is active" setting (auto-restore on hide) as a follow-up — happy to file/build that if you want it. CodeRabbit findings, all 12 addressed:
Gates: cargo 1314 green + clippy 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/streamdeck-plugin/bin/videorc-client.js (1)
71-79: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSynchronously tear down connection state and settle pending intents.
Setting
this.ws = nullimmediately afterclose()prevents the asynchronousonGonehandler from executing its body (sincethis.ws === wsevaluates tofalse). As a result, pending requests will hang until they time out, and thedisconnectedevent is not emitted.Execute the teardown directly in
stop()to guarantee clean lifecycle termination.🐛 Proposed fix
stop() { this.stopped = true; if (this.reconnectTimer) { clearTimeout(this.reconnectTimer); this.reconnectTimer = null; } - this.ws?.close(); - this.ws = null; + if (this.ws) { + this.ws.close(); + this.ws = null; + } + this.settleAllPending(false); + if (this.connected) { + this.connected = false; + this.emit('disconnected'); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/streamdeck-plugin/bin/videorc-client.js` around lines 71 - 79, Update stop() to synchronously perform the same teardown and pending-intent settlement normally handled by the websocket onGone path before clearing this.ws. Ensure pending requests are settled and the disconnected event is emitted, then close the socket and clear connection state without allowing the asynchronous onGone handler to skip cleanup because this.ws was nulled first.apps/desktop/src/renderer/src/components/tabs/settings-tab.tsx (1)
377-379: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the remote-control copy platform-neutral.
“This Mac” and
Cmd+…examples are wrong on Windows/Linux and can guide users toward unusable bindings. Use “this computer” and deriveCmd/Ctrlexamples fromruntimeInfo.platform.Also applies to: 426-437
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/src/components/tabs/settings-tab.tsx` around lines 377 - 379, Update the Remote control copy in the settings tab to use “this computer” instead of “this Mac,” and make all shortcut examples in the related content platform-aware by deriving Cmd for macOS and Ctrl for Windows/Linux from runtimeInfo.platform. Ensure the displayed bindings consistently use the derived modifier.apps/streamdeck-plugin/src/videorc-client.ts (2)
105-113: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFully clean up admitted intents on timeout and stop.
Once a request moves to
pendingAcks, its timeout callback only deletespendingRequests, leaving an unbounded stalependingAcksentry if no ACK arrives. Additionally,stop()nullsthis.wsbeforeonGone, so pending intents andconnectedstate are not cleaned immediately.Delete the matching ACK entry inside
settle(), and havestop()callsettleAllPending(false)and transition connection state explicitly. Add timeout and stop lifecycle tests.Also applies to: 127-152, 209-215
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/streamdeck-plugin/src/videorc-client.ts` around lines 105 - 113, Update settle() to remove the corresponding entry from pendingAcks whenever a request is resolved or timed out, preventing stale admitted intents. In stop(), invoke settleAllPending(false) and explicitly transition the connection to disconnected before or while closing the socket, rather than relying on onGone after nulling ws. Add lifecycle tests covering timeout cleanup and stop cleanup, including pending intents and connected state.
58-72: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate discovery fields before constructing the WebSocket URL.
readDiscovery()only checks truthiness, so wrong-type values like{ port: "bad", token: [] }can still reach connection setup. Require a 1..65535 integerport, a non-empty stringtoken, and a stringhost; add wrong-type tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/streamdeck-plugin/src/videorc-client.ts` around lines 58 - 72, Update readDiscovery to validate parsed.host, parsed.port, and parsed.token before returning a Discovery: require host to be a string, token to be a non-empty string, and port to be an integer in the 1–65535 range. Reject invalid values before WebSocket URL construction, preserve valid defaults where applicable, and add tests covering wrong-type fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/src/components/tabs/settings-tab.tsx`:
- Around line 119-132: Update the remote-control switch rendering and action
flow around remoteStatus and runRemoteAction so the switch is disabled whenever
remoteStatus is null, including while status is unavailable or reconnecting. Add
an explicit unavailable/reconnecting status message for that state, while
preserving the existing pending-state behavior and normal actions once the
backend has seeded a status.
In `@apps/desktop/src/renderer/src/lib/global-shortcuts.ts`:
- Around line 22-35: Update GlobalShortcuts.sync to compare against the
currently pending shortcuts as well as lastRegisteredBody, cancel any scheduled
registration when the requested configuration matches the registered state, and
replace the zero-delay timer with a debounce delay so rapid edits produce one
registration. Ensure the timer is cleared/reset when cancelling stale pending
work, while register continues applying the latest pending configuration.
In `@apps/desktop/src/renderer/src/lib/remote-surface.ts`:
- Around line 58-81: Update the RemoteSurface connection lifecycle around
attach, markConnected, and detach to maintain a generation counter, incrementing
it on each call. Capture the generation when publishing, then after each await
ignore completions from obsolete generations before scheduling retries or
updating lastPublishedBody, so stale connections cannot affect deduplication for
the current connection.
In `@apps/streamdeck-plugin/bin/videorc-client.js`:
- Around line 94-98: Update the settle function to remove the corresponding
requestId entry from pendingAcks in addition to pendingRequests, ensuring both
maps are cleaned up when an intent resolves or times out.
---
Outside diff comments:
In `@apps/desktop/src/renderer/src/components/tabs/settings-tab.tsx`:
- Around line 377-379: Update the Remote control copy in the settings tab to use
“this computer” instead of “this Mac,” and make all shortcut examples in the
related content platform-aware by deriving Cmd for macOS and Ctrl for
Windows/Linux from runtimeInfo.platform. Ensure the displayed bindings
consistently use the derived modifier.
In `@apps/streamdeck-plugin/bin/videorc-client.js`:
- Around line 71-79: Update stop() to synchronously perform the same teardown
and pending-intent settlement normally handled by the websocket onGone path
before clearing this.ws. Ensure pending requests are settled and the
disconnected event is emitted, then close the socket and clear connection state
without allowing the asynchronous onGone handler to skip cleanup because this.ws
was nulled first.
In `@apps/streamdeck-plugin/src/videorc-client.ts`:
- Around line 105-113: Update settle() to remove the corresponding entry from
pendingAcks whenever a request is resolved or timed out, preventing stale
admitted intents. In stop(), invoke settleAllPending(false) and explicitly
transition the connection to disconnected before or while closing the socket,
rather than relying on onGone after nulling ws. Add lifecycle tests covering
timeout cleanup and stop cleanup, including pending intents and connected state.
- Around line 58-72: Update readDiscovery to validate parsed.host, parsed.port,
and parsed.token before returning a Discovery: require host to be a string,
token to be a non-empty string, and port to be an integer in the 1–65535 range.
Reject invalid values before WebSocket URL construction, preserve valid defaults
where applicable, and add tests covering wrong-type fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ee533fd-75d7-4712-b111-7991e6789c1e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
apps/desktop/package.jsonapps/desktop/src/renderer/src/components/tabs/settings-tab.tsxapps/desktop/src/renderer/src/hooks/use-studio.tsxapps/desktop/src/renderer/src/lib/global-shortcuts.tsapps/desktop/src/renderer/src/lib/remote-surface.tsapps/streamdeck-plugin/bin/plugin.jsapps/streamdeck-plugin/bin/videorc-client.jsapps/streamdeck-plugin/manifest.jsonapps/streamdeck-plugin/src/plugin.tsapps/streamdeck-plugin/src/videorc-client.test.tsapps/streamdeck-plugin/src/videorc-client.tsapps/streamdeck-plugin/ui/property-inspector.htmlcrates/videorc-backend/src/main.rscrates/videorc-backend/src/remote_control.rsdocs/remote-control.mdscripts/smoke-remote-control-app.mjs
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/remote-control.md
- apps/streamdeck-plugin/manifest.json
- apps/streamdeck-plugin/bin/plugin.js
- crates/videorc-backend/src/remote_control.rs
- apps/streamdeck-plugin/src/plugin.ts
- scripts/smoke-remote-control-app.mjs
- crates/videorc-backend/src/main.rs
Comment on lines
+119
to
+132
| // Remote-control status is pushed into the studio context by the backend | ||
| // (remote.control.status events) — this tab only renders it and fires | ||
| // actions; the switch settles when the backend confirms. | ||
| const remoteStatus = remoteControl.status | ||
| const [remotePending, setRemotePending] = useState(false) | ||
| const runRemoteAction = async ( | ||
| action: () => Promise<RemoteControlStatus | null> | ||
| ): Promise<void> => { | ||
| setRemotePending(true) | ||
| try { | ||
| await action() | ||
| } finally { | ||
| setRemotePending(false) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Disable the switch while remote status is unavailable.
When remoteStatus is null, remoteControlRequest() can return null, making the enabled switch silently do nothing. Disable it until status is seeded and show an unavailable/reconnecting message.
As per coding guidelines, “Prefer explicit status and diagnostics over silent fallbacks.”
Also applies to: 366-375
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/src/components/tabs/settings-tab.tsx` around lines
119 - 132, Update the remote-control switch rendering and action flow around
remoteStatus and runRemoteAction so the switch is disabled whenever remoteStatus
is null, including while status is unavailable or reconnecting. Add an explicit
unavailable/reconnecting status message for that state, while preserving the
existing pending-state behavior and normal actions once the backend has seeded a
status.
Source: Coding guidelines
Comment on lines
+22
to
+35
| sync(shortcuts: GlobalShortcutsConfig): void { | ||
| const body = JSON.stringify(shortcuts) | ||
| if (body === this.lastRegisteredBody) { | ||
| return | ||
| } | ||
| this.pending = shortcuts | ||
| if (this.timer) { | ||
| return | ||
| } | ||
| this.timer = setTimeout(() => { | ||
| this.timer = null | ||
| this.register() | ||
| }, 0) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cancel stale registrations and debounce actual edits.
After A → B → A, the second A returns early while pending B remains scheduled, so the OS can register a shortcut no longer shown in Settings. Also, setTimeout(..., 0) registers every separately typed character.
Proposed fix
sync(shortcuts: GlobalShortcutsConfig): void {
const body = JSON.stringify(shortcuts)
if (body === this.lastRegisteredBody) {
+ this.pending = null
+ if (this.timer) {
+ clearTimeout(this.timer)
+ this.timer = null
+ }
return
}
this.pending = shortcuts
if (this.timer) {
- return
+ clearTimeout(this.timer)
}
this.timer = setTimeout(() => {
this.timer = null
this.register()
- }, 0)
+ }, 250)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sync(shortcuts: GlobalShortcutsConfig): void { | |
| const body = JSON.stringify(shortcuts) | |
| if (body === this.lastRegisteredBody) { | |
| return | |
| } | |
| this.pending = shortcuts | |
| if (this.timer) { | |
| return | |
| } | |
| this.timer = setTimeout(() => { | |
| this.timer = null | |
| this.register() | |
| }, 0) | |
| } | |
| sync(shortcuts: GlobalShortcutsConfig): void { | |
| const body = JSON.stringify(shortcuts) | |
| if (body === this.lastRegisteredBody) { | |
| this.pending = null | |
| if (this.timer) { | |
| clearTimeout(this.timer) | |
| this.timer = null | |
| } | |
| return | |
| } | |
| this.pending = shortcuts | |
| if (this.timer) { | |
| clearTimeout(this.timer) | |
| } | |
| this.timer = setTimeout(() => { | |
| this.timer = null | |
| this.register() | |
| }, 250) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/src/lib/global-shortcuts.ts` around lines 22 - 35,
Update GlobalShortcuts.sync to compare against the currently pending shortcuts
as well as lastRegisteredBody, cancel any scheduled registration when the
requested configuration matches the registered state, and replace the zero-delay
timer with a debounce delay so rapid edits produce one registration. Ensure the
timer is cleared/reset when cancelling stale pending work, while register
continues applying the latest pending configuration.
Comment on lines
+58
to
+81
| attach(client: RemoteSurfaceClient): void { | ||
| this.client = client | ||
| this.connected = false | ||
| } | ||
|
|
||
| /** The socket is live: the backend's surface slate is blank, so the last | ||
| * published body no longer counts — republish immediately. */ | ||
| markConnected(): void { | ||
| this.connected = true | ||
| this.lastPublishedBody = null | ||
| this.retryAttempt = 0 | ||
| this.schedule(0) | ||
| } | ||
|
|
||
| detach(): void { | ||
| this.client = null | ||
| this.connected = false | ||
| this.lastPublishedBody = null | ||
| this.retryAttempt = 0 | ||
| if (this.timer) { | ||
| clearTimeout(this.timer) | ||
| this.timer = null | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Ignore publication completions from obsolete connections.
An old in-flight publish can resolve after reconnect, restore lastPublishedBody, and make the new connection skip its initial publish. Track a generation incremented by attach, markConnected, and detach; verify it after await and before retrying or updating dedupe state.
Proposed fix
export class RemoteSurfacePublisher {
+ private generation = 0
attach(client: RemoteSurfaceClient): void {
+ this.generation += 1
this.client = client
this.connected = false
}
markConnected(): void {
+ this.generation += 1
this.connected = true
...
}
detach(): void {
+ this.generation += 1
...
}
private async flush(): Promise<void> {
+ const generation = this.generation
const { client, latest } = this
...
await client.request('remote.surface.publish', latest)
+ if (generation !== this.generation || client !== this.client || !this.connected) return
this.lastPublishedBody = body
} catch (error) {
+ if (generation !== this.generation || client !== this.client) return
...Also applies to: 107-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/src/lib/remote-surface.ts` around lines 58 - 81,
Update the RemoteSurface connection lifecycle around attach, markConnected, and
detach to maintain a generation counter, incrementing it on each call. Capture
the generation when publishing, then after each await ignore completions from
obsolete generations before scheduling retries or updating lastPublishedBody, so
stale connections cannot affect deduplication for the current connection.
Comment on lines
+94
to
+98
| const settle = (ok) => { | ||
| clearTimeout(timer); | ||
| this.pendingRequests.delete(requestId); | ||
| resolve(ok); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Prevent closure leak in pendingAcks on intent timeout.
When an admitted intent times out waiting for a remote.ack, the timer calls settle(false). Currently, settle removes the request from pendingRequests but leaves it in pendingAcks, permanently leaking the closure and inflating the map over time.
Find and remove the entry from pendingAcks inside settle to properly clean up timed-out intents.
♻️ Proposed fix
- const settle = (ok) => {
- clearTimeout(timer);
- this.pendingRequests.delete(requestId);
- resolve(ok);
- };
+ const settle = (ok) => {
+ clearTimeout(timer);
+ this.pendingRequests.delete(requestId);
+ for (const [id, s] of this.pendingAcks) {
+ if (s === settle) {
+ this.pendingAcks.delete(id);
+ break;
+ }
+ }
+ resolve(ok);
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const settle = (ok) => { | |
| clearTimeout(timer); | |
| this.pendingRequests.delete(requestId); | |
| resolve(ok); | |
| }; | |
| const settle = (ok) => { | |
| clearTimeout(timer); | |
| this.pendingRequests.delete(requestId); | |
| for (const [id, s] of this.pendingAcks) { | |
| if (s === settle) { | |
| this.pendingAcks.delete(id); | |
| break; | |
| } | |
| } | |
| resolve(ok); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/streamdeck-plugin/bin/videorc-client.js` around lines 94 - 98, Update
the settle function to remove the corresponding requestId entry from pendingAcks
in addition to pendingRequests, ensuring both maps are cleaned up when an intent
resolves or times out.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #143.
Two integration tiers
Global shortcuts (works today, no plugin): Settings → Global shortcuts registers OS-wide accelerators for record/stream/mic — bind them to a Stream Deck Hotkey action or any macro tool. Off by default.
Remote protocol + official plugin: a token-gated local surface with a hard security contract, plus
apps/streamdeck-plugin(Elgato SDK v2) with six actions: Record, Go Live, Mic, Scene, Takeover (BRB), Window — key titles render backend-confirmed state.Security model (public repo — assume hostile port probes)
remote: allowlist of exactly two methods (remote.describe,remote.intent);session.*,oauth.*, everything else →forbidden-method(pinned by tests)remote.state/remote.ackat connection setup; the filter-mutation commands are refusedVerification
smoke:remote-control(new, insmoke:local-gates) drives the real app end-to-end: discovery contract (0600/port/token), allowlist, filter lock, micToggle + sceneApply round trips against confirmed state, 150ms debounce, regenerate-cuts-clients — PASS-D warnings, fmt, 1113 desktop tests (incl. a renderer intent-dispatch integration test), typecheck/lint/format, plugin buildsdocs/remote-control.mdPlan: vault "2026-07-16 - Videorc Remote Control Stream Deck Plan" (RC0–RC5). Physical-deck by-eye = owner/franky47 acceptance; Elgato Marketplace submission is a follow-up.
🤖 Generated with Claude Code
Summary by CodeRabbit