Skip to content

Maintenance: Extract shared parseEvent() helper from channel transports#34328

Closed
mixelburg wants to merge 1 commit into
storybookjs:nextfrom
mixelburg:refactor/extract-telejson-parse-helper
Closed

Maintenance: Extract shared parseEvent() helper from channel transports#34328
mixelburg wants to merge 1 commit into
storybookjs:nextfrom
mixelburg:refactor/extract-telejson-parse-helper

Conversation

@mixelburg
Copy link
Copy Markdown

@mixelburg mixelburg commented Mar 25, 2026

What does this PR do?

Resolves the duplicated telejson deserialization pattern across all three channel transport implementations.

Problem

The conditional typeof data === 'string' && isJSON(data) ? parse(data, ...) : data expression was copied in three places with subtly different parse options:

File Options passed to parse
channels/websocket/index.ts none (missing options entirely)
channels/postmessage/index.ts global.CHANNEL_OPTIONS || {}
core-server/utils/get-server-channel.ts {} (empty)

The websocket transport was silently ignoring CHANNEL_OPTIONS, which could cause deserialization mismatches when custom options are configured.

Fix

Extract a parseEvent(data) helper to code/core/src/channels/parse-event.ts that always uses global.CHANNEL_OPTIONS || {}. All three transport implementations now call the shared helper, ensuring consistent behaviour and making future option changes a single-file edit.

Closes #34291

Summary by CodeRabbit

  • Refactor
    • Consolidated incoming event message parsing logic across all communication transport mechanisms—WebSocket, PostMessage, and server utilities—into a centralized handler. This unified approach ensures consistent message processing standards across all channels, eliminates redundant code, and enhances long-term maintainability and reliability of the internal communication infrastructure.

…ransports

The conditional telejson deserialization expression was duplicated
across all three channel transports, with inconsistent parse options:
- websocket: parse(data)           — no options
- postmessage: parse(data, global.CHANNEL_OPTIONS || {})
- server-channel: parse(data, {})  — empty options

Extract parseEvent() to code/core/src/channels/parse-event.ts.
All transports now use CHANNEL_OPTIONS (browser) or {} (server/node),
making behaviour consistent across the board and easier to update.

Fixes storybookjs#34291
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 25, 2026

📝 Walkthrough

Walkthrough

A new parseEvent utility function is introduced to centralize message deserialization logic across channel implementations. The function conditionally parses JSON strings using telejson.parse with global.CHANNEL_OPTIONS, otherwise returns the input unchanged. Multiple channel modules (PostMessage, WebSocket, and server channel utility) now delegate parsing to this utility instead of duplicating the conditional detection and parsing logic inline.

Changes

Cohort / File(s) Summary
New parseEvent utility
code/core/src/channels/parse-event.ts
New module exporting parseEvent(data: unknown): any function that conditionally deserializes JSON strings via telejson.parse with global channel options, or returns the original value for non-JSON inputs.
Channel implementations refactored to use parseEvent
code/core/src/channels/postmessage/index.ts, code/core/src/channels/websocket/index.ts, code/core/src/core-server/utils/get-server-channel.ts
Removed inline conditional telejson.isJSON + parse logic from incoming message handlers and replaced with centralized parseEvent() utility calls. Retained stringify imports for outbound message encoding.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
code/core/src/channels/websocket/index.ts (1)

96-101: Consider adding fallback for consistency with parseEvent.

The sendNow method spreads global.CHANNEL_OPTIONS without the || {} fallback used in parseEvent. While spreading undefined is technically safe (it's a no-op), the inconsistency could be confusing for future maintainers.

♻️ Optional: Add fallback for consistency
 private sendNow(event: any) {
   const data = stringify(event, {
     maxDepth: 15,
-    ...global.CHANNEL_OPTIONS,
+    ...(global.CHANNEL_OPTIONS || {}),
   });
   this.socket.send(data);
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/core/src/channels/websocket/index.ts` around lines 96 - 101, The sendNow
method spreads global.CHANNEL_OPTIONS directly which is inconsistent with
parseEvent's use of a fallback; update sendNow (method sendNow in this file) to
spread (global.CHANNEL_OPTIONS || {}) when calling stringify so it behaves the
same as parseEvent and avoids confusion for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@code/core/src/channels/websocket/index.ts`:
- Around line 96-101: The sendNow method spreads global.CHANNEL_OPTIONS directly
which is inconsistent with parseEvent's use of a fallback; update sendNow
(method sendNow in this file) to spread (global.CHANNEL_OPTIONS || {}) when
calling stringify so it behaves the same as parseEvent and avoids confusion for
future maintainers.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0a8e8ce5-2b48-4f31-97b9-3704a49ff5a0

📥 Commits

Reviewing files that changed from the base of the PR and between 1c38a36 and f0aaf04.

📒 Files selected for processing (4)
  • code/core/src/channels/parse-event.ts
  • code/core/src/channels/postmessage/index.ts
  • code/core/src/channels/websocket/index.ts
  • code/core/src/core-server/utils/get-server-channel.ts

import { stringify } from 'telejson';
import WebSocket, { WebSocketServer } from 'ws';

import { parseEvent } from '../../channels/parse-event';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

relative references to sub package exports like storybook/internal/channels are not allowed

@valentinpalkovic valentinpalkovic self-assigned this Mar 30, 2026
@valentinpalkovic valentinpalkovic changed the title refactor(channels): extract shared parseEvent() helper from channel transports Maintenance: Extract shared parseEvent() helper from channel transports Mar 30, 2026
@valentinpalkovic
Copy link
Copy Markdown
Contributor

Closing. See reason here: #34453 (comment)

@github-project-automation github-project-automation Bot moved this from On Hold to Done in Core Team Projects Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Duplicate Code: telejson parse pattern repeated across all channel transport implementations

2 participants