fix(realtime): post-review polish from final branch review
Five fixes surfaced by the branch-wide code review on the realtime layer:
- server.ts: replace dynamic `import("@repo/auth/di/container")` with a
static top-of-file import. The dynamic-import workaround from 6a0ac63 is
no longer needed once the root tsconfig + TSX_TSCONFIG_PATH expose
decorator metadata to tsx; verified by booting `pnpm dev` clean.
- server.ts: correct the inline structural type for `validateSession` to
match the real `IAuthenticationService` contract (non-nullable, throws
on invalid session) and wrap the call in try/catch so unauthenticated
bubbles to a `null` return instead of dead-code `result ? ... : null`.
- bind-production.ts: extract `maybeRegisterRealtimePing()` that wraps the
built-in ping inbound handler in the same `withSpan(withCapture(...))`
sandwich the realtime-handler generator emits (R41–R44), so the
proof-of-life channel models the convention rather than registering raw.
- bind-production.test.ts: add 4 tests for the `REALTIME_PING_DISABLED`
env-gate (registered when unset in both binders, not registered when
"true", treated as enabled when "1").
- docs/guides/realtime.md: correct the integration-test reference at
line 285 — the test does not call `bindAllDevSeed()`; it builds the
Socket.IO server inline and exercises gates 1+2 only (gates 3+4 live in
socket-io-realtime-server.test.ts).
- adr-016: add a "Known follow-ups" section recording 6 lower-priority
refinements deferred from this branch (bridge stub test scaffolding,
registry register/registerChannel precedence, channel-template dot
constraint, server bare catch{}, BindAllDeps Partial widening, AGENTS.md
anchor count phrasing).
CI gates: lint 0 errors / 4 warnings (pre-existing turbo.json warnings),
typecheck clean, 24 web-next tests pass (was 20; 4 new env-gate tests),
boundaries 0 issues across 504 files.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -84,6 +84,17 @@ Handlers are wrapped in the same `withSpan(tracer, { op: "realtime-handler" }, w
|
||||
4. **Custom Node server for `cms` and `web-tanstack`.** Both apps continue on their existing runtimes until they need realtime.
|
||||
5. **Production-mode e2e test.** The `realtime-ping` integration test exercises the four checkpoints in-process. A multi-socket test that verifies fan-out across N connected clients is deferred to v2.
|
||||
|
||||
## Known follow-ups (post-merge polish)
|
||||
|
||||
Items surfaced by the final branch review that were intentionally not landed in v1:
|
||||
|
||||
1. **`bindRealtimeBridge` stub has no test scaffolding.** v1 ships `apps/web-next/src/server/bind-production.ts:bindRealtimeBridge` as a no-op (`_`-prefixed args). The dashboard PR adds the first allowlist entries; a minimal contract-shaped test (e.g. accept `allowlist: BridgeEntry[]` and assert subscribe wiring) should land alongside the first entry, not before.
|
||||
2. **`IRealtimeHandlerRegistry.register` + `registerChannel` precedence is implicit.** Calling both for the same channel name silently overwrites the channel-map descriptor while leaving the entry-map intact. Behaviour is correct for current callers; document the precedence in the interface JSDoc or reject conflicting re-registration once the second outbound channel ships.
|
||||
3. **`matchChannelTemplate` placeholders cannot contain dots** (`packages/core-realtime/src/channel-template.ts:14-17` uses `([^.]+)`). Fine for UUID-style identifiers; document the constraint in `defineRealtimeChannel`'s JSDoc when the first non-UUID key shape arrives.
|
||||
4. **`SocketIORealtimeServer` swallows handler errors with bare `catch {}`** (`packages/core-realtime/src/socket-io-realtime-server.ts:108-117`). Wrapped handlers (`withCapture`) already record the error; unwrapped handlers lose it. Adding a server-injected logger that records "handler_error for channel X" would help debug connection-level issues — defer until a debugging incident actually motivates it.
|
||||
5. **`bindAll(deps?: Partial<BindAllDeps>)` permits a half-populated deps object** that mixes a real broadcaster with a fresh registry (or vice versa). In practice no caller does this, but the type doesn't enforce all-or-nothing semantics. Tighten to `deps?: BindAllDeps` (full or absent) when the next consumer lands.
|
||||
6. **AGENTS.md anchor count phrasing.** AGENTS.md says "three fixed `// <gen:realtime-*>` anchor comments per feature." There are three *kinds* but four placements (the handlers anchor lives in both `bind-production.ts` and `bind-dev-seed.ts`). Tighten to "three anchor kinds across both bind files" when the AGENTS.md is next touched.
|
||||
|
||||
## Related
|
||||
|
||||
- ADR-008 — per-feature DI containers
|
||||
|
||||
Reference in New Issue
Block a user