cal/paper-dynasty-database
1
0
Fork 0
Code Issues 7 Pull Requests 4 Actions Packages Projects Releases Wiki Activity

feat: render pipeline optimization (Phase 0) #94

Merged
cal merged 2 commits from feature/render-pipeline-optimization into main 2026-03-16 16:15:42 +00:00
Conversation 0 Commits 2 Files Changed 4 +95 -11
cal commented 2026-03-13 04:58:54 +00:00
Owner
Copy Link
  • Persistent Chromium browser instance with auto-reconnect (WP-02)
  • FastAPI lifespan hooks for browser startup/shutdown (WP-03)
  • Self-hosted WOFF2 fonts via base64 embedding, remove Google Fonts CDN (WP-01)
  • Fix pre-existing lint issues (unused imports, f-string placeholders)

Eliminates ~1.5s browser spawn overhead and ~0.4s font CDN round-trip
per card render. Target: per-card render from ~3s to <1s.

Refs: #88, #89, #90

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

- Persistent Chromium browser instance with auto-reconnect (WP-02) - FastAPI lifespan hooks for browser startup/shutdown (WP-03) - Self-hosted WOFF2 fonts via base64 embedding, remove Google Fonts CDN (WP-01) - Fix pre-existing lint issues (unused imports, f-string placeholders) Eliminates ~1.5s browser spawn overhead and ~0.4s font CDN round-trip per card render. Target: per-card render from ~3s to <1s. Refs: #88, #89, #90 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cal added 1 commit 2026-03-13 04:58:55 +00:00 
- Persistent Chromium browser instance with auto-reconnect (WP-02)
- FastAPI lifespan hooks for browser startup/shutdown (WP-03)
- Self-hosted WOFF2 fonts via base64 embedding, remove Google Fonts CDN (WP-01)
- Fix pre-existing lint issues (unused imports, f-string placeholders)

Eliminates ~1.5s browser spawn overhead and ~0.4s font CDN round-trip
per card render. Target: per-card render from ~3s to <1s.

Refs: #88, #89, #90

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cal changed target branch from next-release to main 2026-03-13 05:00:45 +00:00
cal changed target branch from main to next-release 2026-03-13 05:01:08 +00:00
cal changed target branch from next-release to main 2026-03-13 05:02:59 +00:00
cal force-pushed feature/render-pipeline-optimization from 74294c38ce to c262bb431e 2026-03-13 05:07:09 +00:00 Compare
Claude added the
ai-reviewing
 label 2026-03-13 05:18:26 +00:00
Claude approved these changes 2026-03-13 05:21:56 +00:00
Dismissed
Claude left a comment
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/main.py (modified)
  • app/routers_v2/players.py (modified)
  • storage/templates/player_card.html (modified)
  • storage/templates/style.html (modified — base64 WOFF2 font data)

Findings

Correctness

  • Browser lifecycle is correct. Lifespan hook pre-warms the browser on startup (get_browser()) and cleanly shuts it down on exit (shutdown_browser()). The render refactor correctly replaces the per-request async with async_playwright() context manager with get_browser() + page.close() in a finally block — browser leak on exception is prevented.
  • Auto-reconnect logic is sound. get_browser() checks _browser.is_connected() before reuse, tears down the stale playwright instance, then re-launches. Correct for crash recovery.
  • Viewport addition is safe. new_page(viewport={"width": 1280, "height": 720}) was added; screenshot clips to {"x": 0, "y": 0, "width": 1200, "height": 600} which is within bounds — no visual regression expected.
  • Google Fonts removal is correct. player_card.html drops the CDN <link> tags; style.html now provides the same fonts via @font-face + base64 WOFF2, eliminating the ~0.4s CDN round-trip.
  • Lint fixes are correct. Unnecessary f-strings (f"0.1.1", f"There are no players to filter") removed. filename → _filename suppresses the unused-variable warning.

Edge Cases

  • Race condition in get_browser() under concurrent reconnect. The singleton guard if _browser is None or not _browser.is_connected() is not protected by an asyncio lock. If two requests hit this check simultaneously while _browser is None or disconnected (e.g., after a crash), both coroutines can enter the reconnect branch and the first playwright instance is leaked. In practice, risk is low: the lifespan hook pre-warms before any request is served, and browser crashes are rare in a containerized env. Flagged as a suggestion only.

Security

  • No [REDACTED] log pattern regressions — verified all logging calls in the diff.
  • --no-sandbox + --disable-dev-shm-usage are standard Chromium flags for containerized environments — no security concern.
  • Removing the Google Fonts CDN reduces external dependency surface area — a mild security improvement.
  • No credentials or tokens exposed.

Style & Conventions

  • Importing get_browser / shutdown_browser from a router module into main.py is slightly unconventional, but pragmatic given the browser state must live alongside the render code. Consistent with the codebase's existing patterns.
  • _filename (line ~728 post-patch) is still dead code — suppressed rather than deleted. Minor; within the PR's stated scope of "pre-existing lint fixes".

Suggestions

  • Consider extracting the browser singleton into a dedicated app/browser.py module so main.py doesn't import implementation details from a router.
  • If the card render endpoint ever handles burst traffic, add an asyncio.Lock around the reconnect block in get_browser() to prevent playwright instance leaks: 
    _browser_lock = asyncio.Lock()
async def get_browser() -> Browser:
    async with _browser_lock:
        if _browser is None or not _browser.is_connected():
            ...

Verdict: APPROVED

Clean, well-scoped implementation. The persistent browser singleton and lifespan hooks are correct and properly integrated. Font self-hosting is a straightforward CDN removal with correct @font-face rules. The theoretical race condition in get_browser() is low-risk given startup pre-warming and is flagged as a suggestion only.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/main.py` (modified) - `app/routers_v2/players.py` (modified) - `storage/templates/player_card.html` (modified) - `storage/templates/style.html` (modified — base64 WOFF2 font data) ### Findings #### Correctness - **Browser lifecycle is correct.** Lifespan hook pre-warms the browser on startup (`get_browser()`) and cleanly shuts it down on exit (`shutdown_browser()`). The render refactor correctly replaces the per-request `async with async_playwright()` context manager with `get_browser()` + `page.close()` in a `finally` block — browser leak on exception is prevented. - **Auto-reconnect logic is sound.** `get_browser()` checks `_browser.is_connected()` before reuse, tears down the stale playwright instance, then re-launches. Correct for crash recovery. - **Viewport addition is safe.** `new_page(viewport={"width": 1280, "height": 720})` was added; screenshot clips to `{"x": 0, "y": 0, "width": 1200, "height": 600}` which is within bounds — no visual regression expected. - **Google Fonts removal is correct.** `player_card.html` drops the CDN `<link>` tags; `style.html` now provides the same fonts via `@font-face` + base64 WOFF2, eliminating the ~0.4s CDN round-trip. - **Lint fixes are correct.** Unnecessary f-strings (`f"0.1.1"`, `f"There are no players to filter"`) removed. `filename` → `_filename` suppresses the unused-variable warning. #### Edge Cases - **Race condition in `get_browser()` under concurrent reconnect.** The singleton guard `if _browser is None or not _browser.is_connected()` is not protected by an asyncio lock. If two requests hit this check simultaneously while `_browser` is None or disconnected (e.g., after a crash), both coroutines can enter the reconnect branch and the first playwright instance is leaked. **In practice, risk is low:** the lifespan hook pre-warms before any request is served, and browser crashes are rare in a containerized env. Flagged as a suggestion only. #### Security - No `[REDACTED]` log pattern regressions — verified all logging calls in the diff. - `--no-sandbox` + `--disable-dev-shm-usage` are standard Chromium flags for containerized environments — no security concern. - Removing the Google Fonts CDN reduces external dependency surface area — a mild security improvement. - No credentials or tokens exposed. #### Style & Conventions - Importing `get_browser` / `shutdown_browser` from a router module into `main.py` is slightly unconventional, but pragmatic given the browser state must live alongside the render code. Consistent with the codebase's existing patterns. - `_filename` (line ~728 post-patch) is still dead code — suppressed rather than deleted. Minor; within the PR's stated scope of "pre-existing lint fixes". #### Suggestions - Consider extracting the browser singleton into a dedicated `app/browser.py` module so `main.py` doesn't import implementation details from a router. - If the card render endpoint ever handles burst traffic, add an `asyncio.Lock` around the reconnect block in `get_browser()` to prevent playwright instance leaks: ```python _browser_lock = asyncio.Lock() async def get_browser() -> Browser: async with _browser_lock: if _browser is None or not _browser.is_connected(): ... ``` ### Verdict: APPROVED Clean, well-scoped implementation. The persistent browser singleton and lifespan hooks are correct and properly integrated. Font self-hosting is a straightforward CDN removal with correct `@font-face` rules. The theoretical race condition in `get_browser()` is low-risk given startup pre-warming and is flagged as a suggestion only. --- *Automated review by Claude PR Reviewer*
Claude added the
ai-reviewed
 label 2026-03-13 05:22:33 +00:00
cal added 1 commit 2026-03-13 13:29:07 +00:00
fix: add asyncio.Lock to get_browser() and deduplicate font-face blocks
All checks were successful
Build Docker Image / build (pull_request) Successful in 3m32s
Details
6ab50ba5f2 
Address two review findings from PR #94:

1. Race condition: concurrent requests could both launch Chromium when
   _browser is None. Wrap the init check in asyncio.Lock so only one
   coroutine creates the browser process.

2. Font duplication: the WOFF2 files are variable fonts covering all
   needed weights. Consolidate 5 @font-face blocks (3x Open Sans,
   2x Source Sans 3) into 2 using CSS font-weight range syntax,
   saving ~163KB of redundant base64 per render.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cal dismissed Claude’s review 2026-03-13 13:29:07 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

cal merged commit 07caa613d4 into main 2026-03-16 16:15:42 +00:00
cal deleted branch feature/render-pipeline-optimization 2026-03-16 16:15:42 +00:00
cal removed the
ai-reviewing
 label 2026-03-23 15:33:05 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-merged

PR merged by AI ops agent

  
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-reviewing

   
ai-working

   
bug

   
enhancement

   
evolution

Card Evolution system

  
performance

   
phase-0

Render Pipeline Optimization

  
phase-1a

Schema & Data Foundation

  
phase-1b

API & Formula Engine

  
phase-1c

Bot Integration

  
phase-1d

Deployment

  
security

   
tech-debt

   
todo
No Label
ai-changes-requested
ai-failed
ai-merged
ai-pr-opened
ai-reviewed
ai-reviewing
ai-reviewing
ai-working
bug
enhancement
evolution
performance
phase-0
phase-1a
phase-1b
phase-1c
phase-1d
security
tech-debt
todo
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/paper-dynasty-database#94
No description provided.
Delete Branch "feature/render-pipeline-optimization"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?