feat(datatypes): datatype rename reference propagation with impact modal (DOPE-536) - #994
Conversation
…act modal (DOPE-536) Renaming a referenced data type previously only renamed the type itself, leaving every referencing variable and type pointing at a dead name and breaking compilation. datatypeActions.rename is now async: it finds all references (POU variables, globals, struct fields, array base types), awaits a confirmation modal when any exist, and rewrites them before the rename. Cancel leaves the store untouched. Code-mode variable buffers are regenerated after propagation because sanitizePou persists editor.variable.code as the authoritative variables block on save, and every touched container file is flagged dirty so single-file save and the close-project check pick up the propagated content. The array display value is rebuilt too — variable serialization emits type.value verbatim into the saved declaration. Mirror of the openplc-web change (shared frontend surface). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Br23N5eg2d4ASekgSmTFzz
WalkthroughData-type renames now detect affected references and request confirmation. Confirmed renames update project references and editor state. Rename controls await completion, while the application layout hosts a configurable impact modal. ChangesData-type rename workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RenameEditor
participant SharedSlice
participant ImpactModal
participant ProjectActions
RenameEditor->>SharedSlice: request asynchronous rename
SharedSlice->>ImpactModal: show reference impact
ImpactModal->>SharedSlice: confirm or cancel
SharedSlice->>ProjectActions: propagate confirmed name
ProjectActions-->>SharedSlice: update references
SharedSlice-->>RenameEditor: resolve rename result
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![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
🤖 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 56-66: The asynchronous datatypeActions.rename calls must
explicitly handle rejected promises. In
src/frontend/components/_features/[workspace]/data-type/index.tsx:56-66, add a
catch that restores editorContent.name to dataTypeName, shows the failure toast,
and calls setIsEditing(false); in
src/frontend/components/_molecules/project-tree/index.tsx:597-600, add a catch
that restores the previous label via setNewLabel(label || '').
In `@src/frontend/store/slices/shared/slice.ts`:
- Around line 372-379: Guard the pendingDatatypeRename assignment in the rename
flow so a second request cannot overwrite the existing resolver. When a rename
is already pending, explicitly reject or resolve the new request with the
existing cancellation/error result, while preserving the first request’s await
and normal propagation behavior.
🪄 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: 96036f3b-cbd4-4c62-a701-1b8eaab553e1
📒 Files selected for processing (15)
src/frontend/components/_features/[workspace]/data-type/index.tsxsrc/frontend/components/_molecules/project-tree/index.tsxsrc/frontend/components/_molecules/rename-impact-modal/data-type-rename-impact-modal.tsxsrc/frontend/components/_molecules/rename-impact-modal/index.tsxsrc/frontend/components/_templates/app-layout.tsxsrc/frontend/store/__tests__/project-slice.test.tssrc/frontend/store/__tests__/shared-slice.test.tssrc/frontend/store/slices/project/slice.tssrc/frontend/store/slices/project/types.tssrc/frontend/store/slices/shared/slice.tssrc/frontend/store/slices/shared/types.tssrc/frontend/utils/__tests__/data-type-references.test.tssrc/frontend/utils/data-type-references.tssrc/frontend/utils/data-type-references/types.tssrc/frontend/utils/variable-references/types.ts
…36-datatype-rename-propagation
…ejections Review findings from CodeRabbit on this PR: - A second rename arriving while one awaits the impact modal used to overwrite pendingDatatypeRename, dropping the first resolver and stranding its await forever (Enter + blur can double-fire). The second request is now rejected explicitly. - Both rename callers consumed the promise with .then only; an unexpected rejection would skip the recovery path and leave the name input stuck in edit mode. Both now catch, restore the previous name, and surface the error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Br23N5eg2d4ASekgSmTFzz
c33004f to
29c8159
Compare
Review — implementation is solid; the blocker is process, not codeReviewed together with the web counterpart Autonomy-Logic/openplc-web#653 — all 15 shared files are byte-identical between the repos, so everything below applies to both; that PR additionally adapts the AI tool executor, which this repo does not have. The finder covers the four structured places a data type is referenced — POU variables, global variables, other structures' fields, other arrays' base types — in both shapes (direct I probed the rewriter at the points where this kind of code usually breaks: The How I verified: 15/15 shared files byte-identical ( Two findings from the sibling PRs are fixed hereWorth stating explicitly, because it changes the advice on those PRs:
🟡 1 — CI hasn't run on either PR (this is what blocks merge)For contrast, #993 reports 12 jobs — so this isn't repo configuration, it's this branch not triggering any workflow. Why it matters. 🟡 2 — The array display-string rebuild normalises formattingMeasured: an array that came through the XML importer holds Why I'm flagging it. The rebuild itself is necessary and the comment explains why correctly — Impact. For projects that came through the importer, renaming a data type rewrites the formatting of those declarations as well as the name — byte churn beyond the rename, showing up as a larger Source Control diff than the user's action explains. In a codebase that invests as much as this one in byte-stability (DOPE-477, "persist the raw object", the byte-identical mirror gates), that deserves to be a conscious call rather than a surprise. Preserving the original spacing when the current 🟡 3 — A fifth copy of the type name lives in the graphical nodes (question, not a claim)The finder covers the four structured sites. There's one more place a type name is cached: The question: is that cache re-derived from the POU on relink — in which case it self-heals after a rename — or persisted in the flow? If it's persisted, the scenario is: a user FB with a I'm not claiming it's broken — I didn't go deep enough to conclude. But it's the only reference site left outside the finder, and whoever knows the relink path can answer it in minutes. 🟢 Smaller
What's good
VerdictHold only for CI. Once the workflows run and pass, I'd approve: 🟡 2 is a decision to record, 🟡 3 is a minutes-long question for whoever knows the relink path, and the 🟢 is a string. One ordering note: since the case-insensitive guard lives in Review assisted by Claude Code. |
|
Thanks for the deep review, @Gustavohsdp — point-by-point disposition: 🟡 1 — CIRoot cause was the GitHub Actions major outage on Aug 6 (githubstatus.com had Actions at 🟡 2 — Array display-string normalization: accepted, recorded hereDecision: we keep the rebuild (serializer format) and accept the formatting normalization for XML-imported arrays. Rationale for the byte-stability concern: 🟡 3 — The fifth copy: your suspicion is right, and it does NOT self-heal → DOPE-548Verified: block signatures are copied into Important scoping fact: this is a pre-existing, general hole — editing an FB pin's type from INT to REAL today breaks placed blocks the exact same way; the rename is just one more path into it. Filed DOPE-548 for the principled fix (re-derive user-FB variants on relink, the counterpart of what restamp does for libraries), which heals every entry into the hole rather than special-casing rename inside this PR. 🟢 Case-only rename messageKeeping the current message for now — decided it's not worth a change in this PR. 🟢 'Resource' magic-string testAlready covered: Ordering noteMoot, happily: the Reply assisted by Claude Code. |
…36-datatype-rename-propagation
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/frontend/store/slices/shared/slice.ts (1)
380-405: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBlock every data-type rename while confirmation is pending.
Line 396 rejects a second request only when that request also has references. If
OldDT → NewDTawaits confirmation andOtherDT → NewDThas zero references, the second request skips this guard and completes. The confirmed request then operates with a target name that is no longer available.Check
pendingDatatypeRenamebeforereconcileDatatypeTextand reference analysis. This also prevents a rejected request from reconciling unrelated editor state. Add a test for a zero-reference rename that targets a pending rename’snewName.Proposed fix
const nameCheck = validateElementName(newName) if (!nameCheck.ok) return nameCheck +if (getState().pendingDatatypeRename) { + return { ok: false, message: 'Another data type rename is awaiting confirmation' } +} + const reconcile = state.projectActions.reconcileDatatypeText(oldName) if (!reconcile.ok) return { ok: false, message: reconcile.message } // ... - if (getState().pendingDatatypeRename) { - return { ok: false, message: 'Another data type rename is awaiting confirmation' } - }🤖 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/store/slices/shared/slice.ts` around lines 380 - 405, Update the rename flow around the datatype rename action to check pendingDatatypeRename before calling reconcileDatatypeText or findAllReferencesToDataType, rejecting any concurrent rename while confirmation is pending. Remove the later conditional-only guard, preserve the existing rejection message, and add coverage for a zero-reference rename targeting the pending rename’s newName.src/frontend/components/_features/[workspace]/data-type/index.tsx (2)
288-314: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse buttons for the visualization controls.
TableIconandCodeIconare SVG click targets. Keyboard users cannot activate them or determine the selected visualization. Wrap each icon in a<button type="button">and setaria-pressedfromdisplay.Proposed fix
- <TableIcon - aria-label='Data type table visualization' - onClick={() => handleVisualizationTypeChange('table')} - size='md' - currentVisible={display === 'table'} - className={cn( - display === 'table' ? 'fill-brand' : 'fill-neutral-100 dark:fill-neutral-900', - 'rounded-l-md transition-colors ease-in-out hover:cursor-pointer', - )} - /> + <button + type='button' + aria-label='Data type table visualization' + aria-pressed={display === 'table'} + onClick={() => handleVisualizationTypeChange('table')} + className='rounded-l-md focus-visible:outline focus-visible:outline-2 focus-visible:outline-brand' + > + <TableIcon + aria-hidden + size='md' + currentVisible={display === 'table'} + className={display === 'table' ? 'fill-brand' : 'fill-neutral-100 dark:fill-neutral-900'} + /> + </button>🤖 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 288 - 314, Update the visualization controls around TableIcon and CodeIcon to use type="button" elements, moving each icon’s click handler onto its button. Set aria-pressed based on whether display matches the corresponding visualization, while preserving the existing styling, labels, and handleVisualizationTypeChange behavior.Source: Learnings
175-186: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the Node type assertions.
Lines 177 and 184 use
as Node; narrowe.targetande.relatedTargetwithinstanceof Nodebefore passing them tocontains.Proposed fix
- if (containerRef.current.contains(e.target as Node)) return + const target = e.target + if (target instanceof Node && containerRef.current.contains(target)) 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🤖 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 175 - 186, Update onDocMouseDown and onFocusOut to remove the Node type assertions; first narrow e.target and e.relatedTarget with instanceof Node, then call containerRef.current.contains only for confirmed Node values while preserving the existing commit behavior.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/store/slices/shared/slice.ts`:
- Around line 385-392: Update the data-type rename validation around
findAllReferencesToDataType to check state.project.data.pous with nameMatches
before reconciliation or confirmation. Reject targets matching any POU name in
either exact-case or case-insensitive form, preserving the existing data-type
and unparsed-file checks. Add tests covering both exact-case and case-variant
POU name collisions.
---
Outside diff comments:
In `@src/frontend/components/_features/`[workspace]/data-type/index.tsx:
- Around line 288-314: Update the visualization controls around TableIcon and
CodeIcon to use type="button" elements, moving each icon’s click handler onto
its button. Set aria-pressed based on whether display matches the corresponding
visualization, while preserving the existing styling, labels, and
handleVisualizationTypeChange behavior.
- Around line 175-186: Update onDocMouseDown and onFocusOut to remove the Node
type assertions; first narrow e.target and e.relatedTarget with instanceof Node,
then call containerRef.current.contains only for confirmed Node values while
preserving the existing commit behavior.
In `@src/frontend/store/slices/shared/slice.ts`:
- Around line 380-405: Update the rename flow around the datatype rename action
to check pendingDatatypeRename before calling reconcileDatatypeText or
findAllReferencesToDataType, rejecting any concurrent rename while confirmation
is pending. Remove the later conditional-only guard, preserve the existing
rejection message, and add coverage for a zero-reference rename targeting the
pending rename’s newName.
🪄 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: 5ddcfee5-3b8a-4f09-9352-5e08708c8c05
📒 Files selected for processing (7)
src/frontend/components/_features/[workspace]/data-type/index.tsxsrc/frontend/components/_molecules/project-tree/index.tsxsrc/frontend/store/__tests__/project-slice.test.tssrc/frontend/store/__tests__/shared-slice.test.tssrc/frontend/store/slices/project/slice.tssrc/frontend/store/slices/project/types.tssrc/frontend/store/slices/shared/slice.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/frontend/components/_molecules/project-tree/index.tsx
- src/frontend/store/slices/project/types.ts
- src/frontend/store/tests/project-slice.test.ts
- src/frontend/store/tests/shared-slice.test.ts
- src/frontend/store/slices/project/slice.ts
2 participants
DOPE-536 — Datatype rename reference propagation (impact modal)
Renaming a data type now propagates the new name into everything that references it, behind a confirmation modal — matching what variable renames already do. Previously the rename only changed the type's own name, leaving every referencing variable pointing at a dead name and breaking compilation.
What changed
datatypeActions.renameis now async: after the existing guards (case-insensitive uniqueness, unreadable-.dtcollision, name validity) it finds all references and, when any exist, parks apendingDatatypeRenamein the shared slice and awaits the impact modal. Confirm → propagate then rename. Cancel → zero state change, result{ok: false, cancelled: true}(callers skip the failure toast). Zero references or same-name rename skips the modal entirely.src/frontend/utils/data-type-references.ts(+data-type-references/types.ts):findAllReferencesToDataTypecovers POU variables, global variables, other structures' fields, and array data types' base types — both directuser-data-typereferences andarraybase types. Mirrorsvariable-referencesand returns the sameReferenceImpactAnalysisshape so the existing modal renders it unchanged.projectActions.propagateDatatypeRename: rewrites all four reference shapes in one Immer pass, rebuilding theARRAY [dims] OF <type>display string — required for correctness since variable serialization emitstype.valueverbatim into saved declarations.sanitizePoupersistseditor.variable.codeas the authoritativevariablesTexton save, so a stale buffer would resurrect the old type name.RenameImpactModalgains optional copy-override props (title, list labels, button labels, cancel description); defaults keep the variable-rename wording byte-for-byte. New store-drivenDataTypeRenameImpactModalhost mounted in app-layout.ReferenceImpactAnalysismade generic (<Location = VariableReferenceLocation>) — existing variable flows compile unchanged.executeTool) has no counterpart here — the editor ships no AI adapter.Deliberately not done
isDataTypeFilesEnabled()gating — this is a correctness fix independent of the.dtpersistence switch.Validation
store/slices+utilsgates green in both repos (jest + vitest).compare-surfaces.py: 987 files, 0 diffs).Mirror of https://github.com/Autonomy-Logic/openplc-web/pull/653
🤖 Generated with Claude Code
https://claude.ai/code/session_01Br23N5eg2d4ASekgSmTFzz
Summary by CodeRabbit
New Features
Bug Fixes