Add global search and quick-action launcher (Ctrl+K)#603
Conversation
- Add /api/search?q= endpoint with board and card results - SearchService queries boards and cards accessible to the user - Add SearchAcrossBoardsAsync to ICardRepository for cross-board text search - Register SearchService in DI container
- Add searchApi client for the /api/search endpoint - Add useGlobalSearch composable with 200ms debounce - Enhance ShellCommandPalette to show search results grouped by type (Commands, Boards, Cards) with keyboard navigation - Wire navigateToBoard and navigateToCard events in AppShell - Add footer with keyboard navigation hints
- 7 tests for useGlobalSearch: debounce, error handling, reset, query clearing - 12 tests for ShellCommandPalette: rendering, search results, keyboard navigation, emit events, group headers, loading indicator, footer - Fix AppShell test for renamed palette option ID prefix
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Self-review finding: the maxResults query parameter was accepted by the controller but not meaningfully forwarded (service uses internal caps). Removed to avoid misleading API surface.
Adversarial Self-ReviewIssues Found and Addressed1. Misleading Issues Reviewed and Accepted2. XSS in search results -- NOT an issue 3. Board search is in-memory filtering 4. Card search case sensitivity 5. Search debouncing 6. 7. No backend unit tests for SearchService Accessibility
Performance
|
There was a problem hiding this comment.
Code Review
This pull request introduces a global search feature that allows users to search for boards and cards directly from the command palette. It includes a new SearchController and SearchService in the backend, along with repository extensions for cross-board searching. On the frontend, a useGlobalSearch composable and updates to the ShellCommandPalette component provide a unified search interface with debouncing and keyboard navigation. Feedback focuses on improving data consistency by making the card description nullable, addressing the ignored maxResults parameter in the search service, and optimizing performance by moving board filtering to the database level. Additionally, minor improvements to the frontend API request path and Vue template keys were suggested.
| Guid ColumnId, | ||
| string ColumnName, | ||
| string Title, | ||
| string Description |
There was a problem hiding this comment.
The Description field in SearchCardHitDto should be nullable (string?) to remain consistent with SearchBoardHitDto and to correctly reflect that cards in Taskdeck can have empty or null descriptions. If Nullable Reference Types (NRT) are enabled, this will also prevent potential null assignment issues during mapping from the database entity.
string? Description| public async Task<Result<GlobalSearchResultDto>> SearchAsync( | ||
| Guid userId, | ||
| string query, | ||
| int maxResults = 20, |
There was a problem hiding this comment.
| var readableBoards = (await _unitOfWork.Boards.GetReadableByUserIdAsync( | ||
| userId, | ||
| includeArchived: false, | ||
| cancellationToken)).ToList(); |
There was a problem hiding this comment.
Fetching all readable boards into memory via ToList() and then filtering them with Where (line 44) is inefficient, especially for users with access to many boards. This logic should be pushed down to the database level (e.g., by adding a search method to the board repository) to reduce memory overhead and improve query performance.
| const params = new URLSearchParams() | ||
| params.append('q', query) | ||
| const { data } = await http.get<GlobalSearchResult>(`/search?${params}`) | ||
| return data |
There was a problem hiding this comment.
Using a leading slash in the request path (/search) can cause some HTTP clients (like Axios) to ignore the baseURL path suffix (e.g., /api) if the client is configured with one. It is generally safer to use a relative path like search?${params} to ensure it appends correctly to the base API path.
| return data | |
| const { data } = await http.get<GlobalSearchResult>(`search?${params}`) |
| > | ||
| <div class="td-command-palette__group"> | ||
| <div class="td-command-palette__group-title">Commands</div> | ||
| <template v-for="(item, index) in allPaletteItems" :key="`${item.type}-${item.type === 'command' ? item.data.id : item.type === 'board' ? item.data.id : item.data.id}`"> |
There was a problem hiding this comment.
The :key expression is unnecessarily complex. Since all types in the PaletteItem union (CommandItem, SearchBoardHit, SearchCardHit) possess an id property, you can simplify the key generation to improve readability.
<template v-for="(item, index) in allPaletteItems" :key="`${item.type}-${item.data.id}`">
Adversarial Code Review -- PR #603 (Global Search Launcher)Overall this is a well-structured feature. The authorization model is correct (scoped via BUG -- Abort controller is created but never wired up (Severity: Medium)File: The composable creates an const result = await searchApi.search(searchQuery.trim())And Repro: Type "tes" (debounce fires request A), then quickly append "ting" (debounce fires request B). If request A resolves after request B, the user sees stale results for "tes" instead of "testing". Fix: Pass PERFORMANCE --
|
| Finding | Severity |
|---|---|
| AbortController never wired to HTTP call (stale results bug) | Medium |
| All boards loaded into memory for search | Medium |
| No backend tests for SearchService | Medium |
| No query length cap | Low-Medium |
| Large IN clause scalability | Low |
| Case sensitivity inconsistency | Low |
| No rate limiting | Low |
| Error state not surfaced in UI | Low |
| Unused maxResults parameter | Nitpick |
The abort controller bug is the most actionable -- it's a real correctness issue that will manifest as stale search results during fast typing. The missing backend tests should also be addressed before merge.
Fixes lint error: variable assigned but never used.
Card descriptions can be null — match SearchBoardHitDto consistency.
The abort signal was created but never passed to the HTTP call, making request cancellation dead code. Stale results from slow earlier requests could overwrite fresh results.
search() now accepts an AbortSignal as second argument.
Placeholder changed from 'Type a command or search...' to 'Type a command or search boards and cards...' as part of the global search feature.
Summary
Closes #93
/api/search?q=endpoint withSearchServicethat queries boards and cards across all user-accessible boards, respecting existing authorization boundariesShellCommandPalette(Ctrl+K / Cmd+K) to include live search results from the backend alongside command navigationsearchApiclient for the new endpointuseGlobalSearchcomposable with 200ms debounce and abort-on-supersedeTest plan
npx vitest --run-- all 1380+ tests passdotnet test backend/Taskdeck.sln -c Release -m:1-- all 1393+ tests pass