Add undo/redo and unsaved-changes protection to the flow builder - #4278
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds snapshot-based undo/redo history, keyboard shortcuts, dirty-state tracking, preview-aware saving, unsaved-change navigation protection, toolbar controls, a discard dialog, translations, and associated tests. ChangesFlow Builder History and Save Protection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds editing safety features to the Flow Builder UI (login-flow builder + shared visual-flow components): snapshot-based undo/redo, unsaved-changes (dirty) indicators + navigation protection, and enables saving while in preview by exiting preview first.
Changes:
- Introduces a debounced snapshot history hook (
useFlowHistory) and wires undo/redo into the canvas toolbar and keyboard shortcuts. - Adds dirty-state tracking with a Save-button dot indicator, a discard-confirmation dialog on “Back to Flows”, and a
beforeunloadprompt. - Keeps Save enabled during preview, stopping simulation before persisting the viewport/layout.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/packages/i18n/src/locales/en-US.ts | Adds i18n strings for discard-changes dialog + undo/redo labels. |
| frontend/apps/console/src/features/login-flow/hooks/useFlowSave.ts | Adds an optional onSaved callback to clear dirty state after persistence. |
| frontend/apps/console/src/features/login-flow/hooks/useFlowHistory.ts | New snapshot-based undo/redo hook + graph signature function. |
| frontend/apps/console/src/features/login-flow/hooks/tests/useFlowHistory.test.tsx | Unit tests covering history behavior (debounce, cap, undo/redo). |
| frontend/apps/console/src/features/login-flow/components/LoginFlowBuilder.tsx | Wires history + dirty tracking + shortcuts + beforeunload warning into login flow builder. |
| frontend/apps/console/src/features/flows/components/visual-flow/DiscardChangesDialog.tsx | New dialog component for confirming discard of unsaved changes. |
| frontend/apps/console/src/features/flows/components/visual-flow/DecoratedVisualFlow.tsx | Adds Save dirty indicator, discard dialog on navigation, and save-while-previewing behavior. |
| frontend/apps/console/src/features/flows/components/visual-flow/CanvasToolbar.tsx | Adds optional undo/redo buttons to the canvas toolbar. |
| frontend/apps/console/src/features/flows/components/visual-flow/tests/DiscardChangesDialog.test.tsx | Tests the discard dialog rendering and callbacks. |
| frontend/apps/console/src/features/flows/components/visual-flow/tests/DecoratedVisualFlow.test.tsx | Updates expectation: save remains enabled during preview. |
| frontend/apps/console/src/features/flows/components/visual-flow/tests/CanvasToolbar.test.tsx | Adds tests for undo/redo toolbar controls. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
def0327 to
16ff5e0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
frontend/apps/console/src/features/login-flow/components/LoginFlowBuilder.tsx:227
isDirtyis derived only from the nodes/edges signature and the baseline is currently set viasetSavedSignature(...)during render. This means (a) unsaved edits to the flow name/handle won’t trigger the dirty indicator or navigation guards, and (b) the render-time state update can lead to React warnings/unstable behavior (especially under StrictMode). Consider folding name/handle into the dirty signature and moving baseline initialization into an effect.
// Unsaved-changes tracking. The baseline is the graph signature captured after
// the initial load and after each successful save; the flow is dirty when the
// current graph diverges from it.
const [savedSignature, setSavedSignature] = useState<string | null>(null);
const currentSignature = useMemo(() => computeGraphSignature(nodes, edges), [nodes, edges]);
05e146b to
072af3a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
frontend/apps/console/src/features/login-flow/components/LoginFlowBuilder.tsx:233
- Calling setSavedSignature(...) during render can trigger React warnings/extra renders, and it can also capture the baseline before the initial edges are generated (new-flow initialization sets nodes first, then edges in a later RAF), which will immediately mark the flow dirty on load. Move baseline initialization into an effect and wait for the initial graph to fully settle (nodes + edges) before setting the baseline signature.
// The graph keeps mutating for a moment after load (metadata resolution, field
// normalization, node internals), so the baseline is captured only once the
// graph has gone quiet — not on the first frame, which would read as dirty.
const baselineReadyRef = useRef<boolean>(false);
frontend/apps/console/src/features/login-flow/hooks/useFlowHistory.ts:67
- computeGraphSignature() is order-dependent (iterates nodes/edges in array order) and it omits edge.type. That means semantically identical graphs can produce different signatures if array order changes, and edge-style changes (which update edge.type) won’t be reflected in history/dirty tracking. Sort nodes/edges by id and include edge.type in the signature.
export function computeGraphSignature(nodes: Node[], edges: Edge[]): string {
let signature = '';
for (const node of nodes) {
const data = node.data as StepData | undefined;
signature +=
frontend/apps/console/src/features/flows/components/visual-flow/CanvasToolbar.tsx:99
- Redo is enabled/disabled solely based on canRedo, but onRedo itself is optional. If a caller passes canRedo=true without an onRedo handler, the button will be enabled but do nothing. Make the disabled condition also depend on the handler being present.
<IconButton
size="small"
onClick={onRedo}
disabled={!canRedo}
sx={{borderRadius: 1, color: 'text.secondary'}}
072af3a to
583ef75
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/apps/console/src/features/flows/components/visual-flow/__tests__/DecoratedVisualFlow.test.tsx (1)
424-432: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the save interaction, not only button availability.
This test never clicks
save-flow-button, so it does not verify the behavior described by its title: stopping preview before saving. CaptureonSave, click Save while previewing, and assert persistence plus preview exit or canvas-fit behavior.Suggested test adjustment
- renderComponent(<DecoratedVisualFlow {...defaultProps} nodes={nodes} onSave={vi.fn()} />); + const onSave = vi.fn(); + renderComponent(<DecoratedVisualFlow {...defaultProps} nodes={nodes} onSave={onSave} />); expect(screen.getByTestId('save-flow-button')).toBeEnabled(); fireEvent.click(screen.getByTestId('simulate-flow-button')); expect(screen.getByTestId('save-flow-button')).toBeEnabled(); + fireEvent.click(screen.getByTestId('save-flow-button')); + expect(onSave).toHaveBeenCalledTimes(1);As per coding guidelines, write tests for new features and bug fixes, targeting at least 80% coverage.
🤖 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 `@frontend/apps/console/src/features/flows/components/visual-flow/__tests__/DecoratedVisualFlow.test.tsx` around lines 424 - 432, Update the test using DecoratedVisualFlow to capture the onSave mock, click save-flow-button after starting preview with simulate-flow-button, and assert that saving occurs while preview is stopped, including the expected preview exit or canvas-fit behavior. Retain the enabled-state assertion only as supporting coverage.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
`@frontend/apps/console/src/features/flows/components/visual-flow/DecoratedVisualFlow.tsx`:
- Around line 717-724: Replace the hardcoded '/flows' destinations in
handleBackToFlows and handleConfirmDiscard with the corresponding RouteConfig
route value. Ensure both navigate calls resolve their destination from
RouteConfig while preserving the existing discard-dialog behavior.
- Line 900: Update the Save label’s translation call in the component rendering
this header panel to provide the fallback string “Save”, matching the
surrounding i18n calls such as the one near line 862 and preserving the existing
translation key.
In
`@frontend/apps/console/src/features/flows/components/visual-flow/DiscardChangesDialog.tsx`:
- Around line 39-46: Update the Dialog in DiscardChangesDialog to set
aria-labelledby to a stable title identifier, and assign the same id to
DialogTitle. Also connect DialogContentText to the Dialog through
aria-describedby with a matching stable description identifier unless the
component library already wires it automatically.
---
Nitpick comments:
In
`@frontend/apps/console/src/features/flows/components/visual-flow/__tests__/DecoratedVisualFlow.test.tsx`:
- Around line 424-432: Update the test using DecoratedVisualFlow to capture the
onSave mock, click save-flow-button after starting preview with
simulate-flow-button, and assert that saving occurs while preview is stopped,
including the expected preview exit or canvas-fit behavior. Retain the
enabled-state assertion only as supporting coverage.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d42ca107-3aba-4b35-a298-bac329415480
📒 Files selected for processing (11)
frontend/apps/console/src/features/flows/components/visual-flow/CanvasToolbar.tsxfrontend/apps/console/src/features/flows/components/visual-flow/DecoratedVisualFlow.tsxfrontend/apps/console/src/features/flows/components/visual-flow/DiscardChangesDialog.tsxfrontend/apps/console/src/features/flows/components/visual-flow/__tests__/CanvasToolbar.test.tsxfrontend/apps/console/src/features/flows/components/visual-flow/__tests__/DecoratedVisualFlow.test.tsxfrontend/apps/console/src/features/flows/components/visual-flow/__tests__/DiscardChangesDialog.test.tsxfrontend/apps/console/src/features/login-flow/components/LoginFlowBuilder.tsxfrontend/apps/console/src/features/login-flow/hooks/__tests__/useFlowHistory.test.tsxfrontend/apps/console/src/features/login-flow/hooks/useFlowHistory.tsfrontend/apps/console/src/features/login-flow/hooks/useFlowSave.tsfrontend/packages/i18n/src/locales/en-US.ts
583ef75 to
82f85de
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
frontend/apps/console/src/features/flows/components/visual-flow/CanvasToolbar.tsx:57
showHistoryControlsbecomes true when either handler is provided, but the toolbar renders both buttons. If only one handler is passed (e.g.,onRedowithoutonUndo), the other button can render with anundefinedclick handler (and could even be enabled ifcanUndo/canRedoare set), leading to a non-functional control. Require both handlers before showing the history controls (or gate each button individually).
const showHistoryControls = Boolean(onUndo ?? onRedo);
frontend/apps/console/src/features/flows/components/visual-flow/DecoratedVisualFlow.tsx:412
- When saving during preview,
fitView()errors currently fall back topersistCanvas(). IffitView()rejects (which the codebase already treats as possible elsewhere), this can still persist the preview’s zoomed-in viewport, defeating the intent of stopping preview before saving. Consider retryingfitView()on the next animation frame before falling back to persisting.
const handleToggleSimulation = useCallback((): void => {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
frontend/apps/console/src/features/flows/components/visual-flow/CanvasToolbar.tsx:57
showHistoryControlsbecomes true when eitheronUndooronRedois provided, but the component always renders both buttons. That can surface an enabled button whose handler isundefined(e.g.,canUndotrue butonUndomissing), resulting in a non-functional control.
const showHistoryControls = Boolean(onUndo ?? onRedo);
frontend/apps/console/src/features/login-flow/components/LoginFlowBuilder.tsx:278
handleSavedclears the dirty baseline using the current nodes/edges at the moment the save request succeeds. Since the UI does not disable editing whileuseFlowSaveis pending, users can continue editing during the in-flight save; when the request resolves, this will incorrectly mark those post-click edits as saved (clearing the dirty indicator and rebasing history).
const handleSaved = useCallback(() => {
hasInteractedRef.current = true;
setSavedSignature(computeGraphSignature(nodes, edges));
resetHistory();
}, [nodes, edges, resetHistory]);
82f85de to
f54e247
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
frontend/apps/console/src/features/flows/components/visual-flow/DecoratedVisualFlow.tsx:754
handleBackToFlowsnavigates using a hard-coded'/flows'path, while the discard-confirm path usesRouteConfig.flows.list(). This can drift if route segments change; useRouteConfig.flows.list()consistently.
const handleBackToFlows = useCallback((): void => {
if (isDirty) {
setIsDiscardDialogOpen(true);
return;
}
// eslint-disable-next-line @typescript-eslint/no-floating-promises
navigate('/flows');
}, [isDirty, navigate]);
f54e247 to
41d4da3
Compare
Adds a snapshot-based undo/redo history for canvas edits (toolbar buttons and Ctrl/Cmd+Z / Ctrl/Cmd+Shift+Z), a dirty-state indicator with a navigation guard (discard-changes dialog on back, beforeunload on tab-close/reload), and lets Save run during preview by stopping the preview first. Refs thunder-id#4062
41d4da3 to
f87647e
Compare
Purpose
Adds a safety net for editing in the flow builder canvas, implementing all three parts of #4062:
MAX_HISTORY_ITEMS(20). Ctrl/Cmd+Z undoes, Ctrl/Cmd+Shift+Z (and Ctrl+Y) redoes; undo/redo buttons sit in the canvas toolbar next to auto-layout/zoom.beforeunloadprompt on tab-close/reload.Node/edge deletion stays a single click with no confirmation — with undo/redo in place that is the intended safety net, per the issue.
Approach
Undo/redo (snapshot history). Canvas mutations are scattered across ~15 hooks and go through both
setNodes/setEdgesand React Flow'supdateNodeData/deleteElements, so a newuseFlowHistoryhook observes the authoritativenodes/edgesat the state owner (LoginFlowBuilder) rather than wrapping every call site. A debounced (400ms) effect commits the prior graph to the undo stack once the graph settles into a new structural signature, so one drag or one burst of typing collapses to a single entry. Undo/redo apply cloned snapshots through the existing setters; an "applying" guard stops a restore from recording itself.undo/redo/canUndo/canRedothread down toCanvasToolbaras props (mirroringonAutoLayout); a keydown listener inLoginFlowBuilderhandles the shortcuts and ignores events from inputs/rich-text so native text undo still works.Dirty tracking + navigation guard.
LoginFlowBuildercaptures a graph-signature baseline after load and after each save (useFlowSavegained anonSavedcallback);isDirtyis the current-vs-baseline diff. The app usesBrowserRouter(nouseBlocker), so the guard is manual: abeforeunloadhandler for hard exits and a newDiscardChangesDialogon the Back-to-Flows button. (Known gap: SPA sidebar navigation isn't intercepted — that needs a data-router migration, deferred.)Save during preview. Since the preview only affects the viewport and transient CSS classes (never the authoritative nodes/edges),
handleSavestops the simulation, fits the view instantly, then persists.Scope decisions (confirmed before implementing): one PR for all three parts; local draft autosave via
AUTO_SAVE_INTERVALdeferred to a follow-up; manual nav guard rather than a router migration.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests