added copy button to copy response (#5409)#6266
Conversation
Co-authored-by: Shashank Shekhar <48152748+sha5-git@users.noreply.github.com>
|
Warning Rate limit exceeded@sid-bruno has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughAdds a ResponseCopy button and a ResponseActions dropdown to ResponsePane, refactors ResponseClear and ResponseSave to support rendering as dropdown items and to call onClose, swaps the Save/Clear UI with the new components, and adds an E2E test verifying copying a response to the clipboard. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.js (1)
1-29: LGTM! Consider extracting shared button styles.The StyledWrapper is well-structured and follows the project's theming patterns. However, I notice similar button styling exists in ResponseCopy/StyledWrapper.js. Consider extracting common button styles to a shared styled component to reduce duplication and ensure consistency.
tests/response/response-actions.spec.ts (1)
1-51: LGTM! Well-structured test with one recommendation.The test follows Playwright best practices:
- Uses
test.stepfor clear reporting- Multiple assertions
- Proper cleanup with
afterAll- Locator-based element selection
However, the test depends on the external service
httpbin.org(line 26), which could introduce flakiness. Consider using a local mock server or fixture-based response data for more reliable test execution.packages/bruno-app/src/components/ResponsePane/ResponseActions/index.js (1)
13-19: Add displayName for better debugging.The
forwardRefcomponent should have adisplayNamefor better React DevTools experience and clearer error messages.Apply this diff:
const MenuIcon = forwardRef((_props, ref) => { return ( <div ref={ref} className="cursor-pointer"> <IconDots size={18} strokeWidth={1.5} /> </div> ); }); + MenuIcon.displayName = 'MenuIcon';packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (1)
19-34: Simplify redundant error handling.The function has redundant error handling with both a
.catch()on the promise and an outertry-catchblock. Both show the same error message. The outertry-catchis unnecessary since the clipboard API errors are already handled by.catch().Apply this diff to simplify:
const copyResponse = () => { - try { - const textToCopy = typeof response.data === 'string' - ? response.data - : JSON.stringify(response.data, null, 2); + const textToCopy = typeof response.data === 'string' + ? response.data + : JSON.stringify(response.data, null, 2); - navigator.clipboard.writeText(textToCopy).then(() => { - toast.success('Response copied to clipboard'); - setCopied(true); - }).catch(() => { - toast.error('Failed to copy response'); - }); - } catch (error) { + navigator.clipboard.writeText(textToCopy) + .then(() => { + toast.success('Response copied to clipboard'); + setCopied(true); + }) + .catch(() => { - toast.error('Failed to copy response'); - } + toast.error('Failed to copy response'); + }); };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseActions/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseClear/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js(2 hunks)packages/bruno-app/src/components/ResponsePane/index.js(2 hunks)tests/response/response-actions.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
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
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.jspackages/bruno-app/src/components/ResponsePane/ResponseClear/index.jstests/response/response-actions.spec.tspackages/bruno-app/src/components/ResponsePane/ResponseActions/index.jspackages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.jspackages/bruno-app/src/components/ResponsePane/ResponseCopy/index.jspackages/bruno-app/src/components/ResponsePane/ResponseSave/index.jspackages/bruno-app/src/components/ResponsePane/index.js
tests/**/**.*
⚙️ CodeRabbit configuration file
tests/**/**.*: Review the following e2e test code written using the Playwright test library. Ensure that:
Follow best practices for Playwright code and e2e automation
Try to reduce usage of
page.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright callsAvoid using
page.pause()in codeUse locator variables for locators
Avoid using test.only
Use multiple assertions
Promote the use of
test.stepas much as possible so the generated reports are easier to readEnsure that the
fixtureslike the collections are nested inside thefixturesfolderFixture Example*: Here's an example of possible fixture and test pair
. ├── fixtures │ └── collection │ ├── base.bru │ ├── bruno.json │ ├── collection.bru │ ├── ws-test-request-with-headers.bru │ ├── ws-test-request-with-subproto.bru │ └── ws-test-request.bru ├── connection.spec.ts # <- Depends on the collection in ./fixtures/collection ├── headers.spec.ts ├── persistence.spec.ts ├── variable-interpolation │ ├── fixtures │ │ └── collection │ │ ├── environments │ │ ├── bruno.json │ │ └── ws-interpolation-test.bru │ ├── init-user-data │ └── variable-interpolation.spec.ts # <- Depends on the collection in ./variable-interpolation/fixtures/collection └── subproto.spec.ts
Files:
tests/response/response-actions.spec.ts
🧬 Code graph analysis (6)
packages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.js (1)
packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js (1)
StyledWrapper(3-6)
tests/response/response-actions.spec.ts (1)
tests/utils/page/actions.ts (1)
closeAllCollections(130-130)
packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js (1)
packages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.js (1)
StyledWrapper(3-27)
packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (4)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (1)
response(9-9)packages/bruno-app/src/components/ResponsePane/index.js (1)
response(55-55)packages/bruno-js/src/sandbox/quickjs/shims/bruno-request.js (1)
setTimeout(115-117)packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js (1)
StyledWrapper(3-6)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (3)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (17)
window(2586-2586)item(285-285)item(306-306)item(390-390)item(411-411)item(448-448)item(487-487)item(608-608)item(640-640)item(655-655)item(734-749)item(765-765)item(776-776)item(844-844)item(858-858)item(907-907)item(933-933)packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (1)
response(7-7)packages/bruno-app/src/components/ResponsePane/index.js (1)
response(55-55)
packages/bruno-app/src/components/ResponsePane/index.js (2)
packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (1)
ResponseCopy(6-47)packages/bruno-app/src/components/ResponsePane/ResponseActions/index.js (1)
ResponseActions(8-33)
⏰ 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: SSL Tests - Windows
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - macOS
- GitHub Check: Playwright E2E Tests
- GitHub Check: Unit Tests
- GitHub Check: CLI Tests
🔇 Additional comments (5)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (1)
7-12: LGTM! Props extended appropriately.The new props
asDropdownItemandonCloseare integrated correctly. CallingonClose()before initiating the save operation ensures the dropdown closes immediately, providing good UX.packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js (1)
1-8: LGTM!Clean, focused styling that follows the project's theming approach.
packages/bruno-app/src/components/ResponsePane/ResponseClear/index.js (1)
7-28: LGTM! Props and rendering paths integrated correctly.The
asDropdownItemandonCloseprops are implemented consistently withResponseSave. TheonClosecallback is properly invoked before the clear action, ensuring the dropdown closes immediately.packages/bruno-app/src/components/ResponsePane/index.js (2)
19-21: LGTM! Import statements follow project conventions.The new imports use the
src/alias consistently with the rest of the file.
188-192: LGTM! Component integration is correct.The new
ResponseCopyandResponseActionscomponents are rendered with appropriate props and conditional logic. They only appear for successful responses (non-error), which provides a good user experience.
4c11726 to
4de225f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (1)
25-37: Remove invaliddisabledattribute from div.The
disabledattribute has no effect on div elements. The visual styling is correct, but the attribute itself should be removed. Consider usingaria-disabledfor accessibility if needed.<div className="dropdown-item" onClick={saveResponseToFile} - disabled={!response.dataBuffer} + aria-disabled={!response.dataBuffer} style={!response.dataBuffer ? { opacity: 0.5, cursor: 'not-allowed' } : {}} >packages/bruno-app/src/components/ResponsePane/ResponseActions/index.js (1)
21-23: Add null check for ref access.Guard against potential null access on the ref.
const handleClose = () => { - menuDropdownTippyRef.current.hide(); + menuDropdownTippyRef.current?.hide(); };
🧹 Nitpick comments (1)
packages/bruno-app/src/components/ResponsePane/ResponseActions/index.js (1)
13-19: MoveMenuIconoutside the component to prevent unnecessary re-renders.Defining
MenuIconinsideResponseActionscreates a new component identity on every render. This can cause React to unmount/remount the dropdown trigger unnecessarily. Move it outside or wrap withuseMemo.+const MenuIcon = forwardRef((_props, ref) => { + return ( + <div ref={ref} className="cursor-pointer"> + <IconDots size={18} strokeWidth={1.5} /> + </div> + ); +}); +MenuIcon.displayName = 'MenuIcon'; + const ResponseActions = ({ collection, item }) => { const menuDropdownTippyRef = useRef(); const onMenuDropdownCreate = (ref) => (menuDropdownTippyRef.current = ref); - const MenuIcon = forwardRef((_props, ref) => { - return ( - <div ref={ref} className="cursor-pointer"> - <IconDots size={18} strokeWidth={1.5} /> - </div> - ); - }); - const handleClose = () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseActions/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseClear/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js(2 hunks)packages/bruno-app/src/components/ResponsePane/index.js(2 hunks)tests/response/response-actions.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/response/response-actions.spec.ts
- packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js
- packages/bruno-app/src/components/ResponsePane/index.js
- packages/bruno-app/src/components/ResponsePane/ResponseActions/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
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
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
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.jspackages/bruno-app/src/components/ResponsePane/ResponseSave/index.jspackages/bruno-app/src/components/ResponsePane/ResponseActions/index.jspackages/bruno-app/src/components/ResponsePane/ResponseClear/index.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: sid-bruno
Repo: usebruno/bruno PR: 6266
File: packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js:38-38
Timestamp: 2025-12-02T09:45:31.698Z
Learning: In the ResponseCopy component (packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js), the copy button is intentionally disabled using `!response.data` to prevent copying stream resets which result in empty strings.
📚 Learning: 2025-12-02T09:45:31.698Z
Learnt from: sid-bruno
Repo: usebruno/bruno PR: 6266
File: packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js:38-38
Timestamp: 2025-12-02T09:45:31.698Z
Learning: In the ResponseCopy component (packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js), the copy button is intentionally disabled using `!response.data` to prevent copying stream resets which result in empty strings.
Applied to files:
packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.jspackages/bruno-app/src/components/ResponsePane/ResponseSave/index.jspackages/bruno-app/src/components/ResponsePane/ResponseActions/index.js
🧬 Code graph analysis (3)
packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (3)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (1)
response(9-9)packages/bruno-app/src/components/ResponsePane/index.js (1)
response(55-55)packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js (1)
StyledWrapper(3-6)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (2)
packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (1)
response(7-7)packages/bruno-app/src/components/ResponsePane/index.js (1)
response(55-55)
packages/bruno-app/src/components/ResponsePane/ResponseClear/index.js (1)
packages/bruno-app/src/components/ResponsePane/index.js (1)
dispatch(30-30)
⏰ 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: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
- GitHub Check: Playwright E2E Tests
- GitHub Check: Unit Tests
- GitHub Check: CLI Tests
🔇 Additional comments (2)
packages/bruno-app/src/components/ResponsePane/ResponseClear/index.js (1)
7-28: LGTM!Clean implementation of the dropdown item rendering path. The
onClosecallback placement before the dispatch is appropriate here since the clear action always succeeds.packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (1)
1-48: LGTM! Clean implementation with proper error handling and user feedback.The component is well-structured with appropriate state management, comprehensive error handling for both synchronous and asynchronous failures, and good user experience with toast notifications and visual feedback. The
disabled={!response.data}condition at line 38 is intentional per previous discussion.Based on learnings, the disabled condition prevents copying stream resets that result in empty strings.
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js
Outdated
Show resolved
Hide resolved
4de225f to
9d19e8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (1)
11-31: GuarddataBufferbeforeonCloseand tidy disabled dropdown behavior.Right now, clicking the visually disabled “Download” item will still trigger
onClose()because the guard runs afterward. If you want the dropdown to stay open when there’s nothing to save, it’s safer to short‑circuit first:const saveResponseToFile = () => { - if (onClose) onClose(); - if (!response.dataBuffer) return; + if (!response.dataBuffer) return; + if (onClose) onClose(); return new Promise((resolve, reject) => {You can then rely on the early return and visual styles, and optionally drop the non‑semantic
disabledattribute from the<div className="dropdown-item">, since it doesn’t affect click behavior:- <div - className="dropdown-item" - onClick={saveResponseToFile} - disabled={!response.dataBuffer} - style={!response.dataBuffer ? { opacity: 0.5, cursor: 'not-allowed' } : {}} - > + <div + className="dropdown-item" + onClick={saveResponseToFile} + style={!response.dataBuffer ? { opacity: 0.5, cursor: 'not-allowed' } : {}} + >
🧹 Nitpick comments (2)
packages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.js (1)
18-26: Addcursor: pointerto.cursor-pointerhelper class for consistency.The
.cursor-pointerhelper name suggests a pointer cursor but the rule only sets layout and color. Consider addingcursor: pointerso its behavior matches its name and intent:.cursor-pointer { display: flex; align-items: center; color: var(--color-tab-inactive); + cursor: pointer; + &:hover { color: var(--color-tab-active); } }tests/response/response-actions.spec.ts (1)
30-37: Avoid external HTTP dependencies in E2E tests for stability.This step depends on
https://httpbin.org/json, which can introduce network and third‑party flakiness into your CI. If possible, switch to a local test server, mock, or a bundled fixture response so the “send request and wait for response” flow is deterministic and independent of external services.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseActions/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseClear/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js(2 hunks)packages/bruno-app/src/components/ResponsePane/index.js(2 hunks)tests/response/response-actions.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bruno-app/src/components/ResponsePane/ResponseActions/index.js
- packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js
- packages/bruno-app/src/components/ResponsePane/index.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
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
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/ResponsePane/ResponseClear/index.jspackages/bruno-app/src/components/ResponsePane/ResponseSave/index.jspackages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.jstests/response/response-actions.spec.tspackages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js
tests/**/**.*
⚙️ CodeRabbit configuration file
tests/**/**.*: Review the following e2e test code written using the Playwright test library. Ensure that:
Follow best practices for Playwright code and e2e automation
Try to reduce usage of
page.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright callsAvoid using
page.pause()in codeUse locator variables for locators
Avoid using test.only
Use multiple assertions
Promote the use of
test.stepas much as possible so the generated reports are easier to readEnsure that the
fixtureslike the collections are nested inside thefixturesfolderFixture Example*: Here's an example of possible fixture and test pair
. ├── fixtures │ └── collection │ ├── base.bru │ ├── bruno.json │ ├── collection.bru │ ├── ws-test-request-with-headers.bru │ ├── ws-test-request-with-subproto.bru │ └── ws-test-request.bru ├── connection.spec.ts # <- Depends on the collection in ./fixtures/collection ├── headers.spec.ts ├── persistence.spec.ts ├── variable-interpolation │ ├── fixtures │ │ └── collection │ │ ├── environments │ │ ├── bruno.json │ │ └── ws-interpolation-test.bru │ ├── init-user-data │ └── variable-interpolation.spec.ts # <- Depends on the collection in ./variable-interpolation/fixtures/collection └── subproto.spec.ts
Files:
tests/response/response-actions.spec.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: sid-bruno
Repo: usebruno/bruno PR: 6266
File: packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js:38-38
Timestamp: 2025-12-02T09:45:31.698Z
Learning: In the ResponseCopy component (packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js), the copy button is intentionally disabled using `!response.data` to prevent copying stream resets which result in empty strings.
📚 Learning: 2025-12-02T09:45:31.698Z
Learnt from: sid-bruno
Repo: usebruno/bruno PR: 6266
File: packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js:38-38
Timestamp: 2025-12-02T09:45:31.698Z
Learning: In the ResponseCopy component (packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js), the copy button is intentionally disabled using `!response.data` to prevent copying stream resets which result in empty strings.
Applied to files:
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.jspackages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js
🧬 Code graph analysis (5)
packages/bruno-app/src/components/ResponsePane/ResponseClear/index.js (1)
packages/bruno-app/src/components/ResponsePane/index.js (1)
dispatch(30-30)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (2)
packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (1)
response(7-7)packages/bruno-app/src/components/ResponsePane/index.js (1)
response(55-55)
packages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.js (1)
packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js (1)
StyledWrapper(3-6)
tests/response/response-actions.spec.ts (1)
tests/utils/page/actions.ts (1)
closeAllCollections(130-130)
packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (3)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (1)
response(9-9)packages/bruno-app/src/components/ResponsePane/index.js (1)
response(55-55)packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js (1)
StyledWrapper(3-6)
⏰ 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: Unit Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - Linux
- GitHub Check: CLI Tests
- GitHub Check: SSL Tests - macOS
🔇 Additional comments (2)
packages/bruno-app/src/components/ResponsePane/ResponseClear/index.js (1)
7-36: Dropdown integration for Clear action looks consistent.
asDropdownItemandonCloseare wired cleanly, and both the dropdown item and button share the sameclearResponselogic, keeping behavior consistent across entry points.packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (1)
6-45: Copy behavior and disabled-state handling align with the intended design.The copy logic, success/error toasts, and 2‑second
copiedstate toggle are straightforward, and gating the button with!response.datamatches the documented intent to avoid copying stream‑reset empty responses. Based on learnings, this looks correct and aligned with the rest of the ResponsePane behavior.
9d19e8b to
5dabd60
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (1)
25-37: Dropdown item variant looks good.The guard in
saveResponseToFilehandles the disabled state functionally, and the inline styles provide appropriate visual feedback. Previous review concerns have been addressed.One minor note: the
disabledattribute on line 30 is technically invalid HTML for adiv, but since the click behavior is properly guarded and this was already discussed in prior reviews, leaving as-is is acceptable.
🧹 Nitpick comments (3)
packages/bruno-app/src/components/ResponsePane/ResponseClear/index.js (1)
21-27: Consider making the dropdown item keyboard-accessibleUsing a clickable
<div>works, but it won’t be focusable or activatable via keyboard by default. Consider using a<button>styled as a dropdown item, or addingrole="menuitem",tabIndex={0}, and anonKeyDownhandler for Enter/Space to improve accessibility.packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (2)
19-34: Consider async/await for cleaner readability.The current error handling is correct, but using async/await would make the flow more straightforward and align with modern patterns.
Apply this diff to refactor with async/await:
- const copyResponse = () => { + const copyResponse = async () => { try { const textToCopy = typeof response.data === 'string' ? response.data : JSON.stringify(response.data, null, 2); - navigator.clipboard.writeText(textToCopy).then(() => { + await navigator.clipboard.writeText(textToCopy); toast.success('Response copied to clipboard'); setCopied(true); - }).catch(() => { - toast.error('Failed to copy response'); - }); } catch (error) { toast.error('Failed to copy response'); } };
36-46: Consider adding aria-label for screen readers.The button has a title attribute for hover tooltips, but an aria-label would improve accessibility for screen reader users.
Apply this diff to enhance accessibility:
- <button onClick={copyResponse} disabled={!response.data} title="Copy response to clipboard"> + <button onClick={copyResponse} disabled={!response.data} title="Copy response to clipboard" aria-label="Copy response to clipboard">
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseActions/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseClear/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js(1 hunks)packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js(2 hunks)packages/bruno-app/src/components/ResponsePane/index.js(2 hunks)tests/response/response-actions.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js
- packages/bruno-app/src/components/ResponsePane/ResponseActions/index.js
- packages/bruno-app/src/components/ResponsePane/index.js
- packages/bruno-app/src/components/ResponsePane/ResponseActions/StyledWrapper.js
- tests/response/response-actions.spec.ts
🧰 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
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
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
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/ResponsePane/ResponseClear/index.jspackages/bruno-app/src/components/ResponsePane/ResponseCopy/index.jspackages/bruno-app/src/components/ResponsePane/ResponseSave/index.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: sid-bruno
Repo: usebruno/bruno PR: 6266
File: packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js:38-38
Timestamp: 2025-12-02T09:45:31.698Z
Learning: In the ResponseCopy component (packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js), the copy button is intentionally disabled using `!response.data` to prevent copying stream resets which result in empty strings.
📚 Learning: 2025-12-02T09:45:31.698Z
Learnt from: sid-bruno
Repo: usebruno/bruno PR: 6266
File: packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js:38-38
Timestamp: 2025-12-02T09:45:31.698Z
Learning: In the ResponseCopy component (packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js), the copy button is intentionally disabled using `!response.data` to prevent copying stream resets which result in empty strings.
Applied to files:
packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.jspackages/bruno-app/src/components/ResponsePane/ResponseSave/index.js
🧬 Code graph analysis (2)
packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (3)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (1)
response(9-9)packages/bruno-app/src/components/ResponsePane/index.js (1)
response(55-55)packages/bruno-app/src/components/ResponsePane/ResponseCopy/StyledWrapper.js (1)
StyledWrapper(3-6)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (2)
packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (1)
response(7-7)packages/bruno-app/src/components/ResponsePane/index.js (1)
response(55-55)
⏰ 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: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
- GitHub Check: CLI Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: Unit Tests
🔇 Additional comments (6)
packages/bruno-app/src/components/ResponsePane/ResponseSave/index.js (2)
7-9: LGTM!Clean signature extension with
asDropdownItemandonCloseprops. The conditional handling throughout the component properly accounts for these being optional.
11-23: Guard placement now correct.The early return on line 12 now precedes the
onClose()call, ensuring the dropdown stays open when no data buffer exists. This addresses the previous review concern.packages/bruno-app/src/components/ResponsePane/ResponseClear/index.js (1)
7-19: Clear handler and new props wiring look solidExtending the signature with
asDropdownItem/onCloseand routing everything through a singleclearResponsethat optionally callsonClosebefore dispatchingresponseClearedis clean and keeps behavior consistent with the previous implementation. No functional issues from my side here.packages/bruno-app/src/components/ResponsePane/ResponseCopy/index.js (3)
1-8: LGTM! Clean imports and consistent response extraction.The response extraction pattern matches other components in the codebase (ResponseSave, ResponsePane), maintaining consistency.
10-17: LGTM! Proper cleanup of timeout.The useEffect correctly clears the timeout on unmount or when the effect re-runs, preventing memory leaks.
38-38: LGTM! Disabled condition is intentional.The
!response.datacheck correctly prevents copying stream resets that result in empty strings, as confirmed in previous discussions.Based on learnings from previous review discussions.
* added copy button to copy response * add: response action component * fix: lint error * add: playwright test --------- Co-authored-by: Shashank Shekhar <48152748+sha5-git@users.noreply.github.com> Co-authored-by: Sid <siddharth@usebruno.com>
5dabd60 to
2a251b1
Compare
Description
Extends #5409
JIRA
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
Enhancements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.