sync(ai): mirror web AI feature improvements (paired PR for DOPE #375)#739
Conversation
Paired PR for Autonomy-Logic/openplc-web#375. Mirrors shared-surface changes from the web repo: - Per-hunk Accept/Reject UI with brand colors (Monaco overlay) - Orphan cleanup on Undo: snapshot/restore tabs, editors, flows, libraries, files - Data-type tools: update_datatype, delete_datatype + fix create_datatype field propagation - Variable sync fix: use variable name instead of undefined id - Per-POU pending diff state lifted to AI slice (survives tab switches) - Disable inline suggestions toggle + settings popover - Hardcoded model selection (sonnet for chat, haiku for inline) - Agentic loop iteration cap raised from 8 to 16 - Workspace resize handle moved out of nested panel Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughThis PR refactors AI diff-review functionality by: (1) renaming accept/reject UI actions and optimizing button positioning with consolidated scroll handling, (2) migrating diff-review state from local component state to the global store with new pending diff management actions, (3) adding editor instance tracking to ensure correct diff-review reattachment after tab switching, and (4) introducing inline completion preferences while improving error handling in variable deletion. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/frontend/store/__tests__/project-slice.test.ts (1)
775-783: Stale test description/assertion for the renamedfailcontract.The sibling tests for "variable not found" cases were renamed to "returns fail" and now assert
result.ok === false(Lines 785–792, 2176–2184, 2555–2564). This local-scopevariableNamecase follows the same new code path (fail('Variable ... not found')at Line 513 ofslice.ts) but the title still says "does nothing" and only checks state length. Tighten it for consistency and to lock in the new contract.🧪 Proposed fix
- it('does nothing when variableName not found', () => { + it('returns fail when variableName not found (local scope)', () => { seedPou(store, makePou('Main', 'program', [makeVariable('x')])) - store.getState().projectActions.deleteVariable({ + const result = store.getState().projectActions.deleteVariable({ scope: 'local', associatedPou: 'Main', variableName: 'nonexistent', }) + expect(result.ok).toBe(false) expect(store.getState().project.data.pous[0].interface?.variables).toHaveLength(1) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/store/__tests__/project-slice.test.ts` around lines 775 - 783, The test currently titled "does nothing when variableName not found" should be updated to reflect the new fail contract used by projectActions.deleteVariable: rename the test title to "returns fail when variableName not found" and assert the action result indicates failure (e.g., result.ok === false and result.error/message matches "Variable ... not found" or contains 'Variable' and 'not found') instead of only checking the variables length; target the call to store.getState().projectActions.deleteVariable and verify the returned result object conforms to the fail contract used in slice.ts.src/frontend/store/__tests__/ai-slice.test.ts (1)
348-357: Consider adding coverage forsetPendingDiffoverwriting an existing entry.The test at Line 348-351 covers initial write, and Line 353-357 covers multi-POU coexistence, but there's no explicit case for "same POU key, called twice" — which the slice's summary calls out as expected behavior ("creates new per-POU
DiffReviewEntryentries or overwrites existing ones"). A quick assertion that a secondsetPendingDiff('main', different)fully replaces the first would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/store/__tests__/ai-slice.test.ts` around lines 348 - 357, Add a test that verifies setPendingDiff overwrites an existing entry: call store.getState().aiActions.setPendingDiff('main', sampleEntry), then call setPendingDiff('main', a different entry e.g. with a changed oldBody), and assert that store.getState().ai.pendingDiffs.main strictly equals the second entry (and not the first); reference the aiActions.setPendingDiff method and the ai.pendingDiffs map to locate where to add this assertion.src/frontend/components/_features/[workspace]/editor/monaco/index.tsx (2)
829-854: DoubleuseOpenPLCStore()subscription.
aiState = useOpenPLCStore().aiis a second store subscription in the same component that already destructuresaiat Line 146/164. Every unrelated AI state change (messages, chat open, loading, etc.) now re-renders this heavy Monaco component because the selector returns the full root state. Consider folding theaiState.preferences.inlineCompletionsEnabled,aiState.isEnabled, andaiState.hasConsentedreads into the top-level destructure or use a narrow selector.♻️ Proposed: consolidate the subscription
- const aiState = useOpenPLCStore().ai - useEffect(() => { if (!capabilities.hasAIAssistant) return - if (!aiState.isEnabled) return - if (!aiState.hasConsented) return - if (!aiState.preferences.inlineCompletionsEnabled) return + if (!aiIsEnabled) return + if (!aiHasConsented) return + if (!aiInlineCompletionsEnabled) return…and pull
aiIsEnabled,aiHasConsented, andaiInlineCompletionsEnabledout of the top-leveluseOpenPLCStore()destructure alongsideai: { pendingDiffs }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/editor/monaco/index.tsx around lines 829 - 854, The component currently calls useOpenPLCStore() twice (assigning aiState = useOpenPLCStore().ai) causing an extra subscription; replace this by pulling ai.isEnabled, ai.hasConsented, and ai.preferences.inlineCompletionsEnabled out of the existing top-level useOpenPLCStore() destructure (or switch to a narrow selector) so the component only subscribes once; update any references to aiState.isEnabled, aiState.hasConsented, and aiState.preferences.inlineCompletionsEnabled to use the new aiIsEnabled/aiHasConsented/aiInlineCompletionsEnabled variables (or the narrow selector names) and remove the aiState variable and its useOpenPLCStore() call.
219-231:clearPendingDiffduring effect body triggers an extra render.When
entry.acceptedHunksbecomes empty, Line 229 dispatchesclearPendingDiff(name)inside the effect, which synchronously updates Zustand state and triggers another render of this component (and the re-run of this effect). On the second runentryis undefined so it bails at Line 224 — correct, but it's an avoidable round-trip. A more idiomatic approach is forhandleKeepHunk/handleUndoHunkto own the "clear when last hunk resolved" transition (which they already do) and let this effect's guard be purely "no entry → nothing to render". Worth reconsidering whether this branch is reachable at all in steady state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/editor/monaco/index.tsx around lines 219 - 231, The effect currently calls clearPendingDiff(name) when there are no pendingHunks which causes an extra synchronous Zustand state update and rerender; remove the clearPendingDiff(name) call from the useEffect in the monaco editor and simplify the guard to bail out when there's no entry (e.g., if (!entry) return), letting the existing handleKeepHunk/handleUndoHunk functions own the “clear when last hunk resolved” transition (verify those handlers already clear the entry when appropriate).src/frontend/store/slices/ai/types.ts (1)
24-39: MisleadingacceptedHunksfield name retained.The documentation acknowledges that
acceptedHunksactually holds the IDs of hunks that are still pending (not accepted). Carrying a historical misnomer into a freshly introduced type (which didn't exist before this lift-to-store) is a nice opportunity to rename to e.g.pendingHunkIds. All callers in this PR are new, so the blast radius is limited to this slice +monaco/index.tsx+ the new tests. Worth considering while the surface is fresh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/store/slices/ai/types.ts` around lines 24 - 39, Rename the misleading DiffReviewEntry field acceptedHunks to pendingHunkIds and update all usages accordingly: change the type declaration in DiffReviewEntry (acceptedHunks -> pendingHunkIds: string[]), update any code in monaco/index.tsx that references handleKeepHunk/handleUndoHunk to read/write pendingHunkIds, and adjust the tests and any selectors/serializers in this slice to use pendingHunkIds so semantics match the comment and callers.src/frontend/components/_features/[workspace]/editor/monaco/ai-diff-review.ts (1)
82-90: Stale-container cleanup is too broad if multiple overlays coexist.
editorDom.querySelectorAll('.ai-hunk-buttons').forEach((el) => el.remove())wipes every container with this class in the editor's DOM. IfrenderDiffReviewis ever invoked before a previous caller's cleanup fn runs (e.g. effect re-run race on remount), this defensive sweep also removes the in-flight overlay owned by the previouscleanupclosure — and the previous cleanup will then no-op those already-removed containers. Not a bug in the common path, but consider scoping the cleanup to this call'sbuttonContainersarray only, and treating leftover orphans as the caller's responsibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/editor/monaco/ai-diff-review.ts around lines 82 - 90, The global DOM sweep using editorDom.querySelectorAll('.ai-hunk-buttons') is too broad; restrict cleanup to this renderDiffReview call's own containers by iterating the buttonContainers array and removing each element if it still exists in the DOM (e.g., check el.parentNode before remove), instead of removing all elements by class name; keep the existing defensive behavior for orphans but don’t clear overlays owned by other callers—use the local buttonContainers variable (and ensure it’s initialized before this cleanup) to scope removals.
🤖 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/frontend/components/_features/`[workspace]/editor/monaco/ai-diff-review.ts:
- Around line 145-168: In layoutButtons(), the visibility check uses the
unadjusted natural position so collision-adjusted top can end up off-screen;
instead compute the adjusted top = Math.max(natural, lastBottom + MIN_GAP), then
clamp or test visibility against editorHeight - BUTTON_HEIGHT and 0 using that
adjusted top (e.g., if adjustedTop < 0 || adjustedTop > editorHeight -
BUTTON_HEIGHT then hide the container), otherwise set container.style.display
and container.style.top to the adjusted/clamped value and update lastBottom =
adjustedTop + BUTTON_HEIGHT; reference layoutButtons, natural, lastBottom,
MIN_GAP, BUTTON_HEIGHT, editorHeight, sortedHunks and buttonContainers.
In `@src/frontend/store/slices/project/slice.ts`:
- Around line 498-528: deleteVariable now returns a ProjectResponse indicating
failure in several previously-silent cases, but all 11 callers ignore that
return value; update each caller (e.g., variables-editor/index.tsx,
global-variables-editor/index.tsx, delete-confirmation-modal.tsx,
ladder-toolbox.tsx, graphical-editor/fbd/index.tsx,
graphical-editor/ladder/rung/body.tsx, and the four block editor locations) to
capture the return from deleteVariable, check response.ok, and if false show an
error toast (or otherwise surface response message) and abort further UI
changes/flows instead of proceeding unconditionally; ensure you reference the
deleteVariable call site in each file, use the ProjectResponse fields
(ok/message) to decide behavior, and stop execution when deletion failed.
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/editor/monaco/ai-diff-review.ts:
- Around line 82-90: The global DOM sweep using
editorDom.querySelectorAll('.ai-hunk-buttons') is too broad; restrict cleanup to
this renderDiffReview call's own containers by iterating the buttonContainers
array and removing each element if it still exists in the DOM (e.g., check
el.parentNode before remove), instead of removing all elements by class name;
keep the existing defensive behavior for orphans but don’t clear overlays owned
by other callers—use the local buttonContainers variable (and ensure it’s
initialized before this cleanup) to scope removals.
In `@src/frontend/components/_features/`[workspace]/editor/monaco/index.tsx:
- Around line 829-854: The component currently calls useOpenPLCStore() twice
(assigning aiState = useOpenPLCStore().ai) causing an extra subscription;
replace this by pulling ai.isEnabled, ai.hasConsented, and
ai.preferences.inlineCompletionsEnabled out of the existing top-level
useOpenPLCStore() destructure (or switch to a narrow selector) so the component
only subscribes once; update any references to aiState.isEnabled,
aiState.hasConsented, and aiState.preferences.inlineCompletionsEnabled to use
the new aiIsEnabled/aiHasConsented/aiInlineCompletionsEnabled variables (or the
narrow selector names) and remove the aiState variable and its useOpenPLCStore()
call.
- Around line 219-231: The effect currently calls clearPendingDiff(name) when
there are no pendingHunks which causes an extra synchronous Zustand state update
and rerender; remove the clearPendingDiff(name) call from the useEffect in the
monaco editor and simplify the guard to bail out when there's no entry (e.g., if
(!entry) return), letting the existing handleKeepHunk/handleUndoHunk functions
own the “clear when last hunk resolved” transition (verify those handlers
already clear the entry when appropriate).
In `@src/frontend/store/__tests__/ai-slice.test.ts`:
- Around line 348-357: Add a test that verifies setPendingDiff overwrites an
existing entry: call store.getState().aiActions.setPendingDiff('main',
sampleEntry), then call setPendingDiff('main', a different entry e.g. with a
changed oldBody), and assert that store.getState().ai.pendingDiffs.main strictly
equals the second entry (and not the first); reference the
aiActions.setPendingDiff method and the ai.pendingDiffs map to locate where to
add this assertion.
In `@src/frontend/store/__tests__/project-slice.test.ts`:
- Around line 775-783: The test currently titled "does nothing when variableName
not found" should be updated to reflect the new fail contract used by
projectActions.deleteVariable: rename the test title to "returns fail when
variableName not found" and assert the action result indicates failure (e.g.,
result.ok === false and result.error/message matches "Variable ... not found" or
contains 'Variable' and 'not found') instead of only checking the variables
length; target the call to store.getState().projectActions.deleteVariable and
verify the returned result object conforms to the fail contract used in
slice.ts.
In `@src/frontend/store/slices/ai/types.ts`:
- Around line 24-39: Rename the misleading DiffReviewEntry field acceptedHunks
to pendingHunkIds and update all usages accordingly: change the type declaration
in DiffReviewEntry (acceptedHunks -> pendingHunkIds: string[]), update any code
in monaco/index.tsx that references handleKeepHunk/handleUndoHunk to read/write
pendingHunkIds, and adjust the tests and any selectors/serializers in this slice
to use pendingHunkIds so semantics match the comment and callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8b174828-29dd-4bbc-8488-5e1d30594f49
📒 Files selected for processing (10)
src/frontend/components/_features/[workspace]/editor/monaco/ai-diff-review.tssrc/frontend/components/_features/[workspace]/editor/monaco/index.tsxsrc/frontend/screens/workspace-screen.tsxsrc/frontend/store/__tests__/ai-slice.test.tssrc/frontend/store/__tests__/project-slice.test.tssrc/frontend/store/slices/ai/index.tssrc/frontend/store/slices/ai/slice.tssrc/frontend/store/slices/ai/types.tssrc/frontend/store/slices/project/slice.tssrc/middleware/shared/ports/types.ts
| const layoutButtons = () => { | ||
| const scrollTop = editor.getScrollTop() | ||
| const editorHeight = editor.getLayoutInfo().height | ||
| let lastBottom = -Infinity | ||
| sortedHunks.forEach((hunk, idx) => { | ||
| const container = buttonContainers[idx] | ||
| const natural = editor.getTopForLineNumber(hunk.startLine) - scrollTop | ||
|
|
||
| // Hide buttons whose anchor line is outside the visible editor area — otherwise | ||
| // hunks that scroll off the top would clamp to top:0 and pile up at the viewport edge. | ||
| if (natural < 0 || natural > editorHeight - BUTTON_HEIGHT) { | ||
| container.style.display = 'none' | ||
| return | ||
| } | ||
|
|
||
| container.style.display = 'inline-flex' | ||
| const top = Math.max(natural, lastBottom + MIN_GAP) | ||
| container.style.top = `${top}px` | ||
| lastBottom = top + BUTTON_HEIGHT | ||
| }) | ||
|
|
||
| // Store disposable for cleanup | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| ;(container as any)._scrollDisposable = scrollDisposable | ||
| } | ||
|
|
||
| layoutButtons() | ||
| scrollDisposables.push(editor.onDidScrollChange(() => layoutButtons())) |
There was a problem hiding this comment.
Collision offset can push buttons outside the viewport without hiding them.
The visibility check at Line 155 uses natural (unadjusted scroll-relative position), but the applied top at Line 161 may have been pushed down by lastBottom + MIN_GAP when adjacent hunks collide. For closely-spaced hunks near the bottom of the viewport, natural can be visible while the bumped top is below editorHeight - BUTTON_HEIGHT, so buttons render off-screen.
Consider clamping or hiding based on the final top instead:
🩹 Proposed fix
sortedHunks.forEach((hunk, idx) => {
const container = buttonContainers[idx]
const natural = editor.getTopForLineNumber(hunk.startLine) - scrollTop
- // Hide buttons whose anchor line is outside the visible editor area — otherwise
- // hunks that scroll off the top would clamp to top:0 and pile up at the viewport edge.
- if (natural < 0 || natural > editorHeight - BUTTON_HEIGHT) {
+ if (natural < 0 || natural > editorHeight - BUTTON_HEIGHT) {
container.style.display = 'none'
return
}
- container.style.display = 'inline-flex'
const top = Math.max(natural, lastBottom + MIN_GAP)
+ if (top > editorHeight - BUTTON_HEIGHT) {
+ container.style.display = 'none'
+ return
+ }
+ container.style.display = 'inline-flex'
container.style.top = `${top}px`
lastBottom = top + BUTTON_HEIGHT
})📝 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.
| const layoutButtons = () => { | |
| const scrollTop = editor.getScrollTop() | |
| const editorHeight = editor.getLayoutInfo().height | |
| let lastBottom = -Infinity | |
| sortedHunks.forEach((hunk, idx) => { | |
| const container = buttonContainers[idx] | |
| const natural = editor.getTopForLineNumber(hunk.startLine) - scrollTop | |
| // Hide buttons whose anchor line is outside the visible editor area — otherwise | |
| // hunks that scroll off the top would clamp to top:0 and pile up at the viewport edge. | |
| if (natural < 0 || natural > editorHeight - BUTTON_HEIGHT) { | |
| container.style.display = 'none' | |
| return | |
| } | |
| container.style.display = 'inline-flex' | |
| const top = Math.max(natural, lastBottom + MIN_GAP) | |
| container.style.top = `${top}px` | |
| lastBottom = top + BUTTON_HEIGHT | |
| }) | |
| // Store disposable for cleanup | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| ;(container as any)._scrollDisposable = scrollDisposable | |
| } | |
| layoutButtons() | |
| scrollDisposables.push(editor.onDidScrollChange(() => layoutButtons())) | |
| const layoutButtons = () => { | |
| const scrollTop = editor.getScrollTop() | |
| const editorHeight = editor.getLayoutInfo().height | |
| let lastBottom = -Infinity | |
| sortedHunks.forEach((hunk, idx) => { | |
| const container = buttonContainers[idx] | |
| const natural = editor.getTopForLineNumber(hunk.startLine) - scrollTop | |
| if (natural < 0 || natural > editorHeight - BUTTON_HEIGHT) { | |
| container.style.display = 'none' | |
| return | |
| } | |
| const top = Math.max(natural, lastBottom + MIN_GAP) | |
| if (top > editorHeight - BUTTON_HEIGHT) { | |
| container.style.display = 'none' | |
| return | |
| } | |
| container.style.display = 'inline-flex' | |
| container.style.top = `${top}px` | |
| lastBottom = top + BUTTON_HEIGHT | |
| }) | |
| } | |
| layoutButtons() | |
| scrollDisposables.push(editor.onDidScrollChange(() => layoutButtons())) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/components/_features/`[workspace]/editor/monaco/ai-diff-review.ts
around lines 145 - 168, In layoutButtons(), the visibility check uses the
unadjusted natural position so collision-adjusted top can end up off-screen;
instead compute the adjusted top = Math.max(natural, lastBottom + MIN_GAP), then
clamp or test visibility against editorHeight - BUTTON_HEIGHT and 0 using that
adjusted top (e.g., if adjustedTop < 0 || adjustedTop > editorHeight -
BUTTON_HEIGHT then hide the container), otherwise set container.style.display
and container.style.top to the adjusted/clamped value and update lastBottom =
adjustedTop + BUTTON_HEIGHT; reference layoutButtons, natural, lastBottom,
MIN_GAP, BUTTON_HEIGHT, editorHeight, sortedHunks and buttonContainers.
| const handleUndoHunk = (hunkId: string) => { | ||
| // "Undo" = reject this hunk, revert those lines to old version | ||
| setDiffReview((prev) => { | ||
| if (!prev) return prev | ||
| const newAccepted = new Set(prev.acceptedHunks) | ||
| newAccepted.delete(hunkId) | ||
|
|
||
| // Rebuild body: accepted hunks keep new code, this rejected hunk keeps old code | ||
| const keptIds = new Set<string>() | ||
| for (const h of prev.hunks) { | ||
| if (h.id === hunkId) continue // This one is undone | ||
| if (!newAccepted.has(h.id)) { | ||
| // Already resolved as kept | ||
| keptIds.add(h.id) | ||
| } else { | ||
| // Still pending — treat as kept for now (new code) | ||
| keptIds.add(h.id) | ||
| } | ||
| } | ||
|
|
||
| const newBody = applyAcceptedHunks(prev.oldBody, prev.newBody, prev.hunks, keptIds) | ||
|
|
||
| // Update editor model with rebuilt body | ||
| const model = editor.getModel() | ||
| if (model) { | ||
| isSyncingModelRef.current = true | ||
| const fullRange = model.getFullModelRange() | ||
| editor.executeEdits('ai-diff-undo-hunk', [{ range: fullRange, text: newBody }]) | ||
| isSyncingModelRef.current = false | ||
| } | ||
| setLocalText(newBody) | ||
|
|
||
| // Update store | ||
| const state = openPLCStoreBase.getState() | ||
| state.projectActions.updatePou({ name, content: { language, value: newBody } }) | ||
|
|
||
| // Recompute hunks for remaining pending changes | ||
| const remainingHunks = prev.hunks.filter((h) => newAccepted.has(h.id)) | ||
| if (remainingHunks.length === 0) return null | ||
| const state = openPLCStoreBase.getState() | ||
| const current = state.ai.pendingDiffs[name] | ||
| if (!current) return | ||
|
|
||
| // Rebuild body: every hunk except this one is treated as "kept" (new code), | ||
| // the rejected one reverts to the old text. | ||
| const keptIds = new Set(current.hunks.filter((h) => h.id !== hunkId).map((h) => h.id)) | ||
| const newBody = applyAcceptedHunks(current.oldBody, current.newBody, current.hunks, keptIds) | ||
|
|
||
| // Update editor model with rebuilt body | ||
| const model = editor.getModel() | ||
| if (model) { | ||
| isSyncingModelRef.current = true | ||
| const fullRange = model.getFullModelRange() | ||
| editor.executeEdits('ai-diff-undo-hunk', [{ range: fullRange, text: newBody }]) | ||
| isSyncingModelRef.current = false | ||
| } | ||
| setLocalText(newBody) | ||
|
|
||
| // Recompute line positions for remaining hunks | ||
| const freshHunks = computeHunks(prev.oldBody, newBody) | ||
| const freshAccepted = new Set(freshHunks.map((h) => h.id)) | ||
| // Propagate to project slice | ||
| state.projectActions.updatePou({ name, content: { language, value: newBody } }) | ||
|
|
||
| if (freshHunks.length === 0) return null | ||
| return { ...prev, newBody, hunks: freshHunks, acceptedHunks: freshAccepted } | ||
| // Recompute hunks for remaining pending changes, against the new body. | ||
| const freshHunks = computeHunks(current.oldBody, newBody) | ||
| if (freshHunks.length === 0) { | ||
| clearPendingDiff(name) | ||
| return | ||
| } | ||
| updatePendingDiff(name, { | ||
| newBody, | ||
| hunks: freshHunks, | ||
| acceptedHunks: freshHunks.map((h) => h.id), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Rejecting a hunk discards the user's prior Accept decisions.
In handleUndoHunk, computeHunks(current.oldBody, newBody) regenerates hunks with fresh UUIDs, then acceptedHunks: freshHunks.map(h => h.id) marks every remaining hunk as pending again. If the user had already Accepted some hunks in the same review session, those decisions are lost and the buttons reappear on lines the user had already resolved.
If this is intentional (a Reject "resets" the review for all remaining hunks), consider making it explicit in the doc comment above. Otherwise, the accepted set should be reconstructed by remapping surviving hunks (e.g., by (startLine, type) signature) from current.acceptedHunks rather than resetting to "all pending".
Given acceptedHunks historically means "still pending" (per the comment in types.ts Line 29-32), verify the intended semantics before landing.
| let response: ProjectResponse = { ok: true } | ||
| setState( | ||
| produce((slice: ProjectSlice) => { | ||
| const variables = | ||
| scope === 'local' && associatedPou | ||
| ? slice.project.data.pous.find((p) => p.name === associatedPou)?.interface?.variables | ||
| : slice.project.data.configurations.resource.globalVariables | ||
| if (!variables) return | ||
| if (!variables) { | ||
| response = fail('Variable container not found') | ||
| return | ||
| } | ||
|
|
||
| if (variableName) { | ||
| const idx = variables.findIndex((v) => v.name.toLowerCase() === variableName.toLowerCase()) | ||
| if (idx !== -1) variables.splice(idx, 1) | ||
| if (idx === -1) { | ||
| response = fail(`Variable "${variableName}" not found`, 'Variable not found') | ||
| return | ||
| } | ||
| variables.splice(idx, 1) | ||
| return | ||
| } | ||
| const found = getVariableBasedOnRowIdOrVariableId(variables, rowId, variableId) | ||
| if (found) variables.splice(found.index, 1) | ||
| if (!found) { | ||
| response = fail('Variable not found') | ||
| return | ||
| } | ||
| variables.splice(found.index, 1) | ||
| }), | ||
| ) | ||
| return ok() | ||
| return response | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every call site of deleteVariable and show surrounding context so we can confirm
# whether the caller inspects the ProjectResponse.
rg -nP --type=ts --type=tsx -C5 '\bdeleteVariable\s*\(' -g '!**/__tests__/**' -g '!**/*.test.*'Repository: Autonomy-Logic/openplc-editor
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Search for deleteVariable calls with proper file type handling
rg -nP --type=ts -C5 'deleteVariable\s*\(' -g '!**/__tests__/**' -g '!**/*.test.*'Repository: Autonomy-Logic/openplc-editor
Length of output: 13429
🏁 Script executed:
#!/bin/bash
# Also search JavaScript-like patterns without type restriction as backup
rg -nP -C5 'deleteVariable\s*\(' --glob='*.ts' --glob='*.tsx' -g '!**/__tests__/**' -g '!**/*.test.*'Repository: Autonomy-Logic/openplc-editor
Length of output: 13429
🏁 Script executed:
#!/bin/bash
# Check the variables-editor deletion handler more carefully
sed -n '405,425p' src/frontend/components/_organisms/variables-editor/index.tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 736
🏁 Script executed:
#!/bin/bash
# Check the global-variables-editor deletion handler more carefully
sed -n '265,280p' src/frontend/components/_organisms/global-variables-editor/index.tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 640
All 11 call sites discard the return value—no regressions, but error handling is incomplete.
The behavioral shift is safe: deleteVariable now returns fail(...) for three previously-silent cases, but since all call sites already ignore the return value, this doesn't regress existing behavior. However, this means none of the 11 callers can surface the new failure messages to users.
Update call sites in:
variables-editor/index.tsx(Line 413)global-variables-editor/index.tsx(Line 272)delete-confirmation-modal.tsx(Line 119)ladder-toolbox.tsx(Line 92)graphical-editor/fbd/index.tsx(Line 493)graphical-editor/ladder/rung/body.tsx(Lines 578, 590)- Block editors (4 more locations)
Each should capture the response, check ok, and display error toasts when deletion fails instead of proceeding unconditionally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/store/slices/project/slice.ts` around lines 498 - 528,
deleteVariable now returns a ProjectResponse indicating failure in several
previously-silent cases, but all 11 callers ignore that return value; update
each caller (e.g., variables-editor/index.tsx,
global-variables-editor/index.tsx, delete-confirmation-modal.tsx,
ladder-toolbox.tsx, graphical-editor/fbd/index.tsx,
graphical-editor/ladder/rung/body.tsx, and the four block editor locations) to
capture the return from deleteVariable, check response.ok, and if false show an
error toast (or otherwise surface response message) and abort further UI
changes/flows instead of proceeding unconditionally; ensure you reference the
deleteVariable call site in each file, use the ProjectResponse fields
(ok/message) to decide behavior, and stop execution when deletion failed.
1 participant
Summary
Paired PR for Autonomy-Logic/openplc-web#375. Mirrors the shared-surface changes so the cross-repo Surface Sync check passes.
Test plan
Paired with
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style