-
Notifications
You must be signed in to change notification settings - Fork 225
Detect dev session takeover #6799
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: main
Are you sure you want to change the base?
Conversation
packages/app/src/cli/services/dev/processes/dev-session/dev-session.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/dev/processes/dev-session/dev-session.ts
Outdated
Show resolved
Hide resolved
d7e0917 to
f2eb296
Compare
Coverage report
Test suite run success3721 tests passing in 1438 suites. Report generated by 🧪jest coverage report action from e448893 |
d1954be to
3389ecf
Compare
3389ecf to
9ab5b37
Compare
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
|
I left a comment in slack that I'm surprised were doing this validation logic regarding dev preview takeover for updates in the CLI (vs the DevApi)? I expected the latter (server validation vs client validation reasons; + it would make tuning this logic/copy easier in the future since CLI is versioned) but stuck with how it was for now. |
ea7bd9d to
fa92fff
Compare
fa92fff to
e448893
Compare
|
/snapit |
|
🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260210131100Caution After installing, validate the version by running just |
gonzaloriestra
left a comment
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.
Working great! Comments are not blocking
| await this.logger.error( | ||
| `Another developer ${newUserEmail ? `(${newUserEmail}) ` : ''}has taken ownership of this dev preview.`, | ||
| ) |
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.
Why showing the error twice? I'd get rid of this if we are going to abort.
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 think it's nice to see something in the log, even if we end up saying the same thing in the final error message. I asked about copy for these messages here and while I didn't explicitly ask if both were warranted, there wasn't push back against having both.
| const message = `Another user${ | ||
| newUserEmail ? ` (${newUserEmail})` : '' | ||
| } has taken ownership of this dev preview. Your preview is no longer active.` |
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.
Not a big deal, but the issue I mentioned here is not fixed.
If the only change is the websocket URL, we are saying Another user, but showing the same email.
| if (warnings.length > 0) { | ||
| await Promise.all( | ||
| warnings.map((warning) => { | ||
| const message = warning.code === 'SESSION_TAKEOVER' ? `⚠️ ${warning.message}` : warning.message |
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.
Why not just adding the emoji to all the warnings?
| const message = warning.code === 'SESSION_TAKEOVER' ? `⚠️ ${warning.message}` : warning.message | |
| const message = `⚠️ ${warning.message}` |

Summary
Implements client-side detection and handling for multi-user dev session conflicts, building on backend changes in https://github.com/shop/world/pull/383392.
When multiple developers run
shopify app devon the same app/shop combination, the CLI now provides clear feedback to both users about the session takeover and gracefully handles the conflict.Changes
GraphQL Query Updates
devSession,warnings, anduserErrorsfieldsdevSessionwith user and websocket informationSchema Updates
Updated local schema file (
app_dev_schema.graphql) to match backend changes:devSessionfield toDevSessionCreatePayloadwarningsarray toDevSessionCreatePayloaddevSessionfield toDevSessionUpdatePayloadDevSessionWarningandDevSessionWarningCodeenumImplementation (dev-session.ts)
Session State Tracking:
DevSessionStateinterface to track websocket URL and user informationcurrentSessionStateinstance variable to maintain session metadataCreate-Time Warning Display:
SESSION_TAKEOVER)Update-Time Takeover Detection:
websocketUrlagainst expected local websocket URL on every updateAbortErrorto gracefully terminate the dev session with actionable next stepsUser Experience
Scenario: User B Takes Over User A's Session
User A starts dev:
User B starts dev (same app/shop):
User A makes a code change:
Benefits
Dependencies
This PR depends on backend changes:
Testing Plan
Once the backend PR is merged:
Addresses https://github.com/shop/issues-develop/issues/21606
Measuring impact
Checklist