feat(examples): cursors with raw websockets example #3245
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
How to use the Graphite Merge QueueAdd 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. |
PR Review: Cursors with Raw WebSockets ExampleThank you for this example! This is a great demonstration of real-time collaborative features using RivetKit. Here's my detailed review: ✅ Strengths
🐛 Potential Bugs & IssuesCritical Issues
Moderate Issues
Minor Issues
🔒 Security Concerns
⚡ Performance Considerations
🧪 Test CoverageExisting Tests
Missing Test Coverage
📝 Code Quality & Best PracticesGood Practices
Improvements Needed
📋 Summary & RecommendationsPriority Fixes
Nice to Have
Overall AssessmentThis is a well-structured example that demonstrates RivetKit's capabilities effectively. The code is clean and readable. The main issues are around production-readiness (rate limiting, validation, error handling) which are expected for an example. The cursor keying bug should be fixed as it affects functionality. Recommendation: ✅ Approve with minor changes requested (fix cursor keying bug) Great work on this example! 🚀 |
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
NathanFlurry
force-pushed
the
10-22-feat_examples_cursors_with_raw_websockets_example
branch
from
86f2ec4 to
aa8205e
Compare
NathanFlurry
force-pushed
the
10-22-feat_examples-cursors_cursors_example
branch
from
8766cdd to
eb78d9e
Compare
Code Review: Cursors with Raw WebSockets ExampleGreat work on this example! This is a clean demonstration of real-time collaborative features with RivetKit. Below is my detailed feedback: ✅ Strengths
🐛 Bugs and IssuesCritical: Cursor Key Mismatch (registry.ts:48, App.tsx:127)The backend uses Backend: cursors[sessionId] = cursor; // Line 48Frontend: [message.data.userId]: message.data // Line 127This causes cursors to be keyed inconsistently. The backend should use Recommendation: Use Bug: Cursor Removal Uses Wrong Key (registry.ts:189, 193)When removing a cursor on disconnect, the backend sends if (id !== sessionId) { // Line 188This should be consistent with how cursors are keyed. Bug: Cursor Identified Incorrectly (App.tsx:374)const isOwnCursor = id === userId;The This logic seems correct if cursors are properly keyed by
|
Code Review: Cursors with Raw WebSockets ExampleThank you for this example! It demonstrates real-time collaborative cursor tracking with RivetKit. Here's my detailed review: 🎯 Overall AssessmentThis is a well-structured example that effectively demonstrates WebSocket usage with actors. The code is clean and demonstrates good practices for real-time collaborative features. 🐛 Potential Bugs & Issues1. Backend: Missing state persistence call (registry.ts:141-144)When updating or adding text labels, the state is modified but Issue: Text labels may not persist across actor restarts. Fix: Add if (existingIndex >= 0) {
c.state.textLabels[existingIndex] = textLabel;
} else {
c.state.textLabels.push(textLabel);
}
c.saveState({}); // Add this lineAlso needed after line 162 for 2. Backend: Potential memory leak with dead WebSocket connections (registry.ts:114-121)The broadcast loop doesn't check socket Recommendation: Check socket state before sending: for (const { socket } of c.vars.websockets.values()) {
if (socket.readyState === 1) { // 1 = OPEN
socket.send(JSON.stringify({
type: "cursorMoved",
data: cursor,
}));
}
}3. Frontend: Cursor key mismatch (App.tsx:125-128, 372-413)In line 128, cursors are keyed by Looking at the backend (registry.ts:46-49), cursors are keyed by Fix: Key cursors by a unique identifier that includes the session: case "cursorMoved": {
// Backend should send sessionId with the cursor data
setCursors((prev) => ({
...prev,
[message.data.sessionId]: message.data,
}));
break;
}The backend needs to include 4. Frontend: Missing error boundary (App.tsx)React component has no error boundary for WebSocket or parsing errors. Errors in message handlers could crash the component. 5. Frontend: Race condition on room change (App.tsx:81-191)When Recommendation: Add connection ID tracking or ignore messages from stale connections. 🔒 Security Concerns1. Input validation missing
Recommendations: case "updateText": {
const { id, userId, text, x, y } = message.data;
// Validate inputs
if (!id || !userId || typeof text !== 'string' ||
typeof x !== 'number' || typeof y !== 'number') {
return; // or send error
}
// Bounds checking
if (x < 0 || x > 1920 || y < 0 || y > 1080) {
return;
}
// Length limits
if (text.length > 500) {
return;
}
// ...rest of the logic
}2. No rate limitingA malicious client could flood the server with cursor updates or text changes, causing:
Recommendation: Implement rate limiting per session or use RivetKit's built-in throttling if available. 3. XSS vulnerability in frontend (App.tsx:337-338)Text labels are rendered directly without sanitization. While React escapes by default, the text content should still be explicitly validated. 4. No authenticationAnyone can connect and modify room state. For a demo this is acceptable, but production use would need proper auth. 🚀 Performance Considerations1. Excessive broadcasts (registry.ts:113-121)Every cursor movement broadcasts to all clients including the sender. For a room with N users, each movement generates N messages. Optimization: Skip sending back to the originating session: for (const [id, { socket }] of c.vars.websockets.entries()) {
if (id !== sessionId && socket.readyState === 1) {
socket.send(JSON.stringify({
type: "cursorMoved",
data: cursor,
}));
}
}2. Mouse movement spam (App.tsx:205-215)Every Recommendation: Throttle cursor updates: const throttledSendCursor = useCallback(
throttle((x: number, y: number) => {
if (wsRef.current?.readyState === WebSocket.OPEN) {
wsRef.current.send(JSON.stringify({
type: "updateCursor",
data: { userId, x, y },
}));
}
}, 50), // Send at most every 50ms
[userId]
);3. Re-rendering all cursors on every update (App.tsx:372-413)Each cursor movement causes all cursor components to re-render. Optimization: Memoize cursor components: const CursorComponent = React.memo(({ cursor, isOwnCursor }) => {
// ...cursor rendering logic
});4. No cleanup of stale cursorsCursors remain in state forever. If a client disconnects ungracefully, their cursor stays. Recommendation: Add timestamp-based cleanup or rely on WebSocket close events (which you already handle, but could fail). 5. Text labels grow unbounded (registry.ts:28)No limit on number of text labels. Memory will grow indefinitely. Recommendation: Add a limit like the chat example does (lines 44-47 in raw-websocket-handler-proxy example). ✅ Code Quality & Best PracticesStrengths:
Areas for Improvement:1. Inconsistent error handling
2. Magic numbers
3. Mixed concerns in frontendThe
Recommendation: Extract WebSocket logic into a custom hook: function useRoomConnection(roomId: string, sessionId: string) {
// WebSocket connection logic
return { cursors, textLabels, sendCursor, sendText, connected };
}4. Unused timestamp fields
5. Missing JSDoc commentsPublic interfaces and actions should have documentation explaining their purpose and parameters. 🧪 Test CoverageCurrent Coverage:
Missing Tests:
Recommendation: Add WebSocket integration tests: test("broadcasts cursor updates to all connected clients", async (ctx: any) => {
const { client } = await setupTest(ctx, registry);
const room = client.cursorRoom.getOrCreate(["test-broadcast"]);
// Create multiple WebSocket connections
// Send cursor update from one
// Verify all others receive it
});📝 DocumentationREADME Issues:
Code Comments:
🎨 Minor Issues
✨ Recommendations SummaryCritical (Fix before merge):
High Priority:
Nice to Have:
🏁 ConclusionThis is a solid example that demonstrates RivetKit's WebSocket capabilities well. The main concerns are:
With the critical fixes applied, this would be an excellent reference example. Great work overall! 🚀 |
NathanFlurry
force-pushed
the
10-22-feat_examples_cursors_with_raw_websockets_example
branch
from
aa8205e to
a27327f
Compare
NathanFlurry
force-pushed
the
10-22-feat_examples-cursors_cursors_example
branch
from
eb78d9e to
23bc2c4
Compare
Pull Request Review: Cursors with Raw WebSockets ExampleThank you for this example! This is a well-implemented demonstration of real-time collaborative cursors using RivetKit. Here's my detailed review: ✅ Strengths
🐛 Potential Bugs & IssuesBackend (
|
Code Review: Cursors Raw WebSocket ExampleThis PR adds a new example demonstrating real-time collaborative cursors using raw WebSocket connections instead of RivetKit's higher-level RPC actions. Overall, this is a well-structured example that demonstrates advanced WebSocket usage. Here's my detailed feedback: ✅ Strengths
🐛 Potential Issues1. Cursor ID Mismatch Bug (registry.ts:46-48, App.tsx:118-122)There's a critical bug in cursor tracking. The backend uses // registry.ts:46
cursors[sessionId] = cursor;But the frontend uses // App.tsx:121
[message.data.userId]: message.dataImpact: Cursors won't be correctly tracked/removed, leading to stale cursor states. Fix: Use consistent keys. Since the cursor removal logic (registry.ts:187-196) broadcasts 2. Memory Leak in WebSocket Event Listeners (registry.ts:93-179)Event listeners are added to WebSocket but never explicitly removed. While this may not cause issues in practice (since listeners are scoped to the connection lifetime), it's better to be explicit. Recommendation: Store event listener cleanup functions if needed, though this is a minor concern since the WebSocket lifecycle is managed by the actor. 3. Race Condition in Initial State (registry.ts:75-82)The initial state sent to new connections includes cursors from all existing connections, but doesn't account for connections that haven't sent a cursor position yet. A new connection gets Impact: Low - just means the initial state might be slightly stale until first cursor movement. 4. Input Validation Missing (registry.ts:99-173)The WebSocket message handlers don't validate the incoming data structure. Missing or malformed fields could cause runtime errors. Recommendation: Add validation for required fields: case "updateCursor": {
const { userId, x, y } = message.data;
if (!userId || typeof x !== 'number' || typeof y !== 'number') {
console.error('invalid cursor data:', message.data);
break;
}
// ... rest of logic
}⚡ Performance Considerations1. Cursor Update Frequency (App.tsx:199-209)Every mouse movement triggers a WebSocket send. This could generate a lot of messages with fast mouse movements. Recommendation: Consider throttling cursor updates (e.g., max 60 updates/second): const throttledMouseMove = useCallback(
throttle((x: number, y: number) => {
if (wsRef.current?.readyState === WebSocket.OPEN) {
wsRef.current.send(JSON.stringify({ type: 'updateCursor', data: { userId, x, y }}));
}
}, 16), // ~60fps
[userId]
);2. Broadcasting to All Including Sender (registry.ts:113-121)The cursor update broadcasts to all connections including the sender. The sender already knows their own cursor position, so this creates unnecessary network traffic. Recommendation: Either filter out the sender or document why this is intentional (perhaps for server-authoritative position?). 🔒 Security Concerns1. No Input Sanitization (registry.ts:126, 159)Text content and IDs are not sanitized before being stored and broadcast. While this is an example app, it's worth noting for production use. Recommendation: Add a comment noting that production apps should sanitize text input to prevent XSS or add basic sanitization as a best practice demonstration. 2. No Rate LimitingA malicious client could spam cursor updates or text updates, potentially overwhelming the server or other clients. Recommendation: For a production example, consider demonstrating basic rate limiting or add a comment about it. 3. CORS Configuration (server.ts:4-6)The CORS origin is hardcoded to localhost. This is fine for an example, but worth documenting. 📝 Code Quality & Best Practices1. Inconsistent Logging Style (registry.ts:68-70, 183)The logging doesn't follow the structured logging pattern mentioned in CLAUDE.md. Per the guidelines, logs should use structured logging, not string interpolation. Should be: console.log('websocket connected', { sessionId, actorId: c.actorId });2. Error Handling Could Be More Specific (registry.ts:176-178, App.tsx:158-160)Generic 3. Magic Values (App.tsx:216)The text ID generation uses a pattern Recommendation: Use crypto.randomUUID() or add a counter. 4. README Path Issue (README.md:18)README says 🧪 Test CoverageTests Are Too Basic (cursors.test.ts)The tests only check that actions can be called, but don't test:
Recommendation: Add tests for WebSocket behavior, especially the critical cursor tracking and broadcast logic. 📚 Documentation1. Missing Architecture ExplanationThe README doesn't explain why someone would use raw WebSockets instead of the RPC-based approach from the regular Recommendation: Add a section explaining:
2. Comparison with Regular Cursors ExampleSince there's already a 🎯 Suggested Improvements
SummaryThis is a solid example that demonstrates raw WebSocket usage well. The main issues are:
The code is generally clean and well-structured. With the bug fix and some improvements to documentation and testing, this will be an excellent addition to the examples! |
NathanFlurry
force-pushed
the
10-22-feat_examples-cursors_cursors_example
branch
from
23bc2c4 to
db3c3ad
Compare
NathanFlurry
force-pushed
the
10-22-feat_examples_cursors_with_raw_websockets_example
branch
from
a27327f to
0ffdb8b
Compare
Pull Request Review: Cursors with Raw WebSockets ExampleThanks for adding this comprehensive example! This demonstrates raw WebSocket usage with RivetKit actors nicely. Below is my review organized by category: Code Quality & Best PracticesPositive Observations
Suggestions
} catch (error) {
console.error("error handling websocket message:", error);
}Consider logging more context (sessionId, message type, actorId) for better debugging.
Potential Bugs & IssuesCritical Issues
Medium Priority Issues
Performance ConsiderationsIssues
Suggestions// Add at top of file
const throttle = (fn: Function, wait: number) => {
let lastCall = 0;
return (...args: any[]) => {
const now = Date.now();
if (now - lastCall >= wait) {
lastCall = now;
fn(...args);
}
};
};Security ConcernsIssues
Medium Priority
Test CoverageCurrent StateThe test file (
Missing Coverage
Recommended Test Structuretest("Cursor position updates are broadcast to all clients", async (ctx: any) => {
// Create WebSocket connections for 2 users
// Send cursor update from user 1
// Verify user 2 receives the update
});
test("Text labels persist across connections", async (ctx: any) => {
// Connect user 1, place text, disconnect
// Connect user 2
// Verify user 2 sees the text label
});
test("Invalid messages are handled gracefully", async (ctx: any) => {
// Send malformed JSON
// Send invalid message type
// Verify actor doesn't crash
});Additional Suggestions
SummaryThis is a solid example that demonstrates RivetKit's WebSocket capabilities well! The main concerns are: Must Fix (P0):
Should Fix (P1):
Nice to Have (P2):
Overall, this is a great addition to the examples! With the performance and security fixes, it will be production-ready. Great work! 🎉 |
Pull Request Review: Cursors with Raw WebSocket ExampleThank you for this example! Great demonstration of real-time collaborative features using RivetKit. Strengths
Critical Issues1. Bug: Cursor Keying Mismatch (src/frontend/App.tsx:119 and src/backend/registry.ts:48) 2. Missing Input Validation (src/backend/registry.ts:99, 126) 3. Memory Leak: Unbounded State (src/backend/registry.ts:143-144) 4. Missing Error Handling (src/backend/registry.ts:115, 148, 166) 5. Performance: Excessive Updates (src/frontend/App.tsx:199-208) 6. Stale Closure Bug (src/frontend/App.tsx:83-185) 7. Test Coverage (tests/cursors.test.ts) 8. Documentation (README.md:19) RecommendationsHigh Priority (fix before merge):
Medium Priority:
SecurityNo major vulnerabilities but lack of input validation could enable DoS or UI manipulation. Suitable for dev/demo but needs hardening for production. Great work! Core functionality is solid, issues are mostly robustness and edge cases. |
NathanFlurry
force-pushed
the
10-22-feat_examples_cursors_with_raw_websockets_example
branch
from
0ffdb8b to
f0c5e79
Compare
Pull Request Review: Cursors with Raw WebSockets ExampleSummaryThis PR adds a new example demonstrating real-time collaborative cursors using raw WebSockets with RivetKit. The implementation is well-structured and demonstrates proper usage of the actor model. Code Quality & Best PracticesStrengths
Issues & RecommendationsBug: Cursor Key Mismatch (App.tsx:121, 368)The cursor state is keyed by userId but compared with sessionId. This means the 'you' label will never display correctly. Use sessionId consistently as the key throughout. Critical: Input Validation Missing (registry.ts:99-104, 126-134, 159-162)
Bug: State Mutation (registry.ts:110, 141)Direct mutation of nested state objects. Consider if the actor framework expects immutable updates. Performance: Unnecessary Re-broadcasts (registry.ts:113-121)Broadcasting cursor updates to the sender wastes bandwidth. Skip the sender in broadcasts. Missing Error Handling (App.tsx:82-184)
Resource Leak Potential (registry.ts:93-179)Event listeners are added but never explicitly removed. Missing Rate Limiting (registry.ts:98-123)No rate limiting on cursor updates. A malicious client could overwhelm the server. Console LoggingUse tracing.info with structured data instead of console.log per CLAUDE.md guidelines. README Issue (README.md:19)Path is incorrect: should be cursors-raw-websocket not cursors. Test CoverageOnly 2 basic tests. Missing tests for WebSocket functionality, concurrent users, and edge cases. Performance Considerations
Security Concerns
SummaryMust Fix:
Should Fix:
Nice to Have:
Overall, solid example code but needs hardening before production-ready. Great work! |
NathanFlurry
force-pushed
the
10-22-feat_examples_cursors_with_raw_websockets_example
branch
from
f0c5e79 to
e786fc1
Compare
NathanFlurry
force-pushed
the
10-22-feat_examples-cursors_cursors_example
branch
from
120bdcb to
b2d5bfe
Compare
PR Review: Cursors with Raw WebSockets ExampleThanks for this example! This is a great demonstration of real-time collaborative features using RivetKit. Here's my detailed review: ✅ Strengths
🐛 Potential IssuesCritical: Cursor Keying Bug (registry.ts:48, App.tsx:121, App.tsx:368)The backend keys cursors by
This means if a user opens two tabs, they'll see two separate cursors instead of one unified cursor. The keying should be consistent - either both use Recommendation: Use Security: Input Validation Missing (registry.ts:99, 126, 159)The WebSocket message handlers don't validate incoming data:
Recommendation: Add validation like: if (typeof x !== 'number' || x < 0 || x > MAX_X) return;
if (text.length > 500) return; // or truncateRace Condition: WebSocket State Mutation (registry.ts:93-179)The Recommendation: Add connection state checks before sending messages. Memory Leak: Event Listeners (registry.ts:93)Event listeners are added to the WebSocket but never explicitly removed. While WebSocket closure typically cleans these up, it's better to be explicit. Recommendation: Store listeners in variables and remove them on close.
|
PR Review: Cursors with Raw WebSockets ExampleThank you for this example! This is a clean demonstration of real-time collaborative cursors using RivetKit. Here's my detailed review: ✅ Strengths
🐛 Potential IssuesCritical
Medium Priority
Low Priority
🔒 Security Concerns
🚀 Performance Considerations
🧪 Test CoverageCurrent coverage: Basic (2 tests) Missing tests:
Recommendation: Add integration tests that simulate WebSocket connections and verify message broadcasting 📝 Code Quality & Best Practices
🎯 Recommendations SummaryMust fix before merge:
Should fix: Nice to have: Overall AssessmentThis is a solid example that demonstrates RivetKit's WebSocket capabilities well. The code is clean and readable. The main concerns are around edge cases and error handling that could cause issues in real-world usage. For an example, the current implementation is good, but the sessionId/userId inconsistency should be resolved to avoid confusion. Recommendation: ✅ Approve with requested changes (fix critical issues #1 and #2) Great work on this example! 🎉 |
NathanFlurry
deleted the
10-22-feat_examples_cursors_with_raw_websockets_example
branch
1 participant
No description provided.