Skip to content

Ship animation identity planes only when the client can key-match - #330

Merged
Alek99 merged 1 commit into
mainfrom
alek/skip-unmatched-key-planes
Jul 27, 2026
Merged

Ship animation identity planes only when the client can key-match#330
Alek99 merged 1 commit into
mainfrom
alek/skip-unmatched-key-planes

Conversation

@Alek99

@Alek99 Alek99 commented Jul 26, 2026

Copy link
Copy Markdown
Member

The remaining item from the #319 review's perf-audit residual — but a different, more tractable half of it than the one I originally recorded.

What was wrong

match defaults to "index". So key= combined with a bare xy.animation(...), with enabled=False, or with no animation spec at all was putting two u32 identity columns into the payload that no client code reads.

chart spec keys shipped before ever read
animation(match="key") yes yes
animation(match="index") yes no
animation(duration=250) — match defaulted yes no
animation(enabled=False) yes no
no animation(...) at all yes no

At 50k rows the payload was half dead key columns: 800,000 → 400,000 bytes, plus 400 KB retained for the widget lifetime.

What this does not change

Encoding still runs in every case. Duplicate-key and row-count errors are construction contract, not animation policy, and _encode_transition_keys materializes as part of validating — so the saving is retention and transport, not CPU. Tests assert both error classes still fire in all three now-skipped shapes.

Payloads that key-match are byte-identical. Verified by hashing spec and blob across the merge base and this branch:

match=key     spec 0d46cee54989e43a   blob 2e3a2ed38bad6748    <- both branches
match=index   blob 800000 -> 400000 bytes, retained 400000 -> 0

It surfaced a worse bug — filed as #329

Animation.to_spec() emits every field, and both the Python merge and the client's {...spec.animation, ...trace.animation} are plain dict spreads. So a mark-level xy.animation(duration=90) resets match, easing, enter, update, and interpolate to defaults:

xy.scatter_chart(
    xy.scatter(x=x, y=y, key=keys, animation=xy.animation(duration=90)),
    xy.animation(match="key", easing="linear"),
)
# resolved: match="index", easing="ease-out"

That silently turns off chart-level match="key", and suppresses the match='key' requires key= guard entirely. It was invisible before this change because keys shipped whether or not the resolved spec could use them.

test_aggregate_tier_records_key_matching_fallback was written in exactly that shape and passed for that reason; it now restates match="key" to express its intent, with a pointer to #329. test_mark_animation_spec_clobbers_chart_level_fields pins the buggy behavior so a fix is deliberate rather than silent.

The two compose safely. key_matching reads the same effective dict #329 would correct, so fixing the merge automatically restores plane shipping wherever key matching starts working again.

Verification

  • pytest tests/ — 2574 passed, 4 skipped
  • ruff@0.15.8 check / format --check clean
  • Payload spec+blob hash comparison across five animation configurations, before and after
  • Browser smoke assertions on animation_fallback all operate on match="key" payloads, so they are untouched

Not done, deliberately

The other half of the perf-audit residual — keys encoded for rows discarded above MAX_ANIMATION_MATCH_ROWS — stays open. Finite-row filtering means n_points > 200k does not guarantee the shipped count exceeds the limit, so it genuinely cannot be decided at attach time. Parallelizing the kernel would optimize exactly that discarded work, so it should wait on this being resolved.

Summary by CodeRabbit

  • Bug Fixes

    • Removed unused animation identity data from payloads when key-based matching is unavailable.
    • Preserved key validation and existing payload output for animations that support key matching.
    • Prevented unnecessary sorting and row reordering when identity keys are not used.
  • Documentation

    • Clarified when animation identity data is retained and transmitted.
    • Documented default matching behavior and key-based animation scenarios.

`match` defaults to "index", so `key=` combined with a bare
`xy.animation(...)`, with `enabled=False`, or with no animation spec at
all was putting two u32 identity columns into the payload that no client
code reads — 8 B/row retained for the widget lifetime and 8 B/row on the
wire. At 50k rows the payload was literally half dead key columns:
800,000 -> 400,000 bytes.

Encoding still runs in every case. Duplicate-key and row-count errors
are construction contract, not animation policy, and `_encode_transition
_keys` materializes as part of validating, so the saving is retention
and transport rather than CPU. The line-like geometry sort is skipped
along with the planes.

Payloads that do key-match are byte-identical — verified by hashing spec
and blob across the merge base and this branch:

  match=key     spec 0d46cee54989e43a  blob 2e3a2ed38bad6748  (both)
  match=index   blob 800000 -> 400000 bytes, retained 400000 -> 0
  bare animation()/enabled=False/no animation: same as match=index

This surfaced a separate pre-existing bug, filed as #329 and pinned by
test_mark_animation_spec_clobbers_chart_level_fields: `Animation
.to_spec()` emits every field, and both the Python merge and the
client's `{...spec.animation, ...trace.animation}` are plain dict
spreads, so a mark-level `xy.animation(duration=90)` resets `match`,
`easing`, `enter`, `update`, and `interpolate` to defaults. That
silently turns off a chart-level `match="key"` and suppresses the
`match='key' requires key=` guard. It was invisible before this change
because keys shipped regardless of whether the resolved spec could use
them; test_aggregate_tier_records_key_matching_fallback was written in
exactly that shape and now restates `match="key"` to express its intent.

The two fixes compose: `key_matching` reads the same `effective` dict
that #329 would correct, so fixing the merge restores plane shipping
wherever key matching starts working again.

Verified: 2574 passed / 4 skipped; ruff check and format clean.
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7d23dd1a-dbcc-44b9-9553-b16545e94f5c

📥 Commits

Reviewing files that changed from the base of the PR and between 7c377ce and 25c3005.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • python/xy/components.py
  • spec/design/animation.md
  • tests/test_animation.py

📝 Walkthrough

Walkthrough

Key identity planes are now retained and transmitted only when the resolved animation performs key matching. Key validation remains active, while line-like key sorting is skipped for non-key-matching animations. Tests and documentation cover the updated behavior.

Changes

Animation key-plane retention

Layer / File(s) Summary
Effective key-matching transition metadata
python/xy/components.py
Transition-key sorting and trace attachment now depend on effective enabled and match settings, while encoded key length validation remains active.
Key-plane behavior validation and documentation
tests/test_animation.py, spec/design/animation.md, CHANGELOG.md
Tests cover conditional transmission, validation preservation, matching payload consistency, and aggregation fallback; documentation records the updated stable-identity contract.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Possibly related PRs

  • reflex-dev/xy#321 — Touches the transition-key encoding pipeline in python/xy/components.py.

Suggested reviewers: carlosabadia

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main change: only shipping animation identity planes when key matching is possible.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch alek/skip-unmatched-key-planes

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

@codspeed-hq

codspeed-hq Bot commented Jul 26, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 103 untouched benchmarks
⏩ 2 skipped benchmarks1


Comparing alek/skip-unmatched-key-planes (25c3005) with main (7c377ce)

Open in CodSpeed

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Alek99
Alek99 merged commit 3746302 into main Jul 27, 2026
28 checks passed
Alek99 added a commit that referenced this pull request Jul 27, 2026
…ng it (#331)

Fixes #329.

`Animation.to_spec()` emits every field, and both the Python merge in
`_attach_keys` and the client's `_resolvedAnimation` are plain dict
spreads. So a mark-level `xy.animation(duration=90)` was a complete spec
spread over the chart's, resetting `match`, `easing`, `enter`, `update`,
and `interpolate` to their defaults. Chart-level `match="key"` silently
became index matching — no error, no fallback string — and the
`match='key' requires key=` guard stopped firing in that configuration
because it reads the same clobbered dict.

`animation()` now records the caller's exact argument names behind
sentinel defaults, so "set to the default" is distinguishable from "not
set": a mark that passes `match="index"` against a chart's
`match="key"` still wins. `to_override_spec()` returns just those
fields. Direct `Animation(...)` construction is public too and falls
back to diffing the defaults, which is right except for that same
explicit-default case.

The cascade resolves once, in Python, over a defaults base:

    {**defaults, **chart_spec, **mark_override}

and a trace that carries an override ships the complete resolved policy.
Shipping the sparse override instead would break a mark-level animation
on a chart with no chart-level one — the client would read `undefined`
for duration and easing. Resolving here means the client's spread stays
a no-op rather than a second merge needing its own copy of the defaults.
A trace with no override still ships no animation dict, and a chart with
neither ships none, which is what keeps an unanimated chart static.

Verified with a 59-combination cascade matrix (6 chart specs x 10 mark
overrides including bools and None), each replaying the client's exact
spread and checking all 8 fields plus whether identity planes ship.
Identity-plane shipping tracks the resolved `match`, so this composes
with #330: key matching that starts working again also gets its planes
back.

Three tests asserted the old behavior as a literal and now assert the
cascade. test_mark_animation_spec_clobbers_chart_level_fields, added in
#330 to pin the bug, becomes the regression test for the fix.

Verified: 2578 passed / 4 skipped; ruff check and format clean; ty at
the 14-diagnostic baseline with an identical diagnostic set; public API
verification OK.
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.

1 participant