Add centered thinking pills to agent avatars - #351
Conversation
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 025d163713
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| snapshot?.typing.some( | ||
| (entry) => | ||
| entry.agent === agentPubkey && | ||
| (!channelId || entry.channelId === channelId), |
There was a problem hiding this comment.
When an agent types in a thread, this channel-only predicate marks every avatar for that agent in the channel as thinking, including the top-level timeline and sibling threads. This conflicts with docs/presence.md's displayed-conversation scoping and with the existing exact threadRootId matching in TypingIndicator and ActivityAccessory; pass the displayed thread root into AgentAvatar and require the typing entry to match it exactly.
AGENTS.md reference: AGENTS.md:L39-L42
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord's automated source review via Wes's account.
Two actionable findings, inline: the 88px canvas scales fallback initials down with the artwork, and the new thinking state has no accessible equivalent in clickable message avatars or the known-presence Profile Info view. Keep the intended centered-pill design; repair text sizing and state exposure in their existing owners.
Reviewed head: 025d163713c328a5bcc0bc7aaaadc89d9d0f017c
Base / merge base: 96f074343d580c0e27c0cb11da1153b5ff867c20
Scope: complete 36-file diff; shared badge geometry/animation, AgentAvatar sizing and subscription cleanup, activity freshness/expiry and plugin-owned capture, message/DM/card/profile callers, presence/status changes and regression-test source. The DM sidebar retains a separate accessible channel-working marker. One PR commit has a DCO sign-off trailer.
Validation limits: source-only; no installs, tests, builds, app launches or PR-code execution. Typography and accessibility findings are source deductions, not observed browser/screen-reader runs. Native appearance, live agent-turn behavior and human acceptance remain unverified. One exact-head hosted CI snapshot at 2026-09-28T17:54:27Z: CI required failed; Chromium shard 1/3 and all three WebKit shards failed; Chromium 2/3 and 3/3, JavaScript, Rust/tool integration, browser measurements, security and DCO passed; Windows was skipped. Job logs were unavailable through the attempted API reads; annotations only report exit code 1, so I am not attributing those failures to these findings. No polling or reruns.
This is non-blocking COMMENT feedback, not approval or merge authorization.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6dc0c2fbe
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| width: 22px; | ||
| height: 22px; | ||
| transform: translate(-50%, -50%); | ||
| background: var(--status-online); |
There was a problem hiding this comment.
For every online agent avatar at rest, this paints the entire replacement badge with --status-online (Green 11). The presence contract in docs/presence.md and DESIGN.md requires a Green 10 center inside a 1px Green 11 outline, but AgentAvatar suppresses Avatar’s standard badge while this replacement is visible, so no inner layer restores that center. Recreate the two-layer online badge for the available endpoint before morphing it into the thinking pill.
AGENTS.md reference: AGENTS.md:L39-L42
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source follow-up via Wes’s account
No actionable follow-up findings. The two issues from the prior review are addressed in this source revision:
- Fallback typography:
AgentAvatarnow renders the shared avatar at destination size. Only the badge canvas scales; the cutout retains its normalized coordinate mapping. The new browser regression compares rendered glyph bounds at 22px and 40px, rather than relying on computed font size. - Thinking accessibility: the clickable message-avatar button and decorative Profile Info portrait now expose thinking through their owning button/region descriptions. The added component tests cover idle → working → idle while preserving decorative artwork and existing presence semantics.
Reviewed the complete 16-file follow-up from 025d163713c328a5bcc0bc7aaaadc89d9d0f017c, its affected callers/styles, and regression-test changes for defects introduced by these repairs. This is not a new review of settled feature decisions.
Pins: head b6dc0c2fbe618d1dfe5ee8f7db0bd89adfefe401; base 96f074343d580c0e27c0cb11da1153b5ff867c20. Pinned Git objects only; no dirty checkout inputs.
Validation limits: source-only; I did not run tests, builds, installs, PR code, or the app. One exact-head hosted CI snapshot showed JavaScript, Rust/tool integration, browser measurements, security, DCO, and Chromium shards 2/3 and 3/3 passing; Chromium 1/3 and all three WebKit shards were still running, and Windows native validation was skipped. Both PR commits contain DCO sign-offs. Full CI completion, native/live-agent behavior, assistive-technology behavior, and attended human acceptance are not established by this review. The PR’s native visual-check statement is author-reported, not independently reproduced.
Non-blocking COMMENT only — not approval or merge authorization.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source follow-up via Wes’s account
No actionable follow-up findings. This merge reconciles the accepted human-byline behavior with the newer observed-presence journey without restoring optimistic badges or changing the settled centered-pill design.
- In
tests/browser/presence.spec.mjs:255–276, the profile is opened and its initial Active evidence is awaited before emitting Away. That matches the existing ownership: human message rows no longer request presence; the explicit profile does. The live Away assertion and production demand-scoped delivery checks remain. - At lines 536–610, the human self-message consistently expects no badge during pending, confirmed Away, and Offline states. The account avatar, selected radio, held-publication gate, socket publication and reload checks still verify the observed status.
- Compared the previous reviewed feature patch at
b6dc0c2fbe618d1dfe5ee8f7db0bd89adfefe401against its base with this head against its new base. The substantive new feature-patch difference is the presence journey above;avatar-loading.spec.mjsdiffers only in indentation. No new production patch was added by this integration. The earlier fallback-typography and thinking-description repairs remain in place.
Pins: head 65a5334b01b425c81871099483f4e0fc828c793e; base c6b47a5837fd8912dc84d98bb959714a23d70818. Reviewed immutable, Git-blob-verified source; no dirty checkout inputs.
Validation and publication limits
- Source-only: no PR code, tests, installs, builds, app or browser flow executed. Inspected the affected presence/Avatar/profile callers and lifecycle; this merge adds no focusable control or error/retry focus transition. Runtime, native appearance, assistive-technology behavior and attended human acceptance remain unverified.
- One hosted check snapshot for this head: browser measurements, Chromium shard 3/6, security checks and DCO passed; JavaScript, Rust/tool integration and eleven browser shards were still running; Windows was skipped. This is not a completed CI verdict and was not polled.
- Public repository: inspected the current description, full feature patch and all three PR commit messages for publication hygiene. No concrete internal URL, deployment/workspace identifier or secret was found in those inspected surfaces; legitimate authorship/sign-off metadata is retained. The description has no attached images or other media.
- Description follow-up (non-blocking): it still says “keeping this PR in draft” although the PR is ready, and omits the repository-required browser-case accounting/fail-then-pass evidence. This integration adds/removes zero cases; the cumulative feature adds two (rendered fallback geometry and centered-pill motion), both browser-specific. Update the description with the actual evidence and remaining checks; this source review does not establish the readiness attestation.
Non-blocking COMMENT only — not approval or merge authorization.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25dfe43718
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| if (time < 1) frame = requestAnimationFrame(draw); | ||
| } | ||
| frame = requestAnimationFrame(draw); |
There was a problem hiding this comment.
When an avatar mounts with the common thinking=false state, from and to are both zero, but this still schedules frames for 300 ms and rebuilds the hundreds-of-points badgeCutoutPath on every frame. This happens for every mounted agent avatar, including offline/unknown avatars where enabled is false, so opening a conversation with several agent messages creates avoidable main-thread work and can jank the initial render. Draw the endpoint once and stop when from === to rather than entering the animation loop.
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source follow-up via Wes’s account
No new actionable findings in this bounded merge-integration follow-up. The previously reviewed production feature patch is unchanged apart from diff context/line offsets; this does not reopen the settled avatar design or supersede the prior review’s validation caveats.
src/bundled/agents/AgentLibrary.test.tsx:2–3adds the existingstubAvatarBrowserApis()setup to the newly integrated library test. ItsbeforeEachsupplies the browser interfaces reached throughAgentLibrary → AgentCard → AgentAvatar → ThinkingBadge(ResizeObserverandmatchMedia); the identity-card, public-key disclosure, Escape-dismissal and disposal assertions remain intact. This is jsdom compatibility, not geometry/motion validation.- Compared the covered head
65a5334b01b425c81871099483f4e0fc828c793eagainst its basec6b47a5837fd8912dc84d98bb959714a23d70818with this head against its base. Beyond that two-line test setup, the feature-patch additions/deletions differ only by removal of an obsolete author-header presence comment. The presence journey retains upstream’s smaller fixture and Away color, still waits for the explicit profile’s initial presence evidence before emitting Away, and preserves the badge-free human-byline assertions. Profile and message-row feature edits remain unchanged. - Traced mount/unmount cleanup and the surrounding disclosure and inventory error/Retry paths. This delta changes neither production focus ownership nor success, cancellation, or error/retry transitions; their runtime behavior is not certified here.
Pins: head 25dfe43718695568be9d74942df9f116c3d44126; base 61da2662d74aeea46ea8344fb4dc6e247baaab68. Immutable source archive verified against 1,707 regular Git blobs; no dirty checkout inputs.
Limits: Source-only: no PR code, tests, installs, builds or app/browser flows executed. Native appearance, motion, assistive technology, actual focus restoration and attended acceptance remain unverified. One hosted-check snapshot showed JavaScript and eight browser shards still running; four browser shards, browser measurements, Rust/tool integration, security checks and DCO had passed; Windows validation was skipped. No CI polling or completed-CI claim.
Public material: Repository is public. Inspected the current description (no attached images/media), all four PR commit messages and the merge-specific changes, and scanned the cumulative feature patch for internal links, deployment/workspace identifiers and secrets. No new concrete publication-hygiene finding in those surfaces. Prior description/validation-accounting caveats remain; this is not a readiness attestation.
Non-blocking COMMENT only — not approval or merge authorization.
…t-update-drafts * commit '0a4982797f38164d75e3e8f48e58fabb9dd59e66': (66 commits) Show saved local and relay inventory while retaining existing import controls (#286) feat(channels): edit channel details with confirmed saves (#369) test(channels): discover the hoverable width for activity corners (#416) Fix flaky WebKit menu focus browser test (#409) Test Goose connections and fix Pi test false failures (#383) feat: open threads with verified newest-first windows (#154) Add agent conversation context selection (#382) test: keep behavioral coverage without cosmetic matrices (#410) Fix reading position and composer caret on channel return (#411) fix(channels): prevent clipped activity rows and remove separators (#377) ci: publish signed macOS updater artifacts in prereleases (#387) feat(messages): add jump to latest controls (#374) Align reply summaries with message content (#408) Add centered thinking pills to agent avatars (#351) Keep focus where the user moved it when a menu finishes closing (#355) Browse legacy identities without a destination and review text before cloning (#285) Show separate identity cards and prevent duplicate imports (#225) Polish message and thread spacing, grouping, and typography (#364) Remove the Away avatar badge stroke (#395) fix(profiles): hide activity on human profiles (#391) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>
* origin/main: (25 commits) Show saved local and relay inventory while retaining existing import controls (#286) feat(channels): edit channel details with confirmed saves (#369) test(channels): discover the hoverable width for activity corners (#416) Fix flaky WebKit menu focus browser test (#409) Test Goose connections and fix Pi test false failures (#383) feat: open threads with verified newest-first windows (#154) Add agent conversation context selection (#382) test: keep behavioral coverage without cosmetic matrices (#410) Fix reading position and composer caret on channel return (#411) fix(channels): prevent clipped activity rows and remove separators (#377) ci: publish signed macOS updater artifacts in prereleases (#387) feat(messages): add jump to latest controls (#374) Align reply summaries with message content (#408) Add centered thinking pills to agent avatars (#351) Keep focus where the user moved it when a menu finishes closing (#355) Browse legacy identities without a destination and review text before cloning (#285) Show separate identity cards and prevent duplicate imports (#225) Polish message and thread spacing, grouping, and typography (#364) Remove the Away avatar badge stroke (#395) fix(profiles): hide activity on human profiles (#391) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentsPage.test.tsx # src/bundled/agents/AgentsPage.tsx
Summary
Validation