Skip to content

Conversation

@vicancy
Copy link
Member

@vicancy vicancy commented Nov 26, 2025

Packages impacted by this PR

@azure/web-pubsub-client

Issues associated with this PR

Describe the problem that is addressed by this PR

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

  • Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)

Copilot AI review requested due to automatic review settings November 26, 2025 04:20
Copilot finished reviewing on behalf of vicancy November 26, 2025 04:25
@github-actions
Copy link

API Change Check

APIView identified API level changes in this PR and created the following API reviews

@azure/web-pubsub-client

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds invoke support to the @azure/web-pubsub-client package, enabling clients to send upstream events and await correlated responses from the service. This implements a request-response pattern on top of the existing WebSocket-based pub/sub messaging system.

Key Changes

  • New InvocationManager: Manages pending invocations with correlation by invocation ID, supporting abort signals and automatic cleanup on disconnection
  • New message types: Adds InvokeMessage, InvokeResponseMessage, and CancelInvocationMessage to the protocol
  • Public API: Introduces invokeEvent() method for sending invocations with configurable invocation IDs and abort signals
  • Error handling: New InvocationError class for invocation-specific failures, which bypasses retry logic to prevent duplicate processing

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/invocationManager.ts New manager class for tracking pending invocations, handling promise resolution/rejection, and managing abort signal lifecycle
src/webPubSubClient.ts Integrates InvocationManager; adds invokeEvent() public API; handles invokeResponse messages; rejects pending invocations on disconnection; prevents retry on InvocationError
src/protocols/jsonProtocolBase.ts Adds parsing and serialization for invoke, invokeResponse, and cancelInvocation messages
src/models/messages.ts Defines InvokeMessage, InvokeResponseMessage, CancelInvocationMessage, and InvokeResponseError interfaces
src/models/index.ts Exports InvokeEventOptions and InvokeEventResult for public API surface
src/errors/index.ts Adds InvocationError class and InvocationErrorOptions interface
test/invocationManager.spec.ts Comprehensive unit tests for InvocationManager covering registration, resolution, rejection, abort handling, and rejectAll
test/client.invoke.spec.ts Integration tests for invokeEvent covering successful invocations, service errors, and abort signal handling with cancelInvocation
review/web-pubsub-client-node.api.md API review file updated with new public interfaces and types
README.md Documentation example showing how to use the new invokeEvent feature

Comment on lines +367 to +368
export interface CancelInvocationMessage extends WebPubSubMessageBase {
readonly kind: "cancelInvocation";
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add JSDoc comments to the kind and invocationId properties of CancelInvocationMessage to match the documentation pattern used in other message interfaces.

Suggested change
export interface CancelInvocationMessage extends WebPubSubMessageBase {
readonly kind: "cancelInvocation";
export interface CancelInvocationMessage extends WebPubSubMessageBase {
/**
* The message kind.
*/
readonly kind: "cancelInvocation";
/**
* The invocation ID to cancel.
*/

Copilot uses AI. Check for mistakes.
}
}


Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Remove the extra blank line to maintain consistent code formatting.

Suggested change

Copilot uses AI. Check for mistakes.
await expect(invokePromise).rejects.toThrow(InvocationError);

// Verify cancelInvocation message is sent
// The last call should be cancelInvocation.
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Remove the trailing space at the end of the comment line.

Suggested change
// The last call should be cancelInvocation.
// The last call should be cancelInvocation.

Copilot uses AI. Check for mistakes.
Comment on lines +46 to +47
export interface InvocationErrorOptions {
invocationId: string;
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add JSDoc comments to the InvocationErrorOptions interface properties to be consistent with the pattern used for SendMessageErrorOptions above it.

Suggested change
export interface InvocationErrorOptions {
invocationId: string;
export interface InvocationErrorOptions {
/**
* The invocation id of the request.
*/
invocationId: string;
/**
* Error details from the service if available.
*/

Copilot uses AI. Check for mistakes.
Comment on lines +326 to +327
export interface InvokeMessage extends WebPubSubMessageBase {
readonly kind: "invoke";
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add JSDoc comments to the kind and invocationId properties of InvokeMessage to match the documentation pattern used in other message interfaces like SendToGroupMessage.

Suggested change
export interface InvokeMessage extends WebPubSubMessageBase {
readonly kind: "invoke";
export interface InvokeMessage extends WebPubSubMessageBase {
/**
* Message type
*/
readonly kind: "invoke";
/**
* The invocation id
*/

Copilot uses AI. Check for mistakes.
*/
export interface InvokeMessage extends WebPubSubMessageBase {
readonly kind: "invoke";
invocationId: string;
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add JSDoc comments to the target property of InvokeMessage to match the documentation pattern used in other message interfaces.

Suggested change
invocationId: string;
invocationId: string;
/**
* The invocation target type. Currently, only upstream events are supported.
*/

Copilot uses AI. Check for mistakes.
Comment on lines +347 to +352
export interface InvokeResponseMessage extends WebPubSubMessageBase {
readonly kind: "invokeResponse";
invocationId: string;
success?: boolean;
dataType?: WebPubSubDataType;
data?: JSONTypes | ArrayBuffer;
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add JSDoc comments to all properties of InvokeResponseMessage to match the documentation pattern used in other message interfaces like SendToGroupMessage.

Suggested change
export interface InvokeResponseMessage extends WebPubSubMessageBase {
readonly kind: "invokeResponse";
invocationId: string;
success?: boolean;
dataType?: WebPubSubDataType;
data?: JSONTypes | ArrayBuffer;
export interface InvokeResponseMessage extends WebPubSubMessageBase {
/**
* Message type.
*/
readonly kind: "invokeResponse";
/**
* The invocation ID that this response is for.
*/
invocationId: string;
/**
* Indicates whether the invocation was successful.
*/
success?: boolean;
/**
* Data type of the payload.
*/
dataType?: WebPubSubDataType;
/**
* Payload data.
*/
data?: JSONTypes | ArrayBuffer;
/**
* Error details if the invocation failed.
*/

Copilot uses AI. Check for mistakes.
Comment on lines +359 to +360
export interface InvokeResponseError {
name: string;
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add JSDoc comments to the name and message properties of InvokeResponseError to match the documentation pattern used in other interfaces.

Suggested change
export interface InvokeResponseError {
name: string;
export interface InvokeResponseError {
/**
* The error name.
*/
name: string;
/**
* The error message.
*/

Copilot uses AI. Check for mistakes.
}
};

abortSignal.addEventListener("abort", onAbort);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these listeners may leak if _sendMessage throws in _invokeEventAttempt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants