feat: add settings update broadcast support for multi-instance synchronization #7833
Conversation
…onization - Add SettingsUpdated event to ExtensionBridgeEventName enum - Define event payload schema with organization and user version numbers - Implement broadcasting of settings updates in CloudSettingsService - Add listener for remote settings updates with version checking - Update ExtensionChannel to publish settings update events - Configure BridgeOrchestrator to route settings update events - Add comprehensive tests for settings broadcast functionality This enables automatic settings synchronization across multiple connected extension instances when one instance updates settings.
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| // We use dynamic import to avoid circular dependency | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const { CloudService } = require("../CloudService.js") as { |
There was a problem hiding this comment.
Is using require() here intentional? This dynamic import pattern to avoid circular dependencies feels like a code smell. Could we refactor the module structure to avoid this, perhaps using dependency injection or an event bus pattern instead?
| // Check if we should refetch settings based on version numbers | ||
| if (this.shouldRefetchSettings(versions)) { | ||
| this.log("[cloud-settings] Received settings update event with newer versions, refetching...") | ||
| this.fetchSettings().catch((error) => { |
There was a problem hiding this comment.
The fetchSettings() call here doesn't have a timeout or retry limit. Could this potentially lead to infinite retry loops if the API is consistently failing? Consider adding a max retry count or exponential backoff.
| * Set up listener for settings update events from other instances | ||
| * This will be called by CloudService after initialization | ||
| */ | ||
| public setupBridgeListener(): void { |
There was a problem hiding this comment.
I notice this method is just a placeholder. Should this be an abstract method or have an actual implementation? The comment suggests the BridgeOrchestrator will handle it, but having an empty method might be confusing.
| } | ||
|
|
||
| private shouldRefetchSettings(eventVersions: { organization: number; user: number }): boolean { | ||
| const currentOrgVersion = this.settings?.version ?? -1 |
There was a problem hiding this comment.
Minor suggestion: The magic number -1 here could be a named constant like UNINITIALIZED_VERSION for better clarity.
| this.emit("settings-updated", {} as Record<string, never>) | ||
|
|
||
| // Broadcast settings update event to other instances | ||
| this.broadcastSettingsUpdate() |
There was a problem hiding this comment.
Race condition consideration: What happens if multiple instances try to update settings simultaneously? The 409 conflict is handled in updateUserSettings(), but the broadcast flow doesn't seem to account for this scenario. Should we add retry logic or conflict resolution?
| cloudSettingsService.dispose() | ||
| }) | ||
|
|
||
| describe("broadcastSettingsUpdate", () => { |
There was a problem hiding this comment.
Test coverage suggestion: Consider adding tests for concurrent update scenarios and network partition cases where events might be delayed or reordered. These edge cases could reveal interesting race conditions.
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
|
Closing for now, not sure who triggered this PR but it seems incomplete. |
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
Overview
This PR implements support for broadcasting settings update events through the extension bridge, enabling automatic settings synchronization across multiple connected extension instances.
Problem
When multiple extension instances are connected, updating settings in one instance doesn't automatically reflect in other instances, requiring manual refresh or restart.
Solution
Implemented a broadcast mechanism that:
Implementation Details
Event Flow
CloudSettingsService.updateUserSettings()SettingsUpdatedevent through ExtensionChannelfetchSettings()Key Changes
SettingsUpdatedtoExtensionBridgeEventNameenum with version payloadTesting
Review Confidence
Code review confidence: 92% (High)
Important
Adds settings update broadcast support for multi-instance synchronization via extension bridge.
SettingsUpdatedevent toExtensionBridgeEventNameincloud.ts.CloudSettingsServiceemitsSettingsUpdatedevent after settings update and broadcasts viaBridgeOrchestrator.BridgeOrchestratorlistens forSettingsUpdatedevents and notifiesCloudSettingsService.ExtensionChannelhandles publishing ofSettingsUpdatedevents.CloudSettingsServicechecks version numbers inhandleRemoteSettingsUpdate()to decide on refetching settings.BridgeOrchestratorroutes settings update events and uses dynamic import to avoid circular dependencies.ExtensionChannelincludespublishSettingsUpdate()for broadcasting.CloudSettingsService.broadcast.test.tsfor broadcasting and handling remote updates.This description was created by
for 35da5cc. You can customize this summary. It will automatically update as commits are pushed.