petal-power/second-brain
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
claude/source-edit-and-retry
second-brain/reviews/a-review-2026-05-25.md
Travis Herbranson 22f4a0915f 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>
2026-05-25 16:15:39 -04:00

2.6 KiB
Raw Permalink Blame History

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.
    <!-- 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.