-
Notifications
You must be signed in to change notification settings - Fork 0
fix(api): address PR #972 review findings for release readiness #974
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -283,72 +283,74 @@ export class ZediChatModel extends BaseChatModel<BaseChatModelCallOptions> { | |
| let finishReason: string | undefined; | ||
| let done = false; | ||
|
|
||
| for await (const chunk of gen) { | ||
| if (chunk.content) { | ||
| accumulated += chunk.content; | ||
| const chatChunk = new ChatGenerationChunk({ | ||
| text: chunk.content, | ||
| message: new AIMessageChunk({ content: chunk.content }), | ||
| }); | ||
| // Surface incremental tokens to LangChain callback consumers so any | ||
| // `streamEvents` listener (e.g. SSE mapper) sees deltas before the | ||
| // final usage event. | ||
| await runManager?.handleLLMNewToken( | ||
| chunk.content, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| { | ||
| chunk: chatChunk, | ||
| }, | ||
| ); | ||
| yield chatChunk; | ||
| } | ||
| if (chunk.done) { | ||
| finishReason = chunk.finishReason; | ||
| done = true; | ||
| break; | ||
| try { | ||
| for await (const chunk of gen) { | ||
| if (chunk.content) { | ||
| accumulated += chunk.content; | ||
| const chatChunk = new ChatGenerationChunk({ | ||
| text: chunk.content, | ||
| message: new AIMessageChunk({ content: chunk.content }), | ||
| }); | ||
| // Surface incremental tokens to LangChain callback consumers so any | ||
| // `streamEvents` listener (e.g. SSE mapper) sees deltas before the | ||
| // final usage event. | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| await runManager?.handleLLMNewToken( | ||
| chunk.content, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| { | ||
| chunk: chatChunk, | ||
| }, | ||
| ); | ||
| yield chatChunk; | ||
| } | ||
| if (chunk.done) { | ||
| finishReason = chunk.finishReason; | ||
| done = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } finally { | ||
| // Streaming providers in this codebase do not return token counts; mirror | ||
| // chat.ts and estimate. Pre-encoded message length is what the user "paid" | ||
| // for, response length is what they received. | ||
| const promptLength = zediMessages.reduce((sum, m) => sum + m.content.length, 0); | ||
| const inputTokens = Math.ceil(promptLength / 4); | ||
| const outputTokens = Math.ceil(accumulated.length / 4); | ||
|
|
||
| // Streaming providers in this codebase do not return token counts; mirror | ||
| // chat.ts and estimate. Pre-encoded message length is what the user "paid" | ||
| // for, response length is what they received. | ||
| const promptLength = zediMessages.reduce((sum, m) => sum + m.content.length, 0); | ||
| const inputTokens = Math.ceil(promptLength / 4); | ||
| const outputTokens = Math.ceil(accumulated.length / 4); | ||
|
|
||
| const usage = await recordZediUsage({ | ||
| db: this.db, | ||
| userId: this.userId, | ||
| modelId: this.modelRowId, | ||
| feature: this.feature, | ||
| usage: { inputTokens, outputTokens }, | ||
| inputCostUnits: this.inputCostUnits, | ||
| outputCostUnits: this.outputCostUnits, | ||
| apiMode: this.apiMode, | ||
| }); | ||
| const usage = await recordZediUsage({ | ||
| db: this.db, | ||
| userId: this.userId, | ||
| modelId: this.modelRowId, | ||
| feature: this.feature, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 対応しました。 |
||
| usage: { inputTokens, outputTokens }, | ||
| inputCostUnits: this.inputCostUnits, | ||
| outputCostUnits: this.outputCostUnits, | ||
| apiMode: this.apiMode, | ||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 対応しました。
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| // Final chunk surfaces aggregate usage so downstream consumers (sseMapper, | ||
| // LangChain callbacks) can read totals from a single ChatGenerationChunk. | ||
| yield new ChatGenerationChunk({ | ||
| text: "", | ||
| message: new AIMessageChunk({ | ||
| content: "", | ||
| response_metadata: { | ||
| // Final chunk surfaces aggregate usage so downstream consumers (sseMapper, | ||
| // LangChain callbacks) can read totals from a single ChatGenerationChunk. | ||
| yield new ChatGenerationChunk({ | ||
| text: "", | ||
| message: new AIMessageChunk({ | ||
| content: "", | ||
| response_metadata: { | ||
| finishReason: finishReason ?? (done ? "stop" : "incomplete"), | ||
| }, | ||
| usage_metadata: { | ||
| input_tokens: usage.inputTokens, | ||
| output_tokens: usage.outputTokens, | ||
| total_tokens: usage.inputTokens + usage.outputTokens, | ||
| }, | ||
| }), | ||
| generationInfo: { | ||
| finishReason: finishReason ?? (done ? "stop" : "incomplete"), | ||
| costUnits: usage.costUnits, | ||
| }, | ||
| usage_metadata: { | ||
| input_tokens: usage.inputTokens, | ||
| output_tokens: usage.outputTokens, | ||
| total_tokens: usage.inputTokens + usage.outputTokens, | ||
| }, | ||
| }), | ||
| generationInfo: { | ||
| finishReason: finishReason ?? (done ? "stop" : "incomplete"), | ||
| costUnits: usage.costUnits, | ||
| }, | ||
| }); | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,13 +91,18 @@ export async function resolveWebSearchModelId( | |
| ) | ||
| .orderBy(asc(aiModels.inputCostUnits), asc(aiModels.outputCostUnits)); | ||
|
|
||
| // Prefer OpenAI when costs tie, since `useWebSearch` (chat completions | ||
| // `web_search_options`) is well-tested in `aiProviders.ts`; Google's | ||
| // `googleSearch` tool is also supported but requires the `tools` payload. | ||
| // 同コストなら OpenAI を優先(`useWebSearch` 経路が安定)。 | ||
| const openai = rows.find((r) => r.provider === "openai"); | ||
| if (openai) return openai.id; | ||
| const google = rows.find((r) => r.provider === "google"); | ||
| if (google) return google.id; | ||
| return null; | ||
| if (rows.length === 0) return null; | ||
|
|
||
| const first = rows[0]; | ||
| if (!first) return null; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 直前の 94 行目で rows.length === 0 の場合に null を返しているため、この if (!first) チェックは冗長です。コードを簡潔にするために削除を検討してください。 References
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 対応しました。 |
||
|
|
||
| // Prefer OpenAI only among cheapest rows (cost tie-break), since `useWebSearch` | ||
| // is well-tested in `aiProviders.ts`. | ||
| const cheapestInput = first.inputCostUnits; | ||
| const cheapestOutput = first.outputCostUnits; | ||
| const cheapest = rows.filter( | ||
| (r) => r.inputCostUnits === cheapestInput && r.outputCostUnits === cheapestOutput, | ||
| ); | ||
| const preferred = cheapest.find((r) => r.provider === "openai") ?? cheapest[0]; | ||
| return preferred?.id ?? null; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,7 +136,10 @@ export function projectComposeStateValues( | |
| // Interrupt-derived phase wins; row `phase` is only a fallback. | ||
| // interrupt 由来の phase を優先し、行の phase はフォールバックのみ。 | ||
| if (typeof state.phase === "string" && projection.phase === undefined) { | ||
| projection.phase = phaseFromSessionRow(state.phase, "interrupted"); | ||
| projection.phase = | ||
| state.phase.startsWith("completed") || state.phase === "completed" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. state.phase.startsWith("completed") が真であれば state.phase === "completed" の条件も満たされるため、後者のチェックは冗長です。コードを簡潔にするために削除を検討してください。 References
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 対応しました。 |
||
| ? "completed" | ||
| : phaseFromSessionRow(state.phase, "interrupted"); | ||
| } | ||
|
|
||
| return projection; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.