-
Notifications
You must be signed in to change notification settings - Fork 148
feat(rivetkit): add icon & name customization #4033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 01-24-queues_example
Are you sure you want to change the base?
feat(rivetkit): add icon & name customization #4033
Conversation
|
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.
How to use the Graphite Merge QueueAdd 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. |
Pull Request Review: Icon & Name CustomizationThis 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
🔍 Issues & Suggestions1. Performance Issue: useMemo Missing DependenciesLocation: The const iconElement = useMemo(() => {
// Uses: iconValue, isEmoji, lookupFaIcon, faActorsBorderless
}, [iconValue]); // ⚠️ Missing: isEmoji, lookupFaIconImpact: While 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 2. Incomplete Emoji Regex PatternLocation: 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:
Examples that won't work:
Recommendation: Use a well-tested emoji detection library like // 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-regeximport 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 GapLocation: 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 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 CoverageLocation: 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:
5. FontAwesome Icon Lookup Edge CasesLocation: function lookupFaIcon(iconName: string): IconProp | null {
const pascalName = `fa${toPascalCase(iconName)}`;
const iconDef = (allIcons as Record<string, IconProp>)[pascalName];
return iconDef ?? null;
}Potential issues:
Suggestions:
6. Documentation EnhancementLocation: Great documentation! Minor suggestions:
🔒 SecurityNo security concerns identified. The feature only affects UI presentation and doesn't introduce XSS risks (React escapes string content automatically). ⚡ PerformanceMinor concern with the
📝 Code Style & ConventionsGenerally follows the project's patterns well. A few observations:
🎯 Recommendations SummaryMust Fix:
Should Fix: Nice to Have: ✨ Overall AssessmentThis 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 Recommendation: Approve with requested changes for items 1-3 above. Great work on the documentation and thoughtful API design with the precedence system! 🎉 |
503ae21 to
a3e52db
Compare
7404ebe to
32bed1a
Compare

No description provided.