Migrate to generated TypeScript client from ros2_medkit_clients#53
Open
Migrate to generated TypeScript client from ros2_medkit_clients#53
Conversation
There was a problem hiding this comment.
Pull request overview
Migrates the UI from the hand-maintained SovdApiClient to the generated @selfpatch/ros2-medkit-client-ts client, centralizing API path routing and response-shape normalization in the store layer to keep component interfaces stable.
Changes:
- Replaced the custom REST client with a generated OpenAPI client and added typed dispatch helpers (
api-dispatch.ts) for entity-type routing. - Introduced pure response mappers (
transforms.ts) and corresponding unit tests to reshape gateway responses into UI-friendly types. - Updated the Zustand store and multiple components to use store actions instead of direct client calls; removed the old client implementation/tests.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/utils.ts | Adds shared helpers (fault entity type mapping, byte/duration formatting) previously living in the old client. |
| src/lib/utils.test.ts | Adds unit coverage for new utility helpers. |
| src/lib/types.ts | Re-exports select generated-client schema types and keeps manual UI types. |
| src/lib/transforms.ts | Adds response transformation utilities extracted from the old client logic. |
| src/lib/transforms.test.ts | Adds unit tests validating the new transformation behaviors. |
| src/lib/api-dispatch.ts | Adds per-entity-type OpenAPI path dispatch wrappers (apps/components/areas/functions). |
| src/lib/store.ts | Switches to generated client, wires dispatch/transforms, updates fault streaming, and adds component-facing store actions. |
| src/lib/sovd-api.ts | Removes the legacy hand-maintained REST client implementation. |
| src/lib/sovd-api.test.ts | Removes tests for the legacy REST client. |
| src/components/TopicPublishForm.tsx | Switches topic publishing to store action instead of direct client usage. |
| src/components/DataPanel.tsx | Removes client prop dependency and delegates publish behavior to store actions. |
| src/components/EntityDetailPanel.tsx | Updates resource-count and data-loading flow to use store actions. |
| src/components/EntityResourceTabs.tsx | Migrates per-tab resource loads to store actions and store-backed configuration state. |
| src/components/AppsPanel.tsx | Replaces direct client calls with store actions for app resources. |
| src/components/FunctionsPanel.tsx | Replaces direct client calls with store actions for function resources and config counts. |
| src/components/FaultsPanel.tsx | Migrates faults list/detail/clear behaviors to store actions and shared utils mapper. |
| src/components/FaultsDashboard.tsx | Migrates fault detail loading to store action and shared utils mapper. |
| src/components/RosbagDownloadButton.tsx | Switches downloads to store action and moves formatting helpers to utils. |
| src/components/SnapshotCard.tsx | Moves formatting helper imports from old client to utils. |
| src/components/OperationsPanel.tsx | Updates SovdResourceEntityType import location post-migration. |
| src/components/ConfigurationPanel.tsx | Updates SovdResourceEntityType import location post-migration. |
| src/components/ServerInfoPanel.tsx | Switches server info fetches to store actions instead of direct client calls. |
| package.json | Adds generated client dependency (and OpenAPI fetch dependency). |
| .npmrc | Configures scoped registry for @selfpatch packages. |
src/lib/store.ts
Outdated
Comment on lines
+1049
to
+1056
| const apiPath = selectedPath.replace(/^\/server/, ''); | ||
| const details = await client.getEntityDetails(apiPath); | ||
| set({ selectedEntity: details, isRefreshing: false }); | ||
| const pathSegments = apiPath.split('/').filter(Boolean); | ||
| const entityType = (pathSegments[0] || 'areas') as SovdResourceEntityType; | ||
| const entityId = pathSegments[1] || ''; | ||
| const { data } = await (await import('./api-dispatch')).getEntityDetail(client, entityType, entityId); | ||
| if (data) { | ||
| set({ selectedEntity: data as unknown as SovdEntityDetails, isRefreshing: false }); | ||
| } else { |
There was a problem hiding this comment.
refreshSelectedEntity() derives entityType/entityId from selectedPath by taking the first two path segments. With the current tree paths (/server/<areaId>/<componentId>), the first segment after /server is an entity ID (not a SovdResourceEntityType), so the refresh will call the wrong endpoint and silently fail. Consider deriving entityType from selectedEntity.type (area/component/app/function) and using selectedEntity.id for the path params, or reusing the same path-parsing logic you use for resource navigation.
src/lib/store.ts
Outdated
Comment on lines
+1655
to
+1674
| const { client } = get(); | ||
| if (!client) return null; | ||
| const { getEntityBulkData } = await import('./api-dispatch'); | ||
| // For binary download, we need the URL to fetch directly with progress | ||
| // Build URL from client base and use fetch directly | ||
| const { data } = await getEntityBulkData(client, entityType, entityId, category); | ||
| if (!data) return null; | ||
| // Find the file descriptor to get download info | ||
| const items = (data as unknown as { items?: Array<{ id: string; name?: string }> })?.items || []; | ||
| const fileDesc = items.find((item) => item.id === fileId); | ||
| const filename = fileDesc?.name || fileId; | ||
| // Use the generated client for actual download | ||
| await import('./api-dispatch'); | ||
| // Construct the download URL manually since openapi-fetch doesn't support blob responses well | ||
| const baseUrl = (client as unknown as { baseUrl?: string }).baseUrl || ''; | ||
| const downloadUrl = `${baseUrl}/${entityType}/${entityId}/bulk-data/${category}/${fileId}`; | ||
| const response = await fetch(downloadUrl); | ||
| if (!response.ok) return null; | ||
| const blob = await response.blob(); | ||
| return { blob, filename }; |
There was a problem hiding this comment.
downloadBulkData() builds the download URL by reading (client as { baseUrl?: string }).baseUrl and concatenating path segments without encoding. This is brittle (relies on a non-public property that may be undefined) and can produce invalid URLs (double slashes, missing base path, unescaped category/fileId). Prefer using the known serverUrl (and any configured API base path) from store state, ensure each segment is encodeURIComponent’d, and avoid depending on internal client fields. Also consider adding basic error reporting (status/code) instead of returning null on a non-OK response to make failures diagnosable.
Suggested change
| const { client } = get(); | |
| if (!client) return null; | |
| const { getEntityBulkData } = await import('./api-dispatch'); | |
| // For binary download, we need the URL to fetch directly with progress | |
| // Build URL from client base and use fetch directly | |
| const { data } = await getEntityBulkData(client, entityType, entityId, category); | |
| if (!data) return null; | |
| // Find the file descriptor to get download info | |
| const items = (data as unknown as { items?: Array<{ id: string; name?: string }> })?.items || []; | |
| const fileDesc = items.find((item) => item.id === fileId); | |
| const filename = fileDesc?.name || fileId; | |
| // Use the generated client for actual download | |
| await import('./api-dispatch'); | |
| // Construct the download URL manually since openapi-fetch doesn't support blob responses well | |
| const baseUrl = (client as unknown as { baseUrl?: string }).baseUrl || ''; | |
| const downloadUrl = `${baseUrl}/${entityType}/${entityId}/bulk-data/${category}/${fileId}`; | |
| const response = await fetch(downloadUrl); | |
| if (!response.ok) return null; | |
| const blob = await response.blob(); | |
| return { blob, filename }; | |
| const { client, serverUrl } = get(); | |
| if (!client) return null; | |
| const { getEntityBulkData } = await import('./api-dispatch'); | |
| // For binary download, we need the URL to fetch directly with progress | |
| const { data } = await getEntityBulkData(client, entityType, entityId, category); | |
| if (!data) return null; | |
| // Find the file descriptor to get download info | |
| const items = (data as unknown as { items?: Array<{ id: string; name?: string }> })?.items || []; | |
| const fileDesc = items.find((item) => item.id === fileId); | |
| const filename = fileDesc?.name || fileId; | |
| if (!serverUrl) { | |
| toast.error('Server URL is not configured; cannot download file.'); | |
| return null; | |
| } | |
| // Construct the download URL manually since openapi-fetch doesn't support blob responses well. | |
| // Use the configured serverUrl and ensure each dynamic segment is URL-encoded. | |
| const normalizedBaseUrl = serverUrl.replace(/\/+$/, ''); | |
| const downloadPath = `/${entityType}/${encodeURIComponent( | |
| entityId | |
| )}/bulk-data/${encodeURIComponent(category)}/${encodeURIComponent(fileId)}`; | |
| const downloadUrl = `${normalizedBaseUrl}${downloadPath}`; | |
| try { | |
| const response = await fetch(downloadUrl); | |
| if (!response.ok) { | |
| toast.error( | |
| `Failed to download file (status ${response.status}${ | |
| response.statusText ? `: ${response.statusText}` : '' | |
| }).` | |
| ); | |
| return null; | |
| } | |
| const blob = await response.blob(); | |
| return { blob, filename }; | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error); | |
| toast.error(`Failed to download file: ${message}`); | |
| return null; | |
| } |
Comment on lines
102
to
105
| // Actions | ||
| connect: (url: string, baseEndpoint?: string) => Promise<boolean>; | ||
| connect: (url: string) => Promise<boolean>; | ||
| disconnect: () => void; | ||
| setTreeViewMode: (mode: TreeViewMode) => Promise<void>; |
There was a problem hiding this comment.
connect was changed to accept only (url: string), and baseEndpoint was removed from the store state. There are still call sites in the repo that pass a second argument / read baseEndpoint (e.g. App auto-connect and the connection dialog), which will fail type-checking and/or break auto-connect behavior. Either reintroduce baseEndpoint handling here (if still supported) or update all callers and any persisted-state migration to stop relying on it.
src/lib/store.ts
Outdated
Comment on lines
308
to
320
| const apiPath = path.replace(/^\/server/, ''); | ||
| const details = await client.getEntityDetails(apiPath); | ||
| const pathSegments = apiPath.split('/').filter(Boolean); | ||
| const entityType = (pathSegments[0] || 'apps') as SovdResourceEntityType; | ||
| const entityId = pathSegments[1] || ''; | ||
| const { getEntityDetail } = await import('./api-dispatch'); | ||
| const { data: detailData } = await getEntityDetail(client, entityType, entityId); | ||
| const details = (detailData || { id: entityId, name: entityId, type: entityType, href: apiPath }) as SovdEntityDetails; | ||
|
|
||
| // Update tree with full data merged with direction info | ||
| const updatedTree = updateNodeInTree(rootEntities, path, (n) => ({ | ||
| ...n, | ||
| data: { ...details.topicData, isPublisher, isSubscriber }, | ||
| data: { ...((details as unknown as Record<string, unknown>)?.topicData as Record<string, unknown>), isPublisher, isSubscriber }, | ||
| })); |
There was a problem hiding this comment.
Topic selection is currently fetching entity details via getEntityDetail() using entityType = pathSegments[0] and entityId = pathSegments[1]. With the current tree path scheme (/server/<areaId>/<componentId>/data/<topic>), pathSegments[0] is an area ID, not a SovdResourceEntityType, so this dispatch will hit the wrong endpoint. Also, getEntityDetail() responses won’t contain topicData, so details.topicData will be undefined and the tree update won’t actually enrich the topic node. Consider parsing around the data segment (like the old client did) and calling the data-item endpoint (getEntityDataItem) to populate selectedEntity.topicData (and update the node cache) instead of getEntityDetail().
src/lib/store.ts
Outdated
Comment on lines
532
to
539
| const apiPath = path.replace(/^\/server/, ''); | ||
| const details = await client.getEntityDetails(apiPath); | ||
| // Parse entity type and id from path: /areas/foo -> areas, foo | ||
| const pathSegments = apiPath.split('/').filter(Boolean); | ||
| const entityType = (pathSegments[0] || 'areas') as SovdResourceEntityType; | ||
| const entityId = pathSegments[1] || ''; | ||
| const { data } = await (await import('./api-dispatch')).getEntityDetail(client, entityType, entityId); | ||
| const details = (data || { id: entityId, name: entityId, type: entityType, href: apiPath }) as SovdEntityDetails; | ||
| set({ selectedEntity: details, isLoadingDetails: false }); |
There was a problem hiding this comment.
fetchEntityFromApi() assumes paths look like /<entityType>/<entityId> and dispatches via getEntityDetail(entityType, entityId). However, navigation from the detail panel builds resource paths like ${selectedPath}/data/<topic> (e.g. /server/<areaId>/<componentId>/data/<topic>), which won’t match this parsing and will call the wrong endpoint. This will break deep navigation to topics/operations/configurations/faults when those nodes aren’t present in the tree. Consider adding explicit handling for /data/, /operations/, /configurations/, /faults/ paths here (dispatch to the corresponding typed endpoints and transform into SovdEntityDetails), and only fall back to entity detail fetch for true entity paths.
Add generated TypeScript client and openapi-fetch peer dependency. Configure .npmrc for GitHub Packages registry.
Create standalone transform functions for fault, operations, data, configurations, and bulk data responses. These handle x-medkit vendor extension extraction and field renaming.
Move formatBytes, formatDuration, and mapFaultEntityTypeToResourceType to utils.ts in preparation for sovd-api.ts removal.
Add import of components schema from @selfpatch/ros2-medkit-client-ts. Re-export VersionInfo and GenericError (as SovdError) from generated schema where shapes match. Other types stay manual due to significant API differences - transforms.ts handles runtime mapping.
Route generic (entityType, entityId) calls to the correct per-entity typed API paths required by openapi-fetch. Covers configurations, data, operations, executions, faults, and bulk data.
Replace all SovdApiClient method calls with typed openapi-fetch GET/POST/PUT/DELETE calls via api-dispatch helpers. Migrate SSE fault streaming from EventSource to SseStream async iterable. Remove baseEndpoint from AppState (URL normalization is automatic).
Add fetchEntityData, fetchEntityOperations, listEntityFaults, getFaultWithEnvironmentData, publishToEntityData, getServerCapabilities, getVersionInfoAction, downloadBulkData, getFunctionHosts, and prefetchResourceCounts actions.
…th store actions Migrate 14 components from direct SovdApiClient usage to store actions: - Group 1 (import-only): SnapshotCard, ConfigurationPanel, OperationsPanel - Group 2 (import + client replacement): RosbagDownloadButton, FaultsPanel, FaultsDashboard, EntityResourceTabs, AppsPanel, FunctionsPanel, ServerInfoPanel - Group 3 (remove client prop chain): DataPanel, TopicPublishForm, EntityDetailPanel All imports updated from @/lib/sovd-api to @/lib/types or @/lib/utils. All direct client.method() calls replaced with store actions. Updated RosbagDownloadButton test to mock store action instead of client.
Delete sovd-api.ts (~2087 lines) and its tests. All API calls now go through the generated @selfpatch/ros2-medkit-client-ts client via the Zustand store and api-dispatch helpers.
- App.tsx: remove baseEndpoint from store selector and connect() call - ServerConnectionDialog.tsx: remove base endpoint input field entirely (generated client normalizes URLs automatically) - store.ts: remove unused dynamic import, use store serverUrl instead of fragile type cast for download URL construction
bburda
force-pushed
the
feat/generated-client-migration
branch
from
e1007d4 to
c7f8997
Compare
The publish UI was showing even when disconnected because the client null check was dropped during migration. Replaced with isConnected store check.
…lback fetch - refreshSelectedEntity: use selectedEntity.type/id instead of path parsing - handleTopicSelection: find parent entity from tree, fetch via getEntityDataItem - fetchEntityFromApi: use depth heuristic instead of treating path[0] as entityType - downloadBulkData: add encodeURIComponent for path segments - Replace all dynamic imports with static imports
- Add entity type field to raw API responses (areas, components, apps) - Fix topic detail: transform raw DataItem to ComponentTopic for schema - Fix SSE fault stream: pass fetch.bind(globalThis) to createMedkitClient - Fix fetchEntityFromApi: parse /data/ and /operations/ resource paths - Fix snapshot duplicate React keys in FaultsDashboard and FaultsPanel - Fix getFaultWithEnvironmentData: single request, no brute-force fallback - Remove console.log info lines, add console.error on all error paths - Rename browser tab to ros2_medkit Web UI - Remove all dynamic imports, use static imports only
- ServerConnectionDialog: title and description - EmptyState: connection prompt text - ServerInfoPanel: fallback server name - store.ts: fallback in loadRootEntities - Fix auto-connect loop in React strict mode - Add @testing-library/dom to fix peer dep resolution
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
Replace the hand-maintained REST client (
sovd-api.ts, ~2087 lines) with the generated TypeScript client from@selfpatch/ros2-medkit-client-ts. The Zustand store absorbs all API translation via dispatch helpers and response transforms, keeping the component layer's public interface unchanged.Key changes:
sovd-api.ts(2087 lines) and its testsapi-dispatch.ts(524 lines) - entity-type routing for openapi-fetch typed pathstransforms.ts(362 lines) - response reshaping (field renames, x-medkit extraction)store.ts- swapped SovdApiClient for MedkitClient, migrated SSE fault streaming to SseStream async iterableIssue
Type
Testing
npm test)npm run typecheck)npm run lint)Checklist
npm run lint)npm run build)