Skip to content

QVAC-18524 fix: avoid Node-only Buffer in RN duplex RPC path#1915

Merged
Victor-Rodzko merged 3 commits into
mainfrom
QVAC-18524-fix-buffer-rn-hermes-duplex
May 8, 2026
Merged

QVAC-18524 fix: avoid Node-only Buffer in RN duplex RPC path#1915
Victor-Rodzko merged 3 commits into
mainfrom
QVAC-18524-fix-buffer-rn-hermes-duplex

Conversation

@Victor-Rodzko
Copy link
Copy Markdown
Contributor

@Victor-Rodzko Victor-Rodzko commented May 6, 2026

Note: be concise and prefer bullet points.

🎯 What problem does this PR solve?

  • On RN/Hermes (iOS + Android) every transcribeStream() call from @qvac/sdk throws ReferenceError: Property 'Buffer' doesn't exist on the very first write.
  • The new transcribe-stream-events-* e2e tests (added in QVAC-17282 feat[api]: expose whisper VAD and end-of-turn events in transcribeStream #1848 / QVAC-17282) cannot run on mobile at all today.
  • Root cause: the bare-rpc duplex stream polyfill internally does Buffer.from(chunk, encoding) for string writes, but Hermes has no global Buffer — so requestStream.write(payload, "utf-8") blows up before any audio data reaches the worker.

📝 How does it solve it?

  • Pre-encode the JSON request payload to a Uint8Array via TextEncoder in createDuplexSession so the polyfill's binary branch is taken on every runtime.
  • Drop Buffer.from(string) from the profiled duplex response generator and yield strings directly — downstream chunk.toString() consumers work for both Buffer and string.
  • Widen the public TranscribeStream*Session.write(audioChunk) signatures from Buffer to Buffer | Uint8Array so RN callers can pass binary slices directly instead of polyfilling Buffer.
  • Stop wrapping Uint8Array slices in Buffer.from(...) inside the shared transcribe-stream test runner.
  • Skip transcribe-stream-events-* on iOS under the existing TODO(QVAC-18460) since the underlying native Silero/Whisper crash is identical to the one already blocking transcription-* on iOS — flipping both skips will be a single follow-up commit when 18460 lands.
  • Add a one-line rule to tests-qvac.mdc: no Node-only globals in tests/shared/ or tests/mobile/.

Type Changes

Additive type widening — all existing Buffer-based callers compile and behave identically.

// Before
interface TranscribeStreamSession {
  write(audioChunk: Buffer): void;
  // ...
}

// After
interface TranscribeStreamSession {
  write(audioChunk: Buffer | Uint8Array): void;
  // ...
}

Also widened on TranscribeStreamMetadataSession and TranscribeStreamConversationSession, and on the DuplexWritable / DuplexReadable internal RPC types.

Example RN/Bare-friendly usage:

import { transcribeStream } from "@qvac/sdk";

const session = await transcribeStream({ /* ... */ });
const pcm = new Uint8Array(audioBuffer); // straight from any binary source
session.write(pcm);                       // ✅ no Buffer needed
session.end();
for await (const text of session) console.log(text);

🧪 How was it tested?

  • Local bun run lint + tsc --noEmit on packages/sdk — green.
  • Local iOS run via npx qvac-test run:local:ios --filter transcribe-stream-events:
    • Before: ReferenceError: Property 'Buffer' doesn't exist ~70 ms after test start.
    • After: tests skipped on iOS with the QVAC-18460 reason; the JS-side wire-up no longer throws when run against a non-crashing native (verified locally before the iOS-specific Silero crash kicks in).
  • Desktop transcribe-stream-events-* continue to pass — Buffer callers compile and execute unchanged thanks to the union widening.

ishanvohra2
ishanvohra2 previously approved these changes May 6, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

Tier-based Approval Status

**PR Tier:** TIER1

**Current Status:** ✅ APPROVED

**Requirements:**
- 1 Team Member approval ✅ (1/1)
- 1 Team Lead OR Management approval ✅ (1/1)



---
*This comment is automatically updated when reviews change.*

Comment thread packages/sdk/client/rpc/rpc-client.ts
simon-iribarren
simon-iribarren previously approved these changes May 6, 2026
lauripiisang
lauripiisang previously approved these changes May 7, 2026
@lauripiisang
Copy link
Copy Markdown
Contributor

lauripiisang commented May 7, 2026

@Victor-Rodzko before merge I'd ask to consider removing [api] tag and the "API Changes" from description though -- I'm not sure this constitutes a meaningful API change and will just pollute the changleog
Already did it

@lauripiisang lauripiisang changed the title QVAC-18524 fix[api]: avoid Node-only Buffer in RN duplex RPC path QVAC-18524 fix: avoid Node-only Buffer in RN duplex RPC path May 7, 2026
Victor-Rodzko and others added 2 commits May 7, 2026 18:52
On RN/Hermes the bare-rpc duplex stream polyfill calls
`Buffer.from(chunk, encoding)` for string writes, which throws
`Property 'Buffer' doesn't exist` because Hermes has no global
`Buffer`. This blocks every transcribeStream() call on iOS/Android.

- expo-rpc-client: pre-encode the JSON payload to Uint8Array via
  TextEncoder so the polyfill's binary branch is taken everywhere.
- rpc-client: drop Buffer.from in the profiled response generator,
  widen DuplexWritable/DuplexReadable to accept Uint8Array/string.
- transcribe API + transcription schemas: widen all
  TranscribeStream*Session.write(audioChunk) signatures from `Buffer`
  to `Buffer | Uint8Array` so RN callers can pass Uint8Array directly.
- tests-qvac shared runner: stop wrapping Uint8Array slices in
  Buffer.from before writing.
- tests-qvac mobile consumer: skip transcribe-stream-events-* on iOS
  under the existing QVAC-18460 TODO (same native Silero/Whisper
  crash path as transcription-*).
- tests-qvac.mdc: add a one-liner rule about avoiding Node-only
  globals in shared/mobile test code.

Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the `Buffer | Uint8Array` parameter unions on
`TranscribeStream*Session.write()` and `DuplexWritable.write()` with
plain `Uint8Array`. Node `Buffer` extends `Uint8Array`, so existing
`Buffer`-passing callers keep typechecking, but the public surface no
longer mentions a Node-only type. Per review feedback from @lauripiisang.
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.

5 participants