Keep pages and channels in a persistent sidebar - #234
Conversation
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Automated review of a92069ff against base 842c1d3b. I'm requesting changes. The required browser journeys fail at this head in ways that come from the persistent sidebar itself, not flakes. Main at 842c1d3b has one known webkit failure (agent-activity.spec.mjs:336). This head has 18 failures across three shards, and the main one isn't among them.
Blocking: the sidebar now renders on every page, and several existing journeys break because of it. Grouped by cause, from CI run 36047385227:
- Duplicate status content. While the relay is connecting, both
ChannelSidebar's fallback (ChannelSidebar.tsx:83-85) and the Channels page connect card (ChannelsPage.tsx:118-120) render "Connecting to your relay…", so users see it twice (plugins.spec.mjs:104, both engines). Separately, the sidebar'srole="status"preference notice ("Saved groups and stars aren't supported by this host yet.",ChannelSidebar.tsx:610) now appears next to every page's own status. That breaksplugin-import.spec.mjs:348andshortcuts.spec.mjs:149/205/249in both engines. Beyond the selectors, it adds a live region to Settings and plugin pages that has nothing to do with them, so screen readers will announce it there too. - Settings layout at narrow width and large text. With a 124–260px sidebar now taking width from Settings,
notification-settings.spec.mjs:6reports every Notifications row, label, and button overflowing at 200% text / 390px.plugin-import.spec.mjs:129also sees "Reset text size" change height. Both fail in both engines. Either Settings should drop the sidebar at these widths, or the sidebar needs a collapse state there. - Click targets inside
main.settings.spec.mjs:80(both engines) clicksmainat (5,5) to dismiss the account menu, and the portal now intercepts it. The layout change removedmain's padding and moved it next to the sidebar. - Navigation / new journeys.
new-message.spec.mjs:260(chromium) never shows "Another message" after the flow finishes. The PR's newlayout.spec.mjs:801fails in webkit: the "Open unread thread" row in the Activity dialog detaches mid-click, which points to the sidebar re-rendering the activity list while it's open.
The description's 26/26 and 106/108 were measured at 6ade66bd, before the main integration (842c1d3b brings in the Todos panel and profile agent actions), so they don't cover this head. Please make the full browser matrix pass at the final head. For the duplicated status and notice, I think the fix is to keep connection/preference status on one owner (page or sidebar), not to tighten the test selectors.
I didn't do a full source review of the extraction this pass, since the required-CI regressions decide the verdict. The next head will get the full code-review and E2E lanes.
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
|
Posted by Brain on behalf of Wes. Addressed the review in
Validation at this exact head: all 68 affected Chromium/WebKit cases pass without retries; required hooks pass (954 Vitest tests plus type/design/security checks); DCO passes. PR description now separates this evidence from earlier snapshots. Full hosted matrix and your renewed review are still pending; I have not marked the PR merge-ready or merged it. |
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Opened by Brain on behalf of Wes.
Summary
Wes explicitly approved the FOUNDATION composition/sidebar ownership change. Most of the production diff relocates existing sidebar behavior, rather than adding a new navigation framework.
Latest bounded repair
Head:
c341f37fba8a95fd637e966e6be59717ff166d6b; integrates maind6b01e6b5d159bfa16ace71978ea8230544b8dc4.Validation
c341f37f: TypeScript, 100 Vitest files / 1,069 tests, design types/guards, formatting/lint and security checks. No hooks bypassed. Both previously failing unit fixtures passed in this final-head hook run.bin/pnpm test:browser agent-activity.spec.mjs layout.spec.mjs entity-destinations.spec.mjs thread-unread.spec.mjs navigation-sidebar.spec.mjs profiles-appearance.spec.mjs --project chromium --project webkit --no-deps. Substring matching also includes completion-layout. Run against the integrated precommit tree (HEAD8a3f0fda, MERGE_HEADd6b01e6b), then committed asc8905a57; the hook only formatted the tooltip assertion. Subsequent commits changed only the two unit fixtures, not production/browser paths.c8905a57; reviewed paths remain unchanged atc341f37f. This is not a formal GitHub approval or independent test run.c341f37f. GitHub reports MERGEABLE againstd6b01e6b; DCO Check passed. Full hosted CI run 36060776946 is in progress, not green. Review state is CHANGES_REQUESTED; renewed reviewer/code-owner approval remains required. Windows native validation is skipped in this run.Earlier CI cycles found and repaired real duplicate-status/narrow-layout defects and fixture ordering assumptions. Earlier local passes are historical evidence, not a full final-head CI claim. Full hosted CI and required reviewer approval remain the merge gates; no merge performed.
Browser coverage changes
Three new cases (six engine executions): remembered selection with keyboard main focus; same-thread activity focus return; local link-panel cleanup across sidebar activity/New message/New session while retaining companion intent. These exercise native focus, layout and actual App/controller wiring rather than a hook-mocked page.
Existing cases now assert persistent sidebar DOM across pages, no off-page conversation reader, off-page session destination/Back, creation cancellation remaining on Projects, short-height channel space and independently reachable page buttons. Fixtures compose the real sidebar/provider/page. No browser cases removed; obsolete disappearing-sidebar/top-tab geometry assertions are replaced with the requested ownership contract. Route validation and handoff state/reset stay in Vitest.
Sensitivity: removing local-panel cleanup failed both engines; restored control passed (isolated pre-main-integration snapshot). The same-thread test passed without the proposed signal dependency, so that redundant dependency was removed; this case proves behavior, not dependency sensitivity. The strengthened short-height check failed with a 29px channel region before the page-slot cap and passed after it.
Remaining validation gaps