feat(environments): split variables and secrets into separate tabs#8191
Open
pooja-bruno wants to merge 6 commits into
Open
feat(environments): split variables and secrets into separate tabs#8191pooja-bruno wants to merge 6 commits into
pooja-bruno wants to merge 6 commits into
Conversation
…earch functionality
Remember and restore selected environment per tab by storing envUid in tabState (EnvironmentSettings, WorkspaceEnvironments) and dispatching updateTabState when selection changes. Ensure collection.activeEnvironmentUid is set in EnvironmentVariablesTable. Improve collections slice handling of environmentsDraft to reset only when switching environments and otherwise update variables in place. UI tweaks: increase DeleteEnvironment modal size to md and remove extra space before question mark, adjust StyledWrapper top margin from 12px to 16px. MenuDropdown/SubMenuItem: propagate selectedItemId and showTickMark, highlight submenu parent when a child is selected. Add .gstack to .gitignore.
Clean up .gitignore by removing the obsolete `.gstack` entry. This removes an unused ignore rule to reflect current repository files and tooling.
pooja-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughSplits Environment Variables and Secrets into separate tabs: UI (ResponsiveTabs), Redux per‑tab state persistence, tab-scoped EnvironmentVariablesTable behavior (save/reset/filtering), layout/styling updates, and Playwright tests validating tab isolation across collection and global environments. ChangesVariables/Secrets Tab Separation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/StyledWrapper.js (1)
28-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
.btn-actionscoping and remove the extra closing brace.
.sidebaris closed before the first.btn-action, and there is an extra}afterward. This creates two competing root-level.btn-actionrules (here and Line 130), making size/hitbox styling ambiguous.Suggested fix
.sidebar { width: 240px; min-width: 240px; display: flex; flex-direction: column; - } - - .btn-action { + + .btn-action { display: flex; align-items: center; justify-content: center; width: 24px; height: 24px; @@ &:hover { background: ${(props) => props.theme.sidebar.collection.item.hoverBg}; color: ${(props) => props.theme.text}; } } }🤖 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/components/Environments/EnvironmentSettings/EnvironmentList/StyledWrapper.js` around lines 28 - 55, The .btn-action rule is incorrectly scoped because .sidebar is closed early and an extra closing brace remains; move the .btn-action block so it is nested inside the .sidebar rule (remove the stray closing brace that currently creates a second root-level .btn-action) and consolidate/remove the duplicate root-level .btn-action definition to ensure size/hitbox styles come only from the nested .sidebar .btn-action selector.
🧹 Nitpick comments (2)
tests/environments/variable-secret-tabs/variable-secret-tabs.spec.ts (1)
10-17: ⚡ Quick winConsider data-testid for search button selector.
The search button is located via
titleattribute and the input via class selector.search-input. While functional,data-testidwould be more stable for E2E tests.🔍 Suggested improvement
Request the component team to add data-testids:
- await page.locator('.env-search-container button[title="Search"]').click(); - const input = page.locator('.search-input'); + await page.getByTestId('env-search-button').click(); + const input = page.getByTestId('env-search-input');🤖 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/variable-secret-tabs/variable-secret-tabs.spec.ts` around lines 10 - 17, The test uses fragile selectors in searchEnv ('.search-input' and '.env-search-container button[title="Search"]'); update the test to use stable data-testid attributes instead (e.g., replace '.search-input' and the button selector with 'data-testid' locators) and if those testids don't exist, ask the component team to add them (for example data-testid="env-search-input" and data-testid="env-search-button"), then change the locator calls in searchEnv to use those data-testid values and keep the existing waitFor/fill logic intact.packages/bruno-app/src/components/Environments/EnvironmentSettings/index.js (1)
13-29: ⚡ Quick winExtract this tab-scoped environment selection/persistence logic into a shared hook.
This block duplicates the same business logic already present in
packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/index.js, which increases drift risk for fallback and persistence behavior.As per coding guidelines: "MUST: Prefer custom hooks for business logic, data fetching, and side-effects in React".
🤖 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/components/Environments/EnvironmentSettings/index.js` around lines 13 - 29, Extract the tab-scoped environment selection/persistence logic (the useMemo computing selectedEnvironment and the setSelectedEnvironment function that dispatches updateTabState using activeTabUid and env.uid) into a shared custom hook (e.g., useTabEnvironment or useTabSelectedEnvironment) and replace the inline logic in both EnvironmentSettings and WorkspaceEnvironments with that hook; the hook should accept environments and collection (or their minimal pieces) and return { selectedEnvironment, setSelectedEnvironment }, internally use useSelector to read activeTabUid and persistedEnvUid and dispatch updateTabState, and preserve the existing fallback order (persistedEnvUid → collection.activeEnvironmentUid → first environment) and no-op behavior when activeTabUid or env.uid are missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/index.js`:
- Around line 202-251: Add stable Playwright selectors by adding data-testid
attributes to the new action and search controls: add data-testid props to the
three ActionIcon instances (the Rename, Copy and Delete buttons) and the Search
ActionIcon (used when isSearchExpanded is false), add data-testid to the search
input element (the input using searchInputRef) and to the clear search button
(the element with class "clear-search"); use descriptive keys like
data-testid="env-rename-btn", "env-copy-btn", "env-delete-btn",
"env-search-btn", "env-search-input", and "env-clear-search-btn" so tests can
reliably locate these controls (ensure ActionIcon forwards unknown props if
needed).
In
`@packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/index.js`:
- Around line 204-253: Add stable Playwright selectors by adding data-testid
attributes to the new action icons and search controls: add data-testid values
on the ActionIcon usages (e.g. the Rename, Copy, Delete and Search icons) such
as data-testid="env-action-rename", "env-action-copy", "env-action-delete",
"env-action-search"; add data-testid="env-search-input" to the input element
rendered when isSearchExpanded is true (the element using searchInputRef and
value/searchQuery), and add data-testid="env-clear-search" to the clear button
that calls handleClearSearch; ensure these attributes are placed on the elements
inside the ResponsiveTabs rightContent so tests can target ActionIcon, the
search input and the clear button reliably.
---
Outside diff comments:
In
`@packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/StyledWrapper.js`:
- Around line 28-55: The .btn-action rule is incorrectly scoped because .sidebar
is closed early and an extra closing brace remains; move the .btn-action block
so it is nested inside the .sidebar rule (remove the stray closing brace that
currently creates a second root-level .btn-action) and consolidate/remove the
duplicate root-level .btn-action definition to ensure size/hitbox styles come
only from the nested .sidebar .btn-action selector.
---
Nitpick comments:
In `@packages/bruno-app/src/components/Environments/EnvironmentSettings/index.js`:
- Around line 13-29: Extract the tab-scoped environment selection/persistence
logic (the useMemo computing selectedEnvironment and the setSelectedEnvironment
function that dispatches updateTabState using activeTabUid and env.uid) into a
shared custom hook (e.g., useTabEnvironment or useTabSelectedEnvironment) and
replace the inline logic in both EnvironmentSettings and WorkspaceEnvironments
with that hook; the hook should accept environments and collection (or their
minimal pieces) and return { selectedEnvironment, setSelectedEnvironment },
internally use useSelector to read activeTabUid and persistedEnvUid and dispatch
updateTabState, and preserve the existing fallback order (persistedEnvUid →
collection.activeEnvironmentUid → first environment) and no-op behavior when
activeTabUid or env.uid are missing.
In `@tests/environments/variable-secret-tabs/variable-secret-tabs.spec.ts`:
- Around line 10-17: The test uses fragile selectors in searchEnv
('.search-input' and '.env-search-container button[title="Search"]'); update the
test to use stable data-testid attributes instead (e.g., replace '.search-input'
and the button selector with 'data-testid' locators) and if those testids don't
exist, ask the component team to add them (for example
data-testid="env-search-input" and data-testid="env-search-button"), then change
the locator calls in searchEnv to use those data-testid values and keep the
existing waitFor/fill logic intact.
🪄 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: 01332825-d840-47c0-a93f-2a768956c2dc
📒 Files selected for processing (19)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/DeleteEnvironment/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/StyledWrapper.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/StyledWrapper.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/index.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/DeleteEnvironment/index.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/index.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/providers/ReduxStore/slices/tabs.jspackages/bruno-app/src/ui/MenuDropdown/SubMenuItem/index.jspackages/bruno-app/src/ui/MenuDropdown/index.jstests/environments/variable-secret-tabs/variable-secret-tabs.spec.tstests/utils/page/actions.ts
Comment on lines
+202
to
+251
| <ActionIcon label="Rename" onClick={handleRenameClick}> | ||
| <IconEdit size={15} strokeWidth={1.5} /> | ||
| </button> | ||
| <button onClick={() => setOpenCopyModal(true)} title="Copy"> | ||
| </ActionIcon> | ||
| <ActionIcon label="Copy" onClick={() => setOpenCopyModal(true)}> | ||
| <IconCopy size={15} strokeWidth={1.5} /> | ||
| </button> | ||
| <button onClick={() => setOpenDeleteModal(true)} title="Delete"> | ||
| </ActionIcon> | ||
| <ActionIcon label="Delete" onClick={() => setOpenDeleteModal(true)} colorOnHover="danger"> | ||
| <IconTrash size={15} strokeWidth={1.5} /> | ||
| </button> | ||
| </ActionIcon> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="tabs-container"> | ||
| <ResponsiveTabs | ||
| tabs={TABS} | ||
| activeTab={activeTab} | ||
| onTabSelect={setActiveTab} | ||
| rightContent={( | ||
| <div ref={rightContentRef} className="env-search-container"> | ||
| {isSearchExpanded ? ( | ||
| <div className="search-input-wrapper"> | ||
| <IconSearch size={14} strokeWidth={1.5} className="search-icon" /> | ||
| <input | ||
| ref={searchInputRef} | ||
| type="text" | ||
| placeholder={activeTab === 'secrets' ? 'Search secrets...' : 'Search variables...'} | ||
| value={searchQuery} | ||
| onChange={(e) => setSearchQuery(e.target.value)} | ||
| onBlur={handleSearchBlur} | ||
| className="search-input" | ||
| autoComplete="off" | ||
| autoCorrect="off" | ||
| autoCapitalize="off" | ||
| spellCheck="false" | ||
| /> | ||
| {searchQuery && ( | ||
| <button | ||
| className="clear-search" | ||
| onClick={handleClearSearch} | ||
| onMouseDown={(e) => e.preventDefault()} | ||
| title="Clear search" | ||
| > | ||
| <IconX size={14} strokeWidth={1.5} /> | ||
| </button> | ||
| )} | ||
| </div> | ||
| ) : ( | ||
| <ActionIcon label="Search" onClick={handleSearchIconClick}> | ||
| <IconSearch size={15} strokeWidth={1.5} /> | ||
| </ActionIcon> |
Contributor
There was a problem hiding this comment.
Add stable Playwright selectors to the new actions and search controls.
The new testable controls introduced here (Rename/Copy/Delete/Search actions, search input, clear button) should expose data-testid to avoid brittle locator strategies.
Suggested update
- <ActionIcon label="Rename" onClick={handleRenameClick}>
+ <ActionIcon data-testid="env-rename-action" label="Rename" onClick={handleRenameClick}>
<IconEdit size={15} strokeWidth={1.5} />
</ActionIcon>
- <ActionIcon label="Copy" onClick={() => setOpenCopyModal(true)}>
+ <ActionIcon data-testid="env-copy-action" label="Copy" onClick={() => setOpenCopyModal(true)}>
<IconCopy size={15} strokeWidth={1.5} />
</ActionIcon>
- <ActionIcon label="Delete" onClick={() => setOpenDeleteModal(true)} colorOnHover="danger">
+ <ActionIcon data-testid="env-delete-action" label="Delete" onClick={() => setOpenDeleteModal(true)} colorOnHover="danger">
<IconTrash size={15} strokeWidth={1.5} />
</ActionIcon>
...
- <input
+ <input
+ data-testid="env-tab-search-input"
ref={searchInputRef}
...
- <button
+ <button
+ data-testid="env-tab-search-clear"
className="clear-search"
...
- <ActionIcon label="Search" onClick={handleSearchIconClick}>
+ <ActionIcon data-testid="env-search-action" label="Search" onClick={handleSearchIconClick}>
<IconSearch size={15} strokeWidth={1.5} />
</ActionIcon>As per coding guidelines, "Add data-testid to testable elements for Playwright in React components".
🤖 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/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/index.js`
around lines 202 - 251, Add stable Playwright selectors by adding data-testid
attributes to the new action and search controls: add data-testid props to the
three ActionIcon instances (the Rename, Copy and Delete buttons) and the Search
ActionIcon (used when isSearchExpanded is false), add data-testid to the search
input element (the input using searchInputRef) and to the clear search button
(the element with class "clear-search"); use descriptive keys like
data-testid="env-rename-btn", "env-copy-btn", "env-delete-btn",
"env-search-btn", "env-search-input", and "env-clear-search-btn" so tests can
reliably locate these controls (ensure ActionIcon forwards unknown props if
needed).
Comment on lines
+204
to
+253
| <ActionIcon label="Rename" onClick={handleRenameClick}> | ||
| <IconEdit size={15} strokeWidth={1.5} /> | ||
| </button> | ||
| <button onClick={() => setOpenCopyModal(true)} title="Copy"> | ||
| </ActionIcon> | ||
| <ActionIcon label="Copy" onClick={() => setOpenCopyModal(true)}> | ||
| <IconCopy size={15} strokeWidth={1.5} /> | ||
| </button> | ||
| <button onClick={() => setOpenDeleteModal(true)} title="Delete"> | ||
| </ActionIcon> | ||
| <ActionIcon label="Delete" onClick={() => setOpenDeleteModal(true)} colorOnHover="danger"> | ||
| <IconTrash size={15} strokeWidth={1.5} /> | ||
| </button> | ||
| </ActionIcon> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="tabs-container"> | ||
| <ResponsiveTabs | ||
| tabs={TABS} | ||
| activeTab={activeTab} | ||
| onTabSelect={setActiveTab} | ||
| rightContent={( | ||
| <div ref={rightContentRef} className="env-search-container"> | ||
| {isSearchExpanded ? ( | ||
| <div className="search-input-wrapper"> | ||
| <IconSearch size={14} strokeWidth={1.5} className="search-icon" /> | ||
| <input | ||
| ref={searchInputRef} | ||
| type="text" | ||
| placeholder={activeTab === 'secrets' ? 'Search secrets...' : 'Search variables...'} | ||
| value={searchQuery} | ||
| onChange={(e) => setSearchQuery(e.target.value)} | ||
| onBlur={handleSearchBlur} | ||
| className="search-input" | ||
| autoComplete="off" | ||
| autoCorrect="off" | ||
| autoCapitalize="off" | ||
| spellCheck="false" | ||
| /> | ||
| {searchQuery && ( | ||
| <button | ||
| className="clear-search" | ||
| onClick={handleClearSearch} | ||
| onMouseDown={(e) => e.preventDefault()} | ||
| title="Clear search" | ||
| > | ||
| <IconX size={14} strokeWidth={1.5} /> | ||
| </button> | ||
| )} | ||
| </div> | ||
| ) : ( | ||
| <ActionIcon label="Search" onClick={handleSearchIconClick}> | ||
| <IconSearch size={15} strokeWidth={1.5} /> | ||
| </ActionIcon> |
Contributor
There was a problem hiding this comment.
Add stable Playwright selectors to the new actions and tab search controls.
The newly introduced controls here should include data-testid (actions/search input/clear) so tests don’t depend on icon or layout details.
As per coding guidelines, "Add data-testid to testable elements for Playwright in React components".
🤖 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/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/index.js`
around lines 204 - 253, Add stable Playwright selectors by adding data-testid
attributes to the new action icons and search controls: add data-testid values
on the ActionIcon usages (e.g. the Rename, Copy, Delete and Search icons) such
as data-testid="env-action-rename", "env-action-copy", "env-action-delete",
"env-action-search"; add data-testid="env-search-input" to the input element
rendered when isSearchExpanded is true (the element using searchInputRef and
value/searchQuery), and add data-testid="env-clear-search" to the clear button
that calls handleClearSearch; ensure these attributes are placed on the elements
inside the ResponsiveTabs rightContent so tests can target ActionIcon, the
search input and the clear button reliably.
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-app/src/components/EnvironmentVariablesTable/index.js`:
- Around line 550-592: The comparison and reset must use the post-persistence
shape that blanks secret values: compute a normalizedPersisted =
persistedVariables.map(v => v.secret ? {...v, value: ''} : v) and use
JSON.stringify(normalizedPersisted.map(stripEnvVarUid)) when computing
hasChanges (instead of using raw persistedVariables), and pass
cloneDeep(normalizedPersisted) into onSave resolution and into formik.resetForm
(appending the new empty row) so the form state matches the persisted
environment shape; update references to persistedVariables ->
normalizedPersisted in hasChanges, onSave(...), and formik.resetForm(...) while
keeping orderVarsBySecret, stripEnvVarUid, savedValues, namedValues, onSave, and
formik.resetForm as the anchors.
🪄 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: 66ec8711-c805-432e-b456-66120bc61c9b
📒 Files selected for processing (5)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/StyledWrapper.jspackages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/index.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/StyledWrapper.jspackages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/index.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/StyledWrapper.js
- packages/bruno-app/src/components/WorkspaceHome/WorkspaceEnvironments/EnvironmentList/EnvironmentDetails/index.js
- packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/index.js
- packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/StyledWrapper.js
Comment on lines
+550
to
+592
| const persistedVariables = orderVarsBySecret(namedValues); | ||
|
|
||
| const hasChanges | ||
| = JSON.stringify(persistedVariables.map(stripEnvVarUid)) !== JSON.stringify(savedValues.map(stripEnvVarUid)); | ||
| if (!hasChanges) { | ||
| toast.error('No changes to save'); | ||
| return; | ||
| } | ||
|
|
||
| const hasValidationErrors = namedValues.some((variable) => { | ||
| if (!variable.name || variable.name.trim() === '') { | ||
| return true; | ||
| } | ||
| ]; | ||
| formik.resetForm({ values: resetValues }); | ||
| setIsModified(false); | ||
| }, [environment.variables, setIsModified]); | ||
| if (!variableNameRegex.test(variable.name)) { | ||
| return true; | ||
| } | ||
| return false; | ||
| }); | ||
|
|
||
| if (hasValidationErrors) { | ||
| toast.error('Please fix validation errors before saving'); | ||
| return; | ||
| } | ||
|
|
||
| onSave(cloneDeep(persistedVariables)) | ||
| .then(() => { | ||
| toast.success('Changes saved successfully'); | ||
| onDraftClear(); | ||
|
|
||
| formik.resetForm({ | ||
| values: [ | ||
| ...persistedVariables, | ||
| { | ||
| uid: uuid(), | ||
| name: '', | ||
| value: '', | ||
| type: 'text', | ||
| secret: isSecretTab, | ||
| enabled: true | ||
| } | ||
| ] | ||
| }); | ||
| setIsModified(false); |
Contributor
There was a problem hiding this comment.
Normalize secret rows to the persisted shape before diffing and resetting.
stripEnvVarUid() includes value, but the environment persistence contract blanks secret values (value: ''). Here handleSaveAll() compares and then resetForm() with raw namedValues, so a successful save can leave the form holding the entered secret while environment.variables comes back blank. That makes the table dirty again immediately and keeps re-saving an unchanged secret payload. Compare/reset against the post-persistence shape instead of the raw Formik values.
🤖 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/components/EnvironmentVariablesTable/index.js` around
lines 550 - 592, The comparison and reset must use the post-persistence shape
that blanks secret values: compute a normalizedPersisted =
persistedVariables.map(v => v.secret ? {...v, value: ''} : v) and use
JSON.stringify(normalizedPersisted.map(stripEnvVarUid)) when computing
hasChanges (instead of using raw persistedVariables), and pass
cloneDeep(normalizedPersisted) into onSave resolution and into formik.resetForm
(appending the new empty row) so the form state matches the persisted
environment shape; update references to persistedVariables ->
normalizedPersisted in hasChanges, onSave(...), and formik.resetForm(...) while
keeping orderVarsBySecret, stripEnvVarUid, savedValues, namedValues, onSave, and
formik.resetForm as the anchors.
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
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
New Features
Improvements
Bug Fixes
Tests