-
Notifications
You must be signed in to change notification settings - Fork 239
feat(compass-collection): manage faker mapping request state in redux CLOUDP-333850 #7251
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): manage faker mapping request state in redux CLOUDP-333850 #7251
Conversation
…lection-redux-integrates-mock-data-schema
| // todo: dedup/abort requests using requestId (CLOUDP-333850) | ||
| const requestId = new UUID().toString(); |
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.
For controlling the abort flow we usually use the thunk arg to keep the reference to the abort controller, see compass-indexes plugin for example
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.
didn't see one there but I think this ref in compass-schema-validation that Nataly shared with me works as an example too
| ) => { | ||
| const { schemaAnalysis, fakerSchemaGeneration, namespace } = getState(); | ||
| if (schemaAnalysis.status !== SCHEMA_ANALYSIS_STATE_COMPLETE) { | ||
| logger.log.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.
This should probably be a warn, not an error if it doesn't actually cause any harm, compass users look at logs sometimes and do get confused sometimes by errors in the log that are not really errors
|
Finished a round of diffs for applying the redux changes on actual modal contents |
| }; | ||
| } | ||
|
|
||
| it('cancels in-flight faker mapping requests when the cancel button is clicked', async () => { |
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 coverage!
...mpass-collection/src/components/mock-data-generator-modal/mock-data-generator-modal.spec.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.
Nice work Kevin!
| function renderModal({ | ||
| isOpen = true, | ||
| currentStep = MockDataGeneratorStep.SCHEMA_CONFIRMATION, | ||
| mockServices = createMockServices(), |
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.
Just FYI, we call out testing-library-compass in development guide for a reason. A lot of manual setup you do here and in createMockServices we already do via testing helpers exposed from testing-library-compass (specifically https://github.com/mongodb-js/compass/tree/main/configs/testing-library-compass#createplugintesthelpers for testing parts of the plugin)
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 attempted createPluginTestHelpers but had some trouble, I'll try again in my upcoming PR and look at the existing usage more closely
Description
Updates the redux reducer in
package/compass-collectionto track request state for the AtlasAiServicegetMockDataSchemaoperation backed by the NLQ API and control the first few screens of the modal.The contents of the schema confirmation and schema editor steps will be fleshed out in future tickets and screenshots will be shared there. For now, placeholders are rendered.
Checklist
Motivation and Context
Open Questions
npm install --save @mongodb-js/compass-generative-ai@ --workspace=packages/compass-collection(per CONTRIBUTING.md). Is there more I need to do? There's a change to the dependency happening at the same time as the dep addition, not sure if the upgrade bot handles thatI reviewed the official Redux style guide (essential to recommended). Would like reviewers to check for anti-patterns.I added the first reducer test for this module, and I find organizing it by "for this reducer logic's state" -> "when X action is received" is sensible.Dependents
Types of changes