feat: unified simulator Start/Stop with auto-debug#625
Merged
Conversation
… click When "Simulator" is selected as the target, the Build and Debug buttons are now disabled. Clicking Start triggers the full pipeline: save project, compile, load firmware into the simulator, and auto-connect the debugger — skipping the redundant compileForDebugger and MD5 verification steps. Clicking Stop disconnects the debugger first, then stops the simulator. Also fixes two pre-existing bugs: - Sidebar activity bar tooltips were invisible because Radix Tooltip's positioning ref was lost through non-ref-forwarding button wrappers. Wrapping children in a native DOM element gives Radix an anchor. - Debugger disconnect race condition: a straggling poll could fire after disconnect cleared the connection type, producing a spurious "connection lost" error. An early return now silences this. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
WalkthroughAdds centralized debugger-session utilities, integrates them into workspace activity bar to support simulator auto-attach and lifecycle handling, adds a null-guard in the main IPC debugger variables handler, and makes a small tooltip DOM wrapper change. Changes
Sequence DiagramsequenceDiagram
participant User
participant Renderer
participant BuildProcess
participant DebugFile
participant DebuggerService
participant IPCMain
User->>Renderer: Click Start (simulate + auto-debug)
Renderer->>BuildProcess: Trigger build/verify (await)
BuildProcess-->>Renderer: Build success (debug file produced)
Renderer->>DebugFile: Parse debug file, build index & trees
DebugFile-->>Renderer: indexMap, treeMap, fbDebugInstancesMap
Renderer->>DebuggerService: connectAndActivateDebugger(params)
DebuggerService->>IPCMain: Establish IPC connection to simulator debugger
IPCMain-->>DebuggerService: Connection established
DebuggerService-->>Renderer: Activated (indices, selection, UI shown)
User->>Renderer: Click Stop
Renderer->>DebuggerService: disconnectDebugger()
DebuggerService->>IPCMain: Clear connection / state
IPCMain-->>DebuggerService: Disconnected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/renderer/components/_organisms/workspace-activity-bar/default.tsx (2)
657-716: Dead code: simulator block inhandleDebuggerClickis now unreachable.The early return at Line 659 (
if (isCurrentBoardSimulator) return) makes theif (isSimulatorTarget(currentBoardInfo))block at Lines 697–716 (and theconnectionType = 'simulator'assignment) permanently unreachable. This should be removed.♻️ Proposed cleanup
const handleDebuggerClick = async () => { // Simulator target uses the unified Start/Stop flow instead if (isCurrentBoardSimulator) return const { workspace, project, deviceDefinitions, workspaceActions, consoleActions, deviceActions } = useOpenPLCStore.getState() // ... (disconnect path unchanged) ... let targetIpAddress: string | undefined let connectionType: 'tcp' | 'rtu' | 'websocket' | 'simulator' = 'tcp' // ... - if (isSimulatorTarget(currentBoardInfo)) { - // Check if simulator has firmware loaded - const running = await (window.bridge.simulatorIsRunning as () => Promise<boolean>)() - if (!running) { - const response = await showDebuggerMessage( - 'warning', - 'Simulator Empty', - 'No firmware is running on the simulator. Would you like to build and upload the project first?', - ['Build & Upload', 'Cancel'], - ) - if (response === 0) { - setIsDebuggerProcessing(false) - void verifyAndCompile() - return - } else { - setIsDebuggerProcessing(false) - return - } - } - connectionType = 'simulator' - } else if (isRuntimeTarget) { + if (isRuntimeTarget) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx` around lines 657 - 716, The simulator-specific branch in handleDebuggerClick is unreachable because of the early guard if (isCurrentBoardSimulator) return; remove the dead code: delete the isSimulatorTarget(currentBoardInfo) conditional block (the simulatorIsRunning check, showDebuggerMessage flow, and any returns inside it) and remove any assignment of connectionType = 'simulator' so the function no longer contains unused simulator logic; keep the initial isCurrentBoardSimulator guard in place and ensure no other references (e.g., connectionType, simulator-specific variables) remain unused after removal.
467-655: Substantial code duplication betweenconnectDebuggerAfterBuildandhandleMd5Verification.The index-map building (Lines 490–522), debug-tree construction (Lines 524–575), and FB-instance-map building (Lines 577–636) in
connectDebuggerAfterBuildare near-identical copies of the same logic inhandleMd5Verification(Lines 1176–1341). A bug fix or enhancement in one will likely not be applied to the other.Consider extracting a shared helper, e.g.:
const buildDebugContext = ( project: typeof useOpenPLCStore.getState()['project'], parsed: ReturnType<typeof parseDebugFile>, ): { indexMap: Map<string, number>; treeMap: Map<string, DebugTreeNode>; fbInstancesMap: Map<string, FbInstanceInfo[]> } => { // ... shared implementation ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx` around lines 467 - 655, The connectDebuggerAfterBuild function duplicates the index-map, debug-tree and FB-instance-map logic already present in handleMd5Verification; extract that shared logic into a helper (suggested name buildDebugContext) that accepts the current project and parsed debug file (parseDebugFile result) and returns { indexMap, treeMap, fbInstancesMap } (Maps of debug indexes, DebugTreeNode map and FbInstanceInfo[] map). Move the traversal/lookup logic that uses buildDebugTree, findGlobalVariableIndex, findVariableIndexWithFallback, StandardFunctionBlocks and the project.data.pous/instances iteration into buildDebugContext, have it also perform the complex count and silent-tree build catches, and then replace the duplicated blocks in connectDebuggerAfterBuild and handleMd5Verification with calls to buildDebugContext and subsequent calls to workspaceActions.setDebugVariableTree, workspaceActions.setFbDebugInstances, workspaceActions.setDebugVariableIndexes and workspaceActions.setFbSelectedInstance as before.
🤖 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/_organisms/workspace-activity-bar/default.tsx`:
- Around line 344-352: handleSimulatorControl sets
pendingSimulatorDebugRef.current = true before calling verifyAndCompile(), but
handleRequest can return early on validation failures without resetting that
flag; update the early-exit paths in handleRequest (the CPP/Python validation
failure branches referenced around where it returns before firmware handling) to
explicitly set pendingSimulatorDebugRef.current = false before returning so the
flag is cleared on all non-firmware/early-abort paths, leaving the existing
.catch() that clears it for firmware errors and preserving
connectDebuggerAfterBuild behavior when the build succeeds.
---
Nitpick comments:
In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx`:
- Around line 657-716: The simulator-specific branch in handleDebuggerClick is
unreachable because of the early guard if (isCurrentBoardSimulator) return;
remove the dead code: delete the isSimulatorTarget(currentBoardInfo) conditional
block (the simulatorIsRunning check, showDebuggerMessage flow, and any returns
inside it) and remove any assignment of connectionType = 'simulator' so the
function no longer contains unused simulator logic; keep the initial
isCurrentBoardSimulator guard in place and ensure no other references (e.g.,
connectionType, simulator-specific variables) remain unused after removal.
- Around line 467-655: The connectDebuggerAfterBuild function duplicates the
index-map, debug-tree and FB-instance-map logic already present in
handleMd5Verification; extract that shared logic into a helper (suggested name
buildDebugContext) that accepts the current project and parsed debug file
(parseDebugFile result) and returns { indexMap, treeMap, fbInstancesMap } (Maps
of debug indexes, DebugTreeNode map and FbInstanceInfo[] map). Move the
traversal/lookup logic that uses buildDebugTree, findGlobalVariableIndex,
findVariableIndexWithFallback, StandardFunctionBlocks and the
project.data.pous/instances iteration into buildDebugContext, have it also
perform the complex count and silent-tree build catches, and then replace the
duplicated blocks in connectDebuggerAfterBuild and handleMd5Verification with
calls to buildDebugContext and subsequent calls to
workspaceActions.setDebugVariableTree, workspaceActions.setFbDebugInstances,
workspaceActions.setDebugVariableIndexes and
workspaceActions.setFbSelectedInstance as before.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/main/modules/ipc/main.tssrc/renderer/components/_molecules/workspace-activity-bar/tooltip-button.tsxsrc/renderer/components/_organisms/workspace-activity-bar/default.tsx
Comment on lines
+344
to
+352
| .then(async (result) => { | ||
| if (result.success) { | ||
| setSimulatorRunning(true) | ||
| addLog({ id: crypto.randomUUID(), level: 'info', message: 'Simulator is running.' }) | ||
|
|
||
| // Auto-connect debugger after build when triggered from the Start button | ||
| if (pendingSimulatorDebugRef.current) { | ||
| pendingSimulatorDebugRef.current = false | ||
| await connectDebuggerAfterBuild() |
Contributor
There was a problem hiding this comment.
pendingSimulatorDebugRef.current not reset if handleRequest returns early before firmware.
handleSimulatorControl sets pendingSimulatorDebugRef.current = true before calling void verifyAndCompile(). If compilation aborts early inside handleRequest (e.g., CPP/Python validation failure at Lines 248–251 or 938–941), neither of those paths resets the flag. The firmware-path .catch() at Line 364 correctly resets it, but pre-firmware failure paths do not.
While this is benign today (Compile is disabled for simulator targets and a subsequent Start press unconditionally sets the flag back to true), explicitly resetting it in the early-exit paths of handleRequest would make the lifecycle easier to reason about.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx` around
lines 344 - 352, handleSimulatorControl sets pendingSimulatorDebugRef.current =
true before calling verifyAndCompile(), but handleRequest can return early on
validation failures without resetting that flag; update the early-exit paths in
handleRequest (the CPP/Python validation failure branches referenced around
where it returns before firmware handling) to explicitly set
pendingSimulatorDebugRef.current = false before returning so the flag is cleared
on all non-firmware/early-abort paths, leaving the existing .catch() that clears
it for firmware errors and preserving connectDebuggerAfterBuild behavior when
the build succeeds.
Extract 5 helper functions (disconnectDebugger, buildVariableIndexMap, buildDebugVariableTreeMap, buildFbInstanceMap, connectAndActivateDebugger) into a new debugger-session.ts module. Both the simulator flow and the non-simulator MD5 verification flow now call the same helpers, removing ~300 lines of duplicated logic and 2 inline normalizeTypeString redefinitions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/renderer/components/_organisms/workspace-activity-bar/default.tsx (4)
286-287:⚠️ Potential issue | 🟡 MinorRemove debug
console.logstatements.🐛 Proposed fix
- console.log('processado:', processedProjectData) - console.log('original:', projectData)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx` around lines 286 - 287, Remove the two debug console.log calls that print processedProjectData and projectData in the Workspace Activity Bar component (remove the lines referencing console.log('processado:', processedProjectData) and console.log('original:', projectData)); if you need to preserve diagnostic information, replace them with a proper logger call or conditional debug-only logging tied to an existing logging utility, otherwise just delete the statements from default.tsx.
20-34:⚠️ Potential issue | 🟠 MajorType declarations are splitting the import block — move them after all imports.
The
CppPouDataandProjectDataWithCpptype declarations at lines 20–28 are sandwiched between two import groups (lines 1–18 and lines 29–34). This violates thesimple-import-sortrule and will fail the ESLint check.♻️ Proposed fix
import { wrapUnsupportedComments } from '@root/utils/PLC/wrap-unsupported-comments' import { parsePlcStatus } from '@root/utils/plc-status' import { addPythonLocalVariables } from '@root/utils/python/addPythonLocalVariables' import { generateSTCode } from '@root/utils/python/generateSTCode' import { injectPythonCode } from '@root/utils/python/injectPythonCode' import { useEffect, useRef, useState } from 'react' +type CppPouData = { + name: string + code: string + variables: unknown[] +} + +type ProjectDataWithCpp = PLCProjectData & { + originalCppPous?: CppPouData[] +} + import { ... } from '../../_molecules/...'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx` around lines 20 - 34, The type declarations CppPouData and ProjectDataWithCpp are placed between import groups and break the simple-import-sort rule; move the two type blocks (CppPouData and ProjectDataWithCpp) so they appear after all import statements (i.e., below the last import such as injectPythonCode) to keep imports contiguous and satisfy ESLint; ensure the moved types remain in the same module scope and update any local references to CppPouData / ProjectDataWithCpp if necessary.
185-185:⚠️ Potential issue | 🟡 MinorRemove debug
console.log— line 185 is a leftover artifact.🐛 Proposed fix
- console.log(processedProjectData)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx` at line 185, Remove the leftover debug statement console.log(processedProjectData) from the component (the stray call to console.log with processedProjectData); if you need persistent telemetry or debug info, replace it with the app logging utility (e.g., processLogger.debug or the established logger) or conditionally log behind a DEBUG flag, otherwise just delete the console.log line.
558-613: 🛠️ Refactor suggestion | 🟠 MajorDead simulator code path in
handleDebuggerClickshould be removed.The early return at line 560 (
if (isCurrentBoardSimulator) return) makes the entire simulator-specific branch at lines 594–613 unreachable. This is stale logic left over from before the unified Start/Stop flow.♻️ Proposed fix — remove unreachable simulator branch
const handleDebuggerClick = async () => { // Simulator target uses the unified Start/Stop flow instead if (isCurrentBoardSimulator) return const { workspace, project, deviceDefinitions, workspaceActions, consoleActions, deviceActions } = useOpenPLCStore.getState() if (workspace.isDebuggerVisible) { await disconnectDebugger(workspaceActions) return } ... - if (isSimulatorTarget(currentBoardInfo)) { - // Check if simulator has firmware loaded - const running = await (window.bridge.simulatorIsRunning as () => Promise<boolean>)() - if (!running) { - const response = await showDebuggerMessage( - 'warning', - 'Simulator Empty', - 'No firmware is running on the simulator. Would you like to build and upload the project first?', - ['Build & Upload', 'Cancel'], - ) - if (response === 0) { - // Trigger full build, then restart debugger flow - setIsDebuggerProcessing(false) - void verifyAndCompile() - return - } else { - setIsDebuggerProcessing(false) - return - } - } - connectionType = 'simulator' - } else if (isRuntimeTarget) { + if (isRuntimeTarget) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx` around lines 558 - 613, The early return via isCurrentBoardSimulator makes the whole simulator-specific branch in handleDebuggerClick unreachable; remove the entire if (isSimulatorTarget(currentBoardInfo)) { ... } block (including calls to window.bridge.simulatorIsRunning, showDebuggerMessage and the verifyAndCompile restart path) inside handleDebuggerClick, and then tidy up any resulting unused variables or imports (e.g., showDebuggerMessage, verifyAndCompile, or simulatorIsRunning references) to avoid dead code or linter errors.
♻️ Duplicate comments (1)
src/renderer/components/_organisms/workspace-activity-bar/default.tsx (1)
248-251:pendingSimulatorDebugRef.currentnot reset on CPP validation failure early-return.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx` around lines 248 - 251, When validationFailed triggers the early return in the block that currently calls setIsCompiling(false), also clear the pending simulator debug reference by resetting pendingSimulatorDebugRef.current (e.g., set to null/undefined) before returning so leftover pending debug state is not retained; update the validationFailed branch that contains setIsCompiling to explicitly clear pendingSimulatorDebugRef.current (and any related timeout/cleanup if applicable) prior to the return.
🤖 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/_organisms/workspace-activity-bar/default.tsx`:
- Around line 1295-1302: The DebuggerButton's disabled styling uses a hardcoded
class string and omits the sibling/child hover suppression in
disabledButtonClass; update the className logic on DebuggerButton (inside
TooltipSidebarWrapperButton) to use the same disabledButtonClass when
(isDebuggerProcessing || isCurrentBoardSimulator) is true (or add the missing
[&>*:first-child]:hover:bg-transparent to the existing class string) so the
inner element's hover state is disabled consistently like DownloadButton.
In `@src/renderer/utils/debugger-session.ts`:
- Around line 276-278: The exported function getWorkspaceActions is unused;
remove its export and definition to eliminate dead code (delete the
getWorkspaceActions function that returns
useOpenPLCStore.getState().workspaceActions), or if it was intended for external
use add explicit usage/imports where needed or document it; locate the symbol
getWorkspaceActions and the call to useOpenPLCStore.getState().workspaceActions
to remove or wire it up accordingly.
---
Outside diff comments:
In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx`:
- Around line 286-287: Remove the two debug console.log calls that print
processedProjectData and projectData in the Workspace Activity Bar component
(remove the lines referencing console.log('processado:', processedProjectData)
and console.log('original:', projectData)); if you need to preserve diagnostic
information, replace them with a proper logger call or conditional debug-only
logging tied to an existing logging utility, otherwise just delete the
statements from default.tsx.
- Around line 20-34: The type declarations CppPouData and ProjectDataWithCpp are
placed between import groups and break the simple-import-sort rule; move the two
type blocks (CppPouData and ProjectDataWithCpp) so they appear after all import
statements (i.e., below the last import such as injectPythonCode) to keep
imports contiguous and satisfy ESLint; ensure the moved types remain in the same
module scope and update any local references to CppPouData / ProjectDataWithCpp
if necessary.
- Line 185: Remove the leftover debug statement
console.log(processedProjectData) from the component (the stray call to
console.log with processedProjectData); if you need persistent telemetry or
debug info, replace it with the app logging utility (e.g., processLogger.debug
or the established logger) or conditionally log behind a DEBUG flag, otherwise
just delete the console.log line.
- Around line 558-613: The early return via isCurrentBoardSimulator makes the
whole simulator-specific branch in handleDebuggerClick unreachable; remove the
entire if (isSimulatorTarget(currentBoardInfo)) { ... } block (including calls
to window.bridge.simulatorIsRunning, showDebuggerMessage and the
verifyAndCompile restart path) inside handleDebuggerClick, and then tidy up any
resulting unused variables or imports (e.g., showDebuggerMessage,
verifyAndCompile, or simulatorIsRunning references) to avoid dead code or linter
errors.
---
Duplicate comments:
In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx`:
- Around line 248-251: When validationFailed triggers the early return in the
block that currently calls setIsCompiling(false), also clear the pending
simulator debug reference by resetting pendingSimulatorDebugRef.current (e.g.,
set to null/undefined) before returning so leftover pending debug state is not
retained; update the validationFailed branch that contains setIsCompiling to
explicitly clear pendingSimulatorDebugRef.current (and any related
timeout/cleanup if applicable) prior to the return.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/renderer/components/_organisms/workspace-activity-bar/default.tsxsrc/renderer/utils/debugger-session.ts
Comment on lines
+1295
to
1302
| <TooltipSidebarWrapperButton tooltipContent={isCurrentBoardSimulator ? 'Use Start to debug' : 'Debugger'}> | ||
| <DebuggerButton | ||
| onClick={() => void handleDebuggerClick()} | ||
| disabled={isDebuggerProcessing} | ||
| disabled={isDebuggerProcessing || isCurrentBoardSimulator} | ||
| isActive={isDebuggerVisible} | ||
| className={cn(isDebuggerProcessing && 'cursor-not-allowed opacity-50')} | ||
| className={cn((isDebuggerProcessing || isCurrentBoardSimulator) && 'cursor-not-allowed opacity-50')} | ||
| /> | ||
| </TooltipSidebarWrapperButton> |
Contributor
There was a problem hiding this comment.
DebuggerButton disabled styling is inconsistent with disabledButtonClass.
Line 1300 uses 'cursor-not-allowed opacity-50' directly, missing the [&>*:first-child]:hover:bg-transparent class present in disabledButtonClass (line 95). The hover state will remain active on the inner element, inconsistent with DownloadButton at line 1261.
🐛 Proposed fix
- className={cn((isDebuggerProcessing || isCurrentBoardSimulator) && 'cursor-not-allowed opacity-50')}
+ className={cn((isDebuggerProcessing || isCurrentBoardSimulator) && disabledButtonClass)}📝 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
| <TooltipSidebarWrapperButton tooltipContent={isCurrentBoardSimulator ? 'Use Start to debug' : 'Debugger'}> | |
| <DebuggerButton | |
| onClick={() => void handleDebuggerClick()} | |
| disabled={isDebuggerProcessing} | |
| disabled={isDebuggerProcessing || isCurrentBoardSimulator} | |
| isActive={isDebuggerVisible} | |
| className={cn(isDebuggerProcessing && 'cursor-not-allowed opacity-50')} | |
| className={cn((isDebuggerProcessing || isCurrentBoardSimulator) && 'cursor-not-allowed opacity-50')} | |
| /> | |
| </TooltipSidebarWrapperButton> | |
| <TooltipSidebarWrapperButton tooltipContent={isCurrentBoardSimulator ? 'Use Start to debug' : 'Debugger'}> | |
| <DebuggerButton | |
| onClick={() => void handleDebuggerClick()} | |
| disabled={isDebuggerProcessing || isCurrentBoardSimulator} | |
| isActive={isDebuggerVisible} | |
| className={cn((isDebuggerProcessing || isCurrentBoardSimulator) && disabledButtonClass)} | |
| /> | |
| </TooltipSidebarWrapperButton> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx` around
lines 1295 - 1302, The DebuggerButton's disabled styling uses a hardcoded class
string and omits the sibling/child hover suppression in disabledButtonClass;
update the className logic on DebuggerButton (inside
TooltipSidebarWrapperButton) to use the same disabledButtonClass when
(isDebuggerProcessing || isCurrentBoardSimulator) is true (or add the missing
[&>*:first-child]:hover:bg-transparent to the existing class string) so the
inner element's hover state is disabled consistently like DownloadButton.
- Remove redundant `projectPous` param from `buildFbInstanceMap` (always identical to `pous`) - Catch rejected `verifyAndCompile()` promise so `pendingSimulatorDebugRef` doesn't stick true on IPC failure - Remove unused `getWorkspaceActions` function and `useOpenPLCStore` import - Remove unnecessary `WorkspaceActions` re-export Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/renderer/components/_organisms/workspace-activity-bar/default.tsx (2)
1302-1302:⚠️ Potential issue | 🟡 MinorUse the shared disabled style constant for
DebuggerButtontoo.This still bypasses
disabledButtonClass, so the inner hover suppression is inconsistent with the other disabled buttons.🔧 Proposed fix
- className={cn((isDebuggerProcessing || isCurrentBoardSimulator) && 'cursor-not-allowed opacity-50')} + className={cn((isDebuggerProcessing || isCurrentBoardSimulator) && disabledButtonClass)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx` at line 1302, The DebuggerButton is using an inline className expression which bypasses the shared disabled styles; update the DebuggerButton render to apply the shared disabledButtonClass constant instead of the inline "(isDebuggerProcessing || isCurrentBoardSimulator) && 'cursor-not-allowed opacity-50'" expression so the button uses the same disabledButtonClass (including hover suppression) as other disabled buttons—locate the DebuggerButton JSX in default.tsx and replace the inline className logic with a conditional that adds disabledButtonClass when (isDebuggerProcessing || isCurrentBoardSimulator).
450-453:⚠️ Potential issue | 🟡 Minor
pendingSimulatorDebugRefcan stay true when Start aborts before firmware load.Line 450 sets the pending flag, but early-abort paths (e.g., validation/save exits) do not reliably clear it. Please clear it when compile does not actually start, not only in promise rejection handlers.
🔧 Proposed fix (explicit “compile started” contract)
- const handleRequest = () => { + const handleRequest = (): boolean => { @@ if (validationFailed) { setIsCompiling(false) - return + return false } @@ window.bridge.runCompileProgram( @@ ) + return true } - const verifyAndCompile = async () => { + const verifyAndCompile = async (): Promise<boolean> => { if (editingState === 'unsaved') { const res = await saveProject({ data: projectData, meta: projectMeta }, deviceDefinitions) if (!res.success) { - return + return false } - handleRequest() + return handleRequest() } else { - handleRequest() + return handleRequest() } } @@ - pendingSimulatorDebugRef.current = true - verifyAndCompile().catch(() => { - pendingSimulatorDebugRef.current = false - }) + pendingSimulatorDebugRef.current = true + const started = await verifyAndCompile().catch(() => false) + if (!started) { + pendingSimulatorDebugRef.current = false + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx` around lines 450 - 453, pendingSimulatorDebugRef.current is set true before calling verifyAndCompile(), but verifyAndCompile may early-abort (validation/save exits) and never actually start compilation, leaving the flag stuck; change the contract so verifyAndCompile() communicates whether compilation actually started (e.g., return Promise<boolean> or provide an onStart callback) and only set pendingSimulatorDebugRef.current = true when verifyAndCompile indicates compilation began (or clear it immediately if the returned value/flag is false); update the callsite around pendingSimulatorDebugRef and the verifyAndCompile implementation to use that boolean/onStart signal and keep the existing .catch() clearing as a fallback.
🤖 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/utils/debugger-session.ts`:
- Around line 26-32: Make disconnectDebugger perform cleanup regardless of
whether window.bridge.debuggerDisconnect() rejects: wrap the await
window.bridge.debuggerDisconnect() in a try/catch (or use try { await ... }
catch (e) { /* log via console or process logger but do not rethrow */ }) and
move the workspaceActions mutations (setDebuggerVisible, setDebuggerTargetIp,
setDebugForcedVariables, clearFbDebugContext) into a finally block (or after the
catch) so state is reset even if the bridge call fails; reference the
disconnectDebugger function and workspaceActions methods and ensure errors from
window.bridge.debuggerDisconnect are swallowed/logged but not propagated.
---
Duplicate comments:
In `@src/renderer/components/_organisms/workspace-activity-bar/default.tsx`:
- Line 1302: The DebuggerButton is using an inline className expression which
bypasses the shared disabled styles; update the DebuggerButton render to apply
the shared disabledButtonClass constant instead of the inline
"(isDebuggerProcessing || isCurrentBoardSimulator) && 'cursor-not-allowed
opacity-50'" expression so the button uses the same disabledButtonClass
(including hover suppression) as other disabled buttons—locate the
DebuggerButton JSX in default.tsx and replace the inline className logic with a
conditional that adds disabledButtonClass when (isDebuggerProcessing ||
isCurrentBoardSimulator).
- Around line 450-453: pendingSimulatorDebugRef.current is set true before
calling verifyAndCompile(), but verifyAndCompile may early-abort
(validation/save exits) and never actually start compilation, leaving the flag
stuck; change the contract so verifyAndCompile() communicates whether
compilation actually started (e.g., return Promise<boolean> or provide an
onStart callback) and only set pendingSimulatorDebugRef.current = true when
verifyAndCompile indicates compilation began (or clear it immediately if the
returned value/flag is false); update the callsite around
pendingSimulatorDebugRef and the verifyAndCompile implementation to use that
boolean/onStart signal and keep the existing .catch() clearing as a fallback.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/renderer/components/_organisms/workspace-activity-bar/default.tsxsrc/renderer/utils/debugger-session.ts
Comment on lines
+26
to
+32
| export async function disconnectDebugger(workspaceActions: WorkspaceActions): Promise<void> { | ||
| await window.bridge.debuggerDisconnect() | ||
| workspaceActions.setDebuggerVisible(false) | ||
| workspaceActions.setDebuggerTargetIp(null) | ||
| workspaceActions.setDebugForcedVariables(new Map()) | ||
| workspaceActions.clearFbDebugContext() | ||
| } |
Contributor
There was a problem hiding this comment.
Make disconnect cleanup best-effort even when bridge disconnect fails.
If window.bridge.debuggerDisconnect() rejects, cleanup state mutations are skipped. That can leave debugger UI/state stale and cause unhandled promise rejections in fire-and-forget call sites.
🔧 Proposed fix
export async function disconnectDebugger(workspaceActions: WorkspaceActions): Promise<void> {
- await window.bridge.debuggerDisconnect()
- workspaceActions.setDebuggerVisible(false)
- workspaceActions.setDebuggerTargetIp(null)
- workspaceActions.setDebugForcedVariables(new Map())
- workspaceActions.clearFbDebugContext()
+ try {
+ await window.bridge.debuggerDisconnect()
+ } catch {
+ // best-effort disconnect: still clear local debugger state
+ } finally {
+ workspaceActions.setDebuggerVisible(false)
+ workspaceActions.setDebuggerTargetIp(null)
+ workspaceActions.setDebugForcedVariables(new Map())
+ workspaceActions.clearFbDebugContext()
+ }
}📝 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
| export async function disconnectDebugger(workspaceActions: WorkspaceActions): Promise<void> { | |
| await window.bridge.debuggerDisconnect() | |
| workspaceActions.setDebuggerVisible(false) | |
| workspaceActions.setDebuggerTargetIp(null) | |
| workspaceActions.setDebugForcedVariables(new Map()) | |
| workspaceActions.clearFbDebugContext() | |
| } | |
| export async function disconnectDebugger(workspaceActions: WorkspaceActions): Promise<void> { | |
| try { | |
| await window.bridge.debuggerDisconnect() | |
| } catch { | |
| // best-effort disconnect: still clear local debugger state | |
| } finally { | |
| workspaceActions.setDebuggerVisible(false) | |
| workspaceActions.setDebuggerTargetIp(null) | |
| workspaceActions.setDebugForcedVariables(new Map()) | |
| workspaceActions.clearFbDebugContext() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/utils/debugger-session.ts` around lines 26 - 32, Make
disconnectDebugger perform cleanup regardless of whether
window.bridge.debuggerDisconnect() rejects: wrap the await
window.bridge.debuggerDisconnect() in a try/catch (or use try { await ... }
catch (e) { /* log via console or process logger but do not rethrow */ }) and
move the workspaceActions mutations (setDebuggerVisible, setDebuggerTargetIp,
setDebugForcedVariables, clearFbDebugContext) into a finally block (or after the
catch) so state is reset even if the bridge call fails; reference the
disconnectDebugger function and workspaceActions methods and ensure errors from
window.bridge.debuggerDisconnect are swallowed/logged but not propagated.
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
compileForDebuggeror performs MD5 verification for the simulator target, since the firmware was just built and loaded in the same action.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes