mirror of
https://github.com/gnh1201/welsonjs.git
synced 2025-07-13 12:08:09 +00:00
updated pr-agent best practices with auto analysis
root
parent
43a739599a
commit
0656c1083f
183
.pr_agent_auto_best_practices.md
Normal file
183
.pr_agent_auto_best_practices.md
Normal file
|
|
@ -0,0 +1,183 @@
|
|||
<b>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.
|
||||
</b>
|
||||
|
||||
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(',');
|
||||
}
|
||||
}
|
||||
```
|
||||
<details><summary>Relevant past accepted suggestions:</summary>
|
||||
|
||||
|
||||
___
|
||||
|
||||
<b>
|
||||
Suggestion 1</b>:
|
||||
|
||||
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)
|
||||
-
|
||||
```
|
||||
|
||||
|
||||
|
||||
___
|
||||
|
||||
<b>
|
||||
Suggestion 2</b>:
|
||||
|
||||
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(','));
|
||||
+}
|
||||
```
|
||||
|
||||
|
||||
|
||||
___
|
||||
|
||||
<b>
|
||||
Suggestion 3</b>:
|
||||
|
||||
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);
|
||||
}
|
||||
};
|
||||
```
|
||||
|
||||
|
||||
|
||||
</details>
|
||||
|
||||
|
||||
|
||||
___
|
||||
|
||||
|
||||
<b>Pattern 2: Validate string length and bounds before performing substring operations or string manipulations to prevent ArgumentOutOfRangeException and ensure safe string processing.
|
||||
</b>
|
||||
|
||||
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|[^/]+)/");
|
||||
```
|
||||
<details><summary>Relevant past accepted suggestions:</summary>
|
||||
|
||||
|
||||
___
|
||||
|
||||
<b>
|
||||
Suggestion 1</b>:
|
||||
|
||||
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
|
||||
};
|
||||
```
|
||||
|
||||
|
||||
|
||||
___
|
||||
|
||||
<b>
|
||||
Suggestion 2</b>:
|
||||
|
||||
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|[^/]+)/");
|
||||
```
|
||||
|
||||
|
||||
|
||||
</details>
|
||||
|
||||
|
||||
|
||||
___
|
||||
|
||||
`[Auto-generated best practices - 2025-06-26]`
|
||||
|
||||
Loading…
Reference in New Issue
Block a user