perf(graphical-editor): remove edit hot-path deep clones + debounce flow write-back (DOPE-490) - #948
Conversation
…low write-back (DOPE-490) Every graphical edit deep-cloned the whole flow twice (JSON round-trip undo snapshot + structuredClone write-back), producing 150-200MB of transient garbage per edit burst on large projects. - new store/slices/shared/flow-writeback.ts: per-POU debounced (200ms) flow -> pou.body write-back; persists the raw flow by reference (immer copy-on-write makes sharing safe); flush/cancel entry points - save paths flush pending write-backs before serializing, so a save landing inside the debounce window persists the fresh body - undo/redo + snapshot capture hold plain references (no JSON clones) and flush the POU first so history never pairs a stale body with a fresh flow; project open cancels stale timers - ladder editor's local flow-less captureSnapshot replaced with the shared hook: undo of rung add/reorder now restores the canvas Mirror of the openplc-web PR for DOPE-490. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017RGT8nUsyY26HXFSuTBFLz
…dit-hot-path-clones
|
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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughGraphical Ladder and FBD edits now use centralized debounced write-back scheduling. Saves, snapshots, undo/redo, and project loading coordinate with pending flow writes to keep persisted and historical state synchronized. ChangesGraphical flow persistence and history
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LadderOrFBDEditor
participant FlowWriteBackScheduler
participant OpenPLCStore
participant SaveOrHistoryAction
LadderOrFBDEditor->>FlowWriteBackScheduler: schedule updated flow
FlowWriteBackScheduler->>OpenPLCStore: persist validated flow body after debounce
SaveOrHistoryAction->>FlowWriteBackScheduler: flush pending writes
FlowWriteBackScheduler->>OpenPLCStore: apply pending write-back
SaveOrHistoryAction->>OpenPLCStore: read synchronized state
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017RGT8nUsyY26HXFSuTBFLz
![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 (1)
src/frontend/components/_features/[workspace]/editor/graphical/FBD/index.tsx (1)
82-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
openPLCStoreBase.getStatefor deferred/out-of-React state access, per store guidelines.All three sites pass
useOpenPLCStore.getStateinto scheduler functions (scheduleFlowWriteBack/flushFlowWriteBacks) whose callbacks run later outside the React render/hook lifecycle (inside asetTimeout). The store guideline for this repo calls foropenPLCStoreBase.getState()for direct state access outside React;save-actions.tsalready follows this correctly for the same kind of deferred access. Verified thatauto-zustand-selectors-hook'screateSelectorHooksreturns the same underlying store object (zustand'screatealready copiesgetStateonto the bound hook), souseOpenPLCStore.getStateandopenPLCStoreBase.getStateare the same function today — this is a consistency/guideline-compliance issue rather than a live bug, but worth aligning so the two access patterns don't silently diverge if the store wrapper ever changes.
src/frontend/components/_features/[workspace]/editor/graphical/FBD/index.tsx#L82-L91: passopenPLCStoreBase.getStatetoscheduleFlowWriteBackinstead ofuseOpenPLCStore.getState.src/frontend/components/_features/[workspace]/editor/graphical/ladder/index.tsx#L138-L147: same change for itsscheduleFlowWriteBackcall.src/frontend/hooks/use-pou-snapshot.ts#L24-L42: same change for theflushFlowWriteBackscall.As per coding guidelines, "direct state access outside React should use
openPLCStoreBase.getState()."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/_features/`[workspace]/editor/graphical/FBD/index.tsx around lines 82 - 91, Replace usePLCStore.getState with openPLCStoreBase.getState when calling scheduleFlowWriteBack in src/frontend/components/_features/[workspace]/editor/graphical/FBD/index.tsx lines 82-91 and src/frontend/components/_features/[workspace]/editor/graphical/ladder/index.tsx lines 138-147. Make the same replacement for the flushFlowWriteBacks call in src/frontend/hooks/use-pou-snapshot.ts lines 24-42, preserving the existing deferred write-back behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/store/slices/shared/flow-writeback.ts`:
- Around line 48-55: The flow write-back path around schema.safeParse, including
flushFlowWriteBacks, must propagate validation failures instead of returning
silently. Return an explicit failure result when validation fails, have
flushFlowWriteBacks abort or surface that failure to the save flow, and only
update the POU and clear updated flags after all flows validate and write back
successfully.
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/editor/graphical/FBD/index.tsx:
- Around line 82-91: Replace usePLCStore.getState with openPLCStoreBase.getState
when calling scheduleFlowWriteBack in
src/frontend/components/_features/[workspace]/editor/graphical/FBD/index.tsx
lines 82-91 and
src/frontend/components/_features/[workspace]/editor/graphical/ladder/index.tsx
lines 138-147. Make the same replacement for the flushFlowWriteBacks call in
src/frontend/hooks/use-pou-snapshot.ts lines 24-42, preserving the existing
deferred write-back behavior.
🪄 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: 27090057-6111-4a3e-801e-172b26da8592
📒 Files selected for processing (7)
src/frontend/components/_features/[workspace]/editor/graphical/FBD/index.tsxsrc/frontend/components/_features/[workspace]/editor/graphical/ladder/index.tsxsrc/frontend/hooks/use-pou-snapshot.tssrc/frontend/services/save-actions.tssrc/frontend/store/__tests__/flow-writeback.test.tssrc/frontend/store/slices/shared/flow-writeback.tssrc/frontend/store/slices/shared/slice.ts
| const schema = language === 'ld' ? zodLadderFlowSchema : zodFBDFlowSchema | ||
| if (!schema.safeParse(flow).success) return | ||
|
|
||
| const { updated: _updated, ...flowBody } = flow | ||
| state.projectActions.updatePou({ name: pouName, content: { language, value: flowBody } }) | ||
|
|
||
| const flowActions = language === 'ld' ? state.ladderFlowActions : state.fbdFlowActions | ||
| flowActions.setFlowUpdated({ editorName: pouName, updated: false }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Propagate validation failures so save cannot discard graphical edits.
When validation fails, this returns without updating the POU, but flushFlowWriteBacks reports no failure. Save then serializes the stale body and clears every flow’s updated flag after success, silently losing the graphical edit. Return a failure result from write-back/flush and abort or surface the save until the flow is valid.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/store/slices/shared/flow-writeback.ts` around lines 48 - 55, The
flow write-back path around schema.safeParse, including flushFlowWriteBacks,
must propagate validation failures instead of returning silently. Return an
explicit failure result when validation fails, have flushFlowWriteBacks abort or
surface that failure to the save flow, and only update the POU and clear updated
flags after all flows validate and write back successfully.
…dit-hot-path-clones
1 participant
Summary
DOPE-490 — Perf 4/5 of the DOPE-446 editor-performance series.
Every graphical edit deep-cloned the ENTIRE flow twice: a 5× JSON round-trip undo snapshot plus an undebounced
structuredClonewrite-back intoproject.data.pous(live measurement: heap sawtooth 181↔389 MB, ~150–200 MB transient garbage per edit burst).store/slices/shared/flow-writeback.ts— per-POU debounced (200 ms) flow →pou.body.valuewrite-back. Persists the raw flow by reference: the store is immer-managed (frozen, copy-on-write), so the project copy and the live flow safely share structure. Zod validation + the DOPE-477 raw-object policy live here now.executeSaveProject/executeSaveFileflush pending write-backs before serializing, so a save landing inside the debounce window persists the fresh body (key acceptance criterion). Compile is covered via its auto-save.use-pou-snapshot.tsandsnapshotActions.undo/redono longer JSON-clone; they flush the affected POU first so a history entry can never pair a stale body with a fresh flow. Project open cancels stale timers.captureSnapshot(from 📝 Add docstrings toDOPE-21-FEAT-highlist-st#277) meant undo of rung add/reorder never restored the canvas; it now uses the sharedcaptureAndPush(includes flows).Validation
flow-writeback.test.ts); full jest suite passes (5216 tests).Mirror of https://github.com/Autonomy-Logic/openplc-web/pull/610
🤖 Generated with Claude Code
https://claude.ai/code/session_017RGT8nUsyY26HXFSuTBFLz
Summary by CodeRabbit