elementari icon indicating copy to clipboard operation
elementari copied to clipboard
verified publisher icon janosh

Use `structuredClone` to prevent `DEFAULTS` mutation in `Structure`

Open janosh opened this issue 2 months ago • 1 comments

Problem

The 3D structure viewer would occasionally spin at 750x normal speed (auto_rotate: 150 instead of 0.2) after browser back/forward navigation.

Root Cause

In Structure.svelte, line 45:

// BEFORE (buggy)
let scene_props = $state(DEFAULTS.structure)

Svelte 5's $state() does not deep-clone its initial value—it creates a reactive proxy over the same object reference. When Object.assign(scene_props, ...) was called, it mutated the global DEFAULTS.structure shared by all component instances.

Fix

// AFTER (fixed)
let scene_props = $state(structuredClone(DEFAULTS.structure))

Using structuredClone() creates a true deep copy, isolating each component's state from global defaults.

Changes

  • Wrap DEFAULTS.structure with structuredClone() when initializing scene_props
  • Add regression test verifying structuredClone isolation (mutation test verified it catches the bug)
  • Refactor settings tests to use test.each for better parameterization and cleaner output

Testing

  • All 49 settings tests pass
  • Mutation testing verified the regression test fails when structuredClone is removed
  • Coverage preserved at 100% for settings.ts

Summary by CodeRabbit

  • Bug Fixes

    • Prevented shared defaults from being mutated so component configuration changes no longer leak across instances.

  • Refactor

    • Renamed legend styling prop from wrapper_style to style across plotting components and demos to standardize inline legend styling.

  • Documentation

    • Updated demo and example snippets to use the new legend style property.

  • Tests

    • Consolidated and parameterized tests; added explicit immutability and performance regression checks; adjusted CI-aware test wrappers.

✏️ Tip: You can customize this high-level summary in your review settings.

janosh avatar Jan 13 '26 01:01 janosh

📝 Walkthrough

Walkthrough

Initializes cloned component defaults, renames PlotLegend prop wrapper_style → style across code/docs/tests, adds local sliding-window outlier removal to the plot cleaning pipeline (with types and tests), updates wyckoff handling and SymmetryStats UI, and adds WASM test init plus related test refactors.

Changes

Cohort / File(s) Summary
Structure component
src/lib/structure/Structure.svelte		 Initialize local scene_props with structuredClone(DEFAULTS.structure) to avoid mutating shared defaults.
PlotLegend API & callsites
src/lib/plot/PlotLegend.svelte, src/lib/plot/BarPlot.svelte, src/lib/plot/Histogram.svelte, src/lib/plot/ScatterPlot.svelte, src/lib/plot/ScatterPlot3D.svelte, src/routes/test/plot-legend/+page.svelte, tests/vitest/plot/PlotLegend.test.ts		 Public prop renamed from wrapper_style → style; typings updated (Omit<HTMLAttributes,'style'> + style?: string); callers, concatenation, and tests updated to use style.
Data cleaning — feature & types
src/lib/plot/data-cleaning.ts, src/lib/plot/types.ts, tests/vitest/plot/data-cleaning.test.ts		 Added local sliding-window outlier detection API remove_local_outliers, new LocalOutlierConfig/LocalOutlierResult types, extended CleaningConfig/CleaningQuality, integrated step before smoothing, and added extensive unit tests and exports.
Symmetry logic & UI
src/lib/symmetry/index.ts, src/lib/symmetry/SymmetryStats.svelte, tests/vitest/symmetry/SymmetryStats.test.ts, tests/vitest/symmetry/moyo-integration.test.ts		 Wyckoff processing now iterates over standardized-cell atom count (std_cell.numbers) to cover all atoms; Hermann–Mauguin symbol is shown inline with Space Group; tests updated and a WASM-based integration suite added.
Tests — setup, refactors & CI helpers
tests/vitest/settings.test.ts, tests/vitest/setup.ts, tests/playwright/trajectory.test.ts		 Settings tests refactored to parameterized patterns; added init_moyo_for_tests() WASM init helper; introduced describe_local_only to skip heavy Playwright suites on CI.
Demos / docs updates
src/routes/(demos)/plot/..., src/routes/(demos)/plot/bar-plot/+page.md, src/routes/(demos)/plot/histogram/+page.md, src/routes/(demos)/plot/scatter-plot/+page.md, src/routes/(demos)/plot/data-cleaning/+page.svelte		 Demo and documentation snippets updated to use legend.style instead of legend.wrapper_style.
Misc tests / UI tweaks
src/routes/test/plot-legend/+page.svelte, tests/vitest/plot/PlotLegend.test.ts		 State variables and labels renamed from legend_wrapper_style → legend_style; assertions adjusted for longhand CSS property parsing.
Package metadata
package.json		 Added engines.node: ">=22".

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • janosh/matterviz#243 — related changes to plot data-cleaning, types, and tests (local outliers and cleaning flow).
  • janosh/matterviz#123 — touches Structure.svelte and related initialization/scene handling.
  • janosh/matterviz#81 — related legend styling/prop rename and ScatterPlot legend handling.

Suggested labels

fix

Poem

🐰 I cloned the defaults so they wouldn't fray,
I nudged the legend's name a gentler way.
Outliers hopped out, WASM warmed its shell,
Tests ran and passed — the plots look well.
A happy little rabbit gives a quiet hooray.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately and concisely describes the main change: using structuredClone to prevent DEFAULTS mutation in the Structure component, which is the primary fix addressed in this pull request.
Docstring Coverage ✅ Passed Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • [ ] 📝 Generate docstrings
📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d052b61bff4c48b853250e573209434d269c6d9d and 0f291b9854810f70835d72a11b490abd5fa99e04.

📒 Files selected for processing (3)
  • package.json
  • src/lib/plot/PlotLegend.svelte
  • src/lib/plot/data-cleaning.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/plot/data-cleaning.ts (1)
src/lib/plot/types.ts (2)
  • LocalOutlierConfig (1043-1047)
  • LocalOutlierResult (1050-1054)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: tests / end-to-end
  • GitHub Check: tests / unit
  • GitHub Check: test-dash
🔇 Additional comments (10)
package.json (1)

10-12: The Node 22 requirement is justified. The codebase uses import ... with { type: 'json' } syntax (JSON module assertions with the with keyword) in multiple files including src/lib/colors/index.ts and extensions/vscode/src/extension.ts. This syntax is part of Node 22+; earlier versions used the deprecated import ... assert form. No changes needed.

src/lib/plot/PlotLegend.svelte (4)

14-14: LGTM! Clean prop rename from wrapper_style to style.

The Omit<HTMLAttributes<HTMLDivElement>, 'style'> correctly excludes the inherited style attribute to avoid type conflicts with the explicitly defined style?: string prop. This provides a cleaner API that aligns with standard HTML element conventions.

Also applies to: 27-31

141-147: Style concatenation looks correct.

The grid-template CSS strings already include trailing semicolons (e.g., auto);), so direct concatenation with the user-provided style prop will produce valid CSS.

411-412: Minor padding adjustments look intentional.

The reduced top padding for indented items (0 vs 1px) and group headers (2px vs previous) creates tighter vertical spacing. These are reasonable styling refinements.

Also applies to: 447-447

378-381: Verify project's minimum browser version requirements.

The light-dark() CSS function requires browser support for Chrome 123+, Firefox 120+, Safari 17.5+, and Edge 123+, and depends on color-scheme: light dark being set on :root. The project already has proper color-scheme setup in src/lib/theme/index.ts. However, confirm that these browser versions align with your project's documented minimum requirements.

src/lib/plot/data-cleaning.ts (5)

12-13: LGTM! New type imports.

The LocalOutlierConfig and LocalOutlierResult types are properly imported from ./types to support the new local outlier detection functionality.

419-424: Good defaults for local outlier detection.

The default values are reasonable:

  • window_half = 7 → 15-point neighborhood (7 on each side)
  • mad_threshold = 2.0 → approximately 3σ equivalent for normal distributions
  • max_iterations = 5 → sufficient for most clustered outlier scenarios

428-466: Local median and MAD functions correctly exclude center point.

Excluding the center point from its own local statistics is the correct approach for outlier detection—it prevents a true outlier from masking itself by skewing the local median/MAD. The functions handle edge cases (empty windows, non-finite values) appropriately.

471-549: Well-designed iterative outlier removal with intentional stability trade-off.

The algorithm correctly:

  1. Validates minimum array size before processing (lines 486-492)
  2. Iterates with a fresh mask copy per pass (line 499)
  3. Uses original values for statistics to prevent cascading false positives (lines 500-502)
  4. Skips zero-MAD cases where no meaningful threshold exists (line 520)

The design choice to compute statistics from original values (rather than the remaining "kept" values) is documented and reasonable—it prioritizes stability over aggressive removal of clustered outliers. This matches the comment's intent.

742-760: Clean integration of local outlier removal into the cleaning pipeline.

The new Step 3 correctly:

  • Applies outlier removal before smoothing (which would otherwise obscure outliers)
  • Updates quality.outliers_removed metadata
  • Uses apply_filter to synchronize all associated arrays (x, y, metadata, color_values, size_values)

The step renumbering (smoothing → Step 4, instability → Step 5) maintains logical flow: invalid values → bounds → outliers → smooth → detect instability.

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Jan 13 '26 01:01 coderabbitai[bot]