-
Notifications
You must be signed in to change notification settings - Fork 231
feat(compass-collection): Render Output Script in Result Screen - Mock Data Generator CLOUDP-333858 #7355
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
base: main
Are you sure you want to change the base?
Conversation
… across mock data generator components
…d outside validation method
…ngodb-js/compass into CLOUDP-333858
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 connects the document generation engine to the Mock Data Generator's result screen to render the output script. It refactors the array length mapping system from a complex nested structure to a simpler flat mapping using dot notation with []
markers for arrays, and integrates this with the script generation utilities.
- Updates
processSchema
to return both field information and array length mapping - Simplifies
ArrayLengthMap
type from nested object structure to flatRecord<string, number>
- Connects the script generation to the result screen component with proper data flow
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
transform-schema-to-field-info.ts | Adds array length calculation and updates processSchema to return both field info and array length map |
transform-schema-to-field-info.spec.ts | Updates tests to handle new processSchema return structure with fieldInfo and arrayLengthMap properties |
schema-analysis-types.ts | Adds arrayLengthMap property to SchemaAnalysisCompletedState interface |
collection-tab.ts | Updates reducer and action to handle arrayLengthMap from processSchema result |
to-simplified-field-info.ts | Updates function signature to accept fieldInfo property from processSchema result |
script-screen.tsx | Integrates script generation with faker schema and array length map, renders actual generated script |
script-generation-utils.ts | Simplifies ArrayLengthMap type and updates rendering logic to use flat dot notation paths |
script-generation-utils.spec.ts | Updates test cases to use simplified array length map structure |
mock-data-generator-modal.tsx | Connects array length map from schema analysis to script screen component |
mock-data-generator-modal.spec.tsx | Updates tests to provide completed faker schema generation state for script screen tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/compass-collection/src/components/mock-data-generator-modal/script-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.
Just a general note, you have a lot of logic in this file formatting the script (and it still looks pretty messy) whereas we already have prettier bundled in the app and use it in other parts of the app for output code formatting, you can remove all this indenting logic and just format the script at the very end of this process:
import { prettify } from '@mongodb-js/code-editor';
// ...
funciton renderScript() {
// ...
return prettify(unformattedScriptString, 'javascript')
}
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 to know, changed!
packages/compass-collection/src/components/mock-data-generator-modal/script-screen.tsx
Show resolved
Hide resolved
if (fakerSchemaGenerationState.status === 'completed') { | ||
return ( | ||
<ScriptScreen |
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 is a weird split in component responsibilities, feels better suited for the ScriptScreen. So is the property passing, all this stuff is coming from state, so you should just connect the ScriptScreen component to this state directly as the redux documentation recommends
<Code | ||
copyable={scriptResult.success} | ||
language={Language.JavaScript} | ||
style={{ maxHeight: '230px', overflowY: 'auto' }} |
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.
That's an unexpected usage of style
property, this is static, so can be defined as a className outside of render with css({})
{!scriptResult.success && ( | ||
<Banner variant="danger"> | ||
<strong>Script Generation Failed:</strong> {scriptResult.error} | ||
<br /> | ||
Please go back to the start screen to re-submit the collection | ||
schema. | ||
</Banner> | ||
)} |
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.
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.
packages/compass-collection/src/components/mock-data-generator-modal/script-generation-utils.ts
Fixed
Show fixed
Hide fixed
packages/compass-collection/src/components/mock-data-generator-modal/script-generation-utils.ts
Fixed
Show fixed
Hide fixed
expect(result.script).to.contain(expectedReturnBlock); | ||
expect(result.script).to.contain('Array.from'); | ||
expect(result.script).to.contain('length: 3'); | ||
expect(result.script).to.contain('faker.lorem.word()'); |
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.
Changing some of the simpler tests to be less strict because formatting is now handled by prettier
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Description
Connect the document generation engine to the result screen to render the output script.
Updates the array length map mocked up in previous PR to be simpler.
(Note that I was branching off another PR before it was merged, which is why there are some commits appended in the commit history)
Checklist
Types of changes