Skip to content

Commit 1c61761

Browse files
committed
fix: address PR #49 review feedback
- Replace npx with pnpm dlx in CI actions for consistency - Add type guard for safer response type checking - Refactor IIFE patterns in getMessage/getText to private helper methods - Add braces to single-line if statement for readability
1 parent 9073996 commit 1c61761

File tree

2 files changed

+58
-26
lines changed

2 files changed

+58
-26
lines changed

.github/actions/validate-sdk/action.yaml

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,19 +43,24 @@ runs:
4343
working-directory: examples/nextjs-example
4444
run: npm install
4545

46+
- name: Setup pnpm
47+
uses: pnpm/action-setup@v4
48+
with:
49+
version: 9
50+
4651
- name: Typecheck nextjs-example
4752
shell: bash
4853
working-directory: examples/nextjs-example
49-
run: npx tsc --noEmit
54+
run: pnpm dlx tsc --noEmit
5055

5156
- name: Run unit tests
5257
shell: bash
5358
env:
5459
OPENROUTER_API_KEY: ${{ inputs.openrouter-api-key }}
55-
run: npx vitest --run --exclude 'tests/e2e/**'
60+
run: pnpm dlx vitest --run --exclude 'tests/e2e/**'
5661

5762
- name: Run e2e tests
5863
shell: bash
5964
env:
6065
OPENROUTER_API_KEY: ${{ inputs.openrouter-api-key }}
61-
run: npx vitest --run tests/e2e/
66+
run: pnpm dlx vitest --run tests/e2e/

src/lib/response-wrapper.ts

Lines changed: 50 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,20 @@ export class ResponseWrapper {
7070
this.options = options;
7171
}
7272

73+
/**
74+
* Type guard to check if a value is a non-streaming response
75+
*/
76+
private isNonStreamingResponse(value: unknown): value is models.OpenResponsesNonStreamingResponse {
77+
return (
78+
value !== null &&
79+
typeof value === "object" &&
80+
"id" in value &&
81+
"object" in value &&
82+
"output" in value &&
83+
!("toReadableStream" in value)
84+
);
85+
}
86+
7387
/**
7488
* Initialize the stream if not already started
7589
* This is idempotent - multiple calls will return the same promise
@@ -265,10 +279,13 @@ export class ResponseWrapper {
265279
const value = newResult.value;
266280
if (value && typeof value === "object" && "toReadableStream" in value) {
267281
// It's a stream, consume it
268-
const stream = new ReusableReadableStream(value as EventStream<models.OpenResponsesStreamEvent>);
282+
const streamValue = value as EventStream<models.OpenResponsesStreamEvent>;
283+
const stream = new ReusableReadableStream(streamValue);
269284
currentResponse = await consumeStreamForCompletion(stream);
285+
} else if (this.isNonStreamingResponse(value)) {
286+
currentResponse = value;
270287
} else {
271-
currentResponse = value as models.OpenResponsesNonStreamingResponse;
288+
throw new Error("Unexpected response type from API");
272289
}
273290

274291
currentRound++;
@@ -280,6 +297,32 @@ export class ResponseWrapper {
280297
return this.toolExecutionPromise;
281298
}
282299

300+
/**
301+
* Internal helper to get the message after tool execution
302+
*/
303+
private async getMessageInternal(): Promise<models.AssistantMessage> {
304+
await this.executeToolsIfNeeded();
305+
306+
if (!this.finalResponse) {
307+
throw new Error("Response not available");
308+
}
309+
310+
return extractMessageFromResponse(this.finalResponse);
311+
}
312+
313+
/**
314+
* Internal helper to get the text after tool execution
315+
*/
316+
private async getTextInternal(): Promise<string> {
317+
await this.executeToolsIfNeeded();
318+
319+
if (!this.finalResponse) {
320+
throw new Error("Response not available");
321+
}
322+
323+
return extractTextFromResponse(this.finalResponse);
324+
}
325+
283326
/**
284327
* Get the completed message from the response.
285328
* This will consume the stream until completion, execute any tools, and extract the first message.
@@ -290,16 +333,7 @@ export class ResponseWrapper {
290333
return this.messagePromise;
291334
}
292335

293-
this.messagePromise = (async (): Promise<models.AssistantMessage> => {
294-
await this.executeToolsIfNeeded();
295-
296-
if (!this.finalResponse) {
297-
throw new Error("Response not available");
298-
}
299-
300-
return extractMessageFromResponse(this.finalResponse);
301-
})();
302-
336+
this.messagePromise = this.getMessageInternal();
303337
return this.messagePromise;
304338
}
305339

@@ -312,16 +346,7 @@ export class ResponseWrapper {
312346
return this.textPromise;
313347
}
314348

315-
this.textPromise = (async () => {
316-
await this.executeToolsIfNeeded();
317-
318-
if (!this.finalResponse) {
319-
throw new Error("Response not available");
320-
}
321-
322-
return extractTextFromResponse(this.finalResponse);
323-
})();
324-
349+
this.textPromise = this.getTextInternal();
325350
return this.textPromise;
326351
}
327352

@@ -501,7 +526,9 @@ export class ResponseWrapper {
501526
const consumer = this.reusableStream.createConsumer();
502527

503528
for await (const event of consumer) {
504-
if (!("type" in event)) continue;
529+
if (!("type" in event)) {
530+
continue;
531+
}
505532

506533
// Transform responses events to chat-like format
507534
// This is a simplified transformation - you may need to adjust based on your needs

0 commit comments

Comments
 (0)