emilandClaude Opus 4.7
701398f627
Adds an opt-in modal that prompts a PRO user with local data to migrate their localStorage notes and sessionStorage initiative into the cloud. - src/lib/client/import.ts: pure helpers (hasLocalData, shouldShowImport) - src/components/dm/ImportModal.astro: modal UI + import orchestration with conflict handling for existing "DM Notes" record - src/i18n/dm-translations.ts: 14 new keys for the import flow - Wired into both EN and RU dm/index.astro - tests/import-flow.test.ts: 18 unit tests on the helpers Local storage is preserved as backup; only a "dm-import-asked" flag is set after the user makes a decision (import or skip). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1145 lines
63 KiB
Markdown
1145 lines
63 KiB
Markdown
# Auth Anti-Pattern Fixes — Learnings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### Patterns Applied
|
||
|
||
1. **Fail-fast env validation with Zod**
|
||
- Created `src/lib/auth/env.ts` with a `z.object()` schema that validates `JWT_SECRET`, `VK_CLIENT_ID`, `VK_CLIENT_SECRET`, `YANDEX_CLIENT_ID`, `YANDEX_CLIENT_SECRET`, and `PUBLIC_APP_URL` at module load time.
|
||
- `process.env.JWT_SECRET!` non-null assertion replaced with `authEnv.JWT_SECRET` which throws a descriptive error if the env var is missing.
|
||
- `requireEnv()` helper provides typed access to validated env values.
|
||
|
||
2. **SameSite cookie fix**
|
||
- Changed `sameSite: 'strict'` to `sameSite: 'lax'` in `setAuthCookie()` (`src/lib/auth/jwt.ts`).
|
||
- `SameSite=Strict` breaks cross-site OAuth redirects because the browser doesn't send the cookie on the redirect back from the identity provider. `Lax` + `Secure` is the correct combination for OAuth session cookies.
|
||
|
||
3. **Removed all `|| ''` fallbacks**
|
||
- `src/lib/auth/oauth.ts` had 5 instances of `process.env.XXX || ''`.
|
||
- Replaced with `authEnv.XXX` references. Empty-string fallbacks silently produce invalid OAuth requests (e.g., `client_id: ''` causes 400 errors from VK/Yandex).
|
||
|
||
4. **Logout session cleanup**
|
||
- `src/pages/api/auth/logout.ts` now reads the `auth_token` cookie and calls `deleteSession(token)` before clearing the cookie.
|
||
- Previously the session row remained in the DB indefinitely.
|
||
|
||
5. **Middleware stale cookie cleanup**
|
||
- `src/middleware/index.ts` now calls `context.cookies.delete('auth_token', { path: '/' })` in two places:
|
||
- When `verifyToken()` throws (invalid/expired JWT)
|
||
- When `getSession()` returns `null` (session expired or deleted from DB)
|
||
- Previously the stale cookie stayed in the browser, causing repeated failed auth attempts on every request.
|
||
|
||
6. **PUBLIC_APP_URL exclusivity**
|
||
- `src/pages/api/auth/callback/vk.ts`, `yandex.ts`, and the login handlers (`login/vk.ts`, `login/yandex.ts`) all used `process.env.PUBLIC_APP_URL || url.origin`.
|
||
- Replaced with `authEnv.PUBLIC_APP_URL` exclusively.
|
||
- `url.origin` breaks behind nginx reverse proxy because it returns `http://localhost:4321` instead of the public HTTPS URL.
|
||
|
||
### Testing Notes
|
||
|
||
- happy-dom strips `Set-Cookie` headers from `Response` objects (both via `Headers` instance and array entries). This is a known limitation — cookie clearing behavior must be tested via integration/Playwright instead of unit tests.
|
||
- `vi.mock` with alias paths (`@/...`) does not reliably match dynamically imported modules after `vi.resetModules()` in vitest. Using `__mocks__` directories or avoiding deep DB mocking in unit tests is more stable.
|
||
- `crypto.subtle` needs Node.js `webcrypto` for `jose` to work in vitest. Added `node:crypto` webcrypto to `tests/setup.ts`.
|
||
- `astro:middleware` is a virtual module that doesn't resolve in vitest. Added an alias in `vitest.config.ts` pointing to `tests/mocks/astro-middleware.ts`.
|
||
|
||
### Verification
|
||
|
||
- `npx tsc --noEmit`: 0 errors
|
||
- `npx vitest run src/lib/auth/`: 18/18 tests passed
|
||
|
||
---
|
||
|
||
# CORS Configuration for DM API Routes — Learnings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### Patterns Applied
|
||
|
||
1. **Reusable CORS utility (`src/lib/cors.ts`)**
|
||
- Created `getCorsHeaders()`, `createCorsResponse()`, `handleCorsPreflight()`, and `jsonResponse()` helpers.
|
||
- Allowed origins are validated by hostname using `URL` parsing: `randify.pro`, `dm.randify.pro`, `localhost:4321`.
|
||
- Disallowed origins receive an empty `Access-Control-Allow-Origin` header (no wildcard).
|
||
- `Vary: Origin` header is included to prevent CDN caching issues with CORS.
|
||
|
||
2. **DM API route pattern (`src/pages/api/dm/health.ts`)**
|
||
- New DM API routes should import helpers from `@/lib/cors`.
|
||
- Every route must export an `OPTIONS` handler that calls `handleCorsPreflight(origin)`.
|
||
- Every response should be created via `jsonResponse()` or `createCorsResponse()` to ensure CORS headers are present.
|
||
- `export const prerender = false` is required for API routes.
|
||
|
||
3. **Allowed origins**
|
||
- `https://randify.pro`
|
||
- `https://dm.randify.pro`
|
||
- `http://localhost:4321`
|
||
- Any protocol is accepted as long as the hostname matches.
|
||
|
||
4. **happy-dom `Request` limitation**
|
||
- `new Request(url, { headers: { origin: '...' } })` strips the `origin` header in happy-dom (vitest environment).
|
||
- Tests that call Astro API routes must construct a mock request object with a custom `headers.get()` method instead of relying on the global `Request` class for origin propagation.
|
||
- Example pattern in `tests/cors.test.ts` (`mockRequest` helper).
|
||
|
||
### Verification
|
||
|
||
- `npx vitest run tests/cors.test.ts`: 12/12 passed
|
||
- `npx vitest run` (full suite): 218/218 passed
|
||
|
||
---
|
||
|
||
# Drizzle Migration Generation — Learnings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### Context
|
||
|
||
Task: Generate and apply Drizzle migration for updated DB schema (Task 2 completed).
|
||
|
||
### What Was Done
|
||
|
||
1. **Generated migration via `drizzle-kit generate`**
|
||
- Command: `DATABASE_URL=postgresql://postgres:postgres@localhost:5432/randify npx drizzle-kit generate`
|
||
- Generated file: `drizzle/0001_curved_black_panther.sql`
|
||
- Journal updated: `drizzle/meta/_journal.json`
|
||
|
||
2. **Reviewed generated SQL for correctness**
|
||
- New tables created: `npcs`, `generation_counters`, `translations`, `notes`, `initiative_sessions`
|
||
- Existing table altered: `users` — added `tier` (varchar(20), default 'free') and `boosty_verified_at` (timestamp)
|
||
- All FK constraints use `ON DELETE cascade` as specified in schema
|
||
- Indexes match schema definitions:
|
||
- `generation_counters_user_window_model_idx` (unique)
|
||
- `translations_slug_type_language_idx`
|
||
- CHECK constraint added: `users_tier_check` enforcing `('free', 'pro')`
|
||
- `translations.slug` correctly marked as `UNIQUE`
|
||
|
||
3. **Verified migration applies cleanly**
|
||
- Command: `DATABASE_URL=postgresql://postgres:postgres@localhost:5432/randify npm run db:migrate`
|
||
- Result: Migrations completed successfully (exit 0)
|
||
- Verified tables in DB: 7 tables present (`users`, `sessions`, `npcs`, `generation_counters`, `translations`, `notes`, `initiative_sessions`)
|
||
|
||
4. **Created down migration for reversibility**
|
||
- File: `drizzle/0001_curved_black_panther.down.sql`
|
||
- Operations: drop new tables, remove added columns/constraints from `users`
|
||
- Tested down migration against local DB: executed cleanly
|
||
- Re-applied up migration after test to restore expected state
|
||
|
||
### Key Findings
|
||
|
||
- **`.env` DATABASE_URL mismatch**: `.env` points to `dmuser:dmpass@localhost:5432/dmdashboard`, but the docker-compose PostgreSQL uses `postgres:postgres@localhost:5432/randify`. For DB commands to work locally, `DATABASE_URL` must be overridden inline or `.env` must be updated.
|
||
- **Drizzle-kit does not auto-generate `.down.sql` files**. Reversibility must be handled manually. The down migration should drop tables in reverse dependency order and remove columns after dropping their constraints.
|
||
- **Testing down migrations**: After testing a down migration, reset `drizzle.__drizzle_migrations` and re-run the official `db:migrate` script rather than applying raw SQL, to keep Drizzle's internal tracking consistent.
|
||
|
||
### Verification
|
||
|
||
- `npm run db:migrate` with correct `DATABASE_URL`: exit 0
|
||
- `\dt` in `randify` DB: 7 tables confirmed
|
||
- Down migration tested manually: all statements executed without error
|
||
|
||
---
|
||
|
||
# Notes and Initiative DB API Routes — Learnings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### Patterns Applied
|
||
|
||
1. **DM API route pattern (`src/pages/api/dm/notes.ts`, `src/pages/api/dm/initiative.ts`)**
|
||
- Both routes import `APIRoute` from `astro`, CORS helpers from `@/lib/cors`, and use `prerender = false`.
|
||
- Auth check is centralized in a `requireAuth()` helper that returns a `Response | null`. If `locals.user` is null, returns `jsonResponse({ error: "Unauthorized" }, 401, origin)`.
|
||
- All mutable methods (POST, PUT, DELETE) validate request body with Zod schemas before touching the DB.
|
||
- `PUT` and `DELETE` read the resource ID from `url.searchParams.get("id")`, not from the request body. This keeps the REST semantic clean.
|
||
|
||
2. **Zod validation per route**
|
||
- `notes.ts`: `createNoteSchema` requires `title` (string, 1-255 chars), `content` is optional. `updateNoteSchema` makes both fields optional.
|
||
- `initiative.ts`: `createSessionSchema` requires `name` (string, 1-255 chars), `participants` is optional array of `participantSchema` objects.
|
||
- On validation failure, return `400` with `jsonResponse({ error: "Validation failed", details: parsed.error.format() }, 400, origin)`.
|
||
|
||
3. **Cross-user access blocking**
|
||
- Every DB query that targets a single resource uses `and(eq(table.id, id), eq(table.userId, locals.user!.id))`.
|
||
- If `returning()` yields an empty array, the route returns `404` (not `403`). A 404 leaks less information about whether a resource exists at all.
|
||
- This was verified with explicit test cases for PUT and DELETE accessing another user's resource.
|
||
|
||
4. **CORS consistency**
|
||
- Both routes export `OPTIONS` handler calling `handleCorsPreflight(origin)`.
|
||
- Every response uses `jsonResponse()` or `createCorsResponse()` to ensure CORS headers are present.
|
||
- Origin is read from `request.headers.get("origin")` at the start of each handler.
|
||
|
||
5. **Mocking Drizzle ORM in unit tests**
|
||
- Mocking the entire `db` chain (`select().from().where().orderBy()`, `insert().values().returning()`, etc.) is fragile because `where` conditions are complex SQL objects.
|
||
- **Solution**: Mock `drizzle-orm`'s `eq` and `and` functions to return plain objects:
|
||
```ts
|
||
vi.mock("drizzle-orm", async (importOriginal) => {
|
||
const actual = await importOriginal<typeof import("drizzle-orm")>();
|
||
return {
|
||
...actual,
|
||
eq: (column: { name: string }, value: unknown) => ({ type: "eq", column: column.name, value }),
|
||
and: (...conditions: unknown[]) => ({ type: "and", conditions }),
|
||
};
|
||
});
|
||
```
|
||
- The mock DB then implements a simple `evaluateCondition()` recursive evaluator that checks `type === "eq"` against item properties (converting snake_case column names to camelCase).
|
||
- This allows the mock to correctly filter by `userId` and `id`, making cross-user access tests reliable.
|
||
|
||
6. **Test coverage**
|
||
- `tests/notes-api.test.ts`: 19 tests covering auth 401s, CORS OPTIONS, GET list, POST create, POST validation errors, PUT update, PUT 404 (missing + cross-user), DELETE remove, DELETE 404 (missing + cross-user), missing/invalid id params.
|
||
- `tests/initiative-api.test.ts`: 20 tests covering the same patterns plus participant validation (rejecting non-numeric `hp`, etc.).
|
||
|
||
### Key Findings
|
||
|
||
- **Do not import from `.d.ts` files in tests**: `import type { User } from "../src/env.d.ts"` fails with "File is not a module". Define mock objects inline with `as const` assertions instead.
|
||
- **Ensure mock user objects match the full `User` interface**: `env.d.ts` includes `boostyVerifiedAt: Date | null`. Missing it causes `TS2741` errors even in tests.
|
||
- **Avoid duplicate variable declarations after edits**: A partial `edit` replacement can leave behind duplicate `const` declarations (e.g., `mockUser2` defined twice), which TypeScript flags as redeclaration errors and vitest fails to transform. Always verify the file after edits.
|
||
|
||
### Verification
|
||
|
||
- `npx tsc --noEmit`: 0 errors
|
||
- `npx vitest run`: 271/271 tests passed (19 test files)
|
||
- `npx vitest run tests/notes-api.test.ts`: 19/19 passed
|
||
- `npx vitest run tests/initiative-api.test.ts`: 20/20 passed
|
||
|
||
---
|
||
|
||
# Rate Limiting Service for AI Generation — Learnings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### What Was Done
|
||
|
||
1. **Created `src/lib/rate-limit.ts`**
|
||
- `checkRateLimit(userId, tier)` — aggregates `sum(count)` across ALL models for the user's current hour window (`date_trunc('hour', now())`). Returns `{ allowed, remaining, resetAt }`.
|
||
- `getRemainingQuota(userId, tier)` — same aggregation logic, lightweight query for UI badges.
|
||
- `incrementGenerationCounter(userId, model)` — upserts per-model counter row using `onConflictDoUpdate` with composite unique target `(userId, hourWindow, model)`.
|
||
- `getRetryAfterSeconds()` — computes seconds until next hour boundary for 429 `Retry-After` header.
|
||
- FREE tier hard limit: 7/hour. PRO tier: 100/hour advisory (not hard-blocked).
|
||
|
||
2. **Created `src/pages/api/dm/quota.ts`**
|
||
- `GET` returns `{ remaining, resetAt, limit, tier }` for authenticated user.
|
||
- `OPTIONS` handler for CORS preflight.
|
||
- `prerender = false` required for middleware auth to populate `locals.user`.
|
||
|
||
3. **CORS fix (`src/lib/cors.ts`)**
|
||
- Added `PUT` and `DELETE` to `Access-Control-Allow-Methods`. Already verified in existing `tests/cors.test.ts`.
|
||
|
||
4. **Tests (`tests/rate-limit.test.ts`)**
|
||
- 14 tests covering: within limit, at limit, over limit, pro advisory behavior, resetAt correctness, tier differentiation, increment upsert, Retry-After logic.
|
||
- Mock DB pattern: mock `@/db/client` with `select` returning configurable total, `insert` capturing values and resolving `onConflictDoUpdate`.
|
||
- `vi.mock("drizzle-orm", ...)` needed to mock `eq`, `and`, and `sql` template tag so Drizzle query chains don't crash in vitest.
|
||
|
||
### Key Findings
|
||
|
||
- **Aggregating vs per-model limits**: The `generationCounters` schema has a composite unique index on `(userId, hourWindow, model)`, so counters are stored per-model. The rate limit check must `sum(count)` across all models to enforce a per-user total limit, while `incrementGenerationCounter` still tracks per-model for analytics.
|
||
- **`sql` template tag mocking**: In vitest, `` sql`expr` `` is a template tag function. The mock must accept `(strings: TemplateStringsArray, ...values)` and return a serializable object, otherwise Drizzle's internal SQL object breaks mocked query chains.
|
||
- **File write stability**: The `write` tool occasionally fails or reverts for existing files in this workspace. Using `bash` with `cat > file << 'EOF'` is more reliable for overwriting.
|
||
- **`getRetryAfterSeconds` at hour boundary**: At exactly 00:00 of an hour, the "next hour" is 3600 seconds away, not 0. This is correct behavior for `Retry-After` since the new hour window just started.
|
||
|
||
### Verification
|
||
|
||
- `npx tsc --noEmit`: 0 errors
|
||
- `npx vitest run tests/rate-limit.test.ts`: 14/14 passed
|
||
- `npx vitest run` (full suite): 285/285 passed (20 test files)
|
||
|
||
---
|
||
|
||
# Rate Limiting Service for AI Generation Routes — Learnings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### Patterns Applied
|
||
|
||
1. **Rate limiting service (`src/lib/rate-limit.ts`)**
|
||
- `checkRateLimit(userId, tier)` queries the current hour's generation count BEFORE the AI API call to avoid wasting provider quota.
|
||
- `incrementGenerationCounter(userId, model)` upserts the counter AFTER a successful AI response using PostgreSQL `ON CONFLICT DO UPDATE`.
|
||
- `getRemainingQuota(userId, tier)` provides a lightweight query for UI badges.
|
||
- `getRetryAfterSeconds()` computes seconds until the next hour for 429 `Retry-After` headers.
|
||
- Tier limits are exported as `TIER_LIMITS` (`free: 7`, `pro: 100`).
|
||
|
||
2. **Hourly reset via `date_trunc('hour', now())`**
|
||
- The `hourWindow` column uses `sql`date_trunc('hour', now())`` in both SELECT and INSERT/UPSERT queries.
|
||
- This ensures counters reset at the top of each hour, not on a rolling window.
|
||
|
||
3. **FREE hard-block vs PRO advisory**
|
||
- FREE tier: `allowed = count < limit` — hard-blocked at the limit.
|
||
- PRO tier: `allowed = true` always — advisory only, allows minor burst over 100/hour.
|
||
- The `remaining` value can go negative for PRO to signal advisory overage.
|
||
|
||
4. **Race-safe upsert with Drizzle `onConflictDoUpdate`**
|
||
- Instead of read-then-write (race-prone), the insert uses `onConflictDoUpdate` with the unique index columns as target.
|
||
- The `set` clause increments `count` atomically: `sql`${generationCounters.count} + 1``.
|
||
|
||
5. **Quota API route (`src/pages/api/dm/quota.ts`)**
|
||
- Exposes `GET /api/dm/quota` returning `{ remaining, resetAt, limit, tier }`.
|
||
- Requires auth (`locals.user`).
|
||
- Follows DM API route pattern: CORS headers, `prerender = false`, `OPTIONS` preflight handler.
|
||
|
||
6. **CORS methods fix**
|
||
- Added `PUT` and `DELETE` to `Access-Control-Allow-Methods` in `src/lib/cors.ts`.
|
||
- Updated `tests/cors.test.ts` assertions to match.
|
||
- Required for notes/initiative API routes that use PUT/DELETE.
|
||
|
||
### Testing Notes
|
||
|
||
- Mocking `drizzle-orm` in vitest: mocking `eq`, `and`, and `sql` to return plain objects allows the mock DB to evaluate `where` conditions without complex SQL parsing.
|
||
- Dynamic imports inside tests (`await import("@/lib/rate-limit")`) avoid vitest module caching issues when combined with `vi.mock`.
|
||
- The `getRetryAfterSeconds` test uses `vi.useFakeTimers()` to verify exact hour-boundary behavior.
|
||
|
||
### Verification
|
||
|
||
- `npx vitest run tests/rate-limit.test.ts`: 14/14 passed
|
||
- `npx vitest run tests/cors.test.ts`: 12/12 passed
|
||
- `npx vitest run` (full suite): 285/285 passed (20 test files)
|
||
- `npx tsc --noEmit`: 0 errors
|
||
- `npm run build`: fails due to pre-existing auth env validation (`JWT_SECRET` etc. missing in build env) — not related to rate-limiting changes
|
||
|
||
### Key Findings
|
||
|
||
- **Concurrent agent modification**: During implementation, `src/lib/rate-limit.ts` and `tests/rate-limit.test.ts` were repeatedly overwritten by another concurrent agent. The final accepted implementation aggregates counts across all models per user/hour (using `coalesce(sum(count), 0)`) rather than per-model. This aligns with the task requirement `checkRateLimit(userId, tier)` which does not accept a model parameter.
|
||
- **Do not use `vi.mock` factory with top-level variables**: The factory is hoisted before variable initialization, causing `ReferenceError` for `const`/`let` bindings. Use dynamic imports for the module under test, or define mock objects inside the factory.
|
||
|
||
---
|
||
|
||
# DM Dashboard AI Tab Navigation — Learnings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### What Was Done
|
||
|
||
1. **Added AI ("ИИ") tab to DM Dashboard navigation**
|
||
- Updated `src/i18n/dm-translations.ts` with new keys:
|
||
- `ai`, `anchorAi`, `tabAi` — tab/anchor labels
|
||
- `aiSignInCta: "Войдите, чтобы использовать ИИ"` — unauthenticated CTA message
|
||
- `aiPlaceholder: "Генератор контента с помощью ИИ"` — authenticated placeholder
|
||
- Updated `src/components/dm/DmTabs.astro` — added `{ id: "ai", label: T.ai, hash: "#ai" }` as 5th tab in both server-rendered array and client-side script array.
|
||
- Updated `src/components/dm/DmSidebar.astro` — added `{ id: "ai", label: T.ai, icon: "✦" }` to the tools navigation list.
|
||
- Updated `src/layouts/DmLayout.astro` — extended `tabIds` array in mobile visibility script from `["dice", "initiative", "reference", "notes"]` to include `"ai"`.
|
||
|
||
2. **Added AI section to DM index pages**
|
||
- `src/pages/dm/index.astro` — added `<section id="ai" class="dm-tab-section">` inside the `main` slot, after initiative.
|
||
- `src/pages/ru/dm/index.astro` — mirrored the same section.
|
||
- Section uses `Astro.locals.user` to conditionally render:
|
||
- Authenticated: placeholder text inside `DmCard`
|
||
- Unauthenticated: sign-in CTA text inside `DmCard`
|
||
- Section follows the exact same heading pattern (orange `bg-[var(--accent)]` bar + `<h2>`) as dice and initiative.
|
||
|
||
### Key Findings
|
||
|
||
- **DmTabs client-side array must stay in sync with server-side array.** The `<script>` block in `DmTabs.astro` has a hardcoded `tabs` array that drives hash validation, keyboard navigation, and event dispatching. Adding the server tab without updating the client array breaks mobile tab switching.
|
||
- **DmLayout mobile visibility script must include the new tab ID.** The `tabIds` array in `DmLayout.astro` controls which sections are shown/hidden on mobile via `updateMobileSections()`. Missing `"ai"` here would cause the section to never appear on mobile, even when the tab is active.
|
||
- **Both EN and RU DM index pages must be updated.** `src/pages/dm/index.astro` and `src/pages/ru/dm/index.astro` are separate files with identical structure. Changes must be mirrored.
|
||
- **RU page pre-existing inconsistency:** `ru/dm/index.astro` passes no `user` prop to `DmSidebar` (pre-existing). I left it untouched per scope discipline, but `Astro.locals.user` is still available for the AI section conditional.
|
||
|
||
### Verification
|
||
|
||
- `npx tsc --noEmit`: 0 errors
|
||
|
||
---
|
||
|
||
# AI NPC Generation API Route — Learnings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### What Was Done
|
||
|
||
1. **Created `src/pages/api/dm/ai/generate.ts`**
|
||
- POST endpoint with `prerender = false` for middleware auth.
|
||
- Request body validated with Zod: `npcParamsSchema.extend({ useOpen5eReference: z.boolean().optional() })`.
|
||
- Flow:
|
||
1. Auth check (`requireAuth` helper) → 401 if unauthenticated.
|
||
2. Rate limit check (`checkRateLimit(user.id, tier)`) → 429 with `Retry-After` header if blocked.
|
||
3. Open5e reference fetch (`searchMonsters(role)`) only when `useOpen5eReference` is true. Failures are caught and the request continues without reference.
|
||
4. AI client selection: `generateKimiNPC` for PRO tier, `generateOpenRouterNPC` for FREE tier.
|
||
5. AI generation with params + optional reference.
|
||
6. Defensive Zod validation of AI response using `npcResultSchema` from shared types.
|
||
7. Save NPC to `npcs` table via `db.insert().values().returning()`.
|
||
8. Increment generation counter via `incrementGenerationCounter(user.id, modelName)`.
|
||
9. Return `{ npc: NPCResult & { id }, reference?: Monster }`.
|
||
- OPTIONS handler for CORS preflight.
|
||
|
||
2. **Created `tests/ai-generate-api.test.ts`**
|
||
- 10 tests covering:
|
||
- 401 unauthenticated
|
||
- 429 rate limited with `Retry-After` header assertion
|
||
- 400 invalid JSON
|
||
- 400 validation failure (missing required fields)
|
||
- 200 happy path for free user without Open5e reference
|
||
- 200 happy path for pro user with Open5e reference
|
||
- 200 graceful degradation when Open5e fetch fails
|
||
- 502 invalid AI response (schema validation failure)
|
||
- 502 AI generation throws (API error)
|
||
- 204 OPTIONS with CORS headers
|
||
|
||
3. **Mocking strategy**
|
||
- Mocked `@/lib/ai/openrouter`, `@/lib/ai/kimi`, `@/lib/open5e/client`, `@/lib/rate-limit`, and `@/db/client` using `vi.mock()` with `var`-declared mock functions.
|
||
- Used `vi.fn()` reassignment in `beforeEach` to reset mocks between tests.
|
||
- Dynamic imports (`await import("../src/pages/api/dm/ai/generate")`) inside each test to avoid vitest module caching issues with mocked dependencies.
|
||
|
||
### Key Findings
|
||
|
||
- **`jsonResponse` does not accept extra headers.** The helper in `src/lib/cors.ts` only takes 3 arguments: data, status, origin. For the 429 response that needs a `Retry-After` header, `createCorsResponse` must be used directly with `JSON.stringify` and explicit `Content-Type`.
|
||
- **Schema inconsistency between OpenRouter and shared types.** `src/lib/ai/types.ts` `npcResultSchema` requires `level: z.number().int()`, but `src/lib/ai/openrouter.ts`'s internal schema omits `level`. This means an OpenRouter response without `level` will pass OpenRouter's own validation but fail the defensive validation in the API route. All mock responses in tests include `level` to avoid this.
|
||
- **`var` declarations for `vi.mock` factories.** Vitest hoists `vi.mock` calls, so variables referenced inside the factory must be declared with `var` (not `const`/`let`) to avoid `ReferenceError` from hoisting.
|
||
- **Graceful degradation for Open5e.** If `searchMonsters` throws or returns empty results, the route continues without reference rather than failing the entire request. This is better UX since the reference is optional enrichment, not a hard dependency.
|
||
- **Counter increment is non-fatal.** If `incrementGenerationCounter` throws (e.g., DB transient error), the catch block is empty and the successful NPC response is still returned. The user got their content; analytics can tolerate a dropped counter.
|
||
|
||
### Verification
|
||
|
||
- `npx vitest run tests/ai-generate-api.test.ts`: 10/10 passed
|
||
- `npx vitest run` (full suite): 306/306 passed in 22 test files (1 pre-existing broken test file `tests/history-api.test.ts` with parse error, unrelated)
|
||
- `npx tsc --noEmit`: 0 errors in project code (1 pre-existing parse error in `tests/history-api.test.ts`)
|
||
- `npm run build`: fails due to pre-existing auth env validation at build-time (`JWT_SECRET` missing) — unrelated to this change
|
||
|
||
---
|
||
|
||
# NPC Result Card Component — Planning Findings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### Existing Patterns Analyzed
|
||
|
||
**DmCard.astro** (`src/components/dm/DmCard.astro`):
|
||
- Base classes: `rounded-xl bg-[var(--bg-card)] border border-[var(--border-gold-strong)] shadow-lg shadow-black/20 hover:border-[var(--border-gold)] transition-all duration-[var(--transition-base)]`
|
||
- Padding variants: `none` (""), `sm` (p-4), `md` (p-5), `lg` (p-6)
|
||
- Props: `class?: string`, `padding?: "none" | "sm" | "md" | "lg"`, `dataTestid?: string`
|
||
- Use `padding="lg"` for the NPC card to match the spacious feel of DM cards.
|
||
|
||
**DmButton.astro** (`src/components/dm/DmButton.astro`):
|
||
- Variants: `primary` (accent bg, white text), `secondary` (card bg + border), `ghost`
|
||
- Sizes: `sm` (px-4 py-1.5 text-sm rounded-lg), `md` (px-6 py-2.5 text-base rounded-xl), `lg`
|
||
- For action buttons, use `variant="secondary" size="sm"` for "Copy JSON" and `variant="primary" size="sm"` for "Regenerate".
|
||
|
||
**DM Theme CSS (`src/styles/dm-theme.css`)**:
|
||
- Backgrounds: `--bg-primary: #16120e`, `--bg-card: #221e18`, `--bg-secondary: #1e1912`
|
||
- Text: `--text-primary: #f4f4f5`, `--text-secondary: #a1a1aa`, `--text-muted: #71717a`, `--text-cream: #e8dcc8`
|
||
- Borders: `--border-color: #3a3428`, `--border-gold: rgba(200,168,75,0.15)`, `--border-gold-strong: rgba(180,150,80,0.1)`
|
||
- Gold accents: `--gold: #c8a84b`, `--gold-light: #d4b76a`, `--gold-dark: #a88a3a`
|
||
|
||
**⚠️ CRITICAL: Theme Discrepancy — Purple vs Orange**
|
||
- `src/styles/dm-theme.css` sets `--accent: #534AB7` (purple), but AGENTS.md claims DM overrides to `#E87722` (orange).
|
||
- The task mandates: "Orange theme (#E87722), NO purple".
|
||
- **Recommendation**: Update `dm-theme.css` line 3 from `--accent: #534AB7` to `--accent: #E87722` and update `--accent-light`, `--accent-dark`, `--accent-hover` accordingly. This fixes the theme for ALL DM components at once (DmButton primary, focus rings, etc.). Then the NPC card can safely use `var(--accent)`.
|
||
- If modifying the theme CSS is out of scope, hardcode `#E87722` in the NPC card for all accent usage (stat values, story border, tags, buttons).
|
||
|
||
### NPCResult Schema (`src/lib/ai/types.ts`)
|
||
|
||
Available fields:
|
||
- `name: string`, `race: string`, `role: string`, `level: number`
|
||
- `hp: number`, `ac: number`, `cr: string`, `speed: string`
|
||
- `appearance: string`, `trait: string`, `motivation: string`, `secret: string`, `history: string`
|
||
|
||
**Missing fields for tags**: There is **no `tags`, `skills`, or `abilities` array** in `NPCResult`. The requirement asks for "Tags: skills, abilities (pill chips)".
|
||
- **Decision**: Derive pseudo-tags from `race` and `role` (display both as chips), OR omit the tags section entirely since the data model doesn't support it. Recommending the pseudo-tag approach for visual completeness.
|
||
|
||
### Sanitization Strategy
|
||
|
||
**Astro auto-escapes server-rendered expressions** — `{npc.name}`, `{npc.appearance}`, etc. are safe by default. No manual escaping needed in the `.astro` template.
|
||
|
||
**No `innerHTML` usage**: The card can be fully static markup. Interactivity (copy JSON, regenerate) uses `addEventListener` on existing DOM nodes. Copy JSON does `navigator.clipboard.writeText(JSON.stringify(npc))`. Regenerate calls the passed callback.
|
||
|
||
**No shared `escapeHtml` utility** exists in `src/lib/client/`. It is duplicated in 3 files. For this component, since it's pure Astro template with no dynamic HTML injection, `escapeHtml` is unnecessary. But if future iterations add client-side NPC rendering, consolidate `escapeHtml` into `src/lib/client/escape-html.ts`.
|
||
|
||
### i18n Gaps
|
||
|
||
`src/i18n/dm-translations.ts` has **no NPC-specific labels**. Must add:
|
||
```ts
|
||
// NPC labels
|
||
npcName: "Имя",
|
||
npcRace: "Раса",
|
||
npcRole: "Класс",
|
||
npcLevel: "Уровень",
|
||
npcHp: "ХП",
|
||
npcAc: "КЗ",
|
||
npcCr: "ОП",
|
||
npcSpeed: "Скорость",
|
||
npcAppearance: "Внешность",
|
||
npcTrait: "Черта",
|
||
npcMotivation: "Мотивация",
|
||
npcSecret: "Тайна",
|
||
npcHistory: "История",
|
||
npcCopyJson: "Копировать JSON",
|
||
npcCopied: "Скопировано",
|
||
npcRegenerate: "Перегенерировать",
|
||
```
|
||
|
||
### Clipboard Utility
|
||
|
||
`src/lib/client/clipboard.ts` exports `CopyFeedback` class for copy → checkmark icon swap animation. Can be used for the "Copy JSON" button if implementing a visual feedback state.
|
||
|
||
### Recommended Component Structure
|
||
|
||
```astro
|
||
---
|
||
import DmCard from "./DmCard.astro";
|
||
import DmButton from "./DmButton.astro";
|
||
import { dmTranslations as T } from "@/i18n/dm-translations";
|
||
import type { NPCResult } from "@/lib/ai/types";
|
||
|
||
export interface Props {
|
||
npc: NPCResult;
|
||
onRegenerate?: () => void;
|
||
}
|
||
|
||
const { npc, onRegenerate } = Astro.props;
|
||
---
|
||
|
||
<DmCard padding="lg" dataTestid="ai-npc-result">
|
||
<!-- Header: Name + Meta + Actions -->
|
||
<div class="flex items-start justify-between gap-4 mb-5">
|
||
<div class="min-w-0">
|
||
<h3 class="text-xl font-bold text-[var(--text-primary)] truncate">{npc.name}</h3>
|
||
<p class="text-sm text-[var(--text-secondary)] mt-1">
|
||
{npc.race} · {npc.role} · Ур. {npc.level} · ОП {npc.cr}
|
||
</p>
|
||
</div>
|
||
<div class="flex items-center gap-2 shrink-0">
|
||
<DmButton variant="secondary" size="sm" id="npc-copy-btn" dataTestid="npc-copy-btn">
|
||
{T.npcCopyJson}
|
||
</DmButton>
|
||
{onRegenerate && (
|
||
<DmButton variant="primary" size="sm" id="npc-regen-btn" dataTestid="npc-regen-btn">
|
||
{T.npcRegenerate}
|
||
</DmButton>
|
||
)}
|
||
</div>
|
||
</div>
|
||
|
||
<!-- Stats Row: HP, AC, Speed, CR (4-col grid) -->
|
||
<div class="grid grid-cols-2 sm:grid-cols-4 gap-3 mb-5">
|
||
<StatBox label={T.npcHp} value={String(npc.hp)} />
|
||
<StatBox label={T.npcAc} value={String(npc.ac)} />
|
||
<StatBox label={T.npcSpeed} value={npc.speed} />
|
||
<StatBox label={T.npcCr} value={npc.cr} />
|
||
</div>
|
||
|
||
<!-- Attributes: Appearance, Trait, Motivation, Secret -->
|
||
<div class="space-y-3 mb-5">
|
||
<AttributeRow label={T.npcAppearance} value={npc.appearance} />
|
||
<AttributeRow label={T.npcTrait} value={npc.trait} />
|
||
<AttributeRow label={T.npcMotivation} value={npc.motivation} />
|
||
<AttributeRow label={T.npcSecret} value={npc.secret} />
|
||
</div>
|
||
|
||
<!-- Story Block: History/Backstory -->
|
||
<div class="border-l-4 border-[#E87722] pl-4 py-2 italic text-[var(--text-cream)] bg-[var(--bg-secondary)]/50 rounded-r-lg">
|
||
<p class="text-sm leading-relaxed">{npc.history}</p>
|
||
</div>
|
||
|
||
<!-- Tags (pseudo-tags from race + role) -->
|
||
<div class="flex flex-wrap gap-2 mt-5">
|
||
<span class="px-3 py-1 text-xs font-medium rounded-full bg-[#E87722]/10 text-[#E87722] border border-[#E87722]/20">
|
||
{npc.race}
|
||
</span>
|
||
<span class="px-3 py-1 text-xs font-medium rounded-full bg-[#E87722]/10 text-[#E87722] border border-[#E87722]/20">
|
||
{npc.role}
|
||
</span>
|
||
</div>
|
||
</DmCard>
|
||
|
||
<script>
|
||
// Client-side: copy JSON + regenerate handler
|
||
document.addEventListener("DOMContentLoaded", () => {
|
||
const copyBtn = document.getElementById("npc-copy-btn");
|
||
const regenBtn = document.getElementById("npc-regen-btn");
|
||
|
||
if (copyBtn) {
|
||
copyBtn.addEventListener("click", async () => {
|
||
const card = copyBtn.closest('[data-testid="ai-npc-result"]');
|
||
const npcData = card?.getAttribute("data-npc");
|
||
if (!npcData) return;
|
||
try {
|
||
await navigator.clipboard.writeText(npcData);
|
||
const originalText = copyBtn.textContent;
|
||
copyBtn.textContent = "Скопировано";
|
||
setTimeout(() => { copyBtn.textContent = originalText; }, 1500);
|
||
} catch {
|
||
/* ignore clipboard errors */
|
||
}
|
||
});
|
||
}
|
||
|
||
if (regenBtn) {
|
||
regenBtn.addEventListener("click", () => {
|
||
regenBtn.dispatchEvent(new CustomEvent("npc-regenerate", { bubbles: true }));
|
||
});
|
||
}
|
||
});
|
||
</script>
|
||
```
|
||
|
||
**Note on data passing to client script**: Astro components cannot directly pass objects to `<script>` blocks. Options:
|
||
1. Serialize NPC as `data-npc` JSON attribute on the card container (`data-npc={JSON.stringify(npc)}`). The script reads it via `getAttribute` + `JSON.parse`. This is safe since Astro auto-escapes attribute values.
|
||
2. Have the parent page manage the copy/regenerate logic and dispatch custom events.
|
||
3. Use `is:inline` script with `define:vars` (Astro feature) to pass the npc object directly to the script.
|
||
|
||
**Recommended**: Use `define:vars` with `is:inline` script for clean data passing without DOM attribute parsing.
|
||
|
||
### Checklist for Implementation
|
||
|
||
- [ ] Add NPC i18n keys to `src/i18n/dm-translations.ts`
|
||
- [ ] Fix `dm-theme.css` `--accent` to `#E87722` (or hardcode orange in component)
|
||
- [ ] Create `src/components/dm/AiNpcResult.astro`
|
||
- [ ] Props: `npc: NPCResult`, `onRegenerate?: () => void`
|
||
- [ ] Use `DmCard` with `padding="lg"`
|
||
- [ ] Header: Name (bold, truncate), meta line (race · role · level · CR), Copy + Regenerate buttons
|
||
- [ ] Stats: 4-col grid (HP, AC, Speed, CR) with `bg-[var(--bg-secondary)]` boxes
|
||
- [ ] Attributes: 4 rows with label (muted, uppercase) + value
|
||
- [ ] Story block: left orange border (`border-l-4 border-[#E87722]`), italic, cream text, subtle bg
|
||
- [ ] Tags: pill chips for race + role (or omit if out of scope)
|
||
- [ ] Copy JSON: `navigator.clipboard.writeText(JSON.stringify(npc))` with brief "Скопировано" feedback
|
||
- [ ] Regenerate: dispatch custom event or call callback
|
||
- [ ] Zero `innerHTML` usage — pure Astro template + event listeners
|
||
- [ ] No favorite button
|
||
- [ ] Add `dataTestid` attributes for testing
|
||
|
||
---
|
||
|
||
# InitiativeTracker DB Persistence Migration — Learnings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### Context
|
||
|
||
Task: Migrate InitiativeTracker to use DB persistence for PRO users, keep sessionStorage for FREE.
|
||
|
||
### Current State Analysis
|
||
|
||
1. **`src/components/dm/InitiativeTracker.astro`** — Pure client-side component (418 lines).
|
||
- Uses `sessionStorage` key `"dm-initiative"` with legacy `"it-combatants"` fallback migration.
|
||
- `activeIndex` is a module-level variable — never persisted to any storage.
|
||
- No Astro props accepted. Duplicated inline `escapeHtml()` (pre-existing anti-pattern).
|
||
|
||
2. **`src/pages/api/dm/initiative.ts`** — Full CRUD API already exists.
|
||
- `GET` returns `[{ id, name, participants, createdAt, updatedAt }]` for authenticated user.
|
||
- `POST` creates session with `{ name, participants? }`.
|
||
- `PUT` updates by `?id=` with `{ name?, participants? }`.
|
||
- `DELETE` removes by `?id=`.
|
||
- Auth via `locals.user`; CORS via `jsonResponse()` helper.
|
||
|
||
3. **DB Schema** (`initiative_sessions`): `id`, `userId`, `name`, `participants` (JSONB), `createdAt`, `updatedAt`.
|
||
|
||
4. **Data model compatibility**: Client `CombatantData = { id, name, initiative, hp? }` maps cleanly into API `participantSchema`. The schema is **not** `.strict()`, so extra fields are stripped and optional fields are tolerated. Sending `{ id, name, initiative, hp }` from client is fully valid.
|
||
|
||
5. **Parent pages**: `src/pages/dm/index.astro` and `src/pages/ru/dm/index.astro` use `<InitiativeTracker />` with no props. `Astro.locals.user` provides `tier` and `id`.
|
||
|
||
### Implementation Plan
|
||
|
||
#### Props
|
||
```astro
|
||
export interface Props {
|
||
tier?: 'free' | 'pro';
|
||
userId?: number;
|
||
}
|
||
```
|
||
Parent pages pass: `<InitiativeTracker tier={user?.tier ?? 'free'} userId={user?.id} />`
|
||
|
||
#### Server-to-Client Bridge
|
||
Add `data-tier={tier}` and `data-user-id={userId ?? ''}` to root `<div>`. Client script reads:
|
||
```ts
|
||
const tier = (trackerEl?.dataset.tier as 'free' | 'pro') || 'free';
|
||
const userId = trackerEl?.dataset.userId ? parseInt(trackerEl.dataset.userId, 10) : undefined;
|
||
const isPro = tier === 'pro' && !!userId;
|
||
```
|
||
|
||
#### PRO-Only UI Panel (Astro template, conditional `{tier === 'pro'}`)
|
||
Rendered above add-combatant form inside a `DmCard`:
|
||
- `DmInput id="it-session-name"` — session name input.
|
||
- `DmButton id="it-save-session-btn"` — manual save trigger.
|
||
- `DmButton id="it-new-session-btn"` — clears current session state.
|
||
- `#it-session-list` — scrollable list of saved sessions (name + date + delete button).
|
||
- `#it-session-status` — transient success/error message.
|
||
|
||
#### New Translation Keys Needed
|
||
- `saveSession`, `newSession`, `sessionNamePlaceholder`, `noSavedSessions`, `sessionSaved`, `saveError`, `loadSessionError`, `deleteSessionConfirm`.
|
||
|
||
#### Client Script Additions
|
||
|
||
**State**:
|
||
```ts
|
||
let currentSessionId: number | null = null;
|
||
let currentSessionName = '';
|
||
```
|
||
|
||
**API wrappers**:
|
||
- `apiFetchSessions()` → GET /api/dm/initiative
|
||
- `apiSaveSession(name, participants, id?)` → POST (new) or PUT (update existing)
|
||
- `apiDeleteSession(id)` → DELETE /api/dm/initiative?id=X
|
||
|
||
**Core functions**:
|
||
- `renderSessionList()` — async fetch, renders rows with click-to-load and delete. Highlights current session.
|
||
- `loadDbSession(id)` — fetches sessions, maps `participants` → `CombatantData[]`, calls `saveCombatants()` to sessionStorage (graceful fallback), resets `activeIndex = 0`, re-renders.
|
||
- `handleSaveSession()` — reads name input (defaults to timestamp), POST or PUT, updates `currentSessionId`, shows status, refreshes list. On error: shows red status, **does not touch sessionStorage**.
|
||
- `handleNewSession()` — clears `currentSessionId/name/input`, calls `clearAll()`.
|
||
- `deleteDbSession(id)` — confirm dialog, DELETE, re-renders list.
|
||
|
||
**Event listeners** (attached defensively with `?.`):
|
||
```ts
|
||
document.getElementById('it-save-session-btn')?.addEventListener('click', handleSaveSession);
|
||
document.getElementById('it-new-session-btn')?.addEventListener('click', handleNewSession);
|
||
```
|
||
|
||
**Init**:
|
||
```ts
|
||
updateVisibility();
|
||
renderList();
|
||
if (isPro) renderSessionList();
|
||
```
|
||
|
||
### Key Design Decisions
|
||
|
||
| Decision | Rationale |
|
||
|----------|-----------|
|
||
| **Manual save only** | Requirement explicitly forbids auto-save. |
|
||
| **sessionStorage always kept** | `saveCombatants()` still writes to `dm-initiative`. On API failure, user loses nothing. On PRO save, sessionStorage is **not** deleted. |
|
||
| **activeIndex never in API payload** | It stays a pure in-memory var. Loading a DB session always resets it to 0. |
|
||
| **Data attributes for props** | Cleanest bridge for a module `<script>` without inlining vars via `define:vars`. |
|
||
| **Zod compatibility** | API schema is not `.strict()`. Client sends minimal payload; server accepts silently. |
|
||
| **Session name required** | API `createSessionSchema` requires `name`. Client defaults to `"Сессия {localeDate}"` if empty. |
|
||
| **Escape user content** | Session names rendered via existing `escapeHtml()` to prevent XSS in `innerHTML`. |
|
||
| **Missing userId fallback** | If `tier === 'pro'` but `userId` is missing, `isPro = false` and behavior falls back to FREE. |
|
||
|
||
### Files to Modify
|
||
|
||
1. `src/components/dm/InitiativeTracker.astro` — add props, conditional PRO UI, API integration, session management.
|
||
2. `src/i18n/dm-translations.ts` — add 8 new keys.
|
||
3. `src/pages/dm/index.astro` — pass `tier` and `userId` props.
|
||
4. `src/pages/ru/dm/index.astro` — mirror prop changes.
|
||
|
||
### Verification Steps
|
||
|
||
- `npx tsc --noEmit` — zero errors.
|
||
- `npx vitest run` — full suite passes (271+ tests).
|
||
- Manual QA:
|
||
- FREE user: sessionStorage works exactly as before.
|
||
- PRO user: add combatants → Save → reload → Load → combatants restored.
|
||
- PRO user: `activeIndex` resets to 0 on every session load.
|
||
- PRO user: offline/API failure → error shown, sessionStorage intact.
|
||
|
||
### Pre-existing Bug Note
|
||
|
||
`src/pages/dm/index.astro` contains a duplicated AI section (lines 80–98 mirror 59–77). Out of scope for this task but should be cleaned up separately.
|
||
|
||
---
|
||
|
||
# Task 22: FREE → PRO Import Flow — Learnings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### What Was Done
|
||
|
||
1. **Pure helpers in `src/lib/client/import.ts`** — `getLocalNotes()`, `getLocalInitiative()` (with `it-combatants` legacy fallback), `hasLocalData()`, `wasAsked()`, `markAsked()`, `shouldShowImport(tier, userId)`. Pure, testable, no DOM coupling.
|
||
2. **`src/components/dm/ImportModal.astro`** — modal renders only when `shouldShowImport()` returns true on mount. Trigger condition: `tier === 'pro'` && `userId` present && `hasLocalData()` && `!wasAsked()`. Spec says "on tier upgrade" — interpreted as "first PRO page load with local data, not yet asked".
|
||
3. **Conflict handling** — only for notes (initiative API allows multiple sessions, so import always creates a new session named `"Импорт {date}"`). If an existing note titled "DM Notes" has non-empty content, prompt Keep both / Replace / Skip. Keep both creates a second note titled "DM Notes (импорт)".
|
||
4. **localStorage / sessionStorage preserved** — per Task 22 "Must NOT do". Even on successful import, local copies are kept as backup; only the `dm-import-asked` flag is set.
|
||
5. **i18n keys** — 14 new keys added to `src/i18n/dm-translations.ts` (`importTitle`, `importPrompt`, `importBtn`, `importSkip`, etc.).
|
||
6. **Wired into both EN and RU dm/index.astro** with `<ImportModal tier={user?.tier ?? 'free'} userId={user?.id} />`.
|
||
7. **Unit tests** — `tests/import-flow.test.ts`, 18 tests covering all branches of the helpers + decision matrix.
|
||
|
||
### Key Findings
|
||
|
||
- **Detecting tier upgrade is hard without server-side event**: The plan says "show on tier upgrade event". Without a server-side notification channel, the practical proxy is "first PRO page load with local data that hasn't been asked yet". The `dm-import-asked` flag in localStorage suppresses re-asking.
|
||
- **API parity**: Notes have one "DM Notes" canonical record per user (NotesPanel convention) — import upserts that. Initiative API allows multiple sessions, so import just POSTs a new one.
|
||
- **Don't pre-fetch on every PRO load**: The decision `shouldShowImport` is cheap (local storage reads), but the existence check against the DB (for note conflict) only happens AFTER the user clicks "Import" — keeps the page load lean.
|
||
|
||
### Verification
|
||
|
||
- `npx tsc --noEmit`: 0 errors
|
||
- `npx vitest run tests/import-flow.test.ts`: 18/18 passed
|
||
- `npx vitest run` (full suite): 339/339 passed in 25 test files
|
||
|
||
---
|
||
|
||
# Final Wave Cleanup — Learnings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### Verdicts
|
||
|
||
- **F1 Plan Compliance Audit (oracle)**: APPROVE — 10/10 Must Have, 10/10 Must NOT Have clean, 24/24 tasks marked complete.
|
||
- **F2 Code Quality Review**: initially REJECT WITH ISSUES — 98 lint errors. Fixed all 13 production-code findings; brought total to 70 (test-only).
|
||
- **F3 Manual QA**: skipped — required Playwright + running dev server; build is blocked at this env without `JWT_SECRET` etc.
|
||
- **F4 Scope Fidelity**: APPROVE — minor non-blocking flags: `test-results/` Playwright artifacts committed in Wave 2 (should be gitignored); `dm-theme.css` purple → orange swap in Wave 6 not bound to a specific task line item but aligned with guardrails.
|
||
|
||
### Production-Code Lint Fixes
|
||
|
||
- `GenerationHistory.astro` — dropped unused `userId` param from `fetchProHistory`; changed `let activeId` to `const`.
|
||
- `InitiativeTracker.astro` — removed write-only `currentSessionName` and its 4 assignments; sessionNameInput is the visible source of truth.
|
||
- `NotesPanel.astro` — removed unused `isLoading` flag.
|
||
- `Open5eReference.astro` — dropped unused `setCachedTranslation` and `getCachedTranslation` imports; used `translated` from `fetchTranslation` directly (eliminated wasteful re-lookup).
|
||
- `src/lib/ai/openrouter.ts` — removed unused `z` and `Monster` imports; added `{ cause: err }` to AbortError rethrows.
|
||
- `src/lib/ai/kimi.ts` — removed unused `z` import.
|
||
- `src/pages/api/dm/ai/generate.ts` — annotated the empty `catch` block: counter increment is best-effort.
|
||
- `src/pages/api/dm/ai/history.ts` — removed unused `sql` import.
|
||
|
||
### Remaining Lint Tech Debt (70 errors, all in test files)
|
||
|
||
70 `@typescript-eslint/no-explicit-any` errors in test mocks for Drizzle ORM query chains (`tests/notes-api.test.ts`, `initiative-api.test.ts`, `translate-api.test.ts`, `history-api.test.ts`, `ai-generate-api.test.ts`, `cors.test.ts`). Replacing these with proper types requires modeling Drizzle's complex chained SQL builder shape — out of scope for the final verification wave. These were committed across Waves 5–6 by prior agent runs and should be addressed in a dedicated test-refactor sweep.
|
||
|
||
### Other Cleanups
|
||
|
||
- `tests/translation-client.test.ts` — dropped unused `_name` param from `MockBroadcastChannel`; removed unused destructured `getCachedTranslation`.
|
||
- `tests/open5e-types.test.ts` — added eslint-disable comments for the intentional type-test aliases.
|
||
- `tests/{initiative,notes}-api.test.ts` — removed unused `mockUser2` placeholder objects.
|
||
- `tests/{notes,initiative,translate,history}-api.test.ts` — changed `let mockDb` to `const mockDb` (never reassigned).
|
||
- `tests/ai-generate-api.test.ts` — wrapped the `var` mock declarations in `eslint-disable no-var`; documented that vi.mock hoisting requires `var`.
|
||
|
||
### Final State
|
||
|
||
- tsc: 0 errors
|
||
- vitest: 339/339 pass (25 files)
|
||
- lint: 70 errors (all test-mock `as any`, all pre-existing from Waves 5–6)
|
||
- build: still blocked by missing JWT_SECRET / OAuth env at build time (pre-existing, documented in Wave 1 learnings)
|
||
|
||
---
|
||
|
||
# FREE → PRO Data Import Flow — Planning Findings
|
||
|
||
## Date: 2026-05-15
|
||
|
||
### Context
|
||
|
||
Task: Build an import modal that migrates FREE-tier localStorage/sessionStorage data into PRO-tier DB when a user upgrades.
|
||
|
||
### Existing Patterns Analyzed
|
||
|
||
**1. Modal / Overlay Pattern (`Open5eReference.astro`)**
|
||
- Container: `fixed inset-0 z-50 bg-black/60 backdrop-blur-sm flex items-start justify-center p-4 pt-12`
|
||
- Background click-to-close: absolute `inset-0` div with click listener
|
||
- Content card: `DmCard padding="lg"` with `w-full max-w-2xl max-h-[80vh] overflow-y-auto relative border-2 border-[var(--accent)] z-10`
|
||
- Close button: absolute top-right with SVG × icon, `aria-label="Закрыть"`, `focus-visible:ring-2 focus-visible:ring-[var(--accent)]`
|
||
- ARIA: `role="dialog" aria-modal="true"`
|
||
|
||
**2. DmButton.astro Variants**
|
||
- `primary` = `bg-[var(--accent)] text-white` — orange theme, use for main action (Import)
|
||
- `secondary` = card bg + border — use for secondary actions
|
||
- `ghost` = transparent — use for dismiss (Skip)
|
||
- Sizes: `sm` for compact modal actions
|
||
|
||
**3. DmCard.astro**
|
||
- Base: `rounded-xl bg-[var(--bg-card)] border border-[var(--border-gold-strong)] shadow-lg`
|
||
- Padding variants: `none`, `sm` (p-4), `md` (p-5), `lg` (p-6)
|
||
- Use `padding="lg"` for the modal content card
|
||
|
||
**4. Theme System (`src/styles/dm-theme.css`)**
|
||
- `--accent: #E87722` is already correctly set in `.dm-theme`
|
||
- Using `var(--accent)` anywhere inside `.dm-theme` scope yields orange
|
||
- NO purple anywhere — `var(--accent)` is safe
|
||
|
||
**5. Data Storage Locations**
|
||
- **Notes**: `localStorage` key `dm-notes` — plain string (note content)
|
||
- **Initiative**: `sessionStorage` key `dm-initiative` — JSON array of `CombatantData[]` with shape `{ id, name, initiative, hp?, maxHp?, modifier?, ac?, isPlayer?, active? }`
|
||
- **Legacy initiative fallback**: `sessionStorage` key `it-combatants` — older format, already migrated by `InitiativeTracker.astro` on load
|
||
|
||
**6. API Routes**
|
||
- `GET /api/dm/notes` → returns `Array<{ id, title, content, createdAt, updatedAt }>` for authenticated user
|
||
- `POST /api/dm/notes` → body `{ title: string, content?: string }` → creates note, returns `{ id, title, content, createdAt, updatedAt }`
|
||
- `PUT /api/dm/notes?id=X` → body `{ title?, content? }` → updates existing note
|
||
- `GET /api/dm/initiative` → returns `Array<{ id, name, participants, createdAt, updatedAt }>` for authenticated user
|
||
- `POST /api/dm/initiative` → body `{ name: string, participants?: Array<Participant> }` → creates session
|
||
- `PUT /api/dm/initiative?id=X` → body `{ name?, participants? }` → updates existing session
|
||
- All routes require auth (`locals.user`), return 401 if unauthenticated
|
||
- CORS headers handled by `jsonResponse()` helper
|
||
|
||
**7. NotesPanel DB Integration Pattern**
|
||
- NotesPanel looks for note with `title === 'DM Notes'` or falls back to `notes[0]`
|
||
- When loading DB notes, if none exists, it auto-creates an empty note via POST
|
||
- This means a PRO user who has visited the notes tab may already have a blank "DM Notes" entry in DB
|
||
- **Implication for import**: a blank note (content null/empty) should probably be treated as "no existing data" for conflict purposes, OR we should replace it. The import modal should check `content?.trim()` length.
|
||
|
||
**8. Tier & Auth Flow**
|
||
- Middleware attaches `Astro.locals.user` with `tier: 'free' | 'pro'`
|
||
- `src/pages/dm/index.astro` passes `tier={user?.tier ?? 'free'} userId={user?.id}` to child components
|
||
- There is **no server-side mechanism** to detect a tier upgrade (server can't read localStorage)
|
||
- Tier change detection must be client-side: store previous tier in `localStorage` (`dm-tier`), compare on page load
|
||
|
||
---
|
||
|
||
### Implementation Plan
|
||
|
||
#### New File: `src/components/dm/ImportModal.astro`
|
||
|
||
**Props:**
|
||
```astro
|
||
export interface Props {
|
||
tier?: 'free' | 'pro';
|
||
}
|
||
```
|
||
|
||
**Aero Template Structure:**
|
||
```astro
|
||
<div
|
||
id="dm-import-modal"
|
||
class="hidden fixed inset-0 z-50 bg-black/60 backdrop-blur-sm flex items-start justify-center p-4 pt-12"
|
||
role="dialog"
|
||
aria-modal="true"
|
||
data-tier={tier}
|
||
data-testid="import-modal"
|
||
>
|
||
<!-- Click-outside backdrop -->
|
||
<div id="dm-import-overlay" class="absolute inset-0" data-testid="import-modal-overlay"></div>
|
||
|
||
<DmCard padding="lg" class="w-full max-w-lg relative z-10 border-2 border-[var(--accent)]">
|
||
<!-- Header -->
|
||
<div class="flex items-center justify-between mb-4">
|
||
<h2 class="text-lg font-bold text-[var(--text-primary)]">{T.importTitle}</h2>
|
||
<button id="dm-import-close" type="button" ... aria-label={T.actionClose}>
|
||
<svg>...</svg>
|
||
</button>
|
||
</div>
|
||
|
||
<!-- Dynamic content container -->
|
||
<div id="dm-import-content" class="space-y-4"></div>
|
||
|
||
<!-- Action buttons -->
|
||
<div id="dm-import-actions" class="flex items-center justify-end gap-2 mt-6">
|
||
<DmButton id="dm-import-skip" variant="ghost" size="sm">{T.importSkip}</DmButton>
|
||
<DmButton id="dm-import-start" variant="primary" size="sm">{T.importStart}</DmButton>
|
||
</div>
|
||
</DmCard>
|
||
</div>
|
||
```
|
||
|
||
**Client Script Logic (state machine):**
|
||
|
||
```ts
|
||
enum State {
|
||
DETECTING = 'detecting', // checking local/session storage + DB preflight
|
||
REVIEW = 'review', // show found data + conflict options
|
||
IMPORTING = 'importing', // show progress per step
|
||
COMPLETE = 'complete', // success, dispatch event
|
||
ERROR = 'error', // show error + retry
|
||
NO_DATA = 'no-data', // nothing to import, auto-dismiss
|
||
}
|
||
```
|
||
|
||
**Flow:**
|
||
|
||
1. **Mount / Auto-detect:**
|
||
- Read `container.dataset.tier` as current tier
|
||
- Read `localStorage.getItem('dm-tier')` as previous tier
|
||
- If `prevTier !== 'free' || currentTier !== 'pro'` → do nothing (not an upgrade event)
|
||
- If upgrade detected:
|
||
- Read `localStorage.getItem('dm-notes')` → `hasNotes = !!value && value.trim().length > 0`
|
||
- Read `sessionStorage.getItem('dm-initiative')` → `hasInitiative = !!value && JSON.parse(value).length > 0`
|
||
- If `!hasNotes && !hasInitiative` → show NO_DATA state, auto-dismiss after 2s
|
||
- Else → proceed to DETECTING
|
||
|
||
2. **DETECTING state:**
|
||
- If `hasNotes`: `fetch('/api/dm/notes')` to check for existing DB notes
|
||
- If `hasInitiative`: `fetch('/api/dm/initiative')` to check for existing DB sessions
|
||
- Store preflight results: `existingNotes: Note[]`, `existingSessions: Session[]`
|
||
- Transition to REVIEW
|
||
|
||
3. **REVIEW state:**
|
||
- Show bullet list of found items:
|
||
- "Найдены заметки" (if hasNotes)
|
||
- "Найдена сессия инициативы" (if hasInitiative)
|
||
- For each item where DB already has data, show conflict radio group:
|
||
- Label: "В базе уже есть [заметки / сессии]. Что делать?"
|
||
- Options (default selected: "keep-both"):
|
||
- `keep-both`: "Сохранить оба" — create NEW DB entry with imported data
|
||
- `replace`: "Заменить" — update the most recent existing DB entry
|
||
- `skip`: "Пропустить" — do not import this data type
|
||
- Buttons: "Импортировать" (primary) / "Пропустить" (ghost)
|
||
|
||
4. **IMPORTING state:**
|
||
- Hide action buttons, show progress list
|
||
- For each data type in order (notes → initiative):
|
||
- Show "Импорт [типа]..." with spinner
|
||
- Execute POST/PUT
|
||
- On success: update to "[Тип] импортирован ✓"
|
||
- After all done → transition to COMPLETE
|
||
|
||
5. **COMPLETE state:**
|
||
- Show "Импорт завершён! ✓"
|
||
- Show summary: "Заметки: импортированы, Сессия: импортирована"
|
||
- Single button: "Закрыть" (primary)
|
||
- On close: dispatch `CustomEvent('import-complete', { bubbles: true })`
|
||
|
||
6. **ERROR state:**
|
||
- Show which step failed + error message from response
|
||
- Buttons: "Повторить" (primary) / "Пропустить" (ghost)
|
||
- Retry re-attempts only the failed step(s)
|
||
|
||
7. **Dismissal (Skip / Close / Backdrop click / Escape):**
|
||
- Hide modal with `classList.add('hidden')`
|
||
- Dispatch `CustomEvent('import-dismissed', { bubbles: true })`
|
||
- Do NOT delete localStorage/sessionStorage
|
||
- Store a flag `localStorage.setItem('dm-import-dismissed', '1')` to prevent re-prompting on subsequent page loads during the same session? (optional — task says "only on tier upgrade event", so on next login the prevTier will already be 'pro', so it won't trigger again. But if user refreshes the page immediately after dismissing, the prevTier in localStorage is still 'free' because we haven't updated it yet.)
|
||
- **Important**: Update `localStorage.setItem('dm-tier', 'pro')` immediately upon showing the modal, OR upon any dismissal/completion, so that a refresh doesn't re-trigger. Recommendation: update `dm-tier` to current tier as soon as the modal opens.
|
||
|
||
**Conflict Resolution Details:**
|
||
|
||
| Data Type | Keep Both | Replace | Skip |
|
||
|-----------|-----------|---------|------|
|
||
| **Notes** | POST new note with `title: 'Заметки (импорт)'`, `content: localStorage notes` | PUT the most recent existing note (`existingNotes[existingNotes.length-1].id`) with `content: localStorage notes` | No API call |
|
||
| **Initiative** | POST new session with `name: 'Импортированная сессия'`, `participants: sessionStorage combatants` | PUT the most recent existing session (`existingSessions[existingSessions.length-1].id`) with `name: 'Импортированная сессия'`, `participants: sessionStorage combatants` | No API call |
|
||
|
||
**Note on "Replace" for notes**: NotesPanel looks for `title === 'DM Notes'`. If the existing note has a different title and we PUT its content, NotesPanel will still find it (falls back to `notes[0]`). If the user already has a "DM Notes" entry, replacing it updates that exact note. This is the desired behavior.
|
||
|
||
**API Error Handling:**
|
||
- `401 Unauthorized` → shouldn't happen for a PRO user, but show "Сессия устарела. Обновите страницу."
|
||
- `400 Validation failed` → shouldn't happen with well-formed data, but show generic error
|
||
- `fetch` throws (network) → show "Ошибка соединения. Проверьте интернет."
|
||
- Any error → transition to ERROR state with specific message
|
||
|
||
**Keyboard Accessibility:**
|
||
- `Escape` key closes/dismisses modal
|
||
- `Tab` traps focus inside modal while open (optional but recommended)
|
||
- Close button and backdrop click both dismiss
|
||
|
||
**Data Attributes for Testing:**
|
||
- `data-testid="import-modal"`
|
||
- `data-testid="import-modal-overlay"`
|
||
- `data-testid="import-content"`
|
||
- `data-testid="import-actions"`
|
||
- `data-testid="import-start-btn"`
|
||
- `data-testid="import-skip-btn"`
|
||
- `data-testid="import-close-btn"`
|
||
- `data-testid="import-retry-btn"`
|
||
- `data-state` on content container reflecting current state
|
||
|
||
---
|
||
|
||
#### Modified File: `src/i18n/dm-translations.ts`
|
||
|
||
Add the following keys (all Russian, RU-only DM Dashboard):
|
||
|
||
```ts
|
||
// Import Modal
|
||
importTitle: "Импорт данных",
|
||
importDescription: "Обнаружены данные из бесплатной версии. Хотите импортировать их в PRO?",
|
||
importNotesFound: "Найдены заметки",
|
||
importInitiativeFound: "Найдена сессия инициативы",
|
||
importNoData: "Нет данных для импорта",
|
||
importNoDataDesc: "Ваши локальные данные уже синхронизированы или отсутствуют.",
|
||
importStart: "Импортировать",
|
||
importSkip: "Пропустить",
|
||
importClose: "Закрыть",
|
||
importInProgressNotes: "Импорт заметок...",
|
||
importInProgressInitiative: "Импорт сессии инициативы...",
|
||
importDoneNotes: "Заметки импортированы",
|
||
importDoneInitiative: "Сессия инициативы импортирована",
|
||
importComplete: "Импорт завершён!",
|
||
importCompleteDesc: "Все данные успешно перенесены в PRO.",
|
||
importError: "Ошибка импорта",
|
||
importErrorDesc: "Не удалось импортировать: {item}. Попробуйте снова.",
|
||
importRetry: "Повторить",
|
||
importConflictTitle: "В базе уже есть данные",
|
||
importConflictNotesLabel: "Заметки:",
|
||
importConflictInitiativeLabel: "Инициатива:",
|
||
importKeepBoth: "Сохранить оба",
|
||
importReplace: "Заменить",
|
||
importSkipItem: "Пропустить",
|
||
```
|
||
|
||
---
|
||
|
||
#### Modified Files: `src/pages/dm/index.astro` + `src/pages/ru/dm/index.astro`
|
||
|
||
1. Import the component:
|
||
```astro
|
||
import ImportModal from "@/components/dm/ImportModal.astro";
|
||
```
|
||
|
||
2. Add the modal inside `DmLayout` (best placement: at the end of the layout, outside slots, so it overlays everything):
|
||
```astro
|
||
<ImportModal tier={user?.tier ?? 'free'} />
|
||
```
|
||
|
||
3. No additional client script needed in the parent — `ImportModal` handles its own tier-change detection internally.
|
||
|
||
**RU page note**: `src/pages/ru/dm/index.astro` is structurally identical. Mirror all changes exactly.
|
||
|
||
---
|
||
|
||
#### State Transition Diagram
|
||
|
||
```
|
||
MOUNT
|
||
│
|
||
▼
|
||
[Check prevTier vs currentTier]
|
||
│
|
||
├─ Not free→pro ──→ HIDDEN (do nothing)
|
||
│
|
||
└─ free→pro ──→ [Check local/session storage]
|
||
│
|
||
├─ No data ──→ NO_DATA state ──→ auto-dismiss after 2s
|
||
│ │
|
||
│ ▼
|
||
│ dispatch 'import-dismissed'
|
||
│
|
||
└─ Has data ──→ DETECTING state
|
||
│
|
||
▼
|
||
[GET /api/dm/notes]
|
||
[GET /api/dm/initiative]
|
||
│
|
||
▼
|
||
REVIEW state
|
||
│
|
||
┌───────────────┼───────────────┐
|
||
▼ ▼ ▼
|
||
User clicks User clicks User clicks
|
||
"Import" "Skip" backdrop/Escape
|
||
│ │ │
|
||
▼ ▼ ▼
|
||
IMPORTING dispatch dispatch
|
||
│ 'import-dismissed' 'import-dismissed'
|
||
▼
|
||
[POST/PUT per step]
|
||
│
|
||
┌─────────┴─────────┐
|
||
▼ ▼
|
||
All success Any error
|
||
│ │
|
||
▼ ▼
|
||
COMPLETE state ERROR state
|
||
│ │
|
||
▼ ▼
|
||
dispatch 'import-complete' User clicks
|
||
"Retry" ──→ IMPORTING
|
||
"Skip" ───→ dispatch 'import-dismissed'
|
||
```
|
||
|
||
---
|
||
|
||
### Edge Cases & Decisions
|
||
|
||
| Edge Case | Decision |
|
||
|-----------|----------|
|
||
| User is FREE (no upgrade) | Modal never shows. `dm-tier` stays 'free' or undefined. |
|
||
| User upgrades but has no local data | Show "No data to import" for 2s, then auto-dismiss. |
|
||
| User upgrades, has data, but DB is empty | Skip conflict UI, go straight to IMPORTING. |
|
||
| User upgrades, has data, DB has blank note | Treat blank note (content null/empty/whitespace) as no conflict. Still replace if user chooses replace. |
|
||
| API fails mid-import | Transition to ERROR state. Retry re-attempts only failed steps. Already-successful steps are not re-run. |
|
||
| User refreshes page during IMPORTING | State is lost. On re-mount, `dm-tier` is now 'pro' (updated on modal open), so no re-trigger. Partial imports remain in DB. |
|
||
| User clicks backdrop or Escape | Same as "Skip": dispatch 'import-dismissed', hide modal. |
|
||
| `sessionStorage` lost (new tab) | `sessionStorage` is tab-scoped. If user upgrades in a new tab, `dm-initiative` may not be present. This is acceptable — we can only import what's in the current tab's sessionStorage. |
|
||
| Legacy `it-combatants` in sessionStorage | `InitiativeTracker.astro` auto-migrates `it-combatants` → `dm-initiative` on load. Since the modal runs after page load, `dm-initiative` will already contain the migrated data. No special handling needed. |
|
||
|
||
---
|
||
|
||
### Files to Create / Modify
|
||
|
||
| File | Action | Lines (est.) |
|
||
|------|--------|--------------|
|
||
| `src/components/dm/ImportModal.astro` | **Create** — full component | ~350 |
|
||
| `src/i18n/dm-translations.ts` | **Modify** — append ~20 new keys | +20 lines |
|
||
| `src/pages/dm/index.astro` | **Modify** — import + render `<ImportModal />` | +2 lines |
|
||
| `src/pages/ru/dm/index.astro` | **Modify** — mirror EN page changes | +2 lines |
|
||
|
||
---
|
||
|
||
### Verification Plan
|
||
|
||
1. **TypeScript**: `npx tsc --noEmit` — zero errors
|
||
2. **Unit tests**: `npx vitest run` — full suite passes
|
||
3. **Manual QA scenarios**:
|
||
- FREE user with notes + initiative → visits page → no modal
|
||
- FREE user → upgrades to PRO → visits page → modal appears with found items
|
||
- PRO user with no local data → modal shows "No data to import" → auto-dismisses
|
||
- PRO user with data, empty DB → clicks Import → progress shown → success → 'import-complete' dispatched
|
||
- PRO user with data, DB has data → conflict options shown → "Replace" → PUT updates existing → success
|
||
- PRO user with data, DB has data → "Keep both" → POST creates new → success
|
||
- PRO user with data, DB has data → "Skip" → no API calls → 'import-dismissed' dispatched
|
||
- Network failure during import → error shown → Retry → succeeds
|
||
- localStorage `dm-notes` and sessionStorage `dm-initiative` still present after import
|
||
4. **Accessibility**: Tab focus stays inside modal; Escape dismisses; aria-modal="true" present
|
||
|
||
---
|
||
|