refactor: live server #7720
refactor: live server #7720
Conversation
|
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. WalkthroughRefactors live app startup into a lifecycle-driven Server with a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Proc as Process
participant Start as start.ts
participant Server as Server
participant Redis as RedisManager
participant HPMgr as HocusPocusServerManager
participant Ext as getExtensions
participant Router as Express Router
participant Ctrl as Controllers
Proc->>Start: spawn
Start->>Server: new Server()
Start->>Server: initialize()
Server->>Redis: initialize()
Redis-->>Server: ready or disabled
Server->>HPMgr: getInstance().initialize()
HPMgr->>Ext: getExtensions()
Ext-->>HPMgr: [Logger, Database, Redis]
HPMgr-->>Server: hocuspocus server
Server->>Router: registerControllers(CONTROLLERS)
Server-->>Start: initialized
Start->>Server: listen()
sequenceDiagram
autonumber
participant Client
participant WSRoute as /collaboration (WS)
participant Collab as CollaborationController
participant HP as Hocuspocus
participant Auth as onAuthenticate
participant UserAPI as UserService
Client->>WSRoute: WebSocket connect (token/headers/params)
WSRoute->>Collab: handleConnection(ws, req)
Collab->>HP: hocuspocus.handleConnection(ws, req)
HP->>Auth: onAuthenticate(token/headers/params)
Auth->>UserAPI: currentUser(cookie)
UserAPI-->>Auth: user
Auth-->>HP: auth result (allow or reject)
HP-->>Client: accept or close(1011)
sequenceDiagram
autonumber
participant Client
participant HTTP as /convert-document
participant Ctrl as ConvertDocumentController
participant Util as convertHTMLDocumentToAllFormats
Client->>HTTP: POST { description_html, variant }
HTTP->>Ctrl: handleConvertDocument(req)
Ctrl->>Util: convertHTMLDocumentToAllFormats(...)
Util-->>Ctrl: { description, description_html, description_binary }
Ctrl-->>HTTP: 200 JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the live server code into a more standardized architecture by migrating from a legacy file structure to a modern controller-based pattern using decorators, implementing proper singleton patterns for server management, and centralizing Redis and HocusPocus configuration.
- Migrated from direct server instantiation to a controller-based architecture using the
@plane/decoratorspackage - Refactored HocusPocus server setup into a singleton manager pattern with proper initialization lifecycle
- Centralized Redis connection management with a dedicated RedisManager singleton class
Reviewed Changes
Copilot reviewed 28 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/live/src/start.ts | New entry point with proper server initialization and graceful shutdown handling |
| apps/live/src/server.ts | Refactored server class to use controller registration and singleton managers |
| apps/live/src/hocuspocus.ts | New HocusPocus server manager implementing singleton pattern |
| apps/live/src/redis.ts | New Redis connection manager with comprehensive connection handling |
| apps/live/src/controllers/* | New controller classes implementing the decorator pattern for routes |
| apps/live/src/utils/document.ts | Migrated document conversion utilities from core helpers |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/live/src/services/page.service.ts (1)
23-31: Return the correct binary type for fetchDescriptionBinaryResponse is an arraybuffer; tighten the return type.
- async fetchDescriptionBinary(workspaceSlug: string, projectId: string, pageId: string, cookie: string): Promise<any> { + async fetchDescriptionBinary(workspaceSlug: string, projectId: string, pageId: string, cookie: string): Promise<ArrayBuffer> { @@ - .then((response) => response?.data) + .then((response) => response?.data as ArrayBuffer)
🧹 Nitpick comments (22)
apps/live/src/utils/document.ts (6)
6-11: Avoid deep imports from @plane/editor/lib; prefer stable top-level exports.
Deep paths are brittle across package refactors. If possible, re-export these from @plane/editor and import from the root.-import { CoreEditorExtensionsWithoutProps, DocumentEditorExtensionsWithoutProps } from "@plane/editor/lib"; +import { CoreEditorExtensionsWithoutProps, DocumentEditorExtensionsWithoutProps } from "@plane/editor";Also applies to: 15-15
25-51: Good extraction; add minimal runtime validation for external inputs.
Variant is a union at type level, but HTTP bodies are untyped at runtime. Guard early and normalize input.-export const convertHTMLDocumentToAllFormats = (args: TArgs): TDocumentPayload => { - const { document_html, variant } = args; +export const convertHTMLDocumentToAllFormats = (args: TArgs): TDocumentPayload => { + const { document_html, variant } = args ?? ({} as TArgs); + if (variant !== "rich" && variant !== "document") { + throw new Error(`Invalid variant provided: ${variant}`); + } + const html = typeof document_html === "string" ? document_html : "";
53-75: Potential schema mismatch: this helper always uses Document Editor schema.
If callers pass Rich Text Editor binary, this will produce incorrect JSON/HTML. Either:
- add a variant parameter and branch accordingly, or
- deprecate this in favor of the editor-specific helpers you already import.
Option A (variant-aware):
-export const getAllDocumentFormatsFromBinaryData = ( - description: Uint8Array -): { +export const getAllDocumentFormatsFromBinaryData = ( + description: Uint8Array, + variant: "document" | "rich" = "document" +): { contentBinaryEncoded: string; contentJSON: object; contentHTML: string; } => { - // encode binary description data - const base64Data = Buffer.from(description).toString("base64"); - const yDoc = new Y.Doc(); - Y.applyUpdate(yDoc, description); - // convert to JSON - const type = yDoc.getXmlFragment("default"); - const contentJSON = yXmlFragmentToProseMirrorRootNode(type, documentEditorSchema).toJSON(); - // convert to HTML - const contentHTML = generateHTML(contentJSON, DOCUMENT_EDITOR_EXTENSIONS); - - return { - contentBinaryEncoded: base64Data, - contentJSON, - contentHTML, - }; + if (variant === "rich") { + return getAllDocumentFormatsFromRichTextEditorBinaryData(description); + } + return getAllDocumentFormatsFromDocumentEditorBinaryData(description); };Option B (deprecate):
/** @deprecated Use getAllDocumentFormatsFromDocumentEditorBinaryData or getAllDocumentFormatsFromRichTextEditorBinaryData */
61-61: Consistency: use shared base64 helper to avoid environment assumptions.
Using Buffer ties this helper to Node. Prefer the editor’s convertBinaryDataToBase64String to keep behavior uniform.- const base64Data = Buffer.from(description).toString("base64"); + // import { convertBinaryDataToBase64String } from "@plane/editor"; + const base64Data = convertBinaryDataToBase64String(description);
77-92: Mirror the variant handling for HTML→binary to prevent misuse.
This helper is document-editor–specific by construction; either rename to reflect that or accept a variant param and branch.-export const getBinaryDataFromHTMLString = ( - descriptionHTML: string -): { +export const getBinaryDataFromHTMLString = ( + descriptionHTML: string, + variant: "document" | "rich" = "document" +): { contentBinary: Uint8Array; } => { - // convert HTML to JSON - const contentJSON = generateJSON(descriptionHTML ?? "<p></p>", DOCUMENT_EDITOR_EXTENSIONS); - // convert JSON to Y.Doc format - const transformedData = prosemirrorJSONToYDoc(documentEditorSchema, contentJSON, "default"); - // convert Y.Doc to Uint8Array format - const encodedData = Y.encodeStateAsUpdate(transformedData); + if (variant === "rich") { + const encodedData = getBinaryDataFromRichTextEditorHTMLString(descriptionHTML); + return { contentBinary: encodedData }; + } + const contentJSON = generateJSON(descriptionHTML ?? "<p></p>", DOCUMENT_EDITOR_EXTENSIONS); + const transformedData = prosemirrorJSONToYDoc(documentEditorSchema, contentJSON, "default"); + const encodedData = Y.encodeStateAsUpdate(transformedData); return { contentBinary: encodedData, }; };
25-51: Add basic unit tests for both variants ("rich" and "document").
Low-effort tests will catch regressions in editor conversions and payload shape.I can scaffold tests that feed minimal HTML for both variants and assert description_html/description_binary/description keys. Want me to open a task and push a spec file?
apps/live/src/controllers/collaboration.controller.ts (1)
8-10: Remove unused metrics or increment it on errorCurrently unused; either remove or increment in the error handler.
- private metrics = { - errors: 0, - };Alternatively:
ws.on("error", (error: Error) => { logger.error("WebSocket connection error:", error); + this.metrics.errors += 1; ws.close(1011, "Internal error"); });apps/live/src/services/page.service.ts (1)
41-56: Harden request body types and normalize error propagationAvoid the broad object type and make error handling consistent with other methods.
- data: { - description_binary: string; - description_html: string; - description: object; - }, + data: { + description_binary: string; + description_html: string; + description: Record<string, unknown>; + }, @@ - .catch((error) => { - throw error; - }); + .catch((error) => { + throw error?.response?.data ?? error; + });apps/live/src/services/user.service.ts (1)
17-27: Tighten typing and align error handling with PageServiceUse the generic of get (if supported) and normalize thrown errors.
- async currentUser(cookie: string): Promise<IUser> { - return this.get("/api/users/me/", { + async currentUser(cookie: string): Promise<IUser> { + return this.get<IUser>("/api/users/me/", { @@ - .catch((error) => { - throw error; - }); + .catch((error) => { + throw error?.response?.data ?? error; + });If APIService.get doesn’t support generics, keep the return type but retain the normalized catch.
apps/live/src/controllers/index.ts (1)
1-5: Reorder imports to satisfy lint and keep array stableSort imports per lint and optionally mark the array as const.
-import { HealthController } from "./health.controller"; -import { CollaborationController } from "./collaboration.controller"; -import { ConvertDocumentController } from "./convert-document.controller"; +import { CollaborationController } from "./collaboration.controller"; +import { ConvertDocumentController } from "./convert-document.controller"; +import { HealthController } from "./health.controller"; @@ -export const CONTROLLERS = [CollaborationController, ConvertDocumentController, HealthController]; +export const CONTROLLERS = [CollaborationController, ConvertDocumentController, HealthController] as const;apps/live/src/controllers/health.controller.ts (1)
7-12: Drop unnecessary async and set no-store cache header on health responseNo awaits in handler; also prevent intermediaries from caching health responses.
- async healthCheck(_req: Request, res: Response) { - res.status(200).json({ + healthCheck(_req: Request, res: Response) { + res.set("Cache-Control", "no-store"); + res.status(200).json({ status: "OK", timestamp: new Date().toISOString(), version: process.env.APP_VERSION || "1.0.0", }); }apps/live/src/start.ts (3)
8-14: Clean up partially initialized resources on startup failureCall destroy() before exiting if initialize() partially succeeded.
try { await server.initialize(); server.listen(); } catch (error) { logger.error("Failed to start server:", error); - process.exit(1); + try { + if (server) { + await server.destroy(); + } + } finally { + process.exit(1); + } }
19-30: Handle SIGTERM/SIGINT for graceful shutdown (Kubernetes, systemd, Docker)Add termination signal handlers to close sockets and Redis cleanly.
process.on("unhandledRejection", async (err: any) => { logger.error(`UNHANDLED REJECTION! 💥 Shutting down...`, err); try { if (server) { await server.destroy(); } } finally { logger.info("Exiting process..."); process.exit(1); } }); + +// Graceful shutdown on termination signals +for (const signal of ["SIGTERM", "SIGINT"] as const) { + process.on(signal, async () => { + logger.info(`${signal} received. Shutting down gracefully...`); + try { + if (server) { + await server.destroy(); + } + process.exit(0); + } catch (err) { + logger.error("Error during graceful shutdown:", err); + process.exit(1); + } + }); +}
33-35: Prefer unknown over any for error parametersNarrow types; cast only where needed.
-process.on("uncaughtException", async (err: any) => { +process.on("uncaughtException", async (err: unknown) => { logger.error(`UNCAUGHT EXCEPTION! 💥 Shutting down...`, err);apps/live/src/lib/page.ts (1)
9-16: Add explicit return types and remove redundant toString() on URLSearchParams.get()Improves readability and type safety.
-export const updatePageDescription = async ( +export const updatePageDescription = async ( params: URLSearchParams, pageId: string, updatedDescription: Uint8Array, cookie: string | undefined -) => { +): Promise<void> => { @@ - const workspaceSlug = params.get("workspaceSlug")?.toString(); - const projectId = params.get("projectId")?.toString(); + const workspaceSlug = params.get("workspaceSlug"); + const projectId = params.get("projectId"); @@ -const fetchDescriptionHTMLAndTransform = async ( +const fetchDescriptionHTMLAndTransform = async ( workspaceSlug: string, projectId: string, pageId: string, cookie: string -) => { +): Promise<Uint8Array | undefined> => { @@ -export const fetchPageDescriptionBinary = async ( +export const fetchPageDescriptionBinary = async ( params: URLSearchParams, pageId: string, cookie: string | undefined -) => { +): Promise<Uint8Array | null> => { @@ - const workspaceSlug = params.get("workspaceSlug")?.toString(); - const projectId = params.get("projectId")?.toString(); + const workspaceSlug = params.get("workspaceSlug"); + const projectId = params.get("projectId");Also applies to: 38-54, 56-63
apps/live/src/server.ts (3)
39-42: Clarify error log to cover all dependenciesMessage mentions only Redis though HocusPocus init is also in the try block.
- logger.error("Failed to setup Redis:", error); + logger.error("Failed to setup live-server dependencies:", error);
64-66: Avoid any in controller registrationStrengthen types in registerControllers to accept Router and controller constructors; drop the cast.
I can open a small PR in @plane/decorators to export a typed registerControllers signature so this site can remove the cast.
56-56: Restrict CORS via configurationOpen CORS is risky. Consider env-driven allowlist.
Example:
const allowed = (process.env.CORS_ORIGINS || "").split(",").filter(Boolean); this.app.use(cors({ origin: allowed.length ? allowed : false, credentials: true }));apps/live/src/redis.ts (2)
140-151: TTL handling: treat 0 explicitly and avoid falsey check
if (ttl)skips 0; prefer explicit numeric check and enforce positive TTL for setex.- if (ttl) { - await client.setex(key, ttl, value); + if (typeof ttl === "number" && ttl > 0) { + await client.setex(key, ttl, value); } else { await client.set(key, value); }
111-121: Optional: expose a waitUntilConnected() to reduce misuseMany callers will forget to await initialize() (see server usage). A tiny helper that awaits an in-flight connection before returning the client can make integration safer.
public getClient(): Redis | null { if (!this.redisClient || !this.isConnected) { logger.warn("Redis client not available or not connected"); return null; } return this.redisClient; } + + // Await any ongoing initialization and re-check availability. + public async waitUntilConnected(): Promise<Redis | null> { + if (this.connectionPromise) await this.connectionPromise; + if (!this.redisClient || !this.isConnected) return null; + return this.redisClient; + }If you’d like, I can follow through with call-site changes.
apps/live/src/hocuspocus.ts (2)
1-18: Replaceanywith Hocuspocus payload typesUse the official payload types for hooks and Database extension to satisfy lint (“Unexpected any”) and get safer access to fields.
-import { Server, Hocuspocus } from "@hocuspocus/server"; +import { Server, Hocuspocus } from "@hocuspocus/server"; +import type { + onAuthenticatePayload, + onStatelessPayload, + fetchPayload, + storePayload, +} from "@hocuspocus/server"; @@ - private onAuthenticate = async ({ requestHeaders, context, token }: any) => { + private onAuthenticate = async ({ requestHeaders, context, token }: onAuthenticatePayload) => { @@ - private onStateless = async ({ payload, document }: any) => { + private onStateless = async ({ payload, document }: onStatelessPayload) => { @@ - private onDatabaseFetch = async ({ context, documentName: pageId, requestParameters }: any) => { + private onDatabaseFetch = async ({ + context, + documentName: pageId, + requestParameters, + }: fetchPayload) => { @@ - private onDatabaseStore = async ({ context, state, documentName: pageId, requestParameters }: any) => { + private onDatabaseStore = async ({ + context, + state, + documentName: pageId, + requestParameters, + }: storePayload) => {Type names and hook shapes are documented/exported by @hocuspocus/server. (tiptap.dev, app.unpkg.com)
Also applies to: 47-90, 92-98, 100-116, 118-131
71-90: Auth: tighten error logging level for token parse fallbackConsider using warn instead of error for JSON parse fallback to reduce noise in healthy flows where headers are used.
- logger.error("Token parsing failed, using request headers:", error); + logger.warn("Token parsing failed, using request headers");
📜 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 (30)
apps/live/Dockerfile.live(1 hunks)apps/live/package.json(1 hunks)apps/live/src/ce/lib/fetch-document.ts(0 hunks)apps/live/src/ce/lib/update-document.ts(0 hunks)apps/live/src/ce/types/common.d.ts(0 hunks)apps/live/src/controllers/collaboration.controller.ts(1 hunks)apps/live/src/controllers/convert-document.controller.ts(1 hunks)apps/live/src/controllers/health.controller.ts(1 hunks)apps/live/src/controllers/index.ts(1 hunks)apps/live/src/core/extensions/index.ts(0 hunks)apps/live/src/core/helpers/convert-document.ts(0 hunks)apps/live/src/core/helpers/error-handler.ts(0 hunks)apps/live/src/core/helpers/logger.ts(0 hunks)apps/live/src/core/hocuspocus-server.ts(0 hunks)apps/live/src/core/lib/authentication.ts(0 hunks)apps/live/src/core/lib/utils/redis-url.ts(0 hunks)apps/live/src/ee/lib/fetch-document.ts(0 hunks)apps/live/src/ee/lib/update-document.ts(0 hunks)apps/live/src/ee/types/common.d.ts(0 hunks)apps/live/src/hocuspocus.ts(1 hunks)apps/live/src/lib/page.ts(4 hunks)apps/live/src/redis.ts(1 hunks)apps/live/src/server.ts(2 hunks)apps/live/src/services/page.service.ts(1 hunks)apps/live/src/services/user.service.ts(1 hunks)apps/live/src/start.ts(1 hunks)apps/live/src/types/index.ts(1 hunks)apps/live/src/utils/document.ts(1 hunks)apps/live/src/utils/index.ts(1 hunks)packages/decorators/package.json(1 hunks)
💤 Files with no reviewable changes (13)
- apps/live/src/ee/lib/fetch-document.ts
- apps/live/src/ce/types/common.d.ts
- apps/live/src/core/helpers/logger.ts
- apps/live/src/core/helpers/convert-document.ts
- apps/live/src/core/helpers/error-handler.ts
- apps/live/src/core/hocuspocus-server.ts
- apps/live/src/ee/lib/update-document.ts
- apps/live/src/core/extensions/index.ts
- apps/live/src/ce/lib/fetch-document.ts
- apps/live/src/core/lib/utils/redis-url.ts
- apps/live/src/ee/types/common.d.ts
- apps/live/src/ce/lib/update-document.ts
- apps/live/src/core/lib/authentication.ts
🧰 Additional context used
🧬 Code graph analysis (8)
apps/live/src/controllers/convert-document.controller.ts (2)
apps/live/src/types/index.ts (1)
TConvertDocumentRequestBody(7-10)apps/live/src/utils/document.ts (1)
convertHTMLDocumentToAllFormats(25-51)
apps/live/src/utils/document.ts (2)
packages/types/src/page/core.ts (1)
TDocumentPayload(66-70)packages/editor/src/core/helpers/yjs-utils.ts (4)
getBinaryDataFromRichTextEditorHTMLString(56-64)getAllDocumentFormatsFromRichTextEditorBinaryData(86-108)getBinaryDataFromDocumentEditorHTMLString(71-79)getAllDocumentFormatsFromDocumentEditorBinaryData(115-137)
apps/live/src/lib/page.ts (1)
packages/logger/src/config.ts (1)
logger(14-14)
apps/live/src/hocuspocus.ts (7)
packages/editor/src/core/types/config.ts (1)
TUserDetails(35-40)apps/live/src/types/index.ts (2)
HocusPocusServerContext(3-5)TDocumentTypes(1-1)apps/live/src/services/user.service.ts (1)
UserService(6-28)packages/editor/src/core/constants/document-collaborative-events.ts (1)
DocumentCollaborativeEvents(1-8)apps/live/src/lib/page.ts (2)
fetchPageDescriptionBinary(56-81)updatePageDescription(9-36)apps/live/src/redis.ts (1)
redisManager(210-210)apps/live/src/server.ts (1)
Server(16-90)
apps/live/src/controllers/collaboration.controller.ts (3)
apps/live/src/controllers/convert-document.controller.ts (1)
Controller(9-36)apps/live/src/controllers/health.controller.ts (1)
Controller(4-14)packages/logger/src/config.ts (1)
logger(14-14)
apps/live/src/redis.ts (1)
packages/logger/src/config.ts (1)
logger(14-14)
apps/live/src/server.ts (5)
apps/live/src/redis.ts (1)
redisManager(210-210)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(19-191)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)
apps/live/src/start.ts (2)
apps/live/src/server.ts (1)
Server(16-90)packages/logger/src/config.ts (1)
logger(14-14)
🪛 GitHub Check: Build and lint web apps
apps/live/src/controllers/index.ts
[warning] 1-1:
./health.controller import should occur after import of ./convert-document.controller
apps/live/src/hocuspocus.ts
[warning] 118-118:
Unexpected any. Specify a different type
[warning] 100-100:
Unexpected any. Specify a different type
[warning] 92-92:
Unexpected any. Specify a different type
[warning] 47-47:
Unexpected any. Specify a different type
apps/live/src/controllers/collaboration.controller.ts
[warning] 21-21:
Unexpected any. Specify a different type
[warning] 15-15:
Unexpected any. Specify a different type
[warning] 4-4:
@plane/decorators import should occur before import of @plane/logger
[warning] 2-2:
@hocuspocus/server type import should occur before type import of express
⏰ 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)
apps/live/src/utils/index.ts (1)
1-1: LGTM: clear, minimal re-export.
Keeps the utils surface tidy.apps/live/src/types/index.ts (1)
1-1: TDocumentTypes narrowing is local to apps/live and only used in hocuspocus.ts; no other internal references. Verify any external API consumers expecting otherdocumentTypevalues are updated.apps/live/src/server.ts (1)
26-28: No.wsroutes on the router; no changes needed now
Ripgrep confirms no.ws(or@Ws(usages inapps/live/src, soexpressWs(this.app)is sufficient. If you later register WebSocket routes onthis.router(e.g. inCollaborationController), patch it as follows:const ws = expressWs(this.app); // Patch router so it gains .ws handlers expressWs(this.router as any, ws.getWss());apps/live/src/redis.ts (1)
12-33: Singleton + idempotent initialize(): solid foundationThe instance pattern, connectionPromise guard, and idempotent initialize() look good.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/decorators/src/controller.ts (2)
42-44: Preserve DI for REST controllers.
Let registerRestController reuse the provided instance; fallback only if none supplied.Apply:
-function registerRestController(router: Router, Controller: ControllerConstructor): void { - const instance = new Controller(); +function registerRestController( + router: Router, + Controller: ControllerConstructor, + existingInstance?: ControllerInstance +): void { + const instance = existingInstance || new Controller();
86-95: Type error: Router.ws is not on express.Router.
This won’t type-check in TS. Use a type guard (or cast) before accessing ws.Apply:
- if (typeof handler === "function" && "ws" in router && typeof router.ws === "function") { - router.ws(`${baseRoute}${route}`, (ws: WebSocket, req: Request) => { + if (typeof handler === "function" && isWsRouter(router)) { + 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"); } }); }Add this helper near the top of the file (outside the selected range):
type WsRouter = Router & { ws: (path: string, handler: (ws: WebSocket, req: Request) => void) => void; }; function isWsRouter(router: Router): router is WsRouter { return "ws" in (router as any) && typeof (router as any).ws === "function"; }
♻️ Duplicate comments (1)
apps/live/src/server.ts (1)
86-95: Guard serverInstance.close(); avoid throwing when server wasn’t started.Calling close() when listen() hasn’t run will throw. Make shutdown idempotent. (Echoing a prior suggestion.)
Apply this diff:
- if (this.serverInstance) { - // Close the Express server - this.serverInstance.close(() => { - logger.info("Express server closed gracefully."); - }); - } else { - logger.warn("Express server not found"); - throw new Error("Express server not found"); - } + if (this.serverInstance && typeof this.serverInstance.close === "function") { + // Close the Express server + this.serverInstance.close(() => { + logger.info("Express server closed gracefully."); + }); + } else { + logger.warn("Express server was not started or is already closed."); + }
🧹 Nitpick comments (8)
packages/decorators/src/controller.ts (3)
56-63: Wrap async handlers to avoid unhandled promise rejections in Express 4.
If any controller method is async, bind alone won’t route errors to next().Apply:
- handler.bind(instance) + ((req, res, next) => + Promise.resolve((handler as RequestHandler).call(instance, req, res, next)).catch(next))
80-81: Minor: use HttpMethod type consistently.
Tighten typing for metadata.Apply:
- const method = Reflect.getMetadata("method", instance, methodName) as string; + const method = Reflect.getMetadata("method", instance, methodName) as HttpMethod;
91-93: Prefer a shared logger over console.error.
Keeps output consistent with app logging.Possible change:
- console.error(`WebSocket error in ${Controller.name}.${methodName}`, error); + logger?.error?.(`WebSocket error in ${Controller.name}.${methodName}`, error);Or inject a logger into the controller package.
apps/live/src/server.ts (2)
17-21: Tighten types: replace any with concrete types; remove as any later.Use proper types for app/router/server and the Hocuspocus server. This catches interface drift at compile time.
Example:
-import express, { Request, Response } from "express"; +import express, { Request, Response } from "express"; +import type { Application, Router } from "express"; +import type { Server as HttpServer } from "http"; +import type { Hocuspocus } from "@hocuspocus/server"; @@ - private app: any; - private router: any; - private hocuspocusServer: any; - private serverInstance: any; + private app: Application; + private router: Router; + private hocuspocusServer: Hocuspocus | null = null; + private serverInstance: HttpServer | null = null;Also, type CONTROLLERS as ControllerConstructor[] in apps/live/src/controllers/index.ts so you can drop the cast:
- CONTROLLERS.forEach((controller) => registerController(this.router, controller as any)); + CONTROLLERS.forEach((Controller) => registerController(this.router, Controller));
56-56: Consider restricting CORS.Open CORS may be undesirable for a live server. Consider env-driven allowed origins and credentials.
Example:
const allowedOrigins = (process.env.LIVE_ALLOWED_ORIGINS || "").split(",").filter(Boolean); this.app.use(cors({ origin: allowedOrigins.length ? allowedOrigins : false, credentials: true, }));apps/live/src/utils/document.ts (3)
19-23: Nit: prefer camelCase in internal TS types.Consider documentHtml internally and mapping from snake_case at the controller boundary to keep code style consistent. Low priority.
24-50: Minor dedup + exhaustiveness: consolidate branches and ensure compile-time coverage.You can reduce duplication and make the variant handling exhaustive.
Apply within this range:
-export const convertHTMLDocumentToAllFormats = (args: TArgs): TDocumentPayload => { - const { document_html, variant } = args; - - if (variant === "rich") { - const contentBinary = getBinaryDataFromRichTextEditorHTMLString(document_html); - const { contentBinaryEncoded, contentHTML, contentJSON } = - getAllDocumentFormatsFromRichTextEditorBinaryData(contentBinary); - return { - description: contentJSON, - description_html: contentHTML, - description_binary: contentBinaryEncoded, - }; - } - - if (variant === "document") { - const contentBinary = getBinaryDataFromDocumentEditorHTMLString(document_html); - const { contentBinaryEncoded, contentHTML, contentJSON } = - getAllDocumentFormatsFromDocumentEditorBinaryData(contentBinary); - return { - description: contentJSON, - description_html: contentHTML, - description_binary: contentBinaryEncoded, - }; - } - - throw new Error(`Invalid variant provided: ${variant}`); -}; +export const convertHTMLDocumentToAllFormats = (args: TArgs): TDocumentPayload => { + const { document_html, variant } = args; + + const toPayload = ({ contentBinaryEncoded, contentHTML, contentJSON }: { + contentBinaryEncoded: string; contentHTML: string; contentJSON: object; + }): TDocumentPayload => ({ + description: contentJSON, + description_html: contentHTML, + description_binary: contentBinaryEncoded, + }); + + switch (variant) { + case "rich": { + const contentBinary = getBinaryDataFromRichTextEditorHTMLString(document_html); + return toPayload(getAllDocumentFormatsFromRichTextEditorBinaryData(contentBinary)); + } + case "document": { + const contentBinary = getBinaryDataFromDocumentEditorHTMLString(document_html); + return toPayload(getAllDocumentFormatsFromDocumentEditorBinaryData(contentBinary)); + } + default: + // keep runtime guard; see assertNever below for TS exhaustiveness + throw new Error(`Invalid variant provided: ${variant as never}`); + } +};Add once (outside this range) to get TS exhaustiveness errors if new variants are added:
// helper for exhaustive switch const assertNever = (x: never): never => { throw new Error(`Invalid variant provided: ${x}`); };
52-74: Delegate legacy helpers to @plane/editor to avoid drift.These utilities duplicate logic already exposed by @plane/editor and may diverge from editor schemas/extensions over time.
export const getAllDocumentFormatsFromBinaryData = ( description: Uint8Array ): { contentBinaryEncoded: string; contentJSON: object; contentHTML: string; } => { - // encode binary description data - const base64Data = Buffer.from(description).toString("base64"); - const yDoc = new Y.Doc(); - Y.applyUpdate(yDoc, description); - // convert to JSON - const type = yDoc.getXmlFragment("default"); - const contentJSON = yXmlFragmentToProseMirrorRootNode(type, documentEditorSchema).toJSON(); - // convert to HTML - const contentHTML = generateHTML(contentJSON, DOCUMENT_EDITOR_EXTENSIONS); - - return { - contentBinaryEncoded: base64Data, - contentJSON, - contentHTML, - }; + return getAllDocumentFormatsFromDocumentEditorBinaryData(description); }; export const getBinaryDataFromHTMLString = ( descriptionHTML: string ): { contentBinary: Uint8Array; } => { - // convert HTML to JSON - const contentJSON = generateJSON(descriptionHTML ?? "<p></p>", DOCUMENT_EDITOR_EXTENSIONS); - // convert JSON to Y.Doc format - const transformedData = prosemirrorJSONToYDoc(documentEditorSchema, contentJSON, "default"); - // convert Y.Doc to Uint8Array format - const encodedData = Y.encodeStateAsUpdate(transformedData); - - return { - contentBinary: encodedData, - }; + return { contentBinary: getBinaryDataFromDocumentEditorHTMLString(descriptionHTML) }; };If you adopt this, some top-level imports (generateHTML/generateJSON/getSchema/prosemirrorJSONToYDoc/yXmlFragmentToProseMirrorRootNode/Y) can be dropped.
Also applies to: 76-91
📜 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 (7)
apps/live/src/controllers/collaboration.controller.ts(1 hunks)apps/live/src/controllers/convert-document.controller.ts(1 hunks)apps/live/src/server.ts(2 hunks)apps/live/src/utils/document.ts(1 hunks)apps/live/tsdown.config.ts(1 hunks)packages/decorators/src/controller.ts(1 hunks)packages/decorators/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/live/src/controllers/convert-document.controller.ts
- apps/live/src/controllers/collaboration.controller.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/live/src/utils/document.ts (2)
packages/types/src/page/core.ts (1)
TDocumentPayload(66-70)packages/editor/src/core/helpers/yjs-utils.ts (4)
getBinaryDataFromRichTextEditorHTMLString(56-64)getAllDocumentFormatsFromRichTextEditorBinaryData(86-108)getBinaryDataFromDocumentEditorHTMLString(71-79)getAllDocumentFormatsFromDocumentEditorBinaryData(115-137)
packages/decorators/src/controller.ts (3)
apps/live/src/controllers/collaboration.controller.ts (1)
Controller(7-27)apps/live/src/controllers/convert-document.controller.ts (1)
Controller(10-37)packages/decorators/src/rest.ts (1)
Controller(12-17)
apps/live/src/server.ts (6)
apps/live/src/redis.ts (1)
redisManager(210-210)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(19-191)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)packages/decorators/src/controller.ts (1)
registerController(17-40)
⏰ 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 (5)
packages/decorators/src/index.ts (1)
1-3: No internal references toRest.,WebSocketNS., orregisterControllersfound
Ripgrep across *.ts, *.tsx, *.js, and *.jsx returned no matches. Manually verify that no external consumers depend on these namespaces or theregisterControllerssymbol before merging.apps/live/src/server.ts (1)
32-43: Verify RedisManager.initialize() signature and await if necessary
• Confirm whetherredisManager.initialize()returns a Promise; if it does, prependawaitto avoid racing HocusPocus setup.
• Change the catch log to a generic message (e.g."Failed to initialize live server components:") instead of blaming only Redis.apps/live/src/utils/document.ts (3)
13-15: Type contract import matches the payload shape. LGTM.
24-50: Core logic is clear and correct for both variants; payload mapping looks right.
6-11: Confirmed SSR/Node safety These @plane/editor helpers reference no browser globals (window/document/DOMParser) and use only Node-compatible APIs (Buffer, Y.Doc), so they’re SSR-safe and tree-shakeable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/live/src/server.ts (2)
87-96: Guard server close; don’t throw when not startedGraceful destroy should not throw if
listen()never ran. Also checkcloseshape.- if (this.serverInstance) { - // Close the Express server - this.serverInstance.close(() => { - logger.info("Express server closed gracefully."); - }); - } else { - logger.warn("Express server not found"); - throw new Error("Express server not found"); - } + if (this.serverInstance && typeof this.serverInstance.close === "function") { + this.serverInstance.close(() => { + logger.info("Express server closed gracefully."); + }); + this.serverInstance = null; + } else { + logger.warn("Express server was not started or is already closed."); + }
23-31: Fix WS and middleware ordering; move 404 to the very end
- Router is created/mounted before express-ws, so
router.wsmay be undefined.- Router is mounted before global middlewares (helmet, logger, body parsers, cors), so those won’t run for router routes that end the response.
- 404 is registered inside
setupMiddleware; register it last so it doesn’t swallow routes after reordering.Apply:
@@ constructor() { - this.app = express(); - this.router = express.Router(); - this.app.set("port", process.env.PORT || 3000); - this.app.use(process.env.LIVE_BASE_PATH || "/live", this.router); - expressWs(this.app); - this.setupMiddleware(); - this.setupRoutes(); + this.app = express(); + // Patch WS BEFORE creating any routers + expressWs(this.app); + this.app.set("port", process.env.PORT || 3000); + // Register global middlewares first + this.setupMiddleware(); + // Create and mount the router AFTER middlewares so routes benefit from them + this.router = express.Router(); + this.app.use(process.env.LIVE_BASE_PATH || "/live", this.router); + this.setupRoutes(); + // Register the 404 handler LAST + this.setupNotFoundHandler(); } @@ private setupMiddleware() { @@ - // cors middleware + // CORS middleware this.app.use(cors()); - this.app.use((_req: Request, res: Response) => { - res.status(404).json({ - message: "Not Found", - }); - }); } @@ private setupRoutes() { // eslint-disable-next-line @typescript-eslint/no-explicit-any - CONTROLLERS.forEach((controller) => registerController(this.router, controller as any)); + CONTROLLERS.forEach((controller) => registerController(this.router, controller as any)); }Add outside the ranges above:
private setupNotFoundHandler() { this.app.use((_req: Request, res: Response) => { res.status(404).json({ message: "Not Found" }); }); }Also applies to: 46-63, 65-69
🧹 Nitpick comments (3)
apps/live/src/server.ts (3)
21-21: Type the HTTP server instead ofanyUse Node’s
http.Serverto regain type-safety.+import type { Server as HttpServer } from "http"; @@ - private serverInstance: any; + private serverInstance: HttpServer | null = null;
65-68: Eliminateas anyby exporting a typed controllers arrayPrefer types over casts. Update controllers to export a typed list so this call site needs no
any.Proposed change in apps/live/src/controllers/index.ts (outside this file):
import type { ControllerConstructor } from "@plane/decorators"; export const CONTROLLERS: ControllerConstructor[] = [CollaborationController, ConvertDocumentController, HealthController];Then here:
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - CONTROLLERS.forEach((controller) => registerController(this.router, controller as any)); + CONTROLLERS.forEach((Controller) => registerController(this.router, Controller));
70-74: Optional: set HTTP timeouts to mitigate slowlorisAdd conservative timeouts after
listen().public listen() { this.serverInstance = this.app.listen(this.app.get("port"), () => { logger.info(`Plane Live server has started at port ${this.app.get("port")}`); }); + // Hardening + if (this.serverInstance) { + this.serverInstance.keepAliveTimeout = 65_000; + this.serverInstance.headersTimeout = 66_000; + this.serverInstance.requestTimeout = 60_000; + } }
📜 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 (3)
apps/live/src/controllers/index.ts(1 hunks)apps/live/src/server.ts(1 hunks)apps/live/src/start.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/live/src/controllers/index.ts
- apps/live/src/start.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/live/src/server.ts (6)
apps/live/src/redis.ts (1)
redisManager(210-210)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(19-191)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)packages/decorators/src/controller.ts (1)
registerController(17-40)
⏰ 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)
sriramveeraghanta
force-pushed
the
feat-live-server-sync
branch
from
b7f2f51 to
68049cc
Compare
fix: code refactor fix: lint errors
sriramveeraghanta
force-pushed
the
feat-live-server-sync
branch
from
68049cc to
e45815f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/decorators/src/controller.ts (2)
42-67: Preserve injected deps for REST; default baseRoute safely.Let REST reuse the same instance and guard undefined 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) || "";
86-95: Fix typings for router.ws to avoid TS errors across packages.This package may not see express-ws augmentation. Use a local type to call ws safely.
- if (typeof handler === "function" && "ws" in router && typeof router.ws === "function") { - router.ws(`${baseRoute}${route}`, (ws: WebSocket, req: Request) => { + if (typeof handler === "function" && "ws" in router) { + type RouterWS = Router & { ws: (path: string, cb: (ws: WebSocket, req: Request) => void) => void }; + (router as RouterWS).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"); } }); }
♻️ Duplicate comments (7)
apps/live/package.json (1)
12-12: Require a compatible Node engine and enforce zero lint warnings.node --env-file needs Node ≥20.11. Add engines and make lint fail on warnings for consistent CI.
"type": "module", + "engines": { "node": ">=20.11.0" }, "scripts": { @@ - "check:lint": "eslint . --max-warnings 10", + "check:lint": "eslint . --max-warnings 0",Also applies to: 13-13
apps/live/src/hocuspocus.ts (1)
135-165: Initialize Redis and start without the Redis extension if unavailable.Throwing here prevents the server from starting even when Redis is optional. Await init and conditionally include the extension.
public async initialize(): Promise<Hocuspocus> { @@ - const redisClient = redisManager.getClient(); - if (!redisClient) { - throw new Error("Redis client not initialized"); - } + await redisManager.initialize(); + const redisClient = redisManager.getClient(); @@ - this.server = Server.configure({ + const extensions = [ + new Logger({ + onChange: false, + log: (message) => { + logger.info(message); + }, + }), + new Database({ + fetch: this.onDatabaseFetch, + store: this.onDatabaseStore, + }), + ]; + if (redisClient) { + extensions.push(new Redis({ redis: redisClient })); + } else { + logger.warn("Redis not configured/connected; starting without Redis extension"); + } + + this.server = Server.configure({ name: this.serverName, onAuthenticate: this.onAuthenticate, onStateless: this.onStateless, - extensions: [ - new Logger({ - onChange: false, - log: (message) => { - logger.info(message); - }, - }), - new Database({ - fetch: this.onDatabaseFetch, - store: this.onDatabaseStore, - }), - new Redis({ - redis: redisClient, - }), - ], + extensions, debounce: 10000, });packages/decorators/src/controller.ts (1)
17-40: Always register both REST and WS using the same instance.Current branching skips REST when WS handlers exist and loses DI for REST. Register both.
export function registerController( router: Router, Controller: ControllerConstructor, dependencies: unknown[] = [] ): void { - // Create the controller instance with dependencies - const instance = new Controller(...dependencies); - - // 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); - } + const instance = new Controller(...dependencies); + registerRestController(router, Controller, instance); + registerWebSocketController(router, Controller, instance); }apps/live/src/server.ts (4)
23-32: Nice: correct express-ws and middleware order
expressWs patched before creating/mounting the router; middlewares precede routes; 404 last. This avoids router.ws issues and missing middlewares.
69-72: Remove any-cast; type controllers to avoid eslint disable
Type CONTROLLERS and drop the cast/disable.- // eslint-disable-next-line @typescript-eslint/no-explicit-any - CONTROLLERS.forEach((controller) => registerController(this.router, controller as any)); + CONTROLLERS.forEach((Controller) => registerController(this.router, Controller));Update apps/live/src/controllers/index.ts:
// apps/live/src/controllers/index.ts import type { ControllerConstructor } from "@plane/decorators"; export const CONTROLLERS: ReadonlyArray<ControllerConstructor> = [ CollaborationController, ConvertDocumentController, HealthController, ];
34-41: Good: dependency init ordering and error propagation
Awaiting Redis before HocusPocus prevents race; errors are logged and rethrown.
91-100: Don’t throw during destroy(); guard close() and await itThrowing on “server not found” breaks graceful shutdown paths (e.g., startup errors, signal exits). Also guard and await http.Server.close to avoid races.
- if (this.serverInstance) { - // Close the Express server - this.serverInstance.close(() => { - logger.info("Express server closed gracefully."); - }); - } else { - logger.warn("Express server not found"); - throw new Error("Express server not found"); - } + if (this.serverInstance && typeof (this.serverInstance as any).close === "function") { + await new Promise<void>((resolve, reject) => { + this.serverInstance!.close((err?: Error) => { + if (err) { + logger.error("Error closing Express server.", err); + return reject(err); + } + logger.info("Express server closed gracefully."); + resolve(); + }); + }); + } else { + logger.warn("Express server was not started or is already closed."); + }
🧹 Nitpick comments (10)
apps/live/src/utils/document.ts (2)
52-74: Avoid duplicating editor logic; delegate to editor helper.Wrap the editor’s getAllDocumentFormatsFromDocumentEditorBinaryData to reduce drift and keep schema logic centralized.
-export const getAllDocumentFormatsFromBinaryData = ( - description: Uint8Array -): { - contentBinaryEncoded: string; - contentJSON: object; - contentHTML: string; -} => { - // encode binary description data - const base64Data = Buffer.from(description).toString("base64"); - const yDoc = new Y.Doc(); - Y.applyUpdate(yDoc, description); - // convert to JSON - const type = yDoc.getXmlFragment("default"); - const contentJSON = yXmlFragmentToProseMirrorRootNode(type, documentEditorSchema).toJSON(); - // convert to HTML - const contentHTML = generateHTML(contentJSON, DOCUMENT_EDITOR_EXTENSIONS); - - return { - contentBinaryEncoded: base64Data, - contentJSON, - contentHTML, - }; -}; +export const getAllDocumentFormatsFromBinaryData = (description: Uint8Array) => + getAllDocumentFormatsFromDocumentEditorBinaryData(description);
60-61: Prefer shared base64 utility for consistency.If you keep a local implementation, consider using the editor’s convertBinaryDataToBase64String for identical encoding behavior across services.
packages/decorators/src/controller.ts (1)
56-63: Optional: async error wrapper for REST handlers.Prevent unhandled promise rejections from async handlers by wrapping.
- (router[method] as (path: string, ...handlers: RequestHandler[]) => void)( + (router[method] as (path: string, ...handlers: RequestHandler[]) => void)( `${baseRoute}${route}`, ...middlewares, - handler.bind(instance) + ((fn) => + function wrapped(req, res, next) { + Promise.resolve(fn.call(instance, req, res, next)).catch(next); + })(handler as RequestHandler) );apps/live/src/start.ts (4)
11-14: Ensure teardown on startup failureCall destroy() in the catch to avoid leaving Redis/HocusPocus initialized when listen() fails.
} catch (error) { logger.error("Failed to start server:", error); - process.exit(1); + try { + if (server) { + await server.destroy(); + } + } finally { + process.exit(1); + } }
20-43: Unify and harden shutdown hooks (once, dedupe, SIGTERM/SIGINT, proper typing)
- Use process.once to prevent duplicate handlers.
- Add SIGTERM/SIGINT for container/K8s shutdowns.
- Deduplicate with a shared shutdown() and guard re-entrancy.
- Type unhandledRejection reason as unknown.
-// Graceful shutdown on unhandled rejection -process.on("unhandledRejection", async (err: Error) => { - logger.error(`UNHANDLED REJECTION! 💥 Shutting down...`, err); - try { - if (server) { - await server.destroy(); - } - } finally { - logger.info("Exiting process..."); - process.exit(1); - } -}); - -// Graceful shutdown on uncaught exception -process.on("uncaughtException", async (err: Error) => { - logger.error(`UNCAUGHT EXCEPTION! 💥 Shutting down...`, err); - try { - if (server) { - await server.destroy(); - } - } finally { - logger.info("Exiting process..."); - process.exit(1); - } -}); +// Graceful shutdown hooks +process.once("unhandledRejection", async (err: unknown) => { + await shutdown("UNHANDLED REJECTION! 💥 Shutting down...", err); +}); + +process.once("uncaughtException", async (err: Error) => { + await shutdown("UNCAUGHT EXCEPTION! 💥 Shutting down...", err); +}); + +process.once("SIGTERM", async () => { + await shutdown("SIGTERM received. Shutting down gracefully...", undefined, 0); +}); + +process.once("SIGINT", async () => { + await shutdown("SIGINT received. Shutting down gracefully...", undefined, 0); +});Add near the top (supports the diff above):
let isShuttingDown = false; async function shutdown(reason: string, err?: unknown, exitCode = 1) { if (isShuttingDown) return; isShuttingDown = true; if (err) logger.error(reason, err); else logger.warn(reason); try { if (server) await server.destroy(); } catch (e) { logger.error("Error during shutdown:", e as Error); } finally { logger.info("Exiting process..."); process.exit(exitCode); } }
4-4: Prefer nullable typing for late assignmentAvoids false positives in guards and improves readability.
-let server: Server; +let server: Server | null = null;
17-18: Register shutdown hooks before startingMove startServer() below the hook registrations to catch early failures outside the try/catch.
apps/live/src/server.ts (3)
21-21: Type the server instance as http.Server
Improves safety and removes the need for any.- private serverInstance: any; + private serverInstance?: HttpServer;Add import at the top:
import { Server as HttpServer } from "http";
47-59: Set request body limits and tighten CORS
Prevents oversized payloads and unintended wide-open CORS in production.- // Body parsing middleware - this.app.use(express.json()); - this.app.use(express.urlencoded({ extended: true })); - // cors middleware - this.app.use(cors()); + // Body parsing middleware + this.app.use(express.json({ limit: process.env.LIVE_BODY_LIMIT || "1mb" })); + this.app.use(express.urlencoded({ extended: true, limit: process.env.LIVE_BODY_LIMIT || "1mb" })); + // CORS middleware + this.app.use( + cors({ + origin: process.env.CORS_ORIGIN ? process.env.CORS_ORIGIN.split(",") : true, + credentials: true, + }) + );
24-24: Hide X-Powered-By header
Minor hardening; Helmet no longer hides it by default.constructor() { - this.app = express(); + this.app = express(); + this.app.disable("x-powered-by");
📜 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 (33)
apps/live/Dockerfile.live(1 hunks)apps/live/package.json(2 hunks)apps/live/src/ce/lib/fetch-document.ts(0 hunks)apps/live/src/ce/lib/update-document.ts(0 hunks)apps/live/src/ce/types/common.d.ts(0 hunks)apps/live/src/controllers/collaboration.controller.ts(1 hunks)apps/live/src/controllers/convert-document.controller.ts(1 hunks)apps/live/src/controllers/health.controller.ts(1 hunks)apps/live/src/controllers/index.ts(1 hunks)apps/live/src/core/extensions/index.ts(0 hunks)apps/live/src/core/helpers/convert-document.ts(0 hunks)apps/live/src/core/helpers/error-handler.ts(0 hunks)apps/live/src/core/helpers/logger.ts(0 hunks)apps/live/src/core/hocuspocus-server.ts(0 hunks)apps/live/src/core/lib/authentication.ts(0 hunks)apps/live/src/core/lib/utils/redis-url.ts(0 hunks)apps/live/src/ee/lib/fetch-document.ts(0 hunks)apps/live/src/ee/lib/update-document.ts(0 hunks)apps/live/src/ee/types/common.d.ts(0 hunks)apps/live/src/hocuspocus.ts(1 hunks)apps/live/src/lib/page.ts(4 hunks)apps/live/src/redis.ts(1 hunks)apps/live/src/server.ts(2 hunks)apps/live/src/services/page.service.ts(1 hunks)apps/live/src/services/user.service.ts(1 hunks)apps/live/src/start.ts(1 hunks)apps/live/src/types/index.ts(1 hunks)apps/live/src/utils/document.ts(1 hunks)apps/live/src/utils/index.ts(1 hunks)apps/live/tsdown.config.ts(1 hunks)packages/decorators/package.json(1 hunks)packages/decorators/src/controller.ts(1 hunks)packages/decorators/src/index.ts(1 hunks)
💤 Files with no reviewable changes (13)
- apps/live/src/ee/lib/update-document.ts
- apps/live/src/ce/types/common.d.ts
- apps/live/src/core/lib/utils/redis-url.ts
- apps/live/src/ee/types/common.d.ts
- apps/live/src/core/helpers/convert-document.ts
- apps/live/src/ce/lib/update-document.ts
- apps/live/src/core/hocuspocus-server.ts
- apps/live/src/core/extensions/index.ts
- apps/live/src/core/helpers/error-handler.ts
- apps/live/src/core/helpers/logger.ts
- apps/live/src/core/lib/authentication.ts
- apps/live/src/ee/lib/fetch-document.ts
- apps/live/src/ce/lib/fetch-document.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/live/src/utils/index.ts
- apps/live/src/controllers/index.ts
- apps/live/Dockerfile.live
- apps/live/src/services/page.service.ts
- packages/decorators/package.json
- packages/decorators/src/index.ts
- apps/live/tsdown.config.ts
- apps/live/src/redis.ts
- apps/live/src/controllers/health.controller.ts
- apps/live/src/lib/page.ts
- apps/live/src/services/user.service.ts
- apps/live/src/controllers/collaboration.controller.ts
- apps/live/src/types/index.ts
- apps/live/src/controllers/convert-document.controller.ts
🧰 Additional context used
🧬 Code graph analysis (5)
apps/live/src/start.ts (2)
apps/live/src/server.ts (1)
Server(17-101)packages/logger/src/config.ts (1)
logger(14-14)
apps/live/src/utils/document.ts (3)
packages/editor/src/core/extensions/core-without-props.ts (1)
DocumentEditorExtensionsWithoutProps(105-105)packages/types/src/page/core.ts (1)
TDocumentPayload(66-70)packages/editor/src/core/helpers/yjs-utils.ts (4)
getBinaryDataFromRichTextEditorHTMLString(56-64)getAllDocumentFormatsFromRichTextEditorBinaryData(86-108)getBinaryDataFromDocumentEditorHTMLString(71-79)getAllDocumentFormatsFromDocumentEditorBinaryData(115-137)
apps/live/src/hocuspocus.ts (7)
packages/editor/src/core/types/config.ts (1)
TUserDetails(35-40)apps/live/src/types/index.ts (2)
HocusPocusServerContext(3-5)TDocumentTypes(1-1)apps/live/src/services/user.service.ts (1)
UserService(6-28)packages/editor/src/core/constants/document-collaborative-events.ts (1)
DocumentCollaborativeEvents(1-8)apps/live/src/lib/page.ts (2)
fetchPageDescriptionBinary(56-81)updatePageDescription(9-36)apps/live/src/redis.ts (1)
redisManager(210-210)apps/live/src/server.ts (1)
Server(17-101)
apps/live/src/server.ts (6)
apps/live/src/redis.ts (1)
redisManager(210-210)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(19-191)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)packages/decorators/src/controller.ts (1)
registerController(17-40)
packages/decorators/src/controller.ts (4)
apps/live/src/controllers/collaboration.controller.ts (1)
Controller(7-27)apps/live/src/controllers/convert-document.controller.ts (1)
Controller(10-37)apps/live/src/controllers/health.controller.ts (1)
Controller(4-14)packages/decorators/src/rest.ts (1)
Controller(12-17)
⏰ 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: Build and lint web apps
🔇 Additional comments (2)
apps/live/package.json (1)
36-36: dotenv.config() fallback already implemented config() is imported from “dotenv” and invoked in apps/live/src/services/api.service.ts, so no changes needed.apps/live/src/utils/document.ts (1)
24-50: API looks solid; explicit variant handling is clear.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/live/src/server.ts (1)
69-72: Remove theas anycast; type the controllers.This repeats a past comment. Export the ControllerConstructor type from @plane/decorators and type CONTROLLERS accordingly to avoid the cast here.
Example:
- CONTROLLERS.forEach((controller) => registerController(this.router, controller as any, [this.hocuspocusServer])); + // after: import type { ControllerConstructor } from "@plane/decorators"; + (CONTROLLERS as ControllerConstructor[]).forEach((Controller) => + registerController(this.router, Controller, [this.hocuspocusServer!]) + );
🧹 Nitpick comments (6)
apps/live/src/server.ts (6)
74-78: Enforce init-before-listen contract.listen() can run before initialize() and silently start without WS routes if registration is moved. Either make listen() async and call initialize() or assert initialized, failing fast.
Minimal guard:
public listen() { + if (!this.hocuspocusServer) { + throw new Error("Call initialize() before listen()."); + } this.serverInstance = this.app.listen(this.app.get("port"), () => { logger.info(`Plane Live server has started at port ${this.app.get("port")}`); }); }
91-100: Make destroy() idempotent; guard.close(); don’t throw when server wasn’t started.Shutdown paths shouldn’t throw. Also, verify close exists and await it.
- if (this.serverInstance) { - // Close the Express server - this.serverInstance.close(() => { - logger.info("Express server closed gracefully."); - }); - } else { - logger.warn("Express server not found"); - throw new Error("Express server not found"); - } + if (this.serverInstance && typeof this.serverInstance.close === "function") { + await new Promise<void>((resolve, reject) => + this.serverInstance!.close((err?: Error) => { + if (err) return reject(err); + logger.info("Express server closed gracefully."); + resolve(); + }) + ); + } else { + logger.warn("Express server was not started or is already closed."); + }
80-85: Reset reference after shutdown.Avoid double-destroy surprises by clearing the handle.
if (this.hocuspocusServer) { await this.hocuspocusServer.destroy(); logger.info("HocusPocus server WebSocket connections closed gracefully."); + this.hocuspocusServer = null; }
18-22: Type the HTTP server instead ofany.Improves safety of destroy() checks.
+import type { Server as HttpServer } from "node:http"; @@ - private serverInstance: any; + private serverInstance: HttpServer | null = null;
47-59: Middleware ordering and limits: move CORS earlier; set body size limits.
- Put CORS before parsers to short-circuit blocked origins.
- Add explicit limits to json/urlencoded to prevent DoS via large bodies.
// Security middleware this.app.use(helmet()); - // Middleware for response compression - this.app.use(compression({ level: 6, threshold: 5 * 1000 })); - // Logging middleware - this.app.use(loggerMiddleware); - // Body parsing middleware - this.app.use(express.json()); - this.app.use(express.urlencoded({ extended: true })); - // cors middleware - this.app.use(cors()); + // CORS first + this.app.use(cors()); + // Compression & logging + this.app.use(compression({ level: 6, threshold: 5 * 1000 })); + this.app.use(loggerMiddleware); + // Body parsing with limits + this.app.use(express.json({ limit: process.env.LIVE_JSON_LIMIT || "1mb" })); + this.app.use(express.urlencoded({ extended: true, limit: process.env.LIVE_URLENCODED_LIMIT || "1mb" }));
1-1: Useimport typefor type-only import.Avoids pulling in @hocuspocus/server at runtime just for typing.
-import { Hocuspocus } from "@hocuspocus/server"; +import type { Hocuspocus } from "@hocuspocus/server";
📜 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 (2)
apps/live/src/controllers/collaboration.controller.ts(1 hunks)apps/live/src/server.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/live/src/controllers/collaboration.controller.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/live/src/server.ts (6)
apps/live/src/redis.ts (1)
redisManager(210-210)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(19-191)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)packages/decorators/src/controller.ts (1)
registerController(17-40)
⏰ 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: Build and lint web apps
🔇 Additional comments (2)
apps/live/src/server.ts (2)
34-45: Good: dependency init now awaited and errors surfaced.
61-67: Good: 404 registered after routes.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/live/src/server.ts (1)
49-61: Use validated env for compression and CORS config
Hardcoded compression and process.env bypass the schema. Prefer env variables you already validate.- this.app.use(compression({ level: 6, threshold: 5 * 1000 })); + this.app.use(compression({ level: env.COMPRESSION_LEVEL, threshold: env.COMPRESSION_THRESHOLD }));
♻️ Duplicate comments (8)
apps/live/package.json (3)
12-12: Either add engines.node for --env-file or remove redundancy with dotenvxnode --env-file requires Node ≥20.11. Make this explicit, or rely solely on dotenvx in env.ts and drop --env-file.
Option A (keep --env-file):
"type": "module", + "engines": { "node": ">=20.11.0" }, "scripts": {Option B (drop redundancy):
- "start": "node --env-file=.env dist/start.js", + "start": "node dist/start.js",
13-13: Standardize lint policy to fail on warningsAlign with repo policies and avoid drifting thresholds.
- "check:lint": "eslint . --max-warnings 10", + "check:lint": "eslint . --max-warnings 0",
6-12: Align "main" with build output; dist/server.js doesn’t existBuild/start targets dist/start.js, but "main" points to dist/server.js. Update or drop "main" to avoid broken refs.
- "main": "./dist/server.js", + "main": "./dist/start.js",apps/live/src/server.ts (1)
25-34: Register routes after initialize(); avoid injecting null HocusPocus
Controllers are registered in the constructor while hocuspocusServer is null. Move setupRoutes/404 after successful initialize to ensure WS controllers receive a valid instance and avoid double registration.constructor() { this.app = express(); expressWs(this.app); this.setupMiddleware(); this.router = express.Router(); this.app.set("port", env.PORT || 3000); this.app.use(env.LIVE_BASE_PATH, this.router); - this.setupRoutes(); - this.setupNotFoundHandler(); } + private routesRegistered = false; public async initialize(): Promise<void> { try { await redisManager.initialize(); logger.info("Redis setup completed"); const manager = HocusPocusServerManager.getInstance(); this.hocuspocusServer = await manager.initialize(); logger.info("HocusPocus setup completed"); + if (!this.routesRegistered) { + this.setupRoutes(); + this.setupNotFoundHandler(); + this.routesRegistered = true; + } } catch (error) { logger.error("Failed to initialize live server dependencies:", error); throw error; } } private setupRoutes() { // eslint-disable-next-line @typescript-eslint/no-explicit-any - CONTROLLERS.forEach((controller) => registerController(this.router, controller as any, [this.hocuspocusServer])); + if (!this.hocuspocusServer) throw new Error("HocusPocus not initialized"); + CONTROLLERS.forEach((controller) => + registerController(this.router, controller as any, [this.hocuspocusServer]) + ); }Also applies to: 36-47, 84-87
apps/live/src/redis.ts (1)
103-109: Clean up client on failed connect to avoid leaks
If connect() throws, ensure sockets/timers are torn down and state is reset.} catch (error) { logger.error("Failed to initialize Redis client:", error); this.isConnected = false; - throw error; + try { + this.redisClient?.disconnect(); + } finally { + this.redisClient = null; + } + throw error; } finally { this.connectionPromise = null; }apps/live/src/hocuspocus.ts (3)
94-100: Guard unknown stateless events
Optional chaining avoids runtime errors on unexpected payloads.- const response = DocumentCollaborativeEvents[payload as TDocumentEventsServer].client; + const response = DocumentCollaborativeEvents[payload as TDocumentEventsServer]?.client;
142-167: Start without Redis if unavailable; await init here too
Avoid hard failure when Redis isn’t configured/reachable; degrade gracefully.- const redisClient = redisManager.getClient(); - if (!redisClient) { - throw new Error("Redis client not initialized"); - } + await redisManager.initialize(); + const redisClient = redisManager.getClient(); - this.server = Server.configure({ + const extensions = [ + new Logger({ + onChange: false, + log: (message) => { + logger.info(message); + }, + }), + new Database({ + fetch: this.onDatabaseFetch, + store: this.onDatabaseStore, + }), + ]; + if (redisClient) { + extensions.push( + new Redis({ + redis: redisClient, + }) + ); + } else { + logger.warn("Redis not configured/connected; starting without Redis extension"); + } + + this.server = Server.configure({ name: this.serverName, onAuthenticate: this.onAuthenticate, onStateless: this.onStateless, - extensions: [ - new Logger({ - onChange: false, - log: (message) => { - logger.info(message); - }, - }), - new Database({ - fetch: this.onDatabaseFetch, - store: this.onDatabaseStore, - }), - new Redis({ - redis: redisClient, - }), - ], + extensions, debounce: 10000, });
49-71: Allow cookie-only auth; userId should be optional
Rejecting when userId is absent blocks valid Cookie-based flows.private onAuthenticate = async ({ requestHeaders, context, token }: any) => { let cookie: string | undefined = undefined; - let userId: string | undefined = undefined; + let userId: string | undefined = undefined; @@ - if (!cookie || !userId) { + if (!cookie) { throw new Error("Credentials not provided"); } @@ - if (user.id !== userId) { - throw new Error("Authentication unsuccessful!"); - } + if (userId && user.id !== userId) throw new Error("Authentication unsuccessful!");Also applies to: 76-91
🧹 Nitpick comments (5)
apps/live/package.json (1)
23-23: Remove the unuseddotenvdependencyThere are no
import 'dotenv'orrequire('dotenv')calls inapps/live/src—only@dotenvx/dotenvxis used inapps/live/src/env.ts, so drop"dotenv"fromapps/live/package.json.apps/live/src/services/api.service.ts (2)
7-14: Guard empty baseURL (defensive check)Even with env validation, adding a runtime guard makes failures explicit.
constructor() { - this.baseURL = env.API_BASE_URL ?? ""; + this.baseURL = env.API_BASE_URL ?? ""; + if (!this.baseURL) { + throw new Error("API_BASE_URL is not configured"); + }
16-21: Clarify get signature; “params” is actually axios configRename to avoid confusion and prevent accidental misuse.
- get(url: string, params = {}, config = {}) { - return this.axiosInstance.get(url, { - ...params, - ...config, - }); + get(url: string, config = {}) { + return this.axiosInstance.get(url, config);Update callers accordingly (e.g., pass { headers } as the single config arg).
apps/live/src/server.ts (2)
106-115: Don’t throw when server wasn’t started
Destroy should be idempotent; logging a warning is sufficient.if (this.serverInstance) { // Close the Express server this.serverInstance.close(() => { logger.info("Express server closed gracefully."); }); } else { logger.warn("Express server not found"); - throw new Error("Express server not found"); }
23-24: Type serverInstance
Prefer http.Server over any for safer shutdown handling.- private serverInstance: any; + private serverInstance: import("http").Server | null = null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/live/package.json(3 hunks)apps/live/src/controllers/health.controller.ts(1 hunks)apps/live/src/env.ts(1 hunks)apps/live/src/hocuspocus.ts(1 hunks)apps/live/src/redis.ts(1 hunks)apps/live/src/server.ts(2 hunks)apps/live/src/services/api.service.ts(1 hunks)apps/live/src/services/page.service.ts(1 hunks)apps/live/src/services/user.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/live/src/controllers/health.controller.ts
- apps/live/src/services/page.service.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/live/src/services/api.service.ts (1)
apps/live/src/env.ts (1)
env(36-36)
apps/live/src/redis.ts (2)
packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/env.ts (1)
env(36-36)
apps/live/src/hocuspocus.ts (9)
apps/live/src/env.ts (1)
env(36-36)packages/editor/src/core/types/config.ts (1)
TUserDetails(35-40)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/types/index.ts (2)
HocusPocusServerContext(3-5)TDocumentTypes(1-1)apps/live/src/services/user.service.ts (1)
UserService(6-28)packages/editor/src/core/constants/document-collaborative-events.ts (1)
DocumentCollaborativeEvents(1-8)apps/live/src/lib/page.ts (2)
fetchPageDescriptionBinary(56-81)updatePageDescription(9-36)apps/live/src/redis.ts (1)
redisManager(211-211)apps/live/src/server.ts (1)
Server(19-116)
apps/live/src/server.ts (7)
apps/live/src/env.ts (1)
env(36-36)apps/live/src/redis.ts (1)
redisManager(211-211)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(21-193)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)packages/decorators/src/controller.ts (1)
registerController(17-40)
⏰ 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)
apps/live/src/services/user.service.ts (1)
4-9: LGTM; aligns with env-driven APIServiceConstructor now delegates to env-backed APIService. No issues.
apps/live/src/server.ts (1)
90-92: Ensure numeric port on listen()
If PORT is ever a string, passing it directly may open a named pipe instead of TCP.- this.serverInstance = this.app.listen(this.app.get("port"), () => { + this.serverInstance = this.app.listen(Number(this.app.get("port")), () => { logger.info(`Plane Live server has started at port ${this.app.get("port")}`); });
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
apps/live/src/server.ts (4)
25-34: Defer route registration until after initialization (prevents null HocusPocus injection).Routes are registered in the constructor while
this.hocuspocusServeris stillnull. WS controllers that expect HocusPocus will be constructed with a null dependency. Move route registration (and 404) to afterinitialize()completes.Apply:
constructor() { this.app = express(); expressWs(this.app); this.setupMiddleware(); this.router = express.Router(); this.app.set("port", env.PORT || 3000); this.app.use(env.LIVE_BASE_PATH, this.router); - this.setupRoutes(); - this.setupNotFoundHandler(); }
36-47: Register routes after deps are ready.Register routes and 404 after Redis + HocusPocus init.
public async initialize(): Promise<void> { try { await redisManager.initialize(); logger.info("Redis setup completed"); const manager = HocusPocusServerManager.getInstance(); this.hocuspocusServer = await manager.initialize(); logger.info("HocusPocus setup completed"); + // Wire controllers after deps are ready + this.setupRoutes(); + this.setupNotFoundHandler(); } catch (error) { logger.error("Failed to initialize live server dependencies:", error); throw error; } }
63-75: CORS: fix wildcard with credentials and use parsed env.
origin: "*" + credentials: truebreaks cookies; and you’re bypassingenv. Useenv.CORS_ALLOWED_ORIGINSand reflect origin when wildcard.- const allowedOrigins = - process.env.CORS_ALLOWED_ORIGINS === "*" - ? "*" - : process.env.CORS_ALLOWED_ORIGINS?.split(",")?.map((s) => s.trim()) || []; - this.app.use( - cors({ - origin: allowedOrigins, - credentials: true, - methods: ["GET", "POST", "PUT", "DELETE", "OPTIONS"], - allowedHeaders: ["Content-Type", "Authorization", "x-api-key"], - }) - ); + const raw = env.CORS_ALLOWED_ORIGINS ?? "*"; + const allowedOrigins = raw === "*" ? "*" : raw.split(",").map((s) => s.trim()).filter(Boolean); + this.app.use( + cors({ + // Reflect request origin when wildcard so credentials work + origin: allowedOrigins === "*" ? true : allowedOrigins, + credentials: true, + methods: ["GET", "POST", "PUT", "DELETE", "OPTIONS"], + allowedHeaders: ["Content-Type", "Authorization", "x-api-key"], + }) + );
108-116: Make shutdown idempotent; don’t throw when server wasn’t started; await close.Throwing on destroy() when
listen()never ran is hostile to graceful teardown.- if (this.serverInstance) { - // Close the Express server - this.serverInstance.close(() => { - logger.info("Express server closed gracefully."); - }); - } else { - logger.warn("Express server not found"); - throw new Error("Express server not found"); - } + if (this.httpServer && typeof this.httpServer.close === "function") { + await new Promise<void>((resolve) => this.httpServer!.close(() => resolve())); + logger.info("Express server closed gracefully."); + } else { + logger.warn("Express server was not started or is already closed."); + }
🧹 Nitpick comments (1)
apps/live/src/server.ts (1)
49-61: Minor hardening: disable X-Powered-By.Small security nit to reduce fingerprinting.
private setupMiddleware() { + this.app.disable("x-powered-by"); // Security middleware this.app.use(helmet());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/live/src/server.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/live/src/server.ts (7)
apps/live/src/env.ts (1)
env(36-36)apps/live/src/redis.ts (1)
redisManager(211-211)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(21-193)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)packages/decorators/src/controller.ts (1)
registerController(17-40)
⏰ 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)
apps/live/src/server.ts (2)
36-42: Good: Redis init is awaited before HocusPocus.This fixes the prior race.
25-34: Bootstrap initialization order is correct — in apps/live/src/start.ts, the code constructs the server, then awaitsserver.initialize()before callingserver.listen(), and there is no manualsetupRoutes()invocation. No further changes required.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/live/src/extensions/redis.ts (1)
6-17: Constructor requires an already-initialized client
getRedisClient()throws if the manager isn’t ready; coupled with eager construction this can fail at import time. Instantiate this class only after Redis is connected (see comment in extensions/index.ts).Confirm startup order ensures Redis connects before
new Redis()is called.#!/bin/bash rg -nP -C2 '\bredisManager\.(connect|init)\b' apps/live rg -nP -C2 'new\s+Redis\s*\(' apps/liveapps/live/src/lib/auth.ts (2)
17-39: Allow cookie-only auth; verify userId only when present in token.Prevents rejecting clients that send only Cookie headers.
export const onAuthenticate = async ({ requestHeaders, context, token }: any) => { let cookie: string | undefined = undefined; - let userId: string | undefined = undefined; + let userIdFromToken: string | undefined = undefined; @@ try { const parsedToken = JSON.parse(token) as TUserDetails; - userId = parsedToken.id; + userIdFromToken = parsedToken.id; cookie = parsedToken.cookie; } catch (error) { @@ - if (!cookie || !userId) { + if (!cookie) { throw new Error("Credentials not provided"); } @@ try { const userService = new UserService(); const user = await userService.currentUser(cookie); - if (user.id !== userId) { + if (userIdFromToken && user.id !== userIdFromToken) { throw new Error("Authentication unsuccessful!"); }Also applies to: 45-49
62-68: Guard unknown stateless events to avoid runtime errors.payload may not map to a known event; optional-chain the lookup.
export const onStateless = async ({ payload, document }: any) => { - // broadcast the client event (derived from the server event) to all the clients so that they can update their state - const response = DocumentCollaborativeEvents[payload as TDocumentEventsServer].client; - if (response) { - document.broadcastStateless(response); - } + const serverEvent = DocumentCollaborativeEvents[payload as TDocumentEventsServer]; + const response = serverEvent?.client; + if (response) document.broadcastStateless(response); };
🧹 Nitpick comments (9)
apps/live/src/extensions/logger.ts (1)
7-11: Preserve log structure/context instead of free-form stringsRoute logs with context so downstream processing can filter by component.
- log: (message) => { - logger.info(message); - }, + log: (message) => logger.info({ component: "live:hocuspocus", message }),apps/live/src/extensions/redis.ts (3)
23-24: Micro: drop unnecessary Buffer.concat allocation
s/Buffer.concat([Buffer.from([0])])/Buffer.from([0])/- const emptyPrefix = Buffer.concat([Buffer.from([0])]); + const emptyPrefix = Buffer.from([0]);
19-22: Guard input and stringify failuresValidate
documentNameand handle JSON.stringify errors (circular structures).- public broadcastToDocument(documentName: string, payload: any): Promise<number> { - const stringPayload = typeof payload === "string" ? payload : JSON.stringify(payload); + public broadcastToDocument(documentName: string, payload: unknown): Promise<number> { + if (!documentName) throw new Error("documentName is required"); + let stringPayload: string; + if (typeof payload === "string") stringPayload = payload; + else { + try { + stringPayload = JSON.stringify(payload); + } catch (e) { + throw new Error("Failed to serialize payload for broadcast"); + } + } const message = new OutgoingMessage(documentName).writeBroadcastStateless(stringPayload);
25-29: Avoid using internalpubKeyandpublishBuffer
Hocuspocus’s Redis extension doesn’t expose a public channel‐computing helper or officially supportpublishBuffer—vendor the channel‐key logic from your pinned version into a local helper to prevent breakage on future upgrades.apps/live/src/extensions/database.ts (3)
15-23: Don’t use exceptions for control flow on unsupported document types; warn and return null.Throwing here only to catch-and-null immediately below logs an error for a non-error path.
- if (documentType === "project_page") { - const data = await fetchPageDescriptionBinary(params, pageId, cookie); - return data; - } - throw new Error(`Invalid document type ${documentType} provided.`); + if (documentType === "project_page") { + const data = await fetchPageDescriptionBinary(params, pageId, cookie); + return data; + } + logger.warn(`Unsupported document type in fetch: ${documentType ?? "undefined"}`); + return null;
32-35: Mirror fetch-path behavior in store for unsupported types.Currently a no-op without visibility. Log and exit for consistency.
- if (documentType === "project_page") { - await updatePageDescription(params, pageId, state, cookie); - } + if (documentType === "project_page") { + await updatePageDescription(params, pageId, state, cookie); + } else { + logger.warn(`Unsupported document type in store: ${documentType ?? "undefined"}`); + }
8-8: Tighten handler typings (avoid any).Gives safer contracts for requestParameters/state and prevents accidental misuse.
-const onFetch = async ({ context, documentName: pageId, requestParameters }: any) => { +const onFetch = async ({ + context, + documentName: pageId, + requestParameters, +}: { context: unknown; documentName: string; requestParameters: URLSearchParams }) => {-const onStore = async ({ context, state, documentName: pageId, requestParameters }: any) => { +const onStore = async ({ + context, + state, + documentName: pageId, + requestParameters, +}: { context: unknown; state: Uint8Array; documentName: string; requestParameters: URLSearchParams }) => {Also applies to: 25-25
apps/live/src/lib/auth.ts (1)
17-17: Type the handler input instead of any.Keeps us honest about headers/token shapes.
-export const onAuthenticate = async ({ requestHeaders, context, token }: any) => { +export const onAuthenticate = async ({ + requestHeaders, + context, + token, +}: { requestHeaders: { cookie?: string }; context: unknown; token?: string }) => {apps/live/src/hocuspocus.ts (1)
34-49: Make initialize idempotent under concurrent calls (add in-flight promise guard).Prevents double configuration if initialize() is called concurrently.
export class HocusPocusServerManager { private static instance: HocusPocusServerManager | null = null; private server: Hocuspocus | null = null; private isInitialized: boolean = false; + private initializingPromise: Promise<Hocuspocus> | null = null; @@ - public async initialize(): Promise<Hocuspocus> { - if (this.isInitialized && this.server) { - return this.server; - } - - this.server = Server.configure({ - name: this.serverName, - onAuthenticate: onAuthenticate, - onStateless: onStateless, - extensions, - debounce: 10000, - }); - - this.isInitialized = true; - return this.server; - } + public async initialize(): Promise<Hocuspocus> { + if (this.server) return this.server; + if (this.initializingPromise) return this.initializingPromise; + this.initializingPromise = Promise.resolve( + Server.configure({ + name: this.serverName, + onAuthenticate, + onStateless, + extensions, + debounce: 10000, + }), + ).then((srv) => { + this.server = srv; + this.isInitialized = true; + return srv; + }).finally(() => { + this.initializingPromise = null; + }); + return this.initializingPromise; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/live/src/extensions/database.ts(1 hunks)apps/live/src/extensions/index.ts(1 hunks)apps/live/src/extensions/logger.ts(1 hunks)apps/live/src/extensions/redis.ts(1 hunks)apps/live/src/hocuspocus.ts(1 hunks)apps/live/src/lib/auth.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/live/src/extensions/index.ts (3)
apps/live/src/extensions/logger.ts (1)
Logger(4-13)apps/live/src/extensions/database.ts (1)
Database(40-44)apps/live/src/extensions/redis.ts (1)
Redis(14-31)
apps/live/src/extensions/database.ts (2)
apps/live/src/types/index.ts (2)
HocusPocusServerContext(3-5)TDocumentTypes(1-1)apps/live/src/lib/page.ts (2)
fetchPageDescriptionBinary(56-81)updatePageDescription(9-36)
apps/live/src/extensions/logger.ts (1)
packages/logger/src/config.ts (1)
logger(14-14)
apps/live/src/lib/auth.ts (3)
packages/editor/src/core/types/config.ts (1)
TUserDetails(35-40)apps/live/src/types/index.ts (1)
HocusPocusServerContext(3-5)packages/editor/src/core/constants/document-collaborative-events.ts (1)
DocumentCollaborativeEvents(1-8)
apps/live/src/extensions/redis.ts (1)
apps/live/src/redis.ts (1)
redisManager(211-211)
apps/live/src/hocuspocus.ts (3)
apps/live/src/env.ts (1)
env(36-36)apps/live/src/server.ts (1)
Server(19-118)apps/live/src/lib/auth.ts (2)
onAuthenticate(17-60)onStateless(62-68)
⏰ 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: Build and lint web apps
🔇 Additional comments (1)
apps/live/src/hocuspocus.ts (1)
39-45: LGTM on server configuration wiring.Clean separation of concerns; debounce and extension injection look good.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
apps/live/src/server.ts (5)
20-24: Type the HTTP server handle and the controller registration; dropanyUse Node’s HTTP server type and the decorators’
ControllerConstructor. Also guard route setup to run once and require a non-null HocusPocus.+import type { Server as HttpServer } from "http"; +import { registerController, type ControllerConstructor } from "@plane/decorators"; @@ - private httpServer: any; + private httpServer: HttpServer | null = null; + private routesRegistered = false; @@ - private setupRoutes() { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - CONTROLLERS.forEach((controller) => registerController(this.router, controller as any, [this.hocuspocusServer])); - } + private setupRoutes() { + if (this.routesRegistered) return; + if (!this.hocuspocusServer) { + throw new Error("HocusPocus server not initialized. Call initialize() before setting up routes."); + } + CONTROLLERS.forEach((controller) => + registerController(this.router, controller as ControllerConstructor, [this.hocuspocusServer!]) + ); + this.routesRegistered = true; + } @@ public listen() { - this.httpServer = this.app.listen(this.app.get("port"), () => { + this.httpServer = this.app.listen(this.app.get("port"), () => { logger.info(`Plane Live server has started at port ${this.app.get("port")}`); }); }Also applies to: 86-89, 91-94
63-75: Use validated env for CORS and avoid direct process.env accessStick to
env.CORS_ALLOWED_ORIGINSto keep config centralized and validated.private setupCors() { - const allowedOrigins = - process.env.CORS_ALLOWED_ORIGINS === "*" - ? "*" - : process.env.CORS_ALLOWED_ORIGINS?.split(",")?.map((s) => s.trim()) || []; + const allowedOrigins = + env.CORS_ALLOWED_ORIGINS === "*" + ? "*" + : env.CORS_ALLOWED_ORIGINS?.split(",")?.map((s) => s.trim()) || []; this.app.use( cors({ origin: allowedOrigins, credentials: true, methods: ["GET", "POST", "PUT", "DELETE", "OPTIONS"], allowedHeaders: ["Content-Type", "Authorization", "x-api-key"], }) ); }
108-117: Guard server close and don’t throw when it wasn’t started
destroy()should be idempotent; avoid throwing iflisten()never ran.- if (this.httpServer) { - // Close the Express server - this.httpServer.close(() => { - logger.info("Express server closed gracefully."); - }); - } else { - logger.warn("Express server not found"); - throw new Error("Express server not found"); - } + if (this.httpServer && typeof this.httpServer.close === "function") { + this.httpServer.close(() => { + logger.info("Express server closed gracefully."); + }); + } else { + logger.warn("Express server was not started or is already closed."); + }
26-33: Don’t register routes before deps are ready (null HocusPocus injected)
setupRoutes()runs in the constructor whilethis.hocuspocusServeris still null, so WS controllers receive a null server. Move route/404 registration to afterinitialize()completes and make it idempotent.constructor() { this.app = express(); expressWs(this.app); this.setupMiddleware(); this.router = express.Router(); this.app.set("port", env.PORT || 3000); this.app.use(env.LIVE_BASE_PATH, this.router); - this.setupRoutes(); - this.setupNotFoundHandler(); }And wire after successful init (see diff in Lines 36-47).
36-47: Initialize, then wire controllers and 404; add idempotency guardEnsure controllers see a live HocusPocus instance and avoid double registration.
public async initialize(): Promise<void> { try { await redisManager.initialize(); logger.info("Redis setup completed"); const manager = HocusPocusServerManager.getInstance(); this.hocuspocusServer = await manager.initialize(); logger.info("HocusPocus setup completed"); + // Register routes after deps are ready (idempotent) + this.setupRoutes(); + this.setupNotFoundHandler(); } catch (error) { logger.error("Failed to initialize live server dependencies:", error); throw error; } }
🧹 Nitpick comments (2)
apps/live/src/extensions/index.ts (1)
5-5: Gate Redis extension and type the factory to avoid crashes when Redis is unavailableIf Redis isn’t configured/initialized yet,
new Redis()will throw. Build the list defensively and add an explicit return type.+import type { Extension } from "@hocuspocus/server"; +import { logger } from "@plane/logger"; @@ -export const getExtensions = () => [new Logger(), new Database(), new Redis()]; +export const getExtensions = (): Extension[] => { + const ex: Extension[] = [new Logger(), new Database()]; + try { + ex.push(new Redis()); + } catch (err) { + logger.warn("Redis not configured/connected; starting without Redis extension"); + } + return ex; +}apps/live/src/hocuspocus.ts (1)
33-46: Make initialize() concurrency-safe to prevent double server configurationTwo concurrent calls can create two HocusPocus instances before
this.serveris set. Guard with aninitializingpromise.export class HocusPocusServerManager { private static instance: HocusPocusServerManager | null = null; private server: Hocuspocus | null = null; + private initializing: Promise<Hocuspocus> | null = null; @@ public async initialize(): Promise<Hocuspocus> { - if (this.server) { - return this.server; - } - - this.server = Server.configure({ - name: this.serverName, - onAuthenticate: onAuthenticate, - onStateless: onStateless, - extensions: getExtensions(), - debounce: 10000, - }); - - return this.server; + if (this.server) return this.server; + if (this.initializing) return this.initializing; + + this.initializing = (async () => { + const srv = Server.configure({ + name: this.serverName, + onAuthenticate, + onStateless, + extensions: getExtensions(), + debounce: 10000, + }); + this.server = srv; + this.initializing = null; + return srv; + })(); + return this.initializing; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/live/src/env.ts(1 hunks)apps/live/src/extensions/index.ts(1 hunks)apps/live/src/hocuspocus.ts(1 hunks)apps/live/src/lib/auth.ts(1 hunks)apps/live/src/server.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/live/src/env.ts
- apps/live/src/lib/auth.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/live/src/hocuspocus.ts (4)
apps/live/src/env.ts (1)
env(36-36)apps/live/src/server.ts (1)
Server(19-118)apps/live/src/lib/auth.ts (2)
onAuthenticate(17-60)onStateless(66-72)apps/live/src/extensions/index.ts (1)
getExtensions(5-5)
apps/live/src/extensions/index.ts (3)
apps/live/src/extensions/logger.ts (1)
Logger(4-13)apps/live/src/extensions/database.ts (1)
Database(40-44)apps/live/src/extensions/redis.ts (1)
Redis(14-31)
apps/live/src/server.ts (7)
apps/live/src/env.ts (1)
env(36-36)apps/live/src/redis.ts (1)
redisManager(211-211)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(10-62)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)packages/decorators/src/controller.ts (1)
registerController(17-40)
⏰ 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)
apps/live/src/extensions/index.ts (1)
1-5: Factory over eager init: good moveSwitching to a factory avoids module-load-time side effects and lets infra come up first.
apps/live/src/server.ts (1)
26-31: Good ordering: express-ws before creating/mounting routersThis prevents “router.ws is not a function” and ensures middlewares apply globally.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/live/src/services/api.service.ts (1)
25-30: Fix: APIService.get misclassifies axios config objects — include validateStatusThe proposed change is good but misses axios config keys used in the codebase (e.g. { validateStatus: null } in apps/web/core/services/user.service.ts). That would be treated as query params and break requests. Update apps/live/src/services/api.service.ts (get method) so isConfigLike also checks for validateStatus (and consider adding other common AxiosRequestConfig keys or using AxiosRequestConfig for the config parameter).
🧹 Nitpick comments (12)
apps/live/src/utils/document.ts (2)
6-11: Avoid mixing deep and top-level imports from @plane/editorYou’re importing helpers from "@plane/editor" while extensions come from "@plane/editor/lib" (see Line 12). Mixing entrypoints can duplicate bundles/types or break tree-shaking. Prefer importing both from the package entrypoint.
Replace the deep import at Line 12 with the top-level entrypoint:
import { CoreEditorExtensionsWithoutProps, DocumentEditorExtensionsWithoutProps } from "@plane/editor";
49-50: Optional: enforce exhaustiveness at compile-timeUse an assertNever helper for the union to catch future variant additions during compile-time.
Example:
const assertNever = (x: never): never => { throw new Error(`Invalid variant provided: ${x as string}`); }; // ... return assertNever(variant as never);apps/live/src/services/project-page/extended.service.ts (1)
4-6: Remove redundant constructorSince it only calls super() with no added logic, you can omit the constructor.
export class ProjectPageExtendedService extends ProjectPageCoreService { - constructor() { - super(); - } + // No-op: inherits everything from ProjectPageCoreService }apps/live/src/services/api.service.ts (1)
17-23: Harden header handling
- Return a shallow copy to avoid accidental external mutation.
- Consider a request interceptor to auto-attach headers instead of passing them at every callsite.
- getHeader() { - return this.header; - } + getHeader() { + return { ...this.header }; + }Interceptor sketch (optional):
this.axiosInstance.interceptors.request.use((config) => { config.headers = { ...(config.headers ?? {}), ...this.header }; return config; });apps/live/src/extensions/database.ts (1)
10-10: Type the handlers to catch shape drift at compile-timePrefer concrete handler types over
anyso misuse (e.g., wrongstatetype) is caught early.-const onFetch = async ({ context, documentName, requestParameters }: any) => { +const onFetch = async ({ + context, + documentName, + requestParameters, +}: { + context: any; // replace with your Hocuspocus context type + documentName: string; + requestParameters: URLSearchParams | Record<string, string>; +}) => { @@ -const onStore = async ({ context, state, documentName, requestParameters }: any) => { +const onStore = async ({ + context, + state, + documentName, + requestParameters, +}: { + context: any; + state: Uint8Array; + documentName: string; + requestParameters: URLSearchParams | Record<string, string>; +}) => {Also applies to: 30-30
apps/live/src/services/project-page/index.ts (3)
10-10: Strongly type params; make storeDocument async for symmetryTight types prevent runtime “Missing required fields.” round-trips and improve DX.
+type ProjectPageParams = { + workspaceSlug: string; + projectId: string; + pageId: string; + cookie: string; +}; @@ - async fetchDocument(params: any) { + async fetchDocument(params: ProjectPageParams) { @@ - storeDocument(params: any, data: Uint8Array) { + async storeDocument(params: ProjectPageParams, data: Uint8Array) {Also applies to: 33-33
25-27: Remove redundant truthy check; helper always returns Uint8Array
s.getBinaryDataFromHTMLStringreturns aUint8Array, so the condition is unnecessary.- const convertedBinaryData = getBinaryDataFromHTMLString(pageDetails.description_html ?? "<p></p>"); - if (convertedBinaryData) { - return convertedBinaryData; - } + const convertedBinaryData = getBinaryDataFromHTMLString(pageDetails.description_html ?? "<p></p>"); + return convertedBinaryData;
22-30: Optional: backfill binary on first-read to avoid repeat HTML fetchesIf empty binary is common, consider persisting the converted binary (idempotently) after the first fetch to reduce subsequent latency.
I can draft a guarded upsert that calls
updateDescriptionBinarywhenbyteLength === 0, behind a feature flag.apps/live/src/services/project-page/core.service.ts (4)
5-9: Narrow payload type
objectis too broad; prefer a safer structural type.export type TPageDescriptionPayload = { description_binary: string; description_html: string; - description: object; + description: Record<string, unknown>; };
26-33: Use Accept on GET instead of Content-TypeFor GET, advertise expected response via
Acceptheader;Content-Typeis for request bodies.return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/pages/${pageId}/description/`, { headers: { - ...this.getHeader(), - "Content-Type": "application/octet-stream", + ...this.getHeader(), + Accept: "application/octet-stream", }, responseType: "arraybuffer", })
26-26: Tighten return type for binary-fetchClarify the expected shape to help callers and tooling.
- protected async fetchDescriptionBinary(workspaceSlug: string, projectId: string, pageId: string): Promise<any> { + protected async fetchDescriptionBinary(workspaceSlug: string, projectId: string, pageId: string): Promise<ArrayBuffer> {
49-52: Normalize error shapeMatch other methods and propagate server payload when available.
- .catch((error) => { - throw error; - }); + .catch((error) => { + throw error?.response?.data ?? error; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/live/src/extensions/database.ts(1 hunks)apps/live/src/services/api.service.ts(1 hunks)apps/live/src/services/project-page/core.service.ts(2 hunks)apps/live/src/services/project-page/extended.service.ts(1 hunks)apps/live/src/services/project-page/index.ts(1 hunks)apps/live/src/utils/document.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/live/src/services/project-page/extended.service.ts (1)
apps/live/src/services/project-page/core.service.ts (1)
ProjectPageCoreService(11-54)
apps/live/src/extensions/database.ts (3)
apps/live/src/services/project-page/index.ts (1)
ProjectPageService(5-51)apps/live/src/types/index.ts (1)
TDocumentTypes(1-1)packages/logger/src/config.ts (1)
logger(14-14)
apps/live/src/services/project-page/index.ts (2)
apps/live/src/services/project-page/extended.service.ts (1)
ProjectPageExtendedService(3-7)apps/live/src/utils/document.ts (2)
getBinaryDataFromHTMLString(76-83)getAllDocumentFormatsFromBinaryData(52-74)
apps/live/src/services/project-page/core.service.ts (1)
packages/types/src/page/core.ts (1)
TPage(5-25)
apps/live/src/services/api.service.ts (1)
apps/live/src/env.ts (1)
env(36-36)
apps/live/src/utils/document.ts (3)
packages/editor/src/core/extensions/core-without-props.ts (1)
DocumentEditorExtensionsWithoutProps(105-105)packages/types/src/page/core.ts (1)
TDocumentPayload(66-70)packages/editor/src/core/helpers/yjs-utils.ts (4)
getBinaryDataFromRichTextEditorHTMLString(56-64)getAllDocumentFormatsFromRichTextEditorBinaryData(86-108)getBinaryDataFromDocumentEditorHTMLString(71-79)getAllDocumentFormatsFromDocumentEditorBinaryData(115-137)
⏰ 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 (5)
apps/live/src/utils/document.ts (2)
24-50: LGTM: clear, symmetric conversion for both variantsThe public API and payload mapping are consistent with TDocumentPayload. Error on invalid variant is appropriate.
76-83: Signature change is safe — no callers expect { contentBinary }Repo search found only apps/live/src/services/project-page/index.ts (line 24) calling getBinaryDataFromHTMLString; it returns/passes the Uint8Array directly. No occurrences of .contentBinary were found.
apps/live/src/services/api.service.ts (1)
8-15: LGTM: env-driven baseURLConstructor refactor to use env.API_BASE_URL and centralized axios instance looks good.
apps/live/src/extensions/database.ts (1)
24-28: Return null only for “not found”; rethrow unexpected errorsToday all failures look like “document missing” to callers. Consider returning null for 404/empty cases and rethrowing others.
Would you like me to adjust this with specific status checks after we confirm what the API returns on “no description” vs “auth” errors?
apps/live/src/services/project-page/index.ts (1)
1-1: Verify utils re-exportsConfirm that
@/utils(e.g.utils/index.ts) re-exports the named functionsgetAllDocumentFormatsFromBinaryDataandgetBinaryDataFromHTMLString; if not, add those re-exports or import directly from their defining module (e.g.utils/document.ts).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/live/src/extensions/database.ts (2)
33-36: Redact and structure error logs (avoid logging raw error objects).Raw errors can include headers/config. Log safe context and message only.
- } catch (error) { - logger.error("Error in fetching document", error); + } catch (error: any) { + logger.error("Error in fetching document", { + documentName, + message: error?.message ?? String(error), + status: error?.response?.status, + }); return null; } @@ - } catch (error) { - logger.error("Error in updating document:", error); + } catch (error: any) { + logger.error("Error in updating document", { + documentName, + message: error?.message ?? String(error), + status: error?.response?.status, + }); }Also applies to: 57-59
12-16: URLSearchParams spread yields empty object; cookie/documentType not propagated → service construction fails.This breaks getPageService (documentType undefined) and ProjectPageService (cookie missing). Normalize query params and plumb cookie from headers or qp.
- const params = { - ...context, - ...requestParameters, - pageId: documentName, - }; + const qp = + typeof requestParameters?.get === "function" + ? Object.fromEntries(requestParameters.entries()) + : { ...(requestParameters ?? {}) }; + const documentType = (qp.documentType ?? + requestParameters?.get?.("documentType")) as TDocumentTypes | undefined; + const params = { + workspaceSlug: qp.workspaceSlug, + projectId: qp.projectId, + pageId: documentName, + cookie: + context?.request?.headers?.cookie ?? + context?.requestHeaders?.cookie ?? + qp.cookie, + documentType, + }; @@ - const params = { - ...context, - ...requestParameters, - pageId: documentName, - data: state, - }; + const qp2 = + typeof requestParameters?.get === "function" + ? Object.fromEntries(requestParameters.entries()) + : { ...(requestParameters ?? {}) }; + const documentType2 = (qp2.documentType ?? + requestParameters?.get?.("documentType")) as TDocumentTypes | undefined; + const params = { + workspaceSlug: qp2.workspaceSlug, + projectId: qp2.projectId, + pageId: documentName, + data: state, + cookie: + context?.request?.headers?.cookie ?? + context?.requestHeaders?.cookie ?? + qp2.cookie, + documentType: documentType2, + };Also applies to: 41-46
🧹 Nitpick comments (5)
apps/live/src/services/page/handler.ts (1)
5-13: Harden documentType extraction (avoid toString and support URLSearchParams).Downstream callers (e.g., extensions/database.ts) may pass URLSearchParams; spreading it won’t surface keys, so documentType becomes undefined and this throws. Prefer a resilient extraction with a fallback to requestParameters.get.
-export const getPageService = (params: any) => { - const documentType = params["documentType"]?.toString() as TDocumentTypes | undefined; +export const getPageService = (params: any) => { + const documentType = (params?.documentType ?? + params?.requestParameters?.get?.("documentType")) as TDocumentTypes | undefined;apps/live/src/services/page/project-page.service.ts (1)
6-16: Encode path segments and normalize cookie.Encode workspaceSlug/projectId to avoid malformed paths and coerce cookie to string before setting the header.
- constructor(params: any) { + constructor(params: any) { super(); - const { workspaceSlug, projectId } = params; - if (!workspaceSlug || !projectId) throw new Error("Missing required fields."); + const { workspaceSlug, projectId, cookie } = params; + if (!workspaceSlug || !projectId) { + throw new Error("Missing required fields: workspaceSlug and projectId."); + } // validate cookie - if (!params.cookie) throw new Error("Cookie is required."); + if (!cookie) throw new Error("Cookie is required."); // set cookie - this.setHeader("Cookie", params.cookie); + this.setHeader("Cookie", String(cookie)); // set base path - this.basePath = `/api/workspaces/${workspaceSlug}/projects/${projectId}`; + this.basePath = `/api/workspaces/${encodeURIComponent( + workspaceSlug + )}/projects/${encodeURIComponent(projectId)}`; }apps/live/src/services/page/core.service.ts (2)
28-35: Use Accept on GET instead of Content-Type.Setting Content-Type on GET is unnecessary and occasionally problematic. Prefer Accept to indicate the expected payload.
- return this.get(`${this.basePath}/pages/${pageId}/description/`, { - headers: { - ...this.getHeader(), - "Content-Type": "application/octet-stream", - }, + return this.get(`${this.basePath}/pages/${pageId}/description/`, { + headers: { + ...this.getHeader(), + Accept: "application/octet-stream", + }, responseType: "arraybuffer", })
46-49: Normalize error shape across methods.Other methods rethrow error.response.data; updateDescriptionBinary throws the raw error. Align to avoid leaking transport internals.
- .catch((error) => { - throw error; - }); + .catch((error) => { + throw error?.response?.data ?? error; + });apps/live/src/extensions/database.ts (1)
21-23: Avoid unnecessary copy; support Buffer/Uint8Array directly.Axios may return Buffer (subclass of Uint8Array) in Node. Don’t always allocate a new Uint8Array.
- const response = await service.fetchDescriptionBinary(params.pageId); - const binaryData = new Uint8Array(response); + const response = await service.fetchDescriptionBinary(params.pageId); + const binaryData = + response instanceof Uint8Array ? response : new Uint8Array(response);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/live/src/extensions/database.ts(1 hunks)apps/live/src/services/page/core.service.ts(1 hunks)apps/live/src/services/page/handler.ts(1 hunks)apps/live/src/services/page/project-page.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/live/src/services/page/project-page.service.ts (1)
apps/space/core/store/publish/publish.store.ts (1)
workspaceSlug(93-95)
apps/live/src/services/page/handler.ts (2)
apps/live/src/types/index.ts (1)
TDocumentTypes(1-1)apps/live/src/services/page/project-page.service.ts (1)
ProjectPageService(3-17)
apps/live/src/services/page/core.service.ts (1)
packages/types/src/page/core.ts (1)
TPage(5-25)
apps/live/src/extensions/database.ts (3)
apps/live/src/services/page/handler.ts (1)
getPageService(5-13)apps/live/src/utils/document.ts (2)
getBinaryDataFromHTMLString(76-83)getAllDocumentFormatsFromBinaryData(52-74)packages/logger/src/config.ts (1)
logger(14-14)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
apps/live/src/extensions/database.ts (4)
29-31: Don’t log raw Error objects; redact and structure logsRaw errors may include headers. Log minimal, structured context.
Apply:
- } catch (error) { - logger.error("Error in fetching document", error); + } catch (error: any) { + logger.error("Error in fetching document", { + documentName, + message: error?.message ?? String(error), + }); return null; }
53-55: Structure logs in store catch as wellMirror the fetch-side redaction.
Apply:
- } catch (error) { - logger.error("Error in updating document:", error); + } catch (error: any) { + logger.error("Error in updating document", { + documentName, + message: error?.message ?? String(error), + }); }
10-16: Don’t spread URLSearchParams; plumb documentType and cookie (current code always fails getPageService)
...requestParametersyields no keys;documentTypeandcookiearen’t set, sogetPageServicethrows and fetch returns null.Apply:
- const params = { - ...context, - ...requestParameters, - pageId: documentName, - }; + const qp = + typeof requestParameters?.get === "function" + ? Object.fromEntries(requestParameters.entries()) + : { ...(requestParameters ?? {}) }; + const documentType = qp.documentType ?? requestParameters?.get?.("documentType"); + const params = { + workspaceSlug: qp.workspaceSlug, + projectId: qp.projectId, + documentType, + pageId: documentName, + // Prefer header cookie; fall back to query param if explicitly provided + cookie: + (context as any)?.request?.headers?.cookie ?? + (context as any)?.requestHeaders?.cookie ?? + qp.cookie, + }; + if (!documentType) return null;
37-43: Repeat: normalize params for store; include documentType and cookieSame issue as fetch; store will also fail to resolve the service.
Apply:
- const params = { - ...context, - ...requestParameters, - pageId: documentName, - data: state, - }; + const qp = + typeof requestParameters?.get === "function" + ? Object.fromEntries(requestParameters.entries()) + : { ...(requestParameters ?? {}) }; + const documentType = qp.documentType ?? requestParameters?.get?.("documentType"); + const params = { + workspaceSlug: qp.workspaceSlug, + projectId: qp.projectId, + documentType, + pageId: documentName, + data: state, + cookie: + (context as any)?.request?.headers?.cookie ?? + (context as any)?.requestHeaders?.cookie ?? + qp.cookie, + }; + if (!documentType) return;
🧹 Nitpick comments (4)
apps/live/src/services/page/extended.service.ts (2)
3-7: Docstring overpromises; clarify intentThe class currently adds no functionality. Reword to reflect that this is an EE override hook/base.
9-11: Remove redundant constructorAbstract classes don’t need an explicit no-op constructor; TypeScript will synthesize super() in subclasses.
Apply:
-export abstract class PageService extends PageCoreService { - constructor() { - super(); - } -} +export abstract class PageService extends PageCoreService {}apps/live/src/extensions/database.ts (2)
45-53: store should be fire-and-forget; don’t return service resultHocuspocus Database store hook expects void; returning the service’s promise isn’t used.
Apply:
- return service.updateDescriptionBinary(params.pageId, payload); + await service.updateDescriptionBinary(params.pageId, payload);
1-7: Address “Unexpected any” by adding local payload typesSatisfy lint and document shapes without pulling external types.
Apply:
import { Database as HocuspocusDatabase } from "@hocuspocus/extension-database"; import { logger } from "@plane/logger"; // lib import { getPageService } from "@/services/page/handler"; // utils import { getAllDocumentFormatsFromBinaryData, getBinaryDataFromHTMLString } from "@/utils"; +type RequestParams = URLSearchParams | Record<string, string | undefined>; +type LiveFetchArgs = { context: Record<string, unknown>; documentName: string; requestParameters: RequestParams }; +type LiveStoreArgs = LiveFetchArgs & { state: Uint8Array }; -const fetchDocument = async ({ context, documentName, requestParameters }: any) => { +const fetchDocument = async ({ context, documentName, requestParameters }: LiveFetchArgs) => { @@ -const storeDocument = async ({ context, state, documentName, requestParameters }: any) => { +const storeDocument = async ({ context, state, documentName, requestParameters }: LiveStoreArgs) => {Also applies to: 8-8, 35-35
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/live/src/extensions/database.ts(1 hunks)apps/live/src/services/page/extended.service.ts(1 hunks)apps/live/src/services/page/project-page.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/live/src/services/page/project-page.service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/live/src/extensions/database.ts (3)
apps/live/src/services/page/handler.ts (1)
getPageService(5-13)apps/live/src/utils/document.ts (2)
getBinaryDataFromHTMLString(76-83)getAllDocumentFormatsFromBinaryData(52-74)packages/logger/src/config.ts (1)
logger(14-14)
🪛 GitHub Check: Build and lint web apps
apps/live/src/extensions/database.ts
[warning] 35-35:
Unexpected any. Specify a different type
[warning] 8-8:
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)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/live/src/server.ts (3)
24-24: Add idempotency guard for route registration.Prevents double-registration if initialize() is called more than once (e.g., tests).
Apply:
private httpServer: HttpServer | null = null; + private routesRegistered = false;
89-91: Make setupRoutes idempotent.Guard and flip the flag after first registration.
Apply:
- private setupRoutes(hocuspocusServer: Hocuspocus) { - CONTROLLERS.forEach((controller) => registerController(this.router, controller, [hocuspocusServer])); - } + private setupRoutes(hocuspocusServer: Hocuspocus) { + if (this.routesRegistered) return; + CONTROLLERS.forEach((controller) => registerController(this.router, controller, [hocuspocusServer])); + this.routesRegistered = true; + }
110-118: Await HTTP server close and don’t throw when not started.Ensure destroy() resolves after the server is closed; avoid throwing when listen() never ran.
Apply:
- if (this.httpServer) { - // Close the Express server - this.httpServer.close(() => { - logger.info("Express server closed gracefully."); - }); - } else { - logger.warn("Express server not found"); - throw new Error("Express server not found"); - } + if (this.httpServer) { + // Close the Express server and wait for completion + await new Promise<void>((resolve) => { + this.httpServer!.close(() => { + logger.info("Express server closed gracefully."); + resolve(); + }); + }); + } else { + logger.warn("Express server was not started or is already closed."); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/live/src/server.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/live/src/server.ts (7)
apps/live/src/env.ts (1)
env(36-36)apps/live/src/redis.ts (1)
redisManager(211-211)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(10-62)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)packages/decorators/src/controller.ts (1)
registerController(17-40)
⏰ 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 (3)
apps/live/src/server.ts (3)
26-33: Good lifecycle and ordering (express-ws → middleware → router).Router is created after express-ws and after global middlewares. Looks correct.
35-46: Initialize sequencing LGTM.Redis awaited before HocusPocus, then routes and 404 after deps ready. Solid.
66-71: Use validated env instead of process.env for CORS origins.Leverage the parsed
envfor consistency with the rest of the server config.Apply:
- const allowedOrigins = - process.env.CORS_ALLOWED_ORIGINS === "*" - ? "*" - : process.env.CORS_ALLOWED_ORIGINS?.split(",")?.map((s) => s.trim()) || []; + const allowedOrigins = + env.CORS_ALLOWED_ORIGINS === "*" + ? "*" + : env.CORS_ALLOWED_ORIGINS.split(",").map((s) => s.trim());
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/live/src/lib/auth.ts (1)
52-52: Remove redundant cookie fallback.Line 52 assigns
cookie ?? requestParameters.get("cookie") ?? "". However,cookieis already validated as truthy on Line 47, making the null-coalescing chain redundant. TherequestParameters.get("cookie")fallback will never be reached.Simplify:
- context.cookie = cookie ?? requestParameters.get("cookie") ?? ""; + context.cookie = cookie;apps/live/src/services/page/core.service.ts (1)
28-40: ReplacePromise<any>with a proper type forfetchDescriptionBinary.Returning
Promise<any>bypasses TypeScript's type safety. Based on theresponseType: "arraybuffer", the return type should bePromise<ArrayBuffer>.Apply this diff:
- async fetchDescriptionBinary(pageId: string): Promise<any> { + async fetchDescriptionBinary(pageId: string): Promise<ArrayBuffer> { return this.get(`${this.basePath}/pages/${pageId}/description/`, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/live/src/extensions/database.ts(1 hunks)apps/live/src/hocuspocus.ts(1 hunks)apps/live/src/lib/auth.ts(1 hunks)apps/live/src/lib/stateless.ts(1 hunks)apps/live/src/server.ts(2 hunks)apps/live/src/services/api.service.ts(1 hunks)apps/live/src/services/page/core.service.ts(1 hunks)apps/live/src/services/page/handler.ts(1 hunks)apps/live/src/services/page/project-page.service.ts(1 hunks)apps/live/src/types/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/live/src/services/page/handler.ts
- apps/live/src/services/page/project-page.service.ts
🧰 Additional context used
🧬 Code graph analysis (7)
apps/live/src/extensions/database.ts (3)
apps/live/src/types/index.ts (2)
FetchPayloadWithContext(23-25)StorePayloadWithContext(27-29)apps/live/src/services/page/handler.ts (1)
getPageService(5-15)packages/editor/src/core/helpers/yjs-utils.ts (2)
getBinaryDataFromDocumentEditorHTMLString(71-79)getAllDocumentFormatsFromDocumentEditorBinaryData(115-137)
apps/live/src/services/page/core.service.ts (1)
packages/types/src/page/core.ts (1)
TPage(5-25)
apps/live/src/hocuspocus.ts (3)
apps/live/src/env.ts (1)
env(36-36)apps/live/src/server.ts (1)
Server(20-117)apps/live/src/extensions/index.ts (1)
getExtensions(5-5)
apps/live/src/lib/stateless.ts (2)
packages/editor/src/core/constants/document-collaborative-events.ts (1)
DocumentCollaborativeEvents(1-8)apps/live/src/core/hocuspocus-server.ts (1)
onStateless(59-65)
apps/live/src/services/api.service.ts (1)
apps/live/src/env.ts (1)
env(36-36)
apps/live/src/server.ts (7)
apps/live/src/env.ts (1)
env(36-36)apps/live/src/redis.ts (1)
redisManager(211-211)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(11-63)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)packages/decorators/src/controller.ts (1)
registerController(17-40)
apps/live/src/lib/auth.ts (3)
apps/live/src/types/index.ts (2)
HocusPocusServerContext(6-12)TDocumentTypes(3-3)packages/editor/src/core/types/config.ts (1)
TUserDetails(35-40)packages/logger/src/config.ts (1)
logger(14-14)
⏰ 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: Cursor Bugbot
🔇 Additional comments (14)
apps/live/src/types/index.ts (1)
1-29: LGTM! Type definitions are well-structured.The type definitions correctly model the Hocuspocus context and payload extensions. Nullable fields (
projectId,workspaceSlug) align with how they're populated fromURLSearchParams.get()in the authentication flow.apps/live/src/lib/auth.ts (2)
64-82: LGTM! Authentication validation is secure.The user ID cross-check (Line 69-70) prevents token/cookie mismatch attacks. Error handling correctly obscures internal details from clients.
47-49: Requiring bothcookieanduserIdis safe—no cookie-only auth flows found.
I searched the codebase for WebSocket connections using only a cookie-based token and found no matches. Remove this guard only if you introduce any cookie-only authentication paths elsewhere; otherwise leave it as is.apps/live/src/extensions/database.ts (3)
14-34: Critical issues in fetchDocument require fixes.Multiple issues persist:
Buffer conversion error (Line 19):
new Uint8Array(response)will corrupt data ifservice.fetchDescriptionBinaryreturns a base64 string or Node.js Buffer.Missing null-guard (Line 23): Accessing
pageDetails.description_htmlwithout checking ifpageDetailsis null/undefined will crash.Error logging exposes sensitive data (Lines 30-32): Logging raw errors may leak headers, tokens, or other sensitive information.
Apply this diff to address all three issues:
try { const service = getPageService(context.documentType, context); // fetch details const response = await service.fetchDescriptionBinary(pageId); - const binaryData = new Uint8Array(response); + const binaryData: Uint8Array = + typeof response === "string" + ? Uint8Array.from(Buffer.from(response, "base64")) + : response instanceof Uint8Array + ? response + : Buffer.isBuffer(response) + ? new Uint8Array(response) + : new Uint8Array(0); // if binary data is empty, convert HTML to binary data if (binaryData.byteLength === 0) { const pageDetails = await service.fetchDetails(pageId); + if (!pageDetails) { + return new Uint8Array(0); + } const convertedBinaryData = getBinaryDataFromDocumentEditorHTMLString(pageDetails.description_html ?? "<p></p>"); if (convertedBinaryData) { return convertedBinaryData; } } // return binary data return binaryData; - } catch (error) { - logger.error("Error in fetching document", error); + } catch (error: any) { + logger.error("Error in fetching document", { + pageId, + documentType: context.documentType, + message: error?.message ?? String(error), + }); throw error; }
36-53: Sanitize error logging in storeDocument.Raw error objects may contain sensitive headers or authentication tokens. Log only sanitized metadata.
Apply this diff:
} catch (error: any) { - logger.error("Error in updating document:", error); + logger.error("Error in updating document", { + pageId, + documentType: context.documentType, + message: error?.message ?? String(error), + }); throw error; }
14-16: cookie is properly propagated in onAuthenticate and passed to ProjectPageService; no further action required.apps/live/src/hocuspocus.ts (1)
34-48: Remove the Redis initialization warning.apps/live/src/server.tsawaitsredisManager.initialize()(line 37) before callingHocusPocusServerManager.initialize(), sogetRedisClient()in the Redis extension constructor will always be ready.Likely an incorrect or invalid review comment.
apps/live/src/server.ts (3)
35-49: LGTM! Async initialization properly sequences dependencies.The
initialize()method correctly awaits Redis before HocusPocus, ensuring the Redis client is ready when extensions need it. Route setup happens after all dependencies are initialized, preventing null injection issues.
26-33: Constructor lifecycle is correctly ordered.The constructor now applies
expressWsbefore creating the router, registers middleware before mounting routes, and defers route/404 setup toinitialize(). This resolves the previous ordering issues.
69-74: CORS wildcard with credentials is invalid and will be rejected by browsers.The current configuration sets
origin: "*"(whenCORS_ALLOWED_ORIGINSis"*") combined withcredentials: true. This violates the CORS specification—browsers will reject responses with this combination.Apply this fix:
this.app.use( cors({ - origin: allowedOrigins, - credentials: true, + origin: allowedOrigins === "*" ? true : allowedOrigins, + credentials: allowedOrigins !== "*", methods: ["GET", "POST", "PUT", "DELETE", "OPTIONS"], allowedHeaders: ["Content-Type", "Authorization", "x-api-key"], }) );When
allowedOriginsis"*", useorigin: true(reflect request origin) and disablecredentials. For explicit allow-lists, keepcredentials: trueand pass the array.apps/live/src/services/api.service.ts (2)
9-16: LGTM! Constructor properly uses validated env.The constructor now defaults to
env.API_BASE_URLfrom the validated environment configuration, eliminating the need for dotenv and ensuring type-safe config access.
7-24: Header injection pattern confirmed intentional
Service methods in apps/live/src/services/page/core.service.ts already spreadthis.getHeader()into each request config, indicating manual header management is by design. If you’d rather automate merging, add the interceptor shown in the original comment.apps/live/src/services/page/core.service.ts (2)
52-98: LGTM! AbortSignal handling is robust.The
updatePagePropertiesmethod correctly implements abort support with:
- Early abort check before making the request
- Promise.race pattern with abort listener
- Proper cleanup in the finally block
- Correct DOMException wrapping for AbortError
This provides a solid foundation for cancellable requests.
14-16: Remove the empty constructor.The constructor only calls
super()with no arguments, which is the default behavior. Per the past review comment, empty constructors should be removed.Apply this diff:
export abstract class PageCoreService extends APIService { protected abstract basePath: string; - constructor() { - super(); - } - async fetchDetails(pageId: string): Promise<TPage> {
|
|
||
| // set cookie in context, so it can be used throughout the ws connection | ||
| context.cookie = cookie ?? requestParameters.get("cookie") ?? ""; | ||
| context.documentType = requestParameters.get("documentType")?.toString() as TDocumentTypes; |
There was a problem hiding this comment.
Validate documentType before casting.
requestParameters.get("documentType") may return null, and the unsafe cast to TDocumentTypes could assign null to a field typed as non-nullable.
Add validation:
- context.documentType = requestParameters.get("documentType")?.toString() as TDocumentTypes;
+ const docType = requestParameters.get("documentType")?.toString();
+ if (!docType || (docType !== "project_page")) {
+ throw new Error("Invalid or missing documentType");
+ }
+ context.documentType = docType as TDocumentTypes;📝 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.
| context.documentType = requestParameters.get("documentType")?.toString() as TDocumentTypes; | |
| // Validate and parse documentType from the query parameters | |
| const docType = requestParameters.get("documentType")?.toString(); | |
| if (!docType || docType !== "project_page") { | |
| throw new Error("Invalid or missing documentType"); | |
| } | |
| context.documentType = docType as TDocumentTypes; |
🤖 Prompt for AI Agents
In apps/live/src/lib/auth.ts around line 53,
requestParameters.get("documentType") is cast directly to TDocumentTypes even
though get(...) can return null; validate the value before casting by retrieving
it into a const, checking for null/undefined, and confirming it is one of the
allowed TDocumentTypes (or use a type guard/enum lookup); if invalid, handle it
explicitly (throw a clear error, return a default, or set context.documentType
to a safe fallback) instead of performing the unsafe cast.
| export const onStateless = async ({ payload, document }: onStatelessPayload) => { | ||
| const response = DocumentCollaborativeEvents[payload as TDocumentEventsServer].client; | ||
| if (response) { | ||
| document.broadcastStateless(response); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Guard against unknown stateless events to prevent runtime errors.
The lookup DocumentCollaborativeEvents[payload as TDocumentEventsServer] may return undefined if payload is not a valid key. Accessing .client on undefined will throw.
Apply this diff to add optional chaining:
export const onStateless = async ({ payload, document }: onStatelessPayload) => {
- const response = DocumentCollaborativeEvents[payload as TDocumentEventsServer].client;
+ const response = DocumentCollaborativeEvents[payload as TDocumentEventsServer]?.client;
if (response) {
document.broadcastStateless(response);
}
};📝 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 const onStateless = async ({ payload, document }: onStatelessPayload) => { | |
| const response = DocumentCollaborativeEvents[payload as TDocumentEventsServer].client; | |
| if (response) { | |
| document.broadcastStateless(response); | |
| } | |
| }; | |
| export const onStateless = async ({ payload, document }: onStatelessPayload) => { | |
| const response = DocumentCollaborativeEvents[payload as TDocumentEventsServer]?.client; | |
| if (response) { | |
| document.broadcastStateless(response); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In apps/live/src/lib/stateless.ts around lines 8 to 13, the lookup into
DocumentCollaborativeEvents can return undefined and accessing .client will
throw; change the lookup to use optional chaining when retrieving .client from
DocumentCollaborativeEvents[payload] and keep the existing if (response) guard
so you only call document.broadcastStateless when response is defined; ensure
the payload is still cast to the appropriate event type if needed and no direct
property access happens on a possibly undefined lookup.
| public async destroy() { | ||
| // Close the HocusPocus server WebSocket connections | ||
| await this.hocuspocusServer.destroy(); | ||
| manualLogger.info("HocusPocus server WebSocket connections closed gracefully."); | ||
| // Close the Express server | ||
| this.serverInstance.close(() => { | ||
| manualLogger.info("Express server closed gracefully."); | ||
| process.exit(1); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| const server = new Server(); | ||
| server.listen(); | ||
| if (this.hocuspocusServer) { | ||
| await this.hocuspocusServer.destroy(); | ||
| logger.info("HocusPocus server WebSocket connections closed gracefully."); | ||
| } | ||
|
|
||
| // Graceful shutdown on unhandled rejection | ||
| process.on("unhandledRejection", async (err: any) => { | ||
| manualLogger.info("Unhandled Rejection: ", err); | ||
| manualLogger.info(`UNHANDLED REJECTION! 💥 Shutting down...`); | ||
| await server.destroy(); | ||
| }); | ||
| // Disconnect Redis | ||
| await redisManager.disconnect(); | ||
| logger.info("Redis connection closed gracefully."); | ||
|
|
||
| // Graceful shutdown on uncaught exception | ||
| process.on("uncaughtException", async (err: any) => { | ||
| manualLogger.info("Uncaught Exception: ", err); | ||
| manualLogger.info(`UNCAUGHT EXCEPTION! 💥 Shutting down...`); | ||
| await server.destroy(); | ||
| }); | ||
| if (this.httpServer) { | ||
| // Close the Express server | ||
| this.httpServer.close(() => { | ||
| logger.info("Express server closed gracefully."); | ||
| }); | ||
| } else { | ||
| logger.warn("Express server not found"); | ||
| throw new Error("Express server not found"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
HTTP server close should be promisified for proper async cleanup.
The destroy() method is async but doesn't await the HTTP server's close() callback, which means the method may return before the server actually closes. This could cause race conditions during shutdown.
Apply this diff:
public async destroy() {
// Close the HocusPocus server WebSocket connections
if (this.hocuspocusServer) {
await this.hocuspocusServer.destroy();
logger.info("HocusPocus server WebSocket connections closed gracefully.");
}
// Disconnect Redis
await redisManager.disconnect();
logger.info("Redis connection closed gracefully.");
if (this.httpServer) {
// Close the Express server
- this.httpServer.close(() => {
- logger.info("Express server closed gracefully.");
- });
+ await new Promise<void>((resolve, reject) => {
+ this.httpServer!.close((err) => {
+ if (err) {
+ logger.error("Error closing Express server:", err);
+ reject(err);
+ } else {
+ logger.info("Express server closed gracefully.");
+ resolve();
+ }
+ });
+ });
} else {
logger.warn("Express server not found");
throw new Error("Express server not found");
}
}📝 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.
| public async destroy() { | |
| // Close the HocusPocus server WebSocket connections | |
| await this.hocuspocusServer.destroy(); | |
| manualLogger.info("HocusPocus server WebSocket connections closed gracefully."); | |
| // Close the Express server | |
| this.serverInstance.close(() => { | |
| manualLogger.info("Express server closed gracefully."); | |
| process.exit(1); | |
| }); | |
| } | |
| } | |
| const server = new Server(); | |
| server.listen(); | |
| if (this.hocuspocusServer) { | |
| await this.hocuspocusServer.destroy(); | |
| logger.info("HocusPocus server WebSocket connections closed gracefully."); | |
| } | |
| // Graceful shutdown on unhandled rejection | |
| process.on("unhandledRejection", async (err: any) => { | |
| manualLogger.info("Unhandled Rejection: ", err); | |
| manualLogger.info(`UNHANDLED REJECTION! 💥 Shutting down...`); | |
| await server.destroy(); | |
| }); | |
| // Disconnect Redis | |
| await redisManager.disconnect(); | |
| logger.info("Redis connection closed gracefully."); | |
| // Graceful shutdown on uncaught exception | |
| process.on("uncaughtException", async (err: any) => { | |
| manualLogger.info("Uncaught Exception: ", err); | |
| manualLogger.info(`UNCAUGHT EXCEPTION! 💥 Shutting down...`); | |
| await server.destroy(); | |
| }); | |
| if (this.httpServer) { | |
| // Close the Express server | |
| this.httpServer.close(() => { | |
| logger.info("Express server closed gracefully."); | |
| }); | |
| } else { | |
| logger.warn("Express server not found"); | |
| throw new Error("Express server not found"); | |
| } | |
| } | |
| public async destroy() { | |
| // Close the HocusPocus server WebSocket connections | |
| if (this.hocuspocusServer) { | |
| await this.hocuspocusServer.destroy(); | |
| logger.info("HocusPocus server WebSocket connections closed gracefully."); | |
| } | |
| // Disconnect Redis | |
| await redisManager.disconnect(); | |
| logger.info("Redis connection closed gracefully."); | |
| if (this.httpServer) { | |
| // Close the Express server | |
| await new Promise<void>((resolve, reject) => { | |
| this.httpServer!.close((err) => { | |
| if (err) { | |
| logger.error("Error closing Express server:", err); | |
| reject(err); | |
| } else { | |
| logger.info("Express server closed gracefully."); | |
| resolve(); | |
| } | |
| }); | |
| }); | |
| } else { | |
| logger.warn("Express server not found"); | |
| throw new Error("Express server not found"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/live/src/server.ts around lines 96 to 116, the async destroy() method
calls this.httpServer.close(callback) but does not await its completion, risking
premature return; change the HTTP server shutdown to return a Promise and await
it (e.g., wrap this.httpServer.close in a new Promise that resolves on close and
rejects on error), then await that Promise before logging and exiting so the
destroy() method only completes after the server is fully closed.
| async updateDescriptionBinary(pageId: string, data: TPageDescriptionPayload): Promise<any> { | ||
| return this.patch(`${this.basePath}/pages/${pageId}/description/`, data, { | ||
| headers: this.getHeader(), | ||
| }) | ||
| .then((response) => response?.data) | ||
| .catch((error) => { | ||
| throw error; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling: updateDescriptionBinary should rethrow error?.response?.data.
All other methods in this class rethrow error?.response?.data to expose API error details, but updateDescriptionBinary rethrows the raw error. This inconsistency complicates error handling for callers.
Apply this diff:
async updateDescriptionBinary(pageId: string, data: TPageDescriptionPayload): Promise<any> {
return this.patch(`${this.basePath}/pages/${pageId}/description/`, data, {
headers: this.getHeader(),
})
.then((response) => response?.data)
.catch((error) => {
- throw error;
+ throw error?.response?.data;
});
}📝 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.
| async updateDescriptionBinary(pageId: string, data: TPageDescriptionPayload): Promise<any> { | |
| return this.patch(`${this.basePath}/pages/${pageId}/description/`, data, { | |
| headers: this.getHeader(), | |
| }) | |
| .then((response) => response?.data) | |
| .catch((error) => { | |
| throw error; | |
| }); | |
| } | |
| async updateDescriptionBinary(pageId: string, data: TPageDescriptionPayload): Promise<any> { | |
| return this.patch(`${this.basePath}/pages/${pageId}/description/`, data, { | |
| headers: this.getHeader(), | |
| }) | |
| .then((response) => response?.data) | |
| .catch((error) => { | |
| throw error?.response?.data; | |
| }); | |
| } |
🤖 Prompt for AI Agents
In apps/live/src/services/page/core.service.ts around lines 100 to 108, the
catch block currently rethrows the raw error which is inconsistent with other
methods; change the catch to rethrow error?.response?.data (i.e., throw
error?.response?.data) so callers receive the API error payload; ensure the
Promise rejection shape matches other methods in the class and keep the rest of
the method (headers/patch call) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/live/src/lib/auth.ts (2)
52-52: Remove redundant fallback logic.At line 47, you've already validated that
cookieis truthy. The?? requestParameters.get("cookie") ?? ""fallback on line 52 is redundant.Apply this diff:
- context.cookie = cookie ?? requestParameters.get("cookie") ?? ""; + context.cookie = cookie;
64-82: Consider logging the authentication error for observability.The catch block discards the error details (
_error), making debugging authentication failures difficult. While the generic error message is good for security, consider logging the actual error internally.Apply this diff:
} catch (_error) { + logger.error("Authentication failed:", { userId, error: _error }); throw Error("Authentication unsuccessful!"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/live/src/lib/auth.ts(1 hunks)apps/live/src/lib/stateless.ts(1 hunks)apps/live/src/types/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/live/src/lib/stateless.ts
- apps/live/src/types/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/live/src/lib/auth.ts (3)
apps/live/src/types/index.ts (2)
HocusPocusServerContext(23-29)TDocumentTypes(20-20)packages/editor/src/core/types/config.ts (1)
TUserDetails(35-40)packages/logger/src/config.ts (1)
logger(14-14)
⏰ 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: Cursor Bugbot
🔇 Additional comments (2)
apps/live/src/lib/auth.ts (2)
8-8: LGTM! Import issue resolved.The
TDocumentTypesimport that was previously missing has been added correctly.
53-53: Validate documentType before casting.
requestParameters.get("documentType")may returnnull, and the unsafe cast toTDocumentTypescould assign an invalid value. This issue was flagged in a previous review but remains unresolved.Add validation:
- context.documentType = requestParameters.get("documentType")?.toString() as TDocumentTypes; + const docType = requestParameters.get("documentType")?.toString(); + if (!docType || docType !== "project_page") { + throw new Error("Invalid or missing documentType"); + } + context.documentType = docType as TDocumentTypes;
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/live/src/server.ts (1)
69-74: CORS wildcard with credentials is still invalid.The configuration uses
origin: "*"withcredentials: true, which browsers reject per the CORS spec. Whencredentials: true, the origin must be explicit—wildcards are not allowed.Apply this fix:
this.app.use( cors({ - origin: allowedOrigins, + origin: allowedOrigins === "*" ? true : allowedOrigins, - credentials: true, + credentials: allowedOrigins !== "*", methods: ["GET", "POST", "PUT", "DELETE", "OPTIONS"], allowedHeaders: ["Content-Type", "Authorization", "x-api-key"], }) );Using
origin: truereflects the request'sOriginheader when wildcards are intended, and disabling credentials for wildcards ensures the CORS response is valid.
🧹 Nitpick comments (1)
apps/live/package.json (1)
10-12: Consider adding explicit Node version requirement.The
startanddevscripts usenode --env-file=.env, which requires Node.js 20.11.0 or later. Without anenginesfield, users on older Node versions will encounter runtime errors.Add to package.json:
"engines": { "node": ">=20.11.0" },This makes the Node version requirement explicit and allows package managers to warn users early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/live/package.json(4 hunks)apps/live/src/server.ts(1 hunks)apps/live/tsdown.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/live/src/server.ts (7)
apps/live/src/env.ts (1)
env(36-36)apps/live/src/redis.ts (1)
redisManager(211-211)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(11-63)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)packages/decorators/src/controller.ts (1)
registerController(17-40)
⏰ 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: Cursor Bugbot
🔇 Additional comments (6)
apps/live/src/server.ts (3)
35-49: Good async initialization flow.The refactored
initialize()properly awaits Redis before HocusPocus, and registers routes only after dependencies are ready. This fixes the race conditions and null-injection issues flagged in previous reviews.
110-120: HTTP server close is now properly promisified.Wrapping
this.httpServer!.close()in a Promise ensuresdestroy()waits for the server to fully shut down before returning. Good async cleanup pattern.
26-33: Constructor order looks correct.Applying
expressWsbefore creating the router and mounting it after middleware ensures WebSocket support and proper middleware application. The reordering from past reviews is correctly implemented.apps/live/tsdown.config.ts (1)
4-4: Entry point change aligns with new startup flow.Switching from
src/server.tstosrc/start.tscorrectly reflects the refactored bootstrap pattern wherestart.tsorchestrates the server lifecycle.apps/live/package.json (2)
6-6: Entry point now correctly points to build output.The
mainfield now references./dist/start.js, which matches the tsdown build output. This resolves the previous mismatch.
23-50: New dependencies support the refactored architecture.The additions of
@plane/decorators,@plane/logger,zod(for env validation), and@dotenvx/dotenvxalign with the controller-based architecture and validated configuration introduced in this PR.
|
@Palanikannan1437 Once check the code in decorators package |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/decorators/src/controller.ts (1)
17-42: Controllers with mixed REST and WS handlers lose REST routes; DI broken for REST.The if/else logic (lines 33-40) registers either WebSocket or REST handlers, not both:
- If any method has WS metadata, only
registerWebSocketControlleris called; all REST routes are skipped.- When REST is registered (line 39),
registerRestControllercreates a fresh instance (line 45) without dependencies, breaking dependency injection.This issue was flagged in a previous review and marked as addressed, but the problematic branching logic remains.
Apply this diff to register both types using the same instance:
- 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 routes using the same instance with dependencies + registerRestController(router, Controller, instance); + registerWebSocketController(router, Controller, instance);Additionally, update
registerRestControllerto accept and use the instance parameter:-function registerRestController(router: Router, Controller: ControllerConstructor): void { - const instance = new Controller(); +function registerRestController( + router: Router, + Controller: ControllerConstructor, + existingInstance?: ControllerInstance +): void { + const instance = existingInstance || new Controller(); const baseRoute = Reflect.getMetadata("baseRoute", Controller) as string;apps/live/src/server.ts (1)
65-76: CRITICAL: Wildcard origin with credentials=true violates CORS specification.When
env.CORS_ALLOWED_ORIGINS === "*", the code setsorigin: "*"withcredentials: true, which browsers will reject per the CORS specification. This was flagged in previous reviews and by GitHub security scanning but remains unfixed.Apply this fix to handle wildcard origins correctly:
private setupCors() { const allowedOrigins = env.CORS_ALLOWED_ORIGINS === "*" ? "*" : env.CORS_ALLOWED_ORIGINS.split(",").map((s) => s.trim()); this.app.use( cors({ - origin: allowedOrigins, - credentials: true, + origin: allowedOrigins === "*" ? true : allowedOrigins, + credentials: allowedOrigins !== "*", methods: ["GET", "POST", "PUT", "DELETE", "OPTIONS"], allowedHeaders: ["Content-Type", "Authorization", "x-api-key"], }) ); }With
origin: true, CORS will reflect the requesting origin (allowing all), andcredentials: falseensures the configuration is valid. If you need credentials, use an explicit allowlist instead of wildcard.
🧹 Nitpick comments (3)
apps/live/src/controllers/health.controller.ts (1)
9-16: Consider removing theasynckeyword.The
healthCheckmethod doesn't useawaitor return a Promise, so theasynckeyword is unnecessary. This is purely a nitpick—functionality is correct.Apply this diff if you'd like to simplify:
- async healthCheck(_req: Request, res: Response) { + healthCheck(_req: Request, res: Response) { res.status(200).json({ status: "OK", timestamp: new Date().toISOString(), version: env.APP_VERSION, }); }packages/decorators/src/controller.ts (1)
13-13: Weakening type safety withany[].Changing the constructor parameter type from
unknown[]toany[]bypasses type checking. Consider keepingunknown[]to enforce type narrowing at call sites, or document whyany[]is required.apps/live/src/server.ts (1)
35-49: Consider adding idempotency guard to prevent double-initialization.The
initialize()method correctly awaits Redis setup before HocusPocus initialization and defers route registration until dependencies are ready. However, if called multiple times, routes would be re-registered, potentially causing conflicts.Consider adding a flag to make initialization idempotent:
export class Server { private app: Express; private router: Router; private hocuspocusServer: Hocuspocus | undefined; private httpServer: HttpServer | undefined; + private initialized = false; public async initialize(): Promise<void> { + if (this.initialized) { + logger.warn("Server already initialized, skipping"); + return; + } try { await redisManager.initialize(); logger.info("Redis setup completed"); const manager = HocusPocusServerManager.getInstance(); this.hocuspocusServer = await manager.initialize(); logger.info("HocusPocus setup completed"); this.setupRoutes(this.hocuspocusServer); this.setupNotFoundHandler(); + this.initialized = true; } catch (error) { logger.error("Failed to initialize live server dependencies:", error); throw error; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/live/src/controllers/convert-document.controller.ts(1 hunks)apps/live/src/controllers/health.controller.ts(1 hunks)apps/live/src/server.ts(1 hunks)packages/decorators/src/controller.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/live/src/controllers/convert-document.controller.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/live/src/server.ts (7)
apps/live/src/env.ts (1)
env(36-36)apps/live/src/redis.ts (1)
redisManager(211-211)packages/logger/src/config.ts (1)
logger(14-14)apps/live/src/hocuspocus.ts (1)
HocusPocusServerManager(11-63)packages/logger/src/middleware.ts (1)
loggerMiddleware(6-11)packages/decorators/src/controller.ts (1)
registerControllers(17-42)apps/live/src/controllers/index.ts (1)
CONTROLLERS(5-5)
apps/live/src/controllers/health.controller.ts (4)
apps/live/src/controllers/convert-document.controller.ts (1)
Controller(10-39)apps/live/src/controllers/collaboration.controller.ts (1)
Controller(8-33)packages/decorators/src/rest.ts (1)
Get(34-34)apps/live/src/env.ts (1)
env(36-36)
⏰ 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: Cursor Bugbot
- GitHub Check: Cursor Bugbot
🔇 Additional comments (10)
apps/live/src/controllers/health.controller.ts (2)
1-3: LGTM!Imports are clean and appropriate for the health controller endpoint.
5-7: LGTM!Class declaration follows the pattern established in other controllers (CollaborationController, ConvertDocumentController). The index signature is consistent with the decorator framework's requirements.
apps/live/src/server.ts (8)
1-18: LGTM: Imports are well-organized and complete.The imports are properly structured with clear sections (Node types, external dependencies, Plane packages, local modules). The HttpServer type import addresses previous type safety concerns, and all necessary dependencies are present.
21-24: LGTM: Class fields are properly typed.All fields use explicit types instead of
any, addressing previous type safety concerns. ThehttpServerandhocuspocusServerfields correctly use union types withundefinedto reflect their initialization lifecycle.
26-33: LGTM: Constructor lifecycle is correctly ordered.The constructor now properly defers route registration until after
initialize()completes, addressing previous lifecycle issues where controllers would receive a null HocusPocus server. The middleware and router setup order is correct.
51-63: LGTM: Middleware stack is well-configured.The middleware setup follows best practices with proper ordering (security → compression → logging → parsing → CORS) and leverages environment-driven configuration for compression settings. The use of centralized
loggerMiddlewarefrom@plane/loggerpromotes consistency.
78-84: LGTM: 404 handler is correctly positioned.The not-found handler is registered after routes via
setupNotFoundHandler(), ensuring it only catches unmatched requests. The JSON response format is appropriate for an API server.
86-88: LGTM: Route registration correctly wires dependencies.The
setupRoutesmethod properly passes the initializedhocuspocusServerto controllers via theregisterControllersdependency array, ensuring controllers receive a valid server instance. This addresses previous lifecycle issues.
90-99: LGTM: Server startup is properly typed and error-handled.The
listen()method correctly assigns the typedHttpServerinstance, includes error handling that logs and rethrows, and provides clear startup confirmation. This addresses previous type safety concerns.
101-122: LGTM: Graceful shutdown is correctly implemented.The
destroy()method properly sequences shutdown (HocusPocus → Redis → HTTP server) and promisifies the HTTP server'sclose()callback to ensure the method only resolves after all resources are released. This addresses previous async cleanup concerns.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Palanikannan1437
force-pushed
the
feat-live-server-sync
branch
from
649e51c to
465006b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/live/package.json (1)
23-23: Remove unused dotenv dependency
Thedotenvpackage in apps/live/package.json isn’t imported or used anywhere (only@dotenvx/dotenvxis imported in apps/live/src/env.ts). Remove thedotenventry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/live/package.json(4 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: Cursor Bugbot
- GitHub Check: Build and lint web apps
🔇 Additional comments (3)
apps/live/package.json (3)
6-6: LGTM! Entry point aligned with build output.The main field now correctly points to
dist/start.js, matching the new entrypoint defined intsdown.config.tsand used throughout the scripts.
28-28: LGTM! Workspace dependencies support the refactor.The additions of
@plane/decoratorsand@plane/loggeralign with the controller-based architecture and centralized logging mentioned in the PR objectives.Also applies to: 30-30
61-61: LGTM! Type definitions match WebSocket library version.The
@types/wsversion aligns with thewspackage version (8.18.x), which is appropriate for TypeScript type safety.
| "build": "tsc --noEmit && tsdown", | ||
| "dev": "tsdown --watch --onSuccess \"node --env-file=.env dist/start.js\"", | ||
| "start": "node --env-file=.env dist/start.js", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Node.js version requirement is documented.
The scripts use node --env-file=.env, which requires Node.js ≥20.11. The past review comment mentions "engines": { "node": ">=22.18.0" } was added, but it's not visible in the current file.
Run the following to confirm the engines field is present:
🏁 Script executed:
#!/bin/bash
# Check if engines field exists in package.json
cat apps/live/package.json | jq -r '.engines'Length of output: 4
Add Node.js engine requirement in apps/live/package.json
Include an engines entry specifying Node.js ≥20.11.0 to guarantee support for the --env-file flag.
{
"engines": {
"node": ">=20.11.0"
}
}🤖 Prompt for AI Agents
In apps/live/package.json around lines 10 to 12, add an "engines" field to
require Node.js >=20.11.0 so the CLI "--env-file" flag is supported; update the
package.json by inserting an "engines": { "node": ">=20.11.0" } entry at the
top-level (near other metadata like name/version/scripts) and run a JSON
linter/validator to ensure the file remains valid.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| export abstract class PageService extends PageCoreService { | ||
| constructor() { | ||
| super(); | ||
| } |
There was a problem hiding this comment.
Bug: Empty Constructor Violates PR Guidelines
Empty constructor violates the PR requirement to "remove empty constructors wherever used". The constructor only calls super() and adds no additional functionality.
| throw new Error("Redis client not initialized"); | ||
| } | ||
| return redisClient; | ||
| }; |
There was a problem hiding this comment.
Bug: Redis Configuration Error Causes Server Crash
The server now crashes on startup if Redis isn't configured. The Redis extension is unconditionally added, and its constructor throws an error when redisManager.getClient() returns null. This prevents the application from starting, changing previous behavior where Redis was optional.
Additional Locations (1)
|
|
||
| constructor() { | ||
| super(); | ||
| } |
There was a problem hiding this comment.
Bug: Empty Constructor Violates PR Guidelines
Empty constructor violates the PR requirement to "remove empty constructors wherever used". The constructor only calls super() and adds no additional functionality.
Description
Type of Change
Summary by CodeRabbit
New Features
Improvements
Chores
Note
Rearchitects the Live app into a modular controller-based server with validated env, Redis-backed Hocuspocus, centralized extensions, and a new start entrypoint with updated build/run scripts.
@plane/decorators(controllers/*) for REST and WebSocket endpoints.HocusPocusServerManager(src/hocuspocus.ts) and streamlinedServer(src/server.ts).@plane/logger.src/env.ts) usingzodand@dotenvx/dotenvx.extensions/logger,extensions/database,extensions/redis.RedisManager(src/redis.ts) and Redis-backed broadcast helper.src/lib/auth.ts,src/lib/stateless.ts).GET /health,POST /convert-document, andWS /collaborationvia controllers.APIService) with header management; introducesservices/page/*with type-safe payloads.utils/documentand simplifies binary/HTML transforms.src/start.ts;tsdownentry updated; package scripts point todist/start.js.apps/live/dist/start.js.Written by Cursor Bugbot for commit 2d568b5. This will update automatically on new commits. Configure here.