Conversation
Socket Security Pull Request Report👍 No new dependency issues detected in pull request Pull request report summary
Bot CommandsTo ignore an alert, reply with a comment starting with Ignoring: Powered by socket.dev |
georgewrmarshall
left a comment
There was a problem hiding this comment.
Looking good! I've created another issue to update internal links once this has been merged #17110
fb013f8 to
336c244
Compare
georgewrmarshall
left a comment
There was a problem hiding this comment.
Looks good! Make sure to create issues for the stories that you've updated to stories-to-do.js so we can track them 👣 You can use the other tickets as a template #16820
6d00f5b to
fc8f96d
Compare
Builds ready [c49b9c7]
Page Load Metrics (1274 ± 87 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
@SocketSecurity ignore playwright@1.29.2 |
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
8f76d5f to
58cfa6d
Compare
Builds ready [6bf33e9]
Page Load Metrics (1215 ± 58 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
6bf33e9 to
84c402e
Compare
ef79e22 to
01bcc48
Compare
8b67fee to
acc82ec
Compare
acc82ec to
bfb8aa2
Compare
Builds ready [cb47bcb]
Page Load Metrics (1381 ± 118 ms)
|
Builds ready [2b0b4b0]
Page Load Metrics (1236 ± 105 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Gudahtt
left a comment
There was a problem hiding this comment.
Great work!
I had a couple of suggestions, most notably moving the new dependencies from dependencies into devDependencies. But that can be done in a follow-up (in the interest of getting this merged sooner to avoid conflicts)
| "nonce-tracker": "^1.0.0", | ||
| "obj-multiplex": "^1.0.0", | ||
| "pify": "^5.0.0", | ||
| "playwright": "^1.29.2", |
There was a problem hiding this comment.
Could you move both of these from dependencies to devDependencies? We only use dependencies for things that end up in the built extension
There was a problem hiding this comment.
Thanks for the review Mark, I will create a follow up PR to add these changes :)
| * | ||
| * @param {string} string - The string to sanitize. | ||
| * @returns The sanitized string. | ||
| * @param {fileName} string - The fileName to get the story id. |
There was a problem hiding this comment.
Nit: Could we add a description here as well? Ideally all of our new functions would have a description.
We require this with a lint rule in other repos, but it hasn't been enabled in the extension yet due to the huge backlog of descriptions we'd have to add.
| * @param {fileName} string - The fileName to get the story id. | |
| * Get the ID for a story file. | |
| * | |
| * @param {fileName} string - The filename of the story file. |
This PR is to add the storybook check for stories
Fixes #15760
Changes:
idfield from all the stories.js files. Stories won't be relying onfilename/idanymore.idsare gone, new story URLs are generated based on the title of each story.stories.json.Ref: #16721