Design: Support select-and-regenerate flows with candidate previews#2187
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🔴
Review Details
Code Review Summary
This update adds scoped selection reprompts for Design: captured node context is routed through ask/preview modes, candidate HTML is stored as an application-state proposal, rendered reversibly in the iframe, and accepted or rejected through a version-checked action. The overall decomposition is sensible, with strong subtree resolution, access checks, optimistic version validation, and focused tests around proposal validation, bridge targeting, and navigation. I classified this as Standard risk because it combines shared state/mutation logic with substantial editor UI and bridge changes.
Key Findings
- 🔴 HIGH — The reprompt action guard can be bypassed when thread lookup fails, and its allowlist permits
resolve-node-rewrite, so an agent turn can potentially persist a candidate without a separately verifiable user acceptance. - 🟡 MEDIUM — Client-side proposal and pending-state writes are unconditional and can race with resolution, refinement, or newer requests, resurrecting stale proposals or deleting newer work.
- 🟡 MEDIUM — Candidate HTML is stored in application state at up to three 250 KB variants, conflicting with the template storage contract and increasing state/query load.
- 🟡 MEDIUM —
view-screenperforms one application-state read per file, creating an avoidable N+1 path for multi-screen designs.
The iframe preview implementation includes restoration markers and source/version checks, but the action/state boundaries need to be made fail-closed and conditional before this flow is dependable under concurrency.
🧪 Browser testing: Will run after this review (PR touches UI code)
| if (context?.caller === "tool" && context.threadId) { | ||
| const thread = await getThread(context.threadId); | ||
| if (thread) { |
There was a problem hiding this comment.
🔴 Fail closed when reprompt context cannot be loaded
If getThread(context.threadId) fails or returns no thread, the guard silently proceeds to entry.run, allowing content-writing tools to bypass the [Reprompt selection] preview-only restriction. Treat an unverifiable thread as an error (or otherwise fail closed) before invoking the action.
|
Here's a visual recap of what changed: Open the full interactive recap |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🔴
Review Details
Incremental Code Review Summary
The latest commits substantially improve the previous lifecycle issues: proposal keys are now scoped by reprompt ID, view-screen uses design-scoped state scans, candidate payloads have a serialized size cap, and cancellation/refinement cleanup uses compare-and-set. I verified and resolved seven previously open comments covering preview acceptance, stale proposal resurrection, newer-request cleanup, failed refinement cleanup, orphaned pending state, per-screen reads, and oversized state payloads. The fail-closed context comment remains open and was not reposted.
Risk assessment: Standard. The remaining concerns are concurrency bugs in the accept and cleanup paths, where compare-and-set checks are not composed into the content-write/cleanup transaction.
New findings
- 🔴 HIGH — Accept can persist an older proposal after a newer request replaces the pending state.
- 🔴 HIGH — Cleanup ignores a failed pending compare-and-set and can report success while leaving inconsistent state.
- 🟡 MEDIUM — Prior proposal cleanup ignores compare-and-set failure, allowing stale records to accumulate.
The added application-state helpers and targeted tests are directionally sound, but these operations still need an atomic ownership/transition protocol before the design write and paired cleanup are safe.
🧪 Browser testing: Will run after this review (PR touches UI code)
| source, | ||
| fileType: file.fileType, | ||
| }); | ||
| const write = await writeInlineSourceFile({ |
There was a problem hiding this comment.
🔴 Reserve the pending request before accepting a proposal
The action validates the current pending record, then performs the version-checked content write without atomically claiming that pending request. A newer refinement can replace the pending state after line 180 and before this write, allowing the older proposal to persist over the newer request. Establish ownership with a compare-and-set transition (or equivalent serialized critical section) before writing content, and abort if the request was superseded.
| const [proposalCleared] = await Promise.all([ | ||
| compareAndSetAppState( | ||
| proposalKey, | ||
| proposal as unknown as Record<string, unknown>, | ||
| null, | ||
| ), | ||
| compareAndSetAppState(pendingKey, pending, null), | ||
| ]); | ||
| if (!proposalCleared) { | ||
| throw new Error("Node rewrite proposal changed while it was resolving."); | ||
| } |
There was a problem hiding this comment.
🔴 Handle pending compare-and-set failure during cleanup
clearProposalState checks only proposalCleared; it ignores the result of the pending-state compare-and-set. If the pending record changes concurrently, the action can return success after clearing only the proposal and leave the current pending request behind. Check both CAS results and fail without claiming the paired cleanup succeeded when either record changed.
| if (pending.priorProposalId && pending.priorRepromptId) { | ||
| const priorProposalKey = designRepromptProposalStateKey( | ||
| file.designId, | ||
| file.id, | ||
| pending.priorRepromptId, | ||
| ); | ||
| const priorProposal = await readAppState(priorProposalKey); | ||
| if (priorProposal?.proposalId === pending.priorProposalId) { | ||
| await compareAndSetAppState(priorProposalKey, priorProposal, null); |
There was a problem hiding this comment.
🟡 Handle failed cleanup of a prior proposal
The superseded prior proposal is removed with compareAndSetAppState, but its boolean result is ignored. If that CAS loses to a concurrent accept/reject, the new proposal is still published while the old record remains, allowing stale state to accumulate and making later proposal discovery ambiguous. Handle the failed CAS explicitly or use an atomic supersession transition.

Uh oh!
There was an error while loading. Please reload this page.