Audit BaseNode render isolation at 500-node scale - #1063
Draft
snuziale wants to merge 1 commit into
Draft
Conversation
Contributor
|
Apollo Coded App preview deployments are ready.
|
Contributor
Dependency License Review
License distribution
Excluded packages
|
Contributor
📊 Coverage + size by packagePer-package coverage and bundle size on this PR. New-line coverage = of the source lines this PR adds or changes, the % hit by tests.
"Coverage" is each package's own |
Contributor
Storybook visual diffBaseline is the deployed main Storybook, so changes merged to main after this branch was last updated can also appear here. Logs Updated (PT): Sep 04, 2026, 09:06:26 AM |
snuziale
force-pushed
the
claude/basenode-audit-performance-td8ca5
branch
from
August 20, 2026 19:57
a3c1fe2 to
735fd1e
Compare
snuziale
force-pushed
the
claude/basenode-audit-performance-td8ca5
branch
from
August 26, 2026 17:04
735fd1e to
b6bf27d
Compare
Audit BaseNode's render path at 500-node scale, fix the findings, and pin the invariants that keep large canvases viable. Audit and guards: - PERFORMANCE.md: audit report with baseline numbers, ranked findings F1-F6, fix status, and a before/after improvements table - BaseNode.perf.test.tsx: deterministic render-count guards at N=500 (one render per node on mount, position-only sweeps render zero bodies, select/hover/data edits re-render exactly one node, height write-back converges with at most one store write per node and zero when seeded) - ConnectedHandlesContext.perf.test.tsx: granular notification guards (an edge change notifies only its endpoint nodes; identical edge arrays and untouched nodes keep stable Set snapshots) - BaseNode.bench.tsx + canvas-scale.bench.ts: vitest benchmarks for the component pipeline and per-node hot paths (resolveHandles, resolveDisplay, memo comparator, resolveCollisions), run via the new `bench` script Fixes: - F1: resolve handle configurations once in BaseNode for every source (context override, data override, manifest) and pass preResolved to useButtonHandles, which skips its internal re-resolution and its node-data memo dependency on that path (1000 -> 500 resolution passes per 500-node mount). Other callers keep the previous behavior, and the manifest-path field whitelist is preserved so previously-ignored manifest group fields (customPositionAndOffsets, boundary) still do not reach the handle renderers. - F2: drop interaction state (isConnecting/isSelected/isDragging) from statusContext in BaseNode and LoopNode; neither resolveToolbar nor resolveAdornments reads it, so connect gestures no longer re-resolve toolbars and adornments across all nodes (2000 -> 0 resolver runs per gesture at 500 nodes). Interaction-driven toolbar behavior stays in offsetToolbar and NodeToolbar props, and the now never-populated NodeStatusContext interaction fields are deprecated. - F3: execution/validation status hooks now read the context getter during render (memoized) instead of setState-in-effect: state is available on the first render and each published update costs one render per node instead of two. - F4: extract computeBaseNodeHeight to utils/node-height (exported) so consumers can seed node.height at creation and skip the mount write-back; BaseNode uses the same function so the rule cannot drift. It accepts a resolutionContext so heights seeded from raw manifests resolve repeat/string-visibility handles exactly like BaseNode. - F5: add the missing useSmartHandles dependency to toolbarSideHandleAffordances (stale toolbar offset under context handle overrides). - F6: keep the previous handleConfigurations identity when resolution output is value-identical (areResolvedHandleGroupsEqual), so label-only data edits no longer trigger updateNodeInternals DOM re-measures or handle element rebuilds. Bench: 500-node mount improved ~15% (mean ~488ms -> ~412ms, happy-dom, component code only). Build hygiene: the rslib entry glob excluded test and stories files but not bench or perf fixtures, so those compiled into dist and shipped in the published package requiring vitest and @testing-library/react; Vitest 4 also dropped `**/dist/**` from its default exclude, so the bench glob matched that build output. Exclude bench and perf fixtures from the rslib entry and tsconfig (which also stops empty declaration stubs) and exclude dist from both the test and benchmark runners. Bench files are therefore no longer covered by the dts type check, matching the existing treatment of test files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
snuziale
force-pushed
the
claude/basenode-audit-performance-td8ca5
branch
from
September 4, 2026 15:40
b6bf27d to
b0acfe2
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It makes broad, performance-critical changes to BaseNode’s render/handle-resolution pipeline and adds a large new perf-test/bench harness that warrants final human verification.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
18
to
+55
| @@ -22,6 +30,7 @@ export const useButtonHandles = ({ | |||
| nodeWidth, | |||
| nodeHeight, | |||
| portalActions, | |||
| preResolved, | |||
| }: { | |||
| handleConfigurations: HandleGroupManifest[]; | |||
| shouldShowHandles: boolean; | |||
| @@ -37,6 +46,14 @@ export const useButtonHandles = ({ | |||
| nodeHeight?: number; | |||
| portalActions?: boolean; | |||
|
|
|||
| /** | |||
| * Set when `handleConfigurations` is already the output of `resolveHandles` | |||
| * (templates replaced, repeats expanded, visibility booleans resolved). | |||
| * Skips the hook's internal resolution pass and its node-data dependency, | |||
| * so the same configuration is never resolved twice per render. | |||
| */ | |||
| preResolved?: boolean; | |||
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
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
Comprehensive performance audit of
BaseNodeand its render path at 500-node scale, with deterministic regression tests and benchmarks to prevent future regressions. All identified issues are fixed and pinned by tests.Verdict: 500 nodes is comfortably supported. Mount costs ~370ms (one-time), and steady-state interactions are O(1): selecting/hovering a node re-renders exactly that node, position sweeps cost ~2.7ms.
Key Changes
New Test & Benchmark Files
BaseNode.perf.test.tsx(308 lines): Deterministic render-count regression guards at 500-node scaleConnectedHandlesContext.perf.test.tsx(133 lines): Subscription-isolation guardsBaseNode.bench.tsx(113 lines): Timing benchmarks (non-CI-gating)canvas-scale.bench.ts(142 lines): Pure-function benchmarks for hot pathsresolveHandles,resolveDisplay,areNodePropsEqualIgnoringPosition,resolveCollisionsPERFORMANCE.md(221 lines): Audit findings, current numbers, and design decisions that scale wellnode-height.test.ts(92 lines): Height computation tests with resolution-context parityExecutionStatusContext.test.tsx(88 lines): Render-efficiency regression tests for status hooksFixture & Utility Files
BaseNode.perf-fixtures.tsx: Shared test/bench data builders and provider stacknode-height.ts: Extracted height computation logic with public API for seeding nodesCore Fixes (All Pinned by Tests)
F1: Handles resolved twice per node — FIXED
BaseNodenow resolves handles once and passespreResolvedtouseButtonHandlesuseButtonHandlesskips internal resolution whenpreResolvedis providedcustomPositionAndOffsets,boundary)onAction, etc.)areResolvedHandleGroupsEqualcomparator to avoid cascading invalidationF2: Connect gestures re-resolved toolbars/adornments on all nodes — FIXED
statusContextnow carries onlynodeId,executionState,validationState,modeisConnecting,isSelected,isDragging(neither resolver reads them)offsetToolbarand NodeToolbar propsF3: Execution/validation hooks double-rendered per update — FIXED
useNodeExecutionStateanduseElementValidationStatusnow read state viauseMemoon context identityuseState-in-effect pattern; state available on first renderModified Files
BaseNode.tsx: RemovedisConnecting/isSelected/isDraggingfromstatusContext, integrated height computation, handle resolution refactoreduseButtonHandles.tsx: AddedpreResolvedpath with inert subscription sentinelExecutionStatusContext.tsx: Switched fromuseState-in-effect touseMemogetter patternValidationStatusContext.tsx: Same pattern as ExecutionStatusContexthttps://claude.ai/code/session_01UNiMAk1eE9HTe3N5RD1wHD