Add status badge cutouts to avatars - #211
klopez4212 wants to merge 35 commits into
Conversation
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com> # Conflicts: # tests/fixtures/design-system/viewer.spec.ts
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
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. |
![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: fabfd3980f
ℹ️ 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".
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 accessibility finding in the new light-mode status colors (inline). Merge criteria: make the status boundary meet the existing 3:1 non-text target on supported surfaces and cover those mappings in the existing contrast guard. Live presence wiring remains out of scope.
Validation: source-only review of fabfd3980fdea11e05e5455651df2b7fc329e7dc against 804413b22c3866af4b6710d99b154a0414e3b142, plus committed screenshots; no builds/tests or live/native rendering executed. Existing journey shards passed, but they do not run the new design-viewer test. Browser measurements failed: channel-opening warm visibility was 105 ms versus <100 ms. I have not established that this diff caused it; required CI remains a separate merge gate.
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one new accessibility regression in the profile integration; restore non-color status distinctions on the collapsed avatar. The inline P3 fallback note is nonblocking.
The previous contrast finding is resolved on all four declared surfaces in both themes. Actual online/away/offline changes preserve the image subtree; no presence redesign is requested.
Validation: source-only primary review, independent review, and coordinator integration at 9b45af4ac7f00c66d35c36eb9e6f37b563fe963b against 41c43393894fa18408a3dc1a6de8420156263b09. Existing exact-head CI reports 11 successes and one native skip. No builds/tests or live rendering were executed by this review; app-backdrop paint remains unverified.
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: 0abf7d9708
ℹ️ 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".
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one remaining P2. The profile trigger's shared interaction-background feedback is suppressed; reproduction and the bounded fix are in this thread.
Reviewed head 0abf7d970832cbf6d4909646bac0607409db3db8 against base 41c43393894fa18408a3dc1a6de8420156263b09. Earlier status-contrast, non-color status-shape, and unnamed-profile fallback findings are resolved in the current implementation. The complementary lifecycle review found no additional material issue in the changed paths.
Validation: a review-local probe of the real built app reproduced the remaining defect in Chromium and WebKit, light/dark, and verified the cascade cause by removing only the override selector in the DOM. Contrast guard and diff whitespace check pass. Existing hosted checks are green; Windows native validation is skipped. No broad suite or native validation was rerun locally.
Merge criterion: restore shared hover/pressed/menu-open feedback and cover that rendered control. No additional presence feature work requested.
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: 4d26aa3e53
ℹ️ 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".
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com> # Conflicts: # src/bundled/profiles/ProfilePanel.tsx
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: 65b07f0abf
ℹ️ 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".
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: two P2 regressions in the newly added live-agent badge integration. Inline findings cover loaded-artwork resets on unknown/known transitions and visible presence that is absent from accessible names/descriptions. The previous profile interaction-fill defect remains fixed.
Reviewed head 65b07f0abff17402f541ad25ca9878a2eb0e9c2e against base 82bb3a631b96b92cd73940f16bcfb05fdaa7607e. Both independent delta-review lanes returned; these findings were separately challenged after focused reproduction.
Validation: 14 affected presence journeys pass in Chromium/WebKit; 48 focused component tests, the contrast guard and diff check pass. Additional review-local browser probes using the real AgentCard, AgentChoice, shared Avatar and CSS reproduce both findings in both engines; these probes control presence state at the external-store boundary, not through the relay. Existing byline coverage uses human authors, so its pass does not exercise the new agent contract. No production edits, live-account or native validation. Hosted CI was still running at the last snapshot; Windows native validation was skipped.
Merge criteria: keep loaded artwork stable across unknown/known presence changes, expose the visible agent status to assistive technology, and add regressions for those paths. Separately, I asked Wes to confirm the deliberate solid-fill and agent-byline direction changes; this review does not require reverting an authorized design decision. Align the presence documentation/coverage with that decision.
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: e615314335
ℹ️ 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".
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com> # Conflicts: # src/bundled/channels/ChannelSidebarItem.tsx # src/bundled/channels/ChannelSidebarRow.tsx
Enable the strict read-state fixture for the profile activity journey and wait for its actual frontier publication. Avoid duplicate known-status narration on the profile Info tab while retaining the portrait name when status is unknown and status labels on other tabs. Document the intentional solid-fill badge exception. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Carl, an automated reviewer, commenting via Wes’s GitHub account.
The findings in this review are fixed or explicitly superseded by the documented product direction. Rechecked during the PR takeover at 5004159. This dismissal is not an approval; required CI and independent/code-owner approval still apply.
|
Carl, an automated reviewer, commenting via Wes’s GitHub account. Addressed the outstanding threads and pushed No remaining code blocker from the integrated review. Exact-head pre-push checks and all 32 affected Chromium/WebKit journeys pass; validation scope and fail-then-pass evidence are in the PR description. Earlier Carl changes-requested reviews have been dismissed as resolved/superseded, not converted into approval. Hosted CI/DCO and required independent/code-owner approval remain the merge gates. |
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Bring in the existing read-state delivery-gate fix without losing profile integration. Wait for wheel completion before recording the channel anchor and clear the rich composer through its keyboard selection command. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Resolve AgentChoice against shared MenuRadioItem and ChoiceRow components. Preserve known-status names and admission disclosures; cover both after a presence transition. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com> # Conflicts: # tests/browser/agent-activity.spec.mjs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eefcec8c1
ℹ️ 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".
| it is not relabeled Offline. Message and thread bylines neither display presence | ||
| nor demand snapshots. This describes recent Buzz | ||
| session status in this community, not proof that a person is available or an | ||
| Profiles show Active, Away, or Offline with text, while avatar badges use |
There was a problem hiding this comment.
The reviewed commit message contains no Signed-off-by trailer (git interpret-trailers --parse returns no trailers), so a PR containing this commit cannot satisfy the repository's mandatory DCO policy. Add a sign-off from the commit's actual author before integration.
AGENTS.md reference: AGENTS.md:L153-L163
Useful? React with 👍 / 👎.
Signed-off-by: klopez4212 <klopez4212@gmail.com> # Conflicts: # tests/browser/agent-activity.spec.mjs
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Source review clear; not merge-ready. No actionable code/product/security findings in head b47f4fc97fa89167793062855a85316099859f1f against base 223028bbb59273dae9c109675b6b7f3141d13340.
- The shared avatar retains loaded artwork across status transitions; known presence is narrated by its owning control/image, unknown stays unbadged, and all consumers reuse the bounded session-owned presence directory. The documented solid-fill product exception is preserved.
- Existing CI run 36076581594 passed for these exact head/base pins. The JavaScript checkout log confirms synthetic merge
5580c0f813d282079520c3913ba17c563b25af50; 321 files / 3,513 tests passed. Hosted Chromium/WebKit journeys and required checks passed; Windows native validation was skipped. This source-only review ran no code or live visual/accessibility acceptance. - Remaining merge gate: GitHub currently reports
mergeable=false,mergeable_state=dirty, and no merge commit at the same pins. Resolve that conflict state; any changed head/base needs fresh integration CI. Not every agent surface has a production-broker browser journey. I contributed earlier fixes, so this COMMENT is not independent/code-owner approval; obtain required independent approval before merging.
Signed-off-by: klopez4212 <klopez4212@gmail.com> # Conflicts: # src/app/shell/ProfileButton.tsx # src/bundled/profiles/ProfilePanel.tsx # tests/browser/message-navigation.spec.mjs # tests/browser/presence.spec.mjs
Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 native-Linux accessibility defect, detailed inline. Reviewed ab2421cb6246f9d70595fdb4069a79b1d0a8835d against df7b7e7f45739f3e06e12d81623385701acdc51d. The prior merge-conflict gate is resolved; the merge preserves main’s account/status behavior, and the DM polling change retains the authors predicate. The Linux gap was missed in my earlier review, not introduced by this merge.
Merge criteria: expose known presence once at each of the three affected controls through a WebKitGTK-compatible accessible name/description, retain silent unknown state, and update the corresponding accessibility assertions. The intentional solid-fill design does not need changing.
Existing CI run 36107597476, attempt 1, passed for these exact head/base pins: 660 Chromium/WebKit browser cases with zero retries and 3,705 unit tests. This was source-only review on isolated Blox, not a new test run or native accessibility validation. No native-window, live-relay or screen-reader acceptance was exercised; the optional design-viewer suite is outside hosted coverage and its author-reported WebKit toast-focus failure remains unverified here. I contributed earlier fixes, so this is not independent/code-owner approval.
| aria-current={ | ||
| selected === channel.id && !draftSelected ? "page" : undefined | ||
| } | ||
| aria-description={presenceDescription} |
There was a problem hiding this comment.
[P2] Use a presence description that WebKitGTK exposes
With known presence, this DM row puts the status only in aria-description; its nested avatar has alt="" and is therefore aria-hidden. The same pattern appears in MessageRow.tsx:209–211 and ActivityAccessory.tsx:155–157. Native Linux users therefore lose the accessible presence value on these three controls, although the badge is visible.
This is specifically the WebKitGTK/ATSPI path, not a general Firefox or Safari defect. At WebKit revision ceba48c3, the ATSPI description getter consumes accessibilityText(), whose help-text producer maps aria-describedby but not aria-description. The latter goes through extendedDescription(), which the Cocoa wrapper consumes, not ATSPI. Linux is a documented native target; the native CI job installs libwebkit2gtk-4.1-dev.
Use aria-describedby referencing status text, or include status in the owning accessible name, at all three sites. Preserve the single owner and omit presence when unknown. Update the tests to assert the resulting accessible name/description rather than merely the attribute’s presence.
Evidence is source-level: I did not run a native accessibility-tree/screen-reader probe or identify a specific distro’s WebKitGTK release. Green jsdom attribute assertions and functional browser journeys do not establish this native mapping.
3 participants
Summary
Conflict resolution:
00a50264Merged current main
1e15d5d3(#212 menu/popover changes) with PR head8035c372. ResolvedAgentChoice.tsxby retaining main’s shared MenuRadioItem/ChoiceRow/small-avatar layout and the PR’s live badges and accessible status/admission disclosure. Added admission-label assertions across unknown → known presence. Independent source review passed for the resolution and overlapping auto-merges.Validation: all 14 cases in the complete
agent-activity.spec.mjsandnew-message.spec.mjspass in Chromium/WebKit, two workers, zero retries (41.4s local). Run was on the integration before commit; the pre-commit hook only reformatted AgentChoice. Exact-head mandatory push hooks pass TypeScript, 654 related unit tests (53 files), design types and guards. Prior broader evidence is recorded below, not claimed as rerun at this head. No browser cases added/removed by conflict resolution; no toast repair or full viewer rerun. GitHub reports MERGEABLE (no conflicts) and DCO is green; hosted CI completion remains pending at this new head.Previous validation:
8035c372Carl, an automated contributor, updated this PR on Wes’s behalf.
be53d8e1, including test: isolate direct-message delivery gate from read-state publications #233’s existing fix for read-state publications stealing the held direct-message delivery callback. Independent source review verified that main’s profile Message/status/Memories changes and the PR’s presence accessibility changes survive.scrollendbefore recording the reading anchor; clear the rich composer using its select-all/backspace commands and assert it is empty before mention completion. The hosted WebKit trace shows the last 25px of wheel movement occurring after the old baseline; the local Chromium trace showsfill("")leaving the first mention intact. Independent reviewers checked both repairs.new-message,message-navigation,agent-activity,profiles,presence,avatar-shapes,dm-labels. Run used the staged integration tree, verified byte-for-byte equal to committedeef19a3e; the sole later change at8035c372updates the separate viewer test’s exact avatar count. New-message cases took 3.7s / 4.8s and the reading-anchor case 2.1s / 2.7s (Chromium / WebKit). Earlier integration run: 35/36 pass, 1.9m; different scopes, not a speedup comparison. No browser cases added or removed in this repair.eef19a3e, all 94 design unit tests, app build and both design builds also pass.8035c372(1.3m, one worker, zero retries). All avatar cases pass. Repaired the existing exact identity count to include this PR’s six new Morgan specimens (5 → 11); it failed in both engines before the correction. The remaining failure is WebKit toast modal focus attests/fixtures/design-system/viewer.spec.ts:1622; it also failed in the preceding full viewer run. This is outside the avatar repair and has not been diagnosed or suppressed. The design-viewer browser suite is not part of the hosted functional matrix.Not yet merge-ready: hosted CI at this head is running; the separate toast-focus failure needs owner disposition. GitHub currently reports approval, but no new approval or merge was performed by this takeover. No live-account or native-window validation; Windows CI is skipped. Local macOS evidence does not establish hosted Linux success.
Earlier takeover and screenshot-cleanup evidence
Validation
Carl, an automated contributor, updated this PR on Wes’s behalf.
At
50041597(includes main through7de47085):05917094, immediately before removing two unsupported RTLexactoptions from the test only.agent-activity.spec.mjs,presence.spec.mjs, andprofiles.spec.mjspass in Chromium/WebKit on the exact pushed head (86 seconds local wall time). No browser cases added/removed.Hosted CI/DCO at the new head and the required independent/code-owner approval remain merge gates. No live-account or native-window validation was performed in this takeover. The full hosted matrix supplies broad integration coverage.
Screenshot cleanup follow-up
At
918ff0fa(includes main throughf6caa83f):