Skip to content

fix(frontend): custom actor icons not displaying#5083

Merged
jog1t merged 1 commit into
mainfrom
05-21-fix_frontend_custom_actor_icons_not_displaying
May 21, 2026
Merged

fix(frontend): custom actor icons not displaying#5083
jog1t merged 1 commit into
mainfrom
05-21-fix_frontend_custom_actor_icons_not_displaying

Conversation

@jog1t
Copy link
Copy Markdown
Contributor

@jog1t jog1t commented May 20, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 20, 2026

Code Review: fix(frontend): custom actor icons not displaying

Overview

This PR fixes a React rendering bug where custom actor icons were not displaying, refactors the duplicated lazy-icon logic into a shared lazy-icon.tsx module, adds infinite scroll to ActorsGrid, and optimises the actor-count probe query in engine-data-provider.tsx.


Root Cause Fix

The bug was in actors-grid.tsx: getLazyIcon called lazy() on every render without caching, giving each render a new component identity. React treats a new identity as an unmount+remount, causing the surrounding Suspense boundary to flash the loading fallback on every parent re-render and making the icon appear stuck permanently.

The fix, caching the lazy() result per export name in a module-level map, is correct, and the comment in getLazyIcon explains the invariant clearly.

actor-builds-list.tsx had a per-file cache already, but the logic was duplicated across files. This PR consolidates both into the shared lazyIconCache in lazy-icon.tsx, eliminating future drift.


frontend/src/components/lazy-icon.tsx (new file)

Correct:

  • Clean separation between LazyIcon (primitive, caller owns Suspense) and ActorIcon (self-contained with its own Suspense boundary and pulsing fallback).
  • fallback as a render-time prop rather than captured in the lazy() closure means the cache key only needs to be the export name. This is a subtle but important correctness point.
  • Glob path ../../packages/icons/dist/icons/*.js resolves correctly from frontend/src/components/ to frontend/packages/icons/dist/icons/, matching the actual file layout (verified on disk).
  • toIconExportName produces names matching the actual .js filenames on disk (verified: faGear.js, faArrowRight.js, faRobot.js).

Minor: emojiClassName fallback:
When emojiClassName is not provided, the component falls back to className. This is intentional but not obvious from the prop name alone. A one-line JSDoc comment on the prop documenting the fallback behaviour would help future callers.

Pre-existing: emoji regex coverage:
The regex (copied from the old files) misses emoji blocks added after Unicode 12 (e.g. U+1FA00 symbols and pictographs). Not introduced by this PR and not a blocker, but worth a follow-up.


frontend/src/app/actors-grid.tsx

Correct:

  • Pagination wiring (hasNextPage, fetchNextPage, isFetchingNextPage) follows the same pattern as actor-builds-list.tsx.
  • Skeleton cards live inside the grid div so they participate in the CSS grid layout. VisibilitySensor lives outside the div in the fragment, which is the right placement.
  • hasNextPage && !isFetchingNextPage guard on the sensor prevents a second fetch from firing while a page is already in flight.

Pre-existing: indentation inside the grid div:
The {sorted.map(...)} block sits at the same indentation level as the opening <div> rather than one level deeper. This was already present before this PR (confirmed via git show HEAD) and is out of scope here.


frontend/src/app/actor-builds-list.tsx

Correctly delegates to ActorIcon from lazy-icon.tsx. The file no longer imports Suspense or lazy since ActorIcon manages its own boundary. Extracting the shared class string to ICON_CLASS and passing it via both className and emojiClassName is clean.


frontend/src/components/ui/icon-picker.tsx

The old private getLazyIcon and lazyIconCache are removed. IconRenderer now delegates to LazyIcon from lazy-icon.tsx. The new LazyIcon applies toIconExportName internally, which produces the same result as the old local toExportName. The shared cache is keyed by the same export-name string, so cache hits work correctly across call sites.


frontend/src/app/data-providers/engine-data-provider.tsx

The batched early-exit probe is a measurable improvement. For a namespace with any actors, the first batch of 32 parallel requests usually returns in one round-trip. Only an empty namespace pays a full sequential scan, which is at most 4 round-trips for the 100-name limit. The probe now returns 0 or 1 rather than a true count, which is fine since all callers only check > 0.

Minor: TODO comment length:
The 12-line comment block is longer than the surrounding convention. The essential constraint fits in two sentences: this returns 0 or 1 since callers only check greater than zero; replace with a single request once the engine supports namespace-wide actor existence. The batch-sizing rationale can be trimmed without losing anything a reader cannot infer from the code.

Minor: BATCH_SIZE placement:
Declared inside the queryFn closure. A module-level constant would be easier to find when tuning, though this is a style preference.


frontend/src/components/lazy-icon.stories.tsx (new file)

Stories cover every branch of ActorIcon: emoji, valid named icon, kebab-case name, unknown name (falls back to default), and null. The regression case, an unknown name producing the same visual as the loading pulse, is explicitly called out in the comment. This is the right level of story coverage for a visual rendering fix. The ladle CSS import path resolves correctly from frontend/src/components/ and matches the pattern in other story files.


Summary

Area Status
Root bug fix (lazy cache per export name) Correct and well-motivated
Deduplication of lazy-icon logic Clean; shared cache eliminates future drift
Pagination in ActorsGrid Consistent with existing pattern
Actor count optimisation Measurable improvement for large namespaces
Stories coverage All branches including regression case
Blocker issues None

The emojiClassName documentation gap and the long TODO comment are minor items; nothing blocks merge.

Copy link
Copy Markdown
Contributor Author

jog1t commented May 21, 2026

Merge activity

  • May 21, 8:51 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • May 21, 9:05 PM UTC: Graphite rebased this pull request as part of a merge.
  • May 21, 9:06 PM UTC: @jog1t merged this pull request with Graphite.

@jog1t jog1t changed the base branch from 05-21-refactor_frontend_unify_settings_pages to graphite-base/5083 May 21, 2026 21:02
@jog1t jog1t changed the base branch from graphite-base/5083 to main May 21, 2026 21:03
@jog1t jog1t force-pushed the 05-21-fix_frontend_custom_actor_icons_not_displaying branch from f6fa0d4 to 7ba3a0d Compare May 21, 2026 21:04
@jog1t jog1t merged commit 9a55a60 into main May 21, 2026
8 of 11 checks passed
@jog1t jog1t deleted the 05-21-fix_frontend_custom_actor_icons_not_displaying branch May 21, 2026 21:06
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.

1 participant