Code Review Phase 3: Domain Layer — Pure Business Logic

[jitu5]

Code Review Phase 3: Domain Layer — Pure Business Logic

Purpose: Review the framework-agnostic business logic. This layer has zero React or Redux dependencies — it contains the graph algorithms and ID generation that the rest of the app relies on.

Prerequisites: Phase 1 (Type System)
Estimated time: 1 hour
Files: 4

---

Files to Review (in order)

Implementation

• src/domain/IdGenerator.ts — ID generation functions with TypeScript function overloads, type re-exports from types/ids.ts
• src/domain/PipelineGraph.ts — Bipartite graph builder, DFS-based cycle detection, orphan detection (nodes/datasets with no connections)
• src/domain/index.ts — Barrel export

Tests

• src/domain/PipelineGraph.test.ts — 27 tests: graph building, cycle detection, orphan finding

---

Key Concepts

• TypeScript function overloads (generateId(type: 'node'): NodeId) provide type-narrowed return types from a single function
• The bipartite graph constraint means connections must alternate between nodes and datasets — never node-to-node or dataset-to-dataset
• buildDependencyGraph collapses the bipartite graph into an effective node-to-node dependency graph for cycle detection

---

Focus Areas

• generateNodeId() now uses crypto.randomUUID() with a Date.now() + random fallback for environments without crypto support. Verify the fallback is adequate.
• In detectCycleDFS: when a cycle is found, is the cycle path correctly extracted? Verify the DFS implementation handles all edge cases.
• parseIdType identifies connections by checking if string contains BOTH node- and dataset- — could this misfire on edge cases?
• findOrphanedNodes and findOrphanedDatasets use a Set for O(1) lookup — verify correctness
• Are the domain functions properly unit-testable? (No hidden dependencies on React, Redux, or browser APIs?)
• generateCopyId adds a random suffix — verify the format is consistent with ID type guards in ids.ts
• Review PipelineGraph.test.ts — do the 27 tests cover all edge cases (empty graph, single node, self-loop, diamond dependency)?

---

Next: Phase 4 (Redux Store & Slices) — depends on this phase

[ravi-kumar-pilla]

I see a test folder and I also see test files within other folders. This is a bit confusing. Can we stick to single format ? I would prefer having tests in individual folders like we have it on kedro-viz. Please let me know if there is a reason to do this. Thank you

[ravi-kumar-pilla]

• generateNodeId() now uses crypto.randomUUID() with a Date.now() + random fallback for environments without crypto support. Verify the fallback is adequate.
• In detectCycleDFS: when a cycle is found, is the cycle path correctly extracted? Verify the DFS implementation handles all edge cases.
• parseIdType identifies connections by checking if string contains BOTH node- and dataset- — could this misfire on edge cases?

[ ] findOrphanedNodes and findOrphanedDatasets use a Set for O(1) lookup — verify correctness

This is not O(1) lookup as we have const connectedNodes = getConnectedNodes(connections); and const connectedDatasets = getConnectedDatasets(connections); respectively. If this is a hard requirement, we need to move the looping out or think of a different strategy.

• Are the domain functions properly unit-testable? (No hidden dependencies on React, Redux, or browser APIs?)
• generateCopyId adds a random suffix — verify the format is consistent with ID type guards in ids.ts

[ ] Review PipelineGraph.test.ts — do the 27 tests cover all edge cases (empty graph, single node, self-loop, diamond dependency)?

Optional few tests to include -

1. detectCycles on empty graph
2. wouldCreateCycle

[ravi-kumar-pilla]

Hi @jitu5 , There are no major issues in these files. I left 2 minor comments. Thank you

[rashidakanchwala]

Nit: We currently have two DFS implementations for cycle detection (hasCycleInGraph and detectCycleDFS). They serve different return needs (boolean vs path), but the traversal logic is mostly duplicated.

Maybe we could extract a shared internal DFS helper and keep two small wrapper functions on top (e.g., hasAnyCycle(graph) and findAnyCyclePath(graph)) to avoid duplicating traversal behavior?