Skip to content

Removed duplicate components whose names ending with two#1647

Merged
johbaxter merged 8 commits intodevfrom
feat/1571-remove-extra-components
Aug 12, 2025
Merged

Removed duplicate components whose names ending with two#1647
johbaxter merged 8 commits intodevfrom
feat/1571-remove-extra-components

Conversation

@Gowrishankar-Palanisamy
Copy link
Copy Markdown
Contributor

This PR contains the following changes

--> Removed the duplicated components whose name were ending with "Two".
--> Updated the common components as per the requirement.

@Gowrishankar-Palanisamy Gowrishankar-Palanisamy requested a review from a team as a code owner August 5, 2025 11:46
@Gowrishankar-Palanisamy Gowrishankar-Palanisamy linked an issue Aug 5, 2025 that may be closed by this pull request
2 tasks
@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 5, 2025

@CodiumAI-Agent /describe

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 5, 2025

@CodiumAI-Agent /review

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 5, 2025

@CodiumAI-Agent /improve

@QodoAI-Agent
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Import

Ensure that all added uses of ListItem and ListItemButton are accompanied by the correct imports to prevent build errors.

import { InfoOutlined } from '@mui/icons-material';

import {
    ListItem,
    List,
    ListItemIcon,
    ListItemText,
    ListItemButton,
    TextField,

Autocomplete Mode
Review the introduction of multiple={false} on Autocomplete to confirm it aligns with expected single‐select behavior across all cases.

Layout Regression

Verify that the new StyledBox wrappers replicate the spacing, alignment, and flex behavior previously provided by BoxTwo.

const StyledBox = styled(Box)(() => ({
    display: 'flex',
    alignItems: 'center',
    gap: '10px'
}))

@QodoAI-Agent
Copy link
Copy Markdown

QodoAI-Agent commented Aug 5, 2025

PR Code Suggestions ✨

Latest suggestions up to fd23e31

CategorySuggestion                                                                                                                                    Impact
Possible issue
Restore correct List subcomponents

Do not replace MUI ListItem, ListItemIcon, and ListItemText with List.Item,
List.Icon, and List.ItemText unless such names exist in your UI lib. This likely
breaks rendering and accessibility. Revert to the actual components or the
equivalents exported by @semoss/ui.

libs/renderer/src/components/block-defaults/visualization-filter-block/FilterComponent.tsx [188-217]

 <List sx={{ maxHeight: 200, overflowY: "auto" }} dense>
 	{multi && (
-		<List.Item key="select-all" onClick={handleToggle("Select All")}>
-			<List.Icon>
+		<ListItem key="select-all" onClick={handleToggle("Select All")}>
+			<ListItemIcon>
 				<Checkbox
 					edge="start"
 					checked={allChecked}
 					indeterminate={indeterminate}
 					tabIndex={-1}
 					disableRipple
 				/>
-			</List.Icon>
-			<List.ItemText primary="Select All" />
-		</List.Item>
+			</ListItemIcon>
+			<ListItemText primary="Select All" />
+		</ListItem>
 	)}
 
 	{filteredOptions.map((option) => (
-		<List.Item key={option} onClick={handleToggle(option)}>
-			<List.Icon>
+		<ListItem key={option} onClick={handleToggle(option)}>
+			<ListItemIcon>
 				<Checkbox
 					edge="start"
 					checked={checked.includes(option)}
 					tabIndex={-1}
 					disableRipple
 				/>
-			</List.Icon>
-			<List.ItemText primary={option} />
-		</List.Item>
+			</ListItemIcon>
+			<ListItemText primary={option} />
+		</ListItem>
 	))}
 </List>
Suggestion importance[1-10]: 9

__

Why: The PR replaces ListItem/ListItemIcon/ListItemText with List.Item/List.Icon/List.ItemText, which are likely nonexistent in @semoss/ui and would break rendering and a11y; reverting prevents a functional bug.

High
Use proper AlertTitle component

Alert.Title may not exist on the Alert namespace after the export change; using it
will cause runtime errors. Import and use the dedicated AlertTitle component (or the
correct namespaced export) explicitly.

packages/client/src/components/blocks-workspace/panels/SelectedBlockPanel.tsx [91-95]

 import {
 	Alert,
+	AlertTitle,
 	Collapse,
 	IconButton,
 	Stack,
 	styled,
 	Typography,
 } from "@semoss/ui";
 ...
-const StyledAlertTitle = styled(Alert.Title)(({ theme }) => ({
+const StyledAlertTitle = styled(AlertTitle)(({ theme }) => ({
 	alignSelf: "stretch",
 	color: "#666",
 	fontFamily: "Inter",
Suggestion importance[1-10]: 8

__

Why: The PR changes exports so Alert.Title may no longer exist; importing and styling AlertTitle directly avoids runtime errors and matches the updated UI exports.

Medium
Avoid invalid empty comparator

Prevent empty comparator updates when clearing selection by using undefined rather
than an empty string, aligning with controlled state expectations and avoiding
invalid rule states. Also ensure string fallback for labels handles nullish values.

packages/client/src/components/blocks-workspace/blocks/settings/custom/grid-two/operations/ColorByValue.tsx [311-330]

 <Autocomplete
     fullWidth
     size="small"
     multiple={false}
     value={
         columnComparision.find(
             (c) => c.value === editingRule?.comparator,
         ) ?? null
     }
     onChange={(_, newValue) => {
         updateFields(
             "comparator",
-            typeof newValue === 'object' && newValue !== null ? newValue.value : ""
+            typeof newValue === 'object' && newValue !== null
+                ? newValue.value
+                : undefined
         );
     }}
     options={columnComparision}
     getOptionLabel={(option) =>
-        typeof option === 'string' ? option : option.name
+        typeof option === 'string' ? option : String(option?.name ?? '')
     }
Suggestion importance[1-10]: 6

__

Why: Using undefined instead of empty string for cleared selection better reflects absence of a value and reduces invalid state risk; matches the shown code context.

Low
Validate and safely flatten values

Guard against non-array items before calling flat() to prevent runtime errors when
options include strings or objects. Ensure newValue is always an array of strings
prior to updating state.

packages/client/src/pages/jobs/JobTypesSendEmailBuilder.tsx [22-26]

 onChange={(_, newValue) => {
-	setBuilderField("tags", newValue.flat());
+	const values = Array.isArray(newValue) ? (newValue as unknown[]).flat() : [];
+	setBuilderField("tags", values as string[]);
 }}
Suggestion importance[1-10]: 6

__

Why: The PR changes onChange handlers to use newValue.flat(), which can throw if newValue isn't an array; guarding improves robustness with minimal overhead and aligns with varied option shapes.

Low
Harden switch change handler

Guard the onChange handler against non-input event types to prevent runtime errors
if the UI library dispatches a generic event. Use a type-agnostic handler and safely
read checked using a type assertion.

packages/client/src/components/blocks-workspace/blocks/settings/custom/e-charts/variant/bar-chart/DataTabStyling.tsx [663-670]

 <Switch
     checked={checkedInstruction}
-    onChange={(event: React.ChangeEvent<HTMLInputElement>) =>
-        setCheckedInstruction(event.target.checked)
+    onChange={(event) =>
+        setCheckedInstruction(
+            (event.target as HTMLInputElement)?.checked ?? false
+        )
     }
-    size='small'
+    size="small"
     inputProps={{ "aria-label": "controlled" }}
 />
Suggestion importance[1-10]: 5

__

Why: The guard is a minor robustness improvement; current code already types the event, but the safer cast can prevent edge-case runtime issues without changing behavior.

Low
General
Robust option label fallback

Ensure getOptionLabel never returns an empty string to avoid blank chips/labels when
the component passes a string during input. Fallback to String(option) for
non-object cases.

packages/client/src/components/blocks-workspace/blocks/block-settings/radio-block/config.tsx [92-109]

 <Autocomplete
     fullWidth
     multiple={false}
     options={options}
     value={options.find((opt) => opt.value === selectedValue) || null}
     onChange={(_, newValue) => {
-        setBlockData(typeof newValue === 'object' && newValue !== null ? newValue.value : undefined);
+        setBlockData(
+            typeof newValue === 'object' && newValue !== null
+                ? newValue.value
+                : undefined
+        );
     }}
-    getOptionLabel={(option) => typeof option === 'object' && option !== null ? option.label : ''}
+    getOptionLabel={(option) =>
+        typeof option === 'object' && option !== null
+            ? option.label
+            : String(option ?? '')
+    }
     isOptionEqualToValue={(option, value) =>
         typeof option === 'object' &&
         option !== null &&
         typeof value === 'object' &&
         value !== null &&
         'value' in option &&
         'value' in value &&
         option.value === value.value
     }
Suggestion importance[1-10]: 6

__

Why: Ensuring getOptionLabel returns a non-empty string improves UX and stability; change aligns with Autocomplete behavior and is accurate to the diff.

Low

Previous suggestions

Suggestions up to commit 8466a24
CategorySuggestion                                                                                                                                    Impact
Possible issue
Differentiate Exclude value

The Exclude action is using the same value as Filter, causing ambiguity when
handling menu events. Change the value prop to "exclude" so that Exclude and Filter
can be distinguished.

libs/renderer/src/components/block-defaults/echart-visualization-block/CustomContextMenu.tsx [72-79]

 <MenuItem
     dense={true}
-    value={"filter"}
+    value={"exclude"}
     onClick={() => {
         // ...
         onClose();
     }}
 >
     Exclude
 </MenuItem>
Suggestion importance[1-10]: 8

__

Why: The value prop is incorrectly set to "filter" for the Exclude menu item, causing both actions to be indistinguishable; updating to "exclude" fixes this functional bug.

Medium
Use valid chip palette colors

The color prop on MUI Chip only accepts palette keys like 'success' or 'error', not
raw color names. Update the ternary to use the valid palette values to restore the
intended styling and avoid runtime warnings.

packages/client/src/pages/jobs/HistoryRow.tsx [54-58]

 <Chip
     label={row.success ? 'Success' : 'Failed'}
     avatar={null}
     variant="filled"
-    color={row.success ? 'green' : 'red'}
+    color={row.success ? 'success' : 'error'}
 />
Suggestion importance[1-10]: 8

__

Why: The MUI Chip color prop expects palette keys like success or error, not raw color names, so changing to valid values prevents runtime warnings and ensures consistent styling.

Medium
Fix contextMenu value check

The code checks typeof contextMenu.value === "string", but contextMenu.value is an
object with a value field. Update the check to inspect contextMenu.value.value to
correctly branch on string vs object.

libs/renderer/src/components/block-defaults/echart-visualization-block/variant/bar-chart/ChartContextMenu.tsx [179-181]

-{typeof contextMenu.value === "string"
-    ? contextMenu.value
+{typeof contextMenu.value.value === "string"
+    ? contextMenu.value.value
     : JSON.stringify(contextMenu.value.value)}
Suggestion importance[1-10]: 7

__

Why: The code checks typeof contextMenu.value but the actual payload is an object with a value field, so switching to typeof contextMenu.value.value correctly handles string vs object cases.

Medium
General
Add option equality check

Without a custom equality check, the autocomplete may fail to match selected objects
from its options. Add isOptionEqualToValue to compare objects by their value field
and ensure the selection stays in sync.

packages/client/src/components/blocks-workspace/blocks/settings/custom/grid-two/operations/ColorByValue.tsx [317-334]

 <Autocomplete
     fullWidth
     size="small"
     multiple={false}
     value={
         columnComparision.find(
             (c) => c.value === editingRule?.comparator,
         ) ?? null
     }
     onChange={(_, newValue) => {
         updateFields(
             'comparator',
             typeof newValue === 'object' && newValue !== null ? newValue.value : ''
         );
     }}
     options={columnComparision}
     getOptionLabel={(option) =>
         typeof option === 'string' ? option : option.name
     }
+    isOptionEqualToValue={(option, value) => option.value === value.value}
     renderInput={(params) => (
         <TextField {...params} label="Comparator" size="small" />
     )}
 />
Suggestion importance[1-10]: 7

__

Why: Defining isOptionEqualToValue ensures the Autocomplete correctly matches complex option objects by their value, preventing selection sync issues.

Medium
Use valid red border color

An empty string for borderColor generates invalid CSS. Use a valid color (e.g. the
error palette) or omit the property to avoid rendering issues.

libs/ui/src/components/Chip/Chip.tsx [170-183]

 ...(chipColor === "red" && {
     backgroundColor: theme.palette.error.main,
     color: palette.text.white,
-    borderColor: "",
+    borderColor: theme.palette.error.dark,
     ".MuiSvgIcon-root": {
         color: palette.text.white,
     },
     "& .MuiChip-deleteIcon": {
         color: palette.text.white,
     },
     "&&:hover": {
-        backgroundColor: theme.palette.error.main,
+        backgroundColor: theme.palette.error.dark,
     },
 }),
Suggestion importance[1-10]: 5

__

Why: Defining borderColor: "" emits invalid CSS and may break styling; using a valid color like theme.palette.error.dark ensures consistent chip borders.

Low

@Gowrishankar-Palanisamy Gowrishankar-Palanisamy marked this pull request as draft August 8, 2025 09:03
@Gowrishankar-Palanisamy Gowrishankar-Palanisamy marked this pull request as ready for review August 11, 2025 06:23
@johbaxter
Copy link
Copy Markdown
Contributor

image

You removed quite a bit of them as was intended. But we left a couple out that we should probably make the component files for.

Create semoss/ui components for leftovers in index.ts

@johbaxter johbaxter merged commit 535559e into dev Aug 12, 2025
3 checks passed
@johbaxter johbaxter deleted the feat/1571-remove-extra-components branch August 12, 2025 19:17
@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /update_changelog

@QodoAI-Agent
Copy link
Copy Markdown

Changelog updates: 🔄

2025-08-12 *

Changed

  • Removed duplicate UI components with names ending in "Two" and updated usages to standard components
  • Refined common components and adjusted related settings and context menus accordingly

to commit the new content to the CHANGELOG.md file, please type:
'/update_changelog --pr_update_changelog.push_changelog_changes=true'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove extra components from @semoss/ui

3 participants