fix(file-mode): crash issue while searching in file mode#8532
Conversation
|
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:
WalkthroughUpdates CodeEditor search handling to focus the search bar through a ref, routes file-mode and JS saves through ChangesFile-mode save and editor search
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/bruno-app/src/components/FileEditor/CodeEditor/index.js (1)
184-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRef callback returns a value — use a statement body for React 19 compatibility.
The expression-body arrow
(node) => (this.searchBarRef.current = node)returnsnode. React 19 discourages returning values from ref callbacks (returning a function is treated as a cleanup function). The other ref callback in this file (line 190) already uses curly braces correctly.♻️ Proposed fix
- ref={(node) => (this.searchBarRef.current = node)} + ref={(node) => { + this.searchBarRef.current = node; + }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/FileEditor/CodeEditor/index.js` at line 184, The ref callback assigned to searchBarRef in CodeEditor should not use an expression body that returns the node value, because React 19 treats ref callback return values as cleanup functions. Update the ref callback in the CodeEditor component to use a block body with an assignment statement, matching the safer pattern already used by the other ref callback in this file.tests/transient-requests/transient-file-mode-save.spec.ts (1)
11-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCentralize selectors and move assertions out of helpers per path instructions.
The spec defines helpers (
switchToFileMode,setUrlInFileModeRaw,saveTransientViaModal,expectFileModeRawContainsUrl,searchInFileMode,closeFileModeSearch) with inline raw CSS selectors like.file-mode .CodeMirror,.bruno-modal-card,#request-name,.bruno-search-bar, and.cm-search-current. Several helpers also containexpect()assertions. Per path instructions, selectors should be centralized in page modules undertests/utils/page/*and assertions should live in the spec, not in helpers.As per path instructions, "Centralize locators and actions in page modules under
tests/utils/page/*; never inline raw selectors in a spec" and "Assertions must live in the spec (expect(...)), while page-module helpers should only synchronize."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/transient-requests/transient-file-mode-save.spec.ts` around lines 11 - 79, This spec mixes raw selectors and assertions into local helpers, which should instead be centralized and kept action-only. Move the locator logic from switchToFileMode, setUrlInFileModeRaw, saveTransientViaModal, expectFileModeRawContainsUrl, searchInFileMode, and closeFileModeSearch into page modules under tests/utils/page/* using shared locators for file mode, the save modal, and search bar. Remove expect() calls from those helpers so they only perform synchronization/actions, and keep all assertions in the spec body after calling the page-module helpers.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/bruno-app/src/components/FileEditor/CodeEditor/index.js`:
- Line 184: The ref callback assigned to searchBarRef in CodeEditor should not
use an expression body that returns the node value, because React 19 treats ref
callback return values as cleanup functions. Update the ref callback in the
CodeEditor component to use a block body with an assignment statement, matching
the safer pattern already used by the other ref callback in this file.
In `@tests/transient-requests/transient-file-mode-save.spec.ts`:
- Around line 11-79: This spec mixes raw selectors and assertions into local
helpers, which should instead be centralized and kept action-only. Move the
locator logic from switchToFileMode, setUrlInFileModeRaw, saveTransientViaModal,
expectFileModeRawContainsUrl, searchInFileMode, and closeFileModeSearch into
page modules under tests/utils/page/* using shared locators for file mode, the
save modal, and search bar. Remove expect() calls from those helpers so they
only perform synchronization/actions, and keep all assertions in the spec body
after calling the page-module helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 182246ad-d025-4538-9cbe-d53c1e234769
📒 Files selected for processing (4)
packages/bruno-app/src/components/FileEditor/CodeEditor/index.jspackages/bruno-app/src/components/RequestTabs/RequestTab/index.jspackages/bruno-app/src/components/SaveTransientRequest/index.jstests/transient-requests/transient-file-mode-save.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/utils/page/file-mode.ts (1)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove assertions out of action helpers
switchToFileMode,saveTransientViaModal,searchInFileMode, andcloseFileModeSearchcontainexpect()assertions. Per the Playwright testing guide, actions should only synchronize/wait — assertions belong in the spec. Replaceexpect().toBeVisible()withwaitFor()for synchronization, and move behavioral assertions (search highlights, result counts) to the spec.As per path instructions, the referenced documentation states: "Add assertions in the spec (expect) while actions should only synchronize/wait (no assertions inside actions)."
♻️ Proposed refactor for file-mode.ts action helpers
export const switchToFileMode = async (page: Page) => { await page.getByTestId('view-mode-file').click(); - await expect(fileModeEditor(page)).toBeVisible(); + await fileModeEditor(page).waitFor(); };export const saveTransientViaModal = async (page: Page, requestName: string) => { const saveModal = page.locator('.bruno-modal-card').filter({ hasText: 'Save Request' }); - await expect(saveModal).toBeVisible({ timeout: 5000 }); + await saveModal.waitFor({ timeout: 5000 }); const requestNameInput = saveModal.locator('`#request-name`'); await requestNameInput.clear(); await requestNameInput.fill(requestName); await saveModal.getByRole('button', { name: 'Save' }).click(); };+export const searchHighlight = (page: Page) => + fileModeEditor(page).locator('.cm-search-current').first(); +export const searchResultCount = (page: Page) => + page.locator('.bruno-search-bar .searchbar-result-count'); + export const searchInFileMode = async (page: Page, term: string) => { await fileModeEditor(page).click(); await page.keyboard.press(findShortcut); const searchBar = page.locator('.bruno-search-bar'); - await expect(searchBar).toBeVisible(); + await searchBar.waitFor(); await searchBar.locator('input').fill(term); - await expect(fileModeEditor(page).locator('.cm-search-current').first()).toBeVisible(); - await expect(searchBar.locator('.searchbar-result-count')).not.toHaveText('0 results'); };export const closeFileModeSearch = async (page: Page) => { await page.locator('.bruno-search-bar input').press('Escape'); - await expect(page.locator('.bruno-search-bar')).toBeHidden(); + await page.locator('.bruno-search-bar').waitFor({ state: 'hidden' }); };Then in the spec, add the behavioral assertions after calling
searchInFileMode:await test.step('Search "meta" -> matches are highlighted', async () => { await searchInFileMode(page, 'meta'); await expect(searchHighlight(page)).toBeVisible(); await expect(searchResultCount(page)).not.toHaveText('0 results'); });Remember to re-export
searchHighlightandsearchResultCountfromtests/utils/page/index.ts.Also applies to: 22-29, 61-69, 71-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/page/file-mode.ts` around lines 8 - 11, `switchToFileMode`, `saveTransientViaModal`, `searchInFileMode`, and `closeFileModeSearch` currently mix actions with `expect()` assertions; move those behavioral checks into the spec and keep helpers limited to synchronization. Replace the visibility assertion in `switchToFileMode` with a `waitFor()`-based wait on `fileModeEditor(page)`, and apply the same pattern to the other helpers where they assert UI state. Update the relevant spec to assert search highlights and result counts after calling `searchInFileMode`, and re-export `searchHighlight` and `searchResultCount` from the page index so the spec can use them.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/utils/page/file-mode.ts`:
- Around line 8-11: `switchToFileMode`, `saveTransientViaModal`,
`searchInFileMode`, and `closeFileModeSearch` currently mix actions with
`expect()` assertions; move those behavioral checks into the spec and keep
helpers limited to synchronization. Replace the visibility assertion in
`switchToFileMode` with a `waitFor()`-based wait on `fileModeEditor(page)`, and
apply the same pattern to the other helpers where they assert UI state. Update
the relevant spec to assert search highlights and result counts after calling
`searchInFileMode`, and re-export `searchHighlight` and `searchResultCount` from
the page index so the spec can use them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 590b4af8-4374-45f2-baa2-416f463312f9
📒 Files selected for processing (4)
packages/bruno-app/src/components/FileEditor/CodeEditor/index.jstests/transient-requests/transient-file-mode-save.spec.tstests/utils/page/file-mode.tstests/utils/page/index.ts
✅ Files skipped from review due to trivial changes (1)
- tests/utils/page/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/components/FileEditor/CodeEditor/index.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/utils/page/file-mode.ts (1)
6-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer semantic selectors over raw CSS class matching where feasible.
editor,editorContent, andcurrentSearchMatchuse raw CSS selectors (.file-mode .CodeMirror,.cm-search-current) rather thangetByTestId/getByRole. Consider adding adata-testidon the file-mode editor wrapper for consistency with the other locators in this file.As per path instructions: "Use semantic selectors in page modules (
getByTestId,getByRole,getByLabel) rather than raw CSS selectors."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/page/file-mode.ts` around lines 6 - 9, The file-mode locator helpers are using raw CSS selectors instead of the preferred semantic test selectors. Update buildFileModeLocators to use getByTestId/getByRole/getByLabel where possible, and add a data-testid on the file-mode editor wrapper if needed so editor, editorContent, and currentSearchMatch can be targeted consistently without class-based selectors.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/utils/page/file-mode.ts`:
- Around line 6-9: The file-mode locator helpers are using raw CSS selectors
instead of the preferred semantic test selectors. Update buildFileModeLocators
to use getByTestId/getByRole/getByLabel where possible, and add a data-testid on
the file-mode editor wrapper if needed so editor, editorContent, and
currentSearchMatch can be targeted consistently without class-based selectors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8c070a2-2870-42c9-853b-918047b4aa5d
📒 Files selected for processing (6)
packages/bruno-app/src/components/CodeMirrorSearch/index.jspackages/bruno-app/src/components/RequestTabs/RequestTab/index.jspackages/bruno-app/src/components/SaveTransientRequest/index.jstests/transient-requests/transient-file-mode-save.spec.tstests/utils/page/file-mode.tstests/utils/page/locators.ts
✅ Files skipped from review due to trivial changes (1)
- packages/bruno-app/src/components/CodeMirrorSearch/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/components/RequestTabs/RequestTab/index.js
- tests/transient-requests/transient-file-mode-save.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/utils/page/file-mode.ts (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the existing Code Editor label for the top-level locator
editor()can target the wrapper’saria-label="Code Editor"instead of.file-mode .CodeMirror.editorContent()andcurrentSearchMatch()still rely on CodeMirror internals, so add a semantic hook there if these assertions need to stay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/page/file-mode.ts` around lines 7 - 9, The top-level editor locator in the file-mode page object should use the existing “Code Editor” aria label instead of the CodeMirror class selector. Update the `editor()` helper in `file-mode.ts` to target the accessible wrapper, and keep `editorContent()` and `currentSearchMatch()` on CodeMirror internals only if needed by adding a stable semantic hook for those assertions.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/utils/page/file-mode.ts`:
- Around line 7-9: The top-level editor locator in the file-mode page object
should use the existing “Code Editor” aria label instead of the CodeMirror class
selector. Update the `editor()` helper in `file-mode.ts` to target the
accessible wrapper, and keep `editorContent()` and `currentSearchMatch()` on
CodeMirror internals only if needed by adding a stable semantic hook for those
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16b18fb4-89e0-4d6c-ba3c-74ab6ea4763c
📒 Files selected for processing (2)
packages/bruno-app/src/components/RequestTabs/RequestTab/index.jstests/utils/page/file-mode.ts
💤 Files with no reviewable changes (1)
- packages/bruno-app/src/components/RequestTabs/RequestTab/index.js
* fix(file-mode): crash issue while searching in file mode * fix: moved test utils & updated locators * fix: resolved comments * fix: resolved comments * fix: resolve comments & update locators --------- Co-authored-by: shubh-bruno <shubh-bruno@shubh-bruno.local>
3 participants
Description
JIRA - (Unable to Save a
.bruFile in File Mode for a Transient Request)JIRA - (
Ctrl/Cmd+Fin code editor crashes app)Fixes: #8437
Problem
When you create a transient request, switch to File Mode, edit the raw content, and save, the edits were silently dropped and the request was saved from stale data. Related close/keybinding paths were inconsistent, and the file-mode editor had a search crash.
Root cause
File-mode edits are stored only in item.draft.raw, but the transient save path serialized the structured request (transformRequestToSaveToFilesystem), which never reads raw.
Solution
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
Cmd/Ctrl+Fnow reliably opens the search UI and immediately focuses the search input.data-testidattributes for the search UI.