feat(settings): Move settings from the modal into a tab panel. #186
Conversation
|
Warning Rate limit exceeded@junhaoliao has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes refactor the sidebar configuration interface by transitioning from a modal-based approach to a tab-based system. Component logic and CSS styling have been updated to integrate a new SettingsTabPanel, while state management now relies on context rather than local state. Import paths have been corrected and new CSS classes are introduced to support the updated layout. Additionally, the legacy SettingsModal and its associated styles have been removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant ST as SidebarTabs
participant SC as StateContext
participant SB as Sidebar
participant STP as SettingsTabPanel
User->>ST: Clicks on a tab (e.g., Settings)
ST->>SC: Calls changeActiveTabName("Settings")
SC-->>SB: Updates activeTabName state
SB->>STP: Renders SettingsTabPanel based on activeTabName
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
This PR depends on #184 |
# Conflicts: # src/components/modals/SettingsModal/SettingsDialog.tsx # src/components/modals/SettingsModal/index.css
…id overflowing the wrapping container.
|
@junhaoliao - is this off draft? if so, lmk and I will look |
Just fixed some overflowing issues. It's now ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx (2)
39-81: Consider adding TypeScript interface for form field configuration.The form field configuration is well-structured, but adding a TypeScript interface would improve type safety and maintainability.
+interface ConfigFormField { + helperText: string | JSX.Element; + initialValue: string | number; + key: LOCAL_STORAGE_KEY; + label: string; + type: 'text' | 'number'; +} -const CONFIG_FORM_FIELDS = [ +const CONFIG_FORM_FIELDS: ConfigFormField[] = [
137-179: Consider grouping related form fields.The form fields could be organized into logical groups using fieldsets for better organization and accessibility.
<Box className={"settings-form-fields-container"}> <ThemeSwitchFormField/> + <fieldset> + <legend>Decoder Settings</legend> {CONFIG_FORM_FIELDS.filter(field => field.key.startsWith('decoderOptions')).map((field, index) => ( <FormControl key={index}> <FormLabel>{field.label}</FormLabel> <Input defaultValue={field.initialValue} name={field.key} type={field.type}/> <FormHelperText>{field.helperText}</FormHelperText> </FormControl> ))} + </fieldset> + <fieldset> + <legend>View Settings</legend> {CONFIG_FORM_FIELDS.filter(field => !field.key.startsWith('decoderOptions')).map((field, index) => ( <FormControl key={index}> <FormLabel>{field.label}</FormLabel> <Input defaultValue={field.initialValue} name={field.key} type={field.type}/> <FormHelperText>{field.helperText}</FormHelperText> </FormControl> ))} + </fieldset> </Box>src/components/CentralContainer/Sidebar/index.tsx (2)
50-54: Consider debouncing the resize handler.To improve performance, consider debouncing the resize handler to prevent excessive state updates.
+import debounce from 'lodash/debounce'; const handleResizeHandleRelease = useCallback(() => { if (getPanelWidth() === tabListRef.current?.clientWidth) { changeActiveTabName(TAB_NAME.NONE); } -}, [changeActiveTabName]); +}, [changeActiveTabName]); + +const debouncedHandleResizeHandleRelease = debounce(handleResizeHandleRelease, 150);
80-97: Consider throttling window resize handler.To improve performance during window resizing, consider throttling the handler.
+import throttle from 'lodash/throttle'; useEffect(() => { - const handleWindowResize = () => { + const handleWindowResize = throttle(() => { const availableWidth = Math.max( window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS, PANEL_CLIP_THRESHOLD_IN_PIXELS ); if (getPanelWidth() > availableWidth) { setPanelWidth(availableWidth); } - }; + }, 100);src/contexts/StateContextProvider.tsx (2)
111-111: Consider using a more type-safe default implementation.Instead of using a null-returning arrow function, consider throwing an error to catch uninitialized context usage:
- changeActiveTabName: () => null, + changeActiveTabName: () => { + throw new Error('StateContext not initialized'); + },
279-292: Consider moving side effects to useEffect.The
setConfigcalls within the state setter function could lead to unnecessary re-renders. Consider moving them to a useEffect:const changeActiveTabName = useCallback((tabName: TAB_NAME) => { setActiveTabName((oldTabName) => { if (oldTabName === tabName) { - setConfig({key: CONFIG_KEY.INITIAL_TAB_NAME, value: TAB_NAME.NONE}); return TAB_NAME.NONE; } - setConfig({key: CONFIG_KEY.INITIAL_TAB_NAME, value: tabName}); return tabName; }); }, []); +useEffect(() => { + setConfig({key: CONFIG_KEY.INITIAL_TAB_NAME, value: activeTabName}); +}, [activeTabName]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/CentralContainer/Sidebar/SidebarTabs/CustomTabPanel.css(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/ThemeSwitchFormField.css(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/ThemeSwitchFormField.tsx(2 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.css(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx(2 hunks)src/components/CentralContainer/Sidebar/index.tsx(3 hunks)src/components/modals/SettingsModal/SettingsDialog.tsx(0 hunks)src/components/modals/SettingsModal/index.css(0 hunks)src/components/modals/SettingsModal/index.tsx(0 hunks)src/contexts/StateContextProvider.tsx(12 hunks)
💤 Files with no reviewable changes (3)
- src/components/modals/SettingsModal/index.tsx
- src/components/modals/SettingsModal/index.css
- src/components/modals/SettingsModal/SettingsDialog.tsx
✅ Files skipped from review due to trivial changes (3)
- src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/ThemeSwitchFormField.css
- src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.css
- src/components/CentralContainer/Sidebar/SidebarTabs/CustomTabPanel.css
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/ThemeSwitchFormField.tsxsrc/components/CentralContainer/Sidebar/index.tsxsrc/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsxsrc/contexts/StateContextProvider.tsxsrc/components/CentralContainer/Sidebar/SidebarTabs/index.tsx
🔇 Additional comments (6)
src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/ThemeSwitchFormField.tsx (3)
15-17: LGTM! Import paths are correctly updated.The import paths have been properly adjusted to reflect the new file location in the directory structure.
25-27: LGTM! Theme handling is well-implemented.The component correctly uses the
useColorSchemehook from MUI Joy for theme management.
33-34: LGTM! UI layout improvement.The addition of the
theme-switch-toggle-button-groupclass improves the layout by enabling button wrapping.src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx (2)
48-52: LGTM! State management is properly migrated to context.The component correctly uses StateContext for tab management, improving state management consistency.
65-99: LGTM! Tab panel integration is well-implemented.The tabs are properly configured with correct orientation and variant, and the SettingsTabPanel is well-integrated.
src/components/CentralContainer/Sidebar/index.tsx (1)
99-120: LGTM! Tab state handling is well-implemented.The effect properly handles panel width updates based on the active tab state, including edge cases.
| const handleConfigFormReset = (ev: React.FormEvent) => { | ||
| ev.preventDefault(); | ||
| window.localStorage.clear(); | ||
| window.location.reload(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider selective localStorage clearing.
The current implementation clears all localStorage data. Consider clearing only configuration-related keys to preserve other application data.
- window.localStorage.clear();
+ Object.values(LOCAL_STORAGE_KEY).forEach(key => {
+ window.localStorage.removeItem(key);
+ });📝 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.
| const handleConfigFormReset = (ev: React.FormEvent) => { | |
| ev.preventDefault(); | |
| window.localStorage.clear(); | |
| window.location.reload(); | |
| }; | |
| const handleConfigFormReset = (ev: React.FormEvent) => { | |
| ev.preventDefault(); | |
| Object.values(LOCAL_STORAGE_KEY).forEach(key => { | |
| window.localStorage.removeItem(key); | |
| }); | |
| window.location.reload(); | |
| }; |
| const handleConfigFormSubmit = useCallback( | ||
| (ev: React.FormEvent) => { | ||
| ev.preventDefault(); | ||
| const formData = new FormData(ev.target as HTMLFormElement); | ||
| const getFormDataValue = (key: string) => formData.get(key) as string; | ||
|
|
||
| const formatString = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_FORMAT_STRING); | ||
| const logLevelKey = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_LOG_LEVEL_KEY); | ||
| const timestampKey = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_TIMESTAMP_KEY); | ||
| const pageSize = getFormDataValue(LOCAL_STORAGE_KEY.PAGE_SIZE); | ||
|
|
||
| let error: Nullable<string> = null; | ||
| error ||= setConfig({ | ||
| key: CONFIG_KEY.DECODER_OPTIONS, | ||
| value: {formatString, logLevelKey, timestampKey}, | ||
| }); | ||
| error ||= setConfig({ | ||
| key: CONFIG_KEY.PAGE_SIZE, | ||
| value: Number(pageSize), | ||
| }); | ||
|
|
||
| if (null !== error) { | ||
| postPopUp({ | ||
| level: LOG_LEVEL.ERROR, | ||
| message: error, | ||
| timeoutMillis: DO_NOT_TIMEOUT_VALUE, | ||
| title: "Unable to apply config.", | ||
| }); | ||
| } else { | ||
| window.location.reload(); | ||
| } | ||
| }, | ||
| [postPopUp], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation before applying configuration.
Consider validating the form data before applying the configuration to prevent invalid values.
const handleConfigFormSubmit = useCallback(
(ev: React.FormEvent) => {
ev.preventDefault();
const formData = new FormData(ev.target as HTMLFormElement);
const getFormDataValue = (key: string) => formData.get(key) as string;
const formatString = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_FORMAT_STRING);
const logLevelKey = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_LOG_LEVEL_KEY);
const timestampKey = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_TIMESTAMP_KEY);
const pageSize = getFormDataValue(LOCAL_STORAGE_KEY.PAGE_SIZE);
+ // Validate page size
+ const pageSizeNum = Number(pageSize);
+ if (isNaN(pageSizeNum) || pageSizeNum <= 0) {
+ postPopUp({
+ level: LOG_LEVEL.ERROR,
+ message: "Page size must be a positive number",
+ timeoutMillis: DO_NOT_TIMEOUT_VALUE,
+ title: "Invalid configuration",
+ });
+ return;
+ }📝 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.
| const handleConfigFormSubmit = useCallback( | |
| (ev: React.FormEvent) => { | |
| ev.preventDefault(); | |
| const formData = new FormData(ev.target as HTMLFormElement); | |
| const getFormDataValue = (key: string) => formData.get(key) as string; | |
| const formatString = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_FORMAT_STRING); | |
| const logLevelKey = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_LOG_LEVEL_KEY); | |
| const timestampKey = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_TIMESTAMP_KEY); | |
| const pageSize = getFormDataValue(LOCAL_STORAGE_KEY.PAGE_SIZE); | |
| let error: Nullable<string> = null; | |
| error ||= setConfig({ | |
| key: CONFIG_KEY.DECODER_OPTIONS, | |
| value: {formatString, logLevelKey, timestampKey}, | |
| }); | |
| error ||= setConfig({ | |
| key: CONFIG_KEY.PAGE_SIZE, | |
| value: Number(pageSize), | |
| }); | |
| if (null !== error) { | |
| postPopUp({ | |
| level: LOG_LEVEL.ERROR, | |
| message: error, | |
| timeoutMillis: DO_NOT_TIMEOUT_VALUE, | |
| title: "Unable to apply config.", | |
| }); | |
| } else { | |
| window.location.reload(); | |
| } | |
| }, | |
| [postPopUp], | |
| ); | |
| const handleConfigFormSubmit = useCallback( | |
| (ev: React.FormEvent) => { | |
| ev.preventDefault(); | |
| const formData = new FormData(ev.target as HTMLFormElement); | |
| const getFormDataValue = (key: string) => formData.get(key) as string; | |
| const formatString = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_FORMAT_STRING); | |
| const logLevelKey = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_LOG_LEVEL_KEY); | |
| const timestampKey = getFormDataValue(LOCAL_STORAGE_KEY.DECODER_OPTIONS_TIMESTAMP_KEY); | |
| const pageSize = getFormDataValue(LOCAL_STORAGE_KEY.PAGE_SIZE); | |
| // Validate page size | |
| const pageSizeNum = Number(pageSize); | |
| if (isNaN(pageSizeNum) || pageSizeNum <= 0) { | |
| postPopUp({ | |
| level: LOG_LEVEL.ERROR, | |
| message: "Page size must be a positive number", | |
| timeoutMillis: DO_NOT_TIMEOUT_VALUE, | |
| title: "Invalid configuration", | |
| }); | |
| return; | |
| } | |
| let error: Nullable<string> = null; | |
| error ||= setConfig({ | |
| key: CONFIG_KEY.DECODER_OPTIONS, | |
| value: { formatString, logLevelKey, timestampKey }, | |
| }); | |
| error ||= setConfig({ | |
| key: CONFIG_KEY.PAGE_SIZE, | |
| value: Number(pageSize), | |
| }); | |
| if (null !== error) { | |
| postPopUp({ | |
| level: LOG_LEVEL.ERROR, | |
| message: error, | |
| timeoutMillis: DO_NOT_TIMEOUT_VALUE, | |
| title: "Unable to apply config.", | |
| }); | |
| } else { | |
| window.location.reload(); | |
| } | |
| }, | |
| [postPopUp], | |
| ); |
| const handleTabButtonClick = (tabName: TAB_NAME) => { | ||
| switch (tabName) { | ||
| case TAB_NAME.SETTINGS: | ||
| setIsSettingsModalOpen(true); | ||
| break; | ||
| case TAB_NAME.DOCUMENTATION: | ||
| openInNewTab(DOCUMENTATION_URL); | ||
| break; | ||
| default: | ||
| onActiveTabNameChange(tabName); | ||
| changeActiveTabName(tabName); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security attributes for external links.
When opening documentation in a new tab, consider adding security-related attributes.
case TAB_NAME.DOCUMENTATION:
- openInNewTab(DOCUMENTATION_URL);
+ openInNewTab(DOCUMENTATION_URL, { rel: 'noopener noreferrer' });
break;📝 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.
| const handleTabButtonClick = (tabName: TAB_NAME) => { | |
| switch (tabName) { | |
| case TAB_NAME.SETTINGS: | |
| setIsSettingsModalOpen(true); | |
| break; | |
| case TAB_NAME.DOCUMENTATION: | |
| openInNewTab(DOCUMENTATION_URL); | |
| break; | |
| default: | |
| onActiveTabNameChange(tabName); | |
| changeActiveTabName(tabName); | |
| } | |
| }; | |
| const handleTabButtonClick = (tabName: TAB_NAME) => { | |
| switch (tabName) { | |
| case TAB_NAME.DOCUMENTATION: | |
| openInNewTab(DOCUMENTATION_URL, { rel: 'noopener noreferrer' }); | |
| break; | |
| default: | |
| changeActiveTabName(tabName); | |
| } | |
| }; |
| activeTabName, | ||
| onActiveTabNameChange, | ||
| }, | ||
| const SidebarTabs = forwardRef<HTMLDivElement>(( |
There was a problem hiding this comment.
Should probably just fix this now? it's deprecated
https://react.dev/reference/react/forwardRef
| @@ -297,7 +313,7 @@ const StateContextProvider = ({children}: StateContextProviderProps) => { | |||
| children: "Settings", | |||
| startDecorator: <SettingsOutlinedIcon/>, | |||
| onClick: () => { | |||
| setIsSettingsModalOpen(true); | |||
| changeActiveTabName(TAB_NAME.SETTINGS); | |||
There was a problem hiding this comment.
can we just call setActiveTabName here, and get rid of changeActiveTabName? We could then not delete exisitng handleActiveTabNameChange in src/components/CentralContainer/Sidebar/index.tsx
There was a problem hiding this comment.
I feel like the less in StateContextProvider the better
There was a problem hiding this comment.
We do need to save the last active tab name into localStorage and I feel it's best if we can save the name whenever a tab name change is request, to avoid duplicate code. If the function is not purely state settings, i want to avoid naming it with the set prefix to reduce confusion for code readers.
That said, an attempt was made to remove the toggling on same tab name logic from changeActiveTabName, which also fixes a bug in this pop up, where an active settings tab can be closed once the user click this primary action.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx (1)
59-61:⚠️ Potential issueAdd security attributes for external links.
When opening documentation in a new tab, consider adding security-related attributes.
Apply this diff:
case TAB_NAME.DOCUMENTATION: - openInNewTab(DOCUMENTATION_URL); + openInNewTab(DOCUMENTATION_URL, { rel: 'noopener noreferrer' }); break;
🧹 Nitpick comments (1)
src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx (1)
28-28: Move hardcoded URL to configuration file.Consider moving the
DOCUMENTATION_URLto a configuration file for better maintainability and centralized management of external URLs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx
🧠 Learnings (1)
src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx (1)
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#74
File: new-log-viewer/src/components/SidebarContainer/index.tsx:107-107
Timestamp: 2024-11-10T16:46:58.543Z
Learning: The `PanelTabs` component is already wrapped with `React.forwardRef` and passes the `ref` to the appropriate child element.
🔇 Additional comments (1)
src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx (1)
67-102: Well-structured tab implementation with proper state management.The tab implementation effectively uses context for state management and maintains a clean separation between tab buttons and panels. The structure aligns well with the PR objective of moving settings into a tab panel.
| const SidebarTabs = ({ | ||
| ref, | ||
| }: SidebarTabsProps) => { |
There was a problem hiding this comment.
Restore forwardRef wrapper.
Based on the retrieved learnings, this component should be wrapped with React.forwardRef. The current implementation might break ref forwarding functionality.
Apply this diff to restore the forwardRef wrapper:
-const SidebarTabs = ({
- ref,
-}: SidebarTabsProps) => {
+const SidebarTabs = forwardRef<HTMLDivElement, SidebarTabsProps>(({}, ref) => {📝 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.
| const SidebarTabs = ({ | |
| ref, | |
| }: SidebarTabsProps) => { | |
| -const SidebarTabs = ({ | |
| - ref, | |
| -}: SidebarTabsProps) => { | |
| +const SidebarTabs = forwardRef<HTMLDivElement, SidebarTabsProps>(({}, ref) => { |
# Conflicts: # src/components/modals/SettingsModal/SettingsDialog.tsx
Description
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Style
Refactor