Skip to content

fix(charts): scope focus guide gradient paints with idPrefix - #217

Open
getdownandcode wants to merge 1 commit into
TanStack:mainfrom
getdownandcode:fix/focus-guide-id-prefix
Open

getdownandcode wants to merge 1 commit into
TanStack:mainfrom
getdownandcode:fix/focus-guide-id-prefix

Conversation

@getdownandcode

@getdownandcode getdownandcode commented Oct 13, 2026 •

Copy link
Copy Markdown

Fixes #211

What changed & why

When a chart is configured with idPrefix, <defs> gradients are scoped (e.g. <linearGradient id="chart-a-fade">), and renderChartSvg rewrites mark paints via resolvePaint. However, transient focus guides rendered via renderFocusGuideLayer used default hooks without resolvePaint, leaving url(#id) references unprefixed on hover:

- <line class="ts-chart-focus-guide" stroke="url(#fade)" ... />
+ <line class="ts-chart-focus-guide" stroke="url(#chart-a-fade)" ... />

Changes

  • Added createFocusGuideRenderHooks(scene) in svg-focus-guide-layer.ts to scope url(#id) paints against scene.gradients (returns undefined when the scene has no gradients).
  • Cached the hooks per scene update in svg-surface.ts and motion.ts so no Set or closure allocations happen during high-frequency pointer hover events.
  • Left static SVG document rendering (svg.ts / svg-renderer.ts) untouched, preserving isolated static SVG bundle boundaries.
  • Updated universal-baseline.json and adjusted 7 derivative bundle ceilings in scripts/measure-bundles.mjs to reflect the small helper cost (~70 B gzip on DOM host).
  • Added regression tests in svg-surface.test.ts and motion.test.ts.
  • Added a patch changeset for @tanstack/charts.
Validation output
✓ packages/charts-core/src/svg-surface.test.ts (14 tests)
✓ packages/charts-core/src/motion.test.ts (66 tests)
Test Files  2 passed (2)
Tests  80 passed (80)

Bundle size policy passed.
Framework package gate passed for 7 adapters.
Unified @tanstack/charts artifact gate passed.

Summary by CodeRabbit

  • Bug Fixes
    • Focus guides and crosshairs now correctly reference chart-specific gradients when an idPrefix is configured.

Scope focus guide and crosshair url(#id) gradient paint references when an idPrefix is configured on both static SVG and motion surfaces.

Fixes TanStack#211
@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Review in 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: d0aa82c4-da71-40a5-9350-53ff6770b0a8
📥 Commits

Reviewing files that changed from the base of the PR and between 57774e1 and ae1703f.

📒 Files selected for processing: 8
  • .changeset/focus-guide-id-prefix-paint.md
  • benchmarks/bundle-size/universal-baseline.json
  • packages/charts-core/src/motion.test.ts
  • packages/charts-core/src/motion.ts
  • packages/charts-core/src/svg-focus-guide-layer.ts
  • packages/charts-core/src/svg-surface.test.ts
  • packages/charts-core/src/svg-surface.ts
  • scripts/measure-bundles.mjs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Focus-guide paint references that match scene gradient IDs now use the chart’s sanitized idPrefix. The SVG surface and motion renderer pass render hooks when painting focus guides. Tests cover both rendering paths.

Changes: Focus-guide paint scoping

Layer / File(s) Summary
Create hooks and wire the SVG surface
packages/charts-core/src/svg-focus-guide-layer.ts, packages/charts-core/src/svg-surface.ts, packages/charts-core/src/svg-surface.test.ts
The hook factory rewrites matching gradient paint references using the sanitized prefix. The default SVG surface renderer passes the hooks to focus-guide rendering. A test checks the prefixed crosshair gradient reference.
Wire motion rendering and update records
packages/charts-core/src/motion.ts, packages/charts-core/src/motion.test.ts, .changeset/focus-guide-id-prefix-paint.md, benchmarks/bundle-size/universal-baseline.json, scripts/measure-bundles.mjs
The motion renderer passes hooks to focus-guide painting and markup rendering. A test checks the prefixed gradient reference. The changeset and bundle-size records are updated.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: tannerlinsley

Merge Risk: ⚪ Minimal · up to ae170

The verified rendering paths keep focus-guide gradient references aligned with the chart’s resources. No actionable merge-blocking risk remains after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 6 files. (2 skipped: 2 … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed 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 main change: scoping focus-guide gradient paints with the configured idPrefix.
Linked Issues check Passed Issue #211 requires scoped url(#id) paints for focus guides and crosshairs when idPrefix is set. The PR adds createFocusGuideRenderHooks(scene), matches gradient IDs from scene.gradients, sani…
Out of Scope Changes check Passed The changes remain connected to issue #211. The new tests verify the fix. The changeset documents the fix. The bundle baselines and ceilings account for the helper cost. No unrelated product behavior …
Full details: Docstring Coverage

Explanation

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

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@changeset-bot

changeset-bot Bot commented Oct 13, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ae1703f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@tanstack/charts Patch
@tanstack/alpine-charts Patch
@tanstack/angular-charts Patch
@tanstack/lit-charts Patch
@tanstack/octane-charts Patch
@tanstack/preact-charts Patch
@tanstack/react-charts Patch
@tanstack/react-native-charts Patch
@tanstack/solid-charts Patch
@tanstack/svelte-charts Patch
@tanstack/vue-charts Patch
@tanstack/charts-scales Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tannerlinsley

Copy link
Copy Markdown
Member

Thanks @getdownandcode, I verified the gradient fix at ae1703f. All 72 cases passed in Chromium, Firefox and WebKit, covering both SVG renderers, sanitized prefixes, gradient removal and re-addition, marker fills, and unrelated paint references. The same checks reproduce 12 failures per browser on main.

The remaining concern is bundle cost. A paired check of the changed runtime files across 114 entry points found increases up to 0.095 kB gzip and 0.096 kB Brotli. This is a targeted check, not the full repository validation. The proposed budget increases still need approval before merging.

I also tried sharing the existing SVG paint resolver. That reduced the maximum gzip increase to 0.048 kB, but affected more entry points and increased the worst Brotli delta to 0.114 kB, so I haven't applied that experiment to your PR. Keeping #211 open until the fix clears the remaining checks and lands.

@getdownandcode

Copy link
Copy Markdown
Author

@tannerlinsley Thanks for verifying the tests and bundle size! Glad all 72 cases passed. I'll let the remaining checks finish—let me know if you need any other changes before this lands.

@tannerlinsley

tannerlinsley commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Thanks @getdownandcode, the isolated checkout and package checks below are complete at ae1703f. All 2,214 unit tests and 23 tests across the six additional framework configurations pass, along with TypeScript and the docs contract. The packed-package gate also passes, including bare React Native, Expo, seven adapters and the unified artifact.

I reran the 72 focus-guide cases against the final packed exports in Chromium, Firefox and WebKit. All pass, covering sanitized prefixes, gradient removal and re-addition, marker fills and unrelated paint references. The negative control fails as expected.

The bundle check passes against this PR’s proposed limits, but that does not approve those increases. The budget decision and required CI remain separate gates, so #211 stays open until the fix lands. No limits were changed during this verification.

Remaining repository checks: the catalog-preview fingerprint is stale at this head. I rendered all 188 cases in both themes from the isolated production build and regenerated their portable SVGs. Every SVG is byte-identical to the committed asset, so only manifest sourceHash needs to become 84b32be4f1786eb915ea8c9bea859339705ce3df101ec090924db31031b1f4b9. That local correction passes the preview gate. The native declarations, catalog index/examples and conformance build also pass. The full comparison check fails only on its recorded input fingerprint (8444bc4f… versus measured 6afbd8b3…), not size tolerances. I have not changed comparison records or budgets. These results are not an all-CI-green claim.

Verified head: ae1703f46a1042fec0c3f657c4ce9d7140a5d4cb.

@getdownandcode

Copy link
Copy Markdown
Author

Makes total sense on keeping the budget decision as a separate gate. Really appreciate you running such thorough checks across the full test suite and packed exports. @tannerlinsley, Happy to hold tight until the team signs off on it.

This branch has not been deployed

No deployments
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.

[Bug] Focus guides: url(#id) paints ignore idPrefix

2 participants