fix(plugins): Browse registry fails with 'Plugin "registry" not found' (route shadowed by /plugins/:id) (#1936)
### Problem
In **Project Settings → Plugins**, the **Browse registry** panel fails
to load with:
```
Failed to load registry: Plugin "registry" not found
```
The registry listing never appears, so one-click plugin
discovery/install is unusable.
### Root cause
In `createApiRoutes` (`packages/dashboard/src/routes.ts`), the generic
project-scoped route **`GET /plugins/:id`** is registered **before** the
plugin sub-router (`createPluginRouter`) — which owns **`GET
/plugins/registry`** — is mounted (`router.use("/plugins",
createPluginRouter(...))`).
Express matches routes in registration order, so a request to
`/api/plugins/registry` matches `:id` with `id === "registry"`, calls
`pluginStore.getPlugin("registry")`, and throws `Plugin "registry" not
found`. The real registry handler (which builds the curated registry
manifest) is never reached.
### Why not just reorder the mount?
The two handlers are **not** equivalent:
- the inline `GET /plugins/:id` is **project-scoped** via
`getProjectContext(req)`,
- the sub-router uses the **global** plugin store.
Mounting the sub-router first would shadow the project-scoped routes and
change scoping semantics for other endpoints. So reordering is the wrong
fix.
### Fix
Let the reserved static path fall through. In the inline `GET
/plugins/:id` handler, when `id === "registry"`, call `next()` so the
later-mounted sub-router serves the registry listing (it already handles
the `projectId` query param). Minimal and targeted — `"registry"` is the
only static GET path under `/plugins` that collides with the
single-segment `:id` pattern.
```ts
router.get("/plugins/:id", async (req, res, next) => {
if (req.params.id === "registry") { next(); return; } // fall through to the registry sub-route
...
});
```
### Tests
Adds a regression test in `plugin-routes.routes.test.ts` (using the
existing `createApiRoutes` harness that reproduces the real mount order)
asserting `GET /api/plugins/registry`:
- returns **200** with a `plugins` array, and
- **never** calls `pluginStore.getPlugin("registry")` (i.e. is no longer
shadowed by `:id`).
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Fixed the `/plugins/registry` endpoint so it responds correctly
instead of being treated like a plugin ID.
* Improved route handling to ensure the registry page/API is reached
even when generic plugin routes are registered first.
* **Tests**
* Added regression coverage for the `/plugins/registry` route to verify
the correct response and prevent the generic plugin lookup from
intercepting it.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -305,6 +305,42 @@ describe("GET /plugins/:id", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("GET /plugins/registry (route-shadowing regression)", () => {
|
||||
let store: TaskStore;
|
||||
let pluginStore: PluginStore;
|
||||
|
||||
beforeEach(() => {
|
||||
pluginStore = createMockPluginStore();
|
||||
store = createMockTaskStore({
|
||||
getPluginStore: vi.fn().mockReturnValue(pluginStore),
|
||||
});
|
||||
});
|
||||
|
||||
function buildApp() {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use("/api", createApiRoutes(store, {
|
||||
pluginStore,
|
||||
pluginLoader: createMockPluginLoader(),
|
||||
}));
|
||||
return app;
|
||||
}
|
||||
|
||||
// Regression: the generic "GET /plugins/:id" route is registered before the
|
||||
// plugin sub-router that owns "GET /plugins/registry". Without the pass-through
|
||||
// guard, "/api/plugins/registry" matched ":id" (id === "registry"), called
|
||||
// pluginStore.getPlugin("registry") and failed with 'Plugin "registry" not found'.
|
||||
it("serves the registry listing instead of being shadowed by /plugins/:id", async () => {
|
||||
const res = await GET(buildApp(), "/api/plugins/registry");
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toHaveProperty("plugins");
|
||||
expect(Array.isArray(res.body.plugins)).toBe(true);
|
||||
// The ":id" handler must NOT have been consulted for the literal "registry".
|
||||
expect(pluginStore.getPlugin as ReturnType<typeof vi.fn>).not.toHaveBeenCalledWith("registry");
|
||||
});
|
||||
});
|
||||
|
||||
describe("GET /plugins/:id/settings", () => {
|
||||
let store: TaskStore;
|
||||
let pluginStore: PluginStore;
|
||||
|
||||
@@ -3476,7 +3476,17 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout
|
||||
* Get a single plugin by ID.
|
||||
* Query: { projectId?: string }
|
||||
*/
|
||||
router.get("/plugins/:id", async (req: Request, res: Response) => {
|
||||
router.get("/plugins/:id", async (req: Request, res: Response, next: NextFunction) => {
|
||||
// "registry" is a static sub-route (GET /plugins/registry) owned by the
|
||||
// plugin sub-router mounted further below. Because this generic ":id" route
|
||||
// is registered first, Express would otherwise match it for the literal
|
||||
// path "/plugins/registry" (id === "registry") and throw
|
||||
// 'Plugin "registry" not found', shadowing the real registry handler.
|
||||
// Fall through so the mounted sub-router can serve the registry listing.
|
||||
if (req.params.id === "registry") {
|
||||
next();
|
||||
return;
|
||||
}
|
||||
const { store: scopedStore } = await getProjectContext(req);
|
||||
const pluginStore = scopedStore.getPluginStore();
|
||||
const id = req.params.id as string;
|
||||
|
||||
Reference in New Issue
Block a user