Add IconsController and update SVG icon styling#11
Merged
Conversation
…emo, update package.json and tsconfig for React support
Contributor
📝 WalkthroughWalkthroughAdds React support and TSX JSX setting, removes a static icons HTML page, and introduces an IconsController, routes, and a React-based server-rendered demo view to list and serve SVG icons from Changes
Sequence DiagramsequenceDiagram
actor Client
participant Express as "Express App"
participant Router as "Icons Router"
participant Controller as "IconsController"
participant FileSystem as "File System"
participant View as "Icons Demo View"
Client->>Express: GET /icons/demo
Express->>Router: route -> /icons/demo
Router->>Controller: getDemoPage()
Controller->>FileSystem: read icon filenames
FileSystem-->>Controller: list of icons
Controller->>View: generateIconsDemoHTML(icons)
View-->>Controller: HTML string
Controller-->>Express: 200 text/html
Express-->>Client: demo page
Client->>Express: GET /icons
Express->>Router: route -> /icons
Router->>Controller: getAllIcons()
Controller->>FileSystem: (cached) or read filenames
Controller-->>Express: 200 application/json
Express-->>Client: icons JSON
Client->>Express: GET /icons/:name
Express->>Router: route -> /icons/:name
Router->>Controller: getIcon(name)
Controller->>FileSystem: read {name}.svg
FileSystem-->>Controller: svg content
Controller-->>Express: 200 image/svg+xml (with long cache)
Express-->>Client: svg file
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
|
Note Docstrings generation - SUCCESS |
coderabbitai Bot
added a commit
that referenced
this pull request
Mar 5, 2026
Docstrings generation was requested by @pphatdev. * #11 (comment) The following files were modified: * `src/routes/icons.routes.ts` * `src/routes/register.ts` * `src/views/icons-demo.view.tsx`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/controllers/icons.controller.ts (1)
24-29: Consider extracting shared icon-list loading into one helper.
getAllIconsandgetDemoPageduplicate.readdir().filter().map()logic; centralizing it will reduce drift and make sorting/caching behavior consistent.Also applies to: 87-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/icons.controller.ts` around lines 24 - 29, Extract the duplicated readdir/filter/map logic into a single helper on IconsController (e.g., a private static method like loadIconsList or ensureIconsCached) that reads IconsController.iconsDir, filters for '.svg', strips the extension, and sets/returns IconsController.iconsCache (applying any sorting/caching policy consistently); then replace the inline logic in getAllIcons and getDemoPage to call that helper so both use the same cached/sorted list and avoid drift (also update any references around IconsController.iconsCache to use the helper’s returned value).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/controllers/icons.controller.ts`:
- Around line 54-56: The getIcon handler builds iconPath from user-controlled
req.params.name (iconName) and is vulnerable to path traversal; validate and
constrain the name and enforce containment within IconsController.iconsDir:
first strip a trailing ".svg" from req.params.name into iconName, validate
iconName against a strict filename regex (e.g., only alphanumeric, dots,
underscores, hyphens), return a 400 on failure, then compute the resolved path
with path.resolve(IconsController.iconsDir, `${iconName}.svg`) and ensure the
resolved path starts with or is contained by
path.resolve(IconsController.iconsDir) (reject and 400 if not); use
IconsController.getIcon, IconsController.iconsDir, iconName and iconPath as the
referenced symbols to locate and update the code.
In `@src/views/icons-demo.view.tsx`:
- Around line 13-17: The inline script injects iconsJSON into scriptContent
without escaping, which can break the script or enable XSS; update the code that
builds scriptContent (where iconsJSON is created and interpolated into
scriptContent) to escape the JSON for safe HTML embedding (for example, replace
"<" with "\u003c" and also escape the sequence "</script>" to "<\/script>" or
apply an HTML-safe serializer) before interpolation, and apply the same escaping
wherever iconsJSON is used to inject inline scripts (e.g., the other usage noted
around the second occurrence).
- Line 33: The markdown URL currently hardcodes "https://stats.pphat.top" when
building markdownUrl; change it to derive the origin at runtime (e.g., use
window.location.origin or location.origin) and join that with the
"/assets/icons/" + icon + ".svg" path so self-hosted or staging deployments
work; update the markdownUrl assignment in src/views/icons-demo.view.tsx (the
markdownUrl variable that uses icon) to use the runtime origin and include a
safe fallback if origin is undefined.
- Around line 48-52: The current fetch call assigns raw SVG text into
wrapper.innerHTML (see the fetch('/assets/icons/' + icon + '.svg') ...
wrapper.innerHTML = svgContent block), which risks active SVG payloads; replace
this by creating an <img> element and set its src to the icon URL (or, if you
must inline SVG, properly sanitize svgContent before insertion), then append or
replace the existing wrapper child with that safe element (use the existing
wrapper variable and the same URL generation logic '/assets/icons/' + icon +
'.svg').
- Around line 320-322: The button's onClick won't work with renderToStaticMarkup
— remove the inline onClick from the <button className="reload-btn"> and instead
attach the listener in the injected client script like the search box pattern:
in the same script that queries document.querySelector for the search input, add
document.querySelector('.reload-btn') and add a click handler that calls
window.location.reload(); ensure the selector matches "reload-btn" and the
script is included in the static output so the handler is bound at runtime.
---
Nitpick comments:
In `@src/controllers/icons.controller.ts`:
- Around line 24-29: Extract the duplicated readdir/filter/map logic into a
single helper on IconsController (e.g., a private static method like
loadIconsList or ensureIconsCached) that reads IconsController.iconsDir, filters
for '.svg', strips the extension, and sets/returns IconsController.iconsCache
(applying any sorting/caching policy consistently); then replace the inline
logic in getAllIcons and getDemoPage to call that helper so both use the same
cached/sorted list and avoid drift (also update any references around
IconsController.iconsCache to use the helper’s returned value).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88e3b5d2-64a4-40c3-a6b3-d9f9317e9bb0
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/assets/icons/xampp.svgis excluded by!**/*.svg
📒 Files selected for processing (8)
package.jsonpublic/icons.htmlsrc/controllers/icons.controller.tssrc/routes/docs.routes.tssrc/routes/icons.routes.tssrc/routes/register.tssrc/views/icons-demo.view.tsxtsconfig.json
💤 Files with no reviewable changes (1)
- public/icons.html
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/controllers/icons.controller.ts (1)
26-29: Consider usingpath.basename()for suffix-safe extension removal.The
replace('.svg', '')method removes the first occurrence of the substring, not strictly the trailing extension. While no icon files in the current repository exhibit this issue, filenames likefoo.svg.bar.svgwould be incorrectly normalized (e.g.,foo.bar.svginstead offoo.svg.bar). Since thepathmodule is already imported, usingpath.basename(file, '.svg')is a more robust alternative.🔧 Suggested patch
IconsController.iconsCache = files .filter(file => file.endsWith('.svg')) - .map(file => file.replace('.svg', '')); + .map(file => path.basename(file, '.svg')); @@ const icons = files .filter(file => file.endsWith('.svg')) - .map(file => file.replace('.svg', '')); + .map(file => path.basename(file, '.svg'));Also applies to: 111-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/icons.controller.ts` around lines 26 - 29, Replace the fragile string replace-based extension stripping in IconsController with path.basename(file, '.svg') to safely remove only the trailing ".svg" extension; update the assignment where IconsController.iconsCache is computed (the .filter(...).map(...) block that uses file.replace('.svg', '')) and the similar logic later in the file (the other occurrence around the block referenced at lines 111-113) to call path.basename(file, '.svg') instead of file.replace('.svg', '') so filenames like "foo.svg.bar.svg" are normalized correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/controllers/icons.controller.ts`:
- Around line 41-45: The catch blocks in icons.controller.ts that build 500
responses (the res.status(500).json blocks that currently include error
instanceof Error ? error.message : 'Unknown error') are leaking internal error
details to clients; replace the response payload so it returns a generic
client-facing message (e.g., "Internal server error" or keep the existing
"Failed to retrieve icons" without the raw error.message) and move the detailed
error logging to server logs (use your app logger or console.error(error) inside
the same catch). Update all three catch sites (the three res.status(500).json
occurrences) to follow this pattern.
---
Nitpick comments:
In `@src/controllers/icons.controller.ts`:
- Around line 26-29: Replace the fragile string replace-based extension
stripping in IconsController with path.basename(file, '.svg') to safely remove
only the trailing ".svg" extension; update the assignment where
IconsController.iconsCache is computed (the .filter(...).map(...) block that
uses file.replace('.svg', '')) and the similar logic later in the file (the
other occurrence around the block referenced at lines 111-113) to call
path.basename(file, '.svg') instead of file.replace('.svg', '') so filenames
like "foo.svg.bar.svg" are normalized correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64a9ade3-b80f-4488-abf2-402e9ea85a64
📒 Files selected for processing (1)
src/controllers/icons.controller.ts
Comment on lines
+41
to
+45
| } catch (error) { | ||
| res.status(500).json({ | ||
| error: 'Failed to retrieve icons', | ||
| message: error instanceof Error ? error.message : 'Unknown error' | ||
| }); |
Contributor
There was a problem hiding this comment.
Avoid exposing raw internal errors in API responses.
These responses currently return error.message directly to clients, which can leak internal filesystem/runtime details. Return a generic client message and keep details in server logs only.
🔧 Suggested patch
} catch (error) {
res.status(500).json({
error: 'Failed to retrieve icons',
- message: error instanceof Error ? error.message : 'Unknown error'
+ message: 'Internal server error'
});
}
@@
} catch (error) {
res.status(500).json({
error: 'Failed to retrieve icon',
- message: error instanceof Error ? error.message : 'Unknown error'
+ message: 'Internal server error'
});
}
@@
} catch (error) {
res.status(500).json({
error: 'Failed to load demo page',
- message: error instanceof Error ? error.message : 'Unknown error'
+ message: 'Internal server error'
});
}Also applies to: 97-100, 120-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/controllers/icons.controller.ts` around lines 41 - 45, The catch blocks
in icons.controller.ts that build 500 responses (the res.status(500).json blocks
that currently include error instanceof Error ? error.message : 'Unknown error')
are leaking internal error details to clients; replace the response payload so
it returns a generic client-facing message (e.g., "Internal server error" or
keep the existing "Failed to retrieve icons" without the raw error.message) and
move the detailed error logging to server logs (use your app logger or
console.error(error) inside the same catch). Update all three catch sites (the
three res.status(500).json occurrences) to follow this pattern.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/controllers/icons.controller.ts (2)
48-53:⚠️ Potential issue | 🟠 MajorDo not expose internal error messages in 500 responses.
These handlers still return
error.messageto clients, which can leak filesystem/runtime internals. Return a generic message and log the detailed error server-side.🔧 Suggested fix
} catch (error) { + console.error(error); res.status(500).json({ error: 'Failed to retrieve icons', - message: error instanceof Error ? error.message : 'Unknown error' + message: 'Internal server error' }); } @@ } catch (error) { + console.error(error); res.status(500).json({ error: 'Failed to retrieve icon', - message: error instanceof Error ? error.message : 'Unknown error' + message: 'Internal server error' }); } @@ } catch (error) { + console.error(error); res.status(500).json({ error: 'Failed to load demo page', - message: error instanceof Error ? error.message : 'Unknown error' + message: 'Internal server error' }); }Also applies to: 103-108, 123-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/icons.controller.ts` around lines 48 - 53, The catch blocks in src/controllers/icons.controller.ts currently expose internal error.message in the 500 JSON responses (they use res.status(500).json({ error: ..., message: error instanceof Error ? error.message : ... })); change these to return a generic client-facing message (e.g., "Internal server error" or "An unexpected error occurred") and send detailed error information to server logs instead (use the existing logger or console.error to log the caught error variable). Apply this change to all catch blocks in this file (the ones around lines handling icon retrieval—the catch blocks referenced at 48-53, 103-108, and 123-128) so clients never receive internal error messages while retaining detailed logs for debugging.
26-26:⚠️ Potential issue | 🟡 MinorUse suffix-only extension stripping when building icon names.
file.replace('.svg', '')removes the first match, not necessarily the trailing extension. This can generate names that won’t round-trip viagetIcon.🔧 Suggested fix
- .map(file => file.replace('.svg', '')); + .map(file => file.replace(/\.svg$/i, ''));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/icons.controller.ts` at line 26, The code that builds icon names uses a naive replacement (.map(file => file.replace('.svg', ''))) which can remove a non-suffix occurrence and produce names that won’t round-trip via getIcon; update the mapping in the icons controller to strip only a trailing ".svg" (e.g., use a suffix-only removal such as replacing /\.svg$/ or using a path-based basename-without-extension approach) so file names are reliably converted to icon keys (locate the .map(...) call that builds the icon names in icons.controller.ts).
🧹 Nitpick comments (1)
src/views/icons-demo.view.tsx (1)
41-48: Handle clipboard write failures explicitly.If clipboard permission is denied, the promise rejects and the UI provides no fallback/feedback.
🔧 Suggested fix
- navigator.clipboard.writeText(markdownUrl).then(() => { + navigator.clipboard.writeText(markdownUrl).then(() => { copyBtn.innerHTML = '<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><polyline points="20 6 9 17 4 12"></polyline></svg>'; copyBtn.classList.add('copied'); setTimeout(() => { copyBtn.innerHTML = '<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="lucide lucide-copy"><rect width="14" height="14" x="8" y="8" rx="2" ry="2"></rect><path d="M4 16c-1.1 0-2-.9-2-2V4c0-1.1.9-2 2-2h10c1.1 0 2 .9 2 2"></path></svg>'; copyBtn.classList.remove('copied'); }, 2000); - }); + }).catch(() => { + copyBtn.classList.remove('copied'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/icons-demo.view.tsx` around lines 41 - 48, navigator.clipboard.writeText(markdownUrl) is used without a rejection handler, so clipboard permission denials leave the UI silent; wrap the promise returned by navigator.clipboard.writeText(markdownUrl) with a .catch(...) (or use try/catch if converted to async) to handle errors, update the copyBtn to show an error state/message (e.g., change innerHTML/text, add an 'error' class and ensure aria-live feedback), and ensure any temporary state set by the success path (like adding 'copied') is not left stuck on error (remove 'copied' in the error handler or clear timers set by setTimeout); reference the existing navigator.clipboard.writeText(markdownUrl), copyBtn, setTimeout and the success-path DOM updates when implementing this.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/views/icons-demo.view.tsx`:
- Line 40: The code concatenates raw icon names into URLs (e.g., the markdownUrl
construction using the icon variable in src/views/icons-demo.view.tsx) which
breaks for spaces/reserved characters; update the URL builders (both occurrences
that build '/assets/icons/' + icon + '.svg', including the markdownUrl and the
similar use at line 56) to URL-encode the filename portion by applying an
encoding function such as encodeURIComponent(icon) when composing the path so
generated markdown/image links remain valid.
---
Duplicate comments:
In `@src/controllers/icons.controller.ts`:
- Around line 48-53: The catch blocks in src/controllers/icons.controller.ts
currently expose internal error.message in the 500 JSON responses (they use
res.status(500).json({ error: ..., message: error instanceof Error ?
error.message : ... })); change these to return a generic client-facing message
(e.g., "Internal server error" or "An unexpected error occurred") and send
detailed error information to server logs instead (use the existing logger or
console.error to log the caught error variable). Apply this change to all catch
blocks in this file (the ones around lines handling icon retrieval—the catch
blocks referenced at 48-53, 103-108, and 123-128) so clients never receive
internal error messages while retaining detailed logs for debugging.
- Line 26: The code that builds icon names uses a naive replacement (.map(file
=> file.replace('.svg', ''))) which can remove a non-suffix occurrence and
produce names that won’t round-trip via getIcon; update the mapping in the icons
controller to strip only a trailing ".svg" (e.g., use a suffix-only removal such
as replacing /\.svg$/ or using a path-based basename-without-extension approach)
so file names are reliably converted to icon keys (locate the .map(...) call
that builds the icon names in icons.controller.ts).
---
Nitpick comments:
In `@src/views/icons-demo.view.tsx`:
- Around line 41-48: navigator.clipboard.writeText(markdownUrl) is used without
a rejection handler, so clipboard permission denials leave the UI silent; wrap
the promise returned by navigator.clipboard.writeText(markdownUrl) with a
.catch(...) (or use try/catch if converted to async) to handle errors, update
the copyBtn to show an error state/message (e.g., change innerHTML/text, add an
'error' class and ensure aria-live feedback), and ensure any temporary state set
by the success path (like adding 'copied') is not left stuck on error (remove
'copied' in the error handler or clear timers set by setTimeout); reference the
existing navigator.clipboard.writeText(markdownUrl), copyBtn, setTimeout and the
success-path DOM updates when implementing this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4602aa52-1f5c-41d1-a570-8c0af896def6
📒 Files selected for processing (2)
src/controllers/icons.controller.tssrc/views/icons-demo.view.tsx
| copyBtn.onclick = (e) => { | ||
| e.stopPropagation(); | ||
| const origin = window.location.origin || 'https://stats.pphat.top'; | ||
| const markdownUrl = ''; |
Contributor
There was a problem hiding this comment.
Encode icon names before building URLs.
icon is concatenated directly into URL paths. Filenames with spaces or reserved characters will generate broken markdown/image links.
🔧 Suggested fix
- const markdownUrl = '';
+ const encodedIcon = encodeURIComponent(icon);
+ const markdownUrl = '';
@@
- img.src = '/assets/icons/' + icon + '.svg';
+ img.src = '/assets/icons/' + encodeURIComponent(icon) + '.svg';Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/icons-demo.view.tsx` at line 40, The code concatenates raw icon
names into URLs (e.g., the markdownUrl construction using the icon variable in
src/views/icons-demo.view.tsx) which breaks for spaces/reserved characters;
update the URL builders (both occurrences that build '/assets/icons/' + icon +
'.svg', including the markdownUrl and the similar use at line 56) to URL-encode
the filename portion by applying an encoding function such as
encodeURIComponent(icon) when composing the path so generated markdown/image
links remain valid.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a new system for serving and documenting animated tech icons via API endpoints, replacing the previous static HTML approach. It adds a dedicated controller, new Express routes for listing, retrieving, and demoing icons, and integrates these routes into API documentation. It also updates dependencies to include React and its type definitions.
Icons API and Demo System
IconsController(src/controllers/icons.controller.ts) to handle:/icons)/icons/:nameor/icons/:name.svg)/icons/demo)public/icons.htmlfile, moving demo rendering to server-side codesrc/routes/icons.routes.tsfor the above endpoints, including route documentation helperssrc/routes/register.ts) [1] [2]src/routes/docs.routes.ts) [1] [2]Dependency Updates
react,react-dom, and their type definitions topackage.jsondependencies and devDependencies [1] [2]Summary by CodeRabbit
New Features
Dependencies
Chores