feat(ui): checkpoint a chat turn, fork from a checkpoint, and duplicate a chat - #2775
feat(ui): checkpoint a chat turn, fork from a checkpoint, and duplicate a chat#2775Charlesthebird wants to merge 22 commits into
Conversation
A checkpoint saves the conversation's newest turn boundary; forking one
starts a new chat holding the transcript up to it. Duplicating a chat from
the rail is the same pair of calls with no boundary to choose.
- `agentInstances.checkpoints.{create,list,fork}` reach `CheckpointService`
directly; `agentInstances.fork` now composes create and fork.
- The chat marks a checkpointed message by matching `headTaskId` against the
turn its messages carry, so the marks survive a reload.
- Fork moves from "the latest message" to "any checkpointed message", which
is what `ForkAgentInstance` already supported.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
…a fork The checkpointed label already carries a save icon, so the button beside it went. Forking is the menu's only item and needs a boundary, so a message without one has no menu rather than one item that refuses. The menu a checkpointed message does have stays on screen, in the panel's own colour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
… line The control moves off the messages: `CreateCheckpoint` saves the conversation as it stands, which is the composer's tense rather than any one message's, and it is disabled once the latest boundary is already saved. The boundary itself is drawn as a rule under the turn it was taken at, with the fork beside it and room either side, because what it marks is where a fork would cut. The rail's duplicate moves into the row menu, under Rename chat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
… line The line says where a fork would cut; a control sitting in the break read as part of the label. Below it and outlined, kept small so each boundary pushes the conversation apart as little as it can. The composer's tooltip loses the sentence about the latest message, which the button's place already says. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Beside the label rather than on a row of its own, so a boundary costs the conversation one line and no more. Outlined, because a borderless icon there reads as part of the mark rather than as something to press; its name moves into the tooltip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
…ontrol The fork reads "Fork" beside the checkpoint, separated by a tick of the same rule, with the destination in its tooltip. The button back to the foot of a conversation takes the brand purple for its edge and fills with it on hover: its previous tint was a near-white on the light theme and answered nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
…s own The rail's row menu is drawn on every row rather than revealed on hover, and carries Chat details and Share chat — both take an instance id, so neither needs the conversation open. It is square and as tall as the row, and so is the bulk menu above it. Checkboxes get a target the width of the row's height, hover and press states that read on both themes, and a focus ring drawn inside the box: the rail clips at exactly the column they sit in, so anything outside it came back with its left side sliced flat. They line up with Select all above them. The three columns now fold as the window narrows — the agent panel at 1440, the application sidebar at antd's `xl`, the rail at 1040 — and unfold again. The panel's drag handle goes with it: the wrapper clipped to a fixed width, so dragging widened an aside inside a box that never grew. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
The bulk bar and the list reserve the same scrollbar track, so their menus sit in one column whether or not the list scrolls — the bar reserves it without becoming something that scrolls. A menu button is filled while its menu is open, driven from React because the icon takes its colour as a prop. The checkboxes keep antd's box and gain a ring around it: pale on hover, thicker and brand-coloured on press. The rail and its list widen their clip boxes to the left so the ring is drawn whole — both clip at exactly the column the boxes sit in, and every earlier attempt came back sliced flat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Models, MCP servers, prompts and agents each offered theirs in a different case, and the schedules list already said "New Schedule". MCP servers said "Add" where the other four said "New". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Boundaries saved on a page are remembered against the message they were taken at, because the reader's newest message has no turn id yet. A fork is handed copies of its source's messages under the same ids, so it inherited marks for boundaries it does not have until something reloaded it. The memory is dropped when the conversation changes; the controller's list is the only truth. The rail's rows get room between the checkbox, the name and the menu, the open one is a tint and a weight rather than an outlined pill that met the button beside it, and the two share the smaller radius. An open menu is outlined rather than filled, and there is a divider above Delete chat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
|
Note Outdated. Superseded by the checkpoint-and-fork reel further down this PR. The original commentEverything this branch adds, against a real cluster: checkpoints taken from the composer and surviving a reload, forks from two different lines, two duplicates that keep their own favourite city, the agent rail's menus and checkboxes, and the three columns folding as the window narrows. summary-reel.mp4🤖 written by Claude |
The bar reserved the list's scrollbar track with a `scrollbar-gutter` of its own, which meant making a bar that never scrolls into a scroll container — and a scroll container clips, so the focus ring on its menu lost its top and bottom. The track is measured off the list instead: 0 where scrollbars overlay the content, about 11px where a reader has asked for them always, and the two columns of controls stay in line either way with nothing clipping anything. The list itself gains the room the first and last rows' rings need. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
…holds A checkpoint pins a copy of a conversation's runtime in the substrate — that is what a fork starts from, and what it costs. They were reachable only from inside the conversation they belong to and could not be removed at all, while Duplicate chat takes one every time, so a cluster accumulated snapshots nobody had asked for and nobody could find. The page lists every one the caller can see, searches them, and deletes them singly or in a batch. A batch is one request each — `DeleteCheckpoint` takes one id — sequential so a run of forty does not arrive at once, under one `toast.promise`. A failure does not stop the run: the rest still go, each is logged, and the toast says how it went rather than claiming success over a partial one. `ListCheckpoints` takes an instance id and requires it, so the page reads the conversations first and asks each for its own. One conversation's failure leaves the other rows intact. Also from review of this branch: - The Checkpoint button is gated on the whole active turn, not just the streaming phase; between Send and the first token it was live and refused. - The transcript renders one flat run keyed by message id. Keyed by group, a regrouping when the boundaries landed unmounted the messages under it and every rendered mermaid diagram flashed back to loading — which is what the polled assertion in `chat.spec.ts` was really papering over. It is strict again. - A checkpoint that saved but whose re-read failed reported the failure. - `ListCheckpoints` follows its page token; past 50 boundaries the oldest were silently dropped. - The checkpoint line is a labelled group, not a `separator` — ARIA prunes a separator's children, which hid the only fork control in the application. - Fixture checkpoints persist across a reload, as the controller's do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
`BeginDeleteAgentInstanceCheckpoint` will not remove one while a chat still names it as the boundary it was forked from, and reports that as `NotFound` — which the page cannot tell from a snapshot that has genuinely gone. So the condition is stated up front rather than guessed at per failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
|
Note Outdated. The Snapshots page moved to #2798. The original commentSnapshots, against a real cluster: three checkpoints taken in a chat, the new page listing every one across all conversations, and three deleted together — one request each, sequential, under one toast. summary-reel.mp4🤖 written by Claude |
|
Note Outdated. Superseded — the listing moved to the controller, in #2798. The original commentThe Snapshots page here reads every checkpoint into the browser and searches, sorts and pages there. That is deliberate — #2783 is the follow-up: an RPC that does pagination, search and multi-column sort server-side. It needs a proto change, so it is out of scope here. 🤖 written by Claude |
…ng to Clearing them in an effect on `id` was a synchronous setState in an effect, which lint refuses and CI failed on. Carrying the conversation with the marks drops them without one: a different id reads as no marks, with no render in between where the old ones still apply. The shape `useChat` gives its transcript, for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
There was a problem hiding this comment.
Note
Outdated. This reviewed the branch before the Snapshots page and its listing moved to #2798. The findings still hold, but half of them are against files this PR no longer touches.
The original review
This automated review is a first pass. A manual review will follow once the blockers are addressed.
I wrote this branch, so this is a review of my own work: everything below was found, fixed and pushed before posting. Nothing here is outstanding.
The one thing a reviewer should weigh rather than take from me is the Snapshots page reading every checkpoint into the browser. It is a request per conversation, it is deliberate, and the reasoning is in the description and in #2783. If that trade is wrong, the page should not merge in this shape.
What I fixed after review (12)
| Level | Where | What was wrong |
|---|---|---|
| 🔴 High | ChatComposer.tsx |
Checkpoint was gated on the streaming phase alone, so it was live between Send and the first token — where CreateCheckpoint refuses with FailedPrecondition. Gated on the whole active turn. |
| 🟠 Medium | ChatTranscript.tsx |
Each group was a fragment keyed by its first message. The grouping changes when the boundaries land, so two groups becoming one retired a key and React remounted the messages under it — every rendered mermaid diagram flashed back to loading. One flat run keyed by message id. |
| 🟠 Medium | chat.spec.ts |
I had converted a strict assertion into a 20s poll and blamed the checkpoint read. The real cause was the remount above; the poll was hiding it. Strict again. |
| 🟠 Medium | AgentChatPage.tsx |
A checkpoint that saved but whose list re-read failed reported Could not checkpoint — over a line already on screen. The re-read is outside the try. |
| 🟠 Medium | grpc/operations.ts |
ListCheckpoints read one page and dropped nextPageToken; the controller defaults to 50. Past that the oldest boundaries silently stopped drawing lines. It follows the token. |
| 🟠 Medium | CheckpointDivider.tsx |
role="separator" puts its children on ARIA's presentational list, which pruned the label and the only fork control in the application. A labelled group instead. |
| 🟠 Medium | mocks/state.ts |
Fixture checkpoints lived in a module array while transcripts persist in sessionStorage, so a checkpoint vanished on reload and the messages it marked did not. The fixture contradicted the controller. Persisted. |
| 🟠 Medium | mocks/state.ts |
A second seeded boundary shared a headTaskId with the first, so one line was drawn for two and its Fork silently targeted one of them. Different turn. |
| 🟠 Medium | AgentChatPage.tsx |
A fork inherited its source's marks: they are keyed by message id and a fork gets copies of those messages under the same ids. Now held against the conversation they belong to. Guarded by a test I watched fail without the fix. |
| 🟡 Low | ChatComposer.tsx |
The tooltip sat on a disabled antd button, which fires no pointer events — invisible in the one state where it explains itself. |
| 🟡 Low | domain/checkpoints.ts |
canForkFrom was exported and never called. Deleted. |
| 🟡 Low | rail | Focus rings sliced flat on three controls, each by a different clipping ancestor. |
✅ What I checked
- Fork truncation, on a cluster. Forking turn 1 of a five-turn chat gives a chat that knows only the first fact; forking turn 4 knows all four. Not inferred from the SQL — driven in the browser and read back from the agent.
- Duplicates diverge. Two copies of one fork, given different favourite cities, each keep their own after two further messages.
- Marks survive a reload. Three lines saved, reloaded, still three, and the button still refuses a fourth at the same boundary.
- A fork inherits nothing. Reproduced the leak first (fork showed 1 line with 0 checkpoints), then fixed it, then reverted the fix to watch the new test fail.
- Deleting releases the snapshot.
CreatecallsCreateTag,DeletecallsDeleteTag; nothing else does. Verified againstservice.go. - Both themes, every new control. Photographed at rest, hover, press and focus.
- The collapse order. Measured at seven widths in both directions: agent panel 1440, sidebar
xl, rail 1040. - Control alignment under both scrollbar modes, including
--disable-features=OverlayScrollbar, which is the only way the misalignment reproduces. - CI green, including the lint error I had been misreading locally — I was treating "0 errors and 1 warning potentially fixable" as the total.
Worth a reviewer's judgement
- The fan-out. One request per conversation to draw Snapshots. #2783 has the RPC it needs.
NotFoundfor a forked snapshot. The controller refuses to delete one a chat was forked from and reports it as not found. The page states the condition; correcting the error is a backend change.- Toasts sit over the composer.
position="bottom-right"puts them on Send and Checkpoint, so a click is intercepted while one shows. Pre-existing chrome, but this branch made it easy to hit. - Repeated forks are all named
X (fork). Distinguishable only by age in the rail.
🤖 written by Claude
Closes #2783. The Snapshots page read every checkpoint of every conversation and then filtered, sorted and paged them in the browser, so the first paint waited on the whole history and a search reported "no matches" about rows it had already been handed. ListCheckpoints now takes a filter, an ordered list of sort columns and an offset, and answers with the count the filter matched. `agent_instance_id` became optional so one request can span a caller's conversations. The store runs both as one page query plus one count; the ordering is passed as parameters rather than concatenated in, which keeps the statement preparable by TestInlineSQLPrepares and leaves no ORDER BY to inject into. A row is named, filtered and ordered by `conversation_name`, the name recorded when the snapshot was taken. That is the only name a query can reach — a conversation's current name lives inside an encoded payload — and showing the current one would give a search that misses the words on screen. The chat still needs every boundary of its own conversation to draw its dividers, so that caller walks the pages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
b183cdf to
3d8cc79
Compare
…eckpoints Signed-off-by: Nicholas Bucher <behappy54321@gmail.com> # Conflicts: # go/core/internal/database/client_agent_instance_test.go # go/core/internal/service/checkpoint/service_test.go
The conversations were read inside the snapshots query, so every page turn, every
sort and every keystroke re-read them alongside the page: turning a page cost
three requests and typing a five-letter search cost fifteen. They do not depend on
the query — they only decide which names link anywhere — so they now have their own
key and are read once.
The filter is debounced for the same reason, now that it travels to the controller.
`useDebounced` moves out of SubstratePage, which had it for this exact problem.
Also restores the page's empty state: the shared rule reads a `{snapshots, total}`
object as "not empty", so "No snapshots yet" could never show.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
|
Note Outdated. The Snapshots page moved to #2798. The original commentSnapshots page against a real cluster, 31 snapshots over two pages. snapshots-server-side.mp4
Turning a page, sorting, and a five-letter search are one 🤖 written by Claude |
Fixes four things found reviewing the query this PR added. The filter interpolated raw input into `ILIKE '%' || $3 || '%'`, so a search for `%` matched every row and `user_id` matched anything with `user`, any character, `id`. It is `position(lower($3) IN ...)` now — a plain substring, nothing to escape. The page and its count were two statements; `count(*) OVER ()` folds them into one. An offset past the end answers with no rows and no total, which the page reads as its cue to go back to the first. Nothing indexed `user_id`, so every listing was a sequential scan of the whole table. `(user_id, state, id DESC)` covers it, folded into the unreleased baseline migration as that file asks. Sorting is one column, not a list of them: the UI sends one, and the six paired CASE expressions that supported three were most of the statement's length. The sort by state goes with them — a listing is READY checkpoints only, so it ordered every row by the same value. Also bounds the decoded page token, which reached the store as an int32 and would have wrapped into a negative OFFSET. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
The store test seeded an agent_instance row and its encoded payload; the listing reads neither, and a retained checkpoint outlives its conversation anyway. The page offset was checked for being negative twice, once inside the token decoder and again in the caller that bounds it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Migration files are immutable once merged and this one is already on main, so the index needs its own migration rather than a line in the baseline. Left for the follow-up: without it the listing is a sequential scan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
|
Note Outdated. The Snapshots page moved to #2798. The original commentWhat the review changed, against a real cluster with 34 snapshots. review-fixes.mp4
The index that would keep this off a sequential scan is not here — migration files are immutable once merged, so it needs its own. Tracked in #2783. 🤖 written by Claude |
|
Note Outdated. The Snapshots page moved to #2798. The original commentInjection payloads, and the view living in the address bar. injection-and-url-state.mp4
Every value reaches Postgres as a bind parameter — the statement is one literal with 🤖 written by Claude |
This PR is the chat side: checkpoint a turn, fork from a checkpoint, duplicate a chat, and the rail work that came out of using it. The Snapshots page needs a `ListCheckpoints` that pages, searches and sorts across conversations, which is a proto and store change, and neither belongs in a UI review. The checkpoint client keeps the shape the chat needs: one call, one conversation, every boundary. The stacked PR widens it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
|
The Snapshots page moved to #2798, stacked on this branch. This PR is now the chat side only: checkpoint a turn, fork from a checkpoint, duplicate a chat, and the agent rail work. No proto, no Go — 30 files, all under #2798 carries the page itself, the Merge this one first; GitHub retargets #2798 to The reels above are marked outdated for that reason — the ones about snapshots belong with the other PR now. 🤖 written by Claude |
|
Checkpoint and fork against a real cluster, with a real agent answering. checkpoint-and-fork.mp4One conversation, two checkpoints three turns apart, then a fork from each:
Neither fork shows the source's lines as its own — a fork reads its own boundaries. 🤖 written by Claude |
🤖 written by Claude (start)
Checkpoint a turn in a chat, fork from a checkpoint, duplicate a chat, and the agent rail work that came out of using it.
The Snapshots page and the
ListCheckpointschange it needs are stacked on top, in #2798.Checkpoints and forks
CreateCheckpointtakes no cutoff — what it saves is always "as it stands now" — and it is disabled during a turn and once the latest boundary is saved.Forkon it. The line is the promise: everything above it travels into a fork and nothing below it does. Matched byheadTaskIdagainst the turn each message carries, so the lines survive a reload.Duplicate chatin each rail row's menu is a checkpoint of the conversation plus a fork of it, and the copy opens.The agent rail
Rows gained the room and the states they were missing: a menu on every row carrying Chat details and Share chat as well as rename, duplicate and delete; checkbox hover and press states; controls that line up whether or not the list scrolls. The three columns fold as the window narrows — the agent panel at 1440, the application sidebar at antd's
xl, the rail at 1040 — and unfold again. The agent panel's drag handle is gone withResizableAside: its wrapper clipped to a fixed width, so dragging widened an aside inside a box that never grew.The create buttons on the list pages are title-cased, which is unrelated to the rest and was asked for separately.
Testing
ENABLE_MOCK_UI=true yarn dev, open a conversation, and press Checkpoint beside Send. A line appears under the last turn and is still there after a reload.Forkon an early line. The fork opens holding what was above it and nothing below.Duplicate chat. The copy opens with the whole transcript.The reel in the comments shows this against a real cluster: two checkpoints three turns apart, a fork from each, and the earlier fork knowing only what was above its line. Reels about the Snapshots page moved with it to #2798 and are marked outdated here.
🤖 written by Claude (end)