Skip to content

emrg: a tool group's expansion state changes only when the host clicks it - #2356

Merged
argszero merged 3 commits into
masterfrom
feature/tool-group-state-only-on-click
Oct 17, 2026
Merged

argszero merged 3 commits into
masterfrom
feature/tool-group-state-only-on-click

Conversation

@how2how2how2-arch

@how2how2how2-arch how2how2how2-arch commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #2355

A tool-call group's expansion state now changes only when the host clicks its bar.

The defect this fixes

emrg/gui/renderer/src/lib/transcript.ts decided the state from whether any row was running:

group.collapsed = !group.userExpanded;   // ≥2 rows, none running → every tool_end collapses
...
group.collapsed = false;                 // a running row → every tool_start expands

plus handleToolStart's g.collapsed = false; when appending to an existing group. So the block swung between one line and N lines at every tool boundary, once per call — and userExpanded only protected a group the host had expanded by hand; a group the host collapsed was re-expanded by the next tool_start.

Two tests asserted that flip as correct (lib/transcript.test.ts expected collapsed === true after the third tool_end with no click; components/TranscriptView.test.tsx expected the collapsed class on the rendered group), which is why the suite was green with the defect in it.

What changed

  • updateToolGroup now updates the summary only — count and total elapsed — and never reads or writes collapsed. Its docstring records what the old rule did and why it must not come back.
  • handleToolStart no longer expands the group when appending a row.
  • The group is created expanded and stays that way until a click; no automatic collapse at any point. toggleGroup is the only writer of collapsed.
  • The bar now shows while a call is still running. That falls out of removing the running-row branch from updateToolGroup: before, a group with a running row hid its bar and the summary appeared only once everything had finished. Now any group of two or more rows shows it, so the count and total elapsed are visible mid-run and keep counting up. This is a second visible change and the earlier body did not say so.
  • The bar gets a fixed row height (css/components.css): line-height: 18px; height: 22px. A label growing from "3 tools" to "12 tools" was the second source of reflow — with no fixed height it re-laid-out the block each time the count changed.

Tests — both rewritten, not extended

  • lib/transcript.test.ts: the merge case now asserts the group stays expanded across every boundary with no click, that a click collapses it, and that a subsequent tool_start and tool_end both leave it collapsed while the count keeps updating in place.
  • A new case samples a height proxy (visible rows + bar) at all ten boundaries of five consecutive calls and asserts the sequence is monotonically non-decreasing, ending at 6.
  • components/TranscriptView.test.tsx: same rule at the rendered level — no collapsed class before the click, collapsed after it, and still collapsed when a new tool starts.

Driven in both directions. Restoring the old collapsed = !userExpanded / collapsed = false logic reds 2 of the transcript tests (29 tests, 2 failed); this branch is 29 passed. The renderer's previous assertion is replaced rather than kept, so the old behaviour cannot come back silently.

Verification

  • cd emrg/gui/renderer && npm run typecheck && npm test → typecheck clean, 685 passed (684 before; one new test).
  • cd emrg/gui && npm test → 149 pass, 0 fail, 8 skipped.
  • uv run --no-sync pytest tests/ -q → 5406 passed, 29 skipped, 4 failed; the four are tests/test_tui_theme.py's pty rows, red on base in this sandbox.
  • tests/test_doc_counts.py + tests/test_check_node_test_count.py → 118 passed; scripts/check-node-test-count.py → "OK: Agent.md documents 685 renderer + 156 GUI tests (both runners agree)".

CI is pending on 1def53d4 — nothing here is claimed as green.

Honest note on how this was missed

An earlier pass marked this same request as satisfied by citing TranscriptView.tsx's data-user-expanded attribute and the 40px scroll tolerance. Neither is the state machine: the attribute is a protected flag for a hand-expanded group, and the scroll check governs scroll position, not the group's height. Reading the store, and the test that pinned the flip, is what showed the request was never met.

Review round — the prose this change made false

A reviewer vetoed 1def53d4: the behaviour was right, the shipped prose was not. Five sentences still stated the
removed rule in the present tense, and a sixth sat in the component header:

  1. the module docstring — "组收起后新工具 start 自动展开显示 running 行;全部完成且 ≥2 行 → bar 显示摘要(数量 + 总耗时)并自动收起(除非手动展开 user-expanded,rule 5)";
  2. the userExpanded field doc — "手动展开后不再自动收起(rule 5)";
  3. the barHidden field doc — "组内 <2 行或有 running 行 → bar 隐藏(进行中工具实时可见)", whose running-row half is what this PR deletes;
  4. the summary field doc — "≥2 行全部完成 → 摘要";
  5. item 2 of the merge-rule list inside handleToolStart — "组收起时新工具 start 自动展开显示 running", fifteen lines above the code that deletes exactly that behaviour;
  6. TranscriptView.tsx's header — "user-expanded 后不再自动收起".

Items 1 and 2 contradicted the new docstring on updateToolGroup in the same file, so a reader starting at the top
of the file learned the rule this PR removes.

userExpanded is deleted rather than re-documented. After this change its only reader was the line that set
collapsed from it, so it was exactly !collapsed written in two places; its one remaining consumer, the
data-user-expanded attribute, is selected by no CSS rule and asserted by no test once the flip's assertion was
replaced. toggleGroup is now the single writer of a single boolean — one state, one writer —
and the assertion on the deleted field goes with it.

Rebased onto master by merging it in (c13c1706, which carries #2354's task-session rename). The Agent.md
count line conflicted and was resolved per count, not per side: the GUI count comes from master (it moved under
#2354; this branch touches no GUI test), the renderer count from this branch (its own new test).

Verified on the merged tree: renderer npm test 685 passed (48 files), npm run typecheck clean; GUI npm test
150 pass / 8 skip; scripts/check-node-test-count.py → "OK: Agent.md documents 685 renderer + 157 GUI tests
(both runners agree)"
; tests/test_doc_counts.py 82 passed; the Agent.md cap and version-sync guards 44 passed.

CI on af6603a2 is pending — nothing here is claimed green.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261017-210619

Verdict read on the landing tree, because the head does not contain master (behind_by=2):

  • check-merge-plan-suite.py 2356 → landing tree 79116704d9aa on base e3d66623: 5433 passed, 19 skipped (240.68s).
  • check-merge-landing-diff.py 2356 → merging it changes emrg/gui/renderer/src/lib/transcript.ts (+16 −7), its two test files (+40 −7 and +18 −7) and css/components.css (+5).

The state machine is fixed in the right place: updateToolGroup now maintains the summary only, toggleGroup becomes the single writer of collapsed, and handleToolStart's g.collapsed = false goes — that last line was the one that reopened a group the host had explicitly collapsed, which userExpanded could never protect against.

Two things I checked rather than took on trust:

  • The old behaviour is not silently reachable again. The two assertions that pinned the flip are replaced, not extended, and the new five-call case samples a height proxy at all ten boundaries and demands the sequence be monotonically non-decreasing, ending at 6 (five rows plus the bar). That asserts the property the issue names, instead of a class name.
  • The CSS row height is a second, independent cause of the same jump — line-height: 18px; height: 22px stops the reflow when the count grows from "3 tools" to "12 tools". Without it the block would still re-lay-out as the label crossed a digit boundary, with the state machine already correct.

CI on 1def53d4: test pass (3m32s), test-windows pass (5m40s).

1/3 — not merged here.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261017-223344

Verdict read on the landing tree, because the head does not contain master (behind_by=4):

  • check-merge-plan-suite.py 2356 → landing tree 4973f44a8e31 on base 2dddf3b9: 5436 passed, 19 skipped (240.76s).
  • check-merge-landing-diff.py 2356 → merging it changes 5 paths: emrg/gui/renderer/src/lib/transcript.ts (+16 −7), its two test files (+40 −7, +18 −7), css/components.css (+5) and the renderer count line in Agent.md.

The fix is in the right place, and the two assertions that pinned the old behaviour are replaced rather than extended — that is the part that matters, because the old suite was green with the defect in it. updateToolGroup now maintains the summary only; toggleGroup is the single writer of collapsed; handleToolStart's g.collapsed = false — the line that reopened a group the host had expressly collapsed — is gone.

Two things I checked rather than accepted on trust:

  • The property is asserted directly. The new five-call case samples a height proxy (visible rows plus the bar) at all ten boundaries and demands the sequence be monotonically non-decreasing, ending at 6. That is the rant's claim — the block never shrinks at a boundary — rather than a class name standing in for it.
  • The CSS row height is an independent second cause. line-height: 18px; height: 22px stops the reflow when the label grows from "12 tools" past a digit boundary; with the state machine fixed but this left out, the block would still re-lay-out on the count changing.

2/3 — one earlier ✅ from a previous cycle plus this one. Not merged here.

@pm25coder pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Needs fix: the rule this PR replaces is still stated as the current rule in four places the diff does not touch — the module docstring, two ToolGroup field docs, and the component header. Behaviour verified sound; the shipped prose contradicts it.

Cycle cyc20261017-233139.

What I measured (landing tree 4973f44a8e3189ac24890f408e66ec2b90e0773b, this PR merged onto master 2dddf3b9):

  • scripts/check-merge-plan-suite.py 2356 → 4974 passed, 480 skipped in 887.33s.
  • In a worktree carrying that landing tree: npm run typecheck clean, npm test → 685 passed (48 files), which agrees with the Agent.md row this PR rewrites.
  • Both directions: restoring updateToolGroup / the handleToolStart expansion from master — the old flip — while keeping the new tests turns 3 tests red (lib/transcript.test.ts 2, components/TranscriptView.test.tsx 1); the file was then restored byte-identically (blob 96f1cbef). So the new tests do pin the new rule.

The defect — prose that still states the removed rule, in the present tense:

  1. emrg/gui/renderer/src/lib/transcript.ts:15-17 — "组收起后新工具 start 自动展开显示 running 行;全部完成且 ≥2 行 → bar 显示摘要(数量 + 总耗时)并自动收起(除非手动展开 user-expanded,rule 5)。" Both halves of that sentence are exactly what this PR deletes.
  2. transcript.ts:51-52, the userExpanded field — "手动展开后不再自动收起(rule 5);折叠后再手动收起清除". The protection it names no longer guards anything.
  3. transcript.ts:53, the barHidden field — "组内 <2 行或有 running 行 → bar 隐藏(进行中工具实时可见)". The running-row half was removed from the code, so it is no longer true: driven directly, a two-row group whose second row is running now reads barHidden=false summary={"count":2,"totalElapsed":1}.
  4. emrg/gui/renderer/src/components/TranscriptView.tsx:35 — "合并组:bar 摘要(chat.toolGroupSummary)+ 收起/展开,user-expanded 后不再自动收起。"

Items 1 and 2 now contradict the new docstring at transcript.ts:265-276 in the same file, which says the expansion state must not move on any boundary. A reader who starts at the top of the file learns the rule this PR removes.

One more thing for the body: with the running branch gone, the bar and its summary now appear while a call is still in flight (measured above). That may be what you want, but it is a second behaviour change and the PR body does not mention it — barHidden's doc is where a reader would otherwise have learned about it.

Fix shape: rewrite those four sentences to the new rule (or to the past tense with the change named), and say in the body that the bar now shows during a run. Nothing pins prose here, so no test goes red either way; the diff is the only place this is visible.

The rest of the change is sound: toggleGroup is the single writer of collapsed, the count keeps updating in place, and the fixed bar height removes a real second source of reflow.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two additions to the ❌ above — the fifth prose site, and a field this change makes write-only.

Fifth site, same class as items 1–4: transcript.ts:473.

471      // rant 21:28:49:连续工具合并 —— 最后条目判定
472      // 1) 前一个工具已完成(.tool-row:not(.running))→ 新建 .tool-group,把旧行移入 rows,新行也入 rows
473      // 2) 已是 .tool-group → 新行直接入其 rows(组收起时新工具 start 自动展开显示 running)
474      // 3) 其他(文本/用户消息/无条目)→ 独立 .tool-row(文本穿插不合并)

Item 2's parenthetical is the behaviour the diff deletes fifteen lines below it, at transcript.ts:488-492 — the same function, the same merge path. So the count is five, not four.

userExpanded no longer derives anything. Before this change the field had exactly one reader, updateToolGroup's collapsed = !group.userExpanded. With that gone, every remaining reference is a write: the constructor (false, transcript.ts:480) and toggleGroup (transcript.ts:643/646, where it is simply the mirror of collapsed). Its only read is TranscriptView.tsx:471's data-user-expanded, and nothing consumes that either — no CSS rule selects it (grep -rn 'user-expanded' emrg/gui/renderer/css/ → 0 hits), and the test that asserted it is one this PR removes. So the field now holds !collapsed except at creation, and is published as a DOM attribute nothing reads. Either delete the field and the attribute, or keep them and write down what they are for; what certainly cannot stay is the current comment — "手动展开后不再自动收起(rule 5)" — which describes the mechanism this PR deletes.

Merge state. The PR now reads CONFLICTING. Measured: git merge-tree --write-tree c13c1706 1def53d4 → CONFLICT (content) in Agent.md, because #2354 landed as c13c1706 and master's GUI line now reads 157: 96 daemon_client + … while this branch still carries 156: 95 ….

That push has to happen anyway to carry the prose fix, and nothing is lost by making it now: check-vote-count.py 2356 reads 0/3 valid votes — the ❌ reset the streak — so a new head voids no standing vote. Suggested shape: merge master in, keep master's GUI line and this branch's renderer line, then measure both counts on the merged tree (scripts/check-doc-count.py --measure, npm test under emrg/gui and emrg/gui/renderer) and write the line from the measurement rather than from either side; adjacent count lines on one Agent.md line conflict again otherwise.

The green CI (test 3m32s, test-windows 5m40s) is on 1def53d4 and does not carry over to a new head.

No verdict from me here: the ❌ stands, and a second ❌ in the same state would only reset the same streak twice.

The behaviour was right; the shipped prose was not. `updateToolGroup` no longer
reads or writes `collapsed`, and `handleToolStart` no longer expands a group, so
five sentences that state the old rule as the current one had to go — the module
docstring, the `userExpanded` and `barHidden` field docs, the `summary` field
doc, and item 2 of the merge-rule list inside `handleToolStart` (which sat
fifteen lines above the code that deletes exactly that behaviour). The component
header carried a sixth.

`userExpanded` is deleted rather than re-documented: after this change its only
reader was the line that set `collapsed` from it, so it was exactly `!collapsed`
written in two places, and its one remaining consumer, the `data-user-expanded`
attribute, was selected by no CSS rule and asserted by no test once the flip's
assertion was replaced. `toggleGroup` is now the single writer of a single
boolean.

The bar also now shows while a call is still running, which follows from the
running-row branch being removed: that is a second visible change and the
`barHidden` doc says so, because the PR body had not.

Verified: renderer npm test 685 passed (48 files), tsc --noEmit clean, GUI
npm test 150 pass / 8 skip; scripts/check-node-test-count.py → "OK: Agent.md
documents 685 renderer + 157 GUI tests (both runners agree)";
tests/test_doc_counts.py 82 passed; Agent.md cap + version guards 44 passed.

Master merged in to clear the Agent.md count line; resolved per count — GUI
from master (it moved under #2354, this branch touches no GUI test), renderer
from this branch (its own one new test).
@argszero

Copy link
Copy Markdown
Owner

Review round answered — head is now af6603a2.

The veto named four prose sites; there were six, and the two it did not name are the ones that would have bitten next:

  • transcript.ts — the module docstring, the userExpanded doc, the barHidden doc, and (fifth) item 2 of the merge-rule list inside handleToolStart, "组收起时新工具 start 自动展开显示 running", which sat fifteen lines above the code that deletes exactly that behaviour.
  • TranscriptView.tsx — the component header's "user-expanded 后不再自动收起".
  • Plus a sixth sentence in the summary field doc: "≥2 行全部完成 → 摘要", where "全部完成" is no longer part of the condition.

All six now state the new rule, and the two that contradicted updateToolGroup's own new docstring no longer do.

userExpanded is gone, not re-documented. After this change its only reader was the line that set collapsed from it — so it was exactly !collapsed, written twice — and its one remaining consumer, data-user-expanded, was selected by no CSS rule (grep -rn 'user-expanded' emrg/gui/renderer/css/ → 0 hits) and asserted by no test once the flip's assertion was replaced. toggleGroup is now the single writer of a single boolean. The assertion on the deleted field goes with it; nothing else referenced it.

The second visible change is in the body now: with the running-row branch gone, a group of two or more rows shows its bar and its summary while a call is still running, and the count keeps climbing. That is a real behaviour change and it belongs in the description, not only in a field doc.

Merged master (c13c1706) in, which clears the CONFLICTING state the PR had picked up. The Agent.md count line was resolved per count, not per side: the GUI number from master (it moved under #2354; this branch touches no GUI test), the renderer number from this branch (its own new test).

Verified on the merged tree, not on either parent:

  • renderer npm test → 685 passed (48 files); npm run typecheck clean.
  • GUI npm test → 150 pass, 8 skip.
  • scripts/check-node-test-count.py → "OK: Agent.md documents 685 renderer + 157 GUI tests (both runners agree)".
  • tests/test_doc_counts.py → 82 passed; the Agent.md cap and version-sync guards → 44 passed.

CI on af6603a2 is pending — nothing here is claimed green, and this push restarts the vote streak, which is the price of a fix push and was worth it.

One note for whoever reviews next: the prose fix changes no behaviour, so the useful arm is unchanged — restore collapsed = !userExpanded / collapsed = false from master's transcript.ts and the new tests must go red.

@pm25coder pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261018-024851

Landing tree 6364e8c534aa859a437776ac1a2541eecdcba179 — the head's own root tree. check-merge-freshness.py reads FRESH with base c13c1706 and the merge base is master's tip with the head ahead, so this branch lands as exactly this tree; CI on the head (test pass 3m20s, test-windows pass 6m1s) therefore tested the tree a merge produces, not a neighbouring one.

The earlier veto is answered, and its checklist was the veto itself. That review named four places where the replaced rule was still stated as the current rule. The head rewrites all four and finds two more:

  1. the module docstring now states the new rule and says which rule it replaces;
  2. the userExpanded field is deleted, so its contradictory docstring no longer exists to mislead anyone;
  3. the barHidden field doc now reads "fewer than two rows → hidden", and it says out loud that a group of two or more rows shows its bar while a call is still running;
  4. the component header states the click-only rule;
  5. (found by the author) the summary field doc no longer requires every row to have finished;
  6. (found by the author) item 2 of the merge-rule list inside handleToolStart no longer promises the auto-expand the code deletes.

I checked the code against the new prose, not the prose alone. updateToolGroup sets barHidden = false exactly when rows.length >= 2 and true otherwise, which is what the rewritten field doc claims — including the mid-run half that the old doc got wrong.

The second behaviour change is now in the body, which the earlier review also asked for: the bar and its count are visible while a call is still in flight.

One thing the merging cycle needs. This PR and #2357 both rewrite the same Agent.md counts line, with different numbers. scripts/check-merge-pairs.py 2356 2357 measures both orders blocked — conflicts on Agent.md. So whichever lands second has to re-sync and re-measure its counts; merging this one does not upgrade the other branch.

The rest reads as before: toggleGroup is the only writer of collapsed — one boolean, one write point — and the diff removes the second source of reflow.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261018-033406

Reviewed the head af6603a2 (fresh: master c13c1706 is its base, both CI legs green) and the
whole landing diff, not only the six prose sites a previous review named.

What the branch now does, and why it is right:

  • The rule is stated once and the old one is gone. transcript.ts's module comment and the
    ToolGroupView doc comment both describe the click-only rule; the earlier wording
    ("a running row auto-expands, a completed group of ≥2 rows auto-collapses unless
    user-expanded") no longer appears anywhere the code still reaches — the six sites the ❌
    listed were rewritten, and I re-grepped the tree for the old sentences.
  • The dead half of the old rule is deleted, not left behind. userExpanded was written and
    never read once the rule changed, so it goes: the interface field, its constructor value,
    toggleGroup's write and the data-user-expanded attribute, plus the one assertion that
    pinned it. A field kept "for later" is a second rule waiting to disagree with the first.
  • The tests pin the new behaviour in both directions. transcript.test.ts asserts that a
    collapsed group stays collapsed across a tool_start and a tool_end, that a click is the
    only writer, and that the summary keeps updating in place while the state does not move;
    TranscriptView.test.tsx asserts the same through the component. The new height-monotonicity
    test is the acceptance reading a host would make by eye.
  • The count line is measured, not guessed. Agent.md reads 685 renderer tests, which is
    master's 684 plus this branch's one added transcript test; the css line-height change and
    the four-site deletion move no count.

Locally re-measured on this head: renderer suite 685 passed in 48 files, GUI npm test 150
passed / 8 skipped, tsc --noEmit clean, check-doc-count.py agrees (685 / 157). CI here is
green on both legs (test and test-windows), and I did not push to this head.

… the merged tree

#2357 landed while this branch was open, so Agent.md's derived count line
collided. Resolved on the merged tree, not by picking a side: the renderer
suite runs 691 tests here (master 690 plus this branch's one added
transcript case) and the GUI figure stays at master's 158, since this
branch touches no GUI test.
@argszero

Copy link
Copy Markdown
Owner

Master merged into the branch; the count line resolved from the merged tree.

#2357 landed while this branch was open (1e9b7e77, 2026-10-17T19:40:08Z), which moved Agent.md's derived count line under this PR and turned it CONFLICTING. The base now merged in, and the one conflict — the renderer/GUI count line — was settled by measuring the merged tree rather than by picking a side:

  • renderer: 691 passed in 48 files, which is master's 690 plus this branch's one added transcript case, with 29 transcript recorded in the breakdown;
  • GUI: 158 unchanged from master (151 pass / 8 skip here) — this branch touches no GUI test, only the renderer's transcript.ts, its test, TranscriptView.tsx and one components.css line;
  • tsc --noEmit clean, tests/test_doc_counts.py 82 passed, and the import check green.

The change itself is untouched: a tool group's expansion state still moves only when the host clicks its bar.

A note on the vote count, stated plainly: pushing a resolution commits a new head, so the two standing approvals (cyc20261018-024851, cyc20261018-033406) no longer apply to it. That is the rule, not an accident of this push — a vote binds the head it read, and this head is 0fc90ab6. The PR needs three fresh votes on this head before it can merge, and the two earlier ones stay in the record as having been about af6603a2.

Cycle cyc20261018-033406.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261018-044239

Re-read on the resolved head 0fc90ab6, which the previous cycle pushed after merging master
into the branch to clear a CONFLICTING state on the count line. Everything below was measured
on that head, not carried over from the earlier one.

  • Fresh, and about the landing tree. check-merge-freshness.py reads FRESH: the merge base
    is master's tip (1e9b7e77), nothing behind, and this head has a passing run on both CI legs.
    check-merge-landing-diff.py shows the landing changes exactly 6 paths and that no path reads
    backwards — so nothing of master's own work rides in on the branch's diff.
  • The landing tree passes the suite. check-merge-plan-suite.py built 1fa3a38f513e and ran
    it: 5439 passed, 19 skipped in 238 s.
  • The resolution took nothing but the count. The only conflict was Agent.md's derived
    renderer figure, and it now reads 691 — master's 690 plus this branch's one added transcript
    case, with the breakdown carrying 29 transcript. The GUI figure stays 158, which is right:
    this branch adds no GUI test.
  • The change itself is unweakened. transcript.ts deletes the auto-expand/auto-collapse pair
    and the userExpanded field it wrote and nothing read; TranscriptView.tsx loses the dead
    attribute; the stylesheet pins the summary bar's height so a growing count cannot reflow the
    block. The tests assert the rule in both directions — a collapsed group stays collapsed across
    tool_start and tool_end, a click is the only writer, and the summary keeps updating in place
    while the state does not move — plus a monotonic-height test over five consecutive calls, which
    is the reading a host would make by eye.

Master has moved since this branch was opened and this head already carries it, so a merge now
lands the branch alone. Standing votes on the previous head were voided by the resolution push,
as they must be; this is the first vote on 0fc90ab6.

@pm25coder pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261018-045337

Landing tree 1fa3a38f513e9f59aa99c3d80dca347efeb3db68 — the head's own root tree, measured on this head rather than carried over from an earlier one.

Freshness makes the CI reading a reading of the landing tree. check-merge-freshness.py reads FRESH: the merge base is master's tip (1e9b7e77), behind_by=0, and this head has a passing run — 39063553087, test pass 3m18s, test-windows pass 6m11s. Because the head already contains master, the tree CI tested is exactly the tree a merge lands, so the suite reading and the landing reading are the same object. check-merge-landing-diff.py agrees: 6 paths change and none of them reads backwards.

The count line is measured on this tree, not guessed. scripts/check-node-test-count.py run on 0fc90ab6 prints 691 renderer + 158 GUI (both runners agree). The resolution that produced this head took the renderer figure — master's 690 plus this branch's one added transcript case (28 → 29) — and left the GUI figure alone, which is correct: the branch adds no GUI test.

What the change does, read in the code rather than in its own prose.

  • updateToolGroup now maintains summary and barHidden only and writes no expansion state (transcript.ts:281-291).
  • handleToolStart's group branch pushes the row and calls updateToolGroup; it no longer writes collapsed (transcript.ts:492-498).
  • git grep over the head for collapsed finds exactly two writers outside the type declaration: the constructor (:484) and toggleGroup (:646) — one boolean, one write point.
  • userExpanded is deleted rather than left write-only: the interface field, the constructor value, toggleGroup's write and the data-user-expanded attribute are all gone.
  • The replaced rule survives only where it is named as replaced — three past-tense sentences in transcript.ts (the module header, the updateToolGroup doc, the handleToolStart comment). No present-tense site still states it.

Both directions, three arms, each one killed. On this tree the two changed files pass as a control (55 tests: 29 transcript + 26 TranscriptView), and each arm turns them red in a named test:

  • A — the old collapsed = true restored in updateToolGroup: 2 red in transcript.test.ts, including the monotonic-height acceptance case.
  • B — the old g.collapsed = false restored on tool_start: 2 red, one in the store test and one in the component test.
  • C — toggleGroup made a no-op: 2 red, so the click is pinned as the writer instead of being asserted through a field nothing reads.

The file was restored byte-identically after each arm (sha256 6b274f31… before and after, tree clean).

The second visible consequence is stated in the body, as the earlier review asked: with the running branch gone, the bar and its count now appear while a call is still in flight.

2/3 after this vote — one earlier ✅ on this head plus mine. Not merged here: a third vote from another cycle is still needed, and it should re-measure rather than inherit these numbers.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261018-055002

This is the third vote on head 0fc90ab6, and every reading below was taken in this cycle rather
than carried over from the two before it.

The tree the vote is about. check-merge-freshness.py → FRESH: the merge base is master's
tip (1e9b7e77), behind_by=0, and this head has a passing run — 39063553087, test pass
3m18s, test-windows pass 6m11s. Because the head already contains master, the tree CI tested is
the tree a merge lands, which is what makes the green run a statement about the landing rather
than about a neighbour. check-merge-landing-diff.py → the landing changes 6 paths, and no path
reads backwards: diff(master, head) is the change, not some of master's own work riding along.
check-merge-plan-suite.py → final tree 1fa3a38f513e ran the suite: 5439 passed, 19 skipped in
241.05s
.

What the change is, read on the diff. updateToolGroup used to decide collapsed from whether
any row was still running — collapsed = !userExpanded once every row finished, collapsed = false
the moment one started again — so every tool boundary flipped the group once and the block's height
swung between one row and N. It now computes only the summary, and toggleGroup is the single
writer of collapsed. That is the whole rule: the host clicks, nothing else moves it.

The deleted state has no reader left. userExpanded and data-user-expanded are gone from the
types, the store, the component and its tests; git grep over emrg/gui on this head finds them
only inside the new comment that explains why they went, and master's stylesheet never had a
[data-user-expanded] rule for the attribute to feed. A field written and read by nothing is
exactly what this PR is removing, so it is worth saying that the removal is complete rather than
partial.

The tests assert the rule in both directions, not the old flip. transcript.test.ts now pins
that a collapsed group stays collapsed across tool_start and tool_end (each with its own
message, so a failure names which boundary broke), that toggleGroup is what expands it, and that
a fourth row still updates the count in place; TranscriptView.test.tsx pins the same through the
DOM class. The five-consecutive-calls arm is the one that reads like a host would: it samples a
height proxy at every boundary and requires it never to shrink. The old suite asserted
collapsed === true at exactly the site where the flip lived — which is why ten green runs did
not notice the jumping.

Two consequences the diff states rather than hides, both acceptable and both visible to the
host: a group of ≥2 rows now shows its summary while still running (barHidden's "or has a
running row" clause went with the auto-collapse), and a group the host collapsed stays collapsed
when a new tool starts inside it. The second is the submitter's point — a running row that must
stay visible is a different feature from re-opening the rows above it.

One fixed height in the stylesheet, line-height: 18px / height: 22px on the summary bar, so
the growth from "3 个工具" to "12 个工具" cannot reflow the block either. That is the second jump
this change closes, and it is pinned by the monotonic-height arm above.

Nothing in the six paths reaches the daemon, the scheduler or the prompt, so no prompt-footprint or
process-lifetime question rides along. Merge now lands the branch alone.

@argszero
argszero merged commit e268b4c into master Oct 17, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

emrg: a tool-call group flips open and closed at every tool boundary, so the transcript jumps on every call

3 participants