Skip to content

Fix retargeted focus group motion - #144

Open
tannerlinsley wants to merge 1 commit into
mainfrom
taren/fix-focus-motion-context
Open

Fix retargeted focus group motion#144
tannerlinsley wants to merge 1 commit 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.

@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: 00f442cd-ee7d-4056-8058-928b6ee90438

📥 Commits

Reviewing files that changed from the base of the PR and between 32e3c73 and 35fbde9.

📒 Files selected for processing (17)
  • .changeset/focus-group-motion.md
  • 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/mark-composite-internal.ts
  • packages/charts-core/src/mark-composite.test.ts
  • packages/charts-core/src/motion.test.ts
  • packages/charts-core/src/motion.ts
  • packages/charts-core/src/scene-embed-internal.test.ts
  • packages/charts-core/src/scene-embed-internal.ts
  • packages/charts-core/src/scene.ts
  • packages/charts-core/src/types.ts
  • scripts/measure-bundles.mjs

Included review availability: Your plan provides up to 4 included reviews per hour; 2 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 phase 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.

Priority: ➖ Normal

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

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 35fbd

The documentation copies are consistent, and no actionable merge-blocking issue remains.

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
🚥 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. (8 skipped: 8… 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 identifies the primary change: fixing motion behavior for retargeted focus groups.
Linked Issues check ✅ Passed The changes satisfy issue #136. They preserve the owning mark through retargeted, composite, and embedded focus groups, apply focus-mark motion to structural groups, preserve valid descendant point mo…
Out of Scope Changes check ✅ Passed The changes are within scope. Source changes, tests, documentation, changeset data, and refreshed bundle and comparison baselines support the focus-group motion fix and its verification.
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. (8 skipped: 8 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 35fbde9

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

☁️ Nx Cloud last updated this comment at 2026-09-09 22:13:51 UTC

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

1 participant