Repository navigation
AIDA-2338: Keep sidebar headings readable at large sizes - #2263
Conversation
Co-authored-by: BuzzPRReviewAssistant1 <a581a7e8115222acdc32ffb14bfce55d48c24478d98e155f8eba126b69b83c7b@buzz.block.builderlab.xyz> Signed-off-by: Ravneet Arora <rarora@squareup.com>
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Three layout/behavior regressions remain at this commit. The changed browser files pass, but existing overflow and section-creation cases fail; reverting only the CSS to the comparison base makes both pass.
| .sectionHeader > details { | ||
| flex: 1 1 9rem; | ||
| min-width: 0; | ||
| } |
There was a problem hiding this comment.
🤖 [P2] Size the disclosure from its content before wrapping actions
At 100% interface size and a 220px sidebar, the fixed 9rem basis pushes the actions onto a second row even for “Channels”, doubling the header from 32px to 64px. Because the actions remain transparent until hover/focus, this appears as empty space. At 200%, measured Channels/DM headers grow from 64px to 168px/208px. Let actual title width determine wrapping (or preserve appropriately scaled reserved action space). Add a regression assertion that short titles and actions share a row when they fit; the new separation assertion currently accepts the extra row. Fix suggestions have not been tested.
There was a problem hiding this comment.
🤖 Fixed in 3cbc353. Removed the fixed 9rem basis and wrapping header layout, restoring the original single-row height. The action reservation is now capped by available width using shared sizing tokens. The new geometry case checks compact height, action alignment, and the full disclosure target across five interface sizes and three sidebar widths. Restoring the reviewed stylesheet makes that case fail on compact height; the revision passes in Chromium and WebKit.
| } | ||
| .sectionHeader > details .sectionTitle { | ||
| white-space: normal; | ||
| overflow-wrap: anywhere; |
There was a problem hiding this comment.
🤖 [P2] Reconcile unconditional wrapping with the existing overflow contract
These rules change long section names from fading to wrapping even at 100%. The existing navigation-group-icons.spec.mjs:213 case fails in Chromium and WebKit CI: wrapped=true, fits=true, clearOfActions=false, versus expected false/false/true. A local Chromium run fails at this head and passes with only this stylesheet restored to the base. Preserve the default-size overflow behavior or explicitly update the intended product contract and its regression coverage; leaving the conflicting test failing is not sufficient.
There was a problem hiding this comment.
🤖 Fixed in 3cbc353. Restored the existing nowrap/FadingLabel behavior and default-size action clearance. The original overflow, clearOfActions, centering, and widened-recovery assertions in navigation-group-icons.spec.mjs are preserved and pass in both engines. Extended that same case to check useful text width and single-line fading for a long custom heading at 200%. No existing overflow assertion was weakened.
| } | ||
| /* Keep disclosure actions in flow so enlarged controls cannot consume the | ||
| label's entire width. The rem basis follows the interface-size preference. */ | ||
| .sectionHeader:has(> details) { |
There was a problem hiding this comment.
🤖 [P2] Preserve the narrow-sidebar create-section drag workflow
At 200% in a narrow sidebar, sidebar-section-creation.spec.mjs:382 fails to reveal [data-create-section-drop] during revealCreate (:228). This fails in Chromium CI and locally at the candidate, and passes locally when only Channels.module.css is reverted to the comparison base. The CSS dependency is reproduced; the precise collision mechanism remains unverified. Recheck and repair this drag workflow alongside the header geometry change before merging.
There was a problem hiding this comment.
🤖 Fixed the reproduction in 3cbc353 by restoring the original compact header geometry. The complete sidebar-section-creation.spec.mjs file passes in Chromium and WebKit, including the 200% narrow-sidebar case and its existing ResizeObserver gate. No drag collision code, observer gate, or assertion was changed. The latest hosted browser journey shards also pass. I have not claimed a more specific collision-mechanism diagnosis.
loganj
left a comment
There was a problem hiding this comment.
🤖 Larry: an independent adversarial review of 23d83b9. Source review only; I did not run the app at this head.
Verdict: comment, not approve. I agree with the three findings already posted on this head, and CI confirms two of them: navigation-group-icons.spec.mjs:213 (fade on overflow) fails in Chromium and WebKit, and sidebar-section-creation.spec.mjs:382 (narrow create-section drag) fails in Chromium. Fixing them probably changes the layout approach, so the result needs another look.
Must fix
- The two failing existing browser contracts above (error).
Additional findings (inline)
- The chevron moves to the far end of the disclosure at every size, 100% included (error, unless intended; then state it in the PR body).
- The summary's own
flex-wraplets the chevron drop to a row by itself, so a heading can take three rows (error). - An alternative that keeps one row and the fade contract: overlay the hidden actions as Inbox rows already do (advice, untested).
- The light/dark loop doubles the geometry matrix without covering a layout difference (advice).
Question: before this change the summary spanned the whole header, with padding under the actions, so a click in the empty action area toggled the section. Now that area is outside the summary, which is why navigation-sidebar.spec.mjs moved from the summary edge to the header edge. Is the smaller toggle target intended?
Co-authored-by: BuzzPRReviewAssistant1 <a581a7e8115222acdc32ffb14bfce55d48c24478d98e155f8eba126b69b83c7b@buzz.block.builderlab.xyz> Signed-off-by: Ravneet Arora <rarora@squareup.com>
|
🤖 Response to the review: pushed the fixes in 3cbc353 and replied to all three inline findings. Restored compact single-row headers and the existing fading/action-clearance contract; the narrow create-section drag case passes without changing its assertions or collision code. The four complete affected browser files passed 40 checks locally across Chromium and WebKit, and the latest hosted browser journey shards pass. The remaining native media-stream CI failure matches the known spool-progress race tracked by #1970; CI required remains blocked by that failure. |
|
🤖 Response to the review and collapse-target question: the smaller toggle target was unintended. In 3cbc353 the summary again spans the full header, and navigation-sidebar.spec.mjs uses its original summary-edge alignment assertion. The new case also clicks the trailing disclosure edge and checks keyboard menu/collapse independence. Removed the chevron auto margin, label growth, and all header/summary wrapping; preserved existing fading; adopted a bounded overlay at cramped enlarged widths; and removed the duplicate theme geometry loop. Replied to each inline comment with its specific change and coverage. All 40 local affected browser checks and hosted browser journey shards pass. Native CI remains blocked by the known spool-progress race tracked by #1970; desktop acceptance and re-review remain pending. |
loganj
left a comment
There was a problem hiding this comment.
🤖 Larry: re-review of 3cbc353 (delta from 23d83b9). Source review only; CI is green on this head.
All four of my findings are addressed. The clamped reservation keeps the summary's current padding at 100% and 120%. At 200%/220px it leaves about three control widths for the label and chevron, and the opaque action background covers the trailing text only while the actions show. This is one row at every size, the chevron sits next to the title again, the whole header width toggles the section, and the fade contract stays the same (navigation-group-icons.spec.mjs now also covers 200%). The new spec checks each of these directly.
Non-blocking advice: .sectionActions now paints --surface-panel. When a section is a drop target (.channelSection[data-drop-target], --affordance-panel-hover) and the pointer is over its header, the actions box can show as a separate rectangle on the tinted section. Check it during a channel drag; background: inherit, or a token that follows the section's state, would remove it.
Co-authored-by: Codex <noreply@openai.com> * origin/main: (327 commits) feat(bestie): remove your Bestie, and let remote agents be Bestie after a warning (#2041) feat(messages): open the sender's profile from the byline name (#2253) feat(native-mcp): one-command launcher and MCP registration for agent driving (#2272) perf(composer): stop per-keystroke chip re-renders and reparses in long rich drafts (#2251) fix(agents): prevent history from forging prompt requests (#2223) Enable automatic key backup in preview releases (#2325) feat(read-state): n-blob NIP-RS sync with a 30-day floor, frontier only (#1286) feat(huddles): provide read-only huddleStatus for optional consumers (#2327) feat(search): show verified names in people search (#2322) Fix voice-note sending and restore waveform previews (#2283) Preserve Solo queued messages across restarts (#1704) feat(plugins): resolve attachment renderers by order, like panels and links (#2284) fix(channels): keep muted DM names from becoming bold (#2184) fix: command-click opening desktop message links in the browser (#2288) AIDA-2390: Explain oversized paste rejection (#2271) AIDA-2399: Make failed uploads easy to retry or remove (#2265) AIDA-2338: Keep sidebar headings readable at large sizes (#2263) chore(plugins): export added-tool types and clarify upload docs (#2292) feat(solo): open chats for community viewing (#2107) feat(dev): read BUZZ_ settings from .env.local into the desktop launch environment (#2254) ... Signed-off-by: Daniel Rouleau <drouleau@squareup.com> # Conflicts: # docs/browser-testing.md # docs/local-artifact-storage.md # src/app/services.ts # src/features/artifact-storage/service.test.ts # src/features/artifact-storage/service.ts # src/features/artifact-storage/store.ts # src/features/communities/service.ts # tests/browser/artifact-storage.spec.mjs # tests/integration/browser-ci.test.mjs
…w-tab * origin/main: (21 commits) fix: prevent false leave failures after membership updates (#2310) feat(bestie): remove your Bestie, and let remote agents be Bestie after a warning (#2041) feat(messages): open the sender's profile from the byline name (#2253) feat(native-mcp): one-command launcher and MCP registration for agent driving (#2272) perf(composer): stop per-keystroke chip re-renders and reparses in long rich drafts (#2251) fix(agents): prevent history from forging prompt requests (#2223) Enable automatic key backup in preview releases (#2325) feat(read-state): n-blob NIP-RS sync with a 30-day floor, frontier only (#1286) feat(huddles): provide read-only huddleStatus for optional consumers (#2327) feat(search): show verified names in people search (#2322) Fix voice-note sending and restore waveform previews (#2283) Preserve Solo queued messages across restarts (#1704) feat(plugins): resolve attachment renderers by order, like panels and links (#2284) fix(channels): keep muted DM names from becoming bold (#2184) fix: command-click opening desktop message links in the browser (#2288) AIDA-2390: Explain oversized paste rejection (#2271) AIDA-2399: Make failed uploads easy to retry or remove (#2265) AIDA-2338: Keep sidebar headings readable at large sizes (#2263) chore(plugins): export added-tool types and clarify upload docs (#2292) feat(solo): open chats for community viewing (#2107) ... # Conflicts: # src/features/relay/outbox.ts
## Why At 200% interface size, a 220px sidebar can leave no width for “Channels” or “Direct messages”. The header reserves scaled button space inside a fixed-width sidebar. Fixes [AIDA-2338](https://linear.app/squareup/issue/AIDA-2338). ## What Cap the space reserved for actions so headings retain useful text width. Keep the existing single-row height, adjacent chevron, full collapse target, and fading for long names. ## How Normal widths retain the original action clearance. At cramped enlarged sizes, the padding leaves room for the label and chevron. Hovered or keyboard-focused actions cover the trailing text with the panel surface, following the existing Inbox overlay behavior. Shared buttons, menus, and saved sidebar widths keep their existing owners. Add one browser case for real text geometry, compact height, chevron placement, pointer targets, and keyboard focus. Extend the existing overflow-and-recovery case to cover a long custom name at 200%. No existing browser cases are removed. These contracts need a browser layout engine. The geometry matrix runs once; light and dark screenshots cover paint separately. ## Risk At the narrowest enlarged sizes, revealed actions cover the end of a heading. Long names still fade, and the full accessible names remain available. Default-size overflow and action alignment retain their existing contracts. ## Testing Local Chromium and WebKit fixtures exercised 100%, 120%, 150%, 180%, and 200% interface sizing at 220/260/520px sidebar widths. The complete heading, navigation, group-icon, and section-creation files passed 40 checks, including the previously failing overflow and narrow drag cases. Screenshots show the actual app UI with fixture messages at 100% and 200%, including both themes and revealed actions. Mutation checks restore the original CSS at 200%/220px and the reviewed wrapping layout: the new regression case rejects missing label space and extra header rows respectively. The checks pass with the revised CSS. All hosted browser journey shards passed at `3cbc353e1`. CI required is currently blocked by the native media-stream test failure at `media_stream_tests.rs:300`, matching the known spool-progress race tracked by [block#1970](block#1970). Human desktop acceptance remains pending. To try it: set Settings → Appearance → Interface size to 200%, narrow the sidebar fully, then hover and tab through section actions. Headers should remain compact; actions should open without collapsing their section. Generated with Codex --------- Signed-off-by: Ravneet Arora <rarora@squareup.com> Co-authored-by: BuzzPRReviewAssistant1 <a581a7e8115222acdc32ffb14bfce55d48c24478d98e155f8eba126b69b83c7b@buzz.block.builderlab.xyz>
Why
At 200% interface size, a 220px sidebar can leave no width for “Channels” or “Direct messages”. The header reserves scaled button space inside a fixed-width sidebar. Fixes AIDA-2338.
What
Cap the space reserved for actions so headings retain useful text width. Keep the existing single-row height, adjacent chevron, full collapse target, and fading for long names.
How
Normal widths retain the original action clearance. At cramped enlarged sizes, the padding leaves room for the label and chevron. Hovered or keyboard-focused actions cover the trailing text with the panel surface, following the existing Inbox overlay behavior. Shared buttons, menus, and saved sidebar widths keep their existing owners.
Add one browser case for real text geometry, compact height, chevron placement, pointer targets, and keyboard focus. Extend the existing overflow-and-recovery case to cover a long custom name at 200%. No existing browser cases are removed. These contracts need a browser layout engine. The geometry matrix runs once; light and dark screenshots cover paint separately.
Risk
At the narrowest enlarged sizes, revealed actions cover the end of a heading. Long names still fade, and the full accessible names remain available. Default-size overflow and action alignment retain their existing contracts.
Testing
Local Chromium and WebKit fixtures exercised 100%, 120%, 150%, 180%, and 200% interface sizing at 220/260/520px sidebar widths. The complete heading, navigation, group-icon, and section-creation files passed 40 checks, including the previously failing overflow and narrow drag cases. Screenshots show the actual app UI with fixture messages at 100% and 200%, including both themes and revealed actions.
Mutation checks restore the original CSS at 200%/220px and the reviewed wrapping layout: the new regression case rejects missing label space and extra header rows respectively. The checks pass with the revised CSS.
All hosted browser journey shards passed at
3cbc353e1. CI required is currently blocked by the native media-stream test failure atmedia_stream_tests.rs:300, matching the known spool-progress race tracked by #1970. Human desktop acceptance remains pending. To try it: set Settings → Appearance → Interface size to 200%, narrow the sidebar fully, then hover and tab through section actions. Headers should remain compact; actions should open without collapsing their section.Generated with Codex