Conversation
Reviewer's Guide by SourceryThis pull request refactors the search functionality to support more comprehensive search results with separate items and summaries. It modifies the search result structure to include separate items and summaries, improves search result handling in the command palette and search handlers, and removes duplicate filtering logic in search results. The Rust and Flutter code are updated to support the new search result structure, and search-related data types and interfaces are modified. Sequence diagram for performing a searchsequenceDiagram
participant User
participant CommandPaletteBloc
participant SearchBackendService
participant SearchManager
participant DocumentSearchHandler
participant FolderSearchHandler
participant CloudService
participant FolderIndexManager
User->>CommandPaletteBloc: Enters search query
CommandPaletteBloc->>SearchBackendService: performSearch(query, workspaceId)
SearchBackendService->>SearchManager: performSearch(query, streamPort, filter, searchId)
SearchManager->>DocumentSearchHandler: perform_search(query, filter)
SearchManager->>FolderSearchHandler: perform_search(query, filter)
DocumentSearchHandler->>CloudService: document_search(workspaceId, query)
activate CloudService
CloudService-->>DocumentSearchHandler: SearchDocumentResponseItem[]
deactivate CloudService
DocumentSearchHandler-->>SearchManager: SearchResultPB[] (via stream)
FolderSearchHandler->>FolderIndexManager: search(query)
activate FolderIndexManager
FolderIndexManager-->>FolderSearchHandler: SearchResponseItemPB[]
deactivate FolderIndexManager
FolderSearchHandler-->>SearchManager: SearchResultPB[] (via stream)
SearchManager-->>SearchBackendService: SearchResponsePB (via stream)
SearchBackendService-->>CommandPaletteBloc: SearchResponsePB (via stream)
CommandPaletteBloc->>User: Update UI with search results
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🥷 Ninja i18n – 🛎️ Translations need to be updatedProject
|
| lint rule | new reports | level | link |
|---|---|---|---|
| Missing translation | 87 | warning | contribute (via Fink 🐦) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @appflowy - I've reviewed your changes - here's some feedback:
Overall Comments:
- The changes introduce a new
TantivyStatestruct; consider using dependency injection to avoid tight coupling with Tantivy. - The
perform_searchfunction now returns aStream; ensure that errors during streaming are properly handled and propagated to the UI. - The Flutter code now uses
SearchResponseItemPBandSearchSummaryPB; ensure that the UI is updated to handle these new data structures correctly.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| use lib_infra::async_trait::async_trait; | ||
| use tokio::sync::broadcast; | ||
| use lib_infra::isolate_stream::{IsolateSink, SinkExt}; |
There was a problem hiding this comment.
suggestion (performance): Stream using IsolateSink requires resource cleanup.
Verify that all created IsolateSink instances (and their underlying RawReceivePorts) are eventually disposed to avoid resource leaks, especially when searches are cancelled or new ones initiated.
Suggested implementation:
pub struct Manager {
// existing fields...
// Track active IsolateSink resources keyed by search type.
active_isolate_sinks: HashMap<SearchType, IsolateSink>,
}impl Manager {
// existing methods...
// Call this method when a search is cancelled or replaced.
// It disposes the associated IsolateSink and logs any error.
pub async fn cleanup_sink(&mut self, search_type: &SearchType) {
if let Some(mut sink) = self.active_isolate_sinks.remove(search_type) {
if let Err(e) = sink.close().await {
error!("Error closing IsolateSink for {:?}: {:?}", search_type, e);
}
}
}
}• In the parts of the code that create or restart a search (or when searches are cancelled), ensure you call Manager::cleanup_sink with the appropriate SearchType before starting a new search so that the old IsolateSink (and its underlying RawReceivePort) is disposed to avoid resource leaks.
• Adjust the type of IsolateSink if it takes type parameters and make sure that SinkExt::close() (or the equivalent cleanup method) is available. If not, implement further cleanup either by calling drop or the proper release method.
• Verify that the Manager struct is instantiated accordingly and that cleanup_sink is awaited since it is async.
| filter: Option<SearchFilterPB>, | ||
| channel: Option<String>, | ||
| search_id: String, | ||
| ) { |
There was a problem hiding this comment.
issue (complexity): Consider using stream combinators like futures::stream::select_all and take_while to compose search streams and filter for current search cancellation, which simplifies the asynchronous logic and reduces nesting in the perform_search method, making it more readable and maintainable by avoiding manual task management and nesting..
The new implementation spawns multiple tasks and manually joins their outputs which increases nesting and control flow complexity. Consider composing the individual search streams into a single stream using combinators like futures::stream::select_all and filtering for current search cancellation with take_while. This would keep the async logic linear and easier to follow.
For example:
use futures::{stream, StreamExt};
let search_streams = handlers.into_iter().map(|(_, handler)| {
handler.perform_search(query.clone(), filter.clone())
}).collect::<Vec<_>>();
let merged = stream::select_all(search_streams)
.take_while(|_| async { is_current_search(¤t_search, &search_id).await });
merged.for_each_concurrent(None, |result| async {
if let Ok(result) = result {
let resp = SearchResponsePB {
result: Some(result),
search_id: search_id.clone(),
is_loading: true,
};
if let Ok::<Vec<u8>, _>(data) = resp.try_into() {
if let Err(err) = sink.send(data).await {
error!("Failed to send search result: {}", err);
}
}
}
}).await;These changes maintain the existing functionality while reducing manual task management and nesting in the perform_search method.
appflowy
force-pushed
the
search_summary
branch
from
79240c0 to
5abf0c3
Compare
appflowy
force-pushed
the
search_summary
branch
from
5abf0c3 to
4997ac9
Compare
appflowy
force-pushed
the
search_summary
branch
from
3f6466b to
4d17276
Compare
1 participant
Summary by Sourcery
Refactor and enhance the search functionality in AppFlowy, introducing a new streaming-based search approach with support for AI-generated summaries and improved search result handling.
New Features:
Enhancements:
Chores: