-
Notifications
You must be signed in to change notification settings - Fork 3.3k
internal: (studio) initialize cloud studio asynchronously #31469
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: develop
Are you sure you want to change the base?
Conversation
cypress
|
Project |
cypress
|
Branch Review |
10501-studio-telemetry
|
Run status |
|
Run duration | 18m 39s |
Commit |
|
Committer | astone123 |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
9
|
|
1232
|
|
0
|
|
32147
|
View all changes introduced in this branch ↗︎ |
UI Coverage
46.2%
|
|
---|---|
|
186
|
|
164
|
Accessibility
92.71%
|
|
---|---|
|
3 critical
8 serious
2 moderate
2 minor
|
|
888
|
@astone123 Lint is failing here |
@@ -164,7 +164,12 @@ export interface CoreDataShape { | |||
cloudProject: CloudDataShape | |||
eventCollectorSource: EventCollectorSource | null | |||
didBrowserPreviouslyHaveUnexpectedExit: boolean | |||
studio: StudioManagerShape | null | |||
studioLifecycleManager?: { |
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 it make sense to put this type in the @types package similar to StudioManagerShape. So that we can have that type enforced in both places it's effectively used?
|
||
if (studioManager.status === 'INITIALIZED') { | ||
// Start studio manager initialization but don't block | ||
const studioManagerPromise = getAndInitializeStudioManager({ |
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 wondering whether we could move this promise to be either part of the constructor for StudioLifecycleManager (so that it's not really mutable after its set). Or maybe even better, encapsulate this logic entirely inside of StudioLifecycleManager (but only if it's not super messy to do that).
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.
Looks good and works locally
Additional details
We'd like to know how many people are using Cypress Studio today. This PR does a few things:
StudioLifecycleManager
to handle accessing StudioSteps to test
Ensure that studio still loads correctly - the cloud studio panel should still load when you pass the correct environment variables, and it should not when these variables are not provided.
Check the tests that I added that verify that the cloud code to collect telemetry is always called when Studio loads even if AI is not enabled.
How has the user experience changed?
Users shouldn't notice any changes to how Studio works today - the external user experience should remain unchanged.
PR Tasks
cypress-documentation
?type definitions
?