diff --git a/docs/plans/2026-09-04-session-workflow-dag-design.md b/docs/plans/2026-09-04-session-workflow-dag-design.md new file mode 100644 index 00000000000..c45d17a11f4 --- /dev/null +++ b/docs/plans/2026-09-04-session-workflow-dag-design.md @@ -0,0 +1,216 @@ +# Session Workflow DAG — design plan + +Status: implemented by this change. Written before code, reviewed against the +brief once, revised. The revision — and the two corrections found during +implementation — are recorded at the bottom rather than silently folded in. + +## Brief + +An execution trace of a dependency-ordered plan, watched live while an agent +works through it. Reader: a developer. The three questions they arrive with, +in order: **what is running now**, **what is blocked on what**, **what needs +me**. + +This is a product surface, not a greenfield page: it inherits the Web Shell +token system, light/dark, CSS modules, and a stylesheet-source test +convention. So the palette is not up for invention — the plan spends its +freedom on structure and on the _rule_ governing existing tokens. + +## What is wrong today + +Read against the three questions, not as taste: + +1. **Reading order is inverted.** The node leads with a status glyph, then the + raw Todo id, then the status word; `nodeContent` — the only element that + says what the step _is_ — is third. At graph scale the reader scans + `● step-3 Blocked` before "Compare findings". An id is an address, and it + is the least useful thing at the largest scale. +2. **Status is stated three or four times per node**: the glyph, the status + word, the border tint, and an "attention" badge when it applies. On a 240px + node, most of the visible elements are about status. +3. **Dependencies are stated twice** — as a drawn edge, and again as + "Depends on step-3, step-7" in raw ids. +4. **Every node looks equally important.** In a real trace at most one node is + running and a few need attention; the rest are done or waiting. Same border + weight, same fill, same radius on all of them — the graph does not answer + "where do I look". +5. **Time is invisible.** This is an execution trace, and the layout encodes + dependency order but never elapsed time. +6. **The port dots are always `--agent-blue-500`** — the selection colour — + on every node that has edges. The accent is spent on something that never + changes, so selection has to shout over it. + +## Color + +No new values; a rule for the existing tokens. + +| role | token | used for | +| ----- | ----------------------- | ------------------------------------------------------------------------------------------ | +| ink | `--foreground` | step content — the only full-contrast text | +| quiet | `--muted-foreground` | counts, elapsed, ids | +| rule | `--border` | node borders; dependency lines are `--muted-foreground` with `--agent-blue-500` arrowheads | +| live | `--status-running-fg` | running and in_progress | +| done | `--status-done-fg` | completed, and nothing else | +| wait | `--status-attention-fg` | needs attention, and paused (plus the step-details chips) | +| pick | `--agent-blue-500` | selection and focus only, never decoration | + +**The rule: a node carries at most one status colour, in exactly one place, +and only when the status is worth saying.** Waiting/blocked is the resting +state of most nodes in any real plan, so it carries none — the stylesheet +already makes this argument for `blocked`; this extends it consistently +instead of applying it to one status. + +## Type + +One family (the app's). Three sizes inside a node: + +- **content** 13px / 500 — promoted to the first line +- **meta** 11px / 400 quiet — one line beneath +- **number** 11px tabular — the step's index + +No status word on the node face in the default case; the left rule says it. +No mono on the node face — the id is an address, so it belongs where it is +copied (the detail panel), not on every card at graph scale. + +## Layout + +Two candidates, then the pick. + +**A — content-first, status as a left rule** + +``` +┌──────────────────────────────┐ +│▌ 3 Compare findings │ ▌ 3px status rule, absent when waiting +│ 2 agents 1m14s │ +└──────────────────────────────┘ +``` + +**B — status as a filled number column** + +``` +┌───┬──────────────────────────┐ +│ 3 │ Compare findings │ number cell filled with the status tone +│ │ 2 agents 1m14s │ +└───┴──────────────────────────┘ +``` + +**Pick: A, carrying B's step number.** The number has to appear on the node +because the inspector list and the new dependency chips both address steps by +number — three surfaces, one identity, or they disagree. But B spends ~24px +of a 240px lane on a cell whose only other job is a tone the rule already +carries. + +Alignment is left throughout. The lanes are already a strong vertical rhythm; +centring anything fights it. + +The node's border stays `rule` for every status. Removing the per-status +border tint is what stops the canvas reading as a wall of tinted boxes and +lets the one running node actually stand out. + +## Where the boldness goes + +One place: **the running node.** Usually exactly one node is running, and +"is this thing alive or hung" is the first question a developer asks of a live +trace. It gets the only saturated treatment on the canvas — a filled left rule +and a live elapsed count — and everything around it stays hairline and quiet. + +Motion: none added. The elapsed count is data changing, not an animation; no +transition on it. The existing `prefers-reduced-motion` block stands. + +## Principles + +1. The step's content is the node; everything else is annotation. +2. One status, one place, and only when it is worth saying. +3. The edge is the dependency statement — do not restate it as text. +4. The step number is one identity across graph, list, and dependency chip. +5. Exactly one thing on the canvas is loud: what is running now. + +## Explicitly not changing + +Edge routing, the layering algorithm, the return-lane mechanics, and the +measure pass. They work, they are heavily tested, and touching them would turn +a node-face redesign into a second oversized PR. This proposal is the node +face plus the colour rule. + +## Review against the brief + +Worked through the generic-default calibration; two findings, one of which +changed the plan. + +- Cream/serif/terracotta, or near-black with an acid accent: not applicable, + the tokens are the product's. +- Hairline rules everywhere: I _am_ moving toward hairlines, which is itself a + default look. Kept, but for a subject-specific reason — the hairline + replaces a per-status border tint, and the argument is the one the codebase + already makes for `blocked`: most nodes in a healthy plan are resting, and + tinting them all makes a healthy graph read as a wall of warnings. The 8px + radius stays, so this is not a broadsheet. +- The SaaS-card kit — identical rounded cards, one border treatment, a status + wash on each: this is what the node is today. Dropping the status border and + two of the three status tokens moves away from it. +- **Numbered markers (01 / 02 / 03):** the calibration warns against these + unless the content is genuinely a sequence. Here it is — a topologically + ordered plan — and the number is already how the inspector addresses steps. + Kept. +- **`·`-joined meta strings — caught, and the plan changed.** The first draft + of the node meta line read `step-3 · 2 agents · 1m14s`. That is on the tell + list, and it is the exact pattern just removed from the inspector. Revised + to two facts with real spacing and no id on the node face, matching the + `.metrics` flex row already built. This also buys back horizontal room in a + 240px lane, which is what makes the narrow-viewport case work honestly + rather than by shrinking type. + +## Caveat found while planning + +The "Depends on …" text row cannot simply be deleted. There are three layout +states, not two: + +- `hasDependencies === false` → flat layout, no `blockedBy` exists anywhere, + so nothing to state. Safe. +- `drawsDependencyEdges === true` → edges are drawn, the text is redundant. + Remove. +- `hasDependencies && !drawsDependencyEdges` → dependencies exist but exceed + `MAX_RENDERED_PLAN_EDGES` (500), so **no edges are drawn**. Here the text is + the only statement of the dependency and must stay. + +So the row is kept and gated on `!drawsDependencyEdges`, rendered as the same +number-and-title chips the inspector uses rather than as raw ids. + +## Corrections found after the plan, during implementation + +Recorded rather than folded in, because both are places the plan's own rule +was wrong and a later reader should see why. + +1. **"One status, one place" was too strong — it deleted an accessibility + channel.** The plan replaced the status glyph with a colour-only left rule. + But the glyph carried a documented purpose the plan never checked + (`PlanExecutionView.tsx`: "Status is carried by a glyph as well as a colour + so the graph survives colour-blindness, high-contrast mode, and a greyscale + screenshot"). Colour plus a screen-reader-only word leaves a sighted + colour-blind reader, high-contrast mode, and any greyscale capture with no + way to tell running from completed. The rule is corrected to: one carrier + of the status _colour_, plus a redundant non-colour channel. The glyph + returns for every status, in the meta line so the content still leads, and + muted so the rule stays the only thing carrying the tone. + +2. **A pseudo-element rule needed clipping, and clipping cut the port.** + Drawing the rule as `.node::before` required `overflow: hidden` for the 8px + radius to apply — which also clips the outgoing port dot, deliberately + positioned at `right: -4px` outside the box. The rule is a `border-left` + instead, which border-radius clips natively and needs no overflow. Both + halves are now pinned by the stylesheet guard, since neither is visible to + jsdom or to a passing unit run. + +## Open questions + +The first two were answered by the implementation; the answers are recorded +here so the decisions stay visible. + +1. Does the elapsed count on a running node belong on the node face, or only + in the inspector? — On the face: the meta line renders it whenever it is + greater than zero. +2. Should `attention` keep a badge, or fold into the left rule as the `wait` + tone? — Folded into the rule: `data-attention` carries it, declared after + every status tone so it wins the cascade. +3. Verification: this box cannot run a browser, so the visual pass would ship + read from CSS. Worth a real render before merge. diff --git a/packages/web-shell/client/components/messages/PlanExecutionView.css.test.ts b/packages/web-shell/client/components/messages/PlanExecutionView.css.test.ts new file mode 100644 index 00000000000..09511e5fdda --- /dev/null +++ b/packages/web-shell/client/components/messages/PlanExecutionView.css.test.ts @@ -0,0 +1,205 @@ +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; + +// jsdom does not compute the CSS cascade, so pin the stylesheet's source +// shape instead. The DAG reserves bottom padding for its layer-spanning +// return lanes, and that reservation has to use the same pitch that places +// the lanes: TS owns `EDGE_LANE_HEIGHT` and the canvas placed lanes at +// `lane * EDGE_LANE_HEIGHT`, while the stylesheet re-stated the pitch as a +// literal `9px`. Raising the constant alone left the reservation short, the +// clamp pinned the outer lanes together, and layer-spanning edges overlapped +// again — the exact defect the lanes exist to fix — with no test signal, +// because the lane test mocks `offsetHeight` directly. Strip comments so the +// guards match declarations only, not prose about them. +const planCss = readFileSync( + fileURLToPath(new URL('./PlanExecutionView.module.css', import.meta.url)), + 'utf8', +).replace(/\/\*[\s\S]*?\*\//g, ''); +const planSource = readFileSync( + fileURLToPath(new URL('./PlanExecutionView.tsx', import.meta.url)), + 'utf8', +); + +describe('PlanExecutionView stylesheet', () => { + it('takes the edge-lane pitch from TS instead of restating it', () => { + const canvas = planCss.match(/\.dagCanvas\s*\{[^}]*\}/)?.[0]; + expect(canvas).toBeTruthy(); + expect(canvas).toMatch(/var\(--plan-edge-lane-height/); + // The literal that used to stand in for the constant must not return + // anywhere in the reservation. + expect(canvas).not.toMatch(/--plan-edge-lanes\)\s*\*\s*9px/); + // No second copy of the constant as a var() fallback either: the pitch + // has one source, and the default only covers the zero-lane case. + expect(canvas).not.toMatch(/--plan-edge-lane-height,\s*9px/); + // TS has to actually publish it, or the fallback silently takes over. + expect(planSource).toMatch( + /'--plan-edge-lane-height':\s*`\$\{EDGE_LANE_HEIGHT\}px`/, + ); + }); + + // The output port is positioned at `right: -4px`, deliberately outside the + // node's box, so the node must not clip its overflow — and the status rule + // must therefore be a border, which border-radius clips natively, rather + // than a pseudo-element that would need `overflow: hidden` to be rounded. + // Neither half of that is visible to jsdom or to a green unit run. + it('keeps the node unclipped so its outgoing port survives', () => { + const node = planCss.match(/(^|\n)\.node\s*\{[^}]*\}/)?.[0]; + expect(node).toBeTruthy(); + expect(node).not.toMatch(/overflow:\s*hidden/); + expect(node).toMatch(/border-left:\s*3px solid var\(--node-rule\)/); + expect(planCss).not.toMatch(/\.node::before\s*\{/); + expect(planCss).toMatch( + /\.dagCanvas\s+\.node\[data-plan-output='true'\]::after\s*\{[^}]*right:\s*-4px/, + ); + // Clipping on ANY rule whose subject is `.node` — a status variant, + // attention, selection, a `:has()` state, the media-scoped copies — cuts + // the port just as surely as clipping on the base rule, and jsdom never + // sees it. Walk every rule block and clear every `.node` subject of + // overflow clipping; rules that merely descend from `.node` (such as + // `.node[...] .nodeGlyph`) keep their own overflow business, as do the + // chip and title rules that clip legitimately. + const clipped: string[] = []; + // At-rules wrap rules without adding a selector subject of their own, so + // unwrap their preludes and scan the inner rules like top-level ones + // (the leftover closing braces are lone text no block regex matches). + const unwrapped = planCss.replace(/@[a-z-]+[^{]*\{/gi, ''); + for (const [, selectorList, body] of unwrapped.matchAll( + /([^{}]+)\{([^}]*)\}/g, + )) { + if (!/overflow:\s*(hidden|clip)/.test(body)) continue; + for (const selector of selectorList.split(',')) { + const subject = + selector + .trim() + .split(/\s*[>+~\s]\s*/) + .pop() ?? ''; + if (/\.node(?![A-Za-z0-9-])/.test(subject)) { + clipped.push(selector.trim()); + } + } + } + expect(clipped).toEqual([]); + }); + + // The selected rule's `border-color` shorthand expands to + // border-left-color and outranks the base rule's + // `border-left: 3px solid var(--node-rule)` longhand (0,2,0 beats 0,1,0) + // while the 3px width survives — so a selected blocked/ready node, whose + // rule is deliberately transparent, grew a 3px muted left bar and + // selection read as a status change. jsdom computes no cascade, so the + // re-pin is asserted at the source. + it('keeps selection off the status rule', () => { + const selected = planCss.match( + /(^|\n)\.node\[data-selected='true'\]\s*\{[^}]*\}/, + )?.[0]; + expect(selected).toBeTruthy(); + expect(selected).toMatch(/border-left-color:\s*var\(--node-rule\)/); + }); + + // `text-overflow` applies to a block container's own inline content, so + // declaring it on the inline-flex chip did nothing: the title sat in an + // anonymous flex item and was clipped with no ellipsis. jsdom computes no + // layout, so this is pinned at the source instead. + it('puts dependency truncation on the title, not the flex box', () => { + for (const name of ['dependencyChip', 'dependencyLink']) { + const rule = planCss.match( + new RegExp(`(^|\\n)\\.${name}\\s*\\{[^}]*\\}`), + )?.[0]; + expect(rule).toBeTruthy(); + expect(rule).toMatch(/display:\s*inline-flex/); + expect(rule).not.toMatch(/text-overflow/); + } + const title = planCss.match( + /\.dependencyChip\s+\.dependencyTitle,\s*\n\s*\.dependencyLink\s+\.dependencyTitle\s*\{[^}]*\}/, + )?.[0]; + expect(title).toMatch(/text-overflow:\s*ellipsis/); + // `text-overflow` only takes effect on a box whose own `overflow` is + // hidden; overflow is not inherited, and the chip's own clipping does + // not reach the title span, so the declaration must live on this rule. + expect(title).toMatch(/overflow:\s*hidden/); + expect(title).toMatch(/min-width:\s*0/); + // The `> span` rules mute every direct-child span, the title included; + // without its own colour the title would render muted while the chip's + // `color: var(--foreground)` applies to no rendered text. + expect(title).toMatch(/color:\s*var\(--foreground\)/); + }); + + it('declares the attention tone after every status tone', () => { + // All these selectors carry the same specificity, so the cascade lets + // the later rule win. Attention co-occurs with every status (a linked + // agent task can fail under any of them), so the attention rule must + // stay declared after every status tone, or a node that needs attention + // would wear its status ring instead of the attention tone its comment + // promises — whichever status block a later edit regroups. + const attentionAt = planCss.indexOf(".node[data-attention='true']"); + expect(attentionAt).toBeGreaterThan(-1); + for (const status of [ + 'completed', + 'running', + 'in_progress', + 'paused', + 'blocked', + 'ready', + ]) { + const statusAt = planCss.indexOf(`.node[data-status='${status}']`); + expect(statusAt).toBeGreaterThan(-1); + expect(attentionAt).toBeGreaterThan(statusAt); + } + }); + + it('narrows the DAG lane at two viewport steps', () => { + // Three 240px lanes plus two 64px gutters exceed a phone viewport, so the + // canvas scrolled in both axes at once and the horizontal scroll hid the + // layer the vertical scroll was looking for. + expect(planCss).toMatch( + /\.dagCanvas\s+\.layer\s*\{[^}]*flex:\s*0\s+0\s+var\(--plan-layer-width/, + ); + // Pinned as the source shape, not as an outcome: jsdom computes no + // cascade and no layout, so this cannot assert that a plan of a given + // depth stops scrolling — only that both steps exist and narrow. + const narrow = planCss.match( + /@media\s*\(max-width:\s*720px\)\s*\{[\s\S]*?\n\}/, + )?.[0]; + // The width pin names the block's owner selector too: a declaration + // moved onto a selector `.layer` never reads from must fail the guard + // even though the value still appears inside the media block. + expect(narrow).toMatch(/\.dagCanvas\s*\{[^}]*--plan-layer-width:\s*168px/); + // The fit also rests on the paired gutter narrowing: dropping it falls + // back to the base 64px gap, putting a three-layer plan past the + // viewport this step exists to fit. + expect(narrow).toMatch(/gap:\s*32px/); + const narrower = planCss.match( + /@media\s*\(max-width:\s*480px\)\s*\{[\s\S]*?\n\}/, + )?.[0]; + expect(narrower).toMatch( + /\.dagCanvas\s*\{[^}]*--plan-layer-width:\s*116px/, + ); + // Same pairing at the phone step: 3×116px lanes only fit because the + // gap narrows to 18px and the canvas padding to 6px a side + // (3×116 + 2×18 + 2×6 = 396px). Dropping any of the three restores + // the two-axis scroll the step was added to remove. + expect(narrower).toMatch(/gap:\s*18px/); + expect(narrower).toMatch(/padding-right:\s*6px/); + expect(narrower).toMatch(/padding-left:\s*6px/); + }); + + it('keeps the sr-only dependency summary off screen', () => { + // The interactive graph states its dependencies to assistive tech only, + // because the visible chip row is deliberately off there and the drawn + // edges are aria-hidden. That only holds while the summary shares the + // status word's screen-reader clipping: dropping it from this selector + // list turns the summary into a visible row — the very thing the diff + // removed — with no test signal, since jsdom computes no cascade and the + // component test can only see the class name. + const srOnly = planCss.match( + /(^|\n)\.nodeStatusText,\s*\n\s*\.nodeDependencyText\s*\{[^}]*\}/, + )?.[0]; + expect(srOnly).toBeTruthy(); + expect(srOnly).toMatch(/position:\s*absolute/); + expect(srOnly).toMatch(/clip-path:\s*inset\(50%\)/); + expect(srOnly).toMatch(/width:\s*1px/); + expect(srOnly).toMatch(/height:\s*1px/); + expect(srOnly).toMatch(/overflow:\s*hidden/); + }); +}); diff --git a/packages/web-shell/client/components/messages/PlanExecutionView.module.css b/packages/web-shell/client/components/messages/PlanExecutionView.module.css index 6e8e297e4f1..8f19abede02 100644 --- a/packages/web-shell/client/components/messages/PlanExecutionView.module.css +++ b/packages/web-shell/client/components/messages/PlanExecutionView.module.css @@ -136,7 +136,11 @@ } .dagCanvas { + /* Both are overwritten from TS whenever the graph has dependencies; these + defaults only cover the no-dependency case, where --plan-edge-lanes is 0 + and the reservation collapses regardless of the pitch. */ --plan-edge-lanes: 0; + --plan-edge-lane-height: 0px; position: relative; display: flex; align-items: center; @@ -144,7 +148,11 @@ gap: 64px; width: max-content; min-width: 100%; - padding: 8px 10px calc(28px + var(--plan-edge-lanes) * 9px); + /* Both operands come from TS (PlanExecutionView sets --plan-edge-lanes and + --plan-edge-lane-height from EDGE_LANE_HEIGHT) so the reservation cannot + drift from the lane placement it is reserving for. */ + padding: 8px 10px + calc(28px + var(--plan-edge-lanes) * var(--plan-edge-lane-height)); } .dagEdges { @@ -218,25 +226,67 @@ .dagCanvas .layer { position: relative; z-index: 1; - flex: 0 0 240px; + flex: 0 0 var(--plan-layer-width, 240px); min-width: 0; } -/* Status drives one accent, and the accent drives every edge treatment on the - node: the border, and the selection ring. Before this, status set - `border-color` while selection stacked an always-blue ring on top, so a - completed (green) or blocked (amber) node picked up a second border in a - clashing hue the moment it was selected. */ +/* Three 240px lanes plus two 64px gutters are ~850px, so on a phone the graph + became a canvas that scrolls in both axes at once — the horizontal scroll + hiding the layer the vertical scroll is looking for. Narrowing the lane and + the gutter keeps the whole topology on the horizontal axis the graph + actually needs, and leaves the layer semantics (and the measured edge + routing, which reads real rects) untouched. */ +@media (max-width: 720px) { + .dagCanvas { + --plan-layer-width: 168px; + gap: 32px; + } +} + +/* 168px lanes still put a three-layer plan at ~588px, which scrolls on a + 390px phone and grows by a lane with every extra layer. This step brings + a three-layer plan to ~396px (3×116px lanes, 2×18px gaps, 2×6px canvas + padding): it fits phones wider than ~396px, and a 390px phone still + scrolls the last ~6px. Deeper plans scroll horizontally at any phone + width, which is the honest limit of narrowing alone — reusing the flat + layout is tracked in #10866. */ +@media (max-width: 480px) { + .dagCanvas { + --plan-layer-width: 116px; + gap: 18px; + padding-right: 6px; + padding-left: 6px; + } +} + +/* Status now drives the left rule; the border stays neutral for every status + and the accent is left to mean selection. Tinting every border made a + healthy plan read as a wall of coloured boxes and left the one running node + with nothing to stand out against — the first thing a reader of an + execution trace looks for. The older hazard this replaces is still worth + knowing: status used to set `border-color` while selection stacked an + always-blue ring on top, so a completed or blocked node picked up a second + border in a clashing hue the moment it was selected. With status off the + border, the two can no longer collide. */ .node { --node-accent: var(--border); --node-ring: var(--agent-blue-500); + --node-rule: transparent; position: relative; display: flex; flex-direction: column; - gap: 6px; + gap: 5px; min-width: 0; padding: 9px 10px; border: 1px solid var(--node-accent); + /* Status, stated once. A border rather than a pseudo-element so the node's + own border-radius clips it natively: clipping a ::before would have + needed `overflow: hidden`, which also cuts the output port dot in half — + it sits at `right: -4px`, deliberately outside the box. Transparent when + resting, and the node's background paints under it (background-clip is + border-box), so a resting node reads as 3px of extra left padding with + no width difference between states. */ + border-left: 3px solid var(--node-rule); border-radius: 8px; background: var(--background); transition: @@ -244,25 +294,25 @@ box-shadow 160ms ease; } -.dagCanvas .node::before, +/* Only the outgoing port is drawn; an incoming edge already terminates in an + arrowhead, which is the input marker. It also drops the accent: every node + with an edge had one, so a permanently blue dot across most of the graph + left selection no colour of its own. It is structure, so it takes the + hairline. Note this sits outside the node's box, so the node must not clip + its overflow. */ .dagCanvas .node::after { display: none; position: absolute; top: 50%; width: 8px; height: 8px; - border: 1px solid var(--agent-blue-500); + border: 1px solid var(--border); border-radius: 999px; background: var(--background); content: ''; transform: translateY(-50%); } -.dagCanvas .node[data-plan-input='true']::before { - left: -4px; - display: block; -} - .dagCanvas .node[data-plan-output='true']::after { right: -4px; display: block; @@ -274,26 +324,18 @@ real plan, so it stays neutral; painting it amber made a perfectly healthy graph read as a wall of warnings. */ .node[data-status='completed'] { - --node-accent: color-mix(in srgb, var(--status-done-fg) 55%, var(--border)); + --node-rule: var(--status-done-fg); --node-ring: var(--status-done-fg); } .node[data-status='running'], .node[data-status='in_progress'] { - --node-accent: color-mix( - in srgb, - var(--status-running-fg) 55%, - var(--border) - ); + --node-rule: var(--status-running-fg); --node-ring: var(--status-running-fg); } .node[data-status='paused'] { - --node-accent: color-mix( - in srgb, - var(--status-attention-fg) 55%, - var(--border) - ); + --node-rule: var(--status-attention-fg); --node-ring: var(--status-attention-fg); } @@ -303,10 +345,25 @@ --node-ring: var(--muted-foreground); } +/* Attention folds into the same rule instead of adding a fourth status + token to the node face: one status, one place. It outranks the status + tone because it is the state the reader is being asked to act on — so + it is declared after every status tone: at equal specificity the cascade + lets the later rule win, and a blocked/ready node that also needs + attention would otherwise wear the muted ring. */ +.node[data-attention='true'] { + --node-rule: var(--status-attention-fg); + --node-ring: var(--status-attention-fg); +} + /* Selection reads as "the same edge, turned up": the border goes to full strength and the ring echoes it, rather than introducing a second colour. */ .node[data-selected='true'] { border-color: var(--node-ring); + /* The shorthand above expands to border-left-color too: on blocked/ready, + whose --node-rule stays transparent, it would repaint the 3px status + rule in the ring tone and selection would read as a status change. */ + border-left-color: var(--node-rule); box-shadow: 0 0 0 2px color-mix(in srgb, var(--node-ring) 32%, transparent); } @@ -333,10 +390,50 @@ outline: none; } +/* Content first. The node led with a glyph, the raw Todo id and the status + word, so the only element that says what the step *is* came third — and an + id is an address, the least useful thing at graph scale. */ .nodeTop { display: flex; - align-items: center; - gap: 6px; + align-items: baseline; + gap: 7px; +} + +/* The same number the inspector list and the dependency chips use, so the + three surfaces address a step identically. */ +.nodeNumber { + flex: none; + color: var(--muted-foreground); + font-size: 11px; + font-variant-numeric: tabular-nums; +} + +/* The left rule is colour, which assistive tech cannot read, so the status + stays in the accessibility tree as words. The dependency summary shares the + channel: the interactive graph draws the dependency as an aria-hidden edge + rather than stating it, so the node's accessible name has to carry it. */ +.nodeStatusText, +.nodeDependencyText { + position: absolute; + overflow: hidden; + width: 1px; + height: 1px; + padding: 0; + border: 0; + margin: -1px; + clip-path: inset(50%); + white-space: nowrap; +} + +/* Two facts, spaced, rather than one middle-dot-joined string: they wrap as + a unit and read out as one run of text when glued. */ +.nodeMeta { + display: flex; + flex-wrap: wrap; + gap: 1px 12px; + color: var(--muted-foreground); + font-size: 11px; + font-variant-numeric: tabular-nums; } .nodeId { @@ -373,13 +470,87 @@ } .nodeContent { + min-width: 0; color: var(--foreground); font-size: 13px; + font-weight: 500; overflow-wrap: anywhere; } +/* Rendered only when no edge states the dependency — above + MAX_RENDERED_PLAN_EDGES the graph draws none, and then this row is the + only statement of it. Chips carry the step number and title, matching the + inspector, instead of a comma-joined run of raw ids. */ .dependencies { + display: flex; + flex-wrap: wrap; + align-items: baseline; + gap: 3px 6px; + color: var(--muted-foreground); + font-size: 11px; +} + +/* `text-overflow` applies to a block container's own inline content, so + declaring it on the flex box did nothing — the title sat in an anonymous + flex item and was clipped without an ellipsis. The truncation belongs on + the title span, which is a real box. */ +.dependencyChip { + display: inline-flex; + max-width: 100%; + align-items: baseline; + overflow: hidden; + gap: 5px; + padding: 1px 6px; + border: 1px solid var(--border); + border-radius: var(--radius-md, 6px); + color: var(--foreground); + white-space: nowrap; +} + +/* The `> span` rules below mute every direct-child span — the title + included — and a direct declaration beats the chip's inherited colour, + so the title states its own full-weight colour here. */ +.dependencyChip .dependencyTitle, +.dependencyLink .dependencyTitle { + overflow: hidden; + min-width: 0; + color: var(--foreground); + text-overflow: ellipsis; + white-space: nowrap; +} + +.dependencyChip > span { + color: var(--muted-foreground); + font-variant-numeric: tabular-nums; +} + +/* The step-details panel's references, which sit outside the node's button + and so can be real controls. Same face as the node's read-only chips. */ +.dependencyLink { + display: inline-flex; + max-width: 100%; + align-items: baseline; + overflow: hidden; + gap: 5px; + padding: 1px 6px; + border: 1px solid var(--border); + border-radius: var(--radius-md, 6px); + background: var(--background); + color: var(--foreground); + cursor: pointer; font-size: 11px; + text-align: left; + white-space: nowrap; +} + +.dependencyLink:hover { + border-color: color-mix(in srgb, var(--agent-blue-500) 45%, var(--border)); + background: var(--accent); +} + +.dependencyLink > span { + color: var(--muted-foreground); + font-variant-numeric: tabular-nums; } .executions { @@ -584,10 +755,12 @@ button.nestedExecution:not(:disabled):focus-visible { /* The status glyph. Sized and aligned to the mono ID beside it so the two read as one identifier block rather than an icon bolted onto a label. */ +/* Muted, not `--node-ring`: the left rule is the single carrier of the + status colour, and this is the redundant *shape* channel beside it. */ .nodeGlyph { flex: 0 0 auto; width: 13px; - color: var(--node-ring); + color: var(--muted-foreground); font-family: var(--font-mono, monospace); font-size: 12px; font-style: normal; @@ -595,6 +768,23 @@ button.nestedExecution:not(:disabled):focus-visible { text-align: center; } +/* The attention sigil, muted like the status glyph: the left rule stays the + only carrier of tone, and this is the shape channel that survives forced + colours — the data-attention rule re-declares the very token paused + already wears, so without a mark a paused node needing attention paints + pixel-identical to a healthy one. */ +.nodeAttentionMark { + flex: 0 0 auto; + width: 13px; + color: var(--muted-foreground); + font-family: var(--font-mono, monospace); + font-size: 12px; + font-style: normal; + font-weight: 700; + line-height: 1; + text-align: center; +} + /* Work in flight is the only thing on the page that moves, so the eye finds it without a legend. */ .node[data-status='running'] .nodeGlyph { diff --git a/packages/web-shell/client/components/messages/PlanExecutionView.test.tsx b/packages/web-shell/client/components/messages/PlanExecutionView.test.tsx index bf277862763..482a685ec71 100644 --- a/packages/web-shell/client/components/messages/PlanExecutionView.test.tsx +++ b/packages/web-shell/client/components/messages/PlanExecutionView.test.tsx @@ -6,6 +6,7 @@ import { describe, expect, it, vi } from 'vitest'; import type { DaemonSessionAgentTaskStatus } from '@qwen-code/sdk/daemon'; import type { ACPToolCall, TodoItem } from '../../adapters/types'; import { I18nProvider } from '../../i18n'; +import styles from './PlanExecutionView.module.css'; import { TranscriptRenderModeProvider } from '../../transcriptRenderMode'; import { getActiveAgents, @@ -14,6 +15,7 @@ import { layerPlanTodos, nestedAgentToolsForTool, nestedTasksForTool, + PLAN_STATUS_GLYPH, PlanExecutionView, } from './PlanExecutionView'; @@ -571,7 +573,64 @@ describe('PlanExecutionView', () => { ); }); - expect(container.textContent).toContain('Depends on: research'); + // The drawn edge is the dependency statement, so the node face does not + // restate it *visibly* — the chip row stays off. The step-details panel + // still states it, asserted below, and the sr-only summary carries it to + // assistive tech because the drawn edges are aria-hidden (asserted in + // 'names the blockers in the node accessible name…'). + const buildNode = container + .querySelector('[data-plan-node-id="build"]') + ?.closest('article'); + expect(buildNode?.querySelector(`.${styles.dependencies}`)).toBeNull(); + // Content leads the node; the step number matches the inspector list and + // the dependency chips, and the agent count and elapsed carry the "is + // this alive" signal onto the face. + expect(buildNode?.textContent).toContain('Build'); + // Root agent plus the live child task below (parentAgentId set, no + // transcript entry): the node renders two agent rows, so the face tally + // must read 2 — counting only transcript subTools is the R8-2 divergence. + expect(buildNode?.textContent).toContain('2 agents'); + expect(buildNode?.textContent).toContain('1m 5s'); + // The plural branch is pinned above; `1 agents` would ship green without + // the negative assertion here. The singular branch is pinned in + // 'the node-face agent tally matches the rows it renders'. + const researchNode = container + .querySelector('[data-plan-node-id="research"]') + ?.closest('article'); + expect(buildNode?.textContent).not.toContain('1 agents'); + expect(researchNode?.textContent ?? '').not.toContain('1 agents'); + // Status is colour on the left rule, so it stays in the accessibility + // tree as words rather than being dropped. Asserted on a node with no + // linked agent, so the word can only come from the node's own status + // element and not from an execution row inside it. + const verifyNode = container + .querySelector('[data-plan-node-id="verify"]') + ?.closest('article'); + expect( + verifyNode?.querySelector(`.${styles.nodeStatusText}`)?.textContent, + ).toBe('Blocked'); + // `verify` has no linked tool call in this fixture, so its visible face + // is still just the number, the content and the status glyph; the two + // sr-only spans (status word, dependency summary) are the only other text + // the node carries. + expect(verifyNode?.querySelector(`.${styles.nodeTop}`)?.textContent).toBe( + '3Verify', + ); + expect(verifyNode?.querySelector(`.${styles.nodeMeta}`)?.textContent).toBe( + PLAN_STATUS_GLYPH.blocked, + ); + // The glyph is the non-colour status channel: the left rule that carries + // status visually is colour only, so without a shape beside it the graph + // would lose status entirely under colour-blindness, high-contrast mode + // or a greyscale screenshot. Pinned per status, not just as "a glyph". + expect(buildNode?.textContent).toContain(PLAN_STATUS_GLYPH.running); + expect( + container + .querySelector('[data-plan-node-id="research"]') + ?.closest('article')?.textContent, + ).toContain(PLAN_STATUS_GLYPH.completed); + expect(PLAN_STATUS_GLYPH.blocked).not.toBe(PLAN_STATUS_GLYPH.running); + expect(PLAN_STATUS_GLYPH.running).not.toBe(PLAN_STATUS_GLYPH.completed); expect(container.textContent).toContain('33%'); expect(container.textContent).toContain('1 / 3'); // 3, not 2: the strip now derives from the same source as the node @@ -590,8 +649,26 @@ describe('PlanExecutionView', () => { const details = container.querySelector('[data-plan-step-details]'); expect(details?.textContent).toContain('Step details'); expect(details?.textContent).toContain('Build'); - expect(details?.textContent).toContain('Depends on: research'); - expect(details?.textContent).toContain('Unblocks: verify'); + // Outside the node's own button, so these references are real controls: + // each names the step by number and title, and selects it on click. + const upstreamLink = details?.querySelector( + '[data-plan-dependency="research"]', + ); + expect(upstreamLink?.tagName).toBe('BUTTON'); + // The host keyboard handlers isolate plan controls through this marker + // (ToolApproval/TasksStatusMessage early-return on it), so these buttons + // must carry it like every other control in the view. + expect(upstreamLink?.hasAttribute('data-plan-interactive')).toBe(true); + expect(upstreamLink?.textContent).toBe('1Research'); + expect( + details?.querySelector('[data-plan-dependency="verify"]')?.textContent, + ).toBe('3Verify'); + expect( + details + ?.querySelector('[data-plan-dependency="verify"]') + ?.hasAttribute('data-plan-interactive'), + ).toBe(true); + expect(details?.textContent).not.toContain('Depends on: research'); expect(details?.textContent).toContain('Subagents'); expect(details?.textContent).toContain( 'Current activity:Inspecting the implementation', @@ -606,6 +683,193 @@ describe('PlanExecutionView', () => { act(() => button?.click()); expect(onOpen).toHaveBeenCalledWith(agentTool('build')); + // The downstream block wires its own onClick; witness it before the + // upstream click below moves the selection off `build`. Dropping that + // handler must turn this red. + act(() => + details + ?.querySelector('[data-plan-dependency="verify"]') + ?.click(), + ); + expect( + container + .querySelector('[data-plan-node-id="verify"]') + ?.getAttribute('aria-pressed'), + ).toBe('true'); + // That click selected `verify` and re-rendered the panel around it, so + // bring the selection back to `build` before exercising the upstream + // reference. + act(() => { + container + .querySelector('[data-plan-node-id="build"]') + ?.click(); + }); + expect( + container + .querySelector('[data-plan-node-id="build"]') + ?.getAttribute('aria-pressed'), + ).toBe('true'); + + // Last, because it moves the selection: following an upstream reference + // selects the step it names, which is what makes the dependency list the + // graph's navigation rather than a run of text. + act(() => + details + ?.querySelector('[data-plan-dependency="research"]') + ?.click(), + ); + expect( + container + .querySelector('[data-plan-node-id="research"]') + ?.getAttribute('aria-pressed'), + ).toBe('true'); + + act(() => root.unmount()); + container.remove(); + }); + + it('drops unresolvable dependency ids from the step-details controls', () => { + // blockedBy is model-authored todo_write output parsed without + // normalization, so it can name a step that does not exist. Such an id + // must not render as a control: clicking it would select a ghost id, + // empty selectedTodo, and hide the panel mid-navigation. + const ghostTodos = todos.map((todo) => + todo.id === 'build' + ? { ...todo, blockedBy: ['research', 'ghost-step'] } + : todo, + ); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + , + ); + }); + act(() => { + container + .querySelector('[data-plan-node-id="build"]') + ?.click(); + }); + const details = container.querySelector('[data-plan-step-details]'); + expect(details).not.toBeNull(); + // The resolvable dependency stays a working control… + expect( + details?.querySelector('[data-plan-dependency="research"]'), + ).not.toBeNull(); + // …while the ghost id renders none at all. + expect( + details?.querySelector('[data-plan-dependency="ghost-step"]'), + ).toBeNull(); + expect(details?.textContent).not.toContain('ghost-step'); + // The ellipsis lives on `.dependencyTitle`; pin the class wiring so a + // dropped className cannot re-clip titles while these text assertions + // stay green. + expect( + Array.from( + details + ?.querySelector('[data-plan-dependency="research"]') + ?.querySelectorAll('span') ?? [], + ).some((span) => span.classList.contains(styles.dependencyTitle)), + ).toBe(true); + + act(() => root.unmount()); + container.remove(); + }); + + it('announces attention beside the status word, not instead of it', () => { + // A failed descendant puts the node into attention while its own status + // stays running. The sr-only span must keep announcing the status word + // too — attention is additive for assistive tech, never a replacement. + const runningRoot = task('running'); + const failedChild = task('failed', { + id: 'agent-child', + toolUseId: 'call-child', + parentAgentId: runningRoot.id, + }); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + , + ); + }); + + const buildNode = container + .querySelector('[data-plan-node-id="build"]') + ?.closest('article'); + expect(buildNode?.textContent).toContain('Running'); + expect(buildNode?.textContent).toContain('Needs attention'); + expect( + buildNode + ?.querySelector(`.${styles.nodeStatusText}`) + ?.textContent?.trim(), + ).toBe('Running, Needs attention'); + // The attribute is the hook the `.node[data-attention='true']` colour + // rule selects on; dropping it keeps the word but silently loses the + // attention tone the stylesheet derives from this state. + expect(buildNode?.getAttribute('data-attention')).toBe('true'); + // Colour alone cannot carry attention: on a paused node the + // data-attention rule re-declares the token paused already wears, so + // without a shape channel the two paint pixel-identical. The visible + // sigil lives in the meta row beside the status glyph; the words stay + // in the sr-only span above. + const attentionMark = buildNode?.querySelector( + `.${styles.nodeMeta} .${styles.nodeAttentionMark}`, + ); + expect(attentionMark?.textContent).toBe('!'); + expect(attentionMark?.getAttribute('aria-hidden')).toBe('true'); + + act(() => root.unmount()); + container.remove(); + }); + + it('pluralizes the agent count when two agents share one node', () => { + // Every other fixture links a single agent per node, so the EN + // template's plural branch ships unobserved: a template that always + // emits `${count} agent` renders "2 agent" and nothing turns red. + // Link two root agents to one step and pin the plural rendering. + const secondBuildTool: ACPToolCall = { + ...agentTool('build'), + callId: 'call-build-2', + title: 'Agent build 2', + }; + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + , + ); + }); + + const buildNode = container + .querySelector('[data-plan-node-id="build"]') + ?.closest('article'); + expect(buildNode?.textContent).toContain('2 agents'); + act(() => root.unmount()); container.remove(); }); @@ -868,7 +1132,11 @@ describe('PlanExecutionView', () => { } if (this.tagName === 'ARTICLE') { const [left, top] = - positions[this.querySelector('span')!.textContent!]!; + positions[ + this.querySelector('[data-plan-node-id]')!.getAttribute( + 'data-plan-node-id', + )! + ]!; return scaledRect(100 + left * 0.72, 50 + top * 0.72, 144, 57.6); } return scaledRect(0, 0, 0, 0); @@ -951,7 +1219,11 @@ describe('PlanExecutionView', () => { } if (this.tagName === 'ARTICLE') { const [left, top] = - positions[this.querySelector('span')!.textContent!]!; + positions[ + this.querySelector('[data-plan-node-id]')!.getAttribute( + 'data-plan-node-id', + )! + ]!; return rect(100 + left, 50 + top, 200, 80); } return rect(0, 0, 0, 0); @@ -988,6 +1260,14 @@ describe('PlanExecutionView', () => { (match) => Number(match[1]), ); expect(Math.max(...routedYs)).toBeGreaterThan(200); + // ...and arrives at the target's own row, not merely at its column. The + // `release` node sits at y 10 with height 80 inside a viewport whose + // origin is (100, 50), so its vertical centre is 50. Without this, a tail + // drawn from the return lane's `routeY` instead of the edge's `endY` + // lands ~170px below the node while the column and clearance assertions + // above both still pass. + expect(spanningEdge).toMatch(/ 50 H 876$/); + expect(routedYs).toContain(50); act(() => root.unmount()); container.remove(); @@ -1086,7 +1366,11 @@ describe('PlanExecutionView', () => { } if (this.tagName === 'ARTICLE') { const [left, top] = - positions[this.querySelector('span')!.textContent!]!; + positions[ + this.querySelector('[data-plan-node-id]')!.getAttribute( + 'data-plan-node-id', + )! + ]!; return rect(100 + left, 50 + top, 200, 80); } return rect(0, 0, 0, 0); @@ -1137,6 +1421,211 @@ describe('PlanExecutionView', () => { } }); + it('keeps the arrowhead pointing at the target on every gutter tier', () => { + // The narrow gutters this PR adds are tighter than the router's 24px + // shoulder, so the control point landed on (32px tier) or past (18px + // tier) the end point. The curve then arrives with a zero or negative x + // tangent and `orient="auto"` flips the arrowhead back at its source — + // measured in the browser as (0, 0) at 700px and (-14, 0) at 430/390px + // against (28, 0) at 1440px. The input port is gone, so that arrowhead + // is the last direction cue the graph has. Restoring the fixed + // `Math.max(24, …)` shoulder turns the two narrow tiers red. + const chain: TodoItem[] = [ + { id: 'source', content: 'Source', status: 'completed' }, + { + id: 'target', + content: 'Target', + status: 'pending', + blockedBy: ['source'], + }, + ]; + // The lane width the 700px tier narrows to; only the gutter varies. + const lane = 168; + // gutter → 64px (≥721px), 32px (≤720px), 18px (≤480px). + for (const gap of [64, 32, 18]) { + const rect = (left: number, top: number, width: number, height: number) => + ({ + x: left, + y: top, + left, + top, + width, + height, + right: left + width, + bottom: top + height, + toJSON: () => ({}), + }) as DOMRect; + const rectSpy = vi + .spyOn(HTMLElement.prototype, 'getBoundingClientRect') + .mockImplementation(function () { + if (this.parentElement?.hasAttribute('data-plan-workflow')) { + return rect(100, 50, 1100, 300); + } + if (this.tagName === 'ARTICLE') { + const id = this.querySelector('[data-plan-node-id]')!.getAttribute( + 'data-plan-node-id', + )!; + return rect(100 + (id === 'target' ? lane + gap : 0), 60, lane, 80); + } + return rect(0, 0, 0, 0); + }); + const widthSpy = vi + .spyOn(HTMLElement.prototype, 'offsetWidth', 'get') + .mockReturnValue(1100); + const heightSpy = vi + .spyOn(HTMLElement.prototype, 'offsetHeight', 'get') + .mockReturnValue(300); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + try { + act(() => { + root.render( + + + , + ); + }); + + const d = container + .querySelector('[data-from="source"][data-to="target"]') + ?.getAttribute('d'); + expect(d, `${gap}px gutter drew no edge`).toBeTruthy(); + // `M startX startY C c1x c1y, c2x c2y, endX endY`. + const n = [...d!.matchAll(/-?[\d.]+/g)].map((m) => Number(m[0])); + expect(n).toHaveLength(8); + // Prove the fixture really reproduced the tier: the router insets + // both ends by 4px, so the run is the gutter minus 8 (56 / 24 / 10). + expect(n[6] - n[0], `${gap}px gutter run`).toBe(gap - 8); + // The end tangent is the end point minus the last control point, and + // it is what the arrowhead orients on. Its x must be strictly + // positive at every tier, or the head points back at the source. + expect(n[6] - n[4], `${gap}px gutter end tangent x`).toBeGreaterThan(0); + } finally { + act(() => root.unmount()); + container.remove(); + rectSpy.mockRestore(); + widthSpy.mockRestore(); + heightSpy.mockRestore(); + } + } + }); + + it('keeps a layer-skipping lane out of the step it passes', () => { + // The other half of R6-3. The router gave every layer-skipping edge a + // fixed 24px shoulder, which is wider than the ≤480px gutter (18px, a + // 10px run). At 390/430px the rise sat 10px inside the intervening step, + // so a dependency from step 1 to step 3 read as one into step 2 — and the + // SVG paints under the nodes, so the lane simply disappeared into it. + // Restoring `startX + 24` / `endX - 24` turns the 18px tier red. + const skip: TodoItem[] = [ + { id: 'one', content: 'One', status: 'completed' }, + { id: 'two', content: 'Two', status: 'pending', blockedBy: ['one'] }, + { + id: 'three', + content: 'Three', + status: 'pending', + // The layer-skipping edge: `one` is layer 0, `three` is layer 2. + blockedBy: ['two', 'one'], + }, + ]; + // The lane width the 700px tier narrows to; only the gutter varies. + const lane = 168; + // gutter → 64px (≥721px), 32px (≤720px), 18px (≤480px). + for (const gap of [64, 32, 18]) { + const rect = (left: number, top: number, width: number, height: number) => + ({ + x: left, + y: top, + left, + top, + width, + height, + right: left + width, + bottom: top + height, + toJSON: () => ({}), + }) as DOMRect; + // One node per layer here, so the step the lane passes is all of layer 1. + const leftOf = (layer: number) => layer * (lane + gap); + const rectSpy = vi + .spyOn(HTMLElement.prototype, 'getBoundingClientRect') + .mockImplementation(function () { + if (this.parentElement?.hasAttribute('data-plan-workflow')) { + return rect(100, 50, 1100, 300); + } + if (this.tagName === 'ARTICLE') { + const id = this.querySelector('[data-plan-node-id]')!.getAttribute( + 'data-plan-node-id', + )!; + const layer = id === 'one' ? 0 : id === 'two' ? 1 : 2; + return rect(100 + leftOf(layer), 60, lane, 80); + } + return rect(0, 0, 0, 0); + }); + const widthSpy = vi + .spyOn(HTMLElement.prototype, 'offsetWidth', 'get') + .mockReturnValue(1100); + const heightSpy = vi + .spyOn(HTMLElement.prototype, 'offsetHeight', 'get') + .mockReturnValue(300); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + try { + act(() => { + root.render( + + + , + ); + }); + + const d = container + .querySelector('[data-from="one"][data-to="three"]') + ?.getAttribute('d'); + expect(d, `${gap}px gutter drew no lane`).toBeTruthy(); + // The route's four turns, as [control x, end x]: Q1/Q2 carry dropX, + // Q3/Q4 carry riseX. Those are the two vertical segments' columns. + const turns = [ + ...d!.matchAll(/Q ([-\d.]+) [-\d.]+ ([-\d.]+) [-\d.]+/g), + ].map((match) => [Number(match[1]), Number(match[2])]); + expect(turns).toHaveLength(4); + const dropX = turns[0][0]; + const riseX = turns[2][0]; + const startX = lane + 4; + const endX = leftOf(2) - 4; + // The lane may only turn inside its own gutter, never inside layer 1. + expect(dropX, `${gap}px drop column`).toBeGreaterThan(startX); + expect(dropX, `${gap}px drop column`).toBeLessThan(leftOf(1)); + expect(riseX, `${gap}px rise column`).toBeGreaterThan(leftOf(1) + lane); + expect(riseX, `${gap}px rise column`).toBeLessThan(endX); + // The corner is halved at 18px so the run into the arrowhead keeps a + // positive length; at zero the head flips back at its source. + expect(Number(/H ([-\d.]+)$/.exec(d!)![1])).toBe(endX); + expect( + endX - turns[3][1], + `${gap}px gutter end tangent x`, + ).toBeGreaterThan(0); + + if (gap === 64) { + // A 64px gutter affords the full 24px shoulder on both sides, so + // desktop geometry is untouched. Pinned as a golden path because + // the requirement is byte-identical, not merely non-crossing. + expect(d).toBe( + 'M 172 50 H 190 Q 196 50 196 56 V 98 Q 196 104 202 104 ' + + 'H 430 Q 436 104 436 98 V 56 Q 436 50 442 50 H 460', + ); + } + } finally { + act(() => root.unmount()); + container.remove(); + rectSpy.mockRestore(); + widthSpy.mockRestore(); + heightSpy.mockRestore(); + } + } + }); + it('does not synchronously remeasure unchanged topology on task polling', () => { const rectSpy = vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect'); const container = document.createElement('div'); @@ -1342,6 +1831,275 @@ describe('PlanExecutionView', () => { container.remove(); }); + it('states the dependency on the node when no panel can state it', () => { + // The drawn edge is aria-hidden and the cockpit passes + // showStepDetails={false}, so there the chip row is the dependency's + // only statement anywhere — restoring the bare !drawsDependencyEdges + // gate turns this red. + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + , + ); + }); + + expect(container.querySelector('[data-plan-step-details]')).toBeNull(); + const buildNode = container + .querySelector('[data-plan-node-id="build"]') + ?.closest('article'); + expect(buildNode?.textContent).toContain('Depends on'); + expect(buildNode?.textContent).toContain('Research'); + + act(() => root.unmount()); + container.remove(); + }); + + it('names the blockers in the node accessible name when the graph draws them', () => { + // The other arm of the same gate. In the interactive graph the edges are + // drawn and the visible chip row is off by design, but the edge layer is + // aria-hidden — so the node's accessible name was the only place left to + // state the dependency, and it stopped stating it: base read + // `Blocked … Depends on: survey-api, read-tests`, head read + // `Blocked 4 Compare findings and draft the migration plan`. A + // screen-reader user had to activate every node to learn what blocks it. + // Dropping the sr-only summary turns this red. + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + // showStepDetails defaults to true: the Plan & Review card and the + // Plan & tasks dialog both render the graph that way. + act(() => { + root.render( + + + , + ); + }); + + // Nothing selected, so no details panel states the dependency either. + expect(container.querySelector('[data-plan-step-details]')).toBeNull(); + expect( + container.querySelector('[data-from="research"][data-to="build"]'), + ).not.toBeNull(); + + const buildButton = container.querySelector( + '[data-plan-node-id="build"]', + ); + const summary = buildButton?.querySelector(`.${styles.nodeDependencyText}`); + // A button's accessible name is its descendant text in DOM order, so this + // span is what a screen reader appends after the step's own title. + expect(summary?.textContent).toContain('Depends on:'); + // Human labels, matching the chips: step number plus title, not the id. + expect(summary?.textContent).toContain('1 Research'); + expect(summary?.textContent).not.toContain('research'); + // It restores the words only — the visible row stays off, which is the + // point of the diff. + expect(buildButton?.querySelector(`.${styles.dependencies}`)).toBeNull(); + // And it reads after the step it belongs to, like the base name did. + const announced = buildButton?.textContent ?? ''; + expect(announced.indexOf('Build')).toBeLessThan( + announced.indexOf('Depends on:'), + ); + // Sibling nodes with no blockers announce nothing extra. + expect( + container + .querySelector('[data-plan-node-id="research"]') + ?.querySelector(`.${styles.nodeDependencyText}`), + ).toBeNull(); + + act(() => root.unmount()); + container.remove(); + }); + + it('counts nested subagents in the node-face agent tally', () => { + // The chip counts agents, like the execution rows beneath it and the + // inspector's Subagents list — not just root executions. Reverting the + // count to executions.length reads "1 agent" here and goes red. + const rootTool: ACPToolCall = { + ...agentTool('build'), + subTools: [ + { + callId: 'call-nested', + toolName: 'Agent', + title: 'Nested agent', + status: 'in_progress', + }, + ], + }; + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + , + ); + }); + + const buildNode = container + .querySelector('[data-plan-node-id="build"]') + ?.closest('article'); + expect(buildNode?.textContent).toContain('2 agents'); + // The same node renders both agent rows the tally names. + expect(buildNode?.textContent).toContain('Agent build'); + expect(buildNode?.textContent).toContain('Nested agent'); + + act(() => root.unmount()); + container.remove(); + }); + + it('the node-face agent tally matches the rows it renders', () => { + // R8-2 witness: a live child task whose toolUseId matches no subTools + // callId renders a row (via toolForNestedTask) while the transcript-only + // tally never saw it — the node read "1 agent" beside two agent rows. + const rootTool: ACPToolCall = { + ...agentTool('build'), + subTools: [ + { + callId: 'call-nested', + toolName: 'Agent', + title: 'Nested agent', + status: 'in_progress', + }, + ], + }; + const rootTask = task('running'); + const liveChild = task('running', { + id: 'agent-live-child', + label: 'Live child', + toolUseId: 'call-live-child', + parentAgentId: rootTask.id, + }); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + , + ); + }); + + const buildNode = container + .querySelector('[data-plan-node-id="build"]') + ?.closest('article'); + // Three rows render: root, the transcript nested agent, and the live + // child — and the tally counts the same three. + expect(buildNode?.textContent).toContain('3 agents'); + expect(buildNode?.textContent).toContain('Nested agent'); + expect(buildNode?.textContent).toContain('Live child'); + // The lone root on the research node keeps the singular path pinned. + const researchNode = container + .querySelector('[data-plan-node-id="research"]') + ?.closest('article'); + expect(researchNode?.textContent).toContain('1 agent'); + expect(researchNode?.textContent).not.toContain('1 agents'); + + act(() => root.unmount()); + container.remove(); + }); + + it('counts an agent seen as both a live task and a transcript sub-tool once', () => { + // The tally counts the same deduped union the rows render from, so an + // agent observed through BOTH a live child task and a persisted subTools + // entry is one agent, not two. Dropping the `!liveCallIds.has(…)` filter + // reads "3 agents" beside two rows and turns this red (mutant M20). + const rootTool: ACPToolCall = { + ...agentTool('build'), + subTools: [ + { + callId: 'call-nested', + toolName: 'Agent', + title: 'Nested agent', + status: 'in_progress', + }, + ], + }; + const rootTask = task('running'); + // The live child carries the same toolUseId as the sub-tool above: one + // agent, two observations of it. + const liveChild = task('running', { + id: 'agent-live-child', + label: 'Nested agent', + toolUseId: 'call-nested', + parentAgentId: rootTask.id, + }); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + , + ); + }); + + const buildNode = container + .querySelector('[data-plan-node-id="build"]') + ?.closest('article'); + expect(buildNode?.textContent).toContain('2 agents'); + // Root row plus one nested row: the tally and the rows agree because they + // dedup the same way. + expect(buildNode?.textContent).toContain('Nested agent'); + expect( + buildNode?.querySelectorAll(`.${styles.nestedExecution}`), + ).toHaveLength(1); + + act(() => root.unmount()); + container.remove(); + }); + + it('renders no agent tally for a non-agent tool tagged to the step', () => { + // Declaring todo_id does not make a tool an agent: the tally hides + // rather than printing "0 agents" beside the tool's row. + const editTool: ACPToolCall = { + callId: 'call-edit', + toolName: 'edit', + title: 'Edit package.json', + status: 'completed', + args: { todo_id: 'verify' }, + }; + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + act(() => { + root.render( + + + , + ); + }); + + const verifyNode = container + .querySelector('[data-plan-node-id="verify"]') + ?.closest('article'); + expect(verifyNode?.textContent).toContain('Edit package.json'); + expect(verifyNode?.textContent).not.toContain('agent'); + + act(() => root.unmount()); + container.remove(); + }); + it('skips SVG edge materialization for an excessively dense plan', () => { const denseTodos = Array.from( { length: 33 }, @@ -1358,6 +2116,13 @@ describe('PlanExecutionView', () => { ), }), }), + ).map((todo) => + // blockedBy is model-authored and can repeat an id — or name the todo + // itself; the chip row must dedup repeats and drop self-references + // like the topology builder does. + todo.id === 'dense-1' + ? { ...todo, blockedBy: ['dense-0', 'dense-0', 'dense-1'] } + : todo, ); const container = document.createElement('div'); document.body.appendChild(container); @@ -1372,13 +2137,33 @@ describe('PlanExecutionView', () => { expect(container.querySelector('[data-plan-workflow]')).not.toBeNull(); expect(container.querySelectorAll('[data-plan-edge]')).toHaveLength(0); - expect( - container.querySelectorAll('[data-plan-input], [data-plan-output]'), - ).toHaveLength(0); + expect(container.querySelectorAll('[data-plan-output]')).toHaveLength(0); expect(container.textContent).toContain('Dense 32'); // Lines disappearing with no explanation reads as a broken render, so the // skip is stated rather than silent. expect(container.textContent).toContain('Too many dependencies to draw'); + // With no edges drawn, the node's dependency row is the only statement of + // the dependency, so it must survive here even though a node whose edges + // ARE drawn drops it. Each reference reads as the step it names — number + // and title, matching the inspector — not as a raw id. + const denseNode = container + .querySelector('[data-plan-node-id="dense-1"]') + ?.closest('article'); + expect(denseNode?.textContent).toContain('Depends on'); + expect(denseNode?.textContent).toContain('Dense 0'); + // The repeated id renders one chip, not two. + expect((denseNode?.textContent ?? '').split('Dense 0').length - 1).toBe(1); + // A self-reference never renders the step as its own blocker either: the + // node's own title is the only 'Dense 1' in its text. + expect((denseNode?.textContent ?? '').split('Dense 1').length - 1).toBe(1); + // The truncation rule targets `.dependencyTitle`; pin the class wiring + // so a dropped className cannot re-clip titles while text assertions + // stay green. + expect( + Array.from(denseNode?.querySelectorAll('span') ?? []).some((span) => + span.classList.contains(styles.dependencyTitle), + ), + ).toBe(true); act(() => root.unmount()); container.remove(); diff --git a/packages/web-shell/client/components/messages/PlanExecutionView.tsx b/packages/web-shell/client/components/messages/PlanExecutionView.tsx index cae98502b47..7a2764ca6c5 100644 --- a/packages/web-shell/client/components/messages/PlanExecutionView.tsx +++ b/packages/web-shell/client/components/messages/PlanExecutionView.tsx @@ -62,6 +62,12 @@ const EMPTY_GRAPH_LAYOUT: PlanGraphLayout = { const EDGE_LANE_HEIGHT = 9; /** Corner radius where an orthogonal edge turns. */ const EDGE_CORNER = 6; +/** + * Widest horizontal shoulder a layer-spanning edge runs before turning down + * into its return lane. It is a ceiling, never a floor: a narrow gutter takes + * half of its own run instead. See the router for why. + */ +const EDGE_SHOULDER = 24; const MAX_RENDERED_PLAN_EDGES = 500; @@ -560,6 +566,7 @@ export function PlanExecutionView({ // rather than defeating itself on fresh array identities. const { todosById, + stepNumberByTodo, toolsByTodo, unassigned, statesByTodo, @@ -579,6 +586,12 @@ export function PlanExecutionView({ } = useMemo(() => { const knownIds = new Set(todos.map((todo) => todo.id)); const todosById = new Map(todos.map((todo) => [todo.id, todo])); + // The step number addresses a step in the inspector list and in the + // dependency chips, so the graph shows the same number or the three + // surfaces name the same step differently. + const stepNumberByTodo = new Map( + todos.map((todo, index) => [todo.id, index + 1]), + ); const toolsByTodo = new Map(); const unassigned: ACPToolCall[] = []; for (const tool of tools) { @@ -655,6 +668,7 @@ export function PlanExecutionView({ } return { todosById, + stepNumberByTodo, toolsByTodo, unassigned, statesByTodo, @@ -783,6 +797,9 @@ export function PlanExecutionView({ ? graphRect.height / graphElement.offsetHeight : 1; const measuredNodes = new Map(); + // Horizontal extent of every topological layer, so an edge that skips + // a layer can size its shoulders from the gutter it really crosses. + const layerBounds = new Map(); let maxNodeBottom = 0; for (const [todoId, node] of nodeRefs.current) { const rect = node.getBoundingClientRect(); @@ -797,6 +814,17 @@ export function PlanExecutionView({ } as DOMRect; measuredNodes.set(todoId, normalizedRect); maxNodeBottom = Math.max(maxNodeBottom, normalizedRect.bottom); + const layer = layerByTodoRef.current.get(todoId) ?? 0; + const bounds = layerBounds.get(layer); + if (bounds) { + bounds.left = Math.min(bounds.left, normalizedRect.left); + bounds.right = Math.max(bounds.right, normalizedRect.right); + } else { + layerBounds.set(layer, { + left: normalizedRect.left, + right: normalizedRect.right, + }); + } } const edges: PlanEdgePath[] = []; const spanning: Array<{ @@ -833,7 +861,16 @@ export function PlanExecutionView({ }); continue; } - const controlX = startX + Math.max(24, (endX - startX) / 2); + // Half the run, not the old fixed 24px floor. The ≤720px gutter + // leaves a 24px run and the ≤480px gutter a 10px one, so a 24px + // shoulder put the control point exactly on the end point (zero + // tangent) or past it (negative tangent) — and `orient="auto"` then + // flips the arrowhead back at its source, which is the only + // direction cue left now that the input port is gone. Every gutter + // wide enough to afford the floor already resolved to run / 2, so + // wide lanes keep their exact curve while the end tangent's x stays + // strictly positive at every tier. + const controlX = startX + (endX - startX) / 2; edges.push({ from: dependencyId, to: todoId, @@ -851,8 +888,40 @@ export function PlanExecutionView({ maxNodeBottom + 14 + lane * EDGE_LANE_HEIGHT, Math.max(graphHeight - 6, maxNodeBottom + 14), ); - const dropX = edge.startX + 24; - const riseX = edge.endX - 24; + // Shoulders derived from the gutter each side actually has, the same + // way the adjacent-layer edge already does it — not a fixed 24px. The + // ≤480px gutter is 18px, so a 24px shoulder left a 10px run and put + // the vertical segment inside the intervening lane: a dependency from + // step 3 to step 5 looked like it entered step 4, and the SVG paints + // under the nodes so the lane just vanished into it. Halving the run + // centres the segment in its own gutter. EDGE_SHOULDER still wins + // wherever the gutter can afford it (the 64px desktop tier has a 56px + // run), so wide lanes keep the exact curve they had. + const sourceLayer = layerByTodoRef.current.get(edge.from) ?? 0; + const targetLayer = layerByTodoRef.current.get(edge.to) ?? 0; + // An unmeasured neighbouring layer falls back to the full shoulder. + const nextLayerLeft = + layerBounds.get(sourceLayer + 1)?.left ?? Number.POSITIVE_INFINITY; + const prevLayerRight = + layerBounds.get(targetLayer - 1)?.right ?? Number.NEGATIVE_INFINITY; + const dropShoulder = Math.min( + EDGE_SHOULDER, + (nextLayerLeft - 4 - edge.startX) / 2, + ); + const riseShoulder = Math.min( + EDGE_SHOULDER, + (edge.endX - prevLayerRight - 4) / 2, + ); + // The corner has to fit inside both shoulders. At 18px the shoulder + // is 5, so the unhalved 6px radius left a zero-length final segment + // and the arrowhead lost its direction again. + const corner = Math.min( + EDGE_CORNER, + dropShoulder / 2, + riseShoulder / 2, + ); + const dropX = edge.startX + dropShoulder; + const riseX = edge.endX - riseShoulder; const down = routeY > edge.startY ? 1 : -1; const up = edge.endY > routeY ? 1 : -1; edges.push({ @@ -860,14 +929,14 @@ export function PlanExecutionView({ to: edge.to, d: `M ${edge.startX} ${edge.startY} ` + - `H ${dropX - EDGE_CORNER} ` + - `Q ${dropX} ${edge.startY} ${dropX} ${edge.startY + EDGE_CORNER * down} ` + - `V ${routeY - EDGE_CORNER * down} ` + - `Q ${dropX} ${routeY} ${dropX + EDGE_CORNER} ${routeY} ` + - `H ${riseX - EDGE_CORNER} ` + - `Q ${riseX} ${routeY} ${riseX} ${routeY + EDGE_CORNER * up} ` + - `V ${edge.endY - EDGE_CORNER * up} ` + - `Q ${riseX} ${edge.endY} ${riseX + EDGE_CORNER} ${edge.endY} ` + + `H ${dropX - corner} ` + + `Q ${dropX} ${edge.startY} ${dropX} ${edge.startY + corner * down} ` + + `V ${routeY - corner * down} ` + + `Q ${dropX} ${routeY} ${dropX + corner} ${routeY} ` + + `H ${riseX - corner} ` + + `Q ${riseX} ${routeY} ${riseX} ${routeY + corner * up} ` + + `V ${edge.endY - corner * up} ` + + `Q ${riseX} ${edge.endY} ${riseX + corner} ${edge.endY} ` + `H ${edge.endX}`, }); }); @@ -936,6 +1005,12 @@ export function PlanExecutionView({ const selectedDependents = selectedTodo ? (dependentsByTodo.get(selectedTodo.id) ?? []) : []; + // The same filtered projection the edges draw from: the topology builder + // drops ids that name no step (and self-references), so no control here + // can select a ghost id and hide this panel mid-navigation. + const selectedDependencies = selectedTodo + ? (dependencyIdsByTodo.get(selectedTodo.id) ?? []) + : []; const detailsId = `plan-step-details-${graphId}`; const overallProgressId = `plan-overall-progress-${graphId}`; @@ -1183,7 +1258,12 @@ export function PlanExecutionView({ ref={hasDependencies ? graphRef : undefined} style={ hasDependencies - ? ({ '--plan-edge-lanes': graph.lanes } as CSSProperties) + ? ({ + '--plan-edge-lanes': graph.lanes, + // Publish the lane pitch so .dagCanvas reserves bottom + // padding from the same constant that places the lanes. + '--plan-edge-lane-height': `${EDGE_LANE_HEIGHT}px`, + } as CSSProperties) : undefined } > @@ -1254,10 +1334,63 @@ export function PlanExecutionView({ {layer.map((todo) => { const executions = toolsByTodo.get(todo.id) ?? []; const state = statesByTodo.get(todo.id)!; + // Agent time this step has taken, summed across its root + // agent tasks. It is the node's "is this alive" signal, so it + // is on the face rather than only in the inspector. + const nodeRuntimeMs = executions.reduce( + (total, tool) => + total + (taskForTool(tool, taskIndex)?.runtimeMs ?? 0), + 0, + ); + // Agents, not bare executions: a nested subagent counts too, + // matching the rows this node renders and the inspector's + // Subagents list for the same step. The runtime above stays + // on roots, so nested time is not summed twice. Count from + // the same two sources renderExecution draws the rows from — + // live child tasks from the task index plus transcript + // subTools, deduped by toolUseId exactly as the rows are — + // otherwise a live child task with no transcript entry + // renders a row the count never sees. + const agentCount = executions.reduce((count, tool) => { + const liveNested = nestedTasksFromIndex(tool, taskIndex); + const liveCallIds = new Set( + liveNested.flatMap(({ task }) => + task.toolUseId ? [task.toolUseId] : [], + ), + ); + const transcriptOnly = nestedAgentToolsForTool(tool).filter( + ({ tool: nested }) => !liveCallIds.has(nested.callId), + ); + return ( + count + + (isSubAgentToolCall(tool) ? 1 : 0) + + liveNested.length + + transcriptOnly.length + ); + }, 0); + // blockedBy is model-authored and can repeat an id — or name + // the todo itself. Dedup and drop self-references like the + // topology builder; ghost ids stay, because above the edge + // budget this row is the dependency's only statement. + const faceDependencies = [ + ...new Set(todo.blockedBy ?? []), + ].filter((id) => id !== todo.id); + // Whether the visible chip row carries the dependency. It is + // rendered whenever nothing else can: drawn edges are + // aria-hidden, so they state it only visually; above + // MAX_RENDERED_PLAN_EDGES no edges draw at all; and with the + // details panel off (the cockpit) or selection disabled + // (document mode) the panel never states it either. When this + // is false the sr-only summary below carries the same fact to + // assistive tech instead, so the dependency is stated exactly + // once either way. + const statesDependenciesVisibly = + !drawsDependencyEdges || !showStepDetails || documentMode; return (
setHoveredTodoId(todo.id)} onPointerLeave={() => setHoveredTodoId((current) => @@ -1272,11 +1405,11 @@ export function PlanExecutionView({ current === todo.id ? undefined : current, ) } - data-plan-input={ - (drawsDependencyEdges && - (dependencyIdsByTodo.get(todo.id)?.length ?? 0) > 0) || - undefined - } + // No input port: the left edge now carries the status + // rule, and an incoming edge already terminates in an + // arrowhead at the node — that arrowhead is the input + // marker. Outgoing edges leave their source unmarked, so + // the output port stays. data-plan-output={ (drawsDependencyEdges && (dependentsByTodo.get(todo.id)?.length ?? 0) > 0) || @@ -1317,29 +1450,96 @@ export function PlanExecutionView({ } disabled={documentMode} > + {/* Status reaches assistive tech as words; the left + rule that carries it visually is colour only. + Attention is announced beside the status word, + never instead of it. */} + + {t(statusKey(state.status))} + {state.attention + ? `, ${t('planExecution.attention')}` + : ''} +
+ + {(stepNumberByTodo.get(todo.id) ?? 0) || ''} + + + {todo.content} + +
+
+ {/* The glyph is the non-colour status channel, kept + for every status so the graph still survives + colour-blindness, high-contrast mode and a + greyscale screenshot. It moved off the first line + so the step's content leads, and it is muted so + the left rule remains the only carrier of the + status *colour*. */} - {todo.id} - - {t(statusKey(state.status))} - + {/* Attention's shape channel: on a paused node the + data-attention rule re-declares the token paused + already wears, so colour alone cannot tell it + apart from healthy. */} {state.attention && ( - - {t('planExecution.attention')} + + )} + {agentCount > 0 && ( + + {t('planExecution.agentCount', { + count: agentCount, + })} )} + {nodeRuntimeMs > 0 && ( + {formatRuntime(nodeRuntimeMs)} + )}
-
{todo.content}
- {(todo.blockedBy?.length ?? 0) > 0 && ( -
- {t('planExecution.dependsOn')}{' '} - {todo.blockedBy!.join(', ')} -
- )} + {statesDependenciesVisibly && + faceDependencies.length > 0 && ( +
+ {t('planExecution.dependsOn')} + {faceDependencies.map((id) => ( + + + {(stepNumberByTodo.get(id) ?? 0) || '?'} + + + {todosById.get(id)?.content ?? id} + + + ))} +
+ )} + {/* The interactive graph draws the dependency instead + of stating it, and drawn edges are aria-hidden — so + without this the node's accessible name stops + naming its blockers and a screen-reader user has to + activate every node to find out what blocks it. + Same sr-only channel as the status word, same + step-number-plus-title labels as the chips, and it + never brings the visible row back. */} + {!statesDependenciesVisibly && + faceDependencies.length > 0 && ( + + {t('planExecution.dependsOn')}{' '} + {faceDependencies + .map( + (id) => + `${(stepNumberByTodo.get(id) ?? 0) || '?'} ${ + todosById.get(id)?.content ?? id + }`, + ) + .join(', ')} + + )} {executions.length > 0 && (
@@ -1375,15 +1575,50 @@ export function PlanExecutionView({ )}
{selectedTodo.content}
- {(selectedTodo.blockedBy?.length ?? 0) > 0 && ( + {/* This panel is outside the node's own button, so unlike the + chips on the node face these references can be controls: each + one selects the step it names, which is what makes the + dependency list the graph's navigation. */} + {selectedDependencies.length > 0 && (
- {t('planExecution.dependsOn')}{' '} - {selectedTodo.blockedBy!.join(', ')} + {t('planExecution.dependsOn')} + {selectedDependencies.map((id) => ( + + ))}
)} {selectedDependents.length > 0 && (
- {t('planExecution.unblocks')} {selectedDependents.join(', ')} + {t('planExecution.unblocks')} + {selectedDependents.map((id) => ( + + ))}
)} {selectedExecutions.length > 0 && ( diff --git a/packages/web-shell/client/components/workflow/SessionWorkflowCockpit.module.css b/packages/web-shell/client/components/workflow/SessionWorkflowCockpit.module.css index a48bf34c6b7..953a487da93 100644 --- a/packages/web-shell/client/components/workflow/SessionWorkflowCockpit.module.css +++ b/packages/web-shell/client/components/workflow/SessionWorkflowCockpit.module.css @@ -47,12 +47,31 @@ min-width: 0; } -.identity span { - color: var(--agent-blue-500); - font-size: 10px; - font-weight: 800; - letter-spacing: 0.1em; - text-transform: uppercase; +/* The accent is the selection colour the graph below uses to say "this node". + Spending it on a fixed label above the title made the one blue thing on the + page the one thing that never changes, so the label drops to the subtitle + line and the accent is left to mean selection. */ +.identity small { + display: flex; + flex-wrap: wrap; + align-items: baseline; + gap: 2px 10px; +} + +.identity small > span { + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.workspaceName { + max-width: 28ch; +} + +/* The short session id is an address the user copies or matches against a + daemon log, so it keeps the mono face the rest of the line does not need. */ +.sessionRef { + font-family: var(--font-mono, monospace); } /* preflight.css resets headings to `font: inherit`, so size and weight both @@ -60,7 +79,7 @@ `min-width: 0` already caps the width; a fixed max-width only stranded space on wide viewports. */ .identity h1 { - margin: 4px 0 3px; + margin: 0 0 3px; overflow: hidden; font-size: clamp(20px, 2.2vw, 28px); font-weight: 600; @@ -71,7 +90,6 @@ .identity small { color: var(--muted-foreground); - font-family: var(--font-mono, monospace); font-size: 11px; } diff --git a/packages/web-shell/client/components/workflow/SessionWorkflowCockpit.tsx b/packages/web-shell/client/components/workflow/SessionWorkflowCockpit.tsx index 15ed8ef087e..df7dd7ae931 100644 --- a/packages/web-shell/client/components/workflow/SessionWorkflowCockpit.tsx +++ b/packages/web-shell/client/components/workflow/SessionWorkflowCockpit.tsx @@ -88,14 +88,19 @@ export function SessionWorkflowCockpit({ {t('workflow.chatTitle')}
- {t('workflow.inspector.graphCanvas')}

{sessionName || t('workflow.session.defaultTitle')}

- {sessionId.slice(0, 8)} ·{' '} - {workspaceCwd?.split('/').at(-1) || - t('workflow.session.workspace')} + {t('workflow.inspector.graphCanvas')} + + {workspaceCwd?.split('/').at(-1) || + t('workflow.session.workspace')} + + {sessionId.slice(0, 8)}
diff --git a/packages/web-shell/client/components/workflow/SessionWorkflowInspector.css.test.ts b/packages/web-shell/client/components/workflow/SessionWorkflowInspector.css.test.ts index 665e50aaccd..ebc2388cb0c 100644 --- a/packages/web-shell/client/components/workflow/SessionWorkflowInspector.css.test.ts +++ b/packages/web-shell/client/components/workflow/SessionWorkflowInspector.css.test.ts @@ -29,4 +29,44 @@ describe('SessionWorkflowInspector stylesheet', () => { expect(inspectorCss).not.toMatch(/\.summaryHeading\s+span/); expect(inspectorCss).toMatch(/\.summaryHeading\s*>\s*div\s+span/); }); + + // `.activityList > button` (0,1,1) declares the four-column activity grid + // for every direct-child button. A control styled with a bare class (0,1,0) + // loses to it and has its label auto-placed into the 52px first column, + // wrapping to three or four lines with three empty columns beside it — + // invisible to jsdom, to the visuals suite and to a green unit run, so it + // is pinned here at the source. + it('lets the show-all control out-specify the activity grid', () => { + expect(inspectorCss).toMatch( + /\.activityList\s*>\s*button\.showAllActivity\s*\{/, + ); + expect(inspectorCss).not.toMatch(/(^|\n)\.showAllActivity\s*\{/); + const rule = inspectorCss.match( + /\.activityList\s*>\s*button\.showAllActivity\s*\{[^}]*\}/, + )?.[0]; + // It must actually undo the inherited grid, not merely win the cascade. + expect(rule).toMatch(/display:\s*block/); + expect(rule).toMatch(/grid-template-columns:\s*none/); + }); + + // Same collision, already fixed for the wrapping metrics row: keep both + // pinned so neither regresses to a bare class. + it('lets the metrics row out-specify its single-line row styles', () => { + expect(inspectorCss).toMatch(/\.linkedAgents\s+button\s+small\.metrics/); + expect(inspectorCss).toMatch(/\.deliverables\s+button\s+small\.metrics/); + }); + + // The dependency chip's parent
  • is the flex item of .dependencyList; + // its default min-width: auto floored it at the title's full nowrap + // width, so the chip grew out of its column and the title's ellipsis + // never engaged. jsdom computes no layout, so the shrink floor is pinned + // at the source. + it('lets the dependency list item shrink so the chip ellipsis engages', () => { + const rule = inspectorCss.match( + /\.dependencyList\s*>\s*li\s*\{[^}]*\}/, + )?.[0]; + expect(rule).toBeTruthy(); + expect(rule).toMatch(/min-width:\s*0/); + expect(rule).toMatch(/max-width:\s*100%/); + }); }); diff --git a/packages/web-shell/client/components/workflow/SessionWorkflowInspector.module.css b/packages/web-shell/client/components/workflow/SessionWorkflowInspector.module.css index babf30eda37..8811c1b85f7 100644 --- a/packages/web-shell/client/components/workflow/SessionWorkflowInspector.module.css +++ b/packages/web-shell/client/components/workflow/SessionWorkflowInspector.module.css @@ -49,35 +49,39 @@ gap: 3px; } +/* Section labels name what follows; they are not decoration. Tracked-out + uppercase made every one of them shout over the content it introduces, so + they read as chrome. Sentence case at a muted weight puts the emphasis back + on the step title and the status beside it. Kept scoped to the heading's + inner div so the count beside it keeps its own tone. */ .summaryHeading > div span, .sectionHeading > div > span { color: var(--muted-foreground); font-size: 11px; - font-weight: 700; - letter-spacing: 0.06em; - text-transform: uppercase; + font-weight: 500; } .summaryHeading strong { font-size: 16px; } +/* Completed-over-total is a reading of the progress bar directly beneath it, + and the status word sits to its left, so a third tinted surface said the + same thing a third time. The count keeps the tone as ink and drops the + pill. */ .summaryCount { - padding: 5px 8px; - border-radius: 999px; color: var(--status-running-fg); - background: var(--status-running-bg); - font-size: 12px; + font-size: 13px; + font-variant-numeric: tabular-nums; + font-weight: 600; } .summaryCount[data-status='attention'] { color: var(--status-attention-fg); - background: var(--status-attention-bg); } .summaryCount[data-status='completed'] { color: var(--status-done-fg); - background: var(--status-done-bg); } .progress { @@ -281,6 +285,92 @@ overflow-wrap: anywhere; } +/* A dependency is an edge in the graph beside it, so it is rendered as the + step it points at — number and title — and selects that step on click. The + previous comma-joined id list made the reader find the row by eye. */ +.dependencyList { + display: flex; + flex-wrap: wrap; + gap: 4px; + margin: 0; + padding: 0; + list-style: none; +} + +/* The chip's parent
  • is the flex item, and its default min-width: auto + floors it at the title's full nowrap width — the chip grew out of its + column and the title's ellipsis never engaged. The number badge is kept + out of the shrink by its own flex: none. */ +.dependencyList > li { + min-width: 0; + max-width: 100%; +} + +.dependencyLink { + display: flex; + max-width: 100%; + align-items: center; + gap: 6px; + padding: 3px 7px 3px 4px; + border: 1px solid var(--border); + border-radius: var(--radius-md); + cursor: pointer; + font-size: 11px; + text-align: left; +} + +.dependencyLink > span:last-child { + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.dependencyLink:hover { + border-color: color-mix(in srgb, var(--agent-blue-500) 45%, var(--border)); + background: var(--accent); +} + +.dependencyNumber { + display: grid; + width: 16px; + height: 16px; + flex: none; + place-items: center; + border-radius: var(--radius-sm, 4px); + color: var(--muted-foreground); + background: var(--muted); + font-size: 10px; + font-variant-numeric: tabular-nums; +} + +/* The summary beside the list reports the true total, so the preview cap + needs a way out of it rather than a silent truncation. + + Specificity note, same class of collision as `.metrics` above: this button + is a DIRECT CHILD of `.activityList`, whose base rule + `.activityList > button` (0,1,1) declares the four-column activity grid + (`52px 26px minmax(0, 1fr) auto`) and `padding: 7px 4px`. A single-class + rule loses to it, and the label — a bare text run that Grid wraps into an + anonymous item — would auto-place into the 52px first column and wrap to + three or four lines with three empty columns beside it. So this rule + carries the same `.activityList >` prefix, and resets `display` and + `grid-template-columns` explicitly rather than relying on order. */ +.activityList > button.showAllActivity { + display: block; + grid-template-columns: none; + padding: 7px 8px; + border-radius: var(--radius-md); + color: var(--muted-foreground); + cursor: pointer; + font-size: 11px; + text-align: left; +} + +.activityList > button.showAllActivity:hover { + color: var(--foreground); + background: var(--accent); +} + .linkedAgents { margin-top: 14px; padding-top: 12px; @@ -436,6 +526,22 @@ font-size: 11px; } +/* Runtime, tool calls and tokens are separate facts, so they are separate + elements with real spacing rather than one string glued with middle dots — + which also let them wrap as a unit and be read out as one run of text. */ +/* Specificity note: `.linkedAgents button small` (0,1,2) and + `.deliverables small` (0,1,1) both declare the single-line ellipsis shape + these rows use, so the wrapping metrics row has to out-specify them rather + than rely on source order. */ +.linkedAgents button small.metrics, +.deliverables button small.metrics { + display: flex; + overflow: visible; + flex-wrap: wrap; + gap: 2px 10px; + white-space: normal; +} + .stateLabel { color: var(--muted-foreground); font-size: 11px; diff --git a/packages/web-shell/client/components/workflow/SessionWorkflowInspector.test.tsx b/packages/web-shell/client/components/workflow/SessionWorkflowInspector.test.tsx index f9666140d69..200ba61e4d4 100644 --- a/packages/web-shell/client/components/workflow/SessionWorkflowInspector.test.tsx +++ b/packages/web-shell/client/components/workflow/SessionWorkflowInspector.test.tsx @@ -223,4 +223,242 @@ describe('SessionWorkflowInspector', () => { act(() => root.unmount()); container.remove(); }); + + it('selects the referenced step from a dependency, by number and title', () => { + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + const onSelectedTodoIdChange = vi.fn(); + const todos: TodoItem[] = [ + { id: 'prepare', content: 'Prepare inputs', status: 'completed' }, + { + id: 'ship', + content: 'Ship result', + status: 'in_progress', + blockedBy: ['prepare'], + }, + ]; + + act(() => { + root.render( + + + , + ); + }); + + const upstream = container.querySelector( + '[data-testid="workflow-dependency-prepare"]', + ); + // The reference reads as the step it points at — the step number the list + // and graph both show, plus its title — not the bare `prepare` id. + expect(upstream).toBeTruthy(); + expect(upstream?.textContent).toContain('1'); + expect(upstream?.textContent).toContain('Prepare inputs'); + // The host keyboard handlers isolate plan controls through this marker + // (TasksStatusMessage/ToolApproval early-return on it), so these + // controls must carry it like the graph's own buttons do. + expect(upstream?.hasAttribute('data-plan-interactive')).toBe(true); + + onSelectedTodoIdChange.mockClear(); + act(() => upstream?.click()); + expect(onSelectedTodoIdChange).toHaveBeenCalledWith('prepare'); + + // Downstream too: the two directions render through the same helper but + // read from different sources (`blockedBy` vs the projection's + // `dependentsByTodo`), so covering one does not cover the other. + act(() => { + root.render( + + + , + ); + }); + const downstream = container.querySelector( + '[data-testid="workflow-dependency-ship"]', + ); + expect(downstream?.textContent).toContain('2'); + expect(downstream?.textContent).toContain('Ship result'); + expect(downstream?.hasAttribute('data-plan-interactive')).toBe(true); + onSelectedTodoIdChange.mockClear(); + act(() => downstream?.click()); + expect(onSelectedTodoIdChange).toHaveBeenCalledWith('ship'); + + act(() => root.unmount()); + container.remove(); + }); + + it('dedups a repeated dependency id in the upstream list', () => { + // blockedBy is model-authored and can repeat an id; without the dedup + // the list rendered two links sharing one key. + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + const todos: TodoItem[] = [ + { id: 'prepare', content: 'Prepare inputs', status: 'completed' }, + { + id: 'ship', + content: 'Ship result', + status: 'in_progress', + blockedBy: ['prepare', 'prepare'], + }, + ]; + + act(() => { + root.render( + + + , + ); + }); + + expect( + container.querySelectorAll('[data-testid="workflow-dependency-prepare"]'), + ).toHaveLength(1); + + act(() => root.unmount()); + container.remove(); + }); + + it('drops a self-reference from the upstream list', () => { + // The projection's dependentsByTodo builder skips self-blocks + // (session-workflow-model.ts), and the DAG draws no edge for them; the + // inspector's upstream filter must apply the same rule, or a todo + // naming itself in blockedBy renders a chip that only re-selects the + // already-selected step. + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + const todos: TodoItem[] = [ + { id: 'prepare', content: 'Prepare inputs', status: 'completed' }, + { + id: 'ship', + content: 'Ship result', + status: 'in_progress', + blockedBy: ['ship', 'prepare'], + }, + ]; + + act(() => { + root.render( + + + , + ); + }); + + expect( + container.querySelector('[data-testid="workflow-dependency-ship"]'), + ).toBeNull(); + expect( + container.querySelectorAll('[data-testid="workflow-dependency-prepare"]'), + ).toHaveLength(1); + + act(() => root.unmount()); + container.remove(); + }); + + it('keeps every activity row reachable past the preview cap', () => { + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + const todos: TodoItem[] = [ + { id: 'build', content: 'Build', status: 'in_progress' }, + ]; + const tools: ACPToolCall[] = Array.from({ length: 8 }, (_, index) => ({ + callId: `agent-${index}`, + toolName: 'Agent', + title: `Agent ${index}`, + status: 'completed' as const, + args: { todo_id: 'build' }, + })); + const tasks = tools.map((tool, index) => ({ + kind: 'agent' as const, + id: `task-${index}`, + label: `Agent ${index}`, + description: `Ran step ${index}`, + status: 'completed' as const, + startTime: 1_000 + index, + endTime: 2_000 + index, + runtimeMs: 1_000, + isBackgrounded: true, + toolUseId: tool.callId, + })); + + act(() => { + root.render( + + + , + ); + }); + + const activityList = () => + Array.from(container.querySelectorAll('button')).filter((button) => + /^\d{1,2}:\d{2}/.test(button.textContent?.trim() ?? ''), + ); + const beforeExpand = activityList().length; + const showAll = container.querySelector( + '[data-testid="workflow-activity-show-all"]', + ); + // The count beside the list reports the true total, so the cap has to be + // an invitation rather than a silent truncation. + expect(beforeExpand).toBe(6); + expect(showAll?.textContent).toContain('8'); + // Same host-keyboard isolation as the dependency links above. + expect(showAll?.hasAttribute('data-plan-interactive')).toBe(true); + + act(() => showAll?.click()); + expect(activityList().length).toBe(8); + expect( + container.querySelector('[data-testid="workflow-activity-show-all"]'), + ).toBeNull(); + + act(() => root.unmount()); + container.remove(); + }); }); diff --git a/packages/web-shell/client/components/workflow/SessionWorkflowInspector.tsx b/packages/web-shell/client/components/workflow/SessionWorkflowInspector.tsx index df6b2547d75..5684e2e64f5 100644 --- a/packages/web-shell/client/components/workflow/SessionWorkflowInspector.tsx +++ b/packages/web-shell/client/components/workflow/SessionWorkflowInspector.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo } from 'react'; +import { useEffect, useMemo, useState } from 'react'; import type { DaemonSessionArtifact, DaemonSessionTaskStatus, @@ -22,6 +22,12 @@ import { } from './session-workflow-model'; import styles from './SessionWorkflowInspector.module.css'; +/** + * Activity rows shown before the list asks to be expanded. The summary beside + * it reports the true total, so the remainder must stay reachable. + */ +const ACTIVITY_PREVIEW_COUNT = 6; + export interface SessionWorkflowInspectorProps { todos: readonly TodoItem[]; tools: readonly ACPToolCall[]; @@ -53,6 +59,15 @@ export function SessionWorkflowInspector({ [tasks, todos, tools], ); const defaultTodoId = getDefaultWorkflowTodoId(todos, projection); + // Dependencies are stated as Todo ids, which are addresses, not labels. The + // step number is what the list and the graph both show, so carry it into + // every dependency reference. + const stepNumberById = useMemo(() => { + const numbers = new Map(); + todos.forEach((todo, index) => numbers.set(todo.id, index + 1)); + return numbers; + }, [todos]); + const [showAllActivity, setShowAllActivity] = useState(false); const effectiveSelectedTodoId = projection.todosById.has(selectedTodoId ?? '') ? selectedTodoId : defaultTodoId; @@ -84,9 +99,16 @@ export function SessionWorkflowInspector({ const selectedTools = selectedTodo ? (projection.agentToolsByTodo.get(selectedTodo.id) ?? []) : []; - const upstream = selectedTodo?.blockedBy?.filter((id) => - projection.todosById.has(id), - ); + // blockedBy is model-authored and can repeat an id — or name the todo + // itself. Dedup before mapping so a repeated reference cannot emit a + // duplicate key or a second link, and drop self-references with the same + // rule the projection's dependentsByTodo builder applies, so the + // inspector never shows a step blocked by itself. + const upstream = selectedTodo + ? [...new Set(selectedTodo.blockedBy ?? [])].filter( + (id) => id !== selectedTodo.id && projection.todosById.has(id), + ) + : undefined; // The projection already derives this for the graph's edges; recomputing it // here rescanned every todo's `blockedBy` for the same answer. It also drops // a todo that lists itself in `blockedBy`, which the previous filter kept as @@ -95,6 +117,30 @@ export function SessionWorkflowInspector({ ? (projection.dependentsByTodo.get(selectedTodo.id) ?? []) : []; + // A dependency reference selects the step it names: the list of ids was + // read-only text, so following an edge meant finding the row by eye. + const dependencyLink = (todoId: string) => { + const target = projection.todosById.get(todoId); + const number = stepNumberById.get(todoId); + return ( +
  • + +
    • + ); + }; + const detail = selectedTodo && selectedState && (
    @@ -111,17 +157,25 @@ export function SessionWorkflowInspector({
    {t('workflow.dependencies.upstream')}
    - {upstream?.length - ? upstream.join(', ') - : t('workflow.dependencies.none')} + {upstream?.length ? ( +
      + {upstream.map((id) => dependencyLink(id))} +
    + ) : ( + t('workflow.dependencies.none') + )}
    {t('workflow.dependencies.unblocks')}
    - {downstream.length - ? downstream.map((todo) => todo.id).join(', ') - : t('workflow.dependencies.noDownstream')} + {downstream.length ? ( +
      + {downstream.map((todo) => dependencyLink(todo.id))} +
    + ) : ( + t('workflow.dependencies.noDownstream') + )}
    @@ -168,7 +222,13 @@ export function SessionWorkflowInspector({ task.description} )} - {metrics.length > 0 && {metrics.join(' · ')}} + {metrics.length > 0 && ( + + {metrics.map((metric) => ( + {metric} + ))} + + )} {task && ( @@ -314,7 +374,10 @@ export function SessionWorkflowInspector({ {projection.activity.length}
    - {projection.activity.slice(0, 6).map((task) => { + {(showAllActivity + ? projection.activity + : projection.activity.slice(0, ACTIVITY_PREVIEW_COUNT) + ).map((task) => { const tool = projection.toolsByTaskId.get(task.id); const at = task.endTime ?? task.startTime; const content = ( @@ -359,6 +422,20 @@ export function SessionWorkflowInspector({ {projection.activity.length === 0 && (

    {t('workflow.activity.empty')}

    )} + {!showAllActivity && + projection.activity.length > ACTIVITY_PREVIEW_COUNT && ( + + )}
    @@ -377,8 +454,9 @@ export function SessionWorkflowInspector({ > {artifact.title} - - {artifact.kind} · {artifact.status} + + {artifact.kind} + {artifact.status}