packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.js (2)

29-38: Constants can be hoisted; tableContainerRef appears unused.

MIN_H and MAX_H are stable values—consider moving them outside the component to avoid redefinition on each render. Also, tableContainerRef is assigned via scrollerRef but never consumed elsewhere in this file.

Suggested refactor

+const MIN_H = 50;
+const MAX_H = 650;
+
 const EnvironmentVariables = ({ environment, setIsModified, collection }) => {
   const dispatch = useDispatch();
   const { storedTheme } = useTheme();
   const { globalEnvironments, activeGlobalEnvironmentUid } = useSelector((state) => state.globalEnvironments);

   const environmentsDraft = collection?.environmentsDraft;
   const hasDraftForThisEnv = environmentsDraft?.environmentUid === environment.uid;
-  const MIN_H = 50;
-  const MAX_H = 650;
-
-  const tableContainerRef = React.useRef(null);
   const [tableHeight, setTableHeight] = React.useState(MIN_H);

---

405-413: Consider using <th> for header cells.

For semantic HTML and accessibility, table headers should use <th> elements rather than <td>.

Suggested change

             fixedHeaderContent={() => (
               <tr>
-                <td className="text-center"></td>
-                <td>Name</td>
-                <td>Value</td>
-                <td className="text-center">Secret</td>
-                <td></td>
+                <th className="text-center"></th>
+                <th>Name</th>
+                <th>Value</th>
+                <th className="text-center">Secret</th>
+                <th></th>
               </tr>
             )}

packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.js (2)

31-40: Same refactor opportunity as the sibling component.

Similar to the EnvironmentSettings variant: MIN_H and MAX_H could be hoisted outside the component, and tableContainerRef is assigned but never consumed.

---

343-351: Use <th> for header cells for consistency.

Same recommendation as the sibling component—semantic headers improve accessibility.

Suggested change

             fixedHeaderContent={() => (
               <tr>
-                <td className="text-center"></td>
-                <td>Name</td>
-                <td>Value</td>
-                <td className="text-center">Secret</td>
-                <td></td>
+                <th className="text-center"></th>
+                <th>Name</th>
+                <th>Value</th>
+                <th className="text-center">Secret</th>
+                <th></th>
               </tr>
             )}

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.

Learnt from: kanakkholwal
Repo: usebruno/bruno PR: 6767
File: packages/bruno-app/src/components/ResponseExample/index.js:221-226
Timestamp: 2026-01-09T18:25:14.640Z
Learning: In the Bruno Electron renderer code (packages/bruno-app), assume window.ipcRenderer is always available and skip existence checks. Do not guard for ipcRenderer in this Electron context; use window.ipcRenderer directly (e.g., window.ipcRenderer.send(...), window.ipcRenderer.on(...)). If there are non-Electron contexts (such as test environments or non-Electron builds), add guards or mocks to avoid runtime errors there, but for the intended Electron renderer files, this pattern should be applied broadly within packages/bruno-app.

packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.js (2)

25-29: TableRow memoization has limited effectiveness.

The custom comparison function checks prevProps.children === nextProps.children via reference equality. Since itemContent recreates children on each render, this comparison will almost always return false, defeating the memoization purpose. Also, the key prop inside the returned <tr> is ineffective—keys are handled at the parent level, not within the component.

Consider simplifying or removing the custom comparison if children are always recreated.

♻️ Suggested simplification

-const TableRow = React.memo(({ children, item }) => <tr key={item.uid} data-testid={`env-var-row-${item.name}`}>{children}</tr>, (prevProps, nextProps) => {
-  const prevUid = prevProps?.item?.uid;
-  const nextUid = nextProps?.item?.uid;
-  return prevUid === nextUid && prevProps.children === nextProps.children;
-});
+const TableRow = React.memo(({ children, item }) => (
+  <tr data-testid={`env-var-row-${item?.name}`}>{children}</tr>
+));

---

347-355: Use <th> for header cells instead of <td>.

Semantic HTML: header cells should use <th> elements for accessibility and proper table semantics.

♻️ Proposed fix

         fixedHeaderContent={() => (
           <tr>
-            <td className="text-center"></td>
-            <td>Name</td>
-            <td>Value</td>
-            <td className="text-center">Secret</td>
-            <td></td>
+            <th className="text-center"></th>
+            <th>Name</th>
+            <th>Value</th>
+            <th className="text-center">Secret</th>
+            <th></th>
           </tr>
         )}

packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.js (2)

22-26: Same TableRow memoization concern as the other file.

Reference comparison on children won't be effective since children are recreated each render. The key prop inside the <tr> is also ineffective.

♻️ Suggested simplification

-const TableRow = React.memo(({ children, item }) => <tr key={item.uid} data-testid={`env-var-row-${item.name}`}>{children}</tr>, (prevProps, nextProps) => {
-  const prevUid = prevProps?.item?.uid;
-  const nextUid = nextProps?.item?.uid;
-  return prevUid === nextUid && prevProps.children === nextProps.children;
-});
+const TableRow = React.memo(({ children, item }) => (
+  <tr data-testid={`env-var-row-${item?.name}`}>{children}</tr>
+));

---

408-416: Use <th> for header cells instead of <td>.

Same semantic HTML issue as the other file—header cells should use <th> elements.

♻️ Proposed fix

         fixedHeaderContent={() => (
           <tr>
-            <td className="text-center"></td>
-            <td>Name</td>
-            <td>Value</td>
-            <td className="text-center">Secret</td>
-            <td></td>
+            <th className="text-center"></th>
+            <th>Name</th>
+            <th>Value</th>
+            <th className="text-center">Secret</th>
+            <th></th>
           </tr>
         )}

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.

Learnt from: kanakkholwal
Repo: usebruno/bruno PR: 6767
File: packages/bruno-app/src/components/ResponseExample/index.js:221-226
Timestamp: 2026-01-09T18:25:14.640Z
Learning: In the Bruno Electron renderer code (packages/bruno-app), assume window.ipcRenderer is always available and skip existence checks. Do not guard for ipcRenderer in this Electron context; use window.ipcRenderer directly (e.g., window.ipcRenderer.send(...), window.ipcRenderer.on(...)). If there are non-Electron contexts (such as test environments or non-Electron builds), add guards or mocks to avoid runtime errors there, but for the intended Electron renderer files, this pattern should be applied broadly within packages/bruno-app.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}