[Web runtime] Add WebSocket action support (2/3) - #1593
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new action() options handling can throw on null, and cancel() returns a payload despite being typed/documented as Promise<void>, so the implementation and declared API need to be aligned.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds ROS 2 action support to the rclnodejs/web WebSocket SDK and its type surface, enabling browser/runtime clients to send goals, receive feedback, await results, and request cancellation via the runtime’s action capability dispatch.
Changes:
- Adds action-related TypeScript types (
ActionName, wire-shape helpers,ActionHandle,ActionStatus) and a typedRosClient.action()overload inweb/index.d.ts. - Implements WebSocket-side goal tracking/dispatch in
web/client.js(goal map, feedback routing, terminal result handling, and cleanup on connection failure). - Adds new runtime+SDK integration tests for action dispatch and expands tsd coverage for the new types.
File summaries
| File | Description |
|---|---|
| web/index.d.ts | Introduces action type helpers and the RosClient.action() API surface for the web SDK. |
| web/client.js | Implements the WebSocket protocol handling for action goals, feedback, results, and cancellation. |
| test/types/index.test-d.ts | Adds tsd assertions to validate the new ActionHandle/ActionStatus typings. |
| test/test-web-action.js | Adds end-to-end tests covering action send_goal, feedback, result statuses, cancellation, and connection loss behavior. |
Review details
- Files reviewed: 3/4 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.
| action(capability, payload, { onFeedback } = {}) { | ||
| if (this._closed || this._isUserClosed) { |
| get status() { | ||
| return status; | ||
| }, | ||
| cancel: () => this._cancelGoal(id), |
|
coverage: 90.904% (+0.1%) from 90.805% — minggangw:fix-1579-websocket into RobotWebTools:develop |
There was a problem hiding this comment.
🟡 Changes recommended
The cancellation return type mismatch and missing action-client type coverage must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
web/client.js:276
_request()resolves with the cancel frame's payload, and the dispatcher includes the serializedCancelGoalresponse on success (lib/runtime/dispatcher.js:452-456). Consequently JavaScript callers receive an object even thoughActionHandle.cancel()is declared asPromise<void>. Discard the internal response here (or change the public return type); the former preserves the newly documented API.
_cancelGoal(goalId) {
return this._request({ kind: 'action', op: 'cancel', goalId });
- Files reviewed: 3/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| declare const webAction: ActionHandle<{ sequence: number[] }>; | ||
| expectType<Promise<{ sequence: number[] }>>(webAction.result); | ||
| expectType<ActionStatus | undefined>(webAction.status); | ||
| expectAssignable<ActionStatus>('succeeded'); | ||
| expectAssignable<ActionStatus>('canceled'); | ||
| expectAssignable<ActionStatus>('aborted'); | ||
| expectAssignable<ActionStatus>('unknown'); | ||
| expectError((webAction.status = 'succeeded')); | ||
| expectType<Promise<void>>(webAction.cancel()); |
There was a problem hiding this comment.
🟡 Changes recommended
Validate feedback callbacks and prevent action() from reopening networking after client closure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| if (this._isReconnecting) { | ||
| return Promise.reject(_connectionLostError()); | ||
| } | ||
| const { onFeedback } = options ?? {}; |
| * @param {(feedback: *) => void} [options.onFeedback] | ||
| */ | ||
| async action(capability, payload, options) { | ||
| const ws = await this._ensureWs(); |
minggangw
changed the title
This PR adds ROS 2 action support to the
rclnodejs/webWebSocket SDK and its type surface, enabling browser/runtime clients to send goals, receive feedback, await results, and request cancellation via the runtime’s action capability dispatch.Changes:
RosClient.action(), deliver feedback throughonFeedback, and return an action handle after goal acceptance with a result promise and per-goal cancellation.succeeded,canceled,aborted, andunknownoutcomes without changing result payloads. Status remains undefined until a terminal response arrives.cancel(), resolving toundefinedwhen accepted and propagating rejection errors to match thePromise<void>contract.Fix: #1579