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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 104 additions & 0 deletions .windsurf/skills/error-logging-flow/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
---
name: error-logging-flow
description: Use when writing or reviewing error handling anywhere in this app-starter — API route handlers, services, frontend data calls, or anything that catches an exception. Explains the first-class error-logging flow (5xx persist to the error log and surface on the /errors page) and the hard rule against swallowing real defects with try/catch + console.error. Use it to wire errors correctly and to spot swallowed errors during review.
---

# Error-logging flow

Error reporting is a first-class feature here, not an afterthought: real defects
(5xx) are persisted to a DB-backed error log and shown on the `/errors` page. The
cardinal sin is a `try { … } catch { console.error(e) }` that eats a real error —
it makes production failures invisible. This skill is how the flow works end to
end and how to stay inside it.

> **Scope:** IIS flavor. The error log lives in `_meta_errors` (T-SQL), written by
> `usp_error_log`, read by `fn_error_list`/`fn_error_get` via `ErrorsRepo`.

## The one rule

**Never swallow a real defect.** In an API route, you do *not* catch-and-log —
you call `next(err)` and let the central error handler decide. The only code that
legitimately swallows is the logger itself (so a logging failure can't take down
the request) — and it still writes to the console as a last resort.

## API side — how it flows

1. **Route handlers `next(err)`.** Every handler is `async (req, res, next) => {
try { … } catch (err) { next(err); } }`. You never `res.status(500)` yourself
and never `console.error` and move on.
2. **Throw typed errors for expected 4xx.** Use `throw new HttpError(404, 'Not
found')` (or the `notFound`/`badRequest`/… response helpers) for client errors.
3. **The central handler** (`packages/api/src/middleware/error-handler.ts`,
mounted last) decides:
- `ZodError` → 422 with a field map (validation — not logged).
- `HttpError` → its status + message (4xx — **not** logged; 4xx is user input
and would drown out real bugs).
- anything else → **500, and persisted** to `_meta_errors` via
`ErrorsRepo.log({ level, source:'api', message, stack, context, user_id })`,
fire-and-forget.
4. **The 500 response body** is generic in production (`serverError` in
`response.ts`) — real `error.message` strings leak table/column names. The
server-side log is the source of truth.

```ts
// CORRECT — route handler
router.post('/', validate(ProjectCreateSchema), async (req, res, next) => {
try {
const id = await projects.create({ ...req.body, owner_id: req.user!.id });
const row = await projects.getById(id);
if (!row) throw new HttpError(500, 'Project disappeared after create');
created(res, row);
} catch (err) {
next(err); // ← logging happens centrally; do NOT console.error here
}
});
```

```ts
// WRONG — swallows a real defect; nothing reaches /errors
try {
await projects.create(input);
} catch (err) {
console.error(err); // ❌ invisible in prod
res.status(500).json({ error: 'oops' }); // ❌ bypasses the handler + log
}
```

## Frontend side — automatic

You almost never log manually on the frontend. The plumbing already covers it:

- **`apiFetch`** (`lib/api.ts`) routes any **5xx** response through `reportError`
→ `POST /errors`. 4xx is left for the form layer to display (it throws
`ApiFetchError`, which `<AppForm>` maps to field errors / a banner). The
`/errors` path itself is excluded to avoid an infinite loop.
- **`installGlobalErrorHandlers`** (`lib/errorLog.ts`) catches uncaught errors and
unhandled promise rejections and reports them.
- The **`ErrorBoundary`** reports render-time crashes.

So: build features with `apiFetch` and you get error logging for free. Don't add a
parallel `try/catch … console.error` around API calls — it would hide the very
5xx the flow is meant to capture.

## Reviewing for swallowed errors

Grep for the anti-patterns and confirm each is legitimate:

```bash
rg -n "catch" packages/api/src | rg -i "console\.(error|log)" # suspicious in API
rg -n "catch\s*\(.*\)\s*\{\s*\}" packages/ # empty catch blocks
```

A catch block is acceptable only when it **re-throws**, calls **`next(err)`**, or
**is** the logger (`ErrorsRepo.log` / `reportError`, which intentionally swallow
and console-fallback). Anything else that eats an error and returns a success-ish
result is a defect. (CLAUDE.md critical rule: do not bypass the error-logging flow
for real defects.)

## Verify it actually logs

Force a 500 (e.g. a route hitting the DB while it's unreachable, or a deliberate
throw) while running the app, then open **`/errors`** — the entry should be there
with level `error`, source `api` (or `frontend`), the message, and the request
context. If it isn't, the error was swallowed somewhere upstream — find and fix
that, don't paper over it.
90 changes: 56 additions & 34 deletions .windsurf/skills/feature-surface-map/SKILL.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
name: feature-surface-map
description: Use when adding, removing, reviewing, or auditing a feature / domain entity (e.g. tasks, issues, projects, invoices) in this app-starter monorepo. Lists every layer and exact file a feature touches — shared types, the SQL Server data layer (tables, functions, repo, migration, schema snapshot, tests), the API (route + the two wiring files), and the frontend (hooks, pages, and the three registration files). Walk it in build order to implement a feature without missing a layer, or in reverse to remove one without leaving orphans behind.
description: Use when adding, removing, reviewing, or auditing a feature / domain entity (e.g. tasks, issues, projects, invoices) in this app-starter monorepo. Lists every layer and exact file a feature touches — shared types, the SQL Server data layer (tables, stored procedures, repo, migration, schema snapshot, tests), the API (route + the two wiring files), and the frontend (hooks, pages, and the three registration files). Walk it in build order to implement a feature without missing a layer, or in reverse to remove one without leaving orphans behind.
---

# Feature surface map (IIS + SQL Server flavor)
Expand All @@ -16,6 +16,19 @@ feature lives so nothing is missed in either direction.
> If you are in the un-ejected template instead, the SQL Server package is at
> `packages/db-mssql/` and `packages/db/` is the Postgres copy — adjust paths.

> **⚠ The DB layer below is described for THIS project, not the app-starter template.**
> This project keeps data access in **stored procedures** (very few functions),
> under a path like **`db/schema/stored-procedures/`**, with **schema-qualified,
> prefix-less** names such as `crd.adjustment_dtl_delete` or `dbo.<entity>_<action>`
> across schemas (`dbo`, `crd`, …). Their exact naming and read/write **conventions &
> best practices are TBD — determine them by analyzing the existing procs**, then
> capture them in a `stored-proc-authoring` skill. (The shipped app-starter template
> instead uses `usp_`/`fn_` objects under `dbo`; the `mssql-function-authoring` skill
> covers template T-SQL mechanics — GO batches, positional binding — that transfer
> regardless of naming.) The *shape* of the surface map (types → DB → API → frontend,
> plus the "register" files) holds either way — only the DB paths and object names
> change.

Throughout, `<entity>` is the singular domain noun (`task`, `issue`, `project`).
The running real example is **`issues`**, which exists in the repo end to end and
even has a child entity (`issue_comments`) — pattern-match against it.
Expand All @@ -30,9 +43,9 @@ Build top-to-bottom (each layer depends on the ones above). Remove bottom-to-top
|---|-------|--------------------------------|------|
| 1 | **Shared types** | `packages/types/src/<entity>.ts` | define |
| 1r | **Types barrel** | `packages/types/src/index.ts` → `export * from './<entity>.js'` | **register** |
| 2 | **DB migration** | `packages/db/src/migrations/NNN_<entity>.sql` (+ optional `.down.sql`) | define |
| 3 | **Table snapshot** | `packages/db/schema/tables/<table>.sql` | define |
| 4 | **Function snapshots** | `packages/db/schema/functions/{usp,fn}_<entity>_*.sql` (one file each) | define |
| 2 | **DB migration / DDL** | `packages/db/src/migrations/NNN_<entity>.sql` (+ optional `.down.sql`) — creates the table **and its stored procedures** | define |
| 3 | **Table snapshot** | `schema/tables/<table>.sql` | define |
| 4 | **Stored procedures** (reads **and** writes; one `.sql` per proc) | `db/schema/stored-procedures/<schema>.<entity>_<action>.sql` — e.g. `crd.adjustment_dtl_delete.sql` (schemas `dbo`/`crd`; **no** `usp_`/`fn_` prefix). Exact naming + read/write conventions & best practices **TBD — determine them by analyzing this project's existing stored procedures** (then capture them in a `stored-proc-authoring` skill). | define |
| 5 | **Row interface** | `packages/db/src/database.ts` → `interface <Entity>Table` + entry on `Database` | define |
| 6 | **Repository** | `packages/db/src/repositories/<entity>.repo.ts` | define |
| 6r | **DB barrel** | `packages/db/src/index.ts` → export the repo + row type | **register** |
Expand Down Expand Up @@ -77,8 +90,8 @@ rg -i "issue" packages/ --glob '!**/node_modules/**'
# Files only (a quick map of the blast radius):
rg -il "issue" packages/ --glob '!**/node_modules/**'

# The SQL function names specifically:
rg -l "fn_issue|usp_issue" packages/
# The DB object names specifically (stored procs / functions for the entity):
rg -l "issue_(create|update|delete|get|list|select)|fn_issue|usp_issue" packages/
```

Run this twice — once with the singular (`issue`) and once with the plural/table
Expand All @@ -97,21 +110,24 @@ Go top-to-bottom. Each step has a real file to copy.
model `z.infer`-ed from it, plus `…CreateSchema` / `…UpdateSchema`. Then add
the `export * from './<entity>.js'` line to `index.ts`. **Never** redefine a
schema in the frontend — import from `@app/types`.
2. **Migration** (`packages/db/src/migrations/NNN_<entity>.sql`): create the
table, `CHECK` constraints matching the enum tuples from step 1, and the
functions — `usp_<entity>_create/update/delete` (writes) and
`fn_<entity>_get/list` (reads). **No direct table access anywhere else** —
functions are the only door.
3. **Snapshots** (`schema/tables/` + `schema/functions/`): one file per table
and per function, **in the same commit** as the migration. The contract test
fails otherwise.
2. **Migration + DB objects**: create the table, `CHECK` constraints matching the
enum tuples from step 1, and the **stored procedures** for every read and write
(`<entity>_create/update/delete`, `<entity>_get/list/select` — use your repo's
actual action vocabulary and schema, e.g. `crd.<entity>_<action>`). **No direct
table access anywhere else** — a proc (or function) is the only door. *(Exact
naming + read/write conventions & best practices are TBD — derive them from this
project's existing stored procedures; see the `mssql-function-authoring` skill.)*
3. **Snapshots**: one `.sql` per table and **per stored proc / function**, in the
same commit as the migration (your project: `db/schema/stored-procedures/`;
template: `schema/{tables,functions}/`). The contract test fails otherwise.
4. **`database.ts`**: add the `<Entity>Table` interface and the `Database` entry.
5. **Repo** (`repositories/<entity>.repo.ts`, mirror `issues.repo.ts`): a thin
wrapper where every method calls a function via the `callFunction*` helpers.
Export it from `src/index.ts`.
6. **Tests** (`test/<entity>.test.ts`): seed via `usp_`, read via `fn_`, validate
the raw row with `<Entity>Schema.strict()`; CRUD round-trip. This is the
highest-value coverage in the repo (the SQL↔TS seam `tsc` can't see).
6. **Tests** (`test/<entity>.test.ts`): seed via the write proc, read via the read
proc/function, validate the raw row with `<Entity>Schema.strict()`; CRUD
round-trip. This is the highest-value coverage in the repo (the SQL↔TS seam
`tsc` can't see).
7. **Route** (`routes/<entity>.ts`, mirror `routes/tasks.ts`): `validate(schema)`
at the boundary, `auth.requireAuth`, always `next(err)` for 5xx (the error
handler logs to the error table — never `console.error` and swallow).
Expand Down Expand Up @@ -151,19 +167,21 @@ transient broken states). Grep first (see above), then walk the map in reverse:
both files for a now-unused repo/type.
4. **DB** — delete the repo, its `src/index.ts` exports, the `<Entity>Table`
interface + `Database` entry in `database.ts`, the `test/<entity>.test.ts`, the
migration file(s), and **the snapshot files** (`schema/tables/` +
`schema/functions/`). Removing snapshots without removing functions (or vice
versa) fails the contract test. If a deployed DB already has the objects, write
a `DROP`-ing migration rather than just deleting files.
migration file(s), and **every snapshot file** for the entity (the table
snapshot **and each stored-proc/function `.sql`** — your project:
`db/schema/stored-procedures/`; template: `schema/{tables,functions}/`).
Removing snapshots without removing the DB objects (or vice versa) fails the
contract test. If a deployed DB already has the objects, write a `DROP`-ing
migration rather than just deleting files.
5. **Types** — delete `packages/types/src/<entity>.ts` and its line in `index.ts`.
6. **Sweep** — re-run the grep. Zero hits for the entity name (outside git
history) means clean. Then run the full verify stack — a dangling import or a
stale schema snapshot will surface there.

> **Watch for shared/child references.** `issues` has a child `issue_comments`
> (its own table, `fn_issue_comments_list`, `usp_issue_comment_create`, and a
> detail page) — removing `issues` means removing those too. Conversely, don't
> delete something a *surviving* feature still imports (e.g. a shared formatter).
> (its own table, its own `issue_comments_*` procs, and a detail page) — removing
> `issues` means removing those too. Conversely, don't delete something a
> *surviving* feature still imports (e.g. a shared formatter).

---

Expand All @@ -172,12 +190,14 @@ transient broken states). Grep first (see above), then walk the map in reverse:
For each layer in the map, confirm **present + consistent**:

- **Schema is the single source of truth.** The `<Entity>Table` interface
(`database.ts`), the SQL function output, and the frontend form all trace back
to the one `@app/types` schema. No re-spelled "form schema."
- **Functions-only.** No `selectFrom` / raw table access — reads go through
`fn_*`, writes through `usp_*`.
- **Snapshots match the DB.** Every `fn_`/`usp_` has a `schema/functions/` file;
the contract test enforces it.
(`database.ts`), the read proc/function's output, and the frontend form all
trace back to the one `@app/types` schema. No re-spelled "form schema."
- **Procedures/functions-only.** No `selectFrom` / raw table access — every read
and write goes through a stored procedure (or function). *(Follow this project's
existing proc naming/conventions — they don't use `usp_`/`fn_` prefixes.)*
- **Snapshots match the DB.** Every stored proc / function has a matching `.sql`
snapshot (your project: `db/schema/stored-procedures/`; template:
`schema/functions/`); the contract test enforces it.
- **Errors flow to the log.** Routes `next(err)` rather than swallowing — 5xx
must land in the error log / `/errors` page.
- **No `<table>`, no bare forms.** Lists use `<DataTable>`; forms use `<AppForm>`.
Expand Down Expand Up @@ -214,11 +234,13 @@ test for UI the suite can't cover.

## SQL Server specifics (this flavor's data layer)

When writing migrations/functions, T-SQL conventions differ from Postgres:
When writing migrations / stored procs, T-SQL conventions differ from Postgres:

- Writes are **stored procedures** called via `EXEC`, returning the new id as a
`value` column; reads are **inline table-valued functions**
(`SELECT * FROM dbo.fn_<entity>_get(@id)`).
- Reads **and** writes go through **stored procedures** (the occasional function
aside). How they're invoked, how ids/rows come back, and how they're named are
**TBD — determine them from this project's existing procs** (e.g.
`crd.<entity>_<action>`, no `usp_`/`fn_` prefix); see the `mssql-function-authoring`
skill.
- UUID PKs: `uniqueidentifier DEFAULT NEWID()`. Timestamps: `datetimeoffset(7)`
defaulting to `SYSUTCDATETIME()` (UTC). JSON columns: `nvarchar(max)` with an
`ISJSON` check.
Expand Down
Loading
Loading