feat(ui): unify the activity pane's rows and make them navigable - #3909
Conversation
…targets The activity timeline mixed a `sm` avatar on the "created this task" row with `lg` ones on every row below it, and quill's `ThreadItemAuthor`/`ThreadItemBody` default to 14px while the pane's own copy is 13px — so a row's name read as a heading over its own message. - Every timeline node is now a `sm` avatar (or the matching `size-6` status bubble), centered in the 2.5rem gutter; the vertical line moves to 1.75rem to stay under them. - Author names and body copy share one 13px size across the pane. - PR cards in the timeline and the Artifacts tab now open the in-app review split when clicked, with a dedicated trailing button for GitHub. Previously the timeline card only ever left the app and the artifacts row only ever stayed in it. Generated-By: PostHog Code Task-Id: 3da5213f-90f2-4123-bbbd-c648c7b08e06
|
😎 Merged successfully - details. |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
The pane had three typographic treatments and two row skeletons competing, so nothing lined up. It now has exactly two row species: - Lifecycle events (task created, run finished) are an icon bubble plus a single muted line. "Task created" gets its own icon rather than the creator's avatar, so it reads the same as "Task completed" instead of impersonating an authored row. - Authored rows (messages, PR/canvas artifacts) keep the avatar and author line, with content on its own line below. Also: - Message rows preview the first non-empty line and truncate, rather than clamping four lines of wrapped text. The Comments tab still shows messages in full — that's where you read them. - Content sits 0.375rem below its header, so the PR card is no longer jammed against its title. - Timestamps ride a right-edge column via `ThreadTimestamp`'s className. The panel-level `[data-slot=thread-item-timestamp]` override this replaces never matched: quill's `TooltipTrigger` overwrites the wrapped element's `data-slot` with its own, which is why timestamps had been hugging the names. Generated-By: PostHog Code Task-Id: 3da5213f-90f2-4123-bbbd-c648c7b08e06
…pe pass Typography: - Timestamps drop to 11px, a step below the row's 13px copy. - Event rows take the same text colour as every other row. Only the icon distinguishes them now, so the pane reads as one typographic system rather than greying out its own lifecycle markers. Clicking a message row scrolls the transcript to that message. The Activity pane is a sibling of the transcript, so it can't reach the scroller's context or the windowed body's jump callback — a small `threadNavigationStore` carries the request and each transcript serves it with its own jump, mirroring how `reviewNavigationStore` already brokers scroll-to-file between the changes list and the review pane. Both transcripts are wired, since which one renders depends on the `useNewChatThread` setting: `ConversationView` reuses its existing `handleJumpToMessage` (grouped-row index), and `ChatThread` gets a bridge inside `ChatMessageScrollerProvider` that prefers the windowed body's `jumpToMessage` and falls back to the engine's `scrollToMessage`. Both panes derive items from the same `useSessionViewState` events, so the conversation item ids match. Only message rows are clickable — lifecycle events and artifacts have nothing to scroll to. The row carries the button role itself: quill's `ThreadItem` renders an <article>, which a <button> may not wrap and which takes no `render` prop. Generated-By: PostHog Code Task-Id: 3da5213f-90f2-4123-bbbd-c648c7b08e06
…ivity pane diff Self-review pass. One bug, the rest is fat. The bug: `ActivityTimeline` renders in two surfaces, and only one of them mounts the task's own view. In the channel-home sidebar (`WebsiteChannelHome`) there is no `TaskDetail`, so opening a PR "in place" set review state nobody reads and a scroll request nobody serves — both silently did nothing, and the PR card used to open GitHub. A `canOpenInPlace` flag, set only by the task route where the transcript and review pane sit alongside, now gates both: without it message rows stay inert and PRs open externally. That also fixes the same pre-existing dead click on the Artifacts tab's PR row, which is where the pattern came from. Trimmed: - Dropped four exported `THREAD_*_CLASS` constants in favour of the literal Tailwind classes. Each cost a docblock, an export and a cross-file import to avoid repeating one class; the timestamp's alignment moved into `ThreadTimestamp`, which owns timestamps, so no caller passes classes at all. - Replaced `messagePreview()` and its tests with `line-clamp-1 whitespace-pre-wrap`. The file already clamped at four lines; changing the number is the whole edit. - `ThreadTimeline` no longer takes a `taskId` it only forwarded, and `ArtifactCardButton`/`ArtifactListRow` derive the external button's label from the title instead of taking a prop for it. - Folded the scroll-request hook into `threadNavigationStore` (never used apart) and cut its test from four cases to two. - `ThreadScrollRequestBridge` loses a `useCallback` and a branch: the hook's effect no-ops unless a request is pending, so `jumpToMessage ?? scrollToMessage` is enough. Kept the bridge rather than dropping it — `useNewChatThread` is off by default, but cutting it would reintroduce exactly the dead click above for anyone who flips that setting. 485+/145- down from 562+/140-, with the bug fix included. Generated-By: PostHog Code Task-Id: 3da5213f-90f2-4123-bbbd-c648c7b08e06
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 1 issue
Files (11)
packages/ui/src/features/canvas/components/ActivityPanel.tsxpackages/ui/src/features/canvas/components/ActivityTimeline.tsxpackages/ui/src/features/canvas/components/TaskArtifactsList.tsxpackages/ui/src/features/canvas/components/ThreadPanel.tsxpackages/ui/src/features/canvas/components/ThreadSidebar.tsxpackages/ui/src/features/canvas/components/ThreadTimestamp.tsxpackages/ui/src/features/code-review/openPrInReview.tspackages/ui/src/features/sessions/components/ConversationView.tsxpackages/ui/src/features/sessions/components/chat-thread/ChatThread.tsxpackages/ui/src/features/sessions/threadNavigationStore.tspackages/ui/src/router/routes/website/$channelId/tasks/$taskId.tsx
| const name = author ? userDisplayName(author) : "You"; | ||
| // The row itself is the hit target. `ThreadItem` renders an <article>, which a | ||
| // <button> may not wrap and which can't become one (quill's primitive takes no | ||
| // `render`), so it carries the button role and its own key handling. | ||
| const activation = onSelect | ||
| ? ({ | ||
| role: "button", | ||
| tabIndex: 0, | ||
| "aria-label": `Jump to message from ${name}`, | ||
| onClick: onSelect, | ||
| onKeyDown: (event: KeyboardEvent<HTMLElement>) => { | ||
| if (event.key !== "Enter" && event.key !== " ") return; | ||
| event.preventDefault(); | ||
| onSelect(); | ||
| }, | ||
| } as const) | ||
| : {}; |
There was a problem hiding this comment.
Clickable message row's aria-label hides the visible name/timestamp/content from assistive tech and is identical for every message in the timeline
Why we think it's a valid issue
- Checked: PR-head
UserMessageRow(ActivityTimeline.tsx:75-91—role="button"+aria-label={\Jump to message from ${name}`}applied to the wholeThreadItem`), the caller loop (lines 162-176), and ARIA accessible-name precedence rules. - Found:
aria-labelon arole="button"element is the accessible name and overrides any name composed from descendant text (W3C AccName computation), so the focused announcement is only 'Jump to message from {name}, button' — the timestamp and one-line content preview are excluded from the name. This role is new in this PR; previously the row was a non-interactive<article>whose author/timestamp/body were readable as ordinary content, so the PR is what removes that info from AT. - Found: Every
user_messagerow is rendered withauthor={task.created_by}(line 169), soname— and thus the entire aria-label — is identical for all clickable rows. Deterministic for any task with ≥2 user messages. - Impact: Screen-reader and voice-control users get a list of buttons with byte-for-byte identical accessible names and cannot target a specific message, and lose the per-row timestamp/content that sighted users see — the jump-to-message affordance this PR adds is effectively unusable via assistive tech. Concrete trigger + concrete consequence, non-speculative, not style/taste, not handled elsewhere in the label. Meets the keep bar; the codebase clearly invests in a11y (this PR adds multiple aria-labels), and the suggested per-row detail /
aria-labelledbyfix is actionable.
Issue description
When onSelect is present, UserMessageRow applies role="button" plus aria-label={Jump to message from ${name}} to the whole ThreadItem. Per standard accessible-name computation, an explicit aria-label on an element with a widget role overrides the accessible name/description that would otherwise be built from its rendered children — so a screen reader user tabbing to this row hears only "Jump to message from {name}, button" and never the timestamp or the one-line content preview that a sighted user can see. That's a real loss of information sighted users get for free, introduced by this PR's own change (the row had no interactive role before). It's also concretely worse than a normal same-author collision: in this loop, author is always task.created_by for every user_message conversationItem (line ~169's author={task.created_by}), so name — and therefore the entire aria-label string — is byte-for-byte identical across every clickable message row rendered in a given task's Activity timeline. A screen-reader or voice-control user has no way to distinguish "jump to the first message" from "jump to the fifth message" by label alone; every row in the list announces exactly the same thing.
Suggested fix
Include something that varies per row in the accessible name — e.g. append a formatted time (Jump to message from ${name} at ${time}) or a short snippet of the clamped content — or better, point aria-labelledby at the row's existing author + timestamp + body elements (giving each row real ids) so the accessible name mirrors what's actually shown instead of a synthesized, invariant template string.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/canvas/components/ActivityTimeline.tsx#L75-91
@packages/ui/src/features/canvas/components/ActivityTimeline.tsx#L169-176
<issue_description>
When `onSelect` is present, `UserMessageRow` applies `role="button"` plus `aria-label={`Jump to message from ${name}`}` to the whole `ThreadItem`. Per standard accessible-name computation, an explicit `aria-label` on an element with a widget role overrides the accessible name/description that would otherwise be built from its rendered children — so a screen reader user tabbing to this row hears only "Jump to message from {name}, button" and never the timestamp or the one-line content preview that a sighted user can see. That's a real loss of information sighted users get for free, introduced by this PR's own change (the row had no interactive role before). It's also concretely worse than a normal same-author collision: in this loop, `author` is always `task.created_by` for every `user_message` conversationItem (line ~169's `author={task.created_by}`), so `name` — and therefore the entire aria-label string — is byte-for-byte identical across every clickable message row rendered in a given task's Activity timeline. A screen-reader or voice-control user has no way to distinguish "jump to the first message" from "jump to the fifth message" by label alone; every row in the list announces exactly the same thing.
</issue_description>
<issue_validation>
- **Checked:** PR-head `UserMessageRow` (`ActivityTimeline.tsx:75-91` — `role="button"` + `aria-label={\`Jump to message from ${name}\`}` applied to the whole `ThreadItem`), the caller loop (lines 162-176), and ARIA accessible-name precedence rules.
- **Found:** `aria-label` on a `role="button"` element is the accessible name and overrides any name composed from descendant text (W3C AccName computation), so the focused announcement is only 'Jump to message from {name}, button' — the timestamp and one-line content preview are excluded from the name. This role is new in this PR; previously the row was a non-interactive `<article>` whose author/timestamp/body were readable as ordinary content, so the PR is what removes that info from AT.
- **Found:** Every `user_message` row is rendered with `author={task.created_by}` (line 169), so `name` — and thus the entire aria-label — is identical for all clickable rows. Deterministic for any task with ≥2 user messages.
- **Impact:** Screen-reader and voice-control users get a list of buttons with byte-for-byte identical accessible names and cannot target a specific message, and lose the per-row timestamp/content that sighted users see — the jump-to-message affordance this PR adds is effectively unusable via assistive tech. Concrete trigger + concrete consequence, non-speculative, not style/taste, not handled elsewhere in the label. Meets the keep bar; the codebase clearly invests in a11y (this PR adds multiple aria-labels), and the suggested per-row detail / `aria-labelledby` fix is actionable.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Include something that varies per row in the accessible name — e.g. append a formatted time (`Jump to message from ${name} at ${time}`) or a short snippet of the clamped content — or better, point `aria-labelledby` at the row's existing author + timestamp + body elements (giving each row real ids) so the accessible name mirrors what's actually shown instead of a synthesized, invariant template string.
</potential_solution>
…ts contents
ReviewHog caught a real a11y regression in this PR. The row's `aria-label` was
`Jump to message from ${name}`, and an explicit label on a widget role replaces
the name that would otherwise be composed from the row's children — so a screen
reader announced neither the timestamp nor the content preview a sighted user
sees. Worse, every conversation row is attributed to the task's creator, so that
label was byte-identical on every row and gave no way to tell them apart.
Dropping the label is the fix: `role="button"` takes its name from its contents,
so each row now announces its own author, time and preview. The avatar gutter is
marked `aria-hidden` so the initials don't double up on the name written beside
them.
Adds `ActivityTimeline.test.tsx`, which had no coverage before: it asserts the
computed accessible names are distinct and carry each row's content (the old label
fails it), that clicking files a scroll request for that item, and that rows stay
inert without a transcript alongside.
Generated-By: PostHog Code
Task-Id: 3da5213f-90f2-4123-bbbd-c648c7b08e06
|
I actually don't like the timestamp on the right, this makes it a one-off design |
@adamleithp I hear you. Do you prefer close to the actor? since it's a timeline i went with the timestamp on the right, but i don't have strong opinion about it, can still change before the merge |
…ass tests
Per Shy and Adam: the timestamp stays at 11px but sits next to the actor again
rather than being pushed to the row's right edge. `ml-auto`/`pl-2` come off; the
header's own gap handles the spacing, which is where the timestamp sat before this
PR touched it. Everything else about the row stays as it is.
Also deletes three tests of mine that were asserting Tailwind classes rather than
behaviour — including an `expect(time).not.toHaveClass("ml-auto")`, which pinned
the absence of an arbitrary class and so had no failure mode matching a real bug.
jsdom computes no layout, so a clamp or an alignment simply cannot be verified
there; those assertions only restated the source, and would have failed the moment
someone renamed a utility without changing what a user sees.
What's left in `ThreadMessageRow` is the one behavioural fact worth holding: the
full message text renders, so no future change quietly reintroduces slicing the
string. Presentation is left to review and to the visual-regression suite.
Generated-By: PostHog Code
Task-Id: 3da5213f-90f2-4123-bbbd-c648c7b08e06
|
/trunk merge |
Both sides added an import at the same spot in ChatThread.tsx: keep the branch's useSessionViewActions (collapse-mode overrides) alongside main's useThreadScrollRequest (activity-pane scroll bridge, #3909).
Problem
The activity pane's rows didn't line up with each other:
quill's
ThreadItemAuthor/ThreadItemBodydefault to 14px while the rest ofthe pane is 13px — so a person's name read as a heading over their own message.
greyed-out lifecycle rows, timestamps at a third size.
left the app for GitHub, the Artifacts row always stayed in it, and the card sat
jammed against its title.
Changes
Two row species, and no more:
line, in the same size and colour as every other row. "Task created" gets its
own icon instead of the creator's avatar, so it matches "Task completed" rather
than impersonating an authored row.
content below with room to breathe.
Plus:
sm) and one text size (13px), with timestamps a step down at11px, sitting beside the actor. The timeline's vertical line moves to 1.75rem, which is just the gutter
centre, so the old magic number derived from the large avatar's width is gone.
messages in full — that's where you read them.
threadNavigationStorebrokers the request, since the pane is a sibling of thetranscript — the same shape
reviewNavigationStorealready uses forscroll-to-file. Both transcripts are wired, as which one renders depends on the
useNewChatThreadsetting.trailing button opens GitHub. The timeline card and the Artifacts row share it.
a
canOpenInPlaceflag keeps rows inert and sends PRs straight to GitHub, soneither interaction becomes a dead click. This also fixes a pre-existing dead
click on the Artifacts tab's PR row in that surface.
Worth flagging for review: this deletes a panel-level
[data-slot=thread-item-timestamp]override that never matched anything, becausequill's
TooltipTriggeroverwrites the wrapped element'sdata-slotwith its own.Timestamp styling now lives in
ThreadTimestampitself.Not included: automation-authored messages (e.g. the CI re-entry prompt) still
render under your name. That text originates in the Tasks backend and arrives as
an ordinary user message with no marker, so it needs a change there first.
How did you test this?
pnpm --filter @posthog/ui test— 271 files / 2316 tests pass, including newcoverage for the row's accessible name (distinct per row, mirroring what's
shown), click-to-jump filing a scroll request, rows staying inert with no
transcript alongside, both PR open targets and the external fallback, and the
scroll-request store. Presentation (clamp, sizes, alignment) is deliberately not
unit-tested — jsdom computes no layout, so such assertions only restate the
source; the visual-regression suite covers it.
pnpm typecheckacross the workspace;biome check packages/ui/srcclean.pane's real width (attached to the task), not from the running app — this
sandbox has no display, so the layout and the scroll-to-message jump have not
been exercised in Electron.
Automatic notifications
Created with PostHog Code