Skip to content

Conversation

@NicholasKissel
Copy link
Member

chore(website): flatten actors docs structure

chore: create branch for v3 website changes on v2

chore(site): website changes v3

Add Quickstart tab to docs page and update all Get Started links

  • Added new Quickstart tab next to Overview and Integrations in docs navigation
  • Created new quickstart page at /docs/quickstart/
  • Removed quickstart section from Overview tab sidebar
  • Updated all 'Get Started' buttons on home page to point to /docs/quickstart/
  • Updated Local Development quickstart arrow to point to /docs/quickstart/
  • Added quickstart to mobile navigation dropdown

chre: v4 website changes

chore: v5 website changes

NathanFlurry and others added 6 commits November 14, 2025 18:22
- Added new Quickstart tab next to Overview and Integrations in docs navigation
- Created new quickstart page at /docs/quickstart/
- Removed quickstart section from Overview tab sidebar
- Updated all 'Get Started' buttons on home page to point to /docs/quickstart/
- Updated Local Development quickstart arrow to point to /docs/quickstart/
- Added quickstart to mobile navigation dropdown
@vercel
Copy link

vercel bot commented Nov 19, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivet-inspector Error Error Nov 19, 2025 5:30am
rivet-site Error Error Nov 19, 2025 5:30am
rivetkit-serverless Error Error Nov 19, 2025 5:30am
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 19, 2025 5:30am

Copy link
Member Author

NicholasKissel commented Nov 19, 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.

@claude
Copy link

claude bot commented Nov 19, 2025

PR Review - chore(website): flatten actors docs structure

Summary

This PR primarily focuses on website redesign with marketing page updates, along with some frontend improvements and minor backend changes. The changes span 3,216 additions across frontend, backend (engine), and website components.


✅ Positive Changes

Backend/Engine Changes

  1. Import organization (api-peer/src/actors/kv_get.rs): Good fix moving base64::Engine import before the prelude import - follows Rust convention of alphabetical ordering.

  2. Code formatting (api-public/src/actors/kv_get.rs): The multi-line string formatting for the URL construction improves readability.

Frontend Improvements

  1. React Hook Dependencies: Excellent fixes throughout to address exhaustive dependency warnings in multiple files including connect-manual-serverfull-form.tsx, connect-manual-serverless-form.tsx, inspector-root.tsx, login.tsx, actor-events-list.tsx, and actors-list.tsx

  2. Dead Code Removal (default-data-provider.tsx): Removed unreachable return statements after throw new Error - this is a good cleanup that eliminates dead code.

  3. React Best Practices: Proper children placement inside JSX tags, added biome-ignore for legitimate use of array indices as keys in skeleton loaders, and converted calculateColumnSizes to useCallback for proper memoization.

  4. Conditional Hook Calls Fix: In connect-vercel-frame.tsx and connect.tsx, the hooks are now called unconditionally at the top level, then the results are conditionally used - this follows React Rules of Hooks correctly.

  5. TypeScript improvements (rivetkit/src/utils/node.ts): Changed from @ts-ignore to @ts-expect-error, which is the recommended practice.


⚠️ Issues & Concerns

1. Security: dangerouslySetInnerHTML Usage

Location: website/src/app/(v2)/(blog)/changelog/page.tsx:17-54

The StyleInjector component uses dangerouslySetInnerHTML to inject CSS. While the content appears to be static, this is generally discouraged.

Recommendation: Move this CSS to a proper CSS module or use styled-components/CSS-in-JS

2. Missing Error Handling in useAsyncMemo

Location: frontend/src/components/hooks/use-async-memo.tsx:23

The promise rejection is not handled - if the promise rejects, it will be an unhandled rejection.

Recommendation: Add .catch() to handle promise rejections and log errors appropriately.

3. Potential Memory Leak in useScrollToBottom

Location: frontend/src/components/actors/actor-events.tsx:197-216

If the component unmounts quickly, the requestAnimationFrame callback might still execute after unmount when shouldFollow() returns false.

Recommendation: Always schedule the RAF and cancel it in cleanup, checking shouldFollow() inside the callback instead.

4. Code Style: Inline Styles

Locations: website/src/app/(v2)/(marketing)/(index)/page.tsx:16 and website/src/app/(v2)/(blog)/changelog/page.tsx:146

Using inline styles goes against React best practices and makes theme management harder.

Recommendation: Use CSS modules or Tailwind classes (which the project already uses).

5. Unused Function Parameter Issue

Location: frontend/src/components/actors/worker/actor-worker-context.tsx:29-45

The useInspectorToken function accepts runnerName parameter but in the inspector case, it doesn't use it at all.

Recommendation: Consider restructuring to only call hooks conditionally if they're truly needed, or clarify the intent.

6. Inconsistent Guard Clause

Location: frontend/src/components/actors/guard-connectable-inspector.tsx:243

The dependency array was updated but actorId was removed. Verify this is intentional and won't cause stale closures.


📋 Minor Observations

1. Website Content Changes

The PR includes substantial marketing page redesigns with new sections: NewHeroSection, SocialProofSection, ProblemSection, SolutionSection, NewFeaturesBento, NewUseCases, NewCTASection.

These appear to be purely presentational and don't raise technical concerns, but ensure they've been reviewed by marketing/design teams.

2. Documentation Updates

  • website/public/llms.txt: Removed API reference link
  • website/public/llms-full.txt: Removed API reference section
  • website/src/content/docs/quickstart/index.mdx: New quickstart page added
  • website/src/content/docs/self-hosting/index.mdx: Added clarification about deployment vs self-hosting

These documentation changes look appropriate.

3. Commit Messages

Some commit messages have typos: "chre: v4 website changes" (should be "chore")

Consider squashing/cleaning up commit history before merging.


🧪 Testing Recommendations

  1. Frontend:

    • Test all useEffect hooks with rapid mount/unmount cycles
    • Verify the scroll-to-bottom behavior in actor events
    • Test async memo hook with promises that reject
  2. Backend:

    • Verify the KV get endpoint still works correctly with the URL encoding changes
    • Test with special characters in keys
  3. Website:

    • Verify the changelog page renders correctly
    • Test all new marketing sections across different screen sizes
    • Verify animations don't cause performance issues

📊 Overall Assessment

Code Quality: ✅ Good - The React hook dependency fixes are excellent and show attention to detail.

Best Practices: ⚠️ Mostly Good - Some areas need improvement (error handling, inline styles, dangerouslySetInnerHTML).

Security: ⚠️ Minor Concerns - The dangerouslySetInnerHTML usage should be addressed.

Performance: ✅ Good - The memoization improvements are positive.

Test Coverage: ❓ Unknown - No test files included in this PR.


🎯 Recommendations Before Merge

  1. Must Fix:

    • Add error handling to useAsyncMemo hook
    • Replace dangerouslySetInnerHTML with proper CSS solution
  2. Should Fix:

    • Fix the potential RAF memory leak in useScrollToBottom
    • Replace inline styles with CSS modules or Tailwind
    • Fix typo in commit message
  3. Nice to Have:

    • Add tests for the hook dependency changes
    • Consider adding error boundaries around new website sections
    • Document the new marketing sections

Great work overall! The React hook fixes are particularly well done. Just address the error handling and security concerns, and this will be ready to merge. 🚀

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.

3 participants