fix(datatypes): wire undo/redo snapshots and dirty tracking for data type edits [DOPE-534]

[JoaoGSP]

Summary

Fixes two pre-existing datatype editor bugs (DOPE-534, sub-task of DOPE-385):

Undo/redo was silently broken for data types. usePouSnapshot().captureAndPush and snapshotActions.undo/redo are POU-keyed and early-returned for datatype names, so every capture call in the datatype molecules was a no-op. Now:

• captureAndPush falls back to a data type lookup and pushes a dataTypes snapshot. The field is added to PouHistorySnapshot (shared/types.ts, a mirrored type) by this PR, mirroring the shape of the pre-existing-but-unused HistorySnapshot.dataTypes; what already existed unused and is now wired is projectActions.applyDatatypeSnapshot.
• snapshotActions.undo/redo branch on data type names and restore via the previously unwired projectActions.applyDatatypeSnapshot.
• The array molecule's initial-value and base-type edits now capture snapshots too (they didn't before), and addNewRow captures on every add instead of only the first.

Undo/redo divergence now flags dirty — POUs included. When undo/redo moves history away from the saved depth, the file + workspace are marked unsaved (previously only the "back at saved depth → mark saved" direction existed). This changes POU undo/redo behavior too, not just data types — without it, undoing away from a saved state left the file flagged saved while the store diverged from disk, and the next save-all silently skipped the revert.

Content edits didn't mark the project dirty. Only create/rename/delete/duplicate flagged unsaved. Now every content edit (enum values, struct fields via table cells/add/remove/reorder, array dimensions/base type/initial value) calls handleFileAndWorkspaceSavedState, matching the variables editor pattern.

Review follow-ups (Gustavo): capture-side tests for the hook fallback, capture/dirty unified on editor.meta.name (the data prop lags one render behind the store), renameHistory rekey so undo survives a rename (restored snapshots pin name to the current key).

Bonus fix folded in: the datatype header rename bypassed datatypeActions.rename (no name validation, no duplicate check, file-slice entry left orphaned under the old name). It now goes through the shared action and shows a toast on rejection.

Tests

• 5 new cases in shared-slice.test.ts covering datatype undo/redo, saved-depth restore, and snapshots without dataTypes entries.
• jest shared-slice suite: 163 passed; full suite 5433 passed. Typecheck, prettier, eslint clean.

Mirror of https://github.com/Autonomy-Logic/openplc-web/pull/649

🤖 Generated with Claude Code

https://claude.ai/code/session_014LCcPrjCcCF29NQbG8xbwA

Summary by CodeRabbit

• New Features

  • Added undo and redo support for data type changes, including enumerated, array, and structure data types.
  • Data type history is preserved and restored alongside related editor state.
  • Data type renaming now keeps history synchronized and shows an error notification if unsuccessful.

• Bug Fixes

  • Improved saved-state tracking after data type edits.
  • Corrected enumerated value synchronization and array dimension changes.
  • Grouped rapid array initial-value edits into clearer undo steps.

[coderabbitai]

Walkthrough

Data type editing now centralizes rename validation, tracks saved state after array, enumerated, and structure updates, and supports data type snapshots in undo and redo history. Tests cover restoration, history movement, saved-depth tracking, and missing snapshot data.

Changes

Data type state management

Layer / File(s)	Summary
Centralized data type rename src/frontend/components/_features/[workspace]/data-type/index.tsx	Rename handling now uses datatypeActions.rename. Failed renames restore the previous name and show an error toast.
Edit and saved-state tracking src/frontend/components/_molecules/data-types/array/..., src/frontend/components/_molecules/data-types/enumerated/..., src/frontend/components/_molecules/data-types/structure/...	Data type editors mark file and workspace state after edits. Array initial-value edits group snapshots by typing burst and rearm on blur or external changes.
Data type undo and redo src/frontend/hooks/use-pou-snapshot.ts, src/frontend/store/slices/shared/types.ts, src/frontend/store/slices/shared/slice.ts, src/frontend/store/__tests__/shared-slice.test.ts, src/frontend/hooks/__tests__/use-pou-snapshot.test.ts	Snapshots can contain data types. Undo and redo restore data type state and preserve existing POU behavior. Tests cover history and saved-state changes.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DataTypeEditor
  participant usePouSnapshot
  participant sharedSlice
  DataTypeEditor->>usePouSnapshot: Capture data type snapshot
  usePouSnapshot->>sharedSlice: Store dataTypes snapshot
  DataTypeEditor->>sharedSlice: Request undo or redo
  sharedSlice->>sharedSlice: Move snapshot between history stacks
  sharedSlice->>DataTypeEditor: Restore data type state

Loading

Possibly related PRs

• Autonomy-Logic/openplc-editor#940: Both changes update shared history and saved-state behavior.
• Autonomy-Logic/openplc-editor#948: Both changes modify snapshot capture and undo/redo behavior.
• Autonomy-Logic/openplc-editor#973: Both changes modify shared history and usePouSnapshot.

Suggested reviewers: thiagoralves

Poem

I’m a rabbit with snapshots tucked tight,
Data types hop through undo and redo tonight.
Names change safely, edits stay known,
Saved states follow each seed that is sown.
Tap, blur, restore—no history blight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the datatype undo/redo snapshot and dirty-tracking fixes.
Description check	✅ Passed	The description explains the changes, references DOPE-534, documents testing, and provides sufficient implementation context.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/DOPE-534-datatype-undo-dirty

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🤖 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 `@src/frontend/hooks/use-pou-snapshot.ts`:
- Around line 34-38: Disambiguate POU and datatype history entries in the
snapshot, undo, and redo flows around the POU resolution branch in
usePouSnapshot: use namespaced keys or propagate an explicit entity kind so a
shared name always restores the correct entity. Ensure datatype edits capture
datatype snapshots rather than matching a same-named POU, while preserving
existing behavior for unique names.

In `@src/frontend/store/__tests__/shared-slice.test.ts`:
- Around line 1483-1484: Remove the non-null assertions from the datatype
lookups in the shared-slice tests, including the occurrences near the Colors,
corresponding later fixtures, and their uses in pushToHistory. Narrow each
Array.find result with an assertion or test helper that throws when the datatype
is missing before passing it onward.

In `@src/frontend/store/slices/shared/slice.ts`:
- Around line 1121-1123: Update the undo/redo saved-state handling around
applyDatatypeSnapshot so the saved flag is derived from whether savedAtDepth
equals history.past.length after both operations, setting saved false when
depths differ. Add a regression test covering redo after the saved-depth undo
case and verify the workspace becomes unsaved.

🪄 Autofix

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: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 61da3794-29fc-4577-8bdb-ac1399fc0d36

📥 Commits

Reviewing files that changed from the base of the PR and between 7e502e1 and 1b9f166.

📒 Files selected for processing (11)

• src/frontend/components/_features/[workspace]/data-type/index.tsx
• src/frontend/components/_molecules/data-types/array/index.tsx
• src/frontend/components/_molecules/data-types/array/table/index.tsx
• src/frontend/components/_molecules/data-types/enumerated/index.tsx
• src/frontend/components/_molecules/data-types/enumerated/table/index.tsx
• src/frontend/components/_molecules/data-types/structure/index.tsx
• src/frontend/components/_molecules/data-types/structure/table/index.tsx
• src/frontend/hooks/use-pou-snapshot.ts
• src/frontend/store/__tests__/shared-slice.test.ts
• src/frontend/store/slices/shared/slice.ts
• src/frontend/store/slices/shared/types.ts

[Gustavohsdp]

Review — approve after one test, plus a few small adjustments

Reviewed together with the web counterpart Autonomy-Logic/openplc-web#649 — all 11 files are byte-identical between the repos, so everything below applies to both.

No logic defects found. The diagnosis is the kind that hides in plain sight: captureAndPush and undo/redo both did if (!pou) return, so every capture call in the datatype molecules was a no-op — the shortcut existed and did nothing. The fix is minimal and lands in the right places (fallback in the lookup, branch in the restore, the already-unused applyDatatypeSnapshot).

Three details show real care: the useRef that collapses a typing burst into one history entry (without it every keystroke in the initial-value field would be its own undo step), addNewRow capturing on every add instead of only the first, and the header rename moving off a raw updateDatatype onto datatypeActions.rename (validation + toast + rekeying file/tab/editor).

How I verified: 11/11 files byte-identical across the repos; shared-slice.test.ts 165/165 under both runners (vitest on web, jest on the editor); tsc, prettier, validate:arch clean in both; CI green on both. I also checked each new behaviour is pinned by a test — one isn't, and that's the main ask below.

---

🟡 1 — The fallback that makes the feature work has no test (the one thing I'd like before merge)

hooks/use-pou-snapshot.ts:34-39

if (!pou) {
  const dataType = project.data.dataTypes.find((d) => d.name === pouName)
  if (!dataType) return
  pushToHistory(pouName, { variables: [], body: null, dataTypes: [dataType] })
  return
}

What I found. I deleted that whole block (back to if (!pou) return) and all 165 tests still passed. The seven new tests call pushToHistory directly with a hand-built { dataTypes: [...] } snapshot, so they cover the restore side and never the capture side.

Why it needs fixing. The capture side is the side that was broken — the original bug was precisely "no snapshot is taken". As it stands, if someone simplifies this hook later, datatype undo silently reverts to a no-op and nothing in the suite complains. And the symptom (a shortcut that quietly does nothing) is exactly the kind nobody notices until a user reports it. There's no test file for use-pou-snapshot.ts or for the molecules today, and neither path is under a coverage threshold, so nothing forces it.

Suggested fix. A renderHook test (the repo already has @testing-library/react): call captureAndPush('Colors') and assert undoRedo['Colors'].past[0].dataTypes. Cheaper alternative: extract the body into a pure buildSnapshotFor(state, name) and test that.

---

🟡 2 — The dirty-tracking half is also untested (fine as follow-up)

Every new handleFileAndWorkspaceSavedState call lives in the six molecules and the feature index — files with no tests at all. I checked the part that actually matters by hand: all of them are inside handlers, none inside a useEffect, so opening a datatype doesn't dirty the project. The initialValue effect was rewritten to compare against lastWrittenInitialValue.current and only re-arm the capture, without touching the dirty flag. It's correct — it just isn't protected.

With eight call sites across seven files, one component test for the most representative case (edit a struct table cell → getSavedState({ name }) === false) would buy the guarantee without turning into a project of its own.

---

🟡 3 — Capture and dirty use different name sources

In the molecules, capture uses editor.meta.name while the dirty call uses data.name.

Why it needs fixing. Your own comment states that data lags one render behind the store ("the parent copies it via effect"). In the window where the two disagree — right after a rename — the capture goes to the new key and the dirty call to the old one. handleFileAndWorkspaceSavedState then fails to find the file entry (the rename already rekeyed it), hits the console.warn('File with name X does not exist.') at slice.ts:488, and marks nothing dirty.

Impact. Narrow window, but rename-then-edit is now a first-class flow because of this PR's header-rename fix, and the failure is silent: the user's edit isn't flagged unsaved. Using editor.meta.name on both sides removes the whole class.

---

🟡 4 — This PR and its sibling collide in three files

Both this and the .dt persistence PR (openplc-web#651 / openplc-editor#991) are open, both branch from development, and both touch:

src/frontend/store/slices/shared/slice.ts
src/frontend/store/slices/shared/types.ts
src/frontend/store/__tests__/shared-slice.test.ts

Why it matters. Whichever merges second needs a rebase, and since all three are mirrored-surface files, byte-identity has to be re-verified after conflict resolution — that's exactly where a mirror breaks. This PR looks like the natural one to land first (smaller, no flag). Worth agreeing the order explicitly and warning whoever rebases.

---

🟡 5 — Two claims in the description don't match the diff

1. "(the already-existing but never-wired PouHistorySnapshot.dataTypes field)" — I checked development: PouHistorySnapshot has variables, body, globalVariables, ladderFlow, fbdFlow. The dataTypes field is added by this PR (shared/types.ts). What did already exist unused is applyDatatypeSnapshot, which the next sentence describes correctly. As written it reads as though no shared type changed — one did, on a mirrored file.
2. The summary attributes the dirty tracking to content edits in the molecules only, but the else { handleFileAndWorkspaceSavedState(...) } you added in undo/redo applies to POUs as well — it changes POU undo behaviour, not just datatype. It's correct (it's the fix for CodeRabbit's saved-depth finding) and it's in the second commit's message, but not in the summary, which is what most people read.

---

🟢 Smaller things

• snapshot.dataTypes?.[0] paired with dataTypes: [dataType] — a single-element array on both sides. If it's always one, a dataType?: PLCDataType field says so in the type and drops the ?.[0] in both branches. If it's room for multi-type later, a comment would help; today the reader can't tell which.
• Rename silently discards history. renameElement updates the editor model, file entry and tab, but doesn't move the undoRedo key — after a rename, undo is a no-op and the old stack is orphaned in the map. Pre-existing for POUs, but it starts mattering for datatypes now that they have history, and this PR is what makes the header rename an official path. Cheap to fold into the rekeying renameElement already does.
• body: null for datatype snapshots works (body: unknown, and applyPouSnapshot only runs in the POU branch), but the field now carries two meanings — one line of comment next to dataTypes covers it.

---

What's good

• The bug is real and specific, not speculative refactoring: if (!pou) return made every datatype capture a no-op.
• A typing burst becomes one history entry, armed on the first keystroke and re-armed on blur or when the store changes underneath (undo/redo). Without it the feature would "work" and be unusable.
• useState(data.initialValue || '') replacing the mount effect removes a render with an empty field, for free.
• The header rename now goes through datatypeActions.rename with local-state rollback and a toast on rejection — before it skipped name validation and duplicate checks and left the file entry orphaned under the old name.
• Both actionable CodeRabbit findings are genuinely closed: the ! assertions are gone from the tests (helper that throws), and saved-depth divergence now marks dirty — that one is pinned by tests, which I confirmed by removing the branch and watching exactly those two fail.
• The reply to the third finding is the best part of the thread: rather than namespacing just the history keys, it points out the whole workspace is keyed by raw element name (file slice, tabs, editor models, library entries), that IEC puts POUs and derived types in one identifier namespace, and files DOPE-538 to reject cross-kind duplicates at the source. Agreed on both the diagnosis and the choice.
• Mirror discipline holds (11/11) and CI is green on both sides.

---

One thread that connects three PRs

Looking at the series together, DOPE-538 is the root cause behind three symptoms that have now surfaced in three separate reviews:

Where	Symptom of the same root cause
here	POU and datatype sharing a name → capture resolves the POU first, so editing the datatype captures the POU and undo touches the wrong element
the .dt PR	the same name becomes a file path — and in my blocker there, Foo / foo collide on datatypes/Foo.dt on macOS/Windows
pre-existing	tabs, editor models and file entries all keyed by raw name

Until DOPE-538 lands, each PR in the series will trip over one facet of it. Worth prioritising before the DOPE-385 release PR — and note the case-folding blocker I raised on the .dt PR probably belongs to DOPE-538's scope rather than that PR's.

_{Review assisted by Claude Code.}

[JoaoGSP]

Thanks — all actionable items are addressed in the latest commit (web c2e0c4e3 / editor 8d070f5d9, byte-identical as usual):

#1 (capture-side test) — new hooks/__tests__/use-pou-snapshot.test.ts with 4 renderHook cases against the real store: datatype capture (asserts undoRedo[name].past[0].dataTypes), capture of the current state after an edit, the POU path, and the neither-POU-nor-datatype no-op. Deleting the fallback block now fails the suite.

#3 (name-source mismatch) — all dirty-marking calls in the datatype molecules now use editor.meta.name, same as capture. You were right that the data-prop lag made the old-key console.warn path reachable right after a rename.

🟢 rename discards history — folded in: renameElement now rekeys the undoRedo bucket via a new snapshotActions.renameHistory, and restored datatype snapshots pin name to the current history key (a pre-rename snapshot would otherwise restore the old name into the renamed entry and desync tabs/files/editors). Covered by unit tests + a rename→undo integration test.

🟢 doc nits — one-liners added on PouHistorySnapshot.dataTypes (single-element today, array shape mirrors HistorySnapshot) and the null body of datatype snapshots.

#5 (description drift) — both PR bodies corrected: dataTypes is added to PouHistorySnapshot by this PR (what pre-existed unused was applyDatatypeSnapshot and the HistorySnapshot shape), and the divergence-dirty else-branch is now called out as a POU behavior change too.

#2 (dirty-tracking component test) — deferring as you suggested; proposal is to fold it into the DOPE-385 form/code-toggle subtask, which already has to touch these molecules.

#4 (merge order) — agreed this one lands first: it's smaller and flag-free. Whoever rebases the .dt PR afterwards should re-run compare-surfaces.py after resolving shared/slice.ts / shared/types.ts / shared-slice.test.ts.

Root-cause thread — DOPE-538 updated with your case-folding point: IEC identifiers are case-insensitive and .dt names become file paths, so the uniqueness gate will compare case-insensitively across POUs + datatypes; the Foo/foo blocker from the .dt PR is folded into its scope.