From f8013451de1fe291aed12ae3de17a55a3f7ddde6 Mon Sep 17 00:00:00 2001 From: Danijel Martinek Date: Sat, 9 May 2026 10:16:46 +0200 Subject: [PATCH] fix(realtime): post-review polish from final branch review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- apps/web-next/server.ts | 35 +++++++++------- .../src/server/bind-production.test.ts | 42 +++++++++++++++++++ apps/web-next/src/server/bind-production.ts | 34 ++++++++++++--- docs/decisions/adr-016-realtime-layer.md | 11 +++++ docs/guides/realtime.md | 2 +- 5 files changed, 101 insertions(+), 23 deletions(-) diff --git a/apps/web-next/server.ts b/apps/web-next/server.ts index 9a4959b..86f7ba1 100644 --- a/apps/web-next/server.ts +++ b/apps/web-next/server.ts @@ -11,9 +11,17 @@ import { type IRealtimeAuthenticator, } from "@repo/core-realtime"; import { SESSION_COOKIE } from "@repo/auth"; +import { authContainer } from "@repo/auth/di/container"; import { AUTH_SYMBOLS } from "@repo/auth/di/symbols"; import { bindAll } from "./src/server/bind-production.js"; +// Real shape of IAuthenticationService.validateSession: returns non-nullable +// on success and throws UnauthenticatedError on missing/invalid sessions. +// Kept as an inline structural type to avoid leaking auth's internal interface. +type AuthService = { + validateSession: (id: string) => Promise<{ user: { id: string }; session: unknown }>; +}; + const dev = process.env.NODE_ENV !== "production"; const port = Number(process.env.PORT ?? 3000); @@ -34,22 +42,17 @@ const authenticator: IRealtimeAuthenticator = { authenticate: async ({ cookies }) => { const sessionId = cookies[SESSION_COOKIE]; if (!sessionId) return null; - // Lazy-import the auth container after bindAll() has already populated it. - // Dynamic import defers tsx's module transformation to runtime, which lets - // reflect-metadata and the DI decorators resolve correctly in this server - // entry point. - const { authContainer } = await import("@repo/auth/di/container"); - const authService = authContainer.get<{ - validateSession: (id: string) => Promise<{ user: { id: string }; session: unknown } | null>; - }>(AUTH_SYMBOLS.IAuthenticationService); - const result = await authService.validateSession(sessionId); - return result - ? { - userId: result.user.id, - // Roles are not yet in the session shape; extend here when DB-backed roles ship. - roles: (result as unknown as { roles?: string[] }).roles ?? [], - } - : null; + const authService = authContainer.get(AUTH_SYMBOLS.IAuthenticationService); + try { + const { user } = await authService.validateSession(sessionId); + // Roles are not yet in the session shape; extend here when DB-backed roles ship. + return { userId: user.id, roles: (user as { roles?: string[] }).roles ?? [] }; + } catch { + // Invalid/expired session → reject the connection. Real auth-service errors + // (DB outages etc.) intentionally collapse to "unauthenticated" here too, + // which is the conservative choice for a public-facing socket. + return null; + } }, }; diff --git a/apps/web-next/src/server/bind-production.test.ts b/apps/web-next/src/server/bind-production.test.ts index c6112c6..7453d70 100644 --- a/apps/web-next/src/server/bind-production.test.ts +++ b/apps/web-next/src/server/bind-production.test.ts @@ -192,6 +192,48 @@ describe("bindAll dispatcher", () => { }); }); +describe("realtime-ping registration (REALTIME_PING_DISABLED env-gate)", () => { + beforeEach(() => { + vi.resetModules(); + vi.clearAllMocks(); + vi.unstubAllEnvs(); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("registers realtime.ping when REALTIME_PING_DISABLED is unset (production)", async () => { + const { bindAllProduction } = await import("./bind-production"); + const deps = makeDeps(); + await bindAllProduction(deps); + expect(deps.realtimeRegistry.listChannels().map((d) => d.name)).toContain("realtime.ping"); + }); + + it("registers realtime.ping when REALTIME_PING_DISABLED is unset (dev seed)", async () => { + const { bindAllDevSeed } = await import("./bind-production"); + const deps = makeDeps(); + await bindAllDevSeed(deps); + expect(deps.realtimeRegistry.listChannels().map((d) => d.name)).toContain("realtime.ping"); + }); + + it("does NOT register realtime.ping when REALTIME_PING_DISABLED='true'", async () => { + vi.stubEnv("REALTIME_PING_DISABLED", "true"); + const { bindAllProduction } = await import("./bind-production"); + const deps = makeDeps(); + await bindAllProduction(deps); + expect(deps.realtimeRegistry.listChannels().map((d) => d.name)).not.toContain("realtime.ping"); + }); + + it("treats REALTIME_PING_DISABLED='1' as not-disabled (only literal 'true' disables)", async () => { + vi.stubEnv("REALTIME_PING_DISABLED", "1"); + const { bindAllDevSeed } = await import("./bind-production"); + const deps = makeDeps(); + await bindAllDevSeed(deps); + expect(deps.realtimeRegistry.listChannels().map((d) => d.name)).toContain("realtime.ping"); + }); +}); + describe("bindAll instrumentation orthogonality (Rule 0, R47)", () => { beforeEach(() => { vi.resetModules(); diff --git a/apps/web-next/src/server/bind-production.ts b/apps/web-next/src/server/bind-production.ts index b5b135a..3368fdf 100644 --- a/apps/web-next/src/server/bind-production.ts +++ b/apps/web-next/src/server/bind-production.ts @@ -7,6 +7,8 @@ import config from "@repo/core-cms"; import { bindNoopInstrumentation, bindSentryInstrumentation, + withCapture, + withSpan, type ITracer, type ILogger, } from "@repo/core-shared/instrumentation"; @@ -121,9 +123,7 @@ export async function bindAllProduction(deps: BindAllDeps): Promise { bindProductionMarketingPages(resolvedConfig, tracer, logger, bus, queue, realtime, realtimeRegistry); // Phase E task 20 bindProductionNavigation(resolvedConfig, tracer, logger, bus, queue, realtime, realtimeRegistry); // Phase E task 21 bindProductionMedia(resolvedConfig, tracer, logger, bus, queue, realtime, realtimeRegistry); // Phase E task 22 - if (process.env.REALTIME_PING_DISABLED !== "true") { - realtimeRegistry.register(realtimePingInboundDescriptor(realtime)); - } + maybeRegisterRealtimePing(realtimeRegistry, realtime, tracer, logger); bindRealtimeBridge(bus, realtime); } @@ -143,9 +143,7 @@ export async function bindAllDevSeed(deps: BindAllDeps): Promise { await bindDevSeedMarketingPages(tracer, logger, bus, queue, realtime, realtimeRegistry); // Phase E task 20 await bindDevSeedNavigation(tracer, logger, bus, queue, realtime, realtimeRegistry); // Phase E task 21 await bindDevSeedMedia(tracer, logger, bus, queue, realtime, realtimeRegistry); // Phase E task 22 - if (process.env.REALTIME_PING_DISABLED !== "true") { - realtimeRegistry.register(realtimePingInboundDescriptor(realtime)); - } + maybeRegisterRealtimePing(realtimeRegistry, realtime, tracer, logger); bindRealtimeBridge(bus, realtime); } @@ -181,6 +179,30 @@ export async function bindAll(deps?: Partial): Promise { await bindAllDevSeed(resolvedDeps); } +// Wraps the built-in realtime-ping inbound handler in the same span+capture +// sandwich the realtime-handler generator emits (R41–R44), so the +// proof-of-life channel models the convention rather than registering raw. +// Skipped entirely when REALTIME_PING_DISABLED === "true". +function maybeRegisterRealtimePing( + registry: IRealtimeHandlerRegistry, + realtime: IRealtimeBroadcaster, + tracer: ITracer, + logger: ILogger, +): void { + if (process.env.REALTIME_PING_DISABLED === "true") return; + const { descriptor, handler } = realtimePingInboundDescriptor(realtime); + const wrappedHandler = withSpan( + tracer, + { name: "core-realtime.realtimePing", op: "realtime-handler" }, + withCapture( + logger, + { feature: "core-realtime", layer: "realtime-handler", name: "core-realtime.realtimePing" }, + handler, + ), + ); + registry.register({ descriptor, handler: wrappedHandler }); +} + function bindRealtimeBridge(_bus: IEventBus, _broadcaster: IRealtimeBroadcaster): void { // v1 ships with an empty allowlist. The dashboard PR adds the first entries here. // Example shape (commented out so v1 doesn't try to use it): diff --git a/docs/decisions/adr-016-realtime-layer.md b/docs/decisions/adr-016-realtime-layer.md index d178453..0055eb0 100644 --- a/docs/decisions/adr-016-realtime-layer.md +++ b/docs/decisions/adr-016-realtime-layer.md @@ -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)` 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 `// ` 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 diff --git a/docs/guides/realtime.md b/docs/guides/realtime.md index f4d199a..b4b24c1 100644 --- a/docs/guides/realtime.md +++ b/docs/guides/realtime.md @@ -282,7 +282,7 @@ The six ADR-015 anchors (``, ``, `