Fix: clear context to avoid [belongerr] when Spark return [error] #638
Conversation
|
Thank you. I have a concern. Clearing the context, or reporting an error and requiring creating a new chat. Which solution is better? I have no idea now. |
|
I agree with you that finding a perfect solution is tricky, here is what I think:
|
|
Inspired by your reply. I have the answer now. Any error should be exposed to users as is. Only the user can determine what to do next. There has been code that does not follow this rule and I will modify them. |
There was a problem hiding this comment.
Hi @samanhappy, I hope you're doing great! Are you still interested in continuing with this PR? It looks like there are a few areas that might need some attention. Let me know what you think!
|
Hi @PeterDaveHello, it's been a long time since the issue was created. I'm still interested but unsure about the next steps. Any guidance? |
|
Hey @samanhappy, Thanks for getting back to us! Since the consensus was to expose the error to the users and let them decide how to proceed, we could adjust your PR to reflect that. Instead of automatically clearing the context when Spark returns an error, could we modify it so that the error message is displayed to the user? That way, they can choose whether to clear the context or start a new chat themselves. Let me know what you think. |
|
That's fine. I'll adjust the relevant code to implement it and let you know when it's done. |
WalkthroughThe Changes
Possibly related PRs
Suggested reviewers
Poem
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/bots/SparkBot.js (1)
Line range hint
95-118: Ensure promises are properly resolved or rejected upon errorWhen an error occurs (
event.data === "[error]"), the current implementation sets anerrorflag but does not resolve or reject the promise immediately. If the<end>event is not received after an error, the promise may remain pending indefinitely. To prevent this, consider resolving or rejecting the promise as soon as the error is detected.For example, you can reject the promise when the error is encountered:
} else if (event.data === "[error]") { - error = true; + source.close(); + reject(new Error("An error occurred during message processing.")); } else if (event.data.slice(-5) === "<sid>") {
| } else if (event.data === "[error]") { | ||
| error = true; |
There was a problem hiding this comment.
Display error message to the user instead of automatically clearing the context
The current implementation automatically clears the chat context when an error occurs by setting error = true and later invoking this.setChatContext(null). According to the PR discussion, the preferred approach is to display the error message to the user, allowing them to decide whether to clear the context or start a new chat. Please modify the code to present the error to the user instead of clearing the context automatically.
Would you like assistance in updating the implementation to display the error message to the user?
| // clear context if error | ||
| if (error) { | ||
| this.setChatContext(null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid clearing context inside the event listener for each message
Placing this.setChatContext(null); inside the message event listener means the context could be cleared multiple times during message processing, especially if multiple errors occur. This might not be the intended behavior and could lead to inconsistent state management. Consider handling context clearance outside the event listener or ensuring it occurs only once after all events are processed.
You might move the context clearing logic to where the source is closed or when the promise is resolved or rejected due to an error.
| let error = false; | ||
| source.addEventListener("message", (event) => { | ||
| if (event.data === "<end>") { | ||
| onUpdateResponse(callbackParam, { done: true }); | ||
| source.close(); | ||
| resolve(); | ||
| } else if (event.data === "[error]") { | ||
| error = true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify error handling by removing the unnecessary error flag
Introducing an error flag adds extra state management that may not be necessary. Since you can detect the error condition directly when event.data === "[error]", you can handle the error immediately without setting a separate flag. This simplifies the code and reduces the potential for bugs related to state handling.
Consider modifying the code as follows:
let text = "";
-let error = false;
source.addEventListener("message", (event) => {
if (event.data === "<end>") {
onUpdateResponse(callbackParam, { done: true });
source.close();
resolve();
- } else if (event.data === "[error]") {
- error = true;
+ } else if (event.data === "[error]") {
+ // Handle the error immediately
+ this.setChatContext(null); // Or display error to user
+ source.close();
+ reject(new Error("An error occurred during message processing."));
} else if (event.data.slice(-5) === "<sid>") {
// ignore <sid> message
return;Committable suggestion was skipped due to low confidence.
|
@PeterDaveHello |
|
If the problem just gone away, then yes, maybe we can spend the time and effort on some other issues ;) |
|
Unable to reproduce |
This PR aims to address the [belongerr] issue of SparkBot in #633
The reason to clear context is Spark will make the current conversation unusable for sensitive message
so I choose to clear context whenever encoutering this situation
Summary by CodeRabbit