Repository navigation
Conversation
pilipilisbot
left a comment
There was a problem hiding this comment.
Reviewed the dashboard read model refactor against the persistence/read-only contracts. The split to DashboardQueries keeps HTTP/CLI/monitor orchestration off raw SQL, JobListFilters binds values through an allowlisted filter shape, and status/health now reports migration problems without creating or mutating the DB.
Checked the changed backend/CLI/monitor call sites, dashboard_data query paths, schema assumptions, and the new regression tests. Local validation from a dedicated clean worktree:
- pytest -q tests/test_dashboard_queries.py tests/test_backend.py -k 'dashboard or job_list or jobs_can_filter or order_index' -> 76 passed, 1 deselected, 1 known Starlette/httpx warning
- pytest -q -> 529 passed, 1 known Starlette/httpx warning
No blocking findings from my review.
|
@giscebot resol els conflictes |
Co-authored-by: ecarreras <ecarreras@users.noreply.github.com>
b7d39b4 to
0eaae6c
Compare
|
Conflictes resolts al commit He rebasat el commit del read model sobre la base actual Validació:
També he actualitzat el recompte de tests de la descripció del PR. |
|
Post-merge sync completed for #284. Verified PR state: Workspace cleanup:
No code changes were made. |
|
Post-merge cleanup correction: the dedicated PR worktree was still present at (the earlier scan only checked the naming). It was clean, the remote branch was already gone, and it has now been removed and pruned. The merged commit remains ; no repository code or PR metadata was changed. |
|
Correction to the cleanup note above: the dedicated PR worktree was still present at |
|
Verificació post-merge feta sobre el merge exacte
L’aprovació anterior continua sent vàlida i no cal cap acció addicional. |
Summary
DashboardQueriesas the read-only persistence boundary consumed by FastAPI handlers, CLI job/status commands, monitoring, and session streaming{column: value}SQL composition with typed, allowlistedJobListFilterstable_exists()/column_exists()compatibility branches now that migration history is validated explicitlyStack
Depends on #283, which makes migration validation explicit and non-mutating. This PR intentionally targets
fix/issue-260-explicit-migrations; after #283 merges, its base can be retargeted tomainwithout changing this diff.Validation
pytest -q— 535 passed, 1 pre-existing Starlette/httpx deprecation warningEXPLAIN QUERY PLANregression still provesidx_jobs_dashboard_orderis used without a temporary ORDER BY B-treeRisk
The read model now assumes the current migrated schema. Health/status reports
schema_ok=falsefor missing or pending migration history; normal queries no longer guess around partial schemas. Public module-level query functions remain as compatibility shims for Python callers.Requested by: @ecarreras
Related to #260