chore(compass-collection): Mock data generator design items CLOUDP-352955 #7500
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements UI/UX improvements for the mock data generator's schema mapping workflow. The changes streamline the user flow by removing the intermediate confirmation step and adding visual indicators for unrecognized field mappings.
Key Changes:
- Removed the
isSchemaConfirmedstate requirement - clicking "Confirm mappings" now directly advances to the next screen - Added warning icons to fields with unrecognized or invalid faker method mappings
- Fixed a typo in the error message tooltip text
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| script-generation-utils.ts | Replaced magic string with constant for unrecognized faker method |
| schema-field-selector.tsx | Added warning icon display logic for unrecognized/invalid field mappings |
| mock-data-generator-modal.tsx | Simplified state management by removing isSchemaConfirmed state and related logic |
| mock-data-generator-modal.spec.tsx | Updated tests to reflect new flow where confirm mappings advances directly to next step |
| faker-schema-editor-screen.tsx | Simplified callback signature by removing boolean parameter and state reset calls |
| collection-header-actions.tsx | Fixed typo in error message (added missing space after closing quote) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| fakerSchema, | ||
| }) => { | ||
| const darkMode = useDarkMode(); | ||
| const logger = createNoopLogger(); |
There was a problem hiding this comment.
The noop logger is being recreated on every render of the FieldSelector component. Move this outside the component or memoize it to avoid unnecessary object creation on each render.
| const shouldShowUnrecognizedIcon = (field: string): boolean => { | ||
| const mapping = fakerSchema?.[field]; | ||
| if (!mapping) return false; | ||
|
|
||
| return ( | ||
| mapping.fakerMethod === UNRECOGNIZED_FAKER_METHOD || | ||
| !isValidFakerMethod(mapping.fakerMethod, mapping.fakerArgs, logger) | ||
| .isValid | ||
| ); | ||
| }; |
There was a problem hiding this comment.
The shouldShowUnrecognizedIcon function is being recreated on every render and called for each field in the list. Consider memoizing this function with useCallback and/or computing the results for all fields once using useMemo to avoid repeated validation calls.
| fakerSchema, | ||
| }) => { | ||
| const darkMode = useDarkMode(); | ||
| const logger = createNoopLogger(); |
There was a problem hiding this comment.
Why was noop logger added here?
There was a problem hiding this comment.
We are already logging when we validate the faker schema when we generate the faker method mappings, so we don't need to log again here when we are validating the faker schema for the "warning" icons.
There was a problem hiding this comment.
I mean, I guess it's fair it's not really obvious unless you look at the usage, but noop logger wasn't created to plug into functions just to satisfy the interface outside the test code 🙂
If you want to use this validation method in the context where logger is not available or not needed, it would be better to change the interface and make it clear that the function can be used without the logger when needed. But also as I mentioned below it doesn't look like you need to do this check because, being an expensive operation, backend response validation and mapping already happened and in UI all methods are either valid or UNRECOGNIZED_FAKER_METHOD, so you don't need to use this check here
There was a problem hiding this comment.
Yeah good point, we were already thorough there so we don't need to be doing another check here for the valid faker method. Good thing is that we don't need the noop logger, but yeah, you're right, the function should have logger as an optional arg if it isn't needed.
| return ( | ||
| mapping.fakerMethod === UNRECOGNIZED_FAKER_METHOD || | ||
| !isValidFakerMethod(mapping.fakerMethod, mapping.fakerArgs, logger) | ||
| .isValid | ||
| ); |
There was a problem hiding this comment.
Invalid faker methods are mapped to unrecognized, so this second check is not doing anything
There was a problem hiding this comment.
I'll remove the redundancy and simplify things here a bit
|
Note: that tooltip copy is being updated anyway: #7495 |
|
Just want to confirm that this was done, from the ticket description, since I noticed some inconsistency with the fallback method appearing for fields marked unrecognized: |
Yes, I've confirmed the default fallback faker method applies here. Changed the string comparison to use our constant for "Unrecognized" |
Description
Checklist
Motivation and Context
Types of changes
Fixes the following copy:
