Skip to content

Fix retargeted focus group motion - #144

Merged
tannerlinsley merged 33 commits into
mainfrom
taren/fix-focus-motion-context
Sep 10, 2026
Merged

tannerlinsley merged 33 commits into
mainfrom
taren/fix-focus-motion-context

Conversation

@tannerlinsley

@tannerlinsley tannerlinsley commented Sep 9, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Record the owning mark on retargeted focus layers and preserve that identity through composite and embedded scenes.
  • Resolve structural group timing from the focus mark while keeping valid selected descendants on their point motion.
  • Keep malformed slots, out-of-range slots, and colliding scene keys from borrowing unrelated point context.
  • Refresh the reviewed bundle, comparison, and catalog baselines for the small metadata cost.

Fixes #136

Testing

  • pnpm validate, 288 test files and 1,992 tests
  • Focused motion, focus, composite, and embedding suite, 84 tests
  • pnpm bundle:check
  • pnpm changeset status

Summary by CodeRabbit

  • Bug Fixes

    • Focused chart elements now consistently use their configured motion settings, including custom phase callbacks and disabled motion.
    • Improved animation behavior for grouped, nested, embedded, and composed focused elements.
    • Prevented invalid or unrelated selection data from affecting focus animations.
  • Documentation

    • Updated motion behavior guidance and comparison information.
  • Chores

    • Refreshed bundle-size and conformance benchmarks to reflect current measurements.

sukvvon and others added 23 commits August 8, 2026 13:09
A y scale with `side: 'right'` took its tick-label anchor from the physical
side alone. That anchor reaches the DOM as SVG `text-anchor`, which resolves
against inline base direction, so an RTL container painted the labels leftward
into the plot. The margin pass then read those bounds correctly and reserved
nothing on the right, widening the plot until it ran under its own labels.

Anchor the far side with `end` once the container reads right to left, and
mirror the same relation in the text estimator so a host without a DOM
measurer resolves the identical layout.
The layout fix costs 73 B raw in shared code, which puts the difference-mark
increment 0.02 kB over its ceiling. Gzip falls on most entries.
# Conflicts:
#	benchmarks/conformance/cases/103-bubble-map/tanstack.ts
#	benchmarks/conformance/cases/34-pointer-tooltip/tanstack.ts
#	benchmarks/conformance/cases/83-focus-context-window/view.tsx
#	benchmarks/conformance/cases/86-streaming-window-preservation/view.tsx
#	benchmarks/conformance/cases/88-echarts-free-cursor/controls.ts
#	benchmarks/conformance/cases/91-timeline-playback-scrubber/tanstack.ts
#	benchmarks/conformance/cases/92-editable-event-range/tanstack.ts
@coderabbitai

coderabbitai Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: b196bc2b-44cf-4e4e-a4aa-b0e4f8263389

📥 Commits

Reviewing files that changed from the base of the PR and between 35fbde9 and 915ac56.

📒 Files selected for processing (11)
  • API-FRICTION.md
  • benchmarks/bundle-size/README.md
  • benchmarks/bundle-size/universal-baseline.json
  • benchmarks/comparison/bundle-baseline.json
  • benchmarks/conformance/previews/manifest.json
  • docs/comparison.md
  • packages/charts-core/docs/comparison.md
  • packages/charts-core/src/motion.test.ts
  • packages/charts-core/src/scene.ts
  • packages/charts-core/src/types.ts
  • scripts/measure-bundles.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
  • benchmarks/conformance/previews/manifest.json
  • packages/charts-core/docs/comparison.md
  • docs/comparison.md
  • benchmarks/bundle-size/universal-baseline.json

Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change records focus mark ownership on scene groups, preserves namespacing through composite and embedded scenes, and applies focused mark motion to retargeted structural groups. Tests cover callbacks, disabled motion, point ownership, invalid slots, and nested ownership. Bundle and comparison baselines are refreshed.

Changes

Focus group motion

Layer / File(s) Summary
Focus ownership and namespacing
packages/charts-core/src/types.ts, packages/charts-core/src/scene.ts, packages/charts-core/src/mark-composite-internal.ts, packages/charts-core/src/scene-embed-internal.ts, packages/charts-core/src/*test.ts
Focus scene groups now store markId. Composite and embedded scenes namespace this identifier. Tests cover nested and embedded ownership.
Retarget motion context and validation
packages/charts-core/src/motion.ts, packages/charts-core/src/motion.test.ts
Retarget resolution validates selection slots, separates point ownership from structural group ownership, and applies focused mark motion to group entry and exit.
Release and measurement updates
.changeset/focus-group-motion.md, API-FRICTION.md, benchmarks/*, docs/comparison.md, packages/charts-core/docs/comparison.md, scripts/measure-bundles.mjs
Release notes and friction records describe the fix. Bundle budgets, baselines, preview hashes, revisions, and comparison measurements are refreshed.

Estimated code review effort: 4 (Complex) | ~45 minutes

Severity of issue fixed: Medium

Suggested reviewers: gillkyle

Sequence Diagram(s)

sequenceDiagram
  participant FocusMark
  participant SceneGroup
  participant RetargetContext
  participant MotionRenderer
  FocusMark->>SceneGroup: emit focus group with markId
  SceneGroup->>RetargetContext: provide focus ownership and slot metadata
  RetargetContext->>RetargetContext: resolve point and validate slot
  RetargetContext->>MotionRenderer: provide markId and motion context
  MotionRenderer->>SceneGroup: animate or remove structural group
Loading

Merge Risk: ⚪ Minimal · up to 915ac

The focus-motion update preserves mark ownership across composed and embedded scenes while retaining point-level motion for valid selections. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 9 files. (7 skipped: 7… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: fixing motion behavior for retargeted focus groups.
Linked Issues check ✅ Passed The changes satisfy issue #136 by preserving the owning focus mark, applying its motion to structural retargeted groups, preserving valid selected-descendant point motion, handling motion callbacks an…
Out of Scope Changes check ✅ Passed The implementation, tests, changeset, friction log, and refreshed bundle and comparison baselines support the stated focus-motion fix. No unrelated code changes are evident.
Full details: Docstring Coverage

Explanation

Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 9 files. (7 skipped: 7 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch taren/fix-focus-motion-context

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@nx-cloud

nx-cloud Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit 77c7a63

Command Status Duration Result
nx run charts-workspace:ci-distributed ✅ Succeeded <1s View ↗
nx run charts-workspace:package-check ✅ Succeeded <1s View ↗
nx run charts-workspace:benchmark-check ✅ Succeeded <1s View ↗

☁️ Nx Cloud last updated this comment at 2026-09-10 04:51:56 UTC

@tannerlinsley
tannerlinsley merged commit 98177d8 into main Sep 10, 2026
18 checks passed
@github-actions github-actions Bot mentioned this pull request Sep 10, 2026
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.

A mark's motion does not reach the SceneGroup its builder emits, and SceneGroup has no motion of its own

4 participants