-
Notifications
You must be signed in to change notification settings - Fork 229
feat(data-modeling): add rename collection to side panel COMPASS-9658 #7174
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
base: main
Are you sure you want to change the base?
Conversation
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 adds the ability to rename collections directly from the side panel in the data modeling workspace. The implementation includes validation to prevent empty names and duplicate collection names, along with proper state management to update selections and relationships when a collection is renamed.
Key changes:
- Added
RenameCollection
edit type with validation and state handling - Implemented collection name editing UI with real-time validation in the side panel
- Updated relationship references when collections are renamed
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/compass-data-modeling/src/store/diagram.ts | Adds RenameCollection action, reducer logic, and selection state management |
packages/compass-data-modeling/src/services/data-model-storage.ts | Defines RenameCollection schema validation |
packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx | Updates existing tests and adds new test coverage for rename functionality |
packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx | Implements editable collection name input with validation |
packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx
Outdated
Show resolved
Hide resolved
|
||
const isDuplicate = namespacesWithoutCurrent.some( | ||
(ns) => | ||
ns === `${toNS(namespace).database}.${collectionName}` || |
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.
The duplicate check logic is redundant - checking both trimmed and untrimmed versions when the second condition will always include the first. Consider simplifying to only check the trimmed version.
ns === `${toNS(namespace).database}.${collectionName}` || |
Copilot uses AI. Check for mistakes.
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 mean, obviously Copilot's suggestion behaves differently from the current logic, but I think you could actually condense this into a single condition if you want to, e.g.
ns.trim() === `${toNS(namespace).database}.${collectionName.trim()}`.trim()
|
||
const isDuplicate = namespacesWithoutCurrent.some( | ||
(ns) => | ||
ns === `${toNS(namespace).database}.${collectionName}` || |
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 mean, obviously Copilot's suggestion behaves differently from the current logic, but I think you could actually condense this into a single condition if you want to, e.g.
ns.trim() === `${toNS(namespace).database}.${collectionName.trim()}`.trim()
COMPASS-9658
Adding e2e tests for this in the next pr for this ticket that has remove collection.
rename.collection.mp4
Note that the vid has the old, revert on onfocus when there's an invalid value. Now we don't revert the text input value. https://docs.google.com/document/d/15Dn5FB3POEj9sYQrYIUajZySqFzlPfpcgjkPhzFqSFg/edit?disco=AAABnqG-O7o