feat: Implemented log search functionality with highlighted results #647
Conversation
WalkthroughThe changes introduce a comprehensive search and navigation feature to the system logs interface. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LogSearch
participant SystemLogs
participant LogEntry (PlainLog/FormattedLog)
User->>LogSearch: Types search query / uses navigation controls
LogSearch->>SystemLogs: onSearch(query) / onNavigate(direction)
SystemLogs->>SystemLogs: Update search state, find matches, update highlights
SystemLogs->>LogEntry: Pass searchQuery prop
LogEntry->>LogEntry: Highlight matching text
SystemLogs->>SystemLogs: Scroll to highlighted match (if navigating)
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
web-server/src/components/Service/SystemLog/FormattedLog.tsx (1)
13-30: 🛠️ Refactor suggestionFactor‑out
HighlightedTextto avoid code duplicationSee earlier comment in
PlainLog.tsx.
🧹 Nitpick comments (4)
web-server/src/components/Service/SystemLog/PlainLog.tsx (1)
3-7: Unused prop:index
indexis accepted inPlainLogPropsbut never used in the component. Either remove it from the prop list or use it (e.g. askeywhen mapping). Keeping unused props increases cognitive load.web-server/src/components/Service/SystemLog/LogSearch.tsx (2)
7-16:gaphas no effect withoutdisplay:flex(or grid)
SearchContainersetsgap: 5but the default display isblock, so the property is ignored. Adddisplay: flex(or grid) to get the intended spacing:-const SearchContainer = styled('div')(() => ({ +const SearchContainer = styled('div')(() => ({ position: 'sticky', top: 0, zIndex: 1, gap: 5, + display: 'flex', paddingBottom: 8, alignItems: 'center', backdropFilter: 'blur(10px)', borderRadius: 5, }));
64-71: Prevent the default “Enter” behaviour in the text fieldPressing in a single‑line
TextFieldcan submit the surrounding form (if any) or trigger an unintended click on the first<button>in the DOM. Addevent.preventDefault()after handling navigation.- if (event.key === 'Enter' && event.shiftKey) { + if (event.key === 'Enter' && event.shiftKey) { + event.preventDefault(); onNavigate('prev'); } else if (event.key === 'Enter') { + event.preventDefault(); onNavigate('next'); }web-server/src/content/Service/SystemLogs.tsx (1)
75-78: Use a data‑attribute instead of inline style to mark matchesSelecting by
span[style*="background-color: yellow"]can yield false positives and is brittle if the style changes. When rendering highlights, adddata-log-matchand query via[data-log-match]to decouple logic from styling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web-server/src/components/Service/SystemLog/FormattedLog.tsx(2 hunks)web-server/src/components/Service/SystemLog/LogSearch.tsx(1 hunks)web-server/src/components/Service/SystemLog/PlainLog.tsx(1 hunks)web-server/src/content/Service/SystemLogs.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/src/components/Service/SystemLog/FormattedLog.tsx (1)
web-server/src/types/resources.ts (1)
ParsedLog(1056-1062)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web-server/src/content/Service/SystemLogs.tsx (1)
75-87:⚠️ Potential issueCritical issue: useEffect inside a callback function
React hooks can only be called at the top level of your component or other hooks, not inside regular JavaScript functions, conditions, or loops. The
useEffecthook inside yourhandleSearchcallback will cause runtime errors.Move this effect to the component's top level:
- const handleSearch = useCallback((query: string) => { - setSearchQuery(query); - setCurrentMatch(0); - setTotalMatches(0); - setHighlightedElements([]); - updateHighlight(null); - - if (!query) return; - - useEffect(() => { - const elements = Array.from( - containerRef.current?.querySelectorAll('span[style*="background-color: yellow"]') ?? [] - ) as HTMLElement[]; - setHighlightedElements(elements); - setTotalMatches(elements.length); - setCurrentMatch(elements.length ? 1 : 0); - - if (elements.length) { - updateHighlight(elements[0]); - elements[0].scrollIntoView({ behavior: 'smooth', block: 'center' }); - } - }, [searchQuery, logs, updateHighlight]); - - }, []); + const handleSearch = useCallback((query: string) => { + setSearchQuery(query); + setCurrentMatch(0); + setTotalMatches(0); + setHighlightedElements([]); + updateHighlight(null); + }, [updateHighlight]); + + useEffect(() => { + if (!searchQuery) return; + + const elements = Array.from( + containerRef.current?.querySelectorAll('span[style*="background-color: yellow"]') ?? [] + ) as HTMLElement[]; + setHighlightedElements(elements); + setTotalMatches(elements.length); + setCurrentMatch(elements.length ? 1 : 0); + + if (elements.length) { + updateHighlight(elements[0]); + elements[0].scrollIntoView({ behavior: 'smooth', block: 'center' }); + } + }, [searchQuery, logs, updateHighlight]);
🧹 Nitpick comments (1)
web-server/src/content/Service/SystemLogs.tsx (1)
51-64: Consider using CSS classes for highlights instead of inline stylesUsing inline styles for highlights works, but it can be harder to maintain. Consider using CSS classes with defined styles in your stylesheet.
- if (currentHighlightRef.current) { - currentHighlightRef.current.style.backgroundColor = 'yellow'; - currentHighlightRef.current.style.color = 'black'; - } - - if (element) { - element.style.backgroundColor = 'rgb(255, 161, 10)'; - element.style.color = 'white'; - currentHighlightRef.current = element; - } else { - currentHighlightRef.current = null; - } + if (currentHighlightRef.current) { + currentHighlightRef.current.classList.remove('active-highlight'); + currentHighlightRef.current.classList.add('search-highlight'); + } + + if (element) { + element.classList.remove('search-highlight'); + element.classList.add('active-highlight'); + currentHighlightRef.current = element; + } else { + currentHighlightRef.current = null; + }And add these CSS classes to your stylesheet:
.search-highlight { background-color: yellow; color: black; } .active-highlight { background-color: rgb(255, 161, 10); color: white; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web-server/src/content/Service/SystemLogs.tsx(4 hunks)
🔇 Additional comments (7)
web-server/src/content/Service/SystemLogs.tsx (7)
128-137: Properly implemented container ref on scrollable elementGreat job fixing the container ref issue from the previous review! The
containerRefis now correctly attached to the scrollable FlexBox, which will allow the scroll logic to work properly.
91-107: The navigation logic works wellThe navigation function properly handles cycling through matches in both directions. It updates the current match index, scrolls to the selected element, and updates the highlight styling.
33-36: Simplified scroll detection logicGood simplification of the scroll detection logic! Using a direct scroll event listener is more straightforward than an IntersectionObserver for this use case.
133-134: Properly integrated search query with log componentsThe search query is correctly passed down to both
PlainLogandFormattedLogcomponents, which will enable highlighting of matched text in the logs.Also applies to: 135-136
116-121: Well-integrated search UI componentThe LogSearch component is properly integrated with all the necessary props for search functionality and navigation.
42-49: Smooth scrolling implementationThe scrollToBottom function properly implements smooth scrolling behavior to enhance the user experience.
77-77: 🛠️ Refactor suggestionPotential null reference issue
The optional chaining on
containerRef.current?.querySelectorAllprotects against null, but if the component hasn't rendered yet, there won't be any elements to query.Add a check to ensure the search is only performed when the container is available:
- const elements = Array.from( - containerRef.current?.querySelectorAll('span[style*="background-color: yellow"]') ?? [] - ) as HTMLElement[]; + const elements = containerRef.current + ? Array.from(containerRef.current.querySelectorAll('span[style*="background-color: yellow"]')) as HTMLElement[] + : [];Likely an incorrect or invalid review comment.
|
from the looks of it is just highlighting the logs, not actually searching through them or filtering the search results. Seems satisfactory from the issue. But can we make this more usable ? |
…t field and improve INP
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
web-server/src/content/Service/SystemLogs.tsx (1)
154-167: 🛠️ Refactor suggestionRemove direct reference to DOM elements in JSX calculation.
Accessing DOM properties directly in render calculations can cause issues during SSR and is generally not a React best practice. Move this calculation to a useEffect or memoized value.
+ const [buttonMarginLeft, setButtonMarginLeft] = useState(0); + + useEffect(() => { + if (containerRef.current) { + setButtonMarginLeft(containerRef.current.clientWidth / 2 - 67); + } + }, []); // In the JSX <Button onClick={scrollToBottom} component={MotionBox} initial={{ opacity: 0, scale: 0.7 }} animate={{ opacity: 1, scale: 1 }} transition={{ ease: 'easeOut' }} bottom={20} sx={{ position: 'fixed', - marginLeft: `calc(${containerRef.current - ? containerRef.current.clientWidth / 2 - 67 - : 0 - }px)` + marginLeft: `${buttonMarginLeft}px` }} >
♻️ Duplicate comments (1)
web-server/src/content/Service/SystemLogs.tsx (1)
69-84: DOM query for highlighted elements is brittle and should be improved.The current approach of querying elements by their inline style attribute is fragile. React might reformat or reorder style properties, causing the selector to fail. This was noted in a previous review comment as well.
As suggested in the previous review, use a dedicated class or data attribute for highlighted elements and update your query selector:
- const elements = Array.from( - containerRef.current?.querySelectorAll('span[style*="background-color: yellow"]') ?? [] - ) as HTMLElement[]; + const elements = Array.from( + containerRef.current?.querySelectorAll('span.log-highlight') ?? [] + ) as HTMLElement[];Make sure to add the corresponding class to the highlighted spans in both PlainLog and FormattedLog components.
🧹 Nitpick comments (2)
web-server/src/content/Service/SystemLogs.tsx (2)
54-67: Consider debouncing the search input to improve performance.For an improved user experience, consider adding debounce to prevent excessive processing during rapid typing. This is especially important since you're performing DOM operations in response to search input.
+import { debounce } from 'lodash'; + const SystemLogs = memo(({ serviceName }: { serviceName?: ServiceNames }) => { // ... existing code - const handleSearch = useCallback((query: string) => { + const debouncedSearch = useCallback( + debounce((query: string) => { + // Reset states only when query is empty or below a threshold + if (!query || query.length < 3) { + setCurrentMatch(0); + setTotalMatches(0); + setHighlightedElements([]); + updateHighlight(null); + return; + } + // Let the effect handle DOM updates + }, 300), + [updateHighlight] + ); + + const handleSearch = useCallback((query: string) => { setSearchQuery(query); - - // Reset states only when query is empty or below a threshold - if (!query || query.length < 3) { - setCurrentMatch(0); - setTotalMatches(0); - setHighlightedElements([]); - updateHighlight(null); - return; - } - - // Let the effect handle DOM updates + debouncedSearch(query); }, [updateHighlight]);
87-103: Add error handling for navigation edge cases.The navigation function works well, but lacks error handling for edge cases. Consider adding checks to ensure the element still exists in the DOM before attempting to scroll to it.
const handleNavigate = useCallback((direction: 'prev' | 'next') => { if (highlightedElements.length === 0) return; let newIndex = currentMatch; if (direction === 'next') { newIndex = currentMatch < totalMatches ? currentMatch + 1 : 1; } else { newIndex = currentMatch > 1 ? currentMatch - 1 : totalMatches; } setCurrentMatch(newIndex); const element = highlightedElements[newIndex - 1]; - if (element) { + if (element && document.body.contains(element)) { element.scrollIntoView({ behavior: 'smooth', block: 'center' }); updateHighlight(element); + } else { + // Element no longer in DOM, re-query highlighted elements + const freshElements = Array.from( + containerRef.current?.querySelectorAll('span[style*="background-color: yellow"]') ?? [] + ) as HTMLElement[]; + setHighlightedElements(freshElements); + if (freshElements.length) { + setTotalMatches(freshElements.length); + setCurrentMatch(1); + updateHighlight(freshElements[0]); + freshElements[0].scrollIntoView({ behavior: 'smooth', block: 'center' }); + } } }, [currentMatch, totalMatches, highlightedElements, updateHighlight]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web-server/src/components/Service/SystemLog/LogSearch.tsx(1 hunks)web-server/src/content/Service/SystemLogs.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web-server/src/components/Service/SystemLog/LogSearch.tsx
🔇 Additional comments (8)
web-server/src/content/Service/SystemLogs.tsx (8)
3-3: Component optimization with React.memo and new imports look good.The component is properly wrapped with
memofor performance optimization, and all necessary hooks and components are imported. This will prevent unnecessary re-renders when theserviceNameprop doesn't change.Also applies to: 8-8, 19-19, 175-175
22-28: State management for search functionality is well-structured.The state variables are appropriately separated for different aspects of the search functionality. Good use of the ref for tracking the currently highlighted element across renders.
30-37: Clean implementation of scrollToBottom function.The scroll functionality is simplified with direct scrollTo calls, which is more straightforward than the previous approach.
39-52: The highlight management callback looks good.The updateHighlight callback effectively toggles between the default yellow background and the distinct orange highlight for the active match.
105-115: Initial scroll implementation looks good.The initial scroll implementation now correctly uses a ref to track the initial load and requestAnimationFrame to ensure the DOM is ready before scrolling.
117-125: Memoized renderLogs function is a good optimization.Extracting the logs rendering logic into a memoized callback is a good optimization that prevents unnecessary re-renders.
134-139: Good integration of the LogSearch component.The LogSearch component is properly integrated with all the necessary props. This cleanly separates the search UI from the main component.
146-148: Container ref placement is now correct.The containerRef is now properly attached to the scrollable container, fixing the previous issue where it was attached to an empty element.
Improved pointer and key input interactions from 150 ms to 80ms |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web-server/src/content/Service/SystemLogs.tsx (2)
88-90: Replace fragile style selector with semantic class / data-attributeThe DOM lookup relies on matching the literal string
background-color: yellow, which is brittle (style order, minification, different color formats, etc.).
A robust approach is to mark the highlighted<span>with a class (log-highlight) ordata-attribute and query that instead.This point was raised in a previous review but is still unresolved.
22-23:showScrollDownButtonis never toggled – add scroll listener
showScrollDownButtonis initialised but never updated when the user scrolls away from the bottom, so the floating button will never appear.A simple scroll handler on the
containerRefwill fix this:+ /* show / hide “scroll-to-bottom” button */ + useEffect(() => { + const el = containerRef.current; + if (!el) return; + + const handleScroll = () => { + const { scrollTop, scrollHeight, clientHeight } = el; + const atBottom = Math.abs(scrollHeight - scrollTop - clientHeight) < 10; + showScrollDownButton.set(!atBottom); + }; + + el.addEventListener('scroll', handleScroll); + handleScroll(); // initialise + return () => el.removeEventListener('scroll', handleScroll); + }, [showScrollDownButton]);Also applies to: 179-185
🧹 Nitpick comments (1)
web-server/src/content/Service/SystemLogs.tsx (1)
191-195: Hard-codedmarginLeftbreaks on resize – use CSS centeringThe floating button’s horizontal position is calculated once during render and won’t adjust if the log pane resizes.
Usingleft: 50%withtransform: translateX(-50%)is simpler and fully responsive:- bottom={20} - sx={{ - position: 'fixed', - marginLeft: `calc(${containerRef.current - ? containerRef.current.clientWidth / 2 - 67 - : 0 - }px)` - }} + sx={{ + position: 'fixed', + bottom: 20, + left: '50%', + transform: 'translateX(-50%)' + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web-server/src/content/Service/SystemLogs.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/src/content/Service/SystemLogs.tsx (5)
web-server/src/hooks/useSystemLogs.tsx (1)
useSystemLogs(5-22)web-server/src/utils/logFormatter.ts (1)
parseLogLine(12-95)web-server/src/components/Service/SystemLog/PlainLog.tsx (1)
PlainLog(34-40)web-server/src/components/Service/SystemLog/FormattedLog.tsx (1)
FormattedLog(37-82)web-server/src/components/Service/SystemLog/SystemLogsErrorFllback.tsx (1)
SystemLogsErrorFallback(12-60)
| useLayoutEffect(() => { | ||
| if (!debouncedSearchQuery || debouncedSearchQuery.length < 3) return; | ||
|
|
||
| const elements = Array.from( | ||
| containerRef.current?.querySelectorAll('span[style*="background-color: yellow"]') ?? [] | ||
| ) as HTMLElement[]; | ||
|
|
||
| setHighlightedElements(elements); | ||
| highlightedElementsRef.current = elements; | ||
| const newTotalMatches = elements.length; | ||
| setTotalMatches(newTotalMatches); | ||
| totalMatchesRef.current = newTotalMatches; | ||
|
|
||
| if (currentMatchRef.current === 0) { | ||
| setCurrentMatch(newTotalMatches ? 1 : 0); | ||
| currentMatchRef.current = newTotalMatches ? 1 : 0; | ||
| } | ||
| }, [debouncedSearchQuery]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Re-run highlight scan when new logs arrive
useLayoutEffect only depends on debouncedSearchQuery.
If new log lines stream in while the user keeps the same query, they will never be highlighted.
- }, [debouncedSearchQuery]);
+ }, [debouncedSearchQuery, logs]);Adding logs ensures new matches are detected as soon as fresh data renders.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useLayoutEffect(() => { | |
| if (!debouncedSearchQuery || debouncedSearchQuery.length < 3) return; | |
| const elements = Array.from( | |
| containerRef.current?.querySelectorAll('span[style*="background-color: yellow"]') ?? [] | |
| ) as HTMLElement[]; | |
| setHighlightedElements(elements); | |
| highlightedElementsRef.current = elements; | |
| const newTotalMatches = elements.length; | |
| setTotalMatches(newTotalMatches); | |
| totalMatchesRef.current = newTotalMatches; | |
| if (currentMatchRef.current === 0) { | |
| setCurrentMatch(newTotalMatches ? 1 : 0); | |
| currentMatchRef.current = newTotalMatches ? 1 : 0; | |
| } | |
| }, [debouncedSearchQuery]); | |
| useLayoutEffect(() => { | |
| if (!debouncedSearchQuery || debouncedSearchQuery.length < 3) return; | |
| const elements = Array.from( | |
| containerRef.current?.querySelectorAll('span[style*="background-color: yellow"]') ?? [] | |
| ) as HTMLElement[]; | |
| setHighlightedElements(elements); | |
| highlightedElementsRef.current = elements; | |
| const newTotalMatches = elements.length; | |
| setTotalMatches(newTotalMatches); | |
| totalMatchesRef.current = newTotalMatches; | |
| if (currentMatchRef.current === 0) { | |
| setCurrentMatch(newTotalMatches ? 1 : 0); | |
| currentMatchRef.current = newTotalMatches ? 1 : 0; | |
| } | |
| }, [debouncedSearchQuery, logs]); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
web-server/src/content/Service/SystemLogs.tsx (3)
87-89: Use a stable marker for highlighted spans instead of relying on inline styleThe selector
'span[style*="background-color: yellow"]'is fragile as React may normalize styles or reorder properties.- const elements = Array.from( - containerRef.current?.querySelectorAll('span[style*="background-color: yellow"]') ?? [] - ) as HTMLElement[]; + const elements = Array.from( + containerRef.current?.querySelectorAll('.log-highlight') ?? [] + ) as HTMLElement[];This means you'll need to add the
className="log-highlight"to the highlighted spans in PlainLog and FormattedLog components.
84-102: Re-run highlight scan when new logs arriveThe useLayoutEffect only depends on debouncedSearchQuery. If new logs load while a search query is active, matches in those logs won't be highlighted or included in navigation.
- }, [debouncedSearchQuery]); + }, [debouncedSearchQuery, logs]);
22-22: Missing implementation for showScrollDownButton toggleYou've removed the IntersectionObserver but there's no mechanism to detect when the user scrolls up to show the scroll-down button.
const SystemLogs = memo(({ serviceName }: { serviceName?: ServiceNames }) => { // ... existing code + useEffect(() => { + const handleScroll = () => { + if (!containerRef.current) return; + + const { scrollTop, scrollHeight, clientHeight } = containerRef.current; + const isScrolledToBottom = Math.abs(scrollHeight - scrollTop - clientHeight) < 10; + showScrollDownButton.set(!isScrolledToBottom); + }; + + const scrollContainer = containerRef.current; + if (scrollContainer) { + scrollContainer.addEventListener('scroll', handleScroll); + } + + return () => { + if (scrollContainer) { + scrollContainer.removeEventListener('scroll', handleScroll); + } + }; + }, [showScrollDownButton]);
🧹 Nitpick comments (2)
web-server/src/content/Service/SystemLogs.tsx (2)
53-66: Direct DOM manipulation may conflict with React's renderingYou're directly manipulating DOM element styles instead of using React's state-based rendering. This can lead to issues if React re-renders the elements.
Consider modifying the
HighlightedTextcomponent to accept anisActiveprop, so React can handle the highlighting:// In PlainLog and FormattedLog components: <HighlightedText text={text} searchQuery={searchQuery} + isActiveMatch={matchIndex === currentActiveMatchIndex} /> // In HighlightedText component: const getStyle = (isMatch, isActive) => { if (isMatch) { return isActive ? { backgroundColor: 'rgb(255, 161, 10)', color: 'white' } : { backgroundColor: 'yellow', color: 'black' }; } return {}; };This would eliminate the need for direct DOM manipulation.
72-81: Consider extracting magic numbers as constantsThe minimum search query length (3 characters) is hardcoded in multiple places.
+const MIN_SEARCH_LENGTH = 3; const handleSearch = useCallback((query: string) => { setSearchQuery(query); isSearchingRef.current = !!query; - if (!query || query.length < 3) { + if (!query || query.length < MIN_SEARCH_LENGTH) { // ...Also update line 85 to use the constant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web-server/src/content/Service/SystemLogs.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/src/content/Service/SystemLogs.tsx (6)
web-server/src/hooks/useSystemLogs.tsx (1)
useSystemLogs(5-22)web-server/src/utils/logFormatter.ts (1)
parseLogLine(12-95)web-server/src/components/Service/SystemLog/PlainLog.tsx (1)
PlainLog(34-40)web-server/src/components/Service/SystemLog/FormattedLog.tsx (1)
FormattedLog(37-82)web-server/src/components/Service/SystemLog/SystemLogsErrorFllback.tsx (1)
SystemLogsErrorFallback(12-60)web-server/src/components/Service/SystemLog/LogSearch.tsx (1)
LogSearch(178-178)
🔇 Additional comments (5)
web-server/src/content/Service/SystemLogs.tsx (5)
176-178: Proper implementation of container with refThis is a good improvement that addresses a previous critical issue - you've correctly attached the
containerRefto the scrollable container.
164-169: Good integration of the search componentThe LogSearch component is well-integrated with proper props being passed for search functionality and navigation.
142-155: Good extraction of renderLogs as a memoized callbackThe extraction of the log rendering logic into a memoized callback with the correct dependencies is a good performance optimization.
27-34: 🛠️ Refactor suggestionRedundant state and ref duplications
You have duplicate state and refs for highlightedElements, currentMatch, and totalMatches. This creates potential synchronization issues.
Remove the redundant state variables and use either the state or the refs consistently:
- const [highlightedElements, setHighlightedElements] = useState<HTMLElement[]>([]); - const [currentMatch, setCurrentMatch] = useState(0); - const [totalMatches, setTotalMatches] = useState(0); const currentHighlightRef = useRef<HTMLElement | null>(null); const isInitialLoad = useRef(true); const isSearchingRef = useRef(false); const currentMatchRef = useRef(0); const totalMatchesRef = useRef(0); const highlightedElementsRef = useRef<HTMLElement[]>([]); const isNavigatingRef = useRef(false);Then update your component to only use the refs for DOM manipulation and state for rendering:
// When updating for UI rendering - setHighlightedElements(elements); highlightedElementsRef.current = elements; - setTotalMatches(newTotalMatches); totalMatchesRef.current = newTotalMatches; - setCurrentMatch(newTotalMatches ? 1 : 0); currentMatchRef.current = newTotalMatches ? 1 : 0; // In LogSearch component props <LogSearch onSearch={handleSearch} onNavigate={handleNavigate} - currentMatch={currentMatch} - totalMatches={totalMatches} + currentMatch={currentMatchRef.current} + totalMatches={totalMatchesRef.current} />Likely an incorrect or invalid review comment.
68-82:✅ Verification successful
Consider enhancing search with filtering capability
According to the PR comments, the current implementation only highlights matches but doesn't filter the logs to show only matching entries.
Consider enhancing the feature by adding an option to filter logs to show only entries containing the search query:
+ const [shouldFilterLogs, setShouldFilterLogs] = useState(false); // In handleSearch: const handleSearch = useCallback((query: string) => { // ...existing code... }, [updateHighlight]); + const filteredLogs = useMemo(() => { + if (!debouncedSearchQuery || !shouldFilterLogs) return logs; + return logs.filter(log => log.toLowerCase().includes(debouncedSearchQuery.toLowerCase())); + }, [logs, debouncedSearchQuery, shouldFilterLogs]); // In renderLogs: - return logs.map((log, index) => { + return (shouldFilterLogs ? filteredLogs : logs).map((log, index) => { // Update LogSearch component: <LogSearch onSearch={handleSearch} onNavigate={handleNavigate} currentMatch={currentMatch} totalMatches={totalMatches} + onToggleFilter={setShouldFilterLogs} + isFilterEnabled={shouldFilterLogs} />Then update your LogSearch component to include a filter toggle button.
🏁 Script executed:
#!/bin/bash # Check if this enhancement aligns with the PR requirements echo "PR objectives mention:" cat << EOF The pull request titled "feat: Implemented log search functionality with highlighted results" aims to enhance the log viewing experience by adding search capabilities within logs. The comment on the PR by user samad-yar-khan notes that the current implementation appears to highlight the logs but does not actually perform searching through or filtering of the search results. The commenter suggests considering enhancements to make the feature more usable. EOF echo -e "\nChecking the current LogSearch component implementation:" fd -t f LogSearch.tsx | xargs catLength of output: 5083
🏁 Script executed:
#!/bin/bash # Locate SystemLogs.tsx and show relevant sections around log rendering file=$(fd -t f SystemLogs.tsx) echo "----- File: $file -----" sed -n '1,200p' "$file" echo "----- Rendering logs in SystemLogs.tsx -----" grep -R "renderLogs" -n "$file" grep -R "logs" -n "$file" | sed -n '1,50p'Length of output: 1131
🏁 Script executed:
#!/bin/bash set -e # Find SystemLogs.tsx files files=$(fd -t f SystemLogs.tsx) for f in $files; do echo "===== File: $f =====" # Show first 200 lines for context sed -n '1,200p' "$f" echo -e "\n--- Occurrences of 'logs', 'render', and 'renderLogs' in $f ---" grep -E -n "(logs|renderLogs|render)" "$f" || true echo -e "\n\n" doneLength of output: 9077
No changes needed: filtering logs is a future enhancement
The current PR implements in-line match highlighting per its scope. Introducing entry‐level filtering would be a valuable follow-up but isn’t required to fulfill the “highlight search results” objective here.• The existing
handleSearch+renderLogsflow correctly highlights matches without altering the log list.
• If you’d like to expose a “show only matching entries” toggle, consider capturing it in a separate issue or PR to avoid expanding the current feature scope.
There was a problem hiding this comment.
Okay, first things first, my review might be overwhelming.
Sorry about that, should have done this in parts perhaps.
But given that I rarely get time to review PRs in our OSS repo, I decided doing it all in one go might save you more time.
That being said, great first attempt, because it functions alright. :)
However, the code quality leaves a lot to be desired.
Take this as a challenge, I'm hoping you'll come away having learnt a lot of serious stuff from this.
Some learning material I've linked in one of the comments:
https://kentcdodds.com/blog/make-impossible-states-impossible
https://www.youtube.com/watch?v=IcgmSRJHu_8 (link is about Elm lang, but focus on the concept - it's beautifully presented)
| } | ||
|
|
||
| const HighlightedText = ({ text, searchQuery }: { text: string; searchQuery?: string }) => { | ||
| if (!searchQuery) return <>{text}</>; |
There was a problem hiding this comment.
A component can directly return text. Text nodes are valid react nodes.
| const HighlightedText = ({ text, searchQuery }: { text: string; searchQuery?: string }) => { | ||
| if (!searchQuery) return <>{text}</>; | ||
|
|
||
| const escapeRegExp = (string: string) => |
There was a problem hiding this comment.
This is a nitpick. You don't need to address this.
Constants, functions, or any logic that doesn't strictly depend on the component itself can be declared outside of it.
This allows the reference to be maintained and eliminates and chances of rerendering of anything that depends on this stuff.
Right now it's not a required change because no child components depend on it at the moment. But doing so anyway is a good practice because if someone needs to use this function in the future:
- They don't have to use it from a prop.
- They won't have rerender issues due to references getting changed.
| ) : ( | ||
| part | ||
| ) |
There was a problem hiding this comment.
Don't really need to wrap in a parenthesis
| <> | ||
| {parts.map((part, i) => |
There was a problem hiding this comment.
You'll need to check this, but I think you can directly return the parts.map and stuff without wrapping in a fragment. Confirm once.
| searchQuery?: string; | ||
| } | ||
|
|
||
| const HighlightedText = ({ text, searchQuery }: { text: string; searchQuery?: string }) => { |
There was a problem hiding this comment.
Calling the component HighlightedText is misleading because this is used to render logs regardless of whether it's highlighted or not.
FormattedLog was perhaps a better name.
|
|
||
| const containerElement = containerRef.current; | ||
| const timer = setTimeout(() => { | ||
| setDebouncedSearchQuery(searchQuery); |
There was a problem hiding this comment.
I think the pattern you're using is:
- User input
- debounce 300ms
- Update search query
- wait 300ms
- handle search highlighting
This is something I've seen beginners do, and, when it's laid out for you I'm sure you can see what's wrong here.
Debouncing highlighting on user input is definitely a good idea, but your flow should just be:
- user inputs
- debounce 300ms
- trigger highlighting handler
You also shouldn't maintain two states for search queries, one input by the user, one used by the highlighting handler.
P.S.: This is the kind of stuff that causes folks to fail our frontend interviews, so with this learning, you now have an advantage over them. :) Hopefully you'll come out a lot stronger out of this program. :)
| if (currentHighlightRef.current) { | ||
| currentHighlightRef.current.style.backgroundColor = 'yellow'; | ||
| currentHighlightRef.current.style.color = 'black'; | ||
| } | ||
|
|
||
| if (element) { | ||
| element.style.backgroundColor = 'rgb(255, 161, 10)'; | ||
| element.style.color = 'white'; | ||
| currentHighlightRef.current = element; | ||
| } else { | ||
| currentHighlightRef.current = null; | ||
| } |
There was a problem hiding this comment.
Well... that's not a great idea my dude. Use CSS classes, or even MUI's css-in-js based on some supplied prop. Don't mutate the DOM like this unless there's no other way (sometimes that is the case, but it's not right now).
If you're not sure why I'm saying this, feel free to ask. I urge anyone working with us to not just absorb feedback if they aren't convinced.
| const handleSearch = useCallback((query: string) => { | ||
| setSearchQuery(query); | ||
| isSearchingRef.current = !!query; | ||
|
|
||
| if (!query || query.length < 3) { | ||
| setCurrentMatch(0); | ||
| currentMatchRef.current = 0; | ||
| setTotalMatches(0); | ||
| totalMatchesRef.current = 0; | ||
| setHighlightedElements([]); | ||
| highlightedElementsRef.current = []; | ||
| updateHighlight(null); | ||
| return; | ||
| } | ||
| }, [updateHighlight]); | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!debouncedSearchQuery || debouncedSearchQuery.length < 3) return; | ||
|
|
||
| const elements = Array.from( | ||
| containerRef.current?.querySelectorAll('span[style*="background-color: yellow"]') ?? [] | ||
| ) as HTMLElement[]; | ||
|
|
||
| setHighlightedElements(elements); | ||
| highlightedElementsRef.current = elements; | ||
| const newTotalMatches = elements.length; | ||
| setTotalMatches(newTotalMatches); | ||
| totalMatchesRef.current = newTotalMatches; | ||
|
|
||
| if (currentMatchRef.current === 0) { | ||
| setCurrentMatch(newTotalMatches ? 1 : 0); | ||
| currentMatchRef.current = newTotalMatches ? 1 : 0; | ||
| } | ||
| }, [debouncedSearchQuery]); |
There was a problem hiding this comment.
Same feedback as my above comments about reducers and state machines.
This code is too easy to add bugs to.
There was a problem hiding this comment.
Other easily avoidable problems:
- Direct DOM reads and manipulation
- Too many ref changes tracked and updated
There was a problem hiding this comment.
Also...
💀 (in this context)
querySelectorAll('span[style*="background-color: yellow"]')
-100 points to Gryffindor.
| requestAnimationFrame(() => { | ||
| let newIndex = currentMatchRef.current; | ||
| const total = totalMatchesRef.current; | ||
|
|
||
| if (direction === 'next') { | ||
| newIndex = newIndex < total ? newIndex + 1 : 1; | ||
| } else { | ||
| newIndex = newIndex > 1 ? newIndex - 1 : total; | ||
| } | ||
| }; | ||
| }, [showScrollDownButton]); | ||
|
|
||
| setCurrentMatch(newIndex); | ||
| currentMatchRef.current = newIndex; | ||
| const element = highlightedElementsRef.current[newIndex - 1]; | ||
| if (element) { | ||
| element.scrollIntoView({ behavior: 'smooth', block: 'center' }); | ||
| updateHighlight(element); | ||
| } | ||
| isNavigatingRef.current = false; | ||
| }); | ||
| }, [updateHighlight]); |
There was a problem hiding this comment.
This is an example of adding hacky code to make existing hacky code work.
Using a proper state machine architecture will make this SO MUCH EASIER to handle.
| marginLeft: `calc(${containerRef.current | ||
| ? containerRef.current.clientWidth / 2 - 67 | ||
| : 0 | ||
| }px)` | ||
| }px)` |
There was a problem hiding this comment.
Is this an intentional change? Or something that the linter did? Or maybe your IDE?
Linked Issue(s)
#556
Acceptance Criteria fulfillment
Proposed changes
Screen.Recording.2025-04-21.at.4.27.38.PM.mov
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements