refactor: Introduce task phases system in preparation for dynamic tasks #483
Conversation
🦋 Changeset detectedLatest commit: ed837ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update introduces a task phases system to the sync package. Several interfaces and functions have been extended with a new optional Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant GR as Generator Runner
participant PS as Phase Sorter
participant DM as Dependency Resolver
participant TE as Task Executor
C->>GR: Initiate generator execution
GR->>PS: Extract & sort task phases
PS-->>GR: Sorted phases array
loop For each phase in sorted phases
GR->>DM: Resolve dependencies for current phase
DM-->>GR: Return dependency map
GR->>TE: Execute tasks for current phase
TE-->>GR: Return phase execution results
end
GR->>C: Return aggregated output metadata
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Definitions (1)packages/sync/src/runner/dependency-map.unit.test.ts (4)
🔇 Additional comments (6)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/sync/src/phases/types.ts (1)
4-13: Well-designed TaskPhaseOptions interface for dependency management.The
TaskPhaseOptionsinterface is well-structured for managing phase dependencies. However, be cautious about potential circular references sinceconsumesOutputFromandaddsDynamicTasksToproperties referenceTaskPhase[]arrays, which could create cycles in your dependency graph.Consider using a more primitive reference system like phase names (strings) instead of full TaskPhase objects to avoid deep nesting and potential circular dependencies:
export interface TaskPhaseOptions { - consumesOutputFrom?: TaskPhase[]; + consumesOutputFrom?: string[] | TaskPhase[]; // Allow referencing by name - addsDynamicTasksTo?: TaskPhase[]; + addsDynamicTasksTo?: string[] | TaskPhase[]; // Allow referencing by name }This would provide more flexibility in how phases reference each other while reducing the risk of circular structures.
packages/sync/src/phases/sort-task-phases.ts (2)
5-13: Validate behavior on circular dependencies or missing references.Currently, the function checks for duplicate phase names but doesn’t explicitly handle cycles or invalid references beyond throwing an error if the same name appears. Relying on
toposort.arraywill raise an error on circular dependencies, but it might be beneficial to wrap that error with a more specific message (e.g., which phases caused the cycle).
15-23: Consider grouping the dependency graph construction logic for clarity.You build the map of all phases here, then add edges in the next block. It can be clearer to keep phase lookups and edge building in a single place, especially if you plan to expand or validate more properties. This would also reduce the overhead of multiple passes over the same phases.
packages/sync/src/runner/generator-runner.unit.test.ts (1)
28-39: Centralizing phases for all tests.Defining
phase1andphase2outside of the individual tests allows multiple specs to share them, but ensure no unintended side effects carry over from one test to another. Consider resetting or re-initializing them in a beforeEach if they’re ever mutated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (17)
.changeset/purple-schools-sell.md(1 hunks)packages/sync/src/generators/build-generator-entry.ts(3 hunks)packages/sync/src/generators/generators.ts(2 hunks)packages/sync/src/index.ts(1 hunks)packages/sync/src/phases/index.ts(1 hunks)packages/sync/src/phases/sort-task-phases.ts(1 hunks)packages/sync/src/phases/sort-task-phases.unit.test.ts(1 hunks)packages/sync/src/phases/types.ts(1 hunks)packages/sync/src/phases/utils.ts(1 hunks)packages/sync/src/providers/providers.ts(1 hunks)packages/sync/src/runner/dependency-map.ts(4 hunks)packages/sync/src/runner/dependency-map.unit.test.ts(3 hunks)packages/sync/src/runner/dependency-sort.ts(1 hunks)packages/sync/src/runner/generator-runner.ts(2 hunks)packages/sync/src/runner/generator-runner.unit.test.ts(6 hunks)packages/sync/src/runner/tests/factories.test-helper.ts(2 hunks)packages/sync/src/runner/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (10)
packages/sync/src/runner/utils.ts (3)
packages/sync/src/generators/build-generator-entry.ts (1)
GeneratorTaskEntry(17-54)packages/sync/src/phases/types.ts (1)
TaskPhase(18-21)packages/sync/src/phases/utils.ts (1)
sortTaskPhases(5-52)
packages/sync/src/phases/utils.ts (2)
packages/sync/src/phases/sort-task-phases.ts (1)
sortTaskPhases(5-63)packages/sync/src/phases/types.ts (1)
TaskPhase(18-21)
packages/sync/src/runner/generator-runner.unit.test.ts (3)
packages/sync/src/phases/types.ts (1)
TaskPhase(18-21)packages/sync/src/runner/tests/factories.test-helper.ts (2)
buildTestGeneratorEntry(61-86)buildTestGeneratorTaskEntry(29-59)packages/sync/src/runner/generator-runner.ts (1)
executeGeneratorEntry(30-221)
packages/sync/src/runner/generator-runner.ts (4)
packages/sync/src/generators/build-generator-entry.ts (1)
GeneratorEntry(59-80)packages/sync/src/output/generator-task-output.ts (3)
GeneratorOutput(90-92)GeneratorTaskOutput(56-69)GeneratorOutputMetadata(71-85)packages/sync/src/runner/utils.ts (2)
flattenGeneratorTaskEntries(28-33)extractSortedTaskPhases(41-46)packages/sync/src/runner/dependency-map.ts (2)
buildGeneratorIdToScopesMap(255-261)resolveTaskDependenciesForPhase(272-285)
packages/sync/src/generators/build-generator-entry.ts (1)
packages/sync/src/phases/types.ts (1)
TaskPhase(18-21)
packages/sync/src/phases/sort-task-phases.unit.test.ts (3)
packages/sync/src/phases/types.ts (1)
createTaskPhase(29-37)packages/sync/src/phases/sort-task-phases.ts (1)
sortTaskPhases(5-63)packages/sync/src/phases/utils.ts (1)
sortTaskPhases(5-52)
packages/sync/src/generators/generators.ts (1)
packages/sync/src/phases/types.ts (1)
TaskPhase(18-21)
packages/sync/src/runner/tests/factories.test-helper.ts (2)
packages/sync/src/generators/build-generator-entry.ts (1)
GeneratorTaskEntry(17-54)packages/sync/src/generators/generators.ts (1)
GeneratorTask(132-186)
packages/sync/src/runner/dependency-map.ts (2)
packages/sync/src/generators/build-generator-entry.ts (1)
GeneratorEntry(59-80)packages/sync/src/phases/types.ts (1)
TaskPhase(18-21)
packages/sync/src/runner/dependency-map.unit.test.ts (4)
packages/sync/src/phases/types.ts (1)
TaskPhase(18-21)packages/sync/src/generators/build-generator-entry.ts (1)
GeneratorEntry(59-80)packages/sync/src/runner/dependency-map.ts (3)
EntryDependencyMap(193-196)buildGeneratorIdToScopesMap(255-261)resolveTaskDependenciesForPhase(272-285)packages/sync/src/runner/tests/factories.test-helper.ts (2)
buildTestGeneratorEntry(61-86)buildTestGeneratorTaskEntry(29-59)
🔇 Additional comments (55)
.changeset/purple-schools-sell.md (1)
1-6: Changeset metadata looks correct.The changeset correctly identifies a patch version update for the
@halfdomelabs/syncpackage with a clear, concise description of the change.packages/sync/src/index.ts (1)
5-5: New export for phases module is properly integrated.The export statement is correctly added to expose the phases functionality from the main package entry point.
packages/sync/src/phases/index.ts (1)
1-2: Export statements are correctly structured.The index file appropriately re-exports the functionality from the sort-task-phases and types modules, providing a clean API for phase-related utilities.
packages/sync/src/runner/dependency-sort.ts (1)
57-61: Added phase-awareness for dependency resolution looks good.This change correctly prevents adding dependencies for tasks that have already been executed in a previous phase. The comment explains the purpose clearly, and the implementation uses an appropriate check to filter out irrelevant dependencies.
packages/sync/src/phases/types.ts (2)
18-21: Clean TaskPhase interface definition.The
TaskPhaseinterface is concise and well-defined. The structure accurately represents the concept of a task phase with its name and configuration options.
29-37: Well-implemented createTaskPhase factory function.The
createTaskPhasefunction is properly implemented with sensible defaults for the options parameter. The function signature is clearly typed and the JSDoc documentation is thorough.packages/sync/src/phases/sort-task-phases.ts (2)
25-50: Verify references forconsumesOutputFromandaddsDynamicTasksTo.These loops push edges for dependencies and dependents, but currently do not confirm that the node array (derived from
phases) includes all references until topological sort. Validate whether it’s necessary to throw a more consistent error message if a referenced phase name is not inphases.
53-63: Confirm that orphan phases remain in the final sorted result.Using
toposort.array(nodes, edges)ensures that phases without dependencies still appear in the output. This is correct if you need to process phases even when they have no direct dependencies. Just verify that the result acts as intended for phases that neither depend on nor are depended on by others.packages/sync/src/runner/generator-runner.unit.test.ts (4)
4-4: New type import appears correct.Importing
TaskPhaseextends test coverage for phased tasks. No issues noted; usage is consistent with the rest of the code.
21-24: Helper factories import is well-structured.Bringing in the
buildTestGeneratorEntryandbuildTestGeneratorTaskEntryutilities fosters consistency across test setups. Implementation looks neat.
342-430: Comprehensive test for phased task execution.This test ensures that phases run in sequence and that output providers from earlier phases are properly consumed. The file checks confirm the correct generation outputs. Excellent coverage of real-world usage.
431-466: Correct handling of errors for non-output providers across phases.This negative test scenario correctly verifies that an error is thrown if a regular provider is passed across phases. Good job enforcing domain constraints.
packages/sync/src/phases/utils.ts (1)
41-43:toposort(edges)may skip phases with no edges.Unlike
toposort.array(nodes, edges), calling justtoposort(edges)doesn’t guarantee the inclusion of nodes that have no edges. This may inadvertently drop phases that neither depend on nor are depended on by others. If you want them included in the final sequence, switch totoposort.array.packages/sync/src/providers/providers.ts (1)
178-178: SetisOutputproperly increateProviderType.This line correctly forwards the
isOutputoption to the provider type. It integrates seamlessly with the rest of the type creation logic for read-only/export providers, ensuring consistent usage across the dependency and export maps. No further issues spotted.packages/sync/src/phases/sort-task-phases.unit.test.ts (8)
1-5: Test imports initialization looks good.Imports for
vitest, thesortTaskPhasesutility, and thecreateTaskPhasehelper are concise and clear. No issues spotted.
6-18: Test basic sorting scenario coverage.The test ensures that phases are orderly sorted when chained by straightforward dependencies (
A -> B -> C). Verifies topological ordering effectively. Looks correct.
20-29: Test multiple dependency handling.This test checks that a phase depending on multiple predecessors is sorted correctly. It is a standard and essential scenario, well-handled here.
31-42: TestaddsDynamicTasksTorelationship.This verifies that tasks which add dynamic tasks to another phase are properly positioned before consuming phases. The expectation
['B', 'A', 'C']clarifies how the dynamic insertion reorders tasks. Great coverage.
44-51: Test duplicate phase detection.The test correctly ensures an error is thrown for identical phase names, preventing confusion in the topological sort. Looks solid.
53-62: Test missing dependency detection.Ensures an error is thrown if a phase references a dependency not present in the array of phases. This check is essential for robust dependency handling.
64-71: Test independent phases ordering.Verifies that phases without interdependencies remain in their original order. Straightforward and correct.
73-87: Test complex multi-dependency graph.Scenarios with multiple edges leading into a final phase are tested thoroughly. The ordering
['A', 'B', 'C', 'D']after sorting confirms correct topological resolution.packages/sync/src/runner/generator-runner.ts (9)
12-16: Extended imports for generator outputs.Bringing
GeneratorOutput,GeneratorOutputMetadata, andGeneratorTaskOutputinto scope is appropriate for collecting and returning metadata from each phase. No issues spotted.
20-23: Dependency resolution imports match the new phase logic.These imports facilitate phase-specific dependency resolution and are consistent with the new task phases approach.
25-28: Helper imports for flattening tasks & extracting sorted phases.Aligning with the newly introduced task-phase system. Proper usage of existing utilities simplifies the code.
35-35: Extract and sort task phases early.Storing
taskPhasesfromextractSortedTaskPhases(taskEntries)is a solid step. The logic clearly delineates tasks by phase for sequential processing.
38-38: Building generator-to-scopes map.Constructing this map before phase iteration provides a reliable context for dependency resolutions. No concerns here.
44-44: CollectingGeneratorOutputMetadata[].Accumulating phase metadata into
generatorMetadataspaves the way to produce a unified output. Straightforward design.
46-61: Iterate over phases, including an “undefined” pre-phase.This mechanism allows a phase-agnostic pass (
undefined) followed by phase-specific passes. It’s a clean approach for tasks that do not specify a phase. No immediate issues.
62-185: Phase-specific run steps.
- Splits tasks into “init” and “build” steps.
- Reliably checks for output-based dependencies crossing phase boundaries.
- Thoroughly raises errors for misconfigurations (e.g., non-output from previous phase).
- Correctly updates
providerMapByIdand ensures missing providers are caught.
Overall, the logic is robust and aligns with topological sorting results. This is a well-organized approach to multi-phase execution.
203-210: Merging all metadata.Combining
generatorProviderRelationshipsandgeneratorTaskEntriesfrom multiple phases into a single metadata block is appropriate. Ensures final output coverage is complete.packages/sync/src/runner/tests/factories.test-helper.ts (7)
3-6: No issues with the new type imports.These imports accurately reference the necessary types and appear consistent with usage in this file.
30-32: Good approach to flexible test data construction.Using
Omit<Partial<GeneratorTaskEntry>, 'task'>in combination with an optionaltaskproperty enables a clear separation of thetaskfield from other entry properties.
34-34: Phase fallback logic looks correct.Falling back to
data?.phasewhendata?.task?.phaseis undefined helps unify phase specification.
36-39: Fallback assignments for task fields are well-structured.Choosing
data?.task?.<field>beforedata?.<field>provides fine-grained overrides for testing.
42-44: Accurate exposure of dependencies, exports, and outputs.Ensuring these properties remain consistent on the returned entry streamlines testing.
47-47: Explicit phase on the returned object is beneficial.Including
phaseon the high-level entry object mirrors the nested task property for readability.
49-57: Constructing the nestedtaskobject is coherent and thorough.Spreading in
data?.taskat the end handles additional overrides gracefully.packages/sync/src/runner/dependency-map.ts (4)
3-3: Import ofTaskPhaseis properly introduced.This aligns well with the new phase-related logic.
211-228: Filtering tasks by phase inbuildEntryDependencyMapRecursive.Adding
phase: TaskPhase | undefinedand filtering tasks ensures only tasks of the specified phase are processed, which aligns with the new phase-specific dependency resolution. Verify that this behavior is intentional whenphaseisundefined, as it will exclusively match tasks whose phase is explicitlyundefined.
249-261: Helper for building generator ID to scopes map is well-structured.This exported function neatly encapsulates scope resolution logic.
263-285:resolveTaskDependenciesForPhasedocumentation and implementation look solid.Renaming to reflect explicit phase handling improves clarity. Confirm that tasks lacking a defined phase should be excluded when a specific phase is passed.
packages/sync/src/runner/dependency-map.unit.test.ts (8)
3-6: New type imports are correct.These imports match the implementations under test.
9-9:EntryDependencyMapimport is aligned with usage.No issues found.
15-18: Importing phase-related API is consistent.Makes the new functionalities available for testing.
58-70: Local helper functionresolveTaskDependenciesis well factored.Wrapping
buildGeneratorIdToScopesMapandresolveTaskDependenciesForPhasesimplifies usage in tests.
72-72: New describe block introduces comprehensive phase-based tests.Clearly groups phase-specific tests under a dedicated suite.
643-689: Phase-specific dependency resolution test is clear and robust.Verifies that tasks in
phase1differ from those inphase2as expected.
691-747: Phase-specific scopes test.Ensures provider scopes for one phase can be consumed by tasks in another phase if designed.
749-825: Nested phase dependencies test.Gracefully validates multi-level scenarios where tasks in earlier phases feed outputs to subsequent phases.
packages/sync/src/generators/build-generator-entry.ts (3)
1-1: Import added for task phase support.The import for
TaskPhaseis correctly added to support the new phase functionality in generator tasks.
50-53: Phase property added to GeneratorTaskEntry interface.The addition of the
phaseproperty to theGeneratorTaskEntryinterface aligns with the PR objective of introducing a task phases system. The property is properly documented and typed asTaskPhase | undefined.
111-111: Task phase correctly assigned to the task entry.The implementation correctly assigns the
task.phaseto thephaseproperty of the task entry, ensuring that phase information is properly propagated.packages/sync/src/runner/utils.ts (1)
5-7: New imports added for task phase functionality.The required imports for
TaskPhaseandsortTaskPhasesare correctly added to support the extraction and sorting of task phases.packages/sync/src/generators/generators.ts (1)
4-4: Import added for task phase support.The import for
TaskPhasetype is correctly added to support the new phase property in generator tasks.
kingston
deleted the
kingston/eng-580-introduce-dynamic-providers-and-task-phases
branch
Summary by CodeRabbit
New Features
Tests