Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/fe/refactor/input #1174

Merged
merged 4 commits into from
Nov 26, 2024
Merged

Feat/fe/refactor/input #1174

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3cc4402
Add route for test page
ajhollid Nov 21, 2024
1b51e58
update mui
ajhollid Nov 21, 2024
e7fd439
Rewrite Field component as TextInput, extract styles to theme
ajhollid Nov 21, 2024
2e2ce43
Merge branch 'develop' into feat/fe/refactor/input
ajhollid Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 153 additions & 18 deletions Client/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Client/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"@fontsource/roboto": "^5.0.13",
"@mui/icons-material": "^5.15.17",
"@mui/lab": "^5.0.0-alpha.170",
"@mui/material": "^5.16.7",
"@mui/material": "6.1.8",
"@mui/x-charts": "^7.5.1",
"@mui/x-data-grid": "7.22.2",
"@mui/x-date-pickers": "7.22.2",
Expand Down
6 changes: 6 additions & 0 deletions Client/src/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import { logger } from "./Utils/Logger"; // Import the logger
import { networkService } from "./main";
import { Infrastructure } from "./Pages/Infrastructure";
import InfrastructureDetails from "./Pages/Infrastructure/Details";
import Test from "./Pages/test";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Yo, this test route needs some protection sauce! 🍝

Looking at App.jsx, the test route is completely exposed without any ProtectedRoute wrapper, unlike other routes in the app. This is a security concern since it's testing sensitive form components.

  • Line 93-97 in App.jsx needs to be wrapped with ProtectedRoute:
- <Route
-   path="/test"
-   element={<Test />}
- />
+ <Route
+   path="/test"
+   element={<ProtectedRoute Component={Test} />}
+ />
🔗 Analysis chain

Yo dawg, we need to protect that test route! 🍝

The Test component import looks good, but considering this is for testing the refactored Field component, we should ensure it's not accessible in production.

Let's check if there are any environment checks:

Consider wrapping this import with an environment check:

- import Test from "./Pages/test";
+ const Test = process.env.NODE_ENV === 'development' 
+   ? require('./Pages/test').default 
+   : () => null;

The previous search didn't work because we need to first locate the test file. Let's check its actual content and the route definition to understand how it's being used and whether there are any existing protections.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for environment-related code in the Test component
rg -g '!node_modules' -g '*/test.{jsx,tsx,js,ts}' 'process.env'

Length of output: 212

Script:

#!/bin/bash
# Let's first find the test file location
fd -e jsx -e js -e tsx -e ts "test" ./Client/src/Pages

# Then check its content
fd -e jsx -e js -e tsx -e ts "test" ./Client/src/Pages -x cat {}

# Also check the route definition in App.jsx
cat ./Client/src/App.jsx

Length of output: 12427

function App() {
const AdminCheckedRegister = withAdminCheck(Register);
const MonitorsWithAdminProp = withAdminProp(Monitors);
Expand Down Expand Up @@ -89,6 +90,11 @@ function App() {
path="/"
element={<HomeLayout />}
>
<Route
path="/test"
element={<Test />}
/>

Comment on lines +93 to +97
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Mom's spaghetti moment: Route needs protection! 🍝

The test route is currently exposed without any protection. Unlike other routes that use ProtectedRoute, this one's raw like fresh pasta!

Apply this change to protect the route:

     <Route
       path="/test"
-      element={<Test />}
+      element={
+        process.env.NODE_ENV === 'development' 
+          ? <ProtectedRoute Component={Test} />
+          : <Navigate to="/monitors" />
+      }
     />

Also, consider moving this route closer to other development-only routes for better organization.

📝 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.

Suggested change
<Route
path="/test"
element={<Test />}
/>
<Route
path="/test"
element={
process.env.NODE_ENV === 'development'
? <ProtectedRoute Component={Test} />
: <Navigate to="/monitors" />
}
/>

<Route
exact
path="/"
Expand Down
4 changes: 2 additions & 2 deletions Client/src/Components/Inputs/Field/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
}

.field-infrastructure-alert .MuiInputBase-root:has(input) {
height: var(--env-var-height-2);
/* height: var(--env-var-height-2); */
}

.field h3.MuiTypography-root,
Expand All @@ -23,7 +23,7 @@
padding-right: var(--env-var-spacing-1-minus);
}
.field .MuiInputBase-root:has(input) {
height: var(--env-var-height-2);
/* height: var(--env-var-height-2); */
}
.field .MuiInputBase-root:has(.MuiInputAdornment-root) {
padding-right: var(--env-var-spacing-1-minus);
Expand Down
8 changes: 4 additions & 4 deletions Client/src/Components/Inputs/Field/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const Field = forwardRef(
error,
disabled,
hidden,
className
className,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Yo dawg, let's talk about that className prop! 🍝

While adding the className prop is good for component flexibility, using it to control error visibility seems like an unexpected side effect. Consider separating these concerns.

Consider these improvements:

  1. Add a separate prop for error visibility control
  2. Simplify the className concatenation using template literals
- className={`${className ?? `field field-${type}`}`}
+ className={`field field-${type} ${className ?? ''}`}

Also applies to: 228-228

},
ref
) => {
Expand Down Expand Up @@ -186,11 +186,11 @@ const Field = forwardRef(
),
}}
/>
{ error && (
{error && (
<Typography
component="span"
className="input-error"
hidden={className? true: false}
hidden={className ? true : false}
color={theme.palette.error.main}
mt={theme.spacing(2)}
sx={{
Expand Down Expand Up @@ -225,7 +225,7 @@ Field.propTypes = {
error: PropTypes.string,
disabled: PropTypes.bool,
hidden: PropTypes.bool,
className: PropTypes.string
className: PropTypes.string,
};

export default Field;
60 changes: 60 additions & 0 deletions Client/src/Components/Inputs/TextInput/Adornments/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { Stack, Typography, InputAdornment, IconButton } from "@mui/material";
import { useTheme } from "@mui/material/styles";
import { useState } from "react";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Yo dawg, you've got an unused import there!

The useState import isn't being used in either component. Let's keep our code clean like mom's spaghetti!

-import { useState } from "react";
📝 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.

Suggested change
import { useState } from "react";

import PropTypes from "prop-types";
import VisibilityOff from "@mui/icons-material/VisibilityOff";
import Visibility from "@mui/icons-material/Visibility";
export const HttpAdornment = ({ https }) => {
const theme = useTheme();
return (
<Stack
direction="row"
alignItems="center"
height="100%"
sx={{
borderRight: `solid 1px ${theme.palette.border.dark}`,
backgroundColor: theme.palette.background.accent,
pl: theme.spacing(6),
}}
>
<Typography
component="h5"
paddingRight={"var(--env-var-spacing-1-minus)"}
color={theme.palette.text.secondary}
sx={{ lineHeight: 1, opacity: 0.8 }}
>
{https ? "https" : "http"}://
</Typography>
</Stack>
);
};

export const PasswordEndAdornment = ({ fieldType, setFieldType }) => {
const theme = useTheme();
return (
<InputAdornment position="end">
<IconButton
aria-label="toggle password visibility"
onClick={() => setFieldType(fieldType === "password" ? "text" : "password")}
sx={{
color: theme.palette.border.dark,
padding: theme.spacing(1),
"&:focus-visible": {
outline: `2px solid ${theme.palette.primary.main}`,
outlineOffset: `2px`,
},
"& .MuiTouchRipple-root": {
pointerEvents: "none",
display: "none",
},
}}
>
{fieldType === "password" ? <VisibilityOff /> : <Visibility />}
</IconButton>
</InputAdornment>
);
};
Comment on lines +32 to +56
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Lose yourself in the PropTypes, the moment, you better never let it go!

The component is missing PropTypes validation for fieldType and setFieldType. Also, since this component might be re-rendered frequently when the password field changes, we should optimize it.

Add PropTypes:

PasswordEndAdornment.propTypes = {
  fieldType: PropTypes.oneOf(['password', 'text']).isRequired,
  setFieldType: PropTypes.func.isRequired,
};

Optimize with memo:

+import { memo } from 'react';

-export const PasswordEndAdornment = ({ fieldType, setFieldType }) => {
+export const PasswordEndAdornment = memo(({ fieldType, setFieldType }) => {
   // ... component code ...
-};
+});


HttpAdornment.propTypes = {
https: PropTypes.bool.isRequired,
};
Loading