feat(websocket): Phase 1 & 2 — replace SSE notification count with WebSocket#36965
feat(websocket): Phase 1 & 2 — replace SSE notification count with WebSocket#36965mohammad-rj wants to merge 40 commits intogo-gitea:mainfrom
Conversation
Add a thin in-memory pubsub broker and a SharedWorker-based WebSocket client to deliver real-time notification count updates. This replaces the SSE path for notification-count events with a persistent WebSocket connection shared across all tabs. New files: - services/pubsub/broker.go: fan-out pubsub broker (DefaultBroker singleton) - services/websocket/notifier.go: polls notification counts, publishes to broker - routers/web/websocket/websocket.go: /-/ws endpoint, per-user topic subscription - web_src/js/features/websocket.sharedworker.ts: SharedWorker with exponential backoff reconnect (50ms initial, 10s max, reconnect on close and error) Modified files: - routers/init.go: register websocket_service.Init() - routers/web/web.go: add GET /-/ws route - services/context/response.go: add Hijack() to forward http.Hijacker so coder/websocket can upgrade the connection - web_src/js/features/notification.ts: port from SSE SharedWorker to WS SharedWorker - webpack.config.ts: add websocket.sharedworker entry point Part of RFC go-gitea#36942.
Replace timeutil.TimeStampNow() calls in the websocket notifier with a nowTS() helper that reads time.Now().Unix() directly. TimeStampNow reads a package-level mock variable that TestIncomingEmail writes concurrently, causing a race detected by the race detector in test-pgsql CI.
assets/go-licenses.json was missing the license entry for the newly added github.com/coder/websocket dependency. Running make tidy regenerates this file via build/generate-go-licenses.go.
There was a problem hiding this comment.
Pull request overview
Implements Phase 1 of the SSE → WebSocket migration for real-time notification count updates by introducing a WebSocket endpoint plus a SharedWorker client, backed by an in-memory fan-out pubsub broker.
Changes:
- Add an in-memory pubsub broker (
DefaultBroker) and a polling notifier that publishes{"type":"notification-count","count":N}per user. - Add
GET /-/wsWebSocket endpoint that authenticates the signed-in user and streams broker messages. - Add a WebSocket SharedWorker entry + client wiring to replace the SSE SharedWorker path for notification count updates.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
services/pubsub/broker.go |
New in-memory fan-out pubsub broker used to broadcast per-user events |
services/websocket/notifier.go |
Polls notification counts and publishes JSON events to the broker |
routers/web/websocket/websocket.go |
Implements /-/ws WebSocket upgrade + message forwarding from broker |
services/context/response.go |
Adds Hijack() forwarding to support WebSocket upgrades |
routers/init.go |
Registers the new websocket service init |
routers/web/web.go |
Registers the new GET /-/ws route |
web_src/js/features/websocket.sharedworker.ts |
New SharedWorker maintaining one WS connection per URL with reconnect/backoff |
web_src/js/features/notification.ts |
Switch notification count updates from SSE worker to WS worker |
webpack.config.ts |
Adds the SharedWorker entrypoint to the build |
go.mod / go.sum |
Adds github.com/coder/websocket as a direct dependency |
assets/go-licenses.json |
Adds the license entry for github.com/coder/websocket |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Move /-/ws route inside reqSignIn middleware group; remove manual ctx.IsSigned check from handler (auth is now enforced by the router) - Fix scheduleReconnect() to schedule using current delay then double, so first reconnect fires after 50ms not 100ms (reported by silverwind) - Replace sourcesByPort.set(port, null) with delete() to prevent MessagePort retention after tab close (memory leak fix) - Centralize topic naming in pubsub.UserTopic() — removes duplication between the notifier and the WebSocket handler - Skip DB polling in notifier when broker has no active subscribers to avoid unnecessary load on idle instances - Hold RLock for the full Publish fan-out loop to prevent a race where cancel() closes a channel between slice read and send
|
All review feedback addressed in the latest commit:
Re the sharedworker consolidation (#36896): understood, will rebase and merge the WS logic into the single |
reqSignIn sends a 303 redirect which breaks WebSocket upgrade; use the same pattern as /user/events: register the route without middleware and return 401 inside the handler when the user is not signed in. Also fix copyright year to 2026 in all three new Go files and add a console.warn for malformed JSON in the SharedWorker.
… worker
- Remove `export {}` which caused webpack to tree-shake the entire
SharedWorker bundle, resulting in an empty JS file with no connect
handler — root cause of WebSocket never opening
- Rename SharedWorker instance from 'notification-worker' to
'notification-worker-ws' to force browser to create a fresh worker
instance instead of reusing a cached empty one
Please click "resolve" on the resolved review comments above :) |
- Add export{} to declare websocket.sharedworker.ts as an ES module,
preventing TypeScript TS2451 redeclaration errors caused by global
scope conflicts with eventsource.sharedworker.ts
- Always delete port from sourcesByPort on close regardless of remaining
subscriber count, preventing MessagePort keys from leaking in the Map
Add a thin in-memory pubsub broker and a SharedWorker-based WebSocket client to deliver real-time notification count updates. This replaces the SSE path for notification-count events with a persistent WebSocket connection shared across all tabs. New files: - services/pubsub/broker.go: fan-out pubsub broker (DefaultBroker singleton) - services/websocket/notifier.go: polls notification counts, publishes to broker - routers/web/websocket/websocket.go: /-/ws endpoint, per-user topic subscription - web_src/js/features/websocket.sharedworker.ts: SharedWorker with exponential backoff reconnect (50ms initial, 10s max, reconnect on close and error) Modified files: - routers/init.go: register websocket_service.Init() - routers/web/web.go: add GET /-/ws route - services/context/response.go: add Hijack() to forward http.Hijacker so coder/websocket can upgrade the connection - web_src/js/features/notification.ts: port from SSE SharedWorker to WS SharedWorker - webpack.config.ts: add websocket.sharedworker entry point Part of RFC go-gitea#36942.
Replace timeutil.TimeStampNow() calls in the websocket notifier with a nowTS() helper that reads time.Now().Unix() directly. TimeStampNow reads a package-level mock variable that TestIncomingEmail writes concurrently, causing a race detected by the race detector in test-pgsql CI.
assets/go-licenses.json was missing the license entry for the newly added github.com/coder/websocket dependency. Running make tidy regenerates this file via build/generate-go-licenses.go.
- Move /-/ws route inside reqSignIn middleware group; remove manual ctx.IsSigned check from handler (auth is now enforced by the router) - Fix scheduleReconnect() to schedule using current delay then double, so first reconnect fires after 50ms not 100ms (reported by silverwind) - Replace sourcesByPort.set(port, null) with delete() to prevent MessagePort retention after tab close (memory leak fix) - Centralize topic naming in pubsub.UserTopic() — removes duplication between the notifier and the WebSocket handler - Skip DB polling in notifier when broker has no active subscribers to avoid unnecessary load on idle instances - Hold RLock for the full Publish fan-out loop to prevent a race where cancel() closes a channel between slice read and send
reqSignIn sends a 303 redirect which breaks WebSocket upgrade; use the same pattern as /user/events: register the route without middleware and return 401 inside the handler when the user is not signed in. Also fix copyright year to 2026 in all three new Go files and add a console.warn for malformed JSON in the SharedWorker.
… worker
- Remove `export {}` which caused webpack to tree-shake the entire
SharedWorker bundle, resulting in an empty JS file with no connect
handler — root cause of WebSocket never opening
- Rename SharedWorker instance from 'notification-worker' to
'notification-worker-ws' to force browser to create a fresh worker
instance instead of reusing a cached empty one
… starts The stopwatch navbar icon and popup were only rendered by the server when a stopwatch was already active at page load. If a tab was opened before the stopwatch started, `initStopwatch()` found no `.active-stopwatch` element in the DOM, returned early, and never registered a SharedWorker listener. As a result the WebSocket push from the stopwatch notifier had nowhere to land and the icon never appeared. Fix by always rendering both the icon anchor and the popup skeleton in the navbar (hidden with `tw-hidden` when no stopwatch is active). `initStopwatch()` can now set up the SharedWorker in every tab, and `updateStopwatchData` can call `showElem`/`hideElem` as stopwatch state changes arrive in real time. Also add `onShow` to `createTippy` so the popup content is re-cloned from the (JS-updated) original each time the tooltip opens, keeping it current even when the stopwatch was started after page load. Add a new e2e test (`stopwatch appears via real-time push`) that verifies the icon appears after `apiStartStopwatch` is called with the page already loaded.
…bar popup The navbar popup stop/cancel forms used form-fetch-action, which always reloads the page when the server returns an empty redirect. Remove that class and add a dedicated delegated submit handler in stopwatch.ts that POSTs the action silently; the WebSocket push (or periodic poller) then updates the icon without any navigation.
… sidebar Replace link-action buttons in the sidebar time-tracker with plain buttons handled by a delegated click listener in stopwatch.ts. After a successful POST the two button groups (.issue-start-buttons / .issue-stop-cancel-buttons) are toggled immediately via show/hide, so the page never reloads. The navbar icon continues to update via the WebSocket push or periodic poller as before.
The shared worker no longer emits 'no-event-source' messages since the EventSource transport was removed. Clean up the unreachable branches in both notification.ts and stopwatch.ts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This Gitea Actions workflow was used for local compliance testing on git.epid.co and should not be part of the upstream PR.
…i-phase2-push # Conflicts: # routers/init.go # routers/web/web.go # routers/web/websocket/websocket.go # web_src/js/features/eventsource.sharedworker.ts
# Conflicts: # templates/base/head_navbar_icons.tmpl
Done — Phase 2 is now pushed. SSE is fully removed: stopwatches, logout, and notification count all go through WebSocket now. The Also fixed a pre-existing issue where the stopwatch icon only appeared in the tab that started the timer — now it shows in all tabs via WS push. The e2e tests cover: notification count, stopwatch at page load, stopwatch via real-time push, and logout propagation. |
|
FYI htmx removal is in progress at #37079, don't think you have any touching points with it here but I recall it was mentioned during the initial discussion. |
Thanks for the heads up. Checked, no htmx usage in our changes. |
…ous connections - Merge Init and InitStopwatch into a single Init() so the service has one entry point; remove the separate mustInit call from routers/init.go - Add NotificationCountChange to the notify.Notifier interface and implement it in a new wsNotifier; uinotification calls it after each DB write so connected clients get an immediate push instead of waiting for the next polling tick - Replace PublishEmptyStopwatches with PublishStopwatchesForUser which fetches and publishes the current list; call it on start/stop/cancel so all open tabs update without waiting for the next tick - Accept anonymous WebSocket connections (remove 401 early-return); only signed-in users subscribe to user-specific topics, leaving the endpoint ready for future public event types - Add routing.MarkLongPolling at the start of Serve() so the request logger treats WS connections consistently with SSE
| // pushes it immediately to all connected WebSocket clients, bypassing the | ||
| // periodic polling loop for this specific user. | ||
| func (n *wsNotifier) NotificationCountChange(ctx context.Context, userID int64) { | ||
| count, err := db.Count[activities_model.Notification](ctx, activities_model.FindNotificationOptions{ |
There was a problem hiding this comment.
It's better to check whether the user is in the connection pool, otherwise we don't need to do anything.
There was a problem hiding this comment.
The pub/sub system should not behave like a traditional persistent pub/sub model. If an event is fired when no clients are subscribed, it should be discarded immediately rather than stored and delivered later when a client comes online. It should function as a runtime-only pub/sub system. When a new client connects, the page should load the latest state directly from the database, so replaying missed events is unnecessary.
| } else if (event.data.type === 'listen') { | ||
| const source = sourcesByPort.get(port)!; | ||
| source.listen(event.data.eventType); | ||
| const wsUrl = url.replace(/^http/, 'ws').replace(/\/user\/events$/, '/-/ws'); |
There was a problem hiding this comment.
Use this.wsUrl instead of clumsy replace?
| }); | ||
|
|
||
| test('stopwatch appears via real-time push', async ({page, request}) => { | ||
| const name = `ev-sw-push-${Date.now()}-${Math.random().toString(36).slice(2, 6)}`; |
|
|
||
| // Start stopwatch after page is loaded — icon should appear via WebSocket push | ||
| await apiStartStopwatch(request, name, name, 1, {headers}); | ||
| await expect(stopwatch).toBeVisible({timeout: 15000}); |
There was a problem hiding this comment.
15000 * timeoutFactor, check if it actually needs to be that high. The number 15000 should be roughly 3 times what it takes on a modern machine, so if it takes 5000 on your machine, it is correct.
Summary
Completes the SSE → WebSocket migration proposed in #36942.
Phase 1 — Infrastructure + notification count
services/pubsub/broker.goDefaultBrokersingletonservices/websocket/notifier.goGetUIDsAndNotificationCounts, publishes{"type":"notification-count","count":N}to broker. RegisterswsNotifiersoNotificationCountChangefires immediately on DB writeservices/websocket/notification_notifier.gonotify.Notifier(NotificationCountChange); called byuinotificationafter each DB write for targeted immediate pushrouters/web/websocket/websocket.go/-/wsendpoint — accepts anonymous and signed-in connections; signed-in users subscribe touser-{id}topic, anonymous users stay connected for future public event typesrouters/init.gowebsocket_service.Init()(replaces separate Init + InitStopwatch calls)routers/web/web.goGET /-/wsrouteweb_src/js/features/eventsource.sharedworker.tsWsSourceclass alongside existingSource— one connection per origin shared across tabs, exponential backoff reconnect (50ms → 10s max)services/notify/notifier.goNotificationCountChange(ctx, userID)toNotifierinterfaceservices/notify/null.goNotificationCountChangetoNullNotifierservices/notify/notify.goNotificationCountChangedispatch functionservices/uinotification/notify.goNotificationCountChangeafter DB write whenReceiverID != 0Phase 2 — Remove remaining SSE, complete migration
New files:
services/websocket/stopwatch_notifier.goPublishStopwatchesForUserfor immediate push on start/stop/cancelservices/websocket/logout_publisher.gotests/e2e/events.test.tsModified files:
routers/web/websocket/websocket.gorouting.MarkLongPolling; addrewriteLogout— rewrites raw session ID to"here"/"elsewhere"routers/web/events/events.gorouters/web/repo/issue_stopwatch.goPublishStopwatchesForUseron start/stop/cancel for immediate push to all open tabsrouters/web/web.go/user/eventsSSE routerouters/common/blockexpensive.go/user/eventsto/-/wsrouters/common/qos.gorouters/web/auth/auth.goeventsourceimport withwebsocketfor logoutservices/user/user.gomodules/eventsource/(5 files)tests/integration/eventsource_test.goweb_src/js/features/eventsource.sharedworker.tsEventSource, keep onlyWsSourcefor WebSocketweb_src/js/features/stopwatch.tsUserEventsSharedWorkerfor WS events; silent POST handlers for navbar popup and issue sidebarweb_src/js/features/notification.tsUserEventsSharedWorkerinstead of raw SharedWorkerweb_src/js/modules/worker.tsUserEventsSharedWorkerclass — shared abstraction for notification and stopwatch listenersStopwatch multi-tab fix
The stopwatch icon previously only appeared in the tab where the timer was started. This was because the navbar icon and popup were conditionally rendered with
{{if $activeStopwatch}}, so tabs loaded before the stopwatch started had no DOM element for the JS to update.templates/base/head_navbar_icons.tmpl.active-stopwatchwithtw-hiddenwhen inactivetemplates/base/head_navbar.tmpl.active-stopwatch-popupoutside conditionaltemplates/repo/issue/sidebar/stopwatch_timetracker.tmpltw-hiddenweb_src/js/features/stopwatch.tsonShowre-clones popup content; delegated handlers for silent POST (no page reload on start/stop/cancel)Design decisions
WsSourcelives insideeventsource.sharedworker.ts. One connection per origin, shared across all tabs.github.com/coder/websocket— lightweight, actively maintained WebSocket library.NotificationCountChangehook — targeted immediate push on DB write; the periodic poller acts as fallback for theReceiverID==0(all-watchers) case.What's removed
modules/eventsource— stopwatch polling, logout broadcasting, SSE manager run loop/user/eventsSSE routetests/integration/eventsource_test.goeventsource.sharedworker.tsEventSource class (only WsSource remains)AI disclosure
Parts of this PR were developed with AI assistance (Claude).
Closes #36942.