Improve chat queue management and context meter freshness - #2495
Improve chat queue management and context meter freshness#2495timmilazzo wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
builder-io-integration
Bot
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🔴
Review Details
Code Review Summary
PR #2495 adds queue controls to the shared chat UI, preserves historical turn failures, attributes transcript work durations to individual groups, and adds context-manifest freshness reporting/refetch behavior. The overall direction is sound: the changes add focused pure helpers, typed manifest outcomes, bounded in-memory tracking, and meaningful unit coverage across queueing, SSE processing, transcript rendering, and context freshness. This is standard risk because it changes shared chat state, event processing, and persistence behavior.
Key Findings
- 🔴 HIGH — Unmatched tool completions can be silently dropped when separate same-name calls have the same result.
- 🟡 MEDIUM — The context meter can miss the final manifest write when the active-run polling interval is removed.
- 🟡 MEDIUM — Overlapping fire-and-forget manifest writes can let an older failure invalidate a newer successful manifest.
- 🟡 MEDIUM — Failure state is process-local, so a failed write may be invisible when the next request is handled by another worker or after restart.
The dev server is healthy and the browser test planner produced a full 18-case plan across /chat, /brain, and /dispatch, but all executor sessions lacked browser navigation/click/screenshot tools, so visual verification could not be completed in this environment.
🧪 Browser testing: Skipped — browser automation unavailable in executor sessions; dev server healthy.
| const alreadyRecorded = content.some( | ||
| (part) => | ||
| part.type === "tool-call" && | ||
| part.toolName === toolName && | ||
| part.result === (ev.result ?? ""), | ||
| ); | ||
| if (alreadyRecorded) return; |
There was a problem hiding this comment.
🔴 Orphan tool_done deduplication loses results for different IDs with same result
When no pending card matches, this replay check uses only the tool name and result. Two distinct same-name calls with different IDs and identical results can therefore cause the second completion to be discarded, losing a tool card from the transcript. Restrict deduplication to the same event ID (or only apply the fallback check to id-less events).
Additional Info
Found by 1 of 3 code-review agents; validated against the unmatched-completion path and same-name parallel-call scenario.
| { | ||
| enabled: shouldQuery, | ||
| staleTime: 1000, | ||
| refetchInterval: runActive ? ACTIVE_RUN_REFETCH_MS : false, |
There was a problem hiding this comment.
🟡 Refetch once after the active run clears
The interval is disabled as soon as the active run clears, but the final manifest write can occur after the last 5-second tick. Since app-state updates do not invalidate this action query, the meter can remain on an earlier reading until an unrelated refetch. Trigger one final refetch when runActive transitions from true to false.
Additional Info
Found by 1 of 3 code-review agents; the changed polling configuration has no completion refetch.
| const writeFailedAfterStored = | ||
| outcome?.status === "failed" && | ||
| outcome.failedAt > (stored?.updatedAt ?? 0); |
There was a problem hiding this comment.
🟡 Do not let an older async write failure invalidate a newer manifest
Manifest writes are intentionally fire-and-forget, so an older iteration can fail after a later iteration has successfully persisted. That older failure becomes the latest process-local outcome and is then surfaced against the newer stored manifest, causing the meter to show unavailable incorrectly. Track write generation/ownership and only surface a failure when it applies to the stored manifest.
Additional Info
Found by 1 of 3 code-review agents; consecutive transform iterations can overlap writes and all share the same logical turnId.
| * manifest — this map is the only thing that can tell a reader the newest | ||
| * turn never made it to storage. | ||
| */ | ||
| const writeOutcomes = new Map<string, ContextManifestWriteOutcome>(); |
There was a problem hiding this comment.
🟡 Persist manifest write failures outside the process-local outcome map
The failure outcome is retained only in an in-memory map, while the manifest and run lookup are durable. If the write runs in a different worker, or the process restarts before context-manifest-get, the failed latest turn is indistinguishable from an older manifest and the UI can show a stale percentage instead of unavailable. Persist a bounded failure marker or derive the failure state from durable data, clearing/replacing it on success.
Additional Info
Found by 1 of 3 code-review agents; deployment behavior makes process-local outcome tracking insufficient for the stated freshness guarantee.
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest commits add the offer-download action and inline artifact download card, wire it into the workspace-file tool set and resource route, and retain the earlier queue, transcript, and context-meter changes. The new artifact flow has good access scoping, explicit missing-file errors, URL encoding, and header sanitization. I also rechecked the four prior comments: all remain applicable and were not reposted or resolved.
This remains standard risk because the PR changes shared chat/runtime behavior, persisted run-event reconstruction, resource delivery, and public TypeScript contracts.
New Findings
- 🟡 MEDIUM — Per-event timestamps are added only to the in-memory envelope and are not persisted or propagated through the SSE payload, so reloads still lose the new duration data.
- 🟡 MEDIUM — Valid zero-byte artifacts fall through to the metadata response instead of downloading an empty file.
- 🟡 MEDIUM — The required
costRankedfield can break external consumers constructing the exported model-group type. - 🟡 MEDIUM — A first-turn manifest write failure is surfaced as
writeStatus: failedeven when no stored manifest exists.
The dev server is healthy and routes return HTTP 200. Browser verification was attempted with a full 17-case plan, including the new artifact flow, but executor sessions again lacked browser automation tools.
🧪 Browser testing: Skipped — browser automation unavailable in executor sessions; dev server healthy.
| const runEvent: RunEvent = { | ||
| seq: run.events.length, | ||
| event, | ||
| at: Date.now(), |
There was a problem hiding this comment.
🟡 Persist event timestamps with the run events
at is added only to the in-memory RunEvent, while emitRunEvent persists JSON.stringify(runEvent.event) and the live SSE payload likewise omits the envelope timestamp. SQL replay and transcript rebuilds therefore still lack per-part timestamps and fall back to whole-turn durations after reload or reconnect. Persist and restore the timestamp through the event storage/SSE path.
Additional Info
New finding from 1 of 3 incremental review agents; verified against the surrounding persistence code.
| const wantsDownload = query.download !== undefined; | ||
| const wantsRaw = query.raw !== undefined || wantsDownload; |
There was a problem hiding this comment.
🟡 Download empty workspace artifacts as files
?download sets wantsRaw, but the raw response still requires resource.content to be truthy. A legitimate zero-byte artifact has content: "", so it falls through to the metadata response without Content-Disposition and does not download the requested empty file. Handle empty string content as a valid raw payload.
Additional Info
New finding from 1 of 3 incremental review agents; confirmed by the truthiness guard at the raw-serving branch.
| label: string; | ||
| models: string[]; | ||
| configured: boolean; | ||
| /** | ||
| * True when every model in `models` matched a known cost tier, so the list | ||
| * really does run cheapest to most expensive. False when at least one model | ||
| * is unrecognised: unrecognised models sort to the end regardless of what | ||
| * they actually cost, and the picker must not claim an order it cannot back. | ||
| */ | ||
| costRanked: boolean; |
There was a problem hiding this comment.
🟡 Keep the exported model-group field backward compatible
EngineModelGroup is exported through the shared chat model APIs, but costRanked is now required. Host applications that construct EngineModelGroup[] without this display-only field will fail TypeScript compilation on upgrade even though the runtime can safely treat it as absent. Make the core interface field optional, while continuing to populate it in buildChatModelGroups.
Additional Info
New finding from 1 of 3 incremental review agents; the toolkit-side equivalent already treats the field as optional.
| const writeFailedAfterStored = | ||
| outcome?.status === "failed" && | ||
| outcome.failedAt > (stored?.updatedAt ?? 0); |
There was a problem hiding this comment.
🟡 Do not mark an empty manifest as a failed persisted reading
When there is no stored manifest, (stored?.updatedAt ?? 0) makes any in-memory failure outcome satisfy this condition. The action then adds writeStatus: "failed" to the synthetic empty manifest, conflating “no manifest has ever been written” with “an older persisted reading was invalidated.” Only apply this failed-write marker when a stored manifest exists.
Additional Info
New finding from 1 of 3 incremental review agents; verified against the empty-manifest branch.
|
@timmilazzo can you share some screenshots on this one? |
|
Fought Fusion / OAuth for Agent-Native in Interact a lot today to try to capture screenshots on this one. Couldn't get there which was frustrating, despite duplicating the branch, bringing it back up to Main sync multiple times. Will try again tomorrow. |
3 participants
Summary
This PR improves queued chat message handling, transcript accuracy, and context meter reliability by making queued/error states more visible and giving the context meter a way to detect and surface stale or unavailable readings.
Problem
Queued chat messages couldn't be managed (edited, reordered, removed, or sent immediately), and turn errors disappeared from the transcript as soon as another message was sent. Work group durations in the transcript incorrectly repeated the whole-turn time, and tools that never reported back or reported after being dropped left the UI stuck in a spinning or misleading state. Separately, the context meter had no way to know if the manifest it was displaying was stale relative to the actively running turn, so it could freeze on a previous turn's percentage or silently show incorrect data after a failed persist.
Solution
context-manifest-getaction.costRankedflag to model family groupings so the composer's model picker can note when a family is listed cheapest first.Key Changes
packages/core/src/agent/context-xray/manifest.ts:writeContextManifestnow returns a typedContextManifestWriteOutcome(written/failed) instead of throwing/swallowing errors; addedgetContextManifestWriteOutcome,recordContextManifestWriteFailure, and an in-memory outcome tracker capped at 500 threads.packages/core/src/agent/context-xray/actions/context-manifest-get.ts: resolves the thread's latest run/turn and includeslatestTurnId,latestTurnStartedAt, andwriteStatusin the returned manifest when a write failed after the stored manifest.packages/core/src/shared/context-xray.ts: addedContextManifestWriteStatusandContextManifestFreshnesstypes, newupdatedAt/writeStatus/latestTurnId/latestTurnStartedAtfields onContextManifest, and aresolveManifestFreshnesshelper that never marks an untrustworthy manifest ascurrent.packages/toolkit/src/context-ui/ContextMeter.tsx:ContextMeterViewaccepts afreshnessprop, dims the meter and shows an em dash or explanatory note when data is stale/unavailable, and exportsContextMeterFreshness.packages/toolkit/src/composer/TiptapComposer.tsx: added optionalcostRankedfield to model group props and renders a "Listed cheapest first" note when applicable; also exportscompactComposerModelNamefrom the composer index.manifest-write.spec.ts, freshness resolution tests) and three changesets documenting the queue management, transcript accuracy, and context meter freshness fixes.To clone this PR locally use the Github CLI with command
gh pr checkout 2495You can tag me at @BuilderIO for anything you want me to fix or change