refactor(session): rewrite session view per W1 visual lock for slice 11b.1#589
Closed
Astro-Han wants to merge 60 commits into
Closed
refactor(session): rewrite session view per W1 visual lock for slice 11b.1#589Astro-Han wants to merge 60 commits into
Astro-Han wants to merge 60 commits into
Conversation
Adds the pure grouping reducer that splits an assistant message's parts into ordered render units (prose / reasoning / trow-block) for the session-view rewrite in slice 11b.1. Algorithm follows DESIGN.md L466 / issue #440 design doc §3.1: prose and reasoning act as flush boundaries between consecutive tool runs; hidden / unknown / empty parts are skipped silently and do not split a logical operation. Tests cover the eight scenarios listed in §5.1 row #1 plus ordering preservation across multiple flushes. Refs #440.
Adds the pure derivation that the upcoming session-turn-trow-block
component will use to compute its summary row: total count, running
flag, failed count, and family icon resolved from the first tool's
type. The reducer mirrors getToolInfo()'s icon switch as a pure
function (no i18n context) so the leading icon can be picked outside
of a Solid root and unit-tested.
i18n key selection (`session.trow.summary.{running|withFailed|completed}`)
also lives here so the component is a thin wrapper. The component itself
is a stub at this commit; the visual implementation lands in slice 11b.1
Phase 2b together with chev / expand / shimmer wiring.
Refs #440 design doc §3.1 / §3.6 / DESIGN.md L471.
Astro-Han
added
P2
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds AttachmentChip UI/CSS, message-part types/registry/markdown/tool-info, deterministic grouping and trow-block UI, many tool renderers, session-turn leaf components and bridges, timeline staging/scroll/turn-change/websearch helpers, theme/token/animation tweaks, and tests/E2E. ChangesSession turn v2 leaf components and utilities
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements logic to group assistant message parts into renderable units, specifically aggregating consecutive tool calls into summary blocks while maintaining prose and reasoning as distinct segments. It includes the groupParts utility, a reducer for tool block metadata, and corresponding unit tests. Feedback was provided to simplify the groupParts function by removing unnecessary type casts, allowing TypeScript's control flow analysis to handle type narrowing.
Comment on lines
+79
to
+94
| if (part.type === "tool") { | ||
| pendingTools.push(part as ToolPart) | ||
| continue | ||
| } | ||
| if (part.type === "text") { | ||
| flushTools() | ||
| const t = part as TextPart | ||
| groups.push({ kind: "prose", partID: t.id, text: t.text }) | ||
| continue | ||
| } | ||
| if (part.type === "reasoning") { | ||
| flushTools() | ||
| const r = part as ReasoningPart | ||
| groups.push({ kind: "reasoning", partID: r.id, text: r.text }) | ||
| continue | ||
| } |
There was a problem hiding this comment.
The explicit type casts to ToolPart, TextPart, and ReasoningPart are unnecessary. TypeScript's control flow analysis automatically narrows the type of part within the if (part.type === ...) blocks. Removing these casts makes the code cleaner and allows the type system to catch potential errors, improving maintainability.
if (part.type === "tool") {
pendingTools.push(part)
continue
}
if (part.type === "text") {
flushTools()
groups.push({ kind: "prose", partID: part.id, text: part.text })
continue
}
if (part.type === "reasoning") {
flushTools()
groups.push({ kind: "reasoning", partID: part.id, text: part.text })
continue
}W1 visual preview (docs/design/preview/message-flow.html, locked 2026-05-12) widened the shimmer duration to 1800ms to match the Bill Chung "slow and steady" 1.5-2s industry midrange and to keep long phrases continuously visibly shimmering instead of finishing in one screen-width. Two-line change: `--text-shimmer-duration` in text-shimmer.css and `--animate-pw-shimmer` keyframe duration in animations.css. The component API, gradient size, and direction stay unchanged — only the wall-clock speed shifts. Confirmed no remaining 1200ms shimmer references via `grep -rE "1200ms" packages/ui/src`. Refs #440 design doc §4.1 / DESIGN.md Motion.
Adds a `fork` entry to the chrome icon registry so the slice 11b.1 agent-toolbar Fork button can resolve `<Icon name="fork" />` without falling back to a missing-icon state. Placeholder path data is the existing `branch` glyph — the W1 preview lock-stamp explicitly documents this as the agreed temporary stand-in until imagegen Sheet 15's dedicated Fork candidate is picked by Astro-Han. The inline comment names the swap-out site so the eventual one-line SVG replacement is unambiguous. Refs #440 design doc §4.2.
Adds the shared attachment chip primitive that the user-message bubble
(read-only mode) and the composer dock (editable mode) will both render,
fulfilling DESIGN.md L444 "one attachment spec across the product".
Geometry mirrors the W1 visual lock in
docs/design/preview/message-flow.html:
- File chip: 64-tall row with a 48×48 surface-sunken square holding a
32-px file-type icon (left), and a 2-line right slot (filename body
with ellipsis + uppercase extension caption with letter-spacing 0.5).
- Image chip: 64×64 square with `object-fit: cover`, hairline border,
shared radius-md with the file chip so they read as one family.
- Close button: 24-circle, inverse fill, floating at -6/-6 top-right,
gated by `removable={true}`. Bubble passes `removable={false}` so
archived attachments never show the × (W1 L1088).
The composer dock migration to this primitive is intentionally deferred
to a sibling slice — slice 11b.1 is bubble-side only — but the API
ships ready for both consumers.
Tests follow the source-grep convention used by message-part-stale:
they pin the structural invariants (data-kind branches, Show-gated
remove button, W1 CSS geometry) without introducing a new
testing-library dependency. Behavioural coverage lands in
session-turn-user-bubble.test.tsx (Phase 2c).
Refs #440 design doc §3.7 / DESIGN.md L444.
Adds the presentational user bubble for the slice 11b.1 conversation
flow rewrite. The bubble consumes the shared AttachmentChip primitive
in `removable={false}` mode for the attachment row, joins multiple
non-synthetic text parts with `\n\n`, renders plain text via JSX
interpolation (XSS-safe), and exposes the hover-only bottom toolbar
with a meta segment (model + 24h `HH:mm` time, no `·` separator) and
an actions segment ([Copy] [Reset]).
Visibility is CSS-only: the toolbar is always mounted, with
`opacity: 0; pointer-events: none` defaults that flip on `:hover`,
`:focus-within`, or a `data-hover` attribute. The `:focus-within`
trigger is the a11y patch on top of the W1 preview lock so
keyboard-only users can surface the toolbar by tabbing.
The bubble is context-free: it accepts pre-resolved `modelName`,
i18n `labels`, and an `actions` object for clipboard / reset entry
points. The slice 11b.1 SessionTurn shell wires real SDK paths
(`sdk.client.session.revert` + `navigator.clipboard.writeText`) into
these props in the upcoming Phase 2a integration commit.
Refs #440 design doc §3.3 / §3.6 / §6.13-6.18 / DESIGN.md L453-L462.
Adds the muted-caption system event line for slice 11b.1. Only the `interrupted` kind is wired tonight (assistantMessage.error?.name === "MessageAbortedError"), but the SystemEventKind enum predeclares the full W1 taxonomy (`connection-lost` / `connection-restored`) so the sync-layer follow-up in 11b.2 can light them up without changing this component's public API. The component is context-free: it accepts a pre-resolved `label` string from the caller, so the i18n routing decision lives in the SessionTurn agent round, not here. Visually: one `<div>` with `--type-caption` / `--fg-weak`, left-aligned, no chrome — DESIGN.md L475's muted-line spec. Refs #440 design doc §3.5 / DESIGN.md L475.
Adds the floating jump-to-bottom button that appears above the composer dock whenever the user has scrolled up from the bottom of the timeline. Visibility is a single signal: `!pinned`. The "has new content since unlock" gate the W1 first draft toyed with is intentionally absent — one fewer state signal, one fewer test branch, one fewer mental model to maintain (slice 11b.1 §3.4 three-question simplification). The button is presentational only — it accepts `visible` / `onClick` / `label` props and nothing else. The scroll-state plumbing (`useTimelineScroll`) lives in packages/app/src/pages/session/ and computes `jumpButtonVisible` + `onJumpClick` for the SessionTurn shell to thread through. Geometry follows DESIGN.md L477's icon- button round floating variant (30×30, surface-raised, hairline, shadow-floating, hover 4% overlay, chevron-down 16-px glyph). Refs #440 design doc §3.4 / DESIGN.md L477.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/ui/src/components/icon.tsx (1)
18-23: ⚡ Quick winAvoid duplicating the full SVG payload for placeholder
forkThis works, but duplicating the huge path string makes future updates error-prone. Reuse a shared glyph constant for both
branchandforkuntil the final fork asset lands.♻️ Suggested refactor
+const branchGlyph = `<g transform="translate(1.000 1.233) scale(0.093264)"><path d="..." fill="currentColor"/></g>` export const icons = { - "branch": `<g transform="translate(1.000 1.233) scale(0.093264)"><path d="..." fill="currentColor"/></g>`, + "branch": branchGlyph, // Slice 11b.1 placeholder... - "fork": `<g transform="translate(1.000 1.233) scale(0.093264)"><path d="..." fill="currentColor"/></g>`, + "fork": branchGlyph, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/icon.tsx` around lines 18 - 23, Replace the duplicated SVG string for the "fork" icon by referencing the same shared glyph used for "branch": create a single constant (e.g., BRANCH_GLYPH or SHARED_BRANCH_SVG) holding the large <g>…</g> payload and assign that constant to both the "branch" and "fork" keys instead of inlining the full path for "fork"; update the icons map so "fork" points to the shared constant to avoid future divergence.packages/ui/src/components/attachment-chip.test.ts (1)
43-43: ⚡ Quick winLoosen the image CSS regex to avoid order-coupled test failures.
This assertion currently depends on two selectors being adjacent in a specific order; harmless CSS reordering can break the test.
Suggested patch
- expect(css).toMatch(/\[data-kind="image"\][^}]*\}\s*\n\s*\[data-component="attachment-chip"\]\[data-kind="image"\][^{}]*\[data-slot="attachment-chip-image"\][^{}]*\{[^}]*object-fit:\s*cover/) + expect(css).toMatch(/\[data-component="attachment-chip"\]\[data-kind="image"\][^{}]*\[data-slot="attachment-chip-image"\][^{}]*\{[^}]*object-fit:\s*cover/)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/attachment-chip.test.ts` at line 43, The test's regex in attachment-chip.test.ts is order-coupled and requires the [data-kind="image"] selector and the [data-component="attachment-chip"][data-kind="image"][data-slot="attachment-chip-image"] block to be adjacent; loosen it by changing the assertion on the css string so it no longer requires specific adjacency—either assert the presence of each selector/block separately (e.g., two toMatch calls checking [data-kind="image"] and the block containing object-fit: cover) or modify the regex to allow arbitrary intervening characters between the selectors (use a non-order-dependent pattern) so harmless CSS reordering won't break the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/attachment-chip.css`:
- Around line 100-115: The remove button styled by the selector
[data-component="attachment-chip"] [data-slot="attachment-chip-remove"] is
interactive but uses cursor: default; change its cursor to pointer so it appears
clickable. Locate the rule for the remove control (selector with
data-component="attachment-chip" and data-slot="attachment-chip-remove") and
replace cursor: default with cursor: pointer (or add cursor: pointer) while
leaving the other layout/visual properties intact.
In `@packages/ui/src/components/attachment-chip.tsx`:
- Around line 66-67: The image branch renders even when previewUrl is absent;
update the conditional guards on the Show components that check props.kind ===
"image" to also require props.previewUrl (e.g., change to props.kind === "image"
&& props.previewUrl) so the image slot only renders when a previewUrl exists;
apply the same guard to the second Show block that handles the image preview
(the block around the logic currently at lines ~107-111) and ensure any <img>
usage uses props.previewUrl safely.
- Around line 88-97: When rendering the remove button in the attachment chip,
ensure it always has an accessible name by providing a default label when
props.removeLabel is missing: change the aria-label usage on the button in the
removable branch to use props.removeLabel ?? 'Remove attachment' (and do the
same for the second occurrence around the Icon branch at the other location).
Also mark the Icon component as decorative (e.g., ensure Icon is aria-hidden or
otherwise not exposing an extra name) so the button's aria-label is the actual
accessible name, and keep onRemove as the click handler.
---
Nitpick comments:
In `@packages/ui/src/components/attachment-chip.test.ts`:
- Line 43: The test's regex in attachment-chip.test.ts is order-coupled and
requires the [data-kind="image"] selector and the
[data-component="attachment-chip"][data-kind="image"][data-slot="attachment-chip-image"]
block to be adjacent; loosen it by changing the assertion on the css string so
it no longer requires specific adjacency—either assert the presence of each
selector/block separately (e.g., two toMatch calls checking [data-kind="image"]
and the block containing object-fit: cover) or modify the regex to allow
arbitrary intervening characters between the selectors (use a
non-order-dependent pattern) so harmless CSS reordering won't break the test.
In `@packages/ui/src/components/icon.tsx`:
- Around line 18-23: Replace the duplicated SVG string for the "fork" icon by
referencing the same shared glyph used for "branch": create a single constant
(e.g., BRANCH_GLYPH or SHARED_BRANCH_SVG) holding the large <g>…</g> payload and
assign that constant to both the "branch" and "fork" keys instead of inlining
the full path for "fork"; update the icons map so "fork" points to the shared
constant to avoid future divergence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a19f39b9-06ad-476c-9cfa-4f59e65df6f7
📒 Files selected for processing (10)
packages/ui/src/components/attachment-chip.csspackages/ui/src/components/attachment-chip.test.tspackages/ui/src/components/attachment-chip.tsxpackages/ui/src/components/icon.tsxpackages/ui/src/components/message-part-group.test.tspackages/ui/src/components/message-part-group.tspackages/ui/src/components/session-turn-trow-block.reducer.test.tspackages/ui/src/components/session-turn-trow-block.tsxpackages/ui/src/components/text-shimmer.csspackages/ui/src/styles/animations.css
Comment on lines
+100
to
+115
| [data-component="attachment-chip"] [data-slot="attachment-chip-remove"] { | ||
| position: absolute; | ||
| top: -6px; | ||
| right: -6px; | ||
| width: 24px; | ||
| height: 24px; | ||
| border-radius: 9999px; | ||
| background: var(--fg-strong); | ||
| color: var(--bg-base); | ||
| border: 0; | ||
| padding: 0; | ||
| cursor: default; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } |
Contributor
There was a problem hiding this comment.
Use a pointer cursor for the removable close button.
The remove control is clickable, but cursor: default makes it appear non-interactive.
Suggested patch
[data-component="attachment-chip"] [data-slot="attachment-chip-remove"] {
position: absolute;
top: -6px;
right: -6px;
@@
- cursor: default;
+ cursor: pointer;
display: inline-flex;
align-items: center;
justify-content: center;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [data-component="attachment-chip"] [data-slot="attachment-chip-remove"] { | |
| position: absolute; | |
| top: -6px; | |
| right: -6px; | |
| width: 24px; | |
| height: 24px; | |
| border-radius: 9999px; | |
| background: var(--fg-strong); | |
| color: var(--bg-base); | |
| border: 0; | |
| padding: 0; | |
| cursor: default; | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| } | |
| [data-component="attachment-chip"] [data-slot="attachment-chip-remove"] { | |
| position: absolute; | |
| top: -6px; | |
| right: -6px; | |
| width: 24px; | |
| height: 24px; | |
| border-radius: 9999px; | |
| background: var(--fg-strong); | |
| color: var(--bg-base); | |
| border: 0; | |
| padding: 0; | |
| cursor: pointer; | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/attachment-chip.css` around lines 100 - 115, The
remove button styled by the selector [data-component="attachment-chip"]
[data-slot="attachment-chip-remove"] is interactive but uses cursor: default;
change its cursor to pointer so it appears clickable. Locate the rule for the
remove control (selector with data-component="attachment-chip" and
data-slot="attachment-chip-remove") and replace cursor: default with cursor:
pointer (or add cursor: pointer) while leaving the other layout/visual
properties intact.
Comment on lines
+66
to
+67
| <Show | ||
| when={props.kind === "image"} |
Contributor
There was a problem hiding this comment.
Guard the image branch when previewUrl is missing.
If kind="image" arrives without previewUrl, the component renders a broken/empty image slot.
Suggested patch
<Show
- when={props.kind === "image"}
+ when={props.kind === "image" && !!props.previewUrl}
fallback={Also applies to: 107-111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/attachment-chip.tsx` around lines 66 - 67, The
image branch renders even when previewUrl is absent; update the conditional
guards on the Show components that check props.kind === "image" to also require
props.previewUrl (e.g., change to props.kind === "image" && props.previewUrl) so
the image slot only renders when a previewUrl exists; apply the same guard to
the second Show block that handles the image preview (the block around the logic
currently at lines ~107-111) and ensure any <img> usage uses props.previewUrl
safely.
Comment on lines
+88
to
+97
| <Show when={props.removable}> | ||
| <button | ||
| type="button" | ||
| data-slot="attachment-chip-remove" | ||
| aria-label={props.removeLabel} | ||
| onClick={(event) => props.onRemove?.(event)} | ||
| > | ||
| <Icon name="close" /> | ||
| </button> | ||
| </Show> |
Contributor
There was a problem hiding this comment.
Ensure the remove button always has an accessible name.
When removable is true and removeLabel is omitted, the button can be rendered without a usable accessible label.
Suggested patch
export function AttachmentChip(rawProps: AttachmentChipProps & Omit<ComponentProps<"div">, keyof AttachmentChipProps>) {
@@
const resolvedExt = () => props.ext ?? defaultExtFromName(props.name)
+ const resolvedRemoveLabel = () => props.removeLabel ?? "Remove attachment"
@@
<button
type="button"
data-slot="attachment-chip-remove"
- aria-label={props.removeLabel}
+ aria-label={resolvedRemoveLabel()}
onClick={(event) => props.onRemove?.(event)}
>
@@
<button
type="button"
data-slot="attachment-chip-remove"
- aria-label={props.removeLabel}
+ aria-label={resolvedRemoveLabel()}
onClick={(event) => props.onRemove?.(event)}
>Also applies to: 112-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/attachment-chip.tsx` around lines 88 - 97, When
rendering the remove button in the attachment chip, ensure it always has an
accessible name by providing a default label when props.removeLabel is missing:
change the aria-label usage on the button in the removable branch to use
props.removeLabel ?? 'Remove attachment' (and do the same for the second
occurrence around the Icon branch at the other location). Also mark the Icon
component as decorative (e.g., ensure Icon is aria-hidden or otherwise not
exposing an extra name) so the button's aria-label is the actual accessible
name, and keep onRemove as the click handler.
Adds the agent round — the second-largest single file in slice 11b.1 (~340 lines including pure helpers and the running-tick effect). The component renders one round of agent activity on the new flat surface (no turn-frame wrapper, DESIGN.md L463-L469): - Working-time header: always-visible single caption that ticks every second while the round is the latest live one, freezes at the final elapsed value once `time.completed` lands. Same DOM node across states; only the wrap's `data-running` attribute flips. The `document.hidden` gate skips ticking when the tab is hidden, and a `visibilitychange` listener refreshes `now` immediately on focus return so the counter doesn't show a stale value. - Grouped body: `groupParts()` (Phase 1a) produces prose / reasoning / trow-block units; the caller injects a `renderProse` slot so the shell can plug in the 11a MessageMarkdown renderer without dragging it into this component. Reasoning defaults to italic + `--fg-weak` (DESIGN.md L482 spec calls for `--fg-secondary` — token not yet in theme.css; a one-line swap once it ships). - System event: when the latest assistant in the round carries a `MessageAbortedError`, a single `SystemEvent kind="interrupted"` line renders after the body. The full W1 taxonomy is reserved by `SystemEventKind` for sync-layer follow-ups in 11b.2. - Agent toolbar: hover-only meta (completion time, 24h `HH:mm`) + actions ([Copy] [Fork]). While running, the toolbar is fully suppressed (visibility hidden + pointer-events none + aria-hidden) so Tab cannot reach inert buttons mid-stream. Fork has the §6.14 in-flight `disabled` guard inside a `try/finally` so an unmount during navigation does not leak the busy flag. Pure helpers `selectFirstAssistant` / `selectLatestAssistant` / `isInterrupted` / `computeElapsedSec` are exported for unit testing without Solid runtime. 22 tests pin the contract (4 selectors / 4 elapsed math cases / 4 interrupt cases + 6 source-grep posture checks for race / cleanup / suppression). Refs #440 design doc §3.1 / §3.2 / §3.5 / §3.6 / §6.12 / §6.14 / §6.17 / DESIGN.md L463-L482.
Fills the Phase 1b TrowBlock stub with the actual render shell: a native <details> element (W1 lock) whose <summary> shows the leading family icon (16px), the i18n summary text, and a 12-px chev that rotates 90° on open. The summary text switches between three i18n keys based on the pure reduceTrowBlock derivation (running / completed / completedWithFailed), matching the §E14 spec. The chev / disclosure is suppressed for tool groups whose tools have no body worth expanding (matches W1 preview's "无中间输出的工具 summary 不渲染 chev" rule). Per-tool rich rendering — file accordions, raw output, copy button, diff body — is intentionally delegated to a caller-provided `renderTool` slot. The SessionTurn shell wires this to the existing GenericTool / BasicTool / Part renderers from message-part.tsx in the Phase 2a integration commit, so slice 11b.1 does not reimplement 11a's tool body. A minimal "name + status" fallback row ships for callers without a slot (storybook / dev preview). Pipes a `labels.trow` sub-object through SessionTurnAgentRound so the round can pass i18n-resolved labels into each TrowBlock without the trow component reading any context — same pattern as the rest of slice 11b.1's components. Verified: 11 reducer tests (unchanged) + ui suite stable at 472 pass / 3 pre-existing baseline fail. Typecheck packages/ui GREEN. Refs #440 design doc §3.1 / §3.6 / §E14 / DESIGN.md L412-L417 / L471.
`--fg-secondary` (reasoning) and `--icon-chev` (trow chev) are specced in DESIGN.md L482 / W1 lock but are not yet registered in theme.css. This slice uses `--fg-weak` / `--icon-base` as the closest substitutes and adds `TODO(Phase 5 sibling slice)` comments next to each fallback so the next slice can grep and flip in one line each. Risk Notes in PR #589 carry the same deferral.
Wires the five new leaf components (user-bubble + agent-round + trow-block + system-event) together against the existing useData / useI18n data layer. The shell pre-resolves i18n labels and parts-by- message maps so the leaves stay context-free and unit-testable. This component is opt-in by import — it is exported from @opencode-ai/ui/session-turn-v2 via the package's `./*` glob but is NOT mounted as the default user-path. The existing SessionTurn keeps ownership of turnChanges undo/redo, diffs accordion, SessionRetry, the error card, and the compaction MessageDivider; those land in a sibling slice after Phase 3a extracts the per-role displays cleanly. i18n adds `ui.sessionTurnV2.workingTime` + `ui.sessionTurnV2.trow.*` keys to en.ts / zh.ts. Source-grep tests (9 pass) lock the hybrid contract: opt-in marker, leaf wiring, renderProse slot, context-free i18n, no deferred-feature re-implementation, action adapter shape, isLatestRound derivation, and the flat `--bg-base` 24px CSS rhythm. Slice 11b.1 PR #589 Status section documents the hybrid mount.
The previous render shell used `<Show fallback={defaultRenderTools(props.parts)}>`,
which built the fallback `<For>` from the parts array captured at
component-creation time. New tools that landed mid-stream while
`renderTool` was undefined (the v2 shell's current path) would not
appear in the body — only the initial parts rendered.
Flip the JSX so `<For each={props.parts}>` is the outer reactive
primitive and the per-item branch picks between `renderTool` and the
inline fallback renderer. New tool parts now flow into the body the
moment they land.
Self-review caught this on the manual pass; no test regression
because the existing reducer tests don't drive the JSX render path.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/ui/src/components/session-turn-v2.tsx (1)
57-57: ⚡ Quick win
showReasoningSummariesis currently a no-op.This prop is exposed on
SessionTurnV2Propsbut never influences rendering, so callers cannot actually disable reasoning summaries. Please either wire it through toSessionTurnAgentRoundbehavior or remove it from this API for now.Also applies to: 236-245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/session-turn-v2.tsx` at line 57, The prop showReasoningSummaries on SessionTurnV2Props is declared but unused; either remove it from the public API or pass it into the child component that renders summaries. Fix by threading showReasoningSummaries from the SessionTurnV2 props into the SessionTurnAgentRound invocation (and into any deeper components that actually render reasoning summaries) and then use that boolean to conditionally skip rendering the reasoning summary UI (wherever the summary JSX/logic is implemented). Alternatively, if you choose removal, delete showReasoningSummaries from SessionTurnV2Props and any references in SessionTurnV2 and update tests/consumers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/session-turn-trow-block.tsx`:
- Around line 98-101: The trowSummaryI18nKey helper returns outdated keys under
"session.trow.summary.*"; update the return values in function
trowSummaryI18nKey (which takes a TrowBlockSummary) to match the new dictionary
namespace "ui.sessionTurnV2.trow.*" (e.g., map running, withFailed, completed to
"ui.sessionTurnV2.trow.running", "ui.sessionTurnV2.trow.withFailed", and
"ui.sessionTurnV2.trow.completed" respectively) so lookups resolve to existing
i18n entries.
In `@packages/ui/src/components/session-turn-user-bubble.tsx`:
- Around line 79-85: The guard that checks the timestamp (const created =
props.message.time?.created) uses typeof created === "number" which permits NaN
and infinities; replace that check with Number.isFinite(created) before passing
created into Intl.DateTimeFormat so only finite numeric timestamps are
formatted; apply the same change to the second occurrence that formats the time
(the other Intl.DateTimeFormat call) to harden both guards.
---
Nitpick comments:
In `@packages/ui/src/components/session-turn-v2.tsx`:
- Line 57: The prop showReasoningSummaries on SessionTurnV2Props is declared but
unused; either remove it from the public API or pass it into the child component
that renders summaries. Fix by threading showReasoningSummaries from the
SessionTurnV2 props into the SessionTurnAgentRound invocation (and into any
deeper components that actually render reasoning summaries) and then use that
boolean to conditionally skip rendering the reasoning summary UI (wherever the
summary JSX/logic is implemented). Alternatively, if you choose removal, delete
showReasoningSummaries from SessionTurnV2Props and any references in
SessionTurnV2 and update tests/consumers accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ce99588d-f6e2-438b-a164-5a296887d466
📒 Files selected for processing (19)
packages/ui/src/components/session-turn-agent-round.csspackages/ui/src/components/session-turn-agent-round.test.tspackages/ui/src/components/session-turn-agent-round.tsxpackages/ui/src/components/session-turn-event.csspackages/ui/src/components/session-turn-event.test.tspackages/ui/src/components/session-turn-event.tsxpackages/ui/src/components/session-turn-jump.csspackages/ui/src/components/session-turn-jump.test.tspackages/ui/src/components/session-turn-jump.tsxpackages/ui/src/components/session-turn-trow-block.csspackages/ui/src/components/session-turn-trow-block.tsxpackages/ui/src/components/session-turn-user-bubble.csspackages/ui/src/components/session-turn-user-bubble.test.tspackages/ui/src/components/session-turn-user-bubble.tsxpackages/ui/src/components/session-turn-v2.csspackages/ui/src/components/session-turn-v2.test.tspackages/ui/src/components/session-turn-v2.tsxpackages/ui/src/i18n/en.tspackages/ui/src/i18n/zh.ts
✅ Files skipped from review due to trivial changes (7)
- packages/ui/src/components/session-turn-event.css
- packages/ui/src/components/session-turn-user-bubble.test.ts
- packages/ui/src/components/session-turn-event.tsx
- packages/ui/src/components/session-turn-jump.test.ts
- packages/ui/src/components/session-turn-jump.css
- packages/ui/src/components/session-turn-event.test.ts
- packages/ui/src/components/session-turn-trow-block.css
Comment on lines
+98
to
+101
| export function trowSummaryI18nKey(summary: TrowBlockSummary): string { | ||
| if (summary.running) return "session.trow.summary.running" | ||
| if (summary.failedCount > 0) return "session.trow.summary.withFailed" | ||
| return "session.trow.summary.completed" |
Contributor
There was a problem hiding this comment.
Fix trowSummaryI18nKey to return existing i18n keys.
This helper returns session.trow.summary.*, but the dictionaries added in this PR use ui.sessionTurnV2.trow.*. That mismatch will resolve to missing text when the helper is consumed.
Suggested patch
export function trowSummaryI18nKey(summary: TrowBlockSummary): string {
- if (summary.running) return "session.trow.summary.running"
- if (summary.failedCount > 0) return "session.trow.summary.withFailed"
- return "session.trow.summary.completed"
+ if (summary.running) return "ui.sessionTurnV2.trow.running"
+ if (summary.failedCount > 0) return "ui.sessionTurnV2.trow.withFailed"
+ return "ui.sessionTurnV2.trow.completed"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function trowSummaryI18nKey(summary: TrowBlockSummary): string { | |
| if (summary.running) return "session.trow.summary.running" | |
| if (summary.failedCount > 0) return "session.trow.summary.withFailed" | |
| return "session.trow.summary.completed" | |
| export function trowSummaryI18nKey(summary: TrowBlockSummary): string { | |
| if (summary.running) return "ui.sessionTurnV2.trow.running" | |
| if (summary.failedCount > 0) return "ui.sessionTurnV2.trow.withFailed" | |
| return "ui.sessionTurnV2.trow.completed" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/session-turn-trow-block.tsx` around lines 98 -
101, The trowSummaryI18nKey helper returns outdated keys under
"session.trow.summary.*"; update the return values in function
trowSummaryI18nKey (which takes a TrowBlockSummary) to match the new dictionary
namespace "ui.sessionTurnV2.trow.*" (e.g., map running, withFailed, completed to
"ui.sessionTurnV2.trow.running", "ui.sessionTurnV2.trow.withFailed", and
"ui.sessionTurnV2.trow.completed" respectively) so lookups resolve to existing
i18n entries.
Comment on lines
+79
to
+85
| const created = props.message.time?.created | ||
| if (typeof created !== "number") return undefined | ||
| return new Intl.DateTimeFormat(props.locale, { | ||
| hour: "2-digit", | ||
| minute: "2-digit", | ||
| hour12: false, | ||
| }).format(created) |
Contributor
There was a problem hiding this comment.
Harden timestamp guards against NaN / non-finite values.
On Line 80 and Line 90, typeof created === "number" allows NaN and infinities. That can produce invalid time formatting or throw depending on runtime path. Use Number.isFinite(created) before formatting.
Suggested fix
const timeStamp = createMemo(() => {
const created = props.message.time?.created
- if (typeof created !== "number") return undefined
+ if (typeof created !== "number" || !Number.isFinite(created)) return undefined
return new Intl.DateTimeFormat(props.locale, {
hour: "2-digit",
minute: "2-digit",
hour12: false,
}).format(created)
})
const timeTitle = createMemo(() => {
const created = props.message.time?.created
- if (typeof created !== "number") return undefined
+ if (typeof created !== "number" || !Number.isFinite(created)) return undefined
return new Date(created).toString()
})Also applies to: 89-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/session-turn-user-bubble.tsx` around lines 79 -
85, The guard that checks the timestamp (const created =
props.message.time?.created) uses typeof created === "number" which permits NaN
and infinities; replace that check with Number.isFinite(created) before passing
created into Intl.DateTimeFormat so only finite numeric timestamps are
formatted; apply the same change to the second occurrence that formats the time
(the other Intl.DateTimeFormat call) to harden both guards.
The slice 11b.1 design doc (#440 comment-4432066973 §1) explicitly forbids co-existence of old and new `session-turn.tsx`: > The existing `session-turn.tsx` (748 lines, pre-rewrite) is > overwritten in place. There is no co-existence of old/new > `session-turn.tsx`. The hybrid opt-in shell violated that contract by trying to ship new leaf components behind an opt-in mount point while keeping the legacy `SessionTurn` as the default user-path. Per the in-thread clarification this slice ships the full rewrite: the new shell overwrites the existing `session-turn.tsx`, no parallel surface. This commit removes: - `session-turn-v2.tsx` + `.css` + `.test.ts` - `ui.sessionTurnV2.*` i18n keys (en.ts / zh.ts) The new leaf components themselves remain — they will be wired into the rewritten `session-turn.tsx` in subsequent commits on this branch, alongside the User/Assistant display extractions and the message-timeline.tsx split.
The W1 visual lock and slice 11b.1 design doc §3.1 / DESIGN.md L482
reference two CSS tokens that were not yet present in theme.css:
--fg-secondary — reasoning summary ink, distinct from --fg-weak
so future rebalances can shift secondary alone
--icon-chev — trow-block chevron color, distinct from --icon-base
Both tokens are registered in theme.css (light + dark + @media first-
paint mirror) and mirrored in pawwork.json overrides so theme-parity.
test.ts stays GREEN (it enforces a 1:1 key+value sync between the two
sources).
Token values today are intentionally identical to the closest existing
neighbor (--fg-weak / --icon-base respectively); they ship as carve-
outs so the next visual rebalance can move them without touching
every callsite that referenced --fg-weak or --icon-base for an
unrelated purpose.
Switch the two leaf components that previously used the fallbacks
(`session-turn-agent-round.css` reasoning, `session-turn-trow-block.
css` chev) over to the real tokens and remove the `TODO(Phase 5)`
comments. Phase 5 token deferral noted in PR #589 Risk Notes is now
closed.
Slice 11b.1 step 1/14 of the message-part.tsx split mandated by AGENTS.md (>500-line files must be split) and the slice design doc (GPT-X-authored 14-file split plan). Extracts the type-only declarations — `MessageProps`, `SessionAction`, `UserActions`, `MessagePartProps`, `PartComponent`, `ToolProps`, `ToolComponent` — into a new `message-part-types.ts` module. The new file is a pure type module (no runtime exports, no JSX, no Solid primitive use), so renderer/tool files can pull in the shared shapes without dragging the heavier message-part.tsx transitive import graph. `message-part.tsx` keeps its existing public API by re-exporting the types verbatim, so `session-turn.tsx` / `message-part.stories.tsx` / unit tests need no update in this step. The next slice will migrate consumers to import from `./message-part-types` directly and retire the re-exports. Net: message-part.tsx loses ~35 declaration lines; types live in a 49-line file. Typecheck GREEN, 478 pass / 3 baseline fail unchanged.
Slice 11b.1 step 2/14 of the message-part.tsx split. Moves the part
and tool registries — the foundation that every renderer plugs into
through `PART_MAPPING[...] = ...` and `registerTool({...})` — out of
the 2400-line monolith into a 75-line registry module.
New file `message-part-registry.tsx` exports:
- `PART_MAPPING` the mutable part-type → renderer map
- `registerPartComponent` setter helper
- `Part` dispatcher component (looks up the
renderer registered for a given part type)
- `registerTool` tool renderer registration
- `getTool` renderer lookup by tool name
- `ToolRegistry` legacy alias object kept for the
BasicTool / GenericTool callsites
`message-part.tsx` keeps the same public symbols through a re-export
block so existing callers (session-turn.tsx, stories, tests) don't
have to move yet — the migration to direct imports happens in the
next slice.
Net: message-part.tsx 2459 → 2399 lines. Typecheck GREEN, 478 pass /
3 baseline fail unchanged.
Slice 11b.1 step 3/14 of the message-part.tsx split. Moves the 24-ms paced text reveal hook (`createPacedValue`), the path helpers (`isAbsoluteLikePath`, `joinWorkspacePath`, `relativizeProjectPath`, `getDirectory`), and the `<MessageMarkdown>` / `<PacedMarkdown>` wrappers into a new `message-part-markdown.tsx` module. These helpers are consumed by the text / reasoning renderers (paced streaming), the assistant message display (markdown body), and a handful of tool renderers that show paths. Lifting them out lets each consumer import the markdown stack directly without dragging the full message-part dispatcher's transitive graph. Behavior is unchanged: same 24-ms cadence, same 8-char whitespace lookahead, same cleanup semantics on signal change / unmount. The `useData()` access inside `<MessageMarkdown>` and `getDirectory()` is preserved at the same call sites — these helpers must be invoked inside Solid render scopes, never at module load. message-part.tsx 2399 → 2263 lines (-136). Typecheck GREEN, 478 pass / 3 baseline fail unchanged.
Slice 11b.1 step 4/14 of the message-part.tsx split. Moves the i18n- aware tool-info factory (`getToolInfo`) and the per-tool state narrowing helpers (`toolStateMetadata`, `toolStateError`) out of the monolith into `message-part-tool-info.ts`, alongside the agent tone palette + `taskAgent` resolver that the agent/task tool renderer uses. These helpers are all "pure (per render scope)" — `getToolInfo` internally calls `useI18n()`, so it must remain inside a Solid render scope; it has not become module-level callable. The public `buildToolInfo` / `ToolInfo` re-export now flows from `./tool-info` directly (its original source) rather than transiting through message-part.tsx — equivalent behavior, smaller import graph inside the new tool-info module. message-part.tsx 2263 → 2103 lines (-160). Typecheck GREEN, 478 pass / 3 baseline fail unchanged.
Slice 11b.1 step 5/14 of the message-part.tsx split. Moves the legacy assistant-pipeline helpers — list / latestDefined / same / sameRef / sameGroup / sameGroups / PartRef / PartGroup / groupParts (legacy) / index / renderable / partDefaultOpen / HIDDEN_TOOLS / CONTEXT_GROUP_TOOLS / isContextGroupTool — into a dedicated module `message-part-render-groups.ts`. These are deliberately kept separate from the v2 grouping helper at `./message-part-group.ts` (both files own a `groupParts` symbol; the shapes differ — legacy emits context-tool groups, v2 emits prose / reasoning / trow-block groups). Inside message-part.tsx the legacy import is aliased to `legacyGroupParts` so the call sites stay explicit about which grouping they consume. The `message-part-stale.test.ts` source-grep that pinned `latestDefined` to `message-part.tsx` is updated to assert it lives in `message-part-render-groups.ts` (the new home) while still checking that the assistant dispatcher in `message-part.tsx` reads it. No behavioral change. message-part.tsx 2103 → 1986 lines (-117). Typecheck GREEN, 478 pass / 3 baseline fail unchanged.
…extToolGroup
Slice 11b.1 step 6/14 of the message-part.tsx split. Moves the entire
legacy assistant rendering surface into a dedicated module:
`assistant-message-display.tsx` (470 lines)
- `AssistantMessageDisplay` single-message dispatcher
- `AssistantParts` multi-message round renderer (used by
the legacy SessionTurn shell)
- `ContextToolGroup` collapsible context-tool row
- `ExaOutput` links-only fallback for webfetch/search
- private helpers: `urls`, `contextToolDetail`, `contextToolTrigger`,
`contextToolSummary`
Back-compat preserved: `message-part.tsx` re-exports
`AssistantMessageDisplay` and `AssistantParts` so existing consumers
(session-turn.tsx imports `AssistantParts` directly) keep resolving;
`ExaOutput` is imported into message-part.tsx and used by the websearch
tool renderer that still lives here.
This is the larger of the two named extractions in design doc §1
("extracted from message-part.tsx L901"). The User display extraction
is the next step.
message-part.tsx 1986 → 1607 lines (-379). Typecheck GREEN, 478 pass /
3 baseline fail unchanged.
…body AstroHan's second W1 retest flagged that 4 tool renderers (read / find / web search / bash) still render their legacy sans / base / large header chrome inside the trow body, breaking the W1 caption surface. Rewriting every renderer is overkill and would re-introduce the same drift the next time a tool is added. Wrap each rich `renderTool` result in a single `trow-result-body` slot inside `session-turn-leaf-bridges`, and scope the DESIGN.md L417 frame (transparent + 1px --border-weaker + radius-sm + 8/12 padding + mono-small / fg-weak) to that slot in `session-turn-trow-block.css`. Hard-coded sans/base/large typography on `basic-tool-tool-title` / `-subtitle` / `-arg` and `message-part-title` / `-text` / `-filename` is flattened to the W1 caption family only as a descendant of `[data-slot="trow-result-body"]` — the reset boundary is the trow result body wrapper, not the renderer registry, so new tools inherit the caption typography automatically.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/session-turn-trow-block.css`:
- Around line 21-29: The summary slot selector
[data-component="session-turn-trow-block"] [data-slot="trow-summary"] lacks
keyboard focus styles; add :focus and :focus-visible rules for that interactive
<summary> element to provide a visible focus indicator (e.g., outline or
box-shadow using your design token like --focus-ring or --accent-focus, and
preserve border-radius and padding), ensure the styles only show on keyboard
focus via :focus-visible if available, and keep cursor and other layout
properties unchanged so keyboard users can clearly see which trow-summary is
focused.
- Line 25: Change the cursor style for the interactive summary element from a
non-interactive pointer to a clickable affordance: replace the rule that sets
"cursor: default" for the summary selector (the CSS rule that currently contains
"cursor: default;") with "cursor: pointer;" so the <summary> element shows a
pointer on hover and signals it is interactive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a508779c-6b6a-44d1-a4f0-48bc64b216e7
📒 Files selected for processing (13)
packages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/session-timeline-scroll-controller.tspackages/app/src/pages/session/use-session-timeline-interaction.tspackages/ui/src/components/session-turn-agent-round-helpers.tspackages/ui/src/components/session-turn-agent-round.csspackages/ui/src/components/session-turn-agent-round.test.tspackages/ui/src/components/session-turn-agent-round.tsxpackages/ui/src/components/session-turn-leaf-bridges.tsxpackages/ui/src/components/session-turn-trow-block.csspackages/ui/src/components/session-turn-user-bubble.csspackages/ui/src/components/session-turn-user-bubble.test.tspackages/ui/src/components/session-turn-user-bubble.tsxpackages/ui/src/hooks/create-auto-scroll.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/ui/src/components/session-turn-agent-round.test.ts
- packages/ui/src/components/session-turn-user-bubble.test.ts
- packages/ui/src/components/session-turn-agent-round.tsx
- packages/ui/src/components/session-turn-leaf-bridges.tsx
- packages/ui/src/components/session-turn-user-bubble.tsx
- packages/ui/src/components/session-turn-agent-round.css
Comment on lines
+21
to
+29
| [data-component="session-turn-trow-block"] [data-slot="trow-summary"] { | ||
| display: inline-flex; | ||
| align-items: center; | ||
| gap: 8px; | ||
| cursor: default; | ||
| list-style: none; | ||
| color: var(--fg-base); | ||
| font: var(--type-body); | ||
| } |
Contributor
There was a problem hiding this comment.
Add keyboard focus styles for accessibility.
The PR objectives explicitly call out "keyboard a11y" as a key feature, but no :focus or :focus-visible styles are defined for the interactive <summary> element. Keyboard users need visible focus indicators to navigate the interface effectively, and WCAG 2.1 Success Criterion 2.4.7 requires visible focus.
⌨️ Suggested focus styles
[data-component="session-turn-trow-block"] [data-slot="trow-summary"] {
display: inline-flex;
align-items: center;
gap: 8px;
cursor: default;
list-style: none;
color: var(--fg-base);
font: var(--type-body);
}
+
+[data-component="session-turn-trow-block"] [data-slot="trow-summary"]:focus-visible {
+ outline: 2px solid var(--focus-ring, `#0066cc`);
+ outline-offset: 2px;
+ border-radius: 4px;
+}Note: Adjust the focus ring color token to match your design system's focus indicator token.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/session-turn-trow-block.css` around lines 21 - 29,
The summary slot selector [data-component="session-turn-trow-block"]
[data-slot="trow-summary"] lacks keyboard focus styles; add :focus and
:focus-visible rules for that interactive <summary> element to provide a visible
focus indicator (e.g., outline or box-shadow using your design token like
--focus-ring or --accent-focus, and preserve border-radius and padding), ensure
the styles only show on keyboard focus via :focus-visible if available, and keep
cursor and other layout properties unchanged so keyboard users can clearly see
which trow-summary is focused.
| display: inline-flex; | ||
| align-items: center; | ||
| gap: 8px; | ||
| cursor: default; |
Contributor
There was a problem hiding this comment.
Change cursor to pointer for interactive summary element.
The <summary> element is inherently interactive (toggles the <details> expansion), but cursor: default makes it appear non-interactive to users. Standard practice is to use cursor: pointer for clickable elements to signal affordance.
🖱️ Proposed fix
- cursor: default;
+ cursor: pointer;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cursor: default; | |
| cursor: pointer; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/session-turn-trow-block.css` at line 25, Change
the cursor style for the interactive summary element from a non-interactive
pointer to a clickable affordance: replace the rule that sets "cursor: default"
for the summary selector (the CSS rule that currently contains "cursor:
default;") with "cursor: pointer;" so the <summary> element shows a pointer on
hover and signals it is interactive.
AstroHan's third W1 retest (msg=362e9b72) reported that the light mode user bubble also loses its shape — `--bg-cream` (#fdf6e3) and `--bg-base` (#fbf3df) sit ~3 RGB apart, an equally thin separation for the brighter end of the luminance curve as dark's ~5 RGB gap. The previous slice 11b.1 fix added a 1px inset ring in dark only on the assumption that light had enough luminance delta to read; the retest disproves that. Apply the hairline in both modes. Light uses `--border-weaker` (the dimmer of the two tokens) because the high-luminance side has more edge sensitivity and `--border-weak` would shout; dark stays on `--border-weak`. The base rule is the light hairline; the dark override flips it to `--border-weak`. Geometry is unchanged — `box-shadow: inset` does not affect layout.
AstroHan's third W1 retest (msg=362e9b72) raised two visual issues on the trow block; both land in `session-turn-trow-block.css`. **#2 — per-tool box wrapper.** Slice 11b.1 #5 B+ added a transparent + 1px --border-weaker frame around each tool's rich body (`data-slot="trow-result-body"`) as the L417-spec scope marker. AstroHan reported the chrome makes tools look like isolated cards and (when bash is expanded) stacks on top of bash-output's own border. The W1 intent is a flat stacked feed where tools 叠在一起. The wrapper stays — it is still the scope boundary for the inner chrome typography reset — but its visual chrome (border / radius / padding) is removed. **#4 — chev direction + inner tool chev size.** Two parts: (a) Trow summary chev direction. The third retest re-defined the affordance: 折叠朝右、展开朝下. The second-retest fix that pointed the chev up on `details[open]` is reverted — file-tree, Collapsible, and the W1 mocks all use down-when- open, so trow should match. Default `transform: rotate(-90deg)` (chevron-down → right when collapsed); `details[open]` flips to `rotate(0)` (natural down). (b) Inner Collapsible.Arrow chev size inside the trow result body. AstroHan flagged that the chev inside each tool trigger ("执行 命令 XXXX" row) renders as a "大 chevron" — Collapsible's default is 16px, but the W1 caption surface (DESIGN.md L412 lock) caps the chev at 12px. Scoped reset fires only inside `[data-slot="trow-result-body"]`, so the Collapsible default elsewhere (file-tree, settings, etc.) is unaffected.
AstroHan's third W1 retest (msg=362e9b72) flagged that the "思考中"
row at the agent bottom drifts far from the rest of the round when
reasoning / trow / prose is already present — it lands ~12px below
the assistant-content block as an isolated tail, duplicating the
liveness signal the TrowBlock shimmer / streaming prose / working-
time tick already provide. GPT-X reviewed (msg=22179280) and the
three-question read came out: less is enough, the placeholder is
only useful before any visible part exists; keep it as a pre-first-
visible-output affordance, drop it from the steady-state.
Tighten `showThinking` to `working && !error && status.type !==
"retry" && assistantVisible === 0`. The previous condition leaked in
the `showReasoningSummaries: false` branch where the gate fell back
to `return true` regardless of visibility — which is exactly the
state AstroHan tested in. The new gate applies the
`assistantVisible === 0` clamp uniformly; reasoning parts still
register as "not visible" when the summary toggle is off, so the
existing "show thinking while reasoning streams under the toggle"
behaviour is preserved.
Position alignment ("对齐 agent prose 起点") falls out of the
tightening — once the row only renders when no visible part exists,
the assistant-content block above it has an empty agent-body, so
the 12px gap no longer reads as a detached tail. The placeholder
sits at the column's natural left edge (same as where prose would
start), inheriting `session-turn-message-container`'s flex-start
alignment. No structural move needed.
The "Tool Display Spec" follow-up that would let the placeholder
merge into the agent working-time header is filed as issue #596;
that work is out of scope for slice 11b.1.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ui/src/components/session-turn-trow-block.css (1)
21-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStill missing interactive affordance and keyboard focus styling on the summary trigger.
Line 25 keeps
cursor: default, and the interactive summary has no visible keyboard focus state. This remains an accessibility/usability gap.♿ Minimal fix
[data-component="session-turn-trow-block"] [data-slot="trow-summary"] { display: inline-flex; align-items: center; gap: 8px; - cursor: default; + cursor: pointer; list-style: none; color: var(--fg-base); font: var(--type-body); } + +[data-component="session-turn-trow-block"] [data-slot="trow-summary"]:focus-visible { + outline: 2px solid currentColor; + outline-offset: 2px; + border-radius: 4px; +}#!/bin/bash # Verify whether summary trigger has focus-visible styles and interactive cursor affordance. rg -n '\[data-slot="trow-summary"\].*cursor:\s*default|cursor:\s*pointer' packages/ui/src/components/session-turn-trow-block.css -A3 -B3 rg -n '\[data-slot="trow-summary"\]:(focus|focus-visible)' packages/ui/src/components/session-turn-trow-block.css -A2 -B2 rg -n '<summary|data-slot="trow-summary"' packages/ui/src/components/session-turn-trow-block.tsx -A3 -B3🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/session-turn-trow-block.css` around lines 21 - 29, The summary trigger [data-slot="trow-summary"] is styled with cursor: default and has no keyboard focus visuals; update its CSS to use an interactive cursor (cursor: pointer) and add a focus/focus-visible rule (e.g., [data-slot="trow-summary"]:focus-visible) that applies a visible outline/box-shadow and preserves accessible contrast, ensuring the selector in session-turn-trow-block.css targets the summary trigger and its focus state so keyboard users can see when it’s focused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/ui/src/components/session-turn-trow-block.css`:
- Around line 21-29: The summary trigger [data-slot="trow-summary"] is styled
with cursor: default and has no keyboard focus visuals; update its CSS to use an
interactive cursor (cursor: pointer) and add a focus/focus-visible rule (e.g.,
[data-slot="trow-summary"]:focus-visible) that applies a visible
outline/box-shadow and preserves accessible contrast, ensuring the selector in
session-turn-trow-block.css targets the summary trigger and its focus state so
keyboard users can see when it’s focused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 773de814-400c-4774-8b5b-66e239736ae6
📒 Files selected for processing (3)
packages/ui/src/components/session-turn-trow-block.csspackages/ui/src/components/session-turn-user-bubble.csspackages/ui/src/components/session-turn.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/components/session-turn.tsx
… WebFetch parity AstroHan's fourth W1 retest (msg=ac13481a) flagged three trow visuals. **#1 — Missing expand/collapse animation.** The header comment on `session-turn-trow-block.css` claimed `session-turn.css` owned the `::details-content` rule, but it was never actually wired anywhere. Add the rule (mirror of preview `message-flow.html` L13-23): height + opacity transition gated by `prefers-reduced-motion: no-preference`, with `transition-behavior: allow-discrete` covering the discrete `content-visibility` step. The `:root { interpolate-size: allow-keywords }` declaration in utilities.css is what lets `height: 0` ↔ `height: auto` interpolate; the rule silently no-ops to a snap without it. Inner Collapsible.Content gets a matching scoped animation — Solid's `collapsible.css` ships its keyframes commented out (L92-101), so without the override the per-tool body would snap open. The scoped selector keeps the file-tree / settings Collapsibles on their default behaviour. **#2 — Tighter expanded body.** AstroHan reported the expanded trow body felt too loose vs preview. `.mf-tlist` in `docs/design/preview/message-flow.html` L199 locks gap at 6px between sub-tool rows; trow-body now matches (was 8px). **#3 — WebFetch layout parity.** WebFetch is the only tool that ships a right-aligned `[data-component="tool-action"]` arrow inside its trigger row (`message-part-tools-basic.tsx` L142). The other tools use the plain `trigger={{ title, subtitle }}` object form, so they already flow left without the action chrome. Suppress the action icon only inside the trow result body — standalone WebFetch outside the trow keeps the affordance.
…te thinking heading Two related AstroHan W1 retest 4 issues (msg=ac13481a) on the "思考中" surface and the reasoning-summaries setting. **#4 — Thinking heading overflows right edge.** When `showReasoningSummaries` is off the thinking row renders a `<TextReveal>` showing the latest reasoning heading. `TextReveal` defaults to `white-space: nowrap` on its entering/leaving spans, so a long heading bleeds past the column's right edge. The component already exposes a `truncate` prop that caps width and adds `text-overflow: ellipsis`; opt in. **#5a — Reasoning-summaries setting had no effect on COT body.** The settings toggle ("显示推理摘要 / Show reasoning summaries") flows through to `SessionTurn` but only gated `showThinking`'s assistant- visible counting and the secondary reasoning heading inside the thinking row. `AgentRound` never received the prop, so reasoning groups always rendered regardless of the toggle. Pass `showReasoningSummaries` to `AgentRound` and drop `reasoning` groups from the rendered output when it is explicitly `false`. The grouper itself stays structural (does not know about user preferences); the filter lives at the leaf consuming the groups, matching the existing seam with `partState()` in `session-turn-helpers.ts`.
W1 trow group wraps every tool body, default-collapsed. The legacy "Expand shell tool parts" / "Expand edit tool parts" settings mapped `defaultOpen` onto each per-tool Part, but in W1 that prop only changes state inside an already-hidden body — the toggles silently did nothing until the user expanded the outer trow first, so they read as broken. Removes the two SettingsRow controls + i18n keys (en/zh) and stops threading `shellToolDefaultOpen` / `editToolDefaultOpen` through the timeline → SessionTurn → AgentRound → TrowBlock chain, plus the LeafBridge `defaultOpen` fall-through (which also accidentally landed on every non-bash tool, not just edit / write / patch). The persistence keys `general.shellToolPartsExpanded` / `editToolPartsExpanded` are left in place on the settings context for migration compat with users who already have the value in localStorage; reintroducing a tool-expansion preference later should ship under a W1-shaped name.
…tent
W1 retest 4 reproduced the off-bottom round → next agent
`content_resize` → viewport snaps to bottom (or, for a round far from
bottom, to the user-turn top via the message-level anchor restore).
RCA narrows the trigger to the trow `<details>` toggle: it changes the
scroll geometry around the round but the scroll controller never
treated it as a user-layout intent, so the next content_resize fell
through `following_latest` and called `restore_latest`.
Minimal fix threads a `layout_interaction` intent through the same
channel scroll gestures already use:
- `<TrowBlock>` summary fires `onUserLayoutInteraction` on
`onPointerDown` (and Enter / Space keydown), *before* the native
`<details>` toggle.
- `<SessionTurnAgentRound>` / `<SessionTurn>` / `TimelineMessage` /
`<MessageTimeline>` forward the callback.
- `MessageTimeline` translates the callback into
`onTimelineScrollIntent({ type: "layout_interaction", source:
"trow_toggle" })`.
- `session-timeline-scroll-controller` accepts the new intent type,
flips `state.mode = "reading_history"`, drops the latest-protected
latch, and emits with `reason: "layout_interaction_reading"` and no
recovery so the next `content_resize` leaves the viewport alone.
- `use-session-timeline-interaction` pauses the autoScroll owner and
cancels the bottom-follow lock for `layout_interaction`, matching
the existing scroll-gesture branch.
Adds a regression e2e (`session-w1-scroll-layout.spec.ts`): stages a
long trow + off-bottom scroll, clicks the trow summary, triggers an
agent append, and asserts the viewport stayed inside a "reading"
range — neither snapped to user-turn top nor pinned flush against the
bottom edge.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/e2e/session/session-w1-scroll-layout.spec.ts`:
- Around line 31-35: The test title string in the test(...) call is marker-heavy
and not following the repo style; replace the current title "@regression P0 `#6` —
trow toggle + agent append keeps viewport in reading position" with a lowercase,
behavior-first descriptive name (e.g., "trow toggle and agent append preserve
viewport reading position") by editing the test(...) invocation in
session-w1-scroll-layout.spec.ts so the test string is concise, lowercase, and
focused on the behavior.
- Around line 80-82: The test uses a fixed wall-clock wait via
page.waitForTimeout(800) which makes it flaky; replace it by observing DOM state
(e.g., measure scrollHeight before appending and then wait until it increases)
using Playwright's expect.poll or expect with page.evaluate. Concretely: capture
previousHeight = await page.evaluate(() =>
document.querySelector('<your-scroll-container-selector>').scrollHeight)
immediately before the append/operation, remove the page.waitForTimeout(800)
call, and add an assertion like await expect.poll(async () => await
page.evaluate(() =>
document.querySelector('<your-scroll-container-selector>').scrollHeight)).toBeGreaterThan(previousHeight)
(or use expect with a timeout) to wait deterministically; replace
'<your-scroll-container-selector>' with the actual selector used in this spec
and remove the fixed timeout call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 68e97c82-763e-46a4-ac94-1d0bada20835
📒 Files selected for processing (19)
packages/app/e2e/session/session-renderer-diagnostics.spec.tspackages/app/e2e/session/session-w1-bubble.spec.tspackages/app/e2e/session/session-w1-error.spec.tspackages/app/e2e/session/session-w1-events.spec.tspackages/app/e2e/session/session-w1-scroll-layout.spec.tspackages/app/e2e/session/session-w1-trow.spec.tspackages/app/src/components/settings-general.tsxpackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/session/message-timeline-row.tsxpackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/session-timeline-scroll-controller.tspackages/app/src/pages/session/use-session-timeline-interaction.tspackages/ui/src/components/session-turn-agent-round.tsxpackages/ui/src/components/session-turn-leaf-bridges.tsxpackages/ui/src/components/session-turn-trow-block.csspackages/ui/src/components/session-turn-trow-block.tsxpackages/ui/src/components/session-turn.tsxpackages/ui/src/components/timeline-playground.stories.tsx
💤 Files with no reviewable changes (3)
- packages/ui/src/components/timeline-playground.stories.tsx
- packages/app/src/i18n/zh.ts
- packages/app/src/i18n/en.ts
✅ Files skipped from review due to trivial changes (1)
- packages/app/e2e/session/session-renderer-diagnostics.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/app/e2e/session/session-w1-error.spec.ts
- packages/app/e2e/session/session-w1-trow.spec.ts
- packages/app/e2e/session/session-w1-events.spec.ts
- packages/app/e2e/session/session-w1-bubble.spec.ts
- packages/app/src/pages/session/message-timeline.tsx
Comment on lines
+31
to
+35
| test("@regression P0 #6 — trow toggle + agent append keeps viewport in reading position", async ({ | ||
| page, | ||
| llm, | ||
| project, | ||
| }) => { |
Contributor
There was a problem hiding this comment.
Use a lowercase, descriptive test name.
At Line 31, the test title is marker-heavy (@regression P0 #6 ...) and not in the repo’s lowercase descriptive style. Rename to a behavior-first title.
Suggested rename
-test("@regression P0 `#6` — trow toggle + agent append keeps viewport in reading position", async ({
+test("trow toggle and agent append keep viewport in reading position", async ({As per coding guidelines: "Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("@regression P0 #6 — trow toggle + agent append keeps viewport in reading position", async ({ | |
| page, | |
| llm, | |
| project, | |
| }) => { | |
| test("trow toggle and agent append keep viewport in reading position", async ({ | |
| page, | |
| llm, | |
| project, | |
| }) => { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/app/e2e/session/session-w1-scroll-layout.spec.ts` around lines 31 -
35, The test title string in the test(...) call is marker-heavy and not
following the repo style; replace the current title "@regression P0 `#6` — trow
toggle + agent append keeps viewport in reading position" with a lowercase,
behavior-first descriptive name (e.g., "trow toggle and agent append preserve
viewport reading position") by editing the test(...) invocation in
session-w1-scroll-layout.spec.ts so the test string is concise, lowercase, and
focused on the behavior.
Comment on lines
+80
to
+82
| // Give the renderer a moment to apply the content_resize. | ||
| await page.waitForTimeout(800) | ||
|
|
Contributor
There was a problem hiding this comment.
Replace fixed timeout with observable-state waiting.
At Line 81, page.waitForTimeout(800) makes this regression flaky under variable render timing. Wait on measurable state (e.g., scroll height change after append).
Suggested stabilization
- // Give the renderer a moment to apply the content_resize.
- await page.waitForTimeout(800)
+ // Wait until appended content causes a measurable layout change.
+ await expect
+ .poll(async () => {
+ const m = await scroller.evaluate((el) => ({
+ scrollHeight: el.scrollHeight,
+ scrollTop: el.scrollTop,
+ }))
+ return m.scrollHeight
+ })
+ .toBeGreaterThan(before.scrollHeight)As per coding guidelines: "Never use wall-clock waits like page.waitForTimeout(...) to make a test pass" and "Wait on observable state with expect(...)/expect.poll(...)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Give the renderer a moment to apply the content_resize. | |
| await page.waitForTimeout(800) | |
| // Wait until appended content causes a measurable layout change. | |
| await expect | |
| .poll(async () => { | |
| const m = await scroller.evaluate((el) => ({ | |
| scrollHeight: el.scrollHeight, | |
| scrollTop: el.scrollTop, | |
| })) | |
| return m.scrollHeight | |
| }) | |
| .toBeGreaterThan(before.scrollHeight) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/app/e2e/session/session-w1-scroll-layout.spec.ts` around lines 80 -
82, The test uses a fixed wall-clock wait via page.waitForTimeout(800) which
makes it flaky; replace it by observing DOM state (e.g., measure scrollHeight
before appending and then wait until it increases) using Playwright's
expect.poll or expect with page.evaluate. Concretely: capture previousHeight =
await page.evaluate(() =>
document.querySelector('<your-scroll-container-selector>').scrollHeight)
immediately before the append/operation, remove the page.waitForTimeout(800)
call, and add an assertion like await expect.poll(async () => await
page.evaluate(() =>
document.querySelector('<your-scroll-container-selector>').scrollHeight)).toBeGreaterThan(previousHeight)
(or use expect with a timeout) to wait deterministically; replace
'<your-scroll-container-selector>' with the actual selector used in this spec
and remove the fixed timeout call.
Each per-tool row inside the trow result body is wrapped in Solid Collapsible's `<Collapsible.Trigger>`, which carries a hardcoded `height: 30px` (collapsible.css L19) + a 26x26 arrow wrapper. The 30px floor overrode the 18px natural mono-small line height the preview spec (message-flow.html L200-208) and the screenshot evidence (W1 retest 5 msg=34822d84 attachment) demanded; row stride was effectively 30 + 6 = 36px regardless of `gap: 6` on `trow-body`. Scoped reset inside `[data-slot="trow-result-body"]` flattens the trigger to `height: auto; min-height: 0` and shrinks the arrow wrapper to 12x12 (matching the chev icon scoped reset that already shipped). Collapsible elsewhere (file-tree, settings, every other trigger) keeps the 30px default because the resets only fire as descendants of the trow result body scope. Row stride drops from ~36px to ~24px.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/app/src/pages/session/use-session-timeline-interaction.ts (1)
258-264: 💤 Low valueDead
createEffectwith a no-op handler.This effect tracks
sessionID, the last visible user message id, andhistoryWindow.turnStart(), but its handler is() => undefinedanddefer: trueskips the initial tick. It produces no observable behavior and reads three reactive sources for nothing — it looks like a leftover from the previous bottom-follow-lock flow that this PR removed.♻️ Proposed cleanup
- createEffect( - on( - () => [input.sessionID(), input.visibleUserMessages().at(-1)?.id, historyWindow.turnStart()] as const, - () => undefined, - { defer: true }, - ), - )If this effect was meant to host follow-up logic in a later slice, consider adding a
TODOcomment referencing the slice so it doesn't read as dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/pages/session/use-session-timeline-interaction.ts` around lines 258 - 264, The createEffect using on(...) with dependencies input.sessionID(), input.visibleUserMessages().at(-1)?.id and historyWindow.turnStart() is a no-op (handler is () => undefined and defer: true) and should be removed or annotated; either delete the entire createEffect block (since it only reads reactive sources and has no behavior), or replace the handler with a brief TODO comment referencing the follow-up slice and why these specific signals are tracked (so future work knows this was intentional). Locate the createEffect/on call in use-session-timeline-interaction.ts and apply one of these two fixes to eliminate the dead reactive read.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/pages/session/session-timeline-scroll-anchors.ts`:
- Around line 116-126: In restoreLatest, the sentinel branch is effectively dead
because viewport.scrollTop is set with Math.max(sentinelTop, maxScrollTop) which
always yields maxScrollTop; change the assignment to use Math.min(sentinelTop,
maxScrollTop) so the scroll positions places the bottomSentinel flush with the
viewport bottom but never exceeds the maximum scrollable position (update the
line setting viewport.scrollTop inside restoreLatest that currently uses
Math.max to use Math.min instead).
In `@packages/app/src/pages/session/session-timeline-scroll-controller.ts`:
- Around line 519-533: The nearBottom branch of observe currently assigns
state.lastSafePosition to latest unconditionally, which can overwrite a
target_message anchor; update the nearBottom handling in observe so it does not
change state.lastSafePosition when the controller is currently targeting a
message whose lastSafePosition.kind === "target_message" (i.e., if state.mode
=== "targeting_message" && state.lastSafePosition?.kind === "target_message"
then skip updating lastSafePosition), but still perform the existing mode/
latestProtected updates and return the same result; this preserves the invariant
used by the target restoration logic and aligns with
updateObservedSafePosition's intent.
---
Nitpick comments:
In `@packages/app/src/pages/session/use-session-timeline-interaction.ts`:
- Around line 258-264: The createEffect using on(...) with dependencies
input.sessionID(), input.visibleUserMessages().at(-1)?.id and
historyWindow.turnStart() is a no-op (handler is () => undefined and defer:
true) and should be removed or annotated; either delete the entire createEffect
block (since it only reads reactive sources and has no behavior), or replace the
handler with a brief TODO comment referencing the follow-up slice and why these
specific signals are tracked (so future work knows this was intentional). Locate
the createEffect/on call in use-session-timeline-interaction.ts and apply one of
these two fixes to eliminate the dead reactive read.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dbb5c289-e9b4-4890-b032-b85301d69bfb
📒 Files selected for processing (17)
packages/app/src/pages/session/message-timeline-row.tsxpackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/session-timeline-scroll-anchors.tspackages/app/src/pages/session/session-timeline-scroll-controller.test.tspackages/app/src/pages/session/session-timeline-scroll-controller.tspackages/app/src/pages/session/use-session-hash-scroll.test.tspackages/app/src/pages/session/use-session-scroll-dock.test.tspackages/app/src/pages/session/use-session-scroll-dock.tspackages/app/src/pages/session/use-session-timeline-interaction.tspackages/ui/src/components/session-turn-agent-round.csspackages/ui/src/components/session-turn-agent-round.test.tspackages/ui/src/components/session-turn-agent-round.tsxpackages/ui/src/components/session-turn-leaf-bridges.tsxpackages/ui/src/components/session-turn-parent.test.tspackages/ui/src/components/session-turn-trow-block.csspackages/ui/src/components/session-turn-trow-block.tsxpackages/ui/src/components/session-turn.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/app/src/pages/session/message-timeline-row.tsx
- packages/ui/src/components/session-turn-trow-block.tsx
- packages/ui/src/components/session-turn-agent-round.css
- packages/ui/src/components/session-turn-agent-round.test.ts
- packages/ui/src/components/session-turn-trow-block.css
- packages/ui/src/components/session-turn.tsx
- packages/ui/src/components/session-turn-agent-round.tsx
- packages/app/src/pages/session/message-timeline.tsx
- packages/ui/src/components/session-turn-leaf-bridges.tsx
Comment on lines
116
to
126
| function restoreLatest(viewport: HTMLElement, bottomSentinel?: HTMLElement | null) { | ||
| const maxScrollTop = Math.max(0, viewport.scrollHeight - viewport.clientHeight) | ||
| if (bottomSentinel) { | ||
| const viewportRect = viewport.getBoundingClientRect() | ||
| const sentinelRect = bottomSentinel.getBoundingClientRect() | ||
| viewport.scrollTop = Math.max(0, viewport.scrollTop + sentinelRect.bottom - viewportRect.bottom) | ||
| const sentinelTop = Math.max(0, viewport.scrollTop + sentinelRect.bottom - viewportRect.bottom) | ||
| viewport.scrollTop = Math.max(sentinelTop, maxScrollTop) | ||
| return | ||
| } | ||
| viewport.scrollTop = Math.max(0, viewport.scrollHeight - viewport.clientHeight) | ||
| viewport.scrollTop = maxScrollTop | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find callers/tests of restoreLatest / restoreTimelineSafePosition that exercise the sentinel branch
rg -nP --type=ts -C3 '\brestoreTimelineSafePosition\s*\('
rg -nP --type=ts -C3 'data-timeline-bottom-sentinel'Repository: Astro-Han/pawwork
Length of output: 7756
🏁 Script executed:
cat -n packages/app/src/pages/session/session-timeline-scroll-anchors.test.ts | head -250Repository: Astro-Han/pawwork
Length of output: 8357
🏁 Script executed:
cat -n packages/app/src/pages/session/session-timeline-scroll-anchors.test.ts | tail -100Repository: Astro-Han/pawwork
Length of output: 3385
🏁 Script executed:
cat -n packages/app/src/pages/session/session-timeline-scroll-anchors.ts | sed -n '100,200p'Repository: Astro-Han/pawwork
Length of output: 4621
🏁 Script executed:
grep -n "bottomSentinel" packages/app/src/pages/session/session-timeline-scroll-anchors.test.tsRepository: Astro-Han/pawwork
Length of output: 43
🏁 Script executed:
cat -n packages/app/src/pages/session/use-session-timeline-interaction.ts | sed -n '190,210p'Repository: Astro-Han/pawwork
Length of output: 920
🏁 Script executed:
git log --oneline -n 20 -- packages/app/src/pages/session/session-timeline-scroll-anchors.tsRepository: Astro-Han/pawwork
Length of output: 126
🏁 Script executed:
cat -n packages/app/src/pages/session/message-timeline.tsx | sed -n '465,475p'Repository: Astro-Han/pawwork
Length of output: 495
Math.max makes the sentinel branch dead code — change to Math.min.
With viewport.scrollTop = Math.max(sentinelTop, maxScrollTop):
- If
sentinelTop ≤ maxScrollTop: result ismaxScrollTop - If
sentinelTop > maxScrollTop: assignedsentinelTop, but browser clamps tomaxScrollTop
Either way, viewport.scrollTop becomes maxScrollTop, which is identical to the no-sentinel fallback. The sentinel-aware computation never affects behavior.
The intended logic should scroll to the position placing the bottom sentinel flush with the viewport bottom, but never past the maximum scrollable position. That requires Math.min(sentinelTop, maxScrollTop), not Math.max.
Note: This code path is used in production (use-session-timeline-interaction.ts:194, 207) but has no test coverage.
🐛 Proposed fix
function restoreLatest(viewport: HTMLElement, bottomSentinel?: HTMLElement | null) {
const maxScrollTop = Math.max(0, viewport.scrollHeight - viewport.clientHeight)
if (bottomSentinel) {
const viewportRect = viewport.getBoundingClientRect()
const sentinelRect = bottomSentinel.getBoundingClientRect()
const sentinelTop = Math.max(0, viewport.scrollTop + sentinelRect.bottom - viewportRect.bottom)
- viewport.scrollTop = Math.max(sentinelTop, maxScrollTop)
+ viewport.scrollTop = Math.min(sentinelTop, maxScrollTop)
return
}
viewport.scrollTop = maxScrollTop
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/app/src/pages/session/session-timeline-scroll-anchors.ts` around
lines 116 - 126, In restoreLatest, the sentinel branch is effectively dead
because viewport.scrollTop is set with Math.max(sentinelTop, maxScrollTop) which
always yields maxScrollTop; change the assignment to use Math.min(sentinelTop,
maxScrollTop) so the scroll positions places the bottomSentinel flush with the
viewport bottom but never exceeds the maximum scrollable position (update the
line setting viewport.scrollTop inside restoreLatest that currently uses
Math.max to use Math.min instead).
Comment on lines
519
to
533
| if (observation.type === "scroll_sample") { | ||
| if (observation.metrics.nearBottom) { | ||
| updateObservedSafePosition(state, observation.safePosition ?? { kind: "latest" }) | ||
| if (state.lastIntent && isExplicitBottomIntent(state.lastIntent)) { | ||
| state.lastSafePosition = observation.safePosition?.kind === "latest" ? observation.safePosition : { kind: "latest" } | ||
| if (state.mode !== "targeting_message") { | ||
| state.mode = "following_latest" | ||
| state.latestProtected = true | ||
| return result({ | ||
| before, | ||
| observation, | ||
| accepted: true, | ||
| recovery: noRecovery, | ||
| reason: "explicit_bottom_navigation", | ||
| }) | ||
| state.latestProtected = false | ||
| } | ||
| return result({ | ||
| before, | ||
| observation, | ||
| accepted: true, | ||
| recovery: noRecovery, | ||
| reason: "explicit_bottom_navigation", | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there's no other branch in observe() that re-establishes the target anchor after this overwrite.
ast-grep --pattern $'observe(observation) {
$$$
}'
rg -nP --type=ts -C2 'lastSafePosition\s*='Repository: Astro-Han/pawwork
Length of output: 2935
🏁 Script executed:
#!/bin/bash
# 1. Check the condition at line 610 and surrounding context
sed -n '600,620p' packages/app/src/pages/session/session-timeline-scroll-controller.ts
# 2. Check updateObservedSafePosition function (lines 318-332)
sed -n '318,340p' packages/app/src/pages/session/session-timeline-scroll-controller.ts
# 3. Get line count of the file to understand structure
wc -l packages/app/src/pages/session/session-timeline-scroll-controller.tsRepository: Astro-Han/pawwork
Length of output: 1559
🏁 Script executed:
#!/bin/bash
# 4. Look at the observe function to find all branches that handle lastSafePosition after line 521
# Search for any reassignments of lastSafePosition or calls to updateObservedSafePosition after line 521
sed -n '500,650p' packages/app/src/pages/session/session-timeline-scroll-controller.ts | cat -nRepository: Astro-Han/pawwork
Length of output: 6319
🏁 Script executed:
#!/bin/bash
# 5. Check the test file to see if the described test exists and what scenarios are covered
find packages/app -name "*scroll-controller.test.ts" -o -name "*scroll-controller.spec.ts"Repository: Astro-Han/pawwork
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Check if the test "target message anchor is not overwritten by ordinary scroll samples" exists
# and what it tests
rg -A 15 "target message anchor is not overwritten" packages/app/src/pages/session/session-timeline-scroll-controller.test.tsRepository: Astro-Han/pawwork
Length of output: 505
🏁 Script executed:
#!/bin/bash
# Look for any existing test involving nearBottom and targeting_message
rg -B 2 -A 10 "nearBottom" packages/app/src/pages/session/session-timeline-scroll-controller.test.ts | head -50Repository: Astro-Han/pawwork
Length of output: 692
scroll_sample nearBottom branch overwrites the target anchor even in targeting_message mode.
Line 521 unconditionally writes state.lastSafePosition to { kind: "latest" } (or the supplied latest safePosition). The mode guard on lines 522–525 protects mode and latestProtected, but lastSafePosition is updated for all modes — including targeting_message.
This breaks the invariant the rest of observe relies on:
- Line 111 requires
state.mode === "targeting_message" && state.lastSafePosition.kind === "target_message"to restore the target onwindow_changed/dock_resize/content_resize. Once the kind has been demoted to"latest", that branch is dead and observations fall through to the generic "weak_scroll_observed" path at line 135, so the target anchor is silently lost. updateObservedSafePosition(lines 323–332) deliberately refuses to overwrite atarget_messageanchor with a non-target sample. The new nearBottom branch bypasses that guard.
Repro: user navigates to a message that is rendered near the bottom; the next scroll_sample arrives with nearBottom; subsequent agent-append content_resize / window_changed events no longer restore the target. The existing "target message anchor is not overwritten by ordinary scroll samples" test only exercises middleMetrics and won't catch this.
🐛 Proposed fix
if (observation.type === "scroll_sample") {
if (observation.metrics.nearBottom) {
- state.lastSafePosition = observation.safePosition?.kind === "latest" ? observation.safePosition : { kind: "latest" }
- if (state.mode !== "targeting_message") {
+ if (state.mode !== "targeting_message") {
+ state.lastSafePosition =
+ observation.safePosition?.kind === "latest" ? observation.safePosition : { kind: "latest" }
state.mode = "following_latest"
state.latestProtected = false
}
return result({
before,
observation,
accepted: true,
recovery: noRecovery,
reason: "explicit_bottom_navigation",
})
}Suggested companion test:
test("target message anchor survives a nearBottom scroll sample", () => {
const { controller } = makeController()
controller.intent({ type: "target_message", messageID: "msg_target", align: "nearest" })
controller.observe({
type: "scroll_sample",
metrics: bottomMetrics,
safePosition: { kind: "latest", messageID: "msg_latest" },
})
expect(controller.state().mode).toBe("targeting_message")
expect(controller.state().lastSafePosition.kind).toBe("target_message")
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (observation.type === "scroll_sample") { | |
| if (observation.metrics.nearBottom) { | |
| updateObservedSafePosition(state, observation.safePosition ?? { kind: "latest" }) | |
| if (state.lastIntent && isExplicitBottomIntent(state.lastIntent)) { | |
| state.lastSafePosition = observation.safePosition?.kind === "latest" ? observation.safePosition : { kind: "latest" } | |
| if (state.mode !== "targeting_message") { | |
| state.mode = "following_latest" | |
| state.latestProtected = true | |
| return result({ | |
| before, | |
| observation, | |
| accepted: true, | |
| recovery: noRecovery, | |
| reason: "explicit_bottom_navigation", | |
| }) | |
| state.latestProtected = false | |
| } | |
| return result({ | |
| before, | |
| observation, | |
| accepted: true, | |
| recovery: noRecovery, | |
| reason: "explicit_bottom_navigation", | |
| }) | |
| } | |
| if (observation.type === "scroll_sample") { | |
| if (observation.metrics.nearBottom) { | |
| if (state.mode !== "targeting_message") { | |
| state.lastSafePosition = | |
| observation.safePosition?.kind === "latest" ? observation.safePosition : { kind: "latest" } | |
| state.mode = "following_latest" | |
| state.latestProtected = false | |
| } | |
| return result({ | |
| before, | |
| observation, | |
| accepted: true, | |
| recovery: noRecovery, | |
| reason: "explicit_bottom_navigation", | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/app/src/pages/session/session-timeline-scroll-controller.ts` around
lines 519 - 533, The nearBottom branch of observe currently assigns
state.lastSafePosition to latest unconditionally, which can overwrite a
target_message anchor; update the nearBottom handling in observe so it does not
change state.lastSafePosition when the controller is currently targeting a
message whose lastSafePosition.kind === "target_message" (i.e., if state.mode
=== "targeting_message" && state.lastSafePosition?.kind === "target_message"
then skip updating lastSafePosition), but still perform the existing mode/
latestProtected updates and return the same result; this preserves the invariant
used by the target restoration logic and aligns with
updateObservedSafePosition's intent.
…e dirty tail Streaming flushes were re-running `temp.innerHTML + decorate + morphdom` against the full visible message every 24 ms — the trace's largest single contributor to INP 200+ in PR #589's session view. The cache around `Markdown` already shared parsed HTML between flushes, but every flush still tore down and rebuilt the entire DOM tree, so cache hits saved a parse without saving the DOM work. `stream()` now emits a separate stable head block and dirty tail block whenever the lexer produced more than one top-level token, alongside the existing unclosed-fence split. Reference-style links and single-token texts stay as one dirty block — splitting them would risk orphaning inline tokens that resolve against later definitions. `<Markdown>` builds a wrapper child per block, keyed by `cacheKey + html + copy/copied labels`. A wrapper whose fingerprint is unchanged is left entirely alone — no innerHTML, no decorate, no morphdom — so the dirty tail's growth no longer drags closed prose above it back through the full pipeline. Labels live in the fingerprint so an i18n locale switch still re-decorates already-rendered code blocks (W1 i18n gate). When abort or settle collapses N dirty wrappers into a single full-mode block, trailing wrappers are trimmed before write so no orphan blocks linger. Tests: - `markdown-stream.test.ts`: existing cases extended with `stable`; new multi-token boundary + half-list / half-link / half-fence edge cases + a stability invariant property (same head text → identical head block across streaming progressions). - `markdown.test.ts`: new contract for `renderBlocksToContainer` covering wrapper identity preservation, abort consistency, stable-key-with-changed-html must update, labels-change must invalidate copy button decoration, and the copy-button-not-duplicated guarantee under stable skip. - `message-part-markdown.test.ts` (new): pins `createPacedValue`'s lifecycle contract (settle / abort / non-prefix sync / settle-not-permanent). The existing helper already satisfies it; the spec is for regression coverage. `packages/ui/package.json`'s `test` script gains `--conditions=browser` so Bun resolves SolidJS through its browser entry — the SSR entry has no reactive runtime and silently bypasses the lifecycle assertions. Scoped to this package only; the root and other workspaces are unaffected. Part of PR #589 C-min perf treatment (5 cuts): Cut 1 of 5.
…t round While an agent round is actively streaming (`[data-running]` on the round element), the visible body churns frame-by-frame as the dirty tail grows. Stacking trow expand / collapse height transitions and the per-tool Collapsible.Content height transition on top of those layout passes was contributing to the main-thread budget pressure GPT-X Pass 2 traced — both animate `height: auto`, which re-resolves layout on every frame as new tokens arrive. Suppress them inside the streaming scope only; the user's own expand / collapse interactions after streaming settles still get the full animation. For the same reason, every `<TextShimmer>` inside the streaming round's `[data-slot="agent-body"]` falls back to a static base render — animation off, char-shimmer faded, char-base forced visible. The shimmer in the agent header (`[data-slot="agent-status"]`, outside the agent body) is the one allowed running instance per the C-min plan's "shimmer 收敛到 agent header 单实例" boundary. The override mirrors the `prefers-reduced-motion` block already in `text-shimmer.css`. CSS only. The 5-minute visual checklist (`docs/design/session-view-w1-manual-checklist.md`) covers verification. Part of PR #589 C-min perf treatment (5 cuts): Cut 4 of 5.
`SessionSidePanel`'s effect calls `view().terminal.close()` whenever the
panel collapses or its active tab leaves "terminal", but the side panel's
own tab content gate (`<Show when={sidePanelTab() === "terminal"}>`)
checked only the active tab — not `open()`. The result, called out in
GPT-X's Pass 2 trace, is that ghostty-web's `startRenderLoop` kept rAF'ing
even after the user collapsed the panel: `<Terminal>` stayed mounted
because `TerminalPanel`'s own `<Show when={terminal.active()}>` is keyed
on the active terminal id, which doesn't clear on panel-driven view
closes. The trace recorded `startRenderLoop` running 4565 times for 598ms
of JS plus 245ms of style during a session that wasn't even surfacing the
terminal panel.
Gating `<Show>` on `opened() && terminal.active()` unmounts `<Terminal>`
through its existing cleanup / persist / websocket close path; reopening
remounts and reconnects normally. ghostty-web's internal render loop is
intentionally untouched — that lives behind the encapsulation boundary
and is a P1 follow-up if hidden-tab dirty-render becomes a real-world
cost again.
Verification: `bun run typecheck` clean across app + ui, focused
`bun test packages/app/src/pages` 511/511 pass, ui `bun run test` 502/3
unchanged (3 pre-existing W1 baseline drift). The visual side of the
behavior — `<Terminal>` actually unmounting on panel collapse — relies on
the 5-minute checklist's manual DevTools profile pass (Lead's brief
acknowledged spec is too costly to wire through the ghostty-web boundary).
Part of PR #589 C-min perf treatment (5 cuts): Cut 5 of 5.
Owner
Author
|
Closing in favor of #599. The 5-cut perf cluster (commits 969f6a9, f7646cb, 61e7a0f, aed3994, f49fd6d) regressed home INP from ~213-240ms to 199-3120ms on retest. Root cause and lessons captured in #599 body (postmortem section). Message-flow rewrite continues in #601 behind the perf gate from #600. |
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rewrites the session view (conversation flow surface) against the W1 visual lock for issue #440 slice 11b.1, and finishes the work by mounting the new W1 leaves on the default user path so the visual is reachable from a fresh
bun run dev:desktop.groupPartsreducer +TrowBlocksummary reducer with full unit tests.session-turn-user-bubble·session-turn-agent-round·session-turn-event·session-turn-jump·session-turn-trow-block— context-free, caller-injected i18n labels + render slots, focus-within keyboard a11y.session-turn.tsx): 748 → 446 lines. ExtractedTurnChangesList+SessionTurnDiffsList+ pure helpers (session-turn-helpers.ts) into siblings.session-turn.tsx+message-timeline.tsx):<Message>and<AssistantParts>are replaced on the default user path by<SessionTurnUserBubble>and<SessionTurnAgentRound>; the<JumpToBottom>leaf replaces the inline ↓ floating button inmessage-timeline.tsx. Bridge logic (model name resolver,partsByMessagemap, partID → owner-message lookup,isLatestRoundderivation, i18n label bundles, action wrappers,renderProse/renderToolslots) lives in a new siblingsession-turn-leaf-bridges.tsxso the shell stays ≤500 lines.actions.forkis wired through to a newforkRoundhandler insession.tsxthat mirrors the existingDialogFork.handleSelectSDK flow (no dialog — the message id is implicit from the round the toolbar lives on). Slice 11b.1 §6.14 rapid-click guard included.message-part.tsx2459 → 91 lines. Pulled User/Assistant displays, registry, markdown helpers, tool-info, render-groups, tool dispatcher + display chrome, core part renderers, and four tool-category modules (basic / agent / shell / file) into focused siblings; the aggregator now hosts only the role dispatcher + public barrel re-exports.message-timeline.tsx1211 → 467 lines. Extracted seven helpers (staging·scroll·comments·history-load·turn-changes·row·websearch) and dropped orphan title-editor / session-export code left behind byb49b67999(verified no JSX consumption, no exports, no external consumers via git grep).forkin the icon registry (lucide GitFork-style stand-in pending the Sheet 15 candidate selection by @Astro-Han); sharedAttachmentChipprimitive.--fg-secondary(reasoning text) and--icon-chev(trow chevron color) registered inpackages/ui/src/style/theme.csslight + dark + media-query block + mirrored inpackages/ui/src/theme/themes/pawwork.jsonper thetheme-parity.test.tsliteral-value contract.ui.sessionTurn.workingTime,ui.sessionTurn.trow.summary.{running,completed,withFailed},ui.sessionTurn.trow.failed,ui.sessionTurn.bubble.fork— added to bothzhandenin lock-step.packages/app/e2e/session/covering the 16 golden-path routes from issue [Feature] Rewrite packages/ui + packages/app visual layer against PawWork design system (incremental PR series) #440 §5.2 —session-w1-bubble.spec.ts(E1, E7, E8, E9, E10),session-w1-trow.spec.ts(E3, E4, E11, E12, E13),session-w1-events.spec.ts(E2, E5, E15, E16),session-w1-jump.spec.ts(E6),session-w1-error.spec.ts(E14). Routes that issue [Feature] Rewrite packages/ui + packages/app visual layer against PawWork design system (incremental PR series) #440 §5.2 explicitly defers to unit / visual (focus-visible state matrix, working-time precise timing, shimmer rhythm, reduce-motion, i18n copy switch) stay in the unit suite.Hybrid surface revoked: an earlier hybrid
SessionTurnV2opt-in mount was reverted (commit0fbee2945) in favor of the design-doc-mandated in-place rewrite —docs/design/preview/message-flow.html§1 explicitly forbids co-existence of the old and newsession-turn.tsx. The Phase 2b integration commit (cb90f6b5c) is the moment the W1 surface becomes the default user path.AGENTS.md ≤500 line rule satisfied:
session-turn.tsx497,session-turn-leaf-bridges.tsx176, …).PR is held in
in_reviewuntil @Astro-Han runs the dev:desktop checklist hands-on. No option-b auto-merge signal will be posted — visual-impact PRs must clear hands-on validation before merge.Why
DESIGN.md L406-L477 and workshop preview
docs/design/preview/message-flow.html(locked 2026-05-12) replace the current session-turn / message-part outer shell with a flat agent round + cream user bubble + interleaved tool-status rows + always-visible working-time header + hover toolbars + system event line + ↓ floating button. The pre-rewrite files were 748 / 2459 / 1211 lines respectively — well over the AGENTS.md 500-line ceiling and impossible to review at the structural level. This rewrite splits each into single-responsibility files so the right-pane work in slice 11b.2 has a frozen public contract to import, and ships the integration commit that makes the W1 visual the live default — design doc §1 forbids co-existence.Related Issue
Refs #440 (parent), #154 and #65 (sibling concerns, already closed).
Human Review Status
Status: in_review — pending hands-on dev:desktop verification by @Astro-Han.
Per slice 11b.1 safety brief, this PR must not be merged on diff-only review. The 17-item dev:desktop checklist (light/dark, attachments, working time tick, trow grouping transitions, shimmer rhythm, ↓ button, system event, reset path, fork path, palette /fork backup, multi-turn scroll, reduce-motion, long prose, trow error, raw output, focus matrix, i18n switch) is reproduced under How To Verify for @Astro-Han to run before merge.
Review Focus
docs/design/preview/message-flow.html): bubble cream + radius-lg, attachments above bubble, agent flat (no turn-frame),mf-agent-headeralways-visible working-time tick, hover toolbar 8 / 4 / 12 group spacing without·separators, shimmer 1800ms across all callsites.cb90f6b5c): the shell adapter from the legacy<Message>/<AssistantParts>path to the W1 leaves. The bridge factory insession-turn-leaf-bridges.tsxresolves the model display name, builds thepartsByMessagemap for the agent-round'sgroupParts()walk, resolves the partID → owner-message lookup for the trowrenderToolslot, derivesisLatestRound, wrapsactions.revertandactions.forkinto the leaves' zero-arg callbacks, and injects<PacedMarkdown>+<Part>as the prose / tool render slots so per-tool rich bodies (file accordion, raw output, copy button on hover) keep working. No behavior shift forassistantCopyPartID,turnDurationMs,showAssistantCopyPartID, or auto-scroll anchoring.SessionTurn,JumpToBottom,UserMessageDisplay,AssistantMessageDisplay,MessageTimeline,useTimelineScrollare exported. The new sibling modules (message-part-*,message-timeline-*,session-turn-*-list,session-turn-helpers,session-turn-leaf-bridges,session-turn-{user-bubble,agent-round,event,jump,trow-block}) are private to this package and should stay so.message-part-group.ts): prose and reasoning act as flush boundaries between consecutive tool runs; hidden tools (todowrite), runningquestiontools, empty text, and unknown SDK part types are filtered without flushing — please scrutinise the test matrix and the unknown-type silent-skip path.:focus-withinadded next to:hoveron bubble/agent toolbars (keyboard-only users); agent toolbar usesvisibility: hidden+pointer-events: none+aria-hidden(not opacity-only) while running, to keep Tab order from reaching inert buttons.SessionTurnUserBubbleresettingsignal) and the newforkRoundhandler insession.tsx(top-levelforkingsignal) both gate concurrent SDK calls. Reset is idempotent, Fork is not — the guard is mandatory for Fork.titleMutation/onExport/openTitleEditor/closeTitleEditor/saveTitleEditor/navigateParent/errorMessage/exportAvailable/tint/workingStatus/timeoutDoneplus their dependent imports were left behind byb49b67999("refactor(app): drop duplicate session title bar"). git grep verified zero JSX consumption, zero exports, zero external readers; the cleanup is part of this PR rather than a follow-up to keepmessage-timeline.tsx≤500 lines without artificial padding.Risk Notes
Pre-existing dev baseline has 3 unrelated failing tests in
packages/ui(not introduced by this PR):Button — single canonical size (28px) > height 28px declared outside any variant selectorIconButton — single canonical size (24×24) > width 24px declared at root levelIconButton — single canonical size (24×24) > height 24px declared at root levelRoot cause: fixture rot — these tests still assert the pre-rebalance sizes after Button / IconButton CSS was updated to W1 spec (iconbtn 24→30, radius-sm→radius-md). These tests are not in any required CI check (
unit-app/unit-opencode/unit-desktopturbo filters do not runpackages/uitests). Tracked as a separate follow-up.Fork glyph is a placeholder: a lucide GitFork-style stand-in is registered in
packages/ui/src/components/icon.tsxasfork. The dedicated Sheet 15 candidate will be selected by @Astro-Han; a one-line replacement updates the SVG once the final candidate is chosen — no other code changes required.Local-only docs reconciliation: any follow-up edits to
docs/DESIGN.mdanddocs/design/preview/message-flow.htmlare excluded via.git/info/excludeand do not appear in the PR diff. The code changes here track the visual lock as of 2026-05-12; doc edits land separately.Long-session performance: virtualization is explicitly out of slice 11b.1 (excluded by [Feature] Rewrite packages/ui + packages/app visual layer against PawWork design system (incremental PR series) #440 / [Task] Optimize long session timeline rendering to prevent streaming flicker #397).
<For>keyed reconciliation +createMemowith referential equality keep re-render scope tight, but a 100+ turn session is not load-tested in this PR.E2E coverage scope: the five new spec files focus on structural / behavioural assertions for the W1 surface (DOM presence, click-through wiring, render-slot dispatch). Visual rhythm (shimmer cadence, reduce-motion) and timing-sensitive items (precise working-time tick rate) stay in the unit suite and the dev:desktop checklist per issue [Feature] Rewrite packages/ui + packages/app visual layer against PawWork design system (incremental PR series) #440 §5.2 — those routes are unstable in Playwright fake-timer scenarios.
How To Verify
Local:
bun install && bun run dev:desktop, then walk the 17-item checklist below in light + dark.CI:
tsgo --noEmitGREEN acrosspackages/uiandpackages/app.bun test packages/ui478 pass / 3 baseline fail (Button/IconButton fixture rot — pre-existing, see Risk Notes).bun test packages/app1050 pass / 0 fail.bun test:e2e -- session/session-w1-*.spec.tscovers E1-E16.typecheck/analyze-js-ts/unit-app/unit-desktop/unit-opencode/smoke-macos-arm64/CodeRabbit/pr-title-lint/ CodeQL.Hands-on dev:desktop checklist (@Astro-Han):
--fg-secondary+--icon-chevresolve to design-doc values; no fallback flashes on initial paint.已工作 16sticks every second while the round runs; freezes at the final value when the round completes (no spinner, no pulsing dot).--fg-weakat gap 12 to neighbours; no icon, no background, no prefix.prefers-reduced-motion: reducedisables shimmer + transitions + the trow expand animation.aria-hidden+pointer-events: none);:focus-withinreveals the hover toolbar.Test Plan
bun run check(turbo: typecheck + lint + format)bun test packages/ui(478 pass / 3 baseline fail — see Risk Notes)bun test packages/app(1050 pass / 0 fail)bun test:e2e -- session/session-w1-*.spec.ts(CI run)bun run dev:desktophands-on 17-item checklist (@Astro-Han)Screenshots
Hands-on screenshots from @Astro-Han's dev:desktop run will be attached before merge.
Notes for Reviewers
SessionTurn,JumpToBottom,UserMessageDisplay,AssistantMessageDisplay,MessageTimeline,useTimelineScroll) is re-exported from the barrel files. The new sibling modules (message-part-*,message-timeline-*,session-turn-{user-bubble,agent-round,event,jump,trow-block,leaf-bridges,*-list,helpers}) are intentionally internal so a future reorganization doesn't break out-of-package consumers.import "./module"statements in the aggregators so the load order is grep-able.session-turn.tsxsymbol. The earlier hybridSessionTurnV2mount was reverted in0fbee2945; Phase 2b commitcb90f6b5cis the moment the W1 leaves replace the legacy renderers insession-turn.tsx's JSX.Summary by CodeRabbit
New Features
Improvements