-
Notifications
You must be signed in to change notification settings - Fork 231
feat(compass-collection): finish contents of mock data generator confirmation screen CLOUDP-333852 #7299
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
feat(compass-collection): finish contents of mock data generator confirmation screen CLOUDP-333852 #7299
Conversation
? 'A sample of document values from your collection will be sent to an LLM for processing.' | ||
: 'We have identified the following schema from your documents. This schema will be sent to an LLM for processing.'; | ||
|
||
// todo: should establish if unfinished schema analysis edge case should be handled within the modal or |
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.
Alternatively, we could just disabled button
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 lean towards disabling the button plus updating the hover tooltip. It would be awkward to have a modal open and the user isn't able to act on anything but wait
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.
Feel free to go ahead and do so. We can make the tooltip copy "Schema analysis in progress" for now
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.
Net diffs is high in this PR. I think I can weave the tooltip into CLOUDP-333853 which is already small in scope
...mpass-collection/src/components/mock-data-generator-modal/raw-schema-confirmation-screen.tsx
Outdated
Show resolved
Hide resolved
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.
Looks great Kevin! My one note is to perhaps add unit tests for RawSchemaConfirmationScreen
@jcobis I made another diff test to organize the confirmation tests better. Can you re-review the test suite under |
...es/compass-collection/src/components/collection-header-actions/collection-header-actions.tsx
Outdated
Show resolved
Hide resolved
: 'Document Schema Identified'; | ||
|
||
const descriptionText = enableSampleDocumentPassing | ||
? 'A sample of document values from your collection will be sent to an LLM for processing.' |
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 realize this is from Figma, but just going to say, the wording here seems odd, "document value" is somewhat clear but it's definitely not standard terminology. Can this be
? 'A sample of document values from your collection will be sent to an LLM for processing.' | |
? 'A sample of documens from your collection will be sent to an LLM for processing.' |
?
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.
Certainly
<Body className={descriptionStyles}>{descriptionText}</Body> | ||
<Code language="javascript" copyable={false} className={codeStyles}> | ||
{enableSampleDocumentPassing | ||
? JSON.stringify(schemaAnalysis.sampleDocument, null, 4) |
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.
What happens with types that are not representible in JSON?
The Figma designs seem to be aiming for something like our DocumentListView
component rather than JSON
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.
Changed to the Document
component. I don't think there should be any more representation issues on the schema-only or sample-doc variants
} | ||
|
||
if (part.replaceAll('[]', '') === '') { | ||
throw Error("expected fieldPath to have a non-empty part before '[]'"); |
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 test is stricter than the error message here. Also, maybe not strictly necessary, but in JS you'd generally use
throw Error("expected fieldPath to have a non-empty part before '[]'"); | |
throw new Error("expected fieldPath to have a non-empty part before '[]'"); |
* inputs are required to simulate these unlikely errors. | ||
*/ | ||
function validateFieldPath(fieldPath: string) { | ||
const parts = fieldPath.split('.'); |
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.
What happens if a field path element contains a .
?
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.
Discussed this with @jcobis. I acknowledge that this is possible even if the official docs actively discourage it. We're going to disable the "Generate Mock Data" button in this case with a corresponding tooltip and info-log for now
edit: we're still discussing (link) how to handle this across the whole flow (including the API)
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.
This also has me thinking about fields whose names specifically end in []
. Another edge case to consider
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.
Started a Slack Thread
|
) | ||
).to.exist; | ||
// fragment from { "name": "John" } | ||
expect(screen.queryByText('"John"')).to.exist; |
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.
Can we use getBy*
instead of queryBy*
where we check that an element does exist? queryBy*
will not throw an error if it doesn't find the element, potentially failing silently.
<div data-testid="raw-schema-confirmation"> | ||
{schemaAnalysis.status === 'complete' ? ( | ||
<> | ||
<Body className={namespaceStyles}>{namespace}</Body> |
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.
Looks like we want to show the namespace on all screens according to Figma. This should probably go in mock-data-generator-modal.tsx
so it's shared between all the screens.
@addaleax @gribnoysup @ncarbon I addressed latest round of diffs and updated the screenshots. There is a separate thread (link) for any additional design feedback from Maria, which I consider to be a non-blocker and can be followed up on. |
); | ||
} | ||
|
||
// Check that [] only appears as complete pairs and only at the end of the part |
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.
This may be overly stringent actually.
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.
Relating to this comment right? #7299 (comment)
I'll remove for now
On the Evergreen failures. There's one cyclic dependency to resolve due to adding compass-crud as a dep to compass-collection for
The 1st dep is solely used to re-use a type. One naive way to resolve would be to delete the import and duplicate this type. import type { CollectionTabPluginMetadata } from '@mongodb-js/compass-collection'; The 2nd dep was introduced this PR. Touching the 3rd dep is out of question. I reviewed Document's implementation and it's not quite something that can be moved to compass-components easily Maybe there's a better alternative I'm not seeing. |
@kpamaran good catch, I'll open a ticket to move the document card completely out of crud, but in the meantime I think you should be able to use |
borderRadius: spacing[400], | ||
}); | ||
// note: the "> div" selector works around the lack of a className prop on HadronDocument | ||
const documentContainerStyles = css` |
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.
We prefer object notation over template literals, do you mind changing this back? Also if you want to add a className prop support to the component, feel free to do so
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.
went the className prop route
packages/compass-collection/src/transform-schema-to-field-info.ts
Outdated
Show resolved
Hide resolved
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.
Great work Kevin!
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.
Let's remove unneeded branching logic in the action, otherwise LGTM
Description
Jira: https://jira.mongodb.org/browse/CLOUDP-333852
Figma (link)
The confirmation screen allows the user to preview and confirm to the contents sent to the LLM, which respects the setting for whether Generative AI features should send sample data.
Edit: New screenshots using compass Document component instead of leafygreen Code. The result is closer to the Figma
Sample Documents Variant
Schema-only Variant
You will notice that there is no syntax highlighting based on the mongo type in the schema-only variant. That may be viable with the
customKeywords
prop on the leafygreen Code component, but that requires 4 major version upgrades and may introduce breaking changes.Checklist
Motivation and Context
Open Questions
Dependents
Types of changes