Skip to content

internal: (studio) fix studio and runner states during opening, closing, and refreshing #32153

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Aug 11, 2025
Merged

internal: (studio) fix studio and runner states during opening, closing, and refreshing #32153

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
807b949
internal: (studio) fix studio states during opening, closing, and ref…
astone123 Aug 4, 2025
9a0521e
test title
astone123 Aug 4, 2025
967300c
fix tests
astone123 Aug 4, 2025
e94f9be
Merge branch 'release/15.0.0' into fix-studio-reload
astone123 Aug 6, 2025
4df5587
Merge branch 'release/15.0.0' into fix-studio-reload
astone123 Aug 6, 2025
071e1a4
feedback
astone123 Aug 8, 2025
62a2550
Merge branch 'release/15.0.0' into fix-studio-reload
astone123 Aug 8, 2025
ed87881
add test coverage
astone123 Aug 8, 2025
d516243
Merge branch 'release/15.0.0' into fix-studio-reload
astone123 Aug 11, 2025
b541855
Update packages/app/cypress/e2e/studio/studio.cy.ts
astone123 Aug 11, 2025
95faad5
Update packages/app/src/store/studio-store.ts
astone123 Aug 11, 2025
8bc82a7
Merge branch 'develop' into fix-studio-reload
astone123 Aug 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions packages/app/cypress/e2e/studio/studio.cy.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • AUT URL getting replaced with "Enter URL" when tests are running and the "new test" view is open in the studio panel
  • The spec re-runs when the user closes the studio panel from the "new test" view

We should add a couple tests to verify this new behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added tests for both of these 👍🏻

Original file line number Diff line number Diff line change
Expand Up @@ -751,4 +751,70 @@ describe('studio functionality', () => {

cy.location().its('hash').and('not.contain', 'suiteId=').and('not.contain', 'studio=')
})

it('stays in new test mode when studio panel is opened when the spec is running', () => {
loadProjectAndRunSpec()

cy.waitForSpecToFinish()

cy.findByTestId('studio-button').click()
cy.findByTestId('studio-panel').should('be.visible')
cy.findByTestId('new-test-button').should('be.visible')

// Verify we're initially in new test mode
cy.location().its('hash').should('contain', 'suiteId=r1').and('not.contain', 'testId=')

// Now restart the spec, which will call interceptTest with the running test
// This is where the bug would manifest - it would incorrectly switch from
// "new test" mode to "edit the running test" mode
cy.get('button.restart').click()

cy.get('.test').should('have.length', 1)
cy.get('.test').first().should('have.class', 'runnable-active')

cy.wait(500)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another way we can wait instead of a hard-coded value? Seems like may we should be using waitForSpecToFinish here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up not needing this. I originally added it because the test wasn't correctly failing when I backed out my code changes - but I just tested it again and everything works without waiting


// verify we're still in new test mode
cy.findByTestId('studio-panel').should('be.visible')
cy.findByTestId('new-test-button').should('be.visible')

// these should not exist if we stayed in new test mode
cy.findByTestId('studio-single-test-title').should('not.exist')
cy.findByTestId('record-button-recording').should('not.exist')

// verify URL still shows suite mode, not edit test mode
cy.location().its('hash').should('contain', 'suiteId=r1').and('not.contain', 'testId=')

cy.waitForSpecToFinish()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to waitForSpecToFinish here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not - removed

})

it('shows test body sections correctly when studio panel is open and page is refreshed', () => {
loadProjectAndRunSpec()

cy.waitForSpecToFinish()

cy.findByTestId('studio-button').click()
cy.findByTestId('studio-panel').should('be.visible')
cy.findByTestId('new-test-button').should('be.visible')

cy.reload()

cy.waitForSpecToFinish()

cy.findByTestId('studio-panel').should('be.visible')
cy.findByTestId('new-test-button').should('be.visible')

// verify test body section is visible after refresh
cy.get('.runnable-instruments').should('be.visible')
cy.get('.runnable-commands-region').should('be.visible')

// verify the test body hook is present
cy.get('.hook-item').contains('test body').should('be.visible')

// verify commands are visible within the test body
cy.get('.command-name-visit').should('be.visible')

// Verify URL parameters show suite mode, not test mode
cy.location().its('hash').should('contain', 'suiteId=r1').and('not.contain', 'testId=')
})
})
1 change: 1 addition & 0 deletions packages/app/src/runner/SpecRunnerHeaderOpenMode.cy.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ describe('SpecRunnerHeaderOpenMode', { viewportHeight: 500 }, () => {
// This emulates the 'needsUrl' state in the studio store
studioStore.setActive(true)
studioStore.setUrl(undefined)
studioStore._hasStarted = true

cy.mountFragment(SpecRunnerHeaderFragmentDoc, {
render: (gqlVal) => {
Expand Down
29 changes: 20 additions & 9 deletions packages/app/src/runner/event-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,16 +307,25 @@ export class EventManager {
studioInitSuite({ suiteId })
})

const maybeCleanUpProtocol = () => {
const needsReload = this.studioStore.needsProtocolCleanup()

this.studioStore.cancel()

// only reload the page if Studio has actually been used for recording
if (needsReload) {
window.location.reload()
}
}

this.reporterBus.on('studio:cancel', () => {
this.ws.emit('studio:destroy', ({ error }) => {
if (error) {
// eslint-disable-next-line no-console
console.error(error)
}

this.studioStore.cancel()
// Reloading for now. This is the easiest way to clear out the protocol code from the front end
window.location.reload()
maybeCleanUpProtocol()
})
})

Expand Down Expand Up @@ -366,9 +375,7 @@ export class EventManager {
console.error(error)
}

this.studioStore.cancel()
// Reloading for now. This is the easiest way to clear out the protocol code from the front end
window.location.reload()
maybeCleanUpProtocol()
})
})

Expand Down Expand Up @@ -857,7 +864,9 @@ export class EventManager {
performance.measure('run', 'run-s', 'run-e')
})

const hasRunnableId = !!this.studioStore.testId || !!this.studioStore.suiteId
const hasActiveStudio = !!this.studioStore.testId ||
(!!this.studioStore.suiteId && this.studioStore._hasStarted) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this check is needed.

                           (!!this.studioStore.suiteId && this.studioStore._hasStarted) ||

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah good call, it isn't. I removed those two conditions from this variable

!!this.studioStore.newTestLineNumber

const studioSingleTestActive = this.studioStore.newTestLineNumber != null || !!this.studioStore.testId

Expand All @@ -869,7 +878,7 @@ export class EventManager {
autoScrollingEnabled: runState.autoScrollingEnabled,
isSpecsListOpen: runState.isSpecsListOpen,
scrollTop: runState.scrollTop,
studioActive: hasRunnableId,
studioActive: hasActiveStudio,
studioSingleTestActive,
} as ReporterStartInfo)
}
Expand Down Expand Up @@ -928,7 +937,9 @@ export class EventManager {
}

_interceptStudio (displayProps) {
if (this.studioStore.isActive) {
// Only intercept logs when Studio is actually recording a specific test
// Don't intercept when Studio is just open in "new test" mode
if (this.studioStore.isActive && this.studioStore.testId) {
Comment on lines +939 to +941
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function still needed? We don't even display logs anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this still gets called - we don't display anything but it seems like if the logs are intercepted here, they don't get displayed in the command log. So I need to update this because otherwise when we refresh the page and studio is open to "new test", the test body section doesn't get displayed in the left panel.

displayProps.hookId = this.studioStore.hookId

if (displayProps.name === 'visit' && displayProps.state === 'failed') {
Expand Down
9 changes: 7 additions & 2 deletions packages/app/src/store/studio-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,11 @@ export const useStudioStore = defineStore('studioRecorder', {
this.newTestLineNumber = undefined
},

needsProtocolCleanup () {
// Protocol cleanup (page reload) is only needed if Studio has actually been used for recording
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment is accurate anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is... we only want to refresh the page when studio closes if we've actually entered recording mode (like, we've made it past the "new test" screen and actually created a test or opened an existing test in studio)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so by recording you just mean entering single test mode not that the user actually recorded anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, exactly - I'll update the comment to make that more clear

return this._hasStarted || this.logs.length > 0 || this._isStudioCreatedTest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the check on logs needed? We don't display studio logs anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that condition isn't needed... I changed it to this.testId

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would want to cleanup protocol regardless of whether or not an action has been recorded since protocol would'e still been initialized and possibly written to. I'm thinking we may just need to check if there is a testId.

},

openInstructionModal () {
this.instructionModalIsOpen = true
},
Expand Down Expand Up @@ -246,7 +251,7 @@ export const useStudioStore = defineStore('studioRecorder', {

interceptTest (test) {
// if this test is the one we created, we can just set the test id
if ((this.newTestLineNumber && test.invocationDetails?.line === this.newTestLineNumber) || this.suiteId) {
if ((this.newTestLineNumber && test.invocationDetails?.line === this.newTestLineNumber) || (this.suiteId && this._hasStarted)) {
this._isStudioCreatedTest = true
this.setTestId(test.id)
getCypress().runner.setIsStudioCreatedTest(true)
Expand Down Expand Up @@ -862,7 +867,7 @@ export const useStudioStore = defineStore('studioRecorder', {
},

needsUrl: (state) => {
return state.isActive && !state.url && !state.isFailed
return state.isActive && !state.url && !state.isFailed && state._hasStarted
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was causing the AUT URL to get removed whenever the panel was open at all, even if it was just the new test screen

},

testError: (state) => {
Expand Down
Loading