generated from john/python-template
81 lines
6.8 KiB
Markdown
81 lines
6.8 KiB
Markdown
You are working in the `transcription` repository (Python 3.12+, FastAPI, NiceGUI, SQLModel/SQLAlchemy, Pydantic V2, asyncio). Follow `.github/instructions/ui.instructions.md` and `.github/instructions/services.instructions.md` for any code you touch, and keep `docs/ver4/*` as canonical authority for intended behavior. Do not modify unrelated code.
|
|
|
|
**Prerequisite:** This prompt assumes Phases 1-3 (`docs/phase1-codex-prompt.md`, `docs/phase2-codex-prompt.md`, `docs/phase3-codex-prompt.md`) are already merged.
|
|
|
|
## Goal
|
|
|
|
Implement **Phase 4 (Consolidation & refactoring)** from `docs/architecture-code-review-2026-08-20.md`, based on section 7 (Duplication & Consolidation Report). This phase is behavior-preserving refactoring: no functional change should be introduced, only reduction of duplication. Every extraction must be covered by the existing test suite passing unchanged (plus any new unit tests for the extracted helper itself).
|
|
|
|
### 1. Extract shared UI action/error helper
|
|
**Locations with the duplicated pattern:** `src/transcription/ui/pages/documents_page.py`, `src/transcription/ui/pages/jobs_page.py`, `src/transcription/ui/pages/people_page.py`, `src/transcription/ui/pages/settings_page.py`, `src/transcription/ui/pages/sources_page.py`
|
|
|
|
**Problem:** These pages repeat a `try/except Exception -> show_error(...)` wrapper pattern around UI actions. Estimated 50-80 duplicated lines.
|
|
|
|
**Required work:**
|
|
- Read `src/transcription/ui/components/error_presenter.py` to understand the existing error-presentation primitives (e.g. `show_error`) and NiceGUI conventions already in use.
|
|
- Add a shared helper in `error_presenter.py` (or another UI components module per `.github/instructions/ui.instructions.md` ownership rules) with a signature similar to:
|
|
```python
|
|
async def run_ui_action(*, operation: str, title: str, action: Callable[[], Awaitable[T]]) -> T | None:
|
|
...
|
|
```
|
|
Adjust the exact signature/name as needed to fit the existing calling conventions across the five pages (e.g. some call sites may need access to a spinner/notification/loading state — inspect each site before finalizing the signature).
|
|
- Replace each duplicated `try/except Exception -> show_error(...)` block across the five listed pages with a call to the new shared helper, preserving exact existing behavior (same error messages, same UI state transitions, same logging if any).
|
|
- Add a focused unit test for the new helper (success path, exception path, and confirm it surfaces the same error message/formatting the old inline blocks did).
|
|
|
|
### 2. Consolidate registry/read-model adapter shapes
|
|
**Locations:** `src/transcription/services/documents.py`, `src/transcription/services/people.py`, `src/transcription/api/v4_documents.py`
|
|
|
|
**Problem:** Repeated registry/read-model adapter shapes for document/person registries. Estimated 20-35 duplicated lines.
|
|
|
|
**Required work:**
|
|
- Compare the registry/read-model adapter code in `documents.py` and `people.py` (and how `v4_documents.py` consumes it) to identify the common shape.
|
|
- Introduce a shared abstraction near `src/transcription/services/registry.py` (or a new `src/transcription/api/read_models.py` if the duplication is primarily API-layer) — the review suggests something like:
|
|
```python
|
|
@dataclass(frozen=True)
|
|
class RegistrySummary[ModelT]:
|
|
...
|
|
```
|
|
Adapt the generic shape to what the actual duplicated code needs (inspect both usages first — do not force-fit a generic that doesn't match real field/behavior overlap).
|
|
- Refactor `documents.py`, `people.py`, and `v4_documents.py` to use the shared abstraction, preserving exact existing read-model output (field names/types/values returned to callers/API responses must not change).
|
|
|
|
### 3. Consolidate storage-error wrapping around file persistence
|
|
**Locations:** `src/transcription/services/store.py`, `src/transcription/ui/homepage_store.py`, `src/transcription/services/people.py`
|
|
|
|
**Problem:** Repeated storage-error wrapping around file persistence. Estimated 20-30 duplicated lines.
|
|
|
|
**Required work:**
|
|
- Identify the common file-persistence + error-wrapping pattern across the three locations.
|
|
- Introduce a higher-level wrapper in `src/transcription/services/media_storage.py`, e.g. along the lines of:
|
|
```python
|
|
async def persist_named_media(..., error: type[AppError], root: Path, namespace: str | None = None) -> Path:
|
|
...
|
|
```
|
|
Adjust the signature to match the real parameters used at each call site (inspect all three before finalizing).
|
|
- Refactor the three call sites to use the shared wrapper, preserving exact existing error types/messages raised on failure (this matters especially given Phase 3's error-taxonomy work — make sure this consolidation uses whatever the post-Phase-3 canonical/internal error categories are, not the pre-Phase-3 ones).
|
|
|
|
### 4. Perform an orphaned/dead-code sweep (behavior-preserving)
|
|
**Scope:** `src/transcription/**` and closely related tests/docs touched by this phase.
|
|
|
|
**Problem:** Prior reviews have found orphaned code blocks/files that are no longer referenced, which increases maintenance burden and can hide drift.
|
|
|
|
**Required work:**
|
|
- Identify candidate orphaned code (modules/functions/classes) with zero inbound references.
|
|
- Treat these as potential orphans only after checking expected exceptions: app entrypoints, framework/plugin registration paths, dynamic imports/reflection, CLI hooks, and test-only utilities.
|
|
- Verify each candidate with deterministic repository checks (e.g. `rg` import/call-site search plus relevant runtime/tests for the owning area), rather than assumption.
|
|
- For each confirmed orphan:
|
|
- remove it if safe and truly unused, or
|
|
- keep it with a short justification where dynamic wiring or contract requirements make static references incomplete.
|
|
- Include a concise "orphaned code audit" section in the final report listing:
|
|
- confirmed removed orphans,
|
|
- intentionally retained candidates and justification,
|
|
- any uncertain candidates that require follow-up.
|
|
|
|
## Validation
|
|
|
|
- Run `pytest` (via the project's normal invocation, e.g. `uv run pytest`) after each consolidation step and ensure the full suite still passes unchanged — this is a refactor, so a full-suite regression is the primary correctness signal.
|
|
- Run `ruff check` and `ty check` and ensure no new issues are introduced.
|
|
- Do not change observable behavior anywhere (error messages, HTTP status codes, UI states, persisted values) as part of this consolidation — if you find you need a behavior change to complete an extraction cleanly, stop and flag it rather than silently changing behavior.
|
|
- Do not touch provenance/env config or ruff/ty baseline cleanup — that is Phase 5.
|
|
|
|
Report back with: the final helper signatures actually implemented, all files changed per consolidation area, approximate lines removed per area (compare against the review's estimates), and final `pytest`/`ruff`/`ty` results.
|