Skip to content

Fix/unit scaling review followups#220

Merged
novykh merged 14 commits into
mainfrom
fix/unit-scaling-review-followups
Jul 13, 2026
Merged

Fix/unit scaling review followups#220
novykh merged 14 commits into
mainfrom
fix/unit-scaling-review-followups

Conversation

@novykh

@novykh novykh commented Jul 13, 2026

Copy link
Copy Markdown
Member

No description provided.

@novykh novykh requested a review from ktsaou July 13, 2026 10:31

@ktsaou ktsaou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The performance direction is sound, particularly removing the popover remount and bounding dense canvas work. Two blockers remain:

  1. The dense stacked-area reduction chooses retained samples independently per series, so adjacent layers can render different approximations of the same shared boundary.
  2. The value/unit grid change is unbounded in narrow columns and the branch currently fails its own test suite.

Local validation:

  • 139/140 test suites passed
  • 1,345 tests passed, 2 failed, 2 skipped
  • CJS/ES6 build passed
  • Storybook build passed
  • changed-file ESLint passed

Please address the two inline findings before merge.

Comment thread src/chartLibraries/dygraph/plotters/stackedArea.js Outdated
Comment thread src/components/line/dimensions/valueWithUnit.js
@novykh novykh requested a review from ktsaou July 13, 2026 11:47

@ktsaou ktsaou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared-index change fixes the original stacked-boundary mismatch, and the updated value/unit tests now pass. One new production correctness blocker remains in the replacement reducer: it buckets real Dygraph points by normalized x coordinates rather than canvas pixels, collapsing dense charts to only a handful of samples.

Validation on 0e59f98:

  • 140/140 test suites passed (1,349 passed, 2 skipped)
  • CJS and ES6 builds passed
  • Storybook build passed
  • changed-file ESLint passed
  • local Codacy found no introduced actionable issue

Comment thread src/chartLibraries/dygraph/plotters/stackedArea.js Outdated

@ktsaou ktsaou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No remaining blocking findings. The latest fix uses Dygraphs canvas pixel coordinates while retaining a fallback for synthetic points, and the production-shape regression test verifies every canvas bucket is represented.

Validation on 451aff0:

  • 140/140 test suites passed (1,350 passed, 2 skipped)
  • focused stacked-area regression test passed
  • 86,400-point reproduction selected 4,806 points across 801 pixel buckets
  • CJS and ES6 builds passed
  • Storybook build passed
  • changed-file ESLint passed
  • local Codacy found no introduced actionable issue

@novykh novykh merged commit b1fcda1 into main Jul 13, 2026
2 checks passed
@novykh novykh deleted the fix/unit-scaling-review-followups branch July 13, 2026 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants