Tech/migrate state util into features#4504
Open
ChristianHuehn wants to merge 15 commits into
Open
Conversation
Sole consumer is navBar/services/readFiles.ts; relocate the whole gameObjectsParser module (importer, validator, schema, mocks, specs) into features/navBar/util/. Verified by dependency-cruiser (0 errors) and jest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TruncateTextPipe had no template usage and no class references outside its own spec. Removing the orphan. dependency-cruiser clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-verified every fan-out move candidate against the real import graph with dependency-cruiser + jest. Most util predictions were wrong (grep blind spot for intra-util relative imports / state-core consumers / app.config effect registration). Only gameObjectsParser move + dead truncateText pipe deletion held up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pHelper cycle buildingArrayToMap had no production callers (only its own test) and was the sole reason util/algorithm/treeMapHelper imported CodeMapBuilding from the codeMap feature. Removing it eliminates the util->feature inversion and 30 of the 126 no-circular warnings (126 -> 96). CodeMapBuilding stays in the codeMap feature where it belongs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ks into codeMap The 5 CodeMapBuilding test fixtures (CODE_MAP_BUILDING*, CONSTANT_HIGHLIGHT) lived in the shared util/dataMocks god-file but are consumed only by codeMap specs, forcing util/ to import up into the codeMap feature. Move them to a codeMap-owned features/codeMap/rendering/codeMapBuilding.mocks.ts and repoint the 3 codeMap specs. Removes the last util->codeMap inversion; CodeMapBuilding stays in the feature. dependency-cruiser 0 errors, 143 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ks/ root folder dataMocks is a ~2500-line cross-layer test-fixture file that was sitting in util/ and obscuring real util problems. Move it to app/codeCharta/mocks/ (a sibling of util/), repoint all 57 importers, and fix its one same-dir import (./codeMapHelper -> ../util/codeMapHelper). Pure relocation: dependency-cruiser 0 errors, tests green across every import depth. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nalysis dataMocks (now in mocks/) and *.mocks.ts files are test fixtures, not production architecture, yet dataMocks alone added ~67 edges as a hub node that obscured real structure in the graph. Exclude mocks/ and *.mocks.ts so the dependency graph and boundary rules reflect production code only (fixtures are imported solely by tests). 4622 -> 4555 dependencies cruised. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mockHelper.ts had zero references anywhere (production or test). Orphan, like the truncateText pipe — removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ture fileParser + fileValidator are consumed only by loadFile in production, so relocate them to features/loadFile/util/. The one cross-feature consumer (the globalSettings spec using getNameDataPair) now imports it via loadFile's public facade, which is the depcruise-legal way to cross features. generatedSchema.json stays in util/ (generated artifact) and is imported feature->util. mockHelper already removed. dependency-cruiser 0 errors, 74 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
codeCharta.model (the leaf domain types) was reaching up into two higher layers, forming type-only cycles: - NameDataPair.content needed ExportCCFile -> codeCharta.model <-> codeCharta.api.model - CcState.files needed FileState -> codeCharta.model <-> model/files/files Fix by making the leaf stop importing upward: - Move NameDataPair into codeCharta.api.model (next to ExportCCFile); repoint its 8 type importers. - Move FileState/FileSelectionState into codeCharta.model; model/files/files re-exports them (natural files->model direction), so its 47 importers are untouched. Both edges removed: dependency-cruiser warnings 96 -> 94, 0 errors, tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fileValidator move left a 4-level relative path pointing at codeCharta/ instead of features/loadFile/. e2e files aren't type-checked by jest and depcruise ignores unresolvable modules, so tsc surfaced it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ehind a barrel Step 1 of incrementally breaking up the 363-line model god-file. Definitions now live in model/domain.model.ts (leaf .cc.json domain types) and model/state.model.ts (the ngrx state tree, which imports the domain one-way). codeCharta.model.ts becomes a pure barrel re-exporting both, so all 416 existing importers keep working unchanged. Consumers can migrate to the specific files over time, then the barrel can be dropped. Verified: dependency-cruiser 0 errors / 94 warnings / no new cycles, tsc 0 errors across all importers, model tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+ pin the wire DTO loadFile now owns cc.json ingestion: moved urlExtractor in, and extracted the wire-parsing functions (getCCFile, getCCFileAndDecorateFileChecksum) out of the mixed util/fileHelper into loadFile/util/ccFileHelper. The cross-feature callers reach them through loadFile's facade (navBar upload, globalSettings url-reset). util/fileHelper keeps only getSelectedFilesSize (a FileState helper, no wire format). fileDownloader (export side) stays shared. Adds a dependency-cruiser rule 'wire-dto-only-in-serialization-boundary' that confines codeCharta.api.model to the loadFile feature, the navBar gameObjects importer, and util/fileDownloader — so rendering/state/UI can't couple to the wire format and the upcoming cc.json 2.0 change stays contained to the converter. tsc 0 errors, dependency-cruiser 0 errors / 94 warnings, tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds planning docs and refactors shared models, load-file helpers, fixture imports, and tree-map/file-helper utilities while tightening dependency-cruiser rules around the API model boundary. ChangesPlanning Documents
Code and Test Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (5)
visualization/app/codeCharta/util/fileHelper.spec.ts (1)
8-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Arrange/Act/Assert comments to this spec.
The test name is fine, but the body no longer follows the required Arrange-Act-Assert structure with comments.
Suggested update
it("should sum up sizes of selected files", () => { + // Arrange const file1 = { fileMeta: { fileName: "file1", fileChecksum: "invalid-md5-checksum-1", projectName: "Sample Project", @@ const severalFiles: FileState[] = [ { file: file1, selectedAs: FileSelectionState.Partial }, @@ } ] - expect(getSelectedFilesSize(severalFiles)).toBe(20) + // Act + const totalSize = getSelectedFilesSize(severalFiles) + + // Assert + expect(totalSize).toBe(20) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@visualization/app/codeCharta/util/fileHelper.spec.ts` around lines 8 - 54, The spec for getSelectedFilesSize no longer shows the required Arrange-Act-Assert structure. Update the test body in fileHelper.spec.ts by adding explicit Arrange, Act, and Assert comments around the existing setup of file1/file2/unselectedFile and severalFiles, the call to getSelectedFilesSize, and the expect assertion, keeping the current test name and behavior unchanged.Source: Coding guidelines
visualization/app/codeCharta/features/loadFile/util/ccFileHelper.ts (1)
48-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
potentiallyUpdateBlacklistTypesmutates the caller's input in place.
entry.typeis reassigned directly on the items of the passed-in array, which isfileContent.blacklistfrom the originalNameDataPair.content. This produces a side effect on the source object that survives outside the function. Prefer returning a new array.As per coding guidelines: "Prefer immutable data structures, especially in the model layer."♻️ Non-mutating variant
-function potentiallyUpdateBlacklistTypes(blacklist): BlacklistItem[] { - for (const entry of blacklist) { - if (entry.type === ExportBlacklistType.hide) { - entry.type = "flatten" - } - } - return blacklist -} +function potentiallyUpdateBlacklistTypes(blacklist): BlacklistItem[] { + return blacklist.map(entry => + entry.type === ExportBlacklistType.hide ? { ...entry, type: "flatten" } : entry + ) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@visualization/app/codeCharta/features/loadFile/util/ccFileHelper.ts` around lines 48 - 55, `potentiallyUpdateBlacklistTypes` is mutating the input blacklist array and its items in place, which leaks changes back into `NameDataPair.content.fileContent.blacklist`. Update the function to return a new `BlacklistItem[]` instead of modifying the passed-in array, preserving immutability. Use the `potentiallyUpdateBlacklistTypes` helper in `ccFileHelper.ts` to map each entry to a copied object and change `type` only on the copy when `ExportBlacklistType.hide` is encountered.Source: Coding guidelines
visualization/.dependency-cruiser.js (1)
137-140: 📐 Maintainability & Code Quality | 🔵 TrivialArchitectural improvement: Keep mocks in the dependency graph for enforceability
While the current codebase has no production files importing mocks (verified by the exclusion of
*.mocks.tsfiles), keeping themocks/directory and*.mocks.tsfiles entirely excluded from the dependency graph is risky for future maintainability. This configuration preventsdependency-cruiserfrom flagging any accidental future imports of mocks into production code, as these paths are invisible to the rules.Recommendation:
Remove the exclusion ofapp/codeCharta/mocks/and*.mocks.tsfrom thepatharray invisualization/.dependency-cruiser.js(lines 137-140). Instead, explicitly define a rule that forbids non-test production files from importing these mocks. This ensures the graph remains complete and that any accidental usage is caught immediately rather than silently ignored.// .dependency-cruiser.js // Instead of excluding these paths, define a rule like: // { // name: "no-mock-imports", // severity: "error", // comment: "Do not import mocks in production code. Only test files should import from mocks.", // path: ["^app/codeCharta/"], // All app code // to: ["(mocks/|\\.mocks\\.ts$)"], // Targeting mocks // condition: ["not_path: \\.(spec|e2e|test|po)\\.ts$"] // Exclude test files // }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@visualization/.dependency-cruiser.js` around lines 137 - 140, The dependency-cruiser config is hiding mocks from the graph by excluding app/codeCharta/mocks/ and *.mocks.ts in the path array, which prevents future mock imports from being enforced. Update visualization/.dependency-cruiser.js by removing those exclusions and instead add an explicit no-mock-imports rule that matches production code and forbids imports into the mocks folder or *.mocks.ts files, while allowing test files such as spec/test/e2e/po to keep using them. Keep the existing node_modules exception unchanged and ensure the new rule is the place where mock usage is blocked.visualization/app/codeCharta/features/globalSettings/components/globalConfigurationDialog/confirmResetMapDialog/confirmResetMapDialog.spec.ts (1)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnify the facade import path and consolidate into one statement.
Lines 8/11 use
../../../../loadFile/facadewhile lines 9/10 use../../../../../features/loadFile/facade; both resolve to the same module.jest.mock("../../../../loadFile/facade")(Line 21) still mocks the same resolved module, so this works, but the divergent spellings are confusing. Merge all four into a single import using one consistent path.♻️ Proposed consolidation
-import { getNameDataPair } from "../../../../loadFile/facade" -import { LoadFileService } from "../../../../../features/loadFile/facade" -import { LoadInitialFileService, sampleFile1, sampleFile2 } from "../../../../../features/loadFile/facade" -import { UrlExtractor } from "../../../../loadFile/facade" +import { getNameDataPair, LoadFileService, LoadInitialFileService, sampleFile1, sampleFile2, UrlExtractor } from "../../../../loadFile/facade"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@visualization/app/codeCharta/features/globalSettings/components/globalConfigurationDialog/confirmResetMapDialog/confirmResetMapDialog.spec.ts` around lines 8 - 11, The spec has duplicate imports from the same facade module using two different relative paths, which is confusing and should be consolidated. Update the imports in confirmResetMapDialog.spec.ts to use one consistent path for all symbols, and merge getNameDataPair, LoadFileService, LoadInitialFileService, sampleFile1, sampleFile2, and UrlExtractor into a single import statement that matches the existing jest.mock target.visualization/app/codeCharta/features/loadFile/util/urlExtractor.ts (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the two imports from
codeCharta.api.model.Lines 3 and 5 both pull from
../../../codeCharta.api.model; merge them into a single import statement for clarity.♻️ Proposed consolidation
-import { NameDataPair } from "../../../codeCharta.api.model" import { getCCFileAndDecorateFileChecksum } from "./ccFileHelper" -import { ExportCCFile, ExportWrappedCCFile } from "../../../codeCharta.api.model" +import { NameDataPair, ExportCCFile, ExportWrappedCCFile } from "../../../codeCharta.api.model"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@visualization/app/codeCharta/features/loadFile/util/urlExtractor.ts` around lines 3 - 5, Consolidate the duplicate imports from codeCharta.api.model in urlExtractor.ts by merging the NameDataPair, ExportCCFile, and ExportWrappedCCFile symbols into a single import statement; keep the existing ccFileHelper import unchanged and update the top-level import block so there is only one reference to ../../../codeCharta.api.model.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@visualization/app/codeCharta/codeCharta.api.model.ts`:
- Around line 3-7: NameDataPair.content is typed as non-nullable, but
getCCFileAndDecorateFileChecksum can return null and that value is being passed
through unchanged. Update the callers in uploadFiles.service and urlExtractor to
guard against null before constructing or assigning NameDataPair, or, if null is
a valid state, change NameDataPair.content in codeCharta.api.model.ts to reflect
ExportCCFile | null (or optional) and adjust downstream consumers accordingly.
Use the NameDataPair interface and getCCFileAndDecorateFileChecksum as the key
symbols when making the change.
In `@visualization/app/codeCharta/features/loadFile/util/ccFileHelper.spec.ts`:
- Around line 78-85: The test case in ccFileHelper.spec.ts is duplicated and
misleading because it uses the same description as an earlier test while
asserting on fileSettings.attributeDescriptors; rename the test to a unique,
descriptive title that matches the assertion so it clearly reflects the behavior
being verified in getCCFile and stays distinct from the other “should return
attributeTypes if nodes exist” case.
---
Nitpick comments:
In `@visualization/.dependency-cruiser.js`:
- Around line 137-140: The dependency-cruiser config is hiding mocks from the
graph by excluding app/codeCharta/mocks/ and *.mocks.ts in the path array, which
prevents future mock imports from being enforced. Update
visualization/.dependency-cruiser.js by removing those exclusions and instead
add an explicit no-mock-imports rule that matches production code and forbids
imports into the mocks folder or *.mocks.ts files, while allowing test files
such as spec/test/e2e/po to keep using them. Keep the existing node_modules
exception unchanged and ensure the new rule is the place where mock usage is
blocked.
In
`@visualization/app/codeCharta/features/globalSettings/components/globalConfigurationDialog/confirmResetMapDialog/confirmResetMapDialog.spec.ts`:
- Around line 8-11: The spec has duplicate imports from the same facade module
using two different relative paths, which is confusing and should be
consolidated. Update the imports in confirmResetMapDialog.spec.ts to use one
consistent path for all symbols, and merge getNameDataPair, LoadFileService,
LoadInitialFileService, sampleFile1, sampleFile2, and UrlExtractor into a single
import statement that matches the existing jest.mock target.
In `@visualization/app/codeCharta/features/loadFile/util/ccFileHelper.ts`:
- Around line 48-55: `potentiallyUpdateBlacklistTypes` is mutating the input
blacklist array and its items in place, which leaks changes back into
`NameDataPair.content.fileContent.blacklist`. Update the function to return a
new `BlacklistItem[]` instead of modifying the passed-in array, preserving
immutability. Use the `potentiallyUpdateBlacklistTypes` helper in
`ccFileHelper.ts` to map each entry to a copied object and change `type` only on
the copy when `ExportBlacklistType.hide` is encountered.
In `@visualization/app/codeCharta/features/loadFile/util/urlExtractor.ts`:
- Around line 3-5: Consolidate the duplicate imports from codeCharta.api.model
in urlExtractor.ts by merging the NameDataPair, ExportCCFile, and
ExportWrappedCCFile symbols into a single import statement; keep the existing
ccFileHelper import unchanged and update the top-level import block so there is
only one reference to ../../../codeCharta.api.model.
In `@visualization/app/codeCharta/util/fileHelper.spec.ts`:
- Around line 8-54: The spec for getSelectedFilesSize no longer shows the
required Arrange-Act-Assert structure. Update the test body in
fileHelper.spec.ts by adding explicit Arrange, Act, and Assert comments around
the existing setup of file1/file2/unselectedFile and severalFiles, the call to
getSelectedFilesSize, and the expect assertion, keeping the current test name
and behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e0df91c-0da5-45e8-a49d-64510b350cec
⛔ Files ignored due to path filters (2)
visualization/app/codeCharta/features/loadFile/util/__snapshots__/ccFileHelper.spec.ts.snapis excluded by!**/*.snapvisualization/app/codeCharta/features/navBar/util/gameObjectsParser/__snapshots__/gameObjectsImporter.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (94)
plans/2026-06-24-go-zoneless.mdplans/2026-06-24-migrate-remaining-ui-to-features.mdplans/2026-06-24-on-map-layout-switcher.mdplans/2026-06-25-state-util-into-features.mdvisualization/.dependency-cruiser.jsvisualization/app/codeCharta/codeCharta.api.model.tsvisualization/app/codeCharta/codeCharta.model.tsvisualization/app/codeCharta/features/3dPrint/components/export3DMapDialog/export3DMapDialog.component.spec.tsvisualization/app/codeCharta/features/codeMap/arrow/codeMap.arrow.service.spec.tsvisualization/app/codeCharta/features/codeMap/codeMap.mouseEvent.service.spec.tsvisualization/app/codeCharta/features/codeMap/codeMap.render.service.spec.tsvisualization/app/codeCharta/features/codeMap/rendering/codeMapBuilding.mocks.tsvisualization/app/codeCharta/features/codeMap/rendering/codeMapBuilding.spec.tsvisualization/app/codeCharta/features/codeMap/rendering/codeMapMesh.spec.tsvisualization/app/codeCharta/features/codeMap/rendering/geometryGenerator.spec.tsvisualization/app/codeCharta/features/codeMap/threeViewer/threeSceneService.spec.tsvisualization/app/codeCharta/features/fileExtensionBar/selectors/hoveredNodeMetricDistribution.selector.spec.tsvisualization/app/codeCharta/features/globalSettings/components/globalConfigurationDialog/confirmResetMapDialog/confirmResetMapDialog.component.tsvisualization/app/codeCharta/features/globalSettings/components/globalConfigurationDialog/confirmResetMapDialog/confirmResetMapDialog.spec.tsvisualization/app/codeCharta/features/globalSettings/components/globalConfigurationDialog/resetMapButton/resetMapButton.component.spec.tsvisualization/app/codeCharta/features/labelSettings/components/labelSettingsPanel/labelSettingsPanel.component.spec.tsvisualization/app/codeCharta/features/labelSettings/services/labelCollision.service.spec.tsvisualization/app/codeCharta/features/loadFile/facade.tsvisualization/app/codeCharta/features/loadFile/services/loadFile.service.spec.tsvisualization/app/codeCharta/features/loadFile/services/loadFile.service.tsvisualization/app/codeCharta/features/loadFile/services/loadFilesValidationToErrorDialog.spec.tsvisualization/app/codeCharta/features/loadFile/services/loadFilesValidationToErrorDialog.tsvisualization/app/codeCharta/features/loadFile/services/loadInitialFile.service.spec.tsvisualization/app/codeCharta/features/loadFile/services/loadInitialFile.service.tsvisualization/app/codeCharta/features/loadFile/util/ccFileHelper.spec.tsvisualization/app/codeCharta/features/loadFile/util/ccFileHelper.tsvisualization/app/codeCharta/features/loadFile/util/fileParser.tsvisualization/app/codeCharta/features/loadFile/util/fileValidator.spec.tsvisualization/app/codeCharta/features/loadFile/util/fileValidator.tsvisualization/app/codeCharta/features/loadFile/util/urlExtractor.spec.tsvisualization/app/codeCharta/features/loadFile/util/urlExtractor.tsvisualization/app/codeCharta/features/metricsBar/services/nodeSelection.service.spec.tsvisualization/app/codeCharta/features/navBar/components/deltaSelector/deltaSelector.component.spec.tsvisualization/app/codeCharta/features/navBar/components/mapSelector/mapSelector.component.spec.tsvisualization/app/codeCharta/features/navBar/components/navBarFolderButton/navBarFolderButton.e2e.tsvisualization/app/codeCharta/features/navBar/services/fileSelectionMode.service.spec.tsvisualization/app/codeCharta/features/navBar/services/readFiles.tsvisualization/app/codeCharta/features/navBar/services/uploadFiles.service.spec.tsvisualization/app/codeCharta/features/navBar/services/uploadFiles.service.tsvisualization/app/codeCharta/features/navBar/util/gameObjectsParser/gameObjectsImporter.spec.tsvisualization/app/codeCharta/features/navBar/util/gameObjectsParser/gameObjectsImporter.tsvisualization/app/codeCharta/features/navBar/util/gameObjectsParser/gameObjectsMocks.tsvisualization/app/codeCharta/features/navBar/util/gameObjectsParser/gameObjectsSchema.jsonvisualization/app/codeCharta/features/navBar/util/gameObjectsParser/gameObjectsValidator.spec.tsvisualization/app/codeCharta/features/navBar/util/gameObjectsParser/gameObjectsValidator.tsvisualization/app/codeCharta/features/sidebarExplorer/selectors/sortNode.spec.tsvisualization/app/codeCharta/features/viewCubeToolbox/components/viewCubeToolbox/viewCubeToolbox.component.spec.tsvisualization/app/codeCharta/mocks/dataMocks.tsvisualization/app/codeCharta/model/domain.model.tsvisualization/app/codeCharta/model/files/files.helper.spec.tsvisualization/app/codeCharta/model/files/files.tsvisualization/app/codeCharta/model/state.model.tsvisualization/app/codeCharta/state/effects/addBlacklistItemsIfNotResultsInEmptyMap/addBlacklistItemsIfNotResultsInEmptyMap.effect.spec.tsvisualization/app/codeCharta/state/effects/resetColorRange/resetColorRange.effect.spec.tsvisualization/app/codeCharta/state/effects/unfocusNodes/unfocusNodes.effect.spec.tsvisualization/app/codeCharta/state/effects/updateFileSettings/updateFileSettings.effect.spec.tsvisualization/app/codeCharta/state/effects/updateFileSettings/utils/blacklist.merger.spec.tsvisualization/app/codeCharta/state/effects/updateFileSettings/utils/edges.merger.spec.tsvisualization/app/codeCharta/state/effects/updateFileSettings/utils/markedPackages.merger.spec.tsvisualization/app/codeCharta/state/effects/updateQueryParameters/updateQueryParameters.spec.tsvisualization/app/codeCharta/state/selectors/accumulatedData/idToNode.selector.spec.tsvisualization/app/codeCharta/state/selectors/accumulatedData/metricData/edgeMetricData.calculator.spec.tsvisualization/app/codeCharta/state/selectors/accumulatedData/metricData/nodeMetricData.calculator.spec.tsvisualization/app/codeCharta/state/selectors/accumulatedData/metricData/sortedNodeEdgeMetricsMap.selector.spec.tsvisualization/app/codeCharta/state/selectors/amountOfBuildingsWithSelectedEdgeMetric/amountOfBuildingsWithSelectedEdgeMetric.selector.spec.tsvisualization/app/codeCharta/state/selectors/areMultipleMapsVisible.selector.spec.tsvisualization/app/codeCharta/state/selectors/searchedNodes/getNodesByGitignorePath.spec.tsvisualization/app/codeCharta/state/selectors/visibleFileStates/visibleFileStates.selector.spec.tsvisualization/app/codeCharta/state/store/fileSettings/attributeTypes/attributeTypes.reducer.spec.tsvisualization/app/codeCharta/state/store/fileSettings/edges/edges.reducer.spec.tsvisualization/app/codeCharta/state/store/fileSettings/markedPackages/markedPackages.reducer.spec.tsvisualization/app/codeCharta/state/store/files/files.reducer.spec.tsvisualization/app/codeCharta/state/store/util/getPartialDefaultState.spec.tsvisualization/app/codeCharta/util/algorithm/streetLayout/horizontalStreet.spec.tsvisualization/app/codeCharta/util/algorithm/streetLayout/house.spec.tsvisualization/app/codeCharta/util/algorithm/streetLayout/streetLayoutGenerator.spec.tsvisualization/app/codeCharta/util/algorithm/treeMapLayout/treeMapGenerator.spec.tsvisualization/app/codeCharta/util/algorithm/treeMapLayout/treeMapHelper.spec.tsvisualization/app/codeCharta/util/algorithm/treeMapLayout/treeMapHelper.tsvisualization/app/codeCharta/util/codeMapHelper.spec.tsvisualization/app/codeCharta/util/deltaGenerator.spec.tsvisualization/app/codeCharta/util/fileDownloader.spec.tsvisualization/app/codeCharta/util/fileExtension/fileExtensionCalculator.spec.tsvisualization/app/codeCharta/util/fileHelper.spec.tsvisualization/app/codeCharta/util/fileHelper.tsvisualization/app/codeCharta/util/mockHelper.tsvisualization/app/codeCharta/util/nodeDecorator.spec.tsvisualization/app/codeCharta/util/pipes/truncateText.pipe.tsvisualization/app/codeCharta/util/pipes/truncateText.spec.ts
💤 Files with no reviewable changes (5)
- visualization/app/codeCharta/util/pipes/truncateText.pipe.ts
- visualization/app/codeCharta/util/pipes/truncateText.spec.ts
- visualization/app/codeCharta/util/mockHelper.ts
- visualization/app/codeCharta/util/algorithm/treeMapLayout/treeMapHelper.ts
- visualization/app/codeCharta/util/fileHelper.ts
Comment on lines
+3
to
+7
| export interface NameDataPair { | ||
| fileName: string | ||
| fileSize: number | ||
| content: ExportCCFile | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check strictNullChecks/strict config and confirm producer return type
fd -t f 'tsconfig*.json' visualization | xargs -I{} sh -c 'echo "== {} =="; cat {}'
ast-grep run --pattern 'export function getCCFileAndDecorateFileChecksum($_): $RET { $$$ }' --lang typescript visualization/app/codeCharta/features/loadFile/util/ccFileHelper.tsRepository: MaibornWolff/codecharta
Length of output: 3711
Handle nullable return from getCCFileAndDecorateFileChecksum before assigning to content.
The producer getCCFileAndDecorateFileChecksum returns ExportCCFile | null (see features/loadFile/util/ccFileHelper.ts line 57). Code in features/navBar/services/uploadFiles.service.ts (line 55) and features/loadFile/util/urlExtractor.ts (line 70) assigns this result directly to NameDataPair.content, which is typed as non-nullable ExportCCFile. Although strictNullChecks is disabled in visualization/tsconfig.json, this allows null to flow into downstream consumers that assume a valid object, risking runtime crashes (e.g., accessing content.nodes).
Validate the result before assignment or update the interface to reflect the nullable state:
- Add a null check before creating or assigning to
NameDataPair. - If
nullis a valid upstream state, updateNameDataPair.contenttocontent?: ExportCCFile | null.
Relevant snippets
// File: visualization/app/codeCharta/features/navBar/services/uploadFiles.service.ts:55
// Direct assignment of nullable result to non-nullable property
content: getCCFileAndDecorateFileChecksum(content)
// File: visualization/app/codeCharta/features/loadFile/util/urlExtractor.ts:70
// Explicit cast to non-nullable type
const content: ExportCCFile = getCCFileAndDecorateFileChecksum(responseData)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@visualization/app/codeCharta/codeCharta.api.model.ts` around lines 3 - 7,
NameDataPair.content is typed as non-nullable, but
getCCFileAndDecorateFileChecksum can return null and that value is being passed
through unchanged. Update the callers in uploadFiles.service and urlExtractor to
guard against null before constructing or assigning NameDataPair, or, if null is
a valid state, change NameDataPair.content in codeCharta.api.model.ts to reflect
ExportCCFile | null (or optional) and adjust downstream consumers accordingly.
Use the NameDataPair interface and getCCFileAndDecorateFileChecksum as the key
symbols when making the change.
Comment on lines
+78
to
+85
| it("should return attributeTypes if nodes exist", () => { | ||
| fileContent.attributeDescriptors = TEST_ATTRIBUTE_DESCRIPTORS_HALF_FILLED | ||
|
|
||
| const nameDataPair: NameDataPair = { content: fileContent, fileName: "fileName", fileSize: 30 } | ||
| const result = getCCFile(nameDataPair) | ||
|
|
||
| expect(result.settings.fileSettings.attributeDescriptors).toEqual(TEST_ATTRIBUTE_DESCRIPTORS_HALF_FILLED) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Duplicate/incorrect test description.
This test reuses the exact name of the test at Line 63 but actually asserts on attributeDescriptors. Rename for clarity and uniqueness.
📝 Suggested rename
- it("should return attributeTypes if nodes exist", () => {
+ it("should return attributeDescriptors when provided", () => {
fileContent.attributeDescriptors = TEST_ATTRIBUTE_DESCRIPTORS_HALF_FILLED📝 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
| it("should return attributeTypes if nodes exist", () => { | |
| fileContent.attributeDescriptors = TEST_ATTRIBUTE_DESCRIPTORS_HALF_FILLED | |
| const nameDataPair: NameDataPair = { content: fileContent, fileName: "fileName", fileSize: 30 } | |
| const result = getCCFile(nameDataPair) | |
| expect(result.settings.fileSettings.attributeDescriptors).toEqual(TEST_ATTRIBUTE_DESCRIPTORS_HALF_FILLED) | |
| }) | |
| it("should return attributeDescriptors when provided", () => { | |
| fileContent.attributeDescriptors = TEST_ATTRIBUTE_DESCRIPTORS_HALF_FILLED | |
| const nameDataPair: NameDataPair = { content: fileContent, fileName: "fileName", fileSize: 30 } | |
| const result = getCCFile(nameDataPair) | |
| expect(result.settings.fileSettings.attributeDescriptors).toEqual(TEST_ATTRIBUTE_DESCRIPTORS_HALF_FILLED) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@visualization/app/codeCharta/features/loadFile/util/ccFileHelper.spec.ts`
around lines 78 - 85, The test case in ccFileHelper.spec.ts is duplicated and
misleading because it uses the same description as an earlier test while
asserting on fileSettings.attributeDescriptors; rename the test to a unique,
descriptive title that matches the assertion so it clearly reflects the behavior
being verified in getCCFile and stays distinct from the other “should return
attributeTypes if nodes exist” case.
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
Documentation
Refactor
Tests