Skip to content

feat(minimal): support configuring the file sync during activate - #776

Merged
twitchyliquid64 merged 1 commit into
mainfrom
tom/attach
Jul 16, 2026
Merged

feat(minimal): support configuring the file sync during activate#776
twitchyliquid64 merged 1 commit into
mainfrom
tom/attach

Conversation

@twitchyliquid64

@twitchyliquid64 twitchyliquid64 commented Jul 15, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Member

Also need this so i can experiment with git push

Summary by CodeRabbit

  • New Features

    • Added a --sync option to the activate command.
    • Projects can now be synchronized using a tarball or activated without uploading workspace files.
    • Tarball synchronization remains the default behavior.

  • Tests

    • Updated activation coverage to explicitly verify tarball-based synchronization.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The activate command adds a configurable synchronization mode. Tarball synchronization remains the default, while None skips uploading workspace files to the daemon. Related integration tests explicitly select tarball synchronization.

Changes

Activation synchronization

Layer / File(s) Summary
Sync mode contract
crates/minimal/src/lib.rs		 Adds the public SyncMode enum and the --sync argument on ActivateArgs, defaulting to tarball synchronization.
Conditional project upload
crates/minimal/src/lib.rs, crates/minimal/tests/cli.rs		 Activation uploads project files only for SyncMode::Tarball; integration tests explicitly configure that mode.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: 0chroma

Poem

I’m a bunny with files in a tarball bright,
Or skipping the upload with a hop out of sight.
--sync picks the path for the day,
While tests keep the carrots safely in play.
Hop, hop—activation leads the way!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: configurable file syncing during activate.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@twitchyliquid64
twitchyliquid64 enabled auto-merge (squash) July 15, 2026 23:31
coderabbitai[bot]
coderabbitai Bot reviewed Jul 15, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/minimal/tests/cli.rs (1)

221-221: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for the new synchronization modes.

These tests explicitly select Tarball, so they do not verify the clap default or the new SyncMode::None behavior. Add a parser test for omitted --sync and an activation test confirming that None leaves project files absent.

As per coding guidelines, run cargo test -p minimal for this Rust change.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimal/tests/cli.rs` at line 221, Add tests covering the new
synchronization modes: verify CLI parsing defaults to the expected mode when
--sync is omitted, and verify activation with SyncMode::None leaves project
files absent. Extend the relevant parser and activation tests near the existing
SyncMode::Tarball coverage, then run cargo test -p minimal.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/minimal/tests/cli.rs`:
- Line 221: Add tests covering the new synchronization modes: verify CLI parsing
defaults to the expected mode when --sync is omitted, and verify activation with
SyncMode::None leaves project files absent. Extend the relevant parser and
activation tests near the existing SyncMode::Tarball coverage, then run cargo
test -p minimal.
ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 427aaea1-49a3-4009-9528-6ec290305125

📥 Commits

Reviewing files that changed from the base of the PR and between 704d36c and 7825b39.

📒 Files selected for processing (2)
  • crates/minimal/src/lib.rs
  • crates/minimal/tests/cli.rs

@twitchyliquid64
twitchyliquid64 merged commit b69b429 into main Jul 16, 2026
27 checks passed
@twitchyliquid64
twitchyliquid64 deleted the tom/attach branch July 16, 2026 00:01
This was referenced Jul 20, 2026
Merged
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Jul 30, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@norrietaylor norrietaylor norrietaylor approved these changes
@0chroma 0chroma Awaiting requested review from 0chroma

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@twitchyliquid64 @norrietaylor