[SILO-454] chore: refactor decorator, logger packages#7618
[SILO-454] chore: refactor decorator, logger packages#7618sriramveeraghanta merged 8 commits intopreviewfrom
Conversation
- add registerControllers function abstracting both rest, ws controllers - update logger to a simple json based logger
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughController registration now accepts multiple controllers and delegates REST vs WebSocket registration to new helpers; the dedicated websocket-controller module was removed. Decorator exports were reshaped. Logger config and middleware moved to an express-winston Console-only setup. Several package.json scripts and devDependency pins were updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant Reg as registerControllers
participant Router as Router
participant RestReg as registerRestController
participant WsReg as registerWebSocketController
App->>Reg: registerControllers(router, controllers[], deps[])
loop each Controller
Reg->>Reg: instantiate Controller(...deps)
Reg->>Reg: scan metadata for methods
alt REST methods present
Reg->>RestReg: register REST routes
RestReg->>Router: router[verb](path, handler)
end
alt WS methods present
Reg->>WsReg: register WS routes
WsReg->>Router: router.ws(path, async handler)
note right of WsReg #f0f4c3: WS handler wrapped in try/catch\non error: log & ws.close(1011,msg)
end
end
sequenceDiagram
autonumber
participant Client as HTTP Client
participant Express as Express App
participant MW as loggerMiddleware (express-winston)
participant Console as Console Transport
Client->>Express: HTTP Request
Express->>MW: Invoke loggerMiddleware
MW->>Console: Log {method, url, status, responseTime}
Express-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the decorator and logger packages to improve maintainability and consolidate functionality. It simplifies the logger configuration to use JSON formatting and consolidates controller registration into a single unified function.
- Simplified logger package from a complex winston configuration to a minimal JSON-based logger using express-winston
- Consolidated REST and WebSocket controller registration into a single
registerControllersfunction - Removed redundant websocket-controller.ts file by moving functionality into controller.ts
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/logger/src/middleware.ts | Replaced custom request logger middleware with express-winston |
| packages/logger/src/config.ts | Simplified logger configuration to JSON format with minimal transports |
| packages/logger/package.json | Updated dependencies to use express-winston instead of winston-daily-rotate-file |
| packages/decorators/src/websocket-controller.ts | Removed file - functionality moved to controller.ts |
| packages/decorators/src/rest.ts | Minor formatting change removing trailing comma |
| packages/decorators/src/index.ts | Removed export of registerWebSocketControllers function |
| packages/decorators/src/controller.ts | Added unified registerControllers function handling both REST and WebSocket controllers |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/logger/package.json:37
- The @types/node dependency was removed but may still be needed for Node.js type definitions in the logger package. Verify that this removal doesn't break type checking.
"tsup": "8.4.0",
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/decorators/src/controller.ts (1)
1-138: Fix missing imports and tighten controller typings in controller.tsThe recent changes introduce WebSocket handling and generic DI arrays but overlook some key type imports and leave broad
any[]signatures. Please address the following:• Add the missing
Requestimport for the WebSocket handler callback
• Replace the looseany[]parameter types with safer alternatives (e.g.unknown[]or a specific constructor array)
• Augment or cast theRoutertype so thatrouter.ws(...)is correctly typed, since Express’sRouterinterface doesn’t include.wsby defaultAffected locations:
– packages/decorators/src/controller.ts
• Line 1–2: import list
• Line 25–29:registerControllerssignature
• Line 116–120:router.wsusageSuggested diff:
--- a/packages/decorators/src/controller.ts +++ b/packages/decorators/src/controller.ts -import { RequestHandler, Router } from "express"; +import { RequestHandler, Router, Request } from "express"; -export function registerControllers( - router: Router, - controllers: any[], - dependencies: any[] = [] -): void { +export function registerControllers( + router: Router, + controllers: Array<ControllerConstructor>, + dependencies: unknown[] = [] +): void {And around the WebSocket registration:
- if ( - typeof handler === "function" && - "ws" in router && - typeof router.ws === "function" - ) { - router.ws(`${baseRoute}${route}`, (ws: WebSocket, req: Request) => { + // Cast to any or extend Router so `.ws` is recognized + const wsRouter = router as unknown as { + ws(path: string, cb: (ws: WebSocket, req: Request) => void): void; + }; + if (typeof handler === "function") { + wsRouter.ws(`${baseRoute}${route}`, (ws: WebSocket, req: Request) => {These changes will restore proper typing for
Request, eliminate unsafeany[], and ensurerouter.wscompiles without errors.
🧹 Nitpick comments (11)
packages/decorators/src/controller.ts (7)
27-29: Tighten types for controllers/dependencies (resolves “Unexpected any” warnings)Use concrete constructor and
unknown[]instead ofany[]. Addresses static analysis warnings at these lines.-export function registerControllers( - router: Router, - controllers: any[], - dependencies: any[] = [] -): void { +export function registerControllers( + router: Router, + controllers: ControllerConstructor[], + dependencies: unknown[] = [] +): void {Optional: make this generic to improve inference for shared DI tuples:
export function registerControllers<D extends unknown[]>( router: Router, controllers: Array<new (...args: D) => ControllerInstance>, dependencies: D = [] as unknown as D ): void { /* ... */ }
105-110: Unify metadata type: cast to HttpMethod, not stringMinor consistency tweak; keeps types aligned across REST/WS paths.
- ) as string; + ) as HttpMethod;
58-60: Guard against missing @controller decorator (baseRoute may be undefined)If a controller lacks the
@Controllerclass decorator,baseRoutemetadata isundefined, leading to paths likeundefined/users. Provide a safe default.- const baseRoute = Reflect.getMetadata("baseRoute", Controller) as string; + const baseRoute = + (Reflect.getMetadata("baseRoute", Controller) as string | undefined) ?? "";Apply in both REST and WS registration helpers.
Also applies to: 100-101
120-133: Optional: replace console.error with pluggable logger to align with “simple JSON-based logger”To keep this package logger-agnostic yet align with the PR’s logging direction, accept an optional logger in dependencies or via a lightweight hook, and fall back to
console.error. Keeps separation of concerns.
61-91: Minor: bind handler once and avoid unknownNot critical, but you can narrow the handler type and bind once for clarity.
- const handler = instance[methodName] as unknown; + const handler = instance[methodName] as unknown as RequestHandler; if (typeof handler === "function") { if (method !== "ws") { ( router[method] as ( path: string, ...handlers: RequestHandler[] ) => void - )(`${baseRoute}${route}`, ...middlewares, handler.bind(instance)); + )(`${baseRoute}${route}`, ...middlewares, handler.bind(instance)); } }
30-42: Note on WS detection heuristicThe
isWebsocketflag is derived from any method tagged"ws". With the earlier change to always register both, this heuristic becomes optional. If you keep it, it can be repurposed for minor optimizations (skip WS registration when no WS methods exist).
25-33: Consider exposing a typed Controller type in public APIRight now
ControllerConstructorandControllerInstanceare internal. Exposing them (or a subset) would let apps type their controller arrays withoutanyescape hatches.I can draft a minimal
types.tsand re-export it for consumers if helpful.packages/logger/src/config.ts (1)
3-11: Capture error stacks and harden the logger for exceptions/rejectionsAdding errors({ stack: true }) preserves stack traces, and dedicated handlers prevent process crashes on unhandled errors while keeping output JSON-only.
Apply:
export const loggerConfig: LoggerOptions = { level: process.env.LOG_LEVEL || "info", format: format.combine( + format.errors({ stack: true }), format.timestamp({ format: "YYYY-MM-DD HH:mm:ss.SSS", }), format.json() ), - transports: [new transports.Console()], + transports: [new transports.Console()], + exceptionHandlers: [new transports.Console()], + rejectionHandlers: [new transports.Console()], + exitOnError: false, };packages/logger/src/middleware.ts (3)
6-7: Drop redundant transports overrideloggerConfig already specifies Console transport; overriding it here adds duplication and future drift.
Apply:
- ...loggerConfig, - transports: [new transports.Console()], + ...loggerConfig,
9-9: Remove expressFormat when emitting JSONexpressFormat is for morgan-style human logs; with format.json() in loggerConfig you’ll get a JSON object with a “message” field set from msg. expressFormat adds no value and can confuse expectations.
Apply:
- expressFormat: true,
5-5: Type the middleware without reintroducing express typesTo avoid any, but also not depend on @types/express, you can expose the inferred type from express-winston without leaking express types in your API surface by exporting a function factory instead of a concrete instance. That defers typing to the app layer.
Example:
-import expressWinston from "express-winston"; +import expressWinston from "express-winston"; -export const loggerMiddleware: any = expressWinston.logger({ +export const createLoggerMiddleware = () => + expressWinston.logger({ ...loggerConfig, msg: "{{req.method}} {{req.url}} {{res.statusCode}} {{res.responseTime}}ms", - expressFormat: true, -}); + meta: false, + statusLevels: true, + });Consumers call createLoggerMiddleware() in their Express app, getting proper types from their own express install.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/decorators/src/controller.ts(4 hunks)packages/decorators/src/index.ts(0 hunks)packages/decorators/src/rest.ts(1 hunks)packages/decorators/src/websocket-controller.ts(0 hunks)packages/logger/package.json(1 hunks)packages/logger/src/config.ts(1 hunks)packages/logger/src/middleware.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/decorators/src/websocket-controller.ts
- packages/decorators/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/logger/src/middleware.ts (1)
packages/logger/src/config.ts (1)
loggerConfig(3-12)
packages/decorators/src/controller.ts (3)
packages/decorators/src/rest.ts (1)
Controller(12-17)packages/decorators/src/index.ts (2)
Controller(2-2)WebSocket(4-4)packages/decorators/src/websocket.ts (1)
WebSocket(8-13)
packages/logger/src/config.ts (1)
apps/live/src/core/helpers/logger.ts (1)
logger(22-37)
🪛 GitHub Check: Build and lint web apps
packages/decorators/src/controller.ts
[warning] 28-28:
Unexpected any. Specify a different type
[warning] 27-27:
Unexpected any. Specify a different type
🔇 Additional comments (8)
packages/decorators/src/rest.ts (1)
24-26: LGTM: non-functional formatting change onlyRemoving the trailing comma in the factory arg list is a no-op; decorators’ behavior and metadata remain intact.
packages/decorators/src/controller.ts (4)
94-102: WS helper mirrors REST helper nicelySeparation of concerns and reuse of baseRoute logic look good; the try/catch around handler invocation is a good safety net for WS handlers.
121-133: WS error handling is pragmaticClosing with 1011 and surfacing the error message is reasonable. Once a pluggable logger is available, route these logs through it.
61-76: Middleware metadata handling looks correctFetching, defaulting to [], and spreading into the route registration matches the REST decorator semantics.
77-89: Route registration binding is correct
handler.bind(instance)ensuresthiscorrectness for controller methods.packages/logger/package.json (1)
31-32: Peer dependency compatibility verified; run security audit before mergingThe peerDependency for express-winston@4.2.0 is
winston >=3.x <4, and your logger’s package.json pins"winston": "^3.17.0", so the versions align correctly.Please perform a fresh install and run a non-dev security audit on the
packages/loggerworkspace to catch any potential vulnerabilities:npm install npm audit --workspace=packages/logger --omit=devpackages/logger/src/config.ts (1)
14-14: LGTM: central logger exportCentralizing createLogger(loggerConfig) is clean and keeps consumers consistent.
packages/logger/src/middleware.ts (1)
8-8: No action needed:{{res.responseTime}}is supported in express-winston v4.xAccording to the official express-winston README, the
msgoption examples include the{{res.responseTime}}mstoken to interpolate the response time when logging HTTP requests. This placeholder is valid in v4.x out of the box, so you can safely leave your template as-is.
- Example from the v4.x README:
msg: "{{res.statusCode}} {{req.method}} {{res.responseTime}}ms {{req.url}}"(github.com)- The same usage is documented in the npm package description for v1.x (which the v4.x README mirrors):
msg: "{{res.statusCode}} {{req.method}} {{res.responseTime}}ms {{req.url}}"(npmjs.com)If you ever need to log
responseTimeat the root level instead of underres, you can alternatively use{{responseTime}}or configuredynamicMeta, but no change is required for the current setup.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
packages/decorators/src/controller.ts (4)
25-29: Improve type safety for controller parameters.The use of
any[]for bothcontrollersanddependenciesparameters removes type safety, as noted in previous reviews.Apply this diff to improve type safety:
export function registerControllers( router: Router, - controllers: ControllerConstructor[], - dependencies: any[] = [] + controllers: ControllerConstructor[], + dependencies: unknown[] = [] ): void {The
controllersparameter is already properly typed, butdependenciesshould useunknown[]instead ofany[]for better type safety.
30-52: Critical logic flaw: Mixed controllers and REST dependency injection.The current implementation has two significant issues:
Mixed controllers aren't fully supported: A controller with both REST and WebSocket methods will only be registered as WebSocket, skipping all REST endpoints.
REST controllers lose dependency injection: REST registration creates a new instance without dependencies, breaking controllers that require constructor arguments.
Apply this diff to fix both issues:
- if (isWebsocket) { - // Register as WebSocket controller - // Pass the existing instance with dependencies to avoid creating a new instance without them - registerWebSocketController(router, Controller, instance); - } else { - // Register as REST controller - doesn't accept an instance parameter - registerRestController(router, Controller); - } + // Register both REST and WebSocket handlers if present, using the same DI-backed instance + registerRestController(router, Controller, instance); + registerWebSocketController(router, Controller, instance);This ensures the same dependency-injected instance is used for both registrations and supports controllers with mixed REST/WebSocket methods.
54-57: Update REST controller to accept existing instance.The
registerRestControllerfunction needs to accept an optional existing instance to support dependency injection properly.Apply this diff to support dependency injection:
function registerRestController( router: Router, - Controller: ControllerConstructor + Controller: ControllerConstructor, + existingInstance?: ControllerInstance ): void { - const instance = new Controller(); + const instance = existingInstance ?? new Controller();
115-121: Fix TypeScript error with Router.ws access.TypeScript will fail because
router.wsis not part of the Express Router type definition. The current dot notation access will cause compilation errors.Apply this diff to fix the type error:
if ( typeof handler === "function" && - "ws" in router && - typeof router.ws === "function" + "ws" in (router as any) && + typeof (router as any).ws === "function" ) { - router.ws(`${baseRoute}${route}`, (ws: WebSocket, req: Request) => { + (router as any).ws(`${baseRoute}${route}`, (ws: WebSocket, req: Request) => {packages/logger/src/middleware.ts (2)
1-1: Good: explicit RequestHandler typing resolves prior feedback.Exporting a concrete Express RequestHandler type is clear and helps consumers.
6-11: Disable meta logging to avoid leaking PII/secrets; add sane levels and ignore health.With express-winston, meta is true by default and can include headers (Authorization/Cookie), bodies, and other sensitive fields. For lean JSON lines, explicitly disable meta and map status codes to log levels; also consider ignoring health probes.
Apply this diff:
export const loggerMiddleware: RequestHandler = expressWinston.logger({ ...loggerConfig, - transports: [new transports.Console()], - msg: "{{req.method}} {{req.url}} {{res.statusCode}} {{res.responseTime}}ms", - expressFormat: true, + msg: "{{req.method}} {{req.url}} {{res.statusCode}} {{res.responseTime}}ms", + meta: false, + statusLevels: true, // 5xx=error, 4xx=warn, else=info + ignoreRoute: (req) => req.url === "/health" || req.url === "/ready", });If you need selected metadata later, flip
meta: trueand add strictrequestFilter/responseFilterto strip bodies and headers; I can draft that variant.
🧹 Nitpick comments (3)
packages/logger/package.json (1)
30-33: Declare Express as a peer dependency for clearer integration contracts.This package ships Express middleware. Depending apps should provide their own Express version to avoid duplicate instances/mismatches. Add a peerDependency instead of bundling Express here.
Apply this diff:
"dependencies": { "express-winston": "^4.2.0", "winston": "^3.17.0" }, + "peerDependencies": { + "express": "^4.17.0" + },packages/logger/src/middleware.ts (2)
9-10: Avoid mixingmsgandexpressFormat; pick one to prevent confusion.Setting both is redundant;
msgoverrides the morgan-style message. Use only the custommsg(recommended for stable JSON).Apply this minimal diff if you prefer morgan-style instead:
- msg: "{{req.method}} {{req.url}} {{res.statusCode}} {{res.responseTime}}ms", - expressFormat: true, + expressFormat: true,Or keep
msgand dropexpressFormatas shown in the earlier diff.
3-3: Remove unused import after dropping explicit transports override.Once transports are sourced exclusively from
loggerConfig, this import becomes unused.-import { transports } from "winston";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/decorators/package.json(1 hunks)packages/decorators/src/controller.ts(4 hunks)packages/decorators/tsconfig.json(0 hunks)packages/logger/package.json(1 hunks)packages/logger/src/middleware.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/decorators/tsconfig.json
🧰 Additional context used
🧬 Code graph analysis (2)
packages/logger/src/middleware.ts (1)
packages/logger/src/config.ts (1)
loggerConfig(3-12)
packages/decorators/src/controller.ts (3)
packages/decorators/src/rest.ts (1)
Controller(12-17)packages/decorators/src/index.ts (2)
Controller(2-2)WebSocket(4-4)packages/decorators/src/websocket.ts (1)
WebSocket(8-13)
🪛 GitHub Check: Build and lint web apps
packages/decorators/src/controller.ts
[warning] 28-28:
Unexpected any. Specify a different type
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
packages/decorators/package.json (2)
14-15: Simplified build configuration looks good.The updated build and dev scripts are cleaner and leverage tsup's default configuration. The removal of explicit
--externalflags suggests dependencies are now handled differently (likely through the tsup config or peer dependencies).
29-29: Good move to devDependencies.Moving
reflect-metadatato devDependencies is appropriate since it's only needed at build time for metadata generation, not at runtime.packages/decorators/src/controller.ts (2)
1-2: Good addition of missing type imports.The
RequestandWebSockettype imports are now properly included, resolving the TypeScript compilation issues flagged in previous reviews.
121-133: Good WebSocket error handling implementation.The error handling wrapper for WebSocket handlers is well-implemented with proper error logging and graceful connection closure using the appropriate WebSocket close codes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/utils/package.json (1)
39-39: Centralize@types/lodashpin and verify types locally
🔍 Confirmed a single version (
4.17.20) of@types/lodashis installed across the monorepo (pnpm-lock.yaml) [run-scripts output].
⚠️ Automated type-check script (check:types) isn’t defined in several workspaces, so please run your local type checks after installing dependencies:pnpm install pnpm run check:types # or your project’s equivalent type-check command💡 To avoid having to bump each package’s
@types/lodashentry, add a root-level override/resolution inpackage.json:For npm:
{ "overrides": { "@types/lodash": "4.17.20" } }For pnpm:
{ "pnpm": { "overrides": { "@types/lodash": "4.17.20" } } }For Yarn (Berry):
{ "resolutions": { "@types/lodash": "4.17.20" } }packages/i18n/package.json (1)
28-28: Good pin; matches lodash v4 and reduces unexpected DT drift.No functional impact at runtime; only TS types. Keep this consistent across the repo or consider a root-level override as noted in utils.
apps/web/package.json (1)
75-75: Pin looks good; keep @types scoped to devDependencies (as done here).This avoids shipping type packages to prod images. No additional changes needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/admin/package.json(1 hunks)apps/space/package.json(1 hunks)apps/web/package.json(1 hunks)packages/i18n/package.json(1 hunks)packages/ui/package.json(1 hunks)packages/utils/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/space/package.json
- apps/admin/package.json
🔇 Additional comments (1)
packages/ui/package.json (1)
69-69: LGTM on pinning @types/lodash; aligns with other workspaces.No concerns. Ensure Storybook type checks still pass with TS 5.8.3 (covered by the verification script suggested in utils).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/decorators/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/decorators/package.json (2)
16-16: Lint budget raised to 1 warning — confirm intentAllowing warnings can hide regressions. If unintentional, revert to 0.
- "check:lint": "eslint . --max-warnings 1", + "check:lint": "eslint . --max-warnings 0",
14-16: Install dependencies and verify build artifacts
The build failed with “tsc: not found,” so ensure workspace dependencies are installed (e.g. runnpm installor your monorepo install command at the repo root) before rerunning the build. Then in packages/decorators, run:npm run build ls dist # confirm dist/index.js, dist/index.mjs, and dist/index.d.ts exist
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/decorators/src/controller.ts (4)
18-21: Tighten types for dependencies (avoid any[]); consider generics to bind DI args to constructors.Current any[] loses type-safety. Minimal: switch to unknown[]. Better: generic D extends unknown[] across the API.
Minimal change:
- controllers: ControllerConstructor[], - dependencies: any[] = [] + controllers: ControllerConstructor[], + dependencies: unknown[] = []Stronger generic alternative (optional):
-type ControllerConstructor { - new (...args: unknown[]): ControllerInstance; - prototype: ControllerInstance; -} +type ControllerConstructor<D extends unknown[] = unknown[]> = + (new (...args: D) => ControllerInstance) & { prototype: ControllerInstance }; @@ -export function registerControllers( - router: Router, - controllers: ControllerConstructor[], - dependencies: any[] = [] -): void { +export function registerControllers<D extends unknown[]>( + router: Router, + controllers: ControllerConstructor<D>[], + dependencies: D = [] as unknown as D +): void {If you take the generic route, update helper signatures similarly (Controller: ControllerConstructor).
22-41: Don’t drop REST when WS is present; reuse the same DI-backed instance for both.Mixed controllers (having both REST and WS methods) currently register only WS; REST is skipped. Also, REST re-instantiates without DI. Always register both using the same instance.
- // Determine if it's a WebSocket controller or REST controller by checking - // if it has any methods with the "ws" method metadata - const isWebsocket = Object.getOwnPropertyNames(Controller.prototype).some((methodName) => { - if (methodName === "constructor") return false; - return Reflect.getMetadata("method", instance, methodName) === "ws"; - }); - - if (isWebsocket) { - // Register as WebSocket controller - // Pass the existing instance with dependencies to avoid creating a new instance without them - registerWebSocketController(router, Controller, instance); - } else { - // Register as REST controller - doesn't accept an instance parameter - registerRestController(router, Controller); - } + // Register both REST and WS handlers (if present) using the same DI-backed instance. + registerRestController(router, Controller, instance); + registerWebSocketController(router, Controller, instance);
44-46: Propagate DI instance into REST; guard against missing baseRoute.REST path ignores DI and can produce “undefined/…” routes if the class lacks @controller. Accept an existing instance and default baseRoute.
-function registerRestController(router: Router, Controller: ControllerConstructor): void { - const instance = new Controller(); - const baseRoute = Reflect.getMetadata("baseRoute", Controller) as string; +function registerRestController( + router: Router, + Controller: ControllerConstructor, + existingInstance?: ControllerInstance +): void { + const instance = existingInstance ?? new Controller(); + const baseRoute = (Reflect.getMetadata("baseRoute", Controller) as string) || "";
88-97: Fix TS type error: Router.ws isn’t on Express’ Router; also avoid leaking error details to clients.Accessing router.ws fails type-checking; narrow/cast before use. Return a generic WS close reason and log details server-side only.
- if (typeof handler === "function" && "ws" in router && typeof router.ws === "function") { - router.ws(`${baseRoute}${route}`, (ws: WebSocket, req: Request) => { - try { - handler.call(instance, ws, req); - } catch (error) { - console.error(`WebSocket error in ${Controller.name}.${methodName}`, error); - ws.close(1011, error instanceof Error ? error.message : "Internal server error"); - } - }); - } + if (typeof handler === "function") { + const wsRouter = router as Router & { + ws?: (path: string, handler: (ws: WebSocket, req: Request) => void) => void; + }; + if (typeof wsRouter.ws === "function") { + wsRouter.ws(`${baseRoute}${route}`, (ws: WebSocket, req: Request) => { + try { + handler.call(instance, ws, req); + } catch (error) { + console.error(`WebSocket error in ${Controller.name}.${methodName}`, error); + ws.close(1011, "Internal server error"); + } + }); + } + }
🧹 Nitpick comments (1)
packages/decorators/src/controller.ts (1)
71-100: WebSocket wiring is solid with error handling; consider pluggable logger.Runtime try/catch and 1011 close are good. Optionally inject a logger or use your new JSON logger to keep consistency across packages.
I can wire this to your JSON logger (or accept a logger param with a default) if you want a commit-ready patch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/decorators/.prettierrc(1 hunks)packages/decorators/README.md(1 hunks)packages/decorators/src/controller.ts(2 hunks)packages/decorators/src/rest.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/decorators/.prettierrc
- packages/decorators/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/decorators/src/rest.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/decorators/src/controller.ts (2)
packages/decorators/src/rest.ts (1)
Controller(12-17)packages/decorators/src/websocket.ts (1)
WebSocket(8-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (1)
packages/decorators/src/controller.ts (1)
51-64: REST wiring looks correct.Metadata usage, middleware application, and handler binding are fine.
Description
Type of Change
Test Scenarios
References
SILO-454
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores