oabrivard
/
ai_synth
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

docs: add v2 code audit reports from 5-agent team

Browse Source 
Architect, Frontend Expert, Rust Expert, Tech Lead, and QA Engineer
analyzed the codebase post multi-theme/scheduling features. Key findings:
synthesis.rs God Module (2010 lines), ThemeManager.tsx (935 lines),
scheduler untested, 15 test coverage gaps identified.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
master
oabrivard 3 months ago
parent 1b20d38bbd
commit b30d04c6c5
6 changed files with 2194 additions and 0 deletions
Show Stats Download Patch File Download Diff File

434
docs/audit/v2-architect-report.md
Unescape Escape View File

@ -0,0 +1,434 @@
# AI Weekly Synth -- Architecture Audit Report (v2)
**Date**: 2026-03-27
**Scope**: Full backend codebase (Rust/Axum), key frontend architecture observations
**Auditor**: Software Architect (automated)
---
## Executive Summary
AI Weekly Synth is a well-structured Rust/Axum application that has grown substantially from its initial design. The codebase demonstrates strong fundamentals: consistent error handling, good security practices, clean layer separation between handlers/services/db, and idiomatic use of Axum extractors. Unit test coverage is solid across models, services, and middleware.
However, the growth -- particularly the addition of multi-theme synthesis, scheduled generation, Brave Search, windowed source extraction, and article history tracing -- has introduced several architectural tensions. The synthesis pipeline (`synthesis.rs`) has become a 1500+ line monolith carrying at least five distinct responsibilities. The scheduler bypasses the job store abstraction. And several cross-cutting concerns (provider resolution, rate limiting, history tracking) are tightly coupled to concrete implementations, making the system harder to test, extend, and reason about.
This report organizes findings by SOLID principles, design patterns, architecture, and dependency management, then closes with prioritized recommendations.
---
## 1. SOLID Principles
### 1.1 Single Responsibility Principle (SRP)
**Critical: `synthesis.rs` is a God Module**
At 1500+ lines, `services/synthesis.rs` carries at least six distinct responsibilities:
| Responsibility | Lines (approx.) | Should be |
|---|---|---|
| Job store (in-memory concurrent map) | 1-193 | Own module `services/job_store.rs` |
| Progress event types + emission | 37-71, 1063-1071 | Own module or part of job store |
| Pipeline orchestration (phases 1 + 2 + save) | 200-1038 | `services/pipeline.rs` or `services/generation/mod.rs` |
| Article scraping + classification logic | 471-616, 700-880 | `services/article_processor.rs` |
| URL filtering, normalization, hashing | 1255-1339 | `services/url_utils.rs` |
| Provider/model/key resolution | 1342-1446 | `services/provider_resolver.rs` |
The `run_generation_inner` function alone is ~840 lines. It manages five `HashMap`/`HashSet` tracking structures, two nested loop levels (waves and batches), two separate pipeline phases (personalized sources and web search fallback), and three code paths in Phase 2 (Brave Search, LLM search, skip). This makes the function extremely difficult to test in isolation, review for correctness, or extend with new pipeline stages.
**Moderate: `scheduler.rs` duplicates pipeline invocation logic**
The scheduler constructs its own `watch::channel` and `AtomicBool`, calls `run_generation_inner` directly, and handles email sending inline. It bypasses the `JobStore` entirely, which means:
- Scheduled jobs are invisible to the SSE progress API
- The one-job-per-user guard does not apply (it only checks `job_store.has_active_job`)
- Email sending logic (fetch synthesis, iterate recipients, call `email::send_synthesis_email`) is duplicated -- the handler version is in `syntheses.rs` handler
**Moderate: `AppState` accumulates responsibilities**
`AppState` holds configuration, database pool, HTTP client, auth rate limiter, provider rate limiter, per-user rate limiters, and the job store. While `Clone`-cheap (all Arc-based), it acts as a service locator, making it unclear which components a given handler actually depends on. With 8 fields, this is approaching the point where injecting specific dependencies would improve clarity.
**Minor: Handler-level response types**
Some response types are defined in handlers (`AdminUserResponse` in `handlers/admin.rs`, `GenerateResponse` in `handlers/generation.rs`, `ListResponse` in `handlers/syntheses.rs`) while others are in models. This inconsistency is minor but creates ambiguity about where to look for types.
### 1.2 Open/Closed Principle (OCP)
**Well-applied: LLM Provider abstraction**
The `LlmProvider` trait + factory pattern is the cleanest abstraction in the codebase. Adding a new provider (e.g., Mistral) requires:
1. A new module implementing `LlmProvider`
2. A new match arm in `factory.rs`
3. No changes to the pipeline
This is textbook OCP.
**Violation: Pipeline Phase 2 branching**
Phase 2 of the pipeline has a hard-coded `if settings.use_brave_search { ... } else { ... }` branch that selects between two entirely different code paths (Brave Search vs. LLM web search). Each path contains ~150 lines of nearly identical scrape+classify logic. Adding a third search strategy (e.g., Bing, Perplexity, SearXNG) would require another `else if` branch with the same duplicated scrape/classify logic.
**Violation: Provider resolution fallback defaults**
`resolve_model` contains hard-coded fallback model names (`"gemini-2.5-pro"`, `"gpt-4o"`, `"claude-sonnet-4-20250514"`). These will silently become stale as providers release new models. The fallback chain should be configurable or fail loudly.
### 1.3 Liskov Substitution Principle (LSP)
**Generally well-respected.** The `LlmProvider` trait implementations are fully substitutable. The `MockLlmProvider` correctly implements the same interface and is used in tests via the `provider_override` parameter.
**Minor concern**: The mock provider identifies call types by inspecting the system prompt content (`sys_lower.contains("classer")`, `sys_lower.contains("precis")`). This couples the mock to the French-language prompt wording, making it fragile if prompts change.
### 1.4 Interface Segregation Principle (ISP)
**Well-applied: Axum extractors**
`AuthUser` and `AdminUser` are clean, focused extractors. Handlers declare exactly the auth level they need. The `AdminUser` wrapper pattern (newtype over `AuthUser`) is idiomatic and minimal.
**Opportunity: `LlmProvider` trait could be narrower**
The current trait has two methods: `provider_id()` and `call_llm()`. If the codebase later needs streaming, embedding, or tool-calling capabilities, the trait should be split rather than extended, per ISP.
### 1.5 Dependency Inversion Principle (DIP)
**Critical: Pipeline depends on concrete implementations**
`run_generation_inner` directly calls:
- `db::settings::get_or_create_default` (concrete DB queries)
- `db::themes::get_by_id` (concrete DB queries)
- `db::sources::list_for_user` (concrete DB queries)
- `db::article_history::check_urls_exist` (concrete DB queries)
- `db::article_history::batch_insert_entries` (concrete DB queries)
- `crate::services::scraper::scrape_url` (concrete HTTP scraping)
- `source_scraper::extract_article_links` (concrete link extraction)
- `crate::services::brave_search::search` (concrete Brave API)
- `encryption::decrypt` / `encryption::MasterKey::from_hex` (concrete crypto)
None of these are injected as traits. The `provider_override` parameter for `LlmProvider` is the only dependency that can be swapped -- and it was added specifically for testing. This makes the pipeline untestable without a live Postgres database and network access.
**Moderate: `ProviderRateLimiter` embeds its own SQL**
The `ProviderRateLimiter::reload_from_db` method contains raw `sqlx::query_as` calls rather than going through the `db::rate_limits` module. The comment says "to avoid circular dependency," but this violates the layer boundary and duplicates the DB schema knowledge.
---
## 2. Design Patterns
### 2.1 Well-Applied Patterns
| Pattern | Where | Assessment |
|---|---|---|
| **Factory Method** | `llm/factory.rs` | Clean, tested, extensible |
| **Strategy** | `LlmProvider` trait | Proper polymorphism via `Arc<dyn LlmProvider>` |
| **Observer** | `watch::channel` for SSE | Elegant use of tokio primitives; late subscribers get latest state |
| **Repository** | `db/` modules | Clean separation of SQL from business logic |
| **Extractor** | `AuthUser`, `AdminUser` | Idiomatic Axum; composable auth |
| **Builder** | `AppState::new`, `build_scraper_client` | Consistent construction patterns |
| **Newtype** | `AdminUser(AuthUser)`, `MasterKey` | Type safety for authorization and crypto |
### 2.2 Missing or Needed Patterns
**Pipeline / Chain of Responsibility**
The synthesis generation is conceptually a pipeline with discrete stages:
1. Load settings + theme + sources
2. Phase 1: Source extraction (windowed)
3. Phase 1: Scrape + classify (batched)
4. Phase 2: Web search fallback (Brave or LLM)
5. Phase 2: Scrape + classify fallback results
6. Assemble sections + save
Each stage could be a separate struct implementing a `PipelineStage` trait, with shared context passed through. This would make the pipeline testable per-stage, enable adding/removing stages declaratively, and reduce `run_generation_inner` from 840 lines to ~50.
**Unit of Work / Transaction Manager**
Article history tracing uses a manual `pending_traces` buffer that is flushed at "logical boundaries." This ad-hoc batching is scattered across 7 locations in the pipeline. A dedicated `TraceBatcher` struct could encapsulate the buffer, auto-flush thresholds, and error handling.
**Event Sourcing (lightweight)**
The `ProgressEvent` enum is close to an event-sourced model but is currently fire-and-forget via `watch::channel` (which only retains the latest value). If the system needs progress history for debugging or UI replay, the events should be collected in a log alongside the watch channel.
### 2.3 Anti-Patterns
**Copy-Paste Programming (Critical)**
The scrape+classify logic appears nearly identically in three places:
1. Phase 1 source processing (lines ~470-616)
2. Phase 2 Brave Search processing (lines ~700-880)
3. Phase 2 LLM search validation (lines ~936-960, simpler variant)
Each instance: spawns a `JoinSet` for scraping, collects results, checks rate limits, spawns a `JoinSet` for classification, parses LLM responses, checks `is_article`, extracts dates, assigns categories, updates tracking maps. The only differences are: which `source_type` string is recorded and whether `source_url` is `Some`.
A single `scrape_and_classify_batch` function parameterized by source type would eliminate ~300 lines of duplication.
**Primitive Obsession**
The pipeline uses six `HashMap`/`HashSet` variables (`article_scraped`, `source_counts`, `url_source`, `filled_counts`, `seen_urls`, and the `pending_traces` vec) as raw tracking state. These represent a coherent concept -- "pipeline context" or "generation state" -- and should be bundled into a struct:
```rust
struct GenerationContext {
articles_by_category: HashMap<String, Vec<NewsItem>>,
source_domain_counts: HashMap<String, usize>,
url_to_source: HashMap<String, String>,
category_fill_counts: HashMap<String, usize>,
seen_urls: HashSet<String>,
pending_traces: Vec<ArticleHistoryEntry>,
}
```
**Magic Strings**
Category keys like `"category_0"`, `"category_autre"`, `"category_no_date"`, and status strings like `"filtered_history"`, `"filtered_diversity"`, `"filtered_not_article"`, `"filtered_too_old"`, `"filtered_empty"`, `"filtered_homepage"`, `"filtered_cross_phase_dedup"`, `"used"` are scattered as string literals. These should be constants or an enum.
**Stringly-Typed Configuration**
Several settings use strings where enums would be safer:
- `settings.ai_provider` (could be `enum Provider { Gemini, OpenAi, Anthropic }`)
- `settings.search_agent_behavior` (free-form, but could at least validate non-HTML)
- `synthesis.status` (always `"completed"` in the codebase, but stored as `String`)
---
## 3. Architecture
### 3.1 Layer Separation
The codebase follows a three-layer architecture:
```
handlers/ (HTTP layer) --> services/ (business logic) --> db/ (data access)
|
models/ (shared types)
```
**Assessment: Good but with leaks.**
- Handlers are thin and focused. They validate input, call services/db, and format responses. This is excellent.
- The `db/` layer is clean -- pure SQL queries returning typed results. No business logic leaks into SQL.
- The `services/` layer is where responsibilities blur. `synthesis.rs` calls `db::` modules directly (bypassing any service abstraction), constructs its own SQL in `resolve_model`, and embeds scraping/classification logic that could be separate services.
**Concern: The scheduler sits at the service layer but orchestrates at the handler level**
The scheduler calls `synthesis::run_generation_inner` (a service) but also does email sending (another service), DB fetching (data layer), and schedule marking (data layer) all inline. It should either be a handler (if it needs to compose services) or delegate to a higher-level "generation + notification" service.
### 3.2 Error Handling
**Strengths:**
- Unified `AppError` enum with `IntoResponse` -- all errors produce consistent JSON
- Internal errors log full details but return generic messages to clients (security-conscious)
- `From<sqlx::Error>` and `From<anyhow::Error>` conversions are clean
- Error sanitization in `sanitize_error_message` prevents API key leakage
**Weaknesses:**
- Errors are silently swallowed in multiple places via `.ok()`:
- `db::article_history::batch_insert_entries(...).await.ok()` (7 occurrences) -- if tracing fails, there is no indication
- `db::llm_call_log::truncate_old(...).await.ok()` -- cleanup failure is invisible
- `db::schedules::mark_run(...).await.ok()` -- if this fails, the schedule may fire again next minute
- `unwrap_or_default()` on `serde_json::from_value` calls silently drops malformed data (e.g., `theme.categories` deserialization). A warning log would be more appropriate.
### 3.3 State Management
**In-memory state:**
- `JobStore` (DashMap-based) -- well-designed with TTL cleanup, user locking, and cancellation support
- `RateLimiter` / `ProviderRateLimiter` -- properly `Arc`-wrapped for `Clone`-cheap sharing
- `user_rate_limiters: DashMap<Uuid, UserRateLimitEntry>` -- handles setting changes atomically
**Concern: No persistence for job state**
If the server restarts during a generation, the in-memory job is lost with no way to recover. For a self-hosted single-instance app this may be acceptable, but if resilience is a goal, the job state should be backed by the database.
**Concern: Scheduled job email state is fire-and-forget**
The scheduler sends emails to up to 3 recipients per schedule. If one email fails, the others still send, but there is no retry or notification mechanism. `mark_run` is called unconditionally after a successful generation, even if all emails failed.
### 3.4 Concurrency Model
**Strengths:**
- Proper use of `tokio::task::JoinSet` for parallel scraping and classification
- `DashMap` + `DashSet` for lock-free concurrent access to shared state
- `AtomicBool` for cooperative cancellation (avoids mutex overhead)
- `watch::channel` for fan-out progress notifications
**Weakness: Global rate limiter shared across scheduled + manual jobs**
The `ProviderRateLimiter` is global. A scheduled job and a manual job for different users hitting the same provider share the same bucket. Under load, scheduled jobs could starve manual users (or vice versa). The architecture should consider per-user-or-per-job rate tracking for fairness.
### 3.5 Security Architecture
**Strengths:**
- AES-256-GCM encryption for API keys at rest with per-key nonces
- SSRF prevention in both `scraper.rs` and `source_scraper.rs` (IP allowlist checking, redirect validation)
- CSRF protection via `X-Requested-With` header check on all mutating API endpoints
- Session cookies are `HttpOnly`, `SameSite=Lax`, optionally `Secure`
- Anti-enumeration in auth (same response for existent/non-existent emails, timing attack mitigation)
- Error sanitization prevents API key leakage in SSE error events
- CSP, X-Frame-Options, HSTS, Referrer-Policy headers
**Concern: Gemini API key in URL**
The `GeminiProvider` constructs the API URL as `...?key={api_key}`. While the error handler carefully avoids logging the full URL, the key is still in the URL query string. This means:
- It may appear in HTTP access logs on intermediary proxies
- `reqwest` may include it in error messages despite the `kind`-only logging
- If tracing is set to DEBUG level, the URL may be logged by tower-http's `TraceLayer`
This is a known Gemini API design limitation, but the risk should be documented.
---
## 4. Dependency Management and Testability
### 4.1 Test Architecture
**Strengths:**
- Unit tests for all model validation logic (settings, theme, schedule, source, synthesis)
- Unit tests for error handling, rate limiting, URL normalization, link extraction
- Mock LLM provider enables end-to-end pipeline testing without real API calls
- Factory tests verify correct provider instantiation
**Weaknesses:**
- The core pipeline (`run_generation_inner`) cannot be unit-tested. It requires:
- A live `PgPool` (for all `db::` calls)
- A real `AppState` (for config, rate limiters, job store)
- Network access (for scraping via `http_client`)
- Only the LLM provider can be mocked (via `provider_override`)
- No integration tests for the scheduler
- No tests for the Brave Search integration path
- No tests for Phase 2 (web search fallback) at all
### 4.2 Dependency Injection
The codebase uses Axum's `State(AppState)` extractor as its sole DI mechanism. This works well for handlers but breaks down for services:
- Services receive `&AppState` directly, gaining access to everything
- There is no trait boundary between the pipeline and its dependencies (db, scraper, search)
- The `provider_override: Option<Arc<dyn LlmProvider>>` parameter proves the value of DI -- it is the only seam that enables testing
**Recommendation**: Introduce a `PipelineDeps` trait (or struct of trait objects) that the pipeline receives, encapsulating:
- Database access (settings, sources, themes, article history)
- Scraping (source page scraping, article scraping)
- Search (Brave, LLM web search)
- Rate limiting
- Key resolution
This would allow the entire pipeline to be tested with in-memory fakes.
### 4.3 Module Coupling
The dependency graph is mostly clean:
```
handlers --> services --> db
| |
models <-----+
|
errors
```
Exceptions:
- `synthesis.rs` calls `db::` directly (bypasses service layer for settings, themes, sources, history, llm_call_log, syntheses)
- `synthesis.rs` calls `crate::services::prompts`, `crate::services::llm::schema`, `crate::services::scraper`, `crate::services::source_scraper`, `crate::services::brave_search`, `crate::services::encryption` -- essentially importing the entire services layer
- `rate_limiter.rs` contains its own SQL queries
- `handlers/syntheses.rs::list` constructs a `Synthesis` model manually from the `SynthesisWithThemeName` join row, duplicating field mapping
---
## 5. Specific Code-Level Findings
### 5.1 The `#[allow(clippy::too_many_arguments)]` Smell
Two functions suppress this lint:
- `build_search_prompt` (9 parameters)
- `log_llm_call` (10 parameters)
Both are symptoms of missing context objects. `build_search_prompt` should take a `SearchPromptConfig` struct. `log_llm_call` should take a `LlmCallRecord` struct.
### 5.2 `run_generation_inner` Parameter List
The function takes 7 parameters: `job_id`, `state`, `user_id`, `theme_id`, `tx`, `provider_override`, `cancelled`. The first four are "what to generate," the last three are "infrastructure." A `GenerationJob` struct and a `PipelineInfra` struct would make intent clearer.
### 5.3 Inconsistent `serde_json::Value` vs. Typed Models
`theme.categories` is stored as `serde_json::Value` and deserialized inline with `serde_json::from_value(...).unwrap_or_default()`. This pattern appears at least 5 times across the codebase (themes, schedules, syntheses). Consider using `sqlx::types::Json<Vec<String>>` for typed extraction at the query level.
### 5.4 French/English Mixing
User-facing messages are consistently in French (good for i18n consistency), but code-level strings mix languages:
- Status strings: `"filtered_history"` (English), `"category_autre"` (French), `"Articles sans date"` (French)
- Log messages: English
- Error messages to users: French
The recommendation is to keep all internal identifiers in English and use the i18n layer for user-facing strings.
---
## 6. Prioritized Recommendations
### P0 -- High Impact, Do First
1. **Extract `GenerationContext` struct** from the 6 tracking variables in `run_generation_inner`. This is a safe refactor that immediately improves readability and reduces parameter passing.
2. **Extract `scrape_and_classify_batch` function** to eliminate the 300-line duplication between Phase 1, Brave Search, and LLM search paths. Parameterize by `source_type: &str` and `source_url: Option<&str>`.
3. **Move `JobStore` to its own module** (`services/job_store.rs`). It is already self-contained with no dependencies on synthesis logic. This reduces `synthesis.rs` by ~190 lines.
### P1 -- Important, Do Soon
4. **Introduce `TraceBatcher`** struct to encapsulate the pending traces buffer, batch insert calls, and error handling. Replace the 7 manual flush sites with `batcher.flush()`.
5. **Make the scheduler use `JobStore`** for scheduled jobs. This provides visibility into scheduled generation progress and prevents race conditions between scheduled and manual jobs. Add email sending as a post-completion hook rather than inline in the scheduler.
6. **Replace magic strings with constants or enums** for article statuses (`"filtered_history"`, etc.), category keys (`"category_0"`, `"category_autre"`, `"category_no_date"`), and synthesis status.
7. **Add structured logging to `.ok()` calls**. Replace `batch_insert_entries(...).await.ok()` with `if let Err(e) = batch_insert_entries(...).await { tracing::warn!(...) }`.
### P2 -- Improve Quality
8. **Split Phase 2 search strategies** into a `SearchStrategy` trait with `BraveSearchStrategy` and `LlmSearchStrategy` implementations. The pipeline would call `strategy.search(query, max_results)` without knowing which backend is used.
9. **Extract `provider_resolver.rs`** for the provider/key/model resolution logic (~100 lines currently in `synthesis.rs`).
10. **Introduce `PipelineDeps` trait or struct** to enable full pipeline testing without Postgres/network. Start with the article history check as the first dependency to extract, since it is the most frequently called.
11. **Remove inline SQL from `rate_limiter.rs`** and `synthesis.rs::resolve_model`. Route all queries through `db/` modules.
### P3 -- Nice to Have
12. **Type the `categories` field** as `sqlx::types::Json<Vec<String>>` instead of `serde_json::Value` to eliminate runtime deserialization.
13. **Consolidate response types** -- either all in `models/` or all in `handlers/`, with a consistent convention.
14. **Add a `SearchPromptConfig` struct** to replace the 9-parameter `build_search_prompt` function.
15. **Document the TOCTOU risk** in the Gemini API key URL pattern and consider using the `x-goog-api-key` header instead (if supported by the Gemini API version in use).
---
## 7. What the Codebase Does Well
It is important to acknowledge the strengths that should be preserved during refactoring:
- **Error handling discipline**: The `AppError` enum is consistently used everywhere. No panics in production code. Internal details are never leaked.
- **Security-first design**: SSRF prevention, encrypted secrets, CSRF protection, anti-enumeration, session management -- all implemented correctly.
- **Idiomatic Axum usage**: Extractors, state management, middleware composition, SSE streaming -- all follow framework conventions.
- **Test coverage on leaf components**: Models, utils, and isolated services have thorough unit tests with boundary cases.
- **Documentation**: Module-level doc comments, function-level doc comments, and inline comments explaining non-obvious decisions (e.g., the TOCTOU note in `scraper.rs`).
- **Operational features**: Graceful shutdown, session cleanup, job TTL, rate limit hot-reload, cooperative cancellation -- these show production-mindedness.
---
## Appendix: File Size Summary
| File | Lines | Assessment |
|---|---|---|
| `services/synthesis.rs` | ~1550 | Critical -- needs decomposition |
| `services/scraper.rs` | ~400 | Acceptable |
| `services/rate_limiter.rs` | ~470 | Acceptable (includes thorough tests) |
| `services/prompts.rs` | ~370 | Acceptable (includes thorough tests) |
| `handlers/auth.rs` | ~380 | Acceptable |
| `handlers/sources.rs` | ~280 | Good |
| `handlers/admin.rs` | ~440 | Acceptable |
| `handlers/syntheses.rs` | ~240 | Good |
| `handlers/generation.rs` | ~180 | Good |
| `models/settings.rs` | ~260 | Good (includes thorough tests) |
| `models/synthesis.rs` | ~415 | Acceptable (includes thorough tests) |
| `errors.rs` | ~173 | Good |
| `app_state.rs` | ~82 | Good |
| `router.rs` | ~178 | Good |
| `scheduler.rs` | ~93 | Good size, but needs architectural changes |

100
docs/audit/v2-consolidated-summary.md
Unescape Escape View File

@ -0,0 +1,100 @@
# Code Audit v2 — Consolidated Summary
**Date**: 2026-03-27
**Auditors**: Software Architect, Frontend Expert, Rust Expert, Tech Lead, QA Engineer
## Overall Assessment
The codebase has strong fundamentals — zero `unsafe`, zero `unwrap()` in production, excellent security (SSRF, encryption, CSRF), clean error handling, and 689 total tests. The project has grown significantly with multi-theme, scheduling, preferred sources, and Brave Search. The main issues are accumulated complexity in `synthesis.rs` and `ThemeManager.tsx`.
---
## Top Issues (agreed by 3+ auditors)
### 1. `synthesis.rs` God Module — 2010 lines, ~800-line main function
**Confirmed by**: Architect, Tech Lead, Rust Expert
The scrape+classify batch loop is duplicated across Phase 1 and Brave Search paths (~300 lines). The function manages 6+ tracking HashMaps and mixes job store, pipeline orchestration, URL utilities, and provider resolution.
**Recommended refactoring:**
- Extract `scrape_and_classify_batch()` — shared helper (~300 lines saved)
- Extract `GenerationContext` struct for tracking state (filled_counts, source_counts, etc.)
- Move `JobStore` to its own module (`services/job_store.rs`)
- Split into `synthesis/pipeline.rs`, `synthesis/job_store.rs`, `synthesis/helpers.rs`
### 2. `ThemeManager.tsx` — 935 lines with ~30 signals
**Confirmed by**: Frontend Expert, Tech Lead
Mixes theme content editing, source management, bulk import, CSV import/export, preferred toggles, and schedule config in one component.
**Recommended refactoring:**
- Extract `SourceManager` component (reusable, shared with Sources.tsx)
- Extract `ThemeContentForm` component (content settings)
- Already have `SettingsSchedule` and `SettingsBraveSearch` as sub-components
### 3. Scheduler has zero tests
**Confirmed by**: QA Engineer, Rust Expert
`run_scheduled_jobs()` is an autonomous background process that triggers generation and sends emails. No unit, integration, or E2E tests.
### 4. Hardcoded French strings bypass i18n
**Confirmed by**: Frontend Expert, Tech Lead
"Lire la suite", "Reduire", day labels in SynthesisDetail.tsx and other pages use hardcoded French instead of the i18n system.
---
## High Priority Issues
| # | Issue | Source | Impact |
|---|-------|--------|--------|
| 5 | No concurrency limit on JoinSet spawns | Rust Expert | Under load, hundreds of parallel HTTP requests |
| 6 | Silent `.ok()` on DB trace writes | Rust Expert | Lost traceability data without logging |
| 7 | Duplicated source management logic (~80%) between Sources.tsx and ThemeManager.tsx | Tech Lead | Maintenance burden |
| 8 | `createEffect` with fire-and-forget async in ThemeManager | Frontend Expert | Race conditions on rapid theme switching |
| 9 | SSE progress stream untested at integration level | QA Engineer | Only tested via flaky E2E |
| 10 | Date extraction/max_age filtering untested | QA Engineer | Pipeline filtering logic uncovered |
---
## Medium Priority Issues
| # | Issue | Source |
|---|-------|--------|
| 11 | `serde_json::Value` for categories/days/emails — should be `Vec<String>` | Rust Expert |
| 12 | Inconsistent feedback patterns (inline banners vs Toast) | Frontend Expert |
| 13 | Duplicated delete confirmation logic (3 different patterns) | Frontend Expert |
| 14 | Raw SQL in `resolve_model()` breaking db-module abstraction | Tech Lead |
| 15 | `UpdateThemeRequest` has no validation | Rust Expert |
| 16 | Email validation too permissive (just checks `@`) | Rust Expert |
| 17 | Scheduler runs jobs sequentially — one blocks all | Rust Expert |
| 18 | 10 failing frontend unit tests | QA Engineer |
| 19 | `normalizeUrl`/`isValidUrl` utilities live in page files | Frontend Expert |
---
## Positive Highlights
All auditors noted these strengths:
- **Zero unsafe code**, zero unwrap in production paths
- **Strong security**: AES-256-GCM, SSRF with 74 tests, CSRF, anti-enumeration
- **Clean `AppError` enum** — consistent JSON responses, internal errors never leaked
- **Well-designed `LlmProvider` trait** — Strategy + Factory patterns
- **689 tests** — good coverage on scraper, providers, encryption, CRUD
- **SSE utility** is exemplary SolidJS reactive code
- **Clean API client layer** — type-safe, consistent patterns
- **Excellent module documentation** — `//!` doc comments on every module
---
## Detailed Reports
- [Architect Report](v2-architect-report.md) — SOLID principles, design patterns, architecture
- [Frontend Report](v2-frontend-report.md) — SolidJS patterns, component architecture
- [Rust Expert Report](v2-rust-expert-report.md) — Idiomatic Rust, async, safety
- [Tech Lead Report](v2-tech-lead-report.md) — Complexity, duplication, refactoring plan
- [QA Report](v2-qa-report.md) — Test coverage gaps (15 gaps found)

443
docs/audit/v2-frontend-report.md
Unescape Escape View File

@ -0,0 +1,443 @@
# Frontend Audit Report v2 -- SolidJS + Tailwind CSS
**Date**: 2026-03-27
**Scope**: Pages (ThemeManager, Settings, GenerateSynthesis, Home, SynthesisDetail), settings components (SettingsSchedule, SettingsBraveSearch, SettingsRateLimit), ApiKeyManager, API layer, types, utilities
**Codebase snapshot**: commit `7c8a196`
---
## Executive Summary
The frontend is well-structured for a learning project. SolidJS primitives are used correctly and idiomatically in most places. The API layer is clean, the type system is comprehensive, and the i18n approach is sound. The main areas for improvement are: oversized page components that mix too many concerns, inconsistent feedback patterns (inline messages vs. toasts), a few SolidJS reactivity subtleties, and opportunities for better component reuse.
---
## 1. SolidJS Patterns
### 1.1 Reactive Primitives -- Correct Usage
Signals, effects, and resources are used appropriately throughout. Notable good patterns:
- **`createResource`** in `Settings.tsx` for providers/API keys -- idiomatic for async data that should auto-refetch.
- **`createMemo`** in `Home.tsx` (`groupedByTheme`) -- correctly memoizes an expensive grouping computation.
- **`createSSEConnection`** in `utils/sse.ts` -- elegantly wraps EventSource into reactive signals with `onCleanup`. This is textbook SolidJS.
- **Optimistic updates** in `ThemeManager.tsx` (`handleTogglePreferred`) -- updates local state first, reverts on error. Exactly the right pattern.
### 1.2 Reactive Primitives -- Issues
**ISSUE (Medium): `createEffect` triggering async side effects in `ThemeManager.tsx` (line 123-138)**
```tsx
createEffect(() => {
const theme = selectedTheme();
if (theme) {
// ...populate signals...
fetchSources(theme.id); // async fire-and-forget inside effect
}
});
```
Calling an async function inside `createEffect` without tracking the returned promise means errors in `fetchSources` are silently swallowed by the effect (the `try/catch` inside `fetchSources` helps, but the pattern itself is fragile). More importantly, if `selectedTheme()` changes rapidly, multiple concurrent `fetchSources` calls can race and the final state may reflect a stale theme's sources.
**Recommendation**: Debounce or cancel previous fetches. Consider using `createResource` keyed on `selectedThemeId()` instead, which natively handles this:
```tsx
const [sources] = createResource(selectedThemeId, (id) => sourcesApi.list(id));
```
**ISSUE (Low): Duplicate `createEffect` for auto-selecting provider in `Settings.tsx` (lines 76-88 and 122-131)**
Two separate effects handle provider-related logic. The first checks if the saved provider is still available; the second auto-selects when only one exists. These could be unified into a single effect for clarity.
### 1.3 Show/For Usage -- Generally Correct
- `<Show when={...}>` with the callback form `{(accessor) => ...}` is used correctly in most places (e.g., Settings.tsx line 232, ThemeManager.tsx line 489).
- `<For each={...}>` is used everywhere lists are rendered -- correct.
**ISSUE (Low): Multiple adjacent `<Show>` blocks with inverted conditions in `GenerateSynthesis.tsx` (lines 393-401)**
```tsx
<Show when={status() === 'done'}>...</Show>
<Show when={status() === 'in-progress'}>...</Show>
<Show when={status() === 'pending'}>...</Show>
```
Three mutually exclusive `<Show>` blocks evaluate the same reactive expression three times. SolidJS's `<Switch>`/`<Match>` is the idiomatic pattern for exclusive branching:
```tsx
<Switch>
<Match when={status() === 'done'}>...</Match>
<Match when={status() === 'in-progress'}>...</Match>
<Match when={status() === 'pending'}>...</Match>
</Switch>
```
**ISSUE (Low): Nested `<Show>` chains in `SynthesisDetail.tsx` provenance section (lines 494-538)**
Three nested `<Show>` blocks for loading/empty/data states. A `<Switch>` would be cleaner.
### 1.4 onCleanup -- Mostly Good
- `ThemeManager.tsx` clears both `deleteTimer` and `deleteThemeTimer` on cleanup -- good.
- `Home.tsx` clears all delete timers from the record -- good.
- `SynthesisDetail.tsx` clears the email success timer -- good.
- `createSSEConnection` properly closes EventSource and clears retry timeout on cleanup -- excellent.
**ISSUE (Medium): Missing `onCleanup` for navigation timer in `GenerateSynthesis.tsx`**
The `onCleanup` on line 199 is placed inside a `createEffect`, which is correct for that specific timer. However, the SSE connection itself is only cleaned up via `onCleanup` inside `createSSEConnection`. If the component is destroyed while a generation is in flight and the user never calls `handleRetry`, the connection is properly cleaned up by the SSE utility's own `onCleanup`. This is fine -- just noting the implicit dependency.
**ISSUE (Low): `emailTimer` stored as a signal in `SynthesisDetail.tsx` (line 134)**
```tsx
const [emailTimer, setEmailTimer] = createSignal<ReturnType<typeof setTimeout> | undefined>();
```
Timer IDs do not need to be reactive (nothing in the UI depends on the timer value). A plain `let` variable would be simpler, as done in `ThemeManager.tsx` and `Home.tsx`. The signal causes an unnecessary subscription.
---
## 2. Component Architecture
### 2.1 Oversized Components
**ISSUE (High): `ThemeManager.tsx` -- 935 lines, ~30 signals, single monolithic component**
This is the largest and most complex page. It manages:
- Theme CRUD (list, create, save, delete)
- Content settings editing (name, topic, categories, max age, max items, summary length)
- Source CRUD (add, delete, toggle preferred)
- Bulk import (text-based)
- CSV import/export
- Schedule delegation (via `SettingsSchedule`)
The file defines ~30 `createSignal` calls (lines 42-81). This is a strong signal that the component should be decomposed. Suggested extraction:
| Extracted Component | Signals Moved | Lines Saved |
|---|---|---|
| `ThemeContentForm` | editName, editThemeTopic, editCategories, editMaxAge, editMaxItems, editSummaryLength, newCategory, savingTheme, themeMessage | ~180 |
| `ThemeSourceList` | sources, loadingSources, newTitle, newUrl, adding, addError, confirmingDeleteId, deleteTimer | ~230 |
| `ThemeBulkImport` | bulkText, importing, importError | ~50 |
| `ThemeCsvImport` | csvError, fileInputRef, importing | ~40 |
**ISSUE (Medium): `SynthesisDetail.tsx` -- 548 lines, manages email, export, delete, provenance, and display level**
This component could benefit from extracting:
- `SynthesisEmailSection` (email state + send logic)
- `SynthesisExportSection` (markdown + PDF export)
- `SynthesisProvenanceSection` (provenance data + table)
The `NewsItemCard` and `Section` sub-components are already well-extracted within the file (lines 36-105) -- good pattern.
**ISSUE (Medium): `Settings.tsx` -- 694 lines**
Already partially decomposed via `SettingsBraveSearch`, `SettingsRateLimit`, and `ApiKeyManager`. However, the remaining provider selection card (lines 356-512) is ~160 lines of dense JSX with deeply nested `<Show>` blocks. It could become a `SettingsProviderCard` component.
**ISSUE (Low): `GenerateSynthesis.tsx` -- 471 lines**
Acceptable size. The `completedSteps()` computation (lines 135-169) is somewhat complex but well-documented. The step checklist UI could be a separate `StepChecklist` component for readability.
### 2.2 State Management
The application uses a lightweight approach: `AuthContext` for global auth state, `ToastProvider` for notifications, and local signals within each page. This is appropriate for the app's complexity level. There is no global state management library, and none is needed.
**Positive**: The `useToast` context is used consistently in newer components (`SettingsSchedule`, `SettingsBraveSearch`, `ApiKeyManager`).
### 2.3 Reusability
**Good reuse patterns:**
- `Button` component with variant/loading/icon props -- used across pages.
- `LoadingSpinner` with optional `fullPage` prop -- used everywhere.
- `SettingsSchedule`, `SettingsBraveSearch`, `SettingsRateLimit` -- well-scoped settings sub-components.
**ISSUE (Medium): Duplicated inline button styles**
Several pages define raw `<button>` elements with long Tailwind class strings instead of using the `Button` component:
- `ThemeManager.tsx` lines 799-808 (bulk import submit)
- `GenerateSynthesis.tsx` lines 449-461 (generate button)
- `SynthesisDetail.tsx` lines 372-386 (send email button), 410-439 (export buttons)
- `Home.tsx` lines 229-235 (new synthesis button)
These should all use `<Button>` for visual consistency and reduced duplication.
**ISSUE (Medium): Duplicated CSV Export/Import buttons in `ThemeManager.tsx` (lines 744-763)**
The CSV import button uses a raw `<label>` + hidden `<input>` pattern with long inline styles. This pattern also appears in `Settings.tsx` (lines 637-655). A reusable `FileUploadButton` component would eliminate this duplication.
**ISSUE (Low): `SynthesisCard` defined as a function, not a component, in `Home.tsx` (line 133)**
```tsx
const SynthesisCard = (synth: SynthesisListItem) => (...)
```
This is a plain function returning JSX, called as `SynthesisCard(synth)`. In SolidJS this works but bypasses the component lifecycle -- it won't get its own error boundary or Suspense tracking. For a list card that's fine, but it's inconsistent with the rest of the codebase which uses `Component<>`. More importantly, calling it as `SynthesisCard(synth)` instead of `<SynthesisCard synth={synth} />` means SolidJS treats it as an inline expression, not a tracked component boundary. This makes debugging harder in dev tools.
---
## 3. Type Safety
### 3.1 TypeScript Configuration -- Excellent
`tsconfig.json` has `strict: true`, `isolatedModules: true`, and `forceConsistentCasingInFileNames: true`. No escape hatches.
### 3.2 Type Definitions -- Good
`types.ts` is comprehensive with 303 lines covering all domain entities. Types are used consistently across API clients, pages, and components.
**Positive patterns:**
- `satisfies` used in `syntheses.ts` line 86 (`{ email } satisfies SendEmailRequest`) -- ensures the object conforms to the type without widening.
- Union types for `ApiError` fields (`field_errors?: Record<string, string>`).
- Proper nullability (`theme_id: string | null`, `date?: string | null`).
### 3.3 Type Safety Issues
**ISSUE (Medium): `isApiError` type guard is too loose (types.ts lines 163-170)**
```tsx
export function isApiError(error: unknown): error is ApiError {
return (
typeof error === 'object' &&
error !== null &&
'status' in error &&
'message' in error
);
}
```
Any object with `status` and `message` properties will pass this guard -- including native `Response` objects, DOM errors, or other library errors. Adding a type discriminator field (e.g., `kind: 'api_error'`) or checking `typeof error.status === 'number'` would improve safety.
**ISSUE (Low): Theme types defined in `api/themes.ts` rather than `types.ts`**
`ThemeResponse`, `CreateThemeRequest`, and `UpdateThemeRequest` are defined in `api/themes.ts`. All other domain types (Source, Synthesis, UserSettings, etc.) live in `types.ts`. This inconsistency means consumers must import from two different locations. The same applies to `ScheduleResponse` and `UpsertScheduleRequest` in `api/schedules.ts`.
**ISSUE (Low): Non-null assertion in `Settings.tsx` line 543**
```tsx
<ApiKeyManager providers={providers()!} />
```
The `!` assertion is safe here because it's inside a `<Show when={providers() && providers()!.length > 0}>` block, but it would be cleaner to use the callback form of `<Show>` to get a guaranteed-non-null accessor.
**ISSUE (Low): Non-null assertion in `ThemeManager.tsx` line 913**
```tsx
<SettingsSchedule themeId={selectedThemeId()!} />
```
Same pattern -- safe because it's inside `<Show when={selectedTheme()}>`, but the `!` could be avoided by threading the theme's ID through the callback accessor.
### 3.4 API Contracts
The API client (`client.ts`) is well-designed:
- Generics on all methods (`get<T>`, `post<T>`, etc.) provide return-type safety.
- `FormData` is handled transparently (no `Content-Type` header set, letting the browser add the multipart boundary).
- 401 redirect is centralized.
- 204 handling returns `undefined as T` -- this is a reasonable trade-off; the alternative would be a separate `deleteVoid` method.
**ISSUE (Low): `API_BASE` defined in two places**
`API_BASE` is defined as a constant in both `client.ts` (line 3) and `syntheses.ts` (line 4). The `syntheses.ts` copy is only used for `fetchFile` and `progressUrl`, which bypass the `ApiClient`. If `fetchFile` were added to `ApiClient`, the duplication would be eliminated.
---
## 4. Code Organization
### 4.1 File Structure -- Good
The project follows clear conventions:
- `pages/` for route-level components
- `components/` for reusable pieces, with `ui/` for atomic elements and `settings/` for domain-specific composites
- `api/` for HTTP clients, one file per resource
- `contexts/` for global state
- `utils/` for pure helpers
- `i18n/` for translations
### 4.2 Import Conventions -- Consistent
All imports use the `~/` alias. No relative path chaos. Named imports are used everywhere (no `import *`).
### 4.3 Issues
**ISSUE (Low): `normalizeUrl` and `isValidUrl` exported from `pages/Sources.tsx` and imported by `pages/ThemeManager.tsx` (line 23)**
```tsx
import { normalizeUrl, isValidUrl } from '~/pages/Sources';
```
Page components should not export utility functions consumed by other pages. These helpers belong in `~/utils/url.ts` (or similar). The Sources page is currently redirected to `/themes` in the router (App.tsx line 67), suggesting it may be deprecated soon, which would break the import.
**ISSUE (Low): `fetchFile` and `triggerDownload` exported from `api/syntheses.ts` and imported by `api/sources.ts`**
General-purpose file download utilities live inside a domain-specific API module. They should be in `api/client.ts` or a dedicated `utils/download.ts`.
---
## 5. Page-by-Page Analysis
### 5.1 ThemeManager.tsx
**Strengths:**
- JSDoc block comment at the top explains the page's purpose.
- Proper `onCleanup` for timers.
- Optimistic updates for preferred source toggling.
- Delegates schedule management to `SettingsSchedule`.
**Weaknesses:**
- Largest file in the frontend (935 lines). Should be decomposed (see section 2.1).
- Mixes content settings, source CRUD, bulk import, and CSV import in one render tree.
- The two-click delete pattern (with timer-based confirmation reset) is duplicated for both themes and sources -- could be a reusable hook (`createConfirmDelete`).
### 5.2 Settings.tsx
**Strengths:**
- Excellent JSDoc documenting the page's key behaviors.
- Import/Export as a collapsed `<details>` section -- good UX.
- Provider auto-detection when only one provider exists.
- `DEFAULT_SETTINGS` used as a merge base for imported settings -- robust.
- Sticky save button at the bottom.
**Weaknesses:**
- Provider card section is large and deeply nested.
- Two separate `createEffect` blocks for provider logic.
### 5.3 GenerateSynthesis.tsx
**Strengths:**
- Clean SSE state machine documented in the JSDoc.
- `STEPS` array as a module-level constant -- easy to extend.
- `createEffect` for auto-redirect on completion is clean.
- Good UX: "you can leave this page" note, stop button, retry button.
**Weaknesses:**
- `completedSteps()` has O(steps * events) complexity -- fine for 3 steps, but the logic is hard to follow.
- Three adjacent `<Show>` blocks for step status should be `<Switch>/<Match>`.
- `handleStop` makes a raw `fetch` call instead of using the API client.
### 5.4 Home.tsx
**Strengths:**
- Sort toggle (date vs. theme) is a nice UX addition.
- `groupedByTheme` memo is efficient and well-commented.
- Delete confirmation with auto-cancel timers, one per card -- well-implemented.
**Weaknesses:**
- `SynthesisCard` is a function, not a component (see 2.3).
- Inline SVG for LLM logs icon (lines 191-193) -- should use a Lucide icon or a reusable component.
- The `deleteTimers` signal stores a `Record<string, timeout>` -- every timer update creates a new object. For many cards, a `Map` stored as a `let` variable would be more efficient (timer IDs don't need to be reactive).
### 5.5 SynthesisDetail.tsx
**Strengths:**
- `NewsItemCard` and `Section` are well-extracted sub-components.
- Display level slider for article detail density -- creative UX.
- `truncateSummary` is a pure function outside the component -- good.
- Pre-fills email from auth context.
**Weaknesses:**
- 548 lines -- should extract email/export/provenance sections (see 2.1).
- Hardcoded French strings in `NewsItemCard` (lines 75, 83: "Lire la suite" and "Reduire"). These bypass the i18n system.
- Provenance table has no pagination -- could be large.
---
## 6. Consistency Issues
### 6.1 Feedback Patterns -- Inconsistent
| Component | Feedback Mechanism |
|---|---|
| ThemeManager | Inline message banner (`themeMessage` signal) |
| Settings | Inline message banner (`message` signal) |
| SettingsSchedule | Toast notifications (`useToast`) |
| SettingsBraveSearch | Toast notifications (`useToast`) |
| ApiKeyManager | Toast notifications (`useToast`) |
| GenerateSynthesis | Inline error/success banners |
| Home | Inline error banner |
| SynthesisDetail | Inline error/success banners per section |
Newer components (SettingsSchedule, SettingsBraveSearch, ApiKeyManager) use the Toast system. Older pages use inline message banners with manual state management. **Recommendation**: Migrate all feedback to the Toast system for consistency, except for blocking errors that prevent the user from proceeding (those should remain inline).
### 6.2 Delete Confirmation Patterns -- Duplicated
Three different implementations:
1. **Two-click with timer** (ThemeManager sources, ThemeManager theme): first click enters confirm state, auto-resets after 3-5 seconds.
2. **Two-click with timer via signal record** (Home): same concept, but uses a `Record<string, timeout>` signal.
3. **Modal-style banner** (SynthesisDetail): `showDeleteConfirm` signal shows a banner with Cancel/Confirm buttons.
A reusable `createDeleteConfirmation(timeoutMs)` primitive could unify all three.
### 6.3 Error Handling Patterns -- Consistent
The `isApiError(err)` guard is used consistently across all pages and components for try/catch blocks. The fallback pattern `t('some.errorKey')` is applied uniformly. This is good.
---
## 7. Accessibility
**ISSUE (Medium): No `aria-label` on many interactive elements**
- Star toggle buttons in ThemeManager (line 833): `title` attribute is set but no `aria-label`.
- Delete buttons in source list (line 869): same issue.
- Sort toggle buttons in Home (lines 289-308): no accessible name.
**ISSUE (Low): `<details>/<summary>` in Settings (line 619) may not announce state changes to screen readers**
Native `<details>` has good built-in accessibility, but the `<p>` inside `<summary>` may cause unexpected behavior in some screen readers.
---
## 8. Performance Considerations
**ISSUE (Low): Sorting inside `<For each={...}>` in Home.tsx (line 314)**
```tsx
<For each={[...syntheses()].sort(...)}>
```
Every reactive update to `syntheses()` will re-sort the array. This should use `createMemo`:
```tsx
const sortedByDate = createMemo(() =>
[...syntheses()].sort((a, b) => new Date(b.created_at).getTime() - new Date(a.created_at).getTime())
);
```
The `groupedByTheme` already does this correctly via `createMemo`.
**ISSUE (Low): Growing events array in SSE connection**
`createSSEConnection` appends every event to the `events` signal array. For long-running generation jobs, this array grows unboundedly. The `completedSteps()` function in GenerateSynthesis iterates over all events on every reactive update. For typical jobs this is negligible, but for very long jobs with many progress events it could become noticeable.
---
## 9. Summary of Recommendations
### Priority High
1. **Decompose `ThemeManager.tsx`** into 3-4 focused sub-components (ThemeContentForm, ThemeSourceList, ThemeBulkImport, ThemeCsvImport).
### Priority Medium
2. **Unify feedback patterns**: Migrate inline message banners to the Toast system where appropriate.
3. **Extract a reusable delete confirmation primitive** to replace the three duplicated implementations.
4. **Replace raw `<button>` elements** with the existing `<Button>` component across all pages.
5. **Move `normalizeUrl`/`isValidUrl`** from `pages/Sources.tsx` to `utils/url.ts`.
6. **Move `fetchFile`/`triggerDownload`** from `api/syntheses.ts` to a shared location.
7. **Use `createResource`** instead of `createEffect` + manual fetch for theme sources in ThemeManager.
8. **Strengthen `isApiError` type guard** with `typeof` checks on `status` and `message`.
9. **Hardcoded French strings** in SynthesisDetail NewsItemCard ("Lire la suite", "Reduire") must use `t()`.
### Priority Low
10. **Use `<Switch>/<Match>`** instead of multiple adjacent `<Show>` blocks for mutually exclusive states.
11. **Consolidate theme/schedule types** into `types.ts` alongside all other domain types.
12. **Memoize the date-sorted synthesis list** in Home.tsx instead of sorting inline in the `<For>`.
13. **Convert `SynthesisCard`** from a plain function to a proper `Component<>`.
14. **Use `let` instead of `createSignal`** for timer IDs that are not rendered in the UI.
---
## 10. What's Done Well
- **SSE utility** (`createSSEConnection`) is an exemplary SolidJS utility: wraps imperative APIs into reactive signals, handles reconnection with exponential backoff, and cleans up properly.
- **API client** is clean, type-safe, and handles all edge cases (FormData, 204, 401 redirect).
- **i18n system** is simple, type-safe (translation keys are a union type), and consistently used.
- **Toast system** with Portal rendering and auto-dismiss timers is well-implemented.
- **Button component** with variant/loading/icon props is a good reusable primitive.
- **Auth context** is minimal and correct -- derived signals like `isAdmin()` avoid redundant state.
- **Route-level code splitting** via `lazy()` in App.tsx -- good for initial load performance.
- **JSDoc comments** on pages and utilities are thorough and explain *why*, not just *what*.

396
docs/audit/v2-qa-report.md
Unescape Escape View File

@ -0,0 +1,396 @@
# AI Weekly Synth -- Test Coverage Audit Report (v2)
**Date:** 2026-03-27
**Auditor:** QA Engineer (automated analysis)
---
## 1. Test Inventory
### 1.1 Backend Unit Tests (`cargo test --lib`)
**Total: 358 tests** -- all passing.
| Source file | # tests | Coverage area |
|---|---|---|
| `services/scraper.rs` | 74 | SSRF IP checks, soft-404, redirect, HTML parsing |
| `services/synthesis.rs` | 36 | Pipeline logic, schema building, category overflow |
| `services/llm/anthropic.rs` | 20 | Response parsing, error handling |
| `services/prompts.rs` | 18 | Prompt template generation |
| `services/csv.rs` | 18 | CSV parsing, serialisation |
| `models/synthesis.rs` | 16 | Model validation, serialisation |
| `services/rate_limiter.rs` | 15 | Token bucket, concurrency |
| `services/llm/openai.rs` | 13 | Response parsing, error handling |
| `models/source.rs` | 12 | URL / title validation |
| `models/settings.rs` | 12 | Settings validation, defaults |
| `services/export.rs` | 12 | Markdown / PDF rendering |
| `services/llm/gemini.rs` | 10 | Response parsing, error handling |
| `models/provider.rs` | 10 | Provider / model validation |
| `services/email.rs` | 9 | Email rendering, bypass mode |
| `services/encryption.rs` | 8 | AES-256-GCM encrypt/decrypt |
| `services/source_scraper.rs` | 8 | Link extraction, is_article filter |
| `services/llm/schema.rs` | 8 | JSON schema generation |
| `util/token.rs` | 8 | Token generation, hashing |
| `models/api_key.rs` | 8 | API key validation |
| `middleware/csrf.rs` | 7 | CSRF header check |
| `models/rate_limit.rs` | 6 | Rate limit model validation |
| `config.rs` | 6 | Config parsing |
| `middleware/auth.rs` | 5 | Session extraction |
| `services/llm/factory.rs` | 5 | Provider factory |
| `handlers/admin.rs` | 4 | Admin handler validation |
| `services/brave_search.rs` | 1 | Brave search (minimal) |
| `services/llm/mock.rs` | 0 | Mock provider (no assertions) |
| `errors.rs` | 0 | Error types (no unit tests) |
### 1.2 Backend Integration Tests (`backend/tests/`)
**Total: 183 tests** across 17 files (requires Postgres).
| File | # tests | Coverage area |
|---|---|---|
| `api_sources_test.rs` | 36 | Sources CRUD, validation, CSV, bulk import, max limit |
| `api_admin_test.rs` | 30 | Provider CRUD, rate limits, user mgmt, audit log, config |
| `api_keys_test.rs` | 18 | API key CRUD, encryption, ownership, test endpoint |
| `api_syntheses_test.rs` | 17 | Synthesis CRUD, pagination, ownership, generation trigger |
| `api_auth_test.rs` | 16 | Register, login, verify, logout, session |
| `api_export_test.rs` | 13 | Email send, Markdown export, PDF export |
| `api_themes_test.rs` | 10 | Theme CRUD, validation, ownership |
| `api_schedules_test.rs` | 9 | Schedule CRUD, validation, ownership |
| `api_settings_test.rs` | 7 | Settings CRUD, defaults, boundary, isolation |
| `pipeline_test.rs` | 6 | Phase 1 extraction, Phase 2 search, overflow, diversity, dedup, preferred |
| `api_article_history_test.rs` | 4 | History list, clear, provenance |
| `api_csrf_test.rs` | 4 | CSRF header enforcement |
| `api_stop_generation_test.rs` | 4 | Stop job, ownership, 404 |
| `api_llm_logs_test.rs` | 3 | LLM logs auth, 404, happy path |
| `api_sources_preferred_test.rs` | 3 | Preferred sources set/clear/auth |
| `minimal_test.rs` | 2 | Infrastructure sanity (oneshot) |
| `api_health_test.rs` | 1 | Health check |
### 1.3 Frontend Unit Tests (`vitest`)
**Total: 141 tests** (131 passing, 10 failing) across 18 files.
| File | # tests | Coverage area |
|---|---|---|
| `sources-utils.test.ts` | 20 | Source utilities |
| `provider-info.test.ts` | 11 | Provider info helpers |
| `api-keys.test.ts` | 11 | API keys client |
| `synthesis-utils.test.ts` | 11 | Synthesis utilities |
| `sse.test.ts` | 11 | SSE client |
| `settings-validation.test.ts` | 3 | Settings validation |
| `i18n.test.ts` | 9 | i18n translations |
| `api-client.test.ts` | 7 | API client |
| `config-api.test.ts` | 7 | Config API |
| `pages/settings.test.tsx` | 10 | Settings page |
| `pages/sources.test.tsx` | 8 | Sources page |
| `pages/home.test.tsx` | 7 | Home page |
| `pages/generate.test.tsx` | 6 | Generate page |
| `synthesis-export.test.ts` | 6 | Export utilities |
| `pages/login.test.tsx` | 4 | Login page |
| `pages/register.test.tsx` | 4 | Register page |
| `auth-context.test.tsx` | 3 | Auth context |
| `admin-route-guard.test.tsx` | 3 | Admin route guard |
### 1.4 E2E Tests (Playwright)
**Total: 7 tests** across 7 files.
| File | Coverage area |
|---|---|
| `registration.spec.ts` | Full magic link registration flow |
| `settings.spec.ts` | Settings persistence across reloads |
| `settings-export.spec.ts` | Settings export/import roundtrip |
| `sources.spec.ts` | Source CRUD + preferred sources via API |
| `themes.spec.ts` | Theme CRUD + schedule CRUD via API |
| `admin-providers.spec.ts` | Admin provider management, settings dropdown |
| `generation-live.spec.ts` | Full pipeline with real OpenAI key (gated) |
---
## 2. Feature Coverage Matrix
| Feature | Unit Tests | Integration Tests | E2E Tests | Coverage |
|---|---|---|---|---|
| **Auth: register** | - | 4 tests | 1 test | GOOD |
| **Auth: login** | - | 3 tests | - | GOOD |
| **Auth: magic link verify** | - | 3 tests | 1 test | GOOD |
| **Auth: /me** | - | 3 tests | - | GOOD |
| **Auth: logout** | - | 3 tests | - | GOOD |
| **Auth: session expiry** | 5 (middleware) | - | - | PARTIAL |
| **CSRF protection** | 7 (middleware) | 4 tests | - | GOOD |
| **Settings CRUD** | 12 (model) | 7 tests | 2 tests | GOOD |
| **Sources CRUD** | 12 (model) | 36 tests | 1 test | GOOD |
| **Sources: CSV import/export** | 18 (csv) | 6 tests | - | GOOD |
| **Sources: preferred** | - | 3 tests | 1 test | GOOD |
| **Sources: max limit** | - | 2 tests | - | GOOD |
| **Themes CRUD** | - | 10 tests | 1 test | GOOD |
| **Schedules CRUD** | - | 9 tests | 1 test | GOOD |
| **Scheduled execution** | 0 | 0 | 0 | **NONE** |
| **Syntheses CRUD** | 16 (model) | 11 tests | - | GOOD |
| **Syntheses: pagination** | - | 2 tests | - | GOOD |
| **Syntheses: ownership isolation** | - | 2 tests | - | GOOD |
| **Generation: trigger** | - | 4 tests | 1 test (gated) | GOOD |
| **Generation: SSE progress** | 11 (sse client) | 0 | 1 test (gated) | PARTIAL |
| **Generation: stop** | - | 4 tests | 0 | GOOD |
| **Pipeline: Phase 1 (scrape)** | 74+8 (scraper) | 1 test | 1 test (gated) | GOOD |
| **Pipeline: Phase 2 (search)** | - | 1 test | 1 test (gated) | GOOD |
| **Pipeline: category overflow** | 36 (synthesis) | 1 test | - | GOOD |
| **Pipeline: is_article filter** | 8 (source_scraper) | 0 | 0 | PARTIAL |
| **Pipeline: summary_length** | 18 (prompts) | 0 | 0 | PARTIAL |
| **Pipeline: date extraction** | 0 | 0 | 0 | **NONE** |
| **Pipeline: article history dedup** | - | 1 test | - | GOOD |
| **Pipeline: source diversity cap** | - | 1 test | 1 test (gated) | GOOD |
| **Pipeline: preferred ordering** | - | 1 test | - | GOOD |
| **Pipeline: Brave Search** | 1 (minimal) | 0 | 0 | **WEAK** |
| **API keys CRUD** | 8 (model) | 18 tests | - | GOOD |
| **API keys: encryption at rest** | 8 (encryption) | 1 test | - | GOOD |
| **API keys: test endpoint** | - | 2 tests | - | GOOD |
| **Admin: provider CRUD** | 10 (model) + 4 (handler) | 9 tests | 1 test | GOOD |
| **Admin: rate limits** | 6 (model) + 15 (limiter) | 4 tests | - | GOOD |
| **Admin: user management** | - | 6 tests | - | GOOD |
| **Admin: audit log** | - | 2 tests | - | GOOD |
| **Config: providers** | - | 3 tests | - | GOOD |
| **Export: Markdown** | 12 (export) | 4 tests | - | GOOD |
| **Export: PDF** | 12 (export) | 4 tests | - | GOOD |
| **Export: Email** | 9 (email) | 5 tests | - | GOOD |
| **SSRF protection** | 74 (scraper) | 0 | 0 | PARTIAL |
| **LLM call logging** | - | 3 tests | 1 test (gated) | GOOD |
| **LLM providers (Gemini)** | 10 | - | - | GOOD |
| **LLM providers (OpenAI)** | 13 | - | - | GOOD |
| **LLM providers (Anthropic)** | 20 | - | - | GOOD |
| **Rate limiting** | 15 | 0 | 0 | PARTIAL |
| **Turnstile captcha** | bypass only | bypass only | bypass only | PARTIAL |
---
## 3. Coverage Gaps and Recommendations
### GAP-01: Scheduled Execution (scheduler.rs) -- No tests
**Priority: Critical**
The `run_scheduled_jobs()` function in `services/scheduler.rs` has zero unit tests and zero integration tests. This is a critical autonomous process that triggers generation and sends emails without user interaction.
**Tests to write:**
1. **Unit test** (`scheduler.rs`): `run_scheduled_jobs` triggers generation for themes whose schedule matches the current day+time.
2. **Unit test** (`scheduler.rs`): `run_scheduled_jobs` does NOT trigger generation for disabled schedules.
3. **Unit test** (`scheduler.rs`): `run_scheduled_jobs` does NOT trigger generation when the current day is not in the schedule's `days` list.
4. **Integration test** (`api_schedules_test.rs`): Create a schedule set to "now", verify the scheduler picks it up and a synthesis is created (or at least attempted with a mock provider).
5. **Integration test**: Verify that after `run_scheduled_jobs` executes, the schedule's `last_run_at` timestamp is updated.
---
### GAP-02: SSE Progress Stream -- No integration test
**Priority: High**
The SSE progress endpoint (`GET /api/v1/syntheses/generate/:job_id/progress`) is only tested in the E2E suite (gated behind a real API key). There is no integration test that verifies the SSE connection, event format, or error propagation.
**Tests to write:**
1. **Integration test** (`api_syntheses_test.rs`): Connect to SSE endpoint for a running job and verify the stream sends well-formed events (`progress`, `complete`, or `error`).
2. **Integration test**: SSE endpoint returns 404 for a non-existent job_id.
3. **Integration test**: SSE endpoint returns 401 without auth.
---
### GAP-03: Brave Search Pipeline -- Minimal coverage
**Priority: High**
The `services/brave_search.rs` has only 1 unit test. The Brave Search path in the pipeline (`use_brave_search: true`) is explicitly commented as untestable in `pipeline_test.rs` because it requires a real API key. The entire search-via-Brave code path is unverified in integration tests.
**Tests to write:**
1. **Unit test** (`brave_search.rs`): Parse a valid Brave Search API response and extract URLs.
2. **Unit test** (`brave_search.rs`): Handle Brave API error responses (429 rate limit, 401 invalid key).
3. **Unit test** (`brave_search.rs`): Handle malformed Brave API JSON response gracefully.
4. **Integration test** (`pipeline_test.rs`): Use wiremock to mock the Brave API endpoint and run the pipeline with `use_brave_search: true`, verifying that Brave results feed into the pipeline.
---
### GAP-04: Date Extraction / max_age_days Filtering -- No tests
**Priority: High**
The `max_age_days` field is on themes and is used to filter old articles. However, there are no tests (unit or integration) that verify articles older than `max_age_days` are excluded from the synthesis. The prompt includes date extraction instructions, but there is no test that validates article age filtering actually works.
**Tests to write:**
1. **Unit test** (`synthesis.rs`): Articles with a `published_at` date older than `max_age_days` are excluded.
2. **Integration test** (`pipeline_test.rs`): Set `max_age_days: 1` and provide wiremock articles with old dates, verify they are filtered out.
3. **Unit test** (`prompts.rs`): Verify the date extraction instruction appears in the prompt when `max_age_days > 0`.
---
### GAP-05: Rate Limiting -- No integration test
**Priority: High**
The rate limiter has 15 unit tests but zero integration tests. There is no test that verifies the rate limiter actually blocks LLM calls when the limit is exceeded in a real pipeline run, nor any test that the admin-configured rate limits are loaded and applied.
**Tests to write:**
1. **Integration test**: Configure a very low rate limit (e.g., `max_requests: 1, time_window_seconds: 60`), trigger generation with multiple sources, and verify the rate limiter introduces delays (or that the pipeline logs rate-limit waits).
2. **Integration test**: Verify that user-level rate limit overrides (from settings `rate_limit_max_requests`) are applied when set.
---
### GAP-06: SSRF Protection -- No integration test
**Priority: Medium**
The SSRF IP checks have 74 unit tests in `scraper.rs` (excellent is_private_ip coverage), but there is no integration test that verifies the full `check_ssrf` function actually blocks a request to a private IP through the scraper pipeline. The tests bypass SSRF with `SKIP_SSRF_CHECK=1`.
**Tests to write:**
1. **Integration test** (`pipeline_test.rs`): Add a source with a URL pointing to `127.0.0.1` (without `SKIP_SSRF_CHECK`), verify the scraper rejects it and the pipeline continues with other sources.
2. **Integration test**: Verify that redirect to a private IP is blocked (source URL redirects to `http://192.168.1.1`).
---
### GAP-07: Pipeline is_article Filter -- No end-to-end verification
**Priority: Medium**
The `is_article` heuristic in `source_scraper.rs` has 8 unit tests, but there is no integration test that verifies non-article pages (e.g., category index pages, about pages) are actually filtered out during a pipeline run.
**Tests to write:**
1. **Integration test** (`pipeline_test.rs`): Set up wiremock with a source page linking to both article pages and non-article pages (e.g., `/about`, `/contact`, `/category`), verify only articles make it into the synthesis.
---
### GAP-08: Pipeline summary_length -- No integration test
**Priority: Medium**
The `summary_length` field on themes controls the number of sentences in generated summaries. The prompts unit tests verify the instruction appears in the prompt, but no test verifies the LLM response is actually constrained to the requested length.
**Tests to write:**
1. **Integration test** (`pipeline_test.rs`): Use MockLlmProvider with `summary_length: 1` and verify the mock generates 1-sentence summaries (or verify the prompt includes the correct `summary_length` instruction).
---
### GAP-09: Settings Validation Rejections -- No negative tests
**Priority: Medium**
The settings integration tests cover boundary values but do not test rejection of out-of-range values. There are no tests for `max_articles_per_source: 0`, `batch_size: -1`, `max_links_per_source: 999`, etc.
**Tests to write:**
1. **Integration test** (`api_settings_test.rs`): `PUT /settings` with `max_articles_per_source: 0` returns 422.
2. **Integration test** (`api_settings_test.rs`): `PUT /settings` with `batch_size: 0` returns 422.
3. **Integration test** (`api_settings_test.rs`): `PUT /settings` with `max_links_per_source: 999` returns 422.
4. **Integration test** (`api_settings_test.rs`): `PUT /settings` with `article_history_days: -1` returns 422.
---
### GAP-10: Synthesis Export Content with Special Characters -- No test
**Priority: Low**
Export tests verify structure and content-type but do not test synthesis content with special characters (accented characters, HTML entities, emoji, very long URLs). There is no test for PDF generation with such edge cases.
**Tests to write:**
1. **Integration test** (`api_export_test.rs`): Insert a synthesis with UTF-8 characters, URLs with query strings, and long summaries. Export as Markdown and verify content integrity.
2. **Integration test** (`api_export_test.rs`): Same synthesis exported as PDF. Verify PDF magic bytes and non-empty content.
---
### GAP-11: Concurrent Generation -- No test
**Priority: Low**
The `generate_twice_returns_error_for_second` test verifies the same user cannot run two jobs. But there is no test for two *different* users generating simultaneously, which is the expected multi-tenant behavior.
**Tests to write:**
1. **Integration test** (`api_syntheses_test.rs`): Two users trigger generation simultaneously. Both should get 202. Verify they do not interfere with each other.
---
### GAP-12: Frontend -- Failing tests
**Priority: Medium**
10 of 141 frontend unit tests are currently failing. These failures need investigation and resolution before any new coverage work.
**Action:**
1. Run `npx vitest run` and capture the 10 failing test names.
2. Investigate root cause (likely stale mocks or component changes).
3. Fix or update the failing tests.
---
### GAP-13: Article History Ownership Isolation -- No test
**Priority: Medium**
The article history endpoint tests cover auth and empty-state, but there is no test verifying that User B cannot see User A's article history.
**Tests to write:**
1. **Integration test** (`api_article_history_test.rs`): User A generates a synthesis (creating history entries). User B calls `GET /article-history` and sees empty results.
2. **Integration test** (`api_article_history_test.rs`): User B calls `DELETE /article-history` and it does not affect User A's history entries.
---
### GAP-14: Provenance Ownership Isolation -- No test
**Priority: Medium**
The provenance endpoint test only covers the 404 case. There is no test verifying that User B cannot access User A's synthesis provenance.
**Tests to write:**
1. **Integration test** (`api_article_history_test.rs`): User A has a synthesis with provenance. User B calls `GET /syntheses/:id/provenance` on User A's synthesis and gets 404.
---
### GAP-15: E2E Coverage of Core Flows -- Missing
**Priority: Low**
Several core user journeys are only covered by integration tests, not E2E tests:
- Login flow (only registration has an E2E test)
- API key management (add, test, delete)
- Synthesis list/detail view
- Synthesis deletion
- Markdown/PDF export download
- Email send from synthesis view
**Tests to write (selected):**
1. **E2E test**: Login via magic link, verify session persists.
2. **E2E test**: Add an API key, verify it appears masked in the list, test it, delete it.
3. **E2E test**: View synthesis detail page, click export Markdown, verify download.
---
## 4. Summary
### Overall Assessment
| Metric | Value |
|---|---|
| Backend unit tests | 358 (all passing) |
| Backend integration tests | 183 |
| Frontend unit tests | 141 (10 failing) |
| E2E tests | 7 |
| **Total test count** | **689** |
| Critical gaps | 1 (scheduled execution) |
| High-priority gaps | 4 (SSE stream, Brave Search, date filtering, rate limiting integration) |
| Medium-priority gaps | 6 |
| Low-priority gaps | 3 |
### Strengths
- **SSRF protection** has the best unit test coverage in the project (74 tests) covering all private IP ranges, IPv4-mapped IPv6, and redirect blocking.
- **Sources CRUD** is the most thoroughly tested API endpoint (36 integration tests) including CSV import/export, bulk import, max limits, and boundary values.
- **Admin module** has comprehensive access control tests (access denied for non-admin, non-authenticated, each endpoint).
- **Ownership isolation** is consistently tested across all user-scoped endpoints (syntheses, sources, API keys, themes, schedules, stop generation).
- **Pipeline tests** use wiremock and MockLlmProvider to test the full generation flow without real API keys, covering scraping, classification, overflow, diversity, dedup, and preferred ordering.
- **Encryption verification** directly queries the database to confirm API keys are not stored in plaintext.
- **Audit logging** is verified by querying the audit_log table after admin actions.
- **E2E generation test** exercises the complete pipeline with a real OpenAI key including provenance and LLM log verification.
### Weaknesses
- **Scheduled execution** has zero test coverage -- a critical autonomous process.
- **Brave Search** pipeline path is untested beyond a single unit test.
- **Date extraction/age filtering** has no tests at any level.
- **Rate limiting** is well unit-tested but has no integration verification.
- **SSE progress stream** has no integration test (only gated E2E).
- **Frontend** has 10 failing tests that need immediate attention.
- **Settings validation** lacks negative boundary tests (rejection of invalid values).

503
docs/audit/v2-rust-expert-report.md
Unescape Escape View File

@ -0,0 +1,503 @@
# Rust Expert Audit Report v2 -- AI Weekly Synth Backend
**Date**: 2026-03-27
**Scope**: Full audit of backend Rust code focusing on idiomatic patterns, async correctness, type safety, memory/performance, safety, and newly added modules (scheduler, themes, schedules, preferred sources pipeline).
---
## Executive Summary
The codebase demonstrates strong Rust fundamentals for a learning project. Error handling is consistent, `unwrap()` is absent from production code, SQL injection is prevented via parameterized queries, and the async architecture is sound. The main areas for improvement are: (1) the `synthesis.rs` monolith function is ~850 lines with heavy code duplication between Phase 1 and Phase 2; (2) the type system underuses newtypes and enums where stringly-typed data could be statically checked; (3) several concurrency patterns have subtle correctness or performance gaps; (4) the scheduler has a safety regression with two bare `unwrap_or_default()` calls on serde deserialization.
**Severity Legend**: `[CRITICAL]` = correctness/data bug, `[HIGH]` = production risk, `[MEDIUM]` = code quality / maintainability, `[LOW]` = polish / best practice.
---
## 1. Idiomatic Rust
### 1.1 Ownership and Borrowing
**Positive**: The code avoids gratuitous `clone()` in most places. `Arc<dyn LlmProvider>` is correctly shared across spawned tasks. The `ArticleTrace<'a>` struct borrows data from the pipeline scope, avoiding allocation until `build_trace_entry` converts to owned strings for DB insertion.
**[MEDIUM] Redundant clone in category initialization (synthesis.rs:272-276)**
```rust
let user_categories = if theme_categories.is_empty() {
Vec::new()
} else {
theme_categories.clone()
};
```
This clone is unnecessary since `theme_categories` is already a `Vec<String>` owned by this scope. The `if` branch constructing an empty vec vs cloning is equivalent to just using `theme_categories` directly (it is already a potentially empty vec). The code could simply be `let user_categories = theme_categories;`.
**[MEDIUM] Excessive `.clone()` on source iteration (synthesis.rs:320-322)**
```rust
let preferred: Vec<_> = rotated_sources.iter().filter(|s| s.is_preferred).cloned().collect();
let non_preferred: Vec<_> = rotated_sources.iter().filter(|s| !s.is_preferred).cloned().collect();
```
This makes two full passes and clones every `Source`. A single `partition()` or `sort_by_key()` would halve the clones and passes:
```rust
let mut ordered_sources = rotated_sources;
ordered_sources.sort_by_key(|s| !s.is_preferred); // true sorts before false (reversed)
```
### 1.2 Error Handling
**Positive**: `AppError` is a well-designed `thiserror` enum with `IntoResponse` for Axum. The `From<sqlx::Error>` impl correctly hides DB details. `from` conversion on `anyhow::Error` provides the catch-all. All LLM providers map errors through `map_provider_http_error()` for consistency.
**[HIGH] Silent `.ok()` on DB writes with no fallback (synthesis.rs, multiple locations)**
Throughout the pipeline, `db::article_history::batch_insert_entries(...).await.ok()` and `db::llm_call_log::insert(...).await.ok()` silently discard DB errors. While the comment says "Don't fail synthesis if logging fails", there is no logging when these calls fail. If the DB goes down mid-pipeline, the user's synthesis completes but all traceability data is lost without any indication. At minimum, these should use `.map_err(|e| tracing::warn!(...))` or `.unwrap_or_else(|e| ...)` to log the failure.
**[MEDIUM] Validation returns `String` instead of structured errors**
`CreateThemeRequest::validate()`, `UpdateSettingsRequest::validate()`, and `UpsertScheduleRequest::validate()` all return `Result<(), String>`. Idiomatic Rust would use a dedicated `ValidationError` type (or at least `AppError::Validation`) to enable programmatic inspection and i18n. The current pattern requires callers to wrap the string:
```rust
req.validate().map_err(AppError::Validation)?;
```
This works but is fragile. Multiple validation failures cannot be reported at once.
### 1.3 Iterators and Functional Style
**Positive**: Good use of `filter_map`, `and_then`, chaining in `brave_search.rs`, `source_scraper.rs`, and the LLM response parsers. The `sanitize_json_null_bytes` recursive function is clean.
**[LOW] Manual loop where `Iterator::find` suffices (synthesis.rs:1229-1232)**
```rust
if !classification_categories.iter().any(|c| c.to_lowercase() == llm_category.to_lowercase()) {
llm_category = "Divers".to_string();
}
```
The repeated `to_lowercase()` on `classification_categories` for every article is O(n*m) string allocations. Pre-lowering once into a `HashSet<String>` at pipeline start would reduce this to O(1) lookup per article.
### 1.4 Trait Usage
**Positive**: The `LlmProvider` trait is well-designed -- minimal surface area, `Send + Sync` bounds on the trait, `async_trait` for object safety. The factory pattern in `factory.rs` is clean.
**[LOW] `LlmProvider` trait could benefit from a `name()` method alias**
`provider_id()` returns `&str`, which is good. However, the API key is stored as a `String` field directly in each provider struct. If a provider were ever `Clone`d, the API key would be duplicated in memory. Consider wrapping it in a `zeroize::Zeroizing<String>` (the `zeroize` crate is already a dependency).
---
## 2. Async Patterns
### 2.1 JoinSet Usage
**Positive**: The pipeline uses `JoinSet` correctly for parallel scraping and classification. Tasks are spawned, then drained with `join_next().await`. This is the right pattern for bounded concurrency.
**[HIGH] No concurrency limit on JoinSet spawns**
In Phase 1, *all* URLs in a wave's batch are scraped in parallel via JoinSet (synthesis.rs:472-482). If `max_links_per_source` is 30 and `source_extraction_window` is 10, that could be 300 simultaneous HTTP requests in a single JoinSet batch. Similarly, the link extraction phase (line 350-378) spawns all sources in a wave concurrently. There is no semaphore or concurrency cap.
**Impact**: Under load with many sources, this could exhaust file descriptors, trigger target-site WAFs, or cause reqwest connection pool contention.
**Recommendation**: Add a `tokio::sync::Semaphore` with a configurable max (e.g., 20 concurrent scrapes) to `JoinSet` tasks.
**[MEDIUM] JoinSet task panic handling**
If a task inside `scrape_set.spawn(...)` panics, `join_next().await` returns `Err(JoinError)`. The code handles this with `if let Ok(...)`, silently dropping panicked tasks. This is correct behavior (no crash propagation), but panics should at minimum be logged:
```rust
match join_result {
Ok(tuple) => { /* process */ }
Err(e) => tracing::error!("Task panicked: {}", e),
}
```
### 2.2 Cancellation
**Positive**: The cancellation mechanism via `Arc<AtomicBool>` is well-designed. The pipeline checks `cancelled.load(Ordering::Relaxed)` at wave boundaries and batch boundaries. On cancellation, it saves collected articles rather than discarding them.
**[MEDIUM] Cancellation during LLM calls is not preemptive**
Once an LLM call is in progress (`provider.call_llm(...)` inside a JoinSet task), cancellation cannot interrupt it. The user must wait for the LLM API response (up to 120s per call) before the cancellation flag is checked. This is a limitation of the current design, not a bug, but worth documenting. A `tokio::select!` with a cancellation token could abort waiting.
**[LOW] `Ordering::Relaxed` for cancellation flag**
`cancelled.store(true, Ordering::Relaxed)` and `cancelled.load(Ordering::Relaxed)` are used. `Relaxed` is correct here because:
- There is only one writer (the cancel endpoint) and one reader (the pipeline).
- There are no dependent memory operations that need ordering guarantees.
- The flag is checked at batch boundaries, so a few microseconds of delay in visibility is irrelevant.
This is fine. `Relaxed` is the right choice.
### 2.3 Spawned Tasks and Lifetimes
**[MEDIUM] Fire-and-forget cleanup spawn (synthesis.rs:239-242)**
```rust
tokio::spawn(async move {
tokio::time::sleep(Duration::from_secs(300)).await;
store.remove(&jid);
});
```
This spawned task lives for 5 minutes after every generation completes. If many generations finish, these accumulate. The task is untracked -- if the server shuts down, these are dropped mid-sleep (which is fine, but the job lingers in the store until process exit). Consider using `cleanup_expired()` on a timer instead of per-job spawns.
**[MEDIUM] Double-spawn panic handler pattern (generation.rs:87-109)**
The handler spawns a task, then spawns *another* task to monitor the first for panics. This works but is unusual. The inner spawn handles timeout, and the outer spawn catches panics. This could be simplified with `tokio::spawn(...).catch_unwind()` or by wrapping `AssertUnwindSafe` around the inner future. The current pattern is correct but adds an extra long-lived task per generation.
### 2.4 Scheduler
**[HIGH] Scheduler runs sequentially (scheduler.rs:43)**
```rust
for schedule in due {
// ...runs generation synchronously...
}
```
If multiple users have schedules due at the same minute, they run sequentially. A single slow generation (up to 15 minutes) blocks all subsequent scheduled jobs. This could cause significant delays.
**Recommendation**: Use `JoinSet` or `FuturesUnordered` with a concurrency limit (e.g., 3 concurrent scheduled jobs) to process them in parallel.
**[MEDIUM] Scheduler time comparison is string-based (scheduler.rs:29, schedules.rs:91)**
```rust
let time = chrono::Utc::now().format("%H:%M").to_string();
```
The SQL query uses `time_utc <= $2` as a string comparison. This works for `HH:MM` format because it is lexicographically ordinal. However, if the scheduler tick happens at `08:01` and the schedule was for `08:00`, the schedule runs. If the tick happens at `08:59`, the `08:00` schedule runs again -- but `last_run_at` date check prevents double execution. This is correct but relies on the `CURRENT_DATE` check. If the server restarts after midnight UTC but before the tick, a `23:30` schedule from the previous day would not run (missed execution). This is a known limitation of tick-based scheduling.
---
## 3. Type System
### 3.1 Newtype Gaps
**[MEDIUM] Stringly-typed identifiers**
The codebase passes `provider_name: &str`, `source_type: &str`, `status: &str`, `call_type: &str` as raw strings throughout the pipeline. These should be enums:
```rust
enum ProviderName { Gemini, OpenAI, Anthropic }
enum ArticleStatus { Used, FilteredHistory, FilteredDiversity, FilteredEmpty, FilteredTooOld, FilteredNotArticle, FilteredHomepage }
enum SourceType { PersonalizedSource, WebSearch, BraveSearch }
```
Currently, a typo in a status string (e.g., `"filtered_diversty"`) would silently produce incorrect data. Enums catch this at compile time.
**[MEDIUM] `serde_json::Value` as domain type**
`Theme.categories` is `serde_json::Value` rather than `Vec<String>`. Every access requires `serde_json::from_value(theme.categories).unwrap_or_default()`. This pattern appears in:
- `synthesis.rs:265`
- `models/theme.rs:104`
- `models/schedule.rs:89-92`
- `scheduler.rs:67,70`
Deserializing to `Vec<String>` at the DB layer (in `FromRow` or `TryFrom`) would eliminate this repeated pattern and make invalid states unrepresentable.
**[LOW] `i32` used for semantic integers**
`max_items_per_category`, `max_age_days`, `summary_length`, `batch_size`, `source_extraction_window` are all `i32`. These are always positive. Using a newtype like `NonZeroU32` or a validated wrapper would prevent negative values at the type level rather than at validation time.
### 3.2 Enum Design
**Positive**: `AppError` and `ProgressEvent` are well-designed tagged enums. `ProgressEvent` uses `#[serde(tag = "type")]` for clean JSON serialization.
**[LOW] `ProgressEvent::Error` should carry structured error info**
Currently `ProgressEvent::Error { message: String }`. Adding an optional error code would help the frontend differentiate between user-fixable errors (bad API key) and transient errors (rate limit).
---
## 4. Memory and Performance
### 4.1 Cloning
**[MEDIUM] `AppState` is cloned on every handler call**
`AppState` is `#[derive(Clone)]` and contains a `DashMap<Uuid, UserRateLimitEntry>` directly (not behind `Arc`). The `DashMap` itself is `Clone`, but it wraps `Arc` internally, so this is actually cheap. This is fine -- just worth noting that the `Clone` is O(1) due to internal `Arc`.
**[LOW] `sources.clone()` before rotation (synthesis.rs:318)**
```rust
let rotated_sources = rotate_sources(sources.clone(), last_source_url.as_deref());
```
`sources` is not used after this point, so `rotate_sources(sources, ...)` would avoid the clone. The function already takes ownership.
### 4.2 Allocations
**[MEDIUM] Repeated string allocations in hot path**
In the article processing loop, `to_lowercase()` is called on category names, source URLs, and domain names for every article. Pre-computing lowered versions at pipeline start would reduce allocations:
```rust
// At pipeline start:
let classification_categories_lower: HashSet<String> = classification_categories.iter()
.map(|c| c.to_lowercase()).collect();
```
**[LOW] `format!("category_{}", i)` repeated many times**
The pattern `format!("category_{}", i)` appears ~15 times across the pipeline. Pre-computing a `Vec<String>` of category keys at pipeline start would avoid redundant format allocations.
### 4.3 Arc Usage
**Positive**: `Arc` is used appropriately for shared data across spawned tasks:
- `Arc<watch::Sender<ProgressEvent>>` for progress channel
- `Arc<AtomicBool>` for cancellation flag
- `Arc<dyn LlmProvider>` for the LLM provider
- `Arc<String>` for `model_research` (shared across classify tasks)
- `Arc<Vec<String>>` for `classification_categories`
- `Arc<Value>` for `classify_schema`
These are all correct. The `Arc` is needed because the values cross `tokio::spawn` boundaries.
---
## 5. Safety
### 5.1 `unwrap()` in Production Code
**Positive**: No `unwrap()` calls exist in production (non-test) code paths. All `unwrap()` usage is confined to:
- `#[cfg(test)]` blocks
- `LazyLock` static initializers (`Selector::parse("a[href]").unwrap()`) -- these are infallible for valid CSS selectors and run once at startup.
**[HIGH] `unwrap_or_default()` on serde deserialization in scheduler (scheduler.rs:67,70)**
```rust
let emails: Vec<String> = serde_json::from_value(schedule.emails.clone()).unwrap_or_default();
// ...
let sections: Vec<NewsSection> = serde_json::from_value(synth.sections).unwrap_or_default();
```
While `unwrap_or_default()` is not `unwrap()`, these silently swallow malformed JSON. If `schedule.emails` is corrupted, no email is sent and no error is logged. The `sections` case is worse: a corrupted synthesis would send an empty email. These should at minimum log a warning:
```rust
let emails: Vec<String> = match serde_json::from_value(schedule.emails.clone()) {
Ok(e) => e,
Err(e) => {
tracing::warn!(schedule_id = %schedule.id, error = %e, "Failed to parse schedule emails");
continue; // or Vec::new()
}
};
```
### 5.2 SQL Safety
**Positive**: All queries use parameterized bindings (`$1`, `$2`, etc.) via sqlx. No string interpolation into SQL. The `find_due_schedules` query uses `to_jsonb($1::text)` for JSONB containment, which is safe.
**[LOW] Raw SQL in `resolve_model` (synthesis.rs:1419-1429)**
```rust
let model = sqlx::query_scalar::<_, String>(
r#"SELECT m->>'model_id' FROM admin_providers, jsonb_array_elements(models_scraping) AS m
WHERE provider_name = $1 AND is_enabled = true AND (m->>'is_default')::boolean = true LIMIT 1"#,
)
```
This is parameterized and safe, but uses a cross-join with `jsonb_array_elements` which could be expensive if `models_scraping` is large. In practice it is always small (< 10 models per provider), so this is fine.
### 5.3 Input Validation
**Positive**: All request types have `validate()` methods that check bounds. URL validation requires `http://` or `https://` scheme. Email validation in `UpsertScheduleRequest` checks for `@`.
**[MEDIUM] Email validation is too permissive (schedule.rs:62-64)**
```rust
if !email.contains('@') {
return Err(format!("Invalid email address: '{}'", email));
}
```
The `email_address` crate is already a dependency (used for auth registration). Scheduled email validation should use it for consistency:
```rust
if email_address::EmailAddress::parse(&email, None).is_err() { ... }
```
**[LOW] No URL validation on source URLs beyond scheme check**
`validate_url()` checks scheme and length but does not parse with `url::Url::parse()`. A string like `https://` (no host) passes validation but will fail at scrape time. Adding a `Url::parse()` check would catch these early.
### 5.4 SSRF Prevention
**Positive**: Comprehensive SSRF protection:
- DNS resolution before connecting (`check_ssrf`)
- IPv4/IPv6 private range checking including mapped addresses
- Redirect policy with per-hop DNS validation
- Documented TOCTOU limitation
**[LOW] `SKIP_SSRF_CHECK` env var bypass**
```rust
if std::env::var("SKIP_SSRF_CHECK").is_ok() {
return Ok(());
}
```
This is documented as being for integration tests, but the env check runs on every scrape call. In production, if this env var is accidentally set, all SSRF protection is disabled. Consider gating this with `#[cfg(test)]` or a compile-time feature flag.
### 5.5 Secret Handling
**Positive**:
- API keys are encrypted at rest with AES-256-GCM.
- `sanitize_error_message()` strips potential API key patterns from error messages sent to clients.
- Gemini provider specifically avoids logging the full URL (which contains the API key in a query parameter).
- `AppError::Internal` hides details from the client response.
**[MEDIUM] Gemini API key in URL query parameter**
```rust
fn api_url(&self, model: &str) -> String {
format!("...?key={}", self.api_key)
}
```
The API key is in the URL. If reqwest ever logs the URL at debug level, the key leaks to logs. The provider already mitigates this by logging only the error kind, not the full error (which includes the URL). However, any middleware or proxy that logs URLs would expose the key. Consider using a header-based auth approach if Gemini supports it, or ensuring the HTTP client never logs request URLs.
---
## 6. New Modules Analysis
### 6.1 Scheduler (`scheduler.rs`)
**Architecture**: Simple tick-based scheduler. An external caller (likely a periodic `tokio::spawn` with `tokio::time::interval`) calls `run_scheduled_jobs()` each minute.
**[HIGH] No jitter or distributed lock**
If multiple instances of the application run (e.g., horizontal scaling), all would execute the same scheduled jobs simultaneously because `find_due_schedules` has no locking. The `CLAUDE.md` says "single-tenant self-hosted", so this is likely acceptable, but the design should be documented as single-instance only.
**[MEDIUM] `mark_run` is called after generation completes**
If the server crashes during generation, the schedule is never marked as run. On restart, it would re-run. This is probably acceptable behavior (retry on failure), but could lead to duplicate syntheses if the first run partially succeeded and saved to DB.
### 6.2 Themes (`db/themes.rs`, `models/theme.rs`)
**Well-structured**: Clean CRUD operations with user_id scoping. The `COALESCE` pattern for partial updates is idiomatic SQL. The `TryFrom<Theme> for ThemeResponse` conversion handles the `serde_json::Value -> Vec<String>` deserialization.
**[LOW] `UpdateThemeRequest` lacks validation**
Unlike `CreateThemeRequest` which has a thorough `validate()` method, `UpdateThemeRequest` has no validation at all. Optional fields bypass all checks. If `max_items_per_category` is `Some(-1)`, it passes through to the DB unchecked.
### 6.3 Schedules (`db/schedules.rs`, `models/schedule.rs`)
**Well-structured**: The `ON CONFLICT (theme_id) DO UPDATE` upsert pattern is clean. `find_due_schedules` uses JSONB containment (`@>`) correctly.
**[LOW] `time_utc` is stored as `String` rather than a SQL `TIME` type**
Using a `TEXT` column for time means the DB cannot enforce format constraints. A `TIME` column would provide built-in validation and enable time arithmetic in queries.
### 6.4 Preferred Sources Pipeline Logic
**Well-designed**: The pipeline correctly:
1. Rotates sources to avoid always starting from the same one
2. Separates preferred sources (processed first) from non-preferred
3. Enforces per-source diversity limits (`max_articles_per_source`)
4. Checks article history for deduplication
5. Batches scraping and classification for efficiency
6. Tracks URL provenance for history
**[MEDIUM] Source rotation uses last URL, not last source index**
```rust
let last_source = db::article_history::get_last_source_url(&state.pool, user_id).await.unwrap_or(None);
let rotated_sources = rotate_sources(sources.clone(), last_source.as_deref());
```
If a source is deleted and re-added with a different URL, rotation breaks. If the last-used source URL changes (e.g., site redesign), the rotation starts from index 0. Using source ID instead of URL for rotation tracking would be more robust.
---
## 7. Code Structure and Maintainability
### 7.1 `synthesis.rs` Monolith
**[HIGH] `run_generation_inner` is ~800 lines with significant duplication**
The Phase 1 (personalized sources) and Phase 2 Brave Search path share nearly identical scrape+classify logic. The batch processing loop (scrape in parallel -> classify in parallel -> assign category -> track counts) is copy-pasted with minor variations (presence/absence of `source_url`).
**Recommendation**: Extract shared logic into helper functions:
- `scrape_and_classify_batch(urls, provider, model, schema, categories, ...)`
- `process_classify_response(response, categories, filled_counts, max_per_category, ...)`
This would reduce the function by ~300 lines and eliminate the risk of fixing a bug in one path but not the other.
### 7.2 `#[allow(clippy::too_many_arguments)]`
This attribute appears on `log_llm_call` and `build_search_prompt`. Functions with 8+ parameters are a code smell. Builder patterns or parameter structs would improve readability:
```rust
struct LlmCallLog { user_id: Uuid, job_id: Uuid, call_type: &str, ... }
```
---
## 8. Dependency Review
| Crate | Version | Notes |
|-------|---------|-------|
| `axum` | 0.8 | Current. Good. |
| `sqlx` | 0.8 | Current. Using runtime-checked queries (no compile-time verification). |
| `reqwest` | 0.12 | Current. |
| `tokio` | 1 | Full features. Fine for a backend server. |
| `dashmap` | 6 | Current. Used appropriately for concurrent maps. |
| `rand` | 0.8 | One major version behind (0.9 available). Not urgent. |
| `anyhow` + `thiserror` | 1 + 2 | Good combination: `thiserror` for structured errors, `anyhow` for internal catch-all. |
| `zeroize` | 1 | Present in dependencies but not used on API key strings. See recommendation above. |
| `async-trait` | 0.1 | Still needed for trait object dispatch. Rust 1.75+ has native async traits, but not for `dyn` dispatch. |
---
## 9. Summary of Findings by Severity
### CRITICAL (0)
None found.
### HIGH (5)
1. **No concurrency limit on JoinSet scrape/classify spawns** -- risk of resource exhaustion under load
2. **Silent `.ok()` on DB writes** -- traceability data loss without any logging
3. **Scheduler runs due jobs sequentially** -- one slow job blocks all others
4. **Scheduler `unwrap_or_default()` silently swallows malformed data** -- corrupted schedule sends no email with no warning
5. **`run_generation_inner` monolith** -- 800+ lines with duplicated Phase 1/Phase 2 logic; high bug introduction risk
### MEDIUM (12)
1. Redundant clone in category initialization
2. Excessive `.clone()` on source partitioning
3. Validation returns `String` instead of structured errors
4. `serde_json::Value` used as domain type for categories/days/emails
5. Stringly-typed identifiers (provider names, status codes, source types)
6. JoinSet task panic handling drops errors silently
7. Fire-and-forget cleanup spawn per job
8. Double-spawn panic handler pattern in generation handler
9. Scheduler time comparison and missed-execution edge case
10. Email validation too permissive in schedules
11. `Gemini API key in URL` -- logging risk
12. Source rotation based on URL rather than ID
### LOW (9)
1. Repeated `to_lowercase()` allocations in hot path
2. Repeated `format!("category_{}", i)` allocations
3. `sources.clone()` before rotation is avoidable
4. `ProgressEvent::Error` lacks structured error code
5. `UpdateThemeRequest` lacks validation
6. `time_utc` stored as String instead of SQL TIME
7. No URL parsing in `validate_url()`
8. `SKIP_SSRF_CHECK` env var bypass not compile-gated
9. `zeroize` unused on API key strings despite being a dependency
---
## 10. Recommendations (Prioritized)
1. **Extract scrape+classify batch logic** from `run_generation_inner` into a shared helper. This is the single highest-ROI refactor.
2. **Add a concurrency semaphore** to JoinSet-based scraping (e.g., `Semaphore::new(20)`).
3. **Log DB write failures** instead of using bare `.ok()`.
4. **Parallelize the scheduler** with bounded concurrency for due jobs.
5. **Replace `serde_json::Value` with typed fields** in `Theme.categories`, `ThemeSchedule.days`, `ThemeSchedule.emails`.
6. **Introduce enums for stringly-typed constants** (`ProviderName`, `ArticleStatus`, `SourceType`).
7. **Add validation to `UpdateThemeRequest`** to match `CreateThemeRequest`.
8. **Use `email_address` crate** for schedule email validation.
9. **Wrap API key strings in `Zeroizing<String>`** in LLM provider structs.
10. **Consider `#[cfg(test)]` gating** for `SKIP_SSRF_CHECK` instead of runtime env check.

318
docs/audit/v2-tech-lead-report.md
Unescape Escape View File

@ -0,0 +1,318 @@
# V2 Tech Lead Audit Report — AI Weekly Synth
**Date:** 2026-03-27
**Scope:** Full codebase (backend + frontend), complexity, duplication, readability, maintainability
---
## Executive Summary
The codebase is well-structured for a learning project and demonstrates solid engineering practices: clean error handling, SSRF protection, rate limiting, encryption at rest, and thorough test coverage for utility functions. However, organic growth has introduced one critical complexity hotspot (`synthesis.rs` at 2010 lines), significant frontend duplication between `Sources.tsx` and `ThemeManager.tsx`, and several patterns that will impede future development if not addressed.
**Priority:** 14 findings ranked P1 (do first) through P4 (nice to have).
---
## 1. Complexity Hotspots
### 1.1 [P1] `backend/src/services/synthesis.rs` — 2010 lines, God Function
**File:** `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs`
`run_generation_inner()` spans approximately **800 lines** (lines 246-1038). It handles initialization, source rotation, link extraction, article history filtering, preferred-source shuffling, batch scraping, LLM classification, date filtering, category assignment, Brave Search fallback, LLM web search fallback, final assembly, and database persistence — all in a single function.
**Specific issues:**
- **Deep nesting**: The wave loop (`'wave_loop`) contains a batch loop (`while !done`), which contains a JoinSet collection loop, which contains match arms with multiple `continue` branches. This is 4-5 levels of nesting.
- **Duplicated scrape+classify logic**: The Phase 1 scrape+classify block (lines 471-632) and the Brave Search scrape+classify block (lines 704-888) are near-identical. Both build a JoinSet, spawn scrape tasks, collect results, build another JoinSet for LLM classification, parse responses, check `is_article`, filter by date, handle no-date articles, and assign categories.
- **12 calls to `build_trace_entry()`** with the same boilerplate `ArticleTrace` struct construction scattered throughout.
- **7 flush-pending-traces blocks** (check `!pending_traces.is_empty()`, call `batch_insert_entries`, call `pending_traces.clear()`).
**Recommendation:** Extract into a pipeline module with distinct phases:
```
services/pipeline/mod.rs — orchestrator (run_generation_inner)
services/pipeline/phase1.rs — personalized source processing
services/pipeline/phase2.rs — web search fallback (Brave + LLM)
services/pipeline/classify.rs — shared scrape+classify batch logic
services/pipeline/tracing.rs — ArticleTrace builder + flush helper
services/pipeline/progress.rs — ProgressEvent + emit_progress
```
### 1.2 [P2] `backend/src/services/scraper.rs` — 1280 lines
**File:** `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scraper.rs`
This file is reasonably well-organized but large. The 600+ lines of tests (starting at line 678) constitute nearly half the file. The SSRF validation, HTML parsing, date extraction, and soft-404 detection are logically distinct concerns.
**Recommendation:** Move tests to `backend/src/services/scraper/tests.rs` using a `#[cfg(test)] mod tests;` pattern. Consider splitting the file into `scraper/ssrf.rs`, `scraper/html.rs`, `scraper/dates.rs` if it continues to grow.
### 1.3 [P3] `frontend/src/pages/ThemeManager.tsx` — 935 lines, monolithic component
**File:** `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/ThemeManager.tsx`
This single component manages 20+ signals, handles theme CRUD, source CRUD, bulk import, CSV import/export, preferred sources, category editing, and a schedule sub-component. The render function alone (lines 429-931) is 500 lines of JSX.
**Recommendation:** Extract sub-components:
- `ThemeContentForm` — name, topic, categories, max age/items, summary length
- `ThemeSourceList` — source list, add, delete, preferred toggle
- `ThemeImportExport` — CSV and bulk import sections
---
## 2. Code Duplication
### 2.1 [P1] Sources.tsx and ThemeManager.tsx — ~80% duplicated source management logic
**Files:**
- `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/Sources.tsx` (481 lines)
- `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/ThemeManager.tsx` (935 lines)
Nearly every source-management function in `ThemeManager.tsx` is a copy-paste of `Sources.tsx` with minor adaptations (adding `theme_id` parameter):
- `handleAddSource` — identical validation logic, same error handling pattern
- `handleDeleteClick` / `performDelete` — identical two-click confirmation with timer
- `handleExportCsv` / `handleImportCsv` — identical
- `handleBulkImport` — identical line parsing, same semicolon splitting
The JSX for source list rendering (star toggle, delete button, link display) is also duplicated.
**Recommendation:** Extract a `SourceManager` component that accepts an optional `themeId` prop. Both pages delegate to it. The `normalizeUrl` and `isValidUrl` functions are already exported from `Sources.tsx` and imported by `ThemeManager.tsx` — this pattern should extend to the full source management UI.
### 2.2 [P1] Synthesis pipeline: duplicated scrape+classify blocks
As noted in 1.1, the Phase 1 and Brave Search paths in `synthesis.rs` duplicate approximately 120 lines of scrape-then-classify logic. The only differences are:
- Phase 1 tracks `source_url` per article; Brave does not
- Phase 1 uses `(String, String, String, String)` tuples; Brave uses `(String, String, String)`
**Recommendation:** Create a `scrape_and_classify_batch()` function parameterized by source type and optional source URL. This eliminates the duplication and makes adding future search backends (e.g., Google Search, Bing) trivial.
### 2.3 [P2] Frontend error handling boilerplate — 40+ occurrences
The pattern `catch (err) { if (isApiError(err)) { setX(err.message) } else { setX(t('...')) } }` appears 40 times across 14 files. This is mechanical and could be simplified.
**Recommendation:** Create a `handleApiError(err, fallbackKey)` utility:
```typescript
function handleApiError(err: unknown, t: TFunction, fallbackKey: string): string {
return isApiError(err) ? err.message : t(fallbackKey);
}
```
### 2.4 [P3] Admin audit logging boilerplate
In `admin.rs`, every handler follows the same pattern: perform action, then call `db::audit::create_entry` with a `CreateAuditLog` struct. This is 5 occurrences, each ~15 lines.
**Recommendation:** Consider an audit middleware or macro that captures the action, target type, and details from the handler return value.
---
## 3. Readability
### 3.1 [P2] French/English mixing in backend code
User-facing strings in `synthesis.rs` and `prompts.rs` are hardcoded in French:
- Progress messages: `"Chargement des parametres..."`, `"Analyse des sources personnalisees..."`
- Error messages: `"Aucun article valide trouve. Verifiez vos sources et categories."`
- Prompt text: entire system/user prompts in French
Meanwhile, code comments, doc strings, log messages, and error variants are in English. This inconsistency makes it harder for non-French speakers to contribute and prevents future i18n.
**Recommendation:** Move all user-facing strings to constants or a backend i18n module. Keep code, comments, and logs in English.
### 3.2 [P3] `#[allow(clippy::too_many_arguments)]` — 3 occurrences
**Files:** `synthesis.rs`, `prompts.rs`, `llm_call_log.rs`
These suppressions indicate functions with parameter counts exceeding Clippy's threshold (typically 7+). They are code smells signaling that parameters should be grouped into structs.
- `build_search_prompt` takes 9 parameters
- `log_llm_call` takes 10 parameters
- `insert` in `llm_call_log.rs` takes 10 parameters
**Recommendation:** Introduce parameter structs:
```rust
struct SearchPromptParams<'a> {
theme: &'a str,
categories: &'a [String],
max_items_per_category: i32,
// ...
}
```
### 3.3 [P4] Magic strings for category keys
Category keys like `"category_0"`, `"category_autre"`, `"category_no_date"` are used as HashMap keys throughout `synthesis.rs` and in `schema.rs`. These appear as raw string literals in ~15 places.
**Recommendation:** Define constants or an enum:
```rust
const CATEGORY_OTHER: &str = "category_autre";
const CATEGORY_NO_DATE: &str = "category_no_date";
fn category_key(index: usize) -> String { format!("category_{}", index) }
```
---
## 4. Maintainability Risks
### 4.1 [P2] Tight coupling between synthesis pipeline and database
`run_generation_inner()` directly calls `db::settings::get_or_create_default`, `db::themes::get_by_id`, `db::sources::list_for_user`, `db::article_history::*`, `db::llm_call_log::insert`, `db::syntheses::create`, and a raw `sqlx::query_scalar` (line 1419-1429 for `resolve_model`). The function takes `AppState` which bundles the database pool, HTTP client, job store, and rate limiters.
**Impact:** Unit testing the pipeline logic requires either a real Postgres database or a complete mock of `AppState`. The existing E2E tests use a mock LLM provider (good) but still need Postgres (expensive).
**Recommendation:** Introduce a `PipelineContext` trait or struct that abstracts data access. This would allow testing the orchestration logic with in-memory implementations.
### 4.2 [P2] Raw SQL inline in `resolve_model()`
**File:** `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs`, lines 1419-1429
```rust
let model = sqlx::query_scalar::<_, String>(
r#"SELECT m->>'model_id' FROM admin_providers, ..."#,
)
```
This is the only place in the service layer that contains raw SQL. All other queries go through the `db/` module, maintaining a clean separation. This breaks the pattern.
**Recommendation:** Move to `db::providers::get_default_scraping_model(pool, provider_name)`.
### 4.3 [P3] LLM provider implementations share identical HTTP error handling
Each of the three providers (`gemini.rs`, `openai.rs`, `anthropic.rs`) implements the same pattern:
1. Build request body (provider-specific)
2. Send HTTP request
3. Map network errors with `is_timeout()` / `is_connect()` classification
4. Parse response JSON
5. Check HTTP status and map errors
6. Extract content from provider-specific response structure
Steps 2-4 are identical across all three providers (~20 lines each). Only steps 1, 5, and 6 differ.
**Recommendation:** Extract a `send_llm_request()` helper in `llm/mod.rs`:
```rust
async fn send_llm_request(
client: &reqwest::Client,
url: &str,
body: &Value,
headers: &[(String, String)],
provider_name: &str,
) -> Result<(u16, Value), AppError>
```
### 4.4 [P3] `Providers.tsx` — 854 lines, complex inline state management
**File:** `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/admin/Providers.tsx`
The admin Providers page manages local editable copies of provider state in a `Record<string, ProviderFormState>` map, with functions for model array manipulation (add, remove, toggle default), scraping vs. websearch model lists, and inline validation. This is the most complex admin page and would benefit from splitting the model list editor into a reusable `ModelListEditor` component.
### 4.5 [P4] `Settings.tsx` — 694 lines, growing form complexity
**File:** `/Users/oabrivard/Projects/rust/ai_synth/frontend/src/pages/Settings.tsx`
The settings page has already been partially decomposed (`SettingsBraveSearch`, `SettingsRateLimit`, `ApiKeyManager`), which is good. The remaining monolithic JSX sections (provider selection, model dropdowns, import/export) could follow the same pattern for consistency.
---
## 5. Simplification Opportunities
### 5.1 [P3] `Sources.tsx` may be dead code
With the introduction of `ThemeManager.tsx`, which subsumes all source management under themes, the standalone `Sources.tsx` page may no longer be reachable by users. It is still registered in the router, but if all sources must now belong to a theme, the standalone page serves no purpose.
**Action:** Verify whether `Sources.tsx` is still linked in the navigation. If not, remove it and its route to eliminate 481 lines of duplicated code.
### 5.2 [P3] `list_for_user` query branch duplication
**File:** `/Users/oabrivard/Projects/rust/ai_synth/backend/src/db/sources.rs`, lines 15-44
The function has two nearly identical SQL queries — one with `AND theme_id = $2` and one without. The only difference is the optional WHERE clause.
**Recommendation:** Use a single query with a conditional clause:
```rust
sqlx::query_as::<_, Source>(
"SELECT ... FROM sources WHERE user_id = $1 AND ($2::uuid IS NULL OR theme_id = $2) ORDER BY ..."
)
.bind(user_id)
.bind(theme_id)
```
### 5.3 [P4] `bulk_create` uses sequential inserts instead of batch
**File:** `/Users/oabrivard/Projects/rust/ai_synth/backend/src/db/sources.rs`, lines 97-127
Sources are inserted one by one in a loop. For bulk imports of 50-100 sources, this generates 50-100 round-trips to the database.
**Recommendation:** Use `sqlx`'s batch insert or build a single `INSERT ... VALUES ($1, $2), ($3, $4), ...` query. This is a performance optimization, not a correctness issue.
### 5.4 [P4] Hardcoded snippet sizes
In `synthesis.rs`, the snippet size is computed from `summary_length` in two separate places (Phase 1 at line 437 and Brave at lines 766-770):
```rust
let snippet_size = match theme.summary_length { 1 => 500, 2 => 2000, _ => 4000 };
```
**Recommendation:** Extract to a function `fn snippet_size_for_length(summary_length: i32) -> usize`.
---
## 6. File Size Summary
### Backend (top 10 by line count)
| File | Lines | Assessment |
|------|-------|------------|
| `services/synthesis.rs` | 2010 | Needs decomposition (P1) |
| `services/scraper.rs` | 1280 | Acceptable, extract tests |
| `services/rate_limiter.rs` | 471 | Clean |
| `services/llm/anthropic.rs` | 471 | Minor shared-code opportunity |
| `services/export.rs` | 459 | Clean |
| `handlers/admin.rs` | 438 | Audit boilerplate |
| `models/synthesis.rs` | 416 | Clean |
| `services/email.rs` | 384 | Clean |
| `handlers/auth.rs` | 381 | Clean |
| `services/llm/openai.rs` | 373 | Minor shared-code opportunity |
### Frontend (top 10 by line count)
| File | Lines | Assessment |
|------|-------|------------|
| `pages/ThemeManager.tsx` | 935 | Needs decomposition (P1/P3) |
| `pages/admin/Providers.tsx` | 854 | Extract ModelListEditor (P3) |
| `pages/Settings.tsx` | 694 | Partially decomposed, continue (P4) |
| `pages/SynthesisDetail.tsx` | 548 | Acceptable |
| `pages/Sources.tsx` | 481 | Possibly dead code (P3) |
| `pages/GenerateSynthesis.tsx` | 471 | Clean |
| `i18n/fr.ts` | 462 | Expected size for translations |
| `pages/ArticleHistory.tsx` | 371 | Clean |
| `pages/Home.tsx` | 345 | Clean |
| `components/settings/SettingsSchedule.tsx` | 286 | Clean |
---
## 7. Positive Observations
These aspects of the codebase are well-executed and should be preserved:
1. **Error handling**: `AppError` enum with `IntoResponse` is clean, consistent, and hides internal details. Tests verify that secrets are never leaked.
2. **Security**: SSRF prevention with DNS resolution checks, AES-256-GCM encryption for API keys, CSRF via `X-Requested-With`, timing-attack mitigation in auth, and sensitive data scrubbing in error messages.
3. **LLM provider abstraction**: The `LlmProvider` trait + factory pattern makes adding new providers straightforward.
4. **Documentation**: Module-level `//!` doc comments on every file, function-level `///` doc comments with examples, and clear CLAUDE.md project instructions.
5. **Frontend component extraction**: `SettingsBraveSearch`, `SettingsRateLimit`, `SettingsSchedule`, and `ApiKeyManager` demonstrate good instincts for decomposition.
6. **Type safety**: Frontend `types.ts` is clean, well-organized, and provides `isApiError` type guard.
7. **Test coverage**: Unit tests for error handling, SSRF checks, URL normalization, job store, role validation, and CSV parsing.
---
## 8. Prioritized Action Plan
| Priority | Item | Effort | Impact |
|----------|------|--------|--------|
| **P1** | Decompose `synthesis.rs` into pipeline module (1.1) | Large | Reduces complexity, enables testing |
| **P1** | Extract shared `SourceManager` component (2.1) | Medium | Eliminates ~300 lines of duplication |
| **P1** | Extract shared scrape+classify function (2.2) | Medium | Eliminates ~120 lines of duplication |
| **P2** | Move hardcoded French strings to constants (3.1) | Medium | Enables future i18n, improves consistency |
| **P2** | Frontend error-handling helper (2.3) | Small | Reduces boilerplate in 14 files |
| **P2** | Abstract data access from pipeline (4.1) | Large | Enables unit testing without Postgres |
| **P2** | Move inline SQL from `resolve_model` to db module (4.2) | Small | Maintains architecture consistency |
| **P2** | Extract scraper tests to separate file (1.2) | Small | Improves file navigation |
| **P3** | Decompose `ThemeManager.tsx` into sub-components (1.3) | Medium | Improves readability |
| **P3** | Introduce parameter structs for long signatures (3.2) | Small | Removes clippy suppressions |
| **P3** | Define category key constants (3.3) | Small | Prevents typo bugs |
| **P3** | Audit whether `Sources.tsx` is dead code (5.1) | Small | Potential -481 lines |
| **P3** | Consolidate LLM HTTP request handling (4.3) | Medium | Reduces duplication across 3 files |
| **P4** | Batch insert for `bulk_create` (5.3) | Small | Performance improvement |
Write Preview
Loading…
Cancel
Save