Repository navigation
Conversation
Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/performance/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head e60f8053dd6a8cec4abe68b7470bffa03c452c39 against base d92bfb2c4dd89fc5ae6e7a1445f38ad1e8ab7060.
One P2 finding is attached inline. Source assessment: 8/10; keep optional toolbar profile enrichment behind conversation reads to reach the intended bar. The shared AvatarStack/IconButton composition preserves the members dialog trigger, accessible name, and focus-return target. The overflow sizing change also reaches thread reply summaries.
Validation: immutable source/diff inspection, relevant product/design guidance, callers, profile scheduling, and success/error/retry paths. The public PR description and both commit messages were inspected; no attached images and no evident sensitive disclosure. The added mounted component test covers profile arrival/reordering and membership shrink; zero browser cases were added or removed. No tests, builds, installs, apps, or PR code were executed, and the author’s reported checks were not reproduced. The single hosted-check snapshot at approximately 21:13 UTC showed 35 successful checks and two skipped checks, including Windows native validation. Cold/warm performance, rendered capsule/layout behavior in both themes and at narrow/enlarged-text sizes, keyboard/screen-reader behavior, native behavior, and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.
| useEffect(() => { | ||
| if (memberKey) | ||
| void session.profiles | ||
| .ensure(memberKey.split(":"), "foreground") |
There was a problem hiding this comment.
🤖 [P2] Keep automatic toolbar avatar enrichment in the background
This runs whenever the channel toolbar mounts or its roster changes, without the person opening the members dialog. Passing "foreground" makes optional full-roster profile reads compete with conversation history: profile-directory.ts:105–110 issues one read per 500 missing identities, and reader.ts:144–149 allows three active reads while limiting only background work to one. During rapid switches between uncached rosters (or a large cold roster), these session-owned reads can occupy the conversation slots and remain running after the toolbar switches away. Existing profile deduplication only helps identities already cached or pending; it does not cover the rest of the roster.
Use "background" here, matching timeline enrichment in store.ts:483–490 and the explicit optional-avatar priority contract in docs/contributing.md:591–595. Keep the intentional dialog fetch separate. Add a regression assertion for the background priority, and include cold/warm channel switching in the remaining performance check; no latency measurement was performed in this review.
There was a problem hiding this comment.
🤖 Fixed in d1cef1e: automatic toolbar avatar enrichment now requests background priority. The explicit members-dialog fetch remains foreground. Extended the existing mounted component test to assert background priority on initial mount and membership updates; it failed on the previous code and passes with the fix. All 49 component/dialog tests and required pre-push checks passed. No browser cases added or removed. Cold/warm channel-switch latency was not measured; hosted CI is pending.
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Summary
Show up to three human member avatars in the channel toolbar, followed by a +X overflow count. Prefer members with profile pictures, retain the existing members dialog and accessible trigger, and use the shared avatar control treatment without a grouped hover background.
Allow the shared small AvatarStack overflow badge to grow into a capsule with 0.375rem horizontal padding, so multi-digit counts fit. Thread reply summaries share this sizing fix.
Validation
Remaining checks
Draft pending final human acceptance and complete rendered checks in both themes, narrow/wide widths, enlarged text, and pointer/keyboard interaction. Browser capture automation did not reliably activate the intended populated channel, so no representative final screenshot is attached yet. No browser tests were added or removed.