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

PR Review: Icon & Name Customization

Thanks for this feature! The ability to customize actor appearance in the Inspector is a great developer experience improvement. Overall, the implementation is solid and well-documented. Here's my feedback:


✅ Strengths

  1. Excellent Documentation: The new appearance.mdx page is comprehensive with clear examples and explains both basic usage and the advanced RunConfig pattern for library authors.

  2. Clean Architecture: The precedence system (actor options > run metadata) is logical and well-implemented in registry/config/index.ts:267-284.

  3. Type Safety: Good use of Zod schemas and TypeScript types throughout. The RunConfigSchema properly validates the structure.

  4. Helper Functions: getRunFunction and getRunMetadata provide clean separation of concerns in actor/config.ts:59-74.


🔍 Issues & Suggestions

1. Performance Issue: useMemo Dependencies in Frontend ⚠️

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

The iconElement is memoized but depends on iconValue, which is calculated outside the useMemo. The computation is already very cheap (emoji regex test + object lookup), so useMemo adds unnecessary complexity here.

Recommendation: Remove useMemo entirely - the computation is already very cheap:

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;

let iconElement;
if (iconValue && isEmoji(iconValue)) {
  iconElement = <span className="...">{iconValue}</span>;
} else {
  const faIcon = iconValue ? lookupFaIcon(iconValue) : null;
  iconElement = <Icon icon={faIcon ?? faActorsBorderless} className="..." />;
}

2. Incomplete Emoji Regex ⚠️

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

The current regex misses many valid emojis:

  • Missing ranges like supplemental symbols, extended pictographs
  • Doesn't handle compound emojis with ZWJ sequences (👨‍👩‍👧‍👦)
  • Doesn't handle skin tone modifiers (👋🏽)

Recommendation: Use a more comprehensive pattern or the emoji-regex npm package for production-grade emoji detection.


3. Case Sensitivity in FontAwesome Lookup 💡

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

The conversion assumes kebab-case input (e.g., "chart-line""faChartLine"), but doesn't handle variations like camelCase or snake_case.

Recommendation: Document the expected format (kebab-case) clearly in the docs, or add input normalization to handle common variations.


4. Type Safety: any in Helper Functions 💡

Location: rivetkit-typescript/packages/rivetkit/src/actor/config.ts:59-74

Using any for function arguments/return types loses type safety. Consider using unknown or generic constraints.


5. Missing Error Handling for Invalid Icons 💡

Current behavior: Falls back to faActorsBorderless silently.

Recommendation: Consider adding development warnings when invalid icon names are provided to help developers debug configuration issues.


6. Documentation: Missing Frontmatter 📝

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

According to CLAUDE.md, docs require frontmatter with title, description, and skill fields. This file is missing frontmatter entirely.

Required:

---
title: "Icons & Names"
description: "Customize actor appearance with display names and icons in the Rivet Inspector"
skill: false
---

7. Code Style: Comment Format 📝

Location: rivetkit-typescript/packages/rivetkit/src/actor/config.ts:45-52

Minor inconsistency: Mixing regular comments (//) with JSDoc comments (/** */). Use JSDoc for all exported types/functions for better IDE integration.


🧪 Testing Suggestions

No tests were added for this feature. Consider adding:

  1. Unit tests for helper functions:

    • Test isEmoji with various emoji types
    • Test toPascalCase with edge cases
    • Test lookupFaIcon with valid/invalid icon names
  2. Integration tests:

    • Verify buildActorNames correctly merges options and run metadata
    • Test precedence: actor options should override run metadata
  3. Frontend tests:

    • Verify emoji and FontAwesome icons render correctly
    • Verify fallback behavior

🔒 Security

No security concerns identified. The icon values are used only for UI rendering and don't pose XSS risks since React escapes strings automatically.


📊 Summary

Category Status
Code Quality ⚠️ Minor issues with useMemo usage
Bugs ⚠️ Emoji regex incomplete, may miss valid emojis
Performance ⚠️ Unnecessary complexity from useMemo
Security ✅ No concerns
Testing ⚠️ No tests added
Documentation ⚠️ Missing frontmatter, otherwise excellent

🎯 Recommendations Priority

Must fix before merge:

  1. Add frontmatter to appearance.mdx

Should fix:

  1. Fix useMemo usage in actor-builds-list.tsx (remove it)
  2. Improve emoji regex or use a library
  3. Add tests for helper functions

Nice to have:

  1. Add development warnings for invalid icon names
  2. Improve type safety (any → unknown/generics)
  3. Document expected icon name format (kebab-case)

Great work overall! This is a valuable UX improvement. Let me know if you'd like me to help with any of these issues.

@NathanFlurry NathanFlurry force-pushed the 01-24-feat_rivetkit_add_icon_name_customization branch from 503ae21 to a3e52db Compare January 28, 2026 01:12
@NathanFlurry NathanFlurry force-pushed the 01-24-feat_rivetkit_add_icon_name_customization branch from a3e52db to d5e770c Compare January 30, 2026 08:19
@claude
Copy link

claude bot commented Jan 30, 2026

Pull Request Review: feat(rivetkit): add icon & name customization

Overview

This PR adds support for customizing actor appearance in the UI with display names and icons. Actors can now specify name and icon properties that appear in the Rivet inspector and dashboard.

Code Quality & Best Practices

✅ Strengths

  1. Well-structured type safety: The implementation uses Zod schemas effectively with proper TypeScript types
  2. Good documentation: Comprehensive MDX documentation with examples and clear explanations
  3. Backward compatibility: The feature is fully optional and doesn't break existing code
  4. Clean separation: Metadata extraction is properly separated with getRunFunction() and getRunMetadata() utility functions
  5. Sitemap updated: The new docs page is properly registered in the sitemap

⚠️ Issues & Recommendations

1. Frontend: Missing useMemo dependencies (frontend/src/app/actor-builds-list.tsx:60-76)

The useMemo hook has an incomplete dependency array. It only includes iconValue but doesn't track all the values it uses.

Recommendation: Either add all dependencies or remove useMemo entirely since the computation is lightweight (just conditional rendering). The memoization might be premature optimization here.

2. Frontend: Emoji regex is incomplete (frontend/src/app/actor-builds-list.tsx:12-13)

The emoji regex doesn't cover all Unicode emoji ranges. Missing skin tone modifiers, zero-width joiners, variation selectors, and newer emoji blocks.

Recommendation: Use emoji-regex library or Intl.Segmenter for proper emoji detection.

3. Frontend: Helper functions could be extracted

Utility functions (isEmoji, capitalize, toPascalCase, lookupFaIcon) are defined inline.

Recommendation: Extract to frontend/src/utils/icon-helpers.ts for better testability and potential reuse.

Potential Bugs

🟡 Medium Priority

Null/undefined handling in frontend (frontend/src/app/actor-builds-list.tsx:51-58)

The code doesn't validate if build.name.metadata exists before accessing properties. If undefined, this throws a runtime error.

Fix: Add optional chaining for build.name.metadata?.icon and build.name.metadata?.name

Performance Considerations

  1. Icon lookup: lookupFaIcon performs a dynamic lookup on every render. Consider adding a module-level cache.
  2. Wildcard import: import * as allIcons could impact bundle size. Verify tree-shaking is working properly.

Security Concerns

✅ No major issues - XSS safe, icon injection safe, metadata type-checked.

🟡 Minor: Consider length limits for very long emoji sequences.

Test Coverage

❌ No tests included

This PR adds significant functionality but includes no tests. Recommend adding:

  1. Unit tests for getRunFunction and getRunMetadata
  2. Tests for buildActorNames verifying metadata precedence
  3. Frontend utility tests for isEmoji, lookupFaIcon, etc.
  4. Integration test verifying workflow() sets diagram-project icon

Summary

Approve with recommended changes

Priority fixes:

  1. Important: Add optional chaining for build.name.metadata access (prevents runtime errors)
  2. Recommended: Add test coverage for new functionality
  3. Nice to have: Improve emoji detection and extract utility functions

Overall assessment: ✅ Approve with minor changes

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