Use ?url imports on svgs to fix production build issue with rich text component icons

[danielnaab]

Fixes #24

[changeset-bot]

⚠️ No Changeset found

Latest commit: 7771bbc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[claude]

Pull Request Review

Summary

This PR fixes missing rich text editor icons in production builds by changing SVG imports to use the ?url query parameter. The change is correct and well-implemented.

✅ Positive Aspects

1. Root Cause Fix: The ?url suffix tells Vite to return the asset URL as a string rather than attempting to process the SVG as a module/component. This is the standard Vite approach for handling static assets that need to be referenced by their URL.

2. Type Safety Improvement: Removed the unsafe any type from the icons record:

   // Before
   const icons: Record<string, string | any> = { ... }
   
   // After
   const icons: Record<string, string> = { ... }

3. Simplified Logic: The getIconPath function no longer needs the hacky Object.values(icons[iconPath])[0] workaround:

   // Before
   const getIconPath = (iconPath: string) => {
     return Object.values(icons[iconPath])[0] as string;
   };
   
   // After
   const getIconPath = (iconPath: string) => {
     return icons[iconPath];
   };

🔍 Observations

1. Inconsistent Pattern: There's a similar SVG import pattern in AddPatternDropdown.tsx (lines 5-11) that imports SVGs without the ?url suffix:

   import blockIcon from './images/block-icon.svg';
   import checkboxIcon from './images/checkbox-icon.svg';
   // etc.

   However, that file uses a different approach - it mixes imported SVG assets with USWDS sprite references (e.g., '#attach_file'), and uses Object.values(iconValue)[0] for the imported SVGs (line 56). This suggests those imports might be experiencing the same issue, though they may work if only used in development builds or if the bundler handles them differently in that context.

2. Missing Type Definitions: The package doesn't have explicit TypeScript definitions for .svg?url imports. While Vite handles this at build time, you may want to add type definitions to eliminate TypeScript warnings:

   // types/vite-env.d.ts (or similar)
   /// <reference types="vite/client" />
   
   declare module '*.svg?url' {
     const content: string;
     export default content;
   }

🧪 Test Coverage

• Positive: The component has Storybook stories with interaction tests (RichTextPatternEdit.stories.tsx) that verify the formatting buttons work correctly.
• Note: The test uses getByRole('button', { name: buttonName }) which relies on the button's accessible name from the icon's alt attribute. Since the icons now load as URLs, this should continue working.

🎯 Recommendations

1. Consider applying the same fix to AddPatternDropdown.tsx if those icons are also missing in production builds. The pattern on lines 5-11 looks similar to what this PR fixes.

2. Add Vite type definitions for .svg?url imports to improve the TypeScript development experience.

3. Document the pattern: Consider adding a comment in the code or project documentation about the SVG import pattern:

   // Import SVGs as URL strings for use in <img src={...}> attributes
   // The ?url suffix tells Vite to return the asset URL
   import boldSvg from './images/format_bold.svg?url';

✅ Verdict

Approved - This is a clean, minimal fix that addresses the production build issue correctly. The code quality improvements (type safety and simplified logic) are welcome bonuses.

Code Quality: ⭐⭐⭐⭐⭐
Test Coverage: ⭐⭐⭐⭐ (Existing Storybook tests cover the functionality)
Security: ✅ No concerns
Performance: ✅ No concerns (URLs are resolved at build time)

---

Review performed with attention to CLAUDE.md conventions and monorepo architecture

[claude]

Code Review Summary

Overall Assessment: ✅ Good fix - this PR correctly addresses the production build issue with SVG imports.

---

✅ Strengths

1. Correct Solution: Using ?url imports is the standard Vite approach for importing assets as URL strings rather than inlined/transformed assets. This ensures SVGs are properly handled in production builds.

2. Type Safety Improvement: The change in RichTextPatternEdit.tsx:31 improves type safety:

   - const icons: Record<string, string | any> = {
   + const icons: Record<string, string> = {

   Removing the any type is a best practice.

3. Code Simplification: The getIconPath function is now cleaner:

   - return Object.values(icons[iconPath])[0] as string;
   + return icons[iconPath];

   This suggests the previous code was working around the incorrect import format.

4. Consistent Application: The fix is applied consistently across both affected files.

---

📝 Observations

1. No Tests Found: There are no unit tests or Storybook stories for these components. While this PR doesn't require adding tests (it's a build fix), consider adding visual regression tests for these UI components in the future.

2. Vite Configuration: The vite.config.ts doesn't explicitly configure asset handling, which is fine since Vite's defaults handle ?url imports correctly out of the box.

3. Production vs Development: This fix specifically addresses production builds. The difference occurs because:

   • Development mode: Vite serves assets directly
   • Production mode: Rollup bundles and processes assets, requiring explicit URL imports

---

🔍 No Issues Found

• ✅ Code Quality: Clean, minimal changes
• ✅ Best Practices: Follows Vite documentation for asset imports
• ✅ Performance: No performance impact (may even be slightly better by avoiding unnecessary processing)
• ✅ Security: No security concerns
• ✅ Type Safety: Actually improves type safety by removing any

---

💡 Recommendations for Future Work

1. Add Visual Tests: Consider adding Storybook stories for RichTextPatternEdit and AddPatternDropdown to catch visual regressions like missing icons.

2. Build Verification: Consider adding a CI check that builds the design package and verifies critical assets are present in the output.

3. Documentation: If this is a common pattern in the codebase, consider adding a note to CLAUDE.md about using ?url for SVG imports in the design package.

---

Verdict: This is a solid fix that correctly addresses issue #24. The code quality is good, and the changes actually improve type safety. No blocking issues found. ✅