Skip to content

Fix locally-imported SVG files (direct or barrel-re-exported) missing from Session Replay - #1350

Merged
jonathanmos merged 7 commits into
developfrom
jmoskovich/fix-svg-not-captured
Jul 28, 2026
Merged

Fix locally-imported SVG files (direct or barrel-re-exported) missing from Session Replay#1350
jonathanmos merged 7 commits into
developfrom
jmoskovich/fix-svg-not-captured

Conversation

@jonathanmos

Copy link
Copy Markdown
Member

What does this PR do?

Inline ... JSX shapes always worked correctly. But an SVG referenced via a separate file import — import Logo from './logo.svg', or re-exported through a barrel (export { default as Logo } from './logo.svg') — was never captured in Session Replay, in any build. Root cause: buildSvgMap(), the scan that resolves these file imports, never actually ran due to an initialization-order bug, plus two compounding issues that would have kept it broken even after that.

Changes

  • Call setApiTypes before buildSvgMap in the plugin's pre() hook and the generate-sr-assets CLI — buildSvgMap() silently no-ops without this, so the scan for local .svg imports never executed.
  • Fix barrel re-export naming: export { default as Logo } from './icon.svg' was keyed under 'default' instead of 'Logo', so aliased re-exports would still have gone unmatched even with the scan running.
  • Reuse the ReactNativeSVG instance across files in pre() instead of rebuilding (and rescanning) it per file.
Screenshot 2026-07-26 at 15 09 36 Screenshot 2026-07-26 at 15 09 56

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)
  • If this PR is auto-generated, please make sure also to manually update the code related to the change

buildSvgMap() is a no-op until setApiTypes() has been called (it
guards on this.t). Both the plugin's pre() hook and the
generate-sr-assets CLI called buildSvgMap() first, so the project-wide
SVG scan silently produced an empty map on every real build.
For `export { default as Logo } from './icon.svg'`, the scan keyed
the map entry on spec.local.name ('default') instead of spec.exported
('Logo') -- the name consumers actually import and render as <Logo/>.
Aliased re-exports were therefore never matched.
@datadog-datadog-us1-prod

datadog-datadog-us1-prod Bot commented Jul 27, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: a10ebaa | Docs | Datadog PR Page | Give us feedback!

@jonathanmos
jonathanmos marked this pull request as ready for review July 27, 2026 07:57
@jonathanmos
jonathanmos requested a review from a team as a code owner July 27, 2026 07:57
Copilot AI review requested due to automatic review settings July 27, 2026 07:57
@jonathanmos
jonathanmos requested a review from cdn34dd July 27, 2026 07:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes Session Replay SVG asset extraction for SVGs imported from local .svg files (including via barrel re-exports) by ensuring the SVG import scan actually runs and by correcting how re-exported names are keyed.

Changes:

  • Ensure setApiTypes() is invoked before buildSvgMap() so the local .svg import scan doesn’t silently no-op.
  • Fix barrel re-export mapping so export { default as Logo } from './icon.svg' is keyed under Logo (not default).
  • Add/extend tests covering buildSvgMap() population for default/named imports and re-exports.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
packages/react-native-babel-plugin/test/react-native-svg.test.ts Adds tests validating buildSvgMap() behavior for imports and re-exports.
packages/react-native-babel-plugin/src/libraries/react-native-svg/index.ts Fixes exported-name handling for SVG barrel re-exports during scan.
packages/react-native-babel-plugin/src/index.ts Adjusts plugin initialization order and attempts to reuse the SVG processor instance.
packages/react-native-babel-plugin/src/cli/generate-sr-assets.ts Initializes SVG scan in the CLI by setting Babel types before building the map.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/react-native-babel-plugin/src/index.ts Outdated
Comment thread packages/react-native-babel-plugin/src/cli/generate-sr-assets.ts Outdated
Comment thread packages/react-native-babel-plugin/test/react-native-svg.test.ts Outdated
pre() assigned options.__internal_reactNativeSVG directly with no
fallback, so in a real build (no injected instance) every file
discarded the previous scan and rebuilt/rescanned from scratch.
pre() runs before Program.enter's own platform === 'web' check, so
the scan ran regardless of platform even though nothing downstream
uses its results for web (Program.enter/JSXElement skip immediately).
Before this fix's ordering change, buildSvgMap() was a no-op there by
accident; now that it actually runs, web builds would otherwise pay
for a full project scan they never needed.

Also adds a regression test proving pre() reuses the same
ReactNativeSVG instance across files (only one buildSvgMap() call for
two files processed by the same plugin instance).
buildSvgMap() used its own hardcoded, much shorter ignore list and
never considered followSymlinks, so generate-sr-assets could scan
directories (build output, native folders, user --ignore targets)
that its own file-processing loop was told to skip -- doing
significantly more work than necessary.
Copilot AI review requested due to automatic review settings July 27, 2026 11:19
@jonathanmos
jonathanmos force-pushed the jmoskovich/fix-svg-not-captured branch from 0ef8dc9 to 9372190 Compare July 27, 2026 11:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment thread packages/react-native-babel-plugin/src/libraries/react-native-svg/index.ts Outdated
Comment thread packages/react-native-babel-plugin/src/index.ts
The same pattern was listed twice, causing fast-glob to match it
against the filesystem twice for no benefit.
svgMap accumulated one entry per processed SVG element (write-only,
never read anywhere) and svgOffset was never read or written at all.
Now that pre() reuses the ReactNativeSVG instance across files, this
dead state would grow unboundedly for the life of the worker instead
of being discarded per file -- removing it entirely rather than
resetting it per file, since nothing consumes it.
Copilot AI review requested due to automatic review settings July 27, 2026 12:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@cdn34dd

cdn34dd commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

@jonathanmos Since we're now making improvements to this babel code, this may be worth looking into:
// TODO: Support aliased paths (RUM-12185)

@jonathanmos
jonathanmos merged commit 9035334 into develop Jul 28, 2026
12 checks passed
@jonathanmos
jonathanmos deleted the jmoskovich/fix-svg-not-captured branch July 28, 2026 09:06
@cdn34dd cdn34dd mentioned this pull request Jul 28, 2026
4 tasks
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.

3 participants