-
Notifications
You must be signed in to change notification settings - Fork 53
Add support for base64-encoded blueprints in deeplinks #2042
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
📊 Performance Test ResultsComparing cece592 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
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 @ivan-ottinger for adding this feature! I have tested it, and it works as expected. I can see the error with the invalid blueprint, and I have successfully created a site installing Pendant by using a base64 encoded blueprint in the deeplink. Code changes also LGTM! ![]()
| Invalid blueprint | Blueprint installing Pendant |
|---|---|
![]() |
![]() |
I have also tested any regressions and I have not found any issues 👌
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.
Thanks @ivan-ottinger for tackling this! The changes LGTM and work as described. 🎉 I was able to create a site with WooCommerce installed using this link:
wpcom-local-dev://add-site?blueprint=ewogICJwbHVnaW5zIjogWwogICAgIndvb2NvbW1lcmNlIgogIF0sCiAgInN0ZXBzIjogW10sCiAgInByZWZlcnJlZFZlcnNpb25zIjogewogICAgInBocCI6ICI4LjMiLAogICAgIndwIjogImxhdGVzdCIKICB9LAogICJmZWF0dXJlcyI6IHt9LAogICJsb2dpbiI6IHRydWUKfQ==
ℹ️ Please note that we will be adjusting the UI/UX once the designs are ready. This PR only adds raw support for base64-encoded blueprints in deeplinks.
As we discussed on the meetup last week, I added this issue for the UI tweaks, and assigned you.
|
@ivan-ottinger Thank you for working on this. It works as expected. I have a suggestion from a technical implementation perspective. Instead of adding additional code to handle the base64 blueprint directly, how about saving the decoded content to a temporary JSON file and then reusing the existing logic that already handles blueprint paths via the IPC event? This approach should allow us to remove a significant amount of new code in this PR and rely on the established flow. Here is a simple outline of the idea:
What do you think about this approach? cc @gcsecsey |
|
Thank you for your reviews and testing, Roberto, Gergely and Rahul!
I think that could work well too! What I am thinking is to merge this PR as-is and then create a subsequent one with a refactor - as I was planning to update the step handling as well (as currently the blueprint that is linked in the URL goes to the "choose a blueprint" step - as opposed to the Add site step). |
1f95f35 to
d35c230
Compare
Sure thing. Feel free to merge this and iterate on it later. |
d35c230 to
f4cbcc7
Compare
| JSON.parse( blueprintJson ); | ||
| await sendIpcEventToRenderer( 'add-site-blueprint-from-base64', { blueprintJson } ); | ||
| } catch ( error ) { | ||
| Sentry.captureException( error ); |
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.
Do we need to track such errors in Sentry? How would we use them?
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.
Yes, no need for that. I have already removed the logging in the refactor PR: #2103.


Related issues
Related to STU-921.
Proposed Changes
createBlueprintFromDataandapplyPreferredVersionslogicℹ️ Please note that we will be adjusting the UI/UX once the designs are ready. This PR only adds raw support for base64-encoded blueprints in deeplinks.
Testing Instructions
npm install && npm start.Broken blueprint:
Working blueprint:
Add sitebutton the site with the related blueprint should create itselfTest for regressions:
Pre-merge Checklist