Skip to content

chore: 🤖 add normalisation pass in SVG convertor process/tool#978

Open
punkbit wants to merge 8 commits intomainfrom
chore/add-optimization-pass-in-svg-convertor
Open

chore: 🤖 add normalisation pass in SVG convertor process/tool#978
punkbit wants to merge 8 commits intomainfrom
chore/add-optimization-pass-in-svg-convertor

Conversation

@punkbit
Copy link
Copy Markdown
Contributor

@punkbit punkbit commented Apr 10, 2026

Why?

SVG inputs can break conversion due to malformed markup. This adds a normalisation step (based on conservative optimisation to reduce chances of visual diff) to fix structural issues upfront.

How?

  • Add a step/pass in the convertion script
  • Before: SVG File → SVGR (SVGO) → React Component
  • After: SVG File → SVGO Normalize → Create temp file → SVGR (no SVGO) → React Component → Delete temp file

Preview?

Screenshot 2026-04-10 at 10 17 19 Screenshot 2026-04-10 at 10 17 08

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 10, 2026

🦋 Changeset detected

Latest commit: caa1eba

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

This PR includes changesets to release 1 package
Name Type
@clickhouse/click-ui 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

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 10, 2026

Claude finished @punkbit's task in 1m 36s —— View job


Code Review

Nice work on the normalization pass! The approach is solid — separating the SVGO normalization from SVGR's internal SVGO pass is a sensible fix for malformed SVG issues. A few things worth addressing:


🚨 Must Fix

  • .temp directory not gitignored: .scripts/js/.temp/ is created at runtime but isn't in .gitignore. If a developer runs git add . after a crashed run, temp files could get committed. Add an entry to .gitignore:
    .scripts/js/.temp/
    
    Fix this →

💡 Suggestions (non-blocking)

  • Silent catch block (convert-svg-to-react-component, line ~239): The empty catch when unlinking the temp SVGR config is silent. Should at least warn, consistent with the outer cleanup:

    } catch (error) {
      console.warn('⚠️ Failed to clean up temp SVGR config:', error.message);
    }
  • Confusing parameter name skipSvgo: Called as !skipNormalize, which inverts the semantic. When reading convertSvgToComponent(..., !skipNormalize), it's not obvious that "skip SVGO = normalization ran". A name like preNormalized or svgoAlreadyDone would make the intent clearer.

  • Deleted useful warning comment: The diff removes this comment from the --type flag extraction:

    // WARN: Do not change as this extracts and removes the --type flag from args if present, to allow args in package.json scripts

    Worth preserving — it explains a subtle side-effect for future maintainers.

  • Changeset type: This only changes a dev tool script, not the published library. A patch bump is technically harmless but arguably unnecessary unless the repo's policy requires it for any committed change.


  • Read CLAUDE.md and conventions
  • Read changed files
  • Analyze the normalization script
  • Post review findings

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 10, 2026

Claude finished @punkbit's task in 1m 15s —— View job


Code Review

Great follow-up — the previous must-fix (.gitignore) and most suggestions have been addressed. Here's the current state:


Previous findings resolved ✅

  • .scripts/js/.temp/ is now gitignored
  • Parameter renamed skipSvgopreNormalized — much clearer
  • The WARN comment on --type flag extraction is preserved
  • Outer temp file cleanup has a proper console.warn on failure

💡 One remaining minor issue

The inner catch block that cleans up the temp SVGR config file (.scripts/js/convert-svg-to-react-component, ~line 239) is still empty and silent:

} catch (error) {
}

This is the same pattern as the outer cleanup — which now correctly logs a warning. For consistency, it should do the same:

} catch (error) {
  console.warn('⚠️ Failed to clean up temp SVGR config:', error.message);
}

Fix this →


Note (non-blocking): The --regenerate path doesn't apply the normalization step — only single-file conversion does. This is likely intentional (regenerate re-processes already-converted files), but worth a comment in the code if that assumption ever changes.


Otherwise this is solid. The two-phase approach (SVGO normalize → SVGR without SVGO) is clean and the conservative plugin config (removeViewBox: false, convertPathData: false) is the right call for avoiding unintended visual diffs. 🙌

@workflow-authentication-public
Copy link
Copy Markdown
Contributor

📚 Storybook Preview Deployed

✅ Preview URL: https://click-29rd6mz5r-clickhouse.vercel.app

Built from commit: 4147810975810f0739aabe3ffce172fdfa0539c3

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