internal: (studio) persist telemetry session ID in URL #32170
Conversation
astone123
requested review from
jennifer-shehane,
mschile,
ryanthemanuel and
mabela416
cypress
|
||||||||||||||||||||||||||||||||||||||||
| Project |
cypress
|
| Branch Review |
studio-persist-session
|
| Run status |
|
| Run duration | 19m 54s |
| Commit |
|
| Committer | Adam Stone-Lord |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
13
|
Pending
|
1101
|
Skipped
|
0
|
Passing
|
26538
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
44.99%
|
|
|---|---|
|
Untested elements
|
187
|
|
Tested elements
|
157
|
Accessibility
97.71%
|
|
|---|---|
|
Failed rules
|
4 critical
8 serious
2 moderate
2 minor
|
|
Failed elements
|
110
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| const suiteId = hashParams.get('suiteId') | ||
| const visitUrl = hashParams.get('url') | ||
| const newTestLineNumber = hashParams.get('newTestLineNumber') ? Number(hashParams.get('newTestLineNumber')) : undefined | ||
| const cloudStudioSessionId = hashParams.get('cloudStudioSessionId') |
There was a problem hiding this comment.
Since we are already in the context of studio, suggest renaming the url parameter sessionId.
| @@ -432,20 +460,12 @@ export const useStudioStore = defineStore('studioRecorder', { | |||
| }, | |||
|
|
|||
| _getUrlParams () { | |||
There was a problem hiding this comment.
Why does _getUrlParams call getUrlParams? Can we just keep _getUrlParams and update it for the sessionId?
There was a problem hiding this comment.
I moved the logic from _getUrlParams into that function so that we could also call it here when studio state is initialized to grab the value from the url params https://github.com/cypress-io/cypress/pull/32170/files#diff-89b06d274623cbdd0fb8c318c957b3c3659dacee52c0be410100b1ac83283d09R131-R133
There was a problem hiding this comment.
I see, I would just remove _getUrlParams then and just use getUrlParams directly.
There was a problem hiding this comment.
Suggest updating the url tests in studio.cy.ts to also test for the sessionId.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
| @@ -694,7 +694,7 @@ describe('studio functionality', () => { | |||
| it('does not remove the studio url parameters when saving test changes', () => { | |||
| launchStudio() | |||
|
|
|||
| cy.location().its('hash').should('contain', 'testId=r3').and('contain', 'studio=') | |||
| cy.location().its('hash').should('contain', 'testId=r3').and('contain', 'studio=').and('contain', 'sessionId=') | |||
There was a problem hiding this comment.
Bug: URL Assertion Omission in Studio Mode
Multiple cy.location().its('hash') assertions in studio.cy.ts were not updated to include a check for the sessionId URL parameter. When Studio is active, the sessionId parameter should be present in the URL, but these assertions incorrectly omit this check, unlike other similar assertions updated in the same commit.
Additional Locations (2)
| @@ -227,6 +250,10 @@ export const useStudioStore = defineStore('studioRecorder', { | |||
| this._initialUrl = studio.url | |||
| } | |||
|
|
|||
| if (studio.sessionId) { | |||
| this.sessionId = studio.sessionId | |||
| } | |||
There was a problem hiding this comment.
Bug: Inconsistent URL Parameter Handling
The sessionId is redundantly initialized from URL parameters during store state creation and again in the setup method. This is inconsistent with how other URL parameters (testId, suiteId, newTestLineNumber) are handled, which are only set in the setup method. This redundancy could lead to a programmatically set sessionId being overwritten or cause timing issues.
Additional details
This PR adds the studio session ID to the URL parameters for studio so that the session ID survives a page refresh. Right now if the user is in studio and they refresh the page, the ID is lost and no telemetry events are sent until a new studio session is opened.
Steps to test
Enable debug logging for cloud studio, open a studio session, verify that telemetry events are successfully sent. Refresh the page and verify that they are still successfully sent. Verify that when you close studio, the session ID gets removed from the URL. Also verify that when you open studio again, a new session ID is generated.
PR Tasks
cypress-documentation?type definitions?