perf(graphical-editor): scope per-edit scans + hot-path cleanups (DOPE-491) - #949
Conversation
…rites (DOPE-491) - syncNodesWithVariables(FBD): batched updateNodes callback (one store commit per sweep) + optional rungId scoping; ladder add/drag-stop are rung-scoped, remove stays flow-scoped (block removal can delete variables referenced by other rungs); variable-table edits, project open and save relink keep the full sweep - getFBDPouVariablesRungNodeAndEdges: WeakMap-cached per-rung lookups (node-by-id, edges-by-source/target) + variable name index — per-node render cost drops from O(nodes+edges) to O(1) amortized - ladder/FBD debug styling: pure state computation with adjacency maps, deps narrowed to pouType + hasProgramInstance, content-stable guard so polls that don't change a rung keep styledNodes/styledEdges identity - FBD mouse tracking: state -> refs read at paste time; no re-renders on canvas mouse travel - debug poll: single setDebugValues commit per poll cycle (replaces setDebugBoolValues + setDebugNonBoolValues) Mirror of openplc-web fix/dope-491-hot-path-scans Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017RGT8nUsyY26HXFSuTBFLz
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR batches ladder and FBD node synchronization, consolidates debugger value updates, stabilizes debugger edge rendering, adds FBD lookup caches, and moves FBD clipboard pointer tracking from state to refs. ChangesGraphical synchronization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DebugPolling
participant WorkspaceActions
participant FBDBody
participant RungBody
DebugPolling->>WorkspaceActions: setDebugValues(boolValues, nonBoolValues)
WorkspaceActions->>FBDBody: update debug maps
WorkspaceActions->>RungBody: update debug maps
FBDBody->>FBDBody: compute stable edge states
RungBody->>RungBody: compute stable edge and node states
Possibly related PRs
Suggested labels: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/frontend/hooks/use-content-stable.ts (1)
18-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: add unit tests for the new hook/comparator.
useContentStable/mapsEqualnow underpin both ladder and FBD debug-edge stabilization, so a couple of tests (content-equal keeps identity, content-diff swaps reference, null↔value transitions) would lock in the identity-stability contract. Want me to draft them?🤖 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/hooks/use-content-stable.ts` around lines 18 - 24, Add unit tests for useContentStable and its mapsEqual comparator covering content-equal values retaining the previous reference, content-different values replacing it, and null-to-value and value-to-null transitions. Keep the tests focused on the hook’s identity-stability contract.src/frontend/components/_molecules/graphical-editor/fbd/index.tsx (1)
70-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional: consider extracting the shared debug-edge recursion.
determineEdgeState/isPassThroughNode(and much ofgetNodeOutputState) here closely mirror the ladder implementation insrc/frontend/components/_molecules/graphical-editor/ladder/rung/body.tsx. The block-output key building andgetNodeOutputStatearity differ between the two, so a shared helper would need parameterization — worth it only if you expect these to keep evolving together. Otherwise leaving them separate is fine.🤖 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/_molecules/graphical-editor/fbd/index.tsx` around lines 70 - 199, The debug-edge recursion and pass-through handling in computeFBDEdgeStates duplicate the ladder implementation, so optionally extract their shared logic into a reusable helper while parameterizing differing getNodeOutputState signatures and block-output key construction. Keep FBD-specific and ladder-specific behavior intact; if shared behavior is not expected to evolve together, leave the implementations separate.
🤖 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/__tests__/fbd-slice.test.ts`:
- Around line 270-301: Make the dirty-state checks in the updateNodes tests
meaningful by resetting the flow’s updated flag to false after setup and before
each updateNodes batch. In the successful batch test, assert updated becomes
true afterward; in the invalid-entry test, also assert it remains false while
the existing node data is unchanged. Update the tests around the updateNodes
cases without altering production behavior.
In `@src/frontend/store/slices/ladder/types.ts`:
- Around line 112-113: Update the updateNodes contract in
src/frontend/store/slices/ladder/types.ts lines 112-113 and
src/frontend/store/slices/fbd/types.ts lines 64-65 to return { ok: boolean;
message?: string }. In src/frontend/store/slices/fbd/slice.ts lines 152-167,
have updateNodes count successfully applied entries during the single store
commit and return an outcome indicating whether any update was applied, with an
optional message when none were applied.
---
Nitpick comments:
In `@src/frontend/components/_molecules/graphical-editor/fbd/index.tsx`:
- Around line 70-199: The debug-edge recursion and pass-through handling in
computeFBDEdgeStates duplicate the ladder implementation, so optionally extract
their shared logic into a reusable helper while parameterizing differing
getNodeOutputState signatures and block-output key construction. Keep
FBD-specific and ladder-specific behavior intact; if shared behavior is not
expected to evolve together, leave the implementations separate.
In `@src/frontend/hooks/use-content-stable.ts`:
- Around line 18-24: Add unit tests for useContentStable and its mapsEqual
comparator covering content-equal values retaining the previous reference,
content-different values replacing it, and null-to-value and value-to-null
transitions. Keep the tests focused on the hook’s identity-stability contract.
🪄 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: a88993b3-be93-4bf0-a035-b940f3e9d0b1
📒 Files selected for processing (21)
src/frontend/components/_atoms/graphical-editor/fbd/utils/utils.tssrc/frontend/components/_molecules/graphical-editor/fbd/fbd-utils/useCopyPaste.tssrc/frontend/components/_molecules/graphical-editor/fbd/index.tsxsrc/frontend/components/_molecules/graphical-editor/ladder/rung/body.tsxsrc/frontend/components/_molecules/variables-table/selectable-cell.tsxsrc/frontend/components/_organisms/variables-editor/index.tsxsrc/frontend/hooks/use-content-stable.tssrc/frontend/hooks/useDebugPolling.tssrc/frontend/services/save-actions.tssrc/frontend/store/__tests__/fbd-slice.test.tssrc/frontend/store/__tests__/ladder-slice.test.tssrc/frontend/store/__tests__/workspace-slice.test.tssrc/frontend/store/slices/fbd/slice.tssrc/frontend/store/slices/fbd/types.tssrc/frontend/store/slices/ladder/slice.tssrc/frontend/store/slices/ladder/types.tssrc/frontend/store/slices/shared/slice.tssrc/frontend/store/slices/workspace/slice.tssrc/frontend/store/slices/workspace/types.tssrc/frontend/utils/graphical/__tests__/sync-nodes-with-variables.test.tssrc/frontend/utils/graphical/sync-nodes-with-variables.ts
…ontent-stable Addresses CodeRabbit review: reset the FBD flow's updated flag before each updateNodes batch so the assertions actually prove the action's behavior, and add unit tests locking in the useContentStable/mapsEqual identity-stability contract. Mirror of openplc-web fix/dope-491-hot-path-scans Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017RGT8nUsyY26HXFSuTBFLz
|
@coderabbitai On the two nitpicks: unit tests for |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 01b84a3c3
git show --format=fuller --find-renames --find-copies -- 01b84a3c3 -- \
'src/frontend/hooks/use-content-stable.ts' \
'src/frontend/hooks/**' \
'src/frontend/components/_molecules/graphical-editor/ladder/rung/body.tsx' \
'src/frontend/components/_molecules/graphical-editor/fbd/index.tsx'
echo '--- Hook/test files ---'
fd -i 'use-content-stable' src/frontend
echo '--- Debug helper declarations/call contexts ---'
rg -n -C 5 'computeRungDebugStates|computeFBDEdgeStates|getNodeOutputState|forced' \
src/frontend/components/_molecules/graphical-editor/ladder/rung/body.tsx \
src/frontend/components/_molecules/graphical-editor/fbd/index.tsxLength of output: 18273
I also agree that the ladder and FBD recursion has material semantic differences—not merely superficial duplication—so a shared helper would introduce an unhelpful abstraction. I’m withdrawing that nitpick. 🐇💭 ✏️ Learnings added
|
1 participant
Summary
Perf 5/5 of the DOPE-446 series (DOPE-491) — eliminates the remaining measured hot-path costs after Perf 1–4:
sync-nodes-with-variables.ts): the util now collects corrections and calls a batchedupdateNodes(updates[])slice action once — one store commit per sweep instead of oneproduce()per mismatched node — plus an optionalrungIdscope. Ladder add/drag-stop are rung-scoped; remove stays flow-scoped (block removal can delete variables referenced by other rungs). Full-sweep paths retained (per the DOPE-477 caution): variable-table commit, type change, project-open relink, save-time relink.getFBDPouVariablesRungNodeAndEdges): WeakMap-cached per-rung lookup maps + variable-name index; signature and semantics unchanged for all callers. Render pass drops from O(N²) to O(N) amortized.find/filterinside the DFS), deps narrowed topouType+hasProgramInstance, and a newuseContentStablehook keepsstyledNodes/styledEdgesidentities across polls that don't change the rung — the rung skips re-render entirely.mousePosition/insideViewportstate → refs read at paste time (~75 commits/s of idle overhead removed).setDebugValuescommit per poll cycle (replaces the two separate setters).The optional
canEditorZoom/canEditorPan→nodrag/nowheelitem is deferred until the React Flow upgrade, as the card specifies.Validation
Mirror of https://github.com/Autonomy-Logic/openplc-web/pull/611
🤖 Generated with Claude Code
https://claude.ai/code/session_017RGT8nUsyY26HXFSuTBFLz
Summary by CodeRabbit