feat: viewport-aware debugger polling for all editor languages#686
Closed
thiagoralves wants to merge 1 commit into
Closed
feat: viewport-aware debugger polling for all editor languages#686thiagoralves wants to merge 1 commit into
thiagoralves wants to merge 1 commit into
Conversation
Replace the ~280-line LD/FBD/catch-all polling sections in workspace-screen with a unified push-based architecture. Each visual component (LD, FBD, Monaco ST/IL) computes its own visible variable set debounced to ~1s and pushes it to the store. The polling tick simply reads the pre-computed set. - LD: detects visible rungs via scroll position, extracts variable names - FBD: filters nodes by viewport bounds using ReactFlow getViewport() - ST/IL: scans visible editor lines for variable references - Extract stripLineComments and collectSTVariableNames as shared utilities - Add debugViewportVarNames store field with cleanup on debug hide - Hoist fbInstanceCtx computation to avoid duplication across languages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
WalkthroughThis PR introduces a viewport-driven debugging system where editors (Ladder, Monaco ST/IL, FBD) automatically collect and report visible variable names via a shared store. The workspace polling logic then uses these viewport-collected variable names to optimize which debugger variables to poll, replacing extensive conditional branches with a centralized, viewport-aware approach. Changes
Sequence DiagramsequenceDiagram
actor User
participant Editor as Ladder/Monaco/FBD Editor
participant Viewport as Viewport Scanner<br/>(debounced)
participant Store as Workspace Store
participant Polling as Workspace Polling<br/>Logic
User->>Editor: Scroll/Pan viewport
Editor->>Viewport: Trigger debounced scan (1s)
Viewport->>Viewport: Collect visible variable names
Viewport->>Store: setDebugViewportVarNames(visibleVars)
Store->>Store: Update debugViewportVarNames
Store->>Polling: Notify state change
Polling->>Polling: Compute polling targets from<br/>debugViewportVarNames
Polling->>Polling: Poll selected variables from debugger
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable poems in the walkthrough.Disable the |
![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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/renderer/components/_features/`[workspace]/editor/graphical/ladder/index.tsx:
- Around line 257-260: The code only adds blockData.numericId for block nodes,
but the polling uses debugViewportVarNames to match
currentPou.data.variables[*].name, so you must also publish the block instance
name; update the block branch (where node.type === 'block' and blockData is
read) to also add the block instance identifier (e.g., blockData.variant?.type
or the instance/name field present on node.data) into the names set alongside
numericId so visible instances like TON0 are emitted and will match
currentPou.data.variables[*].name.
In `@src/renderer/components/_features/`[workspace]/editor/monaco/index.tsx:
- Around line 251-288: When there are no variables the effect currently returns
early and never clears the workspace-global debug viewport state; change the
early-return branch in the useEffect inside the Monaco editor (the block that
computes varNames from pou?.data.variables) to call setDebugViewportVarNames([])
before returning so the global store is reset; ensure this uses the same editor
effect (referenced symbols: pou?.data.variables, varNames, computeVisibleVars,
setDebugViewportVarNames) so switching POUs clears the previous viewport names.
In `@src/renderer/components/_molecules/graphical-editor/fbd/index.tsx`:
- Around line 724-730: The collector currently adds only blockData.numericId for
nodes of type 'block', which mismatches the visible variable names used
elsewhere (e.g., workspace-screen.tsx) and prevents enqueuing member names like
TON0.Q or PID0.OUT; change the 'block' branch in index.tsx to extract and add
the block's variable name (e.g., blockData.variable?.name or the field that
contains the human-facing block name) to varNames instead of—or in addition
to—numericId so the key-space matches the visible variable names used when
expanding FB members.
In `@src/renderer/screens/workspace-screen.tsx`:
- Around line 1213-1249: The viewport-poll expansion only handles derived-type
parents and misses function-output numericId markers and children of array/UDT
parents; update the logic that builds debugVariableKeys (around
debugViewportVarNames, makeCompositeKeyForCurrentPou, variableInfoMapRef,
currentPou, fbInstanceCtx) to also: 1) when a visible variable is a
function/block marker (numericId) call the same composite-key mapping used for
direct vars (via makeCompositeKeyForCurrentPou or the existing composite-key
convention) and add the resulting _TMP_* keys to debugVariableKeys; 2) treat
visible array/UDT parents similarly to derived by scanning
variableInfoMapRef.current entries for children whose varInfo.variable.name
startsWith the parent name (using the same pou-context checks with
fbInstanceCtx/currentPou.data.name) and add pouName:childName for each child so
array/UDT fields and struct members are included in polling.
In `@src/renderer/utils/debug/collect-st-variables.ts`:
- Around line 10-14: The regex in the variableNames.map block uses a negative
lookahead (/(?![\w.\[])/) that prevents matching parent symbols when they're
accessed via member or array notation (e.g., TON0.Q, cfg.limit, arr[1]); update
the RegExp construction in that block (the code that builds patterns from
variableNames/escaped) to remove the negative lookahead and instead rely on the
existing word-boundary (\\b) or a more permissive lookahead that still prevents
partial identifier matches but allows '.' and '[' after the identifier (for
example, use new RegExp(`\\b${escaped}`, 'i')). This will ensure
debugViewportVarNames can include parent symbols used only via member or array
access so downstream checks (e.g., debugViewportVarNames.has(v.name) in
workspace-screen.tsx) work correctly.
In `@src/renderer/utils/debug/strip-line-comments.ts`:
- Around line 5-43: stripLineComments fails to track string quotes so it treats
// inside single-quoted literals as comments; update the BlockCommentState union
to include 'single-quote' and modify stripLineComments (use the local state
variable s, the chars array and the scanning while loop) to detect entering a
single-quote when not already in a comment (on chars[i] === ''' ), set s =
'single-quote' and advance, and when s === 'single-quote' scan forward until the
closing quote (handle escaped single-quotes by treating doubled quotes '' as an
escaped quote and advancing past both) without blanking characters (unlike
block-comment states), then on closing quote set s = false and continue so //
inside strings won't be treated as line comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: da2353c0-6a7a-4e1f-a1e2-95b9e4956240
📒 Files selected for processing (8)
src/renderer/components/_features/[workspace]/editor/graphical/ladder/index.tsxsrc/renderer/components/_features/[workspace]/editor/monaco/index.tsxsrc/renderer/components/_molecules/graphical-editor/fbd/index.tsxsrc/renderer/screens/workspace-screen.tsxsrc/renderer/store/slices/workspace/slice.tssrc/renderer/store/slices/workspace/types.tssrc/renderer/utils/debug/collect-st-variables.tssrc/renderer/utils/debug/strip-line-comments.ts
Comment on lines
+257
to
+260
| if (node.type === 'block') { | ||
| const blockData = node.data as { variant?: { type: string }; numericId?: string } | ||
| if (blockData.numericId) names.add(blockData.numericId) | ||
| } |
Contributor
There was a problem hiding this comment.
Publish the block instance name here as well.
For block nodes this only emits numericId. The polling side expands visible FB members by matching debugViewportVarNames against currentPou.data.variables[*].name, so a visible instance like TON0 never matches and TON0.Q/ET/... stop updating unless they are watched separately.
Suggested fix
- if (node.type === 'block') {
- const blockData = node.data as { variant?: { type: string }; numericId?: string }
- if (blockData.numericId) names.add(blockData.numericId)
+ if (node.type === 'block') {
+ const blockData = node.data as {
+ variant?: { type?: string }
+ numericId?: string
+ variable?: { name?: string }
+ }
+ if (blockData.variable?.name) names.add(blockData.variable.name)
+ if (blockData.variant?.type === 'function' && blockData.numericId) names.add(blockData.numericId)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/renderer/components/_features/`[workspace]/editor/graphical/ladder/index.tsx
around lines 257 - 260, The code only adds blockData.numericId for block nodes,
but the polling uses debugViewportVarNames to match
currentPou.data.variables[*].name, so you must also publish the block instance
name; update the block branch (where node.type === 'block' and blockData is
read) to also add the block instance identifier (e.g., blockData.variant?.type
or the instance/name field present on node.data) into the names set alongside
numericId so visible instances like TON0 are emitted and will match
currentPou.data.variables[*].name.
Comment on lines
+251
to
+288
| useEffect(() => { | ||
| if (!isDebuggerVisible || !editorRef.current || (language !== 'st' && language !== 'il')) return | ||
|
|
||
| const editor = editorRef.current | ||
| const model = editor.getModel() | ||
| if (!model) return | ||
|
|
||
| const varNames = pou?.data.variables.map((v) => v.name).filter((n) => n && n.trim() !== '') || [] | ||
| if (varNames.length === 0) return | ||
|
|
||
| const computeVisibleVars = () => { | ||
| const ranges = editor.getVisibleRanges() | ||
| const visibleLines: string[] = [] | ||
| for (const range of ranges) { | ||
| for (let line = range.startLineNumber; line <= range.endLineNumber; line++) { | ||
| visibleLines.push(model.getLineContent(line)) | ||
| } | ||
| } | ||
| const sourceText = visibleLines.join('\n') | ||
| const visibleVarNames = collectSTVariableNames(sourceText, varNames) | ||
| setDebugViewportVarNames(visibleVarNames) | ||
| } | ||
|
|
||
| // Compute immediately | ||
| computeVisibleVars() | ||
|
|
||
| // Debounce on scroll | ||
| let timer: ReturnType<typeof setTimeout> | ||
| const disposable = editor.onDidScrollChange(() => { | ||
| clearTimeout(timer) | ||
| timer = setTimeout(computeVisibleVars, 1000) | ||
| }) | ||
|
|
||
| return () => { | ||
| disposable.dispose() | ||
| clearTimeout(timer) | ||
| } | ||
| }, [isDebuggerVisible, editorMounted, name, language, pou?.data.variables, setDebugViewportVarNames]) |
Contributor
There was a problem hiding this comment.
Reset the global viewport set when this editor has nothing to publish.
Line 258-259 returns early for POUs with no variables, and cleanup never clears the store. Since debugViewportVarNames is workspace-global, the previous tab's names keep driving workspace-screen.tsx polling after a POU switch.
Suggested fix
useEffect(() => {
- if (!isDebuggerVisible || !editorRef.current || (language !== 'st' && language !== 'il')) return
+ if (!isDebuggerVisible || !editorRef.current || (language !== 'st' && language !== 'il')) {
+ setDebugViewportVarNames(new Set<string>())
+ return
+ }
const editor = editorRef.current
const model = editor.getModel()
if (!model) return
const varNames = pou?.data.variables.map((v) => v.name).filter((n) => n && n.trim() !== '') || []
- if (varNames.length === 0) return
+ if (varNames.length === 0) {
+ setDebugViewportVarNames(new Set<string>())
+ return
+ }
@@
return () => {
disposable.dispose()
clearTimeout(timer)
+ setDebugViewportVarNames(new Set<string>())
}
}, [isDebuggerVisible, editorMounted, name, language, pou?.data.variables, setDebugViewportVarNames])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (!isDebuggerVisible || !editorRef.current || (language !== 'st' && language !== 'il')) return | |
| const editor = editorRef.current | |
| const model = editor.getModel() | |
| if (!model) return | |
| const varNames = pou?.data.variables.map((v) => v.name).filter((n) => n && n.trim() !== '') || [] | |
| if (varNames.length === 0) return | |
| const computeVisibleVars = () => { | |
| const ranges = editor.getVisibleRanges() | |
| const visibleLines: string[] = [] | |
| for (const range of ranges) { | |
| for (let line = range.startLineNumber; line <= range.endLineNumber; line++) { | |
| visibleLines.push(model.getLineContent(line)) | |
| } | |
| } | |
| const sourceText = visibleLines.join('\n') | |
| const visibleVarNames = collectSTVariableNames(sourceText, varNames) | |
| setDebugViewportVarNames(visibleVarNames) | |
| } | |
| // Compute immediately | |
| computeVisibleVars() | |
| // Debounce on scroll | |
| let timer: ReturnType<typeof setTimeout> | |
| const disposable = editor.onDidScrollChange(() => { | |
| clearTimeout(timer) | |
| timer = setTimeout(computeVisibleVars, 1000) | |
| }) | |
| return () => { | |
| disposable.dispose() | |
| clearTimeout(timer) | |
| } | |
| }, [isDebuggerVisible, editorMounted, name, language, pou?.data.variables, setDebugViewportVarNames]) | |
| useEffect(() => { | |
| if (!isDebuggerVisible || !editorRef.current || (language !== 'st' && language !== 'il')) { | |
| setDebugViewportVarNames(new Set<string>()) | |
| return | |
| } | |
| const editor = editorRef.current | |
| const model = editor.getModel() | |
| if (!model) return | |
| const varNames = pou?.data.variables.map((v) => v.name).filter((n) => n && n.trim() !== '') || [] | |
| if (varNames.length === 0) { | |
| setDebugViewportVarNames(new Set<string>()) | |
| return | |
| } | |
| const computeVisibleVars = () => { | |
| const ranges = editor.getVisibleRanges() | |
| const visibleLines: string[] = [] | |
| for (const range of ranges) { | |
| for (let line = range.startLineNumber; line <= range.endLineNumber; line++) { | |
| visibleLines.push(model.getLineContent(line)) | |
| } | |
| } | |
| const sourceText = visibleLines.join('\n') | |
| const visibleVarNames = collectSTVariableNames(sourceText, varNames) | |
| setDebugViewportVarNames(visibleVarNames) | |
| } | |
| // Compute immediately | |
| computeVisibleVars() | |
| // Debounce on scroll | |
| let timer: ReturnType<typeof setTimeout> | |
| const disposable = editor.onDidScrollChange(() => { | |
| clearTimeout(timer) | |
| timer = setTimeout(computeVisibleVars, 1000) | |
| }) | |
| return () => { | |
| disposable.dispose() | |
| clearTimeout(timer) | |
| setDebugViewportVarNames(new Set<string>()) | |
| } | |
| }, [isDebuggerVisible, editorMounted, name, language, pou?.data.variables, setDebugViewportVarNames]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/_features/`[workspace]/editor/monaco/index.tsx around
lines 251 - 288, When there are no variables the effect currently returns early
and never clears the workspace-global debug viewport state; change the
early-return branch in the useEffect inside the Monaco editor (the block that
computes varNames from pou?.data.variables) to call setDebugViewportVarNames([])
before returning so the global store is reset; ensure this uses the same editor
effect (referenced symbols: pou?.data.variables, varNames, computeVisibleVars,
setDebugViewportVarNames) so switching POUs clears the previous viewport names.
Comment on lines
+724
to
+730
| if (node.type === 'input-variable' || node.type === 'output-variable' || node.type === 'inout-variable') { | ||
| const varName = (node.data as { variable?: { name?: string } }).variable?.name | ||
| if (varName) varNames.add(varName) | ||
| } | ||
| if (node.type === 'block') { | ||
| const blockData = node.data as { variant?: { type: string }; numericId?: string } | ||
| if (blockData.numericId) varNames.add(blockData.numericId) |
Contributor
There was a problem hiding this comment.
Collect the FBD block variable name, not just numericId.
This has the same key-space mismatch as the ladder collector: workspace-screen.tsx looks for visible variable names when expanding FB members, but this code only sends numericId, so blocks like TON0/PID0 never enqueue TON0.Q, PID0.OUT, etc.
🤖 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 724 - 730, The collector currently adds only blockData.numericId for nodes
of type 'block', which mismatches the visible variable names used elsewhere
(e.g., workspace-screen.tsx) and prevents enqueuing member names like TON0.Q or
PID0.OUT; change the 'block' branch in index.tsx to extract and add the block's
variable name (e.g., blockData.variable?.name or the field that contains the
human-facing block name) to varNames instead of—or in addition to—numericId so
the key-space matches the visible variable names used when expanding FB members.
Comment on lines
+1213
to
+1249
| if (currentPou && debugViewportVarNames.size > 0) { | ||
| // Add direct POU variables | ||
| for (const varName of debugViewportVarNames) { | ||
| const compositeKey = makeCompositeKeyForCurrentPou(varName) | ||
| if (compositeKey) { | ||
| debugVariableKeys.add(compositeKey) | ||
| } | ||
| } | ||
|
|
||
| // For FB POUs, poll nested FB variables using instance context | ||
| // For program POUs, poll FB instance variables using the standard approach | ||
| if (currentPou.type === 'function-block' && fbdFbInstanceCtx) { | ||
| // Poll all nested BOOL variables within the FB instance | ||
| Array.from(variableInfoMapRef.current.entries()).forEach(([_, varInfos]) => { | ||
| // Add FB instance nested variables for derived-type variables in the visible set | ||
| // (e.g., if TON0 is visible, also poll TON0.ET, TON0.Q, TON0.IN, TON0.PT) | ||
| const visibleFbInstances = currentPou.data.variables.filter( | ||
| (v) => v.type.definition === 'derived' && debugViewportVarNames.has(v.name), | ||
| ) | ||
| visibleFbInstances.forEach((fbInstance) => { | ||
| Array.from(variableInfoMapRef.current!.entries()).forEach(([_, varInfos]) => { | ||
| for (const varInfo of varInfos) { | ||
| if ( | ||
| varInfo.pouName === fbdFbInstanceCtx.programName && | ||
| varInfo.variable.name.startsWith(`${fbdFbInstanceCtx.fbVariableName}.`) && | ||
| varInfo.variable.type.definition === 'base-type' && | ||
| varInfo.variable.type.value.toLowerCase() === 'bool' | ||
| ) { | ||
| const compositeKey = `${varInfo.pouName}:${varInfo.variable.name}` | ||
| debugVariableKeys.add(compositeKey) | ||
| } | ||
| } | ||
| }) | ||
| } else { | ||
| const functionBlockInstances = currentPou.data.variables.filter( | ||
| (variable) => variable.type.definition === 'derived', | ||
| ) | ||
|
|
||
| functionBlockInstances.forEach((fbInstance) => { | ||
| Array.from(variableInfoMapRef.current!.entries()).forEach(([_, varInfos]) => { | ||
| for (const varInfo of varInfos) { | ||
| if (fbInstanceCtx) { | ||
| // FB POU context | ||
| if ( | ||
| varInfo.pouName === currentPou.data.name && | ||
| varInfo.variable.name.startsWith(`${fbInstance.name}.`) && | ||
| varInfo.variable.type.definition === 'base-type' && | ||
| varInfo.variable.type.value.toLowerCase() === 'bool' | ||
| varInfo.pouName === fbInstanceCtx.programName && | ||
| varInfo.variable.name.startsWith(`${fbInstanceCtx.fbVariableName}.${fbInstance.name}.`) | ||
| ) { | ||
| const compositeKey = `${varInfo.pouName}:${varInfo.variable.name}` | ||
| debugVariableKeys.add(compositeKey) | ||
| debugVariableKeys.add(`${varInfo.pouName}:${varInfo.variable.name}`) | ||
| } | ||
| } | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| // For FB POUs, poll function outputs using instance context | ||
| // For program POUs, poll function outputs using the standard approach | ||
| if (currentPou.type === 'function-block' && fbdFbInstanceCtx && currentFbdFlow) { | ||
| currentFbdFlow.rung.nodes.forEach((node) => { | ||
| if (node.type === 'block') { | ||
| const blockData = node.data as { | ||
| variant?: { type: string } | ||
| numericId?: string | ||
| } | ||
|
|
||
| if (blockData.variant?.type === 'function' && blockData.numericId) { | ||
| Array.from(variableInfoMapRef.current!.entries()).forEach(([_, varInfos]) => { | ||
| for (const varInfo of varInfos) { | ||
| if ( | ||
| varInfo.pouName === fbdFbInstanceCtx.programName && | ||
| varInfo.variable.name.startsWith(`${fbdFbInstanceCtx.fbVariableName}.`) && | ||
| varInfo.variable.name.includes(blockData.numericId!) | ||
| ) { | ||
| const compositeKey = `${varInfo.pouName}:${varInfo.variable.name}` | ||
| debugVariableKeys.add(compositeKey) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| }) | ||
| } else { | ||
| const instances = currentProject.data.configuration.resource.instances | ||
| const programInstance = instances.find((inst) => inst.program === currentPou.data.name) | ||
| if (programInstance && currentFbdFlow) { | ||
| currentFbdFlow.rung.nodes.forEach((node) => { | ||
| if (node.type === 'block') { | ||
| const blockData = node.data as { | ||
| variant?: { type: string } | ||
| numericId?: string | ||
| } | ||
|
|
||
| if (blockData.variant?.type === 'function' && blockData.numericId) { | ||
| Array.from(variableInfoMapRef.current!.entries()).forEach(([_, varInfos]) => { | ||
| for (const varInfo of varInfos) { | ||
| if ( | ||
| varInfo.pouName === currentPou.data.name && | ||
| varInfo.variable.name.includes(blockData.numericId!) | ||
| ) { | ||
| const compositeKey = `${varInfo.pouName}:${varInfo.variable.name}` | ||
| debugVariableKeys.add(compositeKey) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Poll all variables of the active POU so non-BOOL values can be displayed on the diagram. | ||
| // This adds every variable registered in variableInfoMapRef that belongs to the current POU, | ||
| // enabling the DebugValueBadge components to show real-time values for INT, REAL, etc. | ||
| if (currentPou) { | ||
| if (currentPou.type === 'function-block') { | ||
| // For FB POUs, resolve the selected instance context and match variables by | ||
| // program name + instance path prefix (same pattern used for BOOL polling above). | ||
| const fbTypeKey = currentPou.data.name.toUpperCase() | ||
| const selectedKey = fbSelectedInstance.get(fbTypeKey) | ||
| if (selectedKey) { | ||
| const instances = fbDebugInstances.get(fbTypeKey) || [] | ||
| const selectedInstance = instances.find((inst) => inst.key === selectedKey) | ||
| if (selectedInstance) { | ||
| const instancePrefix = `${selectedInstance.fbVariableName}.` | ||
| Array.from(variableInfoMapRef.current.values()).forEach((varInfos) => { | ||
| for (const varInfo of varInfos) { | ||
| if ( | ||
| varInfo.pouName === selectedInstance.programName && | ||
| varInfo.variable.name.startsWith(instancePrefix) | ||
| ) { | ||
| debugVariableKeys.add(`${varInfo.pouName}:${varInfo.variable.name}`) | ||
| } | ||
| } else { | ||
| // Program POU context | ||
| if ( | ||
| varInfo.pouName === currentPou.data.name && | ||
| varInfo.variable.name.startsWith(`${fbInstance.name}.`) | ||
| ) { | ||
| debugVariableKeys.add(`${varInfo.pouName}:${varInfo.variable.name}`) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| } else { | ||
| const activePouName = currentPou.data.name | ||
| Array.from(variableInfoMapRef.current.values()).forEach((varInfos) => { | ||
| for (const varInfo of varInfos) { | ||
| if (varInfo.pouName === activePouName) { | ||
| debugVariableKeys.add(`${varInfo.pouName}:${varInfo.variable.name}`) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Viewport polling still ignores non-direct markers.
This branch only expands derived parents. It never turns graphical numericId markers into the _TMP_* keys used by function outputs, and it skips array/UDT parents even though variableInfoMapRef already contains their children. Visible function blocks, struct fields, and array elements will therefore stay stale unless they are watched or tree-expanded elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/screens/workspace-screen.tsx` around lines 1213 - 1249, The
viewport-poll expansion only handles derived-type parents and misses
function-output numericId markers and children of array/UDT parents; update the
logic that builds debugVariableKeys (around debugViewportVarNames,
makeCompositeKeyForCurrentPou, variableInfoMapRef, currentPou, fbInstanceCtx) to
also: 1) when a visible variable is a function/block marker (numericId) call the
same composite-key mapping used for direct vars (via
makeCompositeKeyForCurrentPou or the existing composite-key convention) and add
the resulting _TMP_* keys to debugVariableKeys; 2) treat visible array/UDT
parents similarly to derived by scanning variableInfoMapRef.current entries for
children whose varInfo.variable.name startsWith the parent name (using the same
pou-context checks with fbInstanceCtx/currentPou.data.name) and add
pouName:childName for each child so array/UDT fields and struct members are
included in polling.
Comment on lines
+10
to
+14
| // Build regex patterns for each variable name (word-boundary match) | ||
| const patterns = variableNames.map((name) => { | ||
| const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') | ||
| return { name, pattern: new RegExp(`\\b${escaped}(?![\\w.\\[])`, 'i') } | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/renderer/utils/debug/collect-st-variables.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 1554
🏁 Script executed:
cat -n src/renderer/screens/workspace-screen.tsx | head -100Repository: Autonomy-Logic/openplc-editor
Length of output: 5066
🏁 Script executed:
rg -n "debugViewportVarNames" src/renderer/screens/workspace-screen.tsx -A 5 -B 5Repository: Autonomy-Logic/openplc-editor
Length of output: 1322
🏁 Script executed:
rg -n "collectSTVariableNames" --type ts --type tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 100
🏁 Script executed:
rg -n "debugViewportVarNames" src/ -B 3 -A 3 | head -80Repository: Autonomy-Logic/openplc-editor
Length of output: 5037
🏁 Script executed:
rg "collectSTVariableNames" src/ -A 5 -B 5Repository: Autonomy-Logic/openplc-editor
Length of output: 4180
🏁 Script executed:
find src -name "*.ts" -o -name "*.tsx" | xargs grep -l "collectSTVariableNames"Repository: Autonomy-Logic/openplc-editor
Length of output: 191
🏁 Script executed:
find src -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.test.tsx" -o -name "*.spec.tsx" \) | xargs grep -l "collectSTVariableNames"Repository: Autonomy-Logic/openplc-editor
Length of output: 57
🏁 Script executed:
cat -n src/renderer/components/_features/[workspace]/editor/monaco/index.tsx | sed -n '1,150p' | tail -80Repository: Autonomy-Logic/openplc-editor
Length of output: 3468
🏁 Script executed:
cat -n src/renderer/components/_features/[workspace]/editor/monaco/index.tsx | grep -A 30 "const visibleVarNames = collectSTVariableNames"Repository: Autonomy-Logic/openplc-editor
Length of output: 1428
🏁 Script executed:
cat -n src/renderer/components/_features/[workspace]/editor/monaco/index.tsx | grep -B 30 "const visibleVarNames = collectSTVariableNames"Repository: Autonomy-Logic/openplc-editor
Length of output: 1480
🏁 Script executed:
cat -n src/renderer/components/_features/[workspace]/editor/monaco/index.tsx | sed -n '258p;259p;260p'Repository: Autonomy-Logic/openplc-editor
Length of output: 234
🏁 Script executed:
cat -n src/renderer/utils/debug/collect-st-variables.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 1554
The negative lookahead pattern prevents parent symbols from being detected when only accessed via member or array notation.
When source code contains TON0.Q, cfg.limit, or arr[1], the pattern \b${escaped}(?![\w.\[]) rejects parent matches because the negative lookahead forbids matches followed by . or [. As a result, parent symbols are never added to debugViewportVarNames. Since nested FB variable polling depends on parent presence (workspace-screen.tsx line 1225: debugViewportVarNames.has(v.name)), users cannot debug derived-type members unless the parent symbol appears standalone in the visible code. This breaks debugging when only member accesses are visible.
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 12-12: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\b${escaped}(?![\\w.\\[]), 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/utils/debug/collect-st-variables.ts` around lines 10 - 14, The
regex in the variableNames.map block uses a negative lookahead (/(?![\w.\[])/)
that prevents matching parent symbols when they're accessed via member or array
notation (e.g., TON0.Q, cfg.limit, arr[1]); update the RegExp construction in
that block (the code that builds patterns from variableNames/escaped) to remove
the negative lookahead and instead rely on the existing word-boundary (\\b) or a
more permissive lookahead that still prevents partial identifier matches but
allows '.' and '[' after the identifier (for example, use new
RegExp(`\\b${escaped}`, 'i')). This will ensure debugViewportVarNames can
include parent symbols used only via member or array access so downstream checks
(e.g., debugViewportVarNames.has(v.name) in workspace-screen.tsx) work
correctly.
Comment on lines
+5
to
+43
| function stripLineComments(line: string, state: BlockCommentState): { stripped: string; state: BlockCommentState } { | ||
| const chars = [...line] | ||
| let i = 0 | ||
| let s = state | ||
|
|
||
| while (i < chars.length) { | ||
| if (s) { | ||
| const endMarker = s === 'paren' ? ')' : '/' | ||
| if (chars[i] === '*' && chars[i + 1] === endMarker) { | ||
| chars[i] = ' ' | ||
| chars[i + 1] = ' ' | ||
| i += 2 | ||
| s = false | ||
| } else { | ||
| chars[i] = ' ' | ||
| i++ | ||
| } | ||
| } else { | ||
| if (chars[i] === '/' && chars[i + 1] === '/') { | ||
| for (let j = i; j < chars.length; j++) chars[j] = ' ' | ||
| break | ||
| } | ||
| if (chars[i] === '(' && chars[i + 1] === '*') { | ||
| chars[i] = ' ' | ||
| chars[i + 1] = ' ' | ||
| i += 2 | ||
| s = 'paren' | ||
| } else if (chars[i] === '/' && chars[i + 1] === '*') { | ||
| chars[i] = ' ' | ||
| chars[i + 1] = ' ' | ||
| i += 2 | ||
| s = 'slash' | ||
| } else { | ||
| i++ | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return { stripped: chars.join(''), state: s } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "strip-line-comments.ts" -type fRepository: Autonomy-Logic/openplc-editor
Length of output: 122
🏁 Script executed:
cat -n src/renderer/utils/debug/strip-line-comments.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 1654
🏁 Script executed:
rg -r "stripLineComments" --type ts --type tsx -A 3 -B 1Repository: Autonomy-Logic/openplc-editor
Length of output: 469
🏁 Script executed:
# Look for tests related to this function
fd ".*test.*" --type f | xargs grep -l "stripLineComments" 2>/dev/null || echo "No test files found"Repository: Autonomy-Logic/openplc-editor
Length of output: 92
🏁 Script executed:
# Check if there's documentation or comments about the function's purpose
head -30 src/renderer/utils/debug/strip-line-comments.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 1016
🏁 Script executed:
# Look at the broader debug utilities directory
ls -la src/renderer/utils/debug/Repository: Autonomy-Logic/openplc-editor
Length of output: 328
🏁 Script executed:
rg "stripLineComments" --type ts --type tsx -A 3 -B 1Repository: Autonomy-Logic/openplc-editor
Length of output: 100
🏁 Script executed:
# Check the collect-st-variables file which is in the same directory
cat -n src/renderer/utils/debug/collect-st-variables.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 1554
🏁 Script executed:
# Search for usage in the whole codebase to understand the context better
rg "stripLineComments\|BlockCommentState" --type tsRepository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Look for any tests or examples of how this is used
fd "test\|spec" --type f | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Check if there are any other related comment-stripping functions
rg "comment" src/renderer/utils/debug/ --type tsRepository: Autonomy-Logic/openplc-editor
Length of output: 598
🏁 Script executed:
# Check the language context - look for IEC 61131-3 or ST language references
rg -i "iec\|structured text\|st language" src/renderer/ --type ts | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Look at the tsconfig or package.json to understand the project
cat tsconfig.json | head -50Repository: Autonomy-Logic/openplc-editor
Length of output: 1116
🏁 Script executed:
# Check for any documentation about string literal handling in ST language
find . -name "*.md" -o -name "*.txt" | xargs grep -l "string\|literal\|quote" 2>/dev/null | head -5Repository: Autonomy-Logic/openplc-editor
Length of output: 282
🏁 Script executed:
# Check the project documentation for ST/IEC 61131-3 language info
cat ./CLAUDE.md | head -100Repository: Autonomy-Logic/openplc-editor
Length of output: 3123
🏁 Script executed:
# Look for ST language specification or parsing logic
rg -i "st\|structured text" src/renderer/utils/debug/ --type ts -A 5 -B 2Repository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Check the collectSTVariableNames usage to understand what language/domain
rg "collectSTVariableNames" --type ts -B 5 -A 5Repository: Autonomy-Logic/openplc-editor
Length of output: 4180
🏁 Script executed:
# Understand ST language string literal syntax better - check examples in codebase
rg "':=" --type ts | head -10Repository: Autonomy-Logic/openplc-editor
Length of output: 410
🏁 Script executed:
# Check if there are any existing string/quote handling utilities
rg "quote\|string.*literal" src/renderer/utils/ --type ts -A 2 -B 2Repository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Look at the Monaco editor integration to see how stripLineComments is used in practice
sed -n '1,100p' src/renderer/components/_features/[workspace]/editor/monaco/index.tsx | head -50Repository: Autonomy-Logic/openplc-editor
Length of output: 1969
🏁 Script executed:
# Search for any string literal pattern definitions in the codebase
rg "\'.*\'" src/renderer/ --type ts | grep -v node_modules | head -15Repository: Autonomy-Logic/openplc-editor
Length of output: 145
🏁 Script executed:
# Search for ST language string or quoted literal handling
rg "string\|quoted" src/renderer/components/_features/workspace/editor/monaco/configs/languages/st/ --type ts -A 3 -B 1Repository: Autonomy-Logic/openplc-editor
Length of output: 192
🏁 Script executed:
# Look at the ST language configuration
cat src/renderer/components/_features/\[workspace\]/editor/monaco/configs/languages/st/st.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 13834
🏁 Script executed:
# Search for IEC 61131-3 string literal syntax documentation in code
rg -i "string.*literal\|quote" docs/ --type md -A 2 -B 1Repository: Autonomy-Logic/openplc-editor
Length of output: 55
Add quote-state tracking to prevent stripping comment markers inside string literals.
The stripLineComments() function tracks block-comment state (false | 'paren' | 'slash') but does not track quote state. In ST/IEC 61131-3, single-quoted strings can contain // sequences (e.g., msg := 'http://x'; ready := TRUE;). Without quote tracking, the function treats // inside the string as a line comment, blanking the rest of the line and hiding the ready identifier from both viewport variable collection and debug badge scanning.
Extend BlockCommentState to include 'single-quote' and implement quote detection in the scanner loop, mirroring the existing block-comment state transitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/utils/debug/strip-line-comments.ts` around lines 5 - 43,
stripLineComments fails to track string quotes so it treats // inside
single-quoted literals as comments; update the BlockCommentState union to
include 'single-quote' and modify stripLineComments (use the local state
variable s, the chars array and the scanning while loop) to detect entering a
single-quote when not already in a comment (on chars[i] === ''' ), set s =
'single-quote' and advance, and when s === 'single-quote' scan forward until the
closing quote (handle escaped single-quotes by treating doubled quotes '' as an
escaped quote and advancing past both) without blanking characters (unlike
block-comment states), then on closing quote set s = false and continue so //
inside strings won't be treated as line comments.
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
1 participant
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.
Summary
getViewport()+ container dimensionsgetVisibleRanges()stripLineCommentsandcollectSTVariableNamesas shared utilities undersrc/renderer/utils/debug/debugViewportVarNamesstore field with proper cleanup on debug hide and workspace clearfbInstanceCtxcomputation to avoid duplication across language sectionsTest plan
?briefly then values within ~1s🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Performance