feat(compass-collection): gate experiment feature behind redux state check instead of hook call - CLOUDP-344022 #7298
Conversation
jcobis
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR removes the experiment hook-based gating for the Mock Data Generator feature and replaces it with redux state checking. The change simplifies the experiment integration by relying on schema analysis state to determine feature availability instead of direct experiment assignment checks.
- Removes
useAssignmenthook usage and related experiment checking logic - Updates feature gating to check schema analysis status (analyzing/complete states)
- Simplifies test setup by removing experimentation provider mocking
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-telemetry/src/provider.tsx | Removes export of useAssignment hook |
| packages/compass-telemetry/src/experimentation-provider.tsx | Removes useAssignment hook implementation and related types |
| packages/compass-collection/src/components/collection-header/collection-header.spec.tsx | Updates tests to remove experimentation provider setup and hook mocking |
| packages/compass-collection/src/components/collection-header-actions/collection-header-actions.tsx | Replaces experiment assignment check with schema analysis status check |
| packages/compass-collection/src/components/collection-header-actions/collection-header-actions.spec.tsx | Removes experimentation provider from test setup |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...es/compass-collection/src/components/collection-header-actions/collection-header-actions.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
| const renderMethod = connectionInfo | ||
| ? renderWithActiveConnection | ||
| : renderWithConnections; |
There was a problem hiding this comment.
Nit: seems unnecessary, active connection already requires this function to be handled as async, so you can just use the renderWithActiveConnection for all tests
| @@ -72,8 +51,3 @@ export const CompassExperimentationProvider: React.FC<{ | |||
| </ExperimentationContext.Provider> | |||
| ); | |||
| }; | |||
|
|
|||
| // Hook for components to access experiment assignment | |||
There was a problem hiding this comment.
I don't think we need to remove this, this seems useful for future use-cases
| const shouldShowMockDataButton = | ||
| isInMockDataTreatmentVariant && | ||
| (schemaAnalysisStatus === SCHEMA_ANALYSIS_STATE_ANALYZING || | ||
| schemaAnalysisStatus === SCHEMA_ANALYSIS_STATE_COMPLETE) && |
There was a problem hiding this comment.
This change means that even when the experiment related code is removed, the button now will always jump in the view because the analysis doesn't start immediately: first render happens before that. This also makes the button pop out if the analysis has failed. Not sure if this is an intended change in the behavior here. Feels like checking the assignment status is a clearer way of dealing with this UI: when experiment code is removed, this button will always be visible if other conditions are met, right?
There was a problem hiding this comment.
My thinking was that this will actually reduce the latency for which the button pops in, since before this change, it'd have to wait for the useAssignment call to resolve before showing the button.
True that currently this PR has the button pop out on failed analysis, but I think that's okay, as that's an unexpected error state. Alternatively, we could keep the button displayed but disabled.
Yes, when we remove experiment code, we can remove this. I can add a note to do so here if in the future we do remove the experiment gate.
If you feel this PR is better left unmerged, happy to skip it. Maybe we can keep it unmerged for now.
There was a problem hiding this comment.
Hmmm, If the plan is to remove the button if we failed to analyze instead of surfacing the error somehow, then indeed it's probably better to not show the button at all and limit the state checks even more than you have here to only showing it when analysis is completed successfully, otherwise in the current version the button shows up hen analysis starts, it's disabled because analysis is in progress, then it just disappears with no notice on error, this doesn't make much sense as a behavior 🙂
There was a problem hiding this comment.
Ok, sounds good. For now, I'll leave this PR and we can revisit/reconsider it once higher priority work is done.
There was a problem hiding this comment.
Sure! Do you mind switching it to draft so that it doesn't look like ready for review?
addaleax
changed the title
addaleax
changed the title
Description
Because we now gate the schema analysis action behind the experiment check (as part of this PR) in the plugin activation, we can now remove the experiment check in the component and rely solely on the redux state to gate the feature.
Empty Collection - Button Disabled with Tooltip
Overly Nested Collection - Button Disabled with Tooltip
Time Series - Button Hidden
Collection Has Data - Button Displayed and Enabled
Button Opens Modal
Checklist
Types of changes