fix: tab type resolution for non request types#8097
Merged
bijin-bruno merged 3 commits intoMay 26, 2026
Conversation
sid-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
WalkthroughThis PR extends snapshot deserialization to special-case ChangesFolder Settings Tab Persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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: 1
🧹 Nitpick comments (5)
packages/bruno-app/src/utils/snapshot/index.spec.js (1)
289-312: ⚡ Quick winAdd a regression test for non-request type preservation when pathname resolves to an item.
These tests cover request-pane defaults, but not the new Line 565 guard that preserves
folder-settingstype when the resolved item is non-request.Suggested test addition
+ it('keeps folder-settings type when pathname resolves to a non-request item', () => { + const collectionWithFolderItem = { + ...collection, + items: [ + { + uid: 'folder-1', + pathname: '/collections/a/folder', + type: 'folder' + } + ] + }; + + const snapshotTab = { + type: 'folder-settings', + accessor: 'pathname', + pathname: '/collections/a/folder', + permanent: true + }; + + const tab = deserializeTab(snapshotTab, collectionWithFolderItem); + expect(tab.type).toBe('folder-settings'); + expect(tab.folderUid).toBe('folder-1'); + expect(tab.requestPaneTab).toBe('headers'); + });🤖 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/utils/snapshot/index.spec.js` around lines 289 - 312, Add a regression test to ensure deserializeTab preserves a tab with type 'folder-settings' when the provided pathname resolves to a non-request item: construct a snapshotTab with type 'folder-settings', accessor 'pathname', and a pathname that matches a non-request item in the test collection, call deserializeTab(snapshotTab, collection), and assert that the returned tab.type is still 'folder-settings' (and that requestPaneTab is preserved or defaults as appropriate). Target the deserializeTab behavior around the new guard (the logic introduced near Line 565) so the test fails if the resolver incorrectly converts folder-settings into a request tab.tests/utils/page/actions.ts (2)
807-807: ⚡ Quick winRename to camelCase for consistency.
The function name
selectfolderPaneTabshould beselectFolderPaneTabto match the naming convention used byselectRequestPaneTabandselectResponsePaneTab.♻️ Suggested rename
-const selectfolderPaneTab = async (page: Page, tabName: string) => { +const selectFolderPaneTab = async (page: Page, tabName: string) => {Don't forget to update the export and the test file import/usage.
🤖 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/actions.ts` at line 807, Rename the function selectfolderPaneTab to camelCase selectFolderPaneTab everywhere: update the function declaration in tests/utils/page/actions.ts, adjust its export, and update all imports/usages (e.g., where selectfolderPaneTab is imported or called in tests) to use selectFolderPaneTab so it matches selectRequestPaneTab/selectResponsePaneTab naming. Ensure any references in test suites or index/export files are updated to the new identifier to avoid unresolved symbol errors.
787-787: ⚡ Quick winRename to camelCase for consistency.
The function name
openfoldershould beopenFolderto match the naming convention used by other helpers in this file (openRequest,openFolderRequest,createFolder, etc.).♻️ Suggested rename
-const openfolder = async (page: Page, collectionName: string, folderName: string, { persist = false } = {}) => { +const openFolder = async (page: Page, collectionName: string, folderName: string, { persist = false } = {}) => {Don't forget to update the export and the test file import/usage.
🤖 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/actions.ts` at line 787, Rename the helper function identifier openfolder to camelCase openFolder wherever it is declared and referenced (including its export and all imports/usages in tests) so it matches the existing helpers like openRequest/openFolderRequest; update the function declaration (openfolder), any export named openfolder, and all call sites in tests and other modules to openFolder to avoid breaking imports.tests/snapshots/folder.spec.ts (2)
49-49: ⚡ Quick winReplace magic timeout with event-driven wait.
Per coding guidelines, avoid
page.waitForTimeout()unless absolutely necessary. If this timeout is needed for snapshot persistence, consider usingexpect.poll()or another event-driven approach instead.As per coding guidelines: "Try to reduce usage of
page.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright calls. Replace magic timeouts with event-driven waits in E2E tests."🤖 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/snapshots/folder.spec.ts` at line 49, Replace the magic sleep call await page.waitForTimeout(1000); with an event-driven wait: identify the element or event you actually need to be stable (e.g., a snapshot container locator or network response) and replace the call to page.waitForTimeout with a targeted waiter such as await expect(page.locator('SELECTOR_FOR_SNAPSHOT')).toBeVisible(), await page.waitForSelector('SELECTOR_FOR_SNAPSHOT'), await expect.poll(() => /* check snapshot saved state */).toEqual(true), or await page.waitForResponse(resp => resp.url().includes('snapshot') && resp.status() === 200); update the selector or response predicate to match the snapshot persistence logic in tests/snapshots/folder.spec.ts and remove the page.waitForTimeout usage.
88-88: ⚡ Quick winReplace magic timeout with event-driven wait.
Same issue as in the first test: avoid
page.waitForTimeout()unless absolutely necessary. Consider usingexpect.poll()or another event-driven approach to verify snapshot persistence.As per coding guidelines: "Try to reduce usage of
page.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright calls. Replace magic timeouts with event-driven waits in E2E tests."🤖 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/snapshots/folder.spec.ts` at line 88, Replace the magic sleep call await page.waitForTimeout(2000); in tests/snapshots/folder.spec.ts with an event-driven wait: use Playwright's expect/expect.poll or a specific wait method (e.g., await expect(locator).toHaveText(...), await expect(locator).toBeVisible(), await page.waitForResponse(...), or await expect.poll(() => /* check snapshot persisted */).toBeTruthy()) that asserts the snapshot persisted instead of sleeping; locate the test that contains await page.waitForTimeout and update it to wait on the appropriate locator or response that demonstrates snapshot persistence.
🤖 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.
Inline comments:
In `@tests/snapshots/folder.spec.ts`:
- Line 96: Remove the debug console.log call in the test file: delete the
console.log({ tab }); statement found in tests/snapshots/folder.spec.ts (inside
the snapshot test where the variable tab is logged) so tests don't emit spurious
debug output before merging.
---
Nitpick comments:
In `@packages/bruno-app/src/utils/snapshot/index.spec.js`:
- Around line 289-312: Add a regression test to ensure deserializeTab preserves
a tab with type 'folder-settings' when the provided pathname resolves to a
non-request item: construct a snapshotTab with type 'folder-settings', accessor
'pathname', and a pathname that matches a non-request item in the test
collection, call deserializeTab(snapshotTab, collection), and assert that the
returned tab.type is still 'folder-settings' (and that requestPaneTab is
preserved or defaults as appropriate). Target the deserializeTab behavior around
the new guard (the logic introduced near Line 565) so the test fails if the
resolver incorrectly converts folder-settings into a request tab.
In `@tests/snapshots/folder.spec.ts`:
- Line 49: Replace the magic sleep call await page.waitForTimeout(1000); with an
event-driven wait: identify the element or event you actually need to be stable
(e.g., a snapshot container locator or network response) and replace the call to
page.waitForTimeout with a targeted waiter such as await
expect(page.locator('SELECTOR_FOR_SNAPSHOT')).toBeVisible(), await
page.waitForSelector('SELECTOR_FOR_SNAPSHOT'), await expect.poll(() => /* check
snapshot saved state */).toEqual(true), or await page.waitForResponse(resp =>
resp.url().includes('snapshot') && resp.status() === 200); update the selector
or response predicate to match the snapshot persistence logic in
tests/snapshots/folder.spec.ts and remove the page.waitForTimeout usage.
- Line 88: Replace the magic sleep call await page.waitForTimeout(2000); in
tests/snapshots/folder.spec.ts with an event-driven wait: use Playwright's
expect/expect.poll or a specific wait method (e.g., await
expect(locator).toHaveText(...), await expect(locator).toBeVisible(), await
page.waitForResponse(...), or await expect.poll(() => /* check snapshot
persisted */).toBeTruthy()) that asserts the snapshot persisted instead of
sleeping; locate the test that contains await page.waitForTimeout and update it
to wait on the appropriate locator or response that demonstrates snapshot
persistence.
In `@tests/utils/page/actions.ts`:
- Line 807: Rename the function selectfolderPaneTab to camelCase
selectFolderPaneTab everywhere: update the function declaration in
tests/utils/page/actions.ts, adjust its export, and update all imports/usages
(e.g., where selectfolderPaneTab is imported or called in tests) to use
selectFolderPaneTab so it matches selectRequestPaneTab/selectResponsePaneTab
naming. Ensure any references in test suites or index/export files are updated
to the new identifier to avoid unresolved symbol errors.
- Line 787: Rename the helper function identifier openfolder to camelCase
openFolder wherever it is declared and referenced (including its export and all
imports/usages in tests) so it matches the existing helpers like
openRequest/openFolderRequest; update the function declaration (openfolder), any
export named openfolder, and all call sites in tests and other modules to
openFolder to avoid breaking imports.
🪄 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: e7431aa0-593e-4a8e-a4ae-8cb3d2c303d5
📒 Files selected for processing (5)
packages/bruno-app/src/utils/snapshot/index.jspackages/bruno-app/src/utils/snapshot/index.spec.jstests/snapshots/folder.spec.tstests/utils/page/actions.tstests/utils/page/locators.ts
Removed console log statement from folder.spec.ts
bijin-bruno
approved these changes
May 26, 2026
lohit-bruno
pushed a commit
that referenced
this pull request
May 26, 2026
* fix: tab type resolution for non request types * Remove console log from snapshot test Removed console log statement from folder.spec.ts * test(snapshot): deserializeTab test addition for the removed guard
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
2 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
folder-settingstab type isn't deserialized properly because the request type takes over.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
Bug Fixes
Tests