Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 22, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-next-js-max Error Error Oct 22, 2025 4:24am
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Skipped Skipped Oct 22, 2025 4:24am

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless October 22, 2025 04:24 Inactive
Copy link
Member Author

NathanFlurry commented Oct 22, 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 either label to this PR to merge it via the merge queue:

  • merge queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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 22, 2025

PR Review: Cursors Example

Thank you for this well-crafted example! This is a solid demonstration of RivetKit's real-time capabilities. Below is my detailed review:

✅ Strengths

Code Quality

  • Clean architecture: Clear separation between backend (actor logic) and frontend (React UI)
  • Well-documented: Good inline comments explaining key RivetKit concepts
  • Type safety: Proper TypeScript types throughout
  • Comprehensive tests: 5 test cases covering all major functionality

Documentation

  • Excellent README: Follows the project's README template perfectly
  • Clear feature list: Helps users understand what the example demonstrates
  • Good instructions: Easy-to-follow setup and development steps

Implementation

  • Event-driven design: Proper use of c.broadcast() for real-time updates
  • State management: Persistent actor state for cursors and text labels
  • User experience: Nice touch with color-coded cursors and typing indicators

🔍 Issues Found

1. Missing .gitignore Entry ⚠️

File: .gitignore:2

The .gitignore file is missing a newline at the end, which violates standard Git conventions.

 .actorcore
-node_modules
\ No newline at end of file
+node_modules

2. Inconsistent Package Name ⚠️

File: package.json:2

According to CLAUDE.md guidelines, all examples should be named example-{name}. The chat-room example doesn't follow this (it's just chat-room), but your example correctly uses example-cursors. Good consistency!

However, comparing with other examples, React dependencies should be in dependencies, not devDependencies:

Current:

"devDependencies": {
  "@rivetkit/react": "workspace:*",
  "react": "^18.2.0",
  "react-dom": "^18.2.0"
},
"dependencies": {
  "rivetkit": "workspace:*"
}

Should be (matching examples/react/package.json):

"devDependencies": {
  "rivetkit": "workspace:*",
  // ... other dev dependencies
},
"dependencies": {
  "@rivetkit/react": "workspace:*",
  "react": "^18.2.0",
  "react-dom": "^18.2.0"
}

3. Potential ID Collision 🐛

File: src/backend/registry.ts:39

The text label ID generation could have collisions if the same user places multiple texts in the same millisecond:

id: `${userId}-${Date.now()}`,

While unlikely in practice, consider using a more robust approach:

id: `${userId}-${Date.now()}-${Math.random().toString(36).substring(2, 9)}`,

Or use a counter:

id: `${userId}-${c.state.textLabels.length}-${Date.now()}`,

4. Missing Cursor Cleanup Logic 🐛

File: src/frontend/App.tsx:157-163

The cleanup effect depends on cursorRoom.connection and userId, but there's a potential race condition. If the component unmounts while the connection is still being established, the cursor won't be removed. Consider:

useEffect(() => {
  const currentUserId = userId;
  const currentConnection = cursorRoom.connection;
  
  return () => {
    if (currentConnection) {
      currentConnection.removeCursor(currentUserId);
    }
  };
  // Intentionally using refs to avoid dependency array issues
}, []); // Run only on mount/unmount

Or alternatively, remove the dependencies from the dependency array to only run on mount/unmount.

5. Performance Consideration 💡

File: src/frontend/App.tsx:119-124

The handleMouseMove function sends a cursor update on every mouse movement. For a production app, you'd want to throttle these updates:

// At the top of the component
const throttledUpdateCursor = useCallback(
  throttle((userId: string, x: number, y: number) => {
    if (cursorRoom.connection) {
      cursorRoom.connection.updateCursor(userId, x, y);
    }
  }, 50), // Update at most every 50ms
  [cursorRoom.connection]
);

const handleMouseMove = (e: React.MouseEvent<HTMLDivElement>) => {
  if (cursorRoom.connection && canvasRef.current) {
    const { x, y } = screenToCanvas(e.clientX, e.clientY);
    throttledUpdateCursor(userId, x, y);
  }
};

Since this is an example, it's not critical, but worth mentioning for learners.

6. Test Coverage Gap 📝

File: tests/cursors.test.ts

The tests don't verify that broadcast events are actually sent. Consider adding tests that:

  • Listen for cursorMoved events
  • Listen for textPlaced events
  • Listen for cursorRemoved events

Example:

test("Cursor updates broadcast events", async (ctx) => {
  const { client } = await setupTest(ctx, registry);
  const room = client.cursorRoom.getOrCreate(["test-events"]);
  
  const events: CursorPosition[] = [];
  room.on("cursorMoved", (cursor) => {
    events.push(cursor);
  });
  
  await room.updateCursor("user1", 100, 200);
  
  expect(events).toHaveLength(1);
  expect(events[0]).toMatchObject({
    userId: "user1",
    x: 100,
    y: 200,
  });
});

7. Unbounded State Growth ⚠️

File: src/backend/registry.ts:23

The textLabels array grows indefinitely. In a long-running room, this could become a memory issue. Consider:

  • Adding a maximum label count
  • Implementing label cleanup after a certain age
  • Adding a "clear all" action

Example:

placeText: (c, userId: string, text: string, x: number, y: number) => {
  const textLabel: TextLabel = {
    id: `${userId}-${Date.now()}`,
    userId,
    text,
    x,
    y,
    timestamp: Date.now(),
  };
  c.state.textLabels.push(textLabel);
  
  // Keep only the last 100 labels
  if (c.state.textLabels.length > 100) {
    c.state.textLabels.shift();
  }
  
  c.broadcast("textPlaced", textLabel);
  return textLabel;
},

🎯 Summary

Required Changes:

  1. Add newline to .gitignore
  2. Move React dependencies to dependencies section in package.json

Recommended Changes:
3. Fix potential ID collision in text label generation
4. Fix cursor cleanup race condition
5. Address unbounded state growth in textLabels

Nice to Have:
6. Add throttling to cursor updates (with a comment explaining it's omitted for simplicity)
7. Add event broadcasting tests

Security & Best Practices

✅ No security concerns identified
✅ No malicious code patterns
✅ Follows RivetKit conventions from CLAUDE.md
✅ Proper error handling patterns
✅ Good use of TypeScript types

Final Verdict

This is a high-quality example that effectively demonstrates RivetKit's real-time capabilities. The issues found are mostly minor, with the most important being the package.json structure and the .gitignore newline. Once those two required changes are addressed, this will be an excellent addition to the examples collection.

Great work! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants