feat(compass-collection): Retrigger schema analysis after adding data to an empty collection to enable Mock Data Generator button – CLOUDP-356729 #7533
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements automatic schema analysis re-triggering when documents are added to previously empty collections, enabling the Mock Data Generator button for users in the experiment treatment group.
Key Changes
- Added event listener for
document-insertedevents that triggers schema analysis when collections transition from empty to populated - Refactored experiment assignment checking into a reusable utility function
- Schema analysis now re-triggers both on error states and when analysis completed with no schema data
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/compass-collection/src/stores/collection-tab.ts | Added helper functions for experiment checking and schema re-triggering logic; implemented document-inserted event listener |
| packages/compass-collection/src/stores/collection-tab.spec.ts | Added comprehensive test coverage for the new event listener functionality including different collections, connections, and experiment variants |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| eventListeners.get(eventName)!.push(listener); | ||
| } | ||
| ), |
There was a problem hiding this comment.
Can we just use new EventEmitter() instead of building a custom tracker here?
There was a problem hiding this comment.
I don't see any updates in that direction but this was a suggestion, not a strict requirement
There was a problem hiding this comment.
I’m a little unsure what you’re suggesting, could you clarify?
There was a problem hiding this comment.
Actually, looking into this in more detail ... why are we providing these mocks here? Tests seem to pass fine (and they should) if we just use the "real" helpers directly, and we're not spying on the listeners here or anything like that:
diff --git a/packages/compass-collection/src/stores/collection-tab.spec.ts b/packages/compass-collection/src/stores/collection-tab.spec.ts
index 1d11548755ff..b775583f19aa 100644
--- a/packages/compass-collection/src/stores/collection-tab.spec.ts
+++ b/packages/compass-collection/src/stores/collection-tab.spec.ts
@@ -4,7 +4,8 @@ import { selectTab } from '../modules/collection-tab';
import * as collectionTabModule from '../modules/collection-tab';
import { waitFor } from '@mongodb-js/testing-library-compass';
import Sinon from 'sinon';
-import AppRegistry from '@mongodb-js/compass-app-registry';
+import type { ActivateHelpers } from '@mongodb-js/compass-app-registry';
+import AppRegistry, { createActivateHelpers } from '@mongodb-js/compass-app-registry';
import { expect } from 'chai';
import type { workspacesServiceLocator } from '@mongodb-js/compass-workspaces/provider';
import type { ExperimentationServices } from '@mongodb-js/compass-telemetry/provider';
@@ -94,33 +95,7 @@ describe('Collection Tab Content store', function () {
.stub(collectionTabModule, 'analyzeCollectionSchema')
.returns(async () => {});
- // Track event listeners for proper cleanup
- const eventListeners: Array<{
- registry: AppRegistry;
- eventName: string;
- listener: (...args: unknown[]) => void;
- }> = [];
-
- const mockActivateHelpers = {
- on: sandbox.spy(
- (
- registry: AppRegistry,
- eventName: string,
- listener: (...args: unknown[]) => void
- ) => {
- registry.on(eventName, listener);
- eventListeners.push({ registry, eventName, listener });
- }
- ),
- cleanup: sandbox.spy(() => {
- // Remove all tracked event listeners
- eventListeners.forEach(({ registry, eventName, listener }) => {
- registry.removeListener(eventName, listener);
- });
- eventListeners.length = 0;
- }),
- addCleanup: sandbox.spy(),
- };
+ let mockActivateHelpers: ActivateHelpers;
const dataService = {} as never;
const atlasAiService = {} as never;
@@ -168,7 +143,7 @@ describe('Collection Tab Content store', function () {
logger,
preferences,
},
- mockActivateHelpers as never
+ mockActivateHelpers
));
await waitFor(() => {
expect(store.getState())
@@ -178,6 +153,10 @@ describe('Collection Tab Content store', function () {
return store;
};
+ beforeEach(function() {
+ mockActivateHelpers = createActivateHelpers();
+ });
+
afterEach(function () {
mockActivateHelpers.cleanup();
sandbox.resetHistory();There was a problem hiding this comment.
(also, I'd strongly recommend using as any instead of as never – there may be a linter warning about the former, but it more accurately describes what we're actually doing)
| const currentState = store.getState(); | ||
| if (shouldRetriggerSchemaAnalysis(currentState)) { | ||
| // Check if user is in Mock Data Generator experiment variant before re-triggering | ||
| void shouldRunSchemaAnalysis( |
There was a problem hiding this comment.
does the void + then combo make sense here? it's not like there is something else to jump to afterwards. not a big deal, but for a better readability I'd avoid it unless there is a purpose
| (!state.schemaAnalysis.processedSchema || | ||
| Object.keys(state.schemaAnalysis.processedSchema).length === 0)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
I've been thinking about the relation between these checks and the button being disabled.
For the error we check a specific type in ->
hasSchemaAnalysisData is calculated here ->
Throwing in an idea - we could have e.g. hasSchemaAnalysisData selector and then if something changes we'd only need to update the selector (see https://redux.js.org/usage/deriving-data-selectors)
There was a problem hiding this comment.
Just an idea though, I'm more curious why the error checks differ
There was a problem hiding this comment.
Took a stab at this, let me know what you think
There was a problem hiding this comment.
Thanks! It's definitely better to see the logic consolidated in one place. I'm still having trouble understanding how we want to handle all the different cases though -
!selectHasSchemaAnalysisData(state) will be true if the analysis state is anything else than SCHEMA_ANALYSIS_STATE_COMPLETE. If that's intended, then why the extra check for SCHEMA_ANALYSIS_STATE_ERROR? Or was the second check meant to be strictly for completed and empty?
There was a problem hiding this comment.
Same for:
!hasSchemaAnalysisData || exceedsMaxNestingDepth || hasUnsupportedStateError; --- any state other than "complete" will pass the first condition
|
In addition to |
| !selectHasSchemaAnalysisData(state) && | ||
| state.schemaAnalysis?.status !== SCHEMA_ANALYSIS_STATE_ANALYZING | ||
| ); | ||
| } |
There was a problem hiding this comment.
Note: this would also be true if the analysis failed or hasn't been initialized, right?
There was a problem hiding this comment.
I think that's what we want, since we want to display this message for initial state and also error state. Right now, collection-empty is set to error state I believe.
Description
When users add data to an empty collection, the Mock Data Generator button stays disabled because schema analysis doesn't re-run.
Added an event listener for document-inserted events that re-triggers schema analysis when a collection transitions from empty to having data. The listener filters by connection ID and namespace, and only runs for users in the experiment treatment group.
Includes test coverage for the new event listener functionality.
Demonstration
Retrigger.Schema.Analysis.mov
Checklist
Types of changes