fix: import pipeline in v2 should navigate to a proper file#2109
Conversation
🎩 PreviewA preview build has been created at: |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| @@ -33,14 +35,30 @@ function deserializeSpec(data: unknown, idGen?: IdGenerator): ComponentSpec { | |||
| return spec; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
🤖 This is an AI-generated code review comment.
backfillFromLegacyStore directly instantiates RootFolderDbStorageDriver, creating a concrete dependency in a module that otherwise works against the PipelineStorageService abstraction.
This works today because legacy pipelines are always in IndexDB, but if the root folder's driver ever changes, this backfill would silently check the wrong store.
Consider either:
- Checking via
storage.rootFolder.findFile(name)(which uses the folder's configured driver) - Or adding a comment clarifying this coupling is intentional since legacy data lives exclusively in IndexDB
There was a problem hiding this comment.
That's ok for now
There was a problem hiding this comment.
I understand the problem this is solving, but want to question if this is the best solution.
Importing a pipeline will append a fileId to the url whilst manually navigating to a pipeline will not.
import:
navigate:
Both cases work fine and load the pipeline but the discrepancy in the url is a bit of a smell to me. we could end up with diverging pipeline state in the future depending on whether fileId was included.
Have you looked into trying to make this work without having to append fileId to the url?
|
^ Claude seems to suggest |
|
I believe in fileId more than in $pipelineName - since multiple sources of pipelines can be used (localStorage, API, GDrive) - name clashes is highly likely. That's why having "id" is more desirable |
3 participants
Description
After importing a pipeline, the editor now navigates using both the pipeline name and its file ID (via
PipelineRef). This ensures the editor can reliably locate the correct file rather than relying solely on the name.An
onImportCompletecallback has been added toImportPipeline, allowing callers to override the default post-import navigation behavior. TheFileMenuin the v2 editor uses this callback to navigate with the fullPipelineRef, including thefileIdas a search parameter.A backfill path has also been introduced in
useLoadSpecso that pipelines stored in the legacyRootFolderDbStorageDriverare automatically migrated to the new storage layer when accessed by name, preventing "pipeline not found" errors for existing pipelines that haven't yet been assigned a file ID.Related Issue and Pull requests
Type of Change
Checklist
Screenshots (if applicable)
Test Instructions
fileIdsearch parameter.fileId) and confirm it loads correctly without a "not found" error.Additional Comments
The backfill logic in
useLoadSpecis a compatibility shim for pipelines that exist in the legacy store but have not yet been assigned a file ID in the new storage layer. It assigns the file on first access, so subsequent loads will resolve by ID.