Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 28, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivet-site Error Error Nov 1, 2025 0:55am
rivetkit-serverless Error Error Nov 1, 2025 0:55am
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 1, 2025 0:55am
rivet-inspector Ignored Ignored Preview Nov 1, 2025 0:55am

Copy link
Member Author

NathanFlurry commented Oct 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.

@claude
Copy link

claude bot commented Oct 28, 2025

Pull Request Review

Summary

This PR refactors the website marketing homepage and documentation structure by flattening the actors docs structure and enhancing the homepage with new sections showcasing use cases and deployment options.

Code Quality & Best Practices

Strengths

  • Good component separation with dedicated UseCases and DeploymentOptionsSection components
  • Proper TypeScript typing with interfaces
  • Good responsive design with Tailwind classes
  • Proper semantic HTML and accessibility

Issues Found

1. Typo in self-hosting/index.mdx line 6
applciations should be applications

2. Broken link in DeploymentOptionsSection.tsx line 69
/docs/general/self-hosting should be /docs/self-hosting based on sitemap

3. Grammar in HeroSection.tsx line 26
in to should be into (one word)

4. Commented code in HeroSection.tsx
Multiple blocks of commented hero copy (lines 30-86) should be removed

Potential Bugs

1. UseCases.tsx lines 135-142
Non-null assertions assume use cases exist. If missing, will throw runtime error. Add null checks.

2. UseCases.tsx handleMouseMove
Runs on every mousemove without throttling. Could cause performance issues with multiple cards.

Test Coverage

No tests included for new components. Consider adding unit tests and link validation tests.

Recommendations

High Priority:

  1. Fix typo: applciations to applications
  2. Fix broken link: /docs/general/self-hosting to /docs/self-hosting
  3. Fix grammar: in to to into
  4. Add null checks for use case lookups

Medium Priority:

  1. Remove commented-out code
  2. Add performance optimization for mouse tracking
  3. Add tests

Overall Assessment

Solid refactoring with good code quality. Main issues are minor but should be fixed before merging.

Recommendation: Request changes for typo, broken link, and grammar fixes.

@NicholasKissel NicholasKissel changed the base branch from 10-25-chore_rivetkit-typescript_remove_dependency_on_node_modules to graphite-base/3280 November 1, 2025 00:50
@NicholasKissel NicholasKissel changed the base branch from graphite-base/3280 to main November 1, 2025 00:50
@claude
Copy link

claude bot commented Nov 1, 2025

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

I've reviewed this PR and found that while the PR title mentions "flatten actors docs structure", the changes are primarily focused on refactoring Node.js module imports in the rivetkit-typescript package to support cross-platform compatibility (likely for edge/browser environments). Here's my detailed feedback:


🔴 Critical Issues

1. Security Vulnerability: Timing-Safe Comparison Removed

Location: rivetkit-typescript/packages/rivetkit/src/inspector/utils.ts:23-28

The compareSecrets function has had its timing-safe comparison commented out:

// TODO:
// // Perform timing-safe comparison
// if (!crypto.timingSafeEqual(a, b)) {
// 	return false;
// }
return true;  // This always returns true now!

Impact: This is a critical security vulnerability. The function now always returns true regardless of whether the secrets match, effectively disabling authentication for the inspector endpoint.

Recommendation:

  • Either implement a timing-safe comparison without Node.js crypto (using a constant-time comparison loop)
  • Or dynamically import and use crypto.timingSafeEqual when available
  • Do not merge this PR until this is fixed

Example fix:

export function compareSecrets(providedSecret: string, validSecret: string) {
    if (providedSecret.length !== validSecret.length) {
        return false;
    }
    
    const encoder = new TextEncoder();
    const a = encoder.encode(providedSecret);
    const b = encoder.encode(validSecret);
    
    // Constant-time comparison
    let result = 0;
    for (let i = 0; i < a.length; i++) {
        result |= a[i] ^ b[i];
    }
    return result === 0;
}

⚠️ High Priority Issues

2. Error Handling in Dynamic Imports

Location: rivetkit-typescript/packages/rivetkit/src/utils/node.ts:51-58

The error handling logs a warning but doesn't provide guidance on when this is expected vs. problematic:

} catch (err) {
    console.warn(
        "Node.js modules not available, file system driver will not work",
        err,
    );
    throw err;
}

Issues:

  • Uses console.warn then immediately throws - this is confusing
  • Should use the structured logging pattern from the codebase (tracing)
  • Per CLAUDE.md: "Use tracing for logging. Do not format parameters into the main message"

Recommendation:

} catch (err) {
    // Don't warn if in browser/edge environment where this is expected
    throw new Error(
        "Node.js modules not available. File system driver requires Node.js runtime."
    );
}

3. Async Function Not Awaited

Location: rivetkit-typescript/packages/rivetkit/src/registry/mod.ts:209-214

The server start is added to readyPromises but wrapped in an IIFE that may not be awaited correctly:

if (!config.disableDefaultServer) {
    const serverPromise = (async () => {
        const out = await crossPlatformServe(config, hono, undefined);
        upgradeWebSocket = out.upgradeWebSocket;
    })();
    readyPromises.push(serverPromise);
}

Issue: The upgradeWebSocket assignment happens asynchronously but is used synchronously later. This is a potential race condition.

Recommendation: Ensure this promise is properly awaited before the registry is considered "ready".


📝 Code Quality Issues

4. Missing Type Safety

Location: rivetkit-typescript/packages/rivetkit/src/registry/serve.ts:13-26

Dynamic imports use any type:

let serve: any;

Recommendation: Add proper type assertions or import types separately.

5. Inconsistent Error Messages

Location: Multiple getNode* functions in src/utils/node.ts

All getter functions have identical error messages. They should specify which module failed:

export function getNodeCrypto(): typeof import("node:crypto") {
    if (!nodeCrypto) {
        throw new Error(
            "Node crypto module not loaded. Ensure importNodeDependencies() has been called.",
        );
    }
    return nodeCrypto;
}

This is good, but could be even better with context about where the call originated.

6. Commented Out Code

Location: rivetkit-typescript/packages/rivetkit/src/inspector/utils.ts:1

// import crypto from "node:crypto";

Recommendation: Remove commented-out imports rather than leaving them in.

7. Unexplained Comment

Location: rivetkit-typescript/packages/rivetkit/src/engine-process/mod.ts:254

}
//
function resolveTargetTriplet(): { targetTriplet: string; extension: string } {

Random comment should be removed.


✅ Positive Changes

  1. Good abstraction pattern - The Node.js module abstraction in utils/node.ts is well-structured with:

    • Singleton pattern for imports
    • Clear error messages
    • Proper TypeScript typing for return values
    • Availability checking function
  2. Webpack ignore comments - Properly using /* webpackIgnore: true */ to prevent bundling of Node.js modules

  3. Proper async/await propagation - Most functions correctly changed to async (e.g., createFileSystemDriver, chooseDefaultDriver)

  4. Test updates - Tests properly updated to await the now-async driver creation functions


🤔 Questions

  1. PR Title Mismatch: The PR is titled "flatten actors docs structure" but the majority of changes are about Node.js module abstraction. Are these unrelated changes bundled together, or is the title incorrect?

  2. Website Changes: There are extensive website changes (new SVG icons, component updates) that seem unrelated to the rivetkit changes. Should these be in a separate PR?

  3. Biome Configuration: The biome.json changes add extensive import restrictions for Node.js modules. Is this related to the same refactoring effort?


📊 Summary

Do Not Merge until the critical security issue (#1) is resolved.

Stats:

  • 42 files changed
  • 984 additions, 348 deletions
  • Mix of infrastructure, security, and website changes

Priority Fixes:

  1. 🔴 Fix compareSecrets security vulnerability (CRITICAL)
  2. ⚠️ Fix error handling in importNodeDependencies
  3. ⚠️ Verify upgradeWebSocket race condition
  4. 📝 Clean up commented code and improve type safety

Let me know if you'd like me to elaborate on any of these points or if you need help implementing the fixes!

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