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

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Jul 14, 2025

billede

Review

It is best to review this PR in individual commits:

fe04ed8 - largely a drive-by clean-up of the current event setup by adding more of the values into the DocumentEvents constant and cleaning up our export structure.

e7dc505 - adds a DocumentEvents.EditingFinished event handling to document-edit-actions-footer.tsx so its status can be correctly changed when editing is finished through means other than the cancel button. Also moves the action into its own group at the top and renames it.

Motivation

'Stop editing' when right clicking does not fully stop editing the element. This is because the Editing Actions footer does not change status when editing is finished.

'Stop editing' should also be bumped up into a separate group and renamed to Cancel editing as a note from design.

@Copilot Copilot AI review requested due to automatic review settings July 14, 2025 10:54
@gagik gagik requested a review from a team as a code owner July 14, 2025 10:54
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?

Copy link

@Copilot Copilot AI left a 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 centralizes document and element event constants into DocumentEvents and ElementEvents, updates all event references to use those constants, and adjusts the cancel-editing UI logic.

  • Introduce DocumentEvents and ElementEvents constants and their types.
  • Replace raw event strings and default exports with the new constants across the document and editor modules.
  • Rename the context-menu action from “Stop editing” to “Cancel editing”, group menu items, add handling for EditingFinished, and update tests.

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/hadron-document/src/index.ts Refactor exports to use centralized DocumentEvents and ElementEvents
packages/hadron-document/src/document-events.ts Introduce DocumentEvents constant and DocumentEventsType
packages/hadron-document/src/element-events.ts Introduce ElementEvents constant and ElementEventsType
packages/hadron-document/src/document.ts Replace hardcoded event strings with DocumentEvents constants
packages/hadron-document/src/element.ts Replace bubbling event references with ElementEvents constants
packages/hadron-document/src/editor/*.ts Updated imports and event emissions to use ElementEvents
packages/compass-crud/src/stores/crud-store.spec.ts Update tests to use DocumentEvents constants and onceDocumentEvent helper
packages/compass-crud/src/components/use-document-item-context-menu.tsx Rename “Stop editing” to “Cancel editing” and group context menu items
packages/compass-components/src/components/document-list/document-edit-actions-footer.tsx Add EditingFinished listener, reset status, and update cancel logic
Comments suppressed due to low confidence (1)

packages/compass-components/src/components/document-list/document-edit-actions-footer.tsx:186

  • Consider adding a unit test to verify that the status resets when the EditingFinished event is emitted, ensuring the new handler works as intended.
    doc.on(DocumentEvents.EditingFinished, onEditingFinished);

@@ -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

@gagik gagik force-pushed the gagik/right-click-adjustments branch from 62a393b to b3ceb4e Compare July 14, 2025 11:07
@gagik gagik force-pushed the gagik/right-click-adjustments branch from 4ca0fb3 to 529855f Compare July 15, 2025 15:21
@gagik gagik merged commit a6f8780 into main Jul 16, 2025
59 checks passed
@gagik gagik deleted the gagik/right-click-adjustments branch July 16, 2025 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants