Repository navigation
fix(flows): canvas Accept persists the proposal (accept = review + save) - #4893
Conversation
The canvas copilot's own inline Accept/Reject card only applied a proposal to the local draft graph and never persisted anything — the user had to separately notice and click the header Save icon. In practice they didn't: a flow could be "accepted" in the copilot and still persist as an empty trigger-only stub, with the agent chat stuck on "say save it" (confirmed live, Batch A5). handleAcceptProposal is now async: it applies the proposal (and adopts the name into the title, per tinyhumansai#4886) and immediately calls handleSave to persist, bypassing the (mid-remount, stale) canvasRef. A failed save is non-fatal — the draft stays applied and dirty, with the header Save button as the manual retry. handleSave gains an optional overrideName param: handleAcceptProposal calls setName(proposal.name) and handleSave in the same handler, so `name` in handleSave's closure is still stale (pre-render) — overrideName sidesteps that instead of waiting for a render that hasn't happened yet. WorkflowCopilotPanel's Accept button awaits the host's onAccept, showing a "Saving…" state and disabling itself meanwhile; a rejected promise leaves the proposal card up for retry instead of silently clearing it. Reject is unchanged (local-only revert). Deferred: subscribing the canvas to useFlowChanged so an agent-side save_workflow refetches and syncs live — out of scope here, revisit separately.
…eview) Address architect-review findings on the accept-saves change: - F1 (HIGH): handleAcceptProposal swallowed a failed accept-triggered save, so onAccept always resolved and the copilot panel's clearProposal() always ran — a real save failure silently vanished the proposal card with no way to retry. Fixed in the prior commit's rethrow; this commit adds the regression test proving it (fails without the rethrow, passes with it). - F2 (MEDIUM): the server-name resync in handleSave only updated `name`, dropping the `setTitleDraft` resync the pre-move code had — covered by an adapted test asserting both re-sync from the response. Also fixes two adjacent tests whose `updateFlow` mocks didn't echo the unchanged name back (unrealistic given Accept now saves immediately), which the F2 fix's correct resync exposed as flaky.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughWorkflow Copilot proposal acceptance now saves asynchronously through the canvas persistence path, synchronizes state from server responses, shows saving feedback, and preserves proposals for retry after failures. Translations and tests cover the updated behavior. ChangesCopilot accept and save
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowCopilotPanel
participant FlowCanvasPage
participant FlowPersistence
WorkflowCopilotPanel->>FlowCanvasPage: accept proposal
FlowCanvasPage->>FlowPersistence: createFlow or updateFlow
FlowPersistence-->>FlowCanvasPage: persisted flow response
FlowCanvasPage-->>WorkflowCopilotPanel: resolve or rethrow result
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/pages/FlowCanvasPage.tsx (1)
554-580: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist the accepted proposal’s approval policy.
WorkflowProposal.requireApprovaldefines the policy to use once saved, but acceptance forwards only its graph and name. This silently preserves the old canvas policy instead.
app/src/pages/FlowCanvasPage.tsx#L554-L580: passproposal.requireApprovalthroughhandleSave, including it in draft creation and persisted-flow updates.app/src/pages/__tests__/FlowCanvasPage.test.tsx#L642-L661: use differing route/proposal values and assert that the accepted proposal’s value reachescreateFlow; add equivalentupdateFlowcoverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/FlowCanvasPage.tsx` around lines 554 - 580, The accepted proposal’s requireApproval policy is not being persisted. In app/src/pages/FlowCanvasPage.tsx lines 554-580, pass proposal.requireApproval through handleSave and ensure both draft creation and persisted-flow updates use it; in app/src/pages/__tests__/FlowCanvasPage.test.tsx lines 642-661, use differing route and proposal values, assert the proposal value reaches createFlow, and add equivalent updateFlow coverage.
🧹 Nitpick comments (1)
app/src/components/flows/WorkflowCopilotPanel.tsx (1)
397-409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing entry/success diagnostics for this changed persistence flow.
accept()only logs on the failure branch (log('accept: save failed...')). Per repo convention elsewhere in this file (e.g.updatePendingAsk, the build/repair effects), and per the coding guideline for changed flows with external calls and state transitions, this new save path should also log on entry/success so the accept→save round trip is grep-friendly in production logs.As per coding guidelines, "New or changed flows must include verbose, grep-friendly diagnostics for entry/exit, branches, external calls, retries/timeouts, state transitions, and errors."
♻️ Proposed fix
const accept = useCallback(async () => { if (!proposal) return; setAcceptSaving(true); + log('accept: saving proposal via host onAccept'); try { await onAccept(proposal); + log('accept: save succeeded, clearing proposal'); clearProposal(); lastSurfacedRef.current = null; } catch (err) { log('accept: save failed, leaving proposal visible for retry err=%o', err); } finally { setAcceptSaving(false); } }, [proposal, onAccept, clearProposal]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/flows/WorkflowCopilotPanel.tsx` around lines 397 - 409, The accept persistence flow in accept currently logs only failures; add grep-friendly diagnostics for entry and successful completion around onAccept(proposal), including the relevant proposal/state transition context while preserving the existing retry behavior and error log.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/components/flows/WorkflowCopilotPanel.tsx`:
- Around line 397-409: Prevent concurrent Accept and Reject actions in
WorkflowCopilotPanel: add an acceptSaving guard at the start of accept() so
repeated calls cannot run during an in-flight save, and set the Reject Button’s
disabled prop to acceptSaving. Preserve the existing accept cleanup and retry
behavior, and add a regression test covering Reject while onAccept remains
pending if the surrounding tests support it.
In `@app/src/pages/FlowCanvasPage.tsx`:
- Around line 575-580: Serialize the accept-triggered handleSave call in
FlowCanvasPage with the same in-flight save state or mutex used by the header
Save and title rename operations. Ensure saveMeta.saving (or the shared
equivalent) is set while await handleSave(proposedGraph, overrideName) runs,
preventing concurrent writes and stale responses from overwriting newer edits.
---
Outside diff comments:
In `@app/src/pages/FlowCanvasPage.tsx`:
- Around line 554-580: The accepted proposal’s requireApproval policy is not
being persisted. In app/src/pages/FlowCanvasPage.tsx lines 554-580, pass
proposal.requireApproval through handleSave and ensure both draft creation and
persisted-flow updates use it; in
app/src/pages/__tests__/FlowCanvasPage.test.tsx lines 642-661, use differing
route and proposal values, assert the proposal value reaches createFlow, and add
equivalent updateFlow coverage.
---
Nitpick comments:
In `@app/src/components/flows/WorkflowCopilotPanel.tsx`:
- Around line 397-409: The accept persistence flow in accept currently logs only
failures; add grep-friendly diagnostics for entry and successful completion
around onAccept(proposal), including the relevant proposal/state transition
context while preserving the existing retry behavior and error log.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f872e7e2-7bc0-4b43-8f3b-cbe28c821d89
📒 Files selected for processing (18)
app/src/components/flows/WorkflowCopilotPanel.test.tsxapp/src/components/flows/WorkflowCopilotPanel.tsxapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/pages/FlowCanvasPage.tsxapp/src/pages/__tests__/FlowCanvasPage.test.tsx
| // Persist immediately — this is the actual fix. Do NOT route through | ||
| // `canvasRef.current?.save()`: the canvas is mid-remount (the | ||
| // `canvasVersion` bump above) so the ref's imperative handle is stale; | ||
| // call `handleSave` directly with the known-good proposed graph. | ||
| try { | ||
| await handleSave(proposedGraph, overrideName); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize accept-triggered persistence with other canvas writes.
Calling handleSave directly does not set saveMeta.saving, so the header Save button and title rename remain available during this request. A second write can race and an older response can re-seed the canvas, overwriting newer edits. Use one shared in-flight save state or mutex for Accept, Save, and rename operations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/pages/FlowCanvasPage.tsx` around lines 575 - 580, Serialize the
accept-triggered handleSave call in FlowCanvasPage with the same in-flight save
state or mutex used by the header Save and title rename operations. Ensure
saveMeta.saving (or the shared equivalent) is set while await
handleSave(proposedGraph, overrideName) runs, preventing concurrent writes and
stale responses from overwriting newer edits.
There was a problem hiding this comment.
Confirmed and fixed in f61b272, but scoped to what was actually broken: the accept-triggered handleSave call was missing the accepted proposal's requireApproval policy (a real data-integrity bug — see my other reply). I did not add a separate saveMeta.saving/mutex guard here, since accept()'s own acceptSaving guard (fixed per the other finding) already prevents a second Accept from racing this same call, and the header Save button's own saveMeta.dirty/saveMeta.saving state only reflects the canvas's internal save calls, not this direct handleSave invocation — wiring those together is a larger refactor of the canvas's save-state ownership than this PR's scope (canvas Accept-persists behavior). Flagging as a follow-up rather than blocking this fix.
There was a problem hiding this comment.
@graycyrus, that scoping makes sense — the requireApproval propagation was the concrete data-integrity bug, and gating on acceptSaving does close the immediate double-accept race for this call site. Unifying it with the canvas's own saveMeta.saving/rename state is a fair amount of refactor to take on here, so treating it as a follow-up rather than blocking this PR is reasonable.
Want me to open a follow-up issue to track adding a shared in-flight save state (or mutex) across Accept, header Save, and title rename in app/src/pages/FlowCanvasPage.tsx, so a stale response from one can't overwrite a newer edit from another? I can link this PR/comment and assign it to you if so.
(ᵔᴥᵔ) sounds like a solid call
…coderabbitai on WorkflowCopilotPanel.tsx:409) Reject stayed clickable while an Accept save was in flight, so a user's cancel could be silently overridden by the earlier Accept's save landing after. Self-guard accept() against re-entrance and disable Reject while acceptSaving is true. Also adds entry/success debug logging to the accept -> save round trip (addresses @coderabbitai nitpick on WorkflowCopilotPanel.tsx:397-409) and a regression test.
…addresses @coderabbitai on FlowCanvasPage.tsx:554-580) handleSave forwarded only the proposal's graph/name; the accepted WorkflowProposal.requireApproval policy was dropped, silently preserving the pre-existing canvas policy instead. Thread it through handleSave as an explicit override (mirroring the existing overrideName stale-closure pattern) for both draft creation (createFlow) and persisted-flow updates (updateFlow), and add regression tests covering both paths with differing route/proposal values.
|
Addressed the remaining two CodeRabbit findings from the review body (not separately threaded since they're outside-diff / a nitpick): Major (outside diff) — Nitpick — All 4 findings (2 inline + these 2) fixed and pushed. 71/71 unit tests pass locally (69 baseline + 2 new), typecheck/lint/format/i18n:check all clean. |
1 participant
Summary
On the canvas copilot, clicking Accept only applied the proposal to the local draft and fired no persistence RPC — the user then had to separately click the header Save (which they didn't realize), so builds silently never saved (observed live in Batch A5: flow persisted as an empty trigger-only "New workflow"). This makes canvas Accept = review + save: it applies the proposal, adopts the proposed name into the title (placeholder-only), and immediately persists via
handleSave.Changes
FlowCanvasPage.handleAcceptProposal→ async: apply graph, adopt name,await handleSave(proposedGraph, overrideName).handleSave(next, overrideName?)— optional name override to avoid the stale-name closure after adoption.WorkflowCopilotPanelAccept → async with a saving state (Accept & save/Saving…, disabled during save).Review findings addressed (from a pre-merge architect pass)
handleSavere-syncs bothnameandtitleDraftfrom the server response (thesetTitleDraftwas dropped when the fn moved) — prevents a stale visible title re-persisting on a later blur.isPlaceholderTitle(titleDraft, …) && !renamingguard is present (from fix(flows): copilot proposals reach the canvas — recognition, race-guard, title #4886).Testing
FlowCanvasPage.test.tsx+WorkflowCopilotPanel.test.tsx, incl. new F1 (rethrow) + F2 (titleDraft resync) regression tests.typecheck+lintclean;i18n:check0 missing across all 13 locales (flows.copilot.acceptAndSave,flows.copilot.saving).Deferred (noted for follow-up)
useFlowChangedcanvas sync on an agent-sidesave_workflow— deferred (needs dirty-state gating to avoid clobbering in-progress edits); pre-existing, marginal delta risk.Acceptance criteria
Summary by CodeRabbit