[JoaoGSP]

Fixes DOPE-616.

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

Problem

Declaring an already-taken variable name in the variables code view and switching back to the table was accepted: two rows with the same name, both written on save, failing only at compile time (strucpp: Symbol 'MOTOR' already defined in scope 'MAIN'). The table's own create/update refuse the same duplicate. Pre-existing on main — the text path never validated names.

Fix

Validate the parsed list in the store, where every text path converges, instead of in each editor:

• findDuplicateVariableName — first name declared twice, case-insensitive (IEC identifiers are case-insensitive, same rule as checkIfVariableExists).
• setPouVariables, setGlobalVariables (code view → table for POU variables and resource globals) and reconcileVariablesText (row edit / Ctrl+S while the code view is open) return fail('"<name>" is declared more than once…', 'Variable already exists') before touching state.
• commitCode and the globals editor already turn a non-ok response into the "Syntax error" toast and keep the code view open, so no component change.

Tests

project-slice.test.ts: duplicate and case-only duplicate refused by setPouVariables (table untouched), duplicate refused by setGlobalVariables, reconcile path refused via createVariable on a code-mode buffer with Motor/motor.

Shared surface: src/frontend/** only, byte-identical with the sibling PR.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Code-mode variable editing now rejects duplicate names, including names that differ only by letter case, and displays a clear error message.
  • Variable lists applied through undo, redo, or restore now preserve their provided contents, including duplicate names.
• Tests
  • Added coverage for duplicate detection and variable-list handling across global, POU, and code-mode editing workflows.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 8f3dbe26-3c39-4374-abe2-9855d2bc9854

📥 Commits

Reviewing files that changed from the base of the PR and between db395df and 76d1aca.

📒 Files selected for processing (6)

• src/frontend/components/_organisms/global-variables-editor/index.tsx
• src/frontend/components/_organisms/variables-editor/index.tsx
• src/frontend/store/__tests__/project-slice.test.ts
• src/frontend/store/slices/project/slice.ts
• src/frontend/utils/__tests__/generate-iec-string-to-variables.test.ts
• src/frontend/utils/generate-iec-string-to-variables.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

Walkthrough

Shared utilities now detect case-insensitive duplicate variable names. Code-mode reconciliation and both variable editors reject duplicates. Direct POU and global list replacement accepts incoming lists as-is. Tests cover both behaviors.

Changes

Duplicate variable validation

Layer / File(s)	Summary
Shared duplicate detection src/frontend/utils/generate-iec-string-to-variables.ts, src/frontend/utils/__tests__/generate-iec-string-to-variables.test.ts	Shared utilities detect case-insensitive duplicate names and format the duplicate-name error message. Tests cover unique, empty, same-case, and case-only duplicates.
Store reconciliation behavior src/frontend/store/slices/project/slice.ts, src/frontend/store/__tests__/project-slice.test.ts	Code-mode reconciliation rejects duplicate names without changing existing state. POU and global list replacement now accepts duplicate names as-is.
Editor commit validation src/frontend/components/_organisms/variables-editor/index.tsx, src/frontend/components/_organisms/global-variables-editor/index.tsx	Both editors reject duplicate names after parsing and show the existing parse-error feedback before committing changes.

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

Merge Risk: ⚪ Minimal · up to 76d1a

Code-view commits now reject duplicate variable names case-insensitively without changing stored variables, while restore and reclassification paths can retain existing duplicate-containing projects. The change is ready to merge.

Suggested reviewers: thiagoralves

Poem

A rabbit found two names alike
One wore caps, one liked the night
Code mode paused with a clear reply
Direct lists passed by
Shared helpers kept the rules precise

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely identifies the main change: refusing duplicate variable names entered through the variables code view. The issue reference is relevant.
Description check	✅ Passed	The description provides the issue reference, problem statement, implementation details, affected paths, error behavior, scope, and test coverage. It does not reproduce the DOD checklist, but the subs…
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 6…
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.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch bugfix/DOPE-616-code-view-duplicate-variable-names

---

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.}

[Gustavohsdp]

Code Review — DOPE-616

Verdict: request changes — one placement issue, everything else is clean.

Small, well-targeted fix. Validating the parsed list in the store, where every text path converges, is the right instinct — better than patching each editor. Both files are byte-identical with the mirror, CI is fully green including all four sync jobs, and I mutation-tested the guard: neutralising findDuplicateVariableName's hit turns exactly the 4 new tests red (326 pass otherwise), so they are load-bearing.

The problem is where the guard sits, not what it does.

---

🔴 The guard was added to generic setters, but four of six call sites ignore the failure

setPouVariables and setGlobalVariables can now return fail(...). Six call sites reach them; only the two code-view editors check the response:

Call site	Checks result?	What happens on a duplicate
variables-editor/index.tsx:906	✅ const response = ...	toast, code view stays open
global-variables-editor/index.tsx:339	✅ const response = ...	toast, code view stays open
save-actions.ts:1084	❌ ignored	reclassification silently skipped on save
shared/slice.ts:1394	❌ ignored	reclassification silently skipped on project open
shared/slice.ts:1768	❌ ignored	undo silently doesn't restore globals
shared/slice.ts:1843	❌ ignored	redo silently doesn't restore globals

Note what the input to those four is: not user-typed text, but already-stored state round-tripped through generateIecVariablesToString → parseIecStringToVariables, or a history snapshot. They can therefore only fail on a project that already contains duplicates — which is precisely the population this bug created and that this PR exists to help. Those projects are out there.

The undo/redo pair is the sharp one. applyPouSnapshot restores the POU's variables and body, and applyLadderFlowSnapshot restores the flow — but the setGlobalVariables call between them silently no-ops. The result is a partially applied undo: body and POU variables revert, globals don't, and nothing tells the user. On the reclassify paths the effect is milder (the write is skipped, existing variables stay), but it is equally silent — there is no toast, no console.warn, nothing to diagnose from.

Your own doc comment states the intent exactly: "Text paths (code view commit, reconcile) skip the table's per-row validation, so the list is checked whole." Agreed — but setPouVariables/setGlobalVariables are not text paths; they are generic setters that internal restore paths also use.

Suggested: keep the guard on the text boundary only — reconcileVariablesText (already correct) plus the two code-view commits — and leave the generic setters unconditional. If you would rather keep it in the setters, then give the internal restores an explicit opt-out, and at minimum log a warning at the four ignoring sites so the condition is diagnosable when a user reports "my undo didn't fully undo".

---

🟢 Minor

• nameMatches sits three lines above ("the rule every element-name lookup folds by"), and findDuplicateVariableName now folds case inline with its own .toLowerCase(). The Set scan is the right O(n) choice — but extracting a shared foldName(name) used by both would stop the two from drifting apart later.

✅ What is solid

• Right diagnosis, right layer. Validating once where the text paths converge, rather than in each editor, is what keeps the third text path from reintroducing this.
• Guard placed before setState in all three actions — no partial write on rejection.
• Case-insensitive, consistent with checkIfVariableExists and with IEC identifier rules; the case-only duplicate is covered by its own test.
• Tests are load-bearing and specific: duplicate and case-only duplicate on setPouVariables (asserting the table is untouched, not merely that it returned ok: false), duplicate on setGlobalVariables, and the reconcile path exercised through createVariable on a code-mode buffer. Neutralising the guard turns exactly those four red.
• Byte-identical with the mirror PR; no change needed in commitCode or the globals editor, since both already route a non-ok response to the existing "Syntax error" toast.
• No conflict with DOPE-599 (fix(variables): compare variable names case-insensitively everywhere (DOPE-599) #1083/refactor(ai): sync shared surfaces for AI chat enhancements #732), still open — that one touches validation/variables.ts, this one project/slice.ts, and the two case-folding rules agree.

---

To approve: move the guard off the generic setters, or make the four internal call sites explicit about it. Everything else here is ready.

[JoaoGSP]

Thanks — the call-site table is exactly right, and the undo/redo case is the one that mattered: a project that already carries a duplicate would have come back from an undo with its POU variables and body restored but its globals silently left behind. Took your first option.

Guard moved to the text boundary, setters unconditional again:

• setPouVariables / setGlobalVariables are back to plain replace, so undo/redo and the reclassify-on-open/save paths behave as before regardless of what the stored project contains. The store test now states that contract explicitly (replaces the list as-is, even with a duplicate name: undo/redo and reclassification restore through here).
• reconcileVariablesText keeps its check — it is the third text path.
• variables-editor commitCode and global-variables-editor commitCode check the parsed list right after parseIecStringToVariables and throw Variable already exists: "<name>" is declared more than once…, which lands in the existing catch: "Syntax error" toast, setParseError, code view stays open with the text intact.

On the minor: the helper now lives next to the parser (findDuplicateVariableName + duplicateVariableNameMessage in generate-iec-string-to-variables.ts, with their own tests), so the slice no longer has two case-folding rules side by side — nameMatches stays the one rule in slice.ts. Didn't add a shared foldName across modules; both sides are a single .toLowerCase() and I'd rather not introduce a util for that until a third caller appears.

Both files byte-identical with the mirror; store + utils suites green locally in both repos.