feat(examples-cursors): cursors example #3244
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
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.
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. |
Pull Request Review: Cursors ExampleThank you for contributing this collaborative cursors example! This is a great demonstration of real-time functionality with RivetKit. I've identified several issues that need to be addressed before merging. Critical Issues1. Test Implementation Doesn't Match Actor API
|
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
changed the base branch from
10-22-feat_rivetkit_support_routing_via_query_params
to
graphite-base/3244
NathanFlurry
force-pushed
the
graphite-base/3244
branch
from
ea8ef90 to
ed34c5a
Compare
NathanFlurry
force-pushed
the
10-22-feat_examples-cursors_cursors_example
branch
from
8766cdd to
eb78d9e
Compare
NathanFlurry
changed the base branch from
graphite-base/3244
to
10-22-feat_core_allow_routing_traffic_to_actors_via_path
Pull Request Review: Cursors ExampleGreat work on this collaborative cursors example! This is a solid demonstration of RivetKit's real-time capabilities. I've reviewed the code and have the following feedback: ✅ Strengths
🐛 Issues FoundCritical: Test API MismatchLocation: The tests are calling methods that don't exist in the registry:
Registry only defines: Impact: All tests will fail when run. Fix Required: Either:
Bug: Event Handler Never TriggeredLocation: cursorRoom.useEvent("cursorRemoved", (cursor: CursorPosition) => {
// ...
});The backend never emits a Impact: Remote cursors won't disappear when users disconnect, leading to "ghost cursors" on the canvas. Fix: Either remove this handler (if RivetKit handles cleanup automatically) or add an event emission when connections close.
|
PR Review: Cursors ExampleThanks for this contribution! This is a well-structured example demonstrating real-time collaborative cursors. I've identified several issues that need to be addressed before merging. Critical Issues1. Test Method Mismatch (Tests Will Fail)Location: The tests call methods that don't exist in the registry:
The only action defined in Fix needed: // Instead of:
const cursors = await room.getCursors();
const labels = await room.getTextLabels();
// Use:
const state = await room.getRoomState();
const cursors = state.cursors;
const labels = state.textLabels;
// Instead of:
await room.placeText("user1", "Hello", 50, 75);
// Use:
const labelId = `user1-${Date.now()}`;
await room.updateText(labelId, "user1", "Hello", 50, 75);You'll need to rewrite all 5 test cases to use the correct API. 2. Cursor Removal Logic MissingLocation: The tests expect a Suggested fix:
3. Event Not EmittedLocation: The frontend listens for a cursorRoom.useEvent("cursorRemoved", (cursor: CursorPosition) => {
// This will never be called
});Either remove this event listener or implement cursor removal broadcasts in the registry. Bugs4. Cursor Dictionary Key MismatchLocation: Object.entries(cursors).map(([id, cursor]) => {
const isOwnCursor = id === userId; // BUG: comparing wrong valuesThe const isOwnCursor = cursor.userId === userId;5. Missing .gitignore EntryLocation: File is missing newline at end of file (line 2). While minor, this should be fixed for consistency. Code Quality Suggestions6. Error HandlingLocation: Good that you added error handling, but consider showing an error message to the user instead of just console logging: .catch((error) => {
console.error('error loading room state', error);
// Consider: setError('Failed to load room state')
});7. Cursor Position Updates PerformanceLocation: Consider throttling cursor updates to reduce broadcast frequency. Sending every mousemove event could be expensive with many users. A 16ms throttle (60fps) would be reasonable: // In frontend, throttle the updateCursor calls
const throttledUpdateCursor = useThrottle(
(x: number, y: number) => cursorRoom.connection.updateCursor(userId, x, y),
16
);8. Text Label ID GenerationLocation: The text ID generation const newTextId = `${userId}-${Date.now()}-${Math.random()}`;9. Input Blur BehaviorLocation: When the hidden input loses focus, text is cancelled. This could be frustrating UX - consider keeping the input focused or completing the text on blur instead of canceling it. Documentation10. Missing Information
Security✅ No security concerns identified. The code:
Test CoverageNeed to add tests for:
SummaryRequired Changes:
Recommended Changes:
Once the critical issues are fixed, this will be a great example of RivetKit's real-time collaboration capabilities! |
NathanFlurry
force-pushed
the
10-22-feat_core_allow_routing_traffic_to_actors_via_path
branch
from
ed34c5a to
89114d1
Compare
NathanFlurry
force-pushed
the
10-22-feat_examples-cursors_cursors_example
branch
from
eb78d9e to
23bc2c4
Compare
PR Review: Cursors ExampleThis is a great example demonstrating real-time collaborative cursors with RivetKit! The code is generally well-structured and demonstrates the framework's capabilities effectively. Here are my findings: ✅ Strengths
🐛 Issues FoundCritical: Test/Implementation MismatchThe tests reference methods that don't exist in the actor implementation: In
Actual actor methods in
The tests will fail as written. You need to either:
Frontend IssuesLine 118-124 in App.tsx: Event listener for Recommendation: Either remove the unused event listener or implement cursor removal broadcasting in the backend. 📋 Code QualityBackend (
|
PR Review: Cursors ExampleThank you for this well-structured example! This is a great demonstration of RivetKit's capabilities for real-time collaborative features. I've reviewed the code and have several findings to share: Critical Issues1. Test-Implementation MismatchLocation: The tests reference methods that don't exist in the actor implementation:
Impact: Tests will fail when run. Recommendation: Either:
2. Missing Newline in .gitignoreLocation: The file is missing a trailing newline, which is a common practice and may cause linting warnings. Moderate Issues3. Cursor Position Keying IssueLocation: The cursors are keyed by connection ID (the map key), but compared with Object.entries(cursors).map(([id, cursor]) => {
const isOwnCursor = id === userId; // This comparison won't workThe key is from 4. Event Listener Not EmittedLocation: The code listens for a Recommendation: Remove the unused event listener or document that cursor cleanup happens automatically via connection state. 5. Potential Race ConditionLocation: The onBlur={() => {
if (!textInput.trim() && cursorRoom.connection && currentTextId) {
cursorRoom.connection.removeText(currentTextId);
setCurrentTextId(null);
}
setIsTyping(false);
}}If a user presses Enter and then the input loses focus, both handlers might try to finalize the text. Consider adding a debounce or checking Minor Issues & Suggestions6. Missing Input ValidationLocation: The actions don't validate input:
Recommendation: Add basic validation, especially for coordinates to ensure they're valid numbers. 7. Performance: Unnecessary Re-rendersLocation: Each event listener causes a state update and re-render. For high-frequency cursor updates, consider:
8. Missing Error HandlingLocation: Multiple Recommendation: Add try-catch blocks or 9. TypeScript StrictnessLocation: While "noUncheckedIndexedAccess": true,
"noImplicitOverride": true10. Memory Leak: Event ListenersLocation: The 11. Accessibility IssuesLocation: The canvas has
Recommendation: Add ARIA attributes and consider keyboard navigation for accessibility. Positive Highlights✅ Clean Architecture: Nice separation between backend (registry) and frontend Security Assessment✅ No major security concerns for an example project
Test Coverage AssessmentThe tests cover core functionality well:
Missing coverage:
Recommendations SummaryBefore Merge:
After Merge (Future Improvements): Overall, this is a solid example that demonstrates RivetKit's capabilities well! The critical issue is the test mismatch - everything else is relatively minor for an example project. Great work! 🎉 |
NathanFlurry
force-pushed
the
10-22-feat_core_allow_routing_traffic_to_actors_via_path
branch
from
89114d1 to
e5f9730
Compare
NathanFlurry
force-pushed
the
10-22-feat_examples-cursors_cursors_example
branch
from
23bc2c4 to
db3c3ad
Compare
PR Review: Cursors ExampleOverviewThis PR adds a real-time collaborative cursors example demonstrating RivetKit's capabilities. The implementation is well-structured and provides a good showcase of the framework's features. ✅ Strengths
🐛 Issues FoundCritical
High Priority
Medium Priority
🎯 Code QualityGood Practices Observed
Suggestions
🔒 Security
⚡ Performance
📚 Documentation
🧪 Test CoverageCurrent Coverage: Good basic coverage Missing Test Cases:
📝 RecommendationsMust Fix Before Merge
Should Fix Before Merge
Nice to Have
🎉 ConclusionThis is a solid example implementation that effectively demonstrates RivetKit's real-time capabilities. The code is generally well-structured and follows the project's conventions. The main issues are test mismatches and some missing edge case handling, which should be addressed before merging. Recommendation: ✅ Approve with requested changes Great work on this example! 🚀 |
PR Review: Cursors ExampleSummaryThis PR adds a real-time collaborative cursors example demonstrating RivetKit's actor capabilities. The implementation includes cursor tracking, text placement, and multi-room support with a React frontend. Code Quality & Best PracticesStrengths ✅
Issues & Recommendations1. State Management Bug in registry.ts
|
NathanFlurry
force-pushed
the
10-22-feat_examples-cursors_cursors_example
branch
from
db3c3ad to
120bdcb
Compare
NathanFlurry
force-pushed
the
10-22-feat_core_allow_routing_traffic_to_actors_via_path
branch
from
e5f9730 to
a5f5632
Compare
Pull Request Review: Cursors ExampleSummaryThis PR adds a real-time collaborative cursors example for RivetKit. Overall, this is a high-quality example that demonstrates RivetKit's capabilities well. The code is clean, well-structured, and includes comprehensive tests. However, I've identified several issues that should be addressed. 🔴 Critical Issues1. Test-Backend API MismatchLocation: The tests reference methods that don't exist in the backend registry:
The backend only exposes:
Impact: All tests will fail when run. Fix Required: Either:
🟡 Medium Priority Issues2. Missing Input ValidationLocation: The
Recommendation: Add validation: updateText: (c, id: string, userId: string, text: string, x: number, y: number) => {
// Validate text length
if (text.length > 1000) {
throw new Error('text too long');
}
// Validate coordinates
if (!Number.isFinite(x) || !Number.isFinite(y)) {
throw new Error('invalid coordinates');
}
// ... rest of implementation
}3. Memory Leak PotentialLocation: The Recommendation: Add a limit or cleanup strategy:
4. Race Condition in FrontendLocation: The onBlur={() => {
if (!textInput.trim() && cursorRoom.connection && currentTextId) {
cursorRoom.connection.removeText(currentTextId);
setCurrentTextId(null);
}
setIsTyping(false);
}}Recommendation: Add debouncing or check 5. Hardcoded Backend URLLocation: const { useActor } = createRivetKit<typeof registry>("http://localhost:6420");Hardcoding Recommendation: Use environment variables: const BACKEND_URL = import.meta.env.VITE_BACKEND_URL || "http://localhost:6420";
const { useActor } = createRivetKit<typeof registry>(BACKEND_URL);🟢 Minor Issues / Suggestions6. Missing Newline at EOFLocation: The 7. Unused Event ListenerLocation: The
8. Console.error Instead of Proper Error HandlingLocation: .catch((error) => {
console.error('error loading room state', error);
});Recommendation: Show user-facing error message or retry logic. 9. TypeScript InconsistencyLocation: Using Suggestion: Use 10. Performance: O(n) Text Label UpdatesLocation: Every text update triggers a ✅ Positive Aspects
🔒 Security NotesNo major security concerns for an example project, but for production use:
📋 RecommendationsBefore Merging:
Future Enhancements:
ConclusionThis is a well-crafted example that will be valuable for the RivetKit community. The main blocker is the test/backend API mismatch which will cause test failures. Once the critical issues are addressed, this will be ready to merge. Great work overall! 🎉 |
PR Review: Cursors Example for RivetKitThis is a well-crafted example demonstrating real-time collaborative cursor tracking. The code is generally clean and functional, but there are several issues that need addressing. Critical Issues1. Test-Implementation Mismatch
|
NathanFlurry
force-pushed
the
10-22-feat_examples-cursors_cursors_example
branch
from
120bdcb to
b2d5bfe
Compare
NathanFlurry
force-pushed
the
10-22-feat_core_allow_routing_traffic_to_actors_via_path
branch
from
a5f5632 to
82faa7e
Compare
Pull Request Review: Cursors ExampleSummaryThis PR adds a real-time collaborative cursors example demonstrating RivetKit's actor system capabilities. Overall, the implementation is well-structured and demonstrates good practices. However, there are several issues that need to be addressed. Critical Issues1. Backend API Mismatch with Tests
|
PR Review: Cursors ExampleGreat work on creating this collaborative cursors example! This is a well-structured demonstration of RivetKit's real-time capabilities. Here's my detailed review: ✅ Strengths
🐛 Critical IssuesTest API MismatchThe tests reference methods that don't exist in the registry: // tests/cursors.test.ts:10, 28, 43, 47, 60, 82
await room.getCursors() // ❌ Not defined
await room.getTextLabels() // ❌ Not defined
await room.placeText(...) // ❌ Not defined
await room.removeCursor(...) // ❌ Not definedThe registry only defines:
Fix needed: Either:
|
1 participant
No description provided.