feat: 支持 acp-link 包进行 acp 通用的 remote-control#292
Conversation
- 新增 ACP WebSocket handler (agent 注册、EventBus 订阅) - 新增 relay handler (前端 WS → acp-link 透传 + EventBus inbound 转发) - 新增 SSE event stream 供外部消费者订阅 channel group 事件 - ACP REST 接口无鉴权 (agents、channel-groups) - WebSocket 端点保留 token 鉴权 - SPA 路由 /acp/ 指向 acp.html Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 新增 /acp/ SPA 页面 (agent 列表 + 实时交互) - Agent 列表按 channel group 分组，显示在线状态 - 通过 RCS WebSocket relay 与 agent 通信 - Vite multi-page 构建 (index.html + acp.html) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- rcs-upstream 新增 messageHandler 转发非控制消息 - server.ts 新增虚拟 WS + relay client state 处理 relay ACP 消息 - newSession/loadSession 补充 mcpServers 参数 - 连接成功后显示 ACP Dashboard URL Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 删除 FileExplorer 组件 - ACPMain 移除 Files tab，仅保留 Chat 和 History - client.ts 移除 listDir/readFile/onFileChanges 等方法 - types.ts 移除 FileItem/FileContent/FileChange 等类型 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- store: EnvironmentRecord 增加 capabilities 字段、storeFindEnvironmentByMachineName 复用逻辑 - store: 新增 storeGetSessionOwners，支持未绑定 session 自动 claim - environment: registerEnvironment 支持 ACP 复用已有记录，返回 session_id - session: resolveOwnedWebSessionId 支持无 owner session 自动绑定 - acp-ws-handler: 新增 handleIdentify 支持 REST+WS 两步注册 - acp routes: /acp/relay 和 /acp/agents 支持 UUID 认证 - event-bus: 增加 error 类型 payload 日志 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- rcs-upstream: 新增 registerViaRest() 通过 POST /v1/environments/bridge 注册
- rcs-upstream: WS 连接后发送 identify 替代 register，携带 agentId
- rcs-upstream: 入口链接改为 /code/?sid=${sessionId} 实现用户绑定
- server: 修复心跳跳过 relay 虚拟连接的 bug
- server: maxSessions 配置传入 RCS upstream
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 新增 chat/ 组件: ChatView, ChatInput, MessageBubble, ToolCallGroup, PermissionPanel, SessionSidebar, CommandMenu - ACPMain: 重构支持完整 ACP 协议交互（session/prompt/permission） - rcs-chat-adapter: 统一 bridge session SSE 适配器 - ACPClient: 增强 session 管理、permission 流程、streaming 支持 - index.css: 新增 chat 相关样式、动画、布局 - useCommands: 新增快捷命令 hook Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 删除 acp.html、acp-main.tsx 入口文件和 pages/acp/ 目录 - SessionDetail: ACP session 在同一页面渲染 ACPSessionDetail 组件 - App.tsx: ?sid= 参数自动调用 apiBind 绑定用户 UUID - Dashboard: 统一 session 列表导航，ACP 显示紫色标签 - relay-client: 改用 UUID 认证替代 API token - EnvironmentList: 显示 workerType 标签（ACP Agent / Claude Code） - index.ts: 移除 /acp/ SPA 路由，vite.config 移除 acp 入口 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
📝 WalkthroughWalkthroughThis PR introduces comprehensive ACP (Agent Control Protocol) support across the platform. It adds a new Changes
Sequence DiagramssequenceDiagram
actor Browser
participant RCS as Remote Control<br/>Server
participant ACPLink as acp-link<br/>Proxy
participant Agent as Local<br/>Agent
Browser->>RCS: Connect WebSocket<br/>(/acp/ws)
activate RCS
Browser->>Browser: Register agent ID
RCS->>RCS: Track WS connection<br/>(agent_id, capabilities)
Browser->>RCS: Send Prompt<br/>(text/ContentBlock)
RCS->>RCS: Route to Agent WS
RCS->>ACPLink: Forward via proxy
activate ACPLink
ACPLink->>Agent: Spawn process<br/>+ establish ACP
activate Agent
ACPLink->>Agent: Send prompt<br/>(JSON-RPC)
Agent->>Agent: Process request
Agent->>ACPLink: Return response
ACPLink->>RCS: Relay response
deactivate Agent
deactivate ACPLink
RCS->>Browser: Stream updates<br/>(SessionUpdate)
Browser->>RCS: Permission Response
RCS->>RCS: Resolve pending<br/>permission
RCS->>ACPLink: Forward approval
ACPLink->>Agent: Send RPC response
sequenceDiagram
actor Frontend as Frontend<br/>User
participant WebApp as React App<br/>(web/src)
participant ACPClient as ACPClient<br/>(browser WebSocket)
participant RCS as RCS ACP<br/>Handler
participant Agent as Bound<br/>Agent WS
Frontend->>WebApp: Click "Connect"
WebApp->>WebApp: Create ACPClient<br/>(proxyUrl)
WebApp->>ACPClient: connect()
activate ACPClient
ACPClient->>RCS: Upgrade WebSocket<br/>(/ws + auth token)
RCS->>RCS: Generate wsId,<br/>track connection
RCS->>ACPClient: Connected
deactivate ACPClient
WebApp->>WebApp: Register handlers<br/>(session/update/model)
Frontend->>WebApp: Type message<br/>+ Submit
WebApp->>ACPClient: sendPrompt(text)
activate ACPClient
ACPClient->>RCS: Send proxy message<br/>(type: prompt)
RCS->>Agent: Forward to bound<br/>agent process
activate Agent
Agent->>Agent: Process via ACP SDK
Agent->>RCS: Stream updates<br/>(inbound events)
RCS->>RCS: Publish to channel<br/>group EventBus
RCS->>ACPClient: Send proxy response<br/>(SessionUpdate)
deactivate Agent
ACPClient->>WebApp: Handler fires<br/>(onSessionUpdate)
deactivate ACPClient
WebApp->>WebApp: Update thread/UI
Frontend->>Frontend: See response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Rationale: This is a major multi-package feature addition (~5000+ lines across new acp-link package and substantial remote-control-server/web extensions). The changes are heterogeneous—spanning CLI architecture, WebSocket transport, event-driven state management, REST/SSE handlers, store schema updates, and 50+ new React components with mixed complexity. While some components follow repetitive patterns (UI wrappers), each domain (transport, store, client library, hooks) requires separate reasoning and understanding of new protocol flows, permission/session mechanics, and ACP lifecycle management. The integration involves intricate state coordination (WebSocket → EventBus → SSE → React), non-trivial error handling (reconnect backoff, timeouts, graceful shutdown), and web-specific patterns (QR scanning, file upload compression, localStorage persistence). Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/remote-control-server/src/store.ts (1)
101-142:⚠️ Potential issue | 🟠 MajorReuse offline ACP records on reconnect.
storeMarkAcpAgentOffline()preserves the record for reconnect, butstoreCreateEnvironment()only reusesstatus === "active"records. A legacy ACP agent reconnecting after disconnect will create a duplicate environment for the same machine.Suggested fix
-/** Find an active environment by machineName (optionally filtered by workerType) */ +/** Find an environment by machineName (optionally filtered by workerType/status) */ export function storeFindEnvironmentByMachineName( machineName: string, workerType?: string, + statuses: string[] = ["active"], ): EnvironmentRecord | undefined { for (const rec of environments.values()) { - if (rec.machineName === machineName && rec.status === "active") { + if (rec.machineName === machineName && statuses.includes(rec.status)) { if (!workerType || rec.workerType === workerType) { return rec; } } @@ if (req.workerType === "acp" && req.machineName) { - const existing = storeFindEnvironmentByMachineName(req.machineName, "acp"); + const existing = storeFindEnvironmentByMachineName(req.machineName, "acp", [ + "active", + "offline", + ]);Also applies to: 388-402
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/store.ts` around lines 101 - 142, The ACP reconnect path in storeCreateEnvironment only reuses records with status === "active" causing duplicate environments for offline agents; change the lookup to find existing ACP records regardless of status (either by updating storeFindEnvironmentByMachineName to accept a status filter or adding a new finder like storeFindEnvironmentByMachineNameIncludingInactive) and, in the block where req.workerType === "acp" and req.machineName is used, locate the existing record even if offline, then "resurrect" it by setting status: "active", lastPollAt/updatedAt to new Date(), and merging maxSessions/bridgeId/capabilities/username as currently done; apply the same change to the other identical ACP-reuse block found later (the second occurrence).
🧹 Nitpick comments (33)
packages/remote-control-server/web/src/components/TaskPanel.tsx (1)
5-24: Consider basic modal accessibility.As an overlay dialog, this would benefit from:
role="dialog"+aria-modal="true"+aria-labelledbyon the panel, anaria-labelon the close×button (currently only an HTML entity glyph, which screen readers may announce as "times"), and an Escape-key handler to invokeonClose. Focus management (trap within the panel while open, restore on close) would also round this out.♻️ Example adjustments
-export function TaskPanel({ onClose }: TaskPanelProps) { +export function TaskPanel({ onClose }: TaskPanelProps) { + useEffect(() => { + const onKey = (e: KeyboardEvent) => { + if (e.key === 'Escape') onClose(); + }; + window.addEventListener('keydown', onKey); + return () => window.removeEventListener('keydown', onKey); + }, [onClose]); return ( <div className="fixed inset-0 z-50 flex justify-end bg-black/30" onClick={onClose}> <div + role="dialog" + aria-modal="true" + aria-labelledby="task-panel-title" className="w-80 border-l border-border bg-surface-1 p-4" onClick={(e) => e.stopPropagation()} > <div className="mb-4 flex items-center justify-between"> - <h3 className="font-display font-semibold text-text-primary">Tasks</h3> + <h3 id="task-panel-title" className="font-display font-semibold text-text-primary">Tasks</h3> <button + type="button" + aria-label="Close tasks panel" onClick={onClose} className="rounded-md px-2 py-1 text-text-muted hover:bg-surface-2 hover:text-text-secondary transition-colors" > × </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/src/components/TaskPanel.tsx` around lines 5 - 24, The TaskPanel component should be made accessible: add role="dialog" and aria-modal="true" to the overlay panel and give the heading (h3) an id so the panel uses aria-labelledby pointing to that id; add an aria-label (e.g., "Close tasks") to the close button that currently renders "×" and ensure the button still calls onClose; implement an Escape-key handler in the TaskPanel (listen for keydown and call onClose on Escape) and add basic focus management—set initial focus into the panel on mount, trap focus within the panel while open, and restore focus to the previously focused element on unmount/close (tie all changes to the TaskPanel component and its onClose prop).packages/remote-control-server/web/src/components/Navbar.tsx (2)
46-67: Consider memoizingcolorMapor hoisting it to module scope.
colorMapis a static lookup recreated on every render ofStatusBadge. Since it never depends on props, lifting it to module scope (or wrapping inuseMemowith[]) is a cheap win when this badge is rendered in long session/environment lists.🛠️ Proposed fix
-export function StatusBadge({ status }: { status: string }) { - const colorMap: Record<string, string> = { +const STATUS_COLOR_MAP: Record<string, string> = { active: "bg-status-active/20 text-status-active", running: "bg-status-running/20 text-status-running", idle: "bg-status-idle/20 text-status-idle", inactive: "bg-text-muted/20 text-text-muted", requires_action: "bg-status-warning/20 text-status-warning", archived: "bg-text-muted/20 text-text-muted", error: "bg-status-error/20 text-status-error", - }; +}; +export function StatusBadge({ status }: { status: string }) { return ( <span className={cn( "inline-flex items-center rounded-full px-2 py-0.5 text-xs font-medium", - colorMap[status] || "bg-surface-3 text-text-secondary", + STATUS_COLOR_MAP[status] || "bg-surface-3 text-text-secondary", )} > {status} </span> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/src/components/Navbar.tsx` around lines 46 - 67, The colorMap object inside the StatusBadge component is recreated on every render; hoist it to module scope (move the const colorMap = { ... } outside the StatusBadge function) or memoize it with useMemo([]) to avoid unnecessary allocations. Update references inside StatusBadge to use the top-level colorMap (or the memoized value) and ensure the exported function signature StatusBadge({ status }: { status: string }) remains unchanged.
27-39: Minor a11y/semantic nits on the Identity button.
- Add
type="button"to avoid accidental form submission if this is ever rendered inside a<form>.- The inline
<svg>should havearia-hidden="true"(like the logo svg above) since the visible "Identity" text already labels the button.🛠️ Proposed fix
<button + type="button" onClick={onIdentityClick} className="flex items-center gap-1 rounded-md px-3 py-1.5 text-sm text-text-secondary hover:bg-surface-2 hover:text-text-primary transition-colors" title="Identity & QR" > - <svg width="16" height="16" viewBox="0 0 16 16" fill="none"> + <svg width="16" height="16" viewBox="0 0 16 16" fill="none" aria-hidden="true">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/src/components/Navbar.tsx` around lines 27 - 39, The Identity button in Navbar.tsx should be made explicitly non-submitting and the decorative SVG hidden from assistive tech: update the <button> (the element with onIdentityClick) to include type="button" to prevent accidental form submission, and add aria-hidden="true" to the inline <svg> inside that button so screen readers rely on the visible "Identity" text label.packages/remote-control-server/src/types/api.ts (1)
22-22: Optionalcapabilities/worker_typeadditions look good — consider tightening the type.
Record<string, unknown>is fine as a generic escape hatch, but since capabilities are produced/consumed by this server (and theacp-linkpackage), a namedWorkerCapabilitiesinterface with known fields (e.g.acp?: boolean, supported models, etc.) would give call sites real type safety without needing narrowing at every use site. Non-blocking.Also applies to: 109-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/types/api.ts` at line 22, Replace the broad Record<string, unknown> type used for the capabilities property with a concrete WorkerCapabilities interface and use that type wherever the generic appears (e.g. the capabilities?: Record<string, unknown> declaration and the other occurrences noted around the file). Define WorkerCapabilities with the known fields your server and acp-link exchange (for example acp?: boolean, supportedModels?: string[], maxConcurrency?: number, worker_type?: string, etc.), export it from the module, and update the capabilities property and any related parameters/returns to use WorkerCapabilities so callers get compile-time safety without repetitive narrowing.packages/remote-control-server/web/components/ui/badge.tsx (1)
28-34: Extract the badge props into an interface.This keeps the new UI primitive aligned with the repository’s TypeScript shape convention.
Proposed refactor
+interface BadgeProps + extends React.ComponentProps<"span">, + VariantProps<typeof badgeVariants> { + asChild?: boolean +} + function Badge({ className, variant, asChild = false, ...props -}: React.ComponentProps<"span"> & - VariantProps<typeof badgeVariants> & { asChild?: boolean }) { +}: BadgeProps) {As per coding guidelines, Prefer
interfaceovertypefor defining object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/components/ui/badge.tsx` around lines 28 - 34, Extract the Badge component props into a named interface (e.g., BadgeProps) instead of using an inline intersection type; define interface BadgeProps extends React.ComponentProps<"span">, VariantProps<typeof badgeVariants> { asChild?: boolean } and then change the function signature to function Badge({ className, variant, asChild = false, ...props }: BadgeProps) to keep the component aligned with the repo's TypeScript conventions and make the shape reusable.packages/remote-control-server/web/components/ui/resizable.tsx (1)
28-34: Extract the handle props into an interface.The inline
withHandleobject shape should be named for consistency with the TSX guidelines.Proposed refactor
+interface ResizableHandleProps extends ResizablePrimitive.SeparatorProps { + withHandle?: boolean +} + function ResizableHandle({ withHandle, className, ...props -}: ResizablePrimitive.SeparatorProps & { - withHandle?: boolean -}) { +}: ResizableHandleProps) {As per coding guidelines, Prefer
interfaceovertypefor defining object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/components/ui/resizable.tsx` around lines 28 - 34, Extract the inline prop shape into a named interface: create an interface (e.g., ResizableHandleProps) that extends ResizablePrimitive.SeparatorProps and declares withHandle?: boolean and any other props like className, then update the ResizableHandle function signature to accept (props: ResizableHandleProps) (or destructure from that typed param) instead of the inline annotation; use the new interface in place of the current inline type to follow the guideline preferring interface over type.packages/remote-control-server/web/components/ChatMessage.tsx (1)
66-96: Optional: extract status → style mapping to reduce repetition.The status-to-class mapping is repeated three times (container border/bg, icon color, label color). A small lookup object keyed by status would make future additions/edits less error-prone.
♻️ Proposed refactor
+const STATUS_STYLES: Record<ToolCall["status"], { container: string; text: string }> = { + running: { + container: "bg-yellow-50 dark:bg-yellow-900/20 border-yellow-200 dark:border-yellow-800", + text: "text-yellow-600 dark:text-yellow-400", + }, + complete: { + container: "bg-green-50 dark:bg-green-900/20 border-green-200 dark:border-green-800", + text: "text-green-600 dark:text-green-400", + }, + error: { + container: "bg-red-50 dark:bg-red-900/20 border-red-200 dark:border-red-800", + text: "text-red-600 dark:text-red-400", + }, +}; + function ToolCallDisplay({ toolCall }: ToolCallDisplayProps) { + const styles = STATUS_STYLES[toolCall.status]; return ( <div className={cn( "flex items-center gap-2 text-xs px-2 py-1.5 rounded border", - toolCall.status === "running" && "bg-yellow-50 ...", - toolCall.status === "complete" && "bg-green-50 ...", - toolCall.status === "error" && "bg-red-50 ..." + styles.container )} > {toolCall.status === "running" ? ( - <Loader2 className="w-3 h-3 animate-spin text-yellow-600 dark:text-yellow-400" /> + <Loader2 className={cn("w-3 h-3 animate-spin", styles.text)} /> ) : ( - <Wrench className={cn("w-3 h-3", ...)} /> + <Wrench className={cn("w-3 h-3", styles.text)} /> )} <span className="truncate">{toolCall.title}</span> - <span className={cn("ml-auto text-[10px] uppercase font-medium", ...)}> + <span className={cn("ml-auto text-[10px] uppercase font-medium", styles.text)}> {toolCall.status} </span> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/components/ChatMessage.tsx` around lines 66 - 96, Extract the repeated status→class mappings in ToolCallDisplay into a single lookup object (e.g., const STATUS_STYLES = { running: { container: "...", icon: "...", label: "..." }, complete: {...}, error: {...} }) and then replace the three repeated cn(...) usages with references to STATUS_STYLES[toolCall.status].container, .icon, and .label inside the JSX (affecting the container div className, the Wrench/Loader icon className, and the status span className) so style definitions live in one place and avoid duplication.packages/remote-control-server/web/src/hooks/useQRScanner.ts (1)
73-78: Validate parsed QR payload instead of casting toQRCodeData.
JSON.parse(...) as QRCodeDatais a straight cast; if the JSON parses to a non-object (e.g., a string or number), thedata.url && data.tokencheck can throw on property access of non-objects (actually fine for primitives in JS, returns undefined), but TS strictness is misleading. PreferRecord<string, unknown>plus a type guard, matching the coding guideline for objects with unknown structure.🛡️ Proposed fix
- const data = JSON.parse(result.data) as QRCodeData; - if (data.url && data.token) { - onScanRef.current(data); + const parsed = JSON.parse(result.data) as unknown; + if ( + parsed !== null && + typeof parsed === "object" && + typeof (parsed as Record<string, unknown>).url === "string" && + typeof (parsed as Record<string, unknown>).token === "string" + ) { + onScanRef.current(parsed as QRCodeData); } else { onErrorRef.current?.("Invalid QR code: missing url or token"); }As per coding guidelines, "Use
Record<string, unknown>instead ofanyfor objects with unknown structure" and "Use type guards (type narrowing) for union types instead of type casting".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/src/hooks/useQRScanner.ts` around lines 73 - 78, The code in useQRScanner currently casts JSON.parse(result.data) to QRCodeData which is unsafe; change parsing to const parsed = JSON.parse(result.data) as Record<string, unknown> and add a type guard (e.g. isQRCodeData(obj): obj is QRCodeData) that checks parsed is an object and that parsed.url and parsed.token are strings before calling onScanRef.current; if the guard fails call onErrorRef.current with the invalid QR message. Update references to QRCodeData in the hook to use the guard to narrow types when invoking onScanRef and avoid direct casting.packages/acp-link/src/rcs-upstream.ts (2)
154-163: Mixingconsole.logwith the structured logger.The client uses
RcsUpstreamClient.logeverywhere except here, where a dashboard URL is printed throughconsole.log. If this is intentionally user-facing CLI output (and not for log ingestion), consider piping it through a dedicatedlog.infocall or a CLI-only writer so log levels/formatting remain consistent across the package. If it's meant to survive in non-TTY deployments, note that it will bypass any structured-log sinks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/acp-link/src/rcs-upstream.ts` around lines 154 - 163, Replace the ad-hoc console.log usage with the class's structured logger (RcsUpstreamClient.log or this.log) to keep formatting and sinks consistent; locate the block that prints the dashboard URL and agent ID (references: sessionId, agentId, webBase) and change each console.log call to a structured info-level call that formats the same message (use this.log.info or the package's log.info) or route it through a CLI-only writer if you explicitly want unstructured TTY output, ensuring messages still appear in non-TTY deployments by using the structured logger when appropriate.
52-56: DRY: duplicate URL-normalization logic.The
rcsUrl → http(s) base URLnormalization at lines 52-56 is duplicated verbatim at 149-153. Extract into a private helper (e.g.,buildHttpBaseUrl()) and reuse in bothregisterViaRest()and theidentifiedhandler.Also applies to: 149-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/acp-link/src/rcs-upstream.ts` around lines 52 - 56, Extract the duplicated URL-normalization logic into a single private helper (e.g., buildHttpBaseUrl()) on the same class in rcs-upstream.ts that returns the normalized http(s) base URL; move the chain of replaces (the ws:// → http://, wss:// → https://, strip /acp/ws... and trailing slash) into that helper and replace the inline code in both registerViaRest() and the identified event handler to call this.buildHttpBaseUrl() instead so both sites reuse the same function.packages/remote-control-server/web/src/components/IdentityPanel.tsx (2)
1-130: Duplicated scanner logic — reuseuseQRScanner.This component re-implements
QrScannerlifecycle management (start, stop, destroy, file import, cleanup on close) that is already encapsulated in the newuseQRScannerhook added in this same PR (packages/remote-control-server/web/src/hooks/useQRScanner.ts). Consolidating onto the hook removes a real source of drift (e.g., ref-based cleanup and options likehighlightScanRegionlive in only one place) and fits the PR's DRY direction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/src/components/IdentityPanel.tsx` around lines 1 - 130, The IdentityPanel duplicates QrScanner lifecycle and upload logic; switch it to use the existing useQRScanner hook: remove scannerRef, startCamera, stopCamera and the file-import flow in IdentityPanel and instead call the hook (e.g., const { start, stop, scanImage, scanning } = useQRScanner(...)) to start/stop scanning and to process images, forwarding the same scan callback (handleScannedData) and options like returnDetailedScanResult/highlightScanRegion; ensure the effect that cleans up on close calls stop() from the hook and replace direct scanner.start()/scanner.destroy()/QrScanner.scanImage usages with the hook's start/stop/scanImage helpers while keeping UUID handling in handleScannedData and preserving setScanning state by mapping hook scanning state to the component UI.
79-102: Parsed QR payload should be narrowed, not implicitly typed.
const parsed = JSON.parse(data)is implicitlyany, andparsed.url && parsed.tokenreads arbitrary properties without narrowing. Parse asunknownand use a type guard for the object shape.🛡️ Proposed fix
- try { - // Try ACP format: { url, token } - const parsed = JSON.parse(data); - if (parsed.url && parsed.token) { + try { + const parsed: unknown = JSON.parse(data); + if ( + parsed !== null && + typeof parsed === "object" && + typeof (parsed as Record<string, unknown>).url === "string" && + typeof (parsed as Record<string, unknown>).token === "string" + ) {As per coding guidelines, "Use
Record<string, unknown>instead ofanyfor objects with unknown structure" and "Use type guards (type narrowing) for union types instead of type casting".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/src/components/IdentityPanel.tsx` around lines 79 - 102, The QR JSON parsing uses an implicit any by doing const parsed = JSON.parse(data) and then reading parsed.url and parsed.token; update this to parse into unknown (e.g., const parsed: unknown = JSON.parse(data)) and add a type guard that verifies parsed is an object (Record<string, unknown>) and that url and token exist as strings (e.g., typeof parsed.url === "string" && typeof parsed.token === "string") before using them; apply this guard in the block that currently stops the camera and calls onClose so you only call stopCamera(), onClose(), or setUuid(importedUuid) after the shape checks succeed (references: parsed, stopCamera, onClose, setUuid).packages/acp-link/src/cli/command.ts (1)
54-57: Extract the CLI flag shape into an interface.The inline
flagsobject type is long and gets reused conceptually by the command; an interface keeps the function signature cleaner.Proposed refactor
+interface CommandFlags { + port: number; + host: string; + debug: boolean; + "no-auth": boolean; + https: boolean; +} + export const command = buildCommand({ @@ func: async function ( this: LocalContext, - flags: { port: number; host: string; debug: boolean; "no-auth": boolean; https: boolean }, + flags: CommandFlags, ...args: readonly string[] ) {As per coding guidelines, “Prefer
interfaceovertypefor defining object shapes in TypeScript”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/acp-link/src/cli/command.ts` around lines 54 - 57, The inline flags object type on the command handler should be extracted to a named interface to clean up the signature and follow the guideline to prefer interfaces for object shapes; create an interface (e.g., CliFlags) with properties port:number, host:string, debug:boolean, "no-auth":boolean, https:boolean, then replace the inline type in the async function signature (the func method on the command) with this new CliFlags interface.packages/remote-control-server/web/src/App.tsx (1)
44-50: Path regex is unanchored at the end — consider whether nested paths should match.
/^\/code\/([^/]+)/matches/code/abc/xyz/…withmatch[1] === "abc". If that's intentional (future sub-routes), ignore. Otherwise tighten to/^\/code\/([^/]+)\/?$/to reject unexpected deeper paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/src/App.tsx` around lines 44 - 50, The path regex used to extract session id from path is unanchored and currently allows deeper nested paths (match = path.match(/^\/code\/([^/]+)/)) which returns "abc" for "/code/abc/xyz"; update the pattern to explicitly allow only an optional trailing slash and no further segments (e.g., /^\/code\/([^/]+)\/?$/) or otherwise document/handle sub-routes, then use that new regex where you setCurrentSessionId(match[1]) so only exact "/code/{id}" (or "/code/{id}/") yields a session id and deeper paths result in setCurrentSessionId(null).packages/remote-control-server/src/transport/event-bus.ts (1)
44-47: Log receives an empty-string argument on non-error events.For every non-error publish this passes
""as the second log arg, producing a trailing empty token in whatever transportloguses. Cleaner to build the message conditionally or use a varargs spread:♻️ Proposed fix
- log( - `[RC-DEBUG] bus publish: sessionId=${event.sessionId} type=${event.type} dir=${event.direction} seq=${full.seqNum} subscribers=${this.subscribers.size}`, - event.type === "error" ? `payload=${JSON.stringify(event.payload)}` : "", - ); + const base = `[RC-DEBUG] bus publish: sessionId=${event.sessionId} type=${event.type} dir=${event.direction} seq=${full.seqNum} subscribers=${this.subscribers.size}`; + if (event.type === "error") { + log(base, `payload=${JSON.stringify(event.payload)}`); + } else { + log(base); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/transport/event-bus.ts` around lines 44 - 47, The log call in event-bus.ts is passing an empty-string as the second argument for non-error events; update the publish logging in the function where `log(...)` is called (the line that references `event.sessionId`, `event.type`, `event.direction`, `full.seqNum`, and `this.subscribers.size`) to only include the payload fragment when `event.type === "error"` (e.g. build the full message string conditionally or collect parts into an array and spread them) so you never pass `""` as a logger argument.packages/remote-control-server/web/components/chat/PermissionPanel.tsx (1)
45-72: Minor a11y polish: mark icons as decorative and give action buttons programmatic names.The lucide icons are decorative (text label follows), so they should carry
aria-hidden="true"to avoid duplicated announcements. The Allow/Deny buttons would also benefit fromaria-labelin English (oraria-describedbypointing at the tool name) so screen readers not in a Chinese locale still convey intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/components/chat/PermissionPanel.tsx` around lines 45 - 72, The icons (ShieldAlert, Check, X) are decorative and should be hidden from assistive tech and the action buttons need programmatic names: add aria-hidden="true" to ShieldAlert, Check and X, and add aria-label attributes (e.g. aria-label="Allow tool" and aria-label="Deny tool") to the Allow/Deny buttons in PermissionPanel (or alternatively add aria-describedby that references the element rendering request.toolName) so screen readers correctly announce the actions; keep onRespond(request.requestId, ...) behavior unchanged.packages/acp-link/package.json (1)
7-14: Consider adding anexportsmap for cleaner ESM consumption.With
"type": "module"and onlymain/types, consumers using strict ESM resolvers won't get subpath protection and tools like TS'smoduleResolution: "bundler"/"node16"prefer anexportsfield. Optional but conventional for new ESM packages:"main": "./dist/server.js", "types": "./dist/server.d.ts", + "exports": { + ".": { + "types": "./dist/server.d.ts", + "import": "./dist/server.js" + }, + "./package.json": "./package.json" + }, "bin": {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/acp-link/package.json` around lines 7 - 14, The package.json currently relies on "main", "types" and "bin" but lacks an "exports" map which ESM consumers and modern TS resolvers expect; add an "exports" entry that maps the package root to "./dist/server.js" (and the types entry to "./dist/server.d.ts"), expose the CLI subpath (e.g. "./cli" -> "./dist/cli/bin.js" or "./dist/cli/bin.js" as the "./bin" subpath) and include a "./package.json" export if desired; ensure the "exports" object covers "." (entry), any CLI subpath you publish, and points the types to the same files so tools using "moduleResolution: bundler/node16" can resolve correctly while preserving the existing "main", "types", and "bin" semantics (reference: "main", "types", "bin", and the CLI name "acp-link").packages/acp-link/src/__tests__/server.test.ts (1)
42-63:clientMessageTypesis a local duplicate, not the real union.The array here is a free-standing literal — it doesn't verify the server actually recognizes these types, and will silently diverge from
server.ts. If the intent is contract coverage, import the type/union (or a runtime list) fromserver.ts/types.tsand derive the assertions from that. Otherwise this test mostly checksArray.prototype.includes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/acp-link/src/__tests__/server.test.ts` around lines 42 - 63, The test defines a local clientMessageTypes array that duplicates the real contract and can drift; replace the literal with an import from the server source (either the runtime list or the exported union/type) and drive assertions from that single source of truth. Specifically, remove the local clientMessageTypes literal in the test and import the server export (e.g., an exported CLIENT_MESSAGE_TYPES array or the union/type such as ClientMessageType) from server.ts/types.ts, then assert against that import (compare lengths, membership, or equality) instead of hardcoding values so the test fails if the server contract changes.packages/remote-control-server/src/transport/acp-sse-writer.ts (1)
33-47: Consider buffering approach ifbus.getEventsSincebecomes async to prevent out-of-order delivery.The current synchronous implementation of
getEventsSinceandsubscribemeans events published between the replay loop and subscription are unlikely to create ordering issues on a single event loop tick. However, ifbus.getEventsSinceever becomes asynchronous, events published during the await could be delivered before the replay completes, causing the SSE client to observe non-monotonicidvalues. While the WHATWG SSE specification does not mandate monotonic event IDs (servers may send them in any order), applications that depend on sequential ordering should adopt the suggested approach: subscribe to a buffer first, flush historical events, then flush buffered + live events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/transport/acp-sse-writer.ts` around lines 33 - 47, Wrap the subscription/replay logic to avoid out-of-order IDs by first subscribing with a temporary in-memory buffer, then replaying historical events, then flushing the buffered live events to the SSE, and finally switching to direct delivery: call bus.subscribe into a local array buffer (use the existing unsub handler to stop when switching), then call bus.getEventsSince(channelGroupId, seqNum) and enqueue those historical events via controller.enqueue/encoder.encode, then drain the buffered events (ensuring you emit them only after the replay completes) and after flushing set the subscription to deliver directly (or replace the buffered subscriber with one that calls controller.enqueue), preserving use of channelGroupId, event.seqNum, encoder.encode and controller.enqueue; ensure unsub is invoked on error/close and buffer ordering is preserved when flushing.packages/remote-control-server/web/components/chat/ToolCallGroup.tsx (1)
184-187:match[1]may beundefinedunder strict TS.With
noUncheckedIndexedAccess,match[1]isstring | undefined, sosimplifyToolName’s declaredstringreturn type is not sound. Simple fix:- const match = title.match(/^(\w+)/); - return match ? match[1] : title; + const match = title.match(/^(\w+)/); + return match?.[1] ?? title;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/components/chat/ToolCallGroup.tsx` around lines 184 - 187, The simplifyToolName function currently uses match[1] which can be undefined under strict TS/noUncheckedIndexedAccess; update simplifyToolName to return a definite string by using a nullish fallback (e.g., return match?.[1] ?? title) or explicitly coerce match[1] to a string before returning so the function's declared string return type is satisfied; reference the simplifyToolName function and the local match variable when making this change.packages/remote-control-server/web/components/model-selector/ModelSelectorPicker.tsx (1)
28-41: Consider extractingfuzzyMatchinto a shared util.The same implementation is duplicated in
packages/remote-control-server/web/components/chat/CommandMenu.tsx. Extracting intosrc/lib/fuzzyMatch.ts(or similar) would keep behavior consistent and avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/components/model-selector/ModelSelectorPicker.tsx` around lines 28 - 41, The fuzzyMatch implementation is duplicated and should be centralized: extract the function fuzzyMatch(query: string, text: string): boolean into a new shared module (e.g., src/lib/fuzzyMatch.ts), export it, and update callers in ModelSelectorPicker and CommandMenu to import and use that single exported function; ensure the signature and behavior remain identical (including the early return for empty query) and update any imports/exports accordingly.packages/remote-control-server/web/components/ACPMain.tsx (1)
218-241:groupByRecencyuses a fixed 86_400_000ms step — DST-sensitive and index access under strict TS.Two nits:
- Subtracting
86400000from a local-midnightDateskips/duplicates an hour on DST transition days, so on those days recent sessions can land in the wrong bucket. Prefer calendar math:const yesterday = new Date(today); yesterday.setDate(yesterday.getDate() - 1);- With
noUncheckedIndexedAccess(common with TS strict),groups[0].sessions.push(...)etc. are typed as possiblyundefined. Either keep direct refs (const todayGroup = { label: "今天", sessions: [] as AgentSessionInfo[] };) or assert non-null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/components/ACPMain.tsx` around lines 218 - 241, The groupByRecency function uses a fixed 86400000ms subtraction which breaks on DST days and also indexes into groups (groups[0].sessions etc.) that can be seen as possibly undefined under noUncheckedIndexedAccess; fix by computing yesterday via calendar math (e.g. const yesterday = new Date(today); yesterday.setDate(yesterday.getDate() - 1);) and avoid unsafe indexed access by constructing explicit group objects with typed sessions arrays (e.g. const todayGroup = { label: "今天", sessions: [] as AgentSessionInfo[] }, yesterdayGroup = ..., earlierGroup = ...) or otherwise asserting non-null before pushing, then push into those named group variables and return [todayGroup, yesterdayGroup, earlierGroup].packages/remote-control-server/web/components/ai-elements/code-block.tsx (1)
14-22: Use interfaces for these object-shaped props.These are object shapes and can be expressed as interfaces, which matches the repository guideline.
Suggested refactor
-type CodeBlockProps = HTMLAttributes<HTMLDivElement> & { +interface CodeBlockProps extends HTMLAttributes<HTMLDivElement> { code: string; language?: string; showLineNumbers?: boolean; -}; +} -type CodeBlockContextType = { +interface CodeBlockContextType { code: string; -}; +} -export type CodeBlockCopyButtonProps = ComponentProps<typeof Button> & { +export interface CodeBlockCopyButtonProps extends ComponentProps<typeof Button> { onCopy?: () => void; onError?: (error: Error) => void; timeout?: number; -}; +}As per coding guidelines, "Prefer
interfaceovertypefor defining object shapes in TypeScript."Also applies to: 79-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/components/ai-elements/code-block.tsx` around lines 14 - 22, Replace the object-shaped type aliases with interfaces: change the type alias CodeBlockProps to an interface named CodeBlockProps (extending HTMLAttributes<HTMLDivElement>) and change CodeBlockContextType to an interface named CodeBlockContextType (with the code: string property); then apply the same refactor to any other object-shaped type aliases in this file (e.g., the types referenced around lines 79-83) so all object shapes use interface per repo guidelines.packages/remote-control-server/web/src/components/EventStream.tsx (2)
194-196: Nit: using array index as React key for streaming messages.
key={i}on a list where items are appended and (viareset()) cleared is usually fine, but if you ever reorder or filter in-place, React will mis-associate component state (e.g.expandedstate onAssistantBubble/ToolCard). Consider giving eachDisplayMessagea stableidat creation time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/src/components/EventStream.tsx` around lines 194 - 196, Replace the list key from the unstable index to a stable id by ensuring each DisplayMessage produced for the messages array gets a unique, persistent identifier at creation (e.g., message.id) and use that id as the React key in the mapping instead of key={i}; update the code that constructs messages (where reset() and new messages are created) to assign that id, and change the map to <MessageRow key={message.id} ...> so nested components like AssistantBubble and ToolCard maintain correct state.
260-271: Verifyesc()implementation and consider refactoring to a hardened renderer.The
esc()function (atpackages/remote-control-server/web/src/lib/utils.ts:8–14) correctly implements HTML escaping using thediv.textContent→div.innerHTMLpattern, which safely encodes all special characters. The flow informatAssistantContentandformatPlanContent—esc(content)→ regex replacements →dangerouslySetInnerHTML—is currently safe because escaped entities like<inside<pre>and<code>tags remain as text content and are not re-parsed as HTML tags.However, this pattern is brittle. Any future addition (e.g., link rendering, image support) that processes content without going through
esc()will reintroduce XSS risk. Relying ondangerouslySetInnerHTMLwith manual escaping is maintenance-heavy and error-prone.Consider replacing the regex stack with
marked+DOMPurifyorreact-markdownto shift markup rendering to a hardened library and eliminate the need fordangerouslySetInnerHTMLentirely. This applies to bothformatAssistantContent(260–271) andformatPlanContent(752–763).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/src/components/EventStream.tsx` around lines 260 - 271, formatAssistantContent currently uses esc(content) then manual regex replacements and eventual dangerouslySetInnerHTML, which is brittle and error-prone; update both formatAssistantContent and formatPlanContent to use a hardened renderer instead of manual regexes: replace the regex-based processing with a markdown renderer + sanitizer (e.g., marked + DOMPurify or react-markdown) so input is parsed by the markdown library and then sanitized (remove direct use of dangerouslySetInnerHTML with raw HTML produced by hand), and ensure any remaining use of esc() is either removed or only used for explicit text-only paths; look for functions named formatAssistantContent, formatPlanContent and references to esc() and dangerouslySetInnerHTML to make the changes.packages/acp-link/src/types.ts (1)
9-14: Optional: JSON-RPC 2.0 allowsid: nullin error responses.Per the JSON-RPC 2.0 spec, when an error occurs parsing the request (e.g. invalid JSON), the response
idmust benull. If you ever need to surface such errors, widen theidtype tostring | number | null. Not blocking for current use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/acp-link/src/types.ts` around lines 9 - 14, The JsonRpcResponse type currently declares id as "string | number" but per JSON-RPC 2.0 error responses the id can be null; update the JsonRpcResponse interface (symbol name: JsonRpcResponse) to allow id: string | number | null so error responses (e.g., parse errors) can set id to null while retaining existing cases.packages/remote-control-server/src/routes/acp/index.ts (2)
29-29: Nit:& {}intersection is a no-op.
ReturnType<typeof storeGetEnvironment> & {}is equivalent toReturnType<typeof storeGetEnvironment>. If the intent was to stripundefined/null, useNonNullable<ReturnType<typeof storeGetEnvironment>>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/routes/acp/index.ts` at line 29, The intersection with an empty object in the toAcpAgentResponse signature is redundant: replace the current ReturnType<typeof storeGetEnvironment> & {} with either just ReturnType<typeof storeGetEnvironment> (to remove the no-op) or, if the goal was to exclude null/undefined, use NonNullable<ReturnType<typeof storeGetEnvironment>>; update the function declaration for toAcpAgentResponse accordingly and ensure any callers still satisfy the tightened type.
127-128: Recommended: hoist theuuidimport to the module top.
await import("uuid")runs on every upgrade attempt. Node's dynamic import cache makes this cheap after the first call, but it still forces this handler to be async-for-no-reason and adds an unnecessary microtask on every new connection. A top-levelimport { v4 as uuidv4 } from "uuid";is clearer and matches ESM conventions used elsewhere.Also applies to: 182-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/routes/acp/index.ts` around lines 127 - 128, Hoist the dynamic import of uuid to a top-level ESM import and replace the inline await import usage: add `import { v4 as uuidv4 } from "uuid";` at the module top, then change `const wsId = \`acp_ws_${uuid().replace(/-/g, "")}\`` to use `uuidv4()` (and likewise update the second occurrence around the ws creation at the other location), removing the unnecessary `await import("uuid")` calls so the handler no longer performs a dynamic import or extra microtask.packages/acp-link/src/server.ts (3)
81-88: Recommended: replace module-level mutable config with closure-scoped state.
AGENT_COMMAND/AGENT_ARGS/AGENT_CWD/SERVER_PORT/SERVER_HOST/AUTH_TOKENare declared as module-levellets and mutated bystartServer. This makes the server non-reentrant (two calls stomp on each other) and makes testing awkward because leftover state from one test leaks into the next. Moving them into a config object captured by closure (or a class) avoids the footgun and mirrors theServerConfigargument you already accept.Also applies to: 591-600
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/acp-link/src/server.ts` around lines 81 - 88, Replace the module-level mutable variables AGENT_COMMAND, AGENT_ARGS, AGENT_CWD, SERVER_PORT, SERVER_HOST, and AUTH_TOKEN with a closure-scoped config (or encapsulate in a class) that startServer captures/constructs from the existing ServerConfig argument; remove the top-level let declarations and instead pass or bind a single config object into functions that need them (e.g., startServer, any handlers referenced later around the block where those vars are used and the code near the other occurrence around the 591-600 region), ensuring no global mutation so multiple server instances/tests won't share state.
801-818: Minor: heartbeat interval and signal listeners are never torn down.
- The
setIntervalat line 801 has no handle and no clear path — ifstartServeris ever invoked twice (e.g. tests, or hot-reload), intervals accumulate.process.on("SIGINT"/"SIGTERM")is registered per call tostartServer. Multiple invocations will leak listeners and potentially trigger MaxListenersExceededWarning. Also, because they only close the RCS upstream and exit, they won't flush in-flight WS messages or kill agent subprocesses.Consider holding the interval id, clearing it on shutdown, and consolidating signal handlers to run full cleanup (close WS server, kill agent children, drop clients) before
process.exit.Also applies to: 877-889
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/acp-link/src/server.ts` around lines 801 - 818, The heartbeat setInterval created near startServer is never cleared and process.on("SIGINT"/"SIGTERM") handlers are registered per startServer call, causing interval and listener leaks; capture the interval id (e.g. assign setInterval to heartbeatIntervalId), store it in a scope tied to the server instance, clearInterval(heartbeatIntervalId) during shutdown, and remove the signal listeners (or register them once globally) to avoid duplicates; in the shutdown handler invoked by the signals, perform full cleanup: clear the interval, close the WS server, iterate clients to close/terminate sockets and clear the clients map, and kill any agent subprocesses before calling process.exit to ensure in-flight messages flush and child processes are terminated (reference symbols: startServer, heartbeatIntervalId/setInterval, HEARTBEAT_INTERVAL_MS, clients, process.on("SIGINT"), process.on("SIGTERM")).
107-118:createRelayWs()usesas unknown as WSContextto fabricate a virtual context.The double cast works, but Hono only accesses
send,close, andreadyStateonWSContext—all of which are correctly implemented here. The other stubbed properties are set for user-level access but never read by Hono internals. That said, the double cast obscures the contract. Extracting a small interface (e.g.,MessageSink) documenting the three required methods would improve clarity without additional risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/acp-link/src/server.ts` around lines 107 - 118, The createRelayWs function is fabricating a WSContext via a double-cast which obscures the contract; define a small interface (e.g., MessageSink) that documents the three Hono-used members (readonly readyState: number, send: (data: any) => void, close: () => void), update createRelayWs to return an object typed as MessageSink (implementing those three members) and then, only where a WSContext is strictly required, do a single explicit cast from MessageSink to WSContext; update references to use the new MessageSink type so the no-op stubs for other properties are removed and the contract is clear (refer to createRelayWs and WSContext in your changes).packages/remote-control-server/web/components/ai-elements/conversation.tsx (1)
119-129: Optional: scope thequerySelectorto the current conversation container.
document.querySelector(\[${LAST_USER_MESSAGE_ATTR}="true"]`)runs against the whole document, so if two conversation UIs ever mount in parallel (e.g. a side-panel preview plus the main view) the wrong one may be scrolled. Accepting acontainerRef` prop and searching within that element would make the behavior deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/components/ai-elements/conversation.tsx` around lines 119 - 129, The scroll handler in ConversationScrollToLastUserMessageButton uses document.querySelector for LAST_USER_MESSAGE_ATTR which can target the wrong conversation if multiple mounts exist; update the component to accept a containerRef prop (e.g., containerRef: RefObject<HTMLElement>) and change handleScrollToLastUserMessage to query within containerRef.current (using containerRef.current?.querySelector(`[${LAST_USER_MESSAGE_ATTR}="true"]`) ) falling back to document only if the ref is not provided, ensuring scrollIntoView is called on the element found inside the specific conversation container.packages/remote-control-server/web/components/ChatInterface.tsx (1)
436-443: Nit: handler reset is inconsistent — one usesnull, others() => {}.
client.setSessionSwitchingHandler(null)vs() => {}for the others. If all of these acceptnullfor "unregister", prefer that uniformly to avoid leaving harmless closures attached; if onlySessionSwitchingsupports it, a code comment would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/components/ChatInterface.tsx` around lines 436 - 443, Handlers are being cleared inconsistently: use a consistent unregister approach for client handlers in the cleanup block of ChatInterface.tsx — either call client.setSessionSwitchingHandler(null) style for all handlers (setSessionCreatedHandler, setSessionLoadedHandler, setSessionUpdateHandler, setPromptCompleteHandler, setPermissionRequestHandler, setErrorMessageHandler) if the API accepts null to unregister, or if only setSessionSwitchingHandler supports null, change that one to use () => {} and add a short comment on setSessionSwitchingHandler explaining why it accepts null while others require a no-op function; update the cleanup to be consistent and add the explanatory comment referencing the specific setter names above.
| // Save certificates | ||
| writeFileSync(KEY_PATH, pems.private); | ||
| writeFileSync(CERT_PATH, pems.cert); |
There was a problem hiding this comment.
Private key is written world-readable — restrict to 0600.
writeFileSync(KEY_PATH, pems.private) uses the default mode, so the PEM-encoded private key ends up with permissions derived from the process umask (typically 0644). Any local user can then read it. Write the key with mode 0600 (and ideally also tighten the cert):
🔐 Proposed fix
- // Save certificates
- writeFileSync(KEY_PATH, pems.private);
- writeFileSync(CERT_PATH, pems.cert);
+ // Save certificates — private key must not be world-readable
+ writeFileSync(KEY_PATH, pems.private, { mode: 0o600 });
+ writeFileSync(CERT_PATH, pems.cert, { mode: 0o644 });📝 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.
| // Save certificates | |
| writeFileSync(KEY_PATH, pems.private); | |
| writeFileSync(CERT_PATH, pems.cert); | |
| // Save certificates — private key must not be world-readable | |
| writeFileSync(KEY_PATH, pems.private, { mode: 0o600 }); | |
| writeFileSync(CERT_PATH, pems.cert, { mode: 0o644 }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/acp-link/src/cert.ts` around lines 161 - 163, The private key is
being written with default permissions making it world-readable; update the
writeFileSync calls that use KEY_PATH and CERT_PATH (where pems.private and
pems.cert are saved) to explicitly set file modes so the private key is
restricted (use mode 0o600) and tighten the certificate file as well (e.g. 0o600
or at least 0o644) by passing an options object with the mode flag to
writeFileSync when saving the files in cert.ts.
| function send(ws: WSContext, type: string, payload?: unknown): void { | ||
| if (ws.readyState === 1) { | ||
| // WebSocket.OPEN | ||
| ws.send(JSON.stringify({ type, payload })); | ||
| } | ||
| // Forward to RCS upstream if connected | ||
| if (rcsUpstream?.isRegistered()) { | ||
| rcsUpstream.send({ type, payload }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: send() fan-out leaks every local WS response onto the RCS upstream.
Whenever the RCS upstream is registered (rcsUpstream?.isRegistered()), every call to send(ws, ...) — including responses destined for a specific local /ws client — is also forwarded to RCS. That means:
- A local browser WS's prompt outputs, session updates, and permission requests get echoed to the remote RCS relay.
- Responses for the RCS-relay virtual WS are correctly routed (local send is a no-op for
relayWs), but a concurrent local WS session will cross-contaminate the remote channel.
The forwarding should be gated on whether the triggering message came from the RCS relay path, not on rcsUpstream presence. One clean fix is to track the origin on ClientState and forward only when state belongs to the relay:
-function send(ws: WSContext, type: string, payload?: unknown): void {
- if (ws.readyState === 1) {
- // WebSocket.OPEN
- ws.send(JSON.stringify({ type, payload }));
- }
- // Forward to RCS upstream if connected
- if (rcsUpstream?.isRegistered()) {
- rcsUpstream.send({ type, payload });
- }
-}
+function send(ws: WSContext, type: string, payload?: unknown): void {
+ const state = clients.get(ws);
+ if (state?.isRelay) {
+ rcsUpstream?.send({ type, payload });
+ return;
+ }
+ if (ws.readyState === 1) {
+ ws.send(JSON.stringify({ type, payload }));
+ }
+}(Add isRelay: true when constructing relayState at line ~614.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/acp-link/src/server.ts` around lines 132 - 141, The send function
currently fans out every local WS message to rcsUpstream; change the logic to
forward to rcsUpstream only when the message originated from the relay by adding
an origin flag to the client state and checking it in send: add isRelay: true to
the relayState construction (the relayState object created around line ~614),
update send(ws: WSContext, type: string, payload?: unknown) to accept or look up
the associated ClientState (e.g., state) and only call rcsUpstream.send({ type,
payload }) when rcsUpstream?.isRegistered() && state?.isRelay === true; keep
local ws.send(JSON.stringify(...)) behavior unchanged for non-relay clients.
| // Auto-bind: if the session exists but has no owner, claim it for the requesting user | ||
| const existingId = resolveExistingSessionId(sessionId); | ||
| if (existingId) { | ||
| const owners = storeGetSessionOwners(existingId); | ||
| if (!owners || owners.size === 0) { | ||
| storeBindSession(existingId, uuid); | ||
| return existingId; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect storeBindSession and storeGetSessionOwners to verify atomicity / semantics.
fd -t f 'store.ts' packages/remote-control-server/src
rg -nP -C5 '\bstoreBindSession\b|\bstoreGetSessionOwners\b' packages/remote-control-server/srcRepository: claude-code-best/claude-code
Length of output: 21261
Auto-bind creates a session-claim race and mutates state from a "resolve" path.
Two critical concerns with the new auto-bind block:
-
TOCTOU race / hijack risk.
storeBindSessiondoes not enforce atomic "bind-if-absent" semantics—it unconditionally callsowners.add(uuid). Two concurrent requests can both observe empty owners and race to bind. Unless you serialize access, both succeed. Further, any request with a guessable or leaked session ID will claim ownership. Consider:- Adding a transactional
storeBindSessionIfAbsent()primitive that atomically checks and binds only if no owners exist - Restricting this claim to an explicit endpoint (e.g., POST
/web/bind) rather than hooking it into a generic ownership-resolution helper
- Adding a transactional
-
Surprising side effects on a read-shaped API. Callers of
resolveOwnedWebSessionIdexpect idempotent ownership checks. Folding a persistent write into this path couples authorization with state mutation and risks accidentally granting ownership from unintended entry points. Extract the claim into a named function (e.g.,claimUnownedSession) invoked explicitly by handlers that should perform the claim.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/remote-control-server/src/services/session.ts` around lines 111 -
120, The auto-bind block in resolveOwnedWebSessionId mutates state and has a
TOCTOU race: replace the unconditional storeBindSession call with an atomic
primitive and remove side effects from this read-path; introduce a new function
claimUnownedSession (or a transactional storeBindSessionIfAbsent) that performs
an atomic "bind if no owners" check-and-add using the same identifying symbols
(resolveExistingSessionId, storeGetSessionOwners, storeBindSession ->
storeBindSessionIfAbsent) and call that only from explicit handler code (e.g., a
POST /web/bind) rather than inside resolveOwnedWebSessionId so ownership
resolution remains read-only and claiming is explicit.
| /** Global registry of per-channel-group ACP event buses */ | ||
| const acpBuses = new Map<string, EventBus>(); | ||
|
|
||
| export function getAcpEventBus(channelGroupId: string): EventBus { | ||
| let bus = acpBuses.get(channelGroupId); | ||
| if (!bus) { | ||
| bus = new EventBus(); | ||
| acpBuses.set(channelGroupId, bus); | ||
| } | ||
| return bus; | ||
| } | ||
|
|
||
| export function removeAcpEventBus(channelGroupId: string) { | ||
| const bus = acpBuses.get(channelGroupId); | ||
| if (bus) { | ||
| bus.close(); | ||
| acpBuses.delete(channelGroupId); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find callers of removeAcpEventBus to verify lifecycle coverage
rg -nP '\bremoveAcpEventBus\s*\(' -g '!**/node_modules/**'
echo "---"
# Compare with getAcpEventBus call sites (should be paired by lifecycle)
rg -nP '\bgetAcpEventBus\s*\(' -g '!**/node_modules/**'Repository: claude-code-best/claude-code
Length of output: 1022
🏁 Script executed:
# Search for disconnect, cleanup, or deletion handlers in the relevant files
rg -n -A 5 -B 2 '(disconnect|cleanup|delete|removeChannel|removeGroup)' packages/remote-control-server/src/transport/acp-relay-handler.ts packages/remote-control-server/src/transport/acp-ws-handler.ts --max-count=20Repository: claude-code-best/claude-code
Length of output: 4036
🏁 Script executed:
# Look for channel-group lifecycle management - where groups are created and destroyed
rg -n 'channelGroup' packages/remote-control-server/src/transport/ -t ts --max-count=30Repository: claude-code-best/claude-code
Length of output: 3810
Call removeAcpEventBus in agent and relay disconnect handlers to prevent memory leak.
The acpBuses map grows unbounded. Currently, removeAcpEventBus is never invoked despite 7 calls to getAcpEventBus. When agents/relays disconnect (acp-ws-handler.ts ~254, acp-relay-handler.ts ~130), connection records are cleaned up but buses remain indefinitely. Long-lived servers will leak up to MAX_EVENTS_PER_BUS events per abandoned channel group.
Add removeAcpEventBus(channelGroupId) calls in:
- Agent disconnect path (acp-ws-handler.ts, after
connections.delete()) - Relay disconnect path (acp-relay-handler.ts, after
relayConnections.delete())
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/remote-control-server/src/transport/event-bus.ts` around lines 98 -
116, The acpBuses map is never pruned, causing a memory leak; after you delete
connection records in the agent disconnect handler (where
connections.delete(...) is called in the code that uses getAcpEventBus) and in
the relay disconnect handler (after relayConnections.delete(...)), call
removeAcpEventBus(channelGroupId) to close and remove the bus for that channel
group; ensure you reference the same channelGroupId used to obtain the EventBus,
and add an import for removeAcpEventBus if the handler files don’t already
import it.
| /** Called from onOpen — subscribes to event bus, forwards outbound events to bridge WS */ | ||
| export function handleWebSocketOpen(ws: WSContext, sessionId: string) { | ||
| const openTime = Date.now(); | ||
| const lastClientActivity = Date.now(); |
There was a problem hiding this comment.
Read the mutable activity timestamp from the cleanup entry.
lastClientActivity is captured as the initial open time, but handleWebSocketMessage updates entry.lastClientActivity. The keepalive loop never sees those updates, so active clients still get closed after CLIENT_ACTIVITY_TIMEOUT_MS.
🐛 Proposed fix
- const silenceMs = Date.now() - lastClientActivity;
+ const currentEntry = cleanupBySession.get(sessionId);
+ if (!currentEntry) {
+ clearInterval(keepalive);
+ return;
+ }
+ const silenceMs = Date.now() - currentEntry.lastClientActivity;Also applies to: 89-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/remote-control-server/src/transport/ws-handler.ts` at line 42, The
keepalive loop is reading a captured immutable timestamp (const
lastClientActivity = Date.now()) instead of the mutable cleanup entry field, so
it never observes updates made by handleWebSocketMessage; change the logic in
the keepalive/cleanup code (the block using lastClientActivity and the loop
around CLIENT_ACTIVITY_TIMEOUT_MS, including the region referenced at lines
~89-118) to read entry.lastClientActivity each iteration (or compute now -
entry.lastClientActivity) rather than using the captured constant, so the loop
honors updates done by handleWebSocketMessage and only closes truly idle
clients.
| const { currentBranch, setBranches, branches } = useMessageBranch(); | ||
| const childrenArray = Array.isArray(children) ? children : [children]; | ||
|
|
||
| // Use useEffect to update branches when they change | ||
| useEffect(() => { | ||
| if (branches.length !== childrenArray.length) { | ||
| setBranches(childrenArray); | ||
| } | ||
| }, [childrenArray, branches, setBranches]); | ||
|
|
||
| return childrenArray.map((branch, index) => ( | ||
| <div | ||
| className={cn( | ||
| "grid gap-2 overflow-hidden [&>div]:pb-0", | ||
| index === currentBranch ? "block" : "hidden" | ||
| )} | ||
| key={branch.key} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify strict TypeScript still passes after the branch typing change.
bunx tsc --noEmitRepository: claude-code-best/claude-code
Length of output: 116
🏁 Script executed:
# Locate the target file and check its contents
find . -name "message.tsx" -path "*/ai-elements/*" | head -5Repository: claude-code-best/claude-code
Length of output: 143
🏁 Script executed:
# Find tsconfig.json to understand the project's TypeScript configuration
find . -name "tsconfig.json" | head -10Repository: claude-code-best/claude-code
Length of output: 501
🏁 Script executed:
# Check if tsc is available and what version
which tsc || echo "tsc not found in PATH"Repository: claude-code-best/claude-code
Length of output: 154
🏁 Script executed:
# Read the message.tsx file, particularly the section around lines 192-208
wc -l ./packages/remote-control-server/web/components/ai-elements/message.tsxRepository: claude-code-best/claude-code
Length of output: 147
🏁 Script executed:
# Read the full message.tsx file (if reasonable size)
cat -n ./packages/remote-control-server/web/components/ai-elements/message.tsx | head -250Repository: claude-code-best/claude-code
Length of output: 7854
🏁 Script executed:
# Find remote-control-server's tsconfig.json
find ./packages/remote-control-server -name "tsconfig.json"Repository: claude-code-best/claude-code
Length of output: 169
🏁 Script executed:
# Read the remote-control-server's tsconfig.json
cat -n ./packages/remote-control-server/tsconfig.jsonRepository: claude-code-best/claude-code
Length of output: 226
🏁 Script executed:
# Also check the web/tsconfig.json
cat -n ./packages/remote-control-server/web/tsconfig.jsonRepository: claude-code-best/claude-code
Length of output: 800
🏁 Script executed:
# Check root tsconfig.json if it exists
cat -n ./tsconfig.json 2>/dev/null || echo "No root tsconfig.json"Repository: claude-code-best/claude-code
Length of output: 1364
🏁 Script executed:
# Check if there's a package.json in the web directory with build scripts
cat -n ./packages/remote-control-server/web/package.json 2>/dev/null || echo "No web package.json"Repository: claude-code-best/claude-code
Length of output: 91
🏁 Script executed:
# Check the root package.json for type checking scripts
cat -n ./package.json | head -100Repository: claude-code-best/claude-code
Length of output: 4176
🏁 Script executed:
# Try to find if Node and npm/pnpm are available and check their versions
node --version 2>/dev/null && npm --version 2>/dev/null || echo "Node/npm not available"Repository: claude-code-best/claude-code
Length of output: 86
🏁 Script executed:
# Try to run the typecheck script which uses tsc --noEmit
npm run typecheck 2>&1 | head -100Repository: claude-code-best/claude-code
Length of output: 286
🏁 Script executed:
# If that doesn't work, try npx tsc directly
npx tsc --noEmit 2>&1 | head -100Repository: claude-code-best/claude-code
Length of output: 234
🏁 Script executed:
# Check if `@types/react` is installed and what version
npm list `@types/react` 2>&1 | head -20Repository: claude-code-best/claude-code
Length of output: 126
🏁 Script executed:
# Try to find and examine what HTMLAttributes children type is
find . -name "*.d.ts" -path "*/node_modules/@types/react/*" 2>/dev/null | head -5Repository: claude-code-best/claude-code
Length of output: 54
🏁 Script executed:
# Let's manually verify the type issue by creating a simple type check file
cat > /tmp/type-check.ts << 'EOF'
import type { HTMLAttributes, ReactElement } from "react";
// Simulate the issue
type MessageBranchContentProps = HTMLAttributes<HTMLDivElement>;
function useMessageBranch() {
return {
setBranches: (branches: ReactElement[]) => {},
branches: [] as ReactElement[],
currentBranch: 0
};
}
export const MessageBranchContent = ({
children,
...props
}: MessageBranchContentProps) => {
const { setBranches, branches } = useMessageBranch();
const childrenArray = Array.isArray(children) ? children : [children];
// This should error: ReactNode[] is not assignable to ReactElement[]
setBranches(childrenArray);
return childrenArray.map((branch, index) => (
<div key={branch.key}>
{branch}
</div>
));
};
EOF
cat /tmp/type-check.tsRepository: claude-code-best/claude-code
Length of output: 820
🏁 Script executed:
# Try to check this specific file with tsc
npx tsc --noEmit --skipLibCheck /tmp/type-check.ts 2>&1Repository: claude-code-best/claude-code
Length of output: 213
🏁 Script executed:
# Check React's type definitions for HTMLAttributes
find ./node_modules -path "*/react/index.d.ts" 2>/dev/null | head -1 | xargs cat 2>/dev/null | grep -A 20 "interface HTMLAttributes" | head -30Repository: claude-code-best/claude-code
Length of output: 54
🏁 Script executed:
# Let's check what's actually being imported in message.tsx
sed -n '1,30p' ./packages/remote-control-server/web/components/ai-elements/message.tsxRepository: claude-code-best/claude-code
Length of output: 846
🏁 Script executed:
# Search for any type definitions or interfaces related to React in the web folder
rg "children.*ReactNode|children.*ReactElement" ./packages/remote-control-server/web --type ts --type tsx -A 2 -B 2 | head -50Repository: claude-code-best/claude-code
Length of output: 99
🏁 Script executed:
# Try to use rg with correct ts,tsx syntax
rg "children.*ReactNode|children.*ReactElement" ./packages/remote-control-server/web -t ts -t tsx -A 2 -B 2 2>/dev/null | head -50Repository: claude-code-best/claude-code
Length of output: 54
🏁 Script executed:
# Let's check what the actual HTMLAttributes type expects for children
python3 << 'EOF'
# HTMLAttributes in React has children?: ReactNode
# Let's verify by examining the type issue:
# ReactNode = ReactElement | string | number | boolean | ReactFragment | ReactPortal | null | undefined
print("ReactNode types:")
print(" - ReactElement (what setBranches expects)")
print(" - string")
print(" - number")
print(" - boolean")
print(" - ReactFragment")
print(" - ReactPortal")
print(" - null")
print(" - undefined")
print()
print("Issue: children from HTMLAttributes<HTMLDivElement> is ReactNode")
print("But setBranches expects ReactElement[] specifically")
print("Array.isArray(children) ? children : [children] creates ReactNode[]")
print("This violates TypeScript strict mode type safety")
EOFRepository: claude-code-best/claude-code
Length of output: 458
🏁 Script executed:
# Verify the exact imports and types used in message.tsx
grep -n "ReactElement\|ReactNode" ./packages/remote-control-server/web/components/ai-elements/message.tsxRepository: claude-code-best/claude-code
Length of output: 304
🏁 Script executed:
# Check what Children.toArray actually does and if it's available
rg "Children\.toArray|isValidElement" ./packages/remote-control-server/web --type ts --type tsx 2>/dev/null | head -20Repository: claude-code-best/claude-code
Length of output: 54
🏁 Script executed:
# Let's verify one more aspect - check if there are any other usages of setBranches or similar patterns
rg "setBranches|toArray|isValidElement" ./packages/remote-control-server/web --type ts --type tsx -B 2 -A 2 2>/dev/null | head -40Repository: claude-code-best/claude-code
Length of output: 54
🏁 Script executed:
# Check if Children is imported anywhere in the codebase
rg "from.*react.*Children|import.*Children" ./packages/remote-control-server --type ts --type tsx 2>/dev/null | head -20Repository: claude-code-best/claude-code
Length of output: 54
🏁 Script executed:
# Let's examine the full MessageBranchContent component to understand the complete context
sed -n '186,214p' ./packages/remote-control-server/web/components/ai-elements/message.tsxRepository: claude-code-best/claude-code
Length of output: 872
Normalize branches to ReactElement[] before storing them.
children from HTMLAttributes<HTMLDivElement> is typed as ReactNode, which includes strings, numbers, booleans, null, and undefined. Under strict TypeScript mode, passing ReactNode[] to setBranches(ReactElement[]) is a type violation. Additionally, accessing branch.key at line 208 assumes ReactElement but will fail at runtime if children contains non-element nodes.
Use Children.toArray() with isValidElement filtering to safely extract only the valid React elements:
Suggested fix
-import { createContext, lazy, memo, Suspense, useContext, useEffect, useState } from "react";
+import { Children, createContext, isValidElement, lazy, memo, Suspense, useContext, useEffect, useMemo, useState } from "react";
@@
export const MessageBranchContent = ({
children,
...props
}: MessageBranchContentProps) => {
const { currentBranch, setBranches, branches } = useMessageBranch();
- const childrenArray = Array.isArray(children) ? children : [children];
+ const childrenArray = useMemo(
+ () => Children.toArray(children).filter(isValidElement),
+ [children]
+ );
// Use useEffect to update branches when they change
useEffect(() => {
if (branches.length !== childrenArray.length) {
setBranches(childrenArray);
@@
)}
- key={branch.key}
+ key={branch.key ?? index}
{...props}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/remote-control-server/web/components/ai-elements/message.tsx` around
lines 192 - 208, Children may include non-ReactElement nodes, causing a
TypeScript type error when calling setBranches and unsafe access of branch.key;
normalize children to a ReactElement[] before storing and rendering: use
React.Children.toArray(children) filtered with React.isValidElement to produce a
typed elements array, call setBranches(normalizedElements) (useName:
setBranches, useMessageBranch, branches), use that normalized array in the
useEffect dependency instead of childrenArray, and when rendering use branch.key
?? index to safely provide a key.
| function AskUserPanel({ | ||
| questions, | ||
| description, | ||
| onSubmit, | ||
| onSkip, | ||
| }: { | ||
| requestId: string; | ||
| questions: import("../types").Question[]; | ||
| description: string; | ||
| onSubmit: (answers: Record<string, unknown>) => void; | ||
| onSkip: () => void; | ||
| }) { | ||
| const [answers, setAnswers] = useState<Record<string, unknown>>({}); | ||
| const [otherTexts, setOtherTexts] = useState<Record<string, string>>({}); | ||
|
|
||
| const handleSelect = (qIdx: number, oIdx: number, multiSelect: boolean) => { | ||
| if (multiSelect) { | ||
| const current = (answers[qIdx] as number[]) || []; | ||
| const next = current.includes(oIdx) ? current.filter((i) => i !== oIdx) : [...current, oIdx]; | ||
| setAnswers({ ...answers, [qIdx]: next }); | ||
| } else { | ||
| setAnswers({ ...answers, [qIdx]: oIdx }); | ||
| } | ||
| }; | ||
|
|
||
| const handleOtherSubmit = (qIdx: number) => { | ||
| const text = otherTexts[qIdx]?.trim(); | ||
| if (!text) return; | ||
| setAnswers({ ...answers, [qIdx]: text }); | ||
| setOtherTexts({ ...otherTexts, [qIdx]: "" }); | ||
| }; | ||
|
|
||
| const handleSubmit = () => { | ||
| const mapped: Record<string, unknown> = {}; | ||
| for (const [qIdx, val] of Object.entries(answers)) { | ||
| const q = questions[parseInt(qIdx)]; | ||
| if (!q) continue; | ||
| if (typeof val === "number") { | ||
| mapped[qIdx] = q.options?.[val]?.label || String(val); | ||
| } else if (Array.isArray(val)) { | ||
| mapped[qIdx] = val.map((i) => q.options?.[i]?.label || String(i)); | ||
| } else { | ||
| mapped[qIdx] = val; | ||
| } | ||
| } | ||
| onSubmit(mapped); | ||
| }; | ||
|
|
||
| if (questions.length <= 1) { | ||
| const q = questions[0] || { question: description, options: [], multiSelect: false }; | ||
| const selectedIdx = answers[0]; | ||
| const multiSelect = q.multiSelect || false; | ||
|
|
||
| return ( | ||
| <div className="rounded-xl border border-brand/30 bg-surface-1 p-4"> | ||
| <div className="mb-3 text-sm font-semibold text-text-primary"> | ||
| {esc(description || q.question || "Question")} | ||
| </div> | ||
| <div className="space-y-2"> | ||
| {(q.options || []).map((opt, j) => { | ||
| const isSelected = multiSelect | ||
| ? ((answers[0] as number[]) || []).includes(j) | ||
| : selectedIdx === j; | ||
| return ( | ||
| <button | ||
| key={j} | ||
| onClick={() => handleSelect(0, j, multiSelect)} | ||
| className={cn( | ||
| "w-full rounded-lg border px-4 py-2.5 text-left text-sm transition-colors", | ||
| isSelected | ||
| ? "border-brand bg-brand/10 text-text-primary" | ||
| : "border-border bg-surface-2 text-text-secondary hover:border-border-light", | ||
| )} | ||
| > | ||
| <div className="font-medium">{esc(opt.label)}</div> | ||
| {opt.description && <div className="mt-0.5 text-xs text-text-muted">{esc(opt.description)}</div>} | ||
| </button> | ||
| ); | ||
| })} | ||
| <div className="flex gap-2"> | ||
| <input | ||
| type="text" | ||
| value={otherTexts[0] || ""} | ||
| onChange={(e) => setOtherTexts({ ...otherTexts, [0]: e.target.value })} | ||
| placeholder="Other..." | ||
| className="flex-1 rounded-lg border border-border bg-surface-2 px-3 py-2 text-sm text-text-primary placeholder:text-text-muted focus:border-brand focus:outline-none" | ||
| onKeyDown={(e) => e.key === "Enter" && handleOtherSubmit(0)} | ||
| /> | ||
| <button | ||
| onClick={() => handleOtherSubmit(0)} | ||
| className="rounded-lg border border-border px-3 py-2 text-sm text-text-secondary hover:bg-surface-2 transition-colors" | ||
| > | ||
| Send | ||
| </button> | ||
| </div> | ||
| </div> | ||
| <div className="mt-4 flex gap-2"> | ||
| <button | ||
| onClick={handleSubmit} | ||
| className="rounded-lg bg-brand px-4 py-2 text-sm font-medium text-white hover:bg-brand-light transition-colors" | ||
| > | ||
| Submit | ||
| </button> | ||
| <button | ||
| onClick={onSkip} | ||
| className="rounded-lg border border-border px-4 py-2 text-sm text-text-secondary hover:bg-surface-2 transition-colors" | ||
| > | ||
| Skip | ||
| </button> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| // Multiple questions — tab layout | ||
| const [activeTab, setActiveTab] = useState(0); |
There was a problem hiding this comment.
Critical: Rules of Hooks violation — useState(0) at line 527 is reachable only when an earlier conditional return is not taken.
AskUserPanel has an early return at line 464–523 when questions.length <= 1, and then calls const [activeTab, setActiveTab] = useState(0); at line 527 in the "multiple questions" branch. If questions.length transitions across the <= 1 boundary between renders (the server can update questions), the number of hook calls changes and React will throw:
Rendered more hooks than during the previous render.
Move the useState(0) (and activeTab/setActiveTab) to the top of the component before any early return. You can simply not use it in the single-question branch:
function AskUserPanel({ ... }) {
const [answers, setAnswers] = useState<Record<string, unknown>>({});
const [otherTexts, setOtherTexts] = useState<Record<string, string>>({});
+ const [activeTab, setActiveTab] = useState(0);
// handlers ...
if (questions.length <= 1) {
// single-question branch (does not use activeTab)
...
}
- // Multiple questions — tab layout
- const [activeTab, setActiveTab] = useState(0);
return (...);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/remote-control-server/web/src/components/EventStream.tsx` around
lines 412 - 527, AskUserPanel currently calls useState for
activeTab/setActiveTab after an early return which can change hook order; move
the const [activeTab, setActiveTab] = useState(0) declaration to the top of the
AskUserPanel component (before any conditional return or branching) so hooks are
always invoked in the same order; it's fine if activeTab/setActiveTab are unused
in the single-question branch.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation