diff --git a/__tests__/lib/require-json-response.test.ts b/__tests__/lib/require-json-response.test.ts new file mode 100644 index 000000000..1cf410456 --- /dev/null +++ b/__tests__/lib/require-json-response.test.ts @@ -0,0 +1,73 @@ +/** + * @jest-environment node + */ + +/** + * FAMILIARISE_WEB-1C — a consultee's appointments page sent Sentry + * `SyntaxError: Unexpected token '<', "Gateway error"; + +function htmlResponse(status: number): Response { + return new Response(HTML, { + status, + headers: { "content-type": "text/html; charset=utf-8" }, + }); +} + +function jsonResponse(status: number, body: unknown): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }); +} + +describe("requireJsonResponse", () => { + it("turns an HTML error page into a typed error carrying the status", async () => { + await expect( + requireJsonResponse(htmlResponse(502), "Failed to cancel appointment"), + ).rejects.toMatchObject({ + name: "ApiResponseError", + status: 502, + message: "Failed to cancel appointment (HTTP 502)", + }); + }); + + it("rejects an HTML body served at 200 — a followed sign-in redirect", async () => { + const error = await requireJsonResponse( + htmlResponse(200), + "Events fetch failed", + ).catch((e: unknown) => e); + expect(error).toBeInstanceOf(ApiResponseError); + expect((error as Error).name).not.toBe("SyntaxError"); + expect((error as ApiResponseError).status).toBe(200); + }); + + it("keeps the 409 the cancel dialog branches on, and the server's own message", async () => { + await expect( + requireJsonResponse( + jsonResponse(409, { error: "This booking changed state" }), + "Failed to cancel appointment", + ), + ).rejects.toMatchObject({ + status: 409, + message: "This booking changed state", + }); + }); + + it("returns the parsed body on a 2xx JSON response", async () => { + await expect( + requireJsonResponse(jsonResponse(200, { refund: { status: "FAILED" } })), + ).resolves.toEqual({ refund: { status: "FAILED" } }); + }); +}); diff --git a/actions/stream/chat/stream.action.ts b/actions/stream/chat/stream.action.ts index bd25a6aea..056caab86 100644 --- a/actions/stream/chat/stream.action.ts +++ b/actions/stream/chat/stream.action.ts @@ -9,6 +9,7 @@ import { import { streamLogger } from "@/lib/stream-logger"; import { getSession } from "@/lib/auth-server"; import { isPrivileged } from "@/lib/auth-helpers"; +import { markExpected } from "@/lib/observability/expected"; import * as Sentry from "@sentry/nextjs"; // Token expiry for both chat and video (1 hour) @@ -29,7 +30,13 @@ async function assertCanMintToken(forUserId: string): Promise { // expires (#899). const session = await getSession(true); if (!session?.user?.id) { - throw new Error("Unauthorized: sign in to request a Stream token"); + // The connector now gates the mint on its own session, so reaching here is + // a tab whose cookie expired while it sat open — an answer, not a fault. + // The throw stays as the backstop; the marker keeps it off the error feed + // (FAMILIARISE_WEB-10). + throw markExpected( + new Error("Unauthorized: sign in to request a Stream token"), + ); } // Never mint for a banned/suspended user (#693). if (session.user.banned) { @@ -66,7 +73,10 @@ export async function tokenProvider(userId: string): Promise { streamLogger.error("Failed to generate video token", error, { userId: validatedUserId, }); - Sentry.captureException(error instanceof Error ? error : new Error(String(error)), { tags: { subsystem: "stream" } }); + Sentry.captureException( + error instanceof Error ? error : new Error(String(error)), + { tags: { subsystem: "stream" } }, + ); throw error; } } @@ -97,7 +107,10 @@ export async function chatTokenProvider(userId: string): Promise { streamLogger.error("Failed to generate chat token", error, { userId: validatedUserId, }); - Sentry.captureException(error instanceof Error ? error : new Error(String(error)), { tags: { subsystem: "stream" } }); + Sentry.captureException( + error instanceof Error ? error : new Error(String(error)), + { tags: { subsystem: "stream" } }, + ); throw error; } } diff --git a/components/appointments/consultee/useEventActions.ts b/components/appointments/consultee/useEventActions.ts index 463e25841..f6d39d4de 100644 --- a/components/appointments/consultee/useEventActions.ts +++ b/components/appointments/consultee/useEventActions.ts @@ -1,7 +1,8 @@ "use client"; import { useState } from "react"; -import * as Sentry from "@sentry/nextjs"; +import { ApiResponseError, requireJsonResponse } from "@/lib/fetch-helpers"; +import { reportSentryError } from "@/lib/observability/report"; import { useToast } from "@/hooks/use-toast"; import { useParams } from "next/navigation"; import { useQueryClient } from "@tanstack/react-query"; @@ -23,6 +24,26 @@ interface UseEventActionsOptions { consulteeId?: string; } +/** + * One report shape for both write actions on this page. + * + * These two catch blocks used to send whatever they caught at error level, and + * because both parsed the body before checking `res.ok`, an HTML error page + * arrived as `SyntaxError: Unexpected token '<'` — in Sentry and in the toast + * (FAMILIARISE_WEB-1C). With the status carried on the error, a 4xx is a server + * ANSWER rather than a fault, and everything with a status reports at warning. + */ +function reportActionFailure(error: unknown, op: string): void { + const status = error instanceof ApiResponseError ? error.status : undefined; + reportSentryError(error, { + subsystem: "client", + op, + expected: status !== undefined && status >= 400 && status < 500, + ...(status !== undefined ? { level: "warning" as const } : {}), + extra: { httpStatus: status }, + }); +} + /** * Which of the three reschedule outcomes happened. * @@ -197,15 +218,17 @@ export function useEventActions({ const response = await fetch(url, { method: "POST", headers: { "Content-Type": "application/json" }, - body: Object.keys(payload).length - ? JSON.stringify(payload) - : undefined, + body: Object.keys(payload).length ? JSON.stringify(payload) : undefined, }); - const data = await response.json(); - if (!response.ok) { - throw new Error(data.error || "Failed to request reschedule"); - } + const data = (await requireJsonResponse( + response, + "Failed to request reschedule", + )) as { + sessionsAffected?: number; + slotsAffected?: number; + autoConfirmed?: boolean; + }; // #448 — show the SESSION count, not the slot count: a 1-hour session is // 2 × 30-min slots, so the old slotsAffected label read "2 sessions" for a @@ -224,10 +247,7 @@ export function useEventActions({ invalidateBookingData(); return true; } catch (error) { - Sentry.captureException( - error instanceof Error ? error : new Error(String(error)), - { tags: { subsystem: "client" } }, - ); + reportActionFailure(error, "appointment.reschedule"); console.error("Error requesting reschedule:", error); toast({ title: "Error", @@ -262,23 +282,10 @@ export function useEventActions({ `/api/appointments/${appointmentId}/cancel`, { method: "POST", headers: { "Content-Type": "application/json" } }, ); - const data = await response.json(); - if (!response.ok) { - // 409 = the CAS transition guard matched zero rows: the booking - // already changed state (double-cancel, consultant approved a - // stale tab, …). The list refresh IS the answer — not an error. - if (response.status === 409) { - toast({ - title: "Booking already updated", - description: - "This booking changed state in the meantime — refreshing.", - }); - setShowCancelDialog(false); - invalidateBookingData(); - return; - } - throw new Error(data.error || "Failed to cancel appointment"); - } + const data = (await requireJsonResponse( + response, + "Failed to cancel appointment", + )) as { refund?: CancelRefund }; // The route reports what happened to the money on `refund`. Discarding // it meant a cancellation that refunded nothing — or failed to refund — // looked exactly like one that paid out in full. @@ -286,7 +293,7 @@ export function useEventActions({ title: "Appointment cancelled", description: [ `Your ${type.toLowerCase()} "${title}" has been cancelled.`, - describeRefund(data.refund), + describeRefund(data.refund ?? null), ] .filter(Boolean) .join(" "), @@ -294,10 +301,20 @@ export function useEventActions({ setShowCancelDialog(false); invalidateBookingData(); } catch (error) { - Sentry.captureException( - error instanceof Error ? error : new Error(String(error)), - { tags: { subsystem: "client" } }, - ); + // 409 = the CAS transition guard matched zero rows: the booking already + // changed state (double-cancel, consultant approved a stale tab, …). + // The list refresh IS the answer — not an error. + if (error instanceof ApiResponseError && error.status === 409) { + toast({ + title: "Booking already updated", + description: + "This booking changed state in the meantime — refreshing.", + }); + setShowCancelDialog(false); + invalidateBookingData(); + return; + } + reportActionFailure(error, "appointment.cancel"); console.error("Error cancelling appointment:", error); toast({ title: "Error", diff --git a/docs/booking/05-troubleshooting-and-changelog.md b/docs/booking/05-troubleshooting-and-changelog.md index e78108835..a40662cb7 100644 --- a/docs/booking/05-troubleshooting-and-changelog.md +++ b/docs/booking/05-troubleshooting-and-changelog.md @@ -115,6 +115,14 @@ Wave 6 picks up the follow-ups that wave 5 left marked in the code. Each PR appe - **Creation is now a history row.** A freshly created request had never transitioned, so it had no rows at all and the staff timeline answered "nothing has moved on this booking yet" for every new booking on production. A new `appendCreationHistory` helper writes one opening row in the same transaction as the create, from the literal `"CREATED"` to whatever status the row was born in. The literal matters: `appendHistory` renders a missing from-status as `"UNKNOWN"`, which on this surface means a concurrent writer moved the row between the pre-read and the update, and creation is emphatically not that. - **Three creation paths write it.** The direct-checkout consultation and subscription handlers already run inside the checkout transaction and simply append after the appointment exists. The request-for-approval route did not have a transaction at all — its nested create was atomic on its own — so the create and the audit row are now wrapped in one, budgeted well inside the sixty-second slot lock the route already holds. The capture webhook's own creators in `lib/payments/webhooks/handlers.ts` are deliberately out of scope for this PR. They are the legacy fallback that builds a booking when the payment carries no appointment because checkout never made one, so a booking born that way still opens with no creation row and its timeline starts at its first real transition. +### PR F — Sentry findings from the 2026-09-03 releases (`fix/sentry-2026-09-03-findings`) + +- **FAMILIARISE_WEB-1B — a rejected Novu trigger reached us as a schema complaint about the SDK's own error envelope.** The booking-request notification on `POST /api/slots/request-for-approval` failed, but what arrived in Sentry was a `ZodError` saying the field `errors` was required. The `@novu/api` schema for a 422 response requires an `errors` record, and both of the 422s Novu documents for the trigger endpoint — an unknown or unpublished workflow, and an idempotency key reused with a different body — answer with `statusCode` and `message` only. The SDK therefore threw while parsing the rejection and the real status never surfaced. No published version of the SDK relaxes that field, checked through 3.19.1, so the wrapper in `lib/novu/service.ts` now reads the status off the thrown error instead. A non-2xx is reported with its status, Novu's own message and the workflow id; a 2xx the SDK merely failed to parse is treated as an accepted send rather than a lost notification, because the trigger is already queued in that case. +- **FAMILIARISE_WEB-1C — the consultee appointments page parsed an HTML error page as JSON.** Both write actions on that page read the body before checking the response, so a Netlify crash page or a followed redirect to sign-in became `SyntaxError: Unexpected token '<'` — in the toast the consultee read, and in the error feed. A new `requireJsonResponse` in `lib/fetch-helpers.ts` checks the status and the content type before parsing and throws the existing typed `ApiResponseError`, which carries the status. The dashboard query factory and the cancel and reschedule actions now go through it, so the page shows its normal error state, the cancel dialog still recognises the 409 that means the booking changed state underneath it, and Sentry receives a warning naming the status. +- **FAMILIARISE_WEB-1D — a dropped connection paged as an error.** `useNovuSubscriberSync` reported every failure at error level, including `TypeError: Failed to fetch`, which carries no status at all and simply means the request never got an answer. React Query already retries the sync and nothing user-visible depends on it, so a transport fault is no longer reported and only a real HTTP answer is: a 4xx as a modelled outcome, a 5xx as a fault, both at warning with the status attached. The hook also skips the call outright while the browser reports itself offline, which React Query's own `networkMode` does not cover — its online manager starts optimistically and only ever flips on the window connectivity events, so a tab opened while already offline fires the request anyway. +- **FAMILIARISE_WEB-10 — a signed-out tab kept asking for a Stream token.** The token action refuses to mint without a session, and a dashboard left open past its cookie's expiry called it regardless. The connector now consults its own session and skips the mint once the session is known to be absent, treating a still-loading session as permitted so the first token request keeps its place off the join path. The server-side refusal stays as the backstop, marked expected so it lands at warning rather than on the error feed; `lib/observability/expected.ts` carries that marker and `sentry.shared.config.ts` stamps it, which is the only way to reach an error captured by Next's request-error hook. +- **FAMILIARISE_WEB-9 is deliberately untouched.** The Prisma connection timeouts are the instance-boot stall tracked under #1124, not a defect in any of the above, and the pool settings are left alone. + ## Changelog: 2026-09-02 — wave 5 The wave-5 train (#1319) reconciles the original booking and maintenance audit briefs against everything that shipped in waves 1–4 and closes the residuals that survived. Each PR appends its own bullets here. diff --git a/hooks/useNovuSubscriberSync.ts b/hooks/useNovuSubscriberSync.ts index 779224ac5..282c2c4d4 100644 --- a/hooks/useNovuSubscriberSync.ts +++ b/hooks/useNovuSubscriberSync.ts @@ -1,9 +1,20 @@ "use client"; +import { useSyncExternalStore } from "react"; import { useQuery } from "@tanstack/react-query"; import { reportSentryError } from "@/lib/observability/report"; import { useSession } from "@/lib/auth-client"; +/** Re-render on the browser's own connectivity events, so `enabled` can't latch. */ +function subscribeToConnectivity(notify: () => void): () => void { + window.addEventListener("online", notify); + window.addEventListener("offline", notify); + return () => { + window.removeEventListener("online", notify); + window.removeEventListener("offline", notify); + }; +} + /** * Auto-syncs the current user as a Novu subscriber. * Called once per dashboard session. Uses a long staleTime to avoid repeat calls. @@ -11,6 +22,18 @@ import { useSession } from "@/lib/auth-client"; export function useNovuSubscriberSync() { const { data: session } = useSession(); + // React Query's `networkMode: "online"` already pauses a fetch while offline, + // but its `onlineManager` starts at `online = true` and only ever flips on the + // window online/offline EVENTS — it never reads `navigator.onLine`. So a tab + // opened while already offline sails past that guard and fires the request, + // which is one of the ways this hook produced `TypeError: Failed to fetch` + // (FAMILIARISE_WEB-1D). Reading the flag directly closes that opening. + const isOnline = useSyncExternalStore( + subscribeToConnectivity, + () => navigator.onLine !== false, + () => true, + ); + return useQuery({ queryKey: ["novu-subscriber-sync", session?.user?.id], queryFn: async () => { @@ -29,20 +52,27 @@ export function useNovuSubscriberSync() { error && typeof error === "object" && "httpStatus" in error ? (error as { httpStatus?: number }).httpStatus : undefined; - const expected = - typeof httpStatus === "number" && httpStatus >= 400 && httpStatus < 500; - // React Query retries this (retry: 1); a failed sync just delays - // the subscriber row, nothing user-visible breaks. A 5xx/network - // fault is still worth alerting on even though retry papers over it. - reportSentryError(error, { - subsystem: "novu", - expected, - extra: { httpStatus }, - }); + // No status at all means the request never got an answer: a dropped + // connection, a tab navigating away mid-flight, a captive portal. React + // Query retries it and nothing user-visible breaks, so it is noise + // rather than a fault and used to page as `TypeError: Failed to fetch` + // at error level (FAMILIARISE_WEB-1D). A real answer still reports: + // 4xx is a modelled outcome, 5xx is worth alerting on even though the + // retry papers over it. + if (typeof httpStatus === "number") { + reportSentryError(error, { + subsystem: "novu", + op: "subscriber.sync", + expected: httpStatus >= 400 && httpStatus < 500, + level: "warning", + extra: { httpStatus }, + }); + } throw error; } }, - enabled: !!session?.user?.id && !!process.env.NEXT_PUBLIC_NOVU_APP_ID, + enabled: + isOnline && !!session?.user?.id && !!process.env.NEXT_PUBLIC_NOVU_APP_ID, staleTime: 30 * 60 * 1000, // 30 minutes gcTime: 60 * 60 * 1000, // 1 hour retry: 1, diff --git a/lib/dashboard-queries.ts b/lib/dashboard-queries.ts index cdaf9a581..9d2492408 100644 --- a/lib/dashboard-queries.ts +++ b/lib/dashboard-queries.ts @@ -23,6 +23,7 @@ import type { PlannerClassEvent, } from "@/types/planner-events"; import type { RecordingData } from "@/types/recording"; +import { requireJsonResponse } from "@/lib/fetch-helpers"; // ============================================================================= // Types @@ -56,11 +57,15 @@ async function fetchWithErrorHandling( errorPrefix: string, ): Promise { const response = await fetch(url); - if (!response.ok) { - throw new Error(`${errorPrefix}: ${response.statusText}`); - } - const json = await response.json(); - return json.data ?? json; + // `requireJsonResponse` carries the status on the thrown error and refuses to + // parse an HTML body, so a Netlify crash page or a sign-in redirect reaches + // the dashboard as "… (HTTP 502)" rather than a JSON syntax error + // (FAMILIARISE_WEB-1C). `statusText` is empty over HTTP/2, which is why the + // old message ended in a colon and nothing. + const json = (await requireJsonResponse(response, errorPrefix)) as { + data?: T; + } | null; + return (json?.data ?? json) as T; } // Consultant fetchers diff --git a/lib/fetch-helpers.ts b/lib/fetch-helpers.ts index e0a0493c8..c8c702a82 100644 --- a/lib/fetch-helpers.ts +++ b/lib/fetch-helpers.ts @@ -39,10 +39,7 @@ export const apiErrorSchema = z.object({ * or when the `error` field is absent — both happen when the server * crashes before the route handler can serialise its own error envelope. */ -export function errorMessageFromBody( - raw: unknown, - fallback: string, -): string { +export function errorMessageFromBody(raw: unknown, fallback: string): string { const parsed = apiErrorSchema.safeParse(raw); return parsed.success && parsed.data.error ? parsed.data.error : fallback; } @@ -73,6 +70,56 @@ export class ApiResponseError extends Error { } } +/** Does this response actually claim to be JSON? */ +function isJsonResponse(res: Response): boolean { + return (res.headers.get("content-type") ?? "").includes("json"); +} + +/** + * Read a response body as JSON without ever letting markup reach `JSON.parse`, + * and throw {@link ApiResponseError} — which carries the status — for anything + * that is not a 2xx JSON body. + * + * The failure this closes: a Netlify function crash, a function timeout and a + * middleware redirect to the sign-in page all answer with ``, + * and the redirect answers with it at status 200. A bare `res.json()` turned + * that into `SyntaxError: Unexpected token '<'`, which the appointments page + * then put in a toast and sent to Sentry in place of the status anyone could + * have acted on (FAMILIARISE_WEB-1C). + * + * Returns the raw parsed body; callers that want a schema use + * {@link parseJsonResponse}, which is built on this. + */ +export async function requireJsonResponse( + res: Response, + fallbackError = "Request failed", +): Promise { + const raw = isJsonResponse(res) ? await res.json().catch(() => null) : null; + + if (!res.ok) { + const parsedErr = apiErrorSchema.safeParse(raw); + const envelope = parsedErr.success ? parsedErr.data : {}; + // Without an `error` field (a 5xx with an empty or HTML body) the status is + // the only thing that distinguishes "the server crashed" from "validation + // failed", so it goes in the message the user reads. + throw new ApiResponseError( + envelope.error ?? `${fallbackError} (HTTP ${res.status})`, + { status: res.status, code: envelope.code, detail: envelope.detail }, + ); + } + + if (raw === null && !isJsonResponse(res)) { + // A 2xx that is not JSON is a redirect `fetch` followed for us, or a proxy + // page wearing a success status. Neither is an answer this caller can use. + throw new ApiResponseError( + `${fallbackError} (HTTP ${res.status}, non-JSON response)`, + { status: res.status }, + ); + } + + return raw; +} + /** * Parse a successful JSON response through a Zod schema. Throws a typed * error when the network call failed (`!res.ok`) using `errorMessageFromBody`, @@ -94,22 +141,7 @@ export async function parseJsonResponse( schema: S, fallbackError = "Request failed", ): Promise> { - const raw = await res.json().catch(() => null); - if (!res.ok) { - const parsedErr = apiErrorSchema.safeParse(raw); - const envelope = parsedErr.success ? parsedErr.data : {}; - // When the server didn't include an `error` field (typically a 5xx - // with empty/HTML body), append the HTTP status so the toast at - // least pinpoints "the server crashed" vs "validation failed". This - // is what made the old fallback "Failed to create organization" - // actively misleading — every cause produced the same string. - const message = envelope.error ?? `${fallbackError} (HTTP ${res.status})`; - throw new ApiResponseError(message, { - status: res.status, - code: envelope.code, - detail: envelope.detail, - }); - } + const raw = await requireJsonResponse(res, fallbackError); const parsed = schema.safeParse(raw); if (!parsed.success) { // Surfacing the issues in the error message keeps the failure mode @@ -121,9 +153,7 @@ export async function parseJsonResponse( .slice(0, 3) .map((i) => `${i.path.join(".") || ""}: ${i.message}`) .join("; "); - throw new Error( - `Server response did not match expected shape (${issues})`, - ); + throw new Error(`Server response did not match expected shape (${issues})`); } return parsed.data; } diff --git a/lib/novu/service.ts b/lib/novu/service.ts index 96d2de9c3..b3cae6451 100644 --- a/lib/novu/service.ts +++ b/lib/novu/service.ts @@ -69,6 +69,88 @@ function reportNotConfigured(workflowId: string): void { } } +/** + * What the SDK actually told us about a failed trigger. + * + * `@novu/api` validates the RESPONSE against its own generated Zod schema and + * throws `ResponseValidationError` before handing back the status. Its 422 + * schema requires an `errors` record, but the two 422s Novu documents for this + * endpoint — an unknown or unpublished workflow (`workflow_not_found`) and an + * idempotency key reused with a different body — both answer with `statusCode` + * and `message` only. So all that reached Sentry was a ZodError about a field + * of the SDK's own error envelope: the status and Novu's reason were both lost + * (FAMILIARISE_WEB-1B). No published `@novu/api` relaxes that field (checked + * through 3.19.1), so read the status off the error instead of chasing a bump. + * + * Duck-typed on `statusCode`: every `NovuError` subclass carries it, and the + * class itself is not re-exported from the package root, so an `instanceof` + * would mean deep-importing generated internals. + */ +function describeNovuFailure(error: unknown): { + statusCode?: number; + /** Novu's own error text. Never the whole body — it can echo payload values. */ + novuMessage?: string; + /** True when the SDK rejected a body Novu had already accepted. */ + accepted: boolean; +} { + if (!error || typeof error !== "object") return { accepted: false }; + const { statusCode, body } = error as { + statusCode?: unknown; + body?: unknown; + }; + if (typeof statusCode !== "number") return { accepted: false }; + + let novuMessage: string | undefined; + if (typeof body === "string" && body.length > 0) { + try { + const parsed: unknown = JSON.parse(body); + const message = + parsed && typeof parsed === "object" + ? (parsed as { message?: unknown }).message + : undefined; + if (typeof message === "string") novuMessage = message.slice(0, 200); + } catch { + // Not JSON (an HTML gateway page); the status alone is the signal. + } + } + + return { + statusCode, + novuMessage, + accepted: statusCode >= 200 && statusCode < 300, + }; +} + +/** + * One report shape for every `novu.trigger` failure. `accepted` means the + * notification is already queued at Novu and only the SDK's response parsing + * failed, so it is an expected outcome rather than a lost notification. + */ +function reportTriggerFailure( + error: unknown, + workflowId: string, + recipientCount: number, +): { accepted: boolean } { + const { statusCode, novuMessage, accepted } = describeNovuFailure(error); + console.error( + `[Novu] Failed to trigger ${workflowId} (status ${statusCode ?? "none"}):`, + error, + ); + Sentry.captureException( + error instanceof Error ? error : new Error(String(error)), + { + tags: { + subsystem: "novu", + op: "trigger", + expected: String(accepted), + }, + level: "warning", + extra: { workflowId, statusCode, novuMessage, recipientCount }, + }, + ); + return { accepted }; +} + // Deterministic transactionId so app-level retries can't double-notify: Novu // rejects a repeated transactionId. Derived from recipient(s) + workflow + // canonical payload (the payloads carry the entity ids). `dedupeKey` lets a @@ -121,11 +203,11 @@ async function triggerWorkflow( console.log(`[Novu] Triggered ${workflowId} for ${subscriberId}`); return { success: true }; } catch (error) { - console.error(`[Novu] Failed to trigger ${workflowId}:`, error); - Sentry.captureException( - error instanceof Error ? error : new Error(String(error)), - { tags: { subsystem: "novu" }, level: "warning" }, - ); + // A 2xx the SDK could not parse still queued the notification; reporting it + // as a failed send made callers retry a send Novu had already accepted. + if (reportTriggerFailure(error, workflowId, 1).accepted) { + return { success: true }; + } return { success: false, error: error instanceof Error ? error : String(error), @@ -178,7 +260,10 @@ async function triggerForMultiple( ); results.push(...batch.map(() => ({ success: true }) as TriggerResult)); } catch (error) { - console.error(`[Novu] Failed to trigger ${workflowId} for batch:`, error); + if (reportTriggerFailure(error, workflowId, batch.length).accepted) { + results.push(...batch.map(() => ({ success: true }) as TriggerResult)); + continue; + } const err: TriggerResult = { success: false, error: error instanceof Error ? error : String(error), diff --git a/lib/observability/expected.ts b/lib/observability/expected.ts new file mode 100644 index 000000000..1cc76c834 --- /dev/null +++ b/lib/observability/expected.ts @@ -0,0 +1,31 @@ +/** + * Marks a thrown error as a modelled outcome rather than a fault. + * + * `reportSentryError` tags its own captures, but an error that escapes a server + * action or a route handler is captured by Next's `onRequestError` hook, which + * takes no per-call options. The marker therefore rides on the error object and + * `sentry.shared.config.ts` stamps `expected:true` in `beforeSend`, so a guard + * that fired by design lands at warning instead of paging (FAMILIARISE_WEB-10). + * + * Deliberately dependency-free: the Sentry config imports it during init, and a + * non-enumerable symbol keeps the marker out of JSON serialisation and out of + * anything that spreads the error. + */ + +const EXPECTED_ERROR = Symbol.for("familiarise.observability.expectedError"); + +export function markExpected(error: E): E { + Object.defineProperty(error, EXPECTED_ERROR, { + value: true, + enumerable: false, + }); + return error; +} + +export function isExpectedError(error: unknown): boolean { + return ( + !!error && + typeof error === "object" && + (error as Record)[EXPECTED_ERROR] === true + ); +} diff --git a/providers/StreamProviderImpl.tsx b/providers/StreamProviderImpl.tsx index b957590d6..77c93c248 100644 --- a/providers/StreamProviderImpl.tsx +++ b/providers/StreamProviderImpl.tsx @@ -20,6 +20,7 @@ import { import { upsertUserToStream } from "@/actions/stream/chat/user.action"; import { syncUserEventChannels } from "@/actions/stream/chat/event-channel.action"; import { useUserData } from "@/hooks/useUserData"; +import { useSession } from "@/lib/auth-client"; import { mapRoleToStream } from "@/lib/user"; import { streamLogger } from "@/lib/stream-logger"; import { setStreamConnection } from "@/lib/stream/connection-store"; @@ -114,6 +115,24 @@ const StreamProviderImpl = ({ const { userDetails, isLoading } = useUserData(userId); + // The token action refuses to mint without a session, and a tab whose cookie + // expired while it sat open kept calling it anyway — 8 unauthorized throws on + // the error feed with nobody to show them to (FAMILIARISE_WEB-10). The + // client's own copy of the session is the cheap gate. + // + // `isPending` counts as ALLOWED on purpose: blocking the first mint on the + // session round trip would put a serial wait back on the join path, which is + // exactly what the prefetch effect below exists to remove (#248). + const { data: clientSession, isPending: isSessionPending } = useSession(); + const signedOut = !isSessionPending && !clientSession?.user?.id; + // The video SDK calls `tokenProvider` again on its own schedule, long after + // this render; the ref is how that callback sees the current answer without + // changing `getCachedToken`'s identity and re-firing the connect effect. + const signedOutRef = useRef(false); + useEffect(() => { + signedOutRef.current = signedOut; + }, [signedOut]); + // Token caching with expiry tracking — use ref to avoid triggering re-renders // (useState here caused getCachedToken → connectChat → connectServices to // be recreated on every token fetch, making the connectUser useEffect fire @@ -169,14 +188,19 @@ const StreamProviderImpl = ({ return type === "chat" ? cache.chatToken! : cache.videoToken!; } - const existing = type === "chat" ? tokenPromiseRef.current.chat : tokenPromiseRef.current.video; + const existing = + type === "chat" + ? tokenPromiseRef.current.chat + : tokenPromiseRef.current.video; if (existing) return existing; + if (signedOutRef.current) { + throw new Error("Stream token skipped: no signed-in session"); + } + // Generate new token const request = - type === "chat" - ? chatTokenProvider(userId) - : tokenProvider(userId); + type === "chat" ? chatTokenProvider(userId) : tokenProvider(userId); if (type === "chat") tokenPromiseRef.current.chat = request; else tokenPromiseRef.current.video = request; @@ -198,15 +222,17 @@ const StreamProviderImpl = ({ // fails; left unattached that is an unhandled rejection on every failed // mint. The catch swallows exactly that derived rejection — the original // still propagates to `return request` callers. - void request.finally(() => { - if (tokenPromiseRef.current.userId !== userId) return; - if (type === "chat" && tokenPromiseRef.current.chat === request) { - delete tokenPromiseRef.current.chat; - } - if (type === "video" && tokenPromiseRef.current.video === request) { - delete tokenPromiseRef.current.video; - } - }).catch(() => {}); + void request + .finally(() => { + if (tokenPromiseRef.current.userId !== userId) return; + if (type === "chat" && tokenPromiseRef.current.chat === request) { + delete tokenPromiseRef.current.chat; + } + if (type === "video" && tokenPromiseRef.current.video === request) { + delete tokenPromiseRef.current.video; + } + }) + .catch(() => {}); return request; }, @@ -222,14 +248,21 @@ const StreamProviderImpl = ({ // failures are handled by the normal connect paths, which re-request via // getCachedToken (cleared promise ref → fresh attempt). useEffect(() => { - if (!apiKey || !userId) return; + if (!apiKey || !userId || signedOut) return; if (enableChat && !isTokenValid("chat", userId)) { void getCachedToken("chat").catch(() => {}); } if (enableVideo && !isTokenValid("video", userId)) { void getCachedToken("video").catch(() => {}); } - }, [userId, enableChat, enableVideo, getCachedToken, isTokenValid]); + }, [ + userId, + enableChat, + enableVideo, + getCachedToken, + isTokenValid, + signedOut, + ]); // Exponential backoff retry logic const getRetryDelay = useCallback((attempt: number) => { diff --git a/sentry.shared.config.ts b/sentry.shared.config.ts index c5d75ee63..2b8597cf5 100644 --- a/sentry.shared.config.ts +++ b/sentry.shared.config.ts @@ -5,6 +5,7 @@ // centralizes the one config so a sampling/PII/env tweak lands in one place. (#913) import * as Sentry from "@sentry/nextjs"; +import { isExpectedError } from "@/lib/observability/expected"; import { isNotDevelopmentEnvironment, isProductionEnvironment, @@ -69,6 +70,19 @@ export function initSentry(overrides?: Partial): void { /^safari-web-extension:\/\//i, ], + // Errors captured by Next's `onRequestError` hook carry no per-call + // options, so a guard that fires by design (an expired cookie reaching an + // auth check) arrives looking like a fault. `markExpected` puts a marker on + // the thrown error and this stamps the tag. Never drops an event — it only + // re-levels one. (FAMILIARISE_WEB-10) + beforeSend(event, hint) { + if (isExpectedError(hint?.originalException)) { + event.level = "warning"; + event.tags = { ...event.tags, expected: "true" }; + } + return event; + }, + // Last, so a caller can narrow a knob it has better information about. // Undefined spreads to nothing, which is what every app entrypoint does. ...overrides,