Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

Copy link
Member Author

NathanFlurry commented Jan 24, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Jan 24, 2026

Pull Request Review: Icon & Name Customization

This PR adds the ability to customize actor display names and icons in the Inspector UI. Overall, the implementation is solid with good documentation and follows the project's patterns well.


Strengths

  1. Clean API Design: The dual approach (options-level and run-level metadata) provides good flexibility for both end users and library developers.

  2. Comprehensive Documentation: The new appearance.mdx file is well-written with clear examples covering emojis, FontAwesome icons, and advanced run handler metadata patterns.

  3. Backwards Compatibility: Gracefully handles missing metadata by falling back to defaults, maintaining compatibility with existing actors.

  4. Type Safety: Good use of Zod schemas for runtime validation while maintaining TypeScript types.


🔍 Issues & Suggestions

1. Performance Issue: useMemo Missing Dependencies

Location: frontend/src/app/actor-builds-list.tsx:60-76

The useMemo hook is missing dependencies in its dependency array:

const iconElement = useMemo(() => {
  // Uses: iconValue, isEmoji, lookupFaIcon, faActorsBorderless
}, [iconValue]); // ⚠️ Missing: isEmoji, lookupFaIcon

Impact: While isEmoji and lookupFaIcon are stable functions defined outside the component, React's linting rules expect them in the dependency array. This could cause stale closures if these functions were ever moved inside the component or made dynamic.

Fix: Either include all dependencies or move the memoization logic outside the map callback if the dependencies are truly stable.

Alternative approach (more performant):

// Move helper logic outside component to avoid re-creating on each render
function resolveIconElement(iconValue: string | null): JSX.Element {
  if (iconValue && isEmoji(iconValue)) {
    return (
      <span className="opacity-80 group-hover:opacity-100 group-data-active:opacity-100 text-sm">
        {iconValue}
      </span>
    );
  }
  const faIcon = iconValue ? lookupFaIcon(iconValue) : null;
  return (
    <Icon
      icon={faIcon ?? faActorsBorderless}
      className="opacity-80 group-hover:opacity-100 group-data-active:opacity-100"
    />
  );
}

// Then in the component:
const iconElement = resolveIconElement(iconValue);

This avoids useMemo entirely since the computation is cheap and doesn't create new object references.


2. Incomplete Emoji Regex Pattern

Location: frontend/src/app/actor-builds-list.tsx:12-13

const emojiRegex =
  /[\u{1F300}-\u{1F9FF}]|[\u{2600}-\u{26FF}]|[\u{2700}-\u{27BF}]|[\u{1F600}-\u{1F64F}]|[\u{1F680}-\u{1F6FF}]|[\u{1F1E0}-\u{1F1FF}]/u;

Issues:

  • Missing many emoji ranges (e.g., basic emoticons ☺️, symbols ✅, skin tone modifiers, etc.)
  • Won't detect compound emojis (👨‍👩‍👧‍👦, flag sequences like 🇺🇸)
  • Doesn't handle variation selectors (U+FE0F)

Examples that won't work:

  • (U+2705) - Checkmark
  • (U+2B50) - Star
  • 🇺🇸 (Regional indicators)

Recommendation: Use a well-tested emoji detection library like emoji-regex or expand the pattern significantly. For a quick fix:

// More comprehensive (still not perfect for all edge cases)
const emojiRegex = /\p{Emoji}/u;

function isEmoji(str: string): boolean {
  return /^\p{Emoji}+$/u.test(str.trim());
}

Or better yet, install and use a dedicated library:

pnpm add emoji-regex
import emojiRegex from 'emoji-regex';

function isEmoji(str: string): boolean {
  const regex = emojiRegex();
  const match = str.match(regex);
  return match !== null && match[0] === str.trim();
}

3. Potential Type Safety Gap

Location: frontend/src/app/actor-builds-list.tsx:51-58

const iconValue = typeof build.name.metadata.icon === "string"
  ? build.name.metadata.icon
  : null;
const displayName = typeof build.name.metadata.name === "string"
  ? build.name.metadata.name
  : build.id;

This assumes build.name.metadata exists and has the right shape, but there's no runtime validation. If the backend sends unexpected data, this could fail.

Suggestion: Add defensive checks:

const metadata = build.name?.metadata ?? {};
const iconValue = typeof metadata.icon === "string" ? metadata.icon : null;
const displayName = typeof metadata.name === "string" ? metadata.name : build.id;

4. Missing Test Coverage

Location: Entire PR

No test files were added or modified. For a feature touching both backend metadata and frontend rendering, tests would strengthen confidence.

Suggested test cases:

  1. Backend (rivetkit-typescript/packages/rivetkit/src/):

    • getRunFunction with function input
    • getRunFunction with RunConfig object
    • getRunMetadata extraction
    • buildActorNames precedence (options override run metadata)
    • Edge cases: undefined values, empty strings
  2. Frontend (frontend/src/app/):

    • isEmoji detection with various inputs
    • lookupFaIcon with valid/invalid icon names
    • toPascalCase conversion
    • Icon rendering with emoji vs FontAwesome
    • Fallback behavior when metadata is missing

5. FontAwesome Icon Lookup Edge Cases

Location: frontend/src/app/actor-builds-list.tsx:30-34

function lookupFaIcon(iconName: string): IconProp | null {
  const pascalName = `fa${toPascalCase(iconName)}`;
  const iconDef = (allIcons as Record<string, IconProp>)[pascalName];
  return iconDef ?? null;
}

Potential issues:

  • No validation that the icon exists before use
  • Type assertion as Record<string, IconProp> bypasses type safety
  • toPascalCase doesn't handle edge cases (e.g., "chart-line-up" → "faChartLineUp" is correct, but "xmark" → "faXmark" vs "faXMark"?)

Suggestions:

  1. Consider logging a warning when an icon lookup fails (dev mode only)
  2. Document expected icon name format in JSDoc
  3. Add validation for common mistakes (e.g., users passing "fa-rocket" instead of "rocket")

6. Documentation Enhancement

Location: website/src/content/docs/actors/appearance.mdx

Great documentation! Minor suggestions:

  1. Add a "Common Icons" table with examples (gamepad, users, database, cog, etc.)
  2. Link to FontAwesome's search page for discovery
  3. Mention that not all FontAwesome icons may be available (depends on the imported set)
  4. Add a troubleshooting section for "icon not showing"

🔒 Security

No security concerns identified. The feature only affects UI presentation and doesn't introduce XSS risks (React escapes string content automatically).


Performance

Minor concern with the useMemo usage noted above. Otherwise, performance impact is minimal since:

  • Metadata extraction happens once during registry build
  • Icon resolution is lightweight
  • No expensive operations in the render loop

📝 Code Style & Conventions

Generally follows the project's patterns well. A few observations:

  1. Good: Comments follow sentence structure (e.g., "Extract run metadata...")
  2. Good: Proper use of Zod schemas with JSDoc
  3. ⚠️ Note: The inline helper functions in actor-builds-list.tsx (lines 12-34) could be moved to a separate utility file for reusability and testability

🎯 Recommendations Summary

Must Fix:

  1. Fix useMemo dependency array or refactor to avoid memoization

Should Fix:
2. Improve emoji detection regex (consider using a library)
3. Add defensive checks for build.name.metadata
4. Add test coverage

Nice to Have:
5. Enhance FontAwesome icon lookup with better error handling
6. Expand documentation with icon examples table
7. Extract utility functions to separate file


Overall Assessment

This is a well-designed feature with clean API design and excellent documentation. The main concerns are around edge case handling (emoji detection, metadata validation) and missing test coverage. The performance issue with useMemo should be addressed, but it's not critical.

Recommendation: Approve with requested changes for items 1-3 above.

Great work on the documentation and thoughtful API design with the precedence system! 🎉

@NathanFlurry NathanFlurry force-pushed the 01-24-feat_rivetkit_add_icon_name_customization branch from 503ae21 to a3e52db Compare January 28, 2026 01:12
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.

2 participants