feat (icons): Icons API end points for retrieving icons. #209
Conversation
Deploying patternfly-doc-core with
|
| Latest commit: |
a8654f0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bacd433f.patternfly-doc-core.pages.dev |
| Branch Preview URL: | https://icons-api.patternfly-doc-core.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds react-icons integration plus APIs to list/filter icons and serve per-icon SVGs, pre-rendered index and per-set SVG assets, utilities to load/filter/render icons, OpenAPI/API index updates, SVG response helper, and comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Icons API Route
participant Index as Icons Index (iconsIndex.json / fetchApiIndex)
participant Utils as ReactIcons Utils
participant RN as node_modules/react-icons
Client->>API: GET /api/{version}/icons[?filter] or /api/{version}/icons/{set_icon}
API->>Index: fetchIconsIndex(origin)
Index-->>API: icons index (list)
alt version exists
API->>Utils: filterIcons or getIconSvg
Utils->>RN: dynamic import icon set
RN-->>Utils: icon components / metadata
Utils-->>API: icons list or SVG markup
API-->>Client: 200 JSON or image/svg+xml
else version missing
API-->>Client: 400 or 404 error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/__tests__/pages/api/__tests__/`[version]/icons/index.test.ts:
- Line 100: The test's assertion calls expect(body.icons.every((i: { name:
string }) => i.name.includes('circle'))) but never asserts a value, so it always
passes; update the assertion to actually check the boolean (e.g., call
.toBe(true) or .toBeTruthy()) on the expression using body.icons and the
existing predicate to ensure the test fails when no icon names include "circle".
🧹 Nitpick comments (9)
src/utils/icons/reactIcons.ts (3)
50-54: Static analysis ReDoS warning is a false positive in this context.The regex at line 51 is constructed from
setIdvalues that originate fromIconsManifest(react-icons library internal data), not from user input. The pattern^${setPrefix}is a simple anchored prefix match without problematic quantifiers, making ReDoS practically impossible here.However, if you want to be extra defensive, you could sanitize the input or use a literal string match instead:
♻️ Optional: Use string operations instead of regex
} else { const setPrefix = setId.charAt(0).toUpperCase() + setId.slice(1) - const prefix = new RegExp(`^${setPrefix}`, 'i') - base = base.replace(prefix, '') + if (base.toLowerCase().startsWith(setPrefix.toLowerCase())) { + base = base.slice(setPrefix.length) + } }
82-84: Consider logging failed icon set loads for debugging.Silent failures make troubleshooting difficult. While graceful degradation is appropriate, logging would help identify issues during development or when icon sets are unexpectedly unavailable.
♻️ Optional: Add debug logging
- } catch { - // Skip sets that fail to load + } catch (error) { + // Skip sets that fail to load, but log for debugging + console.debug(`Failed to load icon set '${setId}':`, error) }
62-88: Consider cachinggetAllIcons()result to avoid repeated dynamic imports.
getAllIcons()performs dynamic imports for every icon set on each call. For a production API, this could be expensive if called frequently. Consider memoizing the result since icon sets don't change at runtime.♻️ Proposed caching implementation
+let cachedIcons: IconMetadata[] | null = null + export async function getAllIcons(): Promise<IconMetadata[]> { + if (cachedIcons) { + return cachedIcons + } + const icons: IconMetadata[] = [] for (const setId of ICON_SET_IDS) { // ... existing code ... } + cachedIcons = icons return icons }src/__tests__/pages/api/__tests__/[version]/icons/[iconName].test.ts (1)
17-24: Mock duplicates production logic - consider importing the real function.The
parseIconIdmock reimplements the actual logic, which could lead to test/implementation drift. Consider using Jest'srequireActualto import the real function while mocking only what's necessary.♻️ Proposed: Use real parseIconId implementation
jest.mock('../../../../../../utils/icons/reactIcons', () => ({ getIconSvg: jest.fn((setId: string, iconName: string) => { if (setId === 'fa' && iconName === 'FaCircle') return Promise.resolve(mockSvg) return Promise.resolve(null) }), - parseIconId: jest.fn((iconId: string) => { - const underscoreIndex = iconId.indexOf('_') - if (underscoreIndex <= 0) return null - const setId = iconId.slice(0, underscoreIndex) - const iconName = iconId.slice(underscoreIndex + 1) - if (!setId || !iconName) return null - return { setId, iconName } - }), + parseIconId: jest.requireActual('../../../../../../utils/icons/reactIcons').parseIconId, }))src/pages/api/openapi.json.ts (2)
107-163: Consider adding 404 response for version not found.The
/{version}/iconsendpoint returns 404 when the version doesn't exist (per the implementation insrc/pages/api/[version]/icons/index.ts), but this isn't documented in the OpenAPI spec.📝 Add 404 response documentation
'200': { description: 'List of icons with metadata', // ... existing schema ... }, + '404': { + description: 'Version not found', + }, },
164-201: Consider adding 400 response for invalid icon name format.The
/{version}/icons/{icon-name}endpoint returns 400 for invalid icon name formats, but only 404 is documented. This could cause confusion for API consumers.📝 Add 400 response documentation
responses: { '200': { description: 'SVG markup for the icon', content: { 'image/svg+xml': { schema: { type: 'string' }, }, }, }, + '400': { + description: 'Invalid icon name format. Expected: {set}_{iconName}', + }, '404': { description: 'Icon not found', }, },src/pages/api/[version]/icons/[iconName].ts (1)
17-17: Minor: Variable aliasing may cause confusion.The destructuring
{ version, iconName: iconId }creates an alias, buticonNameis reused later (line 53) as a different variable fromparseIconId. While not a bug, this could be clearer.♻️ Suggested clarification
Consider renaming for clarity:
- const { version, iconName: iconId } = params + const { version, iconName: rawIconId } = paramsAnd update references to
iconId→rawIconIdaccordingly.src/__tests__/pages/api/__tests__/[version]/icons/index.test.ts (1)
50-51: Consider usingdescribeblock andafterEachfor cleaner test organization.Tests would benefit from wrapping in a
describeblock and moving the repeatedjest.restoreAllMocks()calls to anafterEachhook.♻️ Suggested structure
+describe('GET /api/{version}/icons', () => { + afterEach(() => { + jest.restoreAllMocks() + }) + it('returns all icons with metadata', async () => { global.fetch = jest.fn(() => ... expect(body.icons[0]).toHaveProperty('unicode') - - jest.restoreAllMocks() }) // ... other tests without individual restoreAllMocks calls +})Also applies to: 79-80
src/pages/api/[version]/icons/index.ts (1)
32-33: Consider pagination for large icon sets.
react-iconscontains thousands of icons across all sets. Returning all icons in a single response could impact performance and client-side rendering. Consider adding pagination (limit/offsetor cursor-based) for production use.
src/__tests__/pages/api/__tests__/[version]/icons/index.test.ts
Outdated
Show resolved
Hide resolved
dlabaj
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/pages/api/`[version]/icons/[iconName].ts:
- Around line 42-59: The call to fetchIconsIndex (and subsequent fetchIconSvgs)
can throw and is not wrapped, causing uncontrolled 500s; wrap the body that uses
fetchIconsIndex, icons, icon, fetchIconSvgs and svg in a try/catch around the
existing logic inside the handler, and in the catch return createJsonResponse({
error: 'Internal server error', details: error?.message ?? String(error) }, 500)
(or similar) so failures from fetchIconsIndex/fetchIconSvgs produce a clean JSON
500 response instead of an unhandled exception.
🧹 Nitpick comments (2)
src/__tests__/pages/api/__tests__/[version]/icons/index.test.ts (1)
62-87: Consider cleaning upglobal.fetchin anafterEachblock.
jest.restoreAllMocks()only restores mocks created viajest.spyOn, not directglobal.fetchassignments. If a test throws before therestoreAllMockscall, the mutatedglobal.fetchleaks. Since each test reassigns it anyway this is low risk, but anafterEachwould be cleaner.♻️ Suggested cleanup pattern
+const originalFetch = global.fetch + +afterEach(() => { + global.fetch = originalFetch + jest.restoreAllMocks() +}) + it('returns all icons with metadata', async () => { global.fetch = createFetchMock() // ... - jest.restoreAllMocks() })src/utils/icons/reactIcons.ts (1)
117-145: Consider the memory and build-time cost of rendering all icons in a set.
getIconSvgsForSetrenders every icon in a set to SVG at build time viarenderToStaticMarkup. For large sets (e.g., Material Design has ~10,000+ icons), this could be memory-intensive during the build. Since this runs at prerender time (not in Workers), it should be fine, but worth noting if build times become a concern.
| const icons = await fetchIconsIndex(url) | ||
| const icon = icons.find((i) => i.reactName === reactName) | ||
| if (!icon?.set) { | ||
| return createJsonResponse( | ||
| { error: `Icon '${reactName}' not found` }, | ||
| 404, | ||
| ) | ||
| } | ||
|
|
||
| const svgs = await fetchIconSvgs(url, icon.set) | ||
| const svg = svgs?.[reactName] ?? null | ||
|
|
||
| if (!svg) { | ||
| return createJsonResponse( | ||
| { error: `Icon '${reactName}' not found in set '${icon.set}'` }, | ||
| 404, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Unhandled error from fetchIconsIndex — will produce an uncontrolled 500.
fetchIconsIndex throws on non-OK responses (see src/utils/icons/fetch.ts lines 19-23), but this call sits outside any try/catch. Compare with src/pages/api/[version]/icons/index.ts, which wraps everything in a single try/catch and returns a clean 500 JSON response.
🐛 Proposed fix: wrap the remaining logic in a try/catch
+ try {
const icons = await fetchIconsIndex(url)
const icon = icons.find((i) => i.reactName === reactName)
if (!icon?.set) {
return createJsonResponse(
{ error: `Icon '${reactName}' not found` },
404,
)
}
const svgs = await fetchIconSvgs(url, icon.set)
const svg = svgs?.[reactName] ?? null
if (!svg) {
return createJsonResponse(
{ error: `Icon '${reactName}' not found in set '${icon.set}'` },
404,
)
}
return createSvgResponse(svg)
+ } catch (error) {
+ const details = error instanceof Error ? error.message : String(error)
+ return createJsonResponse(
+ { error: 'Failed to load icon', details },
+ 500,
+ )
+ }🤖 Prompt for AI Agents
In `@src/pages/api/`[version]/icons/[iconName].ts around lines 42 - 59, The call
to fetchIconsIndex (and subsequent fetchIconSvgs) can throw and is not wrapped,
causing uncontrolled 500s; wrap the body that uses fetchIconsIndex, icons, icon,
fetchIconSvgs and svg in a try/catch around the existing logic inside the
handler, and in the catch return createJsonResponse({ error: 'Internal server
error', details: error?.message ?? String(error) }, 500) (or similar) so
failures from fetchIconsIndex/fetchIconSvgs produce a clean JSON 500 response
instead of an unhandled exception.
1 participant
Closes issue #173 by adding support for PF Icons.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores