-
Notifications
You must be signed in to change notification settings - Fork 228
feat(data-modeling): open diagram COMPASS-9546 #7127
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
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 implements functionality to open diagram files in Compass Data Modeling by adding file import capabilities and comprehensive test coverage. The feature allows users to select a local .compass
file and load it as a new diagram with automatic name deduplication.
- Adds a new "Open Diagram" button with file selection functionality across the data modeling interface
- Implements file parsing, validation, and diagram loading with proper error handling
- Extends existing e2e tests to verify the complete save/open workflow
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/compass-e2e-tests/tests/data-modeling-tab.test.ts |
Extends test to verify diagram opening functionality after download |
packages/compass-e2e-tests/helpers/selectors.ts |
Adds selectors for file input and updates diagram list item selector |
packages/compass-data-modeling/src/store/diagram.ts |
Implements openDiagramFromFile action with file parsing and storage |
packages/compass-data-modeling/src/services/open-and-download-diagram.ts |
Adds file parsing and diagram name generation utilities |
packages/compass-data-modeling/src/services/open-and-download-diagram.spec.ts |
Comprehensive tests for file parsing and name generation functions |
packages/compass-data-modeling/src/services/data-model-storage.ts |
Refactors schema validation for reuse in file parsing |
packages/compass-data-modeling/src/components/saved-diagrams-list.tsx |
Integrates import functionality into diagram list UI |
packages/compass-data-modeling/src/components/open-diagram-button.tsx |
New reusable component for file selection |
packages/compass-data-modeling/src/components/diagram-list-toolbar.tsx |
Adds import button to toolbar |
packages/compass-components/src/index.ts |
Exports new FileSelector component |
packages/compass-components/src/components/file-selector.tsx |
New generic file selection component |
configs/mocha-config-compass/register/jsdom-extra-mocks-register.js |
Adds File and Blob mocks for testing |
const parsedContent = JSON.parse(content); | ||
|
||
if ( | ||
parsedContent.version !== kCurrentVersion && |
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 logical operator should be OR (||) instead of AND (&&). The current condition will only reject files if BOTH version and type are incorrect, but it should reject if EITHER is incorrect.
parsedContent.version !== kCurrentVersion && | |
parsedContent.version !== kCurrentVersion || |
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.
nice catch!
onSelect: (files: File[]) => void; | ||
}; | ||
|
||
export function FileSelector({ |
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.
Not necessarily completely your problem to solve, but the fact that we now expose two file inputs from compass-components is very confusing. In the spirit of trying to make code universal, maybe we should rename the other one to make it clear that it's electron only and mark it as deprecated with a JSDoc annotation?
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.
Renaming it makes sense, but why deprecation?
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.
Universal code should be the default almost always, everything else should be an exception, we call this out often in a lot of places, but something that highlights component as "do not use unless you're really truly sure about it" seems to be a useful mechanism, @deprecated
is just a tool to achieve that if people just pick up components automatically based on import suggestions
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.
done in ad151ea. let me know if it looks good, especially the name :)
parsedContent.version !== kCurrentVersion || | ||
parsedContent.type !== kFileTypeDescription | ||
) { | ||
return reject(new Error('Unsupported diagram file format')); |
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.
Nit: I think makes more sense to just throw here so that the same error message adjustments can be applied to all errors that parsing the file contents can produce
return reject(new Error('Unsupported diagram file format')); | |
throw new Error('Unsupported diagram file format'); |
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.
cleaned in fc64ffb
const { name, edits } = parsedContent; | ||
|
||
if (!name || !edits || typeof edits !== 'string') { | ||
return reject(new Error('Diagram file is missing required fields')); |
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.
Same as above
return reject(new Error('Diagram file is missing required fields')); | |
throw new Error('Diagram file is missing required fields') |
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.
Whoops, have I added the same fixture file you already had there? Sorry!
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.
Two small notes, otherwise LGTM
expectedName: string | ||
): string { | ||
let name = expectedName; | ||
let index = 1; |
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.
Instead of starting with 1 always, we can check if there's already an index, so that re-importing diagram (1)
doesn't resolve in diagram (1) (1)
. Not a big deal, but this would be consistent with the connection copies naming logic.
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.
fixed in 88744b5
dispatch(openDiagram(diagram)); | ||
void dataModelStorage.save(diagram); | ||
} catch (error) { | ||
openToast('data-modeling-file-read-error', { |
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.
nit: we could add filename to the id and maybe to the title as well
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.
although with the current flow it's unlikely someone would not notice failed imports immediately, as we open the diagram after import. but this could change later.
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.
Chances of ending up in error block will be mostly due to a rejection when reading file and in such cases, we will not have the name property yet available.
@@ -242,6 +254,27 @@ const DiagramEditor: React.FunctionComponent<{ | |||
}); | |||
}, [model?.relationships, selectedItems]); | |||
|
|||
const selectedFields = useMemo<Record<string, string[]>>(() => { |
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.
Was this pushed by accident?
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.
Yes, I forgot which branch I was on yesterday :D
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.
Good thing Basit didn't accidentally merge this 🙈
2234438
to
ad151ea
Compare
* to be used in environments like Electron where you have access to nodes fs module | ||
* to read/write files. | ||
* | ||
* For browser environments, you should use the `FileSelector` component instead. |
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 just really want to make this point clear, browser environment shouldn't be an exception, it should be a default
* For browser environments, you should use the `FileSelector` component instead. | |
* Always use `FileSelector` component, only use `FilePickerDialog` if you absolutely know what you're doing |
In this PR, I added an option to open (import) a data modeling diagram.
Preview
Screen.Recording.2025-07-21.at.17.19.16.mov
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes