feat(data-types): add a form/code toggle to the data type editor (DOPE-535) - #995
Merged
Merged
Conversation
Every data type tab gains the variables-style table/Monaco switch, with the code side showing that type's `.dt` text (DOPE-535). Committing the buffer parses it through `parseDataTypeFromText`, so an invalid declaration keeps the user on the text instead of silently dropping the edit, and renaming in the text stays rejected — the file name is the type's identity. `StructureTableType` becomes the same discriminated union the variables table uses, which is why `updateModelStructure` grows `display`/`code` and gains a name-scoped sibling: every open data type is mounted at once, so a background editor writing through the active-editor action would land on the wrong model. `display` is optional there on purpose — a row selection must never flip the view. The reconcile/regenerate pair keeps the buffer and the store in lockstep when something outside the code view moves the type. A tree rename folds pending text edits in first and refuses on invalid text, rather than regenerating over work the user hasn't committed; undo, redo and a disk revert regenerate so Monaco shows the restored state. An unreadable `datatypes/<Name>.dt` has no `PLCDataType`, so it can't appear in the project tree and until now had no way of being found or fixed in-app. Project open now registers it and pre-opens a code-mode tab holding the raw bytes, without stealing focus from the auto-opened POU; committing valid text promotes it to a real type. The store can't raise a toast (layer rule), so the parse warning still goes to the console. Gated by `isDataTypeFilesEnabled()`, which ships false — DOPE-542 owns the flip. Refs DOPE-535 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvGrhB1LyJz9MBwYhjoBDY
Contributor
WalkthroughThe PR adds datatype code-view editing with table/code switching, parsing, validation, model synchronization, snapshot regeneration, rename reconciliation, and support for unreadable ChangesDatatype code-view editing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DataTypeEditor
participant ProjectSlice
participant Parser
participant EditorModel
DataTypeEditor->>ProjectSlice: Reconcile edited datatype text
ProjectSlice->>EditorModel: Read code buffer
ProjectSlice->>Parser: Parse datatype text
Parser-->>ProjectSlice: Parsed model or parse error
ProjectSlice->>EditorModel: Regenerate synchronized code text
ProjectSlice-->>DataTypeEditor: Commit result or validation error
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/frontend/components/_features/[workspace]/data-type/index.tsx (1)
170-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
as Nodeassertions withinstanceof Nodenarrowing.Lines 172 and 180 assert
EventTargettoNode. The coding guidelines forbid type assertions other thanas const.containsalso accepts a non-Nodetarget at runtime without the assertion telling you so.♻️ Proposed refactor
const onDocMouseDown = (e: MouseEvent) => { if (!containerRef.current) return - if (containerRef.current.contains(e.target as Node)) return + const target = e.target + if (target instanceof Node && containerRef.current.contains(target)) return tryCommit() } // Covers keyboard navigation, Tab and shortcuts — anything that // moves focus away without a mousedown. const onFocusOut = (e: FocusEvent) => { if (!containerRef.current) return - const newTarget = e.relatedTarget as Node | null - if (newTarget && containerRef.current.contains(newTarget)) return + const newTarget = e.relatedTarget + if (newTarget instanceof Node && containerRef.current.contains(newTarget)) return tryCommit() }As per coding guidelines: "Do not use type assertions, except
as const".🤖 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/frontend/components/_features/`[workspace]/data-type/index.tsx around lines 170 - 183, Replace the EventTarget-to-Node assertions in onDocMouseDown and onFocusOut with instanceof Node checks before calling containerRef.current.contains. Preserve the existing behavior for targets inside the container and for null relatedTarget values, while complying with the no-type-assertions guideline.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 `@src/frontend/components/_features/`[workspace]/data-type/index.tsx:
- Around line 273-299: Update the visualization toggle around TableIcon and
CodeIcon by wrapping each icon in a native button with type='button'. Move each
icon’s aria-label to its corresponding button, add aria-pressed based on the
active display value, and keep the existing click handlers and visual styling
behavior intact.
- Around line 159-192: Update the commit tracking inside the useEffect’s
tryCommit flow so a failed commit records the current editorCode as the last
rejected buffer and skips subsequent attempts for that same unchanged buffer.
Preserve retry behavior after the editor content changes, while ensuring
focusout and mousedown handlers cannot trigger duplicate commits or toasts for
one failed interaction.
In `@src/frontend/components/_molecules/data-types/structure/index.tsx`:
- Around line 49-60: The structure view effect currently uses the active editor
and the first structure data type instead of this component’s own model. Update
the component to accept the data type name, select the matching model from
editors/dataTypes by name like the parent’s existing logic, and use that model
for the type, display, description, and selectedRow checks; include dataTypes in
the effect dependencies.
In `@src/frontend/store/slices/editor/slice.ts`:
- Line 17: The description update in
src/frontend/store/slices/editor/slice.ts#L17-L17 should preserve an explicitly
provided empty string, falling back to prevDescription only when
data.description is undefined; prefer the appropriate nullish-default behavior.
Update the expectation in
src/frontend/store/__tests__/editor-slice.test.ts#L292-L304 so
updateModelStructure({ description: '' }) results in description: ''.
In `@src/frontend/store/slices/project/slice.ts`:
- Around line 539-546: Update reconcileDatatypeText so replacing
project.data.dataTypes[idx] also keeps editorActions.meta.derivation and related
tab metadata synchronized with the parsed dataType, or rejects reconciliation
when derivation changes. Ensure the data-type table selects the correct editor
for enum, structure, and array buffers, and add a regression test covering
derivation-changing code.
In `@src/frontend/store/slices/shared/slice.ts`:
- Around line 1002-1015: Gate the unparsedDataTypes tab/model creation loop on
isDataTypeFilesEnabled(), including the related file registry entry added
nearby. Only call updateTabs, addModel, and updateModelStructureForName when the
flag is enabled; preserve the existing behavior when it is disabled.
- Around line 681-689: Filter the unparsedDataTypes collection before it is
consumed, excluding entries whose name matches any POU name or parsed data type
name from data.projectData.dataTypes. Reuse the existing raw-name identifiers
used by the surrounding file-entry, tab, and editor-model logic so unreadable
.dt files cannot overwrite existing elements.
---
Nitpick comments:
In `@src/frontend/components/_features/`[workspace]/data-type/index.tsx:
- Around line 170-183: Replace the EventTarget-to-Node assertions in
onDocMouseDown and onFocusOut with instanceof Node checks before calling
containerRef.current.contains. Preserve the existing behavior for targets inside
the container and for null relatedTarget values, while complying with the
no-type-assertions guideline.
🪄 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: b9198d5d-978d-4696-94b1-5090f35181ee
📒 Files selected for processing (14)
src/frontend/components/_features/[workspace]/data-type/index.tsxsrc/frontend/components/_molecules/data-types/structure/index.tsxsrc/frontend/store/__tests__/editor-slice.test.tssrc/frontend/store/__tests__/project-slice.test.tssrc/frontend/store/__tests__/shared-slice.test.tssrc/frontend/store/__tests__/shared-utils.test.tssrc/frontend/store/__tests__/tabs-utils.test.tssrc/frontend/store/slices/editor/slice.tssrc/frontend/store/slices/editor/types.tssrc/frontend/store/slices/project/slice.tssrc/frontend/store/slices/project/types.tssrc/frontend/store/slices/shared/slice.tssrc/frontend/store/slices/shared/utils.tssrc/frontend/store/slices/tabs/utils.ts
…able .dt files off taken names Clicking away from the code view raises document `mousedown` and then container `focusout`. The commit is synchronous, so `isParsingRef` is already clear by the second event, and a failure left `lastParsedCodeRef` un-advanced — so the same invalid buffer was parsed twice and toasted twice. Track the rejected buffer as well, so the pair is one attempt whatever its outcome. The variables editor never hit this because its commit is async and its latch spans both events. An unreadable `datatypes/<Name>.dt` took its name from the file basename with nothing checking it against the elements already loaded. POUs and data types share one identifier namespace and the file registry is keyed by raw name, so a project holding both a POU `foo` and an unreadable `foo.dt` had that POU's registry entry retyped to `data-type` — enough to send its next save down the `.dt` branch. Skip colliding names; the file still rides along in `unparsedDataTypeFiles`, so it is echoed back to disk rather than dropped. Comments across the feature trimmed to the non-obvious constraints. Refs DOPE-535 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvGrhB1LyJz9MBwYhjoBDY
Gustavohsdp
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the variables-style table/Monaco toggle to every data type tab, with the code side showing that type's
.dttext. Part of DOPE-385 (per-typedatatypes/<Name>.dtfiles).Mirror of https://github.com/Autonomy-Logic/openplc-web/pull/654
What changed
Model —
StructureTableTypebecomes the same discriminated unionVariablesTableuses:{ display: 'table', description, selectedRow } | { display: 'code', code? }.updateModelStructuregrowsdisplay/codeand gainsupdateModelStructureForName; every open data type is mounted at once (workspace-screen.tsx), so a background editor writing through the active-editor action would land on the wrong model.displayis optional on purpose — a row selection must never flip the view.Both model factories now seed
display: 'table', andshared/utils.tswas corrected fromselectedRow: ''to'-1'(the inconsistency the card called out: consumers doparseInt(selectedRow) === -1, andparseInt('')isNaN, so the disable-guards on the remove/move buttons never fired until the first row click).Toggle + buffer lifecycle — owned by
DataTypeEditor, which reads its own model (editor.meta.name === dataTypeName ? editor : editors.find(...)), never the active one. Table mode keeps the buffer serialized from the form so the switch is instant; code mode mirrors per keystroke into the model; commit fires on outside-mousedown and container focusout. Commit runsparseDataTypeFromText(buffer, tabName)— failure shows an inline error plus a toast and keeps the user in code mode (trapped-until-valid, variables precedent); success captures history, replaces the type, and flags the file dirty. Renaming inside the text stays rejected by the parser's existing name-match rule, whose message points at the project tree.External-mutation guards —
reconcileDatatypeText/regenerateDatatypeTextin the project slice.datatypeActions.renamereconciles first, so a tree rename folds pending text edits in rather than regenerating over them, and refuses when the text is invalid instead of silently discarding the user's work; afterwards it regenerates so the buffer'sTYPEline follows the new name.applyDatatypeSnapshotregenerates too, which is what makes undo, redo and a source-control revert visible in Monaco.Unreadable
.dtfiles — DOPE-533 preserved them raw but only logged a console warning, so a broken declaration was safe on disk and completely unreachable in the UI (raised by @Gustavo on #651). There is noPLCDataTypefor one, hence no tree leaf. Project open now registers a file entry and pre-opens a code-mode tab holding the raw bytes, without stealing focus from the auto-opened POU; the banner shows the live parse error, and committing valid text promotes it to a real type and drops it fromunparsedDataTypeFiles. No toast — the store layer can't import components (validate:arch), so the existing console warning stands.Flag
Everything user-visible is behind
isDataTypeFilesEnabled(), which still shipsfalse. DOPE-542 owns the flip. Validated locally with the constant flipped totrue.Verification
tsc -p tsconfig.app.jsonclean; editortscclean too — it type-checks test files and caught an assertion web's vitest does not check.compare-surfaces.py→match: true, diffs: 0..dtpre-opening in code mode.Follow-up
DOPE-545 — allow renaming from the code view. Deliberately out of scope here: the buffer is the user's authored text, so a refused rename has nowhere safe to land (overwrite destroys the edit; leaving it means every later commit re-fails the same check, stranding the tab). Doing it properly needs DOPE-536's propagation plus a cancel path that rewrites only the
TYPEname line.Summary by CodeRabbit
New Features
.dtfiles.Bug Fixes