separate task sync roomote control #7799
Conversation
dosubot
bot
added
the
size:XL
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Typo: The property name 'featureRoomoteControlEnabled' appears to be misspelled. It likely should be 'featureRemoteControlEnabled'.
| featureRoomoteControlEnabled: false, | |
| featureRemoteControlEnabled: false, |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for this PR that separates Task Sync and Roomote Control functionality!
I've reviewed the changes and have a few observations:
Positive Changes ✅
- Good separation of concerns between Task Sync and Roomote Control
- Proper dependency management (Roomote Control requires Task Sync)
- Comprehensive test updates for the new isTaskSyncEnabled() methods
- UI properly reflects the relationship between the two features
Questions/Suggestions 🤔
-
BridgeOrchestrator.isEnabled() parameter change: I noticed the calls to BridgeOrchestrator.isEnabled() were changed from using remoteControlEnabled to taskSyncEnabled (lines 887 and 2493 in ClineProvider.ts). Is this intentional? It seems like the bridge should be enabled based on remote control status rather than task sync status, since the bridge is for remote control functionality.
-
Null safety in CloudSettingsService: The isTaskSyncEnabled() method in CloudSettingsService.ts (line 271) calls this.authService.getStoredOrganizationId() without checking if authService exists. While the constructor ensures it's initialized, adding a defensive check might prevent potential runtime errors.
-
Test coverage: Consider adding a test for StaticSettingsService.isTaskSyncEnabled() to match the test coverage of the other services.
-
Naming consistency: Throughout the codebase, I see both 'Roomote Control' and 'Remote Control'. Should we standardize on one spelling?
-
Documentation: It would be helpful to add a comment explaining why Roomote Control depends on Task Sync, as this relationship might not be immediately obvious to future maintainers.
Overall, this is a well-structured change that properly separates these two features while maintaining their relationship. The code is clean and the tests are comprehensive.
hannesrudolph
added
the
Issue/PR - Triage
jr
force-pushed
the
jr/feat/separate-task-sync-roomote-control
branch
3 times, most recently
from
399fe93 to
578acc2
Compare
- Add new taskSyncEnabled setting to control task content syncing - Keep remoteControlEnabled for Roomote Control functionality - Task Sync controls whether task content is sent to cloud - Roomote Control controls whether cloud can send instructions back - Roomote Control now depends on Task Sync being enabled - Usage metrics (tokens, cost) always reported regardless of settings - Update UI with two separate toggles and clear descriptions - Add info text explaining usage metrics are always reported
…ings - Added taskSync, taskSyncDescription, remoteControlRequiresTaskSync, and usageMetricsAlwaysReported keys to all non-English cloud.json files - Updated cloudBenefit keys to match English structure - Ensured all languages have consistent translation keys for the new Task Sync and Roomote Control features
jr
force-pushed
the
jr/feat/separate-task-sync-roomote-control
branch
from
578acc2 to
f4334b1
Compare
hannesrudolph
added
PR - Draft / In Progress
and removed
Issue/PR - Triage
Update / cleanup of #7634
Important
This PR introduces task synchronization functionality, updates remote control to depend on task sync, and modifies UI components and tests to support these changes.
isTaskSyncEnabled()toCloudService.tsandCloudSettingsService.tsto check task sync status based on user and organization settings.TelemetryClient.tsto useisTaskSyncEnabled()for task message telemetry.ClineProvider.tsto usetaskSyncEnabledinstead ofremoteControlEnabled.CloudView.tsxto include task sync toggle and manage its state based on user and organization settings.i18n.CloudView.spec.tsx.ExtensionStateContext.spec.tsxto test new state properties for task sync.remoteControlEnabledfromglobal-settings.tsand related files.ExtensionMessage.tsandWebviewMessage.tsto includetaskSyncEnabled.This description was created by
for f4334b1. You can customize this summary. It will automatically update as commits are pushed.