fix: preserve ephemeral environment variables during modal save and file updates#6832
Conversation
- Updated to only preserve non-ephemeral variables that exist in the new set. - Added tests for and reducers to ensure correct handling of ephemeral and non-ephemeral variables. - Introduced utility with tests to manage variable persistence logic. - Enhanced IPC communication to include alongside . - Updated Bru CLI 2.15.0 class to ensure deletion of variables from both and . - Added tests for to verify correct removal of variables. - Improved environment variable handling in the UI tests to ensure ephemeral variables persist correctly after updates.
sanjaikumar-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughThis PR updates environment-variable handling across the app: adds envVariables to merge/persist flows and IPC payloads, changes mergeAndPersistEnvironment signature to accept envVariables, introduces utilities for merging/persistence decisions, preserves ephemeral vars in reducer logic, and ensures deleteEnvVar clears persistent entries. Changes
Sequence Diagram(s)(Skipped — changes are primarily payload shape and local merge logic without a new multi-component sequential control flow that requires visualization.) 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
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (1)
2764-2783: Guard against missing variables array before re-adding ephemerals.
Ifenvironment.variablesis undefined (e.g., empty env file),pushwill throw. Consider defaulting to an empty array.🛠️ Suggested hardening
- existingEnv.variables = environment.variables; + existingEnv.variables = environment.variables || [];
🧹 Nitpick comments (2)
tests/environments/api-setEnvVar/api-setEnvVar-without-persist.spec.ts (1)
68-100: Considertest.step+ locator variables for readability.This flow is substantial; grouping phases into
test.stepand reusing locators (e.g.,tokenRow,envTab) will make reports and maintenance easier.packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js (1)
2021-2081: All call sites consistently passenvVariablesas an object. The concern is valid defensively, but no current call sites pass an array. If you want to harden against future changes, the optional normalization suggestion is reasonable—but it's not required to fix an existing issue.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/env-reducers.spec.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/utils/environments.spec.jspackages/bruno-electron/src/ipc/network/index.jspackages/bruno-js/src/bru.jspackages/bruno-js/tests/setEnvVar.spec.jstests/environments/api-setEnvVar/api-setEnvVar-without-persist.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:
packages/bruno-js/src/bru.jspackages/bruno-electron/src/ipc/network/index.jstests/environments/api-setEnvVar/api-setEnvVar-without-persist.spec.tspackages/bruno-app/src/providers/ReduxStore/slices/collections/env-reducers.spec.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-js/tests/setEnvVar.spec.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/utils/environments.spec.js
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-setEnvVar/api-setEnvVar-without-persist.spec.ts
🧠 Learnings (6)
📚 Learning: 2026-01-13T13:42:21.661Z
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6792
File: packages/bruno-converters/tests/bruno/bruno-to-postman-translations/variables.test.js:54-73
Timestamp: 2026-01-13T13:42:21.661Z
Learning: In the Bruno converters package (packages/bruno-converters), when translating Bruno variable accessors to Postman: bru.getCollectionVar, bru.getFolderVar, and bru.getRequestVar should all map to pm.variables.get() instead of using Postman's more specific scoped APIs like pm.collectionVariables.get(). The generic pm.variables.get() approach is preferred for these variable types.
Applied to files:
packages/bruno-js/src/bru.js
📚 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-js/src/bru.jspackages/bruno-electron/src/ipc/network/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/env-reducers.spec.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-js/tests/setEnvVar.spec.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/utils/environments.spec.js
📚 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-setEnvVar/api-setEnvVar-without-persist.spec.tspackages/bruno-app/src/utils/environments.spec.js
📚 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-setEnvVar/api-setEnvVar-without-persist.spec.tspackages/bruno-app/src/providers/ReduxStore/slices/collections/env-reducers.spec.jspackages/bruno-js/tests/setEnvVar.spec.jspackages/bruno-app/src/utils/environments.spec.js
📚 Learning: 2026-01-09T18:25:14.640Z
Learnt from: kanakkholwal
Repo: usebruno/bruno PR: 6767
File: packages/bruno-app/src/components/ResponseExample/index.js:221-226
Timestamp: 2026-01-09T18:25:14.640Z
Learning: In the Bruno Electron renderer code (packages/bruno-app), assume window.ipcRenderer is always available and skip existence checks. Do not guard for ipcRenderer in this Electron context; use window.ipcRenderer directly (e.g., window.ipcRenderer.send(...), window.ipcRenderer.on(...)). If there are non-Electron contexts (such as test environments or non-Electron builds), add guards or mocks to avoid runtime errors there, but for the intended Electron renderer files, this pattern should be applied broadly within packages/bruno-app.
Applied to files:
packages/bruno-app/src/providers/ReduxStore/slices/collections/env-reducers.spec.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/utils/environments.spec.js
📚 Learning: 2026-01-07T18:53:37.000Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6735
File: packages/bruno-electron/src/ipc/collection.js:936-939
Timestamp: 2026-01-07T18:53:37.000Z
Learning: In the bruno repository, the Redux reducer determines collection format by checking for the `opencollection` property in brunoConfig. The `version` property is not used for format detection, so having both `version` and `opencollection` properties together doesn't cause issues since `opencollection` takes precedence.
Applied to files:
packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
🧬 Code graph analysis (4)
tests/environments/api-setEnvVar/api-setEnvVar-without-persist.spec.ts (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js (6)
selectEnvironment(2095-2120)selectEnvironment(2095-2120)sendRequest(508-624)sendRequest(508-624)saveEnvironment(1748-1786)saveEnvironment(1748-1786)
packages/bruno-app/src/providers/ReduxStore/slices/collections/env-reducers.spec.js (2)
packages/bruno-app/src/utils/environments.spec.js (1)
require(1-1)packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js (4)
saveEnvironment(1748-1786)saveEnvironment(1748-1786)collectionAddEnvFileEvent(2426-2449)collectionAddEnvFileEvent(2426-2449)
packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js (1)
packages/bruno-app/src/utils/collections/index.js (1)
envVariables(1171-1171)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (5)
packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/GenerateCodeItem/index.js (2)
variables(86-88)environment(34-34)packages/bruno-app/src/utils/collections/index.js (8)
variables(1086-1086)variables(1111-1111)variables(1333-1333)environment(1087-1087)environment(1099-1099)environment(1113-1113)environment(1133-1133)environment(1585-1585)packages/bruno-app/src/components/VariablesEditor/index.js (1)
environment(36-36)packages/bruno-app/src/providers/ReduxStore/slices/global-environments.js (3)
environment(33-33)environment(42-42)environment(61-61)packages/bruno-app/src/utils/codemirror/brunoVarInfo.js (1)
target(638-638)
⏰ 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 - Linux
- GitHub Check: SSL Tests - macOS
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
🔇 Additional comments (7)
packages/bruno-js/src/bru.js (1)
197-199: Keeps env and persisted stores consistent.Nice catch syncing
deleteEnvVarwithpersistentEnvVariables.packages/bruno-js/tests/setEnvVar.spec.js (1)
76-112: Solid coverage for delete behavior.These tests validate both in-memory and persisted deletion paths as expected.
packages/bruno-app/src/utils/environments.spec.js (1)
1-84: Thorough test matrix.Covers save/merge/default behavior and edge cases well.
tests/environments/api-setEnvVar/api-setEnvVar-without-persist.spec.ts (1)
7-16: Good fixture hygiene.Preserving and restoring the Stage.bru fixture keeps this suite isolated.
packages/bruno-electron/src/ipc/network/index.js (1)
427-439: Good payload enrichment.Including
envVariablesalongsidepersistentEnvVariableskeeps renderer state accurate during script updates.Also applies to: 525-529, 571-575, 907-911
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (1)
250-265: Preserving ephemerals on save looks good.
Clean and aligns with the modal-save semantics.packages/bruno-app/src/providers/ReduxStore/slices/collections/env-reducers.spec.js (1)
1-131: Solid coverage for ephemeral preservation and overlay behavior.
These tests pin down the intended reducer semantics well.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Introduced utility to handle merging of existing and new persistent variables while preserving metadata. - Added utility to determine which variable names should be persisted based on existing and new variables. - Updated action to utilize the new utilities for better management of environment variables. - Enhanced unit tests for the new utilities to ensure correct functionality and edge case handling.
1 participant
BRU-2444