CS-10617: Reimplement boxel workspace push command#4356
CS-10617: Reimplement boxel workspace push command#4356
Conversation
|
now that we have real realm integration don't forget to add tests for this against a running realm |
FadhlanR
force-pushed
the
cs-10616-reimplement-boxel-pull-command
branch
from
a0bf836 to
d20a12f
Compare
FadhlanR
force-pushed
the
cs-10617-reimplement-boxel-push-command
branch
from
77e92a1 to
61792e6
Compare
FadhlanR
force-pushed
the
cs-10616-reimplement-boxel-pull-command
branch
from
b407dcf to
6d437cb
Compare
Port push command from standalone boxel-cli into monorepo. Adds manifest-based incremental sync via .boxel-sync.json with MD5 hashing. Supports --delete, --dry-run, --force options. Protected files (.realm.json) are never uploaded or deleted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d integration tests - Move push command from src/commands/push.ts to src/commands/realm/push.ts - Use ProfileManager for auth instead of validateMatrixEnvVars - Register push as realm subcommand (boxel realm push) - Replace mock-based unit tests with real integration tests using fileSystem seeding - Test cases: basic push, incremental, force, delete, dry-run, manifest ignore Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
FadhlanR
changed the base branch from
cs-10616-reimplement-boxel-pull-command
to
main
FadhlanR
force-pushed
the
cs-10617-reimplement-boxel-push-command
branch
from
61792e6 to
ab6364f
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab6364f6f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const currentHash = computeFileHash(localPath); | ||
| const previousHash = manifest.files[relativePath]; | ||
|
|
||
| if (previousHash !== currentHash) { | ||
| filesToUpload.set(relativePath, localPath); |
There was a problem hiding this comment.
The incremental branch only compares currentHash against the local manifest and never checks whether the file still exists (or still matches) on the remote realm, so a file deleted or edited remotely after a prior push is treated as "unchanged" and skipped. In that scenario, rerunning boxel realm push without local edits leaves the realm out of sync until users manually force a full upload (--force or deleting the manifest), which can silently preserve remote drift.
Useful? React with 👍 / 👎.
| continue; | ||
| } | ||
| const currentHash = computeFileHash(localPath); | ||
| const previousHash = manifest.files[relativePath]; |
There was a problem hiding this comment.
A parseable but malformed .boxel-sync.json (for example, missing files after a partial write or manual edit) will crash here with TypeError because manifest.files is dereferenced without validation. Since this path runs whenever workspaceUrl matches, a bad manifest can block all non---force pushes instead of safely falling back to a full upload.
Useful? React with 👍 / 👎.
| await pushCommand(localDir, realmUrl, { profileManager }); | ||
|
|
||
| let remoteFiles = await pullRemoteFiles(realmUrl); | ||
| expect(remoteFiles.get('a.gts')).toContain('a = 999'); | ||
| expect(remoteFiles.get('b.gts')).toContain('b = 2'); |
There was a problem hiding this comment.
let's not verify this way--lets actually look at the server. beacuse you are verifying in the same local dir you wrote too, this doesn't demonstraibly prove that you actually mutated server state. also it's awkward that the push command relies on teh pull command for testing.
| // Push again with force (no changes) — should still succeed | ||
| await pushCommand(localDir, realmUrl, { force: true, profileManager }); | ||
|
|
||
| let remoteFiles = await pullRemoteFiles(realmUrl); |
There was a problem hiding this comment.
ditto--assert realm server state directly
| fs.unlinkSync(path.join(localDir, 'remove.gts')); | ||
| await pushCommand(localDir, realmUrl, { delete: true, profileManager }); | ||
|
|
||
| let remoteFiles = await pullRemoteFiles(realmUrl); |
There was a problem hiding this comment.
ditto--assert realm server state directly
| expect(fs.existsSync(path.join(localDir, '.boxel-sync.json'))).toBe(false); | ||
|
|
||
| // Remote should not have the file (only index.json from realm creation) | ||
| let remoteFiles = await pullRemoteFiles(realmUrl); |
There was a problem hiding this comment.
ditto--assert realm server state directly
|
|
||
| await pushCommand(localDir, realmUrl, { profileManager }); | ||
|
|
||
| let remoteFiles = await pullRemoteFiles(realmUrl); |
There was a problem hiding this comment.
ditto--assert realm server state directly
| expect(Object.keys(manifest.files)).toContain('data.json'); | ||
|
|
||
| // Verify files exist on remote via pull | ||
| let remoteFiles = await pullRemoteFiles(realmUrl); |
There was a problem hiding this comment.
ditto--assert realm server state directly
|
|
||
| for (const [relativePath, localPath] of filesToUpload) { | ||
| try { | ||
| await this.uploadFile(relativePath, localPath); |
There was a problem hiding this comment.
this is doing the uploads serially. I thin kit might be better to use an atomic write, or something that is able to deal with changes concurrently
| force?: boolean; | ||
| } | ||
|
|
||
| class RealmPusher extends RealmSyncBase { |
There was a problem hiding this comment.
I'm super confused--i thought we were reimplementing this, but RealmSyncBase is already in the repo? which commit added that? we are we not reimplementing that too?
There was a problem hiding this comment.
RealmSyncBase is added here: 72d1f10#diff-85179b7b382ff103db64d2324166436d7ef6db399991b68fea86fe9a08376a73
There was a problem hiding this comment.
why is your implementation of this so close to the existing boxel-cli?
|
Are we really re-implementing this? i just compared your changes to what is currently in boxel-cli repo and they are the same. re-implementing is not the same as copying. i'm confused what's going on. Let's talk about this in office hours |
so per our discussion with @lukemelia yes, we are copying this over and making targetted adjustments as we see them. sorry for the confusion! |
2 participants
Summary
boxel realm pushcommand from standalone boxel-cli into monorepo.boxel-sync.jsonwith MD5 hashing--delete(remove remote-only files),--dry-run,--force(upload all).realm.json) are never uploaded or deletedTest plan
pnpm --filter @cardstack/boxel-cli test— 38 tests passpnpm --filter @cardstack/boxel-cli build— builds successfully (101.1 KB)pnpm --filter @cardstack/boxel-cli lint— no errors🤖 Generated with Claude Code