91 lines
6.9 KiB
Markdown
91 lines
6.9 KiB
Markdown
# Codebase Consistency & Best-Practices Audit — 2026-06-06 (overnight)
|
||
|
||
**Branch:** `chore/codebase-consistency-audit` (off `master` @ v1.9.0). Nothing here is pushed or deployed; for user review in the morning.
|
||
|
||
**Mandate (from user):** scan the whole codebase, find inconsistencies, unify manners/conventions, apply best practices (use context7 for library docs). Edit CLAUDE.md to codify rules. Work only in the project dir. Use many agents.
|
||
|
||
**Operating rules I'm following (self-imposed for safe autonomous work):**
|
||
|
||
- Isolated branch only. Never touch `master`, never push, never deploy, never touch prod/DB.
|
||
- Keep `tsc` (server+client) at 0 and `vitest` green after every change set.
|
||
- Prefer documenting risky/large refactors over auto-applying them. Auto-apply only safe, verified, mechanical unifications.
|
||
- Don't `git add -A`; stage explicit paths.
|
||
|
||
---
|
||
|
||
## 🔴 CRITICAL FINDINGS (need user action)
|
||
|
||
### C1 — v1.9.0 production regression: plan edit-modal delete button missing
|
||
|
||
- **Cause:** `FormModal`'s `footerLeft` / `hideCloseButton` props + modal animations lived only in the user's _uncommitted_ working tree. Committed `PlanCellModal.tsx:225` (and the modal system) depend on `footerLeft`. Releases build from committed code (WIP stashed), so v1.9.0 shipped a `FormModal` without the footer-left slot → the **delete button in the plan entry/override edit modal does not render in production**. Also caused a committed-code `tsc` error (TS2322).
|
||
- **Impact:** plan entry/override deletion via the edit modal is broken in prod v1.9.0. Category management is NOT affected (it uses `hideFooter` + in-body delete). Minor cosmetic: modal entry animation, `hideCloseButton`.
|
||
- **Fix:** committed the modal enhancements (`86e4cbf` on this branch). **Needs a patch release (v1.9.1) to fix prod.**
|
||
- **Status:** fixed on branch; prod patch pending user.
|
||
|
||
### C2 — test suite was red on master/v1.9.0 (2 failing auth tests)
|
||
|
||
- **Cause:** the JWT log-noise fix (`88c9b7c`) stopped logging expected invalid/expired tokens, but `auth.service.test.ts` still asserted it logged. The wrong auth test file (`auth.test.ts`) was run when that change landed, so 2 failing tests shipped on master.
|
||
- **Fix:** updated the tests to assert null + no-log for `JsonWebTokenError` cases (`55a2c50`). Suite back to **134/134**.
|
||
- **Status:** fixed on branch. (No runtime impact; CI/test hygiene + would also be cleared by the v1.9.1 patch.)
|
||
|
||
---
|
||
|
||
## Findings log
|
||
|
||
(Discovery sweep results appended below as agents report. Severity: critical/high/medium/low. Fix class: safe-auto / needs-judgment / manual.)
|
||
|
||
## Changes applied on this branch (all verified: server+client tsc 0, vitest 134, build 0)
|
||
|
||
Correctness / bugs:
|
||
|
||
- `86e4cbf` **C1** modal `footerLeft`/`hideCloseButton` enhancements committed (fixes v1.9.0 regression + tsc error).
|
||
- `55a2c50` **C2** fix 2 failing `verifyAccessToken` tests (red on master).
|
||
- `379725a` **H1/H4** `isAdminLike` reads `roleName` not `role` (admins were self-scoped on /entries,/overrides); dropped 4 stale `as any` category casts.
|
||
- `c5f986a` **H5** corrected + unified audit `entity_type` label map (was showing raw English for invoice/order/quotation/trip/vehicle + all warehouse\_\*).
|
||
- `1f05ef6` date: `todayLocalStr()` replaces 11 UTC-`today` defaults (off-by-one in post-midnight Prague).
|
||
- `744baec` security: audit-log session termination (single + all-others); new `session` EntityType.
|
||
|
||
Consistency / cleanup:
|
||
|
||
- `9ae49c0` parseId (trips/sessions), broad `["offers"]` invalidation, Czech messages (attendance/orders/offers/project-files), Zod 4 `z.strictObject`.
|
||
- `522afef` undefined `--danger-color`→`--danger`, dedup `formatDate` + `require2FAOptions`, removed dead `systemSettingsOptions`, dropped double `plan.css` import.
|
||
- `9c582e1` **H2/H3** log the 30 silent NAS filesystem catches (kept 3 ENOENT-expected as deliberate no-logs).
|
||
|
||
Docs:
|
||
|
||
- `94a9fb8` CLAUDE.md: codified "Conventions (enforced)" section; corrected stale known-issues #4 (NAS) and #5 (sanitization is in place, not a gap).
|
||
- `58bf034`/`0559664` this report + full 84-finding catalog (`codebase-audit-findings-2026-06-06.md`).
|
||
|
||
## Remaining recommendations (NOT auto-applied — for your review)
|
||
|
||
Deliberately left for review (too broad/behavior-risky to apply unattended, or low value):
|
||
|
||
- **`reply.send` → helpers** (medium): ~1/3 of route files use raw `reply.send({success:true,...})` instead of `success()/paginated()`. Mechanical but touches many files + response-shape sensitive — convert per-file when touched. (List in findings catalog, dimension `routes`.)
|
||
- **Zod coercion helpers** (medium): the number/nullable-number/boolean-from-form coercions are copy-pasted ~150×; extract to `src/schemas/common.ts` (with a `NaN` guard — current coercion silently yields `NaN`). Big mechanical sweep — do with review.
|
||
- **console.\* → request logger** (medium): services log via `console.*` (bypasses pino/log-level). Architectural; needs a service-logging strategy.
|
||
- **`markOverdueInvoices`** (medium): swallows the DB-update failure and returns void, so the route treats a failed update as success — surface the error. Also it compares a `@db.Date` due_date against full `new Date()` (flips "overdue" on the due day) — compare date-only.
|
||
- **Dead exports** (low): `previewPattern()` (numbering.service), `usersQuery`/`planKeys.users()` (lib/queries/plan) — unused API surface; remove or wire up.
|
||
- **`ShiftFormModal`** (low): inline styles use `--danger-color`/`--success-color`/etc. (undefined; rely on fallbacks) — switch to the `--danger`/`--success` design tokens.
|
||
- **`.env.example` drift** (low): out of sync with vars read in `config/env.ts` — regenerate.
|
||
- **`@/*` path alias** (low): declared in tsconfig but not wired into Vite/vitest — either wire it up or remove.
|
||
- **warehouse soft-delete test** (low): a test named "soft delete" asserts a hard delete its own comment flags as buggy — clarify intent.
|
||
- **TOTP verify** doesn't increment failed-login counter (relies on rate limit); **HSTS** lacks `preload` — security hardening, low.
|
||
|
||
Full detail + file:line for every item: `docs/codebase-audit-findings-2026-06-06.md`.
|
||
|
||
## Progress tracker
|
||
|
||
- [x] Branch + clean baseline (tsc server 0 / client 0; vitest 134).
|
||
- [x] C1 modal regression + C2 red tests found & fixed (134/134).
|
||
- [x] Discovery sweep (16 parallel dimensions, 84 findings).
|
||
- [x] Synthesis + prioritization (high + safe-auto).
|
||
- [x] Safe unifications applied (12 commits, each verified green).
|
||
- [x] CLAUDE.md updated with codified conventions.
|
||
- [x] Remaining items documented as a review roadmap.
|
||
- [x] Final verification.
|
||
|
||
## ⚠️ Action for you in the morning
|
||
|
||
- **v1.9.1 patch:** C1 (plan edit-modal delete button) + C2 (red tests) affect production v1.9.0. Merge this branch (or cherry-pick `86e4cbf`,`55a2c50`,`379725a`,`c5f986a`) and cut a patch.
|
||
- Review this branch (`chore/codebase-consistency-audit`, 12 commits) and merge what you like. Nothing here is pushed or deployed.
|