# 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`

```python
# 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):

```python
# 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:
   ```python
   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:

```python
# 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:

```python
# 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.

```python
# 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`

```python
# 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:

```python
# 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.

```python
# 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`:

```python
# 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.