Open
Conversation
shubh-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-tab UI persistence to tabs state (column widths, timeline expansion, response filter, docs editing, GraphQL docs visibility) and exposes controlled column-width props on EditableTable; wires many table consumers and timeline/filter components to read/update that Redux state. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditableTable
participant Consumer
participant Redux
User->>EditableTable: Drag column edge to resize
EditableTable->>Consumer: call onColumnWidthsChange(tableId, widths)
Consumer->>Redux: dispatch(updateTableColumnWidths({ uid, tableId, widths }))
Redux->>Redux: persist focusedTab.tableColumnWidths[tableId]
Consumer->>Consumer: useSelector reads focusedTab.tableColumnWidths
Consumer->>EditableTable: pass columnWidths (controlled)
EditableTable->>EditableTable: re-render with applied widths
User->>Consumer: switch active tab
Consumer->>Redux: activeTabUid changes
Consumer->>EditableTable: pass new tab's columnWidths
EditableTable->>EditableTable: render with new tab-specific widths
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/tabs.js (1)
60-77:⚠️ Potential issue | 🟠 MajorPreview tab replacement missing new UI state fields.
When a preview tab is replaced (lines 60-77), the new tab object doesn't include
responseFilter,responseFilterExpanded,timelineExpandedItems,gqlDocsOpen,tableColumnWidths, ordocsEditing. These are only initialized when pushing a new tab (lines 92-98).Accessing these fields on replaced tabs will return
undefinedinstead of the expected defaults.🐛 Proposed fix
state.tabs[state.tabs.length - 1] = { uid, collectionUid, requestPaneWidth: lastTab.requestPaneWidth ?? null, requestPaneHeight: lastTab.requestPaneHeight ?? null, requestPaneTab: requestPaneTab || defaultRequestPaneTab, responsePaneTab: 'response', responseFormat: null, responseViewTab: null, + responseFilter: null, + responseFilterExpanded: false, + timelineExpandedItems: {}, + gqlDocsOpen: false, + tableColumnWidths: {}, scriptPaneTab: null, + docsEditing: false, type: type || 'request',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/tabs.js` around lines 60 - 77, The replacement logic for preview tabs in the reducer (where state.tabs[state.tabs.length - 1] is set using uid, collectionUid, lastTab, nonReplaceableTabTypes, etc.) omits several UI state fields (responseFilter, responseFilterExpanded, timelineExpandedItems, gqlDocsOpen, tableColumnWidths, docsEditing) that are initialized when pushing a new tab; update that replacement object to include those fields with the same default values used in the push/new-tab code path so replaced tabs have the expected defaults and avoid undefined access.
🧹 Nitpick comments (6)
packages/bruno-app/src/components/RequestPane/RequestHeaders/index.js (1)
21-32: Consider extracting the tab-width plumbing into a shared hook.This selector/dispatch block is repeated in a lot of table components in this PR. A small helper like
useTabTableColumnWidths(tableId)would trim the copy/paste and make callback wiring less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/RequestHeaders/index.js` around lines 21 - 32, Extract the repeated Redux plumbing for table column widths into a reusable hook: create useTabTableColumnWidths(tableId) that internally uses useSelector to read tabs and activeTabUid, computes focusedTab and returns { widths: focusedTab?.tableColumnWidths?.[tableId] || {}, setWidths: (widths) => dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId, widths })) }; then replace the local logic in RequestHeaders (the tabs/activeTabUid/focusedTab/headersWidths and handleColumnWidthsChange code) with a call to useTabTableColumnWidths('request-headers') and use the returned widths and setWidths, keeping existing symbols like headersWidths and handleColumnWidthsChange behavior intact.packages/bruno-app/src/components/RequestPane/Assertions/index.js (1)
58-68: Consider a smalluseTabTableColumnWidths()hook.This selector/lookup/dispatch block is now duplicated across most of the table components in the PR, and the inline
// Get column widths from Reduxcomment is just restating the next line. Centralizing this would trim the boilerplate and make theEditableTablecallback wiring harder to misapply.As per coding guidelines "Add meaningful comments instead of obvious ones where complex code flow is explained properly".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/Assertions/index.js` around lines 58 - 68, The repeated selector/lookup/dispatch pattern (useSelector for state.tabs.tabs and state.tabs.activeTabUid, finding focusedTab by uid, reading focusedTab.tableColumnWidths['assertions'] into assertionsWidths, and the handleColumnWidthsChange that dispatches updateTableColumnWidths) should be extracted into a reusable hook (e.g., useTabTableColumnWidths) so components like Assertions can call const { widths, onChange } = useTabTableColumnWidths('assertions'); implement the hook to encapsulate reading tabs and activeTabUid, resolving focusedTab, returning the specific table widths and a memoized onChange that calls dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId, widths })); replace the local variables assertionsWidths and handleColumnWidthsChange in Assertions with the hook to remove duplicated boilerplate and improve consistency.packages/bruno-app/src/components/ResponsePane/QueryResult/index.js (1)
1-1: Unused import:debouncefrom lodash.The
debouncefunction is imported but no longer used after the filter state management was moved to Redux. Remove it to avoid dead code.🧹 Proposed fix
-import { debounce } from 'lodash';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/ResponsePane/QueryResult/index.js` at line 1, Remove the unused lodash import by deleting the "debounce" import at the top of the QueryResult component file; locate the import statement (import { debounce } from 'lodash';) in packages/bruno-app/src/components/ResponsePane/QueryResult/index.js and remove it since filter state is now managed in Redux and debounce is no longer referenced.packages/bruno-app/src/components/ResponsePane/index.js (1)
4-4: Unused import:updateTimelineExpandedItems.This action is imported but never dispatched in this file. Timeline handles its own dispatch internally.
🧹 Proposed fix
-import { updateResponsePaneTab, updateResponseFormat, updateResponseViewTab, updateResponseFilter, updateResponseFilterExpanded, updateTimelineExpandedItems } from 'providers/ReduxStore/slices/tabs'; +import { updateResponsePaneTab, updateResponseFormat, updateResponseViewTab, updateResponseFilter, updateResponseFilterExpanded } from 'providers/ReduxStore/slices/tabs';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/ResponsePane/index.js` at line 4, Remove the unused import updateTimelineExpandedItems from the import list in ResponsePane (where updateResponsePaneTab, updateResponseFormat, updateResponseViewTab, updateResponseFilter, updateResponseFilterExpanded are imported) — since timeline manages its own dispatch, simply delete updateTimelineExpandedItems from the named imports to eliminate the unused-import warning and keep the other action imports intact.packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.js (1)
4-4: Unused import:updateTimelineExpandedItems.This action is imported but never dispatched in this file. The Timeline component handles its own dispatching internally.
🧹 Proposed fix
-import { updateResponsePaneTab, updateTimelineExpandedItems } from 'providers/ReduxStore/slices/tabs'; +import { updateResponsePaneTab } from 'providers/ReduxStore/slices/tabs';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.js` at line 4, Remove the unused import updateTimelineExpandedItems from the import statement in the GrpcResponsePane module: locate the import line that currently imports updateResponsePaneTab and updateTimelineExpandedItems from 'providers/ReduxStore/slices/tabs' and delete updateTimelineExpandedItems so only updateResponsePaneTab remains; ensure no other references to updateTimelineExpandedItems exist in GrpcResponsePane (e.g., in any functions or JSX) before committing.packages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultFilter/index.js (1)
15-17: Redundant: manually clearinginputRef.current.value.Since the input is now controlled (
value={filter || ''}), clearinginputRef.current.valueis unnecessary. TheonChange('')call at line 14 will update the Redux state, which flows back through props to clear the input.🧹 Proposed fix
// Reset filter search input when closing if (!newExpanded) { onChange(''); - if (inputRef?.current) { - inputRef.current.value = ''; - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultFilter/index.js` around lines 15 - 17, Remove the redundant manual DOM mutation that sets inputRef.current.value = '' inside the clear handler: the input is controlled via value={filter || ''}, and onChange('') already updates Redux which will clear the input; delete the inputRef.current value assignment in the clear function (referencing inputRef and the existing onChange('') call) so the component relies on the controlled value flow instead of directly mutating inputRef.current.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/EditableTable/index.js`:
- Around line 83-85: The resize handler only calls handleColumnWidthsChange when
isControlled is true, so in uncontrolled mode the localColumnWidths state isn't
updated; modify the branch around isControlled (the block invoking
handleColumnWidthsChange) to also set the internal state when not controlled by
calling setLocalColumnWidths(newWidths) (i.e., if (isControlled) call
handleColumnWidthsChange(newWidths) else call setLocalColumnWidths(newWidths));
update the equivalent location referenced at line ~111 as well so both resize
code paths update localColumnWidths when !isControlled.
In `@packages/bruno-app/src/components/RequestPane/GraphQLSchemaActions/index.js`:
- Line 46: The span in the GraphQLSchemaActions component contains a leftover
debug string "Docs123"; update the span text in the GraphQLSchemaActions
component (the span with className "ml-1") to the intended production label
"Docs" so the UI shows the correct text.
In
`@packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleFileBody/index.js`:
- Around line 25-27: The handler handleColumnWidthsChange currently expects
(tableId, widths) but EditableTable calls onColumnWidthsChange(widths) with a
single widths arg, so widths becomes undefined; change handleColumnWidthsChange
to accept a single widths parameter and obtain the tableId from the component
scope (or the prop/state where the table's id is stored) before dispatching
updateTableColumnWidths({ uid: activeTabUid, tableId, widths }); ensure the
EditableTable onColumnWidthsChange prop is passed this updated
handleColumnWidthsChange and reference the symbols handleColumnWidthsChange,
onColumnWidthsChange, updateTableColumnWidths, and activeTabUid when locating
the code to modify.
In
`@packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleFormUrlEncodedParams/index.js`:
- Around line 21-23: The onColumnWidthsChange handler is passed directly but
handleColumnWidthsChange expects (tableId, widths), so wrap the callback to
include the table id; change the prop usage to call handleColumnWidthsChange
with the unique id (e.g., 'example-form-url-encoded') and the widths:
onColumnWidthsChange={(widths) =>
handleColumnWidthsChange('example-form-url-encoded', widths)} (also apply the
same wrapping for the other occurrence mentioned around lines 101–103) so the
dispatched updateTableColumnWidths call receives the correct tableId and widths.
In
`@packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleHeaders/index.js`:
- Around line 27-29: The onColumnWidthsChange callback is being called without
the tableId so the signature for handleColumnWidthsChange(uid, tableId, widths)
receives newWidths as tableId and widths becomes undefined; bind the literal
tableId 'example-headers' when wiring the EditableTable so the handler gets
(tableId='example-headers', widths=newWidths). Update the wiring where
onColumnWidthsChange is passed (and the similar occurrence at the second
location) to pass a bound callback or wrapper (e.g., arrow or .bind) that
supplies 'example-headers' as the first argument before the widths so
dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId, widths }))
receives the correct tableId and widths.
In
`@packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleMultipartFormParams/index.js`:
- Around line 27-29: The two-argument handler handleColumnWidthsChange is being
passed directly to EditableTable which only calls
onColumnWidthsChange(newWidths), causing the widths to be dispatched as the
tableId and losing widths; update the prop wiring where onColumnWidthsChange is
assigned (in ResponseExampleMultipartFormParams) to bind the tableId string
'example-multipart-form' (e.g., onColumnWidthsChange={widths =>
handleColumnWidthsChange('example-multipart-form', widths)}) so
updateTableColumnWidths receives { uid: activeTabUid, tableId:
'example-multipart-form', widths } correctly; apply the same fix to the other
occurrence at lines 272-274.
In
`@packages/bruno-app/src/components/ResponseExample/ResponseExampleResponsePane/ResponseExampleResponseHeaders/index.js`:
- Around line 28-30: The onColumnWidthsChange handler is being passed a two-arg
signature but EditableTable only calls onColumnWidthsChange(newWidths), so the
first param becomes widths and tableId becomes undefined; fix by binding the
table id literal 'example-response-headers' when wiring the handler so that
handleColumnWidthsChange receives only widths: call the existing
handleColumnWidthsChange via a one-arg wrapper or pre-bind the tableId (using
the symbol handleColumnWidthsChange) so dispatch(updateTableColumnWidths({ uid:
activeTabUid, tableId: 'example-response-headers', widths })) receives the
correct tableId; ensure the prop name onColumnWidthsChange is passed this bound
function (reference onColumnWidthsChange, handleColumnWidthsChange,
updateTableColumnWidths, activeTabUid).
In `@packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.js`:
- Around line 84-86: The getTabPanel function currently references focusedTab
(specifically focusedTab?.timelineExpandedItems) before focusedTab is declared,
causing a fragile TDZ dependency; change getTabPanel to accept the
timelineExpandedItems (or the whole focusedTab) as a parameter (e.g.,
getTabPanel(tabKey, timelineExpandedItems)) and replace usages of
focusedTab?.timelineExpandedItems inside getTabPanel with that parameter, then
update the call site where getTabPanel('timeline') is invoked (pass
focusedTab?.timelineExpandedItems or a safe default {}), ensuring Timeline is
rendered with timelineExpandedItems coming from the new parameter and keeping
activeTabUid, collection, and item as before.
In
`@packages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultFilter/index.js`:
- Line 7: The local state isExpanded is only initialized from the Redux prop
filterExpanded and won't follow updates; add a useEffect that watches
filterExpanded and calls setIsExpanded(filterExpanded) to keep local state in
sync, and ensure you import useEffect alongside useState at the top of the file
(so update the import that currently imports useState to include useEffect).
In
`@packages/bruno-app/src/components/ResponsePane/Timeline/GrpcTimelineItem/index.js`:
- Around line 32-35: The prop-to-local state mapping is inverted: isExpanded
from Redux should make content visible, but current line sets isCollapsed =
isExpanded when it should set isCollapsed to the inverse; update the assignment
in the GrpcTimelineItem so that isCollapsed uses !isExpanded when isExpanded is
provided (i.e., replace the ternary to use the negation), keeping
localIsCollapsed as the fallback; reference the symbols isCollapsed, isExpanded,
and localIsCollapsed and ensure existing render logic like renderEventContent()
and chevron rendering continues to rely on isCollapsed.
In `@packages/bruno-graphql-docs/src/components/DocExplorer.tsx`:
- Line 90: Revert test/debug artifacts in DocExplorer: replace the informal "No
Schema Available Sorry..." assigned to the content variable with a proper
user-facing string like "No schema available.", remove the stray "123" from the
aria-label so the element using aria-label reads "Documentation Explorer"
(restore the original accessible label on the element that sets aria-label), and
remove the "123" from the search input placeholder so placeholder uses `Search
${navItem.name}...` without the test number; update the assignments where
`content`, the element with `aria-label`, and the input `placeholder` prop are
defined in the DocExplorer component accordingly.
- Around line 25-28: The initialNav constant contains stray debug text in its
title; update the NavStackItem initialization (initialNav) to revert the title
from "Documentation Explorer 2424" back to the original user-facing string
(e.g., "Documentation Explorer") so the UI doesn't show test/debug numbers;
ensure only the title field of initialNav is changed and keep the name and type
(NavStackItem) unchanged.
---
Outside diff comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/tabs.js`:
- Around line 60-77: The replacement logic for preview tabs in the reducer
(where state.tabs[state.tabs.length - 1] is set using uid, collectionUid,
lastTab, nonReplaceableTabTypes, etc.) omits several UI state fields
(responseFilter, responseFilterExpanded, timelineExpandedItems, gqlDocsOpen,
tableColumnWidths, docsEditing) that are initialized when pushing a new tab;
update that replacement object to include those fields with the same default
values used in the push/new-tab code path so replaced tabs have the expected
defaults and avoid undefined access.
---
Nitpick comments:
In `@packages/bruno-app/src/components/RequestPane/Assertions/index.js`:
- Around line 58-68: The repeated selector/lookup/dispatch pattern (useSelector
for state.tabs.tabs and state.tabs.activeTabUid, finding focusedTab by uid,
reading focusedTab.tableColumnWidths['assertions'] into assertionsWidths, and
the handleColumnWidthsChange that dispatches updateTableColumnWidths) should be
extracted into a reusable hook (e.g., useTabTableColumnWidths) so components
like Assertions can call const { widths, onChange } =
useTabTableColumnWidths('assertions'); implement the hook to encapsulate reading
tabs and activeTabUid, resolving focusedTab, returning the specific table widths
and a memoized onChange that calls dispatch(updateTableColumnWidths({ uid:
activeTabUid, tableId, widths })); replace the local variables assertionsWidths
and handleColumnWidthsChange in Assertions with the hook to remove duplicated
boilerplate and improve consistency.
In `@packages/bruno-app/src/components/RequestPane/RequestHeaders/index.js`:
- Around line 21-32: Extract the repeated Redux plumbing for table column widths
into a reusable hook: create useTabTableColumnWidths(tableId) that internally
uses useSelector to read tabs and activeTabUid, computes focusedTab and returns
{ widths: focusedTab?.tableColumnWidths?.[tableId] || {}, setWidths: (widths) =>
dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId, widths })) };
then replace the local logic in RequestHeaders (the
tabs/activeTabUid/focusedTab/headersWidths and handleColumnWidthsChange code)
with a call to useTabTableColumnWidths('request-headers') and use the returned
widths and setWidths, keeping existing symbols like headersWidths and
handleColumnWidthsChange behavior intact.
In `@packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.js`:
- Line 4: Remove the unused import updateTimelineExpandedItems from the import
statement in the GrpcResponsePane module: locate the import line that currently
imports updateResponsePaneTab and updateTimelineExpandedItems from
'providers/ReduxStore/slices/tabs' and delete updateTimelineExpandedItems so
only updateResponsePaneTab remains; ensure no other references to
updateTimelineExpandedItems exist in GrpcResponsePane (e.g., in any functions or
JSX) before committing.
In `@packages/bruno-app/src/components/ResponsePane/index.js`:
- Line 4: Remove the unused import updateTimelineExpandedItems from the import
list in ResponsePane (where updateResponsePaneTab, updateResponseFormat,
updateResponseViewTab, updateResponseFilter, updateResponseFilterExpanded are
imported) — since timeline manages its own dispatch, simply delete
updateTimelineExpandedItems from the named imports to eliminate the
unused-import warning and keep the other action imports intact.
In `@packages/bruno-app/src/components/ResponsePane/QueryResult/index.js`:
- Line 1: Remove the unused lodash import by deleting the "debounce" import at
the top of the QueryResult component file; locate the import statement (import {
debounce } from 'lodash';) in
packages/bruno-app/src/components/ResponsePane/QueryResult/index.js and remove
it since filter state is now managed in Redux and debounce is no longer
referenced.
In
`@packages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultFilter/index.js`:
- Around line 15-17: Remove the redundant manual DOM mutation that sets
inputRef.current.value = '' inside the clear handler: the input is controlled
via value={filter || ''}, and onChange('') already updates Redux which will
clear the input; delete the inputRef.current value assignment in the clear
function (referencing inputRef and the existing onChange('') call) so the
component relies on the controlled value flow instead of directly mutating
inputRef.current.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cec5264a-a25c-423e-a80f-210c479af1a1
📒 Files selected for processing (30)
packages/bruno-app/src/components/CollectionSettings/Headers/index.jspackages/bruno-app/src/components/CollectionSettings/Vars/VarsTable/index.jspackages/bruno-app/src/components/Documentation/index.jspackages/bruno-app/src/components/EditableTable/index.jspackages/bruno-app/src/components/FolderSettings/Headers/index.jspackages/bruno-app/src/components/FolderSettings/Vars/VarsTable/index.jspackages/bruno-app/src/components/RequestPane/Assertions/index.jspackages/bruno-app/src/components/RequestPane/FormUrlEncodedParams/index.jspackages/bruno-app/src/components/RequestPane/GraphQLSchemaActions/index.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/index.jspackages/bruno-app/src/components/RequestPane/QueryParams/index.jspackages/bruno-app/src/components/RequestPane/RequestHeaders/index.jspackages/bruno-app/src/components/RequestPane/Vars/VarsTable/index.jspackages/bruno-app/src/components/RequestTabPanel/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleFileBody/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleFormUrlEncodedParams/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleHeaders/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleMultipartFormParams/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleParams/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleResponsePane/ResponseExampleResponseHeaders/index.jspackages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.jspackages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultFilter/index.jspackages/bruno-app/src/components/ResponsePane/QueryResult/index.jspackages/bruno-app/src/components/ResponsePane/Timeline/GrpcTimelineItem/index.jspackages/bruno-app/src/components/ResponsePane/Timeline/TimelineItem/index.jspackages/bruno-app/src/components/ResponsePane/Timeline/index.jspackages/bruno-app/src/components/ResponsePane/WsResponsePane/index.jspackages/bruno-app/src/components/ResponsePane/index.jspackages/bruno-app/src/providers/ReduxStore/slices/tabs.jspackages/bruno-graphql-docs/src/components/DocExplorer.tsx
Comment on lines
+83
to
+85
| if (isControlled) { | ||
| handleColumnWidthsChange(newWidths); | ||
| } |
Contributor
There was a problem hiding this comment.
Uncontrolled mode resize is broken.
handleColumnWidthsChange is only called when isControlled is true. In uncontrolled mode, the resize visually happens but the localColumnWidths state is never updated, so widths reset on re-render.
🐛 Proposed fix
- if (isControlled) {
- handleColumnWidthsChange(newWidths);
- }
+ handleColumnWidthsChange(newWidths);Apply the same fix at line 111:
- if (isControlled) {
- handleColumnWidthsChange({ ...widths, ...newWidths });
- }
+ handleColumnWidthsChange({ ...widths, ...newWidths });📝 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
| if (isControlled) { | |
| handleColumnWidthsChange(newWidths); | |
| } | |
| handleColumnWidthsChange(newWidths); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/EditableTable/index.js` around lines 83 -
85, The resize handler only calls handleColumnWidthsChange when isControlled is
true, so in uncontrolled mode the localColumnWidths state isn't updated; modify
the branch around isControlled (the block invoking handleColumnWidthsChange) to
also set the internal state when not controlled by calling
setLocalColumnWidths(newWidths) (i.e., if (isControlled) call
handleColumnWidthsChange(newWidths) else call setLocalColumnWidths(newWidths));
update the equivalent location referenced at line ~111 as well so both resize
code paths update localColumnWidths when !isControlled.
packages/bruno-app/src/components/RequestPane/GraphQLSchemaActions/index.js
Outdated
Show resolved
Hide resolved
...p/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleFileBody/index.js
Show resolved
Hide resolved
Comment on lines
+21
to
+23
| const handleColumnWidthsChange = (tableId, widths) => { | ||
| dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId, widths })); | ||
| }; |
Contributor
There was a problem hiding this comment.
Wrap onColumnWidthsChange with the table id.
handleColumnWidthsChange expects (tableId, widths), but this table passes it directly. That means the widths payload will be dispatched as tableId and widths becomes undefined, so persistence for example-form-url-encoded won’t work correctly.
Suggested fix
- <EditableTable
- tableId="example-form-url-encoded"
- columnWidths={formUrlEncodedWidths}
- onColumnWidthsChange={handleColumnWidthsChange}
+ <EditableTable
+ tableId="example-form-url-encoded"
+ columnWidths={formUrlEncodedWidths}
+ onColumnWidthsChange={(widths) => handleColumnWidthsChange('example-form-url-encoded', widths)}Also applies to: 101-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleFormUrlEncodedParams/index.js`
around lines 21 - 23, The onColumnWidthsChange handler is passed directly but
handleColumnWidthsChange expects (tableId, widths), so wrap the callback to
include the table id; change the prop usage to call handleColumnWidthsChange
with the unique id (e.g., 'example-form-url-encoded') and the widths:
onColumnWidthsChange={(widths) =>
handleColumnWidthsChange('example-form-url-encoded', widths)} (also apply the
same wrapping for the other occurrence mentioned around lines 101–103) so the
dispatched updateTableColumnWidths call receives the correct tableId and widths.
...pp/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleHeaders/index.js
Show resolved
Hide resolved
Comment on lines
84
to
86
| case 'timeline': { | ||
| return <Timeline collection={collection} item={item} />; | ||
| return <Timeline collection={collection} item={item} timelineExpandedItems={focusedTab?.timelineExpandedItems || {}} activeTabUid={activeTabUid} />; | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: focusedTab used before declaration (TDZ).
focusedTab is referenced at line 85 inside getTabPanel, but it's declared later at line 113. When getTabPanel('timeline') is called during render (line 158), focusedTab will be in scope, but static analysis and early returns (lines 93-116) could cause confusion or issues if order changes. The code currently works because getTabPanel is only invoked after focusedTab is assigned, but this is fragile.
Consider moving focusedTab declaration above getTabPanel or pass timelineExpandedItems as a parameter to getTabPanel.
🔧 Suggested refactor: pass as parameter
- const getTabPanel = (tab) => {
+ const getTabPanel = (tab, expandedItems) => {
switch (tab) {
...
case 'timeline': {
- return <Timeline collection={collection} item={item} timelineExpandedItems={focusedTab?.timelineExpandedItems || {}} activeTabUid={activeTabUid} />;
+ return <Timeline collection={collection} item={item} timelineExpandedItems={expandedItems} activeTabUid={activeTabUid} />;
}
...
}
};Then at call site (line 158):
- <>{getTabPanel(focusedTab.responsePaneTab)}</>
+ <>{getTabPanel(focusedTab.responsePaneTab, focusedTab?.timelineExpandedItems || {})}</>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.js`
around lines 84 - 86, The getTabPanel function currently references focusedTab
(specifically focusedTab?.timelineExpandedItems) before focusedTab is declared,
causing a fragile TDZ dependency; change getTabPanel to accept the
timelineExpandedItems (or the whole focusedTab) as a parameter (e.g.,
getTabPanel(tabKey, timelineExpandedItems)) and replace usages of
focusedTab?.timelineExpandedItems inside getTabPanel with that parameter, then
update the call site where getTabPanel('timeline') is invoked (pass
focusedTab?.timelineExpandedItems or a safe default {}), ensuring Timeline is
rendered with timelineExpandedItems coming from the new parameter and keeping
activeTabUid, collection, and item as before.
| const QueryResultFilter = ({ filter, filterExpanded, onChange, onExpandChange, mode }) => { | ||
| const inputRef = useRef(null); | ||
| const [isExpanded, toggleExpand] = useState(false); | ||
| const [isExpanded, setIsExpanded] = useState(filterExpanded || false); |
Contributor
There was a problem hiding this comment.
Local state won't sync with Redux prop changes.
isExpanded is initialized from filterExpanded only on mount. When the user switches tabs, filterExpanded will change via Redux, but isExpanded won't update because useState only uses the initial value once.
Add a useEffect to sync when filterExpanded changes:
🐛 Proposed fix
const QueryResultFilter = ({ filter, filterExpanded, onChange, onExpandChange, mode }) => {
const inputRef = useRef(null);
const [isExpanded, setIsExpanded] = useState(filterExpanded || false);
+
+ useEffect(() => {
+ setIsExpanded(filterExpanded || false);
+ }, [filterExpanded]);Also update the import:
-import React, { useMemo, useRef, useState } from 'react';
+import React, { useEffect, useMemo, useRef, 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
| const [isExpanded, setIsExpanded] = useState(filterExpanded || false); | |
| import React, { useEffect, useMemo, useRef, useState } from 'react'; | |
| const QueryResultFilter = ({ filter, filterExpanded, onChange, onExpandChange, mode }) => { | |
| const inputRef = useRef(null); | |
| const [isExpanded, setIsExpanded] = useState(filterExpanded || false); | |
| useEffect(() => { | |
| setIsExpanded(filterExpanded || false); | |
| }, [filterExpanded]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultFilter/index.js`
at line 7, The local state isExpanded is only initialized from the Redux prop
filterExpanded and won't follow updates; add a useEffect that watches
filterExpanded and calls setIsExpanded(filterExpanded) to keep local state in
sync, and ensure you import useEffect alongside useState at the top of the file
(so update the import that currently imports useState to include useEffect).
Comment on lines
+32
to
+35
| // Use props if provided (Redux), otherwise fallback to local state | ||
| // isCollapsed = true means content is shown (expanded state) | ||
| // When isExpanded is true, we want isCollapsed to be true (content shown) | ||
| const isCollapsed = isExpanded !== undefined ? isExpanded : localIsCollapsed; |
Contributor
There was a problem hiding this comment.
Inverted state mapping causes incorrect expansion behavior.
The comment claims isCollapsed = true means content is shown but the rendering logic at line 287 ({!isCollapsed && renderEventContent()}) shows content when isCollapsed is false. Similarly, line 262 shows the collapsed chevron when isCollapsed is true.
When isExpanded=true is passed from Redux, isCollapsed becomes true, which hides content instead of showing it. The mapping should be inverted:
🐛 Proposed fix
// Use props if provided (Redux), otherwise fallback to local state
- // isCollapsed = true means content is shown (expanded state)
- // When isExpanded is true, we want isCollapsed to be true (content shown)
- const isCollapsed = isExpanded !== undefined ? isExpanded : localIsCollapsed;
+ // When isExpanded is true (from Redux), we want isCollapsed to be false (content shown)
+ const isCollapsed = isExpanded !== undefined ? !isExpanded : localIsCollapsed;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/bruno-app/src/components/ResponsePane/Timeline/GrpcTimelineItem/index.js`
around lines 32 - 35, The prop-to-local state mapping is inverted: isExpanded
from Redux should make content visible, but current line sets isCollapsed =
isExpanded when it should set isCollapsed to the inverse; update the assignment
in the GrpcTimelineItem so that isCollapsed uses !isExpanded when isExpanded is
provided (i.e., replace the ternary to use the negation), keeping
localIsCollapsed as the fallback; reference the symbols isCollapsed, isExpanded,
and localIsCollapsed and ensure existing render logic like renderEventContent()
and chevron rendering continues to rely on isCollapsed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/bruno-app/src/components/ResponsePane/Timeline/GrpcTimelineItem/index.js (1)
32-36:⚠️ Potential issue | 🟠 MajorControlled expansion is still mapped backwards.
Line 32 makes
isExpanded={true}behave asisCollapsed={true}, so persisted expanded gRPC timeline items reopen as closed. Line 36 is inverted too: clicking a collapsed item sendsfalse, which keeps Redux in the collapsed state.Suggested fix
- const isCollapsed = isExpanded !== undefined ? isExpanded : localIsCollapsed; + const isCollapsed = isExpanded !== undefined ? !isExpanded : localIsCollapsed; const toggleCollapse = () => { if (onToggleExpand) { - // Pass !isCollapsed: if currently collapsed (isCollapsed=false), pass true to mean "expanded" - onToggleExpand(!isCollapsed); + onToggleExpand(isCollapsed); } else { setLocalIsCollapsed((prev) => !prev); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/ResponsePane/Timeline/GrpcTimelineItem/index.js` around lines 32 - 36, The controlled expansion boolean is inverted: the component maps the prop isExpanded into isCollapsed incorrectly and thus flips toggle events; change the computation in the GrpcTimelineItem so isCollapsed is derived as isCollapsed = isExpanded !== undefined ? !isExpanded : localIsCollapsed (flip the controlled value), and keep toggleCollapse calling onToggleExpand(!isCollapsed) so clicks emit the new expanded state correctly; update references in the file to use the corrected isCollapsed logic (isExpanded, localIsCollapsed, toggleCollapse, onToggleExpand).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js`:
- Around line 69-76: The local state columnWidths is only initialized from
storedColumnWidths once, causing widths to persist across tab switches; add a
useEffect that listens for changes to storedColumnWidths (and/or activeTabUid
and tableId) and calls setColumnWidths(storedColumnWidths || { name: '30%',
value: 'auto' }) to sync the local state when the focusedTab or its
tableColumnWidths change, and avoid updating state if the values are equal to
prevent unnecessary re-renders; reference focusedTab, storedColumnWidths,
columnWidths, setColumnWidths, activeTabUid, and tableId.
---
Duplicate comments:
In
`@packages/bruno-app/src/components/ResponsePane/Timeline/GrpcTimelineItem/index.js`:
- Around line 32-36: The controlled expansion boolean is inverted: the component
maps the prop isExpanded into isCollapsed incorrectly and thus flips toggle
events; change the computation in the GrpcTimelineItem so isCollapsed is derived
as isCollapsed = isExpanded !== undefined ? !isExpanded : localIsCollapsed (flip
the controlled value), and keep toggleCollapse calling
onToggleExpand(!isCollapsed) so clicks emit the new expanded state correctly;
update references in the file to use the corrected isCollapsed logic
(isExpanded, localIsCollapsed, toggleCollapse, onToggleExpand).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ed30ee6-731a-48dd-837c-b9cd5d9b6947
📒 Files selected for processing (5)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.jspackages/bruno-app/src/components/ResponsePane/Timeline/GrpcTimelineItem/index.jspackages/bruno-app/src/components/ResponsePane/Timeline/TimelineItem/index.jspackages/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/ResponsePane/Timeline/TimelineItem/index.js
Comment on lines
+69
to
+76
| // Get column widths from Redux - derived value (not state) | ||
| const focusedTab = tabs?.find((t) => t.uid === activeTabUid); | ||
| const storedColumnWidths = focusedTab?.tableColumnWidths?.[tableId]; | ||
|
|
||
| // Local state initialized from Redux (computed once on mount/environment change via key) | ||
| const [columnWidths, setColumnWidths] = useState(() => { | ||
| return storedColumnWidths || { name: '30%', value: 'auto' }; | ||
| }); |
Contributor
There was a problem hiding this comment.
Sync columnWidths when the active tab changes.
This state only reads storedColumnWidths on the first render. If the table stays mounted while activeTabUid changes, the previous tab’s widths keep rendering until a remount, so the new per-tab persistence leaks UI state across tabs.
Suggested fix
const [columnWidths, setColumnWidths] = useState(() => {
return storedColumnWidths || { name: '30%', value: 'auto' };
});
+
+ useEffect(() => {
+ setColumnWidths(storedColumnWidths || { name: '30%', value: 'auto' });
+ }, [tableId, storedColumnWidths]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js` around
lines 69 - 76, The local state columnWidths is only initialized from
storedColumnWidths once, causing widths to persist across tab switches; add a
useEffect that listens for changes to storedColumnWidths (and/or activeTabUid
and tableId) and calls setColumnWidths(storedColumnWidths || { name: '30%',
value: 'auto' }) to sync the local state when the focusedTab or its
tableColumnWidths change, and avoid updating state if the values are equal to
prevent unnecessary re-renders; reference focusedTab, storedColumnWidths,
columnWidths, setColumnWidths, activeTabUid, and tableId.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.js (1)
82-84: WraphandleColumnWidthsChangeinuseCallback.This function is recreated every render and is a dependency of
handleResizeStart(line 128). This causeshandleResizeStartto be recreated unnecessarily on each render.Suggested fix
- const handleColumnWidthsChange = (id, widths) => { + const handleColumnWidthsChange = useCallback((id, widths) => { dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId: id, widths })); - }; + }, [dispatch, activeTabUid]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js` around lines 82 - 84, The handler handleColumnWidthsChange is recreated every render; wrap it in React.useCallback to stabilize its identity so dependent callbacks (like handleResizeStart) don't get recreated. Change handleColumnWidthsChange to a useCallback that calls dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId: id, widths })) and include dispatch and activeTabUid in the dependency array so it updates correctly when those values change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js`:
- Line 128: The useEffect that adds the mouseup listener closes over tableId via
the handleMouseUp logic but only lists handleColumnWidthsChange in its
dependency array; update the dependency array to include tableId (e.g., change
"}, [handleColumnWidthsChange]);" to include tableId) so the effect re-runs when
tableId changes and avoid stale closures; if handleMouseUp is defined inline,
consider wrapping it with useCallback or moving its definition so it captures
the latest tableId consistently (refer to handleMouseUp and
handleColumnWidthsChange).
---
Nitpick comments:
In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js`:
- Around line 82-84: The handler handleColumnWidthsChange is recreated every
render; wrap it in React.useCallback to stabilize its identity so dependent
callbacks (like handleResizeStart) don't get recreated. Change
handleColumnWidthsChange to a useCallback that calls
dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId: id, widths }))
and include dispatch and activeTabUid in the dependency array so it updates
correctly when those values change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e86cea1f-a99a-48c8-89bb-61912c0dbc32
📒 Files selected for processing (1)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.js
| document.addEventListener('mousemove', handleMouseMove); | ||
| document.addEventListener('mouseup', handleMouseUp); | ||
| }, []); | ||
| }, [handleColumnWidthsChange]); |
Contributor
There was a problem hiding this comment.
Missing tableId in dependency array.
tableId is used inside handleMouseUp (line 121) but not listed as a dependency. This could cause stale closure issues if tableId changes while a resize is in progress.
- }, [handleColumnWidthsChange]);
+ }, [handleColumnWidthsChange, tableId]);📝 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
| }, [handleColumnWidthsChange]); | |
| }, [handleColumnWidthsChange, tableId]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js` at line
128, The useEffect that adds the mouseup listener closes over tableId via the
handleMouseUp logic but only lists handleColumnWidthsChange in its dependency
array; update the dependency array to include tableId (e.g., change "},
[handleColumnWidthsChange]);" to include tableId) so the effect re-runs when
tableId changes and avoid stale closures; if handleMouseUp is defined inline,
consider wrapping it with useCallback or moving its definition so it captures
the latest tableId consistently (refer to handleMouseUp and
handleColumnWidthsChange).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleMultipartFormParams/index.js (1)
27-29: Consider memoizinghandleColumnWidthsChangewithuseCallback.This handler creates a new function reference on every render. If
EditableTableis memoized or uses shallow prop comparison, this could trigger unnecessary re-renders.♻️ Proposed fix
- const handleColumnWidthsChange = (tableId, widths) => { - dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId, widths })); - }; + const handleColumnWidthsChange = useCallback((tableId, widths) => { + dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId, widths })); + }, [dispatch, activeTabUid]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleMultipartFormParams/index.js` around lines 27 - 29, handler handleColumnWidthsChange is recreated on every render causing potential unnecessary re-renders of memoized children; wrap it in React.useCallback to memoize it, e.g., memoize handleColumnWidthsChange that calls dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId, widths })) and include dependencies [dispatch, activeTabUid] so EditableTable receives a stable prop reference and avoids extra renders.packages/bruno-app/src/components/ResponseExample/ResponseExampleResponsePane/ResponseExampleResponseHeaders/index.js (1)
24-30: Extract the table id into a shared constant.
'example-response-headers'is used as the lookup key, dispatch payload, andEditableTableprop. Hoisting it avoids a silent read/write mismatch later.♻️ Suggested cleanup
+const RESPONSE_HEADERS_TABLE_ID = 'example-response-headers'; + const ResponseExampleResponseHeaders = ({ editMode, item, collection, exampleUid }) => { const dispatch = useDispatch(); const { storedTheme } = useTheme(); const tabs = useSelector((state) => state.tabs.tabs); const activeTabUid = useSelector((state) => state.tabs.activeTabUid); const [isBulkEditMode, setIsBulkEditMode] = useState(false); - // Get column widths from Redux const focusedTab = tabs?.find((t) => t.uid === activeTabUid); - const responseHeadersWidths = focusedTab?.tableColumnWidths?.['example-response-headers'] || {}; + const responseHeadersWidths = focusedTab?.tableColumnWidths?.[RESPONSE_HEADERS_TABLE_ID] || {}; const handleColumnWidthsChange = (tableId, widths) => { dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId, widths })); }; @@ <EditableTable - tableId="example-response-headers" + tableId={RESPONSE_HEADERS_TABLE_ID} columnWidths={responseHeadersWidths} - onColumnWidthsChange={(widths) => handleColumnWidthsChange('example-response-headers', widths)} + onColumnWidthsChange={(widths) => handleColumnWidthsChange(RESPONSE_HEADERS_TABLE_ID, widths)} columns={columns}As per coding guidelines, "Add meaningful comments instead of obvious ones where complex code flow is explained properly."
Also applies to: 184-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/ResponseExample/ResponseExampleResponsePane/ResponseExampleResponseHeaders/index.js` around lines 24 - 30, Replace the repeated string literal 'example-response-headers' with a single shared constant (e.g., TABLE_ID_EXAMPLE_RESPONSE_HEADERS) and use that constant everywhere it's used: when reading focusedTab?.tableColumnWidths?.[...], when dispatching updateTableColumnWidths in handleColumnWidthsChange, and when passing the tableId prop to EditableTable; also replace the vague comment "Get column widths from Redux" with a meaningful comment stating that column widths are per-tab and stored on focusedTab.tableColumnWidths keyed by the table id constant. Ensure references to focusedTab, responseHeadersWidths, handleColumnWidthsChange, updateTableColumnWidths, and EditableTable all use the new constant to avoid silent read/write mismatches.packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleHeaders/index.js (1)
23-29: Collapse the hardcodedtableIdwiring.
'example-headers'is fixed in this component, so the generic(tableId, widths)handler plus the extra wrapper onEditableTablejust adds indirection. I'd inline the constant into the handler and pass it through directly; the comment on Line 23 can also go since the selector code is already self-explanatory.Suggested cleanup
- // Get column widths from Redux const focusedTab = tabs?.find((t) => t.uid === activeTabUid); const exampleHeadersWidths = focusedTab?.tableColumnWidths?.['example-headers'] || {}; - const handleColumnWidthsChange = (tableId, widths) => { - dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId, widths })); + const handleColumnWidthsChange = (widths) => { + dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId: 'example-headers', widths })); }; ... - onColumnWidthsChange={(widths) => handleColumnWidthsChange('example-headers', widths)} + onColumnWidthsChange={handleColumnWidthsChange}As per coding guidelines, "Avoid single-line abstractions where all that's being done is increasing the call stack with one additional function" and "Add meaningful comments instead of obvious ones where complex code flow is explained properly."
Also applies to: 146-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleHeaders/index.js` around lines 23 - 29, The handler indirection can be removed: replace the generic handleColumnWidthsChange(tableId, widths) with a single-argument handler that uses the hardcoded tableId 'example-headers' directly (update the function referenced by EditableTable accordingly), and remove the now-redundant comment; specifically modify the existing handleColumnWidthsChange and its calls so it dispatches updateTableColumnWidths({ uid: activeTabUid, tableId: 'example-headers', widths }) and stop passing a tableId parameter from the EditableTable invocation, using focusedTab and exampleHeadersWidths as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleHeaders/index.js`:
- Around line 23-29: The handler indirection can be removed: replace the generic
handleColumnWidthsChange(tableId, widths) with a single-argument handler that
uses the hardcoded tableId 'example-headers' directly (update the function
referenced by EditableTable accordingly), and remove the now-redundant comment;
specifically modify the existing handleColumnWidthsChange and its calls so it
dispatches updateTableColumnWidths({ uid: activeTabUid, tableId:
'example-headers', widths }) and stop passing a tableId parameter from the
EditableTable invocation, using focusedTab and exampleHeadersWidths as-is.
In
`@packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleMultipartFormParams/index.js`:
- Around line 27-29: handler handleColumnWidthsChange is recreated on every
render causing potential unnecessary re-renders of memoized children; wrap it in
React.useCallback to memoize it, e.g., memoize handleColumnWidthsChange that
calls dispatch(updateTableColumnWidths({ uid: activeTabUid, tableId, widths }))
and include dependencies [dispatch, activeTabUid] so EditableTable receives a
stable prop reference and avoids extra renders.
In
`@packages/bruno-app/src/components/ResponseExample/ResponseExampleResponsePane/ResponseExampleResponseHeaders/index.js`:
- Around line 24-30: Replace the repeated string literal
'example-response-headers' with a single shared constant (e.g.,
TABLE_ID_EXAMPLE_RESPONSE_HEADERS) and use that constant everywhere it's used:
when reading focusedTab?.tableColumnWidths?.[...], when dispatching
updateTableColumnWidths in handleColumnWidthsChange, and when passing the
tableId prop to EditableTable; also replace the vague comment "Get column widths
from Redux" with a meaningful comment stating that column widths are per-tab and
stored on focusedTab.tableColumnWidths keyed by the table id constant. Ensure
references to focusedTab, responseHeadersWidths, handleColumnWidthsChange,
updateTableColumnWidths, and EditableTable all use the new constant to avoid
silent read/write mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2f29c37-e7b7-420c-8d21-66908583f297
📒 Files selected for processing (5)
packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleFileBody/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleFormUrlEncodedParams/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleHeaders/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleMultipartFormParams/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleResponsePane/ResponseExampleResponseHeaders/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleFileBody/index.js
- packages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleFormUrlEncodedParams/index.js
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/bruno-app/src/components/EditableTable/index.js (1)
77-85:⚠️ Potential issue | 🔴 CriticalUncontrolled resize is still bypassed.
Lines 83-85 and 110-112 still guard
handleColumnWidthsChange(...)behindisControlled, so unmanagedEditableTableinstances never updatelocalColumnWidthsduring drag or on mouseup. That regresses existing callers that don't passcolumnWidths/onColumnWidthsChange.Proposed fix
- if (isControlled) { - handleColumnWidthsChange(newWidths); - } + handleColumnWidthsChange(newWidths);- if (isControlled) { - handleColumnWidthsChange({ ...widths, ...newWidths }); - } + handleColumnWidthsChange({ ...widths, ...newWidths });Also applies to: 109-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/EditableTable/index.js` around lines 77 - 85, The resize logic currently only calls handleColumnWidthsChange when isControlled is true, so uncontrolled EditableTable never updates localColumnWidths during drag or on mouseup; change the behavior so after computing newWidths (the object with [columnKey] and [nextColumnKey]) you always update the internal state/localColumnWidths for uncontrolled instances (e.g., call the setter that updates localColumnWidths or dispatch the same internal update used elsewhere), and only conditionally call handleColumnWidthsChange if isControlled is true—apply this same change in both the drag/update path where newWidths is created and in the mouseup/finalize path that currently guards handleColumnWidthsChange behind isControlled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/bruno-app/src/components/EditableTable/index.js`:
- Around line 77-85: The resize logic currently only calls
handleColumnWidthsChange when isControlled is true, so uncontrolled
EditableTable never updates localColumnWidths during drag or on mouseup; change
the behavior so after computing newWidths (the object with [columnKey] and
[nextColumnKey]) you always update the internal state/localColumnWidths for
uncontrolled instances (e.g., call the setter that updates localColumnWidths or
dispatch the same internal update used elsewhere), and only conditionally call
handleColumnWidthsChange if isControlled is true—apply this same change in both
the drag/update path where newWidths is created and in the mouseup/finalize path
that currently guards handleColumnWidthsChange behind isControlled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf07b8bc-5266-4cda-bfa2-ec989626a298
📒 Files selected for processing (1)
packages/bruno-app/src/components/EditableTable/index.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
JIRA
Problem
UI State Lost When Switching Tabs or Navigating Between Requests
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