Fix multi-file workflow: pipeline sourceMap tracking, recursive imports, prevent inline rebuild on onChange#3
Conversation
…onChange inline rebuild - resolveImports: add resolvePath helper and internal mergeFile that recursively resolves nested `imports:` / `application.workflows[].file:` entries depth-first using a visited set to prevent cycles - resolveImports: track pipeline names in sourceMap alongside modules so they round-trip back to their source file via exportToFiles (fixes silent inlining) - resolveImports: preserve name/version from application: section - WorkflowEditor: onChange emits main-file content only in multi-file mode, preventing the host from receiving the fully merged YAML on every render (ReactFlow's dimension measurements triggered onChange with merged YAML) - test-fixtures/multifile/: add main/base/api/database.yaml nested fixture scenario - serialization-multifile.test.ts: 14 new tests for nested resolution, pipeline sourceMap, round-trip no-inlining, change routing, cycle detection, error handling Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-editor/sessions/90bd1df4-654d-4fc2-b077-f7dfb344a3ea
There was a problem hiding this comment.
Pull request overview
Fixes multi-file workflow editing so imported content round-trips back to its originating files instead of being inlined into the main YAML, and prevents ReactFlow’s initial layout events from emitting merged/inlined YAML via onChange.
Changes:
- Refactors
resolveImportsto recursively resolve nestedimports:andapplication.workflows[].file:references and to track pipelines insourceMap. - Preserves
name/versionwhen they live underapplication:in the main file. - Updates
WorkflowEditorto emit main-file YAML (with references) in multi-file mode instead of emitting the fully merged YAML.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/serialization.ts |
Adds recursive import resolution, path resolution helper, and pipeline sourceMap tracking. |
src/components/WorkflowEditor.tsx |
Changes onChange to emit main-file content from exportToFileMap() in multi-file mode. |
src/utils/serialization-multifile.test.ts |
Adds tests covering nested resolution, pipeline routing via sourceMap, round-trip export, and error handling. |
test-fixtures/multifile/main.yaml |
Adds multi-file fixture main config (application workflow file references). |
test-fixtures/multifile/base.yaml |
Adds nested-import fixture. |
test-fixtures/multifile/database.yaml |
Adds leaf fixture with modules. |
test-fixtures/multifile/api.yaml |
Adds fixture with modules/workflows/pipelines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function mergeFile(resolvedPath: string, strictConflicts: boolean): Promise<void> { | ||
| if (visited.has(resolvedPath)) return; | ||
| visited.add(resolvedPath); | ||
|
|
||
| const content = await resolver(resolvedPath); | ||
| if (content === null) { | ||
| errors.push(strictConflicts ? `Workflow file not found: ${resolvedPath}` : `Import not found: ${resolvedPath}`); | ||
| return; |
There was a problem hiding this comment.
visited is marked before the resolver/parsing succeeds. This means if a file is first encountered via lenient imports: (or is missing), later references to the same path via strict application.workflows[].file will be skipped entirely, potentially suppressing required conflict errors and even changing the intended error message type. Consider tracking an inProgress set for cycle detection and only adding to a visited/completed set after a successful load+parse, and/or allowing a later strict merge to re-run or at least perform conflict checks.
| if (subApp && Array.isArray(subApp.workflows)) { | ||
| for (const entry of subApp.workflows as Array<Record<string, unknown>>) { | ||
| if (typeof entry.file === 'string') { | ||
| await mergeFile(resolvePath(resolvedPath, entry.file), false); |
There was a problem hiding this comment.
Nested application.workflows[].file entries are currently merged with strictConflicts = false. That changes the documented semantics for application.workflows[].file (conflicts should be reported as errors) and also produces Import not found: errors instead of Workflow file not found: for nested workflow-file references. Passing true here (or otherwise treating all application.workflows[].file references as strict at any depth) would keep conflict/error behavior consistent.
| await mergeFile(resolvePath(resolvedPath, entry.file), false); | |
| // Workflow file references are always strict (conflicts are errors), | |
| // even when they are nested inside other workflow files. | |
| await mergeFile(resolvePath(resolvedPath, entry.file), true); |
| } catch (e) { | ||
| errors.push(`Error parsing ${importPath}: ${(e as Error).message}`); | ||
| mergedPipelines[key] = value; | ||
| sourceMap.set(key, resolvedPath); |
There was a problem hiding this comment.
sourceMap is now used for both module names and pipeline names, but it’s still a single Map<string, string>. If a module and a pipeline share the same name, the later sourceMap.set will overwrite the earlier one, breaking round-trip routing for at least one of them. Consider namespacing keys (e.g., module:<name> / pipeline:<name>) or splitting this into separate maps so modules and pipelines can’t collide.
| sourceMap.set(key, resolvedPath); | |
| const pipelineKey = `pipeline:${key}`; | |
| sourceMap.set(pipelineKey, resolvedPath); |
| if (hasMultiFileRef.current) { | ||
| // In multi-file mode emit only the main file content (with imports: references) | ||
| // rather than the fully merged YAML, to prevent the host from inlining all files. | ||
| const fileMap = exportToFileMap(); | ||
| onChange(fileMap.get(null) ?? configToYaml(exportToConfig())); | ||
| } else { | ||
| const config = exportToConfig(); | ||
| onChange(configToYaml(config)); | ||
| } |
There was a problem hiding this comment.
In multi-file mode the subscription now calls exportToFileMap() on every store update, which serializes all files (YAML dump per file) even though only the main file content is emitted. This can become a noticeable performance cost for larger multi-file workspaces (e.g., during node drags). Consider adding a cheaper exportMainFileYaml() path, or have exportToFileMap() optionally compute only the null entry when used for onChange.
| // Fixture files mirroring test-fixtures/multifile/ | ||
| const FIXTURE_MAIN = ` | ||
| application: | ||
| name: my-platform | ||
| version: 2.0.0 | ||
| workflows: | ||
| - file: base.yaml | ||
| - file: api.yaml | ||
| `.trim(); | ||
|
|
||
| const FIXTURE_BASE = ` | ||
| # Base infrastructure — embeds the database layer | ||
| imports: | ||
| - database.yaml | ||
|
|
||
| modules: | ||
| - name: cache | ||
| type: nosql.redis | ||
| config: | ||
| host: localhost | ||
| port: 6379 | ||
| `.trim(); | ||
|
|
||
| const FIXTURE_DATABASE = ` | ||
| modules: | ||
| - name: db | ||
| type: database.postgres | ||
| config: | ||
| host: localhost | ||
| port: 5432 | ||
| database: myapp | ||
| `.trim(); | ||
|
|
||
| const FIXTURE_API = ` | ||
| modules: | ||
| - name: http-server | ||
| type: http.server | ||
| config: | ||
| port: 8080 | ||
| - name: router | ||
| type: http.router | ||
| config: {} | ||
|
|
||
| workflows: | ||
| http: | ||
| server: http-server | ||
| router: router | ||
| routes: | ||
| - method: POST | ||
| path: /api/users | ||
| handler: user-create | ||
|
|
||
| pipelines: | ||
| user-create: | ||
| steps: | ||
| - name: validate | ||
| type: step.validate | ||
| - name: insert | ||
| type: step.db_exec | ||
| user-get: | ||
| steps: | ||
| - name: fetch | ||
| type: step.db_exec | ||
| `.trim(); | ||
|
|
There was a problem hiding this comment.
The test defines inline YAML fixture strings “mirroring test-fixtures/multifile/”, but the PR also adds those fixture files and they appear unused by the test suite. This duplication increases maintenance risk (fixtures can drift). Consider loading the YAML from the fixture files in the test, or removing the on-disk fixtures if they aren’t meant to be used.
|
@copilot apply changes based on the comments in this thread |
- resolveImports: replace single `visited` set with separate `inProgress` (cycle detection) and `completed` (deduplication) sets so failed-to-load files are not silently skipped when later encountered via a stricter call site - resolveImports: nested application.workflows[].file: entries now use strictConflicts=true, matching top-level semantics - pipelineKey() helper: pipeline names stored in sourceMap under `pipeline:<name>` to prevent collision with module names; update all read sites (configToNodes, exportToFiles, exportMainFileYaml) - exportMainFileYaml(): new utility + store method for cheap main-file-only YAML serialization (no per-file dump); WorkflowEditor.onChange now calls exportMainFile() instead of exportToFileMap().get(null) - serialization-multifile.test.ts: load fixture YAML from on-disk files via loadFixture() using fs.readFileSync, eliminating inline string duplication; update sourceMap constructions to use pipeline: prefix Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-editor/sessions/6b6bb52c-1684-4d6b-a858-447c8743cc3e
Applied all 5 changes from the review thread in commit
|
Opening a YAML with
application.workflows[].file:references caused the editor to inline all imported files back into the main file. Three distinct bugs combined to produce this:Bugs Fixed
1. Pipelines silently dropped from
sourceMapinresolveImportsModules from imported files were tracked in
sourceMap; pipelines were not. SinceexportToFilesroutes content back to source files viasourceMap, all pipelines fell through to the main file (null key) on every export — inlining their full content.2. Nested file references not resolved
If an imported file itself declared
imports:orapplication.workflows[].file:, those entries were ignored. Only the top level was processed.3.
onChangeemitted fully merged YAML in multi-file modeReactFlow fires
onNodesChangewithdimensionschanges on initial render (to record measured node sizes). This triggered theonChangesubscription — afterimportingRef.current = false— with the fully merged YAML, handing the host a single inlined document on every render.Changes
src/utils/serialization.tsresolvePath(basePath, relPath)helper for relative path resolutionpipelineKey(name)helper that namespaces pipeline names aspipeline:<name>insourceMap, preventing collision with identically-named modulesresolveImportsaround an internalmergeFilerecursive helper:imports:/application.workflows[].file:entriesinProgress(added before fetch, removed after — cycle detection) andcompleted(added only after successful merge — deduplication), so a file that fails to load is never silently skipped when later referenced via a stricter call siteapplication.workflows[].file:references always use strict conflict semantics, consistent with top-level behavioursourceMapunder namespaced keysname/versionnow extracted from theapplication:section when not at the top levelexportMainFileYaml(config, sourceMap)utility that produces only the main-file YAML without serialising every imported file, backed by extractedbuildMainFileContent()helpersourceMapread sites (configToNodes,exportToFiles,exportMainFileYaml) updated to usepipelineKey()for pipeline lookupssrc/utils/index.tsexportMainFileYamlutilitysrc/stores/workflowStore.tsexportMainFileYaml()store method for cheap single-file YAML generationsrc/components/WorkflowEditor.tsxonChangenow callsexportMainFile()(the new cheap path) instead ofexportToFileMap().get(null), avoiding serialisation of all imported files on every store update (e.g. during node drags)Test Fixtures & Coverage
Added
test-fixtures/multifile/{main,base,api,database}.yaml— a 3-level nested scenario (main → base → database,main → api) with modules, workflows, and pipelines spread across files. These files are the single source of truth: tests load them from disk vialoadFixture()(fs.readFileSync) rather than duplicating the YAML inline.New tests in
serialization-multifile.test.tscover: nested resolution across all levels, pipelinesourceMapassignment (withpipeline:prefix), round-trip export (no inlining), change routing back to source files, cycle detection, and missing-file error handling.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.