feat(compass-collection): Move assign call for Mock Data Generator inside check for collection type - CLOUDP-347301

[jcobis]

Description

Move assign call inside the check for !metadata.isReadonly && !metadata.isTimeSeries in packages/compass-collection/src/stores/collection-tab.ts

While not necessary, this is a good optimization to make to only assign when we absolutely need to.

(We initially considered displaying different experimental behavior for these cases (tooltips) but decided with designs against that.)

Also hides the button for time series collections

Checklist

• New tests and/or benchmarks are included
• Documentation is changed or added
• If this change updates the UI, screenshots/videos are added and a design review is requested
• I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Types of changes

• Backport Needed
• Patch (non-breaking change which fixes an issue)
• Minor (non-breaking change which adds functionality)
• Major (fix or feature that would cause existing functionality to change)

[Copilot]

Pull Request Overview

This PR refactors the Mock Data Generator experiment assignment logic to only execute for collections that support the feature. The change moves the experiment assignment code inside the collection type check, preventing unnecessary assignment calls for readonly and time series collections.

Key Changes:

• Moved experiment assignment logic inside the !metadata.isReadonly && !metadata.isTimeSeries conditional check
• Updated inline comments to reflect the additional collection type requirement
• Added test coverage for readonly and time series collection scenarios

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
packages/compass-collection/src/stores/collection-tab.ts	Restructured conditional logic to check collection type before assigning Mock Data Generator experiment
packages/compass-collection/src/stores/collection-tab.spec.ts	Added tests verifying experiment assignment is skipped for readonly and time series collections

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ncarbon]

This assign call may or may not complete by the time shouldRunSchemaAnalysis is called and schema analysis is started. If we're okay with this behavior, then nothing to do. But if we want to ensure the schema analysis is started during the same page visit then consider awaiting the .assignExperiment call or moving the shouldRunSchemaAnalysis code inside the assign callback.

[jcobis]

I think the Experiment SDK holds calls to getAssignment until in-flight calls to assign complete

[kpamaran]

⚠️ Based on my reading of the SDK in mms getAssignment first checks in the SDK's state, and then makes a GET call to the assignment endpoint if it is not found there. So there should be a race condition if it's possible for shouldRunSchemaAnalysis to finish before assignExperiment is done

Which is indeed possible, so I'd recommend then-chaining shouldRunSchemaAnalysis on the assignExperiment Promise

good observation @ncarbon

[jcobis]

I'm looking at the React SDK here: https://github.com/10gen/mdb-experiment-js-sdk/blob/a01a8e716ba27f514806abce136ebe374852d142/src/ExperimentSDK.ts#L553-L557

[kpamaran]

I don't see documentation that suggests an enforced ordering of getAssignment calls. Even then, it's possible for the assign call to take longer than the sibling-getAssignment call below

[kpamaran]

@jcobis ah what you showed is more up to date. I checked the implementation there and it does indeed block on the assign call with the same experiment name, before it fires the GET assignment API

the conditional eval sequence is
!(this.#isAssignmentDataInState(experimentName) // true
-> this.#doesAssignCallExist(experimentName) // true
-> this.#waitForPendingAssignCall(experimentName, options?.team)

once that wait-call resolves we enter

this.#hasNoExistingOrPendingAssignment(experimentName) // true
-> this.#doesGetAssignmentCallExist(experimentName) // false
else -> (get assignment data)

[ncarbon]

Ah, good to know! I think we're fine with the current changes then. I'll resolve this comment.