feat(settings): Move settings from the modal into a tab panel. #186
Conversation
WalkthroughThe changes refactor the sidebar settings interface by replacing the modal-based approach with an integrated tab-based design. Several CSS files were updated or added to support new UI components, and import paths were modified accordingly. Component logic in the sidebar and context handling was streamlined, with the introduction of an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarTabs
participant StateContext
participant SettingsTabPanel
User->>SidebarTabs: Clicks on settings tab
SidebarTabs->>StateContext: Call changeActiveTabName("settings")
StateContext-->>SidebarTabs: Active tab updated
SidebarTabs->>SettingsTabPanel: Render settings form
SettingsTabPanel->>User: Display configuration options
Suggested reviewers
✨ Finishing Touches
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.
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) => { |
Description
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Style
Refactor