Conversation
fe0afc9 to
18b2a5f
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head 18b2a5f478e434fa298aa4119f477b553c0f28aa against base 319635e1ca0696fc06101f9d5c294fb12bfcbc23. Changes requested.
Two P2 blockers:
- Hidden DMs reappear on startup when the visibility snapshot beats channel metadata.
- The only Hide action is excluded from keyboard navigation, with no reachable alternative.
See the inline findings. Exit criteria: retain hide state through both discovery response orders without suppressing non-DMs, and make Hide reachable/operable with the keyboard. Preserve canonical exact-recipient reopen and membership semantics.
Validation
- Reviewed the session/discovery lifecycle, canonical roster admission, purpose-bound signing, transport failure semantics and UI integration with complementary reviewers.
- At this exact head with production source unchanged and a temporary two-case regression test added, the full
bin/pnpm exec vitest runrun passed all 2,069 existing tests plus the metadata-first control. The hidden-snapshot-first regression failed: the supposedly hidden DM remained in the ready channel list. These are real session/store/discovery tests with signed fixtures and explicitly gated network completions, not browser or native interaction evidence. - Existing CI is not green: Chromium channel-opening measurement was 117.9ms against a <100ms warm-switch bound; WebKit shard 1 failed the saved-dark-document reload test with
Importing binding name 'n' is not found.JavaScript, Rust/tool integration, both Chromium journey shards and WebKit shard 2 passed. I have not attributed these CI failures to this diff or established that they are pre-existing/flaky. - I did not exercise a live identity or an actual native UI in this review.
Non-blocking diagnostic correction
dev/relay-broker.mjs:1284-1293 calls every SocketRequestError(sent=false) a relay rejection and returns 422 with sent:true, including locally unsent capacity/pre-dispatch-timeout errors. Preserve local non-delivery and distinguish it from an actual correlated negative relay receipt. This is a misleading error/provenance issue, not an established delivery-safety blocker: the current DM transport treats both as proven failure and the UI does not retry automatically.
Add Base UI menu and context-menu primitives, shared floating styles, and the component registry/specimen entry. Signed-off-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
Signed-off-by: Clay Delk <clay.delk@gmail.com>
Signed-off-by: Clay Delk <clay.delk@gmail.com>
Signed-off-by: Clay Delk <clay.delk@gmail.com>
Signed-off-by: Clay Delk <clay.delk@gmail.com>
Signed-off-by: Clay Delk <clay.delk@gmail.com>
18b2a5f to
e2553a1
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head e2553a1f0724a6f759ab7bf0ce9d67add9296ec8 against base 298a50a9861c19d33e45b2b52bdadb81b128f15d.
Changes requested
P2: Keep the eight-recipient conversation submittable. NewMessageView.tsx:496 disables the recipient input when selected.length >= 8, but that same input's Enter handler is the only user path to open() (lines 428–433, 509). There is no form submission or Open button. Select eight distinct recipients through the advertised group-DM flow: the input becomes disabled, cannot receive Enter, and the screen still says “Press Enter to open this conversation.” Removing someone is the only way to regain submission, so the supported maximum-size group cannot be opened.
Keep submission reachable at the limit while preventing a ninth recipient, without relaxing the host limit. Add a regression that selects eight exact recipients and submits that exact set through a real enabled user control; also verify a ninth cannot be added. A synthetic key event dispatched to a disabled input would not prove this contract.
P2: Preserve the highlighted recipient by identity across asynchronous search expansion. NewMessageView.tsx:160–172 merges a second result set and re-sorts it, but highlighted remains an array index. The existing “dr” fixture demonstrates the ordering: the first 25 results include Dr. 0; the later result Drafty sorts before it under lines 302–328. Arrow to Dr. 0 while the extended read is pending, release that read, then press Enter: lines 430–432 now select Drafty, not the identity the user highlighted. The current selection-preservation test resolves with an empty result and misses this case. Anchor keyboard selection to the pubkey (or deliberately clear it if no longer eligible), and test a nonempty deferred result that inserts ahead of the active identity. No extra user input should silently retarget a DM recipient.
P2: Make Hide's disabled state match its global in-flight guard. ChannelsPage.tsx:755–756 drops every hide request while any DM is being hidden, but the hover button and keyboard menu disable only the matching row (lines 833, 845). Hold Hide A pending, then activate the still-enabled Hide B: the action silently returns without a request, feedback, or eventual hide. The smallest fix is to disable all Hide controls while that global guard is active; concurrent per-channel machinery is not needed. Cover the pending and recovery states with a deferred first hide.
Prior review disposition and scope
The prior startup visibility and keyboard-Hide blockers are resolved in the reviewed code paths: visibility IDs survive either metadata/snapshot order, the store filters only DM visibility while retaining authoritative membership, and the existing focusable row now opens Hide through Shift+F10/ContextMenu. The broker diagnostic fix also distinguishes receipt-less proven-unsent failures from correlated relay rejection. Exact-recipient canonical reopen and purpose-bound signing remain intact.
Review covered the full current feature diff across sidebar/search/menu integration, session lifecycle and discovery/store authority, broker/transport/socket receipt provenance, and the added regression tests. Read-only Blox source review with independent UI and authority lanes; no checkout, build, tests, native interaction, or live writes were run for this review.
Existing exact-head CI evidence
JavaScript, Rust/tool integration, both Chromium journey shards, WebKit shard 2, DCO and security checks passed. CI is not green: WebKit shard 1 failed the saved-dark-document startup case with missing import binding n; browser measurements measured warm channel visibility at 120.4 ms against the <100 ms gate (two later cases did not run). Windows native validation was skipped. Separately, ChannelsPage.tsx:856–863 replaces stable row callbacks with inline closures, defeating the existing ChannelSidebarItem memo boundary on parent updates. Preserve stable callbacks when addressing the sidebar performance gate; source alone does not establish that this caused the measured failure. Those CI failures still require resolution before merge; this review does not establish they are unrelated or flaky, and does not authorize relaxing their assertions.
Why
Messages can show existing direct messages, but people cannot start, reopen, or hide a conversation from this app. The DMs section also disappears when it is empty, which leaves no path to start one.
What
How
The development broker signs purpose-bound kind
41010open and kind41012hide commands on the existing live socket. The session accepts a returned channel only after fresh relay-authored metadata and exact roster evidence match the request.Hidden DMs use the relay’s kind
30622viewer snapshot. The session suppresses only the hidden conversation and keeps shared profile names, authorization, and other sidebar rows intact.New Message uses a keyboard combobox. The first result is not selected until an arrow key moves the active row. Search filters known names from the first character and only uses public-key prefixes after eight characters.
Risk
This adds signed DM writes and a new finite visibility read to the development broker. Commands are bounded, session-scoped, cancelled on teardown, and never retried after uncertain delivery.
This draft currently carries Carl’s shared-menu commit from #89 as its first commit, with authorship and DCO preserved. It was rebased onto current
mainand moved to the required Phosphor icon gateway. Drop or reconcile that commit when #89 lands.Testing
Manual testing against
buzz.block.builderlab.xyz:Local
just scanevidence:terminal::tests::real_spawn_fences_secrets_and_preserves_login_contextwas blocked by this machine’s interactive ClickFix acknowledgement prompt.Bigger picture
This PR adds direct-message actions only. Channel creation, joining, sidebar DM avatars, and cleanup of the existing diagnostics menu remain separate work.
Generated with Goose