fix(desktop): restore avatar spacing and stack clipping - #7283
fix(desktop): restore avatar spacing and stack clipping#7283klopez4212 wants to merge 3 commits into
Conversation
Move remote-agent provenance beside sidebar name adornments, inset presence badges for squircle avatars, and restore radial overlap cutouts across shared avatar stacks. Co-authored-by: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <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. |
Before / after: DM sidebar provenance placementThe cloud is retained for the remote agent. It now shares the name-adornment area used by people’s status emoji, leaving trailing row space to unread state and hover actions.
|
🔐 Codex Security Review
|
![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: c6c75a400d
ℹ️ 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".
| key={normalizedPubkey} | ||
| style={{ zIndex: index + 1 }} | ||
| style={{ | ||
| ...(index < visibleMembers.length - 1 && { |
There was a problem hiding this comment.
When a channel has more than three members, the third visible avatar is still followed and overlapped by the +N counter, but this condition only masks avatars followed by another visible member. The resulting final avatar/counter seam lacks the radial cutout restored for every other overlapping stack; use the already computed stackItemCount so the last visible avatar is masked whenever an overflow item follows it.
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: 6cf514ed90981d1db4acdb97a272543ecca83993..c6c75a400d49859883f86967b907dc6c75792069 (exact head c6c75a400d49859883f86967b907dc6c75792069)
Risk: medium — shared, user-visible avatar/provenance geometry across several Desktop surfaces, but no persistence, relay, IPC, identity, or release contract change.
Behavior/contracts traced: remote-agent recognition through delayed directory readiness; provenance placement beside name/status adornments; wrapper-owned radial overlap masks across team, channel-management, group-DM, thread-summary, and system-message stacks; mixed circle/squircle footprints; profile presence-badge inset; unread/action spacing; accessibility semantics.
Findings: no blocking or non-blocking defect found. The restored cutouts follow the overlapping foreground footprint without changing the underlying avatar shape, and provenance/readiness behavior remains stable at both immediate and delayed directory availability.
Author action: none.
Verification owner: CI/release gate for the unrelated smoke failure; Desktop test owners for any future exhaustive visual matrix.
Validation at exact head:
- Desktop
pnpm typecheck: passed. - TypeScript + Vite E2E build: passed.
- Targeted rendered E2Es: 6/6 passed after rebuild, covering team-card radial overlap, profile ingress/presence inset, capped group-DM stack, cloud provenance at 0 ms and 6000 ms directory delay, and contiguous-arrival/system-message avatars.
- Additional targeted suites passed for team/agent-profile, group-DM/channel-management/arrival-system-message, and cloud provenance; assertions inspect semantic ownership plus computed masks/bounds and targeted screenshots rather than class names alone.
- GitHub Actions run
33777697627: affected Desktop core, Windows build, macOS build, integration E2E, and smoke shards 1/2/4 passed. Smoke shard 3 failed only inprofile-custom-emoji-status.spec.ts:196after all retries, with five other tests marked flaky; those files and status/composer/overscroll contracts are outside this PR's avatar/provenance diff. This is an external gate confidence gap, not an author-actionable PR defect.
Manual/native evidence: supplied before/after and rendered DM sidebar/header/profile states were inspected, including delayed directory readiness. No exhaustive native light/dark, dense-sidebar, OS-scale matrix was run.
Residual risk: untested platform/theme/scale combinations and absence of broad pixel-diff baselines for every restored stack. Targeted rendered inspection, screenshots, semantic assertions, and computed geometry checks materially bound that risk.
— :bot: Jude’s code review agent
Fresh visual verificationCaptured from PR head Cloud placementThe remote-agent cloud now sits beside the DM name—the same adornment area used by status emoji—rather than in the row's trailing action/status space.
The same provenance remains visible in the agent profile.
Squircle status insetThe presence badge is inset from the squircle's lower-right edges instead of hanging over the corner.
Avatar-stack clippingOverlap cutouts are restored in both group-DM and team stacks, so each foreground squircle has a clean clipped boundary rather than painting over the previous avatar.
|
Anchor the squircle status cutout to the avatar edge while independently insetting the visible dot. Assert that both human and agent sidebar masks render open clip-path notches. Co-authored-by: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
…ar-ui Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6..baa67d9ecf15fa7ac22c63f97d3568245cd25316 (exact live head baa67d9ecf15fa7ac22c63f97d3568245cd25316)
Risk: medium — this changes shared, user-visible avatar/status and overlap-mask geometry across sidebar, profile, channel, team, and DM surfaces.
Blocking finding: desktop/src/features/channels/ui/ChannelMemberAvatarStack.tsx:47-59 omits the radial cutout on the last visible avatar whenever an overflow item follows it. The component computes stackItemCount = visibleMembers + overflow, but gates the mask with index < visibleMembers.length - 1. In the existing four-member fixture, avatar 3 is immediately overlapped by the negatively margined +1 circle (:71-79) with no cutout, leaving the channel-management overflow seam inconsistent with the clipping this PR restores. Analogous team/header/DM stacks account for the overflow item. The existing E2E at desktop/tests/e2e/channels.spec.ts:2730-2745 renders exactly 3 + +1 but asserts only count/text, so it passes despite the defect.
Author action: include a following overflow item in the mask predicate (for example, compare against stackItemCount - 1) and extend the four-member channel-management E2E to assert masks on the first and last visible avatars and no mask on +1, matching the regression shape already used for group-DM stacks at channels.spec.ts:1355-1370.
Other traced behavior: no further material defect found. Human/agent status framing keeps the squircle cutout edge-anchored while insetting only the visible dot; circle geometry remains unchanged. Focused mutation evidence showed the new open-notch assertion fails when the former folded-closed agent geometry is restored. Profile/sidebar/cloud-provenance/agent-profile consumers and inspected Playwright artifacts were coherent.
Validation at clean exact head: pnpm typecheck passed; full Desktop pnpm test passed (6236/6236); pnpm build:e2e passed; targeted rendered channels tests passed (3/3). The green four-member channel-management fixture demonstrates the stated coverage hole rather than clearing it. Exact-head CI had Windows and macOS builds, relay-backed Desktop integration, DCO, Semgrep, and zizmor green; Desktop Core and smoke shards remained in progress at the final snapshot.
Manual/native evidence: focused Chromium/Playwright rendered artifacts were inspected. A complete native WebKit × theme × zoom/density matrix was not run; this is residual verification risk, not a second author-actionable defect.
Verification owner: author for the predicate fix and biting regression; reviewer/CI for fresh-head affected gates. Any new head invalidates this review until its delta is checked.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6..baa67d9ecf15fa7ac22c63f97d3568245cd25316 (exact head baa67d9ecf15fa7ac22c63f97d3568245cd25316)
Risk: medium — shared, user-visible avatar/status geometry across several Desktop surfaces.
Blocking defect — the channel-management overflow seam is still unmasked. ChannelMemberAvatarStack includes the overflow chip in stackItemCount, but applies a radial mask only while index < visibleMembers.length - 1 (desktop/src/features/channels/ui/ChannelMemberAvatarStack.tsx:18-26,47-59). In the rendered four-member case, three avatars are followed by an overlapping +1 chip (:71-79), yet the third avatar receives mask-image: none. The foreground overflow circle therefore overlaps without the radial cutout this PR restores. Equivalent team/header/DM stacks compare against total stack-item count and correctly mask the final visible avatar when overflow follows. The existing channel-management E2E renders this exact 3 + +1 shape but checks only count/text (desktop/tests/e2e/channels.spec.ts:2730-2745), so its green result does not cover the seam. This also matches the still-open thread discussion_r3926472247.
Author action: include the overflow item in the mask predicate (for example, compare with stackItemCount - 1) and extend the four-member E2E to assert masks on the first and last visible avatars and no mask on +1, matching the group-DM regression shape at channels.spec.ts:1355-1370.
Verification owner: author for patch and regression; this reviewer for fresh-head delta and affected gates.
Changed-head validation: the new squircle-status change correctly separates edge-anchored cutout geometry from visible-dot inset geometry; circle callers remain unchanged. At exact head, Desktop typecheck passed, full Desktop unit tests passed 6236/6236, E2E build passed, and targeted rendered channels tests passed 3/3. The channel-management fixture's pass demonstrates the coverage hole above, not correct seam geometry. Product/UI review found no additional defect; the remaining native WebKit/theme/zoom matrix is a confidence gap only. CI was still running at integration time; completed Windows/macOS builds, DCO, Semgrep, and zizmor were green.
— :bot: Jude’s code review agent
Chessing234
left a comment
Chessing234
left a comment
There was a problem hiding this comment.
in the avatar-stack e2e helper, both participants.first() and participants.last() assert not.toHaveCSS("mask-image", "none") — but the implementation only masks entries with a follower (index < count - 1), so the last overlapping participant should be toHaveCSS("mask-image", "none") (the system-message assertion later in the pr already gets this right). as written that helper will fail or is testing the wrong thing.
Chessing234
left a comment
Chessing234
left a comment
There was a problem hiding this comment.
retracting my earlier request-changes: with stackItemCount including the +N chip, the last overlapping participant still has a follower, so both ends having a mask is correct. cutouts look good.
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 avatar-stack defects. Reviewed head baa67d9ecf15fa7ac22c63f97d3568245cd25316 against base b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6.
P2: Scale the cutout with the actual avatar/overlap geometry
TeamIdentityCard.tsx:173–180 adds a fixed 32px radius and +8px center offset to avatars sized/spaced with rem-based h-14 w-14 -ml-5. Desktop Cmd +/- changes the root font size from 12 to 24px while keeping native webview zoom at 1 (useWebviewZoomShortcuts.ts:11–15,83–87,103–112), so the new mask does not follow the portraits.
Source-derived witness: show a team with two image-backed members and zoom to 75%. Each avatar is 42px wide and the next begins at x=27, but the first avatar’s transparent disk begins at x≈18.32. That erases almost 9px before the next portrait (over 7px even allowing its existing outline). At 150%, the next starts at x=54 while the cutout begins at x≈60.32, inside the foreground overlap, so it no longer separates the seam. Base did not have this radial mask.
Derive mask radius and center from the same dimensions/overlap as the stack. The new header, channel-member, thread-summary and grouped-system masks repeat the rem/px mismatch. The DM intro uses fixed 60px avatars with rem overlap, so do not blindly convert every constant to rem. Cover the supported small/default/large zoom geometry, not merely mask-image != none.
P2: Include the overflow chip when deciding which channel-member avatar to mask
ChannelMemberAvatarStack.tsx:53–56 compares against visibleMembers.length - 1, even though stackItemCount includes the overlapping +N chip. Open channel management for four members: it renders three avatars plus +1, but avatar 3 has no radial mask despite having a follower. The first two seams get the restored cutout and the final seam does not. The production Members row passes the full member list (ChannelManagementSheet.tsx:800–810).
Use the total rendered stack count for the predicate, retaining an unmasked final avatar when there is no overflow. Extend the existing four-member fixture (channels.spec.ts:2730–2745) to assert participant masks and the unmasked overflow chip. This corroborates the existing exact-head finding and is incomplete restoration, not loss of a previously correct base cutout. The retracted group-DM test complaint is unrelated: that test’s last participant correctly remains masked because +1 follows it.
Scope and validation: source-only review of the changed stack surfaces, profile badge consumers, sidebar provenance/readiness and E2E seams. No additional actionable defect established in the badge-center override or provenance relocation. The small-avatar clip helper tests the notch, not the independently moved dot; a dot-bounds assertion would improve that coverage. Exact-ref cached blobs were checked against Git tree hashes. No checkout, build, test, browser rendering or PR-code execution; other reviewers’ runtime claims are external evidence, not verification performed here. Backend, mobile and unchanged media-loading internals were outside this UI-only diff’s review boundary.
4 participants
Summary
Related issue
N/A — no matching open issue or PR found.
Testing
c6c75a400d49859883f86967b907dc6c75792069:desktop-check,desktop-typecheck, file-size checks, and all 6,110 desktop tests passed