feat: show message info on click - #2802
Conversation
|
@alectimison-maker is attempting to deploy a commit to the esokullu's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
Adds inspectable message timestamps and verbose assistant-generation metrics across Chrome and Firefox.
Changes:
- Adds click and keyboard-accessible message-info rows.
- Aggregates generation tokens, duration, speed, and stop reason.
- Persists timestamps and completion metadata across sidepanel restores.
Reviewed changes
Copilot reviewed 50 out of 55 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/chrome/src/agent/agent.js |
Publishes aggregate completion metrics. |
src/chrome/src/message-info.js |
Formats timestamps and generation metrics. |
src/chrome/src/ui/sidepanel.js |
Renders, toggles, and persists message info. |
src/chrome/styles/sidepanel.css |
Styles interactive messages and info pills. |
src/chrome/src/ui/locales/ar.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/bn.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/de.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/en.js |
Adds English message-info strings. |
src/chrome/src/ui/locales/es.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/fa.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/fr.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/he.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/hi.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/id.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/ja.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/ko.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/ms.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/nl.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/pl.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/pt.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/ru.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/th.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/tl.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/tr.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/uk.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/vi.js |
Adds message-info fallback strings. |
src/chrome/src/ui/locales/zh.js |
Adds message-info fallback strings. |
src/firefox/src/agent/agent.js |
Mirrors completion metric publishing. |
src/firefox/src/message-info.js |
Mirrors message-info formatting. |
src/firefox/src/ui/sidepanel.js |
Mirrors message-info UI and persistence. |
src/firefox/styles/sidepanel.css |
Mirrors message-info styling. |
src/firefox/src/ui/locales/ar.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/bn.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/de.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/en.js |
Adds English message-info strings. |
src/firefox/src/ui/locales/es.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/fa.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/fr.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/he.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/hi.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/id.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/ja.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/ko.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/ms.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/nl.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/pl.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/pt.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/ru.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/th.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/tl.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/tr.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/uk.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/vi.js |
Adds message-info fallback strings. |
src/firefox/src/ui/locales/zh.js |
Adds message-info fallback strings. |
test/run.js |
Tests formatting, aggregation, and mirrored wiring. |
Files not reviewed (5)
- src/chrome/src/ui/locales/bn.js: Generated file
- src/chrome/src/ui/locales/hi.js: Generated file
- src/chrome/src/ui/locales/ru.js: Generated file
- src/chrome/src/ui/locales/th.js: Generated file
- src/chrome/src/ui/locales/uk.js: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| const chatMainTurn = async (chatMessages, chatOptions, requestContext) => { | ||
| const startedAt = Date.now(); | ||
| const result = await chatMainTurnRaw(chatMessages, chatOptions, requestContext); |
There was a problem hiding this comment.
Fixed in aafa249. The collector in _chatStreamWithCostAllowance now preserves the terminal reason from the provider done chunk (and its raw payload when present) and passes it into the result, so aggregateMessageCompletion can read result.finishReason instead of always receiving an empty reason on the primary streaming Ask path. The streaming providers that actually observe a terminal reason (OpenAI Chat Completions choice.finish_reason, Anthropic message_delta.delta.stop_reason, llama.cpp / Azure choice.finish_reason, Bedrock stopReason) now attach it to their done chunk. The Responses API has no per-generation stop reason, and the aggregation test already treats lifecycle status as not-a-reason, so no value is fabricated there. Verified: node test/run.js 1755 passed.
| break; | ||
|
|
||
| case 'run_complete': | ||
| setMessageCreatedAt(eventAssistantEl || currentAssistantEl, data?.endedAt, { replace: true }); |
There was a problem hiding this comment.
Confirmed fixed in 5b116dc: the synthesized run_complete in applyActiveRunState now includes data.endedAt: runUi.endedAt, so terminal restore uses the snapshot completion time instead of falling back to request-start/panel-open. The new behavioral test asserts the replaced sent time is retained across restore.
| break; | ||
|
|
||
| case 'run_complete': | ||
| setMessageCreatedAt(eventAssistantEl || currentAssistantEl, data?.endedAt, { replace: true }); |
There was a problem hiding this comment.
Mirrored in 5b116dc on the Firefox sidepanel; covered by the same behavioral restore assertion in the new test.
| msgEl.tabIndex = 0; | ||
| msgEl.title = t('sp.message_info.hint'); | ||
| msgEl.setAttribute('aria-expanded', String(msgEl.classList.contains('message-info-open'))); |
There was a problem hiding this comment.
Fixed in aafa249 with a semantic toggle button, per your suggestion:
- Each message with a timestamp now gets a real
<button type="button" class="message-info-toggle">as a sibling of the info row (outside the bubble, so no nesting inside the message links/buttons), carryingaria-expandedandaria-controlspointing at a stable row id (message-info-<n>). - The message div no longer receives
tabIndex = 0,aria-expanded, or a keydown handler; keyboard users activate the toggle button directly. - Bubble-click delegation is retained as a mouse convenience, and clicks on the button are excluded from it via the existing
messageInfoClickIsInteractiveguard. refreshOpenMessageInfoRowsrefreshes the button label instead of the div title; restored messages re-create/reuse the button throughmessageInfoToggleButton.
One deliberate design note vs. a role="button" on the bubble: a real <button> gives native Enter/Space handling, focus management, and correct AT announcements, which a div with aria-expanded alone cannot — this matches your recommendation.
| msgEl.tabIndex = 0; | ||
| msgEl.title = t('sp.message_info.hint'); | ||
| msgEl.setAttribute('aria-expanded', String(msgEl.classList.contains('message-info-open'))); |
There was a problem hiding this comment.
Mirrored in aafa249 — the Firefox sidepanel implements the same semantic toggle button (aria-controls/aria-expanded on a real button, stable row id, no tabIndex/aria-expanded/keydown on the bubble div, bubble-click delegation kept). Both builds covered by the new behavioral test.
| } | ||
| }); | ||
|
|
||
| test('sidepanels reveal persisted message info while verbose gates completion details', () => { |
There was a problem hiding this comment.
Addressed in aafa249 with a behavioral DOM test (message info toggles behaviorally through a semantic button, terminal replay, and restore in both builds). It extracts the real message-info functions from each sidepanel via vm and drives them with a small fake DOM, exercising: toggle via bubble click and via the semantic button, aria-expanded/aria-controls sync, applyMessageCompletion → dataset round-trip, terminal replay (run_complete endedAt replacing the sent time via setMessageCreatedAt(..., { replace: true })), and a restore round-trip that rebuilds a message from persisted datasets + the open class and asserts the completion timestamp and metrics are retained. The repo test runner is deliberately dependency-free ("pure Node, no framework"), so this follows the same vm-extraction pattern as the existing /print behavioral tests instead of adding jsdom.
…estore Terminal restore in applyActiveRunState synthesized a run_complete event without data.endedAt, so restored assistant messages fell back to the request-start timestamp instead of the actual completion time. Mirror the same fix in both Chrome and Firefox builds.
…antic info toggle Address the remaining Copilot review comments on PR webbrain-one#2802: - Stream collectors now forward the terminal finish_reason/stop_reason observed by providers (OpenAI Chat Completions, Anthropic, llama.cpp, Azure OpenAI, Bedrock) through the done chunk into aggregate results, so the verbose stop-reason pill appears in the primary Ask path. - Replace the focusable message div (tabIndex + aria-expanded on a bare div) with a real toggle button carrying aria-controls/aria-expanded and a stable row id, while keeping bubble-click delegation. - Add a behavioral DOM test that exercises toggle, persist/restore, and terminal replay for both builds.
|
Summary — all four Copilot review items are now addressed:
Verification: |
Summary
Motivation
Message bubbles currently provide no way to inspect when a message was sent or, in verbose mode, how the assistant response was generated. This implements the compact/verbose behavior requested in #2798 without coupling the UI to trace storage.
Design
message-info.jsseam normalizes provider usage aliases, aggregates multi-call assistant turns, and returns localized pill descriptors.Testing
npm run test:toolbar-guard— 33 passednode test/run.js— 1754 passednpm run test:security— 60 passedsrc/chromeas an unpacked extension in headed Chromium and verified click, Enter/Space toggle, normal/verbose rendering, restored binding, and clean console/page errorsCompatibility and risk
Closes #2798