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

new-log-viewer: Integrate latest clp-ffi-js with support for log-level filtering; Add log-level selector to StatusBar. #77

Merged
merged 51 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
0e36cd9
Add log level selector component to StatusBar.
junhaoliao Sep 26, 2024
c25499f
Add "Log Level" label as a Chip to LogLevelSelect component.
junhaoliao Sep 26, 2024
c7666e1
Rename parameter in LogLevelChip's onSelectedLogLevelsChange callback.
junhaoliao Sep 26, 2024
578f42a
Replace inline style with a CSS class for the dummy MenuItem.
junhaoliao Sep 26, 2024
62b67d5
Add comparator to .sort() - Apply suggestions from code review
junhaoliao Sep 26, 2024
12ecbc6
Change select box behaviour to always enable the selected log level a…
junhaoliao Sep 30, 2024
e199006
Do not specify LogLevelChip tooltip color based on the log level; add…
junhaoliao Sep 30, 2024
d8b68d3
Limit LogLevelSelect listbox width.
junhaoliao Sep 30, 2024
6eb3ed8
Remove log-level deselection handling on the chips.
junhaoliao Sep 30, 2024
d1400f4
Rename log level `NONE` -> `UNKNOWN`; Add an "ALL" option to clear fi…
junhaoliao Sep 30, 2024
9149712
Add checkbox to LogLevelSelect to allow individual selections.
junhaoliao Sep 30, 2024
a55c600
Specify listbox placement to avoid the listbox moving as the select b…
junhaoliao Sep 30, 2024
f2a4096
Set max-width directly to 0 to prevent auto-resizing with the Select …
junhaoliao Sep 30, 2024
4d0f7c8
Fix an issue where clicking on some Option edge deselects all options.
junhaoliao Sep 30, 2024
a590f2b
Extract `PlacementLeftTooltip` and `PlacementRightTooltip` components…
junhaoliao Sep 30, 2024
e35d7cc
Refactor Tooltip components to use destructuring.
junhaoliao Sep 30, 2024
da65862
Extract components from LogLevelSelect.
junhaoliao Sep 30, 2024
e9e9afb
Rename `LOG_LEVEL.NONE` -> `LOG_LEVEL.UNKNOWN`.
junhaoliao Sep 30, 2024
682b01d
In the checkbox tooltip, place +/- icon on the left of the LEVEL name.
junhaoliao Sep 30, 2024
12ef8ad
Rename "ALL" option to "Clear filters"; move "Clear filters" to the t…
junhaoliao Sep 30, 2024
67dae77
Revert tsconfig changes now that we don't need to use `Array.toRevers…
junhaoliao Sep 30, 2024
22061bb
Move LogLevelSelect to the right of the LogEventNum button in the sta…
junhaoliao Sep 30, 2024
a60ba66
Remove unused React import from LogLevelChip.tsx.
junhaoliao Oct 1, 2024
a8972f5
Mitigate a rounding issue in JoyUI when calculating the listbox posit…
junhaoliao Oct 1, 2024
31c538f
Remove Roboto Mono font and adjust LogLevelChip styles.
junhaoliao Oct 1, 2024
288a2e8
Docs - Apply suggestions from code review
junhaoliao Oct 1, 2024
ce6b038
Docs & Reformat - Apply suggestions from code review
junhaoliao Oct 1, 2024
5309671
Document one-argument behaviour in `range`.
junhaoliao Oct 1, 2024
1f7071d
Inline PlacementXXXTooltip components.
junhaoliao Oct 1, 2024
7259067
Remove extraneous onMouseDown event handler.
junhaoliao Oct 1, 2024
da171dd
Change ClearFiltersOption value to null.
junhaoliao Oct 1, 2024
bfe6c0e
Remove colon from Log Event button label.
junhaoliao Oct 1, 2024
fae7441
Move the declaration of the `result` array closer to its usage within…
junhaoliao Oct 1, 2024
91ae520
Refactor `range` function parameter names `start` -> ``begin`, `stop`…
junhaoliao Oct 1, 2024
3b34d52
Refactor `range` function signature to better support varied argument…
junhaoliao Oct 1, 2024
d7e1352
Attach the select change handler to the <Option/>s instead; rename th…
junhaoliao Oct 1, 2024
a8e9147
Add check for undefined data-value attribute in LogLevelSelect.
junhaoliao Oct 1, 2024
b8b0682
Docs - Apply suggestions from code review
junhaoliao Oct 1, 2024
2d64c6c
Change log level select clear option value to INVALID_LOG_LEVEL_VALUE.
junhaoliao Oct 1, 2024
4ad92f4
Change "TRACE" & "DEBUG" chips' color to "success".
junhaoliao Oct 1, 2024
6d8e98a
Remove commented out code.
junhaoliao Oct 1, 2024
fe1c3b3
Merge branch 'main' into log-level-select
junhaoliao Oct 3, 2024
dffcce2
Change `LOG_LEVEL.NONE` to `LOG_LEVEL.UNKNOWN`.
junhaoliao Oct 3, 2024
03b40c0
Stop propagation from checkbox click to the option.
junhaoliao Oct 5, 2024
c634ea2
Merge remote-tracking branch 'junhao/log-level-select' into log-level…
junhaoliao Oct 5, 2024
b76ebef
Remove JsonlDecoder.ts as it should have been done when merging from …
junhaoliao Oct 5, 2024
22ab6da
Add / remove empty lines - Apply suggestions from code review
junhaoliao Oct 7, 2024
7a0989f
Merge branch 'main' into HEAD
davemarco Oct 11, 2024
aa064c1
tie button and add ir support
davemarco Oct 11, 2024
ec194cd
fix lint
davemarco Oct 11, 2024
4b412fd
fix seizure
davemarco Oct 11, 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
8 changes: 4 additions & 4 deletions new-log-viewer/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 1 addition & 2 deletions new-log-viewer/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
"scripts": {
"build": "webpack --config webpack.prod.js",
"start": "webpack serve --open --config webpack.dev.js",

"lint": "npm run lint:check",
"lint:check": "npm-run-all --sequential --continue-on-error lint:check:*",
"lint:check:css": "stylelint src/**/*.css",
Expand All @@ -30,7 +29,7 @@
"@mui/icons-material": "^6.1.0",
"@mui/joy": "^5.0.0-beta.48",
"axios": "^1.7.2",
"clp-ffi-js": "^0.1.0",
"clp-ffi-js": "^0.2.0",
"dayjs": "^1.11.11",
"monaco-editor": "^0.50.0",
"react": "^18.3.1",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
.log-level-chip {
/* stylelint-disable-next-line custom-property-pattern */
--Chip-radius: 0;
}

.log-level-chip span {
width: 1.4ch;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import {
Chip,
Tooltip,
} from "@mui/joy";
import {DefaultColorPalette} from "@mui/joy/styles/types/colorSystem";

import {LOG_LEVEL} from "../../../typings/logs";

import "./LogLevelChip.css";


/**
* Maps log levels to colors from JoyUI's default color palette.
*/
const LOG_LEVEL_COLOR_MAP: Record<LOG_LEVEL, DefaultColorPalette> = Object.freeze({
[LOG_LEVEL.UNKNOWN]: "neutral",
[LOG_LEVEL.TRACE]: "success",
[LOG_LEVEL.DEBUG]: "success",
[LOG_LEVEL.INFO]: "primary",
[LOG_LEVEL.WARN]: "warning",
[LOG_LEVEL.ERROR]: "danger",
[LOG_LEVEL.FATAL]: "danger",
});

interface LogLevelChipProps {
name: string,
value: LOG_LEVEL,
}

/**
* Renders a log level chip.
*
* @param props
* @param props.name
* @param props.value
* @return
*/
const LogLevelChip = ({name, value}: LogLevelChipProps) => (
<Tooltip
key={value}
title={name}
>
<Chip
className={"log-level-chip"}
color={LOG_LEVEL_COLOR_MAP[value]}
variant={"outlined"}
>
{name[0]}
</Chip>
</Tooltip>
);


export default LogLevelChip;
junhaoliao marked this conversation as resolved.
Show resolved Hide resolved
21 changes: 21 additions & 0 deletions new-log-viewer/src/components/StatusBar/LogLevelSelect/index.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
.log-level-select {
/* JoyUI has a rounding issue when calculating the listbox position, causing it to misjudge if
the listbox will fit on the right side. To mitigate this, we shift the select box 1px to the
left. */
margin-right: 1px;
}

.log-level-select-render-value-box {
display: flex;
gap: 2px;
}

.log-level-select-render-value-box-label {
/* Disable `Chip`'s background style. */
background-color: initial !important;
junhaoliao marked this conversation as resolved.
Show resolved Hide resolved
}

.log-level-select-listbox {
/* Disallow width auto-resizing with the `Select` button. */
max-width: 0;
}
252 changes: 252 additions & 0 deletions new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,252 @@
import React, {
useCallback,
useContext,
useEffect,
useState,
} from "react";

import {SelectValue} from "@mui/base/useSelect";
import {
Box,
Checkbox,
Chip,
IconButton,
ListItemContent,
ListItemDecorator,
Option,
Select,
SelectOption,
Stack,
Tooltip,
} from "@mui/joy";

import AddIcon from "@mui/icons-material/Add";
import CloseIcon from "@mui/icons-material/Close";
import KeyboardArrowUpIcon from "@mui/icons-material/KeyboardArrowUp";
import RemoveIcon from "@mui/icons-material/Remove";

import {StateContext} from "../../../contexts/StateContextProvider";
import {
INVALID_LOG_LEVEL_VALUE,
LOG_LEVEL,
LOG_LEVEL_NAMES,
MAX_LOG_LEVEL,
} from "../../../typings/logs";
import {range} from "../../../utils/data";
import LogLevelChip from "./LogLevelChip";

import "./index.css";


interface LogSelectOptionProps {
isChecked: boolean,
logLevelName: string,
logLevelValue: LOG_LEVEL,
onCheckboxClick: React.MouseEventHandler
onOptionClick: React.MouseEventHandler
}

/**
* Renders an <Option/> in the <LogLevelSelect/> for selecting some log level and/or the levels
* above it.
*
* @param props
* @param props.isChecked
* @param props.logLevelName
* @param props.logLevelValue
* @param props.onCheckboxClick
* @param props.onOptionClick
* @return
*/
const LogSelectOption = ({
isChecked,
logLevelName,
logLevelValue,
onCheckboxClick,
onOptionClick,
}: LogSelectOptionProps) => {
return (
<Option
data-value={logLevelValue}
key={logLevelName}
value={logLevelValue}
onClick={onOptionClick}
>
<ListItemDecorator>
<Tooltip
placement={"left"}
title={
<Stack
alignItems={"center"}
direction={"row"}
>
{isChecked ?
<RemoveIcon/> :
<AddIcon/>}
{logLevelName}
</Stack>
}
>
<Checkbox
checked={isChecked}
size={"sm"}
value={logLevelValue}
onClick={onCheckboxClick}/>
</Tooltip>
</ListItemDecorator>
<Tooltip
placement={"left"}
title={
<Stack
alignItems={"center"}
direction={"row"}
>
{logLevelName}
{" and below"}
</Stack>
}
>
<ListItemContent>
{logLevelName}
</ListItemContent>
</Tooltip>
</Option>
);
};
Comment on lines +41 to +115
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance accessibility and optimize performance of LogSelectOption

The LogSelectOption component is well-structured, but consider the following improvements:

  1. Accessibility: Add an aria-label to the Checkbox for screen readers.
  2. Performance: Memoize the component using React.memo to prevent unnecessary re-renders.

Here's a suggested implementation:

import React, { memo } from 'react';

const LogSelectOption = memo(({
    isChecked,
    logLevelName,
    logLevelValue,
    onCheckboxClick,
    onOptionClick,
}: LogSelectOptionProps) => {
    return (
        <Option
            data-value={logLevelValue}
            key={logLevelName}
            value={logLevelValue}
            onClick={onOptionClick}
        >
            <ListItemDecorator>
                <Tooltip
                    placement="left"
                    title={
                        <Stack alignItems="center" direction="row">
                            {isChecked ? <RemoveIcon /> : <AddIcon />}
                            {logLevelName}
                        </Stack>
                    }
                >
                    <Checkbox
                        checked={isChecked}
                        size="sm"
                        value={logLevelValue}
                        onClick={onCheckboxClick}
                        aria-label={`Select ${logLevelName} log level`}
                    />
                </Tooltip>
            </ListItemDecorator>
            <Tooltip
                placement="left"
                title={
                    <Stack alignItems="center" direction="row">
                        {logLevelName} and below
                    </Stack>
                }
            >
                <ListItemContent>{logLevelName}</ListItemContent>
            </Tooltip>
        </Option>
    );
});

LogSelectOption.displayName = 'LogSelectOption';

These changes will improve accessibility for screen reader users and potentially boost performance by reducing unnecessary re-renders.


interface ClearFiltersOptionProps {
onClick: () => void
}

/**
* Renders an <Option/> to clear all filters in the <LogLevelSelect/>.
*
* @param props
* @param props.onClick
* @return
*/
const ClearFiltersOption = ({onClick}: ClearFiltersOptionProps) => (
<Option
value={INVALID_LOG_LEVEL_VALUE}
onClick={onClick}
>
<ListItemDecorator>
<CloseIcon/>
</ListItemDecorator>
Clear filters
</Option>
);

/**
* Renders a dropdown box for selecting log levels.
*
* @return
*/
const LogLevelSelect = () => {
const [selectedLogLevels, setSelectedLogLevels] = useState<LOG_LEVEL[]>([]);
const {setLogLevelFilter} = useContext(StateContext);

const handleRenderValue = (selected: SelectValue<SelectOption<LOG_LEVEL>, true>) => (
<Box className={"log-level-select-render-value-box"}>
<Chip className={"log-level-select-render-value-box-label"}>
Log Level
</Chip>
{selected.map((selectedOption) => (
<LogLevelChip
key={selectedOption.value}
name={selectedOption.label as string}
value={selectedOption.value}/>
))}
</Box>
);

const handleCheckboxClick = useCallback((ev: React.MouseEvent<HTMLInputElement>) => {
ev.preventDefault();
ev.stopPropagation();

const target = ev.target as HTMLInputElement;
const value = Number(target.value) as LOG_LEVEL;
let newSelectedLogLevels: LOG_LEVEL[];
if (selectedLogLevels.includes(value)) {
newSelectedLogLevels = selectedLogLevels.filter((logLevel) => logLevel !== value);
} else {
newSelectedLogLevels = [
...selectedLogLevels,
value,
];
}
setSelectedLogLevels(newSelectedLogLevels.sort((a, b) => a - b));
}, [selectedLogLevels]);
Comment on lines +163 to +179
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 state updates in handleCheckboxClick

Consider using the functional update form of setSelectedLogLevels to optimize this function:

const handleCheckboxClick = useCallback((ev: React.MouseEvent<HTMLInputElement>) => {
    ev.preventDefault();
    ev.stopPropagation();

    const target = ev.target as HTMLInputElement;
    const value = Number(target.value) as LOG_LEVEL;
    
    setSelectedLogLevels(prevSelected => {
        const newSelected = prevSelected.includes(value)
            ? prevSelected.filter(logLevel => logLevel !== value)
            : [...prevSelected, value];
        return newSelected.sort((a, b) => a - b);
    });
}, []);

This approach ensures that we're always working with the most up-to-date state and reduces the risk of stale state issues.


const handleOptionClick = useCallback((ev: React.MouseEvent) => {
const currentTarget = ev.currentTarget as HTMLElement;
if ("undefined" === typeof currentTarget.dataset.value) {
console.error("Unexpected undefined value for \"data-value\" attribute");

return;
}

const selectedValue = Number(currentTarget.dataset.value);
setSelectedLogLevels(range({begin: selectedValue, end: 1 + MAX_LOG_LEVEL}));
}, []);
Comment on lines +181 to +191
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling in handleOptionClick

The current error handling in handleOptionClick logs to the console but doesn't provide user feedback. Consider throwing an error or using a more robust error handling mechanism:

const handleOptionClick = useCallback((ev: React.MouseEvent) => {
    const currentTarget = ev.currentTarget as HTMLElement;
    const dataValue = currentTarget.dataset.value;
    
    if (typeof dataValue === 'undefined') {
        throw new Error("Unexpected undefined value for 'data-value' attribute");
    }

    const selectedValue = Number(dataValue);
    setSelectedLogLevels(range({begin: selectedValue, end: 1 + MAX_LOG_LEVEL}));
}, []);

This change ensures that errors are caught and can be handled appropriately by an error boundary or other error handling mechanism in your application.


const handleSelectClearButtonClick = () => {
setSelectedLogLevels([]);
};

useEffect(() => {
setLogLevelFilter((0 === selectedLogLevels.length ?
null :
selectedLogLevels));
}, [
setLogLevelFilter,
selectedLogLevels,
]);

return (
<Select
className={"log-level-select"}
multiple={true}
renderValue={handleRenderValue}
size={"sm"}
value={selectedLogLevels}
variant={"soft"}
indicator={0 === selectedLogLevels.length ?
<KeyboardArrowUpIcon/> :
<Tooltip title={"Clear filters"}>
<IconButton
variant={"plain"}
onClick={handleSelectClearButtonClick}
>
<CloseIcon/>
</IconButton>
</Tooltip>}
placeholder={
<Chip className={"log-level-select-render-value-box-label"}>
Log Level
</Chip>
}
slotProps={{
listbox: {
className: "log-level-select-listbox",
placement: "top-end",
},
}}
>
<ClearFiltersOption onClick={handleSelectClearButtonClick}/>
{LOG_LEVEL_NAMES.map((logLevelName, logLevelValue) => {
const checked = selectedLogLevels.includes(logLevelValue);
return (
<LogSelectOption
isChecked={checked}
key={logLevelName}
logLevelName={logLevelName}
logLevelValue={logLevelValue}
onCheckboxClick={handleCheckboxClick}
onOptionClick={handleOptionClick}/>
);
})}
</Select>
);
Comment on lines +206 to +250
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider performance optimization for Select component

The Select component is re-rendering on every state change. To optimize performance, consider memoizing the options:

const memoizedOptions = useMemo(() => (
    LOG_LEVEL_NAMES.map((logLevelName, logLevelValue) => {
        const checked = selectedLogLevels.includes(logLevelValue);
        return (
            <LogSelectOption
                isChecked={checked}
                key={logLevelName}
                logLevelName={logLevelName}
                logLevelValue={logLevelValue}
                onCheckboxClick={handleCheckboxClick}
                onOptionClick={handleOptionClick}
            />
        );
    })
), [selectedLogLevels, handleCheckboxClick, handleOptionClick]);

// In the return statement
{memoizedOptions}

This optimization prevents unnecessary re-renders of the options when the selected log levels haven't changed.

};
export default LogLevelSelect;
1 change: 1 addition & 0 deletions new-log-viewer/src/components/StatusBar/index.css
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
bottom: 0;

display: flex;
flex-wrap: wrap;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is to avoid the individual components in the status bar squeezing themselves when the browser window is too narrow.

One may not like that the components are wrapped outside of the viewport so that a scroll-down becomes necessary to access the wrapped components. If so, an issue can be submitted to keep track of the behaviour and we can look for a better solution in the future.

align-items: center;

width: 100%;
Expand Down
Loading
Loading