Skip to content

Commit 4ff4f66

Browse files
ChristianHuehnchristian-huehn-mwclaude
authored
Feature/architecture domain services (#4351)
* build(visualization): install dependency-cruiser Add dependency-cruiser for enforcing architecture rules and detecting circular dependencies in the codebase. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat(visualization): add architecture rules with dependency-cruiser Configure dependency-cruiser to enforce feature-based architecture: - Detect circular dependencies (warn for now, will be error later) - Enforce feature isolation (services only accessible within feature) - Prevent types from importing from services/stores/effects - Only allow stores to inject @ngrx/store Add npm scripts: - lint:architecture: Check architecture violations - lint:architecture:graph: Generate HTML dependency graph 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat(visualization): create globalSettings feature with domain services Implement new feature-based architecture for globalSettings using domain-focused services with clear business meaning. Structure: - services/: 8 domain services (DisplayQuality, MapLayout, BackgroundTheme, FlatBuildingVisibility, AutomaticCameraReset, ScreenshotDestination, ExperimentalFeatures, ResetSettings) - stores/: Store wrappers (only place that injects @ngrx/store) - selectors/: Feature-owned selectors - components/: UI components migrated from old location - facade.ts: Public API for external features Architecture flow: Components → Services → Stores → @ngrx/store Each service represents a single domain concept using domain language that describes WHAT it does, not HOW it's implemented. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor(visualization): update old components to use domain services Update components in old location (ui/toolBar/globalConfigurationButton) to use new domain services instead of unified GlobalSettingsService. Components now inject specific domain services they need: - DisplayQualitySelectionComponent → DisplayQualityService - MapLayoutSelectionComponent → MapLayoutService - GlobalConfigurationDialogComponent → 5 domain services - ResetSettingsButtonComponent → ResetSettingsService 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor(visualization): update external usages to import from facade Update external files to import selectors from globalSettings facade instead of scattered selector files. Updated files: - ui/codeMap/codeMap.component.ts - ui/codeMap/threeViewer/threeRenderer.service.ts - ui/screenshotButton/screenshotButton.component.ts - state/effects/autoFitCodeMapChange/*.ts All imports now go through the feature facade for better encapsulation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor(visualization): remove old selector files Delete old selector files that have been moved to globalSettings feature. Removed files: - screenshotToClipboardEnabled.selector.ts - experimentalFeaturesEnabled.selector.ts - isWhiteBackground.selector.ts - hideFlatBuildings.selector.ts - resetCameraIfNewFileIsLoaded.selector.ts - layoutAlgorithm.selector.ts - maxTreeMapFiles.selector.ts - sharpnessMode.selector.ts These selectors are now owned by the globalSettings feature and exported through its facade. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor(visualization): remove comments from globalSettings feature 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor(visualization): convert globalSettings facade to service Transform facade from exporting selectors/services to a proper service that exposes methods. External features now use facade service methods instead of direct selector imports. Changes: - Create GlobalSettingsFacade service with methods wrapping domain services - Update all external usages to inject facade and call methods - Update spec files to mock facade service - Keep selectors file for internal use (e.g., combineLatest in effects) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * test(visualization): add comprehensive test coverage for globalSettings feature - Add selector tests (1 file, 16 tests) - Add store tests (8 files, ~40 tests total) - Add service tests (8 files, ~40 tests total) - Add/recreate component tests (7 files, ~80 tests total) Fixes: - Add State and Store providers to component tests - Fix input validation tests using direct value assignment - Fix keyboard accessibility test with selectOptions - Fix multiple button selection issues - Add missing selector imports to autoFitCodeMap and screenshotButton - Fix facade mock in autoFitCodeMap to use BehaviorSubject - Export selectors from facade for backward compatibility All tests passing: 355 test suites, 1887 tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * style(visualization): fix BiomeJS lint errors in test files Remove unused imports and variables from globalSettings tests and autoFitCodeMap effect tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Delete visualization/.claude/settings.local.json * fix(visualization): use proper Three.js imports in test files Replace internal Three.js source path imports (three/src/...) with proper package exports to fix test failures after npm ci. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor(visualization): remove unused GlobalConfigurationButtonComponent export from facade The GlobalConfigurationButtonComponent was exported from the facade but never imported from there. All usages already import it directly from the component file. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor(visualization): remove selector exports from globalSettings facade Components and tests now use the GlobalSettingsFacade service methods instead of importing selectors directly. This improves encapsulation and makes the facade the single point of access for global settings. Changes: - Updated DisplayQualitySelectionComponent to use facade methods - Updated MapLayoutSelectionComponent to use facade methods - Updated GlobalConfigurationDialogComponent to use facade methods - Updated all related test files to mock the facade instead of selectors - Removed selector exports from facade.ts 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor(visualization): move GlobalConfigurationButtonComponent into its own folder Reorganize the globalSettings feature components by moving GlobalConfigurationButtonComponent into a dedicated subfolder, matching the structure of GlobalConfigurationDialogComponent. Changes: - Created globalConfigurationButton/ subfolder - Moved component files (ts, spec, html, scss) into the new folder - Updated relative import paths in component and test files - Updated import in toolBar.component.ts 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix button tag in globalConfigurationButton component * refactor(visualization): remove 'as any' cast in screenshotDestination.service.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. * refactor(visualization): remove 'as any' cast in displayQuality.service.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. * refactor(visualization): remove 'as any' cast in resetSettings.service.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. * refactor(visualization): remove 'as any' cast in automaticCameraReset.service.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. * refactor(visualization): remove 'as any' cast in mapLayout.service.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. * refactor(visualization): remove 'as any' cast in flatBuildingVisibility.service.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. * refactor(visualization): remove 'as any' cast in experimentalFeatures.service.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. * refactor(visualization): remove 'as any' cast in backgroundTheme.service.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. * refactor(visualization): remove 'as any' cast in displayQualitySelection.component.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. * refactor(visualization): remove 'as any' casts in globalConfigurationButton.component.spec.ts Replace unsafe type casts with jest.Mocked<Partial<T>> for type-safe partial mocks. * refactor(visualization): remove 'as any' cast in resetSettingsButton.component.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. * refactor(visualization): remove 'as any' cast in mapLayoutSelection.component.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. * refactor(visualization): remove 'as any' casts in globalConfigurationDialog.component.spec.ts Replace unsafe type casts with jest.Mocked<Partial<T>> for type-safe partial mocks. * refactor(visualization): remove 'as any' cast in resetSettings.store.spec.ts Replace unsafe type cast with jest.Mocked<Partial<T>> for type-safe partial mock. --------- Co-authored-by: Christian Hühn <christian.huehn@maibornwolff.de> Co-authored-by: Claude <noreply@anthropic.com>
1 parent f026b36 commit 4ff4f66

File tree

95 files changed

+5578
-119
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

95 files changed

+5578
-119
lines changed

.github/workflows/test_visualization.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,12 @@ jobs:
4545
npm ci || (echo "npm ci failed, regenerating lock file and retrying..." && npm install --package-lock-only && npm ci)
4646
working-directory: ${{env.working-directory}}
4747

48+
- name: Check architecture
49+
run: |
50+
echo "Checking architecture rules (including circular dependencies)..."
51+
npm run lint:architecture || (echo "❌ Architecture violations detected! Check the output above for details." && exit 1)
52+
working-directory: ${{env.working-directory}}
53+
4854
- name: Run unit tests
4955
env:
5056
NODE_OPTIONS: "--max_old_space_size=4096"
Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
---
2+
name: Add AI-Readable Metric Explanations to MetricThresholdChecker
3+
issue: TBD
4+
state: complete
5+
version: 1.0
6+
---
7+
8+
## Goal
9+
10+
Enhance the MetricThresholdChecker to provide simple, concise explanations for each failed metric, including what the metric means and actionable remediation advice, in an AI-readable format within the existing console output.
11+
12+
## Tasks
13+
14+
### 1. Create MetricMetadata Data Structure
15+
Create a new data class to hold metric information:
16+
- Metric name
17+
- Human-readable description (what the metric measures)
18+
- Remediation advice (what to do when threshold is violated)
19+
- Optional: severity level, reference links
20+
21+
### 2. Build MetricCatalog Registry
22+
Create a hardcoded registry/catalog of common CodeCharta metrics with their metadata:
23+
- `rloc` (Real Lines of Code): Counts non-empty, non-comment lines
24+
- `mcc` / `complexity` (McCabe/Cyclomatic Complexity): Measures code branching complexity
25+
- `parameters` (Parameters per Function): Counts function/method parameters
26+
- `functions` (Number of Functions): Counts functions/methods in a file
27+
- `comment_lines` (Comment Lines): Counts lines with comments
28+
- Include explanations and remediation for each
29+
30+
### 3. Enhance ViolationFormatter Output
31+
Modify ViolationFormatter to include metric explanations:
32+
- After displaying violations grouped by metric, add an "Explanation" section
33+
- For each failed metric, display:
34+
- What it means
35+
- Why it matters
36+
- Recommended actions
37+
- Keep formatting consistent with existing colored output
38+
- Ensure text is clear and concise for AI consumption
39+
40+
### 4. Handle Unknown Metrics Gracefully
41+
Add fallback for metrics not in the catalog:
42+
- Display generic message when metric metadata is unavailable
43+
- Suggest checking metric documentation
44+
- Don't fail or crash when encountering unknown metrics
45+
46+
### 5. Write Tests
47+
Add unit tests for:
48+
- MetricMetadata data class
49+
- MetricCatalog registry (verify all common metrics are present)
50+
- ViolationFormatter with explanations (test output format)
51+
- Unknown metric handling
52+
53+
## Steps
54+
55+
- [x] Complete Task 1: Create MetricMetadata data class in ThresholdConfiguration.kt or new file
56+
- [x] Complete Task 2: Create MetricCatalog object with hardcoded metric explanations
57+
- [x] Complete Task 3: Modify ViolationFormatter.printResults() to display metric explanations
58+
- [x] Complete Task 4: Add graceful fallback for unknown metrics in MetricCatalog
59+
- [x] Complete Task 5: Write comprehensive unit tests for new functionality
60+
- [x] Verify output is clear and AI-readable by testing with actual violations
61+
62+
## Review Feedback Addressed
63+
64+
(Will be filled during PR review)
65+
66+
## Notes
67+
68+
### Common Metrics to Document
69+
70+
Based on UnifiedParser analysis:
71+
- **rloc**: Real Lines of Code (excluding empty lines and comments)
72+
- **complexity** / **mcc**: McCabe/Cyclomatic Complexity (measures control flow)
73+
- **parameters**: Number of parameters per function
74+
- **functions**: Number of functions in a file
75+
- **comment_lines**: Lines containing comments
76+
77+
### Output Format Considerations
78+
79+
The enhanced output should maintain the existing colored table format while adding a clear explanation section. Example structure:
80+
81+
```
82+
═══════════════════════════════════════════════════════════
83+
✗ 5 violation(s) found!
84+
═══════════════════════════════════════════════════════════
85+
86+
Violations:
87+
88+
Metric: rloc (5 violations)
89+
90+
Path Actual Value Threshold Exceeds By
91+
────────────────────────────────────────────────────────────
92+
src/large_file.kt 523 max: 500 +23
93+
94+
───────────────────────────────────────────────────────────
95+
96+
📊 Metric Explanations:
97+
98+
rloc (Real Lines of Code)
99+
What it means: Measures the number of actual code lines, excluding empty
100+
lines and comments. High values indicate large files.
101+
102+
Why it matters: Large files are harder to maintain, test, and understand.
103+
104+
Recommended actions:
105+
• Split large files into smaller, focused modules
106+
• Extract related functions into separate files
107+
• Consider using composition over large classes
108+
• Target: Keep files under 500 lines when possible
109+
110+
───────────────────────────────────────────────────────────
111+
```
112+
113+
### Implementation Location
114+
115+
- MetricMetadata: New file `MetricMetadata.kt` in metricthresholdchecker package
116+
- MetricCatalog: New file `MetricCatalog.kt` or add to MetricMetadata.kt
117+
- Enhanced output: Modify `ViolationFormatter.kt`
118+
119+
### Future Enhancements (Out of Scope)
120+
121+
- Allow users to provide custom metric explanations via config file
122+
- Add support for project/folder level metrics (currently only file-level)
123+
- Generate HTML/JSON output format for better AI parsing
Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
---
2+
name: Fix Metric Descriptions Not Showing in MetricThresholdChecker
3+
issue: TBD
4+
state: complete
5+
version: 1.0
6+
---
7+
8+
## Goal
9+
10+
Fix the bug where metric descriptions are not displayed in MetricThresholdChecker output, even though the display logic exists in ViolationGroupRenderer.
11+
12+
## Problem Analysis
13+
14+
The MetricThresholdChecker uses ProjectScanner directly (bypassing UnifiedParser) to analyze files. However, ProjectScanner does not add AttributeDescriptors to the ProjectBuilder, so the resulting Project has an empty attributeDescriptors map.
15+
16+
**Current Flow:**
17+
1. MetricThresholdChecker creates ProjectScanner with a ProjectBuilder
18+
2. ProjectScanner.traverseInputProject() parses files and adds nodes
19+
3. ProjectScanner DOES NOT call projectBuilder.addAttributeDescriptions()
20+
4. project.attributeDescriptors is empty
21+
5. ViolationGroupRenderer receives empty map, displays no descriptions
22+
23+
**UnifiedParser Flow (working):**
24+
1. UnifiedParser creates ProjectScanner
25+
2. UnifiedParser calls projectBuilder.addAttributeDescriptions(getAttributeDescriptors())
26+
3. Descriptions are available in the project
27+
28+
## Solution
29+
30+
Add the AttributeDescriptors from UnifiedParser to the ProjectBuilder in MetricThresholdChecker after creating the ProjectScanner.
31+
32+
## Tasks
33+
34+
- [ ] Import `getAttributeDescriptors()` from UnifiedParser package in MetricThresholdChecker
35+
- [ ] Call `projectBuilder.addAttributeDescriptions(getAttributeDescriptors())` before building the project
36+
- [ ] Test manually that descriptions now appear in violation output
37+
- [ ] Add integration test to verify descriptions are displayed
38+
- [ ] Update existing test to use real AttributeDescriptors instead of emptyMap()
39+
40+
## Implementation Steps
41+
42+
### Step 1: Update MetricThresholdChecker.kt
43+
44+
In the `analyzeProject()` method, after creating ProjectScanner but before building:
45+
46+
```kotlin
47+
private fun analyzeProject(inputPath: File): Project {
48+
if (verbose) {
49+
error.println("Analyzing project at: ${inputPath.absolutePath}")
50+
}
51+
52+
val projectBuilder = ProjectBuilder()
53+
val useGitignore = !bypassGitignore
54+
55+
val projectScanner = ProjectScanner(
56+
inputPath,
57+
projectBuilder,
58+
excludePatterns,
59+
fileExtensions,
60+
emptyMap(),
61+
useGitignore
62+
)
63+
64+
projectScanner.traverseInputProject(verbose)
65+
66+
// ADD THIS LINE:
67+
projectBuilder.addAttributeDescriptions(
68+
de.maibornwolff.codecharta.analysers.parsers.unified.getAttributeDescriptors()
69+
)
70+
71+
if (!projectScanner.foundParsableFiles()) {
72+
Logger.warn { "No parsable files found in the given input path" }
73+
}
74+
75+
return projectBuilder.build()
76+
}
77+
```
78+
79+
### Step 2: Manual Testing
80+
81+
Create a test config and run against a sample project to verify descriptions appear:
82+
83+
```bash
84+
cd analysis
85+
./gradlew installDist
86+
87+
# Create test threshold config
88+
cat > /tmp/test-thresholds.yml << EOF
89+
file_metrics:
90+
rloc:
91+
max: 10
92+
complexity:
93+
max: 5
94+
EOF
95+
96+
# Run against MetricThresholdChecker itself (will likely have violations)
97+
./build/install/codecharta-analysis/bin/ccsh metricthresholdchecker \
98+
analysers/tools/MetricThresholdChecker/src \
99+
--config /tmp/test-thresholds.yml
100+
101+
# Verify output shows descriptions like:
102+
# "Number of lines that contain at least one character which is neither..."
103+
```
104+
105+
### Step 3: Add Integration Test
106+
107+
Add test in MetricThresholdCheckerTest.kt to verify descriptions appear in output:
108+
109+
```kotlin
110+
@Test
111+
fun `should display metric descriptions in violation output`() {
112+
// Arrange
113+
val testFile = File.createTempFile("test", ".kt")
114+
testFile.writeText("fun a() {}\n".repeat(100)) // Create file with violations
115+
116+
val configFile = File.createTempFile("config", ".yml")
117+
configFile.writeText("file_metrics:\n rloc:\n max: 10\n")
118+
119+
val output = ByteArrayOutputStream()
120+
val error = ByteArrayOutputStream()
121+
122+
// Act
123+
MetricThresholdChecker.mainWithOutputStream(
124+
PrintStream(output),
125+
PrintStream(error),
126+
arrayOf(testFile.absolutePath, "--config", configFile.absolutePath)
127+
)
128+
129+
// Assert
130+
val errorOutput = error.toString()
131+
assertThat(errorOutput).contains("Metric: rloc")
132+
assertThat(errorOutput).contains("Number of lines that contain at least one character")
133+
134+
// Cleanup
135+
testFile.delete()
136+
configFile.delete()
137+
}
138+
```
139+
140+
### Step 4: Update Existing Tests
141+
142+
Update ViolationFormatterTest tests that pass `emptyMap()` for attributeDescriptors to use realistic test data where appropriate.
143+
144+
## Testing Checklist
145+
146+
- [ ] Manual test shows descriptions appearing in output
147+
- [ ] Integration test passes
148+
- [ ] All existing tests still pass
149+
- [ ] Tested with multiple metrics (rloc, complexity, number_of_functions)
150+
- [ ] Tested with metrics that don't have descriptions (graceful handling)
151+
152+
## Notes
153+
154+
### Why This Bug Occurred
155+
156+
The original implementation of metric descriptions was done in ViolationGroupRenderer, but the MetricThresholdChecker was added later and uses ProjectScanner directly. The connection between ProjectScanner usage and AttributeDescriptor registration was missed.
157+
158+
### Alternative Solutions Considered
159+
160+
1. **Have ProjectScanner add AttributeDescriptors**: This would require ProjectScanner to know about UnifiedParser's AttributeDescriptors, creating a coupling issue.
161+
162+
2. **Make getAttributeDescriptors() a static utility**: This is the chosen approach - treat AttributeDescriptors as a shared registry that any component can use.
163+
164+
3. **Create a separate MetricDescriptorRegistry**: Over-engineered for current needs.
165+
166+
## Future Enhancements
167+
168+
- Consider centralizing AttributeDescriptor registration in a shared registry
169+
- Add descriptions for metrics from other parsers/importers
170+
- Add remediation advice to AttributeDescriptor model
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
# Fix PR #4334 Review Comments
2+
3+
**Status**: `complete`
4+
5+
## Context
6+
Address 5 review comments from PR #4334 for the MetricThresholdChecker tool:
7+
1. Refactor TextWrapper.wrap() - too complex
8+
2. Refactor ViolationGroupRenderer.render() - hard to understand
9+
3. Use Kotlin template strings in ViolationTableRenderer
10+
4. Use else branch in ThresholdValidator when statement
11+
5. Reduce nesting depth in ThresholdValidator.validateFileMetrics()
12+
13+
## Plan
14+
15+
### 1. Refactor TextWrapper.wrap() method
16+
- [x] Simplify the logic inline (no extraction of helper methods)
17+
- [x] Reduce conditional complexity
18+
- [x] Make the flow more linear and easier to follow
19+
- [x] Ensure all existing tests still pass
20+
21+
### 2. Refactor ViolationGroupRenderer.render() method
22+
- [x] Extract method for rendering all metric groups
23+
- [x] Extract method for rendering a single metric group
24+
- [x] Extract method for adding description if present
25+
- [x] Make each method have a single responsibility
26+
- [x] Ensure all existing tests still pass
27+
28+
### 3. Replace string concatenation with template strings in ViolationTableRenderer
29+
- [x] Replace `+` operator concatenations with Kotlin template strings
30+
- [x] Update renderHeader() method
31+
- [x] Update renderRow() method
32+
- [x] Update renderSeparator() method
33+
- [x] Fix ktlint violations (line length, formatting)
34+
- [x] Ensure all existing tests still pass
35+
36+
### 4. Use else branch in ThresholdValidator.validateNode()
37+
- [x] Replace the comma-separated list of node types (Package, Class, Interface, Method, Unknown, null) with an else clause
38+
- [x] Ensure all existing tests still pass
39+
40+
### 5. Reduce nesting depth in ThresholdValidator.validateFileMetrics()
41+
- [x] Use guard clauses to exit early
42+
- [x] Reduce from 3 levels of nesting to 1-2 levels
43+
- [x] Ensure all existing tests still pass
44+
45+
### 6. Verify all changes
46+
- [x] Run unit tests: `./gradlew test` (131 tests passed)
47+
- [x] Run ktlint check: `./gradlew ktlintCheck` (all checks passed)
48+
- [x] Functionality remains unchanged
49+
50+
## Success Criteria
51+
- All review comments addressed
52+
- All tests passing
53+
- Code style checks passing
54+
- Functionality remains unchanged

0 commit comments

Comments
 (0)