diff --git a/frontend/SENTRY_SETUP.md b/frontend/SENTRY_SETUP.md index 63214adc..60c23d79 100644 --- a/frontend/SENTRY_SETUP.md +++ b/frontend/SENTRY_SETUP.md @@ -47,9 +47,24 @@ Reference `.env.example` for all required variables. After setting the DSN, Sentry will automatically: - Initialize on client and server - Capture unhandled exceptions -- Record session replays for errors - Report API failures with context +## Sampling Defaults (issue #1987) + +- **Traces**: `NEXT_PUBLIC_SENTRY_TRACES_SAMPLE_RATE` defaults to `1.0` in + development but only `0.1` in production, so one forgotten deployment + cannot exhaust the quota. Set the variable explicitly to change this + (values outside [0, 1] fall back to the default). The server and edge + configs share the same helper (`lib/sentry-sampling.ts`). +- **Session replays are opt-in only**: both + `NEXT_PUBLIC_SENTRY_REPLAYS_SESSION_SAMPLE_RATE` and + `NEXT_PUBLIC_SENTRY_REPLAYS_ON_ERROR_SAMPLE_RATE` default to `0`. Enable + them explicitly (e.g. `0.1` / `1.0`) if you want recording; the replay + integration still masks all text and blocks all media whenever it runs. +- **Non-Error rejections** never crash the `beforeSend` hook anymore: thrown + strings, `undefined` or plain objects are reported as `NonError` events + with a stringified message instead of being dropped. + ## Error Context Errors are tagged with: - `errorBoundary`: Which error boundary caught the error (global/wallet) diff --git a/frontend/app/layout.tsx b/frontend/app/layout.tsx index 4609d2d7..9cad8425 100644 --- a/frontend/app/layout.tsx +++ b/frontend/app/layout.tsx @@ -1,4 +1,4 @@ -import type { Metadata } from "next"; +import type { Metadata, Viewport } from "next"; import { Geist, Geist_Mono } from "next/font/google"; import { Providers } from "@/lib/providers/providers"; import { OfflineBanner } from "@/components/admin/offline-banner"; @@ -14,15 +14,24 @@ const geistMono = Geist_Mono({ subsets: ["latin"], }); +/** + * Since Next.js 14 the viewport must be exported separately (issue #1986); + * leaving it inside `metadata` produces build warnings and is ignored in + * newer releases. `app/manifest.ts` uses the same theme colour. + */ +export const viewport: Viewport = { + width: "device-width", + initialScale: 1, + themeColor: "#0f172a", +}; + export const metadata: Metadata = { - viewport: "width=device-width, initial-scale=1", title: { default: "ManageHub - Smart Hub & Workspace Management", template: "%s | ManageHub", }, description: "Smart Hub & Workspace Management System for modern teams. Streamline operations, manage resources, and boost productivity with our comprehensive management platform.", - themeColor: "#0f172a", keywords: [ "workspace management", "hub management", diff --git a/frontend/components/auth/auth-form.test.tsx b/frontend/components/auth/auth-form.test.tsx index 0d77303e..356dade9 100644 --- a/frontend/components/auth/auth-form.test.tsx +++ b/frontend/components/auth/auth-form.test.tsx @@ -21,11 +21,34 @@ vi.mock("@/lib/stores/session-store", () => ({ ) => selector({ setSession: mocks.setSession }), })); -vi.mock("@/lib/auth-api", () => ({ - login: mocks.login, - register: mocks.register, -})); +// The mocked module must expose the same AuthApiError class the component +// uses for its instanceof checks (issue #1988), so thrown errors are +// recognised as structured field failures. +vi.mock("@/lib/auth-api", () => { + class MockAuthApiError extends Error { + status: number; + fieldErrors: Record; + + constructor( + status: number, + fieldErrors: Record, + message: string, + ) { + super(message); + this.name = "AuthApiError"; + this.status = status; + this.fieldErrors = fieldErrors; + } + } + + return { + AuthApiError: MockAuthApiError, + login: mocks.login, + register: mocks.register, + }; +}); +import { AuthApiError } from "@/lib/auth-api"; import { AuthForm } from "./auth-form"; function renderForm(mode: "login" | "register") { @@ -82,3 +105,135 @@ describe("AuthForm registration confirmation", () => { expect(mocks.push).toHaveBeenCalledWith("/wallet"); }); }); + +describe("AuthForm per-field API errors (issue #1988)", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("renders 400 validation messages next to their own field", async () => { + const user = userEvent.setup(); + mocks.register.mockRejectedValue( + new AuthApiError( + 400, + { + email: "email must be an email", + password: "password must be longer than or equal to 8 characters", + }, + "Bad Request", + ), + ); + renderForm("register"); + + await user.type(screen.getByLabelText("Email"), "not-an-email"); + await user.type(screen.getByLabelText("Password"), "short"); + await user.type(screen.getByLabelText("Confirm password"), "short"); + await user.click(screen.getByRole("button", { name: "Create account" })); + + const emailError = await screen.findByText("email must be an email"); + expect(emailError).toHaveAttribute("id", "email-error"); + expect(screen.getByLabelText("Email")).toHaveAttribute( + "aria-invalid", + "true", + ); + expect(screen.getByLabelText("Email")).toHaveAttribute( + "aria-describedby", + "email-error", + ); + + const passwordError = screen.getByText( + "password must be longer than or equal to 8 characters", + ); + expect(passwordError).toHaveAttribute("id", "password-error"); + expect(screen.getByLabelText("Password")).toHaveAttribute( + "aria-invalid", + "true", + ); + + // The generic form-level banner is silent when the fields carry the + // detail - the message is already next to the offending input. + expect(screen.queryByText("Bad Request")).not.toBeInTheDocument(); + // The first invalid field receives focus for keyboard users. + expect(document.activeElement).toBe(screen.getByLabelText("Email")); + }); + + it("maps a 409 taken-email failure onto the email field", async () => { + const user = userEvent.setup(); + mocks.register.mockRejectedValue( + new AuthApiError( + 409, + { email: "Email is already registered" }, + "Conflict", + ), + ); + renderForm("register"); + + await user.type(screen.getByLabelText("Email"), "taken@example.com"); + await user.type(screen.getByLabelText("Password"), "password-one"); + await user.type(screen.getByLabelText("Confirm password"), "password-one"); + await user.click(screen.getByRole("button", { name: "Create account" })); + + expect( + await screen.findByText("Email is already registered"), + ).toBeInTheDocument(); + expect(screen.getByLabelText("Email")).toHaveAttribute( + "aria-invalid", + "true", + ); + }); + + it("shows a single form-level message when rate limited (429)", async () => { + const user = userEvent.setup(); + mocks.login.mockRejectedValue( + new AuthApiError(429, {}, "Too many attempts, try again in 60 seconds"), + ); + renderForm("login"); + + await user.type(screen.getByLabelText("Email"), "person@example.com"); + await user.type(screen.getByLabelText("Password"), "whatever"); + await user.click(screen.getByRole("button", { name: "Sign in" })); + + const alert = await screen.findByRole("alert"); + expect(alert).toHaveTextContent( + "Too many attempts. Please wait a few minutes and try again.", + ); + expect(screen.queryByText(/seconds/)).not.toBeInTheDocument(); + expect(screen.getByLabelText("Email")).not.toHaveAttribute( + "aria-invalid", + ); + }); + + it("keeps the top banner for failures without field detail (401)", async () => { + const user = userEvent.setup(); + mocks.login.mockRejectedValue(new AuthApiError(401, {}, "Invalid credentials")); + renderForm("login"); + + await user.type(screen.getByLabelText("Email"), "person@example.com"); + await user.type(screen.getByLabelText("Password"), "wrong-password"); + await user.click(screen.getByRole("button", { name: "Sign in" })); + + expect( + await screen.findByText("Invalid credentials"), + ).toBeInTheDocument(); + expect(screen.getByLabelText("Email")).not.toHaveAttribute( + "aria-invalid", + ); + }); + + it("clears a field's error when the user edits that field", async () => { + const user = userEvent.setup(); + mocks.register.mockRejectedValue( + new AuthApiError(400, { email: "email must be an email" }, "Bad Request"), + ); + renderForm("register"); + + await user.type(screen.getByLabelText("Email"), "nope"); + await user.type(screen.getByLabelText("Password"), "password-one"); + await user.type(screen.getByLabelText("Confirm password"), "password-one"); + await user.click(screen.getByRole("button", { name: "Create account" })); + await screen.findByText("email must be an email"); + + await user.type(screen.getByLabelText("Email"), "x"); + expect(screen.queryByText("email must be an email")).not.toBeInTheDocument(); + }); +}); diff --git a/frontend/components/auth/auth-form.tsx b/frontend/components/auth/auth-form.tsx index fa7065a9..1b329e1a 100644 --- a/frontend/components/auth/auth-form.tsx +++ b/frontend/components/auth/auth-form.tsx @@ -3,7 +3,7 @@ import { useState } from "react"; import { useRouter, useSearchParams } from "next/navigation"; import Link from "next/link"; -import { login, register } from "@/lib/auth-api"; +import { login, register, AuthApiError } from "@/lib/auth-api"; import { getSafeReturnTo } from "@/lib/auth-redirect"; import { useSessionStore } from "@/lib/stores/session-store"; @@ -17,14 +17,29 @@ export function AuthForm({ mode }: { mode: Mode }) { const [password, setPassword] = useState(""); const [confirmPassword, setConfirmPassword] = useState(""); const [error, setError] = useState(null); + // Per-field validation errors from the API (issue #1988): keys are + // "email" / "password", values are backend messages for that field. + const [fieldErrors, setFieldErrors] = useState< + Record | null + >(null); const [confirmationError, setConfirmationError] = useState(false); const [submitting, setSubmitting] = useState(false); const isLogin = mode === "login"; + function clearFieldError(field: string) { + setFieldErrors((prev) => { + if (!prev || !(field in prev)) return prev; + const next = { ...prev }; + delete next[field]; + return next; + }); + } + async function handleSubmit(event: React.FormEvent) { event.preventDefault(); setError(null); + setFieldErrors(null); if (!isLogin && password !== confirmPassword) { setConfirmationError(true); return; @@ -43,7 +58,29 @@ export function AuthForm({ mode }: { mode: Mode }) { ); router.push(returnTo); } catch (err) { - setError(err instanceof Error ? err.message : "Something went wrong"); + // Rate limiting (#1988): show a single, calm form-level message and + // make it clear the problem is not a specific field. + if (err instanceof AuthApiError && err.status === 429) { + setFieldErrors(null); + setError("Too many attempts. Please wait a few minutes and try again."); + return; + } + + const fieldErrors = + err instanceof AuthApiError ? { ...err.fieldErrors } : {}; + const firstInvalidField = Object.entries(fieldErrors)[0]; + + if (firstInvalidField) { + // Render each message next to its field, announce them to screen + // readers (aria-live via role="alert") and focus the first invalid + // input so keyboard users land straight on what to fix. + setFieldErrors(fieldErrors); + setError(null); + document.getElementById(firstInvalidField[0])?.focus(); + } else { + setFieldErrors(null); + setError(err instanceof Error ? err.message : "Something went wrong"); + } } finally { setSubmitting(false); } @@ -74,9 +111,25 @@ export function AuthForm({ mode }: { mode: Mode }) { type="email" required value={email} - onChange={(e) => setEmail(e.target.value)} + onChange={(e) => { + setEmail(e.target.value); + clearFieldError("email"); + }} + aria-invalid={ + fieldErrors?.email ? true : undefined + } + aria-describedby={fieldErrors?.email ? "email-error" : undefined} className="w-full rounded-md border border-gray-300 dark:border-gray-700 bg-transparent px-3 py-2 text-sm" /> + {fieldErrors?.email && ( + + )}
@@ -96,9 +149,23 @@ export function AuthForm({ mode }: { mode: Mode }) { onChange={(e) => { setPassword(e.target.value); setConfirmationError(false); + clearFieldError("password"); }} + aria-invalid={fieldErrors?.password ? true : undefined} + aria-describedby={ + fieldErrors?.password ? "password-error" : undefined + } className="w-full rounded-md border border-gray-300 dark:border-gray-700 bg-transparent px-3 py-2 text-sm" /> + {fieldErrors?.password && ( + + )}
{!isLogin && ( diff --git a/frontend/lib/__tests__/sentry-client-config.test.ts b/frontend/lib/__tests__/sentry-client-config.test.ts index 1e6d7720..b06013e3 100644 --- a/frontend/lib/__tests__/sentry-client-config.test.ts +++ b/frontend/lib/__tests__/sentry-client-config.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; const sentryMocks = vi.hoisted(() => ({ init: vi.fn(), @@ -7,7 +7,11 @@ const sentryMocks = vi.hoisted(() => ({ vi.mock("@sentry/nextjs", () => sentryMocks); -import { getSampleRate } from "../../sentry.client.config"; +import { + defaultTracesSampleRate, + describeException, + getSampleRate, +} from "../../sentry.client.config"; describe("getSampleRate", () => { it("uses a configured rate within the valid range", () => { @@ -21,3 +25,61 @@ describe("getSampleRate", () => { expect(getSampleRate("1.1", 0.1)).toBe(0.1); }); }); + +describe("defaultTracesSampleRate (issue #1987)", () => { + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("samples 100 percent in development", () => { + vi.stubEnv("NODE_ENV", "development"); + expect(defaultTracesSampleRate()).toBe(1.0); + }); + + it("samples at a production-safe 10 percent by default", () => { + vi.stubEnv("NODE_ENV", "production"); + expect(defaultTracesSampleRate()).toBe(0.1); + }); +}); + +describe("describeException (issue #1987)", () => { + it("passes real Errors through with their identity", () => { + const error = new RangeError("boom"); + expect(describeException(error)).toMatchObject({ + name: "RangeError", + message: "boom", + }); + expect(describeException(error).stack).toContain("describeException"); + }); + + it("labels thrown strings as NonError instead of crashing", () => { + expect(describeException("plain string failure")).toEqual({ + name: "NonError", + message: "plain string failure", + }); + }); + + it("handles undefined and null rejections without throwing", () => { + expect(describeException(undefined)).toEqual({ + name: "NonError", + message: JSON.stringify(undefined) ?? "", + }); + expect(describeException(null).name).toBe("NonError"); + }); + + it("stringifies plain objects and never throws on circular ones", () => { + expect( + describeException({ reason: "websocket closed" }), + ).toMatchObject({ + name: "NonError", + message: JSON.stringify({ reason: "websocket closed" }), + }); + + const circular: Record = {}; + circular.self = circular; + expect(describeException(circular)).toEqual({ + name: "NonError", + message: String(circular), + }); + }); +}); diff --git a/frontend/lib/auth-api.ts b/frontend/lib/auth-api.ts index d8bd3853..5895760c 100644 --- a/frontend/lib/auth-api.ts +++ b/frontend/lib/auth-api.ts @@ -1,3 +1,4 @@ +import { getApiErrorMessage } from "@/lib/api-report"; import { getApiBaseUrl } from "@/lib/env"; import { request } from "@/lib/http"; import type { AuthUser } from "@/lib/auth-types"; @@ -11,6 +12,57 @@ export interface AuthResponse { user: AuthUser; } +/** + * Auth failures that carry structured field information (issue #1988). + * `fieldErrors` maps a form field ("email", "password") to the first + * validation message the backend produced for it; keys are absent when the + * backend did not attribute the failure to a specific field (401, network + * downtime, unexpected payloads). + */ +export class AuthApiError extends Error { + readonly status: number; + readonly fieldErrors: Readonly>; + + constructor( + status: number, + fieldErrors: Record, + message: string, + ) { + super(message); + this.name = "AuthApiError"; + this.status = status; + this.fieldErrors = fieldErrors; + } +} + +const AUTH_FIELDS = ["email", "password"] as const; + +/** + * Maps backend validation output onto form fields. Nest/class-validator + * validation failures arrive as a 400 whose `message` is an array of strings + * such as "email must be an email" or "password must be longer than or + * equal to 8 characters"; each message is matched to the field it names so + * the form can render it next to the offending input. + */ +function collectFieldErrors(errorBody: unknown): Record { + const fieldErrors: Record = {}; + const message = (errorBody as { message?: unknown } | null)?.message; + + if (Array.isArray(message)) { + for (const text of message) { + if (typeof text !== "string") continue; + const field = AUTH_FIELDS.find((name) => + text.toLowerCase().startsWith(name), + ); + if (field && !fieldErrors[field]) { + fieldErrors[field] = text; + } + } + } + + return fieldErrors; +} + /** * Every request includes `credentials: "include"` (issue #1975): the backend * answers register/login with an HttpOnly `accessToken` cookie, so the session @@ -28,10 +80,29 @@ function authRequest( // A 401 from login/register means bad credentials, not an expired // session - resolving the session and redirecting here would be wrong. expireOn401: false, - fallbackMessage: (status) => - status === 401 - ? "Invalid credentials" - : `Authentication failed (${status})`, + makeError: (status, errorBody) => { + const fallbackMessage = + status === 401 + ? "Invalid credentials" + : `Authentication failed (${status})`; + // Attach per-field validation detail when the backend provided it + // (issue #1988): 400 field arrays, and 409 for a taken email address. + const fieldErrors = collectFieldErrors(errorBody); + if (status === 409 && !fieldErrors.email) { + fieldErrors.email = getApiErrorMessage( + errorBody as Record | null, + "Email is already registered", + ); + } + return new AuthApiError( + status, + fieldErrors, + getApiErrorMessage( + errorBody as Record | null, + fallbackMessage, + ), + ); + }, }); } diff --git a/frontend/lib/sentry-sampling.ts b/frontend/lib/sentry-sampling.ts new file mode 100644 index 00000000..29e60c6c --- /dev/null +++ b/frontend/lib/sentry-sampling.ts @@ -0,0 +1,27 @@ +/** + * Sampling helpers shared by the three Sentry configs (client, server, edge) + * so their defaults cannot drift apart (issue #1987). + */ + +/** + * Parses a sampling-rate env var. Returns the fallback when the value is + * missing, not a number, or outside [0, 1]. + */ +export function getSampleRate(value: string | undefined, fallback: number): number { + if (!value) return fallback; + + const parsed = Number(value); + return Number.isFinite(parsed) && parsed >= 0 && parsed <= 1 + ? parsed + : fallback; +} + +/** + * Default trace sampling when NEXT_PUBLIC_SENTRY_TRACES_SAMPLE_RATE is not + * set: 100% in development (cheap, everything is local) and 10% by default in + * production so one forgotten deployment cannot exhaust the Sentry quota. + * Applications that want full tracing in production opt in explicitly. + */ +export function defaultTracesSampleRate(): number { + return process.env.NODE_ENV === "production" ? 0.1 : 1.0; +} diff --git a/frontend/sentry.client.config.ts b/frontend/sentry.client.config.ts index 1a69b3e5..94cf65d2 100644 --- a/frontend/sentry.client.config.ts +++ b/frontend/sentry.client.config.ts @@ -1,13 +1,12 @@ import * as Sentry from "@sentry/nextjs"; +import { + defaultTracesSampleRate, + getSampleRate, +} from "@/lib/sentry-sampling"; -export function getSampleRate(value: string | undefined, fallback: number): number { - if (!value) return fallback; - - const parsed = Number(value); - return Number.isFinite(parsed) && parsed >= 0 && parsed <= 1 - ? parsed - : fallback; -} +// Re-exported so tooling and tests can treat this config as the single entry +// point for the client sampling behavior. +export { defaultTracesSampleRate, getSampleRate }; // Keys whose values must never be sent to Sentry. const SENSITIVE_KEY_PATTERN = @@ -29,20 +28,53 @@ function scrubValue(value: unknown): unknown { return value; } +/** + * Describes the original exception for the event's `contexts.error` without + * assuming it is an `Error` (issue #1987): rejected strings, `undefined`, + * thrown objects that are not Errors or promise rejection reasons arrived at + * the old cast and crashed this hook, silently dropping the whole event. + * Anything that is not a real Error is labelled `NonError` and stringified as + * safely as possible - it must never throw here. + */ +export function describeException(original: unknown): { + name: string; + message: string; + stack?: string; +} { + if (original instanceof Error) { + return { + name: original.name, + message: original.message, + stack: original.stack, + }; + } + if (typeof original === "string") { + return { name: "NonError", message: original }; + } + try { + return { name: "NonError", message: JSON.stringify(original) ?? "" }; + } catch { + return { name: "NonError", message: String(original) }; + } +} + Sentry.init({ dsn: process.env.NEXT_PUBLIC_SENTRY_DSN, environment: process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT || process.env.NODE_ENV, tracesSampleRate: getSampleRate( process.env.NEXT_PUBLIC_SENTRY_TRACES_SAMPLE_RATE, - 1.0, + defaultTracesSampleRate(), ), + // Replays are opt-in only (issue #1987): no env var, no replay. Recording + // every session by default is a privacy and quota decision that belongs to + // the people deploying the app, not the default configuration. replaysSessionSampleRate: getSampleRate( process.env.NEXT_PUBLIC_SENTRY_REPLAYS_SESSION_SAMPLE_RATE, - 0.1, + 0, ), replaysOnErrorSampleRate: getSampleRate( process.env.NEXT_PUBLIC_SENTRY_REPLAYS_ON_ERROR_SAMPLE_RATE, - 1.0, + 0, ), integrations: [ Sentry.replayIntegration({ @@ -52,14 +84,11 @@ Sentry.init({ ], beforeSend(event, hint) { if (event.exception) { - const error = hint.originalException as Error; + const original = + hint && typeof hint === "object" ? hint.originalException : undefined; event.contexts = { ...event.contexts, - error: { - message: error.message, - name: error.name, - stack: error.stack, - }, + error: describeException(original), }; } // Second line of defence: never ship request bodies, and redact any diff --git a/frontend/sentry.edge.config.ts b/frontend/sentry.edge.config.ts index fdc900bc..ef0f8a61 100644 --- a/frontend/sentry.edge.config.ts +++ b/frontend/sentry.edge.config.ts @@ -1,7 +1,16 @@ import * as Sentry from "@sentry/nextjs"; +import { + defaultTracesSampleRate, + getSampleRate, +} from "@/lib/sentry-sampling"; +// Kept consistent with sentry.client.config.ts (issue #1987): 100% in +// development, 10% in production unless explicitly configured. Sentry.init({ dsn: process.env.NEXT_PUBLIC_SENTRY_DSN, environment: process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT || process.env.NODE_ENV, - tracesSampleRate: 1.0, + tracesSampleRate: getSampleRate( + process.env.NEXT_PUBLIC_SENTRY_TRACES_SAMPLE_RATE, + defaultTracesSampleRate(), + ), }); diff --git a/frontend/sentry.server.config.ts b/frontend/sentry.server.config.ts index fdc900bc..ef0f8a61 100644 --- a/frontend/sentry.server.config.ts +++ b/frontend/sentry.server.config.ts @@ -1,7 +1,16 @@ import * as Sentry from "@sentry/nextjs"; +import { + defaultTracesSampleRate, + getSampleRate, +} from "@/lib/sentry-sampling"; +// Kept consistent with sentry.client.config.ts (issue #1987): 100% in +// development, 10% in production unless explicitly configured. Sentry.init({ dsn: process.env.NEXT_PUBLIC_SENTRY_DSN, environment: process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT || process.env.NODE_ENV, - tracesSampleRate: 1.0, + tracesSampleRate: getSampleRate( + process.env.NEXT_PUBLIC_SENTRY_TRACES_SAMPLE_RATE, + defaultTracesSampleRate(), + ), });