webui: handle SSE error events and avoid false completion#1277
webui: handle SSE error events and avoid false completion#1277tomerh2001 wants to merge 1 commit intoAudionut:masterfrom
Conversation
|
Thanks for taking the time to contribute to this project. Upload Assistant is currently in a complete rewrite, and no new development is being conducted on this python source at this time. If you have come this far, please feel free to leave open, any pull requests regarding new sites being added to the source, as these can serve as the baseline for later conversion. If your pull request relates to a critical bug, this will be addressed in this code base, and a new release published as needed. If your pull request only addresses a quite minor bug, it is not likely to be addressed in this code base. Details for the new code base will follow at a later date. |
📝 WalkthroughWalkthroughThe update enhances SSE event handling in the Web UI by introducing state tracking for output, terminal, and error events. It improves completion messaging logic to accurately reflect execution status—showing error, no-output, or success messages based on actual events received during execution, rather than always reporting success. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web_ui/static/js/app.js`:
- Around line 1105-1108: The current code always calls appendSystemMessage with
a success checkmark for all exits even though non-zero codes set sawErrorEvent;
change the logic around appendSystemMessage in the exit handler so it uses the
exit code to choose the proper message and semantics: when Number(data.code) ===
0 call appendSystemMessage with the success message (✓ Process exited...),
otherwise call appendSystemMessage (or a dedicated error logger used elsewhere)
with an error-formatted message (e.g., ✗ or "Process exited with non-zero code")
and set sawErrorEvent = true; update references to appendSystemMessage,
sawErrorEvent and data.code to implement this branching so the UI no longer
shows a success checkmark on error exits.
| appendSystemMessage(`✓ Process exited with code ${data.code}`); | ||
| if (Number(data.code) !== 0) { | ||
| sawErrorEvent = true; | ||
| } |
There was a problem hiding this comment.
Use error semantics for non-zero exit line
At Line 1105, the UI always prints ✓ Process exited..., even when Line 1106-Line 1108 classifies the exit as an error. This creates conflicting status in the same run.
Suggested fix
} else if (data.type === 'exit') {
sawTerminalEvent = true;
+ const exitCode = Number(data.code);
+ const isErrorExit = !Number.isFinite(exitCode) || exitCode !== 0;
+ if (isErrorExit) {
+ sawErrorEvent = true;
+ }
if (!(localController && localController.signal.aborted)) {
appendSystemMessage('');
- appendSystemMessage(`✓ Process exited with code ${data.code}`);
- if (Number(data.code) !== 0) {
- sawErrorEvent = true;
- }
+ appendSystemMessage(
+ `${isErrorExit ? '✗' : '✓'} Process exited with code ${data.code}`,
+ isErrorExit ? 'error' : 'info'
+ );
}
}📝 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.
| appendSystemMessage(`✓ Process exited with code ${data.code}`); | |
| if (Number(data.code) !== 0) { | |
| sawErrorEvent = true; | |
| } | |
| } else if (data.type === 'exit') { | |
| sawTerminalEvent = true; | |
| const exitCode = Number(data.code); | |
| const isErrorExit = !Number.isFinite(exitCode) || exitCode !== 0; | |
| if (isErrorExit) { | |
| sawErrorEvent = true; | |
| } | |
| if (!(localController && localController.signal.aborted)) { | |
| appendSystemMessage(''); | |
| appendSystemMessage( | |
| `${isErrorExit ? '✗' : '✓'} Process exited with code ${data.code}`, | |
| isErrorExit ? 'error' : 'info' | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web_ui/static/js/app.js` around lines 1105 - 1108, The current code always
calls appendSystemMessage with a success checkmark for all exits even though
non-zero codes set sawErrorEvent; change the logic around appendSystemMessage in
the exit handler so it uses the exit code to choose the proper message and
semantics: when Number(data.code) === 0 call appendSystemMessage with the
success message (✓ Process exited...), otherwise call appendSystemMessage (or a
dedicated error logger used elsewhere) with an error-formatted message (e.g., ✗
or "Process exited with non-zero code") and set sawErrorEvent = true; update
references to appendSystemMessage, sawErrorEvent and data.code to implement this
branching so the UI no longer shows a success checkmark on error exits.
1 participant
Fixes #1276.
What changed
errorevents in Web UI execution stream and display them in terminal output.systemevents so server-side status messages are visible.✓ Execution completedon error/empty-output termination.exitcodes as error outcomes.Why
The backend can emit SSE
errorevents (for example lock contention: "Another in-process run is active").The frontend previously ignored these events and unconditionally printed
✓ Execution completedwhen the stream ended, causing false-success runs with no useful output.Validation
app.js(react/no-unescaped-entitieslines 1690, 2212).web_ui/static/js/app.js.Summary by CodeRabbit
Bug Fixes
Improvements