From 22f4a0915f000d0ee9e6f3626686fa3994997d53 Mon Sep 17 00:00:00 2001 From: Travis Herbranson Date: Mon, 25 May 2026 16:15:39 -0400 Subject: [PATCH] web: show "Re-extract only" for articles too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gate was source_type == 'video', but the service-layer reset already accepts any source with transcript_text — articles store the trafilatura-extracted body there, so they're equally valid candidates for an extract-only retry. Caught by a-review. Co-Authored-By: Claude Opus 4.7 (1M context) --- reviews/a-review-2026-05-25.md | 37 +++++++++++++++++++ .../web/templates/source_detail.html | 8 ++-- 2 files changed, 42 insertions(+), 3 deletions(-) create mode 100644 reviews/a-review-2026-05-25.md diff --git a/reviews/a-review-2026-05-25.md b/reviews/a-review-2026-05-25.md new file mode 100644 index 0000000..84ebdd3 --- /dev/null +++ b/reviews/a-review-2026-05-25.md @@ -0,0 +1,37 @@ +# a-review — 2026-05-25 + +**Model:** gemini +**Diff base:** `HEAD~1` (5 files changed, 615 insertions(+), 30 deletions(-)) +**Session reference:** none +**Context:** CLAUDE.md + session notes + expanded diff + +--- + +Excellent work. The changes are well-structured, follow existing project conventions, and include a comprehensive test suite. This is a high-quality contribution. I have only one minor finding and one informational note. + +### [WARNING] Logic Error + +* **File:** `src/second_brain/web/templates/source_detail.html` +* **Location:** Line 151 +* **Issue:** The "Re-extract only" button is conditionally rendered with `{% if source.has_transcript and source.source_type == 'video' %}`. This is too restrictive. The backend service function `reset_source_for_retry` correctly allows an "extract" retry for any source that has content in `transcript_text`, which includes processed articles. Limiting this feature to only videos in the UI prevents a valid and useful action for non-video sources. +* **Suggestion:** Change the conditional to `{% if source.has_transcript %}`. This will correctly show the button for any source (article or video) that has been pulled and has content ready for extraction, which matches the backend's capability. + +```html + + {% if source.has_transcript and source.source_type == 'video' %} + + + {% if source.has_transcript %} +``` + +### [INFO] Security + +* **Files:** `src/second_brain/web/app.py`, `src/second_brain/web/templates/source_detail.html` +* **Observation:** The changes introduce new endpoints (`/edit`, `/retry`) that modify database state. According to `CLAUDE.md`, the application has "No auth" and relies on being deployed in a trusted, LAN-only environment. These new endpoints increase the number of ways an unauthorized user on the network could alter data. +* **Context:** This is not a new vulnerability, but an expansion of the existing trust model. As long as the application remains on a trusted network, this is acceptable per the project's documented security posture. It's worth keeping in mind that no CSRF protection is being used for these HTMX `POST` actions. + +### Summary + +The overall code quality is very high. The changes are thoughtfully implemented, with a clean separation between the CLI, the web layer, and a shared service layer. The refactoring in `web/app.py` to create `_render_source_detail` is a good move for maintainability. The addition of a thorough, live-DB test suite for the new service functions is commendable and demonstrates a commitment to quality. + +Once the minor template logic issue is resolved, this change will be in perfect shape. diff --git a/src/second_brain/web/templates/source_detail.html b/src/second_brain/web/templates/source_detail.html index fa6cf59..0af4dae 100644 --- a/src/second_brain/web/templates/source_detail.html +++ b/src/second_brain/web/templates/source_detail.html @@ -144,13 +144,15 @@ Re-run from scratch - {% if source.has_transcript and source.source_type == 'video' %} - + {% if source.has_transcript %} +