Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 73 additions & 37 deletions architecture_review.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,12 @@ Service (log-workout.ts)

### The 4-Layer Stack

| Layer | Location | Responsibility |
|---|---|---|
| **HTTP Boundary** | `src/app/api/*/route.ts` | Auth, rate-limit, Zod parse. No business logic. |
| **Application Services** | `src/lib/services/**/*.ts` | Orchestrates domain + persistence + side effects. |
| **Domain Logic** | `src/lib/domain/*.ts` | **Pure functions only.** Zero infrastructure imports. |
| **Data Access** | `src/lib/data/*-db.ts` | Dumb persistence. Wraps MongoDB. Owns `toXxx()` mappers. |
| Layer | Location | Responsibility |
| ------------------------ | -------------------------- | -------------------------------------------------------- |
| **HTTP Boundary** | `src/app/api/*/route.ts` | Auth, rate-limit, Zod parse. No business logic. |
| **Application Services** | `src/lib/services/**/*.ts` | Orchestrates domain + persistence + side effects. |
| **Domain Logic** | `src/lib/domain/*.ts` | **Pure functions only.** Zero infrastructure imports. |
| **Data Access** | `src/lib/data/*-db.ts` | Dumb persistence. Wraps MongoDB. Owns `toXxx()` mappers. |

---

Expand All @@ -59,45 +59,55 @@ Service (log-workout.ts)
### 🏗 Creational

**Singleton (MongoDB Connection Pooling)**

- `mongodb.ts` uses `global._mongoClientPromise` to prevent hot-reload from spawning extra clients in development.

**Factory Function**

- `getCollection<WorkoutDoc>("workoutsCollection")` acts as a typed factory hiding all connection details from callers.

**Lazy Initialization**

- `getRedis()` in `sse-publisher.ts` creates a Redis client on-demand per call — correctly stateless for Vercel's serverless model.

---

### 🔧 Structural

**Adapter / Data Mapper**

- `toWorkout()`, `toUser()`, `toRun()` — each `*-db.ts` module owns a private mapper translating `ObjectId`/`Date` MongoDB documents into clean domain types (`WorkoutDoc → Workout`).

**Facade**

- `api-client/index.ts` is a barrel facade. Consumers import from one place, and the internals are split across 9 focused modules (`workouts.ts`, `runs.ts`, etc.).

**Decorator (Auth Guard)**

- `getAuthUserId()` acts as a consistent auth gate — every API route calls it first as a synchronized cross-cutting concern.

---

### 🎭 Behavioral

**Strategy (Quest Dispatch Table)**

- `QUEST_ACTIVITY_UPDATES` in `quest-rules.ts` is a type-safe dispatch table. Adding a new activity type (e.g., `yoga_session`) requires zero changes to calling code — just a new entry in the table.

**Optimistic Locking (Retry Loop)**

- `grant-user-xp.ts` uses a `__v` version field + retry loop to prevent XP race conditions without full pessimistic locks or advisory locks.

**Observer / Event-Driven**

- Side effects (level-up SSE notification, achievement evaluation) are decoupled from the transaction using fire-and-forget `async` patterns with `.catch()` error boundaries. The DB transaction never waits on them.

---

## 03 — Code Smells & Issues

### 🔴 HIGH — Dead Commented-Out Code

**File:** `src/lib/data/achievements-db.ts`

~40 lines of achievement seeding logic is commented out. This is a code graveyard — it adds cognitive noise and implies the seeding approach was abandoned mid-refactor without cleanup.
Expand All @@ -106,18 +116,22 @@ Service (log-workout.ts)

---

### 🟠 HIGH — Wrong HTTP Status Codes (Deployment Risk)
### 🟠 HIGH — Wrong HTTP Status Codes [✅ RESOLVED]

**File:** `src/app/api/workouts/route.ts` (and replicated in ~8 other routes)

In the POST handler's catch-all, any non-Zod, non-Auth error (e.g., a MongoDB connection timeout) returns a `400 Bad Request`. A server crash should be `500 Internal Server Error`.
_Note: This was resolved by implementing the `handleApiError` utility and semantic error classes (`AppError`). Unhandled server crashes now correctly return `500 Internal Server Error`, while domain exceptions return appropriate statuses like `400`, `401`, `404`, and `409`._

```typescript
// BEFORE (smell) — catch-all returns 400 for everything
const message = err instanceof Error ? err.message : "Invalid request";
return NextResponse.json({ error: message }, { status: 400 }); // ← WRONG

// AFTER (fix) — distinguish domain errors from infra errors
if (err instanceof Error && err.message === "This workout was already logged.") {
if (
err instanceof Error &&
err.message === "This workout was already logged."
) {
return NextResponse.json({ error: err.message }, { status: 409 });
}
return NextResponse.json({ error: "Internal Server Error" }, { status: 500 });
Expand All @@ -126,9 +140,11 @@ return NextResponse.json({ error: "Internal Server Error" }, { status: 500 });
---

### 🟡 MEDIUM — Two Conflicting Streak Functions

**File:** `src/lib/domain/user-rules.ts`

The domain has **two** streak calculation functions:

- `calcNewStreak()` — called during activity, increments the stored streak
- `calculateStreak()` — recalculates from an array of dates; appears **unused** in any service

Expand All @@ -137,9 +153,11 @@ The domain has **two** streak calculation functions:
---

### 🟡 MEDIUM — Business Logic Leaking Into the Data Mapper

**File:** `src/lib/data/user-db.ts` — `toUser()` function

The `toUser()` Data Mapper (Layer 4) is executing non-trivial domain rules:

1. Calling `calcRecoveredStamina()` — a stamina domain calculation
2. Containing inline streak display validation logic

Expand All @@ -164,6 +182,7 @@ const stamina = calcRecoveredStamina(raw.stamina, raw.lastStaminaUpdate, new Dat
---

### 🔵 LOW — Hardcoded SSE Duration

**File:** `src/app/api/friends/events/route.ts`

```typescript
Expand All @@ -175,6 +194,7 @@ Should be driven by an env var so upgrading Vercel plans doesn't require a code
---

### 🔵 LOW — Missing Env Var Validation for Upstash Redis

**File:** `src/env.ts`

`UPSTASH_REDIS_REST_URL` and `UPSTASH_REDIS_REST_TOKEN` are **not** declared in `env.ts`. A missing Vercel env var causes a runtime crash, not a clean startup failure.
Expand Down Expand Up @@ -207,21 +227,28 @@ await userState.applyActivity(userId, { xp, workout, stamina }, session);

---

### Candidate B — Route Error Handler HOF ⭐ Worth Exploring
### Candidate B — Route Error Handler HOF [✅ RESOLVED VIA UTILITY]

Every route has identical try/catch boilerplate. A `withApiHandler()` wrapper eliminates duplication and fixes status codes in one shot:
We explored creating a `withApiHandler()` wrapper, but ultimately chose to implement a centralized `handleApiError` utility function. This allowed us to keep the explicit `try/catch` blocks in every route for better debugging traceability, while reducing the catch block to a single, standardized line: `return handleApiError(err);`.

```typescript
// src/lib/api/with-api-handler.ts
export function withApiHandler(fn: RouteHandler): RouteHandler {
return async (req, ctx) => {
try { return await fn(req, ctx); }
catch (err) {
try {
return await fn(req, ctx);
} catch (err) {
if (err instanceof Error && err.message === "Unauthorized")
return NextResponse.json({ error: "Unauthorized" }, { status: 401 });
if (err instanceof z.ZodError)
return NextResponse.json({ error: err.issues[0]?.message }, { status: 400 });
return NextResponse.json({ error: "Internal Server Error" }, { status: 500 });
return NextResponse.json(
{ error: err.issues[0]?.message },
{ status: 400 },
);
return NextResponse.json(
{ error: "Internal Server Error" },
{ status: 500 },
);
}
};
}
Expand All @@ -231,38 +258,42 @@ export function withApiHandler(fn: RouteHandler): RouteHandler {

### Candidate C — Vercel Deployment Readiness ⭐ Strong — Do This First

| | Item | Priority |
|---|---|---|
| ✅ | Add `UPSTASH_REDIS_*` to `env.ts` | **Fix before deploy** |
| ✅ | Verify `BETTER_AUTH_URL` is set in Vercel env vars | **Fix before deploy** |
| ✅ | Wrap fire-and-forget promises in `waitUntil()` (`log-workout.ts` & others) | **Fix before deploy** |
| ✅ | Confirm `migrate-db.ts` targets Atlas (not Docker) on Vercel build | Verify |
| ✅ | Run seed script against Atlas cluster before go-live | Manual step |
| ⚠️ | SSE drops every 60s on Hobby plan — add "reconnecting" UI state | Nice to have |
| ⚠️ | Remove `console.log` from `evaluate-achievements.ts` | Nice to have |
| | Item | Priority |
| --- | -------------------------------------------------------------------------- | --------------------- |
| ✅ | Add `UPSTASH_REDIS_*` to `env.ts` | **Fix before deploy** |
| ✅ | Verify `BETTER_AUTH_URL` is set in Vercel env vars | **Fix before deploy** |
| ✅ | Wrap fire-and-forget promises in `waitUntil()` (`log-workout.ts` & others) | **Fix before deploy** |
| ✅ | Confirm `migrate-db.ts` targets Atlas (not Docker) on Vercel build | Verify |
| ✅ | Run seed script against Atlas cluster before go-live | Manual step |
| ⚠️ | SSE drops every 60s on Hobby plan — add "reconnecting" UI state | Nice to have |
| ⚠️ | Remove `console.log` from `evaluate-achievements.ts` | Nice to have |

---

## 05 — Authentication & Authorization

### The Framework: `better-auth`

The application uses **[better-auth](https://better-auth.com/)** with the `mongodbAdapter` as the core identity provider.

- **Provider:** Email and Password (configured in `src/lib/auth/server.ts`).
- **Session Storage:** Server-side sessions persisted in MongoDB.

### Authorization Method: The Decorator Pattern

There is no complex Role-Based Access Control (RBAC) yet. Authorization is handled via a simple but effective decorator-style gatekeeper: `getAuthUserId()`.

1. **The Gatekeeper (`auth-helpers.ts`):**
1. **The Gatekeeper (`auth-helpers.ts`):**
Reads the incoming request headers and checks `auth.api.getSession()`. If no valid session exists, it deliberately throws `new Error("Unauthorized")`.
2. **The Route Handlers (`route.ts`):**
Every protected API route begins by calling `const userId = await getAuthUserId();`.
Every protected API route begins by calling `const userId = await getAuthUserId();`.
3. **The Error Boundary:**
The pervasive `try/catch` block in every route handler catches that specific `"Unauthorized"` error string and returns a `401` HTTP response.

**Architectural Assessment of Auth:**
- **Pros:** It's extremely explicit. You can't accidentally expose a protected route because you must call `getAuthUserId()` to get the `userId` needed for any database query.
- **Cons:** It relies on throwing a generic `Error` with a magic string (`"Unauthorized"`) rather than a custom exception class (e.g., `class UnauthorizedError extends Error`). This is part of the reason why the `withApiHandler()` HOF (Candidate B) is strongly recommended — it would clean up this magic string matching across all routes.

- **Pros:** It's extremely explicit. You can't accidentally expose a protected route because you must call `getAuthUserId()` to get the `userId` needed for any database query.
- **Cons:** Initially, it relied on throwing a generic `Error` with a magic string (`"Unauthorized"`). This has since been resolved by introducing a semantic `UnauthorizedError` class and the centralized `handleApiError` utility, which cleanly translates it to a `401` response.

---

Expand All @@ -272,7 +303,6 @@ There is no complex Role-Based Access Control (RBAC) yet. Authorization is handl
2. **Wrap background tasks in `waitUntil()`** — prevent Vercel from freezing container mid-execution.
3. **Seed MongoDB Atlas** with achievements and quests. Vercel build does NOT run the seed script.
4. **Delete the commented-out code** in `achievements-db.ts` — 2-minute cleanup.
5. **Fix HTTP status codes** in API routes — `500` for server errors, not `400`.

> **Overall assessment:** This is a genuinely well-structured Next.js codebase. The 4-layer separation is clean, the pure domain layer is excellent, and the optimistic locking pattern for XP is sophisticated. The main technical debt is in error handling consistency and domain logic leaking into the data mapper. Both are fixable in a day.

Expand All @@ -283,18 +313,24 @@ There is no complex Role-Based Access Control (RBAC) yet. Authorization is handl
Based on our architectural review and brainstorming session, the following frontend-heavy gamification features are approved for the next development cycle before the Vercel deployment:

### 1. Interactive Human Anatomy UI

A visual representation of the human muscular system mapping to the `TargetMuscle` enum (`Chest`, `Back`, `Legs`, etc.). It will be implemented across 3 contexts:
* **The 7-Day Heatmap (Dashboard):** Evaluates workout history over the past 7 days. Muscle groups glow red/orange based on training volume, helping identify neglected muscle groups.
* **Live Session "Pump" Tracker (Workout View):** As exercises are added to an active session, the corresponding muscles light up instantly, acting as a visual checklist.
* **Recovery Monitor (Profile/Dashboard):** Evaluates fatigue. Muscles trained recently start red (exhausted) and slowly transition to green (recovered) over a 48-72 hour window.

- **The 7-Day Heatmap (Dashboard):** Evaluates workout history over the past 7 days. Muscle groups glow red/orange based on training volume, helping identify neglected muscle groups.
- **Live Session "Pump" Tracker (Workout View):** As exercises are added to an active session, the corresponding muscles light up instantly, acting as a visual checklist.
- **Recovery Monitor (Profile/Dashboard):** Evaluates fatigue. Muscles trained recently start red (exhausted) and slowly transition to green (recovered) over a 48-72 hour window.

### 2. Workout Templates

Frictionless workout entry. Allows users to save a collection of exercises as a named template (e.g., "Push Day", "Upper Body Power").
* **Data Layer:** Will require a new MongoDB collection or sub-document on the user profile (`Template[]`).
* **UX:** A 1-click "Start from Template" button on the workout screen that pre-populates the exercise list.

- **Data Layer:** Will require a new MongoDB collection or sub-document on the user profile (`Template[]`).
- **UX:** A 1-click "Start from Template" button on the workout screen that pre-populates the exercise list.

### 3. "Beat Your Ghost" (Personal Records Integration)

A gamified pacing mechanic utilizing the existing `PersonalRecord[]` data.
* **Live Target:** When logging an exercise (e.g., Bench Press) or a run, the UI fetches and displays the user's historical PR as the "Ghost to beat".
* **Social Hype:** If the user logs a value that exceeds their ghost, it triggers a confetti/explosion animation locally.
* **Event Integration:** Hooked into the Upstash Redis SSE system to broadcast a special achievement toast to all friends: *"Adrian just shattered their Bench Press record!"*

- **Live Target:** When logging an exercise (e.g., Bench Press) or a run, the UI fetches and displays the user's historical PR as the "Ghost to beat".
- **Social Hype:** If the user logs a value that exceeds their ghost, it triggers a confetti/explosion animation locally.
- **Event Integration:** Hooked into the Upstash Redis SSE system to broadcast a special achievement toast to all friends: _"Adrian just shattered their Bench Press record!"_
5 changes: 3 additions & 2 deletions project_briefing.md
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ POST /api/workouts
- **Serverless Background Tasks (`after()` API):** Migrated all fire-and-forget background promises (`evaluateAchievements`, `notifyFriendsLevelUp`) to the Next.js `after()` API to ensure they complete in serverless environments (like Vercel) before the container freezes.
- **Unified `UserStateService`:** Consolidated 4 fragmented database updates (XP, Streak, Stamina, Stats) into a single deep module. `log-workout.ts` and `log-run.ts` now execute a single atomic `findOneAndUpdate` via `UserStateService.applyActivity()` instead of scattering 4+ separate update calls.
- **Vitest Mocking:** Added a global `vitest.setup.ts` to elegantly mock the Next.js `next/server` `after()` API (handling both callbacks and Promises) without wiping out `NextResponse`, keeping the test suite fast and 100% green.
- **Centralized API Error Handling:** Migrated all services to throw semantic exceptions (`ConflictError`, `NotFoundError`, `UnauthorizedError`) and standardized all 19 API routes to use a centralized `handleApiError` utility, ensuring consistent HTTP status codes across the app.

### Why was it built this way?

Expand All @@ -245,7 +246,7 @@ These are architectural upgrades that make the system more scalable and robust.
| --- | -------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| 16 | **Quest template caching** | Right now, every quest read hits MongoDB to fetch all active templates. Templates almost never change. A 60-second in-memory cache (or `React.cache`) would cut this to near-zero DB reads. |

| 19 | **Centralized error handling** | Every API route has its own `try/catch` with slightly different error handling. A `withAuth(handler)` wrapper would standardize all of this. |
| 19 | **Centralized error handling (COMPLETED)** | Every API route had its own `try/catch` with slightly different error handling. We standardized this using semantic error classes and a `handleApiError` utility. |

---

Expand Down Expand Up @@ -330,7 +331,7 @@ This feature has been fully implemented. It serves two purposes:
| Priority | Issue | Status |
| -------- | ----------------------------------------------------------------- | --------- |
| 🟢 P3 | Quest template caching | In plan |
| 🟢 P3 | Centralized error handler wrapper | In plan |
| ✅ Done | Centralized API error handling | Completed |
| ✅ Done | Friend System + Real-Time SSE | Completed |
| 🟢 P3 | log-workout-test, stamina and lastStaminaUpdate update test mocks | In plan |

Expand Down
7 changes: 2 additions & 5 deletions src/app/api/achievements/route.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { NextResponse } from "next/server";
import { getAllAchievementsForUser } from "@/lib/data/achievements-db";
import { getAuthUserId } from "@/lib/auth/auth-helpers";
import { handleApiError } from "@/lib/api/handle-api-error";

export async function GET() {
try {
Expand All @@ -10,10 +11,6 @@ export async function GET() {
headers: { 'Cache-control': 'private, max-age=60, stale-while-revalidate=300' }
});
} catch (error) {
console.error("Failed to fetch achievements:", error);
return NextResponse.json(
{ error: "Failed to fetch achievements" },
{ status: 500 }
);
return handleApiError(error);
}
}
Loading
Loading