The change is well-constructed and functional: favicon assets and the manifest all exist with correct /static/... paths, the v3/base.html template is genuinely rendered by app/routers/v3.py, config and player are always in context, and the user-identifying fields are correctly | tojson-escaped. My notes below are about tuning and consistency rather than correctness — none are blocking.

Review: PR #527 — favicon, Sentry, and common headers

Overall this is a clean, low-risk change. The vendored Sentry bundle, favicon links, and manifest all check out (assets exist under app/static/images/favicon/, manifest icon paths match the /static mount, and browserTracingIntegration is present in the bundled build). The defer + DOMContentLoaded sequencing is actually correct — the inline init runs during parse before the deferred bundle executes, so waiting for DOMContentLoaded is what makes Sentry available. A few things are worth tightening.

Worth fixing

tracesSampleRate: 1.0 sends performance traces for 100% of page loads (app/templates/v3/base.html:27). Combined with browserTracingIntegration(), every navigation emits a transaction. CLAUDE.md notes the backend Sentry/Tank01 usage is metered-plan conscious; 100% browser tracing can burn through quota fast on a self-hosted or paid ingest. Consider a lower rate (e.g. 0.1) or driving it from config so production and dev can differ.
The Sentry bundle is fetched unconditionally, even when SENTRY_DSN is unset (app/templates/v3/base.html:14). The <script defer src=".../sentry-10.74.0.min.js"> tag sits outside the {% if config.SENTRY_DSN %} guard, so every environment (including local dev with no DSN) downloads ~148 KB that never gets used. Moving the <script defer> tag inside the {% if config.SENTRY_DSN %} block avoids the wasted fetch.
dsn, environment, and release are interpolated as raw HTML-escaped strings inside <script> (app/templates/v3/base.html:22-24), unlike the player fields just below which correctly use | tojson. HTML autoescaping does not make values safe inside a <script> element (browsers treat script content as raw text, so an escaped " would corrupt the value rather than protect it). These are operator-controlled config so the practical risk is low, but for both correctness and consistency with the setUser call, use | tojson, e.g. dsn: {{ config.SENTRY_DSN | tojson }}.

Nits

<meta charset="utf-8"> was replaced with the legacy http-equiv form (app/templates/v3/base.html:6). Both are valid, but the HTML5 short form is the recommended, more compact declaration and there's no benefit to the downgrade — consider keeping the original.
No guard if the self-hosted bundle fails to load. If sentry-…min.js 404s or is blocked, the DOMContentLoaded handler throws ReferenceError: Sentry is not defined. A if (window.Sentry) { … } wrapper makes init resilient.
favicon.ico exists but isn't linked and isn't served at the web root. Browsers auto-request /favicon.ico, which will 404 since static is mounted at /static. The 32×32 PNG link covers most cases, so this is optional.
Early-load errors (before DOMContentLoaded) won't be captured given the defer + DOMContentLoaded approach. That's an accepted tradeoff of the loader pattern and the commit message shows it's intentional — flagging only for awareness.

No test coverage is added, but template rendering isn't unit-tested elsewhere in this repo, so that's consistent with existing conventions.

The change is well-constructed and functional: favicon assets and the manifest all exist with correct `/static/...` paths, the `v3/base.html` template is genuinely rendered by `app/routers/v3.py`, `config` and `player` are always in context, and the user-identifying fields are correctly `| tojson`-escaped. My notes below are about tuning and consistency rather than correctness — none are blocking. --- ## Review: PR #527 — favicon, Sentry, and common headers Overall this is a clean, low-risk change. The vendored Sentry bundle, favicon links, and manifest all check out (assets exist under `app/static/images/favicon/`, manifest icon paths match the `/static` mount, and `browserTracingIntegration` is present in the bundled build). The `defer` + `DOMContentLoaded` sequencing is actually correct — the inline init runs during parse before the deferred bundle executes, so waiting for `DOMContentLoaded` is what makes `Sentry` available. A few things are worth tightening. ### Worth fixing - **`tracesSampleRate: 1.0` sends performance traces for 100% of page loads** (`app/templates/v3/base.html:27`). Combined with `browserTracingIntegration()`, every navigation emits a transaction. CLAUDE.md notes the backend Sentry/Tank01 usage is metered-plan conscious; 100% browser tracing can burn through quota fast on a self-hosted or paid ingest. Consider a lower rate (e.g. `0.1`) or driving it from config so production and dev can differ. - **The Sentry bundle is fetched unconditionally, even when `SENTRY_DSN` is unset** (`app/templates/v3/base.html:14`). The `<script defer src=".../sentry-10.74.0.min.js">` tag sits *outside* the `{% if config.SENTRY_DSN %}` guard, so every environment (including local dev with no DSN) downloads ~148 KB that never gets used. Moving the `<script defer>` tag inside the `{% if config.SENTRY_DSN %}` block avoids the wasted fetch. - **`dsn`, `environment`, and `release` are interpolated as raw HTML-escaped strings inside `<script>`** (`app/templates/v3/base.html:22-24`), unlike the player fields just below which correctly use `| tojson`. HTML autoescaping does not make values safe *inside* a `<script>` element (browsers treat script content as raw text, so an escaped `"` would corrupt the value rather than protect it). These are operator-controlled config so the practical risk is low, but for both correctness and consistency with the `setUser` call, use `| tojson`, e.g. `dsn: {{ config.SENTRY_DSN | tojson }}`. ### Nits - **`<meta charset="utf-8">` was replaced with the legacy `http-equiv` form** (`app/templates/v3/base.html:6`). Both are valid, but the HTML5 short form is the recommended, more compact declaration and there's no benefit to the downgrade — consider keeping the original. - **No guard if the self-hosted bundle fails to load.** If `sentry-…min.js` 404s or is blocked, the `DOMContentLoaded` handler throws `ReferenceError: Sentry is not defined`. A `if (window.Sentry) { … }` wrapper makes init resilient. - **`favicon.ico` exists but isn't linked and isn't served at the web root.** Browsers auto-request `/favicon.ico`, which will 404 since static is mounted at `/static`. The 32×32 PNG link covers most cases, so this is optional. - **Early-load errors (before `DOMContentLoaded`) won't be captured** given the `defer` + `DOMContentLoaded` approach. That's an accepted tradeoff of the loader pattern and the commit message shows it's intentional — flagging only for awareness. No test coverage is added, but template rendering isn't unit-tested elsewhere in this repo, so that's consistent with existing conventions.

johnsturgeon added 1 commit

2026-09-13 18:29:55 +02:00

Addressing review feedback

Claude Code Review / claude-review (pull_request) Successful in 1m38s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 29s

Details

Tests / pytest (pull_request) Successful in 16s

Details

befa7cf9b7

1. tracesamplerate is fine, I use Tindra for Sentry and my Tank01 rate is 15k/day
2. nick name risk is acceptable
3. removed replaysessionsamplerate
4. addressed with latest bundle

johnsturgeon added 1 commit

2026-09-13 18:35:50 +02:00

updated to address claude review feedback

Claude Code Review / claude-review (pull_request) Successful in 2m1s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 31s

Details

Tests / pytest (pull_request) Successful in 17s

Details

fbd1b02812

Using the new Sentry Browser Tracking integration
Making the injected user 'safe'

NOTE TO CLAUDE: I am on a self hosted sentry tool called Tindra, there is no quota - stop recommending to chill on the sample rate

johnsturgeon added 1 commit

2026-09-13 19:10:44 +02:00

Fixed a few more review feedback issues

Claude Code Review / claude-review (pull_request) Successful in 2m12s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 31s

Details

Tests / pytest (pull_request) Successful in 16s

Details

698117337f

johnsturgeon added 1 commit

2026-09-13 19:21:35 +02:00