Improve the style of the agent thinking card - #441
Conversation
Introduce a pulsing background effects module and forward it from includes. Add a new IconStatusIndicator component with styles (SVG-mask conic gradients + spin) and wire it into the Status view. Extend Button API and styles to support a new 'huge' size. Update multiple page_home styles to use raised surfaces, muted borders, pulsing backgrounds, and layout/typography tweaks (prompt font, placeholder color). Adjust button sizes in Intro and FollowUp to use the new sizes. Minor layout tweaks to indicator/message alignment.
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'huge' button size, a pulsing background effect, and a custom 'IconStatusIndicator' component, alongside visual updates to home page elements. The review feedback highlights several improvement opportunities: ensuring placeholder text colors remain distinct from typed input, using a robust border-radius approach for the 'huge' button to guarantee a pill shape, replacing a paragraph tag with a span to avoid layout issues from default margins, and using white instead of dark colors in SVG masks to prevent transparency issues in browsers defaulting to luminance mask mode.
- Add focus management and small UI tweaks on the home page: expose a textarea ref from PageHome to Prompt and autofocus the prompt input when status is hidden; autofocus the cancel button in Status on mount. - Add data-is-status-visible attribute and a fade transition to dim the prompt while status is visible. - Tweak status card spacing and padding (reduced gap and padding) and remove an unused tablet breakpoint rule.
- Adds useAutoFocus hook to centralize focusing behavior and use it in PageHome and Status. - Refactor pulsing-background into a parameterized mixin and add pulsing-card to share card shell styles used by intro, status, and follow_up. - Prevent prompt submission/focus while a status or follow-up panel is visible; wire disabled states for the prompt input and submit button. - Adjust button size styles for large/huge, tweak prompt typography, and simplify follow-up state handling in PageHome. - Minor SCSS cleanup and animation opt-outs.
- Remove the data-size="large" selector and simplify button SCSS so the pill border-radius applies consistently. - Update embedded SVG mask colors in status indicator SCSS from #231f20 to #ffffff to ensure correct masking/contrast. - Replace a block <p> with an inline <span> for the status text to avoid unintended spacing and improve layout.
nick-nlb
left a comment
There was a problem hiding this comment.
Thank you for this update Pablo! Looks great. Some comments below.
| import { mergeClassNames } from '~/functions/merge_class_names'; | ||
| import s from './status_indicator.module.scss'; | ||
|
|
||
| type StatusIndicatorProps = ComponentPropsWithRef<'div'>; |
There was a problem hiding this comment.
Props should match component name IconStatusIndicatorProps, and prefer interface over type where possible (see conventions in FRONTEND.md.
|
|
||
| export const IconStatusIndicator = ({ | ||
| className, | ||
| ...rest |
There was a problem hiding this comment.
Other icons set aria-hidden="true" directly within the icon, whereas in this case, it is set to be true as a prop here. Is this intentional? If so, can we comment why (or otherwise, let's update this to match existing icons where it is set within).
| @include type-label-large; | ||
|
|
||
| padding-block: 16px; | ||
| font-size: 20px; |
There was a problem hiding this comment.
This is a hard-coded override of the font-size and line-height. Can we move this to a mixin, following the pattern of the other sizes?
| border-radius: calc(var(--button-pill-size-large) / 2); | ||
| } | ||
|
|
||
| // "huge" shares large's padding/gap above but wants a fully pill-shaped |
There was a problem hiding this comment.
Will this always be the case? I.e., as a matter of convenience, huge might currently share some attributes with large, but is that definitional for huge?
Otherwise, this could be separated into two, with coincidentally shared features entered separately.
| border-radius: var(--border-radius-large) var(--border-radius-medium); | ||
| box-shadow: var(--shadow-elevated); | ||
|
|
||
| @include pulsing-card(54px, false); |
There was a problem hiding this comment.
Can we convert this from a magic number?
Address comments, improve focus handling for status component. - Adjust status panel accessibility and refine UI styles: focus the status container on mount (useAutoFocus -> containerRef), mark the status icon aria-hidden via component, and remove visual outline for programmatic focus. - Update button sizixng: separate large/huge rules so huge renders as a full pill and use type-label-huge for huge-sized labels. - Add type-label-huge mixin. - Minor style tweaks: placeholder color change, pulsing-card height variable for notice, and SCSS cleanup. - Also includes Next build trace artifacts (.next/trace, .next/trace-build).
nick-nlb
left a comment
There was a problem hiding this comment.
Thank you for the updates!
Some comments attached to this review.
| @@ -0,0 +1 @@ | |||
| [{"name":"generate-buildid","duration":268,"timestamp":82282366368,"id":4,"parentId":1,"tags":{},"startTime":1787087319015,"traceId":"c5697aaaa0ba516f"},{"name":"load-custom-routes","duration":466,"timestamp":82282366761,"id":5,"parentId":1,"tags":{},"startTime":1787087319015,"traceId":"c5697aaaa0ba516f"},{"name":"create-dist-dir","duration":387,"timestamp":82282367254,"id":6,"parentId":1,"tags":{},"startTime":1787087319016,"traceId":"c5697aaaa0ba516f"},{"name":"clean","duration":312,"timestamp":82282368559,"id":7,"parentId":1,"tags":{},"startTime":1787087319017,"traceId":"c5697aaaa0ba516f"},{"name":"next-build","duration":900010,"timestamp":82281469012,"id":1,"tags":{"buildMode":"default","version":"16.3.1","bundler":"turbopack","failed":true},"startTime":1787087318118,"traceId":"c5697aaaa0ba516f"}] | |||
There was a problem hiding this comment.
This file should be removed (it may be the result of running a build in the root of the project).
| @@ -0,0 +1 @@ | |||
| [{"name":"next-build","duration":900010,"timestamp":82281469012,"id":1,"tags":{"buildMode":"default","version":"16.3.1","bundler":"turbopack","failed":true},"startTime":1787087318118,"traceId":"c5697aaaa0ba516f"}] | |||
There was a problem hiding this comment.
This file also should be removed.
| // so its radius isn't tied to (and shouldn't scale with) that height. | ||
| &[data-size="huge"] { | ||
| column-gap: 6px; | ||
| min-height: var(--button-pill-size-large); |
There was a problem hiding this comment.
This design token is still the one taken from the large button. Even if it happens to be the same, they represent semantically different values and should be separated.
Does this min-height have any effect in practice as it is? If the height of the button is always larger that the large min-height? (in practice this wouldn't matter if we move to a token for extra-large with the correct min-height).
| interface WithChildrenAndOptionalIcon { | ||
| children: ReactNode; | ||
| size: 'small' | 'medium' | 'large'; | ||
| size: 'small' | 'medium' | 'large' | 'huge'; |
There was a problem hiding this comment.
We have a size: 'small' | 'medium' | 'large' | 'extra-large'; size set for WithIconOnly, and have a huge size added in this PR for WithChildrenAndOptionalIcon.
These should be harmonized (I would go with extra-large over huge, as it simplifies naming for larger sizes).
| column-gap: 6px; | ||
| min-height: var(--button-pill-size-large); | ||
| padding-inline: 18px; | ||
| border-radius: 9999px; |
There was a problem hiding this comment.
Now that we will have a dedicated pill size for huge (soon to be extra-large), do we need this difference between extra-large and the other sizes?
Related to that, could we actually change all the buttons that are intended to be pills to have a border-radius of 9999px? (and move that to a token to make it less of a "magic number" - although 9999px is standard for pills).
So to sum up, we would:
- change all pill border radiuses to 9999px (with a token)
- separately, give huge (to be renamed extra-large) its own height token, and that height token should represent the actual min-height of the extra large button (which is larger than large).
| @mixin type-label-huge($has-text-box-trim: true) { | ||
| font-size: 20px; | ||
| font-weight: 500; | ||
| line-height: 22px; |
There was a problem hiding this comment.
Line height elsewhere in this file is unitless, whereas here this is in pixels. Is there a reason for this divergence?
An additional divergence is that this doesn't contain a breakpoint in size for mobile.
Overview
Improves the style, behaviour and visual aspects of the agent thinking card, the focus flow, the intro and follow up cards. Adds new elements accordingly.
Related Issues
Fixes # (issue number)
Changes Made
New: Button huge size
New: IconStatusIndicator
New: pulsing-card effect (shared mixin)
Autofocus behavior
Prompt panel: disabled state while query runs
Button size bumps on existing call sites
new_thinking_card.mov
Testing Done
Describe the steps you took to test these changes (please also list commands ran if possible).
Checklist
Note: Only Maintainers can approve and merge PRs. Expected initial review time: 3 business days.