fix: dashboard submits to wrong endpoint and doesn't handle completion events#175
fix: dashboard submits to wrong endpoint and doesn't handle completion events#175josephfung wants to merge 2 commits into
Conversation
…n events Two bugs in the dashboard frontend: 1. Chat messages were sent to POST /api/steer (mid-flight steering for running tasks) instead of POST /api/task (submits to orchestrator). Since no task was running with the default jobId, messages went to a file on disk that nothing read. 2. The SSE handler only cleared "Working..." on job_update events, which the backend never sends. Added handlers for done, task.end, and job_failed — the event types the orchestrator actually emits. Also suppress task.start, turn.start, turn.end lifecycle events from creating noise in the chat as system messages.
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
📝 WalkthroughWalkthroughFixes dashboard chat submission and SSE handling: client now posts prompts to Changes
Sequence Diagram(s)(none — changes are bug fixes to existing event handling and do not introduce a new multi-component feature flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the task submission endpoint from /api/steer to /api/task and expands the event handling logic to include specific handlers for done, task.end, and job_failed events. A review comment identifies an issue with side effects (incrementing a counter and creating a new date) inside a React state updater function, which should be kept pure to ensure predictable behavior.
| setMessages(prev => { | ||
| const last = prev[prev.length - 1]; | ||
| if (last?.type === 'agent') return prev; // already shown via text streaming | ||
| return [...prev, { id: ++messageIdCounter, type: 'agent' as const, content: text, timestamp: new Date() }].slice(-MAX_MESSAGES); | ||
| }); |
There was a problem hiding this comment.
React state updaters should be pure functions. Performing side effects like incrementing a global counter ('++messageIdCounter') or creating a 'new Date()' inside the 'setMessages' updater can lead to unpredictable behavior, especially in React's Strict Mode or with concurrent rendering where the updater might be called multiple times. It is better to generate these values before calling the state setter.
const nextId = ++messageIdCounter;
const now = new Date();
setMessages(prev => {
const last = prev[prev.length - 1];
if (last?.type === 'agent') return prev; // already shown via text streaming
return [...prev, { id: nextId, type: 'agent' as const, content: text, timestamp: now }].slice(-MAX_MESSAGES);
});
There was a problem hiding this comment.
Good catch — fixed in b02cb86. Hoisted nextId and now out of the updater so the setMessages callback is a pure function. Thanks!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/dashboard/frontend/src/App.tsx (1)
379-406: Consider de-duplicating the event dispatch via a small map/switch.The if/else-if ladder is now 8 branches deep and mixes "return early" vs. "fall through to generic push" control flow, which is easy to break when adding new event types. A lookup table or
switchwould make the control flow more explicit and keep early-return handlers (done,task.end,task.start,turn.*,text*) visually separate from fallthrough ones (job_failed,job_update,tool_call, etc.).Non-blocking — current logic is correct for the documented event set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dashboard/frontend/src/App.tsx` around lines 379 - 406, Refactor the deep if/else-if ladder that inspects data.type in App.tsx into a clearer dispatch: replace the current chain (handling data.type values like 'done', 'task.end', 'job_failed', 'task.start', 'turn.start'/'turn.end', 'job_update', etc.) with a switch or a lookup map of handlers keyed by data.type; keep early-return lifecycle handlers (the 'done' branch that uses setMessages, messageIdCounter and setTaskRunning, the 'task.end' and 'task.start' and 'turn.start'/'turn.end' branches) as explicit handlers that return immediately, and move fall-through/visible-message cases (e.g. 'job_failed' which sets msgType/content and calls setTaskRunning, plus 'job_update'/'tool_call' style cases) into dedicated handler functions so control flow is explicit and easier to extend while reusing setMessages, setTaskRunning, messageIdCounter and MAX_MESSAGES.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/dashboard/frontend/src/App.tsx`:
- Around line 379-406: Refactor the deep if/else-if ladder that inspects
data.type in App.tsx into a clearer dispatch: replace the current chain
(handling data.type values like 'done', 'task.end', 'job_failed', 'task.start',
'turn.start'/'turn.end', 'job_update', etc.) with a switch or a lookup map of
handlers keyed by data.type; keep early-return lifecycle handlers (the 'done'
branch that uses setMessages, messageIdCounter and setTaskRunning, the
'task.end' and 'task.start' and 'turn.start'/'turn.end' branches) as explicit
handlers that return immediately, and move fall-through/visible-message cases
(e.g. 'job_failed' which sets msgType/content and calls setTaskRunning, plus
'job_update'/'tool_call' style cases) into dedicated handler functions so
control flow is explicit and easier to extend while reusing setMessages,
setTaskRunning, messageIdCounter and MAX_MESSAGES.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07f4a8f6-be8d-4cf7-9fd4-a38388ca27e4
📒 Files selected for processing (1)
src/dashboard/frontend/src/App.tsx
|
Thanks @coderabbitai — all pre-merge checks passing, appreciated the walkthrough. Note the PR also includes a follow-up commit (b02cb86) addressing a React purity issue flagged by Gemini: |
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/dashboard/frontend/src/App.tsx (1)
396-399: Nit: comment framing is slightly off vs. actual event ordering.Per
src/providers/claude-provider.ts(context snippet 3),task.endis yielded beforedone, so in the normal happy pathtask.endactually fires first anddoneis the redundant one. Behavior is still correct (both cleartaskRunningidempotently), but consider rewording — e.g. "Lifecycle marker — also clears progress;donetypically follows with the final text".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dashboard/frontend/src/App.tsx` around lines 396 - 399, Update the comment on the 'task.end' event handling to reflect actual event order: note that 'task.end' is emitted before 'done' (so 'done' is usually redundant) and that this branch idempotently clears progress via setTaskRunning(false); locate the block handling data.type === 'task.end' in App.tsx and replace the comment with something like "Lifecycle marker — also clears progress; 'done' typically follows with the final text."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/dashboard/frontend/src/App.tsx`:
- Around line 396-399: Update the comment on the 'task.end' event handling to
reflect actual event order: note that 'task.end' is emitted before 'done' (so
'done' is usually redundant) and that this branch idempotently clears progress
via setTaskRunning(false); locate the block handling data.type === 'task.end' in
App.tsx and replace the comment with something like "Lifecycle marker — also
clears progress; 'done' typically follows with the final text."
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5359164e-f542-433f-80d4-e730ad1d3e6c
📒 Files selected for processing (1)
src/dashboard/frontend/src/App.tsx
|
Hi there, I'm going to close this PR for now, as it hasn't received a response or review from the maintainers. I completely understand that open source projects can get busy, and there’s no pressure at all. If there's ever interest in revisiting this, id be happy to reopen or resubmit. |
1 participant
Summary
Fixes #167.
Two bugs in the dashboard frontend that prevent task submission and completion from working end-to-end:
Wrong submission endpoint — chat messages were sent to
POST /api/steer(mid-flight steering for already-running tasks) instead ofPOST /api/task(submits a new task to the orchestrator). Since no task was running with the defaultjobId, messages were written to a file on disk that nothing read.SSE completion events never handled — the event handler only cleared the "Working..." state on
job_updateevents, which the backend never emits during normal task completion. Added handlers fordone,task.end, andjob_failed— the actual event types the orchestrator emits — so the spinner clears and responses appear correctly.Also suppresses
task.start,turn.start, andturn.endlifecycle events from creating noise as system messages in the chat.Changes
POST /api/steerwithPOST /api/taskfor new task submissiondone,task.end,job_failed,task.start,turn.start,turn.endTesting
job_failedshows an error message in the chatSummary by CodeRabbit
Refactor
Bug Fixes