packages/bruno-app/src/components/EditableTable/StyledWrapper.js (1)

22-44: Excessive !important declarations.

Multiple !important rules in thead styling (lines 23, 25, 26, 31, 35, 36) suggest possible specificity conflicts. Consider reviewing CSS cascade to reduce reliance on !important, which can make future maintenance harder.

packages/bruno-app/src/components/CollectionSettings/Vars/VarsTable/index.js (1)

32-60: Consider memoizing the columns array.

The columns array contains render functions that depend on storedTheme, onSave, collection, and varType. It's recreated each render. Wrapping with useMemo would prevent unnecessary re-renders in EditableTable.

-  const columns = [
+  const columns = useMemo(() => [
     {
       key: 'name',
       ...
     },
     {
       key: 'value',
       ...
     }
-  ];
+  ], [storedTheme, onSave, collection, varType]);

packages/bruno-app/src/components/RequestPane/RequestHeaders/index.js (1)

45-84: Consider memoizing columns.

Similar to VarsTable, the columns array with render functions is recreated each render. Memoizing with useMemo would optimize re-renders.

packages/bruno-app/src/components/CollectionSettings/Headers/index.js (2)

34-69: Consider memoizing columns.

Same optimization opportunity as other EditableTable usages in this PR.

---

98-103: Consider adding reorderable support for consistency.

RequestHeaders component includes reorderable={true} with drag support, but this collection-level Headers component doesn't. If header ordering matters at the collection level, consider adding the same capability.

       <EditableTable
         columns={columns}
         rows={headers}
         onChange={handleHeadersChange}
         defaultRow={defaultRow}
+        reorderable={true}
+        onReorder={handleHeaderDrag}
       />

packages/bruno-app/src/components/RequestPane/Assertions/index.js (1)

86-108: Unused parameters in render function.

The value and isLastEmptyRow parameters are destructured but never used in the operator column's render function.

-      render: ({ row, value, onChange, isLastEmptyRow }) => {
+      render: ({ row, onChange }) => {

packages/bruno-app/src/components/RequestPane/QueryParams/index.js (1)

167-179: No-op onChange for path params is intentional but worth documenting.

The onChange={() => {}} is correct here since path param value changes are handled via handlePathParamChange in the column render function, and the table doesn't allow adding/deleting rows. Consider adding a brief comment to clarify this isn't an oversight.

         {pathParams && pathParams.length > 0 ? (
           <EditableTable
             columns={pathColumns}
             rows={pathParams}
-            onChange={() => {}}
+            onChange={() => {}} // Path param changes handled via render callback
             defaultRow={{}}
             showCheckbox={false}
             showDelete={false}
             showAddRow={false}
           />

packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (1)

76-84: Unused onChange parameter in handleValueChange.

The onChange parameter is passed but never used since the function dispatches directly via handleParamsChange.

-  const handleValueChange = useCallback((row, newValue, onChange) => {
+  const handleValueChange = useCallback((row, newValue) => {

Also update the call site at line 144:

-                onChange={(newValue) => handleValueChange(row, newValue, onChange)}
+                onChange={(newValue) => handleValueChange(row, newValue)}

packages/bruno-app/src/components/RequestPane/FormUrlEncodedParams/index.js (1)

66-70: Missing enabled field in defaultRow.

The EditableTable component defaults checkboxKey to 'enabled', but defaultRow doesn't include it. This could cause inconsistent state when new rows are created. The Redux reducer setFormUrlEncodedParams defaults enabled to true, so it works, but adding it here improves clarity.

 const defaultRow = {
   name: '',
   value: '',
-  description: ''
+  description: '',
+  enabled: true
 };

packages/bruno-app/src/components/RequestPane/Vars/VarsTable/index.js (1)

79-83: Missing enabled field in defaultRow.

Same issue as FormUrlEncodedParams - defaultRow should include enabled: true for consistency with the EditableTable checkbox behavior.

 const defaultRow = {
   name: '',
   value: '',
+  enabled: true,
   ...(varType === 'response' ? { local: false } : {})
 };

packages/bruno-app/src/components/EditableTable/index.js (2)

108-116: Filter logic could exclude valid rows with falsy key values.

The condition keyVal && (typeof keyVal !== 'string' || keyVal.trim() !== '') filters out rows where keyVal is any falsy value (0, false, empty string). If a numeric key field could legitimately be 0, this would incorrectly filter it out.

Given the current usage context (string-based keys like "name"), this is unlikely to cause issues, but worth noting for future use cases.

-const result = updatedRows.filter((row, i) => {
-  if (showAddRow && i === updatedRows.length - 1) return false;
-  if (!showAddRow) return true;
-  const keyVal = keyColumn ? (keyColumn.getValue ? keyColumn.getValue(row) : row[keyColumn.key]) : '';
-  return keyVal && (typeof keyVal !== 'string' || keyVal.trim() !== '');
-});
+const result = updatedRows.filter((row, i) => {
+  if (showAddRow && i === updatedRows.length - 1) return false;
+  if (!showAddRow) return true;
+  const keyVal = keyColumn ? (keyColumn.getValue ? keyColumn.getValue(row) : row[keyColumn.key]) : '';
+  if (keyVal === null || keyVal === undefined) return false;
+  if (typeof keyVal === 'string') return keyVal.trim() !== '';
+  return true;
+});

---

26-34: createEmptyRow spreads defaultRow after setting checkboxKey.

If defaultRow contains a property matching checkboxKey (typically enabled), it will override the [checkboxKey]: true set on line 31. This is likely fine if defaultRow always sets enabled: true, but could cause unexpected behavior if a consumer passes enabled: false in defaultRow.

Consider spreading defaultRow first to allow explicit override:

 const createEmptyRow = useCallback(() => {
   const newUid = uuid();
   emptyRowUidRef.current = newUid;
   return {
+    ...defaultRow,
     uid: newUid,
-    [checkboxKey]: true,
-    ...defaultRow
+    [checkboxKey]: true
   };
 }, [defaultRow, checkboxKey]);

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:32.975Z
Learning: Applies to **/*.{jsx,tsx} : Styled Components are used as wrappers to define both self and children components style; Tailwind classes are used specifically for layout based styles

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:32.975Z
Learning: Applies to **/*.{jsx,tsx} : Styled Component CSS might also change layout but Tailwind classes shouldn't define colors

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:32.975Z
Learning: Applies to **/*.{jsx,tsx} : Use styled component's theme prop to manage CSS colors and not CSS variables when in the context of a styled component or any React component using the styled component

packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (1)

1401-1420: setMultipartFormParams missing description field in mapping.

This was flagged in a previous review and appears unaddressed. Other form param actions (e.g., setFormUrlEncodedParams at line 1311) include description. This could cause data loss:

-item.draft.request.body.multipartForm = map(params, ({ uid, name = '', value = '', contentType = '', type = 'text', enabled = true }) => ({
+item.draft.request.body.multipartForm = map(params, ({ uid, name = '', value = '', contentType = '', type = 'text', description = '', enabled = true }) => ({
   uid: uid || uuid(),
   name,
   value,
   contentType,
   type,
+  description,
   enabled
 }));

packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (2)

69-77: Potential stale closure on params in handleClearFile.

Same pattern as the fixed handleBrowseFiles - if the user makes changes before this callback runs, params could be outdated. Consider reading fresh state from item:

 const handleClearFile = useCallback((row) => {
+  const currentParams = item.draft
+    ? get(item, 'draft.request.body.multipartForm')
+    : get(item, 'request.body.multipartForm');
-  const updatedParams = params.map((p) => {
+  const updatedParams = (currentParams || []).map((p) => {
     if (p.uid === row.uid) {
       return { ...p, type: 'text', value: '' };
     }
     return p;
   });
   handleParamsChange(updatedParams);
-}, [params, handleParamsChange]);
+}, [item, handleParamsChange]);

---

79-87: Same stale closure concern and unused onChange parameter.

The params closure risk applies here too. Additionally, onChange is passed but never used - remove it to avoid confusion:

-const handleValueChange = useCallback((row, newValue, onChange) => {
-  const updatedParams = params.map((p) => {
+const handleValueChange = useCallback((row, newValue) => {
+  const currentParams = item.draft
+    ? get(item, 'draft.request.body.multipartForm')
+    : get(item, 'request.body.multipartForm');
+  const updatedParams = (currentParams || []).map((p) => {
     if (p.uid === row.uid) {
       return { ...p, type: 'text', value: newValue };
     }
     return p;
   });
   handleParamsChange(updatedParams);
-}, [params, handleParamsChange]);
+}, [item, handleParamsChange]);

Update the caller at line 147 accordingly:

-onChange={(newValue) => handleValueChange(row, newValue, onChange)}
+onChange={(newValue) => handleValueChange(row, newValue)}

packages/bruno-app/src/components/EditableTable/index.js (1)

81-116: Consider simplifying the filter logic.

The filtering at lines 108-113 is complex. It filters out empty rows to pass to onChange, but the conditions are hard to follow. Consider extracting to a named function for clarity:

const filterEmptyTrailingRows = (rows) => {
  const keyColumn = columns.find((col) => col.isKeyField);
  return rows.filter((row, i) => {
    if (showAddRow && i === rows.length - 1) return false;
    if (!keyColumn) return true;
    const keyVal = keyColumn.getValue ? keyColumn.getValue(row) : row[keyColumn.key];
    return keyVal && (typeof keyVal !== 'string' || keyVal.trim() !== '');
  });
};

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

packages/bruno-app/src/components/GlobalEnvironments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js (1)

60-64: Checkbox alignment tweak is a solid simplification

Centering via vertical-align: middle and resetting margin is cleaner and more predictable than positional nudging; this should align with the new table styling well.

If you later see browser-specific misalignment, consider also normalizing line-height on the checkbox container rather than the input itself, but it’s not necessary for this PR.

packages/bruno-app/src/components/WorkspaceHome/StyledWrapper.js (1)

99-99: Consider refactoring CSS variable to theme prop for consistency.

Line 99 uses a CSS variable for color, while the rest of the file uses theme props. Based on learnings, styled components should use theme props for colors.

Apply this diff for consistency:

-    color: var(--color-tab-inactive);
+    color: ${(props) => props.theme.tabs.inactive.color};

Note: Verify the correct theme path exists for inactive tab color.

packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/StyledWrapper.js (1)

1-133: Theme usage and layout in StyledWrapper look solid.

The wrapper correctly uses props.theme for all colors/typography and mirrors the workspace rename/error patterns, so it should integrate cleanly with the existing UI. There is some duplication with WorkspaceHome/StyledWrapper (e.g., .inline-actions, .inline-action-btn, error tooltip), which could be factored into a shared helper later if this pattern spreads further.

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

21-27: Environment variables table styling aligns with EditableTable patterns.

Updating td padding/vertical-align, header color/font-size, and input/checkbox alignment makes this table visually consistent with the new EditableTable styles and keeps colors theme-driven. If more tables converge on this look, consider sharing more of this styling with the generic EditableTable StyledWrapper to avoid drift.

Also applies to: 41-45, 103-109, 119-126

packages/bruno-app/src/components/RequestPane/Assertions/index.js (2)

12-52: Consolidate assertion operator helpers to avoid duplication.

unaryOperators, the full operators list inside parseAssertionOperator, and the operator options in AssertionOperator all encode essentially the same sets. Keeping these in one shared module (e.g., assertionOperators.js) would reduce drift when adding/removing operators and make tests easier to maintain.

---

142-147: EditableTable wiring for assertions looks appropriate; consider cleaning up the extra operator field.

Passing columns, rows={assertions || []}, defaultRow (with value: 'eq '), and reorderable/onReorder into EditableTable matches the intended API and should work well with drag-reorder and empty-row handling. The persisted operator is now encoded inside value ("operator value"), so the separate operator: 'eq' field in defaultRow appears unused; you might be able to drop or repurpose it to avoid confusion about the source of truth.

Also applies to: 150-157

packages/bruno-app/src/components/EditableTable/index.js (3)

36-79: Clarify or enforce the requirement for an isKeyField column.

rowsWithEmpty, isEmptyRow, isLastEmptyRow, and the final result filter all assume there is a column with isKeyField: true. If no such column exists, keyColumn is undefined, and the filter will effectively drop all rows when showAddRow is true. At minimum, it’s worth documenting that EditableTable requires exactly one key column; alternatively, you could add a runtime guard (e.g., console.warn or throwing in dev) when !columns.some(c => c.isKeyField).

---

81-116: Last empty row drops non-key edits; consider whether that UX is acceptable.

handleValueChange only promotes the last row into a “real” row when the key field changes from empty to non-empty. Edits to other columns on the last row are applied to updatedRows but then filtered out (the last row is removed from result and recreated from defaultRow on the next render). That means typing into a non-key field of the add-row is effectively discarded.

If you want to allow users to fill non-key fields first, you could:

• Treat a row as “non-empty” when any of a configurable set of columns has data, or
• Skip the i === updatedRows.length - 1 filter when !isEmptyRow(row) so partial edits on the last row are preserved.

As-is, it’s a reasonable tradeoff if the intended flow is “enter key column first,” but it’s worth confirming this matches UX expectations.

---

24-25: Remove unused dragStart state to reduce noise.

dragStart is set in handleDragStart and cleared in handleDragEnd, but never read. You can safely drop this state (and its setter) to avoid unnecessary state updates during drag-and-drop.

Also applies to: 132-136, 158-161

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.{jsx,tsx} : Styled Components are used as wrappers to define both self and children components style; Tailwind classes are used specifically for layout based styles

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.{jsx,tsx} : Styled Component CSS might also change layout but Tailwind classes shouldn't define colors

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.{jsx,tsx} : Use styled component's theme prop to manage CSS colors and not CSS variables when in the context of a styled component or any React component using the styled component

packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (1)

1404-1423: setMultipartFormParams missing description field in mapping.

Other form param actions (e.g., setFormUrlEncodedParams at line 1314) include description in the mapping. This omission causes data loss when updating multipart form params.

-item.draft.request.body.multipartForm = map(params, ({ uid, name = '', value = '', contentType = '', type = 'text', enabled = true }) => ({
+item.draft.request.body.multipartForm = map(params, ({ uid, name = '', value = '', contentType = '', type = 'text', description = '', enabled = true }) => ({
   uid: uid || uuid(),
   name,
   value,
   contentType,
   type,
+  description,
   enabled
 }));

packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (1)

64-66: Silent failure on file browse error.

When browseFiles() fails, the error is only logged to console. Users won't know the operation failed.

Consider adding a toast notification:

 .catch((error) => {
   console.error(error);
+  // Add user-facing feedback, e.g.:
+  // toast.error('Failed to select files');
 });

packages/bruno-app/src/components/RequestPane/Assertions/index.js (2)

91-93: Redundant parsing of row.value.

parseAssertionOperator(row.value) is called twice in succession (lines 92 and 93). Destructure both values in one call:

-        const { operator } = parseAssertionOperator(row.value);
-        const assertionValue = parseAssertionOperator(row.value).value;
+        const { operator, value: assertionValue } = parseAssertionOperator(row.value);

---

151-155: operator field in defaultRow appears unused.

The assertion data model stores the operator as a prefix in the value field (e.g., "eq someValue"). The standalone operator property in defaultRow isn't persisted and may cause confusion.

 const defaultRow = {
   name: '',
-  value: 'eq ',
-  operator: 'eq'
+  value: 'eq '
 };

packages/bruno-app/src/components/EditableTable/index.js (2)

109-118: hasAnyValue may incorrectly filter rows with intentional default values.

If defaultRow contains meaningful defaults (e.g., type: 'text'), comparing val !== defaultVal will treat rows with those defaults as "having a value" and persist them prematurely.

Consider checking only the key field or columns explicitly marked as significant:

 const hasAnyValue = (row) => {
-  for (const col of columns) {
-    const val = col.getValue ? col.getValue(row) : row[col.key];
-    const defaultVal = defaultRow[col.key];
-    if (val && val !== defaultVal && (typeof val !== 'string' || val.trim() !== '')) {
-      return true;
-    }
-  }
-  return false;
+  const keyColumn = columns.find((col) => col.isKeyField);
+  if (!keyColumn) return false;
+  const val = keyColumn.getValue ? keyColumn.getValue(row) : row[keyColumn.key];
+  return val && (typeof val !== 'string' || val.trim() !== '');
 };

---

265-282: Drag handle lacks keyboard accessibility.

Drag-and-drop reordering is mouse-only. For accessibility, consider adding keyboard-based reorder (e.g., up/down arrow keys on focused rows). This is a nice-to-have for future improvement.

packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (1)

194-199: defaultRow missing description field.

For consistency with the data model and other form param components, include description:

 const defaultRow = {
   name: '',
   value: '',
   contentType: '',
-  type: 'text'
+  type: 'text',
+  description: ''
 };

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

tests/request/newlines/newlines-persistence.spec.ts (1)

32-51: Add assertions to verify rows exist before interaction.

The test now uses .first() to select pre-existing rows for params, headers, and vars without verifying they exist. This could lead to test failures if the tables don't have default rows.

Consider adding visibility assertions before interaction:

  await page.getByRole('tab', { name: 'Params' }).click();
  const paramRow = page.locator('table tbody tr').first();
+ await expect(paramRow).toBeVisible();
  await getTableCell(paramRow, 0).locator('input[type="text"]').fill('queryParamKey');

  await page.getByRole('tab', { name: 'Headers' }).click();
  const headerRow = page.locator('table tbody tr').first();
+ await expect(headerRow).toBeVisible();
  await getTableCell(headerRow, 0).locator('.CodeMirror').click();

tests/variable-tooltip/variable-tooltip.spec.ts (1)

70-77: Improved selector scoping.

The new pattern using headerTable -> headerRow -> editor is cleaner and more specific than selecting table rows globally. This reduces the risk of selecting unintended elements if multiple tables are present.

However, the same concern applies here: the test assumes the first row exists in the header table. Consider adding:

await expect(headerRow).toBeVisible();

before interacting with its editors.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created

tests/collection/create/create-collection.spec.ts (1)

32-32: Good locator scoping; factor out a reusable locator variable

Scoping the textarea under #request-url is a solid improvement and should be more robust than the old generic textarea selector. To cut duplication and better follow the “use locator variables” guideline, consider extracting this into a local locator:

   // Set the URL
-  await page.locator('#request-url .CodeMirror').click();
-  await page.locator('#request-url').locator('textarea').fill('http://localhost:8081');
+  const requestUrlTextarea = page.locator('#request-url textarea');
+  await page.locator('#request-url .CodeMirror').click();
+  await requestUrlTextarea.fill('http://localhost:8081');
@@
   // Send a request
-  await page.locator('#request-url .CodeMirror').click();
-  await page.locator('#request-url').locator('textarea').fill('/ping');
+  await page.locator('#request-url .CodeMirror').click();
+  await requestUrlTextarea.fill('/ping');

If you want to double‑check best practices for locator reuse in Playwright 1.51.x, please confirm against the current Playwright docs.

Also applies to: 37-37

tests/collection/moving-requests/cross-collection-drag-drop-request.spec.ts (1)

17-21: Scoped URL textarea locator is solid; consider DRYing via a locator variable

The new page.locator('#request-url').locator('textarea') is a clear improvement over a generic textarea and should be more stable. To better follow the guideline of using locator variables and to avoid repeating the #request-url selector, consider extracting a locator per test and reusing it for both click and fill:

@@
-    // Set the URL
-    await page.locator('#request-url .CodeMirror').click();
-    await page.locator('#request-url').locator('textarea').fill('https://echo.usebruno.com');
+    // Set the URL
+    const requestUrl = page.locator('#request-url');
+    await requestUrl.locator('.CodeMirror').click();
+    await requestUrl.locator('textarea').fill('https://echo.usebruno.com');
@@
-    // Set the URL
-    await page.locator('#request-url .CodeMirror').click();
-    await page.locator('#request-url').locator('textarea').fill('https://echo.usebruno.com');
+    // Set the URL
+    const sourceRequestUrl = page.locator('#request-url');
+    await sourceRequestUrl.locator('.CodeMirror').click();
+    await sourceRequestUrl.locator('textarea').fill('https://echo.usebruno.com');
@@
-    // Set the URL
-    await page.locator('#request-url .CodeMirror').click();
-    await page.locator('#request-url').locator('textarea').fill('https://echo.usebruno.com');
+    // Set the URL
+    const targetRequestUrl = page.locator('#request-url');
+    await targetRequestUrl.locator('.CodeMirror').click();
+    await targetRequestUrl.locator('textarea').fill('https://echo.usebruno.com');

Also applies to: 74-78, 88-92

tests/collection/moving-tabs/move-tabs.spec.ts (1)

53-55: Selector refinement looks good; consider using a locator variable for reuse

Scoping the textarea fill to #request-url is an improvement over a broad textarea selector. To align with the Playwright guideline of using locator variables and to avoid duplicating the selector across both tests, you could capture the locator once in each test and reuse it:

@@
-    await page.locator('#request-url').locator('textarea').fill('https://echo.usebruno.com');
+    const requestUrlTextarea = page.locator('#request-url textarea');
+    await requestUrlTextarea.fill('https://echo.usebruno.com');
@@
-    await page.locator('#request-url').locator('textarea').fill('https://echo.usebruno.com');
+    const requestUrlTextarea = page.locator('#request-url textarea');
+    await requestUrlTextarea.fill('https://echo.usebruno.com');

This keeps the selector DRY and makes future selector changes easier in both tests.

Also applies to: 143-146

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

tests/utils/page/locators.ts (1)

126-126: Add type annotations for parameters.

The row and index parameters lack type annotations. Consider adding them for better type safety:

-export const getTableCell = (row, index) => row.locator('td').nth(index + 1);
+export const getTableCell = (row: any, index: number) => row.locator('td').nth(index + 1);

(Replace any with the appropriate Playwright Locator type if available in your imports.)

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro