web: show "Re-extract only" for articles too
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) <noreply@anthropic.com>
This commit is contained in:
parent
5131f3ccf6
commit
22f4a0915f
37
reviews/a-review-2026-05-25.md
Normal file
37
reviews/a-review-2026-05-25.md
Normal file
@ -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
|
||||||
|
<!-- Change this -->
|
||||||
|
{% if source.has_transcript and source.source_type == 'video' %}
|
||||||
|
|
||||||
|
<!-- To this -->
|
||||||
|
{% 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.
|
||||||
@ -144,13 +144,15 @@
|
|||||||
Re-run from scratch
|
Re-run from scratch
|
||||||
</button>
|
</button>
|
||||||
|
|
||||||
{% if source.has_transcript and source.source_type == 'video' %}
|
{% if source.has_transcript %}
|
||||||
<!-- Light retry: skip the re-download/re-transcribe, just re-extract. -->
|
<!-- Light retry: skip the re-pull, just re-run LLM extraction. Works
|
||||||
|
for any source with stored body text (videos have transcripts,
|
||||||
|
articles have the trafilatura-extracted body). -->
|
||||||
<button hx-post="/sources/{{ source.id }}/retry"
|
<button hx-post="/sources/{{ source.id }}/retry"
|
||||||
hx-vals='{"mode": "extract"}'
|
hx-vals='{"mode": "extract"}'
|
||||||
hx-target="body"
|
hx-target="body"
|
||||||
hx-swap="innerHTML"
|
hx-swap="innerHTML"
|
||||||
hx-confirm="Re-extract only — keep the existing transcript and re-run the LLM extraction?"
|
hx-confirm="Re-extract only — keep the existing transcript/body and re-run the LLM extraction?"
|
||||||
class="px-5 py-2 bg-amber-100 text-amber-800 text-sm font-semibold rounded hover:bg-amber-200 border border-amber-300 transition-colors">
|
class="px-5 py-2 bg-amber-100 text-amber-800 text-sm font-semibold rounded hover:bg-amber-200 border border-amber-300 transition-colors">
|
||||||
Re-extract only
|
Re-extract only
|
||||||
</button>
|
</button>
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user