Skip to content

Fix missing SVGs from barrel re-exports and unbuilt local SVG map#1329

Draft
jonathanmos wants to merge 1 commit into
developfrom
jmoskovich/fix-svg-missing-in-replay
Draft

Fix missing SVGs from barrel re-exports and unbuilt local SVG map#1329
jonathanmos wants to merge 1 commit into
developfrom
jmoskovich/fix-svg-missing-in-replay

Conversation

@jonathanmos

Copy link
Copy Markdown
Member

What does this PR do?

This fixes SVGs going missing from Session Replay in cases beyond simple inline usage. The root cause was that ReactNativeSVG.buildSvgMap() was being called before setApiTypes() in both call sites (the Babel plugin's pre() hook and the generate-sr-assets CLI), and buildSvgMap() no-ops until setApiTypes() has run — so localSvgMap was silently never populated, meaning any SVG imported via a default/named import or a barrel re-export (rather than declared inline) never got wrapped for Session Replay tracking. On top of that, barrel re-exports like export { default as Logo } from './icon.svg' were keyed by the wrong name (default instead of Logo), so even a correctly-built map wouldn't match usages. The plugin also rebuilt and rescanned the whole project on every file instead of reusing the instance across a build.

On the Android side, SvgViewMapper returned an empty wireframe list whenever the hash, entry data, or child view was unavailable, making the entire element disappear from the replay instead of degrading gracefully. It now always emits a container ShapeWireframe (bounding box/border) up front, so a placeholder outline renders even when the SVG payload itself can't be resolved. Added Jest coverage for the map-building fixes (default/named/barrel imports, aliased/non-aliased re-exports, instance reuse) and updated the Kotlin mapper tests to assert the fallback wireframe instead of an empty result.

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

@jonathanmos

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dce270e341

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

);
// buildSvgMap() is a no-op until setApiTypes() has run.
reactNativeSVG.setApiTypes(api.types);
reactNativeSVG.buildSvgMap();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Scope SVG aliases to the current file

With buildSvgMap() now actually running before transforms, the plugin enables a project-wide localSvgMap keyed only by JSX tag name, while HandlerResolver.create() only checks localSvgMap[name] and does not verify that the current file imported that tag from the mapped SVG. In a project where any scanned file imports or re-exports Icon/Logo from an SVG, every <Icon />/<Logo /> in transformed files can be wrapped with that SVG asset, including normal React components or same-named SVG imports from other files, so Session Replay can record the wrong element/resource. The map needs to be scoped by source file/import binding (or resolved from the current file) before this global build is reused.

Useful? React with 👍 / 👎.

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