feat: replace notification polling with WebSocket push#2199
feat: replace notification polling with WebSocket push#2199
Conversation
|
auggie review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2d4dd0e06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🤖 Augment PR SummarySummary: This PR replaces periodic notification polling with a WebSocket-based push channel so new notifications can appear immediately. Changes:
Technical Notes: Server WebSockets are implemented using 🤖 Was this summary useful? React with 👍 or 👎 |
6551de8 to
5a5c56d
Compare
|
auggie review |
06626fe to
dfeddc2
Compare
Connections start unauthenticated. Client must send an auth message with a JWT token as the first message. Only after successful auth can the client subscribe/unsubscribe to topics.
Dispatches a NotificationCreatedEvent after saving to DB. The WebSocket listener picks it up and pushes to connected clients subscribed to the notifications topic.
Add license headers, fix unused parameter, use assert.Empty
Notifications are now pushed in real-time via WebSocket. Initial state is still loaded via REST on mount. Falls back to polling when WebSocket is not connected.
Use the configured CORS origins for WebSocket origin verification, fixing the upgrade failure when the frontend dev server runs on a different port than the API.
The local authTypeLinkShare constant was 1, but the canonical AuthTypeLinkShare in the auth package is 2 (AuthTypeUser is 1). This caused valid user tokens to be rejected. Use the auth package constant directly and check for AuthTypeUser instead.
When auth fails, the error was put on the send channel but ReadLoop closed the connection before WriteLoop could drain it. Write the error directly to the websocket so the client receives it.
The notification content was stored as []byte, which Go's json.Marshal base64-encodes. Using json.RawMessage instead embeds it as raw JSON in the WebSocket event payload.
When the event system hasn't been initialized (e.g. in feature tests that don't call events.Fake()), Dispatch would panic on a nil pubsub. Return early with a debug log instead.
Prevents unbounded growth of the connections map over long runtimes by cleaning up the userID key when all connections for that user have been unregistered.
Return 503 if the global hub hasn't been initialized instead of panicking later when Register/Unregister is called.
When either loop exits (e.g. write/ping failure), the shared context is cancelled so the other loop also stops promptly. This prevents orphaned connections staying registered in the hub when only the writer fails.
Previously only authenticated was set to false while connected stayed true, causing callers to think the WebSocket was functional. Now the socket is closed immediately on auth errors so connected reflects the actual state and fallback polling can kick in.
- Wrap initial loadNotifications in try/catch so WS subscription and fallback polling are still set up even if the REST call fails - Reload notifications via REST when WebSocket disconnects to catch events that may have been missed during the disconnect window
Move API token lookup, expiry check, and user resolution into a shared ValidateAPITokenString function in pkg/modules/auth. Both the HTTP middleware and WebSocket auth now use this shared function, ensuring token validity is checked consistently everywhere. This also fixes the WebSocket path previously not checking API token expiry.
…ping reconnects - Strip trailing slashes from window.API_URL before appending /ws to avoid producing double-slash URLs like .../api/v1//ws - Clear any pending reconnect timer before scheduling a new one to prevent overlapping reconnect attempts during flappy connections
…h watch errors - Only poll for notifications when the tab is visible, restoring the previous behavior that avoided background requests every 10s - Catch errors in the wsConnected watch callback to prevent unhandled promise rejections when the REST reload fails
The DB notification is already committed at this point, so returning a Dispatch error would make upstream operations appear to fail even though the notification was persisted. Log the error instead.
Add a 30s auth deadline so that upgraded-but-never-authenticated connections don't keep read/write goroutines alive indefinitely, preventing potential resource exhaustion from abandoned connections.
dfeddc2 to
584fca1
Compare
Preview DeploymentPreview deployments for this PR are available at:
The preview environment will start automatically on first visit. Subsequent pushes to this PR will update the Run locally with Dockerdocker pull ghcr.io/go-vikunja/vikunja:pr-2199
docker run -p 3456:3456 ghcr.io/go-vikunja/vikunja:pr-2199Last updated for commit 584fca1 |
Summary
/api/v1/wswith auth-after-connect (client sends JWT as first message)pkg/websocket/package with connection hub, per-connection read/write goroutines, and topic-based pub/subNotificationCreatedEventafter DB insert, picked up by a Watermill listener that fans out to connected WebSocket clientsuseWebSocketcomposable manages a single multiplexed connection with exponential backoff reconnectNotifications.vueswitches from 10s polling to real-time WebSocket push, with polling as fallback when WS is unavailableTest plan
GET /notificationswhile WebSocket is connectedmage lintandcd frontend && pnpm lint— no new issuesgo test ./pkg/websocket/...— all 12 tests pass