fix(ui): refresh session usage info on step and usage events - #731
Conversation
pascalandr
left a comment
There was a problem hiding this comment.
CHANGES_REQUESTED
The active session.step.ended / session.step.failed projection can refresh the chips, but this does not yet restore the complete V2 usage contract:
session.usage.updatedcarries the authoritative cumulativecost/tokens, yet the new path does not consume that payload and cannot update inactive sessions or initial hydration. This is the same first-load gap explicitly noted in the PR description.session.revert.committedremoves normalized messages after the new refresh point, sosessionInfoByInstanceand thread totals retain the removed usage.- No regression test was added for this central event-ordering path. Please cover active step completion, usage-only/inactive or initial hydration, and a usage-decreasing revert in one of the runnable store suites (for example
runtime-contract.test.ts).
I reproduced both stale-state cases locally: an authoritative session with cost 2.5 and tokens 1200/300 still published 0/0/$0, and removing a 10/5/$1 assistant message cleared the message-store usage while the displayed session info remained 10/5/$1.
The test/typecheck jobs and completed builds are green; the Tauri Windows and macOS x64 builds were still running when this review was submitted. No merge performed.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/35537978024 Artifacts expire in 7 days.
|
958eef2 to
a7df40c
Compare
The V2 migration dropped the live updateSessionInfo() calls from the SSE handlers, so the Status tab's cost/token chips were only written at message load (and stayed at 0 for sessions that were empty when opened). Recompute them after projecting messages for session.step.ended, session.step.failed, session.usage.updated, and forced resyncs.
- fall back to the session record's cost/tokens when no message usage is loaded - refresh on session.usage.updated for any session, after the record is updated - refresh after session.revert.committed removes messages - initialize info for already-loaded sessions on open - add session-usage-contract.test.ts to the force-exit CI list
7d2a4d3 to
1085812
Compare
pascalandr
left a comment
There was a problem hiding this comment.
CHANGES_REQUESTED on 10858125.
The new event ordering, inactive-session path, hydration fallback, and runnable regression file address much of the earlier review. One correctness gap still leaves the Status totals dependent on the bounded resident transcript: any single loaded usage entry wins over the authoritative session totals, and an all-message revert falls back to the same non-decrementing native counters. Please address the inline P1 and cover both boundaries. No merge performed.
…parately The loaded transcript is a bounded window, so message sums cannot stand in for the session totals when the server reports them. Reverted messages are recorded as a separate adjustment because the server's usage counters do not decrement on session.revert.committed.
|
Addressed in
Known limit: |
pascalandr
left a comment
There was a problem hiding this comment.
CHANGES_REQUESTED on 489e8a9c.
The two boundaries from the previous review are fixed, and the submitted usage tests pass. However, the acknowledged limitations are normal CodeNomad flows and leave the same displayed totals with different values depending on transcript residency and renderer lifetime:
- Bounded historical window: with native totals
50 / 25 / $5, resident boundary messagem1 = 10 / 5 / $1, and newerm2 = 40 / 20 / $4outside the resident window, committing the revert tom1leaves40 / 20 / $4instead of zero.removedcan only enumerate resident records. - Reload after revert: reverting the only
10 / 5 / $1message shows zero until the renderer state is rebuilt; hydration then restores10 / 5 / $1from OpenCode's persisted cumulative counters because the module-local adjustment is gone.
OpenCode 2.0.11 deleting messages without decrementing SessionInfo.cost/tokens explains the constraint, but it does not make the inconsistent result safe to ship. Please choose one stable contract: either lifetime usage (never subtract on revert) or current-transcript usage backed by complete, reload-reconstructible authority. Add regressions for a revert from an anchored historical window and post-reload hydration.
Local validation: UI typecheck passed; 81/81 relevant native-event, request-authority, and usage tests passed; both additional boundary reproductions failed as described. No merge performed.
- session totals come from the server's cumulative usage counters and are not adjusted on revert, so the display matches across windows and reloads - tests cover revert from an anchored window and post-reload hydration
|
Switched to the lifetime-usage contract in
PR description updated to match. UI typecheck clean; the usage, runtime-contract, and native-events suites pass (22/22) under the force-exit command. |
pascalandr
left a comment
There was a problem hiding this comment.
The new lifetime-usage contract resolves the two production inconsistencies from the prior review: anchored-window reverts and renderer reloads now intentionally retain the same native cumulative totals. One focused regression-test flaw remains before signoff; the current reload case never discards the first in-memory session-info value. The known system-message timeouts are ignored per project-owner direction.
|
Fixed the reload test in |
pascalandr
left a comment
There was a problem hiding this comment.
Thanks @markerikson !
The Status tab's INPUT / OUTPUT / COST chips show 0 / 0 / $0.00 for sessions that were empty when opened, and never update while a session runs.
The Status tab reads
sessionInfoByInstance, which is only written byupdateSessionInfo(). The V2 migration (#647) removed the calls to it from the live SSE handlers, leaving only the message-load and agent/model-switch paths.updateSessionInfo()also summed usage from resident messages only, which in V2 is a bounded window rather than the whole session.The displayed totals are now the session's lifetime usage, taken from the server's cumulative
Session.cost/Session.tokenscounters:updateSessionInfo()usessession.cost/session.tokenswhenever the server reports them, independent of which messages are loaded. Message sums only stand in when the server sends no session usage.session.usage.updatedhandler (outside the active-transcript gate, so inactive sessions update too), aftersession.step.ended/session.step.failedprojection, and on load for an already-hydrated session that has no info yet.session.revert.committed, so subtracting client-side was only correct for messages that happened to be resident and was lost on reload. Lifetime totals are the same number in every window and after every reload.stores/session-usage-contract.test.ts(added to the force-exit CI list) covers: active step completion, usage-only event with an empty transcript, inactive session, initial hydration from the session record, authoritative totals over a partial window, revert with resident messages, revert from an anchored historical window with the newer message not resident, and post-reload hydration after a revert.