Skip to content

fix(pingrip): forward data text frames in OPEN response#683

Merged
HubertBel merged 8 commits into
masterfrom
bread/pingrip
May 6, 2026
Merged

fix(pingrip): forward data text frames in OPEN response#683
HubertBel merged 8 commits into
masterfrom
bread/pingrip

Conversation

@HubertBel

Copy link
Copy Markdown
Contributor

Pushpin's default message-prefix is "m:", text/binary frames in the OPEN response not starting with that prefix are silently dropped. OpenResponseBuilder.text() and .binary() now prepend the prefix automatically so data frames are forwarded to the WebSocket client as intended. Pushpin strips the prefix before delivering to the browser.

@HubertBel HubertBel requested a review from a team as a code owner May 5, 2026 14:10
@greptile-apps

greptile-apps Bot commented May 5, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes silent message drops in Pushpin's WebSocket-over-HTTP protocol by prepending the m: message-prefix to data frames emitted from OpenResponseBuilder. It also corrects a latent double-prefix bug in subscribe() (flagged in a previous review) by routing control frames directly through _builder.text() rather than through the now-prefixing OpenResponseBuilder.text().

  • OpenResponseBuilder.text() and .binary() now prepend m: so Pushpin forwards the payload to the WebSocket client instead of silently dropping it.
  • subscribe() is fixed to call this._builder.text() directly, ensuring c:-prefixed control frames are not mistakenly wrapped with m:.
  • A new test file (outputs.test.ts) validates all four cases: data text, data binary, subscribe control frames, and unsubscribe control frames.

Confidence Score: 5/5

Safe to merge — the fix is narrow, well-tested, and the double-prefix regression flagged in a prior review has been correctly addressed.

All three changed paths (text, binary, subscribe) are straightforward, covered by the new test suite, and the previously identified subscribe double-prefix issue is resolved by routing directly through _builder.text(). No other code paths are affected.

No files require special attention.

Important Files Changed

Filename Overview
pingrip/src/outputs.ts Prepends m: in text()/binary() and routes subscribe() through _builder.text() directly — double-prefix issue from previous review is resolved.
pingrip/src/outputs.test.ts New test file covering text(), binary(), subscribe(), and unsubscribe() frame prefixing; assertions match expected Pushpin GRIP wire format.
pingrip/package.json Patch version bump from 0.1.1 to 0.1.2 reflecting the bug fix.

Sequence Diagram

sequenceDiagram
    participant App
    participant OpenResponseBuilder
    participant ResponseBuilder
    participant Pushpin
    participant WSClient as WebSocket Client

    App->>OpenResponseBuilder: .text("hello")
    OpenResponseBuilder->>ResponseBuilder: .text("m:hello")
    Note over ResponseBuilder: stores TEXT frame with content "m:hello"

    App->>OpenResponseBuilder: .binary(buf)
    OpenResponseBuilder->>ResponseBuilder: .binary(concat("m:", buf))
    Note over ResponseBuilder: stores BINARY frame with m: prefix

    App->>OpenResponseBuilder: .subscribe(["chan"])
    OpenResponseBuilder->>ResponseBuilder: .text('c:{"type":"subscribe","channel":"chan"}')
    Note over ResponseBuilder: stores TEXT frame with c: prefix (no m: wrapping)

    App->>OpenResponseBuilder: .toResponse()
    OpenResponseBuilder->>ResponseBuilder: .toResponse()
    ResponseBuilder-->>Pushpin: HTTP response body (serialized frames) + Grip headers

    Pushpin->>Pushpin: strips "m:" prefix from data frames
    Pushpin->>WSClient: forwards "hello" as text frame
    Pushpin->>Pushpin: processes "c:" control frames internally (subscribe)
Loading

Reviews (2): Last reviewed commit: "fix tests name" | Re-trigger Greptile

@HubertBel HubertBel requested a review from SebastienPoitras May 5, 2026 14:32
@HubertBel

Copy link
Copy Markdown
Contributor Author

@greptile

@SebastienPoitras

Copy link
Copy Markdown
Contributor

note: As discussed, we'll refactor the package to remove Grip headers if they turn out to be unnecessary and we'll correctly add the m: and c: prefixes to binary and text messages

@HubertBel HubertBel requested a review from SebastienPoitras May 5, 2026 19:51
Comment thread pingrip/src/outputs.ts Outdated
Comment thread pingrip/src/outputs.ts Outdated
@HubertBel HubertBel requested a review from SebastienPoitras May 6, 2026 12:07
@HubertBel HubertBel merged commit ced3d45 into master May 6, 2026
1 check passed
@HubertBel HubertBel deleted the bread/pingrip branch May 6, 2026 12:29
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.

2 participants