Skip to content

chore: websocket fixes for cursor example#3973

Closed
NicholasKissel wants to merge 1 commit intomainfrom
01-18-chore_websocket_fixes_for_cursor_example
Closed

chore: websocket fixes for cursor example#3973
NicholasKissel wants to merge 1 commit intomainfrom
01-18-chore_websocket_fixes_for_cursor_example

Conversation

@NicholasKissel
Copy link
Member

No description provided.

@railway-app
Copy link

railway-app bot commented Jan 19, 2026

🚅 Deployed to the rivet-pr-3973 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-inspector 😴 Sleeping (View Logs) Web Jan 19, 2026 at 5:43 am
frontend-cloud 😴 Sleeping (View Logs) Web Jan 19, 2026 at 5:41 am
website 😴 Sleeping (View Logs) Web Jan 19, 2026 at 5:39 am

@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3973 January 19, 2026 01:42 Destroyed
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.

@claude
Copy link

claude bot commented Jan 19, 2026

PR Review: WebSocket Fixes for Cursor Example

Summary

This PR makes two key changes to fix WebSocket connections in the cursors-raw-websocket example:

  1. Separates HTTP and WebSocket URLs to work around vite-plugin-srvx not proxying WebSocket connections
  2. Defaults to "json" encoding when no subprotocol is provided for raw WebSocket connections

Code Quality ✅

Positive aspects:

  • Clear, explanatory comments added in both files
  • Minimal, focused changes that solve the specific problem
  • Follows existing code patterns and conventions

Changes Analysis

1. Frontend URL Separation (App.tsx)

The change splits the HTTP and WebSocket URLs due to vite-plugin-srvx limitations.

Concerns:

  • ⚠️ Hardcoded localhost:6420: This will break in production or when RivetKit runs on a different port
  • ⚠️ No protocol detection: Hardcodes ws:// instead of using wss:// for secure connections
  • This is a workaround rather than a proper solution

Suggestion: Make the WebSocket URL configurable via environment variables or derive it from the HTTP URL with appropriate protocol switching.

2. Default Encoding (router-websocket-endpoints.ts:388)

Defaults to "json" encoding for raw WebSocket connections without subprotocols.

Analysis:

  • ✅ Makes sense for raw WebSocket connections which typically use JSON
  • ✅ Maintains backward compatibility
  • ✅ The comment clearly explains the intent
  • ⚠️ Potential breaking change: This changes behavior for raw WebSocket connections that previously would have failed. However, this is likely the desired behavior.

Security Considerations 🔒

  • No CORS or origin validation concerns introduced
  • EncodingSchema.parse() still validates the encoding value
  • Hardcoded localhost URL is not a security issue but could cause environment confusion

Performance Considerations ⚡

  • No performance impact from these changes
  • The default value (?? operator) has negligible overhead

Test Coverage 🧪

Missing tests:

  • No test coverage added for the new default encoding behavior
  • This example lacks automated tests for WebSocket connection flow

Recommendation: Add test cases to verify raw WebSocket connections work without explicit encoding subprotocol.

Potential Bugs 🐛

  1. Environment-specific failure: The hardcoded ws://localhost:6420 will fail when:

    • Running in production
    • RivetKit is bound to a different port
    • Running in Docker containers with different networking
    • Multiple developers have port conflicts
  2. Protocol mismatch: Using ws:// when the app might be served over HTTPS will cause mixed content warnings in browsers

Recommendations 📝

High Priority

  1. Make WebSocket URL configurable: Use environment variables or derive from HTTP URL with protocol switching
  2. Add tests: Include test coverage for the default encoding behavior

Medium Priority

  1. Consider fixing vite-plugin-srvx: Document this limitation or use different dev server configuration
  2. Improve error messages: Add connection error handling with helpful messages

Low Priority

  1. Protocol detection: Use wss:// when served over HTTPS

Alignment with CLAUDE.md ✅

  • ✅ Comments follow the guideline to write as complete sentences
  • ✅ Code changes are minimal and focused
  • ✅ Follows existing TypeScript patterns in the codebase

Conclusion

The changes solve the immediate problem but introduce environment-specific hardcoding that will cause issues outside of local development. The default encoding change is sound and well-justified.

Recommendation: Request changes to make the WebSocket URL configurable before merging.


Review generated by Claude Code

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 19, 2026

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3973

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3973

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3973

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3973

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3973

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3973

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3973

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3973

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3973

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3973

commit: 80fe3e8

@NicholasKissel NicholasKissel marked this pull request as ready for review January 19, 2026 01:51
@NicholasKissel NicholasKissel force-pushed the 01-18-chore_websocket_fixes_for_cursor_example branch from 502ec0c to 80fe3e8 Compare January 19, 2026 05:31
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3973 January 19, 2026 05:31 Destroyed
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 19, 2026

Merge activity

  • Jan 19, 5:04 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Jan 19, 5:04 PM UTC: CI is running for this pull request on a draft pull request (#3974) due to your merge queue CI optimization settings.
  • Jan 19, 5:05 PM UTC: Merged by the Graphite merge queue via draft PR: #3974.

graphite-app bot pushed a commit that referenced this pull request Jan 19, 2026
@graphite-app graphite-app bot closed this Jan 19, 2026
@graphite-app graphite-app bot deleted the 01-18-chore_websocket_fixes_for_cursor_example branch January 19, 2026 17:05
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