pull-reprint: stop touching internal Reprint state directly - #4491
Conversation
pull-reprint: stop touching internal Reprint state directly
There was a problem hiding this comment.
To reiterate: I will remove this file before merging. It's here to make testing easier.
📊 Performance Test ResultsComparing b8ef175 vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
| savePullSelection( session, selection ); | ||
| return selection; | ||
| } | ||
| const tree = await fetchJetpackPullTree( wpComAccessToken, wpComSiteId ); |
There was a problem hiding this comment.
With the raw-scratch reset removed, how does a pull recover when raw/ is non-empty and no cursor exists? Studio does not pass --on-fs-root-nonempty, so Reprint falls back to error and throws "Filesystem root is not empty and no cursor found". Does the move to state/remotes/<md5(url)>/pull/state.json put existing scratches from older builds into exactly that state?
There was a problem hiding this comment.
Good catch 👍
The situation was a little different when this logic was implemented in #3976, but the only realistic situation where we'd get a non-empty fs-root and no cursor today is if the Reprint API URL changes, because that's what generates the state/remotes/<md5(url)> path.
After shipping this project, that won't happen, but it will definitely happen for us engineers working on this before then, because the code flips between the "v1" and "v2" API endpoints based on availability (and we just had to revert the v2 API endpoints in Automattic/jetpack#51179).
In any case: there's no legitimate case to keep this when viewing it from the end user's perspective now. We engineers might have to do some cleanup when the Jetpack API endpoint is reinstated, but that's it.
As for --on-fs-root-nonempty – that's going away when WordPress/reprint#529 lands, so there's no point in starting to pass that.
|
Thanks for the review, @gavande1! Several good feedback items 👍 My work is a bit spread out atm. I'll try to consolidate it, but it may be best to inherit one or two of these issues into another PR. I've pushed fixes related to the No need to review again just yet. I'll revisit all of this tomorrow |
Related issues
--target-*options for theapply-runtimecommand WordPress/reprint#540How AI was used in this PR
Codex wrote the whole thing based on a detailed upfront plan that I drafted with Claude. The process was highly iterative.
Proposed Changes
The
pull-reprintcommand in Studio currently both reads and writes private Reprint state. This was implemented as a temporary measure to make selective sync work, and I'm now working on making the necessary upstream changes and then adapting Studio accordingly.Here are the specific changes in this PR:
import-metadatacommand (see Expose source layout and pull artifact status through import-metadata WordPress/reprint#436).pullcommand to get the file tree.pull-files --onlyshould support file paths, not just directories WordPress/reprint#539), the file selector has been made to only display directories. This should be reverted once we have an upstream fix in place.--target-engineand--target-sqlite-pathto theapply-runtimeReprint command. If a user pulls a remote site for the first time and deselects the database, this ensures Reprint still knows to configure the site to use a SQLite database. This part depends on--target-*options for theapply-runtimecommand WordPress/reprint#540.This PR depends on WordPress/reprint#436 and WordPress/reprint#540 being included in the bundled PHAR. To make things easier for testers, I've simply committed the patched PHAR.
Testing Instructions
npm run cli:buildcdinto itSTUDIO_ENABLE_PULL_REPRINT=1 studiodev pull-reprintPre-merge Checklist