fix: improve debug forcing for coils, contacts, and variables#608
Conversation
Use immediate forced values from debugForcedVariables instead of polled values for coils and contacts, eliminating the 200ms delay. Respect negated variant when displaying forced state colors. Fix composite key generation in variable element to support function block instance context. Add debug logging for unresolved variable indexes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughReplaces inline composite-key construction with a new hook ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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.
🧹 Nitpick comments (2)
src/renderer/screens/workspace-screen.tsx (1)
288-292: Diagnostic warnings may be noisy during normal operation.These
console.warncalls fire for every unresolvable variable during the setup phase. IfdebugVariableIndexesis populated asynchronously or incrementally, these warnings will fire on initial runs before indexes are fully available, potentially flooding the dev console. Consider gating them behind a debug/verbose flag, or batching them into a single summary warning.Also applies to: 440-444
src/renderer/components/_atoms/graphical-editor/ladder/contact.tsx (1)
108-122:getCompositeKeyis duplicated verbatim incontact.tsx,coil.tsx, andvariable.tsx.Consider extracting this helper into a shared utility (e.g.,
src/utils/graphical-editor/composite-key.ts) to eliminate the triple copy-paste. All three copies accept the same store values and perform identical logic.
There was a problem hiding this comment.
Pull request overview
This pull request improves the debug forcing functionality for ladder diagram elements (coils, contacts, and variables) by addressing three key issues: immediate visual feedback when forcing variables, correct color display for negated variants, and proper composite key generation for variables inside function block instances.
Changes:
- Added diagnostic
console.warnlogs when debug variable index resolution fails to help troubleshoot debugging issues - Fixed composite key generation for ladder variables inside function block instances to properly resolve the full FB instance path
- Fixed forced variable display to use immediate values from
debugForcedVariablesinstead of waiting for the 200ms polling delay - Fixed negated coil/contact color display to correctly calculate
displayStatebefore determining debug colors
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/renderer/screens/workspace-screen.tsx | Added console.warn diagnostics for failed variable index lookups in nested variables, program variables, and force operations |
| src/renderer/components/_atoms/graphical-editor/ladder/variable.tsx | Added getCompositeKey helper to properly resolve FB instance paths, updated all composite key usages to support FB instances |
| src/renderer/components/_atoms/graphical-editor/ladder/contact.tsx | Refactored debug color logic to use immediate forced values and apply negated variant logic before color selection |
| src/renderer/components/_atoms/graphical-editor/ladder/coil.tsx | Refactored debug color logic to use immediate forced values and apply negated variant logic before color selection |
| package-lock.json | Removed unused bcryptjs dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Consolidate 6 duplicated getCompositeKey implementations (4 plain functions in atom components, 2 useCallback-wrapped in molecule components) into a single shared hook with proper memoization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/_molecules/graphical-editor/fbd/index.tsx (1)
217-225:⚠️ Potential issue | 🟠 MajorMissing
getCompositeKeyinstyledEdgesdependency array — stale edges when FB instance changes.
getNodeOutputStatecallsgetCompositeKeyinternally, butgetCompositeKeyis not listed in theuseMemodependencies. If the user switches the selected FB debug instance,getCompositeKeywill return different keys, butstyledEdgeswon't recompute.Note that
rung/body.tsx(Line 237) correctly includesgetCompositeKeyin its equivalentstyledEdgesdependency array.Proposed fix
], [ rungLocal.edges, rungLocal.nodes, isDebuggerVisible, debugVariableValues, debugForcedVariables, editor.meta.name, project, + getCompositeKey, ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_molecules/graphical-editor/fbd/index.tsx` around lines 217 - 225, The styledEdges useMemo does not include getCompositeKey in its dependency array, causing stale edge styling when the FB debug instance changes; update the dependency list for the useMemo that computes styledEdges to include getCompositeKey (the same way rung/body.tsx does) so that getNodeOutputState (which calls getCompositeKey) triggers recomputation when the composite key logic changes or the FB instance switches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/renderer/components/_molecules/graphical-editor/fbd/index.tsx`:
- Around line 217-225: The styledEdges useMemo does not include getCompositeKey
in its dependency array, causing stale edge styling when the FB debug instance
changes; update the dependency list for the useMemo that computes styledEdges to
include getCompositeKey (the same way rung/body.tsx does) so that
getNodeOutputState (which calls getCompositeKey) triggers recomputation when the
composite key logic changes or the FB instance switches.
Web editor exports project.json without data.pous (POUs are stored as separate files), causing Zod validation to fail and the entire project data to be replaced with defaults. This lost the project name and introduced null for debugVariables.pous (ZodRecord was unhandled). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The hybrid POU serializer (Python/C++) was not appending the END keyword (END_FUNCTION_BLOCK, END_PROGRAM, END_FUNCTION) to saved files, breaking compatibility with openplc-web which correctly includes it. The parser also did not strip the END keyword on load, causing it to appear in the code editor when opening projects saved by openplc-web. - Serializer: append END keyword for hybrid POUs, matching textual/graphical - Parser: strip trailing END keyword from body, matching textual/graphical Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/PLC/pou-text-parser.ts (1)
244-255: ReusegetEndKeywordutility instead of duplicating the keyword map.
getEndKeywordfrompou-file-extensions.tsalready maps POU types to theirEND_*keywords and is imported in the sibling serializer file. The sameendKeywordsmap is also duplicated inparseTextualPouFromString(line 118) andparseGraphicalPouFromString(line 367) within this file.Regarding the static analysis ReDoS warning on line 253: this is a false positive since
endKeywordis always one of three hardcoded constant strings (END_PROGRAM,END_FUNCTION,END_FUNCTION_BLOCK).♻️ Suggested refactor
- // Strip the trailing END keyword from the body content, matching how textual/graphical parsers handle it - const endKeywords: Record<string, string> = { - program: 'END_PROGRAM', - function: 'END_FUNCTION', - 'function-block': 'END_FUNCTION_BLOCK', - } - const endKeyword = endKeywords[type] - let bodyContent = remainingContent.slice(bodyStartIndex).trim() - if (endKeyword) { - const endKeywordRegex = new RegExp(`\\s*\\b${endKeyword}\\b\\s*$`, 'i') - bodyContent = bodyContent.replace(endKeywordRegex, '').trim() - } + // Strip the trailing END keyword from the body content, matching how textual/graphical parsers handle it + const endKeyword = getEndKeyword(type) + let bodyContent = remainingContent.slice(bodyStartIndex).trim() + const endKeywordRegex = new RegExp(`\\s*\\b${endKeyword}\\b\\s*$`, 'i') + bodyContent = bodyContent.replace(endKeywordRegex, '').trim()
getEndKeywordis already imported in the serializer; add it to the parser's import:-import { getLanguageFromExtension } from './pou-file-extensions' +import { getEndKeyword, getLanguageFromExtension } from './pou-file-extensions'The same consolidation could be applied to
parseTextualPouFromStringandparseGraphicalPouFromStringto eliminate all three duplicated maps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/PLC/pou-text-parser.ts` around lines 244 - 255, The duplicated endKeywords map should be replaced by the shared getEndKeyword utility: import and call getEndKeyword(type) instead of defining the endKeywords object in pou-text-parser.ts, and remove the duplicated maps in parseTextualPouFromString and parseGraphicalPouFromString as well; update the code that computes endKeyword (currently in the block around bodyContent/bodyStartIndex) to use getEndKeyword(type) and keep the existing regex trimming logic (the ReDoS warning is a false positive since the value is one of the known constants).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/PLC/pou-text-parser.ts`:
- Around line 244-255: The duplicated endKeywords map should be replaced by the
shared getEndKeyword utility: import and call getEndKeyword(type) instead of
defining the endKeywords object in pou-text-parser.ts, and remove the duplicated
maps in parseTextualPouFromString and parseGraphicalPouFromString as well;
update the code that computes endKeyword (currently in the block around
bodyContent/bodyStartIndex) to use getEndKeyword(type) and keep the existing
regex trimming logic (the ReDoS warning is a false positive since the value is
one of the known constants).
Summary
negatedvariant logicprogramName:fbVariableName.variableName)console.warnlogs when debug variable index resolution failsResolves
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores