-
Notifications
You must be signed in to change notification settings - Fork 229
feat(compass-collection): Collection Header Experimentation Integration for Mock Data Generator – CLOUDP-333847 #7181
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
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 integrates the Mock Data Generator feature into the Collection Header component. The feature adds a new "Generate Mock Data" button that appears conditionally based on experiment assignment, Atlas environment, and collection type.
- Adds new experiment test group enum for Mock Data Generator experimentation
- Implements Mock Data Generator button in Collection Header Actions with conditional visibility
- Integrates mock data generator modal with collection header state management
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 |
---|---|
packages/compass-telemetry/src/provider.tsx | Exports experiment test groups and assignment hook for external use |
packages/compass-telemetry/src/index.ts | Adds ExperimentTestGroup export to public API |
packages/compass-telemetry/src/growth-experiments.ts | Defines experiment test groups for Mock Data Generator |
packages/compass-collection/src/components/collection-header/collection-header.tsx | Integrates mock data generator modal state and UI |
packages/compass-collection/src/components/collection-header-actions/collection-header-actions.tsx | Implements conditional Mock Data Generator button with experiment gating |
packages/compass-collection/src/components/collection-header-actions/collection-header-actions.spec.tsx | Adds comprehensive test coverage for Mock Data Generator button behavior |
...es/compass-collection/src/components/collection-header-actions/collection-header-actions.tsx
Show resolved
Hide resolved
...es/compass-collection/src/components/collection-header-actions/collection-header-actions.tsx
Outdated
Show resolved
Hide resolved
trigger={ | ||
<div> | ||
<Button | ||
data-testid="collection-header-generate-mock-data" |
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.
data-testid
should have a -button
suffix or similar
@@ -168,4 +204,182 @@ describe('CollectionHeaderActions [Component]', function () { | |||
); | |||
}); | |||
}); | |||
|
|||
context('Mock Data Generator Button', function () { |
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 suggest adding data-testid
for the tooltip and asserting its existence directly in maybe new tests like 'should show Tooltip when ...'
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 idea, how about we add this once we hook up the data so we can test its presence when collection is empty?
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.
Sounds good
|
||
expect(onOpenMockDataModal).to.have.been.calledOnce; | ||
}); | ||
}); |
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.
would be nice to have a test that the modal opens initially with step MockDataGeneratorStep.AI_DISCLAIMER
since that is controlled by the header
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.
Note: what step we open up to will depend on the state of project level settings, as well as a user-level "opt in" flag. We do have the test should call onOpenMockDataModal when CTA button is clicked
, so I think the step we open to will depend based on functionality we will add in later tickets. Do you think this test is sufficient for now, or would you suggest adding something?
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 good. No point in adding a test that'd be irrelevant so soon
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.
Only had minor feedback around testing. Looks good in general
export enum ExperimentTestGroup { | ||
mockDataGeneratorVariant = 'mockDataGeneratorVariant', | ||
mockDataGeneratorControl = 'mockDataGeneratorControl', | ||
} |
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 small note, it's not a hard-and-fast rule, but we generally have a mild preference for literal unions over enum types – i.e. export type ExperimentTestGroup = 'mockDataGeneratorVariant' | 'mockDataGeneratorControl';
since that has a few small advantages over enum
s:
- You don't unnecessarily alias a value to a property name if they're both the same anyway
- You can often avoid importing and resolving the module at runtime when using enum values, and instead either have a type-only import or none at all
- Literal unions would be compatible with
--erasableSyntaxOnly
if we ever decide to adopt this flag - Literal unions are also just generally a bit more flexible and can include additional values if we need to add them later, e.g.
type Foo = 'abc' | 'def' | `numeric:${number}`;
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.
Very good point, I tend to agree. I was following the existing pattern here and I think it might have something to do with how experiments can be iterated on, as in these values can actually change. These values have to correspond to Pholiota values, so for eg. MOCK_DATA_GENERATOR_20251001
can become MOCK_DATA_GENERATOR_20260123
if an experiment iteration is conducted. See the MMS code as well. I am open to changing this though, particularly for ExperimentTestGroup
return await renderWithActiveConnection(ui, connectionInfo); | ||
} else { | ||
// For tests that only need basic component rendering (readonly checks, view buttons, etc.) | ||
const { render } = await import('@mongodb-js/testing-library-compass'); |
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.
Why is this a dynamic import?
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.
Consolidated
|
||
if (connectionInfo) { | ||
// For tests that need Atlas metadata (Mock Data Generator button visibility, etc.) | ||
return await renderWithActiveConnection(ui, connectionInfo); |
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.
You can just use it for both cases, what difference does it make?
}; | ||
|
||
const ui = ( | ||
<TelemetryProvider options={{ sendTrack: sinon.stub() }}> |
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.
compass-testing-library render helpers already provide default track function, why are you adding this provider?
expect( | ||
screen.getByTestId('collection-header-generate-mock-data-button') | ||
).to.exist; | ||
expect(screen.getByText('Generate Mock Data')).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.
get by text is already pretty good, get by role is even better, why do you duplicate this check with get by test id?
// Mock Data Generator modal state | ||
const [isMockDataModalOpen, setIsMockDataModalOpen] = useState(false); | ||
const [currentStep, setCurrentStep] = useState( | ||
MockDataGeneratorStep.AI_DISCLAIMER | ||
); |
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'm not sure that having this as a UI state makes sense: you have form state transitions that clearly would depend on async actions that are handled by the store and reducers and syncing from store to local state is always a pain (and an antipattern that React calls out in the documentation). Shouldn't this state be moved to the store already?
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.
Will move this to the redux store!
Description
Implements the Mock Data Generator button in the Collection Header Actions component. The feature adds a new "Generate Mock Data" button that appears conditionally based on experiment assignment, Atlas environment, and collection type.
Also adds the modal state to the Redux store.
Enabled
Disabled
Motivation and Context
This implements the Mock Data Generator feature integration into the Collection Header, allowing users to access AI-powered mock data generation directly from the collection view. The feature is gated behind an experiment.
Types of changes