chore(admin): typesafe API client + TanStack Query rails (#7638)#7695
Open
JohnMcLear wants to merge 2 commits intoether:developfrom
Open
chore(admin): typesafe API client + TanStack Query rails (#7638)#7695JohnMcLear wants to merge 2 commits intoether:developfrom
JohnMcLear wants to merge 2 commits intoether:developfrom
Conversation
…#2) * docs(admin): design for typesafe API client + TanStack Query rails (ether#7638) Rails-only scope: codegen toolchain, runtime client, and provider — no call-site migrations. Admin endpoints are not yet covered by the OpenAPI spec, so a separate issue will follow before any migration is useful. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(admin): implementation plan for typesafe API client rails (ether#7638) Step-by-step task breakdown for the rails-only PR: codegen toolchain, runtime client, TanStack Query provider, CI freshness check, docs. No call-site migrations until admin endpoints are added to the OpenAPI spec (separate follow-up). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(api): export generateDefinitionForVersion from openapi hook Required by the admin codegen script (ether#7638) to dump the OpenAPI spec without booting Express. No behavior change for the request hook. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(admin): add OpenAPI codegen + TanStack Query deps (ether#7638) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(admin): add OpenAPI spec dump entry (ether#7638) Loaded via tsx by gen-api.mjs in the next commit. Writes JSON to a file path argument so log4js stdout output (from Settings init) does not pollute the spec output. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(admin): wire OpenAPI codegen into build (ether#7638) Adds gen:api script and amends build/build-copy to regenerate admin/src/api/schema.d.ts before compiling. The generated file is checked in so it shows up in PR review and so a fresh checkout doesn't need codegen to typecheck. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(admin): typed openapi-fetch + react-query client (ether#7638) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(admin): TanStack Query provider, dev-only devtools (ether#7638) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(admin): mount TanStack Query provider at root (ether#7638) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(admin): smoke test for typed openapi-fetch client (ether#7638) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci(admin): verify generated OpenAPI schema is up to date (ether#7638) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(admin): document OpenAPI codegen workflow (ether#7638) Replaces the default Vite scaffold README with admin-specific scripts table and codegen workflow notes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * build(admin): exclude __tests__ from tsc include (ether#7638) The smoke test imports node:test/node:assert which need @types/node. Admin source is browser-only, so excluding __tests__ from the production typecheck is cleaner than adding Node types to the bundle config. The test still runs under tsx, which doesn't share this constraint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: regenerate lockfile with pnpm 10 to restore overrides block (ether#7638) Adding admin deps with pnpm 11 stripped the top-level \`overrides:\` section from pnpm-lock.yaml, which CI uses pnpm 10 to verify. Result: ERR_PNPM_LOCKFILE_CONFIG_MISMATCH on every job. Re-running pnpm 10 \`install --lockfile-only\` restores the overrides block; the new admin package entries land in the same commit. Two stale lockfile entries not present in package.json (\`serialize-javascript\` version pin and \`uuid@<14.0.0\`) were normalized by the regen — package.json is the source of truth for those. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * build(docker): preserve strictDepBuilds=false in trimmed workspace yaml (ether#7638) Adding tsx as an admin devDep brings esbuild@0.27.x into the resolved subgraph, and the Dockerfile's runtime stage was overwriting pnpm-workspace.yaml with a stripped-down version that lost the strictDepBuilds=false setting from the source repo. With pnpm 10's default of strictDepBuilds=true, the install then errors on ERR_PNPM_IGNORED_BUILDS for esbuild + scarf rather than warning. Restore the strictDepBuilds=false and the @scarf/scarf ignore in the trimmed yaml so the production install matches develop's behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(admin): point client baseUrl at /api/<version> via codegen (ether#7638) Qodo flagged: with baseUrl='/' and schema paths like '/createGroup', calls landed at /createGroup, but the backend mounts the FLAT-style spec under /api/<version>/. So once a call site lands, every request 404s. gen:api now also emits admin/src/api/version.ts containing LATEST_API_VERSION (read from info.version in the spec) and a derived API_BASE_URL = `/api/<version>`. client.ts imports API_BASE_URL. Workflow freshness check covers both generated files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * build(admin): cross-platform spawn in gen-api.mjs (ether#7638) Windows CI failed because spawnSync('pnpm', ...) cannot resolve pnpm.cmd without a shell. Set shell:true on win32 only so Linux/macOS runs avoid Node's DEP0190 warning. All spawn args are literal strings, so the shell variant is not an injection risk. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Member
Author
|
/review |
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Code Review by Qodo
1. Schema lacks /admin-auth paths
|
Review Summary by QodoSet up typesafe OpenAPI admin API client with TanStack Query and codegen rails
WalkthroughsDescription• Establishes foundational infrastructure for a typesafe, OpenAPI-derived admin API client backed by TanStack Query • Implements codegen toolchain (pnpm --filter admin gen:api) that produces admin/src/api/schema.d.ts and admin/src/api/version.ts from the OpenAPI spec builder • Creates runtime client (openapi-fetch + openapi-react-query) at admin/src/api/client.ts with automatic baseUrl configuration pointing to /api/<version>/ • Mounts <QueryProvider> at admin root with dev-only React Query devtools (excluded from production bundle) • Adds CI freshness check in frontend-admin-tests.yml to ensure generated files stay synchronized with the source spec • Includes comprehensive documentation (design spec, implementation plan, and updated admin README) explaining the workflow • Adds smoke test verifying the API client module loads correctly • Exports OpenAPI spec builder functions from core etherpad modules to enable external codegen pipeline • No call sites migrated yet; admin endpoints must be added to OpenAPI spec before migrations are useful Diagramflowchart LR
A["OpenAPI Spec<br/>in Express"] -->|"dump-spec.ts"| B["OpenAPI JSON"]
B -->|"openapi-typescript"| C["schema.d.ts<br/>version.ts"]
C -->|"import"| D["client.ts<br/>fetchClient + $api"]
D -->|"use"| E["React Components"]
F["QueryProvider.tsx"] -->|"wrap"| E
G["CI Check"] -->|"verify"| C
File Changes1. admin/src/api/schema.d.ts
|
Code Review by Qodo
Context used 1. schema.d.ts missing /admin-auth paths
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+65
to
+67
| The admin endpoints are not yet present in the OpenAPI spec — this client is | ||
| in place to support upcoming work (see issue #7638 follow-up). For now, it is | ||
| exercised only by the smoke test. |
There was a problem hiding this comment.
1. schema.d.ts missing /admin-auth paths 📎 Requirement gap ≡ Correctness
The generated admin OpenAPI schema/types do not include any /admin-auth/* operations, so the typed client cannot cover the admin authentication endpoints the UI uses. This fails the requirement that /admin-auth/* be represented in the OpenAPI output used for codegen.
Agent Prompt
## Issue description
The OpenAPI-generated admin types do not include `/admin-auth/*` operations, so the typed client cannot be used for existing admin auth calls.
## Issue Context
The admin schema is generated from the OpenAPI hook output, and the compliance requirement for this PR expects `/admin-auth/*` endpoints to be included in that output so `admin/src/api/schema.d.ts` exposes typed paths/operations.
## Fix Focus Areas
- src/node/hooks/express/openapi.ts[422-575]
- admin/src/api/schema.d.ts[9-31]
- admin/README.md[65-67]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+7
| // GENERATED — do not edit. Run `pnpm --filter admin gen:api` to regenerate. | ||
| // Source: src/node/hooks/express/openapi.ts (#7638) | ||
|
|
||
| /** | ||
| * This file was auto-generated by openapi-typescript. | ||
| * Do not make direct changes to the file. | ||
| */ |
There was a problem hiding this comment.
2. Generated schema committed 📘 Rule violation ⚙ Maintainability
This PR adds generated files (admin/src/api/schema.d.ts, admin/src/api/version.ts) and documents/enforces committing them via CI. This violates the policy against committing generated build/runtime artifacts.
Agent Prompt
## Issue description
Generated files (`admin/src/api/schema.d.ts`, `admin/src/api/version.ts`) are committed and CI enforces that workflow, which violates the repository compliance rule against committing generated artifacts.
## Issue Context
The generator is already wired into the admin build (`pnpm gen:api`), so the repo can instead generate these files during build/CI without storing outputs in git.
## Fix Focus Areas
- admin/src/api/schema.d.ts[1-7]
- admin/src/api/version.ts[1-5]
- admin/package.json[7-20]
- .github/workflows/frontend-admin-tests.yml[71-80]
- admin/README.md[34-40]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+9
to
+31
| export interface paths { | ||
| "/createGroup": { | ||
| parameters: { | ||
| query?: never; | ||
| header?: never; | ||
| path?: never; | ||
| cookie?: never; | ||
| }; | ||
| /** creates a new group */ | ||
| get: operations["createGroupUsingGET"]; | ||
| put?: never; | ||
| /** creates a new group */ | ||
| post: operations["createGroupUsingPOST"]; | ||
| delete?: never; | ||
| options?: never; | ||
| head?: never; | ||
| patch?: never; | ||
| trace?: never; | ||
| }; | ||
| "/createGroupIfNotExistsFor": { | ||
| parameters: { | ||
| query?: never; | ||
| header?: never; |
There was a problem hiding this comment.
1. Schema lacks /admin-auth paths 📎 Requirement gap ≡ Correctness
The generated admin/src/api/schema.d.ts does not include any /admin-auth/* (or /admin/*) paths, and the updated README explicitly notes admin endpoints are not yet in the OpenAPI spec. This fails the requirement that the OpenAPI-derived admin schema reflects the admin UI authentication/admin endpoint surface.
Agent Prompt
## Issue description
The generated admin OpenAPI TypeScript schema does not expose any `/admin-auth/*` or `/admin/*` typed paths, so the new typed client cannot be used for real admin UI traffic.
## Issue Context
The compliance checklist requires the OpenAPI spec (and therefore the generated `admin/src/api/schema.d.ts`) to include the admin authentication and admin endpoint surfaces.
## Fix Focus Areas
- src/node/hooks/express/openapi.ts[769-774]
- admin/src/api/schema.d.ts[9-31]
- admin/README.md[65-67]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+16
to
+44
| const tmpDir = mkdtempSync(path.join(tmpdir(), 'etherpad-openapi-')); | ||
| const specPath = path.join(tmpDir, 'spec.json'); | ||
|
|
||
| // On Windows pnpm resolves to pnpm.cmd, which spawnSync can only find via a | ||
| // shell. Use shell on Windows only to avoid Node's DEP0190 warning elsewhere. | ||
| // Every argument here is fixed (no user input) so the shell:true variant is | ||
| // not an injection risk. | ||
| const spawnOpts = { | ||
| cwd: adminRoot, | ||
| stdio: 'inherit', | ||
| shell: process.platform === 'win32', | ||
| }; | ||
|
|
||
| try { | ||
| const dump = spawnSync( | ||
| 'pnpm', | ||
| ['exec', 'tsx', 'scripts/dump-spec.ts', specPath], | ||
| spawnOpts, | ||
| ); | ||
| if (dump.status !== 0) { | ||
| console.error(`dump-spec.ts failed with exit code ${dump.status}`); | ||
| process.exit(dump.status ?? 1); | ||
| } | ||
|
|
||
| const gen = spawnSync( | ||
| 'pnpm', | ||
| ['exec', 'openapi-typescript', specPath, '-o', outFile], | ||
| spawnOpts, | ||
| ); |
There was a problem hiding this comment.
3. Windows gen:api arg splitting 🐞 Bug ☼ Reliability
admin/scripts/gen-api.mjs uses shell: true on Windows while passing file paths (tmpdir + repo paths) as arguments; if those paths contain spaces (common in Windows user temp dirs), cmd.exe tokenization can break the pnpm exec ... <path> calls and make pnpm gen:api fail.
Agent Prompt
### Issue description
`admin/scripts/gen-api.mjs` sets `shell: process.platform === 'win32'` and then calls `spawnSync('pnpm', [..., specPath], spawnOpts)`. On Windows, enabling the shell can cause argument parsing issues for paths that include spaces (common for user temp directories), breaking `pnpm gen:api`.
### Issue Context
The script enables `shell` only to make `pnpm` resolvable on Windows. This can be solved without `shell: true` by invoking the correct executable name on Windows.
### Fix Focus Areas
- admin/scripts/gen-api.mjs[19-54]
### Suggested change
- Use `const pnpmCmd = process.platform === 'win32' ? 'pnpm.cmd' : 'pnpm';`
- Keep `shell: false` (or omit it) in `spawnOpts`.
- Replace both `spawnSync('pnpm', ...)` calls with `spawnSync(pnpmCmd, ...)`.
This avoids cmd.exe parsing entirely while still working on Windows.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…pt (ether#7638) Qodo flagged the committed admin/src/api/schema.d.ts as a build artifact that violates the rule against committing generated files (rule 467291, "Exclude build artifacts and runtime-generated files from version control"). This commit: - Adds admin/src/api/{schema.d.ts,version.ts} to .gitignore. - Removes both from VCS (the prior squash had committed them). - Chains \`gen:api\` into the dev and test scripts so a fresh checkout lands a working dev server / test run without an extra step. build and build-copy already chained gen:api. - Drops the now-redundant CI freshness diff step from frontend-admin-tests.yml — with the files no longer committed, the build step's gen:api invocation is the only check needed. - Updates admin/README.md to describe the new generated-file workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Qodo's earlier rule violation on the JohnMcLear/etherpad fork PR (rule 467291: "Exclude build artifacts and runtime-generated files from version control") is now addressed in 40fe8a4:
|
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
1 participant
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.
Summary
Lays down the rails for a typesafe, OpenAPI-derived admin API client backed by TanStack Query. Closes #7638.
pnpm --filter admin gen:api) producingadmin/src/api/schema.d.tsandadmin/src/api/version.tsfromsrc/node/hooks/express/openapi.ts.openapi-fetch+openapi-react-query) atadmin/src/api/client.tswithbaseUrl = \/api/${LATEST_API_VERSION}`so calls land at/api//createGroup`.<QueryProvider>mounted at the admin root with dev-only React Query devtools (verified absent from the production bundle).frontend-admin-tests.yml.admin/README.mddocumenting the workflow.No call sites migrated. Admin endpoints aren't in the OpenAPI spec yet — that gap is filed as #7693 and must land before any migration is useful.
Why one squashed commit
This was developed iteratively on the fork (JohnMcLear#2 — merged into the fork's
develop) with 16 small fixup commits along the way (Windows shell, Dockerfile workspace yaml, lockfile overrides, baseUrl emit, etc.). Squashed for upstream so review is one cohesive change rather than the back-and-forth.Semver
Patch — build tooling + currently-unused runtime libs, no observable behavior change.
Test plan
pnpm --filter admin gen:apiruns clean (48 paths, OpenAPI 3.0.2, version 1.3.1)pnpm --filter admin run buildsucceeds; production bundle excludesReactQueryDevtoolspnpm --filter admin testpasses (smoke test: 1 pass)pnpm ts-checkclean for admin/openapi files@tanstack/react-query-devtools(verifiedimport.meta.env.DEV === trueand the lazy import is rendered)🤖 Generated with Claude Code