diff --git a/webui/.agents/skills/migrate-to-tanstack/SKILL.md b/webui/.agents/skills/migrate-to-tanstack/SKILL.md index a27f593de..80686465f 100644 --- a/webui/.agents/skills/migrate-to-tanstack/SKILL.md +++ b/webui/.agents/skills/migrate-to-tanstack/SKILL.md @@ -11,15 +11,16 @@ description: Migrate a data-fetching endpoint from the legacy ExtensionRegistryS `src/extension-registry-service.ts` (`ExtensionRegistryService`, plus `service.admin.*`) holds every server call. Legacy, un-migrated consumers call these methods straight from a component, passing an `AbortController` and relying on `fetch-retry`'s 10-attempt backoff inside `sendRequest`. That drags along per-component `AbortController` refs, `useEffect` fetch-on-mount wiring, and hand-rolled loading/error state. -Migrated endpoints instead go through a `use*` hook wrapping `useQuery`/`useMutation`, and retries move to the shared query client (`src/query-client.ts`). Roughly half the service is migrated — grep before assuming either state. +Migrated endpoints instead go through a `use*` hook wrapping `useQuery`/`useMutation`, retries move to the shared query client (`src/query-client.ts`), and error handling moves into the transport via `sendStrictRequest`. Roughly half the service is migrated — grep before assuming either state. ## Steps 1. **Find every consumer** of the method you're migrating: `grep -rn "service\.\|\.(" src`. List them — you'll migrate all of them or a named subset. -2. **Decide the retry scope — ask if unsure.** Ideally the service method flips from `sendRequest` (retriable) to `sendNonRetriableRequest`, handing retries to TanStack. Only do that when **every** consumer is moving to a hook — a legacy consumer still calling the method directly would silently lose its retry. If you're migrating just one of several consumers, either leave the method retriable (the query then double-retries, tolerated in the interim) or confirm scope with the user. When the request doesn't make the consumer scope clear, ask. +2. **Decide the retry scope — ask if unsure.** Ideally the service method flips from `sendRequest` (retriable) to `sendStrictRequest`, handing retries to TanStack. Only do that when **every** consumer is moving to a hook — a legacy consumer still calling the method directly would silently lose its retry *and* start seeing rejections where it used to get a resolved error result. If you're migrating just one of several consumers, either leave the method retriable (the query then double-retries, tolerated in the interim) or confirm scope with the user. When the request doesn't make the consumer scope clear, ask. 3. **Adjust the service method.** + - Switch it to `sendStrictRequest` and **drop `| ErrorResult` from its return type** (`Promise>` → `Promise>`). The method now resolves with data or rejects; the hook needs no `isError` check, and consumers lose their `as SuccessResult` casts. - Query methods: keep the `AbortController` param — the hook passes `controllerFromSignal(signal)`. - Mutation methods: **drop the `AbortController` param** — we no longer abort writes. @@ -27,7 +28,9 @@ Migrated endpoints instead go through a `use*` hook wrapping `useQuery`/`useMuta 5. **Update the consumers.** Replace the `AbortController` / `useEffect` / manual-state boilerplate with the hook, destructuring and renaming its result (`const { data: user, error: userError } = ...`; `const { mutateAsync, isPending } = ...`). Delete the dead boilerplate. -6. **Finish per the `write-code` skill:** add or update tests (`write-tests`), add a changelog entry, and pass `yarn lint`. +6. **Fix the tests that stubbed the old contract.** A spec stubbing the service method with `mockResolvedValue({ error: '…' })` was standing in for the old resolve-an-error-result behaviour — flip it to `mockRejectedValue({ error: '…' })`. `sendStrictRequest` itself is covered once, in `test/unit/server-request.spec.ts`; don't re-test it per endpoint. + +7. **Finish per the `write-code` skill:** add or update tests (`write-tests`), add a changelog entry, and pass `yarn lint`. ## Don't diff --git a/webui/.agents/skills/tanstack-query-conventions/SKILL.md b/webui/.agents/skills/tanstack-query-conventions/SKILL.md index b769eaef5..a93992a00 100644 --- a/webui/.agents/skills/tanstack-query-conventions/SKILL.md +++ b/webui/.agents/skills/tanstack-query-conventions/SKILL.md @@ -18,15 +18,13 @@ export const useUserExtension = (target: UserExtensionTarget) => { const { service } = useContext(MainContext); return useQuery({ queryKey: ['user', 'extension', target.namespace, target.extension], - queryFn: async ({ signal }) => { - const result = await service.getExtension(controllerFromSignal(signal), target.namespace, target.extension); - if (isError(result)) throw result; // let errors reach TanStack - return result; - } + queryFn: ({ signal }) => + service.getExtension(controllerFromSignal(signal), target.namespace, target.extension) }); }; ``` +- The `queryFn` is a one-liner because the service method rejects on failure — see "Errors belong to the service" below. Never re-check `isError` in a hook. - `controllerFromSignal(signal)` (`query-client.ts`) bridges TanStack's `AbortSignal` to the `AbortController` the service expects — service signatures stay untouched, component-level `AbortController` refs go away. - `useQuery` forbids `undefined`; normalise a "no result" case to `null`. - Query keys are hierarchical arrays (`['admin', 'namespace', name]`). When a key is reused for invalidation, export a small `*Keys` helper next to the hook. @@ -46,12 +44,28 @@ export const useCreateNamespace = () => { - **No `AbortController` / signal in mutations** — we don't abort writes anymore. - Mutations don't retry (TanStack's default `retry: 0`), which is correct for non-idempotent writes. -- `throw` on an error result when the caller relies on a `catch` / `onError` path. +- The `mutationFn` forwards the service call directly; the service rejects on failure, so `mutateAsync` callers get their `catch` / `onError` path for free. - Invalidate or remove affected queries in `onSuccess`. +## Errors belong to the service, not the hook + +The registry answers some failures with a `200` carrying an `{ error: '…' }` body instead of a non-2xx status. `sendStrictRequest` (`server-request.ts`) is the single place that normalises this: it is non-retriable *and* rejects on such a body, so a migrated service method resolves with data or rejects — never both. + +```ts +// extension-registry-service.ts — migrated methods +async getNamespace(abortController: AbortController, name: string): Promise> { + return sendStrictRequest({ abortController, credentials: true, endpoint: /* … */ }); +} +``` + +- A migrated method's return type **drops `| ErrorResult`** — that union is what forced the `isError` check on every caller. +- Hooks therefore never contain `if (isError(result)) throw result`. If you find yourself writing one, the service method still needs migrating. +- `sendRequest` / `sendNonRetriableRequest` keep resolving error bodies; legacy, un-migrated consumers check `isError` themselves. Don't change their behaviour. +- Same rule in tests: stub a failing service method with `mockRejectedValue({ error: '…' })`, not `mockResolvedValue`. + ## Retries and caching are owned by the shared client -- One singleton `queryClient` (`query-client.ts`) retries network/5xx with backoff, never 4xx; 429s are waited out inside `sendRequest`. Migrated service methods use `sendNonRetriableRequest`, so this is the only retry layer. +- One singleton `queryClient` (`query-client.ts`) retries network/5xx with backoff, never 4xx; 429s are waited out inside `sendRequest`. Migrated service methods use `sendStrictRequest` (fetch-retry disabled for non-429 responses), so TanStack should remain the only layer retrying network/5xx. - Defaults: `refetchOnWindowFocus: false`, `staleTime: 60s`. Override per hook only with reason — `staleTime: 0` / `gcTime: 0` when data must always be fresh (right after publish/delete), `retry: false` to let a 404 surface immediately. ## Options objects, not positional flags diff --git a/webui/CHANGELOG.md b/webui/CHANGELOG.md index 6dd2f5341..c18d9f6b6 100644 --- a/webui/CHANGELOG.md +++ b/webui/CHANGELOG.md @@ -30,6 +30,7 @@ This change log covers only the frontend library (webui) of Open VSX. - Fix the admin dashboard Scan tab getting stuck on the loading spinner after switching tabs, even though the new tab's data had already loaded successfully - Fix the page jumping to the top whenever a menu, select or dialog opens. - Fix the extension detail page's download menu so each target-platform option is clickable across its whole row, not just its text: the option was an inline link nested inside a non-interactive menu item, rather than the menu item itself being the link +- Fix `sendRequest` re-enabling fetch-retry's own retries for the request that follows a 429 wait, because the recursive call didn't forward the original `retry` flag. A `sendStrictRequest`/`sendNonRetriableRequest` call that hit a 429 could end up having its follow-up request retried twice - once by fetch-retry and once by the query client - Fix the create-namespace dialog acting on Enter when its button is disabled: an empty or over-long name was submitted anyway, and a held key sent the request more than once ### Dependencies diff --git a/webui/src/extension-registry-service.ts b/webui/src/extension-registry-service.ts index b506ecfd2..17a8faaef 100644 --- a/webui/src/extension-registry-service.ts +++ b/webui/src/extension-registry-service.ts @@ -148,10 +148,7 @@ export class ExtensionRegistryService { }); } - async search( - abortController: AbortController, - filter?: ExtensionFilter - ): Promise> { + async search(abortController: AbortController, filter?: ExtensionFilter): Promise> { const query: { key: string; value: string | number }[] = []; if (filter) { if (filter.query) query.push({ key: 'query', value: filter.query }); @@ -162,8 +159,8 @@ export class ExtensionRegistryService { if (filter.sortOrder) query.push({ key: 'sortOrder', value: filter.sortOrder }); } const endpoint = createAbsoluteURL([this.serverUrl, 'api', '-', 'search'], query); - // Non-retriable: retries are owned by the TanStack query that calls this. - return sendNonRetriableRequest({ abortController, endpoint }); + // Retries are owned by the TanStack query that calls this. + return sendStrictRequest({ abortController, endpoint }); } async getExtensionDetail( @@ -613,7 +610,7 @@ export class ExtensionRegistryService { ): Promise> { const headers: Record = {}; - return sendNonRetriableRequest({ + return sendStrictRequest({ abortController, method: 'GET', credentials: true, @@ -626,7 +623,7 @@ export class ExtensionRegistryService { namespace: string; extension: string; targetPlatformVersions?: object[]; - }): Promise> { + }): Promise> { const csrfResponse = await this.getCsrfToken(); const headers: Record = { 'Content-Type': 'application/json;charset=UTF-8' @@ -636,7 +633,7 @@ export class ExtensionRegistryService { headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'POST', credentials: true, endpoint: createAbsoluteURL([this.serverUrl, 'user', 'extension', req.namespace, req.extension, 'delete']), @@ -657,21 +654,21 @@ export interface AdminService { namespace: string; extension: string; targetPlatformVersions?: object[]; - }): Promise>; + }): Promise>; purgeExtensions(req: { namespace: string; extension: string; targetPlatformVersions?: object[]; - }): Promise>; + }): Promise>; getNamespace(abortController: AbortController, name: string): Promise>; - createNamespace(namespace: { name: string }): Promise>; - deleteNamespace(namespace: { name: string }): Promise>; + createNamespace(namespace: { name: string }): Promise>; + deleteNamespace(namespace: { name: string }): Promise>; changeNamespace(req: { oldNamespace: string; newNamespace: string; removeOldNamespace: boolean; mergeIfNewNamespaceAlreadyExists: boolean; - }): Promise>; + }): Promise>; getPublisherInfo( abortController: AbortController, provider: string, @@ -685,10 +682,10 @@ export interface AdminService { provider: string, login: string, role: 'admin' | 'privileged' | 'none' - ): Promise>; - revokePublisherContributions(provider: string, login: string): Promise>; - revokeAccessTokens(provider: string, login: string): Promise>; - forgetUser(provider: string, login: string): Promise>; + ): Promise>; + revokePublisherContributions(provider: string, login: string): Promise>; + revokeAccessTokens(provider: string, login: string): Promise>; + forgetUser(provider: string, login: string): Promise>; getAllScans( abortController: AbortController, params?: { @@ -745,15 +742,15 @@ export interface AdminService { getTiers(abortController: AbortController): Promise>; createTier(tier: Tier): Promise>; updateTier(name: string, tier: Tier): Promise>; - deleteTier(name: string): Promise>; + deleteTier(name: string): Promise>; getCustomers(abortController: AbortController): Promise>; getCustomer(abortController: AbortController, name: string): Promise>; createCustomer(customer: Customer): Promise>; updateCustomer(name: string, customer: Customer): Promise>; - deleteCustomer(name: string): Promise>; + deleteCustomer(name: string): Promise>; getCustomerMembers(abortController: AbortController, name: string): Promise>; - addCustomerMember(name: string, user: UserData): Promise>; - removeCustomerMember(name: string, user: UserData): Promise>; + addCustomerMember(name: string, user: UserData): Promise>; + removeCustomerMember(name: string, user: UserData): Promise>; getUsageStats( abortController: AbortController, customerName: string, @@ -770,7 +767,7 @@ export interface AdminService { customerName: string ): Promise>; createCustomerRateLimitToken(customerName: string, description: string): Promise>; - deleteCustomerRateLimitToken(customerName: string, tokenId: number): Promise>; + deleteCustomerRateLimitToken(customerName: string, tokenId: number): Promise>; getSettings(abortController: AbortController): Promise>; updateSettings(settings: Settings): Promise>; getConsistencyChecks(abortController: AbortController): Promise>; @@ -778,8 +775,8 @@ export interface AdminService { abortController: AbortController, checkId: string ): Promise>; - fixConsistencyFindings(checkId: string): Promise>; - fixConsistencyFinding(checkId: string, entityId: number): Promise>; + fixConsistencyFindings(checkId: string): Promise>; + fixConsistencyFinding(checkId: string, entityId: number): Promise>; } export interface AdminServiceConstructor { @@ -794,7 +791,7 @@ export class AdminServiceImpl implements AdminService { namespace: string, extension: string ): Promise> { - return sendNonRetriableRequest({ + return sendStrictRequest({ abortController, credentials: true, endpoint: createAbsoluteURL([this.registry.serverUrl, 'admin', 'extension', namespace, extension]) @@ -805,7 +802,7 @@ export class AdminServiceImpl implements AdminService { namespace: string; extension: string; targetPlatformVersions?: object[]; - }): Promise> { + }): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = { 'Content-Type': 'application/json;charset=UTF-8' @@ -815,7 +812,7 @@ export class AdminServiceImpl implements AdminService { headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'POST', credentials: true, endpoint: createAbsoluteURL([ @@ -835,7 +832,7 @@ export class AdminServiceImpl implements AdminService { namespace: string; extension: string; targetPlatformVersions?: object[]; - }): Promise> { + }): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = { 'Content-Type': 'application/json;charset=UTF-8' @@ -845,7 +842,7 @@ export class AdminServiceImpl implements AdminService { headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'POST', credentials: true, endpoint: createAbsoluteURL([ @@ -862,14 +859,14 @@ export class AdminServiceImpl implements AdminService { } async getNamespace(abortController: AbortController, name: string): Promise> { - return sendNonRetriableRequest({ + return sendStrictRequest({ abortController, credentials: true, endpoint: createAbsoluteURL([this.registry.serverUrl, 'admin', 'namespace', name]) }); } - async createNamespace(namespace: { name: string }): Promise> { + async createNamespace(namespace: { name: string }): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = { 'Content-Type': 'application/json;charset=UTF-8' @@ -878,7 +875,7 @@ export class AdminServiceImpl implements AdminService { const csrfToken = csrfResponse as CsrfTokenJson; headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ credentials: true, endpoint: createAbsoluteURL([this.registry.serverUrl, 'admin', 'create-namespace']), method: 'POST', @@ -887,7 +884,7 @@ export class AdminServiceImpl implements AdminService { }); } - async deleteNamespace(namespace: { name: string }): Promise> { + async deleteNamespace(namespace: { name: string }): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = { 'Content-Type': 'application/json;charset=UTF-8' @@ -896,7 +893,7 @@ export class AdminServiceImpl implements AdminService { const csrfToken = csrfResponse as CsrfTokenJson; headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ credentials: true, endpoint: createAbsoluteURL([this.registry.serverUrl, 'admin', 'namespace', namespace.name]), method: 'DELETE', @@ -908,7 +905,7 @@ export class AdminServiceImpl implements AdminService { newNamespace: string; removeOldNamespace: boolean; mergeIfNewNamespaceAlreadyExists: boolean; - }): Promise> { + }): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = { 'Content-Type': 'application/json;charset=UTF-8' @@ -917,7 +914,7 @@ export class AdminServiceImpl implements AdminService { const csrfToken = csrfResponse as CsrfTokenJson; headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ credentials: true, endpoint: createAbsoluteURL([this.registry.serverUrl, 'admin', 'change-namespace']), method: 'POST', @@ -968,7 +965,7 @@ export class AdminServiceImpl implements AdminService { provider: string, login: string, role: 'admin' | 'privileged' | 'none' - ): Promise> { + ): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = {}; if (!isError(csrfResponse)) { @@ -976,7 +973,7 @@ export class AdminServiceImpl implements AdminService { headers[csrfToken.header] = csrfToken.value; } const query = [{ key: 'role', value: role }]; - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'POST', credentials: true, endpoint: createAbsoluteURL([this.registry.serverUrl, 'admin', 'user', provider, login, 'role'], query), @@ -984,17 +981,14 @@ export class AdminServiceImpl implements AdminService { }); } - async revokePublisherContributions( - provider: string, - login: string - ): Promise> { + async revokePublisherContributions(provider: string, login: string): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = {}; if (!isError(csrfResponse)) { const csrfToken = csrfResponse as CsrfTokenJson; headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'POST', credentials: true, endpoint: createAbsoluteURL([this.registry.serverUrl, 'admin', 'publisher', provider, login, 'revoke']), @@ -1002,14 +996,14 @@ export class AdminServiceImpl implements AdminService { }); } - async revokeAccessTokens(provider: string, login: string): Promise> { + async revokeAccessTokens(provider: string, login: string): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = {}; if (!isError(csrfResponse)) { const csrfToken = csrfResponse as CsrfTokenJson; headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'POST', credentials: true, endpoint: createAbsoluteURL([ @@ -1025,14 +1019,14 @@ export class AdminServiceImpl implements AdminService { }); } - async forgetUser(provider: string, login: string): Promise> { + async forgetUser(provider: string, login: string): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = {}; if (!isError(csrfResponse)) { const csrfToken = csrfResponse as CsrfTokenJson; headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'POST', credentials: true, endpoint: createAbsoluteURL([this.registry.serverUrl, 'admin', 'publisher', provider, login, 'delete']), @@ -1302,7 +1296,7 @@ export class AdminServiceImpl implements AdminService { }); } - async deleteTier(name: string): Promise { + async deleteTier(name: string): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = { 'Content-Type': 'application/json;charset=UTF-8' @@ -1311,7 +1305,7 @@ export class AdminServiceImpl implements AdminService { const csrfToken = csrfResponse as CsrfTokenJson; headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'DELETE', credentials: true, endpoint: createAbsoluteURL([this.registry.serverUrl, 'admin', 'ratelimit', 'tiers', name]), @@ -1371,7 +1365,7 @@ export class AdminServiceImpl implements AdminService { }); } - async deleteCustomer(name: string): Promise { + async deleteCustomer(name: string): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = { 'Content-Type': 'application/json;charset=UTF-8' @@ -1380,7 +1374,7 @@ export class AdminServiceImpl implements AdminService { const csrfToken = csrfResponse as CsrfTokenJson; headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'DELETE', credentials: true, endpoint: createAbsoluteURL([this.registry.serverUrl, 'admin', 'ratelimit', 'customers', name]), @@ -1399,7 +1393,7 @@ export class AdminServiceImpl implements AdminService { }); } - async addCustomerMember(name: string, user: UserData): Promise> { + async addCustomerMember(name: string, user: UserData): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = {}; if (!isError(csrfResponse)) { @@ -1410,7 +1404,7 @@ export class AdminServiceImpl implements AdminService { { key: 'user', value: user.loginName }, { key: 'provider', value: user.provider } ]; - return sendNonRetriableRequest({ + return sendStrictRequest({ headers, method: 'POST', credentials: true, @@ -1421,7 +1415,7 @@ export class AdminServiceImpl implements AdminService { }); } - async removeCustomerMember(name: string, user: UserData): Promise> { + async removeCustomerMember(name: string, user: UserData): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = {}; if (!isError(csrfResponse)) { @@ -1432,7 +1426,7 @@ export class AdminServiceImpl implements AdminService { { key: 'user', value: user.loginName }, { key: 'provider', value: user.provider } ]; - return sendNonRetriableRequest({ + return sendStrictRequest({ headers, method: 'POST', credentials: true, @@ -1528,10 +1522,7 @@ export class AdminServiceImpl implements AdminService { }); } - async deleteCustomerRateLimitToken( - customerName: string, - tokenId: number - ): Promise> { + async deleteCustomerRateLimitToken(customerName: string, tokenId: number): Promise> { const csrfResponse = await this.registry.getCsrfToken(); const headers: Record = { 'Content-Type': 'application/json;charset=UTF-8' @@ -1540,7 +1531,7 @@ export class AdminServiceImpl implements AdminService { const csrfToken = csrfResponse as CsrfTokenJson; headers[csrfToken.header] = csrfToken.value; } - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'DELETE', credentials: true, endpoint: createAbsoluteURL([ @@ -1602,9 +1593,9 @@ export class AdminServiceImpl implements AdminService { }); } - async fixConsistencyFindings(checkId: string): Promise> { + async fixConsistencyFindings(checkId: string): Promise> { const headers = await this.csrfHeaders(); - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'POST', credentials: true, endpoint: createAbsoluteURL([this.registry.serverUrl, 'admin', 'consistency', checkId, 'fix']), @@ -1612,9 +1603,9 @@ export class AdminServiceImpl implements AdminService { }); } - async fixConsistencyFinding(checkId: string, entityId: number): Promise> { + async fixConsistencyFinding(checkId: string, entityId: number): Promise> { const headers = await this.csrfHeaders(); - return sendNonRetriableRequest({ + return sendStrictRequest({ method: 'POST', credentials: true, endpoint: createAbsoluteURL([ diff --git a/webui/src/hooks/use-infinite-search.ts b/webui/src/hooks/use-infinite-search.ts index 98bf61464..bfb8dcd3c 100644 --- a/webui/src/hooks/use-infinite-search.ts +++ b/webui/src/hooks/use-infinite-search.ts @@ -14,7 +14,7 @@ import { useContext } from 'react'; import { keepPreviousData, useInfiniteQuery } from '@tanstack/react-query'; import { MainContext } from '../context'; -import { isError, SearchResult } from '../extension-registry-types'; +import { SearchResult } from '../extension-registry-types'; import { ExtensionFilter } from '../extension-registry-service'; import { controllerFromSignal } from '../query-client'; @@ -37,20 +37,15 @@ export const useInfiniteSearch = (filter: ExtensionFilter) => { const { query, category, size, sortBy, sortOrder } = filter; return useInfiniteQuery({ queryKey: searchKeys.list({ query, category, size, sortBy, sortOrder }), - queryFn: async ({ pageParam, signal }): Promise => { - const result = await service.search(controllerFromSignal(signal), { + queryFn: ({ pageParam, signal }): Promise => + service.search(controllerFromSignal(signal), { query, category, size, sortBy, sortOrder, offset: pageParam - }); - if (isError(result)) { - throw result; - } - return result as SearchResult; - }, + }), initialPageParam: 0, getNextPageParam: (lastPage, allPages) => { const loaded = allPages.reduce((sum, page) => sum + page.extensions.length, 0); diff --git a/webui/src/pages/admin-dashboard/customers/use-customers.ts b/webui/src/pages/admin-dashboard/customers/use-customers.ts index 6ebec6081..3df68311c 100644 --- a/webui/src/pages/admin-dashboard/customers/use-customers.ts +++ b/webui/src/pages/admin-dashboard/customers/use-customers.ts @@ -14,7 +14,7 @@ import { useContext } from 'react'; import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { MainContext } from '../../../context'; -import { type Customer, isError, type UserData } from '../../../extension-registry-types'; +import { type Customer, type UserData } from '../../../extension-registry-types'; import { controllerFromSignal } from '../../../query-client'; export const customerKeys = { @@ -123,13 +123,7 @@ export const useDeleteCustomerToken = (name: string) => { const { service } = useContext(MainContext); const queryClient = useQueryClient(); return useMutation({ - mutationFn: async (tokenId: number) => { - const result = await service.admin.deleteCustomerRateLimitToken(name, tokenId); - if (isError(result)) { - throw result; - } - return result; - }, + mutationFn: (tokenId: number) => service.admin.deleteCustomerRateLimitToken(name, tokenId), onSuccess: () => { queryClient.invalidateQueries({ queryKey: customerKeys.tokens(name) }); } @@ -154,13 +148,7 @@ export const useAddCustomerMember = (name: string) => { const { service } = useContext(MainContext); const queryClient = useQueryClient(); return useMutation({ - mutationFn: async (user: UserData) => { - const result = await service.admin.addCustomerMember(name, user); - if (isError(result)) { - throw result; - } - return result; - }, + mutationFn: (user: UserData) => service.admin.addCustomerMember(name, user), onSuccess: () => { queryClient.invalidateQueries({ queryKey: customerKeys.members(name) }); } @@ -174,13 +162,7 @@ export const useRemoveCustomerMember = (name: string) => { const { service } = useContext(MainContext); const queryClient = useQueryClient(); return useMutation({ - mutationFn: async (user: UserData) => { - const result = await service.admin.removeCustomerMember(name, user); - if (isError(result)) { - throw result; - } - return result; - }, + mutationFn: (user: UserData) => service.admin.removeCustomerMember(name, user), onSuccess: () => { queryClient.invalidateQueries({ queryKey: customerKeys.members(name) }); } diff --git a/webui/src/pages/admin-dashboard/namespace-change-dialog.tsx b/webui/src/pages/admin-dashboard/namespace-change-dialog.tsx index 34296670c..4498d6e18 100644 --- a/webui/src/pages/admin-dashboard/namespace-change-dialog.tsx +++ b/webui/src/pages/admin-dashboard/namespace-change-dialog.tsx @@ -21,7 +21,7 @@ import { TextField } from '@mui/material'; import { ButtonWithProgress } from '../../components/button-with-progress'; -import { Namespace, SuccessResult } from '../../extension-registry-types'; +import { Namespace } from '../../extension-registry-types'; import { MainContext } from '../../context'; import { InfoDialog } from '../../components/info-dialog'; import { useChangeNamespace } from './use-namespace-admin'; @@ -78,10 +78,9 @@ export const NamespaceChangeDialog: FunctionComponent = {updateRole.isSuccess && ( - {(updateRole.data as Partial).success ?? ''} + {updateRole.data.success} )} diff --git a/webui/src/pages/admin-dashboard/use-extension-admin.ts b/webui/src/pages/admin-dashboard/use-extension-admin.ts index c71b3bb72..afe3c95aa 100644 --- a/webui/src/pages/admin-dashboard/use-extension-admin.ts +++ b/webui/src/pages/admin-dashboard/use-extension-admin.ts @@ -14,7 +14,6 @@ import { useContext } from 'react'; import { useMutation, useQuery } from '@tanstack/react-query'; import { MainContext } from '../../context'; -import { isError } from '../../extension-registry-types'; import { controllerFromSignal } from '../../query-client'; interface ExtensionTarget { @@ -37,17 +36,8 @@ export const useAdminExtension = (target: ExtensionTarget | null) => { const { service } = useContext(MainContext); return useQuery({ queryKey: ['admin', 'extension', target?.namespace ?? '', target?.extension ?? ''], - queryFn: async ({ signal }) => { - const result = await service.admin.getExtension( - controllerFromSignal(signal), - target!.namespace, - target!.extension - ); - if (isError(result)) { - throw result; - } - return result; - }, + queryFn: ({ signal }) => + service.admin.getExtension(controllerFromSignal(signal), target!.namespace, target!.extension), enabled: !!target, retry: false, staleTime: 0 @@ -55,9 +45,7 @@ export const useAdminExtension = (target: ExtensionTarget | null) => { }; /** - * Deletes extension versions. Mirrors the previous behaviour of not throwing on - * an error result; thrown (network/server) errors reject so the caller's catch - * path runs. + * Deletes extension versions. */ export const useDeleteExtension = () => { const { service } = useContext(MainContext); diff --git a/webui/src/pages/admin-dashboard/use-namespace-admin.ts b/webui/src/pages/admin-dashboard/use-namespace-admin.ts index dab0ee74e..ec848338d 100644 --- a/webui/src/pages/admin-dashboard/use-namespace-admin.ts +++ b/webui/src/pages/admin-dashboard/use-namespace-admin.ts @@ -14,7 +14,6 @@ import { useContext } from 'react'; import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { MainContext } from '../../context'; -import { isError } from '../../extension-registry-types'; import { controllerFromSignal } from '../../query-client'; interface ChangeNamespaceRequest { @@ -37,13 +36,7 @@ export const useAdminNamespace = (name: string) => { const { service } = useContext(MainContext); return useQuery({ queryKey: namespaceAdminKeys.detail(name), - queryFn: async ({ signal }) => { - const namespace = await service.admin.getNamespace(controllerFromSignal(signal), name); - if (isError(namespace)) { - throw namespace; - } - return namespace; - }, + queryFn: ({ signal }) => service.admin.getNamespace(controllerFromSignal(signal), name), enabled: !!name, retry: false, staleTime: 0 @@ -65,36 +58,22 @@ export const useCreateNamespace = () => { }; /** - * Renames (and optionally merges) a namespace. Throws on an error result so the - * caller's catch / onError path runs. + * Renames (and optionally merges) a namespace. */ export const useChangeNamespace = () => { const { service } = useContext(MainContext); return useMutation({ - mutationFn: async (req: ChangeNamespaceRequest) => { - const result = await service.admin.changeNamespace(req); - if (isError(result)) { - throw result; - } - return result; - } + mutationFn: (req: ChangeNamespaceRequest) => service.admin.changeNamespace(req) }); }; /** - * Deletes a namespace. Throws on an error result so the caller's catch / - * onError path runs. + * Deletes a namespace. */ export const useDeleteNamespace = () => { const { service } = useContext(MainContext); return useMutation({ - mutationFn: async (name: string) => { - const result = await service.admin.deleteNamespace({ name }); - if (isError(result)) { - throw result; - } - return result; - } + mutationFn: (name: string) => service.admin.deleteNamespace({ name }) }); }; diff --git a/webui/src/pages/admin-dashboard/use-publisher-admin.ts b/webui/src/pages/admin-dashboard/use-publisher-admin.ts index 35356716e..096971874 100644 --- a/webui/src/pages/admin-dashboard/use-publisher-admin.ts +++ b/webui/src/pages/admin-dashboard/use-publisher-admin.ts @@ -14,7 +14,6 @@ import { useContext } from 'react'; import { keepPreviousData, useInfiniteQuery, useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { MainContext } from '../../context'; -import { isError } from '../../extension-registry-types'; import { controllerFromSignal } from '../../query-client'; export type PublisherRole = 'admin' | 'privileged' | 'none'; @@ -67,20 +66,15 @@ export const useInfinitePublishers = (search: string, role: string) => { /** * Updates a publisher's role and refreshes the publisher list on success so the - * new role is reflected. Throws on an error result so the caller's catch path runs. + * new role is reflected. */ export const useUpdatePublisherRole = () => { const { service } = useContext(MainContext); const queryClient = useQueryClient(); return useMutation({ mutationKey: [...publisherMutationKey, 'role'], - mutationFn: async ({ provider, login, role }: { provider: string; login: string; role: PublisherRole }) => { - const result = await service.admin.updateUserRole(provider, login, role); - if (isError(result)) { - throw result; - } - return result; - }, + mutationFn: ({ provider, login, role }: { provider: string; login: string; role: PublisherRole }) => + service.admin.updateUserRole(provider, login, role), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ['admin', 'publishers'] }); } @@ -88,55 +82,37 @@ export const useUpdatePublisherRole = () => { }; /** - * Revokes all contributions of a publisher. Throws on an error result so the - * caller's catch path runs. + * Revokes all contributions of a publisher. */ export const useRevokePublisherContributions = () => { const { service } = useContext(MainContext); return useMutation({ mutationKey: [...publisherMutationKey, 'revoke-contributions'], - mutationFn: async ({ provider, login }: { provider: string; login: string }) => { - const result = await service.admin.revokePublisherContributions(provider, login); - if (isError(result)) { - throw result; - } - return result; - } + mutationFn: ({ provider, login }: { provider: string; login: string }) => + service.admin.revokePublisherContributions(provider, login) }); }; /** - * Revokes the access tokens of a publisher. Throws on an error result so the - * caller's catch path runs. + * Revokes the access tokens of a publisher. */ export const useRevokeAccessTokens = () => { const { service } = useContext(MainContext); return useMutation({ mutationKey: [...publisherMutationKey, 'revoke-tokens'], - mutationFn: async ({ provider, login }: { provider: string; login: string }) => { - const result = await service.admin.revokeAccessTokens(provider, login); - if (isError(result)) { - throw result; - } - return result; - } + mutationFn: ({ provider, login }: { provider: string; login: string }) => + service.admin.revokeAccessTokens(provider, login) }); }; /** - * Forgets a user in response to a data-protection erasure request. Throws on - * an error result so the caller's catch path runs. + * Forgets a user in response to a data-protection erasure request. */ export const useForgetUser = () => { const { service } = useContext(MainContext); return useMutation({ mutationKey: [...publisherMutationKey, 'forget-user'], - mutationFn: async ({ provider, login }: { provider: string; login: string }) => { - const result = await service.admin.forgetUser(provider, login); - if (isError(result)) { - throw result; - } - return result; - } + mutationFn: ({ provider, login }: { provider: string; login: string }) => + service.admin.forgetUser(provider, login) }); }; diff --git a/webui/src/pages/home/use-home-data.ts b/webui/src/pages/home/use-home-data.ts index 435dc441d..fd718611d 100644 --- a/webui/src/pages/home/use-home-data.ts +++ b/webui/src/pages/home/use-home-data.ts @@ -14,7 +14,7 @@ import { useContext, useMemo } from 'react'; import { useQueries } from '@tanstack/react-query'; import { MainContext } from '../../context'; -import { SearchEntry, SearchResult, SortOrder, isError } from '../../extension-registry-types'; +import { SearchEntry, SortOrder } from '../../extension-registry-types'; import { ExtensionCategory } from '../../extension-registry-types'; import { useCategories } from '../../components/categories'; import { HomeCuratedSection } from '../../page-settings'; @@ -80,10 +80,7 @@ export function useCuratedRows(curatedSections: HomeCuratedSection[]): CuratedRo sortBy: section.sortBy, sortOrder: 'desc' as SortOrder }); - if (isError(result)) { - throw result; - } - return (result as SearchResult).extensions; + return result.extensions; } })) }); diff --git a/webui/src/pages/user/extensions/use-user-extension.ts b/webui/src/pages/user/extensions/use-user-extension.ts index 6d77da1e8..69d229e03 100644 --- a/webui/src/pages/user/extensions/use-user-extension.ts +++ b/webui/src/pages/user/extensions/use-user-extension.ts @@ -14,7 +14,6 @@ import { useContext } from 'react'; import { useMutation, useQuery } from '@tanstack/react-query'; import { MainContext } from '../../../context'; -import { isError } from '../../../extension-registry-types'; import { controllerFromSignal, NO_CACHE } from '../../../query-client'; interface UserExtensionTarget { @@ -35,21 +34,13 @@ export const useUserExtension = (target: UserExtensionTarget) => { const { service } = useContext(MainContext); return useQuery({ queryKey: ['user', 'extension', target.namespace, target.extension], - queryFn: async ({ signal }) => { - const result = await service.getExtension(controllerFromSignal(signal), target.namespace, target.extension); - if (isError(result)) { - throw result; - } - return result; - }, + queryFn: ({ signal }) => service.getExtension(controllerFromSignal(signal), target.namespace, target.extension), ...NO_CACHE }); }; /** - * Deletes extension versions. Mirrors the previous behaviour of not throwing on - * an error result; thrown (network/server) errors reject so the caller's catch - * path runs. + * Deletes extension versions. */ export const useDeleteUserExtensionVersions = () => { const { service } = useContext(MainContext); diff --git a/webui/src/server-request.ts b/webui/src/server-request.ts index bc2fcb26e..b15b6a717 100644 --- a/webui/src/server-request.ts +++ b/webui/src/server-request.ts @@ -93,7 +93,7 @@ export async function sendRequest(req: ServerAPIRequest, retry: boolean = t const jitter = Math.floor(Math.random() * 100); const timeoutMillis = (Number(retrySeconds) + 1) * 1000 + jitter; return new Promise(resolve => setTimeout(resolve, timeoutMillis, req)).then(request => - sendRequest(request) + sendRequest(request, retry) ); } else { let err: ErrorResponse; diff --git a/webui/test/unit/components/extension/extension-version-delete-dialog.spec.tsx b/webui/test/unit/components/extension/extension-version-delete-dialog.spec.tsx new file mode 100644 index 000000000..63147c000 --- /dev/null +++ b/webui/test/unit/components/extension/extension-version-delete-dialog.spec.tsx @@ -0,0 +1,82 @@ +/******************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + ********************************************************************************/ + +import { describe, expect, it, vi } from 'vitest'; +import { screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { renderWithProviders } from '../../support/test-providers'; +import { DeleteVersionDialog } from '../../../../src/components/extension/extension-version-delete-dialog'; +import { Extension, VersionTargetPlatforms } from '../../../../src/extension-registry-types'; + +const extension = { name: 'bar', namespace: 'foo', displayName: 'Bar Tools' } as unknown as Extension; + +const version: VersionTargetPlatforms = { + version: '1.0.0', + targetPlatforms: [{ targetPlatform: 'universal', removed: false }] +} as unknown as VersionTargetPlatforms; + +// MUI's Dialog transition trips a jsdom getComputedStyle bug that every getByRole query +// walks into while the dialog is open - query the dialog's buttons by text instead. +const removeButton = () => screen.getByText('Remove', { selector: 'button' }); + +function renderDialog(onRemove: () => Promise) { + const onDeleted = vi.fn(); + const onClose = vi.fn(); + const handleError = vi.fn(); + renderWithProviders( + , + { mainContext: { handleError } } + ); + return { onDeleted, onClose, handleError }; +} + +describe('DeleteVersionDialog', () => { + it('closes and notifies the page when the removal succeeds', async () => { + const { onDeleted, onClose, handleError } = renderDialog(vi.fn().mockResolvedValue({ success: 'ok' })); + + await userEvent.click(removeButton()); + + await waitFor(() => expect(onDeleted).toHaveBeenCalled()); + expect(onClose).toHaveBeenCalled(); + expect(handleError).not.toHaveBeenCalled(); + }); + + it('reports a rejected removal instead of reporting success', async () => { + const { onDeleted, onClose, handleError } = renderDialog(vi.fn().mockRejectedValue({ error: 'boom' })); + + await userEvent.click(removeButton()); + + await waitFor(() => expect(handleError).toHaveBeenCalledWith({ error: 'boom' })); + expect(onDeleted).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + }); + + it('defers closing to the error dialog when the versions are stale (409)', async () => { + const { onDeleted, onClose, handleError } = renderDialog(vi.fn().mockRejectedValue({ status: 409 })); + + await userEvent.click(removeButton()); + + await waitFor(() => expect(handleError).toHaveBeenCalled()); + expect(onDeleted).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + + // The page refreshes and the dialog closes only once the user acknowledges the error. + handleError.mock.calls[0][1].onClose(); + expect(onDeleted).toHaveBeenCalled(); + expect(onClose).toHaveBeenCalled(); + }); +}); diff --git a/webui/test/unit/pages/admin-dashboard/publisher-forget-user-button.spec.tsx b/webui/test/unit/pages/admin-dashboard/publisher-forget-user-button.spec.tsx index 31ec40dbf..9b907d249 100644 --- a/webui/test/unit/pages/admin-dashboard/publisher-forget-user-button.spec.tsx +++ b/webui/test/unit/pages/admin-dashboard/publisher-forget-user-button.spec.tsx @@ -84,8 +84,9 @@ describe('PublisherForgetUserButton', () => { expect(handleUserDeleted).toHaveBeenCalled(); }); - it('reports an error result to the page and leaves the dialog open', async () => { - const forgetUser = vi.fn().mockResolvedValue({ error: 'boom' }); + it('reports a rejected request to the page and leaves the dialog open', async () => { + // The service rejects on failure (`sendStrictRequest`), including a 200 carrying an error body. + const forgetUser = vi.fn().mockRejectedValue({ error: 'boom' }); const handleError = vi.fn(); const handleUserDeleted = vi.fn(); renderWithProviders( diff --git a/webui/test/unit/server-request.spec.ts b/webui/test/unit/server-request.spec.ts new file mode 100644 index 000000000..c10be978e --- /dev/null +++ b/webui/test/unit/server-request.spec.ts @@ -0,0 +1,96 @@ +/******************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + ********************************************************************************/ + +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { sendNonRetriableRequest, sendStrictRequest } from '../../src/server-request'; + +function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' } + }); +} + +function stubFetch(...responses: Response[]) { + const fetchMock = vi.fn(); + responses.forEach(response => fetchMock.mockResolvedValueOnce(response)); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; +} + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('sendStrictRequest', () => { + it('resolves with the parsed body of a successful response', async () => { + stubFetch(jsonResponse({ name: 'redhat' })); + + await expect(sendStrictRequest({ endpoint: 'https://open-vsx.org/api/redhat' })).resolves.toEqual({ + name: 'redhat' + }); + }); + + it('rejects with the error result when the server answers 200 with an error body', async () => { + stubFetch(jsonResponse({ error: 'Namespace not found' })); + + await expect(sendStrictRequest({ endpoint: 'https://open-vsx.org/api/nope' })).rejects.toEqual({ + error: 'Namespace not found' + }); + }); + + it('rejects with the error response of a failed request', async () => { + stubFetch(jsonResponse({ error: 'Forbidden', message: 'no access' }, 403)); + + await expect(sendStrictRequest({ endpoint: 'https://open-vsx.org/admin/namespace/x' })).rejects.toMatchObject({ + error: 'Forbidden', + status: 403 + }); + }); + + it('does not retry a server error - retries are owned by the query client', async () => { + const fetchMock = stubFetch(jsonResponse({ error: 'boom' }, 500)); + + await expect(sendStrictRequest({ endpoint: 'https://open-vsx.org/api/-/search' })).rejects.toMatchObject({ + status: 500 + }); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + // Regression test: sendRequest re-invokes itself on 429 without forwarding the original `retry` + // flag, so it used to fall back to the default `retry = true` - re-enabling fetch-retry's own + // retries for the follow-up request even though sendStrictRequest asked for none. A 500 after the + // 429 would then be retried by fetch-retry itself instead of surfacing as a single rejection. + it('does not re-enable retries for the request that follows a 429', async () => { + vi.useFakeTimers(); + const fetchMock = stubFetch(jsonResponse({}, 429), jsonResponse({ error: 'boom' }, 500)); + + // Attach the rejection assertion before letting the fake 429 delay elapse, so the rejection it + // triggers is never briefly unhandled. + const assertion = expect( + sendStrictRequest({ endpoint: 'https://open-vsx.org/api/-/search' }) + ).rejects.toMatchObject({ status: 500 }); + await vi.runAllTimersAsync(); + await assertion; + + expect(fetchMock).toHaveBeenCalledTimes(2); + vi.useRealTimers(); + }); +}); + +describe('sendNonRetriableRequest', () => { + it('resolves an error body instead of rejecting', async () => { + stubFetch(jsonResponse({ error: 'Namespace not found' })); + + await expect(sendNonRetriableRequest({ endpoint: 'https://open-vsx.org/api/nope' })).resolves.toEqual({ + error: 'Namespace not found' + }); + }); +});