fix: Handle deleted environment variables in UI#6703
fix: Handle deleted environment variables in UI#6703bijin-bruno merged 7 commits intousebruno:mainfrom
Conversation
WalkthroughHandle environment variables deleted via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js:
- Around line 389-402: The filter logic incorrectly removes variables that were
modified by scripts because it checks variable.ephemeral before restoring
variable.persistedValue; update the callback that processes env variables so it
checks if variable.persistedValue !== undefined first and restores
variable.value, clears variable.ephemeral and deletes variable.persistedValue,
then proceed to the ephemeral check (variable.ephemeral) to decide removal;
ensure the references to variable.persistedValue and variable.ephemeral in that
callback are handled in that order so persisted disk values are restored rather
than dropped.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.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. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
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
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
🧬 Code graph analysis (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (1)
packages/bruno-app/src/utils/collections/index.js (1)
envVariables(1171-1171)
⏰ 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: CLI Tests
- GitHub Check: Unit Tests
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.ts (1)
4-33: Usetest.step()to structure test flow for better report readability.Break the test into logical steps using
test.step()to make the generated test reports clearer and easier to debug. This also helps document the test flow.♻️ Proposed refactor using test.step()
test('should remove ephemeral variable from UI after deletion', async ({ pageWithUserData: page }) => { - // Select the collection and request - await page.locator('#sidebar-collection-name').click(); - await page.getByText('api-deleteEnvVar', { exact: true }).click(); - - // open environment dropdown - await page.getByTestId('environment-selector-trigger').click(); - - // select stage environment - await expect(page.locator('.environment-list .dropdown-item', { hasText: 'Stage' })).toBeVisible(); - await page.locator('.environment-list .dropdown-item', { hasText: 'Stage' }).click(); - await expect(page.locator('.current-environment', { hasText: 'Stage' })).toBeVisible(); - - // Send the request (sets and then deletes the variable) - await sendRequest(page, 200); - - // Verify variable is removed from UI - await page.getByTestId('environment-selector-trigger').click(); - await page.locator('#configure-env').click(); - - const envTab = page.locator('.request-tab').filter({ hasText: 'Environments' }); - await expect(envTab).toBeVisible(); - - // Variable should not be visible in the UI - await expect(page.locator('.table-container tbody')).not.toContainText('tempToken'); - - await envTab.hover(); - await envTab.getByTestId('request-tab-close-icon').click(); + await test.step('Select api-deleteEnvVar request', async () => { + await page.locator('#sidebar-collection-name').click(); + await page.getByText('api-deleteEnvVar', { exact: true }).click(); + }); + + await test.step('Select Stage environment', async () => { + await page.getByTestId('environment-selector-trigger').click(); + const stageOption = page.locator('.environment-list .dropdown-item', { hasText: 'Stage' }); + await expect(stageOption).toBeVisible(); + await stageOption.click(); + await expect(page.locator('.current-environment', { hasText: 'Stage' })).toBeVisible(); + }); + + await test.step('Send request and verify variable deletion', async () => { + await sendRequest(page, 200); + }); + + await test.step('Verify variable is removed from UI', async () => { + await page.getByTestId('environment-selector-trigger').click(); + await page.locator('#configure-env').click(); + + const envTab = page.locator('.request-tab').filter({ hasText: 'Environments' }); + await expect(envTab).toBeVisible(); + await expect(page.locator('.table-container tbody')).not.toContainText('tempToken'); + + await envTab.hover(); + await envTab.getByTestId('request-tab-close-icon').click(); + }); });As per coding guidelines, promote the use of
test.stepto make generated reports easier to read.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.tstests/environments/api-deleteEnvVar/fixtures/collection/api-deleteEnvVar.brutests/environments/api-deleteEnvVar/fixtures/collection/bruno.jsontests/environments/api-deleteEnvVar/fixtures/collection/environments/Stage.brutests/environments/api-deleteEnvVar/init-user-data/collection-security.jsontests/environments/api-deleteEnvVar/init-user-data/preferences.json
🔥 Files not summarized due to errors (1)
- tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.ts: Error: Server error: no LLM provider could handle the message
✅ Files skipped from review due to trivial changes (2)
- tests/environments/api-deleteEnvVar/fixtures/collection/environments/Stage.bru
- tests/environments/api-deleteEnvVar/fixtures/collection/bruno.json
🧰 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. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
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
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.ts
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/environments/api-deleteEnvVar/api-deleteEnvVar.spec.tstests/environments/api-deleteEnvVar/init-user-data/preferences.jsontests/environments/api-deleteEnvVar/init-user-data/collection-security.jsontests/environments/api-deleteEnvVar/fixtures/collection/api-deleteEnvVar.bru
🧠 Learnings (1)
📚 Learning: 2025-12-16T07:16:23.647Z
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:23.647Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.
Applied to files:
tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.tstests/environments/api-deleteEnvVar/init-user-data/collection-security.jsontests/environments/api-deleteEnvVar/fixtures/collection/api-deleteEnvVar.bru
⏰ 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: CLI Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - macOS
🔇 Additional comments (4)
tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.ts (1)
5-5: The fixture collection is already correctly placed intests/environments/api-deleteEnvVar/fixtures/collection/and follows the expected directory structure. No changes needed.Likely an incorrect or invalid review comment.
tests/environments/api-deleteEnvVar/init-user-data/preferences.json (1)
1-6: LGTM!The preferences configuration correctly references the test fixture collection path and sets appropriate initialization settings.
tests/environments/api-deleteEnvVar/init-user-data/collection-security.json (1)
1-10: LGTM!The security configuration correctly specifies the collection path and sets the JavaScript sandbox mode to "safe", which is appropriate for testing the
bru.deleteEnvVar()functionality in a controlled environment.tests/environments/api-deleteEnvVar/fixtures/collection/api-deleteEnvVar.bru (1)
1-20: Extract locators to variables and usetest.stepfor better test structure.The fixture structure is correct and properly placed in
tests/**/fixtures/collection. However, the test implementation has two Playwright best practice violations:
Locators should be stored in variables (lines 7-32): Multiple inline locators like
page.locator('#sidebar-collection-name'),page.getByTestId('environment-selector-trigger'), andpage.locator('.environment-list .dropdown-item', { hasText: 'Stage' })should be extracted to locator variables at the beginning of the test.Use
test.stepfor readability: Wrap logical test sections (e.g., "Select collection and request", "Open environment dropdown", "Send request", "Verify deletion") intest.stepcalls for better report clarity.⛔ Skipped due to learnings
Learnt from: sanish-bruno Repo: usebruno/bruno PR: 6090 File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8 Timestamp: 2025-12-16T07:16:23.647Z Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.Learnt from: CR Repo: usebruno/bruno PR: 0 File: CODING_STANDARDS.md:0-0 Timestamp: 2025-12-05T20:31:33.005Z Learning: Use consistent patterns and helper utilities where they improve clarity. Prefer shared test utilities over copy-pasted setup code, but only when it actually reduces complexityLearnt from: CR Repo: usebruno/bruno PR: 0 File: CODING_STANDARDS.md:0-0 Timestamp: 2025-12-05T20:31:33.005Z Learning: Keep tests readable and maintainable. Optimise for clarity over cleverness. Name tests descriptively, keep setup minimal, and avoid unnecessary abstraction
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.ts (1)
18-19: Extract repeated table locator to a variable.The
.table-container tbodylocator is used twice. Extracting it to a variable improves readability and maintainability.♻️ Extract locator to variable
const envTab = page.locator('.request-tab').filter({ hasText: 'Environments' }); await expect(envTab).toBeVisible(); - await expect(page.locator('.table-container tbody')).toContainText('host'); - await expect(page.locator('.table-container tbody')).not.toContainText('tempToken'); + const environmentTable = page.locator('.table-container tbody'); + await expect(environmentTable).toContainText('host'); + await expect(environmentTable).not.toContainText('tempToken');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.ts
🧰 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. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
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
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.ts
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/environments/api-deleteEnvVar/api-deleteEnvVar.spec.ts
🧠 Learnings (2)
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created
Applied to files:
tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.ts
📚 Learning: 2025-12-16T07:16:23.647Z
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:23.647Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.
Applied to files:
tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.ts
🧬 Code graph analysis (1)
tests/environments/api-deleteEnvVar/api-deleteEnvVar.spec.ts (1)
tests/utils/page/actions.ts (4)
openRequest(869-869)openEnvironmentSelector(861-861)closeEnvironmentPanel(866-866)closeAllCollections(852-852)
⏰ 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: Unit Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - macOS
BRU-2424
Fixes: #5005
Issue:
After calling
bru.deleteEnvVar(), deleted variables remained in the UI. The reducer only processed variables present inenvVariables, so deletions weren't handled.Fix:
Added logic to handle variables missing from
envVariables: