-
Notifications
You must be signed in to change notification settings - Fork 673
fix: handle changes to supportsModelTypeTags #14160
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 3 commits
c57c86e
92245e8
837eaaa
14a9826
35aeeea
26becd1
ccb7424
ee61632
71db868
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 |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { effectScope } from 'vue' | ||
| import type { EffectScope } from 'vue' | ||
|
|
||
| import { | ||
| httpSupportsModelTypeTags, | ||
| refreshSupportsModelTypeTags, | ||
| useSupportsModelTypeTagsRefresh | ||
| } from '@/platform/assets/composables/useModelTypeTagsRefresh' | ||
| import { api } from '@/scripts/api' | ||
|
|
||
| function buildResponse( | ||
| body: unknown, | ||
| init: { ok?: boolean; status?: number } = {} | ||
| ): Response { | ||
| return { | ||
| ok: init.ok ?? true, | ||
| status: init.status ?? 200, | ||
| json: vi.fn().mockResolvedValue(body) | ||
| } as unknown as Response | ||
| } | ||
|
|
||
| const fetchApiSpy = vi.spyOn(api, 'fetchApi') | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| httpSupportsModelTypeTags.value = undefined | ||
| }) | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| describe('refreshSupportsModelTypeTags', () => { | ||
| it('reads the flag from the raw /features response', async () => { | ||
| fetchApiSpy.mockResolvedValue( | ||
| buildResponse({ supports_model_type_tags: true }) | ||
| ) | ||
|
|
||
| await refreshSupportsModelTypeTags() | ||
|
|
||
| expect(fetchApiSpy).toHaveBeenCalledWith('/features', { | ||
| cache: 'no-store' | ||
| }) | ||
| expect(httpSupportsModelTypeTags.value).toBe(true) | ||
| }) | ||
|
|
||
| it('tracks a flip to false', async () => { | ||
| httpSupportsModelTypeTags.value = true | ||
| fetchApiSpy.mockResolvedValue( | ||
| buildResponse({ supports_model_type_tags: false }) | ||
| ) | ||
|
|
||
| await refreshSupportsModelTypeTags() | ||
|
|
||
| expect(httpSupportsModelTypeTags.value).toBe(false) | ||
| }) | ||
|
|
||
| it('clears the value when the backend stops serving the key', async () => { | ||
| httpSupportsModelTypeTags.value = true | ||
| fetchApiSpy.mockResolvedValue(buildResponse({ other_flag: true })) | ||
|
|
||
| await refreshSupportsModelTypeTags() | ||
|
|
||
| expect(httpSupportsModelTypeTags.value).toBeUndefined() | ||
| }) | ||
|
|
||
| it('ignores a non-boolean value', async () => { | ||
| fetchApiSpy.mockResolvedValue( | ||
| buildResponse({ supports_model_type_tags: 'yes' }) | ||
| ) | ||
|
|
||
| await refreshSupportsModelTypeTags() | ||
|
|
||
| expect(httpSupportsModelTypeTags.value).toBeUndefined() | ||
| }) | ||
|
|
||
| it('keeps the last known value on an error response', async () => { | ||
| httpSupportsModelTypeTags.value = true | ||
| fetchApiSpy.mockResolvedValue(buildResponse({}, { ok: false, status: 500 })) | ||
|
|
||
| await refreshSupportsModelTypeTags() | ||
|
|
||
| expect(httpSupportsModelTypeTags.value).toBe(true) | ||
| }) | ||
|
|
||
| it('keeps the last known value when the fetch fails', async () => { | ||
| httpSupportsModelTypeTags.value = false | ||
| fetchApiSpy.mockRejectedValue(new Error('offline')) | ||
|
|
||
| await refreshSupportsModelTypeTags() | ||
|
|
||
| expect(httpSupportsModelTypeTags.value).toBe(false) | ||
| }) | ||
|
|
||
| it('discards a superseded response that resolves after a newer one', async () => { | ||
| let resolveSlow: (response: Response) => void | ||
| fetchApiSpy.mockReturnValueOnce( | ||
| new Promise<Response>((resolve) => { | ||
| resolveSlow = resolve | ||
| }) | ||
| ) | ||
| const slowRefresh = refreshSupportsModelTypeTags() | ||
|
|
||
| fetchApiSpy.mockResolvedValueOnce( | ||
| buildResponse({ supports_model_type_tags: false }) | ||
| ) | ||
| await refreshSupportsModelTypeTags() | ||
| expect(httpSupportsModelTypeTags.value).toBe(false) | ||
|
|
||
| resolveSlow!(buildResponse({ supports_model_type_tags: true })) | ||
| await slowRefresh | ||
|
|
||
| expect(httpSupportsModelTypeTags.value).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe('useSupportsModelTypeTagsRefresh', () => { | ||
| let scope: EffectScope | ||
|
|
||
| beforeEach(() => { | ||
| fetchApiSpy.mockResolvedValue( | ||
| buildResponse({ supports_model_type_tags: true }) | ||
| ) | ||
| scope = effectScope() | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| scope.stop() | ||
| vi.useRealTimers() | ||
| }) | ||
|
|
||
| it('fetches immediately and again on websocket reconnect', async () => { | ||
| scope.run(() => useSupportsModelTypeTagsRefresh()) | ||
| expect(fetchApiSpy).toHaveBeenCalledTimes(1) | ||
|
|
||
| api.dispatchCustomEvent('reconnected') | ||
|
|
||
| expect(fetchApiSpy).toHaveBeenCalledTimes(2) | ||
| await vi.waitFor(() => expect(httpSupportsModelTypeTags.value).toBe(true)) | ||
| }) | ||
|
|
||
| it('re-fetches when the tab becomes visible again', () => { | ||
| scope.run(() => useSupportsModelTypeTagsRefresh()) | ||
| expect(fetchApiSpy).toHaveBeenCalledTimes(1) | ||
|
|
||
| document.dispatchEvent(new Event('visibilitychange')) | ||
|
|
||
| expect(fetchApiSpy).toHaveBeenCalledTimes(2) | ||
| }) | ||
|
|
||
| it('re-fetches on the polling interval', () => { | ||
| vi.useFakeTimers() | ||
| scope.run(() => useSupportsModelTypeTagsRefresh()) | ||
| expect(fetchApiSpy).toHaveBeenCalledTimes(1) | ||
|
|
||
| vi.advanceTimersByTime(120_000) | ||
| expect(fetchApiSpy).toHaveBeenCalledTimes(2) | ||
| vi.advanceTimersByTime(120_000) | ||
| expect(fetchApiSpy).toHaveBeenCalledTimes(3) | ||
| }) | ||
|
|
||
| it('does not fetch on visibility events or interval ticks while hidden', () => { | ||
| vi.useFakeTimers() | ||
| const hiddenSpy = vi.spyOn(document, 'hidden', 'get').mockReturnValue(false) | ||
| scope.run(() => useSupportsModelTypeTagsRefresh()) | ||
| expect(fetchApiSpy).toHaveBeenCalledTimes(1) | ||
|
|
||
| hiddenSpy.mockReturnValue(true) | ||
| document.dispatchEvent(new Event('visibilitychange')) | ||
| vi.advanceTimersByTime(120_000) | ||
| expect(fetchApiSpy).toHaveBeenCalledTimes(1) | ||
|
|
||
| hiddenSpy.mockReturnValue(false) | ||
| document.dispatchEvent(new Event('visibilitychange')) | ||
| expect(fetchApiSpy).toHaveBeenCalledTimes(2) | ||
| hiddenSpy.mockRestore() | ||
| }) | ||
|
|
||
| it('stops all refresh triggers when the scope is disposed', () => { | ||
| vi.useFakeTimers() | ||
| scope.run(() => useSupportsModelTypeTagsRefresh()) | ||
| expect(fetchApiSpy).toHaveBeenCalledTimes(1) | ||
|
|
||
| scope.stop() | ||
| api.dispatchCustomEvent('reconnected') | ||
| document.dispatchEvent(new Event('visibilitychange')) | ||
| vi.advanceTimersByTime(120_000) | ||
|
|
||
| expect(fetchApiSpy).toHaveBeenCalledTimes(1) | ||
| }) | ||
| }) | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import { useEventListener, useIntervalFn } from '@vueuse/core' | ||
| import { ref } from 'vue' | ||
|
|
||
| import { api } from '@/scripts/api' | ||
|
|
||
| const REFRESH_INTERVAL_MS = 120_000 | ||
|
|
||
| /** | ||
| * `supports_model_type_tags` as served by HTTP `GET /features`, or undefined | ||
| * until a fetch has seen the key. The websocket copy of the flag is sent only | ||
| * once per connection, so an open session never observes a server-side flip | ||
| * (a rollback leaves stale tabs on the wrong tagging scheme); this HTTP | ||
| * source exists so the flag can refresh mid-session. Deliberately not | ||
| * sourced from remoteConfig: refreshRemoteConfig clears to {} on any fetch | ||
| * error (would flap this flag and trigger spurious model reloads), lacks | ||
| * reconnect/visibility triggers, and has no response-ordering guard. | ||
| * Consolidation tracked in FE-1439. | ||
| */ | ||
| export const httpSupportsModelTypeTags = ref<boolean | undefined>(undefined) | ||
|
|
||
| let refreshSequence = 0 | ||
|
|
||
| export async function refreshSupportsModelTypeTags(): Promise<void> { | ||
| const sequence = ++refreshSequence | ||
| try { | ||
| const response = await api.fetchApi('/features', { cache: 'no-store' }) | ||
|
Contributor
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. why are we doing another fetch of features here? Why can't we piggyback off of useFeatureFlags?
Contributor
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. Fair question — the duplication with The real reasons this doesn't piggyback on
The right fix is to harden Generated by Claude Code
claude[bot] marked this conversation as resolved.
Outdated
|
||
| if (!response.ok) return | ||
|
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. 🟡 Medium — Returning early on any
Contributor
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. Retain-on-error is deliberate and test-pinned ("keeps the last known value on an error response" in useModelTypeTagsRefresh.test.ts): clearing on 404 would reintroduce flag flap during rolling deploys, where mixed pod sets behind a load balancer can briefly 404 the route while the websocket copy lacks the key — flipping the effective flag and firing a spurious full model-library reload, which is exactly the failure mode this composable exists to avoid. Note a 200 response without the key already clears the value (useModelTypeTagsRefresh.ts:45-47), so the common downgrade case converges correctly. The residual window is only a downgrade to a backend predating /features entirely, where the websocket value takes over on the next page load — an accepted trade-off for this PR's scope. Generated by Claude Code |
||
| const features: unknown = await response.json() | ||
| // The refresh triggers can overlap (reconnect, visibility, interval); a | ||
| // superseded fetch must not commit, or a slow pre-flip response could | ||
| // revert a newer value. | ||
| if (sequence !== refreshSequence) return | ||
|
claude[bot] marked this conversation as resolved.
Outdated
|
||
| const value = | ||
| typeof features === 'object' && features !== null | ||
| ? (features as Record<string, unknown>)['supports_model_type_tags'] | ||
| : undefined | ||
| httpSupportsModelTypeTags.value = | ||
| typeof value === 'boolean' ? value : undefined | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| } catch { | ||
| // A failed fetch keeps the last known value; a backend that never serves | ||
| // the key stays undefined and the websocket flag remains authoritative. | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Keeps {@link httpSupportsModelTypeTags} current for the app's lifetime: | ||
| * fetches immediately, again whenever the websocket reconnects or the tab | ||
| * returns to the foreground, and on a slow interval while visible. The server | ||
| * caches `/features` responses, so the interval is cheap. | ||
| */ | ||
| export function useSupportsModelTypeTagsRefresh(): void { | ||
| void refreshSupportsModelTypeTags() | ||
| useEventListener(api, 'reconnected', () => { | ||
| void refreshSupportsModelTypeTags() | ||
| }) | ||
| useEventListener(document, 'visibilitychange', () => { | ||
| if (!document.hidden) void refreshSupportsModelTypeTags() | ||
| }) | ||
| useIntervalFn(() => { | ||
| if (document.hidden) return | ||
| void refreshSupportsModelTypeTags() | ||
| }, REFRESH_INTERVAL_MS) | ||
| } | ||
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.
why different pattern for this flag?
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.
This flag resolves differently because it's a server capability that must be able to change mid-session: the WS copy (
api.getServerFeature) arrives once per connection and is never re-pushed, and theremoteConfig-based resolution used by the other flags inheritsrefreshRemoteConfig's problems — it clearsremoteConfigto{}on any fetch error (which would flap this flag and cause spurious model-library reloads), has no reconnect/visibility triggers, and has no out-of-order response guard. So this one prefers a dedicated refreshable HTTP value and falls back to the WS copy for backends that don't serve the key over HTTP.To be clear, the earlier code comment here claiming a dynamic-config entry could "shadow" the capability was wrong (remoteConfig is the verbatim
/featuresresponse) and has been rewritten at head 837eaaa with the real rationale above. The duplication is acknowledged as temporary: hardeningrefreshRemoteConfigand folding the dedicated poller into it is tracked in FE-1439 (assigned to Simon), after which this flag can resolve through the common path.Generated by Claude Code