You cannot select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
206 lines
11 KiB
Markdown
206 lines
11 KiB
Markdown
# 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.
|