📱 feat: Give the Mobile Drawer One Place for Each Control - #16248
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b17112431d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!showSearch) { | ||
| return null; |
There was a problem hiding this comment.
On devices with a nonzero bottom safe-area, this return removes the drawer's only env(safe-area-inset-bottom) padding whenever search is disabled or the active panel is prompts, memories, MCP, and so on. The footer previously remained mounted because it also contained New Chat, so the panel stayed above the home indicator; now bottom rows and actions can render beneath the system gesture area. Keep a safe-area spacer or move the padding to the drawer independently of search visibility.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| "client/src/components/Nav/AgentMarketplaceButton.tsx": { | ||
| "shadcn/no-restyle": { | ||
| "count": 1 | ||
| } |
There was a problem hiding this comment.
This adds a fresh shadcn/no-restyle allowance for AgentMarketplaceButton, masking the new Button override instead of using or extending an appropriate shared primitive variant. The repository explicitly prohibits widening eslint-suppressions.json to land a restyle, so the component should be corrected and this entry removed.
AGENTS.md reference: AGENTS.md:L88-L94
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0f9be8d. The row uses the Button's shared row size with layout-only classes, and the AgentMarketplaceButton suppression entry is gone. The design-rule suppressions gate passes, and the BottomBar no-restyle entry drops by 4.
danny-avila
added
the
🗺️ Chat UI Shell
b171124 to
0f9be8d
Compare
|
@codex review Please review the current PR head 0f9be8d. State the exact reviewed commit and ignore findings that apply only to earlier heads. Purpose supplied by the requester: rebuilt on canary; 3 fixes since reviewed head b171124: safe-area spacer when search is absent, marketplace row on the shared row size (suppression removed), empty marketplace wrapper hidden; plus scrim role and dark drawer edge (berry-13#199) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f9be8dda2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| > | ||
| {/* The dialogs' theme-owned scrim role, painted on its own layer so the | ||
| button itself carries no color of its own. */} | ||
| <span aria-hidden="true" className="bg-scrim absolute inset-0" /> |
There was a problem hiding this comment.
At reviewed head 0f9be8dda29644c2524342e90ce7284e9032b7bb, when the drawer-strip setting is enabled and a keyboard user focuses the dismiss scrim, this absolutely positioned bg-scrim child is painted over the Button's inset box-shadow, substantially obscuring the only visible focus indicator under the default 75–80% scrim opacity. The new browser test only checks that boxShadow is computed, which remains true while the shadow is hidden beneath this child; paint the fill on the button or place the focus indicator above the fill.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dd0bd19. The indicator now draws on a layer above the fill. Fixing that exposed a second problem: at 80% a single dark ring reaches only 2.3:1 on the light palettes' mid-gray, so it is two-tone (a surface band plus the focus ring). The scenarios mobile-drawer-scrim-focus-{default,clickhouse}-{light,dark} hit-test the topmost layer at the scrim's edge and require a tone at 3:1 against the fill, rather than reading the computed box-shadow.
dd0bd19 to
875516f
Compare
|
@codex review Please review the current PR head 875516f. State the exact reviewed commit and ignore findings that apply only to earlier heads. Purpose supplied by the requester: 1 fix since reviewed head 0f9be8d: the scrim focus indicator paints above the fill, two-tone so one tone reaches 3:1 in all four palettes (finding 4153260127); rebased on canary with no content change |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review Please review the current PR head cab7354. State the exact reviewed commit and ignore findings that apply only to earlier heads. Purpose supplied by the requester: 1 change since clean head 875516f: the drawer close toggle keeps canary's header-action surface (CI scenario mobile-drawer-close-toggle-stays-the-same-control from #15894) |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The drawer put its two global destinations in opposite corners and repeated one of them under every panel. The marketplace was a bare icon in the top strip, the hardest thing there to notice; new chat was a large footer button sitting under prompts, memories and MCP settings alike, none of which it has anything to do with; and the close toggle wore the chat header's bordered plate beside two flat icons, so the row read as one control and two afterthoughts. They swap. New chat takes the icon slot beside the panel switcher, which is where the drawer keeps what means the same thing whichever panel is showing, and its click behaviour moves with it into a component of its own rather than staying behind in the footer. The marketplace becomes a full-width entry above the panel content, labelled, in the list where someone already looks for somewhere to go, and outside the scroller so it stays put while the panels change under it. The toggle drops to `ghost` like its neighbours. The footer is left with search alone and stands down entirely on a panel that has nothing to search, rather than taking its padding out of the list above it. The drawer now carries a text colour beside its background. Only the panel nav set one, so the header strip, the new marketplace row and the footer all inherited black from the document and rendered near-invisible in dark mode, except where a control happened to name a token itself. That was hidden while the toggle wore `header-action`, which brought its own.
…part from its scrim The footer now renders a safe-area spacer when search is absent, so panel content no longer runs under the system gesture area on panels with nothing to search. The marketplace row uses the Button's `row` size instead of restyling it, which drops its new lint suppression. The scrim paints the theme's `bg-scrim` role (80% by default) on its own layer, which separates the drawer by 3:1 in both light palettes, and in dark mode the drawer draws a `border-xheavy` edge, since no scrim opacity separates two near-black surfaces. The scrim's focus ring is checked in the browser instead of by class name.
The scrim's theme fill sits on its own layer, which painted over the button's inset focus ring, so a keyboard user focusing the scrim with the chat strip on saw almost none of it. The indicator now draws on a layer above the fill, in two tones: at the theme's 80% scrim a single dark ring reaches only 2.3:1 on the light palettes' mid-gray, so a surface band carries it in light and the focus ring in dark. The scenarios check, in all four palettes, that the topmost layer at the scrim's edge carries an indicator with a tone at 3:1 against the fill.
The drawer's close toggle is the chat header's open toggle flipping state, so it keeps the header-action surface and size it shares with that control rather than going flat like its new neighbours.
cab7354 to
152bb9a
Compare
* feat(client): Give the mobile drawer one place for each control The drawer put its two global destinations in opposite corners and repeated one of them under every panel. The marketplace was a bare icon in the top strip, the hardest thing there to notice; new chat was a large footer button sitting under prompts, memories and MCP settings alike, none of which it has anything to do with; and the close toggle wore the chat header's bordered plate beside two flat icons, so the row read as one control and two afterthoughts. They swap. New chat takes the icon slot beside the panel switcher, which is where the drawer keeps what means the same thing whichever panel is showing, and its click behaviour moves with it into a component of its own rather than staying behind in the footer. The marketplace becomes a full-width entry above the panel content, labelled, in the list where someone already looks for somewhere to go, and outside the scroller so it stays put while the panels change under it. The toggle drops to `ghost` like its neighbours. The footer is left with search alone and stands down entirely on a panel that has nothing to search, rather than taking its padding out of the list above it. The drawer now carries a text colour beside its background. Only the panel nav set one, so the header strip, the new marketplace row and the footer all inherited black from the document and rendered near-invisible in dark mode, except where a control happened to name a token itself. That was hidden while the toggle wore `header-action`, which brought its own. * fix(client): Keep the mobile drawer clear of the home indicator and apart from its scrim The footer now renders a safe-area spacer when search is absent, so panel content no longer runs under the system gesture area on panels with nothing to search. The marketplace row uses the Button's `row` size instead of restyling it, which drops its new lint suppression. The scrim paints the theme's `bg-scrim` role (80% by default) on its own layer, which separates the drawer by 3:1 in both light palettes, and in dark mode the drawer draws a `border-xheavy` edge, since no scrim opacity separates two near-black surfaces. The scrim's focus ring is checked in the browser instead of by class name. * fix(client): Paint the mobile scrim's focus indicator above its fill The scrim's theme fill sits on its own layer, which painted over the button's inset focus ring, so a keyboard user focusing the scrim with the chat strip on saw almost none of it. The indicator now draws on a layer above the fill, in two tones: at the theme's 80% scrim a single dark ring reaches only 2.3:1 on the light palettes' mid-gray, so a surface band carries it in light and the focus ring in dark. The scenarios check, in all four palettes, that the topmost layer at the scrim's edge carries an indicator with a tone at 3:1 against the fill. * fix(client): Keep the mobile drawer close toggle on the header's control The drawer's close toggle is the chat header's open toggle flipping state, so it keeps the header-action surface and size it shares with that control rather than going flat like its new neighbours. (cherry picked from commit 0da8987) Original-PR: #16248
2 participants
Pull Request
Summary
On a phone, the sidebar drawer split its two global destinations across opposite corners and repeated one of them under every panel. The Agent Marketplace was a bare icon in the header strip, the easiest thing there to miss. New chat was a large footer button sitting under prompts, memories and MCP settings, none of which it relates to. Text in the header strip and footer also inherited black from the document, so it was near-invisible in dark mode.
This PR gives each control one place. New chat becomes an icon beside the panel switcher, where the drawer keeps what means the same thing on every panel. It starts a chat and closes the drawer. The marketplace becomes a labelled full-width row above the panel content, outside the scroller, so it stays put while panels change. The footer holds search alone and shrinks to the bottom safe-area spacer on a panel with nothing to search. The drawer sets
text-text-primary.It also folds in the drawer's scrim and boundary follow-ups (berry-13#105, berry-13#145, berry-13#199, berry-13#65). The scrim now paints the theme's
bg-scrimrole (80% by default, Click UI's 75% in ClickHouse) instead of a fixed 50%, which separates the drawer by 3:1 in both light palettes. In dark mode no scrim opacity separates two near-black surfaces, so the drawer draws aborder-xheavyedge (6.1:1 default, 3.7:1 ClickHouse). The scrim's keyboard focus indicator paints above the fill in two tones, a surface band and the focus ring, because a single dark ring falls to 2.3:1 on the 80% light scrim. Audit criteria: themable C9 (fiveno-restylesuppressions removed, none added), themable C4 (the scrim role gains a consumer beyond the dialogs), ClickHouse C11 (the drawer defect left over from #105).Type of change
Testing
Tested environments/configuration:
reviewctl verify): desktop light, desktop dark and mobile emulationAutomated tests:
e2e/specs/mock/scenarios/mobile-drawer-controls.spec.ts(14 scenarios, desktop light, desktop dark and mobile, 42 passing): New chat placement and behavior, the marketplace row's position across panel changes and its navigation, no gap when the marketplace is off, the footer standing down, dark-mode text contrast, the drawer boundary in four palettes, and the scrim role plus a focus indicator that paints above the fill at 3:1 in four palettesreviewctl precheckagainstorigin/canary(ESLint, Prettier, import order, design-rule suppressions, typecheck, related jest): passnpx jest src/components/UnifiedSidebar src/components/Nav/__tests__/AgentMarketplaceButton.spec.tsx(client): 11 suites, 57 tests passnpx tsc --noEmit -p client/tsconfig.json: cleanScreenshots / recordings
Phone width (390px), drawer open, loaded at that width. Each image is before (canary) then after, light then dark.
Default theme:
ClickHouse theme:
With the chat strip setting on, which shows the scrim (dark mode also shows the drawer's new edge):
Risk / compatibility
The scrim is darker than before (80% instead of 50% by default), deliberately, to meet the 3:1 boundary. Chromium cannot emulate a nonzero
env(safe-area-inset-bottom), so the home-indicator spacer is covered by the unit test and by inspection rather than in the browser.Checklist