fix(desktop): correct Agent Graph collapse chevron direction - #3466
fix(desktop): correct Agent Graph collapse chevron direction#34661625567290 wants to merge 2 commits into
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — this is the smallest correct fix at the existing disclosure-state seam. I reviewed exact head e3225248f02d675029971801a9385482fb15a002; aria-expanded, rendered content, and the collapsed data attribute stay under one state owner, and the current-head test check is green. I found no P0–P2 issues; one optional visual-regression hardening note is inline.
AI-assisted review disclosure: OpenAI Codex performed an independent exact-head review. I verified the state/CSS mapping, focused tests, live review state, mergeability, and current-head CI, and I independently made the approval decision.
e322524 to
78911a8
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for chasing this down — the chevron pointing the wrong way is one of those things that quietly makes a panel feel broken, and the actual fix is exactly right: the rotation belongs on the collapsed state, not the expanded one.
My comments are entirely about the 122 lines of test added around that one-line change, not about the change.
The short version is that CI is currently red on this PR, and what is failing is the new e2e itself — electron e2e/agent-graph-chevron-smoke.mjs exits non-zero in the Desktop e2e job. So as it stands the instrument added to protect a one-line CSS fix is the thing keeping the fix out of the tree. That is worth weighing directly rather than debugging around.
There is also a deeper problem with that smoke test that I would raise even if it were green, and it is the one I would most like you to look at: it loads agent-graph.css on its own into a bare data: URL with a hand-written <button>, so it verifies the rule in isolation. The regression it exists to catch — this rule losing to something else, or the markup drifting — happens in the real cascade with the app's full stylesheet and the real component. A test that cannot fail for the scenario it is guarding against is not paying for its cost, and this one costs a real Electron process per run.
getComputedStyle(...).transform === 'matrix(-1, 0, 0, -1, 0, 0)' also pins Chromium's matrix serialization rather than the intent. Rewriting the rule as rotate(-180deg) would be visually identical and would fail this assertion.
I am not asking you to add heavier coverage — the opposite. For a one-line CSS direction fix, I think the honest options are to keep the small jsdom assertion, or to accept that this class of bug is caught by looking at it. Both seem better than an Electron process that is red today and blind to the real failure mode.
No [P0]/[P1] — the product change is correct.
Review assisted by AI (Claude Opus 5). Findings were verified against the files and the failing CI run at this head; the reviewer is accountable for them.
| nodeIntegration: false, | ||
| sandbox: true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
[P2] This is the finding I care most about, and it holds independently of the CI failure.
The page under test is a hand-written <button> plus agent-graph.css alone, injected into a data: URL. Nothing else from the app is present. So this asserts that the rule works when it is the only rule.
But the regression this is guarding against does not look like that. A chevron points the wrong way because the selector is wrong (what you just fixed), because another stylesheet later wins the cascade, or because the component stops emitting aria-expanded on that element. Only the first is visible here — and the first is already visible far more cheaply. The second and third are exactly what an end-to-end test is supposed to buy, and this one is blind to both: it never loads the real stylesheet set and never renders the real component.
So the test costs a full Electron process per run and covers the one failure mode that did not need it.
Separately, matrix(-1, 0, 0, -1, 0, 0) pins Chromium's serialization of the transform rather than the intent. rotate(-180deg) is the same picture and fails this assertion.
| const execFileAsync = promisify(execFile); | ||
| const electronPath = createRequire(import.meta.url)('electron') as string; | ||
|
|
||
| test('agent graph collapse chevron follows the disclosure state', async () => { |
There was a problem hiding this comment.
[P2] CI is red on this head and this is what is failing:
test Desktop e2e Error: Command failed: .../node_modules/electron/dist/electron e2e/agent-graph-chevron-smoke.mjs
1 failed
A real BrowserWindow on a headless CI runner is a materially different environment from your machine, and show: false does not remove the dependency on a working display/GPU path. Whatever the specific cause, the situation is that a one-line CSS fix cannot land because of the test written to protect it.
Worth stating plainly since it cuts against the usual instinct: dropping this spec is a legitimate resolution here, not a retreat. The cost of the instrument should stay proportional to what it protects, and one CSS declaration does not justify an Electron process in the critical path of every CI run.
| } | ||
|
|
||
| describe('AgentGraphPanel collapse', () => { | ||
| it('keeps the disclosure state and controlled content in sync', async () => { |
There was a problem hiding this comment.
[P3] This test is well written — it drives the real component through the existing harness and asserts aria-expanded, data-collapsed, and that the aria-controls target actually mounts and unmounts. If I had to keep exactly one of the three tests in this PR, it would be this one.
The note is only that none of the behaviour it covers is changed by this PR. Disclosure-state syncing worked before and works after; the diff to the product is one CSS selector. So a reviewer reading +122/-1 has to work out that 121 of those lines are unrelated coverage.
Not asking you to remove it — new coverage of untested behaviour is worth having. Just noting that it belongs in its own change, where it can be judged on its own merits rather than riding along with a chevron fix.
| const expandedAgain = await transformFor(true); | ||
| if ( | ||
| expanded !== 'none' || | ||
| collapsed !== 'matrix(-1, 0, 0, -1, 0, 0)' || |
There was a problem hiding this comment.
[P3] expandedAgain cannot differ from expanded. Between the two reads the only thing that changed is the attribute, which is set explicitly each time, and there is no component, no state, and no transition (--duration-quick is pinned to 0s above) that could carry anything across.
An assertion that cannot fail reads as coverage without being coverage.
2 participants
Summary
Testing
The concurrent Desktop suite reached 1059/1061. The unrelated Git Review and MCP runtime failures both passed when rerun independently.
Closes #3407.