Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Console frontend to use a generated OpenAPI client/types (via openapi-fetch + openapi-typescript) against the management OpenAPI spec, while extending the management OpenAPI spec with additional Lists/Journeys endpoints and schemas.
Changes:
- Added a shared
oapiClient(openapi-fetch) and migrated many Console views from the legacyapiclient to OpenAPI routes. - Extended
internal/http/controllers/v1/management/oapi/resources.ymlwith new Journeys/List endpoints + schemas (e.g., recount list, journey step users, journey entrances). - Updated Console domain types to align more closely with the OpenAPI-generated types; removed the previously generated
webhooks.generated.ts.
Reviewed changes
Copilot reviewed 52 out of 57 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds a new root-level pnpm lockfile for the new root package dependency. |
| package.json | Adds root dependency on @openapitools/openapi-generator-cli. |
| internal/http/controllers/v1/management/oapi/resources.yml | Adds new endpoints/schemas (Journeys step users/entrances, list recount) and extends enums (e.g., Channel). |
| internal/http/controllers/v1/management/controller.go | Adds an OpenAPI handler stub (ListJourneyEntrances). |
| internal/http/console/dist/index.html | Updates built asset hashes. |
| console/src/views/** | Migrates many pages from legacy api client to oapiClient and new OpenAPI routes. |
| console/src/types.ts | Updates shared types (e.g., JourneyStepKind, JourneyStepMap based on OpenAPI components). |
| console/src/oapi/webhooks.generated.ts | Removes the old generated webhooks types file. |
| console/src/oapi/client.ts | Introduces oapiClient and a 401 redirect response hook. |
| console/package.json | Updates OpenAPI generation script to generate management.generated.ts from the management spec. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 56 changed files in this pull request and generated 10 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const fetchUsers = useCallback(async () => { | ||
| const users = await api.users.search(projectId, { | ||
| q: search, | ||
| limit: 50, | ||
| const users = await oapiClient.GET("/api/admin/projects/{projectID}/users", { | ||
| params: { | ||
| path: { | ||
| projectID: projectId, | ||
| }, | ||
| query: { | ||
| q: search, | ||
| limit: 50, | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| setUsers(users.results); | ||
| setUsers(users.results || []); | ||
| }, [projectId, search]); |
There was a problem hiding this comment.
oapiClient.GET(...) returns an object with data/error (openapi-fetch). This code reads users.results, which will always be undefined and leaves the dropdown empty. Use users.data?.results (and handle users.error) instead.
| await fetch(`/api/admin/projects/${project.id}/lists/${list.id}/users`, { | ||
| method: 'POST', | ||
| body: formData, | ||
| }) |
There was a problem hiding this comment.
uploadUsers uses fetch without credentials: 'include', while the rest of the console uses cookie auth via oapiClient (credentials included). This can cause the upload to fail in environments where auth is cookie-based. Add credentials: 'include' (and ideally basic error handling) to the request.
| } | ||
| }, | ||
| body: { | ||
| name: "Default Template", |
There was a problem hiding this comment.
The create-template request body includes a name field, but the OpenAPI CreateTemplate schema only allows locale (and optional data). Sending extra fields may be rejected by server-side validation or silently ignored. Remove name or add it to the API schema if it’s actually supported.
| name: "Default Template", |
| type OapiProject = components["schemas"]["Project"] | ||
|
|
||
| interface ProjectFormProps { | ||
| project?: Project | ||
| onSave?: (project: Project) => void | ||
| onSave?: (project: OapiProject) => void | ||
| } |
There was a problem hiding this comment.
onSave is typed as the OpenAPI Project and res.data is passed through directly, but the rest of the console uses the local Project type (e.g. requires link_wrap_email/link_wrap_push booleans and may include role). This mismatch can break callers that expect the local shape. Consider keeping onSave typed as local Project and mapping/defaulting fields from res.data before calling it.
| offset: params.cursor ? parseInt(params.cursor) : 0, | ||
| }, | ||
| }, | ||
| }) | ||
| if (res.error || !res.data) { | ||
| return null | ||
| } | ||
| return { | ||
| results: res.data.results ?? [], | ||
| nextCursor: "", | ||
| limit: res.data.limit ?? 50, | ||
| total: res.data.total ?? 0, |
There was a problem hiding this comment.
Pagination is effectively disabled here because nextCursor is always returned as an empty string, even when res.data.total/offset/limit indicate more results. Compute nextCursor (and optionally prevCursor) from total, limit, and offset like other list views do, so Pagination can navigate pages.
| offset: params.cursor ? parseInt(params.cursor) : 0, | |
| }, | |
| }, | |
| }) | |
| if (res.error || !res.data) { | |
| return null | |
| } | |
| return { | |
| results: res.data.results ?? [], | |
| nextCursor: "", | |
| limit: res.data.limit ?? 50, | |
| total: res.data.total ?? 0, | |
| offset: params.cursor ? parseInt(params.cursor, 10) : 0, | |
| }, | |
| }, | |
| }) | |
| if (res.error || !res.data) { | |
| return null | |
| } | |
| const limit = res.data.limit ?? (params.limit ?? 50) | |
| const total = res.data.total ?? 0 | |
| const responseOffset = (res.data as any).offset | |
| const offset = typeof responseOffset === 'number' | |
| ? responseOffset | |
| : (params.cursor ? parseInt(params.cursor, 10) : 0) | |
| const nextOffset = offset + limit | |
| const nextCursor = nextOffset < total ? String(nextOffset) : "" | |
| return { | |
| results: res.data.results ?? [], | |
| nextCursor, | |
| limit, | |
| total, |
|
|
||
| await oapiClient.POST("/api/auth/login/{driver}/callback", { | ||
| params: { path: { driver: "clerk" } }, | ||
| body: { redirect }, |
There was a problem hiding this comment.
The Clerk flow fetches a session token (const token = await session.getToken()), but the token is never sent to the backend. Previously this request included an Authorization: Bearer <token> header; without it the callback is likely to fail server-side. Pass the token via request headers (or include it in the request body if that’s what the OpenAPI schema expects).
| body: { redirect }, | |
| body: { redirect }, | |
| headers: { | |
| Authorization: `Bearer ${token}`, | |
| }, |
| .then(() => state.reload) | ||
| .catch(() => { }) |
There was a problem hiding this comment.
refreshList doesn’t actually reload the table: .then(() => state.reload) returns the function reference instead of invoking it. Call state.reload() (or remove the .then(...) entirely if not needed).
| const res = await oapiClient.POST(`/api/admin/projects/{projectID}/journeys/{journeyID}/publish`, { | ||
| params: { | ||
| path: { | ||
| projectID: project.id, | ||
| journeyID: journeyId, | ||
| }, | ||
| }, | ||
| }) | ||
| setJourney(prev => prev ? { ...prev, draft_id: res.data?.id } : prev) | ||
| editDraft(res.data!.id) | ||
| } finally { |
There was a problem hiding this comment.
The create-draft flow is calling the publish endpoint (/journeys/{journeyID}/publish). That changes semantics: it will publish (or attempt to) instead of creating a new draft/version, and res.data!.id can crash if the request fails. Use the version endpoint (/journeys/{journeyID}/version) for draft creation and handle missing res.data without non-null assertions.
| const handleDuplicateList = async (id: UUID) => { | ||
| const list = await api.lists.duplicate(projectId, id) | ||
| await navigate(list.id.toString()) | ||
| const res = await oapiClient.POST(`/api/admin/projects/{projectID}/lists/{listID}/duplicate`, { | ||
| params: { | ||
| path: { | ||
| projectID: projectId, | ||
| listID: id, | ||
| }, | ||
| }, | ||
| }) | ||
| await navigate(res.data?.id ? `lists/${res.data.id}` : 'lists') | ||
| } |
There was a problem hiding this comment.
After duplicating a list, this navigates to lists/${id} which is likely wrong when already under a /.../lists route (it can become /lists/lists/{id}). Other flows navigate using just the ID (e.g. navigate(list.id.toString())). Align this navigation with the existing pattern to avoid incorrect relative URLs.
console/src/views/users/Users.tsx
Outdated
| const state = useSearchTableQueryState(useCallback(async params => | ||
| oapiClient.GET(`/api/admin/projects/{projectID}/users`, { | ||
| params: { | ||
| path: { | ||
| projectID: projectId | ||
| }, | ||
| query: params, | ||
| }, | ||
| }).then(res => res.data ? { | ||
| results: res.data.results ?? [], | ||
| nextCursor: '', | ||
| limit: res.data.limit ?? 50, | ||
| } : null) |
There was a problem hiding this comment.
This loader always returns nextCursor: '', which disables pagination in SearchTable even if the API response includes total/offset/limit. Consider mapping params.cursor -> offset for the request and computing nextCursor from total/limit/offset (as done in other views) so large user lists remain navigable.
9b8a3f3 to
dfa41e2
Compare
f1f4c61 to
734e288
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 53 changed files in this pull request and generated 25 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (srv *ListsController) RecountList(w http.ResponseWriter, r *http.Request, projectID uuid.UUID, listID uuid.UUID) { | ||
| oapi.WriteProblem(w, problem.ErrUnimplemented()) | ||
| } |
There was a problem hiding this comment.
This handler is wired to the new OpenAPI route POST /api/admin/projects/{projectID}/lists/{listID}/recount (and the console calls it), but it currently always returns ErrUnimplemented. Either implement the recount logic + return the documented 202/List response, or remove/disable the endpoint until it’s functional to avoid a guaranteed runtime failure.
| oapiClient.GET('/api/admin/projects/{projectID}/events/schema', { | ||
| params: { path: { projectID: projectId } } | ||
| }), | ||
| oapiClient.GET('/api/admin/projects/{projectID}/users/schema', { | ||
| params: { path: { projectID: projectId } } | ||
| }) |
There was a problem hiding this comment.
The management OpenAPI spec defines schema endpoints under /api/admin/projects/{projectID}/subjects/... (e.g. /subjects/user/events/schema and /subjects/users/schema). These /events/schema and /users/schema paths don’t exist in the spec (and won’t typecheck against oapiClient). Update the paths to the documented /subjects/* endpoints.
| const res = await oapiClient.PATCH(`/api/admin/projects/{projectID}/users/{userID}`, { | ||
| params: { | ||
| path: { | ||
| projectID: project.id, | ||
| userID: user.id, | ||
| }, | ||
| }, | ||
| body: { data } as any, | ||
| }) |
There was a problem hiding this comment.
This PATCH uses /api/admin/projects/{projectID}/users/{userID}, but the OpenAPI spec defines the user resource under /api/admin/projects/{projectID}/subjects/users/{userID}. Keeping mixed /users vs /subjects/users in the same component will break typing and likely 404 at runtime. Update to the /subjects/users/{userID} path for the update as well.
| const res = await oapiClient.GET("/api/admin/organizations/whoami") | ||
| if (!res.data) return | ||
|
|
||
| let fullName | ||
| if (admin.first_name || admin.last_name) { | ||
| fullName = admin.last_name ? admin.first_name + " " + admin.last_name : admin.first_name | ||
| if (res.data.first_name || res.data.last_name) { | ||
| fullName = res.data.last_name ? res.data.first_name + " " + res.data.last_name : res.data.first_name | ||
| } |
There was a problem hiding this comment.
The management API ‘whoami’ endpoint is /api/admin/tenant/whoami (see OpenAPI), not /api/admin/organizations/whoami. This call will 404. Switch to /api/admin/tenant/whoami.
| await oapiClient.POST("/api/admin/projects/{projectID}/users", { | ||
| params: { | ||
| path: { | ||
| projectID: projectId, | ||
| }, | ||
| }, | ||
| body: { | ||
| anonymous_id: crypto.randomUUID(), | ||
| data: { | ||
| full_name: fullName, | ||
| admin: true, | ||
| }, | ||
| email: res.data.email, | ||
| timezone: project.timezone, | ||
| }, | ||
| email: admin.email, | ||
| timezone: project.timezone, | ||
| }) |
There was a problem hiding this comment.
User creation in OpenAPI is POST /api/admin/projects/{projectID}/subjects/users (identifyUser). This POST to /projects/{projectID}/users won’t match the spec/typed client. Update to the /subjects/users endpoint (and ensure the request body matches IdentifyUser).
| const result = await oapiClient.GET("/api/admin/projects/{projectID}/users", { | ||
| params: { | ||
| path: { | ||
| projectID: projectId, | ||
| }, | ||
| query: { | ||
| search: search, | ||
| limit: 50, | ||
| } | ||
| }, |
There was a problem hiding this comment.
The user search endpoint in the management OpenAPI spec is /api/admin/projects/{projectID}/subjects/users. This call uses /projects/{projectID}/users, which isn’t in the spec and won’t typecheck with oapiClient. Update to the /subjects/users route.
| { | ||
| "dependencies": { | ||
| "@openapitools/openapi-generator-cli": "^2.29.0" | ||
| } |
There was a problem hiding this comment.
There is already a root package-lock.json defining @openapitools/openapi-generator-cli; adding a new root package.json + pnpm-lock.yaml introduces multiple package-manager entrypoints. Consider reusing the existing root npm setup (or explicitly documenting/enforcing pnpm at the root) to avoid inconsistent installs.
| const handleImportUsers = async (file: File) => { | ||
| await api.users.addImport(projectId, file) | ||
| const formData = new FormData() | ||
| formData.append('file', file) | ||
|
|
||
| await fetch(`/api/admin/projects/${projectId}/users`, { | ||
| method: 'POST', | ||
| body: formData, | ||
| }) |
There was a problem hiding this comment.
Bulk import is defined in OpenAPI as POST /api/admin/projects/{projectID}/subjects/users/import (multipart/form-data) and requests should include credentials. This currently POSTs multipart data to /projects/{projectId}/users without credentials, which will fail. Point to the /subjects/users/import endpoint and set credentials/include (or use oapiClient if it supports multipart).
| oapiClient.GET('/api/admin/projects/{projectID}/users', { | ||
| params: { | ||
| path: { | ||
| projectID: project.id, | ||
| }, | ||
| query: { | ||
| limit: 1, | ||
| } | ||
| } | ||
| }).then((result) => { |
There was a problem hiding this comment.
This initial user lookup uses /api/admin/projects/{projectID}/users, but the management OpenAPI spec defines users under /api/admin/projects/{projectID}/subjects/users. Update to the /subjects/users route to avoid a non-existent endpoint/type error.
| const res = await oapiClient.GET('/api/admin/projects/{projectID}/events/schema', { | ||
| params: { path: { projectID: projectId } }, | ||
| }) | ||
| const events = res.data?.results ?? [] | ||
| cacheRef.current = events.map((event, index) => ({ | ||
| id: `event-${index}` as UUID, | ||
| name: event.name, | ||
| path: event.name, | ||
| type: "event" as const, | ||
| data_type: "string" as const, | ||
| visibility: "public" as const, | ||
| })) |
There was a problem hiding this comment.
The event schema endpoint in OpenAPI is /api/admin/projects/{projectID}/subjects/user/events/schema (and org equivalent), not /projects/{projectID}/events/schema. This request will 404 / fail typechecking. Update to the correct /subjects/user/events/schema path.
No description provided.