feat(remote): multi-channel bridge foundation and sidebar surface - #1390
Conversation
|
Warning Review limit reached
More reviews will be available in 22 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe remote control feature is moved from a settings tab into a dedicated ChangesRemote control surface and bridge refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant RemoteSurface
participant DialogConnect
participant PreloadAPI
participant RemoteBridgeRuntime
participant TelegramPairer
participant CredentialStore
User->>RemoteSurface: Click "Connect" (Telegram)
RemoteSurface->>DialogConnect: open (lazy import)
User->>DialogConnect: Enter bot token → Submit
DialogConnect->>PreloadAPI: startPairing("telegram", {token})
PreloadAPI->>RemoteBridgeRuntime: IPC remote:start-pairing
RemoteBridgeRuntime->>TelegramPairer: pair(start, signal, onProgress)
TelegramPairer-->>RemoteBridgeRuntime: onProgress(awaitingBind)
RemoteBridgeRuntime-->>PreloadAPI: broadcast remote:pairing
PreloadAPI-->>DialogConnect: onPairing(awaitingBind) → bind phase
TelegramPairer-->>RemoteBridgeRuntime: onProgress(captured)
RemoteBridgeRuntime-->>PreloadAPI: broadcast remote:pairing
PreloadAPI-->>DialogConnect: onPairing(captured) → confirm phase
User->>DialogConnect: Click "Allow"
DialogConnect->>PreloadAPI: confirmPairing("telegram")
PreloadAPI->>RemoteBridgeRuntime: IPC remote:confirm-pairing
RemoteBridgeRuntime->>CredentialStore: save([account])
RemoteBridgeRuntime->>RemoteBridgeRuntime: startBridge + supervisePlatforms
RemoteBridgeRuntime-->>PreloadAPI: broadcast remote:status
PreloadAPI-->>RemoteSurface: onStatus → connected
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/app.tsx, packages/app/src/components/dialog-connect-remote.tsx, packages/app/src/desktop-api-contract.ts, packages/app/src/desktop-api.ts, packages/app/src/i18n/en.ts, packages/app/src/i18n/remote-placeholders.test.ts, packages/app/src/i18n/zh.ts, packages/app/src/pages/layout.tsx, packages/app/src/pages/layout/pawwork-sidebar-top.tsx, packages/app/src/pages/layout/pawwork-sidebar.tsx, packages/app/src/pages/layout/surface-routes.ts, packages/app/src/pages/remote/connect-toast.test.ts, packages/app/src/pages/remote/connect-toast.ts, packages/app/src/pages/remote/platform-marks.tsx, packages/app/src/pages/remote/remote-connect-dialog.tsx, packages/app/src/pages/remote/remote-route.tsx, packages/app/src/pages/remote/remote-surface.tsx, packages/app/src/pages/settings/remote-connect-toast.test.ts, packages/app/src/pages/settings/remote.tsx, packages/app/src/pages/settings/settings-shell.tsx, packages/desktop-electron/src/main/index.ts, packages/desktop-electron/src/main/ipc/remote.ts, packages/desktop-electron/src/main/remote-bridge.test.ts, packages/desktop-electron/src/main/remote-bridge.ts, packages/desktop-electron/src/main/remote-credentials.test.ts, packages/desktop-electron/src/main/remote-credentials.ts, packages/desktop-electron/src/main/remote-pairers.ts, packages/desktop-electron/src/preload/index.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
Add a per-platform supervisor so each channel runs in its own restart loop: a dead channel reports degraded and retries with exponential backoff while the others keep serving, replacing the old all-or-nothing start where one platform's rejection tore the whole bridge down. The gateway wires the supervisor and keeps the event stream as the single fatal path. Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
Generalize the desktop remote contract from a single Telegram credential to a channels[] array with event-driven pairing (awaitingBind -> captured -> confirm). RemotePlatform is a discriminated union seeded with telegram, so each new platform adds its own account shape and pairing variant rather than widening a flat record. Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
Add the bridge runtime behind a PlatformPairer registry seam: pair an account, build its Platform, persist credentials as an account list, and drive status / connect / disconnect over IPC. Telegram is the only registered pairer (buildRemotePairers); new platforms register their own there. Credentials stay in the main process; the renderer only sees masked per-channel status. Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
Move remote control out of Settings into a dedicated sidebar surface with its own route, connect dialog, platform marks, and connect toast. Drop the old settings page, dialog component, and settings-remote snap; add a remote-surface snap covering the telegram connect flow end to end. Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
18cb73e to
26464f9
Compare
Disconnecting the last channel rewrote an empty encrypted list via save([]) but never removed the bridge state file, so a reconnect inherited stale session pointers / event cursor; and save([]) requires the OS keyring, so a locked keyring blocked the revoke outright. Restore a removal path: when no channels remain, clear() the credentials file (a delete needs no encryption) and rm the state file. Covers Codex's keyring-locked disconnect and the state-inheritance regression. Add tests: state-file removal on last disconnect, revoke while secure storage is unavailable, and clear() needing no encryption. Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
packages/app/src/desktop-api-contract.ts (1)
135-135: ⚡ Quick winMake pairing cancellation platform-scoped before freezing the IPC contract.
startPairing,confirmPairing,disconnect, and pairing events are platform-scoped, butcancelPairing()is global. When another platform is added, closing/cancelling one pairing flow can abort whichever global flow is current, and fixing that later requires another IPC contract break.Suggested contract direction
- cancelPairing(): Promise<void> + cancelPairing(platform: RemotePlatform): Promise<void>🤖 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 `@packages/app/src/desktop-api-contract.ts` at line 135, The cancelPairing() method is currently global while related pairing methods like startPairing, confirmPairing, and disconnect are platform-scoped. To fix this, modify the cancelPairing() method signature to accept a platform parameter (consistent with the other platform-scoped pairing methods) so that cancelling a pairing flow only affects the specific platform being cancelled, preventing cross-platform pairing conflicts.
🤖 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 `@packages/app/src/pages/remote/remote-connect-dialog.tsx`:
- Line 75: Replace the hardcoded "telegram" string in the api.startPairing()
call with the platform prop or variable that is available in the component. This
will make the code platform-agnostic and allow it to work with different
platforms without requiring changes for each new platform added in the future.
In `@packages/app/src/pages/remote/remote-surface.tsx`:
- Around line 233-241: The handleDisconnect function lacks explicit error
handling for the disconnect call on line 237, which can lead to unhandled
rejections and silent failures. Add a catch block after the await statement to
handle any errors thrown by the disconnect operation, and ensure the
dialog.close() call only executes on successful disconnection. Keep the
setBusy(false) in the finally block to always reset the busy state regardless of
success or failure, but move the dialog.close() into the try block so it only
closes after a successful disconnect, allowing the user to retry if the
disconnect fails.
- Around line 61-66: The onMount hook in remote-surface.tsx has a race condition
where the getStatus() snapshot fetch can resolve after onStatus() live updates,
causing stale data to overwrite newer state. To fix this, modify the applyStatus
function to track when updates occur (using a timestamp or version counter) and
only apply the getStatus() response if it represents data newer than what has
already been applied via the onStatus() subscription. This ensures that
late-arriving snapshot data will not overwrite more recent live channel updates.
In `@packages/desktop-electron/src/main/remote-bridge.test.ts`:
- Around line 223-225: The RemoteBridgeRuntime constructor is being called with
duplicate deps(...) arguments at the test setup location around line 223-225.
The constructor expects a single RemoteBridgeDeps object, but it's currently
receiving two identical deps(...) calls which causes type-checking to fail.
Remove one of the duplicate deps(...) constructor arguments, keeping only a
single deps(...) call that wraps the buildApp mock object with the revoked token
error behavior.
In `@packages/desktop-electron/src/main/remote-bridge.ts`:
- Around line 157-177: The catch block at line 171 and the check at line 175
emit "cancelled" events whenever the pairing controller changes, even though a
newer pairing has superseded the old one. To fix this, update both conditions to
only emit "cancelled" when the current pairing controller is still the one that
initiated this attempt (meaning this.pairingAc === ac), rather than when a newer
controller exists. This prevents stale cancellation events from old pairing
attempts from being consumed by the current dialog after a new pairing has
started.
- Around line 219-234: The state file is being deleted while the bridge is still
running, allowing the platform to recreate stale pointers after deletion. Move
the `await this.startBridge()` call to execute before the conditional block that
contains the `rmSync` call, so the bridge is torn down first and cannot rewrite
the file after it is deleted. This ensures that when there are no accounts
remaining, the bridge is stopped before removing the state file.
In `@packages/remote-bridge/README.md`:
- Line 17: The fenced code block containing the ASCII diagram (showing "phone
chat app ──outbound──▶ Platform adapter") is missing a language identifier,
which triggers the markdownlint MD040 rule. Add the language identifier "text"
to the opening fence of this code block by changing ``` to ```text to properly
document the diagram content type.
In `@packages/remote-bridge/src/supervisor.ts`:
- Around line 80-90: The supervised start race is not self-contained because the
abort listener created by onAbort(signal) remains attached after the race
settles, and synchronous throws from platform.start() can escape without being
properly reported. Wrap the entire startPromise creation and Promise.race in a
try-catch block to handle synchronous errors from platform.start() and report
them as degraded retryable failures. Additionally, ensure the abort listener
from onAbort(signal) is properly cleaned up when the race completes, regardless
of which promise wins, by either using an AbortController to manage the signal
scope or attaching an explicit cleanup handler that runs after the Promise.race
settles. This same pattern should be applied to the locations referenced at
lines 100-103.
---
Nitpick comments:
In `@packages/app/src/desktop-api-contract.ts`:
- Line 135: The cancelPairing() method is currently global while related pairing
methods like startPairing, confirmPairing, and disconnect are platform-scoped.
To fix this, modify the cancelPairing() method signature to accept a platform
parameter (consistent with the other platform-scoped pairing methods) so that
cancelling a pairing flow only affects the specific platform being cancelled,
preventing cross-platform pairing conflicts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 49a555f5-7b1a-4b10-a234-7897b0362fb1
📒 Files selected for processing (35)
packages/app/e2e/settings/settings-shell.spec.tspackages/app/e2e/snap/remote-surface.snap.tspackages/app/e2e/snap/settings-remote.snap.tspackages/app/src/app.tsxpackages/app/src/components/dialog-connect-remote.tsxpackages/app/src/desktop-api-contract.tspackages/app/src/desktop-api.tspackages/app/src/i18n/en.tspackages/app/src/i18n/remote-placeholders.test.tspackages/app/src/i18n/zh.tspackages/app/src/pages/layout.tsxpackages/app/src/pages/layout/pawwork-sidebar-top.tsxpackages/app/src/pages/layout/pawwork-sidebar.tsxpackages/app/src/pages/layout/surface-routes.tspackages/app/src/pages/remote/connect-toast.test.tspackages/app/src/pages/remote/connect-toast.tspackages/app/src/pages/remote/platform-marks.tsxpackages/app/src/pages/remote/remote-connect-dialog.tsxpackages/app/src/pages/remote/remote-route.tsxpackages/app/src/pages/remote/remote-surface.tsxpackages/app/src/pages/settings/remote-connect-toast.test.tspackages/app/src/pages/settings/remote.tsxpackages/app/src/pages/settings/settings-shell.tsxpackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc/remote.tspackages/desktop-electron/src/main/remote-bridge.test.tspackages/desktop-electron/src/main/remote-bridge.tspackages/desktop-electron/src/main/remote-credentials.test.tspackages/desktop-electron/src/main/remote-credentials.tspackages/desktop-electron/src/main/remote-pairers.tspackages/desktop-electron/src/preload/index.tspackages/remote-bridge/README.mdpackages/remote-bridge/src/gateway.tspackages/remote-bridge/src/supervisor.test.tspackages/remote-bridge/src/supervisor.ts
💤 Files with no reviewable changes (4)
- packages/app/src/components/dialog-connect-remote.tsx
- packages/app/src/pages/settings/remote-connect-toast.test.ts
- packages/app/src/pages/settings/remote.tsx
- packages/app/e2e/snap/settings-remote.snap.ts
awaitingBind ("now message the bot") was emitted before the token was
proven, so a bad token walked the user to the phone and only then errored
— contradicting the contract, which documents awaitingBind as token-
validated. captureFirstSender now takes an onValidated callback fired
after the proving drain + getMe; the pairer emits awaitingBind from it, so
the hint shows only once the token is good. Tests: onValidated fires only
after the drain, and never for a bad (401) token.
Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
The runtime ships the multi-channel paths (channels[], per-platform status, disconnect filtering) but they were only exercised with the single Telegram channel. Add a two-channel test (a stand-in 2nd platform cast past the single-member union): both connect, one degrading leaves the other connected, and disconnecting one removes only it. Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
The connect dialog optimistically jumped to "message the bot" the moment the token was submitted, so a bad token still walked the user to the phone before erroring — the main-process emit-after-validate fix alone wasn't visible. Add a "checking" phase: submit shows it, and only the awaitingBind event (now emitted after the token is validated) advances to bind. The snap drives that event to reach the bind shot, so its grid is unchanged. Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/remote-bridge/src/platforms/telegram.ts`:
- Around line 520-524: The onValidated callback is not guarded against
cancellation, which allows it to fire even after the operation has been aborted
right after getMe resolves. Before calling onValidated?.() in this location,
check if the abort signal has been triggered (checking the aborted state of the
signal) and only emit the callback if the operation has not been cancelled. This
prevents stale callbacks from executing for cancelled pairings.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cd1c467-6ff9-4550-ac7a-30d312108f95
📒 Files selected for processing (8)
packages/app/e2e/snap/remote-surface.snap.tspackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/remote/remote-connect-dialog.tsxpackages/desktop-electron/src/main/remote-bridge.test.tspackages/desktop-electron/src/main/remote-pairers.tspackages/remote-bridge/src/platforms/telegram.test.tspackages/remote-bridge/src/platforms/telegram.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/app/e2e/snap/remote-surface.snap.ts
- packages/app/src/i18n/zh.ts
- packages/desktop-electron/src/main/remote-pairers.ts
- packages/app/src/pages/remote/remote-connect-dialog.tsx
- packages/app/src/i18n/en.ts
- packages/desktop-electron/src/main/remote-bridge.test.ts
A platform that kept failing and retrying registered a fresh abort listener every attempt and never removed it, leaking one per retry on a long-degraded channel. Race start() against abort through raceStartOrAbort, which removes the listener as soon as start settles. Also run start() via Promise.resolve().then so a synchronous throw from a misbehaving adapter degrades and retries instead of tearing the supervisor down. Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
disconnect() deleted the state file and then stopped the bridge, so an in-flight inbound handler could write session pointers back after the file was gone — a reconnect would inherit them. Await stopBridge() first, then clear the credentials and remove the state file. Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
The remote surface read getStatus() and subscribed onStatus separately, so a slow snapshot could resolve after a live update and flip the page from connected back to disconnected. Extract subscribeRemoteStatus: subscribe first, apply the one-shot snapshot only if no live update has landed. Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
The connect dialog took a platform prop but called startPairing("telegram"),
so a future platform's dialog would pair Telegram. Pass the prop through, as
confirmPairing already does. (The title copy stays Telegram-only until a
second platform brings its own.)
Claude-Session: https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
A failed disconnect (e.g. a locked OS keyring) used to close the dialog anyway, falsely implying access was revoked while the credential was still saved. Resolve the attempt as data via attemptDisconnect, close only on ok, and otherwise show the cause inline so the user can retry or cancel. Claude-Session: https://claude.ai/code/session_01RFZp5SLzjC1CnqNtPsbNG7
The bind step's spinner read as generic loading. Replace it with a sonar
pulse rippling out from the Telegram mark (new --animate-radar primitive),
and split the copy into a prominent instruction ("message the bot from your
phone") and a quieter security note, so the state reads as "act on your phone
now" rather than "something is loading".
Claude-Session: https://claude.ai/code/session_01RFZp5SLzjC1CnqNtPsbNG7
captureFirstSender fired onValidated (the "message the bot" hint) right after getMe resolved, with no abort check between. A cancel that raced the getMe response would emit a stale bind hint into a closed dialog. Re-check the signal before signaling so a pairing cancelled during the identity fetch stays silent. Claude-Session: https://claude.ai/code/session_01RFZp5SLzjC1CnqNtPsbNG7
subscribeRemoteStatus left getStatus()'s promise unguarded: a failed IPC surfaced as an unhandled rejection, and a snapshot resolving after the caller unsubscribed could still apply. Add a rejection handler and a disposed guard; the live stream stays the source of truth. Claude-Session: https://claude.ai/code/session_01RFZp5SLzjC1CnqNtPsbNG7
A pairing attempt replaced by a newer startPairing still emitted `cancelled`, injecting a stray terminal event into the new attempt's event stream. Settle a superseded attempt silently (a user-initiated cancel still emits) so each attempt owns a clean event stream. Claude-Session: https://claude.ai/code/session_01RFZp5SLzjC1CnqNtPsbNG7
Summary
First of three flat PRs re-landing mobile-companion Wave 1 (the oversized #1379 was closed in favor of this split). This is the platform-agnostic foundation plus the remote-control UI relocation, Telegram-only. The WeChat and Feishu adapters follow as additive PRs from this foundation.
Part of #1188.
What's in it
On top of the merged Telegram foundation (#1339):
packages/remote-bridge/src/supervisor.ts) — each platform runs in its own restart loop with exponential backoff, so one channel goingdegradednever tears the bridge down. The PawWork event stream stays the single fatal path.desktop-electron) —RemoteBridgeRuntimedrives any number of channels through aPlatformPairerseam;buildRemotePairers()registers the concrete platforms (Telegram today). Adding a platform is a new pairer, not a runtime change.desktop-api-contract.ts) —RemoteStatuscarries achannels[]array and pairing is event-driven (startPairing→onPairing: bind → captured → confirm). Adding a platform needs no contract change.packages/app/src/pages/remote/*) — remote control moves from the Settings tab to a first-class sidebar surface with masked per-channel status and the scan-to-connect flow. The old Settings remote UI is removed.Why the foundation is multi-channel now
This PR is deliberately the shared substrate for a 3-PR sequence (Telegram → WeChat → Feishu), not a single-Telegram implementation that gets refactored later. The multi-channel seam is the deliverable:
PlatformPairerregistry, thechannels[]contract, and the per-platform supervisor in place, a new platform is a new pairer — no runtime or contract change. Shipping a hardcoded single-Telegram runtime now would force PR2 to refactor the runtime and IPC contract while adding WeChat: exactly the large, hard-to-review shape that got feat(remote): mobile companion Wave 1 — Feishu + WeChat scan-to-connect #1379 closed.degradedmust never tear the bridge down, which matters with a single channel too. The rest is one registry map and an array-shaped status.dev). The only multi-channel artifact beyond the seam is a test-only fakeprobeplatform (cast past the single-memberRemotePlatformunion) that proves the runtime's multi-channel invariants — independent per-channel status and disconnect isolation — without a second real platform.What's deliberately not here
dev— no SDK, no axios override, no bun patch, no qrcode.@larksuiteoapi/node-sdkvs a thin vendored long connection) is deferred to the Feishu PR, isolated from everything else.Verification
tsgotypecheck clean:remote-bridge,desktop-electron,app.remote-bridge141,desktop-electronremote 26,appremote 8 — including a two-channel runtime isolation test (Telegram + a test-only fake platform), pairing abort/cancel/supersede edge cases, and disconnect credential + state-file teardown.eslintclean on touched product files.bun run snap remote-surfacereviewed — telegram-only grid (disconnected / token / checking / bind / confirm / connected / disconnect / degraded); the bind step shows the "listening" pulse.Flat PR 1 of 3 (foundation+UI → WeChat → Feishu).
https://claude.ai/code/session_013LqJ6qQMaYJrsmz2jafRJd
Summary by CodeRabbit