Skip to content

Refactor shared team-season page scaffolding - #51

Merged
Mattsface merged 3 commits into
mainfrom
issue-44-route-scaffolding
Sep 23, 2026
Merged

Mattsface merged 3 commits into
mainfrom
issue-44-route-scaffolding

Conversation

@Mattsface

Copy link
Copy Markdown
Member

Summary

Implements the first, route-scaffolding slice of #44.

  • adds cross-route characterization tests for all eight analytics pages
  • extracts the duplicated pre-analytics team/season page lifecycle into a small typed helper
  • preserves requested-vs-resolved navigation semantics, existing 200/404/409/422/503 behavior, and DB-only browser rendering
  • keeps metric-specific record loading, missing-data handling, league gating, analytics, charts, and cards explicit in each route
  • documents the new abstraction boundary
  • consolidates the new scaffold test matrix from 265 cases to 64 while retaining coverage and adding explicit zero-MLB-call browser tests for all eight analytics routes

Scope intentionally excluded

  • chart refactors
  • Pydantic validator refactors
  • persistence or ingestion changes
  • template/static changes
  • Player UI
  • URL or analytics behavior changes

Validation

Branch review found no production-code blocker. PR CI should be treated as the authoritative full pytest/Ruff validation before merge.

Refs #44

Mattsface and others added 3 commits September 23, 2026 12:53
Pin the pre-analytics behavior every analytics route shares, across all
eight routes: empty database, unknown team/season 404s, explicit and
default selection, FastAPI 422 validation, and the missing-schema 503.
Terminal states keep the requested query values in navigation; resolved
selections rebuild navigation from the resolved values.

Refs #44

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Extract the duplicated route prefix (team-season lookup, 503/empty/404
handling, team and season resolution, navigation rebuild) into
_prepare_team_season_page, returning a small PreparedTeamSeasonPage or a
rendered terminal response. Declare the team_id/season/window query
contract once as Annotated aliases.

The abstraction stops before metric-specific record loading: every
route's code from its first repository call onward is unchanged, and the
generated OpenAPI schema is identical.

Refs #44

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Group the shared team-season scaffold assertions so each analytics route
makes all checks for one state against one migrated database, instead of
one tiny test per assertion. Every contract is still pinned on all eight
routes, including the requested-vs-resolved navigation distinction.

Add a parametrized DB-only regression proving each analytics browser
route renders from persisted data without constructing an MLB client or
calling the network-facing services, using the same monkeypatch pattern
as the existing route suites.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Mattsface

Copy link
Copy Markdown
Member Author

@claude

@claude

claude Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Claude finished @Mattsface's task in 2m 13s —— View job


Claude's Task List

  • Gather context (read AGENTS.md, changed files, diff)
  • Review app/web/routes.py changes
  • Review new test file tests/test_web_page_scaffolding.py
  • Review docs/architecture-deep-dive.md updates
  • Run tests/linting if possible — not possible in this sandbox (see note below)
  • Post review feedback

Review

The trigger comment was just @claude with no specific question, so I treated it as a request for a general review of this PR.

Summary: This is a clean, behavior-preserving mechanical refactor. I diffed each of the eight routes' pre-refactor bodies against the new _prepare_team_season_page helper line-by-line, and the extracted logic is a faithful copy — same schema-missing → 503, empty → "empty" state, unknown team/season → 404 (requested values kept in nav), and resolved selection → nav rebuilt from resolved values, parameterized only by path and template_name. No production-code correctness issues found.

Architecture (AGENTS.md) fit:

  • The extraction point (app/web/routes.py:1040) is a reasonable place to draw the line — the helper stops exactly where routes start differing (record loading, missing-data states, league gating), matching the "small focused modules" and "refactor only after the repeated pattern is understood" guidance. Eight near-identical 90-line route bodies is a well-earned abstraction, not a premature one.
  • PreparedTeamSeasonPage (app/web/routes.py:1026) is a small, explicitly-typed dataclass rather than a loosely-typed tuple/dict return, consistent with the type-safety rules.
  • No architectural layering violations: the helper still only touches repositories, selection, navigation, and template rendering — no analytics or MLB calls crept in.
  • docs/architecture-deep-dive.md update accurately reflects the new code (I checked the diagram against the actual helper/route split) and correctly preserves the historical note about the original four-route decision rather than rewriting history.

Tests (tests/test_web_page_scaffolding.py):

  • Good characterization coverage: empty DB, unknown team, unknown season, explicit selection, default selection, FastAPI 422 validation, missing schema, and a zero-MLB-call regression — each parametrized across all 8 routes.
  • Correctly asserts the requested-vs-resolved navigation distinction, which is the subtlest part of this contract.
  • Offline only (network calls monkeypatched to raise), consistent with the "Offline tests" rule.

Minor observations (non-blocking):

  • app/web/routes.py:1027 — PreparedTeamSeasonPage.season: int combined with context: dict[str, Any] is consistent with the pre-existing style in this file (the whole template-context dict was already dict[str, Any]), so this isn't a regression, just noting it's the same tradeoff as before.
  • The 8 call sites each pass templates, settings, request, session positionally then a block of keyword args — slightly verbose, but matches the file's existing preference for explicit values over hidden defaults, so I wouldn't change it.

Validation limitation: I was not able to run pytest/ruff in this sandbox (no shell access beyond git/basic lookups was permitted for this task), so I could not independently execute the test suite. The PR description already flags PR CI as the authoritative validation for pytest/Ruff — that should be relied on before merge.

No blocking issues found. Scope stays within the stated route-scaffolding slice of #44 (no chart, validator, persistence, template, or Player-UI changes touched, matching the PR's stated exclusions).

@Mattsface
Mattsface merged commit 73de637 into main Sep 23, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant