Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Jul 28, 2025

Changes

Added a context menu component to the UI library and implemented a logo context menu in the site header. The context menu allows users to copy the Rivet logo as SVG or download brand assets.

The implementation includes:

  • Added @radix-ui/react-context-menu dependency
  • Created a new context menu component in the components package
  • Added a LogoContextMenu component that wraps the Rivet logo in the header
  • Implemented functionality to copy the logo as SVG and download brand assets

@vercel
Copy link

vercel bot commented Jul 28, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rivet-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 28, 2025 7:20pm

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 28, 2025

Deploying rivet-studio with  Cloudflare Pages  Cloudflare Pages

Latest commit: 044ffbb
Status:🚫  Build failed.

View logs

@claude
Copy link

claude bot commented Jul 28, 2025

Claude finished @jog1t's task —— View job


PR Review: Brand Dropdown Implementation

🎯 Overall Assessment

This is a well-implemented feature that adds a context menu to the Rivet logo, allowing users to copy the logo as SVG or download brand assets. The implementation follows good patterns and integrates cleanly with the existing codebase.

✅ Strengths

Code Quality & Architecture

  • Clean component structure: The context menu component follows established Radix UI patterns with proper TypeScript typing
  • Good separation of concerns: UI components are separated from business logic
  • Consistent styling: Uses the existing design system with proper Tailwind classes and design tokens
  • Accessibility: Leverages Radix UI's built-in keyboard navigation and screen reader support

Integration

  • Non-invasive changes: Header integration is clean and doesn't break existing functionality
  • Proper exports: New components are correctly exported from the component library
  • Dependency management: Uses appropriate Radix UI component for context menus

⚠️ Issues & Improvements

1. Unused Variable - site/src/components/v2/LogoContextMenu.tsx:19

const menuRef = useRef<HTMLDivElement>(null); // ← Never used

Fix: Remove the unused menuRef or implement its intended functionality.

2. Potential Fetch Failure - site/src/components/v2/LogoContextMenu.tsx:23

const response = await fetch(logoUrl.src);

Issue: If the logo is served as a static asset, fetching via HTTP might fail in some environments.
Suggestion: Consider embedding the SVG content directly or ensuring the logo is accessible via HTTP.

3. Hardcoded URL - site/src/components/v2/LogoContextMenu.tsx:34

window.open("https://releases.rivet.gg/press-kit.zip", "_blank");

Issue: Hardcoded URLs are brittle and difficult to maintain.
Suggestion: Move to configuration or constants file:

const BRAND_ASSETS_URL = "https://releases.rivet.gg/press-kit.zip";

4. Error Handling Gap - site/src/components/v2/LogoContextMenu.tsx:33-35

const downloadBrandAssets = () => {
	window.open("https://releases.rivet.gg/press-kit.zip", "_blank");
};

Issue: No error handling if window.open fails (popup blockers, etc.)
Suggestion: Add error handling and fallback messaging.

🔒 Security Review

  • Safe implementation: Uses standard web APIs appropriately
  • External URL: The brand assets URL appears to be from the same domain, which is good
  • No injection risks: No user input is processed

⚡ Performance Considerations

  • Lightweight: Minimal performance impact
  • Lazy loading: Context menu content only renders when triggered
  • Bundle size: Adds ~2KB with the Radix context menu dependency

🧪 Test Coverage

Missing: No tests are included for the new functionality.
Recommendation: Consider adding tests for:

  • Context menu rendering and interaction
  • Copy functionality (with mocked clipboard API)
  • Error handling scenarios
  • Accessibility features

📝 Minor Code Style Notes

  1. frontend/packages/components/src/ui/context-menu.tsx: Excellent implementation following established patterns
  2. Icon sizing consistency: Uses consistent size-4 classes for icons
  3. Error messages: Good user-friendly error messages with toast notifications

✨ Suggestions for Future Enhancements

  1. Configuration: Make brand assets URL configurable
  2. Analytics: Consider tracking context menu usage
  3. Additional options: Could add more brand-related actions (copy link, etc.)
  4. Download progress: For large assets, show download progress

🎯 Recommendation

✅ Approve with minor fixes

This PR implements the requested feature well with good code quality. The issues identified are minor and don't prevent the feature from working correctly. I recommend:

  1. Required: Remove unused menuRef variable
  2. Recommended: Extract hardcoded URL to constants
  3. Nice-to-have: Add basic test coverage

The implementation is production-ready and follows good practices overall.


Copy link
Contributor Author

jog1t commented Jul 28, 2025


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.

@jog1t jog1t marked this pull request as ready for review July 28, 2025 19:17
Copy link
Contributor Author

@jog1t jog1t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @NathanFlurry for vis

@graphite-app
Copy link
Contributor

graphite-app bot commented Jul 28, 2025

Merge activity

  • Jul 28, 7:18 PM UTC: jog1t added this pull request to the Graphite merge queue.
  • Jul 28, 7:19 PM UTC: CI is running for this pull request on a draft pull request (#2813) due to your merge queue CI optimization settings.
  • Jul 28, 7:20 PM UTC: Merged by the Graphite merge queue via draft PR: #2813.

@cloudflare-workers-and-pages
Copy link

Deploying rivet with  Cloudflare Pages  Cloudflare Pages

Latest commit: 044ffbb
Status:🚫  Build failed.

View logs

@cloudflare-workers-and-pages
Copy link

Deploying rivet-hub with  Cloudflare Pages  Cloudflare Pages

Latest commit: 044ffbb
Status: ✅  Deploy successful!
Preview URL: https://1e061462.rivet-hub-7jb.pages.dev
Branch Preview URL: https://07-28-feat-site-add-brand-dr.rivet-hub-7jb.pages.dev

View logs

graphite-app bot pushed a commit that referenced this pull request Jul 28, 2025
## Changes

Added a context menu component to the UI library and implemented a logo context menu in the site header. The context menu allows users to copy the Rivet logo as SVG or download brand assets.

The implementation includes:
- Added `@radix-ui/react-context-menu` dependency
- Created a new context menu component in the components package
- Added a `LogoContextMenu` component that wraps the Rivet logo in the header
- Implemented functionality to copy the logo as SVG and download brand assets
@graphite-app
Copy link
Contributor

graphite-app bot commented Jul 28, 2025

Graphite Automations

"Test" took an action on this PR • (07/28/25)

1 assignee was added to this PR based on Kacper Wojciechowski's automation.

@graphite-app graphite-app bot closed this Jul 28, 2025
@graphite-app graphite-app bot deleted the 07-28-feat_site_add_brand_dropdown branch July 28, 2025 19:20
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