GET-44: icon update for badges

[AntonJames-Sistence]

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number:
GET-44: badges and icons update

What is the new behavior?

Screen.Recording.2025-10-21.at.12.55.34.PM.mov

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[netlify]

✅ Deploy Preview for reagraph ready!

Name	Link
🔨 Latest commit	56163be
🔍 Latest deploy log	https://app.netlify.com/projects/reagraph/deploys/68f7cf37afd7a60008c8a016
😎 Deploy Preview	https://deploy-preview-389--reagraph.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[claude]

Code Review - PR #389: Icon support for Badges

Thank you for adding icon support to the Badge component! This is a nice enhancement that adds more flexibility to badge customization.

Strengths

1. Good feature implementation - The icon support is well-integrated with flexible positioning options
2. Comprehensive Storybook examples - Three new stories demonstrate the feature well
3. Proper TypeScript typing - New types (IconPosition) are well-defined and exported
4. Follows project conventions - Code style matches existing patterns (useMemo, prop destructuring, etc.)
5. Dynamic layout calculations - Badge width adjusts correctly when icons are present

Issues Found

Critical Issues

1. Inconsistent SVG import pattern (stories/demos/Badge.story.tsx:6-9) - You are using @ts-expect-error comments, but other story files import SVGs without this suppression. See stories/demos/UseCases.story.tsx which imports SVGs cleanly. Fix: Remove the @ts-expect-error comments.

2. Typo in story example (stories/demos/Badge.story.tsx:102) - Extra space in position array. Fix: Change position={[0, -10 ,0]} to position={[0, -10, 0]}

3. Removed custom from BadgePosition type (src/symbols/nodes/Badge.tsx:15) - The old code had custom as an option, now removed. This could be a breaking change. Was this intentional?

Performance and Code Quality

4. Text width calculation duplication (src/symbols/nodes/Badge.tsx:199, 206, 211, 213) - label.length * 0.15 is calculated multiple times. Extract to a const for better performance.

5. Magic number for icon spacing (src/symbols/nodes/Badge.tsx:213) - 0.15 appears as hardcoded spacing. Extract to named constant.

6. Incomplete custom position handling (src/symbols/nodes/Badge.tsx:190-196) - When using custom iconPosition array, text remains at [0, 0]. Is this intended?

Missing Elements

7. No tests added - PR checklist shows tests are unchecked. Consider adding unit tests for contentLayout calculation logic.

8. No documentation updates - PR checklist shows docs are unchecked.

9. Whitespace changes in unrelated files - .husky/pre-commit and package.json have newline removals at EOF.

Security

No security concerns identified.

Summary

Overall Assessment: Good feature addition with some fixable issues.

Required Changes:

• Remove @ts-expect-error comments
• Fix spacing typo in story
• Clarify removal of custom from BadgePosition
• Refactor text width calculation

Recommended Changes:

• Add tests for layout calculations
• Document custom iconPosition behavior
• Extract magic numbers to constants

Review generated with Claude Code

[claude]

Code Review

Thank you for this feature addition! The implementation adds icon support to badges, which is a nice enhancement. Here is my detailed feedback:

Strengths

1. Good Type Safety: The new IconPosition type and prop interfaces are well-defined with clear JSDoc comments
2. Consistent Patterns: Follows the project conventions for component structure, prop destructuring, and React patterns
3. Performance Optimization: Proper use of useMemo for expensive layout calculations
4. Documentation: Well-documented props with clear descriptions
5. Demo Coverage: Multiple story examples showing different use cases

Code Quality Issues

1. Missing Gap in Width Calculation (src/symbols/nodes/Badge.tsx:155-158)

The width calculation adds iconSize but does not account for the gap between icon and text. Line 157 should conditionally add ICON_TEXT_GAP only when icon position is start or end, not for custom positions.

Current: estimatedWidth += iconSize
Should be: estimatedWidth += iconSize + ICON_TEXT_GAP (when not custom position)

This will cause the badge to be slightly too narrow when using start or end positions, potentially causing text to overflow or touch the icon.

2. Inconsistent Position Calculation (src/symbols/nodes/Badge.tsx:213)

Line 213 contains a magic number 0.5 that lacks context:
iconX: startX + iconSize - 0.5 / 2

This should either use a named constant explaining what 0.5 represents, or be removed if it is meant to be iconSize / 2 (which would make more sense for centering).

3. Incomplete Custom Position Handling

When using custom iconPosition as [x, y], the width calculation still includes the icon in the badge width (line 157), but the layout logic places the icon at custom coordinates. This could cause the badge to be unnecessarily wide or icons to extend beyond the background.

Suggested fix: Only add icon width when iconPosition is not an array.

Testing Issues

Critical: No tests were added for this feature. Per the PR checklist and CLAUDE.md, tests should be added for new features. The project uses Vitest for testing.

Recommendations:

1. Add tests for contentLayout calculation logic covering icon positions start, end, and custom arrays
2. Add tests for badgeDimensions calculation with various icon sizes and text lengths
3. Test edge cases like empty icon, very long text, large iconSize values

Style and Conventions

Minor: The PR includes newline removals from unrelated files (.husky/pre-commit and package.json). These changes should ideally be in a separate cleanup PR or reverted if unintentional.

Security

No security concerns identified. The Image component from drei handles URL loading safely.

Performance

No performance concerns. Proper memoization is used and layout calculations are O(1).

Recommendations

1. Fix the width calculation bug to include ICON_TEXT_GAP conditionally
2. Add comprehensive tests for layout logic
3. Clarify or fix the magic number on line 213
4. Consider edge cases: very large iconSize values, validation for custom position arrays, interaction with maxWidth on Text component

PR Checklist Status

• Tests for the changes have been added: NO
• Feature implementation complete: YES
• Minor unrelated file changes: whitespace only

Verdict

The feature is well-implemented overall and follows project conventions nicely. However, it needs:

1. Tests added (required per checklist)
2. Width calculation bug fixed (functional issue)
3. Magic number clarified (code quality)

Once these are addressed, this will be ready to merge! Great work on the feature and documentation.

[claude]

Code Review - PR #389: Icon update for badges

Thank you for this contribution! This is a well-structured feature addition that adds icon support to the Badge component. Here's my detailed review:

✅ Strengths

1. Good TypeScript typing: The new IconPosition type and updated BadgeProps interface follow the project's conventions correctly
2. Comprehensive documentation: JSDoc comments clearly explain the new props and their behavior
3. Good use of constants: CHAR_WIDTH_ESTIMATE and ICON_TEXT_GAP improve maintainability
4. Proper memoization: The contentLayout calculation is correctly memoized with appropriate dependencies
5. Excellent demo coverage: Three new story examples demonstrate the different icon positioning modes effectively
6. Follows project patterns: Uses useMemo() appropriately and maintains the established coding style

🐛 Potential Issues

1. Missing gap in width calculation (src/symbols/nodes/Badge.tsx:156-158)

   // Add icon width if icon is present
   if (icon) {
     estimatedWidth += iconSize;
   }

   The width calculation adds iconSize but doesn't account for the ICON_TEXT_GAP constant that's used in the layout calculation (line 222). This could cause the icon or text to be clipped at the badge edge.

   Suggestion: Should be:

   if (icon && Array.isArray(iconPosition)) {
     estimatedWidth += iconSize;
   } else if (icon) {
     estimatedWidth += iconSize + ICON_TEXT_GAP;
   }

2. Magic number in layout calculation (src/symbols/nodes/Badge.tsx:213)

   iconX: startX + iconSize - 0.5 / 2,

   The hardcoded 0.5 / 2 appears to be a positioning adjustment but lacks explanation. This should either be a named constant or have a comment explaining what it represents.

3. Removed 'custom' from BadgePosition type (src/symbols/nodes/Badge.tsx:14-19)
   The old code included 'custom' in the BadgePosition union type, which has been removed. While this change isn't necessarily wrong, it could be a breaking change if external code was using position="custom". The PR is marked as non-breaking.

4. Incomplete dependency arrays consideration (src/symbols/nodes/Badge.tsx:226)
   The contentLayout useMemo depends on label.length but the text positioning also uses CHAR_WIDTH_ESTIMATE. If this constant were ever made dynamic, the memo would need updating. This is minor but worth noting for future maintenance.

⚡ Performance Considerations

1. Image loading: The <Image> component from drei loads images asynchronously. Consider:

   • What happens during loading? Is there a flash or layout shift?
   • Should there be error handling for failed image loads?
   • For large graphs with many badges, image loading could impact performance

2. Multiple calculations: The text width is calculated twice - once in badgeDimensions (line 149) and again in contentLayout (line 207). While both are memoized, you could optimize by computing once and passing through.

🔒 Security Considerations

✅ No security concerns. The icon prop accepts URLs/paths which are passed directly to drei's <Image> component - this is safe as it's just rendering, not executing code.

🧪 Test Coverage

Missing: No unit tests were added for the new functionality. Consider adding tests for:

• Badge width calculation with icons
• Content layout positioning for 'start' and 'end' positions
• Content layout with custom [x, y] coordinates
• Edge cases: empty labels with icons, very long labels with icons

📝 Minor Nitpicks

1. Whitespace changes: The changes to .husky/pre-commit and package.json removing final newlines appear unintentional
2. Unused dependency in badgeDimensions: The iconSize is added to width even when iconPosition is a custom array where it might not be needed in the badge width

📋 Recommendations

Before merging:

1. ✅ Fix the width calculation to include ICON_TEXT_GAP
2. ✅ Extract magic number 0.5 / 2 to a named constant
3. ✅ Verify the removal of 'custom' from BadgePosition is intentional and document if it's a breaking change
4. ✅ Revert unintentional whitespace changes to .husky/pre-commit and package.json

Nice to have:
5. Consider adding error handling or loading states for icons
6. Add unit tests for the new positioning logic
7. Document the behavior when image loading fails

Summary

This is a solid feature implementation that follows the project's conventions well. The main issue is the width calculation bug that could cause visual clipping. Once the width calculation is fixed and the breaking change is clarified, this will be ready to merge.

Overall assessment: ⚠️ Needs minor fixes before merge

---

Review generated with assistance from Claude Code following the project's CLAUDE.md guidelines

[claude]

PR Review: Icon Update for Badges

I've reviewed PR #389 and have the following feedback organized by category:

✅ Strengths

1. Good TypeScript Practices

   • Proper type definitions with IconPosition type and extended BadgeProps interface
   • Comprehensive JSDoc comments for new properties
   • Follows the repository's convention of prop interface naming (BadgeProps)

2. Performance Optimizations

   • Properly uses useMemo for contentLayout calculations (line 187-226)
   • Dependencies are correctly specified in all memoization hooks
   • Layout constants extracted to top-level (CHAR_WIDTH_ESTIMATE, ICON_TEXT_GAP)

3. Good Documentation

   • Three new Storybook examples demonstrating different icon positions
   • Clear JSDoc descriptions for new props

🔧 Issues & Recommendations

1. Breaking Change - Removed 'custom' from BadgePosition (❗ High Priority)

Problem: The BadgePosition type previously included 'custom' but it was removed:

// Before (implied from diff)
export type BadgePosition = 'top-right' | 'top-left' | 'bottom-right' | 'bottom-left' | 'center' | 'custom';

// After  
export type BadgePosition = 'top-right' | 'top-left' | 'bottom-right' | 'bottom-left' | 'center';

This is a breaking change that could affect existing consumers using position="custom".

Recommendation:

• If 'custom' was never actually used/implemented, document this removal in the PR description
• If it was used, keep it for backward compatibility or add a migration note
• Check if any code in the repository uses position="custom"

2. Missing Gap in Icon Layout Calculation (🐛 Bug)

Problem: The ICON_TEXT_GAP constant is defined but only used in the 'end' position case (line 222), not in the 'start' position case (line 212-217).

if (iconPosition === 'start') {
  return {
    iconX: startX + iconSize - 0.5 / 2,  // ❌ No gap added
    iconY: 0,
    textX: startX + iconSize + estimatedTextWidth / 2,  // ❌ No gap added
    textY: 0
  };
}

Recommendation: Add gap for consistency:

if (iconPosition === 'start') {
  return {
    iconX: startX + iconSize / 2,
    iconY: 0,
    textX: startX + iconSize + ICON_TEXT_GAP + estimatedTextWidth / 2,
    textY: 0
  };
}

3. Magic Number in Icon Position Calculation (⚠️ Code Quality)

Problem: Line 213 contains a magic number 0.5 / 2:

iconX: startX + iconSize - 0.5 / 2,

Recommendation: Extract to a named constant or add a comment explaining what this represents.

4. Inconsistent Width Calculation (⚠️ Potential Bug)

Problem: When an icon is present, the badge width adds the full iconSize (line 157), but the layout calculations suggest icon and text overlap in the available space. The gap between icon and text isn't accounted for in width calculation.

// In badgeDimensions (line 156-157)
if (icon) {
  estimatedWidth += iconSize;  // ❌ Missing gap
}

// But in contentLayout (line 222) for 'end' position
iconX: startX + estimatedTextWidth + ICON_TEXT_GAP + iconSize / 2,

Recommendation:

if (icon) {
  estimatedWidth += iconSize + ICON_TEXT_GAP;
}

5. Missing Test Coverage (📋 Testing)

Problem: No test file exists for the Badge component. The PR checklist shows:

• Tests for the changes have been added (for bug fixes / features)

Recommendation:

• Add unit tests for the new icon positioning logic
• Test edge cases: empty labels, very long labels, missing icons, custom positions
• Test the layout calculations with different icon sizes

Example test cases:

describe('Badge with icon', () => {
  test('calculates correct layout for icon at start position', () => { ... });
  test('calculates correct layout for icon at end position', () => { ... });
  test('handles custom icon position array', () => { ... });
  test('calculates correct badge width with icon and gap', () => { ... });
});

6. Whitespace-Only Changes (⚠️ Code Quality)

Problem: Changes to .husky/pre-commit and package.json only remove trailing newlines. These changes:

• Add noise to the PR
• Could cause merge conflicts
• Don't relate to the feature

Recommendation: Remove these changes or commit them separately.

7. Missing Accessibility Considerations (💡 Enhancement)

Recommendation: Consider adding:

• alt text support for icons via new prop
• Semantic meaning for screen readers
• ARIA labels when icons convey important information

Example:

icon?: string;
iconAlt?: string;  // New prop for accessibility

📊 Performance Considerations

✅ Good use of useMemo for expensive calculations
✅ Proper dependency arrays prevent unnecessary recalculations
⚠️ The Image component from drei loads images - consider:

• Preloading frequently used icons
• Error handling if image fails to load
• Loading states for async image loads

🔒 Security Concerns

⚠️ URL Injection Risk: The icon prop accepts any URL string and passes it directly to the Image component. While drei likely handles this safely, consider:

1. Input validation if user-generated content is allowed
2. Documentation warning about using only trusted icon sources
3. CSP considerations for production environments

📝 Documentation Gaps

The PR description mentions "GET-44: badges and icons update" but:

• Missing information about what GET-44 is
• No explanation of why 'custom' was removed from BadgePosition
• No migration guide for the breaking change

✅ Action Items

Must Fix (Blocking):

1. Fix gap inconsistency in icon+text layout (bug Improve Label Visibility #2)
2. Fix badge width calculation to include gap (bug Hierarchal Layout #4)
3. Address the 'custom' removal breaking change (framer-motion research #1)

Should Fix (Important):
4. Add tests as indicated in PR checklist (#5)
5. Remove unrelated whitespace changes (#6)
6. Replace magic number with constant (#3)

Nice to Have:
7. Add accessibility props for icons (#7)
8. Document security considerations for icon URLs
9. Add error handling for failed icon loads

---

Overall Assessment: This is a solid feature addition with good TypeScript practices and performance optimizations. However, there are calculation bugs in the icon positioning logic and a potential breaking change that need to be addressed before merging. The code follows the repository's conventions well and the Storybook examples are helpful.

Would you like help fixing any of these issues? I'm happy to suggest specific code changes.