Skip to content

fix(ui): tighten grouped activity spacing - #446

Open
Zerlight wants to merge 2 commits into
ruocheng/code-576from
ruocheng/code-376
Open

fix(ui): tighten grouped activity spacing#446
Zerlight wants to merge 2 commits into
ruocheng/code-576from
ruocheng/code-376

Conversation

@Zerlight

Copy link
Copy Markdown
Member

Summary

  • tighten spacing only for activities nested inside expanded activity groups
  • preserve normal spacing for group headers and top-level activities
  • cover standard, tooltip-backed, and bodyless activity rows in the bundled browser smoke

Why

Grouped activity rows used the same block padding as top-level rows, which weakened the visual hierarchy. The group now scopes denser padding to each direct child header.

Validation

  • pnpm check:ci
  • pnpm test --maxWorkers=2 --maxConcurrency=2
  • pnpm -F @linkcode/webview run e2e:browser

Linear: CODE-376

@linear-code

linear-code Bot commented Aug 12, 2026

Copy link
Copy Markdown

CODE-376

@Zerlight
Zerlight marked this pull request as ready for review August 12, 2026 14:12
Copilot AI lite review requested due to automatic review settings August 12, 2026 14:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Tightens the visual hierarchy of chat activity runs by reducing padding for tool/thought rows rendered inside an expanded activity run, while leaving the run header and non-grouped activity rows unchanged. It also expands the mock showcase and bundled webview smoke test to exercise tooltip-backed and bodyless activity rows under the new spacing.

Changes:

  • Scope denser vertical padding to the first child header of each collapsible activity row inside an expanded ActivityRun.
  • Extend the showcase mock activity run with a “bodyless” read tool call entry.
  • Add a bundled browser-smoke assertion that expanded activity-run children render with denser padding than the run header, covering tooltip-trigger and non-tooltip rows.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/presentation/ui/src/chat/activity-run.tsx Applies a scoped Tailwind arbitrary selector so nested activity rows render with tighter vertical padding inside expanded activity runs.
packages/client/workbench/src/mock/data/showcase.ts Adds a bodyless read tool-call fixture used to exercise non-tooltip activity rows in the showcase thread.
apps/webview/e2e/browser-smoke.e2e.mts Adds a smoke check that expands the showcase activity run and validates nested row hierarchy/padding (including tooltip-trigger + bodyless rows).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread apps/webview/e2e/browser-smoke.e2e.mts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ One assertion-message nit inline; the change itself verified out cleanly.

Reviewed changes — a one-line spacing fix in ActivityRun plus the mock row and browser-smoke check that pin it. This PR is stacked on ruocheng/code-576, so the diff is against that branch, not master.

  • Denser grouped rowsactivity-run.tsx:157 extends the expanded-group body class with [&>[data-slot=collapsible]>*:first-child]:py-0.5, dropping each direct child header from py-1 (4px) to py-0.5 (2px) while the group trigger and top-level rows keep py-1.
  • Bodyless showcase rowshowcase.ts adds a read call with no content, rawInput, or locations, so hasToolBody() is false and its header renders as a plain div instead of a trigger button.
  • Browser-smoke hierarchy checkverifyActivityRunHierarchy expands the mixed showcase run and asserts every child header's padding-block-start is non-zero and smaller than the group header's.

What I verified rather than assumed:

  • The arbitrary variant actually compiles and wins the cascade. Running the candidate through @tailwindcss/node (v4.3.3) emits … > [data-slot=collapsible] > :first-child { padding-block: calc(var(--spacing) * 0.5) } — specificity (0,3,0) against .py-1's (0,1,0), and emitted after it in the same layer. @source "./" in packages/presentation/ui/src/styles.css covers the file, so the class is scanned.
  • All three header shapes are matched. Every branch of the run.items.map bottoms out in a coss-ui Collapsible root whose first element child is the header: ToolCallItem/QuestionCallItem via Tool, ThoughtBlock via Reasoning. The tooltip-backed case is base-ui's TooltipTrigger render={…}, which merges onto the same node rather than wrapping it, so *:first-child still lands on the real header.
  • The new check is CI-enforced, not local-only — ci.yml:215 runs pnpm -F @linkcode/webview e2e:browser unconditionally on pull requests — and it genuinely fails without the fix, since the children would otherwise measure equal to the group header.
  • The mock addition doesn't disturb the one suite that asserts against the showcase session: dev-mock-transport.test.ts passes (13/13) with the new tool call. Resulting row height is 24px (text-sm's 20px line box + 2×2px), exactly the WCAG 2.2 AA target minimum, and no theme overrides --text-sm--line-height or --spacing.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread apps/webview/e2e/browser-smoke.e2e.mts Outdated
Copilot AI review requested due to automatic review settings August 12, 2026 15:06

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changesb3c819f is test-only hardening of the browser-smoke check added in 8f31665; the ActivityRun spacing fix and the showcase mock row are unchanged since the last review.

  • Loosened the activity-run locator — the exact accessible name 'Activity details: An action failed · Ran a command · Made a file change · Explored 2 times' is replaced by RE_ACTIVITY_RUN_DETAILS (browser-smoke.e2e.mts:25-26), so the check no longer breaks when a clause count shifts.
  • Added failure messages to the two bare asserts — the tooltip-trigger and bodyless-row checks now dump metrics.children, matching their neighbours.

The one thing worth verifying about the looser locator is whether it can now match a second button and trip Playwright's strict mode. It can't — I enumerated every activity run the showcase thread renders and derived each aria-label:

  • Bursts are separated by agent-message-chunk narration (dev-mock-host.ts:1391-1428), and a non-tool item flushes the in-progress run (activity-groups.ts:85-88), so each burst becomes its own run.
  • exploreExplored 4 times; filesMade 6 file changes; commandsAn action failed · Ran 3 commands; afterTaskMade a file change · Explored once. None carries all four clauses the regex requires.
  • Only activityRun.beforeNarration matches, and only its first five items: the preceding agent-thought-chunk (dev-mock-host.ts:1416) joins as ACTIVITY_RUN_GLUE_KEY glue while mock-activity-integration classifies as brand:linear and flushes the run (activity-groups.ts:44-46, 90-91). Reasoning + read + read-bodyless + execute-failed + edit — exactly the 5 children children.length >= 5 expects.

Dropping Explored 2 times from the locator doesn't cost coverage either: the bodyless row that clause stood for is pinned directly by the tagName === 'DIV' assertion, which is now the one that reports what went missing.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants