Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/cloud/src/CloudService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,9 @@ export class CloudService extends EventEmitter<CloudServiceEvents> implements Di
cloudSettingsService.on("settings-updated", this.settingsListener)
await cloudSettingsService.initialize()

// Set up bridge listener for settings updates from other instances
cloudSettingsService.setupBridgeListener()

this._settingsService = cloudSettingsService
}

Expand Down
53 changes: 53 additions & 0 deletions packages/cloud/src/CloudSettingsService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Copy link
Contributor Author

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.

// 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) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor suggestion: The magic number -1 here could be a named constant like UNINITIALIZED_VERSION for better clarity.

const currentUserVersion = this.userSettings?.version ?? -1

return eventVersions.organization > currentOrgVersion || eventVersions.user > currentUserVersion
}

private async fetchSettings(): Promise<boolean> {
const token = this.authService.getSessionToken()

Expand Down Expand Up @@ -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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 updateUserSettings(), but the broadcast flow doesn't seem to account for this scenario. Should we add retry logic or conflict resolution?

}

return true
Expand All @@ -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
Expand Down
Loading
Loading