fix: use path utilities for file references stored to version-controlled config#7895
Open
prateek-bruno wants to merge 10 commits intousebruno:mainfrom
Open
fix: use path utilities for file references stored to version-controlled config#7895prateek-bruno wants to merge 10 commits intousebruno:mainfrom
prateek-bruno wants to merge 10 commits intousebruno:mainfrom
Conversation
… file Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
prateek-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-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:
WalkthroughReplaces ad-hoc relative-path logic with a new exported helper Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Renderer (Component)
participant Dialog as Filesystem / Electron Dialog
participant Utils as getStoredFilePath
participant Store as Persistence (YAML)
User->>UI: Click "Browse" for file
UI->>Dialog: showOpenDialog()
Dialog-->>UI: absolute filePath
UI->>Utils: getStoredFilePath(collectionPath, filePath)
Utils-->>UI: storedPath (relative or original)
UI->>Store: save request YAML with storedPath
Store-->>UI: saved
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/collection/opencollection/multipart-file-path.spec.ts`:
- Around line 66-70: In expectRequestFileToContainRelativePayload, the negative
assertion only checks payloadPath using the native separator and can miss
absolute paths stored with the other separator; update the test to assert the
file does not contain payloadPath in either separator form by importing node's
path and checking both payloadPath and an alternate-separator variant (e.g.,
payloadPath.replace(path.sep, path.sep === '/' ? '\\' : '/')) or by normalizing
both file content and payloadPath to a consistent separator before asserting;
reference the function expectRequestFileToContainRelativePayload and the
variables requestFilePath, payloadPath, and relativePayloadPath when making the
change.
In `@tests/utils/page/actions.ts`:
- Around line 880-886: In addMultipartFileToLastRow, the assertion checks the
first row's file cell instead of the row we interacted with; update the test to
assert against the targeted lastRow by replacing
table.allRows().locator('.file-value-cell').first() with
lastRow.locator('.file-value-cell') (and keep the expect
toContainText(path.basename(filePath))) so the helper validates the uploaded
file in the correct row.
- Around line 867-874: The mockBrowseFiles helper currently overwrites
dialog.showOpenDialog permanently; change it so it saves the original
dialog.showOpenDialog inside the electron evaluate context, replaces it with the
stub that returns the provided filePaths, then restores the original handler
before the evaluate callback finishes (or after a single invocation). In other
words, inside mockBrowseFiles' call to electronApp.evaluate, capture const
original = dialog.showOpenDialog, set dialog.showOpenDialog = async () => ({
canceled: false, filePaths: selectedPaths }), and then reassign
dialog.showOpenDialog = original (or implement a one-time stub) so
mockBrowseFiles leaves dialog.showOpenDialog unchanged for subsequent tests.
🪄 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: 855c9613-bbd8-4e40-90ed-81aea013493c
📒 Files selected for processing (10)
packages/bruno-app/src/components/CollectionSettings/ClientCertSettings/index.jspackages/bruno-app/src/components/FilePickerEditor/index.jspackages/bruno-app/src/components/RequestPane/Auth/OAuth1/index.jspackages/bruno-app/src/components/RequestPane/MultipartFormParams/index.jspackages/bruno-app/src/components/ResponseExample/ResponseExampleRequestPane/ResponseExampleMultipartFormParams/index.jspackages/bruno-app/src/utils/common/path.jspackages/bruno-app/src/utils/common/path.spec.jspackages/bruno-app/src/utils/common/path.windows.spec.jstests/collection/opencollection/multipart-file-path.spec.tstests/utils/page/actions.ts
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/collection/opencollection/multipart-file-path.spec.ts (1)
131-133: ⚡ Quick winAssert the remove step before re-adding the multipart file.
Right now the test can still pass even if removal silently fails, because only the post re-add state is asserted. Add a direct assertion after save on remove.
Suggested fix
await removeFirstMultipartFile(page); await saveRequest(page); + await expect.poll(async () => fs.promises.readFile(requestFilePath, 'utf-8')).not.toContain(relativePayloadPath); await addMultipartFileToLastRow(page, electronApp, payloadPath);As per coding guidelines: “Cover both the ‘happy path’ and the realistically problematic paths… tests should fail usefully.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/collection/opencollection/multipart-file-path.spec.ts` around lines 131 - 133, The test currently calls removeFirstMultipartFile(page) and then saveRequest(page) but never asserts that the removal succeeded; add an immediate assertion after saveRequest (or directly after removeFirstMultipartFile if save persists state) that verifies the multipart file was actually removed—e.g., assert the absence of the removed file element or that the multipart file list length decreased—by using the same page selectors/helpers used elsewhere in the spec so the failure is detected before re-adding the file; reference removeFirstMultipartFile and saveRequest to locate where to add this assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/collection/opencollection/multipart-file-path.spec.ts`:
- Line 15: Replace the hardcoded separator in the relativePayloadPath constant
with an OS-native join: import or require the path module and set
relativePayloadPath using path.join('files', 'payload.json') so tests run on
Windows and POSIX; update any dependent assertions that expect a string to use
the new constant.
---
Nitpick comments:
In `@tests/collection/opencollection/multipart-file-path.spec.ts`:
- Around line 131-133: The test currently calls removeFirstMultipartFile(page)
and then saveRequest(page) but never asserts that the removal succeeded; add an
immediate assertion after saveRequest (or directly after
removeFirstMultipartFile if save persists state) that verifies the multipart
file was actually removed—e.g., assert the absence of the removed file element
or that the multipart file list length decreased—by using the same page
selectors/helpers used elsewhere in the spec so the failure is detected before
re-adding the file; reference removeFirstMultipartFile and saveRequest to locate
where to add this assertion.
🪄 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: 2099949c-9bb1-422c-b37e-43bbe65c55a3
📒 Files selected for processing (1)
tests/collection/opencollection/multipart-file-path.spec.ts
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/collection/opencollection/multipart-file-path.spec.ts`:
- Around line 124-139: The relaunched Electron instance started via
launchElectronApp is never closed, leaking processes; after the final assertion
(expectRequestFileToContainRelativePayload) ensure you close the electronApp by
awaiting its close() (i.e., await electronApp.close()), or if you kept the
original instance in a separate variable, await close() on both—place the await
electronApp.close() call after expectRequestFileToContainRelativePayload to
guarantee the second app is shut down.
- Around line 131-135: Add an explicit assertion after calling
removeFirstMultipartFile(page) to verify the file was actually removed before
proceeding to re-add; update the test to check the UI/state (via existing
helpers or new assertion) that confirms the multipart item is absent (e.g.,
verify row count or that the removed filename is not present) after
removeFirstMultipartFile(page) and before addMultipartFileToLastRow(page,
electronApp, payloadPath), keeping the subsequent saveRequest(page) calls
unchanged and referencing removeFirstMultipartFile, addMultipartFileToLastRow
and saveRequest to locate where to insert the check.
🪄 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: c4f21b94-4792-4126-8ac1-7ac538059c8a
📒 Files selected for processing (1)
tests/collection/opencollection/multipart-file-path.spec.ts
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
prateek-bruno
force-pushed
the
bugfix/path-normalization
branch
from
b1a37ba to
4506b42
Compare
Co-authored-by: Prateek Sunal <prtksunal@gmail.com>
prateek-bruno
changed the title
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
Fixes #7858
Replace ad-hoc
path.relative()andstartsWith-based containment in file picker flows with centralized utilities that handle cross-platform paths.getRelativePathWithinBasePathwithpath.relative-based containmentguard and optional posixify support
getRelativePath(posixified, safe forgit-collaborated
.bru/bruno.json)→
getRelativePathWithinBasePath(backwards-compat default) replacingfragile
startsWithcontainment (false-positives on sibling dirs,case-sensitive on Windows)
Contribution Checklist:
Summary by CodeRabbit
Refactor
Tests