chore: prettier format audit report markdown
Lint-staged reformat after the previous commit added the file. Same content, prettier's preferred line wrap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -117,6 +117,7 @@ Right border draws on full-width-stacked element instead of bottom separator.
|
||||
**Fix:** `border-b sm:border-r border-r-0`.
|
||||
|
||||
**C3-C7. 5 tap-target violations below WCAG 44×44px minimum**
|
||||
|
||||
- C3: chevron expand button (`folder-tree-sidebar.tsx:125`) — 20×20px
|
||||
- C4: row expand chevron (`documents-hub.tsx:210-216`) — no sizing
|
||||
- C5: "view signing details" (`entity-folder-view.tsx:82-89`) — ~20px tall
|
||||
@@ -167,36 +168,43 @@ v2 distinguishes Decline (recipient refuses) from Reject (admin cancels). The sw
|
||||
Listed by audit domain. Each has a file:line ref in its source audit; I'll quote the highlights here for triage.
|
||||
|
||||
### Security
|
||||
|
||||
- **`storagePath` + `storageBucket` exposed via aggregated files API** (`files.ts:533-534`) — internal storage paths reach authenticated rep clients via `GET /api/v1/files?entityType=X`. Auditors flagged this from both Security and Integration angles. Sanitize at service layer.
|
||||
- **Missing `portId` on UPDATE in folder-move route** (`api/v1/documents/[id]/folder/route.ts:41-44`) — pre-flight read scopes by portId so no current exploit, but defense-in-depth gap that breaks if pre-flight is ever refactored.
|
||||
- **Signer emails exposed to all `documents.view` holders** — confirm with product whether read-only roles should see signatory email addresses or get them redacted.
|
||||
|
||||
### Database / Migration
|
||||
|
||||
- **`uniq_document_folders_entity` doesn't cover `entity_type = NULL`** — rows with NULL entity_type but non-NULL entity_id can duplicate. Closes when CHECK constraint is tightened (A5 above).
|
||||
- **Backfill transaction holds advisory lock across N `ensureEntityFolder` calls** — at 10k files the lock is held for minutes. Batch in chunks of 500.
|
||||
- **`CREATE INDEX` without `CONCURRENTLY`** in migration 0051 — blocks writes briefly. Quantify: short-duration on small tables, moderate on prod-sized. Split for zero-downtime if needed.
|
||||
|
||||
### Concurrency / Error Paths
|
||||
|
||||
- **Storage blob orphaned on DB-insert failure** in `handleDocumentCompleted` — `storage.put` before `db.insert(files)`. No janitor. Long-standing tradeoff; document explicitly.
|
||||
- **`ensureSystemRoots`/`ensureEntityFolder` outside backfill transaction** — folder rows persist if the wrapping tx rolls back. Idempotent so re-run heals.
|
||||
- **`syncEntityFolderName` 50-attempt cap with concurrent renames to same target** — silent log + stale folder name. Accepted divergence.
|
||||
|
||||
### Performance
|
||||
|
||||
- **N+1 grows with linked entities** — leasing company with 50 yachts = 110 queries per page load. Worst case (5 companies + 100 yachts) = 216. Acceptable for now; future optimization: single CTE with grouping.
|
||||
- **Count queries can collapse via window function** — `count(*) OVER ()` halves round-trip count at scale.
|
||||
- **Missing composite indexes `(port_id, client_id)` / `(port_id, company_id)` / `(port_id, yacht_id)` on `files`** — same for `documents`. Add before prod backfill at scale.
|
||||
- **`listDocuments` calls `listTree()` twice when `includeDescendants=true`** — pass already-fetched tree into `hydrateDocumentsWithDownloadUrl`.
|
||||
|
||||
### Data migration (importer)
|
||||
|
||||
- **System-root collision risk** — bucket folders named `Clients`/`Companies`/`Yachts` silently merge into auto-created system roots. Add a pre-flight check that warns when any top-level segment matches a system root name.
|
||||
|
||||
### Observability
|
||||
|
||||
- **Archive/restore hooks missing `portId` in log context** (`companies.service.ts:215`, `yachts.service.ts:193`) — clients has it; companies and yachts don't.
|
||||
- **Backfill CLI has no row-count telemetry** — only "Backfill complete" on success. Want files-processed / folders-created / FKs-propagated counts.
|
||||
- **No log on empty aggregated projection** — `assertEntityInPort` returning false produces a silent empty result. Log warn with `portId + entityType + entityId`.
|
||||
- **`handleDocumentCompleted` outer catch loses `portId`** (line 1197).
|
||||
|
||||
### UI/UX
|
||||
|
||||
- **Em-dash in `SigningDetailsDialog` description** (line 62) — user-facing copy.
|
||||
- **Em-dashes baked into aggregated group labels** (`FROM COMPANY — ACME CORP`) — rendered on every entity folder view. `files.ts:335`, `documents.service.ts:1877`. Replace with colon or slash.
|
||||
- **Mixed `Loading...` (ASCII) and `Loading…` (Unicode ellipsis)** across components. Normalize.
|
||||
@@ -204,6 +212,7 @@ Listed by audit domain. Each has a file:line ref in its source audit; I'll quote
|
||||
- **"view signing details" button too subtle** — inline-text in a tight muted cluster, blends into the date. Consider `<Button variant="ghost" size="sm">`.
|
||||
|
||||
### Integration conformance (with v1 + v2 support)
|
||||
|
||||
- **Documenso poll worker double-fire of `handleDocumentCompleted`** writes a second blob + second `files` row and overwrites `signedFileId`. Confirmed by both concurrency and integration audits. Resolved by A1's idempotency gate.
|
||||
- **Poll worker omits `portId`** when calling `handleRecipientSigned` / `handleDocumentCompleted` — multi-port correctness risk.
|
||||
- **MinIO operations have no socket timeout** — TCP blackhole stalls workers indefinitely. `fetchWithTimeout` doesn't cover the minio client's `putObject`/`getObject`. Wrap with an external timeout (`AbortController` or `Promise.race`).
|
||||
@@ -213,27 +222,32 @@ Listed by audit domain. Each has a file:line ref in its source audit; I'll quote
|
||||
- **`RECIPIENT_VIEWED` / `RECIPIENT_SIGNED`** v2 event aliases — currently silently dropped. Confirm whether v2 actually fires these or maps to `DOCUMENT_OPENED` / `DOCUMENT_SIGNED` like v1. If v2 fires them, add handlers.
|
||||
|
||||
### Realtime / Socket.IO
|
||||
|
||||
- **`useRealtimeInvalidation` is inside `FlatFolderListing`, not `DocumentsHub`** — torn down when navigating away. Lifting to DocumentsHub closes this and unblocks A2 cleanly.
|
||||
- **`['document-folders']` query key has no realtime invalidation path** — rep B renaming a folder takes up to 30s `staleTime` to surface for rep A. Add a folder-rename socket emit + invalidate.
|
||||
|
||||
### Audit log completeness
|
||||
|
||||
- **`createFolder` has no audit log** (line 102-136) — inconsistent with rename/move/delete which all audit.
|
||||
- **`handleDocumentCompleted` file insert has no audit** (line 1163-1180) — signed PDFs created with no audit trail.
|
||||
- **`syncEntityFolderName` ignores `_userId`** — folder renames driven by entity rename leave no audit trail.
|
||||
- **Archive/restore suffix helpers no audit** — parent entity action audits, but folder mutation doesn't.
|
||||
|
||||
### Type-safety
|
||||
|
||||
- **`entityType as 'client'|'company'|'yacht'`** in `documents-hub.tsx:134` — no runtime guard. Fix with `ENTITY_TYPES.has()`.
|
||||
- **`INFLIGHT_STATUSES as unknown as string[]`** — replace with `[...INFLIGHT_STATUSES]`.
|
||||
- **Loose `files?/workflows?` union + unconstrained `T`** in `AggregatedSection` — refactor to discriminated union + `T extends { id: string }`.
|
||||
|
||||
### Test quality
|
||||
|
||||
- **`mapWorkflowStatus` `partially_signed` fix has no regression test**.
|
||||
- **`applyEntityRestoredSuffix` "restore without prior archive" path not tested**.
|
||||
- **`folderId="" → null` validator transform has zero test coverage**.
|
||||
- **`syncEntityFolderName` collision beyond `(2)` untested** — if `isSiblingNameConflict` ever mis-classifies the error shape, retries never fire and the test wouldn't notice.
|
||||
|
||||
### Mobile
|
||||
|
||||
- **DocumentsHub sets no `useMobileChrome`/`setChrome` title** — falls back to URL-segment title-casing.
|
||||
- **FolderActionsMenu trigger overrides to 28×28px** — should use default `size="icon"` (44×44).
|
||||
- **SigningDetailsDialog signer email no `truncate`** — long emails overflow on narrow viewports.
|
||||
@@ -260,31 +274,32 @@ Approximately 30 minor findings across all domains. Highlights:
|
||||
|
||||
## Audit-by-audit completion log
|
||||
|
||||
| # | Audit | Status | Critical | Important | Minor |
|
||||
|---|---|---|---|---|---|
|
||||
| 1 | Security & multi-tenant isolation | ✓ | 0 | 3 | 0 |
|
||||
| 2 | Database & migration safety | ✓ | 1 | 3 | 3 |
|
||||
| 3 | Concurrency, idempotency, error paths | ✓ | 1 | 3 | 3 |
|
||||
| 4 | Performance & query plans | ✓ | 1 | 3 | 3 |
|
||||
| 5 | Data migration from old system | ✓ | 1 | 1 | 3 |
|
||||
| 6 | Production observability | ✓ | 2 | 4 | 3 |
|
||||
| 7 | UI/UX | ✓ | 0 | 5 | 4 |
|
||||
| 8 | Integration conformance (Context7) | ✓ | 0 | 0 | 3 |
|
||||
| 9 | Dependency audit | ✓ | 0 | 0 | ~10 |
|
||||
| 10 | Accessibility (WCAG 2.1 AA) | ✓ | 4 | 5 | 4 |
|
||||
| 11 | Test quality & coverage | ✓ | 2 | 6 | 3 |
|
||||
| 12 | Realtime / Socket.IO | ✓ | 3 | 2 | 1 |
|
||||
| 13 | Audit log completeness | ✓ | 0 | 4 | 4 |
|
||||
| 14 | Type-safety | ✓ | 0 | 3 | 3 |
|
||||
| 15 | Mobile / responsive | ✓ | 6 | 5 | 3 |
|
||||
| 16 | Integration holes (MinIO + Documenso) | ✓ | 2 | 5 | 5 |
|
||||
| 17 | Data structure & sales process completeness | ✓ | 5 | 6 | 6 |
|
||||
| # | Audit | Status | Critical | Important | Minor |
|
||||
| --- | ------------------------------------------- | ------ | -------- | --------- | ----- |
|
||||
| 1 | Security & multi-tenant isolation | ✓ | 0 | 3 | 0 |
|
||||
| 2 | Database & migration safety | ✓ | 1 | 3 | 3 |
|
||||
| 3 | Concurrency, idempotency, error paths | ✓ | 1 | 3 | 3 |
|
||||
| 4 | Performance & query plans | ✓ | 1 | 3 | 3 |
|
||||
| 5 | Data migration from old system | ✓ | 1 | 1 | 3 |
|
||||
| 6 | Production observability | ✓ | 2 | 4 | 3 |
|
||||
| 7 | UI/UX | ✓ | 0 | 5 | 4 |
|
||||
| 8 | Integration conformance (Context7) | ✓ | 0 | 0 | 3 |
|
||||
| 9 | Dependency audit | ✓ | 0 | 0 | ~10 |
|
||||
| 10 | Accessibility (WCAG 2.1 AA) | ✓ | 4 | 5 | 4 |
|
||||
| 11 | Test quality & coverage | ✓ | 2 | 6 | 3 |
|
||||
| 12 | Realtime / Socket.IO | ✓ | 3 | 2 | 1 |
|
||||
| 13 | Audit log completeness | ✓ | 0 | 4 | 4 |
|
||||
| 14 | Type-safety | ✓ | 0 | 3 | 3 |
|
||||
| 15 | Mobile / responsive | ✓ | 6 | 5 | 3 |
|
||||
| 16 | Integration holes (MinIO + Documenso) | ✓ | 2 | 5 | 5 |
|
||||
| 17 | Data structure & sales process completeness | ✓ | 5 | 6 | 6 |
|
||||
|
||||
---
|
||||
|
||||
## Suggested remediation order
|
||||
|
||||
**Pre-merge (block this branch):**
|
||||
|
||||
1. A1 (concurrency idempotency) — 1 line, 5 minutes.
|
||||
2. A2 (realtime hookup) — ~30 min: lift one hook up two layers in component tree.
|
||||
3. A4 (importer folder_id) — 1 line in scripts/import-organized-documents.ts.
|
||||
@@ -297,6 +312,7 @@ Approximately 30 minor findings across all domains. Highlights:
|
||||
10. F1 (DOCUMENT_DECLINED) — add case to switch.
|
||||
|
||||
**Pre-prod cutover (independent of branch):**
|
||||
|
||||
- A3 (LEFT JOIN port_id) — performance fix.
|
||||
- D1 (storage migration table list) — populate TABLES_WITH_STORAGE_KEYS.
|
||||
- D2 (.env.example URL) — strip `/api/v1`.
|
||||
@@ -306,6 +322,7 @@ Approximately 30 minor findings across all domains. Highlights:
|
||||
- DOCUMENSO_API_VERSION documentation + v2 event audit.
|
||||
|
||||
**Post-prod (backlog on main):**
|
||||
|
||||
- Important UI/UX (em-dashes, loading state consistency, status pill on HubRootView).
|
||||
- Important audit log completeness.
|
||||
- Important type-safety tightening.
|
||||
@@ -335,6 +352,7 @@ The dependency, integration-conformance (Context7), and type-safety audits are c
|
||||
The soft-rescue transaction `UPDATE`s `documents.folderId = newParent`, then deletes the folder row. The schema cascade on `files.folderId` is `ON DELETE SET NULL` (not `SET DEFAULT newParent`) — so any files in the deleted folder land at **root**, while documents in the same folder correctly land at the deleted folder's **parent**. A folder containing both will scatter on delete.
|
||||
|
||||
Fix: inside the transaction, between the documents UPDATE and the folder DELETE:
|
||||
|
||||
```ts
|
||||
await tx
|
||||
.update(files)
|
||||
@@ -348,6 +366,7 @@ await tx
|
||||
`scratchpadNotes.linkedClientId references clients.id` with no `onDelete` → defaults to RESTRICT. The hard-delete service nullifies six nullable FKs (files, documents, formSubmissions, emailThreads, reminders, documentSends) but skips `scratchpadNotes`. Any rep who scratchpad-linked a note to a client → hard-delete throws an FK violation and aborts the transaction.
|
||||
|
||||
Fix: add to the nullification block:
|
||||
|
||||
```ts
|
||||
await tx
|
||||
.update(scratchpadNotes)
|
||||
@@ -361,9 +380,11 @@ await tx
|
||||
The unique index `uniq_document_folders_entity` on `(portId, entityType, entityId)` enforces a singleton system folder per entity. Hard-delete removes the client row but does not call `demoteSystemFolderOnEntityDelete`. The folder persists with `systemManaged=true, entityType='client', entityId=<deleted-id>` — invisible in the sidebar but holding the unique slot.
|
||||
|
||||
Fix: after the client delete, fire-and-forget the demote:
|
||||
|
||||
```ts
|
||||
void demoteSystemFolderOnEntityDelete(args.portId, 'client', args.clientId).catch(logger.error);
|
||||
```
|
||||
|
||||
(This is the same wire-up A7 in the main report flagged — confirmed missing on the hard-delete pathway specifically.)
|
||||
|
||||
**G-C4. Five of seven berth-rule triggers are defined but never called**
|
||||
@@ -372,6 +393,7 @@ void demoteSystemFolderOnEntityDelete(args.portId, 'client', args.clientId).catc
|
||||
`DEFAULT_RULES` defines triggers for `eoi_sent`, `eoi_signed`, `deposit_received`, `contract_signed`, `interest_archived`, `interest_completed`, `berth_unlinked`. Only `eoi_sent` and `eoi_signed` are passed to `evaluateRule` anywhere in the codebase.
|
||||
|
||||
Concrete consequences:
|
||||
|
||||
- Deposit received (invoice paid) → no berth state change. Should auto-mark berth as Sold.
|
||||
- Contract signed → no berth state change.
|
||||
- Interest archived → no "berth available" suggestion fires.
|
||||
@@ -379,6 +401,7 @@ Concrete consequences:
|
||||
- Interest unlinked from berth → no rule trigger (off-by-default, but configurable and silently dead).
|
||||
|
||||
Fix sketches:
|
||||
|
||||
- `invoices.ts:741` (after `advanceStageIfBehind('deposit_10pct')`):
|
||||
```ts
|
||||
const { evaluateRule } = await import('@/lib/services/berth-rules-engine');
|
||||
@@ -396,6 +419,7 @@ Fix sketches:
|
||||
Effect: deals stall at whatever stage they hit when the reservation agreement was sent, until a rep manually drags them in the Kanban.
|
||||
|
||||
Fix: in `documents.service.ts`:
|
||||
|
||||
- `sendDocument` pathway (~line 798): if `doc.documentType === 'reservation_agreement'`, fire `advanceStageIfBehind(..., 'contract_sent', meta, 'Reservation agreement sent')`.
|
||||
- `handleDocumentCompleted` (~line 887, where `contractFileId` is set): fire `advanceStageIfBehind(..., 'contract_signed', meta, 'Reservation agreement signed')` and `evaluateRule('contract_signed', ...)`.
|
||||
|
||||
@@ -456,6 +480,7 @@ With Audit 17 folded in, the corrected count is **~28 Critical, ~38 Important, ~
|
||||
### Suggested remediation order — addendum
|
||||
|
||||
After the A/B/C/D/E/F block from the main report:
|
||||
|
||||
1. **G-C1** — files folder UPDATE in `deleteFolderSoftRescue` transaction (1-line addition).
|
||||
2. **G-C2** — nullify `scratchpadNotes.linkedClientId` in `clientHardDelete` (1-line addition).
|
||||
3. **G-C3** — call `demoteSystemFolderOnEntityDelete` after client hard-delete (1-line addition).
|
||||
|
||||
Reference in New Issue
Block a user