bincio-activity/refactoring.md

Refactoring Plan

Branch: refactoring
Approach: test-first — each step starts with tests that prove correctness of the current behaviour, then the refactor makes those tests pass against the new structure.

---

Step 1 — Extract shared image utilities

Problem

_ALLOWED_IMAGE_TYPES, _MAX_IMAGE_BYTES, and _unique_image_name() are defined identically in two files:

File	Lines
bincio/edit/server.py	46–58
bincio/serve/server.py	337–357

Any change (e.g. adding image/avif) must be made in both places.

Target

New module: bincio/shared/images.py

# bincio/shared/images.py
from pathlib import Path

ALLOWED_IMAGE_TYPES: frozenset[str] = frozenset({
    "image/jpeg", "image/png", "image/webp", "image/gif"
})
MAX_IMAGE_BYTES: int = 10 * 1024 * 1024  # 10 MB

def unique_image_name(directory: Path, filename: str) -> str:
    """Return a filename that does not collide with existing files in directory."""
    stem, suffix = Path(filename).stem, Path(filename).suffix
    candidate = filename
    counter = 1
    while (directory / candidate).exists():
        candidate = f"{stem}_{counter}{suffix}"
        counter += 1
    return candidate

Test plan

New file: tests/test_shared_images.py

Write these tests first (they will fail until the module exists):

# tests/test_shared_images.py
from pathlib import Path
import pytest
from bincio.shared.images import ALLOWED_IMAGE_TYPES, MAX_IMAGE_BYTES, unique_image_name

def test_constants():
    assert "image/jpeg" in ALLOWED_IMAGE_TYPES
    assert "image/png" in ALLOWED_IMAGE_TYPES
    assert "image/webp" in ALLOWED_IMAGE_TYPES
    assert "image/gif" in ALLOWED_IMAGE_TYPES
    assert "image/avif" not in ALLOWED_IMAGE_TYPES          # guard against accidental expansion
    assert MAX_IMAGE_BYTES == 10 * 1024 * 1024

def test_unique_name_no_collision(tmp_path):
    assert unique_image_name(tmp_path, "photo.jpg") == "photo.jpg"

def test_unique_name_single_collision(tmp_path):
    (tmp_path / "photo.jpg").touch()
    assert unique_image_name(tmp_path, "photo.jpg") == "photo_1.jpg"

def test_unique_name_multiple_collisions(tmp_path):
    (tmp_path / "photo.jpg").touch()
    (tmp_path / "photo_1.jpg").touch()
    assert unique_image_name(tmp_path, "photo.jpg") == "photo_2.jpg"

def test_unique_name_no_suffix(tmp_path):
    (tmp_path / "photo").touch()
    assert unique_image_name(tmp_path, "photo") == "photo_1"

def test_unique_name_preserves_case(tmp_path):
    assert unique_image_name(tmp_path, "MyPhoto.PNG") == "MyPhoto.PNG"

Implementation steps

1. Create bincio/shared/__init__.py (empty).
2. Create bincio/shared/images.py with the public constants and function (no leading underscore).
3. In bincio/edit/server.py: replace lines 46–58 with:

   from bincio.shared.images import ALLOWED_IMAGE_TYPES as _ALLOWED_IMAGE_TYPES
   from bincio.shared.images import MAX_IMAGE_BYTES as _MAX_IMAGE_BYTES
   from bincio.shared.images import unique_image_name as _unique_image_name
   

4. In bincio/serve/server.py: same replacement at lines 337–357.
5. Run pytest tests/test_shared_images.py — all pass.
6. Run full test suite to confirm no regressions.

---

Step 2 — Extract embedded HTML template from edit/server.py

Problem

bincio/edit/server.py contains 285 lines of static HTML/CSS/JS as a Python string literal (_HTML, lines 63–347). This:

• Makes the file 30% larger than it needs to be.
• Prevents syntax highlighting and linting of the HTML/JS.
• Cannot be tested in isolation (template substitution is done inline by the route handler).

Target

New file: bincio/edit/templates/edit.html

The template already uses three placeholder tokens:

• __SITE_URL__ — replaced with site_url at request time
• __SPORT_OPTIONS__ — replaced with generated <option> tags
• __STAT_CHECKBOXES__ — replaced with generated <label> tags

Extract a helper that loads and renders the template:

# bincio/edit/server.py — replaces the _HTML string and inline render
from pathlib import Path as _Path
_TEMPLATE_PATH = _Path(__file__).parent / "templates" / "edit.html"

def _render_edit_html(activity_id: str) -> str:
    template = _TEMPLATE_PATH.read_text(encoding="utf-8")
    sport_options = "\n".join(
        f'<option value="{s}">{s.title()}</option>' for s in SPORTS
    )
    stat_checkboxes = "\n".join(
        f'<label class="check-item"><input type="checkbox" data-stat="{k}"> {v}</label>'
        for k, v in STAT_PANELS.items()
    )
    return (
        template
        .replace("__SITE_URL__", site_url)
        .replace("__SPORT_OPTIONS__", sport_options)
        .replace("__STAT_CHECKBOXES__", stat_checkboxes)
    )

Test plan

Extend tests/test_edit_server.py with a new section:

# tests/test_edit_server.py — new tests for template loading
import bincio.edit.server as edit_server

def test_edit_ui_returns_html(tmp_path):
    """GET /edit/<id> returns 200 with an HTML body containing the form."""
    edit_server.data_dir = tmp_path
    activities = tmp_path / "activities"
    activities.mkdir()
    (activities / "run-001.json").write_text('{"id":"run-001"}')
    resp = CLIENT.get("/edit/run-001")
    assert resp.status_code == 200
    assert resp.headers["content-type"].startswith("text/html")
    assert '<form id="form"' in resp.text

def test_edit_ui_injects_site_url(tmp_path):
    """Template placeholder __SITE_URL__ is replaced with the configured site_url."""
    edit_server.data_dir = tmp_path
    edit_server.site_url = "http://localhost:1234"
    (tmp_path / "activities").mkdir(exist_ok=True)
    (tmp_path / "activities" / "run-001.json").write_text('{"id":"run-001"}')
    resp = CLIENT.get("/edit/run-001")
    assert "http://localhost:1234" in resp.text
    assert "__SITE_URL__" not in resp.text

def test_edit_ui_no_unresolved_placeholders(tmp_path):
    """No placeholder tokens remain after rendering."""
    edit_server.data_dir = tmp_path
    (tmp_path / "activities").mkdir(exist_ok=True)
    (tmp_path / "activities" / "run-001.json").write_text('{"id":"run-001"}')
    resp = CLIENT.get("/edit/run-001")
    for token in ("__SITE_URL__", "__SPORT_OPTIONS__", "__STAT_CHECKBOXES__"):
        assert token not in resp.text, f"Unresolved placeholder: {token}"

def test_edit_template_file_exists():
    """The template file is present on disk (guards against accidental deletion)."""
    from pathlib import Path
    template = Path(edit_server.__file__).parent / "templates" / "edit.html"
    assert template.exists(), f"Template not found at {template}"

Implementation steps

1. Create bincio/edit/templates/ directory.
2. Move the HTML content of _HTML into bincio/edit/templates/edit.html verbatim (keep the __PLACEHOLDER__ tokens as-is).
3. Delete the _HTML = """...""" string literal from server.py (lines 63–347).
4. Add _render_edit_html() helper as shown above.
5. Update the /edit/{activity_id} route handler to call _render_edit_html(activity_id) instead of the inline _HTML.replace(...) chain.
6. Run pytest tests/test_edit_server.py — all pass (new + existing).

---

Step 3 — Split serve/server.py into APIRouter modules

Problem

bincio/serve/server.py is 3,230 lines containing ~60 routes across 10 logical domains, all shared dependencies, all Pydantic models, and all background task machinery. It cannot be meaningfully reviewed, tested in isolation, or understood at a glance.

Target structure

bincio/serve/
├── server.py          # ~150 lines: app factory, middleware, router registration, startup
├── deps.py            # module-level globals + shared FastAPI dependency functions
├── models.py          # all Pydantic request/response models
├── tasks.py           # background workers: site-rebuild, rebuild-for-handle, jobs registry
├── db.py              # unchanged
└── routers/
    ├── __init__.py
    ├── auth.py        # /api/auth/*, /api/register, /api/invites
    ├── me.py          # /api/me/*
    ├── admin.py       # /api/admin/*
    ├── activities.py  # /api/activity/*, /api/activities/*
    ├── uploads.py     # /api/upload/*
    ├── segments.py    # /api/segments/*
    ├── strava.py      # /api/strava/*
    ├── garmin.py      # /api/garmin/*
    ├── ideas.py       # /api/ideas/*, /api/feedback
    └── feed.py        # /api/feed, /api/stats, /api/me (read-only), /api/wheel/*

deps.py — shared state and dependency functions

All module-level globals that multiple routers need move here. The CLI sets them on this module before uvicorn starts.

# bincio/serve/deps.py
from __future__ import annotations
import os
import re
from pathlib import Path
from typing import Optional
import sqlite3

from fastapi import Cookie, HTTPException
from bincio.serve.db import User, get_session, get_user

# ── Module-level state (set by CLI) ──────────────────────────────────────────
data_dir: Path | None = None
site_dir: Path | None = None
webroot: Path | None = None
strava_client_id: str = ""
strava_client_secret: str = ""
public_url: str = ""
dem_url: str = "https://api.open-elevation.com"
sync_secret: str = ""
garmin_key: bytes | None = None
_db: sqlite3.Connection | None = None

# ── Constants ─────────────────────────────────────────────────────────────────
VALID_HANDLE = re.compile(r'^[a-z0-9][a-z0-9_-]{0,29}$')
SESSION_COOKIE = "bincio_session"
COOKIE_MAX_AGE = 30 * 86400
SESSION_DOMAIN = os.environ.get("SESSION_DOMAIN") or None

# ── Dependency functions ───────────────────────────────────────────────────────
def get_data_dir() -> Path:
    if data_dir is None:
        raise HTTPException(500, "Server not configured")
    return data_dir

def get_db() -> sqlite3.Connection:
    global _db
    if _db is None:
        from bincio.serve.db import open_db
        _db = open_db(get_data_dir())
    return _db

def get_current_user(bincio_session: Optional[str] = Cookie(default=None)) -> User:
    if not bincio_session:
        raise HTTPException(401, "Not authenticated")
    sess = get_session(get_db(), bincio_session)
    if sess is None:
        raise HTTPException(401, "Invalid or expired session")
    user = get_user(get_db(), sess.handle)
    if user is None or user.suspended:
        raise HTTPException(401, "Account not found or suspended")
    return user

def require_admin(bincio_session: Optional[str] = Cookie(default=None)) -> User:
    user = get_current_user(bincio_session)
    if not user.is_admin:
        raise HTTPException(403, "Admin required")
    return user

models.py — Pydantic models

All class *Request(BaseModel) and class *Response(BaseModel) definitions move to bincio/serve/models.py. Routers import from there.

tasks.py — background workers

Move _site_rebuild_worker, _site_rebuild_event, _rebuild_for_handle, _active_jobs, _jobs_lock, _job_start, _job_update, _job_finish to bincio/serve/tasks.py. These import from deps.py for webroot and site_dir.

Router example — routers/auth.py

# bincio/serve/routers/auth.py
from fastapi import APIRouter, Cookie, Depends, HTTPException, Request, Response
from bincio.serve import deps
from bincio.serve.models import LoginRequest, LoginResponse, ...

router = APIRouter()

@router.post("/api/auth/login", response_model=LoginResponse)
async def login(body: LoginRequest, request: Request, response: Response):
    db = deps.get_db()
    ...

Main server.py becomes:

# bincio/serve/server.py  (~150 lines)
from fastapi import FastAPI
from fastapi.middleware.cors import CORSMiddleware
from fastapi.middleware.gzip import GZipMiddleware

from bincio.serve.routers import auth, me, admin, activities, uploads, segments, strava, garmin, ideas, feed

app = FastAPI(title="BincioActivity Serve")
app.add_middleware(GZipMiddleware, minimum_size=1024)
app.add_middleware(CORSMiddleware, ...)

for router in [auth.router, me.router, admin.router, activities.router,
               uploads.router, segments.router, strava.router, garmin.router,
               ideas.router, feed.router]:
    app.include_router(router)

@app.on_event("startup")
async def _on_startup() -> None: ...

Test plan

Philosophy: write tests that call routes through the full app (via TestClient), so they remain valid before and after the split. Each router gets its own test file.

Before starting the split

Write tests/serve/test_auth_router.py (and equivalents for each router). These tests pass against the current monolith. After the split they must still pass — this is the regression guard.

# tests/serve/test_auth_router.py
import pytest
from fastapi.testclient import TestClient
from bincio.serve.server import app
import bincio.serve.deps as deps

@pytest.fixture()
def client(tmp_path):
    deps.data_dir = tmp_path
    deps._db = None
    return TestClient(app, raise_server_exceptions=False)

def test_login_missing_body(client):
    r = client.post("/api/auth/login", json={})
    assert r.status_code == 422

def test_login_wrong_password(client):
    r = client.post("/api/auth/login", json={"handle": "nobody", "password": "x"})
    assert r.status_code in (401, 404)

def test_logout_unauthenticated(client):
    r = client.post("/api/auth/logout")
    assert r.status_code == 401

def test_register_invite_required(client):
    r = client.post("/api/register", json={"handle": "alice", "password": "pass1234", "invite": ""})
    assert r.status_code in (400, 403)

Write similar test files for me, admin, activities, uploads before touching any production code.

After the split

Run the full test suite. No test should fail — the tests are route-level and do not import from the monolith's internals.

Additional tests enabled by the split

Once routers are isolated, add unit tests for dependency functions in deps.py:

# tests/serve/test_deps.py
import pytest
from fastapi import HTTPException
import bincio.serve.deps as deps

def test_get_data_dir_raises_when_unset():
    deps.data_dir = None
    with pytest.raises(HTTPException) as exc:
        deps.get_data_dir()
    assert exc.value.status_code == 500

def test_get_current_user_raises_without_cookie(tmp_path):
    deps.data_dir = tmp_path
    deps._db = None
    with pytest.raises(HTTPException) as exc:
        deps.get_current_user(bincio_session=None)
    assert exc.value.status_code == 401

Implementation steps (do not start until all pre-split tests are green)

1. Create bincio/serve/deps.py — move globals, constants, get_db, get_current_user, require_admin, get_data_dir. Update serve/cli.py to set deps.* instead of server.*.
2. Create bincio/serve/models.py — move all Pydantic models. Update imports in server.py.
3. Create bincio/serve/tasks.py — move _site_rebuild_worker, _site_rebuild_event, _rebuild_for_handle, jobs registry. Import from deps.
4. Create bincio/serve/routers/__init__.py (empty).
5. Extract one router at a time in this order (least-coupled first):
   • feed.py (read-only, minimal deps)
   • auth.py (depends only on db, no user dir operations)
   • me.py (depends on current user + user dir)
   • ideas.py
   • segments.py
   • strava.py
   • garmin.py
   • activities.py
   • uploads.py
   • admin.py (most complex, depends on tasks)
6. After each router extraction: run the full test suite before proceeding.
7. Once all routers are extracted, reduce server.py to the app factory and middleware only.

---

Step 4 — Narrow broad except Exception: catches

Problem

After the Step 3 split, the router files inherited ~35 bare except Exception: clauses from the original monolith. Most were in route handlers where a specific narrow type is knowable and preferable — broad catches hide bugs and let surprising failures silently produce wrong results.

Classification rule

Situation	Decision
Background thread top-level guard (calls log.exception)	Keep — last-resort, full traceback essential
SSE stream generator top-level	Keep — must convert any error to a client event
Per-item batch loop (must not abort on one failure)	Keep — log.warning/error already present
Explicitly non-fatal post-upload merge step	Keep — log.warning present; upload already succeeded
Route handler: reading/writing JSON files	(OSError, json.JSONDecodeError)
Route handler: datetime parsing	ValueError
Route handler: base64 decoding	ValueError
Route handler: YAML parsing	(OSError, yaml.YAMLError)
Route handler: GeoJSON coord extraction	(TypeError, IndexError, AttributeError)
Startup cleanup (Path.unlink)	OSError
JSON line parsing inside SSE batch	json.JSONDecodeError

What was changed (28 catches narrowed across 8 files)

• server.py — startup tmp*.zip cleanup → OSError
• segments.py — file-scan loops (6 catches) → (OSError, json.JSONDecodeError, ValueError) / ValueError
• me.py — credential file reads, manifest write (4 catches) → (OSError, json.JSONDecodeError)
• activities.py — index/cache reads (2 catches) → (OSError, json.JSONDecodeError); YAML enrichment (1) → (OSError, yaml.YAMLError) with import yaml moved above the try
• admin.py — diag index reads, strava-status loop reads (5 catches) → (OSError, json.JSONDecodeError) / json.JSONDecodeError
• ideas.py — idea file reads (3 catches) → (OSError, json.JSONDecodeError)
• strava.py — index parse in reset endpoint → (OSError, json.JSONDecodeError, ValueError)
• uploads.py — GeoJSON coords, base64 decode, cache update (3 catches)

What was kept (11 catches, all intentional)

tasks.py:97, tasks.py:133 — background thread tops with log.exception
admin.py:579 — admin strava-sync background thread top with log.exception
admin.py:630 — per-activity batch loop in recompute-elevation with log.warning
garmin.py:112, strava.py:164, uploads.py:491 — SSE stream tops
uploads.py:143, uploads.py:259 — non-fatal post-upload merge with log.warning
uploads.py:245 — extraction failure → 422 (any parser failure must surface as 422)
uploads.py:404 — per-file batch loop in upload event stream

---

Progress tracker

#	Step	Status
1	Extract shared image utilities → bincio/shared/images.py	Done
2	Extract HTML template → bincio/edit/templates/edit.html	Done
3	Split serve/server.py into deps.py + routers/*	Done
4	Narrow broad except Exception: catches	Done

Note on dependency pinning: not included. uv.lock already pins every dependency (including transitives) to exact versions, which is strictly stronger than switching >= to ~= in pyproject.toml. The lockfile is the right mechanism for this concern.