Skip to content

fix(APP-993): Prefix SVG asset ids to prevent url(#id) collisions between inlined icons and illustrations - #723

Merged
milosh86 merged 2 commits into
mainfrom
app-993-character-illustrations-are-rendering-poorly
Jul 14, 2026
Merged

fix(APP-993): Prefix SVG asset ids to prevent url(#id) collisions between inlined icons and illustrations#723
milosh86 merged 2 commits into
mainfrom
app-993-character-illustrations-are-rendering-poorly

Conversation

@milosh86

Copy link
Copy Markdown
Contributor

Description

Human illustrations (e.g. the SMILE_WINK expression) render with broken, jagged linework whenever they share a page with common icons (MENU, CLOSE, COPY, nav icons, etc.).

Root cause: 57 icon SVGs and several illustration SVGs hardcode short ids like id="a" for their <clipPath>/<mask> defs. Since SVGR inlines these as raw <svg> markup, all ids share the document's id space — the browser resolves url(#a) to the first matching element in the DOM, so e.g. SMILE_WINK's face mask reference silently resolves to a 16×16 icon clip square instead.

SVGR's default SVGO config includes the prefixIds plugin (namespaces every id per source file, e.g. menu_svg__a), but passing any custom svgoConfig replaces the default wholesale rather than merging. #715 added a custom config to preserve viewBox and thereby silently dropped prefixIds, introducing the collision.

Changes:

  • Add a shared svgo.config.js (single source of truth) that keeps the removeViewBox: false override from fix: keep SVG viewBox so icons scale instead of clipping #715 and restores prefixIds.
  • rollup.config.mjs (library build) imports the shared config instead of its inline copy.
  • .storybook/main.ts: vite-plugin-svgr runs no SVGO at all by default, so Storybook previews didn't match the published build (and still showed the collision). It now opts into @svgr/plugin-svgo with the same shared config; @svgr/plugin-jsx must be re-listed since setting plugins replaces the default chain.
  • Add svgo, @svgr/plugin-svgo, @svgr/plugin-jsx devDependencies (referenced by name as plugin strings, so they must be root-resolvable under pnpm).

Verified by building and inspecting dist/index.es.js: all bundled SVG ids are unique (e.g. menu_svg__a, smile-wink_svg__a), zero duplicates.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Developer Checklist:

  • Manually smoke tested the functionality locally
  • Confirmed there are no new warnings or errors in the browser console
  • Made the corresponding changes to the documentation
  • Added tests that prove my fix is effective or that my feature works
  • Confirmed there are no new warnings on automated tests
  • Selected the correct base branch
  • Commented the code in hard-to-understand areas
  • Followed the code style guidelines of this project
  • Reviewed that the files changed in GitHub's UI reflect my intended changes
  • Confirmed the pipeline checks are not failing

Review Checklist:

  • Tested locally that all Acceptance Criteria or Expected Outcomes are satisfied
  • Confirmed that changes follow the code style guidelines of this project

Signed-off-by: Milos Dzepina <milos@aragon.org>
@linear-code

linear-code Bot commented Jul 14, 2026

Copy link
Copy Markdown

APP-993

@milosh86
milosh86 marked this pull request as ready for review July 14, 2026 07:37
@milosh86
milosh86 requested a review from a team as a code owner July 14, 2026 07:37
@arabot-1

arabot-1 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

🚀 Preview Deployment: View Here

@thekidnamedkd thekidnamedkd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Previews look the same so that's great, but doesn't 100% prove the solution on the app where the real issue lives with the IDs and render race. I assume you checked that though.

I'm personally less opposed to using packages, but historically we have always retained a high level of scrutiny as a posture before we add anything. We're there vanilla solutions possibly? Or conversely, is there anything more we can do with this package to earn its keep?

@milosh86

milosh86 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Previews look the same so that's great, but doesn't 100% prove the solution on the app where the real issue lives with the IDs and render race. I assume you checked that though.

@thekidnamedkd yes, of course, I checked it in the app. It was tricky to pin point the issue because it was visible only in the app, but the actual problem was in the ui-kit.

I'm personally less opposed to using packages, but historically we have always retained a high level of scrutiny as a posture before we add anything. We're there vanilla solutions possibly? Or conversely, is there anything more we can do with this package to earn its keep?

These are all required config packages. svgo was used internally, but not explicitly defined, and the other 2 are for Storybook so that it does the same SVG transformations as build step.

@thekidnamedkd
thekidnamedkd self-requested a review July 14, 2026 11:06

@thekidnamedkd thekidnamedkd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM amigo 👏

@milosh86
milosh86 merged commit c097ac7 into main Jul 14, 2026
6 checks passed
@milosh86
milosh86 deleted the app-993-character-illustrations-are-rendering-poorly branch July 14, 2026 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants