-
Notifications
You must be signed in to change notification settings - Fork 17
feat(cli): add stories pull/push commands #397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces CLI commands for pulling and pushing stories between Storyblok spaces, enabling space-to-space synchronization and third-party CMS migrations. The implementation includes reference mapping capabilities to handle circular dependencies and story relationships, along with resumable push operations via a manifest system.
Key Changes:
- Added
stories pullcommand to download stories from a space as JSON files - Added
stories pushcommand to upload local stories with automatic reference remapping - Implemented reference mapping system for story IDs and UUIDs across fields (richtext, multilink, bloks, options)
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/commands/stories/pull/index.ts | Implements the pull command to fetch and save stories locally |
| packages/cli/src/commands/stories/push/index.ts | Implements the push command with two-pass processing for story creation and reference updates |
| packages/cli/src/commands/stories/streams.ts | Provides stream-based processing utilities for fetching, reading, mapping, and writing stories |
| packages/cli/src/commands/stories/ref-mapper.ts | Core reference mapping logic that traverses story content and remaps IDs using component schemas |
| packages/cli/src/commands/stories/utils.ts | Utility functions including component schema discovery |
| packages/cli/src/utils/filesystem.ts | Enhanced filesystem utilities with write locking for concurrent operations |
| packages/cli/src/constants.ts | Added stories directory constant and color palette entry |
| packages/cli/src/program.ts | Fixed directory constant references from singular to plural form |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/cli/src/commands/stories/__test__/story-with-all-field-types.ts
Show resolved
Hide resolved
| import type { SpaceDatasource } from '../constants'; | ||
| import { vol } from 'memfs'; | ||
| import { FileSystemError } from '../../../utils'; | ||
| import { FileSystemError } from '../../../utils/error/filesystem-error'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed to make those changes because importing over the utils/index.ts file leads to circular imports breaking the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
e9350b9 to
fa26422
Compare
@storyblok/astro
storyblok
@storyblok/eslint-config
@storyblok/js
storyblok-js-client
@storyblok/management-api-client
@storyblok/nuxt
@storyblok/react
@storyblok/region-helper
@storyblok/richtext
@storyblok/svelte
@storyblok/vue
commit: |
| finally { | ||
| logger.info('Pushing stories finished', summary); | ||
| ui.stopAllProgressBars(); | ||
| const failedStories = Math.max(summary.creationResults.failed, summary.processResults.failed, summary.updateResults.failed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Failure count uses max instead of sum across stages
The failure count calculation uses Math.max across different stages, but each stage tracks failures for different stories. When Story A fails at creation and Story B fails at update, Math.max(1, 0, 1) returns 1 instead of the correct total of 2 failures. The same issue exists in the pull command where Math.max(summary.fetchStories.failed, summary.save.failed) undercounts when stories fail at both fetch and save stages. The failures across stages should be summed, not max'd, since they represent distinct story failures.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maoberlehner what do you think on this point? seems legit
| const mappedRemoteStory = mappedStoryId && await getRemoteStory({ spaceId, storyId: Number(mappedStoryId) }); | ||
| // We check the UUID to make sure it is the exact same story and not just a | ||
| // story with the same numeric ID in a different space. | ||
| if (mappedRemoteStory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing UUID check despite comment claiming verification
The comment on lines 312-313 states "We check the UUID to make sure it is the exact same story and not just a story with the same numeric ID in a different space." However, line 314 only checks if (mappedRemoteStory) without any UUID verification. Contrast this with lines 323-325 where the UUID check is correctly performed (existingRemoteStory.uuid === localStory.uuid). The missing check could cause incorrect behavior when resuming a push if the manifest points to a story ID that exists in the target space but belongs to a different story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maoberlehner could the comment be outdated?
| .filter(f => path.extname(f) === '.json') | ||
| .map((f) => { | ||
| const filePath = path.join(directoryPath, f); | ||
| const fileContent = readFileSync(filePath, 'utf-8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion (non-blocking): just to be extra safe in case there are many large files, using readFile (async) would prevent from blocking main thread
| logger.info('Pushing stories finished', summary); | ||
| ui.stopAllProgressBars(); | ||
| const failedStories = Math.max(summary.creationResults.failed, summary.processResults.failed, summary.updateResults.failed); | ||
| ui.info(`Push results: ${summary.creationResults.total} ${summary.creationResults.total === 1 ? 'story' : 'stories'} pushed, ${failedStories} ${failedStories === 1 ? 'story' : 'stories'} failed`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: creationResults are the stories placeholders created, but not the full story pushed, correct? If so, maybe we can instead inform with something like stories succeded/skipped to avoid confusion?
alexjoverm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent an internal message with feedback on the approach - blocking the merge for now until discussion.
Introduce a `stories push` CLI command to upload local JSON stories to a space, correctly remapping story references using component schemas and a manifest for resumable runs. This enables safe space-to-space story sync, including circular references and third‑party IDs. Fixes WDX-134
fa26422 to
f368eb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| * Map Asset References in Stories | ||
| */ | ||
| // TODO test | ||
| const hasNewFileReferences = maps.assets.entries().some(([k, v]) => k !== v); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterator helper .some() may fail on older Node.js
High Severity
The call maps.assets.entries().some(([k, v]) => k !== v) uses iterator helpers (.some() on an iterator) which are only available in Node.js 22+ (V8 12.x). Without an engines field in package.json requiring Node.js 22+, users on Node.js 18 or 20 LTS will get a runtime TypeError: maps.assets.entries(...).some is not a function. The fix would be to convert to an array first, e.g. [...maps.assets.entries()].some(...) or Array.from(maps.assets).some(...).
| fetchStoryPages: { total: 0, succeeded: 0, failed: 0 }, | ||
| fetchStories: { total: 0, succeeded: 0, failed: 0 }, | ||
| processResults: { total: 0, succeeded: 0, failed: 0 }, | ||
| updateResults: { total: 0, succeeded: 0, failed: 0 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary totals never initialized, causing negative values
Medium Severity
In mapAssetReferencesInStoriesPipeline, summaries.processResults.total and summaries.updateResults.total are initialized to 0 but never set to the actual story count from setTotalStories. However, onStoryError decrements summaries.updateResults.total by 1 (line 260), causing negative totals. The progress bar is then set to this negative value (line 264), resulting in broken progress reporting. The TODO comment at line 210 indicates this is incomplete work.
Additional Locations (1)
| const { headers } = result; | ||
| const total = Number(headers.get('Total')); | ||
| perPage = Number(headers.get('Per-Page')); | ||
| totalPages = Math.ceil(total / perPage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stories stream missing header fallback causes pagination failure
Medium Severity
The fetchStoriesStream function lacks defensive handling that exists in the equivalent fetchAssetsStream. Line 55 sets perPage = Number(headers.get('Per-Page')) without a fallback (unlike assets which uses || perPage), and line 56 calculates totalPages without Math.max(1, ...). If the API response lacks a Per-Page header, perPage becomes NaN, causing totalPages to be NaN. The loop condition page <= NaN evaluates to false, causing the stream to exit after processing only the first page even when more pages exist.
Add
stories pullcommand to pull stories from a space.Fixes WDX-216
Add
stories pushCLI command to upload local JSON stories to aspace, correctly remapping story references using component schemas and
a manifest for resumable runs. This enables safe space-to-space story
sync, including circular references and third‑party IDs.
Fixes WDX-134
Note
Introduces end-to-end CLI workflows for migrating content between spaces or from external systems.
assetsandstoriescommands withpull/pushsubcommands, docs, and extensive tests--publishand--update-storiesdirectories.logs/reportspaths in logs/reports commands; refactor error imports toutils/error/*; type updates to use MAPI resource typesWritten by Cursor Bugbot for commit f368eb1. This will update automatically on new commits. Configure here.