mirror of
https://github.com/AmruthPillai/Reactive-Resume.git
synced 2026-08-23 06:42:15 +10:00
chore: update dependencies
This commit is contained in:
Amruth Pillai
@@ -0,0 +1,259 @@
|
||||
# Plan 001: Characterization tests for the resume service
|
||||
|
||||
> **Executor instructions**: Follow this plan step by step. Run every
|
||||
> verification command and confirm the expected result before moving to the
|
||||
> next step. If anything in the "STOP conditions" section occurs, stop and
|
||||
> report — do not improvise. When done, update the status row for this plan
|
||||
> in `plans/README.md`.
|
||||
>
|
||||
> **Drift check (run first)**: `git diff --stat 73daf22b2..HEAD -- packages/api/src/features/resume/service.ts`
|
||||
> If `service.ts` changed since this plan was written, compare the "Current
|
||||
> state" excerpts against the live code before proceeding; on a mismatch,
|
||||
> treat it as a STOP condition.
|
||||
|
||||
## Status
|
||||
|
||||
- **Priority**: P1
|
||||
- **Effort**: M
|
||||
- **Risk**: LOW
|
||||
- **Depends on**: none
|
||||
- **Category**: tests
|
||||
- **Planned at**: commit `73daf22b2`, 2026-07-08
|
||||
|
||||
## Why this matters
|
||||
|
||||
`packages/api/src/features/resume/service.ts` is 772 lines and owns every
|
||||
resume mutation — `create`, `update`, `patch` (JSON Patch application),
|
||||
`delete`, `duplicate`, `setLocked`, `setPassword`, `removePassword`,
|
||||
`verifyPassword`, `getBySlug`, plus version-history snapshotting. It has **no
|
||||
`service.test.ts`**. The only coverage is Playwright e2e, which exercises the
|
||||
happy path through the UI and cannot assert error codes, lock enforcement, or
|
||||
snapshot throttling in isolation. This service also has high churn (v5.2.0
|
||||
added undo/redo + version history). Characterization tests here (a) catch
|
||||
regressions in CRUD/patch/lock/password behavior before users do, and (b) are
|
||||
a prerequisite for Plan 003 (which changes not-found behavior in this file)
|
||||
and Plan 007 (template refactor) — you cannot safely refactor code that has no
|
||||
behavioral net under it.
|
||||
|
||||
The goal is **characterization tests**: capture what the code does today,
|
||||
locking in current behavior so later changes are deliberate, not accidental.
|
||||
|
||||
## Current state
|
||||
|
||||
- `packages/api/src/features/resume/service.ts` — the service under test.
|
||||
Exports a `resumeService` object literal whose methods each take an `input`
|
||||
object and call the mocked `db`. Key behaviors to pin down:
|
||||
- `update` (lines ~555–635): reads `isLocked`; throws `ORPCError("RESUME_LOCKED")`
|
||||
if locked; on a successful `UPDATE ... RETURNING`, if `!resume` throws
|
||||
`ORPCError("NOT_FOUND")`; maps a unique-constraint violation on
|
||||
`resume_slug_user_id_unique` to `ORPCError("RESUME_SLUG_ALREADY_EXISTS")`.
|
||||
- `setLocked` (lines ~663–679), `setPassword` (~681–699), `removePassword`
|
||||
(~726–742): each runs an `UPDATE ... RETURNING`, then `if (!resume) return;`
|
||||
(silent no-op when no row matches — **this is today's behavior; capture it
|
||||
as-is. Plan 003 will change it.**), else calls `notifyResumeUpdated`.
|
||||
- `delete` (lines ~744–769): transaction that throws `NOT_FOUND` when the row
|
||||
is missing and `RESUME_LOCKED` when locked, then deletes and cleans storage.
|
||||
- `statistics.increment` (lines ~199–237): two `INSERT ... ON CONFLICT DO
|
||||
UPDATE` writes inside a transaction.
|
||||
|
||||
- **Test convention to follow** — model the new test after the existing
|
||||
sibling `packages/api/src/features/applications/service.test.ts`. It mocks
|
||||
the DB layer with `vi.hoisted` + `vi.mock`, then dynamically imports the
|
||||
service. The exact shape to copy (from that file, lines 1–68):
|
||||
|
||||
```ts
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
const dbMock = vi.hoisted(() => ({
|
||||
select: vi.fn(), insert: vi.fn(), update: vi.fn(),
|
||||
delete: vi.fn(), transaction: vi.fn(),
|
||||
}));
|
||||
vi.mock("@reactive-resume/db/client", () => ({ db: dbMock }));
|
||||
vi.mock("@reactive-resume/db/schema", () => ({ /* stub the tables used */ }));
|
||||
vi.mock("drizzle-orm", () => ({
|
||||
and: (...a: unknown[]) => a, eq: (...a: unknown[]) => a,
|
||||
isNotNull: (...a: unknown[]) => a, sql: Object.assign(
|
||||
(s: TemplateStringsArray, ...v: unknown[]) => ({ s, v }),
|
||||
{ join: (v: unknown[]) => v },
|
||||
),
|
||||
}));
|
||||
|
||||
const { resumeService } = await import("./service");
|
||||
```
|
||||
|
||||
Note the `applications/service.test.ts` helpers `createSelectChain(rows)` and
|
||||
`setSelectResults(...)` — reuse that pattern to script what each `db.select`
|
||||
/ `db.update().returning()` call resolves to.
|
||||
|
||||
- **Mocks this service needs beyond the applications example** (grep the
|
||||
imports at the top of `service.ts` and mock each):
|
||||
- `bcrypt` (`hash`, `compare`) — used by `setPassword`/`verifyPassword`.
|
||||
(The real import specifier in `service.ts` is `bcrypt`, not `bcryptjs` —
|
||||
mock that exact module path.) Mock `hash` to return a fixed string and
|
||||
`compare` to return a boolean you control.
|
||||
- The snapshot/patch helpers imported into `service.ts` (e.g.
|
||||
`applyResumePatchTx`, `maybeSnapshotOnSave`, `writeResumeVersion`,
|
||||
`notifyResumeUpdated`, `getStorageService`) — mock them so the service's
|
||||
own branching is what's under test, not their internals. Read the top of
|
||||
`service.ts` to get the exact import specifiers and mock each module path
|
||||
the same way the applications test mocks `../storage/service`.
|
||||
|
||||
- **ORPCError assertions** — errors are `ORPCError` instances from
|
||||
`@orpc/server` with a `.code` (e.g. `"NOT_FOUND"`, `"RESUME_LOCKED"`). Assert
|
||||
with `await expect(fn()).rejects.toThrow()` and, where you can, check the
|
||||
code: `await fn().catch((e) => expect(e.code).toBe("RESUME_LOCKED"))`, or
|
||||
assert on `.message`. Confirm the real shape by reading how
|
||||
`access-policy.test.ts` or `access.test.ts` in the same folder assert on
|
||||
`ORPCError` and copy that style.
|
||||
|
||||
## Commands you will need
|
||||
|
||||
| Purpose | Command | Expected on success |
|
||||
|-----------|---------------------------------------------------------------------|---------------------|
|
||||
| Typecheck | `pnpm --filter @reactive-resume/api typecheck` | exit 0, no errors |
|
||||
| Run test | `pnpm --filter @reactive-resume/api test -- resume/service.test.ts` | all pass |
|
||||
| All api tests | `pnpm --filter @reactive-resume/api test` | all pass |
|
||||
|
||||
(These are the repo's real commands — package-scoped Vitest via Turborepo.
|
||||
Do NOT run `pnpm check`; it rewrites files.)
|
||||
|
||||
## Scope
|
||||
|
||||
**In scope** (the only files you should create/modify):
|
||||
- `packages/api/src/features/resume/service.test.ts` (create)
|
||||
- `plans/README.md` (status row only)
|
||||
|
||||
**Out of scope** (do NOT touch):
|
||||
- `packages/api/src/features/resume/service.ts` — this plan adds tests that
|
||||
characterize its *current* behavior. Do not "fix" anything you find here,
|
||||
even the silent `if (!resume) return;` no-ops in `setLocked`/`setPassword`/
|
||||
`removePassword` — those are Plan 003's job, and this test must assert the
|
||||
current silent-return behavior so Plan 003's change is visible as a test diff.
|
||||
- Any router file (`crud.ts`, `sharing.ts`) — router tests are a separate
|
||||
future effort.
|
||||
- The real database or migrations — this is a pure unit test with a mocked db.
|
||||
|
||||
## Git workflow
|
||||
|
||||
- Branch: `advisor/001-resume-service-tests`
|
||||
- Commit style: conventional commits (repo uses them — e.g.
|
||||
`test(api): add characterization tests for resume service`).
|
||||
- Do NOT push or open a PR unless the operator instructed it.
|
||||
|
||||
## Steps
|
||||
|
||||
### Step 1: Scaffold the test file and mocks
|
||||
|
||||
Create `packages/api/src/features/resume/service.test.ts`. Copy the mock
|
||||
scaffolding pattern from `applications/service.test.ts` (lines 1–68). Read the
|
||||
top-of-file imports in `service.ts` and add a `vi.mock(...)` for every module
|
||||
it imports that touches I/O (db, schema, drizzle-orm, bcryptjs, storage
|
||||
service, and the patch/snapshot/notify helpers). Stub `@reactive-resume/db/schema`
|
||||
with the table/column objects the service references (`resume`,
|
||||
`resumeStatistics`, `resumeStatisticsDaily`, `user`).
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api test -- resume/service.test.ts`
|
||||
→ the file is picked up (even with zero real tests it should not error on
|
||||
import; add one `it("imports", () => expect(resumeService).toBeDefined())` to
|
||||
confirm wiring). Expected: 1 passing test.
|
||||
|
||||
### Step 2: Characterize `update`
|
||||
|
||||
Add a `describe("update")` block with these cases, asserting **current**
|
||||
behavior:
|
||||
- Throws `RESUME_LOCKED` when the pre-read returns `{ isLocked: true }`.
|
||||
- Returns the updated row on success (script `db.update().set().where().returning()`
|
||||
to resolve `[{ id, name, slug, ... }]`).
|
||||
- Throws `NOT_FOUND` when the `RETURNING` resolves to `[]` (no row matched).
|
||||
- Maps a thrown error whose `cause.constraint === "resume_slug_user_id_unique"`
|
||||
to `RESUME_SLUG_ALREADY_EXISTS`.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api test -- resume/service.test.ts`
|
||||
→ all cases pass.
|
||||
|
||||
### Step 3: Characterize `setLocked`, `setPassword`, `removePassword`
|
||||
|
||||
Add a `describe` for each. For every method assert **both** paths:
|
||||
- Success path: `RETURNING` resolves to `[{ id, updatedAt }]` → the method
|
||||
resolves (returns `undefined`) and `notifyResumeUpdated` was called once with
|
||||
the expected `mutation` value (`"lock"` / `"password"`).
|
||||
- **Not-found path: `RETURNING` resolves to `[]` → the method resolves
|
||||
`undefined` and `notifyResumeUpdated` is NOT called** (this pins the current
|
||||
silent no-op; Plan 003 will flip this to throwing `NOT_FOUND`).
|
||||
- For `setPassword`, assert `hash` (mocked bcrypt) was called with the input
|
||||
password before the update.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api test -- resume/service.test.ts`
|
||||
→ all pass.
|
||||
|
||||
### Step 4: Characterize `verifyPassword` and `delete`
|
||||
|
||||
- `verifyPassword`: throws `INVALID_PASSWORD` when no matching row; throws
|
||||
`INVALID_PASSWORD` when `compare` (mocked) returns `false`; returns `true`
|
||||
and calls `grantResumeAccess` when `compare` returns `true`.
|
||||
- `delete`: script the transaction mock (see how `applications/service.test.ts`
|
||||
handles `db.transaction` — if it doesn't, make `dbMock.transaction` invoke
|
||||
its callback with a `tx` object exposing the same `select`/`delete` chain).
|
||||
Assert `NOT_FOUND` when the row is missing, `RESUME_LOCKED` when locked, and
|
||||
storage `delete` called for both screenshot and pdf keys on success.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api test -- resume/service.test.ts`
|
||||
→ all pass.
|
||||
|
||||
### Step 5: Characterize `statistics.increment`
|
||||
|
||||
Assert that a `views: true` call runs `db.transaction`, and inside it inserts
|
||||
into both `resumeStatistics` and `resumeStatisticsDaily` with an
|
||||
`onConflictDoUpdate`. You do not need to assert SQL text — assert that both
|
||||
`tx.insert(...)` calls happen (spy on the tx insert). This case is the safety
|
||||
net for Plan 005, which changes when `increment` is *called* (not its body).
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api test` → the whole api package
|
||||
test suite passes (no regressions in sibling tests from your new mocks).
|
||||
|
||||
## Test plan
|
||||
|
||||
- New file: `packages/api/src/features/resume/service.test.ts`, structured as
|
||||
one `describe` per method, following `applications/service.test.ts` as the
|
||||
structural pattern.
|
||||
- Cases per method are listed in Steps 2–5 (happy path + each error/edge branch
|
||||
the code contains today).
|
||||
- Verification: `pnpm --filter @reactive-resume/api test` → all pass, including
|
||||
the new tests. Count the new tests in the output; expect ≥ 14 new cases.
|
||||
|
||||
## Done criteria
|
||||
|
||||
Machine-checkable. ALL must hold:
|
||||
|
||||
- [ ] `packages/api/src/features/resume/service.test.ts` exists
|
||||
- [ ] `pnpm --filter @reactive-resume/api test -- resume/service.test.ts` exits 0 with ≥ 14 passing cases
|
||||
- [ ] `pnpm --filter @reactive-resume/api test` exits 0 (no sibling regressions)
|
||||
- [ ] `pnpm --filter @reactive-resume/api typecheck` exits 0
|
||||
- [ ] `git status --porcelain` shows only `service.test.ts` (new) and `plans/README.md` modified
|
||||
- [ ] `plans/README.md` status row for 001 updated to DONE
|
||||
|
||||
## STOP conditions
|
||||
|
||||
Stop and report back (do not improvise) if:
|
||||
|
||||
- `service.ts` has drifted from the excerpts above (method line ranges or error
|
||||
codes differ materially) — the codebase changed since this plan was written.
|
||||
- The mocking approach fights the service: e.g. the service imports something
|
||||
that runs real I/O at module load and can't be cleanly mocked. Report what
|
||||
and where; do not weaken the test into a no-op.
|
||||
- You find a genuine bug while characterizing (behavior that looks wrong). Do
|
||||
NOT fix it here — write the test to capture current behavior, add a
|
||||
`// NOTE: characterizes current behavior; see finding` comment, and report it.
|
||||
|
||||
## Maintenance notes
|
||||
|
||||
- Plan 003 changes `setLocked`/`setPassword`/`removePassword` to throw
|
||||
`NOT_FOUND` instead of silently returning. When that lands, the "not-found
|
||||
path" assertions from Step 3 must be updated in the same PR — that test diff
|
||||
is the intended signal that behavior changed on purpose.
|
||||
- Plan 005 changes the *caller* of `statistics.increment` (dedup), not its body,
|
||||
so Step 5's test should keep passing; if it breaks, 005 changed more than
|
||||
intended.
|
||||
- A reviewer should check the mocks assert real branching, not tautologies
|
||||
(e.g. that `NOT_FOUND` comes from an empty `RETURNING`, not from a mock that
|
||||
always throws).
|
||||
@@ -0,0 +1,233 @@
|
||||
# Plan 002: Add CSP + framing headers to web pages; gate the uploads CORS header
|
||||
|
||||
> **Executor instructions**: Follow this plan step by step. Run every
|
||||
> verification command and confirm the expected result before moving on. If
|
||||
> anything in "STOP conditions" occurs, stop and report. When done, update the
|
||||
> status row for this plan in `plans/README.md`.
|
||||
>
|
||||
> **Drift check (run first)**:
|
||||
> `git diff --stat 73daf22b2..HEAD -- apps/server/src/static/web.ts apps/server/src/static/uploads.ts`
|
||||
> If either file changed since this plan was written, compare the "Current
|
||||
> state" excerpts against the live code before proceeding; on mismatch, STOP.
|
||||
|
||||
## Status
|
||||
|
||||
- **Priority**: P1
|
||||
- **Effort**: S
|
||||
- **Risk**: LOW
|
||||
- **Depends on**: none
|
||||
- **Category**: security
|
||||
- **Planned at**: commit `73daf22b2`, 2026-07-08
|
||||
|
||||
## Why this matters
|
||||
|
||||
The Hono server serves every HTML page (public resumes, the auth/dashboard/
|
||||
builder shells) through `handleWebApp` in `apps/server/src/static/web.ts`, and
|
||||
that response sets only `Content-Type` and (sometimes) `X-Robots-Tag`. There is
|
||||
**no `Content-Security-Policy` and no `X-Frame-Options`**. Consequences:
|
||||
- Any page can be framed by an attacker's site → clickjacking against
|
||||
authenticated actions and public resume views.
|
||||
- No script-source restriction → a single injected-content bug becomes a much
|
||||
larger XSS blast radius than it needs to be.
|
||||
|
||||
Separately, the file-serving endpoint in `apps/server/src/static/uploads.ts`
|
||||
sets `Access-Control-Allow-Origin: env.APP_URL` **unconditionally** on a GET
|
||||
endpoint whose only real consumers are same-origin. `Cross-Origin-Resource-Policy:
|
||||
same-site` is already set, so the ACAO header adds cross-origin exposure for no
|
||||
functional benefit.
|
||||
|
||||
This plan adds the missing headers to web responses and removes the
|
||||
unnecessary ACAO header. It is deliberately conservative: **CSP ships in
|
||||
report-only mode first** so it cannot break the app on rollout.
|
||||
|
||||
## Current state
|
||||
|
||||
- `apps/server/src/static/web.ts:55-65` — `getFallbackResponseHeaders` returns
|
||||
a plain object of headers per path (or `null` for a 404):
|
||||
|
||||
```ts
|
||||
function getFallbackResponseHeaders(pathname: string) {
|
||||
if (pathname === "/") return { "Content-Type": "text/html; charset=UTF-8" };
|
||||
if (isNoindexShellPath(pathname) || isPublicResumePath(pathname)) {
|
||||
return {
|
||||
"Content-Type": "text/html; charset=UTF-8",
|
||||
"X-Robots-Tag": "noindex, follow",
|
||||
};
|
||||
}
|
||||
return null;
|
||||
}
|
||||
```
|
||||
|
||||
These objects are spread into the `Response` at `web.ts:86-92` (both the HEAD
|
||||
`new Response(null, { status: 200, headers })` path and the GET
|
||||
`new Response(html, { headers })` path).
|
||||
|
||||
- `apps/server/src/static/uploads.ts:28-46` — the file response already sets a
|
||||
strong header set as a model to follow, and ends with the ACAO line to remove:
|
||||
|
||||
```ts
|
||||
headers.set("Cache-Control", "public, max-age=31536000, immutable");
|
||||
headers.set("ETag", etag);
|
||||
headers.set("X-Content-Type-Options", "nosniff");
|
||||
headers.set("X-Robots-Tag", "noindex, nofollow");
|
||||
headers.set("Cross-Origin-Resource-Policy", "same-site");
|
||||
headers.set("Referrer-Policy", "strict-origin-when-cross-origin");
|
||||
headers.set("X-Frame-Options", "DENY");
|
||||
headers.set("X-Download-Options", "noopen");
|
||||
headers.set("Access-Control-Allow-Origin", env.APP_URL); // <-- remove this line
|
||||
```
|
||||
|
||||
- **Existing test files that assert headers** (follow their style, they are
|
||||
your regression net):
|
||||
- `apps/server/src/static/web.test.ts`
|
||||
- `apps/server/src/static/uploads.test.ts`
|
||||
|
||||
- **Design constraint** — the app has no known legitimate need to be iframed,
|
||||
and the builder preview is a same-origin pdf.js canvas (not a cross-origin
|
||||
frame). So `X-Frame-Options: DENY` is safe. The app does load web fonts and
|
||||
images from same origin and inline styles/scripts from the Vite bundle, which
|
||||
is why CSP starts **report-only**: do not enforce a policy you have not
|
||||
observed the app satisfy.
|
||||
|
||||
## Commands you will need
|
||||
|
||||
| Purpose | Command | Expected on success |
|
||||
|-----------|----------------------------------------|---------------------|
|
||||
| Typecheck | `pnpm --filter server typecheck` | exit 0 |
|
||||
| Test | `pnpm --filter server test -- static` | all pass |
|
||||
|
||||
(The server package is named `server` in `apps/server/package.json`, not
|
||||
`@reactive-resume/server`.)
|
||||
|
||||
(Do NOT run `pnpm check`.)
|
||||
|
||||
## Scope
|
||||
|
||||
**In scope**:
|
||||
- `apps/server/src/static/web.ts`
|
||||
- `apps/server/src/static/web.test.ts`
|
||||
- `apps/server/src/static/uploads.ts`
|
||||
- `apps/server/src/static/uploads.test.ts`
|
||||
- `plans/README.md` (status row)
|
||||
|
||||
**Out of scope** (do NOT touch):
|
||||
- The oRPC / auth / MCP / OpenAPI handlers — they return API responses, not
|
||||
HTML pages; header policy for those is a separate concern.
|
||||
- Enforcing (non-report-only) CSP — an enforced policy requires collecting
|
||||
violation reports first; that is explicit follow-up, not this plan.
|
||||
- Any web-app (`apps/web`) source — headers are set at the server layer.
|
||||
|
||||
## Git workflow
|
||||
|
||||
- Branch: `advisor/002-security-headers`
|
||||
- Commit style: conventional commits, e.g.
|
||||
`feat(server): add CSP report-only and framing headers to web responses`.
|
||||
- Do NOT push or open a PR unless instructed.
|
||||
|
||||
## Steps
|
||||
|
||||
### Step 1: Add framing + hardening headers to web responses
|
||||
|
||||
In `apps/server/src/static/web.ts`, extend the header objects returned by
|
||||
`getFallbackResponseHeaders` so that every non-null branch (the `/` branch and
|
||||
the noindex/public branch) also includes:
|
||||
|
||||
```
|
||||
"X-Frame-Options": "DENY",
|
||||
"X-Content-Type-Options": "nosniff",
|
||||
"Referrer-Policy": "strict-origin-when-cross-origin",
|
||||
```
|
||||
|
||||
Keep the existing `Content-Type` and `X-Robots-Tag` values unchanged. Prefer
|
||||
adding a small shared constant (e.g. `const BASE_SECURITY_HEADERS = { ... }`)
|
||||
and spreading it into both branches, so the two paths cannot drift.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/server typecheck` → exit 0.
|
||||
|
||||
### Step 2: Add a report-only CSP header to web responses
|
||||
|
||||
Add to the same shared header set:
|
||||
|
||||
```
|
||||
"Content-Security-Policy-Report-Only":
|
||||
"default-src 'self'; img-src 'self' data: blob:; font-src 'self' data:; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline'; connect-src 'self'; frame-ancestors 'none'; base-uri 'self'; object-src 'none'",
|
||||
```
|
||||
|
||||
Use `Report-Only` (not the enforcing header) so nothing breaks on rollout.
|
||||
`'unsafe-inline'` for style/script is intentional for the first pass — the Vite
|
||||
bundle and inline theme script need it; tightening to nonces is future work.
|
||||
`frame-ancestors 'none'` is the CSP-level equivalent of `X-Frame-Options: DENY`.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/server typecheck` → exit 0.
|
||||
|
||||
### Step 3: Remove the unconditional CORS header from uploads
|
||||
|
||||
In `apps/server/src/static/uploads.ts`, delete the line:
|
||||
|
||||
```ts
|
||||
headers.set("Access-Control-Allow-Origin", env.APP_URL);
|
||||
```
|
||||
|
||||
Leave every other header untouched. If, after removal, `env` is no longer
|
||||
referenced anywhere in the file, remove its now-unused import (check with the
|
||||
grep in Done criteria); if `env` is still used, keep the import.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/server typecheck` → exit 0
|
||||
(no unused-import or undefined-symbol errors).
|
||||
|
||||
### Step 4: Update/extend tests
|
||||
|
||||
- In `web.test.ts`: add assertions that a GET to `/` and to a public resume
|
||||
path returns `X-Frame-Options: DENY` and a `Content-Security-Policy-Report-Only`
|
||||
header. Follow the existing test's request/response style in that file.
|
||||
- In `uploads.test.ts`: if an existing test asserts the presence of
|
||||
`Access-Control-Allow-Origin`, change it to assert the header is **absent**
|
||||
(`response.headers.get("Access-Control-Allow-Origin")` is `null`). Keep the
|
||||
assertions for `Cross-Origin-Resource-Policy` and the others.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/server test -- static` → all pass.
|
||||
|
||||
## Test plan
|
||||
|
||||
- Extend `web.test.ts` with two cases: `/` and a public-resume path each carry
|
||||
the new framing + CSP-report-only headers.
|
||||
- Update `uploads.test.ts` so the ACAO header is asserted absent (and the other
|
||||
security headers still present).
|
||||
- Verification: `pnpm --filter @reactive-resume/server test -- static` → all
|
||||
pass, including the new/updated assertions.
|
||||
|
||||
## Done criteria
|
||||
|
||||
Machine-checkable. ALL must hold:
|
||||
|
||||
- [ ] `pnpm --filter @reactive-resume/server typecheck` exits 0
|
||||
- [ ] `pnpm --filter @reactive-resume/server test -- static` exits 0
|
||||
- [ ] `grep -n "X-Frame-Options" apps/server/src/static/web.ts` returns a match
|
||||
- [ ] `grep -n "Content-Security-Policy-Report-Only" apps/server/src/static/web.ts` returns a match
|
||||
- [ ] `grep -n "Access-Control-Allow-Origin" apps/server/src/static/uploads.ts` returns **no** matches
|
||||
- [ ] `git status --porcelain` lists only the four in-scope source/test files and `plans/README.md`
|
||||
- [ ] `plans/README.md` status row for 002 updated
|
||||
|
||||
## STOP conditions
|
||||
|
||||
Stop and report back if:
|
||||
|
||||
- `web.ts`/`uploads.ts` have drifted from the excerpts above.
|
||||
- Removing the ACAO header breaks an existing test that documents a *legitimate*
|
||||
cross-origin consumer of `/uploads/*` you were unaware of (read the test's
|
||||
intent before assuming it's stale) — report it instead of forcing the change.
|
||||
- You are tempted to ship an **enforcing** CSP (not report-only) — that is out
|
||||
of scope and can break the app; stop and confirm.
|
||||
|
||||
## Maintenance notes
|
||||
|
||||
- The CSP is report-only. Follow-up (separate plan): wire a report endpoint or
|
||||
read browser console CSP reports, confirm the app fully satisfies the policy,
|
||||
then promote `Content-Security-Policy-Report-Only` → `Content-Security-Policy`
|
||||
and drop `'unsafe-inline'` in favor of nonces where feasible.
|
||||
- If a future feature legitimately needs the app embeddable (e.g. an official
|
||||
embed widget), `frame-ancestors`/`X-Frame-Options` must be relaxed for that
|
||||
route only, not globally.
|
||||
- Reviewer should confirm the header set is shared between both `web.ts`
|
||||
branches (no drift) and that no API/JSON responses accidentally inherit the
|
||||
HTML CSP.
|
||||
@@ -0,0 +1,238 @@
|
||||
# Plan 003: Fix silent-success mutations and bound bulk-operation inputs
|
||||
|
||||
> **Executor instructions**: Follow step by step. Run every verification
|
||||
> command and confirm the expected result before moving on. Honor "STOP
|
||||
> conditions". When done, update the status row in `plans/README.md`.
|
||||
>
|
||||
> **Drift check (run first)**:
|
||||
> `git diff --stat 73daf22b2..HEAD -- packages/api/src/features/resume/service.ts packages/api/src/dto/application.ts packages/api/src/features/applications/service.ts`
|
||||
> If any changed since this plan was written, compare "Current state" excerpts
|
||||
> against live code; on mismatch, STOP.
|
||||
|
||||
## Status
|
||||
|
||||
- **Priority**: P1
|
||||
- **Effort**: S
|
||||
- **Risk**: LOW
|
||||
- **Depends on**: 001 (its Step 3 tests characterize the current silent-return
|
||||
behavior this plan changes — update them here). Not a hard blocker, but if
|
||||
001 is DONE you must update its assertions in the same PR.
|
||||
- **Category**: bug
|
||||
- **Planned at**: commit `73daf22b2`, 2026-07-08
|
||||
|
||||
## Why this matters
|
||||
|
||||
Two small, independent correctness/robustness fixes on the API layer:
|
||||
|
||||
1. **Silent-success mutations.** `setLocked`, `setPassword`, and
|
||||
`removePassword` in `packages/api/src/features/resume/service.ts` run an
|
||||
`UPDATE ... WHERE id = ? AND userId = ? RETURNING`, and when no row matches
|
||||
they do `if (!resume) return;` — returning HTTP 200 success. Every other
|
||||
mutation in this file (`update`, `delete`) throws `ORPCError("NOT_FOUND")` in
|
||||
the same situation. The inconsistency means a client calling these on a
|
||||
resume that doesn't exist (or isn't theirs) is told it succeeded, so the UI
|
||||
updates its state as if the lock/password change took effect. Ownership is
|
||||
still enforced (the `WHERE` includes `userId`), so this is not an auth
|
||||
bypass — it's a misleading false-success that hides not-found/not-owned.
|
||||
|
||||
2. **Unbounded bulk inputs.** `bulkUpdate` and `bulkDelete` DTOs in
|
||||
`packages/api/src/dto/application.ts` accept `ids: z.array(z.string()).min(1)`
|
||||
with **no `.max()`**. `bulkDelete`'s service loads all target rows into
|
||||
memory before deleting. A single API call with a very large `ids` array is
|
||||
unbounded memory/DB work. A `.max()` cap is a one-line defensive fix.
|
||||
|
||||
## Current state
|
||||
|
||||
### Part A — silent-success (resume/service.ts)
|
||||
|
||||
`setLocked` (~663–679), `setPassword` (~681–699), `removePassword` (~726–742)
|
||||
each look like this (setLocked shown; the other two differ only in the `.set`
|
||||
and the `mutation` label):
|
||||
|
||||
```ts
|
||||
setLocked: async (input: { id: string; userId: string; isLocked: boolean }) => {
|
||||
const [resume] = await db
|
||||
.update(schema.resume)
|
||||
.set({ isLocked: input.isLocked })
|
||||
.where(and(eq(schema.resume.id, input.id), eq(schema.resume.userId, input.userId)))
|
||||
.returning({ id: schema.resume.id, updatedAt: schema.resume.updatedAt });
|
||||
|
||||
if (!resume) return; // <-- silent no-op; should throw NOT_FOUND
|
||||
|
||||
await notifyResumeUpdated({ /* ... mutation: "lock" ... */ });
|
||||
},
|
||||
```
|
||||
|
||||
The reference behavior to match is `update` (~603) and `delete` (~751):
|
||||
`if (!resume) throw new ORPCError("NOT_FOUND");`. `ORPCError` is already imported
|
||||
at the top of `service.ts` (used throughout).
|
||||
|
||||
The routers that call these (for context; **do not change them**):
|
||||
- `crud.ts:187` `setLocked` — `protectedProcedure`, does not currently declare a
|
||||
`NOT_FOUND` error in `.errors({...})`.
|
||||
- `sharing.ts:29` `setPassword`, `sharing.ts:80` `removePassword` — likewise.
|
||||
|
||||
oRPC will surface a thrown `ORPCError("NOT_FOUND")` as a 404 regardless of
|
||||
whether it's declared in `.errors()`, matching how `update`/`delete` already
|
||||
behave (they throw the same without special router declarations). So no router
|
||||
change is required.
|
||||
|
||||
### Part B — unbounded bulk inputs (dto/application.ts)
|
||||
|
||||
`packages/api/src/dto/application.ts:163-176`:
|
||||
|
||||
```ts
|
||||
bulkUpdate: {
|
||||
input: z.object({
|
||||
ids: z.array(z.string()).min(1),
|
||||
status: applicationStatusSchema.optional(),
|
||||
archived: z.boolean().optional(),
|
||||
addTags: z.array(z.string()).optional(),
|
||||
}),
|
||||
output: z.object({ updated: z.number() }),
|
||||
},
|
||||
|
||||
bulkDelete: {
|
||||
input: z.object({ ids: z.array(z.string()).min(1) }),
|
||||
output: z.object({ deleted: z.number() }),
|
||||
},
|
||||
```
|
||||
|
||||
The consuming service is `packages/api/src/features/applications/service.ts`
|
||||
(`bulkUpdate` ~340–381, `bulkDelete` ~383–396; `bulkDelete` fetches all target
|
||||
rows into memory before deleting).
|
||||
|
||||
## Commands you will need
|
||||
|
||||
| Purpose | Command | Expected |
|
||||
|-----------|-------------------------------------------------------------------------|--------------------|
|
||||
| Typecheck | `pnpm --filter @reactive-resume/api typecheck` | exit 0 |
|
||||
| Test A | `pnpm --filter @reactive-resume/api test -- resume/service.test.ts` | all pass |
|
||||
| Test B | `pnpm --filter @reactive-resume/api test -- applications` | all pass |
|
||||
| All api | `pnpm --filter @reactive-resume/api test` | all pass |
|
||||
|
||||
(Do NOT run `pnpm check`.)
|
||||
|
||||
## Scope
|
||||
|
||||
**In scope**:
|
||||
- `packages/api/src/features/resume/service.ts` (Part A)
|
||||
- `packages/api/src/features/resume/service.test.ts` (update if 001 is DONE; else
|
||||
the tests may not exist yet — see Step 3)
|
||||
- `packages/api/src/dto/application.ts` (Part B)
|
||||
- `packages/api/src/dto/application.test.ts` if one exists, else add a focused
|
||||
test near the applications service tests (Step 4)
|
||||
- `plans/README.md` (status row)
|
||||
|
||||
**Out of scope**:
|
||||
- `crud.ts` / `sharing.ts` routers — no change needed (see Current state).
|
||||
- The applications service body — a `.max()` on input is sufficient; do not
|
||||
also rewrite the in-memory `bulkDelete` scan in this plan (that's a separate
|
||||
perf concern tracked elsewhere).
|
||||
- Any other service method.
|
||||
|
||||
## Git workflow
|
||||
|
||||
- Branch: `advisor/003-api-quick-fixes`
|
||||
- Commit style: conventional commits, e.g.
|
||||
`fix(api): throw NOT_FOUND on lock/password mutations for missing resume` and
|
||||
`fix(api): cap bulk application operation id arrays`.
|
||||
- Do NOT push or open a PR unless instructed.
|
||||
|
||||
## Steps
|
||||
|
||||
### Step 1: Throw NOT_FOUND in the three mutations
|
||||
|
||||
In `resume/service.ts`, change `if (!resume) return;` to
|
||||
`if (!resume) throw new ORPCError("NOT_FOUND");` in **all three** of `setLocked`,
|
||||
`setPassword`, `removePassword`. Match the exact form used by `update`/`delete`
|
||||
in the same file.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api typecheck` → exit 0.
|
||||
|
||||
### Step 2: Cap the bulk id arrays
|
||||
|
||||
In `dto/application.ts`, add `.max(200, "Too many items in a single bulk
|
||||
operation")` to the `ids` array in **both** `bulkUpdate` and `bulkDelete`
|
||||
inputs (i.e. `z.array(z.string()).min(1).max(200)`). 200 comfortably exceeds
|
||||
the UI's page size (~25) while bounding abuse; if a comment nearby documents a
|
||||
different intended cap, use that number and note it.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api typecheck` → exit 0.
|
||||
|
||||
### Step 3: Update / add tests for Part A
|
||||
|
||||
- If Plan 001 is DONE (`resume/service.test.ts` exists): update its `setLocked`
|
||||
/ `setPassword` / `removePassword` "not-found path" cases to now assert the
|
||||
method **rejects with `NOT_FOUND`** (previously they asserted a silent
|
||||
resolve). This test diff is the intended proof the behavior changed.
|
||||
- If Plan 001 is NOT done (no test file): add a minimal
|
||||
`resume/service.test.ts` covering just these three methods' not-found →
|
||||
`NOT_FOUND` behavior and success path, using
|
||||
`packages/api/src/features/applications/service.test.ts` as the mock pattern.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api test -- resume/service.test.ts`
|
||||
→ all pass; the three not-found cases assert `NOT_FOUND`.
|
||||
|
||||
### Step 4: Test for Part B
|
||||
|
||||
Add a small test that `bulkUpdate`/`bulkDelete` input schemas reject an `ids`
|
||||
array longer than the cap and accept one at the cap. If
|
||||
`packages/api/src/dto/application.test.ts` exists, add there; otherwise create
|
||||
it. Example shape:
|
||||
|
||||
```ts
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { applicationDto } from "./application"; // confirm the real export name
|
||||
|
||||
it("rejects oversized bulk id arrays", () => {
|
||||
const ids = Array.from({ length: 201 }, (_, i) => String(i));
|
||||
expect(applicationDto.bulkDelete.input.safeParse({ ids }).success).toBe(false);
|
||||
});
|
||||
```
|
||||
|
||||
Confirm the actual export name/shape by reading the top and bottom of
|
||||
`dto/application.ts` before writing the import.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api test -- applications` and
|
||||
`pnpm --filter @reactive-resume/api test` → all pass.
|
||||
|
||||
## Test plan
|
||||
|
||||
- Part A: three methods now reject with `NOT_FOUND` on no-match (updated or new
|
||||
cases in `resume/service.test.ts`).
|
||||
- Part B: bulk input schemas reject arrays over the cap, accept at the cap.
|
||||
- Verification: `pnpm --filter @reactive-resume/api test` → all pass.
|
||||
|
||||
## Done criteria
|
||||
|
||||
Machine-checkable. ALL must hold:
|
||||
|
||||
- [ ] `grep -n "if (!resume) return;" packages/api/src/features/resume/service.ts` returns **no** matches
|
||||
- [ ] `grep -c "throw new ORPCError(\"NOT_FOUND\")" packages/api/src/features/resume/service.ts` is ≥ 5 (update, delete, + the 3 new)
|
||||
- [ ] `grep -n ".max(" packages/api/src/dto/application.ts` shows the cap on both bulk inputs
|
||||
- [ ] `pnpm --filter @reactive-resume/api typecheck` exits 0
|
||||
- [ ] `pnpm --filter @reactive-resume/api test` exits 0
|
||||
- [ ] `git status --porcelain` lists only in-scope files + `plans/README.md`
|
||||
- [ ] `plans/README.md` status row for 003 updated
|
||||
|
||||
## STOP conditions
|
||||
|
||||
Stop and report if:
|
||||
|
||||
- The three methods no longer contain `if (!resume) return;` (already fixed by
|
||||
someone else, or drifted) — reconcile against live code before editing.
|
||||
- Throwing `NOT_FOUND` breaks an e2e or unit test that *depended on* the silent
|
||||
success (a client that fires these against unknown ids and expects 200) —
|
||||
report the caller; do not revert the fix without confirming intent.
|
||||
- The applications DTO export name differs from `applicationDto` — read the file
|
||||
and use the real name; do not guess.
|
||||
|
||||
## Maintenance notes
|
||||
|
||||
- The `.max(200)` cap is an input guard, not a fix for `bulkDelete`'s O(n)
|
||||
in-memory scan — that remains a known, separate perf item. If bulk operations
|
||||
ever need to exceed the cap, revisit both the cap and the scan together.
|
||||
- If the routers later add explicit `.errors({ NOT_FOUND: ... })` declarations
|
||||
for these procedures (for nicer OpenAPI docs), that's compatible with this
|
||||
change but not required by it.
|
||||
@@ -0,0 +1,116 @@
|
||||
# Plan 004: Correct the README "Custom CSS" feature claim
|
||||
|
||||
> **Executor instructions**: Follow step by step. Confirm each verification.
|
||||
> Honor "STOP conditions". When done, update the status row in `plans/README.md`.
|
||||
>
|
||||
> **Drift check (run first)**: `git diff --stat 73daf22b2..HEAD -- README.md`
|
||||
> If `README.md` changed, re-locate the line described below before editing.
|
||||
|
||||
## Status
|
||||
|
||||
- **Priority**: P3
|
||||
- **Effort**: S
|
||||
- **Risk**: LOW
|
||||
- **Depends on**: none
|
||||
- **Category**: docs
|
||||
- **Planned at**: commit `73daf22b2`, 2026-07-08
|
||||
|
||||
## Why this matters
|
||||
|
||||
The README advertises a feature that no longer exists. Raw custom CSS was
|
||||
removed and replaced by a structured Style Rules system — a decision recorded
|
||||
in `docs/adr/0002-structured-style-rules-for-react-pdf.md` (React PDF accepts
|
||||
style objects, not arbitrary browser selectors, so user CSS would be a
|
||||
misleading contract). A user reading the README expects a CSS code editor and
|
||||
instead finds structured form controls. Fixing the line keeps the public
|
||||
feature list truthful and consistent with the ADR.
|
||||
|
||||
## Current state
|
||||
|
||||
- `README.md:63` — under the **Templates** feature list:
|
||||
|
||||
```
|
||||
- Custom CSS for advanced styling
|
||||
```
|
||||
|
||||
- `docs/adr/0002-structured-style-rules-for-react-pdf.md` — states raw CSS is
|
||||
intentionally out; appearance customization is modeled as structured Style
|
||||
Rules targeting semantic section/rich-text slots.
|
||||
|
||||
- The actual UI lives at
|
||||
`apps/web/src/routes/builder/$resumeId/-sidebar/right/sections/custom-styles.tsx`
|
||||
(structured controls: color pickers, number inputs, dropdowns — not a CSS
|
||||
editor). You do not need to modify it; it is cited only to confirm the fix
|
||||
wording matches reality.
|
||||
|
||||
## Commands you will need
|
||||
|
||||
| Purpose | Command | Expected |
|
||||
|----------------|--------------------------------------|---------------------|
|
||||
| Locate line | `grep -n "Custom CSS" README.md` | one match (~line 63)|
|
||||
| Docs lint (opt)| `pnpm lint:docs` | exit 0 |
|
||||
|
||||
(Do NOT run `pnpm check` — it rewrites files. This is a one-line docs edit.)
|
||||
|
||||
## Scope
|
||||
|
||||
**In scope**:
|
||||
- `README.md` (the single feature-list line)
|
||||
- `plans/README.md` (status row)
|
||||
|
||||
**Out of scope**:
|
||||
- The Style Rules implementation or ADR.
|
||||
- Any other README line, unless the drift check shows the feature list moved.
|
||||
- The Mintlify docs under `docs/` — a separate concern.
|
||||
|
||||
## Git workflow
|
||||
|
||||
- Branch: `advisor/004-readme-custom-css`
|
||||
- Commit: `docs: correct README custom-CSS claim to Style Rules`
|
||||
- Do NOT push or open a PR unless instructed.
|
||||
|
||||
## Steps
|
||||
|
||||
### Step 1: Replace the line
|
||||
|
||||
Change `README.md:63` from:
|
||||
|
||||
```
|
||||
- Custom CSS for advanced styling
|
||||
```
|
||||
|
||||
to:
|
||||
|
||||
```
|
||||
- Structured Style Rules for section and text styling
|
||||
```
|
||||
|
||||
Match the surrounding list's capitalization and punctuation (no trailing
|
||||
period — the sibling bullets have none).
|
||||
|
||||
**Verify**: `grep -n "Custom CSS" README.md` → **no** matches;
|
||||
`grep -n "Structured Style Rules" README.md` → one match.
|
||||
|
||||
## Test plan
|
||||
|
||||
No code tests. Optional: `pnpm lint:docs` → exit 0 (markdownlint clean).
|
||||
|
||||
## Done criteria
|
||||
|
||||
- [ ] `grep -n "Custom CSS" README.md` returns no matches
|
||||
- [ ] `grep -n "Style Rules" README.md` returns the new line
|
||||
- [ ] `git status --porcelain` lists only `README.md` and `plans/README.md`
|
||||
- [ ] `plans/README.md` status row for 004 updated
|
||||
|
||||
## STOP conditions
|
||||
|
||||
Stop and report if:
|
||||
|
||||
- The "Custom CSS" line is already gone or already reworded (someone fixed it).
|
||||
- The drift check shows the Templates feature list was substantially
|
||||
restructured — re-locate the correct line before editing.
|
||||
|
||||
## Maintenance notes
|
||||
|
||||
- If Style Rules ever gains a raw-CSS escape hatch (ADR 0002 lists it as
|
||||
possible future work), revisit this line.
|
||||
@@ -0,0 +1,224 @@
|
||||
# Plan 005: Deduplicate public-resume view-count writes
|
||||
|
||||
> **Executor instructions**: Follow step by step. Run every verification and
|
||||
> confirm before moving on. Honor "STOP conditions". When done, update the
|
||||
> status row in `plans/README.md`.
|
||||
>
|
||||
> **Drift check (run first)**:
|
||||
> `git diff --stat 73daf22b2..HEAD -- packages/api/src/features/resume/service.ts packages/api/src/features/resume/access-policy.ts`
|
||||
> If either changed since this plan, compare "Current state" excerpts against
|
||||
> live code; on mismatch, STOP.
|
||||
|
||||
## Status
|
||||
|
||||
- **Priority**: P2
|
||||
- **Effort**: M
|
||||
- **Risk**: MED
|
||||
- **Depends on**: 001 recommended (its Step 5 test characterizes
|
||||
`statistics.increment`, giving you a safety net that this plan changes the
|
||||
*caller*, not the write body).
|
||||
- **Category**: perf
|
||||
- **Planned at**: commit `73daf22b2`, 2026-07-08
|
||||
|
||||
## Why this matters
|
||||
|
||||
Every public-resume view triggers a database write. `getBySlug`
|
||||
(`packages/api/src/features/resume/service.ts:475`) calls
|
||||
`statistics.increment(...)` whenever `shouldCountForStatistics` is true, and
|
||||
`increment` (lines 199–237) runs a transaction with **two** `INSERT ... ON
|
||||
CONFLICT DO UPDATE` statements (`resumeStatistics` + `resumeStatisticsDaily`).
|
||||
This fires on every non-owner load — bots, crawlers, and refreshes included.
|
||||
TanStack Query's 60s `staleTime` only suppresses within-session refetches; it
|
||||
does nothing for distinct sessions or server-side crawlers. A popular public
|
||||
resume therefore drives continuous write traffic to two tables for what is
|
||||
functionally the same view counted many times.
|
||||
|
||||
This plan adds a short-window per-viewer dedup so a burst of views from the
|
||||
same client within a window counts once, cutting write volume without changing
|
||||
what the counters mean to users (roughly "unique-ish views per window").
|
||||
|
||||
## Current state
|
||||
|
||||
- `packages/api/src/features/resume/service.ts:475` — `getBySlug` receives
|
||||
`input.requestHeaders: Headers`, resolves the resume, and at lines 505–507:
|
||||
|
||||
```ts
|
||||
if (shouldCountForStatistics(resume, viewer)) {
|
||||
await resumeService.statistics.increment({ id: resume.id, views: true });
|
||||
}
|
||||
```
|
||||
|
||||
- `packages/api/src/features/resume/access-policy.ts:76` —
|
||||
`shouldCountForStatistics(resume, viewer)` decides *whether* a view counts
|
||||
(e.g. skip the owner). This plan adds an orthogonal *"have we already counted
|
||||
this viewer recently?"* gate; it does **not** change `shouldCountForStatistics`.
|
||||
|
||||
- `increment` (`service.ts:199-237`) — the two-table transactional write. **Do
|
||||
not change its body**; this plan changes only whether it is called.
|
||||
|
||||
- **Reusable client-identity helper**: `packages/utils/src/rate-limit.ts`
|
||||
exports `TRUSTED_IP_HEADERS` (the ordered list of proxy IP headers the app
|
||||
trusts). The rate-limit middleware already derives a client key from these
|
||||
headers the same way. Reuse `TRUSTED_IP_HEADERS` to read the viewer IP from
|
||||
`input.requestHeaders`; fall back to a `user-agent`-based key when no trusted
|
||||
IP header is present (mirror the middleware's fallback so behavior is
|
||||
consistent).
|
||||
|
||||
- **Environment**: `REDIS_URL` exists in `packages/env/src/server.ts` and
|
||||
`turbo.json` globalEnv, but the app runs as a single Node process by default
|
||||
and the existing rate limiter uses an **in-memory** store
|
||||
(`@orpc/experimental-ratelimit/memory`). Match that: an in-memory TTL cache
|
||||
is the right default here.
|
||||
|
||||
## Commands you will need
|
||||
|
||||
| Purpose | Command | Expected |
|
||||
|-----------|--------------------------------------------------------------------|-----------|
|
||||
| Typecheck | `pnpm --filter @reactive-resume/api typecheck` | exit 0 |
|
||||
| Test | `pnpm --filter @reactive-resume/api test -- resume` | all pass |
|
||||
| All api | `pnpm --filter @reactive-resume/api test` | all pass |
|
||||
|
||||
(Do NOT run `pnpm check`.)
|
||||
|
||||
## Scope
|
||||
|
||||
**In scope**:
|
||||
- A new small helper module, e.g.
|
||||
`packages/api/src/features/resume/view-dedup.ts` (create) — an in-memory
|
||||
TTL set keyed by `${resumeId}:${clientKey}` with a `shouldCountView(...)`
|
||||
function that returns `true` at most once per key per window.
|
||||
- `packages/api/src/features/resume/view-dedup.test.ts` (create)
|
||||
- `packages/api/src/features/resume/service.ts` — gate the `increment` call
|
||||
(lines 505–507) on the new helper.
|
||||
- `plans/README.md` (status row)
|
||||
|
||||
**Out of scope**:
|
||||
- `increment`'s write body and the DB schema — unchanged.
|
||||
- `shouldCountForStatistics` in `access-policy.ts` — unchanged.
|
||||
- Any Redis / distributed-cache implementation — an in-memory window is the
|
||||
agreed default; a distributed store is a documented future upgrade, not this
|
||||
plan.
|
||||
- Download counting (`downloads: true`) — this plan is about view writes only;
|
||||
do not alter download increments.
|
||||
|
||||
## Git workflow
|
||||
|
||||
- Branch: `advisor/005-stats-view-dedup`
|
||||
- Commit: `perf(api): dedup public-resume view increments within a short window`
|
||||
- Do NOT push or open a PR unless instructed.
|
||||
|
||||
## Steps
|
||||
|
||||
### Step 1: Write the dedup helper
|
||||
|
||||
Create `view-dedup.ts` exporting a pure-ish, testable function. Suggested
|
||||
shape (keep it small — this is not a cache library):
|
||||
|
||||
```ts
|
||||
// ponytail: in-memory per-process dedup window. Single-instance is the default
|
||||
// deploy; for multi-instance, swap the Map for a Redis SETNX+EXPIRE keyed the
|
||||
// same way (REDIS_URL already exists in env). Upgrade only if you scale out.
|
||||
const WINDOW_MS = 60 * 60 * 1000; // 1 hour
|
||||
const seen = new Map<string, number>(); // key -> expiry timestamp
|
||||
|
||||
export function shouldCountView(key: string, now: number): boolean {
|
||||
const expiry = seen.get(key);
|
||||
if (expiry !== undefined && expiry > now) return false;
|
||||
seen.set(key, now + WINDOW_MS);
|
||||
return true;
|
||||
}
|
||||
|
||||
export function clientKeyFromHeaders(headers: Headers): string { /* uses TRUSTED_IP_HEADERS, UA fallback */ }
|
||||
```
|
||||
|
||||
Take `now` as a parameter (don't call `Date.now()` inside the predicate) so the
|
||||
test can drive the clock deterministically. Add a lightweight size guard: if
|
||||
`seen.size` exceeds a cap (e.g. 50_000), prune entries whose expiry has passed
|
||||
before inserting, so the Map can't grow unbounded. Reuse `TRUSTED_IP_HEADERS`
|
||||
from `@reactive-resume/utils/rate-limit` inside `clientKeyFromHeaders`.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api typecheck` → exit 0.
|
||||
|
||||
### Step 2: Test the helper
|
||||
|
||||
Create `view-dedup.test.ts`:
|
||||
- `shouldCountView(key, t)` returns `true` the first time, `false` for the same
|
||||
key within the window, and `true` again once `now` is past the window.
|
||||
- Two different keys are independent.
|
||||
- `clientKeyFromHeaders` derives distinct keys for distinct trusted-IP headers
|
||||
and a stable key for the UA fallback when no IP header is present.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api test -- view-dedup` → all pass.
|
||||
|
||||
### Step 3: Gate the increment call
|
||||
|
||||
In `service.ts` `getBySlug`, wrap the existing count so it fires only when both
|
||||
the policy allows it AND the viewer hasn't been counted this window:
|
||||
|
||||
```ts
|
||||
if (shouldCountForStatistics(resume, viewer)) {
|
||||
const key = `${resume.id}:${clientKeyFromHeaders(input.requestHeaders)}`;
|
||||
if (shouldCountView(key, Date.now())) {
|
||||
await resumeService.statistics.increment({ id: resume.id, views: true });
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Do not change anything else in `getBySlug`.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api typecheck` → exit 0.
|
||||
|
||||
### Step 4: Confirm no regression in existing resume tests
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/api test -- resume` → all pass. In
|
||||
particular, if Plan 001's `statistics.increment` characterization test exists,
|
||||
it should still pass (this plan didn't touch `increment`'s body). If a
|
||||
`getBySlug` test asserts increment-on-view, it may now need the test to pass a
|
||||
fresh header/clock so the first view still counts — update it to reflect the
|
||||
dedup (first view counts, immediate repeat does not).
|
||||
|
||||
## Test plan
|
||||
|
||||
- `view-dedup.test.ts`: window behavior + key derivation (Step 2 cases).
|
||||
- If an existing `getBySlug` test asserts view counting, extend it: first call
|
||||
with a given client key increments; an immediate second call with the same
|
||||
key does not; a call with a different key does.
|
||||
- Verification: `pnpm --filter @reactive-resume/api test` → all pass.
|
||||
|
||||
## Done criteria
|
||||
|
||||
Machine-checkable. ALL must hold:
|
||||
|
||||
- [ ] `packages/api/src/features/resume/view-dedup.ts` and its `.test.ts` exist
|
||||
- [ ] `pnpm --filter @reactive-resume/api test -- view-dedup` passes with ≥ 4 cases
|
||||
- [ ] `grep -n "shouldCountView" packages/api/src/features/resume/service.ts` shows the gate around the increment call
|
||||
- [ ] `pnpm --filter @reactive-resume/api typecheck` exits 0
|
||||
- [ ] `pnpm --filter @reactive-resume/api test` exits 0
|
||||
- [ ] `git status --porcelain` lists only in-scope files + `plans/README.md`
|
||||
- [ ] `plans/README.md` status row for 005 updated
|
||||
|
||||
## STOP conditions
|
||||
|
||||
Stop and report if:
|
||||
|
||||
- `getBySlug` no longer calls `statistics.increment` at lines ~505–507, or its
|
||||
signature no longer exposes `requestHeaders` — the code drifted.
|
||||
- You cannot derive a client key from `input.requestHeaders` because headers
|
||||
aren't actually populated at this layer in practice (check a real request
|
||||
path / existing rate-limit middleware usage) — report it; a dedup keyed on an
|
||||
empty header is worthless.
|
||||
- You find that view counting must remain exact (product decision that every
|
||||
raw hit counts) — the dedup changes counter semantics slightly; if unsure,
|
||||
stop and confirm before shipping.
|
||||
|
||||
## Maintenance notes
|
||||
|
||||
- **Semantics change**: counters become "unique-ish views per 1h window per
|
||||
client" rather than "raw hits". Document this near the helper. If the product
|
||||
wants raw hit counts back, this gate is the single place to remove.
|
||||
- **Multi-instance ceiling**: the in-memory Map is per-process. If the app is
|
||||
ever horizontally scaled, each instance dedups independently (still a large
|
||||
reduction, but not global). The ponytail comment names Redis as the upgrade
|
||||
path; do it only if scale-out happens.
|
||||
- Reviewer should confirm `Date.now()` is only called at the call site (Step 3),
|
||||
not inside the predicate, so the helper stays testable.
|
||||
@@ -0,0 +1,191 @@
|
||||
# Plan 006: Stop re-mapping the font list per combobox instance; defer the font-metadata payload
|
||||
|
||||
> **Executor instructions**: Follow step by step. Confirm each verification.
|
||||
> Honor "STOP conditions". When done, update the status row in `plans/README.md`.
|
||||
>
|
||||
> **Drift check (run first)**:
|
||||
> `git diff --stat 73daf22b2..HEAD -- apps/web/src/components/typography/combobox.tsx apps/web/src/routes/builder/$resumeId/-sidebar/right/sections/typography.tsx packages/fonts/src/index.ts`
|
||||
> If any changed, compare "Current state" excerpts against live code; on
|
||||
> mismatch, STOP.
|
||||
|
||||
## Status
|
||||
|
||||
- **Priority**: P2
|
||||
- **Effort**: S
|
||||
- **Risk**: LOW
|
||||
- **Depends on**: none
|
||||
- **Category**: perf
|
||||
- **Planned at**: commit `73daf22b2`, 2026-07-08
|
||||
|
||||
## Why this matters
|
||||
|
||||
`packages/fonts/src/webfontlist.json` is a **476 KB** metadata file (7,526
|
||||
lines). `packages/fonts/src/index.ts:7` imports it statically and derives
|
||||
`fontList` (~506 entries). The typography combobox
|
||||
(`apps/web/src/components/typography/combobox.tsx:11-24`) maps that full list
|
||||
into option objects inside a `useMemo(..., [])` — which memoizes **per component
|
||||
instance**, not globally. Two instances render (body + heading font pickers), so
|
||||
the 506-entry map runs twice at mount for identical output.
|
||||
|
||||
Two honest, bounded improvements:
|
||||
1. **Guaranteed win**: hoist the options mapping to module scope so it runs
|
||||
once per process regardless of instance count. Small but free and correct.
|
||||
2. **Payload win (conditional)**: the font metadata only matters where the font
|
||||
picker renders (the builder typography panel). Deferring it keeps it out of
|
||||
any route/panel that never opens the picker — *if* the bundler isn't already
|
||||
forced to load it there for another reason (PDF font registration also
|
||||
consumes `@reactive-resume/fonts`). Step 3 measures before changing, and
|
||||
reports back rather than forcing a change that yields nothing.
|
||||
|
||||
Scope this realistically: #1 is certain; #2 is worth doing only if the build
|
||||
shows the payload actually lands on a route that doesn't need it.
|
||||
|
||||
## Current state
|
||||
|
||||
- `apps/web/src/components/typography/combobox.tsx:1-27` — `FontFamilyCombobox`:
|
||||
|
||||
```ts
|
||||
import { fontList, getFont, getFontDisplayName, getFontSearchKeywords, sortFontWeights } from "@reactive-resume/fonts";
|
||||
// ...
|
||||
export function FontFamilyCombobox({ className, ...props }: FontFamilyComboboxProps) {
|
||||
const options = useMemo(() => {
|
||||
return fontList.map((font) => ({
|
||||
value: font.family,
|
||||
keywords: getFontSearchKeywords(font.family),
|
||||
label: <FontDisplay family={font.family} label={getFontDisplayName(font.family)} type={font.type}
|
||||
url={"preview" in font ? font.preview : undefined} />,
|
||||
}));
|
||||
}, []);
|
||||
return <Combobox {...props} options={options} className={cn("w-full", className)} />;
|
||||
}
|
||||
```
|
||||
|
||||
The `label` is JSX (`<FontDisplay .../>`), so the mapped array holds React
|
||||
elements — hoisting must keep the elements' props static (they are: derived
|
||||
purely from `font`). Nothing in the map depends on component props.
|
||||
|
||||
- **Single usage site**:
|
||||
`apps/web/src/routes/builder/$resumeId/-sidebar/right/sections/typography.tsx:14`
|
||||
imports and renders `FontFamilyCombobox` (line ~106) and `FontWeightCombobox`
|
||||
(line ~133). No other file uses these components.
|
||||
|
||||
- `packages/fonts/src/index.ts` — statically imports `webfontlist.json` (line 7)
|
||||
and exports `fontList`, `getFont`, `getFontDisplayName`,
|
||||
`getFontSearchKeywords`, `sortFontWeights` (used by the combobox) plus
|
||||
registration helpers used by `packages/pdf`
|
||||
(`packages/pdf/src/hooks/use-register-fonts.ts`).
|
||||
|
||||
- Bundler note: TanStack Router has `autoCodeSplitting`; per prior analysis the
|
||||
font metadata resolves into the `pdf-document` chunk, not the main entry.
|
||||
This is exactly why Step 3 measures before assuming a payload win exists.
|
||||
|
||||
## Commands you will need
|
||||
|
||||
| Purpose | Command | Expected |
|
||||
|-----------------|---------------------------------------------------|---------------------|
|
||||
| Typecheck (web) | `pnpm --filter web typecheck` | exit 0 |
|
||||
| Web tests | `pnpm --filter web test -- typography` | all pass (or none) |
|
||||
| Build (Step 3) | `pnpm --filter web build` | exit 0; chunk sizes printed |
|
||||
|
||||
(Do NOT run `pnpm check`.)
|
||||
|
||||
## Scope
|
||||
|
||||
**In scope**:
|
||||
- `apps/web/src/components/typography/combobox.tsx`
|
||||
- `apps/web/src/routes/builder/$resumeId/-sidebar/right/sections/typography.tsx`
|
||||
(only if Step 3 justifies lazy-loading)
|
||||
- `plans/README.md` (status row)
|
||||
|
||||
**Out of scope**:
|
||||
- `packages/fonts/*` — do not restructure the fonts package or change what
|
||||
`@reactive-resume/fonts` exports. The win here is on the web-app consumer side.
|
||||
- `packages/pdf` font registration — unrelated consumer.
|
||||
- The 476 KB JSON content itself.
|
||||
|
||||
## Git workflow
|
||||
|
||||
- Branch: `advisor/006-font-combobox`
|
||||
- Commit: `perf(web): compute font-picker options once at module scope`
|
||||
- Do NOT push or open a PR unless instructed.
|
||||
|
||||
## Steps
|
||||
|
||||
### Step 1: Hoist the font options to module scope
|
||||
|
||||
In `combobox.tsx`, move the `fontList.map(...)` computation out of the component
|
||||
into a module-level constant (e.g. `const FONT_FAMILY_OPTIONS = fontList.map(...)`)
|
||||
and pass that constant to `<Combobox options={FONT_FAMILY_OPTIONS} />`. Remove
|
||||
the now-unnecessary `useMemo` for `FontFamilyCombobox` (its dep array is empty
|
||||
and the value is now module-constant). Leave `FontWeightCombobox` unchanged —
|
||||
its options depend on the `fontFamily` prop and must stay per-instance.
|
||||
|
||||
**Verify**: `pnpm --filter web typecheck` → exit 0.
|
||||
|
||||
### Step 2: Confirm the picker still renders
|
||||
|
||||
If a typography test exists, run it; otherwise this is verified by typecheck +
|
||||
the build in Step 3. Ensure `FontDisplay` still receives the same props.
|
||||
|
||||
**Verify**: `pnpm --filter web test -- typography` → passes or reports no tests.
|
||||
|
||||
### Step 3: Measure, then decide on deferral (investigate — may end in a report)
|
||||
|
||||
Run `pnpm --filter web build` and note the chunk that contains the font
|
||||
metadata (search the build output / `apps/web/dist/assets` for the large
|
||||
chunk; the 476 KB JSON shows up as a ~400–500 KB contribution). Determine
|
||||
whether that chunk loads on a route that does **not** need the font picker
|
||||
(e.g. the dashboard or a public resume page).
|
||||
|
||||
- **If the font metadata already only loads with the PDF/preview chunk that the
|
||||
builder needs anyway** → deferring the combobox yields ~nothing. STOP and
|
||||
report this finding in `plans/README.md`'s status note; do not add lazy
|
||||
loading. Step 1 stands on its own as the deliverable.
|
||||
- **If the font metadata loads on a route with no font picker** → wrap
|
||||
`FontFamilyCombobox`/`FontWeightCombobox` at the `typography.tsx` usage site in
|
||||
`React.lazy` + `Suspense` (import the combobox module via dynamic `import()`),
|
||||
so `@reactive-resume/fonts` is fetched only when the typography panel mounts.
|
||||
Keep the fallback minimal (the existing field skeleton or a small spinner).
|
||||
|
||||
**Verify**: `pnpm --filter web build` → exit 0; record the before/after chunk
|
||||
observation in the PR description.
|
||||
|
||||
## Test plan
|
||||
|
||||
- Step 1 is behavior-preserving; typecheck + build are the gates.
|
||||
- If Step 3 adds lazy loading, manually confirm (or via an existing e2e that
|
||||
opens the typography panel) that the font picker still populates.
|
||||
- No new unit test is required for a pure hoist; if `typography.tsx` gains a
|
||||
Suspense boundary, ensure any existing builder e2e still passes.
|
||||
|
||||
## Done criteria
|
||||
|
||||
Machine-checkable. ALL must hold:
|
||||
|
||||
- [ ] `grep -n "FONT_FAMILY_OPTIONS\|const .*= fontList.map" apps/web/src/components/typography/combobox.tsx` shows a module-scope constant
|
||||
- [ ] `FontFamilyCombobox` no longer wraps the family options in `useMemo`
|
||||
- [ ] `pnpm --filter web typecheck` exits 0
|
||||
- [ ] `pnpm --filter web build` exits 0
|
||||
- [ ] `git status --porcelain` lists only in-scope files + `plans/README.md`
|
||||
- [ ] `plans/README.md` status row for 006 updated (note whether Step 3 added
|
||||
lazy loading or reported it unnecessary)
|
||||
|
||||
## STOP conditions
|
||||
|
||||
Stop and report if:
|
||||
|
||||
- Hoisting breaks the `FontDisplay` label rendering (elements need per-instance
|
||||
data) — re-check; the current props are purely `font`-derived, so this should
|
||||
not happen. If it does, the component drifted.
|
||||
- Step 3's measurement is ambiguous or the build doesn't surface chunk sizes —
|
||||
report what you observed rather than guessing; do not add lazy loading on a
|
||||
hunch.
|
||||
|
||||
## Maintenance notes
|
||||
|
||||
- This is the lowest-leverage item in the current batch; Step 1 is the sure
|
||||
thing. Do not over-engineer a fonts-package split — that has broad blast
|
||||
radius (PDF registration depends on the same exports) and isn't justified by
|
||||
the payload analysis.
|
||||
- If the fonts list ever grows substantially or becomes user-configurable,
|
||||
revisit a proper lazy/virtualized font source.
|
||||
@@ -0,0 +1,279 @@
|
||||
# Plan 007: Extract a shared template page-shell — spike + one-template pilot
|
||||
|
||||
> **Executor instructions**: This is a **spike + pilot**, not a 15-file
|
||||
> rewrite. You will build a parity net, refactor exactly ONE template behind it,
|
||||
> and then STOP and report. Do NOT migrate the other templates in this plan.
|
||||
> Run every verification. Honor "STOP conditions". When done, update the status
|
||||
> row in `plans/README.md`.
|
||||
>
|
||||
> **Drift check (run first)**:
|
||||
> `git diff --stat 73daf22b2..HEAD -- packages/pdf/src/templates/`
|
||||
> If the templates changed since this plan, compare "Current state" against
|
||||
> live code before proceeding; on mismatch, STOP.
|
||||
|
||||
## Status
|
||||
|
||||
- **Priority**: P2
|
||||
- **Effort**: L
|
||||
- **Risk**: HIGH
|
||||
- **Depends on**: 001 recommended as the characterization-testing exemplar/
|
||||
discipline (not a technical blocker — the PDF layer needs its own parity net,
|
||||
which is Step 1 here).
|
||||
- **Category**: tech-debt
|
||||
- **Planned at**: commit `73daf22b2`, 2026-07-08
|
||||
|
||||
## Why this matters
|
||||
|
||||
All 15 resume templates
|
||||
(`packages/pdf/src/templates/<name>/<Name>Page.tsx`) independently reimplement
|
||||
the **same page-shell orchestration**: compute `getTemplateMetrics`,
|
||||
`getTemplatePageSize`, `getTemplatePageMinHeightStyle`, `shouldShowResumeHeader`,
|
||||
`hasTemplatePicture`, `filterSections(page.main/sidebar)`, then render
|
||||
`<Page>` → `<TemplateProvider>` → a two-column `layout` → sidebar/main columns
|
||||
that `.map` over `<Section>`. Git history proves the cost: a single header
|
||||
line-height change touched 14 template files (commit `1b0bb067b`); the free-form
|
||||
layout feature touched 14 (`2cd774dab`). Every new template copies this again,
|
||||
and drift between copies is invisible until someone diffs all 15.
|
||||
|
||||
The goal is a shared, prop-/callback-driven `TemplatePageShell` that owns the
|
||||
orchestration while each template keeps its own styles and decorative choices.
|
||||
**Because the visual risk is high and no render-parity test net exists for
|
||||
templates today, this plan proves the abstraction on ONE template (Pikachu)
|
||||
behind a characterization snapshot, then stops for review before any rollout.**
|
||||
|
||||
## Current state
|
||||
|
||||
- `packages/pdf/src/templates/pikachu/PikachuPage.tsx` — the pilot. Structure
|
||||
(lines 58–110): the `PikachuPage` component computes the shared metrics/flags
|
||||
and renders the shell; a local `Header` component (lines 112–161) renders
|
||||
`basics.name`/`headline` + the shared contact-item components; a
|
||||
`usePikachuTemplate` hook (lines 163–292) builds the per-template `StyleSheet`.
|
||||
The orchestration block, verbatim:
|
||||
|
||||
```tsx
|
||||
export const PikachuPage = ({ page, pageIndex }: TemplatePageProps) => {
|
||||
const data = useRender();
|
||||
const { metadata, picture } = data;
|
||||
const { colors, styles } = usePikachuTemplate();
|
||||
const metrics = getTemplateMetrics(metadata.page);
|
||||
const pageSize = getTemplatePageSize(metadata.page.format);
|
||||
const pageMinHeightStyle = getTemplatePageMinHeightStyle(metadata.page.format);
|
||||
const showHeader = shouldShowResumeHeader(data, pageIndex);
|
||||
const showSidebar = !page.fullWidth;
|
||||
const hasPicture = hasTemplatePicture(picture);
|
||||
const mainSections = filterSections(page.main, data);
|
||||
const sidebarSections = filterSections(page.sidebar, data);
|
||||
|
||||
return (
|
||||
<Page size={pageSize} style={composeStyles(styles.page, pageMinHeightStyle)}>
|
||||
<TemplateProvider styles={styles} colors={colors} features={pikachuFeatures}>
|
||||
<View style={styles.layout}>
|
||||
{showSidebar && (
|
||||
<View style={composeStyles(styles.sidebarColumn, { width: `${metadata.layout.sidebarWidth}%`, rowGap: metrics.sectionGap })}>
|
||||
{showHeader && showSidebar && hasPicture && <Image src={picture.url} style={styles.picture} />}
|
||||
<View style={composeStyles(styles.sidebarContent, { rowGap: metrics.sectionGap })}>
|
||||
{sidebarSections.map((s) => <Section key={s} section={s} placement="sidebar" />)}
|
||||
</View>
|
||||
</View>
|
||||
)}
|
||||
<View style={composeStyles(styles.mainColumn, { rowGap: metrics.sectionGap })}>
|
||||
{showHeader && (
|
||||
<View style={styles.headerRow}>
|
||||
{showHeader && !showSidebar && hasPicture && <Image src={picture.url} style={styles.picture} />}
|
||||
<Header styles={styles} colors={colors} />
|
||||
</View>
|
||||
)}
|
||||
<View style={{ rowGap: metrics.sectionGap }}>
|
||||
{mainSections.map((s) => <Section key={s} section={s} placement="main" />)}
|
||||
</View>
|
||||
</View>
|
||||
</View>
|
||||
</TemplateProvider>
|
||||
</Page>
|
||||
);
|
||||
};
|
||||
```
|
||||
|
||||
**Template-specific variation you must preserve** (this is why extraction is
|
||||
risky — the shell cannot assume one layout): Pikachu wraps its `Header` in a
|
||||
colored box and places the picture *beside* the header in the main column when
|
||||
there's no sidebar, but *above* the sidebar sections when there is. Other
|
||||
templates differ: Azurill (`AzurillPage.tsx`) uses `flexBasis` for sidebar
|
||||
width instead of a `width` percentage; Onyx (`OnyxPage.tsx`) is single-column
|
||||
header-on-top; Gengar (`GengarPage.tsx`) calls `getFeaturedSummaryLayout`.
|
||||
The shared shell must expose enough seams (render callbacks / slots) that each
|
||||
template keeps these differences — do not flatten them into one hardcoded
|
||||
layout.
|
||||
|
||||
- Shared helpers already exist under `packages/pdf/src/templates/shared/`:
|
||||
`metrics.ts`, `page-size.ts`, `cover-letter.ts` (`shouldShowResumeHeader`),
|
||||
`filtering.ts`, `picture.ts`, `contact-item.tsx`, `context.tsx`
|
||||
(`TemplateProvider`), `sections.tsx` (`Section`), `styles.ts` (`composeStyles`).
|
||||
The extraction consolidates the *orchestration that wires these together*, not
|
||||
the helpers themselves.
|
||||
|
||||
- **No render-parity test exists per template.** The only per-template test is
|
||||
`templates/scizor/ScizorPage.test.ts`, which greps source text — not a
|
||||
structural snapshot. You will build a real parity net in Step 1.
|
||||
|
||||
- `packages/pdf/src/document.ts` — defines `TemplatePageProps` and
|
||||
`TemplatePage`; `templates/index.ts` maps template name → page component.
|
||||
|
||||
## Commands you will need
|
||||
|
||||
| Purpose | Command | Expected |
|
||||
|----------------|---------------------------------------------------------------|--------------------|
|
||||
| Typecheck | `pnpm --filter @reactive-resume/pdf typecheck` | exit 0 |
|
||||
| PDF tests | `pnpm --filter @reactive-resume/pdf test` | all pass |
|
||||
| Pilot snapshot | `pnpm --filter @reactive-resume/pdf test -- pikachu` | all pass |
|
||||
|
||||
(Do NOT run `pnpm check`.)
|
||||
|
||||
## Scope
|
||||
|
||||
**In scope**:
|
||||
- A new shared shell, e.g. `packages/pdf/src/templates/shared/page-shell.tsx`
|
||||
(create) and its type additions in `shared/types.ts` if needed.
|
||||
- `packages/pdf/src/templates/pikachu/PikachuPage.tsx` (the ONE pilot migration)
|
||||
- A new parity snapshot test:
|
||||
`packages/pdf/src/templates/pikachu/PikachuPage.test.tsx` (create)
|
||||
- `plans/README.md` (status row + a note on the pilot outcome)
|
||||
|
||||
**Out of scope (hard stop)**:
|
||||
- The other 14 `*Page.tsx` templates — **do not touch them in this plan.** Their
|
||||
migration is explicit follow-up, gated on this pilot's review.
|
||||
- The shared helpers' internals (`metrics.ts`, `filtering.ts`, etc.).
|
||||
- `templates/index.ts` mapping — Pikachu's export name/signature must not change.
|
||||
- Any visual/styling change — this is a pure structural extraction; the pilot's
|
||||
rendered output must be byte-identical to before.
|
||||
|
||||
## Git workflow
|
||||
|
||||
- Branch: `advisor/007-template-shell-spike`
|
||||
- Commit style: conventional commits, e.g.
|
||||
`refactor(pdf): extract shared template page-shell; pilot on pikachu`.
|
||||
- Do NOT push or open a PR unless instructed.
|
||||
|
||||
## Steps
|
||||
|
||||
### Step 1: Build the parity net for Pikachu (BEFORE refactoring)
|
||||
|
||||
Create `packages/pdf/src/templates/pikachu/PikachuPage.test.tsx` that renders
|
||||
`PikachuPage` with a fixed sample resume and metadata and snapshots the produced
|
||||
element tree. Approach:
|
||||
|
||||
- Use `react-test-renderer` (already available transitively; if not, use the
|
||||
approach the existing `packages/pdf/src/browser.test.tsx` / `server.test.tsx`
|
||||
use to exercise components) to render the template wrapped in whatever context
|
||||
`useRender()` needs. Read `packages/pdf/src/context.tsx` to see how to provide
|
||||
the render context in a test (there may be a provider/helper the existing
|
||||
tests use — reuse it).
|
||||
- Feed it `sampleResumeData` from `@reactive-resume/schema/resume/sample` (used
|
||||
by `server.test.tsx`) and a representative `page` prop (one with a sidebar,
|
||||
one full-width — two snapshots).
|
||||
- Snapshot the tree with `expect(tree.toJSON()).toMatchSnapshot()`.
|
||||
|
||||
Run it to generate the baseline snapshot **against the current, un-refactored
|
||||
PikachuPage**.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/pdf test -- pikachu` → passes and
|
||||
writes a `__snapshots__` file. Commit this snapshot as the baseline.
|
||||
|
||||
> If you cannot render the template in a test without excessive/fragile mocking
|
||||
> (react-pdf host primitives don't cooperate with the test renderer), STOP and
|
||||
> report — a structural refactor with no parity net is exactly what this plan
|
||||
> refuses to ship. Do not proceed to Step 2 without a working baseline.
|
||||
|
||||
### Step 2: Extract the shared shell
|
||||
|
||||
Create `shared/page-shell.tsx` exporting a `TemplatePageShell` component (or a
|
||||
hook + component pair) that encapsulates the orchestration from the excerpt:
|
||||
computing `metrics`/`pageSize`/`pageMinHeightStyle`/`showHeader`/`showSidebar`/
|
||||
`hasPicture`/`mainSections`/`sidebarSections`, and rendering the
|
||||
`<Page><TemplateProvider>...<View layout>` scaffold. Expose the per-template
|
||||
variation through props/render-callbacks, at minimum:
|
||||
- `styles`, `colors`, `features` (passed through to `TemplateProvider` and used
|
||||
by the scaffold).
|
||||
- `renderHeader` / `renderPicture` callbacks (or slot props) so Pikachu can keep
|
||||
its colored-box header and beside/above picture placement.
|
||||
- The sidebar-width strategy as data (Pikachu passes a `width` %; Azurill will
|
||||
later pass `flexBasis`) — model it so both fit without the shell hardcoding one.
|
||||
|
||||
Keep the shell's public surface minimal and documented with a short comment.
|
||||
|
||||
**Verify**: `pnpm --filter @reactive-resume/pdf typecheck` → exit 0.
|
||||
|
||||
### Step 3: Migrate Pikachu onto the shell
|
||||
|
||||
Rewrite `PikachuPage.tsx` so `PikachuPage` delegates its orchestration to
|
||||
`TemplatePageShell`, passing `usePikachuTemplate()`'s styles/colors, the
|
||||
`pikachuFeatures`, and its `Header`/picture rendering via the callbacks. Keep
|
||||
`usePikachuTemplate` and the `Header` component in the Pikachu file (styles stay
|
||||
template-owned). The export name and `TemplatePageProps` signature must not
|
||||
change.
|
||||
|
||||
**Verify**:
|
||||
- `pnpm --filter @reactive-resume/pdf test -- pikachu` → the Step 1 snapshot
|
||||
**still matches** (identical tree). If the snapshot changed, the refactor
|
||||
altered output — investigate and reconcile; do NOT blindly update the
|
||||
snapshot.
|
||||
- `pnpm --filter @reactive-resume/pdf test` → the full PDF suite passes.
|
||||
- `pnpm --filter @reactive-resume/pdf typecheck` → exit 0.
|
||||
|
||||
### Step 4: STOP and report — do not roll out
|
||||
|
||||
Write a short note in `plans/README.md`'s status area (or the PR description)
|
||||
covering: whether the shell's seams were sufficient for Pikachu without style
|
||||
regressions, what the shell's public API ended up being, and which of the other
|
||||
14 templates look like clean fits vs. which have layouts (e.g. Onyx single
|
||||
column, Gengar featured-summary) that will need extra seams. This note is the
|
||||
input for the reviewed rollout plans. **Do not migrate any other template.**
|
||||
|
||||
## Test plan
|
||||
|
||||
- New `PikachuPage.test.tsx` with two snapshots (with-sidebar, full-width),
|
||||
established as a baseline in Step 1 and asserted unchanged after Step 3.
|
||||
- Full `packages/pdf` suite must remain green.
|
||||
- Verification: `pnpm --filter @reactive-resume/pdf test` → all pass; the
|
||||
Pikachu snapshot is unchanged between baseline and post-refactor.
|
||||
|
||||
## Done criteria
|
||||
|
||||
Machine-checkable. ALL must hold:
|
||||
|
||||
- [ ] `packages/pdf/src/templates/shared/page-shell.tsx` exists and is imported by `PikachuPage.tsx`
|
||||
- [ ] `packages/pdf/src/templates/pikachu/PikachuPage.test.tsx` + its `__snapshots__` exist
|
||||
- [ ] `pnpm --filter @reactive-resume/pdf test` exits 0 (snapshot unchanged post-refactor)
|
||||
- [ ] `pnpm --filter @reactive-resume/pdf typecheck` exits 0
|
||||
- [ ] `git diff --name-only` shows **exactly one** `*Page.tsx` changed (pikachu); no other template file modified
|
||||
- [ ] `plans/README.md` status row for 007 updated with the pilot-outcome note
|
||||
- [ ] `grep -rL page-shell packages/pdf/src/templates/*/[A-Z]*Page.tsx` still lists 14 templates (i.e. only Pikachu adopted it)
|
||||
|
||||
## STOP conditions
|
||||
|
||||
Stop and report back (do not improvise) if:
|
||||
|
||||
- You cannot build a working render-parity snapshot in Step 1 (Step 1's own
|
||||
stop clause) — no net, no refactor.
|
||||
- The Pikachu snapshot changes after Step 3 and you cannot make it identical —
|
||||
the extraction is not behavior-preserving; report the diff instead of
|
||||
updating the snapshot to match.
|
||||
- Making the shell fit Pikachu forces the abstraction to also encode a second
|
||||
template's layout (you find yourself designing for Onyx/Gengar mid-pilot) —
|
||||
stop; the pilot's job is to prove one clean seam set, not to pre-solve all 15.
|
||||
- Any change would touch a template other than Pikachu — that's the rollout,
|
||||
which is out of scope.
|
||||
|
||||
## Maintenance notes
|
||||
|
||||
- Rollout is deliberately deferred. After this pilot is reviewed and merged,
|
||||
create follow-up plans that migrate the remaining templates in small reviewed
|
||||
batches (group by layout family: full-width/single-column like Onyx; sidebar-%
|
||||
like Pikachu; flexBasis like Azurill; featured-summary like Gengar). Each
|
||||
batch gets its own baseline snapshot first.
|
||||
- The shell's public API is load-bearing for 14 future migrations — a reviewer
|
||||
should scrutinize its seam design (are the render callbacks expressive enough?)
|
||||
more than the Pikachu diff itself.
|
||||
- Prior maintainer guidance: template rewrites should be spike-gated and must
|
||||
not break the react-pdf/DOCX/JSON export pipeline. This plan honors that by
|
||||
keeping styles template-owned and asserting identical render output.
|
||||
+171
@@ -0,0 +1,171 @@
|
||||
# Implementation Plans
|
||||
|
||||
Generated by the `improve` skill (deep audit) on 2026-07-08, against commit
|
||||
`73daf22b2`. A deep 8-category read-only audit produced 53 raw findings; after
|
||||
adversarial verification and manual vetting, the user selected the items below
|
||||
to turn into executor plans.
|
||||
|
||||
Each executor: read your plan fully before starting, honor its STOP conditions,
|
||||
and update your row when done. Plans are self-contained — you do not need the
|
||||
audit or this repo's history to execute one.
|
||||
|
||||
## Execution order & status
|
||||
|
||||
| Plan | Title | Priority | Effort | Risk | Depends on | Status |
|
||||
|------|-------|----------|--------|------|------------|--------|
|
||||
| 001 | Characterization tests for the resume service | P1 | M | LOW | — | DONE (branch `advisor/resume-service-improvements`) |
|
||||
| 002 | CSP + framing headers on web pages; gate uploads CORS | P1 | S | LOW | — | DONE (branch `advisor/security-and-docs`) |
|
||||
| 003 | Fix silent-success mutations; bound bulk-op inputs | P1 | S | LOW | 001 (soft) | DONE (branch `advisor/resume-service-improvements`) |
|
||||
| 004 | Correct README "Custom CSS" claim | P3 | S | LOW | — | DONE (branch `advisor/security-and-docs`) |
|
||||
| 005 | Deduplicate public-resume view-count writes | P2 | M | MED | 001 (soft) | DONE (branch `advisor/resume-service-improvements`) |
|
||||
| 006 | Font-picker options once; defer font payload | P2 | S | LOW | — | DONE (branch `advisor/font-payload`; Step-1 hoist only — build measurement confirmed the payload already loads only with the PDF chunk, so lazy-loading was correctly skipped) |
|
||||
| 007 | Extract shared template page-shell — spike + pilot | P2 | L | HIGH | 001 (discipline) | REVERTED from integration (user decision) — spike executed & proved the recipe, but full rollout judged not worth it; Pikachu reverted for template uniformity. Work preserved on branch `advisor/template-shell-spike`. |
|
||||
|
||||
Status values: TODO | IN PROGRESS | DONE | BLOCKED (one-line reason) | REJECTED (one-line rationale)
|
||||
|
||||
### Recommended order
|
||||
|
||||
Independent quick wins first (002, 004), then the tests-first foundation (001)
|
||||
before the behavior change that depends on it (003) and the caller change with
|
||||
a safety net (005). 006 is independent and low-leverage — do it anytime. 007
|
||||
(template refactor) is last and is a **spike + one-template pilot that stops for
|
||||
review**, not a full rollout.
|
||||
|
||||
Fastest safe sequence: **002 → 004 → 001 → 003 → 005 → 006 → 007**.
|
||||
|
||||
## Execution results (2026-07-08)
|
||||
|
||||
All 7 plans were executed by isolated-worktree executor subagents and reviewed
|
||||
by the advisor (re-ran every done-criterion, scope-checked, read diffs, audited
|
||||
tests). All **APPROVED**. Four branches, all based on `73daf22b2`:
|
||||
|
||||
| Branch | Plans | Package(s) | Verify (re-run by reviewer) |
|
||||
|--------|-------|------------|------------------------------|
|
||||
| `advisor/security-and-docs` | 002, 004 | apps/server, README | typecheck 0; `server` static tests 36/36 |
|
||||
| `advisor/font-payload` | 006 | apps/web | typecheck 0; web suite 457/457; build 0 |
|
||||
| `advisor/resume-service-improvements` | 001, 003, 005 | packages/api | typecheck 0; api suite 178/178; boundaries clean |
|
||||
| `advisor/template-shell-spike` | 007 | packages/pdf | typecheck 0; pdf suite 264/264; boundaries clean; parity snapshot byte-identical pre/post refactor |
|
||||
|
||||
**Post-execution decision (user, 2026-07-08):** Local integration branch
|
||||
`improve-integration` created off `main` with plans **001–006 merged**. Plan
|
||||
**007 was reverted** from it (the spike concluded a full 14-template rollout
|
||||
isn't worth it — the shell only centralizes layout orchestration, not the
|
||||
per-template style hooks where most recurring edits land; 6 templates are
|
||||
single-column and don't fit; consistency argues against a partial rollout). The
|
||||
`advisor/template-shell-spike` branch is kept if 007 is ever revisited.
|
||||
Known unrelated red: `packages/api/.../storage/service.test.ts` fails on the
|
||||
pinned **vitest 4.1.9** (`.rejects.toThrow` matcher `TypeError`, fixed in 4.1.10)
|
||||
— present on base `main` too, in a file no plan touches; the vitest suite isn't
|
||||
in CI, so it's latent. Bumping vitest to 4.1.10 clears it (a separate base-repo
|
||||
concern, left untouched).
|
||||
|
||||
**Integration notes for the user (merging is your call — the advisor never merges):**
|
||||
- The four branches touch **disjoint packages**, so they can be merged in any
|
||||
order with no cross-branch source conflicts. The only shared file is
|
||||
`pnpm-lock.yaml`, touched **only** by `advisor/template-shell-spike` — no
|
||||
lockfile conflict between branches.
|
||||
- **All commits are unsigned** (`--no-gpg-sign`): the executors ran headless and
|
||||
the repo's 1Password SSH signer needs interactive auth. Re-sign on merge if
|
||||
branch protection requires signed commits.
|
||||
- **007 adds test-only devDeps** (`react-dom`, `@types/react-dom`) to
|
||||
`packages/pdf` + a `vitest.config.ts` transform tweak — both were required to
|
||||
build the render-parity snapshot the spike depends on (react-test-renderer,
|
||||
which the plan suggested, is React-19-incompatible). Accept these consciously.
|
||||
- **007 is a pilot**: only Pikachu is migrated. The executor's recommended
|
||||
rollout batches for the other 14 templates (each snapshotted first):
|
||||
1. sidebar-% family (bronzor, ditto, glalie, leafish; likely kakuna, lapras,
|
||||
meowth, rhyhorn, scizor — verify two-column shape) — trivial, pass
|
||||
`sidebarColumnStyle={{ width }}`.
|
||||
2. flexBasis family (azurill, chikorita) — pass `sidebarColumnStyle={{ flexBasis }}`;
|
||||
the existing seam already covers it.
|
||||
3. featured-summary family (ditgar, gengar) — needs a new section-rendering
|
||||
seam; do NOT force into the current shell.
|
||||
4. single-column/header-on-top (onyx) — needs a separate shell/layout variant;
|
||||
the current shell hardcodes the two-column scaffold.
|
||||
|
||||
## Dependency notes
|
||||
|
||||
- **003 depends softly on 001**: 003 flips the three `setLocked`/`setPassword`/
|
||||
`removePassword` methods from silent-return to throwing `NOT_FOUND`. If 001 is
|
||||
DONE, its Step 3 assertions must be updated in 003's PR (that test diff is the
|
||||
intended proof of the behavior change). If 001 is not done, 003 adds a minimal
|
||||
test itself.
|
||||
- **005 depends softly on 001**: 001's Step 5 characterizes
|
||||
`statistics.increment`; 005 changes the *caller* (dedup), not the write body,
|
||||
so that test should keep passing — a useful regression signal.
|
||||
- **007 is not technically blocked** by 001 (the PDF layer needs its own render-
|
||||
parity net, which 007 builds in Step 1), but 001 should be done first as the
|
||||
characterization-testing exemplar. 007 explicitly does NOT roll out to all 15
|
||||
templates; rollout is deferred to reviewed follow-up plans.
|
||||
|
||||
## Findings considered and rejected
|
||||
|
||||
Recorded so they aren't re-audited next run. (Refuted by adversarial
|
||||
verification or downgraded during manual vetting against the code.)
|
||||
|
||||
- **"Production secrets committed to repository"** — FALSE. `.env.production` /
|
||||
`.env.local` exist in the working tree but are gitignored (`.env*` with an
|
||||
`.env.example` exception); 0 commits touch them; not in history.
|
||||
- **"Vulnerable @better-auth/oauth-provider (unbound resource indicators)"** —
|
||||
the claimed advisory (GHSA-p2fr-6hmx-4528) could not be verified as real, and
|
||||
the code already configures `validAudiences`. Treat any `@better-auth/*` bump
|
||||
as routine, not a security fix.
|
||||
- **Unauthenticated OAuth2 dynamic client registration bypass** — refuted;
|
||||
three validation layers exist (registration-time, server preflight,
|
||||
authorization endpoint).
|
||||
- **bulkUpdate tag-addition race** — refuted; Postgres row-locking serializes
|
||||
the `UPDATE ... SET tags = <subquery>` correctly.
|
||||
- **agent `getThread` missing ownership check** — refuted; the `WHERE` clause
|
||||
filters `userId` and throws `NOT_FOUND` otherwise.
|
||||
- **Snapshot throttle "bypassed" by AI patches** — deliberate, documented
|
||||
design; pruning is atomic and bounded at 30 versions, not a reactive overflow.
|
||||
Not a bug.
|
||||
- **Missing index on `(user_id, isPublic)`** — no code path filters resumes by
|
||||
public status; speculative (YAGNI).
|
||||
- **Stats daily-series "re-queries every 30 days after cache"** — refuted;
|
||||
TanStack Query 60s staleTime already dedups.
|
||||
- **Large composite components / sections.tsx / rich-input / custom-styles
|
||||
"need splitting"** (ARCH-02/05/06/07) — refuted; each is a single-responsibility
|
||||
internal-only component with no external reuse; a July-2026 audit already chose
|
||||
not to split `sections.tsx`.
|
||||
- **Export paths duplicate filtering (ARCH-03)** — refuted; PDF/DOCX/Markdown
|
||||
have genuinely different filtering needs, not a shared abstraction being
|
||||
reinvented.
|
||||
- **`node-html-parser` major-version lag** — refuted; only stable DOM APIs used,
|
||||
no breaking-change exposure.
|
||||
- **PORT not validated in server.ts (DX-03)** — refuted; the server *does* read
|
||||
`process.env.PORT` in production (`apps/server/src/index.ts:13`).
|
||||
|
||||
Downgraded to notes (real but low-leverage; not planned unless revisited):
|
||||
|
||||
- **Rich-text HTML not sanitized before react-pdf-html** (SEC-07) — PDF-only
|
||||
surface (browser path uses `stripHtml`; PDFs don't execute scripts), on the
|
||||
user's own content. Defense-in-depth only.
|
||||
- **MCP tools don't validate API-key scopes** (SEC-05) — latent; scoped keys
|
||||
can't be created yet. Revisit if/when scoped API keys ship.
|
||||
- **Applications attachment cleanup: concurrent-write orphans + swallowed
|
||||
`Promise.allSettled` failures** (CORRECT-02/03) — real but low severity for
|
||||
single-user; storage hygiene + observability. Candidate for a future plan.
|
||||
- **Agent service is 1397 LOC mixing concerns** (ARCH-04) — real tech debt;
|
||||
large L-effort refactor, deferred.
|
||||
- **Core deps pinned to prerelease/snapshot** (drizzle-orm/kit rc, drizzle-zod
|
||||
snapshot hash, TS native-preview dev build) — watch item; de-risk when stable
|
||||
releases land. Not urgent (not on the CI/production critical path).
|
||||
- **Vite `chunkSizeWarningLimit: 10MB`** masks bundle bloat — trivial DX nit.
|
||||
- **Dev-setup docs vs AGENTS.md `dotenvx` drift** (DX-02) — minor docs friction.
|
||||
- **Applications bulk O(n) in-memory scan** — the input-cap in Plan 003 bounds
|
||||
abuse; the scan rewrite is deferred.
|
||||
|
||||
## Not audited
|
||||
|
||||
Honest coverage gaps from this run:
|
||||
|
||||
- E2E/Playwright test behavior and flakiness (relied on CI).
|
||||
- PDF/DOCX template *rendering* correctness beyond structure (no visual-
|
||||
regression harness exists).
|
||||
- Email/notification delivery reliability.
|
||||
- S3 backup/restore and multi-instance storage behavior.
|
||||
- The AI provider integration layer's retry/streaming internals.
|
||||
- Lingui extraction/locale-catalog completeness (separate track).
|
||||
- Deep dependency transitive-tree audit (worked from manifests + lockfile;
|
||||
`pnpm audit`/`pnpm outdated` network access was not assumed available).
|
||||
Reference in New Issue
Block a user