feat(remote-access): desktop integration + settings UI (2/2)#1334
feat(remote-access): desktop integration + settings UI (2/2)#1334Astro-Han wants to merge 1 commit into
Conversation
Electron main/IPC + Go bridge lifecycle, SolidJS Remote Access settings page (platform grid, per-platform config, status), i18n, opencode event-replay support, desktop-smoke CI. Builds on the remote-bridge module. Split out of #1275 as PR 2/2.
📝 WalkthroughWalkthroughAdds a full remote access bridge feature: shared ChangesRemote Access Bridge Feature
Event Replay Expansion
Sequence Diagram(s)sequenceDiagram
participant RemotePage
participant preload_api as Preload API
participant ipcMain as ipcMain (Electron)
participant RemoteBridgeController
participant pawwork_remote_bridge as pawwork-remote-bridge (Go)
RemotePage->>preload_api: remoteAccessStart(config)
preload_api->>ipcMain: invoke("remote-access:start", config)
ipcMain->>RemoteBridgeController: deps.remoteAccess.start(config)
RemoteBridgeController->>RemoteBridgeController: validate audience (hasRemoteAudience)
RemoteBridgeController->>pawwork_remote_bridge: spawn with -config - args
RemoteBridgeController->>pawwork_remote_bridge: write runtime JSON to stdin
pawwork_remote_bridge-->>RemoteBridgeController: stdout/stderr/exit events
RemoteBridgeController-->>ipcMain: RemoteAccessStatus {state: "running"}
ipcMain-->>preload_api: RemoteAccessStatus
preload_api-->>RemotePage: RemoteAccessStatus
RemotePage->>RemotePage: start polling status via setInterval
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 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 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 |
github-actions
Bot
added
ci
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/app.tsx, packages/app/src/desktop-api-contract.ts, packages/app/src/desktop-api.ts, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts, packages/app/src/pages/settings/remote-behavior.test.ts, packages/app/src/pages/settings/remote-platform-icons.ts, packages/app/src/pages/settings/remote-platforms.tsx, packages/app/src/pages/settings/remote-source.test.ts, packages/app/src/pages/settings/remote.tsx, packages/app/src/pages/settings/settings-shell.tsx, packages/desktop-electron/src/main/index-sidecar-source.test.ts, packages/desktop-electron/src/main/index.ts, packages/desktop-electron/src/main/ipc.ts, packages/desktop-electron/src/main/remote-access-ipc-source.test.ts, packages/desktop-electron/src/main/remote-bridge.test.ts, packages/desktop-electron/src/main/remote-bridge.ts, packages/desktop-electron/src/preload/index.ts, packages/desktop-electron/src/preload/types.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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Remote Access feature to connect various chat platforms (such as Feishu, Slack, and Discord) to the desktop PawWork session. It implements a new settings page, adds IPC channels, and introduces a controller to manage the lifecycle of the Go remote bridge sidecar process. Additionally, event replay support is added for completed assistant text and question tool updates. A critical issue was identified regarding the asynchronous cleanup in killSidecar during Electron's synchronous quit events, which could leave the remote bridge process orphaned if the default quit behavior is not prevented.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| app.on("before-quit", () => { | ||
| killSidecar() | ||
| void killSidecar() | ||
| }) | ||
|
|
||
| app.on("will-quit", () => { | ||
| killSidecar() | ||
| void killSidecar() | ||
| }) |
There was a problem hiding this comment.
The killSidecar function is now asynchronous because it awaits remoteAccess.stop(), which performs asynchronous cleanup (including waiting for the child process to exit).
In Electron, the before-quit and will-quit events are synchronous. If you do not call event.preventDefault(), Electron will proceed to quit immediately without waiting for any pending promises. This means killSidecar() will be cut short, potentially leaving the Go remote bridge sidecar process orphaned and running in the background.
To fix this, we should prevent the default quit behavior, await the asynchronous cleanup, and then explicitly call app.quit() once the cleanup is complete.
let isQuitting = false
app.on("before-quit", (event) => {
if (!isQuitting) {
event.preventDefault()
isQuitting = true
void killSidecar().finally(() => {
app.quit()
})
}
})![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/settings/remote.tsx`:
- Around line 193-199: The promise chain in the polling interval created by
setInterval within the createEffect hook is missing error handling. The void
Promise.resolve(refetch()).then() chain lacks a .catch() handler, which means if
refetch() rejects or if the mutation fails, the rejection will go unhandled and
repeat with every interval tick. Add a .catch() handler to the promise chain to
properly handle any errors that occur during the refetch operation or subsequent
mutation, preventing repeated unhandled promise rejections in the polling loop.
In `@packages/desktop-electron/src/main/index.ts`:
- Around line 434-440: The before-quit and will-quit event handlers call
killSidecar() without awaiting it, creating a race condition where the app can
exit before the async cleanup completes. Update both the app.on("before-quit",
...) and app.on("will-quit", ...) handlers to properly await the killSidecar()
function or use the same .finally() pattern employed in the SIGINT/SIGTERM,
quitAndInstall, and relaunch quit paths to ensure remoteAccess.stop() completes
before the app exits.
In `@packages/desktop-electron/src/main/remote-bridge.ts`:
- Around line 245-261: The normalizeConfig function does not validate the
platform value against supported values, allowing any non-empty string to be
accepted and causing failures later during bridge startup. Modify the platform
assignment logic to check that raw.platform is not only a non-empty string, but
also matches one of the supported platform values. If the platform value is
invalid or unsupported, fall back to defaultConfig.platform instead. This early
validation will catch malformed persisted configuration values before they cause
issues downstream.
- Around line 211-227: The waitForExitAfterKill function currently resolves
after a fixed 2-second timeout regardless of whether the child process actually
exits, which can leave orphaned sidecar processes. Refactor this function to
escalate the termination signal: first attempt to kill the child with SIGTERM
(the default signal), and if the process does not exit within a reasonable
timeout (e.g., 1 second), escalate to SIGKILL by calling child.kill("SIGKILL")
to force termination. Only resolve the promise after receiving the actual exit
event or after a final timeout following the SIGKILL attempt. This ensures that
processes that ignore SIGTERM are properly terminated before the function
returns.
In `@packages/opencode/src/server/event-replay.ts`:
- Around line 67-74: The replay predicates only verify that identifier fields
are strings but do not check if they are empty, allowing empty strings to pass
validation. Add checks to ensure that textPart.sessionID (at lines 67-74) and
the corresponding identifier fields (at lines 93-96) are not empty strings by
adding additional conditions like !== "" alongside the existing typeof checks.
This ensures only non-empty identifiers are considered replayable and prevents
malformed envelopes from leaking into replay.
🪄 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: ef6a276f-ff07-4040-b7ff-7187e97bb896
📒 Files selected for processing (29)
.github/workflows/desktop-smoke.ymlpackages/app/e2e/settings/settings-shell.spec.tspackages/app/e2e/snap/settings-shell.snap.tspackages/app/src/app.tsxpackages/app/src/desktop-api-contract.tspackages/app/src/desktop-api.tspackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/settings/remote-behavior.test.tspackages/app/src/pages/settings/remote-platform-icons.tspackages/app/src/pages/settings/remote-platforms.tsxpackages/app/src/pages/settings/remote-source.test.tspackages/app/src/pages/settings/remote.tsxpackages/app/src/pages/settings/settings-shell.tsxpackages/desktop-electron/scripts/build-remote-bridge.tspackages/desktop-electron/scripts/desktop-smoke-workflow-contract.test.tspackages/desktop-electron/scripts/prebuild.tspackages/desktop-electron/scripts/predev.tspackages/desktop-electron/src/main/index-sidecar-source.test.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/main/remote-access-ipc-source.test.tspackages/desktop-electron/src/main/remote-bridge.test.tspackages/desktop-electron/src/main/remote-bridge.tspackages/desktop-electron/src/preload/index.tspackages/desktop-electron/src/preload/types.tspackages/opencode/src/server/event-replay.test.tspackages/opencode/src/server/event-replay.tspackages/opencode/test/github/desktop-smoke-workflow.test.ts
| createEffect(() => { | ||
| if (!isRunning()) return | ||
| const timer = setInterval(() => { | ||
| void Promise.resolve(refetch()).then((next) => { | ||
| if (next) mutate(next) | ||
| }) | ||
| }, REMOTE_ACCESS_STATUS_POLL_MS) |
There was a problem hiding this comment.
Handle polling promise rejections to prevent unhandled errors.
Line 196 starts a polling promise chain without a rejection handler. If status refresh fails, this can emit repeated unhandled rejections while the interval is active.
Suggested fix
const timer = setInterval(() => {
- void Promise.resolve(refetch()).then((next) => {
- if (next) mutate(next)
- })
+ void Promise.resolve(refetch())
+ .then((next) => {
+ if (next) mutate(next)
+ })
+ .catch(() => {
+ // Resource error state is already tracked by createResource; avoid unhandled rejection noise.
+ })
}, REMOTE_ACCESS_STATUS_POLL_MS)📝 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.
| createEffect(() => { | |
| if (!isRunning()) return | |
| const timer = setInterval(() => { | |
| void Promise.resolve(refetch()).then((next) => { | |
| if (next) mutate(next) | |
| }) | |
| }, REMOTE_ACCESS_STATUS_POLL_MS) | |
| createEffect(() => { | |
| if (!isRunning()) return | |
| const timer = setInterval(() => { | |
| void Promise.resolve(refetch()) | |
| .then((next) => { | |
| if (next) mutate(next) | |
| }) | |
| .catch(() => { | |
| // Resource error state is already tracked by createResource; avoid unhandled rejection noise. | |
| }) | |
| }, REMOTE_ACCESS_STATUS_POLL_MS) |
🤖 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/pages/settings/remote.tsx` around lines 193 - 199, The
promise chain in the polling interval created by setInterval within the
createEffect hook is missing error handling. The void
Promise.resolve(refetch()).then() chain lacks a .catch() handler, which means if
refetch() rejects or if the mutation fails, the rejection will go unhandled and
repeat with every interval tick. Add a .catch() handler to the promise chain to
properly handle any errors that occur during the refetch operation or subsequent
mutation, preventing repeated unhandled promise rejections in the polling loop.
| app.on("before-quit", () => { | ||
| killSidecar() | ||
| void killSidecar() | ||
| }) | ||
|
|
||
| app.on("will-quit", () => { | ||
| killSidecar() | ||
| void killSidecar() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files packages/desktop-electron/src/main/Repository: Astro-Han/pawwork
Length of output: 5500
🏁 Script executed:
head -20 packages/desktop-electron/src/main/index.tsRepository: Astro-Han/pawwork
Length of output: 863
🏁 Script executed:
sed -n '430,445p' packages/desktop-electron/src/main/index.tsRepository: Astro-Han/pawwork
Length of output: 375
🏁 Script executed:
sed -n '695,720p' packages/desktop-electron/src/main/index.tsRepository: Astro-Han/pawwork
Length of output: 758
🏁 Script executed:
sed -n '195,220p' packages/desktop-electron/src/main/index.tsRepository: Astro-Han/pawwork
Length of output: 933
🏁 Script executed:
sed -n '440,450p' packages/desktop-electron/src/main/index.tsRepository: Astro-Han/pawwork
Length of output: 279
🏁 Script executed:
sed -n '610,625p' packages/desktop-electron/src/main/index.tsRepository: Astro-Han/pawwork
Length of output: 470
Inconsistent async cleanup handling in app quit handlers — await killSidecar() in before-quit and will-quit.
Lines 434-435 and 438-439 call killSidecar() without awaiting, but killSidecar is async and awaits remoteAccess.stop(). Other quit paths (SIGINT/SIGTERM at line 442, quitAndInstall at line 199, relaunch at line 615) correctly use .finally() to wait for cleanup before exiting. The before-quit and will-quit handlers should follow the same pattern to prevent a race where the app exits before the async cleanup completes.
Proposed fix
+let quittingCleanupStarted = false
+
+async function shutdownSidecarOnce() {
+ if (quittingCleanupStarted) return
+ quittingCleanupStarted = true
+ await killSidecar()
+}
+
- app.on("before-quit", () => {
- void killSidecar()
- })
+ app.on("before-quit", (event: Event) => {
+ if (quittingCleanupStarted) return
+ event.preventDefault()
+ void shutdownSidecarOnce().finally(() => app.quit())
+ })
- app.on("will-quit", () => {
- void killSidecar()
- })
+ app.on("will-quit", (event: Event) => {
+ if (quittingCleanupStarted) return
+ event.preventDefault()
+ void shutdownSidecarOnce().finally(() => app.quit())
+ })🤖 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/desktop-electron/src/main/index.ts` around lines 434 - 440, The
before-quit and will-quit event handlers call killSidecar() without awaiting it,
creating a race condition where the app can exit before the async cleanup
completes. Update both the app.on("before-quit", ...) and app.on("will-quit",
...) handlers to properly await the killSidecar() function or use the same
.finally() pattern employed in the SIGINT/SIGTERM, quitAndInstall, and relaunch
quit paths to ensure remoteAccess.stop() completes before the app exits.
| function waitForExitAfterKill(child: RemoteBridgeChild) { | ||
| return new Promise<void>((resolve) => { | ||
| let settled = false | ||
| let timer: ReturnType<typeof setTimeout> | undefined | ||
| const done = () => { | ||
| if (settled) return | ||
| settled = true | ||
| if (timer) clearTimeout(timer) | ||
| child.off("exit", done) | ||
| child.off("error", done) | ||
| resolve() | ||
| } | ||
| child.once("exit", done) | ||
| child.once("error", done) | ||
| timer = setTimeout(done, 2000) | ||
| if (!child.kill || child.kill() === false) done() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current teardown behavior (timeout resolve without hard-kill escalation)
rg -n "function waitForExitAfterKill|setTimeout\\(|child\\.kill\\(" packages/desktop-electron/src/main/remote-bridge.ts -C3
# Verify whether tests currently cover hard-kill escalation paths
rg -n "SIGKILL|waitForExitAfterKill" packages/desktop-electron/src/main/remote-bridge.test.ts -C3Repository: Astro-Han/pawwork
Length of output: 526
🏁 Script executed:
# Check if test file exists and find test-related files
find packages/desktop-electron/src/main -name "*.test.*" -o -name "*.spec.*" | head -20
# If test file doesn't exist, check what test files do exist
find packages/desktop-electron -name "*.test.*" -o -name "*.spec.*" | head -20Repository: Astro-Han/pawwork
Length of output: 2527
🏁 Script executed:
# Examine RemoteBridgeChild type definition to understand what child.kill() refers to
rg -n "type RemoteBridgeChild|interface RemoteBridgeChild" packages/desktop-electron/src/main/remote-bridge.ts -A10
# Check how RemoteBridgeChild instances are created
rg -n "spawn|exec|fork" packages/desktop-electron/src/main/remote-bridge.ts -B2 -A2Repository: Astro-Han/pawwork
Length of output: 1365
🏁 Script executed:
# Verify current kill() type signature and behavior context
rg -n "kill\(" packages/desktop-electron/src/main/remote-bridge.ts -B3 -A1 | head -40Repository: Astro-Han/pawwork
Length of output: 234
🏁 Script executed:
# Check if any tests reference or import remote-bridge
rg -l "remote-bridge" packages/desktop-electron/src/main/*.test.ts packages/desktop-electron/src/**/*.test.ts 2>/dev/null | head -10
# Check for any start/stop lifecycle tests that might exercise the bridge
rg -n "waitForExitAfterKill\|bridge.*stop\|bridge.*start" packages/desktop-electron -t ts -l 2>/dev/null | head -10Repository: Astro-Han/pawwork
Length of output: 311
🏁 Script executed:
# Examine if there's error handling or cleanup elsewhere for orphaned processes
rg -n "orphan|cleanup|waitForExit" packages/desktop-electron/src/main/remote-bridge.ts -C2
# Check the full stop/cleanup implementation to understand the context
rg -n "stop:|stop\(\)" packages/desktop-electron/src/main/remote-bridge.ts -A15 | head -50Repository: Astro-Han/pawwork
Length of output: 541
🏁 Script executed:
# Verify Node.js child_process.kill() behavior - check if there's documentation or comments about signal handling
rg -n "child\.kill|SIGTERM|SIGKILL" packages/desktop-electron/src/main/remote-bridge.ts -C3Repository: Astro-Han/pawwork
Length of output: 245
🏁 Script executed:
# Read the test file to check coverage of waitForExitAfterKill
cat -n packages/desktop-electron/src/main/remote-bridge.test.ts | head -100Repository: Astro-Han/pawwork
Length of output: 4120
🏁 Script executed:
# Check the full test file size and then examine sections related to stop/cleanup
wc -l packages/desktop-electron/src/main/remote-bridge.test.ts
# Search for waitForExitAfterKill tests
rg -n "waitForExitAfterKill\|stop\(" packages/desktop-electron/src/main/remote-bridge.test.ts -A10 -B2Repository: Astro-Han/pawwork
Length of output: 122
🏁 Script executed:
# Check if there's any mocking or spy on child.kill() to see test behavior
rg -n "mock|jest|kill|SIGKILL|SIGTERM" packages/desktop-electron/src/main/remote-bridge.test.ts -C2Repository: Astro-Han/pawwork
Length of output: 628
🏁 Script executed:
# Read more of the test to understand how waitForExitAfterKill behavior is tested
sed -n '150,250p' packages/desktop-electron/src/main/remote-bridge.test.tsRepository: Astro-Han/pawwork
Length of output: 3908
🏁 Script executed:
# Check if there's any test that mocks a non-responsive kill scenario
sed -n '1,337p' packages/desktop-electron/src/main/remote-bridge.test.ts | grep -n "kill\|exit\|SIGKILL" -B3 -A3Repository: Astro-Han/pawwork
Length of output: 738
🏁 Script executed:
# Verify the actual implementation signature one more time and check if any other callers pass signals
rg -n "\.kill\(" packages/desktop-electron/src/main/remote-bridge.tsRepository: Astro-Han/pawwork
Length of output: 119
Escalate timed-out shutdown to prevent orphaned bridge processes.
At Line 225, the promise resolves after 2s timeout even if the child never exits, which can leave a live sidecar while later starts proceed. The test suite masks this issue because FakeChild.kill() immediately emits "exit"; real Node.js processes may ignore SIGTERM and require SIGKILL escalation.
Suggested fix
type RemoteBridgeChild = EventEmitter & {
stdout?: EventEmitter
stderr?: EventEmitter
stdin?: {
end: (data: string) => void
}
- kill?: () => boolean
+ kill?: (signal?: NodeJS.Signals | number) => boolean
}
function waitForExitAfterKill(child: RemoteBridgeChild) {
return new Promise<void>((resolve) => {
@@
- timer = setTimeout(done, 2000)
+ timer = setTimeout(() => {
+ if (settled) return
+ child.kill?.("SIGKILL")
+ timer = setTimeout(done, 2000)
+ }, 2000)
if (!child.kill || child.kill() === false) done()
})
}🤖 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/desktop-electron/src/main/remote-bridge.ts` around lines 211 - 227,
The waitForExitAfterKill function currently resolves after a fixed 2-second
timeout regardless of whether the child process actually exits, which can leave
orphaned sidecar processes. Refactor this function to escalate the termination
signal: first attempt to kill the child with SIGTERM (the default signal), and
if the process does not exit within a reasonable timeout (e.g., 1 second),
escalate to SIGKILL by calling child.kill("SIGKILL") to force termination. Only
resolve the promise after receiving the actual exit event or after a final
timeout following the SIGKILL attempt. This ensures that processes that ignore
SIGTERM are properly terminated before the function returns.
| function normalizeConfig(value: unknown): RemoteAccessConfig { | ||
| if (!value || typeof value !== "object") return defaultConfig | ||
| const raw = value as Partial<RemoteAccessConfig> | ||
| const platform = typeof raw.platform === "string" && raw.platform ? raw.platform : defaultConfig.platform | ||
| const options: Record<string, unknown> = {} | ||
| if (raw.options && typeof raw.options === "object") { | ||
| for (const [key, option] of Object.entries(raw.options)) { | ||
| const normalized = normalizeOptionValue(option) | ||
| if (normalized !== undefined) options[key] = normalized | ||
| } | ||
| } | ||
| return { | ||
| enabled: raw.enabled === true, | ||
| platform, | ||
| options, | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate platform against supported values during normalization.
Line 248 accepts any non-empty string; malformed persisted values bypass early validation and fail late in bridge startup.
Suggested fix
- const platform = typeof raw.platform === "string" && raw.platform ? raw.platform : defaultConfig.platform
+ const platform =
+ typeof raw.platform === "string" && (REMOTE_ACCESS_PLATFORMS as readonly string[]).includes(raw.platform)
+ ? raw.platform
+ : defaultConfig.platform🤖 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/desktop-electron/src/main/remote-bridge.ts` around lines 245 - 261,
The normalizeConfig function does not validate the platform value against
supported values, allowing any non-empty string to be accepted and causing
failures later during bridge startup. Modify the platform assignment logic to
check that raw.platform is not only a non-empty string, but also matches one of
the supported platform values. If the platform value is invalid or unsupported,
fall back to defaultConfig.platform instead. This early validation will catch
malformed persisted configuration values before they cause issues downstream.
| textPart.type === "text" && | ||
| textPart.ignored !== true && | ||
| typeof textPart.sessionID === "string" && | ||
| typeof textPart.text === "string" && | ||
| textPart.text !== "" && | ||
| textPart.time?.end !== undefined && | ||
| textPart.time.end !== null | ||
| ) |
There was a problem hiding this comment.
Harden replay predicates to reject empty identifier fields.
Line 69 and Lines 93–95 only check typeof ... === "string", so "" is treated as replayable. That diverges from the upstream event contract and can leak malformed message.part.updated envelopes into replay.
Proposed fix
return (
textPart.type === "text" &&
textPart.ignored !== true &&
- typeof textPart.sessionID === "string" &&
+ typeof textPart.sessionID === "string" &&
+ textPart.sessionID !== "" &&
typeof textPart.text === "string" &&
textPart.text !== "" &&
textPart.time?.end !== undefined &&
textPart.time.end !== null
)
@@
return (
toolPart.type === "tool" &&
toolPart.tool === "question" &&
- typeof toolPart.sessionID === "string" &&
- typeof toolPart.messageID === "string" &&
- typeof toolPart.callID === "string"
+ typeof toolPart.sessionID === "string" &&
+ toolPart.sessionID !== "" &&
+ typeof toolPart.messageID === "string" &&
+ toolPart.messageID !== "" &&
+ typeof toolPart.callID === "string" &&
+ toolPart.callID !== ""
)
}Also applies to: 93-96
🤖 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/opencode/src/server/event-replay.ts` around lines 67 - 74, The
replay predicates only verify that identifier fields are strings but do not
check if they are empty, allowing empty strings to pass validation. Add checks
to ensure that textPart.sessionID (at lines 67-74) and the corresponding
identifier fields (at lines 93-96) are not empty strings by adding additional
conditions like !== "" alongside the existing typeof checks. This ensures only
non-empty identifiers are considered replayable and prevents malformed envelopes
from leaking into replay.
1 participant
Summary
Desktop integration + Remote Access settings UI — PR 2 of 2, completing the mobile companion feature on top of the
packages/remote-bridge/Go module landed in #1275.packages/desktop-electron) — spawns and supervises the Go bridge as a sidecar (remote-bridge.ts), wires IPC (ipc.ts,preload/), builds the bridge binary in prebuild/predev, and adds adesktop-smokeCI workflow.packages/app) — Remote Access page (pages/settings/remote.tsx): platform grid, per-platform config (connection fields, secrets, allow-list), live status; platform icons; en/zh i18n; wired into the settings shell.packages/opencode) — event-replay support the bridge consumes on reconnect (server/event-replay.ts).~2.1k insertions across
desktop-electron,app, andopencode, plus thedesktop-smokeworkflow. No changes topackages/remote-bridge/(that is #1275's surface).Why
Issue #1188 (mobile companion): monitor and interact with running agents from chat platforms on your phone. PR 1 (#1275) landed the headless Go bridge; this PR wires it into the desktop app and gives users a settings surface to configure platforms and credentials, completing the feature.
Related Issue
Closes #1188. (PR 2 of 2; PR 1 was #1275.)
Human Review Status
Pending
Review Focus
desktop-electron/src/main/remote-bridge.ts): spawn / supervise / teardown of the Go binary, path resolution across dev vs packaged builds, and failure handling.ipc.ts,preload/): what the renderer can drive, and that config/secrets cross the boundary safely.app/src/pages/settings/remote.tsx): per-platform config correctness — required fields, default seeding, secret handling, allow-list — against what each cc-connect adapter actually reads.opencode/src/server/event-replay.ts): correctness of the replay the bridge consumes on reconnect.Risk Notes
index-sidecar-source+remote-bridgetests.passwordand are persisted via the bridge's config.desktop-smokeworkflow exercises the wiring. Considered for both OSes.settings-shell.spec.ts/settings-shell.snap.ts) cover the shell layout; a human should attach screenshots before sign-off.How To Verify
Rebased onto
devafter #1275 squash-merged; the redundant bridge commit and a duplicatebuild-workflow.test.tsblock (already indev) were dropped.Screenshots or Recordings
Pending human capture — Remote Access settings page (platform grid + per-platform config). e2e snapshot tests cover the settings shell layout in the meantime.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
Release Notes