Polish the channel sidebar - #235
Conversation
18c56e7 to
03d31a5
Compare
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
d0a793e to
b18c3d1
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Request changes: four P2 interaction/rendering regressions, detailed inline. Reviewed head b18c3d19813a3176ecdcf2b2e4ff4cdf13077860 against base 119195ea331de33c8480bab180df0091ca8e9421.
- Validation: focused compiled-app repros in Chromium and WebKit confirm inert unread destinations, final-DM focus loss, and header-action Tab order. Existing CI run 36068233790 independently confirms the invisible working dot in both engines. No production edits or broad local suite reruns.
- Separate CI/integration gates: the PR conflicts with main. Appearance’s white-text expectation and plugin-import’s global 32px-button assumption conflict with the approved sidebar design; update those assertions at their proper scope, not the approved UI. The Chromium inset assertion is scrollbar-engine-dependent (the failure screenshot shows a rounded, contained row); the exact intended inset still needs reconciliation. WebKit Settings’ 0.99297 viewport ratio remains unclassified, not proven inherited or a sidebar regression. Do not blanket-relax assertions.
- Exit criteria: fix the four inline defects, add behavior-level regressions in both browsers, resolve overlapping main changes, and reconcile the failing browser checks at the integrated head. The approved colors, density, and context-menu design are not being reopened.
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review, commenting via Wes’s GitHub account.
Targeted follow-up: the browser-regression selector finding still applies; the earlier Settings-sidebar finding is superseded.
Reconciled the two findings from the source review at db9270e5cf00e88da50d085d5c1e312182625dec against current head 3bf9fb78a690f2c8d4fc6ddf71ade74802015d45 and base 8c1a4ca64734f0fb6a9a49e49d147e649c8eb135 before publishing. The remaining P2 test-migration issue has two concrete locations, detailed inline.
The old SettingsSidebar.tsx and its unconditional absent-card redirect have been removed. The current Settings selection effect no longer has the startup behavior identified in that earlier finding, so I am not asking you to fix that obsolete code.
Scope / exit criteria: this is a source-only reconciliation of those findings, not a fresh exhaustive review of all intervening changes. I ran no tests, builds, app workflows, or PR code and did not verify current CI. Repair the selectors against the rendered structure/current navigation while retaining the behavior assertions, then validate the affected full browser files in Chromium and WebKit at the integrated head. The prior browser/integration exit criteria remain open; the approved colors, density, and context-menu design are not being reopened. This is non-blocking COMMENT feedback, not approval or a changes-requested review.
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
…sh-pr1 * origin/main: Refine direct message opening (#107) feat(messages): report messages to community moderators (#255) perf(channels): stop rerendering message rows after each channel switch (#269) Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
Signed-off-by: Peppermint Butler <29b529ff43be424db89f51bff58a38cf86942acbffff98a14ce50b38c2a57d8f@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review, published through Wes’s GitHub account.
Reviewed head 283b981e56f28f7622eb8cf338f4b0705ebf898d against base/merge-base e44b1e44f35b881b2dcaa44147d2ab607384b058, using pinned Git objects only (no dirty source inputs).
One new P2 finding: the new Inbox/Bestie placeholder routes suppress the responsive side-panel layout while companion/channel-side panels can remain rendered; see the inline comment.
Previous review reconciliation: the section-disclosure selector in sidenav-polish.spec.mjs:140–142 now targets the outer section and actual content, addressing 4106940027 at source level. The existing compose-navigation/DM-section selector finding, 4106940034, remains unresolved at this head; I have not duplicated that inline finding. The earlier unread expansion, working-indicator layout, final-DM focus destination, and header Tab-order repairs are present in source.
Validation: git diff --check passed for these pins. Source review only: no PR code, tests, builds, or app workflows executed, and current CI was not assessed. Browser behavior in Chromium/WebKit, responsive geometry, native-app acceptance, and runtime verification of the earlier repairs remain unverified. This COMMENT is non-blocking feedback, not approval or merge authorization.
Preserve memoized row menus and agent status recovery while retaining the approved sidebar design. Align companion layout with rendered panels and migrate browser navigation to header search. Co-authored-by: Mongo <81cabd2ca1792494372ed410283c00770c0cf35afca052e940fa522883212b45@buzz.block.builderlab.xyz> Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Preserve the shared panel hover treatment and the separate sidebar navigation role. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
…overage Co-authored-by: Mongo <81cabd2ca1792494372ed410283c00770c0cf35afca052e940fa522883212b45@buzz.block.builderlab.xyz> Co-authored-by: Princess Donut <5d97ac8c272fa949af56c71e586146b1706e5c4c3daa125db5b4625e12e0686c@buzz.block.builderlab.xyz> Co-authored-by: Mordecai <f314b6033b6e13df94c8e9e1187d3666b6b9ce54e66ee986e31cdaa909e8dcbc@buzz.block.builderlab.xyz> Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Keep the compact section layout and item-owned menus while integrating group placement, create-section, focus recovery, and shared group icons. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Keep page-specific focus ownership while returning search selection to main. Update section, navigation, and geometry assertions without dropping lifecycle or keyboard coverage. Co-authored-by: Princess Donut <5d97ac8c272fa949af56c71e586146b1706e5c4c3daa125db5b4625e12e0686c@buzz.block.builderlab.xyz> Co-authored-by: Mongo <81cabd2ca1792494372ed410283c00770c0cf35afca052e940fa522883212b45@buzz.block.builderlab.xyz> Co-authored-by: Mordecai <f314b6033b6e13df94c8e9e1187d3666b6b9ce54e66ee986e31cdaa909e8dcbc@buzz.block.builderlab.xyz> Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at 79e0bb0d9ba53f704730dba4a8a9f770e1b58bc9, against base c379c33c342c39f0c91c3f4ec487d076cf4de86a.
P1: replace the PR-description screenshot with sanitized fixture data. The opening before/after image exposes an internal relay hostname, real workspace channel names, user identities and conversation content in this public PR. Changing its host does not sanitize its pixels. Remove the unsanitized image from the description and replace it with synthetic/demo data; ask the repository/attachment owner to handle the already-published attachment. I am deliberately not reproducing those details here.
Two P2 test-migration regressions are detailed inline. Both fail in Chromium and WebKit in the exact-head CI run. Repair the unread-selection contract and Agents scroller selector, then run both affected complete browser files in both engines without weakening their assertions. These failures do not establish a product navigation or scrolling defect.
The earlier disclosure/inert, header tab-order, working-cue, final-DM focus-loss, compose and placeholder-layout repairs check out in source. All delegated lanes have returned. Validation here was source/test inspection plus hosted CI logs and failure traces, not local suite reruns or native acceptance. JavaScript, Rust/tool integration and browser measurements passed; required CI is red.
Optional, non-blocking cleanup:
- Final-DM removal should focus the surviving Direct messages summary/New message action, rather than the first other section.
- Delete the unreachable hover-X/onHideDm and hideTitle branches; drop the duplicate unread.ensure call already owned by useSidebarStartup. Retain the removed collapse-cue and narrow-Settings focus assertions when updating fixtures.
- Commit attribution includes an internal domain. This needs owner-managed metadata cleanup, not a separate product-code blocker.
Exit criteria: sanitized public media, corrected tests with both-engine evidence, and required CI green. No approval or merge is implied.
Preserve actual scroll containment and Alpha selection in browser journeys. Cover fixed-width channel renames and explicit unavailable destination styling. Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord's automated source review (via Wes's account)
No new actionable findings in the subgroup-emoji spacing follow-up. This is a bounded review of the three-file, 27-line change after feedback-covered 435d1db808ea4808d75bc20ae4983d2ba10afecf, not a fresh approval of the entire sidebar change.
Channels.module.css:261–263adds one existing spacing token at the section-label boundary. The icon wrapper owns both Unicode and custom-image content (SidebarGroupIcon.tsx:19–27); the selector does not alter the menu icon spacing or introduce another component/state owner.- The fixture's additional Unicode group is gated by the existing
sidebarIconsoption. The existing browser journey now measures icon-to-text separation and unchanged header height for both custom and Unicode icons; it retains the loading, menu, narrow-dialog and focus assertions. Browser geometry is the appropriate layer for these assertions. No browser case was added or removed by this latest spacing commit. - The earlier test migrations remain present: the unread journey preserves Alpha selection while revealing/focusing the DM, and the Agents journey targets its actual scrolling element. This source check does not independently establish their reported browser passes.
Prior gates remain separate. The current description no longer embeds the previously flagged screenshot, but owner-managed handling of the already-published attachment/history remains explicitly unresolved. I did not retrieve or republish that image. Previously deferred optional cleanup/commit-metadata items are not reopened by this spacing review. Human/native acceptance remains outstanding; this comment does not dismiss previous reviews or clear their exit criteria.
Pinned scope: head 3f9075e53e9e49f48bd9b2431a7b0354153014c5; base/merge-base c379c33c342c39f0c91c3f4ec487d076cf4de86a. Extracted source bytes matched the pinned Git blobs; full-PR and follow-up whitespace checks passed. All 40 commits in the base-to-head range contain a DCO trailer.
Validation limits: source only—no tests, builds, app launches, live relay operations or PR-code execution. The PR's browser/hook results are author reports, not independently rerun here. One exact-head check snapshot showed DCO/Semgrep/zizmor successful, JavaScript/Rust/browser lanes still in progress, and Windows validation skipped; no polling or CI-green claim. Rendered spacing, native behavior and human acceptance remain unverified. This COMMENT is not an approval or merge authorization.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 No further findings at 3f9075e. The prior Inbox/Bestie disabled-state and rename-overflow issues are addressed in the incremental diff. Approving the reviewed change. Full Vitest: 4200 passed, 1 unrelated child-process timeout in dev/vite-config.test.mjs; browser suite not independently run.
PR #235 replaced the shell's plugin-aware page list with three hardcoded rows (Inbox, Bestie, Agents). Inbox and Bestie opened "Content coming soon" placeholders, and Bestie showed even with the buzz.bestie plugin disabled. Projects, Workflows, Sessions and external plugin pages were reachable only through search. App now passes the shell's page navigation back through the sidebar render prop, and ChannelSidebar hosts it above the channel list in the ready, connecting and error states. The hardcoded rows and the agentsEnabled prop are gone. The shell renders page icons in an 18px box at the compact size so rows line up with sidebar sections, and the page list no longer scrolls on its own; it scrolls with the roster. The Inbox and Bestie placeholder routes stay valid for deep links. Browser specs open them by address through a new openChannelPlaceholder helper. A new App-level test checks that the sidebar lists only active plugin pages and drops a row when its plugin is disabled. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Inbox had no plugin and Bestie registered only a companion panel, so neither could declare a sidebar row once rows became opt-in. The rows PR #235 drew by hand opened "Content coming soon" placeholders inside the Channels page through the Inbox and Bestie route params. A new buzz.inbox plugin registers an Inbox page, and buzz.bestie registers a Bestie page beside its panel. Both are workspace pages with primary: true, so each gets a sidebar row that disappears with its plugin. Inbox keeps the placeholder copy; the Bestie page shares the companion card's body until agent chat connects. The shell ranks them after Messages and before Projects, gives Inbox the bell icon, and draws Bestie's artwork through a new BestieIcon in the icon gateway so the search palette's icon contract still holds. The Channels placeholders are gone. Channel routes no longer accept the Inbox or Bestie params, ChannelsPage drops the placeholder branch and its style, and leaving Settings with no prior destination now opens plain Messages rather than the Inbox placeholder. Browser specs open the new pages from their sidebar rows and the openChannelPlaceholder helper is removed. Tests cover the ordering, the icons, the rejected route params, bundled page counts and the App-level sidebar rows. Docs list the new rows and the inbox plugin. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
PR #235 replaced the shell's plugin-aware page list with three hardcoded rows (Inbox, Bestie, Agents). Inbox and Bestie opened "Content coming soon" placeholders, and Bestie showed even with the buzz.bestie plugin disabled. Projects, Workflows, Sessions and external plugin pages were reachable only through search. App now passes the shell's page navigation back through the sidebar render prop, and ChannelSidebar hosts it above the channel list in the ready, connecting and error states. The hardcoded rows and the agentsEnabled prop are gone. The shell renders page icons in an 18px box at the compact size so rows line up with sidebar sections, and the page list no longer scrolls on its own; it scrolls with the roster. The Inbox and Bestie placeholder routes stay valid for deep links. Browser specs open them by address through a new openChannelPlaceholder helper. A new App-level test checks that the sidebar lists only active plugin pages and drops a row when its plugin is disabled. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Inbox had no plugin and Bestie registered only a companion panel, so neither could declare a sidebar row once rows became opt-in. The rows PR #235 drew by hand opened "Content coming soon" placeholders inside the Channels page through the Inbox and Bestie route params. A new buzz.inbox plugin registers an Inbox page, and buzz.bestie registers a Bestie page beside its panel. Both are workspace pages with primary: true, so each gets a sidebar row that disappears with its plugin. Inbox keeps the placeholder copy; the Bestie page shares the companion card's body until agent chat connects. The shell ranks them after Messages and before Projects, gives Inbox the bell icon, and draws Bestie's artwork through a new BestieIcon in the icon gateway so the search palette's icon contract still holds. The Channels placeholders are gone. Channel routes no longer accept the Inbox or Bestie params, ChannelsPage drops the placeholder branch and its style, and leaving Settings with no prior destination now opens plain Messages rather than the Inbox placeholder. Browser specs open the new pages from their sidebar rows and the openChannelPlaceholder helper is removed. Tests cover the ordering, the icons, the rejected route params, bundled page counts and the App-level sidebar rows. Docs list the new rows and the inbox plugin. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Why
The channel sidebar needed a denser, clearer visual hierarchy while preserving navigation, unread state, resizing, and keyboard access.
What
How
The sidebar keeps the existing panel, navigation, session, and preference owners. New section and destination components isolate presentation without adding persistent state. Native scrolling keeps a stable gutter, and sidebar icon actions share the row hover fill.
Risk
This changes a live navigation surface and its browser fixtures. The main risks are focus behavior, narrow-width layout, unread cues, and conflicts with recent changes on
main.Testing
Cynthia reviewed and approved the labeled native build during visual iteration. The takeover repairs still need human/native acceptance; no new human acceptance is claimed.
At
e0a6fc00plus the source changes committed as79e0bb0d:dev/vite-config.test.mjs; the full rerun passed.79e0bb0d: format/lint, app types, 79 files / 919 related tests, design types and all design guards passed. Hosted DCO Check passed at the pushed head. Broader CI is pending, not a claimed pass.Browser coverage changes preserve the existing behavior assertions while migrating removed Pages navigation to header search and section selectors to the new sibling-content DOM. Added responsive companion geometry coverage requires a browser (jsdom cannot measure grid/overlay layout); the source regression was reproduced before repair and passes in both engines after repair. No browser cases were removed during the takeover. Search focus adds pointer/keyboard component regressions and retains browser focus coverage. A full browser/native release validation was not rerun locally.
Takeover update
Updated the existing branch to
79e0bb0d; GitHub reports no merge conflict. Integrated overlapping main work through #207, preserving memoized row menus, group moves/create-section focus, and agent-status recovery. Addressed the remaining review threads for disclosure selectors, compose navigation, and placeholder side-panel layout, plus the search-to-page focus regression uncovered during validation.The subsequently fetched #249 message-ordering change was inspected and does not overlap these sidebar edits; it was not pulled into another local validation cycle. Hosted CI remains responsible for the current merged tree. No approval or merge is implied by this update.
Review and CI follow-up (2026-09-25)
435d1db8: Inbox/Bestie have explicit unavailable/disabled states; fading labels observe text/descendant changes as well as resize. Repaired both CI test migrations: Agents targets its real scroller, unread cues preserve established Alpha selection without selecting the focused DM or publishing reads.3f9075e5: add a 4px token-based gap between subgroup emoji and title, preserving the 28px header. Applies to Unicode and custom emoji.Verification
79e0bb0dplus the changes committed in435d1db8: complete Agents, sidebar-unread and sidenav-polish browser files pass 22/22 in Chromium/WebKit. After final disabled styling, complete sidenav-polish passes 8/8; after the explicit Alpha setup/DM negative assertion, complete sidebar-unread passes 12/12. The rename test demonstrably fails against the pre-fix source. The later TypeScript fixture correction was covered by push hooks.435d1db8plus the spacing changes committed in3f9075e5: complete navigation-group-icons file passes 2/2 in Chromium/WebKit, checking actual 4px text separation for both Unicode/custom icons and unchanged header height.Remaining gates and disclosure
The original unsanitized screenshot has been removed from the current PR description. The repository/attachment owner still needs to handle the already-published GitHub attachment and any retained history/copies. It has not been rehosted. Replacement evidence was generated entirely from synthetic fixture data and is retained locally, not committed; the approved screenshot upload tool is unavailable in this session, so no replacement public image is claimed.
The separate Agents null-scope behavior during a connecting/error community remains disclosed for follow-up; it was not silently broadened into these four repairs. Human/native acceptance of the follow-up and required hosted CI remain gates. No approval or merge is implied.