Refactor: Structured logging and type safety improvements

.releases/unreleased/code-quality-improvements.md

@@ -0,0 +1,5 @@
+---
+type: patch
+area: api
+summary: Improve error handling with structured logging and better type safety in tests
+---

apps/api/src/common/filters/http-error.filter.ts

@@ -3,7 +3,8 @@ import {
   Catch,
   ExceptionFilter,
   HttpException,
-  HttpStatus
+  HttpStatus,
+  Logger
 } from "@nestjs/common";
 import {
   DomainInvariantError,
@@ -14,9 +15,12 @@ import {
 
 @Catch()
 export class HttpErrorFilter implements ExceptionFilter {
+  private readonly logger = new Logger(HttpErrorFilter.name);
+
   catch(exception: unknown, host: ArgumentsHost): void {
     const context = host.switchToHttp();
     const response = context.getResponse();
+    const request = context.getRequest();
 
     if (exception instanceof HttpException) {
       const status = exception.getStatus();
@@ -61,6 +65,14 @@ export class HttpErrorFilter implements ExceptionFilter {
       return;
     }
 
+    // Log unhandled exceptions with context
+    this.logger.error("Unhandled exception", {
+      url: request?.url,
+      method: request?.method,
+      message: exception instanceof Error ? exception.message : String(exception),
+      stack: exception instanceof Error ? exception.stack : undefined
+    });
+
     response.status(HttpStatus.INTERNAL_SERVER_ERROR).json({
       statusCode: HttpStatus.INTERNAL_SERVER_ERROR,
       message: "Internal server error",

apps/api/src/main.ts

@@ -1,5 +1,5 @@
 import "reflect-metadata";
-import { ValidationPipe } from "@nestjs/common";
+import { Logger, ValidationPipe } from "@nestjs/common";
 import { NestFactory } from "@nestjs/core";
 import { ensureEnvironmentLoaded } from "@corpsim/db";
 import { AppModule } from "./app.module";
@@ -101,7 +101,8 @@ async function bootstrap(): Promise<void> {
 }
 
 bootstrap().catch((error: unknown) => {
-  console.error("API bootstrap failed", error);
+  const logger = new Logger("Bootstrap");
+  logger.error("API bootstrap failed", error instanceof Error ? error.stack : error);
   process.exitCode = 1;
 });
 

apps/api/src/prisma/prisma.service.ts

@@ -1,16 +1,23 @@
-import { Injectable, OnModuleDestroy, OnModuleInit } from "@nestjs/common";
+import { Injectable, Logger, OnModuleDestroy, OnModuleInit } from "@nestjs/common";
 import { PrismaClient } from "@prisma/client";
 import { ensureEnvironmentLoaded } from "@corpsim/db";
 
 ensureEnvironmentLoaded();
 
+interface PrismaErrorWithCode extends Error {
+  errorCode?: string;
+}
+
+function isPrismaErrorWithCode(error: unknown): error is PrismaErrorWithCode {
+  return error instanceof Error && "errorCode" in error;
+}
+
 function shouldRetryConnect(error: unknown): boolean {
-  if (!(error instanceof Error)) {
+  if (!isPrismaErrorWithCode(error)) {
     return false;
   }
 
-  const prismaCode = (error as { errorCode?: string }).errorCode;
-  if (prismaCode === "P1001") {
+  if (error.errorCode === "P1001") {
     return true;
   }
 
@@ -25,6 +32,8 @@ function sleep(ms: number): Promise<void> {
 
 @Injectable()
 export class PrismaService extends PrismaClient implements OnModuleInit, OnModuleDestroy {
+  private readonly logger = new Logger(PrismaService.name);
+
   async onModuleInit(): Promise<void> {
     const maxAttempts = 30;
     const baseDelayMs = 1_000;
@@ -39,8 +48,8 @@ export class PrismaService extends PrismaClient implements OnModuleInit, OnModul
         }
 
         const delayMs = Math.min(baseDelayMs * attempt, 5_000);
-        console.warn(
-          `[prisma] database not reachable on attempt ${attempt}/${maxAttempts}; retrying in ${delayMs}ms`
+        this.logger.warn(
+          `Database not reachable on attempt ${attempt}/${maxAttempts}; retrying in ${delayMs}ms`
         );
         await sleep(delayMs);
       }

apps/web/src/lib/api-client.ts

@@ -149,7 +149,16 @@ export async function fetchJson<T>(
     response = await fetch(path, requestInit);
   }
 
-  const payload = (await response.json().catch(() => null)) as unknown;
+  let payload: unknown = null;
+  try {
+    payload = await response.json();
+  } catch (parseError) {
+    // If JSON parsing fails and response is not OK, we'll still throw with status
+    // If JSON parsing fails but response is OK, payload remains null
+    if (process.env.NODE_ENV === "development") {
+      console.warn("Failed to parse API response JSON:", parseError);
+    }
+  }
 
   if (!response.ok) {
     let message = `Request failed: ${response.status}`;

packages/sim/tests/research.test.ts

@@ -1,18 +1,11 @@
-import { Prisma, PrismaClient, ResearchJobStatus } from "@prisma/client";
+import { Prisma, ResearchJobStatus } from "@prisma/client";
 import { describe, expect, it, vi } from "vitest";
 import {
   DomainInvariantError,
   completeDueResearchJobs,
   startResearch
 } from "../src";
-
-function createPrismaTransactionMock(tx: Prisma.TransactionClient): PrismaClient {
-  return {
-    $transaction: async <T>(
-      callback: (transactionClient: Prisma.TransactionClient) => Promise<T>
-    ): Promise<T> => callback(tx)
-  } as unknown as PrismaClient;
-}
+import { createPrismaTransactionMock } from "./test-utils";
 
 describe("research service", () => {
   it("enforces prerequisite gating before starting research", async () => {

packages/sim/tests/test-utils.ts

@@ -0,0 +1,19 @@
+import { Prisma, PrismaClient } from "@prisma/client";
+
+/**
+ * Creates a minimal PrismaClient mock that supports $transaction.
+ * 
+ * Note: This uses `as unknown as PrismaClient` because the actual test
+ * implementations only use the $transaction method, but the service functions
+ * expect the full PrismaClient type. This is a deliberate trade-off to keep
+ * test mocks simple while maintaining type compatibility.
+ */
+export function createPrismaTransactionMock(
+  tx: Prisma.TransactionClient
+): PrismaClient {
+  return {
+    $transaction: async <T>(
+      callback: (transactionClient: Prisma.TransactionClient) => Promise<T>
+    ): Promise<T> => callback(tx)
+  } as unknown as PrismaClient;
+}

packages/sim/tests/workforce.test.ts

@@ -1,19 +1,12 @@
-import { Prisma, PrismaClient } from "@prisma/client";
+import { Prisma } from "@prisma/client";
 import { describe, expect, it, vi } from "vitest";
 import {
   assertValidWorkforceAllocation,
   DomainInvariantError,
   requestCompanyWorkforceCapacityChange,
   runWorkforceForTick
 } from "../src";
-
-function createPrismaTransactionMock(tx: Prisma.TransactionClient): PrismaClient {
-  return {
-    $transaction: async <T>(
-      callback: (transactionClient: Prisma.TransactionClient) => Promise<T>
-    ): Promise<T> => callback(tx)
-  } as unknown as PrismaClient;
-}
+import { createPrismaTransactionMock } from "./test-utils";
 
 describe("workforce service", () => {
   it("validates allocation percentages sum to 100", () => {