# Task 3 Review: Derivazione stato PDF **Diff:** 1a594fb..1efae6b **Reviewer:** code-reviewer agent **Date:** 2026-06-24 --- ### Spec Compliance | Requisito | Stato | |---|---| | `PdfState = 'ready' \| 'stale' \| 'generating' \| 'error' \| 'missing'` | ✅ | | `derivePdfState` — firma `(row \| null, currentHash)` | ✅ | | caso `null` → `missing` | ✅ | | caso `row` senza `stato` (null) → `missing` | ✅ | | caso `stato='generating'` → `generating` | ✅ | | caso `stato='error'` → `error` | ✅ | | caso `stato='ready'` + hash uguale → `ready` | ✅ | | caso `stato='ready'` + hash diverso → `stale` | ✅ | | caso `stato='ready'` + `dataHash` null → `missing` | ✅ | | `PdfStatus` shape: `{ state, currentHash, generatoIl, r2Key, errorMsg }` | ✅ | | `getPdfStatus` ritorna `PdfStatus` con `currentHash` da `computeDvrFingerprint` | ✅ | | `markGenerating` fa upsert con `stato='generating'`, `errorMsg=null` | ✅ | | param `_currentHash` deliberatamente non usato nel body | ✅ (atteso, commentato) | | 6 test su `derivePdfState` verbatim dal brief | ✅ | | `npm run check`: 0 errori, 3 warning preesistenti | ✅ | **Tutti i 7 sotto-casi di `derivePdfState` coperti.** Il caso critico `stato='ready'` + `dataHash null` → `missing` è presente sia nel test (`state.test.ts:10`) che nell'implementazione (`state.ts:55`). --- ### Strengths - `derivePdfState` è pura, ben strutturata, leggibile. La cascata di guard clause è chiara. - Il commento su `_currentHash` (`state.ts:89-91`) spiega correttamente il contratto inter-task — utile per chi lavora su Task 4/6. - `env-stub.ts` è minimalista (4 righe) e non cerca di simulare altro. - Il commento inline in `vitest.config.ts` (righe 127-130) avverte esplicitamente che i test su funzioni async con DB devono mockare — buona documentazione della limitazione. - Nessuna logica introdotta fuori spec. --- ### Issues #### Critical _Nessuno._ #### Important **[IMP-1] `getPdfStatus` crea un client libSQL separato (`fpClient()`) inutile — `db` è già disponibile.** - Righe: `state.ts:69-74`, `state.ts:77-78` - Cosa: `fpClient()` istanzia un secondo `createClient` usando `env.DATABASE_URL`. Il `db` importato da `$lib/server/db` è già l'istanza Drizzle configurata con lo stesso URL. `computeDvrFingerprint` accetta un client raw libSQL (non Drizzle), ma questo client è creato ogni chiamata a `getPdfStatus` senza essere distrutto. - Perché è un problema: (a) ogni chiamata apre una nuova connessione TCP verso Turso (nessuna connessione pooling — libSQL client non fa pool automatico in edge); (b) su Vercel serverless questo non è catastrofico ma è spreco; (c) il `db` importato e il `client` di `fpClient()` puntano entrambi allo stesso DB, ma con credenziali costruite a runtime ogni volta — se Task 2 accetta un `db` Drizzle come alternativa, va preferito. - Come: verificare la firma di `computeDvrFingerprint` (Task 2). Se accetta già un'istanza Drizzle o un client condiviso, usare quello. In alternativa, esportare da `$lib/server/db` il client raw e importarlo qui invece di ricrearlo. **[IMP-2] Il `resolve.alias` per `$lib` carica `$lib/server/db` (e quindi il modulo DB) a import-time in ogni test che importa da `./state`.** - Righe: `vitest.config.ts:123-124`, `state.ts:3` (`import { db } from '$lib/server/db'`) - Cosa: il test `state.test.ts` importa solo `derivePdfState` da `./state`, ma `state.ts` in cima importa `db` da `$lib/server/db`. Con l'alias risolto, quel modulo viene caricato a import-time. Se `$lib/server/db` inizializza una connessione DB o legge `env.DATABASE_URL` al top-level, l'import side-effect si esegue anche nei test di funzioni pure. - Perché: rischio concreto di test che aprono connessioni reali a Turso se `DATABASE_URL` è settato nell'ambiente CI/dev, oppure errori silenti se non è settato (il fallback `file:../import-tools/db/dev.db` in `fpClient()` potrebbe o meno esistere). - Come: la soluzione ideale è separare `derivePdfState` (funzione pura) in un file senza import DB, oppure usare `vi.mock('$lib/server/db', ...)` nel test. Non è un refactor obbligatorio ora, ma va notato per i test futuri (Task 4+) che useranno lo stesso pattern. #### Minor **[MIN-1] `markGenerating` non include `createdAt` nell'`insert` iniziale.** - Riga: `state.ts:99` - Cosa: `.values({ agenziaId, stato: 'generating', updatedAt: now })` — se lo schema `dvrPdf` ha una colonna `createdAt` NOT NULL senza default, l'insert fallirà al primo inserimento (non all'upsert update). Non visibile dal diff — dipende dallo schema Task 1. - Come: verificare schema `dvrPdf`; se `createdAt` è NOT NULL senza default, aggiungere `createdAt: now` nel `.values()`. **[MIN-2] `fpClient()` ha fallback hardcoded `'file:../import-tools/db/dev.db'`.** - Riga: `state.ts:71` - Cosa: il path relativo `../import-tools/db/dev.db` funziona solo se il processo è lanciato da `app/`. In Vercel questo fallback è irrilevante (ci sarà `DATABASE_URL`), ma in sviluppo locale potrebbe puntare al path sbagliato se la CWD cambia. - Come: usare `file:dev.db` o usare il client già configurato (vedi IMP-1). **[MIN-3] `env-stub.ts` type-cast `process.env` come `Record`.** - Riga: `env-stub.ts:4` - Cosa: `process.env` è già `NodeJS.ProcessEnv` che estende `Record` — il cast è ridondante ma innocuo. - Come: `export const env = process.env;` è sufficiente e più diretto. --- ### Assessment **Spec compliance: ✅ completa.** Tutti i 7 sotto-casi di `derivePdfState` implementati e testati. Forma di ritorno di `getPdfStatus` corretta. `markGenerating` fa upsert con `errorMsg=null`. Param `_currentHash` gestito correttamente e commentato. **Task quality: Approved.** Il codice è corretto e rispetta il contratto. IMP-1 (client libSQL duplicato) è il finding più rilevante e andrebbe risolto prima che `getPdfStatus` vada in produzione sotto carico, ma non blocca il task né i task dipendenti. IMP-2 (import side-effect DB nei test) è un rischio latente da tenere presente per Task 4+, non un bug attivo. Gli issue Minor sono cosmesi/difensivi.