From 492555aaf9cf3686488493cd3a72875d6d9cb8ab Mon Sep 17 00:00:00 2001 From: Tonio Date: Fri, 31 Jul 2026 18:43:40 -0700 Subject: [PATCH] design(decisions): flatten decision cards to two task-borrowed types (#10474) The Decisions queue ran five parallel colour/icon vocabularies chosen by source kind, plus a separate severity badge, so two rows needing the same response could look unrelated and none of it matched the task list. Every row now resolves to one of two kinds, each borrowing the task status it corresponds to: blocking renders as `blocked`, review as `in_review`, both through StatusGlyph and the existing --status-task-icon-* tokens. Source kinds keep their own wording; only colour and icon merge. Card anatomy follows the design mock: no left accent rail, rounded cards 16px apart, a "/"-separated meta breadcrumb, a named See more / See less control, and no separately tinted drawer when expanded. Verb order is fixed across both states. Severity moves from chrome to a toolbar filter. Four defects fixed along the way: - blocked rows reported themselves as their own blocker (server-side) - the task key was missing wherever the row's subject IS the task - the task quicklook stuck open, because closing handed focus back to a trigger that opens on focus - the card ring appeared on click, and only on cards with a toggle Also: the standard task preview is aligned to its trigger's text and scales out of it, the task eyebrow renders its project as a tile, and the first motion tokens land alongside the disclosure and crossfade. Supersedes #9574 and #9575. Co-Authored-By: Claude Opus 5 --- .claude/launch.json | 12 + doc/design/DECISION-SHEET.md | 123 ++++ .../src/__tests__/attention-service.test.ts | 73 +++ server/src/services/attention.ts | 39 +- ui/src/components/AttentionQueueRow.test.tsx | 141 ++++- ui/src/components/AttentionQueueRow.tsx | 586 ++++++++++-------- ui/src/components/IssueLinkQuicklook.test.tsx | 167 ++++- ui/src/components/IssueLinkQuicklook.tsx | 199 +++++- ui/src/components/IssuesQuicklook.tsx | 6 +- ui/src/index.css | 123 ++++ ui/src/lib/attention.test.ts | 209 +++++-- ui/src/lib/attention.ts | 170 +++-- ui/src/pages/IssueDetail.tsx | 19 +- ui/src/pages/WhatNeedsMe.tsx | 17 +- 14 files changed, 1408 insertions(+), 476 deletions(-) create mode 100644 .claude/launch.json diff --git a/.claude/launch.json b/.claude/launch.json new file mode 100644 index 0000000000..7cbc05c40a --- /dev/null +++ b/.claude/launch.json @@ -0,0 +1,12 @@ +{ + "version": "0.0.1", + "configurations": [ + { + "name": "paperclip", + "runtimeExecutable": "/bin/sh", + "runtimeArgs": ["-c", "TMPDIR=/tmp pnpm dev"], + "port": 3108, + "autoPort": false + } + ] +} diff --git a/doc/design/DECISION-SHEET.md b/doc/design/DECISION-SHEET.md index 22ea4623ec..9203ba9ca1 100644 --- a/doc/design/DECISION-SHEET.md +++ b/doc/design/DECISION-SHEET.md @@ -99,3 +99,126 @@ Executed on `design/component-convergence` (worktree focused-agnesi). Scope C2/C ## Tune session — CLOSED (Jul 6, 2026) User approved the complete new design language via gallery v4 + live test drive on the :3300 worktree instance ("ship it"). Merged origin/master (12 commits; one conflict — upstream deliberately removed the Wakes-on-confirm chip, deletion accepted). 296 snapshots re-baselined; gates 3/3 CLEAN; typecheck green; final suite verification run against the new baseline. Remaining roadmap: Run 3 (cards/pills/C11 sidebar + investigations + AgentDetail story), issue→task rename run, Run 4 (palette classes + toast), ESLint ratchet. + +## Decision cards flattened to two types (design session, Jul 29 2026) + +**Five colour/icon vocabularies collapse to two, borrowed from the task status system.** User feedback on `/decisions`: the card types carried "several visual and categorizing inconsistencies". Figma reference — current `1148-1253`, proposed `1148-2169` (PCLP-Core). Every row now resolves to `blocking` (failed run, agent error, blocked dependency, recovery, budget) or `review` (approval, confirmation, review, join request), and each borrows a task status rather than declaring its own palette: blocking → `blocked` (red `CircleMinus`), review → `in_review` (violet `CircleDot`), rendered through `` off `--status-task-icon-*`. `attentionTone`/`attentionToneStyle`/`TONE_STYLE` and the per-source `SourceMeta.icon` are deleted; source kinds keep their own *wording* only. Zero new tokens — the point is that the queue and the task list now share one vocabulary by construction (principle 5). + +Also in the same change, per the proposed mock: the 4px left accent rail is gone (colour lives in the glyph); rows became `rounded-xl` cards spaced 16px apart; issue key + project moved up into a `/`-separated meta breadcrumb; the expand affordance became a bottom-left "See more"/"See less" button; and the expanded state lost its separately tinted/bordered drawer — note, gallery and resolver now flow in the card's own column. + +Three deliberate deviations from the mock, each flagged to the user: + +- **The card border stays.** The mock drops it, which reads correctly in dark mode (`--card` 0.205 on `--background` 0.145) but is fatal in light mode, where both tokens are `oklch(1 0 0)` — a borderless card would be invisible white-on-white. Kept `border border-border`; in dark mode it is `oklch(1 0 0 / 10%)` and barely perceptible, so the intended look survives. +- **Radius is `rounded-xl` (11.2px), not the mock's literal 12px.** 12 is off the multiplicative ladder codified Jul 8; minting a one-off token for a 0.8px delta would reopen B4 for nothing. +- **`IssueThreadInteractionCard` keeps its action bar internally.** The mock hoists those buttons out into the card footer beside "See less". That component is shared with the issue-thread surface, so hoisting would silently restyle the chat thread too — out of scope for a decisions-card change, and a separate call. + +**Severity is no longer chrome.** The Critical/High badge (`severityBadge`, deleted) was a third colour vocabulary competing with the type colour — an orange HIGH chip next to a red error icon was exactly the reported inconsistency. Severity survives as a filter/group dimension in the toolbar, so nothing is lost, only relocated. `severityStyle` is left in place (dead but pre-existing; not this change's scope). + +**Verb order is now fixed across states.** Collapsed and expanded rows both order verbs outline → destructive → affirmative, right-aligned, so the affirmative button sits in the same place whether or not a row is expanded (previously collapsed rows ran Approve/Reject/Request revision left-to-right and expanded rows ran Approve/Request revision/Reject, left-aligned). Per-row training moved from a header icon button into the row's overflow menu, matching the mock's header (recency + overflow only); the inline "Trained ✓" badge stays and remains the tested `onTrain` path, since Radix menu items are portal-mounted and this repo does not open them in jsdom. + +## Decision-card follow-ups: task keys, self-blocking, stuck quicklook (design session, Jul 29 2026) + +Three defects surfaced by the flattened cards, each fixed at its own layer. + +**Task key missing on the rows most obviously about a task.** The meta breadcrumb read only `relatedIssue`, but the feed stores the task in two shapes: when the subject IS the task (review, blocked dependency) the identifier sits on `subject` and `relatedIssue` is null; when the subject hangs off a task (thread interaction) the task arrives as `relatedIssue`. `attentionTaskRef` (ui/src/lib/attention.ts) resolves both with one rule, preferring `relatedIssue` when both exist — it is the record the subject alone cannot describe — and returning null for rows genuinely unattached (hire approval, agent error) so they stay blank rather than borrowing a key. 1 of 17 seeded rows → 13. Still open, needs a server change: an approval can carry `subject.metadata.issueId` while `relatedIssue` is null, which reaches the client as a bare UUID with no key or href. + +**Every blocked row claimed it was blocked by itself.** Both `blocker_attention` call sites in server/src/services/attention.ts fell back to the blocked task's own identity when no `blocks` relation was loaded — one hardcoded `{ id: issue.id, identifier: issue.identifier }` outright — so the UI rendered "PAP-23 — Blocked by PAP-23" for all eleven seeded rows. `resolveBlockingIssue` prefers the loaded relation, then a blockerAttention sample identifier, then null (the row falls back to its `whyNow` line, which is honest about not knowing). It also rejects a self-referential relation row as corrupt. The dedup key deliberately keeps its original fallback chain including the issue's own identifier: it is the identity dismissals are recorded against, and narrowing it would resurrect dismissed rows. + +**Quicklook stuck open after expanding a row.** Reported as "the hover task card gets stuck and keeps displaying even when I hover off". Root cause is a self-sustaining loop in the shared `IssueLinkQuicklook`, not in the decision card: Radix returns focus to the trigger when a popover closes, and that link opens the quicklook `onFocus` — so every dismissal refocused the trigger, which reopened the card. Fixed by declining the focus hand-back (`onCloseAutoFocus` prevented, symmetric with the existing `onOpenAutoFocus`): a preview must not move focus in either direction. Added alongside it, a pointer-escape guard that closes on any pointer move clear of both boxes, since the only other close paths were `mouseleave` on trigger/content and no leave fires when the layout shifts an element out from under a stationary pointer. The guard needs both `:hover` and geometry to agree the pointer is gone before closing, so a resting pointer is never dropped, and exempts focus-opened quicklooks for keyboard users. + +Separately, evidence thumbnails in an expanded card no longer carry a task quicklook at all (`disableIssueQuicklook`): `Link` upgrades any /issues/ href into a hover preview, which here popped a text card over the very screenshot being examined, and expanding a row mounts that gallery directly under the pointer. + +## Card-level selection ring is keyboard-only (design session, Jul 29 2026) + +User: "it seems weird that only cards with see more/less have a focus state and not the rest… disable focus state for decision cards but retain the focus state for each interactive component (within cards) for accessibility purposes." + +The card-wide stroke was never a focus state — it is the **keyboard cursor**, marking the row that j/k, e, x and s act on. It leaked into mouse use because `handleToggleExpand` set the selection as a side effect of a click, and only expandable rows have a See more/less toggle to click. Hence the reported inconsistency: clicking one kind of card ringed it, and no other card could ever be ringed. + +Fixed by tracking how the selection was made and drawing the ring only for a keyboard-driven one. Clicking still sets the selection, so keyboard actions continue to target the row you just used — it simply draws nothing. + +**The ring is deliberately kept for j/k navigation** rather than removed outright, which the literal request would imply. Those keys dismiss and snooze the selected row; with no indicator an operator would be firing destructive actions at an invisible target. Flagged to the user as the one place the card-level state survives, and it is theirs to remove if they want it gone there too. + +Focus states on everything inside a card are untouched: the See more/less toggle, decision verbs, the task key, the project link, evidence thumbnails and the row menu all keep their `focus-visible` rings. + +## Decision card eyebrow: project dropped, "·" separator (design session, Jul 29 2026) + +Per the proposed mock, the decision card eyebrow is now **decision kind · task key** and nothing else. + +**Project identity left the card.** It cost the eyebrow's width on every row to repeat a fact the operator has usually just chosen — the queue filters and groups by project from the toolbar — and it competed with the task key, which is the identifier an operator actually navigates by. The project is still one click away on the task itself. + +**The separator changed from "/" to "·".** The eyebrow started as a breadcrumb (kind / key / project), but with the project gone it is a flat list of two facts, not a hierarchy. A slash implies containment those two segments do not have; a middle dot just separates. `ProjectMeta` and its `ProjectTile` import were deleted from the row rather than left unused. + +## Standard task preview card (design session, Jul 29 2026) + +**`IssueQuicklookCard` restructured to the proposed mock, and this is now the app-wide standard** — every hover preview of a task renders it, so the same three rows appear in the same order everywhere: + +1. meta — status glyph · task key [· project] …………… last activity +2. title +3. summary — first lines of the description + +The meta row splits: identity left, recency pinned right. Identity leads because a preview answers "which task is this?", and a title alone does not. The status glyph switches from `StatusIcon` to `StatusGlyph`, so a preview speaks the same status vocabulary as the flattened decision cards and the task list. + +**Status carries no word of its own.** An earlier revision of this card gave status a line under the title ("In review · 1d ago", in `foreground`); the final mock removes it and moves the timestamp up into the meta row, leaving the glyph to be the status — which is what the glyph already is on task rows and decision cards. That leaves shape and colour as the only visual signal, so the glyph is passed a `title`, rendering as `role="img"` with the status as its accessible name. The status stays available to a screen reader without spending a line, and a test pins that (the glyph must carry it and the visible text must not). + +Three shapes the meta row holds, all specified by the mock: + +1. **no project** — glyph, key, timestamp hard right; no separator is rendered +2. **project** — a "·", the tile and the name join the left group +3. **truncation** — a long project name ellipsizes; the key and the timestamp are `shrink-0`, so the two facts that identify the task survive at any width. Verified live: with a 47-character project name the key and timestamp hold their exact widths (39.7px / 36.1px) and only the name clips. + +Two judgment calls: + +- **The project tile is untinted.** `ProjectTile` supports a colour, and the decision card's old chip used it, but a preview is a quiet surface and the project colour would be the loudest thing on it. The mock shows a neutral tile, and `IssueAncestorProject` carries neither colour nor icon — so following the mock costs nothing and needs no new data. If the tile should ever tint, that is a server-side field addition first. +- **11px via `--text-micro`** for the meta row, matching the mock, rather than minting a token for the mock's literal values. + +The other consumer, `IssuesQuicklook` (project workspace linked issues), inherits the new card automatically — which is the point of standardising it. + +## Quicklook aligns to the trigger's text, not its box (design session, Jul 29 2026) + +Radix aligns box to box, so `align="start"` put the preview's *left edge* on the trigger's left edge — leaving the card's text pushed right by the card's own border and padding, and visibly out of line with the task key that opened it. + +`quicklookAlignOffset()` cancels that inset: **13px** — `p-3` (12px) plus the 1px border `PopoverContent` draws. Measured on the live card afterwards, the trigger's text sits at 353.61px and the card's glyph, title and description all sit at 353.50px — a 0.11px residual from the trigger's own sub-pixel position, i.e. aligned. + +The offset follows the align prop (`start` negative, `end` the mirror, `center` zero) rather than being hardcoded to one direction, and both surfaces that render the standard card — `IssueLinkQuicklook` and `IssuesQuicklook` — now share `QUICKLOOK_CONTENT_CLASS` and this helper, so the preview is positioned identically wherever it opens. + +The 13px is a derived constant with the border and padding written out as `12 + 1`, and a test asserts the shell still carries `p-3`, so the two cannot drift apart silently. + +## First motion tokens, and the inert animate-in finding (design session, Jul 29 2026) + +**Motion tokens minted.** Durations and easings were previously written inline at each call site in `index.css`. Four named values now exist, and both new animations consume them: + +- `--motion-duration-enter: 160ms` / `--motion-duration-exit: 110ms` — exit is deliberately shorter: a thing appearing wants to be followed, a thing leaving just needs to get out of the way. +- `--motion-ease-out: cubic-bezier(0.16, 1, 0.3, 1)` — the curve the dialog max-width transition already used, promoted to the system. It decelerates hard at the end, which is what reads as "snappy" rather than "slow start". +- `--motion-ease-in: cubic-bezier(0.4, 0, 1, 1)` for exits. + +**Decision-card disclosure.** See more / See less now animates height through Radix `Collapsible`, which measures the panel and publishes `--radix-collapsible-content-height`, so the card grows and shrinks to a real number instead of snapping. Measured on the live card: 0 → 65 → 98 → 114 → 121 → 125 → 128px over ~136ms. The Root carries `contents` so a collapsed row pays no flex gap for an empty wrapper, and Radix keeps the panel an empty `hidden` shell when closed — verified all 17 closed panels hold zero children, so no row runs a resolver behind a collapsed card. + +**`animate-in` is dead CSS in this repo.** Chasing the quicklook's scale turned up that the shadcn `PopoverContent` class string (`animate-in`, `zoom-in-95`, `fade-in-0`, `slide-in-from-*`) resolves to nothing: those utilities ship with the `tailwindcss-animate` plugin, which is not a dependency and is not imported in `index.css`. A stylesheet scan found no `enter`/`exit` keyframes in the build. **This affects every shadcn surface in the app** — dialogs, dropdowns, tooltips, sheets all carry the same inert classes and have never animated. + +Rather than add the plugin for one surface — which would newly animate every one of those surfaces at once, an app-wide visual change nobody has reviewed — the quicklook defines its own `quicklook-open` / `quicklook-close` keyframes. Adopting the plugin properly is worth its own run. + +**Quicklook motion.** A shallow scale (0.96 → 1) plus opacity, anchored to `--radix-popover-content-transform-origin` so the card grows out of the task key that opened it rather than swelling in place. Verified live: `quicklook-open`, running, 160ms, ease-out, origin `0px 0px`. + +Both animations are disabled under `prefers-reduced-motion: reduce`. + +## Task eyebrow project reads as tile + name (design session, Jul 29 2026) + +The task detail eyebrow showed a bare `Hexagon` outline glyph next to the project name — a shape used nowhere else for projects. It now renders `ProjectTile` at `xs`, matching the sidebar and Projects list. + +Measured against the mock, every value matches and all of it resolves through tokens: 16×16 tile, 4.8px radius, `bg-muted` (`oklch(0.269 0 0)`, the mock's `#313131`), 10px folder icon, 4px gap, 2px/4px padding, 4px link radius, 12px `text-muted-foreground` (the mock's `#a1a1a1`). + +**The tile stays neutral rather than taking the project colour**, which `ProjectTile` would do if passed one. The eyebrow already carries the status glyph's colour, and a second tinted swatch beside it competes with the one mark that means something. Project colour still identifies the project on project-native surfaces. This matches the direction #9574 took for the Decisions feed. + +The seeded header (rendered from `headerSeed` while the issue loads) was updated in lockstep, so the eyebrow does not change shape when the real issue arrives. + +## Collapsed-only content crossfades against the panel (design session, Jul 29 2026) + +Adding the disclosure animation left a seam: the panel grew smoothly, but the content it *replaces* still popped out of existence in one frame. Two things never carry across the two states — the thumbnail strip, whose counterpart is the full gallery; and an inline row's footer, which the resolver takes over once expanded. + +Both now ride an **inverse disclosure** (`open={!expanded}`) using the same keyframes and tokens as the panel, so the collapsed cluster shrinks and fades out while the panel grows and fades in. Verified live during a toggle: `decision-disclosure-close` running at 110ms on the cluster and `decision-disclosure-open` running at 160ms on the panel, in the same frame. + +Exit being shorter than enter is what makes it read as a handoff rather than a blend — the outgoing content clears slightly ahead of the incoming. + +**Only genuine swaps crossfade.** A non-inline row keeps one standing footer: its Open or Restore button and its toggle are the same control in both states, so it stays put rather than crossfading with itself. The inverse cluster is skipped entirely when a row has neither images nor an inline resolver (`hasCollapsedOnlyContent`), because an always-open empty wrapper would otherwise charge the card a 16px flex gap for nothing — the closed panel avoids this for free, since Radix marks it `hidden` and it drops out of flex layout. + +`renderFooter({ compact })` renders the bar in either position. `compact` is false for the standing copy, so an expanded non-inline row does not show collapsed verbs beside the panel's own. diff --git a/server/src/__tests__/attention-service.test.ts b/server/src/__tests__/attention-service.test.ts index ed4ed8eff5..f98ac71298 100644 --- a/server/src/__tests__/attention-service.test.ts +++ b/server/src/__tests__/attention-service.test.ts @@ -927,6 +927,79 @@ describeEmbeddedPostgres("attention service", () => { expect(feed.items.some((item) => item.dedupKey === `blocker:${issueId}:ATP-1`)).toBe(true); }); + // Regression: both blocker_attention call sites fell back to the blocked + // task's own identity when no `blocks` relation was loaded, so every such row + // claimed the task was blocked by itself ("PAP-23 — Blocked by PAP-23"). + it("reports no blocking task rather than a self-reference when the blocker is unknown", async () => { + const { companyId } = await seedCompany("ATV"); + const issueId = await insertIssue({ + companyId, + identifier: "ATV-1", + title: "Blocked with no relation", + status: "blocked", + blockedTransitionAt: new Date(ROUTABLE_BLOCKED_ROLLOUT_AT.getTime() - 1), + }); + + const feed = await attentionService(db).list(companyId, { userId: "board-user" }); + const row = feed.items.find((item) => item.dedupKey === `blocker:${issueId}:ATV-1`); + + expect(row).toBeTruthy(); + expect(row?.detail).toMatchObject({ kind: "blocker", blockingIssue: null }); + // The dedup key keeps its original fallback so existing dismissals survive. + expect(row?.dismissalKey).toBe(`attention:blocker:${issueId}:ATV-1`); + }); + + it("names the real blocking task when a blocks relation exists", async () => { + const { companyId } = await seedCompany("ATW"); + const blockedId = await insertIssue({ + companyId, + identifier: "ATW-1", + title: "Blocked parent", + status: "blocked", + blockedTransitionAt: new Date(ROUTABLE_BLOCKED_ROLLOUT_AT.getTime() - 1), + }); + const blockerId = await insertIssue({ + companyId, + identifier: "ATW-2", + title: "The actual blocker", + status: "in_progress", + }); + await db.insert(issueRelations).values({ + companyId, + issueId: blockerId, + relatedIssueId: blockedId, + type: "blocks", + }); + + const feed = await attentionService(db).list(companyId, { userId: "board-user" }); + const row = feed.items.find((item) => item.sourceKind === "blocker_attention" && item.subject.id === blockedId); + + expect(row?.detail).toMatchObject({ + kind: "blocker", + blockingIssue: { identifier: "ATW-2", title: "The actual blocker" }, + }); + }); + + it("does not name the blocked task as its own blocker on a human-owned unblock row", async () => { + const { companyId } = await seedCompany("ATX"); + const transitionAt = new Date("2026-07-23T18:30:00.000Z"); + const issueId = await insertIssue({ + companyId, + identifier: "ATX-1", + title: "Needs board action", + status: "blocked", + unblockDescriptor: { owner: "board", action: "Approve the exception" }, + blockedTransitionAt: transitionAt, + }); + + const feed = await attentionService(db).list(companyId, { userId: "board-user" }); + const row = feed.items.find( + (item) => item.dedupKey === `blocked-owner:${issueId}:${transitionAt.toISOString()}`, + ); + + expect(row?.detail).toMatchObject({ kind: "blocker", blockingIssue: null }); + }); + it("does not route pre-rollout human unblock descriptors", async () => { const { companyId } = await seedCompany("ATQ"); const transitionAt = new Date(ROUTABLE_BLOCKED_ROLLOUT_AT.getTime() - 1); diff --git a/server/src/services/attention.ts b/server/src/services/attention.ts index f70c63d50c..6d62182983 100644 --- a/server/src/services/attention.ts +++ b/server/src/services/attention.ts @@ -576,6 +576,31 @@ async function blockingIssueMap(db: Db, companyId: string, blockedIssueIds: Arra return map; } +/** + * The task that blocks `issue` — never `issue` itself. + * + * Both blocker_attention call sites used to fall back to the blocked task's own + * identity when no `blocks` relation was loaded, so every such row reported + * "PAP-23 — Blocked by PAP-23". The UI renders that as a real dependency, which + * tells an operator nothing and reads as a bug. + * + * Order: the loaded `blocks` relation, then an identifier sampled by + * blockerAttention, and otherwise nothing — a null lets the row fall back to + * its `whyNow` line, which is honest about not knowing the blocker. + */ +function resolveBlockingIssue( + issue: { id: string; identifier: string | null }, + fromRelation: BlockingIssueSummary | undefined, + sampledIdentifier?: string | null, +): BlockingIssueSummary | null { + // A self-referential relation row would be corrupt data; treat it as unknown. + if (fromRelation && fromRelation.id !== issue.id) return fromRelation; + if (sampledIdentifier && sampledIdentifier !== issue.identifier && sampledIdentifier !== issue.id) { + return { id: null, identifier: sampledIdentifier, title: null }; + } + return null; +} + function readRunIssueId(contextSnapshot: Record | null) { const issueId = contextSnapshot?.issueId ?? contextSnapshot?.taskId; return typeof issueId === "string" && issueId.length > 0 ? issueId : null; @@ -948,15 +973,23 @@ export function attentionService(db: Db) { updatedAt: toIso(issue.updatedAt), relatedIssue: null, ...issueContext(issueSummary), - detail: { kind: "blocker", blockingIssue: { id: issue.id, identifier: issue.identifier, title: issue.title }, images: issueImages(blockedImageMap, issue.id) }, + detail: { + kind: "blocker", + blockingIssue: resolveBlockingIssue(issue, blockingIssues.get(issue.id)), + images: issueImages(blockedImageMap, issue.id), + }, })); } const blockerAttention = issue.blockerAttention; if (blockerAttention?.state !== "stalled" && blockerAttention?.state !== "needs_attention") continue; const issueSummary = blockedIssueSummaries.get(issue.id) ?? null; const summarizedIssue = issueSummary ?? issue; - const sample = blockerAttention.sampleStalledBlockerIdentifier ?? blockerAttention.sampleBlockerIdentifier ?? issue.identifier ?? issue.id; - const blockingIssue = blockingIssues.get(issue.id) ?? { id: null, identifier: sample, title: null }; + const sampledBlocker = blockerAttention.sampleStalledBlockerIdentifier ?? blockerAttention.sampleBlockerIdentifier; + const blockingIssue = resolveBlockingIssue(issue, blockingIssues.get(issue.id), sampledBlocker); + // The dedup key keeps its original fallback chain (including the issue's + // own identifier) on purpose: it is the stable identity a dismissal is + // recorded against, so narrowing it would resurrect dismissed rows. + const sample = sampledBlocker ?? issue.identifier ?? issue.id; const dedupKey = `blocker:${issue.id}:${sample}`; add(createItem({ companyId, diff --git a/ui/src/components/AttentionQueueRow.test.tsx b/ui/src/components/AttentionQueueRow.test.tsx index 8f193860ce..37b4c9b986 100644 --- a/ui/src/components/AttentionQueueRow.test.tsx +++ b/ui/src/components/AttentionQueueRow.test.tsx @@ -238,7 +238,11 @@ describe("AttentionQueueRow", () => { expect(links.some((a) => a.textContent?.includes("Hire agent: Research Analyst"))).toBe(false); }); - it("renders project identity once without a filter button", () => { + // The eyebrow carries the decision kind and the task key only. Project + // identity was removed from the card: the queue is filtered and grouped by + // project from the toolbar, so repeating it on every row spent the eyebrow's + // width on a fact the operator had usually just chosen. + it("keeps project identity off the card", () => { render( { />, ); - const projectMeta = container?.querySelector('[data-testid="attention-project-meta"]'); - expect(projectMeta?.textContent).toBe("Alpha"); - expect(projectMeta?.querySelector("button")).toBeNull(); - expect(projectMeta?.getAttribute("class")).not.toContain("border"); - expect(projectMeta?.getAttribute("class")).not.toContain("bg-"); - expect(container?.querySelector('button[title="Filter by Alpha"]')).toBeNull(); - expect(container?.textContent?.match(/Alpha/g)).toHaveLength(1); + expect(container?.querySelector('[data-testid="attention-project-meta"]')).toBeNull(); + expect(container?.textContent).not.toContain("Alpha"); + }); + + it("separates eyebrow facts with a middle dot, not a slash", () => { + render( + , + ); + + const eyebrow = container?.querySelector('[data-attention-row] > div'); + expect(eyebrow?.textContent).toContain("·"); + expect(eyebrow?.textContent).not.toContain("/"); + }); + + // Regression: the meta breadcrumb used to read only `relatedIssue`, so rows + // whose subject IS the task (reviews, blocked dependencies) showed no key at + // all — the rows most obviously about a task were the ones missing it. + it("shows the task key when the subject is the task itself", () => { + render( + , + ); + + const link = Array.from(container?.querySelectorAll("a") ?? []).find((a) => a.textContent === "PAP-23"); + expect(link).toBeTruthy(); + expect(link?.getAttribute("href")).toBe("/PAP/issues/PAP-23"); + }); + + it("shows no task key on a row that is not attached to a task", () => { + render( + , + ); + expect(container?.textContent).not.toMatch(/PAP-\d+/); }); it("places the timestamp beside the row menu without a clock icon", () => { @@ -277,7 +348,10 @@ describe("AttentionQueueRow", () => { expect(container?.querySelector("svg.lucide-clock")).toBeNull(); }); - it("uses square row edges and can show a keyboard selection ring", () => { + // Rows became rounded cards when the decision types were flattened: with the + // left accent rail gone, the card's own shape carries the separation that the + // rail used to, so square edges no longer read as deliberate. + it("uses rounded card edges and can show a keyboard selection ring", () => { render( { ); const row = container?.querySelector("[data-attention-row]"); - expect(row?.getAttribute("class")).not.toContain("rounded"); + expect(row?.getAttribute("class")).toContain("rounded-xl"); expect(row?.getAttribute("class")).toContain("ring-ring"); }); @@ -318,11 +392,12 @@ describe("AttentionQueueRow", () => { expect(decisionActions?.textContent).toContain("Approve"); expect(decisionActions?.textContent).toContain("Reject"); - // The action bar is its own full-width band (mobile-first) that collapses to - // a right-aligned pill row once the row's container is wide (container query) - // — no longer a stretched right column. + // The footer splits the row's last line: disclosure on the left, decision + // verbs on the right, so the affirmative verb sits in the same place in + // every row whether it is collapsed or expanded. const actionArea = decisionActions?.closest('[data-attention-actions="true"]'); - expect(actionArea?.getAttribute("class")).toContain("@xl:justify-end"); + expect(actionArea?.getAttribute("class")).toContain("justify-between"); + expect(decisionActions?.parentElement?.getAttribute("class")).toContain("@xl:justify-end"); const rowMenu = container?.querySelector('[aria-label="Row actions"]'); expect(rowMenu?.closest('[data-attention-menu="true"]')).toBeTruthy(); @@ -442,7 +517,10 @@ describe("AttentionQueueRow", () => { expect(issuesApi.rejectInteraction).not.toHaveBeenCalled(); }); - it("renders evidence thumbnails in a centered context row below the text stack", () => { + // The old context row bundled project identity + thumbnails together; project + // identity has since moved up into the meta breadcrumb, so evidence is now a + // block of the row's own column rather than a shared strip. + it("renders evidence thumbnails as their own block below the text stack", () => { render( { const thumbnailStack = image?.parentElement?.parentElement; expect(thumbnailStack?.getAttribute("class")).toContain("items-center"); - expect(thumbnailStack?.parentElement?.getAttribute("class")).toContain("items-center"); + // The strip is collapsed-only content: it rides the inverse disclosure so + // it can fade out as the expanded gallery fades in, rather than popping. + const cluster = thumbnailStack?.closest("[data-decision-disclosure]"); + expect(cluster).toBeTruthy(); + expect(cluster?.getAttribute("data-state")).toBe("open"); + expect(cluster?.closest("[data-attention-row]")).toBeTruthy(); }); it("is memoized — a parent re-render with identical props does not re-render the row", async () => { @@ -636,8 +719,12 @@ describe("AttentionQueueRow", () => { }); } - it("shows an untrained train button and fires onTrain when clicked", () => { - const onTrain = vi.fn(); + // Training moved off the header strip into the row's overflow menu, so the + // header carries only recency + overflow. Menu items live in a portal that + // only mounts once opened — environment-flaky in jsdom (see the dismiss test + // above) — so the untrained path asserts the menu exists and no badge is + // shown, and the onTrain contract is exercised through the inline badge. + it("offers training through the row menu and shows no badge until trained", () => { render( { expanded={false} onToggleExpand={noop} onDismiss={noop} - onTrain={onTrain} + onTrain={noop} />, ); - const button = container?.querySelector('[data-testid="attention-train-button"]'); - expect(button).toBeTruthy(); - expect(button?.getAttribute("data-training-state")).toBe("untrained"); + expect(container?.querySelector('[aria-label="Row actions"]')).toBeTruthy(); expect(container?.querySelector('[data-testid="attention-trained-badge"]')).toBeNull(); - act(() => button?.dispatchEvent(new MouseEvent("click", { bubbles: true }))); - expect(onTrain).toHaveBeenCalledWith(expect.objectContaining({ id: "a1" })); }); - it("renders a Trained ✓ badge and a filled button once trained", () => { + it("renders a Trained ✓ badge once trained and fires onTrain when it is clicked", () => { + const onTrain = vi.fn(); render( { expanded={false} onToggleExpand={noop} onDismiss={noop} - onTrain={noop} + onTrain={onTrain} />, ); - expect( - container?.querySelector('[data-testid="attention-train-button"]')?.getAttribute("data-training-state"), - ).toBe("trained"); const badge = container?.querySelector('[data-testid="attention-trained-badge"]'); expect(badge?.textContent).toContain("Trained"); + act(() => badge?.dispatchEvent(new MouseEvent("click", { bubbles: true }))); + expect(onTrain).toHaveBeenCalledWith(expect.objectContaining({ id: "a1" })); }); it("does not offer training on a decision that isn't anchored to an issue", () => { diff --git a/ui/src/components/AttentionQueueRow.tsx b/ui/src/components/AttentionQueueRow.tsx index f316ce11fd..c304c1c3ca 100644 --- a/ui/src/components/AttentionQueueRow.tsx +++ b/ui/src/components/AttentionQueueRow.tsx @@ -1,9 +1,9 @@ -import { memo, useState, type KeyboardEvent } from "react"; +import { memo, useState, type KeyboardEvent, type ReactNode } from "react"; import { useMutation, useQueryClient } from "@tanstack/react-query"; import { AlarmClock, ChevronDown, - ChevronRight, + ChevronUp, ExternalLink, GraduationCap, Loader2, @@ -22,14 +22,16 @@ import { attentionDetailImages, attentionDetailLine, attentionImageUrl, - attentionToneStyle, + attentionStatus, + attentionTaskRef, isInlineResolvable, - severityBadge, sourceMeta, } from "../lib/attention"; import { isTrainable } from "../lib/decisionTraining"; import { cn, relativeTime } from "../lib/utils"; +import { StatusGlyph } from "./StatusGlyph"; import { Button } from "./ui/button"; +import { Collapsible, CollapsibleContent } from "./ui/collapsible"; import { Textarea } from "./ui/textarea"; import { DropdownMenu, @@ -42,7 +44,6 @@ import { DropdownMenuTrigger, } from "./ui/dropdown-menu"; import { AttentionInteractionResolver } from "./AttentionInteractionResolver"; -import { ProjectTile } from "./ProjectTile"; const HOUR_MS = 60 * 60 * 1000; const DAY_MS = 24 * HOUR_MS; @@ -112,9 +113,11 @@ export const AttentionQueueRow = memo(function AttentionQueueRow({ selected = false, }: AttentionQueueRowProps) { const meta = sourceMeta(item.sourceKind); - const tone = attentionToneStyle(item); - const sevBadge = severityBadge(item.severity); - const Icon = meta.icon; + // Colour + glyph are borrowed wholesale from the task status system, so a + // blocking decision reads exactly like a blocked task (DESIGN.md principle 5). + const status = attentionStatus(item); + // The task this row belongs to, whichever field the feed put it in. + const taskRef = attentionTaskRef(item); const isHidden = variant === "hidden"; const inline = !isHidden && isInlineResolvable(item); const href = item.subject.href; @@ -150,18 +153,73 @@ export const AttentionQueueRow = memo(function AttentionQueueRow({ // Which rows contribute an action bar. Inline rows carry compact decision // verbs; deep-link rows carry an Open button; curtain rows carry Restore. const compactActions = !isHidden ? collectCompactActions(item) : []; - const showCompact = !expanded && compactActions.length > 0; const showOpen = !inline && !!href; const showRestore = isHidden && !!onRestore; - const showActionBar = showCompact || showOpen || showRestore; - // Left gutter width (chevron + gap) so the stacked content aligns under the - // headline in the wide layout; when narrow, everything runs full-bleed. - const gutterIndent = "@xl:pl-6"; + // An expanded inline row hands its footer to the resolver, which owns the + // decision verbs — so the toggle rides alongside them on one row rather than + // stranding a lone "See less" under the buttons. That makes the collapsed + // footer a swap rather than a survivor, so it crossfades with the panel. + const hasCollapsedOnlyContent = hasImages || inline; + + // Disclosure control. Now the row's only expand affordance: it names what it + // does instead of leaving a bare chevron to be decoded, and it sits at the + // bottom-left where the eye lands after reading the row. + const toggle = expandable ? ( + + ) : null; + + /** + * The row's action bar: disclosure on the left, decision verbs on the right. + * Rendered either inside the collapsed-only cluster (inline rows, where the + * resolver takes it over once expanded) or as a standing sibling (everything + * else). `compact` is false for the standing copy so an expanded row does not + * show collapsed verbs beside the panel's own. + */ + const renderFooter = ({ compact }: { compact: boolean }) => { + const showCompact = compactActions.length > 0 && (compact || !expanded); + if (!toggle && !showCompact && !showOpen && !showRestore) return null; + return ( +
+ {toggle ?? } + +
+ {showCompact && ( + onToggleExpand(item)} /> + )} + + {showOpen && ( + + )} + + {showRestore && ( + + )} +
+
+ ); + }; return (
- {/* Type accent bar (canonical color map — never severity). */} - + {/* Meta band: one breadcrumb of identity on the left (kind → task → + project), recency + overflow on the right. Not part of the clickable + headline, so the menu never toggles it. */} +
+
+ + + {meta.label} + + {taskRef && ( + <> + + e.stopPropagation()} + > + {taskRef.identifier} + + + )} + {trainable && trained && ( + + )} +
-
- {/* Expand affordance / spacer gutter — keeps headlines aligned across the list. */} - {expandable ? ( - - ) : ( - - )} - - {/* Content column: a single vertical stack that fills the full width on - mobile (no competing right-hand controls) and reads top-to-bottom. */} -
- {/* Meta band: identity on the left, recency + overflow on the right. - Not part of the clickable headline, so the menu never toggles it. */} -
-
- - - {meta.label} - - {sevBadge && ( - - {sevBadge.label} - - )} - {item.relatedIssue?.identifier && ( - e.stopPropagation()} - > - {item.relatedIssue.identifier} - - )} -
- -
- {trainable && ( +
+ {isHidden && snoozedUntil ? ( + + Reappears {reappearLabel(snoozedUntil)} + + ) : ( + {relativeTime(item.activityAt)} + )} + {!isHidden && ( + + - )} - {isHidden && snoozedUntil ? ( - - Reappears {reappearLabel(snoozedUntil)} - - ) : ( - {relativeTime(item.activityAt)} - )} - {!isHidden && ( - - - - - - {onSnooze && onSnooze(item, iso)} />} - onDismiss(item)}> - - Dismiss + + + {/* Training moved off the header strip (which now carries only + recency + overflow) but keeps its testids so the affordance + is still addressable. */} + {trainable && ( + onTrain?.(item)} + > + + {trained ? "View training example" : "Train this decision"} + + )} + {onSnooze && onSnooze(item, iso)} />} + onDismiss(item)}> + + Dismiss + + {href && ( + <> + + + Open source - {href && ( - <> - - - Open source - - - )} - - - )} -
-
- - {/* Headline — the primary expand target for inline rows. Title now wraps - to two lines instead of truncating to a sliver on narrow screens. */} -
- - {item.subject.title ?? meta.label} - -

{detailLine}

-
- - {/* Context row: project identity and evidence thumbnails move below the - text so they never squeeze the headline on mobile. */} - {(item.project || (hasImages && !expanded) || (trainable && trained)) && ( -
- {item.project && } - {trainable && trained && ( - - )} - {hasImages && !expanded && } -
- )} - - {/* Action bar: full-width, thumb-reachable buttons on mobile; - right-aligned dense pills on desktop. Sibling of the headline so - taps never toggle expand. */} - {showActionBar && ( -
- {showCompact && ( - onToggleExpand(item)} - /> - )} - - {showOpen && ( - - )} - - {showRestore && ( - - )} -
+ + )} + + )}
- {expanded && (hasImages || inline) && ( -
- {hasImages && } - {inline && ( - - )} -
+ {/* Headline — the primary expand target for inline rows. Title wraps to + two lines instead of truncating to a sliver on narrow screens. */} +
+ + {item.subject.title ?? meta.label} + +

{detailLine}

+
+ + {/* Collapsed-only content. It has no counterpart to morph into — the + thumbnail strip becomes a full gallery, and an inline row's footer is + replaced by the resolver's own — so it rides an inverse disclosure and + crossfades against the panel below: this shrinks and fades out on the + same tokens as that grows and fades in, instead of popping. */} + {hasCollapsedOnlyContent && ( + + +
+ {hasImages && } + {inline && renderFooter({ compact: true })} +
+ + )} + + {/* The disclosure panel. Collapsible measures the panel and publishes its + height, so the card grows and shrinks to a real number rather than + snapping open. `contents` keeps the Root out of the layout, so a + collapsed row does not pay a flex gap for an empty wrapper — and once + the exit finishes Radix unmounts the panel, so a collapsed row is not + left with a live resolver behind it. */} + onToggleExpand(item)} className="contents"> + +
+ {hasImages && } + {inline && ( + + )} +
+ + + + {/* A non-inline row keeps one footer across both states — its Open or + Restore button and its toggle are the same control either way, so it + stays put rather than crossfading with itself. */} + {!inline && renderFooter({ compact: false })}
); }); +/** + * "·" between eyebrow segments. + * + * The eyebrow is a flat list of two facts (decision kind, task key), not a + * hierarchy, so a middle dot reads more honestly than the "/" this started as — + * a slash implies containment that the two segments do not have. + */ +function EyebrowSeparator() { + return ( + + · + + ); +} + type CompactDecisionAction = "accept" | "approve" | "reject" | "request_revision"; function compactDecisionAction(item: AttentionItem, verbId: string): CompactDecisionAction | null { @@ -415,12 +433,33 @@ function compactDecisionAction(item: AttentionItem, verbId: string): CompactDeci return null; } +/** + * Weight used to order a decision's verbs. The affirmative verb always lands + * rightmost — the same place in every row, collapsed or expanded — so the + * operator's aim never has to move with the verb list. + */ +const VERB_ORDER: Record<"outline" | "destructive" | "default", number> = { + outline: 0, + destructive: 1, + default: 2, +}; + +interface CompactAction { + action: CompactDecisionAction; + label: string; + id: string; + description: string; +} + /** The compact accept/reject verbs a collapsed row can resolve in place. */ -function collectCompactActions(item: AttentionItem): Array<{ action: CompactDecisionAction; label: string; id: string }> { - return item.decisionVerbs.slice(0, 3).flatMap((verb) => { - const action = compactDecisionAction(item, verb.id); - return action ? [{ action, label: verb.label, id: verb.id }] : []; - }); +function collectCompactActions(item: AttentionItem): CompactAction[] { + return item.decisionVerbs + .slice(0, 3) + .flatMap((verb) => { + const action = compactDecisionAction(item, verb.id); + return action ? [{ action, label: verb.label, id: verb.id, description: verb.description ?? "" }] : []; + }) + .sort((a, b) => VERB_ORDER[decisionVerbVariant(a)] - VERB_ORDER[decisionVerbVariant(b)]); } function CompactDecisionActions({ @@ -481,11 +520,11 @@ function CompactDecisionActions({ return (
- {actions.map(({ action, id, label }) => ( + {actions.map(({ action, id, label, description }) => (