mirror of
https://github.com/gnh1201/welsonjs.git
synced 2025-05-11 20:21:03 +00:00
updated pr-agent best practices with auto analysis
parent
9171e5c0ee
commit
a8e4e60487
|
@ -1,3 +1,83 @@
|
|||
<!-- PR --><table><tr><td> <b><a href='2850302530'>PR 245</a></b> (2025-05-05)
|
||||
|
||||
</td></tr></table>
|
||||
|
||||
|
||||
|
||||
<!-- suggestion --><details><summary>[possible issue] Prevent substring exception</summary>
|
||||
|
||||
___
|
||||
|
||||
✅ Prevent substring exception
|
||||
|
||||
**The JqueryCdnPrefix transform is removing the "jquery/" prefix from the path, but this could cause issues if the path doesn't have sufficient length. Add a check to prevent potential ArgumentOutOfRangeException when the path is too short.**
|
||||
|
||||
[WelsonJS.Toolkit/WelsonJS.Launcher/ResourceServer.cs [196-210]](https://github.com/gnh1201/welsonjs/pull/245/files#diff-818c326b0529d19f21de13c9a00464e3eaacb0de06acbbc0bcea444e1cd3bef8R196-R210)
|
||||
|
||||
```diff
|
||||
private async Task<bool> TryServeFromCdn(HttpListenerContext context, string path)
|
||||
{
|
||||
bool isNodePackageExpression = Regex.IsMatch(path, @"^[^/@]+@[^/]+/");
|
||||
|
||||
var sources = new (bool isMatch, string configKey, Func<string, string> transform)[]
|
||||
{
|
||||
(path.StartsWith("ajax/libs/"), "CdnJsPrefix", p => p),
|
||||
(isNodePackageExpression, "UnpkgPrefix", p => p),
|
||||
(isNodePackageExpression, "SkypackPrefix", p => p),
|
||||
(isNodePackageExpression, "EsmShPrefix", p => p),
|
||||
(isNodePackageExpression, "EsmRunPrefix", p => p),
|
||||
(path.StartsWith("npm/") || path.StartsWith("gh/") || path.StartsWith("wp/"), "JsDeliverPrefix", p => p),
|
||||
- (path.StartsWith("jquery/"), "JqueryCdnPrefix", p => p.Substring("jquery/".Length)),
|
||||
+ (path.StartsWith("jquery/") && path.Length > "jquery/".Length, "JqueryCdnPrefix", p => p.Substring("jquery/".Length)),
|
||||
(true, "BlobStoragePrefix", p => p) // fallback
|
||||
};
|
||||
```
|
||||
|
||||
|
||||
|
||||
Suggestion importance[1-10]: 7
|
||||
|
||||
__
|
||||
|
||||
Why: The suggestion correctly identifies a potential `ArgumentOutOfRangeException` if the `path` variable is exactly "jquery/". Adding the length check (`&& path.Length > "jquery/".Length`) prevents this runtime error, improving the code's robustness.
|
||||
|
||||
___
|
||||
|
||||
</details>
|
||||
|
||||
|
||||
|
||||
<!-- suggestion --><details><summary>[general] Improve package detection regex</summary>
|
||||
|
||||
___
|
||||
|
||||
✅ Improve package detection regex
|
||||
|
||||
**The regex pattern for detecting node package expressions might not handle all valid package formats. Consider using a more comprehensive pattern that accounts for scoped packages (starting with @) and various version formats.**
|
||||
|
||||
[WelsonJS.Toolkit/WelsonJS.Launcher/ResourceServer.cs [198]](https://github.com/gnh1201/welsonjs/pull/245/files#diff-818c326b0529d19f21de13c9a00464e3eaacb0de06acbbc0bcea444e1cd3bef8R198-R198)
|
||||
|
||||
```diff
|
||||
-bool isNodePackageExpression = Regex.IsMatch(path, @"^[^/@]+@[^/]+/");
|
||||
+bool isNodePackageExpression = Regex.IsMatch(path, @"^(@[a-z0-9-]+\/[a-z0-9-]+|[a-z0-9-]+)@([0-9]+\.[0-9]+\.[0-9]+|[0-9]+\.[0-9]+|[0-9]+|latest|next|[^/]+)/");
|
||||
```
|
||||
|
||||
|
||||
|
||||
Suggestion importance[1-10]: 7
|
||||
|
||||
__
|
||||
|
||||
Why: The suggestion correctly points out that the original regex `@"^[^/@]+@[^/]+/"` is too simple and might not match all valid node package formats (e.g., scoped packages like `@scope/package@version/`). The improved regex aims to be more comprehensive, enhancing the accuracy of the CDN routing logic.
|
||||
|
||||
___
|
||||
|
||||
</details>
|
||||
|
||||
___
|
||||
|
||||
|
||||
|
||||
<!-- PR --><table><tr><td> <b><a href='2832883684'>PR 242</a></b> (2025-04-27)
|
||||
|
||||
</td></tr></table>
|
||||
|
|
Loading…
Reference in New Issue
Block a user