Enhance source and admin operations UI - #26
Merged
Conversation
📝 WalkthroughWalkthroughAdds admin-protected source/document APIs (read/preview/update content/rename/delete) with audit logging and reindexing; refactors frontend admin and Sources UIs for token-gated mutations and content editing; updates API client/types, auth tests, chat/sidebar reset behavior, and docs. ChangesSource and Document Management Console
Sequence Diagram(s)sequenceDiagram
participant AdminUI
participant API
participant DB
participant Embedder
participant SearchCache
AdminUI->>API: PATCH documents content
API->>DB: load document and chunks
API->>Embedder: request embeddings for chunks
Embedder->>API: return vectors
API->>DB: delete old chunks and insert new chunks/embeddings
API->>DB: update document and insert AuditLog
API->>SearchCache: bump epoch and invalidate
API->>AdminUI: return DocumentContentResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
tomnguyen103
marked this pull request as ready for review
June 7, 2026 00:18
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/components/ConversationSidebar.tsx (1)
376-382:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply consistent new-chat behavior to mobile.
The mobile "New chat" button uses a plain
Linkand doesn't dispatch thesecond-brain-new-chatevent, while the desktop version (lines 159-166) does. This inconsistency means mobile users won't trigger the chat reset logic registered inchat/page.tsx, potentially leaving stale conversation state.🔧 Proposed fix for consistency
<Link href="/chat" + onClick={(e) => { + e.preventDefault(); + window.dispatchEvent(new Event("second-brain-new-chat")); + }} className="flex h-9 w-9 items-center justify-center rounded-lg bg-foreground text-background transition-colors hover:opacity-90 focus-visible:outline-none focus-visible:ring-3 focus-visible:ring-primary/20" aria-label="New chat" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/ConversationSidebar.tsx` around lines 376 - 382, The mobile "New chat" Link in ConversationSidebar doesn't dispatch the same 'second-brain-new-chat' CustomEvent as the desktop button, so update the mobile control in ConversationSidebar (the Link rendering the Plus icon) to dispatch window.dispatchEvent(new CustomEvent('second-brain-new-chat')) on activation (e.g., onClick handler) before navigating; ensure the element still navigates to "/chat" and preserves accessibility attributes so the chat reset logic registered in chat/page.tsx receives the event consistently on both mobile and desktop.
🧹 Nitpick comments (1)
frontend/app/chat/page.tsx (1)
44-48: ⚡ Quick winConsider removing redundant history manipulation.
Both
window.history.replaceStateandrouter.replaceare called to navigate to/chat. Mixing manual History API calls with Next.js router navigation can lead to unexpected behavior, as the router maintains its own history state.router.replace("/chat", { scroll: false })should be sufficient.♻️ Simplified version
const startNewChat = useCallback(() => { resetChatState(); - window.history.replaceState(window.history.state, "", "/chat"); router.replace("/chat", { scroll: false }); }, [resetChatState, router]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/chat/page.tsx` around lines 44 - 48, The startNewChat callback currently calls both window.history.replaceState and router.replace which is redundant and can conflict with Next.js navigation; remove the manual history API call (the window.history.replaceState(...) line) and keep router.replace("/chat", { scroll: false }) inside startNewChat (referencing the startNewChat function, resetChatState, and router.replace) so navigation is handled solely through Next.js router.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/sources.py`:
- Around line 33-48: _document_summary currently computes chunk_count using
len(doc.chunks), which forces selectinload of Document.chunks and hydrates full
chunk bodies; change it to accept a pre-fetched chunk_count (do not iterate
relationship) and stop relying on doc.chunks for counts. In
list_source_documents(), update_document() and the other call sites that
currently selectinload(Document.chunks) (and any code around the symbols
mentioned) replace the relationship load with an aggregate/subquery that returns
chunk_count per document (e.g. SQLAlchemy func.count or a correlated subquery)
and pass that integer into _document_summary when constructing DocumentSummary;
remove or conditionalize any selectinload(Document.chunks) so chunk bodies are
only loaded by content-specific endpoints.
- Around line 354-387: delete_document() currently removes the Document and its
Chunks but never invalidates the search cache; add the same search-epoch bump
that update_document_content() performs immediately after the db.commit() in
delete_document() so cached searches stop returning the removed document—call
the identical function used by update_document_content() (the search epoch/cache
bump helper) with the appropriate source_id (and ensure the helper is imported)
right after the commit and before returning the DeleteDocumentResponse.
- Around line 278-329: The code currently deletes existing Chunk rows and
updates doc even if encode_with_cache returned fewer vectors than pieces (zip
silently truncates); before deleting/rebuilding embeddings, verify that
len(vectors) == len(pieces) (and treat the vectors==[] case if pieces is empty),
and if they differ log an error and abort (raise/return without mutating DB or
setting doc.status/doc.content_hash) so the existing data remains intact;
perform this check after the encode_with_cache call and before
db.execute(delete(Chunk)...), referencing encode_with_cache, pieces, vectors,
zip, doc, Chunk and Embedding in your changes.
In `@backend/tests/integration/test_sources_api.py`:
- Around line 13-16: _enable_admin currently mutates global
app.dependency_overrides permanently and omits _env_file=None, causing state
leakage across tests; change _enable_admin into a context manager that
temporarily sets app.dependency_overrides[deps.get_settings] to a lambda
returning Settings(llm_provider="fake", api_token="test-api-token",
admin_token=TOKEN, _env_file=None) and restores the original override (or
deletes it) on exit, and then wrap admin-only tests with with _enable_admin():
so the override is test-local and .env leakage is prevented.
In `@backend/tests/unit/test_api_auth.py`:
- Around line 65-74: The test matrix in backend/tests/unit/test_api_auth.py only
checks bearer token rejection for admin-only endpoints; add additional
assertions that call the admin-only routes ("patch" "/documents/1", "patch"
"/documents/1/content", "delete" "/documents/1", "delete" "/data/sources/1")
using a valid API bearer token but without the X-Second-Brain-Admin-Token header
and assert the request is rejected (403/unauthorized per app behavior) to ensure
the require_admin protection is enforced; update or add test cases around those
route entries so each has both a bearer-invalid check and a
bearer-valid/no-admin-token check referencing the same route strings to locate
the code.
In `@frontend/components/ConversationSidebar.tsx`:
- Around line 159-166: The anchor with href="/chat" triggers native navigation
alongside the onClick handler startNewChat, causing a race; update the click
handling so the default navigation is prevented and only your custom flow runs —
either modify the startNewChat function to accept an event and call
event.preventDefault(), or change the onClick to an inline handler that calls
event.preventDefault() then invokes startNewChat(); keep the anchor semantics
(or replace with a button) and ensure references to startNewChat and the <a> in
ConversationSidebar.tsx are updated accordingly.
---
Outside diff comments:
In `@frontend/components/ConversationSidebar.tsx`:
- Around line 376-382: The mobile "New chat" Link in ConversationSidebar doesn't
dispatch the same 'second-brain-new-chat' CustomEvent as the desktop button, so
update the mobile control in ConversationSidebar (the Link rendering the Plus
icon) to dispatch window.dispatchEvent(new CustomEvent('second-brain-new-chat'))
on activation (e.g., onClick handler) before navigating; ensure the element
still navigates to "/chat" and preserves accessibility attributes so the chat
reset logic registered in chat/page.tsx receives the event consistently on both
mobile and desktop.
---
Nitpick comments:
In `@frontend/app/chat/page.tsx`:
- Around line 44-48: The startNewChat callback currently calls both
window.history.replaceState and router.replace which is redundant and can
conflict with Next.js navigation; remove the manual history API call (the
window.history.replaceState(...) line) and keep router.replace("/chat", {
scroll: false }) inside startNewChat (referencing the startNewChat function,
resetChatState, and router.replace) so navigation is handled solely through
Next.js router.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8feb0f7e-396c-4a6f-8e91-72e900f72a3b
📒 Files selected for processing (15)
README.mdbackend/app/api/sources.pybackend/app/schemas/sources.pybackend/tests/conftest.pybackend/tests/integration/test_sources_api.pybackend/tests/unit/test_api_auth.pydocs/PROGRESS.mddocs/implementation-notes.mdfrontend/app/admin/page.tsxfrontend/app/chat/page.tsxfrontend/app/ingest/page.tsxfrontend/app/sources/page.tsxfrontend/components/ConversationSidebar.tsxfrontend/lib/api/client.tsfrontend/lib/api/types.ts
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/api/sources.py (2)
46-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop computing
raw_text_availableby touchingdoc.raw_text.
DocumentSummaryonly needs a boolean, but_document_summary()still reads the full text field. In bothlist_source_documents()andupdate_document(), that makes summary-only requests resolve every document body just to populateraw_text_available(or trigger one lazy load per row ifraw_textis deferred). Please pass a projected boolean into the helper and keepraw_textreserved for the content endpoints.Illustrative direction
-def _document_summary(doc: Document, *, chunk_count: int) -> DocumentSummary: +def _document_summary( + doc: Document, + *, + chunk_count: int, + raw_text_available: bool | None = None, +) -> DocumentSummary: return DocumentSummary( id=doc.id, source_id=doc.source_id, title=doc.title, @@ - raw_text_available=doc.raw_text is not None, + raw_text_available=( + doc.raw_text is not None + if raw_text_available is None + else raw_text_available + ),Then have the summary queries project
Document.raw_text.is_not(None)and pass that value into_document_summary()instead of materializingraw_text.Also applies to: 191-206, 249-265
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/sources.py` around lines 46 - 57, The helper _document_summary currently reads doc.raw_text to compute raw_text_available; change its signature to accept a raw_text_available: bool parameter (e.g., def _document_summary(doc: Document, *, chunk_count: int, raw_text_available: bool) -> DocumentSummary) and use that boolean rather than touching doc.raw_text. Update every caller (including list_source_documents, update_document and the other summary call sites around the ranges noted) to project a boolean expression (Document.raw_text.is_not(None)) in the query and pass that projected boolean into _document_summary; ensure no code reads doc.raw_text in summary-only flows so raw text stays deferred and only content endpoints load it.
64-72:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't eager-load chunk bodies on every content read.
_document_content_response()prefersraw_text, but_load_document()alwaysselectinloadsDocument.chunksand the raw-text path only uses them forlen(doc.chunks). That means both/documents/{id}/contentand/documents/{id}/previewhydrate every chunk row and its text payload even when the response is built entirely fromraw_text. Splitchunk_countfrom chunk-body fallback so chunk bodies are only fetched whenraw_textis missing.Also applies to: 76-100, 224-230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/sources.py` around lines 64 - 72, _load_document currently eager-loads Document.chunks (including full chunk bodies) even when callers like _document_content_response only need chunk count and prefer raw_text; change _load_document to remove selectinload(Document.chunks) so chunk bodies are not hydrated by default, and instead add a lightweight chunk count query (e.g., SELECT COUNT(*) FROM Chunk WHERE document_id=...) where only the count is needed, and only fetch chunk rows/bodies (via a separate query or a new loader function) in the code path that falls back to assembling content from chunks when raw_text is missing; update the other places that call _load_document (the content/preview endpoints and the other affected call sites) to use the count query or to explicitly load chunks when required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/integration/test_sources_api.py`:
- Around line 279-301: The test compares two unordered lists of chunk IDs
(old_chunk_ids and new_chunk_ids) which can produce false negatives; update the
assertion in test_sources_api.py to be order-insensitive by comparing sorted
lists or sets (e.g., assert set(new_chunk_ids) == set(old_chunk_ids) or assert
sorted(new_chunk_ids) == sorted(old_chunk_ids)) after the PATCH flow that
populates new_chunk_ids so the test only verifies preservation of the same rows
regardless of order.
---
Outside diff comments:
In `@backend/app/api/sources.py`:
- Around line 46-57: The helper _document_summary currently reads doc.raw_text
to compute raw_text_available; change its signature to accept a
raw_text_available: bool parameter (e.g., def _document_summary(doc: Document,
*, chunk_count: int, raw_text_available: bool) -> DocumentSummary) and use that
boolean rather than touching doc.raw_text. Update every caller (including
list_source_documents, update_document and the other summary call sites around
the ranges noted) to project a boolean expression
(Document.raw_text.is_not(None)) in the query and pass that projected boolean
into _document_summary; ensure no code reads doc.raw_text in summary-only flows
so raw text stays deferred and only content endpoints load it.
- Around line 64-72: _load_document currently eager-loads Document.chunks
(including full chunk bodies) even when callers like _document_content_response
only need chunk count and prefer raw_text; change _load_document to remove
selectinload(Document.chunks) so chunk bodies are not hydrated by default, and
instead add a lightweight chunk count query (e.g., SELECT COUNT(*) FROM Chunk
WHERE document_id=...) where only the count is needed, and only fetch chunk
rows/bodies (via a separate query or a new loader function) in the code path
that falls back to assembling content from chunks when raw_text is missing;
update the other places that call _load_document (the content/preview endpoints
and the other affected call sites) to use the count query or to explicitly load
chunks when required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 811509e2-0cde-416c-8a7e-481df8951c7a
📒 Files selected for processing (6)
backend/app/api/sources.pybackend/tests/integration/test_sources_api.pybackend/tests/unit/test_api_auth.pydocs/PROGRESS.mdfrontend/app/chat/page.tsxfrontend/components/ConversationSidebar.tsx
💤 Files with no reviewable changes (1)
- frontend/app/chat/page.tsx
✅ Files skipped from review due to trivial changes (1)
- docs/PROGRESS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/components/ConversationSidebar.tsx
Comment on lines
+279
to
+301
| old_chunk_ids = [ | ||
| c.id for c in db_session.query(Chunk).filter(Chunk.document_id == document_id) | ||
| ] | ||
| assert old_chunk_ids | ||
|
|
||
| previous_embedder = app.dependency_overrides.get(deps.get_embedder) | ||
| app.dependency_overrides[deps.get_embedder] = lambda: _ShortVectorEmbedder() | ||
| try: | ||
| resp = client.patch( | ||
| f"/documents/{document_id}/content", | ||
| json={"content": "new content that cannot be fully embedded " * 60}, | ||
| headers=ADMIN, | ||
| ) | ||
|
|
||
| assert resp.status_code == 502 | ||
| db_session.expire_all() | ||
| doc = db_session.get(Document, document_id) | ||
| assert doc.content_hash == old_hash | ||
| assert doc.raw_text == old_raw_text | ||
| new_chunk_ids = [ | ||
| c.id for c in db_session.query(Chunk).filter(Chunk.document_id == document_id) | ||
| ] | ||
| assert new_chunk_ids == old_chunk_ids |
There was a problem hiding this comment.
Make the preserved-chunk assertion order-insensitive.
Both chunk-ID queries are unordered, so new_chunk_ids == old_chunk_ids can fail even when the same rows are preserved. Sort them or compare sets here to avoid a flaky test.
Suggested fix
- old_chunk_ids = [
- c.id for c in db_session.query(Chunk).filter(Chunk.document_id == document_id)
- ]
+ old_chunk_ids = {
+ c.id for c in db_session.query(Chunk).filter(Chunk.document_id == document_id)
+ }
assert old_chunk_ids
@@
- new_chunk_ids = [
- c.id for c in db_session.query(Chunk).filter(Chunk.document_id == document_id)
- ]
+ new_chunk_ids = {
+ c.id for c in db_session.query(Chunk).filter(Chunk.document_id == document_id)
+ }
assert new_chunk_ids == old_chunk_ids📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| old_chunk_ids = [ | |
| c.id for c in db_session.query(Chunk).filter(Chunk.document_id == document_id) | |
| ] | |
| assert old_chunk_ids | |
| previous_embedder = app.dependency_overrides.get(deps.get_embedder) | |
| app.dependency_overrides[deps.get_embedder] = lambda: _ShortVectorEmbedder() | |
| try: | |
| resp = client.patch( | |
| f"/documents/{document_id}/content", | |
| json={"content": "new content that cannot be fully embedded " * 60}, | |
| headers=ADMIN, | |
| ) | |
| assert resp.status_code == 502 | |
| db_session.expire_all() | |
| doc = db_session.get(Document, document_id) | |
| assert doc.content_hash == old_hash | |
| assert doc.raw_text == old_raw_text | |
| new_chunk_ids = [ | |
| c.id for c in db_session.query(Chunk).filter(Chunk.document_id == document_id) | |
| ] | |
| assert new_chunk_ids == old_chunk_ids | |
| old_chunk_ids = { | |
| c.id for c in db_session.query(Chunk).filter(Chunk.document_id == document_id) | |
| } | |
| assert old_chunk_ids | |
| previous_embedder = app.dependency_overrides.get(deps.get_embedder) | |
| app.dependency_overrides[deps.get_embedder] = lambda: _ShortVectorEmbedder() | |
| try: | |
| resp = client.patch( | |
| f"/documents/{document_id}/content", | |
| json={"content": "new content that cannot be fully embedded " * 60}, | |
| headers=ADMIN, | |
| ) | |
| assert resp.status_code == 502 | |
| db_session.expire_all() | |
| doc = db_session.get(Document, document_id) | |
| assert doc.content_hash == old_hash | |
| assert doc.raw_text == old_raw_text | |
| new_chunk_ids = { | |
| c.id for c in db_session.query(Chunk).filter(Chunk.document_id == document_id) | |
| } | |
| assert new_chunk_ids == old_chunk_ids |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/integration/test_sources_api.py` around lines 279 - 301, The
test compares two unordered lists of chunk IDs (old_chunk_ids and new_chunk_ids)
which can produce false negatives; update the assertion in test_sources_api.py
to be order-insensitive by comparing sorted lists or sets (e.g., assert
set(new_chunk_ids) == set(old_chunk_ids) or assert sorted(new_chunk_ids) ==
sorted(old_chunk_ids)) after the PATCH flow that populates new_chunk_ids so the
test only verifies preservation of the same rows regardless of order.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SECOND_BRAIN_API_TOKENsidebar usage in README..envadmin/API tokens do not change test expectations.Verification
npm run lintnpm run buildSECOND_BRAIN_LLM_PROVIDER=fake SECOND_BRAIN_TEST_DATABASE_URL=postgresql+psycopg://second_brain:second_brain@localhost:5433/second_brain python -m pytest tests/unit/test_api_auth.py tests/integration/test_sources_api.pygit diff --checkNotes
mainwas rejected by repository rules requiring PRs, so this is opened as a draft.npm run build.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests