From b63e03cff82cd18ea44f314d1ddab1fb7efe186d Mon Sep 17 00:00:00 2001 From: Rhys Howell Date: Thu, 7 Aug 2025 00:40:45 -0400 Subject: [PATCH 1/7] feat(data-modeling): add rename collection to side panel --- .../drawer/collection-drawer-content.tsx | 87 +++++++++++++++++-- .../drawer/diagram-editor-side-panel.spec.tsx | 38 ++++++-- .../src/services/data-model-storage.ts | 5 ++ .../src/store/diagram.ts | 56 ++++++++++++ 4 files changed, 174 insertions(+), 12 deletions(-) diff --git a/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx b/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx index e72e4ad2f8c..017163330c9 100644 --- a/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx +++ b/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx @@ -1,5 +1,6 @@ -import React from 'react'; +import React, { useCallback, useLayoutEffect, useMemo, useState } from 'react'; import { connect } from 'react-redux'; +import toNS from 'mongodb-ns'; import type { Relationship } from '../../services/data-model-storage'; import { Badge, @@ -18,6 +19,7 @@ import { getCurrentDiagramFromState, selectCurrentModel, selectRelationship, + renameCollection, } from '../../store/diagram'; import type { DataModelingState } from '../../store/reducer'; import { getRelationshipName } from '../../utils'; @@ -25,10 +27,12 @@ import DMDrawerSection from './dm-drawer-section'; type CollectionDrawerContentProps = { namespace: string; + namespaces: string[]; relationships: Relationship[]; onCreateNewRelationshipClick: (namespace: string) => void; onEditRelationshipClick: (rId: string) => void; onDeleteRelationshipClick: (rId: string) => void; + onRenameCollection: (fromNS: string, toNS: string) => void; }; const formFieldContainerStyles = css({ @@ -68,15 +72,80 @@ const relationshipContentStyles = css({ marginTop: spacing[400], }); +export function getIsCollectionNameValid( + collectionName: string, + namespaces: string[], + namespace: string +): { + isValid: boolean; + errorMessage?: string; +} { + if (collectionName.trim().length === 0) { + return { + isValid: false, + errorMessage: 'Collection name cannot be empty.', + }; + } + + const namespacesWithoutCurrent = namespaces.filter((ns) => ns !== namespace); + + const isDuplicate = namespacesWithoutCurrent.some( + (ns) => + ns === `${toNS(namespace).database}.${collectionName}` || + ns === `${toNS(namespace).database}.${collectionName.trim()}` + ); + + return { + isValid: !isDuplicate, + errorMessage: isDuplicate ? 'Collection name must be unique.' : undefined, + }; +} + const CollectionDrawerContent: React.FunctionComponent< CollectionDrawerContentProps > = ({ namespace, + namespaces, relationships, onCreateNewRelationshipClick, onEditRelationshipClick, onDeleteRelationshipClick, + onRenameCollection, }) => { + const [collectionName, setCollectionName] = useState( + () => toNS(namespace).collection + ); + + const isCollectionNameValid = useMemo( + () => getIsCollectionNameValid(collectionName, namespaces, namespace), + [collectionName, namespaces, namespace] + ); + + useLayoutEffect(() => { + setCollectionName(toNS(namespace).collection); + }, [namespace]); + + const onBlurCollectionName = useCallback(() => { + const trimmedName = collectionName.trim(); + if (trimmedName === toNS(namespace).collection) { + return; + } + + if (!isCollectionNameValid.isValid) { + // Reset to previous valid name. + // TODO: Maybe we don't reset. + setCollectionName(toNS(namespace).collection); + return; + } + + onRenameCollection(namespace, `${toNS(namespace).database}.${trimmedName}`); + }, [ + collectionName, + namespace, + onRenameCollection, + isCollectionNameValid.isValid, + ]); + return ( <> @@ -84,8 +153,13 @@ const CollectionDrawerContent: React.FunctionComponent< { + setCollectionName(e.target.value); + }} + onBlur={onBlurCollectionName} /> @@ -155,20 +229,21 @@ const CollectionDrawerContent: React.FunctionComponent< export default connect( (state: DataModelingState, ownProps: { namespace: string }) => { + const model = selectCurrentModel(getCurrentDiagramFromState(state).edits); return { - relationships: selectCurrentModel( - getCurrentDiagramFromState(state).edits - ).relationships.filter((r) => { + relationships: model.relationships.filter((r) => { const [local, foreign] = r.relationship; return ( local.ns === ownProps.namespace || foreign.ns === ownProps.namespace ); }), + namespaces: model.collections.map((c) => c.ns), }; }, { onCreateNewRelationshipClick: createNewRelationship, onEditRelationshipClick: selectRelationship, onDeleteRelationshipClick: deleteRelationship, + onRenameCollection: renameCollection, } )(CollectionDrawerContent); diff --git a/packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx b/packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx index 9faf342a756..9a237224963 100644 --- a/packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx +++ b/packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx @@ -19,6 +19,7 @@ import { import dataModel from '../../../test/fixtures/data-model-with-relationships.json'; import type { MongoDBDataModelDescription, + DataModelCollection, Relationship, } from '../../services/data-model-storage'; import { DrawerAnchor } from '@mongodb-js/compass-components'; @@ -76,7 +77,7 @@ describe('DiagramEditorSidePanel', function () { await waitFor(() => { const nameInput = screen.getByLabelText('Name'); expect(nameInput).to.be.visible; - expect(nameInput).to.have.value('flights.airlines'); + expect(nameInput).to.have.value('airlines'); }); }); @@ -131,14 +132,14 @@ describe('DiagramEditorSidePanel', function () { result.plugin.store.dispatch(selectCollection('flights.airlines')); await waitFor(() => { - expect(screen.getByLabelText('Name')).to.have.value('flights.airlines'); + expect(screen.getByLabelText('Name')).to.have.value('airlines'); }); result.plugin.store.dispatch( selectCollection('flights.airports_coordinates_for_schema') ); expect(screen.getByLabelText('Name')).to.have.value( - 'flights.airports_coordinates_for_schema' + 'airports_coordinates_for_schema' ); result.plugin.store.dispatch( @@ -160,7 +161,7 @@ describe('DiagramEditorSidePanel', function () { ).to.be.visible; result.plugin.store.dispatch(selectCollection('flights.planes')); - expect(screen.getByLabelText('Name')).to.have.value('flights.planes'); + expect(screen.getByLabelText('Name')).to.have.value('planes'); }); it('should open and edit relationship starting from collection', async function () { @@ -168,7 +169,7 @@ describe('DiagramEditorSidePanel', function () { result.plugin.store.dispatch(selectCollection('flights.countries')); await waitFor(() => { - expect(screen.getByLabelText('Name')).to.have.value('flights.countries'); + expect(screen.getByLabelText('Name')).to.have.value('countries'); }); // Open relationshipt editing form @@ -217,7 +218,7 @@ describe('DiagramEditorSidePanel', function () { result.plugin.store.dispatch(selectCollection('flights.countries')); await waitFor(() => { - expect(screen.getByLabelText('Name')).to.have.value('flights.countries'); + expect(screen.getByLabelText('Name')).to.have.value('countries'); }); // Find the relationhip item @@ -235,4 +236,29 @@ describe('DiagramEditorSidePanel', function () { expect(screen.queryByText('Airport Country')).not.to.exist; }); }); + + it('should open and edit a collection name', async function () { + const result = renderDrawer(); + result.plugin.store.dispatch(selectCollection('flights.countries')); + + await waitFor(() => { + expect(screen.getByLabelText('Name')).to.have.value('countries'); + }); + + // Update the name. + userEvent.clear(screen.getByLabelText('Name')); + userEvent.type(screen.getByLabelText('Name'), 'pineapple'); + + // Blur/unfocus the input. + userEvent.click(document.body); + + // Check the name in the model. + const modifiedCollection = selectCurrentModel( + getCurrentDiagramFromState(result.plugin.store.getState()).edits + ).collections.find((c: DataModelCollection) => { + return c.ns === 'flights.pineapple'; + }); + + expect(modifiedCollection).to.exist; + }); }); diff --git a/packages/compass-data-modeling/src/services/data-model-storage.ts b/packages/compass-data-modeling/src/services/data-model-storage.ts index 8d95e8a1e40..da1fb0116b4 100644 --- a/packages/compass-data-modeling/src/services/data-model-storage.ts +++ b/packages/compass-data-modeling/src/services/data-model-storage.ts @@ -65,6 +65,11 @@ const EditSchemaVariants = z.discriminatedUnion('type', [ ns: z.string(), newPosition: z.tuple([z.number(), z.number()]), }), + z.object({ + type: z.literal('RenameCollection'), + fromNS: z.string(), + toNS: z.string(), + }), ]); export const EditSchema = z.intersection(EditSchemaBase, EditSchemaVariants); diff --git a/packages/compass-data-modeling/src/store/diagram.ts b/packages/compass-data-modeling/src/store/diagram.ts index d1d487d9d23..aba427a0373 100644 --- a/packages/compass-data-modeling/src/store/diagram.ts +++ b/packages/compass-data-modeling/src/store/diagram.ts @@ -352,6 +352,34 @@ export function moveCollection( return applyEdit(edit); } +export function renameCollection( + fromNS: string, + toNS: string +): DataModelingThunkAction< + void, + ApplyEditAction | ApplyEditFailedAction | CollectionSelectedAction +> { + return (dispatch) => { + const edit: Omit< + Extract, + 'id' | 'timestamp' + > = { + type: 'RenameCollection', + fromNS, + toNS, + }; + + dispatch(applyEdit(edit)); + + // When renaming we want to keep the renamed collection selected in the UI, + // the ID is the collection name. + dispatch({ + type: DiagramActionTypes.COLLECTION_SELECTED, + namespace: toNS, + }); + }; +} + export function applyEdit( rawEdit: EditAction ): DataModelingThunkAction { @@ -541,6 +569,34 @@ function _applyEdit(edit: Edit, model?: StaticModel): StaticModel { }), }; } + case 'RenameCollection': { + return { + ...model, + // Update relationships to point to the renamed namespace. + relationships: model.relationships.map((relationship) => { + const [local, foreign] = relationship.relationship; + + return { + ...relationship, + relationship: [ + { + ...local, + ns: local.ns === edit.fromNS ? edit.toNS : local.ns, + }, + { + ...foreign, + ns: foreign.ns === edit.fromNS ? edit.toNS : foreign.ns, + }, + ], + }; + }), + collections: model.collections.map((collection) => ({ + ...collection, + // Rename the collection. + ns: collection.ns === edit.fromNS ? edit.toNS : collection.ns, + })), + }; + } default: { return model; } From 975bb26d6e4b865b6b76e90324b933a6ed79837c Mon Sep 17 00:00:00 2001 From: Rhys Howell Date: Thu, 7 Aug 2025 00:54:49 -0400 Subject: [PATCH 2/7] fixup: better naming, add test for invalid state --- .../drawer/collection-drawer-content.tsx | 18 +++-- .../drawer/diagram-editor-side-panel.spec.tsx | 67 +++++++++++++++++++ 2 files changed, 75 insertions(+), 10 deletions(-) diff --git a/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx b/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx index 017163330c9..6ce00f63bf8 100644 --- a/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx +++ b/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx @@ -116,7 +116,10 @@ const CollectionDrawerContent: React.FunctionComponent< () => toNS(namespace).collection ); - const isCollectionNameValid = useMemo( + const { + isValid: isCollectionNameValid, + errorMessage: collectionNameEditErrorMessage, + } = useMemo( () => getIsCollectionNameValid(collectionName, namespaces, namespace), [collectionName, namespaces, namespace] ); @@ -131,7 +134,7 @@ const CollectionDrawerContent: React.FunctionComponent< return; } - if (!isCollectionNameValid.isValid) { + if (!isCollectionNameValid) { // Reset to previous valid name. // TODO: Maybe we don't reset. setCollectionName(toNS(namespace).collection); @@ -139,12 +142,7 @@ const CollectionDrawerContent: React.FunctionComponent< } onRenameCollection(namespace, `${toNS(namespace).database}.${trimmedName}`); - }, [ - collectionName, - namespace, - onRenameCollection, - isCollectionNameValid.isValid, - ]); + }, [collectionName, namespace, onRenameCollection, isCollectionNameValid]); return ( <> @@ -154,8 +152,8 @@ const CollectionDrawerContent: React.FunctionComponent< label="Name" sizeVariant="small" value={collectionName} - state={isCollectionNameValid.isValid ? undefined : 'error'} - errorMessage={isCollectionNameValid.errorMessage} + state={isCollectionNameValid ? undefined : 'error'} + errorMessage={collectionNameEditErrorMessage} onChange={(e) => { setCollectionName(e.target.value); }} diff --git a/packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx b/packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx index 9a237224963..64236dfca14 100644 --- a/packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx +++ b/packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx @@ -261,4 +261,71 @@ describe('DiagramEditorSidePanel', function () { expect(modifiedCollection).to.exist; }); + + it('should prevent editing to an empty collection name', async function () { + const result = renderDrawer(); + result.plugin.store.dispatch(selectCollection('flights.countries')); + + await waitFor(() => { + expect(screen.getByLabelText('Name')).to.have.value('countries'); + expect(screen.getByLabelText('Name')).to.have.attribute( + 'aria-invalid', + 'false' + ); + }); + + userEvent.clear(screen.getByLabelText('Name')); + + await waitFor(() => { + expect(screen.getByLabelText('Name')).to.have.attribute( + 'aria-invalid', + 'true' + ); + }); + + // Blur/unfocus the input. + userEvent.click(document.body); + + const notModifiedCollection = selectCurrentModel( + getCurrentDiagramFromState(result.plugin.store.getState()).edits + ).collections.find((c: DataModelCollection) => { + return c.ns === 'flights.countries'; + }); + + expect(notModifiedCollection).to.exist; + }); + + it('should prevent editing to a duplicate collection name', async function () { + const result = renderDrawer(); + result.plugin.store.dispatch(selectCollection('flights.countries')); + + await waitFor(() => { + expect(screen.getByLabelText('Name')).to.have.value('countries'); + expect(screen.getByLabelText('Name')).to.have.attribute( + 'aria-invalid', + 'false' + ); + }); + + userEvent.clear(screen.getByLabelText('Name')); + userEvent.type(screen.getByLabelText('Name'), 'airlines'); + + await waitFor(() => { + expect(screen.getByLabelText('Name')).to.have.attribute( + 'aria-invalid', + 'true' + ); + }); + + // Blur/unfocus the input. + userEvent.click(document.body); + + const notModifiedCollection = selectCurrentModel( + getCurrentDiagramFromState(result.plugin.store.getState()).edits + ).collections.find((c: DataModelCollection) => { + return c.ns === 'flights.countries'; + }); + + expect(notModifiedCollection).to.exist; + }); }); From 0a62d72ae3007d04ea618d1bc50320c7ac656ba7 Mon Sep 17 00:00:00 2001 From: Rhys Howell Date: Fri, 8 Aug 2025 13:27:38 -0400 Subject: [PATCH 3/7] fixup: add selection update from edit case --- .../src/store/diagram.ts | 56 +++++++++++++++---- 1 file changed, 45 insertions(+), 11 deletions(-) diff --git a/packages/compass-data-modeling/src/store/diagram.ts b/packages/compass-data-modeling/src/store/diagram.ts index c90f77e8955..f55ceb52054 100644 --- a/packages/compass-data-modeling/src/store/diagram.ts +++ b/packages/compass-data-modeling/src/store/diagram.ts @@ -180,7 +180,7 @@ export const diagramReducer: Reducer = ( }; } if (isAction(action, DiagramActionTypes.APPLY_EDIT)) { - const newState = { + return { ...state, edits: { prev: [...state.edits.prev, state.edits.current], @@ -189,17 +189,11 @@ export const diagramReducer: Reducer = ( }, editErrors: undefined, updatedAt: new Date().toISOString(), + selectedItems: updateSelectedItemsFromAppliedEdit( + state.selectedItems, + action.edit + ), }; - - if ( - action.edit.type === 'RemoveRelationship' && - state.selectedItems?.type === 'relationship' && - state.selectedItems.id === action.edit.relationshipId - ) { - newState.selectedItems = null; - } - - return newState; } if (isAction(action, DiagramActionTypes.APPLY_EDIT_FAILED)) { return { @@ -261,6 +255,46 @@ export const diagramReducer: Reducer = ( return state; }; +/** + * When an edit impacts the selected item we sometimes need to update + * the selection to reflect that, for instance when renaming a + * collection we update the selection `id` to the new name. + */ +const updateSelectedItemsFromAppliedEdit = ( + currentSelection: SelectedItems | null, + edit: Edit +): SelectedItems | null => { + if (!currentSelection) { + return currentSelection; + } + + switch (edit.type) { + case 'RemoveRelationship': { + if ( + currentSelection?.type === 'relationship' && + currentSelection.id === edit.relationshipId + ) { + return null; + } + break; + } + case 'RenameCollection': { + if ( + currentSelection?.type === 'collection' && + currentSelection.id === edit.fromNS + ) { + return { + type: 'collection', + id: edit.toNS, + }; + } + break; + } + } + + return currentSelection; +}; + export function selectCollection(namespace: string): CollectionSelectedAction { return { type: DiagramActionTypes.COLLECTION_SELECTED, namespace }; } From 19ca3c93fec3504675f6d9ff500f643c5b2a39ae Mon Sep 17 00:00:00 2001 From: Rhys Howell Date: Fri, 8 Aug 2025 13:31:18 -0400 Subject: [PATCH 4/7] fixup: remove batch --- packages/compass-data-modeling/src/store/diagram.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/compass-data-modeling/src/store/diagram.ts b/packages/compass-data-modeling/src/store/diagram.ts index f55ceb52054..927c088b8e6 100644 --- a/packages/compass-data-modeling/src/store/diagram.ts +++ b/packages/compass-data-modeling/src/store/diagram.ts @@ -396,13 +396,6 @@ export function renameCollection( }; dispatch(applyEdit(edit)); - - // When renaming we want to keep the renamed collection selected in the UI, - // the ID is the collection name. - dispatch({ - type: DiagramActionTypes.COLLECTION_SELECTED, - namespace: toNS, - }); }; } From e4d0f09b5eac038eb48c950155a2b8b38cb4f6e2 Mon Sep 17 00:00:00 2001 From: Rhys Howell Date: Fri, 8 Aug 2025 13:38:39 -0400 Subject: [PATCH 5/7] fixup: don't revert collection name on blur when invalid --- .../src/components/drawer/collection-drawer-content.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx b/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx index 92ddc056402..4a6dd5c51da 100644 --- a/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx +++ b/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx @@ -140,8 +140,6 @@ const CollectionDrawerContent: React.FunctionComponent< if (!isCollectionNameValid) { // Reset to previous valid name. - // TODO: Maybe we don't reset. - setCollectionName(toNS(namespace).collection); return; } From e8c49a972f70b06e88ac1beda10465b84b0c7fca Mon Sep 17 00:00:00 2001 From: Rhys Howell Date: Fri, 8 Aug 2025 16:16:26 -0400 Subject: [PATCH 6/7] fixup: remove old comment --- .../src/components/drawer/collection-drawer-content.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx b/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx index 4a6dd5c51da..619c56298cc 100644 --- a/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx +++ b/packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx @@ -139,7 +139,6 @@ const CollectionDrawerContent: React.FunctionComponent< } if (!isCollectionNameValid) { - // Reset to previous valid name. return; } From 73976e8d38612ced1ba45eaa96a7d935c2628fdc Mon Sep 17 00:00:00 2001 From: Rhys Howell Date: Fri, 8 Aug 2025 16:23:31 -0400 Subject: [PATCH 7/7] fixup: tests --- package-lock.json | 2 ++ packages/compass-e2e-tests/package.json | 1 + packages/compass-e2e-tests/tests/data-modeling-tab.test.ts | 3 ++- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/package-lock.json b/package-lock.json index e05c4ccb994..d9861d5694f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -45325,6 +45325,7 @@ "mongodb-build-info": "^1.7.2", "mongodb-connection-string-url": "^3.0.1", "mongodb-log-writer": "^2.3.4", + "mongodb-ns": "^2.4.2", "mongodb-runner": "^5.8.0", "node-fetch": "^2.7.0", "nyc": "^15.1.0", @@ -69112,6 +69113,7 @@ "mongodb-build-info": "^1.7.2", "mongodb-connection-string-url": "^3.0.1", "mongodb-log-writer": "^2.3.4", + "mongodb-ns": "^2.4.2", "mongodb-runner": "^5.8.0", "node-fetch": "^2.7.0", "nyc": "^15.1.0", diff --git a/packages/compass-e2e-tests/package.json b/packages/compass-e2e-tests/package.json index b97bf7b43d0..a5ef583f18b 100644 --- a/packages/compass-e2e-tests/package.json +++ b/packages/compass-e2e-tests/package.json @@ -62,6 +62,7 @@ "mongodb-build-info": "^1.7.2", "mongodb-connection-string-url": "^3.0.1", "mongodb-log-writer": "^2.3.4", + "mongodb-ns": "^2.4.2", "mongodb-runner": "^5.8.0", "node-fetch": "^2.7.0", "nyc": "^15.1.0", diff --git a/packages/compass-e2e-tests/tests/data-modeling-tab.test.ts b/packages/compass-e2e-tests/tests/data-modeling-tab.test.ts index 161a15503c4..805a6f4f9e0 100644 --- a/packages/compass-e2e-tests/tests/data-modeling-tab.test.ts +++ b/packages/compass-e2e-tests/tests/data-modeling-tab.test.ts @@ -18,6 +18,7 @@ import { } from '../helpers/downloads'; import { readFileSync } from 'fs'; import { recognize } from 'tesseract.js'; +import toNS from 'mongodb-ns'; import path from 'path'; import os from 'os'; import fs from 'fs/promises'; @@ -110,7 +111,7 @@ async function selectCollectionOnTheDiagram( const collectionName = await browser.getInputByLabel( drawer.$(Selectors.DataModelNameInput) ); - expect(await collectionName.getValue()).to.equal(ns); + expect(await collectionName.getValue()).to.equal(toNS(ns).collection); } async function getDiagramNodes(