-
Couldn't load subscription status.
- Fork 239
fix: try out and revert invalid edit COMMIT-9945 #7464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2cc1194 to
2d0acb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses issues with error handling when applying invalid edits to data modeling diagrams (COMPASS-9945). The key improvements include: (1) validating edits by attempting to build the model before saving, (2) replacing the ineffective APPLY_EDIT_FAILED action with user-facing toast notifications, and (3) adding collection existence checks for edit operations.
Key Changes:
- Introduced toast notifications for edit validation failures instead of storing errors in state
- Added model building validation after applying edits with automatic revert on failure
- Added collection existence checks in
ensureCollectionExiststo catch invalid operations early
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-data-modeling/test/setup-store.tsx | Added openToast parameter to test store setup for dependency injection |
| packages/compass-data-modeling/src/store/reducer.ts | Added openToast to DataModelingExtraArgs type definition |
| packages/compass-data-modeling/src/store/index.ts | Modified store options to include openToast function with default implementation |
| packages/compass-data-modeling/src/store/diagram.ts | Replaced APPLY_EDIT_FAILED action with REVERT_FAILED_EDIT, added model validation after applying edits, and integrated toast notifications |
| packages/compass-data-modeling/src/store/diagram.spec.ts | Updated tests to use spy for openToast and verify toast calls instead of checking editErrors |
| packages/compass-data-modeling/src/store/apply-edit.ts | Added ensureCollectionExists helper function and validation checks for collection operations |
| packages/compass-data-modeling/src/components/diagram-editor.tsx | Removed unused editErrors prop from diagram component |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
|
||
| // try to build the model with the latest edit | ||
| try { | ||
| selectCurrentModelFromState(getState()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to understand why we are calling this function. I don't see how it directly impacts saving of model. Maybe I am missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to prevent saving a model that is corrupted (the edits cannot be build together).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're only validating the edit on it's own, so if an edit is formatted well we save it. But it can be that the edit doesn't work with the rest (for example we try to rename a collection that doesn't exist, or make an unexpected change in the schema).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the impact - there is none, but the function might throw an error, and that's when we start the recovery process (revert & show toast)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks!
Description
Identified several problems related to https://jira.mongodb.org/browse/COMPASS-9945.
One of the problems is that we never try to actually build the model with the new edit before saving it to storage.
Also the
APPLY_EDIT_FAILEDwas a dead end (this was a leftover from the temporary editor and got forgotten since). Instead, we're now showing a toast.Side change: I realised that we silently ignore non existing collections, added checks.
Will come in separate PRs:
Screen.Recording.2025-10-17.at.12.31.55.mov
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes