feat(core-config): add unsaved changes confirmation dialog#465
Closed
ArthurKoba wants to merge 2 commits into
Closed
feat(core-config): add unsaved changes confirmation dialog#465ArthurKoba wants to merge 2 commits into
ArthurKoba wants to merge 2 commits into
Conversation
- Implement change tracking for core configuration editor - Block modal close when unsaved changes exist - Show confirmation dialog with options to keep editing or discard changes - Add translations for all supported languages (en, ru, fa, zh) - Reset dirty state after successful save operation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughCoreConfigModal now checks react-hook-form's ChangesUnsaved Changes Confirmation Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
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)
dashboard/src/components/dialogs/core-config-modal.tsx (1)
191-208:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential stale dirty-state check can bypass the confirmation dialog.
The close guard depends on
hasUnsavedChanges, which is asynchronously mirrored from form state. A fast edit→close sequence can still readfalseand close without prompting.Suggested fix
- const [hasUnsavedChanges, setHasUnsavedChanges] = useState(false) + const [hasUnsavedChanges, setHasUnsavedChanges] = useState(false) + const [hasUnsavedChangesRef, setHasUnsavedChangesRef] = useState(false) useEffect(() => { const subscription = form.watch(() => { - setHasUnsavedChanges(form.formState.isDirty) + const dirty = form.formState.isDirty + setHasUnsavedChanges(dirty) + setHasUnsavedChangesRef(dirty) }) return () => subscription.unsubscribe() }, [form]) const handleModalCloseWithCheck = useCallback((open: boolean) => { - if (!open && hasUnsavedChanges) { + if (!open && hasUnsavedChangesRef) { setIsConfirmExitDialogOpen(true) } else { onOpenChange(open) if (!open) { setHasUnsavedChanges(false) + setHasUnsavedChangesRef(false) } } - }, [hasUnsavedChanges, onOpenChange]) + }, [hasUnsavedChangesRef, onOpenChange])Also applies to: 218-227
🤖 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 `@dashboard/src/components/dialogs/core-config-modal.tsx` around lines 191 - 208, The local hasUnsavedChanges state can lag behind the form's actual dirty flag and let a fast edit→close bypass the confirm dialog; remove or stop relying on hasUnsavedChanges and instead read the form's current dirty state directly when deciding to prompt (use form.formState.isDirty in the close/guard logic that opens setIsConfirmExitDialogOpen), and update any other checks (e.g., the other close handler around lines 218-227) to use form.formState.isDirty rather than the stale hasUnsavedChanges; keep the form.watch subscription only for UI updates if still needed or remove it entirely to avoid duplication.
🤖 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 `@dashboard/src/components/dialogs/core-config-modal.tsx`:
- Around line 191-208: The local hasUnsavedChanges state can lag behind the
form's actual dirty flag and let a fast edit→close bypass the confirm dialog;
remove or stop relying on hasUnsavedChanges and instead read the form's current
dirty state directly when deciding to prompt (use form.formState.isDirty in the
close/guard logic that opens setIsConfirmExitDialogOpen), and update any other
checks (e.g., the other close handler around lines 218-227) to use
form.formState.isDirty rather than the stale hasUnsavedChanges; keep the
form.watch subscription only for UI updates if still needed or remove it
entirely to avoid duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73bdfa51-0cbf-464b-97ec-ed8aa6d32d18
📒 Files selected for processing (5)
dashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/components/dialogs/core-config-modal.tsx
Use form.formState.isDirty directly instead of mirrored state to prevent race condition where fast edit→close sequence could bypass confirmation dialog. Remove unnecessary state synchronization and simplify logic by reading form state at decision time.
Author
|
I made a change in the wrong branch. I'll fix it now and create a correct pull request from my fork and the dev branch. |
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
1 participant
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.
Solving the issue: #459
Summary by CodeRabbit
Release Notes