feat(cli): persist env, global-env, and collection var changes from scripts#8330
Closed
sanish-bruno wants to merge 34 commits into
Closed
feat(cli): persist env, global-env, and collection var changes from scripts#8330sanish-bruno wants to merge 34 commits into
sanish-bruno wants to merge 34 commits into
Conversation
feat(collections): implement script-driven update for collection variables, ensuring direct root modification and draft synchronization feat(collections): enhance script variable management with baseline tracking and draft preservation
…s by implementing dirty flags fix(collections): handle errors during environment persistence in script execution feat(collections): implement baseline clearing for script execution and optimize variable update handling feat(tests): add default persistence tests for environment variables and update runtime variable handling refactor(collections): streamline variable update handling and improve draft management by removing redundant comments and optimizing code clarity test(collection-vars): add verification for draft edits and script variable visibility in collection settings UI refactor(collection-vars): update header value selection logic for improved clarity and accuracy in draft isolation tests
…lve stale active UIDs and improve persistence logic - Updated the `updateGlobalEnvironments` reducer to handle stale active UIDs by matching against environment names. - Improved the logic for setting global environments and active UIDs to ensure consistency after disk reloads. - Removed outdated tests related to persisted values in favor of more relevant assertions for environment variable handling.
…al and collection environments - Added tests to infer data types (number, boolean, object) when setting environment and collection variables. - Updated the logic to preserve existing data types when variables are not modified by scripts. - Implemented dirty flags to track changes in typed variables, ensuring accurate persistence across sessions. - Refactored related tests to verify the correct behavior of typed variables in various scenarios.
…ion methods - Removed redundant data type inference logic from global and collection variable updates to simplify the codebase. - Updated deletion methods in the Bru class to use Object.keys for improved resilience against user-defined properties. - Added tests to ensure deletion methods function correctly even when properties are shadowed. - Enhanced clarity in draft merge tests by standardizing keyboard shortcuts for selecting all text.
…eractions - Updated test cases to reflect the correct variable name 'wasSaved' instead of 'was-saved'. - Modified environment panel interaction to remove forced click, enhancing test reliability. - Added a utility function to close the environment panel in safe mode tests for better readability and maintainability.
- Introduced a new method to clear script-driven variable baselines for collections, ensuring no stale data leaks into new requests. - Updated the handling of runtime variables in the Bru class to track changes with a new dirty flag, improving state management. - Refactored the application of script environment variables to prevent direct mutations, ensuring immutability and cleaner state updates. - Enhanced the response handling in the script runtime to conditionally include runtime variables based on their dirty state.
…llections and environments - Enhanced event listeners to clear global environment baselines on both 'testrun-started' and 'request-queued' events, preventing stale data issues. - Updated global environment and collection variable update events to ignore stale updates from superseded requests, ensuring accurate state management. - Refactored the Bru class to optimize variable management, including checks for existing keys before updates and deletions, improving performance and reliability. - Introduced request UID tracking to maintain consistency across variable updates during concurrent requests.
- Replaced the dispatch of `_clearScriptGlobalEnvBaseline` with `clearScriptVariableBaselines` to improve clarity and maintainability in the Redux action handling for collections.
…ved variable management - Added a new utility function, `getScriptModifiedKeys`, to identify keys modified by scripts relative to a baseline, enhancing the handling of data types during variable updates. - Updated the application of script environment variables to prevent overwriting user-defined draft changes during no-op writes. - Refactored related logic in collections and global environments to utilize the new utility, ensuring accurate state management and improved clarity in the Redux slices.
…n updateGlobalEnvironments reducer - Streamlined the logic for resolving the active global environment UID by consolidating conditions into a more concise format. - Removed outdated comments to enhance code clarity and maintainability. - Updated tests to ensure accurate resolution of active UIDs based on incoming environment data.
sanish-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno,
sid-bruno,
utkarsh-bruno and
vijayh-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:
WalkthroughThis PR updates script-driven variable handling across Bruno’s runtime, app, IPC, CLI, and tests. It removes explicit env persist options, adds baseline-aware draft merging, skips unchanged writes, and expands UI and integration coverage for env, global, and collection variables. ChangesScript variable update flow
Estimated code review effort🎯 5 (Critical) | ⏱️ ~100 minutes Possibly related PRs
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 |
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: 11
🧹 Nitpick comments (12)
packages/bruno-app/src/utils/collections/index.js (1)
606-610: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression test for export serialization with default
buildPersistedEnvVariablesmode.Line 609 now relies on helper defaults. Please add a focused test that confirms exported environments strip internal metadata while preserving persisted values, so future helper default changes don’t silently alter export output.
🤖 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/collections/index.js` around lines 606 - 610, Add a regression test that validates the export serialization behavior of environments when using the buildPersistedEnvVariables helper with default settings. The test should verify that the variables in exported environments (from the collection serialization code that calls buildPersistedEnvVariables on env?.variables) correctly strips internal metadata while preserving persisted values. This test ensures that future changes to buildPersistedEnvVariables default behavior won't silently alter the export output.packages/bruno-js/tests/runtime.spec.js (1)
229-239: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd runtime-scope null assertions in untouched-scope gating tests.
Line 229 and Line 241 validate null gating for collection/global scopes, but the same contract now applies to
runtimeVariablestoo. Addexpect(result.runtimeVariables).toBeNull();in both cases to fully lock behavior.Suggested patch
expect(result.envVariables).not.toBeNull(); expect(result.envVariables.only_env).toBe('val'); + expect(result.runtimeVariables).toBeNull(); expect(result.collectionVariables).toBeNull(); expect(result.globalEnvironmentVariables).toBeNull(); @@ expect(result.envVariables).not.toBeNull(); expect(result.envVariables.only_env).toBe('val'); + expect(result.runtimeVariables).toBeNull(); expect(result.collectionVariables).toBeNull(); expect(result.globalEnvironmentVariables).toBeNull();As per coding guidelines: "Cover both the 'happy path' and the realistically problematic paths."
Also applies to: 241-253
🤖 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/runtime.spec.js` around lines 229 - 239, The test case verifying null gating for untouched scopes in the dirty-flag gating tests is incomplete. Add an assertion `expect(result.runtimeVariables).toBeNull();` in both test cases (the one with test description "should return null for scopes the script did not touch (dirty-flag gating)" and its companion test case that also validates null behavior for untouched scopes) to ensure that runtimeVariables is also properly gated to null when the script does not modify it, alongside the existing assertions for collectionVariables and globalEnvironmentVariables.Source: Coding guidelines
packages/bruno-cli/tests/utils/persist-variables.spec.js (1)
160-173: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReplace mtime spin-wait assertion with a direct write-call assertion.
This check is timing/filesystem-resolution dependent (
5ms+ busy loop), which is brittle across platforms. Prefer spying onfs.writeFileSync(or the write helper) to assert no write happened.Suggested robust assertion pattern
it('content-comparison guard skips rewrites that are byte-identical', () => { const filePath = writeFile('dev.yml', 'name: dev\nvariables:\n - name: host\n value: same\n' ); - const before = fs.statSync(filePath).mtimeMs; - const wait = Date.now() + 5; - while (Date.now() < wait) { /* spin to ensure mtime can change */ } + const writeSpy = jest.spyOn(fs, "writeFileSync"); persistVariableUpdates( { envVariables: { host: 'same', __name__: 'dev' } }, { envFile: { path: filePath, format: 'yml' } } ); - const after = fs.statSync(filePath).mtimeMs; - expect(after).toBe(before); + expect(writeSpy).not.toHaveBeenCalled(); + writeSpy.mockRestore(); });As per coding guidelines: “Ensure tests are deterministic and reproducible. No randomness, timing dependencies...”
🤖 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-cli/tests/utils/persist-variables.spec.js` around lines 160 - 173, The test currently relies on filesystem modification time (mtime) comparison with a timing-dependent busy loop spin-wait, which is brittle across platforms. Replace this approach by spying on the fs.writeFileSync method to assert that no write occurs when persistVariableUpdates is called with byte-identical content. Remove the timing-dependent busy loop and mtime assertions, and instead verify that the write function is never called during the persistVariableUpdates invocation, making the test deterministic and platform-independent.Source: Coding guidelines
tests/environments/api-setEnvVar/api-setEnvVar-default-persistence.spec.ts (1)
8-8: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse
locators.environment.collectionEnvTab()for locator consistency and precision.Lines 8 and 46 manually create environment tab selectors using substring matching, but the new
collectionEnvTab()locator inlocators.ts(line 102) uses anchored regex (/^Environments$/) for exact matching. This prevents false positives (e.g., if "Global Environments" tab exists) and keeps selector logic centralized.Replace:
- Line 8:
const envTab = page.locator('.request-tab').filter({ hasText: 'Environments' });- Line 46:
const newEnvTab = newPage.locator('.request-tab').filter({ hasText: 'Environments' });With:
- Line 8:
const envTab = locators.environment.collectionEnvTab();- Line 46:
const newEnvTab = newLocators.environment.collectionEnvTab();Also applies to: 46-46
🤖 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-default-persistence.spec.ts` at line 8, The code is manually creating environment tab selectors using substring matching with the hasText filter instead of using the centralized collectionEnvTab() locator. Replace the manual selector assignments for the envTab variable with a call to locators.environment.collectionEnvTab(), and similarly replace the newEnvTab variable assignment with newLocators.environment.collectionEnvTab(). This ensures consistent, exact-match selectors across the test and prevents false positives from similar tab names.tests/environments/collection-vars-draft-merge-with-script/collection-vars-draft-merge-with-script.spec.ts (1)
21-31: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReplace hardcoded wait with event-driven wait for draft debounce.
Line 30's
await page.waitForTimeout(500)defeats the purpose of using Playwright's event-driven patterns and makes the test brittle to timing changes. If the draft debounce is internal UI logic, consider waiting for a visual indicator (e.g., a "pending save" icon disappearing, or polling a stable locator) instead of sleeping.Alternatively, if the debounce duration is unpredictable and no UI event signals completion, document why the wait is necessary and increase the timeout if tests flake under load.
🤖 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/collection-vars-draft-merge-with-script/collection-vars-draft-merge-with-script.spec.ts` around lines 21 - 31, Replace the hardcoded `await page.waitForTimeout(500)` in the 'Open collection settings and edit existingCollVar (create draft)' test step with an event-driven wait that monitors for a visual indicator that the draft save has completed, such as waiting for a pending save icon to disappear or waiting for a stable UI state in the variable editor. If no UI indicator exists, document why the hardcoded wait is necessary for the draft debounce and consider increasing the timeout value to make the test more resilient to timing variations under load.Source: Coding guidelines
tests/scripting/bru-api/variable-persistence-safe-mode/variable-persistence-safe-mode.spec.ts (1)
24-34: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAvoid
force: trueon interactive elements; prefer explicit wait or visibility check.Line 32–33 uses
force: trueto close the tab, which can mask timing issues or indicate the tab isn't actually ready for interaction. Instead, ensure the tab is visible/enabled before clicking, or use a wait strategy likepage.waitForSelector()combined with a stable locator.♻️ Suggested improvement
await locators.environment.collectionEnvTab().hover(); -await locators.environment.collectionEnvTab().getByTestId('request-tab-close-icon').click({ force: true }); +const closeIcon = locators.environment.collectionEnvTab().getByTestId('request-tab-close-icon'); +await expect(closeIcon).toBeVisible(); +await closeIcon.click();🤖 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/variable-persistence-safe-mode/variable-persistence-safe-mode.spec.ts` around lines 24 - 34, The click action on the close icon element with testId 'request-tab-close-icon' uses force: true which can mask timing issues. Remove the force: true parameter from the getByTestId('request-tab-close-icon').click() call and instead add an explicit wait or visibility check to ensure the close icon is ready for interaction. Consider using page.waitForSelector() or a visibility check on the close icon element before attempting to click it, ensuring the tab is actually ready to be closed without forcing the interaction.Source: Coding guidelines
tests/environments/datatype-preservation/datatype-preservation.spec.ts (1)
215-215: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPrefer behavior/state assertion over CSS-class assertion for selected environment.
toHaveClass(/\bactive\b/)couples the test to styling internals. Prefer asserting a stable user-facing selected state (e.g., adata-testid/ARIA-selected marker exposed for the env item) so styling refactors don’t break this flow.As per coding guidelines, E2E tests should verify user-visible behavior and prefer stable selectors over brittle implementation details.
🤖 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/datatype-preservation/datatype-preservation.spec.ts` at line 215, Replace the toHaveClass assertion for the importedItem element with a more stable user-facing behavior assertion. Instead of asserting the active CSS class using toHaveClass(/\bactive\b/), assert a stable semantic marker such as an ARIA attribute (like aria-selected or aria-current) or a data-testid attribute that indicates the selected state. This decouples the test from styling internals and ensures the test verifies actual user-visible behavior rather than implementation details, making it resilient to future styling refactors.Source: Coding guidelines
tests/environments/draft-merge-with-script/fixtures/collections/draft-merge-test/set-env-var.bru (1)
1-23: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winFix fixture directory naming: use
fixtures/collection/(singular) for single collection.Per coding guidelines, fixture directories should use
fixtures/collection/(singular) when there is one collection andfixtures/collections/(plural) when there are multiple. This test usesfixtures/collections/with a single collection nameddraft-merge-test.Move this file from:
tests/environments/draft-merge-with-script/fixtures/collections/draft-merge-test/set-env-var.bruto:
tests/environments/draft-merge-with-script/fixtures/collection/draft-merge-test/set-env-var.bruAnd update
init-user-data/preferences.jsonline 4 to use{{collectionPath}}/draft-merge-testinstead of{{collectionPath}}/draft-merge-test(the collection name stays the same, only the parent directory name changes).🤖 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/draft-merge-with-script/fixtures/collections/draft-merge-test/set-env-var.bru` around lines 1 - 23, The fixture directory is not following the naming convention for single collections. The directory structure uses the plural form fixtures/collections with a single collection, but per coding guidelines, it should use the singular form fixtures/collection. Rename the parent directory from collections to collection, then locate and update the path reference in the init-user-data/preferences.json file to reflect the new singular directory name while keeping the collection name (draft-merge-test) unchanged.Source: Learnings
tests/environments/global-env-draft-merge-with-script/fixtures/collections/global-env-draft-merge-test/bruno.json (1)
1-9: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winFix fixture directory naming: use
fixtures/collection/(singular) for single collection.Same issue as draft-merge-with-script. Move from:
tests/environments/global-env-draft-merge-with-script/fixtures/collections/global-env-draft-merge-test/to:
tests/environments/global-env-draft-merge-with-script/fixtures/collection/global-env-draft-merge-test/🤖 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/global-env-draft-merge-with-script/fixtures/collections/global-env-draft-merge-test/bruno.json` around lines 1 - 9, The fixture directory for the global-env-draft-merge-test collection uses the plural form "collections" when it should use the singular form "collection" since this directory contains a single collection. Move the global-env-draft-merge-test directory from under the plural "collections" parent directory to a new singular "collection" parent directory to maintain consistent naming conventions where singular "collection" is used for single collection directories and plural "collections" would be used for multiple collections.Source: Learnings
tests/scripting/bru-api/variable-persistence-typed/fixtures/collections/variable-persistence-typed-test/bruno.json (1)
1-9: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winFix fixture directory naming: use
fixtures/collection/(singular).Move from:
tests/scripting/bru-api/variable-persistence-typed/fixtures/collections/variable-persistence-typed-test/to:
tests/scripting/bru-api/variable-persistence-typed/fixtures/collection/variable-persistence-typed-test/🤖 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/variable-persistence-typed/fixtures/collections/variable-persistence-typed-test/bruno.json` around lines 1 - 9, The directory structure uses the plural form "collections" but should use the singular form "collection" for consistency. Move the entire fixture directory containing the bruno.json file and the "variable-persistence-typed-test" collection from the "collections" parent directory to a "collection" parent directory, ensuring all contents and file structure within the collection directory remain intact during this reorganization.Source: Learnings
tests/scripting/bru-api/collection-vars-draft-isolation/collection-vars-draft-isolation.spec.ts (1)
34-34: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse a stable locator for the header value editor instead of
nth(1).Targeting the second CodeMirror instance with
nth()is fragile. The PR objectives mention adding stable selectors for environment rows; consider using adata-testidon the value cell or a more semantic locator.🤖 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/collection-vars-draft-isolation/collection-vars-draft-isolation.spec.ts` at line 34, The valueCellEditor locator uses a fragile nth(1) selector that depends on element ordering. Replace the `.locator('.CodeMirror').nth(1)` approach with a more stable selector by adding a data-testid attribute to the value cell in the header row markup, then update the locator to target that specific test ID instead of relying on positional index.tests/scripting/bru-api/variable-persistence-typed/variable-persistence-typed.spec.ts (1)
31-55: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd
test.stepblocks for better test report clarity.The test combines multiple logical phases (file persistence checks, UI validation) without step breakdowns. Per guidelines, wrapping phases in
test.stepimproves generated test reports.✨ Example refactor structure
await test.step('Verify env vars persisted with `@number/`@boolean/@object annotations', async () => { await expect.poll(() => { const content = fs.readFileSync(envFilePath, 'utf8'); return ( /@number\s+envNum:\s*42/.test(content) && /@boolean\s+envBool:\s*true/.test(content) && /@object\s+envObj:/.test(content) && content.includes('"port"') && content.includes('3000') ); }, { timeout: PERSISTENCE_TIMEOUT }).toBe(true); }); // Similarly wrap the UI validation block (lines 57–73) in a test.step🤖 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/variable-persistence-typed/variable-persistence-typed.spec.ts` around lines 31 - 55, Ensure all logical test phases in the test file are wrapped in test.step blocks for better test report clarity. The diff shows two test.step blocks for file persistence checks, but there are additional phases (such as UI validation checks mentioned around lines 57-73) that are not yet wrapped in test.step blocks. Identify any remaining test logic phases that follow the existing test.step blocks and wrap them in appropriate test.step blocks with descriptive step names that clearly indicate what phase is being tested.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/utils/environments.js`:
- Around line 51-53: The applyScriptEnvVars function has two issues: first, it
assumes scriptVars is always an object and calls Object.keys on it at lines 52,
57, and 82, which will throw when scriptVars is null (as allowed by the new
dirty-flag contract); add null checks before these Object.keys calls to handle
null scriptVars. Second, the function uses the `in` operator (key in baseline)
on lines 59, 77, and 107, which incorrectly includes inherited prototype keys
like toString and constructor; replace these checks with
Object.prototype.hasOwnProperty.call(baseline, key) to only check own
properties.
In `@packages/bruno-cli/src/commands/run.js`:
- Around line 633-639: The `persistPaths.envFile` is being set to persist
environment variables that include CLI override values from `--env-var` flags,
which should not be written to disk. Before calling `persistVariableUpdates()`,
track which environment variable keys/sources came from CLI overrides separately
from the base envVars object, then ensure that when persisting variable updates,
only the non-CLI-overlay values are written to the persisted env file at
`persistPaths.envFile` to prevent temporary CLI-supplied values and secrets from
being stored on disk.
In `@packages/bruno-cli/tests/utils/persist-variables.spec.js`:
- Around line 12-20: Replace the shared tmpBase approach with per-test isolation
by modifying the beforeEach hook to generate a unique temporary directory for
each test using fs.mkdtempSync instead of a static path. Move the tmpBase
definition inside the beforeEach function and use fs.mkdtempSync with a prefix
to create a unique directory path for each test execution, then ensure the
afterEach hook still cleans up the dynamically created tmpBase. This prevents
parallel test execution from causing file system collisions.
In `@packages/bruno-electron/src/ipc/network/index.js`:
- Around line 1610-1614: The nested bru.runRequest() calls are not receiving the
parentRequestUid parameter, causing nested variable-update IPCs to generate
fresh UUIDs that get rejected by the renderer's request-UID gate. Pass the
requestUid that is created and queued in the 'main:run-folder-event' message as
the parentRequestUid parameter to any nested runRequest() method calls to ensure
the request context is maintained throughout nested script executions and
prevent script mutations from being dropped.
In
`@tests/environments/collection-vars-draft-merge-with-script/collection-vars-draft-merge-with-script.spec.ts`:
- Around line 1-62: In the test step that edits the existingCollVar, replace the
hardcoded page.waitForTimeout(500) call with an event-driven wait that monitors
an observable state change. Instead of waiting blindly for 500ms, use
expect.poll() to wait for the draft value to persist in the UI by checking that
locators.environment.variableValue('existingCollVar') contains the expected
'draft-edited-coll-value' text, similar to how it's done in the final
verification step. This ensures the test waits for the actual debounce behavior
rather than relying on a brittle timeout.
In
`@tests/environments/update-global-environment-via-script/global-env-update-via-script.spec.ts`:
- Around line 28-33: Replace the raw text selector page.getByText('Configure', {
exact: true }).click() in the test step "Open the Global Environment Config tab"
with the appropriate shared locator helper from the locators.environment object,
consistent with how other UI elements like globalTab() and globalEnvTab() are
being accessed in the same step. Identify the correct locator method for the
Configure button from the locators abstraction (likely
locators.environment.configureButton() or similar) and use that instead of the
raw text selector.
- Around line 24-26: The test step that clicks the sendButton does not wait for
the request to complete before returning, causing subsequent assertions to be
timing-sensitive. After clicking locators.request.sendButton(), add an explicit
wait for the request to complete using event-driven approaches such as waiting
for the network response or waiting for a specific UI element state change that
indicates the request has finished processing, rather than relying on arbitrary
timeouts.
In
`@tests/scripting/bru-api/global-env-var-persistence-typed/global-env-var-persistence-typed.spec.ts`:
- Around line 35-41: The test step 'Verify globalObj has dataType=object
containing the serialized JSON' currently only validates that the 'tier' and
'premium' fields are present in the serialized globalObj value, but it does not
verify that the 'limit' field is also persisted. Add an additional assertion
using the same pattern (await
expect(locators.environment.variableValue('globalObj')).toContainText('limit');)
after the existing field checks to ensure all persisted fields of the typed
object are properly validated.
- Around line 49-52: In the 'Close global environment config' test step, remove
the `force: true` parameter from the click action on the close icon element
obtained via getByTestId('request-tab-close-icon'). After the hover action on
globalEnvTab(), add an assertion to verify the close icon is visible before
clicking. After clicking the close icon, add an assertion to verify the global
environment tab is no longer visible, ensuring the cleanup step actually
validates the expected state rather than forcing the action.
In
`@tests/scripting/bru-api/variable-persistence-multi-request/variable-persistence-multi-request.spec.ts`:
- Around line 34-40: The current validation in the expect.poll callback uses
brittle .includes() checks that match substrings anywhere in the content without
ensuring exact key-value pairs, allowing false positives like counter: 12 to
pass. Replace the separate .includes() checks for 'counter:' with 'counter: 2'
and 'seenInReq2:' with 'seenInReq2: 1' with a more precise approach such as
regex patterns or structured parsing to validate the exact variable values as
complete key-value pairs rather than just checking for substring existence.
- Around line 14-16: The fixtures directory for this test uses the plural form
`collections` but should use the singular form `collection` since there is only
one collection directory. Rename the fixtures directory from
`fixtures/collections/variable-persistence-multi-request-test` to
`fixtures/collection/variable-persistence-multi-request-test` to comply with the
established naming convention where singular `collection` is used for a single
collection directory and plural `collections` is only used when multiple
collection directories exist within fixtures.
---
Nitpick comments:
In `@packages/bruno-app/src/utils/collections/index.js`:
- Around line 606-610: Add a regression test that validates the export
serialization behavior of environments when using the buildPersistedEnvVariables
helper with default settings. The test should verify that the variables in
exported environments (from the collection serialization code that calls
buildPersistedEnvVariables on env?.variables) correctly strips internal metadata
while preserving persisted values. This test ensures that future changes to
buildPersistedEnvVariables default behavior won't silently alter the export
output.
In `@packages/bruno-cli/tests/utils/persist-variables.spec.js`:
- Around line 160-173: The test currently relies on filesystem modification time
(mtime) comparison with a timing-dependent busy loop spin-wait, which is brittle
across platforms. Replace this approach by spying on the fs.writeFileSync method
to assert that no write occurs when persistVariableUpdates is called with
byte-identical content. Remove the timing-dependent busy loop and mtime
assertions, and instead verify that the write function is never called during
the persistVariableUpdates invocation, making the test deterministic and
platform-independent.
In `@packages/bruno-js/tests/runtime.spec.js`:
- Around line 229-239: The test case verifying null gating for untouched scopes
in the dirty-flag gating tests is incomplete. Add an assertion
`expect(result.runtimeVariables).toBeNull();` in both test cases (the one with
test description "should return null for scopes the script did not touch
(dirty-flag gating)" and its companion test case that also validates null
behavior for untouched scopes) to ensure that runtimeVariables is also properly
gated to null when the script does not modify it, alongside the existing
assertions for collectionVariables and globalEnvironmentVariables.
In `@tests/environments/api-setEnvVar/api-setEnvVar-default-persistence.spec.ts`:
- Line 8: The code is manually creating environment tab selectors using
substring matching with the hasText filter instead of using the centralized
collectionEnvTab() locator. Replace the manual selector assignments for the
envTab variable with a call to locators.environment.collectionEnvTab(), and
similarly replace the newEnvTab variable assignment with
newLocators.environment.collectionEnvTab(). This ensures consistent, exact-match
selectors across the test and prevents false positives from similar tab names.
In
`@tests/environments/collection-vars-draft-merge-with-script/collection-vars-draft-merge-with-script.spec.ts`:
- Around line 21-31: Replace the hardcoded `await page.waitForTimeout(500)` in
the 'Open collection settings and edit existingCollVar (create draft)' test step
with an event-driven wait that monitors for a visual indicator that the draft
save has completed, such as waiting for a pending save icon to disappear or
waiting for a stable UI state in the variable editor. If no UI indicator exists,
document why the hardcoded wait is necessary for the draft debounce and consider
increasing the timeout value to make the test more resilient to timing
variations under load.
In `@tests/environments/datatype-preservation/datatype-preservation.spec.ts`:
- Line 215: Replace the toHaveClass assertion for the importedItem element with
a more stable user-facing behavior assertion. Instead of asserting the active
CSS class using toHaveClass(/\bactive\b/), assert a stable semantic marker such
as an ARIA attribute (like aria-selected or aria-current) or a data-testid
attribute that indicates the selected state. This decouples the test from
styling internals and ensures the test verifies actual user-visible behavior
rather than implementation details, making it resilient to future styling
refactors.
In
`@tests/environments/draft-merge-with-script/fixtures/collections/draft-merge-test/set-env-var.bru`:
- Around line 1-23: The fixture directory is not following the naming convention
for single collections. The directory structure uses the plural form
fixtures/collections with a single collection, but per coding guidelines, it
should use the singular form fixtures/collection. Rename the parent directory
from collections to collection, then locate and update the path reference in the
init-user-data/preferences.json file to reflect the new singular directory name
while keeping the collection name (draft-merge-test) unchanged.
In
`@tests/environments/global-env-draft-merge-with-script/fixtures/collections/global-env-draft-merge-test/bruno.json`:
- Around line 1-9: The fixture directory for the global-env-draft-merge-test
collection uses the plural form "collections" when it should use the singular
form "collection" since this directory contains a single collection. Move the
global-env-draft-merge-test directory from under the plural "collections" parent
directory to a new singular "collection" parent directory to maintain consistent
naming conventions where singular "collection" is used for single collection
directories and plural "collections" would be used for multiple collections.
In
`@tests/scripting/bru-api/collection-vars-draft-isolation/collection-vars-draft-isolation.spec.ts`:
- Line 34: The valueCellEditor locator uses a fragile nth(1) selector that
depends on element ordering. Replace the `.locator('.CodeMirror').nth(1)`
approach with a more stable selector by adding a data-testid attribute to the
value cell in the header row markup, then update the locator to target that
specific test ID instead of relying on positional index.
In
`@tests/scripting/bru-api/variable-persistence-safe-mode/variable-persistence-safe-mode.spec.ts`:
- Around line 24-34: The click action on the close icon element with testId
'request-tab-close-icon' uses force: true which can mask timing issues. Remove
the force: true parameter from the getByTestId('request-tab-close-icon').click()
call and instead add an explicit wait or visibility check to ensure the close
icon is ready for interaction. Consider using page.waitForSelector() or a
visibility check on the close icon element before attempting to click it,
ensuring the tab is actually ready to be closed without forcing the interaction.
In
`@tests/scripting/bru-api/variable-persistence-typed/fixtures/collections/variable-persistence-typed-test/bruno.json`:
- Around line 1-9: The directory structure uses the plural form "collections"
but should use the singular form "collection" for consistency. Move the entire
fixture directory containing the bruno.json file and the
"variable-persistence-typed-test" collection from the "collections" parent
directory to a "collection" parent directory, ensuring all contents and file
structure within the collection directory remain intact during this
reorganization.
In
`@tests/scripting/bru-api/variable-persistence-typed/variable-persistence-typed.spec.ts`:
- Around line 31-55: Ensure all logical test phases in the test file are wrapped
in test.step blocks for better test report clarity. The diff shows two test.step
blocks for file persistence checks, but there are additional phases (such as UI
validation checks mentioned around lines 57-73) that are not yet wrapped in
test.step blocks. Identify any remaining test logic phases that follow the
existing test.step blocks and wrap them in appropriate test.step blocks with
descriptive step names that clearly indicate what phase is being tested.
🪄 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: d4663b87-5401-49ee-8c3f-69bf947031f0
📒 Files selected for processing (125)
packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/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/draft-collection-vars-isolation.spec.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/draft-collection-vars-merge.spec.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/draft-env-merge.spec.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.spec.jspackages/bruno-app/src/providers/ReduxStore/slices/global-environments.jspackages/bruno-app/src/providers/ReduxStore/slices/global-environments.spec.jspackages/bruno-app/src/utils/codemirror/autocomplete.jspackages/bruno-app/src/utils/collections/index.jspackages/bruno-app/src/utils/environments.jspackages/bruno-app/src/utils/environments.spec.jspackages/bruno-cli/src/commands/run.jspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-cli/src/utils/persist-variables.jspackages/bruno-cli/tests/utils/persist-variables.spec.jspackages/bruno-electron/src/ipc/ai/script-prompts.jspackages/bruno-electron/src/ipc/collection.jspackages/bruno-electron/src/ipc/network/index.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/collectionVar.spec.jspackages/bruno-js/tests/globalEnvVar.spec.jspackages/bruno-js/tests/runtime.spec.jspackages/bruno-js/tests/setEnvVar.spec.jspackages/bruno-tests/collection/ping.brupackages/bruno-tests/collection/scripting/api/bru/deleteAllCollectionVars.brupackages/bruno-tests/collection/scripting/api/bru/deleteAllEnvVars.brupackages/bruno-tests/collection/scripting/api/bru/deleteAllGlobalEnvVars.brupackages/bruno-tests/collection/scripting/api/bru/deleteCollectionVar.brupackages/bruno-tests/collection/scripting/api/bru/deleteEnvVar.brupackages/bruno-tests/collection/scripting/api/bru/deleteGlobalEnvVar.brupackages/bruno-tests/collection/scripting/api/bru/getAllCollectionVars.brupackages/bruno-tests/collection/scripting/api/bru/setCollectionVar.brupackages/bruno-tests/collection/scripting/api/bru/setGlobalEnvVar.brutests/environments/api-setEnvVar/api-setEnvVar-default-persistence.spec.tstests/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-default-persistence.brutests/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.brutests/environments/collection-vars-draft-merge-with-script/collection-vars-draft-merge-with-script.spec.tstests/environments/collection-vars-draft-merge-with-script/fixtures/collections/collection-vars-draft-merge-test/bruno.jsontests/environments/collection-vars-draft-merge-with-script/fixtures/collections/collection-vars-draft-merge-test/collection.brutests/environments/collection-vars-draft-merge-with-script/fixtures/collections/collection-vars-draft-merge-test/environments/Test.brutests/environments/collection-vars-draft-merge-with-script/fixtures/collections/collection-vars-draft-merge-test/set-collection-var.brutests/environments/collection-vars-draft-merge-with-script/init-user-data/collection-security.jsontests/environments/collection-vars-draft-merge-with-script/init-user-data/preferences.jsontests/environments/datatype-preservation/datatype-preservation.spec.tstests/environments/draft-merge-with-script/draft-merge-with-script.spec.tstests/environments/draft-merge-with-script/fixtures/collections/draft-merge-test/bruno.jsontests/environments/draft-merge-with-script/fixtures/collections/draft-merge-test/environments/Test.brutests/environments/draft-merge-with-script/fixtures/collections/draft-merge-test/set-env-var.brutests/environments/draft-merge-with-script/init-user-data/collection-security.jsontests/environments/draft-merge-with-script/init-user-data/preferences.jsontests/environments/global-env-draft-merge-with-script/fixtures/collections/global-env-draft-merge-test/bruno.jsontests/environments/global-env-draft-merge-with-script/fixtures/collections/global-env-draft-merge-test/set-global-env-var.brutests/environments/global-env-draft-merge-with-script/global-env-draft-merge-with-script.spec.tstests/environments/global-env-draft-merge-with-script/init-user-data/collection-security.jsontests/environments/global-env-draft-merge-with-script/init-user-data/global-environments.jsontests/environments/global-env-draft-merge-with-script/init-user-data/preferences.jsontests/environments/update-global-environment-via-script/global-env-update-via-script.spec.tstests/scripting/bru-api/collection-vars-draft-isolation/collection-vars-draft-isolation.spec.tstests/scripting/bru-api/collection-vars-draft-isolation/fixtures/collections/draft-isolation-test/bruno.jsontests/scripting/bru-api/collection-vars-draft-isolation/fixtures/collections/draft-isolation-test/collection.brutests/scripting/bru-api/collection-vars-draft-isolation/fixtures/collections/draft-isolation-test/environments/Test.brutests/scripting/bru-api/collection-vars-draft-isolation/fixtures/collections/draft-isolation-test/set-collection-var.brutests/scripting/bru-api/collection-vars-draft-isolation/init-user-data/collection-security.jsontests/scripting/bru-api/collection-vars-draft-isolation/init-user-data/preferences.jsontests/scripting/bru-api/global-env-var-persistence-typed/fixtures/collections/global-env-var-persistence-typed-test/bruno.jsontests/scripting/bru-api/global-env-var-persistence-typed/fixtures/collections/global-env-var-persistence-typed-test/set-typed-global-env-vars.brutests/scripting/bru-api/global-env-var-persistence-typed/global-env-var-persistence-typed.spec.tstests/scripting/bru-api/global-env-var-persistence-typed/init-user-data/collection-security.jsontests/scripting/bru-api/global-env-var-persistence-typed/init-user-data/global-environments.jsontests/scripting/bru-api/global-env-var-persistence-typed/init-user-data/preferences.jsontests/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/fixtures/collections/global-env-var-persistence-test/set-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-multi-request/fixtures/collections/variable-persistence-multi-request-test/bruno.jsontests/scripting/bru-api/variable-persistence-multi-request/fixtures/collections/variable-persistence-multi-request-test/environments/Test.brutests/scripting/bru-api/variable-persistence-multi-request/fixtures/collections/variable-persistence-multi-request-test/req-1-write.brutests/scripting/bru-api/variable-persistence-multi-request/fixtures/collections/variable-persistence-multi-request-test/req-2-read-and-write.brutests/scripting/bru-api/variable-persistence-multi-request/init-user-data/collection-security.jsontests/scripting/bru-api/variable-persistence-multi-request/init-user-data/preferences.jsontests/scripting/bru-api/variable-persistence-multi-request/variable-persistence-multi-request.spec.tstests/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-typed-safe-mode/fixtures/collections/variable-persistence-typed-safe-test/bruno.jsontests/scripting/bru-api/variable-persistence-typed-safe-mode/fixtures/collections/variable-persistence-typed-safe-test/environments/Test.brutests/scripting/bru-api/variable-persistence-typed-safe-mode/fixtures/collections/variable-persistence-typed-safe-test/set-typed-collection-vars.brutests/scripting/bru-api/variable-persistence-typed-safe-mode/fixtures/collections/variable-persistence-typed-safe-test/set-typed-env-vars.brutests/scripting/bru-api/variable-persistence-typed-safe-mode/init-user-data/collection-security.jsontests/scripting/bru-api/variable-persistence-typed-safe-mode/init-user-data/preferences.jsontests/scripting/bru-api/variable-persistence-typed-safe-mode/variable-persistence-typed-safe-mode.spec.tstests/scripting/bru-api/variable-persistence-typed/fixtures/collections/variable-persistence-typed-test/bruno.jsontests/scripting/bru-api/variable-persistence-typed/fixtures/collections/variable-persistence-typed-test/environments/Test.brutests/scripting/bru-api/variable-persistence-typed/fixtures/collections/variable-persistence-typed-test/set-typed-collection-vars.brutests/scripting/bru-api/variable-persistence-typed/fixtures/collections/variable-persistence-typed-test/set-typed-env-vars.brutests/scripting/bru-api/variable-persistence-typed/init-user-data/collection-security.jsontests/scripting/bru-api/variable-persistence-typed/init-user-data/preferences.jsontests/scripting/bru-api/variable-persistence-typed/variable-persistence-typed.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.tstests/variable-datatypes/parsed-from-fixture.spec.ts
💤 Files with no reviewable changes (5)
- tests/environments/api-setEnvVar/api-setEnvVar-without-persist.spec.ts
- packages/bruno-electron/src/ipc/ai/script-prompts.js
- packages/bruno-tests/collection/scripting/api/bru/getAllCollectionVars.bru
- packages/bruno-tests/collection/scripting/api/bru/setCollectionVar.bru
- packages/bruno-app/src/providers/ReduxStore/slices/collections/index.spec.js
Comment on lines
+51
to
+53
| export const applyScriptEnvVars = (variables, scriptVars, baseline, { skipKeys = [] } = {}) => { | ||
| const scriptVarNames = new Set(Object.keys(scriptVars)); | ||
| const skip = new Set(skipKeys); |
Contributor
There was a problem hiding this comment.
Harden baseline merge against null inputs and inherited-key false matches
Line 52/57/82 assume scriptVars is always an object; with the new dirty-flag contract, unchanged scopes can be null, which throws at runtime (Object.keys/null). Also, Lines 59, 77, and 107 use key in baseline, which includes prototype keys and can produce incorrect merge/delete behavior for names like toString or constructor.
Suggested fix
export const applyScriptEnvVars = (variables, scriptVars, baseline, { skipKeys = [] } = {}) => {
- const scriptVarNames = new Set(Object.keys(scriptVars));
+ const normalizedScriptVars = scriptVars || {};
+ const scriptVarNames = new Set(Object.keys(normalizedScriptVars));
const skip = new Set(skipKeys);
const next = (variables || []).map((v) => ({ ...v }));
if (baseline) {
- Object.entries(scriptVars).forEach(([key, value]) => {
+ Object.entries(normalizedScriptVars).forEach(([key, value]) => {
if (skip.has(key)) return;
- const isNew = !(key in baseline);
+ const isNew = !Object.hasOwn(baseline, key);
const isModified = !isNew && !isEqual(baseline[key], value);
if (isNew || isModified) {
const existing = next.find((v) => v.name === key && v.enabled);
if (existing) {
existing.value = value;
} else {
next.push({ uid: uuid(), name: key, value, type: 'text', secret: false, enabled: true });
}
}
});
return next.filter((v) => {
if (!v.enabled) return true;
- if (v.name in baseline && !scriptVarNames.has(v.name)) return false;
+ if (Object.hasOwn(baseline, v.name) && !scriptVarNames.has(v.name)) return false;
return true;
});
}
- Object.entries(scriptVars).forEach(([key, value]) => {
+ Object.entries(normalizedScriptVars).forEach(([key, value]) => {
if (skip.has(key)) return;
const existing = next.find((v) => v.name === key && v.enabled);
if (existing) {
existing.value = value;
} else {
next.push({ uid: uuid(), name: key, value, type: 'text', secret: false, enabled: true });
}
});
return next.filter((v) => !v.enabled || scriptVarNames.has(v.name));
};
export const getScriptModifiedKeys = (scriptVars, baseline, { skipKeys = [] } = {}) => {
const skip = new Set(skipKeys);
const out = new Set();
Object.entries(scriptVars || {}).forEach(([key, value]) => {
if (skip.has(key)) return;
if (baseline) {
- const isNew = !(key in baseline);
+ const isNew = !Object.hasOwn(baseline, key);
if (!isNew && isEqual(baseline[key], value)) return;
}
out.add(key);
});
return out;
};Also applies to: 57-63, 75-79, 82-84, 101-108
🤖 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/environments.js` around lines 51 - 53, The
applyScriptEnvVars function has two issues: first, it assumes scriptVars is
always an object and calls Object.keys on it at lines 52, 57, and 82, which will
throw when scriptVars is null (as allowed by the new dirty-flag contract); add
null checks before these Object.keys calls to handle null scriptVars. Second,
the function uses the `in` operator (key in baseline) on lines 59, 77, and 107,
which incorrectly includes inherited prototype keys like toString and
constructor; replace these checks with
Object.prototype.hasOwnProperty.call(baseline, key) to only check own
properties.
Comment on lines
+1610
to
+1614
| const requestUid = uuid(); | ||
|
|
||
| mainWindow.webContents.send('main:run-folder-event', { | ||
| type: 'request-queued', | ||
| requestUid, |
Contributor
There was a problem hiding this comment.
Forward the runner request UID into nested bru.runRequest() calls.
The runner queues an outer requestUid, but the runner-local nested runRequest() call still omits parentRequestUid. Nested variable-update IPCs then use a fresh UUID and can be rejected by the renderer request-UID gate, dropping script mutations from bru.runRequest() during folder runs.
Suggested fix
// Tracks the outer runner item currently executing so a nested bru.runRequest
// can route its oauth2 timeline entry back to this item.
let currentRunnerEventData = null;
+ let currentRunnerRequestUid = null;
@@
runtimeVariables,
runInBackground: true,
parentExecutionMode: 'runner',
- parentRunnerEventData: currentRunnerEventData
+ parentRunnerEventData: currentRunnerEventData,
+ parentRequestUid: currentRunnerRequestUid
});
@@
const requestUid = uuid();
+ currentRunnerRequestUid = requestUid;🤖 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-electron/src/ipc/network/index.js` around lines 1610 - 1614,
The nested bru.runRequest() calls are not receiving the parentRequestUid
parameter, causing nested variable-update IPCs to generate fresh UUIDs that get
rejected by the renderer's request-UID gate. Pass the requestUid that is created
and queued in the 'main:run-folder-event' message as the parentRequestUid
parameter to any nested runRequest() method calls to ensure the request context
is maintained throughout nested script executions and prevent script mutations
from being dropped.
Comment on lines
+1
to
+62
| import { test, expect } from '../../../playwright'; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import { openCollection, sendRequest, selectEnvironment } from '../../utils/page'; | ||
| import { buildCommonLocators } from '../../utils/page/locators'; | ||
|
|
||
| const selectAllShortcut = process.platform === 'darwin' ? 'Meta+a' : 'Control+a'; | ||
|
|
||
| const PERSISTENCE_TIMEOUT = 10000; | ||
|
|
||
| test.describe('Collection variables draft merge with script-set variables', () => { | ||
| test('preserves unsaved draft edits when script sets a new collection variable', async ({ | ||
| pageWithUserData: page, | ||
| collectionFixturePath | ||
| }) => { | ||
| const locators = buildCommonLocators(page); | ||
|
|
||
| await openCollection(page, 'collection-vars-draft-merge-test'); | ||
| await selectEnvironment(page, 'Test'); | ||
|
|
||
| await test.step('Open collection settings and edit existingCollVar (create draft)', async () => { | ||
| await locators.sidebar.collection('collection-vars-draft-merge-test').click(); | ||
| await locators.paneTabs.collectionSettingsTab('vars').click(); | ||
| await expect(locators.environment.variableRowByName('existingCollVar')).toBeVisible(); | ||
| await locators.environment.variableValue('existingCollVar').click(); | ||
| await page.keyboard.press(selectAllShortcut); | ||
| await page.keyboard.type('draft-edited-coll-value'); | ||
|
|
||
| // Wait for draft debounce | ||
| await page.waitForTimeout(500); | ||
| }); | ||
|
|
||
| await test.step('Open request and send it', async () => { | ||
| await locators.sidebar.request('set-collection-var').click(); | ||
| await expect(locators.tabs.requestTab('set-collection-var')).toBeVisible(); | ||
| await sendRequest(page, 200); | ||
| }); | ||
|
|
||
| await test.step('Verify draft edit and script var in collection settings UI', async () => { | ||
| await locators.sidebar.collection('collection-vars-draft-merge-test').click(); | ||
| await locators.paneTabs.collectionSettingsTab('vars').click(); | ||
|
|
||
| await expect(locators.environment.variableRowByName('existingCollVar')).toBeVisible(); | ||
| await expect(locators.environment.variableValue('existingCollVar')).toContainText('draft-edited-coll-value'); | ||
|
|
||
| await expect(locators.environment.variableRowByName('scriptCollVar')).toBeVisible(); | ||
| await expect(locators.environment.variableValue('scriptCollVar')).toContainText('from-script-789'); | ||
| }); | ||
|
|
||
| await test.step('Verify script var persisted to collection.bru', async () => { | ||
| const collectionBruPath = path.join( | ||
| collectionFixturePath!, | ||
| 'collection-vars-draft-merge-test', | ||
| 'collection.bru' | ||
| ); | ||
| await expect.poll(() => { | ||
| const content = fs.readFileSync(collectionBruPath, 'utf8'); | ||
| return content.includes('scriptCollVar') && content.includes('from-script-789'); | ||
| }, { timeout: PERSISTENCE_TIMEOUT }).toBe(true); | ||
| }); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify buildCommonLocators exports all required methods
# Check locators.ts exists and exports buildCommonLocators
rg -A30 'export.*buildCommonLocators' tests/utils/page/locators.ts
# Check that all locator methods used in this test are defined
rg -n 'variableRowByName|variableValue|collectionSettingsTab|requestTab' tests/utils/page/locators.ts
# Verify page helper exports
rg -n 'export.*openCollection|export.*selectEnvironment|export.*sendRequest' tests/utils/page/index.tsRepository: usebruno/bruno
Length of output: 2584
🏁 Script executed:
#!/bin/bash
# Verify page helper functions are exported
rg 'export.*openCollection|export.*selectEnvironment|export.*sendRequest' tests/utils/page/
# Check the fixture definition for collectionFixturePath uniqueness
rg -A5 'collectionFixturePath' tests/playwright.ts
# Verify the locators methods more comprehensively
rg -B2 -A2 'paneTabs|environment' tests/utils/page/locators.tsRepository: usebruno/bruno
Length of output: 2177
🏁 Script executed:
#!/bin/bash
# Find where fixtures are defined
fd 'playwright' tests/ -type f | head -20
# Search for the exact helper functions
rg 'export.*openCollection[^F]|export.*selectEnvironment|export.*sendRequest' tests/
# Check if these might be in index.ts
cat tests/utils/page/index.ts | head -50
# Check for fixture definitions
rg 'pageWithUserData|collectionFixturePath' tests/ -B2 -A2Repository: usebruno/bruno
Length of output: 50560
🏁 Script executed:
#!/bin/bash
# Find fixture definitions
fd 'fixture|conftest' tests/ -type f
# Look for the specific test file to understand structure
head -100 tests/environments/collection-vars-draft-merge-with-script/collection-vars-draft-merge-with-script.spec.ts
# Find where collectionFixturePath comes from
rg 'collectionFixturePath' tests/ -B5 | head -50
# Find helper function implementations
rg 'export const openCollection' tests/ -A10 | head -40
rg 'export const selectEnvironment' tests/ -A10 | head -40
rg 'export const sendRequest' tests/ -A10 | head -40Repository: usebruno/bruno
Length of output: 10243
🏁 Script executed:
#!/bin/bash
# Find playwright config
find tests/ -name "*.config.*" -o -name "*.ts" | grep -E "(playwright|fixture)" | head -20
# Check the main utils/page exports
cat tests/utils/page/index.ts
# Look for fixture definitions
rg 'collectionFixturePath|pageWithUserData' tests/ -l | head -5
# Get one file to understand the fixture pattern
head -50 tests/utils/page/mounting.ts
# Search for the specific helpers
rg 'export.*openCollection[^F]' tests/utils/page/ -A3Repository: usebruno/bruno
Length of output: 2186
🏁 Script executed:
#!/bin/bash
# Look at fixtures/index.ts to find fixture definitions
cat tests/utils/fixtures/index.ts
# Search for openCollection definition
rg 'const openCollection|function openCollection' tests/ -A5
# Look for selectEnvironment
rg 'const selectEnvironment|function selectEnvironment' tests/ -A5
# Look for sendRequest
rg 'const sendRequest|function sendRequest' tests/ -A5Repository: usebruno/bruno
Length of output: 6299
🏁 Script executed:
#!/bin/bash
# Get full implementations of the helper functions
rg 'export.*openCollection[^F]|export.*selectEnvironment|export.*sendRequest' tests/utils/page/actions.ts -A15
# Check if they're exported from index.ts
cat tests/utils/page/actions.ts | grep -E "^export"
# Find fixture definitions for pageWithUserData and collectionFixturePath
cat tests/utils/fixtures/index.ts
# Check if there's a playwright config
cat tests/playwright.config.ts 2>/dev/null || cat playwright.config.ts 2>/dev/null || echo "No config found in expected locations"Repository: usebruno/bruno
Length of output: 1764
🏁 Script executed:
#!/bin/bash
# Look for fixture definitions - they might be in a different location
find tests/ -name "*.ts" -exec grep -l "pageWithUserData\|collectionFixturePath" {} \; | xargs grep -l "fixture\|defineProperty" | head -5
# Check the playwright custom fixture file
cat tests/playwright.ts 2>/dev/null || cat tests/playwright/index.ts 2>/dev/null || find tests -name "*.ts" | xargs grep -l "test\.extend" | head -1 | xargs cat
# Get the full actions exports
tail -100 tests/utils/page/actions.ts
# Check generator.ts for fixture definitions
head -150 tests/utils/fixtures/generator.tsRepository: usebruno/bruno
Length of output: 6675
🏁 Script executed:
#!/bin/bash
# Find where fixtures are defined - look for test.extend
rg 'test\.extend' tests/ -A20 | head -100
# Also check for exports related to fixtures
rg 'export.*fixture|export.*pageWithUserData' tests/ -B2 -A2
# Look in the playwright file that's imported in the test
cat tests/playwright.ts 2>/dev/null || ls -la tests/*.tsRepository: usebruno/bruno
Length of output: 4156
🏁 Script executed:
#!/bin/bash
# Find the playwright module that's being imported
find tests -name "*.ts" | xargs grep -l "import.*playwright" | head -3 | xargs head -20
# Look for test.extend definitions
rg 'export.*test.*=|test\.extend' tests/ -B5 -A15 | head -150
# Check if there's a playwright index or config
find . -path ./node_modules -prune -o -name "playwright.ts" -o -name "playwright" -type d | grep -v node_modulesRepository: usebruno/bruno
Length of output: 2094
🏁 Script executed:
#!/bin/bash
# Found it! Check the playwright module
cat ./playwright.ts 2>/dev/null || cat ./playwright/index.ts 2>/dev/null || ls -la ./playwright* 2>/dev/null || find . -path ./node_modules -prune -o -name "playwright*" -type f -o -name "playwright" -type d | head -20Repository: usebruno/bruno
Length of output: 16282
Replace hardcoded timeout with event-driven wait for draft persistence.
The fixtures, locators, and helper functions are all properly implemented and isolated. However, line 32 uses page.waitForTimeout(500) to wait for draft debounce, which violates e2e best practices. Replace with a wait for observable state change—for example, wait for the variable value to persist to the UI or monitor for the debounce handler to complete via a locator state check.
Why this matters
Hardcoded timeouts are brittle and can cause flaky tests if the actual debounce timing varies. Event-driven waits are deterministic and fail clearly when behaviour doesn't happen as expected.🤖 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/collection-vars-draft-merge-with-script/collection-vars-draft-merge-with-script.spec.ts`
around lines 1 - 62, In the test step that edits the existingCollVar, replace
the hardcoded page.waitForTimeout(500) call with an event-driven wait that
monitors an observable state change. Instead of waiting blindly for 500ms, use
expect.poll() to wait for the draft value to persist in the UI by checking that
locators.environment.variableValue('existingCollVar') contains the expected
'draft-edited-coll-value' text, similar to how it's done in the final
verification step. This ensures the test waits for the actual debounce behavior
rather than relying on a brittle timeout.
Comment on lines
28
to
33
| await test.step('Open the Global Environment Config tab', async () => { | ||
| await page.getByTestId('environment-selector-trigger').click(); | ||
| await page.getByTestId('env-tab-global').click(); | ||
| await locators.environment.selector().click(); | ||
| await locators.environment.globalTab().click(); | ||
| await page.getByText('Configure', { exact: true }).click(); | ||
|
|
||
| const envTab = page.locator('.request-tab').filter({ hasText: 'Global Environments' }); | ||
| await expect(envTab).toBeVisible(); | ||
| await expect(locators.environment.globalEnvTab()).toBeVisible(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Use the shared locator helper for the Configure action.
Using a raw text selector here is more brittle than the existing locator abstraction already used in nearby specs.
Suggested fix
- await page.getByText('Configure', { exact: true }).click();
+ await locators.environment.configureButton().click();As per coding guidelines, “Use locator variables for locators.”
🤖 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/update-global-environment-via-script/global-env-update-via-script.spec.ts`
around lines 28 - 33, Replace the raw text selector page.getByText('Configure',
{ exact: true }).click() in the test step "Open the Global Environment Config
tab" with the appropriate shared locator helper from the locators.environment
object, consistent with how other UI elements like globalTab() and
globalEnvTab() are being accessed in the same step. Identify the correct locator
method for the Configure button from the locators abstraction (likely
locators.environment.configureButton() or similar) and use that instead of the
raw text selector.
Source: Coding guidelines
Comment on lines
+35
to
+41
| await test.step('Verify globalObj has dataType=object containing the serialized JSON', async () => { | ||
| const row = locators.environment.variableRowByName('globalObj'); | ||
| await expect(row).toBeVisible(); | ||
| await expect(locators.dataTypeSelector.typeLabel(row)).toHaveText('object'); | ||
| await expect(locators.environment.variableValue('globalObj')).toContainText('tier'); | ||
| await expect(locators.environment.variableValue('globalObj')).toContainText('premium'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Strengthen the object-value assertion to cover all persisted fields.
This step can still pass if globalObj.limit is dropped or malformed, which weakens regression detection for typed persistence.
Suggested patch
await test.step('Verify globalObj has dataType=object containing the serialized JSON', async () => {
const row = locators.environment.variableRowByName('globalObj');
+ const value = locators.environment.variableValue('globalObj');
await expect(row).toBeVisible();
await expect(locators.dataTypeSelector.typeLabel(row)).toHaveText('object');
- await expect(locators.environment.variableValue('globalObj')).toContainText('tier');
- await expect(locators.environment.variableValue('globalObj')).toContainText('premium');
+ await expect(value).toContainText('tier');
+ await expect(value).toContainText('premium');
+ await expect(value).toContainText('limit');
+ await expect(value).toContainText('1000');
});As per coding guidelines, 'Aim for tests that fail usefully' and 'Prioritise high-value tests over maximum coverage.'
🤖 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-typed/global-env-var-persistence-typed.spec.ts`
around lines 35 - 41, The test step 'Verify globalObj has dataType=object
containing the serialized JSON' currently only validates that the 'tier' and
'premium' fields are present in the serialized globalObj value, but it does not
verify that the 'limit' field is also persisted. Add an additional assertion
using the same pattern (await
expect(locators.environment.variableValue('globalObj')).toContainText('limit');)
after the existing field checks to ensure all persisted fields of the typed
object are properly validated.
Source: Coding guidelines
Comment on lines
+49
to
+52
| await test.step('Close global environment config', async () => { | ||
| await locators.environment.globalEnvTab().hover(); | ||
| await locators.environment.globalEnvTab().getByTestId('request-tab-close-icon').click({ force: true }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "**/global-env-var-persistence-typed.spec.ts" -type fRepository: usebruno/bruno
Length of output: 158
🏁 Script executed:
cat -n ./tests/scripting/bru-api/global-env-var-persistence-typed/global-env-var-persistence-typed.spec.ts | sed -n '45,60p'Repository: usebruno/bruno
Length of output: 513
🏁 Script executed:
cat -n ./tests/scripting/bru-api/global-env-var-persistence-typed/global-env-var-persistence-typed.spec.ts | head -80Repository: usebruno/bruno
Length of output: 3171
🏁 Script executed:
rg "force:\s*true" tests/ --type ts -B 2 -A 2 | head -50Repository: usebruno/bruno
Length of output: 3310
🏁 Script executed:
rg "request-tab-close-icon|closeTab" tests/ --type ts -B 1 -A 1 | head -60Repository: usebruno/bruno
Length of output: 4648
Remove force: true and add explicit visibility/cleanup assertions.
The close icon should be visible after hover, and the tab should be verified hidden after clicking. This aligns with parallel best practices in the test suite and ensures the cleanup step actually validates the expected state.
Suggested patch
await test.step('Close global environment config', async () => {
+ const closeIcon = locators.environment.globalEnvTab().getByTestId('request-tab-close-icon');
await locators.environment.globalEnvTab().hover();
- await locators.environment.globalEnvTab().getByTestId('request-tab-close-icon').click({ force: true });
+ await expect(closeIcon).toBeVisible();
+ await closeIcon.click();
+ await expect(locators.environment.globalEnvTab()).not.toBeVisible();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await test.step('Close global environment config', async () => { | |
| await locators.environment.globalEnvTab().hover(); | |
| await locators.environment.globalEnvTab().getByTestId('request-tab-close-icon').click({ force: true }); | |
| }); | |
| await test.step('Close global environment config', async () => { | |
| const closeIcon = locators.environment.globalEnvTab().getByTestId('request-tab-close-icon'); | |
| await locators.environment.globalEnvTab().hover(); | |
| await expect(closeIcon).toBeVisible(); | |
| await closeIcon.click(); | |
| await expect(locators.environment.globalEnvTab()).not.toBeVisible(); | |
| }); |
🤖 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-typed/global-env-var-persistence-typed.spec.ts`
around lines 49 - 52, In the 'Close global environment config' test step, remove
the `force: true` parameter from the click action on the close icon element
obtained via getByTestId('request-tab-close-icon'). After the hover action on
globalEnvTab(), add an assertion to verify the close icon is visible before
clicking. After clicking the close icon, add an assertion to verify the global
environment tab is no longer visible, ensuring the cleanup step actually
validates the expected state rather than forcing the action.
Source: Coding guidelines
Comment on lines
+14
to
+16
| await openCollection(page, 'variable-persistence-multi-request-test'); | ||
| await selectEnvironment(page, 'Test'); | ||
| await runCollection(page, 'variable-persistence-multi-request-test'); |
Contributor
There was a problem hiding this comment.
Fixture directory should use singular collection not plural collections.
Per established naming convention (verified in CI), use fixtures/collection when there is a single collection directory, and fixtures/collections only when multiple collection dirs exist within fixtures. This test has one collection, so the path should be: tests/scripting/bru-api/variable-persistence-multi-request/fixtures/collection/variable-persistence-multi-request-test/...
Rename the directory structure to match the convention.
🤖 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/variable-persistence-multi-request/variable-persistence-multi-request.spec.ts`
around lines 14 - 16, The fixtures directory for this test uses the plural form
`collections` but should use the singular form `collection` since there is only
one collection directory. Rename the fixtures directory from
`fixtures/collections/variable-persistence-multi-request-test` to
`fixtures/collection/variable-persistence-multi-request-test` to comply with the
established naming convention where singular `collection` is used for a single
collection directory and plural `collections` is only used when multiple
collection directories exist within fixtures.
Source: Learnings
Comment on lines
+34
to
+40
| await expect.poll(() => { | ||
| const content = fs.readFileSync(envFilePath, 'utf8'); | ||
| // Must contain counter=2 (the last write wins) and seenInReq2=1 (request 2 observed request 1). | ||
| return content.includes('counter:') && content.includes('2') | ||
| && content.includes('seenInReq2:') && content.includes('1'); | ||
| }, { timeout: PERSISTENCE_TIMEOUT }).toBe(true); | ||
| }); |
Contributor
There was a problem hiding this comment.
Use structured parsing instead of brittle substring matching for BRU file validation.
The .includes() checks (lines 37–38) are fragile: they confirm substrings exist but not that variables have correct exact values. For example, content.includes('2') after checking for 'counter:' could match counter: 12 or other unexpected values.
Instead, parse the BRU file's variable block or use a more precise regex/pattern to confirm counter: 2 and seenInReq2: 1 as complete key-value pairs.
🤖 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/variable-persistence-multi-request/variable-persistence-multi-request.spec.ts`
around lines 34 - 40, The current validation in the expect.poll callback uses
brittle .includes() checks that match substrings anywhere in the content without
ensuring exact key-value pairs, allowing false positives like counter: 12 to
pass. Replace the separate .includes() checks for 'counter:' with 'counter: 2'
and 'seenInReq2:' with 'seenInReq2: 1' with a more precise approach such as
regex patterns or structured parsing to validate the exact variable values as
complete key-value pairs rather than just checking for substring existence.
…variable row expectations - Eliminated comments related to state sync and inference issues to enhance code clarity. - Adjusted expectations for environment variable row rendering in tests, focusing on relevant assertions.
…in environments - Introduced new test cases to validate the preservation of secret variables when updated via scripts in both collection and global environments. - Implemented tests to ensure that secret values are encrypted before storage and can be correctly decrypted for subsequent requests. - Added fixtures and environment configurations for testing secret variable behavior in both bru and yml formats. - Enhanced utility functions for managing environment configurations and interactions within the test suite.
…le persistence - Updated MultiLineEditor and SingleLineEditor components to include data-testid for secret reveal toggle buttons, improving testability. - Introduced new tests for global environment variable persistence, ensuring non-secret variables survive app restarts and are correctly interpolated. - Added fixtures for workspace and collections to support the new global variable tests, enhancing the overall test coverage for environment management. - Refactored utility functions to streamline interactions with environment variables in tests.
- Simplified the persistence logic for active environments by directly constructing the environment copy, reducing unnecessary cloning. - Updated the collection saving process to utilize the fresh collection state, ensuring accurate data is saved without drafts. - Enhanced error handling during the save operations to improve reliability and maintainability.
sanish-bruno
force-pushed
the
feat/varianble-persistence-cli
branch
from
d4882a2 to
7917352
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-cli/src/commands/run.js (1)
366-390: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore
.yamlhandling for environment file parsing.Line 369 and Line 386 only recognize
.yml. A--env-fileending in.yamlnow falls through tobruparsing and can fail even when the file is valid YAML.Minimal patch
const resolveEnvFileFormat = (filePath) => { const ext = path.extname(filePath).toLowerCase(); if (ext === '.json') return 'json'; - if (ext === '.yml') return 'yml'; + if (ext === '.yml' || ext === '.yaml') return 'yml'; return 'bru'; }; ... - } else if (fileExt === '.yml') { + } else if (fileExt === '.yml' || fileExt === '.yaml') { const content = fs.readFileSync(filePath, 'utf8'); const envJson = parseEnvironment(content, { format: 'yml' }); result = getEnvVars(envJson); - result.__name__ = nameOverride || path.basename(filePath, '.yml'); + result.__name__ = nameOverride || path.basename(filePath, fileExt); } else {🤖 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-cli/src/commands/run.js` around lines 366 - 390, The code only checks for the .yml extension but not .yaml, causing .yaml files to fall through to bru format parsing and fail. Update both the resolveEnvFileFormat function (which currently checks ext === '.yml') and the loadEnvFromFile function (which currently checks fileExt === '.yml') to also handle the .yaml extension. Modify these conditions to recognize both .yml and .yaml as valid YAML file formats so they are parsed correctly instead of defaulting to bru format.
🤖 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-electron/tests/store/env-secrets.spec.js`:
- Line 6: The collectionPath variable is hardcoded with a Unix-specific '/tmp'
path which will fail on Windows. Replace the hardcoded '/tmp/test-collection'
with a cross-platform solution by using os.tmpdir() combined with path.join() to
construct the path. Make sure to import the os and path modules at the top of
the file if they are not already imported.
In
`@tests/environments/api-setGlobalEnvVar-secret/api-setGlobalEnvVar-secret.spec.ts`:
- Around line 180-186: Replace the non-null assertion operator (!) on the secret
variable with explicit existence checks to provide clearer test failure
messages. In the test blocks around findApiTokenSecret calls, add expect
assertions to verify that the secret object exists before dereferencing its
value property (such as using expect(secret).toBeTruthy() or
expect(secret).toBeDefined()). This should be done in the assertion blocks where
secret!.value is currently accessed without checking if secret is null or
undefined, and the same pattern should be applied to the other occurrences
mentioned at lines 267-269 and 283-285.
- Around line 22-25: The findApiTokenSecret function currently uses strict path
equality after normalization, which fails on Windows when paths have different
casing. Modify the comparison logic to convert both paths to absolute paths
using path.resolve and then perform a case-insensitive comparison on win32
platforms. Check if process.platform is 'win32' and if so, compare the
lowercased versions of the absolute paths; otherwise use case-sensitive
comparison. This ensures the path matching works correctly across Windows and
Unix-like systems with varying path casing conventions.
---
Outside diff comments:
In `@packages/bruno-cli/src/commands/run.js`:
- Around line 366-390: The code only checks for the .yml extension but not
.yaml, causing .yaml files to fall through to bru format parsing and fail.
Update both the resolveEnvFileFormat function (which currently checks ext ===
'.yml') and the loadEnvFromFile function (which currently checks fileExt ===
'.yml') to also handle the .yaml extension. Modify these conditions to recognize
both .yml and .yaml as valid YAML file formats so they are parsed correctly
instead of defaulting to bru format.
🪄 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: 40d54b8c-90bf-43d0-96ff-fb6c519f3cf4
📒 Files selected for processing (43)
packages/bruno-app/src/components/MultiLineEditor/index.jspackages/bruno-app/src/components/SingleLineEditor/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/utils/environments.spec.jspackages/bruno-cli/src/commands/run.jspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-cli/src/utils/persist-variables.jspackages/bruno-cli/tests/utils/persist-variables.spec.jspackages/bruno-electron/tests/store/env-secrets.spec.jstests/environments/api-setEnvVar-secret/api-setEnvVar-secret.spec.tstests/environments/api-setEnvVar-secret/fixtures/collections/bru/bruno.jsontests/environments/api-setEnvVar-secret/fixtures/collections/bru/environments/Local.brutests/environments/api-setEnvVar-secret/fixtures/collections/bru/read-secret.brutests/environments/api-setEnvVar-secret/fixtures/collections/bru/seed-secret.brutests/environments/api-setEnvVar-secret/fixtures/collections/bru/set-secret.brutests/environments/api-setEnvVar-secret/fixtures/collections/yml/environments/Local.ymltests/environments/api-setEnvVar-secret/fixtures/collections/yml/opencollection.ymltests/environments/api-setEnvVar-secret/fixtures/collections/yml/read-secret.ymltests/environments/api-setEnvVar-secret/fixtures/collections/yml/seed-secret.ymltests/environments/api-setEnvVar-secret/fixtures/collections/yml/set-secret.ymltests/environments/api-setEnvVar-secret/init-user-data/collection-security.jsontests/environments/api-setEnvVar-secret/init-user-data/preferences.jsontests/environments/api-setEnvVar/api-setEnvVar-default-persistence.spec.tstests/environments/api-setEnvVar/multiple-persist-vars.spec.tstests/environments/api-setGlobalEnvVar-secret/api-setGlobalEnvVar-secret.spec.tstests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/collections/test-collection/bruno.jsontests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/collections/test-collection/read-global-secret.brutests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/collections/test-collection/seed-global-secret.brutests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/collections/test-collection/set-global-secret.brutests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/environments/Local.ymltests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/workspace.ymltests/environments/api-setGlobalEnvVar-secret/init-user-data/collection-security.jsontests/environments/api-setGlobalEnvVar-secret/init-user-data/preferences.jsontests/environments/api-setGlobalEnvVar/api-setGlobalEnvVar-restart.spec.tstests/environments/api-setGlobalEnvVar/fixtures/workspace/collections/test-collection/bruno.jsontests/environments/api-setGlobalEnvVar/fixtures/workspace/collections/test-collection/read-global-var.brutests/environments/api-setGlobalEnvVar/fixtures/workspace/collections/test-collection/set-global-var.brutests/environments/api-setGlobalEnvVar/fixtures/workspace/environments/Local.ymltests/environments/api-setGlobalEnvVar/fixtures/workspace/workspace.ymltests/environments/api-setGlobalEnvVar/init-user-data/collection-security.jsontests/environments/api-setGlobalEnvVar/init-user-data/preferences.jsontests/utils/page/actions.tstests/utils/page/locators.ts
✅ Files skipped from review due to trivial changes (22)
- tests/environments/api-setGlobalEnvVar/fixtures/workspace/collections/test-collection/bruno.json
- tests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/collections/test-collection/read-global-secret.bru
- tests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/collections/test-collection/bruno.json
- tests/environments/api-setGlobalEnvVar/fixtures/workspace/environments/Local.yml
- tests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/collections/test-collection/set-global-secret.bru
- tests/environments/api-setEnvVar-secret/fixtures/collections/yml/set-secret.yml
- tests/environments/api-setGlobalEnvVar/fixtures/workspace/collections/test-collection/read-global-var.bru
- tests/environments/api-setEnvVar-secret/fixtures/collections/bru/set-secret.bru
- tests/environments/api-setGlobalEnvVar-secret/init-user-data/collection-security.json
- tests/environments/api-setGlobalEnvVar/init-user-data/collection-security.json
- tests/environments/api-setEnvVar-secret/fixtures/collections/yml/environments/Local.yml
- tests/environments/api-setEnvVar-secret/init-user-data/preferences.json
- tests/environments/api-setEnvVar-secret/fixtures/collections/yml/opencollection.yml
- packages/bruno-app/src/components/SingleLineEditor/index.js
- tests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/environments/Local.yml
- tests/environments/api-setEnvVar-secret/fixtures/collections/yml/read-secret.yml
- tests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/workspace.yml
- tests/environments/api-setGlobalEnvVar/fixtures/workspace/workspace.yml
- tests/environments/api-setGlobalEnvVar-secret/fixtures/workspace/collections/test-collection/seed-global-secret.bru
- packages/bruno-app/src/components/MultiLineEditor/index.js
- tests/environments/api-setEnvVar-secret/fixtures/collections/bru/environments/Local.bru
- tests/environments/api-setEnvVar-secret/fixtures/collections/yml/seed-secret.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/utils/page/locators.ts
- packages/bruno-cli/src/runner/run-single-request.js
- packages/bruno-app/src/utils/environments.spec.js
- packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js
|
|
||
| describe('EnvironmentSecretsStore', () => { | ||
| let secretsStore; | ||
| const collectionPath = '/tmp/test-collection'; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use an OS-agnostic temp path for collectionPath.
Line 6 hardcodes '/tmp/test-collection', which is Unix-specific and can fail on Windows runners. Use os.tmpdir() + path.join() instead.
🔧 Suggested minimal fix
+const os = require('os');
+const path = require('path');
const EnvironmentSecretsStore = require('../../src/store/env-secrets');
const { decryptStringSafe } = require('../../src/utils/encryption');
describe('EnvironmentSecretsStore', () => {
let secretsStore;
- const collectionPath = '/tmp/test-collection';
+ const collectionPath = path.join(os.tmpdir(), 'test-collection');As per path instructions, “Use os.tmpdir() instead of hardcoding /tmp” for cross-platform compatibility.
🤖 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-electron/tests/store/env-secrets.spec.js` at line 6, The
collectionPath variable is hardcoded with a Unix-specific '/tmp' path which will
fail on Windows. Replace the hardcoded '/tmp/test-collection' with a
cross-platform solution by using os.tmpdir() combined with path.join() to
construct the path. Make sure to import the os and path modules at the top of
the file if they are not already imported.
Source: Path instructions
Comment on lines
+22
to
+25
| const findApiTokenSecret = (secretsJson: any, workspaceDir: string) => { | ||
| const collection = secretsJson.collections?.find((c: any) => | ||
| path.normalize(c.path) === path.normalize(workspaceDir) | ||
| ); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Harden secret-path matching for cross-platform path casing differences.
findApiTokenSecret() currently does strict equality on normalized paths; this can miss matches on Windows when path casing differs (same location, different case). Normalize to absolute paths and compare case-insensitively on win32.
Proposed fix
+const normalizeForCompare = (inputPath: string) => {
+ const resolved = path.resolve(inputPath);
+ return process.platform === 'win32' ? resolved.toLowerCase() : resolved;
+};
+
const findApiTokenSecret = (secretsJson: any, workspaceDir: string) => {
- const collection = secretsJson.collections?.find((c: any) =>
- path.normalize(c.path) === path.normalize(workspaceDir)
- );
+ const targetWorkspace = normalizeForCompare(workspaceDir);
+ const collection = secretsJson.collections?.find((c: any) =>
+ normalizeForCompare(c.path) === targetWorkspace
+ );
const env = collection?.environments?.find((e: any) => e.name === 'Local');
return env?.secrets?.find((s: any) => s.name === 'apiToken');
};As per path instructions, "Never assume case-sensitive or case-insensitive filesystems."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const findApiTokenSecret = (secretsJson: any, workspaceDir: string) => { | |
| const collection = secretsJson.collections?.find((c: any) => | |
| path.normalize(c.path) === path.normalize(workspaceDir) | |
| ); | |
| const normalizeForCompare = (inputPath: string) => { | |
| const resolved = path.resolve(inputPath); | |
| return process.platform === 'win32' ? resolved.toLowerCase() : resolved; | |
| }; | |
| const findApiTokenSecret = (secretsJson: any, workspaceDir: string) => { | |
| const targetWorkspace = normalizeForCompare(workspaceDir); | |
| const collection = secretsJson.collections?.find((c: any) => | |
| normalizeForCompare(c.path) === targetWorkspace | |
| ); | |
| const env = collection?.environments?.find((e: any) => e.name === 'Local'); | |
| return env?.secrets?.find((s: any) => s.name === 'apiToken'); | |
| }; |
🤖 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-setGlobalEnvVar-secret/api-setGlobalEnvVar-secret.spec.ts`
around lines 22 - 25, The findApiTokenSecret function currently uses strict path
equality after normalization, which fails on Windows when paths have different
casing. Modify the comparison logic to convert both paths to absolute paths
using path.resolve and then perform a case-insensitive comparison on win32
platforms. Check if process.platform is 'win32' and if so, compare the
lowercased versions of the absolute paths; otherwise use case-sensitive
comparison. This ensures the path matching works correctly across Windows and
Unix-like systems with varying path casing conventions.
Source: Path instructions
Comment on lines
+180
to
+186
| const secretsJson = JSON.parse(fs.readFileSync(secretsPath, 'utf8')); | ||
| const secret = findApiTokenSecret(secretsJson, workspacePath); | ||
| expect(secret!.value).not.toContain(INITIAL_VALUE); | ||
| expect(JSON.stringify(secretsJson)).not.toContain(INITIAL_VALUE); | ||
| expect(fs.readFileSync(envFilePath, 'utf8')).not.toContain(secret!.value); | ||
| initialEncryptedValue = secret!.value; | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace non-null assertions with explicit existence checks for clearer failures.
Using secret!.value in these blocks can throw a generic runtime error when lookup fails. Add an explicit assertion (and guard) before dereferencing so failures explain the broken behavior.
Proposed fix pattern
const secretsJson = JSON.parse(fs.readFileSync(secretsPath, 'utf8'));
const secret = findApiTokenSecret(secretsJson, workspacePath);
-expect(secret!.value).not.toContain(INITIAL_VALUE);
+expect(secret).toBeDefined();
+if (!secret) {
+ throw new Error('Expected apiToken secret to exist in secrets.json');
+}
+expect(secret.value).not.toContain(INITIAL_VALUE);As per coding guidelines, "Aim for tests that fail usefully. When a test fails, it should clearly indicate what behaviour broke and why."
Also applies to: 267-269, 283-285
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 183-183: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(envFilePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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-setGlobalEnvVar-secret/api-setGlobalEnvVar-secret.spec.ts`
around lines 180 - 186, Replace the non-null assertion operator (!) on the
secret variable with explicit existence checks to provide clearer test failure
messages. In the test blocks around findApiTokenSecret calls, add expect
assertions to verify that the secret object exists before dereferencing its
value property (such as using expect(secret).toBeTruthy() or
expect(secret).toBeDefined()). This should be done in the assertion blocks where
secret!.value is currently accessed without checking if secret is null or
undefined, and the same pattern should be applied to the other occurrences
mentioned at lines 267-269 and 283-285.
Source: Coding guidelines
- Added multiple test cases to validate the persistence of collection variables across app restarts, including typed values and multiple variable settings. - Created new fixtures for collection variables to support the tests, ensuring accurate simulation of variable management scenarios. - Enhanced the existing collection management logic to ensure that variables are correctly set and deleted as per the test requirements.
- Introduced a new test suite to validate the persistence of typed global environment variables across app restarts, ensuring correct data types are maintained. - Created a fixture for the test collection to simulate setting global variables with various data types, including number, boolean, object, and string. - Enhanced the test logic to verify that the environment file reflects the correct state before and after application restarts.
sanish-bruno
force-pushed
the
feat/varianble-persistence-cli
branch
from
7917352 to
aaa0000
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/environments/api-setCollectionVar/api-setCollectionVar-typed.spec.ts (1)
37-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a named locator variable for the request item.
Inline
getByTextin Line 39 is harder to maintain and debug than a dedicated locator variable.As per path instructions, "Use locator variables for locators" and "Replace brittle text/index selectors with role, label, test id, or stable user-facing selectors."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/environments/api-setCollectionVar/api-setCollectionVar-typed.spec.ts` around lines 37 - 40, The getByText selector for 'api-setCollectionVar-typed' on line 39 is inlined and should be extracted into a named locator variable for better maintainability. Create a locator variable that captures the page.getByText call with exact matching for 'api-setCollectionVar-typed', then replace the inline selector with this variable in the click call. This follows the guidance to use dedicated locator variables instead of inline selectors.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-cli/src/commands/run.js`:
- Around line 366-370: The resolveEnvFileFormat function only checks for the
`.yml` extension and defaults to `bru` format for any unmatched extension, which
causes `.yaml` files to be incorrectly treated as `bru` format. Add a condition
to explicitly handle `.yaml` files by checking if the extension equals `.yaml`
and returning 'yml' for it, similar to the existing `.yml` check. Apply the same
fix to the other location mentioned that contains similar extension-matching
logic.
In `@tests/environments/api-setCollectionVar/api-setCollectionVar-typed.spec.ts`:
- Around line 56-76: The test step does not guarantee that the newApp instance
is closed if any assertion fails before reaching the closeElectronApp call on
line 75, which can cause resource leaks affecting subsequent tests. Wrap the
entire body of the test.step callback (starting from the restartApp call through
all the await expect assertions) in a try/finally block, placing the
closeElectronApp(newApp) call in the finally block to ensure the app is always
closed regardless of whether the test assertions pass or fail.
In `@tests/environments/api-setCollectionVar/multiple-persist-vars.spec.ts`:
- Around line 47-48: The current assertion in the expect.poll() call uses a
single regex pattern that expects both collection variables to appear in a
specific order, making the test flaky if the persistence logic changes variable
order. Instead of checking for both multiple-persist-coll-vars-key1 and
multiple-persist-coll-vars-key2 in a specific order within one regex, refactor
the assertion to check for each variable key separately using independent regex
checks or separate toMatch() calls on the same polled file content, ensuring the
test passes regardless of the order in which the variables are persisted to the
collection.bru file.
In
`@tests/environments/api-setGlobalEnvVar/api-setGlobalEnvVar-typed-restart.spec.ts`:
- Line 83: In the E2E test for the configure button interaction, replace the
synthetic dispatchEvent('click') call with the native click() method on the
locators.environment.configureButton() locator. This ensures the test exercises
actual user-actionable behavior through a real click action rather than a
synthetic event, which aligns with E2E testing best practices for verifying
user-visible interactions.
---
Nitpick comments:
In `@tests/environments/api-setCollectionVar/api-setCollectionVar-typed.spec.ts`:
- Around line 37-40: The getByText selector for 'api-setCollectionVar-typed' on
line 39 is inlined and should be extracted into a named locator variable for
better maintainability. Create a locator variable that captures the
page.getByText call with exact matching for 'api-setCollectionVar-typed', then
replace the inline selector with this variable in the click call. This follows
the guidance to use dedicated locator variables instead of inline selectors.
🪄 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: dba68791-496d-4cf8-af98-9461d2071baf
📒 Files selected for processing (18)
packages/bruno-cli/src/commands/run.jspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-cli/src/utils/persist-variables.jspackages/bruno-cli/tests/utils/persist-variables.spec.jstests/environments/api-setCollectionVar/api-deleteCollectionVar.spec.tstests/environments/api-setCollectionVar/api-setCollectionVar-default-persistence.spec.tstests/environments/api-setCollectionVar/api-setCollectionVar-typed.spec.tstests/environments/api-setCollectionVar/fixtures/collection/api-deleteCollectionVar.brutests/environments/api-setCollectionVar/fixtures/collection/api-multi-persist-coll-vars.brutests/environments/api-setCollectionVar/fixtures/collection/api-setCollectionVar-default-persistence.brutests/environments/api-setCollectionVar/fixtures/collection/api-setCollectionVar-typed.brutests/environments/api-setCollectionVar/fixtures/collection/bruno.jsontests/environments/api-setCollectionVar/fixtures/collection/collection.brutests/environments/api-setCollectionVar/init-user-data/collection-security.jsontests/environments/api-setCollectionVar/init-user-data/preferences.jsontests/environments/api-setCollectionVar/multiple-persist-vars.spec.tstests/environments/api-setGlobalEnvVar/api-setGlobalEnvVar-typed-restart.spec.tstests/environments/api-setGlobalEnvVar/fixtures/workspace/collections/test-collection/set-typed-global-vars.bru
✅ Files skipped from review due to trivial changes (5)
- tests/environments/api-setCollectionVar/init-user-data/collection-security.json
- tests/environments/api-setCollectionVar/fixtures/collection/api-setCollectionVar-typed.bru
- tests/environments/api-setCollectionVar/fixtures/collection/bruno.json
- tests/environments/api-setCollectionVar/fixtures/collection/api-setCollectionVar-default-persistence.bru
- tests/environments/api-setCollectionVar/init-user-data/preferences.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-cli/src/runner/run-single-request.js
Comment on lines
+366
to
+370
| const resolveEnvFileFormat = (filePath) => { | ||
| const ext = path.extname(filePath).toLowerCase(); | ||
| if (ext === '.json') return 'json'; | ||
| if (ext === '.yml') return 'yml'; | ||
| return 'bru'; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle .yaml as YAML instead of falling back to bru.
Line 369 and Line 386 only match .yml, so --env-file something.yaml is treated as bru format. That can break parsing and persistence format selection.
Suggested minimal fix
const resolveEnvFileFormat = (filePath) => {
const ext = path.extname(filePath).toLowerCase();
if (ext === '.json') return 'json';
- if (ext === '.yml') return 'yml';
+ if (ext === '.yml' || ext === '.yaml') return 'yml';
return 'bru';
};
...
- } else if (fileExt === '.yml') {
+ } else if (fileExt === '.yml' || fileExt === '.yaml') {
const content = fs.readFileSync(filePath, 'utf8');
const envJson = parseEnvironment(content, { format: 'yml' });
result = getEnvVars(envJson);
- result.__name__ = nameOverride || path.basename(filePath, '.yml');
+ const baseName = fileExt === '.yaml'
+ ? path.basename(filePath, '.yaml')
+ : path.basename(filePath, '.yml');
+ result.__name__ = nameOverride || baseName;Also applies to: 386-390
🤖 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-cli/src/commands/run.js` around lines 366 - 370, The
resolveEnvFileFormat function only checks for the `.yml` extension and defaults
to `bru` format for any unmatched extension, which causes `.yaml` files to be
incorrectly treated as `bru` format. Add a condition to explicitly handle
`.yaml` files by checking if the extension equals `.yaml` and returning 'yml'
for it, similar to the existing `.yml` check. Apply the same fix to the other
location mentioned that contains similar extension-matching logic.
Comment on lines
+56
to
+76
| await test.step('Restart and verify the collection vars tab reflects the persisted datatypes', async () => { | ||
| const newApp = await restartApp(); | ||
| const newPage = await waitForReadyPage(newApp); | ||
| const newLocators = buildCommonLocators(newPage); | ||
|
|
||
| await newLocators.sidebar.collection('collection').click(); | ||
| await newLocators.paneTabs.collectionSettingsTab('vars').click(); | ||
|
|
||
| const numRow = newLocators.environment.variableRowByName('coll_num'); | ||
| const boolRow = newLocators.environment.variableRowByName('coll_bool'); | ||
| const objRow = newLocators.environment.variableRowByName('coll_obj'); | ||
| const strRow = newLocators.environment.variableRowByName('coll_str'); | ||
|
|
||
| await expect(numRow).toBeVisible(); | ||
| await expect(newLocators.dataTypeSelector.typeLabel(numRow)).toHaveText('number', { timeout: 5000 }); | ||
| await expect(newLocators.dataTypeSelector.typeLabel(boolRow)).toHaveText('boolean'); | ||
| await expect(newLocators.dataTypeSelector.typeLabel(objRow)).toHaveText('object'); | ||
| await expect(newLocators.dataTypeSelector.typeLabel(strRow)).toHaveText('string'); | ||
|
|
||
| await closeElectronApp(newApp); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard restarted app teardown with try/finally.
If an assertion fails before Line 75, newApp may stay open and leak into subsequent tests. Wrap the restart block in try/finally to always close it.
Suggested fix
- await test.step('Restart and verify the collection vars tab reflects the persisted datatypes', async () => {
- const newApp = await restartApp();
- const newPage = await waitForReadyPage(newApp);
- const newLocators = buildCommonLocators(newPage);
+ await test.step('Restart and verify the collection vars tab reflects the persisted datatypes', async () => {
+ const newApp = await restartApp();
+ try {
+ const newPage = await waitForReadyPage(newApp);
+ const newLocators = buildCommonLocators(newPage);
- await newLocators.sidebar.collection('collection').click();
- await newLocators.paneTabs.collectionSettingsTab('vars').click();
+ await newLocators.sidebar.collection('collection').click();
+ await newLocators.paneTabs.collectionSettingsTab('vars').click();
- const numRow = newLocators.environment.variableRowByName('coll_num');
- const boolRow = newLocators.environment.variableRowByName('coll_bool');
- const objRow = newLocators.environment.variableRowByName('coll_obj');
- const strRow = newLocators.environment.variableRowByName('coll_str');
+ const numRow = newLocators.environment.variableRowByName('coll_num');
+ const boolRow = newLocators.environment.variableRowByName('coll_bool');
+ const objRow = newLocators.environment.variableRowByName('coll_obj');
+ const strRow = newLocators.environment.variableRowByName('coll_str');
- await expect(numRow).toBeVisible();
- await expect(newLocators.dataTypeSelector.typeLabel(numRow)).toHaveText('number', { timeout: 5000 });
- await expect(newLocators.dataTypeSelector.typeLabel(boolRow)).toHaveText('boolean');
- await expect(newLocators.dataTypeSelector.typeLabel(objRow)).toHaveText('object');
- await expect(newLocators.dataTypeSelector.typeLabel(strRow)).toHaveText('string');
-
- await closeElectronApp(newApp);
+ await expect(numRow).toBeVisible();
+ await expect(newLocators.dataTypeSelector.typeLabel(numRow)).toHaveText('number', { timeout: 5000 });
+ await expect(newLocators.dataTypeSelector.typeLabel(boolRow)).toHaveText('boolean');
+ await expect(newLocators.dataTypeSelector.typeLabel(objRow)).toHaveText('object');
+ await expect(newLocators.dataTypeSelector.typeLabel(strRow)).toHaveText('string');
+ } finally {
+ await closeElectronApp(newApp);
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await test.step('Restart and verify the collection vars tab reflects the persisted datatypes', async () => { | |
| const newApp = await restartApp(); | |
| const newPage = await waitForReadyPage(newApp); | |
| const newLocators = buildCommonLocators(newPage); | |
| await newLocators.sidebar.collection('collection').click(); | |
| await newLocators.paneTabs.collectionSettingsTab('vars').click(); | |
| const numRow = newLocators.environment.variableRowByName('coll_num'); | |
| const boolRow = newLocators.environment.variableRowByName('coll_bool'); | |
| const objRow = newLocators.environment.variableRowByName('coll_obj'); | |
| const strRow = newLocators.environment.variableRowByName('coll_str'); | |
| await expect(numRow).toBeVisible(); | |
| await expect(newLocators.dataTypeSelector.typeLabel(numRow)).toHaveText('number', { timeout: 5000 }); | |
| await expect(newLocators.dataTypeSelector.typeLabel(boolRow)).toHaveText('boolean'); | |
| await expect(newLocators.dataTypeSelector.typeLabel(objRow)).toHaveText('object'); | |
| await expect(newLocators.dataTypeSelector.typeLabel(strRow)).toHaveText('string'); | |
| await closeElectronApp(newApp); | |
| }); | |
| await test.step('Restart and verify the collection vars tab reflects the persisted datatypes', async () => { | |
| const newApp = await restartApp(); | |
| try { | |
| const newPage = await waitForReadyPage(newApp); | |
| const newLocators = buildCommonLocators(newPage); | |
| await newLocators.sidebar.collection('collection').click(); | |
| await newLocators.paneTabs.collectionSettingsTab('vars').click(); | |
| const numRow = newLocators.environment.variableRowByName('coll_num'); | |
| const boolRow = newLocators.environment.variableRowByName('coll_bool'); | |
| const objRow = newLocators.environment.variableRowByName('coll_obj'); | |
| const strRow = newLocators.environment.variableRowByName('coll_str'); | |
| await expect(numRow).toBeVisible(); | |
| await expect(newLocators.dataTypeSelector.typeLabel(numRow)).toHaveText('number', { timeout: 5000 }); | |
| await expect(newLocators.dataTypeSelector.typeLabel(boolRow)).toHaveText('boolean'); | |
| await expect(newLocators.dataTypeSelector.typeLabel(objRow)).toHaveText('object'); | |
| await expect(newLocators.dataTypeSelector.typeLabel(strRow)).toHaveText('string'); | |
| } finally { | |
| await closeElectronApp(newApp); | |
| } | |
| }); |
🤖 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-setCollectionVar/api-setCollectionVar-typed.spec.ts`
around lines 56 - 76, The test step does not guarantee that the newApp instance
is closed if any assertion fails before reaching the closeElectronApp call on
line 75, which can cause resource leaks affecting subsequent tests. Wrap the
entire body of the test.step callback (starting from the restartApp call through
all the await expect assertions) in a try/finally block, placing the
closeElectronApp(newApp) call in the finally block to ensure the app is always
closed regardless of whether the test assertions pass or fail.
Comment on lines
+47
to
+48
| await expect.poll(() => fs.readFileSync(collectionBruPath, 'utf8'), { timeout: 10000 }) | ||
| .toMatch(/multiple-persist-coll-vars-key1:\s*value1[\s\S]*multiple-persist-coll-vars-key2:\s*value2/); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make disk assertion order-independent to avoid flaky failures.
Line 48 couples success to variable order in collection.bru; persistence can be correct even if key order changes. Assert each key separately on the same polled content.
Suggested change
- await expect.poll(() => fs.readFileSync(collectionBruPath, 'utf8'), { timeout: 10000 })
- .toMatch(/multiple-persist-coll-vars-key1:\s*value1[\s\S]*multiple-persist-coll-vars-key2:\s*value2/);
+ await expect.poll(() => fs.readFileSync(collectionBruPath, 'utf8'), { timeout: 10000 })
+ .toMatch(/multiple-persist-coll-vars-key1:\s*value1/);
+ await expect.poll(() => fs.readFileSync(collectionBruPath, 'utf8'), { timeout: 10000 })
+ .toMatch(/multiple-persist-coll-vars-key2:\s*value2/);As per coding guidelines, “Ensure tests are deterministic and reproducible” and “Avoid overfitting tests to current behaviour if future flexibility matters.”
🤖 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-setCollectionVar/multiple-persist-vars.spec.ts` around
lines 47 - 48, The current assertion in the expect.poll() call uses a single
regex pattern that expects both collection variables to appear in a specific
order, making the test flaky if the persistence logic changes variable order.
Instead of checking for both multiple-persist-coll-vars-key1 and
multiple-persist-coll-vars-key2 in a specific order within one regex, refactor
the assertion to check for each variable key separately using independent regex
checks or separate toMatch() calls on the same polled file content, ensuring the
test passes regardless of the order in which the variables are persisted to the
collection.bru file.
Source: Coding guidelines
| await locators.environment.selector().click(); | ||
| await locators.environment.globalTab().click(); | ||
| await locators.environment.configureButton().waitFor({ state: 'visible' }); | ||
| await locators.environment.configureButton().dispatchEvent('click'); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prefer click() over synthetic dispatchEvent('click') in this E2E path.
Line 83 should use click() so the test exercises user-actionability instead of a synthetic event path.
Suggested fix
- await locators.environment.configureButton().dispatchEvent('click');
+ await locators.environment.configureButton().click();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await locators.environment.configureButton().dispatchEvent('click'); | |
| await locators.environment.configureButton().click(); |
🤖 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-setGlobalEnvVar/api-setGlobalEnvVar-typed-restart.spec.ts`
at line 83, In the E2E test for the configure button interaction, replace the
synthetic dispatchEvent('click') call with the native click() method on the
locators.environment.configureButton() locator. This ensures the test exercises
actual user-actionable behavior through a real click action rather than a
synthetic event, which aligns with E2E testing best practices for verifying
user-visible interactions.
Source: Path instructions
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-cli/tests/integration/run-typed-persistence.spec.js`:
- Around line 74-77: The spawn call in the test is hardcoding a Unix-specific
SHELL environment variable to '/bin/sh', which breaks on Windows CI runs and
violates platform portability requirements. Remove the SHELL property from the
env object passed to spawn, so that only the inherited process.env properties
are used. Update the env assignment from `{ ...process.env, SHELL: '/bin/sh' }`
to simply `{ ...process.env }` to ensure cross-platform compatibility.
🪄 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: 75d23d3b-24c0-4d62-aa46-2a790df25746
📒 Files selected for processing (2)
packages/bruno-cli/tests/integration/run-typed-persistence.spec.jspackages/bruno-cli/tests/utils/persist-variables.spec.js
Comment on lines
+74
to
+77
| const child = spawn(process.execPath, [CLI_BIN, ...args], { | ||
| cwd, | ||
| env: { ...process.env, SHELL: '/bin/sh' } | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the Unix-specific SHELL override in spawned CLI env.
Line 76 hardcodes '/bin/sh', which is not portable and can break Windows CI runs. In this test, inheriting process.env is enough.
Minimal fix
const child = spawn(process.execPath, [CLI_BIN, ...args], {
cwd,
- env: { ...process.env, SHELL: '/bin/sh' }
+ env: { ...process.env }
});As per path instructions, “Shell commands or child_process calls must account for platform differences (e.g., which vs where, /bin/sh vs cmd.exe)”.
🤖 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-cli/tests/integration/run-typed-persistence.spec.js` around
lines 74 - 77, The spawn call in the test is hardcoding a Unix-specific SHELL
environment variable to '/bin/sh', which breaks on Windows CI runs and violates
platform portability requirements. Remove the SHELL property from the env object
passed to spawn, so that only the inherited process.env properties are used.
Update the env assignment from `{ ...process.env, SHELL: '/bin/sh' }` to simply
`{ ...process.env }` to ensure cross-platform compatibility.
Source: Path instructions
…r tests - Updated the hover logic for revealing collection actions to handle sidebar re-renders more reliably. - Replaced one-shot hover with a polling mechanism to ensure visibility of actions, enhancing test stability.
sanish-bruno
force-pushed
the
feat/varianble-persistence-cli
branch
from
410c36e to
2257b21
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
packages/bruno-cli/tests/integration/run-typed-persistence.spec.js (1)
74-77: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove the Unix-specific
SHELLoverride from spawned env.Line 76 hardcodes
/bin/sh, which is not portable and can fail on Windows runs.As per path instructions, “Shell commands or child_process calls must account for platform differences (e.g.,
whichvswhere,/bin/shvscmd.exe)”.Minimal fix
const child = spawn(process.execPath, [CLI_BIN, ...args], { cwd, - env: { ...process.env, SHELL: '/bin/sh' } + env: { ...process.env } });🤖 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-cli/tests/integration/run-typed-persistence.spec.js` around lines 74 - 77, The spawn call is hardcoding a Unix-specific SHELL environment variable set to /bin/sh, which will fail on Windows systems. Remove the SHELL: '/bin/sh' property from the env object being passed to the spawn function call, keeping only the spread of process.env to maintain platform portability and allow each system to use its native shell environment.Source: Path instructions
packages/bruno-cli/src/commands/run.js (2)
366-370: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle
.yamlas YAML in both detection and loading paths.Line 369 and Line 386 only match
.yml, so--env-file something.yamlis treated asbru, which breaks parser/serializer selection.Suggested minimal fix
const resolveEnvFileFormat = (filePath) => { const ext = path.extname(filePath).toLowerCase(); if (ext === '.json') return 'json'; - if (ext === '.yml') return 'yml'; + if (ext === '.yml' || ext === '.yaml') return 'yml'; return 'bru'; }; ... - } else if (fileExt === '.yml') { + } else if (fileExt === '.yml' || fileExt === '.yaml') { const content = fs.readFileSync(filePath, 'utf8'); const envJson = parseEnvironment(content, { format: 'yml' }); result = getEnvVars(envJson); - result.__name__ = nameOverride || path.basename(filePath, '.yml'); + const baseName = fileExt === '.yaml' + ? path.basename(filePath, '.yaml') + : path.basename(filePath, '.yml'); + result.__name__ = nameOverride || baseName; } else {Also applies to: 386-390
🤖 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-cli/src/commands/run.js` around lines 366 - 370, The resolveEnvFileFormat function only checks for the `.yml` extension when determining if a file should be treated as YAML format, causing `.yaml` files to be incorrectly treated as `bru` format. Add an additional condition in the function to also match the `.yaml` extension alongside the existing `.yml` check. Additionally, apply the same fix to the other location mentioned at lines 386-390 to ensure consistent handling of both `.yml` and `.yaml` extensions throughout the file detection and loading paths.
633-640: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftPrevent
--env-varoverlays from being written into persisted env files.Line 633 introduces persistence routing, but
envVarsalready includes CLI override values; when persistence is triggered, transient CLI overrides can be serialized to disk.A robust fix is to track override keys/sources during
--env-varparsing and exclude those keys from persisted env payloads before callingpersistVariableUpdates().🤖 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-cli/src/commands/run.js` around lines 633 - 640, The `persistPaths` object created around line 635 is configured to persist all `envVars` to disk, but `envVars` includes transient CLI override values from `--env-var` arguments that should not be persisted. To fix this, track which environment variable keys originate from CLI overrides during `--env-var` argument parsing, then filter out those tracked override keys from the environment payload before calling `persistVariableUpdates()`. This ensures that only the persistent environment file state is written to disk, excluding any transient command-line overrides.packages/bruno-cli/tests/utils/persist-variables.spec.js (1)
12-20: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse an isolated temp directory per test run.
Line 12 uses one shared temp path for the whole file; this risks collisions in parallelized/sharded execution.
As per coding guidelines, “Ensure tests are deterministic and reproducible.”
Suggested change
-const tmpBase = path.join(os.tmpdir(), 'bruno-cli-persist-vars-tests'); +let tmpBase; beforeEach(() => { - fs.mkdirSync(tmpBase, { recursive: true }); + tmpBase = fs.mkdtempSync(path.join(os.tmpdir(), 'bruno-cli-persist-vars-tests-')); }); afterEach(() => { fs.rmSync(tmpBase, { recursive: true, force: true }); });🤖 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-cli/tests/utils/persist-variables.spec.js` around lines 12 - 20, The tmpBase variable is defined once at the file level and reused across all tests, which causes collisions when tests run in parallel. Modify the beforeEach hook to create a unique isolated temporary directory for each test run (for example, by appending a unique identifier like a UUID or timestamp to the base path), update the afterEach hook to clean up that specific test's temporary directory, and ensure tmpBase or the directory path reference is scoped appropriately so each test gets its own isolated directory.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.
Duplicate comments:
In `@packages/bruno-cli/src/commands/run.js`:
- Around line 366-370: The resolveEnvFileFormat function only checks for the
`.yml` extension when determining if a file should be treated as YAML format,
causing `.yaml` files to be incorrectly treated as `bru` format. Add an
additional condition in the function to also match the `.yaml` extension
alongside the existing `.yml` check. Additionally, apply the same fix to the
other location mentioned at lines 386-390 to ensure consistent handling of both
`.yml` and `.yaml` extensions throughout the file detection and loading paths.
- Around line 633-640: The `persistPaths` object created around line 635 is
configured to persist all `envVars` to disk, but `envVars` includes transient
CLI override values from `--env-var` arguments that should not be persisted. To
fix this, track which environment variable keys originate from CLI overrides
during `--env-var` argument parsing, then filter out those tracked override keys
from the environment payload before calling `persistVariableUpdates()`. This
ensures that only the persistent environment file state is written to disk,
excluding any transient command-line overrides.
In `@packages/bruno-cli/tests/integration/run-typed-persistence.spec.js`:
- Around line 74-77: The spawn call is hardcoding a Unix-specific SHELL
environment variable set to /bin/sh, which will fail on Windows systems. Remove
the SHELL: '/bin/sh' property from the env object being passed to the spawn
function call, keeping only the spread of process.env to maintain platform
portability and allow each system to use its native shell environment.
In `@packages/bruno-cli/tests/utils/persist-variables.spec.js`:
- Around line 12-20: The tmpBase variable is defined once at the file level and
reused across all tests, which causes collisions when tests run in parallel.
Modify the beforeEach hook to create a unique isolated temporary directory for
each test run (for example, by appending a unique identifier like a UUID or
timestamp to the base path), update the afterEach hook to clean up that specific
test's temporary directory, and ensure tmpBase or the directory path reference
is scoped appropriately so each test gets its own isolated directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de56d24b-7fe1-44b4-9420-cb978a8c295c
📒 Files selected for processing (7)
packages/bruno-cli/src/commands/run.jspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-cli/src/utils/persist-variables.jspackages/bruno-cli/tests/integration/run-typed-persistence.spec.jspackages/bruno-cli/tests/utils/persist-variables.spec.jstests/scripting/bru-api/variable-persistence-typed/variable-persistence-typed.spec.tstests/utils/page/runner.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/scripting/bru-api/variable-persistence-typed/variable-persistence-typed.spec.ts
- packages/bruno-cli/src/runner/run-single-request.js
…remove ephemeral metadata logic - Simplified the comparison logic for environment variables by removing unnecessary ephemeral metadata handling. - Updated the saving process to directly use the environment variables without stripping metadata, enhancing clarity and maintainability. - Removed outdated comments and unused utility functions related to ephemeral variables, improving code cleanliness.
…ale updates - Modified the persistActiveEnvironment function to accept a requestUid parameter, allowing for better management of stale updates. - Enhanced the logic to prevent disk writes for superseded requests, improving data integrity during environment persistence.
- Eliminated the envName variable from the deleteAllEnvVars method, simplifying the logic for deleting environment variables. - Cleaned up the method by removing unnecessary checks related to the envName, enhancing code clarity and maintainability.
…Var method - Added a check in the deleteEnvVar method to silently ignore attempts to delete the internal __name__ variable, preserving its integrity. - Updated tests to verify that the __name__ variable remains unchanged when deleteEnvVar is called with this key. - Enhanced runtime tests to ensure compatibility with QuickJS by confirming that environment variables set with persist options are handled correctly.
…ence - Introduced a new test suite to validate that the legacy argument for setting environment variables with persistence is still functional in version 4. - Created a fixture to simulate the legacy syntax, ensuring that the variable is correctly persisted on disk without errors. - Enhanced integration testing to confirm that the legacy behavior aligns with the current implementation, maintaining backward compatibility.
…onment and collection files - Introduced new utility functions for applying and persisting variable updates, enhancing the management of environment and collection variables. - Updated the run command to include descriptors for environment files, allowing for better tracking of variable changes. - Enhanced the runSingleRequest function to synchronize variable updates and persist changes to the appropriate files. - Added comprehensive tests to ensure the correct functionality of variable merging and persistence logic across different file formats.
…ronment variables - Updated the run command to exclusively recognize '.yml' files, removing support for '.yaml' extensions. - Enhanced the persistence logic to correctly handle data type inference for newly added environment and collection variables. - Added comprehensive tests to validate the correct writing of variables to YAML and .bru files, ensuring accurate data type preservation and updates. - Improved the handling of existing data types when variables are modified, ensuring consistency across different formats.
- Introduced a new test suite to validate the persistence of typed environment and collection variables set via scripts in the CLI. - Implemented a local server to facilitate HTTP requests during tests, ensuring accurate simulation of variable management. - Enhanced the test logic to verify that the correct data type annotations are written to both .bru and YAML files after CLI execution. - Added comprehensive assertions to confirm the expected disk state for both global and collection variables across different sandboxes.
…s after post response script execution - Eliminated the syncVariableUpdates call from the runSingleRequest function, streamlining the variable handling process. - Adjusted the logic to focus on executing post response variables without unnecessary synchronization, improving performance and clarity.
… improve persistence resilience - Added support for transient environment variable overrides via the `--env-var` CLI option, ensuring these values are not persisted to disk. - Enhanced the `mergeScriptVarsIntoEnvList` function to prevent overridden variables from being written back, preserving existing entries. - Updated the `persistVariableUpdates` function to handle potential disk write errors gracefully, logging warnings instead of failing the execution. - Introduced comprehensive tests to validate the behavior of transient variables and ensure correct persistence logic under various scenarios.
- Improved the `persistEnvFile` function to preserve additional fields (uid, dataType, custom metadata) during JSON writes, ensuring no data loss for unrecognized entries. - Updated the `persistVariableUpdates` function to respect `envVarOverrides` when writing to the global environment file, preventing transient values from being persisted. - Added regression tests to validate the preservation of additional fields and the correct handling of environment variable overrides in persistence scenarios.
…istence tests - Renamed the `writeFileSyncMkdirP` function to `writeFixtureFile` for clarity, emphasizing its role in writing fixture files with parent directory creation. - Replaced multiple calls to `writeFileSyncMkdirP` with the new `writeFixtureFile` function to improve code readability and maintainability. - Added tests to verify the deletion of environment variables from disk when removed from the environment map, ensuring accurate persistence behavior. - Implemented checks to prevent runtime variables from being persisted, safeguarding against unintended data leaks.
…rsistence - Implemented a new test to verify that typed environment variables are correctly persisted to a JSON file using the --env-file option. - Ensured that existing entries retain their uid and custom fields during the persistence process, validating the shape-preservation guarantee. - Enhanced the test to check that new typed variables from the script are accurately written and that untouched entries remain unchanged.
…istence - Implemented new tests to verify the persistence of typed environment variables to both YAML and .bru files using the --env-file option. - Ensured that typed values are correctly serialized with appropriate annotations in the output files, validating the correct handling of different formats. - Enhanced coverage for the persistence behavior of environment variables, confirming that existing entries remain unchanged while new variables are accurately written.
- Added a new test to verify the preservation of native typed values in JSON environment files when unrelated keys are touched during script execution. - Ensured that typed values maintain their original types and are auto-annotated with the correct dataType, validating the integrity of the environment variable persistence process. - Expanded the typed-value inference tests to cover cross-type transitions, confirming that existing dataType annotations are ignored when new values are written.
sanish-bruno
force-pushed
the
feat/varianble-persistence-cli
branch
from
6bac0f0 to
e4788b7
Compare
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.
JIRA
Brings the variable-persistence behavior from #8315 to the CLI. Scripts that call
bru.setEnvVar/bru.setGlobalEnvVar/bru.setCollectionVar(and theirdelete*counterparts) now write changes back to disk in addition to mutating in-memory state — matching desktop-app behavior on the same branch.Important
Depends on #8315 and must be merged after it.
Scopes persisted
--envenv file, or--env-file<workspace>/environments/<globalEnv>.ymlcollection.bru/opencollection.ymlHighlights
bru.setEnvVar('count', 42)) get the rightdataType(number/boolean/object); matching.bru/.ymlannotations land on disk.--env-vartransient overrides never reach disk; the file's existing entry (incl.secret: true) is preserved untouched.uid,dataType, and user-set custom metadata survive on entries the script doesn't echo back.Tests
tests/utils/persist-variables.spec.js— merge logic, file round-trips (.yml/.bru/.json/ collection root), typed-value inference, override regression guards, runtime-vars-NEVER-persisted design guard, content-comparison viawriteFileSyncspy, disk-error resilience.tests/integration/run-typed-persistence.spec.jsdriving the realbin/bru.jsagainst a local HTTP server, bothdeveloperandsafesandboxes — typed annotations on.bruand.ymlvia--env/--global-env,--workspace-pathexplicit flag,--env-fileround-trip across all three formats (.json/.yml/.bru) including JSONuid/ custom-field shape preservation,--env-varleak guard.Contribution Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
bru.setEnvVar(key, value)(no persistence options argument).Documentation