fix(desktop): show channel members before joining - #6852
Conversation
Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-authored-by: Jitter <2b2e6415e748c35180847a37aadae06a11e30dddc76b784e7fd9354c4eb42e7a@buzz.block.builderlab.xyz>
Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-authored-by: Jitter <2b2e6415e748c35180847a37aadae06a11e30dddc76b784e7fd9354c4eb42e7a@buzz.block.builderlab.xyz>
The composer Buzz chip smoke test forced the sent message row's container to 220px with a one-shot inline style, then measured line fragments in a separate evaluate. When the relay echo re-creates the optimistic row between those steps, the style is lost and the chip never wraps, failing CI (all three attempts on Desktop Smoke E2E shard 3). Apply the width and measure atomically inside an expect.poll, and re-apply it when reading fragment rects for the tooltip checks, so a late re-render can no longer strand the measurement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 52621c09bea503f4d5860030dfabfaf9ade71bfa..edc43a7077e5f3ae10d67c01c9f1179a0323569c (exact head edc43a7077e5f3ae10d67c01c9f1179a0323569c)
Risk: high — this adds a new pre-join path into a shared roster whose existing controls include destructive community moderation and managed-agent lifecycle/configuration actions.
Blocking: the pre-join roster is not read-only
The new header correctly renders only member count + Join for an unjoined open channel, and MembersSidebar now gates add/search discovery on channel?.isMember === true. However, the reused member cards still receive other action capabilities without the same membership fence:
- Community owner/admin moderation derives solely from relay role (
desktop/src/features/channels/ui/useMembersSidebarModeration.ts:18-29) and is passed through to human rows without checkingchannel.isMember(desktop/src/features/channels/ui/MembersSidebar.tsx:449-460,651-680). That exposes Ban/Timeout actions in the newly reachable pre-join roster. - A roster bot found in the viewer's managed-agent directory receives lifecycle/configuration actions regardless of channel membership (
desktop/src/features/channels/ui/MembersSidebar.tsx:464-529,623-690;desktop/src/features/channels/ui/MembersSidebarMemberCard.tsx:151-161,373-397). Start/Stop/Restart, Manage agent access, and eligible View activity remain reachable before Join.
Those broader authorities may be valid elsewhere, but this PR explicitly promises a read-only pre-join roster with management actions member-only. The new entry point violates that contract and can surface destructive controls before the viewer joins.
Author action: derive one explicit read-only fence from channel.isMember !== true and apply it to every non-profile row/bulk action and related action-data query—not only add/search. Cover moderation, managed-agent lifecycle/access, View activity, remove/role/add, and any bulk controls. Add a deterministic regression from an unjoined open-channel journey that seeds a community-admin viewer plus owned managed/relay bots, asserts no row menus or action controls before Join, and proves the authorized controls appear after Join. Profile opening may remain if intentionally part of roster reading.
Verification owner: author for the patch and biting Playwright regression; reviewer re-review at the next immutable head.
Traced contracts and non-blocking observations
- Desktop fetches the exact channel's kind
39002roster through the authenticated relay query, then performs bounded kind-0 profile enrichment (desktop/src-tauri/src/commands/channels.rs:142-207). In the inspected relay/DB call chains, open-channel roster visibility is community-scoped and private/DM channels are not admitted through the open-channel path; no cross-community path was found in those searched paths. - Header count and the five-minute roster cache can temporarily disagree after an unobserved third-party membership change. This is a product-truth/freshness gap rather than a separately established PR-caused defect. Consider refetching on pre-join roster open or representing loading/staleness explicitly.
- The test-only
f8df871…edc43adelta atomically reapplies chip width while polling after optimistic-row replacement; no production authorization delta was found.
Validation at exact head
- PASS:
pnpm --dir desktop build:e2e. - PASS: focused smoke regression
non-member open channel header shows its member count and roster(1/1). It proves count/roster visibility, header huddle/channel-management suppression, and no out-of-channel invite results, but it does not inspect member-row or bulk management controls. - PASS:
git diff --check 52621c09bea503f4d5860030dfabfaf9ade71bfa..edc43a7077e5f3ae10d67c01c9f1179a0323569c. - GitHub exact-head snapshot: Desktop Release Candidate, DCO, path detection, relay setup, and Web succeeded; Desktop Core, macOS build, smoke shards, and integration shards were still running when this review was submitted. Their eventual status does not resolve the source-established action leak.
Manual/native evidence: no exact-head native Tauri/AX journey was completed. Source reachability and the focused browser regression establish the blocking contract failure; native, zoom/high-contrast, loading/error, and large-roster visual/performance checks remain confidence gaps rather than additional author defects.
PR #6852 review found the newly reachable pre-join open-channel roster was not actually read-only: member rows still surfaced community moderation (Ban/Timeout, from the relay role) and managed-agent lifecycle, access, and activity controls (from the viewer's agent directory), since those authorities don't derive from channel role. Derive one explicit fence, isReadOnlyRoster = channel.isMember !== true, in MembersSidebar and apply it to every non-profile affordance and its supporting action-data fetch: add/search (previously the only gate), community moderation and the restrictions query, role changes, removal, managed-agent bulk lists, and the agent runtimes query. Member cards take a readOnly prop that suppresses the entire row actions menu while identity, presence, status badges, and profile opening stay. Adds a Playwright regression that seeds the strongest pre-join viewer — a community admin who owns a running managed bot inside the unjoined channel — and proves no row menus or bulk controls render before Join, then that moderation and agent lifecycle/access controls appear after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Chessing234
left a comment
Chessing234
left a comment
There was a problem hiding this comment.
the refactor is clean — pulling ChannelMembersButton out so the pre-join header and the member header render the same control is the right move, and it means the count can't drift between the two.
the question i'd want a maintainer to answer explicitly is scope, because this changes who can see a roster, not just where a button sits.
"show the member count so non-members can understand its size" and "reuse the existing member roster for that pre-join view" are two quite different disclosures. a count tells an outsider the channel has 40 people in it. an openable roster tells them which 40 — display names, avatars, which agents are installed and who manages them. for an open channel that may be exactly right and consistent with how the channel browser already works, but it's a product decision worth naming in the pr rather than arriving as a side effect of reusing a component. if the answer is "count only", the button can render without wiring onToggleMembers.
the read gate is a single boolean and it defaults open. the comment in MembersSidebar is honest about this:
Every management affordance — add/search, community moderation, managed-agent lifecycle/access, view activity, role changes, removal, bulk agent actions — and each supporting action-data fetch hangs off this single membership fence.
enumerating eight affordances behind one flag means the ninth one added next quarter is visible to non-members unless its author remembers this comment. that's the wrong default for something that now has an unauthenticated-ish viewer. the safer shape is a canManage value threaded into the subtree (context or an explicit prop on each action group) so a new action has to opt in to being shown, and a test that mounts the sidebar as a non-member and asserts no element with a management test-id exists — one assertion that catches every future omission, rather than one branch per affordance.
is activeChannel.memberCount populated for a non-member? the member path gets its count from the roster it already loaded; the join path reads it off the channel object. if the relay omits or zeroes that field for channels the reader hasn't joined, the new button renders 0 next to Join, which is worse than not showing it. worth one e2e assertion on a real non-member payload rather than a fixture that happens to include the field.
smaller: the e2e run cited is --project=smoke on one spec. the change touches ChannelMembersBar, which every channel header renders, so the member-side path is the one at risk of a regression from the extraction and it isn't in that file.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 52621c09bea503f4d5860030dfabfaf9ade71bfa..95f8364fbaed63cbb776e6d5d39c60ebec1dfc29 (exact live head 95f8364fbaed63cbb776e6d5d39c60ebec1dfc29)
Risk: high — this exposes a shared roster before channel membership, adjacent to destructive moderation and managed-agent lifecycle/access mutations.
Blocking: membership revocation leaves an already-open mutation dialog active
The new fail-closed fence fixes the fresh pre-join surface: isReadOnlyRoster = channel?.isMember !== true now suppresses the action-data queries, bulk candidates, moderation/removal capabilities, and member-card menus (desktop/src/features/channels/ui/MembersSidebar.tsx:145-151,456-536,673-718). The new before/after-Join regression bites when the card boundary is deliberately reopened.
However, the fence does not close an action surface that was opened while the viewer was a member. Manage agent access stores its selected agent in local state (MembersSidebar.tsx:567-568); the cleanup effect reacts only to the sidebar's open state (:570-579), and the dialog remains mounted solely from that stale agent state (:941-947). If membership is revoked while the sidebar/dialog is open, the row menus disappear but the access dialog remains active. Its Save path calls the global managed-agent update mutation without a channel-membership/read-only guard (desktop/src/features/channels/ui/EditRespondToDialog.tsx:32-35,52-61,99-107).
That transition is supported by the application rather than being a synthetic state: current-user member-added/member-removed notifications invalidate the channel list, channel detail, and roster queries (desktop/src/features/channels/useMembershipNotifications.ts:20-35), so channel.isMember can become false without clearing this local dialog state. Because this PR deliberately keeps the roster surface available after that transition, the promised strictly read-only non-member state must fail closed for already-open controls as well as newly rendered rows.
Author action: when isReadOnlyRoster becomes true, clear/close every open mutation dialog or confirmation state (at minimum editRespondToAgent) and ensure Save cannot dispatch after revocation. Add a focused rerender regression: open Manage agent access with isMember=true, rerender with false, assert the dialog closes, and assert update_managed_agent cannot fire. Mutation-check that regression.
Verification owner: author for the deterministic transition regression; reviewer for changed-head source review and mutation check.
Integrated validation and non-blocking evidence
- Product/UI lane: full
just desktop-testpassed 5508/5508;just desktop-checkandjust desktop-typecheckpassed with only existing unrelated Biome diagnostics; a fresh E2E build plus fullchannel-browser.spec.ts --project=smokepassed 31/31. The pre-join UI has readable rows/status but no row menus, bulk controls, add/invite, huddle, or channel-management controls; after Join, authorized moderation and managed-agent controls return. - Regression mutation: forcing the card's
readOnlyprop false caused the new test to fail atchannel-browser.spec.ts:579with one pre-join menu instead of zero; source was restored and the tree was clean. git diff --check 52621c09bea503f4d5860030dfabfaf9ade71bfa..95f8364fbaed63cbb776e6d5d39c60ebec1dfc29passed.- Exact-head CI is green, including Desktop Core, all four Desktop Smoke shards, both integration shards, macOS build, and Desktop Release Candidate. Green gates do not exercise the live membership-revocation transition above.
- No additional tenancy/authorization defect was found in the reviewed open/private/community paths.
Manual/native evidence: no exact-head native Tauri/AX artifact, narrow/zoom/high-contrast capture, or measured giant-roster run was produced. Those remain confidence gaps, not additional author defects.
Residual risk: the existing transient Members · 0 loading label, possible summary/roster cache mismatch, and five-minute roster freshness behavior remain product-truth gaps; no changed-head regression in those areas was established.
PR #6852 review found the pre-join read-only fence did not fail closed over mutation surfaces opened while the viewer was still a member: with the Manage agent access dialog open, a live membership revocation (member-removed notifications invalidate the channel queries) left the dialog mounted from its local agent state, and its Save path dispatches the global update_managed_agent mutation with no membership guard. Clamp the dialog's agent at render behind isReadOnlyRoster — an effect-only reset would leave Save dispatchable for a frame — and clear the stale selection so a later re-join cannot spontaneously reopen the dialog. Adds a mounted rerender regression (JSDOM, driving the real row menu): a control proving Save dispatches update_managed_agent while a member, then the revocation path asserting the dialog closes on the isMember=false rerender, that a click on the stale Save button cannot dispatch, and that re-joining does not resurrect the dialog. Mutation-checked: reverting the clamp hunk turns the regression red at "revocation must close the already-open Manage agent access dialog" while the control stays green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…nel-members Signed-off-by: Matt Toohey <contact@matttoohey.com> # Conflicts: # desktop/src/features/channels/ui/ChannelScreenHeader.tsx
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: b622003f74aa5bf9b659786452813299a25e4897..6e1b5fdbc09ef2830b52754b71de6d199e4ed6bc (exact live head 6e1b5fdbc09ef2830b52754b71de6d199e4ed6bc)
The prior joined → revoked blocker is resolved. I found no remaining author-actionable defect.
Revocation now fails closed
MembersSidebar synchronously clamps the selected Manage agent access target to null whenever channel.isMember !== true, so the dialog unmounts in the revocation render before stale state can remain actionable; the cleanup effect then clears the retained selection so rejoining cannot resurrect it (desktop/src/features/channels/ui/MembersSidebar.tsx:145-151,567-581,954-960). The read-only fence also suppresses invites, moderation, role/removal, managed-agent lifecycle/access/activity actions, row menus, bulk candidates, and their supporting runtime/search fetches.
The exact-head mounted regression passed 2/2: the member control opened the real row menu and Save dispatched update_managed_agent; after a membership-revocation rerender, the already-open dialog closed, Save disappeared, a retained stale-button click dispatched zero updates, and rejoin did not reopen it. Mutation-checking by removing the render clamp made the revocation assertion fail while the member control stayed green; restoring the shipped code returned 2/2 and a clean tree.
Membership add/remove notifications for the current identity invalidate the channel list, channel detail, and roster, so the tested transition follows the application's live invalidation path. Subscription failures retry with capped exponential backoff and cleanup. Backend command authorization remains the final mutation boundary; no additional tenancy or authorization bypass was found in the reviewed paths.
Pre-join product and accessibility contract
Focused exact-head Playwright coverage passed 2/2. A non-member gets a truthful View channel members (2) accessible name and can open/search the read-only roster; even the strongest-authority fixture (community admin plus owned running bot) exposes no moderation, agent, or bulk menus before Join, then exposes expected authorized controls after Join (desktop/tests/e2e/channel-browser.spec.ts:478-610).
The trigger is a native button with the count in its accessible name (desktop/src/features/channels/ui/ChannelMembersBar.tsx:50-71). The roster has a dialog title, explicitly named close control, labeled/autofocused search, Escape coverage, and explicit loading copy (desktop/src/features/channels/ui/MembersSidebar.tsx:747-810,853-885).
Validation
At exact head with a clean worktree:
- PASS: full desktop unit suite, 5,674/5,674.
- PASS: desktop typecheck.
- PASS: desktop check (exit 0; unrelated existing warnings only).
- PASS: focused mounted revocation regression, 2/2, including causal mutation check.
- PASS: focused pre-join Playwright regression, 2/2.
- PASS CI: Desktop Core, Desktop Release Candidate, macOS build, both integration shards, and aggregate Desktop E2E Integration.
Desktop Smoke shards 3/4 and the aggregate Desktop check failed in profile-hover, onboarding-concurrency, workflow, and thread-panel tests outside this PR's eight changed files and roster path. The same failures were not reproduced as a PR-caused required-gate defect; they do not change this verdict.
Author action: none.
Residual confidence gaps: no native Tauri/AX run or visible screenshot/video artifact was produced. The live revocation transition is established at the mounted React layer plus causal mutation rather than native pixels. A full local smoke attempt was unusable due to suite-wide localStorage ... Access is denied for this document failures; focused exact-head E2E passed from the same build. UI convergence after revocation still depends on notification/refetch completion, with retry recovery and backend authorization as defense in depth.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: exact live head 6e1b5fdbc09ef2830b52754b71de6d199e4ed6bc against current base b622003f74aa5bf9b659786452813299a25e4897.
The prior joined→revoked stale-dialog blocker is resolved. The roster derives one fail-closed read-only boundary, synchronously clamps the selected Manage agent access target before render when membership lapses, and then clears stale state so rejoining cannot resurrect the dialog (desktop/src/features/channels/ui/MembersSidebar.tsx:145-151,567-581,954-960). Fresh pre-join rendering continues to suppress row, bulk, moderation, lifecycle/access, activity, invite, huddle, and management actions.
Integrated exact-head evidence
- The mounted joined→revoked regression passed 2/2: the member control dispatched once; after revocation the already-open dialog closed, Save became unavailable, a retained stale-button click dispatched zero
update_managed_agentcalls, and rejoin did not reopen it. - Independent causal mutations removing the synchronous clamp/stale-state cleanup made exactly the revoked-dialog case fail while the member control stayed green; shipped source was restored and re-proved clean.
- Focused pre-join Playwright passed 2/2 with the strongest-authority fixture (community admin + owned running bot): the roster remained readable but exposed no management menus before Join, then restored appropriately authorized controls afterward. Keyboard/AT inspection confirmed native buttons, accessible count naming, labeled dialog/search controls, autofocus, and Escape behavior.
- Full local package evidence passed at exact head:
just desktop-test5,674/5,674,just desktop-check, andjust desktop-typecheck. - Membership notifications invalidate channel list/detail/roster for the current identity; subscription failure retries with bounded backoff and cleanup. Server/global-command authorization remains the final boundary. No new cross-community/private-channel authorization path was found in the reviewed paths.
- CI passed Desktop Core, smoke 1/2, macOS build, release candidate, DCO, relay E2E, and both integration shards. Smoke 3/4 and aggregate Desktop failed in profile-hover, onboarding concurrency, workflow, and thread-panel tests outside this PR’s changed files and roster flow; the exact-head focused and full package evidence above passed.
Mark Previous Review Staleis separate review-workflow bookkeeping.
Author action: none.
Confidence gaps / verification owner: no native GUI/AX run or native joined→revoked visual artifact was produced; the full local smoke run was unusable due environment-wide localStorage ... Access is denied failures. These are reviewer/native and harness verification gaps, not author defects. The mounted causal regression, focused Playwright, full package suite, and source trace sufficiently establish the changed behavior.
3 participants
Summary
Joinso non-members can understand its size before joining.Related issue
N/A. No matching open issue or pull request found.
Testing
just cicd desktop && pnpm build:e2ecd desktop && npx playwright test tests/e2e/channel-browser.spec.ts --project=smoke(30 passed)#designheader, roster, and no-invite search stateScreenshots
Before
Non-members saw only
Joinin the channel header.After
The member count is available beside
Join.The roster opens in read-only mode with no add-member affordance.
Searching for a non-member does not expose an invite path.