Merged
Conversation
…used three-orbit-controls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sting optimization - Dispose render target buffers, arrow geometry/materials, and floor label textures - Use addUpdateRange for partial color buffer uploads instead of full buffer on hover - Pre-compute scaled bounding boxes in setScales() and use Map for O(1) path lookups - Replace alignment cube with Vector3 math, remove duplicate dispose call - Guard stats rAF loop to skip in production mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dges Import .dc.json files produced by DependaCharta and convert class/function-level dependencies into aggregated file-level edges in cc.json format. Includes full ccsh integration, interactive dialog, documentation, and 87% test coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ze highlighting Skip bottom face indices in the treemap rendering (never visible), reducing GPU index count by ~17%. Restore bottom faces in the 3D print export to keep meshes watertight. Cache highlighted/dimmed color vectors on CodeMapBuilding to avoid repeated HSL conversions. Replace highlighted-building array with a Set for O(1) lookups. Batch render calls via requestAnimationFrame. Update label line animation to modify vertices in-place. Fix edge preview effect to respect user-chosen count of 0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a new DependaChartaImporter to convert DependaCharta .dc.json files into CodeCharta edge metrics and integrates it into CCSH. Large visualization refactors: Three.js upgrade, BVH raycasting, InstancedMesh rendering, label/tooltip/renderer redesigns, resource cleanup, many tests, docs, and planning docs added. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as DependaChartaImporter<br/> (CLI)
participant Parser as DcJsonParser
participant Builder as ProjectBuilder
participant Serializer as ProjectSerializer
participant File as OutputFile
User->>CLI: provide .dc.json
CLI->>CLI: validate input file
CLI->>Parser: parse JSON -> DcProject
Parser->>Parser: resolve dependency paths & aggregate edges
Parser-->>CLI: return List<Edge>
CLI->>Builder: add edges, set attribute types/descriptors
Builder-->>CLI: build Project
CLI->>Serializer: serialize Project (gzip optional)
Serializer->>File: write .cc.json
File-->>User: output file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
visualization/app/codeCharta/ui/codeMap/threeViewer/threeMapControls.service.ts (1)
100-108: Avoid per-callVector3allocations in the camera rotation hot path.
lookAtDirectionFromTargetcan be invoked frequently; allocating a newVector3each time can add GC pressure. Consider reusing a cached vector on the service.♻️ Proposed refactor
export class ThreeMapControlsService { static readonly CAMERA_CHANGED_EVENT_NAME = "camera-changed" MAX_ZOOM = 200 MIN_ZOOM = 10 controls: MapControls + private readonly lookAtPoint = new Vector3() private readonly eventEmitter = new EventEmitter<CameraChangeEvents>() zoomPercentage$ = new BehaviorSubject<number>(100) @@ - const lookAtPoint = new Vector3(this.controls.target.x + adjustedX, this.controls.target.y + y, this.controls.target.z + z) - - this.threeCameraService.camera.lookAt(lookAtPoint) + this.lookAtPoint.set(this.controls.target.x + adjustedX, this.controls.target.y + y, this.controls.target.z + z) + this.threeCameraService.camera.lookAt(this.lookAtPoint)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/threeViewer/threeMapControls.service.ts` around lines 100 - 108, lookAtDirectionFromTarget allocates a new Vector3 (lookAtPoint) on every call which adds GC pressure in this hot path; create a reusable Vector3 property on the service (e.g., this.reusableLookAt or similar) and reuse it by setting its x/y/z each call instead of instantiating a new Vector3, then pass that reused vector to threeCameraService.camera.lookAt; update any tiny-offset logic to set the reused vector values before calling lookAt.visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts (1)
33-48: Consider enforcing immutability on node paths or codify update patterns.
ThebuildingsByPathmap is only populated inadd(), but the underlyingNodeinterface is mutable and thebuildingsgetter exposes the array. While no mutations are currently detected in production code, defensive measures could prevent future regressions: either document thatnode.pathmust never change post-insertion, or update the map whensetNode()is called with a different path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts` around lines 33 - 48, The buildingsByPath map is only populated in add() but Node.path is mutable and the buildings getter exposes the array, risking stale keys when a node's path changes; either enforce immutability by documenting/throwing if Node.path differs in setNode()/add() or proactively update buildingsByPath inside setNode() to handle path changes: detect when the incoming Node's path !== existingNode.path, remove the old key from buildingsByPath and reinsert with the new path (ensure getBuildingByPath, add, setNode, buildingsByPath and the buildings getter are updated accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@analysis/analysers/importers/DependaChartaImporter/build.gradle.kts`:
- Around line 8-9: The dependency declaration uses
implementation(libs.kotter.test) which places kotter.test in the production
classpath; change that to testImplementation(libs.kotter.test) so kotter.test is
only available to test sources (e.g., DialogTest.kt using testSession). Update
the dependency statement in the build.gradle.kts where
implementation(libs.kotter.test) appears to use testImplementation instead.
In
`@analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/AttributeDescriptors.kt`:
- Around line 5-17: The analyzer name in getAttributeDescriptors currently uses
a hard-coded setOf("dependaChartaImporter") which mismatches
DependaChartaImporter.NAME; update the analyzerName definition in
getAttributeDescriptors to use the canonical identifier by replacing the
hard-coded string with setOf(DependaChartaImporter.NAME) (or change the string
to "dependachartaimport") so the AttributeDescriptor for "dependencies" is
associated with DependaChartaImporter correctly.
In
`@analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/DcJsonParser.kt`:
- Around line 9-18: parseEdges currently undercounts dependencies because
resolveDependencyPaths only iterates dependency keys and drops
DcDependency.weight; update resolveDependencyPaths (or its caller parseEdges) to
iterate the actual DcDependency objects from DcProject.leaves[...] .dependencies
(not just keys) and emit pairs annotated with the dependency weight, then change
the aggregation in parseEdges to sum weights (e.g., groupingBy the source/target
pair and reducing by summing dependency.weight) when creating Edge(...) so Edge
metadata uses the total summed weight rather than simple occurrence count.
In
`@analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/DependaChartaImporter.kt`:
- Around line 33-35: The -nc/--not-compressed flag currently binds to a boolean
named compress (default true) so the flag has no effect; change the CLI field to
a negated name (e.g., private var notCompressed = false in
DependaChartaImporter) and pass the inverse when configuring serialization (use
!notCompressed where compress was used), and apply the same change to the
equivalent fields in TokeiImporter and SourceMonitorImporter (update their CLI
option annotations to use --not-compressed and their serializer calls to use the
negation).
In
`@analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/Dialog.kt`:
- Around line 21-38: The returned argument list emits "--output-file=" even when
outputFileName is empty; update the return construction in Dialog.kt so the
"--output-file=…" flag is only included when outputFileName is non-empty (e.g.
conditionally include "--output-file=$outputFileName" using
outputFileName.isNotEmpty() in the listOfNotNull), keep the existing logic for
the compression flag (isCompressed) unchanged.
In `@gh-pages/_docs/04-importer/07-dependacharta.md`:
- Line 12: The doc uses mixed US/UK spelling for
"visualization"/"visualisation"; update the documentation text for
DependaChartaImporter to use a single variant (preferably US "visualization") by
replacing all instances of "visualisation" with "visualization" in this file
(including the line referencing DependaChartaImporter and the occurrence around
line 55) so the terminology is consistent throughout.
- Around line 29-31: The fenced usage block in the document lacks a language tag
(MD040); update the code fence for the line "Usage: ccsh dependachartaimport
[-h] [-nc] [-o=<outputFile>] FILE" to include a language identifier (e.g.,
```text) so the block becomes a proper fenced code block with a language
specifier.
In
`@visualization/app/codeCharta/services/3DExports/3DPreview/MeshModels/mapMesh.ts`:
- Around line 86-89: The code in mapMesh.ts creates a Uint32Array view over
oldIndex.array.buffer (oldIndex, indicesPerNode) which is unsafe if
oldIndex.array is a Uint16Array and can throw a RangeError; instead, allocate
newIndexArray (Uint32Array) and copy values from oldIndex.array
element-by-element (or via Array.from(oldIndex.array)) into newIndexArray so you
don't reinterpret the underlying buffer; update the existing
newIndexArray.set(...) usage to set from the copied 32-bit values rather than
creating a Uint32Array view over the original buffer.
In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapBuilding.spec.ts`:
- Around line 113-131: The test suite overrides ColorConverter.colorToVector3
and mutates (ColorConverter as any).colorToVector3Map but never restores them;
add an afterAll (or afterEach) that restores ColorConverter.colorToVector3 from
the saved originalColorToVector3 and resets (ColorConverter as
any).colorToVector3Map to a new Map() so the global implementation and cache are
returned to their original state after the suite finishes.
In
`@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts`:
- Around line 19-22: The add() method currently pushes new CodeMapBuilding
instances into _buildings and buildingsByPath but doesn't update the scaledBoxes
cache, causing buildings added after setScales() to be missing from intersect()
tests; update add(building: CodeMapBuilding) to compute and insert the
building's scaled box into scaledBoxes (using the same scaling logic/transform
used by setScales() or the helper that computes scaled boxes) so that
scaledBoxes.set(building.node.path, computedScaledBox) is called whenever a
building is added, keeping scaledBoxes in sync with _buildings and preventing
misses in intersect().
---
Nitpick comments:
In
`@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts`:
- Around line 33-48: The buildingsByPath map is only populated in add() but
Node.path is mutable and the buildings getter exposes the array, risking stale
keys when a node's path changes; either enforce immutability by
documenting/throwing if Node.path differs in setNode()/add() or proactively
update buildingsByPath inside setNode() to handle path changes: detect when the
incoming Node's path !== existingNode.path, remove the old key from
buildingsByPath and reinsert with the new path (ensure getBuildingByPath, add,
setNode, buildingsByPath and the buildings getter are updated accordingly).
In
`@visualization/app/codeCharta/ui/codeMap/threeViewer/threeMapControls.service.ts`:
- Around line 100-108: lookAtDirectionFromTarget allocates a new Vector3
(lookAtPoint) on every call which adds GC pressure in this hot path; create a
reusable Vector3 property on the service (e.g., this.reusableLookAt or similar)
and reuse it by setting its x/y/z each call instead of instantiating a new
Vector3, then pass that reused vector to threeCameraService.camera.lookAt;
update any tiny-offset logic to set the reused vector values before calling
lookAt.
Comment on lines
+8
to
+9
| implementation(libs.kotter) | ||
| implementation(libs.kotter.test) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the build.gradle.kts file
find . -name "build.gradle.kts" -path "*/DependaChartaImporter/*" 2>/dev/nullRepository: MaibornWolff/codecharta
Length of output: 136
🏁 Script executed:
# Check the directory structure of DependaChartaImporter
fd . analysis/analysers/importers/DependaChartaImporter --type f 2>/dev/null | head -20Repository: MaibornWolff/codecharta
Length of output: 1675
🏁 Script executed:
# Search for kotter.test usage in main sources
find analysis/analysers/importers/DependaChartaImporter/src/main -name "*.kt" -type f 2>/dev/null -exec grep -l "kotter" {} \;Repository: MaibornWolff/codecharta
Length of output: 204
🏁 Script executed:
# Search for kotter.test usage in test sources
find analysis/analysers/importers/DependaChartaImporter/src/test -name "*.kt" -type f 2>/dev/null -exec grep -l "kotter" {} \;Repository: MaibornWolff/codecharta
Length of output: 208
🏁 Script executed:
# Read the build.gradle.kts file to see current state
cat -n analysis/analysers/importers/DependaChartaImporter/build.gradle.kts 2>/dev/null || echo "File not found, attempting alternative path search"Repository: MaibornWolff/codecharta
Length of output: 664
🏁 Script executed:
# Check imports in Dialog.kt (main source)
cat -n analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/Dialog.kt | head -40Repository: MaibornWolff/codecharta
Length of output: 2053
🏁 Script executed:
# Check imports in DialogTest.kt (test source)
cat -n analysis/analysers/importers/DependaChartaImporter/src/test/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/DialogTest.kt | head -40Repository: MaibornWolff/codecharta
Length of output: 1792
🏁 Script executed:
# More specific search for kotter.test imports
rg "import.*kotter\.test|from.*kotter\.test" analysis/analysers/importers/DependaChartaImporter/src --type kt -A 2Repository: MaibornWolff/codecharta
Length of output: 93
🏁 Script executed:
# Let's search for testCallback usage to understand the context better
rg "testCallback" analysis/analysers/importers/DependaChartaImporter/src --type-list | head -5Repository: MaibornWolff/codecharta
Length of output: 456
🏁 Script executed:
# Search for testCallback definition
rg "fun testCallback|testCallback\(\)" analysis/analysers/importers/DependaChartaImporter/src -A 3Repository: MaibornWolff/codecharta
Length of output: 4776
kotter.test should be testImplementation, not implementation.
The kotter.test library is only used in test sources (DialogTest.kt imports testSession from it). Placing it in implementation scope unnecessarily bundles it into the production artifact.
♻️ Proposed fix
implementation(libs.kotter)
- implementation(libs.kotter.test)
testImplementation(libs.junit.jupiter.api)
testImplementation(libs.kotlin.test)
+ testImplementation(libs.kotter.test)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| implementation(libs.kotter) | |
| implementation(libs.kotter.test) | |
| implementation(libs.kotter) | |
| testImplementation(libs.junit.jupiter.api) | |
| testImplementation(libs.kotlin.test) | |
| testImplementation(libs.kotter.test) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@analysis/analysers/importers/DependaChartaImporter/build.gradle.kts` around
lines 8 - 9, The dependency declaration uses implementation(libs.kotter.test)
which places kotter.test in the production classpath; change that to
testImplementation(libs.kotter.test) so kotter.test is only available to test
sources (e.g., DialogTest.kt using testSession). Update the dependency statement
in the build.gradle.kts where implementation(libs.kotter.test) appears to use
testImplementation instead.
Comment on lines
+5
to
+17
| internal fun getAttributeDescriptors(): Map<String, AttributeDescriptor> { | ||
| val analyzerName = setOf("dependaChartaImporter") | ||
| return mapOf( | ||
| "dependencies" to | ||
| AttributeDescriptor( | ||
| title = "Dependencies", | ||
| description = "Number of code-level dependency links between two files", | ||
| hintLowValue = "Few dependencies between the files", | ||
| hintHighValue = "Many dependencies between the files", | ||
| link = "", | ||
| direction = -1, | ||
| analyzers = analyzerName | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find DependaChartaImporter class and check its analyzer name
fd -a "DependaChartaImporter.kt" --type f | head -5Repository: MaibornWolff/codecharta
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Search for NAME constant and getAnalyserName in DependaChartaImporter
rg -n "class DependaChartaImporter|NAME\s*=|getAnalyserName|getAnalyzerName" analysis/analysers/importers/DependaChartaImporter -g '*.kt' -A 2Repository: MaibornWolff/codecharta
Length of output: 1894
🏁 Script executed:
#!/bin/bash
# Check what analyzer names are used in other AttributeDescriptors for comparison
rg -n "analyzers\s*=" analysis/analysers/importers -g '*AttributeDescriptors.kt' -B 2 -A 1Repository: MaibornWolff/codecharta
Length of output: 28612
Replace hard-coded "dependaChartaImporter" with the actual analyzer name from DependaChartaImporter.NAME.
The analyzer name in AttributeDescriptors ("dependaChartaImporter") does not match DependaChartaImporter.NAME which is "dependachartaimport". This mismatch prevents the attribute descriptor from being properly associated with the analyzer, breaking metadata and UI integration. Update line 6 to use the correct analyzer name: setOf(DependaChartaImporter.NAME) or change the hard-coded value to "dependachartaimport".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/AttributeDescriptors.kt`
around lines 5 - 17, The analyzer name in getAttributeDescriptors currently uses
a hard-coded setOf("dependaChartaImporter") which mismatches
DependaChartaImporter.NAME; update the analyzerName definition in
getAttributeDescriptors to use the canonical identifier by replacing the
hard-coded string with setOf(DependaChartaImporter.NAME) (or change the string
to "dependachartaimport") so the AttributeDescriptor for "dependencies" is
associated with DependaChartaImporter correctly.
Comment on lines
+9
to
+18
| fun parseEdges(dcProject: DcProject): List<Edge> { | ||
| return dcProject.leaves.values | ||
| .filter { it.physicalPath.isNotBlank() } | ||
| .flatMap { leaf -> resolveDependencyPaths(leaf, dcProject.leaves) } | ||
| .groupingBy { it } | ||
| .eachCount() | ||
| .map { (paths, count) -> | ||
| Edge(ROOT_PREFIX + paths.first, ROOT_PREFIX + paths.second, mapOf("dependencies" to count)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether DcDependency.weight should contribute to edge counts.
# 1) Inspect DcDependency model
echo "=== DcDependency Model Definition ==="
rg -n "data class DcDependency" -C 3 -g '*.kt' analysis/analysers/importers/DependaChartaImporter/src/main/kotlin
echo -e "\n=== Weight field usage in DcDependency ==="
rg -n "class DcDependency" -A 10 -g '*.kt' analysis/analysers/importers/DependaChartaImporter/src/main/kotlin | head -30
# 2) Check current usage of .weight in importer code
echo -e "\n=== Usage of .weight in importer code ==="
rg -n "\.weight\b" -C 3 -g '*.kt' analysis/analysers/importers/DependaChartaImporter/src/main/kotlin
# 3) Inspect fixtures for weight values > 1
echo -e "\n=== Test fixtures with weight values ==="
rg -n "\"weight\"" -C 2 -g '*.json' analysis/analysers/importers/DependaChartaImporter/src/test/resourcesRepository: MaibornWolff/codecharta
Length of output: 5635
🏁 Script executed:
#!/bin/bash
# Examine parseEdges and resolveDependencyPaths in detail
echo "=== Full parseEdges and resolveDependencyPaths methods ==="
sed -n '9,29p' analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/DcJsonParser.kt
echo -e "\n=== Check if there are any comments about weight in the codebase ==="
rg -n "weight" -C 3 -g '*.kt' analysis/analysers/importers/DependaChartaImporter/src/main/kotlin | grep -i "weight\|comment\|//"
echo -e "\n=== Check test cases for parseEdges behavior ==="
rg -n "parseEdges\|resolveDependencyPaths" -C 5 -g '*.kt' analysis/analysers/importers/DependaChartaImporter/src/test/kotlinRepository: MaibornWolff/codecharta
Length of output: 1434
Weight field is unused in edge aggregation.
resolveDependencyPaths iterates over leaf.dependencies.keys only, never accessing the DcDependency object containing the weight. This means DcDependency.weight is dropped entirely. If weight represents multiple references, edge counts will be undercounted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/DcJsonParser.kt`
around lines 9 - 18, parseEdges currently undercounts dependencies because
resolveDependencyPaths only iterates dependency keys and drops
DcDependency.weight; update resolveDependencyPaths (or its caller parseEdges) to
iterate the actual DcDependency objects from DcProject.leaves[...] .dependencies
(not just keys) and emit pairs annotated with the dependency weight, then change
the aggregation in parseEdges to sum weights (e.g., groupingBy the source/target
pair and reducing by summing dependency.weight) when creating Edge(...) so Edge
metadata uses the total summed weight rather than simple occurrence count.
Comment on lines
+33
to
+35
| @CommandLine.Option(names = ["-nc", "--not-compressed"], description = ["save uncompressed output File"]) | ||
| private var compress = true | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ProjectSerializer.serializeToFileOrStream signature and usage
rg -n "serializeToFileOrStream" -C 3 --type ktRepository: MaibornWolff/codecharta
Length of output: 93
🏁 Script executed:
# Look at the DependaChartaImporter file to see lines 33-35 and 67
cat -n analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/DependaChartaImporter.kt | head -80Repository: MaibornWolff/codecharta
Length of output: 3817
🏁 Script executed:
# Check other importers for how they handle compression flags
find analysis/analysers -name "*Importer.kt" -type f | head -10Repository: MaibornWolff/codecharta
Length of output: 1169
🏁 Script executed:
# Search for "-nc" or "not-compressed" flags across the codebase
rg -n "\-nc|not-compressed" --type kt analysis/analysersRepository: MaibornWolff/codecharta
Length of output: 93
🏁 Script executed:
rg -n "serializeToFileOrStream" -C 3Repository: MaibornWolff/codecharta
Length of output: 24330
🏁 Script executed:
# Check other importers for compression handling
rg -n "compress|not-compressed|-nc" analysis/analysers/importers --context 3Repository: MaibornWolff/codecharta
Length of output: 50382
🏁 Script executed:
# Verify picocli option annotation handling by checking TokeiImporter implementation
rg -A 5 "@CommandLine.Option.*not-compressed" analysis/analysers/importers/TokeiImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/tokei/TokeiImporter.ktRepository: MaibornWolff/codecharta
Length of output: 350
🏁 Script executed:
# Check if there's any custom logic inverting the compress flag in execute/call methods
rg -B 2 -A 10 "fun call\(\)" analysis/analysers/importers/TokeiImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/tokei/TokeiImporter.kt | head -30Repository: MaibornWolff/codecharta
Length of output: 543
-nc/--not-compressed flag has no effect on compression.
The option binds to compress which defaults to true. In picocli, a boolean flag sets the field to true when present. Since compress already defaults to true, passing -nc doesn't change the behavior—compression remains enabled.
Either invert the logic by using notCompressed and passing !notCompressed to the serializer, or use negatable = true in the annotation to make picocli handle the negation automatically.
This also applies to: 67-67, and affects TokeiImporter and SourceMonitorImporter (same pattern).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/DependaChartaImporter.kt`
around lines 33 - 35, The -nc/--not-compressed flag currently binds to a boolean
named compress (default true) so the flag has no effect; change the CLI field to
a negated name (e.g., private var notCompressed = false in
DependaChartaImporter) and pass the inverse when configuring serialization (use
!notCompressed where compress was used), and apply the same change to the
equivalent fields in TokeiImporter and SourceMonitorImporter (update their CLI
option annotations to use --not-compressed and their serializer calls to use the
negation).
Comment on lines
+21
to
+38
| val outputFileName: String = session.promptInput( | ||
| message = "What is the name of the output file?", | ||
| hint = "output.cc.json", | ||
| allowEmptyInput = true, | ||
| onInputReady = testCallback() | ||
| ) | ||
|
|
||
| val isCompressed = | ||
| (outputFileName.isEmpty()) || | ||
| session.promptConfirm( | ||
| message = "Do you want to compress the output file?", | ||
| onInputReady = testCallback() | ||
| ) | ||
|
|
||
| return listOf(inputFileName) + listOfNotNull( | ||
| "--output-file=$outputFileName", | ||
| if (isCompressed) null else "--not-compressed" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and verify the file and the code at lines 21-38
fd -t f "Dialog.kt" | grep -i dependacharta | head -5Repository: MaibornWolff/codecharta
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Check the content of the Dialog.kt file at the specified lines
cat -n analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/Dialog.kt | sed -n '15,45p'Repository: MaibornWolff/codecharta
Length of output: 1341
🏁 Script executed:
#!/bin/bash
# Search for how --output-file is handled in the codebase
rg -n --type=kt 'output-file|outputFile' analysis --max-count=20Repository: MaibornWolff/codecharta
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Search for --output-file handling without file type restriction
rg -n 'output-file' analysis --max-count=30Repository: MaibornWolff/codecharta
Length of output: 24671
🏁 Script executed:
#!/bin/bash
# Search for where command-line arguments are parsed
rg -n 'outputFile|output.file' analysis --max-count=30Repository: MaibornWolff/codecharta
Length of output: 50379
🏁 Script executed:
#!/bin/bash
# Look for cli argument parser or command builder
fd -t f -e kt | xargs rg -l 'picocli|argparse|CommandLine|--output' | head -10Repository: MaibornWolff/codecharta
Length of output: 1089
🏁 Script executed:
#!/bin/bash
# Look at the DependaChartaImporter class that receives the arguments
cat -n analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/DependaChartaImporter.kt | sed -n '25,75p'Repository: MaibornWolff/codecharta
Length of output: 2467
🏁 Script executed:
#!/bin/bash
# Search for tests that check empty output-file behavior
rg -n "output-file.*isEqualTo.*\"\"" analysis/analysers/importers/ --max-count=10Repository: MaibornWolff/codecharta
Length of output: 517
🏁 Script executed:
#!/bin/bash
# Look at how ProjectSerializer handles empty/null outputFilePath
cat -n analysis/model/src/main/kotlin/de/maibornwolff/codecharta/serialization/ProjectSerializer.kt | sed -n '64,85p'Repository: MaibornWolff/codecharta
Length of output: 1296
🏁 Script executed:
#!/antml:thinking
Let me also check OutputFileHandler to understand how empty output file paths are handled.Repository: MaibornWolff/codecharta
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Look at OutputFileHandler to see how it handles empty paths
cat -n analysis/model/src/main/kotlin/de/maibornwolff/codecharta/serialization/OutputFileHandler.ktRepository: MaibornWolff/codecharta
Length of output: 1612
🏁 Script executed:
#!/bin/bash
# Check if there are similar patterns in other Dialog files
rg -A5 'return listOf.*output-file' analysis/analysers/importers/ | head -80Repository: MaibornWolff/codecharta
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for how other importers/parsers handle the output-file flag in their Dialog classes
grep -r "return.*--output-file" analysis/analysers --include="*.kt" | head -20Repository: MaibornWolff/codecharta
Length of output: 1173
🏁 Script executed:
#!/bin/bash
# Check CoverageImporter Dialog to see the conditional pattern
cat -n analysis/analysers/importers/CoverageImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/coverage/Dialog.kt | sed -n '25,60p'Repository: MaibornWolff/codecharta
Length of output: 1801
🏁 Script executed:
#!/bin/bash
# Check if there's a test for DependaChartaImporter with empty output file
cat -n analysis/analysers/importers/DependaChartaImporter/src/test/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/DialogTest.kt | sed -n '30,60p'Repository: MaibornWolff/codecharta
Length of output: 1390
🏁 Script executed:
#!/bin/bash
# Let's verify the full test to see what arguments are collected when output is empty
cat -n analysis/analysers/importers/DependaChartaImporter/src/test/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/DialogTest.kt | sed -n '20,52p'Repository: MaibornWolff/codecharta
Length of output: 1567
🏁 Script executed:
#!/bin/bash
# Let's check what the collectAnalyserArgs function returns to understand the full argument list
cat -n analysis/analysers/importers/DependaChartaImporter/src/test/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/DialogTest.kt | head -120 | tail -40Repository: MaibornWolff/codecharta
Length of output: 1936
🏁 Script executed:
#!/bin/bash
# Let's see the full context of how Dialog.kt generates the arguments
cat -n analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/Dialog.kt | sed -n '1,42p'Repository: MaibornWolff/codecharta
Length of output: 2143
Omit the --output-file flag when output filename is empty.
Currently, an empty outputFileName causes --output-file= (with no value) to be passed to the command-line parser. While downstream checks (isNullOrBlank() in OutputFileHandler) prevent functional issues, the flag should not be emitted at all when there's no value. This improves clarity and aligns with the pattern used in CoverageImporter (Dialog.kt line 48).
🔧 Suggested fix
return listOf(inputFileName) + listOfNotNull(
- "--output-file=$outputFileName",
+ outputFileName.takeIf { it.isNotBlank() }?.let { "--output-file=$it" },
if (isCompressed) null else "--not-compressed"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val outputFileName: String = session.promptInput( | |
| message = "What is the name of the output file?", | |
| hint = "output.cc.json", | |
| allowEmptyInput = true, | |
| onInputReady = testCallback() | |
| ) | |
| val isCompressed = | |
| (outputFileName.isEmpty()) || | |
| session.promptConfirm( | |
| message = "Do you want to compress the output file?", | |
| onInputReady = testCallback() | |
| ) | |
| return listOf(inputFileName) + listOfNotNull( | |
| "--output-file=$outputFileName", | |
| if (isCompressed) null else "--not-compressed" | |
| ) | |
| val outputFileName: String = session.promptInput( | |
| message = "What is the name of the output file?", | |
| hint = "output.cc.json", | |
| allowEmptyInput = true, | |
| onInputReady = testCallback() | |
| ) | |
| val isCompressed = | |
| (outputFileName.isEmpty()) || | |
| session.promptConfirm( | |
| message = "Do you want to compress the output file?", | |
| onInputReady = testCallback() | |
| ) | |
| return listOf(inputFileName) + listOfNotNull( | |
| outputFileName.takeIf { it.isNotBlank() }?.let { "--output-file=$it" }, | |
| if (isCompressed) null else "--not-compressed" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@analysis/analysers/importers/DependaChartaImporter/src/main/kotlin/de/maibornwolff/codecharta/analysers/importers/dependacharta/Dialog.kt`
around lines 21 - 38, The returned argument list emits "--output-file=" even
when outputFileName is empty; update the return construction in Dialog.kt so the
"--output-file=…" flag is only included when outputFileName is non-empty (e.g.
conditionally include "--output-file=$outputFileName" using
outputFileName.isNotEmpty() in the listOfNotNull), keep the existing logic for
the compression flag (isCompressed) unchanged.
|
|
||
| **Category**: Importer (takes in a `.dc.json` file generated by DependaCharta and outputs cc.json) | ||
|
|
||
| The DependaChartaImporter generates visualisation data from DependaCharta `.dc.json` files. DependaCharta analyses code-level dependencies between classes, functions, and other code entities and records them with their physical file paths. The importer aggregates these entity-level dependencies into file-level edges so they can be visualised in CodeCharta. Note that the metrics generated here are edge metrics. |
There was a problem hiding this comment.
Use a consistent spelling variant for “visualization”.
LanguageTool flags mixed US/UK spellings. Standardize on one variant (example below uses US spelling).
✏️ Suggested fix
-The DependaChartaImporter generates visualisation data from DependaCharta `.dc.json` files. DependaCharta analyses code-level dependencies between classes, functions, and other code entities and records them with their physical file paths. The importer aggregates these entity-level dependencies into file-level edges so they can be visualised in CodeCharta. Note that the metrics generated here are edge metrics.
+The DependaChartaImporter generates visualization data from DependaCharta `.dc.json` files. DependaCharta analyzes code-level dependencies between classes, functions, and other code entities and records them with their physical file paths. The importer aggregates these entity-level dependencies into file-level edges so they can be visualized in CodeCharta. Note that the metrics generated here are edge metrics.
@@
-5. Visualise `merged.cc.json` with the [Visualization](https://github.com/MaibornWolff/codecharta/tree/main/visualization)
+5. Visualize `merged.cc.json` with the [Visualization](https://github.com/MaibornWolff/codecharta/tree/main/visualization)Also applies to: 55-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gh-pages/_docs/04-importer/07-dependacharta.md` at line 12, The doc uses
mixed US/UK spelling for "visualization"/"visualisation"; update the
documentation text for DependaChartaImporter to use a single variant (preferably
US "visualization") by replacing all instances of "visualisation" with
"visualization" in this file (including the line referencing
DependaChartaImporter and the occurrence around line 55) so the terminology is
consistent throughout.
Comment on lines
+29
to
+31
| ``` | ||
| Usage: ccsh dependachartaimport [-h] [-nc] [-o=<outputFile>] FILE | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the usage code block (MD040).
markdownlint flags the fence without a language specifier.
✏️ Suggested fix
-```
+```text
Usage: ccsh dependachartaimport [-h] [-nc] [-o=<outputFile>] FILE</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gh-pages/_docs/04-importer/07-dependacharta.md` around lines 29 - 31, The
fenced usage block in the document lacks a language tag (MD040); update the code
fence for the line "Usage: ccsh dependachartaimport [-h] [-nc] [-o=<outputFile>]
FILE" to include a language identifier (e.g., ```text) so the block becomes a
proper fenced code block with a language specifier.
Comment on lines
+86
to
+89
| const numBoxes = oldIndex.count / indicesPerNode | ||
| const newIndexArray = new Uint32Array(oldIndex.count + numBoxes * bottomFaceIndicesCount) | ||
| newIndexArray.set(new Uint32Array(oldIndex.array.buffer, oldIndex.array.byteOffset, oldIndex.count)) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the mapMesh.ts file around the mentioned lines
cat -n visualization/app/codeCharta/services/3DExports/3DPreview/MeshModels/mapMesh.ts | sed -n '70,105p'Repository: MaibornWolff/codecharta
Length of output: 1843
🏁 Script executed:
# Search for type definitions and usage of oldIndex
rg -n "oldIndex" visualization/app/codeCharta/services/3DExports/3DPreview/MeshModels/mapMesh.ts -B5 -A5Repository: MaibornWolff/codecharta
Length of output: 1232
🏁 Script executed:
# Check what types are used for index arrays in the codebase
rg -n "setIndex\(" -g'*.ts' visualization/app/codeCharta -C3Repository: MaibornWolff/codecharta
Length of output: 6467
🏁 Script executed:
# Look for Uint16Array and Uint32Array patterns
rg -n "new Uint16Array|new Uint32Array" -g'*.ts' visualization/app/codeCharta -C2 | head -50Repository: MaibornWolff/codecharta
Length of output: 5821
Unsafe buffer reinterpretation creates RangeError risk
Line 88 attempts to create a Uint32Array view over oldIndex.array.buffer. If oldIndex.array is a Uint16Array (which is used elsewhere in the codebase), the buffer is only 2 × count bytes, but the view expects 4 × count bytes, causing a RangeError.
🔧 Proposed fix
- newIndexArray.set(new Uint32Array(oldIndex.array.buffer, oldIndex.array.byteOffset, oldIndex.count))
+ newIndexArray.set(oldIndex.array as ArrayLike<number>)Copy element values directly instead of reinterpreting the buffer.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const numBoxes = oldIndex.count / indicesPerNode | |
| const newIndexArray = new Uint32Array(oldIndex.count + numBoxes * bottomFaceIndicesCount) | |
| newIndexArray.set(new Uint32Array(oldIndex.array.buffer, oldIndex.array.byteOffset, oldIndex.count)) | |
| const numBoxes = oldIndex.count / indicesPerNode | |
| const newIndexArray = new Uint32Array(oldIndex.count + numBoxes * bottomFaceIndicesCount) | |
| newIndexArray.set(oldIndex.array as ArrayLike<number>) | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@visualization/app/codeCharta/services/3DExports/3DPreview/MeshModels/mapMesh.ts`
around lines 86 - 89, The code in mapMesh.ts creates a Uint32Array view over
oldIndex.array.buffer (oldIndex, indicesPerNode) which is unsafe if
oldIndex.array is a Uint16Array and can throw a RangeError; instead, allocate
newIndexArray (Uint32Array) and copy values from oldIndex.array
element-by-element (or via Array.from(oldIndex.array)) into newIndexArray so you
don't reinterpret the underlying buffer; update the existing
newIndexArray.set(...) usage to set from the copied 32-bit values rather than
creating a Uint32Array view over the original buffer.
Comment on lines
+113
to
+131
| describe("CodeMapBuilding cached color vectors", () => { | ||
| // These tests use real ColorConverter (no mocking) to verify cached vectors | ||
| // match what the on-demand lightness computation would produce. | ||
| let originalColorToVector3: typeof ColorConverter.colorToVector3 | ||
|
|
||
| beforeEach(() => { | ||
| // Restore real colorToVector3 and clear the static cache so mock | ||
| // values from the other describe block don't leak in. | ||
| if (originalColorToVector3 === undefined) { | ||
| // Save the real implementation on first call (before any mock replaces it) | ||
| // Since the sibling describe mocks it in beforeEach, we grab it from prototype | ||
| originalColorToVector3 = ColorConverter.colorToVector3 | ||
| } | ||
| ;(ColorConverter as any).colorToVector3Map = new Map() | ||
| ColorConverter.colorToVector3 = (color: string) => { | ||
| const convertedColor = Number(`0x${color.slice(1)}`) | ||
| return new Vector3(((convertedColor >> 16) & 0xff) / 255, ((convertedColor >> 8) & 0xff) / 255, (convertedColor & 0xff) / 255) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Restore ColorConverter after tests to avoid cross-file leakage.
The new suite overrides the global ColorConverter.colorToVector3 and leaves it modified after the suite finishes. Because Jest doesn’t guarantee file execution order, this can break other test files that rely on the real implementation. Add an afterEach/afterAll to restore the original function (and optionally reset the cache) so the override stays local to this suite.
✅ Suggested fix
describe("CodeMapBuilding cached color vectors", () => {
// These tests use real ColorConverter (no mocking) to verify cached vectors
// match what the on-demand lightness computation would produce.
let originalColorToVector3: typeof ColorConverter.colorToVector3
beforeEach(() => {
// Restore real colorToVector3 and clear the static cache so mock
// values from the other describe block don't leak in.
if (originalColorToVector3 === undefined) {
// Save the real implementation on first call (before any mock replaces it)
// Since the sibling describe mocks it in beforeEach, we grab it from prototype
originalColorToVector3 = ColorConverter.colorToVector3
}
;(ColorConverter as any).colorToVector3Map = new Map()
ColorConverter.colorToVector3 = (color: string) => {
const convertedColor = Number(`0x${color.slice(1)}`)
return new Vector3(((convertedColor >> 16) & 0xff) / 255, ((convertedColor >> 8) & 0xff) / 255, (convertedColor & 0xff) / 255)
}
})
+
+ afterEach(() => {
+ if (originalColorToVector3) {
+ ColorConverter.colorToVector3 = originalColorToVector3
+ }
+ ;(ColorConverter as any).colorToVector3Map = new Map()
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe("CodeMapBuilding cached color vectors", () => { | |
| // These tests use real ColorConverter (no mocking) to verify cached vectors | |
| // match what the on-demand lightness computation would produce. | |
| let originalColorToVector3: typeof ColorConverter.colorToVector3 | |
| beforeEach(() => { | |
| // Restore real colorToVector3 and clear the static cache so mock | |
| // values from the other describe block don't leak in. | |
| if (originalColorToVector3 === undefined) { | |
| // Save the real implementation on first call (before any mock replaces it) | |
| // Since the sibling describe mocks it in beforeEach, we grab it from prototype | |
| originalColorToVector3 = ColorConverter.colorToVector3 | |
| } | |
| ;(ColorConverter as any).colorToVector3Map = new Map() | |
| ColorConverter.colorToVector3 = (color: string) => { | |
| const convertedColor = Number(`0x${color.slice(1)}`) | |
| return new Vector3(((convertedColor >> 16) & 0xff) / 255, ((convertedColor >> 8) & 0xff) / 255, (convertedColor & 0xff) / 255) | |
| } | |
| }) | |
| describe("CodeMapBuilding cached color vectors", () => { | |
| // These tests use real ColorConverter (no mocking) to verify cached vectors | |
| // match what the on-demand lightness computation would produce. | |
| let originalColorToVector3: typeof ColorConverter.colorToVector3 | |
| beforeEach(() => { | |
| // Restore real colorToVector3 and clear the static cache so mock | |
| // values from the other describe block don't leak in. | |
| if (originalColorToVector3 === undefined) { | |
| // Save the real implementation on first call (before any mock replaces it) | |
| // Since the sibling describe mocks it in beforeEach, we grab it from prototype | |
| originalColorToVector3 = ColorConverter.colorToVector3 | |
| } | |
| ;(ColorConverter as any).colorToVector3Map = new Map() | |
| ColorConverter.colorToVector3 = (color: string) => { | |
| const convertedColor = Number(`0x${color.slice(1)}`) | |
| return new Vector3(((convertedColor >> 16) & 0xff) / 255, ((convertedColor >> 8) & 0xff) / 255, (convertedColor & 0xff) / 255) | |
| } | |
| }) | |
| afterEach(() => { | |
| if (originalColorToVector3) { | |
| ColorConverter.colorToVector3 = originalColorToVector3 | |
| } | |
| ;(ColorConverter as any).colorToVector3Map = new Map() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapBuilding.spec.ts`
around lines 113 - 131, The test suite overrides ColorConverter.colorToVector3
and mutates (ColorConverter as any).colorToVector3Map but never restores them;
add an afterAll (or afterEach) that restores ColorConverter.colorToVector3 from
the saved originalColorToVector3 and resets (ColorConverter as
any).colorToVector3Map to a new Map() so the global implementation and cache are
returned to their original state after the suite finishes.
Comment on lines
19
to
22
| add(building: CodeMapBuilding) { | ||
| this._buildings.push(building) | ||
| this.buildingsByPath.set(building.node.path, building) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check usage patterns for CodeMapGeometricDescription
cd visualization/app/codeCharta
rg -n --type=ts -C3 "new CodeMapGeometricDescription" .Repository: MaibornWolff/codecharta
Length of output: 736
🏁 Script executed:
# Check for setScales usage
cd visualization/app/codeCharta
rg -n --type=ts -C3 "\.setScales\(" .Repository: MaibornWolff/codecharta
Length of output: 463
🏁 Script executed:
# First, find the file and examine its full contents
fd -n "codeMapGeometricDescription.ts" visualization/app/codeChartaRepository: MaibornWolff/codecharta
Length of output: 297
🏁 Script executed:
# Find the file without -n flag
fd "codeMapGeometricDescription.ts" visualization/app/codeChartaRepository: MaibornWolff/codecharta
Length of output: 147
🏁 Script executed:
# Read the full file to understand the implementation
cat -n visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.tsRepository: MaibornWolff/codecharta
Length of output: 3789
🏁 Script executed:
# Check the geometryGenerator.ts to see the order of operations
cat -n visualization/app/codeCharta/ui/codeMap/rendering/geometryGenerator.ts | head -80Repository: MaibornWolff/codecharta
Length of output: 3503
🏁 Script executed:
# Search for where add() is called in relation to setScales
rg -n "\.add\(" visualization/app/codeCharta/ui/codeMap/rendering/ -A2 -B2Repository: MaibornWolff/codecharta
Length of output: 562
🏁 Script executed:
# Check codeMapMesh.ts to understand the interaction
cat -n visualization/app/codeCharta/ui/codeMap/rendering/codeMapMesh.ts | head -120Repository: MaibornWolff/codecharta
Length of output: 5477
🏁 Script executed:
# Search for any dynamic addition of buildings after construction
rg -n "\.add\(" visualization/app/codeCharta --type=ts | grep -v "geometry\|buffer\|map\|array\|classList"Repository: MaibornWolff/codecharta
Length of output: 10201
🏁 Script executed:
# Check if rebuild or updates happen after initial mesh construction
rg -n "\.setScales\|\.add\|CodeMapGeometricDescription" visualization/app/codeCharta/ui/codeMap --type=ts -B2 -A2 | head -100Repository: MaibornWolff/codecharta
Length of output: 49
Ensure scaledBoxes stays in sync when buildings are added after scaling.
If add() is called after setScales(), the new building won't have an entry in scaledBoxes, causing intersect() to silently skip it during mouse intersection testing (the defensive check at line 56-58 prevents crashes but masks the correctness bug). Update the add() method to add the scaled box to the cache.
🔧 Proposed fix
add(building: CodeMapBuilding) {
this._buildings.push(building)
this.buildingsByPath.set(building.node.path, building)
+ this.boxTranslation.set(-this.scales.x * this.mapSize, 0, -this.scales.z * this.mapSize)
+ const box = building.boundingBox.clone()
+ box.min.multiply(this.scales)
+ box.max.multiply(this.scales)
+ box.translate(this.boxTranslation)
+ this.scaledBoxes.push(box)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| add(building: CodeMapBuilding) { | |
| this._buildings.push(building) | |
| this.buildingsByPath.set(building.node.path, building) | |
| } | |
| add(building: CodeMapBuilding) { | |
| this._buildings.push(building) | |
| this.buildingsByPath.set(building.node.path, building) | |
| this.boxTranslation.set(-this.scales.x * this.mapSize, 0, -this.scales.z * this.mapSize) | |
| const box = building.boundingBox.clone() | |
| box.min.multiply(this.scales) | |
| box.max.multiply(this.scales) | |
| box.translate(this.boxTranslation) | |
| this.scaledBoxes.push(box) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts`
around lines 19 - 22, The add() method currently pushes new CodeMapBuilding
instances into _buildings and buildingsByPath but doesn't update the scaledBoxes
cache, causing buildings added after setScales() to be missing from intersect()
tests; update add(building: CodeMapBuilding) to compute and insert the
building's scaled box into scaledBoxes (using the same scaling logic/transform
used by setScales() or the helper that computes scaled boxes) so that
scaledBoxes.set(building.node.path, computedScaledBox) is called whenever a
building is added, keeping scaledBoxes in sync with _buildings and preventing
misses in intersect().
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…tyled labels - Add dedicated Label Settings Panel in ribbon bar grouping all label-related settings (top label count, show names/values, color label toggles with swatches) - Add building hover tooltip that follows cursor and shows area, height, and color metric values - Restyle top labels with frosted-glass look (rounded corners, blur background, soft shadow), smooth fade-in, overlap avoidance with connector lines - Move color label checkboxes from Color Settings Panel to Label Settings Panel - Move label settings from Height Settings Panel to Label Settings Panel - Raise top label limit from 10 to 50; apply limit across all color types combined - Preserve manually set label count when switching files - Fix memory growing steadily over time when reloading/switching maps - Fix setting edge preview count to 0 being ignored - Remove updateShowLabels effect (superseded by updateVisibleTopLabels) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
visualization/app/codeCharta/ui/screenshotButton/screenshotButton.component.ts (1)
104-140:⚠️ Potential issue | 🟠 MajorLabels may not be restored if
html2canvasthrows.If
html2canvasfails,restoreLabelsAfterScreenshotis never called, leaving labels with white backgrounds and disabled filters. Wrap in try/finally to guarantee restoration.🛡️ Proposed fix using try/finally
const savedLabelStyles = this.prepareLabelsForScreenshot() - const tagsNamesToIgnore = new Set([ - "cc-logo", - "cc-tool-bar", - "cc-view-cube", - "cc-ribbon-bar", - "cc-file-extension-bar", - "cc-attribute-side-bar", - "cc-loading-file-progess-spinner" - ]) - - const idsToIgnore = new Set(["legend-panel-button"]) - - const bodyHeight = document.querySelector("body")?.offsetHeight - const ribbonBarHeight = (document.querySelector("cc-ribbon-bar") as HTMLElement)?.offsetHeight - const toolBarHeight = (document.querySelector("cc-tool-bar") as HTMLElement)?.offsetHeight - const fileExtensionBarHeight = (document.querySelector("cc-file-extension-bar") as HTMLElement)?.offsetHeight - const offsetMenuBar = ribbonBarHeight + toolBarHeight + fileExtensionBarHeight - - const canvas = await html2canvas(document.querySelector("body"), { - removeContainer: true, - backgroundColor: null, - scrollY: -offsetMenuBar, - height: bodyHeight - offsetMenuBar, - ignoreElements(element) { - return ( - tagsNamesToIgnore.has(element.tagName.toLowerCase()) || - idsToIgnore.has(element.id) || - (element as HTMLElement).style.zIndex === "10000" - ) - } - }) - - this.restoreLabelsAfterScreenshot(savedLabelStyles) + try { + const tagsNamesToIgnore = new Set([ + "cc-logo", + "cc-tool-bar", + "cc-view-cube", + "cc-ribbon-bar", + "cc-file-extension-bar", + "cc-attribute-side-bar", + "cc-loading-file-progess-spinner" + ]) + + const idsToIgnore = new Set(["legend-panel-button"]) + + const bodyHeight = document.querySelector("body")?.offsetHeight + const ribbonBarHeight = (document.querySelector("cc-ribbon-bar") as HTMLElement)?.offsetHeight + const toolBarHeight = (document.querySelector("cc-tool-bar") as HTMLElement)?.offsetHeight + const fileExtensionBarHeight = (document.querySelector("cc-file-extension-bar") as HTMLElement)?.offsetHeight + const offsetMenuBar = ribbonBarHeight + toolBarHeight + fileExtensionBarHeight + + const canvas = await html2canvas(document.querySelector("body"), { + removeContainer: true, + backgroundColor: null, + scrollY: -offsetMenuBar, + height: bodyHeight - offsetMenuBar, + ignoreElements(element) { + return ( + tagsNamesToIgnore.has(element.tagName.toLowerCase()) || + idsToIgnore.has(element.id) || + (element as HTMLElement).style.zIndex === "10000" + ) + } + }) + + return this.getCroppedCanvas(canvas) + } finally { + this.restoreLabelsAfterScreenshot(savedLabelStyles) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/screenshotButton/screenshotButton.component.ts` around lines 104 - 140, The saved label styles from prepareLabelsForScreenshot must be restored even if html2canvas throws; wrap the html2canvas call and subsequent getCroppedCanvas return in a try/finally so restoreLabelsAfterScreenshot(savedLabelStyles) runs in finally, and rethrow or let errors propagate after restoration; keep savedLabelStyles in scope and call getCroppedCanvas(canvas) inside the try (so finally still executes) and ensure any thrown error bubbles up after restoreLabelsAfterScreenshot is executed.
♻️ Duplicate comments (1)
visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts (1)
55-58: Buildings added aftersetScales()won't be intersectable.The
add()method only pushes to_buildingsandbuildingsByPathbut doesn't updatescaledBoxesor rebuild the BVH. If buildings are added aftersetScales()is called, they won't have entries inscaledBoxesand won't be part of the BVH, making them invisible tointersect().🔧 Proposed fix
add(building: CodeMapBuilding) { this._buildings.push(building) this.buildingsByPath.set(building.node.path, building) + // If scales have been set, add the scaled box for this building + if (this.scaledBoxes.length > 0) { + const box = building.boundingBox.clone() + box.min.multiply(this.scales) + box.max.multiply(this.scales) + box.translate(this.boxTranslation) + this.scaledBoxes.push(box) + // BVH needs rebuild; could defer to next setScales() or rebuild here + this.buildBVH() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts` around lines 55 - 58, The add(building: CodeMapBuilding) currently only updates _buildings and buildingsByPath, so buildings added after setScales() aren't in scaledBoxes or the BVH and thus don't intersect; update add() to compute and push the building's scaled box into scaledBoxes (using the same scale transformation logic as setScales()/buildScaledBoxes) and then rebuild or incrementally update the bvh (the same routine used after setScales()) so the new building is included for intersect() queries; ensure references to scaledBoxes and bvh are maintained in sync with _buildings/buildingsByPath.
🧹 Nitpick comments (18)
visualization/app/material/matLabelSettingsMenu.scss (1)
1-4: Consider responsive width to avoid overflow on small viewports.A fixed
min-width: 420pxcan clip or overflow on narrow screens. Aclamp()ormin()-based width keeps the menu usable across viewports.♻️ Suggested tweak
.mat-mdc-menu-panel.cc-label-settings-menu { - min-width: 420px; - max-width: 500px; + width: clamp(320px, 60vw, 500px); + max-width: 90vw; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/material/matLabelSettingsMenu.scss` around lines 1 - 4, The .mat-mdc-menu-panel.cc-label-settings-menu rule uses a fixed min-width: 420px which can cause overflow on small viewports; update the rule to use a responsive width (e.g., clamp() or min()/max() formula) so the menu scales between a sensible minimum and maximum (keep the existing max-width: 500px) and prevents clipping on narrow screens while preserving desktop sizing.visualization/app/codeCharta/ui/fileExtensionBar/highlightBuildingsByFileExtension.service.spec.ts (1)
84-86: Pre-existing issue:afterEachcleanup is ineffective.The
jest.fn().mockClear()creates a new anonymous mock and clears it, which has no effect on the actual mocks defined in the test setup. This is a pre-existing issue unrelated to this PR's changes.If you intend to reset mock call history between tests, consider clearing the specific mocks:
♻️ Suggested fix
afterEach(() => { - jest.fn().mockClear() + jest.clearAllMocks() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/fileExtensionBar/highlightBuildingsByFileExtension.service.spec.ts` around lines 84 - 86, The afterEach currently calls jest.fn().mockClear(), which clears a newly created anonymous mock and does nothing to your test mocks; replace that line so you actually reset mocks between tests—either call jest.clearAllMocks() (or jest.resetAllMocks()) in the afterEach, or call mockClear()/mockReset() on the specific mock variables used in this spec (e.g., the spies/mocks you created for HighlightBuildingsByFileExtensionService or any mocked functions); update the afterEach block to use those real mock-clearing calls instead of jest.fn().mockClear().CLAUDE.md (1)
392-393: Clarify the security-sensitivedangerouslyDisableSandboxrequirement and "Bash tool" reference.The documentation mentions requiring
dangerouslyDisableSandbox: truebut lacks crucial context:
- Security concern: Disabling sandboxes has security implications. The documentation should explain WHY this is necessary for visualization tests.
- Unclear reference: "the Bash tool" is ambiguous. Specify exactly what this refers to (e.g., Claude Code's bash execution tool, a Jest configuration, a system setting, etc.) and where/how to configure it.
- Missing scope: Does this requirement apply to all test commands (
test:auto,test:updateSnaps,e2e, etc.) or onlynpm run test?📝 Suggested documentation improvements
Consider expanding the note to something like:
-**IMPORTANT**: Always run visualization tests using `npm run test` (not `npx jest` directly). Tests require `dangerouslyDisableSandbox: true` in the Bash tool to run correctly. +**IMPORTANT**: Always run visualization tests using `npm run test` (not `npx jest` directly). + +**For Claude Code users**: When executing test commands, the Bash tool requires `dangerouslyDisableSandbox: true` to run correctly because [explain specific reason - e.g., tests need to access system resources, spawn processes, etc.]. This setting should be configured in [specify where - e.g., Claude Code's tool settings, .claude/config.json, etc.].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 392 - 393, Update the CLAUDE.md note about running visualization tests to explicitly explain the security impact and scope of setting dangerouslyDisableSandbox: true: state why the sandbox must be disabled for visualization tests (e.g., tests need unrestricted shell access to run post-processing/graphics tools), clarify that "Bash tool" refers to the project's CLI/bash execution tool used by Claude Code (or the specific test runner's shell executor) and call out where to set dangerouslyDisableSandbox (CI/job runner or the Claude Code tool configuration) and whether this applies only to npm run test or to other scripts like test:auto, test:updateSnaps, and e2e; keep the change concise and add a short secure-practices note advising to only disable sandbox in trusted CI or local dev environments and not in untrusted contexts.visualization/app/codeCharta/ui/ribbonBar/heightSettingsPanel/heightSettingsPanel.component.spec.ts (1)
18-24: Consider using more idiomatic Testing Library assertions.The assertion
expect(screen.getByTitle("Height")).not.toBe(null)is slightly redundant sincegetByTitlethrows if the element isn't found. A simpler approach would be to usetoBeInTheDocument()matcher or just callgetByTitlewithout an assertion (since its successful execution confirms the element exists).♻️ Suggested simplification
it("should display height slider", async () => { // Arrange / Act await render(HeightSettingsPanelComponent) // Assert - expect(screen.getByTitle("Height")).not.toBe(null) + expect(screen.getByTitle("Height")).toBeInTheDocument() })Note: This requires
@testing-library/jest-domto be configured, which providestoBeInTheDocument().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/ribbonBar/heightSettingsPanel/heightSettingsPanel.component.spec.ts` around lines 18 - 24, The test in the HeightSettingsPanelComponent spec uses expect(screen.getByTitle("Height")).not.toBe(null); replace this with a more idiomatic Testing Library assertion: either assert the element is present with expect(screen.getByTitle("Height")).toBeInTheDocument() (ensure `@testing-library/jest-dom` is configured) or simply call screen.getByTitle("Height") without an assertion since getByTitle will throw if not found; update the test accordingly to use toBeInTheDocument or the lone getByTitle call and remove the redundant not.toBe(null) check.visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.spec.ts (3)
194-205: Optional: assertclearHighlightis invoked.
This keeps the test focused on the intended side‑effect, not just rendering.💡 Suggested test tweak
- const renderSpy = jest.spyOn(threeSceneService["threeRendererService"], "render").mockImplementation(() => {}) + const renderSpy = jest.spyOn(threeSceneService["threeRendererService"], "render").mockImplementation(() => {}) + const clearSpy = jest.spyOn(threeSceneService as any, "clearHighlight") ... threeSceneService.applyClearHighlights() ... expect(renderSpy).toHaveBeenCalled() + expect(clearSpy).toHaveBeenCalled()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.spec.ts` around lines 194 - 205, The test currently only asserts rendering; add an assertion that the highlight clearing is invoked by spying on the highlight service's clearHighlight method before calling threeSceneService.applyClearHighlights(), e.g. create a spy on threeSceneService["highlightService"].clearHighlight (mockImplementation to noop), call applyClearHighlights(), and then expect the clearHighlight spy toHaveBeenCalled() in addition to the existing render assertion so the test verifies the intended side‑effect.
73-80: Consider asserting highlighted node IDs too.
This test now initializeshighlightedNodeIds; adding a minimal assertion would keep coverage aligned with the new state model.💡 Suggested assertion
expect(threeSceneService["highlightedBuildingIds"].has(CODE_MAP_BUILDING.id)).toBe(true) expect(threeSceneService["primaryHighlightedBuilding"]).toBe(CODE_MAP_BUILDING) + expect(threeSceneService["highlightedNodeIds"].has(CODE_MAP_BUILDING.node.id)).toBe(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.spec.ts` around lines 73 - 80, Add an assertion that confirms the corresponding node id was added to the highlightedNodeIds set after calling threeSceneService.addBuildingsToHighlightingList(CODE_MAP_BUILDING); specifically, verify threeSceneService["highlightedNodeIds"].has(CODE_MAP_BUILDING.id) (or the correct node id derived from CODE_MAP_BUILDING) alongside the existing checks for highlightedBuildingIds and primaryHighlightedBuilding to keep coverage aligned with the new state model.
189-190: Optional: verify node highlights are cleared too.
clearHighlightnow handles both building and node IDs; asserting both would lock in the new behavior.💡 Suggested assertion
expect(threeSceneService["highlightedBuildingIds"].size).toBe(0) expect(threeSceneService["primaryHighlightedBuilding"]).toBeNull() + expect(threeSceneService["highlightedNodeIds"].size).toBe(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.spec.ts` around lines 189 - 190, The test currently asserts that threeSceneService["highlightedBuildingIds"] is empty and threeSceneService["primaryHighlightedBuilding"] is null, but clearHighlight now also clears node highlights; update the spec to assert that threeSceneService["highlightedNodeIds"].size is 0 and threeSceneService["primaryHighlightedNode"] is null after calling clearHighlight. Locate the assertions near the existing checks for highlightedBuildingIds and primaryHighlightedBuilding in the threeSceneService.spec.ts and add the two analogous assertions referencing highlightedNodeIds and primaryHighlightedNode to lock in the new behavior.visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.spec.ts (1)
6-20: Avoidas unknown as Node; use a complete Node stub with defaults.The double-cast bypasses type safety and can hide breakage if
Nodegains new required fields or production code starts using currently missing ones. Consider a typed stub (or a shared factory) with sensible defaults.♻️ Proposed refactor
- const node = { - path: `/root/file${id}`, - name: `file${id}`, - id, - width: maxX - minX, - height, - length: maxZ - minZ, - x0: minX, - y0: minZ, - z0: 0, - isLeaf: true - } as unknown as Node + const node: Node = { + path: `/root/file${id}`, + name: `file${id}`, + id, + width: maxX - minX, + height, + length: maxZ - minZ, + depth: 0, + mapNodeDepth: 0, + x0: minX, + y0: minZ, + z0: 0, + isLeaf: true, + attributes: {}, + edgeAttributes: {}, + heightDelta: 0, + visible: true, + link: "", + markingColor: undefined, + flat: false, + color: "#ff0000", + incomingEdgePoint: new Vector3(), + outgoingEdgePoint: new Vector3() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.spec.ts` around lines 6 - 20, The test helper makeBuilding currently uses an unsafe double-cast "as unknown as Node"; replace that with a real typed Node stub: create a Node-typed object (e.g., nodeStub) that fills all required Node properties with sensible defaults (path, name, id, width, height, length, x0, y0, z0, isLeaf, and any other non-optional fields on the Node interface) and pass that to new CodeMapBuilding instead of the double-cast; update makeBuilding to construct and return new CodeMapBuilding(id, box, nodeStub, "#ff0000") so type-safety is preserved and future interface changes will fail fast.visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts (1)
225-226: Consider preallocating the BVH traversal stack.The stack array grows dynamically during traversal. For large scenes, preallocating based on expected tree depth (e.g.,
Math.ceil(Math.log2(count)) * 2) could reduce allocations during mouse-move handlers.-const stack: number[] = [0] -let stackPointer = 1 +const maxDepth = Math.ceil(Math.log2(this._buildings.length + 1)) * 2 +const stack = new Array<number>(Math.max(maxDepth, 16)) +stack[0] = 0 +let stackPointer = 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts` around lines 225 - 226, The traversal stack (stack and stackPointer in the BVH traversal code) is currently allocated dynamically; preallocate it to avoid runtime growth by creating an array sized from the BVH node count (e.g., const capacity = Math.ceil(Math.log2(count)) * 2 or similar), fill it with a default value, and initialize stackPointer to 1; replace the current "const stack: number[] = [0]; let stackPointer = 1" with a preallocated array and keep the same stack usage semantics so traversal logic (push/pop via stackPointer) remains unchanged.visualization/app/codeCharta/ui/codeMap/codeMap.tooltip.service.spec.ts (1)
101-157: Consider restoring window dimensions after edge clamping tests.The tests modify
window.innerWidthandwindow.innerHeightusingObject.definePropertybut don't restore original values. This could affect subsequent tests if they depend on actual window dimensions.🧹 Suggested improvement to restore window dimensions
describe("positionTooltip edge clamping", () => { + let originalInnerWidth: number + let originalInnerHeight: number + + beforeEach(() => { + originalInnerWidth = window.innerWidth + originalInnerHeight = window.innerHeight + }) + + afterEach(() => { + Object.defineProperty(window, "innerWidth", { configurable: true, writable: true, value: originalInnerWidth }) + Object.defineProperty(window, "innerHeight", { configurable: true, writable: true, value: originalInnerHeight }) + }) + it("should clamp tooltip to left edge when flipped position would go off-screen left", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/codeMap.tooltip.service.spec.ts` around lines 101 - 157, The tests set window.innerWidth/innerHeight directly but never restore them, which can leak into other tests; update the "positionTooltip edge clamping" specs to capture the original values before calling Object.defineProperty and restore them after each test (e.g., in a try/finally inside each it or in an afterEach) so tests that call tooltipService.show and tooltipService.updatePosition run against a clean viewport; reference the use of window.innerWidth/window.innerHeight, tooltipService.show, and tooltipService.updatePosition when locating where to save and restore the originals.visualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettingsPanel.component.html (1)
25-39: Consider adding visible text labels for color label checkboxes for better accessibility.The color label checkboxes only display color swatches without text. While
aria-labelattributes are present, users with color vision deficiencies may have difficulty distinguishing between the options. Consider adding visible text labels like "Positive", "Neutral", "Negative" alongside the swatches.💡 Suggested improvement
<mat-checkbox [checked]="colorLabels.positive" (change)="setColorLabel($event, 'positive')" aria-label="positive color label" - ><span class="color-label-swatch" [style.background-color]="mapColors.positive"></span></mat-checkbox> + ><span class="color-label-swatch" [style.background-color]="mapColors.positive"></span> Positive</mat-checkbox> <mat-checkbox [checked]="colorLabels.neutral" (change)="setColorLabel($event, 'neutral')" aria-label="neutral color label" - ><span class="color-label-swatch" [style.background-color]="mapColors.neutral"></span></mat-checkbox> + ><span class="color-label-swatch" [style.background-color]="mapColors.neutral"></span> Neutral</mat-checkbox> <mat-checkbox [checked]="colorLabels.negative" (change)="setColorLabel($event, 'negative')" aria-label="negative color label" - ><span class="color-label-swatch" [style.background-color]="mapColors.negative"></span></mat-checkbox> + ><span class="color-label-swatch" [style.background-color]="mapColors.negative"></span> Negative</mat-checkbox>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettingsPanel.component.html` around lines 25 - 39, The color-only checkboxes in labelSettingsPanel (the mat-checkbox elements bound to colorLabels.positive/neutral/negative and mapColors) lack visible text labels; update each mat-checkbox (the ones using setColorLabel($event, 'positive'/'neutral'/'negative')) to include readable text like "Positive", "Neutral", "Negative" next to the color swatch so sighted users and those with color-vision deficiencies can identify them while keeping existing aria-labels and bindings intact.visualization/app/codeCharta/ui/codeMap/threeViewer/threeRenderer.service.ts (1)
55-56: Subscription should be cleaned up on service destruction.The subscription to
isWhiteBackground$()is created but never unsubscribed. While Angular root services typically live for the application lifetime, explicit cleanup is a best practice:private subscriptions = new Subscription() init(...) { // ... this.subscriptions.add( this.globalSettingsFacade.isWhiteBackground$().subscribe( isWhiteBackground => this.setBackgroundColorToState(isWhiteBackground) ) ) } ngOnDestroy() { this.subscriptions.unsubscribe() this._afterRender$.complete() }This prevents potential memory leaks if the service is ever provided at a non-root level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/threeViewer/threeRenderer.service.ts` around lines 55 - 56, The subscription to globalSettingsFacade.isWhiteBackground$() in the constructor/init is never cleaned up; add a Subscription container (e.g., a private subscriptions = new Subscription()) and add the isWhiteBackground$() subscription to it instead of leaving it unmanaged, implement ngOnDestroy() on the service to call this.subscriptions.unsubscribe() and also complete _afterRender$() there, and keep using setBackgroundColorToState(...) for the callback so the logic location (isWhiteBackground$(), setBackgroundColorToState, _afterRender$) is unchanged.visualization/app/codeCharta/ui/codeMap/rendering/codeMapMesh.ts (2)
468-474: Verify index buffer bounds in fillIndexBuffer.The loop iterates
indicesPerNodetimes (30) and accessestemplateIndices[j]. This should be correct sincetemplateIndiceshas exactly 30 elements. However, ifindicesPerNodeis ever changed independently oftemplateIndices.length, this could cause out-of-bounds access.Consider adding a debug assertion or using
templateIndices.lengthdirectly:private fillIndexBuffer(i: number, indices: Uint32Array) { const vertexOffset = i * verticesPerBox const indexOffset = i * indicesPerNode - for (let j = 0; j < indicesPerNode; j++) { + for (let j = 0; j < templateIndices.length; j++) { indices[indexOffset + j] = templateIndices[j] + vertexOffset } }This ensures the loop is always bounded by the actual template data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapMesh.ts` around lines 468 - 474, The fillIndexBuffer method can read past templateIndices if indicesPerNode and templateIndices.length diverge; update fillIndexBuffer to bound the loop by the actual template size (e.g., use const count = Math.min(indicesPerNode, templateIndices.length) and loop j from 0 to count) or add a debug assertion that indicesPerNode === templateIndices.length before the loop, referencing fillIndexBuffer, templateIndices and indicesPerNode so the index buffer write cannot go out of bounds.
493-495: Clarify the purpose of addGroup with POSITIVE_INFINITY.
geometry.addGroup(0, Number.POSITIVE_INFINITY, 0)creates a draw group covering all indices with material index 0. This is unusual but ensures the entire geometry uses the first (and only) material.Consider adding a brief comment explaining this is required for the multi-material constructor pattern used on line 498:
+ // Single draw group covering all indices, required for material array compatibility geometry.addGroup(0, Number.POSITIVE_INFINITY, 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapMesh.ts` around lines 493 - 495, Add a brief inline comment at the call to geometry.addGroup(0, Number.POSITIVE_INFINITY, 0) explaining that we intentionally set the group length to POSITIVE_INFINITY so the single material (material index 0) covers the entire geometry to satisfy the multi-material constructor pattern used when creating the mesh (refer to geometry.addGroup and the multi-material constructor usage in this file); this clarifies why a seemingly unusual POSITIVE_INFINITY value is used and prevents future accidental removal.visualization/app/codeCharta/ui/codeMap/codeMap.tooltip.service.ts (1)
37-47: Add null assertion or guard after lazy creation.After calling
createTooltipElement(), TypeScript may not narrowthis.tooltipElementto non-null within the same scope. While the code works at runtime, adding an assertion improves type safety:show(node: Node, clientX: number, clientY: number) { if (!this.tooltipElement) { this.createTooltipElement() } + const element = this.tooltipElement! - this.populateTooltip(node) - this.positionTooltip(clientX, clientY) - this.tooltipElement.style.opacity = "1" + this.populateTooltip(node) + this.positionTooltip(clientX, clientY) + element.style.opacity = "1"Alternatively, have
createTooltipElementreturn the element.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/codeMap.tooltip.service.ts` around lines 37 - 47, The show method uses this.tooltipElement after lazily calling createTooltipElement(), but TypeScript may not narrow tooltipElement to non-null; update show (or createTooltipElement) so tooltipElement is guaranteed non-null: either add a post-creation null assertion when using this.tooltipElement in show (e.g., assert/! when calling populateTooltip/positionTooltip and setting style) or change createTooltipElement to return the newly created element and assign it to this.tooltipElement (and then use that local/this reference), ensuring populateTooltip, positionTooltip and the style assignment operate on a non-null HTMLElement; reference the show, createTooltipElement, tooltipElement, populateTooltip and positionTooltip symbols.visualization/app/codeCharta/ui/codeMap/rendering/geometryGenerationHelper.ts (1)
273-348: Consider using a more compact format for templateNormals.The current one-value-per-line format significantly increases file length. Since
prettier-ignoreis already applied, consider a more compact inline format similar totemplatePositions:// prettier-ignore const templateNormals = new Float32Array([ // Left face (vertices 0-3) -1, 0, 0, -1, 0, 0, -1, 0, 0, -1, 0, 0, // Right face (vertices 4-7) 1, 0, 0, 1, 0, 0, 1, 0, 0, 1, 0, 0, // ... etc ])This would improve readability and reduce file size.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/rendering/geometryGenerationHelper.ts` around lines 273 - 348, The templateNormals array is written one value per line which bloats the file; refactor the Float32Array literal for templateNormals into a compact inline format (like templatePositions) while keeping the existing // prettier-ignore, preserving the exact numeric order and count; optionally group values with short inline comments per face (e.g., "// left", "// right") to retain readability, and update the surrounding code that references templateNormals if any formatting assumptions exist.visualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettingsPanel.component.ts (1)
50-52: Consider memory management for debounced function.The
debounceutility typically returns a function with a pending timer. If this component is destroyed and recreated, the old debounced function's timer could still fire. Ensure thedebounceutility used here either:
- Provides a
cancel()method that's called inngOnDestroy, or- The timer is scoped in a way that prevents stale dispatches
If
debounceis a simple closure without cleanup, consider using RxJSdebounceTimewith aSubjectinstead for automatic cleanup via Angular's async subscription handling.♻️ Alternative using RxJS (if cleanup is needed)
private topLabelsSubject = new Subject<number>() constructor(private store: Store<CcState>) { this.topLabelsSubject.pipe( debounceTime(LabelSettingsPanelComponent.DEBOUNCE_TIME) ).subscribe(value => { this.store.dispatch(setAmountOfTopLabels({ value })) }) } applyDebouncedTopLabels(amountOfTopLabels: number) { this.topLabelsSubject.next(amountOfTopLabels) } ngOnDestroy() { this.topLabelsSubject.complete() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettingsPanel.component.ts` around lines 50 - 52, The debounced function applyDebouncedTopLabels created with debounce(LabelSettingsPanelComponent.DEBOUNCE_TIME) may leave a pending timer across component teardown; either call the debounced function's cancel/clear method in ngOnDestroy (if the debounce implementation exposes cancel/clear) to prevent stale dispatches from firing, or replace the debounced closure with an RxJS Subject (e.g., topLabelsSubject) piped through debounceTime(LabelSettingsPanelComponent.DEBOUNCE_TIME) and subscribe to dispatch setAmountOfTopLabels, then complete/unsubscribe the subject in ngOnDestroy to ensure proper cleanup.visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts (1)
229-333: PreferreplaceChildren()overinnerHTML = ""for clearing SVGIt avoids HTML parsing and quiets XSS scanners while achieving the same result.
♻️ Suggested tweak
- svg.innerHTML = "" + svg.replaceChildren() ... - if (this.connectorSvg) { - this.connectorSvg.innerHTML = "" - } + if (this.connectorSvg) { + this.connectorSvg.replaceChildren() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts` around lines 229 - 333, Replace DOM clearing that uses innerHTML = "" with the safer, faster replaceChildren() to avoid HTML parsing/XSS concerns: in drawConnectors() change svg.innerHTML = "" to svg.replaceChildren(), and in clearConnectors() change this.connectorSvg.innerHTML = "" to this.connectorSvg.replaceChildren(); these changes operate on the SVG element created in getOrCreateConnectorSvg() and preserve existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts`:
- Around line 31-35: The service keeps a stale reference in suppressedLabel when
labels are removed; update the code paths that clear or remove labels (the logic
operating on the labels: InternalLabel[] and any methods that remove all labels
or individual labels around the block referenced at lines 83-99) to also set
suppressedLabel = null when the removed label equals suppressedLabel or when
clearing the labels array, ensuring suppressedLabel is reset whenever labels are
cleared or the specific suppressed label is deleted (refer to the
suppressedLabel field and the labels array to locate the correct places to
change).
- Around line 137-227: The resolve is to prevent suppressed labels from
influencing layout: in CodeMapLabelService.resolveCollisions, exclude any info
whose label === this.suppressedLabel from collision detection calculations (both
tooltip collision check and the loop that checks collisions with previous
labels) — you can either filter infos into a separate array of activeInfos
before sorting and sweeping, or skip entries where info.label ===
this.suppressedLabel inside the loops; still keep suppressed labels in the final
apply-offsets pass so they remain hidden (opacity 0) but do not contribute
offsets for others.
In `@visualization/app/codeCharta/ui/codeMap/codeMap.mouseEvent.service.ts`:
- Around line 101-118: The DOM listeners registered in start() via
registerEventListeners() are reattached on multiple starts causing duplicate
handlers; modify the service so registerEventListeners() either records the
actual listener functions (for example the bound handlers like
onDocumentMouseMove, onDocumentMouseUp, onDocumentMouseDown,
onDocumentDoubleClick, onDocumentMouseLeave, onDocumentMouseEnter and the
debounced wheel handler) and removes them in ngOnDestroy(), or add an
"eventsRegistered" boolean guard checked/ set in
start()/registerEventListeners() to ensure registration happens only once;
ensure ngOnDestroy() removes the listeners if you choose the record-and-remove
approach so no duplicate handlers remain.
In `@visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.ts`:
- Around line 302-309: The code currently calls applyHighlights() while
this.selected is still set and then calls resetMaterial() which unconditionally
resets all labels to the highlighted color, wiping dimming; change the order or
logic so highlighted state persists: either call
resetMaterial(getMapMaterials()) first, then set this.selected = null and
finally call applyHighlights(), or alter resetMaterial() to skip/restyle nodes
whose ids are contained in this.highlightedBuildingIds (use getMapMaterials(),
resetMaterial(), applyHighlights(), this.selected, this.highlightedBuildingIds
to locate the code) so resetMaterial does not overwrite the dimming for
non-highlighted nodes.
- Around line 134-141: getMapMaterials currently returns null for meshes with a
single Material because it only returns when child.material is an array; update
getMapMaterials (in threeSceneService, referencing mapGeometry.children and the
child.material extraction) to detect a single Material and wrap it in an array
before returning, i.e., if mat is not an array but is a Material then return
[mat] so highlighting/selection color updates iterate over a consistent
Material[].
In
`@visualization/app/codeCharta/ui/ribbonBar/colorSettingsPanel/colorSettingsPanel.component.html`:
- Around line 1-10: The template uses async-derived variables (isDeltaState$,
colorMetric$, sliderValues$, sliderColors$) but access sliderValues.min/max and
sliderColors before ensuring those objects are set, which can throw if
observables haven't emitted; update the `@if` condition to also check sliderValues
and sliderColors are truthy (e.g., `@if` (!isDeltaState && colorMetric !== "unary"
&& sliderValues && sliderColors)) so cc-metric-color-range-slider only receives
sliderValues.min/max when sliderValues is not null, and similarly guard any
other bindings that rely on sliderColors.
---
Outside diff comments:
In
`@visualization/app/codeCharta/ui/screenshotButton/screenshotButton.component.ts`:
- Around line 104-140: The saved label styles from prepareLabelsForScreenshot
must be restored even if html2canvas throws; wrap the html2canvas call and
subsequent getCroppedCanvas return in a try/finally so
restoreLabelsAfterScreenshot(savedLabelStyles) runs in finally, and rethrow or
let errors propagate after restoration; keep savedLabelStyles in scope and call
getCroppedCanvas(canvas) inside the try (so finally still executes) and ensure
any thrown error bubbles up after restoreLabelsAfterScreenshot is executed.
---
Duplicate comments:
In
`@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts`:
- Around line 55-58: The add(building: CodeMapBuilding) currently only updates
_buildings and buildingsByPath, so buildings added after setScales() aren't in
scaledBoxes or the BVH and thus don't intersect; update add() to compute and
push the building's scaled box into scaledBoxes (using the same scale
transformation logic as setScales()/buildScaledBoxes) and then rebuild or
incrementally update the bvh (the same routine used after setScales()) so the
new building is included for intersect() queries; ensure references to
scaledBoxes and bvh are maintained in sync with _buildings/buildingsByPath.
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 392-393: Update the CLAUDE.md note about running visualization
tests to explicitly explain the security impact and scope of setting
dangerouslyDisableSandbox: true: state why the sandbox must be disabled for
visualization tests (e.g., tests need unrestricted shell access to run
post-processing/graphics tools), clarify that "Bash tool" refers to the
project's CLI/bash execution tool used by Claude Code (or the specific test
runner's shell executor) and call out where to set dangerouslyDisableSandbox
(CI/job runner or the Claude Code tool configuration) and whether this applies
only to npm run test or to other scripts like test:auto, test:updateSnaps, and
e2e; keep the change concise and add a short secure-practices note advising to
only disable sandbox in trusted CI or local dev environments and not in
untrusted contexts.
In `@visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts`:
- Around line 229-333: Replace DOM clearing that uses innerHTML = "" with the
safer, faster replaceChildren() to avoid HTML parsing/XSS concerns: in
drawConnectors() change svg.innerHTML = "" to svg.replaceChildren(), and in
clearConnectors() change this.connectorSvg.innerHTML = "" to
this.connectorSvg.replaceChildren(); these changes operate on the SVG element
created in getOrCreateConnectorSvg() and preserve existing behavior.
In `@visualization/app/codeCharta/ui/codeMap/codeMap.tooltip.service.spec.ts`:
- Around line 101-157: The tests set window.innerWidth/innerHeight directly but
never restore them, which can leak into other tests; update the "positionTooltip
edge clamping" specs to capture the original values before calling
Object.defineProperty and restore them after each test (e.g., in a try/finally
inside each it or in an afterEach) so tests that call tooltipService.show and
tooltipService.updatePosition run against a clean viewport; reference the use of
window.innerWidth/window.innerHeight, tooltipService.show, and
tooltipService.updatePosition when locating where to save and restore the
originals.
In `@visualization/app/codeCharta/ui/codeMap/codeMap.tooltip.service.ts`:
- Around line 37-47: The show method uses this.tooltipElement after lazily
calling createTooltipElement(), but TypeScript may not narrow tooltipElement to
non-null; update show (or createTooltipElement) so tooltipElement is guaranteed
non-null: either add a post-creation null assertion when using
this.tooltipElement in show (e.g., assert/! when calling
populateTooltip/positionTooltip and setting style) or change
createTooltipElement to return the newly created element and assign it to
this.tooltipElement (and then use that local/this reference), ensuring
populateTooltip, positionTooltip and the style assignment operate on a non-null
HTMLElement; reference the show, createTooltipElement, tooltipElement,
populateTooltip and positionTooltip symbols.
In
`@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.spec.ts`:
- Around line 6-20: The test helper makeBuilding currently uses an unsafe
double-cast "as unknown as Node"; replace that with a real typed Node stub:
create a Node-typed object (e.g., nodeStub) that fills all required Node
properties with sensible defaults (path, name, id, width, height, length, x0,
y0, z0, isLeaf, and any other non-optional fields on the Node interface) and
pass that to new CodeMapBuilding instead of the double-cast; update makeBuilding
to construct and return new CodeMapBuilding(id, box, nodeStub, "#ff0000") so
type-safety is preserved and future interface changes will fail fast.
In
`@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts`:
- Around line 225-226: The traversal stack (stack and stackPointer in the BVH
traversal code) is currently allocated dynamically; preallocate it to avoid
runtime growth by creating an array sized from the BVH node count (e.g., const
capacity = Math.ceil(Math.log2(count)) * 2 or similar), fill it with a default
value, and initialize stackPointer to 1; replace the current "const stack:
number[] = [0]; let stackPointer = 1" with a preallocated array and keep the
same stack usage semantics so traversal logic (push/pop via stackPointer)
remains unchanged.
In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapMesh.ts`:
- Around line 468-474: The fillIndexBuffer method can read past templateIndices
if indicesPerNode and templateIndices.length diverge; update fillIndexBuffer to
bound the loop by the actual template size (e.g., use const count =
Math.min(indicesPerNode, templateIndices.length) and loop j from 0 to count) or
add a debug assertion that indicesPerNode === templateIndices.length before the
loop, referencing fillIndexBuffer, templateIndices and indicesPerNode so the
index buffer write cannot go out of bounds.
- Around line 493-495: Add a brief inline comment at the call to
geometry.addGroup(0, Number.POSITIVE_INFINITY, 0) explaining that we
intentionally set the group length to POSITIVE_INFINITY so the single material
(material index 0) covers the entire geometry to satisfy the multi-material
constructor pattern used when creating the mesh (refer to geometry.addGroup and
the multi-material constructor usage in this file); this clarifies why a
seemingly unusual POSITIVE_INFINITY value is used and prevents future accidental
removal.
In
`@visualization/app/codeCharta/ui/codeMap/rendering/geometryGenerationHelper.ts`:
- Around line 273-348: The templateNormals array is written one value per line
which bloats the file; refactor the Float32Array literal for templateNormals
into a compact inline format (like templatePositions) while keeping the existing
// prettier-ignore, preserving the exact numeric order and count; optionally
group values with short inline comments per face (e.g., "// left", "// right")
to retain readability, and update the surrounding code that references
templateNormals if any formatting assumptions exist.
In
`@visualization/app/codeCharta/ui/codeMap/threeViewer/threeRenderer.service.ts`:
- Around line 55-56: The subscription to
globalSettingsFacade.isWhiteBackground$() in the constructor/init is never
cleaned up; add a Subscription container (e.g., a private subscriptions = new
Subscription()) and add the isWhiteBackground$() subscription to it instead of
leaving it unmanaged, implement ngOnDestroy() on the service to call
this.subscriptions.unsubscribe() and also complete _afterRender$() there, and
keep using setBackgroundColorToState(...) for the callback so the logic location
(isWhiteBackground$(), setBackgroundColorToState, _afterRender$) is unchanged.
In
`@visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.spec.ts`:
- Around line 194-205: The test currently only asserts rendering; add an
assertion that the highlight clearing is invoked by spying on the highlight
service's clearHighlight method before calling
threeSceneService.applyClearHighlights(), e.g. create a spy on
threeSceneService["highlightService"].clearHighlight (mockImplementation to
noop), call applyClearHighlights(), and then expect the clearHighlight spy
toHaveBeenCalled() in addition to the existing render assertion so the test
verifies the intended side‑effect.
- Around line 73-80: Add an assertion that confirms the corresponding node id
was added to the highlightedNodeIds set after calling
threeSceneService.addBuildingsToHighlightingList(CODE_MAP_BUILDING);
specifically, verify
threeSceneService["highlightedNodeIds"].has(CODE_MAP_BUILDING.id) (or the
correct node id derived from CODE_MAP_BUILDING) alongside the existing checks
for highlightedBuildingIds and primaryHighlightedBuilding to keep coverage
aligned with the new state model.
- Around line 189-190: The test currently asserts that
threeSceneService["highlightedBuildingIds"] is empty and
threeSceneService["primaryHighlightedBuilding"] is null, but clearHighlight now
also clears node highlights; update the spec to assert that
threeSceneService["highlightedNodeIds"].size is 0 and
threeSceneService["primaryHighlightedNode"] is null after calling
clearHighlight. Locate the assertions near the existing checks for
highlightedBuildingIds and primaryHighlightedBuilding in the
threeSceneService.spec.ts and add the two analogous assertions referencing
highlightedNodeIds and primaryHighlightedNode to lock in the new behavior.
In
`@visualization/app/codeCharta/ui/fileExtensionBar/highlightBuildingsByFileExtension.service.spec.ts`:
- Around line 84-86: The afterEach currently calls jest.fn().mockClear(), which
clears a newly created anonymous mock and does nothing to your test mocks;
replace that line so you actually reset mocks between tests—either call
jest.clearAllMocks() (or jest.resetAllMocks()) in the afterEach, or call
mockClear()/mockReset() on the specific mock variables used in this spec (e.g.,
the spies/mocks you created for HighlightBuildingsByFileExtensionService or any
mocked functions); update the afterEach block to use those real mock-clearing
calls instead of jest.fn().mockClear().
In
`@visualization/app/codeCharta/ui/ribbonBar/heightSettingsPanel/heightSettingsPanel.component.spec.ts`:
- Around line 18-24: The test in the HeightSettingsPanelComponent spec uses
expect(screen.getByTitle("Height")).not.toBe(null); replace this with a more
idiomatic Testing Library assertion: either assert the element is present with
expect(screen.getByTitle("Height")).toBeInTheDocument() (ensure
`@testing-library/jest-dom` is configured) or simply call
screen.getByTitle("Height") without an assertion since getByTitle will throw if
not found; update the test accordingly to use toBeInTheDocument or the lone
getByTitle call and remove the redundant not.toBe(null) check.
In
`@visualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettingsPanel.component.html`:
- Around line 25-39: The color-only checkboxes in labelSettingsPanel (the
mat-checkbox elements bound to colorLabels.positive/neutral/negative and
mapColors) lack visible text labels; update each mat-checkbox (the ones using
setColorLabel($event, 'positive'/'neutral'/'negative')) to include readable text
like "Positive", "Neutral", "Negative" next to the color swatch so sighted users
and those with color-vision deficiencies can identify them while keeping
existing aria-labels and bindings intact.
In
`@visualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettingsPanel.component.ts`:
- Around line 50-52: The debounced function applyDebouncedTopLabels created with
debounce(LabelSettingsPanelComponent.DEBOUNCE_TIME) may leave a pending timer
across component teardown; either call the debounced function's cancel/clear
method in ngOnDestroy (if the debounce implementation exposes cancel/clear) to
prevent stale dispatches from firing, or replace the debounced closure with an
RxJS Subject (e.g., topLabelsSubject) piped through
debounceTime(LabelSettingsPanelComponent.DEBOUNCE_TIME) and subscribe to
dispatch setAmountOfTopLabels, then complete/unsubscribe the subject in
ngOnDestroy to ensure proper cleanup.
In `@visualization/app/material/matLabelSettingsMenu.scss`:
- Around line 1-4: The .mat-mdc-menu-panel.cc-label-settings-menu rule uses a
fixed min-width: 420px which can cause overflow on small viewports; update the
rule to use a responsive width (e.g., clamp() or min()/max() formula) so the
menu scales between a sensible minimum and maximum (keep the existing max-width:
500px) and prevents clipping on narrow screens while preserving desktop sizing.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonvisualization/app/codeCharta/ui/codeMap/rendering/__snapshots__/rendering.spec.ts.snapis excluded by!**/*.snapvisualization/app/codeCharta/ui/codeMap/rendering/shaders/glsl/codeMapVertexShader.glslis excluded by!**/*.glsl
📒 Files selected for processing (63)
CLAUDE.mdpackage.jsonplans/scenarios-prd.mdplans/threejs-map-performance-prd.mdvisualization/CHANGELOG.mdvisualization/app/app.config.tsvisualization/app/codeCharta/codeCharta.model.tsvisualization/app/codeCharta/features/3dPrint/components/export3DMapDialog/export3DMapDialog.component.tsvisualization/app/codeCharta/services/3DExports/3DPreview/MeshModels/mapMesh.spec.tsvisualization/app/codeCharta/state/effects/updateShowLabels/updateShowLabels.effect.spec.tsvisualization/app/codeCharta/state/effects/updateShowLabels/updateShowLabels.effect.tsvisualization/app/codeCharta/state/effects/updateVisibleTopLabels/getNumberOfTopLabels.spec.tsvisualization/app/codeCharta/state/effects/updateVisibleTopLabels/getNumberOfTopLabels.tsvisualization/app/codeCharta/state/effects/updateVisibleTopLabels/updateVisibleTopLabels.effect.spec.tsvisualization/app/codeCharta/state/effects/updateVisibleTopLabels/updateVisibleTopLabels.effect.tsvisualization/app/codeCharta/ui/codeMap/codeMap.component.tsvisualization/app/codeCharta/ui/codeMap/codeMap.label.service.spec.tsvisualization/app/codeCharta/ui/codeMap/codeMap.label.service.tsvisualization/app/codeCharta/ui/codeMap/codeMap.mouseEvent.service.spec.tsvisualization/app/codeCharta/ui/codeMap/codeMap.mouseEvent.service.tsvisualization/app/codeCharta/ui/codeMap/codeMap.render.service.spec.tsvisualization/app/codeCharta/ui/codeMap/codeMap.render.service.tsvisualization/app/codeCharta/ui/codeMap/codeMap.tooltip.service.spec.tsvisualization/app/codeCharta/ui/codeMap/codeMap.tooltip.service.tsvisualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.spec.tsvisualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.tsvisualization/app/codeCharta/ui/codeMap/rendering/codeMapMesh.spec.tsvisualization/app/codeCharta/ui/codeMap/rendering/codeMapMesh.tsvisualization/app/codeCharta/ui/codeMap/rendering/geometryGenerationHelper.tsvisualization/app/codeCharta/ui/codeMap/rendering/geometryGenerator.spec.tsvisualization/app/codeCharta/ui/codeMap/rendering/geometryGenerator.tsvisualization/app/codeCharta/ui/codeMap/threeViewer/threeRenderer.service.spec.tsvisualization/app/codeCharta/ui/codeMap/threeViewer/threeRenderer.service.tsvisualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.spec.tsvisualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.tsvisualization/app/codeCharta/ui/codeMap/threeViewer/threeViewer.service.spec.tsvisualization/app/codeCharta/ui/codeMap/threeViewer/threeViewer.service.tsvisualization/app/codeCharta/ui/customConfigs/uploadCustomConfigButton/uploadCustomConfigButton.component.tsvisualization/app/codeCharta/ui/fileExtensionBar/fileExtensionBar.component.spec.tsvisualization/app/codeCharta/ui/fileExtensionBar/highlightBuildingsByFileExtension.service.spec.tsvisualization/app/codeCharta/ui/fileExtensionBar/highlightBuildingsByFileExtension.service.tsvisualization/app/codeCharta/ui/ribbonBar/colorSettingsPanel/colorSettingsPanel.component.htmlvisualization/app/codeCharta/ui/ribbonBar/colorSettingsPanel/colorSettingsPanel.component.spec.tsvisualization/app/codeCharta/ui/ribbonBar/colorSettingsPanel/colorSettingsPanel.component.tsvisualization/app/codeCharta/ui/ribbonBar/heightSettingsPanel/heightSettingsPanel.component.htmlvisualization/app/codeCharta/ui/ribbonBar/heightSettingsPanel/heightSettingsPanel.component.scssvisualization/app/codeCharta/ui/ribbonBar/heightSettingsPanel/heightSettingsPanel.component.spec.tsvisualization/app/codeCharta/ui/ribbonBar/heightSettingsPanel/heightSettingsPanel.component.tsvisualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettings.component.htmlvisualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettings.component.scssvisualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettings.component.tsvisualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettingsPanel.component.htmlvisualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettingsPanel.component.scssvisualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettingsPanel.component.spec.tsvisualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettingsPanel.component.tsvisualization/app/codeCharta/ui/ribbonBar/ribbonBar.component.htmlvisualization/app/codeCharta/ui/ribbonBar/ribbonBar.component.tsvisualization/app/codeCharta/ui/ribbonBar/searchPanel/mapTreeView/mapTreeViewLevel/mapTreeViewLevel.component.spec.tsvisualization/app/codeCharta/ui/ribbonBar/searchPanel/mapTreeView/mapTreeViewLevel/mapTreeViewLevel.component.tsvisualization/app/codeCharta/ui/screenshotButton/screenshotButton.component.tsvisualization/app/codeCharta/ui/toolBar/uploadFilesButton/uploadFiles.service.tsvisualization/app/material/matLabelSettingsMenu.scssvisualization/app/material/material.scss
💤 Files with no reviewable changes (4)
- visualization/app/codeCharta/state/effects/updateShowLabels/updateShowLabels.effect.spec.ts
- visualization/app/codeCharta/ui/ribbonBar/searchPanel/mapTreeView/mapTreeViewLevel/mapTreeViewLevel.component.ts
- visualization/app/codeCharta/state/effects/updateShowLabels/updateShowLabels.effect.ts
- visualization/app/app.config.ts
✅ Files skipped from review due to trivial changes (2)
- visualization/app/codeCharta/ui/ribbonBar/labelSettingsPanel/labelSettings.component.scss
- visualization/app/codeCharta/ui/customConfigs/uploadCustomConfigButton/uploadCustomConfigButton.component.ts
Comment on lines
+31
to
+35
| private labels: InternalLabel[] = [] | ||
| private connectorSvg: SVGSVGElement | null = null | ||
| private readonly projectionVec = new Vector3() | ||
| private suppressedLabel: InternalLabel | null = null | ||
| private _suppressLayout = false |
There was a problem hiding this comment.
Reset suppressedLabel when labels are removed
If a suppressed label is cleared, the service keeps a stale reference, which can leak and cause unexpected restore behavior. Clear it when removing all labels or the specific label.
🔧 Proposed fix
clearLabels() {
for (const label of this.labels) {
this.threeSceneService.labels.remove(label.cssObject)
}
this.threeSceneService.labels.clear()
this.labels = []
+ this.suppressedLabel = null
this.clearConnectors()
}
clearTemporaryLabel(hoveredNode: Node) {
const index = this.labels.findIndex(({ node }) => node === hoveredNode)
if (index > -1) {
const label = this.labels[index]
this.threeSceneService.labels.remove(label.cssObject)
this.labels.splice(index, 1)
+ if (label === this.suppressedLabel) {
+ this.suppressedLabel = null
+ }
}
}Also applies to: 83-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts` around
lines 31 - 35, The service keeps a stale reference in suppressedLabel when
labels are removed; update the code paths that clear or remove labels (the logic
operating on the labels: InternalLabel[] and any methods that remove all labels
or individual labels around the block referenced at lines 83-99) to also set
suppressedLabel = null when the removed label equals suppressedLabel or when
clearing the labels array, ensuring suppressedLabel is reset whenever labels are
cleared or the specific suppressed label is deleted (refer to the
suppressedLabel field and the labels array to locate the correct places to
change).
Comment on lines
+137
to
227
| updateLabelLayout() { | ||
| if (this._suppressLayout || this.labels.length === 0) { | ||
| this.clearConnectors() | ||
| return | ||
| } | ||
|
|
||
| const infos = this.collectLabelInfos() | ||
| const tooltipRect = this.tooltipService.getRect() | ||
| this.resolveCollisions(infos, tooltipRect) | ||
| this.drawConnectors(infos) | ||
| } | ||
|
|
||
| private collectLabelInfos(): LabelLayoutInfo[] { | ||
| const infos: LabelLayoutInfo[] = [] | ||
|
|
||
| // Phase 1: Reset collision offsets (batch writes — all style mutations before any rect reads) | ||
| for (const label of this.labels) { | ||
| const content = label.cssObject.element.firstElementChild as HTMLDivElement | ||
| if (!content) { | ||
| continue | ||
| } | ||
| content.style.transform = `translateY(${CodeMapLabelService.BASE_OFFSET_PX}px)` | ||
| infos.push({ label, content, rect: null, offset: 0 }) | ||
| } | ||
|
|
||
| // Phase 2: Read all bounding rects after writes are complete. | ||
| // Grouping all getBoundingClientRect calls here (rather than interleaving them with style | ||
| // mutations) limits the number of forced layout recalculations to one per update cycle. | ||
| for (const info of infos) { | ||
| info.rect = info.content.getBoundingClientRect() | ||
| } | ||
|
|
||
| return infos | ||
| } | ||
|
|
||
| private resolveCollisions(infos: LabelLayoutInfo[], tooltipRect: DOMRect | null) { | ||
| // Sort by screen Y and compute collision offsets (greedy sweep) | ||
| infos.sort((a, b) => a.rect.top - b.rect.top) | ||
|
|
||
| for (let i = 0; i < infos.length; i++) { | ||
| const current = infos[i] | ||
|
|
||
| // Check collision with tooltip (immovable obstacle) | ||
| if (tooltipRect) { | ||
| const currentTop = current.rect.top + current.offset | ||
| const currentBottom = current.rect.bottom + current.offset | ||
| const horizontalOverlap = current.rect.right > tooltipRect.left && current.rect.left < tooltipRect.right | ||
| const verticalOverlap = currentBottom > tooltipRect.top && currentTop < tooltipRect.bottom | ||
|
|
||
| if (horizontalOverlap && verticalOverlap) { | ||
| const overlap = tooltipRect.bottom + CodeMapLabelService.LABEL_GAP_PX - currentTop | ||
| if (overlap > 0) { | ||
| current.offset += overlap | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (element.sprite !== undefined) { | ||
| this.disposeSprite(element.sprite) | ||
| // Check collision with previous labels | ||
| for (let j = i - 1; j >= 0; j--) { | ||
| const above = infos[j] | ||
|
|
||
| if (current.rect.right <= above.rect.left || current.rect.left >= above.rect.right) { | ||
| continue | ||
| } | ||
|
|
||
| const aboveBottom = above.rect.bottom + above.offset | ||
| const currentTop = current.rect.top + current.offset | ||
| const overlap = aboveBottom + CodeMapLabelService.LABEL_GAP_PX - currentTop | ||
|
|
||
| if (overlap > 0) { | ||
| current.offset += overlap | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (element.line !== undefined) { | ||
| this.disposeLine(element.line) | ||
| // Apply offsets with max displacement (batch writes) | ||
| for (const info of infos) { | ||
| const content = info.content | ||
| content.style.transition = "transform 0.2s ease-out, opacity 0.2s ease-out" | ||
|
|
||
| if (info.offset > CodeMapLabelService.MAX_DISPLACEMENT_PX) { | ||
| content.style.opacity = "0" | ||
| content.style.transform = `translateY(${CodeMapLabelService.BASE_OFFSET_PX}px)` | ||
| } else if (info.offset !== 0) { | ||
| content.style.opacity = info.label === this.suppressedLabel ? "0" : "1" | ||
| content.style.transform = `translateY(${CodeMapLabelService.BASE_OFFSET_PX + info.offset}px)` | ||
| } else { | ||
| content.style.opacity = info.label === this.suppressedLabel ? "0" : "1" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t let suppressed labels participate in collision offsets
Suppressed labels are hidden but still push other labels. That can create unnecessary displacement while a tooltip is open. Consider skipping suppressed labels in collision calculations.
🔧 Proposed fix
for (let i = 0; i < infos.length; i++) {
const current = infos[i]
+ if (current.label === this.suppressedLabel) {
+ continue
+ }
// Check collision with tooltip (immovable obstacle)
if (tooltipRect) {
const currentTop = current.rect.top + current.offset
const currentBottom = current.rect.bottom + current.offset
const horizontalOverlap = current.rect.right > tooltipRect.left && current.rect.left < tooltipRect.right
const verticalOverlap = currentBottom > tooltipRect.top && currentTop < tooltipRect.bottom
if (horizontalOverlap && verticalOverlap) {
const overlap = tooltipRect.bottom + CodeMapLabelService.LABEL_GAP_PX - currentTop
if (overlap > 0) {
current.offset += overlap
}
}
}
// Check collision with previous labels
for (let j = i - 1; j >= 0; j--) {
const above = infos[j]
+ if (above.label === this.suppressedLabel) {
+ continue
+ }
if (current.rect.right <= above.rect.left || current.rect.left >= above.rect.right) {
continue
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts` around
lines 137 - 227, The resolve is to prevent suppressed labels from influencing
layout: in CodeMapLabelService.resolveCollisions, exclude any info whose label
=== this.suppressedLabel from collision detection calculations (both tooltip
collision check and the loop that checks collisions with previous labels) — you
can either filter infos into a separate array of activeInfos before sorting and
sweeping, or skip entries where info.label === this.suppressedLabel inside the
loops; still keep suppressed labels in the final apply-offsets pass so they
remain hidden (opacity 0) but do not contribute offsets for others.
Comment on lines
101
to
118
| start() { | ||
| this.threeRendererService.renderer.domElement.addEventListener("mousemove", debounce(this.onDocumentMouseMove, 1)) | ||
| this.threeRendererService.renderer.domElement.addEventListener("mouseup", event => this.onDocumentMouseUp(event)) | ||
| this.threeRendererService.renderer.domElement.addEventListener("mousedown", event => this.onDocumentMouseDown(event)) | ||
| this.threeRendererService.renderer.domElement.addEventListener("dblclick", () => this.onDocumentDoubleClick()) | ||
| this.threeRendererService.renderer.domElement.addEventListener("mouseleave", event => this.onDocumentMouseLeave(event)) | ||
| this.threeRendererService.renderer.domElement.addEventListener("mouseenter", () => this.onDocumentMouseEnter()) | ||
| this.threeRendererService.renderer.domElement.addEventListener( | ||
| this.registerEventListeners() | ||
| this.viewCubeMouseEvents.subscribe("viewCubeEventPropagation", this.onViewCubeEventPropagation) | ||
| } | ||
|
|
||
| private registerEventListeners() { | ||
| const domElement = this.threeRendererService.renderer.domElement | ||
| domElement.addEventListener("mousemove", this.onDocumentMouseMove) | ||
| domElement.addEventListener("mouseup", event => this.onDocumentMouseUp(event)) | ||
| domElement.addEventListener("mousedown", event => this.onDocumentMouseDown(event)) | ||
| domElement.addEventListener("dblclick", () => this.onDocumentDoubleClick()) | ||
| domElement.addEventListener("mouseleave", event => this.onDocumentMouseLeave(event)) | ||
| domElement.addEventListener("mouseenter", () => this.onDocumentMouseEnter()) | ||
| domElement.addEventListener( | ||
| "wheel", | ||
| debounce(() => this.threeRendererService.render()) | ||
| ) | ||
| this.viewCubeMouseEvents.subscribe("viewCubeEventPropagation", this.onViewCubeEventPropagation) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find call sites of start() on codeMapMouseEventService
rg -n "\.start\(\)" visualization/app/codeCharta/ui/codeMap/ -C 2 --type=ts
echo "---"
# Also search for CodeMapMouseEventService references
rg -n "CodeMapMouseEventService" visualization/app/codeCharta/ui/codeMap/ --type=tsRepository: MaibornWolff/codecharta
Length of output: 6213
🏁 Script executed:
#!/bin/bash
# Get the full service file to check for cleanup/destroy methods
wc -l visualization/app/codeCharta/ui/codeMap/codeMap.mouseEvent.service.tsRepository: MaibornWolff/codecharta
Length of output: 140
🏁 Script executed:
#!/bin/bash
# Check the full service file for ngOnDestroy and listener cleanup
cat -n visualization/app/codeCharta/ui/codeMap/codeMap.mouseEvent.service.ts | head -120Repository: MaibornWolff/codecharta
Length of output: 5921
🏁 Script executed:
#!/bin/bash
# Check if ngOnDestroy exists and what it does
rg -n "ngOnDestroy" visualization/app/codeCharta/ui/codeMap/codeMap.mouseEvent.service.ts -A 10Repository: MaibornWolff/codecharta
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Check codeMap.component.ts to see the context of start() calls more clearly
cat -n visualization/app/codeCharta/ui/codeMap/codeMap.component.ts | sed -n '20,50p'Repository: MaibornWolff/codecharta
Length of output: 1574
Add DOM event listener cleanup in ngOnDestroy or guard start() to prevent duplicate registration
The service calls start() at least twice: once in ngAfterViewInit() (line 44 in codeMap.component.ts) and potentially multiple times when sharpnessMode changes (line 28 via restartOnSharpnessModeChangesSubscription). Each call to start() registers new DOM event listeners without removing previous ones, and ngOnDestroy() only cleans up store subscriptions—not the DOM listeners. This causes listener stacking and duplicate event handler execution.
Store references to registered listeners and remove them in ngOnDestroy(), or guard registerEventListeners() with a boolean flag to ensure listeners are registered only once.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@visualization/app/codeCharta/ui/codeMap/codeMap.mouseEvent.service.ts` around
lines 101 - 118, The DOM listeners registered in start() via
registerEventListeners() are reattached on multiple starts causing duplicate
handlers; modify the service so registerEventListeners() either records the
actual listener functions (for example the bound handlers like
onDocumentMouseMove, onDocumentMouseUp, onDocumentMouseDown,
onDocumentDoubleClick, onDocumentMouseLeave, onDocumentMouseEnter and the
debounced wheel handler) and removes them in ngOnDestroy(), or add an
"eventsRegistered" boolean guard checked/ set in
start()/registerEventListeners() to ensure registration happens only once;
ensure ngOnDestroy() removes the listeners if you choose the record-and-remove
approach so no duplicate handlers remain.
Comment on lines
+134
to
+141
| private getMapMaterials(): Material[] | null { | ||
| const child = this.mapGeometry.children[0] | ||
| if (!child) { | ||
| return null | ||
| } | ||
| const mat = (child as unknown as { material: unknown }).material | ||
| return Array.isArray(mat) ? (mat as Material[]) : null | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find files related to map mesh
fd -t f '\.ts$' | xargs grep -l 'codeMapMesh\|getMapMaterials\|mapGeometry' | head -20Repository: MaibornWolff/codecharta
Length of output: 846
🏁 Script executed:
# Find codeMapMesh.ts file
fd 'codeMapMesh\.ts$' -type fRepository: MaibornWolff/codecharta
Length of output: 237
🏁 Script executed:
# Find codeMapMesh.ts file (correct syntax)
fd 'codeMapMesh\.ts$' -t fRepository: MaibornWolff/codecharta
Length of output: 131
🏁 Script executed:
# Read codeMapMesh.ts to understand mesh and material creation
cat -n visualization/app/codeCharta/ui/codeMap/rendering/codeMapMesh.tsRepository: MaibornWolff/codecharta
Length of output: 21961
🏁 Script executed:
# Read threeSceneService.ts to see getMapMaterials() and how it's used
cat -n visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.tsRepository: MaibornWolff/codecharta
Length of output: 17096
🏁 Script executed:
# Verify if there are any tests that show the material handling behavior
rg -A 5 -B 5 "getMapMaterials" visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.spec.tsRepository: MaibornWolff/codecharta
Length of output: 49
🏁 Script executed:
# Check if there are other references to how materials are used in the codebase
rg -n "threeMesh.material|child.material" visualization/app/codeCharta/ui/codeMapRepository: MaibornWolff/codecharta
Length of output: 179
Handle single-material meshes in getMapMaterials.
The mesh uses a single Material, not an array, so this method returns null and folder label color updates are skipped during highlighting, selection, and deselection. Wrap the single Material in an array to fix it.
Suggested fix
- const mat = (child as unknown as { material: unknown }).material
- return Array.isArray(mat) ? (mat as Material[]) : null
+ const mat = (child as unknown as { material: unknown }).material
+ if (Array.isArray(mat)) {
+ return mat as Material[]
+ }
+ return mat instanceof Material ? [mat] : null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private getMapMaterials(): Material[] | null { | |
| const child = this.mapGeometry.children[0] | |
| if (!child) { | |
| return null | |
| } | |
| const mat = (child as unknown as { material: unknown }).material | |
| return Array.isArray(mat) ? (mat as Material[]) : null | |
| } | |
| private getMapMaterials(): Material[] | null { | |
| const child = this.mapGeometry.children[0] | |
| if (!child) { | |
| return null | |
| } | |
| const mat = (child as unknown as { material: unknown }).material | |
| if (Array.isArray(mat)) { | |
| return mat as Material[] | |
| } | |
| return mat instanceof Material ? [mat] : null | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.ts`
around lines 134 - 141, getMapMaterials currently returns null for meshes with a
single Material because it only returns when child.material is an array; update
getMapMaterials (in threeSceneService, referencing mapGeometry.children and the
child.material extraction) to detect a single Material and wrap it in an array
before returning, i.e., if mat is not an array but is a Material then return
[mat] so highlighting/selection color updates iterate over a consistent
Material[].
Comment on lines
+302
to
309
| if (this.highlightedBuildingIds.size > 0) { | ||
| this.applyHighlights() | ||
| } | ||
| this.selected = null | ||
| if (this.mapGeometry.children[0]) { | ||
| this.resetMaterial(this.mapGeometry.children[0]["material"]) | ||
| const materials = this.getMapMaterials() | ||
| if (materials) { | ||
| this.resetMaterial(materials) | ||
| } |
There was a problem hiding this comment.
Highlight dimming gets wiped on clearSelection.
Line 302-309: applyHighlights() runs while selected is still set, then resetMaterial() unconditionally resets all labels to highlighted color. This overwrites dimming for non-highlighted nodes, so highlights won’t visually persist after clearing selection.
✅ Suggested fix
- if (this.highlightedBuildingIds.size > 0) {
- this.applyHighlights()
- }
- this.selected = null
- const materials = this.getMapMaterials()
- if (materials) {
- this.resetMaterial(materials)
- }
+ this.selected = null
+ if (this.highlightedBuildingIds.size > 0) {
+ this.applyHighlights()
+ } else {
+ const materials = this.getMapMaterials()
+ if (materials) {
+ this.resetMaterial(materials)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (this.highlightedBuildingIds.size > 0) { | |
| this.applyHighlights() | |
| } | |
| this.selected = null | |
| if (this.mapGeometry.children[0]) { | |
| this.resetMaterial(this.mapGeometry.children[0]["material"]) | |
| const materials = this.getMapMaterials() | |
| if (materials) { | |
| this.resetMaterial(materials) | |
| } | |
| this.selected = null | |
| if (this.highlightedBuildingIds.size > 0) { | |
| this.applyHighlights() | |
| } else { | |
| const materials = this.getMapMaterials() | |
| if (materials) { | |
| this.resetMaterial(materials) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.ts`
around lines 302 - 309, The code currently calls applyHighlights() while
this.selected is still set and then calls resetMaterial() which unconditionally
resets all labels to the highlighted color, wiping dimming; change the order or
logic so highlighted state persists: either call
resetMaterial(getMapMaterials()) first, then set this.selected = null and
finally call applyHighlights(), or alter resetMaterial() to skip/restyle nodes
whose ids are contained in this.highlightedBuildingIds (use getMapMaterials(),
resetMaterial(), applyHighlights(), this.selected, this.highlightedBuildingIds
to locate the code) so resetMaterial does not overwrite the dimming for
non-highlighted nodes.
Comment on lines
+1
to
+10
| @let isDeltaState = isDeltaState$ | async; | ||
| @let colorMetric = colorMetric$ | async; | ||
| @let sliderValues = sliderValues$ | async; | ||
| @let sliderColors = sliderColors$ | async; | ||
|
|
||
| @if (!isDeltaState && colorMetric !== "unary") { | ||
| <div> | ||
| <cc-metric-color-range-slider | ||
| [minValue]="(sliderValues$ | async).min" | ||
| [maxValue]="(sliderValues$ | async).max" | ||
| [currentLeftValue]="(sliderValues$ | async).from" | ||
| [currentRightValue]="(sliderValues$ | async).to" | ||
| [leftColor]="(sliderColors$ | async).leftColor" | ||
| [middleColor]="(sliderColors$ | async).middleColor" | ||
| [rightColor]="(sliderColors$ | async).rightColor" | ||
| [minValue]="sliderValues.min" | ||
| [maxValue]="sliderValues.max" |
There was a problem hiding this comment.
Consider null safety for async-derived template variables.
The @let declarations extract values from async observables, but the @if condition at line 6 doesn't guard against sliderValues or sliderColors being null before the observables emit. If the template renders before observables emit, accessing sliderValues.min (line 9) could throw.
Consider adding null guards:
-@if (!isDeltaState && colorMetric !== "unary") {
+@if (!isDeltaState && colorMetric !== "unary" && sliderValues && sliderColors) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @let isDeltaState = isDeltaState$ | async; | |
| @let colorMetric = colorMetric$ | async; | |
| @let sliderValues = sliderValues$ | async; | |
| @let sliderColors = sliderColors$ | async; | |
| @if (!isDeltaState && colorMetric !== "unary") { | |
| <div> | |
| <cc-metric-color-range-slider | |
| [minValue]="(sliderValues$ | async).min" | |
| [maxValue]="(sliderValues$ | async).max" | |
| [currentLeftValue]="(sliderValues$ | async).from" | |
| [currentRightValue]="(sliderValues$ | async).to" | |
| [leftColor]="(sliderColors$ | async).leftColor" | |
| [middleColor]="(sliderColors$ | async).middleColor" | |
| [rightColor]="(sliderColors$ | async).rightColor" | |
| [minValue]="sliderValues.min" | |
| [maxValue]="sliderValues.max" | |
| `@let` isDeltaState = isDeltaState$ | async; | |
| `@let` colorMetric = colorMetric$ | async; | |
| `@let` sliderValues = sliderValues$ | async; | |
| `@let` sliderColors = sliderColors$ | async; | |
| `@if` (!isDeltaState && colorMetric !== "unary" && sliderValues && sliderColors) { | |
| <div> | |
| <cc-metric-color-range-slider | |
| [minValue]="sliderValues.min" | |
| [maxValue]="sliderValues.max" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@visualization/app/codeCharta/ui/ribbonBar/colorSettingsPanel/colorSettingsPanel.component.html`
around lines 1 - 10, The template uses async-derived variables (isDeltaState$,
colorMetric$, sliderValues$, sliderColors$) but access sliderValues.min/max and
sliderColors before ensuring those objects are set, which can throw if
observables haven't emitted; update the `@if` condition to also check sliderValues
and sliderColors are truthy (e.g., `@if` (!isDeltaState && colorMetric !== "unary"
&& sliderValues && sliderColors)) so cc-metric-color-range-slider only receives
sliderValues.min/max when sliderValues is not null, and similarly guard any
other bindings that rely on sliderColors.
…ive complexity Resolve 13 non-readonly SonarCloud issues: extract helper methods to reduce cognitive complexity, replace nested ternaries, use nullish coalescing, group method parameters into interfaces, and apply ??= operator. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…embers Mark 9 class members as readonly per SonarCloud S2933. Update tests to use .clear() instead of reassigning readonly Set instances. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts (1)
55-58: KeepscaledBoxes/BVH in sync when adding after scaling.This is the same issue flagged in the previous review: if
add()is called aftersetScales(), the new building won’t be inscaledBoxes/BVH and intersections will silently skip it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts` around lines 55 - 58, The add(building: CodeMapBuilding) method currently only appends to _buildings and buildingsByPath, so when add() is called after setScales() the new building is never transformed into scaledBoxes or inserted into the BVH; update add() to also create the building’s scaled box using the same scaling logic used in setScales() (or call the existing helper that produces scaledBoxes), push that scaled box into scaledBoxes, and insert it into the BVH (or rebuild/rehash the BVH entry) so intersections include newly added buildings; reference add, setScales, scaledBoxes, BVH (or bvh), buildingsByPath and the CodeMapBuilding scaling helper when making the change.visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts (2)
83-99:⚠️ Potential issue | 🟡 MinorReset
suppressedLabelwhen labels are cleared/removed.Otherwise a stale reference can be restored later after the label is gone.
🛠️ Suggested fix
clearLabels() { for (const label of this.labels) { this.threeSceneService.labels.remove(label.cssObject) } this.threeSceneService.labels.clear() this.labels = [] + this.suppressedLabel = null this.clearConnectors() } clearTemporaryLabel(hoveredNode: Node) { const index = this.labels.findIndex(({ node }) => node === hoveredNode) if (index > -1) { const label = this.labels[index] this.threeSceneService.labels.remove(label.cssObject) this.labels.splice(index, 1) + if (label === this.suppressedLabel) { + this.suppressedLabel = null + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts` around lines 83 - 99, When removing labels in clearLabels and clearTemporaryLabel, also reset the suppressedLabel property so a stale reference isn't restored later; update clearLabels (after this.labels = [] or after clearConnectors()) to set this.suppressedLabel = undefined (or null) and update clearTemporaryLabel to set this.suppressedLabel = undefined if the removed label === this.suppressedLabel, ensuring suppressedLabel is cleared whenever labels are removed.
137-183:⚠️ Potential issue | 🟡 MinorSuppressed labels should not affect collision offsets.
Hidden labels still push visible ones, which can create unnecessary displacement while a tooltip is active.
🛠️ Suggested fix
for (let i = 0; i < infos.length; i++) { const current = infos[i] + if (current.label === this.suppressedLabel) { + continue + } this.resolveTooltipCollision(current, tooltipRect) this.resolveLabelCollisions(current, infos, i) } @@ for (let j = index - 1; j >= 0; j--) { const above = infos[j] + if (above.label === this.suppressedLabel) { + continue + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts` around lines 137 - 183, The collision logic currently includes suppressed/hidden labels which displace visible labels; update collectLabelInfos (used by updateLabelLayout) to skip labels that are suppressed/hidden (e.g., check label.cssObject.element visibility/state or a suppressed flag) so infos only contains visible labels, and ensure resolveCollisions (and its helpers resolveTooltipCollision, resolveLabelCollisions, applyCollisionOffsets) operates only on that filtered infos array so hidden labels do not contribute offsets.visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.ts (2)
134-141:⚠️ Potential issue | 🟠 MajorHandle single-material meshes in
getMapMaterials.When
child.materialis a singleMaterial, this returnsnulland label color updates are skipped. Wrap single materials in an array.🛠️ Suggested fix
const mat = (child as unknown as { material: unknown }).material - return Array.isArray(mat) ? (mat as Material[]) : null + if (Array.isArray(mat)) { + return mat as Material[] + } + return mat instanceof Material ? [mat] : nullRun this to confirm how the map mesh material is created and typed:
#!/bin/bash # Inspect where the map mesh is created and what type its material is. fd 'geometryGenerator\.ts$|codeMapMesh\.ts$' -t f visualization/app/codeCharta/ui/codeMap/rendering rg -n "new InstancedMesh|new Mesh|material" visualization/app/codeCharta/ui/codeMap/rendering -g'*.ts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.ts` around lines 134 - 141, getMapMaterials currently returns null when child.material is a single Material, skipping label color updates; change it to detect if mat is a single Material and return [mat as Material] in that case. Specifically, inside getMapMaterials inspect (child as { material: unknown }).material: if Array.isArray(mat) return mat as Material[]; else if mat is truthy return [mat as Material]; preserve the existing null return only when there is no child or material is undefined. Update references to mapGeometry and child.material accordingly in getMapMaterials.
300-307:⚠️ Potential issue | 🟠 MajorClearing selection wipes highlight dimming.
applyHighlights()runs whileselectedis still set, thenresetMaterial()overwrites dimming for non-highlighted nodes. This makes highlights vanish after clearing selection.🛠️ Suggested fix
- if (this.highlightedBuildingIds.size > 0) { - this.applyHighlights() - } - this.selected = null - const materials = this.getMapMaterials() - if (materials) { - this.resetMaterial(materials) - } + this.selected = null + if (this.highlightedBuildingIds.size > 0) { + this.applyHighlights() + } else { + const materials = this.getMapMaterials() + if (materials) { + this.resetMaterial(materials) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.ts` around lines 300 - 307, The selection is cleared after applyHighlights() which causes resetMaterial() to overwrite the highlighting; first set this.selected = null before calling applyHighlights() and then ensure resetMaterial(materials) skips resetting materials for highlighted nodes by checking this.highlightedBuildingIds (i.e., modify resetMaterial/getMapMaterials usage so it preserves dimming for any id where this.highlightedBuildingIds.has(id) or otherwise only resets non-highlighted materials), referencing the methods applyHighlights(), resetMaterial(), getMapMaterials(), selected and highlightedBuildingIds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts`:
- Around line 37-44: The subscription to threeRendererService.afterRender$
created in the constructor is not stored or cleaned up; store the Subscription
returned by subscribing (e.g., a private property like afterRenderSubscription)
when calling this.updateLabelLayout(), and then unsubscribe from that property
inside the class destroy() method (call afterRenderSubscription.unsubscribe() if
set) to prevent the observable from continuing to fire after destroy();
reference the constructor, threeRendererService.afterRender$,
updateLabelLayout(), and destroy() when applying this change.
In
`@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts`:
- Around line 229-305: rayIntersectsNode can produce NaN when a ray direction
component is 0; fix by guarding parallel axes: in rayIntersectsNode (using
NODE_OFFSETS.minX/maxX etc.) first detect if ray.direction.x/y/z === 0 (or
invDirX/Y/Z is Infinite) and if so return false immediately when origin.x/y/z is
outside the node slab (origin < min || origin > max); otherwise skip computing
tMin/tMax for that axis or compute them as now when direction is non-zero.
Update rayIntersectsNode to perform these per-axis checks before computing
tEnter/tExit so parallel rays that lie inside the slab continue testing and
those outside are rejected.
In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapMesh.ts`:
- Around line 128-153: In highlightBuilding, the incremental path ignores
changes to constantHighlight because only _prevHighlightedIds is tracked; detect
changes to constantHighlight by introducing a stored _prevConstantHighlightKeys
Set (derived from constantHighlight.keys()) and compare it to the current keys
(fast Set size and membership check); if they differ (or when
_prevIsPresentationMode toggles) set _prevHighlightedIds = null to force the
full update branch, and at the end assign this._prevConstantHighlightKeys = new
Set(constantHighlight.keys()) alongside the existing this._prevHighlightedIds
and this._prevIsPresentationMode updates so newly constant-highlighted buildings
get recolored.
---
Duplicate comments:
In `@visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts`:
- Around line 83-99: When removing labels in clearLabels and
clearTemporaryLabel, also reset the suppressedLabel property so a stale
reference isn't restored later; update clearLabels (after this.labels = [] or
after clearConnectors()) to set this.suppressedLabel = undefined (or null) and
update clearTemporaryLabel to set this.suppressedLabel = undefined if the
removed label === this.suppressedLabel, ensuring suppressedLabel is cleared
whenever labels are removed.
- Around line 137-183: The collision logic currently includes suppressed/hidden
labels which displace visible labels; update collectLabelInfos (used by
updateLabelLayout) to skip labels that are suppressed/hidden (e.g., check
label.cssObject.element visibility/state or a suppressed flag) so infos only
contains visible labels, and ensure resolveCollisions (and its helpers
resolveTooltipCollision, resolveLabelCollisions, applyCollisionOffsets) operates
only on that filtered infos array so hidden labels do not contribute offsets.
In
`@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts`:
- Around line 55-58: The add(building: CodeMapBuilding) method currently only
appends to _buildings and buildingsByPath, so when add() is called after
setScales() the new building is never transformed into scaledBoxes or inserted
into the BVH; update add() to also create the building’s scaled box using the
same scaling logic used in setScales() (or call the existing helper that
produces scaledBoxes), push that scaled box into scaledBoxes, and insert it into
the BVH (or rebuild/rehash the BVH entry) so intersections include newly added
buildings; reference add, setScales, scaledBoxes, BVH (or bvh), buildingsByPath
and the CodeMapBuilding scaling helper when making the change.
In `@visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.ts`:
- Around line 134-141: getMapMaterials currently returns null when
child.material is a single Material, skipping label color updates; change it to
detect if mat is a single Material and return [mat as Material] in that case.
Specifically, inside getMapMaterials inspect (child as { material: unknown
}).material: if Array.isArray(mat) return mat as Material[]; else if mat is
truthy return [mat as Material]; preserve the existing null return only when
there is no child or material is undefined. Update references to mapGeometry and
child.material accordingly in getMapMaterials.
- Around line 300-307: The selection is cleared after applyHighlights() which
causes resetMaterial() to overwrite the highlighting; first set this.selected =
null before calling applyHighlights() and then ensure resetMaterial(materials)
skips resetting materials for highlighted nodes by checking
this.highlightedBuildingIds (i.e., modify resetMaterial/getMapMaterials usage so
it preserves dimming for any id where this.highlightedBuildingIds.has(id) or
otherwise only resets non-highlighted materials), referencing the methods
applyHighlights(), resetMaterial(), getMapMaterials(), selected and
highlightedBuildingIds.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
visualization/app/codeCharta/ui/codeMap/codeMap.label.service.tsvisualization/app/codeCharta/ui/codeMap/codeMap.tooltip.service.tsvisualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.tsvisualization/app/codeCharta/ui/codeMap/rendering/codeMapMesh.tsvisualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- visualization/app/codeCharta/ui/codeMap/codeMap.tooltip.service.ts
Comment on lines
37
to
44
| constructor( | ||
| private readonly state: State<CcState>, | ||
| private readonly threeCameraService: ThreeCameraService, | ||
| private threeSceneService: ThreeSceneService, | ||
| private readonly threeMapControlsService: ThreeMapControlsService | ||
| private threeRendererService: ThreeRendererService, | ||
| private tooltipService: CodeMapTooltipService | ||
| ) { | ||
| this.labels = new Array<InternalLabel>() | ||
| this.threeMapControlsService.subscribe("onCameraChanged", () => this.onCameraChanged()) | ||
| this.threeRendererService.afterRender$.subscribe(() => this.updateLabelLayout()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find usages of CodeMapLabelService
rg -n "CodeMapLabelService" visualization/app/codeCharta -g'*.ts'Repository: MaibornWolff/codecharta
Length of output: 3676
🏁 Script executed:
#!/bin/bash
# Find destroy() calls in codeMap directory
rg -n "destroy\(\)" visualization/app/codeCharta/ui/codeMap -g'*.ts'Repository: MaibornWolff/codecharta
Length of output: 1195
🏁 Script executed:
#!/bin/bash
# Show the current destroy() implementation
cat -n visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts | head -100Repository: MaibornWolff/codecharta
Length of output: 4472
🏁 Script executed:
#!/bin/bash
# See the destroy() method implementation
sed -n '330,360p' visualization/app/codeCharta/ui/codeMap/codeMap.label.service.tsRepository: MaibornWolff/codecharta
Length of output: 1003
🏁 Script executed:
#!/bin/bash
# Check if there are other subscriptions being managed in the service
rg -n "subscribe" visualization/app/codeCharta/ui/codeMap/codeMap.label.service.tsRepository: MaibornWolff/codecharta
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Check CodeMapComponent to see how it destroys services
sed -n '1,100p' visualization/app/codeCharta/ui/codeMap/codeMap.component.tsRepository: MaibornWolff/codecharta
Length of output: 2398
🏁 Script executed:
#!/bin/bash
# Check ThreeViewerService's destroy() implementation
sed -n '100,130p' visualization/app/codeCharta/ui/codeMap/threeViewer/threeViewer.service.tsRepository: MaibornWolff/codecharta
Length of output: 667
🏁 Script executed:
#!/bin/bash
# Get the full constructor and destroy pattern of ThreeViewerService
sed -n '1,120p' visualization/app/codeCharta/ui/codeMap/threeViewer/threeViewer.service.tsRepository: MaibornWolff/codecharta
Length of output: 4115
🏁 Script executed:
#!/bin/bash
# Check if CodeMapLabelService.destroy() is called anywhere
rg -n "\.destroy\(\)" visualization/app/codeCharta/ui/codeMap -B 2 -A 1 | grep -A 3 -B 3 "codeMapLabelService\|labelService"Repository: MaibornWolff/codecharta
Length of output: 1216
Store and unsubscribe from afterRender$ subscription in destroy().
The subscription created in the constructor (line 43) is never unsubscribed. If destroy() is called, the subscription will continue firing and updating layouts unnecessarily. Store the subscription as a property and unsubscribe in destroy().
🛠️ Suggested fix
+ private readonly afterRenderSub = this.threeRendererService.afterRender$.subscribe(() => this.updateLabelLayout())
+
constructor(
private readonly state: State<CcState>,
private threeSceneService: ThreeSceneService,
private readonly threeRendererService: ThreeRendererService,
private readonly tooltipService: CodeMapTooltipService
) {
- this.threeRendererService.afterRender$.subscribe(() => this.updateLabelLayout())
}
destroy() {
+ this.afterRenderSub.unsubscribe()
this.clearLabels()
if (this.connectorSvg) {
this.connectorSvg.remove()
this.connectorSvg = null
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| constructor( | |
| private readonly state: State<CcState>, | |
| private readonly threeCameraService: ThreeCameraService, | |
| private threeSceneService: ThreeSceneService, | |
| private readonly threeMapControlsService: ThreeMapControlsService | |
| private threeRendererService: ThreeRendererService, | |
| private tooltipService: CodeMapTooltipService | |
| ) { | |
| this.labels = new Array<InternalLabel>() | |
| this.threeMapControlsService.subscribe("onCameraChanged", () => this.onCameraChanged()) | |
| this.threeRendererService.afterRender$.subscribe(() => this.updateLabelLayout()) | |
| } | |
| private afterRenderSub: Subscription | |
| constructor( | |
| private readonly state: State<CcState>, | |
| private threeSceneService: ThreeSceneService, | |
| private readonly threeRendererService: ThreeRendererService, | |
| private readonly tooltipService: CodeMapTooltipService | |
| ) { | |
| this.afterRenderSub = this.threeRendererService.afterRender$.subscribe(() => this.updateLabelLayout()) | |
| } | |
| destroy() { | |
| this.afterRenderSub.unsubscribe() | |
| this.clearLabels() | |
| if (this.connectorSvg) { | |
| this.connectorSvg.remove() | |
| this.connectorSvg = null | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@visualization/app/codeCharta/ui/codeMap/codeMap.label.service.ts` around
lines 37 - 44, The subscription to threeRendererService.afterRender$ created in
the constructor is not stored or cleaned up; store the Subscription returned by
subscribing (e.g., a private property like afterRenderSubscription) when calling
this.updateLabelLayout(), and then unsubscribe from that property inside the
class destroy() method (call afterRenderSubscription.unsubscribe() if set) to
prevent the observable from continuing to fire after destroy(); reference the
constructor, threeRendererService.afterRender$, updateLabelLayout(), and
destroy() when applying this change.
Comment on lines
+229
to
+305
| const invDirX = 1 / ray.direction.x | ||
| const invDirY = 1 / ray.direction.y | ||
| const invDirZ = 1 / ray.direction.z | ||
|
|
||
| for (const building of this._buildings) { | ||
| const box: Box3 = building.boundingBox.clone() | ||
| box.min.multiply(this.scales) | ||
| box.max.multiply(this.scales) | ||
| box.translate(boxTranslation) | ||
| const stack: number[] = [0] | ||
| let stackPointer = 1 | ||
|
|
||
| while (stackPointer > 0) { | ||
| const nodeIndex = stack[--stackPointer] | ||
| const offset = nodeIndex * NODE_SIZE | ||
|
|
||
| if (this.rayIntersectsAxisAlignedBoundingBox(ray, box)) { | ||
| const intersectionPoint: Vector3 = ray.intersectBox(box, new Vector3()) | ||
| if (!this.rayIntersectsNode(offset, ray.origin, invDirX, invDirY, invDirZ, leastIntersectedDistance)) { | ||
| continue | ||
| } | ||
|
|
||
| if (intersectionPoint) { | ||
| const intersectionDistance = intersectionPoint.distanceTo(ray.origin) | ||
| const firstIndex = this.bvhNodes[offset + NODE_OFFSETS.first] | ||
|
|
||
| if (intersectionDistance < leastIntersectedDistance) { | ||
| leastIntersectedDistance = intersectionDistance | ||
| intersectedBuilding = building | ||
| } | ||
| if (firstIndex >= 0) { | ||
| const result = this.intersectLeaf(ray, firstIndex, this.bvhNodes[offset + NODE_OFFSETS.count], leastIntersectedDistance) | ||
| if (result.distance < leastIntersectedDistance) { | ||
| leastIntersectedDistance = result.distance | ||
| intersectedBuilding = result.building | ||
| } | ||
| } else { | ||
| stack[stackPointer++] = this.bvhNodes[offset + NODE_OFFSETS.left] | ||
| stack[stackPointer++] = this.bvhNodes[offset + NODE_OFFSETS.right] | ||
| } | ||
| } | ||
|
|
||
| return intersectedBuilding | ||
| } | ||
|
|
||
| private rayIntersectsAxisAlignedBoundingBox(ray: Ray, box: Box3) { | ||
| const tx1 = (box.min.x - ray.origin.x) * (1 / ray.direction.x) | ||
| const tx2 = (box.max.x - ray.origin.x) * (1 / ray.direction.x) | ||
| private intersectLeaf( | ||
| ray: Ray, | ||
| firstIndex: number, | ||
| leafCount: number, | ||
| bestDistance: number | ||
| ): { building: CodeMapBuilding | undefined; distance: number } { | ||
| let building: CodeMapBuilding | undefined | ||
| let distance = bestDistance | ||
|
|
||
| for (let i = firstIndex; i < firstIndex + leafCount; i++) { | ||
| const buildingIndex = this.bvhIndices[i] | ||
| const intersectionPoint = ray.intersectBox(this.scaledBoxes[buildingIndex], this._intersectTarget) | ||
| if (!intersectionPoint) { | ||
| continue | ||
| } | ||
| const d = intersectionPoint.distanceTo(ray.origin) | ||
| if (d < distance) { | ||
| distance = d | ||
| building = this._buildings[buildingIndex] | ||
| } | ||
| } | ||
|
|
||
| return { building, distance } | ||
| } | ||
|
|
||
| let tmin = Math.min(tx1, tx2) | ||
| let tmax = Math.max(tx1, tx2) | ||
| private rayIntersectsNode( | ||
| offset: number, | ||
| origin: Vector3, | ||
| invDirX: number, | ||
| invDirY: number, | ||
| invDirZ: number, | ||
| maxDistance: number | ||
| ): boolean { | ||
| const tMinX = (this.bvhNodes[offset + NODE_OFFSETS.minX] - origin.x) * invDirX | ||
| const tMaxX = (this.bvhNodes[offset + NODE_OFFSETS.maxX] - origin.x) * invDirX | ||
| const tMinY = (this.bvhNodes[offset + NODE_OFFSETS.minY] - origin.y) * invDirY | ||
| const tMaxY = (this.bvhNodes[offset + NODE_OFFSETS.maxY] - origin.y) * invDirY | ||
| const tMinZ = (this.bvhNodes[offset + NODE_OFFSETS.minZ] - origin.z) * invDirZ | ||
| const tMaxZ = (this.bvhNodes[offset + NODE_OFFSETS.maxZ] - origin.z) * invDirZ | ||
|
|
||
| const ty1 = (box.min.y - ray.origin.y) * (1 / ray.direction.y) | ||
| const ty2 = (box.max.y - ray.origin.y) * (1 / ray.direction.y) | ||
| const tEnter = Math.max(Math.min(tMinX, tMaxX), Math.min(tMinY, tMaxY), Math.min(tMinZ, tMaxZ)) | ||
| const tExit = Math.min(Math.max(tMinX, tMaxX), Math.max(tMinY, tMaxY), Math.max(tMinZ, tMaxZ)) | ||
|
|
||
| tmin = Math.max(tmin, Math.min(ty1, ty2)) | ||
| tmax = Math.min(tmax, Math.max(ty1, ty2)) | ||
| return tExit >= tEnter && tExit >= 0 && tEnter < maxDistance | ||
| } |
There was a problem hiding this comment.
Guard parallel ray directions to avoid NaN misses.
When a ray direction component is 0, (min - origin) * Infinity can become NaN (e.g., origin lies on the slab), which makes tEnter/tExit NaN and incorrectly rejects intersections. Handle parallel rays explicitly.
🔧 Proposed fix
- const tMinX = (this.bvhNodes[offset + NODE_OFFSETS.minX] - origin.x) * invDirX
- const tMaxX = (this.bvhNodes[offset + NODE_OFFSETS.maxX] - origin.x) * invDirX
- const tMinY = (this.bvhNodes[offset + NODE_OFFSETS.minY] - origin.y) * invDirY
- const tMaxY = (this.bvhNodes[offset + NODE_OFFSETS.maxY] - origin.y) * invDirY
- const tMinZ = (this.bvhNodes[offset + NODE_OFFSETS.minZ] - origin.z) * invDirZ
- const tMaxZ = (this.bvhNodes[offset + NODE_OFFSETS.maxZ] - origin.z) * invDirZ
+ const minX = this.bvhNodes[offset + NODE_OFFSETS.minX]
+ const maxX = this.bvhNodes[offset + NODE_OFFSETS.maxX]
+ const minY = this.bvhNodes[offset + NODE_OFFSETS.minY]
+ const maxY = this.bvhNodes[offset + NODE_OFFSETS.maxY]
+ const minZ = this.bvhNodes[offset + NODE_OFFSETS.minZ]
+ const maxZ = this.bvhNodes[offset + NODE_OFFSETS.maxZ]
+
+ let tMinX: number
+ let tMaxX: number
+ if (Number.isFinite(invDirX)) {
+ tMinX = (minX - origin.x) * invDirX
+ tMaxX = (maxX - origin.x) * invDirX
+ } else {
+ if (origin.x < minX || origin.x > maxX) {
+ return false
+ }
+ tMinX = Number.NEGATIVE_INFINITY
+ tMaxX = Number.POSITIVE_INFINITY
+ }
+
+ let tMinY: number
+ let tMaxY: number
+ if (Number.isFinite(invDirY)) {
+ tMinY = (minY - origin.y) * invDirY
+ tMaxY = (maxY - origin.y) * invDirY
+ } else {
+ if (origin.y < minY || origin.y > maxY) {
+ return false
+ }
+ tMinY = Number.NEGATIVE_INFINITY
+ tMaxY = Number.POSITIVE_INFINITY
+ }
+
+ let tMinZ: number
+ let tMaxZ: number
+ if (Number.isFinite(invDirZ)) {
+ tMinZ = (minZ - origin.z) * invDirZ
+ tMaxZ = (maxZ - origin.z) * invDirZ
+ } else {
+ if (origin.z < minZ || origin.z > maxZ) {
+ return false
+ }
+ tMinZ = Number.NEGATIVE_INFINITY
+ tMaxZ = Number.POSITIVE_INFINITY
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const invDirX = 1 / ray.direction.x | |
| const invDirY = 1 / ray.direction.y | |
| const invDirZ = 1 / ray.direction.z | |
| for (const building of this._buildings) { | |
| const box: Box3 = building.boundingBox.clone() | |
| box.min.multiply(this.scales) | |
| box.max.multiply(this.scales) | |
| box.translate(boxTranslation) | |
| const stack: number[] = [0] | |
| let stackPointer = 1 | |
| while (stackPointer > 0) { | |
| const nodeIndex = stack[--stackPointer] | |
| const offset = nodeIndex * NODE_SIZE | |
| if (this.rayIntersectsAxisAlignedBoundingBox(ray, box)) { | |
| const intersectionPoint: Vector3 = ray.intersectBox(box, new Vector3()) | |
| if (!this.rayIntersectsNode(offset, ray.origin, invDirX, invDirY, invDirZ, leastIntersectedDistance)) { | |
| continue | |
| } | |
| if (intersectionPoint) { | |
| const intersectionDistance = intersectionPoint.distanceTo(ray.origin) | |
| const firstIndex = this.bvhNodes[offset + NODE_OFFSETS.first] | |
| if (intersectionDistance < leastIntersectedDistance) { | |
| leastIntersectedDistance = intersectionDistance | |
| intersectedBuilding = building | |
| } | |
| if (firstIndex >= 0) { | |
| const result = this.intersectLeaf(ray, firstIndex, this.bvhNodes[offset + NODE_OFFSETS.count], leastIntersectedDistance) | |
| if (result.distance < leastIntersectedDistance) { | |
| leastIntersectedDistance = result.distance | |
| intersectedBuilding = result.building | |
| } | |
| } else { | |
| stack[stackPointer++] = this.bvhNodes[offset + NODE_OFFSETS.left] | |
| stack[stackPointer++] = this.bvhNodes[offset + NODE_OFFSETS.right] | |
| } | |
| } | |
| return intersectedBuilding | |
| } | |
| private rayIntersectsAxisAlignedBoundingBox(ray: Ray, box: Box3) { | |
| const tx1 = (box.min.x - ray.origin.x) * (1 / ray.direction.x) | |
| const tx2 = (box.max.x - ray.origin.x) * (1 / ray.direction.x) | |
| private intersectLeaf( | |
| ray: Ray, | |
| firstIndex: number, | |
| leafCount: number, | |
| bestDistance: number | |
| ): { building: CodeMapBuilding | undefined; distance: number } { | |
| let building: CodeMapBuilding | undefined | |
| let distance = bestDistance | |
| for (let i = firstIndex; i < firstIndex + leafCount; i++) { | |
| const buildingIndex = this.bvhIndices[i] | |
| const intersectionPoint = ray.intersectBox(this.scaledBoxes[buildingIndex], this._intersectTarget) | |
| if (!intersectionPoint) { | |
| continue | |
| } | |
| const d = intersectionPoint.distanceTo(ray.origin) | |
| if (d < distance) { | |
| distance = d | |
| building = this._buildings[buildingIndex] | |
| } | |
| } | |
| return { building, distance } | |
| } | |
| let tmin = Math.min(tx1, tx2) | |
| let tmax = Math.max(tx1, tx2) | |
| private rayIntersectsNode( | |
| offset: number, | |
| origin: Vector3, | |
| invDirX: number, | |
| invDirY: number, | |
| invDirZ: number, | |
| maxDistance: number | |
| ): boolean { | |
| const tMinX = (this.bvhNodes[offset + NODE_OFFSETS.minX] - origin.x) * invDirX | |
| const tMaxX = (this.bvhNodes[offset + NODE_OFFSETS.maxX] - origin.x) * invDirX | |
| const tMinY = (this.bvhNodes[offset + NODE_OFFSETS.minY] - origin.y) * invDirY | |
| const tMaxY = (this.bvhNodes[offset + NODE_OFFSETS.maxY] - origin.y) * invDirY | |
| const tMinZ = (this.bvhNodes[offset + NODE_OFFSETS.minZ] - origin.z) * invDirZ | |
| const tMaxZ = (this.bvhNodes[offset + NODE_OFFSETS.maxZ] - origin.z) * invDirZ | |
| const ty1 = (box.min.y - ray.origin.y) * (1 / ray.direction.y) | |
| const ty2 = (box.max.y - ray.origin.y) * (1 / ray.direction.y) | |
| const tEnter = Math.max(Math.min(tMinX, tMaxX), Math.min(tMinY, tMaxY), Math.min(tMinZ, tMaxZ)) | |
| const tExit = Math.min(Math.max(tMinX, tMaxX), Math.max(tMinY, tMaxY), Math.max(tMinZ, tMaxZ)) | |
| tmin = Math.max(tmin, Math.min(ty1, ty2)) | |
| tmax = Math.min(tmax, Math.max(ty1, ty2)) | |
| return tExit >= tEnter && tExit >= 0 && tEnter < maxDistance | |
| } | |
| private rayIntersectsNode( | |
| offset: number, | |
| origin: Vector3, | |
| invDirX: number, | |
| invDirY: number, | |
| invDirZ: number, | |
| maxDistance: number | |
| ): boolean { | |
| const minX = this.bvhNodes[offset + NODE_OFFSETS.minX] | |
| const maxX = this.bvhNodes[offset + NODE_OFFSETS.maxX] | |
| const minY = this.bvhNodes[offset + NODE_OFFSETS.minY] | |
| const maxY = this.bvhNodes[offset + NODE_OFFSETS.maxY] | |
| const minZ = this.bvhNodes[offset + NODE_OFFSETS.minZ] | |
| const maxZ = this.bvhNodes[offset + NODE_OFFSETS.maxZ] | |
| let tMinX: number | |
| let tMaxX: number | |
| if (Number.isFinite(invDirX)) { | |
| tMinX = (minX - origin.x) * invDirX | |
| tMaxX = (maxX - origin.x) * invDirX | |
| } else { | |
| if (origin.x < minX || origin.x > maxX) { | |
| return false | |
| } | |
| tMinX = Number.NEGATIVE_INFINITY | |
| tMaxX = Number.POSITIVE_INFINITY | |
| } | |
| let tMinY: number | |
| let tMaxY: number | |
| if (Number.isFinite(invDirY)) { | |
| tMinY = (minY - origin.y) * invDirY | |
| tMaxY = (maxY - origin.y) * invDirY | |
| } else { | |
| if (origin.y < minY || origin.y > maxY) { | |
| return false | |
| } | |
| tMinY = Number.NEGATIVE_INFINITY | |
| tMaxY = Number.POSITIVE_INFINITY | |
| } | |
| let tMinZ: number | |
| let tMaxZ: number | |
| if (Number.isFinite(invDirZ)) { | |
| tMinZ = (minZ - origin.z) * invDirZ | |
| tMaxZ = (maxZ - origin.z) * invDirZ | |
| } else { | |
| if (origin.z < minZ || origin.z > maxZ) { | |
| return false | |
| } | |
| tMinZ = Number.NEGATIVE_INFINITY | |
| tMaxZ = Number.POSITIVE_INFINITY | |
| } | |
| const tEnter = Math.max(Math.min(tMinX, tMaxX), Math.min(tMinY, tMaxY), Math.min(tMinZ, tMaxZ)) | |
| const tExit = Math.min(Math.max(tMinX, tMaxX), Math.max(tMinY, tMaxY), Math.max(tMinZ, tMaxZ)) | |
| return tExit >= tEnter && tExit >= 0 && tEnter < maxDistance | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@visualization/app/codeCharta/ui/codeMap/rendering/codeMapGeometricDescription.ts`
around lines 229 - 305, rayIntersectsNode can produce NaN when a ray direction
component is 0; fix by guarding parallel axes: in rayIntersectsNode (using
NODE_OFFSETS.minX/maxX etc.) first detect if ray.direction.x/y/z === 0 (or
invDirX/Y/Z is Infinite) and if so return false immediately when origin.x/y/z is
outside the node slab (origin < min || origin > max); otherwise skip computing
tMin/tMax for that axis or compute them as now when direction is non-zero.
Update rayIntersectsNode to perform these per-axis checks before computing
tEnter/tExit so parallel rays that lie inside the slab continue testing and
those outside are rejected.
Comment on lines
128
to
153
| highlightBuilding( | ||
| highlightedBuildings: CodeMapBuilding[], | ||
| highlightedBuildingIds: Set<number>, | ||
| primaryBuilding: CodeMapBuilding, | ||
| selected: CodeMapBuilding, | ||
| state: CcState, | ||
| constantHighlight: Map<number, CodeMapBuilding> | ||
| ) { | ||
| const highlightBuildingMap = TreeMapHelper.buildingArrayToMap(highlightedBuildings) | ||
| for (const building of this.mapGeomDesc.buildings) { | ||
| if (!this.isBuildingSelected(selected, building)) { | ||
| if (highlightBuildingMap.get(building.id) || constantHighlight.get(building.id)) { | ||
| building.decreaseLightness(CodeMapMesh.LIGHTNESS_INCREASE) | ||
| } else { | ||
| this.adjustSurroundingBuildingColors(highlightedBuildings, building, state) | ||
| } | ||
| this.setVertexColor(building.id, building.getColorVector(), building.getDeltaColorVector()) | ||
| } | ||
| const { isPresentationMode } = state.appSettings | ||
|
|
||
| // force full re-render when presentation mode is toggled mid-hover | ||
| if (this._prevIsPresentationMode !== null && this._prevIsPresentationMode !== isPresentationMode) { | ||
| this._prevHighlightedIds = null | ||
| } | ||
|
|
||
| const prev = this._prevHighlightedIds | ||
|
|
||
| if (prev && !isPresentationMode) { | ||
| this.updateDimmedBuildings(prev, highlightedBuildingIds, constantHighlight, selected) | ||
| this.updateHighlightedBuildings(prev, highlightedBuildingIds, selected) | ||
| } else { | ||
| this.updateAllBuildings(highlightedBuildingIds, constantHighlight, selected, isPresentationMode, primaryBuilding, state) | ||
| } | ||
|
|
||
| this._prevHighlightedIds = new Set(highlightedBuildingIds) | ||
| this._prevIsPresentationMode = isPresentationMode | ||
| this.updateVertices() |
There was a problem hiding this comment.
Incremental highlight path ignores constantHighlight changes.
_prevHighlightedIds only tracks the hover set. If constantHighlight changes while highlighted IDs stay the same, the incremental path never recolors those newly constant-highlighted buildings.
🛠️ Suggested fix (track constant-highlight keys)
- private _prevHighlightedIds: Set<number> | null = null
+ private _prevHighlightedIds: Set<number> | null = null
+ private _prevConstantHighlightIds: Set<number> | null = null
@@
- const prev = this._prevHighlightedIds
+ const currentConstantIds = new Set(constantHighlight.keys())
+ const constantChanged =
+ !this._prevConstantHighlightIds ||
+ currentConstantIds.size !== this._prevConstantHighlightIds.size ||
+ [...currentConstantIds].some(id => !this._prevConstantHighlightIds.has(id))
+ if (constantChanged) {
+ this._prevHighlightedIds = null
+ }
+
+ const prev = this._prevHighlightedIds
@@
- this._prevHighlightedIds = new Set(highlightedBuildingIds)
+ this._prevHighlightedIds = new Set(highlightedBuildingIds)
+ this._prevConstantHighlightIds = currentConstantIds🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@visualization/app/codeCharta/ui/codeMap/rendering/codeMapMesh.ts` around
lines 128 - 153, In highlightBuilding, the incremental path ignores changes to
constantHighlight because only _prevHighlightedIds is tracked; detect changes to
constantHighlight by introducing a stored _prevConstantHighlightKeys Set
(derived from constantHighlight.keys()) and compare it to the current keys (fast
Set size and membership check); if they differ (or when _prevIsPresentationMode
toggles) set _prevHighlightedIds = null to force the full update branch, and at
the end assign this._prevConstantHighlightKeys = new
Set(constantHighlight.keys()) alongside the existing this._prevHighlightedIds
and this._prevIsPresentationMode updates so newly constant-highlighted buildings
get recolored.
- Reset suppressedLabel on clearLabels/clearTemporaryLabel to prevent stale references after map re-renders - Skip suppressed (invisible) labels in collision detection so they don't push visible labels unnecessarily - Add afterEach in codeMapBuilding.spec.ts to restore ColorConverter after tests and prevent within-file mock leakage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…porter Omit --output-file flag when empty, fix US/UK spelling in docs, add language tag to code fence. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
{Meaningful title}
Please read the CONTRIBUTING.md before opening a PR.
Closes: #
Description
Descriptive pull request text, answering:
Definition of Done
A PR is only ready for merge once all the following acceptance criteria are fulfilled:
Screenshots or gifs
Summary by CodeRabbit
New Features
Performance
Bug Fixes
Chores