Code improvement after a code review with Codex
parent
2822baf50d
commit
a566a60663
@ -0,0 +1,17 @@
|
||||
# Audit Pack (2026-03-27)
|
||||
|
||||
## Teammate Reports
|
||||
- [software-architect.md](./software-architect.md)
|
||||
- [frontend-solidjs.md](./frontend-solidjs.md)
|
||||
- [backend-rust.md](./backend-rust.md)
|
||||
- [technical-lead.md](./technical-lead.md)
|
||||
- [qa-integration-e2e.md](./qa-integration-e2e.md)
|
||||
- [clarifications-2026-03-28.md](./clarifications-2026-03-28.md)
|
||||
|
||||
## Orchestration Artifacts
|
||||
- [consolidated-refactoring-plan.md](./consolidated-refactoring-plan.md)
|
||||
- [devils-advocate-priorities.md](./devils-advocate-priorities.md)
|
||||
- [implementation-plan.md](./implementation-plan.md)
|
||||
|
||||
## Note
|
||||
Some source teammate reports were marked as partial by their authors, but all delivered outputs have been captured and saved here in Markdown form.
|
||||
@ -0,0 +1,75 @@
|
||||
# Backend Rust Audit Report (Partial)
|
||||
|
||||
## Scope and limitations
|
||||
- Audited backend docs + code paths in:
|
||||
- `handlers/`, `services/`, `db/`, `models/`, migrations, representative tests
|
||||
- Limitation:
|
||||
- Full backend test execution could not be completed due persistent Cargo artifact lock after interruption.
|
||||
|
||||
## Clarification Questions
|
||||
1. Is "LLM vs HTML source-link extraction mode" still a product requirement, or intentionally removed?
|
||||
- Docs still mention configurability (`functional_specs.md:140`)
|
||||
- Related settings were removed in migrations (`20260326000026_remove_use_llm_for_source_links.sql`, `20260325000018_drop_deprecated_settings.sql`)
|
||||
2. What are canonical theme defaults: docs/DB (`max_items=4`, `summary_length=3`) or handler (`5`, `2`)?
|
||||
|
||||
## Assumptions
|
||||
- Docs are source of truth unless explicitly superseded by migration intent.
|
||||
- Multi-theme source scoping is mandatory.
|
||||
|
||||
## Prioritized Findings
|
||||
|
||||
### P0
|
||||
- Theme-scoped source import/export contract is broken.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/sources.rs:111`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/sources.rs:154`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/sources.rs:234`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:120`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/sources.rs:249`
|
||||
- Spec refs: `/Users/oabrivard/Projects/rust/ai_synth/docs/requirements.md:19`, `/Users/oabrivard/Projects/rust/ai_synth/docs/functional_specs.md:35`
|
||||
- Direction: make import/export strictly theme-aware.
|
||||
|
||||
### P1
|
||||
- Theme update endpoint lacks validation; invalid settings can persist.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/models/theme.rs:77`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/themes.rs:72`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/db/themes.rs:72`
|
||||
- Direction: add `UpdateThemeRequest::validate()` and enforce in handler.
|
||||
|
||||
- Source create/import path does not verify theme ownership.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/sources.rs:72`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/db/sources.rs:55`
|
||||
- Direction: enforce `theme_id` ownership check before insert/update.
|
||||
|
||||
- Theme creation contract drifts from documented behavior/defaults.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/models/theme.rs:43`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/models/theme.rs:47`, `/Users/oabrivard/Projects/rust/ai_synth/docs/functional_specs.md:90`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/themes.rs:56`, `/Users/oabrivard/Projects/rust/ai_synth/docs/functional_specs.md:185`
|
||||
- Direction: align create validation + defaults with agreed product contract.
|
||||
|
||||
### P2
|
||||
- Core synthesis orchestration is monolithic and high-risk to change.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:85`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:152`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:357`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:542`
|
||||
- Direction: split into composable phase modules.
|
||||
|
||||
- Scheduler reliability path is under-tested.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scheduler.rs:94`
|
||||
|
||||
- Phase-2 filtering performs per-URL DB checks (N+1 pattern).
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:1031`
|
||||
|
||||
### P3
|
||||
- Silent error suppression (`.ok()`) hides operational issues.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:351`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:457`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:604`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scheduler.rs:67`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scheduler.rs:85`
|
||||
|
||||
## Idiomatic Rust Assessment
|
||||
- Error handling: good (`AppError`, `Result`, minimal panics).
|
||||
- Layering: mostly good, but orchestration service is overloaded.
|
||||
- Async/concurrency: good use of `JoinSet`, `watch`, `AtomicBool`.
|
||||
- Testability: strong in many modules; weaker around scheduler/autonomous workflows.
|
||||
|
||||
## Refactoring Plan
|
||||
1. Correctness first (1 sprint).
|
||||
- Theme-aware import/export, theme ownership validation, update validation, defaults alignment.
|
||||
2. Pipeline decomposition (1-2 sprints).
|
||||
- Split `run_generation_inner` into phase modules and explicit state contexts.
|
||||
3. Reliability and QA hardening (1 sprint).
|
||||
- Scheduler integration tests + SSE progress integration tests.
|
||||
4. Performance cleanup (incremental).
|
||||
- Batch article-history checks and optimize bulk inserts.
|
||||
|
||||
## Quick Wins
|
||||
1. Add theme ownership guard in `POST /sources`.
|
||||
2. Add update validation for themes.
|
||||
3. Wire `theme_id` through bulk/csv import.
|
||||
4. Make `export-csv` honor `theme_id`.
|
||||
@ -0,0 +1,30 @@
|
||||
# Clarifications Resolved (2026-03-28)
|
||||
|
||||
This memo records product/contract decisions provided after the initial audit.
|
||||
|
||||
## Final Decisions
|
||||
1. Theme creation:
|
||||
- Creating a theme is allowed without sending categories.
|
||||
- Default categories are `Divers` and `Sans date`.
|
||||
- Category creation requires an existing theme.
|
||||
2. Schedule API contract:
|
||||
- `GET /themes/{id}/schedule` returns `200` with `null` when absent.
|
||||
3. Canonical theme defaults:
|
||||
- `max_items_per_category = 4`
|
||||
- `summary_length = 3`
|
||||
4. Preferred source scope:
|
||||
- Preferred sources are managed per theme.
|
||||
5. Import/export scope:
|
||||
- Bulk import, CSV import, and CSV export are always bound to the selected theme.
|
||||
6. Fallback category label:
|
||||
- Canonical label is `Divers`.
|
||||
7. Source-link extraction mode:
|
||||
- Fully deprecated.
|
||||
8. Frontend UI guideline:
|
||||
- Using `Button` component instead of raw `<button>` is strict everywhere.
|
||||
9. QA release gate:
|
||||
- Deterministic coverage is required in CI before release (scheduler/SSE and critical paths).
|
||||
|
||||
## Implementation Impact
|
||||
- Contract drift items flagged by the audit are now actionable with no remaining ambiguity.
|
||||
- Follow-up refactoring/implementation teams should treat this memo as the tie-breaker over older conflicting docs.
|
||||
@ -0,0 +1,74 @@
|
||||
# Consolidated Refactoring Plan (From 5-Agent Audit)
|
||||
|
||||
## Inputs
|
||||
- Software architect report
|
||||
- Frontend SolidJS report
|
||||
- Backend Rust report
|
||||
- Technical lead report
|
||||
- QA integration/E2E report
|
||||
|
||||
## Clarifications Required Before Implementation
|
||||
1. Theme creation contract:
|
||||
- Draft allowed with empty topic/categories, or mandatory valid values on create?
|
||||
2. Schedule GET contract:
|
||||
- `404` when absent, or `200` with `null`?
|
||||
3. Canonical theme defaults:
|
||||
- `max_items_per_category`/`summary_length` should be 4/3 or 5/2?
|
||||
4. Theme scoping policy:
|
||||
- Are import/export/preferred operations always theme-scoped?
|
||||
5. Terminology:
|
||||
- Fallback category label (`Autre` vs `Divers`)
|
||||
- Source-link extraction mode still required or deprecated
|
||||
|
||||
## Executive Summary
|
||||
- The codebase has good foundations (clear layering intent, provider abstraction, strong backend unit coverage), but there is significant **spec/implementation drift** in theme/schedule/source contracts.
|
||||
- Highest risk is correctness, not style: multi-theme data scoping and contract mismatches can break core user flows.
|
||||
- Complexity is concentrated in a few large files; decomposition should happen only after correctness and reliability fixes.
|
||||
|
||||
## Priority Backlog
|
||||
|
||||
## P0 (Immediate)
|
||||
1. Fix theme-scoped source correctness:
|
||||
- Ensure bulk/CSV import preserves `theme_id`
|
||||
- Scope preferred-source updates by theme
|
||||
- Make CSV export theme-aware
|
||||
2. Fix theme creation flow contract mismatch:
|
||||
- Align UI create payload and backend validation
|
||||
3. Add tests for P0 flows:
|
||||
- Multi-theme isolation integration tests
|
||||
- End-to-end create theme happy path
|
||||
|
||||
## P1 (Next)
|
||||
1. Unify schedule API contract across docs/backend/frontend/tests.
|
||||
2. Align theme defaults across migration/backend/frontend/docs.
|
||||
3. Harden scheduler reliability:
|
||||
- deterministic integration tests
|
||||
- timeout parity with manual generation
|
||||
4. Add deterministic SSE progress integration tests.
|
||||
5. Add Brave fallback/rate-limit/max-age pipeline integration coverage.
|
||||
|
||||
## P2 (Then)
|
||||
1. Decompose backend synthesis pipeline module by phase.
|
||||
2. Decompose large frontend pages (`ThemeManager`, `Providers`, `Settings`) into hooks/components.
|
||||
3. Remove direct `fetch` in pages; enforce API client boundary.
|
||||
4. Remove hardcoded UI strings; enforce i18n conventions.
|
||||
5. Replace silent `.ok()` error swallowing with warn/metric patterns.
|
||||
|
||||
## P3 (Opportunistic)
|
||||
1. Convention enforcement via lint/codemods.
|
||||
2. Clean reactive anti-patterns (render-time side effects).
|
||||
3. Performance optimizations (N+1 phase-2 checks, set-based inserts).
|
||||
|
||||
## Suggested Delivery Sequence (For Follow-up Team)
|
||||
1. Decision sprint (0.5-1 day): finalize 5 open clarifications.
|
||||
2. Correctness sprint (1-2 weeks): all P0 + tests merged.
|
||||
3. Reliability sprint (1 week): scheduler/SSE/fallback/rate-limit/max-age tests + runtime parity fixes.
|
||||
4. Simplification sprint (1-2 weeks): decomposition of largest backend/frontend hotspots.
|
||||
5. Governance sprint (2-3 days): lint/conformance checks to prevent drift.
|
||||
|
||||
## Acceptance Criteria
|
||||
1. No spec drift for theme/schedule/source contracts (tests and docs aligned).
|
||||
2. Multi-theme operations fully isolated and verifiably correct.
|
||||
3. Scheduled generation reliability covered by deterministic integration tests.
|
||||
4. SSE and fallback branches have deterministic coverage in CI.
|
||||
5. Largest hotspots reduced in size and complexity with no behavior regression.
|
||||
@ -0,0 +1,36 @@
|
||||
# Devil's Advocate Prioritization Memo
|
||||
|
||||
## Position
|
||||
Do not start with "clean architecture" refactors yet. First, remove business-risk mismatches that can produce incorrect behavior despite clean code.
|
||||
|
||||
## What Should Be Prioritized in This Conversation
|
||||
1. Decide the product contracts that are currently ambiguous.
|
||||
- Theme creation rules
|
||||
- Schedule GET semantics
|
||||
- Canonical defaults
|
||||
- Theme scoping for import/export/preferred
|
||||
2. Approve a short "correctness-first" implementation batch.
|
||||
- Theme-scoped source import/export/preferred fixes
|
||||
- Theme creation contract alignment
|
||||
- Theme ownership validation on source writes
|
||||
3. Lock a test gating policy before refactoring.
|
||||
- Add deterministic tests for scheduler and SSE progress first
|
||||
- Avoid relying on live external-provider E2E for CI confidence
|
||||
|
||||
## What To Defer (For Now)
|
||||
1. Large structural decomposition of `synthesis.rs` and major frontend pages.
|
||||
2. Broad style/lint cleanups.
|
||||
3. Performance tuning beyond obvious correctness-adjacent fixes.
|
||||
|
||||
## Why This Order Is Safer
|
||||
1. It prevents regressions in core multi-theme behavior before code motion.
|
||||
2. It creates a stable contract baseline for any future refactor team.
|
||||
3. It avoids spending effort simplifying code that may be simplified in the wrong direction if contracts change.
|
||||
|
||||
## Concrete Next Decision Request
|
||||
Please confirm these five items so implementation teams can proceed without churn:
|
||||
1. Draft theme creation allowed or not
|
||||
2. Schedule GET absent behavior (`404` vs `200 null`)
|
||||
3. Defaults (`4/3` vs `5/2`)
|
||||
4. Theme-scoped import/export/preferred policy (yes/no)
|
||||
5. Fallback label and source-link extraction mode policy
|
||||
@ -0,0 +1,59 @@
|
||||
# Frontend Audit Report (Partial)
|
||||
|
||||
## Scope and limits
|
||||
- Audited `frontend/src` against:
|
||||
- `/Users/oabrivard/Projects/rust/ai_synth/docs/requirements.md`
|
||||
- `/Users/oabrivard/Projects/rust/ai_synth/docs/functional_specs.md`
|
||||
- `/Users/oabrivard/Projects/rust/ai_synth/docs/technical_specs.md`
|
||||
- `/Users/oabrivard/Projects/rust/ai_synth/docs/dev_guidelines.md`
|
||||
- Validation:
|
||||
- `tsc --noEmit` passed
|
||||
- `vitest` blocked by missing optional package `@rollup/rollup-darwin-x64`
|
||||
|
||||
## Clarification Questions
|
||||
1. Should fallback category label be `Autre` (functional spec) or `Divers` (current behavior)?
|
||||
2. Is the "use `Button` component instead of raw `<button>`" rule strict or only for primary actions?
|
||||
|
||||
## Prioritized Findings
|
||||
|
||||
### P1
|
||||
- Article history filter values are out of contract with documented statuses/source types.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/ArticleHistory.tsx:18`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/ArticleHistory.tsx:28`, `/Users/oabrivard/Projects/rust/ai_synth/docs/technical_specs.md:249`, `/Users/oabrivard/Projects/rust/ai_synth/docs/technical_specs.md:251`
|
||||
- Direction: centralize allowed enums and mirror backend contract.
|
||||
|
||||
- i18n guideline is violated by hardcoded French strings in JSX/logic.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/ArticleHistory.tsx:140`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/ArticleHistory.tsx:308`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/components/settings/SettingsBraveSearch.tsx:58`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/components/ApiKeyManager.tsx:132`, `/Users/oabrivard/Projects/rust/ai_synth/docs/dev_guidelines.md:196`
|
||||
- Direction: move all user-facing strings to `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/i18n/fr.ts`.
|
||||
|
||||
- SSE lifecycle cleanup is fragile and may leave open connections.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/GenerateSynthesis.tsx:225`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/GenerateSynthesis.tsx:241`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/GenerateSynthesis.tsx:251`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/utils/sse.ts:156`
|
||||
- Direction: own connection lifecycle in page-level `onCleanup`; keep SSE utility side-effect free.
|
||||
|
||||
### P2
|
||||
- State mutation occurs during render in admin providers.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/admin/Providers.tsx:559`
|
||||
- Direction: initialize edit state in dedicated `createEffect`.
|
||||
|
||||
- Frontend complexity is too high in several page components.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/ThemeManager.tsx`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/admin/Providers.tsx`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/Settings.tsx`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/SynthesisDetail.tsx`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/GenerateSynthesis.tsx`
|
||||
- Direction: extract composables/hooks and split large UI sections.
|
||||
|
||||
- API layer is bypassed in places with raw `fetch`.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/GenerateSynthesis.tsx:241`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/api/client.ts:25`
|
||||
- Direction: add typed API method and remove direct fetch from pages.
|
||||
|
||||
### P3
|
||||
- Dead reactive effect and redundant flow in article history.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/ArticleHistory.tsx:91`
|
||||
- Direction: replace with `createResource` keyed by filters/page or remove no-op effect.
|
||||
|
||||
- Style guideline drift (imports, raw button usage).
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/index.tsx:4`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/api/themes.ts:1`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/Home.tsx:196`, `/Users/oabrivard/Projects/rust/ai_synth/docs/dev_guidelines.md:181`, `/Users/oabrivard/Projects/rust/ai_synth/docs/dev_guidelines.md:207`
|
||||
- Direction: add ESLint rules/codemods for import alias and button policy.
|
||||
|
||||
## Immediate High-Impact Refactoring Order
|
||||
1. Fix `ArticleHistory` contract mismatches for enums/labels.
|
||||
2. Normalize i18n usage and remove hardcoded strings.
|
||||
3. Refactor SSE lifecycle ownership in generation flow.
|
||||
4. Split `ThemeManager` and `admin/Providers` into hooks + presentational components.
|
||||
5. Enforce conventions with lint rules.
|
||||
@ -0,0 +1,137 @@
|
||||
# Plan d'implementation des suggestions d'audit
|
||||
|
||||
Date: 2026-03-28
|
||||
Base: audits multi-profils + clarifications produit validees le 2026-03-28.
|
||||
|
||||
## 1. Objectif
|
||||
|
||||
Implementer les recommandations de l'audit en priorisant:
|
||||
1. la correction fonctionnelle (contrats/theme-scoping),
|
||||
2. la fiabilite (scheduler + SSE + coverage CI deterministe),
|
||||
3. la maintenabilite (simplification/refactoring progressif sans regression).
|
||||
|
||||
## 2. Contraintes de reference (deja decidees)
|
||||
|
||||
1. Creation de theme possible sans categories utilisateur.
|
||||
2. Categories par defaut toujours presentes: `Divers`, `Sans date`.
|
||||
3. `GET /themes/{id}/schedule` retourne `200` avec `null` si absent.
|
||||
4. Valeurs par defaut theme: `max_items_per_category=4`, `summary_length=3`.
|
||||
5. Sources et "preferred" strictement scopes par theme.
|
||||
6. Import/export sources toujours lies au theme selectionne.
|
||||
7. Mode d'extraction "LLM vs HTML" deprecie (HTML uniquement).
|
||||
8. Couverture deterministe CI obligatoire avant release pour flows critiques.
|
||||
|
||||
## 3. Sequencement recommande
|
||||
|
||||
## Phase 0 - Stabilisation des contrats (3-5 jours)
|
||||
|
||||
### Backend
|
||||
1. Rendre `theme_id` obligatoire pour create/bulk/import-csv/export-csv/preferred.
|
||||
2. Scoper `PUT /sources/preferred` par `theme_id`.
|
||||
3. Verifier ownership `theme_id` sur tous les writes sources.
|
||||
4. Aligner defaults backend sur `4/3`.
|
||||
5. Aligner validation theme create/update sur les regles finales.
|
||||
|
||||
### Frontend
|
||||
1. Adapter clients API (`sources.ts`, `themes.ts`, `schedules.ts`) aux nouveaux contrats.
|
||||
2. Garantir l'envoi systematique du `theme_id` dans tous les flux sources.
|
||||
3. Aligner creation de theme (draft sans categories utilisateur).
|
||||
4. Gerer explicitement `schedule: null`.
|
||||
|
||||
### QA
|
||||
1. Ajouter tests d'integration contrat API (theme + sources + schedule).
|
||||
2. Ajouter tests d'isolation multi-theme (import/export/preferred).
|
||||
|
||||
### Definition of Done
|
||||
1. Contrats API aligns avec docs.
|
||||
2. Tests integration P0 verts en CI.
|
||||
3. Aucun flux UI principal casse.
|
||||
|
||||
## Phase 1 - Fiabilite runtime + gates CI (4-6 jours)
|
||||
|
||||
### Backend
|
||||
1. Ajouter timeout explicite sur execution scheduler (parite avec generation manuelle).
|
||||
2. Brancher cleanup periodique `job_store.cleanup_expired()` au runtime.
|
||||
3. Renforcer erreurs non silencieuses (`.ok()` critiques -> logs/warn explicites).
|
||||
|
||||
### QA
|
||||
1. Tests d'integration deterministes scheduler:
|
||||
- due selection, anti-double-run, conflit job actif, succes/echec email.
|
||||
2. Tests d'integration deterministes SSE:
|
||||
- sequence `progress -> complete/error`, ownership, resilience.
|
||||
3. Rendre ces suites bloquantes dans CI.
|
||||
|
||||
### Definition of Done
|
||||
1. Scheduler/SSE couverts par tests deterministes.
|
||||
2. Gate CI "release" bloque si ces suites echouent.
|
||||
|
||||
## Phase 2 - Coherence produit et hygiene code (4-6 jours)
|
||||
|
||||
### Frontend
|
||||
1. Corriger enums/filters `ArticleHistory` pour coller au contrat backend.
|
||||
2. Supprimer hardcoded strings (i18n centralise).
|
||||
3. Appliquer regle stricte `Button` (remplacer les `<button>` bruts).
|
||||
4. Eliminer fetch directs dans pages au profit du client API.
|
||||
|
||||
### Backend
|
||||
1. Supprimer toute trace fonctionnelle du mode extraction LLM deprecie.
|
||||
2. Verification complete des defaults/documentation runtime.
|
||||
|
||||
### QA
|
||||
1. Tests unitaires/frontend sur i18n + rendering + article-history filters.
|
||||
2. Tests integration pour cas no-date -> `Sans date`.
|
||||
|
||||
### Definition of Done
|
||||
1. Plus de drift visible entre spec/doc/code pour ces zones.
|
||||
2. Lint/tests frontend/backend verts.
|
||||
|
||||
## Phase 3 - Simplification structurelle (1-2 sprints)
|
||||
|
||||
### Backend
|
||||
1. Decomposer `services/synthesis.rs` par phases:
|
||||
- `init`, `phase1`, `phase2`, `finalize`, `trace`.
|
||||
2. Extraire structures de contexte pour limiter l'etat mutable partage.
|
||||
3. Ajouter tests de caracterisation avant et apres refactor.
|
||||
|
||||
### Frontend
|
||||
1. Decomposer `ThemeManager`, `Settings`, `Admin/Providers`.
|
||||
2. Introduire hooks/metiers (`useThemeEditor`, `useGenerationProgress`, etc.).
|
||||
3. Nettoyer anti-patterns reactivite (side effects en render).
|
||||
|
||||
### Definition of Done
|
||||
1. Complexite par fichier/fonction reduite de facon mesurable.
|
||||
2. Aucun changement comportemental non voulu (tests non-regression verts).
|
||||
|
||||
## Phase 4 - Durcissement final (3-5 jours)
|
||||
|
||||
1. Optimiser points chauds (N+1 `article_history`, insertions bulk).
|
||||
2. Completer observabilite (logs structurés, erreurs pipeline visibles).
|
||||
3. Mettre a jour templates PR/checklists anti-drift docs/spec/code.
|
||||
4. Synchroniser `qa_guidelines` avec inventaire reel genere en CI.
|
||||
|
||||
## 4. Plan QA global (obligatoire)
|
||||
|
||||
1. Gate CI minimal:
|
||||
- backend unit
|
||||
- backend integration (incluant scheduler + SSE + source-scoping)
|
||||
- frontend typecheck
|
||||
- frontend unit tests
|
||||
2. E2E live provider reste non-bloquant (supplementaire).
|
||||
3. Aucun merge sur branches release si gate deterministe rouge.
|
||||
|
||||
## 5. Risques et mitigation
|
||||
|
||||
1. Risque: regression sur flux theme/sources.
|
||||
- Mitigation: tests d'integration multi-theme en premier.
|
||||
2. Risque: refactor structurel trop tot.
|
||||
- Mitigation: refactor uniquement apres Phases 0-2 stabilisees.
|
||||
3. Risque: fausse confiance via tests live externes.
|
||||
- Mitigation: CI bloquee sur suites deterministes internes.
|
||||
|
||||
## 6. Ordre d'execution concret (suggestion)
|
||||
|
||||
1. Sprint A: Phase 0 complete.
|
||||
2. Sprint B: Phase 1 complete.
|
||||
3. Sprint C: Phase 2 complete.
|
||||
4. Sprint D-E: Phase 3.
|
||||
5. Sprint F: Phase 4 + cloture.
|
||||
@ -0,0 +1,151 @@
|
||||
# QA Integration/E2E Audit Report
|
||||
|
||||
Date: 2026-03-27
|
||||
Scope: `docs/requirements.md`, `docs/functional_specs.md`, `docs/technical_specs.md`, `docs/qa_guidelines.md`, `backend/tests`, `e2e/tests`, test scripts.
|
||||
|
||||
## 1) Clarification Questions
|
||||
|
||||
1. Should scheduled execution reliability (background scheduler + email fanout) be release-gated with deterministic integration tests, or only monitored in production?
|
||||
2. Is an external-provider live E2E (`OPENAI_TEST_API_KEY`) acceptable as the *only* end-to-end coverage for SSE/progress completion, or do you want deterministic in-house SSE coverage in CI?
|
||||
|
||||
## 2) Assumptions
|
||||
|
||||
- CI quality gates should not rely on external LLM providers.
|
||||
- Core product requirements (scheduled generation, generation progress, fallback paths) should be covered by deterministic integration tests.
|
||||
- This report prioritizes integration/E2E confidence over unit-test volume.
|
||||
|
||||
## 3) Prioritized Findings (P0-P3)
|
||||
|
||||
### P0 — Scheduled execution path is effectively untested (critical requirement risk)
|
||||
- Why it matters: Scheduled generation + email delivery is a core requirement. Regressions here can silently fail user deliverables.
|
||||
- Evidence:
|
||||
- Scheduler runtime logic exists in [scheduler.rs:27](/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scheduler.rs:27) through [scheduler.rs:91](/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scheduler.rs:91).
|
||||
- Only one trivial scheduler unit test exists ([scheduler.rs:98](/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scheduler.rs:98)).
|
||||
- Requirement explicitly expects scheduled reliability ([requirements.md:136](/Users/oabrivard/Projects/rust/ai_synth/docs/requirements.md:136)).
|
||||
- Direction: Add deterministic integration tests for due schedule selection, double-run prevention (`last_run_at`), job contention behavior, and email send invocation outcomes.
|
||||
|
||||
### P1 — SSE progress endpoint has no deterministic integration coverage
|
||||
- Why it matters: Generation UX and cancellation safety depend on SSE correctness.
|
||||
- Evidence:
|
||||
- SSE handler is implemented in [generation.rs:150](/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/generation.rs:150).
|
||||
- Integration tests validate trigger/duplicate behavior only, not progress stream contract ([api_syntheses_test.rs:565](/Users/oabrivard/Projects/rust/ai_synth/backend/tests/api_syntheses_test.rs:565), [api_syntheses_test.rs:617](/Users/oabrivard/Projects/rust/ai_synth/backend/tests/api_syntheses_test.rs:617)).
|
||||
- Existing live SSE check is gated and external ([generation-live.spec.ts:119](/Users/oabrivard/Projects/rust/ai_synth/e2e/tests/generation-live.spec.ts:119)).
|
||||
- Direction: Add integration tests that subscribe to `/progress`, assert `progress -> complete/error` sequence, ownership enforcement, reconnect semantics, and keepalive stability.
|
||||
|
||||
### P1 — Brave Search fallback path lacks integration coverage
|
||||
- Why it matters: Fallback branch is a key functional path and currently high regression risk.
|
||||
- Evidence:
|
||||
- Brave branch in pipeline code: [synthesis.rs:371](/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:371).
|
||||
- Pipeline test file explicitly states this path is not integration-tested ([pipeline_test.rs:257](/Users/oabrivard/Projects/rust/ai_synth/backend/tests/pipeline_test.rs:257)).
|
||||
- Direction: Add mock HTTP server + encrypted Brave key fixture flow to execute `use_brave_search=true` end-to-end in integration tests.
|
||||
|
||||
### P1 — Pipeline integration does not verify rate-limit behavior
|
||||
- Why it matters: Rate limiting is a non-functional requirement; failures can produce outages or provider bans.
|
||||
- Evidence:
|
||||
- Pipeline tests set user rate limit fields to `null` ([pipeline_test.rs:64](/Users/oabrivard/Projects/rust/ai_synth/backend/tests/pipeline_test.rs:64)).
|
||||
- No integration assertions around rate-limited waits/error propagation.
|
||||
- Direction: Add integration scenarios for strict user/provider limits and verify wait/retry/timeout outcomes.
|
||||
|
||||
### P1 — Pipeline integration does not verify max-age article filtering behavior
|
||||
- Why it matters: Freshness is a core content-quality requirement.
|
||||
- Evidence:
|
||||
- Pipeline tests consistently use high `max_age_days` values ([pipeline_test.rs:77](/Users/oabrivard/Projects/rust/ai_synth/backend/tests/pipeline_test.rs:77)).
|
||||
- No integration assertion for `filtered_too_old` trace behavior.
|
||||
- Direction: Add wiremock articles with old publish dates + assertions on filtering and history status.
|
||||
|
||||
### P2 — E2E suite is heavily API-driven, limited UI journey validation
|
||||
- Why it matters: UI regressions can pass E2E while backend endpoints stay healthy.
|
||||
- Evidence:
|
||||
- Sources and themes E2E use `page.evaluate(fetch(...))` for most operations ([sources.spec.ts:23](/Users/oabrivard/Projects/rust/ai_synth/e2e/tests/sources.spec.ts:23), [themes.spec.ts:29](/Users/oabrivard/Projects/rust/ai_synth/e2e/tests/themes.spec.ts:29)).
|
||||
- Direction: Keep API-assisted setup, but assert critical user interactions through UI (form submit, validation messages, control states).
|
||||
|
||||
### P2 — Article history ownership isolation is not explicitly tested
|
||||
- Why it matters: Multi-user data isolation is security-sensitive.
|
||||
- Evidence:
|
||||
- Current article history integration tests cover auth + empty/clear/provenance 404 only ([api_article_history_test.rs:24](/Users/oabrivard/Projects/rust/ai_synth/backend/tests/api_article_history_test.rs:24)).
|
||||
- Direction: Add user A vs user B cross-access tests for history and provenance endpoints.
|
||||
|
||||
### P2 — QA guidelines are out of sync with current codebase signals
|
||||
- Why it matters: stale test inventory causes false confidence in planning and release gates.
|
||||
- Evidence:
|
||||
- Documented counts/status in [qa_guidelines.md:7](/Users/oabrivard/Projects/rust/ai_synth/docs/qa_guidelines.md:7) to [qa_guidelines.md:11](/Users/oabrivard/Projects/rust/ai_synth/docs/qa_guidelines.md:11).
|
||||
- Current local grep counts: backend unit ~359, backend integration ~187, frontend unit ~135 tests, E2E 7.
|
||||
- Direction: Automate inventory generation in CI and update `docs/qa_guidelines.md` from machine output.
|
||||
|
||||
### P3 — Frontend unit test execution environment is currently brittle
|
||||
- Why it matters: slows QA feedback loop and hides regressions.
|
||||
- Evidence:
|
||||
- Local run `cd frontend && npx vitest run` failed due missing optional Rollup binary (`@rollup/rollup-darwin-x64`).
|
||||
- Direction: Add a clean install/bootstrap check in CI and pin known-good Node/npm workflow.
|
||||
|
||||
## 4) Coverage Map (Required Capability vs Current Coverage)
|
||||
|
||||
| Capability | Unit | Integration | E2E | Status |
|
||||
|---|---|---|---|---|
|
||||
| Auth (register/login/verify/session) | Medium | Strong (`api_auth_test.rs`) | Medium (`registration.spec.ts`) | Good |
|
||||
| Theme CRUD | Low | Strong (`api_themes_test.rs`) | Medium (API-driven) | Good |
|
||||
| Source CRUD/import/export/preferred | Medium | Strong (`api_sources_test.rs`) | Medium (API-driven) | Good |
|
||||
| On-demand generation trigger/duplicate/stop | Medium | Medium (`api_syntheses_test.rs`, `api_stop_generation_test.rs`) | Medium (live test gated) | Partial |
|
||||
| SSE progress stream contract | Low | Weak | Weak (only external live) | Gap |
|
||||
| Pipeline Phase 1 (personalized sources) | Medium | Medium (`pipeline_test.rs`) | Low | Partial |
|
||||
| Pipeline Phase 2 (LLM web search) | Medium | Medium (`pipeline_test.rs`) | Low | Partial |
|
||||
| Pipeline Phase 2 (Brave Search) | Low | None | None | Gap |
|
||||
| Scheduled config CRUD | Low | Medium (`api_schedules_test.rs`) | Medium (API-driven in themes E2E) | Partial |
|
||||
| Scheduled execution runtime | Low | None | None | Gap |
|
||||
| Export email/pdf/markdown | Medium | Strong (`api_export_test.rs`) | Low | Good |
|
||||
| Article history/provenance security | Low | Weak (no ownership isolation) | None | Gap |
|
||||
| Rate limiting in real generation flow | Medium | None | None | Gap |
|
||||
| Date freshness filtering in pipeline | Medium (scraper unit) | None | None | Gap |
|
||||
|
||||
## 5) Test Architecture Issues (Flakiness / Speed / Isolation / Observability)
|
||||
|
||||
- Flakiness risk: `generation-live.spec.ts` depends on external OpenAI availability and behavior ([generation-live.spec.ts:1](/Users/oabrivard/Projects/rust/ai_synth/e2e/tests/generation-live.spec.ts:1)).
|
||||
- Speed tradeoff: E2E is stable-ish due single worker and API-first setup, but this under-tests real UI behavior.
|
||||
- Isolation strengths: backend integration per-test DB isolation via `TestApp` is strong.
|
||||
- Observability gap: no dedicated integration assertions for SSE stream semantics and scheduler outcomes.
|
||||
|
||||
## 6) Detailed QA / Refactoring Plan
|
||||
|
||||
### Phase 1 (1-2 weeks): close highest-risk deterministic gaps
|
||||
- Add scheduler integration suite:
|
||||
- due schedule executes once
|
||||
- `last_run_at` blocks double-run
|
||||
- active manual job causes skip
|
||||
- email send errors are logged and do not crash loop
|
||||
- Add SSE integration suite:
|
||||
- authorized subscribe receives latest event
|
||||
- unauthorized/foreign job denied
|
||||
- `complete` and `error` payload schema checks
|
||||
- Add Brave Search integration path with mocked Brave API and stored encrypted key fixture.
|
||||
|
||||
### Phase 2 (1 week): non-functional policy tests
|
||||
- Add pipeline integration tests for:
|
||||
- `max_age_days` filtering (`filtered_too_old` assertions)
|
||||
- user/provider rate-limit behavior under contention
|
||||
- cancellation mid-batch and partial-save invariants.
|
||||
|
||||
### Phase 3 (1 week): E2E realism upgrades
|
||||
- Convert at least 3 API-heavy E2E scenarios to UI-driven workflows:
|
||||
- theme create/update/delete
|
||||
- source add/import/preferred/delete
|
||||
- schedule form save/delete.
|
||||
- Keep API shortcuts only for setup/cleanup.
|
||||
|
||||
### Phase 4 (2-3 days): documentation and gate hardening
|
||||
- Generate test inventory automatically (counts, pass/fail) and publish into QA docs.
|
||||
- Split CI lanes:
|
||||
- deterministic required lane (unit/integration/mock-e2e)
|
||||
- optional live-provider lane (non-blocking).
|
||||
|
||||
## 7) Quick Wins
|
||||
|
||||
- Add one integration test for `/syntheses/generate/{job_id}/progress` happy path + ownership check.
|
||||
- Add one integration test for scheduled execution `mark_run` behavior using controlled due schedule fixture.
|
||||
- Add one article-history cross-user isolation test.
|
||||
- Mark `generation-live.spec.ts` as non-blocking in CI with explicit label/reporting.
|
||||
- Update `docs/qa_guidelines.md` inventory counts to current observed baseline.
|
||||
|
||||
## Execution Notes
|
||||
|
||||
- Ran successfully: `cd backend && cargo test --lib` -> 359 passed.
|
||||
- Could not execute frontend unit tests due environment dependency issue (`@rollup/rollup-darwin-x64` missing in local `node_modules`).
|
||||
@ -0,0 +1,116 @@
|
||||
# Software Architect Audit Report (March 27, 2026)
|
||||
|
||||
## Clarification Questions
|
||||
1. Should a new theme be creatable with empty categories/topic (draft mode), or must it be valid at creation?
|
||||
2. Should `GET /themes/{id}/schedule` return `404` when absent, or `200` with `null`?
|
||||
3. Which defaults are canonical for themes: DB/spec (`max_items_per_category=4`, `summary_length=3`) or UI/handler (`5`, `2`)?
|
||||
|
||||
## Assumptions
|
||||
- Documentation in `docs/` is the intended target behavior.
|
||||
- This is architecture-focused (entry points, service boundaries, major modules), not a line-by-line audit of all files.
|
||||
|
||||
## Prioritized Findings
|
||||
|
||||
### P0
|
||||
- Theme creation contract is internally inconsistent and likely broken in practice.
|
||||
- Evidence: UI creates empty topic/categories in `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/ThemeManager.tsx:144`, while backend rejects empty topic/categories in `/Users/oabrivard/Projects/rust/ai_synth/backend/src/models/theme.rs:43` and enforces validation in `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/themes.rs:45`.
|
||||
- Why it matters: blocks core onboarding flow.
|
||||
- Direction: align API contract + UI create flow (draft-support or valid seeded defaults).
|
||||
|
||||
### P1
|
||||
- Schedule API contract drift across spec, backend, tests, and frontend typing.
|
||||
- Evidence: spec says `ScheduleResponse` or 404 in `/Users/oabrivard/Projects/rust/ai_synth/docs/technical_specs.md:420`; backend returns `200 null` in `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/schedules.rs:40`; tests expect `200 null` in `/Users/oabrivard/Projects/rust/ai_synth/backend/tests/api_schedules_test.rs:77`; frontend assumes non-null in `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/components/settings/SettingsSchedule.tsx:48`.
|
||||
- Why it matters: runtime fragility and contract confusion.
|
||||
- Direction: choose one contract and align all layers.
|
||||
|
||||
- Theme defaults are inconsistent with DB/spec.
|
||||
- Evidence: DB migration defaults 4/3 in `/Users/oabrivard/Projects/rust/ai_synth/backend/migrations/20260326000028_create_themes_and_migrate.sql:8`; handler uses 5/2 in `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/themes.rs:56`; UI uses 5/2 in `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/ThemeManager.tsx:51`.
|
||||
- Why it matters: non-deterministic behavior and drift.
|
||||
- Direction: establish a single source of truth for defaults.
|
||||
|
||||
- Reliability gap: job-store TTL cleanup exists but is not scheduled at runtime.
|
||||
- Evidence: cleanup function exists in `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/job_store.rs:153`; startup schedules other tasks but not job-store cleanup in `/Users/oabrivard/Projects/rust/ai_synth/backend/src/main.rs:66`; docs expect periodic cleanup in `/Users/oabrivard/Projects/rust/ai_synth/docs/technical_specs.md:778`.
|
||||
- Why it matters: stale in-memory entries and lock leakage risk over time.
|
||||
- Direction: add periodic `job_store.cleanup_expired()` task.
|
||||
|
||||
- Scheduled generation path does not enforce 15-minute timeout used by manual generation.
|
||||
- Evidence: manual timeout in `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/generation.rs:89`; scheduler calls pipeline directly in `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scheduler.rs:57`; requirement states 15-minute cap in `/Users/oabrivard/Projects/rust/ai_synth/docs/requirements.md:30`.
|
||||
- Why it matters: scheduled runs can overrun and block throughput.
|
||||
- Direction: apply same timeout semantics to scheduled execution.
|
||||
|
||||
### P2
|
||||
- Core pipeline and key pages are too large.
|
||||
- Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scraper.rs`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/ThemeManager.tsx`, `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/Settings.tsx`.
|
||||
- Why it matters: weaker SRP, higher cognitive load, slower onboarding/review/testing.
|
||||
- Direction: split by bounded responsibilities (phase modules/services/hooks/components).
|
||||
|
||||
- Frontend architectural consistency drift (API and data-loading patterns).
|
||||
- Evidence: direct `fetch` in `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/GenerateSynthesis.tsx:241` despite centralized client in `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/api/client.ts:25`; guideline preference for `createResource` in `/Users/oabrivard/Projects/rust/ai_synth/docs/dev_guidelines.md:174`.
|
||||
- Direction: enforce API client boundary and consistent data-loading patterns.
|
||||
|
||||
## SOLID Scorecard
|
||||
|
||||
| Dimension | Score (/5) | Notes |
|
||||
|---|---:|---|
|
||||
| Single Responsibility | 2 | Several large multi-responsibility modules. |
|
||||
| Open/Closed | 4 | LLM provider abstraction/factory is extensible. |
|
||||
| Liskov Substitution | 4 | Provider implementations conform to trait contract cleanly. |
|
||||
| Interface Segregation | 3 | Mostly good boundaries with a few broad surfaces. |
|
||||
| Dependency Inversion | 3 | Good trait usage for LLM, pipeline still tightly coupled in places. |
|
||||
| Overall maintainability | 3 | Good foundations, but contract drift + module size are bottlenecks. |
|
||||
|
||||
## Refactoring Plan
|
||||
1. Phase 0: Contract decisions (0.5-1 day, low risk).
|
||||
- Decide canonical behavior for theme creation, schedule GET semantics, and defaults.
|
||||
2. Phase 1: Correctness alignment (1-2 days, medium risk).
|
||||
- Align backend/frontend/tests with chosen contracts.
|
||||
3. Phase 2: Reliability hardening (1 day, low-medium risk).
|
||||
- Add job-store cleanup scheduling and timeout parity in scheduler.
|
||||
4. Phase 3: Backend decomposition (3-5 days, medium risk).
|
||||
- Split synthesis pipeline into phase modules with explicit contexts.
|
||||
5. Phase 4: Frontend decomposition (3-5 days, medium risk).
|
||||
- Split large pages into composables/components and remove direct page-level fetch.
|
||||
6. Phase 5: Drift prevention (1-2 days, low risk).
|
||||
- Add conformance tests/checks for contracts/defaults and PR architecture checklist.
|
||||
|
||||
## Architecture/SOLID Scorecard
|
||||
| Dimension | Score (/5) | Notes |
|
||||
|---|---:|---|
|
||||
| Layering clarity | 3 | Clear module boundaries, but some runtime orchestration gaps.
|
||||
| SRP | 2 | Multiple "god files" in backend and frontend.
|
||||
| OCP | 4 | LLM provider abstraction/factory is cleanly extensible.
|
||||
| DIP | 3 | Core synthesis is trait-based for LLM, but still tightly coupled to DB/functions.
|
||||
| Testability | 3 | Strong unit/integration coverage in backend; scheduler/e2e gaps remain.
|
||||
| Spec alignment | 2 | Multiple contract/default mismatches.
|
||||
|
||||
## Refactoring Plan (Architecture-Level)
|
||||
|
||||
### Phase 1 (1-2 days) — Contract and reliability fixes
|
||||
- Align theme creation contract (UI + model validation + tests).
|
||||
- Align schedule GET semantics across docs/handler/tests/frontend types.
|
||||
- Add periodic `job_store.cleanup_expired()` background task.
|
||||
- Apply 15-minute timeout to scheduled generation path.
|
||||
|
||||
### Phase 2 (3-5 days) — Backend decomposition
|
||||
- Split `services/synthesis.rs` into submodules:
|
||||
- `init.rs` (load settings/theme/provider)
|
||||
- `phase1_personalized.rs`
|
||||
- `phase2_fallback.rs`
|
||||
- `finalize.rs`
|
||||
- `tracing.rs`
|
||||
- Introduce explicit pipeline context structs to reduce argument fanout.
|
||||
|
||||
### Phase 3 (3-5 days) — Frontend decomposition
|
||||
- Extract page-level logic from `ThemeManager` and `Settings` into composables/hooks:
|
||||
- `useThemeEditor`, `useSourcesManager`, `useScheduleConfig`, `useSettingsForm`
|
||||
- Normalize API usage through `api/client.ts` only.
|
||||
|
||||
### Phase 4 (2-3 days) — Conformance hardening
|
||||
- Add spec-conformance tests for defaults, schedule contract, and theme creation.
|
||||
- Add architecture fitness checks (lint/custom tests) for forbidden contract drifts.
|
||||
|
||||
## Quick Wins (< 1 day)
|
||||
1. Add scheduled timeout wrapper in `scheduler.rs`.
|
||||
2. Add job-store periodic cleanup task in `main.rs`.
|
||||
3. Fix `ThemeManager` create payload to pass valid initial `theme` and categories.
|
||||
4. Unify schedule GET typing with actual backend behavior.
|
||||
@ -0,0 +1,205 @@
|
||||
# Technical Lead Cross-Cutting Audit (2026-03-27)
|
||||
|
||||
## 1) Clarification Questions
|
||||
- Should `preferred` sources be scoped per theme (as docs imply), or globally per user?
|
||||
- Should bulk/CSV source import/export always target the currently selected theme?
|
||||
- Should creating a new theme be allowed with empty topic/categories first, then completed later in edit mode?
|
||||
|
||||
## 2) Assumptions
|
||||
- Requirements and functional specs are the source of truth over current implementation details.
|
||||
- Theme-level isolation is expected for sources and preferred flags.
|
||||
- The audit is cross-cutting and representative, not a full line-by-line review of all files.
|
||||
- Test baseline in this environment:
|
||||
- `backend`: `cargo test --lib` passed (359/359)
|
||||
- `frontend`: `npx tsc --noEmit` passed
|
||||
- `frontend`: `npx vitest run` blocked by missing optional package `@rollup/rollup-darwin-x64`
|
||||
|
||||
## 3) Prioritized Findings (P0-P3)
|
||||
|
||||
### P0
|
||||
|
||||
#### P0-1: Theme-scoped source import is broken (theme_id dropped)
|
||||
- Why it matters: Bulk/CSV imports can create sources with `theme_id = NULL`, so imported sources are not tied to the active theme and may not be used by per-theme generation.
|
||||
- Evidence:
|
||||
- Frontend sends theme context in bulk import payload: `frontend/src/pages/ThemeManager.tsx` (lines ~394-415).
|
||||
- Backend strips each source to `(title, url)` and calls bulk insert with `None` theme: `backend/src/handlers/sources.rs` (line 154).
|
||||
- DB layer supports `theme_id` but is not used by handler path: `backend/src/db/sources.rs` (lines ~97-117).
|
||||
- Suggested direction: Make `theme_id` required for bulk/CSV imports from theme page, validate ownership, pass through end-to-end.
|
||||
|
||||
#### P0-2: Preferred source update is global per user, not per theme
|
||||
- Why it matters: Marking preferred sources in one theme clears preferred flags in other themes, violating theme independence.
|
||||
- Evidence:
|
||||
- Full reset for all user sources: `UPDATE sources SET is_preferred = false WHERE user_id = $1` in `backend/src/db/sources.rs` (line 154).
|
||||
- Suggested direction: Scope preferred updates by `theme_id` and validate source IDs belong to that theme.
|
||||
|
||||
#### P0-3: Theme creation UX/API contract mismatch likely breaks "Create Theme" flow
|
||||
- Why it matters: UI "create theme" sends empty `theme` and empty `categories`, while backend validation rejects both.
|
||||
- Evidence:
|
||||
- UI sends `theme: ''`, `categories: []`: `frontend/src/pages/ThemeManager.tsx` (lines 144-148).
|
||||
- Backend validation rejects empty topic/categories: `backend/src/models/theme.rs` (lines 43-48).
|
||||
- Suggested direction: Either allow draft theme creation in backend or change UI create action to collect minimal required fields before POST.
|
||||
|
||||
### P1
|
||||
|
||||
#### P1-1: CSV export ignores theme filtering
|
||||
- Why it matters: Export behavior does not match per-theme source management expectations.
|
||||
- Evidence:
|
||||
- Frontend API supports optional `themeId`: `frontend/src/api/sources.ts` (`exportCsv`).
|
||||
- Backend export endpoint always fetches all user sources (`theme_id = None`): `backend/src/handlers/sources.rs` (line 249).
|
||||
- Suggested direction: Add `theme_id` query support to export endpoint and enforce ownership.
|
||||
|
||||
#### P1-2: Default values drift across schema/docs/handler
|
||||
- Why it matters: Inconsistent defaults increase support/debug overhead and produce non-deterministic behavior.
|
||||
- Evidence:
|
||||
- DB defaults: `max_items_per_category=4`, `summary_length=3`: `backend/migrations/20260326000028_create_themes_and_migrate.sql` (lines 8-10).
|
||||
- Handler fallback defaults differ (`5` and `2`): `backend/src/handlers/themes.rs` (lines 56-58).
|
||||
- Suggested direction: Centralize defaults in a shared constant module and use everywhere (migration tests, backend, frontend defaults, docs).
|
||||
|
||||
#### P1-3: Scheduler critical path has minimal test coverage and weak isolation
|
||||
- Why it matters: Autonomous background flow can fail silently and is hard to validate.
|
||||
- Evidence:
|
||||
- Scheduler side-effectful core logic: `backend/src/services/scheduler.rs` (lines 27-91).
|
||||
- Only one unit test present: `backend/src/services/scheduler.rs` (lines 94-102).
|
||||
- Suggested direction: Add integration tests for due-run detection + generation + email dispatch + `last_run_at` semantics; add explicit timeout/panic boundary for scheduled run.
|
||||
|
||||
### P2
|
||||
|
||||
#### P2-1: High complexity concentration in a few large files
|
||||
- Why it matters: Higher regression risk, slower onboarding, difficult refactors.
|
||||
- Evidence (line counts):
|
||||
- `backend/src/services/synthesis.rs` (~1616)
|
||||
- `backend/src/services/scraper.rs` (~1280)
|
||||
- `frontend/src/pages/ThemeManager.tsx` (~935)
|
||||
- `frontend/src/pages/admin/Providers.tsx` (~854)
|
||||
- `frontend/src/pages/Settings.tsx` (~694)
|
||||
- Suggested direction: Split by use-case/phase; extract hooks/services/components with narrow responsibilities.
|
||||
|
||||
#### P2-2: Side effects in render path on admin providers page
|
||||
- Why it matters: Mutating state during render can create subtle reactive bugs and unstable render behavior.
|
||||
- Evidence:
|
||||
- `if (!edits()[provider.id]) { initEdit(provider); }` inside `<For>` rendering: `frontend/src/pages/admin/Providers.tsx` (lines 557-562).
|
||||
- Suggested direction: Move initialization to `createEffect` keyed by provider list diff.
|
||||
|
||||
#### P2-3: API contract drift in frontend source type
|
||||
- Why it matters: Missing field in TS type hides backend/frontend contract mismatch.
|
||||
- Evidence:
|
||||
- Frontend `Source` type omits `theme_id`: `frontend/src/types.ts` (lines 74-81).
|
||||
- Backend response includes `theme_id`: `backend/src/models/source.rs` (`SourceResponse`).
|
||||
- Suggested direction: Align TS types with API payload, add contract tests.
|
||||
|
||||
#### P2-4: i18n guideline leakage in UI strings
|
||||
- Why it matters: Hardcoded text bypasses translation layer and creates inconsistency.
|
||||
- Evidence:
|
||||
- Hardcoded `'Confirmer'`: `frontend/src/pages/ArticleHistory.tsx` (line 140).
|
||||
- Hardcoded `'Erreur inconnue'` fallback: `frontend/src/components/ApiKeyManager.tsx` (line 132), `frontend/src/components/settings/SettingsBraveSearch.tsx` (line 58).
|
||||
- Suggested direction: Move all user-facing literals into i18n keys.
|
||||
|
||||
#### P2-5: Silent error swallowing in synthesis pipeline
|
||||
- Why it matters: data/log persistence failures can go unnoticed, hurting observability and trust.
|
||||
- Evidence:
|
||||
- Multiple DB writes intentionally ignored with `.ok()`: `backend/src/services/synthesis.rs` (e.g., lines 238, 351, 406, 457, 503, 536, 604, 883).
|
||||
- Suggested direction: keep non-blocking behavior but count/emit warning metrics and include failure counters in final job status.
|
||||
|
||||
### P3
|
||||
|
||||
#### P3-1: Coding guideline drift on `expect` in production paths
|
||||
- Why it matters: small but avoidable divergence from stated standards.
|
||||
- Evidence:
|
||||
- `expect` in auth service time delta creation: `backend/src/services/auth.rs` (lines 46, 102).
|
||||
- `expect` in signal handler setup: `backend/src/main.rs` (lines 155, 161).
|
||||
- Suggested direction: convert to explicit error mapping where practical; keep startup-fail-fast only where intentionally justified.
|
||||
|
||||
## 4) Simplification Opportunities
|
||||
- Introduce a `ThemeScopedSourceService` (backend) that owns create/list/import/export/preferred behavior with explicit `theme_id` semantics.
|
||||
- Create a single defaults module (`ThemeDefaults`, `SettingsDefaults`) consumed by backend handlers, frontend defaults, and tests.
|
||||
- Split synthesis pipeline into explicit steps/modules:
|
||||
- `init`, `phase1_sources`, `phase2_search`, `persist`
|
||||
- each with typed input/output context.
|
||||
- Refactor `ThemeManager` into feature slices:
|
||||
- `ThemeSelector`, `ThemeSettingsForm`, `SourceImportPanel`, `SourceList`, `PreferredSourcesPanel`, `SchedulePanel`.
|
||||
- Refactor admin providers page with dedicated local store and reducer-style updates; avoid in-render initialization.
|
||||
- Add API contract verification tests (schema snapshots / shared DTO generation).
|
||||
|
||||
## 5) Refactoring Roadmap (for implementation team)
|
||||
|
||||
### Epic 1: Fix theme-scoping correctness (highest priority)
|
||||
- Scope: source bulk import, CSV import/export, preferred source update.
|
||||
- Deliverables:
|
||||
- Backend request models include required `theme_id` where needed.
|
||||
- DB updates scoped by `user_id + theme_id`.
|
||||
- Integration tests for per-theme isolation.
|
||||
- Acceptance criteria:
|
||||
- Importing under Theme A never affects Theme B.
|
||||
- Preferred toggle under Theme A does not change Theme B.
|
||||
- CSV export returns only selected theme sources.
|
||||
|
||||
### Epic 2: Repair theme creation flow and defaults consistency
|
||||
- Scope: theme create UX/API contract and default values.
|
||||
- Deliverables:
|
||||
- Decide draft-creation vs required-fields-first behavior and make UI/API consistent.
|
||||
- Centralized defaults shared across layers.
|
||||
- Acceptance criteria:
|
||||
- "Create theme" works end-to-end from UI without validation dead-end.
|
||||
- Defaults match docs and migration-backed expectations.
|
||||
|
||||
### Epic 3: Stabilize scheduler reliability
|
||||
- Scope: scheduled generation execution path.
|
||||
- Deliverables:
|
||||
- Integration test suite for scheduler run scenarios.
|
||||
- Timeout/panic guardrails around scheduled runs.
|
||||
- Structured logs/metrics for each schedule execution.
|
||||
- Acceptance criteria:
|
||||
- Deterministic tests for due schedule run, skip, failure, and `last_run_at` behavior.
|
||||
|
||||
### Epic 4: Reduce complexity hotspots
|
||||
- Scope: split largest frontend/backend modules.
|
||||
- Deliverables:
|
||||
- `synthesis.rs` decomposition by phase and helper modules.
|
||||
- `ThemeManager`, `Providers`, `Settings` componentization.
|
||||
- Acceptance criteria:
|
||||
- No module > ~400 lines for page/service orchestrators.
|
||||
- Unit tests cover extracted logic modules.
|
||||
|
||||
### Epic 5: Contract and i18n hardening
|
||||
- Scope: source DTO alignment, string externalization.
|
||||
- Deliverables:
|
||||
- TS types fully aligned with backend responses (`theme_id` included).
|
||||
- Remove hardcoded UI strings from components/pages.
|
||||
- Acceptance criteria:
|
||||
- Type-level checks fail on DTO drift.
|
||||
- All user-visible strings resolve via i18n keys.
|
||||
|
||||
### Epic 6: Observability and QA debt burn-down
|
||||
- Scope: synthesis non-fatal write failures + frontend test reliability.
|
||||
- Deliverables:
|
||||
- Warnings/metrics for swallowed DB/log errors.
|
||||
- Fix frontend test runner dependency issue.
|
||||
- Acceptance criteria:
|
||||
- `npx vitest run` executes in CI and local dev.
|
||||
- Pipeline exposes counters for skipped/failed trace/log writes.
|
||||
|
||||
## 6) Risk Matrix (Impact x Effort)
|
||||
|
||||
| Item | Impact | Effort | Priority |
|
||||
|---|---|---:|---|
|
||||
| Theme-scoped source correctness (imports/preferred/export) | Very High | Medium | P0 |
|
||||
| Theme creation contract fix | High | Low-Medium | P0 |
|
||||
| Scheduler reliability tests + guards | High | Medium | P1 |
|
||||
| Defaults centralization | Medium | Low | P1 |
|
||||
| Complexity decomposition (pipeline + large pages) | High | High | P2 |
|
||||
| Contract/i18n hardening | Medium | Low-Medium | P2 |
|
||||
| Frontend test env stabilization | Medium | Low | P2 |
|
||||
|
||||
## 7) Quick Wins and Anti-Goals
|
||||
|
||||
### Quick wins (1-3 days)
|
||||
- Pass `theme_id` through bulk import path and add one integration test proving theme isolation.
|
||||
- Scope `update_preferred` by `theme_id` and add regression tests.
|
||||
- Align theme defaults to migration/docs (`4/7/3`) and add one backend unit test for create fallback.
|
||||
- Replace hardcoded `'Confirmer'` / `'Erreur inconnue'` with i18n keys.
|
||||
- Move `initEdit` out of render loop in providers page.
|
||||
|
||||
### Anti-goals (do not do now)
|
||||
- Do not redesign the entire generation algorithm while fixing scoping bugs.
|
||||
- Do not introduce new provider abstractions before stabilizing current feature correctness.
|
||||
- Do not perform broad stylistic rewrites in large files until behavioral tests for those flows are in place.
|
||||
Loading…
Reference in New Issue