Skip to content

Add wh notification decoding#273

Merged
roznawsk merged 5 commits into
mainfrom
FCE-3416-notification-batching
Jun 12, 2026
Merged

Add wh notification decoding#273
roznawsk merged 5 commits into
mainfrom
FCE-3416-notification-batching

Conversation

@roznawsk

@roznawsk roznawsk commented Jun 11, 2026

Copy link
Copy Markdown
Member

Description

Add function that decodes notifications received by webhook.

Motivation and Context

Why is this change required? What problem does it solve? If it fixes an open
issue, please link to the issue here.

Documentation impact

  • Documentation update required
  • Documentation updated in another PR
  • No documentation update required

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)

@linear

linear Bot commented Jun 11, 2026

Copy link
Copy Markdown

FCE-3416

Copilot AI left a comment

Copy link
Copy Markdown

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 a webhook-focused decoder that turns raw protobuf webhook bodies into the same typed/mapped notification payloads that the WebSocket notifier emits, including transparent unwrapping of notificationBatch frames.

Changes:

  • Added decodeServerNotifications helper for decoding application/x-protobuf webhook bodies into mapped ServerNotification[].
  • Refactored notification decoding to a shared extractNotifications() path used by both the WebSocket notifier and the webhook decoder (including batch unwrapping + filtering non-surfaced variants).
  • Added test coverage for webhook decoding/batch behavior and strengthened type-level guarantees around ServerNotification.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/js-server-sdk/tests/webhook.test.ts Adds runtime tests for webhook decoding, payload mapping, batch unwrapping, and supported input types.
packages/js-server-sdk/tests/notifications.test.ts Adds a type-level test ensuring ServerNotification['type'] matches ExpectedEvents.
packages/js-server-sdk/src/ws_notifier.ts Switches WS dispatch to use shared extraction/mapping and emit batch contents in wire order.
packages/js-server-sdk/src/webhook.ts Introduces decodeServerNotifications for decoding raw webhook bodies into mapped notifications.
packages/js-server-sdk/src/notifications.ts Adds ServerNotification type and extractNotifications() shared decoding/mapping logic.
packages/js-server-sdk/src/index.ts Exports decodeServerNotifications and ServerNotification from the package entrypoint.
examples/room-manager/src/index.ts Updates protobuf content-type parsing example to return decoded notifications (supports batched webhooks).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@roznawsk roznawsk requested a review from czerwiukk June 12, 2026 13:50
@roznawsk roznawsk marked this pull request as ready for review June 12, 2026 13:51

@czerwiukk czerwiukk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

assuming the emit issue got resolved - lgtm

* ```
* @category Notifications
*/
export const decodeServerNotifications = (data: Uint8Array | ArrayBuffer): ServerNotification[] =>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: shouldn't it land in ./notifications/?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Given that websocket is in its own file, it's ok for WH to be separate (though the amount of code and abstraction is minimal)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comment thread .github/workflows/integration-tests.yaml
@roznawsk roznawsk merged commit 8a9cdd0 into main Jun 12, 2026
4 checks passed
@roznawsk roznawsk deleted the FCE-3416-notification-batching branch June 12, 2026 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants