fix: ensure time.completed is always set on finished assistant messages#113
fix: ensure time.completed is always set on finished assistant messages#113
Conversation
Root cause: some adapters (e.g. OpenCode) strip time.completed during multi-step agent loops and only restore it on session.idle. If that event is dropped (e.g. WebSocket reconnect), the field stays undefined, causing the frontend timer to tick indefinitely on finished messages. Two-layer fix: 1. engine-manager: set time.completed = Date.now() when sendMessage() returns an assistant message without it, then persist and emit. 2. SessionTurn: replace time.created fallback with a capturedEndTime signal that snapshots Date.now() once when the session is first observed as done without time.completed — gives approximately correct duration instead of ~0s. Co-authored-by: OverCart345 <82957545+OverCart345@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Fixes an issue where completed assistant turns could lack time.completed, causing the UI duration timer to keep ticking after the session is actually finished (notably across reconnects / missed idle events), and preventing persistence of finalized assistant messages.
Changes:
- Engine-side: backfill
time.completedon assistant messages returned fromsendMessage()when missing, then persist + emit an update. - Frontend: add a “captured end time” fallback so completed turns without
time.completedrender a static duration instead of ticking.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/components/SessionTurn.tsx | Adds a captured end-time fallback to stop the duration timer for completed turns missing time.completed. |
| electron/main/gateway/engine-manager.ts | Backfills missing time.completed for assistant messages returned from adapters and persists/emits the finalized message. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (result.role === "assistant" && !result.time.completed) { | ||
| const finalized = { ...result, time: { ...result.time, completed: Date.now() } }; | ||
| this.persistMessage(sessionId, finalized); | ||
| this.emit("message.updated", { sessionId, message: finalized }); | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
This new fallback path emits message.updated with message: finalized but does not rewrite finalized.sessionId (or its parts) from the engine session id to the conversation id. The renderer indexes messages by msgInfo.sessionId (see src/pages/Chat.tsx:1383-1386), so this event can be stored under the wrong session key and effectively disappear. Fix by rewriting the payload the same way forwardEvents() does (e.g., call rewriteSessionId({ sessionId: engineSessionId, message: finalized }, engineSessionId, sessionId) or at least set finalized.sessionId = sessionId and rewrite part.sessionId). Also, consider returning the finalized/rewritten message instead of result so the message.send RPC response includes time.completed consistently, and add/adjust the existing EngineManager unit test to cover this branch.
| if (result.role === "assistant" && !result.time.completed) { | |
| const finalized = { ...result, time: { ...result.time, completed: Date.now() } }; | |
| this.persistMessage(sessionId, finalized); | |
| this.emit("message.updated", { sessionId, message: finalized }); | |
| } | |
| return result; | |
| let response = result; | |
| if (result.role === "assistant" && !result.time.completed) { | |
| const finalized = { ...result, time: { ...result.time, completed: Date.now() } }; | |
| this.persistMessage(sessionId, finalized); | |
| const rewritten = rewriteSessionId( | |
| { sessionId: engineSessionId, message: finalized }, | |
| engineSessionId, | |
| sessionId, | |
| ).message as UnifiedMessage; | |
| this.emit("message.updated", { sessionId, message: rewritten }); | |
| response = rewritten; | |
| } | |
| return response; |
| // Safety-net end time: captured once when we first notice the session is done | ||
| // but time.completed is missing (e.g. session.idle event was dropped). | ||
| const [capturedEndTime, setCapturedEndTime] = createSignal<number | undefined>(); | ||
|
|
||
| const finalEndTime = createMemo(() => { | ||
| // Only trust time.completed when isWorking is false — during multi-step | ||
| // tasks, intermediate messages may carry completed timestamps prematurely. | ||
| if (props.isWorking) return undefined; | ||
| const lastAssistant = props.assistantMessages.at(-1); | ||
| return lastAssistant?.time?.completed; | ||
| return lastAssistant?.time?.completed ?? capturedEndTime(); | ||
| }); | ||
|
|
||
| createEffect(() => { | ||
| if (!props.isWorking && !capturedEndTime()) { | ||
| const lastAssistant = props.assistantMessages.at(-1); | ||
| if (lastAssistant && !lastAssistant.time?.completed) { | ||
| setCapturedEndTime(Date.now()); | ||
| } | ||
| } |
There was a problem hiding this comment.
capturedEndTime is component-local state and never resets when this SessionTurn instance is reused for a different turn/session. MessageList renders turns with <Index each={turns()}> (src/components/MessageList.tsx:77), which preserves component instances by index across dataset changes (e.g., switching sessions), so a previously captured end time can leak into a different turn and produce incorrect/negative durations when time.completed is missing. Consider resetting capturedEndTime whenever props.userMessage.id (and/or props.sessionID) changes, or keying the captured value by the last assistant message id.
Summary
Fix timer on inactive sessions: when switching to a chat with completed AI responses, the elapsed time counter was incorrectly ticking instead of showing a static value.
Supersedes #87 — incorporates the review feedback with a two-layer fix.
Root Cause
Some adapters (e.g. OpenCode) strip
time.completedduring multi-step agent loops and only restore it onsession.idle. If that event is dropped (e.g. during a WebSocket reconnect), the field staysundefined, causing the frontend timer to tick indefinitely. Additionally,persistMessage()in engine-manager skips messages withouttime.completed, so they never hit disk.Changes
1. Engine-manager root-cause fix (
engine-manager.ts)When
sendMessage()returns an assistant message withouttime.completed, set it toDate.now(), persist and emit the finalized message.2. Frontend safety net (
SessionTurn.tsx)Replace the
time.createdfallback (which showed ~0s duration) with acapturedEndTimesignal that snapshotsDate.now()once when the session is first observed as done withouttime.completed. This gives an approximately correct duration for any remaining edge cases.Credit
Original bug report and initial fix by @OverCart345 in #87.
Test Plan
npm run typecheck)bun run test:unit— 429 passed)Co-authored-by: OverCart345 82957545+OverCart345@users.noreply.github.com