fix(server-hono): don't double-prefix basePath when Hono already merged it into route.path#1241
Conversation
…ed it into route.path Hono merges basePath into route.path when a sub-app is mounted via app.route(basePath, subApp) or app.basePath(basePath), but keeps basePath on the route as metadata. extractCustomEndpoints blindly prepended basePath to route.path, so a route served at /api/hello was logged as /api/api/hello. Only prepend basePath when route.path does not already include it. Add two regression tests for the real-Hono shape (reporter case and a nested sub-app). Fixes VoltAgent#1238
🦋 Changeset detectedLatest commit: 85cae43 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughA patch to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server-hono/src/utils/custom-endpoints.ts (1)
47-50: Optional: consider a segment-boundary check forstartsWith.
route.path.startsWith(basePath)is a raw string-prefix match, so a pathological input likebasePath="/api"withroute.path="/apiary"would skip the prepend and yield/apiaryinstead of/api/apiary. In practice this shouldn't occur — when Hono populatesroute.basePath,route.pathis guaranteed to be the merged absolute path with a segment boundary — so this is a defensive nit, not a bug.🔧 Optional hardening
- const basePath = route.basePath && route.basePath !== "/" ? route.basePath : ""; - const rawPath = - basePath && !route.path.startsWith(basePath) - ? `${basePath}${route.path}` - : route.path; + const basePath = route.basePath && route.basePath !== "/" ? route.basePath : ""; + const alreadyPrefixed = + !!basePath && + (route.path === basePath || route.path.startsWith(`${basePath}/`)); + const rawPath = basePath && !alreadyPrefixed ? `${basePath}${route.path}` : route.path;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-hono/src/utils/custom-endpoints.ts` around lines 47 - 50, The startsWith check for basePath is too permissive and can mis-detect segment boundaries (e.g., basePath="/api" vs route.path="/apiary"); update the rawPath logic in custom-endpoints.ts to only treat route.path as already prefixed when it equals basePath or when the next character after basePath is a path separator. Concretely, replace the plain route.path.startsWith(basePath) test with a segment-aware check such as: consider it prefixed only if route.path === basePath or route.path.startsWith(basePath + '/'), and otherwise prepend basePath when computing rawPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/server-hono/src/utils/custom-endpoints.ts`:
- Around line 47-50: The startsWith check for basePath is too permissive and can
mis-detect segment boundaries (e.g., basePath="/api" vs route.path="/apiary");
update the rawPath logic in custom-endpoints.ts to only treat route.path as
already prefixed when it equals basePath or when the next character after
basePath is a path separator. Concretely, replace the plain
route.path.startsWith(basePath) test with a segment-aware check such as:
consider it prefixed only if route.path === basePath or
route.path.startsWith(basePath + '/'), and otherwise prepend basePath when
computing rawPath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 705758cf-6d68-4a2e-8a4f-f661603fb697
📒 Files selected for processing (3)
.changeset/custom-routes-no-double-prefix.mdpackages/server-hono/src/utils/custom-endpoints.spec.tspackages/server-hono/src/utils/custom-endpoints.ts
Fixes #1238.
Root cause
When a sub-app is mounted via
app.route(basePath, subApp)orapp.basePath(basePath), Hono's internal_addRoutecallsmergePath(this._basePath, path)and stores the merged result inroute.path, while still keepingbasePathon the route as metadata:So for the reporter's setup:
the entry that lands in
app.routesis{ method: "GET", path: "/api/hello", basePath: "/api" }.extractCustomEndpointsthen did:Normalized that to
/api/api/hello, which is what shows up in the startup log. The actual request still worked because Hono itself routes byroute.path; only the logging / OpenAPI side was wrong.Fix
Only prepend
basePathwhenroute.pathdoes not already include it:This keeps the existing behavior for the defensive mock case
{ path: "/health", basePath: "/api" }(still becomes/api/health), while producing/api/hellofor the shape Hono actually hands us.Tests
Two regression tests added to
custom-endpoints.spec.ts:should not double-prefix basePath when Hono already merged it into path- modelsapp.route('/api', routes); routes.get('/hello').should handle nested sub-apps without double-prefixing- modelsapp.route('/api', sub); sub.route('/sub', inner); inner.get('/x').Both fail without the fix (producing
/api/api/helloand/api/api/sub/innerrespectively). All 44 existing tests still pass. Full spec: 46/46 green.Summary by cubic
Fixes custom endpoint extraction in
@voltagent/server-honoto avoid double-prefixingbasePathwhen Hono already merged it intoroute.path. Logs and OpenAPI now show correct paths for sub-apps; request routing was unaffected.basePathwhenroute.pathdoesn’t start with it.Written for commit 85cae43. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
/api/api/hellonow logs as/api/hello)Tests