Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces streamCore.ts to handle unified SSE transform streams with idle timeout protection, supporting both translation and passthrough modes for various LLM formats. The code review identifies several critical issues, including a large number of missing imports that will cause runtime failures, as well as duplicate and unused imports. Additionally, the reviewer notes that asynchronous onFailure callbacks are wrapped in synchronous try/catch blocks (failing to catch rejections) and that errors in onComplete callbacks are being silently swallowed, which violates the repository's style guide against swallowing errors in SSE streams.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| import { normalizeStreamFailurePayload } from "./errors.ts"; | ||
| import { buildErrorBody } from "../error.ts"; | ||
| import { SSEStreamContext } from "./types.ts"; | ||
|
|
There was a problem hiding this comment.
Several functions, constants, and utilities referenced in this file are not imported, which will cause compilation/runtime errors. Please import the following:
sanitizeStreamingChunkextractThinkingFromContenthasValidUsageestimateUsagefilterUsageForFormataddBufferToUsageconsumeToolFinishTimeformatSSEhasValuableContentfixInvalidIdOMIT_STREAMING_CHUNK_MARKERmarkToolFinishunwrapGeminiChunklogUsagebuildStreamSummaryFromEventsparseTextualToolCallCandidate
| import { convertOpenAIToResponsesToolCall } from "../handlers/responseTranslator.ts"; | ||
| import { translateResponse, initState } from "../../translator/index.ts"; | ||
| import { v4 as uuidv4 } from "uuid"; |
There was a problem hiding this comment.
The imports convertOpenAIToResponsesToolCall and uuidv4 are not used anywhere in this file and can be safely removed.
| import { convertOpenAIToResponsesToolCall } from "../handlers/responseTranslator.ts"; | |
| import { translateResponse, initState } from "../../translator/index.ts"; | |
| import { v4 as uuidv4 } from "uuid"; | |
| import { translateResponse, initState } from "../../translator/index.ts"; |
| import { | ||
| STREAM_IDLE_TIMEOUT_MS, | ||
| FETCH_BODY_TIMEOUT_MS, | ||
| HTTP_STATUS, | ||
| } from "../../config/constants.ts"; |
There was a problem hiding this comment.
The import FETCH_BODY_TIMEOUT_MS is not used in this file and can be safely removed.
import {
STREAM_IDLE_TIMEOUT_MS,
HTTP_STATUS,
} from "../../config/constants.ts";| } from "../../config/constants.ts"; | ||
| import { parseSSELine } from "../streamHelpers.ts"; | ||
| import { recordToolLatency } from "../../services/toolLatencyTracker.ts"; | ||
| import { processBufferedPassthroughLine } from "../passthroughTailProcessor.ts"; |
There was a problem hiding this comment.
The import processBufferedPassthroughLine is not used in this file and can be safely removed.
| import { normalizeStreamFailurePayload } from "./errors.ts"; | ||
| import { buildErrorBody } from "../error.ts"; |
There was a problem hiding this comment.
These imports are duplicates of the imports on lines 18-19 and should be removed.
| if (ctx.onFailure) { | ||
| try { | ||
| void ctx.onFailure(failurePayload); | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
Since ctx.onFailure can return a Promise<void>, wrapping it in a synchronous try/catch block without await will not catch asynchronous rejections. This can lead to unhandled promise rejections. Wrapping it in Promise.resolve().catch() safely catches any asynchronous errors.
| if (ctx.onFailure) { | |
| try { | |
| void ctx.onFailure(failurePayload); | |
| } catch {} | |
| } | |
| if (ctx.onFailure) { | |
| Promise.resolve(ctx.onFailure(failurePayload)).catch((err) => { | |
| console.error("[STREAM] Error in onFailure callback:", err); | |
| }); | |
| } |
| }); | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
Silently swallowing errors thrown inside the onComplete callback makes debugging extremely difficult. Logging the error to the console provides visibility when issues occur.
});
} catch (err) {
console.error("[STREAM] Error in onComplete callback:", err);
}
}| if (ctx.onFailure) { | ||
| try { | ||
| void ctx.onFailure({ | ||
| status: err.status, | ||
| message: err.message, | ||
| code: err.code, | ||
| type: err.type, | ||
| }); | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
Since ctx.onFailure can return a Promise<void>, wrapping it in a synchronous try/catch block without await will not catch asynchronous rejections. Wrapping it in Promise.resolve().catch() safely catches any asynchronous errors.
| if (ctx.onFailure) { | |
| try { | |
| void ctx.onFailure({ | |
| status: err.status, | |
| message: err.message, | |
| code: err.code, | |
| type: err.type, | |
| }); | |
| } catch {} | |
| } | |
| if (ctx.onFailure) { | |
| Promise.resolve( | |
| ctx.onFailure({ | |
| status: err.status, | |
| message: err.message, | |
| code: err.code, | |
| type: err.type, | |
| }) | |
| ).catch((err) => { | |
| console.error("[STREAM] Error in onFailure callback:", err); | |
| }); | |
| } |
| }); | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
Silently swallowing errors thrown inside the onComplete callback makes debugging extremely difficult. Logging the error to the console provides visibility when issues occur.
});
} catch (err) {
console.error("[STREAM] Error in onComplete callback:", err);
}
}
…error handling Address review feedback on diegosouzapw#3927: - Add 15 missing imports (sanitizeStreamingChunk, estimateUsage, etc.) - Remove unused imports (convertOpenAIToResponsesToolCall, uuidv4, FETCH_BODY_TIMEOUT_MS, processBufferedPassthroughLine, duplicates) - Fix async onFailure with Promise.resolve().catch() pattern - Add console.error logging in onComplete catch blocks
Part of Issue diegosouzapw#3594 modularization. Extracted core streaming logic from the monolithic stream.ts. Exports: - createSSEStream, createSSETransformStreamWithLogger, createPassthroughStreamWithLogger - buildSSEStreamContext (internal) This is the main streaming engine handling passthrough, translate, and Responses API modes.
Part of Issue diegosouzapw#3594 modularization. The core streaming module is 2216 lines and is added as a frozen entry since it's a cohesive core module that cannot be easily split further.
…error handling Address review feedback on diegosouzapw#3927: - Add 15 missing imports (sanitizeStreamingChunk, estimateUsage, etc.) - Remove unused imports (convertOpenAIToResponsesToolCall, uuidv4, FETCH_BODY_TIMEOUT_MS, processBufferedPassthroughLine, duplicates) - Fix async onFailure with Promise.resolve().catch() pattern - Add console.error logging in onComplete catch blocks
|
Thanks, @oyi77 🙏. These stream extractions are currently additive — the new modules under |
2 participants
Part of modularization effort (Issue #3594).
Extracts core streaming logic from the monolithic
open-sse/utils/stream.tsinto a dedicated module.Changes:
open-sse/utils/stream/streamCore.ts(2215 lines)createSSEStream,createSSETransformStreamWithLogger,createPassthroughStreamWithLoggerDependencies: Requires the following modules (submitted as prior PRs):
Testing: All stream tests pass (49/49 stream-utils, 14/14 stream-handler).
Note: This is the largest module (~2215 lines). It's the core streaming engine and is cohesive. The prior extracted modules are its dependencies.