Skip to content

chore(compass-crud): adjust cancel editing logic COMPASS-9564 #7107

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

Merged
merged 5 commits into from
Jul 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, { useCallback, useEffect, useRef, useState } from 'react';
import type HadronDocument from 'hadron-document';
import { Element } from 'hadron-document';
import { DocumentEvents, ElementEvents } from 'hadron-document';
import type { Element } from 'hadron-document';
import { Button } from '../leafygreen';
import { css } from '@leafygreen-ui/emotion';
import { palette } from '@leafygreen-ui/palette';
Expand Down Expand Up @@ -165,34 +166,40 @@ function useHadronDocumentStatus(
updateStatus('DeleteError', err, errorDetails);
};

doc.on(Element.Events.Added, onUpdate);
doc.on(Element.Events.Edited, onUpdate);
doc.on(Element.Events.Removed, onUpdate);
doc.on(Element.Events.Reverted, onUpdate);
doc.on(Element.Events.Invalid, onElementInvalid);
doc.on(Element.Events.Valid, onElementValid);
doc.on('update-start', onUpdateStart);
doc.on('update-blocked', onUpdateBlocked);
doc.on('update-success', onUpdateSuccess);
doc.on('update-error', onUpdateError);
doc.on('remove-start', onRemoveStart);
doc.on('remove-success', onRemoveSuccess);
doc.on('remove-error', onRemoveError);
const onEditingFinished = () => {
updateStatus('Initial');
};

doc.on(ElementEvents.Added, onUpdate);
doc.on(ElementEvents.Edited, onUpdate);
doc.on(ElementEvents.Removed, onUpdate);
doc.on(ElementEvents.Reverted, onUpdate);
doc.on(ElementEvents.Invalid, onElementInvalid);
doc.on(ElementEvents.Valid, onElementValid);
doc.on(DocumentEvents.UpdateStarted, onUpdateStart);
Copy link
Contributor Author

@gagik gagik Jul 14, 2025

Choose a reason for hiding this comment

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

I believe 'update-start'.. etc. can be encompassed under DocumentEvents? Not sure if there was an explicit reason they weren't before.

I also renamed their constant string values for consistency and I wouldn't expect this would break things unless we have some external dependentes that expect these exact strings which doesn't seem like it?

doc.on(DocumentEvents.UpdateBlocked, onUpdateBlocked);
doc.on(DocumentEvents.UpdateSuccess, onUpdateSuccess);
doc.on(DocumentEvents.UpdateError, onUpdateError);
doc.on(DocumentEvents.RemoveStarted, onRemoveStart);
doc.on(DocumentEvents.RemoveSuccess, onRemoveSuccess);
doc.on(DocumentEvents.RemoveError, onRemoveError);
doc.on(DocumentEvents.EditingFinished, onEditingFinished);

return () => {
doc.on(Element.Events.Added, onUpdate);
doc.off(Element.Events.Edited, onUpdate);
doc.off(Element.Events.Removed, onUpdate);
doc.off(Element.Events.Reverted, onUpdate);
doc.off(Element.Events.Invalid, onElementInvalid);
doc.off(Element.Events.Valid, onElementValid);
doc.off('update-start', onUpdateStart);
doc.off('update-blocked', onUpdateBlocked);
doc.off('update-success', onUpdateSuccess);
doc.off('update-error', onUpdateError);
doc.off('remove-start', onRemoveStart);
doc.off('remove-success', onRemoveSuccess);
doc.off('remove-error', onRemoveError);
doc.off(ElementEvents.Added, onUpdate);
doc.off(ElementEvents.Edited, onUpdate);
doc.off(ElementEvents.Removed, onUpdate);
doc.off(ElementEvents.Reverted, onUpdate);
doc.off(ElementEvents.Invalid, onElementInvalid);
doc.off(ElementEvents.Valid, onElementValid);
doc.off(DocumentEvents.UpdateStarted, onUpdateStart);
doc.off(DocumentEvents.UpdateBlocked, onUpdateBlocked);
doc.off(DocumentEvents.UpdateSuccess, onUpdateSuccess);
doc.off(DocumentEvents.UpdateError, onUpdateError);
doc.off(DocumentEvents.RemoveStarted, onRemoveStart);
doc.off(DocumentEvents.RemoveSuccess, onRemoveSuccess);
doc.off(DocumentEvents.RemoveError, onRemoveError);
doc.off(DocumentEvents.EditingFinished, onEditingFinished);
};
}, [doc, updateStatus]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
import type Document from 'hadron-document';
import type { CellEditorProps } from './cell-editor';
import type { GridActions } from '../../stores/grid-store';
import type { Element } from 'hadron-document';
import { DocumentEvents, type Element } from 'hadron-document';
import type { BSONObject, CrudActions } from '../../stores/crud-store';

export type FullWidthCellRendererProps = Pick<
Expand Down Expand Up @@ -51,16 +51,22 @@ class FullWidthCellRenderer extends React.Component<
* Subscribe to the update store on mount.
*/
componentDidMount() {
this.doc.on('remove-success', this.handleRemoveSuccess);
this.doc.on('update-success', this.handleUpdateSuccess);
this.doc.on(DocumentEvents.RemoveSuccess, this.handleRemoveSuccess);
this.doc.on(DocumentEvents.UpdateSuccess, this.handleUpdateSuccess);
}

/**
* Unsubscribe from the update store on unmount.
*/
componentWillUnmount() {
this.doc.removeListener('remove-success', this.handleRemoveSuccess);
this.doc.removeListener('update-success', this.handleUpdateSuccess);
this.doc.removeListener(
DocumentEvents.RemoveSuccess,
this.handleRemoveSuccess
);
this.doc.removeListener(
DocumentEvents.UpdateSuccess,
this.handleUpdateSuccess
);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ describe('useDocumentItemContextMenu', function () {
userEvent.click(screen.getByTestId('test-container'), { button: 2 });

// Should show "Stop editing" when editing
expect(screen.getByText('Stop editing')).to.exist;
expect(screen.getByText('Cancel editing')).to.exist;
expect(screen.queryByText('Edit document')).to.not.exist;
// But show other operations
expect(screen.getByText('Expand all fields')).to.exist;
Expand Down Expand Up @@ -182,7 +182,7 @@ describe('useDocumentItemContextMenu', function () {

// Should show "Edit document" when not editing
expect(screen.getByText('Edit document')).to.exist;
expect(screen.queryByText('Stop editing')).to.not.exist;
expect(screen.queryByText('Cancel editing')).to.not.exist;

// Click edit
userEvent.click(screen.getByText('Edit document'), undefined, {
Expand All @@ -207,11 +207,11 @@ describe('useDocumentItemContextMenu', function () {
userEvent.click(screen.getByTestId('test-container'), { button: 2 });

// Should show "Stop editing" when editing
expect(screen.getByText('Stop editing')).to.exist;
expect(screen.getByText('Cancel editing')).to.exist;
expect(screen.queryByText('Edit document')).to.not.exist;

// Click stop editing
userEvent.click(screen.getByText('Stop editing'), undefined, {
userEvent.click(screen.getByText('Cancel editing'), undefined, {
skipPointerEventsCheck: true,
});

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type HadronDocument from 'hadron-document';
import { useContextMenuItems } from '@mongodb-js/compass-components';
import { useContextMenuGroups } from '@mongodb-js/compass-components';

import type { DocumentProps } from './document';

Expand All @@ -15,57 +15,61 @@ export function useDocumentItemContextMenu({
openInsertDocumentDialog,
}: UseDocumentItemContextMenuProps) {
const { expanded: isExpanded, editing: isEditing } = doc;
return useContextMenuItems(
return useContextMenuGroups(
() => [
{
label: isExpanded ? 'Collapse all fields' : 'Expand all fields',
onAction: () => {
if (isExpanded) {
doc.collapse();
} else {
doc.expand();
}
},
},
...(isEditable
? [
{
label: isEditing ? 'Stop editing' : 'Edit document',
onAction: () => {
if (isEditing) {
doc.finishEditing();
} else {
doc.startEditing();
}
[
...(isEditable
? [
{
label: isEditing ? 'Cancel editing' : 'Edit document',
onAction: () => {
if (isEditing) {
doc.finishEditing();
} else {
doc.startEditing();
}
},
},
},
]
: []),
{
label: 'Copy document',
onAction: () => {
copyToClipboard?.(doc);
]
: []),
],
[
{
label: isExpanded ? 'Collapse all fields' : 'Expand all fields',
onAction: () => {
if (isExpanded) {
doc.collapse();
} else {
doc.expand();
}
},
},
{
label: 'Copy document',
onAction: () => {
copyToClipboard?.(doc);
},
},
},
...(isEditable
? [
{
label: 'Clone document...',
onAction: () => {
const clonedDoc = doc.generateObject({
excludeInternalFields: true,
});
void openInsertDocumentDialog?.(clonedDoc, true);
...(isEditable
? [
{
label: 'Clone document...',
onAction: () => {
const clonedDoc = doc.generateObject({
excludeInternalFields: true,
});
void openInsertDocumentDialog?.(clonedDoc, true);
},
},
},
{
label: 'Delete document',
onAction: () => {
doc.markForDeletion();
{
label: 'Delete document',
onAction: () => {
doc.markForDeletion();
},
},
},
]
: []),
]
: []),
],
],
[
doc,
Expand Down
46 changes: 33 additions & 13 deletions packages/compass-crud/src/stores/crud-store.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,13 @@ import { connect } from 'mongodb-data-service';
import AppRegistry, {
createActivateHelpers,
} from '@mongodb-js/compass-app-registry';
import HadronDocument, { Element } from 'hadron-document';
import HadronDocument, {
DocumentEvents,
Element,
type DocumentEventsType,
} from 'hadron-document';
import { MongoDBInstance } from 'mongodb-instance-model';
import type { EventEmitter } from 'events';
import { once } from 'events';
import sinon from 'sinon';
import chai, { expect } from 'chai';
Expand Down Expand Up @@ -110,6 +115,15 @@ function waitForState(store, cb, timeout?: number) {
return waitForStates(store, [cb], timeout);
}

function onceDocumentEvent(
Copy link
Preview

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

[nitpick] Instead of casting to Node’s EventEmitter and using once, leverage the EventEmitter3 API directly, e.g.: return new Promise(resolve => doc.once(event, (...args) => resolve(args)));

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

@gagik gagik Jul 14, 2025

Choose a reason for hiding this comment

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

I mean... not the worst idea I guess

doc: HadronDocument,
event: DocumentEventsType
): Promise<unknown[]> {
// The once function was not meant for strongly typed events, so we need to
// do some additional type casting.
return once(doc as unknown as EventEmitter, event as string);
}

const mockFieldStoreService = {
updateFieldsFromDocuments() {},
updateFieldsFromSchema() {},
Expand Down Expand Up @@ -503,7 +517,7 @@ describe('store', function () {
});

it('sets the error for the document', function (done) {
hadronDoc.on('remove-error', ({ message }) => {
hadronDoc.on(DocumentEvents.RemoveError, ({ message }) => {
expect(message).to.equal('error happened');
done();
});
Expand Down Expand Up @@ -547,11 +561,11 @@ describe('store', function () {
done();
}, store);

hadronDoc.on('update-blocked', () => {
hadronDoc.on(DocumentEvents.UpdateBlocked, () => {
done(new Error("Didn't expect update to be blocked."));
});

hadronDoc.on('update-error', (errorMessage) => {
hadronDoc.on(DocumentEvents.UpdateError, (errorMessage) => {
done(
new Error(
`Didn't expect update to error. Errored with message: ${errorMessage}`
Expand Down Expand Up @@ -586,11 +600,11 @@ describe('store', function () {
setTimeout(() => done(), 100);
}, store);

hadronDoc.on('update-blocked', () => {
hadronDoc.on(DocumentEvents.UpdateBlocked, () => {
done(new Error("Didn't expect update to be blocked."));
});

hadronDoc.on('update-error', (errorMessage) => {
hadronDoc.on(DocumentEvents.UpdateError, (errorMessage) => {
done(
new Error(
`Didn't expect update to error. Errored with message: ${errorMessage}`
Expand All @@ -613,7 +627,7 @@ describe('store', function () {
});

it('sets the error for the document', function (done) {
hadronDoc.on('update-error', ({ message }) => {
hadronDoc.on(DocumentEvents.UpdateError, ({ message }) => {
expect(message).to.equal(
'Unable to update, no changes have been made.'
);
Expand All @@ -636,7 +650,7 @@ describe('store', function () {
});

it('sets the error for the document', function (done) {
hadronDoc.on('update-error', ({ message }) => {
hadronDoc.on(DocumentEvents.UpdateError, ({ message }) => {
expect(message).to.equal('error happened');
done();
});
Expand All @@ -655,7 +669,7 @@ describe('store', function () {
});

it('sets the update blocked for the document', function (done) {
hadronDoc.on('update-blocked', () => {
hadronDoc.on(DocumentEvents.UpdateBlocked, () => {
done();
});

Expand Down Expand Up @@ -728,7 +742,7 @@ describe('store', function () {
const invalidHadronDoc = new HadronDocument(doc);
(invalidHadronDoc as any).getId = null;

invalidHadronDoc.on('update-error', ({ message }) => {
invalidHadronDoc.on(DocumentEvents.UpdateError, ({ message }) => {
expect(message).to.equal(
'An error occured when attempting to update the document: this.getId is not a function'
);
Expand Down Expand Up @@ -765,7 +779,10 @@ describe('store', function () {
});

it('rejects the update and emits update-error', async function () {
const updateErrorEvent = once(hadronDoc, 'update-error');
const updateErrorEvent = onceDocumentEvent(
hadronDoc,
DocumentEvents.UpdateError
);

await store.updateDocument(hadronDoc);
expect((await updateErrorEvent)[0]).to.match(/Update blocked/);
Expand Down Expand Up @@ -998,7 +1015,7 @@ describe('store', function () {
});

it('sets the error for the document', function (done) {
hadronDoc.on('update-error', ({ message }) => {
hadronDoc.on(DocumentEvents.UpdateError, ({ message }) => {
expect(message).to.equal('error happened');
done();
});
Expand Down Expand Up @@ -1085,7 +1102,10 @@ describe('store', function () {
});

it('rejects the update and emits update-error', async function () {
const updateErrorEvent = once(hadronDoc, 'update-error');
const updateErrorEvent = onceDocumentEvent(
hadronDoc,
DocumentEvents.UpdateError
);

await store.replaceDocument(hadronDoc);
expect((await updateErrorEvent)[0]).to.match(/Update blocked/);
Expand Down
Loading
Loading