Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(query): Add a "Search" tab in the Sidebar for wildcard queries. #107

Merged
merged 27 commits into from
Nov 16, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5094abf
refactor SearchTabPanel from new log viewer folder back to current lo…
Henry8192 Oct 24, 2024
dcada93
Rewrite the ResultsGroup return type
Henry8192 Oct 25, 2024
ac22041
show queryResults in search tab panel
Henry8192 Oct 28, 2024
908b6db
fix search results not showing; show contexts before and after search…
Henry8192 Oct 31, 2024
b931505
Nasty solution for individual search results expansion control
Henry8192 Nov 1, 2024
3ddd96a
limit max query results to 1000; add jsdoc for ResultsGroup
Henry8192 Nov 4, 2024
2cbb032
change ResultsGroup structure: each should be a page of results; rewr…
Henry8192 Nov 4, 2024
0889361
amend stylelint
Henry8192 Nov 4, 2024
d8cd28c
edit QUERY_RESULT protocol to show search progress
Henry8192 Nov 5, 2024
4709312
display search progress in status bar
Henry8192 Nov 7, 2024
a8a1551
move search progress bar under search input box
Henry8192 Nov 7, 2024
67957ca
search results now support jump to current log event
Henry8192 Nov 7, 2024
640b3e3
address issues in code review
Henry8192 Nov 11, 2024
477ded6
Rename "search" -> "query" where appropriate; fix CSS issues.
Henry8192 Nov 11, 2024
e1dc2df
Fix panel overflow when there are a few query results; change progres…
Henry8192 Nov 11, 2024
ec8cc58
clean up sx props to css files
Henry8192 Nov 12, 2024
96606b7
Apply suggestions from code review
Henry8192 Nov 12, 2024
854293f
Resolve issues from code review
Henry8192 Nov 12, 2024
30481f9
fix lint
Henry8192 Nov 12, 2024
8a81a87
Apply suggestions from code review
Henry8192 Nov 14, 2024
2c10ab7
resolve rest of suggestions
Henry8192 Nov 14, 2024
fb12080
fix lint
Henry8192 Nov 14, 2024
34a8043
Merge branch 'main' into search-tab-panel
Henry8192 Nov 14, 2024
f71269e
Update src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabP…
Henry8192 Nov 15, 2024
e002f4f
Merge branch 'main' into search-tab-panel
junhaoliao Nov 15, 2024
8793d0b
Rename `DecodeResultType` -> `DecodeResult` as a result of merging fr…
junhaoliao Nov 15, 2024
580b876
Merge branch 'main' into search-tab-panel
Henry8192 Nov 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from "react";

import {
ButtonGroup,
DialogContent,
DialogTitle,
TabPanel,
Expand All @@ -14,6 +15,7 @@ interface CustomTabPanelProps {
children: React.ReactNode,
tabName: string,
title: string,
titleButtons?: React.ReactNode,
}

/**
Expand All @@ -23,9 +25,15 @@ interface CustomTabPanelProps {
* @param props.children
* @param props.tabName
* @param props.title
* @param props.titleButtons
* @return
*/
const CustomTabPanel = ({children, tabName, title}: CustomTabPanelProps) => {
const CustomTabPanel = ({
children,
tabName,
title,
titleButtons,
}: CustomTabPanelProps) => {
return (
<TabPanel
className={"sidebar-tab-panel"}
Expand All @@ -38,6 +46,13 @@ const CustomTabPanel = ({children, tabName, title}: CustomTabPanelProps) => {
>
{title}
</Typography>
<ButtonGroup
size={"sm"}
spacing={"1px"}
variant={"plain"}
>
{titleButtons}
</ButtonGroup>
</DialogTitle>
<DialogContent>
{children}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
.result-button {
user-select: none;

overflow: hidden;
display: block !important;

text-overflow: ellipsis;
white-space: nowrap;
}

.result-text {
display: inline !important;
font-size: 0.875rem !important;
font-weight: 400 !important;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import {
ListItemButton,
Typography,
} from "@mui/joy";

import "./Result.css";


interface ResultProps {
message: string,
matchRange: [number, number]
}

/**
* Displays a button containing a message, which highlights a specific range of text.
*
* @param props
* @param props.message
* @param props.matchRange A two-element array indicating the start and end indices of the substring
* to be highlighted.
* @return
*/
const Result = ({message, matchRange}: ResultProps) => {
const [
beforeMatch,
match,
afterMatch,
] = [
message.slice(0, matchRange[0]),
message.slice(...matchRange),
message.slice(matchRange[1]),
];

return (
<ListItemButton className={"result-button"}>
<Typography
className={"result-text"}
level={"body-xs"}
>
{beforeMatch}
</Typography>
<Typography
className={"result-text"}
level={"body-xs"}
sx={{
backgroundColor: "warning.softBg",
}}
>
{match}
</Typography>
<Typography
className={"result-text"}
level={"body-xs"}
>
{afterMatch}
</Typography>
</ListItemButton>
);
};

export default Result;
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
.results-group-title-button {
flex-direction: row-reverse !important;
gap: 2px !important;
padding-inline-start: 0 !important;
}

.results-group-title-container {
display: flex;
flex-grow: 1;
}

.results-group-title-text-container {
flex-grow: 1;
gap: 0.1rem;
align-items: center;
}

.results-group-content {
margin-left: 1.5px !important;
border-left: 1px solid var(--joy-palette-neutral-outlinedBorder, #cdd7e1);

Check failure on line 20 in src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ResultsGroup.css

View workflow job for this annotation

GitHub Actions / lint-check

Stylelint problem

Expected custom property name "--joy-palette-neutral-outlinedBorder" to be kebab-case - https://stylelint.io/user-guide/rules/custom-property-pattern
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import {
useEffect,
useState,
} from "react";
import * as React from "react";

import {
Accordion,
AccordionDetails,
AccordionSummary,
Box,
Chip,
List,
Stack,
Typography,
} from "@mui/joy";

import DescriptionOutlinedIcon from "@mui/icons-material/DescriptionOutlined";

import {QueryResults} from "../../../../../typings/worker";
import Result from "./Result";

import "./ResultsGroup.css";


interface ResultsGroupProps {
isAllExpanded: boolean,
queryResults: QueryResults,
}

/**

Check warning on line 31 in src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ResultsGroup.tsx

View workflow job for this annotation

GitHub Actions / lint-check

Missing JSDoc block description

Check warning on line 31 in src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ResultsGroup.tsx

View workflow job for this annotation

GitHub Actions / lint-check

Missing JSDoc @return declaration
*
* @param props
* @param props.isAllExpanded
* @param props.queryResults
*/
const ResultsGroup = ({
isAllExpanded,
queryResults,
}: ResultsGroupProps) => {
const [isExpanded, setIsExpanded] = useState<boolean>(isAllExpanded);

const handleAccordionChange = (
_: React.SyntheticEvent,
newValue: boolean
) => {
setIsExpanded(newValue);
};

// On `isAllExpanded` updates, sync current results group's expand status.
useEffect(() => {
setIsExpanded(isAllExpanded);
}, [isAllExpanded]);

return (
<>
{Array.from(queryResults.entries()).map(([pageNum, results]) => (
<Accordion
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Implement immediate performance optimizations

Given the reported browser freezing issues, here are some immediate optimizations to implement while working on virtualization:

  1. Batch render updates
  2. Add result limit with "Show More" button
+const INITIAL_RESULTS_LIMIT = 50;
+
 const ResultsGroup = ({
     isAllExpanded,
     queryResults,
 }: ResultsGroupProps) => {
+    const [resultsLimit, setResultsLimit] = useState(INITIAL_RESULTS_LIMIT);
+    
     // ... existing code ...
     
     return (
         <>
             {Array.from(queryResults.entries()).map(([pageNum, results]) => (
                 <Accordion
                     // ... existing props ...
                 >
                     // ... existing AccordionSummary ...
                     <AccordionDetails className={"results-group-content"}>
                         <List size={"sm"}>
-                            {results.map((r) => (
+                            {results.slice(0, resultsLimit).map((r) => (
                                 <Result
                                     key={r.logEventNum}
                                     matchRange={r.matchRange}
                                     message={r.message}/>
                             ))}
+                            {results.length > resultsLimit && (
+                                <Button
+                                    onClick={() => setResultsLimit(prev => prev + INITIAL_RESULTS_LIMIT)}
+                                    variant="outlined"
+                                    fullWidth
+                                >
+                                    Show More Results
+                                </Button>
+                            )}
                         </List>
                     </AccordionDetails>
                 </Accordion>
             ))}
         </>
     );
 };

Also applies to: 114-119

expanded={isExpanded}
key={pageNum}
onChange={handleAccordionChange}
>
<AccordionSummary
slotProps={{
button: {className: "results-group-title-button"},
}}
>
<Box className={"results-group-title-container"}>
<Stack
className={"results-group-title-text-container"}
direction={"row"}
>
<DescriptionOutlinedIcon fontSize={"inherit"}/>
<Typography
level={"title-sm"}
>
Page
{" "}
{pageNum}
</Typography>
</Stack>
<Chip size={"sm"}>
{results.length}
</Chip>
</Box>
</AccordionSummary>
<AccordionDetails className={"results-group-content"}>
<List size={"sm"}>
{results.map((r) => (
<Result
key={r.logEventNum}
matchRange={r.matchRange}
message={r.message}/>
))}
</List>
</AccordionDetails>
</Accordion>
))}
</>
);
};

export default ResultsGroup;
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import {
useContext,
useRef,
useState,
} from "react";

import {
AccordionGroup,
IconButton,
Textarea,
ToggleButtonGroup,
} from "@mui/joy";

import UnfoldLessIcon from "@mui/icons-material/UnfoldLess";
import UnfoldMoreIcon from "@mui/icons-material/UnfoldMore";

import {StateContext} from "../../../../../contexts/StateContextProvider";
import {
TAB_DISPLAY_NAMES,
TAB_NAME,
} from "../../../../../typings/tab";
import CustomTabPanel from "../CustomTabPanel";
import TitleButton from "../TitleButton";
import ResultsGroup from "./ResultsGroup";


enum SEARCH_OPTION {
IS_CASE_SENSITIVE = "isCaseSensitive",
IS_REGEX = "isRegex"
}

/**
* Displays a panel for submitting search queries and viewing query results.
*
* @return
*/
const SearchTabPanel = () => {
const [isAllExpanded, setIsAllExpanded] = useState<boolean>(true);
const [searchOptions, setSearchOptions] = useState<SEARCH_OPTION[]>([]);
const searchTextRef = useRef<HTMLTextAreaElement>(null);
const {queryResults, startQuery} = useContext(StateContext);
const handleSearch = (event: React.KeyboardEvent<HTMLTextAreaElement>) => {
if ("Enter" === event.key && searchTextRef.current) {
event.preventDefault();
const isCaseSensitive = searchOptions.includes(SEARCH_OPTION.IS_CASE_SENSITIVE);
const isRegex = searchOptions.includes(SEARCH_OPTION.IS_REGEX);
startQuery(searchTextRef.current.value, isRegex, isCaseSensitive);
}
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add performance optimizations and error handling.

Given the reported issues with browser freezing on large result sets, consider implementing:

  1. Debounce the search operation to prevent excessive queries
  2. Add loading state to provide user feedback
  3. Implement error handling for failed searches

Here's a suggested implementation:

+ import { useState, useRef, useContext, useCallback } from 'react';
+ import debounce from 'lodash/debounce';

  const SearchTabPanel = () => {
    const [isAllExpanded, setIsAllExpanded] = useState<boolean>(true);
    const [searchOptions, setSearchOptions] = useState<SEARCH_OPTION[]>([]);
+   const [isLoading, setIsLoading] = useState<boolean>(false);
+   const [error, setError] = useState<string | null>(null);
    const searchTextRef = useRef<HTMLTextAreaElement>(null);
    const {queryResults, startQuery} = useContext(StateContext);

-   const handleSearch = (event: React.KeyboardEvent<HTMLTextAreaElement>) => {
+   const executeSearch = useCallback(async (searchText: string) => {
+     try {
+       setIsLoading(true);
+       setError(null);
        const isCaseSensitive = searchOptions.includes(SEARCH_OPTION.IS_CASE_SENSITIVE);
        const isRegex = searchOptions.includes(SEARCH_OPTION.IS_REGEX);
-       startQuery(searchTextRef.current.value, isRegex, isCaseSensitive);
+       await startQuery(searchText, isRegex, isCaseSensitive);
+     } catch (err) {
+       setError(err instanceof Error ? err.message : 'Search failed');
+     } finally {
+       setIsLoading(false);
+     }
+   }, [searchOptions, startQuery]);

+   const debouncedSearch = useCallback(
+     debounce((searchText: string) => executeSearch(searchText), 300),
+     [executeSearch]
+   );

+   const handleSearch = (event: React.KeyboardEvent<HTMLTextAreaElement>) => {
      if ("Enter" === event.key && searchTextRef.current) {
        event.preventDefault();
+       debouncedSearch(searchTextRef.current.value);
      }
    };
📝 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.

Suggested change
const [isAllExpanded, setIsAllExpanded] = useState<boolean>(true);
const [searchOptions, setSearchOptions] = useState<SEARCH_OPTION[]>([]);
const searchTextRef = useRef<HTMLTextAreaElement>(null);
const {queryResults, startQuery} = useContext(StateContext);
const handleSearch = (event: React.KeyboardEvent<HTMLTextAreaElement>) => {
if ("Enter" === event.key && searchTextRef.current) {
event.preventDefault();
const isCaseSensitive = searchOptions.includes(SEARCH_OPTION.IS_CASE_SENSITIVE);
const isRegex = searchOptions.includes(SEARCH_OPTION.IS_REGEX);
startQuery(searchTextRef.current.value, isRegex, isCaseSensitive);
}
};
const [isAllExpanded, setIsAllExpanded] = useState<boolean>(true);
const [searchOptions, setSearchOptions] = useState<SEARCH_OPTION[]>([]);
const [isLoading, setIsLoading] = useState<boolean>(false);
const [error, setError] = useState<string | null>(null);
const searchTextRef = useRef<HTMLTextAreaElement>(null);
const {queryResults, startQuery} = useContext(StateContext);
const executeSearch = useCallback(async (searchText: string) => {
try {
setIsLoading(true);
setError(null);
const isCaseSensitive = searchOptions.includes(SEARCH_OPTION.IS_CASE_SENSITIVE);
const isRegex = searchOptions.includes(SEARCH_OPTION.IS_REGEX);
await startQuery(searchText, isRegex, isCaseSensitive);
} catch (err) {
setError(err instanceof Error ? err.message : 'Search failed');
} finally {
setIsLoading(false);
}
}, [searchOptions, startQuery]);
const debouncedSearch = useCallback(
debounce((searchText: string) => executeSearch(searchText), 300),
[executeSearch]
);
const handleSearch = (event: React.KeyboardEvent<HTMLTextAreaElement>) => {
if ("Enter" === event.key && searchTextRef.current) {
event.preventDefault();
debouncedSearch(searchTextRef.current.value);
}
};

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enhance search handler with debouncing and error handling.

The current implementation may cause performance issues with large result sets. Consider implementing:

  1. Debounce the search operation
  2. Add loading state
  3. Implement error handling
  4. Validate regex patterns
+import { debounce } from 'lodash';
+
 const SearchTabPanel = () => {
+    const [isLoading, setIsLoading] = useState(false);
+    const [error, setError] = useState<string | null>(null);
+
+    const executeSearch = useCallback(async (
+        searchText: string,
+        isRegex: boolean,
+        isCaseSensitive: boolean
+    ) => {
+        try {
+            if (isRegex) {
+                new RegExp(searchText); // Validate regex
+            }
+            setIsLoading(true);
+            setError(null);
+            await startQuery(searchText, isRegex, isCaseSensitive);
+        } catch (err) {
+            setError(err instanceof Error ? err.message : 'Search failed');
+        } finally {
+            setIsLoading(false);
+        }
+    }, [startQuery]);
+
+    const debouncedSearch = useMemo(
+        () => debounce(executeSearch, 300),
+        [executeSearch]
+    );
+
     const handleSearch = (event: React.KeyboardEvent<HTMLTextAreaElement>) => {
         if ("Enter" === event.key && searchTextRef.current) {
             event.preventDefault();
             const isCaseSensitive = searchOptions.includes(SEARCH_OPTION.IS_CASE_SENSITIVE);
             const isRegex = searchOptions.includes(SEARCH_OPTION.IS_REGEX);
-            startQuery(searchTextRef.current.value, isRegex, isCaseSensitive);
+            debouncedSearch(searchTextRef.current.value, isRegex, isCaseSensitive);
         }
     };

Committable suggestion skipped: line range outside the PR's diff.


return (
<CustomTabPanel
tabName={TAB_NAME.SEARCH}
title={TAB_DISPLAY_NAMES[TAB_NAME.SEARCH]}
titleButtons={
<TitleButton onClick={() => { setIsAllExpanded((v) => !v); }}>
{isAllExpanded ?
<UnfoldLessIcon/> :
<UnfoldMoreIcon/>}
</TitleButton>
}
>
<Textarea
maxRows={7}
placeholder={"Search"}
size={"sm"}
slotProps={{textarea: {ref: searchTextRef}, endDecorator: {sx: {marginBlockStart: 0, display: "block"}}}}

Check warning on line 67 in src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx

View workflow job for this annotation

GitHub Actions / lint-check

This line has a length of 121. Maximum allowed is 100
sx={{flexDirection: "row"}}
endDecorator={
<>
<ToggleButtonGroup
size={"sm"}
spacing={0.3}
sx={{borderRadius: "2px"}}
value={searchOptions}
variant={"plain"}
onChange={(_, newValue) => {
setSearchOptions(newValue);
}}
>
<IconButton
sx={{fontFamily: "Inter"}}
value={SEARCH_OPTION.IS_CASE_SENSITIVE}
>
Aa
</IconButton>
<IconButton
sx={{fontFamily: "Inter"}}
value={SEARCH_OPTION.IS_REGEX}
>
.*
</IconButton>
</ToggleButtonGroup>
</>
}
onKeyDown={handleSearch}/>
<AccordionGroup
disableDivider={true}
size={"sm"}
>
<ResultsGroup
isAllExpanded={isAllExpanded}
queryResults={queryResults}/>
</AccordionGroup>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize rendering performance for large result sets.

To address the re-rendering issues mentioned in the PR comments, consider implementing:

  1. Memoization of the ResultsGroup component
  2. Virtualization for large result sets
+ import { memo } from 'react';
+ import { FixedSizeList } from 'react-window';

- <AccordionGroup
-     disableDivider={true}
-     size={"sm"}
- >
-     <ResultsGroup
-         isAllExpanded={isAllExpanded}
-         queryResults={queryResults}/>
- </AccordionGroup>

+ const MemoizedResultsGroup = memo(ResultsGroup);
+ 
+ <AccordionGroup
+     disableDivider={true}
+     size={"sm"}
+ >
+     <MemoizedResultsGroup
+         isAllExpanded={isAllExpanded}
+         queryResults={queryResults}/>
+ </AccordionGroup>

Consider implementing virtualization if the results can be large:

interface ResultRowProps {
  index: number;
  style: React.CSSProperties;
  data: {
    results: typeof queryResults;
    isExpanded: boolean;
  };
}

const ResultRow = memo(({ index, style, data }: ResultRowProps) => {
  const result = data.results[index];
  return (
    <div style={style}>
      <MemoizedResultsGroup
        isAllExpanded={data.isExpanded}
        queryResults={[result]}
      />
    </div>
  );
});

// In the render method:
<FixedSizeList
  height={400}
  width="100%"
  itemCount={queryResults.length}
  itemSize={50}
  itemData={{ results: queryResults, isExpanded: isAllExpanded }}
>
  {ResultRow}
</FixedSizeList>

</CustomTabPanel>
);
};

export default SearchTabPanel;
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.sidebar-tab-title-button {
min-width: 0 !important;
min-height: 0 !important;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import {
IconButton,
IconButtonProps,
} from "@mui/joy";

import "./TitleButton.css";


/**
* Renders an IconButton with an additional CSS class 'sidebar-tab-title-button'.
*
* @param props
* @return
*/
const TitleButton = (props: IconButtonProps) => (
<IconButton
className={"sidebar-tab-title-button"}
{...props}/>
);

export default TitleButton;
Loading
Loading