Build out the Builder: activity log, notifications, anchored placement, and a rebuilt launch dialog - #334
Merged
Merged
Conversation
…d output placement The Builder's tool-use loop had no durable record of what a build did: every tool call and its result vanished into the loop's in-memory message list. A build that paused or failed announced nothing unless the user happened to be looking at the canvas. New output (parentless node creations) landed near the canvas origin rather than near the build that produced it. This adds: - An activity log on the plan node (PlanState.builder_activity): one row per tool call the loop invokes, written at its single registry.invoke() choke point, so every call - including the four in-band control tools - is recorded in order with its outcome and timing. A capped ring buffer bounds its growth; it rides the existing whole-node scene patch mechanism, so no new wire plumbing was needed. It is deliberately untouched by undo: this is run telemetry, not document content, and the record of what happened must survive reverting what happened. Persisted with the rest of the plan node's state so it survives an app restart. - Notifications when a build lands done, paused, or failed (success/ warning/error respectively). "stopped" is excluded - the user just clicked Stop and is necessarily present. - Anchored placement: a build's parentless node creations (a note, a from-scratch chat or code node) now land near the plan node that launched them instead of scattering near the canvas origin, reusing the run context's existing plan-node reference rather than adding a new one. - builder/deleteRecipe, rounding out the recipe lifecycle - saveRecipe already existed with no way to remove what it saved. Every change here is confined to the loop, its wire contract, and session persistence; the loop's control flow, budget enforcement, and approval routing are unchanged.
The plan node had no way to show the activity log added in the previous commit. This wires PlanState.builder_activity through SceneCanvas into the plan node's data and renders it as a collapsed-by-default disclosure, reusing ChatNodeView's existing "an assistant turn's tool calls" pattern (.chat-node-tool-invocations) rather than a new widget, since the content is the same shape - a tool name, an outcome, one block of detail text - just scoped to a whole build instead of a single turn, with a bounded, independently scrollable list to hold the larger row count. The log auto-scrolls to its newest row while the build is running and the disclosure is open. Verified against a real running backend (a scripted provider driving a real build through the real WS/document/wire path, approval gate included): the activity count, error count, per-row tool name, outcome tint, and summary text all matched the calls made; the completion notification carried the build's own finish summary; and the two created notes landed exactly where the previous commit's anchored-placement fan- out predicts, relative to the plan node.
The launch dialog was the last surface in the app still using a bare native <select>, and its three budget fields were raw, unlabeled numbers with an input bug: typing 0, or clearing a field to retype it, silently snapped back to the default on every keystroke because the coercion used `Number(value) || DEFAULT`, and 0 is falsy. - The recipe picker is now CustomSelect, matching every other dropdown in the app. - Selecting a recipe previews its own steps before launch, not just its description - the backend payload already carried them; nothing rendered them. - The three budgets are now named presets (Quick / Standard / Extended) with a plain-language summary line, with the exact numbers moved behind an "Advanced" disclosure for anyone who wants them. Those fields now clamp to their valid range on blur instead of coercing while typing. - builder/deleteRecipe (added in the first commit of this change) now has a UI: a saved (non-built-in) recipe can be removed from the picker. - Launching a build - from scratch or from a recipe - now centers the viewport on the new plan node. The launcher has no canvas anchor of its own, so the node could previously land anywhere the scene's extent happened to place it, often off the visible viewport entirely. Verified against a real running backend end to end, including the exact input sequence that reproduces the old 0-coercion bug (typed "0" into Max steps, tabbed away, confirmed it settles on 1 - the clamped minimum - not back on the default).
The Builder had no documentation anywhere in the app - it appeared in neither the Help Center nor the first-run onboarding wizard, so its only self-explanation was a single placeholder sentence in its launch dialog's goal field. For the most capable feature in the app, that made it the least explained. Adds a full "Builder" section to the Help Center (how a build works: the goal-plan-build flow, what it can actually do, the two oversight modes, and budgets; the plan node: watching a build run, pause/stop/resume, undoing a build, and recipes) and one sentence to onboarding's first step naming the Builder and what it does, alongside the existing branching explanation.
…iew findings An adversarial review pass across the Builder build-out branch surfaced 13 confirmed findings; this fixes all of them. Correctness: - Undo/"Undo this build" could silently erase activity rows and step completions logged after a mid-run builder.replan call. A replan records a command that snapshots the whole plan node at that instant (the app's existing, deliberate whole-node undo model); undoing that command restored the node wholesale, discarding everything logged afterward - directly contradicting the activity log's own documented "survives an undo" guarantee. Fixed the same way the existing builder_status review-fix handles the identical class of hazard: restoring a snapshot now always carries the CURRENT activity log forward rather than trusting the snapshot's stale copy. - A build's parentless node creations (anchored near the plan node) and an explicit parent_id equal to the plan node's own id (which the executor prompt hands the model directly) used two different sibling-counting mechanisms for the same row - one by position, one by graph edge - so interleaving the two could place two nodes on top of each other. Unified to the position-based count specifically when parent_id is the anchor; an ordinary parent is unaffected. Robustness: - A tool call's name has no upstream length validation and was stored in the activity log uncapped, unlike every other field on that row - defeating the size-bounding the ring buffer and summary cap exist for. Now truncated the same way. - Restoring a saved session's activity log crashed on a non-numeric elapsedMs instead of degrading to 0 the way every other field on that row already tolerates malformed input. Also fixed: the launch dialog's recipe-delete flow ignored whether the delete actually succeeded, silently treating a backend refusal or a network failure as success; a CSS float in the activity log's layout replaced with the flexbox idiom used everywhere else in the app, incidentally making a previously-inert CSS class real. Coverage added for all of the above, plus the ring buffer's exact trim boundary, summary/tool-name truncation, malformed session data, the plan node's running-and-open-only auto-scroll, an empty recipe step list, and the budget preset control's behavior once its fields have been hand-edited away from any preset. Full local suites verified: 3020 backend tests, 1944 frontend tests, zero lint errors, build and bundle size clean.
test_activity_tool_name_and_summary_are_both_truncated_when_over_cap referenced ToolResult without importing it. At runtime the resulting NameError was silently swallowed by ToolRegistry.invoke's own generic exception handler and converted into an error result - whose error text happens to also be long and gets truncated the same way a real success result would, so the test passed without ever exercising the success path it was meant to cover. Caught by CI's ruff check (F821, undefined name), not by the test itself. Fixed the import and added an explicit outcome == "ok" assertion so a future regression back to the error path fails loudly instead of silently passing for the wrong reason.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Builder's engine (planning, budgets, approval routing, undo, resume) is substantial, but its surface was a prototype: builds had no visible activity log, a landed build announced nothing unless the user happened to be looking, output could land anywhere on the canvas, the launch dialog was the last surface in the app using a bare native
<select>with an input bug in its budget fields, and the feature was undocumented anywhere in the app.Change
0, or clearing a field to retype it, silently reverted to the default); a saved recipe can now be deleted.parent_idequal to the plan node's own id used two different sibling-counting mechanisms and could overlap. Also fixed: an uncapped tool-call name field, a delete-recipe flow that ignored whether the delete actually succeeded, and a stray CSS float.Test plan
0, tabbed away, confirmed it settles on the clamped minimum)