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 %} +