Update from the Builder.io agent - #2443
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.
|
Here's a visual recap of what changed: Open the full interactive recap |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Code Review Summary
PR #2443 adds an action and client-side bridge to mark generated workflow cards as failed when an agent run stops, with access checks, request-timestamp guards, and focused unit/component tests. The overall approach is sound: it avoids overwriting terminal or newer workflow state and uses the existing action/application-state surfaces. This is a standard-risk state-management change.
Key Findings
Medium issues:
- The bridge records a timestamp that does not match the timestamp stored in workflow state, so the normal cancellation path is rejected as a newer request.
- Supported MCP host-chat completion events can omit
tabId, leaving those workflow cards stuck ingenerating.
✅ Good patterns include scoped assertAccess, preserving terminal state, guarding against newer requests, and adding regression tests for the matching-tab flow.
🧪 Browser testing: Will run after this review (PR touches UI code)
| const handleChatRunning = (event: Event) => { | ||
| const detail = (event as CustomEvent).detail; | ||
| if (detail?.isRunning !== false || typeof detail.tabId !== "string") |
There was a problem hiding this comment.
🟡 Reconcile host-chat runs without tab IDs
This handler drops every stopped event without a tabId. The direct MCP host-chat path is supported by sendToAgentChat and emits { isRunning: false } without a tab ID, so workflow generations started there cannot be reconciled and remain generating after a stopped run. Preserve/correlate the generated tab ID through that event or handle the host completion path with equivalent per-request correlation.
Additional Info
Found by 1 of 2 code-review agents; validated against the supported host-chat event shape.
There was a problem hiding this comment.
I did not change this. The no-tabId event is emitted by sendToAgentChat when a direct MCP host message handoff settles (agent-chat.ts:995-1007), not when the host agent run stops. Treating it as cancellation would mark a valid workflow failed immediately after successful handoff, while the host may still be generating. Exact run cancellation cannot be safely correlated from that legacy event shape, so the current tab-scoped handling remains the correct behavior.
|
@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable) |
|
Review summary triaged: the timestamp mismatch was valid and is fixed in b5f14aa with regression coverage; the no-tab MCP handoff suggestion was not applied because that event marks handoff settlement rather than agent-run cancellation and would prematurely fail valid generations. Detailed evidence is posted on each inline thread. |
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable) tests still failing friend |
|
@steve8708 Fixed the CI failure in 937e90b. The red aggregate check traced to Calendar lane 4: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
There was a problem with your request, please try again later. Error id: |
|
Latest review summary addressed in 6645395: workflow cancellation correlation is now persisted before dispatch and remains recoverable after reload/remount. The inline thread contains the implementation and verification details. This push restarts the CI and 20-minute quiet window. |
|
There was a problem with your request, please try again later. Error id: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Latest review summary addressed in 12d755d: workflow tracking/stop writes are atomic, stale or rejected tracking cannot dispatch, and the failing Brain privacy eval was stabilized by matching full PII values instead of random numeric metadata fragments. Verification: 15 focused Clips tests pass, Clips typecheck passes, and Brain privacy eval passes repeatedly. Detailed responses are on both inline threads; this push restarts CI and the quiet window. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Latest review summary addressed in 121446c: queued workflows survive synchronous dispatch failure, and application-state connection errors are no longer reported as missing values. Verification: 17 focused Clips tests, 13 core application-state tests, Clips/core typechecks, and formatting all pass. Detailed responses are on both inline threads; CI and the quiet window restart from this head. |
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Incremental Code Review Summary
This revision fixes the prior request-consumption ordering issue by moving workflow request deletion into a separate consume operation that runs only after sendToAgentChat returns, and core application-state reads now propagate connection failures instead of treating them as missing. Those prior comments were verified fixed and resolved. The embedded host-chat/tabless-event issue remains open and was intentionally not reposted.
New Findings
Medium issues:
- If the asynchronous consume operation fails twice, the request remains persisted while the mounted bridge records its dispatch key, so no further consume retry occurs; a reload can then redispatch an already-started workflow.
sendToAgentChatdoes not confirm that the chat receiver accepted the message, but the bridge consumes the only queued request immediately afterward. A dropped or not-yet-mounted receiver can leave the workflow permanentlygeneratingwithout a stop event.
The CAS state transitions, shared timestamps, and read-failure propagation are otherwise sound. Risk remains standard for this state-management integration.
🧪 Browser testing: Skipped — Clips remains unavailable for reliable verification because list-recordings continues to fail with a Drizzle stack-overflow error
| requestedAt: request.requestedAt, | ||
| tabId, | ||
| }; | ||
| void callAction( |
There was a problem hiding this comment.
🟡 Retry failed workflow-request consumption
After dispatch succeeds, the bridge records dispatchKey before the asynchronous consume call. If both consume attempts fail, the queued request remains in application state but the in-memory set prevents another consume attempt for the lifetime of this bridge; after reload, the still-queued request can be dispatched again, creating a duplicate agent run. Keep a retryable pending-consume state or schedule further consume retries without redispatching.
Additional Info
Found by 1 of 2 incremental agents; confirmed from the dispatch-key lifetime and two-attempt consume chain.
There was a problem hiding this comment.
Fixed in fabc5f6. Successful chat delivery is now persisted on the queued request before cleanup. If consume fails or returns a non-success result, the bridge schedules another tick; after reload, a request with deliveredTabId is consumed without calling chat again. Added a remount regression proving durable cleanup does not redispatch.
There was a problem hiding this comment.
Follow-up: 8feb080 closes the remaining retry gap from my earlier fix. A durable delivered request is deduped immediately and handed to a dedicated idempotent consume loop that retries action errors/non-successes; reload also detects deliveredTabId and resumes consume without chat redispatch.
There was a problem hiding this comment.
Final follow-up in b77a85a: consume retries are now bounded correctly—errors and CAS-stale outcomes retry, permanent structured outcomes exit. The delivered marker remains durable across reload, so cleanup resumes without redispatch, while no helper can spin forever.
|
Latest review summary addressed in fabc5f6: workflow chat delivery is acknowledged before queue consumption, acknowledged requests carry a durable delivery marker, and cleanup retries after reload without duplicate generation. Verification: 19 focused Clips tests, Clips typecheck, and formatting pass. Detailed responses are on both inline threads; CI and the quiet window restart from this head. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Latest review summary addressed in 8feb080: workflow claims are exclusive across tabs, rejected delivery releases the exact claim, and delivery-marker/consume cleanup retries independently until idempotent completion. I also posted a follow-up on the previously open consume-retry thread. Verification: 21 focused Clips tests, Clips typecheck, and formatting pass. CI and the quiet window restart from this head. |
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🔴
Review Details
Incremental Code Review Summary
This revision fixes the prior claim-overwrite and delivery-marker retry issues: tracking now rejects a different existing claim, and delivery/consumption work is retried through shared helpers. Those comments were verified fixed and resolved. The host-chat tabless-event and consume-retry comments remain open and were intentionally not reposted.
New Findings
High:
- The shared
retryWorkflowActionloops forever when an action returns a structured failure such asstale,claimed,different-run, ornot-delivered, because it only exits on the success boolean. This can leave background work spinning indefinitely and, for awaited release operations, stall the bridge.
Medium:
- A persisted claim can become abandoned if the browser closes or crashes after
tracksucceeds but before delivery. Later bridges reject the request asclaimed, and no chat stop event exists to release it, leaving the workflow permanently generating.
The CAS and delivery-confirmation approach is otherwise sound. Risk remains standard for this state-management integration.
🧪 Browser testing: Skipped — Clips remains unavailable for reliable verification because list-recordings continues to fail with a Drizzle stack-overflow error
| for (;;) { | ||
| try { | ||
| const result = (await callAction( | ||
| "reconcile-workflow-generation" as any, | ||
| request as any, | ||
| )) as Record<string, unknown>; | ||
| if (result[successKey] === true) return; | ||
| } catch {} | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Exit retry helper on structured action failures
retryWorkflowAction only returns when result[successKey] === true; it ignores structured failure reasons such as stale, claimed, different-run, and not-delivered, so it loops forever. This can leave background work spinning indefinitely and can stall the awaited release path after delivery rejection. Handle unrecoverable reasons explicitly or bound the retry with a recoverable failure result.
Additional Info
Found by 1 of 2 incremental agents; confirmed against all failure branches in reconcile-workflow-generation.ts.
There was a problem hiding this comment.
Fixed in b77a85a. The retry helper now retries only thrown errors and structured stale results; any other structured reason exits with false, so permanent outcomes such as claimed/different-run/not-delivered cannot loop forever or stall release. Added a regression where release returns different-run and is called exactly once.
| if (operation === "track") { | ||
| if (state.tabId === tabId) { | ||
| return { reconciled: false, tracked: true }; | ||
| } | ||
| if (state.tabId) { |
There was a problem hiding this comment.
🟡 Recover abandoned workflow claims
A successful track persists a tab claim with no lease or recovery path. If the browser closes or crashes after this claim succeeds but before sendToAgentChatAndConfirm, the queued request remains undelivered while every later bridge receives reason: "claimed"; no chat stop event exists to release the claim, so the workflow can remain generating indefinitely. Add an expiring claim or a durable takeover/release mechanism for abandoned runs.
Additional Info
Found by 1 of 2 incremental agents; distinct from the fixed same-time duplicate-claim overwrite issue.
There was a problem hiding this comment.
Fixed in b77a85a. Workflow claims now persist claimedAt and are exclusive for a 30-second lease. A later bridge rejects an active claim but can atomically recover and replace an expired claim after a tab closes/crashes. Rejected delivery removes both tabId and claimedAt. Added active-claim rejection, expired-claim recovery, and release tests.
|
Latest review summary addressed in b77a85a: workflow retry loops terminate on permanent structured failures, retry transient/stale outcomes, and exclusive claims now have a 30-second crash-recovery lease. I posted a final update on the long-running consume-retry thread as well. Verification: 22 focused Clips tests, Clips typecheck, and formatting pass. CI and the quiet window restart from this head. |
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
This revision adds a 30-second claim lease and recovery of expired workflow claims. The previously reported abandoned-claim issue is partially addressed for claims with valid claimedAt timestamps. The host-chat tabless-event, consume-retry, and structured retry-helper comments remain open and were intentionally not reposted.
New Finding
Medium: claim recovery fails open for a persisted claim with a missing or malformed claimedAt. Because Date.parse yields NaN and the code only blocks when the parsed value is finite and recent, any legacy or corrupted state containing tabId but no valid timestamp is immediately reclaimable by another tab. That can reintroduce duplicate dispatches. Claims without a verifiable timestamp should be treated as active or otherwise require an explicit recovery path.
The valid-timestamp lease path, CAS transitions, and delivery flow are otherwise sound. Risk remains standard for this state-management integration.
🧪 Browser testing: Skipped — Clips remains unavailable for reliable verification because list-recordings continues to fail with a Drizzle stack-overflow error
| if (state.tabId) { | ||
| const claimedAt = Date.parse(state.claimedAt ?? ""); |
There was a problem hiding this comment.
🟡 Fail closed for claims without a valid timestamp
When state.tabId exists but claimedAt is missing or malformed, Date.parse returns NaN, so the finite-and-recent check is skipped and the claim is immediately replaced. This is possible for persisted state created by an earlier version or corrupted state, and can allow a second tab to dispatch the same workflow while the original owner is still active. Treat an unverifiable claim as active, or require an explicit recovery path rather than reclaiming it automatically.
Additional Info
Found by 1 of 2 incremental agents; distinct from the existing lease-expiry/abandoned-claim review thread.
There was a problem hiding this comment.
Fixed in 98aed73. Claim validation now uses claimedAt with the workflow requestedAt as the legacy fallback, and treats any still-invalid timestamp as an active claim rather than replacing it. Valid expired leases remain recoverable. Added a malformed-timestamp regression test; 23 focused tests and Clips typecheck pass.
|
Latest review summary addressed in 98aed73: malformed workflow claim timestamps now fail closed, while legacy claims use the request timestamp for bounded recovery. Verification: 23 focused Clips tests, Clips typecheck, and formatting pass. The inline thread has details; CI and the quiet window restart from this head. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Latest review summary addressed in a983508: delivery confirmation renews the exact active lease and marks the queue atomically, and stopped-run reconciliation now retries transient failures through the bounded structured policy. Verification: 23 focused Clips tests, Clips typecheck, and formatting pass. Detailed responses are on both inline threads; CI and the quiet window restart from this head. |
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Code Review Summary
The latest revision strengthens delivery ownership by atomically comparing the workflow claim and queued request with compareAndSetManyAppState, and it now retries stop reconciliation through the shared retry helper. Those changes address the two previously reported issues, so their review threads were resolved. The application-state error-propagation change remains conceptually correct, and the lifecycle tests cover the main CAS and lease paths. This remains a standard-risk change because it coordinates shared persistent state with asynchronous agent-chat execution.
Key Findings
- 🟡 MEDIUM — The
chatRunning: falseevent is treated as terminal, but the chat reconnect/auto-continue path can emit the same event before resuming the run. This can mark a still-running workflow failed. - 🟡 MEDIUM — Delivery confirmation refuses to mark the queued request once the workflow state has become terminal, leaving the request queued forever after a fast completion or stop race.
Existing unresolved comments were not reposted. 🧪 Browser testing: Will run after this review (PR touches UI code).
| if (detail?.isRunning !== false || typeof detail.tabId !== "string") | ||
| return; | ||
|
|
||
| const recordingId = recordingIdFromTab(detail.tabId); |
There was a problem hiding this comment.
🟡 Distinguish reconnect handoffs from terminal chat stops
agentNative.chatRunning with isRunning: false is not necessarily a terminal stop: the chat reconnect/auto-continue path emits the same event before resuming the existing run. This handler immediately calls stop, so a reconnect can change the still-running workflow from generating to failed, after which a replacement run may race with the recovered agent. The event payload or handler needs to distinguish a terminal stop from a reconnect handoff before applying the failure transition.
Additional Info
New issue found by 1 of 4 agents; existing host-chat tabless and stopped-retry comments were not reposted.
There was a problem hiding this comment.
Fixed in 11ec909. The shared AssistantChat now labels only the explicit user Stop path with reason: "stopped"; reconnect/auto-continue and ordinary terminal handoffs remain unlabeled. Clips reconciles failure only for that explicit reason, so reconnect false-events cannot fail an active workflow. Added a matching-tab reconnect regression.
| `Invalid generated workflow state for ${recordingId}`, | ||
| ); | ||
| } | ||
| if (parsedState.data.status !== "generating") { |
There was a problem hiding this comment.
🟡 Consume delivery markers after fast terminal completion
mark-delivered now validates the workflow claim, but it rejects the request when the workflow state is already terminal. Because sendToAgentChatAndConfirm only confirms submission and the marker is persisted asynchronously, the agent can write ready (or the stop handler can write failed) before this action reads the state. The request then remains in clips-ai-request-*; subsequent reloads keep retrying reconciliation without ever consuming it. Permit a terminal state with the matching requestedAt/tabId to finish the delivery marker, or atomically consume the already-confirmed request.
Additional Info
New issue found by 1 of 4 agents; distinct from the already-resolved lease-binding comment because this is the terminal-state race after ownership validation.
There was a problem hiding this comment.
Fixed in 11ec909. If the matching workflow is already terminal when mark-delivered runs, the action atomically consumes the still-matching queued request and returns idempotent success instead of leaving it eligible for redispatch. Added a fast-ready-before-marker regression test.
|
Latest review summary addressed in 11ec909: Clips distinguishes explicit user Stop from reconnect handoffs, and fast terminal completion atomically consumes its queued request. Verification: 24 focused Clips tests, Clips/core typechecks, and formatting pass. The prior Content parity failure was an Electron postinstall network |
|
@shomix can you give this a review when you have a sec? |
3 participants
The Builder.io agent is generating a detailed description...
tag @BuilderIO for anything you want the Builder.io agent to do
To clone this PR locally use the GitHub CLI with command
gh pr checkout 2443