V4.6 Phase 1: deletions and quick wins

Pure remediation; no behavior change. Every item traces to a finding in
docs/architecture_code_review_2026-08-17.md.

Deletions
- Delete app_state.py, which had zero importers and whose get_session_factory
  raised TypeError at runtime [HIGH-01].
- Delete services/transcription.py and point build_prompt_execution imports at
  services/sources.py; drop the store.py compatibility aliases [MED-05].
- Delete ServiceBase.queue and its unparameterized asyncio.Queue [MED-07].
- Delete db/operations.get_next_queued_job, a divergent duplicate [CRIT-01].
- Drop the discarded load_docs parameter from list_jobs [LOW-03].

Config
- Delete worker_retry_backoff_seconds; no backoff behavior existed anywhere, so
  wiring it would have been a new feature [MED-02].
- Wire sqlite_check_same_thread through get_engine. The engine hardcoded the
  setting's own default, so this preserves behavior exactly [MED-02].
- Replace DATABASE_URL in docker-compose.yml with the nested DATABASE__DRIVER /
  DATABASE__PATH names. Settings uses env_nested_delimiter with extra="ignore",
  so DATABASE_URL was silently discarded [MED-10].

UI
- Move the 23KB inline VIBESCRIBE_LOGO_SVG to ui/static/vibescribe_logo.svg and
  load it through a cached read_svg sibling of read_css [MED-09].
- Route the portrait upload failure through error_presenter.show_error [LOW-07].
- Cancel the job detail auto-refresh timer instead of only deactivating it, and
  name its interval constant [LOW-06].

Worker
- Make WorkerNotifier runtime_checkable and validate the resolved object in
  resolve_worker_notifier, which previously returned any non-None attribute
  unchecked [LOW-04].

Docs and lint
- Fix two stale paths in services.instructions.md, one of which pointed at the
  module deleted here [LOW-02].
- ruff check --fix to zero [LOW-01].

Verified: 264 passed, 4 skipped; ruff check clean.

Co-authored-by: Copilot App <[email protected]>
This commit is contained in:
zoltan57
2026-08-17 16:06:08 -05:00
co-authored by Copilot App
parent b3d8eb6e97
commit 2ccea77520
21 changed files with 53 additions and 129 deletions
@@ -7,7 +7,7 @@ applyTo: 'src/transcription/services/*.py'
## Structure ## Structure
- Project core data models defined in [models](../../src/transcription/models.py) - Project core data models defined in [models](../../src/transcription/db/models.py)
- 1 service class per data model - 1 service class per data model
- Only services directly interact with the database, and only through async methods - Only services directly interact with the database, and only through async methods
- Services are completely independent of one another. Any operation that needs to use more than a single service, which is most of them, needs to have a separate orchestration function. - Services are completely independent of one another. Any operation that needs to use more than a single service, which is most of them, needs to have a separate orchestration function.
@@ -15,7 +15,7 @@ applyTo: 'src/transcription/services/*.py'
## Error Handling ## Error Handling
- Service-specific errors defined at the top of the respective module and inherit from `AppError` - Service-specific errors defined at the top of the respective module and inherit from `AppError`
- Use a context manager for large `try/except` blocks like in [transcription](../../src/transcription/services/transcription.py) - Use a context manager for large `try/except` blocks like `handle_transcription_errors` in [sources](../../src/transcription/services/sources.py)
## Checklist ## Checklist
+4 -1
View File
@@ -7,7 +7,10 @@ services:
env_file: env_file:
- .env - .env
environment: environment:
DATABASE_URL: sqlite:////app/data/transcription.db # Database configuration uses nested settings names (env_nested_delimiter="__").
# DATABASE_URL is NOT read by the application and must not be used here.
DATABASE__DRIVER: sqlite
DATABASE__PATH: /app/data/transcription.db
UPLOAD_DIR: /app/uploads UPLOAD_DIR: /app/uploads
PROMPT_DIR: /app/prompts PROMPT_DIR: /app/prompts
ports: ports:
-39
View File
@@ -1,39 +0,0 @@
"""Helpers for accessing lifespan-owned application state resources."""
from __future__ import annotations
from fastapi import FastAPI
from sqlalchemy.ext.asyncio import async_sessionmaker
from sqlmodel.ext.asyncio.session import AsyncSession
from transcription.db.runtime import DatabaseRuntime
from transcription.db.session import get_session_factory
from transcription.worker import WorkerNotifier
from transcription.worker import resolve_worker_notifier
def resolve_database_runtime(state: object) -> DatabaseRuntime | None:
"""Return database runtime from app-like state objects when available."""
runtime = getattr(state, "runtime", None)
return runtime if isinstance(runtime, DatabaseRuntime) else None
def require_database_runtime(state: object) -> DatabaseRuntime:
"""Return database runtime or raise when app lifespan has not initialized it."""
runtime = resolve_database_runtime(state)
if runtime is None:
raise RuntimeError("Database runtime is not initialized on application state")
return runtime
def resolve_session_factory(state: object) -> async_sessionmaker[AsyncSession]:
"""Return DB session factory from state when available, otherwise shared runtime."""
runtime = resolve_database_runtime(state)
if runtime is not None:
return runtime.session_factory
return get_session_factory()
def get_worker_notifier(app: FastAPI) -> WorkerNotifier:
"""Return app worker notifier, or a no-op fallback when unavailable."""
return resolve_worker_notifier(app.state)
-1
View File
@@ -106,7 +106,6 @@ class Settings(BaseSettings):
# --- worker reliability --- # --- worker reliability ---
worker_max_retries: int = Field(default=0, ge=0) worker_max_retries: int = Field(default=0, ge=0)
worker_retry_backoff_seconds: float = Field(default=0.0, ge=0.0)
worker_provider_timeout_seconds: float = Field(default=20.0, gt=0.0, le=20.0) worker_provider_timeout_seconds: float = Field(default=20.0, gt=0.0, le=20.0)
worker_min_transcription_chars: int = Field(default=0, ge=0) worker_min_transcription_chars: int = Field(default=0, ge=0)
worker_min_transcription_lines: int = Field(default=0, ge=0) worker_min_transcription_lines: int = Field(default=0, ge=0)
+6 -3
View File
@@ -33,14 +33,17 @@ def get_database_url(settings: Settings) -> str:
def resolve_engine(settings: Settings | None = None) -> AsyncEngine: def resolve_engine(settings: Settings | None = None) -> AsyncEngine:
active_settings = settings or get_settings() active_settings = settings or get_settings()
return get_engine(get_database_url(active_settings)) return get_engine(
get_database_url(active_settings),
sqlite_check_same_thread=active_settings.sqlite_check_same_thread,
)
@cache @cache
def get_engine(database_url: str) -> AsyncEngine: def get_engine(database_url: str, *, sqlite_check_same_thread: bool = False) -> AsyncEngine:
kwargs: dict[str, Any] = {"echo": False, "pool_pre_ping": True} kwargs: dict[str, Any] = {"echo": False, "pool_pre_ping": True}
if database_url.startswith("sqlite"): if database_url.startswith("sqlite"):
kwargs["connect_args"] = {"check_same_thread": False} kwargs["connect_args"] = {"check_same_thread": sqlite_check_same_thread}
if ":memory:" in database_url: if ":memory:" in database_url:
kwargs["poolclass"] = StaticPool kwargs["poolclass"] = StaticPool
-4
View File
@@ -1,4 +1,3 @@
import asyncio
from abc import ABC from abc import ABC
from collections.abc import Sequence from collections.abc import Sequence
from contextlib import asynccontextmanager from contextlib import asynccontextmanager
@@ -17,17 +16,14 @@ class ServiceBase(ABC):
settings: Settings settings: Settings
session_factory: async_sessionmaker[AsyncSession] session_factory: async_sessionmaker[AsyncSession]
queue: asyncio.Queue
def __init__( def __init__(
self, self,
session_factory: async_sessionmaker[AsyncSession] | None = None, session_factory: async_sessionmaker[AsyncSession] | None = None,
queue: asyncio.Queue | None = None,
settings: Settings | None = None, settings: Settings | None = None,
): ):
self.settings = settings or get_settings() self.settings = settings or get_settings()
self.session_factory = session_factory or resolve_session_factory(settings=self.settings) self.session_factory = session_factory or resolve_session_factory(settings=self.settings)
self.queue = queue or asyncio.Queue()
@asynccontextmanager @asynccontextmanager
async def _session_scope(self, session: AsyncSession | None = None): async def _session_scope(self, session: AsyncSession | None = None):
+1 -8
View File
@@ -22,8 +22,8 @@ from ..db.models import JobSource
from ..db.models import JobSourceStatus from ..db.models import JobSourceStatus
from ..db.models import Source from ..db.models import Source
from .sources import TranscriptionError from .sources import TranscriptionError
from .sources import build_prompt_execution
from .sources import validate_source_content from .sources import validate_source_content
from .transcription import build_prompt_execution
logger = logging.getLogger(__name__) logger = logging.getLogger(__name__)
@@ -32,9 +32,6 @@ class SourceStorageError(AppError):
"""Raised when Source content cannot be validated or persisted safely.""" """Raised when Source content cannot be validated or persisted safely."""
UploadError = SourceStorageError
@dataclass(frozen=True) @dataclass(frozen=True)
class JobCreateResult: class JobCreateResult:
"""Summary of explicit Job create records.""" """Summary of explicit Job create records."""
@@ -377,7 +374,3 @@ def _build_stored_filename(*, filename: str, filename_stem: str | None = None) -
suffix = Path(safe_name).suffix.lower() suffix = Path(safe_name).suffix.lower()
stem = filename_stem or str(uuid4()) stem = filename_stem or str(uuid4())
return f"{stem}{suffix}" return f"{stem}{suffix}"
create_upload_job = create_document_job
store_file = store_source_file
@@ -1,44 +0,0 @@
"""Compatibility exports for the Source-owned transcription implementation."""
from .sources import DEFAULT_PROMPT_FILE
from .sources import SOURCE_EXTENSIONS
from .sources import SOURCE_MIME_TYPES
from .sources import PromptExecution
from .sources import PromptLoadError
from .sources import SourceDeleteBlockedError
from .sources import SourceService
from .sources import TranscriptionError
from .sources import TranscriptionNotFoundError
from .sources import build_prompt_execution
from .sources import handle_transcription_errors
from .sources import load_prompt_text
from .sources import load_source_payload
from .sources import source_mime_type
from .sources import transcribe_document_image
from .sources import validate_source_content
TranscriptionService = SourceService
SUPPORTED_EXTENSIONS = SOURCE_EXTENSIONS
load_image_payload = load_source_payload
__all__ = [
"DEFAULT_PROMPT_FILE",
"SOURCE_EXTENSIONS",
"SOURCE_MIME_TYPES",
"SUPPORTED_EXTENSIONS",
"PromptExecution",
"PromptLoadError",
"SourceDeleteBlockedError",
"SourceService",
"TranscriptionError",
"TranscriptionNotFoundError",
"TranscriptionService",
"build_prompt_execution",
"handle_transcription_errors",
"load_image_payload",
"load_prompt_text",
"load_source_payload",
"source_mime_type",
"transcribe_document_image",
"validate_source_content",
]
+2 -2
View File
@@ -4,7 +4,7 @@ from __future__ import annotations
from nicegui import ui from nicegui import ui
from transcription.ui.theme import VIBESCRIBE_LOGO_SVG from transcription.ui.resources import read_svg
NAV_ITEMS: tuple[tuple[str, str, str], ...] = ( NAV_ITEMS: tuple[tuple[str, str, str], ...] = (
("Documents", "/documents", "description"), ("Documents", "/documents", "description"),
@@ -55,7 +55,7 @@ def render_app_shell(*, current_path: str | None = None) -> None:
with ui.header().classes("app-shell"), ui.element("div").classes("app-shell__inner"): with ui.header().classes("app-shell"), ui.element("div").classes("app-shell__inner"):
with ui.element("a").props('href="/ui/homepage"').classes("app-shell__brand no-wrap"): with ui.element("a").props('href="/ui/homepage"').classes("app-shell__brand no-wrap"):
ui.html(VIBESCRIBE_LOGO_SVG).classes("app-shell__brand-mark") ui.html(read_svg("vibescribe_logo.svg")).classes("app-shell__brand-mark")
ui.label("VibeScribe").classes("app-shell__brand-name") ui.label("VibeScribe").classes("app-shell__brand-name")
with ui.element("nav").props('aria-label="Primary navigation"').classes("app-shell__nav"): with ui.element("nav").props('aria-label="Primary navigation"').classes("app-shell__nav"):
+11 -4
View File
@@ -39,6 +39,8 @@ from transcription.worker import resolve_worker_notifier
from ...db.session import SessionFactoryDep from ...db.session import SessionFactoryDep
JOB_DETAIL_REFRESH_INTERVAL_SECONDS = 4.0
def register_page() -> None: # noqa: PLR0915 def register_page() -> None: # noqa: PLR0915
"""Register jobs list and detail routes.""" """Register jobs list and detail routes."""
@@ -238,19 +240,24 @@ def register_page() -> None: # noqa: PLR0915
ui.label("This page updates automatically while the job is active.").classes("text-xs ui-text-muted") ui.label("This page updates automatically while the job is active.").classes("text-xs ui-text-muted")
async def refresh_job() -> None: async def refresh_job() -> None:
def stop_refresh() -> None:
timer = timer_holder[0]
if timer is not None:
timer.cancel()
timer_holder[0] = None
try: try:
current_job[0] = await jobs_service.read_job(job_id=parsed_job_id) current_job[0] = await jobs_service.read_job(job_id=parsed_job_id)
except Exception as exc: # noqa: BLE001 except Exception as exc: # noqa: BLE001
if timer_holder[0] is not None: stop_refresh()
timer_holder[0].active = False
show_error(exc, title="Auto-refresh failed", operation="jobs.detail.refresh") show_error(exc, title="Auto-refresh failed", operation="jobs.detail.refresh")
return return
render_detail.refresh() render_detail.refresh()
if current_job[0].status not in {JobStatus.QUEUED, JobStatus.PROCESSING}: if current_job[0].status not in {JobStatus.QUEUED, JobStatus.PROCESSING}:
timer_holder[0].active = False stop_refresh()
timer_holder[0] = ui.timer(4.0, refresh_job) timer_holder[0] = ui.timer(JOB_DETAIL_REFRESH_INTERVAL_SECONDS, refresh_job)
@ui.page("/jobs/{job_id}/cancel") @ui.page("/jobs/{job_id}/cancel")
async def job_cancel_page(job_id: str, request: Request, session_factory: SessionFactoryDep) -> None: async def job_cancel_page(job_id: str, request: Request, session_factory: SessionFactoryDep) -> None:
+2 -2
View File
@@ -501,8 +501,8 @@ def _bind_portrait_file_picker(portrait_path_input: ui.input, *, settings: Setti
except PersonMediaError as exc: except PersonMediaError as exc:
ui.notify(str(exc), type="negative") ui.notify(str(exc), type="negative")
return return
except Exception: # noqa: BLE001 except Exception as exc: # noqa: BLE001
ui.notify("Unable to store portrait image.", type="negative") show_error(exc, title="Upload failed", operation="people.portrait.store")
return return
try: try:
+12 -2
View File
@@ -10,9 +10,19 @@ from pathlib import PurePosixPath
@cache @cache
def read_css(relative_path: str) -> str: def read_css(relative_path: str) -> str:
"""Read and cache a CSS resource relative to ``ui/static``.""" """Read and cache a CSS resource relative to ``ui/static``."""
return _read_static(relative_path, suffix=".css")
@cache
def read_svg(relative_path: str) -> str:
"""Read and cache an SVG resource relative to ``ui/static``."""
return _read_static(relative_path, suffix=".svg")
def _read_static(relative_path: str, *, suffix: str) -> str:
resource_path = PurePosixPath(relative_path) resource_path = PurePosixPath(relative_path)
if resource_path.is_absolute() or ".." in resource_path.parts or resource_path.suffix != ".css": if resource_path.is_absolute() or ".." in resource_path.parts or resource_path.suffix != suffix:
msg = f"Invalid CSS resource path: {relative_path}" msg = f"Invalid {suffix.lstrip('.').upper()} resource path: {relative_path}"
raise ValueError(msg) raise ValueError(msg)
resource = files("transcription.ui").joinpath("static", *resource_path.parts) resource = files("transcription.ui").joinpath("static", *resource_path.parts)
File diff suppressed because one or more lines are too long

After

Width:  |  Height:  |  Size: 23 KiB

File diff suppressed because one or more lines are too long
+5 -3
View File
@@ -9,6 +9,7 @@ from contextlib import asynccontextmanager
from contextlib import contextmanager from contextlib import contextmanager
from contextlib import suppress from contextlib import suppress
from typing import Protocol from typing import Protocol
from typing import runtime_checkable
from sqlalchemy.ext.asyncio import async_sessionmaker from sqlalchemy.ext.asyncio import async_sessionmaker
from sqlmodel.ext.asyncio.session import AsyncSession from sqlmodel.ext.asyncio.session import AsyncSession
@@ -27,6 +28,7 @@ from .services.workflows import process_next_queued_job as process_next_queued_j
logger = logging.getLogger(__name__) logger = logging.getLogger(__name__)
@runtime_checkable
class WorkerNotifier(Protocol): class WorkerNotifier(Protocol):
"""Abstraction for signaling the worker loop about new work.""" """Abstraction for signaling the worker loop about new work."""
@@ -54,11 +56,11 @@ class NoopWorkerNotifier:
def resolve_worker_notifier(state: object) -> WorkerNotifier: def resolve_worker_notifier(state: object) -> WorkerNotifier:
"""Resolve notifier from app-like state objects with no-op fallback.""" """Resolve notifier from app-like state objects with no-op fallback."""
notifier = getattr(state, "worker_notifier", None) notifier = getattr(state, "worker_notifier", None)
if isinstance(notifier, NoopWorkerNotifier): if isinstance(notifier, WorkerNotifier):
return notifier return notifier
if notifier is None: if notifier is not None:
logger.warning("Ignoring worker_notifier of unsupported type %r; using no-op fallback.", type(notifier))
return NoopWorkerNotifier() return NoopWorkerNotifier()
return notifier
@asynccontextmanager @asynccontextmanager
@@ -5,7 +5,7 @@ from pathlib import Path
import pytest import pytest
from transcription.services.transcription import transcribe_document_image from transcription.services.sources import transcribe_document_image
HAS_OPENROUTER_KEY = bool(os.getenv("OPENROUTER_API_KEY")) HAS_OPENROUTER_KEY = bool(os.getenv("OPENROUTER_API_KEY"))
+1
View File
@@ -20,6 +20,7 @@ class TestAppFactory:
app = create_app() app = create_app()
assert isinstance(app, FastAPI) assert isinstance(app, FastAPI)
@pytest.mark.integration @pytest.mark.integration
class TestAppLifespan: class TestAppLifespan:
"""Verify startup and shutdown lifecycle behavior.""" """Verify startup and shutdown lifecycle behavior."""
+1 -2
View File
@@ -146,7 +146,6 @@ class TestWorkerReliabilitySettings:
"""Verify worker retry settings defaults.""" """Verify worker retry settings defaults."""
def test_worker_retry_defaults(self): def test_worker_retry_defaults(self):
"""worker retry settings default to no retries and no backoff.""" """worker retry settings default to no retries."""
settings = _make_settings() settings = _make_settings()
assert settings.worker_max_retries == 0 assert settings.worker_max_retries == 0
assert settings.worker_retry_backoff_seconds == 0.0
-1
View File
@@ -41,4 +41,3 @@ class TestPageRendering:
assert response.status_code == 200 assert response.status_code == 200
assert "Edit Home Page" in response.text assert "Edit Home Page" in response.text
assert "Homepage markdown" in response.text assert "Homepage markdown" in response.text