Conversation
- Added search controller with cross-content search functionality - Created search routes - Updated app.js to register search routes - Added text indexes to models for optimized search performance
- Added SearchResults dropdown component - Updated DashboardHeader to implement search functionality - Implemented keyboard navigation and filtering by category
- Created Zustand store for search state - Added debounced search functionality - Implemented result filtering and error handling
- Update SearchResults to pass folder information when navigating to notes - Modify NotesPageLayout to automatically open the correct folder - Ensure selected note is displayed immediately after search - Improve user workflow by reducing clicks needed to access notes This change ensures that when users click on a note in search results, they're taken directly to the note with its containing folder opened.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA global search feature was implemented across the client and server. The client now includes an interactive search field in the dashboard header, a dropdown component for categorized results, a Zustand store for search state, and a debounced search service. The server exposes a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DashboardHeader
participant useSearchStore
participant searchService
participant API_Server
participant searchController
participant DB
User->>DashboardHeader: Types in search field
DashboardHeader->>useSearchStore: search(query)
useSearchStore->>searchService: globalSearch(query, options) (debounced)
searchService->>API_Server: GET /api/search?q=...&type=...
API_Server->>searchController: Handle /api/search
searchController->>DB: Query tasks, notes, habits (text search)
DB-->>searchController: Return results
searchController-->>API_Server: Aggregate and return JSON
API_Server-->>searchService: Respond with results
searchService-->>useSearchStore: Return results
useSearchStore-->>DashboardHeader: Update results
DashboardHeader-->>User: Show categorized dropdown and navigation
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
client/package.json (1)
59-61: Consider leveraging existing lodash import instead of adding a new package
Sincelodashis already a dependency, you can importdebounceviaimport debounce from 'lodash/debounce'which will tree-shake unused code instead of addinglodash.debounce. This reduces dependency surface and simplifies version management.server/models/taskModel.js (1)
54-57: Add background indexing to avoid write blocks
Creating text and compound indexes can lock the collection. Consider adding{ background: true }to both indexes so they build without blocking writes in production.server/models/habitModel.js (1)
70-73: Background index creation recommended
For the new text and user/updateTime indexes, specify{ background: true }inhabitSchema.index(...)to prevent collection locks during index builds.server/models/noteModel.js (1)
52-56: Use background indexing and consider index weights
Add{ background: true }to each new index, and if certain fields (e.g.,titleovercontent) are more important in search results, consider specifyingweightsin your text index.client/src/services/searchService.js (1)
23-23: Consider response data consistency.The function returns the full
responseobject instead ofresponse.data. This might be inconsistent with other service functions that typically return just the data payload. Verify if consumers expect the full response or just the data.If consistency with other services is desired:
- return response; + return response.data;client/src/components/SearchResults.jsx (1)
86-86: Consider adding max-width for better mobile responsiveness.The dropdown uses full width which might be too wide on larger screens.
- className="absolute top-full left-0 right-0 mt-1 bg-background border border-border rounded-md shadow-md max-h-[70vh] overflow-y-auto z-50 p-2" + className="absolute top-full left-0 right-0 mt-1 bg-background border border-border rounded-md shadow-md max-h-[70vh] max-w-2xl overflow-y-auto z-50 p-2"server/controllers/searchController.js (1)
82-84: Improve HTML tag removal regex for better security.The current regex for removing HTML tags is basic and might miss some edge cases or malformed HTML.
- const plainTextContent = note.content - ? note.content.replace(/<[^>]*>?/gm, "") // Remove HTML tags - : ""; + const plainTextContent = note.content + ? note.content.replace(/<[^>]*>/g, "").replace(/&[^;]+;/g, " ") // Remove HTML tags and entities + : "";Or consider using a proper HTML sanitization library like
striptagsfor more robust HTML removal.client/src/stores/useSearchStore.js (2)
7-30: Consider cleanup for debounced function on store destruction.The debounced function doesn't have cleanup which could lead to memory leaks if the store is destroyed while pending searches exist.
Consider adding cleanup logic:
export const useSearchStore = create((set, get) => { // Create a debounced search function const debouncedSearchFn = debounce(async (searchQuery, selectedCategory) => { if (!searchQuery.trim()) { set({ results: null, isOpen: false, isSearching: false }); return; } try { const results = await globalSearch(searchQuery, { type: selectedCategory !== "all" ? selectedCategory : "all", }); set({ results, isSearching: false, isOpen: true, }); } catch (error) { console.error("Search failed:", error); set({ isSearching: false, error: "Failed to perform search. Please try again.", }); } }, 300); + // Add cleanup method to the store + const cleanup = () => { + debouncedSearchFn.cancel(); + };
23-29: Consider preserving isOpen state on search error.Currently, search errors don't explicitly set
isOpen: false, which might leave the dropdown open showing an error. This could be the intended behavior, but consider if it should close on error.If errors should close the dropdown:
} catch (error) { console.error("Search failed:", error); set({ isSearching: false, + isOpen: false, error: "Failed to perform search. Please try again.", }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
client/package.json(1 hunks)client/src/components/DashboardHeader.jsx(1 hunks)client/src/components/SearchResults.jsx(1 hunks)client/src/features/Notes/components/layout/NotesPageLayout.jsx(2 hunks)client/src/services/searchService.js(1 hunks)client/src/stores/useSearchStore.js(1 hunks)server/app.js(2 hunks)server/controllers/searchController.js(1 hunks)server/models/habitModel.js(1 hunks)server/models/noteModel.js(1 hunks)server/models/taskModel.js(1 hunks)server/routes/searchRoutes.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
client/src/features/Notes/components/layout/NotesPageLayout.jsx (3)
client/src/features/Notes/hooks/useNoteQueries.js (2)
useNoteQuery(75-87)useNoteQuery(75-87)client/src/features/Notes/pages/NotesPage.jsx (1)
location(18-18)client/src/features/Notes/components/dashboard/NotesDashboardPanel.jsx (1)
folders(57-57)
client/src/services/searchService.js (1)
client/src/services/api/apiClient.js (1)
apiClient(5-12)
server/controllers/searchController.js (1)
server/utils/asyncHandler.js (1)
asyncHandler(8-12)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Backend
🔇 Additional comments (15)
server/app.js (1)
16-16: Ensure search endpoint is secured and documented
You’ve correctly wiredsearchRoutesunder/api/search, but please verify that the route applies the same authentication/middleware as other user-scoped endpoints. Also confirm that Swagger docs include the new/api/searchoperations.Also applies to: 141-142
client/src/features/Notes/components/layout/NotesPageLayout.jsx (2)
59-67: LGTM! Clean folder navigation logic.The folder selection logic properly checks if the folder exists in the fetched folders list before setting it as current, preventing invalid folder states.
39-39: LGTM! Clear documentation of search integration.The updated comments accurately reflect that navigation state can now come from both dashboard and search functionality.
Also applies to: 69-69
client/src/services/searchService.js (2)
3-10: LGTM! Excellent JSDoc documentation.The function documentation is comprehensive and clearly describes parameters, options, and return types.
24-27: LGTM! Proper error handling.Error logging and re-throwing allows for appropriate error handling at the consumer level.
client/src/components/DashboardHeader.jsx (3)
15-31: LGTM! Comprehensive keyboard navigation.The keyboard event handling properly supports arrow key navigation, escape to blur, and prevents default behaviors appropriately. The logic integrates well with the search store.
39-39: LGTM! Proper positioning for dropdown.Adding
relativepositioning to the container is necessary for the SearchResults dropdown to position correctly.
42-48: LGTM! Clean search integration.The input ref and event handlers are properly connected to the search store functionality.
server/routes/searchRoutes.js (3)
7-62: LGTM! Excellent Swagger documentation.The API documentation is comprehensive, covering all parameters, response schemas, and error cases. This will be valuable for API consumers and maintainers.
63-63: LGTM! Proper authentication and routing.The route correctly applies the
protectmiddleware for authentication and delegates to the search controller.
1-5: LGTM! Clean module structure.Standard Express router setup with appropriate imports and controller delegation.
client/src/components/SearchResults.jsx (2)
26-36: LGTM! Good click-outside implementation.The click-outside handler is properly implemented with cleanup in the useEffect dependency array.
80-81:⚠️ Potential issueFix currentIndex mutation during render.
The
currentIndexvariable is being mutated during the render phase, which violates React's principle of pure render functions and can lead to inconsistent behavior.Consider computing the index ranges upfront or using a more functional approach:
- // Calculate current index for keyboard navigation - let currentIndex = -1; + // Calculate item indices for keyboard navigation + const getItemIndex = () => { + const categories = []; + if (selectedCategory === "all" || selectedCategory === "tasks") { + categories.push({ type: 'tasks', items: results?.tasks || [] }); + } + if (selectedCategory === "all" || selectedCategory === "notes") { + categories.push({ type: 'notes', items: results?.notes || [] }); + } + if (selectedCategory === "all" || selectedCategory === "habits") { + categories.push({ type: 'habits', items: results?.habits || [] }); + } + + let currentIndex = -1; + return () => ++currentIndex; + }; + + const getNextIndex = getItemIndex();Likely an incorrect or invalid review comment.
server/controllers/searchController.js (1)
57-70: LGTM! Good parallel execution implementation.The parallel search execution using Promise.all is well implemented and efficient.
client/src/stores/useSearchStore.js (1)
67-91: LGTM! Well-implemented keyboard navigation logic.The navigation logic correctly handles cycling through results and respects category filtering. The modulo arithmetic ensures smooth wrapping between first and last items.
client/src/features/Notes/components/layout/NotesPageLayout.jsx
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replace mutable index counter with functional index generator in SearchResults component. This change addresses CodeRabbit AI feedback by: - Eliminating direct mutations during render - Using a closure-based approach for sequential index generation - Making the code more predictable and maintainable - Better aligning with React's functional paradigm The functionality remains identical, but the implementation is now more robust.
…ry call as per coderabbit review The change removes a redundant enabled condition that was passed as a second argument to the useNoteQuery hook in NotesPageLayout.jsx. The hook implementation already includes the proper enabled condition internally (enabled: !!id && !!userId), making the additional condition unnecessary.
…dexes - as per coderabbit suggestion Replace case-insensitive regex searches with MongoDB's native text search capabilities to improve search performance and relevance. This change: - Utilizes existing text indexes in the models - Adds relevance scoring to sort results by match quality - Improves performance for large datasets - Simplifies search condition code with a unified baseCondition
…as per coderabbit suggestion Add validation for search API parameters to improve security and prevent misuse: - Limit the maximum number of results to 100 - Validate the 'type' parameter against a list of allowed values
…oderabbit suggestion Reset the keyboard navigation index when starting a new search to ensure consistent user experience. This prevents highlighting stale results from previous searches and ensures that keyboard navigation always starts fresh with each new query, avoiding potential out-of-bounds errors when search results length changes.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Fixed issue where focused items would jump or focus incorrectly due to unstable indexes generated on each render. - Implemented stable indexing with a memoized flattened results array. - Added Enter key support for navigating to focused items. - Addressed from Cursor BugBot bug report.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1 participant
closes #33
Summary by CodeRabbit
New Features
Performance Improvements
Bug Fixes
Chores