Skip to content

Conversation

@NicholasKissel
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 15, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivet-inspector Error Error Nov 15, 2025 8:07pm
rivet-site Error Error Nov 15, 2025 8:07pm
rivetkit-serverless Error Error Nov 15, 2025 8:07pm
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 15, 2025 8:07pm

Copy link
Member Author

NicholasKissel commented Nov 15, 2025

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 Nov 15, 2025

PR Review: v4 Website Changes

Overall Assessment: This is a comprehensive website redesign with modern interactive components. The code quality is generally good, but there are several areas that need attention before merging.

CRITICAL ISSUES:

  1. Memory Leak in NewHeroSection.tsx (lines 192-232)
    The useEffect hook in ActorAnimation has missing cleanup that will cause memory leaks. The effect depends on step state that changes within the timeout itself, creating a new timer on every step change without properly cleaning up when component unmounts during mid-sequence.

Fix: Track if component is mounted and prevent state updates after unmount.

  1. Unused Component (line 128)
    DatabaseNode component is defined but never used. Either remove it or use it instead of the inline database node markup at lines 322-331.

PERFORMANCE CONCERNS:

  1. Injecting Styles via dangerouslySetInnerHTML (lines 6-113)
    Issues: No deduplication if multiple instances mount, SSR/CSR mismatch, CSS re-parsed on every render, external Google Fonts import adds network request.
    Recommendation: Move all styles to v2.css or CSS module.

  2. Missing Animation Cleanup in NewFeaturesBento (lines 6-30)
    Event listeners added to .bento-box elements but the return statement is inside forEach instead of useEffect. Cleanup never runs - memory leak.

  3. RequestAnimationFrame Leak in NewUseCases (lines 41-66)
    Old animation frame could still run when effect re-executes with new dependencies.

CODE QUALITY ISSUES:

  1. Hardcoded Magic Numbers (lines 164-165, 178-179)
    Magic pixel values like 366px, 211px, 376px make code fragile. Use constants or CSS custom properties.

  2. String-Based State Management (line 189)
    Using string literals for state is error-prone. Consider TypeScript enums or constants.

  3. Accessibility Issues

  • Missing aria-label on SVG icons
  • No keyboard navigation for drag scrolling
  • No prefers-reduced-motion support

POSITIVE OBSERVATIONS:

  • Clean component structure
  • Proper TypeScript usage
  • Responsive design with Tailwind
  • Smooth animations with good timing
  • Proper use of IntersectionObserver

ACTION ITEMS:

  1. Fix memory leak in ActorAnimation (BLOCKER)
  2. Fix event listener cleanup in NewFeaturesBento (BLOCKER)
  3. Move inline styles to CSS files
  4. Add accessibility features
  5. Remove unused DatabaseNode component

Estimated effort: 1-2 hours

Let me know if you need help implementing these fixes!

@claude
Copy link

claude bot commented Nov 15, 2025

PR Review: v4 Website Changes

Summary

This PR includes a mix of backend fixes, frontend improvements, and a major website redesign. Overall, the code quality is good with proper linting fixes and React best practices applied. However, there are a few areas that need attention.


✅ Strengths

Backend (Rust)

  1. Import Organization - Good cleanup in api-peer/src/actors/kv_get.rs:1-3 fixing import order to follow Rust conventions
  2. Code Formatting - Proper multi-line formatting in api-public/src/actors/kv_get.rs:68-72 improves readability

Frontend (TypeScript/React)

  1. Comprehensive Linting Fixes - Excellent work addressing React Hook dependency warnings throughout the codebase
  2. Proper biome-ignore Usage - Appropriate use of biome-ignore comments with clear justifications (e.g., skeleton loaders, dynamic deps)
  3. Code Quality Improvements:
    • Better children prop usage in plan-card.tsx:62-72 (using children inside component tags instead of as props)
    • Removed unreachable code in default-data-provider.tsx (dead code after throw statements)
    • Fixed useEffect dependencies across multiple files

Website

  1. Modern Design - New hero section with animated actors demonstrates the product effectively
  2. Scroll Animations - Clean ScrollObserver implementation for progressive reveal effects

⚠️ Issues & Recommendations

🔴 Critical Issues

1. Security: XSS Risk with dangerouslySetInnerHTML (High Priority)

Location: website/src/app/(v2)/(marketing)/(index)/sections/NewHeroSection.tsx:8-123 and website/src/app/(v2)/(blog)/changelog/page.tsx:16-43

Issue: Using dangerouslySetInnerHTML to inject entire stylesheets is a security and maintenance concern.

Recommendation:

// Instead of inline style injection, use:
// 1. Move CSS to a proper CSS/SCSS file in website/src/styles/
// 2. Or use CSS-in-JS with a library like styled-components
// 3. Or use Tailwind's @keyframes in tailwind.config.ts

// Example in tailwind.config.ts:
theme: {
  extend: {
    keyframes: {
      'pan-grid': {
        '0%': { backgroundPosition: '0% 0%' },
        '100%': { backgroundPosition: '3rem 3rem' }
      }
    }
  }
}

Why: Using dangerouslySetInnerHTML for CSS defeats build-time optimizations, makes code harder to maintain, and could introduce XSS vulnerabilities if any dynamic content were added.

2. Memory Leak in ScrollObserver (Medium Priority)

Location: website/src/components/ScrollObserver.tsx:6-26

Issue: The useEffect cleanup doesn't account for elements added after initial mount.

Current Code:

useEffect(() => {
  const scrollElements = document.querySelectorAll(".animate-on-scroll");
  // ...
}, []); // Empty deps - only runs once

Recommendation:

useEffect(() => {
  const scrollElements = document.querySelectorAll(".animate-on-scroll");
  
  const observer = new IntersectionObserver(
    (entries) => {
      entries.forEach((entry) => {
        if (entry.isIntersecting) {
          entry.target.classList.add("is-visible");
          observer.unobserve(entry.target);
        }
      });
    },
    { threshold: 0.1 }
  );

  scrollElements.forEach((el) => observer.observe(el));

  return () => {
    observer.disconnect(); // Use disconnect() instead of individual unobserve
  };
}, []); // This is fine if intentionally run once

🟡 Code Quality Issues

3. Inconsistent Pattern in usePublishableToken Hook

Locations:

  • frontend/src/app/dialogs/connect-vercel-frame.tsx:41-56
  • frontend/src/routes/_context/_cloud/orgs.$organization/projects.$project/ns.$namespace/connect.tsx:438-452

Issue: The refactor improves the code but introduces unnecessary redundancy. Both hooks are called regardless of which branch is taken.

Current Pattern:

const cloudProvider = useCloudNamespaceDataProvider();
const engineProvider = useEngineNamespaceDataProvider();
const cloudData = useSuspenseQuery(cloudProvider.publishableTokenQueryOptions());
const engineData = useSuspenseQuery(engineProvider.engineAdminTokenQueryOptions());

return match(__APP_TYPE__)
  .with("cloud", () => cloudData.data)
  .with("engine", () => engineData.data)
  // ...

Issue: This calls both providers and both queries even though only one branch executes. This could cause issues if one provider throws or if the unused query has side effects.

Recommendation:

function usePublishableToken() {
  return match(__APP_TYPE__)
    .with("cloud", () => {
      const provider = useCloudNamespaceDataProvider();
      return useSuspenseQuery(provider.publishableTokenQueryOptions()).data;
    })
    .with("engine", () => {
      const provider = useEngineNamespaceDataProvider();
      return useSuspenseQuery(provider.engineAdminTokenQueryOptions()).data;
    })
    .otherwise(() => {
      throw new Error("Not in a valid context");
    });
}

Why: This follows React's rules of hooks (always call hooks in the same order) while avoiding unnecessary provider instantiation and queries.

4. Missing Error Handling in Animation

Location: website/src/app/(v2)/(marketing)/(index)/sections/NewHeroSection.tsx:204-265

Issue: The animation state machine uses hardcoded timeouts without considering component unmount.

Recommendation: The current cleanup with return () => clearTimeout(timer) is good, but consider what happens if the sequence array changes length or if delays are undefined.

const timer = setTimeout(() => {
  if (step < sequence.length && sequence[step]) {
    sequence[step]();
    setStep((s) => s + 1);
  }
}, delays[step] ?? 1000); // Use nullish coalescing for safety

📝 Minor Suggestions

5. Type Safety in ActorAnimation

Location: Multiple animation components

Consider extracting types for animation states:

type ActorStatus = "hibernating" | "active" | "processing";
type RequestStatus = "idle" | "flying";

6. Magic Numbers

Location: NewHeroSection.tsx - Animation coordinates

Consider extracting magic numbers to named constants:

const ANIMATION_CONFIG = {
  USER_NODE_X: 60,
  ACTOR_CENTER_X: 376,
  ACTOR_LEFT: 280,
  DATABASE_Y: 310,
  // ...
} as const;

7. Accessibility

The new website sections have aria-hidden="true" on decorative elements, which is good. Consider adding:

  • aria-label to the ActorAnimation for screen readers
  • Focus indicators on interactive elements
  • Reduced motion preferences: @media (prefers-reduced-motion: reduce)

🧪 Testing Recommendations

  1. Frontend: Test the new hooks with both cloud and engine app types
  2. Website:
    • Test animations on different screen sizes
    • Verify scroll observer works with dynamic content
    • Check performance with animation running
  3. Backend: The changes are minimal but test the KV get endpoint with URL-encoded keys

📊 Metrics

  • Files Changed: 39
  • Lines Added: 2,248
  • Lines Removed: 221
  • Net Change: +2,027

The large addition is primarily from new website sections, which is expected for a marketing page redesign.


✔️ Approval Recommendation

Status: Approve with minor changes requested

The critical issues (XSS risk, hook pattern) should be addressed before merge, but they're straightforward fixes. The rest of the PR demonstrates good code quality improvements and follows the project's conventions well.

Required Changes:

  1. ✅ Move inline CSS from dangerouslySetInnerHTML to proper CSS files or Tailwind config
  2. ✅ Fix the hook pattern in usePublishableToken and useNamespace to avoid calling both branches

Nice to Have:

  • Extract animation constants
  • Add reduced-motion media query
  • Improve ScrollObserver cleanup

Great work on the linting cleanup and React Hook dependency fixes! 🎉


Reviewed with: Claude Code
Review Type: Automated + Manual

@NicholasKissel NicholasKissel marked this pull request as ready for review November 15, 2025 20:19
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