-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: add settings update broadcast support for multi-instance synchronization #7833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ import { | |
|
|
||
| import { getRooCodeApiUrl } from "./config.js" | ||
| import { RefreshTimer } from "./RefreshTimer.js" | ||
| import { BridgeOrchestrator } from "./bridge/index.js" | ||
|
|
||
| const ORGANIZATION_SETTINGS_CACHE_KEY = "organization-settings" | ||
| const USER_SETTINGS_CACHE_KEY = "user-settings" | ||
|
|
@@ -104,6 +105,37 @@ export class CloudSettingsService extends EventEmitter<SettingsServiceEvents> im | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Set up listener for settings update events from other instances | ||
| * This will be called by CloudService after initialization | ||
| */ | ||
| public setupBridgeListener(): void { | ||
| // This method will be called to set up listening for remote settings updates | ||
| // The actual implementation will be handled through the BridgeOrchestrator | ||
| // which will call handleRemoteSettingsUpdate when events are received | ||
| } | ||
|
|
||
| /** | ||
| * Handle remote settings update events from other instances | ||
| * This method should be called by the BridgeOrchestrator when a settings update event is received | ||
| */ | ||
| public handleRemoteSettingsUpdate(versions: { organization: number; user: number }): void { | ||
| // 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) => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| this.log("[cloud-settings] Error refetching settings after update event:", error) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| private shouldRefetchSettings(eventVersions: { organization: number; user: number }): boolean { | ||
| const currentOrgVersion = this.settings?.version ?? -1 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor suggestion: The magic number |
||
| const currentUserVersion = this.userSettings?.version ?? -1 | ||
|
|
||
| return eventVersions.organization > currentOrgVersion || eventVersions.user > currentUserVersion | ||
| } | ||
|
|
||
| private async fetchSettings(): Promise<boolean> { | ||
| const token = this.authService.getSessionToken() | ||
|
|
||
|
|
@@ -257,6 +289,9 @@ export class CloudSettingsService extends EventEmitter<SettingsServiceEvents> im | |
| this.userSettings = result.data | ||
| await this.cacheSettings() | ||
| this.emit("settings-updated", {} as Record<string, never>) | ||
|
|
||
| // Broadcast settings update event to other instances | ||
| this.broadcastSettingsUpdate() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Race condition consideration: What happens if multiple instances try to update settings simultaneously? The 409 conflict is handled in |
||
| } | ||
|
|
||
| return true | ||
|
|
@@ -266,6 +301,24 @@ export class CloudSettingsService extends EventEmitter<SettingsServiceEvents> im | |
| } | ||
| } | ||
|
|
||
| private broadcastSettingsUpdate(): void { | ||
| // Broadcast settings update event to other instances | ||
| const bridge = BridgeOrchestrator.getInstance() | ||
| if (!bridge) { | ||
| return | ||
| } | ||
|
|
||
| const versions = { | ||
| organization: this.settings?.version ?? 0, | ||
| user: this.userSettings?.version ?? 0, | ||
| } | ||
|
|
||
| // Use the public method to publish settings update | ||
| bridge.publishSettingsUpdate(versions).catch((error) => { | ||
| this.log("[cloud-settings] Error broadcasting settings update event:", error) | ||
| }) | ||
| } | ||
|
|
||
| private async removeSettings(): Promise<void> { | ||
| this.settings = undefined | ||
| this.userSettings = undefined | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.