-
Notifications
You must be signed in to change notification settings - Fork 50
Studio: Update Studio deeplink URL scheme from wpcom-local-dev to wp-studio
#2029
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
Conversation
wpcom-local-dev to wp-studio
gcsecsey
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.
📊 Performance Test ResultsComparing f8786e9 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
epeicher
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.
Thanks @gavande1! I have tested it, and I can see that the new protocol prefix is being used successfully. LGTM! ![]()
| OAuth | After |
|---|---|
![]() |
![]() |
ivan-ottinger
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.
Thanks for the update, Rahul! The change looks good. I think we could also update other occurrences of wpcom-local-dev in the codebase - e.g. in the related tests and docs.
|
One more thing: Maybe I missed it somewhere in other discussions, but what about the |
@ivan-ottinger, Yes. I am planning to address it in a separate PR. I am working on it right now. I will raise the PR soon. |
f89f709 to
2ca248c
Compare
2ca248c to
f0a03ca
Compare
|
To close the loop, I have created Automattic/wp-calypso#107172 PR to address changes related to Calypso. |
ivan-ottinger
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.
I gave this a second review and test round. The changes look good and work as expected. Log in works correctly with the updated deeplink URL. 👍🏼
We just need a rebase. 🙂
f0a03ca to
5ef6d1f
Compare
|
Thanks @epeicher and @ivan-ottinger. I have updated the branch with master. The tests should pass now. |
|
@gcsecsey and @ivan-ottinger, what do you think about merging this? It might affect the auth flow, but smoke testing should uncover any problems. I believe it's safe to go ahead with merging this. |
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.
Thanks @gavande1 for following up on this and updating the tests! I also gave this another test and the deeplink flows work as described. 👍
@gcsecsey and @ivan-ottinger, what do you think about merging this? It might affect the auth flow, but smoke testing should uncover any problems. I believe it's safe to go ahead with merging this.
I agree, I think the Studio side changes are good to go and as we've merged 195524-ghe-Automattic/wpcom previously, we shouldn't see any regressions.
|
Yeah, I agree we can marge this one - unless @nightnei has any objections as the release wrangler. 🙂 → Asked in p1763628611277539/1763546848.727229-slack-C04GESRBWKW. |





Related issues
Proposed Changes
wpcom-local-dev://towp-studio://.Testing Instructions
npm startPre-merge Checklist