chore(server): add route parameter types for @types/express-serve-static-core 5.1.1 compatibility#685
Conversation
…tic-core 5.1.1 compatibility
WalkthroughAdds TypeScript interfaces for Express route parameters across the server and updates handler signatures to use Request. Most edits are type-only; one middleware removed a non-null assertion on decrypt, and one route removed runtime parameter validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/server/src/routes/api/proxy.ts`:
- Around line 213-218: Replace the throw with an Express-style error
propagation: when encryptedAuthAndData is missing, call next(new
APIError(constants.ErrorCode.BAD_REQUEST, undefined, 'EncryptedAuthAndData is
required')) and return immediately (do not throw), so the route handler (in
packages/server/src/routes/api/proxy.ts where encryptedAuthAndData is validated)
yields a 400; alternatively, if you prefer keeping throws, update the
surrounding catch to detect APIError and re-throw or call next(err) for APIError
instances instead.
In `@packages/server/src/routes/builtins/knaben.ts`:
- Around line 39-45: The validation that currently does "throw new
APIError(...)" must be handled by Express error middleware; either move the
type/id validation inside the existing try block so thrown APIError is caught
and passed to next(error), or replace the direct throw with "return next(new
APIError(constants.ErrorCode.BAD_REQUEST, undefined, 'Type and id are
required'))" so the handler uses the same next(error) path (reference symbols:
APIError, constants.ErrorCode.BAD_REQUEST, and next) and ensure you return after
calling next to avoid further execution.
In `@packages/server/src/routes/builtins/torrent-galaxy.ts`:
- Around line 7-13: The file imports createLogger and declares const logger =
createLogger('server') but never uses logger; remove the unused import
(createLogger) from the import list and delete the declaration (const logger =
createLogger('server')) so there are no unused symbols left, or alternatively
replace their removal by using logger in error/log statements if logging was
intended — target the createLogger import and the logger declaration to
implement the fix.
🧹 Nitpick comments (7)
packages/server/src/routes/stremio/meta.ts (1)
35-42: Minor: Inconsistent capitalisation in error message."Type" is capitalised whilst "id" is lowercase. Consider consistent casing for a cleaner user-facing message.
✏️ Proposed fix
if (!type || !id) { res.status(200).json({ meta: StremioTransformer.createErrorMeta({ - errorDescription: 'Missing URL Parameters: Type and id are required', + errorDescription: 'Missing URL parameters: type and id are required', }), }); return; }packages/server/src/routes/builtins/gdrive.ts (1)
40-46: Consider extracting duplicated validation logic into a helper.The validation logic for required parameters is identical across the meta, catalog, and stream routes. Extracting this into a reusable helper would reduce duplication and centralise the error message.
♻️ Suggested helper function
function assertRequiredParams( params: { encodedConfig?: string; type?: string; id?: string } ): asserts params is { encodedConfig: string; type: string; id: string } { if (!params.encodedConfig || !params.type || !params.id) { throw new APIError( constants.ErrorCode.BAD_REQUEST, undefined, 'EncodedConfig, type, and id are required' ); } }Then in each handler:
const { encodedConfig, type, id } = req.params; assertRequiredParams(req.params); // TypeScript now knows encodedConfig, type, and id are definedAlso applies to: 72-78, 103-109
packages/server/src/routes/builtins/easynews.ts (1)
94-99: Avoid shadowingfilenameto reduce confusion later.The request param
filenameis shadowed by the NZB API result. A rename prevents accidental misuse in future edits.Proposed tweak
- const { - encodedAuth, - encodedParams, - aiostreamsAuth: encodedAiostreamsAuth, - filename, - } = req.params; + const { + encodedAuth, + encodedParams, + aiostreamsAuth: encodedAiostreamsAuth, + filename, + } = req.params; @@ - const { content, filename } = await api.fetchNzb(nzbParams); + const { content, filename: resolvedFilename } = await api.fetchNzb(nzbParams); @@ - `attachment; filename="${encodeURIComponent(filename)}"` + `attachment; filename="${encodeURIComponent(resolvedFilename)}"`Also applies to: 196-229
packages/server/src/routes/builtins/torrent-galaxy.ts (1)
48-54: Inconsistent error handling pattern.The validation
throwis outside thetry-catchblock, relying on Express 5's automatic async error forwarding. Whilst this works in Express 5.x, it's inconsistent with the explicitnext(error)pattern used in the same handler (lines 67-68) and reduces clarity.Consider moving the validation inside the
tryblock or usingreturn next(...)for consistency:♻️ Option 1: Move validation inside try block
async (req: Request<TorrentGalaxyStreamParams>, res: Response, next: NextFunction) => { const { encodedConfig, type, id } = req.params; - if (!type || !id) { - throw new APIError( - constants.ErrorCode.BAD_REQUEST, - undefined, - 'Type and id are required' - ); - } try { + if (!type || !id) { + throw new APIError( + constants.ErrorCode.BAD_REQUEST, + undefined, + 'Type and id are required' + ); + } const addon = new TorrentGalaxyAddon(♻️ Option 2: Use return next() pattern
async (req: Request<TorrentGalaxyStreamParams>, res: Response, next: NextFunction) => { const { encodedConfig, type, id } = req.params; if (!type || !id) { - throw new APIError( + return next(new APIError( constants.ErrorCode.BAD_REQUEST, undefined, 'Type and id are required' - ); + )); }packages/server/src/routes/builtins/knaben.ts (2)
7-9: Consider defining the parameter as required with explicit type.The route pattern
/:encodedConfig/manifest.jsonguarantees thatencodedConfigwill always be present when this handler is invoked. Marking it as optional (?) doesn't align with the route definition. If the goal is to handle thestring | string[]change from@types/express-serve-static-core5.1.1, consider explicitly typing it asstringinstead:interface KnabenManifestParams { encodedConfig: string; }This makes the contract clearer and matches the actual route behaviour.
29-33: Same observation as above: consider explicitstringtypes.For the route pattern
/:encodedConfig/stream/:type/:id.json, all three parameters are required path segments and will always be present as strings when the route matches. The optional markers force unnecessary runtime validation for parameters that Express guarantees will be present.interface KnabenStreamParams { encodedConfig: string; type: string; id: string; }packages/server/src/routes/builtins/torznab.ts (1)
30-63: Consider extracting a tiny shared validator fortype/id.The new BAD_REQUEST check is good; a small helper could reduce copy/paste across built-in routes.
|
thx, will review this @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/routes/builtins/easynews.ts (1)
89-94:⚠️ Potential issue | 🔴 CriticalAvoid
filenameredeclaration in the NZB handler.
Line 93 introducesfilename, which is redeclared at Line 185, causing a TypeScript compile error.🛠️ Suggested fix (rename bindings and pick a single download name)
const { encodedAuth, encodedParams, aiostreamsAuth: encodedAiostreamsAuth, - filename, + filename: routeFilename, } = req.params as EasynewsNzbParams; - const { content, filename } = await api.fetchNzb(nzbParams); + const { content, filename: nzbFilename } = await api.fetchNzb(nzbParams); res.setHeader('Content-Type', 'application/x-nzb'); + const downloadName = routeFilename || nzbFilename; res.setHeader( 'Content-Disposition', - `attachment; filename="${encodeURIComponent(filename)}"` + `attachment; filename="${encodeURIComponent(downloadName)}"` );Also applies to: 184-186, 214-217
🤖 Fix all issues with AI agents
In `@packages/server/src/routes/builtins/torbox-search.ts`:
- Around line 15-22: The route parameter syntax for the stream endpoint must
match the types: update the stream route path or the TorboxStreamParams type so
encodedConfig is consistently optional or required; specifically, either change
the stream route path string used where the router defines
'/:encodedConfig/stream/:type/:id.json' to use the optional-segment syntax
'{/:encodedConfig}/stream/:type/:id.json' to match TorboxStreamParams
(encodedConfig?: string), or change the TorboxStreamParams interface to
encodedConfig: string and adjust any downstream logic; locate the
router.get(...) call for the stream endpoint and the
TorboxStreamParams/TorboxManifestParams interfaces to make the corresponding
change.
🧹 Nitpick comments (2)
packages/server/src/routes/api/proxy.ts (1)
234-234: Redundant type assertion after interface introduction.With
ProxyParamsinterface now in place, the explicit castas string | undefinedis unnecessary—TypeScript already infersfilenameasstring | undefinedfrom the interface.Suggested cleanup
- const filename = req.params.filename as string | undefined; + const filename = req.params.filename;packages/server/src/routes/stremio/catalog.ts (1)
15-23: Type definitions are correct; consider addingNextFunctiontype fornextparameter.The
CatalogParamsinterface correctly models the route. However, for consistency with other routes (e.g.,subtitle.ts), consider explicitly typing thenextparameter.Suggested improvement
-import { Router, Request, Response } from 'express'; +import { Router, Request, Response, NextFunction } from 'express';- async (req: Request<CatalogParams>, res: Response<CatalogResponse>, next) => { + async (req: Request<CatalogParams>, res: Response<CatalogResponse>, next: NextFunction) => {
| interface TorboxManifestParams { | ||
| encodedConfig?: string; // optional | ||
| } | ||
|
|
||
| router.get( | ||
| '{/:encodedConfig}/manifest.json', | ||
| async (req: Request, res: Response, next: NextFunction) => { | ||
| async (req: Request<TorboxManifestParams>, res: Response, next: NextFunction) => { | ||
| const { encodedConfig } = req.params; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/server/src/routes/builtins/torbox-search.tsRepository: Viren070/AIOStreams
Length of output: 3103
🌐 Web query:
Express 5 optional route parameters syntax :param? documentation
💡 Result:
In Express 5, the old “optional param” syntax :param? is not supported in route strings anymore. Express 5 follows the newer path-to-regexp syntax where optional parts are written with braces. [1], [2], [3]
Use this in Express 5 (optional segment with braces)
// one optional segment
app.get("/users{/:id}", handler); // matches /users and /users/123
// multiple optional params
app.get("/articles{/:year}{/:month}{/:day}", handler); // matches /articles, /articles/2026, /articles/2026/02, etc.
// optional “extension” example (Express migration guide)
app.get("/:file{.:ext}", handler); // matches /report and /report.pdfDocs: Express 5 migration note (“? is no longer supported, use braces instead”) [2] and path-to-regexp “Optional” section [3].
Sources:
[1] Express routing guide (Express 5 note about ? handling) (expressjs.com)
[2] Express “Migrating to Express 5” (expressjs.com)
[3] path-to-regexp docs (optional braces syntax) (github.com)
Citations:
- 1: https://expressjs.com/en/guide/routing.html?utm_source=openai
- 2: https://expressjs.com/en/guide/migrating-5.html?utm_source=openai
- 3: https://github.com/pillarjs/path-to-regexp?utm_source=openai
Fix Route 2 param/type mismatch—Route 1 is correct.
Route 1 (manifest endpoint) correctly uses Express 5 optional segment syntax with braces: {/:encodedConfig}, which aligns properly with the optional type. However, Route 2 (stream endpoint) at line 57 uses the required pattern /:encodedConfig/stream/:type/:id.json without braces, creating a mismatch: the route expects the segment to always be present, but the TorboxStreamParams interface and code logic treat encodedConfig as optional. Either wrap it in braces (/{:encodedConfig}/stream/...) or make the interface field required (encodedConfig: string).
🤖 Prompt for AI Agents
In `@packages/server/src/routes/builtins/torbox-search.ts` around lines 15 - 22,
The route parameter syntax for the stream endpoint must match the types: update
the stream route path or the TorboxStreamParams type so encodedConfig is
consistently optional or required; specifically, either change the stream route
path string used where the router defines
'/:encodedConfig/stream/:type/:id.json' to use the optional-segment syntax
'{/:encodedConfig}/stream/:type/:id.json' to match TorboxStreamParams
(encodedConfig?: string), or change the TorboxStreamParams interface to
encodedConfig: string and adjust any downstream logic; locate the
router.get(...) call for the stream endpoint and the
TorboxStreamParams/TorboxManifestParams interfaces to make the corresponding
change.
1 participant
Fixes ts errors caused by
@types/express-serve-static-corefixing the definition ofParamsDictionaryfromstringtostring | string[]in version 5.1.1. This matches how expressjs actually handles routes (e.g. wildcard routes).Should be backwards compatible, let me know if you want me to remove the updated
pnpm-lock.yamlfrom the PR (only has the@types/express-serve-static-coreversion bump).Summary by CodeRabbit
Refactor
Bug Fixes