# Task 4 Review — Script di generazione PDF (Turso+R2+Puppeteer) Revisore: agent code-reviewer Base: bf7271a — Head: 0f9aab0 --- ## Spec Compliance | Requisito | Stato | Note | |---|---|---| | `app/scripts/pdf/template.ts` creato — porting di `pdf-prototype/template.js` | ✅ | Verificato nel diff | | `app/scripts/pdf/generate.ts` creato | ✅ | | | `app/package.json` aggiornato con `pdf:gen` e devDeps `puppeteer`/`tsx` | ✅ | diff:1419 | | Chiave R2 = `pdf/{codiceFisico}_DVR.pdf` | ✅ | generate.ts:169 | | Nome file locale = `{codiceFisico}_DVR.pdf` | ✅ | generate.ts:182 | | Gemelle: contenuto DVR da `dvrAgenziaId` (resolveDvrAgenziaId), anagrafica/allegati dall'`agenziaId` proprio | ✅ | data.ts:1484/1625 | | `scripts/pdf/*` non importato da nessuna route SvelteKit | ✅ | grep confermato — solo commento in fingerprint.ts | | `headless: true` (non `'new'`) | ✅ | generate.ts:107; report motivato correttamente (Puppeteer 24.x) | | Produzione `data_hash` tramite `hashFingerprintInput(loadFingerprintInput(...))` | ✅ | generate.ts:141–142 | | Upsert `dvr_pdf` stato=`ready` + `error` su eccezione | ✅ | markError() correttamente separato | | Loop `all` continua sugli errori senza fermarsi | ✅ | generate.ts:1854–1858 | | Dual-mode isolato (R2 quando R2 configurato, locale altrimenti) | ✅ | rilevamento a 4 env, nessuna contaminazione | | `PUPPETEER_SKIP_DOWNLOAD=true` su Vercel | ⚠️ non verificabile | da impostare manualmente su Vercel — non in scope diff | --- ## Strengths - **data.ts vs load.js — confronto query per query: fedele.** Ogni query del prototipo (agenzie JOIN lookup, dvr, settings, dvr_pericoli, puntiAlberatura, dvr_valutazione, dvr_valutazione_misure, dvr_incendio_doc, dvr_attivita_miglioramento, agenzie_allegati) è stata portata con le stesse colonne selezionate, gli stessi alias, gli stessi JOIN, lo stesso ordinamento. Il cambio more importante (sincronico `better-sqlite3` → async `@libsql/client`) è realizzato correttamente con await su ogni `client.execute()`. - **Regola gemelle corretta.** `dvrAgenziaId` è usato per tutte le query che partono da `dvr.id` (pericoli, valutazioni, misure, incendio, attività), mentre `agenziaId` proprio è usato per `agenzie` e `agenzie_allegati`. Questo è esattamente il contratto richiesto. - **`data.ts` implementa il proprio `resolveDvrAgenziaId`** anziché importare quello di `$lib/server/dvr` (che richiederebbe il DB Drizzle + `$env`). La scelta è corretta e il commento parallelo su `fingerprint.ts` lo conferma. - **Dual-mode ben isolato.** La condizione `R2_CONFIGURED` (4 env presenti) è valutata una sola volta all'avvio. Il path prod (R2 configurato) non tocca mai `localFileToDataUri` o `pdf-output/`. L'import dinamico di `mkdirSync` nel path locale è stilisticamente insolito ma non crea problemi funzionali. - **template.ts: spot-check sezioni 1–6 + matrice.** Palette, matrice 5×5, LIVELLO_LABELS, CSS, rendering copertina/inquadramento/pericoli/valutazione/incendio/attività/allegati sono identici al prototipo approvato. Le sole differenze rispetto a `pdf-prototype/template.js` sono: (a) `require` → `import`, (b) `module.exports` → `export function renderHtml`, (c) rimozione di `fileToDataUri` e lettura locale del logo — rimpiazzati da `allegato.dataUri` e `data.logoUri`. Zero cambi al layout/CSS/sezioni. - **markError() separato.** La funzione è estratta anziché duplicata — codice più pulito rispetto al template del brief. - **Validazione argomento in main().** `Number.isNaN(id)` controlla l'argomento non-`all`, con exit code 1. Il brief non lo richiedeva esplicitamente. - **Build Vercel non tocca Puppeteer.** Confermato dal grep: l'unica occorrenza della stringa `scripts/pdf` in `src/` è un commento nel file `fingerprint.ts`, non un import. --- ## Issues ### Critical Nessuno. ### Important #### I-1 — `attivita` query usa `dvr.id` ma non controlla che `dvr.id` sia quello della sede master **File:** `app/scripts/pdf/data.ts:1587–1622` La query su `dvr_attivita_miglioramento` usa `dvr.id as number` — che è corretto perché `dvr` viene già caricato con `dvrAgenziaId`. Tutto ok su questo. **Non è un bug — confermato.** #### I-2 — `r2GetBase64` usa `(o.Body as any).transformToByteArray()` — cast `any` su Body **File:** `app/scripts/pdf/generate.ts:110` Il cast `as any` su `o.Body` è necessario perché `@aws-sdk/client-s3` tipizza `Body` come `StreamingBlobPayloadOutputTypes | undefined`, che non espone `transformToByteArray()` nel tipo TypeScript ma lo ha a runtime per Node.js. Funziona correttamente ma sopprime il type-check. Non è un bug funzionale, ma se l'SDK venisse aggiornato e cambiasse l'API di streaming passerebbe inosservato. Sarebbe preferibile: ```ts import { Readable } from 'node:stream'; const chunks: Uint8Array[] = []; for await (const chunk of o.Body as Readable) chunks.push(chunk); const buf = Buffer.concat(chunks); ``` oppure mantenere il cast ma aggiungere un commento esplicativo. **Impatto: zero in produzione oggi, rischio futuro basso.** #### I-3 — `dvr.id` castato a `number` senza verifica: se `dvr` fosse `undefined` crasherebbe silenziosamente **File:** `app/scripts/pdf/data.ts:1514–1516` ```ts if (!dvrRows.length) throw new Error(`DVR mancante per agenzia ${dvrAgenziaId}`); const dvr = dvrRows[0]; ``` La guardia `throw` è presente, quindi `dvr` non è mai `undefined` oltre quella riga. Le successive `args: [dvr.id as number]` sono sicure. **Non è un bug.** ### Minor #### M-1 — `pdf:gen` in `package.json` non passa gli argomenti **File:** `app/package.json:1419` ```json "pdf:gen": "tsx scripts/pdf/generate.ts" ``` Per passare un argomento via `npm run pdf:gen 467` è necessario aggiungere `--`: ```bash npm run pdf:gen -- 467 ``` Il brief richiede `npx tsx scripts/pdf/generate.ts ` che funziona direttamente. Lo script npm è convenienza aggiuntiva ma il doppio trattino potrebbe non essere ovvio. Nessun impatto funzionale — la modalità `npx tsx` diretta funziona sempre. #### M-2 — `import.meta.dirname` richiede TypeScript/tsx con `--moduleResolution bundler` o `node16` **File:** `app/scripts/pdf/generate.ts:1675` `import.meta.dirname` è disponibile da Node.js 21.2.0 / tsx con `verbatimModuleSyntax`. Il report dice "ESM nativo in tsx" — se il `tsconfig.json` del progetto usa `"module": "NodeNext"` o `"bundler"` è ok. Se l'ambiente di CI ha Node < 21.2, `import.meta.dirname` è `undefined` e `REPO_ROOT` verrebbe calcolato come `/../..` (path errato). Da verificare in CI. Basso rischio ma vale una nota. #### M-3 — Il loop `all` stampa il contatore ANCHE se la generazione ha prodotto errore **File:** `app/scripts/pdf/generate.ts:1860` ```ts console.error(`ERR ${id}: ${e}`); // poi: console.log(`-- ${++n}/${total}`); ``` Il counter `-- N/M` viene stampato anche per le agenzie in errore. È accettabile per il monitoraggio (si vede la progressione), ma il log mescolato può essere leggermente confuso. Minor cosmetic. --- ## Assessment **data.ts ↔ load.js:** Confronto query per query completato. Tutte le query sono portate fedelmente: stessi nomi colonna, stessi alias, stessi JOIN, stesso ordinamento. La forma di ritorno è identica al prototipo (campi `agenzia`, `dvr`, `settings`, `pericoli`, `puntiAlberatura`, `valutazioni`, `valutazioniMisure`, `incendioDoc`, `attivita`, `allegati`). La regola gemelle è applicata correttamente. Il campo `src_p`, `src_g`, `src_col_d`, `src_col_e` per il ricalcolo R è presente e corretto. Allegati filtrati su `excluded = 0`. Nessuna colonna mancante rilevata. **generate.ts:** chiave R2 corretta, upsert ready/error corretto, loop all non si ferma su errori, dual-mode non contamina il path prod, `headless: true`. **template.ts:** porting fedele del prototipo approvato v11 — nessuna modifica di layout/CSS/sezioni/matrice. **Bundle Vercel:** confermato clean da grep. **Task quality: Approved** Il task è implementato correttamente. I due Issue Important (I-2, I-3) non sono bug funzionali: I-2 è un cast `any` che funziona a runtime ma sopprime il type-check; I-3 non è in realtà un bug (la guardia è presente). Le due Minor sono osservazioni su robustezza futura e UX del log. Nessun blocco all'uso in produzione.