💫 style: Align the Phase Summary Rail and Simplify its Fold (#15619)
Some checks are pending
Backend Unit Tests / Build packages (push) Waiting to run
Backend Unit Tests / Codegraph select (push) Waiting to run
Backend Unit Tests / TypeScript type checks (push) Blocked by required conditions
Backend Unit Tests / Circular dependency checks (push) Waiting to run
Backend Unit Tests / Tests: api (shard 1/3) (push) Blocked by required conditions
Backend Unit Tests / Tests: api (shard 2/3) (push) Blocked by required conditions
Backend Unit Tests / Tests: api (shard 3/3) (push) Blocked by required conditions
Backend Unit Tests / Tests: data-provider (push) Blocked by required conditions
Backend Unit Tests / Tests: data-schemas (push) Blocked by required conditions
Backend Unit Tests / Tests: @librechat/api (shard 1/4) (push) Blocked by required conditions
Backend Unit Tests / Tests: @librechat/api (shard 2/4) (push) Blocked by required conditions
Backend Unit Tests / Tests: @librechat/api (shard 3/4) (push) Blocked by required conditions
Backend Unit Tests / Tests: @librechat/api (shard 4/4) (push) Blocked by required conditions
Codegraph E2E Votes / vote (full suite) (push) Waiting to run
Docker Dev Branch Images Build / publish (push) Waiting to run
Frontend Unit Tests / Codegraph select (push) Waiting to run
Frontend Unit Tests / Build packages (push) Waiting to run
Frontend Unit Tests / TypeScript type checks (client) (push) Blocked by required conditions
Frontend Unit Tests / Tests: @librechat/client (push) Blocked by required conditions
Frontend Unit Tests / Tests: Ubuntu (shard 1/2) (push) Blocked by required conditions
Frontend Unit Tests / Tests: Ubuntu (shard 2/2) (push) Blocked by required conditions
Frontend Unit Tests / Vite build verification (push) Blocked by required conditions
Sync Locize Translations & Create Translation PR / Sync Translation Keys with Locize (push) Waiting to run
Sync Locize Translations & Create Translation PR / Create Translation PR on Version Published (push) Blocked by required conditions

* 💫 style: Align the Phase Summary Rail and Simplify its Fold

A phase summary was the only row in a transcript with no icon rail, so its
text sat at 13px while the tool rows it stood for sat at 24px and the rows it
swallowed moved out to 37px behind the card's `px-3` — three left edges in one
block, with every folded row stepping 13px sideways as the box materialized.

- The header takes the same 16px glyph slot every tool row has (`Check`, or
  `TriangleAlert` when the phase failed), which lands its text on the one rail.
- The card chrome is gone: border, background, radius, body padding and
  divider. The summary is a row among rows, so nothing moves horizontally when
  it forms, and the entrance is the two grid rows trading places rather than
  four properties resolving at once.
- The label is a ticker. A synthesized card re-titles itself every time it
  absorbs another finished block, and swapping that text instantly is what made
  the absorbed row look like it simply vanished; the retired summary now rises
  out of the clipped row while the new one comes up from below.
- The label uses `tool-status-text`, so a summary scales with the reader's
  font-size setting like the rows around it instead of sitting at a fixed 14px.

`ToolCallGroup`'s category glyph was a fourth rail at 28px (`h-5 w-5`); it now
matches `ToolIcon` at `size-4`.

`ease-[cubic-bezier(0.16,1,0.3,1)]` emitted nothing: `tailwindcss-animate`
registers its own `ease` utility for `animation-timing-function` alongside
Tailwind's `transition-timing-function` one, an arbitrary value matches both,
and Tailwind resolves that ambiguity by dropping the class. The curve had never
reached the chrome — the two-easings problem the comment warns about, caused by
its own fix. It is written as an arbitrary property now, and it was the only
such usage in the repo.

* 🩹 fix: Settle the Phase Label in One Commit and Drop a Duplicate Media Query

Review follow-ups on the phase ticker.

- `useSmoothStreaming` already resolves to `smoothStreaming && !reducedMotion`;
  subscribing to the same media query again installed one `matchMedia` listener
  per phase card without changing the answer.
- The label copied `text` into state from a passive effect, so a swap with no
  animation could paint the previous summary for a frame while the button's
  `aria-label` already carried the new one. It is adjusted during render now,
  which settles both paths in the same commit.
- The incoming line kept its `animate-in` class only while the retired line
  existed, so clearing that line on its `animationend` could strip the class
  from a slide still in flight. The class now rides its own flag.
- The test's label factory no longer needs `as unknown as`: every field on the
  ACTIVITY_LABEL part but `type` is optional, and the double cast was only
  there because a computed key widens to `string`.

* ♿ fix: State the Phase Header's Inset Focus Ring Locally

The header sits inside a permanent `overflow-hidden` wrapper — the 0fr/1fr
grid needs it — so its focus ring has to be inset or it is drawn outside the
border box and clipped away. That holds today only because the shared ghost
variant supplies `ring-inset`; a change in `packages/client` could remove the
indicator from here with no signal. It is declared on the element now, with a
test that asserts both the clip and the inset ring.
This commit is contained in:
Danny Avila 2026-09-04 17:44:23 -04:00 • committed by GitHub
parent 5e3cdd0b2c
commit d9dfdeb643
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 231 additions and 59 deletions

View file

@ -1,7 +1,7 @@
import { useId, useCallback, useEffect, useMemo, useRef, useState } from 'react';
import { Button } from '@librechat/client';
import { ChevronDown } from 'lucide-react';
import { ContentTypes } from 'librechat-data-provider';
import { Check, ChevronDown, TriangleAlert } from 'lucide-react';
import type { TAttachment, TMessageContentParts } from 'librechat-data-provider';
import type { CSSProperties, ReactNode } from 'react';
import {
@ -16,10 +16,17 @@ import { AttachmentGroup, EmptyText } from './Parts';
import Container from './Container';
import { cn } from '~/utils';
/** Matches `EXPAND_TRANSITION` so the header, the panel, and the card chrome
* all resolve on the same curve — three properties animating on two different
* easings is what makes a fold read as two separate movements. */
const FOLD_EASING = 'duration-300 ease-[cubic-bezier(0.16,1,0.3,1)] motion-reduce:transition-none';
/** Matches `EXPAND_TRANSITION` so the panel and the label ticker resolve on
* the same curve — two properties animating on two different easings is what
* makes a fold read as two separate movements.
*
* Written as an arbitrary PROPERTY, not `ease-[…]`: `tailwindcss-animate`
* registers its own `ease` utility for `animation-timing-function` alongside
* Tailwind's `transition-timing-function` one, so an arbitrary value matches
* both, and Tailwind resolves that ambiguity by emitting NOTHING. The class
* this replaces had been inert since the plugin landed — the curve it names
* never reached the fold. */
const FOLD_EASING = 'duration-300 [animation-timing-function:cubic-bezier(0.16,1,0.3,1)]';
type ActivityPhasePart = Extract<TMessageContentParts, { type: ContentTypes.ACTIVITY_LABEL }> & {
activity_label_type?: 'phase';
@ -49,6 +56,100 @@ function schedulePostPaint(callback: () => void): () => void {
};
}
/** The header's icon slot. Its only job is geometric: every other row in the
* transcript opens with a 16px glyph and an 8px gap, so a summary rendered
* without one sits 24px to the left of the rows it replaces — the fold then
* moves its own text sideways at the moment the reader is trying to follow
* it. */
function PhaseGlyph({ failed }: { failed: boolean }) {
const Icon = failed ? TriangleAlert : Check;
return (
<span
className={cn(
'flex size-4 shrink-0 items-center justify-center',
failed ? 'text-text-warning' : 'text-text-secondary',
)}
aria-hidden="true"
>
<Icon size={14} />
</span>
);
}
/**
* The phase header is a ticker, not a title. A client-synthesized card
* re-titles itself every time it absorbs another finished block, and swapping
* that text instantly is what makes the absorbed row look like it simply
* vanished from under the reader. The retired line rises out of the clipped
* row while the new summary comes up from below, so the work visibly moves
* into the line that now stands for it.
*/
function PhaseLabel({
text,
animate,
failed,
}: {
text: string;
animate: boolean;
failed: boolean;
}) {
const [lines, setLines] = useState<{ current: string; retired: string | null; entered: boolean }>(
{ current: text, retired: null, entered: false },
);
/** Adjusted during render rather than in an effect. A passive effect runs
* after paint, so a swap with no animation would leave the previous summary
* on screen for a frame while the button's `aria-label` already carried the
* new one. React re-renders this component immediately instead. */
if (lines.current !== text) {
const swaps = animate && lines.current.length > 0;
setLines({ current: text, retired: swaps ? lines.current : null, entered: swaps });
}
/** Clears only the retired line. `entered` outlives it on purpose: dropping
* the incoming line's animation class the moment its partner's
* `animationend` fires would snap a still-running slide back to its resting
* position. The class is inert once the animation has finished, and the
* element is keyed by its text, so it cannot replay. */
const clearRetired = useCallback(() => {
setLines((previous) => (previous.retired == null ? previous : { ...previous, retired: null }));
}, []);
return (
<span
className="tool-status-text relative block min-w-0 flex-1 overflow-hidden text-left"
role="status"
title={text}
>
{lines.retired != null && (
<span
key={`retired-${lines.retired}`}
className={cn(
'absolute inset-x-0 top-0 block truncate',
'animate-out fade-out-0 slide-out-to-top-5 fill-mode-forwards',
FOLD_EASING,
failed && 'text-text-warning',
)}
onAnimationEnd={clearRetired}
aria-hidden="true"
>
{lines.retired}
</span>
)}
<span
key={`current-${lines.current}`}
className={cn(
'block truncate',
lines.entered && `animate-in fade-in-0 slide-in-from-bottom-5 ${FOLD_EASING}`,
failed && 'text-text-warning',
)}
>
{lines.current}
</span>
</span>
);
}
export default function ActivityPhaseGroup({
labelPart,
children,
@ -74,18 +175,24 @@ export default function ActivityPhaseGroup({
}) {
const label = getActivityLabelText(labelPart);
const hasFailure = labelPart.status === 'failed' || labelPart.status === 'partial';
/** Already `smoothStreaming && !reducedMotion` — it owns the media query, so
* a second subscription here would install one `matchMedia` listener per
* phase card without changing the answer. */
const smoothStreaming = useSmoothStreaming();
/** Capture the marker's arrival state. The parent renderer records the new
* marker after this commit; a later sibling update must not cancel the
* already-scheduled fold before its first animation frame. */
const [shouldAnimateEntrance] = useState(smoothStreaming && animateEntrance && label.length > 0);
/** A filled phase marker lands on top of activity the reader is already
* looking at. The card therefore mounts in the shape of what was there
* BEFORE it — header at zero height, panel open, chrome transparent — and
* folds into the summary on the next painted frame. Growing the header
* while the panel collapses keeps the block's height strictly decreasing,
* so the content compresses upward instead of being shoved down by a
* header that appeared underneath it and then yanked back up. */
* looking at. The header therefore mounts at zero height with the panel
* open — the shape of what was there BEFORE it — and trades one for the
* other on the next painted frame. Growing the header while the panel
* collapses keeps the block's height strictly decreasing, so the content
* compresses upward instead of being shoved down by a header that appeared
* underneath it and then yanked back up. Those two grid rows are the ONLY
* properties in flight: the card chrome that used to fade in alongside
* them (border, background, padding, divider) is gone, and with it the
* sideways step its inset used to impose on every folded row. */
const foldsIn = shouldAnimateEntrance && hasContent;
const [isExpanded, setIsExpanded] = useState(foldsIn);
const [isSettled, setIsSettled] = useState(!foldsIn);
@ -182,13 +289,15 @@ export default function ActivityPhaseGroup({
const group = !hasContent ? (
<div
className={cn(
'my-2 flex min-h-10 w-full items-center rounded-lg border border-border-light bg-surface-secondary/40 px-3 py-2 text-text-secondary',
'mb-2 mt-1 flex min-h-7 w-full items-center gap-2 py-1 text-text-secondary',
shouldAnimateEntrance && `animate-in fade-in-0 motion-reduce:animate-none ${FOLD_EASING}`,
)}
data-testid="activity-phase-card"
>
<PhaseGlyph failed={hasFailure} />
<span
className={cn(
'min-w-0 flex-1 truncate text-left text-sm font-medium',
'tool-status-text min-w-0 flex-1 truncate text-left font-medium',
hasFailure && 'text-text-warning',
)}
role="status"
@ -198,35 +307,30 @@ export default function ActivityPhaseGroup({
</span>
</div>
) : (
<div
className={cn(
'my-2 w-full rounded-lg border transition-colors',
FOLD_EASING,
isSettled ? 'border-border-light bg-surface-secondary/40' : 'border-transparent',
)}
ref={rootRef}
>
/** No chrome. A phase summary is another row in the same list as the tool
* groups it stands for, so it carries the same geometry: 16px glyph, 8px
* gap, no inset. Boxing it was what put its text on a third left edge and
* forced every folded row 13px sideways as the box materialized. */
<div className="mb-2 mt-1 w-full" ref={rootRef} data-testid="activity-phase-card">
<div style={headerStyle}>
<div className="overflow-hidden">
<Button
variant="ghost"
type="button"
className="flex h-auto min-h-10 w-full items-center justify-start gap-2 rounded-lg bg-transparent px-3 py-2 text-left text-text-secondary hover:bg-surface-hover hover:text-text-primary focus-visible:ring-2 focus-visible:ring-inset focus-visible:ring-ring-primary focus-visible:ring-offset-0"
/** `ring-inset` is not decoration: the clip above is permanent (the
* grid rows need it), so an outset ring would be drawn entirely
* outside the button's border box and clipped away, leaving
* keyboard users with no focus indicator. The ghost variant
* supplies it today; stating it here keeps the requirement with
* the element that depends on it. */
className="inline-flex h-auto min-h-7 w-full items-center justify-start gap-2 rounded-none bg-transparent p-0 py-1 text-left font-medium text-text-secondary hover:bg-transparent hover:text-text-primary focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-inset focus-visible:ring-border-heavy focus-visible:ring-offset-0"
onClick={handleToggle}
aria-expanded={isExpanded}
aria-controls={panelId}
aria-label={label}
title={label}
>
<span
className={cn(
'min-w-0 flex-1 truncate text-left text-sm font-medium',
hasFailure && 'text-text-warning',
)}
role="status"
>
{label}
</span>
<PhaseGlyph failed={hasFailure} />
<PhaseLabel text={label} failed={hasFailure} animate={smoothStreaming} />
<ChevronDown
className={cn(
'size-4 shrink-0 transition-transform duration-200 ease-out motion-reduce:transition-none',
@ -246,19 +350,7 @@ export default function ActivityPhaseGroup({
>
{shouldRenderBody && (
<div className="overflow-hidden" ref={expandRef}>
{/** Padding and the divider ride the same curve as the fold: the
* children occupy the exact position they held before the marker
* arrived and settle into the card as it materializes, instead of
* stepping sideways by the card's inset on the first frame. */}
<div
className={cn(
'border-t transition-[border-color,padding]',
FOLD_EASING,
isSettled ? 'border-border-light px-3 py-2' : 'border-transparent px-0 py-0',
)}
>
{children}
</div>
{children}
</div>
)}
</div>

View file

@ -356,7 +356,7 @@ export default function ToolCallGroup({
* rather than "tools". */
<div
className={cn(
'flex h-5 w-5 shrink-0 items-center justify-center text-text-secondary',
'flex size-4 shrink-0 items-center justify-center text-text-secondary',
!allCompleted && isSubmitting && 'animate-pulse text-text-primary',
)}
aria-hidden="true"

View file

@ -24,14 +24,20 @@ jest.mock('~/hooks', () => {
});
const LABEL = 'Compared both release paths';
const NEXT_LABEL = 'Confirmed the rollback path is clean';
const labelPart = {
const makeLabelPart = (
text: string,
): Extract<TMessageContentParts, { type: ContentTypes.ACTIVITY_LABEL }> => ({
type: ContentTypes.ACTIVITY_LABEL,
[ContentTypes.ACTIVITY_LABEL]: LABEL,
activity_label: text,
activity_label_type: 'phase',
activity_start_index: 0,
pending: false,
} as unknown as Extract<TMessageContentParts, { type: ContentTypes.ACTIVITY_LABEL }>;
});
const labelPart = makeLabelPart(LABEL);
const nextLabelPart = makeLabelPart(NEXT_LABEL);
describe('ActivityPhaseGroup', () => {
let frames: Array<FrameRequestCallback | undefined>;
@ -93,7 +99,7 @@ describe('ActivityPhaseGroup', () => {
expect(container.querySelector('.result-thinking')).toBeInTheDocument();
});
test('renders the label flush left with no leading glyph', () => {
test('opens the summary on the same glyph rail as the rows it replaces', () => {
render(
<ActivityPhaseGroup labelPart={labelPart} hasContent>
<div data-testid="phase-content" />
@ -101,9 +107,26 @@ describe('ActivityPhaseGroup', () => {
);
const trigger = screen.getByRole('button', { name: LABEL });
expect(trigger).toHaveClass('justify-start', 'text-left');
expect(screen.getByText(LABEL)).toHaveClass('flex-1', 'text-left');
expect(trigger.querySelectorAll('svg')).toHaveLength(1);
expect(trigger).toHaveClass('justify-start', 'gap-2', 'p-0');
/** Leading glyph plus the chevron: a summary with no glyph slot sits 24px
* left of every tool row in the same list. */
expect(trigger.querySelectorAll('svg')).toHaveLength(2);
expect(trigger.firstElementChild).toHaveClass('size-4', 'shrink-0');
expect(screen.getByText(LABEL).parentElement).toHaveClass('flex-1', 'text-left');
});
test('keeps the focus ring inside the clipped header', () => {
render(
<ActivityPhaseGroup labelPart={labelPart} hasContent>
<div data-testid="phase-content" />
</ActivityPhaseGroup>,
);
const trigger = screen.getByRole('button', { name: LABEL });
/** The header's wrapper clips permanently — the 0fr/1fr grid needs it — so
* an outset ring would be drawn outside the border box and clipped away. */
expect(trigger.parentElement).toHaveClass('overflow-hidden');
expect(trigger).toHaveClass('focus-visible:ring-inset', 'focus-visible:ring-2');
});
test('mounts in the pre-marker shape, then folds the activity into the summary', () => {
@ -113,15 +136,13 @@ describe('ActivityPhaseGroup', () => {
</ActivityPhaseGroup>,
);
const card = container.querySelector('.my-2') as HTMLElement;
const card = screen.getByTestId('activity-phase-card');
const trigger = screen.getByRole('button', { name: LABEL });
const header = trigger.parentElement?.parentElement as HTMLElement;
const panel = screen.getByTestId('activity-phase-panel');
/** Frame zero must be indistinguishable from the layout the marker
* replaced: no chrome, no header height, activity still open. */
expect(card).toHaveClass('border-transparent');
expect(card).not.toHaveClass('bg-surface-secondary/40');
* replaced: no header height, activity still open. */
expect(header).toHaveStyle({ gridTemplateRows: '0fr', opacity: '0' });
expect(trigger).toHaveAttribute('aria-expanded', 'true');
expect(panel).toHaveAttribute('aria-hidden', 'false');
@ -129,10 +150,64 @@ describe('ActivityPhaseGroup', () => {
flushFrames();
expect(header).toHaveStyle({ gridTemplateRows: '1fr', opacity: '1' });
expect(card).toHaveClass('border-border-light', 'bg-surface-secondary/40');
expect(trigger).toHaveAttribute('aria-expanded', 'false');
expect(panel).toHaveAttribute('aria-hidden', 'true');
expect(mockScheduleLayoutReconcile).toHaveBeenCalledTimes(1);
/** The two grid rows are the whole entrance. Chrome that materializes on
* the same curve is what made the fold read as four movements, and its
* inset is what stepped every folded row sideways. */
expect(card.className).not.toMatch(/border|bg-surface|rounded|px-/);
expect(container.querySelector('[style*="padding"]')).toBeNull();
});
test('tickers the header when the phase absorbs another finished block', () => {
const { rerender } = render(
<ActivityPhaseGroup labelPart={labelPart} hasContent>
<div data-testid="phase-content" />
</ActivityPhaseGroup>,
);
expect(screen.queryByText(NEXT_LABEL)).not.toBeInTheDocument();
rerender(
<ActivityPhaseGroup labelPart={nextLabelPart} hasContent>
<div data-testid="phase-content" />
</ActivityPhaseGroup>,
);
/** The retired summary stays on screen, clipped, and rises out while the
* new one comes up from below — the absorbed block is the thing that
* moved, so it has to be visible while it moves. */
const retired = screen.getByText(LABEL);
expect(retired).toHaveClass('animate-out', 'slide-out-to-top-5');
expect(retired).toHaveAttribute('aria-hidden', 'true');
expect(screen.getByText(NEXT_LABEL)).toHaveClass('animate-in', 'slide-in-from-bottom-5');
/** Retiring the outgoing line must not strip the incoming one's animation
* class: both run for the same 300ms, so removing it on the partner's
* `animationend` would snap a slide that is still in flight. */
fireEvent.animationEnd(retired);
expect(screen.queryByText(LABEL)).not.toBeInTheDocument();
expect(screen.getByText(NEXT_LABEL)).toHaveClass('animate-in', 'slide-in-from-bottom-5');
});
test('swaps the header outright when smooth streaming is off', () => {
mockUseSmoothStreaming.mockReturnValue(false);
const { rerender } = render(
<ActivityPhaseGroup labelPart={labelPart} hasContent>
<div data-testid="phase-content" />
</ActivityPhaseGroup>,
);
rerender(
<ActivityPhaseGroup labelPart={nextLabelPart} hasContent>
<div data-testid="phase-content" />
</ActivityPhaseGroup>,
);
expect(screen.queryByText(LABEL)).not.toBeInTheDocument();
expect(screen.getByText(NEXT_LABEL)).not.toHaveClass('animate-in');
});
test('keeps historical phases closed without replaying the entrance', () => {
@ -163,7 +238,9 @@ describe('ActivityPhaseGroup', () => {
const trigger = screen.getByRole('button', { name: LABEL });
expect(trigger).toHaveAttribute('aria-expanded', 'false');
expect(container.querySelector('.my-2')).toHaveClass('border-border-light');
expect(container.querySelector('[style*="grid-template-rows"]')).toBe(
screen.getByTestId('activity-phase-panel'),
);
expect(pendingFrames()).toBe(0);
});

View file

@ -51,7 +51,10 @@ jest.mock('../ProgressText', () => ({
),
}));
/** Spread the real module: an icon set listed export-by-export goes silently
* undefined the moment a component imports one more glyph. */
jest.mock('lucide-react', () => ({
...jest.requireActual('lucide-react'),
ChevronDown: () => <span>{'chevron'}</span>,
TriangleAlert: () => <span>{'alert'}</span>,
Users: () => <span>{'users'}</span>,