Skip to content

fix: warn before discarding unsaved changes in waveconfig view#3099

Open
majiayu000 wants to merge 4 commits intowavetermdev:mainfrom
majiayu000:fix/issue-2890-warn-unsaved-changes-waveconfig
Open

fix: warn before discarding unsaved changes in waveconfig view#3099
majiayu000 wants to merge 4 commits intowavetermdev:mainfrom
majiayu000:fix/issue-2890-warn-unsaved-changes-waveconfig

Conversation

@majiayu000
Copy link

Fixes #2890

Changes

Add unsaved changes confirmation when navigating away in the Wave Config view. Two navigation paths lose edits without warning:

  1. File sidebar selection: handleFileSelect calls model.loadFile(file) directly, which resets hasEditedAtom and overwrites fileContentAtom — discarding edits silently.

  2. JSON→Visual tab switching: setActiveTab() is called directly. The visual component renders from saved config (not the editor buffer), so edits appear lost.

Fix

  • Added confirmDiscardChanges() to WaveConfigViewModel — checks hasEditedAtom, shows window.confirm() if there are unsaved changes.
  • Guarded handleFileSelect with the confirmation check (skips if already on the selected file).
  • Guarded JSON→Visual tab switch with the confirmation check.

Only prompts when hasEditedAtom is true. Visual→JSON direction is safe (editor loads from fileContentAtom). window.confirm() is synchronous and works well in Electron.

Test Plan

  • Edit JSON, switch file in sidebar → confirmation dialog appears. Cancel stays, confirm switches.
  • Edit JSON, click Visual tab → confirmation dialog appears.
  • Save changes first, then switch → no dialog.
  • Switch without editing → no dialog.
  • npx tsc --noEmit passes.

Add confirmation dialog when navigating away from unsaved changes in the
Wave Config editor. Guards both file sidebar selection and JSON-to-Visual
tab switching with a window.confirm() prompt. No-ops when there are no
pending changes.

Fixes wavetermdev#2890

Signed-off-by: majiayu000 <1835304752@qq.com>
- Simplify Visual tab guard by removing redundant activeTab check
  (confirmDiscardChanges already returns true when no changes exist)
- Document intentional asymmetry: JSON tab button has no guard because
  visual tab saves changes immediately via RPC

Signed-off-by: majiayu000 <1835304752@qq.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 21, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f15068b5-4b1d-4714-ae93-1c907ec64e2b

📥 Commits

Reviewing files that changed from the base of the PR and between d367011 and ec9356b.

📒 Files selected for processing (1)
  • frontend/app/view/waveconfig/waveconfig-model.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/app/view/waveconfig/waveconfig-model.ts

Walkthrough

Adds two methods to WaveConfigViewModel: confirmDiscardChanges() which returns true immediately when no unsaved edits exist or otherwise calls window.confirm(...) with an unsaved-changes prompt, and discardChanges() which restores fileContentAtom from originalContentAtom, clears hasEditedAtom, and resets validationErrorAtom and errorMessageAtom to null. UI handlers in the config sidebar now short-circuit when the clicked file is already selected and call confirmDiscardChanges() before loading a file; the visual-tab switch is guarded by confirmDiscardChanges() and, when confirmed, calls discardChanges() before activating the visual tab.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding a warning before discarding unsaved changes in waveconfig view.
Description check ✅ Passed The description is detailed and directly related to the changeset, explaining the problem, the solution, and the test plan.
Linked Issues check ✅ Passed The PR successfully addresses issue #2890 by adding confirmation dialogs before discarding unsaved changes when navigating away.
Out of Scope Changes check ✅ Passed All changes are directly related to preventing silent loss of unsaved changes in the Wave Config view as required by issue #2890.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2026

CLA assistant check
All committers have signed the CLA.

@majiayu000 majiayu000 marked this pull request as ready for review March 21, 2026 04:53
@kilo-code-bot
Copy link
Contributor

kilo-code-bot bot commented Mar 21, 2026

Code Review Summary

Status: No Issues Found | Recommendation: Merge

The existing comment at line 237 has been addressed - the PR now properly calls model.discardChanges() before switching to the Visual tab. The implementation correctly handles:

  1. File sidebar selection: Guards against selecting the same file and prompts for confirmation before discarding unsaved changes
  2. Visual tab switch: Confirms with user and discards changes before switching
Files Reviewed (2 files)
  • frontend/app/view/waveconfig/waveconfig-model.ts - Added confirmDiscardChanges() and discardChanges() methods
  • frontend/app/view/waveconfig/waveconfig.tsx - Guards file selection and Visual tab switch with confirmation

Reviewed by minimax-m2.5-20260211 · 195,457 tokens

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@frontend/app/view/waveconfig/waveconfig.tsx`:
- Around line 233-236: The click handler currently only switches tabs
(setActiveTab("visual")) after model.confirmDiscardChanges() and does not
actually clear the pending JSON edits, leaving the "Unsaved changes" flag set;
update the handler so that when model.confirmDiscardChanges() returns true you
also reset the JSON editor state—either by calling an existing discard/reset
method on the model (e.g., model.discardChanges() or model.resetDraft()) or by
invoking the component state setter that holds the draft JSON (e.g.,
setEditedJson(originalJson) or setPendingJson(undefined)) before calling
setActiveTab("visual"), ensuring the unsaved/dirty flag is cleared; if no reset
helper exists add one to the model to centralize discard logic and call it here.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f5ee4912-0d13-44c8-a6c6-57081cb47c24

📥 Commits

Reviewing files that changed from the base of the PR and between 8fc4dc3 and 7dc3745.

📒 Files selected for processing (2)
  • frontend/app/view/waveconfig/waveconfig-model.ts
  • frontend/app/view/waveconfig/waveconfig.tsx

When switching from JSON to Visual tab after confirming discard,
also reset fileContent back to original and clear the dirty flag
so the editor doesn't retain stale edits.

Signed-off-by: majiayu000 <1835304752@qq.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
frontend/app/view/waveconfig/waveconfig-model.ts (1)

284-288: Consider clearing validation/error state on discard.

discardChanges() resets content and dirty state, but stale validationErrorAtom / errorMessageAtom can survive after a discard and confuse users when they return to JSON.

Suggested patch
 discardChanges() {
     const originalContent = globalStore.get(this.originalContentAtom);
     globalStore.set(this.fileContentAtom, originalContent);
     globalStore.set(this.hasEditedAtom, false);
+    globalStore.set(this.validationErrorAtom, null);
+    globalStore.set(this.errorMessageAtom, null);
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/waveconfig/waveconfig-model.ts` around lines 284 - 288,
discardChanges() currently restores content and resets hasEditedAtom but leaves
validationErrorAtom and errorMessageAtom stale; update discardChanges() to also
clear validation state by calling globalStore.set on validationErrorAtom (e.g.,
to null or false) and on errorMessageAtom (e.g., to empty string or null) so any
previous JSON errors are removed when content is discarded; ensure you reference
the same atoms (validationErrorAtom, errorMessageAtom, fileContentAtom,
hasEditedAtom, originalContentAtom) and use globalStore.set consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@frontend/app/view/waveconfig/waveconfig-model.ts`:
- Around line 284-288: discardChanges() currently restores content and resets
hasEditedAtom but leaves validationErrorAtom and errorMessageAtom stale; update
discardChanges() to also clear validation state by calling globalStore.set on
validationErrorAtom (e.g., to null or false) and on errorMessageAtom (e.g., to
empty string or null) so any previous JSON errors are removed when content is
discarded; ensure you reference the same atoms (validationErrorAtom,
errorMessageAtom, fileContentAtom, hasEditedAtom, originalContentAtom) and use
globalStore.set consistently.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a81eadc5-2052-45e6-b254-d74b298a1133

📥 Commits

Reviewing files that changed from the base of the PR and between 7dc3745 and d367011.

📒 Files selected for processing (2)
  • frontend/app/view/waveconfig/waveconfig-model.ts
  • frontend/app/view/waveconfig/waveconfig.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/app/view/waveconfig/waveconfig.tsx

Clear validationErrorAtom and errorMessageAtom in discardChanges()
so stale error messages don't persist after discarding edits.

Signed-off-by: majiayu000 <1835304752@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants