Repository navigation
Conversation
…rview flare_docs search/get previously only indexed the crate-root docstring, so a query for e.g. axum::extract::State only hit if that string happened to appear on the crate's overview page. store_fetched now also parses the rustdoc-JSON index/paths maps and upserts one document per item that has both a docstring and a public path, using a trimmed IndexCrate/IndexItem deserializer that never materializes the ItemEnum subtree (item kind comes from paths[id].kind instead). Indexing thousands of items per crate one upsert at a time would mean thousands of individual transactions per fetch, so DocsStore gains upsert_batch: one BEGIN IMMEDIATE transaction for a crate's whole item set. That batch path also exposed a couple of agentflare-store gaps that predate this per-item use case: doc_upsert_with_opts unconditionally snapshotted a history row on every write (pure amplification for docs.rs content, which is stable across refreshes) and there was no DB-level uniqueness constraint on (project_id, path), only the single-writer application code path. DocUpsertOpts gains a track_history flag (default true, existing callers unaffected) and doc_upsert_with_opts now skips the history snapshot when content is unchanged; a UNIQUE INDEX on (project_id, path) backs the new batch path's ON CONFLICT upsert. Agentflare-Agent: claude-code_2-1-220_agent Agentflare-Branch: task/343 Agentflare-Item: 343
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds configurable document history snapshots and unique document keys, introduces rustdoc item extraction, adds transactional batch document upserts, normalizes FTS queries, and stores searchable item documentation during rustdoc ingestion. ChangesDocumentation indexing and storage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RustdocFetcher
participant index_types
participant DocsStore
participant SQLite
RustdocFetcher->>index_types: Parse rustdoc JSON index and paths
index_types->>RustdocFetcher: Return documented indexed items
RustdocFetcher->>DocsStore: Submit BatchItem records
DocsStore->>SQLite: Upsert documents and refresh full-text search
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/flare-docs/src/store.rs (1)
80-141: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStatements re-prepared on every loop iteration; consider
prepare_cached.The doc comment on
upsert_batchexplicitly calls out avoiding per-item transaction overhead for crates with "thousands of items," but the loop body still callstx.query_row(...)/tx.execute(...)with raw SQL strings, which re-parses each of the 3 statements on every iteration (noprepare_cached). For a large rustdoc index this adds up. Hoisting totx.prepare_cached(sql)?(rusqlite caches by SQL text) would cut re-parsing cost while keeping the single-transaction behavior.♻️ Proposed refactor using prepare_cached
- for item in items { - let tags_json = serde_json::to_string(&item.tags).unwrap_or_else(|_| "[]".into()); - let id = db_kit::ids::new_id(); - let rowid: i64 = tx.query_row( - "INSERT INTO store_documents + let mut upsert_stmt = tx.prepare_cached( + "INSERT INTO store_documents (id, project_id, path, content, title, doc_type, blob_hash, mime, tags, session_id, source, metadata, size, version, created_at, updated_at) VALUES (?1, ?2, ?3, ?4, ?5, ?6, NULL, '', ?7, NULL, ?8, '{}', 0, 1, ?9, ?9) ON CONFLICT(project_id, path) DO UPDATE SET content = excluded.content, title = excluded.title, doc_type = excluded.doc_type, tags = excluded.tags, source = excluded.source, version = store_documents.version + 1, updated_at = excluded.updated_at, deleted_at = NULL - RETURNING rowid", + RETURNING rowid", + )?; + let mut fts_del_stmt = tx.prepare_cached("DELETE FROM store_docs_fts WHERE rowid = ?1")?; + let mut fts_ins_stmt = + tx.prepare_cached("INSERT INTO store_docs_fts(rowid, content) VALUES (?1, ?2)")?; + + for item in items { + let tags_json = serde_json::to_string(&item.tags).unwrap_or_else(|_| "[]".into()); + let id = db_kit::ids::new_id(); + let rowid: i64 = upsert_stmt.query_row( rusqlite::params![ id, PROJECT_ID, item.path, item.content, item.title, item.doc_type, tags_json, item.source, now, ], |row| row.get(0), )?; - tx.execute( - "DELETE FROM store_docs_fts WHERE rowid = ?1", - rusqlite::params![rowid], - )?; - tx.execute( - "INSERT INTO store_docs_fts(rowid, content) VALUES (?1, ?2)", - rusqlite::params![rowid, item.content], - )?; + fts_del_stmt.execute(rusqlite::params![rowid])?; + fts_ins_stmt.execute(rusqlite::params![rowid, item.content])?; }🤖 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 `@crates/flare-docs/src/store.rs` around lines 80 - 141, Update upsert_batch to prepare and cache the upsert, FTS delete, and FTS insert statements once before iterating over items, using tx.prepare_cached with the existing SQL and parameters. Reuse those prepared statements inside the loop while preserving the current transaction, rowid handling, and return behavior.
🤖 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 `@crates/agentflare-store/src/documents.rs`:
- Around line 184-196: Update the history-snapshot condition in the document
upsert flow around new_version and the optional-field UPDATEs so history is
skipped only for a true no-op, not merely when old_content equals content. When
track_history is enabled and any persisted document field changes, insert the
superseded version into store_doc_history; retain the skip behavior for
identical-content re-upserts with no other changes and for track_history being
false.
In `@crates/agentflare-store/src/migrations.rs`:
- Around line 89-91: Update the migration’s M::up sequence in migrations.rs to
remove or deterministically consolidate existing duplicate store_documents rows
grouped by (project_id, path) before creating idx_docs_project_path. Ensure the
cleanup preserves the intended row data, then create the unique index so
upsert_batch’s ON CONFLICT(project_id, path) contract works on databases with
pre-existing duplicates.
In `@crates/flare-docs/src/rustdoc.rs`:
- Around line 131-143: Replace the direct upsert-only flow around indexed_items
and store.upsert_batch with a scoped transactional reconciliation for the
current crate path: upsert the current BatchItem entries, remove document and
FTS rows under crate_id_path/item whose paths are absent from the fetched items,
and preserve atomicity. Add coverage for re-fetching latest after an item
disappears, verifying stale content is removed.
---
Nitpick comments:
In `@crates/flare-docs/src/store.rs`:
- Around line 80-141: Update upsert_batch to prepare and cache the upsert, FTS
delete, and FTS insert statements once before iterating over items, using
tx.prepare_cached with the existing SQL and parameters. Reuse those prepared
statements inside the loop while preserving the current transaction, rowid
handling, and return behavior.
🪄 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 Plus
Run ID: 4f8d13c1-570a-41f8-b701-dfba97eecbaf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/agentflare-store/src/documents.rscrates/agentflare-store/src/migrations.rscrates/flare-docs/Cargo.tomlcrates/flare-docs/src/index_types.rscrates/flare-docs/src/lib.rscrates/flare-docs/src/rustdoc.rscrates/flare-docs/src/store.rs
Comment on lines
184
to
+196
|
|
||
| // Snapshot current version to history | ||
| tx.execute( | ||
| "INSERT INTO store_doc_history (id, doc_id, version, content, blob_hash, mime, title, metadata, size, created_at) | ||
| VALUES (?1, ?2, ?3, ?4, ?5, ?6, (SELECT title FROM store_documents WHERE id = ?2), ?7, ?8, ?9)", | ||
| params![history_id, existing_id, old_version, old_content, old_blob_hash, old_mime, old_metadata, old_size, now], | ||
| )?; | ||
| // Snapshot current version to history, unless the caller opted | ||
| // out (cache-type documents) or content is byte-identical to | ||
| // what's already stored (a no-op re-upsert has nothing to | ||
| // snapshot). | ||
| if opts.track_history && old_content != content { | ||
| let history_id = db_kit::ids::new_id(); | ||
| tx.execute( | ||
| "INSERT INTO store_doc_history (id, doc_id, version, content, blob_hash, mime, title, metadata, size, created_at) | ||
| VALUES (?1, ?2, ?3, ?4, ?5, ?6, (SELECT title FROM store_documents WHERE id = ?2), ?7, ?8, ?9)", | ||
| params![history_id, existing_id, old_version, old_content, old_blob_hash, old_mime, old_metadata, old_size, now], | ||
| )?; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
History snapshot skipped by content-equality creates version gaps when other fields change.
The skip condition is opts.track_history && old_content != content, but new_version = old_version + 1 (line 183) and the optional-field UPDATEs (lines 206-261) are applied unconditionally. If a caller updates only metadata, tags, title, etc. while content stays the same (with track_history: true, the default), the document's version still increments but no store_doc_history row is written for the superseded version. This leaves a permanent gap: doc_get_version(id, N) for that version returns None even though version N existed, and doc_history no longer reflects a contiguous version sequence. The new tests (identical_content_reupsert_skips_history_row, track_history_false_skips_history_row) only exercise the "nothing else changed" case, so this gap isn't caught.
🐛 Proposed fix: only skip when the call is a true no-op
- // Snapshot current version to history, unless the caller opted
- // out (cache-type documents) or content is byte-identical to
- // what's already stored (a no-op re-upsert has nothing to
- // snapshot).
- if opts.track_history && old_content != content {
+ // Snapshot current version to history, unless the caller opted
+ // out (cache-type documents) or this call is a true no-op
+ // re-upsert (content unchanged AND no other field is being set).
+ let is_no_op_reupsert = old_content == content
+ && opts.title.is_none()
+ && opts.doc_type.is_none()
+ && opts.blob_hash.is_none()
+ && opts.mime.is_none()
+ && opts.tags.is_none()
+ && opts.session_id.is_none()
+ && opts.source.is_none()
+ && opts.metadata.is_none()
+ && opts.size.is_none();
+ if opts.track_history && !is_no_op_reupsert {
let history_id = db_kit::ids::new_id();📝 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
| // Snapshot current version to history | |
| tx.execute( | |
| "INSERT INTO store_doc_history (id, doc_id, version, content, blob_hash, mime, title, metadata, size, created_at) | |
| VALUES (?1, ?2, ?3, ?4, ?5, ?6, (SELECT title FROM store_documents WHERE id = ?2), ?7, ?8, ?9)", | |
| params![history_id, existing_id, old_version, old_content, old_blob_hash, old_mime, old_metadata, old_size, now], | |
| )?; | |
| // Snapshot current version to history, unless the caller opted | |
| // out (cache-type documents) or content is byte-identical to | |
| // what's already stored (a no-op re-upsert has nothing to | |
| // snapshot). | |
| if opts.track_history && old_content != content { | |
| let history_id = db_kit::ids::new_id(); | |
| tx.execute( | |
| "INSERT INTO store_doc_history (id, doc_id, version, content, blob_hash, mime, title, metadata, size, created_at) | |
| VALUES (?1, ?2, ?3, ?4, ?5, ?6, (SELECT title FROM store_documents WHERE id = ?2), ?7, ?8, ?9)", | |
| params![history_id, existing_id, old_version, old_content, old_blob_hash, old_mime, old_metadata, old_size, now], | |
| )?; | |
| } | |
| // Snapshot current version to history, unless the caller opted | |
| // out (cache-type documents) or this call is a true no-op | |
| // re-upsert (content unchanged AND no other field is being set). | |
| let is_no_op_reupsert = old_content == content | |
| && opts.title.is_none() | |
| && opts.doc_type.is_none() | |
| && opts.blob_hash.is_none() | |
| && opts.mime.is_none() | |
| && opts.tags.is_none() | |
| && opts.session_id.is_none() | |
| && opts.source.is_none() | |
| && opts.metadata.is_none() | |
| && opts.size.is_none(); | |
| if opts.track_history && !is_no_op_reupsert { | |
| let history_id = db_kit::ids::new_id(); | |
| tx.execute( |
🤖 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 `@crates/agentflare-store/src/documents.rs` around lines 184 - 196, Update the
history-snapshot condition in the document upsert flow around new_version and
the optional-field UPDATEs so history is skipped only for a true no-op, not
merely when old_content equals content. When track_history is enabled and any
persisted document field changes, insert the superseded version into
store_doc_history; retain the skip behavior for identical-content re-upserts
with no other changes and for track_history being false.
Comment on lines
+89
to
+91
| M::up( | ||
| "CREATE UNIQUE INDEX IF NOT EXISTS idx_docs_project_path ON store_documents(project_id, path);", | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any other direct INSERT INTO store_documents outside the Store API
# that could have bypassed the (project_id, path) uniqueness assumption.
ast-grep run --pattern 'INSERT INTO store_documents' --lang sql crates 2>/dev/null
rg -n --type=rust -C2 'INSERT INTO store_documents' cratesRepository: getappz/agentflare
Length of output: 1944
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and inspect the migration + write paths.
sed -n '1,180p' crates/agentflare-store/src/migrations.rs
printf '\n--- documents.rs upsert area ---\n'
sed -n '140,210p' crates/agentflare-store/src/documents.rs
printf '\n--- documents.rs insert area ---\n'
sed -n '250,320p' crates/agentflare-store/src/documents.rs
printf '\n--- flare-docs store.rs upsert area ---\n'
sed -n '70,130p' crates/flare-docs/src/store.rsRepository: getappz/agentflare
Length of output: 12799
Clean up existing (project_id, path) duplicates before this index lands CREATE UNIQUE INDEX IF NOT EXISTS will abort on any pre-existing duplicates, and upsert_batch depends on this constraint for ON CONFLICT(project_id, path).
🤖 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 `@crates/agentflare-store/src/migrations.rs` around lines 89 - 91, Update the
migration’s M::up sequence in migrations.rs to remove or deterministically
consolidate existing duplicate store_documents rows grouped by (project_id,
path) before creating idx_docs_project_path. Ensure the cleanup preserves the
intended row data, then create the unique index so upsert_batch’s ON
CONFLICT(project_id, path) contract works on databases with pre-existing
duplicates.
Comment on lines
+131
to
+143
| let batch: Vec<BatchItem> = indexed_items(&index_crate) | ||
| .into_iter() | ||
| .map(|item| BatchItem { | ||
| path: format!("{crate_id_path}/item/{}", item.fq_path), | ||
| content: item.docs, | ||
| title: item.name, | ||
| doc_type: "rust-item".to_string(), | ||
| tags: vec![crate_name.to_string(), "rust".to_string(), item.kind], | ||
| source: "docsrs".to_string(), | ||
| }) | ||
| .collect(); | ||
|
|
||
| Ok(store.upsert_batch(&batch)?) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reconcile items removed by a later fetch.
upsert_batch only inserts or updates current paths; it never deletes prior ${crate_id_path}/item/... rows absent from indexed_items. Re-fetching latest after an item is removed, privatized, or loses docs leaves stale content in FTS. Add a scoped, transactional replace/reconciliation operation that removes stale document and FTS rows, and test a re-fetch where an item disappears.
🤖 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 `@crates/flare-docs/src/rustdoc.rs` around lines 131 - 143, Replace the direct
upsert-only flow around indexed_items and store.upsert_batch with a scoped
transactional reconciliation for the current crate path: upsert the current
BatchItem entries, remove document and FTS rows under crate_id_path/item whose
paths are absent from the fetched items, and preserve atomicity. Add coverage
for re-fetching latest after an item disappears, verifying stale content is
removed.
…yntax Found live while dogfeeding the new per-item rustdoc indexing: searching the exact fully-qualified path syntax this feature exists to make searchable (e.g. "axum::extract::State") errored with "no such column: axum" instead of returning results, because doc_search passed the raw query straight to FTS5 MATCH, and FTS5 parses a bare `word:` prefix as column-filter syntax. flare-search-kit already has fts_phrase_query for exactly this shape of query (a single-column table matched with a bare `MATCH ?1`, no AND/OR structure of our own) -- skill-registry's search already depends on the same crate's fts_query for the equivalent problem. Wires it into doc_search instead of reinventing sanitization. Agentflare-Agent: claude-code_2-1-220_agent Agentflare-Branch: task/343 Agentflare-Item: 343
5 tasks done
store_fetched propagated index_items' error via ?, even though the crate overview doc had already been committed by that point. A single ItemKind variant rustdoc-types doesn't recognize fails IndexCrate's deserialization for the entire crate's item set, which turned an otherwise-successful fetch into a reported failure. Log and continue instead. Also make upsert_batch skip the FTS resync and version bump when an item's content is byte-identical to what's already stored, matching the content-hash short-circuit doc_upsert_with_opts already has for the single-doc path -- otherwise every refetch of an unchanged crate still pays full per-item write cost and grows version unboundedly.
# Conflicts: # Cargo.lock
This was referenced Jul 25, 2026
Closed
5 tasks done
This was referenced Jul 26, 2026
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
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
flare_docssearch/get previously only indexed the crate-root docstring (extract_root_docstring); a query for e.g.axum::extract::Stateonly hit if that exact text appeared on the crate's overview page. Found while dogfooding the tool against real crates.store_fetchednow also parses the already-fetched rustdoc-JSONindex/pathsmaps and upserts one document per item that has both a docstring and a public path, via a new trimmedIndexCrate/IndexItemdeserializer (crates/flare-docs/src/index_types.rs) that never materializes the (large, deeply nested)ItemEnumsubtree — item kind comes frompaths[id].kindinstead.DocsStore::upsertat a time would mean thousands of individualBEGIN IMMEDIATEtransactions per fetch. AddedDocsStore::upsert_batch— one transaction for a whole crate's item set.agentflare-store's document schema that predate this per-item use case (it was designed for occasional user documents, not high-fan-out cache rows):doc_upsert_with_optsunconditionally wrote astore_doc_historysnapshot row on every update, even when content was byte-identical — pure write amplification for docs.rs content, which is stable across refreshes. AddedDocUpsertOpts.track_history: bool(defaulttrue, so existing callers are unaffected) and a content-hash short-circuit that skips the history write when content hasn't changed.(project_id, path)— only the single-writer application code path enforced it. AddedCREATE UNIQUE INDEX idx_docs_project_path, which the new batch path'sINSERT ... ON CONFLICTupsert relies on.Test plan
cargo test -p flare-docs -p agentflare-store— all new + existing tests passcargo test --workspace— 746+ tests, 0 failurescargo clippy -p flare-docs -p agentflare-store --all-targets --all-features -- -D warnings -A unsafe_code -A clippy::pedantic(exact CI flags) — cleancargo fmt --check— cleanagent-registry,daemon_autostart.rs,agent_launch.rs) — confirmed viagit stashthat they reproduce identically on unmodified masterSummary by CodeRabbit