Skip to content

Conversation

@VietND96
Copy link
Member

@VietND96 VietND96 commented Oct 26, 2025

User description

🔗 Related Issues

💥 What does this PR do?

After introducing Dark Mode in Grid UI, there are a few screens not consistent in colors

  • Node status indicators when DRAINING, DOWN
  • Background of Live Preview dialog
Screenshot at Oct 26 20-16-56 Screenshot at Oct 26 20-25-33

After fixing, the UI looks better

  • Node status indicators color, and Node status chip label in the Node detail dialog
Screenshot at Oct 26 21-15-07 Screenshot at Oct 26 21-10-14 Screenshot at Oct 26 21-14-36
  • Live Preview dialog background in Dark mode is fully integrated
Screenshot at Oct 26 20-30-46

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement


Description

  • Integrate theme colors for consistent dark mode support across Grid UI

  • Enhance node status indicators with color-coded visual states (DOWN, DRAINING, UP)

  • Improve Live Preview dialog styling with theme-aware backgrounds

  • Add status chip display in node details dialog with semantic colors


Diagram Walkthrough

flowchart LR
  A["Theme System"] -->|"warning palette"| B["Node Status Indicators"]
  A -->|"background.default"| C["Live Preview Dialog"]
  B -->|"DOWN/DRAINING/UP"| D["Visual Status States"]
  C -->|"theme-aware styling"| E["Dark Mode Support"]
  D -->|"Chip component"| F["Node Details Dialog"]
Loading

File Walkthrough

Relevant files
Enhancement
themes.tsx
Add warning color palette to themes                                           

javascript/grid-ui/src/theme/themes.tsx

  • Added warning color palette to both light and dark themes
  • Light theme warning color set to #FF9800
  • Dark theme warning color set to #FFA726
+6/-0     
LiveView.tsx
Apply theme-aware background to canvas                                     

javascript/grid-ui/src/components/LiveView/LiveView.tsx

  • Import useTheme hook from Material-UI
  • Replace hardcoded background color with theme-aware
    theme.palette.background.default
  • Ensures Live Preview canvas respects current theme settings
+3/-1     
Node.tsx
Enhance node card styling with status-based colors             

javascript/grid-ui/src/components/Node/Node.tsx

  • Refactor getCardStyle function to provide theme-aware styling for
    different node statuses
  • DOWN status: reduced opacity (0.6), red error border, disabled
    background
  • DRAINING status: warning-colored border, hover background
  • Apply theme colors to DialogContent background
+28/-7   
NodeDetailsDialog.tsx
Add status chip and improve details dialog layout               

javascript/grid-ui/src/components/Node/NodeDetailsDialog.tsx

  • Import Chip component from Material-UI
  • Add getStatusColor function to map node status to semantic colors
  • Restructure dialog title layout with flexbox for better alignment
  • Add status Chip component displaying node status with color coding
  • Add explicit Status field in dialog content
+22/-4   
RunningSessions.tsx
Apply theme background to session preview dialog                 

javascript/grid-ui/src/components/RunningSessions/RunningSessions.tsx

  • Apply theme-aware background color to Live Preview dialog content
  • Set padding to 0 for consistent spacing with theme styling
+1/-1     

@selenium-ci selenium-ci added the B-grid Everything grid and server related label Oct 26, 2025
@qodo-merge-pro
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #1234
🔴 Investigate and fix a regression where clicking links with JavaScript in href no longer
triggers in Selenium 2.48.x (works in 2.47.1) on Firefox 42.
Provide a change that restores the alert behavior in the provided test case.
🟡
🎫 #5678
🔴 Diagnose and resolve intermittent "Error: ConnectFailure (Connection refused)" when
instantiating additional ChromeDriver instances on Ubuntu with Chrome 65/ChromeDriver 2.35
and Selenium 3.9.0.
Ensure subsequent ChromeDriver instantiations do not fail with connection refused errors.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 26, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Update canvas background on theme change

Add a useEffect hook to the LiveView component. This hook should update the
rfb.background property whenever the theme changes, ensuring the VNC canvas
background color stays in sync with the application's theme.

javascript/grid-ui/src/components/LiveView/LiveView.tsx [32-44]

 const LiveView = forwardRef((props, ref) => {
   let canvas: any = null
   const theme = useTheme()
 
   const [open, setOpen] = useState(false)
   const [message, setMessage] = useState('')
   const [rfb, setRfb] = useState<RFB>(null)
   const [openErrorAlert, setOpenErrorAlert] = useState(false)
   const [openSuccessAlert, setOpenSuccessAlert] = useState(false)
 
+  useEffect(() => {
+    if (rfb) {
+      rfb.background = theme.palette.background.default
+    }
+  }, [theme, rfb])
+
   const handlePasswordDialog = (state: boolean): void => {
     setOpen(state)
   }

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a bug where the canvas background won't update if the theme changes during an active session, and the proposed useEffect hook is a valid and robust fix.

Medium
Learned
best practice
Strongly type status-to-color mapping

Narrow status to known values and return a valid ChipColor type to avoid
runtime/type mismatches and ensure consistent mapping.

javascript/grid-ui/src/components/Node/NodeDetailsDialog.tsx [39-71]

-const getStatusColor = (status: string) => {
-  if (status === 'DOWN') return 'error'
-  if (status === 'DRAINING') return 'warning'
-  return 'success'
+type NodeStatus = 'UP' | 'DOWN' | 'DRAINING';
+type ChipColor = 'default' | 'primary' | 'secondary' | 'error' | 'info' | 'success' | 'warning';
+
+const getStatusColor = (status: NodeStatus | string): ChipColor => {
+  switch (status) {
+    case 'DOWN':
+      return 'error';
+    case 'DRAINING':
+      return 'warning';
+    case 'UP':
+      return 'success';
+    default:
+      return 'default';
+  }
 }
 
 ...
 
 <Chip 
-  label={nodeInfo.status} 
+  label={nodeInfo.status}
   color={getStatusColor(nodeInfo.status)}
   size='small'
   sx={{ ml: 'auto' }}
 />

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Keep API and documentation accurate and consistent by aligning parameter types and usage with UI component expectations.

Low
General
Refactor styling logic for readability

Refactor the getCardStyle function to use a switch statement instead of multiple
if conditions. This change improves code readability and maintainability.

javascript/grid-ui/src/components/Node/Node.tsx [104-130]

 const getCardStyle = (status: string) => {
   const baseStyle = {
     height: '100%',
     flexGrow: 1
   }
-  
-  if (status === 'DOWN') {
-    return {
-      ...baseStyle,
-      opacity: 0.6,
-      border: '2px solid',
-      borderColor: 'error.main',
-      bgcolor: 'action.disabledBackground'
-    }
+
+  switch (status) {
+    case 'DOWN':
+      return {
+        ...baseStyle,
+        opacity: 0.6,
+        border: '2px solid',
+        borderColor: 'error.main',
+        bgcolor: 'action.disabledBackground'
+      }
+    case 'DRAINING':
+      return {
+        ...baseStyle,
+        border: '2px solid',
+        borderColor: 'warning.main',
+        bgcolor: 'action.hover'
+      }
+    default:
+      return baseStyle
   }
-  
-  if (status === 'DRAINING') {
-    return {
-      ...baseStyle,
-      border: '2px solid',
-      borderColor: 'warning.main',
-      bgcolor: 'action.hover'
-    }
-  }
-  
-  return baseStyle
 }

[Suggestion processed]

Suggestion importance[1-10]: 4

__

Why: The suggestion proposes a valid refactoring from an if-if-return structure to a switch statement, which can improve readability and maintainability, but it is a stylistic preference with minor impact.

Low
  • More

@VietND96 VietND96 merged commit f7ec959 into trunk Oct 26, 2025
49 of 50 checks passed
@VietND96 VietND96 deleted the grid-ui-dark branch October 26, 2025 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-grid Everything grid and server related Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants