Convert topological sort utility to TypeScript#28388
Conversation
no ref This change should have no user impact.
WalkthroughThis PR migrates the topological sort utility from JavaScript to TypeScript with generic typing support. The new implementation defines a 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/core/server/data/seeders/utils/topological-sort.ts (2)
23-40: ⚖️ Poor tradeoffConsider adding cycle detection.
The current DFS implementation silently handles cycles by returning early when a visited node is encountered (line 24-26). While this prevents infinite recursion, circular dependencies in the table schema would produce an ordering that cannot satisfy all constraints. Consider detecting cycles and throwing an error to catch configuration issues early.
For example, if table A depends on B and B depends on A, the algorithm will produce an ordering, but neither ordering [A, B] nor [B, A] satisfies both constraints.
🔍 Potential enhancement for cycle detection
Track nodes in the current DFS path separately from globally visited nodes:
export function topologicalSort<T extends TopologicalSortable>(objects: ReadonlyArray<T>): T[] { const result: T[] = []; const visited = new Set<string>(); + const visiting = new Set<string>(); const objectsByName = new Map<string, T>(); for (const object of objects) { objectsByName.set(object.name, object); } function dfs(name: string): void { + if (visiting.has(name)) { + throw new Error(`Circular dependency detected: ${name}`); + } if (visited.has(name)) { return; } const object = objectsByName.get(name); if (!object) { return; } + visiting.add(name); visited.add(name); for (const dependency of object.dependencies || []) { dfs(dependency); } + visiting.delete(name); result.push(object); }🤖 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 `@ghost/core/core/server/data/seeders/utils/topological-sort.ts` around lines 23 - 40, The DFS currently uses visited to avoid infinite recursion but doesn't detect cycles; update dfs(name: string) to maintain a separate "visiting" (or "stack") Set in addition to visited so that when you encounter a dependency already in visiting you throw a descriptive Error (include the cycle nodes) instead of silently returning; use the existing symbols objectsByName, object.dependencies, visited, result and ensure you remove the name from visiting when unwinding so normal topological ordering still pushes object into result.
28-31: ⚡ Quick winDocument or validate missing dependency behavior.
When a dependency name is declared but the corresponding object is not found in the input array, the function silently skips it (lines 28-31). This could hide configuration errors or typos in dependency declarations.
Consider either:
- Documenting this behavior in the JSDoc if it's intentional (e.g., for external/optional dependencies), or
- Throwing an error to fail fast on misconfiguration
📝 Example: Add validation
const object = objectsByName.get(name); if (!object) { + throw new Error(`Dependency "${name}" not found in input objects`); - return; }Or document the behavior:
/** * This sorting algorithm is used to make sure that dependent tables are imported after their dependencies. + * + * `@param` objects - Array of objects to sort. Each object must have a `name` and optional `dependencies` array. + * `@returns` Sorted array where dependencies appear before dependents. Missing dependencies are silently skipped. */🤖 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 `@ghost/core/core/server/data/seeders/utils/topological-sort.ts` around lines 28 - 31, The code currently silently skips missing dependencies at objectsByName.get(name) (the local variable object) which can hide misconfigurations; update the topological sort implementation (the function containing objectsByName.get(name), e.g., the visit/resolve function in topological-sort.ts) to validate that object exists and throw a clear error including the missing dependency name (and ideally the parent object's name/context) instead of returning silently; alternatively, if missing dependencies are intentional, add a JSDoc comment on the topological sort function explaining that missing names are ignored and why.
🤖 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.
Nitpick comments:
In `@ghost/core/core/server/data/seeders/utils/topological-sort.ts`:
- Around line 23-40: The DFS currently uses visited to avoid infinite recursion
but doesn't detect cycles; update dfs(name: string) to maintain a separate
"visiting" (or "stack") Set in addition to visited so that when you encounter a
dependency already in visiting you throw a descriptive Error (include the cycle
nodes) instead of silently returning; use the existing symbols objectsByName,
object.dependencies, visited, result and ensure you remove the name from
visiting when unwinding so normal topological ordering still pushes object into
result.
- Around line 28-31: The code currently silently skips missing dependencies at
objectsByName.get(name) (the local variable object) which can hide
misconfigurations; update the topological sort implementation (the function
containing objectsByName.get(name), e.g., the visit/resolve function in
topological-sort.ts) to validate that object exists and throw a clear error
including the missing dependency name (and ideally the parent object's
name/context) instead of returning silently; alternatively, if missing
dependencies are intentional, add a JSDoc comment on the topological sort
function explaining that missing names are ignored and why.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80843f2e-87f6-4110-91b0-46341ab54ebe
📒 Files selected for processing (3)
ghost/core/core/server/data/seeders/data-generator.jsghost/core/core/server/data/seeders/utils/topological-sort.jsghost/core/core/server/data/seeders/utils/topological-sort.ts
💤 Files with no reviewable changes (1)
- ghost/core/core/server/data/seeders/utils/topological-sort.js
no ref
This change should have no user impact.