feat(scripting): persist environment variable changes from scripts by default#8186
Open
sanish-bruno wants to merge 12 commits into
Open
feat(scripting): persist environment variable changes from scripts by default#8186sanish-bruno wants to merge 12 commits into
sanish-bruno wants to merge 12 commits into
Conversation
sanish-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:
WalkthroughRefactors script-driven variable persistence: replaces per-call persistentEnvVariables with collectionVariables, simplifies persisted-variable shaping, updates IPC/runtime contracts, splits store updates from persistence side-effects, and re-enables Bru collection/global variable APIs with matching tests and fixtures. ChangesEnvironment Persistence and Variable Flow Rework
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
- Renamed the guide to "Writing Playwright Tests for Bruno" for clarity. - Reorganized the table of contents for better navigation. - Expanded sections on test creation, structure, and best practices. - Added detailed instructions for using codegen and organizing test files. - Introduced new sections on test patterns and recipes for common scenarios. - Updated prerequisites and setup instructions for running tests.
sanish-bruno
force-pushed
the
feat/persist-envs-by-default
branch
from
4d51255 to
2e4cb5c
Compare
sanish-bruno
changed the title
sanish-bruno
changed the title
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-js/src/runtime/test-runtime.js (1)
62-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
runTests()return shape consistent in the no-tests branch.Line 62 returns early without
collectionVariables, while the main return path now includes it (Line 130). This creates a branch-dependent runtime contract and can break downstream consumers when a request has no test script.🔧 Proposed fix
if (!testsFile || !testsFile.length) { return { request, - envVariables, - runtimeVariables, - globalEnvironmentVariables, + envVariables: cleanJson(envVariables), + runtimeVariables: cleanJson(runtimeVariables), + collectionVariables: cleanJson(collectionVariables), + globalEnvironmentVariables: cleanJson(globalEnvironmentVariables), results: __brunoTestResults.getResults(), nextRequestName: bru.nextRequest }; }🤖 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-js/src/runtime/test-runtime.js` around lines 62 - 70, The early return in runTests() when testsFile is empty returns a payload missing collectionVariables; update the no-tests branch to include collectionVariables (same variable used in the main return) so the function's return shape is consistent—ensure the early return object includes request, envVariables, runtimeVariables, globalEnvironmentVariables, collectionVariables, results: __brunoTestResults.getResults(), and nextRequestName: bru.nextRequest.
🧹 Nitpick comments (2)
tests/environments/api-setEnvVar/api-setEnvVar-without-persist.spec.ts (1)
34-50: ⚡ Quick winAlign test naming/path with new default-persist behavior.
Line 34 and Line 50 now validate persistence across restart, but the spec/fixture naming still says
without-persist, which makes intent misleading. Please rename the spec title/path (and matching fixture names) to reflect default persistence semantics.As per coding guidelines: "E2E tests must verify user-visible behaviour... and app-level outcomes." Clear naming should match that verified outcome.
🤖 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/environments/api-setEnvVar/api-setEnvVar-without-persist.spec.ts` around lines 34 - 50, The test currently asserts that environment variables persist across restart but is named "api-setEnvVar-without-persist" which is misleading; rename the spec filename and any referenced fixture/test identifiers (for example the visible collection name string 'api-setEnvVar-without-persist' used in the locator and any fixture files) to a name that reflects persistence (e.g., 'api-setEnvVar-with-persist' or similar), update any matching fixture names and references in the test (selectors, displayed collection name, and test title) so the spec name/path and fixture names align with the verified behavior, and run the suite to ensure selectors like '`#sidebar-collection-name`' and the 'Environments' tab still resolve after renaming.packages/bruno-js/tests/setEnvVar.spec.js (1)
20-30: ⚡ Quick winAdd explicit falsy-value coverage in
allows non-string values.This test is close, but it should also lock behavior for
0,false, and""to prevent regressions around falsy handling.Suggested additions
test('allows non-string values', () => { const bru = makeBru(); bru.setEnvVar('count', 42); expect(bru.envVariables.count).toBe(42); bru.setEnvVar('active', true); expect(bru.envVariables.active).toBe(true); bru.setEnvVar('config', { port: 3000 }); expect(bru.envVariables.config).toEqual({ port: 3000 }); + + bru.setEnvVar('zero', 0); + expect(bru.envVariables.zero).toBe(0); + + bru.setEnvVar('inactive', false); + expect(bru.envVariables.inactive).toBe(false); + + bru.setEnvVar('empty', ''); + expect(bru.envVariables.empty).toBe(''); });Based on learnings: “falsy values are legitimate environment variable values and should not be filtered out.”
🤖 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-js/tests/setEnvVar.spec.js` around lines 20 - 30, Update the "allows non-string values" test to include explicit assertions for falsy values so they are not filtered out: after creating bru via makeBru() and before or after the existing non-string checks, call bru.setEnvVar('zero', 0), bru.setEnvVar('flag', false), and bru.setEnvVar('empty', ''), then assert bru.envVariables.zero === 0, bru.envVariables.flag === false, and bru.envVariables.empty === '' to lock behavior for falsy values when using setEnvVar.
🤖 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 `@packages/bruno-app/src/components/Preferences/General/index.js`:
- Around line 314-326: The new checkbox input with id/name
"scripting.envVariablesPersistEnabled" used by Formik lacks a stable Playwright
selector; add a data-testid attribute to the input (e.g.,
data-testid="scripting.envVariablesPersistEnabled") so E2E tests can target it
reliably, ensuring the attribute is placed on the same input element that binds
to formik.values.scripting.envVariablesPersistEnabled and formik.handleChange.
In `@packages/bruno-app/src/providers/App/useIpcEvents.js`:
- Around line 211-215: The ipcRenderer listener
(removeGlobalEnvironmentVariablesUpdateListener callback) calls
persistActiveGlobalEnvironment() but does not handle its returned Promise, so
rejections become unhandled; update the listener to handle the Promise by
chaining .catch(...) (or using async/await with try/catch) on
persistActiveGlobalEnvironment() and in the catch block dispatch an
error-handling action or show a user-visible notification/log (e.g., dispatch a
persist failure action or call the app's toast/error logger) so schema or IPC
save failures are observed and surfaced to the user.
In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js`:
- Around line 416-419: The current code mutates activeEnvironment.variables by
filtering out enabled vars not present in scriptVarNames, which causes
request-scoped deletions (from deleteEnvVar/deleteAllEnvVars after setEnvVar in
a run) to be persisted to disk; fix by introducing a separate persisted
baseline/sentinel (e.g., persistedVariables or baselineEnv) and change the
save/merge logic so activeEnvironment.variables continues to represent the
request-scoped view while persistedVariables is only updated by explicit
persistent operations — when merging, do not remove an enabled var from
persistedVariables merely because scriptVarNames lacks it; instead, persist
additions/updates from setEnvVar but ignore request-only deletes unless a new
persistent-delete API is invoked (update code paths around
activeEnvironment.variables, scriptVarNames, deleteEnvVar, deleteAllEnvVars, and
the persistence/save routine).
In `@packages/bruno-electron/src/ipc/network/index.js`:
- Around line 557-575: The IPC persistence payloads (events
'main:script-environment-update', 'main:global-environment-variables-update',
'main:collection-variables-update' and the other similar blocks) are missing the
concrete environment identifiers captured at request start, so late IPC messages
may persist to the currently active environment instead of the one used for the
request; update the payloads to include the stable IDs from the request context
(e.g., include scriptResult.environmentUid and scriptResult.globalEnvironmentUid
or whatever the request-start fields are) alongside
requestUid/collectionUid/persist so the renderer can persist against those exact
environment/global-environment IDs rather than resolving active state on
receipt.
In `@packages/bruno-js/tests/runtime.spec.js`:
- Around line 203-209: The test currently uses
expect(result.persistentEnvVariables).toBeUndefined() which still passes if the
property exists but is set to undefined; update the assertion to assert the
property is absent instead — for example, replace that line with a
property-level check such as
expect(result).not.toHaveProperty('persistentEnvVariables') (or assert that
'persistentEnvVariables' in result is false) in the test that calls
ScriptRuntime.runRequestScript so it verifies the field is not present at all.
---
Outside diff comments:
In `@packages/bruno-js/src/runtime/test-runtime.js`:
- Around line 62-70: The early return in runTests() when testsFile is empty
returns a payload missing collectionVariables; update the no-tests branch to
include collectionVariables (same variable used in the main return) so the
function's return shape is consistent—ensure the early return object includes
request, envVariables, runtimeVariables, globalEnvironmentVariables,
collectionVariables, results: __brunoTestResults.getResults(), and
nextRequestName: bru.nextRequest.
---
Nitpick comments:
In `@packages/bruno-js/tests/setEnvVar.spec.js`:
- Around line 20-30: Update the "allows non-string values" test to include
explicit assertions for falsy values so they are not filtered out: after
creating bru via makeBru() and before or after the existing non-string checks,
call bru.setEnvVar('zero', 0), bru.setEnvVar('flag', false), and
bru.setEnvVar('empty', ''), then assert bru.envVariables.zero === 0,
bru.envVariables.flag === false, and bru.envVariables.empty === '' to lock
behavior for falsy values when using setEnvVar.
In `@tests/environments/api-setEnvVar/api-setEnvVar-without-persist.spec.ts`:
- Around line 34-50: The test currently asserts that environment variables
persist across restart but is named "api-setEnvVar-without-persist" which is
misleading; rename the spec filename and any referenced fixture/test identifiers
(for example the visible collection name string 'api-setEnvVar-without-persist'
used in the locator and any fixture files) to a name that reflects persistence
(e.g., 'api-setEnvVar-with-persist' or similar), update any matching fixture
names and references in the test (selectors, displayed collection name, and test
title) so the spec name/path and fixture names align with the verified behavior,
and run the suite to ensure selectors like '`#sidebar-collection-name`' and the
'Environments' tab still resolve after renaming.
🪄 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: 492640cb-64ee-43d3-b18b-91329a8ff513
📒 Files selected for processing (21)
packages/bruno-app/src/components/Preferences/General/index.jspackages/bruno-app/src/providers/App/useIpcEvents.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/providers/ReduxStore/slices/global-environments.jspackages/bruno-app/src/utils/collections/index.jspackages/bruno-app/src/utils/environments.jspackages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/src/store/preferences.jspackages/bruno-js/src/bru.jspackages/bruno-js/src/runtime/script-runtime.jspackages/bruno-js/src/runtime/test-runtime.jspackages/bruno-js/src/runtime/vars-runtime.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.jspackages/bruno-js/tests/runtime.spec.jspackages/bruno-js/tests/setEnvVar.spec.jstests/environments/api-setEnvVar/api-setEnvVar-with-persist.spec.tstests/environments/api-setEnvVar/api-setEnvVar-without-persist.spec.tstests/environments/api-setEnvVar/fixtures/collection/api-setEnvVar-with-persist.brutests/environments/api-setEnvVar/fixtures/collection/multiple-persist-vars-folder/multiple-persist-vars-1.brutests/environments/api-setEnvVar/fixtures/collection/multiple-persist-vars-folder/multiple-persist-vars-2.bru
- Eliminated the scripting environment persistence option from preferences and related components. - Updated form validation and state management to reflect the removal of the scripting section. - Adjusted IPC event handling to simplify environment updates by removing unnecessary persistence checks. - Cleaned up related code in Redux actions and network IPC to enhance clarity and maintainability.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js (3)
2263-2272:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn or handle these async persistence calls.
Both thunks fire async saves and then return
undefined. That makes persistence failures fromsaveEnvironment()/saveCollectionRoot()invisible to the IPC listener path and can surface as unhandled rejections with no user feedback.Minimal fix
export const persistActiveEnvironment = (collectionUid) => (dispatch, getState) => { const state = getState(); const collection = findCollectionByUid(state.collections.collections, collectionUid); if (!collection) return; const environment = findEnvironmentInCollection(collection, collection.activeEnvironmentUid); if (!environment) return; - dispatch(saveEnvironment(environment.variables, environment.uid, collectionUid)); + return dispatch(saveEnvironment(environment.variables, environment.uid, collectionUid)); }; @@ - dispatch(saveCollectionRoot(collectionUid)); + return dispatch(saveCollectionRoot(collectionUid)); };Also applies to: 2313-2314
🤖 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/providers/ReduxStore/slices/collections/actions.js` around lines 2263 - 2272, persistActiveEnvironment currently fires the async thunk saveEnvironment and then returns undefined, hiding failures; change it to return the async result so callers/IPCs can observe errors (e.g., return the promise from dispatch(saveEnvironment(...)) or await and rethrow). Update persistActiveEnvironment (and the similar persist function referenced around saveCollectionRoot) to return the dispatch(...) result and/or await dispatch(...) inside a try/catch and propagate or rethrow the error so persistence failures are visible to the IPC listener.
2313-2313:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis request-driven path is saving the whole collection draft.
saveCollectionRoot()persistscollection.draftwhen it exists, so a script-sidesetCollectionVar()can implicitly write unrelated unsaved collection settings to disk as a side effect of running a request.🤖 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/providers/ReduxStore/slices/collections/actions.js` at line 2313, The dispatch(saveCollectionRoot(collectionUid)) call is persisting collection.draft and thus can write unrelated unsaved settings when run from a script-driven setCollectionVar; change this to only persist request-level changes instead of the whole draft: either call or implement and call a dedicated action like saveCollectionRequests(collectionUid) that writes only the requests/vars, or add a parameter to saveCollectionRoot(collectionUid, { persistDraft: false }) and update saveCollectionRoot to skip saving collection.draft when that flag is set; update the caller here to use the non-draft-saving variant so setCollectionVar() no longer has side effects.
2300-2314:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeleted collection vars never reach Redux before you persist.
After
vars = vars.filter(...), this thunk only upserts the survivors. Any enabled variable removed by the script is still present in Redux, sosaveCollectionRoot(collectionUid)serializes the stale entry right back to disk.🤖 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/providers/ReduxStore/slices/collections/actions.js` around lines 2300 - 2314, After filtering out script-deleted vars you only upsert survivors, so enabled vars removed by the script remain in Redux and get re-saved; fix by computing the set of surviving var UIDs after vars = vars.filter(...), read the current request vars via get(root, 'request.vars.req', []), and for each existing var (ev) that is enabled and whose uid is not in the surviving set dispatch the matching removal action (e.g., removeCollectionVar({ collectionUid, type: 'request', var: ev }) or the project's remove action), then continue with the existing upsert logic using updateCollectionVar/addCollectionVar and finally dispatch(saveCollectionRoot(collectionUid)).
🤖 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.
Outside diff comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js`:
- Around line 2263-2272: persistActiveEnvironment currently fires the async
thunk saveEnvironment and then returns undefined, hiding failures; change it to
return the async result so callers/IPCs can observe errors (e.g., return the
promise from dispatch(saveEnvironment(...)) or await and rethrow). Update
persistActiveEnvironment (and the similar persist function referenced around
saveCollectionRoot) to return the dispatch(...) result and/or await
dispatch(...) inside a try/catch and propagate or rethrow the error so
persistence failures are visible to the IPC listener.
- Line 2313: The dispatch(saveCollectionRoot(collectionUid)) call is persisting
collection.draft and thus can write unrelated unsaved settings when run from a
script-driven setCollectionVar; change this to only persist request-level
changes instead of the whole draft: either call or implement and call a
dedicated action like saveCollectionRequests(collectionUid) that writes only the
requests/vars, or add a parameter to saveCollectionRoot(collectionUid, {
persistDraft: false }) and update saveCollectionRoot to skip saving
collection.draft when that flag is set; update the caller here to use the
non-draft-saving variant so setCollectionVar() no longer has side effects.
- Around line 2300-2314: After filtering out script-deleted vars you only upsert
survivors, so enabled vars removed by the script remain in Redux and get
re-saved; fix by computing the set of surviving var UIDs after vars =
vars.filter(...), read the current request vars via get(root,
'request.vars.req', []), and for each existing var (ev) that is enabled and
whose uid is not in the surviving set dispatch the matching removal action
(e.g., removeCollectionVar({ collectionUid, type: 'request', var: ev }) or the
project's remove action), then continue with the existing upsert logic using
updateCollectionVar/addCollectionVar and finally
dispatch(saveCollectionRoot(collectionUid)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3fcbd240-ea53-4f6e-bd60-45f08eee282f
📒 Files selected for processing (3)
packages/bruno-app/src/providers/App/useIpcEvents.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-electron/src/ipc/network/index.js
- Removed the explicit save to disk action from the collection variable update event. - Implemented silent saving of collection state using IPC, enhancing user experience by eliminating unnecessary notifications. - Improved code clarity by separating state update logic from persistence handling.
… APIs - Introduced a new test suite for collection variable functionalities, covering set, get, check existence, delete, and retrieve all variables. - Added tests to ensure proper handling of edge cases, such as empty keys and invalid characters. - Enhanced existing test cases for delete operations to confirm no errors occur when deleting non-existent keys. - Updated related test scripts to ensure they do not skip execution due to UI update issues.
sanish-bruno
changed the title
- Introduced a new test suite for global environment variable functionalities, covering set, get, check existence, delete, and retrieve all variables. - Added tests to ensure proper handling of edge cases, such as empty keys and non-string values. - Implemented tests for persistence of environment variables in both developer and safe modes. - Created associated Bru scripts to validate the functionality of setting and retrieving global variables.
…riable deletion and persistence - Introduced a new test suite to validate the deletion of global environment variables using the bru.deleteGlobalEnvVar() function. - Added tests to ensure that deleted variables are no longer visible and that existing variables remain unchanged. - Created necessary fixture files for the test collection and environment setup. - Enhanced locators to support new test functionalities in the environment UI.
…dant tests - Updated the baseUrl in the global environment variable configuration to point to the testbench URL. - Removed tests verifying the existence of the baseUrl variable as it is no longer necessary. - Cleaned up related test steps to streamline the global environment variable persistence tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/scripting/bru-api/global-env-var-persistence/global-env-var-persistence.spec.ts`:
- Around line 27-31: The test currently calls
locators.environment.variableValue('baseUrl').textContent() and asserts on the
string which can be null or stale; instead assert directly on the locator so
Playwright can wait for stable text. Replace the textContent extraction and
expect(value).toContain(...) with an assertion on the locator returned by
locators.environment.variableValue('baseUrl') using Playwright's locator
matchers (e.g., toContainText or toHaveText/regex) to check for
'https://testbench-sanity.usebruno.com', keeping the locator expression
unchanged.
In
`@tests/scripting/bru-api/variable-persistence-safe-mode/variable-persistence-safe-mode.spec.ts`:
- Around line 30-31: The test currently calls
locators.environment.variableValue('persistedToken').textContent() and asserts
the returned string; replace this unstable DOM read with a locator-based
assertion by using Playwright's locator expect on the same locator
(locators.environment.variableValue) and assert the visible text contains
"test-value-123" (e.g., use toHaveText/toContainText on the locator) so the
assertion waits/retries and avoids null reads.
- Around line 39-40: Combine the two separate file checks into a single polled
assertion so the test waits until both persisted strings appear together: use
expect.poll(() => fs.readFileSync(envFilePath, 'utf8'), { timeout:
PERSISTENCE_TIMEOUT }).toSatisfy(text => text.includes('persistedToken') &&
text.includes('test-value-123')). Also replace any element.textContent()
followed by expect(value).toContain(...) with a direct Playwright locator
assertion (e.g. use expect(locator).toContainText('...') or
expect(locator).toHaveText(...) on the specific Locator) to avoid null/timing
races.
In `@tests/scripting/bru-api/variable-persistence/variable-persistence.spec.ts`:
- Around line 30-31: Replace the current nullable textContent() usage and string
assertion with Playwright's locator assertion: stop calling await
locators.environment.variableValue('persistedToken').textContent() and instead
assert directly on the locator using toContainText, e.g. use
locators.environment.variableValue('persistedToken') with
expect(...).toContainText('test-value-123'); this removes nullable textContent
handling and enables Playwright's auto-waiting on the locator.
- Around line 39-40: Combine the two-file assertions into a single polling
assertion: replace the pair of calls that first await expect.poll(() =>
fs.readFileSync(envFilePath, 'utf8'), { timeout: PERSISTENCE_TIMEOUT
}).toContain('persistedToken') and then immediately call
expect(fs.readFileSync(envFilePath, 'utf8')).toContain('test-value-123') with
one expect.poll that reads envFilePath and asserts the content contains both
'persistedToken' and 'test-value-123' (or polls a boolean that checks both),
using the same PERSISTENCE_TIMEOUT; remove the immediate second fs.readFileSync.
Apply the identical change to the collection.bru checks (collectionFilePath) so
each persisted *.bru file is validated with a single expect.poll that checks for
both expected substrings.
In `@tests/utils/page/locators.ts`:
- Around line 79-80: The current locator collectionEnvTab matches the substring
"Environments" inside "Global Environments"; update both locators to use
anchored regular expressions so they match the whole tab text exactly: change
collectionEnvTab to use page.locator('.request-tab').filter({ hasText:
/^Environments$/ }) and change globalEnvTab to
page.locator('.request-tab').filter({ hasText: /^Global Environments$/ }) so the
two functions (collectionEnvTab and globalEnvTab) no longer overlap and
strict-mode clicks/hover will target the correct tab.
🪄 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: f798985e-0c04-42d5-afad-5f37965f4018
📒 Files selected for processing (24)
packages/bruno-js/tests/globalEnvVar.spec.jspackages/bruno-js/tests/runtime.spec.jspackages/bruno-tests/collection/scripting/api/bru/setGlobalEnvVar.brutests/scripting/bru-api/global-env-var-persistence/fixtures/collections/global-env-var-persistence-test/bruno.jsontests/scripting/bru-api/global-env-var-persistence/fixtures/collections/global-env-var-persistence-test/delete-global-env-var.brutests/scripting/bru-api/global-env-var-persistence/global-env-var-persistence.spec.tstests/scripting/bru-api/global-env-var-persistence/init-user-data/collection-security.jsontests/scripting/bru-api/global-env-var-persistence/init-user-data/global-environments.jsontests/scripting/bru-api/global-env-var-persistence/init-user-data/preferences.jsontests/scripting/bru-api/variable-persistence-safe-mode/fixtures/collections/variable-persistence-safe-test/bruno.jsontests/scripting/bru-api/variable-persistence-safe-mode/fixtures/collections/variable-persistence-safe-test/environments/Test.brutests/scripting/bru-api/variable-persistence-safe-mode/fixtures/collections/variable-persistence-safe-test/set-collection-var.brutests/scripting/bru-api/variable-persistence-safe-mode/fixtures/collections/variable-persistence-safe-test/set-env-var.brutests/scripting/bru-api/variable-persistence-safe-mode/init-user-data/collection-security.jsontests/scripting/bru-api/variable-persistence-safe-mode/init-user-data/preferences.jsontests/scripting/bru-api/variable-persistence-safe-mode/variable-persistence-safe-mode.spec.tstests/scripting/bru-api/variable-persistence/fixtures/collections/variable-persistence-test/bruno.jsontests/scripting/bru-api/variable-persistence/fixtures/collections/variable-persistence-test/environments/Test.brutests/scripting/bru-api/variable-persistence/fixtures/collections/variable-persistence-test/set-collection-var.brutests/scripting/bru-api/variable-persistence/fixtures/collections/variable-persistence-test/set-env-var.brutests/scripting/bru-api/variable-persistence/init-user-data/collection-security.jsontests/scripting/bru-api/variable-persistence/init-user-data/preferences.jsontests/scripting/bru-api/variable-persistence/variable-persistence.spec.tstests/utils/page/locators.ts
✅ Files skipped from review due to trivial changes (18)
- tests/scripting/bru-api/variable-persistence-safe-mode/fixtures/collections/variable-persistence-safe-test/bruno.json
- tests/scripting/bru-api/variable-persistence-safe-mode/fixtures/collections/variable-persistence-safe-test/environments/Test.bru
- tests/scripting/bru-api/variable-persistence-safe-mode/init-user-data/collection-security.json
- tests/scripting/bru-api/variable-persistence/init-user-data/collection-security.json
- tests/scripting/bru-api/variable-persistence-safe-mode/init-user-data/preferences.json
- tests/scripting/bru-api/variable-persistence/init-user-data/preferences.json
- tests/scripting/bru-api/global-env-var-persistence/fixtures/collections/global-env-var-persistence-test/bruno.json
- tests/scripting/bru-api/variable-persistence-safe-mode/fixtures/collections/variable-persistence-safe-test/set-collection-var.bru
- tests/scripting/bru-api/global-env-var-persistence/init-user-data/collection-security.json
- tests/scripting/bru-api/variable-persistence/fixtures/collections/variable-persistence-test/set-collection-var.bru
- tests/scripting/bru-api/variable-persistence/fixtures/collections/variable-persistence-test/bruno.json
- tests/scripting/bru-api/variable-persistence/fixtures/collections/variable-persistence-test/environments/Test.bru
- tests/scripting/bru-api/global-env-var-persistence/fixtures/collections/global-env-var-persistence-test/delete-global-env-var.bru
- packages/bruno-tests/collection/scripting/api/bru/setGlobalEnvVar.bru
- tests/scripting/bru-api/global-env-var-persistence/init-user-data/preferences.json
- tests/scripting/bru-api/variable-persistence-safe-mode/fixtures/collections/variable-persistence-safe-test/set-env-var.bru
- tests/scripting/bru-api/variable-persistence/fixtures/collections/variable-persistence-test/set-env-var.bru
- tests/scripting/bru-api/global-env-var-persistence/init-user-data/global-environments.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-js/tests/runtime.spec.js
…ariable persistence logic - Removed the persistActiveGlobalEnvironment function to simplify the global environment update process. - Updated the global environment persistence logic to directly use computed variables instead of relying on Redux state. - Enhanced error handling during the persistence operation to log failures. - Added a new test to verify the addition of global environment variables through the bru.setGlobalEnvVar() function.
sanish-bruno
changed the title
sanish-bruno
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/scripting/bru-api/global-env-var-persistence/global-env-var-persistence.spec.ts (1)
52-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse locator text assertions instead of
textContent()extraction in the new test.This repeats the earlier flaky pattern; assert directly on the locator so Playwright handles waiting and nullability.
Suggested change
- const value = await locators.environment.variableValue('newGlobalVar').textContent(); - expect(value).toContain('new-global-value'); + await expect(locators.environment.variableValue('newGlobalVar')).toContainText('new-global-value'); ... - const value = await locators.environment.variableValue('baseUrl').textContent(); - expect(value).toContain('https://testbench-sanity.usebruno.com'); + await expect(locators.environment.variableValue('baseUrl')).toContainText('https://testbench-sanity.usebruno.com');As per coding guidelines, E2E assertions should be robust and behavior-focused rather than brittle value extraction.
Also applies to: 58-62
🤖 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/scripting/bru-api/global-env-var-persistence/global-env-var-persistence.spec.ts` around lines 52 - 56, Replace the manual textContent() extraction with Playwright locator assertions so Playwright waits and handles nullability: for the block using locators.environment.variableRowByName('newGlobalVar') and locators.environment.variableValue('newGlobalVar'), assert directly on the value locator (e.g., use toHaveText/toContainText on variableValue('newGlobalVar')) instead of calling .textContent() and then expect(value). Also update the similar pattern in the subsequent block (the one referenced at 58-62) to use locator-based assertions on locators.environment.variableValue(...) rather than extracting text.Source: Coding guidelines
🤖 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 `@packages/bruno-app/src/providers/ReduxStore/slices/global-environments.js`:
- Around line 306-323: Before dispatching _saveGlobalEnvironment({
environmentUid, variables }) capture the current variables from state (e.g. via
getState() or the environment object) so you can roll back on failure; after
calling environmentSchema.validate(...) and
ipcRenderer.invoke('renderer:save-global-environment', ...), if either promise
rejects dispatch _saveGlobalEnvironment({ environmentUid, variables:
previousVariables }) to revert the store and surface the error (e.g.
console.error and/or show a user-facing notification via ipcRenderer) so the UI
cannot silently diverge from persisted disk state.
In `@packages/bruno-app/src/utils/codemirror/autocomplete.js`:
- Around line 109-113: The autocomplete hint list was expanded to include
bru.setCollectionVar, bru.hasCollectionVar, bru.deleteCollectionVar,
bru.deleteAllCollectionVars, and bru.getAllCollectionVars but there are no
explicit unit assertions for these new hints; add explicit assertions in the
autocomplete tests to cover each new bru.* entry (and the other entries noted
around lines for 138-140) by updating the relevant test file to assert that the
autocomplete suggestions include bru.setCollectionVar(...),
bru.hasCollectionVar(...), bru.deleteCollectionVar(...),
bru.deleteAllCollectionVars(), and bru.getAllCollectionVars() (and the
additional bru.* entries mentioned) so future regressions are caught; target the
test suite that verifies CodeMirror/autocomplete hint output and add simple
presence checks for these symbol strings.
---
Duplicate comments:
In
`@tests/scripting/bru-api/global-env-var-persistence/global-env-var-persistence.spec.ts`:
- Around line 52-56: Replace the manual textContent() extraction with Playwright
locator assertions so Playwright waits and handles nullability: for the block
using locators.environment.variableRowByName('newGlobalVar') and
locators.environment.variableValue('newGlobalVar'), assert directly on the value
locator (e.g., use toHaveText/toContainText on variableValue('newGlobalVar'))
instead of calling .textContent() and then expect(value). Also update the
similar pattern in the subsequent block (the one referenced at 58-62) to use
locator-based assertions on locators.environment.variableValue(...) rather than
extracting text.
🪄 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: 0ae03133-0c36-4189-8120-e4072583d5f9
📒 Files selected for processing (6)
packages/bruno-app/src/providers/App/useIpcEvents.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/providers/ReduxStore/slices/global-environments.jspackages/bruno-app/src/utils/codemirror/autocomplete.jstests/scripting/bru-api/global-env-var-persistence/fixtures/collections/global-env-var-persistence-test/set-global-env-var.brutests/scripting/bru-api/global-env-var-persistence/global-env-var-persistence.spec.ts
💤 Files with no reviewable changes (1)
- packages/bruno-app/src/providers/App/useIpcEvents.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js
- Introduced setCollectionVars action to streamline the update process for collection variables. - Removed redundant logic for updating Redux state, enhancing code clarity and performance. - Adjusted IPC event handling to eliminate unnecessary data transmission related to collection variables.
…ty and performance - Refactored variable persistence tests to use `toContainText` for better clarity and efficiency. - Simplified file reading assertions in persistence tests to check for multiple expected values in a single poll. - Removed unnecessary text content extraction steps to streamline test execution.
…ions and transformations - Enhanced comments in the collection variable update event to clarify the synchronous nature of Redux Toolkit dispatches. - Updated comments in the collection transformation utility to specify the exclusion of internal metadata during environment saving.
…te listeners - Added a comment highlighting the need to implement a dirty flag for the script environment update listener to prevent unnecessary disk writes when no variables change. - This change aims to improve performance in high-frequency runner scenarios while maintaining current functionality.
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.
Resolves BRU-3096
Summary
Makes all
bru.*environment, global environment, and collection variable setter/deleter APIs persist changes to disk by default. Removes the old opt-in{ persist: true }flag frombru.setEnvVar(). Enables previously disabled APIs.APIs now persisting by default
bru.setEnvVar(key, value)— persistence parameter removed, always persistsbru.setGlobalEnvVar(key, value)— already persisted, no changebru.deleteEnvVar(key)— now persists deletionbru.deleteGlobalEnvVar(key)— previously disabled, now enabled and persistsbru.deleteAllGlobalEnvVars()— previously disabled, now enabled and persistsbru.setCollectionVar(key, value)— previously disabled, now enabled and persistsbru.deleteCollectionVar(key)— previously disabled, now enabled and persistsbru.deleteAllCollectionVars()— previously disabled, now enabled and persistsbru.getAllCollectionVars()— previously disabled, now enabled (read-only)What was removed (dead weight)
The entire
persistentEnvVariablesopt-in mechanism:persistentEnvVariablesproperty on the Bru classmain:persistent-env-variables-updateIPC event (all send locations + listener)mergeAndPersistEnvironmentRedux actionephemeral/persistedValueoverlay system inscriptEnvironmentUpdateEventreducerbuildPersistedEnvVariablesmerge mode inenvironments.jsWhat was added
main:collection-variables-updateIPC event — new persistence path for collection variablespersistActiveEnvironmentthunk — saves active env from Redux state after script executionpersistActiveGlobalEnvironmentthunk — saves active global env after script executioncollectionVariablesUpdateEventthunk — syncs collection variable changes to diskscriptEnvironmentUpdateEventreducer — direct value updates, no overlayglobalEnvironmentsUpdateEventnow handles deletions (fixes the "UI sync issue" that keptdeleteGlobalEnvVardisabled)Architecture change
Before (two parallel IPC flows):
After (single flow, always persist):
Re-run safety (idempotency)
set*on existing key: silent overwritedelete*on missing key: silent no-opdeleteAll*on empty scope: silent no-opHolds across single request, collection runner, and CLI execution.
Collection variable scope
bru.setCollectionVar()/bru.deleteCollectionVar()operate on pre-request variables (request.vars.req), not post-response expression variables.CLI behavior
The CLI has never persisted variable changes to disk — not even with the old
{ persist: true }flag. ThepersistentEnvVariablesmechanism was entirely Electron/renderer-side. This PR does not change CLI behavior. CLI persistence is tracked separately in BRU-3096.Bug fixes included
deleteGlobalEnvVar/deleteAllGlobalEnvVarsnow properly sync with the UI (previously disabled due to "UI sync issue")Breaking Change
bru.setEnvVar(key, value)now persists to disk by default{ persist: true/false }options parameter is removed (extra args are silently ignored by JS)setEnvVarwill now see changes saved to the environment fileFiles changed (persistence-related)
packages/bruno-js/src/bru.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.jsscript-runtime.js,test-runtime.js,vars-runtime.jspackages/bruno-electron/src/ipc/network/index.jscollections/index.jscollections/actions.jsglobal-environments.jsuseIpcEvents.jsenvironments.js,collections/index.jssetEnvVar.spec.js,runtime.spec.js,collectionVar.spec.js(new)bruno-tests/collection/scripting/api/bru/— unskipped + addeddeleteEnvVar.brutests/environments/api-setEnvVar/— updated fixtures + expectationsTest plan
npm run test --workspace=packages/bruno-js— 523 tests passnpm run test --workspace=packages/bruno-app— 955 tests passnpm run lint— 0 errorsbru.setEnvVar("k", "v")persists to env file after requestbru.deleteEnvVar("k")removes from env filebru.deleteGlobalEnvVar("k")removes from global envbru.setCollectionVar("k", "v")persists to collection rootnpm run test:e2e(api-setEnvVar suite)🤖 Generated with Claude Code
Contribution Checklist:
Summary by CodeRabbit
New Features
Refactor
Tests