fix(desktop): refresh channels after access-revoked closure - #7784
wesbillman wants to merge 1 commit into
Conversation
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
🔐 Codex Security Review
|
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Verdict: no blocking code findings
Reviewed head 806c1e4e2969fd60ab167fe0b6522c5ba952556a against base a61239f0d8036aff58176f5c0ce7f080c66e21b7. This is a comment review, not approval or merge certification.
The 27-line production change meets the stated recovery contract: only the exact access-revoked CLOSED reason on a known channel-scoped live subscription emits a refresh hint; existing terminal retirement happens first. It does not infer membership/archive state or restart the retired subscription. The hook reuses the existing debounce/idle-aware refresh and cleans up on unmount/session reset.
I traced the authoritative path through get_channels, native archive conversion and the stable list hash (which includes archive/membership), IPC conversion, query settlement, and sidebar/active-channel consumers. Successful updated state reaches existing sidebar filtering and stream composer read-only controls; a failed fetch retains the previous list rather than inventing an empty result. Reconnect replay checks the live map and subscription identity, so retired entries are not restored.
Validation and remaining gates
- Source review only, including the new six-case production-path fixture and complementary independent review lanes. No reviewer test execution or live archive mutation. The PR reports exact-head hooks and 6,514/6,514 desktop tests; I did not reproduce that run.
- The fixture binds real subscription creation, CLOSED dispatch, mounted hook, query refresh and IPC conversion, with native IPC stubbed. Native discovery, rendered sidebar and live incident ordering are not exercised. The existing busy→idle re-arm behavior is source/helper-test coverage, not a new integrated regression. These are disclosed evidence limits, not demonstrated code defects.
- The PR remains draft. Hosted desktop checks were still running at the review snapshot, full local
just ciis explicitly incomplete, and the security bot still requests review of this range. This review does not waive those gates. No code changes requested; complete outstanding validation before treating this as merge-ready. The broader symptom report #7026 and archived-channel delete policy remain outside this PR’s demonstrated result.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: a61239f0d8036aff58176f5c0ce7f080c66e21b7..806c1e4e2969fd60ab167fe0b6522c5ba952556a (exact head 806c1e4e2969fd60ab167fe0b6522c5ba952556a)
Risk: high — this changes relay subscription lifecycle behavior and the renderer's authoritative channel-refresh path after an authorization-related terminal frame.
Finding
P2 — the regression suite does not protect the PR's claimed exact CLOSED-reason boundary. Production correctly gates the hint on exact equality with restricted: channel access revoked (desktop/src/shared/api/relayClientSession.ts:801-806), but the negative case only sends an unrelated invalid: bad filter reason (desktop/src/features/channels/channelAccessRevocation.test.mjs:257-272). It never sends a near-miss restricted: reason on a known channel-scoped live subscription.
I broadened the production predicate to accept every string beginning with restricted:. All six new tests still passed (6/6, exit 0). That mutant would refresh the authoritative channel list for ordinary restricted closures, contrary to the PR's explicit exact-match contract. The repository requires regression tests to bind and falsify their production guard (TESTING.md:25-32; AGENTS.md:192-196).
Author action: add a known, channel-scoped live-subscription negative case using a different restricted reason, such as restricted: not a channel member, and assert that no channel refresh occurs. Verify that broadening the predicate to startsWith("restricted:") makes the test fail, then restore the exact predicate and prove it passes.
Verification owner: author for the regression addition; reviewer to mutation-check the final exact head.
Integrated contract trace
The production path itself appears sound:
- The hint is accepted only for the current connection generation, a known live subscription with a non-empty
#h, and the exact access-revoked reason. Unknown, retired, global, stale-generation, and unrelated frames are ignored. - Existing CLOSED handling remains terminal: the subscription is removed before listeners run and cannot be replayed on reconnect.
- CLOSED remains only a hint. Archive, removal, and membership state still come from the authoritative
get_channelsquery; sidebar/composer behavior derives from that result. - Burst hints use the existing trailing, idle-aware refresh. Failed refreshes retain cached data and do not create a new retry loop. Unmount and community disconnect remove listeners and queued work.
- Archived, removed, unchanged, and failed authoritative outcomes are exercised through real subscription creation/CLOSED dispatch and the query/cache path. No new visual or accessibility control is introduced.
Validation
At exact clean head 806c1e4e2969fd60ab167fe0b6522c5ba952556a:
- Full Desktop package suite: 6,514/6,514 passed, 0 failed.
- Focused production-path fixture: 6/6 passed.
- Causal removal of hook registration and suppression of hint emission each failed the positive outcome rows as expected.
- Exact-boundary broadening mutant (
startsWith("restricted:")): 6/6 still passed — reproduces the finding. pnpm typecheck,pnpm check, andgit diff --check: passed/clean.- Hosted Desktop core, four smoke shards, Windows/macOS builds, integration shards, DCO, Semgrep, and zizmor: green at the reviewed head.
Manual/native evidence: none. The fixture stubs native IPC while exercising the renderer subscription/query seam; no live-relay archive/removal journey or rendered-pixel capture was performed.
Residual risk: the exact incident ordering and busy-to-idle behavior were source/helper traced rather than reproduced through a live native journey. These are confidence gaps, not additional author defects.
Pinky is opening this PR on Wes’s behalf.
Summary
CLOSED restricted: channel access revokedon a known channel-scoped live subscription as a hint to refresh authoritative channel state.useLiveChannelUpdates’ existing debounced, idle-aware query refresh. Keep terminal subscription handling, explicit membership denial, and polling unchanged.The hint does not establish archive or membership state. This closes a missed-update recovery gap without changing the relay or blindly retrying subscriptions.
Related symptom report: #7026 (not claimed fully resolved; archived-channel delete policy is separate).
Related new-client fix: block/buzz-app#106
Originating conversation: buzz://message?channel=cb38cd5a-c920-40a1-b770-e9b89c289476&id=d22c1eed473016cf1f73c889a4cc88f1491ee092cfe3794575f8fea8003aa19e
Validation
At
806c1e4e2969fd60ab167fe0b6522c5ba952556a:Remaining validation / scope
buzz-voice. This was incomplete validation, not a failing test; fulljust cihas not completed. Unrelated native/mobile lanes were not rerun by the path-scoped push hooks.