feat(ui): migrate the tags hooks to the typed fetchClient - #29886
feat(ui): migrate the tags hooks to the typed fetchClient#29886ryan-crabbe-berri wants to merge 7 commits into
Conversation
…al fetch pattern
Introduces fetchClient (openapi-fetch) bound to schema.d.ts so dashboard data fetching infers paths, query params, and request bodies from the proxy's OpenAPI spec. It is used inside ordinary TanStack Query hooks (fetchClient.GET("/path")), keeping the existing useQuery plus query-key-factory style rather than a wrapper API.
A small runtime registry feeds the client the mutable base URL and auth header name (registered by networking) and the session token (published by AuthContext), so call sites carry no token plumbing. Auth-header injection and ApiError mapping live in openapi-fetch middleware, reusing deriveErrorMessage and ApiError from the existing http client; because non-2xx maps to a thrown ApiError, query functions just read .data.
Untyped responses (most GETs lack a response_model, so they arrive as unknown) are cast through a single documented UntypedApiResponse alias to keep call sites uniform. Converts useModelHub to fetchClient.GET("/model_group/info") as the first call site; legacy networking helpers are untouched.
…itellm_ui_fetch_migration_wip
Converts useTags from the legacy tagListCall networking helper to fetchClient.GET("/tag/list"), keeping its TagListResponse contract and query key. The auth token is injected by the client middleware, so the hook no longer threads accessToken into the call. The test mocks fetchClient instead of networking and keeps the success, error, and auth-gating coverage.
Adds a no-restricted-imports override for src/app/(dashboard)/hooks/tags so the folder cannot reach back for networking helpers; the ban patterns are factored into shared constants to avoid duplicating the existing tremor ban. First domain migrated onto the pattern from the fetchClient PR (#29884), on which this stacks.
Branch CI note: schema.d.ts is generated; CI regenerates and diffs it via gen:api.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR introduces the typed
Confidence Score: 4/5Safe to merge after restoring the TagListResponse return type in useTags; all runtime behaviour is unchanged and the new client is well-tested. The only real defect is in useTags.ts: the public return type was changed from the concrete TagListResponse to any (via UntypedApiResponse), even though TagListResponse is still importable from @/components/tag_management/types — a path not covered by the networking ban. Every existing caller that relied on TypeScript to catch misuse of tag data now gets untyped any instead. The rest of the change (runtime module, middleware, auth wiring, ESLint ratchet) is clean and correct. ui/litellm-dashboard/src/app/(dashboard)/hooks/tags/useTags.ts — the return type regression is the one thing to fix before merging.
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/app/(dashboard)/hooks/tags/useTags.ts | Migrates useTags to fetchClient, but drops the TagListResponse return type in favour of UntypedApiResponse (any), breaking type safety for callers unnecessarily. |
| ui/litellm-dashboard/src/lib/http/api.ts | New typed openapi-fetch singleton with URL-rebasing and auth-header middleware; clean implementation with well-structured error handling. |
| ui/litellm-dashboard/src/lib/http/runtime.ts | Module-level mutable state for base URL getter, auth header name getter, and auth token; sensible defaults and clear JSDoc. |
| ui/litellm-dashboard/src/app/(dashboard)/hooks/models/useModels.ts | useModelHub migrated to fetchClient; no pre-existing typed return to preserve here, and data ?? {} fallback is correct. |
| ui/litellm-dashboard/eslint.config.mjs | Extracts ban constants, adds per-folder networking import ban for hooks/tags — correct pattern for incremental migration enforcement. |
| ui/litellm-dashboard/src/contexts/AuthContext.tsx | Adds setAuthToken calls alongside the existing setAccessToken, keeping the runtime module's token in sync with the auth context. |
| ui/litellm-dashboard/src/components/networking.tsx | Registers the proxy base URL and auth header name getters into runtime.ts at module-load time; minimal and correct change. |
| ui/litellm-dashboard/src/lib/http/api.test.ts | Thorough unit tests covering auth injection, URL rebasing, non-2xx error mapping, and successful body parsing. |
| ui/litellm-dashboard/src/app/(dashboard)/hooks/tags/useTags.test.ts | Tests updated to mock fetchClient instead of networking; covers success, error, no-body fallback, and all three auth-guard conditions. |
Reviews (3): Last reviewed commit: "fix(ui): resolve the default request bas..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export const fetchClient = createFetchClient<paths>({ baseUrl: globalThis.location?.origin ?? "" }); | ||
| fetchClient.use(middleware); |
There was a problem hiding this comment.
networking.tsx
fetchClient is a module-level singleton. Its middleware reads getRequestBaseUrl() and getAuthHeaderName(), which only return non-default values after networking.tsx has been imported and its two register* calls have run. If any consumer of fetchClient is bundled into a chunk that loads before networking.tsx (e.g. a lazy-loaded route that doesn't transitively import networking), every request in that chunk will silently use "" as the base URL and "Authorization" as the header name — both wrong if the proxy lives on a different origin or uses a custom header. Documenting the expected load order in the module JSDoc (or adding a runtime assertion) would make this coupling explicit.
Addresses review feedback on the migrated hooks. useTags now returns UntypedApiResponse (matching useModelHub and the untyped-endpoint convention) since /tag/list has no response_model yet; swap in the real type once the backend declares it. Both query functions guard a missing 2xx body with ?? {} so an empty response resolves to an empty object instead of undefined cast to a value type, with a regression test for each.
Leaves the api.ts load-order note (base URL / header getters registered by networking) for a follow-up.
|
@greptileai re review |
Addresses the load-order coupling review comment. fetchClient's middleware reads the base URL from runtime.ts, which defaulted to "" until networking.tsx ran its register call, so a request firing before networking imported would target same-origin even on a split-origin deployment. The default now resolves from the NEXT_PUBLIC_BASE_URL build constant via the shared resolveApiBase, so it is correct without depending on networking loading first; networking still overrides it with the fuller runtime value including the UI-config server root path. The auth header name has no build-time source and stays "Authorization" until the JWT supplies a custom one. Adds runtime.test.ts covering the env-resolved default and the header-name default.
|
@greptileai re review |
The UntypedApiResponse (any) return broke build-ui: create_key_button does Object.values(tagsData).map((tag) => tag.name), and Object.values(any) infers unknown[] under this tsconfig, so tag.name failed to type-check. /tag/list has a good hand-written response type, so useTags keeps TagListResponse (the empty-body guard stays). useModelHub stays UntypedApiResponse since /model_group/info has no modeled response.
…yped Tags was a poor first target: /tag/list has no response_model, so the openapi-fetch version only kept the hand-written TagListResponse behind a cast and broke a consumer when reduced to any. Restore useTags, its test, and the eslint config to the original networking-based version. Tags can be migrated later once the endpoint has a response_model so it is fully typed end to end.
|
Superseding this with #29884, which is the clean foundation off staging plus the first fully-typed caller migration (useCustomers). #29886 had picked up an unrelated, merge-induced schema drift and was migrating an untyped endpoint (tags); #29884 avoids both. Tags will be migrated later once /tag/list has a response_model. |
1 participant
Relevant issues
Stacks on #29884 (the typed
fetchClient). Until that merges, this PR's diff also shows thefetchClientfoundation; once it lands the diff reduces to the tags changes.Linear ticket
N/A
Pre-Submission checklist
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewType
Refactoring
Changes
First domain migrated onto the typed fetch pattern.
useTagspreviously called the hand-writtentagListCallnetworking helper; it now callsfetchClient.GET("/tag/list")and reads.data, keeping itsTagListResponsecontract and its existing query key. The auth token is injected by the client middleware, so the hook no longer threadsaccessTokeninto the call; it still gates the query onaccessToken,userId, anduserRoleexactly as beforeThe test mocks
fetchClientinstead ofnetworkingand keeps the meaningful coverage: it asserts the hook fetches/tag/list, unwraps the body on success, surfaces errors, and does not fetch when any auth value is missing. Each test rebuilds its ownQueryClientso there is no cross-test cache bleedTo stop the folder from regressing, a
no-restricted-importsoverride bans@/components/networkingimports undersrc/app/(dashboard)/hooks/tags. The ban patterns (this one and the existing tremor ban) are pulled into shared constants so the override does not duplicate the tremor rule. This is the per-folder enforcement ratchet that will flip on for each domain as it is migrated, the same mechanism that already bans rawfetchoutsidesrc/lib/httpScreenshots / Proof of Fix
The Tags page reads its data through
useTags, so it exercises the new client end to end. With a proxy running on localhost:4000:python litellm/proxy/proxy_cli.py --config litellm/proxy/dev_config.yaml --detailed_debug --reload --use_v2_migration_resolver 2>&1 | tee litellm.log/tag/listcarries the bearer auth header and returns 200 through the new client