-
Notifications
You must be signed in to change notification settings - Fork 7.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: auto fetch icon list in iconPicker #5446
Conversation
|
Warning Rate limit exceeded@mynetfan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request enhances the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (2)
playground/src/views/demos/features/icons/index.vue (1)
84-109
: Add error handling for icon value changes.The IconPicker instances lack error handling for cases where the selected icon might be invalid or unavailable.
Add error handling by wrapping the v-model bindings with watch handlers:
+const handleIconError = (iconValue: string) => { + console.warn(`Failed to load icon: ${iconValue}`); + // Optionally reset to a default icon +}; + +watch(iconValue1, (value) => { + if (!value.startsWith('ant-design:')) { + handleIconError(value); + } +}); + +watch(iconValue2, (value) => { + if (!value.startsWith('svg:')) { + handleIconError(value); + } +});packages/effects/common-ui/src/components/icon-picker/icon-picker.vue (1)
Line range hint
31-48
: Add prop validation for better type safety.The Props interface could benefit from more specific type constraints and validation.
Add validation by updating the Props interface:
interface Props { - pageSize?: number; + pageSize?: number & { __brand: 'positive' }; /** 图标集的名字 */ - prefix?: string; + prefix?: 'ant-design' | 'mdi' | 'mdi-light' | 'svg'; /** 是否自动请求API以获得图标集的数据.提供prefix时有效 */ autoFetchApi?: boolean; /** * 图标列表 */ icons?: string[]; /** Input组件 */ inputComponent?: VNode; /** 图标插槽名,预览图标将被渲染到此插槽中 */ - iconSlot?: string; + iconSlot?: 'default' | 'addonAfter' | 'addonBefore'; /** input组件的值属性名称 */ - modelValueProp?: string; + modelValueProp?: 'modelValue' | 'value'; /** 图标样式 */ iconClass?: string; type?: 'icon' | 'input'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/effects/common-ui/src/components/icon-picker/icon-picker.vue
(5 hunks)packages/effects/common-ui/src/components/icon-picker/icons.ts
(1 hunks)playground/src/views/demos/features/icons/index.vue
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: post-update (ubuntu-latest)
🔇 Additional comments (1)
playground/src/views/demos/features/icons/index.vue (1)
23-26
: Validate icon values against their respective prefixes.The icon value refs are initialized with specific prefixes, but there's no validation to ensure the values match their intended prefixes.
Run this script to verify icon prefix consistency:
✅ Verification successful
Icon prefixes are correctly configured and supported
All icon values in the demo use valid prefixes:
ant-design
(default prefix)svg
(special case handled by the component)mdi
andmdi-light
(supported via Iconify API)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if icon values match their declared prefixes # Expected: Each icon value should start with its corresponding prefix echo "Checking icon values in index.vue..." rg -U 'const icon.*= ref\((.*)\)' -A 1 | while read -r line; do if [[ $line =~ ref\([\'\"](.*)[\'\"]\) ]]; then icon="${BASH_REMATCH[1]}" prefix="${icon%%:*}" echo "Found icon: $icon with prefix: $prefix" fi doneLength of output: 1464
Script:
#!/bin/bash # Search for IconPicker component and its prefix validation echo "Searching for IconPicker component implementation..." ast-grep --pattern 'const supportedPrefixes = $_' echo -e "\nSearching for prefix validation logic..." rg -l "IconPicker" | xargs rg "prefix" -B 2 -A 2 echo -e "\nSearching for icon-related utilities..." fd -e ts -e js -e vue iconLength of output: 2385
Script:
#!/bin/bash # Check IconPicker implementation and icons configuration echo "Checking IconPicker component implementation..." cat packages/effects/common-ui/src/components/icon-picker/icon-picker.vue echo -e "\nChecking icons configuration..." cat packages/effects/common-ui/src/components/icon-picker/icons.tsLength of output: 8134
packages/effects/common-ui/src/components/icon-picker/icon-picker.vue
Outdated
Show resolved
Hide resolved
packages/effects/common-ui/src/components/icon-picker/icon-picker.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/effects/common-ui/src/components/icon-picker/icons.ts (2)
3-6
: Translate documentation to English for better accessibility.The documentation comment is in Chinese. Consider translating it to English to improve accessibility for all developers.
-/** - * 一个缓存对象,在不刷新页面时,无需重复请求远程接口 - */ +/** + * A cache object that prevents repeated remote API requests + * until the page is refreshed + */
17-40
: Add retry mechanism for transient failures.Network requests can fail due to temporary issues. Consider implementing a retry mechanism with exponential backoff.
+const MAX_RETRIES = 3; +const INITIAL_RETRY_DELAY = 1000; + export async function fetchIconsData(prefix: string) { if (!Reflect.has(ICONS_MAP, prefix) || !ICONS_MAP[prefix]) { + let retries = 0; try { - const controller = new AbortController(); - const timeoutId = setTimeout(() => controller.abort(), 1000 * 10); - const response: IconifyResponse = await fetch( - `https://api.iconify.design/collection?prefix=${prefix}`, - { signal: controller.signal }, - ).then((res) => res.json()); + while (retries < MAX_RETRIES) { + try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 1000 * 10); + // ... rest of the fetch logic ... + break; + } catch (error) { + retries++; + if (retries === MAX_RETRIES) throw error; + await new Promise(resolve => + setTimeout(resolve, INITIAL_RETRY_DELAY * Math.pow(2, retries - 1)) + ); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects/common-ui/src/components/icon-picker/icon-picker.vue
(5 hunks)packages/effects/common-ui/src/components/icon-picker/icons.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effects/common-ui/src/components/icon-picker/icon-picker.vue
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
🔇 Additional comments (3)
packages/effects/common-ui/src/components/icon-picker/icons.ts (3)
6-6
: Implement cache size limit to prevent memory leaks.The unbounded
ICONS_MAP
cache could lead to memory issues if many different prefixes are requested over time.
8-15
: Well-structured interface definition!The
IconifyResponse
interface provides good type safety and accurately represents the Iconify API response format.
22-26
:⚠️ Potential issueAdd response validation and HTTP status check.
The current implementation assumes the response will always be valid JSON and doesn't check the HTTP status.
- const response: IconifyResponse = await fetch( + const res = await fetch( `https://api.iconify.design/collection?prefix=${prefix}`, { signal: controller.signal }, - ).then((res) => res.json()); + ); + if (!res.ok) { + throw new Error(`HTTP error! status: ${res.status}`); + } + const response = (await res.json()) as IconifyResponse; + if (!response.prefix || typeof response.total !== 'number') { + throw new Error('Invalid API response format'); + } clearTimeout(timeoutId);Likely invalid or redundant comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/effects/common-ui/src/components/icon-picker/icons.ts (4)
3-6
: Convert documentation to English for consistency.The documentation comment is in Chinese. For better maintainability and consistency, consider translating it to English.
-/** - * 一个缓存对象,在不刷新页面时,无需重复请求远程接口 - */ +/** + * Cache object to prevent redundant API requests during the page lifecycle + */
19-25
: Convert JSDoc to English for consistency.The function documentation is in Chinese. For better maintainability and consistency, consider translating it to English.
-/** - * 通过Iconify接口获取图标集数据。 - * 同一时间多个图标选择器同时请求同一个图标集时,实际上只会发起一次请求(所有请求共享同一份结果)。 - * 请求结果会被缓存,刷新页面前同一个图标集不会再次请求 - * @param prefix 图标集名称 - * @returns 图标集中包含的所有图标名称 - */ +/** + * Fetches icon set data through the Iconify API. + * Multiple simultaneous requests for the same icon set are deduplicated into a single API call (all requests share the same result). + * Results are cached until page refresh to prevent redundant requests for the same icon set + * @param prefix The icon set name + * @returns Array of all icon names in the set + */
26-32
: Optimize cache check logic.The cache check logic can be simplified while maintaining the same functionality.
export async function fetchIconsData(prefix: string): Promise<string[]> { - if (Reflect.has(ICONS_MAP, prefix) && ICONS_MAP[prefix]) { + if (ICONS_MAP[prefix]?.length) { return ICONS_MAP[prefix]; } - if (Reflect.has(PENDING_REQUESTS, prefix) && PENDING_REQUESTS[prefix]) { + if (PENDING_REQUESTS[prefix]) { return PENDING_REQUESTS[prefix]; }
33-56
: Enhance error handling and add retry mechanism.The current implementation could benefit from:
- Retry logic for transient network failures
- More informative error handling
- Cleanup of pending requests on error
+const MAX_RETRIES = 3; +const RETRY_DELAY = 1000; + +async function fetchWithRetry(url: string, options: RequestInit, retries = MAX_RETRIES): Promise<Response> { + try { + return await fetch(url, options); + } catch (error) { + if (retries > 0) { + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); + return fetchWithRetry(url, options, retries - 1); + } + throw error; + } +} + PENDING_REQUESTS[prefix] = (async () => { try { const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), 1000 * 10); - const response: IconifyResponse = await fetch( + const response: IconifyResponse = await fetchWithRetry( `https://api.iconify.design/collection?prefix=${prefix}`, { signal: controller.signal }, ).then((res) => res.json()); clearTimeout(timeoutId); + if (!response.uncategorized && !response.categories) { + throw new Error('Invalid API response format'); + } const list = response.uncategorized || []; if (response.categories) { for (const category in response.categories) { list.push(...(response.categories[category] || [])); } } ICONS_MAP[prefix] = list.map((v) => `${prefix}:${v}`); } catch (error) { console.error(`Failed to fetch icons for prefix ${prefix}:`, error); + delete PENDING_REQUESTS[prefix]; return [] as string[]; } return ICONS_MAP[prefix]; })();playground/src/views/demos/features/icons/index.vue (2)
84-88
: Add accessibility attributes to IconPicker components.Enhance user experience by adding ARIA labels to the icon pickers.
- <IconPicker v-model="iconValue1" class="w-[200px]" /> + <IconPicker v-model="iconValue1" class="w-[200px]" aria-label="Select Iconify icon" /> - <IconPicker v-model="iconValue2" class="w-[200px]" prefix="svg" /> + <IconPicker v-model="iconValue2" class="w-[200px]" prefix="svg" aria-label="Select SVG icon" />
101-109
: Add tooltip to icon-only picker for better UX.When using icon-only mode, a tooltip would help users understand the functionality.
<Input v-model:value="iconValue4" allow-clear - placeholder="点击这里选择图标" + placeholder="Click here to select an icon" style="width: 300px" > <template #addonAfter> - <IconPicker v-model="iconValue4" prefix1="mdi-light" type="icon" /> + <IconPicker + v-model="iconValue4" + prefix1="mdi-light" + type="icon" + title="Click to select an icon" + /> </template> </Input>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects/common-ui/src/components/icon-picker/icons.ts
(1 hunks)playground/src/views/demos/features/icons/index.vue
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: post-update (ubuntu-latest)
🔇 Additional comments (3)
packages/effects/common-ui/src/components/icon-picker/icons.ts (1)
8-15
: LGTM! Well-structured interface definition.The
IconifyResponse
interface properly defines all necessary fields with appropriate types and optional properties.playground/src/views/demos/features/icons/index.vue (2)
23-26
: LGTM! Good variety in icon sources.The icon references demonstrate proper usage of different icon libraries (ant-design, svg, mdi, mdi-light).
91-98
: Add type validation for prefix property.Consider adding runtime validation for the prefix value to ensure it matches available icon libraries.
Description
改进IconPicker,可以使用默认的Input组件,并且可以自动通过Iconify接口获取图标列表。
close #5375
Type of change
Please delete options that are not relevant.
pnpm-lock.yaml
unless you introduce a new test example.Checklist
pnpm run docs:dev
command.pnpm test
.feat:
,fix:
,perf:
,docs:
, orchore:
.Summary by CodeRabbit
New Features
Improvements