Replace svelte-hero-icons and @zipline-ai/icons with unplugin-icons
#133
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of icon management across the frontend project. The changes involve replacing the Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant PackageJSON as package.json
participant ViteConfig as vite.config.ts
participant Components as Svelte Components
Dev->>PackageJSON: Remove svelte-hero-icons
Dev->>PackageJSON: Add unplugin-icons
Dev->>ViteConfig: Configure Icons plugin
Dev->>Components: Update icon imports
Components->>Components: Replace Icon usage
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
sean-zlai
changed the title
unplugin-icons with @iconify-json/heroiconsunplugin-icons with @iconify-json/heroicons (WIP)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/package.json (1)
23-23: Consider documenting icon usage guidelines.Since we're introducing a new icon system, it would be helpful to document:
- When to use @iconify-json/heroicons vs svelte-hero-icons
- How to import and use icons in components
- Best practices for icon usage
Would you like me to help create documentation for the icon system usage?
Also applies to: 54-54
frontend/src/lib/components/NavigationBar/NavigationBar.svelte (2)
38-39: Remove commented import and verify icon size consistency.The commented import can be safely removed since we're now using the solid variant. Also, ensure that the 16px size is consistent with the design system, as the previous implementation used a dynamic size prop.
-// import IconMagnifyingGlass from '~icons/heroicons/magnifying-glass'; import IconMagnifyingGlass from '~icons/heroicons/magnifying-glass-16-solid';
38-39: LGTM! Clean implementation of unplugin-icons.The changes successfully integrate unplugin-icons while maintaining existing functionality:
- Proper usage of the new icon system
- Preserved styling and behavior
- Consistent with the PR objectives
Consider documenting the icon usage pattern in the project's frontend guidelines to ensure consistency across components.
Also applies to: 105-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
frontend/package.json(2 hunks)frontend/src/lib/components/NavigationBar/NavigationBar.svelte(2 hunks)frontend/tsconfig.json(1 hunks)frontend/vite.config.ts(1 hunks)
🔇 Additional comments (5)
frontend/vite.config.ts (1)
3-3: LGTM! Plugin configuration looks correct.
The unplugin-icons integration is properly configured with the Svelte compiler option. The plugin placement after sveltekit() in the plugins array is appropriate since SvelteKit's plugin should generally be first.
Also applies to: 6-11
frontend/tsconfig.json (1)
12-15: LGTM! TypeScript configuration is properly set up.
The type definition for unplugin-icons is correctly configured. The moduleResolution is appropriately set to "bundler" which is compatible with the added types.
frontend/package.json (2)
54-54: Verify the unplugin-icons version for security.
Let's check if version 0.22.0 is the latest stable version and free from known vulnerabilities.
✅ Verification successful
The unplugin-icons version 0.22.0 is secure and up-to-date
The package is using the latest available version (0.22.0) and there are no known security vulnerabilities reported in the GitHub Security Advisory Database.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest version and security advisories
curl -s https://registry.npmjs.org/unplugin-icons/latest | jq '.version'
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "unplugin-icons") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 429
23-23: Verify compatibility with existing icon package.
I notice that svelte-hero-icons is already present in devDependencies. Consider if both packages are necessary or if one should be removed to avoid duplication.
✅ Verification successful
No duplication concern - packages serve different purposes
The added @iconify-json/heroicons package is a dependency of unplugin-icons and is used differently from svelte-hero-icons. From the codebase analysis:
svelte-hero-iconsis used for component-based icons (e.g.,import { Icon, Plus } from 'svelte-hero-icons')@iconify-json/heroiconswithunplugin-iconsis used for direct icon imports (e.g.,import IconMagnifyingGlass from '~icons/heroicons/magnifying-glass')
Both packages are correctly placed in devDependencies and serve complementary purposes in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential icon component duplications
rg -l "heroicons" --type svelte
Length of output: 62
Script:
#!/bin/bash
# Check for icon-related imports and usage in all files
rg -l "heroicons|svelte-hero-icons"
# Check package.json for existing icon-related dependencies
cat frontend/package.json | jq '.dependencies,.devDependencies' | grep -i "icon"
Length of output: 1055
Script:
#!/bin/bash
# Check how these icon packages are being used in the codebase
rg "@zipline-ai/icons|svelte-hero-icons|heroicons" -B2 -A2
# Check if there are any imports from both packages in the same file
rg "import.*(@zipline-ai/icons|svelte-hero-icons|heroicons)" --type ts --type svelte
Length of output: 14269
frontend/src/lib/components/NavigationBar/NavigationBar.svelte (1)
105-106: Remove commented code and verify icon styling consistency.
The old implementation is commented out and can be safely removed. However, let's verify the styling consistency:
- The old implementation used
microprop and explicitsize="16"- ensure the new 16px solid icon maintains visual consistency with other icons in the navigation bar. - The
text-foregroundclass is preserved, maintaining the color scheme.
✅ Verification successful
Remove commented code - icon styling is consistent
The commented code can be safely removed as the new implementation maintains consistency with the codebase's icon patterns:
- The new
IconMagnifyingGlassis imported from~icons/heroicons/magnifying-glass-16-solid, which follows the pattern of using 16px solid icons - The
text-foregroundclass is preserved, maintaining color consistency with other navigation elements - Multiple instances of
size="16"withmicroprop are found throughout the codebase, and the new solid icon implementation inherently includes this sizing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent icon usage patterns in the codebase
# Look for other icon size definitions to verify consistency
rg -A 1 'size="16"' --type svelte
# Check for other unplugin-icons usage patterns
rg "from '~icons/heroicons/.+-16-" --type svelte
Length of output: 151
Script:
#!/bin/bash
# Description: Check for consistent icon usage patterns in the codebase without file type filters
# Look for other icon size definitions to verify consistency
rg -A 1 'size="16"'
# Look for other IconMagnifyingGlass usages
rg "IconMagnifyingGlass"
# Look for other icon imports from heroicons
rg "from '~icons/heroicons"
# Look for other Icon component usages with MagnifyingGlass
rg "Icon.*MagnifyingGlass"
Length of output: 6625
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
frontend/src/lib/components/BreadcrumbNav/BreadcrumbNav.svelte (1)
23-23: LGTM! Improved icon usage syntaxThe new
<IconSlash />syntax is more concise and type-safe compared to the previous implementation. This change simplifies the code while maintaining the same functionality.Consider adding a comment in the PR description mentioning that the
vscode-iconifyplugin provides autocompletion for these new icon imports, which could help other developers working with this new pattern.frontend/src/lib/components/CollapsibleSection/CollapsibleSection.svelte (2)
45-45: LGTM! Consider organizing Tailwind classesThe icon implementation and animation logic work correctly. For better readability, consider organizing the Tailwind classes.
-<IconChevronDown class="transition-transform duration-200 {open ? '' : 'rotate-180'}" /> +<IconChevronDown class="transition-transform duration-200 {open ? 'rotate-0' : 'rotate-180'}" />
Line range hint
8-45: Icon system migration improves maintainabilityThe transition to
unplugin-iconswith@iconify-json/heroiconsbrings several benefits:
- Better maintainability through direct imports
- Improved type safety
- Simplified icon usage syntax
- Access to a wider range of icons through the Iconify ecosystem
frontend/src/lib/components/CustomEChartLegend/CustomEChartLegend.svelte (1)
123-125: Consider standardizing transition durations.The implementation looks good! The icon rotation animation is well-implemented. However, I notice that the parent
Buttonuses different transition durations for opacity (150ms) compared to the icon rotation (200ms).Consider standardizing the duration for consistency:
- class={cn('ml-2 transition-transform duration-200', isExpanded && '-rotate-180')} + class={cn('ml-2 transition-transform duration-150', isExpanded && '-rotate-180')}frontend/src/lib/components/PageHeader/PageHeader.svelte (1)
20-21: Consider adding hover state styling.The icon implementation looks good and maintains accessibility with the text label. However, you might want to consider matching the hover state color of the icon with the text for a more cohesive user experience.
-<IconBookOpen class="text-neutral-800" /> +<IconBookOpen class="text-neutral-800 group-hover:text-neutral-900" /> -<span class="text-neutral-800">Learn</span> +<span class="text-neutral-800 group-hover:text-neutral-900">Learn</span>Note: Add the
groupclass to the Button component if not already present.frontend/src/lib/types/Entity/Entity.ts (1)
1-21: Well-structured implementation of unplugin-icons.The changes effectively transition from svelte-hero-icons to unplugin-icons while maintaining type safety and the existing entity configuration structure. The consistent naming convention and explicit size specifications in imports enhance code clarity.
Consider documenting the icon import pattern (e.g., in a README or contributing guide) to maintain consistency as more icons are added to the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (3)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/src/lib/icons/chart-line.svgis excluded by!**/*.svgfrontend/src/lib/icons/chart-skew.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
frontend/package.json(2 hunks)frontend/src/lib/components/ActionButtons/ActionButtons.svelte(3 hunks)frontend/src/lib/components/BreadcrumbNav/BreadcrumbNav.svelte(2 hunks)frontend/src/lib/components/CollapsibleSection/CollapsibleSection.svelte(2 hunks)frontend/src/lib/components/CustomEChartLegend/CustomEChartLegend.svelte(2 hunks)frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte(2 hunks)frontend/src/lib/components/InfoTooltip/InfoTooltip.svelte(1 hunks)frontend/src/lib/components/NavigationBar/NavigationBar.svelte(8 hunks)frontend/src/lib/components/PageHeader/PageHeader.svelte(2 hunks)frontend/src/lib/components/ResetZoomButton/ResetZoomButton.svelte(1 hunks)frontend/src/lib/components/SplitView/SplitView.svelte(2 hunks)frontend/src/lib/types/Entity/Entity.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.svelte(3 hunks)frontend/vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/package.json
- frontend/src/lib/components/NavigationBar/NavigationBar.svelte
🔇 Additional comments (22)
frontend/src/lib/components/BreadcrumbNav/BreadcrumbNav.svelte (1)
11-11: LGTM! Clean migration to unplugin-icons
The import statement has been successfully updated to use the new icon system, aligning with the PR's objective to replace svelte-hero-icons with unplugin-icons.
frontend/src/lib/components/ActionButtons/ActionButtons.svelte (2)
4-8: LGTM! Icon imports have been updated correctly.
The migration from svelte-hero-icons to direct imports from ~icons/heroicons follows the PR objectives of setting up unplugin-icons.
49-49: LGTM! Icon usage has been updated consistently.
The component template has been updated to use the new icon components directly, maintaining the same functionality while aligning with the new icon system.
Also applies to: 56-56, 65-65, 70-70, 75-75
frontend/src/routes/joins/[slug]/+page.svelte (2)
6-7: LGTM! Icon imports have been updated correctly.
The migration from svelte-hero-icons and @zipline-ai/icons to direct imports from ~icons/heroicons and ~icons/zipline-ai follows the PR objectives.
568-568: LGTM! Icon usage has been updated consistently.
The TabsTrigger components have been updated to use the new icon components directly, maintaining the same functionality while aligning with the new icon system.
Also applies to: 572-572
frontend/src/lib/components/SplitView/SplitView.svelte (2)
62-62: LGTM! Icon usage simplified.
The icon implementation has been simplified by directly using the IconXMark component while maintaining the same styling.
3-3: LGTM! Icon import updated correctly.
The import change aligns with the PR's objective to transition from svelte-hero-icons to unplugin-icons.
Let's verify the consistency of icon imports across the codebase:
✅ Verification successful
Icon imports have been consistently updated across the codebase
The verification shows:
- No remaining imports from
svelte-hero-iconswere found - All heroicons are now consistently imported using the
~icons/heroicons/pattern - The new imports follow a consistent naming convention with
Iconprefix
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining svelte-hero-icons imports and verify the pattern of new imports
# Check for any remaining old imports
echo "Checking for remaining svelte-hero-icons imports:"
rg "from 'svelte-hero-icons'"
# Verify the pattern of new imports
echo "Verifying new icon import pattern:"
rg "from '~icons/heroicons/"
Length of output: 3240
frontend/src/lib/components/ResetZoomButton/ResetZoomButton.svelte (2)
10-10: LGTM! Cleaner icon implementation.
The direct usage of <IconArrowUturnLeft /> is more straightforward than the previous approach using <Icon> with props.
4-4: LGTM! Clean import using unplugin-icons.
The new import syntax is more direct and aligns with the PR's objective of transitioning to unplugin-icons.
Let's verify the icon path is correctly configured in Vite:
✅ Verification successful
Icon setup is correctly configured ✓
The verification confirms that:
unplugin-iconsis properly set up in the Vite config@iconify-json/heroiconsis correctly installed as a dev dependency (v1.2.2)- The import path
~icons/heroiconsfollows the standard unplugin-icons convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Vite config includes unplugin-icons setup
rg -l "unplugin-icons" frontend/vite.config*
# Verify the heroicons package is properly installed
cat frontend/package.json | jq '.dependencies["@iconify-json/heroicons"]'
Length of output: 151
Script:
#!/bin/bash
# Let's check the Vite config to ensure unplugin-icons is properly configured
rg -A 5 "unplugin-icons" frontend/vite.config.ts
# Check if heroicons might be in devDependencies instead
cat frontend/package.json | jq '.devDependencies["@iconify-json/heroicons"]'
# Check if there are any other icon-related packages that might be relevant
cat frontend/package.json | jq '.dependencies, .devDependencies' | rg -i "icon|hero"
Length of output: 434
frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte (3)
13-13: LGTM: Import change aligns with the migration to unplugin-icons
The new import statement correctly uses the unplugin-icons convention and provides more specific icon metadata (size and style) in the import path.
134-134: LGTM: Simplified icon usage with unplugin-icons
The icon implementation is now more straightforward, eliminating the need for additional props while maintaining the same functionality.
13-13: Verify icon rendering and migration consistency
Let's ensure the icon migration pattern is consistent across the codebase and that the icon renders correctly.
Also applies to: 134-134
✅ Verification successful
Based on the script results, I can now generate the final response:
Icon migration pattern is consistent and properly implemented
The verification shows:
- No remaining usage of
svelte-hero-iconsin the frontend - Consistent usage of unplugin-icons with the
~icons/heroicons/pattern - The calendar icon follows the same size convention (16) and style (solid) as other icons in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent icon migration patterns and potential issues
# Check for any remaining svelte-hero-icons usage
echo "Checking for remaining svelte-hero-icons usage..."
rg "from 'svelte-hero-icons'" frontend/
# Check for consistent unplugin-icons import pattern
echo "Checking for consistent unplugin-icons import pattern..."
rg "from '~icons/heroicons/" frontend/
# Check if the icon size (16) is consistently used for calendar icons
echo "Checking for consistent calendar icon usage..."
rg "calendar.*16.*solid" frontend/
Length of output: 3599
frontend/src/lib/components/CollapsibleSection/CollapsibleSection.svelte (1)
8-8: Verify the icon alias configuration in Vite
The import statement follows the unplugin-icons convention. Let's verify the configuration is properly set up.
✅ Verification successful
Icon configuration is properly set up
The verification confirms:
unplugin-iconsis correctly configured invite.config.tswith the Svelte compiler option@iconify-json/heroiconsis installed as a dev dependency (version ^1.2.2)- The import path
~icons/heroicons/chevron-down-16-solidfollows the standard convention supported by the configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Vite configuration includes unplugin-icons setup
# Check if Vite config exists and contains unplugin-icons configuration
rg -l "unplugin-icons" frontend/vite.config.*
# Check if the specific heroicons package is listed in dependencies
cat frontend/package.json | jq '.dependencies["@iconify-json/heroicons"]'
Length of output: 151
Script:
#!/bin/bash
# Let's check the actual Vite configuration to ensure proper setup
cat frontend/vite.config.ts
# Also check devDependencies for the heroicons package
cat frontend/package.json | jq '.devDependencies["@iconify-json/heroicons"]'
Length of output: 947
frontend/src/lib/components/CustomEChartLegend/CustomEChartLegend.svelte (2)
4-5: LGTM! Clean import changes for icon management.
The changes align well with the PR objectives, replacing svelte-hero-icons with unplugin-icons. The addition of the cn utility suggests improved class name management.
Also applies to: 7-7
4-4: Verify type safety and consistent usage.
Let's ensure the new icon imports are properly typed and consistently used across the codebase.
✅ Verification successful
Icon import is properly typed and consistently used
The verification shows that:
- TypeScript is properly configured with
"types": ["unplugin-icons/types/svelte"]in tsconfig.json - All icon imports across the codebase consistently use the
~icons/heroiconspattern - The
CustomEChartLegendcomponent is properly integrated withinEChart.svelteand the icon import follows the same pattern as other components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent icon import patterns
echo "Checking for any remaining svelte-hero-icons imports..."
rg "from 'svelte-hero-icons'" frontend/
echo "Verifying consistent usage of unplugin-icons..."
rg "from '~icons/heroicons'" frontend/
echo "Checking TypeScript configuration..."
cat frontend/tsconfig.json | grep "types.*unplugin-icons"
Length of output: 463
Script:
#!/bin/bash
# Let's check if tsconfig.json exists and examine its content
echo "Checking tsconfig.json content:"
cat frontend/tsconfig.json
# Let's check for any icon imports in the codebase more broadly
echo -e "\nChecking all icon imports patterns:"
rg "import.*Icon.*from" frontend/
# Let's specifically look at the CustomEChartLegend component usage
echo -e "\nChecking CustomEChartLegend component usage:"
rg -A 5 "CustomEChartLegend" frontend/
Length of output: 5096
frontend/vite.config.ts (2)
3-4: LGTM! Imports are well-structured.
Good use of the node: prefix for the fs module import.
12-15: Verify custom icons setup and usage.
Let's ensure the custom icons are properly set up and used.
✅ Verification successful
Custom icons are properly set up and used
The verification confirms:
- SVG files exist at the correct location:
frontend/src/lib/icons/ - Icons are being imported and used in the codebase (
frontend/src/routes/joins/[slug]/+page.svelte) - The configuration in
vite.config.tscorrectly maps to the existing files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify custom icons setup and usage
# Check if SVG files exist
echo "Checking SVG files existence..."
fd -t f "chart-(line|skew).svg" src/lib/icons
# Find usage of custom icons in the codebase
echo "Checking custom icons usage..."
rg -l "from ['\"]\~icons/zipline-ai/(chart-line|chart-skew)['\"]" -g "*.{svelte,ts,js}"
Length of output: 432
Script:
#!/bin/bash
# Let's verify the correct path for icons and their usage
echo "Checking repository structure for icons..."
fd -t f -e svg . frontend/src
echo "\nChecking the usage context..."
rg "from ['\"]~icons/zipline-ai/(chart-line|chart-skew)['\"]" -A 2 frontend/src/routes/joins/[slug]/+page.svelte
Length of output: 587
frontend/src/lib/components/PageHeader/PageHeader.svelte (1)
4-4: LGTM! Verify icon path availability.
The import statement correctly follows the unplugin-icons pattern.
Let's verify that the icon path exists in the heroicons set:
✅ Verification successful
Icon import is correctly implemented and widely used across the codebase
The verification shows that:
- The
book-open-16-solidicon is properly imported and used in PageHeader.svelte - The import pattern
~icons/heroicons/...-16-solidis consistently used across multiple components in the codebase - Other components successfully use the same unplugin-icons import pattern, confirming this is the established convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the book-open icon in the heroicons set
# Expected: The icon should be found in the installed package
# Check if the heroicons package is installed
if [ -d "node_modules/@iconify-json/heroicons" ]; then
# Search for the book-open icon in the heroicons JSON files
find node_modules/@iconify-json/heroicons -type f -name "*.json" -exec grep -l "book-open" {} \;
fi
Length of output: 49
Script:
#!/bin/bash
# Let's try a different approach to verify the icon's existence:
# 1. Check if the package.json has the required dependencies
echo "Checking package.json for heroicons dependencies:"
cat package.json | grep -i "heroicons"
# 2. Search for any files using this icon pattern
echo -e "\nSearching for files using the book-open icon:"
rg "book-open.*solid" --type svelte
# 3. Look for similar icon imports
echo -e "\nSearching for similar unplugin-icons imports:"
rg "import.*from '~icons/heroicons/" --type svelte
Length of output: 562
Script:
#!/bin/bash
# Let's try without file type restrictions since 'svelte' isn't recognized
# 1. Search for any files using this icon pattern
echo "Searching for files using the book-open icon:"
rg "book-open.*solid"
# 2. Look for similar unplugin-icons imports
echo -e "\nSearching for similar unplugin-icons imports:"
rg "import.*from '~icons/heroicons/"
# 3. Check if the file exists and show its content
echo -e "\nChecking the PageHeader.svelte content:"
cat frontend/src/lib/components/PageHeader/PageHeader.svelte
Length of output: 4265
frontend/src/lib/components/InfoTooltip/InfoTooltip.svelte (2)
3-3: Verify the Vite configuration for icon imports
The import path uses the ~icons alias which should be configured in the Vite setup.
✅ Verification successful
Import configuration is properly set up for unplugin-icons
The verification shows that:
- The Vite configuration in
frontend/vite.config.tscorrectly includes theunplugin-icons/viteplugin with proper setup for Svelte compilation - TypeScript configuration includes the necessary types for unplugin-icons via
"types": ["unplugin-icons/types/svelte"] - The comment in tsconfig.json indicates that path aliases are handled by SvelteKit's configuration, which is the standard way to handle the
~iconsalias in a SvelteKit project
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Vite configuration for unplugin-icons
# Check if Vite config includes the necessary plugin setup
rg -l "unplugin-icons/vite"
rg -A 10 "unplugin-icons/vite"
# Verify TypeScript configuration includes the type definitions
rg "~icons" tsconfig.json
Length of output: 727
Script:
#!/bin/bash
# Let's check for tsconfig files in the frontend directory and their content
fd tsconfig.json frontend/
# If found, check their content for path aliases
fd tsconfig.json frontend/ --exec cat {}
# Also check vite config for any path alias configurations
rg -A 5 "resolve:" frontend/vite.config.ts
Length of output: 839
10-10: LGTM! Consider documenting the icon styling system
The icon implementation is simplified and follows the new pattern correctly. The use of the text-muted-icon-neutral class suggests a systematic approach to icon styling.
✅ Verification successful
Icon styling pattern is consistent but could benefit from documentation
The text-muted-icon-neutral class is consistently used across the codebase, particularly in the NavigationBar and InfoTooltip components. While the icon implementation is correct and follows an established pattern, there's no explicit documentation of the icon styling system in the codebase. The pattern shows two main variants:
text-muted-icon-neutralfor default statetext-muted-icon-primaryfor active state
- Consider adding documentation about the icon styling system in the frontend's style guide or Tailwind configuration
- Document the meaning and usage of
text-muted-icon-neutralandtext-muted-icon-primaryclasses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent icon styling across the codebase
# Look for similar icon styling patterns
rg "text-muted-icon-"
# Check if there's documentation about icon styling
fd -t f "README|CONTRIBUTING|STYLE" -e md | xargs rg -l "icon|styling"
Length of output: 489
Script:
#!/bin/bash
# Let's check for any Tailwind or CSS files that might define these icon styles
fd -e css -e pcss -e postcss | xargs rg "text-muted-icon"
# Also check for any style documentation in the app
fd . -t f | rg -i "style|theme|tailwind" | xargs rg -l "icon"
# Look for similar icon patterns to understand the styling system better
rg "Icon.*class=\"" -g "*.svelte"
Length of output: 1161
frontend/src/lib/types/Entity/Entity.ts (2)
9-9: LGTM! Icon component references updated correctly.
The icon property updates align with the new import names while maintaining the existing configuration structure.
Let's verify the icon components are properly exported from the unplugin-icons package:
Also applies to: 15-15, 21-21
✅ Verification successful
Icons are correctly imported and used consistently
The verification confirms that:
- All required Heroicons (
cube,puzzle-piece,square-3-stack-3d) are properly imported using the unplugin-icons syntax - The icons are consistently used across the codebase
- No naming conflicts or missing exports were detected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify icon component availability
# Check for potential icon naming conflicts or missing exports
rg "heroicons.*(?:cube|puzzle-piece|square-3-stack-3d)-16-solid"
Length of output: 531
1-3: LGTM! Verify icon sizes match the previous implementation.
The new imports correctly follow the unplugin-icons convention. The explicit 16px size specification in the import paths is noted.
Let's verify the previous icon sizes to ensure consistency:
✅ Verification successful
Let's gather more information to verify the icon sizes by checking the actual usage of these icons in the codebase.
Let's do one final check to see how these icons were previously imported and used before the migration:
Let's check the Vite and Tailwind configs since they might contain relevant icon configuration:
Based on all the gathered information, I can now generate the final response:
The icon size transition is correct and consistent
The migration from svelte-hero-icons to unplugin-icons maintains consistency by:
- Using the explicit 16px size variant (
-16-solid) in the imports - Proper configuration in
vite.config.tsfor the icon plugin - Maintaining the same icon usage pattern in the entity configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check previous icon size implementations
# Look for size-related properties in previous icon usage
rg -A 2 "svelte-hero-icons"
Length of output: 618
Script:
#!/bin/bash
# Search for previous icon usage patterns in the codebase
rg -B 2 -A 2 "Cube|PuzzlePiece|Square3Stack3d" --type ts
# Also check for any size-related props or classes
ast-grep --pattern 'class $_ {
$$$
size = $_
$$$
}'
Length of output: 1405
Script:
#!/bin/bash
# Check git history for previous implementation of Entity.ts
git show HEAD~1:frontend/src/lib/types/Entity/Entity.ts
# Also look for any icon-related configuration files
fd -e config.js -e config.ts -x grep -l "icon"
Length of output: 989
Script:
#!/bin/bash
# Check icon-related configurations
cat frontend/vite.config.ts
cat frontend/tailwind.config.js
# Also let's check for the previous commit that might have changed these icons
git log -p --grep="icon" -n 1
Length of output: 4533
| // TODO: Hacky workaround to keep page from locking up | ||
| return; | ||
|
|
There was a problem hiding this comment.
Critical: Address the temporary workaround in loadDistributions.
The TODO comment indicates a temporary workaround that prevents the page from loading distributions. This could impact the functionality of the distributions tab.
Would you like me to help investigate the performance issue and propose a solution? I can help:
- Profile the distribution loading logic
- Implement pagination or virtualization
- Optimize the data fetching strategy
| Icons({ | ||
| compiler: 'svelte', | ||
| customCollections: { | ||
| 'zipline-ai': { | ||
| 'chart-line': () => fs.readFile('./src/lib/icons/chart-line.svg', 'utf-8'), | ||
| 'chart-skew': () => fs.readFile('./src/lib/icons/chart-skew.svg', 'utf-8') | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness of custom icon loading.
While the plugin configuration is functional, consider these improvements:
- Use absolute paths to prevent working directory issues
- Add error handling for file operations
- Consider caching SVG content to improve performance
Here's the suggested implementation:
Icons({
compiler: 'svelte',
customCollections: {
'zipline-ai': {
- 'chart-line': () => fs.readFile('./src/lib/icons/chart-line.svg', 'utf-8'),
- 'chart-skew': () => fs.readFile('./src/lib/icons/chart-skew.svg', 'utf-8')
+ 'chart-line': async () => {
+ try {
+ return await fs.readFile(
+ new URL('./src/lib/icons/chart-line.svg', import.meta.url),
+ 'utf-8'
+ );
+ } catch (error) {
+ console.error('Failed to load chart-line icon:', error);
+ return ''; // Return empty SVG or a fallback
+ }
+ },
+ 'chart-skew': async () => {
+ try {
+ return await fs.readFile(
+ new URL('./src/lib/icons/chart-skew.svg', import.meta.url),
+ 'utf-8'
+ );
+ } catch (error) {
+ console.error('Failed to load chart-skew icon:', error);
+ return ''; // Return empty SVG or a fallback
+ }
+ }
}
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Icons({ | |
| compiler: 'svelte', | |
| customCollections: { | |
| 'zipline-ai': { | |
| 'chart-line': () => fs.readFile('./src/lib/icons/chart-line.svg', 'utf-8'), | |
| 'chart-skew': () => fs.readFile('./src/lib/icons/chart-skew.svg', 'utf-8') | |
| } | |
| } | |
| }) | |
| Icons({ | |
| compiler: 'svelte', | |
| customCollections: { | |
| 'zipline-ai': { | |
| 'chart-line': async () => { | |
| try { | |
| return await fs.readFile( | |
| new URL('./src/lib/icons/chart-line.svg', import.meta.url), | |
| 'utf-8' | |
| ); | |
| } catch (error) { | |
| console.error('Failed to load chart-line icon:', error); | |
| return ''; // Return empty SVG or a fallback | |
| } | |
| }, | |
| 'chart-skew': async () => { | |
| try { | |
| return await fs.readFile( | |
| new URL('./src/lib/icons/chart-skew.svg', import.meta.url), | |
| 'utf-8' | |
| ); | |
| } catch (error) { | |
| console.error('Failed to load chart-skew icon:', error); | |
| return ''; // Return empty SVG or a fallback | |
| } | |
| } | |
| } | |
| } | |
| }) |
sean-zlai
changed the title
unplugin-icons with @iconify-json/heroicons (WIP)svelte-hero-icons and @zipline-ai/icons with unplugin-icons
sean-zlai
force-pushed
the
sean/unplugin-icons
branch
from
f8291f2 to
fc7d0a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/src/lib/components/NavigationBar/NavigationBar.svelte (1)
Line range hint
205-224: Fix incorrect icon reference and optimize icon declarations.Two issues need attention:
- The joins section is using the wrong icon (models instead of joins)
- The icon declarations can be optimized
Fix the incorrect icon reference:
- {@const IconJoins = getEntity('models').icon} + {@const IconJoins = getEntity('joins').icon}Consider moving the icon declarations outside the loop for better performance:
{#if searchResults.length > 0} {@const IconModels = getEntity('models').icon} {@const IconJoins = getEntity('joins').icon} {@const IconGroupBys = getEntity('groupbys').icon} <CommandGroup heading={`Search for "${input}"`}> {#each searchResults as model} <!-- Use the predeclared icon components --> {/each} </CommandGroup> {/if}
🧹 Nitpick comments (1)
frontend/src/lib/components/NavigationBar/NavigationBar.svelte (1)
178-192: Consider adding visual feedback for disabled items.While the icon implementation is correct, consider adding a CSS class to visually indicate the disabled state of these menu items (e.g., reducing opacity).
- <IconExclamationTriangle /> + <IconExclamationTriangle class="opacity-50" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (4)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/src/lib/icons/chart-line.svgis excluded by!**/*.svgfrontend/src/lib/icons/chart-skew.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
frontend/package.json(2 hunks)frontend/src/lib/components/ActionButtons/ActionButtons.svelte(3 hunks)frontend/src/lib/components/BreadcrumbNav/BreadcrumbNav.svelte(2 hunks)frontend/src/lib/components/CollapsibleSection/CollapsibleSection.svelte(2 hunks)frontend/src/lib/components/CustomEChartLegend/CustomEChartLegend.svelte(2 hunks)frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte(2 hunks)frontend/src/lib/components/InfoTooltip/InfoTooltip.svelte(1 hunks)frontend/src/lib/components/NavigationBar/NavigationBar.svelte(8 hunks)frontend/src/lib/components/PageHeader/PageHeader.svelte(2 hunks)frontend/src/lib/components/ResetZoomButton/ResetZoomButton.svelte(1 hunks)frontend/src/lib/components/SplitView/SplitView.svelte(2 hunks)frontend/src/lib/types/Entity/Entity.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.svelte(2 hunks)frontend/tsconfig.json(1 hunks)frontend/vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- frontend/tsconfig.json
- frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte
- frontend/src/lib/components/CollapsibleSection/CollapsibleSection.svelte
- frontend/src/lib/components/CustomEChartLegend/CustomEChartLegend.svelte
- frontend/src/lib/types/Entity/Entity.ts
- frontend/src/lib/components/InfoTooltip/InfoTooltip.svelte
- frontend/src/lib/components/PageHeader/PageHeader.svelte
- frontend/src/lib/components/BreadcrumbNav/BreadcrumbNav.svelte
- frontend/src/lib/components/SplitView/SplitView.svelte
- frontend/package.json
- frontend/src/routes/joins/[slug]/+page.svelte
- frontend/src/lib/components/ActionButtons/ActionButtons.svelte
- frontend/src/lib/components/ResetZoomButton/ResetZoomButton.svelte
- frontend/vite.config.ts
🔇 Additional comments (5)
frontend/src/lib/components/NavigationBar/NavigationBar.svelte (5)
28-34: LGTM! Icon imports are well-organized.
The new imports using unplugin-icons are properly structured with consistent naming and clear size/style specifications.
100-100: LGTM! Icon usage is simplified.
The new icon component usage is more concise while maintaining the styling.
123-123: LGTM! Consistent icon implementation.
The docs button icon follows the same simplified pattern.
151-155: LGTM! Avatar section icons are properly updated.
Both the user icon and chevron icon implementations follow the new pattern consistently.
28-34: Verify icon configuration in Vite.
Let's ensure the icon plugin is properly configured in the Vite config.
✅ Verification successful
Icon configuration is properly set up and icons are correctly used
The verification shows that:
- The Vite configuration has
unplugin-iconsproperly configured with the svelte compiler - All imported heroicons are actively used in the NavigationBar component
- The icon imports follow the correct
~icons/heroiconssyntax
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Vite configuration for unplugin-icons
rg -l 'unplugin-icons/vite' vite.config.ts
# Verify all icon imports are working
rg -l '~icons/heroicons' 'frontend/src/**/*.svelte'
Length of output: 216
Script:
#!/bin/bash
# Let's first find the vite config file and icon-related files
fd -t f "vite.config"
fd -t f "NavigationBar.svelte"
# Then check the content of these files
rg -l "unplugin-icons"
rg "import.*heroicons" "frontend/src/lib/components/NavigationBar/NavigationBar.svelte"
Length of output: 883
Script:
#!/bin/bash
# Let's check the vite.config.ts content to verify the icon plugin configuration
cat frontend/vite.config.ts
# Also check if these icons are actually used in the component
rg "Icon(ArrowsUpDown|AdjustmentsHorizontal|ChevronDown|DocumentText|ExclamationTriangle|MagnifyingGlass|User)" frontend/src/lib/components/NavigationBar/NavigationBar.svelte
Length of output: 1836
sean-zlai
force-pushed
the
sean/unplugin-icons
branch
2 times, most recently
from
10511b2 to
121a179
Compare
…onset) to simplify using other icon sets
…cons/zipline-ai/*`)
sean-zlai
force-pushed
the
sean/unplugin-icons
branch
from
4294e44 to
f2af557
Compare
|
Something funky is going on here. Pulling the latest from this branch, Looking at the diff, I don't see why the errors are here (related to echarts imports, etc). Could it be some weird rebase thing? |
…ons.types` (which overrides defaults) to `app.d.ts`. Fixes ECharts types (svelte-check)
@ken-zlai - Fixed via e32a91f. Was caused because setting compilerOptions.types in PR is ready for a final review
|
There was a problem hiding this comment.
I like how unplugin-icons is more future proof in terms of switching out icon packs.
Since we are now gonna store our icons in this repo, does it make sense to delete https://github.com/zipline-ai/icons as to avoid any confusion?
Possibly, or mark the repo as archived? |
Summary
Replace
svelte-hero-iconsand@zipline-ai/iconswithunplugin-iconswhich improve:vscode-iconifyplugin makes the experience 1000% better. Also browsing iconsets using icones.js.org which have quick copy/paste for "Unplugin Icons".Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation