feat: realtime presence/awareness API#16
Conversation
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @mudiageo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust real-time presence and awareness system, enabling applications to track and display the online status, cursor movements, text selections, and editing activities of users within a collaborative environment. This new functionality is encapsulated in a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
There was a problem hiding this comment.
Code Review
This pull request introduces a real-time presence system, which is a great new feature. The implementation in presence.svelte.ts is comprehensive, covering user state, idle detection, and a heartbeat mechanism. The integration into CollectionStore is clean and provides a simple API for enabling presence on a collection.
My review focuses on improving type safety and fixing a potential memory leak. Specifically, I've pointed out:
- A memory leak in
presence.svelte.tsdue to un-removed global event listeners. - Several opportunities to replace
anywith stronger types, particularly in the event handling systems, to make the code more robust. - A type mismatch in
sync.svelte.tsregarding thepresenceStoreproperty. - The use of hardcoded magic numbers for timers, which could be extracted into constants for better readability.
Overall, this is a solid initial implementation. Addressing these points will enhance the code's reliability and maintainability.
| private setupIdleDetection(): void { | ||
| const resetIdle = () => { | ||
| if (this.myState.status === 'idle') { | ||
| this.setActive(); | ||
| } | ||
| this.resetIdleTimer(); | ||
| }; | ||
|
|
||
| window.addEventListener('mousemove', resetIdle); | ||
| window.addEventListener('keydown', resetIdle); | ||
| window.addEventListener('click', resetIdle); | ||
|
|
||
| this.resetIdleTimer(); | ||
| } |
There was a problem hiding this comment.
The setupIdleDetection method adds global event listeners to window, but they are never removed in the destroy method. This creates a memory leak, as each PresenceStore instance will leave dangling listeners that can cause unexpected behavior even after the store is destroyed.
To fix this, you should promote the resetIdle function to a class method and then remove the listeners in destroy().
Here's how you can refactor:
-
Move
resetIdleout ofsetupIdleDetectionand make it a private class method:private resetIdle = (): void => { if (this.myState.status === 'idle') { this.setActive(); } this.resetIdleTimer(); };
-
Update
setupIdleDetectionto use the new class method:private setupIdleDetection(): void { window.addEventListener('mousemove', this.resetIdle); window.addEventListener('keydown', this.resetIdle); window.addEventListener('click', this.resetIdle); this.resetIdleTimer(); }
-
Update
destroy()to remove the listeners:destroy(): void { if (this.heartbeatInterval) clearInterval(this.heartbeatInterval); if (this.idleTimer) clearTimeout(this.idleTimer); window.removeEventListener('mousemove', this.resetIdle); window.removeEventListener('keydown', this.resetIdle); window.removeEventListener('click', this.resetIdle); if (this.realtimeClient) { // ... } this.eventListeners.clear(); }
|
|
||
| private _fields: FieldsProxy<T>; | ||
|
|
||
| private presenceStore: PresenceStore<T> | null = null; |
There was a problem hiding this comment.
The presenceStore property is typed as PresenceStore<T>, where T is the type of the collection's documents. However, the presence() method initializes it with a custom state of type any, creating a PresenceStore<any>. This assignment is not type-safe because the custom presence data is unrelated to the collection's data type T.
To fix this, the property should be typed as PresenceStore<any> | null.
For even better type safety, you could make the presence() method generic to capture the type of the custom state:
presence<CustomData = any>(config: { user: User; custom?: CustomData }): PresenceStore<CustomData> {
if (!this.presenceStore) {
this.presenceStore = new PresenceStore(
this.engine.realtime,
this.tableName,
config.user,
config.custom
);
}
return this.presenceStore as PresenceStore<CustomData>;
}| private presenceStore: PresenceStore<T> | null = null; | |
| private presenceStore: PresenceStore<any> | null = null; |
| private followingUserId: string | null = $state(null); | ||
| private heartbeatInterval: number | null = null; | ||
| private idleTimer: number | null = null; | ||
| private eventListeners: Map<string, Set<(data: any) => void>> = new Map(); |
There was a problem hiding this comment.
The eventListeners map uses (data: any) => void for its handlers, which weakens type safety. You can create a much safer event system by defining an interface for your events and using generic types to provide strong typing for event payloads.
For example:
interface PresenceEvents<T> {
'update': PresenceState<T>;
'join': PresenceState<T>;
'leave': PresenceState<T>;
'following:update': PresenceState<T>;
}Then, you can update your on and emit methods to be strongly typed:
// Update 'on' method signature
on<K extends keyof PresenceEvents<T>>(event: K, handler: (data: PresenceEvents<T>[K]) => void): () => void
// Update 'emit' method signature
private emit<K extends keyof PresenceEvents<T>>(event: K, data: PresenceEvents<T>[K]): voidThis will provide type checking and autocompletion for event names and their payloads, preventing common bugs.
| this.heartbeatInterval = window.setInterval(() => { | ||
| this.broadcastPresence(); | ||
| }, 30000); |
There was a problem hiding this comment.
The heartbeat interval is hardcoded to 30000. Using magic numbers like this can make the code difficult to understand and maintain. It's better to extract this value into a named constant at the top of the file, for example:
const HEARTBEAT_INTERVAL_MS = 30000;
This same principle applies to the idle timeout (5 * 60 * 1000) in the resetIdleTimer method. For greater flexibility, you could also consider making these values configurable through the PresenceStore constructor.
- Created EphemeralStore for in-memory presence/ephemeral data - Added presence event types to realtime types - Updated RealtimeServer with presence and ephemeral handlers - Added POST endpoint in ServerSyncEngine for client->server messages - Added send() method to RealtimeClient for bidirectional communication - Updated PresenceStore to use send() instead of emit() - Added debouncing for cursor updates (50ms) Co-authored-by: mudiageo <76590594+mudiageo@users.noreply.github.com>
- Created ephemeral-store.test.ts with 25 test cases - Fixed import path in presence.svelte.ts - Fixed error type annotations to use 'unknown' - All tests passing successfully Co-authored-by: mudiageo <76590594+mudiageo@users.noreply.github.com>
- Created SyncChannel for channel-scoped real-time communication - Added channel() method to SyncEngine - Implemented presence tracking, custom events, subscribe/unsubscribe - Created comprehensive channel tests (browser tests need Playwright) Co-authored-by: mudiageo <76590594+mudiageo@users.noreply.github.com>
- Updated src/pkg/index.ts to export SyncChannel, PresenceStore, RealtimeClient - Exported presence and realtime types (User, CursorPosition, PresenceState, etc.) - Updated src/pkg/server/index.ts to export RealtimeServer and EphemeralStore - Exported server realtime types Co-authored-by: mudiageo <76590594+mudiageo@users.noreply.github.com>
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
- Add 9 comprehensive multi-user/multi-client scenario tests - Create presence/awareness documentation (content/docs/presence-awareness.md) - Add internal architecture document (docs/REALTIME_ARCHITECTURE.md) - Create changesets for feature (minor) and API change (patch) - Enhance todos route with real-time collaborator avatars and status - Enhance notes route with editing indicators and presence - All tests passing (82 total: 73 existing + 9 new) Co-authored-by: mudiageo <76590594+mudiageo@users.noreply.github.com>
… store Co-authored-by: mudiageo <76590594+mudiageo@users.noreply.github.com>
- Add 3 integration tests for collection.presence() API - Update notes route to use collection.presence() directly (simpler than channel) - Restructure docs: collection.presence() as primary, channels as alternative - Clarify handle vs explicit routes (use one, not both) - Document when to use each API approach Co-authored-by: mudiageo <76590594+mudiageo@users.noreply.github.com>
… store Co-authored-by: mudiageo <76590594+mudiageo@users.noreply.github.com>
- Created usePresence hook for simplified Svelte 5 DX - Implemented awareness utilities: useCursorTracking, useSelectionTracking, useWhoIsHere - Added comprehensive tests for all new functionality (16 new tests) - Exported new hooks and utilities from main index - All utilities use Svelte 5 runes for reactivity Co-authored-by: mudiageo <76590594+mudiageo@users.noreply.github.com>
- Updated presence-awareness.md with hooks & utilities section - Added detailed examples for usePresence, useCursorTracking, useSelectionTracking, useWhoIsHere - Created HOOKS_USAGE_EXAMPLE.md with complete collaborative whiteboard example - Included API comparisons and real-world patterns Co-authored-by: mudiageo <76590594+mudiageo@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| } | ||
|
|
||
| private setupIdleDetection(): void { | ||
| const resetIdle = () => { | ||
| if (this.myState.status === 'idle') { | ||
| this.setActive(); | ||
| } | ||
| this.resetIdleTimer(); | ||
| }; | ||
|
|
||
| window.addEventListener('mousemove', resetIdle); | ||
| window.addEventListener('keydown', resetIdle); | ||
| window.addEventListener('click', resetIdle); |
There was a problem hiding this comment.
Suggestion: Prevent memory leaks from idle detection
| } | |
| private setupIdleDetection(): void { | |
| const resetIdle = () => { | |
| if (this.myState.status === 'idle') { | |
| this.setActive(); | |
| } | |
| this.resetIdleTimer(); | |
| }; | |
| window.addEventListener('mousemove', resetIdle); | |
| window.addEventListener('keydown', resetIdle); | |
| window.addEventListener('click', resetIdle); | |
| private setupIdleDetection(): void { | |
| window.addEventListener('mousemove', this.idleResetHandler); | |
| window.addEventListener('keydown', this.idleResetHandler); | |
| window.addEventListener('click', this.idleResetHandler); | |
| this.resetIdleTimer(); | |
| } | |
| private resetIdle(): void { | |
| if (this.myState.status === 'idle') { | |
| this.setActive(); | |
| } | |
| this.resetIdleTimer(); |
User description
Description
Brief description of changes.
Type of change
Checklist
npx changeset)Related Issues
Closes #(issue number)
Testing
How has this been tested?
PR Type
Enhancement
Description
Add real-time presence/awareness system for collaborative editing
Implement PresenceStore class tracking user status and cursor positions
Support user following, idle detection, and heartbeat synchronization
Integrate presence API into CollectionStore for easy adoption
Diagram Walkthrough
File Walkthrough
presence.svelte.ts
Real-time presence store implementationsrc/pkg/client/presence.svelte.ts
synchronization
resource
sync.svelte.ts
Integrate presence into CollectionStoresrc/pkg/client/sync.svelte.ts