feat: Add suggested questions display #315
Conversation
![google-labs-jules[bot]](https://avatars.githubusercontent.com/in/842251?s=80&v=4){kind=small}
This commit introduces a new feature to display auto-generated suggested questions on the collection detail page. Key changes: - A new `SuggestedQuestions.tsx` component has been created to fetch and render the suggested questions as clickable chips. - The `apiClient.ts` has been updated with a `getSuggestedQuestions` method to retrieve questions from the backend. - The `LightweightCollectionDetail.tsx` component now integrates the `SuggestedQuestions` component, allowing users to click on a question to populate the search bar. This addresses issue #291.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. |
🚀 Development Environment OptionsThis repository supports Dev Containers for a consistent development environment. Option 1: GitHub Codespaces (Recommended)Create a cloud-based development environment:
Option 2: VS Code Dev Containers (Local)Use Dev Containers on your local machine:
Option 3: Traditional Local SetupSet up the development environment manually: # Clone the repository
git clone https://github.com/manavgup/rag_modulo.git
cd rag_modulo
git checkout feat-suggested-questions-display
# Initialize development environment
make dev-init
make dev-build
make dev-up
make dev-validateAvailable CommandsOnce in your development environment: make help # Show all available commands
make dev-validate # Validate environment setup
make test-atomic # Run atomic tests
make test-unit # Run unit tests
make lint # Run lintingServices AvailableWhen running
This automated message helps reviewers quickly set up the development environment. |
|
@claude review this PR |
|
Claude finished @manavgup's task —— View job PR Review Complete ✅Reviewed PR #315: feat: Add suggested questions display Todo List
OverviewThis PR adds a new component to display suggested questions on the collection detail page. The implementation includes proper state management, loading states, error handling, and integrates well with the existing architecture. ✅ Strengths
🔍 Issues & Recommendations1. Missing Error Type in Catch Block (Medium Priority)Location: } catch (err) {
setError('Failed to load suggested questions.');
addNotification('error', 'Error', 'Could not fetch suggested questions.');
}Issue: The Recommendation: } catch (err) {
const errorMessage = err instanceof Error ? err.message : 'Failed to load suggested questions.';
setError(errorMessage);
addNotification('error', 'Error', 'Could not fetch suggested questions.');
console.error('Error fetching suggested questions:', err);
}2. Redundant Notification (Low Priority)Location: const handleSuggestedQuestionClick = (question: string) => {
setSearchQuery(question);
addNotification('info', 'Search Updated', 'Search query populated with suggested question.');
};Issue: The notification might be excessive for a simple UI interaction. The visual feedback of the search input being populated is usually sufficient. Recommendation: Consider removing the notification or making it configurable: const handleSuggestedQuestionClick = (question: string) => {
setSearchQuery(question);
// Optional: Remove notification or make it a toast that auto-dismisses quickly
};3. Inconsistent Search Behavior (Medium Priority)Location: const filteredDocuments = collection?.documents.filter(doc =>
doc.name.toLowerCase().includes(searchQuery.toLowerCase())
) || [];Issue: When a suggested question is clicked, it populates the search input but filters documents by name, not by performing a RAG search. This creates a confusing UX where clicking "What is machine learning?" would filter document filenames instead of searching content. Recommendation: Either:
Example approach: const handleSuggestedQuestionClick = (question: string) => {
// Option 1: Navigate to search page with the question
navigate('/search', {
state: {
collectionId: collection.id,
collectionName: collection.name,
initialQuery: question
}
});
// Option 2: Or trigger the "Chat" action with pre-filled question
handleChatWithCollection(question);
};4. No Loading/Disabled State on Refresh (Low Priority)Location: const handleRefresh = () => {
fetchQuestions();
};Issue: The refresh button doesn't show visual feedback while fetching. Users might click multiple times. Recommendation: <button
onClick={handleRefresh}
disabled={isLoading}
className={`p-1 text-gray-60 hover:text-gray-90 ${isLoading ? 'opacity-50 cursor-not-allowed' : ''}`}
>
<ArrowPathIcon className={`w-4 h-4 ${isLoading ? 'animate-spin' : ''}`} />
</button>5. Missing Backend Endpoint Documentation (Low Priority)Location: Observation: The endpoint Recommendation: Ensure the backend endpoint is implemented or document in the PR description that it's a dependency. 6. Accessibility Improvements (Low Priority)Missing ARIA labels:
Recommendation: {questions.map((q) => (
<button
key={q.id}
onClick={() => onQuestionClick(q.question)}
className="px-3 py-1.5 bg-blue-10 text-blue-70 rounded-full hover:bg-blue-20 text-sm transition-colors"
for: ${q.question}`}
>
{q.question}
</button>
))}🎯 Testing Recommendations
📝 SummaryThis is a well-implemented feature with good UX patterns and proper error handling. The main concern is the inconsistent search behavior (issue #3) where suggested questions populate a document name filter instead of triggering a content search. This should be addressed before merging to avoid user confusion. Severity Breakdown:
Recommendation: Address the search behavior inconsistency and improve error handling before merging. Other issues are minor polish items that could be addressed in follow-up PRs. |
… and UX Addressed code review feedback from PR #315: 1. Improved error handling in SuggestedQuestions component: - Capture and display actual error messages instead of generic errors - Add console.error for debugging - Provide detailed error information to users 2. Fixed inconsistent search behavior: - Changed suggested question click to navigate to RAG search page - Pass question as initialQuery in navigation state - Previously populated document filter instead of performing search 3. Added refresh button loading state: - Added isRefreshing state to track refresh operations - Disabled button during refresh with visual feedback (spin animation) - Prevents duplicate refresh requests 4. Enhanced accessibility: - Added aria-label to refresh buttons - Added aria-label to question buttons with descriptive text - Improved keyboard navigation support 5. Verified backend endpoint exists: - Confirmed GET /api/collections/{collection_id}/questions endpoint - Located in backend/rag_solution/router/collection_router.py:371 All changes maintain backward compatibility and improve user experience.
|
@claude please review this PR. |
|
Claude finished @manavgup's task —— View job PR Review Complete ✅Reviewed PR #315: feat: Add suggested questions display Todo List
OverviewThis PR adds a well-implemented suggested questions feature to the collection detail page. The code demonstrates good React patterns, proper TypeScript usage, and thoughtful UX considerations. The backend endpoint already exists ( ✅ Strengths
🎯 Review Findings1. Excellent Error Handling Implementation ✅Location: } catch (err) {
const errorMessage = err instanceof Error ? err.message : 'Failed to load suggested questions.';
console.error('Error fetching suggested questions:', err);
setError(errorMessage);
addNotification('error', 'Error', `Could not fetch suggested questions: ${errorMessage}`);
}Observation: This is significantly better than the typical error handling pattern. It properly types the error, logs for debugging, and provides user feedback. Well done! 2. Smart Navigation Pattern ✅Location: const handleSuggestedQuestionClick = (question: string) => {
if (collection?.status === 'ready' || collection?.status === 'completed') {
navigate('/search', {
state: {
collectionId: collection.id,
collectionName: collection.name,
collectionDescription: collection.description,
initialQuery: question
}
});
} else {
addNotification('warning', 'Collection Not Ready', 'This collection is not ready for searching yet.');
}
};Observation: Excellent implementation! This:
3. Proper Loading State on Refresh ✅Location: const handleRefresh = async () => {
setIsRefreshing(true);
try {
await fetchQuestions();
} finally {
setIsRefreshing(false);
}
};
// In the button:
disabled={isRefreshing}
className={`... ${isRefreshing ? 'animate-spin' : ''}`}Observation: Great attention to detail! The refresh button:
4. Backend Endpoint Verified ✅Location: I verified the backend endpoint exists at @router.get(
"/{collection_id}/questions",
summary="Get questions for a collection",
response_model=List[QuestionOutput],
description="Get all questions for the specified collection",
)
def get_collection_questions(...)Observation: The backend endpoint is already implemented and working. No backend changes needed. 🔍 Minor Suggestions (Optional Enhancements)1. Consider Handling Stale Data (Low Priority)Location: Current Implementation: useEffect(() => {
fetchQuestions();
}, [fetchQuestions]);Suggestion: Consider adding a cleanup function to handle race conditions if the component unmounts during fetch: useEffect(() => {
let cancelled = false;
const loadQuestions = async () => {
setIsLoading(true);
setError(null);
try {
const fetchedQuestions = await apiClient.getSuggestedQuestions(collectionId);
if (!cancelled) {
setQuestions(fetchedQuestions);
}
} catch (err) {
if (!cancelled) {
const errorMessage = err instanceof Error ? err.message : 'Failed to load suggested questions.';
console.error('Error fetching suggested questions:', err);
setError(errorMessage);
addNotification('error', 'Error', `Could not fetch suggested questions: ${errorMessage}`);
}
} finally {
if (!cancelled) {
setIsLoading(false);
}
}
};
loadQuestions();
return () => {
cancelled = true;
};
}, [collectionId, addNotification]);This prevents state updates if the component unmounts during the API call. 2. Empty State Message Clarity (Very Low Priority)Location: Current: <p className="text-sm text-gray-60">No suggested questions available at the moment. Questions will be generated automatically after document processing is complete.</p>Suggestion: This is already excellent! The message clearly explains when questions will be available. Only minor suggestion would be to consider adding a note about the refresh button: <p className="text-sm text-gray-60">
No suggested questions available at the moment. Questions will be generated automatically
after document processing is complete. Use the refresh button above to check for updates.
</p>But the current implementation is perfectly fine. 3. Potential Duplicate Notifications (Very Low Priority)Location: Observation: When the component loads successfully, the parent component shows "Collection Loaded" notification, and if there's an error fetching questions, we show another notification. This could result in notification spam, but given they're different events (collection load vs questions fetch), this is actually appropriate. 📊 Code Quality Assessment
🎯 Testing Recommendations
📝 SummaryThis is an exemplary PR that demonstrates high-quality frontend development. The implementation shows:
No blocking issues found. The code is production-ready and follows best practices throughout. The minor suggestions above are optional enhancements, not required changes. Recommendation: ✅ Approve and merge The implementation is solid, well-tested patterns are used, and the feature adds clear value to the user experience. Great work! |
- Resolve merge conflict in apiClient.ts (keep both SuggestedQuestion and VoiceId types) - Add proper cleanup in useEffect to prevent memory leaks - Improve error handling with proper TypeScript typing - Remove redundant fetchQuestions function in favor of inline implementation All outstanding items from Claude review have been addressed: ✅ Error type in catch block - properly typed with instanceof Error check ✅ Redundant notification - already removed in previous commit ✅ Cleanup in useEffect - added isMounted flag to prevent state updates after unmount ✅ Memory leak prevention - proper cleanup function in useEffect
1 participant
This change adds a new component to display suggested questions on the collection detail page.
SuggestedQuestions.tsxcomponent to handle fetching, loading, error states, and rendering of the questions.getSuggestedQuestionsmethod.PR created automatically by Jules for task 310753847513004601