Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 20, 2025

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

4 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 20, 2025 11:35pm
rivet-inspector Ignored Ignored Preview Nov 20, 2025 11:35pm
rivet-site Ignored Ignored Preview Nov 20, 2025 11:35pm
rivetkit-serverless Skipped Skipped Nov 20, 2025 11:35pm

Copy link
Member Author

NathanFlurry commented Nov 20, 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.

@NathanFlurry NathanFlurry force-pushed the 11-13-feat_cloudflare-workers_add_createinlineclient_ branch from 2c9312e to 7210249 Compare November 20, 2025 23:35
@NathanFlurry NathanFlurry force-pushed the 11-20-chore_examples_add_counter-next-js_example branch from 7a7aed6 to bd10c4a Compare November 20, 2025 23:35
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless November 20, 2025 23:35 Inactive
@claude
Copy link

claude bot commented Nov 20, 2025

PR Review: Counter Next.js Example

Thanks for adding this Next.js example! This is a clean implementation of the counter demo. Here's my detailed review:

✅ Strengths

  1. Clear Structure: The project follows Next.js 15 App Router conventions well
  2. Type Safety: Good use of TypeScript throughout
  3. Documentation: Comprehensive README with clear setup instructions
  4. Real-time Sync: Nice demonstration of RivetKit's real-time capabilities
  5. User Experience: Clean UI with connection status indicator and disabled states

🐛 Potential Bugs & Issues

High Priority

  1. Missing Error Handling in Counter Component (src/components/Counter.tsx:37)

    const increment = async (amount: number) => {
        if (counter.connection) {
            await counter.connection.increment(amount);
        }
    };

    The async operation has no error handling. If the RPC call fails, the error will be silently swallowed. Recommend:

    const increment = async (amount: number) => {
        if (counter.connection) {
            try {
                await counter.connection.increment(amount);
            } catch (error) {
                console.error('Failed to increment counter:', error);
                // Consider showing user-facing error message
            }
        }
    };
  2. Race Condition in useEffect (src/components/Counter.tsx:22-29)

    useEffect(() => {
        if (counter.connection) {
            setIsConnected(true);
            counter.connection.getCount().then(setCount);
        } else {
            setIsConnected(false);
        }
    }, [counter.connection]);

    Issues:

    • No error handling for getCount() promise
    • If the component unmounts before getCount() resolves, it will call setCount on an unmounted component
    • Missing dependency: ESLint would flag counter.connection usage in the effect

    Recommend:

    useEffect(() => {
        if (counter.connection) {
            setIsConnected(true);
            let cancelled = false;
            
            counter.connection.getCount()
                .then(newCount => {
                    if (!cancelled) setCount(newCount);
                })
                .catch(error => {
                    console.error('Failed to get count:', error);
                });
            
            return () => { cancelled = true; };
        } else {
            setIsConnected(false);
        }
    }, [counter.connection]);

Medium Priority

  1. No Loading State (src/components/Counter.tsx)
    The component doesn't show a loading state when first connecting or when the count is being fetched. Consider adding:

    const [isLoading, setIsLoading] = useState(true);
  2. Event Handler Type Safety (src/components/Counter.tsx:32)

    counter.useEvent("newCount", (newCount: number) => {
        setCount(newCount);
    });

    The event handler doesn't validate that newCount is actually a number. If the backend sends unexpected data, this could cause issues.

🔒 Security Considerations

  1. Environment Variable Exposure (src/components/Counter.tsx:7-9)

    export const { useActor } = createRivetKit<typeof registry>({
        endpoint: process.env.NEXT_PUBLIC_RIVET_ENDPOINT ?? "http://localhost:3000/api/rivet",
        namespace: process.env.NEXT_PUBLIC_RIVET_NAMESPACE,
        token: process.env.NEXT_PUBLIC_RIVET_TOKEN,
    });
    • NEXT_PUBLIC_* variables are exposed to the browser
    • The token being public-facing could be a security concern depending on use case
    • Consider documenting this in the README with security implications
    • For production use, consider server-side authentication instead
  2. No Input Validation (src/components/Counter.tsx:59)
    The counterId input accepts any string without validation. While this may be intentional for flexibility, consider:

    • Adding length limits
    • Sanitizing special characters if needed
    • Documenting valid counter ID format

⚡ Performance Considerations

  1. Hook Called Unconditionally (src/components/Counter.tsx:32-34)

    counter.useEvent("newCount", (newCount: number) => {
        setCount(newCount);
    });

    This hook is called on every render. While React hooks are designed for this, ensure the underlying implementation properly handles re-registration.

  2. Re-creation on Every Render (src/components/Counter.tsx:7)
    The createRivetKit call is at module scope, which is good. However, verify that useActor with changing key: [counterId] properly cleans up old connections.

  3. CSS Not Optimized (src/app/globals.css)

    • 220 lines of global CSS
    • Consider CSS modules for component-scoped styles
    • Some repeated color values could be CSS variables
    • Media query could be optimized

📝 Code Quality & Best Practices

Good Practices

  • ✅ Proper use of TypeScript
  • ✅ Semantic HTML elements
  • ✅ Accessibility considerations (labels, disabled states)
  • ✅ Client component properly marked with "use client"

Suggestions

  1. CSS Variables (src/app/globals.css)
    Extract repeated colors to CSS variables:

    :root {
        --color-bg-primary: #000000;
        --color-bg-secondary: #1c1c1e;
        --color-bg-tertiary: #2c2c2e;
        --color-border: #2c2c2e;
        --color-text-primary: #ffffff;
        --color-text-secondary: #8e8e93;
        --color-blue: #007aff;
        /* ... */
    }
  2. Separate Concerns (src/components/Counter.tsx:7)
    Consider moving the createRivetKit call to a separate file like src/lib/rivet-client.ts for better organization and reusability.

  3. Constant Extraction (src/components/Counter.tsx:13)

    const [counterId, setCounterId] = useState("default");

    Consider extracting "default" as a constant at the module level.

  4. Add PropTypes or Validation (src/rivet/registry.ts:8)

    increment: (c, x: number) => {

    Consider validating that x is a positive number if that's the expected behavior.

🧪 Test Coverage

Critical Issue: This PR adds no tests whatsoever.

Recommended test coverage:

  1. Unit Tests for the counter actor (src/rivet/registry.ts)

    • Test increment with various values
    • Test getCount returns correct state
    • Test broadcast behavior
  2. Component Tests for Counter.tsx

    • Test rendering in connected/disconnected states
    • Test increment button interactions
    • Test counter ID input changes
    • Test event handler updates count
    • Mock the RivetKit hooks
  3. Integration Tests

    • Test the Next.js API route handler
    • Test connection script
  4. E2E Tests (optional for examples)

    • Test multi-client synchronization

📚 Documentation

Good

  • ✅ Comprehensive README
  • ✅ Clear feature list
  • ✅ Setup instructions

Could Improve

  1. Add JSDoc comments to the actor definition
  2. Document environment variables in README
  3. Add troubleshooting section
  4. Document security considerations for production use

🎯 Summary

Approval Status: ⚠️ Approve with Recommendations

This is a solid example that demonstrates RivetKit's capabilities well. The main concerns are:

  1. Missing error handling - Should be addressed before merge
  2. No tests - While this is an example, basic tests would improve quality
  3. Race condition in useEffect - Should be fixed

The security and performance issues are mostly acceptable for an example project but should be documented in the README if this is intended as a starting template for real applications.

Recommended Changes Before Merge

  1. Add error handling to async operations
  2. Fix the race condition in useEffect with cleanup
  3. Add at least basic unit tests for the actor
  4. Document security implications of NEXT_PUBLIC_RIVET_TOKEN

Nice to Have (Can be follow-up PRs)

  1. Add loading states
  2. Refactor CSS to use variables
  3. Add comprehensive test suite
  4. Add error state UI
  5. Consider adding Next.js middleware for server-side token handling example

@claude
Copy link

claude bot commented Nov 20, 2025

Code Review for PR #3506

Thank you for adding the counter-next-js example! This is a helpful addition that demonstrates RivetKit integration with Next.js. Here's my detailed review:

Code Quality & Best Practices

✅ Strengths

  • Clean component structure with proper TypeScript typing
  • Good use of Next.js 15 App Router patterns
  • Proper client/server component separation
  • Well-organized file structure following Next.js conventions
  • Helpful README with clear instructions

⚠️ Issues Found

1. Missing Registry Type Export (examples/counter-next-js/src/rivet/registry.ts:21)
The registry file is missing the type export that's present in the base counter example. This is needed for proper TypeScript inference:

export type Registry = typeof registry;

Compare with examples/counter/src/registry.ts:23 which includes this export.

2. Inconsistent Type Import (examples/counter-next-js/scripts/connect.ts:2)
The connect script imports registry as a type instead of Registry type, which is inconsistent with the base counter example pattern:

// Current (examples/counter-next-js/scripts/connect.ts:2)
import type { registry } from "../src/rivet/registry";

// Should be (following examples/counter/scripts/connect.ts:2 pattern)
import type { Registry } from "../src/rivet/registry";

Then update line 5 to use Registry instead of typeof registry.

3. Missing Package.json Field (examples/counter-next-js/package.json)
The next-js example doesn't have "stableVersion": "0.8.0" but the counter example does. For consistency across examples, this should be added (see examples/counter/package.json:19).

4. Hardcoded Port in Connect Script (examples/counter-next-js/scripts/connect.ts:5)
The connect script hardcodes "http://localhost:3000/api/rivet" but doesn't match the base counter example pattern which uses port 6420. Consider:

  • Using an environment variable with fallback
  • Documenting the expected port in comments
  • Ensuring consistency with the dev server port

Performance Considerations

1. Potential Race Condition (examples/counter-next-js/src/components/Counter.tsx:26)
The getCount() promise is not handled for errors. If the connection drops during the call, this could cause unhandled promise rejections:

useEffect(() => {
    if (counter.connection) {
        setIsConnected(true);
        counter.connection.getCount().then(setCount);  // No error handling
    } else {
        setIsConnected(false);
    }
}, [counter.connection]);

Consider adding .catch() or wrapping in try-catch.

2. Missing Cleanup (examples/counter-next-js/src/components/Counter.tsx:23-30)
The useEffect doesn't return a cleanup function. While this might be handled by the RivetKit library, it's worth documenting or verifying.

Security Concerns

1. Environment Variables (examples/counter-next-js/src/components/Counter.tsx:8-10)
Good use of environment variables for configuration. The fallback to localhost is appropriate for development. Consider documenting these variables in the README or a .env.example file.

2. User Input Validation (examples/counter-next-js/src/components/Counter.tsx:60)
The counterId input is not validated before being used as an actor key. While this is probably fine for an example, consider:

  • Documenting any naming constraints
  • Adding basic validation (e.g., non-empty, valid characters)

Test Coverage

Missing Tests
The base counter example has tests at examples/counter/tests/counter.test.ts, but this example doesn't include any tests. While not critical for an example, consider:

  • Adding basic integration tests
  • Or documenting that this example is focused on the Next.js integration rather than testing

Style & Conventions

1. CSS Approach
The example uses a large globals.css file (220 lines). While this works, consider:

  • Documenting why this approach was chosen over CSS Modules or Tailwind (which Next.js commonly uses)
  • Or noting this is intentionally minimal for example clarity

2. Font Loading (examples/counter-next-js/src/app/layout.tsx:4-12)
The example loads Geist fonts but doesn't appear to use the font variables in the CSS. The variables are added to the body but not referenced in globals.css.

Minor Observations

  1. The .gitignore includes Yarn-specific patterns (lines 6-11) but the project uses pnpm (per CLAUDE.md). This is from the Next.js template and doesn't hurt, but could be streamlined.

  2. The README mentions "basic counter example" but there's no link to it for easy reference.

  3. The maxDuration = 300 in the API route (examples/counter-next-js/src/app/api/rivet/[...all]/route.ts:4) could use a comment explaining why 5 minutes is needed.

Summary

This is a solid addition to the examples! The main issues are:

  1. Critical: Missing Registry type export causing type inconsistency
  2. Important: Inconsistent typing patterns vs base counter example
  3. Nice-to-have: Missing error handling, tests, and documentation improvements

Overall: ✅ Approve with minor changes recommended

The example is functional and demonstrates the integration well. The suggested fixes are mostly about consistency with existing patterns and defensive programming practices.

@claude
Copy link

claude bot commented Nov 20, 2025

Pull Request Review

Overview

This PR adds a new Next.js counter example to demonstrate RivetKit integration with Next.js 15. The implementation is clean and follows established patterns from the existing next-js example.

Positive Aspects ✅

  1. Consistent Structure: Follows the same structure as examples/next-js, using Next.js 15.4.5 and React 19.1.0
  2. Good Documentation: Clear README with setup instructions and feature descriptions
  3. Clean Implementation: The counter actor and registry are well-structured and straightforward
  4. Real-time Sync: Properly demonstrates real-time state synchronization across clients
  5. TypeScript: Fully typed with proper type imports
  6. User Experience: Nice UI with connection status indicator and multiple increment options

Issues & Recommendations 🔍

1. Potential Race Condition in Counter.tsx (Medium Priority)

Location: src/components/Counter.tsx:23-30

The useEffect that fetches the initial count could have a race condition with no error handling and potential state updates after unmount.

Recommendation: Add cleanup function and error handling to prevent memory leaks and unhandled promise rejections.

2. Missing Error Handling in increment (Medium Priority)

Location: src/components/Counter.tsx:36-40

The increment function has no try-catch to handle network errors gracefully. Users would not receive feedback if the operation fails.

3. Missing Error Handling in connect.ts (Low Priority)

Location: scripts/connect.ts:4-19

The script has no error handling for connection failures or RPC errors.

Security Considerations 🔒

✅ No security issues identified:

  • Environment variables are properly prefixed with NEXT_PUBLIC_
  • No unsafe DOM manipulation
  • No hardcoded secrets or credentials

Performance Considerations ⚡

The increment function is recreated on every render. Consider wrapping it in useCallback for optimization (very low priority for this example).

Test Coverage 📝

⚠️ No tests included. While this is an example project, consider adding basic tests similar to the examples/counter which has vitest configured.

Summary

This is a solid example that demonstrates RivetKit integration with Next.js effectively. The main improvements needed are:

  1. Add error handling for async operations (medium priority)
  2. Fix potential race condition in the useEffect (medium priority)
  3. Consider adding tests (low priority)

Overall Assessment: ✅ Approve with suggestions - The code is good quality and ready to merge, with recommended improvements for better error handling that could be addressed in a follow-up PR or before merging based on team standards.


Review generated by Claude Code

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 21, 2025

Merge activity

  • Nov 21, 2:40 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 21, 2:42 AM UTC: CI is running for this pull request on a draft pull request (#3515) due to your merge queue CI optimization settings.
  • Nov 21, 2:43 AM UTC: Merged by the Graphite merge queue via draft PR: #3515.

@graphite-app graphite-app bot closed this Nov 21, 2025
@graphite-app graphite-app bot deleted the 11-20-chore_examples_add_counter-next-js_example branch November 21, 2025 02:43
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