Dashboard App Code Review — v2

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-17
Reviewer: Claude Sonnet 4.6
Based on: dashboard-code-review-v1.md (2026-04-08)
Status: Open — not yet actioned

Summary

Progress Since v1

All items from the previous review have been addressed except H-1 (acknowledged limitation) and the former L-3 (approveContext race condition, now carried forward as L-1 below).

Files Reviewed

High

H-1 — JWT_SECRET Falls Back to "dev-secret" in Four Production Code Paths

Files:

• src/app/api/socket-token/route.ts:26
• src/app/(dashboard)/channels/page.tsx:25
• src/app/(dashboard)/client-info/page.tsx:33
• src/app/(dashboard)/ai-chat/page.tsx (4-hour JWT)

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

Issue:

All four paths that mint JWTs share the same pattern:

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

If JWT_SECRET is unset in any environment (staging, preview deployments, CI integration tests), tokens are signed with the string "dev-secret". An attacker who knows this fallback can forge a valid JWT for any tenant and call the API server directly.

The API server validates these tokens to permit calls to /tenant/v1/* endpoints (channels, docs, AI chat) and socket authentication (/tenant namespace). A forged token with an arbitrary tenantId bypasses all tenant isolation.

Fix:

Guard the environment variable at startup. Add a check in next.config.ts or a shared module:

// 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 four process.env.JWT_SECRET ?? "dev-secret" occurrences with getJwtSecret(). A missing secret will now cause a server error at request time (500) rather than silently using a predictable value. 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 presented to users

Issue:

getDashboardStats emits stages 1, 2, 4, and 5 but never stage 3:

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

GettingStartedView in dashboard/page.tsx is explicitly designed for stages 1–3 (comment: stages 1–3, step comment: // 3 = goals set) and RegularDashboard triggers at stage >= 5. This means tenants transition from:

• Stage 2 (context approved, no goals) → step 2 active
• Stage 4 (context + goals + no activities) → step 4 active, steps 1–3 shown as done

Step 3 (“Goals & KPIs are set”) is never the active step. A tenant who sets their first goals sees the workspace jump from “strategy pending” to “agents start working” with no acknowledgement of the goal milestone. The bannerText object in GettingStartedView also has an explicit third case (stage > 2) — “Goals & KPIs are set — agents will start working soon” — which is unreachable.

Additionally, the RegularDashboard source comment reads stage 4 — agents are working, but the trigger is stage >= 5. This comment/code mismatch suggests the stage numbering was changed after the v1 getDashboardStats extraction without updating all references.

Fix:

Add stage 3 to the logic so the onboarding wizard shows the intermediate state:

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 dashboard/page.tsx to show RegularDashboard at stage >= 4 (or whichever threshold is correct) and update the stage JSDoc comment accordingly.

M-2 — Timezone Inconsistency Between deliverables/page.tsx and calendar/page.tsx

Files:

• src/app/(dashboard)/deliverables/page.tsx:41–43 (UTC)
• src/app/(dashboard)/calendar/page.tsx:27–29 (local time)

Priority: Medium — tenants in UTC+5:30 and later can see a one-month shift in displayed content

Issue:

deliverables/page.tsx builds period boundaries with Date.UTC:

const periodStart = new Date(Date.UTC(targetYear, targetMonth, 1));
const periodEnd   = new Date(Date.UTC(targetYear, targetMonth + 1, 0, 23, 59, 59));

calendar/page.tsx builds the same month boundaries with new Date() (local server time):

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

If the API server process runs in UTC (typical container default) the two approaches return identical results. However:

1. The server locale is container-specific and not guaranteed — a developer running locally on IST would see different behaviour from production.
2. Activity dueDate values are stored at UTC midnight. Comparing against local-time boundaries means activities scheduled for the last day of a month may fall outside the calendar query for UTC+ zones.
3. The overlap query in deliverables/page.tsx uses periodStart: { lte: periodEnd } with UTC boundaries, while the calendar uses a simpler gte/lt with local boundaries. The two pages can disagree on which month a boundary-date activity belongs to.

Fix:

Standardise on UTC across all date range queries. Extract a shared 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 the local-time construction in calendar/page.tsx with getMonthRangeUTC and use lt: end consistently (exclusive upper bound) rather than mixing inclusive 0, 23, 59, 59 with exclusive new Date(year, month, 1).

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

File: src/components/chat/PresenceProvider.tsx:97
Priority: Medium — agent status panels and topbar notifications lose socket connection when presence unmounts

Issue:

PresenceProvider tears down the global socket on unmount:

return () => {
  cancelled = true;
  if (heartbeatRef.current) clearInterval(heartbeatRef.current);
  const s = socketRef.current;
  if (s) { s.off(...); }
  destroySocket();  // ← sets module-level socket = null and calls socket.disconnect()
};

The socket singleton in src/lib/socket.ts is shared by all consumers: PresenceProvider, TopBar (notifications), AgentStatusPanel, StrategyAgentPanel, and DeliverablePlannerAgentPanel. PresenceProvider wraps the entire DashboardLayout and should never unmount during a session, but during development (HMR) and in edge cases (tab backgrounded for long periods, React strict-mode double-effect in dev), the cleanup fires and the socket is destroyed for all consumers simultaneously.

After destroySocket(), the next call to getSocket() in AgentStatusPanel creates a new socket instance without an auth token (the token fetch is only in connectSocket()), so the socket connects unauthenticated and the server rejects it.

Fix:

PresenceProvider should only remove its own event listeners, not destroy the shared socket. Move the socket lifecycle out of PresenceProvider:

// In cleanup:
return () => {
  cancelled = true;
  if (heartbeatRef.current) clearInterval(heartbeatRef.current);
  const s = socketRef.current;
  if (s) {
    s.off("connect");
    s.off("disconnect");
    s.off("presence:list");
    s.off("user:online");
    s.off("user:offline");
    s.off("connect_error");
    // DO NOT call destroySocket() — other consumers own this reference
  }
};

Socket cleanup should only happen on explicit sign-out (handleSignOut in topbar.tsx).

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

File: src/db/seed.ts:9
Priority: Medium — pnpm --filter dashboard db:seed fails silently or with an unintuitive error

Issue:

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

auth.ts sets minPasswordLength: 8. Better Auth will reject "admin" (5 characters) at the library level. The catch block in seed.ts only suppresses errors whose message contains "already", so a password-too-short error will surface as "❌ Failed to seed admin user" and process.exit(1).

Fix:

Use a password that meets the minimum length requirement:

password: "admin123"   // or any ≥8-char value suitable for dev seeding

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

File: src/app/(dashboard)/PaymentBanner.tsx
Priority: Medium — clicking “Pay Now” leads to a 404 or the settings index page

Issue:

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

The settings directory contains only: agents/, context/, notifications/, profile/, users/. There is no billing/ route. Clicking the payment banner takes the user to a non-existent page at a moment when they need to pay to restore access.

Fix:

Either:

1. Create src/app/(dashboard)/settings/billing/page.tsx with invoice/payment management UI (redirecting to the Razorpay payment link or showing invoice details from client-info/page.tsx).
2. Or redirect to the existing billing section: <a href={/client-info?tab=billing&invoice=${invoiceId}}>.

Low

L-1 — approveContext Race Condition From v1 Still Unfixed

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

Issue:

The implementation still uses the read-then-update pattern from v1 L-3:

const context = await db.clientContext.findUnique({ where: { tenantId } });
// ... status checks
await db.clientContext.update({ where: { id: context.id }, data: { status: "approved" } });
await triggerStrategyWriter(tenantId, userId);  // enqueued twice if two callers race

Fix (from v1):

Replace with updateMany using a where: { status: "completed" } constraint:

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 listener accumulates on HMR and causes duplicate notification insertions

Issue:

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

The mounted = false flag prevents the listener from being attached after unmount, but if the component is alive when the socket resolves and is then remounted (HMR, React strict-mode double-invoke), a second listener is attached to the same socket. Each notification:new event will increment the notification list twice.

Fix:

useEffect(() => {
  let mounted = true;
  let sock: Socket | null = null;
 
  void connectSocket().then((socket) => {
    if (!mounted) return;
    sock = socket;
    socket.on("notification:new", (notif: NotifItem) => {
      setNotifications((prev) => [{ ...notif, read: false }, ...prev]);
    });
  });
 
  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 POST)
• src/app/api/notifications/read-all/route.ts (exports POST)

Priority: Low — dead code; the main route.ts POST is never called

Issue:

api/notifications/route.ts exports both GET (list notifications) and POST (mark all read). There is also a dedicated api/notifications/read-all/route.ts that is the actual endpoint called by handleMarkAllRead in topbar.tsx. The POST in the main route file is unreachable dead code — Next.js’s file-based router means POST /api/notifications and POST /api/notifications/read-all are different endpoints, and the topbar correctly calls the latter.

Fix:

Remove the POST export from src/app/api/notifications/route.ts. The file should only export GET.

L-4 — Inconsistent JWT Lifespan Across Server Components

Files:

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

Priority: Low — undocumented inconsistency; the 4-hour AI chat token greatly extends the server-side attack window

Issue:

Four separate code paths mint JWTs with no documented rationale for their different lifespans. The socket token is 15 minutes (correct, short-lived, has a session behind it). The channels and client-info tokens are 1 hour — long enough to still be valid after a user signs out if the server-side session is invalidated. The AI chat token is 4 hours with no comment explaining why it needs to be longer.

These tokens are not stored or revocable. An intercepted 4-hour token can be used to call /ai-chat API endpoints for the full 4 hours 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 longer continuity, implement token refresh in useChatStream rather than minting upfront long-lived tokens.

L-5 — StatCard Uses <a> Instead of Next.js <Link>

File: src/app/(dashboard)/dashboard/page.tsx:20,62
Priority: Low — forfeits client-side prefetching; navigations from the dashboard cause a full page load

Issue:

Both StatCard and EmptyStatCard render:

<a href={href} className="block ...">

Next.js <Link> prefetches the target route when the card is visible in the viewport, making navigation instant. Using <a> means every stat-card click triggers a full server round-trip. On a slow connection the dashboard becomes the slowest entry point to every section.

Fix:

import Link from "next/link";
// ...
<Link href={href} className="block ...">

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

File: apps/dashboard/next.config.ts:7
Priority: Low — non-production origins should not be hardcoded; could suppress CSRF errors in unexpected environments

Issue:

experimental: {
  serverActions: {
    allowedOrigins: ["localhost:9000"]
  }
}

Next.js validates the Origin header on Server Action POST requests. Adding localhost:9000 to the allowlist means Server Actions can be triggered from a page served on that port regardless of the environment. In production, localhost:9000 is unreachable — but in a CI environment with multiple services on the same host, or if the port is proxied, this allowance is broader than intended.

Fix:

Drive this from an environment variable so the value is environment-specific:

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

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 immediate risk

The comment correctly explains the workaround:

// ── Raw SQL helper — ClientContextLog not yet in Prisma client (EPERM on Windows) ──
async function insertContextLog(...) {
  await db.$executeRaw`INSERT INTO client_context_log ...`;
}

The raw query uses db.$executeRaw with a tagged template literal, which parameterises all values correctly — there is no injection risk. The issue is that if the client_context_log schema changes (column rename, addition of NOT NULL column), TypeScript won’t catch the mismatch.

Action: Once the Windows EPERM migration issue is resolved, add ClientContextLog to the Prisma schema and replace insertContextLog with db.clientContextLog.create.

I-2 — reports/page.tsx Is a Stub

File: src/app/(dashboard)/reports/page.tsx
Priority: Info — known placeholder

return <ReportsClient reports={[]} />;

ReportsClient always receives an empty array. The sidebar link to /reports is active. Users clicking it see an empty state. This is likely tracked on the product backlog, but it’s noted here for completeness.

Recommended Action Order