-
Notifications
You must be signed in to change notification settings - Fork 51
Blueprints: Update validation #2093
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
Blueprints: Update validation #2093
Conversation
📊 Performance Test ResultsComparing 596772d vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
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 for refactoring the validation @ivan-ottinger, and for updating the tests! I tested both blueprints from the drive and a couple from the blueprints gallery, and I'm getting the same validation issues reported as on trunk.
| Invalid URL | Invalid Blueprint | Valid Blueprint |
|---|---|---|
![]() |
![]() |
![]() |
|
|
||
| if ( ! validation.valid ) { | ||
| await fs.remove( blueprintPath ).catch( () => { | ||
| // Ignore cleanup errors |
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 was thinking maybe we can log this, but as this is a temporary file, I think this is fine to skip. 👍
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.
@ivan-ottinger, I believe we are validating the blueprint twice as per the suggested changes in this PR. Is it intended, or am I missing something? Do you think we should refactor that?
studio/src/modules/add-site/hooks/use-blueprint-deeplink.ts
Lines 143 to 150 in 0ea9e30
| const blueprintJson = await getIpcApi().readBlueprintFile( pendingBlueprintPath ); | |
| const validation = await getIpcApi().validateBlueprint( blueprintJson ); | |
| if ( ! validation.valid ) { | |
| const errorMessage = validation.error || __( 'Invalid Blueprint format' ); | |
| throw new Error( errorMessage ); | |
| } |
gavande1
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.
|
Thank you for your reviews and testing, Gergely and Rahul!
That's a good catch, @gavande1! This second validation is no longer needed - since we validate earlier in the flow, before even the modal has a chance to open. I have addressed that in 6560c93 and the PR is ready for another review round. 🙂 I am looking forward to the next simplifications related to STU-1008 in the next PR. |
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 for removing the extra validation, the updated version LGTM and still works well in all scenarios. 🙌
6560c93 to
df4a3f5
Compare
gavande1
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 addressing the feedback. LGTM 👍




Related issues
Proposed Changes
validateBlueprintDatafunction - so it can be reused easilyℹ️ Please note that this PR does not add validation to the handling of deeplinks that include the base64-encoded blueprint. This is because we will have another PR that will refactor and unify the logic that handles both deeplink types: STU-1008.
Testing Instructions
npm install && npm start.Pre-merge Checklist