Repository navigation
Reduce Codacy issues towards an A grade - #29
Conversation
…d ESLint config, tidy C# Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 2 high |
| Security | 2 critical 3 high |
| Complexity | 9 medium |
🟢 Metrics -5 complexity · 0 duplication
Metric Results Complexity -5 Duplication 0
🟢 Coverage 0.00% diff coverage · -0.06% coverage variation
Metric Results Coverage variation ✅ -0.06% coverage variation (-1.00%) Diff coverage ✅ 0.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (efb8776) 790 363 45.95% Head commit (ab841da) 791 (+1) 363 (+0) 45.89% (-0.06%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#29) 1 0 0.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
While this PR successfully addresses a large volume of technical debt and standardizes the codebase with ESLint and Prettier, it currently fails to meet quality standards due to critical logic and security regressions. Most importantly, the transition to non-deterministic GUIDs in SchemaGenerator.cs will cause users to lose their saved visual layouts, which is a major functional regression.
Several security risks were identified in the JavaScript templates, including potential ReDoS vulnerabilities in table-grouping.js and weak random number generation in controls.js. Furthermore, significant logic refactors in the Core and GitHub services lack accompanying unit tests, which is particularly concerning for the CoreSchemaAnalysisService, as it is a high-complexity file with decreasing test coverage on its new code paths.
About this PR
- Several logic refactors, specifically the directory prioritization in 'GitHubService.cs', were implemented without new unit tests. Given the complexity of these changes, automated verification is required to prevent regressions in entity discovery.
- The transition to 'Write-Information' in PowerShell scripts removes default color highlighting. This may negatively impact the readability of script output during manual executions. Consider if the stream management benefits outweigh the loss of visual feedback.
Test suggestions
- Verify that 'CoreSchemaAnalysisService.FindSolutionDirectory' correctly identifies the solution root and handles null/empty inputs.
- Validate the refactored directory priority logic in 'GitHubService' to ensure entities are still correctly prioritized by path (models, entities, domain, data).
- Ensure 'GitHubService.ParseGitHubUrl' correctly rejects non-GitHub hostnames and returns null.
- Add unit tests for 'analyzeRelationshipCardinality' in 'relationships.js' to cover varied EF Core relationship patterns given its high complexity.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that 'CoreSchemaAnalysisService.FindSolutionDirectory' correctly identifies the solution root and handles null/empty inputs.
2. Validate the refactored directory priority logic in 'GitHubService' to ensure entities are still correctly prioritized by path (models, entities, domain, data).
3. Ensure 'GitHubService.ParseGitHubUrl' correctly rejects non-GitHub hostnames and returns null.
4. Add unit tests for 'analyzeRelationshipCardinality' in 'relationships.js' to cover varied EF Core relationship patterns given its high complexity.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace( | ||
| /[xy]/g, | ||
| function (c) { | ||
| const r = (Math.random() * 16) | 0; |
There was a problem hiding this comment.
🔴 HIGH RISK
Replace Math.random() with the Web Crypto API to ensure cryptographically strong and unique identifiers. The current implementation can lead to collisions, potentially corrupting user data in localStorage.
Suggested fix:
const r = crypto.getRandomValues(new Uint8Array(1))[0] % 16 | 0;| var entitiesJson = JsonSerializer.Serialize(result.Entities, _jsonSerializerOptions); | ||
| var documentGuid = _documentGuid ?? Guid.NewGuid().ToString(); | ||
| htmlContent = ModularHtmlTemplate.GenerateWithCustomCss(entitiesJson, documentGuid, customCss); | ||
| var customCssGuid = _documentGuid ?? Guid.NewGuid().ToString(); |
There was a problem hiding this comment.
🔴 HIGH RISK
Use the deterministic GUID from the analysis result to ensure layout persistence when custom CSS is applied. Using Guid.NewGuid() here breaks the tool's ability to retrieve saved table positions from localStorage, as the document GUID will change on every regeneration.
| const dateRect = document.createElementNS( | ||
| "http://www.w3.org/2000/svg", | ||
| "rect", | ||
| ); | ||
| dateRect.setAttribute("x", centerX - 10); // Increased from 4 to 10 (2.5x larger) | ||
| dateRect.setAttribute("y", textY - 8); // Increased from 3 to 8 (2.5x larger) | ||
| dateRect.setAttribute("width", "20"); // Increased from 8 to 20 (2.5x larger) | ||
| dateRect.setAttribute("height", "16"); // Increased from 6 to 16 (2.5x larger) | ||
| dateRect.setAttribute("fill", "none"); | ||
| dateRect.setAttribute("stroke", strokeColor); | ||
| dateRect.setAttribute("stroke-width", "2"); // Increased from 0.8 to 2 (2.5x larger) | ||
| dateRect.classList.add("type-icon-shape-large"); | ||
| elements.push(dateRect); | ||
|
|
||
| const dateLine = document.createElementNS('http://www.w3.org/2000/svg', 'line'); | ||
| dateLine.setAttribute('x1', centerX - 8); // Increased from 3 to 8 (2.5x larger) | ||
| dateLine.setAttribute('y1', textY - 2); // Increased from 1 to 2 (2.5x larger) | ||
| dateLine.setAttribute('x2', centerX + 8); // Increased from 3 to 8 (2.5x larger) | ||
| dateLine.setAttribute('y2', textY - 2); // Increased from 1 to 2 (2.5x larger) | ||
| dateLine.setAttribute('stroke', strokeColor); | ||
| dateLine.setAttribute('stroke-width', '2'); // Increased from 0.8 to 2 (2.5x larger) | ||
| dateLine.classList.add('type-icon-shape-large'); | ||
|
|
||
| const dateLine = document.createElementNS( | ||
| "http://www.w3.org/2000/svg", | ||
| "line", | ||
| ); | ||
| dateLine.setAttribute("x1", centerX - 8); // Increased from 3 to 8 (2.5x larger) | ||
| dateLine.setAttribute("y1", textY - 2); // Increased from 1 to 2 (2.5x larger) | ||
| dateLine.setAttribute("x2", centerX + 8); // Increased from 3 to 8 (2.5x larger) | ||
| dateLine.setAttribute("y2", textY - 2); // Increased from 1 to 2 (2.5x larger) | ||
| dateLine.setAttribute("stroke", strokeColor); | ||
| dateLine.setAttribute("stroke-width", "2"); // Increased from 0.8 to 2 (2.5x larger) | ||
| dateLine.classList.add("type-icon-shape-large"); | ||
| elements.push(dateLine); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Wrap the case clause content in curly braces {} to create a lexical scope for dateRect and dateLine. Declaring them directly inside the case makes them visible to the entire switch block, which can cause runtime errors.
| const regex = new RegExp(rule.pattern, 'i'); | ||
| const matches = entityNames.filter(name => regex.test(name)); | ||
| const regex = new RegExp(rule.pattern, "i"); |
There was a problem hiding this comment.
🔴 HIGH RISK
Validate the regex pattern before construction or use a timeout-based regex execution strategy to mitigate Regular Expression Denial of Service (ReDoS) risks, especially since patterns are retrieved from localStorage.
| Write-Host "?? SchemaMagic Manual Publish Script" -ForegroundColor Cyan | ||
| Write-Host "=====================================" -ForegroundColor Cyan | ||
| Write-Host "" | ||
| Write-Information "" -InformationAction Continue |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Setting the information preference globally at the start of the script ($InformationPreference = 'Continue') is cleaner and more maintainable than appending the action parameter to every individual call.
| const rowHeight = 55; // Increased from 22 to 55 (2.5x larger) | ||
| const headerHeight = 88; // Increased from 35 to 88 (2.5x larger) | ||
| const inheritanceHeight = (entity.baseType && showInheritedProperties) ? 50 : 0; // Increased from 20 to 50 (2.5x larger) | ||
| const inheritanceHeight = | ||
| entity.baseType && showInheritedProperties ? 50 : 0; // Increased from 20 to 50 (2.5x larger) | ||
| const padding = 30; // Increased from 12 to 30 (2.5x larger) | ||
| const minWidth = 1000; // Increased from 400 to 1000 (2.5x larger) | ||
| const iconWidth = 60; // Increased from 24 to 60 (2.5x larger) |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Table dimension constants are duplicated here and in Templates/force-directed-layout.js. Move these to a shared object in variables.js to ensure the layout engine and renderer remain synchronized.
| } | ||
|
|
||
| function analyzeRelationshipCardinality(fromEntityName, toEntityName, propertyName) { | ||
| function analyzeRelationshipCardinality( |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: This logic (complexity: 22) is highly complex and error-prone. Consider refactoring analyzeRelationshipCardinality by extracting helper functions for navigation property detection and optionality analysis to ensure all EF Core patterns are correctly handled.
| const fkProperty = fromEntity.properties.find( | ||
| (p) => p.name === propertyName, |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The variable fkProperty is redundant and unused; fkProp already handles both immediate and inherited properties. Removing this supports the PR's objective of reducing technical debt.
| private static string? FindSolutionDirectory(string? startDirectory) | ||
| { | ||
| if (string.IsNullOrEmpty(startDirectory)) | ||
| { |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The added null/empty check for 'startDirectory' has no test coverage. Given that this file is identified as complex and has decreasing overall coverage, please add a unit test to verify this boundary condition.
| setTimeout(() => URL.revokeObjectURL(url), 1000); | ||
| const blob = new Blob([htmlContent], { type: "text/html" }); | ||
| const url = URL.createObjectURL(blob); | ||
| const newWindow = window.open(url, "_blank"); |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Remove the unused newWindow variable assignment. If the intent is to verify if the window opened, add a null check instead.
Codacy was at C (69): 843 issues, 543 of them Prettier formatting in the JS files.
Build and the 20 tests pass. Remaining work is complexity/duplication (HtmlTemplate.cs, Program.Main, relationships.js etc.).
🤖 Generated with Claude Code