[Coding-dojo] AI-driven refactor#65
Conversation
BundleMonNo change in files bundle size Groups updated (1)
Final result: ✅ View report in BundleMon website ➡️ |
xurxodev
left a comment
There was a problem hiding this comment.
Thanks @nshandra
This refactor addresses the two dependency-rule violations called out in the PR description: it removes ImportExportClient from the use-case constructors and
drops the browser File type from their signatures. Parsing/serialization is moved to a new src/webapp/services/ layer. However, the PR does not achieve the
stated domain goal ("landing nodes and permissions as a single aggregate"), and it trades the constructor-level coupling for a signature-level coupling
(data-layer Persisted* types now appear in domain use-case signatures).
- Should fix
- Align the implementation with the know-how wiki pages on import/export. The two references to apply here:
In particular: parsing belongs in the presentation layer, repositories never receive File, use cases work only with domain types, and export must be explicitly
modelled either as a UI utility (presentation owns it) or as a domain action (use case + domain port whose implementation serializes). The current split sits
awkwardly between the two.
- No domain aggregate introduced. The stated goal of the PR was to stop treating landing nodes and permissions as two separate things and model a proper domain
aggregate. After the change, ImportLandingNodesUseCase and ExportLandingNodesUseCase still (a) fetch nodes from LandingNodeRepository, (b) fetch Settings and
filter landingPagePermissions, (c) merge/split them by hand. The only "aggregate" present is the type alias PersistedLandingNodeWithPermissions in
src/data/entities/PersistedLandingNode.ts — a serialization DTO, not a domain entity. Introduce LandingPage (or equivalent) in src/domain/entities/ grouping {
tree, permissions } with its invariants, and a single LandingPageRepository port so the merge lives inside the repository, not inside the use case. - Aggregate identity must be explicit. When you introduce LandingPage, decide the aggregate root: today landingPagePermissions has an entry per every node in
the tree (not only the root), so the aggregate shape is roughly { tree: LandingNode, permissions: Map<nodeId, Permission> }. Model it explicitly or the merge
will creep back into the use case the next time someone touches it. - Data-layer types leaked into domain use-case signatures. src/domain/usecases/ExportLandingNodesUseCase.ts:14 returns PersistedLandingPageWithPermissions[];
src/domain/usecases/ImportLandingNodesUseCase.ts:13 accepts PersistedLandingNodeWithPermissions[]; src/domain/usecases/ExportActionsUseCase.ts:10 returns
PersistedAction[]; src/domain/usecases/ImportActionsUseCase.ts:15 accepts PersistedAction[]. These types live in src/data/entities/, so the dependency has only
been moved from the constructor to the signatures. Use cases should work only with domain types or application DTOs. With the aggregate above, these signatures
become LandingPage[] / domain entities and the Persisted* types disappear from domain. - ImportExportClient lives in data/ but is invoked from the presentation services. src/webapp/services/LandingImportService.ts and the three sibling files
import from src/data/clients/importExport/ImportExportClient.ts. This is architecturally incoherent: either the client is a presentation-layer serializer (which
matches the Import-from-File wiki's rule "parsing lives in presentation layer, not infrastructure"), or it is hidden behind a domain port implemented in data.
Pick one. In either case, add an interface/port so consumers can be tested without the concrete class (today there is no ImportExportPort, it's a bare class
instantiated directly). - CompositionRoot breaks the project convention by exposing services instead of use cases. src/webapp/CompositionRoot.ts:81,91,92 now returns
ActionExportService, ActionImportService, LandingExportService, LandingImportService under the export / import keys. The convention in this codebase is that the
composition root exposes use cases. Move the File[] → ArrayBuffer → entity conversion and the entity → zip serialization into custom hooks in presentation
(e.g. useImportLandings, useExportLandings), and have CompositionRoot expose the use cases again. The "service" concept is ambiguous (domain service vs.
infrastructure service) and the current classes are really presentation orchestrators — closer to hooks than services. - landingPagePermissions overloads the Settings entity. Permissions live inside Settings, so any change to permissions forces a read-modify-write of the entire
settings document (src/domain/usecases/ImportLandingNodesUseCase.ts:31-40). This is a false transactional coupling and the root cause of the manual merge inside
the use case. Extract permissions to their own entity/repository, or — better — absorb them into the LandingPage aggregate so the use case body collapses to
repo.save(pages). - Domain repositories expose Persisted* types. LandingNodeRepository.getPersistedLandingPages() returns PersistedLandingPage[]. A domain port should not expose
a type whose very name is "Persisted". Have the repository return domain entities; the zip-shaped payload belongs inside the ImportExportClient / serializer,
not on the domain seam. - Import-validation rule sits ad-hoc in the use case. src/domain/usecases/ImportActionsUseCase.ts:14-20 validates "every action of type page references an
existing landing node and that node does not already list it". This is a domain invariant. With the aggregate in place, it lives on LandingPage.addAction(...)
(or similar) and the if (!valid) reject goes away. - Promise.reject(i18n.t(...)) inside a use case. src/domain/usecases/ImportActionsUseCase.ts:23-24 rejects with a localized string. Translation is a
presentation concern. Throw a typed domain error (e.g. ActionsReferenceInvalidPageError) and let the hook localize. - No tests added for the now-pure use cases. One of the declared benefits of removing ImportExportClient from the constructor was testability. The use cases are
now trivially unit-testable with fake repositories, and the import/export flows are exactly the paths most likely to regress silently. Add unit tests for
ImportLandingNodesUseCase, ExportLandingNodesUseCase, ImportActionsUseCase and ExportActionsUseCase as part of this PR — the marginal cost is low and the
benefit is captured right when the seam is clean.
- Recommendations non blocking
- Duplicated pattern across Landing and Action import/export. The four new classes do the same thing: files → arrayBuffer → zip-parse → entity → use case. Once
the presentation hook approach lands, consider a generic ImportSerializerPort / ExportSerializerPort so the horizontal mechanics are written once. - ArrayBuffer is still a DOM primitive inside data/. src/data/clients/importExport/ImportExportClient.ts:32 takes ReadonlyArray. If the client is
reframed as a presentation-layer serializer this stops mattering; if it stays in data/, consider Uint8Array[] to sever the DOM tie entirely. Minor polish,
decide together with the client's placement.
📌 References
📝 Implementation
From the suggested list, these were implemented:
Use cases depending on data-layer artifacts
ExportLandingNodesUseCaseandImportLandingNodesUseCasedepend onImportExportClientandPersistedLandingNode*from the data layer, breaking the dependency rule (use cases should depend on domain contracts).Suggestion: Introduce domain-level ports for export/import; implement them with
ImportExportClientandPersistedLandingNode*in infrastructure. Use cases should work only with domain types (LandingNode,LandingPagePermission) or application DTOs.Use cases coupled to UI/infrastructure (
File)ImportLandingNodesUseCase.execute(files: File[])depends on the browserFiletype.Suggestion: The use case should receive a domain-level input (DTO or parsed entity). The web layer should convert
File[]to that input before calling the use case.📹 Screenshots/Screen capture
🔥 Testing