mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-11-08 17:41:36 -10:00
fix(web-shell): keep layer-skipping plan lanes out of the step they pass (#10938)
wenshao's round-2 verification names two merge conditions on 5f70a13.
At <=480px the router gave every layer-skipping edge a fixed 24px shoulder.
The 18px gutter leaves a 10px run, so the vertical segment landed inside the
intervening lane: a dependency from step 3 to step 5 read as one into step 4,
and the SVG paints under the nodes so the lane just disappeared into it.
Derive each shoulder from the gutter measured on that side, the way the
adjacent-layer edge already does, and halve the corner radius so the run into
the arrowhead keeps a positive length. The 64px desktop tier has a 56px run,
so min(24, 28) is still 24 and every desktop path stays byte-identical; the
new witness pins the exact desktop path string.
The node-face agent tally dedups by toolUseId exactly as the rows do, but
nothing exercised the overlap. Add a fixture where a live child task and a
transcript subTool share one toolUseId and assert it counts once (2 agents
beside 2 rows); dropping the filter reads 3 agents.
Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmtzjtfna1y
This commit is contained in:
jinjing.zzjandQwen-Coder committed
1 parent
5f70a13866
commit
d91a0f7e74
2 files changed
+232
-10
No files matched your search
@@ -1511,6 +1511,121 @@ describe('PlanExecutionView', () => {
|
||||
}
|
||||
});
|
||||
|
||||
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(
|
||||
<I18nProvider language="en">
|
||||
<PlanExecutionView todos={skip} tools={[]} tasks={[]} />
|
||||
</I18nProvider>,
|
||||
);
|
||||
});
|
||||
|
||||
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');
|
||||
@@ -1899,6 +2014,61 @@ describe('PlanExecutionView', () => {
|
||||
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(
|
||||
<I18nProvider language="en">
|
||||
<PlanExecutionView
|
||||
todos={todos}
|
||||
tools={[rootTool]}
|
||||
tasks={[rootTask, liveChild]}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
});
|
||||
|
||||
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.
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -791,6 +797,9 @@ export function PlanExecutionView({
|
||||
? graphRect.height / graphElement.offsetHeight
|
||||
: 1;
|
||||
const measuredNodes = new Map<string, DOMRect>();
|
||||
// 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<number, { left: number; right: number }>();
|
||||
let maxNodeBottom = 0;
|
||||
for (const [todoId, node] of nodeRefs.current) {
|
||||
const rect = node.getBoundingClientRect();
|
||||
@@ -805,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<{
|
||||
@@ -868,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({
|
||||
@@ -877,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}`,
|
||||
});
|
||||
});
|
||||
|
||||
Reference in new issue
Block a user