Add state management, undo / redo capabilities and component API - #324
Add state management, undo / redo capabilities and component API #324handreyrc wants to merge 9 commits into
Conversation
✅ Deploy Preview for openworkflow-editor ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds editor state/history management to support undo/redo (including viewport + selection restore) and exposes a new imperative API on the editor ref for integration/testing.
Changes:
- Introduces generic
useHistoryand workflow-specificuseWorkflowHistoryhooks, plus structural equality comparison to avoid redundant history entries. - Refactors
Diagramand store context provider to seed/history-track models and restore viewport/selection during undo/redo. - Adds Storybook feature story + tests for history behavior and ref API; adds
fast-equalsdependency.
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Adds fast-equals to the workspace catalog. |
| packages/open-workflow-diagram-editor/package.json | Adds fast-equals dependency for structural comparisons. |
| packages/open-workflow-diagram-editor/src/core/hooks/structuralEqual.ts | Implements constructor-agnostic deep structural equality with circular handling. |
| packages/open-workflow-diagram-editor/src/react-flow/hooks/useHistory.ts | Adds generic past/present/future history reducer + hook with stack cap. |
| packages/open-workflow-diagram-editor/src/react-flow/hooks/useWorkflowHistory.ts | Adds workflow-aware history snapshots (model/viewport/selection) + undo/redo behavior. |
| packages/open-workflow-diagram-editor/src/store/DiagramEditorContext.tsx | Extends context type with history + content-format APIs. |
| packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx | Seeds history from content, exposes imperative API, and wires history into context. |
| packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx | Gates ReactFlow mount until first layout, submits snapshots, and restores viewport on undo/redo. |
| packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx | Refactors editor shell + docs and attempts to expose imperative API via context provider. |
| packages/open-workflow-diagram-editor/src/styles.css | Removes stray trailing whitespace. |
| packages/open-workflow-diagram-editor/stories/features/UndoRedoEditor.tsx | Adds a Storybook wrapper with undo/redo toolbar + window-exposed ref. |
| packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx | Adds Storybook docs + interactive story for undo/redo and ref API. |
| packages/open-workflow-diagram-editor/tests/react-flow/hooks/useHistory.test.ts | Adds unit tests for generic history reducer/hook behavior. |
| packages/open-workflow-diagram-editor/tests/react-flow/hooks/useWorkflowHistory.test.ts | Adds tests for workflow history snapshots, equality behavior, and viewport restore. |
| packages/open-workflow-diagram-editor/tests/core/hooks/structuralEqual.test.ts | Adds comprehensive tests for structural equality across class/plain + circular refs. |
| packages/open-workflow-diagram-editor/tests/react-flow/diagram/Diagram.test.tsx | Updates tests to wait for delayed ReactFlow mount after layout gating. |
| packages/open-workflow-diagram-editor/tests/store/DiagramEditorContextProvider.test.tsx | Updates expected render cycles due to history seeding effect. |
| packages/open-workflow-diagram-editor/tests/diagram-editor/DiagramEditor.test.tsx | Expands tests for ref API (undo/redo/getContent/setContent) and async canvas-dependent UI. |
| .changeset/state-management.md | Publishes a minor version bump describing new state/history + API. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // setIsReadOnly is intentionally inoperative: isReadOnly is driven by | ||
| // props, not internal state, so there is no local setter to dispatch to. | ||
| setIsReadOnly: () => {}, |
There was a problem hiding this comment.
Fixed! I opted for removing setIsReadOnly from DiagramEditorContextType.
@lornakelly @fantonangeli @kumaradityaraj, lets be careful with this one. I couldn't find any side effect but it is good to double check it.
There was a problem hiding this comment.
@handreyrc opening the preview and settings isReadOnly to false, I could not move the nodes in the diagram. Am I missing somenthing?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 21 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (8)
packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:51
- The comment says the content format is fixed at mount time, but
setContent()can updatecontentFormat.current. This is misleading documentation and makes it harder to reason aboutgetContent()behavior.
// Detect the serialization format once from the initial content prop.
// JSON content starts with `{` (after trimming); everything else is YAML.
// We use a ref so the format is fixed at mount time and never flips mid-session
// (an undo/redo should round-trip back in the same format the host provided).
packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:89
- Undo/redo changes the
modelfrom history, buterrorsare currently tied toprops.content. Derivingerrorsfrom the currentmodelkeeps validation/error-highlighting consistent across undo/redo snapshots, while still using parse errors when no model is available.
// parseWorkflow drives both errors and the external-content model source.
// errors are never part of a snapshot — always recomputed from current content.
const { model: parsedModel, errors } = React.useMemo(
() => parseWorkflow(props.content),
[props.content],
packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:131
- If
applyAutoLayoutthrows on the initial render,layoutReadystaysfalseand the ReactFlow canvas never mounts, leaving the editor blank. Consider falling back to rendering the un-laid-out graph (or at least settinglayoutReadyto true) on non-abort errors.
.catch((error) => {
if (error.name === "AbortError") {
return;
}
console.error("Failed to apply auto-layout:", error);
packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:86
- This prop doc says the serialization format is preserved for the lifetime of the component, but the ref API docs (and tests) indicate the format can change after a successful
setContent()call. Please align the documentation with the actual behavior.
* The serialisation format is auto-detected on first load and preserved for
* the lifetime of the component — see `getContent()` on `DiagramEditorRef`.
packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:183
DiagramEditorcomputes a fallbacklocale(and uses it for<I18nProvider>and thelangattribute), butDiagramEditorBodypasses the rawprops.localedown intoDiagramEditorContextProvider. Iflocaleis omitted at runtime, the context provider can receiveundefinedand diverge from the I18n provider.
props={props}
packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:19
- Undo/redo changes the
modelfrom history, buterrorsare derived fromparseWorkflow(props.content)and therefore won’t match the restored snapshot. This can make error highlighting inconsistent after undo/redo or after imperativesetContent()(which doesn’t changeprops.content).
This issue also appears on line 85 of the same file.
import { buildFlatGraph, getTaskReferences, parseWorkflow } from "../core";
packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:101
- The Storybook docs state that
getContent()format is fixed at mount time, but the implementation/tests describe format switching after a successfulsetContent()call. This section is internally inconsistent (it later says the format becomes the new format) — please make the docs unambiguous.
Serialises the current model back to the **same format the editor received
on first load**: if the initial \`content\` prop was JSON it returns JSON; if
it was YAML it returns YAML. The format is fixed at mount time and preserved
for the lifetime of the component, so undo/redo always round-trips in the
original format. Returns \`""\` when no valid model has been loaded yet.
packages/open-workflow-diagram-editor/tests/test-utils/render-helpers.tsx:45
DiagramEditorContextTypenow requirescontentFormatand the history API members, but the test mock context value doesn’t provide them. This should be a type error and may also cause runtime issues in tests that rely on these fields.
edges: [],
taskReferences: new Set(),
selectedNodeId: null,
setLocale: noop,
setEdges: noop,
setNodes: noop,
|
Thanks for PR @handreyrc, looks really good, have just tested the story so far but noticed a couple of things:
Screen.Recording.2026-08-12.at.10.46.38.mov |
fantonangeli
left a comment
fantonangeli
left a comment
There was a problem hiding this comment.
I left a small comment which you can consider
| export type HistoryState<T> = { | ||
| /** Past snapshots, oldest first. Length is capped at HISTORY_STACK_SIZE. */ | ||
| past: T[]; | ||
| /** The current snapshot. Null when the history has not been initialised yet. */ | ||
| present: T | null; | ||
| /** Future snapshots available for redo. Index 0 is the most recently undone entry. */ | ||
| future: T[]; | ||
| }; |
There was a problem hiding this comment.
The current history implementation uses three separate arrays (past, present, future) which works correctly. However, I wanted to share an alternative pattern that might simplify the code:
type HistoryState<T> = {
history: T[];
presentIndex: number;
};This way future is simply presentIndex+1.
Wdyt?
There was a problem hiding this comment.
Sure, if we can make it simpler why not?!
I changed the implementation following your recommendation.
Thanks!
| // setIsReadOnly is intentionally inoperative: isReadOnly is driven by | ||
| // props, not internal state, so there is no local setter to dispatch to. | ||
| setIsReadOnly: () => {}, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
packages/open-workflow-diagram-editor/src/core/hooks/structuralEqual.ts:81
innerEqualsdoesn’t short-circuit when comparing the same reference (e.g.a === b). In this PR the history pipeline callsstructuralEqual(present.model, model)frequently with identical object references, so missing this fast-path can turn routine viewport/selection updates into expensive deep traversals.
): boolean {
if (isObjectLike(a) && isObjectLike(b)) {
packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:221
- Viewport pan/zoom changes don’t appear to be persisted into the current history snapshot:
submitModel(...)is only called after layout cycles (and indirectly on selection changes via the effect deps), but there is no subscription to viewport changes. That means if a user pans/zooms and then later triggers an undo/redo, the restored viewport can be stale (typically the last fitView/restored value, not where the user was looking). Hook into React Flow viewport updates (e.g. a viewport/move end callback or store subscription) and callsubmitModel(model, viewport, selectedNodeId)souseWorkflowHistorycan update the present snapshot without pushing a new entry.
onNodesChange={onNodesChange}
onEdgesChange={onEdgesChange}
onSelectionChange={onSelectionChange}
onlyRenderVisibleElements={true}
zoomOnDoubleClick={false}
elementsSelectable={true}
panOnScroll={true}
panOnDrag={false}
zoomOnScroll={false}
preventScrolling={true}
selectionOnDrag={true}
fitView
fitViewOptions={{ ...FIT_VIEW_OPTIONS, duration: 0 }}
packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:51
- The comment says
contentFormatis fixed at mount time and “never flips mid-session”, butsetContent()later updatescontentFormat.current(andcontentFormatVersionexists specifically to re-render when it changes). This is misleading documentation and makes it harder to reason about the ref API contract.
// Detect the serialization format once from the initial content prop.
// JSON content starts with `{` (after trimming); everything else is YAML.
// We use a ref so the format is fixed at mount time and never flips mid-session
// (an undo/redo should round-trip back in the same format the host provided).
5aa160c to
0a70df4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:119
- DiagramEditorContent renders ParsingErrorPage whenever model is null. Since DiagramEditorContextProvider seeds history in a useEffect, valid content briefly produces model=null on the initial render, causing an incorrect error page flicker. Gate the error page on the presence of actual parse errors (or render a neutral placeholder) until the initial parse/seed completes.
const { model } = useDiagramEditorContext();
return model === null ? (
<ParsingErrorPage />
) : (
<Diagram divRef={diagramDivRef} colorMode={colorMode} />
);
The toolbar does not make sense in all contexts the component can be used, however, we need it to showcase how to consume the API so it was completely moved to the "Undo Redo" story and is not part of the editor component anymore. Thanks for reviewing this PR! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 21 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:121
- Selection preservation during layout rebuild only stamps
selected: trueonto the selected node. If the selected element is an edge, the rebuilt edge list will not mark it selected, so React Flow will drop the edge selection and z-index won’t reflect the selection.
const stampedNodes = selectedId
? nodes.map((n) => (n.id === selectedId ? { ...n, selected: true } : n))
: nodes;
setNodes(stampedNodes);
setEdges(applyEdgeZIndex(edges));
packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:99
- onSelectionChange only considers selected nodes and ignores selected edges, so selecting an edge clears selectedNodeId. This prevents edge selection from being preserved across content reloads and undo/redo snapshots (which expect node/edge IDs).
This issue also appears on line 117 of the same file.
const onSelectionChange = React.useCallback<RF.OnSelectionChangeFunc>(
({ nodes: selectedNodes }) => setSelectedNodeId(selectedNodes[0]?.id ?? null),
[setSelectedNodeId],
);
|
@fantonangeli @lornakelly @kumaradityaraj , This PR is ready for reviewing again. Thanks |
fantonangeli
left a comment
fantonangeli
left a comment
There was a problem hiding this comment.
LGTM, thanks a lot @handreyrc
|
@handreyrc I found a bug with the zoom:
Testing this with the last PR merged on Screencast.From.2026-08-13.12-35-13.mp4 |
|
@lornakelly @fantonangeli @kumaradityaraj, This PR is ready for reviewing again. Thanks |
|
@handreyrc, before I re-review this, it seems the test logs are getting really big: Also, I think it would be good if you sync with |
lornakelly
left a comment
lornakelly
left a comment
There was a problem hiding this comment.
LGTM pending fabrizios feedback
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:116
contentFormatis only updated by the imperativesetContent()path. When the host updatesprops.contentfrom YAML→JSON (or vice-versa), history is reseeded butcontentFormatremains stale, sogetContent()can serialize in the wrong format (and undo/redo will round-trip using that wrong format). UpdatecontentFormatin theprops.contentseeding effect after a successful parse so it always reflects the most recently loaded content string.
React.useEffect(() => {
const { model: parsedModel, errors: parsedErrors } = parseWorkflow(props.content);
setErrors(parsedErrors);
if (parsedModel === null) {
packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:149
DiagramEditorcomputes a normalized/fallbacklocale(used for<I18nProvider>and the rootlangattribute), butDiagramEditorContextProviderreceivesprops.localedirectly. If a JS consumer passes an undefined/empty/unsupported locale, context consumers can see a different value than i18n is actually using. Pass the same resolved locale string into the context provider to keep editor state consistent.
locale={props.locale}
packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:196
navigator.clipboard.writeText(...)can reject (permissions, insecure context, missing API), which will currently produce an unhandled promise rejection in Storybook. Handle missing Clipboard API and add a.catch(...)so the story stays stable across environments.
navigator.clipboard.writeText(getContentText).then(() => {
setCopied(true);
setTimeout(() => setCopied(false), 2000);
});
};
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (5)
packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:86
- The
contentprop doc says the serialization format is “preserved for the lifetime of the component”, but the implementation explicitly changes the format whensetContent()loads a different format. This makes the API contract unclear for consumers.
* The workflow definition to visualise, as a YAML or JSON string.
* Updating this prop (e.g. from an addon panel) re-parses the workflow and,
* in edit mode, pushes a new history entry if the model changed structurally.
* The serialisation format is auto-detected on first load and preserved for
* the lifetime of the component — see `getContent()` on `DiagramEditorRef`.
packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:163
- In the auto-layout Promise catch,
setLayoutError(...)can still run after the effect has been cleaned up (e.g. model changes/unmount) because it doesn’t checkisActive/abort state. That can trigger React warnings about setting state on an unmounted component and can also surface stale errors from a cancelled layout.
.catch((error) => {
if (error.name === "AbortError") {
return;
}
setLayoutError(error instanceof Error ? error : new Error(String(error)));
packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:116
contentFormatis only updated bysetContent(). If a host updates the externalcontentprop from YAML→JSON (or vice-versa),getContent()will keep serializing using the old format, which conflicts with the stated “most recently loaded content” behavior and can break format preservation across undo/redo after external reloads.
React.useEffect(() => {
const { model: parsedModel, errors: parsedErrors } = parseWorkflow(props.content);
setErrors(parsedErrors);
if (parsedModel === null) {
// Content is unparseable — reset history to null so downstream consumers
packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:592
- This story’s docs are internally contradictory:
getContent()is described as “fixed at mount time”, butsetContent()is described as changing the format for subsequentgetContent()calls (and the implementation/tests do change it). This can confuse integrators.
Serialises the current model back to the **same format the editor received
on first load**: if the initial \`content\` prop was JSON it returns JSON; if
it was YAML it returns YAML. The format is fixed at mount time and preserved
for the lifetime of the component, so undo/redo always round-trips in the
original format. Returns \`""\` when no valid model has been loaded yet.
packages/open-workflow-diagram-editor/vitest.config.ts:37
onConsoleLogis configured at the roottestlevel, so it suppresses “not wrapped in act” warnings for all Vitest projects (including unit tests), not just Storybook/Chromium tests as the comment implies. Either scope this filter to the Storybook project, or update the comment to reflect the actual behavior.
// Suppress React's "not wrapped in act()" warnings emitted as stderr during
// Storybook (Chromium) tests. These all originate from @xyflow/react internal
// components, not from our own code. They cannot be fixed here because:
@fantonangeli, |
|
This PR is ready for review again! Thanks. |
|
@handreyrc seeing issues when I check storybook Screen.Recording.2026-08-14.at.15.55.36.mov |
fb7e4c3 to
91ad2ad
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 26 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (6)
packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:86
- The
contentprop doc says the serialization format is preserved for the lifetime of the component, but the ref API + tests indicate the format can change when new content is loaded (e.g., viasetContent()and/or host updates). Please align this doc string with the actual behavior/contract.
* Updating this prop (e.g. from an addon panel) re-parses the workflow and,
* in edit mode, pushes a new history entry if the model changed structurally.
* The serialisation format is auto-detected on first load and preserved for
* the lifetime of the component — see `getContent()` on `DiagramEditorRef`.
packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:107
contentFormatis only updated bysetContent(), not when the externalcontentprop changes. This can makegetContent()serialize in the wrong format after a host-driven reload (e.g., YAML → JSON via prop update), contradicting the ref API/docs that say format is re-detected when content is loaded.
const { model: parsedModel, errors: parsedErrors } = parseWorkflow(props.content);
setErrors(parsedErrors);
if (parsedModel === null) {
packages/open-workflow-diagram-editor/.storybook/preview.tsx:31
- Typo in comment: “modifiy” → “modify”.
disableSaveFromUI: true, // Disable modifiy story popup. Stories mustn't be editable from Storybook UI.
packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:588
- The story docs currently say
getContent()is fixed to the initial mount format, but the same document later sayssetContent()re-detects and switches the format. Please make thegetContent()section consistent with the documented API behavior.
Serialises the current model back to the **same format the editor received
on first load**: if the initial \`content\` prop was JSON it returns JSON; if
it was YAML it returns YAML. The format is fixed at mount time and preserved
for the lifetime of the component, so undo/redo always round-trips in the
original format. Returns \`""\` when no valid model has been loaded yet.
packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:565
- The docs snippet uses the package name
@openworkflowspec/open-workflow-diagram-editor, but this package’spackage.jsonname is@openworkflowspec/diagram-editor, so the import example won’t work as written.
import { useRef } from "react";
import { DiagramEditor, DiagramEditorRef } from "@openworkflowspec/open-workflow-diagram-editor";
packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:185
DiagramEditorcomputes a resolvedlocale(includingdetectLocalefallback), but passes the originalpropsobject down. This meansDiagramEditorContextProviderreceivesprops.localerather than the resolved locale, so the context locale can diverge from theI18nProviderlocale.
<DiagramEditorBody
diagramDivRef={diagramDivRef}
resolvedColorMode={resolvedColorMode}
props={props}
editorRef={ref}
/>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
91ad2ad to
23f6fb5
Compare
@lornakelly, There were some layered issues going on:
I did a regression test locally and the PR is ready for review again. Thanks! |
Closes #318
Summary
This PR adds state management as the model changes, undo/redo capabilities by handling a stack of states, and implements an API to expose those features.
Changes
getContent,setContent,undo,redo,canUndo,canRedo, andcolorMode, so it is possible to interact with the editor component by exposing the editor'srefand calling those functions from the browser console, making it easier to integrate with external components.DiagramEditor, store, diagram error handling, and I18n were refactored and optimized to accommodate state management and the changes in thecontextProvider.fast-equalslibrary to detect structural changes between the current model and the new model.