fix(desktop): refresh @ search when channel membership changes - #7191
fix(desktop): refresh @ search when channel membership changes#7191loganj wants to merge 2 commits into
Conversation
9164074 to
b411da5
Compare
b411da5 to
4316a13
Compare
e647e70 to
96c6d9c
Compare
Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com>
96c6d9c to
ff2b68c
Compare
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed base bb8cca21e13099bc6206b18c62272b72928574ca through exact head ff2b68cb15bcd9d01ac37aedff5511b605166ad2.
1. Major — background third-party membership changes leave the mention directory stale
The global notification hook filters on the viewer's pubkey (desktop/src/features/channels/useMembershipNotifications.ts:52-60), while relay membership notifications p-tag only the changed member (crates/buzz-relay/src/handlers/side_effects.rs:913-928). Thus viewer V does not receive that path when actor A adds/removes target T. The mounted-channel path does refresh on membership system rows (desktop/src/features/messages/hooks.ts:375-390), but the all-channel subscription used for background channels accepts those rows without scheduling a refresh (desktop/src/features/channels/useLiveChannelUpdates.ts:232-335,385-401). V can therefore keep a removed agent visible, or omit a newly added agent, until polling despite already receiving the background-channel row.
Author action: detect membership rows in the all-channel live-event production path (or emit a genuinely viewer-addressed global event), invoke the coalescing scheduler there, and add a caller-bound regression for a background-channel A→T add/remove observed by V. Deleting the production invocation should make that test fail.
Verification owner: :bot: Jude’s code review agent will trace active/background delivery, run the full Desktop package, mutation-delete the invocation, and exercise V's first @ search after the background change.
2. Moderate — refresh/reorder can silently retarget keyboard selection
An open picker reranks when the directory changes (desktop/src/features/messages/lib/useMentions.ts:369-399), but selection is retained only as a numeric index and merely clamped (desktop/src/features/messages/lib/useMentionSelection.ts:7-23). Enter/Tab inserts whichever identity now occupies that index (useMentions.ts:776-815). A production-hook probe selected Bob in [Alice, Bob], then rerendered [Agent, Alice, Bob]; selection stayed at index 1 and therefore moved to Alice instead of following Bob to index 2. The next Enter/Tab can mention the wrong recipient without further navigation.
Author action: preserve selection by stable suggestion identity (normalized pubkey/persona ID/team ID) across refresh/reorder, with an explicit safe fallback if the identity disappears. Add a regression that changes ordering while the picker is open, asserts the visible highlight remains on the intended identity, and then verifies Enter/Tab inserts that identity. Restoring index-only clamping should fail the test.
Verification owner: :bot: Jude’s code review agent will verify keyboard/highlight/visible-row behavior and exact inserted recipient identity.
Evidence and confidence
- Full Desktop package:
6,028 passed, 0 failedat the exact head; HEAD unchanged and review trees clean afterward. git diff --checkpassed.- Exact-head CI run
34262722726: Desktop Core, four smoke shards, relay-backed Desktop E2E/integration, macOS/Windows builds green; DCO, Semgrep, and zizmor green. Cancelled entries belong to a superseded run, not this head. - Confidence gap, not a separate defect: no native GUI/accessibility journey was run. After the fixes, exact-head native evidence should cover a background third-party membership change and open-picker reorder.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Requesting changes on exact head ff2b68cb15bcd9d01ac37aedff5511b605166ad2 after reconciling both assigned lanes. Two independent author-actionable defects remain.
-
Open-picker refresh can silently retarget keyboard selection. Membership refresh reranks suggestions (
desktop/src/features/messages/lib/useMentions.ts:369-399), but selection persists only as a numeric index (desktop/src/features/messages/lib/useMentionSelection.ts:7-23); Enter/Tab inserts the identity now occupying that index (useMentions.ts:776-815). A production-hook probe selected Bob in[Alice, Bob], refreshed to[Agent, Alice, Bob], and retained index 1 (Alice) instead of Bob at index 2. This can mention the wrong recipient without further navigation.Required change: preserve selection by stable suggestion identity (normalized pubkey/persona/team identity) across reorder; use an explicit safe fallback if it disappears. Add a regression that reorders an open picker, asserts the same visible identity remains highlighted, then verifies Enter and Tab insert that identity.
-
Third-party membership changes in background channels miss the refresh scheduler. The global notification hook filters
#pto the viewer (desktop/src/features/channels/useMembershipNotifications.ts:52-60), but relay membership notifications tag the changed member (crates/buzz-relay/src/handlers/side_effects.rs:913-928). Mounted-channel rows refresh atdesktop/src/features/messages/hooks.ts:375-390, while the all-channel background path receives system rows but never callsrefreshDirectoryAfterMembershipChange(desktop/src/features/channels/useLiveChannelUpdates.ts:232-335,385-401). A viewer can therefore retain stale mention candidates after another actor changes a third party in a background channel.Required change: invoke the coalescing refresh scheduler from the all-channel production event path (or provide a viewer-addressed global event). Add a caller-bound behavioral regression for a background A→T add/remove observed by V; deleting the production invocation must fail it.
Baseline evidence was green: Desktop 6,028/6,028, clean git diff --check, and 17 successful/10 skipped exact-head checks. An older duplicate run is cancelled. Live head was unchanged immediately before review.
|
On the open-picker reorder retargeting keyboard selection (Moderate finding) — valid at this PR's head; fix owned by #7197 Valid as reviewed at Per this stack's ownership split the fix is #7197's stable installed-snapshot chooser, not a backport here. At #7197's head Existing coverage of this mechanism at #7239's head
Scope, stated plainly: standalone #7191 at |
The app-wide background receiver merged kind 40099 channel-event rows into the timeline cache but never refreshed the mention directory, so a third-party member add/remove in a subscribed background channel left mention candidates stale until the next focused directory poll. Parse the member_joined/member_left/member_removed payload in handleIncomingMessage and call the existing refreshDirectoryAfterMembershipChange with the row's event id, mirroring the mounted-channel path; the helper's coalesce, cancel and event-id dedup semantics are unchanged. A mounted-caller regression drives the real hook with relay-wire 40099 rows and asserts that non-membership rows do not refresh. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com>
|
On the background third-party membership change leaving the mention directory stale (Major finding) — fixed at this PR's new head Valid as reviewed at Fix published as commit New mounted-caller regression, Red / green / deletion control — actual runs bound to this exact candidate (the validated dirty state, now byte-identical to the published commit):
Full-suite proof scope: the entire desktop package suite was run once at this exact candidate — 6031/6031 tests pass, 87 suites, exit 0 ( Stack note: the identical fix is mechanically replayed onto #7192's new head Status, stated plainly: the claim-1 code is fixed and published at
|
🤖
Summary
Keeps the people and agents you can @ mention up to date as membership changes, without restarting or reopening anything.
The refresh supplies up-to-date evidence for the picker only; it never grants permission or optimistically adds anyone to the list.
Part of eight mention-editor PRs: shared recovery prerequisite #7190 → {#7191 → #7192, #7196 → #7323 → #7197 → #7239 → #7240}. This PR builds on #7190 to preserve the baseline visible ambiguous-send failure behavior; membership freshness remains a separate feature unit.
Related issue
Continues the merged mention-editor work from #7124 (authorize remote mentions at publication) and #7128 (preserve spacing after multi-word mentions). No separate tracking issue for this slice.
Testing
membershipDirectorySync.test.mjscover immediate staleness, coalescing, duplicate-event suppression, cancellation of in-flight reads, and reset on community or signing-identity switch.ff2b68cb, CI run 34262722726 completed successfully, including Desktop Core, all four smoke shards, relay-backed integration, and macOS/Windows builds, with DCO, Semgrep and zizmor passing. Current headf66687efadds the claim-1 fix — the app-wide background receiver now refreshes the mention directory on third-party membership rows — validated at this exact candidate by the full local desktop suite 6031/6031, focused red/green with deletion control, types and lint; its CI is dispatched and pending. This is lane-local evidence, not a ready-to-merge or all-lanes-green claim.