Repository navigation
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the delegation default overrides the reader's work-fold toggle.
I found one merge-blocking behavior regression. I reviewed the full diff, relevant TUI/web callers and fold state transitions, recent history, backward compatibility, the updated assertions for weakening/skipping, and the repository rules and architecture vocabulary in AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, ui-tui/CONTEXT.md, and ui-web/CONTEXT.md. I found no additional issues in those areas.
Verification after installing the locked npm dependencies:
ui-web: npm test -- src/features/transcript/store.lane.test.ts src/features/transcript/TranscriptPage.test.tsx src/features/tasks/NodeRecord.test.tsx src/features/tasks/TasksPage.test.tsx- 4 files, 400 tests passed.ui-tui: npm test -- src/__tests__/episodeView.test.tsx- 1 file, 46 tests passed.npm run type-checkin bothui-webandui-tui- passed.
The install warned that Node 23.10.0 is outside Vitest's declared engine ranges, but the commands completed successfully.
|
Windows compatibility review — PR #1129 @ Scope: full base Result: no Windows-specific issue. The change is DOM/virtual-list/state logic and TUI layout constants; no path, filesystem, subprocess, signal, encoding, or platform primitive. Verification (native Windows 11, isolated worktree Limitations: the TUI test ( |
|
Windows compatibility re-review — PR #1129 @ Scope: delta from the previously reviewed Result: no Windows-specific issue. The change is React state/DOM logic and TUI layout tests; no path, filesystem, subprocess, signal, encoding, or platform primitive. Verification (native Windows 11, isolated worktree Limitations: the TUI tests ( |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new revision fixes the outstanding manual-fold regression without introducing another issue. I reviewed the full diff and the revision delta, the fold state callers and merge path, relevant history, backward compatibility of reader-owned state, the changed tests for weakening or skipped coverage, and the repository rules and TUI/web architecture contexts. The earlier thread is replied to and resolved.
Verification:
ui-web: npm test -- src/features/transcript/store.lane.test.ts src/features/transcript/TranscriptPage.test.tsx src/features/tasks/NodeRecord.test.tsx src/features/tasks/TasksPage.test.tsx- 4 files, 403 tests passed.ui-tui: npm test -- src/__tests__/episodeView.test.tsx src/__tests__/virtualHeights.test.ts- 2 files, 75 tests passed.npm run type-checkin bothui-webandui-tui- passed.
|
Not a blocker -- one thing worth measuring before or after merge: the web transcript's replay weight. Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.
Narration between tool calls often runs past 140 characters, so on a long agentic session much of that shut-body weight may come back. The rule is right as a default. The question is only whether the replay of the same forty-turn session still builds a reasonable number of nodes. Re-measuring it, or keeping replayed folds shut and applying the rule only to turns watched live, would settle it. Checked on this head merged onto
|
df2b5ac to
f929789
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The branch was rebased onto the current main. I checked the range-diff and current full diff: the reviewed feature and manual-fold fix retain their behavior against the newer transcript state, while the new tip commit only corrects stale documentation above the delegated-run tests. I also checked the affected callers and merge path, relevant history, backward compatibility of reader-owned fold state, changed tests for weakening or skipped coverage, and the repository rules and TUI/web architecture constraints. My earlier thread remains resolved.
Verification after syncing the locked npm dependencies:
ui-web: npm test -- src/features/transcript/store.lane.test.ts src/features/transcript/TranscriptPage.test.tsx src/features/tasks/NodeRecord.test.tsx src/features/tasks/TasksPage.test.tsx- 4 files, 413 tests passed.ui-tui: npm test -- src/__tests__/episodeView.test.tsx src/__tests__/virtualHeights.test.ts- 2 files, 75 tests passed.npm run type-checkin bothui-webandui-tui- passed.
The previously noted web replay-weight measurement remains a nonblocking follow-up. The install again warned that Node 23.10.0 is outside Vitest's declared engine ranges, but all commands completed successfully.
|
Windows compatibility re-review — PR #1129 @ Scope: the branch was force-updated and rebased onto current main; the diff at this head was taken against its new merge-base Result: no Windows-specific defect. The change is terminal/UI fold state — episode segmentation, height estimation, transcript store folds, and task-node think-block defaults. It touches no path handling, subprocess, signal, socket, file, encoding, or process API; the only character-count rules ( Verification on native Windows 11, detached worktree
Coverage gaps: only the changed test files were run, not the full ui-web/ui-tui suites or the repo's UI gates; the Python suites were not exercised (this head's own diff contains no Python change). |
|
The rule runs on both replay and live on purpose: what keeps a fold open is a property of the turn, not of how the client got it, so a reopened conversation would otherwise disagree with the one the reader watched. I measured the cost on my longest real workspace session (443 user turns, 4,087 wire rows, 13 MB), replaying the same
The 7,361 / 6,400 figure in the code comment is from a session that is not in the repo and predates lazy shut bodies, so I did not compare against it; |
A finished turn's fold is a one-line compression, and it was hiding the content a reader is most likely to need. Three defaults change, in both chat surfaces, and all of them remain manual folds. ui-web: - a turn's fold opens when its work holds an agent message of 140 characters or more (FOLD_KEEP_OPEN_CHARS), or a spawn/dag call; - a step that delegated keeps its call rows out of the work summary; - a task node's reasoning starts folded, like a delegated transcript's. ui-tui: - the work under a message of 100 characters or more (MESSAGE_FOLD_KEEP_OPEN_CHARS) stays open; - every delegation panel in a running stretch stays visible, not only the call in hand; - a trace box no longer prints a line of reasoning unasked. The reader's own toggle still wins: a fold they touch is theirs, and the runtime does not reopen it. The TUI height estimator is updated with the new default so the virtualised rows keep their reserved height.
`keepCalls` made a multi-call step's body visible regardless of `wkOpen`, so the summary row stayed clickable while no click could hide anything and its open marker could disagree with the body. That turned the default into a lock, which is not what the fold rule is for. The default now lives in `paintWork`, gated on the existing `wkPinned` flag -- the same split `FoldData.auto` uses for a turn's fold: the runtime picks the untouched state, and a reader's toggle takes it over for good. A step that delegated still opens by default. A merged silent run inherits a pinned member's choice and otherwise follows the same rule. `StepView` no longer layers a second rule: the summary's open marker and the body both read `seg.wkOpen`. Tests: the reader can shut a delegating step, it stays shut when another call lands, and it opens again on the next click; the merged-run default and its pinned hand-down are pinned directly. The TUI needed no behavior change (its folds resolve through `foldOpen`, where an explicit close always wins); it gains a reader-close test and a height-estimator test proving the estimate and the renderer agree in both states. Existing tests that clicked the summary only to reach the rows now use the `openWork` helper, which clicks a shut summary and leaves an open one -- their subject was never the fold.
The describe block still said a fold shuts over a sub-agent that is still working and that a spawn in flight buys the fold nothing. The assertions below it say the opposite: a spawn or dag call keeps the fold open by default, at the turn's end and on replay, and it is the call kind rather than the running status that does it. The reader's own toggle still wins.
The fold-open weight comment called the protected turns "those few"; on the 443-turn session measured for this PR 182 of 287 folds arrive open and build 35.1% more nodes than the baseline. The TUI's solo-call comment said the detail block stays folded, but the solo branch defers to `cardDefaultOpen`, which opens it whenever the call has content. The web `callKeepsOpen` comment now records that a prompt-mode `load_playbook` is classified `dag` by name and that the TUI has no card for a call with no run.
A long final answer must not open the turn's fold: `finishTurn` promotes it into the answer row and clears the step's prose before `collapse` reads the steps. Two shapes pin that -- an answer-only turn, and an answer sharing its step with a call, where only the cleared `say` keeps the step out of the rule. A mutant that leaves the answer in the step kills the second test; one that collapses before the promotion kills both.
f929789 to
b0edce2
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The branch was rebased onto current main; range-diff shows the three previously reviewed patches are unchanged. The two new commits correct fold-default comments to match the measured behavior and add regression coverage proving a long final answer is promoted out of the fold predicate before collapse. They do not change production behavior. I checked the current full diff, affected callers and fold/merge ordering, relevant history, backward compatibility, changed tests for weakening or skipped coverage, and the repository rules and TUI/web architecture constraints. The replay-weight question now has a measured answer, and my earlier review thread remains resolved.
Verification after syncing the locked npm dependencies:
ui-web: npm test -- src/features/transcript/store.lane.test.ts src/features/transcript/TranscriptPage.test.tsx src/features/tasks/NodeRecord.test.tsx src/features/tasks/TasksPage.test.tsx- 4 files, 415 tests passed.ui-tui: npm test -- src/__tests__/episodeView.test.tsx src/__tests__/virtualHeights.test.ts- 2 files, 75 tests passed.npm run type-checkin bothui-webandui-tui- passed.
|
Windows compatibility re-review — PR #1129 @ Scope: the branch is rebased onto current main (which now carries #1109, #1125, #1141 and #1149); Result: no Windows-specific issue. The feature remains ui-web/ui-tui fold-state logic with no path, subprocess, signal, socket, or encoding surface; this delta contains no production-code change. Verification: no executable check was possible — the shared Coverage gaps: the new test was not executed here, and no browser rendering was done; the review of this delta rests on reading. |
Summary
Three chat fold defaults change, in both ui-web and ui-tui. Each is a default, not a lock: the reader can still open or close the fold, and once they do, their choice wins.
FOLD_KEEP_OPEN_CHARS = 140inui-web/src/features/transcript/store.ts, about two lines at the transcript's ~70-glyph CJK reading measure. One exported constant, not scattered magic numbers.MESSAGE_FOLD_KEEP_OPEN_CHARS = 100inui-tui/src/config/limits.ts, about two rows of a 100-column terminal body.paintWork, gated onwkPinned); clicking the summary still shuts them, and a call landing afterwards does not reopen them. In ui-tui every delegation panel in a running stretch is drawn, not only the call in hand, and a reader's closed key outranks the default. Display only: no tool implementation, schema or orchestration code is touched.load_playbookis classifieddagby name in the web client, so it opens the fold too; the TUI has no card for a call with no run and leaves the same turn folded.NodeRecord); in ui-tui a trace box no longer prints a line of reasoning unasked. The main chat's live reasoning is unchanged.The default and the reader's decision are separate state, the same split
FoldData.autoalready used: the runtime picks the untouched state, andwkPinned(or a closed fold key in the TUI store) takes it over for good. The summary's open marker and the visible body read the sameseg.wkOpen, so they cannot disagree. The TUI height estimator mirrors the new default and still honours a reader's close, so virtual rows keep their reserved height.Type
Verification
Commands and results, all run in the branch worktree:
cd ui-web && npx vitest run --maxWorkers=2-> 262 test files, 4529 tests passed.cd ui-tui && npx vitest run --maxWorkers=2-> 145 test files, 2281 passed, 13 skipped.make lint-ui lint-tui-> 0 errors (pre-existing warnings only; rpc and i18n generated checks OK).npx tsc --noEmit -p tsconfig.jsonin ui-web and ui-tui -> no output.npx commitlint --from origin/main --to HEAD --config commitlint.config.cjs-> OK.scripts/check_commit_messages.py origin/main..HEAD,scripts/check_source_language.py origin/main..HEAD,scripts/check_large_files.py origin/main..HEAD-> exit 0 each.wkPinned(reader-toggle test), a merged run that ignores its delegation (merged-run default test), and a height estimator that ignores the message rule (estimator alignment test).openWorkhelper, which clicks a shut summary and leaves an open one; their subject was never the fold.No docs change: the repository docs describe the design, not these two default constants.
Risk
Replay weight was measured on a 443-turn session: 287 folds, 182 open, 77,194 nodes against 57,141 before the rule (+35.1%). The rule applies on replay as well as live, so a reopened conversation shows the same defaults.
Display-only change; no API, schema, config or persisted-state format changes. Fold state is in-memory per lane. Rollback is a revert of the squashed commit. The two thresholds are single named constants and can be retuned without touching call sites.
Related Issues
N/A