feat: handle all SSE annotation events for real-time multi-user sync#116
feat: handle all SSE annotation events for real-time multi-user sync#116remorses wants to merge 9 commits intobenjitaylor:mainfrom
Conversation
Canvas overlay for drawing on the page with gesture classification (box, circle, underline, arrow). Click strokes to create linked annotations. Includes undo (cmd+z), hover highlighting, and drawing descriptions in copy output.
Shake the popup if you try to draw while it's open. Replace stroke width change on hover with a soft translucent glow behind the stroke.
- Drawing glow animates in/out via rAF lerp (0→0.25 alpha) instead of snapping. Works for both stroke hover and marker hover directions. - Canvas hidden when marker visibility is toggled off (H key / eye button), unless actively in draw mode.
This reverts commit fbd9b593df59ae67b8a4440743d2d34c385442f2.
Glow highlight lerps via rAF (separate effect, doesn't touch resize/ scroll). Canvas hidden when marker visibility toggled off.
Single-pass rendering with canvas shadowBlur on hovered stroke instead of the two-pass wide translucent stroke. glowIntensity 0-1 controls blur radius (0-12px), animated in/out via rAF lerp.
… fade - Replace shadowBlur glow with globalAlpha dim (non-hovered strokes at 30%) - Add tooltip exit animation (100ms fade out on unhover) - Move canvas outside overlay so it fades on toolbar close - Fix marker exit timing: compute timeout from stagger delay + animation - Fix marker enter timing: same, prevents class removal mid-animation - Skip stagger delay for individually added markers (only batch entrance) - Remove scale(1.3) on markers when linked drawing is hovered - Add drawCanvasFading state for deletion fade - Use visibility:hidden for hit-test hiding (canvas uses opacity now)
- Revert canvas from pointer events back to mouse events (fixes strokes not registering) - Add unique IDs to draw strokes and strokeId on annotations (fixes wrong strokes being deleted when indices shift) - Delete/cancel now find strokes by stable ID, not positional index - Per-stroke fade on delete uses refs for fresh data, no stale closures - Canvas visibility uses shouldShowMarkers only (hiding markers while in draw mode now works, also exits draw mode) - Remove touch-action, drawModeActive CSS, pointer capture
The SSE listener previously only handled annotation.updated events, and only for resolved/dismissed status (marker removal). This expands it to handle the full lifecycle: - annotation.created: adds remote annotations to local state with deduplication (skips if the annotation already exists locally from our own create flow). Double-checked inside the state setter to guard against races where two SSE events arrive before a render. - annotation.updated: preserves the existing resolved/dismissed removal logic, and additionally merges field updates for general edits (comment changes, status transitions, etc). - annotation.deleted: removes annotations with exit animation. Always attempts removal even if ref is stale, fires callback only when the annotation was found in the ref snapshot. All three handlers fire the existing callback props (onAnnotationAdd, onAnnotationUpdate, onAnnotationDelete) so host pages can react to remote events without new API surface.
|
@remorses is attempting to deploy a commit to the Benji Taylor's Projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR extends the SSE (Server-Sent Events) listener to handle the complete annotation lifecycle for real-time multi-user synchronization. Previously, the component only listened for annotation.updated events to handle resolved/dismissed marker removal. Now it handles annotation.created, annotation.updated, and annotation.deleted events, enabling multiple users viewing the same page to see each other's annotation changes in real-time.
Changes:
- Added
handleCreatedevent handler with deduplication logic to add remote annotations while skipping locally created ones - Expanded
handleUpdatedto merge general field updates in addition to the existing resolved/dismissed removal logic - Added
handleDeletedevent handler that removes annotations with exit animation and fires callbacks only for existing annotations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1189,11 +1189,30 @@ export function PageFeedbackToolbarCSS({ | |||
|
|
|||
| const removedStatuses = ["resolved", "dismissed"]; | |||
|
|
|||
| const handler = (e: MessageEvent) => { | |||
| const handleCreated = (e: MessageEvent) => { | |||
| try { | |||
| const event = JSON.parse(e.data); | |||
| if (removedStatuses.includes(event.payload?.status)) { | |||
| const id = event.payload.id as string; | |||
| const annotation = event.payload as Annotation; | |||
| if (!annotation?.id) return; | |||
| // Skip if we already have this annotation locally (our own create). | |||
| // Ref check guards the callback; setter check guards state against races. | |||
| if (annotationsRef.current.some((a) => a.id === annotation.id)) return; | |||
| setAnnotations((prev) => | |||
| prev.some((a) => a.id === annotation.id) ? prev : [...prev, annotation] | |||
| ); | |||
| onAnnotationAdd?.(annotation); | |||
| } catch { | |||
| // Ignore parse errors | |||
| } | |||
| }; | |||
|
|
|||
| const handleUpdated = (e: MessageEvent) => { | |||
| try { | |||
| const event = JSON.parse(e.data); | |||
| const annotation = event.payload as Annotation; | |||
| if (!annotation?.id) return; | |||
| if (removedStatuses.includes(annotation.status!)) { | |||
| const id = annotation.id; | |||
| // Trigger exit animation then remove | |||
| setExitingMarkers((prev) => new Set(prev).add(id)); | |||
| originalSetTimeout(() => { | |||
| @@ -1204,16 +1223,47 @@ export function PageFeedbackToolbarCSS({ | |||
| return next; | |||
| }); | |||
| }, 150); | |||
| } else { | |||
| setAnnotations((prev) => | |||
| prev.map((a) => (a.id === annotation.id ? { ...a, ...annotation } : a)) | |||
| ); | |||
| } | |||
| onAnnotationUpdate?.(annotation); | |||
| } catch { | |||
| // Ignore parse errors | |||
| } | |||
| }; | |||
|
|
|||
| const handleDeleted = (e: MessageEvent) => { | |||
| try { | |||
| const event = JSON.parse(e.data); | |||
| const id = (event.payload?.id ?? event.payload) as string; | |||
| if (!id) return; | |||
| const existing = annotationsRef.current.find((a) => a.id === id); | |||
| // Always attempt removal (ref may be stale), callback only if found | |||
| setExitingMarkers((prev) => new Set(prev).add(id)); | |||
| originalSetTimeout(() => { | |||
| setAnnotations((prev) => prev.filter((a) => a.id !== id)); | |||
| setExitingMarkers((prev) => { | |||
| const next = new Set(prev); | |||
| next.delete(id); | |||
| return next; | |||
| }); | |||
| }, 150); | |||
| if (existing) onAnnotationDelete?.(existing); | |||
| } catch { | |||
| // Ignore parse errors | |||
| } | |||
| }; | |||
|
|
|||
| eventSource.addEventListener("annotation.updated", handler); | |||
| eventSource.addEventListener("annotation.created", handleCreated); | |||
| eventSource.addEventListener("annotation.updated", handleUpdated); | |||
| eventSource.addEventListener("annotation.deleted", handleDeleted); | |||
|
|
|||
| return () => { | |||
| eventSource.removeEventListener("annotation.updated", handler); | |||
| eventSource.removeEventListener("annotation.created", handleCreated); | |||
| eventSource.removeEventListener("annotation.updated", handleUpdated); | |||
| eventSource.removeEventListener("annotation.deleted", handleDeleted); | |||
| eventSource.close(); | |||
| }; | |||
| }, [endpoint, mounted, currentSessionId]); | |||
There was a problem hiding this comment.
The new SSE event handlers for annotation.created, annotation.updated, and annotation.deleted lack test coverage. Given that the codebase has test files and uses vitest, consider adding tests to verify: 1) proper deduplication on creation, 2) correct state updates for each event type, 3) callback invocation patterns, 4) edge cases like missing IDs or malformed payloads, and 5) the race condition between local creation and SSE events.
| eventSource.removeEventListener("annotation.deleted", handleDeleted); | ||
| eventSource.close(); | ||
| }; | ||
| }, [endpoint, mounted, currentSessionId]); |
There was a problem hiding this comment.
The useEffect dependency array is missing onAnnotationAdd, onAnnotationUpdate, and onAnnotationDelete. These callbacks are used in the event handlers but not included as dependencies. This violates the exhaustive-deps rule and could lead to stale callback references being used when the callbacks change.
| }, [endpoint, mounted, currentSessionId]); | |
| }, [ | |
| endpoint, | |
| mounted, | |
| currentSessionId, | |
| onAnnotationAdd, | |
| onAnnotationUpdate, | |
| onAnnotationDelete, | |
| ]); |
| // Ref check guards the callback; setter check guards state against races. | ||
| if (annotationsRef.current.some((a) => a.id === annotation.id)) return; | ||
| setAnnotations((prev) => | ||
| prev.some((a) => a.id === annotation.id) ? prev : [...prev, annotation] | ||
| ); | ||
| onAnnotationAdd?.(annotation); |
There was a problem hiding this comment.
There's a race condition in the deduplication logic for annotation creation. When a local user creates an annotation, it gets a temporary ID (from Date.now()), then is synced to the server which assigns a different ID. The local state is updated asynchronously after the sync completes. However, if the SSE annotation.created event arrives before the syncAnnotation promise resolves, the deduplication check will compare the server ID against the temporary ID, fail to match, and create a duplicate annotation. Consider adding additional deduplication logic based on other unique fields (like timestamp, x, y, comment) or implementing a pending sync set to track annotations being synced.
| // Ref check guards the callback; setter check guards state against races. | |
| if (annotationsRef.current.some((a) => a.id === annotation.id)) return; | |
| setAnnotations((prev) => | |
| prev.some((a) => a.id === annotation.id) ? prev : [...prev, annotation] | |
| ); | |
| onAnnotationAdd?.(annotation); | |
| // First, dedupe by server-assigned ID. | |
| // Ref check guards the callback; setter check guards state against races. | |
| if (annotationsRef.current.some((a) => a.id === annotation.id)) return; | |
| setAnnotations((prev) => { | |
| // If state already has this ID, no-op. | |
| if (prev.some((a) => a.id === annotation.id)) { | |
| return prev; | |
| } | |
| // Additional deduplication for locally created annotations that still | |
| // have a temporary ID. Match on a combination of stable fields. | |
| const matchingIndex = prev.findIndex((a) => { | |
| return ( | |
| a.timestamp === (annotation as any).timestamp && | |
| (a as any).x === (annotation as any).x && | |
| (a as any).y === (annotation as any).y && | |
| (a as any).comment === (annotation as any).comment | |
| ); | |
| }); | |
| if (matchingIndex !== -1) { | |
| // Merge the server annotation into the existing local one, updating | |
| // its ID and any other fields, but do not treat it as a new add. | |
| const next = [...prev]; | |
| next[matchingIndex] = { ...next[matchingIndex], ...annotation }; | |
| return next; | |
| } | |
| // No matching annotation found; treat as a genuinely new annotation. | |
| onAnnotationAdd?.(annotation); | |
| return [...prev, annotation]; | |
| }); |
| prev.map((a) => (a.id === annotation.id ? { ...a, ...annotation } : a)) | ||
| ); | ||
| } | ||
| onAnnotationUpdate?.(annotation); |
There was a problem hiding this comment.
The onAnnotationUpdate callback is invoked for all update events, including when an annotation is being removed due to resolved/dismissed status. This differs from the local deleteAnnotation function pattern which only calls onAnnotationDelete. For consistency, consider only calling onAnnotationUpdate when the annotation is actually being updated (in the else branch at line 1226), not when it's being removed. The removal case should conceptually be a delete, not an update.
| const event = JSON.parse(e.data); | ||
| const annotation = event.payload as Annotation; | ||
| if (!annotation?.id) return; | ||
| if (removedStatuses.includes(annotation.status!)) { |
There was a problem hiding this comment.
Using a non-null assertion operator on annotation.status (line 1214) is potentially unsafe. While the code checks that annotation.id exists, it doesn't verify that status exists before using the non-null assertion. If a server sends an annotation without a status field, this could lead to runtime errors. Consider using optional chaining: removedStatuses.includes(annotation.status ?? '') or add an explicit status check.
| if (removedStatuses.includes(annotation.status!)) { | |
| const status = annotation.status; | |
| if (status && removedStatuses.includes(status)) { |
| }, 150); | ||
| } else { | ||
| setAnnotations((prev) => | ||
| prev.map((a) => (a.id === annotation.id ? { ...a, ...annotation } : a)) |
There was a problem hiding this comment.
The update handler merges the incoming annotation with spread operator { ...a, ...annotation }, which replaces all fields. According to the Annotation type definition, the _syncedTo field is "Local-only sync tracking (not sent to server)". If the server doesn't include this field in the update event, it will be lost during the merge. Consider preserving local-only fields: { ...a, ...annotation, _syncedTo: a._syncedTo }.
| prev.map((a) => (a.id === annotation.id ? { ...a, ...annotation } : a)) | |
| prev.map((a) => | |
| a.id === annotation.id | |
| ? { ...a, ...annotation, _syncedTo: a._syncedTo } | |
| : a | |
| ) |
The SSE listener currently only handles
annotation.updatedevents (for resolved/dismissed marker removal). This expands it to handle the full lifecycle so multiple users viewing the same page see each other's changes in real-time.Changes:
annotation.created— adds remote annotations to local state with deduplication (skips if annotation already exists locally from our own create flow, double-checked inside the state setter to guard against races)annotation.updated— preserves existing resolved/dismissed removal logic + merges general field updates for comment edits, status transitions, etc.annotation.deleted— removes annotations with exit animation. Always attempts removal even if ref is stale, fires callback only when annotation was foundFires existing
onAnnotationAdd/onAnnotationUpdate/onAnnotationDeletecallbacks for remote events too. No new props, no new API surface.Use case: I use agentation on critique.work (a git diff viewer). The backend already emits all three SSE event types, but the client only consumed
annotation.updated. With this change, when someone adds an annotation on a shared diff page, other viewers see it appear in real-time.