Merged
Conversation
Contributor
Reviewer's GuideThe pull request introduces a feature to provide users with suggested questions when their initial query against selected documents (RAG sources) doesn't yield a direct answer. This is implemented by:
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @appflowy - I've reviewed your changes - here's some feedback:
- The PR introduces two structs named
RelatedQuestionChainin different modules (context_question_chain.rsandrelated_question_chain.rs) with distinct functionalities; consider renaming one for better clarity (e.g.,ContextualQuestionChainvs.GeneralFollowUpQuestionChain). - The use of
tokio::time::sleepwithin theConversationalRetrieverChain::streammethod for suggested questions might introduce artificial delays; evaluate if this is the intended behavior or if an alternative streaming approach for UI effects is feasible. - The
serde_json::from_valueforQuestionStreamValueinllm_chat.rsfalls back to an empty answer on parse failure; consider if a more explicit error handling or logging strategy for deserialization errors would be more robust.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟢 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.
| @@ -86,13 +107,44 @@ impl ConversationalRetrieverChain { | |||
|
|
|||
| if documents.is_empty() { | |||
Contributor
There was a problem hiding this comment.
suggestion (performance): Review thread sleeping delays in streaming suggested questions.
Are these tokio::time::sleep delays necessary? They introduce pauses that can cause unpredictable latency.
Comment on lines
+69
to
71
| if let Ok(mut w) = self.user_tmpl.try_write() { | ||
| *w = template; | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Evaluate lock acquisition strategy in set_rag_ids.
try_write() can fail silently and skip the update if the lock isn’t available. If updating the prompt template is critical, use a blocking write() instead.
Suggested change
| if let Ok(mut w) = self.user_tmpl.try_write() { | |
| *w = template; | |
| } | |
| let mut w = self.user_tmpl.write().expect("User template RwLock poisoned"); | |
| *w = template; |
| metadata: Option<serde_json::Value>, | ||
| ) -> Result<ChatMessage, FlowyError> { | ||
| let mut message = ChatMessage::new_ai(timestamp(), message.to_string(), Some(question_id)); | ||
| let message_id = ID_GEN.lock().await.next_id(); |
Contributor
There was a problem hiding this comment.
question: Examine the MessageIDGenerator logic.
Ensure message IDs remain monotonic even if the system clock moves backward.
| dbg!(&result); | ||
| dbg!(&result.sources); | ||
|
|
||
| assert!(!result.answer.is_empty()); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Assert the actual format of the AI's response, not just non-emptiness.
In local_ollama_test_chat_format, add an assertion to verify bullet formatting (e.g., assert!(result.answer.contains("* ")) or assert!(result.answer.contains("- "))) instead of only checking non-emptiness.
Suggested implementation:
assert!(!result.answer.is_empty());
assert!(result.answer.contains("* ") || result.answer.contains("- "));This change adds an assertion to ensure that the answer contains either a bullet formatted with "* " or "- " as requested. Make sure this additional assertion aligns with your expectations for the AI's response formatting.
| @@ -16,6 +18,33 @@ async fn local_ollama_test_simple_question() { | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Add test for behavior when context-specific question generation is unavailable or fails.
Add a test in local_ollama_test_simple_question that builds a ConversationalRetrieverChain without a store (making context_question_chain None) and simulates an error from generate_questions. Verify it falls back to CAN_NOT_ANSWER_WITH_CONTEXT when RAG IDs exist but no documents or questions are generated.
Suggested implementation:
// set rag_id but not rag document content, should return CAN_NOT_ANSWER_WITH_CONTEXT
chat.set_rag_ids(vec![doc_id.to_string()]);
// simulate missing context-specific question generation by removing the store from the chain
chat.context_question_chain = None;
let stream = chat
.stream_question("hello world", Default::default())
.await;
let result_fallback = collect_stream(stream).await;
assert!(result_fallback.iter().any(|msg| msg.contains("CAN_NOT_ANSWER_WITH_CONTEXT")),
"Expected fallback message not found when context-specific question generation fails");Make sure that:
- The chat object has a mutable field named context_question_chain.
- The fallback message "CAN_NOT_ANSWER_WITH_CONTEXT" is exactly what the code returns when generate_questions fails.
- If necessary, adjust the test to match your project's conventions for asynchronous tests and error handling.
| use uuid::Uuid; | ||
|
|
||
| #[tokio::test] | ||
| async fn local_ollama_test_context_related_questions() { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Test context-specific suggested questions with multiple RAG documents.
Add a case with multiple RAG docs (e.g., doc_id1/doc_id2). After triggering a fallback with an irrelevant question, assert that suggested questions can come from either doc and that each object_id matches its source, to better test ContextQuestionChain’s filtering.
Suggested implementation:
let doc_id = Uuid::new_v4().to_string();
let trip_docs = load_asset_content("japan_trip.md");
chat.set_rag_ids(vec![doc_id.clone()]);
// Additional test for multiple RAG documents
let doc_id1 = Uuid::new_v4().to_string();
let doc_id2 = Uuid::new_v4().to_string();
chat.set_rag_ids(vec![doc_id1.clone(), doc_id2.clone()]);
// Trigger fallback with an irrelevant question to test context-specific suggestions
let fallback_stream = chat
.stream_question("irrelevant question triggering fallback", Default::default())
.await
.unwrap();
let fallback_result = collect_stream(fallback_stream).await;
assert!(!fallback_result.answer.is_empty());
// Assert that suggested questions come from either of the specified RAG documents
for suggestion in fallback_result.suggested_questions.iter() {
assert!(
suggestion.object_id == doc_id1 || suggestion.object_id == doc_id2,
"Suggested question's object_id does not match any of the RAG document IDs"
);
}Ensure that the fallback_result structure has a suggested_questions field and that each suggestion has an object_id field. Adjust field names if necessary to match your codebase's definitions.
| .await | ||
| .unwrap(); | ||
| let result = collect_stream(stream).await; | ||
| dbg!(&result.suggested_questions); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Test behavior if ContextQuestionChain generates no valid questions after filtering.
Add a test that when ContextQuestionChain::generate_questions returns no valid questions (empty or all filtered out), the code falls back to CAN_NOT_ANSWER_WITH_CONTEXT and sets result.gen_related_question to true. You can mock the LLM response or configure context/RAG IDs to trigger this case.
Suggested implementation:
#[tokio::test]
async fn test_no_valid_questions_fallback() {
// Obtain a chat instance from the test harness. Adjust this to your own method of creating the instance.
let chat = get_chat_instance(); // Replace or modify if your instance initializer is different.
let doc_id = "test-doc-id";
// Embed paragraphs (simulate a valid document with paragraphs)
chat.embed_paragraphs(&doc_id, vec!["trip_docs_placeholder".to_string()])
.await
.unwrap();
// Trigger the fallback scenario by providing a question that causes ContextQuestionChain::generate_questions
// to return no valid questions. This prompt should be adjusted to your specific test configuration.
let stream = chat
.stream_question("Trigger fallback: no valid questions", Default::default())
.await
.unwrap();
let result = collect_stream(stream).await;
dbg!(&result.suggested_questions);
// Assert that the fallback behavior is triggered:
// 1. A default fallback question "CAN_NOT_ANSWER_WITH_CONTEXT" is returned.
// 2. gen_related_question is set to true.
assert_eq!(result.suggested_questions.len(), 1, "Should provide one fallback question");
assert_eq!(result.suggested_questions[0].content, "CAN_NOT_ANSWER_WITH_CONTEXT");
assert!(result.gen_related_question, "gen_related_question should be true when fallback is triggered");
// Optionally, verify that the suggested question's object_id matches the given doc_id.
assert_eq!(result.suggested_questions[0].object_id, doc_id);
}Depending on your codebase, you may need to ensure that:
- The get_chat_instance() method is correctly defined or replaced with your actual chat initialization.
- The input prompt "Trigger fallback: no valid questions" or other parameters are appropriately set to simulate the fallback scenario.
- The expected fallback string "CAN_NOT_ANSWER_WITH_CONTEXT" matches exactly what is returned by your implementation.
Adjust as necessary to reflect your project specifics.
appflowy
force-pushed
the
vault_workspace_suggested_question
branch
from
8614970 to
1d4aaa1
Compare
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
2 participants
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.
Feature Preview
PR Checklist
Summary by Sourcery
Enhance the AI chat functionality by adding context-aware suggested questions and improving the conversational retrieval chain in the AppFlowy local AI system
New Features:
Enhancements:
Tests: