Improve chat queue management and context meter freshness - #2495
Improve chat queue management and context meter freshness#2495timmilazzo wants to merge 4 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.
| 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. |
|
@steve8708 - have tried again today via Fusion and Netlify to get into a working state that allows login so I can test and grab some screenshots, to no avail unfortunately. One of the things this branch fixes is duplication of the same "Worked for 1m 21s" type messages in the chat stream when there are errors, which I'm seeing pretty frequently across the various apps: Here's my original Clip that kicked off my first Fusion session on the actual chat issued addressed, which are a handful: https://clips.agent-native.com/share/bj5M1AE24nkT?ref=clip_share |
|
@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable) |
|
Stopping here to report, because the situation changed materially and you should decide before I continue. The merge is resolved — but main superseded most of this branchWhile PR #2495 was open,
Verification: core typecheck clean, 557/558 tests passing across the affected suites, formatted, fully staged, zero conflict markers. The one failure ( Two things I want to flag rather than paper over: The subagent I delegated the merge to timed out and did a bad partial job — it concatenated two functions into a syntactically broken hybrid and dropped a test's closing braces. I found and repaired both. Worth knowing that resolution was hand-checked, not agent-trusted. I can't commit the merge — environment rules bar me from Not yet done
I did verify signup works and that a real session outranks the dev fallback — so it's doable. I also accidentally created a Tim — do you want me to create the account (I'd need you to pick the password, and it will create "Tim's workspace" as a new org), or would you rather self-serve via the signup page so no agent-invented credential exists? ⠀ |
Visual recap — screenshot failedA recap was published, but the PR-comment screenshot could not be captured or uploaded. Open the interactive recap directly: Open the full interactive recap Diagnostic: dark: screenshot captured but image upload failed |
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest PR head keeps the shared queue, transcript timing, unknown-outcome, model-picker, and context-meter changes while removing the earlier artifact-download additions. I verified that the prior empty-artifact comment is fixed by removal of that feature and resolved its stale review thread. All other previously open comments remain applicable and were skipped rather than reposted.
The latest changes are otherwise consistent with the existing design, but one new transcript-state regression is present. A terminal assistant message can still contain a tool call with no result after an interrupted/reloaded run, and the new rendering branch labels that completed transcript as actively thinking.
This remains standard risk due to shared chat state, event reconstruction, and context freshness behavior.
New Finding
- 🟡 MEDIUM — Terminal unresolved tool cards render a permanent active “Thinking” status when the run is no longer active, instead of an unknown terminal outcome.
The dev server is healthy. Full browser verification was attempted across 19 flows on /chat, /brain/agent, and /dispatch/agent, but browser executor sessions again lacked automation tools.
🧪 Browser testing: Skipped — browser automation unavailable in executor sessions; dev server healthy.
| variant="response" | ||
| /> | ||
| )} | ||
| {isLast && hasUnresolvedTool && !chatRunning && ( |
There was a problem hiding this comment.
🟡 Do not render terminal unresolved tools as active thinking
hasUnresolvedTool includes any persisted tool-call without a result. After an interrupted run is reloaded, chatRunning is false but this branch still renders RunningActivityStatus with “Thinking” indefinitely on the completed turn. Render a terminal unknown state for these cards, or only show this active status while the run is actually live.
Additional Info
New finding from 1 of 3 incremental review agents; confirmed against assistantMessageHasUnresolvedTool at lines 932-939 and the terminal rendering branch.
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