Repository navigation
feat(plugins): plugin UI-extensibility — admin pages, render routes, sidebar (split from #3668) - #3785
feat(plugins): plugin UI-extensibility — admin pages, render routes, sidebar (split from #3668)#3785oyi77 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces plugin UI extensibility, adding a dynamic plugin page component, API routes for rendering plugin pages and fetching UI extensions, and dynamic sidebar integration. The review feedback highlights a critical authentication mismatch where dashboard client-side fetches will fail with a 401 error if API key authentication is strictly required. Additionally, the reviewer recommends using an AbortController to prevent race conditions in the page component, wrapping request.json() in a try-catch block with Zod validation to prevent unhandled exceptions, refactoring duplicated authentication helpers, and correcting the unit tests to properly verify authentication behavior instead of just asserting function existence.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async function checkAuth(request: Request): Promise<Response | null> { | ||
| const apiKeyRaw = extractApiKey(request); | ||
| if (isRequireApiKeyEnabled() && !apiKeyRaw) { | ||
| return errorResp(HTTP_STATUS.UNAUTHORIZED, "Authentication required"); | ||
| } | ||
| if (apiKeyRaw && !(await isValidApiKey(apiKeyRaw))) { | ||
| return errorResp(HTTP_STATUS.UNAUTHORIZED, "Invalid API key"); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
If isRequireApiKeyEnabled() is active, this route strictly requires an API key. However, the dashboard client-side fetches (such as the ones in Sidebar.tsx and the plugin page component) do not provide an API key in their headers. This will cause the plugin pages and sidebar integration to fail with a 401 Unauthorized error for authenticated dashboard users.
Consider allowing session-based authentication (e.g., validating the user's session cookie) as an alternative to API key authentication for these dashboard-facing routes, or retrieve the API key from the dashboard context and pass it in the fetch headers.
| useEffect(() => { | ||
| setLoading(true); | ||
| setError(null); | ||
| fetch(`/api/plugins/${name}/render?page=${slug}`) | ||
| .then((res) => { | ||
| if (!res.ok) throw new Error("Failed to load plugin page"); | ||
| return res.json(); | ||
| }) | ||
| .then((data) => { | ||
| setContent(data.content); | ||
| }) | ||
| .catch((err) => { | ||
| setError(err.message); | ||
| }) | ||
| .finally(() => { | ||
| setLoading(false); | ||
| }); | ||
| }, [name, slug]); |
There was a problem hiding this comment.
To prevent race conditions and memory leaks when the component unmounts or when the name or slug parameters change rapidly, use an AbortController to cancel any pending fetch requests.
useEffect(() => {
const controller = new AbortController();
setLoading(true);
setError(null);
fetch(`/api/plugins/${name}/render?page=${slug}`, { signal: controller.signal })
.then((res) => {
if (!res.ok) throw new Error("Failed to load plugin page");
return res.json();
})
.then((data) => {
setContent(data.content);
})
.catch((err) => {
if (err.name !== "AbortError") {
setError(err.message);
}
})
.finally(() => {
if (!controller.signal.aborted) {
setLoading(false);
}
});
return () => {
controller.abort();
};
}, [name, slug]);
| const body = await request.json(); | ||
| const { page, params: pageParams } = body; |
There was a problem hiding this comment.
Calling request.json() outside of a try-catch block will cause an unhandled exception and a 500 Internal Server Error if the request body is empty or contains invalid JSON. Additionally, the input should be validated using a Zod schema to comply with the repository style guide.
let body;
try {
body = await request.json();
} catch (err) {
return NextResponse.json({ error: "Invalid JSON body" }, { status: 400 });
}
const { page, params: pageParams } = body;References
- Always validate inputs with Zod schemas from src/shared/validation/schemas.ts. (link)
| function errorResp(status: number, message: string): Response { | ||
| return new Response(JSON.stringify(buildErrorBody(status, sanitizeErrorMessage(message))), { | ||
| status, | ||
| headers: { "Content-Type": "application/json", ...CORS_HEADERS }, | ||
| }); | ||
| } | ||
|
|
||
| async function checkAuth(request: Request): Promise<Response | null> { | ||
| const apiKeyRaw = extractApiKey(request); | ||
| if (isRequireApiKeyEnabled() && !apiKeyRaw) { | ||
| return errorResp(HTTP_STATUS.UNAUTHORIZED, "Authentication required"); | ||
| } | ||
| if (apiKeyRaw && !(await isValidApiKey(apiKeyRaw))) { | ||
| return errorResp(HTTP_STATUS.UNAUTHORIZED, "Invalid API key"); | ||
| } | ||
| return null; | ||
| } |
| it("render route GET calls checkAuth", async () => { | ||
| const mod = await import("../../src/app/api/plugins/[name]/render/route.ts"); | ||
| assert.equal(typeof mod.GET, "function"); | ||
| }); | ||
|
|
||
| it("ui-extensions route GET calls checkAuth", async () => { | ||
| const mod = await import("../../src/app/api/plugins/ui-extensions/route.ts"); | ||
| assert.equal(typeof mod.GET, "function"); | ||
| }); |
There was a problem hiding this comment.
These tests claim to verify that the render and ui-extensions routes call checkAuth, but they only assert that typeof mod.GET === "function". This is a duplicate assertion of the previous tests and does not actually verify any authentication behavior.
To properly test this, you should call the route handler with a mock Request (without an API key) and assert that it returns a 401 status code when isRequireApiKeyEnabled() is active.
074a818 to
dc5ebfa
Compare
|
Force-pushed with clean commits — branch now contains only 3 plugin UI commits (was 33 commits with 30 unrelated changes). |
51044a9 to
7422a53
Compare
|
Thanks @oyi77 for the continued work on the plugin system (#3668 split — UI-extensibility). This is conflicting against
Keeping it open and queued — thanks for the patience! |
|
Acknowledged — rebasing onto release/v3.8.30 is in progress. The plugin UI-extensibility is held pending the maintainer's product decision on milestone targeting. Keeping open as queued. 🙏 |
7422a53 to
466bd40
Compare
466bd40 to
6e45e36
Compare
6e45e36 to
7fd345b
Compare
7fd345b to
f06597a
Compare
f06597a to
3d82447
Compare
3d82447 to
1ef2e7e
Compare
1ef2e7e to
a0ffe0a
Compare
…C+D) (cheaper-inference#4622) C — scripts/quality/validate-release-green.mjs (npm run check:release-green): reproduces the release-equivalent validation (typecheck, eslint, db-rules, public-creds, full unit, vitest, ratchets, optional --with-build package-artifact) against the current working tree and classifies each red as HARD (real defect, exit 1) vs DRIFT (ratchet — reported, never affects exit / never blocks). Pure helpers exported + orchestration behind a direct-run guard; unit-tested. D — .github/workflows/nightly-release-green.yml: runs C on the active release branch nightly (and on workflow_dispatch) and opens/updates a single tracking issue on HARD failures. Never a required check, never touches a contributor PR. Closes the gap where the full gate (ci.yml) only ran on the release PR, so reds accrued silently on release/** and surfaced in 40-min layers at release time. Non-blocking by construction; drift is the maintainer's to rebaseline at release. Co-authored-by: Diego Rodrigues de Sa e Souza <diego.souza@cdwasolutions.com.br>
…sidebar (split from cheaper-inference#3668) - Plugin admin pages — dynamic route for rendered plugin pages - API route for plugin page rendering with sanitization (DOMPurify + XSS) - API endpoint for plugin UI extension listings - Sidebar integration — Plugins section - API key auth enforcement on render and ui-extensions routes - Tests for plugin UI extensibility API routes
9e85861 to
78f2fbc
Compare
|
Closing stale PR — conflicts with current release branch. Will reopen fresh if needed. |
Description
Second slice of the original PR #3668 split into 3 independent PRs as requested.
Contains the WordPress-style plugin UI extensibility:
Pluginssection that fetches and renders plugin admin pagesCommits (3):
Related
Part of the split from original PR #3668