feat(chat): move thinking indicator to avatar progress badge#32680
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 864eb32a1d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const activeConversationIsProcessing = | ||
| activeConversationId != null && processingConversationIds.has(activeConversationId); | ||
|
|
||
| const activeConversationHasPendingAssistantResponse = useMemo( | ||
| () => hasPendingAssistantResponse(messages), | ||
| [messages], | ||
| ); | ||
|
|
||
| const hasStreamingAssistantMessage = messages.some((m) => m.isStreaming); | ||
| (activeConversationId != null && | ||
| processingConversationIds.has(activeConversationId)) || | ||
| activeConversation?.isProcessing === true; |
There was a problem hiding this comment.
When the cached conversation row was fetched while the turn was already running (isProcessing: true), terminal SSE only removes the local processingConversationIds entry; the cached activeConversation.isProcessing value remains true until some later list/detail refetch. Because this OR treats that stale snapshot as sufficient, the avatar badge, scroll streaming state, and Stop button can stay active after message_complete/generation_cancelled for conversations opened or refreshed mid-turn. Please patch the conversation cache to isProcessing: false in the terminal path (or otherwise make this snapshot expire) instead of waiting for an unrelated refetch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a8e9115c03. Terminal SSE handlers (handleMessageComplete, handleGenerationCancelled, handleAssistantActivityState(idle)) and both error handlers (handleStreamError, handleConversationErrorEvent) now patchConversation({ isProcessing: false }) before endTurn, so the OR derivation can't stay latched on a stale cached isProcessing: true after the local set is cleared.
| markConversationProcessing: (conversationId) => { | ||
| const { processingConversationIds } = get(); | ||
| if (processingConversationIds.has(conversationId)) return; | ||
| set({ | ||
| processingConversationIds: addToSet( | ||
| processingConversationIds, | ||
| conversationId, | ||
| ), | ||
| }); |
There was a problem hiding this comment.
For turns marked from SSE rather than useSendMessage (for example an external-channel turn in an open tab), this adds the processing id without a processingSnapshots entry. If the user switches away before the turn completes, useAttentionTracking compares the conversation's existing latestAssistantMessageAt against undefined and immediately graduates/removes the processing key, so the sidebar/return-to-chat processing affordance disappears while the assistant is still running. Seed the snapshot from the current cached conversation (or make graduation ignore snapshotless keys) when adding these SSE-only processing ids.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a8e9115c03. handleAssistantTurnStart and handleAssistantTextDelta now call markConversationProcessing(conversationId, cached?.latestAssistantMessageAt). markConversationProcessing itself is idempotent with first-writer-wins snapshot semantics: if the conversation is already in the processing set AND has a recorded snapshot, no-op; otherwise add to the set if missing and seed the snapshot if missing. SSE-only turns now get a baseline snapshot so attention-tracking won't immediately graduate.
| /** | ||
| * Whether the assistant is currently mid-turn for the active | ||
| * conversation. Drives two affordances: | ||
| * | ||
| * 1. SVG path wobble on the character avatar (subtle "thinking" cue) | ||
| * 2. Pulsing dot badge in the bottom-right corner (the primary | ||
| * processing indicator — see {@link ProgressBadge}) | ||
| * | ||
| * Source of truth: `useConversationStore.processingConversationIds.has(activeId)` | ||
| * OR `activeConversation.isProcessing === true`. Either signal lights | ||
| * the badge; convergence is handled by terminal SSE events (clear | ||
| * local) and the next list/detail GET (refresh server snapshot). | ||
| */ |
There was a problem hiding this comment.
none of the other props have docstrings, remove
There was a problem hiding this comment.
Done in a8e9115c03 — JSDoc removed from the isProcessing prop. isStreaming kept as-is since it still drives the wobble animation and is untouched by this PR (all 6 existing callers preserved).
| /** | ||
| * Whether the agent loop is mid-turn for this conversation. Mirrors | ||
| * the daemon's in-memory `Conversation.isProcessing()` and is the | ||
| * single source of truth for the avatar progress badge + every other | ||
| * "is this conversation currently working" affordance. | ||
| * | ||
| * Server-seeded on conversation list/detail responses; SSE start | ||
| * events (`assistant_turn_start`, `assistant_text_delta`) flip it to | ||
| * `true` and terminal events (`message_complete`, | ||
| * `generation_cancelled`) flip it back to `false` via | ||
| * `useConversationStore.processingConversationIds`. | ||
| * | ||
| * Optional because: | ||
| * - Daemon < 0.8.7 doesn't surface the field on the wire. | ||
| * - Optimistic draft conversations exist client-side before any | ||
| * server response. | ||
| */ |
There was a problem hiding this comment.
make it a one-liner
There was a problem hiding this comment.
Done in a8e9115c03 — Conversation.isProcessing docstring is now a one-liner.
| function ProgressBadge({ size }: { size: number }) { | ||
| const dot = Math.max(6, Math.round(size * BADGE_DOT_RATIO)); | ||
| const ring = Math.max(1, Math.round(size * BADGE_RING_RATIO)); | ||
| return ( |
There was a problem hiding this comment.
let's put this behind a _vellumDebug.flags.toggleProgressBadge() flag for now, where when it's disabled, we show the old thinking indicator
There was a problem hiding this comment.
Done in a8e9115c03. New flag module at apps/web/src/lib/feature-flags/progress-badge-flag.ts modeled after impersonate-version-flag.ts (localStorage-backed, reload-on-flip, inspect-only when called with no args). Registered as _vellumDebug.flags.toggleProgressBadge() via the existing flagsApi installer in debug-api.ts. Default is off → legacy transcript thinking-dots indicator renders. Flag on → dots are suppressed (shouldShowThinkingIndicator gated) and the new avatar badge lights up via isProgressBadgeEnabled() inside ChatAvatar. Same OR-derived activeConversationIsProcessing signal drives both modes.
| const turnState: TurnState = { phase, pendingQueuedCount, activeToolCallCount, activeTurnId, lastTerminalReason, statusText, liveWebActivity, autoRoutedProfileLabel }; | ||
| const turnState: TurnState = { |
There was a problem hiding this comment.
This PR is somewhat hard to read with all of the prettier changes - can we split that off into its own PR and merge that in first, so we can focus on the design decisions in this PR?
There was a problem hiding this comment.
Done in a8e9115c03. Hard-reset to base a4e49891f9 and re-applied only additive edits. Diff is now 9 files, +302/-21 (was 18 files, +1253/-975). No ThinkingItem deletion, no UIContext rewrites, no shouldShowThinkingIndicator removal, no prettier reflow on import lines. The prettier-only follow-up (reflow the existing files to match prettier 3.8.1's defaults) will land as its own PR after this.
| const hasStreamingAssistantMessage = messages.some((m) => m.isStreaming); | ||
| (activeConversationId != null && | ||
| processingConversationIds.has(activeConversationId)) || | ||
| activeConversation?.isProcessing === true; |
There was a problem hiding this comment.
activeConversation?.isProcessing === true is the same as !!activeConversation?.isProcessing
There was a problem hiding this comment.
Done in a8e9115c03 — switched to !!activeConversation?.isProcessing.
|
Working through all this. Plan:
Force-push incoming once it's green. |
Adds an avatar-anchored progress badge for the active conversation,
gated behind a new `_vellumDebug.flags.toggleProgressBadge()` flag.
Default (flag off) is the legacy transcript thinking-dots indicator;
flag on hides the dots and lights the avatar badge instead. Either
mode is driven by the same `activeConversationIsProcessing` signal,
so the underlying source-of-truth wiring is shared.
* New flag module `lib/feature-flags/progress-badge-flag.ts` mirrors
the `impersonate-version-flag.ts` pattern (localStorage-backed,
reload-on-flip, inspect-only when called with no args). Registered
under `window._vellumDebug.flags` via the existing `flagsApi`
installer in `debug-api.ts`.
* `Conversation.isProcessing` is now a server-seeded optional field on
the `Conversation` type, populated by `toConversation` from the raw
daemon payload (pre-0.8.7 daemons omit it; optimistic drafts omit it).
* `activeConversationIsProcessing` is now the OR of the local
optimistic set (`processingConversationIds`) and the cached server
snapshot, so the indicator lights for SSE-only turns (Slack /
Telegram external channels) where the local `useSendMessage` flow
never ran.
* `markConversationProcessing(id, snapshot?)` is a new action on the
conversation store. Idempotent (start events fire many times per
turn) and tolerant of pre-seeded entries: if the conversation is
already in the processing set AND has a recorded snapshot, no-op.
Otherwise seed both. First-writer-wins, so a send-side
`addProcessingConversationId` always takes precedence over a later
SSE mark.
* SSE start handlers (`assistant_turn_start`, `assistant_text_delta`)
now call `markConversationProcessing` with the cached conversation's
`latestAssistantMessageAt` so attention-tracking has a baseline to
graduate against. Without the seed, switching tabs mid-turn on an
SSE-only conversation would immediately graduate the processing key
(comparing `number !== undefined`) and drop the sidebar processing
affordance.
* Terminal SSE handlers (`message_complete`, `generation_cancelled`,
`assistant_activity_state(idle)`) and both error handlers
(`handleStreamError`, `handleConversationErrorEvent`) now patch the
cached conversation row to `{ isProcessing: false }` before
`endTurn`, so the OR derivation can't stay latched on a stale server
snapshot after the local set is cleared.
* `ChatAvatar` gains an optional `isProcessing` prop (existing
`isStreaming` is preserved for the wobble animation; all six existing
callers untouched). Internal `showBadge` gates the new
`<ProgressBadge>` on `isProcessing && isProgressBadgeEnabled()`.
* `shouldShowThinkingIndicator` is now suppressed when the flag is on,
so the new badge fully replaces (rather than duplicates) the
in-transcript dots affordance.
vellum-apollo-bot
Bot
force-pushed
the
apollo/b7-avatar-progress-badge
branch
from
864eb32 to
a8e9115
Compare
|
Force-pushed Diff shape
What I did NOT touch
Verification
Replied per-thread to all 7 review comments above. |
… flags test mock Resolves CI lint + type-check failures on the avatar progress-badge PR: - Lint: the chat stream handlers patched the conversations query cache via a cross-domain import. Move the generic Conversation[] cache primitives (updateConversationsCache/findConversation/getConversations/patchConversation) to @/utils/conversation-cache and re-export from conversation-queries so existing consumers are unchanged. No new cross-domain-allowlist entries. - Type check: add toggleProgressBadge to the debug-api test flags mock (and drop the stale toggleTranscriptScrollController reference) so it satisfies VellumDebugFlagsApi. Co-Authored-By: vargas@vellum.ai <vargas@vellum.ai>
1 participant
Summary
Move the chat thinking indicator from a transcript row to a small pulsing badge on the assistant avatar, driven by a single OR-derived signal that converges naturally between local optimistic state and the daemon's server-authoritative
Conversation.isProcessingflag.Before: a
ThinkingItemrow appeared in the transcript whenever local turn-state was "thinking", with a chain of UIContext flags (hasStreamingAssistantMessage,hasPendingAssistantResponse,restoredProcessing, …) trying to reconcile the state of externally-originated turns, tab-switches, and pre-0.8.7 daemons. The flags drifted.After: every read site derives the same boolean
Either source is sufficient — terminal SSE events clear the local set, the next list/detail GET refreshes the server snapshot, and pre-0.8.7 daemons (where
isProcessingisundefinedon the wire) fall back cleanly to the local source.What's new
Conversation.isProcessingcarried throughtoConversationfrom the daemon's OpenAPI schema. Optional because (a) daemon < 0.8.7 doesn't surface it and (b) optimistic draft conversations exist before any server response.useConversationStore.markConversationProcessing(id)— idempotent action invoked from theassistant_turn_startandassistant_text_deltaSSE handlers via a sharedresolveConversationId(event, ctx)helper. Does not touchprocessingSnapshots— snapshot ownership stays with the send pipeline for attention graduation.ChatAvatarprops:isStreaming→isProcessing. When true, renders aProgressBadge(BusyIndicator inside asurface-basering at bottom-right). Geometry scales fromsize: dot diameter =round(size × 0.16), ring =round(size × 0.04). InnerAnimatedAvatarstill receivesisStreamingto preserve the SVG path wobble.canStopGenerationsimplifies to: pending-prompt/surface gates block Stop →false; otherwisetrueiffactiveConversationIsProcessing || isSending(state). The daemon reportsisProcessing: trueeven duringuser_pause, but clicking Stop while a surface is open would discard user input — hence gate-first ordering.thinkingIndicator()→progressBadge()returning{ visible, stopAvailable, failingStopConditions, explanation, … }. The triage payload in the feedback modal follows.What's gone
shouldShowThinkingIndicatorandgetThinkingStatusTextselectors.TranscriptItemkind"thinking", theThinkingIteminterface, the matching transcript-row case.UIContext.hasStreamingAssistantMessageandUIContext.hasPendingAssistantResponse— both were proxies for signals now derivable fromactiveConversationIsProcessing.hasPendingAssistantResponsehelper (zero remaining callers).What stays (deferred)
DisplayMessage.isStreamingfield. Still consumed by message-merge / reconcile /chat-body/scroll-to-latest-button. Removing it cleanly is a follow-up scope so this PR can stay focused on the badge migration.Architecture invariants
chat-route-content.tsxandchat-page.tsxderiveactiveConversationIsProcessingindependently from the same two sources. The two sources may briefly disagree (e.g. SSE complete fires before the next list GET) — and that's fine, because the OR returnstruewhenever either fires andfalseonly when both agree.markConversationProcessingonly touchesprocessingConversationIds. TheprocessingSnapshotsmap (turn-start envelopes for attention graduation) is still owned by the send-side caller — adding turn-start coverage there is a separate concern.Tests
223 cases green across the touched files under
bun test:turn-store.test.ts— 118 cases. DroppedshouldShowThinkingIndicatorandgetThinkingStatusTextdescribe blocks. Reshaped thecanStopGenerationexternally-originated test to useactiveConversationIsProcessing(Slack/Telegram/CLI scenario).debug-api.test.ts— 9-caseprogressBadgedescribe replaces thethinkingIndicatorblock. Covers processing/idle, terminal phases withlastTerminalReason, tool-call-while-processing (tool calls no longer hide the badge), pending-prompt gating (theuser_pausemodel where the agent is processing but the user has a UI to respond to), queued phase, and UIContext snapshot pass-through.build-items.test.ts/partition-latest-turn.test.ts— removed ThinkingItem-specific cases. The partition tests useprofileAutoRoutedItemas the non-message trailer fixture sinceThinkingItemis gone.Trusting CI for the remaining workspace (full typecheck, format, lint:unused). Locally ran prettier on every touched file.
AGENTS.md compliance
Grepped
vellum-assistant/AGENTS.mdforisProcessing,avatar,thinking indicator,conversation list— no codified conventions specific to this surface. The change preserves the existing single-source-of-truth invariant for chat domain state by replacing the latched proxies with a derived OR at read sites.