Dashboard App Code Review — v3

Historical note: This review predates the Better Auth → JWT migration (April 2026). Any findings about @/lib/auth, authClient, or auth.api.getSession() are now resolved — the dashboard uses requireSession() / getSession() from @/lib/server-auth and loginAction() / logoutAction() from @/app/actions/auth.ts.

Scope: apps/dashboard/src/ — full review including all pages, components, lib, API routes, and server actions
Date: 2026-04-21
Reviewer: Claude Sonnet 4.6
Based on: dashboard-code-review-v2.md (2026-04-17)
Status: Open — not yet actioned

Summary

Progress Since v2

Files Reviewed

High

H-1 — JWT_SECRET Falls Back to "dev-secret" — Now Five Locations

Files:

• src/app/api/socket-token/route.ts:23
• src/app/(dashboard)/channels/page.tsx:25
• src/app/(dashboard)/client-info/page.tsx:33
• src/app/(dashboard)/ai-chat/page.tsx:24
• src/app/(dashboard)/insights/page.tsx:35 ← new since v2

Priority: Fix before next deployment — predictable fallback makes all tenant-scoped JWTs forgeable

Issue:

All five paths share the same pattern:

const token = jwt.sign(
  { sub: session.user.id, tenantId: user.tenantId, role: user.role ?? "admin" },
  process.env.JWT_SECRET ?? "dev-secret",
  { expiresIn: "1h" },
);

insights/page.tsx was added since v2 and introduces the same vulnerability. If JWT_SECRET is absent in any environment (staging, preview, CI), tokens are signed with the string "dev-secret". An attacker who knows this fallback can forge a valid JWT for any tenantId and call API server endpoints directly. The insights endpoint (/tenant/v1/insights/*) grants access to the tenant’s channel insight data.

Fix (unchanged from v2):

Create a shared guard that throws on missing secret:

// src/lib/jwt.ts
export function getJwtSecret(): string {
  const secret = process.env.JWT_SECRET;
  if (!secret) throw new Error("JWT_SECRET environment variable is not set");
  return secret;
}

Replace all five process.env.JWT_SECRET ?? "dev-secret" occurrences with getJwtSecret(). Document JWT_SECRET as required in .env.example and deployment runbooks.

Medium

M-1 — getDashboardStats Stage Logic Skips Stage 3; Onboarding Step Never Shown

File: src/lib/dashboard.ts:49–56
Priority: Medium — onboarding wizard step 3 (“Goals & KPIs are set”) is never the active step

Issue (unchanged from v2):

let stage = 1;
if (isContextComplete) {
  if (goalCount === 0)            stage = 2;
  else if (totalActivities === 0) stage = 4; // ← skips 3
  else                            stage = 5;
}

GettingStartedView (comment: stages 1–3) and RegularDashboard (comment: stage 4 — agents are working) are designed around a 1–5 model, but stages 3 is never emitted and RegularDashboard is gated at stage >= 5. A tenant who sets their first goals transitions immediately from step 2 active to step 4 active, skipping the “Goals & KPIs are set” step entirely. The bannerText case for stage > 2 — “Goals & KPIs are set — agents will start working soon” — is unreachable.

Fix (unchanged from v2):

let stage = 1;
if (isContextComplete) {
  if (goalCount === 0)            stage = 2;
  else if (totalActivities === 0) stage = 3; // goals set, no activities yet
  else                            stage = 4;
}

Update DashboardPage to show RegularDashboard at stage >= 4 and update the JSDoc comment.

M-2 — Timezone Inconsistency Between calendar/page.tsx, activities/page.tsx, and goals/page.tsx

Files:

• src/app/(dashboard)/calendar/page.tsx:27–29 (local time)
• src/app/(dashboard)/activities/page.tsx:22–23 (local time)
• src/app/(dashboard)/goals/page.tsx:55–56 (local time)

Priority: Medium — month-boundary dates can be misclassified in UTC+ timezones; goals/page.tsx can silently return zero activity counts

Issue:

The v2 review identified this inconsistency between calendar/page.tsx (local time) and deliverables/page.tsx (UTC). deliverables/page.tsx is correct. Two more pages have been found to use the same local-time pattern:

calendar/page.tsx builds its initial month range with local time:

const startDate = new Date(year, monthNum - 1, 1);
const endDate   = new Date(year, monthNum, 1);

activities/page.tsx computes monthly stats bounds with local time:

const monthStart = new Date(now.getFullYear(), now.getMonth(), 1);
const monthEnd   = new Date(now.getFullYear(), now.getMonth() + 1, 0, 23, 59, 59);

goals/page.tsx looks up the current DeliverablePeriod with local-time bounds:

const periodStart = new Date(now.getFullYear(), now.getMonth(), 1);
const periodEnd   = new Date(now.getFullYear(), now.getMonth() + 1, 0, 23, 59, 59);
// ...
const currentPeriod = await db.deliverablePeriod.findFirst({
  where: {
    periodStart: { gte: periodStart },
    periodEnd:   { lte: periodEnd },
  },
});

DeliverablePeriod.periodStart is stored as UTC midnight. On a server running in UTC+5:30, local midnight is 18:30 UTC the previous day, so new Date(year, month, 1) evaluates to 2026-05-31T18:30:00Z. The period stored as 2026-05-01T00:00:00Z has periodStart < periodStart_local, so the { gte: periodStart } filter excludes it. The goals page returns zero activity progress all month.

Fix:

Extract a shared UTC month-range helper:

// src/lib/date-utils.ts
export function getMonthRangeUTC(year: number, month: number) {
  // month is 1-based
  return {
    start: new Date(Date.UTC(year, month - 1, 1)),
    end:   new Date(Date.UTC(year, month, 1)),   // exclusive upper bound
  };
}

Replace local-time construction in calendar/page.tsx, activities/page.tsx, and goals/page.tsx with getMonthRangeUTC. Use lt: end (exclusive) consistently rather than mixing 0, 23, 59, 59 inclusive with exclusive upper bounds.

M-3 — PresenceProvider Calls destroySocket() on Unmount, Killing the Shared Singleton

File: src/components/chat/PresenceProvider.tsx:97
Priority: Medium — unchanged from v2

Issue (unchanged from v2):

return () => {
  cancelled = true;
  if (heartbeatRef.current) clearInterval(heartbeatRef.current);
  const s = socketRef.current;
  if (s) {
    s.off("connect");
    // ... other event removals ...
  }
  destroySocket();  // ← still present
};

destroySocket() tears down the module-level singleton shared by TopBar, AgentStatusPanel, StrategyAgentPanel, and DeliverablePlannerAgentPanel. After destruction, the next getSocket() call creates a new socket instance without an auth token, causing the API server to reject it.

Fix (unchanged from v2): Remove destroySocket() from the PresenceProvider cleanup. The event listeners are already removed individually. Socket lifecycle should only be managed on explicit sign-out in handleSignOut.

M-4 — db/seed.ts Password "admin" Fails minPasswordLength: 8

File: src/db/seed.ts:9
Priority: Medium — pnpm --filter dashboard db:seed exits with process.exit(1)

Issue (unchanged from v2):

const result = await auth.api.signUpEmail({
  body: { email: "admin@admin.com", password: "admin", name: "Admin" }
});

auth.ts enforces minPasswordLength: 8. Better Auth rejects "admin" (5 characters). The catch block only suppresses "already" errors, so the password-too-short error surfaces as "❌ Failed to seed admin user" and exits.

Fix: Update the seed password to meet the minimum requirement, e.g. password: "admin123".

M-5 — PaymentBanner Links to /settings/billing Which Does Not Exist

File: src/app/(dashboard)/PaymentBanner.tsx:31
Priority: Medium — clicking “Pay Now” leads to a 404 at the critical moment of payment

Issue (unchanged from v2):

<Link href={`/settings/billing?invoice=${invoiceId}`} ...>
  Pay Now
</Link>

/settings/billing is still not a route. The settings directory contains only agents/, context/, notifications/, profile/, users/.

Fix: Either create src/app/(dashboard)/settings/billing/page.tsx or redirect to an existing billing section such as /client-info?tab=billing&invoice=${invoiceId}.

M-6 — reset-password/page.tsx Tells Users Minimum is 4 Characters; Auth Enforces 8

File: src/app/(auth)/reset-password/page.tsx:63,74
Priority: Medium — users following the UI hint will be rejected by the server

Issue:

The reset-password form shows:

<p className="text-slate-400 text-sm mb-6">Must be at least 4 characters.</p>
...
<input
  type={showPassword ? "text" : "password"}
  minLength={4}
  ...
/>

auth.ts sets minPasswordLength: 8. Any password between 4 and 7 characters passes the client-side minLength={4} constraint and the “at least 4 characters” label, but is rejected by Better Auth when the form is submitted. The user sees the form as valid but the server returns an error.

Fix:

<p className="text-slate-400 text-sm mb-6">Must be at least 8 characters.</p>
...
<input minLength={8} ... />

M-7 — leads/export/route.ts Bypasses Contact PII Masking

File: src/app/api/leads/export/route.ts
Priority: Medium — any authenticated tenant member can download all contact PII regardless of role

Issue:

The list endpoint (api/leads/route.ts) correctly masks email and phone contacts for non-privileged (non-owner/admin) users:

const canView = PRIVILEGED.has(actor.role);
// ...
contacts: maskContacts(r.contacts, canView),

The export endpoint does not perform any role check and includes raw contact data for all leads:

// No role check at all
const rows = await db.lead.findMany({
  where:   { tenantId: user.tenantId, isSpam },
  include: { contacts: true },  // all PII included
});

A member-role user who cannot see full email addresses in the UI can download a CSV containing every email and phone number in the tenant’s lead database simply by visiting GET /api/leads/export.

Fix:

Add the same privilege check used in the list endpoint:

const member = await db.tenantMember.findFirst({
  where:  { userId: session.user.id, tenantId: user.tenantId },
  select: { role: true },
});
const canView = new Set(["owner", "admin"]).has(member?.role ?? "");
 
// In the CSV rows loop:
const emails    = canView
  ? row.contacts.filter(c => c.type === "email").map(c => c.value)
  : row.contacts.filter(c => c.type === "email").map(c => maskEmail(c.value));
// ... same for phones and whatsapps

Alternatively, restrict the export endpoint entirely to owner/admin roles and return 403 for member users.

Low

L-1 — approveContext Race Condition From v1 Still Unfixed

File: src/app/(dashboard)/context/actions.ts:22–35
Priority: Low — two simultaneous approvals can enqueue duplicate strategy-writer jobs

Issue (unchanged from v1 and v2): Still uses read-then-update:

const context = await db.clientContext.findUnique({ where: { tenantId } });
if (context.status !== "completed") return { error: "..." };
await db.clientContext.update({ where: { id: context.id }, data: { status: "approved" } });
await triggerStrategyWriter(tenantId, userId);

Fix (unchanged from v2): Use updateMany with an optimistic where-clause:

const result = await db.clientContext.updateMany({
  where: { tenantId, status: "completed" },
  data:  { status: "approved" },
});
if (result.count === 1) {
  await triggerStrategyWriter(tenantId, userId);
}

L-2 — TopBar Socket Listener for notification:new Is Never Cleaned Up

File: src/components/topbar.tsx:63–71
Priority: Low — stale listeners accumulate on HMR, causing duplicate notification insertions

Issue (unchanged from v2):

useEffect(() => {
  let mounted = true;
  void connectSocket().then((socket) => {
    if (!mounted) return;
    socket.on("notification:new", (notif: NotifItem) => { ... });
  });
  return () => { mounted = false; };  // ← listener is never removed
}, []);

Fix (unchanged from v2):

useEffect(() => {
  let mounted = true;
  let sock: Socket | null = null;
  void connectSocket().then((socket) => {
    if (!mounted) return;
    sock = socket;
    socket.on("notification:new", (notif: NotifItem) => { ... });
  });
  return () => {
    mounted = false;
    sock?.off("notification:new");
  };
}, []);

L-3 — Duplicate POST Handler for Mark-All-Read Notifications

Files:

• src/app/api/notifications/route.ts (exports both GET and POST)
• src/app/api/notifications/read-all/route.ts (the actual endpoint called by TopBar)

Priority: Low — dead code; the POST in route.ts is unreachable

Issue (unchanged from v2): topbar.tsx calls POST /api/notifications/read-all. The POST export in api/notifications/route.ts is never called — it is at a different URL (POST /api/notifications).

Fix: Remove the POST export from src/app/api/notifications/route.ts.

L-4 — Inconsistent JWT Lifespan Across Six Server Components

Files:

• src/app/api/socket-token/route.ts: "15m"
• src/app/(dashboard)/channels/page.tsx: "1h"
• src/app/(dashboard)/client-info/page.tsx: "1h"
• src/app/(dashboard)/insights/page.tsx: "1h" ← new since v2
• src/app/(dashboard)/ai-chat/page.tsx: "4h"

Priority: Low — the 4-hour AI chat token extends the server-side attack window after sign-out

Issue (unchanged from v2 except insights adds a sixth path): No documented rationale for different lifespans. A captured 4-hour AI chat token remains valid for the full duration even after the user signs out.

Fix: Standardise on 15–30 minutes for all server-component-minted pass-through JWTs. If the AI chat session needs continuity, implement token refresh in useChatStream rather than minting long-lived upfront tokens.

L-5 — <a> Used Instead of Next.js <Link> in Multiple Dashboard Components

Files:

• src/app/(dashboard)/dashboard/page.tsx — StatCard, EmptyStatCard, goals block, business info chevron, reports chevron, and the “Complete business info” prompt all use <a>
• src/app/(dashboard)/context/actions.ts — N/A (server actions don’t render)

Priority: Low — full-page navigations from the dashboard; missed prefetching opportunity

Issue (unchanged from v2, with wider scope): Every navigation link in dashboard/page.tsx uses <a href>. The component comment itself (// ── Regular dashboard) has an <a href="/goals"> wrapper for the goals block. In addition to StatCard and EmptyStatCard noted in v2, RegularDashboard contains <a href="/client-info?tab=overview"> and <a href="/reports"> inline.

Fix: Replace all <a href> navigation links in dashboard components with <Link href> from next/link.

L-6 — next.config.ts Hardcodes localhost:9000 as Server Actions Allowed Origin

File: apps/dashboard/next.config.ts:7
Priority: Low — unchanged from v2

Fix (unchanged from v2):

serverActions: {
  allowedOrigins: process.env.SERVER_ACTION_ALLOWED_ORIGINS?.split(",") ?? []
}

L-7 — leads/[id]/route.ts Final Read and Delete Do Not Scope to tenantId

File: src/app/api/leads/[id]/route.ts
Priority: Low — defence-in-depth gap; correct in practice today but fragile

Issue:

The PUT handler verifies ownership with db.lead.findFirst({ where: { id, tenantId: actor.tenantId } }), then performs the update with db.lead.update({ where: { id } }) (no tenantId). The final re-fetch also omits tenantId:

const fresh = await db.lead.findFirst({ where: { id } });

The DELETE handler verifies ownership with tenantId, then deletes with:

await db.lead.delete({ where: { id } });  // no tenantId guard

In practice these are safe because the ownership check comes first. However, if the check-then-act ordering is ever modified, or if the id column ever becomes non-unique across tenants (schema change), the tenantId fence is already absent.

Fix:

Include tenantId in the update, delete, and refetch where clauses:

// PUT update
await db.lead.update({ where: { id, tenantId: actor.tenantId }, data: { ... } });
 
// PUT refetch
const fresh = await db.lead.findFirst({ where: { id, tenantId: actor.tenantId }, include: { ... } });
 
// DELETE
await db.lead.delete({ where: { id, tenantId: actor.tenantId } });

L-8 — generatePassword() in signup/page.tsx Uses Math.random()

File: src/app/(auth)/signup/page.tsx:22–24
Priority: Low — not cryptographically secure; affects suggested passwords only

Issue:

function generatePassword() {
  const chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789!@#$";
  return Array.from({ length: 16 }, () => chars[Math.floor(Math.random() * chars.length)]).join("");
}

Math.random() is not cryptographically secure. Passwords generated by this function for users who click “Generate password” have lower entropy than expected. In a browser context, the Web Crypto API is readily available.

Fix:

function generatePassword() {
  const chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789!@#$";
  const bytes = crypto.getRandomValues(new Uint8Array(16));
  return Array.from(bytes, b => chars[b % chars.length]).join("");
}

Info

I-1 — context/actions.ts Uses Raw SQL for client_context_log Table

File: src/app/(dashboard)/context/actions.ts:12–17
Priority: Info — acknowledged technical debt; no injection risk

The parameterised db.$executeRaw template literal remains correct. Once the Windows EPERM migration issue is resolved, replace insertContextLog with db.clientContextLog.create.

Recommended Action Order