fix(sync): split global and automatic controls - #28
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR separates global synchronization from automatic synchronization. It gates inbound delivery, outbound planning, recovery, and restore broadcasts. Manual resend remains independent of automatic synchronization and reports a new disabled outcome when global synchronization is off. ChangesSynchronization gating
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HostAdapter
participant SyncRuntime
participant SettingsPort
participant OutboundPlan
participant RemoteDevice
HostAdapter->>SyncRuntime: request manual resend
SyncRuntime->>SettingsPort: load synchronization settings
SettingsPort-->>SyncRuntime: sync_enabled and auto_sync_enabled
SyncRuntime->>OutboundPlan: create resend plan
OutboundPlan->>RemoteDevice: deliver file when sync_enabled is true
RemoteDevice-->>HostAdapter: receive resent file
SyncRuntime-->>HostAdapter: SynchronizationDisabled when sync_enabled is false
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@crates/uc-application/src/clipboard/sync/outbound_plan.rs`:
- Around line 92-97: Update the settings-load error path in the outbound sync
plan construction to return an empty OutboundSyncPlan with no clipboard intent
or files. Ensure this failure handling occurs before the sync_enabled gate so
unavailable settings cannot permit clipboard transmission.
In `@crates/uc-core/src/settings/model.rs`:
- Around line 174-175: Update the auto_sync_enabled field in
crates/uc-core/src/settings/model.rs:174-175 to accept the legacy auto_sync key
during deserialization via an alias, while retaining the current field name and
default behavior. Add a regression test in
crates/uc-core/src/settings/defaults.rs:625-644 covering deserialization of
{"auto_sync": false} and verifying auto_sync_enabled remains false.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e4e56ff-f086-45e6-a8bc-d92bd2bff408
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
CONTEXT.mdCargo.tomlbindings/uc-engine-uniffi/src/runtime.rscrates/uc-application/src/clipboard/inbound/runtime.rscrates/uc-application/src/clipboard/sync/active_state/restore_broadcast_worker.rscrates/uc-application/src/clipboard/sync/outbound_plan.rscrates/uc-application/src/clipboard/sync/resend_entry.rscrates/uc-application/src/clipboard/sync/sync_runtime.rscrates/uc-application/src/settings/models.rscrates/uc-core/src/settings/defaults.rscrates/uc-core/src/settings/model.rscrates/uc-engine/src/assembly/clipboard_runtime.rscrates/uc-engine/src/contract/operation.rscrates/uc-engine/src/contract/settings.rscrates/uc-engine/src/operations/history/resend.rscrates/uc-engine/src/operations/settings/settings.rscrates/uc-engine/src/testing/host_adapter_contract.rscrates/uc-engine/tests/public_contract.rsdocs/architecture/architecture-bible.mdtests/hosts/ohos/engine/oh-package.json5tests/hosts/uc-mobile-probe-core/src/lib.rs
| if !settings.sync.sync_enabled { | ||
| return OutboundSyncPlan { | ||
| clipboard: None, | ||
| files: vec![], | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when settings loading fails.
The early error path at Lines 72-89 returns an enabled clipboard intent before this gate runs. A user with global synchronization disabled can transmit clipboard content when settings storage is unavailable. Return an empty plan on settings-load failure.
Proposed fix
- // Safe default: allow clipboard sync, skip file sync.
return OutboundSyncPlan {
- clipboard: Some(ClipboardSyncIntent {
- snapshot,
- file_transfers: vec![],
- }),
+ clipboard: None,
files: vec![],
};🤖 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/uc-application/src/clipboard/sync/outbound_plan.rs` around lines 92 -
97, Update the settings-load error path in the outbound sync plan construction
to return an empty OutboundSyncPlan with no clipboard intent or files. Ensure
this failure handling occurs before the sync_enabled gate so unavailable
settings cannot permit clipboard transmission.
| pub sync_enabled: bool, | ||
| pub auto_sync_enabled: bool, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 'auto_sync|auto_sync_enabled|sync_enabled' \
crates/uc-core/src/settings \
crates/uc-application/src/settings \
crates/uc-engine/src \
bindings \
testsRepository: UniClipboard/Engine
Length of output: 26512
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- SyncSettings definition and defaults ---'
sed -n '150,195p' crates/uc-core/src/settings/model.rs
sed -n '100,145p' crates/uc-core/src/settings/defaults.rs
printf '%s\n' '--- Settings loading and migration references ---'
rg -n -C 4 'Settings|serde_json::from|serde_json::to|schema_version|migration|auto_sync' \
crates/uc-core/src/settings crates/uc-application/src/settings crates/uc-infra crates/uc-engine \
-g '*.rs' | head -n 500
printf '%s\n' '--- Relevant repository diff ---'
git diff --stat
git diff -- crates/uc-core/src/settings/model.rs crates/uc-core/src/settings/defaults.rsRepository: UniClipboard/Engine
Length of output: 41122
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- All persisted settings read/write call sites ---'
rg -n -C 5 'settings.*(json|read|write|load|save)|from_str|from_slice|to_string|to_vec|to_writer|write_all|read_to_string' \
crates -g '*.rs' | head -n 700
printf '%s\n' '--- SyncSettings serde attributes and explicit migration code ---'
rg -n -C 8 '#\[serde|impl.*Deserialize|deserialize_with|rename|alias|auto_sync' \
crates/uc-core/src/settings crates/uc-infra crates/uc-application crates/uc-engine -g '*.rs' | head -n 700Repository: UniClipboard/Engine
Length of output: 50376
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Settings infrastructure files ---'
fd -t f -i 'settings|migration' crates/uc-infra crates/uc-core | sort
printf '%s\n' '--- SettingsPort implementations and JSON deserialization ---'
rg -n -C 6 'impl .*SettingsPort|SettingsPort for|serde_json::from_(str|slice|reader)|serde_json::to_(string|vec|writer)|settings\.json|CURRENT_SCHEMA_VERSION|MigrationV[0-9]' \
crates/uc-infra crates/uc-core -g '*.rs'
printf '%s\n' '--- Migration tests and fixtures containing sync fields ---'
rg -n -C 8 'auto_sync|sync_enabled|sync_frequency|SyncSettings' \
crates/uc-infra crates/uc-core -g '*.rs' -g '*.json' | head -n 500Repository: UniClipboard/Engine
Length of output: 50378
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
model = Path("crates/uc-core/src/settings/model.rs").read_text()
defaults = Path("crates/uc-core/src/settings/defaults.rs").read_text()
struct = re.search(r'pub struct SyncSettings\s*\{(?P<body>.*?)\n\}', model, re.S)
assert struct, "SyncSettings definition not found"
body = struct.group("body")
assert "pub auto_sync_enabled: bool" in body
assert "pub auto_sync: bool" not in body
field_start = body.index("pub auto_sync_enabled: bool")
preceding = body[max(0, field_start - 160):field_start]
assert "alias = \"auto_sync\"" not in preceding
default_impl = re.search(
r'impl Default for SyncSettings\s*\{.*?Self\s*\{(?P<body>.*?)\n\s*\}\s*\}',
defaults,
re.S,
)
assert default_impl, "SyncSettings default implementation not found"
default_body = default_impl.group("body")
assert re.search(r'\bauto_sync_enabled:\s*true\b', default_body)
print("SyncSettings has no auto_sync field or serde alias.")
print("SyncSettings::default() sets auto_sync_enabled to true.")
print("Therefore derived serde deserialization treats legacy auto_sync as unknown and uses true.")
PYRepository: UniClipboard/Engine
Length of output: 355
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- uc-infra settings migration ---'
wc -l crates/uc-infra/src/settings/migration.rs
cat -n crates/uc-infra/src/settings/migration.rs
printf '%s\n' '--- SettingsPort implementations ---'
rg -n -C 12 'SettingsPort' crates -g '*.rs' | grep -E 'SettingsPort|impl|async fn load|async fn save|from_str|from_slice|to_string|to_vec|settings_path' | head -n 300
printf '%s\n' '--- Actual settings file reads and writes ---'
rg -n -C 10 'settings_path|settings\.json|read_to_string|write_all|to_string_pretty|to_vec_pretty' \
crates/uc-infra crates/uc-engine crates/uc-application -g '*.rs' | head -n 500Repository: UniClipboard/Engine
Length of output: 50377
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cat -n crates/uc-infra/src/settings/repository.rs | sed -n '1,285p'
printf '%s\n' '--- construction and migration wiring ---'
rg -n -C 12 'FileSettingsRepository::new|SettingsMigrator|migrate_to_latest|settings_repo' \
crates/uc-engine/src crates/uc-infra/src -g '*.rs' | head -n 500Repository: UniClipboard/Engine
Length of output: 50377
Preserve the legacy auto_sync value during deserialization.
FileSettingsRepository deserializes settings directly, and no migration handles auto_sync. An existing { "auto_sync": false } configuration therefore uses the default auto_sync_enabled: true and re-enables automatic synchronization.
Add a deserialization alias for auto_sync and a regression test for this case.
📍 Affects 2 files
crates/uc-core/src/settings/model.rs#L174-L175(this comment)crates/uc-core/src/settings/defaults.rs#L625-L644
🤖 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/uc-core/src/settings/model.rs` around lines 174 - 175, Update the
auto_sync_enabled field in crates/uc-core/src/settings/model.rs:174-175 to
accept the legacy auto_sync key during deserialization via an alias, while
retaining the current field name and default behavior. Add a regression test in
crates/uc-core/src/settings/defaults.rs:625-644 covering deserialization of
{"auto_sync": false} and verifying auto_sync_enabled remains false.
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Documentation