From 0656c1083ff5f1786cd49d09b7d291f5058b88fd Mon Sep 17 00:00:00 2001 From: root Date: Thu, 26 Jun 2025 01:54:37 +0000 Subject: [PATCH] updated pr-agent best practices with auto analysis --- .pr_agent_auto_best_practices.md | 183 +++++++++++++++++++++++++++++++ 1 file changed, 183 insertions(+) create mode 100644 .pr_agent_auto_best_practices.md diff --git a/.pr_agent_auto_best_practices.md b/.pr_agent_auto_best_practices.md new file mode 100644 index 0000000..a7bac28 --- /dev/null +++ b/.pr_agent_auto_best_practices.md @@ -0,0 +1,183 @@ +Pattern 1: Add null checks and validation before accessing object properties or calling methods to prevent null reference exceptions and runtime errors. This includes checking if objects, arrays, or references exist before using them. + + +Example code before: +``` +public void ProcessData(Schema schema) { + var key = schema.PrimaryKey; // Risk of null reference + this.features.join(','); // Risk if features is not array +} +``` + +Example code after: +``` +public void ProcessData(Schema schema) { + if (schema == null) throw new ArgumentNullException(nameof(schema)); + var key = schema.PrimaryKey; + if (Array.isArray(this.features) && this.features.length > 0) { + this.features.join(','); + } +} +``` +
Relevant past accepted suggestions: + + +___ + + +Suggestion 1: + +Fix null reference exception risk + +**The null check for schema should occur before accessing schema.PrimaryKey to prevent potential null reference exceptions. Move the null validation to the beginning of the constructor.** + +[WelsonJS.Toolkit/WelsonJS.Esent/EsentDatabase.cs [35-41]](https://github.com/gnh1201/welsonjs/pull/279/files#diff-21f279a29d8eeede6f37b552960fc5b0aef7c187c40481256398bd8a318f5590R35-R41) + +```diff + public EsentDatabase(Schema schema, string workingDirectory, ICompatibleLogger logger = null) + { ++ if (schema == null) ++ throw new ArgumentNullException(nameof(schema)); ++ + _logger = logger ?? new TraceLogger(); +- + _primaryKey = schema.PrimaryKey; + +- if (schema == null) +- +``` + + + +___ + + +Suggestion 2: + +Fix grammar and add validation + +**The comment contains a grammatical error ("an unwanted features" should be "unwanted features"). Additionally, consider adding validation to ensure disableFeatures is an array before calling join() to prevent runtime errors.** + +[lib/chrome.js [323-324]](https://github.com/gnh1201/welsonjs/pull/276/files#diff-868ee66369a322a75a321ede18241bfb18f199a9e5bb5371d4d049f0af14f598R323-R324) + +```diff +-// disable an unwanted features +-cmd.push("--disable-features=" + this.disableFeatures.join(',')); ++// disable unwanted features ++if (Array.isArray(this.disableFeatures) && this.disableFeatures.length > 0) { ++ cmd.push("--disable-features=" + this.disableFeatures.join(',')); ++} +``` + + + +___ + + +Suggestion 3: + +Add null check + +**The function doesn't check if promptEditorRef.current exists before calling methods on it, which could lead to runtime errors if the ref isn't initialized.** + +[WelsonJS.Toolkit/WelsonJS.Launcher/editor.html [195-202]](https://github.com/gnh1201/welsonjs/pull/242/files#diff-ccffd2e8a5e0cef355ada30018830cd5516644b2e800c61b2298ac8260d778d5R195-R202) + +```diff + const invoke = () => { + try { +- const updated = promptEditorRef.current.get(); +- promptMessagesRef.current = updated; ++ if (promptEditorRef.current) { ++ const updated = promptEditorRef.current.get(); ++ promptMessagesRef.current = updated; ++ } + } catch (e) { + console.error("Invalid JSON structure", e); + } + }; +``` + + + +
+ + + +___ + + +Pattern 2: Validate string length and bounds before performing substring operations or string manipulations to prevent ArgumentOutOfRangeException and ensure safe string processing. + + +Example code before: +``` +string result = path.Substring("prefix/".Length); +bool isValid = Regex.IsMatch(path, @"^[^/@]+@[^/]+/"); +``` + +Example code after: +``` +string result = path.Length > "prefix/".Length ? path.Substring("prefix/".Length) : path; +bool isValid = Regex.IsMatch(path, @"^(@[a-z0-9-]+\/[a-z0-9-]+|[a-z0-9-]+)@([0-9]+\.[0-9]+\.[0-9]+|latest|[^/]+)/"); +``` +
Relevant past accepted suggestions: + + +___ + + +Suggestion 1: + +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 TryServeFromCdn(HttpListenerContext context, string path) + { + bool isNodePackageExpression = Regex.IsMatch(path, @"^[^/@]+@[^/]+/"); + + var sources = new (bool isMatch, string configKey, Func 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 2: + +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|[^/]+)/"); +``` + + + +
+ + + +___ + +`[Auto-generated best practices - 2025-06-26]` +