Repository navigation
feat(chat): fold contiguous todo snapshots without losing attempts - #1846
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
|
| Layer / File(s) | Summary |
|---|---|
Tool-call identity and outcomes shared/protocol.ts, server/claude-transcript.ts, server/codex.ts, server/*test.ts, server/api.contract.test.ts, site/demo/transport.ts |
The conversation tool-part contract adds optional call IDs and states. Claude records pending, successful, or failed outcomes; Codex records pending state and removes it when an output arrives. Tests and demo transport cover these fields. |
Snapshot extraction and grouping src/lib/todos.ts, src/lib/workBlocks.ts, src/lib/*test.ts |
Todo inputs and operations-tool answers produce snapshots when readable. Consecutive Todo calls form groups with stable IDs, ordered attempts, selected snapshots, and pending and failure counts. Tests cover grouping boundaries and state handling. |
Expandable work-block rows src/components/ChatView.tsx, src/components/ChatView.css, src/lib/i18n/chat.*.ts, site/demo/fixtures.ts, DESIGN.md, changelog.d/contiguous-todo-snapshots.md |
The chat renders grouped updates with counts, a current list, and expandable attempt history. Row identity, styling, translations, demo conversations, and design and changelog text are updated. |
Browser regression coverage scripts/todo-snapshots-demo-regression.ts, scripts/ci-browser.sh |
A Playwright regression checks agent metadata, grouping, polling, expansion, display behavior, and responsive layouts. The browser CI lane runs the regression. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Feature
Possibly related issues
devswha/herdr-web-ui#1601— Describes folding consecutive Todo snapshots, recording call outcomes, preserving row identity, and showing each attempt.devswha/herdr-web-ui#1053— Calls for collapsing repeated Todo updates while preserving the latest visible plan.
Suggested reviewers: kilhyeonjun
Merge Risk: 
Minimal · up to a971d
-
The todo-update grouping is ready to merge after normal checks; no concrete blocking issue remains.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passed-
Full details: Title check
-
Explanation
The title "feat(chat): fold contiguous todo snapshots without losing attempts" matches the main change: consecutive todo updates in the chat work block fold into one row, and every attempt stays visible when opened. It is specific, concise, and follows conventional-commit style. The title does not mention the contract change (call_id/state) or the i18n and changelog work, but it does not need to cover every detail.
Full details: Description check
-
Explanation
The description follows the template. The Change section describes the problem (one row per snapshot), the resulting behavior (contiguous todo updates fold into one row), the rules for runs and opening the row, and the additive contract change. It also links DESIGN.md and the changelog fragment. The Validation section lists the checks run (unit tests, contract test, browser regression script) and their results, and it covers the HTTP contract change. The description does not link an issue where a maintainer agreed to the feature, which the template asks for on feature work. This is a minor gap, so the check still passes.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 64.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 19 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
CodeRabbit did not review this PR ("Review limit reached"). Since it changes the Code-review agent (omo-native-code-reviewer): APPROVE, no blockers.
Codex (
All checks were rerun after the fixes, at 3080091 on origin/main 5e67b0b: |
|
Merge pass: one rebase needed. #1874 is merged on main. |
3080091 to
a971dfb
Compare
|
Rebased onto main after #1874 and pushed a971dfb (was 3080091). I dropped this PR's change to |
Todo updates an agent makes one after another now read as one row in the chat's work block instead of one row per snapshot. Contract (additive, GET /api/pane/conversation): a tool part may carry the agent's own call_id and a recorded state. Claude's tool_use id becomes call_id; the call is pending until its tool_result, then ok, or error when the result is flagged is_error (an empty result is ok). Codex keeps its native call_id and is pending until a function_call_output or custom_tool_call_output answers it; then state is omitted, because Codex records no outcome. Output text and its emptiness are never read for a state, and the existing error heuristic stays separate. Other agents and older servers send neither field. Client: runs of two or more todo calls with nothing between them fold into one row, found in the turn's own order before hidden thinking and skills are taken out, so narration or any other part ends a run. The row stands for the latest completed list: Claude's ok list, or an answered snapshot with no recorded outcome shown as "outcome not recorded" without a success claim. Pending and failed updates are counted beside it, an unreadable latest list is said so instead of falling back to an earlier one, and opening the row shows every update in order with its state in words. A run is named by its first call's id within its turn row (row plus first index without ids), so appends and polls keep it open and focused. The demo's curated Claude and Codex chats carry the fields and show one run each, with an omp run that records neither. Strings are translated into Korean, Japanese and Chinese; DESIGN.md records the rules. Checked with bun run check fast, the parser, grouping, todo and revision unit tests, a new api.contract case on an isolated herdr (Claude pending to ok/error, Codex pending to omitted for both output kinds including empty and failure-looking output, nothing for omp), and the new scripts/todo-snapshots-demo-regression.ts at 390 and 1440, light and dark, and in Korean, enrolled in the browser lane.
The todo snapshots demo regression now turns the in-app alert off (the demo's own turn finishing dropped it over the captures), folds the approval card of the blocked Codex pane before a capture, takes end-of-chat states through the app's Jump to latest so the button no longer covers the last run, and moves the pointer off the rows before a rest-state capture. Checked by running bun scripts/todo-snapshots-demo-regression.ts.
A todo update flagged failed by an agent that records no call state (omp, or an older server) was read as an unknown outcome and so became the list the folded row stood for. It is now only counted as failed, and the latest update that did not fail stands for the list. Found by the Codex review; checked by a workBlocks case that fails without the fix.
Main renamed the browser storage keys to ovis:* after this script was written, so its settings and chat view no longer reached the app and the conventions check refused the old names.
Since a failed update is no longer taken for the folded list, the chat's existing failure reading of a Codex output does decide that; the design note said output text was never read.
a971dfb to
57983ee
Compare
|
Rebased onto main after #1845 and pushed 57983ee (was a971dfb). The one conflict was the |
|
The Integration tests failure on 57983ee was real and came from this PR's own new test, not its product code. It's fixed in c00c193 (test-only). Cause: Fix: the stand-in now starts as |
|
Merge pass review-work report. Overall: PASSED. This PR changes the conversation contract additively, and CodeRabbit didn't review it, so per AGENTS.md this comment records the independent review.
QA
Behaviour
Round 1 finding: the two waits in Follow-ups (MINOR)
All required checks are green, and no review thread is open.
CodeRabbit did not review this PR (rate-limited or skipped). The reviews above stand in for it. |
…ough its rewrites (#1937) ## What it does for the user While a GJC turn streams tool calls into Chat, a tool you opened now stays open and stays where it is on screen: - **Opening is reading.** Opening a tool row, a folded work block or a tool's whole output stops following the end, just as scrolling up does. New records arrive below without moving what you are reading. Jump to latest (or New messages) brings you back to the end, and following resumes from there. A reader who opens nothing keeps following exactly as before. - **GJC rewrites that keep the turns no longer reset the chat.** gjc writes its whole session file again and renames it over the old one: on its first save, and when it trims old tool outputs (it does the same when it compacts). The server reads the new file as another history (`history_id` is the file's identity), so the chat started over: every opened detail folded, a reader's place was lost, and at phone width the end was no longer followed. Now a new history ID whose newest page still opens with every turn already shown, in the same places, keeps the rows, what was opened and where you read. Each shown turn's role and start time, each part's kind, and each tool's call id and input must be unchanged (gjc's tool parts now carry their `toolCall` id as `call_id` for this); what a tool answered may be trimmed. Anything else still starts over as before: a compaction that dropped turns or moved where the newest page starts, a new session, a cleared context, a rewind (even one that keeps a turn's tools' names), a moved branch, or a reader holding older pages. - **A todo update you opened stays open.** Since #1846, contiguous todo updates fold into one Plan row. An update you had opened turned into a closed Plan row as soon as the next update arrived, so what you were reading folded away. The Plan row now opens when you had opened one of its updates, with that update still open inside it. A Plan row nobody opened still starts closed. ## Why #1388: "the viewport alternates between the user message and the bottom" and "tool details that the reader expanded collapse when new records arrive". Two causes, both reproduced on current main with GJC-shaped records: 1. `ChatTimeline` remembered a click on a disclosure only until the next rows change (`manualLayout` cleared on every `rows` update). Then ChatView's layout effect and the list's size callback followed the end again, dragging an opened running tool's header up 383–455 px on every record. 2. gjc 0.18's session manager rewrites its file through a temp file + rename (`rewriteEntries` after compaction eviction and tool-output pruning; I read this from the installed gjc 0.18.12 binary). A rename is a new inode, so `transcriptStream` gives a new `history_id` and ChatView's read dropped the row slots, the row memories and the reader's intent. Since #1831, a message sent from this browser's box follows the chat back to the end unless the reader navigated after it went out. Opening a tool now counts as that navigation too, as scrolling up does, so a send on its way no longer pulls the reader off a tool they opened. A gjc rewrite that continues the chat is not navigation (the reader's place is kept); any other new history still is. Design: T3 Code took the other side for a reader at the very bottom (pingdotgg/t3code#17954 keeps a reader who toggles a tool there pinned to the bottom). Here the issue asks for the opened row to stay put while a turn streams, and Ovis already treats scrolling up as "stop following until I come back". Opening a detail joins that rule instead of adding a new one, so nothing new is drawn: the existing Jump to latest and New messages controls carry it. DESIGN.md's chat follow paragraph says so. The server's history identity is unchanged. **HTTP contract:** `GET /api/pane/conversation` now gives a gjc tool part its own `call_id`, the optional field `shared/protocol.ts` already declares for Claude and Codex; omp and pi still carry none (`server/api.contract.test.ts`). CodeRabbit is rate-limited here, so a code-review agent's review stands in, and this needs a real review before merging. A byte-prefix check on the server would miss gjc's rewrites, because pruning changes earlier records. The continuity check runs only in the chat that already holds the turns. ## How it was verified - New `scripts/chat-gjc-streaming-browser-qa.ts` (added to the browser lane). It feeds GJC-shaped `message`/`toolCall`/`toolResult` records one at a time through the real `transcriptPage("gjc-transcript")` parser and the real ChatView → ChatTimeline → LegendList path in Chromium, at 1440 and 390 px. Three readers are covered: one who opened a running tool, one following the end, and one scrolled up. Each runs with plain appends and with a gjc-style temp+rename rewrite. It samples every frame from each refresh until the view settles: - the opened tool's header and the scrolled reader's row stay within 1 px in every frame - the opened tool stays `aria-expanded="true"` - a follower ends at the end after every record - Jump to latest resumes following - On main (d3eefef, before this branch was rebased) it fails 62 assertions: - header displacement −383 to −455 px per record - `aria-expanded="false"` after a rewrite - the scrolled reader's row lost after a rewrite - the 390 px follower 179–205 px short of the end - in a chat that still fit the screen when the tool was opened, the opened tool dragged up to 1502 px once later records overflow it - On this branch every scenario holds at 0 px (20 scenarios, rebased on ddee8be). Current main (ddee8be) still fails 62 assertions. - Its `todo` scenario (added with the rebase) opens a gjc `todo_write` update and appends the next one, plainly and through a rewrite: the Plan row's head stays where the opened update's head was in every frame, it is open, and the update read stays open in it. Run against the commit before that fix, the script fails only those 16 todo assertions. - `server/conversation.test.ts` runs a gjc rewrite through the real parser: the same calls written again continue, and a rewind that reads the same file again under another call id, later, does not. - `src/lib/chatRows.test.ts` covers `continuesHistory`: continuation, a dropped/moved/retimed turn, a page that starts later (dropped first turns), a rewind that puts another call of the same tool in a shown place (other input or call id), another source, scrollback, empty and untimed pages. - Its `jump` scenario opens a tool and takes Jump to latest at once; closes and reopens it and jumps again; closes it while following, scrolls away and jumps back; and does that again but scrolls back to the end by hand. After each Jump the last row grows with no record between, and the end is followed. The script counts an answer as served only once the chat has read its body and React has committed after it. - `scripts/chat-virtual-browser-qa.ts` passes at 1440 and 390 px, dark and light (paging, pagination, virtualization, selection and disclosure restoration). The other chat browser scripts run in this PR's browser lane. - `scripts/todo-snapshots-demo-regression.ts` passes, and so do `chat-local-send-follow-demo-regression.ts` (#1831), `chat-browser-qa.ts` and `chat-tool-paths-demo-regression.ts` on the rebased branch. - `bun run check fast` passes: typecheck, build, generated files, workflow syntax, 6772 unit tests. - Reviewed by a code-review agent (no HIGH; its one real gap, a chat too short to scroll re-arming follow, is fixed in the second commit) and by Codex Sol 6.1 (no HIGH; three MEDIUM findings on the history continuation are fixed in the third commit: it applies to gjc only and needs each shown part's kind and tool to match, an older-page read in flight is dropped, and the kept reading place moves to the new history ID). A Codex re-review of that commit found no remaining HIGH or MEDIUM issue. - After the rebase onto current main, Codex Sol reviewed the rebase delta. The two conflict resolutions with #1831 were clean. It found one MEDIUM, the #1846 todo row folding an update the reader had opened, which the last commit fixes. Its re-review found one more MEDIUM (a remounted Plan row lost the openness it had inherited), fixed in the same commit; its 13-case repro passes. Not covered: native iOS/Safari, a gjc compaction that drops turns (it starts over, as before), real gjc runs (the records are synthetic and the rewrite is simulated as gjc does it), and a reader who has loaded older pages when gjc rewrites. That reader still starts over, because those pages were read from the old file. Fixes #1388 --------- Co-authored-by: mp <mp@local>
… each update's own input and output (#1601) (#2161) ## Change Fixes the two gaps #1601 still had after #1846 folded contiguous todo updates into one Plan row. - **Focus survives the fold.** When the agent's second todo update turned the single row the reader was on into a Plan row, the focused button left the page and the keyboard focus fell to `<body>` (expansion already carried over since #1937). A row head now records in its timeline row's memory that it holds the focus (`useRowFocus`, `src/lib/chatRowState.tsx`). The head drawn in its place in the same render takes the focus back: the same update inside the opened run, or the run's own head when it is folded. Focus the reader moved elsewhere is never taken, a head that leaves with no replacement ends its record (a row scrolled back or reopened later never grabs the focus), and a fold while the reader is in another app keeps it too. - **Each attempt shows its own call.** An update opened inside the Plan row showed the parsed list again, which the run already shows above it. It now shows the input the agent sent (JSON indented, under "Input") and the output it got back, including "Show the whole output" for a cut one. Design: the Codex and Claude apps keep one row per action under the turn and keep the reader's place while a turn streams. This follows that, as #1846's DESIGN.md entry does; the entry now records both rules. "Input" is translated into Korean, Japanese and Chinese. A `changelog.d/` fragment is included. Closes #1601. ## Validation - Regressions that fail before and pass after: - `scripts/chat-gjc-streaming-browser-qa.ts` (browser lane): its todo scenario now asserts the focus is on the opened update inside the Plan row after the next update arrives. On main all four variants (1440/390 × append/rewrite) failed with the focus on BODY; on this branch all pass. - `src/components/ChatTurn.test.ts`: an opened two-update run shows each update's raw input and output. - `scripts/todo-snapshots-demo-regression.ts`: 21/21 pass (1440/390, light/dark, Korean), including "open, focused run through append and poll". - `bun run check fast`: green on b4d51fd (unit tests 7886 pass, 0 fail). On the final head 12c0c34 (one line added to `useRowFocus`), typecheck, build and generated files pass; the unit tests had one timeout in an untouched file, `scripts/ci-browser.test.ts` "creates evidence under .ci when CI has no CHECK_DIR" (5029 ms on a 5 s limit at host load ~66). Rerun on its own, it passes in 3.4 s (7/7). - Not covered by an automated test: the background-window case (headless Chromium always reports the page focused) and StrictMode's rehearsed unmount (the browser fixture renders without StrictMode). Both were reviewed at source level and confirmed by Codex's last round. - Reviews: omo-native-code-reviewer PASS (its MEDIUM, a window blur ending the record, fixed; the over-long DESIGN.md line fixed; other LOWs out of scope). Codex Sol 6.1 Fast xhigh: round 1 MEDIUM (a stale record after a head left with no replacement) fixed; round 2 MEDIUMs (StrictMode rehearsal, focus events deferred while the window is in the background) fixed; the confirmation round on the final head PASS with no findings.
Change
When an agent updates its todo list several times in a row, the chat's work block used to draw one row per snapshot, so a plan that was re-written five times read as five near-identical rows. Contiguous todo updates (Claude Code
TodoWrite, Codexupdate_plan, omp/OmOtodo) now fold into one row:▸ Plan 3 updates · 1/3 done [1 pending] [1 failed]Contract (additive,
GET /api/pane/conversation): a tool part may carrycall_idandstate(pending|ok|error).call_idis thetool_use.id;pendinguntil itstool_result, thenok, orerrorwhen the result is flaggedis_error(an empty result is ok).call_id;pendinguntil afunction_call_output/custom_tool_call_outputanswers it, thenstateis omitted, because Codex records no outcome. Its latest list therefore shows "outcome not recorded" with no success claim.Because this changes an HTTP contract, it needs a real review before merging.
Design: follows the app's own Codex-app-style work block (Codex and Claude apps fold actions under the turn, one row per action). DESIGN.md records the rules. Todos stay inside the conversation, as agreed. Strings are translated into Korean, Japanese and Chinese in
src/lib/i18n/chat.*.ts; achangelog.d/fragment is included.This was a local branch cut ~189 commits before current main; it is rebased onto origin/main. The Claude parser hunks moved with main's split into
server/claude-transcript.ts, and the strings into the i18n parts.Known follow-up (not in this PR): a call whose result is never written (the agent process killed between call and result) stays "pending" in a finished turn.
Validation
bun run check fast: workflow syntax, generated types/themes, typecheck, build, unit tests (6411 pass, 0 fail).todoGroupsgrouping (contiguity, stable ids across polls, latest-completed selection, failed calls never taken for the list),todoSnapshot, and turn revisions.server/api.contract.test.tsnew case on the isolated herdr: Claude pending → ok/error, Codex pending → omitted for both output kinds (including empty and failure-looking output), nothing for omp. Passed: the whole file, 156 pass, 0 fail.scripts/todo-snapshots-demo-regression.ts(enrolled in the browser lane) on demo fixtures at 390 and 1440, light and dark, and in Korean. Passed: 21/21 cases, 84 screenshots; checked by eye that the folded row, its counts and the opened list read right at 390 and 1440, light, dark and Korean, with no overlap.gpt-6-astra(no HIGH; its MEDIUM, a failed update without a recorded state being taken for the list, fixed with a test that fails without the fix).