feat: dark theme support #148
Conversation
|
@AmitChauhan63390 looks great, just take a look at discord to make some fixes |
|
Did as you told me on discord. Hope you like it |
|
@RohitR311 do take a look |
rob1.mp4 |
There was a problem hiding this comment.
at a lot of places, the theme has been reset to default MUI theme. @RohitR311 can you take it from here
|
Warning Rate limit exceeded@AmitChauhan63390 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 48 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. WalkthroughThe changes in this pull request primarily focus on enhancing the theming capabilities of the application. The Changes
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (25)
src/components/molecules/RunContent.tsx (1)
78-121: Refactor tab styling to improve maintainability and consistencyWhile the dark theme implementation looks good, there are a few improvements that could enhance maintainability:
- Use theme tokens instead of hardcoded colors
- Extract duplicate tab styles to reduce code duplication
- Consider adding smooth transitions for hover effects
Here's a suggested refactor:
<Tabs value={tab} onChange={(e, newTab) => setTab(newTab)} aria-label="run-content-tabs" sx={{ '& .MuiTabs-indicator': { - backgroundColor: '#FF00C3', + backgroundColor: theme => theme.palette.primary.main, }, '& .MuiTab-root': { '&.Mui-selected': { - color: '#FF00C3', + color: theme => theme.palette.primary.main, }, } }} > + {[ + { label: "Output Data", value: 'output' }, + { label: "Log", value: 'log' } + ].map(({ label, value }) => ( <Tab - label="Output Data" - value='output' + key={value} + label={label} + value={value} sx={{ color: (theme) => theme.palette.mode === 'dark' ? '#fff' : '#000', '&:hover': { - color: '#FF00C3' + color: theme => theme.palette.primary.main, + transition: theme => theme.transitions.create('color'), }, '&.Mui-selected': { - color: '#FF00C3', + color: theme => theme.palette.primary.main, } }} /> + ))} - <Tab - label="Log" - value='log' - sx={{...}} // Removed duplicate styling - />src/components/molecules/InterpretationLog.tsx (1)
127-129: Consider using theme tokens for colors and removing empty lineA few suggestions to improve the styling:
- Replace the hard-coded color
#ff00c3with a theme token for better maintainability and theme consistency- Remove the empty line in the styling object (line 129)
sx={{ borderRadius: ' 0 0 10px 10px', - color: 'white', position: 'absolute', - background: '#ff00c3', + background: theme.palette.primary.main, border: 'none', padding: '10px 20px', width: '900px', overflow: 'hidden', textAlign: 'left', justifyContent: 'flex-start', '&:hover': { - backgroundColor: '#ff00c3', + backgroundColor: theme.palette.primary.dark, }, }}src/components/organisms/BrowserContent.tsx (3)
143-143: Move inline styles to styled componentConsider moving the inline styles to the
BrowserContentWrapperstyled component for better maintainability and consistency with the project's styling approach.-<div id="browser" style={{ display: "flex", flexDirection: "column" }} > +<div id="browser">Then add to BrowserContentWrapper:
const BrowserContentWrapper = styled.div` position: relative; width: 100vw; height: 100vh; overflow: hidden; + display: flex; + flex-direction: column; `;
163-167: Consider theme-aware styling and mobile viewport handlingTwo suggestions for improvement:
- Since this PR implements dark theme support, consider adding theme-aware styling:
-const BrowserContentWrapper = styled.div` +const BrowserContentWrapper = styled.div<{ theme: Theme }>` position: relative; width: 100vw; height: 100vh; overflow: hidden; + background-color: ${({ theme }) => theme.palette.background.default}; + color: ${({ theme }) => theme.palette.text.primary}; `;
- The use of
100vhcan cause issues on mobile browsers due to dynamic viewport height behavior with toolbars. Consider using alternative approaches:const BrowserContentWrapper = styled.div` position: relative; width: 100vw; - height: 100vh; + height: 100%; + min-height: 100vh; + min-height: 100dvh; /* dynamic viewport height */ overflow: hidden; `;
Theme implementation needs attention in browser components
The verification reveals several components that need theme-related updates:
- BrowserWindow uses inline styles without theme integration
- BrowserTabs imports useThemeMode but doesn't appear to use it for styling
- BrowserContent uses basic styling without theme consideration
Specific locations requiring theme implementation:
- BrowserWindow's container div and modal styles should use theme colors
- BrowserTabs should implement theme-aware styled components
- BrowserContent's container should use theme-aware styling for consistent dark mode support
🔗 Analysis chain
Line range hint
18-161: Verify theme implementation completenessGiven that this PR implements dark theme support, we should verify if this component needs additional theme-related changes for child components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if BrowserNavBar and BrowserWindow components implement theme support ast-grep --pattern 'const $_ = styled.$_` $$$ ${({ theme }) => $$$} $$$ `' src/components/molecules/BrowserNavBar.tsx src/components/organisms/BrowserWindow.tsx # Check for useTheme or useThemeMode hook usage rg -l "useTheme|useThemeMode" src/components/molecules/BrowserNavBar.tsx src/components/organisms/BrowserWindow.tsxLength of output: 329
Script:
#!/bin/bash # Let's check the actual implementation of BrowserNavBar and BrowserWindow # to see their theme-related code # Check BrowserNavBar implementation rg -A 5 "useTheme|theme" src/components/molecules/BrowserNavBar.tsx # Check BrowserWindow styled components and theme usage rg -A 5 "styled\.|theme" src/components/organisms/BrowserWindow.tsx # Check if there are any theme-related styles in BrowserTabs rg -A 5 "styled\.|theme" src/components/molecules/BrowserTabs.tsxLength of output: 800
Script:
#!/bin/bash # Let's check the styled components in BrowserTabs and their theme implementation rg -A 10 "styled\." src/components/molecules/BrowserTabs.tsx # Check if BrowserWindow has any styling that might need theme support rg -A 10 "styled\.|className|style" src/components/organisms/BrowserWindow.tsx # Check if there are any color-related styles in the BrowserContent itself rg -A 5 "background|color|border|style=" src/components/organisms/BrowserContent.tsxLength of output: 3144
src/components/molecules/BrowserTabs.tsx (3)
126-138: Adjust CloseButton styles to adapt to theme modeThe
CloseButtoncomponent uses hardcoded colors that may not align with the current theme. Adjust thebackgroundColorand hover styles based onisDarkModefor consistent theming.Apply this diff to make the
CloseButtonstyles adapt to the theme mode:sx={{ height: '28px', width: '28px', padding: '4px', - backgroundColor: '#3A3A3A', // Synced dark gray background + backgroundColor: `${isDarkMode ? '#3A3A3A' : '#FFFFFF'}`, // Adjust background color based on theme borderRadius: '50%', '&:hover': { - backgroundColor: '#505050', // Synced hover color for close button - color: '#FFFFFF', + backgroundColor: `${isDarkMode ? '#505050' : '#F0F0F0'}`, // Adjust hover background color + color: `${isDarkMode ? '#FFFFFF' : '#000000'}`, // Adjust hover text color }, '&.Mui-disabled': { opacity: 0.4, // Lower opacity for disabled state }, transition: 'background-color 0.3s ease, color 0.3s ease', }}
42-42: Consider making width responsiveSetting a fixed width can impact responsiveness. Use relative units like percentages to ensure the component adjusts to different screen sizes.
Apply this diff to make the width responsive:
- width: '900px', // Fixed width + width: '100%', // Make width responsive
97-108: Remove commented-out codeThere is a block of commented-out code related to the
IconButton. Remove unused code to keep the codebase clean and maintainable.src/components/molecules/NavBar.tsx (1)
215-225: AdjustdiscardButtonstyles to adapt to theme modeThe
discardButtonstyles use hardcoded colors that might not provide optimal contrast in dark mode. Adjust thebackgroundandcolorproperties based ondarkModefor better visibility and theme consistency.Modify the
discardButtonstyle to be a function that acceptsdarkMode:- discardButton: { + discardButton: (darkMode: boolean) => ({ borderRadius: '5px', padding: '8px', - background: 'red', + background: darkMode ? '#ff4d4d' : 'red', color: 'white', marginRight: '10px', '&:hover': { color: 'white', backgroundColor: '#ff0000' } - } + }),Update the usage in the component:
<IconButton onClick={goToMainMenu} - sx={styles.discardButton} + sx={styles.discardButton(darkMode)} >src/App.tsx (1)
3-3: Remove unused importcreateThemeThe
createThemeimport is no longer used after removingThemeProvider. Remove unused imports to clean up the code.Apply this diff to remove the unused import:
-import { createTheme } from "@mui/material/styles";src/context/theme-provider.tsx (1)
5-75: Consider extracting theme configuration to separate filesThe light and dark theme configurations contain duplicated values and could benefit from better organization.
Consider:
- Extract shared values to a common configuration:
// src/theme/constants.ts export const THEME_COLORS = { primary: '#ff00c3', primaryHover: '#e600b3', }
- Create separate files for light and dark themes:
// src/theme/lightTheme.ts import { THEME_COLORS } from './constants'; export const lightTheme = createTheme({...})src/components/molecules/ActionSettings.tsx (1)
9-9: Consider using theme context instead of prop drillingThe component could directly use the theme context instead of receiving darkMode as a prop.
- interface ActionSettingsProps { - action: string; - darkMode?: boolean; - } + interface ActionSettingsProps { + action: string; + } - export const ActionSettings = ({ action, darkMode = false }: ActionSettingsProps) => { + export const ActionSettings = ({ action }: ActionSettingsProps) => { + const { darkMode } = useThemeMode();Also applies to: 12-12
src/components/molecules/BrowserRecordingSave.tsx (1)
Line range hint
14-22: Add error handling for stopRecordingThe navigation function doesn't properly handle API errors.
const goToMainMenu = async () => { if (browserId) { + try { await stopRecording(browserId); notify('warning', 'Current Recording was terminated'); setBrowserId(null); + } catch (error) { + console.error('Failed to stop recording:', error); + notify('error', 'Failed to stop recording'); + return; + } } navigate('/'); };src/pages/Login.tsx (1)
Line range hint
63-71: Implement dark theme supportThe login form uses a hard-coded white background color which doesn't align with the dark theme implementation. Consider using theme-aware colors:
sx={{ textAlign: "center", - backgroundColor: "#ffffff", + backgroundColor: theme.palette.background.paper, padding: 6, borderRadius: 5, - boxShadow: "0px 20px 40px rgba(0, 0, 0, 0.2), 0px -5px 10px rgba(0, 0, 0, 0.15)", + boxShadow: theme.shadows[4], display: "flex", flexDirection: "column", alignItems: "center", maxWidth: 400, width: "100%", }}Don't forget to import and use the
useThemehook:+import { useTheme } from "@mui/material/styles"; const Login = () => { + const theme = useTheme(); // ... rest of the componentsrc/components/organisms/MainMenu.tsx (2)
25-27: Centralize theme color constantsConsider moving the color constants to a theme configuration file for better maintainability:
- const defaultcolor = theme.palette.mode === 'light' ? 'black' : 'white'; - const selectedPink = '#FF00C3'; + const defaultcolor = theme.palette.text.primary; + const selectedPink = theme.palette.primary.main;Create a theme configuration file (e.g.,
src/theme/index.ts) to define these colors:export const themeConfig = { palette: { primary: { main: '#FF00C3', }, // ... other theme configurations }, };
46-69: Extract tab styles to a separate styled componentThe inline styles for tabs are quite lengthy. Consider extracting them to a styled component for better maintainability:
const StyledTabs = styled(Tabs)(({ theme }) => ({ alignItems: 'flex-start', '& .MuiTab-root': { color: theme.palette.text.primary, textTransform: 'none', fontSize: 'medium', justifyContent: 'flex-start', textAlign: 'left', '&.Mui-selected': { color: theme.palette.primary.main, }, '& .MuiTabs-indicator': { backgroundColor: theme.palette.primary.main, }, }, }));Then use it in your component:
-<Tabs +<StyledTabs value={value} onChange={handleChange} orientation="vertical" TabIndicatorProps={{ style: { - backgroundColor: '#ff00c3', + backgroundColor: theme.palette.primary.main, width: '2px', right: 0, }, }} - sx={{...}} />src/components/molecules/BrowserNavBar.tsx (3)
16-26: Consider using MUI's styling system for consistencyThe component mixes styled-components with Material-UI. Consider using MUI's styling system for consistency:
import { styled } from '@mui/material/styles'; const StyledNavBar = styled('div')<{ browserWidth: number }>( ({ theme, browserWidth }) => ({ display: 'flex', alignItems: 'center', padding: '10px 20px', backgroundColor: theme.palette.background.paper, width: `${browserWidth}px`, borderRadius: '0px 0px 8px 8px', boxShadow: theme.shadows[1], transition: theme.transitions.create(['background-color', 'box-shadow']), marginBottom: '15px', }) );
28-46: Extract IconButton component to a separate fileThe
IconButtoncomponent has significant styling logic. Consider moving it to a separate file:// src/components/atoms/buttons/IconButton.tsx import { styled } from '@mui/material/styles'; import { NavBarButton } from './buttons'; export const IconButton = styled(NavBarButton)(({ theme }) => ({ display: 'flex', alignItems: 'center', justifyContent: 'center', padding: '8px', marginRight: '12px', backgroundColor: theme.palette.mode === 'dark' ? '#40444B' : '#E0E0E0', borderRadius: '50%', transition: theme.transitions.create(['background-color', 'transform']), color: theme.palette.text.primary, cursor: 'pointer', '&:hover': { backgroundColor: theme.palette.mode === 'dark' ? '#586069' : '#D0D0D0', }, '&:active': { transform: 'scale(0.95)', }, }));
106-140: Extract navigation buttons into a separate componentThe repeated button pattern could be extracted into a reusable component:
interface NavButtonProps { onClick: () => void; icon: React.ReactNode; mode: string; } const NavigationButton: React.FC<NavButtonProps> = ({ onClick, icon, mode }) => ( <IconButton type="button" onClick={onClick} disabled={false} mode={mode} > {icon} </IconButton> );Usage:
<NavigationButton onClick={() => socket?.emit('input:back')} icon={<ArrowBackIcon />} mode={isDarkMode ? 'dark' : 'light'} />src/components/molecules/ActionDescriptionBox.tsx (1)
97-104: Simplify checkbox styling using theme tokensThe checkbox styling can be simplified by using MUI's theme tokens instead of conditional colors.
Consider this implementation:
- sx={{ - color: isDarkMode ? 'white' : 'default', - '&.Mui-checked': { - color: isDarkMode ? '#90caf9' : '#1976d2', - }, - }} + sx={(theme) => ({ + color: theme.palette.text.primary, + '&.Mui-checked': { + color: theme.palette.primary.main, + }, + })}src/components/molecules/IntegrationSettings.tsx (1)
Line range hint
307-322: Consider extracting theme-specific styles to a theme configuration.While the dark mode implementation works, the hardcoded colors could be moved to a theme configuration for better maintainability.
Consider creating a theme configuration:
const themeColors = { dark: { alertBg: '#3b002d', alertBorder: '#ff00c3', text: 'white' }, light: { alertBg: '#ffc4f1', alertBorder: '#ff00c3', text: 'black' } };src/components/organisms/ProxyForm.tsx (1)
Accessibility concerns identified in dark mode color contrasts
The code reveals potential WCAG contrast issues:
- The pink accent color (#ff00c3) is used consistently across both light and dark modes without adjustment
- Alert component in dark mode uses a very dark purple background (#3b002d) with white text
- Tab indicators and hover states use the same pink (#FF69B4) regardless of theme
To ensure better accessibility:
- Consider using a lighter shade of pink for dark mode to maintain sufficient contrast
- Validate the contrast ratio between the dark purple background (#3b002d) and white text
- Ensure the pink accent colors meet a minimum contrast ratio of 4.5:1 for normal text and 3:1 for large text
🔗 Analysis chain
Line range hint
300-322: Consider improving accessibility for dark mode.While the color scheme is visually appealing, ensure all interactive elements meet WCAG contrast requirements in both themes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential accessibility issues in theme-related styles ast-grep --pattern 'style={{ color:' # Find inline color styles ast-grep --pattern 'sx={{ color:' # Find MUI sx color propsLength of output: 74
Script:
#!/bin/bash # Search for color-related styles in the ProxyForm component with more context rg -A 2 -B 2 "color:|backgroundColor:" "src/components/organisms/ProxyForm.tsx" # Search for theme-related configurations rg -A 5 "createTheme|ThemeProvider|useTheme|isDarkMode" "src/components/organisms/ProxyForm.tsx"Length of output: 3773
src/components/organisms/RightSidePanel.tsx (2)
Line range hint
444-474: Consolidate repeated theme-specific styles.Multiple components share similar theme-specific styling. Consider extracting these into a shared style configuration.
const commonStyles = { background: isDarkMode ? "#1E2124" : 'white', color: isDarkMode ? "white" : 'black' };
415-415: Standardize button styling across the component.Multiple buttons share the same styling properties. Consider creating a custom Button component with theme-aware styling.
const ThemedButton = styled(Button)(({ theme }) => ({ backgroundColor: "#ff00c3", color: theme.darkMode ? 'white' : 'black' }));Also applies to: 482-482, 491-491
src/index.css (1)
14-14: Remove unnecessary empty linesThese empty lines within the CSS rules don't serve any purpose and should be removed for better code cleanliness.
body { scrollbar-gutter: stable; overflow-y: auto; - } #browser-recorder { overflow: hidden; position: relative; - }Also applies to: 47-47
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
src/App.tsx(1 hunks)src/components/atoms/Loader.tsx(2 hunks)src/components/atoms/RecorderIcon.tsx(1 hunks)src/components/atoms/buttons/buttons.tsx(1 hunks)src/components/atoms/canvas.tsx(1 hunks)src/components/molecules/ActionDescriptionBox.tsx(6 hunks)src/components/molecules/ActionSettings.tsx(3 hunks)src/components/molecules/BrowserNavBar.tsx(5 hunks)src/components/molecules/BrowserRecordingSave.tsx(1 hunks)src/components/molecules/BrowserTabs.tsx(4 hunks)src/components/molecules/IntegrationSettings.tsx(4 hunks)src/components/molecules/InterpretationLog.tsx(4 hunks)src/components/molecules/NavBar.tsx(2 hunks)src/components/molecules/RunContent.tsx(2 hunks)src/components/molecules/SaveRecording.tsx(1 hunks)src/components/organisms/BrowserContent.tsx(3 hunks)src/components/organisms/BrowserWindow.tsx(1 hunks)src/components/organisms/MainMenu.tsx(3 hunks)src/components/organisms/ProxyForm.tsx(4 hunks)src/components/organisms/RightSidePanel.tsx(11 hunks)src/context/theme-provider.tsx(1 hunks)src/index.css(3 hunks)src/pages/Login.tsx(1 hunks)src/pages/RecordingPage.tsx(4 hunks)src/pages/Register.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/components/atoms/RecorderIcon.tsx
- src/pages/Register.tsx
- src/components/atoms/canvas.tsx
- src/components/organisms/BrowserWindow.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/index.css
[error] 60-62: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
🔇 Additional comments (6)
src/components/molecules/InterpretationLog.tsx (1)
20-20: LGTM: Theme hook integration looks good!
The integration of the useThemeMode hook is clean and follows React best practices.
Also applies to: 117-118
src/components/atoms/Loader.tsx (1)
Line range hint 10-35: LGTM!
The implementation correctly adapts the loader text color based on the theme mode.
src/pages/Login.tsx (1)
41-41: LGTM: Proper CORS configuration
The addition of withCredentials: true is correct for handling cross-origin authentication properly.
src/components/molecules/IntegrationSettings.tsx (1)
30-42: LGTM: Cookie management implementation is secure and well-typed.
The custom cookie management functions provide a good replacement for the js-cookie library. The implementation follows security best practices by using HttpOnly cookies and proper expiration handling.
src/components/organisms/ProxyForm.tsx (1)
26-45: LGTM: Well-structured custom tab components with proper theme integration.
The custom styled components provide consistent styling and good user experience with proper hover states and selection indicators.
src/index.css (1)
60-62:
Add dark theme styles to the empty #browser selector
Given that this PR implements dark theme support, the empty #browser selector should include theme-specific styles. Consider adding background color, text color, and other relevant dark theme properties.
Let's verify the theme implementation in other files:
Example implementation:
#browser {
background-color: var(--background-color);
color: var(--text-color);
transition: background-color 0.3s ease, color 0.3s ease;
}
/* Dark theme */
[data-theme="dark"] #browser {
--background-color: #1a1a1a;
--text-color: #ffffff;
}
/* Light theme */
[data-theme="light"] #browser {
--background-color: #ffffff;
--text-color: #000000;
}🧰 Tools
🪛 Biome (1.9.4)
[error] 60-62: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
| @@ -159,6 +199,7 @@ export const RunContent = ({ row, currentLog, interpretationInProgress, logEndRe | |||
| background: 'rgba(0,0,0,0.06)', | |||
| maxHeight: '300px', | |||
| overflow: 'scroll', | |||
| backgroundColor: '#19171c' | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use theme-aware background color
The background color should adapt to the current theme mode instead of using a hardcoded dark color.
- backgroundColor: '#19171c'
+ backgroundColor: theme => theme.palette.mode === 'dark' ? theme.palette.grey[900] : theme.palette.grey[100]📝 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.
| backgroundColor: '#19171c' | |
| backgroundColor: theme => theme.palette.mode === 'dark' ? theme.palette.grey[900] : theme.palette.grey[100] |
| background: `${isDarkMode ? '#1e2124' : 'white'}`, | ||
| color: `${isDarkMode ? 'white' : 'black'}`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using MUI's theme system for better integration
Instead of directly using template literals for theme conditions, consider using MUI's theme system for better integration and maintainability:
PaperProps={{
sx: {
- background: `${isDarkMode ? '#1e2124' : 'white'}`,
- color: `${isDarkMode ? 'white' : 'black'}`,
+ bgcolor: 'background.paper',
+ color: 'text.primary',
padding: '10px',
height: 500,
width: width - 10,This approach:
- Uses MUI's theme tokens which automatically handle light/dark modes
- Provides better type safety
- Ensures consistency with MUI's theming system
📝 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.
| background: `${isDarkMode ? '#1e2124' : 'white'}`, | |
| color: `${isDarkMode ? 'white' : 'black'}`, | |
| bgcolor: 'background.paper', | |
| color: 'text.primary', |
| @@ -12,6 +12,7 @@ import { | |||
| } from "../../api/recording"; | |||
| import { Box } from "@mui/material"; | |||
| import { InterpretationLog } from "../molecules/InterpretationLog"; | |||
| import { Height } from "@mui/icons-material"; | |||
There was a problem hiding this comment.
Remove unused import Height
The Height icon from MUI is imported but never used in the component.
-import { Height } from "@mui/icons-material";| backgroundColor: '#f5f5f5', // Synced dark background for tabs | ||
| borderRadius: '8px 8px 0px 0px', | ||
| marginRight: '8px', | ||
| minHeight: '48px', | ||
| textTransform: 'none', | ||
| fontWeight: '500', | ||
| fontSize: '14px', | ||
| color: 'black', // Synced light gray text color | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust Tab styles to adapt to theme mode
The backgroundColor and color properties for the Tab component are hardcoded and do not change with the theme. To ensure consistent theming, adjust these properties based on isDarkMode.
Apply this diff to make the Tab styles adapt to the theme mode:
sx={{
- backgroundColor: '#f5f5f5', // Synced dark background for tabs
+ backgroundColor: `${isDarkMode ? '#2a2a2a' : '#f5f5f5'}`, // Adjust background color based on theme
borderRadius: '8px 8px 0px 0px',
marginRight: '8px',
minHeight: '48px',
textTransform: 'none',
fontWeight: '500',
fontSize: '14px',
- color: 'black', // Synced light gray text color
+ color: `${isDarkMode ? '#FFFFFF' : 'black'}`, // Adjust text color based on theme
'&.Mui-selected': {
backgroundColor: `${isDarkMode ? '#2a2a2a' : '#f5f5f5'}`, // Synced selected tab color
color: '#ff00c3', // Slightly lighter text when selected
},
}}Committable suggestion skipped: line range outside the PR's diff.
| export const NavBarButton = styled.button<{ disabled: boolean, mode: 'light' | 'dark' }>` | ||
| margin-left: 10px; | ||
| margin-right: 5px; | ||
| padding: 0; | ||
| border: none; | ||
| background-color: transparent; | ||
| background-color: ${mode => mode ? '#333' : '#ffffff'}; | ||
| cursor: ${({ disabled }) => disabled ? 'default' : 'pointer'}; | ||
| width: 24px; | ||
| height: 24px; | ||
| border-radius: 12px; | ||
| outline: none; | ||
| color: ${({ disabled }) => disabled ? '#999' : '#333'}; | ||
| color: ${mode => mode ? '#ffffff' : '#333333'}; | ||
There was a problem hiding this comment.
Fix style functions to correctly access the mode prop
In the styled component, the template literals should access the mode prop correctly. Currently, background-color: ${mode => mode ? ...} does not access the mode prop as intended.
Apply this diff to fix the issue:
export const NavBarButton = styled.button<{ disabled: boolean, mode: 'light' | 'dark' }>`
margin-left: 10px;
margin-right: 5px;
padding: 0;
border: none;
- background-color: ${mode => mode ? '#333' : '#ffffff'};
+ background-color: ${({ mode }) => mode === 'dark' ? '#333' : '#ffffff'};
cursor: ${({ disabled }) => disabled ? 'default' : 'pointer'};
width: 24px;
height: 24px;
border-radius: 12px;
outline: none;
- color: ${mode => mode ? '#ffffff' : '#333333'};
+ color: ${({ mode }) => mode === 'dark' ? '#ffffff' : '#333333'};
`;📝 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.
| export const NavBarButton = styled.button<{ disabled: boolean, mode: 'light' | 'dark' }>` | |
| margin-left: 10px; | |
| margin-right: 5px; | |
| padding: 0; | |
| border: none; | |
| background-color: transparent; | |
| background-color: ${mode => mode ? '#333' : '#ffffff'}; | |
| cursor: ${({ disabled }) => disabled ? 'default' : 'pointer'}; | |
| width: 24px; | |
| height: 24px; | |
| border-radius: 12px; | |
| outline: none; | |
| color: ${({ disabled }) => disabled ? '#999' : '#333'}; | |
| color: ${mode => mode ? '#ffffff' : '#333333'}; | |
| export const NavBarButton = styled.button<{ disabled: boolean, mode: 'light' | 'dark' }>` | |
| margin-left: 10px; | |
| margin-right: 5px; | |
| padding: 0; | |
| border: none; | |
| background-color: ${({ mode }) => mode === 'dark' ? '#333' : '#ffffff'}; | |
| cursor: ${({ disabled }) => disabled ? 'default' : 'pointer'}; | |
| width: 24px; | |
| height: 24px; | |
| border-radius: 12px; | |
| outline: none; | |
| color: ${({ mode }) => mode === 'dark' ? '#ffffff' : '#333333'}; | |
| interface CustomBoxContainerProps { | ||
| isDarkMode: boolean; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using MUI's theme system instead of prop drilling
The isDarkMode prop is being passed through multiple components when MUI's theme system could handle this more elegantly.
Consider using MUI's theme system:
interface CustomBoxContainerProps {
theme: Theme; // Use MUI's Theme type
}| background-color: ${({ isDarkMode }) => (isDarkMode ? '#313438' : 'white')}; | ||
| color: ${({ isDarkMode }) => (isDarkMode ? 'white' : 'black')}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use theme tokens instead of hardcoded colors
Hardcoded colors (#313438, white) should be replaced with theme tokens for better maintainability and consistency.
Consider this implementation:
- background-color: ${({ isDarkMode }) => (isDarkMode ? '#313438' : 'white')};
- color: ${({ isDarkMode }) => (isDarkMode ? 'white' : 'black')};
+ background-color: ${({ theme }) => theme.palette.background.paper};
+ color: ${({ theme }) => theme.palette.text.primary};
- border-bottom: 20px solid ${({ isDarkMode }) => (isDarkMode ? '#313438' : 'white')};
+ border-bottom: 20px solid ${({ theme }) => theme.palette.background.paper};Also applies to: 33-33
| if (darkMode) { | ||
|
|
||
| document.body.style.background = 'rgba(18,18,18,1)'; | ||
|
|
||
| } else { | ||
| document.body.style.background = 'radial-gradient(circle, rgba(255, 255, 255, 1) 0%, rgba(232, 191, 222, 1) 100%, rgba(255, 255, 255, 1) 100%)'; | ||
| } | ||
|
|
||
| return () => { | ||
| // Cleanup the background when leaving the page | ||
| document.body.style.background = ''; | ||
| document.body.style.filter = ''; | ||
| }; | ||
| }, []); | ||
| }, [darkMode]); |
There was a problem hiding this comment.
Avoid direct DOM manipulation for theming
Direct manipulation of document.body.style is not recommended for theming. This approach can lead to conflicts with other styles and doesn't leverage MUI's theme system.
Consider these alternatives:
- Use MUI's theme system with styled components:
const PageWrapper = styled('div')(({ theme }) => ({
background: theme.palette.mode === 'dark'
? theme.palette.background.default
: 'radial-gradient(circle, rgba(255, 255, 255, 1) 0%, rgba(232, 191, 222, 1) 100%, rgba(255, 255, 255, 1) 100%)',
minHeight: '100vh'
}));- Or use CSS variables for dynamic theming:
// In your theme setup
document.documentElement.style.setProperty(
'--page-background',
theme.palette.mode === 'dark'
? 'rgba(18,18,18,1)'
: 'radial-gradient(...)'
);
// In your styled component
const PageWrapper = styled('div')`
background: var(--page-background);
min-height: 100vh;
`;| @@ -159,4 +164,5 @@ const RecordingPageWrapper = styled.div` | |||
| width: 100vw; | |||
| height: 100vh; | |||
| overflow: hidden; | |||
|
|
|||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add theme-aware background to RecordingPageWrapper
The styled RecordingPageWrapper component should include theme-aware background styling.
const RecordingPageWrapper = styled.div`
position: relative;
width: 100vw;
height: 100vh;
overflow: hidden;
+ background: ${({ theme }) => theme.palette.background.default};
`;Committable suggestion skipped: line range outside the PR's diff.
| <Paper | ||
| sx={{ | ||
| borderRadius: '8px 8px 0px 0px', // Slightly more rounded corners for a smoother look | ||
| height: '520px', | ||
| width: 'auto', | ||
| alignItems: "center", | ||
| padding: '16px', // Add padding for spacing inside the component | ||
| background: isDarkMode | ||
| ? 'linear-gradient(135deg, #1E2124 0%, #292C2F 100%)' // Subtle gradient for dark mode | ||
| : 'linear-gradient(135deg, #f5f5f5 0%, #ffffff 100%)', // Subtle gradient for light mode | ||
| color: isDarkMode ? '#E0E0E0' : '#333333', // Adjusted color for better contrast | ||
| // Smooth transition for shadows and background changes | ||
| }} | ||
| id="browser-actions" | ||
| elevation={0} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using MUI's theme system for consistent styling.
The current implementation uses inline styles for theme-specific colors. Consider leveraging MUI's theme system for better maintainability and consistency.
Example implementation:
const theme = createTheme({
palette: {
mode: isDarkMode ? 'dark' : 'light',
background: {
paper: isDarkMode
? 'linear-gradient(135deg, #1E2124 0%, #292C2F 100%)'
: 'linear-gradient(135deg, #f5f5f5 0%, #ffffff 100%)'
}
}
});
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
src/pages/PageWrappper.tsx (2)
15-15: Remove unused importThe
AppBarcomponent from@mui/materialis imported but never used in the code.-import { AppBar } from '@mui/material';
54-56: Consider simplifying the NavBar conditional renderingThe current conditional rendering can be simplified by moving the condition inside the NavBar component or using the logical AND operator.
-{!browserId && <NavBar recordingName={recordingName} isRecording={!!browserId} />} +<NavBar + recordingName={recordingName} + isRecording={!!browserId} + show={!browserId} +/>src/components/molecules/NavBar.tsx (4)
101-108: Enhance iframe accessibility and performanceThe GitHub button iframe could benefit from some improvements:
- Loading attribute for performance
- More descriptive title for accessibility
<iframe src="https://ghbtns.com/github-btn.html?user=getmaxun&repo=maxun&type=star&count=true&size=large" frameBorder="0" scrolling="0" width="170" height="30" - title="GitHub" + title="GitHub Star Button for Maxun Repository" + loading="lazy" />
190-226: Consider moving styles to a separate fileThe styles object is quite large and could be moved to a separate file for better maintainability.
Consider creating a new file
NavBar.styles.ts:// NavBar.styles.ts export const styles = { socialButton: {...}, userButton: (darkMode: boolean) => ({...}), discardButton: {...} };
231-235: Consider using MUI's theme system for consistent stylingInstead of using styled-components with manual theme checks, consider leveraging MUI's theme system for better integration and consistency.
import { styled as muiStyled } from '@mui/material/styles'; const NavBarWrapper = muiStyled('div')(({ theme }) => ({ gridArea: 'navbar', backgroundColor: theme.palette.background.paper, padding: theme.spacing(0.5), display: 'flex', justifyContent: 'space-between', borderBottom: `1px solid ${theme.palette.divider}` }));
68-82: Consider component composition for better maintainabilityThe current approach of using render methods could be improved by breaking down into smaller components.
Consider creating separate components:
// components/NavBar/BrandSection.tsx const BrandSection = () => {...} // components/NavBar/SocialButtons.tsx const SocialButtons = () => {...} // components/NavBar/UserMenu.tsx const UserMenu = () => {...} // components/NavBar/ThemeToggle.tsx const ThemeToggle = () => {...} // components/NavBar/RecordingControls.tsx const RecordingControls = () => {...}This would improve:
- Code organization and maintainability
- Testing capabilities
- Potential for code reuse
Also applies to: 84-110, 112-139, 141-155, 157-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/components/molecules/NavBar.tsx(2 hunks)src/pages/Login.tsx(3 hunks)src/pages/PageWrappper.tsx(2 hunks)src/pages/Register.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/Login.tsx
- src/pages/Register.tsx
| const { state, dispatch } = useContext(AuthContext); | ||
| const { user } = state; | ||
| const navigate = useNavigate(); | ||
|
|
||
| const { darkMode, toggleTheme } = useThemeMode(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a theme persistence mechanism
The theme preference should persist across page reloads for better user experience.
Consider storing the theme preference in localStorage:
const { darkMode, toggleTheme } = useThemeMode();
useEffect(() => {
localStorage.setItem('theme', darkMode ? 'dark' : 'light');
}, [darkMode]);| background: 'red', | ||
| color: 'white', | ||
| marginRight: '10px', | ||
| '&:hover': { | ||
| color: 'white', | ||
| backgroundColor: '#ff0000' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error prevention UI
The discard button's styling suggests a destructive action. Consider adding a confirmation dialog before discarding.
const [showConfirmDialog, setShowConfirmDialog] = useState(false);
const handleDiscard = () => {
setShowConfirmDialog(true);
};
// Add Dialog component
<Dialog
open={showConfirmDialog}
onClose={() => setShowConfirmDialog(false)}
>
<DialogTitle>Confirm Discard</DialogTitle>
<DialogContent>
Are you sure you want to discard the current recording?
</DialogContent>
<DialogActions>
<Button onClick={() => setShowConfirmDialog(false)}>Cancel</Button>
<Button onClick={goToMainMenu} color="error">Discard</Button>
</DialogActions>
</Dialog>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/components/molecules/NavBar.tsx (2)
101-108: Enhance GitHub button implementationThe GitHub iframe button might cause layout shifts and accessibility issues. Consider using the GitHub REST API to fetch the star count and implement a custom button component.
Example implementation:
const [starCount, setStarCount] = useState<number>(0); useEffect(() => { fetch('https://api.github.com/repos/getmaxun/maxun') .then(res => res.json()) .then(data => setStarCount(data.stargazers_count)) .catch(console.error); }, []); // Replace iframe with custom button <Button startIcon={<StarIcon />} href="https://github.com/getmaxun/maxun" target="_blank" rel="noopener noreferrer" > Star ({starCount}) </Button>
141-155: Enhance theme toggle accessibilityThe theme toggle button could benefit from additional accessibility features.
const renderThemeToggle = () => ( <Tooltip title="Toggle light/dark theme"> <IconButton onClick={toggleTheme} + aria-label={`Switch to ${darkMode ? 'light' : 'dark'} theme`} + aria-pressed={darkMode} sx={{ color: darkMode ? '#ffffff' : '#333333', '&:hover': { color: '#ff00c3' } }} > {darkMode ? <Brightness7 /> : <Brightness4 />} </IconButton> </Tooltip> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/components/molecules/NavBar.tsx(2 hunks)src/components/molecules/SaveRecording.tsx(1 hunks)src/components/organisms/RightSidePanel.tsx(11 hunks)src/pages/Register.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/organisms/RightSidePanel.tsx
- src/pages/Register.tsx
- src/components/molecules/SaveRecording.tsx
🔇 Additional comments (1)
src/components/molecules/NavBar.tsx (1)
40-40: Theme preference should persist across page reloads
The theme preference is not persisted across page reloads.
| // Styles | ||
| const styles = { | ||
| socialButton: { | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| borderRadius: '5px', | ||
| padding: '8px', | ||
| marginRight: '30px', | ||
| color: '#333333', | ||
| '&:hover': { | ||
| color: '#ff00c3' | ||
| } | ||
| }, | ||
| userButton: (darkMode: boolean) => ({ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| borderRadius: '5px', | ||
| padding: '8px', | ||
| marginRight: '10px', | ||
| color: darkMode ? '#ffffff' : '#333333', | ||
| '&:hover': { | ||
| backgroundColor: darkMode ? '#333' : '#F5F5F5', | ||
| color: '#ff00c3' | ||
| } | ||
| }), | ||
| discardButton: { | ||
| borderRadius: '5px', | ||
| padding: '8px', | ||
| background: 'red', | ||
| color: 'white', | ||
| marginRight: '10px', | ||
| '&:hover': { | ||
| color: 'white', | ||
| backgroundColor: '#ff0000' | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Centralize theme configuration
Consider creating a central theme configuration to manage colors and other theme values. This will improve maintainability and consistency.
Example implementation:
// src/theme/constants.ts
export const themeColors = {
light: {
background: '#ffffff',
text: '#333333',
border: '#e0e0e0',
hover: '#F5F5F5',
},
dark: {
background: '#1e2124',
text: '#ffffff',
border: '#333',
hover: '#333',
},
accent: {
primary: '#ff00c3',
error: '#ff0000',
},
};
// Update styles to use theme constants
const styles = {
userButton: (darkMode: boolean) => ({
// ...other styles
color: darkMode ? themeColors.dark.text : themeColors.light.text,
'&:hover': {
backgroundColor: darkMode ? themeColors.dark.hover : themeColors.light.hover,
color: themeColors.accent.primary
}
}),
};| import { AuthContext } from '../../context/auth'; | ||
| import { SaveRecording } from '../molecules/SaveRecording'; | ||
| import DiscordIcon from '../atoms/DiscordIcon'; | ||
| import { apiUrl } from '../../apiConfig'; | ||
| import MaxunLogo from "../../assets/maxunlogo.png"; | ||
| import { useThemeMode } from '../../context/theme-provider'; |
There was a problem hiding this comment.
💡 Codebase verification
Theme implementation needs improvement for consistent dark/light mode support
Several components have hardcoded color values that should be using theme tokens instead:
SaveRecording.tsx: Uses hardcoded#ff00c3for button backgroundRunContent.tsx: Multiple instances of hardcoded#FF00C3and background colorsBrowserTabs.tsx: Contains hardcoded background colors (#2A2A2A,#3A3A3A, etc.)RecordingsTable.tsx: Uses hardcoded#ff00c3for hover statesMainMenu.tsx: Contains hardcoded pink colors instead of theme values
While the theme provider is properly imported across components, the implementation is inconsistent. Some components correctly use the theme context with conditional dark/light mode values, while others use hardcoded colors that won't respond to theme changes.
🔗 Analysis chain
Verify theme consistency across components
Let's verify that the theme implementation is consistent across all components that import the theme provider.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using the theme provider
echo "Components using theme provider:"
rg -l "useThemeMode|ThemeModeProvider" "src/"
# Search for hardcoded color values that might need theme support
echo "\nPotential hardcoded colors that might need theme support:"
rg -n "'#[0-9a-fA-F]{3,6}'" "src/components/"
Length of output: 9261
20241106-1902-08.3805076.mp4
Hey, I have added the dark theme.
Summary by CodeRabbit
Release Notes
New Features
Loader,NavBar, andActionDescriptionBoxto support dynamic theming.Improvements
BrowserContent,RightSidePanel, andMainMenucomponents to adapt to theme changes.Login,Register, andRecordingPagecomponents for improved user experience.Bug Fixes
IntegrationSettingscomponent for better user experience.Chores