Phase 1 of docs/reviews/2026-08-23-code-review.md.
HIGH-01: process_queued_job committed page evidence and the terminal job
status in separate transactions, so a crash between them left a transcript
persisted against a job stuck in PROCESSING that the worker never reclaims.
The final page's write is now deferred into _finalize_batch_outcome so it
shares the terminal transaction. Intermediate pages remain individually
durable, and the terminal commit is shielded against cancellation the same
way per-page writes already were.
HIGH-04: added tests/integration/test_pipeline_atomicity.py covering both
Transaction B and Transaction C. Confirmed failing against the previous
implementation before the fix.
HIGH-03: classify_unexpected_error interpolated the raw exception into
AppError.message, which the UI renders and the API serializes, leaking the
database path from OperationalError. message is now generic. Because message
also feeds format_error_detail, which writes evidence records, the root cause
is preserved on a new internal-only AppError.detail field rather than
discarded.
HIGH-02: replaced 8 hand-rolled ui.notify error calls in home_page and
people_page with error_presenter.show_error, restoring the correlation
error_id, canonical category, and suggestion. Added an AST guard to
test_ui_boundaries.py so pages cannot hand-roll error notifications again.
Docs updated per documentation-sync: the message/detail split in
docs/error_handling.md and the multi-page atomicity rule in
services.instructions.md.
Verification: ruff clean, 381 tests passing, ty unchanged at 10 known
SQLAlchemy descriptor false positives.
Co-authored-by: Copilot App <[email protected]>
Completes the table consolidation deferred within Phase 5. Section 4 of the
review lists table construction as a duplication pattern; three call sites had
been left bypassing the canonical builder.
- table/common.py: build_table gains a row_key parameter so callers with a
non-"id" primary key can use it.
- linked_people.py: replaces its hand-rolled ui.table with build_table
(row_key="person_id", selection="multiple", rows_per_page=0, no search).
- print_preview_page.py: _render_metadata_table and _render_job_table now share
a local _render_print_table helper. Print tables deliberately do not use
build_table - they must never paginate or render a search box, and they carry
print-only styling. The helper docstring records that rationale.
- tests/test_ui_boundaries.py: new AST guard asserting ui.table() is called from
exactly two modules - components/table/common.py and pages/print_preview_page.py.
Also closes the intermittent tests/ui/test_jobs_page.py failure observed once
after Phase 5 as environmental. Unreproduced across ~54 sequential full-suite
runs (including a 25-run soak) and 5 concurrent-process runs. No code change.
Verification: ruff check src tests clean; 275 passed, 4 skipped.
Co-authored-by: Copilot App <[email protected]>
Fixes the three ui.instructions.md violations recorded as [HIGH-07] and extracts
the page-level duplication catalogued in review section 4.
Boundary violations
- jobs_page no longer imports session_scope or manages a transaction.
store.create_document_job and store.create_job_for_document accept an optional
session_factory and open their own session scope when the caller supplies
neither a session nor a factory.
- sources_page no longer calls sqlalchemy.inspect. SourceService
.read_latest_execution_attempt now returns a LatestExecutionAttempt read model
carrying a plain transport_body_deferred flag, so ORM loader state stays inside
the service. Rendered output is unchanged.
- Deletes ui/components/document_panzoom.py, its export, and its CSS. The
component was exported but used by no page. Pan-zoom is planned for a clean
reintroduction in V4.7 alongside the other photo/image work.
Extracted duplication
- ui/components/media_urls.py: pure upload-URL resolution taking upload_dir and
base_url, replacing two identical ~60-line copies in sources_page and
people_page.
- ui/components/guards.py: parse-then-render-terminal-message, replacing 28
hand-written guard labels across five pages.
- ui/components/confirm_delete.py: the blocked-dependency notice and the
delete/cancel action row, from four delete pages.
- ui/components/upload_panel.py: the auto-uploading file picker, from three
pages. Source accept lists now derive from services.source_media
.SOURCE_EXTENSIONS instead of being hard-coded.
- ui/components/table/registry.py: the two hand-rolled label-registry tables on
the settings page now go through build_table, which gained selection and
rows_per_page options.
- ui/components/formatters.py gains parse_uuid and parse_iso_date, replacing
five and two private copies.
- ui/runtime.py owns resolve_runtime_settings, replacing three copies and
removing get_settings from every page module.
[LOW-05]
- Upload handlers are annotated with events.UploadEventArguments.
- The Document and Person form builders return DocumentFormFields and
PersonFormFields dataclasses instead of dict[str, Any].
Verification
- tests/test_ui_boundaries.py asserts no page imports a session scope, a session
factory, get_settings, sqlalchemy, or sqlmodel, and that no component imports
request or application state.
- 275 passed, 4 skipped. ruff check clean.
Findings: HIGH-07, LOW-05
Co-authored-by: Copilot App <[email protected]>
Jim LancasterandCopilot App