refactor(ai): sync shared surfaces for AI chat enhancements#732
Conversation
Sync byte-identical surfaces from openplc-web refactor/ai-chat-pr353: - Store: project-scoped flat messages[] replacing per-POU conversations, add isAgenticLoopRunning, increase MAX_MESSAGES to 50 - Monaco: replace DiffEditor with ai-pou-updated event handler - Architecture: add adapter-components layer with frontend-level permissions - CSS: add AI diff review decoration styles - Tests: update AI slice tests for new schema All AI feature logic remains web-only (in adapters). These shared surface changes keep store types, components, and validation in sync. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant AI as AI Service
participant Editor as Monaco Editor
participant Utils as Diff Utils
participant UI as Diff Review UI
participant Store as Frontend Store
AI->>Editor: emit `ai-pou-updated` (oldBody?, body)
alt no oldBody
Editor->>Editor: executeEdits('ai-tool-update', body)
Editor->>Store: update localText / updatePou(body)
else oldBody present
Editor->>Utils: computeHunks(oldBody, body)
Utils-->>Editor: hunks[]
Editor->>UI: renderDiffReview(editor, hunks, onKeep, onUndo)
UI-->>Editor: add decorations, viewZones, Keep/Undo buttons
Note over UI,Editor: User clicks Keep/Undo per hunk
UI->>Editor: onKeep(hunkId) / onUndo(hunkId)
Editor->>Store: update acceptedHunks set
Editor->>Utils: applyAcceptedHunks(oldBody, body, hunks, acceptedIds)
Utils-->>Editor: newBody
Editor->>Editor: executeEdits('ai-tool-update', newBody)
Editor->>Store: updatePou(newBody)
end
UI->>UI: cleanup when no pending hunks remain
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/__architecture__/validate.ts (2)
67-70: Nit:adapterslayernameis narrower than whatgetLayer()actually classifies.The updated label claims this layer covers
middleware/adapters/**/services/, middleware/adapters/*.ts, but the classifier on line 152 (rel.startsWith('middleware/adapters/')) still catches any non-components/path under an adapter (e.g.,middleware/adapters/<adapter>/utils/foo.ts, top-level adapter files, etc.), not onlyservices/. When a violation is reported for such a file, the error message will misrepresent where it lives. Consider either tighteninggetLayer()to only matchservices/+ top-level adapter files, or broadening the name back to reflect the actual catch-all scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__architecture__/validate.ts` around lines 67 - 70, The adapters layer label is narrower than the current classifier: getLayer() uses rel.startsWith('middleware/adapters/') and therefore matches any file under that directory (not just services/), so either (A) broaden the adapters.name to accurately describe the catch-all scope or (B) tighten getLayer() to only classify service files and top-level adapter modules by changing the condition that uses rel.startsWith('middleware/adapters/') to require either a '/services/' segment or a top-level adapter filename pattern (e.g., match 'middleware/adapters/*.ts'); update the adapters.name or the getLayer() matching logic accordingly so the reported layer and classifier stay consistent.
71-86: LGTM on the newadapter-componentslayer rule.Ordering in
getLayer()(line 151 before line 152) correctly routesmiddleware/adapters/<adapter>/components/**into this layer before the broaderadaptersfallback. TheallowedDepsset mirrors thecomponentslayer plusadapters, which is consistent with an adapter-level UI layer composing frontend components and calling into adapter services.Two small, optional observations:
- Listing
'adapter-components'in its ownallowedDepsis redundant given thefromLayer !== toLayershort-circuit on line 322 — harmless, just noise.adapter-componentsintentionally cannot import fromtypes/, matching thecomponentslayer. Worth double-checking that any shared Zod schemas used by adapter components are re-exported throughstore/utilsrather than imported fromtypes/directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__architecture__/validate.ts` around lines 71 - 86, Remove the redundant self-reference in the 'adapter-components' rule's allowedDeps array in validate.ts: drop the 'adapter-components' entry (it’s unnecessary because getLayer() plus the fromLayer !== toLayer short-circuit prevents same-layer checks) and run tests; additionally, verify any shared Zod schemas used by middleware/adapters/<adapter>/components are re-exported through allowed layers like store or utils (not imported directly from types) to preserve the intended layering constraints.src/frontend/store/slices/ai/types.ts (1)
69-69: Optional: constant name no longer matches the model.After the move to a flat project-scoped list,
MAX_CONVERSATION_MESSAGESreads as stale (there are no conversations anymore). Consider renaming toMAX_MESSAGESin a follow-up — keep it in lockstep with openplc-web to preserve the byte-identical contract.🤖 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` at line 69, Rename the stale constant MAX_CONVERSATION_MESSAGES to MAX_MESSAGES and update all references/usages (imports, selectors, reducers, tests) to the new symbol (e.g., replace MAX_CONVERSATION_MESSAGES with MAX_MESSAGES in the ai slice and any consumers); ensure the exported name and value remain unchanged except for the identifier so the runtime behavior is identical and keep this change in sync (byte-identical contract) with openplc-web.
🤖 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/index.tsx:
- Around line 946-961: The handlers handlePouUpdated, handleAcceptAllHunks, and
handleRejectAllHunks close over the prop name and can act on a stale POU when
the editor persists; update each handler to read the current POU name from
openPLCStoreBase.getState().editor.meta.name (same pattern as
handleInsertAtCursor) instead of using the closed-over name, so the check
against targetPou uses the runtime store value before applying edits via
editorInstance, toggling isSyncingModelRef.current, and calling setLocalText as
needed.
---
Nitpick comments:
In `@src/__architecture__/validate.ts`:
- Around line 67-70: The adapters layer label is narrower than the current
classifier: getLayer() uses rel.startsWith('middleware/adapters/') and therefore
matches any file under that directory (not just services/), so either (A)
broaden the adapters.name to accurately describe the catch-all scope or (B)
tighten getLayer() to only classify service files and top-level adapter modules
by changing the condition that uses rel.startsWith('middleware/adapters/') to
require either a '/services/' segment or a top-level adapter filename pattern
(e.g., match 'middleware/adapters/*.ts'); update the adapters.name or the
getLayer() matching logic accordingly so the reported layer and classifier stay
consistent.
- Around line 71-86: Remove the redundant self-reference in the
'adapter-components' rule's allowedDeps array in validate.ts: drop the
'adapter-components' entry (it’s unnecessary because getLayer() plus the
fromLayer !== toLayer short-circuit prevents same-layer checks) and run tests;
additionally, verify any shared Zod schemas used by
middleware/adapters/<adapter>/components are re-exported through allowed layers
like store or utils (not imported directly from types) to preserve the intended
layering constraints.
In `@src/frontend/store/slices/ai/types.ts`:
- Line 69: Rename the stale constant MAX_CONVERSATION_MESSAGES to MAX_MESSAGES
and update all references/usages (imports, selectors, reducers, tests) to the
new symbol (e.g., replace MAX_CONVERSATION_MESSAGES with MAX_MESSAGES in the ai
slice and any consumers); ensure the exported name and value remain unchanged
except for the identifier so the runtime behavior is identical and keep this
change in sync (byte-identical contract) with openplc-web.
🪄 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: 1d24350a-0a69-46ab-b643-ac9820bd63e9
📒 Files selected for processing (7)
src/__architecture__/validate.tssrc/backend/shared/styles/globals.csssrc/frontend/components/_features/[workspace]/editor/monaco/index.tsxsrc/frontend/store/__tests__/ai-slice.test.tssrc/frontend/store/slices/ai/index.tssrc/frontend/store/slices/ai/slice.tssrc/frontend/store/slices/ai/types.ts
| // Listen for AI tool updates — sync editor model with new body | ||
| const handlePouUpdated = (e: Event) => { | ||
| const { pouName: targetPou, body } = (e as CustomEvent<{ pouName: string; body: string; oldBody?: string }>) | ||
| .detail | ||
| if (targetPou !== name) return | ||
| setDiffReview({ active: true, proposedBody, variableSummary }) | ||
| } | ||
| window.addEventListener('ai-review-code', handleCodeReview) | ||
|
|
||
| // Listen for AI chat "apply code" — applies after the user accepted the diff | ||
| const handleCodeApplied = (e: Event) => { | ||
| const { pouName: targetPou, body } = (e as CustomEvent<{ pouName: string; body: string }>).detail | ||
| if (targetPou !== name) return | ||
| const model = editorInstance.getModel() | ||
| if (model) { | ||
| if (model && model.getValue() !== body) { | ||
| isSyncingModelRef.current = true | ||
| const fullRange = model.getFullModelRange() | ||
| editorInstance.executeEdits('ai-code-apply', [{ range: fullRange, text: body }]) | ||
| editorInstance.executeEdits('ai-tool-update', [{ range: fullRange, text: body }]) | ||
| isSyncingModelRef.current = false | ||
| editorInstance.focus() | ||
| } | ||
| setLocalText(body) | ||
| } | ||
| window.addEventListener('ai-pou-updated', handlePouUpdated) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the parent remounts MonacoEditor per POU (key strategy) or reuses it.
rg -nP -C4 '<MonacoEditor\b' --type=tsx --type=ts
# Also check how tab/editor switching is handled around this component.
rg -nP -C3 'keepCurrentModel|onDidChangeModel' --type=ts --type=tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Search for MonacoEditor component usage (corrected approach)
rg 'MonacoEditor' -A 5 -B 2 --max-count=10Repository: Autonomy-Logic/openplc-editor
Length of output: 7182
🏁 Script executed:
#!/bin/bash
# Look at the exact file and lines mentioned in the review
head -1000 src/frontend/components/_features/\[workspace\]/editor/monaco/index.tsx | tail -150 | head -100Repository: Autonomy-Logic/openplc-editor
Length of output: 3711
🏁 Script executed:
#!/bin/bash
# Find PrimitiveEditor usage to check key and keepCurrentModel props
rg 'PrimitiveEditor|keepCurrentModel|onDidChangeModel' -B 3 -A 3Repository: Autonomy-Logic/openplc-editor
Length of output: 4607
🏁 Script executed:
#!/bin/bash
# Get full context around handlePouUpdated registration with cleanup
sed -n '900,1000p' src/frontend/components/_features/\[workspace\]/editor/monaco/index.tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 3872
🏁 Script executed:
#!/bin/bash
# Check MonacoEditor component instantiation in workspace-screen for key prop
rg -A 8 '<MonacoEditor' src/frontend/screens/workspace-screen.tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 544
🏁 Script executed:
#!/bin/bash
# Look for useEffect cleanup of the ai-pou-updated listener
rg -B 10 -A 5 "removeEventListener.*ai-pou-updated|ai-pou-updated.*removeEventListener" src/frontend/components/_features/\[workspace\]/editor/monaco/index.tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 744
Fix stale name closure in AI tool event handlers to use dynamic store lookup.
handlePouUpdated, handleAcceptAllHunks, and handleRejectAllHunks close over the name prop at mount time. Since the editor instance persists across tab switches when key={undefined} and keepCurrentModel={true} (Electron mode), these listeners remain active with a stale name value when the user switches POUs. The ai-pou-updated event will then either drop updates for the actual target POU or apply them to the wrong editor instance.
Use openPLCStoreBase.getState().editor.meta.name dynamically inside each handler, matching the pattern already established in handleInsertAtCursor (line 924).
🛠 Suggested shape (resolve current POU dynamically)
- const handlePouUpdated = (e: Event) => {
- const { pouName: targetPou, body } = (e as CustomEvent<{ pouName: string; body: string; oldBody?: string }>)
- .detail
- if (targetPou !== name) return
+ const handlePouUpdated = (e: Event) => {
+ const { pouName: targetPou, body } = (e as CustomEvent<{ pouName: string; body: string; oldBody?: string }>)
+ .detail
+ const currentName = openPLCStoreBase.getState().editor.meta.name
+ if (targetPou !== currentName) returnApply the same treatment to handleAcceptAllHunks and handleRejectAllHunks.
📝 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.
| // Listen for AI tool updates — sync editor model with new body | |
| const handlePouUpdated = (e: Event) => { | |
| const { pouName: targetPou, body } = (e as CustomEvent<{ pouName: string; body: string; oldBody?: string }>) | |
| .detail | |
| if (targetPou !== name) return | |
| setDiffReview({ active: true, proposedBody, variableSummary }) | |
| } | |
| window.addEventListener('ai-review-code', handleCodeReview) | |
| // Listen for AI chat "apply code" — applies after the user accepted the diff | |
| const handleCodeApplied = (e: Event) => { | |
| const { pouName: targetPou, body } = (e as CustomEvent<{ pouName: string; body: string }>).detail | |
| if (targetPou !== name) return | |
| const model = editorInstance.getModel() | |
| if (model) { | |
| if (model && model.getValue() !== body) { | |
| isSyncingModelRef.current = true | |
| const fullRange = model.getFullModelRange() | |
| editorInstance.executeEdits('ai-code-apply', [{ range: fullRange, text: body }]) | |
| editorInstance.executeEdits('ai-tool-update', [{ range: fullRange, text: body }]) | |
| isSyncingModelRef.current = false | |
| editorInstance.focus() | |
| } | |
| setLocalText(body) | |
| } | |
| window.addEventListener('ai-pou-updated', handlePouUpdated) | |
| // Listen for AI tool updates — sync editor model with new body | |
| const handlePouUpdated = (e: Event) => { | |
| const { pouName: targetPou, body } = (e as CustomEvent<{ pouName: string; body: string; oldBody?: string }>) | |
| .detail | |
| const currentName = openPLCStoreBase.getState().editor.meta.name | |
| if (targetPou !== currentName) return | |
| const model = editorInstance.getModel() | |
| if (model && model.getValue() !== body) { | |
| isSyncingModelRef.current = true | |
| const fullRange = model.getFullModelRange() | |
| editorInstance.executeEdits('ai-tool-update', [{ range: fullRange, text: body }]) | |
| isSyncingModelRef.current = false | |
| } | |
| setLocalText(body) | |
| } | |
| window.addEventListener('ai-pou-updated', handlePouUpdated) |
🤖 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 946 - 961, The handlers handlePouUpdated, handleAcceptAllHunks, and
handleRejectAllHunks close over the prop name and can act on a stale POU when
the editor persists; update each handler to read the current POU name from
openPLCStoreBase.getState().editor.meta.name (same pattern as
handleInsertAtCursor) instead of using the closed-over name, so the check
against targetPou uses the runtime store value before applying edits via
editorInstance, toggling isSyncingModelRef.current, and calling setLocalText as
needed.
Security update — fixes mXSS, prototype pollution, and config bypass vulnerabilities. Addresses openplc-web Dependabot PR #359. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dcoutinho1328
force-pushed
the
refactor/ai-chat-pr353
branch
from
e7dd827 to
672e71b
Compare
- Remove AIStatusIndicator badge from Monaco editor (removed in PR #353 per Thiago's request) - Restore per-hunk inline diff review with Keep/Undo buttons per change - Add ai-diff-review utility (computeHunks, applyAcceptedHunks) - Add Monaco ai-diff-review renderer (green/red decorations + floating buttons) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/frontend/components/_features/[workspace]/editor/monaco/ai-diff-review.ts (1)
77-142: Consider consolidating the per-hunk scroll listener and replacing theany-cast disposable storage.Two minor refinements in this block:
- One
editor.onDidScrollChangelistener is registered per hunk (line 134) and each listener only updates one container. For a large refactor touching many hunks this means N listeners fire on every scroll. A single listener that iterates all containers would be cheaper and easier to dispose.- Stashing the disposable on the DOM node via
(container as any)._scrollDisposablerequires eslint-disables (L18-19, L140-141, L154-155). A module-scopedWeakMap<HTMLElement, monaco.IDisposable>keeps the code typesafe and avoids theanyescape hatch.♻️ Sketch
+const scrollDisposables = new WeakMap<HTMLElement, monaco.IDisposable>() ... - // Update position on scroll - const scrollDisposable = editor.onDidScrollChange(() => { - const newTop = Math.max(0, editor.getTopForLineNumber(hunk.startLine) - editor.getScrollTop()) - container.style.top = `${newTop}px` - }) - - // Store disposable for cleanup - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - ;(container as any)._scrollDisposable = scrollDisposable + // Position updates handled by the shared scroll listener below.Then register a single listener outside the per-hunk loop that walks
buttonContainersand repositions each. Cleanup disposes that one listener plus any entries in the WeakMap for stale containers.As per coding guidelines,
.ts/.tsxfiles should avoidanytypes; the WeakMap variant removes all threeany-disables in this file.🤖 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 77 - 142, The per-hunk scroll listener registration inside the loop (editor.onDidScrollChange) causes N listeners and the code stashes disposables on DOM nodes via (container as any)._scrollDisposable; replace this by creating a module-scoped WeakMap<HTMLElement, monaco.IDisposable> to store disposables and register a single editor.onDidScrollChange listener outside the hunks loop that iterates buttonContainers and recalculates each container.style.top using editor.getTopForLineNumber(hunk.startLine) - editor.getScrollTop(); remove the per-hunk listener creation and the (container as any) cast/eslint-disable usages, ensure the single listener is disposed during cleanup and entries are removed from the WeakMap when containers are removed.src/frontend/components/_features/[workspace]/editor/monaco/index.tsx (1)
181-286: Refactor:acceptedHunksnaming is inverted, andhandleUndoHunkhas a dead if/else plus an unused value.Small cleanups that would make this state machine much easier to follow:
acceptedHunksis initialized on L1072 with every hunk id and IDs are deleted from it as hunks get resolved (Keep or Undo). It is used on L216 to derive pending hunks. The field semantically represents "still-pending" hunks; rename topendingHunks(orunresolvedHunks) to avoid confusion with actually-accepted-and-kept hunks.- In
handleUndoHunk(L243-253) both branches ofif (!newAccepted.has(h.id)) … else …add the hunk tokeptIds, so the branching is dead — this can collapse to a single statement (or be replaced withkeptIds = new Set(prev.hunks.map(h => h.id).filter(id => id !== hunkId))).remainingHunksis computed on L272 and then never read;freshHunksfromcomputeHunksfully replaces it on L276-280. Drop L272-273 or useremainingHunks.length === 0as the sole short-circuit and skip the recompute.♻️ Illustrative diff for (2) and (3)
- // 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) - } - } + // Rebuild body: every hunk except the one being undone keeps its new code. + const keptIds = new Set(prev.hunks.map((h) => h.id).filter((id) => id !== hunkId)) ... - // Recompute hunks for remaining pending changes - const remainingHunks = prev.hunks.filter((h) => newAccepted.has(h.id)) - if (remainingHunks.length === 0) return null - - // Recompute line positions for remaining hunks const freshHunks = computeHunks(prev.oldBody, newBody) - const freshAccepted = new Set(freshHunks.map((h) => h.id)) - if (freshHunks.length === 0) return null - return { ...prev, newBody, hunks: freshHunks, acceptedHunks: freshAccepted } + return { ...prev, newBody, hunks: freshHunks, acceptedHunks: new Set(freshHunks.map((h) => h.id)) }🤖 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 181 - 286, The state field acceptedHunks is semantically inverted (it actually tracks unresolved/pending hunks) and should be renamed to pendingHunks (update its initializer and every usage in setDiffReview, the pendingHunks derivation, renderDiffReview call, and any code that mutates it) to avoid confusion; inside handleUndoHunk simplify the keptIds construction by removing the dead if/else and directly building keptIds as all hunk ids except the undone hunk (or use a map/filter on prev.hunks), remove the unused remainingHunks variable before recomputing freshHunks (drop the L272-273 style check or replace it with a single short-circuit using freshHunks.length), and ensure subsequent references to acceptedHunks are updated to pendingHunks (affecting computeHunks, freshAccepted, applyAcceptedHunks, and the setDiffReview 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 `@package.json`:
- Line 110: The package.json currently lists the redundant devDependency
"@types/diff": "^7.0.2" while the installed "diff@^9.0.0" already includes its
own TypeScript definitions; remove the "@types/diff" entry from package.json
(devDependencies) so TypeScript uses the bundled types, then reinstall/update
your lockfile (npm install / yarn install) to ensure the lockfile reflects the
removal and run a quick TypeScript build to verify no type regressions.
In `@src/frontend/components/_features/`[workspace]/editor/monaco/index.tsx:
- Around line 1044-1055: When oldBody === undefined the code updates the Monaco
model and localText but never persists to the store; change the branch so it
first reads the current editor name once from
openPLCStoreBase.getState().editor.meta.name to avoid stale closure, then
perform the model edit with isSyncingModelRef.current toggled as before,
setLocalText(body), and finally call updatePou(name, { body }) to persist the
change; keep isSyncingModelRef usage so onChange/handleWriteInPou still
short-circuits during the programmatic edit but ensure updatePou is invoked
explicitly after unsetting the flag.
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/editor/monaco/ai-diff-review.ts:
- Around line 77-142: The per-hunk scroll listener registration inside the loop
(editor.onDidScrollChange) causes N listeners and the code stashes disposables
on DOM nodes via (container as any)._scrollDisposable; replace this by creating
a module-scoped WeakMap<HTMLElement, monaco.IDisposable> to store disposables
and register a single editor.onDidScrollChange listener outside the hunks loop
that iterates buttonContainers and recalculates each container.style.top using
editor.getTopForLineNumber(hunk.startLine) - editor.getScrollTop(); remove the
per-hunk listener creation and the (container as any) cast/eslint-disable
usages, ensure the single listener is disposed during cleanup and entries are
removed from the WeakMap when containers are removed.
In `@src/frontend/components/_features/`[workspace]/editor/monaco/index.tsx:
- Around line 181-286: The state field acceptedHunks is semantically inverted
(it actually tracks unresolved/pending hunks) and should be renamed to
pendingHunks (update its initializer and every usage in setDiffReview, the
pendingHunks derivation, renderDiffReview call, and any code that mutates it) to
avoid confusion; inside handleUndoHunk simplify the keptIds construction by
removing the dead if/else and directly building keptIds as all hunk ids except
the undone hunk (or use a map/filter on prev.hunks), remove the unused
remainingHunks variable before recomputing freshHunks (drop the L272-273 style
check or replace it with a single short-circuit using freshHunks.length), and
ensure subsequent references to acceptedHunks are updated to pendingHunks
(affecting computeHunks, freshAccepted, applyAcceptedHunks, and the
setDiffReview return).
🪄 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: 3fb4dc89-8491-4dc0-bf56-12c77db8c533
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonsrc/frontend/components/_features/[workspace]/editor/monaco/ai-diff-review.tssrc/frontend/components/_features/[workspace]/editor/monaco/ai-status-indicator.tsxsrc/frontend/components/_features/[workspace]/editor/monaco/index.tsxsrc/frontend/utils/ai-diff-review.ts
💤 Files with no reviewable changes (1)
- src/frontend/components/_features/[workspace]/editor/monaco/ai-status-indicator.tsx
| "@teamsupercell/typings-for-css-modules-loader": "^2.5.2", | ||
| "@testing-library/jest-dom": "^6.6.4", | ||
| "@testing-library/react": "^14.0.0", | ||
| "@types/diff": "^7.0.2", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no code relies on resolution from `@types/diff` specifically and that
# imports from 'diff' resolve against the bundled types in v9.
rg -nP --type=ts --type=tsx -C2 "from\s+['\"]diff['\"]"
rg -nP --type=json -C1 '"@types/diff"'Repository: Autonomy-Logic/openplc-editor
Length of output: 448
🏁 Script executed:
#!/bin/bash
# Check if diff@^9.0.0 is at line 64 in dependencies
sed -n '60,70p' package.json
# Search for imports of 'diff' in all source files (not just TS/TSX)
rg -n "from\s+['\"]diff['\"]|require\s*\(\s*['\"]diff['\"]" --type-list | head -20
rg -rn "from ['\"]diff['\"]|require(['\"]diff['\"])"
# Check the structure of devDependencies around line 110
sed -n '105,115p' package.jsonRepository: Autonomy-Logic/openplc-editor
Length of output: 1297
🏁 Script executed:
#!/bin/bash
# Get the full import statement from the ai-diff-review.ts file
cat -n src/frontend/utils/ai-diff-review.ts | head -50
# Also search more carefully for all uses of 'diff'
rg -n "import.*diff|from.*diff|require.*diff" src/Repository: Autonomy-Logic/openplc-editor
Length of output: 3267
🌐 Web query:
diff npm package v9 type definitions shipped
💡 Result:
No, the "diff" npm package (kpdecker/jsdiff) version 9 type definitions have not shipped as of 2026-04-22. The latest released version is 8.0.4 (published around 2026-03-23). Type definitions were first shipped in v8.0.0 (May 2025), making @types/diff obsolete. Version 9.0.0 exists as a prerelease on GitHub (drops ES5 support, uses TextDecoder/Uint8Array, compiled to ES6 target), but it is not published to npm. GitHub tags confirm no v9 release; latest is 8.0.4. npm/cli recently updated to diff@8.0.3 (Jan 2026).
Citations:
- 1: Please ship TypeScript definitions kpdecker/jsdiff#303
- 2: https://www.npmjs.com/package/@types/diff
- 3: https://www.npmjs.com/package/diff
- 4: https://www.npmjs.com/package/diff?activeTab=versions
- 5: https://github.com/kpdecker/jsdiff/blob/master/release-notes.md
- 6: https://github.com/kpdecker/jsdiff
- 7: https://www.jsdocs.io/package/diff
- 8: https://github.com/kpdecker/jsdiff/tags
- 9: https://www.reacti.me
🏁 Script executed:
#!/bin/bash
# Check the actual resolved version in package-lock.json
grep -A 5 '"diff"' package-lock.json | head -20
# Also check if there's a resolved field with a URL
rg -n '"diff"' package-lock.json -A 3Repository: Autonomy-Logic/openplc-editor
Length of output: 710
🏁 Script executed:
#!/bin/bash
# Find the node_modules/diff entry in package-lock.json
jq '.packages["node_modules/diff"]' package-lock.json 2>/dev/null || {
# Fallback if jq doesn't work
sed -n '/node_modules\/diff/,/^ }/p' package-lock.json | head -20
}Repository: Autonomy-Logic/openplc-editor
Length of output: 348
Remove redundant @types/diff — diff v9 ships its own type definitions.
The @types/diff package has been deprecated; its author explicitly states it is a stub types definition and that "diff provides its own type definitions, so you do not need this installed." From the diff package changelog: type definitions were first shipped in version 8.0.0 (May 2025).
Beyond being redundant, the major versions are mismatched (@types/diff@^7.0.2 vs diff@^9.0.0), which can cause TypeScript to pick up outdated stub types on some resolution setups. Drop the dev dependency and rely on the types bundled with diff.
📦 Suggested fix
- "@types/diff": "^7.0.2",
"@types/eslint": "^9.6.1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 110, The package.json currently lists the redundant
devDependency "@types/diff": "^7.0.2" while the installed "diff@^9.0.0" already
includes its own TypeScript definitions; remove the "@types/diff" entry from
package.json (devDependencies) so TypeScript uses the bundled types, then
reinstall/update your lockfile (npm install / yarn install) to ensure the
lockfile reflects the removal and run a quick TypeScript build to verify no type
regressions.
| // If no old body provided (backward compat), apply directly | ||
| if (oldBody === undefined) { | ||
| const model = editorInstance.getModel() | ||
| if (model && model.getValue() !== body) { | ||
| isSyncingModelRef.current = true | ||
| const fullRange = model.getFullModelRange() | ||
| editorInstance.executeEdits('ai-tool-update', [{ range: fullRange, text: body }]) | ||
| isSyncingModelRef.current = false | ||
| } | ||
| setLocalText(body) | ||
| return | ||
| } |
There was a problem hiding this comment.
Backward-compat branch updates editor but not the store — changes will be lost on remount/tab switch.
When oldBody === undefined, this branch calls editor.executeEdits(...) with isSyncingModelRef.current = true and then setLocalText(body). Because the sync flag is set, the onChange → handleWriteInPou path short-circuits at L1240 (if (isSyncingModelRef.current) return) and updatePou is never called. The diff-review branch below (L269 / L376) does the opposite and calls updatePou explicitly.
Net effect: the Monaco model and local localText show the AI's new body, but pou.body.value in the store still holds the old body. As soon as the POU effect at L195-207 re-syncs localText from openPLCStoreBase.getState().project.data.pous (e.g. on tab switch, or when pous updates for any unrelated reason), the AI's update silently reverts.
🐛 Suggested fix
// If no old body provided (backward compat), apply directly
if (oldBody === undefined) {
const model = editorInstance.getModel()
if (model && model.getValue() !== body) {
isSyncingModelRef.current = true
const fullRange = model.getFullModelRange()
editorInstance.executeEdits('ai-tool-update', [{ range: fullRange, text: body }])
isSyncingModelRef.current = false
}
setLocalText(body)
+ // Persist to the store so the change survives remounts / tab switches.
+ openPLCStoreBase.getState().projectActions.updatePou({
+ name,
+ content: { language, value: body },
+ })
return
}Note: the same name used for the store write here is subject to the stale-closure problem already flagged in the previous review — resolve both together by reading openPLCStoreBase.getState().editor.meta.name once at the top of the handler.
🤖 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 1044 - 1055, When oldBody === undefined the code updates the Monaco model
and localText but never persists to the store; change the branch so it first
reads the current editor name once from
openPLCStoreBase.getState().editor.meta.name to avoid stale closure, then
perform the model edit with isSyncingModelRef.current toggled as before,
setLocalText(body), and finally call updatePou(name, { body }) to persist the
change; keep isSyncingModelRef usage so onChange/handleWriteInPou still
short-circuits during the programmatic edit but ensure updatePou is invoked
explicitly after unsetting the flag.
1 participant
Summary
refactor/ai-chat-pr353messages[]replacing per-POUconversations[], addisAgenticLoopRunningai-pou-updated)adapter-componentslayer with frontend-level permissionsAffected Surfaces
frontend/store/slices/ai/,frontend/store/__tests__/,frontend/components/.../monaco/,backend/shared/styles/,__architecture__/Validation
compare_repos.py— 837 files, all identical)Cross-repo
Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores