Repository navigation
fix(charts): scope focus guide gradient paints with idPrefix - #217
getdownandcode wants to merge 1 commit into
Conversation
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
🦋 Changeset detectedLatest commit: ae1703f The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
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 |
|
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. |
|
@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. |
|
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 Verified head: |
|
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. |
Fixes #211
What changed & why
When a chart is configured with
idPrefix,<defs>gradients are scoped (e.g.<linearGradient id="chart-a-fade">), andrenderChartSvgrewrites mark paints viaresolvePaint. However, transient focus guides rendered viarenderFocusGuideLayerused default hooks withoutresolvePaint, leavingurl(#id)references unprefixed on hover:Changes
createFocusGuideRenderHooks(scene)insvg-focus-guide-layer.tsto scopeurl(#id)paints againstscene.gradients(returnsundefinedwhen the scene has no gradients).svg-surface.tsandmotion.tsso noSetor closure allocations happen during high-frequency pointer hover events.svg.ts/svg-renderer.ts) untouched, preserving isolated static SVG bundle boundaries.universal-baseline.jsonand adjusted 7 derivative bundle ceilings inscripts/measure-bundles.mjsto reflect the small helper cost (~70 B gzip on DOM host).svg-surface.test.tsandmotion.test.ts.@tanstack/charts.Validation output
Summary by CodeRabbit
idPrefixis configured.