Skip to content
Merged
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
5 changes: 2 additions & 3 deletions src/extensions/core/cloudRemoteConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { watchDebounced } from '@vueuse/core'

import { useCurrentUser } from '@/composables/auth/useCurrentUser'
import { useSubscription } from '@/platform/cloud/subscription/composables/useSubscription'
import { loadRemoteConfig } from '@/platform/remoteConfig/remoteConfig'
import { refreshRemoteConfig } from '@/platform/remoteConfig/refreshRemoteConfig'
import { useExtensionService } from '@/services/extensionService'

Expand All @@ -26,7 +25,7 @@ useExtensionService().registerExtension({
{ debounce: 256, immediate: true }
)

// Poll for config updates every 10 minutes
setInterval(() => void loadRemoteConfig(), 600_000)
// Poll for config updates every 10 minutes (with auth)
setInterval(() => void refreshRemoteConfig(), 600_000)
}
})
6 changes: 3 additions & 3 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ import { i18n } from './i18n'
import { isCloud } from '@/platform/distribution/types'

if (isCloud) {
const { loadRemoteConfig } =
await import('@/platform/remoteConfig/remoteConfig')
await loadRemoteConfig()
const { refreshRemoteConfig } =
await import('@/platform/remoteConfig/refreshRemoteConfig')
await refreshRemoteConfig({ useAuth: false })
}

const ComfyUIPreset = definePreset(Aura, {
Expand Down
109 changes: 109 additions & 0 deletions src/platform/remoteConfig/refreshRemoteConfig.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'

import { api } from '@/scripts/api'

import { refreshRemoteConfig } from './refreshRemoteConfig'
import { remoteConfig } from './remoteConfig'

vi.mock('@/scripts/api', () => ({
api: {
fetchApi: vi.fn()
}
}))

global.fetch = vi.fn()
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nit: I usually like stubGlobal for consistency and control


describe('refreshRemoteConfig', () => {
const mockConfig = { feature1: true, feature2: 'value' }

beforeEach(() => {
vi.clearAllMocks()
remoteConfig.value = {}
window.__CONFIG__ = {}
})

describe('with auth (default)', () => {
it('uses api.fetchApi when useAuth is true', async () => {
vi.mocked(api.fetchApi).mockResolvedValue({
ok: true,
json: async () => mockConfig
} as Response)
Comment on lines +26 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need the type assertion?
Could you get away with satisfies Partial<Response>?


await refreshRemoteConfig({ useAuth: true })

expect(api.fetchApi).toHaveBeenCalledWith('/features', {
cache: 'no-store'
})
expect(global.fetch).not.toHaveBeenCalled()
expect(remoteConfig.value).toEqual(mockConfig)
expect(window.__CONFIG__).toEqual(mockConfig)
})

it('uses api.fetchApi by default', async () => {
vi.mocked(api.fetchApi).mockResolvedValue({
ok: true,
json: async () => mockConfig
} as Response)

await refreshRemoteConfig()

expect(api.fetchApi).toHaveBeenCalled()
expect(global.fetch).not.toHaveBeenCalled()
})
})

describe('without auth', () => {
it('uses raw fetch when useAuth is false', async () => {
vi.mocked(global.fetch).mockResolvedValue({
ok: true,
json: async () => mockConfig
} as Response)
Comment on lines +57 to +60
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This pattern seems repeated enough times in here to be worth extracting.


await refreshRemoteConfig({ useAuth: false })

expect(global.fetch).toHaveBeenCalledWith('/api/features', {
cache: 'no-store'
})
expect(api.fetchApi).not.toHaveBeenCalled()
expect(remoteConfig.value).toEqual(mockConfig)
expect(window.__CONFIG__).toEqual(mockConfig)
})
})

describe('error handling', () => {
it('clears config on 401 response', async () => {
vi.mocked(api.fetchApi).mockResolvedValue({
ok: false,
status: 401,
statusText: 'Unauthorized'
} as Response)

await refreshRemoteConfig()

expect(remoteConfig.value).toEqual({})
expect(window.__CONFIG__).toEqual({})
})

it('clears config on 403 response', async () => {
vi.mocked(api.fetchApi).mockResolvedValue({
ok: false,
status: 403,
statusText: 'Forbidden'
} as Response)

await refreshRemoteConfig()

expect(remoteConfig.value).toEqual({})
expect(window.__CONFIG__).toEqual({})
})

it('clears config on fetch error', async () => {
vi.mocked(api.fetchApi).mockRejectedValue(new Error('Network error'))

await refreshRemoteConfig()

expect(remoteConfig.value).toEqual({})
expect(window.__CONFIG__).toEqual({})
})
Comment on lines +73 to +107
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider adding a test for non-auth error statuses (e.g., 500).

The current tests cover 401 and 403 (which clear config) and fetch errors. However, the implementation preserves the existing config for other non-ok statuses like 500. Adding a test for this scenario would document the expected behavior and prevent regressions.

📝 Suggested additional test case
it('preserves config on 500 response', async () => {
  remoteConfig.value = { existingFeature: true }
  window.__CONFIG__ = { existingFeature: true }

  vi.mocked(api.fetchApi).mockResolvedValue({
    ok: false,
    status: 500,
    statusText: 'Internal Server Error'
  } as Response)

  await refreshRemoteConfig()

  // Config should NOT be cleared on transient server errors
  expect(remoteConfig.value).toEqual({ existingFeature: true })
  expect(window.__CONFIG__).toEqual({ existingFeature: true })
})
🤖 Prompt for AI Agents
In `@src/platform/remoteConfig/refreshRemoteConfig.test.ts` around lines 73 - 107,
Add a test that verifies non-auth server errors do not clear config: set
remoteConfig.value and window.__CONFIG__ to a sample object, mock api.fetchApi
to resolve with a Response-like object where ok: false and status: 500
(statusText 'Internal Server Error'), call refreshRemoteConfig(), and assert
that remoteConfig.value and window.__CONFIG__ remain equal to the original
object; reference refreshRemoteConfig, remoteConfig, window.__CONFIG__, and
api.fetchApi when locating where to add the test.

})
})
25 changes: 23 additions & 2 deletions src/platform/remoteConfig/refreshRemoteConfig.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I know it's not coming from this PR, but this feels like it should be a part of the remoteConfig module, not in a separate file.

Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,28 @@ import { api } from '@/scripts/api'

import { remoteConfig } from './remoteConfig'

export async function refreshRemoteConfig(): Promise<void> {
interface RefreshRemoteConfigOptions {
/**
* Whether to use authenticated API (default: true).
* Set to false during bootstrap before auth is initialized.
*/
useAuth?: boolean
}

/**
* Loads remote configuration from the backend /features endpoint
* and updates the reactive remoteConfig ref
*/
export async function refreshRemoteConfig(
options: RefreshRemoteConfigOptions = {}
): Promise<void> {
const { useAuth = true } = options

try {
const response = await api.fetchApi('/features', { cache: 'no-store' })
const response = useAuth
? await api.fetchApi('/features', { cache: 'no-store' })
: await fetch('/api/features', { cache: 'no-store' })

if (response.ok) {
const config = await response.json()
window.__CONFIG__ = config
Expand All @@ -19,5 +38,7 @@ export async function refreshRemoteConfig(): Promise<void> {
}
} catch (error) {
console.error('Failed to fetch remote config:', error)
window.__CONFIG__ = {}
remoteConfig.value = {}
}
}
24 changes: 0 additions & 24 deletions src/platform/remoteConfig/remoteConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
* - Avoiding vendor lock-in for native apps
*
* This module is tree-shaken in OSS builds.
* Used for initial config load in main.ts and polling in the extension.
*/

import { ref } from 'vue'
Expand All @@ -29,26 +28,3 @@ export function configValueOrDefault<K extends keyof RemoteConfig>(
const configValue = remoteConfig[key]
return configValue || defaultValue
}

/**
* Loads remote configuration from the backend /api/features endpoint
* and updates the reactive remoteConfig ref
*/
export async function loadRemoteConfig(): Promise<void> {
try {
const response = await fetch('/api/features', { cache: 'no-store' })
if (response.ok) {
const config = await response.json()
window.__CONFIG__ = config
remoteConfig.value = config
} else {
console.warn('Failed to load remote config:', response.statusText)
window.__CONFIG__ = {}
remoteConfig.value = {}
}
} catch (error) {
console.error('Failed to fetch remote config:', error)
window.__CONFIG__ = {}
remoteConfig.value = {}
}
}