Audit BaseNode render isolation at 500-node scale - #1063
Draft
snuziale wants to merge 4 commits 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): Aug 20, 2026, 01:25:29 PM |
…egression guards Audit BaseNode's render path at 500-node scale and pin the invariants that make large canvases viable: - 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 - PERFORMANCE.md: audit report with baseline numbers, ranked findings, and recommendations (double handle resolution, connect-gesture invalidation, execution-status context fan-out, mount write burst, stale memo dep) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNiMAk1eE9HTe3N5RD1wHD
Implements the fixes for findings F1-F6 from the BaseNode performance audit (PERFORMANCE.md), each pinned by a new regression test: - 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. - 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. - 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. - 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). PERFORMANCE.md updated with fix status and a before/after improvements table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNiMAk1eE9HTe3N5RD1wHD
Apply the five findings from the code review of the BaseNode performance fixes: - restore the manifest-path field whitelist: single-pass resolution no longer lets previously-ignored manifest group fields (customPositionAndOffsets, boundary) reach the handle renderers; override configs keep their runtime fields exactly as before (pinned by new BaseNode tests) - computeBaseNodeHeight accepts resolutionContext so heights seeded from raw manifests resolve repeat/string-visibility handles exactly like BaseNode (parity pinned by utils/node-height.test.ts) - useButtonHandles points its node-data subscription at an inert sentinel on the preResolved path, so pre-resolved nodes carry no live store subscription - deprecate the never-populated interaction fields on NodeStatusContext and document the mode field - replace stale line references in PERFORMANCE.md with symbol references Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNiMAk1eE9HTe3N5RD1wHD
…tion Running `pnpm bench` failed both bench suites with "Vitest cannot be imported in a CommonJS module using require()". Two causes combined. The rslib entry glob excluded test and stories files but not bench or perf fixtures, so those were compiled into dist as CJS and ESM, and shipped in the published package requiring vitest and @testing-library/react. Vitest 4 then dropped `**/dist/**` from its default exclude, which is only `**/node_modules/**` and `**/.git/**`, so the bench glob matched that build output alongside the sources it came from. Exclude bench and perf fixtures from the rslib entry and from tsconfig, which also stops empty declaration stubs being emitted for them, and exclude dist from both the test and benchmark runners so build output cannot be collected again. Bench files are no longer covered by the dts type check. They are type clean today, and the package has no typecheck script, so this matches the existing treatment of test files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
snuziale
force-pushed
the
claude/basenode-audit-performance-td8ca5
branch
from
August 20, 2026 19:57
a3c1fe2 to
735fd1e
Compare
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