add 3 type of reasoning_content support (+deepseek-r1@OpenAI @Alibaba @ByteDance), parse <think></think> from SSE #6204
Conversation
|
@bestsanmao is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis update refines the chat functionality across multiple platform modules by enhancing the chat logic in their respective APIs. Key modifications include the addition of imports such as Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
bestsanmao
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
app/client/platforms/bytedance.ts (1)
179-208: Selective usage of reasoning vs. content.
Current logic prioritizesreasoning_contentwhenever present, ignoringcontentif both exist. If partial merging is desired, handle both. Otherwise, this looks fine for an either/or approach.app/client/platforms/alibaba.ts (2)
175-192: Tool calls merging logic.
Appending or updating arguments based on ID or index is sound. Just verify that partial arguments are re-merged safely if subsequent calls modify them further.
193-223: Reasoning vs. content usage.
Same approach as bytedance.ts, returningreasoningif present. Carefully verify user expectations about partial or combined content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/platforms/alibaba.ts(5 hunks)app/client/platforms/bytedance.ts(4 hunks)
🔇 Additional comments (11)
app/client/platforms/bytedance.ts (5)
8-14: Imports look consistent.
These newly introduced store imports appear valid and are referenced within the file. No immediate concerns.
25-25: New utility import recognized.
ThestreamWithThinkimport is used to manage streaming responses. Implementation references look correct.
28-28: Import usage check.
getMessageTextContentWithoutThinkingis properly used to handle message content for the assistant role.
92-95: Role-based content handling.
Switching togetMessageTextContentWithoutThinkingfor assistant messages, and awaitingpreProcessImageContentfor other roles, is logically sound. However, ensure both functions handle all edge cases (e.g., empty content).
137-142: Plugin store usage verification.
Retrieving tools and functions viausePluginStorefor streaming is a good approach. Confirm thatgetAsToolsreturns objects compatible with the subsequent logic.app/client/platforms/alibaba.ts (6)
8-14: Imports approved.
The additional store imports are consistent with the new streaming logic and plugin features.
15-15: Stream utility import.
streamWithThinkis referenced correctly and matches the new streaming implementation.
25-28: Added message content utilities.
getMessageTextContentandgetMessageTextContentWithoutThinkingare now imported, which aligns with the refined role-based message handling below.
98-101: Role-based content extraction.
UsinggetMessageTextContentWithoutThinkingfor the assistant role prevents the “thinking” portion from appearing in final responses. This is a neat approach. Ensure it meets UX requirements.
131-134: SSE header management.
Conditionally adding"X-DashScope-SSE": "enable"is a clean way to toggle streaming. Just verify servers handle/expect this header properly.
151-156: Integration with plugin store.
usePluginStore.getAsTools(...)usage is consistent with the approach in bytedance.ts. No immediate issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/client/platforms/alibaba.ts (1)
177-194: 🛠️ Refactor suggestionMaintain consistency with ByteDance implementation.
Apply the same type safety improvements as suggested for the ByteDance implementation.
- const index = tool_calls[0]?.index; - const id = tool_calls[0]?.id; - const args = tool_calls[0]?.function?.arguments; + const toolCall = tool_calls[0]; + if (!toolCall) return; + const { index, id, function: func } = toolCall; + const args = func?.arguments; if (id) {
🧹 Nitpick comments (1)
app/client/platforms/bytedance.ts (1)
136-142: Consider adding type safety for plugin tools.The
as anytype assertion for tools could mask type errors.- tools as any, + tools as ChatMessageTool[],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/platforms/alibaba.ts(5 hunks)app/client/platforms/bytedance.ts(6 hunks)
🔇 Additional comments (6)
app/client/platforms/bytedance.ts (3)
8-14: LGTM! Interface and imports are well-structured.The renamed interface and new imports provide better type safety and plugin support.
Also applies to: 25-25, 40-52
92-95: LGTM! Improved message content processing.The conditional processing correctly handles assistant messages by removing thinking content.
214-225: 🛠️ Refactor suggestionRemove @ts-ignore by adding proper type safety.
The type issues should be resolved rather than ignored.
- // @ts-ignore - runTools[index]["function"]["arguments"] += args; + if (index !== undefined && index < runTools.length) { + const tool = runTools[index]; + if (tool?.function) { + tool.function.arguments = (tool.function.arguments || '') + (args || ''); + } + }Likely an incorrect or invalid review comment.
app/client/platforms/alibaba.ts (3)
8-15: LGTM! Well-structured imports and streaming setup.The SSE header is correctly configured based on the streaming flag.
Also applies to: 131-134
98-101: LGTM! Clear role-based content processing.The conditional processing correctly differentiates between assistant and other roles.
200-226: LGTM! Consistent error handling.The implementation correctly handles empty content and reasoning states.
| if (tool_calls?.length > 0) { | ||
| const index = tool_calls[0]?.index; | ||
| const id = tool_calls[0]?.id; | ||
| const args = tool_calls[0]?.function?.arguments; | ||
| if (id) { | ||
| runTools.push({ | ||
| id, | ||
| type: tool_calls[0]?.type, | ||
| function: { | ||
| name: tool_calls[0]?.function?.name as string, | ||
| arguments: args, | ||
| }, | ||
| }); | ||
| } else { | ||
| // @ts-ignore | ||
| runTools[index]["function"]["arguments"] += args; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety checks for tool calls processing.
The code directly accesses array indices and properties without proper validation.
- const index = tool_calls[0]?.index;
- const id = tool_calls[0]?.id;
- const args = tool_calls[0]?.function?.arguments;
+ const toolCall = tool_calls[0];
+ if (!toolCall) return;
+ const { index, id, function: func } = toolCall;
+ const args = func?.arguments;
if (id) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (tool_calls?.length > 0) { | |
| const index = tool_calls[0]?.index; | |
| const id = tool_calls[0]?.id; | |
| const args = tool_calls[0]?.function?.arguments; | |
| if (id) { | |
| runTools.push({ | |
| id, | |
| type: tool_calls[0]?.type, | |
| function: { | |
| name: tool_calls[0]?.function?.name as string, | |
| arguments: args, | |
| }, | |
| }); | |
| } else { | |
| // @ts-ignore | |
| runTools[index]["function"]["arguments"] += args; | |
| } | |
| } | |
| if (tool_calls?.length > 0) { | |
| const toolCall = tool_calls[0]; | |
| if (!toolCall) return; | |
| const { index, id, function: func } = toolCall; | |
| const args = func?.arguments; | |
| if (id) { | |
| runTools.push({ | |
| id, | |
| type: tool_calls[0]?.type, | |
| function: { | |
| name: tool_calls[0]?.function?.name as string, | |
| arguments: args, | |
| }, | |
| }); | |
| } else { | |
| // @ts-ignore | |
| runTools[index]["function"]["arguments"] += args; | |
| } | |
| } |
bestsanmao
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/utils/chat.ts (2)
584-599: Consider extracting tag processing logic into a separate function.The tag processing logic is complex and could benefit from being extracted into a dedicated function for better maintainability and testability.
Here's a suggested refactor:
+ function processThinkingTags(chunk: { isThinking: boolean, content: string }, lastIsThinkingTagged: boolean) { + if (!chunk.isThinking) { + if (chunk.content.startsWith("<think>")) { + chunk.isThinking = true; + chunk.content = chunk.content.slice(7).trim(); + lastIsThinkingTagged = true; + } else if (chunk.content.endsWith("</think>")) { + chunk.isThinking = false; + chunk.content = chunk.content.slice(0, -8).trim(); + lastIsThinkingTagged = false; + } else if (lastIsThinkingTagged) { + chunk.isThinking = true; + } + } + return { chunk, lastIsThinkingTagged }; + } - // deal with <think> and </think> tags start - if (!chunk.isThinking) { - if (chunk.content.startsWith("<think>")) { - chunk.isThinking = true; - chunk.content = chunk.content.slice(7).trim(); - lastIsThinkingTagged = true; - } else if (chunk.content.endsWith("</think>")) { - chunk.isThinking = false; - chunk.content = chunk.content.slice(0, -8).trim(); - lastIsThinkingTagged = false; - } else if (lastIsThinkingTagged) { - chunk.isThinking = true; - } - } - // deal with <think> and </think> tags start + const { chunk: processedChunk, lastIsThinkingTagged: newLastIsThinkingTagged } = + processThinkingTags(chunk, lastIsThinkingTagged); + chunk = processedChunk; + lastIsThinkingTagged = newLastIsThinkingTagged;
600-631: Consider adding unit tests for thinking mode state transitions.The thinking mode state management logic is complex with multiple state transitions. Adding comprehensive unit tests would help ensure reliability.
Would you like me to help generate unit tests for the thinking mode state transitions? The tests would cover:
- Transition from normal to thinking mode
- Transition from thinking to normal mode
- Handling of newlines in thinking content
- Edge cases with empty content
app/client/platforms/openai.ts (2)
297-303: Consider adding resilience to SSE parsing.
Within thestreamWithThinkinvocation,JSON.parse(text)could throw if upstream JSON is malformed or incomplete. Adding a try/catch mechanism, or validating the text before parsing, may help avoid runtime errors. Also, the casttools as anycould be replaced with more precise TypeScript types for stronger safety.Proposed snippet for safer parsing (inside the callback):
// parseSSE (text: string, runTools: ChatMessageTool[]) => { - const json = JSON.parse(text); + let json; + try { + json = JSON.parse(text); + } catch (err) { + console.error("Invalid JSON from SSE:", text, err); + return { isThinking: false, content: "" }; + } const choices = json.choices as Array<{ delta: { content: string; tool_calls: ChatMessageTool[]; reasoning_content: string | null; }; }>;
352-367: Reduce duplicated code in conditional returns.
The code separately handlesreasoningandcontent, returning nearly identical data structures. You can streamline it to make the logic more concise and maintainable.Example refactor:
if ((reasoning?.trim().length ?? 0) > 0) { return { isThinking: true, content: reasoning, }; } - else if (content && content.trim().length > 0) { + if ((content?.trim().length ?? 0) > 0) { return { isThinking: false, content: content, }; } return { isThinking: false, content: "", };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/platforms/openai.ts(4 hunks)app/utils/chat.ts(2 hunks)
🔇 Additional comments (5)
app/utils/chat.ts (2)
403-403: LGTM: Clear variable naming.The
lastIsThinkingTaggedboolean flag clearly indicates its purpose of tracking the state between<think>and</think>tags.
632-635: LGTM: Improved error handling.Good improvement in error handling by logging parse errors without throwing them, preventing stream interruption due to malformed messages.
app/client/platforms/openai.ts (3)
25-25: New import usage looks good.
The addition ofstreamWithThinkis aligned with the rest of the changes and appears correctly imported from@/app/utils/chat.
312-316: Validate newreasoning_contentproperty.
The new fieldreasoning_contentis typed asstring | null. Verify the backend consistently returns a valid string ornullto avoid undefined behavior if the server response deviates from this format. The short-circuit logic forif (!choices?.length)is a solid safeguard.
339-350: Skipping empty SSE chunks is a good optimization.
By discarding events that have neitherreasoning_contentnorcontent, you avoid sending extraneous updates to the user. This is a clean approach to filtering out irrelevant or empty SSE chunks.
|
Could you support reasoning_content of Deepseek/deepseek-r1@OpenRouter |
我试了一下openrouter的api |
我测试了一下 |
I've tested it |
|
Maybe non-streaming could be accepted. I also found the OpenRouter official chat room supports stream reasoning response. |
|
这个修改会导致输出的markdown格式少了空格 |
This modification will cause the output markdown format to be missing spaces |
确实会吞空格 |
Will indeed swallow spaces |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
app/client/platforms/deepseek.ts (1)
144-162: 🛠️ Refactor suggestionAdd safety checks for tool calls processing.
The code directly accesses array indices and properties without proper validation.
-const tool_calls = choices[0]?.delta?.tool_calls; -if (tool_calls?.length > 0) { - const index = tool_calls[0]?.index; - const id = tool_calls[0]?.id; - const args = tool_calls[0]?.function?.arguments; +const tool_calls = choices[0]?.delta?.tool_calls; +if (tool_calls?.length > 0) { + const toolCall = tool_calls[0]; + if (!toolCall) return; + const { index, id, function: func } = toolCall; + const args = func?.arguments;app/client/platforms/glm.ts (1)
257-265: 🛠️ Refactor suggestionRemove type assertions and improve type safety.
The code uses
@ts-ignoreand unsafe array operations.- // @ts-ignore - requestPayload?.messages?.splice( - // @ts-ignore - requestPayload?.messages?.length, - 0, - toolCallMessage, - ...toolCallResult, - ); + if ('messages' in requestPayload && Array.isArray(requestPayload.messages)) { + requestPayload.messages.splice( + requestPayload.messages.length, + 0, + toolCallMessage, + ...toolCallResult, + ); + }
♻️ Duplicate comments (1)
app/client/platforms/bytedance.ts (1)
162-179: 🛠️ Refactor suggestionAdd safety checks for tool calls processing.
The code directly accesses array indices and properties without proper validation.
-const index = tool_calls[0]?.index; -const id = tool_calls[0]?.id; -const args = tool_calls[0]?.function?.arguments; +const toolCall = tool_calls[0]; +if (!toolCall) return; +const { index, id, function: func } = toolCall; +const args = func?.arguments;
🧹 Nitpick comments (4)
app/client/platforms/alibaba.ts (1)
94-97: Improve content extraction logic.The content extraction logic could be simplified using a ternary operator.
- content: - v.role === "assistant" - ? getMessageTextContentWithoutThinking(v) - : getMessageTextContent(v), + content: v.role === "assistant" ? getMessageTextContentWithoutThinking(v) : getMessageTextContent(v),app/client/platforms/baidu.ts (1)
241-249: Improve error handling in stream processing.The error handling in the stream processing could be more informative.
try { const json = JSON.parse(text); const delta = json?.result; if (delta) { remainText += delta; } } catch (e) { - console.error("[Request] parse error", text, msg); + console.error("[Request] Failed to parse stream message:", { + error: e, + text, + message: msg, + }); }app/utils.ts (1)
299-311: Consider improving model name matching logic.While the function works correctly, there are a few improvements that could make it more maintainable:
Consider applying these changes:
export function getTimeoutMSByModel(model: string) { model = model.toLowerCase(); + const thinkingModels = [ + 'dall-e', + 'dalle', + 'o1', + 'o3', + 'deepseek-r', + '-thinking' + ]; - if ( - model.startsWith("dall-e") || - model.startsWith("dalle") || - model.startsWith("o1") || - model.startsWith("o3") || - model.includes("deepseek-r") || - model.includes("-thinking") - ) + if (thinkingModels.some(prefix => + prefix.endsWith('-') ? model.startsWith(prefix) : model.includes(prefix) + )) return REQUEST_TIMEOUT_MS_FOR_THINKING; return REQUEST_TIMEOUT_MS; }Benefits:
- Easier to maintain the list of models that require longer timeouts
- Cleaner logic for prefix vs substring matching
- More efficient string operations
app/client/platforms/openai.ts (1)
297-368: Enhance error handling in streaming response parsing.The streaming response parsing logic has been improved to handle reasoning content, but there are a few areas that could be enhanced:
- The error handling for malformed JSON responses could be more robust
- The type definitions for the response structure could be more explicit
Consider applying these changes:
+interface StreamChoice { + delta: { + content?: string; + tool_calls?: ChatMessageTool[]; + reasoning_content?: string | null; + }; +} + +interface StreamResponse { + isThinking: boolean; + content: string; +} -const json = JSON.parse(text); +let json; +try { + json = JSON.parse(text); +} catch (e) { + console.error('Failed to parse SSE message:', e); + return { isThinking: false, content: '' }; +} -const choices = json.choices as Array<{ - delta: { - content: string; - tool_calls: ChatMessageTool[]; - reasoning_content: string | null; - }; -}>; +const choices = json.choices as StreamChoice[];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/client/platforms/alibaba.ts(4 hunks)app/client/platforms/baidu.ts(3 hunks)app/client/platforms/bytedance.ts(6 hunks)app/client/platforms/deepseek.ts(4 hunks)app/client/platforms/glm.ts(3 hunks)app/client/platforms/google.ts(3 hunks)app/client/platforms/openai.ts(6 hunks)app/client/platforms/siliconflow.ts(3 hunks)app/client/platforms/tencent.ts(3 hunks)app/client/platforms/xai.ts(3 hunks)app/utils.ts(2 hunks)
🔇 Additional comments (10)
app/client/platforms/xai.ts (1)
105-108: LGTM! Improved timeout handling.The change to use
getTimeoutMSByModelfor dynamic timeout configuration is a good improvement that aligns with the standardized approach across the codebase.app/client/platforms/siliconflow.ts (1)
119-123: LGTM! Improved timeout handling and streaming support.The changes to use
getTimeoutMSByModelandstreamWithThinkare well-implemented and align with the standardized approach across the codebase.app/client/platforms/bytedance.ts (2)
38-50: LGTM! Improved type safety with ByteDance-specific interface.The renaming of
RequestPayloadtoRequestPayloadForByteDanceimproves type safety by making the interface specific to ByteDance's requirements.
159-159: LGTM! Added safety check for choices array.The addition of the length check for the choices array prevents potential runtime errors.
app/client/platforms/tencent.ts (1)
140-143: LGTM: Dynamic timeout based on model.The change to use
getTimeoutMSByModelinstead of a static timeout is a good improvement as it allows for model-specific timeout values.app/client/platforms/baidu.ts (1)
151-154: LGTM: Model-specific timeout implementation.The implementation of model-specific timeout is consistent with other platform implementations.
app/client/platforms/glm.ts (1)
191-194: LGTM: Consistent timeout handling.The timeout implementation is consistent with other platform implementations.
app/client/platforms/google.ts (1)
25-25: LGTM! Dynamic timeout handling is a good improvement.The change from static timeout constants to dynamic timeout calculation based on the model type is a good improvement. This allows for more flexible timeout handling, especially for models that require longer processing times.
Also applies to: 204-205
app/client/platforms/openai.ts (2)
24-24: LGTM! Import changes align with new functionality.The imports have been updated to support the new reasoning content feature and dynamic timeout handling.
Also applies to: 44-44
397-399: LGTM! Dynamic timeout handling is consistent.The change to use
getTimeoutMSByModelfor timeout calculation is consistent with the changes in other files.
| }; | ||
| }>; | ||
|
|
||
| if (!choices?.length) return { isThinking: false, content: "" }; |
There was a problem hiding this comment.
Verify array access safety.
The code accesses array elements without checking if the array exists or has elements.
- if (!choices?.length) return { isThinking: false, content: "" };
-
- const tool_calls = choices[0]?.message?.tool_calls;
+ if (!choices?.length || !choices[0]?.message) {
+ return { isThinking: false, content: "" };
+ }
+ const tool_calls = choices[0].message.tool_calls;Also applies to: 173-190
| requestPayload?.input?.messages?.splice( | ||
| requestPayload?.input?.messages?.length, | ||
| 0, | ||
| toolCallMessage, | ||
| ...toolCallResult, | ||
| ); |
There was a problem hiding this comment.
Improve array manipulation safety.
The splice operation could fail if the messages array is undefined.
- requestPayload?.input?.messages?.splice(
- requestPayload?.input?.messages?.length,
- 0,
- toolCallMessage,
- ...toolCallResult,
- );
+ if (requestPayload?.input?.messages) {
+ requestPayload.input.messages.splice(
+ requestPayload.input.messages.length,
+ 0,
+ toolCallMessage,
+ ...toolCallResult,
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| requestPayload?.input?.messages?.splice( | |
| requestPayload?.input?.messages?.length, | |
| 0, | |
| toolCallMessage, | |
| ...toolCallResult, | |
| ); | |
| if (requestPayload?.input?.messages) { | |
| requestPayload.input.messages.splice( | |
| requestPayload.input.messages.length, | |
| 0, | |
| toolCallMessage, | |
| ...toolCallResult, | |
| ); | |
| } |
| // @ts-ignore | ||
| runTools[index]["function"]["arguments"] += args; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove type assertion and improve type safety.
The code uses @ts-ignore to bypass type checking, which could lead to runtime errors.
- // @ts-ignore
- runTools[index]["function"]["arguments"] += args;
+ if (index !== undefined && index < runTools.length && runTools[index]?.function) {
+ runTools[index].function.arguments += args;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // @ts-ignore | |
| runTools[index]["function"]["arguments"] += args; | |
| } | |
| if (index !== undefined && index < runTools.length && runTools[index]?.function) { | |
| runTools[index].function.arguments += args; | |
| } |
…-Web into ali_bytedance_reasoning_content
您现在再pull一下试试 |
|
感谢～ |
Thanks ~ |
支持阿里和字节跳动的deepseek-r1的思考过程
+deepseek-r1@Alibaba
+deepseek-r1@ByteDance
+deepseek-r1@OpenAI
模式的添加
并非指定阿里或字节的地址到DEEPSEEK_BASE_URL
这样不耽误同时使用deepseek官方的模型
增加支持兼容流式
<think></think>标签的思考内容鉴于各个provider都有共享的model（比如r1）重写超时逻辑
solved #6183 #6200 #6212
Summary by CodeRabbit
Summary by CodeRabbit