feat(app-form): paginate the icon picker and auto-detect app favicons - #6248
feat(app-form): paginate the icon picker and auto-detect app favicons#6248junkerderprovinz wants to merge 3 commits into
Conversation
The icon picker requested only the default of 12 icons per repository, so repositories with many matching icons (for example selfh.st) were silently truncated with no way to reach the rest. Track the per-repository limit in state, pass it to the icon search, and add a "Show more icons" button that raises it in steps up to the router maximum. Each new search starts compact again. Closes homarr-labs#4330
When creating or editing an app without a chosen icon yet, fetch the linked page and read its `<link rel="icon">` / `apple-touch-icon` declarations, falling back to the origin's `/favicon.ico`. This mirrors the existing name-based icon auto-selection. The fetch runs in an authenticated tRPC procedure, is restricted to http(s) schemes and bounded in both response size and time. Private/LAN targets are intentionally allowed, since Homarr apps commonly live on the local network and that is the same trust model as configuring an integration URL. Closes homarr-labs#2480
🚨 Preview Deployment Blocked - Security ProtectionYour pull request was blocked from triggering preview deployments Why was this blocked?
How to resolve this:Option 1: Get Collaborator Access (Recommended) Option 2: Request Permission Override For Repository Administrators:To disable this security check ( This security measure protects against malicious code execution in preview deployments. Only trusted collaborators should have the ability to trigger deployments. 🛡️ Learn more about this security featureThis protection prevents unauthorized users from:
Preview deployments are powerful but require trust. Only users with repository write access can trigger them. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds automatic favicon detection through a bounded HTML fetcher, protected API endpoint, and app-form integration. It also adds stepped icon-result pagination with a “show more” control. ChangesAutomatic favicon detection
Icon picker pagination
Sequence Diagram(s)sequenceDiagram
participant AppForm
participant iconsRouter
participant fetchBestIconUrlForAppAsync
participant TargetWebsite
AppForm->>iconsRouter: getIconForUrl({ href })
iconsRouter->>fetchBestIconUrlForAppAsync: resolve app icon
fetchBestIconUrlForAppAsync->>TargetWebsite: fetch HTML
TargetWebsite-->>fetchBestIconUrlForAppAsync: HTML response
fetchBestIconUrlForAppAsync->>fetchBestIconUrlForAppAsync: select linked icon
fetchBestIconUrlForAppAsync->>TargetWebsite: check /favicon.ico when needed
TargetWebsite-->>fetchBestIconUrlForAppAsync: reachability response
fetchBestIconUrlForAppAsync-->>iconsRouter: icon URL or null
iconsRouter-->>AppForm: { url }
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🧹 Nitpick comments (3)
packages/forms-collection/src/icon-picker/icon-picker.tsx (1)
63-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the max limit from
limitPerGroupStepsinstead of hardcoding500.
limitPerGroupStepsalready defines the max step (500), buthasMoreIcons(Line 88) hardcodes500separately andshowMoreIcons(Line 91) also falls back to a hardcoded500. If the steps array is ever changed, these values could silently drift out of sync.♻️ Proposed refactor
+const maxLimitPerGroup = limitPerGroupSteps[limitPerGroupSteps.length - 1]!; + const hasMoreIcons = - limitPerGroup < 500 && (data?.icons.some((group) => group.icons.length >= limitPerGroup) ?? false); + limitPerGroup < maxLimitPerGroup && (data?.icons.some((group) => group.icons.length >= limitPerGroup) ?? false); const showMoreIcons = () => { - setLimitPerGroup((current) => limitPerGroupSteps.find((step) => step > current) ?? 500); + setLimitPerGroup((current) => limitPerGroupSteps.find((step) => step > current) ?? maxLimitPerGroup); };Separately, note that
hasMoreIconsusesgroup.icons.length >= limitPerGroupas a proxy for "truncated" since the API only returns an overallcountIcons, not a per-group total. A group whose true total exactly equals the current limit will show a "show more" button that yields no new icons on click (it self-corrects on the next render since the returned length then becomes smaller than the new limit), which is a minor, self-healing cosmetic quirk rather than a functional bug.Also applies to: 75-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/forms-collection/src/icon-picker/icon-picker.tsx` at line 63, The icon picker is hardcoding the max limit in `hasMoreIcons` and `showMoreIcons` instead of deriving it from `limitPerGroupSteps`, which can drift if the steps change. Update the `IconPicker` logic to compute the maximum from the last entry in `limitPerGroupSteps` and use that shared value wherever the fallback or “more” threshold is needed. Keep the existing `useState(initialLimitPerGroup)` and related pagination behavior intact, but replace the literal max values in the relevant `IconPicker` helpers with the derived constant.packages/validation/src/icons.ts (1)
8-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
.url()chain method is deprecated in Zod v4.Zod v4 promotes string-format validators to top-level functions;
z.string().url()still works but is deprecated in favor ofz.url().♻️ Suggested update
export const iconForUrlSchema = z.object({ // Only http(s) targets are fetched server-side; other schemes are rejected. - href: z - .string() - .trim() - .url() - .regex(/^https?:\/\//i), + href: z + .url() + .regex(/^https?:\/\//i), });Note: confirm this matches the exact Zod version pinned in the repo before applying, and that
.trim()composition still fits the top-levelz.url()API (may require reordering as a pipe/transform).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/validation/src/icons.ts` around lines 8 - 15, The href validator in iconForUrlSchema still uses the deprecated z.string().url() chain. Update this schema to use the Zod v4 top-level URL validator in a way that preserves the existing trim and https-only regex behavior, likely by reordering the checks or piping/transforms around z.url(). Make sure the final shape in icons.ts still rejects non-http(s) schemes and matches the repo’s pinned Zod version before changing the API usage.packages/icons/src/favicon-fetcher.ts (1)
115-186: 🚀 Performance & Scalability | 🔵 TrivialRegex-based
<link>parsing is fragile for malformed/edge-case HTML.
linkTagRegex/attributeRegexwork for the tested cases but won't handle things like link tags inside HTML comments, unquoted attributes with>in values, or non-standard whitespace/ordering as robustly as a real parser. Given the bounded, simple use case this is likely fine for now, but consider a lightweight HTML parser if false negatives on real-world sites become an issue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/icons/src/favicon-fetcher.ts` around lines 115 - 186, The `<link>` extraction in `extractBestIconUrl` relies on `linkTagRegex` and `attributeRegex`, which is too brittle for malformed or edge-case HTML. Replace the regex-based parsing with a lightweight HTML parser or DOM-like parser that can accurately walk `<link>` elements and read `rel`, `href`, and `sizes` attributes. Keep `parseAttributes`, `parseSizeArea`, and `resolveAbsoluteUrl` behavior conceptually the same, but feed them data from the parser instead of raw tag strings. Ensure `extractBestIconUrl` still filters by icon-related rel values and selects the best candidate using relation score and size.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/router/icons.ts`:
- Around line 9-11: The new getIconForUrl tRPC query is missing MCP exposure.
Update the procedure chain in iconsRouter so getIconForUrl includes a .meta({
mcp: { enabled: true, description: "..." } }) call before .input(), matching the
pattern used by findIcons and making the new icon-resolution query available as
an MCP tool.
In `@packages/icons/src/favicon-fetcher.ts`:
- Around line 141-148: The sorting logic in favicon-fetcher currently
hard-prioritizes relationScore in the candidates comparator, so a low-resolution
apple-touch-icon can outrank a much larger icon. Update the comparator in the
candidate selection logic to make sizeArea the primary signal and use
relationScore only as a tiebreaker when sizes are comparable, preserving the
intent of picking the best-quality icon. Keep the fix localized to the
candidates.sort block and the return candidates[0] selection.
---
Nitpick comments:
In `@packages/forms-collection/src/icon-picker/icon-picker.tsx`:
- Line 63: The icon picker is hardcoding the max limit in `hasMoreIcons` and
`showMoreIcons` instead of deriving it from `limitPerGroupSteps`, which can
drift if the steps change. Update the `IconPicker` logic to compute the maximum
from the last entry in `limitPerGroupSteps` and use that shared value wherever
the fallback or “more” threshold is needed. Keep the existing
`useState(initialLimitPerGroup)` and related pagination behavior intact, but
replace the literal max values in the relevant `IconPicker` helpers with the
derived constant.
In `@packages/icons/src/favicon-fetcher.ts`:
- Around line 115-186: The `<link>` extraction in `extractBestIconUrl` relies on
`linkTagRegex` and `attributeRegex`, which is too brittle for malformed or
edge-case HTML. Replace the regex-based parsing with a lightweight HTML parser
or DOM-like parser that can accurately walk `<link>` elements and read `rel`,
`href`, and `sizes` attributes. Keep `parseAttributes`, `parseSizeArea`, and
`resolveAbsoluteUrl` behavior conceptually the same, but feed them data from the
parser instead of raw tag strings. Ensure `extractBestIconUrl` still filters by
icon-related rel values and selects the best candidate using relation score and
size.
In `@packages/validation/src/icons.ts`:
- Around line 8-15: The href validator in iconForUrlSchema still uses the
deprecated z.string().url() chain. Update this schema to use the Zod v4
top-level URL validator in a way that preserves the existing trim and https-only
regex behavior, likely by reordering the checks or piping/transforms around
z.url(). Make sure the final shape in icons.ts still rejects non-http(s) schemes
and matches the repo’s pinned Zod version before changing the API usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb128ce6-28b4-45a7-8ef7-8dbb2ba4f147
📒 Files selected for processing (8)
packages/api/src/router/icons.tspackages/forms-collection/src/icon-picker/icon-picker.tsxpackages/forms-collection/src/new-app/_form.tsxpackages/icons/index.tspackages/icons/src/favicon-fetcher.spec.tspackages/icons/src/favicon-fetcher.tspackages/translation/src/lang/en.jsonpackages/validation/src/icons.ts
| import { createTRPCRouter, protectedProcedure, publicProcedure } from "../trpc"; | ||
|
|
||
| export const iconsRouter = createTRPCRouter({ | ||
| getIconForUrl: protectedProcedure.input(iconForUrlSchema).query(async ({ input }) => { |
There was a problem hiding this comment.
Why protected ? Wouldn't that need a normal public procedure in order to use it for other places as well ? Or is it used only within the add creation / edit process ?
|
@ajnart Good question! Right now it's only used inside the add/edit flow, so protected already covers it. I kept it protected on purpose: it does a server-side fetch of a user-supplied URL to auto-detect the favicon, and exposing that as a public (unauthenticated) procedure would turn it into an SSRF vector, since anyone could make the server fetch arbitrary URLs. Any other place that ends up needing it would be behind auth too, so protected still works there. That said, I'm totally happy to switch it to public if you'd rather have it available more broadly. My vote would be to keep it protected for that reason, but just let me know what you prefer and I'll adjust. Thanks for taking a look! |
…ation-first ranking
|
Hi @ajnart — following up on your question about the favicon-detect procedure: I kept it as a |
2 participants
Two related improvements to the app-creation icon flow, bundled since they converge on the same form and icon router.
Closes #4330
Closes #2480
#4330 — Icon picker: show more than 12 icons per repository
The picker requested only the router default of 12 icons per repository, so repositories with many matches (e.g. selfh.st) were silently truncated with no way to reach the rest. The backend already accepts a
limitPerGroup(1–500), so this is a frontend change: the picker now tracks the limit in state, passes it toicon.findIcons, and renders a "Show more icons" button that raises it in steps (12 → 48 → 192 → 500) while any repository is still truncated. Each new search resets to the compact 12 again.#2480 — Auto-detect the app icon from its URL
Requested by @Meierschlumpf. When creating or editing an app that has no icon chosen yet, Homarr now fetches the linked page and reads its
<link rel="icon">/<link rel="apple-touch-icon">declarations (largest size wins), falling back to the origin's/favicon.ico. This mirrors the existing name-based auto-selection right next to it in_form.tsxand only sets the icon while the field is still empty, so it never overrides a user's choice.Security note (favicon fetch): the fetch happens in an authenticated
protectedProcedure, is restricted tohttp(s)schemes, follows redirects, and is bounded in both response size (512 KB, only the<head>is needed) and time (5 s). Private/LAN targets are intentionally not blocked — Homarr apps overwhelmingly live on the local network, and this is the same trust model as configuring an integration or ping URL that the server already fetches. Happy to tighten this further if you'd prefer.New helper
packages/icons/src/favicon-fetcher.tswith unit tests covering the preference order (apple-touch-icon > largest declared size), the/favicon.icofallback, the no-icon case, and non-http scheme rejection.Checklist
tsc --noEmitclean on all touched packages: icons, api, validation, forms-collection, translation)oxfmt,oxlintclean (only the pre-existingexhaustive-depswarning that the neighbouring name-based effect already emits)devOne new i18n key (
common.iconPicker.showMore) added to theen.jsonsource only, per the Crowdin flow.Summary by CodeRabbit
New Features
Bug Fixes
Tests