fix(environment)/auto save on env table removing some characters#8732
fix(environment)/auto save on env table removing some characters#8732sachin-thakur-bruno wants to merge 5 commits into
Conversation
WalkthroughEnvironment variable forms now reconcile saved snapshots explicitly instead of relying on Formik reinitialization, preserving in-progress edits during autosave and external updates. Unit and Playwright tests cover reconciliation decisions and persisted typed values. ChangesEnvironment autosave reconciliation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Preferences
participant EnvironmentVariablesTable
participant Formik
participant EnvironmentFile
User->>Preferences: Enable autosave and set interval
Preferences-->>User: Close preferences
User->>Formik: Type environment edits
Formik->>EnvironmentFile: Autosave typed values
EnvironmentFile->>EnvironmentVariablesTable: Emit saved snapshot
EnvironmentVariablesTable->>EnvironmentVariablesTable: Reconcile saved and current values
EnvironmentVariablesTable->>Formik: Reset only for clean form
Formik-->>User: Preserve in-flight edits
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
sachin-thakur-bruno
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tests/environments/environment-tabs/autosave-typing-race.spec.ts`:
- Around line 17-30: Move the autosave interaction from the spec-local
enableAutosave helper into the appropriate centralized page module under
tests/utils/page, including the `#autoSaveEnabled`, `#autoSaveInterval`,
.request-tab, and .close-icon locators. Expose an enableAutosave-style page
action that performs the preference updates, debounce wait, and tab close, then
have the spec reuse that action instead of defining raw 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 Plus
Run ID: 8d762762-9446-4064-bc8e-8d81e34de790
📒 Files selected for processing (4)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.jspackages/bruno-app/src/components/EnvironmentVariablesTable/reconcile.jspackages/bruno-app/src/components/EnvironmentVariablesTable/reconcile.spec.jstests/environments/environment-tabs/autosave-typing-race.spec.ts
…uno/bruno into fix/autosave-env-tables
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/environments/environment-tabs/autosave-typing-race.spec.ts (1)
90-98: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPoll for the complete persisted snapshot.
The poll stops as soon as
NAMEis present, then immediately checksVALUEandDESCRIPTION. Autosave may still be writing those later fields, causing a flaky or false failure. Require all three values in the polling predicate before asserting the final content.As per path instructions, use auto-retrying
expectfor the complete asynchronous outcome.Suggested fix
- await expect - .poll(() => readEnvFile(collectionsDir, collectionName, envName), { timeout: 8000 }) - .toContain(NAME); + await expect + .poll(() => { + const content = readEnvFile(collectionsDir, collectionName, envName); + return content.includes(NAME) && content.includes(VALUE) && content.includes(DESCRIPTION); + }, { timeout: 8000 }) + .toBe(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 `@tests/environments/environment-tabs/autosave-typing-race.spec.ts` around lines 90 - 98, Update the autosave verification step in the test to poll until the persisted environment content contains NAME, VALUE, and DESCRIPTION together. Move the complete-content checks into the auto-retrying expect predicate, then retain only the final assertions on the content returned after polling.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.
Outside diff comments:
In `@tests/environments/environment-tabs/autosave-typing-race.spec.ts`:
- Around line 90-98: Update the autosave verification step in the test to poll
until the persisted environment content contains NAME, VALUE, and DESCRIPTION
together. Move the complete-content checks into the auto-retrying expect
predicate, then retain only the final assertions on the content returned after
polling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d0745e79-4feb-4472-8d2a-e23c81246e66
📒 Files selected for processing (2)
tests/environments/environment-tabs/autosave-typing-race.spec.tstests/utils/page/preferences.ts
| expect(content).toContain(DESCRIPTION); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These test looks good adding a one more test for multi-line for name & description would be helpful with slightly lesser time with auto save and with simulating enter press would be more good
|
|
||
| // The form already matches the incoming snapshot (e.g. our own save landing). | ||
| if (current === nextSaved) return 'noop'; | ||
|
|
There was a problem hiding this comment.
Can we combine the checks into a single condition, such as if (prevSaved === nextSaved || current === nextSaved).
Additionally, can we move this logic directly inside EnvironmentVariablesTable itself
2 participants
Description
BRU-3655
With autosave enabled, typing quickly in the environment variables table could lose
characters. When an autosave completed while the user was still typing, the form
reset itself back to the just-saved (older) snapshot, discarding anything typed
during the save's async window.
Root cause: Formik's
enableReinitialize: trueonEnvironmentVariablesTable. ItsinitialValuesare derived fromenvironment.variables, so every change to thesaved snapshot blindly reset the form — including the app's own autosave echoing back
through Redux. It never checked whether the user had unsaved edits.
This affected both the collection and global/workspace environment tables, since they
render the same component.
Fix
Turn off
enableReinitializeand reconcile explicitly: adopt the incoming snapshotonly when the form has no unsaved edits. If the user has typed ahead, keep their
edits — the next draft/autosave cycle persists them, so nothing typed during a save is
lost.
The decision is a small pure helper,
reconcileSavedChange, called from an effect onthe saved snapshot
Testing
Unit — covers each reconcile branch, including the data-loss case (typed ahead
during a save) and a script env update landing while the user is editing.
E2E — types into Name, Value and Description with a 600ms per-keystroke delay
against autosave at its 500ms minimum, so a full autosave cycle completes between every
keystroke. Asserts the typed text survives in the UI and is persisted intact to disk.
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