Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 24, 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 24, 2025 8:08am
rivet-inspector Ignored Ignored Preview Nov 24, 2025 8:08am
rivet-site Ignored Ignored Preview Nov 24, 2025 8:08am
rivetkit-serverless Skipped Skipped Nov 24, 2025 8:08am

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless November 24, 2025 08:08 Inactive
@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 24, 2025

Merge activity

  • Nov 24, 8:08 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 24, 8:08 AM UTC: CI is running for this pull request on a draft pull request (#3535) due to your merge queue CI optimization settings.
  • Nov 24, 8:09 AM UTC: Merged by the Graphite merge queue via draft PR: #3535.

Copy link
Member Author


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 mentioned this pull request Nov 24, 2025
graphite-app bot pushed a commit that referenced this pull request Nov 24, 2025
@claude
Copy link

claude bot commented Nov 24, 2025

PR Review: chore(examples): fix types

Summary

This PR makes several type fixes and API updates across the examples, adds two new example projects (cloudflare-workers-inline-client and counter-next-js), and removes the transport selection feature from the kitchen-sink example.

Code Quality & Best Practices ✅

Positive aspects:

  • Consistent renaming of onFetch to onRequest across examples aligns with apparent API changes
  • Proper null check added in cursors-raw-websocket/src/backend/registry.ts:60-63 for c.request before accessing its properties
  • Clean migration from new_classes to new_sqlite_classes in wrangler.json configurations
  • State simplification in cloudflare-workers/src/registry.ts removing unused connectionCount and messageCount fields

New examples are well-structured:

  • cloudflare-workers-inline-client demonstrates inline client usage pattern cleanly
  • counter-next-js provides a complete Next.js integration example with proper separation of concerns

Potential Issues ⚠️

  1. cursors-raw-websocket/src/backend/registry.ts - The fix references an undefined request variable on line 57 (before the fix). The fix correctly uses c.request, but ensure this change is tested since it's modifying error handling behavior.

  2. kitchen-sink/src/backend/actors/websocket.ts:15 - Changed from opts.request.url to c.request.url without null checking:

    url: c.request.url,

    Unlike the fix in cursors-raw-websocket, this doesn't verify c.request exists before accessing .url. Consider adding a guard if c.request can be undefined in the websocket handler context.

  3. counter-next-js/src/components/Counter.tsx:23-28 - The useEffect dependency array may cause issues:

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

    The promise .then(setCount) doesn't handle errors. Consider adding .catch() for robustness.

  4. counter-next-js/scripts/connect.ts:12 - Infinite loop without cleanup:

    while (true) {
      const out = await counter.increment(1);
      // ...
    }

    This script has no graceful exit mechanism. Consider adding signal handling for cleaner termination.

Security Considerations ✅

  • No obvious security issues identified
  • Environment variables are properly used for configuration endpoints
  • No hardcoded secrets or credentials

Performance Considerations ✅

  • The removal of the transport option simplifies the client configuration
  • No performance concerns with the changes

Test Coverage 📝

This PR primarily modifies example code, which typically doesn't have direct unit tests. However, consider:

  • Running pnpm run check-types on all modified examples to verify type correctness
  • Manual testing of the new examples to ensure they work as documented

Minor Suggestions

  1. The cloudflare-workers-inline-client/src/index.ts:35 has a debug log that might want to be removed for production:

    console.log("URL", url.toString());
  2. In counter-next-js/src/app/globals.css, there's a lot of CSS - consider whether this could be simplified or if a CSS framework is warranted for a demo.

Overall Assessment

The PR correctly updates examples to match API changes (onFetchonRequest, request access pattern changes) and adds valuable new examples. The main concern is the inconsistent null-checking for c.request between the different examples - the fix in cursors-raw-websocket is good but the same pattern should probably be applied in kitchen-sink/websocket.ts.

Recommendation: Address the null-check inconsistency and the unhandled promise rejection, otherwise LGTM 👍

@graphite-app graphite-app bot closed this Nov 24, 2025
@graphite-app graphite-app bot deleted the 11-22-chore_examples_fix_types branch November 24, 2025 08:09
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