feat(desktop): refresh the composer context gauge per settled provider request - #4731
Conversation
caafc60 to
2b35196
Compare
…r request The gauge beside the model picker waited for the turn-end token_usage record, so a long agentic turn — dozens of steps, exactly when context grows fastest — showed the previous turn's number throughout. The Host already seals a latest-context snapshot at every settled request for the inspector's context bar; this feeds the gauge from the same snapshot. A probe owned by the workbar feature pulls context.diagnostics on the inspector's own signal (trace-relevant live events, 400 ms-coalesced), with the same protections: revision guard against out-of-order reads, last value standing on a failed read, in-flight reads dropped when the session changes. The snapshot carries no connectionId, so the route guard matches on (providerType, modelId); when it cannot vouch for the composer's active route the gauge falls back to the per-turn anchor, so neither surface ever shows one model's tokens against another's window. The probe reaches the composer as an injected render-prop component: the app shell gains no hook or bridge path, and the renderer root's refresh policy moves into the feature that owns its only readers. Refs apache#4717 Generated-by: Maka
2b35196 to
5a48e49
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 5a48e49eb4888af19691c05c27553892d31887e8.
This change adds a Workbar-owned diagnostics probe that refreshes the composer context gauge after settled provider requests, moves the shared trace refresh policy under the Workbar feature, injects the probe through the Workbar controller, and updates the renderer architecture inventory.
I found two P2 correctness issues, both noted inline.
Validation completed on this head: clean dependency install; build/test compilation; full typecheck; Desktop Knip; renderer architecture against parent 4ed6255f2998eff93414491cb14305f3bca0884b; 19 focused live-context/session-refresh tests; AppShell hook gate; changed-file Biome; ASF headers; and git diff --check. The hosted CI run 33865711701 passed, including build, typecheck, Knip, renderer architecture, Desktop E2E, browser smoke, and Storybook smoke. The full local Desktop suite reported 2,122 passed, 0 failed, and 8 cancelled tests in the unchanged MCP OAuth timer suite under Node 22.22.1; running that file alone reproduced the same 7 passed / 8 cancelled result.
GitHub reported the PR open and mergeable, with base ref 3dc920f42d16315666196b59588eb45de858ed68. Latest fetched origin/main was 14910a8ee428180bc6c428d4861f17b90b4b04d4; the three-way merge tree was clean (155e1a2850736d8104727c6257dafe6fd4212ec4). I did not run an interactive desktop smoke or execute the synthetic current-main tree.
Codex-assisted review performed under the maintainer-approved review workflow.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| revision += 1; | ||
| target = next; | ||
| coalescer.cancel(); | ||
| if (!next) { |
There was a problem hiding this comment.
[P2] Clear the previous reading when changing targets
This only emits undefined when the new target is absent. The hook's React state survives effect cleanup and recreation, so switching from session/model A to B leaves A's precise usage visible until B's read resolves. If B's first diagnostics read rejects, the rejection handler deliberately preserves that old value indefinitely. I reproduced this on this head by resolving A at 79,436 tokens, switching to B, and rejecting B's first read: the tracker emitted only A's value instead of clearing it. Clear the prior reading before refreshing any changed non-empty target.
There was a problem hiding this comment.
Confirmed by reproduction on the merged head (A resolves at 79,436 → switch to B → B's first read rejects → A's number pinned). Fixed in follow-up #4837: a changed target now clears the reading before the first read on the new one, while re-aiming at the same target keeps the standing value (no flicker on same-target refresh failures). The kept value is now only ever the current target's.
| ref={composerRef} | ||
| {...composerRest} | ||
| contextUsage={contextUsage && liveContextUsage | ||
| ? { ...contextUsage, usageTokens: liveContextUsage.usageTokens } |
There was a problem hiding this comment.
[P2] Carry the snapshot's frozen context window with its usage
LiveContextUsage includes the contextWindow frozen for the same provider request, but this boundary narrows the value to usageTokens and replaces only the numerator. ContextUsageAction then divides it by the current model choice's declared or metadata window. Without a declared override, a catalog/window change can pair the snapshot tokens with a different denominator; when metadata has no window, the gauge omits the percentage even though the snapshot supplied one. Thread contextWindow through and use the order declared override, snapshot window, then metadata fallback.
There was a problem hiding this comment.
Confirmed: the boundary narrowed the reading to { usageTokens }, so the gauge divided the snapshot's numerator by the live catalog's window while the inspector's bar used the snapshot's frozen one. Fixed in follow-up #4837: the frozen contextWindow travels with the tokens as meteredContextWindow, resolved declared override → metered → metadata. The gauge and the inspector bar now read the share from the same row unless the user declared an override, which wins by design.
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head 5a48e49eb4888af19691c05c27553892d31887e8. No P0 or P1. I independently confirmed both P2s already raised by hqhq1025 on this head and have nothing to add to their reproduction — but each has a second edge worth stating, and one of them contradicts a claim this PR makes about itself.
On the target-change P2
The clearing gap is real, and the code's own comment is the cleanest evidence for it:
Any target change — another session, another route, or none — makes the current reading unanswerable until the next read lands
setTarget then emits undefined only when the next target is absent, and takes the refresh() path otherwise. So for the case the comment names first — another session — the unanswerable reading stays on screen.
What makes this more than a brief flicker is the combination of two individually reasonable decisions. Keeping the last value on a failed read is defensible on its own; the comment argues it well ("blanking it would report 'no usage' for a read that simply failed"). Not clearing on target change is survivable on its own, because the next read usually lands. Together they mean a rejected first read on the new target pins the previous target's number in place indefinitely — and neither comment is wrong about its own case, which is exactly why this is easy to miss reading either one alone.
On the frozen-window P2
Also real, and the type at the boundary is where it becomes unfixable rather than merely unhandled. LiveContextUsage carries:
readonly usageTokens: number;
/** The window the request was metered against, frozen at call time. */
readonly contextWindow?: number;That comment says the two are a pair. chat-composer-region.tsx:298 narrows the parameter to { readonly usageTokens: number }, so the pairing is not merely dropped downstream — the boundary's type makes it unrepresentable, and the spread below then replaces the numerator while leaving the denominator to come from the current model choice.
This is where it touches a claim the change makes about itself. The module's own doc comment says:
the inspector's context bar reads the same field, so both indicators in the window draw one number from one row and cannot disagree mid-turn
and the PR summary repeats it. That holds for the token count. It does not hold for the percentage: the inspector renders the snapshot's tokens against the snapshot's frozen window, while the gauge renders the same tokens against the current model choice's declared or metadata window. When those differ — a catalog change, or metadata carrying no window at all — the two indicators disagree, or the gauge drops the percentage the snapshot could have supplied. The guarantee is one field short of what it claims.
Threading contextWindow through and resolving declared override → snapshot window → metadata fallback closes both the correctness case and the claim.
What holds
The route guard is genuinely fail-closed: liveContextUsageFromDiagnostics returns undefined unless diagnostics are available, both route fields are defined, and modelId/providerId both match, with a further reject on non-finite or non-positive token counts. So the fallback to the per-turn anchor is reached by refusal rather than by accident, and the "never one model's tokens against another's window" property is upheld at the numerator — the gap above is on the denominator, which the guard does not cover.
The revision counter is checked inside the resolve handler before onChange, so out-of-order reads cannot overwrite a newer one, and setTarget/dispose both bump it. Moving session-trace-refresh.ts next to its only consumer, and injecting the probe as a prop rather than adding a hook call to the app shell, keeps the change inside the renderer's existing constraints instead of asking for an exception.
test is green on this head and the three-way merge tree against current main is clean. This is a feature, so the merge decision remains a human's.
简体中文
在 5a48e49eb4888af19691c05c27553892d31887e8 上批准。没有 P0/P1。 hqhq1025 在这个 head 上已经提出的两条 P2 我独立复核过,都成立,复现部分我没有可补充的——但每一条都还有第二个侧面值得写出来,而且其中一条与这个 PR 对自己的一处声称相矛盾。
关于「切换 target」那条
清除缺失是真实的,而最干净的证据就是代码自己的注释:
任何 target 变化——另一个 session、另一条 route、或没有——都会让当前读数在下一次读取落地前无法回答(unanswerable)
然而 setTarget 只在下一个 target 为空时才发出 undefined,其余情况直接走 refresh()。于是对注释首先点名的那种情况——另一个 session——那个「无法回答」的读数仍然留在屏幕上。
让它不止是短暂闪烁的,是两个各自合理的决定叠在一起。 读取失败时保留旧值,单独看是站得住的,注释也论证得不错(「blanking it would report 'no usage' for a read that simply failed」);切换时不清除,单独看也能活,因为下一次读取通常会落地。但两者合起来意味着:新 target 的首次读取一旦被拒,上一个 target 的数字就被无限期钉在那里——而两处注释各自都没说错,这正是单独读任何一处都容易漏掉它的原因。
关于「冻结窗口」那条
同样真实,而边界处的类型是它从「未处理」变成「无法处理」的地方。LiveContextUsage 携带:
readonly usageTokens: number;
/** The window the request was metered against, frozen at call time. */
readonly contextWindow?: number;那句注释说的就是:这两者是一对。 而 chat-composer-region.tsx:298 把参数窄化成 { readonly usageTokens: number },于是这个配对不只是在下游被丢弃——边界的类型让它根本无法被表达;紧接着的展开只替换了分子,分母则留给当前模型选择去提供。
这里就碰到了这次改动对自己的一处声称。 模块自己的文档注释写着:
inspector 的 context bar 读的是同一个字段,所以窗口里两个指示器从同一行取同一个数字,不可能不一致
PR 摘要也重复了这句。对 token 数而言它成立;对百分比而言不成立:inspector 用快照的 tokens 除以快照冻结的窗口,而 gauge 用同样的 tokens 除以当前模型选择声明的或 metadata 的窗口。两者不同时——catalog 变化,或 metadata 根本没有窗口——两个指示器就会不一致,或者 gauge 干脆不显示那个快照本可以提供的百分比。这个保证少了一个字段。
把 contextWindow 一并传下去,并按「声明的覆盖值 → 快照窗口 → metadata 兜底」取值,就能同时关掉正确性问题和这处声称。
站得住的部分
route guard 确实是 fail-closed:liveContextUsageFromDiagnostics 在 diagnostics 非 available、route 两个字段任一未定义、或 modelId/providerId 任一不匹配时都返回 undefined,并对非有限值和非正 token 数再拒一道。所以回退到每轮锚点是「被拒绝」的结果,而不是碰巧;而「绝不把一个模型的 tokens 配到另一个模型的窗口上」这条性质,在分子侧是守住的——上面那个缺口在分母侧,而 guard 不覆盖分母。
revision 计数在 resolve 处理器里、onChange 之前就检查,所以乱序读取无法覆盖较新的结果,setTarget 与 dispose 也都会递增它。把 session-trace-refresh.ts 移到它唯一的消费者旁边、以及把 probe 作为 prop 注入而不是给 app shell 新增 hook 调用,都是让这次改动待在渲染层既有约束之内,而不是去申请一个例外。
这个 head 上 test 为绿,与当前 main 的三方合并树干净。这是一个 feature,合并与否仍由人决定。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
…zen window (#4837) Two review follow-ups to #4731, both confirmed against the merged head: - The tracker only cleared its reading when the target went away, so switching sessions or routes left the previous target's number on screen — indefinitely, when the new target's first read rejected, since a failed read deliberately keeps the last value standing. A changed target now clears the reading before the first read on the new one; re-aiming at the same target keeps the value, so a same-target refresh failure still does not flicker. The kept value is now only ever the current target's. - The probe boundary narrowed the live reading to usageTokens, so the gauge divided the snapshot's numerator by the live catalog's window while the inspector's bar used the snapshot's frozen one — the two indicators could disagree on the share, and with no metadata window the gauge dropped a percentage the snapshot could have supplied. The frozen contextWindow now travels with the tokens as meteredContextWindow, resolved after a declared override and before metadata. Refs #4717 Generated-by: Maka
Summary
The desktop composer's context gauge (next to the model picker) only updated once per turn: it read the newest
token_usagerecord'slastRequestAnchor, and that record is written once per send, after the agent loop breaks. During a long agentic turn — dozens of steps, minutes, exactly when context grows fastest — the gauge showed the previous turn's value throughout.The Host already seals a
latest_contextsnapshot at every settled provider request (the inspector's context bar reads it). This PR feeds the gauge from the same snapshot, so it refreshes step by step mid-turn. Both indicators in the window now draw from one row and cannot disagree.Design notes:
LiveContextUsageProbe, a render-prop component in the workbar feature, wraps the composer and pullscontext.diagnostics.queryon the inspector's own signal: trace-relevant live events, coalesced over 400 ms (session-trace-refresh.ts).connectionId, so acceptance matches on (providerType,modelId). On mismatch — or no snapshot — the gauge falls back to the existing per-turn anchor; it never shows one model's tokens against another's window.chat-composer-region.tsxas a prop, so the app shell gains no hook call or bridge path (the renderer debt ratchet forbids both); one pre-existing unused import (ProjectRecord) was removed to keep the ledger flat.session-trace-refresh.tsmoves from the renderer root into the workbar feature, next to its only consumer.Fixes #4717
Verification
live-context-usage.test.ts(13 cases): route guard accept/refuse, debounce coalescing, delta filtering, out-of-order discard, failure-keeps-last, session-switch discard, route-switch re-evaluation, dispose semantics. These fail without the new module.node --test "dist/main/**/__tests__/*.test.js").origin/main; ledger regenerated.tsc --noEmitclean for main/renderer/storybook configs. Preload config shows 8 errors that are identical on the base commit in a fresh worktree (pre-existing, unrelated to this change).AI use
Tool(s) and scope: Maka (AI agent) — issue analysis, design, implementation, tests, and this PR description. Commit carries the
Generated-bytrailer.Checklist
Does this PR entail a change in behavior?