feat(canvas): broadcast UI commands to all connected nodes#31828
feat(canvas): broadcast UI commands to all connected nodes#31828carbaj03 wants to merge 6 commits intoopenclaw:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35f88071b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return [resolveNodeIdFromList(nodes, q)]; | ||
| } | ||
| const canvasConnected = nodes.filter( | ||
| (n) => n.connected && Array.isArray(n.caps) && n.caps.includes("canvas"), |
There was a problem hiding this comment.
Keep broadcast node selection compatible with pair-list fallback
The new broadcast selector now requires connected === true and caps to include "canvas", which makes resolveCanvasNodeIds() return no targets for gateways that fall back to node.pair.list (those records do not include connected/caps, as shown by the existing listNodes fallback behavior). In that environment, present/hide/navigate/a2ui_* without an explicit node now fail with "no connected canvas-capable nodes", whereas the previous resolveNodeId(..., allowDefault=true) path still worked by choosing a default paired node.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. resolveCanvasNodeIds now applies the same graceful fallback as selectDefaultNodeFromList: when caps is absent (pair-list fallback), assume the node may support canvas; when connected is absent, fall back to all candidates instead of filtering to zero. Added a test covering the pair-list fallback path.
Greptile SummaryThis PR upgrades the canvas tool to broadcast UI state commands ( Key changes:
The broadcast semantics are sound, the Confidence Score: 5/5
Last reviewed commit: 41e0347 |
| const canvasConnected = nodes.filter( | ||
| (n) => n.connected && Array.isArray(n.caps) && n.caps.includes("canvas"), | ||
| ); | ||
| if (canvasConnected.length === 0) { | ||
| throw new Error("no connected canvas-capable nodes"); | ||
| } | ||
| return canvasConnected.map((n) => n.nodeId); |
There was a problem hiding this comment.
Fallback nodes excluded from broadcast — breaks older gateway versions
When node.list is unavailable, loadNodes falls back to node.pair.list and maps each result to a NodeListNode without connected or caps fields (both undefined). The filter below requires both to be truthy, so every node from the fallback path is excluded, causing this function to always throw "no connected canvas-capable nodes" for users with older gateway versions.
Compare with selectDefaultNodeFromList, which explicitly handles this with:
Array.isArray(n.caps) ? n.caps.includes(capability) : true— treats nodes withoutcapsas capable of everything- Falls back to
withCapability(all nodes) when no node hasconnected === true
The new filter should apply the same permissiveness to stay consistent with the existing resolution logic:
const canvasConnected = nodes.filter(
(n) =>
(n.connected === true || n.connected === undefined) &&
(!Array.isArray(n.caps) || n.caps.includes("canvas")),
);This preserves the "treat unknown-caps nodes as canvas-capable" grace for legacy gateway clients while still preferring explicitly-connected canvas nodes on modern ones.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/tools/nodes-utils.ts
Line: 181-187
Comment:
**Fallback nodes excluded from broadcast — breaks older gateway versions**
When `node.list` is unavailable, `loadNodes` falls back to `node.pair.list` and maps each result to a `NodeListNode` without `connected` or `caps` fields (both `undefined`). The filter below requires both to be truthy, so every node from the fallback path is excluded, causing this function to always throw `"no connected canvas-capable nodes"` for users with older gateway versions.
Compare with `selectDefaultNodeFromList`, which explicitly handles this with:
- `Array.isArray(n.caps) ? n.caps.includes(capability) : true` — treats nodes without `caps` as capable of everything
- Falls back to `withCapability` (all nodes) when no node has `connected === true`
The new filter should apply the same permissiveness to stay consistent with the existing resolution logic:
```ts
const canvasConnected = nodes.filter(
(n) =>
(n.connected === true || n.connected === undefined) &&
(!Array.isArray(n.caps) || n.caps.includes("canvas")),
);
```
This preserves the "treat unknown-caps nodes as canvas-capable" grace for legacy gateway clients while still preferring explicitly-connected canvas nodes on modern ones.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed. Same approach — resolveCanvasNodeIds now mirrors the fallback logic in selectDefaultNodeFromList: nodes without caps pass through, and nodes without connected are included as candidates when no connected nodes exist. Test added for the pair-list path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47028e2fa8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const connected = withCanvas.filter((n) => n.connected); | ||
| const candidates = connected.length > 0 ? connected : withCanvas; |
There was a problem hiding this comment.
Exclude disconnected nodes from canvas broadcast candidates
resolveCanvasNodeIds falls back to withCanvas whenever connected.length === 0, which treats “connected missing” and “all nodes explicitly disconnected” as the same case. In normal node.list responses, connected is an explicit boolean (src/gateway/server-methods/nodes.ts sets connected: Boolean(live)), so when every canvas node is offline this function still returns their IDs instead of throwing no connected canvas-capable nodes. As a result, broadcast actions will invoke offline nodes and wait on avoidable timeouts before failing, contrary to the helper’s contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The fallback now checks whether any node has a connected field (typeof n.connected === "boolean"). When the field exists (from node.list), only connected: true nodes are included — explicitly disconnected nodes are excluded. The fallback to all candidates only triggers when the field is entirely absent (pair-list path). Added a test for the all-explicitly-disconnected case.
|
Still active. All bot review concerns addressed (P1 pair-list fallback + P2 explicit disconnect handling — see inline replies). CI failures are infra-only (GitHub API rate limit on label job, unrelated macOS test suite failures in Slack routing + ACP persistent-bindings). None related to this PR's changes — happy to rebase or retrigger if needed. This PR unblocks canvas broadcast for multi-node setups (needed for Tenazitas). Would appreciate a review when possible. |
|
👋 Friendly ping — CI is mostly green (the macOS and labeler failures look like infra flakes). This one is ready for review. We're preparing to launch our app and this feature is part of the release. Would really appreciate a review when you get a chance. Thanks! |
|
Hi! Just rebased and retriggered CI — previous failures were infra-only (detect-secrets full-scan false positives and macOS runner hang, both passing on main). Quick summary of what this enables: users with multiple devices (e.g. desktop + phone) only see canvas/a2ui updates on one device today. This PR broadcasts UI-state commands to all connected canvas-capable nodes, while keeping device-specific commands (eval, snapshot) single-node. 3 files changed, 6 tests added. Happy to address any feedback. |
|
@vincentkoc would you have a chance to take a look when you get a moment? |
|
@joshavant @altaywtf — friendly ping, this PR is ready for review (CI failures are infra-only — macOS runner hang and unrelated Windows flakes). We're preparing for our app launch and this feature is in the critical path. Would appreciate a look when you get a chance! |
9af3c51 to
ac2f2ef
Compare
|
@gumadeiras @ngutman — this is a small feature (135 lines, 3 files) that broadcasts canvas UI commands ( CI is green, tests included. Would really appreciate a review! |
|
Hi @vincentkoc and @steipete — gentle ping on this one. This PR broadcasts canvas/a2ui commands to all connected nodes (multi-device support). All CI checks pass, Codex review is positive. Would love a human reviewer to take a look when you have a moment. Thanks! 🙏 |
|
Bumping this for visibility — this PR and #29515 (plugin hook preservation across gateway restart) are a pair, both green with 23/23 CI checks passing. supersantux expressed interest on Discord today and mentioned he'd follow up. Both are small and non-breaking — would really appreciate a maintainer pass when you get a chance. 🙏 |
…odes Canvas commands like a2ui_push, a2ui_reset, present, hide, and navigate are UI updates that should reach every connected device. Previously they were routed to a single node (preferring local Mac), so users with multiple devices (e.g. desktop + phone) would only see updates on one. Now the canvas tool fans out these commands to all connected nodes with the canvas capability via Promise.allSettled, succeeding if at least one node receives the update. Device-specific commands (eval, snapshot) remain single-node since they return a result from one device. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4c3e4b8 to
4428b4d
Compare
|
Closing — stale with merge conflicts and no reviews. Will re-submit on a fresh branch if still needed. |
Summary
a2ui_push,a2ui_reset,present,hide,navigate) are now broadcast to all connected canvas-capable nodes instead of routing to a single nodeeval,snapshot) remain single-node since they return a resultPromise.allSettledwith "at least one success" semantics — if one device is unreachable, others still get the updateresolveCanvasNodeIds()helper that returns all connected canvas-capable node IDsMotivation
Users with multiple devices (desktop + phone, or multiple browsers) only see canvas/a2ui updates on one device. The canvas tool resolved to a single node via
resolveNodeId()withpreferLocalMac: true, so the Mac desktop always won. The other device never received UI updates.Canvas state is inherently device-independent — it's UI data, not system commands. Broadcasting it matches how chat events already work (all clients see them).
Test plan
a2ui_pushupdatesnodeparameter still targets a single nodesnapshotandevalstill go to single node🤖 Generated with Claude Code