Core architecture fixes -- Review in steps - #878
Open
rokotyan wants to merge 24 commits into
Open
Conversation
…empty aliasing The class instance check tested the whole source object instead of the value under the current key, assigning the entire object to every merged key for class instance configs. cloneDeep already copies class instances by reference, so the branch is removed in favor of the default path. merge() now also returns a clone (instead of the original reference) when there is nothing to merge, so callers can never mutate a shared default config. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…without a config Components created without a config kept the module-level default config object as their own (via the `config = this._defaultConfig` initializer), so any mutation of `instance.config` polluted every other instance of the same component class (e.g. XYContainer setting `xAxis.config.type`). Constructors now always call `setConfig()`, which produces a private merged clone of the defaults. `XYComponentCore.setConfig` and `LeafletMap.setConfig` were hardened to tolerate an undefined config argument. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
setData assigned the new data to the data model before comparing, so the change check compared the data with itself and never fired: the tooltip and crosshair were never hidden on data updates. The previous reference is now captured before assignment and compared by reference, which also removes a full deep-equality walk of the dataset on every setData call. isEqual() also gained a reference short-circuit for identical inputs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…urce leaks ComponentCore.destroy() now invokes a protected _onDestroy() hook (and is idempotent), giving components a place to release timers, animation frames, observers and global listeners: - Graph: stop the 35ms link-flow interval timer (previously it kept firing forever after destroy, retaining the whole component); remove the window keydown/keyup listeners and namespace them per instance uid so multiple graphs no longer override each other's shift-brush handlers - Crosshair: cancel the pending render animation frame and detach container mouse/wheel listeners - Axis: cancel the pending tick label collision detection frame - TopoJSONMap, LeafletMap: convert destroy() overrides that skipped super.destroy() (leaving isDestroyed() false and DOM attached) into _onDestroy() implementations - LeafletFlowMap: destroy the internal LeafletMap instance, which was previously never cleaned up Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dratic scans Processing data in the setter previously ran several O(n^2) passes: multi-link detection filtered all links per link, per-node link lists filtered all links per node, state transfer scanned previous items per item, and link endpoint resolution scanned all nodes per endpoint (with a deep-equality comparison for object identifiers). Large graphs spent seconds of main-thread time in setData. All passes are now linear: links are grouped by canonical node-pair key, node link lists and previous-state lookups use maps, and string/object node identifiers resolve through id and reference indexes (keeping the deep-equality scan only as a fallback for value-equal object identifiers). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Component event dispatch collected the matched elements with selection.nodes() inside every event handler invocation; the elements are now snapshotted once at bind time (events are re-bound after every render, so the snapshot stays in sync). The tooltip's delegated mousemove handler queried all trigger elements for every configured trigger on every mouse movement; it now only queries them after an event-path match, when the element index is actually needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er of configs updateComponents matches configs to components positionally, so reordering or inserting a component silently mis-assigns configs. A console warning now surfaces the mismatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SingleContainer hid the tooltip on every setData call (even with preventRender), while XYContainer only hid it on data changes and never on resize. Both containers now hide the tooltip (and XYContainer the crosshair) when the data actually changes and when the container is resized. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Configuring Sizing.Extend or Sizing.FitWidth with a component that doesn't report its own size threw a runtime TypeError on unguarded getWidth()/ getHeight() calls. ExtendedSizeComponent is now a real interface with an isExtendedSizeComponent type guard; the container warns about unsupported sizing and gracefully falls back to Sizing.Fit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ling logic render() defers the actual rendering to the next animation frame, so callers reading the DOM right after it returned would get stale output with no way to know when rendering completed. render() now returns a promise that resolves after the scheduled _render pass (batched render calls share one promise, and destroy() resolves a pending promise so awaiting callers don't hang). The rAF scheduling and resize observer setup moved into a shared _scheduleRender(), so SingleContainer's render() override no longer duplicates the base implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… types XYContainer imported six component config interfaces to build the XYConfigInterface union and cast a found config to AreaConfigInterface to extract the crosshair baseline accessor, inverting the containers/components layering and requiring container edits for every new baseline-aware component. XYComponentCore now exposes a getBaselineAccessor() contract method (overridden by Area), and XYConfigInterface is the base XYComponentConfigInterface, which all concrete component configs extend. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every updateContainer call (which framework wrappers issue on nearly every prop change) detached all children of the SVG element and re-appended them, interrupting in-flight transitions and forcing style recalculations. Containers now reconcile the desired child list against the DOM, moving or removing only the elements that actually changed. The setData pass that updateContainer re-runs also skips components that already hold the current data instance, so config-only updates no longer reprocess data. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both containers duplicated the per-component setSize / setContainerMargin / setColorFunction plumbing in their _preRender implementations, and the copies had drifted: SingleContainer never passed the container color function to annotations. The loop now lives in ContainerCore and is shared by both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Child prop updates called setData/setConfig on the component instance but nothing notified the container, so the chart never redrew until an unrelated container prop change or resize forced a render (container-level data was unaffected, which masked the bug in most examples). The containers now provide a 'dirty' context, and the generated components invoke it after setData/setConfig. The render is scheduled on the next animation frame by the core, so multiple child updates get batched. Also makes the svelte/solid barrel generators emit a trailing newline to match the checked-in files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tions Child data updates called setData without triggering a render, and config updates re-rendered only because component instances were stored in deeply reactive refs — the container's watchEffect happened to track config mutations through the proxy (plus the child also called component.render() directly, double-rendering and skipping container-level scale recalculation). The container accessors now expose a dirty() callback that schedules a chart render (rAF-batched by the core); generated components call it after setData/setConfig. Instances live in shallowRef/shallowReactive, so they are no longer deep-proxied on hot render paths. Also fixes: inject() defaults to undefined so components no longer throw when used outside a container, and SingleContainer's exposed 'component' is now a ref that actually updates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…omponent configs The wrappers spread raw props into setConfig/updateContainer, so the config merge deep-cloned the entire dataset (plus React children elements, className and style) on every update — once per component and once for the container. React templates and containers now destructure the non-config props away, Vue's useForwardProps skips the data key, and the Solid template uses splitProps. React's arePropsEqual also compares 'data' by reference instead of deep equality (deep-comparing large datasets on every parent render dominated the memo check), and a duplicated child-props comparison statement is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The wrappers disagreed on falsy data handling: React/Solid/Angular ignored any falsy value (so an empty-array reset passed but resetting worked by accident), while Svelte/Vue forwarded undefined into setData. Now every wrapper forwards the data prop whenever it is not undefined, in components and containers alike. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each wrapper package had a ~90% identical copy-pasted generator driver with
fragile shell usage: async exec('mkdir') with ignored errors, per-file eslint
shell-outs, and silent failure modes. Only svelte/vue/solid generated their
export barrels — react and angular required manual barrel edits, which is how
the Angular barrel ended up missing RadialBar and Treemap entirely (Angular
consumers could not use those components).
A shared runComponentGenerator driver now lives in
@unovis/shared/integrations/autogen: synchronous file writes with recursive
mkdir, error propagation, a single eslint --fix pass, and barrel generation
for all five packages. The per-framework drivers shrink to their component
templates and path/export conventions. Generate scripts chain with && so a
failed step no longer looks like success.
Fixes the missing Angular exports: VisRadialBarComponent/Module and
VisTreemapComponent/Module are now exported.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every mousemove-driven render called getNearest(), which allocated a wrapper array, sorted the entire dataset (twice per frame with snapping enabled), and then ran O(n) indexOf lookups to recover the datum index. For large datasets this burned milliseconds per frame on pure overhead. The crosshair now caches the data indices sorted by x value (invalidated on data, config, or accessor changes) and bisects over them per frame, returning the original-array index directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…le and memoize expensive bleed getters The container recomputed scale domains three to four times per render cycle (updateComponents, _preRender, and again inside _setAutoMargin), and evaluated component bleed getters repeatedly, each of which did O(n) or worse work: Scatter rebuilt all on-screen point objects, Timeline appended and measured live label elements, and Sankey ran a full probe layout pass. _preRender now sets scales and updates their domains exactly once (auto margin iterations only refresh the ranges, which do change between iterations), and the redundant sync scale update in updateComponents is gone. Scatter, Timeline, and Sankey cache their bleed computation keyed on the data/config instances and the relevant scale state. The bleed passed to onRenderComplete reuses the value computed during _preRender instead of recomputing after every render. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…odel inputs The node update loop read back layout state per node right after attribute and style writes: a getBBox() call per node (used only for custom SVG shapes) and a getComputedStyle() read of one shared CSS variable. For a thousand nodes that's a thousand forced style/layout recalculations per render. The label font size is now resolved once per update pass, and getBBox runs only for custom SVG nodes whose size isn't known upfront. GraphDataModel and MapGraphDataModel also deep-cloned every node, link, area, and point on each data update. They now shallow-copy the records: the library only augments top-level properties (underscore state, layout fields), so nested user objects are safely shared by reference. The map link clone was dropped entirely since new link objects are built from resolved endpoints anyway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every setConfig/updateContainer call deep-cloned the entire default config and every provided value through merge(). Framework wrappers issue these calls on nearly every prop change, so animated or frequently updated charts paid a full config deep clone per component per frame. Config merging now uses mergeByReference(): a new config object is still built on every update (so prevConfig comparisons keep working and top-level writes never touch the defaults), plain-object values present in both sources are merged recursively into new objects, and everything else is assigned by reference. The library never mutates nested config values, so the defaults stay intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n the tick collision pass With autoMargin enabled, preRender() built a complete invisible axis (full d3 axis render, per-tick text wrapping/trimming, several getBBox calls) and threw it away — on every render, twice on first render. The measurement results are now cached keyed on the config and data instances, size, and scale state, so re-renders that don't affect the axis skip the throwaway DOM work entirely. The tick label collision pass re-read every label's getBoundingClientRect for each pair in each of its three iterations (up to thousands of rect reads for dense axes). Labels don't move during collision detection, so the rects are now measured once per pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… gallery A new interactive example with controls that mutate child-level data and config (the <VisLine>'s data and lineWidth props, not the container's), exercising the exact update path the wrapper reactivity fixes address. Buttons: add point, shuffle, toggle width, clear (empty data), reset. Uses duration=0 so updates apply synchronously, and deterministic data so all five framework panels render identically for the same state. Verified in the gallery (React, Vue, Solid, Svelte, vanilla TS) that toggling width re-renders (stroke 2->5), adding points grows the path, clearing hides the line (opacity 0), and reset restores it — identically across all five. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
|
cc @suryahanumandla there's a svelte fix in here. Let's release a beta version with svelte 5 update and see if we could get the community to test it besides just us. |
lee00678
reviewed
Aug 13, 2026
| const { components, config } = this | ||
| if (!data) return | ||
|
|
||
| const hasDataUpdated = this.datamodel.data !== data |
Collaborator
There was a problem hiding this comment.
How did we miss this. :p
Contributor
Author
There was a problem hiding this comment.
Yeah this is embarrassing 😄
lee00678
reviewed
Aug 13, 2026
| for (const item of items) { | ||
| const dPrev = itemsPrev.find((dp) => getId(dp) === getId(item)) | ||
| const dPrev = prevById.get(getId(item)) | ||
| if (dPrev) item._state = { ...dPrev._state } |
Collaborator
|
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.
@lee00678 @suryahanumandla
I'm not sure if we want to merge all of this, but it's definitely worth reviewing. Every commit has a description of the problem it solves.
Summary
This PR is a broad pass over the unovis core, containers, and all five framework wrappers (React, Angular, Svelte, Vue, Solid), fixing a set of long-standing correctness bugs, removing several large per-frame/per-render performance cliffs, and tightening the container/component architecture. It also consolidates the five copy-pasted component generator drivers into one shared implementation and adds a multi-framework gallery example that exercises the reactivity paths fixed here.
No public API is removed. Behavior changes are listed in the "Notes for reviewers" section at the bottom.
Correctness fixes
Shared default config aliasing (
c6bdfdae)Components created without a config kept the module-level default config object as their own instance config, so mutating
instance.configpolluted every other instance of the same component class (e.g.XYContainersettingxAxis.config.typechanged the type for every axis in the app). Constructors now always go throughsetConfig(), which produces a private merged clone.XYComponentCore.setConfigandLeafletMap.setConfigtolerate anundefinedargument.XYContainer.setDatachange detection was dead code (4d412067)setDataassigned the new data to the data model before comparing, so the change check compared the data with itself and never fired — the tooltip and crosshair were never hidden on data updates. The previous reference is now captured before assignment and compared by reference, which also removes a full deep-equality walk of the dataset on everysetData.isEqual()gained a reference short-circuit.Component resource leaks on destroy (
b4ac750a)ComponentCore.destroy()is now idempotent and invokes a new protected_onDestroy()hook. Fixed leaks:keydown/keyuplisteners are now removed and namespaced per instance uid, so multiple graphs no longer clobber each other's shift-brush handlers.destroy()overrides skippedsuper.destroy()(leavingisDestroyed()false and DOM attached); they're now_onDestroy()implementations.LeafletMap, which was previously never cleaned up.Sizing.Extend / Sizing.FitWidth crash → graceful fallback (
40178956)Configuring extended sizing with a component that doesn't report its own size threw a
TypeErroron unguardedgetWidth()/getHeight()calls.ExtendedSizeComponentis now a real interface with anisExtendedSizeComponenttype guard;SingleContainerwarns and falls back toSizing.Fit.Tooltip hide behavior unified across containers (
e5de46a5)SingleContainerhid the tooltip on everysetData(even withpreventRender), whileXYContaineronly hid it on data changes and never on resize. Both containers now hide the tooltip (andXYContainerthe crosshair) when data actually changes and on container resize.Positional config mismatch warning (
339974d9)XYContainer.updateComponentsmatches configs to components positionally, so inserting/reordering components silently mis-assigns configs. A console warning now surfaces count mismatches.Framework wrapper fixes
Svelte: child prop updates never re-rendered the chart (
88b657ca)Child
data/config prop updates calledsetData/setConfigon the component instance but nothing notified the container — the chart didn't redraw until an unrelated container prop change or resize. The containers now provide adirtycontext that generated components invoke aftersetData/setConfig; renders are rAF-batched by the core.Vue: implicit proxy tracking replaced with explicit notifications (
30108713)Child data updates didn't trigger a render at all, and config updates re-rendered only by accident — component instances lived in deeply reactive refs, so the container's
watchEffecthappened to track config mutations through the proxy (and the child also calledcomponent.render()directly, double-rendering and skipping container-level scale recalculation). Now: container accessors expose adirty()callback (rAF-batched), instances live inshallowRef/shallowReactive(no deep proxying on hot render paths),inject()defaults toundefinedso components no longer throw outside a container, andSingleContainer's exposedcomponentis a ref that actually updates.Unified data prop contract across all five frameworks (
eb9d5047)The wrappers disagreed on falsy data: React/Solid/Angular ignored any falsy value (so resetting to an empty array only worked by accident), while Svelte/Vue forwarded
undefinedintosetData. Every wrapper now forwards the data prop whenever it is notundefined, in components and containers alike.Stop passing datasets and framework props into component configs (
124340e3)React/Vue/Solid wrappers spread raw props into
setConfig/updateContainer, so the config merge deep-cloned the entire dataset (plus React children,className,style) on every update — once per component and once for the container. React templates/containers destructure non-config props away, Vue'suseForwardPropsskipsdata, Solid usessplitProps. React'sarePropsEqualnow comparesdataby reference instead of deep equality (deep-comparing large datasets on every parent render dominated the memo check).Performance
Config merging no longer deep-clones (
286d29c7)Every
setConfig/updateContainerdeep-cloned the entire default config and all provided values — wrappers issue these on nearly every prop change, so animated charts paid a full config deep clone per component per frame. Merging now usesmergeByReference(): a new config object is still built each update (soprevConfigcomparisons keep working and defaults are never mutated), plain objects merge recursively into new objects, everything else assigns by reference.GraphDataModel: O(n²) passes → linear (
bcc86a4b)Multi-link detection, per-node link lists, previous-state transfer, and link endpoint resolution were all quadratic (endpoint resolution even used deep equality per comparison). Large graphs spent seconds of main-thread time in
setData. All passes now use maps/indexes (canonical node-pair keys, id + reference indexes), keeping the deep-equality scan only as a fallback for value-equal object identifiers.Graph: per-node forced reflows removed; shallow-copy data inputs (
1de5d846)The node update loop did a
getBBox()and agetComputedStyle()read per node right after style writes — a thousand forced layout recalcs per render for a thousand nodes. Label font size is now resolved once per pass, andgetBBoxruns only for custom SVG nodes whose size isn't known upfront.GraphDataModel/MapGraphDataModelalso deep-cloned every node/link/area/point per update; they now shallow-copy (the library only augments top-level properties, so nested user objects are safely shared).Axis: auto-margin measurement caching + collision-pass rect snapshots (
ac29b47e)With
autoMargin,preRender()built a complete invisible axis (full d3 render, per-tick wrapping/trimming, severalgetBBoxcalls) and threw it away — every render, twice on first render. Measurements are now cached keyed on config/data instances, size, and scale state. The tick collision pass re-read every label'sgetBoundingClientRectfor each pair in each of three iterations; rects are now measured once per pass.Scale domains computed once per render cycle; bleed getters memoized (
5fbd9d4d)XYContainerrecomputed scale domains 3–4× per render (updateComponents,_preRender,_setAutoMargin) and repeatedly evaluated bleed getters that do O(n) or worse work (Scatter rebuilds all on-screen points, Timeline appends and measures live label elements, Sankey runs a full probe layout)._preRendernow sets domains exactly once (auto-margin iterations only refresh ranges); Scatter/Timeline/Sankey cache bleed keyed on data/config instances and scale state;onRenderCompletereuses the bleed computed during_preRender.Crosshair: bisect over cached sorted x values (
37459ef8)Every mousemove render sorted the entire dataset (twice per frame with snapping) and ran O(n)
indexOflookups. The crosshair now caches data indices sorted by x (invalidated on data/config/accessor changes) and bisects per frame.Container: SVG child reconciliation instead of full teardown (
750a8b40)Every
updateContainercall detached all SVG children and re-appended them, killing in-flight transitions and forcing style recalcs. Containers now reconcile the desired child list against the DOM, touching only elements that changed. ThesetDatapass insideupdateContaineralso skips components already holding the current data instance, so config-only updates no longer reprocess data.Tooltip/event dispatch: stop re-querying the DOM (
aa63d022)Event dispatch collected matched elements via
selection.nodes()inside every handler invocation; elements are now snapshotted once at bind time (events re-bind after every render, so the snapshot stays in sync). The tooltip's delegated mousemove handler queried all trigger elements for every configured trigger on every mouse move; it now queries only after an event-path match.Architecture
render()is awaitable (9a53463e) — rendering is rAF-deferred, so callers reading the DOM right afterrender()got stale output. It now returns a promise resolving after the scheduled_renderpass (batched calls share one promise;destroy()resolves pending promises so awaiting callers don't hang). rAF scheduling and resize-observer setup moved into a shared_scheduleRender(), removingSingleContainer's duplicated override.325568b6) —XYContainerimported six component config interfaces and cast toAreaConfigInterfaceto find the crosshair baseline, inverting the layering.XYComponentCorenow exposes agetBaselineAccessor()contract method (overridden by Area), andXYConfigInterfaceis the base interface all component configs extend.9b1d6abb) — both containers duplicated the per-componentsetSize/setContainerMargin/setColorFunctionplumbing and the copies had drifted (SingleContainernever passed the color function to annotations). The loop now lives inContainerCore.Tooling: shared autogen driver (
55b6fd1f)Each wrapper package had a ~90% identical generator driver with fragile shell usage (async
exec('mkdir')with ignored errors, per-file eslint shell-outs, silent failures). Only svelte/vue/solid generated export barrels — react and angular required manual edits, which is how the Angular barrel ended up missing RadialBar and Treemap entirely. A sharedrunComponentGeneratorin@unovis/shared/integrations/autogennow does synchronous writes, error propagation, one eslint pass, and barrel generation for all five packages; generate scripts chain with&&.Fixes:
VisRadialBarComponent/ModuleandVisTreemapComponent/Moduleare now exported from@unovis/angular.New example (
c8d0cab6)Added a
data-reactivitygallery example with controls that mutate child-level data and config (<VisLine>'sdataandlineWidth, not the container's) — the exact path the wrapper reactivity fixes address. Verified across React, Vue, Solid, Svelte, and vanilla TS that width toggling, point addition, clearing (empty data), and reset behave identically in all five panels.Notes for reviewers
mergeByReferencesemantics: configs no longer deep-clone user-provided values — nested objects are shared by reference. The library never mutates nested config values, so defaults stay intact, but user code that mutated a config object after passing it in will now be observed.GraphDataModelno longer deep-clones nodes/links; nested user objects are shared by reference. Top-level augmentation (underscore state, layout fields) still happens on copies.data={[]}now reliably clears charts in all frameworks;data={undefined}is consistently a no-op.XYContainernow hides the tooltip/crosshair on resize (it previously didn't), andSingleContainerno longer hides it on no-opsetDatacalls.packages/*/autogen/andpackages/shared/integrations/autogen.tsrather than each generated file.