fix: allow file selection in multipart form without entering a key first#7640
Merged
sid-bruno merged 4 commits intousebruno:mainfrom Apr 2, 2026
Merged
Conversation
pooja-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:
WalkthroughUpserts multipart form file params by row UID on file selection, reorders the clear-file button in the file-value cell, adjusts button/cell styling across two styled wrappers, and adds a serial Playwright/Electron E2E test that stubs file selection and verifies the filename appears. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer as Renderer (UI)
participant Dialog as Electron Dialog
participant Store
rect rgba(200,200,255,0.5)
User->>Renderer: Click .upload-btn
end
rect rgba(200,255,200,0.5)
Renderer->>Dialog: showOpenDialog()
Dialog-->>Renderer: { canceled: false, filePaths }
end
rect rgba(255,200,200,0.5)
Renderer->>Store: upsert multipart param (uid, type:'file', value:filePaths, defaults)
Store-->>Renderer: updated params/state
Renderer->>User: render file-value cell (filename + clear button)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🧹 Nitpick comments (2)
packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (1)
161-167: Add stable test IDs to file action controls.Since this button is part of a Playwright-covered flow, please add
data-testidso tests don’t depend on CSS class selectors.As per coding guidelines: "Add
data-testidto testable elements for Playwright."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js` around lines 161 - 167, Add a stable data-testid attribute to the file-remove button so Playwright tests don't rely on CSS classes: update the button element that uses onClick={() => handleClearFile(row)} (currently has className "clear-file-btn" and renders <IconX />) to include a deterministic data-testid (e.g. data-testid="remove-file-button" or data-testid={`remove-file-${row.id}`} if row.id is available) so tests can target it reliably.tests/request/multipart-form/multipart-form-file-select.spec.ts (1)
52-67: Add one cancel-flow assertion for this feature.This spec validates the success path well; please add a second test where file picker returns
canceled: trueand assert no file cell/filename is shown.As per coding guidelines: "Cover both the 'happy path' and the realistically problematic paths. Validate expected success behaviour, but also validate error handling, edge cases, and degraded-mode behaviour when appropriate."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/request/multipart-form/multipart-form-file-select.spec.ts` around lines 52 - 67, Add a new test (e.g., "file select should handle cancel flow") that mirrors the happy-path test but stubs the page file-picker to simulate a user canceling: before clicking the upload button (use buildCommonLocators(page).table('editable-table') to find lastRow and uploadBtn), override the app's picker (e.g., window.showOpenFilePicker or the app-specific picker function used by the upload button) to return an empty result/canceled response, then click uploadBtn and assert that the file cell (table.allRows().locator('.file-value-cell').first() or similar) does NOT show the filename "test-file.txt" and remains empty/not visible; ensure you use the same locators (lastRow, uploadBtn, fileCell) so the test verifies the cancel-flow behavior.
🤖 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/RequestPane/MultipartFormParams/index.js`:
- Around line 78-81: When building updatedParams in the append path, the code
overwrites existing row state by using hardcoded enabled: true and contentType:
''. Change that to preserve any existing row values by using row.enabled
(fallback to true) and row.contentType (fallback to empty string) so user
toggles or typed contentType aren’t lost; e.g., construct the new entry as {
uid: row.uid, name: row.name || '', enabled: row.enabled ?? true, type: 'file',
value: processedPaths, contentType: row.contentType ?? '' } replacing the
current hardcoded properties in updatedParams.
In `@tests/request/multipart-form/multipart-form-file-select.spec.ts`:
- Around line 11-13: The test patches the global Electron dialog API
(dialog.showOpenDialog) in beforeAll and saves the original, but the mocked
implementation is never restored, causing cross-test pollution; update the
afterAll hook (currently calling closeAllCollections) to accept electronApp,
retrieve the backed-up original dialog (the variable used when patching
dialog.showOpenDialog in beforeAll), and restore dialog.showOpenDialog to that
original before or after calling closeAllCollections to ensure subsequent tests
run with the real Electron dialog implementation.
---
Nitpick comments:
In `@packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js`:
- Around line 161-167: Add a stable data-testid attribute to the file-remove
button so Playwright tests don't rely on CSS classes: update the button element
that uses onClick={() => handleClearFile(row)} (currently has className
"clear-file-btn" and renders <IconX />) to include a deterministic data-testid
(e.g. data-testid="remove-file-button" or data-testid={`remove-file-${row.id}`}
if row.id is available) so tests can target it reliably.
In `@tests/request/multipart-form/multipart-form-file-select.spec.ts`:
- Around line 52-67: Add a new test (e.g., "file select should handle cancel
flow") that mirrors the happy-path test but stubs the page file-picker to
simulate a user canceling: before clicking the upload button (use
buildCommonLocators(page).table('editable-table') to find lastRow and
uploadBtn), override the app's picker (e.g., window.showOpenFilePicker or the
app-specific picker function used by the upload button) to return an empty
result/canceled response, then click uploadBtn and assert that the file cell
(table.allRows().locator('.file-value-cell').first() or similar) does NOT show
the filename "test-file.txt" and remains empty/not visible; ensure you use the
same locators (lastRow, uploadBtn, fileCell) so the test verifies the
cancel-flow behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a48ca3e6-bce8-4493-9523-1378145da02c
📒 Files selected for processing (4)
packages/bruno-app/src/components/RequestPane/MultipartFormParams/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleMultipartFormParams/StyledWrapper.jstests/request/multipart-form/multipart-form-file-select.spec.ts
pooja-bruno
force-pushed
the
fix/multipart-form-file-select-without-key
branch
from
46abb07 to
f8c68d5
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (1)
68-82:⚠️ Potential issue | 🟡 MinorPreserve row state in the append branch.
On Line 80, the appended object hardcodes
enabledandcontentType, which can drop values already present onrow. Please preserve row values with fallbacks instead.Proposed fix
- updatedParams = [ - ...(currentParams || []), - { uid: row.uid, name: row.name || '', enabled: true, type: 'file', value: processedPaths, contentType: '' } - ]; + updatedParams = [ + ...(currentParams || []), + { + uid: row.uid, + name: row.name || '', + enabled: row.enabled ?? true, + type: 'file', + value: processedPaths, + contentType: row.contentType ?? '' + } + ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js` around lines 68 - 82, The append branch currently creates a new param object that hardcodes enabled and contentType, dropping any existing values from row; update the append branch that builds updatedParams so it spreads row and uses fallbacks (e.g., name: row.name || '', enabled: row.enabled ?? true, type: 'file', value: processedPaths, contentType: row.contentType ?? '') instead of hardcoding those fields—locate the block that checks existsInParams and the array construction that pushes { uid: row.uid, name: ..., enabled: true, type: 'file', value: processedPaths, contentType: '' } and replace it with a version that preserves row fields with fallbacks.
🧹 Nitpick comments (1)
packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (1)
161-167: Add a stable test selector for the clear-file control.Line 161 adds an interactive button without
data-testid; adding one will keep Playwright selectors resilient.Suggested change
<button + data-testid="multipart-clear-file-btn" className="clear-file-btn ml-1" onClick={() => handleClearFile(row)} title="Remove file" >As per coding guidelines: "Add
data-testidto testable elements for Playwright."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js` around lines 161 - 167, Add a stable test selector to the clear-file button in the multipart form UI: update the button with className "clear-file-btn" that calls handleClearFile(row) to include a data-testid attribute (e.g., data-testid="clear-file" or data-testid="clear-file-<rowId>" for per-row uniqueness) so Playwright can reliably target it; ensure the chosen value is stable across renders and update any tests to use this new selector.
🤖 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/RequestPane/MultipartFormParams/index.js`:
- Around line 68-82: The append branch currently creates a new param object that
hardcodes enabled and contentType, dropping any existing values from row; update
the append branch that builds updatedParams so it spreads row and uses fallbacks
(e.g., name: row.name || '', enabled: row.enabled ?? true, type: 'file', value:
processedPaths, contentType: row.contentType ?? '') instead of hardcoding those
fields—locate the block that checks existsInParams and the array construction
that pushes { uid: row.uid, name: ..., enabled: true, type: 'file', value:
processedPaths, contentType: '' } and replace it with a version that preserves
row fields with fallbacks.
---
Nitpick comments:
In `@packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js`:
- Around line 161-167: Add a stable test selector to the clear-file button in
the multipart form UI: update the button with className "clear-file-btn" that
calls handleClearFile(row) to include a data-testid attribute (e.g.,
data-testid="clear-file" or data-testid="clear-file-<rowId>" for per-row
uniqueness) so Playwright can reliably target it; ensure the chosen value is
stable across renders and update any tests to use this new selector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c2057e3-bdb4-4fd2-8f33-9905c0f55ab0
📒 Files selected for processing (4)
packages/bruno-app/src/components/RequestPane/MultipartFormParams/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleMultipartFormParams/StyledWrapper.jstests/request/multipart-form/multipart-form-file-select.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/bruno-app/src/components/RequestPane/MultipartFormParams/StyledWrapper.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/request/multipart-form/multipart-form-file-select.spec.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/request/multipart-form/multipart-form-file-select.spec.ts (2)
55-56: Preferdata-testidlocators over CSS classes in this spec.Using
.dropdown-item,.upload-btn, and.file-value-cellmakes the test fragile to style/markup refactors. Please switch to test IDs for these interactive/asserted elements. As per coding guidelines, "Adddata-testidto testable elements for Playwright."Also applies to: 64-65, 70-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/request/multipart-form/multipart-form-file-select.spec.ts` around lines 55 - 56, The test uses fragile CSS class selectors ('.dropdown-item', '.upload-btn', '.file-value-cell') — update the app/test fixtures to add stable data-testid attributes to the interactive/asserted elements (e.g., data-testid="dropdown-multipart-form" for the dropdown option, data-testid="upload-btn" for the upload button, data-testid="file-value" for the file cell) and then change the Playwright locators in multipart-form-file-select.spec.ts to use locator('[data-testid="..."]') or page.getByTestId(...) for those elements (replace occurrences of '.dropdown-item', '.upload-btn', and '.file-value-cell', including the other mentioned occurrences around lines 64–70). Ensure the test IDs are unique and descriptive for the Multipart Form context so the spec remains robust to styling/markup changes.
62-74: Scope the verification to the same row that triggered upload.Line 70 currently finds the first file-value cell in the table, so the test can pass even if a different row updates. Capture the target row once and assert within that row.
Suggested patch
test('file select should work on empty row without a key', async ({ page }) => { const table = buildCommonLocators(page).table('editable-table'); + const targetRowIndex = (await table.allRows().count()) - 1; + const targetRow = table.allRows().nth(targetRowIndex); await test.step('Click upload on empty last row (no key entered)', async () => { - const lastRow = table.allRows().last(); - const uploadBtn = lastRow.locator('.upload-btn'); + const uploadBtn = targetRow.locator('.upload-btn'); await expect(uploadBtn).toBeVisible(); await uploadBtn.click(); }); await test.step('Verify the file name appears in the row', async () => { - const fileCell = table.allRows().locator('.file-value-cell').first(); + const fileCell = targetRow.locator('.file-value-cell'); await expect(fileCell).toBeVisible({ timeout: 5000 }); await expect(fileCell).toContainText('test-file.txt', { timeout: 5000 }); }); });As per coding guidelines, tests should be behaviour-driven and "Aim for tests that fail usefully."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/request/multipart-form/multipart-form-file-select.spec.ts` around lines 62 - 74, The verification currently queries the first .file-value-cell in the table which may be a different row; instead capture the target row (the same lastRow used to click the .upload-btn) and scope the assertions to that row by using lastRow.locator('.file-value-cell') (replace the table.allRows().locator(...).first() usage), then assert visibility and that it contains 'test-file.txt' (retain the existing timeouts) to ensure the test verifies the specific row that triggered the upload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/request/multipart-form/multipart-form-file-select.spec.ts`:
- Around line 55-56: The test uses fragile CSS class selectors
('.dropdown-item', '.upload-btn', '.file-value-cell') — update the app/test
fixtures to add stable data-testid attributes to the interactive/asserted
elements (e.g., data-testid="dropdown-multipart-form" for the dropdown option,
data-testid="upload-btn" for the upload button, data-testid="file-value" for the
file cell) and then change the Playwright locators in
multipart-form-file-select.spec.ts to use locator('[data-testid="..."]') or
page.getByTestId(...) for those elements (replace occurrences of
'.dropdown-item', '.upload-btn', and '.file-value-cell', including the other
mentioned occurrences around lines 64–70). Ensure the test IDs are unique and
descriptive for the Multipart Form context so the spec remains robust to
styling/markup changes.
- Around line 62-74: The verification currently queries the first
.file-value-cell in the table which may be a different row; instead capture the
target row (the same lastRow used to click the .upload-btn) and scope the
assertions to that row by using lastRow.locator('.file-value-cell') (replace the
table.allRows().locator(...).first() usage), then assert visibility and that it
contains 'test-file.txt' (retain the existing timeouts) to ensure the test
verifies the specific row that triggered the upload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b3c8a28-4bb2-4c23-a798-4c249347e2e7
📒 Files selected for processing (1)
tests/request/multipart-form/multipart-form-file-select.spec.ts
pooja-bruno
force-pushed
the
fix/multipart-form-file-select-without-key
branch
from
1c56c46 to
9f20548
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/request/multipart-form/multipart-form-file-select.spec.ts (1)
62-73: Scope the file-name assertion to the clicked row to reduce flakiness.Line 70 currently validates the first
.file-value-cellin the table, which can pass for the wrong row if other file rows exist later.Proposed refactor
test('file select should work on empty row without a key', async ({ page }) => { const table = buildCommonLocators(page).table('editable-table'); + const initialRowCount = await table.allRows().count(); + const targetRow = table.allRows().nth(initialRowCount - 1); await test.step('Click upload on empty last row (no key entered)', async () => { - const lastRow = table.allRows().last(); - const uploadBtn = lastRow.locator('.upload-btn'); + const uploadBtn = targetRow.locator('.upload-btn'); await expect(uploadBtn).toBeVisible(); await uploadBtn.click(); }); await test.step('Verify the file name appears in the row', async () => { - const fileCell = table.allRows().locator('.file-value-cell').first(); + const fileCell = targetRow.locator('.file-value-cell'); await expect(fileCell).toBeVisible({ timeout: 5000 }); await expect(fileCell).toContainText('test-file.txt', { timeout: 5000 }); }); });As per coding guidelines: "Write behaviour-driven tests, not implementation-driven ones" and "Only assert what needs to be true, not incidental details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/request/multipart-form/multipart-form-file-select.spec.ts` around lines 62 - 73, The assertion is currently checking the first `.file-value-cell` in the whole table which can validate the wrong row; change the selector to scope to the row you clicked (use the existing lastRow locator) and assert the `.file-value-cell` within lastRow (e.g., use lastRow.locator('.file-value-cell') and then expect that scoped locator toContainText 'test-file.txt') so the test only verifies the file name in the clicked row.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/request/multipart-form/multipart-form-file-select.spec.ts`:
- Around line 62-73: The assertion is currently checking the first
`.file-value-cell` in the whole table which can validate the wrong row; change
the selector to scope to the row you clicked (use the existing lastRow locator)
and assert the `.file-value-cell` within lastRow (e.g., use
lastRow.locator('.file-value-cell') and then expect that scoped locator
toContainText 'test-file.txt') so the test only verifies the file name in the
clicked row.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce834449-5300-4b07-a7dc-1e7629f661b5
📒 Files selected for processing (4)
packages/bruno-app/src/components/RequestPane/MultipartFormParams/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleMultipartFormParams/StyledWrapper.jstests/request/multipart-form/multipart-form-file-select.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/components/RequestPane/MultipartFormParams/StyledWrapper.js
- packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js
bijin-bruno
approved these changes
Apr 1, 2026
bijin-bruno
requested changes
Apr 1, 2026
Collaborator
bijin-bruno
left a comment
bijin-bruno
left a comment
There was a problem hiding this comment.
@pooja-bruno Looks like the new tests (tests/request/multipart-form/multipart-form-file-select.spec.ts) are flaky. Please see if we can fix them.
sid-bruno
approved these changes
Apr 2, 2026
chirag-bruno
pushed a commit
to chirag-bruno/bruno
that referenced
this pull request
Apr 2, 2026
chirag-bruno
pushed a commit
to chirag-bruno/bruno
that referenced
this pull request
Apr 2, 2026
bijin-bruno
pushed a commit
that referenced
this pull request
Apr 2, 2026
* fix: update system proxy fetching to use finally (#7652) * fix: update system proxy fetching to use finally for improved reliability * Update packages/bruno-electron/src/index.js Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix: allow file selection in multipart form without entering a key first (#7640) * fix: close previous SSE connection before sending new request When resending an SSE (Server-Sent Events) request using Cmd+Enter, the previous connection was not being closed, causing connection leaks. Changes: - Add SSE cancellation logic to sendRequest action - checks for running stream and cancels it before sending new request - Add return to cancelRequest action to make it properly chainable - Simplify RequestTabPanel by removing duplicate cancel logic (now handled centrally in sendRequest) - Add SSE endpoints to test server for e2e testing - Add Playwright e2e test to verify SSE connection cancellation * fix: address PR review feedback for SSE connection cancellation - Use platform-aware modifier (Meta on macOS, Control on Linux/Windows) instead of hardcoded Meta+Enter for cross-platform CI compatibility - Replace waitForTimeout with expect.poll for deterministic assertions - Remove dead try/catch around cancelRequest (errors already swallowed by cancelRequest's internal .catch) * fix: updated the test to check of connectionIds --------- Co-authored-by: Sid <siddharth@usebruno.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Pooja <pooja@usebruno.com> Co-authored-by: Chirag Chandrashekhar <cchirag85@gmail.com>
This was referenced Apr 7, 2026
5 tasks
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
3 participants
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
Contribution Checklist:
Screen.Recording.2026-04-01.at.1.16.06.PM.mov
Summary by CodeRabbit
Bug Fixes
Style
Tests