Improve tables design #6330
Improve tables design #6330
Conversation
naman-bruno
requested review from
bijin-bruno,
helloanoop and
lohit-bruno
as code owners
WalkthroughReplaces many bespoke per-item tables with a new generic EditableTable component and adds bulk Redux actions to replace entire collections of params/vars/headers/assertions; multiple components now use EditableTable with centralized onChange handlers instead of per-item actions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
packages/bruno-app/src/components/EditableTable/StyledWrapper.js (1)
22-44: Excessive!importantdeclarations.Multiple
!importantrules 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
columnsarray contains render functions that depend onstoredTheme,onSave,collection, andvarType. It's recreated each render. Wrapping withuseMemowould 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
columnsarray with render functions is recreated each render. Memoizing withuseMemowould 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.
RequestHeaderscomponent includesreorderable={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
valueandisLastEmptyRowparameters 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-oponChangefor path params is intentional but worth documenting.The
onChange={() => {}}is correct here since path param value changes are handled viahandlePathParamChangein 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: UnusedonChangeparameter inhandleValueChange.The
onChangeparameter is passed but never used since the function dispatches directly viahandleParamsChange.- 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: Missingenabledfield indefaultRow.The
EditableTablecomponent defaultscheckboxKeyto'enabled', butdefaultRowdoesn't include it. This could cause inconsistent state when new rows are created. The Redux reducersetFormUrlEncodedParamsdefaultsenabledtotrue, 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: Missingenabledfield indefaultRow.Same issue as FormUrlEncodedParams -
defaultRowshould includeenabled: truefor consistency with theEditableTablecheckbox 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 wherekeyValis any falsy value (0, false, empty string). If a numeric key field could legitimately be0, 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:createEmptyRowspreadsdefaultRowafter settingcheckboxKey.If
defaultRowcontains a property matchingcheckboxKey(typicallyenabled), it will override the[checkboxKey]: trueset on line 31. This is likely fine ifdefaultRowalways setsenabled: true, but could cause unexpected behavior if a consumer passesenabled: falseindefaultRow.Consider spreading
defaultRowfirst to allow explicit override:const createEmptyRow = useCallback(() => { const newUid = uuid(); emptyRowUidRef.current = newUid; return { + ...defaultRow, uid: newUid, - [checkboxKey]: true, - ...defaultRow + [checkboxKey]: true }; }, [defaultRow, checkboxKey]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/bruno-app/src/components/CollectionSettings/Headers/index.js(4 hunks)packages/bruno-app/src/components/CollectionSettings/Vars/VarsTable/index.js(1 hunks)packages/bruno-app/src/components/EditableTable/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/EditableTable/index.js(1 hunks)packages/bruno-app/src/components/FolderSettings/Headers/index.js(4 hunks)packages/bruno-app/src/components/FolderSettings/Vars/VarsTable/index.js(1 hunks)packages/bruno-app/src/components/RequestPane/Assertions/index.js(1 hunks)packages/bruno-app/src/components/RequestPane/FormUrlEncodedParams/index.js(1 hunks)packages/bruno-app/src/components/RequestPane/MultipartFormParams/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js(1 hunks)packages/bruno-app/src/components/RequestPane/QueryParams/index.js(4 hunks)packages/bruno-app/src/components/RequestPane/RequestHeaders/index.js(3 hunks)packages/bruno-app/src/components/RequestPane/Vars/VarsTable/index.js(1 hunks)packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js(13 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/EditableTable/StyledWrapper.jspackages/bruno-app/src/components/EditableTable/index.jspackages/bruno-app/src/components/RequestPane/RequestHeaders/index.jspackages/bruno-app/src/components/CollectionSettings/Headers/index.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/index.jspackages/bruno-app/src/components/RequestPane/Assertions/index.jspackages/bruno-app/src/components/RequestPane/QueryParams/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/components/FolderSettings/Headers/index.jspackages/bruno-app/src/components/CollectionSettings/Vars/VarsTable/index.jspackages/bruno-app/src/components/RequestPane/FormUrlEncodedParams/index.jspackages/bruno-app/src/components/FolderSettings/Vars/VarsTable/index.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/Vars/VarsTable/index.js
🧠 Learnings (3)
📚 Learning: 2025-12-05T20:31:32.975Z
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
Applied to files:
packages/bruno-app/src/components/EditableTable/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/StyledWrapper.js
📚 Learning: 2025-12-05T20:31:32.975Z
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
Applied to files:
packages/bruno-app/src/components/EditableTable/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/StyledWrapper.js
📚 Learning: 2025-12-05T20:31:32.975Z
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
Applied to files:
packages/bruno-app/src/components/EditableTable/StyledWrapper.js
🧬 Code graph analysis (6)
packages/bruno-app/src/components/EditableTable/index.js (1)
packages/bruno-app/src/components/RequestPane/Vars/VarsTable/index.js (3)
defaultRow(79-83)columns(47-77)getRowError(38-45)
packages/bruno-app/src/components/RequestPane/RequestHeaders/index.js (5)
packages/bruno-app/src/components/RequestPane/QueryParams/index.js (2)
dispatch(18-18)onSave(26-26)packages/bruno-app/src/components/RequestPane/Vars/VarsTable/index.js (4)
dispatch(14-14)columns(47-77)onSave(17-17)defaultRow(79-83)packages/bruno-app/src/components/EditableTable/index.js (2)
isLastEmptyRow(76-79)EditableTable(7-304)packages/bruno-app/src/utils/codemirror/autocompleteConstants.js (2)
MimeTypes(1-56)MimeTypes(1-56)packages/bruno-app/src/components/BulkEditor/index.js (1)
BulkEditor(7-38)
packages/bruno-app/src/components/CollectionSettings/Headers/index.js (3)
packages/bruno-app/src/components/FolderSettings/Headers/index.js (8)
headerAutoCompleteList(14-14)Headers(16-121)dispatch(17-17)headers(19-21)handleHeadersChange(28-34)handleSave(36-36)columns(38-74)defaultRow(76-80)packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js (2)
saveCollectionSettings(2203-2242)saveCollectionSettings(2203-2242)packages/bruno-app/src/components/EditableTable/index.js (1)
EditableTable(7-304)
packages/bruno-app/src/components/CollectionSettings/Vars/VarsTable/index.js (5)
packages/bruno-app/src/components/FolderSettings/Vars/VarsTable/index.js (8)
VarsTable(13-85)dispatch(14-14)useTheme(15-15)onSave(17-17)handleVarsChange(19-26)getRowError(28-35)columns(37-66)defaultRow(68-72)packages/bruno-app/src/components/CollectionSettings/Headers/index.js (4)
dispatch(17-17)useTheme(18-18)columns(34-69)defaultRow(71-75)packages/bruno-js/src/bru.js (1)
variableNameRegex(7-7)packages/bruno-app/src/components/EditableTable/index.js (1)
EditableTable(7-304)packages/bruno-app/src/components/EditableTable/StyledWrapper.js (1)
StyledWrapper(3-137)
packages/bruno-app/src/components/RequestPane/FormUrlEncodedParams/index.js (2)
packages/bruno-app/src/components/RequestPane/Vars/VarsTable/index.js (5)
dispatch(14-14)onSave(17-17)handleRun(18-18)columns(47-77)defaultRow(79-83)packages/bruno-app/src/components/EditableTable/index.js (2)
isLastEmptyRow(76-79)EditableTable(7-304)
packages/bruno-app/src/components/RequestPane/Vars/VarsTable/index.js (2)
packages/bruno-js/src/bru.js (1)
variableNameRegex(7-7)packages/bruno-app/src/components/EditableTable/index.js (2)
isLastEmptyRow(76-79)EditableTable(7-304)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: Unit Tests
- GitHub Check: CLI Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - macOS
🔇 Additional comments (22)
packages/bruno-app/src/components/CollectionSettings/Vars/VarsTable/index.js (2)
62-66: Clean conditional property spread.Good use of the spread operator for conditionally adding
localproperty based onvarType.
68-78: LGTM - Clean EditableTable integration.Proper wiring of
columns,rows,onChange,defaultRow, andgetRowErrorto the EditableTable component. Aligns well with the pattern used in related files.packages/bruno-app/src/components/RequestPane/RequestHeaders/index.js (3)
25-39: Well-structured memoized handlers.Both
handleHeadersChangeandhandleHeaderDragare properly memoized with correct dependency arrays. Clean separation of concerns between bulk updates and reordering.
106-121: LGTM - Clean EditableTable with reorder support.Proper integration of EditableTable with
reorderable={true}andonReorderhandler. The defensiveheaders || []fallback is a good safeguard.
86-90: Verifydescriptionfield usage in headers data model.The
defaultRowincludes adescriptionfield, but confirm whether thecolumnsarray defines a column for it. If not displayed in the UI, verify this field is intentionally preserved for API data consistency or backend persistence, and check if it's included in header payloads sent to the server.packages/bruno-app/src/components/CollectionSettings/Headers/index.js (2)
19-21: LGTM - Clean conditional header retrieval.Proper use of optional chaining with
collection.draft?.rootto determine the correct headers source.
28-30: Well-memoized change handler.Proper use of
useCallbackwith correct dependencies forhandleHeadersChange.packages/bruno-app/src/components/RequestPane/Assertions/index.js (2)
110-134: LGTM on value column rendering logic.The conditional rendering for unary operators (disabled input) vs binary operators (SingleLineEditor) is clean and handles the operator parsing correctly.
137-141:operatorkey indefaultRowmay be unused.The
defaultRowincludes anoperatorkey, butcolumnsonly definesnameandvalueas column keys. IfEditableTableonly processes columns by theirkey, this property won't be set on new rows. Verify whetheroperatoris needed here or if it's vestigial from a prior implementation.packages/bruno-app/src/components/FolderSettings/Vars/VarsTable/index.js (2)
28-35: Validation only checksnamefield, as expected.The
getRowErrorproperly validates only the name field and allows empty names (for new rows). Clean implementation.
37-66: LGTM on columns configuration.The conditional column header with InfoTip for non-request var types is a nice UX touch. The MultiLineEditor integration looks correct.
packages/bruno-app/src/components/FolderSettings/Headers/index.js (3)
76-80:descriptionfield indefaultRowhas no corresponding column.The
defaultRowincludes adescriptionkey, but there's no column defined for it. Verify ifEditableTablepreserves extra properties fromdefaultRowwhen creating new rows, or if this is dead configuration.
45-55: Good: Header name sanitization.Stripping
\r\nfrom header names (line 50) prevents invalid multi-line header names. Solid defensive coding.
103-108: LGTM on EditableTable integration.The component correctly passes headers as rows and uses the centralized
handleHeadersChangefor updates.packages/bruno-app/src/components/RequestPane/QueryParams/index.js (2)
101-112: Path param value changes bypass the genericonChangeprop.The
handlePathParamChangeis called directly in the render function (line 106). This works but creates a subtle inconsistency with how query params are handled. The current approach is acceptable since path params are read-only except for values.
29-36: LGTM on query params handler.Correctly adds
type: 'query'to all params before dispatching. This ensures the type is preserved through bulk updates.packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (2)
86-99: LGTM ongetFileNamehelper.Good handling of null/empty arrays, platform-specific path separators, and multiple file count display.
114-163: Well-structured conditional rendering for file vs text values.The value cell properly handles file display with clear button, text editing with MultiLineEditor, and conditionally shows the upload button. Clean separation of concerns.
packages/bruno-app/src/components/RequestPane/FormUrlEncodedParams/index.js (1)
1-84: LGTM overall - clean migration to EditableTable.The migration from legacy table components to the unified
EditableTableis well-structured. TheuseCallbackhooks properly stabilize dependencies, and the Redux integration viasetFormUrlEncodedParamsandmoveFormUrlEncodedParamis correct.packages/bruno-app/src/components/RequestPane/Vars/VarsTable/index.js (1)
38-45: Validation callback is well implemented.The
getRowErrorcallback correctly validates variable names usingvariableNameRegex, returnsnullfor empty names (allowing placeholder rows), and provides a clear error message. Good use ofuseCallbackwith empty deps since no external state is referenced.packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (1)
1297-1315: New bulk set actions are well-structured.The
setFormUrlEncodedParams,setRequestAssertions,setRequestVars,setFolderVars, andsetCollectionVarsreducers follow a consistent pattern: validate inputs, create draft if needed, and map with UID preservation. Good defensive defaults throughout.Also applies to: 1749-1767, 1883-1907, 2214-2236, 2448-2471
packages/bruno-app/src/components/EditableTable/index.js (1)
1-304: Well-designed reusable component.The
EditableTablecomponent provides good encapsulation of common table behaviors: row management, checkbox toggling, drag-and-drop reordering, and cell rendering. The use ofuseMemoanduseCallbackoptimizes re-renders appropriately. The empty row management logic handles edge cases like stale UIDs.
packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js
Outdated
Show resolved
Hide resolved
packages/bruno-app/src/components/RequestPane/MultipartFormParams/StyledWrapper.js
Show resolved
Hide resolved
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (1)
1401-1420:setMultipartFormParamsmissingdescriptionfield in mapping.This was flagged in a previous review and appears unaddressed. Other form param actions (e.g.,
setFormUrlEncodedParamsat line 1311) includedescription. 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 }));
🧹 Nitpick comments (3)
packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (2)
69-77: Potential stale closure onparamsinhandleClearFile.Same pattern as the fixed
handleBrowseFiles- if the user makes changes before this callback runs,paramscould be outdated. Consider reading fresh state fromitem: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 unusedonChangeparameter.The
paramsclosure risk applies here too. Additionally,onChangeis 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() !== ''); }); };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bruno-app/src/components/EditableTable/index.js(1 hunks)packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js(1 hunks)packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js(13 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/components/EditableTable/index.js
🧬 Code graph analysis (2)
packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (2)
packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js (2)
browseFiles(2195-2201)browseFiles(2195-2201)packages/bruno-app/src/utils/common/platform.js (2)
isWindowsOS(22-27)isWindowsOS(22-27)
packages/bruno-app/src/components/EditableTable/index.js (1)
packages/bruno-app/src/components/RequestPane/Vars/VarsTable/index.js (2)
columns(47-77)getRowError(38-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
🔇 Additional comments (14)
packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (2)
89-102: LGTM!
getFileNamehandles both single and multiple files correctly, with proper Windows/Unix path separator detection. Edge cases (null, empty array, empty strings) are well covered.
104-185: Clean column configuration and EditableTable integration.The columns setup with custom render functions for file handling is well-structured. The conditional rendering for file display vs. text editor is clear.
Also applies to: 187-206
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (6)
1300-1318: LGTM!
setFormUrlEncodedParamscorrectly maps all fields includingdescriptionand preserves UIDs when provided.
1752-1770: LGTM!
setRequestAssertionscorrectly includes theoperatorfield and follows the established pattern.
1886-1910: LGTM!
setRequestVarscorrectly handles the conditionallocalfield for response-type variables.
2217-2239: LGTM!
setFolderVarsproperly initializesfolder.draftfromfolder.rootand handles both request/response types.
2451-2474: LGTM!
setCollectionVarsfollows the same consistent pattern as other bulk-set actions.
1231-1240: LGTM!
setFolderHeadersnow correctly initializesfolder.draftbefore modifying headers, addressing the previous review concern about bypassing the draft workflow.packages/bruno-app/src/components/EditableTable/index.js (6)
7-20: Well-designed prop interface.Good defaults and clear prop names. The flexibility to customize checkbox behavior, deletion, and reordering makes this component reusable across the codebase.
36-66: Solid trailing empty row management.The logic correctly handles edge cases: empty initial state, preventing duplicate empty rows, and maintaining UID stability. The check at line 57 prevents UID collisions if a row somehow matches the empty row UID.
144-156: Drag-drop reorder excludes trailing empty row correctly.Good handling at line 148 to slice off the empty row before reordering. The
updateReorderedItemcallback shape aligns with the Redux actions expecting UID arrays.
193-208: Error tooltip implementation looks correct.Using
data-tooltip-idon the anchor andidon theTooltipfollows react-tooltip v5+ API. The error is only shown for non-empty rows.
251-280: Drag handle visibility tied to hover state.The conditional rendering based on
hoveredRow === rowIndexmeans the grip icon only appears on hover. This is good UX but note: if users need visual affordance that rows are reorderable, consider showing a subtle indicator always.
236-298: Solid row rendering with conditional drag behavior.The
canDragcheck properly prevents dragging empty rows and respects thereorderableprop. Event handlers are correctly conditionally applied.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
packages/bruno-app/src/components/GlobalEnvironments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js (1)
60-64: Checkbox alignment tweak is a solid simplificationCentering via
vertical-align: middleand resettingmarginis 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-heighton 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.themefor all colors/typography and mirrors the workspace rename/error patterns, so it should integrate cleanly with the existing UI. There is some duplication withWorkspaceHome/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
tdpadding/vertical-align, header color/font-size, and input/checkbox alignment makes this table visually consistent with the newEditableTablestyles and keeps colors theme-driven. If more tables converge on this look, consider sharing more of this styling with the genericEditableTableStyledWrapperto 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 insideparseAssertionOperator, and the operator options inAssertionOperatorall 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 extraoperatorfield.Passing
columns,rows={assertions || []},defaultRow(withvalue: 'eq '), andreorderable/onReorderintoEditableTablematches the intended API and should work well with drag-reorder and empty-row handling. The persisted operator is now encoded insidevalue("operator value"), so the separateoperator: 'eq'field indefaultRowappears 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 anisKeyFieldcolumn.
rowsWithEmpty,isEmptyRow,isLastEmptyRow, and the finalresultfilter all assume there is a column withisKeyField: true. If no such column exists,keyColumnisundefined, and the filter will effectively drop all rows whenshowAddRowis true. At minimum, it’s worth documenting thatEditableTablerequires exactly one key column; alternatively, you could add a runtime guard (e.g.,console.warnor throwing in dev) when!columns.some(c => c.isKeyField).
81-116: Last empty row drops non-key edits; consider whether that UX is acceptable.
handleValueChangeonly 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 toupdatedRowsbut then filtered out (the last row is removed fromresultand recreated fromdefaultRowon 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 - 1filter 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 unuseddragStartstate to reduce noise.
dragStartis set inhandleDragStartand cleared inhandleDragEnd, 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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/bruno-app/src/components/EditableTable/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/EditableTable/index.js(1 hunks)packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js(2 hunks)packages/bruno-app/src/components/GlobalEnvironments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js(2 hunks)packages/bruno-app/src/components/RequestPane/Assertions/index.js(1 hunks)packages/bruno-app/src/components/WorkspaceHome/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js(4 hunks)packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.js(1 hunks)packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/index.js(1 hunks)packages/bruno-app/src/components/WorkspaceHome/index.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/components/EditableTable/StyledWrapper.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/index.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/index.jspackages/bruno-app/src/components/EditableTable/index.jspackages/bruno-app/src/components/RequestPane/Assertions/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/GlobalEnvironments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js
🧠 Learnings (3)
📚 Learning: 2025-12-05T20:31:33.005Z
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
Applied to files:
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/GlobalEnvironments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js
📚 Learning: 2025-12-05T20:31:33.005Z
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
Applied to files:
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js
📚 Learning: 2025-12-05T20:31:33.005Z
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
Applied to files:
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.jspackages/bruno-app/src/components/GlobalEnvironments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js
🧬 Code graph analysis (3)
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/StyledWrapper.js (1)
packages/bruno-app/src/components/WorkspaceHome/StyledWrapper.js (1)
StyledWrapper(3-133)
packages/bruno-app/src/components/EditableTable/index.js (3)
packages/bruno-app/src/components/FolderSettings/Headers/index.js (2)
defaultRow(76-80)columns(38-74)packages/bruno-app/src/components/RequestPane/Vars/VarsTable/index.js (3)
defaultRow(79-83)columns(47-77)getRowError(38-45)packages/bruno-app/src/components/EditableTable/StyledWrapper.js (1)
StyledWrapper(3-144)
packages/bruno-app/src/components/RequestPane/Assertions/index.js (3)
packages/bruno-app/src/components/RequestPane/Assertions/AssertionRow/index.js (8)
unaryOperators(78-91)unaryOperators(117-130)parseAssertionOperator(39-114)parseAssertionOperator(146-146)operators(47-76)operator(93-93)value(94-94)isUnaryOperator(116-133)packages/bruno-app/src/components/RequestPane/Assertions/AssertionOperator/index.js (1)
AssertionOperator(36-92)packages/bruno-app/src/components/EditableTable/index.js (2)
isLastEmptyRow(76-79)EditableTable(7-304)
🔇 Additional comments (7)
packages/bruno-app/src/components/WorkspaceHome/index.js (2)
222-223: LGTM! Header sizing refinement.The reduction in text size (
text-xl→text-sm) and icon size (24 → 18) creates a more compact workspace header, consistent with the broader UI modernization described in the PR summary.
305-305: LGTM! Tab padding refinement.The vertical padding adjustment (
py-2→pt-2 pb-1.5) subtly reduces tab height while maintaining consistent spacing with the updated header styling.packages/bruno-app/src/components/GlobalEnvironments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js (1)
10-15: Table cell vertical alignment change looks goodUsing
vertical-align: middleontdwill help keep text and controls visually centered in rows and is consistent with the broader table cleanup. No issues.packages/bruno-app/src/components/WorkspaceHome/StyledWrapper.js (1)
111-111: LGTM! Clean styling improvement.Adding medium font-weight to active tabs improves visual hierarchy and user feedback.
packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/StyledWrapper.js (2)
10-27: Vertical centering for table cells is a good improvementAdding
vertical-align: middleto the table cell rule will help keep header and body content visually aligned when rows get taller, without affecting logic or theming. Looks good as part of the broader table standardization.
60-64: Checkbox alignment tweak is appropriate and non-breakingCentering the checkbox vertically and resetting its margin to
0should remove the previous “off by a pixel” feel and align it better with adjacent text/controls inside the table cells. This is a safe, localized styling improvement.packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/index.js (1)
10-10: StyledWrapper import looks correct and consistent.Import path and usage of the extracted
StyledWrappermatch the local file structure; no behavior concerns here.
| key: 'operator', | ||
| name: 'Operator', | ||
| width: '120px', | ||
| render: ({ row, rowIndex, isLastEmptyRow }) => { | ||
| const { operator } = parseAssertionOperator(row.value); | ||
| const assertionValue = parseAssertionOperator(row.value).value; | ||
|
|
||
| const handleOperatorChange = (newOperator) => { | ||
| const updatedAssertions = assertions.map((assertion, idx) => { | ||
| if (idx === rowIndex) { | ||
| return { | ||
| ...assertion, | ||
| value: isUnaryOperator(newOperator) ? newOperator : `${newOperator} ${assertionValue}` | ||
| }; | ||
| } | ||
| return assertion; | ||
| }); | ||
| handleAssertionsChange(updatedAssertions); | ||
| }; | ||
|
|
||
| return ( | ||
| <AssertionOperator | ||
| operator={operator} | ||
| onChange={handleOperatorChange} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Use EditableTable’s onChange for operator updates and avoid indexing into assertions.
Right now, the Operator column:
- Parses
row.valuetwice - Rebuilds
updatedAssertionsfrom the externalassertionsarray usingrowIndex - Dispatches
setRequestAssertionsdirectly
This has a couple of downsides:
- Changing the operator on the trailing “empty” row is effectively a no-op (there’s no corresponding entry in
assertions), so the select will snap back to the default and the choice is lost. - It bypasses
EditableTable’sonChange/handleValueChangemachinery, which is what manages the empty row lifecycle and any future validation.
You can simplify and make this more robust by using the value and onChange provided by the column render, and parsing only once, e.g.:
- render: ({ row, rowIndex, isLastEmptyRow }) => {
- const { operator } = parseAssertionOperator(row.value);
- const assertionValue = parseAssertionOperator(row.value).value;
-
- const handleOperatorChange = (newOperator) => {
- const updatedAssertions = assertions.map((assertion, idx) => {
- if (idx === rowIndex) {
- return {
- ...assertion,
- value: isUnaryOperator(newOperator) ? newOperator : `${newOperator} ${assertionValue}`
- };
- }
- return assertion;
- });
- handleAssertionsChange(updatedAssertions);
- };
+ render: ({ value, onChange }) => {
+ const { operator, value: assertionValue } = parseAssertionOperator(value);
+
+ const handleOperatorChange = (newOperator) => {
+ const nextValue = isUnaryOperator(newOperator)
+ ? newOperator
+ : `${newOperator} ${assertionValue}`;
+
+ onChange(nextValue);
+ };- return (
- <AssertionOperator
- operator={operator}
- onChange={handleOperatorChange}
- />
- );
+ return (
+ <AssertionOperator
+ operator={operator}
+ onChange={handleOperatorChange}
+ />
+ );This keeps all row mutation logic inside EditableTable, avoids relying on rowIndex/assertions, and fixes the “last empty row operator change is ignored” behavior.
Also applies to: 116-137
1 participant
Description
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.