fix: resolve process.env variables in global environment level#7600
Open
abhishek-bruno wants to merge 3 commits intousebruno:mainfrom
Open
fix: resolve process.env variables in global environment level#7600abhishek-bruno wants to merge 3 commits intousebruno:mainfrom
abhishek-bruno wants to merge 3 commits intousebruno:mainfrom
Conversation
abhishek-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-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:
WalkthroughInjects workspace-level process.env values into global (no-collection) environment rendering, registers scratch collections with their workspace via an IPC call to enable .env resolution across workspace/global scopes, adds UI test selectors, and adds Playwright E2E test + fixtures validating process.env resolution. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer UI
participant Redux as Redux Store
participant Main as Main (IPC)
participant FS as Workspace/.env
UI->>Redux: read state.workspaces.activeWorkspaceUid & workspaces
Redux-->>UI: return activeWorkspace (includes processEnvVariables)
UI->>UI: if collection == null → set _collection.workspaceProcessEnvVariables
UI->>Main: renderer:add-collection-watcher(scratchCollectionUid)
Main-->>UI: watcher registered
UI->>Main: renderer:set-collection-workspace(scratchCollectionUid, workspace.pathname)
Main->>FS: persist association / enable getProcessEnvVars lookup
UI->>UI: render EnvironmentVariablesTable → MultiLineEditor resolves {{process.env.X}}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.js (1)
48-51: Consider combining selectors for efficiency.The second selector runs
.find()on every render. You could combine these into a single selector or use a memoized selector to avoid repeated array traversals.♻️ Optional: combined selector
- const activeWorkspaceUid = useSelector((state) => state.workspaces?.activeWorkspaceUid); - const activeWorkspace = useSelector((state) => - state.workspaces?.workspaces?.find((w) => w.uid === activeWorkspaceUid) - ); + const { activeWorkspaceUid, activeWorkspace } = useSelector((state) => { + const uid = state.workspaces?.activeWorkspaceUid; + return { + activeWorkspaceUid: uid, + activeWorkspace: state.workspaces?.workspaces?.find((w) => w.uid === uid) + }; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js` around lines 48 - 51, The code calls useSelector twice causing state.workspaces.workspaces.find(...) to run on every render; replace with a single memoized selector that returns the activeWorkspace along with/derived from activeWorkspaceUid (or move the logic into a reselect selector in the workspaces slice) so you only traverse the workspaces array once; update references to activeWorkspaceUid and activeWorkspace to use the new selector (or import the reselect selector) to avoid repeated .find() calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js`:
- Around line 48-51: The code calls useSelector twice causing
state.workspaces.workspaces.find(...) to run on every render; replace with a
single memoized selector that returns the activeWorkspace along with/derived
from activeWorkspaceUid (or move the logic into a reselect selector in the
workspaces slice) so you only traverse the workspaces array once; update
references to activeWorkspaceUid and activeWorkspace to use the new selector (or
import the reselect selector) to avoid repeated .find() calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f483840-552e-48dc-9eaa-2ff6d4be7839
📒 Files selected for processing (2)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
Collaborator
sanish-bruno
left a comment
sanish-bruno
left a comment
There was a problem hiding this comment.
PR Review
Found 2 issue(s): 0 major, 0 minor, 1 suggestion, 1 nit.
Summary
- The fix correctly connects the missing data paths: populating
workspaceProcessEnvVariableson_collectionwhencollectionis null, and mapping scratch collections to their workspace viarenderer:set-collection-workspace. - The downstream
getAllVariablesfunction already supportsworkspaceProcessEnvVariables(merges it withprocessEnvVariables), so no changes are needed there. - The IPC handler
renderer:set-collection-workspaceexists and is already used for non-scratch collections — extending it to scratch collections is the right approach. - Overall a clean, well-scoped fix.
packages/bruno-app/src/components/EnvironmentVariablesTable/index.js
Outdated
Show resolved
Hide resolved
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
Show resolved
Hide resolved
Collaborator
|
@abhishek-bruno is it possible to have playwright test cases for this? |
abhishek-bruno
force-pushed
the
fix/global-env-process-env-resolution
branch
from
40f6e44 to
6d747d8
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
1004-1007:⚠️ Potential issue | 🟡 MinorAvoid aborting scratch mount when workspace mapping IPC fails.
At Line 1006, if
renderer:set-collection-workspacethrows, execution never reaches Line 1009 (openScratchCollectionEvent), even though the watcher was already added at Line 998. Prefer making this mapping call non-blocking (log + continue) so mount flow completes.Suggested defensive change
// Map scratch collection to workspace so getProcessEnvVars can resolve workspace .env values if (workspace.pathname) { - await ipcRenderer.invoke('renderer:set-collection-workspace', scratchCollectionUid, workspace.pathname); + await ipcRenderer + .invoke('renderer:set-collection-workspace', scratchCollectionUid, workspace.pathname) + .catch((error) => { + console.error('Failed to map scratch collection to workspace:', error); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js` around lines 1004 - 1007, The ipc call ipcRenderer.invoke('renderer:set-collection-workspace', scratchCollectionUid, workspace.pathname) can throw and currently aborts the scratch mount before openScratchCollectionEvent runs; wrap that invoke in a try/catch so a failure only logs the error (including scratchCollectionUid and workspace.pathname) and does not rethrow, allowing openScratchCollectionEvent and the existing watcher logic to proceed; ensure logging uses the same logger used elsewhere and keep the mapping attempt non-blocking (no await that blocks critical flow) so mount completes even if mapping fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Around line 1004-1007: The ipc call
ipcRenderer.invoke('renderer:set-collection-workspace', scratchCollectionUid,
workspace.pathname) can throw and currently aborts the scratch mount before
openScratchCollectionEvent runs; wrap that invoke in a try/catch so a failure
only logs the error (including scratchCollectionUid and workspace.pathname) and
does not rethrow, allowing openScratchCollectionEvent and the existing watcher
logic to proceed; ensure logging uses the same logger used elsewhere and keep
the mapping attempt non-blocking (no await that blocks critical flow) so mount
completes even if mapping fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d81ef28-3dec-4b51-a47d-ab9cd8ab91eb
📒 Files selected for processing (13)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.jspackages/bruno-app/src/components/Environments/CollapsibleSection/index.jspackages/bruno-app/src/components/Environments/DotEnvFileDetails/index.jspackages/bruno-app/src/components/Environments/DotEnvFileEditor/DotEnvRawView.jspackages/bruno-app/src/components/Environments/EnvironmentSelector/EnvironmentListContent/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/index.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jstests/environments/global-env-process-env-resolution/fixtures/collection/bruno.jsontests/environments/global-env-process-env-resolution/fixtures/collection/echo-post.brutests/environments/global-env-process-env-resolution/global-env-process-env-resolution.spec.tstests/environments/global-env-process-env-resolution/init-user-data/collection-security.jsontests/environments/global-env-process-env-resolution/init-user-data/global-environments.jsontests/environments/global-env-process-env-resolution/init-user-data/preferences.json
✅ Files skipped from review due to trivial changes (9)
- packages/bruno-app/src/components/Environments/DotEnvFileDetails/index.js
- packages/bruno-app/src/components/Environments/DotEnvFileEditor/DotEnvRawView.js
- tests/environments/global-env-process-env-resolution/fixtures/collection/bruno.json
- packages/bruno-app/src/components/Environments/EnvironmentSelector/EnvironmentListContent/index.js
- tests/environments/global-env-process-env-resolution/init-user-data/preferences.json
- tests/environments/global-env-process-env-resolution/init-user-data/collection-security.json
- tests/environments/global-env-process-env-resolution/fixtures/collection/echo-post.bru
- packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/index.js
- tests/environments/global-env-process-env-resolution/init-user-data/global-environments.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bruno-app/src/components/EnvironmentVariablesTable/index.js
- packages/bruno-app/src/components/Environments/CollapsibleSection/index.js
- tests/environments/global-env-process-env-resolution/global-env-process-env-resolution.spec.ts
…Table
- Added logic to populate process environment variables from the active workspace when no collection is selected, allowing for proper resolution of {{process.env.X}}.
- Updated workspace actions to map scratch collections to their respective workspaces, ensuring that environment variables can be resolved correctly.
This improves the user experience by providing access to workspace-specific environment variables in the environment variables table.
- Refactored the state selection logic in EnvironmentVariablesTable for clarity. - Added data-testid attributes to various components including CollapsibleSection, DotEnvFileDetails, DotEnvRawView, and EnvironmentList to enhance testability. - This change aims to streamline component interactions and facilitate easier testing.
- Introduced data-testid attributes to the EnvironmentList component, enhancing the ability to target elements in tests. - This update includes test IDs for the CollapsibleSection, create .env file button, and the input field for the .env name, facilitating better integration with testing frameworks.
abhishek-bruno
force-pushed
the
fix/global-env-process-env-resolution
branch
from
ba048bb to
e6aa1e8
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/environments/global-env-process-env-resolution/global-env-process-env-resolution.spec.ts (1)
54-56: Consider usingdata-testidfor the environment indicator.The
.current-environmentCSS class selector works, butdata-testidprovides better resilience against styling refactors.♻️ Suggested improvement
await test.step('Verify global environment is active', async () => { - await expect(page.locator('.current-environment')).toContainText('ProcessEnv Test'); + await expect(page.getByTestId('current-environment')).toContainText('ProcessEnv Test'); });This would require adding
data-testid="current-environment"to the corresponding element in the UI component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/environments/global-env-process-env-resolution/global-env-process-env-resolution.spec.ts` around lines 54 - 56, Replace the brittle CSS selector '.current-environment' with a stable test id: add data-testid="current-environment" to the UI element that renders the environment indicator, then update the test step that uses page.locator('.current-environment') (in the test named "Verify global environment is active") to target the test id (e.g., using page.locator('[data-testid="current-environment"]' or getByTestId) and keep the same expect toContainText('ProcessEnv Test').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tests/environments/global-env-process-env-resolution/global-env-process-env-resolution.spec.ts`:
- Around line 54-56: Replace the brittle CSS selector '.current-environment'
with a stable test id: add data-testid="current-environment" to the UI element
that renders the environment indicator, then update the test step that uses
page.locator('.current-environment') (in the test named "Verify global
environment is active") to target the test id (e.g., using
page.locator('[data-testid="current-environment"]' or getByTestId) and keep the
same expect toContainText('ProcessEnv Test').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66617b5b-d13a-43f4-bbec-aabfa8974ed5
📒 Files selected for processing (14)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.jspackages/bruno-app/src/components/Environments/CollapsibleSection/index.jspackages/bruno-app/src/components/Environments/DotEnvFileDetails/index.jspackages/bruno-app/src/components/Environments/DotEnvFileEditor/DotEnvRawView.jspackages/bruno-app/src/components/Environments/EnvironmentSelector/EnvironmentListContent/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/index.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/index.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jstests/environments/global-env-process-env-resolution/fixtures/collection/bruno.jsontests/environments/global-env-process-env-resolution/fixtures/collection/echo-post.brutests/environments/global-env-process-env-resolution/global-env-process-env-resolution.spec.tstests/environments/global-env-process-env-resolution/init-user-data/collection-security.jsontests/environments/global-env-process-env-resolution/init-user-data/global-environments.jsontests/environments/global-env-process-env-resolution/init-user-data/preferences.json
✅ Files skipped from review due to trivial changes (10)
- packages/bruno-app/src/components/Environments/EnvironmentSelector/EnvironmentListContent/index.js
- packages/bruno-app/src/components/Environments/DotEnvFileEditor/DotEnvRawView.js
- tests/environments/global-env-process-env-resolution/fixtures/collection/bruno.json
- packages/bruno-app/src/components/Environments/DotEnvFileDetails/index.js
- tests/environments/global-env-process-env-resolution/init-user-data/preferences.json
- tests/environments/global-env-process-env-resolution/fixtures/collection/echo-post.bru
- tests/environments/global-env-process-env-resolution/init-user-data/collection-security.json
- packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/index.js
- packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/index.js
- tests/environments/global-env-process-env-resolution/init-user-data/global-environments.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
- packages/bruno-app/src/components/Environments/CollapsibleSection/index.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Root Cause
Two missing data connections:
Changes
EnvironmentVariablesTable — when collection is null, read processEnvVariables directly from the active workspace in Redux
mountScratchCollection — call renderer:set-collection-workspace to map scratch collection to workspace path
Fixes #7572
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Bug Fixes
Tests