remove activeEnvironmentUid and migration#7545
Conversation
WalkthroughMigrates active global environment selection out of workspace.yml into a per-workspace persisted store, removes file-based selection APIs and the Redux Changes
Sequence DiagramsequenceDiagram
participant User as User (UI)
participant App as Electron Renderer
participant IPC as Main IPC Handlers
participant Store as globalEnvironmentsStore
participant File as workspace.yml
User->>App: choose environment in UI
App->>IPC: renderer:select-global-environment { environmentUid, workspacePath }
alt workspacePath provided
IPC->>Store: setActiveGlobalEnvironmentUidForWorkspace(workspacePath, environmentUid || null)
Store-->>IPC: ack
else no workspacePath
IPC->>Store: setActiveGlobalEnvironmentUid(environmentUid || null)
Store-->>IPC: ack
end
Note over App,IPC: On startup — get and migrate active selection
App->>IPC: renderer:get-global-environments (workspacePath)
IPC->>Store: getActiveGlobalEnvironmentUidForWorkspace(workspacePath)
alt uid in per-workspace store
Store-->>IPC: return uid
else migrate from file
IPC->>File: read workspace.yml (activeEnvironmentUid?)
alt uid in file
File-->>IPC: return fileUid
IPC->>Store: setActiveGlobalEnvironmentUidForWorkspace(workspacePath, migratedUid)
IPC->>File: remove activeEnvironmentUid from workspace.yml
else check legacy global store
IPC->>Store: getActiveGlobalEnvironmentUid()
Store-->>IPC: return legacyUid
IPC->>Store: setActiveGlobalEnvironmentUidForWorkspace(workspacePath, migratedUid)
end
end
IPC-->>App: { globalEnvironments, activeGlobalEnvironmentUid }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/environments/global-env-migration-from-file/fixtures/workspace/workspace.yml (1)
6-8: Align single-collection fixture naming with convention.Line 8 currently references
collections/test-collection. If this scenario has only one collection fixture, prefer singular naming (collection/...) and keep this path aligned to reduce fixture-layout drift across e2e suites.Based on learnings, "In Bruno e2e tests, organize fixture directories by collection count: use fixtures/collection (singular) when there is a single collection, and fixtures/collections (plural) when there are multiple collection directories inside the fixtures folder."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/environments/global-env-migration-from-file/fixtures/workspace/workspace.yml` around lines 6 - 8, The workspace.yml fixture lists a single collection under the collections array with path "collections/test-collection"; rename the fixture path to the singular convention by changing that path to "collection/test-collection" (keeping the collection name "Test Collection" unchanged) so the fixture directory follows the single-collection naming scheme used across Bruno e2e tests.tests/environments/global-env-migration-from-file/global-env-migration-from-file.spec.ts (1)
13-26: Consider extracting hash functions to a shared utility.The
simpleHashandgenerateUidBasedOnHashfunctions are duplicated frompackages/bruno-electron/src/utils/common.js. If the production implementation changes, this test could pass while actual behavior differs.Consider exporting these from a shared location or importing directly from the source (if build setup permits).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/environments/global-env-migration-from-file/global-env-migration-from-file.spec.ts` around lines 13 - 26, The test duplicates the hashing logic (simpleHash and generateUidBasedOnHash) from packages/bruno-electron/src/utils/common.js which can drift from production; replace the inline functions by importing the canonical implementations (simpleHash and/or generateUidBasedOnHash) from that shared util or move the functions into a shared test-usable utility module and import them in global-env-migration-from-file.spec.ts so the test uses the same hashing code as production (update import statements and remove the local function definitions).
🤖 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-migration-from-file/fixtures/workspace/workspace.yml`:
- Around line 6-8: The workspace.yml fixture lists a single collection under the
collections array with path "collections/test-collection"; rename the fixture
path to the singular convention by changing that path to
"collection/test-collection" (keeping the collection name "Test Collection"
unchanged) so the fixture directory follows the single-collection naming scheme
used across Bruno e2e tests.
In
`@tests/environments/global-env-migration-from-file/global-env-migration-from-file.spec.ts`:
- Around line 13-26: The test duplicates the hashing logic (simpleHash and
generateUidBasedOnHash) from packages/bruno-electron/src/utils/common.js which
can drift from production; replace the inline functions by importing the
canonical implementations (simpleHash and/or generateUidBasedOnHash) from that
shared util or move the functions into a shared test-usable utility module and
import them in global-env-migration-from-file.spec.ts so the test uses the same
hashing code as production (update import statements and remove the local
function definitions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0f99758-3e1a-40c8-843f-ad6f211c78e1
📒 Files selected for processing (16)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jspackages/bruno-electron/src/ipc/global-environments.jspackages/bruno-electron/src/ipc/workspace.jspackages/bruno-electron/src/store/default-workspace.jspackages/bruno-electron/src/store/global-environments.jspackages/bruno-electron/src/store/workspace-environments.jspackages/bruno-electron/src/utils/workspace-config.jstests/environments/global-env-migration-from-file/fixtures/workspace/collections/test-collection/bruno.jsontests/environments/global-env-migration-from-file/fixtures/workspace/environments/Alpha.ymltests/environments/global-env-migration-from-file/fixtures/workspace/environments/Beta.ymltests/environments/global-env-migration-from-file/fixtures/workspace/workspace.ymltests/environments/global-env-migration-from-file/global-env-migration-from-file.spec.tstests/environments/global-env-migration-from-file/init-user-data/preferences.jsontests/environments/global-env-workspace-persistence/global-env-workspace-persistence.spec.tstests/environments/global-env-workspace-persistence/init-user-data/preferences.jsontests/utils/page/actions.ts
💤 Files with no reviewable changes (2)
- packages/bruno-electron/src/utils/workspace-config.js
- packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/bruno-electron/src/store/global-environments.js (1)
93-114: Add JSDoc to the new workspace-scoped API methods.Please add brief JSDoc for
getActiveGlobalEnvironmentUidForWorkspace,setActiveGlobalEnvironmentUidForWorkspace, andremoveActiveGlobalEnvironmentUidForWorkspace.As per coding guidelines "Add JSDoc comments to abstractions for additional details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/store/global-environments.js` around lines 93 - 114, Add JSDoc blocks above the three new workspace-scoped methods getActiveGlobalEnvironmentUidForWorkspace, setActiveGlobalEnvironmentUidForWorkspace, and removeActiveGlobalEnvironmentUidForWorkspace describing their purpose and usage; include `@param` tags (workspacePath: string) for all methods, an additional `@param` (uid: string|null|undefined) for setActiveGlobalEnvironmentUidForWorkspace, and `@returns` tags for getActiveGlobalEnvironmentUidForWorkspace (string|undefined) and void for the setters/removers, keeping the descriptions concise and matching the existing coding style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-electron/src/store/global-environments.js`:
- Around line 93-114: Normalize workspacePath before using it as an object key
in getActiveGlobalEnvironmentUidForWorkspace,
setActiveGlobalEnvironmentUidForWorkspace, and
removeActiveGlobalEnvironmentUidForWorkspace: compute a single canonical key
(e.g., run the incoming workspacePath through a deterministic normalizer that
resolves/normalizes separators and collapses equivalent forms and applies a
consistent case strategy such as toLowerCase() on platforms or paths meant to be
case-insensitive), then use that canonical key for mapping reads, writes and
deletes instead of the raw workspacePath; ensure the same normalizer is applied
both when reading (get...) and when writing/removing (set... / remove...) so
stored entries are always found and removed reliably.
---
Nitpick comments:
In `@packages/bruno-electron/src/store/global-environments.js`:
- Around line 93-114: Add JSDoc blocks above the three new workspace-scoped
methods getActiveGlobalEnvironmentUidForWorkspace,
setActiveGlobalEnvironmentUidForWorkspace, and
removeActiveGlobalEnvironmentUidForWorkspace describing their purpose and usage;
include `@param` tags (workspacePath: string) for all methods, an additional
`@param` (uid: string|null|undefined) for
setActiveGlobalEnvironmentUidForWorkspace, and `@returns` tags for
getActiveGlobalEnvironmentUidForWorkspace (string|undefined) and void for the
setters/removers, keeping the descriptions concise and matching the existing
coding style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6c848b5-6d57-425e-8a5d-f06c610616ac
📒 Files selected for processing (3)
packages/bruno-electron/src/ipc/global-environments.jspackages/bruno-electron/src/ipc/workspace.jspackages/bruno-electron/src/store/global-environments.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-electron/src/ipc/global-environments.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-electron/src/store/global-environments.js (1)
104-110: Consider using strict equality for uid check.The
uid || nullexpression coerces any falsy value (empty string,0) tonull. If UIDs are always non-empty strings, this is fine. Otherwise, a more explicit check may be warranted.Optional: explicit null handling
setActiveGlobalEnvironmentUidForWorkspace(workspacePath, uid) { if (!workspacePath) return; const key = posixifyPath(workspacePath); const mapping = this.store.get('activeGlobalEnvironmentUidByWorkspace', {}); - mapping[key] = uid || null; + mapping[key] = uid != null ? uid : null; this.store.set('activeGlobalEnvironmentUidByWorkspace', mapping); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/store/global-environments.js` around lines 104 - 110, In setActiveGlobalEnvironmentUidForWorkspace, avoid coercing falsy uids to null via `uid || null`; instead explicitly handle absent values so valid falsy UIDs (e.g., empty string or 0) are preserved — for example assign `mapping[key] = uid == null ? null : uid` (or `typeof uid === 'undefined' ? null : uid`) before calling `this.store.set('activeGlobalEnvironmentUidByWorkspace', mapping)`; this change lives alongside posixifyPath, mapping, and the store.get/store.set usage in that function.
🤖 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-electron/src/store/global-environments.js`:
- Around line 104-110: In setActiveGlobalEnvironmentUidForWorkspace, avoid
coercing falsy uids to null via `uid || null`; instead explicitly handle absent
values so valid falsy UIDs (e.g., empty string or 0) are preserved — for example
assign `mapping[key] = uid == null ? null : uid` (or `typeof uid === 'undefined'
? null : uid`) before calling
`this.store.set('activeGlobalEnvironmentUidByWorkspace', mapping)`; this change
lives alongside posixifyPath, mapping, and the store.get/store.set usage in that
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56bb3815-8305-41da-86c2-fb75a4668790
📒 Files selected for processing (1)
packages/bruno-electron/src/store/global-environments.js
* remove activeEnvironmentUid and migration * fix: no environment handling * fix: standardize workspace path handling
Description
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
Refactor
Tests