Frontend Code Checks

Frontend Code Checks (apps/web)

Lessons codified from review comments on Ella's PRs (PR #867 — EntityCard migration). Apply the relevant rule before declaring a frontend task done. Don't skip task typecheck && task lint — these rules catch what the compiler can't.

Trigger scenarios

Run the relevant check when:

• Editing any file under apps/web/src/**
• Introducing a new utility in apps/web/src/lib/** (consistency sweep)
• Migrating a card / list item to DS EntityCard
• Adding a <button> inside a <Link> / <a> wrapper
• Adding an <img> whose src comes from user-supplied / external data
• Writing any catch block

Rule checklist

1. Silent-catch — AGENTS.md §170

Never .catch(() => {}) or try { ... } catch {} except in these cases:

• JSON.parse / parse-or-fallback
• Expected 404 → null
• Cleanup / rollback before re-throwing
• Terminating an onClick→useActionMutate fire-and-forget whose real errors the global QueryProvider pipeline already surfaces (see §1a)

Anti-pattern (swallowing an error that has no other handler):

Promise.resolve(doThingWithNoGlobalHandler(id)).catch(() => {});

Fix:

void doThingWithNoGlobalHandler(id);

Terminate the floating promise without swallowing real errors — they should reach React Query onError / React error boundary / the global API error handler. For the terminator choice (void vs .catch(() => {})), see §1a.

1a. void vs .catch(() => {}) — pick by context (not one global rule)

eslint.config.mjs sets:

• @typescript-eslint/no-floating-promises: 'error'
• no-void: ['error', { allowAsStatement: true }]

Both terminators satisfy no-floating-promises; the floating promise just has to be terminated somehow. Which one is correct depends on the call site — there is no single house style, and treating one as universal is what created the contradiction this rule replaces:

• Do not "fix" an existing .catch(() => {}) on an onClick→useActionMutate path to void — that reintroduces the S905 finding. Confirm the precedent with rg "\.catch\(\(\) => \{\}\)" apps/web/src/components/reports/ReportConfigActions.tsx.
• Do not refactor void foo() → (async () => { await foo() })() — unnecessary and less readable.
• The only acceptable change to a void statement: if the callback was already async, drop the arrow wrapper and await directly.

1b. No nested ternaries — SonarQube S3358

More than one level of ternary nesting is hard to read and easy to misparse. This is a real lint concern (unlike §1a).

Anti-pattern:

const label =
  id === CUSTOM
    ? t('custom')
    : id === OTHER
      ? t('other')
      : (card.categories[0]?.name ?? t('other'));

Fix — extract a local helper (preferred inside useMemo / loops):

const labelFor = (id: string, card: Card): string => {
  if (id === CUSTOM) return t('custom');
  if (id === OTHER) return t('other');
  return card.categories[0]?.name ?? t('other');
};

Fix — if/else if chain when no helper reuse is needed:

let label: string;
if (id === CUSTOM) label = t('custom');
else if (id === OTHER) label = t('other');
else label = card.categories[0]?.name ?? t('other');

One-level ternaries (isX ? a : b) are fine. The rule kicks in the moment a ternary sits in either branch of another ternary.

2. HTML semantics — no nested interactive elements

HTML5 forbids <button> (or another <a>) as a descendant of <a>. e.preventDefault() masks the click but screen readers still double-announce and keyboard focus lands twice.

Anti-pattern:

<Link href="/foo">
  <EntityCard footer={<Button => e.preventDefault()}>Delete</Button>} />
</Link>

Correct approaches:

• Use DS primitives that expose an href prop and render the anchor internally (e.g. a future EntityCard with href support).
• Or: make only the title/content a <Link> and render action buttons as siblings outside the anchor.
• Pick one convention and apply it to all cards in the same surface (don't mix MyIntegrationsGrid / AvailableIntegrationsGrid).

3. External <img> hygiene

Any <img> whose src comes from user/admin input (avatar URLs, integration logos, uploaded assets) must set:

<img
  src={userSuppliedUrl}
  alt={label}
  loading="lazy"
  referrerPolicy="no-referrer"
  ...
/>

• loading="lazy" — rows below the fold defer the fetch.
• referrerPolicy="no-referrer" — prevents leaking the dashboard path to whatever host serves the asset.

4. Utility consistency sweep

When introducing a new helper (e.g. formatPhoneNumber, truncateText), grep the whole apps/web/src/** tree for all places that could benefit and migrate them in the same PR.

Reviewers will flag it as "visual inconsistency" if one card formats but another still renders raw.

Checklist before opening the PR:

• rg "{thing the utility replaces}" apps/web/src — zero remaining raw uses
• Add Vitest unit tests for the utility (apps/web/src/lib/*.test.ts)

5. DS EntityCard conventions

When migrating a card to EntityCard:

• Pass title as a string whenever possible. Strings get the built-in isTitleTruncated + hover-tooltip behaviour; ReactNode titles bypass it. If you need mono / special styling, apply it via the DS prop system, not by wrapping in a custom <h3>.
• Drop mt-1 / bespoke spacing on subtitle. EntityCard's .entity-card-header-content already stacks title-row + subtitle. Adding mt-1 only on some cards creates visual drift across grid pages.
• Set interactive={false} for read-only cards (no <Link> / <button> wrapper).
• Place the card inside a parent with className="group" so the built-in title-hover colour and arrow-indicator activation fire.

6. iconVariant semantic rule

Per-card choice of iconVariant must follow the semantic rule, not a coin flip:

7. Visual state — single source of truth

Don't derive a card's visual state from the combination of multiple fields.

Anti-pattern:

const isFullyActive = isActive && isAssigned;
// shell tone, icon variant, status dot all toggled on isFullyActive

Fix: pick the canonical field (usually status) and drive shell tone / icon variant / StatusDot off it. Use the secondary field (e.g. isAssigned) only for structural content (which footer action to render), not visual state.

8. DRY shared components

If two or more cards render the same status indicator / helper markup, extract to apps/web/src/components/shared/* in the same PR — don't half-adopt. Specifically:

• StatusDot → @/components/shared/StatusDot
• Remove any re-export from feature barrels (e.g. integrations/index.ts) so there's exactly one import path.

9. KISS — reject heavy deps for narrow needs

Before adding an npm dep (especially UI / formatting libs), ask:

• What's the gzipped bundle cost? (libphonenumber-js ≈ 130 KB gzipped for what ends up being 2 country formats.)
• Does the happy path pass with a pure 10-line helper + tests?
• Is fallback-to-raw acceptable for the long tail?

If yes / yes / yes → KISS helper wins. Add the dep only when the feature actually needs parse / validate / i18n semantics.

10. URL ↔ local state sync patterns

When a search input needs both instant feedback (controlled input) and URL bookmarkability (searchParams):

Recommended pattern (used in IntegrationsPageContent):

const [search, setSearch] = useState(() => searchParams.get('search') ?? '');
const deferredSearch = useDeferredValue(search);

// Sync back from external URL changes (back/forward)
useEffect(() => {
  const urlSearch = searchParams.get('search') ?? '';
  setSearch((prev) => (prev === urlSearch ? prev : urlSearch));
}, [searchParams]);

// Update both local state and URL on input
const handleSearchChange = useCallback((value: string) => {
  setSearch(value);
  updateParams({ search: value || undefined });
}, [updateParams]);

Key points:

• useState + controlled input for instant feedback; useDeferredValue defers expensive rendering
• useEffect syncs from searchParams on back/forward navigation
• router.replace (not push) avoids accumulating history entries
• The prev === urlSearch guard is sufficient — Next.js's action queue discards superseded navigations, so searchParams never shows intermediate values during fast typing (verified in app-router-instance.js source)
• Pass deferredSearch (not search) consistently to all child components that render filtered data
• Alternative pattern (Next.js official tutorial): useDebouncedCallback + uncontrolled defaultValue input. Simpler but adds 300ms delay to input feedback. Prefer the controlled pattern when instant responsiveness matters.

11. React effect hydration / persistence scheduling

For effects that hydrate persisted state (chat sessions, URL-backed state, localStorage-backed selections), separate:

• Trigger values: route params, namespace keys, entity ids — values that mean the user is now in a different logical place.
• Latest values read by the effect: callbacks, translation functions, sessionId, refs — values the effect needs to use, but whose identity/value changes should not by itself restart hydration.

Use useEffectEvent when the effect should react only to trigger values but must call the latest callbacks.

Anti-pattern:

useEffect(() => {
  resetChat();
  void loadSession(storedId);
}, [scopedPersistKey, loadSession, resetChat]);

If loadSession / resetChat close over useTranslations() or other unstable deps, this re-runs hydration on ordinary renders, causing reset/load loops and repeated SSE reconnects.

Preferred pattern:

const string) => {
  resetChat();
  void loadSession(readPersistedSession(nextPersistKey));
});

useEffect(() => {
  if (!scopedPersistKey) return;
  return onHydrate(scopedPersistKey);
}, [scopedPersistKey]);

When writing these effects, verify:

• A state value synchronously set by the effect's own async action (for example loadSession() setting sessionId) is not also a dependency that self-cancels the effect.
• Every markNamespacePending(...) path has a matching markNamespaceReady(...) path, including early returns and stale-state fall-through branches.
• Changing namespace while a previous async load is in flight cannot leave hydrating stuck because the old .finally() was skipped by cancelled=true.
• Regression tests cover unstable callback identity and namespace/context switches when the bug involves persistence or hydration scheduling.

12. Scope discipline

If a reviewer suggests extending the refactor to files outside the current PR's scope (e.g. ReportConfigCard.tsx still on .assistant-card):

• Acknowledge the point.
• Track as a follow-up ticket, not a last-minute addition.
• State the reason on the PR (avoid scope creep / avoid a divergent half-migration).

13. Abort detection in fetch catch blocks

When catching errors from fetch() (hooks, route handlers, SSE/streaming proxies), discriminate on the error shape, not global signal state. This is a concrete case of Rule §1 "swallowing expected errors" — AbortError is the expected noise we want to silence.

Anti-pattern (race condition — silently hides real failures):

try {
  const upstream = await fetch(url, { signal: request.signal });
} catch (err: unknown) {
  if (request.signal.aborted) return new Response(null, { status: 499 });
  throw err;
}

request.signal.aborted returns true for any error that happens after the client disconnected — including genuine upstream failures (DNS, ECONNREFUSED, TLS, timeout). Those get silently swallowed as 499 instead of reaching Sentry.

Correct pattern (matches useChat.ts:605, useChat.ts:723, query-retry.ts:43):

try {
  const upstream = await fetch(url, { signal: request.signal });
} catch (err: unknown) {
  if (err instanceof DOMException && err.name === 'AbortError') {
    return new Response(null, { status: 499 });
  }
  throw err;
}

Node.js (undici) and browsers both throw DOMException with name === 'AbortError' when a fetch is aborted via signal — this is the Web spec-compliant shape. Other errors (TypeError for DNS, generic Error for ECONNREFUSED) still propagate to Sentry.

For route handlers (apps/web/src/app/api/**/route.ts):

• Return 499 (Nginx convention: "Client Closed Request"). AWS ALB surfaces this as 460 in access logs; Datadog / Sentry default dashboards treat 499 as non-error so it doesn't pollute error-rate SLOs.
• Do NOT use 200, 204, 408, or 500 — each is semantically wrong (wasn't successful / wasn't a timeout / wasn't a server error).
• Do NOT add a console.warn on this branch just for "visibility". If reviewers request it, push back: the whole point of swallowing expected noise is reducing noise, and LB access logs already show the 499/460 count. Per AGENTS.md: "Do NOT log-and-rethrow — the boundary already logs."

Test coverage is mandatory. When adding a try/catch to a route handler, add a route.test.ts covering:

1. Abort path: global.fetch mocked to throw new DOMException('aborted', 'AbortError') → response status is 499
2. Re-throw path (the race-condition guard): fetch throws new Error('ECONNREFUSED') → await GET(...) rejects with the same error
3. Happy path + early-exit guards (400 for bad input, 401 for missing token, upstream non-OK passthrough)

Template: apps/web/src/app/api/csp-report/route.test.ts. Use vi.mock('@/lib/api-config') for auth and vi.stubGlobal('fetch', ...) for the upstream.

Without the re-throw test (#2), a future refactor that widens the catch or switches back to signal.aborted silently reintroduces the original bug.

14. ?? vs || for empty-string-default fields

?? only catches null / undefined. Empty strings ('') slip through and are passed downstream verbatim.

Anti-pattern (PR #1072):

const vapiVoiceId = flags.aetheronVapiVoiceId ?? defaultFlags.vapiVoiceId;
// If Flagsmith returns '', vapiVoiceId becomes '', and
// getVoiceLabel(id, { defaultVoiceId: '' }) short-circuits the new branch.

Fix — choose by intent:

// (a) treat blank as "use the default":
const vapiVoiceId = flags.aetheronVapiVoiceId || defaultFlags.vapiVoiceId;

// (b) normalize at the read site, then pass through:
const raw = flags.aetheronVapiVoiceId;
const vapiVoiceId = raw === '' ? defaultFlags.vapiVoiceId : raw;

Trigger scenarios where this matters:

• Reads from @aetheron/feature-flags (str() returns the raw string verbatim, including '')
• API responses where backend has not promoted the column to NOT NULL
• Form submits where a cleared select returns '' instead of undefined

15. Default-prop-cascade footgun in shared layouts

When a shared layout component has a permissive default (e.g. vapiVoiceId = ''), every parent mount site that doesn't pass the real value silently lands in the fallback branch.

Anti-pattern (PR #1072):

// AppLayout.tsx
export function AppLayout({ vapiVoiceId = '' }: Props) { ... }

// app/(dashboard)/layout.tsx — passes the real flag
<AppLayout vapiVoiceId={flags.aetheronVapiVoiceId ?? defaultFlags.vapiVoiceId} ... />

// app/demo/layout.tsx — forgets to pass it; renders raw id forever
<AppLayout>{children}</AppLayout>

Fix — pick one:

(a) Make the prop required so TS surfaces every forgotten call site:

type Props = { vapiVoiceId: string; ... };

(b) Pass the real default at every mount site (demo/layout.tsx etc. should read defaultFlags.vapiVoiceId server-side and forward it).

Audit checklist when adding a feature-flag-fed prop to a layout:

• rg "<AppLayout" (or whichever layout) — every match passes the prop
• If the prop is string rather than string | null, prefer required over a permissive default

16. Cross-component consistency sweep when fixing a UI symptom

When fixing a "renders raw value instead of human-readable label" bug in one component, search the codebase for OTHER components rendering the same data shape. Reviewers will flag a sibling component that still has the symptom.

Example (PR #1072): AssistantDetail.tsx was fixed to render getVoiceLabel(voiceId, { defaultVoiceId: vapiVoiceId }). The reviewer found VoiceOverrideIndicator.tsx:40 directly below it still rendered the raw 4Hm8… id in a font-mono span.

Process:

• rg the data shape (e.g. voiceId, voiceOverride, font-mono.*Id), not the component name
• For every match, decide: pipe through the same helper, hide when === defaultId, or document why it stays raw
• Land all of them in the same PR — a half-migrated symptom is worse than the original because reviewers now have to track two states

17. Render test for user-visible "wiring" fixes

Even when the fix is just "pass a prop / resolve a value", add a *.test.tsx that mounts the component end-to-end and asserts the user-visible string. Without it, anyone dropping the hook call, the prop, or the options-bag key would ship green.

Pattern (PR #1072):

it('shows default voice label when assistant uses default voiceId', () => {
  render(
    <FeatureFlagProvider vapiVoiceId='4Hm8iqw2xCZz1wwdml3H'>
      <AssistantDetail assistant={{ ...base, voiceId: '4Hm8iqw2xCZz1wwdml3H' }} />
    </FeatureFlagProvider>,
  );
  expect(screen.getByText('Aetheron Australian')).toBeInTheDocument();
});

Apply when the bug fix is any of:

• A new prop being threaded through a layout / context provider
• A hook call (e.g. useFeatureFlags()) being added to a component
• A new options-bag key (e.g. { defaultVoiceId }) being passed to a util

18. vi.unstubAllGlobals() cleanup convention

When a test stubs global.fetch (or any other global) with vi.stubGlobal, add afterEach(() => vi.unstubAllGlobals()) to match the convention in apps/web/src/app/api/auth/proxy/proxy.test.ts:102-104 and apps/web/src/hooks/useChat.test.ts:114-116.

afterEach(() => {
  vi.unstubAllGlobals();
});

Vitest's default isolate: true makes this safe today, but explicit cleanup is a free guarantee against flakiness if a future config flip removes isolation.

19. Lock down the upstream request shape in route-handler tests

Route-handler tests that only assert response status leave the upstream call shape untested. Dropping the Authorization header, switching to a wrong Accept header, or removing signal: request.signal would all keep the suite green.

Pattern (PR #1038):

expect(global.fetch).toHaveBeenCalledWith(
  expect.stringContaining('/stream'),
  expect.objectContaining({
    headers: expect.objectContaining({
      Authorization: 'Bearer test-token',
      Accept: 'text/event-stream',
    }),
    signal: expect.any(AbortSignal),
  }),
);

For conditional headers (e.g. Last-Event-ID only forwarded when present), add a negative assertion so a refactor to always-include doesn't silently send null:

it('does not include Last-Event-ID header when absent from inbound request', async () => {
  // ...
  expect(global.fetch).toHaveBeenCalledWith(
    expect.anything(),
    expect.objectContaining({
      headers: expect.not.objectContaining({ 'Last-Event-ID': expect.anything() }),
    }),
  );
});

20. AbortSignal.timeout() throws TimeoutError, not AbortError

Heads-up for Rule §13: if you ever wrap a fetch with AbortSignal.timeout(ms) (or compose it via AbortSignal.any([signal, timeoutSignal])), the abort discriminator must accept both names:

if (
  err instanceof DOMException &&
  (err.name === 'AbortError' || err.name === 'TimeoutError')
) {
  return new Response(null, { status: 499 });
}

AbortSignal.timeout() rejects with DOMException name === 'TimeoutError'. The bare AbortError discriminator silently lets timeouts propagate to Sentry as 500s. Reference pattern: apps/api/src/lib/proxy.ts:210.

21. Layout pattern consistency between paired containers

When a page has paired states (loading / error / empty / content), all four containers must share the same className shape. Dropping a class on one of them creates a visual jolt during state transitions and shows up immediately on the support banner edge case.

Anti-pattern (PR #990):

if (isLoading) return <div className='flex flex-1 min-h-0'><Loading /></div>;
if (error)     return <div className='flex-1 min-h-0'><Alert /></div>;
//                                  ^ missing `flex` — content jumps when transitioning
return <div className='flex flex-1 min-h-0'>{children}</div>;

Fix: extract the shared shell, or copy the className verbatim across all four containers. Diff the className strings character-by-character before opening the PR.

22. Intl.DateTimeFormat(undefined, ...) causes SSR/client locale drift

Date.prototype.toLocaleTimeString(undefined, opts) and new Intl.DateTimeFormat(undefined, opts) both fall back to the runtime default locale. On Node (SSR) that's typically en-US from the container; in the browser it's the user's navigator.language. The same formatHourLabel(13) call renders 1:00 PM server-side and 13:00 client-side → hydration mismatch and inconsistent labels across page sections.

Anti-pattern (PR #869):

export function formatHourLabel(hour: number): string {
  const date = new Date(2000, 0, 1, hour);
  return date.toLocaleTimeString(undefined, { hour: 'numeric', hour12: true });
}

Fix — accept an explicit locale param:

export function formatHourLabel(hour: number, locale: string): string {
  const date = new Date(2000, 0, 1, hour);
  return date.toLocaleTimeString(locale, { hour: 'numeric', hour12: true });
}

// Client component
const locale = useLocale();
formatHourLabel(13, locale);

// Server component
const locale = await getLocale();
formatHourLabel(13, locale);

Reference pattern: apps/web/src/components/dashboard/ActivityFeed.tsx. Prefer next-intl's useFormatter() / getFormatter() when available — they pin the locale automatically.

23. Counter-based alternating mocks are fragile

Don't use a module-level counter to alternate between two mock return values. The test silently breaks the moment the component reorders or adds a third hook call.

Anti-pattern (PR #869):

let mutateCallIndex = 0;
vi.mock('@/hooks/useActionMutate', () => ({
  useActionMutate: vi.fn(() => ({
    mutateAsync: mutateCallIndex++ % 2 === 0 ? mockCreate : mockUpdate,
    isPending: false,
  })),
}));

Fix — match on argument identity:

vi.mock('@/hooks/useActionMutate', () => ({
  useActionMutate: vi.fn((action: unknown) => ({
    mutateAsync: action === createReportConfig ? mockCreate : mockUpdate,
    isPending: false,
  })),
}));

The argument-identity match is stable across hook reorders and surfaces the intent inline.

24. Don't mix raw <label> with DS FormField in the same form

When a form uses DS FormField / field.SelectField / field.TextareaField for most rows, every row must use the same wrapper — including composite inputs (chip list + input + button).

Anti-pattern (PR #869):

<FormField label={t('form.title')} hint={t('form.titleHint')}>
  <Input ... />
</FormField>

{/* one composite row sneaks in raw <label> */}
<div>
  <label htmlFor='recipients' className='text-field-name text-base-content'>
    {t('form.recipients')}
  </label>
  <Text className='mt-1 text-sm'>{t('form.recipientsHint')}</Text>
  <RecipientsChipInput id='recipients' ... />
</div>

Fix: wrap the composite in FormField too:

<FormField label={t('form.recipients')} hint={t('form.recipientsHint')}>
  <RecipientsChipInput ... />
</FormField>

This keeps label / hint typography in sync when the DS bumps --text-field-name or related tokens.

25. Clearable selects + invalid empty values

DS SelectField with clearable behaviour returns '' when the user clears the dropdown. For fields where '' is not a valid value (IANA timezone, currency code, ISO country, etc.), field.handleChange(v ?? '') ships an invalid empty string to the API.

Anti-pattern (PR #869):

<field.SelectField
  options={timezones}
  => field.handleChange(v ?? '')} // '' reaches API → 400
/>

Fix — block empty by ignoring the clear:

<field.SelectField
  options={timezones}
  => { if (v) field.handleChange(v); }}
/>

Or remove the clear affordance for required fields. Pair with a nonEmpty validator on submit so the backend isn't the safety net.

26. Don't thread t / formatDate callbacks into Server Components

If a child is a Server Component, it can call getTranslations() / getFormatter() directly via next-intl/server. Threading callbacks through props forces the parent (or some ancestor) to be a Client Component just to provide them, cascading 'use client' unnecessarily and adding bespoke prop types.

Anti-pattern (PR #869):

type Props = {
  t: (key: string) => string;
  formatDate: (d: Date) => string;
  config: ReportConfigDto;
};

export function ReportConfigDetail({ t, formatDate, config }: Props) { ... }

Fix — make the child async and resolve in-place:

import { getTranslations, getFormatter } from 'next-intl/server';

export async function ReportConfigDetail({ config }: { config: ReportConfigDto }) {
  const t = await getTranslations('common.reports');
  const format = await getFormatter();
  // ...
}

Now 'use client' can drop off the parent and the bespoke callback prop types disappear.

27. Form values + useState is a duplicate source of state

Don't track the same field in both defaultValues (form state) and a separate useState. The two diverge the moment one writer skips the other.

Anti-pattern (PR #869):

type ReportConfigFormValues = {
  // ...
  recipients: string[]; // form-state field, never read
};

const [recipients, setRecipients] = useState<string[]>([]); // local-state field, actually used
const ReportConfigFormValues) => {
  saveConfig({ ...values, recipients }); // form-state value silently overridden
};

Fix — pick one:

(a) Fully in form state — register the field, use field.handleChange, drop the useState.

(b) Fully in local state — remove recipients from ReportConfigFormValues and merge in onSubmit only.

Same rule for arrays, draft text, and any "complex" field where a custom UI feels easier than form integration.

28. createdAt vs updatedAt fallback for "last modified" displays

New rows have createdAt populated but updatedAt is often NULL until the first edit (depending on migration / trigger setup). Rendering a "Last updated" footer from updatedAt alone shows blank / "Invalid Date" for fresh entities.

Anti-pattern (PR #869):

<Text className='text-base-content-soft'>
  {t('detail.lastUpdated', { date: formatDate(config.updatedAt) })}
</Text>

Fix — fall back to createdAt (or render both with explicit labels):

<Text className='text-base-content-soft'>
  {t('detail.lastUpdated', { date: formatDate(config.updatedAt ?? config.createdAt) })}
</Text>

Verify against the actual migration: if the DB sets updatedAt = createdAt on insert via trigger, fallback isn't needed — but read the migration before assuming.

29. Client maxLength must mirror backend caps

If the API caps a string field at N chars (e.g. reportInstructions at 4000, reportPrompt at 16000), set maxLength={N} on the input. Without it, the user types the whole essay, hits Save, and learns from a backend 400.

Pattern (PR #869):

<field.TextareaField
  label={t('form.reportInstructions')}
  maxLength={4000} // matches API cap in apps/api/src/schemas/report-config.ts
  rows={4}
/>
<field.TextareaField
  label={t('form.reportPrompt')}
  maxLength={16000}
  rows={8}
/>

When changing a backend cap, rg "{cap}" to find the mirroring frontend constant and update both in the same PR.

30. Test case-insensitive validation and max caps

When a form does case-insensitive dedup (.toLowerCase()) or enforces a MAX_* cap, both invariants need explicit tests. The implementation is easy to delete in a refactor and silently regress.

Pattern (PR #869):

it('rejects case-insensitive duplicate recipient', async () => {
  const { user } = renderForm();
  await addRecipient(user, 'a@b.com');
  await addRecipient(user, 'A@B.COM');
  expect(screen.getByText(t('validation.duplicateEmail'))).toBeInTheDocument();
});

it('disables Add button at max recipients cap', () => {
  renderForm({ defaultValues: { recipients: Array(20).fill(0).map((_, i) => `r${i}@example.com`) } });
  expect(screen.getByRole('button', { name: t('form.addRecipient') })).toBeDisabled();
});

31. ?? null is a no-op on T | null codegen fields

kysely-codegen already types nullable columns as T | null. row.titleSource ?? null is dead code — remove for consistency with the surrounding mapper.

Anti-pattern (PR #992):

return {
  id: row.id,
  title: row.title,                       // typed string | null already
  titleSource: row.titleSource ?? null,   // ← redundant
  sourceKey: row.sourceKey,
};

Fix:

return {
  id: row.id,
  title: row.title,
  titleSource: row.titleSource,
  sourceKey: row.sourceKey,
};

If you need to coerce undefined → null, the column type is wrong upstream — fix the schema, not every read site.

32. Date-only strings need timeZone: 'UTC' when rendered

new Date('YYYY-MM-DD') parses as UTC midnight, not local. format.dateTime / toLocaleDateString then renders in the runtime locale's timezone, so users west of UTC see the previous calendar day.

Anti-pattern (PR #1068):

const start = format.dateTime(new Date(report.periodStart), {
  month: 'short',
  day: 'numeric',
}); // user in UTC-5 sees April 12 for periodStart='2026-04-13'

Fix — pin the timezone explicitly:

const start = format.dateTime(new Date(report.periodStart), {
  month: 'short',
  day: 'numeric',
  timeZone: 'UTC',
});

Apply everywhere a date-only string from the API is rendered. The fix is the same pair of surfaces in this PR (WeeklyReportList.formatPeriod + WeeklyReportViewer.periodLabel) — when you see one, grep the codebase for siblings.

Decision rule: if the string has no timezone/offset ('2026-04-13', '2026-04-13T00:00:00'), pin timeZone: 'UTC'. If it has an offset ('2026-04-13T00:00:00+10:00'), local rendering is fine.

33. useMemo of "current time" doesn't refresh on modal reopen

useMemo(() => somethingTimeBased(), []) snapshots the value when the component first mounts. Long-lived tabs cross midnight and the modal still shows yesterday's defaults.

Anti-pattern (PR #1068):

const defaults = useMemo(() => lastCompletedWeek(new Date()), []);
const [periodStart, setPeriodStart] = useState(defaults.start);

useEffect(() => {
  if (!open) return;
  setPeriodStart(defaults.start); // still last week's stale value
}, [open]);

Fix — recompute inside the open-reset effect:

useEffect(() => {
  if (!open) return;
  const fresh = lastCompletedWeek(new Date());
  setPeriodStart(fresh.start);
  setPeriodEnd(fresh.end);
}, [open]);

Trigger scenarios: modal/dialog defaults derived from current time, "today" pickers, "this week" filters, recently-viewed shortlists.

34. Three-decision checklist for every useQuery / useMutation

Before declaring a query/mutation done, answer all three questions explicitly. Defaults bite hardest when interval × retry × component count multiplies.

Anti-pattern (PR #1068):

return useQuery({
  queryKey: ['report-poll', id],
  queryFn: () => getWeeklyReport({ path: { id } }),
  refetchInterval: 10_000, // ← 10s × default retry: 4 = 4 captures per error per viewer
});

Fix:

return useQuery({
  queryKey: ['weekly-reports', id],
  queryFn: async () => { /* + Sentry capture inside */ },
  refetchInterval: (q) =>
    q.state.data?.status === 'pending' || q.state.data?.status === 'processing'
      ? 10_000
      : false,
  retry: false,
  meta: { suppressGlobalError: true },
});

35. meta.suppressGlobalError is asymmetric — verify both caches honour it

QueryClient exposes two error pipelines: MutationCache.onError and QueryCache.onError. The meta.suppressGlobalError convention is opt-in per cache — wiring it on one without the other makes the flag dead code on the other side.

Anti-pattern (PR #1068, before fix):

// QueryProvider.tsx
const queryClient = new QueryClient({
  mutationCache: new MutationCache({
    onError: (error, _vars, _ctx, mutation) => {
      if (mutation.options.meta?.suppressGlobalError) return; // ← only honoured here
      handleQueryError(error);
    },
  }),
  queryCache: new QueryCache({
    onError: (error) => handleQueryError(error), // ← always fires globally
  }),
});

// useReportContent.ts — flag is dead code
return useQuery({ /* ... */, meta: { suppressGlobalError: true } });

Fix — mirror the bypass in QueryCache.onError:

queryCache: new QueryCache({
  onError: (error, query) => {
    if (query.meta?.['suppressGlobalError']) {
      console.warn('[Query Error - handled locally]', error);
      return;
    }
    handleQueryError(error);
  },
}),

Review-time check: when you see meta: { suppressGlobalError: true } on a useQuery, grep QueryProvider.tsx for the QueryCache.onError branch. If it's missing, the flag is doing nothing.

36. Status enum from backend is forward-compat — UI must have a default branch

Backend status columns grow over time (pending | processing | completed | failed → adds cancelled next quarter). UI switch / multi-if chains that exhaustively match the four current literals render blank for any future status the UI hasn't been taught about.

Anti-pattern (PR #1068):

if (status === 'pending') return <Pending />;
if (status === 'processing') return <Processing />;
if (status === 'completed') return <Completed />;
if (status === 'failed') return <Failed />;
// future 'cancelled' renders nothing

Fix — explicit default with a placeholder:

if (status === 'pending') return <Pending />;
if (status === 'processing') return <Processing />;
if (status === 'completed') return <Completed />;
if (status === 'failed') return <Failed />;
return <CardValue empty>{tViewer('noContent')}</CardValue>;

Apply when: rendering badges, content cards, or icons keyed off a backend enum string. Same rule for WeeklyReportStatusBadge-style components — at least pass the raw string through as a fallback label.

Test it: cast a fictional status ('cancelled' as unknown as Status) into the component and assert the default branch renders.

37. if (data) truthy check vs typeof data === 'string' — empty strings are valid content

For optional content that may legitimately be empty (markdown for a week with no calls, comment for a row with nothing flagged), a truthy check funnels data === '' into the error/empty branch and the user sees "load failed" for healthy data.

Anti-pattern (PR #1068):

if (contentQuery.data) return <MarkdownViewer content={contentQuery.data} />;
return <ContentLoadError />; // empty markdown lands here

Fix — discriminate on type:

if (typeof contentQuery.data === 'string') return <MarkdownViewer content={contentQuery.data} />;
return <ContentLoadError />; // only `null` / `undefined` reach here

Decision table: when the union is string | null and '' is a valid value, use typeof === 'string' (or data !== null). When the union is string | undefined and the difference doesn't matter, truthy is fine — but document the assumption.

Trigger scenarios: markdown / HTML / plain-text content from blob storage, optional descriptions / notes / instructions, transcripts that may be silent.

38. listX(limit:100) for a single name lookup is the wrong tool — use getX(id)

Pages that fetch a list only to translate one foreign-key id into a name pay the cost of the full list AND silently lose the lookup once the org crosses the cap.

Anti-pattern (PR #1068):

const [config, assistants] = await Promise.all([
  getReportConfig({ path: { id } }),
  listAssistants({ query: { limit: 100 } }),
]);
const assistantMap = new Map((assistants.data?.items ?? []).map((a) => [a.id, a.name]));
const assistantName = assistantMap.get(config.assistantId) ?? config.assistantId; // bare UUID for 101st+

Fix — single-record fetch:

const result = await getReportConfig({ path: { id } });
const config = result.data;
let assistantName: string | null = null;
if (config.assistantId) {
  const assistantResult = await getAssistant({ path: { id: config.assistantId } });
  assistantName = assistantResult.data?.name ?? t('reports.unknownAssistant');
}

Trigger scenarios: any detail / viewer page that fetches a list to render a single label or breadcrumb. Common offenders:

• listAssistants({ limit: 100 }) for one assistantId
• listVoices({ limit: 100 }) for one voiceId
• listOrgs(...) for one orgId on a member detail page

Review-time grep: rg "list\w+\(\{ query: \{ limit:" apps/web/src/app — for each match, ask "is this for a list view, or is it a one-record lookup pretending to be a list?"

Don't fix in this PR if: the page genuinely renders the list AND uses one entry from it. Then the cap is a separate problem (paged select, listAll helper) — flag as follow-up.

39. Mutation success ≠ "toast and done" — every error branch needs the three-thing check

Every

…(truncated)