Repository navigation
Conversation
The workflow_builder subagent builds a valid proposal (revise_workflow), but the
Accept/Reject card never rendered — the proposal was silently dropped.
Root cause: in ChatRuntimeProvider.onSubagentToolResult, the proposal extraction
(maybeParseWorkflowProposalTool → setWorkflowProposalForThread) sat AFTER four
early-return guards for timeline/UI bookkeeping. Under the builder's heavy event
volume (50+ progress events; the progress channel holds 64 and the parent
try_sends, dropping on overflow), the timeline-entry lookup fails → a guard
returns early → the proposal is never dispatched → panel's proposal stays null.
Fix (primary, frontend): hoist proposal extraction to the top of the handler,
before the guards — it only needs event.{tool_name,success,output}, no timeline
state. Confirmed event.thread_id is already the PARENT copilot thread (traced in
progress_bridge.rs), so it lands where useWorkflowBuilderChat watches. Added a
regression test: proposal dispatches even with no matching timeline row.
Hardening:
- progress channel capacity 64 → 256 (run_task.rs) — reduce the drop pressure.
- emit SubagentCompleted in dispatch.rs Completed AND Incomplete arms — delegated
subagents no longer show 'running forever' in the timeline (mirrors spawn_subagent).
- orchestrator prompt: clarify describe_workflow is for installed skills, not Flows
automations (which use build_workflow) — stops the wasted erroring tool call.
typecheck/lint/prettier clean; 122 frontend tests pass; cargo check + fmt clean.
Redesigns the Workflow Copilot from propose-and-gate to direct-manipulation, per product direction: 'I say it → it builds it on the canvas, live; I say change X → it changes it, live; no approval, no card, no JSON.' Frontend (auto-apply): - FlowCanvasPage: handleProposal/Accept/Reject + preview state → a single handleAutoApply that applies the graph to the draft immediately (diff for the added-node highlight, snapshot onto a bounded previousDrafts undo stack, then setDraftGraph + canvasVersion bump). Fires an 'Applied — Undo?' toast (existing toast system) whose action pops the stack and restores the prior draft. - WorkflowCopilotPanel: removed the Accept/Reject diff card + onAccept/onReject; now just surfaces onProposal (+ clearProposal to avoid re-firing a stale one). - EditableFlowCanvas: added-node sage-green ring now flashes ~3s then clears. - i18n: flows.copilot.appliedTitle/undo (+ reworded subtitle/empty) x14 locales. Backend (kill JSON + 'save via UI' talk — framing only, no behavior change): - workflow_builder/prompt.md: reframed 'propose, never persist' → 'build directly onto the canvas'; never output JSON / say 'proposal'/'Save & enable'; end with a plain-language summary. (KEEPS 'cannot persist/enable' — agent.toml tool scope still structurally enforces it.) - flows/tools.rs + builder_tools.rs: propose/revise_workflow descriptions reworded to 'applies to the canvas draft'. Save + run-time approval paths untouched. WorkflowProposalCard (regular chat) left as-is (separate context). typecheck/lint/prettier clean; 224 frontend tests pass; cargo check/fmt clean.
📝 WalkthroughWalkthroughThis PR redesigns the Flow Copilot from an Accept/Reject proposal-preview model to a direct-apply model: proposals are surfaced once and applied immediately to the canvas draft, with a bounded undo stack for reverting. Related prompt/tool text, i18n strings, transient node highlighting, proposal extraction timing, and backend progress-event/buffer changes support this redesign. ChangesCopilot Direct-Apply Redesign
Backend Progress Event and Channel Sizing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowCopilotPanel
participant FlowCanvasPage
participant EditableFlowCanvas
WorkflowCopilotPanel->>FlowCanvasPage: onProposal(proposal)
FlowCanvasPage->>FlowCanvasPage: snapshot draftGraph, diffGraphs
FlowCanvasPage->>EditableFlowCanvas: apply draftGraph, addedNodeIds
FlowCanvasPage->>FlowCanvasPage: show Undo toast
FlowCanvasPage->>FlowCanvasPage: undoLastAutoApply() restores prior draft
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
Comment |
coderabbitai
Bot
added
agent
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 456c67d5cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| onProposal={handleProposal} | ||
| onAccept={handleAcceptProposal} | ||
| onReject={handleRejectProposal} | ||
| onProposal={handleAutoApply} |
There was a problem hiding this comment.
If the user sends a copilot turn, closes the panel before workflow_builder finishes, and then edits the canvas, the proposal remains pending because WorkflowCopilotPanel is unmounted. Reopening the panel now wires that pending proposal into handleAutoApply, so the stale graph is immediately applied over the current draft and can wipe out the intervening manual edits; direct-apply needs to happen when the proposal arrives or discard/guard proposals that completed while the panel was closed.
Useful? React with 👍 / 👎.
| addToast({ | ||
| type: 'info', | ||
| title: t('flows.copilot.appliedTitle'), | ||
| action: { label: t('flows.copilot.undo'), handler: undoLastAutoApply }, |
There was a problem hiding this comment.
When two copilot proposals are applied within the toast duration, or the user double-clicks Undo, every visible toast action calls the same undoLastAutoApply stack-pop handler and the toast is not removed after the action. That lets an old or repeated Undo click revert additional AI changes beyond the one the toast represents, so the handler should dismiss/bind the toast to a specific apply entry or otherwise be single-use.
Useful? React with 👍 / 👎.
![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)
src/openhuman/flows/builder_tools.rs (1)
107-110: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLeftover "review card" wording contradicts the new direct-apply contract.
The
instructionparameter description still says "Echoed back for the review card" — but the new tool description above (Line 84-87) and prompt.md now explicitly tell the agent there is no review card and it must never say that phrase. This schema text is part of what the agent sees as the tool's contract, so it re-introduces the exact terminology this PR set out to remove.✏️ Proposed fix
"instruction": { "type": "string", - "description": "The revision instruction that motivated this change (e.g. 'add a Slack step after the summary'). Echoed back for the review card; does not affect validation." + "description": "The revision instruction that motivated this change (e.g. 'add a Slack step after the summary'). Echoed back to the user; does not affect validation." },🤖 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 `@src/openhuman/flows/builder_tools.rs` around lines 107 - 110, The `instruction` field description in `builder_tools.rs` still mentions “review card,” which conflicts with the direct-apply contract. Update the schema text for the `instruction` parameter so it only describes the revision instruction itself and removes any “Echoed back for the review card” wording, keeping it consistent with the surrounding tool description and prompt contract.
🧹 Nitpick comments (3)
src/openhuman/agent_registry/agents/orchestrator/prompt.rs (1)
351-374: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the new
describe_workflow/build_workflowrouting guidance.The updated guidance (Lines 105-110) is a meaningful behavioral instruction, but the existing test only checks for
run_skill/Handoff Plan/skill entries.🧪 Suggested additional assertions
assert!(out.contains("- **ascii-art**: ASCII art via pyfiglet")); assert!(out.contains("- **no-dir**: (no description)")); + assert!(out.contains("build_workflow")); + assert!(out.contains("Flows `workflow_id`")); }🤖 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 `@src/openhuman/agent_registry/agents/orchestrator/prompt.rs` around lines 351 - 374, Add test coverage for the new routing guidance in render_installed_skills by extending the existing installed-skills test to assert that the generated output includes the describe_workflow and build_workflow instructions, not just run_skill and Handoff Plan. Use the render_installed_skills test and its current assertions as the place to verify that the catalog now steers users to the workflow description/build path when appropriate.app/src/lib/i18n/en.ts (1)
4442-4445: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead i18n keys for the removed Accept/Reject preview UI.
flows.copilot.proposalTitle,added,removed,noChanges,accept,reject, andpreviewHintwere used by the diff-preview card that this PR removes fromWorkflowCopilotPanel. If nothing else in the codebase still renders these strings, consider pruning them (across all locales) in a follow-up to avoid unused-translation drift.🤖 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/lib/i18n/en.ts` around lines 4442 - 4445, The removed diff-preview UI in WorkflowCopilotPanel left several unused translation entries behind, so prune the dead flows.copilot.* keys if nothing else references them. Remove proposalTitle, added, removed, noChanges, accept, reject, and previewHint from the locale tables, and update all locale files consistently to keep the i18n catalog in sync.src/openhuman/agent_orchestration/tools/dispatch.rs (1)
216-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
SubagentCompletedemission block — extract a helper.The exact same 15-line progress-send block (agent_id, task_id, elapsed_ms, iterations, output_chars, output, worktree_path=None, changed_files=empty, dirty_status=None) is repeated verbatim in the
CompletedandIncompletearms. Extracting a small async helper (e.g.emit_subagent_completed_progress(outcome: &SubagentRunOutcome)) would remove the duplication and keep future field additions toAgentProgress::SubagentCompleted(seesrc/openhuman/agent/progress.rs:111-138) in one place.♻️ Suggested refactor
+async fn emit_subagent_completed_progress(outcome: &SubagentRunOutcome) { + if let Some(progress) = current_parent().and_then(|p| p.on_progress.clone()) { + let _ = progress + .send(AgentProgress::SubagentCompleted { + agent_id: outcome.agent_id.clone(), + task_id: outcome.task_id.clone(), + elapsed_ms: outcome.elapsed.as_millis() as u64, + iterations: outcome.iterations as u32, + output_chars: outcome.output.chars().count(), + output: outcome.output.clone(), + worktree_path: None, + changed_files: Vec::new(), + dirty_status: None, + }) + .await; + } +}Then call
emit_subagent_completed_progress(&outcome).await;in both arms.Also applies to: 263-280
🤖 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 `@src/openhuman/agent_orchestration/tools/dispatch.rs` around lines 216 - 239, The same AgentProgress::SubagentCompleted send block is duplicated in the Completed and Incomplete branches of dispatch.rs; extract it into a small async helper such as emit_subagent_completed_progress(outcome: &SubagentRunOutcome) and call that helper from both arms. Keep the helper near the existing dispatch logic so the shared field mapping (agent_id, task_id, elapsed_ms, iterations, output_chars, output, worktree_path, changed_files, dirty_status) is maintained in one place and future changes to SubagentCompleted stay consistent.
🤖 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/canvas/EditableFlowCanvas.tsx`:
- Around line 196-209: The useEffect in EditableFlowCanvas is violating the
set-state-in-effect rule by calling setVisibleAddedNodeIds unconditionally
before the early return, and that update is redundant because
visibleAddedNodeIds already starts from addedNodeIds and the component remounts
per proposal. Remove the leading state reset from the effect, keep the
timeout-based fade-out logic for addedNodeIds, and leave the cleanup in place so
only the timer drives the temporary highlight.
In `@app/src/pages/FlowCanvasPage.tsx`:
- Around line 195-262: The stacked auto-apply toast behavior is mismatched with
the shared LIFO undo logic, so older “Undo” actions can revert the wrong
proposal. Update FlowCanvasPage’s toast handling around addToast,
handleAutoApply, and undoLastAutoApply so only one auto-apply toast can be
active at a time, or bind each toast to the specific draft snapshot it
represents instead of always calling the same global undo handler. If you keep
multiple toasts, make the handler restore the exact pre-apply graph captured
when that toast was created rather than popping whatever is currently on top of
previousDrafts.
---
Outside diff comments:
In `@src/openhuman/flows/builder_tools.rs`:
- Around line 107-110: The `instruction` field description in `builder_tools.rs`
still mentions “review card,” which conflicts with the direct-apply contract.
Update the schema text for the `instruction` parameter so it only describes the
revision instruction itself and removes any “Echoed back for the review card”
wording, keeping it consistent with the surrounding tool description and prompt
contract.
---
Nitpick comments:
In `@app/src/lib/i18n/en.ts`:
- Around line 4442-4445: The removed diff-preview UI in WorkflowCopilotPanel
left several unused translation entries behind, so prune the dead
flows.copilot.* keys if nothing else references them. Remove proposalTitle,
added, removed, noChanges, accept, reject, and previewHint from the locale
tables, and update all locale files consistently to keep the i18n catalog in
sync.
In `@src/openhuman/agent_orchestration/tools/dispatch.rs`:
- Around line 216-239: The same AgentProgress::SubagentCompleted send block is
duplicated in the Completed and Incomplete branches of dispatch.rs; extract it
into a small async helper such as emit_subagent_completed_progress(outcome:
&SubagentRunOutcome) and call that helper from both arms. Keep the helper near
the existing dispatch logic so the shared field mapping (agent_id, task_id,
elapsed_ms, iterations, output_chars, output, worktree_path, changed_files,
dirty_status) is maintained in one place and future changes to SubagentCompleted
stay consistent.
In `@src/openhuman/agent_registry/agents/orchestrator/prompt.rs`:
- Around line 351-374: Add test coverage for the new routing guidance in
render_installed_skills by extending the existing installed-skills test to
assert that the generated output includes the describe_workflow and
build_workflow instructions, not just run_skill and Handoff Plan. Use the
render_installed_skills test and its current assertions as the place to verify
that the catalog now steers users to the workflow description/build path when
appropriate.
🪄 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: 95c7f51c-3885-4d4e-811d-2b2ad749696c
📒 Files selected for processing (28)
app/src/components/flows/WorkflowCopilotPanel.test.tsxapp/src/components/flows/WorkflowCopilotPanel.tsxapp/src/components/flows/canvas/EditableFlowCanvas.tsxapp/src/lib/flows/workflowBuilderPrompt.tsapp/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.tsxapp/src/providers/ChatRuntimeProvider.tsxapp/src/providers/__tests__/ChatRuntimeProvider.test.tsxsrc/openhuman/agent_orchestration/tools/dispatch.rssrc/openhuman/agent_registry/agents/orchestrator/prompt.rssrc/openhuman/agent_registry/agents/workflow_builder/prompt.mdsrc/openhuman/channels/providers/web/run_task.rssrc/openhuman/flows/builder_tools.rssrc/openhuman/flows/tools.rs
| // Copilot auto-apply highlight (Phase 6 redesign): `addedNodeIds` arrives | ||
| // once per mount (the host bumps `canvasVersion`/remounts this component on | ||
| // every auto-applied proposal) rather than standing for the duration of a | ||
| // preview-under-review. Mirror it into local state and fade it out after a | ||
| // few seconds so newly added nodes flash sage-green then settle, instead of | ||
| // staying ringed forever with no Accept/Reject to dismiss them. | ||
| const [visibleAddedNodeIds, setVisibleAddedNodeIds] = useState<ReadonlySet<string>>(addedNodeIds); | ||
| useEffect(() => { | ||
| setVisibleAddedNodeIds(addedNodeIds); | ||
| if (addedNodeIds.size === 0) return; | ||
| const timer = setTimeout(() => setVisibleAddedNodeIds(EMPTY_ID_SET), 3000); | ||
| return () => clearTimeout(timer); | ||
| }, [addedNodeIds]); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Unconditional setState at the top of useEffect before a conditional return — and it's redundant here.
setVisibleAddedNodeIds(addedNodeIds) fires unconditionally before the early return when addedNodeIds.size === 0, matching the exact anti-pattern this codebase's react-hooks/set-state-in-effect rule disallows ("resetting state flags at the top of an effect before returning").
It's also unnecessary: visibleAddedNodeIds is already initialized to addedNodeIds (Line 202), and since the host remounts this component (new key) on every auto-apply, addedNodeIds is stable for the component's entire lifetime — this effect only ever runs once per mount, so the leading setState call is a no-op that exists solely to satisfy the lint pattern the codebase avoids elsewhere.
♻️ Proposed fix — drop the redundant setState, keep only the fade-timer
- const [visibleAddedNodeIds, setVisibleAddedNodeIds] = useState<ReadonlySet<string>>(addedNodeIds);
- useEffect(() => {
- setVisibleAddedNodeIds(addedNodeIds);
- if (addedNodeIds.size === 0) return;
- const timer = setTimeout(() => setVisibleAddedNodeIds(EMPTY_ID_SET), 3000);
- return () => clearTimeout(timer);
- }, [addedNodeIds]);
+ const [visibleAddedNodeIds, setVisibleAddedNodeIds] = useState<ReadonlySet<string>>(addedNodeIds);
+ useEffect(() => {
+ if (addedNodeIds.size === 0) return;
+ const timer = setTimeout(() => setVisibleAddedNodeIds(EMPTY_ID_SET), 3000);
+ return () => clearTimeout(timer);
+ // `addedNodeIds` is fixed for this component's lifetime (the host remounts
+ // on every auto-apply), so this effect intentionally runs once on mount.
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, []);Based on learnings, "do not perform synchronous setState ... directly inside useEffect bodies... disallows patterns such as resetting state flags at the top of an effect before returning."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Copilot auto-apply highlight (Phase 6 redesign): `addedNodeIds` arrives | |
| // once per mount (the host bumps `canvasVersion`/remounts this component on | |
| // every auto-applied proposal) rather than standing for the duration of a | |
| // preview-under-review. Mirror it into local state and fade it out after a | |
| // few seconds so newly added nodes flash sage-green then settle, instead of | |
| // staying ringed forever with no Accept/Reject to dismiss them. | |
| const [visibleAddedNodeIds, setVisibleAddedNodeIds] = useState<ReadonlySet<string>>(addedNodeIds); | |
| useEffect(() => { | |
| setVisibleAddedNodeIds(addedNodeIds); | |
| if (addedNodeIds.size === 0) return; | |
| const timer = setTimeout(() => setVisibleAddedNodeIds(EMPTY_ID_SET), 3000); | |
| return () => clearTimeout(timer); | |
| }, [addedNodeIds]); | |
| // Copilot auto-apply highlight (Phase 6 redesign): `addedNodeIds` arrives | |
| // once per mount (the host bumps `canvasVersion`/remounts this component on | |
| // every auto-applied proposal) rather than standing for the duration of a | |
| // preview-under-review. Mirror it into local state and fade it out after a | |
| // few seconds so newly added nodes flash sage-green then settle, instead of | |
| // staying ringed forever with no Accept/Reject to dismiss them. | |
| const [visibleAddedNodeIds, setVisibleAddedNodeIds] = useState<ReadonlySet<string>>(addedNodeIds); | |
| useEffect(() => { | |
| if (addedNodeIds.size === 0) return; | |
| const timer = setTimeout(() => setVisibleAddedNodeIds(EMPTY_ID_SET), 3000); | |
| return () => clearTimeout(timer); | |
| // `addedNodeIds` is fixed for this component's lifetime (the host remounts | |
| // on every auto-apply), so this effect intentionally runs once on mount. | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, []); |
🤖 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/canvas/EditableFlowCanvas.tsx` around lines 196 -
209, The useEffect in EditableFlowCanvas is violating the set-state-in-effect
rule by calling setVisibleAddedNodeIds unconditionally before the early return,
and that update is redundant because visibleAddedNodeIds already starts from
addedNodeIds and the component remounts per proposal. Remove the leading state
reset from the effect, keep the timeout-based fade-out logic for addedNodeIds,
and leave the cleanup in place so only the timer drives the temporary highlight.
Source: Learnings
| // AI-undo stack: pre-apply `draftGraph` snapshots, most-recent last. Bounded | ||
| // so a long copilot session doesn't grow this unboundedly. Only ever read via | ||
| // its own setter's updater function (below), never rendered directly. | ||
| const [, setPreviousDrafts] = useState<WorkflowGraph[]>([]); | ||
| const AI_UNDO_STACK_LIMIT = 10; | ||
| // Node ids the most recent auto-applied proposal added, so the canvas can | ||
| // flash a "just added" highlight on remount (Phase 5c's diff ring, now a | ||
| // transient flash instead of a standing preview state). | ||
| const [lastAddedNodeIds, setLastAddedNodeIds] = useState<Set<string> | undefined>(undefined); | ||
| const [canvasVersion, setCanvasVersion] = useState(0); | ||
| const [toasts, setToasts] = useState<ToastNotification[]>([]); | ||
| const addToast = useCallback((toast: Omit<ToastNotification, 'id'>) => { | ||
| setToasts(prev => [...prev, { ...toast, id: `toast-${Date.now()}-${Math.random()}` }]); | ||
| }, []); | ||
| const removeToast = useCallback((id: string) => { | ||
| setToasts(prev => prev.filter(item => item.id !== id)); | ||
| }, []); | ||
|
|
||
| // Last-persisted graph, independent of canvas remounts (fixes a P1: the | ||
| // editable canvas seeds its own dirty baseline from whatever graph it's | ||
| // mounted with, so bumping `canvasVersion` on Accept — remounting the | ||
| // canvas with the just-accepted proposal as its "initial" graph — made an | ||
| // unsaved accepted proposal instantly read as clean; the accepted change | ||
| // was then lost on back/reload instead of gating behind the required Save. | ||
| // Only ever updated by a real Save (`handleSave` below), so a diff against | ||
| // it survives any number of accept/reject/preview remounts. | ||
| // mounted with, so bumping `canvasVersion` on an auto-apply — remounting | ||
| // the canvas with the just-applied graph as its "initial" graph — made an | ||
| // unsaved AI change instantly read as clean; the change was then lost on | ||
| // back/reload instead of gating behind the required Save. Only ever | ||
| // updated by a real Save (`handleSave` below), so a diff against it | ||
| // survives any number of auto-apply/undo/manual-edit remounts. | ||
| const persistedGraphRef = useRef<WorkflowGraph>(graph); | ||
|
|
||
| const handleGraphChange = useCallback( | ||
| (next: WorkflowGraph) => { | ||
| // Freeze the draft while a proposal is under review — the preview graph | ||
| // (with ghosts) must not overwrite the real draft. | ||
| if (preview) return; | ||
| setDraftGraph(next); | ||
| }, | ||
| [preview] | ||
| ); | ||
| const handleGraphChange = useCallback((next: WorkflowGraph) => { | ||
| setDraftGraph(next); | ||
| }, []); | ||
|
|
||
| const handleProposal = useCallback( | ||
| // Undo the most recent auto-applied AI change: pop the pre-apply snapshot | ||
| // off `previousDrafts` and restore it as the draft. Fired from the "Undo" | ||
| // action on the auto-apply toast. | ||
| const undoLastAutoApply = useCallback(() => { | ||
| setPreviousDrafts(prev => { | ||
| if (prev.length === 0) return prev; | ||
| const restored = prev[prev.length - 1]; | ||
| log('copilot auto-apply: undo — restoring previous draft'); | ||
| setDraftGraph(restored); | ||
| setLastAddedNodeIds(undefined); | ||
| setCanvasVersion(v => v + 1); | ||
| return prev.slice(0, -1); | ||
| }); | ||
| }, []); | ||
|
|
||
| // A copilot proposal applies DIRECTLY to the canvas — no preview, no | ||
| // Accept/Reject. The pre-apply draft is pushed onto the AI-undo stack first | ||
| // so an "Undo" toast can revert it; the canvas remounts (bump | ||
| // `canvasVersion`) with the new graph, highlighting the added nodes. | ||
| const handleAutoApply = useCallback( | ||
| (proposal: WorkflowProposal) => { | ||
| const proposedGraph = proposal.graph as WorkflowGraph; | ||
| const d = diffGraphs(draftGraph, proposedGraph); | ||
| log('copilot proposal: added=%d removed=%d', d.addedNodeIds.size, d.removedNodeIds.size); | ||
| setPreview({ | ||
| proposal, | ||
| base: draftGraph, | ||
| addedNodeIds: d.addedNodeIds, | ||
| removedNodeIds: d.removedNodeIds, | ||
| }); | ||
| log('copilot auto-apply: added=%d removed=%d', d.addedNodeIds.size, d.removedNodeIds.size); | ||
| setPreviousDrafts(prev => [...prev, draftGraph].slice(-AI_UNDO_STACK_LIMIT)); | ||
| setDraftGraph(proposedGraph); | ||
| setLastAddedNodeIds(d.addedNodeIds); | ||
| setCanvasVersion(v => v + 1); | ||
| addToast({ | ||
| type: 'info', | ||
| title: t('flows.copilot.appliedTitle'), | ||
| action: { label: t('flows.copilot.undo'), handler: undoLastAutoApply }, | ||
| }); | ||
| }, | ||
| [draftGraph] | ||
| [draftGraph, addToast, t, undoLastAutoApply] | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stacked "Applied — Undo?" toasts share one LIFO stack — clicking an older toast's Undo reverts the newest change instead.
addToast never dismisses/replaces a prior auto-apply toast (Line 206-208), so if the user triggers two proposals before dismissing the first toast, two toasts stack up, but both action.handler reference the same undoLastAutoApply (Line 230-240), which always pops the top of previousDrafts regardless of which toast was clicked. Clicking the older toast's "Undo" will silently undo the newer apply, not the one that toast represents.
Consider either (a) replacing any existing auto-apply toast before pushing a new one so only one Undo affordance is ever visible, or (b) capturing the specific snapshot index at toast-creation time and binding each toast's handler to restore that exact snapshot.
♻️ Proposed fix: keep only one active auto-apply toast
const handleAutoApply = useCallback(
(proposal: WorkflowProposal) => {
const proposedGraph = proposal.graph as WorkflowGraph;
const d = diffGraphs(draftGraph, proposedGraph);
log('copilot auto-apply: added=%d removed=%d', d.addedNodeIds.size, d.removedNodeIds.size);
setPreviousDrafts(prev => [...prev, draftGraph].slice(-AI_UNDO_STACK_LIMIT));
setDraftGraph(proposedGraph);
setLastAddedNodeIds(d.addedNodeIds);
setCanvasVersion(v => v + 1);
+ // Only one auto-apply "Undo" affordance should be actionable at a time —
+ // undoLastAutoApply always targets the stack's top, so a stale older
+ // toast's Undo button would otherwise revert the wrong change.
+ setToasts(prev => prev.filter(item => item.id !== 'ai-auto-apply'));
addToast({
+ id: 'ai-auto-apply',
type: 'info',
title: t('flows.copilot.appliedTitle'),
action: { label: t('flows.copilot.undo'), handler: undoLastAutoApply },
});
},
[draftGraph, addToast, t, undoLastAutoApply]
);(Note: id would need to be threaded through addToast/ToastNotification rather than always auto-generated, or filter by title as a simpler proxy.)
🤖 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 195 - 262, The stacked
auto-apply toast behavior is mismatched with the shared LIFO undo logic, so
older “Undo” actions can revert the wrong proposal. Update FlowCanvasPage’s
toast handling around addToast, handleAutoApply, and undoLastAutoApply so only
one auto-apply toast can be active at a time, or bind each toast to the specific
draft snapshot it represents instead of always calling the same global undo
handler. If you keep multiple toasts, make the handler restore the exact
pre-apply graph captured when that toast was created rather than popping
whatever is currently on top of previousDrafts.
|
Closing as superseded. #4578 ("prompt bar → instant canvas with backend-driven copilot builder") shipped the same direct-apply Copilot experience via a cleaner backend-agent architecture, so this branch is redundant. The genuinely-useful remaining work (builder producing resolvable graphs — the null-binding bug) is carried forward in #4586. |
What
Redesigns the Workflow Copilot from propose-and-gate to direct-manipulation, per product direction: "I say it → it builds it on the canvas, live; I say change X → it changes it, live; no approval, no card, no JSON."
Frontend — auto-apply
FlowCanvasPage:handleProposal/Accept/Reject+previewstate → onehandleAutoApplythat applies the graph to the draft immediately (diff → added-node highlight; snapshot onto a boundedpreviousDraftsundo stack;setDraftGraph+canvasVersionbump). Fires an "Applied — Undo?" toast whose action pops the stack and restores the prior draft.WorkflowCopilotPanel: removed the Accept/Reject diff card +onAccept/onReject; now just surfacesonProposal(+clearProposalso a stale proposal can't re-fire).EditableFlowCanvas: the added-node sage-green ring now flashes ~3s then clears.Backend — kill the JSON + "save via UI" talk (framing only)
workflow_builder/prompt.md: reframed propose, never persist → build directly onto the canvas; never output JSON, never say proposal/Save & enable; end with a plain-language summary. Keeps "cannot persist/enable" — theagent.tomltool scope still structurally enforces it.tools.rs+builder_tools.rs:propose_workflow/revise_workflowdescriptions reworded to applies to the canvas draft. No behavior/scope change.Untouched (verified)
Save (persist) and run-time approval (sending) — separate code paths, unchanged.
WorkflowProposalCard(regular chat) left as-is.Known follow-up
The shared
DELEGATE_DIRECTIVEnow sends "build directly onto my canvas" wording to the regular-chat builder path too, which still renders a review card — a minor wording/UX mismatch to reconcile in a follow-up (that path is out of scope here).Verification
typecheck/lint/prettierclean · 224 frontend tests pass (Accept/Reject tests rewritten for the no-card flow + new auto-apply/undo tests) ·cargo check/fmtclean.Summary by CodeRabbit
New Features
Bug Fixes