vsearch #174
vsearch #174
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds an optional, feature-gated vector search ("vsearch") using FastEmbed and Qdrant; updates dependencies and config; extends AppState and controller for dual-mode search (keyword vs vsearch); and adds UI controls and styling to select and propagate search_type. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Embedding as TextEmbedding
participant Qdrant
Client->>Controller: POST /search (query, search_type=vsearch)
Controller->>Controller: trim & validate query
Controller->>Embedding: embed(query)
Embedding-->>Controller: embedding vector
Controller->>Qdrant: search_points(embedding, limit+offset)
Qdrant-->>Controller: point IDs & scores
Controller->>Controller: collect IDs from payload or point IDs
Controller-->>Client: results + search_type metadata
sequenceDiagram
participant Client
participant Controller
participant Tantivy
participant Qdrant as VSearch
Client->>Controller: POST /search (query, search_type)
Controller->>Controller: trim & validate query
alt search_type == "keyword"
Controller->>Tantivy: search(query, limit, offset)
Tantivy-->>Controller: results
else search_type == "vsearch"
Controller->>Controller: embed(query)
Controller->>VSearch: search_points(embedding, limit+offset)
VSearch-->>Controller: results
end
Controller-->>Client: results + metadata (search_type, timing)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🤖 Fix all issues with AI agents
In `@src/controller.rs`:
- Around line 119-123: The embedding model is being constructed per-request via
TextEmbedding::try_new(...) (with InitOptions and EmbeddingModel::BGESmallZHV15)
which is expensive; instead initialize the TextEmbedding once at startup, add it
to AppState (e.g. an Arc<Mutex<TextEmbedding>> field), create it in main.rs
during bootstrap, and then in controller replace the per-request
TextEmbedding::try_new call with using
state.embedding_model.lock().unwrap().embed(...) (or equivalent locking) to
reuse the single shared model instance.
- Line 124: The code currently calls model.embed(vec![&search], None).unwrap()
and later uses query_vec.into_iter().next().unwrap(), which can panic on embed
failures or empty results; change these to propagate or handle errors instead:
replace the first unwrap by handling the Result from model.embed (using ? to
return an Err from the enclosing function or match/if let to log and return an
error) and replace the second unwrap by checking for Some(value) from
query_vec.into_iter().next() (e.g., match or if let Some(vec) { ... } else {
return Err(...) } ), ensuring the surrounding function signature returns a
Result or otherwise returns a controlled error path; reference model.embed,
query_vec, and the into_iter().next() usage when making these changes.
In `@templates/search.html`:
- Around line 48-51: The export anchor in templates/search.html currently builds
its href with only search and offset and therefore omits the search_type
parameter; update the export link generation (the <a> element with title
"...导出") to include &search_type={{ search_type }} so the exported results
respect the current search mode (keep using the existing variables search,
offset and search_type).
🧹 Nitpick comments (5)
src/config.rs (1)
15-16: Consider adding a serde default for smoother feature adoption.When users enable the
vsearchfeature, deserialization will fail ifqdrant_grpcis missing from their existingconfig.toml. Adding a default value would provide a better experience:♻️ Suggested improvement
#[cfg(feature = "vsearch")] + #[serde(default = "default_qdrant_grpc")] pub qdrant_grpc: String, } + +#[cfg(feature = "vsearch")] +fn default_qdrant_grpc() -> String { + "http://localhost:6334".to_owned() +}src/bin/main.rs (1)
36-39: Consider adding context to the panic message for Qdrant initialization.Using
unwrap()is acceptable for fail-fast behavior at startup (consistent with other initialization code), but adding context viaexpect()would help with debugging configuration issues.💡 Suggested improvement
#[cfg(feature = "vsearch")] let qclient = Qdrant::from_url(CONFIG.qdrant_grpc.as_str()) .build() - .unwrap(); + .expect("Failed to build Qdrant client");src/controller.rs (3)
83-88: Minor: Thecfg!macro evaluates at compile time, so runtime check is partially redundant.When vsearch feature is disabled,
cfg!(feature = "vsearch")isfalse, so the condition short-circuits. This is fine but could be simplified.💡 Alternative for clarity
- let search_type = - if cfg!(feature = "vsearch") && input.search_type.as_deref() == Some("vsearch") { - "vsearch".to_owned() - } else { - "keyword".to_owned() - }; + #[cfg(feature = "vsearch")] + let search_type = match input.search_type.as_deref() { + Some("vsearch") => "vsearch".to_owned(), + _ => "keyword".to_owned(), + }; + #[cfg(not(feature = "vsearch"))] + let search_type = "keyword".to_owned();
141-151: Consider using a more defensive approach for point ID extraction.The nested
unwrap()calls on lines 143-148 could panic if a point has no ID. While this should not happen in practice, defensive coding would prevent crashes.💡 Safer extraction
for point in &search_result.result { - let id = point - .id - .as_ref() - .unwrap() - .point_id_options - .as_ref() - .unwrap(); - if let PointIdOptions::Num(id) = id { - ids.insert(*id as u32); + if let Some(point_id) = &point.id { + if let Some(PointIdOptions::Num(id)) = &point_id.point_id_options { + ids.insert(*id as u32); + } } }
127-127: Remove redundant limit computation inSearchPointsBuilder.
search_limitis computed aslimit + offsetand passed to the constructor, but then immediately overridden by.limit(limit). Passlimitdirectly to the constructor instead and remove the redundant.limit()call.♻️ Suggested simplification
let client = state.qclient; - let search_limit = limit + offset; if let Ok(search_result) = client .search_points( SearchPointsBuilder::new( "cases", query_vec.into_iter().next().unwrap(), - search_limit as u64, + limit as u64, ) .with_payload(false) - .limit(limit as u64) .offset(offset as u64), )
| } else { | ||
| #[cfg(feature = "vsearch")] | ||
| if search_type == "vsearch" { | ||
| { | ||
| let mut model = TextEmbedding::try_new( | ||
| InitOptions::new(EmbeddingModel::BGESmallZHV15) | ||
| .with_show_download_progress(true), | ||
| ) | ||
| .unwrap(); | ||
| let query_vec = model.embed(vec![&search], None).unwrap(); | ||
|
|
||
| let client = state.qclient; | ||
| let search_limit = limit + offset; | ||
| if let Ok(search_result) = client | ||
| .search_points( | ||
| SearchPointsBuilder::new( | ||
| "cases", | ||
| query_vec.into_iter().next().unwrap(), | ||
| search_limit as u64, | ||
| ) | ||
| .with_payload(false) | ||
| .limit(limit as u64) | ||
| .offset(offset as u64), | ||
| ) | ||
| .await | ||
| { | ||
| for point in &search_result.result { | ||
| let id = point | ||
| .id | ||
| .as_ref() | ||
| .unwrap() | ||
| .point_id_options | ||
| .as_ref() | ||
| .unwrap(); | ||
| if let PointIdOptions::Num(id) = id { | ||
| ids.insert(*id as u32); | ||
| } | ||
| } | ||
| } else { | ||
| tracing::error!("Qdrant search_points failed"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
total is never populated for vsearch, breaking pagination display.
For keyword search, total is set on line 98. For vsearch, total remains 0. This will likely confuse users and break pagination UI. Qdrant can return the total count if you need it.
Also, line 117 has a redundant check—search_type == "vsearch" is always true in this branch since we're in the else of search_type == "keyword".
💡 Fix for redundant check
} else {
#[cfg(feature = "vsearch")]
- if search_type == "vsearch" {
{
// vsearch logic...
}
- }
}📝 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.
| } else { | |
| #[cfg(feature = "vsearch")] | |
| if search_type == "vsearch" { | |
| { | |
| let mut model = TextEmbedding::try_new( | |
| InitOptions::new(EmbeddingModel::BGESmallZHV15) | |
| .with_show_download_progress(true), | |
| ) | |
| .unwrap(); | |
| let query_vec = model.embed(vec![&search], None).unwrap(); | |
| let client = state.qclient; | |
| let search_limit = limit + offset; | |
| if let Ok(search_result) = client | |
| .search_points( | |
| SearchPointsBuilder::new( | |
| "cases", | |
| query_vec.into_iter().next().unwrap(), | |
| search_limit as u64, | |
| ) | |
| .with_payload(false) | |
| .limit(limit as u64) | |
| .offset(offset as u64), | |
| ) | |
| .await | |
| { | |
| for point in &search_result.result { | |
| let id = point | |
| .id | |
| .as_ref() | |
| .unwrap() | |
| .point_id_options | |
| .as_ref() | |
| .unwrap(); | |
| if let PointIdOptions::Num(id) = id { | |
| ids.insert(*id as u32); | |
| } | |
| } | |
| } else { | |
| tracing::error!("Qdrant search_points failed"); | |
| } | |
| } | |
| } | |
| } else { | |
| #[cfg(feature = "vsearch")] | |
| { | |
| { | |
| let mut model = TextEmbedding::try_new( | |
| InitOptions::new(EmbeddingModel::BGESmallZHV15) | |
| .with_show_download_progress(true), | |
| ) | |
| .unwrap(); | |
| let query_vec = model.embed(vec![&search], None).unwrap(); | |
| let client = state.qclient; | |
| let search_limit = limit + offset; | |
| if let Ok(search_result) = client | |
| .search_points( | |
| SearchPointsBuilder::new( | |
| "cases", | |
| query_vec.into_iter().next().unwrap(), | |
| search_limit as u64, | |
| ) | |
| .with_payload(false) | |
| .limit(limit as u64) | |
| .offset(offset as u64), | |
| ) | |
| .await | |
| { | |
| for point in &search_result.result { | |
| let id = point | |
| .id | |
| .as_ref() | |
| .unwrap() | |
| .point_id_options | |
| .as_ref() | |
| .unwrap(); | |
| if let PointIdOptions::Num(id) = id { | |
| ids.insert(*id as u32); | |
| } | |
| } | |
| } else { | |
| tracing::error!("Qdrant search_points failed"); | |
| } | |
| } | |
| } |
| let mut model = TextEmbedding::try_new( | ||
| InitOptions::new(EmbeddingModel::BGESmallZHV15) | ||
| .with_show_download_progress(true), | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
Performance: Embedding model is initialized on every request.
TextEmbedding::try_new() loads model weights and is expensive (potentially hundreds of milliseconds or more). This should be initialized once at startup and shared via AppState, similar to how searcher is handled.
🔧 Recommended approach
Initialize the embedding model once at startup in main.rs and add it to AppState:
// In AppState (src/lib.rs):
#[cfg(feature = "vsearch")]
pub embedding_model: Arc<Mutex<TextEmbedding>>,
// In main.rs:
#[cfg(feature = "vsearch")]
let embedding_model = Arc::new(Mutex::new(
TextEmbedding::try_new(
InitOptions::new(EmbeddingModel::BGESmallZHV15)
.with_show_download_progress(true),
).expect("Failed to initialize embedding model")
));Then in the controller, use state.embedding_model.lock().unwrap().embed(...).
🤖 Prompt for AI Agents
In `@src/controller.rs` around lines 119 - 123, The embedding model is being
constructed per-request via TextEmbedding::try_new(...) (with InitOptions and
EmbeddingModel::BGESmallZHV15) which is expensive; instead initialize the
TextEmbedding once at startup, add it to AppState (e.g. an
Arc<Mutex<TextEmbedding>> field), create it in main.rs during bootstrap, and
then in controller replace the per-request TextEmbedding::try_new call with
using state.embedding_model.lock().unwrap().embed(...) (or equivalent locking)
to reuse the single shared model instance.
| .with_show_download_progress(true), | ||
| ) | ||
| .unwrap(); | ||
| let query_vec = model.embed(vec![&search], None).unwrap(); |
There was a problem hiding this comment.
Multiple unwrap() calls can cause panics on malformed data.
Line 124: model.embed(...).unwrap() will panic if embedding fails.
Line 132: query_vec.into_iter().next().unwrap() will panic if the vector is empty.
Consider graceful error handling.
🔧 Suggested fix with error handling
- let query_vec = model.embed(vec![&search], None).unwrap();
+ let query_vec = match model.embed(vec![&search], None) {
+ Ok(v) => v,
+ Err(e) => {
+ tracing::error!("Embedding failed: {e}");
+ return into_response(&SearchPage {
+ search,
+ search_type,
+ offset,
+ cases: vec![],
+ total: 0,
+ enable_vsearch: true,
+ });
+ }
+ };
+
+ let Some(embedding) = query_vec.into_iter().next() else {
+ tracing::error!("Empty embedding result");
+ return into_response(&SearchPage { /* ... */ });
+ };Also applies to: 132-132
🤖 Prompt for AI Agents
In `@src/controller.rs` at line 124, The code currently calls
model.embed(vec![&search], None).unwrap() and later uses
query_vec.into_iter().next().unwrap(), which can panic on embed failures or
empty results; change these to propagate or handle errors instead: replace the
first unwrap by handling the Result from model.embed (using ? to return an Err
from the enclosing function or match/if let to log and return an error) and
replace the second unwrap by checking for Some(value) from
query_vec.into_iter().next() (e.g., match or if let Some(vec) { ... } else {
return Err(...) } ), ensuring the surrounding function signature returns a
Result or otherwise returns a controlled error path; reference model.embed,
query_vec, and the into_iter().next() usage when making these changes.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.