fix(hub-ui): show failed icons and retry on remount - #381
Conversation
◈ PR Lens
Architecture 
3 components touched across 2 lanes. Inside the changed components — 1 viewComponent view — Iconify rendering & caching Component modules for icon fetching, status validation, and error glyphs 
Data flow 
View
Tip Run 🪧 More tips
Thanks for using PR Lens! It's built by Coldtea, free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
|
@dvcolomban is attempting to deploy a commit to the NuxtLabs Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes directly address the stated failure modes, include targeted regression tests (including real mounted component behavior), and don’t introduce obvious correctness or safety regressions in the reviewed code paths.
Pull request overview
This PR improves the hub UI’s remote Iconify icon loading so transient failures don’t permanently blank mounted dock icons, and so non-OK HTTP responses/invalid SVG bodies aren’t cached as if they were valid icon content.
Changes:
- Added retry logic for transient network/timeout errors and retryable HTTP statuses (408/429/5xx) while sharing in-flight work across concurrent consumers.
- Sanitized and validated fetched SVGs before caching, and ensured failures are evicted so later renders can retry.
- Added jsdom-backed regression tests and a per-package Vitest config to exercise mounted Vue behavior and stale-response handling.
File summaries
| File | Description |
|---|---|
| pnpm-workspace.yaml | Adds jsdom to the shared testing catalog for consistent workspace resolution. |
| pnpm-lock.yaml | Locks jsdom and related transitive dependencies and updates Vitest optional dependency graph accordingly. |
| packages/hub-ui/vitest.config.ts | Introduces hub-ui’s Vitest project config (Vue SFC plugin + workspace aliases). |
| packages/hub-ui/src/client/utils/iconify.ts | Implements retry + status handling and SVG sanitize/validate-before-cache behavior. |
| packages/hub-ui/src/client/utils/iconify.test.ts | Adds regression tests for retries, caching/eviction, sanitization, and mounted component recovery. |
| packages/hub-ui/src/client/components/icons/IconifyIcon.vue | Prevents stale async icon loads from updating state after unmount or icon changes. |
| packages/hub-ui/src/client/components/icons/IconifyIcon.test.ts | Tests stale-response suppression when the icon prop changes mid-flight. |
| packages/hub-ui/package.json | Adds jsdom as a dev dependency for hub-ui’s jsdom-based tests. |
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 7/8 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The changes align with the stated intent, appear safe for runtime usage, and are backed by targeted regression tests; only minor consistency/cleanup nits were identified.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/hub-ui/src/client/components/icons/IconifyIcon.vue:31
iconifyLoaded.valueis set toundefinedat the start of the effect, so the extra assignment in the!iconifyParsedearly-return branch is redundant.
packages/hub-ui/vitest.config.ts:9- In this repo, other Vitest project configs use
defineConfig(e.g.packages/json-render-ui/vitest.config.ts) and import the Vue plugin asvue(lowercase). UsingdefineProject/Vuehere is inconsistent and makes configs harder to scan across packages.
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
00bd653 to
9e058e4
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
9e058e4 to
1f99511
Compare
dvcolomban
changed the title
There was a problem hiding this comment.
🟢 Approval recommended
The changes correctly prevent caching invalid Iconify results, add a reliable fallback + retry-on-remount behavior, and are backed by targeted browser tests for the new failure modes.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
1f99511 to
7a2aab6
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes directly address the caching failure mode described, and the remaining feedback is limited to minor naming/accessibility improvements rather than correctness issues.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The URL/data <img> fallback in IconifyIcon.vue still lacks decorative semantics (alt="" + aria-hidden="true"), making accessibility behavior inconsistent across icon sources.
Review details
Suppressed comments (1)
packages/hub-ui/src/client/components/icons/IconifyIcon.vue:59
- The URL/data
imgbranch isn’t marked decorative: it has noaltand noaria-hidden, while the Iconify/failure SVG branches arearia-hidden. In dock buttons that already have anaria-label, this can cause screen readers to announce a stray “image” depending on icon source. Addalt=""andaria-hidden="true"to keep accessibility semantics consistent.
<img
v-else :src="icon"
class="w-full h-full m-auto"
draggable="false"
>
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are localized, align with the stated failure mode, and the updated logic prevents caching invalid SVG while safely degrading the UI on fetch failures.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are localized and consistent with the stated fix, with only minor non-blocking diagnostics/message improvements noted.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packages/hub-ui/src/client/utils/iconify.ts:30
- The thrown error for non-2xx responses omits which icon failed (collection/icon), which makes debugging production reports harder (especially when multiple icons are loading concurrently). Include the icon id (and optionally statusText/URL) in the message.
This issue also appears on line 35 of the same file.
packages/hub-ui/src/client/utils/iconify.ts:37
- The generic "invalid SVG" error doesn’t include which icon triggered it, which makes it difficult to diagnose when this happens in the wild (e.g. CDN returning an HTML error page with 200). Include the icon id in the error message.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, locally contained, and correctly address the failure-caching and UI fallback behavior described in the PR.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Background (Why)
HTTP errors from Iconify were cached as successful SVG content, leaving blank dock icons. Rejected requests already leave the cache so a later mount can try again.
Changes (What)
Reject unsuccessful HTTP responses before caching, preserving the existing SVG sanitization. Show a bundled, color-inheriting fallback after failure and ignore results from obsolete component requests. Keep the existing shared requests and 10-second timeout; retry on the next mount or icon change.
Verification (Testing)
All 85 existing hub-ui tests, affected lint and type checks pass after simplification. Earlier browser validation covered request failures, shared caching, remount recovery and stale/unmounted components; its temporary harness is excluded from this PR.
The screenshots capture a forced HTTP 503 with Settings still usable, followed by recovery after reload with a deterministic SVG response. Screenshot evidence is stored separately from this PR.