chore: Live server restructuring and error handling #6778
Conversation
WalkthroughThis pull request implements extensive refactoring and enhancements across the codebase. Key updates include revised package configurations, a shift to a class-based server architecture, and improved error handling through new utilities and structured factories. Document handling has been modularized with new handlers and re-export files, and several new modules have been introduced to manage Redis connections, graceful shutdown, and environment validation. These changes streamline dependencies, update module resolutions, and reorganize controllers, services, and middleware integrations. Changes
Sequence Diagram(s)sequenceDiagram
participant S as Start Server (start.ts)
participant Server as Server Class
participant MW as Middleware Configurator
participant Routes as Route Setup
participant Ext as Service Initializer
participant Shutdown as ShutdownManager
S->>Server: Instantiate Server
Server->>MW: Configure Middleware
Server->>Routes: Setup Routes & Register Controllers
Server->>Ext: Initialize Services (HocusPocus, Redis, etc.)
Server->>Shutdown: Register with ShutdownManager
Shutdown->>Server: Monitor for termination signals
Server-->>S: Server started successfully
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Palanikannan1437
force-pushed
the
fix/live-server-restructuring
branch
from
0e9d566 to
8009da2
Compare
Palanikannan1437
force-pushed
the
fix/live-server-restructuring
branch
from
8009da2 to
a9f4427
Compare
Palanikannan1437
force-pushed
the
fix/live-server-restructuring
branch
from
781fe5b to
7cbdcd4
Compare
Palanikannan1437
force-pushed
the
fix/live-server-restructuring
branch
from
3ffea6a to
cabd5f4
Compare
Palanikannan1437
force-pushed
the
fix/live-server-restructuring
branch
3 times, most recently
from
bf4c790 to
a695e81
Compare
Palanikannan1437
force-pushed
the
fix/live-server-restructuring
branch
from
a695e81 to
4eef115
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 13
🧹 Nitpick comments (43)
live/src/ce/document-types/index.ts (1)
7-9: Consider adding error handling to initialization function.The initialization function could benefit from try/catch error handling to prevent uncaught exceptions during application startup.
export function initializeCEDocumentHandlers() { - registerProjectPageHandler(); + try { + registerProjectPageHandler(); + console.log("CE document handlers initialized successfully"); + } catch (error) { + console.error("Failed to initialize CE document handlers:", error); + // Depending on your error handling strategy, you might want to: + // - Rethrow the error + // - Log and continue + } }live/src/lib/decorators.ts (1)
8-21: Improve type safety in the decorator definition.The static analysis tool correctly identified that using
Objectas a type is too broad and could lead to type safety issues.export const CatchErrors = (): MethodDecorator => { - return function (target: Object, propertyKey: string | symbol, descriptor: PropertyDescriptor) { + return function (target: Record<string, any>, propertyKey: string | symbol, descriptor: PropertyDescriptor) { const originalMethod = descriptor.value; // Only apply to methods that are not WebSocket handlers const isWebSocketHandler = Reflect.getMetadata("method", target, propertyKey) === "ws"; if (typeof originalMethod === "function" && !isWebSocketHandler) { descriptor.value = asyncHandler(originalMethod); } return descriptor; }; };🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
live/src/core/controller-registry.ts (1)
9-18: Consider making the CONTROLLERS constant immutable.Since this is a registry that should remain constant after initialization, consider making it immutable to prevent accidental modifications.
-export const CONTROLLERS = { +export const CONTROLLERS = Object.freeze({ // Core system controllers (health checks, status endpoints) CORE: [HealthController], // Document management controllers DOCUMENT: [DocumentController], // WebSocket controllers for real-time functionality WEBSOCKET: [CollaborationController], -}; +});live/src/config/server-config.ts (1)
22-27: Avoid unsafe type casting for compression middlewareThe compression middleware is cast to
express.RequestHandlerusingas unknown as, which suggests a type incompatibility. Consider using a more type-safe approach:- app.use( - compression({ - level: env.COMPRESSION_LEVEL, - threshold: env.COMPRESSION_THRESHOLD, - }) as unknown as express.RequestHandler - ); + app.use( + compression({ + level: env.COMPRESSION_LEVEL, + threshold: env.COMPRESSION_THRESHOLD, + }) + );If there's a genuine type compatibility issue, consider using proper type declarations for the compression package.
live/src/core/handlers/document-handlers/index.ts (1)
16-28: Consider removing type assertion in favor of stronger typingThe function uses
documentType as anywhich bypasses TypeScript's type checking. This could lead to runtime errors if invalid document types are provided.- documentType: documentType as any, + documentType: documentType as TDocumentTypes,Additionally, consider adding validation for the document type to ensure it's a valid value before passing it to the handler factory.
live/src/lib/service-container.ts (1)
18-29: Consider adding generic typing for better type safetyThe service container uses
anyfor service types, which bypasses TypeScript's type checking. This could be improved by adding generic typing to provide better type safety.- export class ServiceContainer implements IServiceContainer { - private services: Map<string, any> = new Map(); + export class ServiceContainer<T extends Record<string, any>> implements IServiceContainer<T> { + private services: Map<keyof T, any> = new Map<keyof T, any>(); - register(serviceName: string, service: any): void { - this.services.set(serviceName, service); + register<K extends keyof T>(serviceName: K, service: T[K]): void { + this.services.set(serviceName, service); - get(serviceName: string): any { - if (!this.services.has(serviceName)) { - throw new Error(`Service ${serviceName} not found in container`); - } - return this.services.get(serviceName); + get<K extends keyof T>(serviceName: K): T[K] { + if (!this.services.has(serviceName)) { + throw new Error(`Service ${serviceName} not found in container`); + } + return this.services.get(serviceName) as T[K];This would require updating the interface definition as well, but would provide much stronger type safety.
live/src/lib/controller.utils.ts (1)
11-32: Smart approach to controller registration with dependency injectionThe implementation intelligently creates a single instance with dependencies and then determines whether to register it as a WebSocket or REST controller based on metadata. This avoids creating multiple instances and ensures dependencies are properly injected.
One suggestion for improvement: consider adding error handling for when the controller instantiation fails, as unexpected errors during this process could be hard to debug.
export function registerControllers(router: Router, controllers: any[], dependencies: any[] = []): void { controllers.forEach((Controller) => { + try { // Create the controller instance with dependencies const instance = new Controller(...dependencies); // ... rest of the code + } catch (error) { + console.error(`Failed to register controller ${Controller.name}:`, error); + throw new Error(`Controller registration failed for ${Controller.name}: ${error.message}`); + } }); }live/src/core/extensions/database.ts (1)
67-116: Well-structured store operation with comprehensive validationThe
handleStorefunction includes appropriate checks for required state and document type, with helpful error messages. The overall structure is clean and follows good practices.One issue to note: unlike the
handleFetchfunction which returns the result, thehandleStorefunction doesn't return anything. This seems intentional but might be worth documenting with a comment to make it clear.const handleStore = async ({ context, state, documentName: pageId, requestParameters, }: { context: HocusPocusServerContext; state: Buffer; documentName: TDocumentTypes; requestParameters: URLSearchParams; + // This function doesn't return anything as the store operation is fire-and-forget }) => {Additionally, similar to the
handleFetchfunction, you should also verify the response fromdocumentHandler.store()and handle any potential errors there.live/src/core/hocuspocus-server.ts (1)
35-66: Consider simplifying the token parsing logicThe token parsing logic with try-catch-finally could be simplified for better readability while maintaining the same functionality.
- try { - const parsedToken = JSON.parse(token) as TUserDetails; - userId = parsedToken.id; - cookie = parsedToken.cookie; - } catch (error) { - // If token parsing fails, fallback to request headers - console.error("Token parsing failed, using request headers:", error); - } finally { - // If cookie is still not found, fallback to request headers - if (!cookie) { - cookie = requestHeaders.cookie?.toString(); - } - } + // Try to parse token first + try { + const parsedToken = JSON.parse(token) as TUserDetails; + userId = parsedToken.id; + cookie = parsedToken.cookie; + } catch (error) { + // If token parsing fails, log it + console.error("Token parsing failed, using request headers:", error); + } + + // Fallback to request headers if needed + if (!cookie) { + cookie = requestHeaders.cookie?.toString(); + }live/src/core/lib/authentication.ts (1)
20-20: Remove debugging console logThere's a "caught?" console log that appears to be debugging code and should be removed before production.
- console.log("caught?");live/src/ce/document-types/project-page/project-page-handler.ts (1)
17-20: Consider adding error handling to handler methodsBoth fetch and store methods should include proper error handling to ensure failures are appropriately captured, logged, and reported.
fetch: async ({ pageId, params, context }: DocumentFetchParams) => { const { cookie } = context; - return await fetchPageDescriptionBinary(params, pageId, cookie); + try { + return await fetchPageDescriptionBinary(params, pageId, cookie); + } catch (error) { + handleError(error, { + component: "project-page-handler", + operation: "fetch", + extraContext: { pageId }, + throw: true, + }); + } }, // ... store: async ({ pageId, state, params, context }: DocumentStoreParams) => { const { cookie } = context; - await updatePageDescription(params, pageId, state, cookie); + try { + await updatePageDescription(params, pageId, state, cookie); + } catch (error) { + handleError(error, { + component: "project-page-handler", + operation: "store", + extraContext: { pageId }, + throw: true, + }); + } },Also applies to: 25-28
live/src/ce/lib/fetch-document.ts (1)
15-21: Unify your required field checks for consistency.The code checks
documentTypeandpageIdseparately but omitscookie. If multiple parameters must always be present, consider consolidating these checks or leveraging a validation utility to ensure uniform error handling across all required fields.live/src/core/extensions/index.ts (1)
8-16: Add a safeguard forcreateDatabaseExtensionfailures.While
createDatabaseExtension()presumably handles errors internally, preventing them from escalating, consider wrapping this call in a try/catch if failures here could break the app flow. A small logging or fallback mechanism could help maintain stability.live/src/core/helpers/error-handling/error-validation.ts (1)
1-159: Overall validation framework is clean and extensible.The
Validatorclass offers a rich set of methods for validating various data types, ensuring consistent error handling viahandleError. Below are some considerations:
- Consider referencing an actual
Errorobject (e.g., aValidationError) instead ofnullinhandleError, for more detailed error traces.- Ensure unit tests cover each validation method to protect against regressions.
- Evaluate any overlap with existing validation utilities (such as
validation.ts) to avoid duplication.live/src/core/helpers/error-handling/error-reporting.ts (2)
5-12: Ensure no PII leaks through ErrorContext logs.
SinceErrorContextcan include request data (e.g.,body,params, etc.), it’s prudent to sanitize sensitive fields before logging to prevent inadvertent exposure of PII.
31-45: Consider integrating fatal errors with the shutdown flow.
Re-emitting a fatal error as an uncaught exception is valid, but ensure consistent shutdown behavior by coordinating withShutdownManagerif truly fatal. Otherwise, the process may remain open.live/src/core/controllers/document.controller.ts (2)
76-119: Double-check for potential PII in error responses.
If the request body contains user data, ensure that no sensitive content is serialized into the error response (e.g., logs or JSON output) without redaction.
122-128: Add unit tests for metrics.
Consider adding tests to confirm thatmetrics.conversionsandmetrics.errorsget incremented as expected, especially under various error scenarios.live/src/core/process-manager.ts (1)
71-81: Validate exit code and alignment with logging.
The graceful termination sets an exit code of1on shutdown signals. Validate this aligns with the bigger operational picture (e.g., CI environments or container orchestration) if a clean shutdown is needed.live/src/core/controllers/collaboration.controller.ts (1)
91-95: Clarify metrics usage.
getErrorMetrics()returns the current error count, but you currently rely on it without incrementing errors in the code. After fixing the increment logic, confirm how and where these metrics are consumed, ensuring they are reset or tracked properly to prevent unbounded growth in error counts.live/src/core/services/page.service.ts (2)
11-21: Avoid losing stack trace on error rethrow.When errors occur, you do:
.catch((error) => { throw error?.response?.data; });This can lose the original stack trace, making debugging difficult. Consider throwing the original error object or wrapping it in a custom error type instead:
.catch((error) => { - throw error?.response?.data; + throw new Error(error?.response?.data?.message ?? "Unknown error occurred."); });
48-52: Retain consistent error structure in updateDescription.Here, you throw
throw error;instead oferror?.response?.data;. This approach retains more details about the error, but differs from the other methods. Consider making error handling consistent across your service methods to avoid confusion and ensure uniform error responses.live/src/ce/document-types/project-page/handlers.ts (2)
42-44: Use a domain-specific error instead of a genericError.Throwing
Error("Invalid updatedDescription…")works, but consider a domain-specific or custom error class to help differentiate this validation error from others.
50-55: Validate the transformations before sending the payload.After extracting
contentBinaryEncoded,contentHTML, andcontentJSON, consider basic checks (e.g., ensuring non-empty content) before pushing the data toupdateDescription. This can prevent sending invalid or incomplete payloads.live/src/core/types/document-handler.d.ts (2)
29-29: ReturningPromise<any>can hamper type safety.Where possible, returning a more specific promise type or leveraging generics for the
fetchmethod can help ensure robust type checks and meaningful IntelliSense.
53-57: Consider using a well-defined enumeration or bounded range for handler priority.Relying on raw numeric values for
prioritycan be error-prone. An enum or bounded numeric range can make it clearer which priorities are valid and enhance maintainability.live/src/core/shutdown-manager.ts (2)
60-65: Log additional information for repeated shutdown attempts.Currently, a warning is logged, but it doesn’t provide the new shutdown reason or caller context. Adding more context (e.g., the reason string) to the warning can help with post-mortem analysis.
98-102: Reevaluate the short delay before process exit.A 100ms delay after final cleanup might not be sufficient in some cases, particularly if long-running tasks or asynchronous cleanup is required. Consider using a slightly longer delay or a callback-based completion check before exiting.
live/src/core/lib/redis-manager.ts (3)
15-15: Allow configuring the maximum reconnect attempts.Hardcoding
3might be limiting for different environments. Consider exposingmaxReconnectAttemptsas a configurable parameter or environment variable to accommodate varying network conditions.
35-39: Decide whether to continue or terminate if Redis is crucial.Skipping Redis setup to continue the startup could lead to unclear system states if Redis is intended to be a critical dependency. Consider failing fast with a descriptive error if Redis is essential.
79-88: Carefully handle destructive error scenarios (e.g., WRONGPASS, NOAUTH).While disconnecting on serious errors is reasonable, ensure these branches are tested thoroughly to avoid silent data corruption or repeated disconnection. Logging specific resolution steps for such errors can also be beneficial.
live/package.json (1)
34-36: Validate security and correctness forcookie-parseranddotenv
Ensure these dependencies have been tested for security-sensitive scenarios, such as secure cookie handling and environment variable configuration.live/src/server.ts (2)
16-19: Augment IServiceContainer with documentation
The interface is concise. Adding doc comments for each method clarifies usage patterns to maintain consistency and help future contributors.
104-122: Graceful Redis connection handling
If Redis fails, consider offering a fallback or retry strategy to keep the server partially functional, rather than failing altogether.packages/logger/package.json (1)
14-15: Build, watch, lint, and clean scripts
Consider using cross-platform utilities (likerimraf) instead of rawrm -rffor better OS compatibility.Also applies to: 17-18
live/src/core/helpers/error-handling/error-handler.ts (2)
218-222: Revisit the decision not to terminate on fatal errors.Current design logs a fatal error but continues running. For severely broken states, consider a graceful shutdown or partial service degradation to prevent inconsistent application behavior.
285-287: Remove or utilize this no-op reference.Accessing
error.context;without using or logging it is effectively a no-op and might cause confusion.live/src/core/helpers/error-handling/error-factory.ts (1)
213-264: Consider renamingconvertErrorfor clarity.While the function is effective, a name like
toAppErrorornormalizeErrormight better convey its purpose.live/src/core/helpers/validation.ts (1)
160-165: Avoid overshadowing the nativenameproperty.Overwriting
this.namewith the field name can cause confusion. Consider renaming it tofieldNameor a similar property to distinguish from the base Error’snameproperty.live/src/start.ts (2)
10-34: Robust server startup function with error handling.The
startServerfunction properly initializes the server with appropriate error handling. The approach of continuing to run even when errors occur during startup is a good practice for maintaining service availability.Two suggestions for improvement:
- Consider adding more specific error type detection for common startup issues
- Add validation for critical environment variables before attempting server startup
const startServer = async () => { try { + // Validate critical environment variables before starting + if (!env.LIVE_BASE_PATH) { + throw new Error("LIVE_BASE_PATH environment variable is not set"); + } + // Log server startup details logger.info(`Starting Plane Live server in ${env.NODE_ENV} environment`); // Initialize and start the server const server = await new Server().initialize(); await server.start(); logger.info(`Server running at base path: ${env.LIVE_BASE_PATH}`); } catch (error) { logger.error("Failed to start server:", error); // Create an AppError but DON'T exit handleError(error, { errorType: "internal", component: "startup", operation: "startServer", - extraContext: { environment: env.NODE_ENV } + extraContext: { + environment: env.NODE_ENV, + details: error instanceof Error ? error.message : String(error) + } }); // Continue running even if startup had issues logger.warn("Server encountered errors during startup but will continue running"); } };
37-37: Consider using IIFE for immediate execution.Since this is the entry point of the application, consider using an immediately invoked function expression (IIFE) to avoid potential variable hoisting issues.
- startServer(); + (async () => { + await startServer(); + })();live/src/core/controllers/health.controller.ts (2)
5-16: Basic health check implementation.The health controller provides a simple status check endpoint, which is essential for monitoring. However, it currently only returns static information without performing actual service health verification.
Consider enhancing the health check to verify critical service dependencies:
import { CatchErrors } from "@/lib/decorators"; import { Controller, Get } from "@plane/decorators"; import type { Request, Response } from "express"; + import { RedisManager } from "@/core/services/redis-manager"; @Controller("/health") export class HealthController { @Get("/") @CatchErrors() async healthCheck(_req: Request, res: Response) { + // Check Redis connection health + const redisStatus = await this.checkRedisHealth(); + + const isHealthy = redisStatus === "OK"; + res.status(200).json({ - status: "OK", + status: isHealthy ? "OK" : "DEGRADED", + services: { + redis: redisStatus + }, timestamp: new Date().toISOString(), version: process.env.APP_VERSION || "1.0.0", }); } + + /** + * Checks the health of the Redis connection + * @returns Status string indicating Redis health + */ + private async checkRedisHealth(): Promise<string> { + try { + const redisManager = RedisManager.getInstance(); + const client = await redisManager.getClient(); + + if (!client) { + return "DISCONNECTED"; + } + + const pingResult = await client.ping(); + return pingResult === "PONG" ? "OK" : "DEGRADED"; + } catch (error) { + return "ERROR"; + } + } }
9-9: Remove unnecessary async keyword.The
healthCheckmethod is marked as async but doesn't contain any await expressions. Unless you plan to add asynchronous operations, the async keyword is unnecessary.- async healthCheck(_req: Request, res: Response) { + healthCheck(_req: Request, res: Response) {
🛑 Comments failed to post (13)
live/src/config/server-config.ts (1)
44-57:
⚠️ Potential issueCORS configuration should use a single middleware instance
The current implementation adds a separate CORS middleware instance for each origin, which can lead to unexpected behavior. Instead, configure a single CORS middleware with an array of origins:
- const origins = env.CORS_ALLOWED_ORIGINS?.split(",").map((origin) => origin.trim()) || []; - for (const origin of origins) { - logger.info(`Adding CORS allowed origin: ${origin}`); - app.use( - cors({ - origin, - credentials: true, - methods: ["GET", "POST", "PUT", "DELETE", "OPTIONS"], - allowedHeaders: ["Content-Type", "Authorization", "x-api-key"], - }) - ); - } + const origins = env.CORS_ALLOWED_ORIGINS?.split(",").map((origin) => origin.trim()) || []; + + // Log the allowed origins + origins.forEach(origin => { + logger.info(`Adding CORS allowed origin: ${origin}`); + }); + + app.use( + cors({ + origin: origins.length === 1 && origins[0] === "*" ? "*" : origins, + credentials: true, + methods: ["GET", "POST", "PUT", "DELETE", "OPTIONS"], + allowedHeaders: ["Content-Type", "Authorization", "x-api-key"], + }) + );This approach correctly handles both wildcard and specific origins while maintaining a single middleware instance.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function configureCors(app: express.Application): void { const origins = env.CORS_ALLOWED_ORIGINS?.split(",").map((origin) => origin.trim()) || []; // Log the allowed origins origins.forEach(origin => { logger.info(`Adding CORS allowed origin: ${origin}`); }); app.use( cors({ origin: origins.length === 1 && origins[0] === "*" ? "*" : origins, credentials: true, methods: ["GET", "POST", "PUT", "DELETE", "OPTIONS"], allowedHeaders: ["Content-Type", "Authorization", "x-api-key"], }) ); }live/src/core/extensions/database.ts (1)
14-65: 🛠️ Refactor suggestion
Robust error handling in fetch operation
The
handleFetchfunction implements thorough validation and error handling, including specific error types and detailed context information. The use ofcatchAsyncfor wrapping the operation is excellent.However, there's a potential issue with the error handling flow. After checking if
fetchedDatais null (line 47), you're usinghandleErrorwithout thethrow: trueoption, which means execution will continue even if the document is not found.if (!fetchedData) { handleError(null, { errorType: "not-found", message: `Failed to fetch document: ${pageId}`, component: "database-extension", operation: "fetch", extraContext: { documentType, pageId }, + throw: true, }); }Without the
throw: trueoption, execution will continue and return a null result, which might cause issues downstream.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const handleFetch = async ({ context, documentName: pageId, requestParameters, }: { context: HocusPocusServerContext; documentName: TDocumentTypes; requestParameters: URLSearchParams; }) => { const { documentType } = context; const params = requestParameters; let fetchedData = null; fetchedData = await catchAsync( async () => { if (!documentType) { handleError(null, { errorType: "bad-request", message: "Document type is required", component: "database-extension", operation: "fetch", extraContext: { pageId }, throw: true, }); } const documentHandler = getDocumentHandler(documentType); fetchedData = await documentHandler.fetch({ context: context as HocusPocusServerContext, pageId, params, }); if (!fetchedData) { handleError(null, { errorType: "not-found", message: `Failed to fetch document: ${pageId}`, component: "database-extension", operation: "fetch", extraContext: { documentType, pageId }, throw: true, }); } return fetchedData; }, { params: { pageId, documentType: context.documentType }, extra: { operation: "fetch" }, } )(); return fetchedData; };live/src/core/handlers/document-handlers/handler-factory.ts (1)
18-28: 🛠️ Refactor suggestion
Add null handling for getHandler method
The method could return undefined if no matching handler is found, but it's typed to always return DocumentHandler. This could lead to runtime errors when the returned value is used.
getHandler(context: HandlerContext): DocumentHandler { // Find the first handler whose selector returns true const matchingHandler = this.handlers.find(h => h.selector(context)); // Return the matching handler or fall back to null/undefined // (This will cause an error if no handlers match, which is good for debugging) - return matchingHandler?.handler as DocumentHandler; + if (!matchingHandler) { + throw new Error(`No handler found for context: ${JSON.stringify(context)}`); + } + return matchingHandler.handler; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Get the appropriate handler based on the provided context */ getHandler(context: HandlerContext): DocumentHandler { // Find the first handler whose selector returns true const matchingHandler = this.handlers.find(h => h.selector(context)); // Return the matching handler or throw an error if none is found if (!matchingHandler) { throw new Error(`No handler found for context: ${JSON.stringify(context)}`); } return matchingHandler.handler; }live/src/core/helpers/error-handling/error-reporting.ts (1)
14-29: 💡 Verification agent
🧩 Analysis chain
Verify that error and context are filtered before logging.
InreportError, the entire error and context are logged. Confirm if sensitive data is excluded or masked to comply with best practices and privacy requirements.
🏁 Script executed:
#!/bin/bash # Description: Verify that no sensitive fields (e.g., passwords) are logged in code paths calling `reportError`. rg -A 5 'reportError\('Length of output: 476
Action Required: Sanitize Sensitive Data in Error Logs
The
reportErrorfunction inlive/src/core/helpers/error-handling/error-reporting.ts(Lines 14-29) logs the complete error object and context without any apparent sanitization. Our grep search forreportError(shows that various code paths—in particular inlive/src/core/helpers/error-handling/error-handler.ts—invoke this function without pre-filtering sensitive fields. This poses a risk of unintentionally logging sensitive data (e.g., passwords).Please ensure that before logging:
- Sensitive fields within both the error and context objects are either masked or removed.
- If the logger is configured to handle filtering automatically, document this behavior clearly for future reference.
live/src/core/controllers/document.controller.ts (1)
35-40:
⚠️ Potential issueMissing import for 'crypto'.
The code referencescrypto.randomUUID(), but there is no import statement forcrypto. This causes a runtime error in most environments.+import crypto from "crypto"; ... const requestId = req.id || crypto.randomUUID();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// At the top of the file (if not already present) import crypto from "crypto"; ... // Existing code from lines 35-40 const requestId = req.id || crypto.randomUUID(); const clientInfo = { ip: req.ip, userAgent: req.get("user-agent"), requestId, };live/src/core/controllers/collaboration.controller.ts (2)
17-36: 🛠️ Refactor suggestion
Verify import of
cryptowhen usingcrypto.randomUUID().You're using
crypto.randomUUID()at line 22, but the file does not importcrypto. In most Node environments, you'll need to:import crypto from "crypto";or a similar statement to ensure
crypto.randomUUID()is available.
38-89:
⚠️ Potential issueIncrement
this.metrics.errorson errors.You log a warning if
this.metrics.errors % 10 === 0, but theerrorscounter is never incremented, so it will remain 0. This makes the error threshold logic ineffective. Consider updatinghandleConnectionErrorto incrementthis.metrics.errorsas follows:private handleConnectionError(error: unknown, clientInfo: Record<string, any>, ws: WS) { + this.metrics.errors++; // Convert to AppError if needed const appError = Errors.convertError(…); … }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private handleConnectionError(error: unknown, clientInfo: Record<string, any>, ws: WS) { + this.metrics.errors++; // Convert to AppError if needed const appError = Errors.convertError(error instanceof Error ? error : new Error(String(error)), { context: { ...clientInfo, component: "WebSocketConnection", }, }); // Log at appropriate level based on error category if (appError.category === ErrorCategory.OPERATIONAL) { logger.info(`WebSocket operational error: ${appError.message}`, { error: appError, clientInfo, }); } else { logger.error(`WebSocket error: ${appError.message}`, { error: appError, clientInfo, stack: appError.stack, }); } // Alert if error threshold is reached if (this.metrics.errors % 10 === 0) { logger.warn(`High WebSocket error rate detected: ${this.metrics.errors} total errors`); } // Try to send error to client before closing try { if (ws.readyState === ws.OPEN) { ws.send( JSON.stringify({ type: "error", message: appError.category === ErrorCategory.OPERATIONAL ? appError.message : "Internal server error", }) ); } } catch (sendError) { // Ignore send errors at this point } // Close with informative message if connection is still open if (ws.readyState === ws.OPEN) { ws.close( 1011, appError.category === ErrorCategory.OPERATIONAL ? `Error: ${appError.message}. Reconnect with exponential backoff.` : "Internal server error. Please retry in a few moments." ); } }live/src/ce/document-types/project-page/handlers.ts (2)
18-19: 🛠️ Refactor suggestion
Return or throw?
You return
nullimmediately ifworkspaceSlug,projectId, orcookieis not present. This could unintentionally mask invalid inputs. Consider throwing an error instead to signal that the parameters are missing.
48-49: 🛠️ Refactor suggestion
Alert on missing parameters.
Similar to
fetchPageDescriptionBinary, you silently return when required parameters are absent. Throwing a descriptive error would improve observability.live/src/core/types/document-handler.d.ts (1)
18-19: 🛠️ Refactor suggestion
Avoid using 'any' for the 'state' property.
To improve type safety, consider defining a more specific or generic type for
stateinstead of relying onany. This ensures better clarity and prevents unintended usage.live/src/core/shutdown-manager.ts (1)
78-88: 🛠️ Refactor suggestion
Evaluate the forced termination approach.
Forcing termination if shutdown takes too long can be necessary, but it may inadvertently cause data loss or partial cleanup. Consider extending the timeout or adding a configurable interval to balance safety and responsiveness.
live/src/server.ts (1)
43-64: 🛠️ Refactor suggestion
Comprehensive constructor and middleware setup
The constructor neatly organizes core services, but castingthis.apptoanyforexpressWscan mask type errors. Consider stricter typings if possible.- expressWs(this.app as any); + const wsInstance = expressWs(this.app);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export class Server { private readonly app: Application; private readonly port: number; private hocusPocusServer!: Hocuspocus; private serviceContainer: IServiceContainer; private redisManager: RedisManager; /** * Creates an instance of the server class. * @param port Optional port number, defaults to environment configuration */ constructor(port?: number) { this.app = express(); this.serviceContainer = new ServiceContainer(); this.port = port || serverConfig.port; this.redisManager = RedisManager.getInstance(); // Initialize express-ws after Express setup const wsInstance = expressWs(this.app); configureServerMiddleware(this.app); } }live/src/core/helpers/error-handling/error-handler.ts (1)
224-224: 🛠️ Refactor suggestion
Use explicit function signature instead of
Function.Lint tools warn against using
Functionas a type, as it is too broad and can lead to typing pitfalls. Consider a more specific type signature:- export const asyncHandler = (fn: Function) => { + import { Request, Response, NextFunction } from "express"; + export const asyncHandler = (fn: (req: Request, res: Response, next: NextFunction) => Promise<unknown>) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import { Request, Response, NextFunction } from "express"; export const asyncHandler = (fn: (req: Request, res: Response, next: NextFunction) => Promise<unknown>) => { // function body... }🧰 Tools
🪛 Biome (1.9.4)
[error] 224-224: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
live/src/core/handlers/document-handlers/index.ts (1)
10-28: Avoid usingas anyfor type casting.The type casting on line 22 (
documentType as any) bypasses TypeScript's type checking system. This can lead to runtime errors if the provided string doesn't match a valid document type.- documentType: documentType as any, + documentType: documentType as TDocumentTypes,Consider importing the
TDocumentTypestype and using it for a more type-safe conversion, or implement validation to ensure the string is a valid document type.live/src/ee/document-types/register.ts (1)
3-5: Good initialization pattern.The function provides a clean entry point for document handler initialization. This follows good separation of concerns and allows the EE module to control which handlers are registered.
Consider adding a comment explaining the purpose of this function. Also, this might be a good place to register any future EE-specific document handlers if needed:
export function initializeDocumentHandlers() { + // Register all document handlers needed for the application registerProjectPageHandler(); + // EE-specific handlers can be registered here }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
live/src/ce/document-types/index.ts(1 hunks)live/src/ce/document-types/register.ts(1 hunks)live/src/core/handlers/document-handlers/index.ts(1 hunks)live/src/ee/document-types/index.ts(1 hunks)live/src/ee/document-types/register.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- live/src/ee/document-types/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- live/src/ce/document-types/index.ts
🧰 Additional context used
🧬 Code Definitions (3)
live/src/ce/document-types/register.ts (1)
live/src/ee/document-types/register.ts (1)
initializeDocumentHandlers(3-5)
live/src/ee/document-types/register.ts (1)
live/src/ce/document-types/register.ts (1)
initializeDocumentHandlers(3-5)
live/src/core/handlers/document-handlers/index.ts (4)
live/src/ce/document-types/register.ts (1)
initializeDocumentHandlers(3-5)live/src/ee/document-types/register.ts (1)
initializeDocumentHandlers(3-5)live/src/core/types/document-handler.d.ts (2)
DocumentHandler(25-35)HandlerContext(40-43)live/src/core/handlers/document-handlers/handler-factory.ts (1)
handlerFactory(32-32)
🔇 Additional comments (4)
live/src/ce/document-types/register.ts (1)
1-5: Implementation matches the enterprise edition pattern.The function follows the same pattern as the EE version, providing a clean initialization mechanism for document handlers. This modular approach makes it easy to manage different document types.
live/src/core/handlers/document-handlers/index.ts (2)
1-9: Initialization structure looks good.Importing and immediately calling
initializeDocumentHandlers()ensures that all document handlers are properly registered when this module is loaded.
30-31: Good practice exposing the factory.Exporting the factory directly provides flexibility for advanced use cases while the
getDocumentHandlerfunction covers the common use case.live/src/ee/document-types/register.ts (1)
1-2: Clean import statement.The import correctly references the community edition's project page handler, following the pattern of reusing CE functionality in EE modules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
live/src/lib/decorators.ts (1)
8-9: Use a more specific type instead ofObject.The generic
Objecttype should be replaced with a more specific interface or type to provide better type safety and clarity.- export const CatchErrors = (): MethodDecorator => { - return function (target: Object, propertyKey: string | symbol, descriptor: PropertyDescriptor) { + export const CatchErrors = (): MethodDecorator => { + return function (target: any, propertyKey: string | symbol, descriptor: PropertyDescriptor) {Alternatively, if you need to maintain more type safety, consider using:
export const CatchErrors = (): MethodDecorator => { return function <T>( target: Record<string, any>, propertyKey: string | symbol, descriptor: TypedPropertyDescriptor<T> ): TypedPropertyDescriptor<T> { // implementation }; };🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
live/src/server.ts (4)
36-55: Well-structured Server class with proper initializationThe Server class has a clear responsibility and good separation of concerns. The constructor initializes core components and configures middleware.
One suggestion: Consider separating configuration from construction by moving the middleware setup to the initialize method. This would make the class more testable and follow the principle of separating construction from behavior.
constructor(port?: number) { this.app = express(); this.port = port || serverConfig.port; this.redisManager = RedisManager.getInstance(); // Initialize express-ws after Express setup expressWs(this.app as any); - - configureServerMiddleware(this.app); } async initialize() { try { + // Configure middleware + configureServerMiddleware(this.app); + // Initialize core services await this.initializeServices();
98-104: Good service initialization with proper loggingThe service initialization is well-structured with informative logging. Consider adding more specific error handling for Redis connection failures versus HocusPocus initialization failures.
private async initializeServices() { logger.info("Initializing Redis connection..."); - await this.redisManager.connect(); + try { + await this.redisManager.connect(); + } catch (error) { + handleError(error, { + errorType: "service-unavailable", + component: "redis", + operation: "connect", + throw: true, + }); + } // Initialize the Hocuspocus server - this.hocusPocusServer = await getHocusPocusServer(); + try { + this.hocusPocusServer = await getHocusPocusServer(); + } catch (error) { + handleError(error, { + errorType: "service-unavailable", + component: "hocuspocus", + operation: "initialize", + throw: true, + }); + } }
109-130: Well-structured route setup with good error handlingThe route setup logic is well-organized and the error handling is comprehensive. The use of dependency injection for controllers is a good practice.
Consider adding more specific context to the error handling, such as which controller failed to register.
private async setupRoutes() { try { const router = express.Router() as WebSocketRouter; // Get all controller classes const controllers = getAllControllers(); // Register controllers with our simplified approach // Pass the hocuspocus server as a dependency to the controllers that need it - registerControllers(router, controllers, [this.hocusPocusServer]); + try { + registerControllers(router, controllers, [this.hocusPocusServer]); + } catch (error) { + handleError(error, { + errorType: "internal", + component: "server", + operation: "registerControllers", + extraContext: { controllers: controllers.map(c => c.name) }, + throw: true, + }); + } // Mount the router on the base path this.app.use(serverConfig.basePath, router); } catch (error) { handleError(error, { errorType: "internal", component: "server", operation: "setupRoutes", throw: true, }); } }
136-160: Robust server startup with graceful shutdown handlingThe start method properly initializes the HTTP server and sets up graceful shutdown handling through the ShutdownManager and ProcessManager. The error handling is comprehensive with appropriate context information.
One suggestion: Consider implementing a recovery strategy for certain types of errors rather than always throwing. For example, if the port is already in use, you might try an alternative port.
async start() { try { const server = this.app.listen(this.port, () => { logger.info(`Plane Live server has started at port ${this.port}`); }); // Register servers with ShutdownManager const shutdownManager = ShutdownManager.getInstance(); shutdownManager.register(server, this.hocusPocusServer); // Setup graceful termination via ProcessManager (for signal handling) const processManager = new ProcessManager(this.hocusPocusServer, server); processManager.registerTerminationHandlers(); return server; } catch (error) { + // Try to determine if this is a port-in-use error + if (error instanceof Error && error.message.includes('EADDRINUSE')) { + logger.warn(`Port ${this.port} is already in use, trying alternative port`); + // Try an alternative port (port + 1) + this.port += 1; + return this.start(); + } + handleError(error, { errorType: "service-unavailable", component: "server", operation: "start", extraContext: { port: this.port }, throw: true, }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
live/src/lib/decorators.ts(1 hunks)live/src/server.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
live/src/lib/decorators.ts (1)
live/src/core/helpers/error-handling/error-handler.ts (1)
asyncHandler(224-244)
🪛 Biome (1.9.4)
live/src/lib/decorators.ts
[error] 9-9: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
🔇 Additional comments (9)
live/src/lib/decorators.ts (5)
1-3: Appropriate imports for decorator functionality.The imports correctly include both "reflect-metadata" (necessary for TypeScript decorators with metadata) and the asyncHandler from the error handling module that will be used to wrap controller methods.
4-7: Well-documented decorator purpose.The JSDoc comment clearly explains the decorator's purpose and functionality, making it easier for other developers to understand and use it correctly.
12-14: Good use of metadata for selective application.Using Reflect metadata to check if the method is a WebSocket handler is a clean approach to selectively applying the decorator, ensuring it's only used where appropriate.
8-21: Well-implemented decorator pattern.The decorator correctly wraps non-WebSocket controller methods with error handling functionality. It maintains the original method reference, properly checks conditions before applying the wrapper, and returns the modified descriptor. This pattern effectively centralizes error handling logic across controller methods.
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
15-17:✅ Verification successful
Explicit handling for WebSocket methods.
The code correctly excludes WebSocket handlers from error handling. However, this raises a question about how errors in WebSocket handlers are being managed.
Are WebSocket handlers using a different error handling approach? It would be helpful to verify this by checking:
🏁 Script executed:
#!/bin/bash # Search for WebSocket error handling implementations rg "ws.*error" --type ts live/src # Look for other decorators that might handle WebSocket errors rg "WebSocket.*Error|Error.*WebSocket" --type ts live/srcLength of output: 424
WebSocket Error Handling Verification: Confirmed Implementation
The investigation confirms that WebSocket handlers are managed differently. In
live/src/core/controllers/collaboration.controller.ts, WebSocket errors are explicitly handled using event listeners (e.g.,ws.on("error", (error) => { ... })) with appropriate logging. This distinct approach explains why theasyncHandlerdecorator inlive/src/lib/decorators.tscorrectly excludes WebSocket methods from its error handling.No further changes are required since WebSocket error management is implemented separately.
live/src/server.ts (4)
1-26: Well-organized import structureThe imports are nicely grouped by functionality with appropriate comments. This organization makes the code more maintainable and easier to understand.
29-31: Good type definition for WebSocketRouterThe interface correctly extends the Express Router with WebSocket functionality, providing proper type safety for the WebSocket handler functions.
61-63: Good testing access pointProviding a method to access the Express application is excellent for testing purposes.
69-93: Comprehensive initialization with proper error handlingThe initialize method has a clear flow and good error handling. The use of handleError with specific context information is excellent.
Note that since the error handler has
throw: true, this method will never return normally in case of an error. This is correctly reflected in your implementation since the TypeScript compiler can infer this.
Palanikannan1437
force-pushed
the
fix/live-server-restructuring
branch
from
f168bd3 to
70dbad6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
live/src/lib/decorators.ts (2)
9-9: ReplaceObjectwith a more specific typeUsing
Objectas a type is not recommended as it's too broad. Consider using a more specific type that better represents whattargetshould be.- export const CatchErrors = (): MethodDecorator => { - return function (target: Object, propertyKey: string | symbol, descriptor: PropertyDescriptor) { + export const CatchErrors = (): MethodDecorator => { + return function (target: any, propertyKey: string | symbol, descriptor: PropertyDescriptor) {The
MethodDecoratortype already constrains the parameters, so usinganyhere is acceptable as it maintains TypeScript's built-in type checking for the decorator.🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
12-13: Consider documenting WebSocket error handling approachThe code explicitly excludes WebSocket handlers from this error handling mechanism. It would be helpful to add documentation about how errors in WebSocket handlers are expected to be handled instead.
live/src/server.ts (1)
98-104: Consider adding specific error handling for Redis connection failures.While you have a general error handler in the
initializemethod, adding specific error handling for Redis connection could help with debugging and recovery.private async initializeServices() { logger.info("Initializing Redis connection..."); - await this.redisManager.connect(); + try { + await this.redisManager.connect(); + logger.info("Redis connection established successfully"); + } catch (error) { + logger.error("Failed to connect to Redis:", error); + handleError(error, { + errorType: "service-unavailable", + component: "redis-manager", + operation: "connect", + throw: true, + }); + } // Initialize the Hocuspocus server this.hocusPocusServer = await getHocusPocusServer(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
live/src/lib/decorators.ts(1 hunks)live/src/server.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
live/src/lib/decorators.ts (1)
live/src/core/helpers/error-handling/error-handler.ts (1)
asyncHandler(224-244)
🪛 Biome (1.9.4)
live/src/lib/decorators.ts
[error] 9-9: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
live/src/lib/decorators.ts (2)
8-21: Good implementation of a decorator for centralized error handling!The
CatchErrorsdecorator provides a clean way to apply error handling across controller methods without duplicating try-catch blocks. The implementation correctly uses theasyncHandlerfrom the error handling module and avoids applying it to WebSocket handlers.🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
15-17: Robust error handling implementationThe implementation correctly applies error handling only to actual functions and non-WebSocket handlers. This selective application ensures that the appropriate error handling strategy is used for each type of controller method.
live/src/server.ts (7)
1-26: Good job organizing imports with clear categorization and comments.The imports are well-organized with descriptive comment blocks for different categories (environment, core functionality, Redis, logging, error handling). This improves readability and maintainability.
29-31: LGTM: WebSocketRouter interface definition is clear.The interface correctly extends the Express Router to support WebSocket functionality, with proper typing for the handler function.
36-55: Well-structured class initialization with proper dependency setup.The constructor follows best practices by:
- Initializing private members
- Supporting an optional port parameter with sensible defaults
- Properly initializing Express and middleware
- Using the singleton pattern for RedisManager
The type assertion on line 52 (
this.app as any) is necessary due to the expressWs library's typing limitations.
57-63: Good addition of a getter method for testing purposes.The
getApp()method makes the Express application instance available for testing without exposing it for modification, which is a good practice.
69-92: Robust error handling in the initialization process.The
initialize()method properly structures the server initialization flow and includes comprehensive error handling with detailed error context. The comment on line 85 is helpful in explaining TypeScript's type inference behavior.
109-130: Good implementation of dynamic controller registration.The route setup properly uses the controller registry pattern, which makes it easier to add new controllers without modifying this file. The error handling is also well-implemented with specific context information.
136-160: Excellent implementation of server startup with graceful shutdown handling.The
start()method:
- Properly starts the Express server
- Logs the successful startup
- Registers both HTTP and Hocuspocus servers with the ShutdownManager
- Sets up process termination handlers
- Includes comprehensive error handling
This implementation follows best practices for Node.js server management.
Palanikannan1437
force-pushed
the
fix/live-server-restructuring
branch
from
a11180d to
1315acf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
live/src/core/shutdown-manager.ts (4)
1-7: Consolidate duplicated imports from thehttpmodule.
Currently, the code importsServerfrom"http"under two different aliases (HttpServerandServer). This can cause confusion and is unnecessary. Consider removing one of these aliases or consolidating them into a single import statement.A possible fix could be:
-import { Server as HttpServer } from "http"; import { Hocuspocus } from "@hocuspocus/server"; import { logger } from "@plane/logger"; import { RedisManager } from "@/core/lib/redis-manager"; // config import { serverConfig } from "@/config/server-config"; -import { Server } from "http"; +import { Server as HttpServer } from "http";
85-85: Revisit the arbitrary 1-second delay.
The 1-second delay before completing the shutdown is a simple approach. However, it may still leave cases where certain asynchronous operations have not completed. Consider a more robust mechanism, such as actively checking open connections or a callback to confirm all tasks are finished, eliminating fixed delays.
114-116: Reassess calling bothdisconnect()andquit()on the Redis client.
Callingdisconnect()forcibly ends the connection, which might renderquit()ineffective. Typically,quit()is sufficient to gracefully close a Redis connection and ensure all queued commands are executed first. Using both can cause unexpected behavior.if (redisClient) { - redisClient.disconnect(); await redisClient.quit(); console.info("Redis connections closed successfully"); } else { console.info("No Redis connections to close"); }
71-77: Use a consistent logging approach instead of mixingconsoleandlogger.
The file interspersesconsole.log,console.info,console.error, andloggercalls. Logging consistency improves clarity and makes log management more uniform. Consider standardizing to theloggerutility throughout:Below is an example that replaces
console.log|info|errorwithloggerequivalents:- console.log("All components shut down successfully"); + logger.info("All components shut down successfully"); - console.error("Error during graceful shutdown:", error); + logger.error("Error during graceful shutdown:", error); - console.info("Closing Redis connections..."); + logger.info("Closing Redis connections...");Also applies to: 88-95, 109-123, 129-141, 147-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
live/src/core/extensions/index.ts(1 hunks)live/src/core/extensions/redis.ts(1 hunks)live/src/core/shutdown-manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- live/src/core/extensions/redis.ts
🧰 Additional context used
🧬 Code Definitions (2)
live/src/core/extensions/index.ts (2)
live/src/core/extensions/database.ts (1)
createDatabaseExtension(7-12)live/src/core/extensions/redis.ts (1)
setupRedisExtension(10-25)
live/src/core/shutdown-manager.ts (3)
live/src/server.ts (1)
Server(36-161)live/src/config/server-config.ts (1)
serverConfig(62-66)live/src/core/lib/redis-manager.ts (1)
RedisManager(11-127)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
live/src/core/extensions/index.ts (5)
1-1: Clean import restructuring with improved module organization.The imports have been reorganized to use dedicated modules for Redis and database extensions, which follows the principle of separation of concerns. The switch to
@plane/loggeralso standardizes the logging approach.Also applies to: 4-6
8-8: Function signature explicitly marked as async for better type safety.The explicit
asynckeyword in the function declaration improves type safety and makes the asynchronous nature of the function clear to callers. This change aligns with TypeScript best practices.
13-13: Standardized logging implementation.Replaced the previous logging mechanism with the centralized
loggerfrom@plane/logger, which ensures consistent logging across the application.
16-16: Modularized database extension creation.The database extension setup has been extracted to a dedicated function, improving code organization and maintainability. This approach encapsulates the database configuration logic and makes this file easier to understand.
19-24: Improved Redis extension setup with proper error handling.The Redis extension setup has been modularized and includes proper error handling. The code now:
- Attempts to set up the Redis extension asynchronously
- Only adds the extension if the setup is successful
- Logs confirmation when Redis is properly configured
This approach is more robust than the previous implementation and allows the application to gracefully continue without Redis if needed.
live/src/core/shutdown-manager.ts (1)
10-10: Evaluate thread/process concurrency implications for this global variable.
isGlobalShutdownInProgressis a global flag, which might function reliably under a single-process, single-thread environment. However, if Node’s cluster mode or multiple processes are used, each process will have its own copy of this variable. Confirm whether multi-process deployments might cause inconsistent shutdown handling, especially if only one process sets this flag while others remain unaware.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
live/package.json (3)
12-14: Revamped Server ScriptsThe scripts now leverage
tsupfor both building and watching file changes. Additionally, the"dev"and"start"commands include the--env-file=.envflag, ensuring environment variables are properly loaded at runtime.Action: Please verify that the environment file loading behaves as expected in your deployment; if your runtime environment doesn’t support
--env-filedirectly with Node, you may need to adjust your startup routine.
26-28: New Internal Package DependenciesThe additions of
"@plane/decorators"and"@plane/logger"(each with a version specifier of"*") indicate that the project will always pull the latest releases of these packages.Suggestion: Consider pinning these dependencies to specific versions (or using a semver range) to ensure stability and reproducibility of builds, unless you specifically intend to always use the latest.
54-60: DevDependencies AdjustmentsThe devDependency changes include updating
"@types/cookie-parser"to"^1.4.8"and pinning"tsup"to"8.4.0".Observation: Pinning the version of
tsup(removing the caret) might be intentional to lock down build behavior. However, consider whether you want to allow minor patches and improvements automatically. Ensure this decision aligns with your continuous integration practices and overall version management strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
live/.babelrc(0 hunks)live/package.json(3 hunks)
💤 Files with no reviewable changes (1)
- live/.babelrc
🔇 Additional comments (3)
live/package.json (3)
6-8: Updated Distribution File ReferencesThe
"main","module", and"types"fields have been updated to point to the distribution files in the./distdirectory. This change clearly indicates a shift from the source (src) files to the built outputs and improves compatibility with ES modules and TypeScript consumers.
34-36: Updated Runtime Dependency VersionsThe inclusion of
"cookie-parser": "^1.4.7"and the update of"dotenv"to"^16.4.7"ensure that the server benefits from the latest security patches and features offered by these libraries.Action: It’s advisable to thoroughly test these changes in your live server environment to ensure that middleware and environment configurations work seamlessly after these upgrades.
45-50: Additional Dependency EnhancementsNew dependencies such as
"reflect-metadata": "^0.2.2","yjs": "^13.6.20", and"zod": "^3.24.2"have been added.
"reflect-metadata"facilitates the use of decorators, which is often essential for modern TypeScript projects."yjs"and"zod"are powerful choices for real-time collaboration and schema validation, respectively, aligning well with the broader application improvements.Note: Confirm that these dependencies integrate smoothly with existing modules and that any new initialization or configuration they require is appropriately addressed elsewhere in the codebase.
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Refactor
Chores