Fix/isValidMessage #5579
Fix/isValidMessage #5579
Conversation
|
@code-october is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request primarily enhance the message handling and session management functionality within the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
app/store/chat.ts
Outdated
| if (message.startsWith("```") && message.endsWith("```")) { | ||
| const jsonString = message.slice(3, -3).trim(); | ||
| try { | ||
| const jsonObject = JSON.parse(jsonString); | ||
| if (jsonObject.error) { | ||
| return false; | ||
| } | ||
| } catch (e) { | ||
| console.log("Invalid JSON format."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure message is a string before invoking string methods
In the isValidMessage function, string methods startsWith and endsWith are called on message without verifying that message is indeed a string. If message is not a string, this can lead to runtime errors.
To prevent potential errors, check if message is a string before invoking string methods.
Apply this diff to add a type check:
+ if (typeof message !== "string") {
+ return false;
+ }
if (message.startsWith("```") && message.endsWith("```")) {
const jsonString = message.slice(3, -3).trim();
try {
const jsonObject = JSON.parse(jsonString);
if (jsonObject.error) {
return false;
}
} catch (e) {
console.log("Invalid JSON format.");
+ return false;
}
}
- return typeof message === "string" && !message.startsWith("```json");
+ return !message.startsWith("```json");
app/store/chat.ts
Outdated
| const jsonString = message.slice(3, -3).trim(); | ||
| try { | ||
| const jsonObject = JSON.parse(jsonString); | ||
| if (jsonObject.error) { | ||
| return false; | ||
| } | ||
| } catch (e) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify JSON extraction and improve accuracy
The current implementation checks for messages enclosed in triple backticks and attempts to parse the content as JSON. However, it might be more precise to specifically check for messages starting with ```json to accurately identify JSON-formatted messages.
Consider adjusting the function to enhance accuracy:
- if (message.startsWith("```") && message.endsWith("```")) {
- const jsonString = message.slice(3, -3).trim();
+ if (typeof message === "string" && message.startsWith("```json") && message.endsWith("```")) {
+ const jsonString = message.slice(7, -3).trim();
try {
const jsonObject = JSON.parse(jsonString);
if (jsonObject.error) {
return false;
}
} catch (e) {
console.log("Invalid JSON format.");
return false;
}
}
- return typeof message === "string" && !message.startsWith("```json");
+ return true;This refactor:
- Ensures the message is a string before processing.
- Checks specifically for JSON code blocks starting with ```json.
- Slices the content correctly to extract the JSON string.
- Returns
falseimmediately if JSON parsing fails. - Returns
trueat the end, assuming all invalid conditions have been checked.
app/store/chat.ts
Outdated
| console.log("Invalid JSON format."); | ||
| } |
There was a problem hiding this comment.
Return false when JSON parsing fails
In the catch block of isValidMessage, when JSON parsing fails, the function logs "Invalid JSON format." but does not return false. This means that messages with invalid JSON will not be correctly marked as invalid.
Consider returning false when JSON parsing fails to ensure invalid messages are properly handled.
Apply this diff to return false when JSON parsing fails:
} catch (e) {
console.log("Invalid JSON format.");
+ return false;
}📝 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.
| console.log("Invalid JSON format."); | |
| } | |
| console.log("Invalid JSON format."); | |
| return false; | |
| } |
| @@ -681,6 +681,7 @@ export const useChatStore = createPersistStore( | |||
| }, | |||
| onFinish(message) { | |||
| console.log("[Memory] ", message); | |||
| if (!isValidMessage(message)) return; | |||
There was a problem hiding this comment.
💡 Codebase verification
Potential unvalidated message processing detected in app/store/chat.ts at line 696.
Please ensure that isValidMessage is invoked consistently wherever messages are processed to maintain validation integrity.
🔗 Analysis chain
Verify that message validation is consistently applied
In the onFinish callback, the check if (!isValidMessage(message)) return; ensures that invalid messages are not processed. To maintain consistency and robustness, ensure that all instances where messages are processed include validation using isValidMessage.
Run the following script to check for other message handling without validation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find instances where messages are handled without validation.
# Expected: All message processing should involve validation via isValidMessage.
rg --type ts --no-heading --line-number --color never 'function\s+\w+\(.*message.*\)' app/store/ | while read -r line; do
file=$(echo "$line" | cut -d: -f1)
lineno=$(echo "$line" | cut -d: -f2)
function_def=$(echo "$line" | cut -d: -f3-)
function_body=$(awk "NR>$lineno && NR<($lineno+20)" "$file")
if ! echo "$function_body" | grep -q "isValidMessage"; then
echo "Potential unvalidated message in $file:$lineno - $function_def"
fi
done
Length of output: 2295
|
Your build has completed! |
|
下个版本会加上jest，到时可以更新主分支加一点测试用例方便review ~ |
Jest will be added to the next version. At that time, the main branch can be updated and some test cases can be added to facilitate review ~ |
|
可以更新一下代码，目前主分支已经加上了jest，可以补充一些测试用例~ |
You can update the code. Currently, jest has been added to the main branch. You can add some test cases~ |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/store/chat.ts (1)
704-714: Ensure consistent language in code commentsThe comments within the
isValidMessagefunction are in Chinese. For consistency across the codebase, consider translating the comments to English.Apply this diff to update the comments:
try { - // 返回 json 格式消息，含 error.message 字段，判定为错误回复，否则为正常回复 + // If the JSON object contains error.message, it's an error response; otherwise, it's a normal response const jsonObject = JSON.parse(jsonString); if (jsonObject?.error?.message) { return false; } return true; } catch (e) { console.log("Invalid JSON format."); - // 非 json 格式，大概率是正常回复 + // Non-JSON format, most likely a normal response return true; }
|
测试用例暂时还不太清楚怎么添加，目前就是想覆盖这类错误提示，消息类型是不带json语言注释的代码块，其中包含error.message 字段：
|
It’s not yet clear how to add test cases. Currently, I just want to cover this type of error prompt. The message type is a code block without json language annotation, which contains the error.message field:
|
|
可以将你这个函数导出 |
You can export your function |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/store/chat.ts (1)
726-726: Approve message validation in summarizeSessionThe addition of the
isValidMessagecheck in thesummarizeSessionfunction is a good improvement. It ensures that only valid messages are processed during summarization, which can prevent potential errors or unexpected behavior.Consider adding some error logging when an invalid message is encountered:
- if (!isValidMessage(message)) return; + if (!isValidMessage(message)) { + console.warn("Invalid message encountered during summarization:", message); + return; + }This will help with debugging and monitoring the quality of messages being processed.
app/store/chat.ts
Outdated
| if (typeof message !== "string") { | ||
| return false; | ||
| } | ||
| if (message.startsWith("```") && message.endsWith("```")) { | ||
| const codeBlockContent = message.slice(3, -3).trim(); | ||
| const jsonString = codeBlockContent.replace(/^json\s*/i, '').trim(); | ||
| try { | ||
| // 返回 json 格式消息，含 error.message 字段，判定为错误回复，否则为正常回复 | ||
| const jsonObject = JSON.parse(jsonString); | ||
| if (jsonObject?.error?.message) { | ||
| return false; | ||
| } | ||
| return true; | ||
| } catch (e) { | ||
| console.log("Invalid JSON format."); | ||
| // 非 json 格式，大概率是正常回复 | ||
| return true; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness of isValidMessage function
The isValidMessage function is a good addition to validate messages before processing. However, consider the following improvements:
- Check if the code block starts with "```json" before attempting to parse it as JSON.
- Handle different types of code blocks, not just JSON.
- Improve error handling to be more specific about what went wrong.
Here's a suggested refactor:
function isValidMessage(message: any): boolean {
if (typeof message !== "string") {
return false;
}
if (message.startsWith("```") && message.endsWith("```")) {
const codeBlockContent = message.slice(3, -3).trim();
- const jsonString = codeBlockContent.replace(/^json\s*/i, '').trim();
+ if (codeBlockContent.toLowerCase().startsWith('json')) {
+ const jsonString = codeBlockContent.replace(/^json\s*/i, '').trim();
+ try {
+ const jsonObject = JSON.parse(jsonString);
+ return !jsonObject?.error?.message;
+ } catch (e) {
+ console.log("Invalid JSON format:", e.message);
+ return false;
+ }
+ }
+ // Non-JSON code blocks are considered valid
+ return true;
}
- try {
- // 返回 json 格式消息，含 error.message 字段，判定为错误回复，否则为正常回复
- const jsonObject = JSON.parse(jsonString);
- if (jsonObject?.error?.message) {
- return false;
- }
- return true;
- } catch (e) {
- console.log("Invalid JSON format.");
- // 非 json 格式，大概率是正常回复
- return true;
- }
- }
return true;
}This refactor:
- Only attempts to parse JSON if the code block explicitly starts with "json".
- Treats non-JSON code blocks as valid messages.
- Improves error logging for invalid JSON.
- Simplifies the overall logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
test/isValidMessage.test.ts (3)
1-22: Good implementation, but consider some improvementsThe
isValidMessagefunction is well-structured and handles various scenarios effectively. However, there are a few areas for potential improvement:
- The input parameter type
anycould be more specific. Consider usingunknownfor better type safety.- The function returns
truefor non-JSON content, which might not always be desirable. Consider adding a parameter to control this behavior.- The error logging could be more informative. Consider including the actual error message.
Here's a suggested refactoring:
function isValidMessage(message: unknown, allowNonJson: boolean = true): boolean { if (typeof message !== "string") { return false; } if (message.startsWith("```") && message.endsWith("```")) { const codeBlockContent = message.slice(3, -3).trim(); const jsonString = codeBlockContent.replace(/^json\s*/i, '').trim(); try { const jsonObject = JSON.parse(jsonString); if (jsonObject?.error === true || jsonObject?.error?.message) { return false; } return true; } catch (e) { console.error(`Invalid JSON format: ${(e as Error).message}`); return allowNonJson; } } return allowNonJson; }This refactoring addresses the mentioned issues and provides more flexibility in handling non-JSON content.
24-96: Comprehensive test suite with room for enhancementThe test suite is well-structured and covers a good range of scenarios, which is commendable. To further improve it:
- Consider adding more edge cases, such as:
- Empty string input
- Malformed JSON
- Non-string inputs (e.g., numbers, objects)
- Make test descriptions more specific about what they're testing. For example, instead of "error msg no.1", use something like "should return false for JSON with error field set to true".
- Consider grouping related tests using
describeblocks for better organization.Here's an example of how you could structure a new test case:
test("should return false for malformed JSON", () => { const message = "```{\"error\": true,```"; expect(isValidMessage(message)).toBe(false); });Adding such cases would increase the robustness of your test suite.
1-96: Overall, good implementation with room for refinementThe
isValidMessagefunction and its accompanying test suite form a solid foundation for message validation in your project. The function effectively handles various scenarios, particularly focusing on identifying error responses in JSON format. The test suite provides good coverage of different cases.To further enhance this implementation:
- Consider the suggested refactoring of the
isValidMessagefunction to improve type safety and flexibility.- Expand the test suite to cover more edge cases and improve test descriptions.
- Group related tests for better organization.
These improvements will contribute to making your code more robust and maintainable.
As you continue to develop this feature, consider how it integrates with the larger error handling strategy of your application. You might want to create a more comprehensive error handling module that includes this function as part of a broader set of utilities for managing and responding to various types of errors consistently across your application.
app/store/chat.ts (2)
726-728: Approve changes with a minor suggestionThe addition of the
isValidMessagecheck is a good practice to ensure that only valid messages are processed. This improves the robustness of the code by filtering out potentially problematic messages.Consider moving the
isValidMessagefunction definition before its usage to improve code readability and maintainability. This would make the code easier to follow and understand.
Line range hint
726-758: Overall improvement in message validation with room for refinementThe changes introduced in this PR enhance the robustness of the chat system by adding message validation checks. The new
isValidMessagefunction and its integration into thesummarizeSessionfunction are positive additions that help filter out potentially problematic messages.However, there are opportunities for further improvement:
- The placement of the
isValidMessagefunction could be optimized for better code organization.- The error handling and validation logic in
isValidMessagecould be more comprehensive and specific.These refinements would further enhance the maintainability and reliability of the code.
Consider creating a separate module for message validation functions if similar checks are needed elsewhere in the codebase. This would improve code organization and reusability.
app/store/chat.ts
Outdated
| if (typeof message !== "string") { | ||
| return false; | ||
| } | ||
| if (message.startsWith("```") && message.endsWith("```")) { | ||
| const codeBlockContent = message.slice(3, -3).trim(); | ||
| const jsonString = codeBlockContent.replace(/^json\s*/i, '').trim(); | ||
| try { | ||
| // 返回 json 格式消息，error 字段为 true 或者包含 error.message 字段，判定为错误回复，否则为正常回复 | ||
| const jsonObject = JSON.parse(jsonString); | ||
| if (jsonObject?.error == true || jsonObject?.error?.message) { | ||
| return false; | ||
| } | ||
| return true; | ||
| } catch (e) { | ||
| console.log("Invalid JSON format."); | ||
| // 非 json 格式，通常可认为是正常回复 | ||
| return true; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness of isValidMessage function
The isValidMessage function is a good addition to validate messages. However, consider the following improvements:
- Handle different types of code blocks, not just JSON.
- Improve error handling to be more specific about what went wrong.
- Consider additional validation for non-JSON messages.
Here's a suggested refactor:
function isValidMessage(message: any): boolean {
if (typeof message !== "string") {
return false;
}
if (message.startsWith("```") && message.endsWith("```")) {
const codeBlockContent = message.slice(3, -3).trim();
- const jsonString = codeBlockContent.replace(/^json\s*/i, '').trim();
+ if (codeBlockContent.toLowerCase().startsWith('json')) {
+ const jsonString = codeBlockContent.replace(/^json\s*/i, '').trim();
+ try {
+ const jsonObject = JSON.parse(jsonString);
+ return !jsonObject?.error?.message;
+ } catch (e) {
+ console.log("Invalid JSON format:", e.message);
+ return false;
+ }
+ }
+ // Non-JSON code blocks are considered valid
+ return true;
}
- try {
- // 返回 json 格式消息，error 字段为 true 或者包含 error.message 字段，判定为错误回复，否则为正常回复
- const jsonObject = JSON.parse(jsonString);
- if (jsonObject?.error == true || jsonObject?.error?.message) {
- return false;
- }
- return true;
- } catch (e) {
- console.log("Invalid JSON format.");
- // 非 json 格式，通常可认为是正常回复
- return true;
- }
- }
return true;
}This refactor:
- Only attempts to parse JSON if the code block explicitly starts with "json".
- Treats non-JSON code blocks as valid messages.
- Improves error logging for invalid JSON.
- Simplifies the overall logic.
📝 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 (typeof message !== "string") { | |
| return false; | |
| } | |
| if (message.startsWith("```") && message.endsWith("```")) { | |
| const codeBlockContent = message.slice(3, -3).trim(); | |
| const jsonString = codeBlockContent.replace(/^json\s*/i, '').trim(); | |
| try { | |
| // 返回 json 格式消息，error 字段为 true 或者包含 error.message 字段，判定为错误回复，否则为正常回复 | |
| const jsonObject = JSON.parse(jsonString); | |
| if (jsonObject?.error == true || jsonObject?.error?.message) { | |
| return false; | |
| } | |
| return true; | |
| } catch (e) { | |
| console.log("Invalid JSON format."); | |
| // 非 json 格式，通常可认为是正常回复 | |
| return true; | |
| } | |
| } | |
| return true; | |
| function isValidMessage(message: any): boolean { | |
| if (typeof message !== "string") { | |
| return false; | |
| } | |
| if (message.startsWith("```") && message.endsWith("```")) { | |
| const codeBlockContent = message.slice(3, -3).trim(); | |
| if (codeBlockContent.toLowerCase().startsWith('json')) { | |
| const jsonString = codeBlockContent.replace(/^json\s*/i, '').trim(); | |
| try { | |
| const jsonObject = JSON.parse(jsonString); | |
| return !jsonObject?.error?.message; | |
| } catch (e) { | |
| console.log("Invalid JSON format:", e.message); | |
| return false; | |
| } | |
| } | |
| // Non-JSON code blocks are considered valid | |
| return true; | |
| } | |
| return true; | |
| } |
| @@ -739,19 +739,20 @@ export const useChatStore = createPersistStore( | |||
| if (typeof message !== "string") { | |||
| return false; | |||
| } | |||
| message = message.trim(); | |||
There was a problem hiding this comment.
应该在此class外将isValidMessage 导出
然后在对应的test 文件引入，而不是复制一份
| @@ -0,0 +1,161 @@ | |||
| function isValidMessage(message: any): boolean { | |||
There was a problem hiding this comment.
import { isValidMessage } from @/app/store/chat
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
📝 补充信息 | Additional Information
Summary by CodeRabbit
New Features
Bug Fixes
Chores