Skip to content

fix(frontend): only request platform icons that ship - #4792

Merged
gantoine merged 5 commits into
masterfrom
fix/platform-icon-manifest
Sep 26, 2026
Merged

gantoine merged 5 commits into
masterfrom
fix/platform-icon-manifest

Conversation

@gantoine

@gantoine gantoine commented Sep 26, 2026 •

Copy link
Copy Markdown
Member

Description

Fixes #4671

The v2 platform icon components tried /assets/platforms/<slug>.svg, then .ico, and let the browser 404 on every miss before landing on default.ico. The prefetch cache did the same probing for every platform on app load. That's where the ps.svg, win.svg and ps.ico 404s in the issue come from.

This takes the approach from #4728 by @andest01 (look icons up in a list of the files that ship instead of probing) and narrows it to the fix:

  • Build-time manifest. A small Vite plugin (scripts/platformIconManifest.ts) exposes the icons under frontend/assets/platforms/ as virtual:platform-icons, a slug to filename map (.svg wins over .ico). It reloads when an icon is added or removed under the dev server. src/v2/utils/platformIcons.ts exports platformIconUrl(slug, fsSlug); unknown slugs go straight to default.ico without a request.
  • Why not import.meta.glob. An eager ?url glob over the folder would import every icon just to read its name. I measured it: it emits 233 hashed copies of the icons (~5.6 MB) into dist/assets and inlines 26 small ones as data: URIs into the lib chunk (165 KB to 273 KB). The virtual module only adds the names.
  • Prefetch. prefetchPlatformIcons skips slugs that ship nothing and fetches one URL per shipped slug.
  • RPlatformIcon becomes components/shared/PlatformIcon. It now depends on platform-specific asset knowledge, so it no longer qualifies as a lib/ primitive. Consumers import it by path; the story moved with it, and the Storybook test harness (test/storybook.test.ts) now covers components/shared stories too, so it keeps rendering and running its axe checks in CI. CachedPlatformIcon uses the same resolver.

Behavior change: custom icon mounts. The docs let users bind-mount their own icons over /var/www/html/assets/platforms. The new UI now only knows the icons built into the image, so it ignores those mounts. This is intended since the classic UI is going away; rommapp/docs#150 removes the custom icon guide and should merge when this ships.

Left out on purpose: the Storybook staticDirs/fixtures work from #4728 (separate PR), and merging CachedPlatformIcon into PlatformIcon (follow-up).

flowchart LR
  A["assets/platforms/*.svg|ico"] -->|"build: readdir"| M["virtual:platform-icons"]
  M --> R["platformIconUrl(slug, fsSlug)"]
  R -->|shipped| U["/assets/platforms/file"]
  R -->|not shipped| D["default.ico, no request"]
  R --> P["PlatformIcon / CachedPlatformIcon"]
  M --> F["prefetchPlatformIcons: one fetch per shipped slug"]
Loading

AI assistance: This PR was written with Claude Code (Claude Opus 5.5): the implementation, tests, review passes and this description. I reviewed the approach against #4728 and the diff before opening.

Checklist

  • I've tested the changes locally
  • I've updated relevant comments
  • I've assigned reviewers for this PR
  • I've added unit tests that cover the changes

Local checks: npm run typecheck, typecheck:scripts, npm run test (235 files, 2655 tests; the Storybook harness now runs 408 stories including 31 shared ones), npm run build, storybook build, and trunk check. I did not do a manual browser pass: nothing visual changes, only which URL each icon requests.

Screenshots (if applicable)

No visual change.

🤖 Generated with Claude Code

The v2 platform icon components probed /assets/platforms/<slug>.svg, then
.ico, and let the browser 404 on every miss before settling on default.ico.
A build-time manifest of the shipped filenames (virtual:platform-icons) now
picks the right file up front, so unknown slugs go straight to the default
and the prefetch cache makes at most one request per shipped slug.

The manifest is a Vite virtual module rather than an eager import.meta.glob,
which would emit hashed copies of all ~240 icons and inline the small ones
into the lib chunk just to read their names.

RPlatformIcon moves out of lib/ to components/shared/PlatformIcon, since it
now depends on platform-specific asset knowledge. Its tooltip also falls
back to the slug when no title or alt is given (the empty-string alt default
used to short-circuit the `??` chain).

Based on the approach in #4728 by @andest01.

Fixes #4671

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 26, 2026 01:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@greptile-apps

greptile-apps Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[Medium risk] Refactors platform icon loading to skip unavailable icons.

This PR should not merge until documented runtime-mounted custom platform icons remain usable and the repository-rule violations are resolved.

Summary

The PR generates a build-time platform-icon manifest, selects known icon URLs without probing missing files, updates prefetching, and moves the v2 icon component out of the primitive library.

  • The fixed manifest conflicts with the documented runtime custom-icon workflow.
  • Icon-load errors no longer try other available platform-specific candidates.
  • Development refresh and two repository test/comment requirements need attention.

Reviews (1) · Last reviewed commit: "fix(frontend): only request platform ico..."

Comment thread frontend/scripts/platformIconManifest.ts Outdated
Comment thread frontend/src/v2/components/shared/PlatformIcon.vue
Comment thread frontend/scripts/platformIconManifest.ts
Comment thread frontend/src/v2/components/shared/PlatformIcon.test.ts Outdated
Comment thread frontend/scripts/platformIconManifest.ts Outdated
@gantoine gantoine mentioned this pull request Sep 26, 2026
4 tasks
gantoine and others added 3 commits September 25, 2026 21:31
- Emit the slug-to-file map from the manifest plugin and reload it when
  icons change under the dev server.
- Default the plugin's icon directory so both configs share one path.
- Keep decorative platform icons at an empty alt, and turn off the slug
  tooltip where the display name already sits next to the icon.
- Clean up the manifest test's temp dir and cover CachedPlatformIcon.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Move the slug-to-URL resolver into src/v2/utils/platformIcons so
  PlatformIcon no longer pulls in the blob cache, and normalize slugs
  through one helper.
- Keep PlatformIcon's tooltip as title or alt, which drops the three
  :show-tooltip="false" overrides the slug fallback needed.
- Drop the plugin's unused directory argument, the redundant onError
  guards and the doubled fsSlug lookup, and shorten the test helper.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
PlatformIcon's story moved out of src/v2/lib, which the harness only globbed, so CI stopped rendering it and running its axe checks. Covering components/shared restores that and adds the four other shared stories.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
gantoine added a commit to rommapp/docs that referenced this pull request Sep 26, 2026
The new UI only loads the icons built into the image (rommapp/romm#4792), and the classic UI is being removed, so bind-mounting custom icons is no longer supported.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@gantoine
gantoine merged commit 72a1cd0 into master Sep 26, 2026
12 checks passed
@gantoine
gantoine deleted the fix/platform-icon-manifest branch September 26, 2026 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] 404 Missing frontend assets

2 participants