feat: Add a search input that searches within virtual elements#1598
Conversation
🦋 Changeset detectedLatest commit: ce1d488 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Review✅ No critical issues found. The implementation is well-structured with:
Minor observations (non-blocking):⚡ Performance consideration → The search performs O(rows × columns) on every query change. For very large datasets (>10k rows), consider adding a max search limit or pagination warning. 📝 useEffect dependency array (line 477 in DBRowTable.tsx) → eslint-disable comment suggests potential dependency issue. The effect depends on tableSearch object properties but disables exhaustive-deps. Consider destructuring only needed properties at the hook call site. Overall: Good to merge. The feature is well-implemented with comprehensive tests and follows React best practices. |
E2E Test Results✅ All tests passed • 59 passed • 4 skipped • 791s
Tests ran across 4 shards in parallel. |
There was a problem hiding this comment.
Pull request overview
This PR adds a custom search input feature that searches through virtualized table rows on the log search page, overriding the browser's native Ctrl/Cmd+F behavior that only searches visible content.
Changes:
- Adds a custom hook to manage table search state, match tracking, and keyboard navigation
- Creates UI components for a floating search input and visual match indicators
- Integrates the search functionality into the DBRowTable component with highlighting and auto-scroll
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/app/src/hooks/useTableSearch.ts |
Custom hook managing search state, debouncing, match indices, and navigation callbacks |
packages/app/src/hooks/__tests__/useTableSearch.test.tsx |
Comprehensive test suite for the useTableSearch hook covering initialization, search, navigation, and edge cases |
packages/app/src/components/DBTable/__tests__/TableSearchInput.test.tsx |
Test suite for TableSearchInput and TableSearchMatchIndicator components including accessibility and keyboard shortcuts |
packages/app/src/components/DBTable/TableSearchInput.tsx |
UI components for search input with keyboard shortcuts, match navigation, and scrollbar indicators |
packages/app/src/components/DBRowTable.tsx |
Integration of search functionality with highlighting, auto-scroll, and state management |
.changeset/eight-bobcats-press.md |
Changeset documenting the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import React from 'react'; | ||
| import { render, screen, waitFor } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; |
There was a problem hiding this comment.
The test file references a renderWithMantine helper function (lines 177, 206, etc.) but it is not imported. This helper is used throughout the tests but is missing from the imports, which would cause test failures.
There was a problem hiding this comment.
renderWithMantine is a global function defined in packages/app/src/setupTests.tsx.
| onClick={() => | ||
| setWrapLinesEnabled(prev => !prev) | ||
| } |
There was a problem hiding this comment.
This formatting change splits the onClick handler across multiple lines unnecessarily. The original single-line format was more concise and equally readable for this simple arrow function.
| onClick={() => | |
| setWrapLinesEnabled(prev => !prev) | |
| } | |
| onClick={() => setWrapLinesEnabled(prev => !prev)} |
There was a problem hiding this comment.
This was done by prettier
|
@dhable Could you please review? I've tried to keep the changes in existing files small, and abstracted most of the functionality into a separate component. The PR is large primarily because of unit tests, which were primarily written by Claude, but I've validated them all. |
PR ReviewCritical IssuesNone found - the implementation looks solid. Important Observations✅ Security: No XSS vulnerabilities - text is properly escaped through React's JSX rendering ✅ Performance: Debouncing (300ms) and case-insensitive search are well-implemented ✅ Accessibility: Good ARIA labels and keyboard navigation (Cmd+F, Enter, Shift+Enter, Escape) ✅ Testing: Comprehensive test coverage with 1927 lines of tests across both components Minor Considerations
SummaryThis is a well-implemented feature with good test coverage, proper security, and accessibility. The only concern is performance on extremely large datasets, but this may be acceptable for the current use case. The eslint-disable comment should be reviewed to ensure it's necessary. Recommendation: ✅ Approve with optional performance optimization for very large datasets. |
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
Add a separate search input triggered by Cmd+F that searches through all matches currently on the page, overriding the native browser search that only searches within visible rows.
Add a separate search input triggered by Cmd+F that searches through all matches currently on the page, overriding the native browser search that only searches within visible rows. Fixes HDX-2687 ### Video https://github.com/user-attachments/assets/beaab78f-e96c-4698-86b7-a4ee3540db1b
Add a separate search input triggered by Cmd+F that searches through all matches currently on the page, overriding the native browser search that only searches within visible rows.
Fixes HDX-2687
Video
CleanShot.2026-01-16.at.13.06.57.mp4