Skip to content
6 changes: 6 additions & 0 deletions browser_tests/fixtures/ComfyPage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { TestIds } from '@e2e/fixtures/selectors'
import { comfyExpect } from '@e2e/fixtures/utils/customMatchers'
import { assetPath } from '@e2e/fixtures/utils/paths'
import { nextFrame, sleep } from '@e2e/fixtures/utils/timing'
import { mockWorkspace, workspace } from '@e2e/fixtures/utils/workspaceMocks'
import { VueNodeHelpers } from '@e2e/fixtures/VueNodeHelpers'
import { BottomPanel } from '@e2e/fixtures/components/BottomPanel'
import { ComfyNodeSearchBox } from '@e2e/fixtures/components/ComfyNodeSearchBox'
Expand Down Expand Up @@ -564,6 +565,11 @@ export const comfyPageFixture = base.extend<{
}

if (testInfo.tags.includes('@cloud')) {
const context = page.context()
await context.route('**/api/auth/session', (route) =>
route.fulfill({ status: 204 })
)
await mockWorkspace(context, workspace('personal', 'owner'), [])
await comfyPage.cloudAuth.mockAuth()
}

Expand Down
18 changes: 9 additions & 9 deletions browser_tests/fixtures/utils/workspaceMocks.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { Page } from '@playwright/test'
import type { BrowserContext, Page } from '@playwright/test'

import type {
Member,
Expand Down Expand Up @@ -38,10 +38,10 @@ export function member(
* this the mint fails and auth cannot resolve the active workspace.
*/
export async function mockWorkspaceTokenMint(
page: Page,
routeTarget: Page | BrowserContext,
ws: Pick<WorkspaceWithRole, 'id' | 'name' | 'type' | 'role'>
) {
await page.route('**/api/auth/token', (r) =>
await routeTarget.route('**/api/auth/token', (r) =>
r.fulfill(
jsonRoute({
token: 'mock-workspace-token',
Expand All @@ -60,10 +60,10 @@ export async function mockWorkspaceTokenMint(
* reload.
*/
export async function mockWorkspaceList(
page: Page,
routeTarget: Page | BrowserContext,
workspaces: WorkspaceWithRole[]
): Promise<void> {
await page.route('**/api/workspaces', async (route) => {
await routeTarget.route('**/api/workspaces', async (route) => {
if (route.request().method() !== 'GET') return route.fallback()
await route.fulfill(jsonRoute({ workspaces }))
})
Expand All @@ -74,13 +74,13 @@ export async function mockWorkspaceList(
* given workspace with the given roster (drives the original-owner gate).
*/
export async function mockWorkspace(
page: Page,
routeTarget: Page | BrowserContext,
ws: WorkspaceWithRole,
members: Member[]
) {
await mockWorkspaceList(page, [ws])
await mockWorkspaceTokenMint(page, ws)
await page.route('**/api/workspace/members**', (r) =>
await mockWorkspaceList(routeTarget, [ws])
await mockWorkspaceTokenMint(routeTarget, ws)
await routeTarget.route('**/api/workspace/members**', (r) =>
r.fulfill(
jsonRoute({
members,
Expand Down
32 changes: 31 additions & 1 deletion src/components/topbar/CurrentUserButton.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,20 @@ vi.mock('@/platform/workspace/components/WorkspaceProfilePic.vue', () => ({
}
}))

const CurrentUserPopoverWorkspaceStub = defineComponent({
name: 'CurrentUserPopoverWorkspace',
props: {
accountActionsOnly: Boolean
},
setup(props) {
return () =>
h('div', [
h('span', 'Workspace Popover Content'),
props.accountActionsOnly ? h('span', 'Account Actions Only') : ''
])
}
})

// Mock the CurrentUserPopoverLegacy component
vi.mock('./CurrentUserPopoverLegacy.vue', () => ({
default: defineComponent({
Expand All @@ -96,7 +110,7 @@ vi.mock('./CurrentUserPopoverLegacy.vue', () => ({
setup(_, { emit }) {
return () =>
h('div', [
'Popover Content',
h('span', 'Popover Content'),
h(
'button',
{
Expand Down Expand Up @@ -132,6 +146,7 @@ describe('CurrentUserButton', () => {
global: {
plugins: [i18n],
stubs: {
CurrentUserPopoverWorkspace: CurrentUserPopoverWorkspaceStub,
Popover: defineComponent({
setup(_, { slots, expose }) {
const shown = ref(false)
Expand Down Expand Up @@ -170,6 +185,21 @@ describe('CurrentUserButton', () => {
expect(screen.getByText('Popover Content')).toBeInTheDocument()
})

it.for(['loading', 'error'])(
'shows account actions while Cloud workspace initialization is %s',
async (initState) => {
mockIsCloud.value = true
mockFeatureFlags.teamWorkspacesEnabled = true
mockTeamWorkspaceStore.initState.value = initState
const { user } = renderComponent()

await user.click(screen.getByRole('button', { name: 'Current user' }))

expect(screen.getByText('Workspace Popover Content')).toBeInTheDocument()
expect(screen.getByText('Account Actions Only')).toBeInTheDocument()
}
)

it('hides popover when closePopover is called', async () => {
const { user } = renderComponent()

Expand Down
10 changes: 3 additions & 7 deletions src/components/topbar/CurrentUserButton.vue
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,13 @@
}"
@show="onPopoverShow"
>
<!-- Workspace mode: workspace-aware popover (only when ready) -->
<CurrentUserPopoverWorkspace
v-if="teamWorkspacesEnabled && initState === 'ready'"
v-if="teamWorkspacesEnabled"
ref="workspacePopoverContent"
:account-actions-only="initState !== 'ready'"
@close="closePopover"
/>
<!-- Legacy mode: original popover -->
<CurrentUserPopoverLegacy
v-else-if="!teamWorkspacesEnabled"
@close="closePopover"
/>
<CurrentUserPopoverLegacy v-else @close="closePopover" />
</Popover>
</div>
</template>
Expand Down
4 changes: 4 additions & 0 deletions src/locales/en/main.json
Original file line number Diff line number Diff line change
Expand Up @@ -4630,6 +4630,10 @@
"switchFailed": "Failed to switch workspace. Please try again."
},
"workspaceAuth": {
"initializationFailed": "Couldn't load your workspace",
"initializationFailedDetail": "Check your connection and try again.",
"initializationFailedSignOutDetail": "Sign out and log in again to continue.",
"retry": "Try again",
"errors": {
"notAuthenticated": "You must be logged in to access workspaces",
"invalidFirebaseToken": "Authentication failed. Please try logging in again.",
Expand Down
76 changes: 71 additions & 5 deletions src/platform/remoteConfig/refreshRemoteConfig.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
import { api } from '@/scripts/api'

import { refreshRemoteConfig } from './refreshRemoteConfig'
import { remoteConfig } from './remoteConfig'
import {
remoteConfig,
remoteConfigErrorStatus,
remoteConfigState
} from './remoteConfig'

vi.mock('@/scripts/api', () => ({
api: {
Expand All @@ -17,7 +21,7 @@ vi.stubGlobal('fetch', vi.fn())
describe('refreshRemoteConfig', () => {
const mockConfig = { feature1: true, feature2: 'value' }

function mockSuccessResponse(config = mockConfig) {
function mockSuccessResponse(config: Record<string, unknown> = mockConfig) {
return {
ok: true,
json: async () => config
Expand All @@ -33,8 +37,11 @@ describe('refreshRemoteConfig', () => {
}

beforeEach(() => {
vi.clearAllMocks()
vi.mocked(api.fetchApi).mockReset()
vi.mocked(global.fetch).mockReset()
remoteConfig.value = {}
remoteConfigErrorStatus.value = null
remoteConfigState.value = 'unloaded'
window.__CONFIG__ = {}
})

Expand Down Expand Up @@ -62,13 +69,70 @@ describe('refreshRemoteConfig', () => {
expect(global.fetch).not.toHaveBeenCalled()
})

it('does not pass an abort signal on the authed branch (so it is never aborted)', async () => {
it('passes an AbortSignal on the authenticated branch', async () => {
vi.mocked(api.fetchApi).mockResolvedValue(mockSuccessResponse())

await refreshRemoteConfig({ useAuth: true })

const init = vi.mocked(api.fetchApi).mock.calls[0][1]
expect(init?.signal).toBeUndefined()
expect(init?.signal).toBeInstanceOf(AbortSignal)
})

it('discards a failed response from a superseded refresh', async () => {
let resolveFirst: ((response: Response) => void) | undefined
vi.mocked(api.fetchApi)
.mockImplementationOnce(
() =>
new Promise<Response>((resolve) => {
resolveFirst = resolve
})
)
.mockResolvedValueOnce(
mockSuccessResponse({ subscription_required: true })
)

const firstRefresh = refreshRemoteConfig({ useAuth: true })
await vi.waitFor(() => expect(api.fetchApi).toHaveBeenCalledTimes(1))
await refreshRemoteConfig({ useAuth: true })
resolveFirst?.(mockErrorResponse(401, 'Unauthorized'))
await firstRefresh

expect(remoteConfig.value).toEqual({ subscription_required: true })
expect(remoteConfigState.value).toBe('authenticated')
expect(remoteConfigErrorStatus.value).toBeNull()
})

it('preserves shared state when the caller cancels the refresh', async () => {
const existingConfig = { subscription_required: true }
remoteConfig.value = existingConfig
remoteConfigState.value = 'authenticated'
remoteConfigErrorStatus.value = 500
window.__CONFIG__ = existingConfig
vi.mocked(api.fetchApi).mockImplementation(
(_route, options) =>
new Promise<Response>((_, reject) => {
if (options?.signal?.aborted) {
reject(new DOMException('Aborted', 'AbortError'))
return
}
options?.signal?.addEventListener('abort', () => {
reject(new DOMException('Aborted', 'AbortError'))
})
})
)
const controller = new AbortController()

const refresh = refreshRemoteConfig({
useAuth: true,
signal: controller.signal
})
controller.abort()
await refresh

expect(remoteConfig.value).toEqual(existingConfig)
expect(remoteConfigState.value).toBe('authenticated')
expect(remoteConfigErrorStatus.value).toBe(500)
expect(window.__CONFIG__).toEqual(existingConfig)
})
})

Expand Down Expand Up @@ -156,6 +220,8 @@ describe('refreshRemoteConfig', () => {

expect(remoteConfig.value).toEqual(existingConfig)
expect(window.__CONFIG__).toEqual(existingConfig)
expect(remoteConfigState.value).toBe('error')
expect(remoteConfigErrorStatus.value).toBeNull()
})
})
})
39 changes: 30 additions & 9 deletions src/platform/remoteConfig/refreshRemoteConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
cachedTeamWorkspacesEnabled,
cachedV1PaymentRecovery,
remoteConfig,
remoteConfigErrorStatus,
remoteConfigState
} from './remoteConfig'

Expand All @@ -18,8 +19,11 @@ interface RefreshRemoteConfigOptions {
* Set to false during bootstrap before auth is initialized.
*/
useAuth?: boolean
signal?: AbortSignal
}

let refreshGeneration = 0

async function fetchRemoteConfig(
useAuth: boolean,
signal?: AbortSignal
Expand All @@ -28,7 +32,7 @@ async function fetchRemoteConfig(
if (!useAuth) {
return fetch(api.apiURL('/features'), { cache: 'no-store', signal })
}
return api.fetchApi('/features', { cache: 'no-store' })
return api.fetchApi('/features', { cache: 'no-store', signal })
}

/**
Expand All @@ -43,20 +47,30 @@ async function fetchRemoteConfig(
export async function refreshRemoteConfig(
options: RefreshRemoteConfigOptions = {}
): Promise<void> {
const { useAuth = true } = options
const { useAuth = true, signal } = options
const generation = ++refreshGeneration
const controller = new AbortController()
const abort = () => controller.abort()
signal?.addEventListener('abort', abort, { once: true })
if (signal?.aborted) abort()

const controller = useAuth ? null : new AbortController()
const timeoutId = controller
? setTimeout(() => controller.abort(), FEATURES_FETCH_TIMEOUT_MS)
: null
const timeoutId = setTimeout(
() => controller.abort(),
FEATURES_FETCH_TIMEOUT_MS
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

try {
const response = await fetchRemoteConfig(useAuth, controller?.signal)
const response = await fetchRemoteConfig(useAuth, controller.signal)
if (generation !== refreshGeneration) return
if (signal?.aborted) return

if (response.ok) {
const config = await response.json()
if (generation !== refreshGeneration) return
if (signal?.aborted) return
window.__CONFIG__ = config
remoteConfig.value = config
remoteConfigErrorStatus.value = null
remoteConfigState.value = useAuth ? 'authenticated' : 'anonymous'
if (useAuth) {
cachedTeamWorkspacesEnabled.value = Boolean(
Expand All @@ -77,14 +91,21 @@ export async function refreshRemoteConfig(
if (response.status === 401 || response.status === 403) {
window.__CONFIG__ = {}
remoteConfig.value = {}
remoteConfigState.value = 'error'
remoteConfigErrorStatus.value = response.status
} else {
remoteConfigErrorStatus.value = null
}
remoteConfigState.value = 'error'
} catch (error) {
if (generation !== refreshGeneration) return
if (signal?.aborted) return
console.error('Failed to fetch remote config:', error)
window.__CONFIG__ = {}
remoteConfig.value = {}
remoteConfigErrorStatus.value = null
remoteConfigState.value = 'error'
} finally {
if (timeoutId !== null) clearTimeout(timeoutId)
clearTimeout(timeoutId)
signal?.removeEventListener('abort', abort)
}
}
2 changes: 2 additions & 0 deletions src/platform/remoteConfig/remoteConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ type RemoteConfigState = 'unloaded' | 'anonymous' | 'authenticated' | 'error'
*/
export const remoteConfigState = ref<RemoteConfigState>('unloaded')

export const remoteConfigErrorStatus = ref<number | null>(null)

/**
* Whether the authenticated config has been loaded.
* Use this to gate access to user-specific feature flags like teamWorkspacesEnabled.
Expand Down
10 changes: 10 additions & 0 deletions src/platform/settings/composables/useSettingUI.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,16 @@ describe('useSettingUI', () => {
expect(defaultCategory.value).toBe(settingCategories.value[0])
})

it('hides the empty Workspace navigation group for logged-out Cloud users', () => {
env.state.isCloud = true
env.state.isLoggedIn = false
env.state.teamWorkspacesEnabled = true

const { navGroups } = useSettingUI()

expect(navGroups.value.map(({ title }) => title)).not.toContain('Workspace')
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.

it('gives defaultPanel precedence over scrollToSettingId', () => {
const { defaultCategory } = useSettingUI('about', 'Comfy.Locale')
expect(defaultCategory.value.key).toBe('about')
Expand Down
Loading
Loading