[Web runtime] Add core action capability dispatch - #1586
Open
minggangw wants to merge 3 commits into
Open
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are protocol/type-surface inconsistencies (missing status in TS types) and an ambiguous error-code choice for unknown action op that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds initial ROS 2 action capability support to the rclnodejs-web runtime by extending the capability registry/CLI surface area and implementing basic action dispatch in the runtime dispatcher, with accompanying test updates.
Changes:
- Extend capability configuration/schema to include
action(CLI parsing, registry, startup banner, TS types). - Implement WebSocket dispatcher handling for action frames (
send_goal,cancel) usingActionClient. - Update CLI/runtime tests to cover action exposure and action-frame rejection paths.
File summaries
| File | Description |
|---|---|
| test/test-web-cli.js | Verifies CLI config parsing supports --action and exposes it in partial config. |
| test/test-runtime.js | Expands unit/e2e tests to include action registry entries and action-frame error handling. |
| lib/runtime/index.d.ts | Extends public types for action capabilities and action frame fields. |
| lib/runtime/dispatcher.js | Implements action dispatch (send_goal, cancel) and connection cleanup for action clients/goals. |
| lib/runtime/cli-config.js | Adds action to defaults/argv parsing and documents the new flag in --help. |
| lib/runtime/capability_registry.js | Adds action support to registry storage, expose(), list(), and resolve(). |
| bin/rclnodejs-web.js | Includes action capabilities in the startup “capabilities” count. |
Review details
- Files reviewed: 5/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+268
to
+272
| conn.send({ | ||
| id, | ||
| ok: false, | ||
| error: `unknown action op: ${op}`, | ||
| code: 'unknown_kind', |
| --call <name>=<type> expose a service capability (repeatable) | ||
| --publish <name>=<type> expose a topic publish (repeatable) | ||
| --subscribe <n>=<type> expose a topic subscription (repeatable) | ||
| --action <name>=<type> expose an action capability (repeatable) |
minggangw
force-pushed
the
fix-1579-core
branch
from
September 3, 2026 08:08
188159b to
dbf9f31
Compare
|
coverage: 90.718% (-0.4%) from 91.068% — minggangw:fix-1579-core into RobotWebTools:develop |
There was a problem hiding this comment.
🟡 Changes recommended
Action lifecycle handling has resource-leak, cancellation-reporting, teardown-race, and coverage gaps.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/7 changed files
- Comments generated: 5
- Review effort level: Balanced
Comment on lines
+414
to
+416
| if ([...state.pendingGoals.values()].includes(name)) return; | ||
| for (const goal of state.goals.values()) { | ||
| if (goal.name === name) return; |
Comment on lines
+311
to
+314
| pendingGoals.set(id, name); | ||
| let sendGoal; | ||
| try { | ||
| sendGoal = actionClient.sendGoal(goal, feedbackCallback); |
| } | ||
| goals.set(id, { name, goalHandle }); | ||
| conn.send({ id, ok: true, payload: { accepted: true } }); | ||
| goalHandle.getResult().then( |
Comment on lines
+399
to
+400
| entry.goalHandle.cancelGoal().then( | ||
| () => conn.send({ id, ok: true }), |
Comment on lines
+262
to
+266
| if (op === 'send_goal') { | ||
| return this._handleActionSendGoal(conn, id, name, frame.payload, state); | ||
| } | ||
| if (op === 'cancel') { | ||
| return this._handleActionCancel(conn, id, frame.goalId, state); |
There was a problem hiding this comment.
🔵 Needs a closer look
Disconnected, non-terminating actions can retain native clients indefinitely, and the public protocol documentation remains contradictory.
Review details
Suppressed comments (1)
lib/runtime/dispatcher.js:88
- A disconnected client can leave this
ActionClientalive indefinitely: ROS actions are not guaranteed to complete, and both an unanswered goal request and an accepted goal whose result never arrives remain inpendingGoals/goals, so_releaseActionClientIfIdlewill never destroy the native client. Repeating this across short-lived WebSocket connections creates unbounded native clients (and orphaned server-side goals). The disconnect path needs a bounded lifecycle—for example, cancel accepted goals and retain the client only while cancel/result settlement is bounded, or move action clients to shared runtime ownership—without destroying while an rcl request is pending.
// Deliberately not cancelling in-flight goals here: cancelGoal() is
// itself async, and destroying the ActionClient synchronously right
// after firing it (without awaiting the response) races the pending
// rcl reply — same "client will not receive response" native crash
// documented on HttpRequestConnection above. A goal already in flight
// when the connection drops just runs to completion server-side;
// conn.send() on a closed connection is already a documented no-op.
- Files reviewed: 5/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.