Conversation
ade5a8d to
b294d17
Compare
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 at b294d177da16ef38e2aed5d5f5b64a9cd149a7c1, against 09c6c04a49712732b97de2caa346a79c6eb2a750. Four P2 defects in the new assignment flow:
1. Respect the shared relay API admission lane
dev/relay-broker.mjs:572-595, also publication at lines 597-627.
The assignment route calls fetchUpstream directly for read/publish/confirmation, bypassing admissions(relay, viewer).api and admittedApiRequest. This ignores an already-known quota pause, bypasses request pacing, and does not teach the shared lane about a new quota refusal. Per-relay mutation serialization does not preserve that existing contract.
Reproduced through the production broker with an ephemeral identity: an ordinary query learns a 61-second API pause; another ordinary query is correctly rejected locally; a sidebar removal then issues all three upstream requests within that same pause. The ordinary route remains paused afterward. Route these calls through the existing principal admission helper, signing at dispatch, preserving cancellation and quota error handling. Add a regression for a learned pause and a quota response from this route.
2. Restore focus to the relocated row after confirmation
ChannelsPage.tsx:287-290, called at lines 311-315.
A successful cross-section assignment replaces the source row, but focus restoration uses the captured source DOM element. After keyboard removal of Beta from Work, both Chromium and WebKit leave document.activeElement on BODY, not the relocated row action. Keyboard users lose their place after the advertised keyboard operation. Resolve focus by channel identity after the new row renders, with a valid visible fallback if its destination is collapsed. Cover successful move/remove and context-menu launch.
3. End the disabled state when a save fails
ChannelsPage.tsx:316-320, disabled predicates at lines 504 and 519.
The catch path keeps { channelId, error } in groupWrite, while every menu action is disabled solely by matching channelId. A rejected save therefore leaves the error visible but all actions permanently disabled for that open menu. Both browser probes reproduced this; focus also leaves the menu. Retain the error separately from the pending state, re-enable retry/other destinations, and restore usable keyboard focus. Closing and reopening is a workaround, not recovery offered by the failed menu. Cover rejection followed by retry without reopening.
4. Fit the actual menu height inside the viewport
ChannelsPage.tsx:339-343, paired with Channels.module.css:181-186.
Placement reserves only 240px below the top coordinate, but CSS permits a 320px panel. With 13 saved groups at an 800×400 viewport, both browsers render the menu from y=160 to y=480. Its bottom 80px, including final choices/removal when scrolled to the end, remains outside the viewport. Clamp using the actual panel height or constrain its available height at the chosen position; cover many groups and a lower-edge launch.
Validation: Current hosted checks are green. I ran only additional focused probes against unchanged production source in a separate worktree: the three UI assertions failed in both Chromium and WebKit, and the production-broker pause probe confirmed the bypass. The rejected-save probe deliberately injects HTTP 502, which also triggers the fixture's generic console-error guard; the disabled-button failure was independently observed before teardown. No live-account writes or broad local suite reruns. The PR reports a pre-rebase full scan plus exact-head focused reruns; hosted CI does not cover the three documented local-only WebKit cases.
Scope / exit criteria: Fix these four paths with targeted regression coverage and appropriate revalidation. Preserve the narrow host-owned mutation and existing read-only/excluded-row behavior. Legacy whole-record replacement remains last-writer-wins across devices; confirmation is not a guarantee against losing unrelated concurrent changes. A new synchronization protocol, speculative relay consistency concerns, and general menu polish are not additional blockers in this review.
b294d17 to
dbbf7a7
Compare
|
Addressed the four blocking review findings in
Regression evidence: 49 focused owner/host tests passed, plus all 6 built Chromium/WebKit group journeys. Those journeys cover keyboard relocation focus and independent encrypted Recent/A–Z section choices; the host tests cover purpose-bound 128-channel activity admission, failure retention, live monotonicity, deterministic fallbacks, and shared signed-admission cooldown behavior. AI-generated implementation report by Rizz. |
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: six P2 defects
Reviewed head dbbf7a7c53a797d97f8de03173693bce16c616ee against base 1ba1544f833082bccb604f8d6a9b40d6d4caacc8. Consolidated the host/admission and UI lanes, then independently traced and reproduced the integration findings.
- Confirmed saved-group sort choices are discarded by the browser transport projection.
- Forum Recent cannot receive live post/reply events because the production subscription excludes their kinds.
- An older roster refresh can delete newer channels' activity from the session projection.
- Moving/removing into a collapsed destination still loses keyboard position.
- Escape from the section sort menu loses trigger focus.
- Failed assignment re-enables actions but restores focus before the enabled DOM is committed, stranding keyboard retry.
The inline comments contain reproductions, anchors and narrow exit criteria. There is also one non-blocking P3 for clicking the open section trigger reopening its menu.
Prior review disposition
The shared API-admission bypass is repaired: assignment/sort read, publish and confirmation hops use the principal's shared admission lane. The permanently-disabled assignment state is repaired, and mouse retry works; the remaining keyboard recovery defect is #6 above. Expanded-destination focus is repaired, but collapsed destinations are not. The dynamic row-menu height stayed inside an 800×350 viewport in both engines in the focused probe; many-group stress was not rerun.
Validation and scope
- Focused probes on Wes's Studio at the exact head, with unchanged tracked source and only temporary review tests. The real encrypted mutator/broker transport/store loses
section:work, the authenticated live REQ omits 45001/45003, and both the projection and real session reproduce inverted roster completion. - Built Chromium and WebKit journeys using the production broker and ephemeral fixture identities reproduce the saved-sort, collapsed-destination, Escape, and failed-save focus defects. Mouse retry and the measured viewport bounds succeed. These are adversarial review probes, not a claim that the package suite passed locally; no real-identity writes or native launch were performed.
- Hosted CI is successful at
dbbf7a7c53a797d97f8de03173693bce16c616ee. Existing broad CI was not duplicated locally. The current saved-group browser test checks one visible row and encrypted publication, not retained radio state or row ordering, so it misses #1.
Exit criteria are the narrow contract repairs and regressions above. Whole-record cross-device last-writer-wins behavior and stale-client section-ID pruning remain non-blocking, outside a broader sync redesign. No approval or merge performed.
| projectSidebarPreferences(undefined, undefined, { | ||
| version: 1, | ||
| groups: value.groups, | ||
| }).sort ?? {} |
There was a problem hiding this comment.
[P2] Preserve saved-group keys when projecting the confirmed sort response
This call supplies neither the sections document nor sortSectionIds. projectSidebarPreferences therefore constructs an empty live-section set and drops every section:* entry. With an existing Work group, selecting Recent successfully publishes and confirms {"section:work":"recent"}, but setSort() receives {} and overwrites the retained sort map. The menu closes as success, stays A–Z when reopened, and other saved-group choices are also removed from memory by subsequent sort writes.
Reproduced through the production encrypted mutator → broker transport → store, then in the built app with the production broker in Chromium and WebKit: encrypted publication says Recent while the reopened Recent radio has aria-checked="false". Pass the current section IDs into the projection; cover actual ordering with at least two rows, the reopened radio state, and preservation when another section is changed.
| const visible = accept(events); | ||
| channelActivity.accept(visible); |
There was a problem hiding this comment.
[P2] Subscribe to forum activity before relying on this live recency feed
The new projection recognizes kinds 45001/45003, and its historical query asks for them, but this receive callback is fed by subscribeRelayTraffic. That subscriber's CHANNEL_KINDS (live.ts:116, used in the channel REQ at 300–315) omits both forum kinds. Neither global route requests them either. The broker and signed transports share this subscriber.
I captured the actual channel REQ after the production subscriber's authentication handshake: it contains [9,40002,40099,40003,5,9005,7,39000,39002,39005]. Consequently, a new forum post/reply cannot advance Forums → Recent until a later historical refresh/reconnect. Include both kinds in the production channel subscription and test a new forum event through the wire/filter boundary, not just direct calls to channelActivity.accept().
| if (result === undefined) next.delete(id); | ||
| else next.set(id, result); | ||
| } | ||
| for (const id of next.keys()) if (!wanted.has(id)) next.delete(id); |
There was a problem hiding this comment.
[P2] Fence superseded roster refreshes before pruning activity
Every refresh captures the same generation until clear/dispose. session.ts:1131–1153 can start a new refresh when the roster changes while the previous one is still pending. This prune then applies whichever invocation finishes last, even if its roster is older.
Deterministic reproduction through the real session: enable Recent with roster [a] and hold its activity read; accept a signed grant for b, producing refresh [a,b]; complete the newer read with b=20, then release the older read. The public channel list still contains b, but its lastActivityAt changes from 20 to undefined. The session's roster key already equals [a,b], so ordinary list updates do not repair it. A visible active channel is relegated to the inactive tail until fresh traffic or another activity refresh. Give each refresh current-roster ownership, or serialize/cancel superseded refreshes, and retain the inverted-completion session regression.
| const action = destination?.querySelector<HTMLButtonElement>( | ||
| `[data-channel-action="${CSS.escape(rowFocus.channelId)}"]`, | ||
| ); | ||
| const link = destination?.querySelector<HTMLButtonElement>( | ||
| `[data-channel-id="${CSS.escape(rowFocus.channelId)}"]`, | ||
| ); | ||
| (action ?? link)?.focus({ preventScroll: true }); | ||
| setRowFocus(undefined); |
There was a problem hiding this comment.
[P2] Fall back to a visible control when the destination is collapsed
The relocated row exists in the DOM inside a closed <details>, so action ?? link resolves, but focusing it cannot succeed. rowFocus is then cleared without a fallback. Reproduced in both Chromium and WebKit with a nonempty collapsed Channels section: open Beta's group menu, use End → Enter to remove it from Work, wait for confirmation, and focus is BODY while Beta is hidden in Channels. The same path applies when moving into a collapsed saved group.
Preserve the collapse choice and focus the destination's visible summary or another deliberate visible sidebar control when its row cannot receive focus. The expanded-destination journey does not cover this remaining part of the prior relocation finding.
| if (event.key === "Escape") { | ||
| event.stopPropagation(); | ||
| closeSectionMenu(); | ||
| return; |
There was a problem hiding this comment.
[P2] Return focus to the section trigger on Escape
Opening the section menu explicitly moves focus into its radio buttons, but Escape calls the non-restoring form of closeSectionMenu. Removing that focused menu leaves document.activeElement as BODY in both Chromium and WebKit. Keyboard users lose their sidebar position after an ordinary dismissal.
Use the restoring close path and retain a valid trigger across the React commit; assert that opening Work's menu and pressing Escape focuses “More actions for Work” in both engines.
| queueMicrotask(() => | ||
| rowMenuPanel.current | ||
| ?.querySelector<HTMLButtonElement>( | ||
| '[role^="menuitem"]:not(:disabled)', | ||
| ) | ||
| ?.focus(), | ||
| ); |
There was a problem hiding this comment.
[P2] Restore failed-assignment focus after the enabled buttons commit
The new pending: false state fixes the permanently-disabled actions, but this microtask is not a React commit boundary. It can query while the menu buttons are still disabled, find no :not(:disabled) target, and never run again after the error render.
With a deferred failed assignment, I verified the pending disabled state, released the failure, and waited for the visible alert and re-enabled Remove action. Both engines left focus on BODY; End/Enter no longer operated the menu. A mouse click could retry successfully without reopening it. A separate HTTP-502 probe also reproduced the keyboard failure in Chromium. Restore focus from a layout/effect tied to the committed failed/pending state rather than a one-shot pre-commit microtask, and add a held-failure → keyboard-retry journey.
| const dismiss = (event: PointerEvent) => { | ||
| if ( | ||
| event.target instanceof Node && | ||
| !sectionMenuPanel.current?.contains(event.target) | ||
| ) | ||
| closeSectionMenu(); | ||
| }; | ||
| document.addEventListener("pointerdown", dismiss); | ||
| return () => document.removeEventListener("pointerdown", dismiss); |
There was a problem hiding this comment.
[P3, non-blocking] Exclude the section trigger from outside dismissal
Clicking the already-open trigger fires this outside pointerdown first, closing the menu; its subsequent onClick then observes the closed state and opens it again. The row-menu guard already excludes its own trigger. Reproduced in Chromium and WebKit: two clicks on “More actions for Work” leave aria-expanded="true" and the menu visible. Exclude sectionMenuTrigger here and cover click-open/click-close.
dbbf7a7 to
8c84486
Compare
Expose group assignment and Star/Unstar through confirmed host-owned preference writes. Preserve unrelated encrypted data, fence stale reads and cancelled sessions, and restore keyboard focus after exclusive row relocation. Signed-off-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
Offer Starred and saved groups in one destination chooser. Confirm the destination assignment before clearing Star so removal returns to Channels without restoring a hidden group after reload. Retain confirmed local placement through partial failures, fence overlapping refreshes, and cover ordered writes, cancellation, retry and browser focus. Signed-off-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
8c84486 to
0cbb69a
Compare
tellaho
changed the title
2 participants
Overview
Category: new-feature
User Impact: Users can move stream channels between Starred and saved groups, or remove them back to Channels without restoring a previous group.
Problem: Saved groups and stars were visible in Messages, but channel rows lacked a consistent way to change their placement. Treating Star as an overlay also made removal unexpectedly restore a hidden previous group.
Solution: Treat Starred as a built-in group pinned first, alongside saved groups in one Move to… chooser with the current destination checked. Confirm encrypted writes before relocating the single visible row, keep failures retryable, and return removals to Channels—even after reload.
Scope and stack
This existing PR is now the permanent grouping + Star slice, replacing #90. It keeps #10's discussion history but replaces its old combined grouping/sorting head; old reviews and validation must be reassessed against this diff. Implemented by Carl (AI agent).
block/buzz-app:tho/sidebar-menusat57dd36eab159367c27063c8632930d88fd6463c9.tellaho/buzz-app:tho/left-nav-groupsat0cbb69ab8e33b5ac11b1e97d503b45f660b1fd09.block/buzz-app:tho/sidebar-grouping-star, mirrored to that exact commit. GitHub base branches belong to the PR's base repository; the fork head is not a same-repository base choice. Keep this mirror when retiring feat(channels): save sidebar groups and starred channels #90.8c844863a8e9acc1b982dbbc49743ddc540a818ais preserved ontellaho/buzz-app:tho/left-nav-groups-pr10-preserved-20260917. The separate local combined snapshote63260dremains untouched.Ownership and persistence limits
session.ts: +28/-0, only optional host-command composition, session/caller cancellation and 20-second deadlines. Move state and refresh fencing belong to the preference owner; encryption/signing, admission and confirmation belong to the development host.channel-sectionsandchannel-starsrecords. A move out of Starred confirms the target assignment (or removal) before clearing Star. Removal rereads the assignment head even when the local cache has no assignment. Only the completed move changes local placement.Changes
File changes
dev/relay-broker.mjs
Expose bounded, origin-checked assignment/Star routes with signed upstream requests, shared admission, per-relay serialization and confirmation reads.
dev/sidebar-preferences.mjs
Apply one group assignment to a fresh verified encrypted record, preserving unrelated raw fields and assignments rather than reconstructing from the lossy view.
dev/sidebar-preferences.d.mts
Describe the host decoder/assignment contract for typed callers and regressions.
dev/sidebar-stars.mjs
Validate explicit Star intent and retain the full encrypted channel map, including tombstones and monotonic entry timestamps.
dev/sidebar-stars.test.mjs
Cover encrypted roundtrips, no-ops, bounds, malformed/untrusted heads, failure and confirmation conflicts.
dev/sidebar-group-moves.test.mjs
Exercise real signed/encrypted records through the preference owner: direct moves, removal with a stale assignment cache, ordered-write failure/retry, refresh fencing, loading recovery and cancellation.
dev/sidebar-preference-writes.test.mjs
Exercise actual HTTP broker and transport routes with modeled relay I/O, including signed request scope/payload, origin rejection, publication failures and oversized responses.
docs/channels.md
Document ownership, cancellation, focus/retry behavior, supported hosts and the non-atomic cross-device conflict model.
src/bundled/channels/Channels.module.css
Retain compact row appearance with contextual highlighting.
src/bundled/channels/ChannelsPage.tsx
Offer one Move to… chooser for Starred and saved groups, plus named removal to Channels. Retain failure/retry state, keyboard invocation and identity-based destination focus.
src/bundled/channels/sidebar-sections.test.ts
Prove exclusive Starred projection and precedence over raw saved assignments. User-level removal clears the assignment; the signed-record move tests prove it does not restore an old group.
src/bundled/channels/useSidebarPreferences.ts
Expose session-owned mutation capabilities to the page without local preference ownership.
src/features/relay/session.ts
Wire the two optional host commands to session/caller lifetime and bounded deadlines only.
src/features/relay/sidebar-preferences-store.ts
Serialize complete moves, confirm the assignment before clearing Star, retain immutable confirmed placement, and fence intermediate refreshes, stale results and cancelled writes.
src/features/relay/sidebar-preferences-store.test.ts
Cover confirmed state, failed-save retry, initial-read admission, serialization, stale refresh success/failure, caller cancellation, clear and disposal.
src/features/relay/sidebar-preferences.test.ts
Retain assignment extraction regressions and protect raw-field preservation, decoder parity, confirmation and prototype-shaped channel IDs.
src/features/relay/sidebar-preferences.ts
Define narrow assignment and Star contracts alongside the existing read-only projection.
src/features/relay/transport.ts
Expose advertised host capabilities and validate/project successful host responses; preserve existing bounded error sanitization.
tests/browser/fixture.mjs
Accept verified encrypted section and Star publications independently through the production-broker fixture. Record the one intentionally failed Star request by exact URL rather than suppressing arbitrary console failures.
tests/browser/navigation-groups.spec.mjs
Exercise grouping, Starred/retry, direct moves and removal surviving reload in both engines; retain the existing warm-navigation frame/scroll regression.
Reproduction Steps
Validation
The tested uncommitted snapshot on
0e7bf1fwas committed unchanged as0cbb69ab8e33b5ac11b1e97d503b45f660b1fd09; commit-diff and new-test checksums were verified. No product changes were made for the replacement screenshots.bin/pnpm test:browser --project chromium --project webkit --no-deps navigation-groups.spec.mjs --workers=1: 6/6 passed, twice on the final code; latest run 1.4m local wall time. Actual built frontend + production broker against isolated modeled relay I/O, not native GUI or live-relay acceptance.0cbb69a: DCO Check passed. JavaScript, Windows native notifications, Semgrep and zizmor passed; browser measurements/journeys and Rust/tool integration were still running. No CI monitoring or merge-readiness claim.just scan. Hosted check status is a separate delivery snapshot, not a promise that CI is green. This remains draft; focused passes do not establish merge readiness.Screenshots / Demos
Actual built product UI in dark mode with the existing product palette, using isolated synthetic fixture data. Cropped to the feature; no real channel content or private identity. These replace the earlier overlay-style screenshots.
Move to…: Starred and Work share one chooser; Work is the current destination.