feat(mcp): add live collaboration demo

[alex-alecu]

What changed

Adds a standalone local demo where Codex or Claude Code can edit a DOCX through MCP and the changes show up live in SuperDoc.

It includes the browser client, Yjs room server, a demo-only HTTP MCP host, and the tests needed to cover the full loop.

Why a new MCP host

We spun up a small, demo-only HTTP host because the PR that adds live updates (#3569) has not merged yet. This lets us exercise the browser-to-MCP flow now while keeping the published stdio server untouched.

This PR currently includes the commits from #3569 because GitHub cannot use that PR's fork branch as the base here. Once #3569 lands, we can rebase this branch onto main and the diff will shrink to just the demo and its HTTP transport.

This is a localhost-only demo. It does not include an LLM or require a model API key.

Visual changes

output-1440p-30fps-1.2x.mp4

Test

• make test

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. HTTP server leaks sessions ✓ Resolved 🐞 Bug ☼ Reliability

Description

In the demo MCP HTTP server, transport.onclose deletes the session from the map and only calls
sessions.closeAll(), never server.close(), so normally-terminated protocol sessions can leave
their McpServer instance alive and unrecoverable by startMcpHttpServer().close().
This can accumulate open resources over time in a long-running demo server (and makes shutdown
incomplete for sessions already removed from the map).

Code

demos/mcp-collaboration/mcp-server/src/http-server.ts[R113-117]

+  transport.onclose = () => {
+    const initializedSessionId = transport.sessionId;
+    if (initializedSessionId) sessions.delete(initializedSessionId);
+    void created.sessions.closeAll();
+  };

Relevance

⭐⭐⭐ High

Cleanup/resource-leak fixes are commonly accepted (dispose/teardown robustness in PR #2892, #2760).

PR-#2892
PR-#2760

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

transport.onclose only deletes the session and calls created.sessions.closeAll(); it never calls
created.server.close(). A proper cleanup helper exists (closeProtocolSession) and explicitly
closes the server, but it is not used on normal session termination, so those servers can remain
open after the session is removed from the tracking map.

demos/mcp-collaboration/mcp-server/src/http-server.ts[104-117]
demos/mcp-collaboration/mcp-server/src/http-server.ts[141-144]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`StreamableHTTPServerTransport.onclose` currently only closes SuperDoc document sessions (`created.sessions.closeAll()`), but it does **not** close the MCP server (`created.server.close()`). Because the session is also removed from the `sessions` map, global shutdown can no longer find and close the orphaned server.

### Issue Context
You already have `closeProtocolSession()` which does the correct full cleanup (`sessions.closeAll()` + `server.close()`), but it isn’t used from `transport.onclose`.

### Fix Focus Areas
- demos/mcp-collaboration/mcp-server/src/http-server.ts[113-126]
- demos/mcp-collaboration/mcp-server/src/http-server.ts[141-144]

### Implementation notes
- In `transport.onclose`, invoke full cleanup (prefer `closeProtocolSession(protocolSession)`) and ensure errors are handled (e.g., `void closeProtocolSession(protocolSession).catch(...)`).
- Keep the map deletion, but ensure the server is closed even if `transport.sessionId` is missing or cleanup races with shutdown.
- Make cleanup idempotent (it may run from both `onclose` and `RunningMcpHttpServer.close()`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Client sync event mismatch ✓ Resolved 🐞 Bug ☼ Reliability

Description

The demo React client only marks the document ready when the provider is already synced or emits
sync(true), so providers that emit only the no-arg synced event (or rely on isSynced) can
leave the UI stuck on the “Syncing document...” state.
This is an incomplete sync contract relative to the codebase’s canonical provider-sync helper.

Code

demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[R60-75]

+  useEffect(() => {
+    // Already synced from a previous mount (HMR)
+    if (room.provider.synced) {
+      setSynced(true);
+      return;
+    }
+
+    const onSync = (isSynced: boolean) => {
+      if (isSynced) setSynced(true);
+    };
+
+    room.provider.on('sync', onSync);
+    return () => {
+      room.provider.off('sync', onSync);
+    };
+  }, [room]);

Relevance

⭐⭐⭐ High

Team favors full provider sync contract; collab robustness fixes accepted previously (PR #2550,
#3101).

PR-#2550
PR-#3101

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The demo client subscribes only to the sync event and only checks provider.synced. The
codebase’s own sync helper explicitly supports providers that emit synced (no args) and uses both
synced/isSynced as state signals, demonstrating that sync-only handling is not the full
supported contract.

demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[60-75]
packages/super-editor/src/editors/v1/core/helpers/collaboration-provider-sync.ts[26-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The demo client waits only for `provider.on('sync', ...)` and checks only `provider.synced` before subscribing. This can hang indefinitely for providers that emit `synced` (no args) and/or use `isSynced`.

### Issue Context
The repository already has a canonical helper (`onCollaborationProviderSynced`) that:
- pre-checks `synced`/`isSynced`
- listens to **both** `sync(boolean)` and `synced`
- re-checks after wiring to avoid races

### Fix Focus Areas
- demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[60-75]

### Implementation notes
- Option A (preferred): reuse the canonical helper if it’s feasible to import in this demo client build.
- Option B: locally mirror the behavior: check `provider.synced || provider.isSynced`, subscribe to both `'sync'` and `'synced'`, and re-check state after subscribing.
- Ensure cleanup unsubscribes both handlers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Bad JSON returns 500 🐞 Bug ≡ Correctness

Description

readJsonBody() calls JSON.parse() without handling parse errors; malformed JSON throws and is
caught by routeMcpRequest, which responds with HTTP 500 rather than a 400-level client error.
This misclassifies client input problems as server failures and can trigger incorrect
retries/alerting.

Code

demos/mcp-collaboration/mcp-server/src/http-server.ts[R160-165]

+async function readJsonBody(request: IncomingMessage): Promise<unknown> {
+  const chunks: Buffer[] = [];
+  for await (const chunk of request) chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk));
+  if (!chunks.length) return undefined;
+  return JSON.parse(Buffer.concat(chunks).toString('utf8'));
+}

Relevance

⭐⭐ Medium

No strong repo precedent found for 400 vs 500 handling of malformed JSON bodies.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

readJsonBody performs an unguarded JSON.parse. Any parse failure bubbles to routeMcpRequest’s
catch, which always sends a 500 JSON-RPC error when headers aren’t already sent.

demos/mcp-collaboration/mcp-server/src/http-server.ts[64-83]
demos/mcp-collaboration/mcp-server/src/http-server.ts[160-165]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Malformed JSON bodies cause `JSON.parse()` to throw, which is handled by the top-level catch in `routeMcpRequest` and returns a 500. This should be a 400 (bad request) / JSON-RPC parse error response.

### Issue Context
`handlePost()` always calls `readJsonBody()` before dispatching. If parsing fails, control jumps to the `routeMcpRequest` catch-all (500).

### Fix Focus Areas
- demos/mcp-collaboration/mcp-server/src/http-server.ts[64-83]
- demos/mcp-collaboration/mcp-server/src/http-server.ts[160-165]

### Implementation notes
- Catch `SyntaxError` from `JSON.parse` (either inside `readJsonBody` or in `handlePost`).
- Respond with a 400 and an appropriate JSON-RPC-shaped error (or plain text) consistent with the rest of the handler.
- Keep 500 for true server-side errors.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d103feac7d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Unencoded roomId in URLs ✗ Dismissed 🐞 Bug ≡ Correctness ⭐ New

Description

The demo client interpolates roomId directly into fetch URLs (and route navigation), so reserved
characters (e.g. /, ?, #, spaces) can change URL parsing and hit the wrong path/404 before the
room server’s validation runs. This makes room join/start behavior fail in non-obvious ways for
user-entered room IDs.

Code

demos/mcp-collaboration/client/src/lib/room-api.ts[R10-22]

+  const response = await fetch(`${ROOM_SERVER_URL}/rooms/${roomId}/start`, { method: 'POST', body });
+  if (!response.ok) throw new Error(`Failed to start room: ${response.status}`);
+  return response.json();
+}
+
+export async function getRoomStatus(roomId: string): Promise<RoomStatus> {
+  const response = await fetch(`${ROOM_SERVER_URL}/rooms/${roomId}/status`);
+  if (!response.ok) throw new Error(`Failed to get room status: ${response.status}`);
+  return response.json();
+}
+
+export function getDownloadUrl(roomId: string): string {
+  return `${ROOM_SERVER_URL}/rooms/${roomId}/download`;

Relevance

⭐⭐⭐ High

Team frequently accepts URL/input sanitization fixes (e.g., link sanitization changes accepted in PR
#2319).

PR-#2319

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

room-api.ts builds request URLs by directly embedding roomId into the path, and JoinRoomForm
embeds the trimmed value directly into the client route; neither uses encodeURIComponent.

demos/mcp-collaboration/client/src/lib/room-api.ts[5-23]
demos/mcp-collaboration/client/src/components/landing/join-room-form.tsx[12-20]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Room IDs are inserted into URLs without encoding, so certain inputs won’t reach the intended endpoint/route.

## Issue Context
The room server validates IDs (`ROOM_ID_PATTERN`), but the client may never reach that validation if the roomId changes the URL structure.

## Fix Focus Areas
- demos/mcp-collaboration/client/src/lib/room-api.ts[5-23]
- demos/mcp-collaboration/client/src/components/landing/join-room-form.tsx[12-20]

## Suggested fix
- In `room-api.ts`, construct URLs with `encodeURIComponent(roomId)` for each path segment:
 - `/rooms/${encodeURIComponent(roomId)}/start`, etc.
- In `JoinRoomForm`, navigate with an encoded param (or validate/normalize to the same regex before navigating).
- (Optional) Apply the same room-id regex client-side so the user gets an immediate validation error instead of a fetch/navigation failure.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Host header locality check ✗ Dismissed 🐞 Bug ⛨ Security ⭐ New

Description

The demo MCP HTTP server treats request.headers.host as proof the request is local, but the Host
header is client-controlled and can be spoofed, so “localhost-only” enforcement can be bypassed if
the service is ever exposed via a proxy/port-forward. This also rejects legitimate local requests
when a proxy sets a different Host value.

Code

demos/mcp-collaboration/mcp-server/src/http-server.ts[R178-183]

+function isLocalHostHeader(hostHeader: string | undefined): boolean {
+  if (!hostHeader) return false;
+  try {
+    const hostname = new URL(`http://${hostHeader}`).hostname;
+    return hostname === '127.0.0.1' || hostname === 'localhost' || hostname === '[::1]';
+  } catch {

Relevance

⭐⭐⭐ High

Security guidance to not trust client-controlled fields is accepted historically (docs updated in PR
#2920).

PR-#2920

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

routeMcpRequest uses request.headers.host to decide whether to reject as “Forbidden host”, and
isLocalHostHeader only parses/compares the header value—this does not verify the actual socket
peer address.

demos/mcp-collaboration/mcp-server/src/http-server.ts[65-72]
demos/mcp-collaboration/mcp-server/src/http-server.ts[178-186]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The demo MCP HTTP server’s locality gate relies on the HTTP `Host` header, which is not a trustworthy signal of the connecting peer.

## Issue Context
The server already binds to loopback, but the code also intends to enforce “local only” at the HTTP layer. Using `Host` for this is ineffective behind any forwarding/proxying and can also block legitimate local setups.

## Fix Focus Areas
- demos/mcp-collaboration/mcp-server/src/http-server.ts[65-72]
- demos/mcp-collaboration/mcp-server/src/http-server.ts[178-186]

## Suggested fix
- Replace `isLocalHostHeader(request.headers.host)` with a check of `request.socket.remoteAddress` (allow `127.0.0.1`, `::1`, and optionally IPv4-mapped IPv6 like `::ffff:127.0.0.1`).
- Alternatively, remove the Host-based check entirely and rely on binding to loopback for the demo (keep Bearer auth).
- If proxy support is desired, make it explicit and validate trusted proxy headers rather than `Host`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Room download throws 500 ✗ Dismissed 🐞 Bug ☼ Reliability

Description

The /rooms/:roomId/download route awaits rooms.export(roomId) without handling errors, but
RoomManager.export() throws when the room isn’t ready or initialization failed. As a result,
clients can get an unstructured HTTP 500 for a normal “not ready / failed” state instead of a
4xx/409-style response.

Code

demos/mcp-collaboration/room-server/src/rooms.ts[R52-56]

+  app.get('/rooms/:roomId/download', async (request, reply) => {
+    const roomId = readRoomId(request, reply);
+    if (!roomId) return;
+    const outputPath = await rooms.export(roomId);
+    if (!outputPath) return reply.code(404).send({ error: 'Room not found' });

Relevance

⭐⭐⭐ High

They favor explicit error propagation/handling over unstructured 500s (export error handling
accepted in PR #3641).

PR-#3641
PR-#3404

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The download handler directly awaits rooms.export() with no error handling, while
RoomManager.export() explicitly throws when the editor is null (not ready or failed), which
Fastify will surface as a 500.

demos/mcp-collaboration/room-server/src/rooms.ts[52-60]
demos/mcp-collaboration/room-server/src/room-manager.ts[48-53]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GET /rooms/:roomId/download` can return 500 when a room is not ready (or failed to initialize) because `RoomManager.export()` throws and the route doesn’t catch/map the error.

## Issue Context
- Unknown room is already mapped to 404 (`export()` returns `null`).
- “Not ready” and “init failed” are currently represented as `room.editor === null` and an optional `room.error`, but they are thrown as generic errors.

## Fix Focus Areas
- demos/mcp-collaboration/room-server/src/rooms.ts[52-60]
- demos/mcp-collaboration/room-server/src/room-manager.ts[48-57]

### Suggested fix
Option A (route-level mapping):
- Wrap `rooms.export(roomId)` in try/catch.
- If the error indicates not-ready, return `409` (or `503`) with JSON `{ error: 'Document not ready' }`.
- If initialization failed, return `422`/`500` with `{ error: roomError }`.

Option B (API-level typing):
- Change `RoomManager.export()` to return a discriminated result (e.g., `{ ok: false, code: 'NOT_READY' | 'FAILED', error?: string }`) and map to status codes in the route.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (2)

4. Upload buffers whole file ✗ Dismissed 🐞 Bug ➹ Performance

Description

The room start endpoint buffers multipart file uploads into chunks and then Buffer.concat()s
them before writing, which temporarily double-allocates memory (chunks + concatenated buffer). With
the 50MB limit, concurrent uploads can create significant memory pressure and may destabilize the
demo server.

Code

demos/mcp-collaboration/room-server/src/rooms.ts[R30-35]

+        if (part.type === 'file' && part.fieldname === 'file') {
+          await mkdir(UPLOAD_DIR, { recursive: true });
+          sourcePath = join(UPLOAD_DIR, `${roomId}.docx`);
+          const chunks: Buffer[] = [];
+          for await (const chunk of part.file) chunks.push(Buffer.from(chunk));
+          await writeFile(sourcePath, Buffer.concat(chunks));

Relevance

⭐⭐ Medium

No repo history found about streaming multipart uploads vs Buffer.concat buffering in demos/servers.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code explicitly accumulates all chunks in an array and then concatenates them into a single
buffer before writing, guaranteeing full buffering (and a second allocation during concat).

demos/mcp-collaboration/room-server/src/rooms.ts[16-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The multipart upload handler reads the entire uploaded DOCX into memory and then concatenates it, causing temporary double-buffering and high memory usage under concurrency.

## Issue Context
There is already a 50MB multipart limit, but the implementation still buffers the entire file before writing.

## Fix Focus Areas
- demos/mcp-collaboration/room-server/src/rooms.ts[17-36]

### Suggested fix
- Replace the `chunks` + `Buffer.concat()` approach with streaming to a file:
 - `const out = createWriteStream(sourcePath)`
 - `await pipeline(part.file, out)` (node:stream/promises)
- Keep the multipart `fileSize` limit in place as a guardrail.
- Ensure the file stream is closed/cleaned up on errors.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. IPv6 localhost rejected ✗ Dismissed 🐞 Bug ≡ Correctness

Description

isLocalHostHeader() compares URL(...).hostname to '[::1]', but URL.hostname normalizes IPv6
loopback to '::1', so requests to http://[::1]:PORT/mcp are incorrectly rejected with 403. This
breaks a common localhost access pattern on IPv6-preferred systems/clients.

Code

demos/mcp-collaboration/mcp-server/src/http-server.ts[R178-183]

+function isLocalHostHeader(hostHeader: string | undefined): boolean {
+  if (!hostHeader) return false;
+  try {
+    const hostname = new URL(`http://${hostHeader}`).hostname;
+    return hostname === '127.0.0.1' || hostname === 'localhost' || hostname === '[::1]';
+  } catch {

Relevance

⭐⭐ Medium

No direct precedent for IPv6 Host header allowlists; only general URL normalization fixes seen
accepted.

PR-#2319
PR-#3404

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The allowlist compares to '[::1]', but the parsed hostname is taken from URL(...).hostname,
which returns the normalized hostname without brackets for IPv6, so the check fails for loopback
IPv6 host headers.

demos/mcp-collaboration/mcp-server/src/http-server.ts[178-183]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The MCP demo HTTP server’s localhost allowlist incorrectly checks for `"[::1]"` instead of the normalized hostname `"::1"`, causing IPv6 loopback clients to be blocked.

## Issue Context
`new URL('http://[::1]:8091').hostname` yields `"::1"` (without brackets). The allowlist should match normalized output.

## Fix Focus Areas
- demos/mcp-collaboration/mcp-server/src/http-server.ts[178-186]

### Suggested fix
Update the check to accept `"::1"` (and optionally keep `"[::1]"` defensively), e.g.:
- `hostname === '::1'` (preferred)
- or normalize by stripping `[`/`]` before comparison.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. YDoc GC config mismatch 🐞 Bug ⚙ Maintainability ⭐ New

Description

The demo editor workspace constructs its collaboration doc with new YDoc() while the repo’s
collaboration setup consistently uses new YDoc({ gc: false }), so the demo is not
configuration-parity with the rest of the system. This increases the risk of demo-only collaboration
behavior differences that are hard to reproduce outside the demo.

Code

demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[R48-49]

+  const ydoc = new YDoc();
+  const provider = new WebsocketProvider(COLLAB_URL, roomId, ydoc);

Relevance

⭐⭐ Medium

No historical review evidence about enforcing YDoc({ gc: false }) parity across demos/examples.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The demo client creates a default YDoc, while the core collaboration provider setup in the repo
explicitly disables GC (gc: false), demonstrating an intentional convention the demo currently
diverges from.

demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[42-51]
packages/superdoc/src/core/collaboration/collaboration.js[70-79]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The demo client creates a Yjs Doc with default GC settings, diverging from the repo’s standard collaboration configuration.

## Issue Context
Multiple collaboration entry points in the repository explicitly set `gc: false`.

## Fix Focus Areas
- demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[42-51]

## Suggested fix
- Change `const ydoc = new YDoc();` to `const ydoc = new YDoc({ gc: false });` to match other collaboration flows.
- If the default GC behavior is intentional for this demo, add an inline comment explaining why the demo differs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. HMR unload handler leak ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

EditorWorkspace registers an anonymous beforeunload listener at module load, which cannot be
removed and can be duplicated across applicable Vite HMR module re-evaluations. This is primarily a
dev-time leak (stale listeners/closures and potentially extra cleanup work), not a production
navigation issue.

Code

demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[R45-55]

+// Clean up on full page unload (not HMR)
+if (typeof window !== 'undefined') {
+  window.addEventListener('beforeunload', () => {
+    if (cached) {
+      cached.provider.disconnect();
+      cached.provider.destroy();
+      cached.ydoc.destroy();
+      cached = null;
+    }
+  });
+}

Relevance

⭐⭐⭐ High

Team often fixes leaked/duplicated listeners and missing teardown (listener-growth fix accepted in
PR #3353).

PR-#3353
PR-#2924
PR-#3091

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The beforeunload handler is installed at module scope with an inline function and no corresponding
removal path, which is exactly the pattern that can accumulate across HMR re-evaluations.

demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[45-55]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A module-scope `beforeunload` listener is registered with an inline anonymous callback; it can’t be removed and may accumulate across certain HMR updates.

## Issue Context
This is mainly a developer-experience issue during Vite HMR, where modules can be re-evaluated and old listeners may remain.

## Fix Focus Areas
- demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[45-55]

### Suggested fix
- Define a named/stable handler function and register it once.
- If using Vite HMR, add `import.meta.hot?.dispose(() => window.removeEventListener('beforeunload', handler))`.
- Optionally also persist/cleanup `cached` via `import.meta.hot.data` so the “survives HMR” comment matches behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 961697c

Results up to commit fcb4dde

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Room download throws 500 ✗ Dismissed 🐞 Bug ☼ Reliability

Description

The /rooms/:roomId/download route awaits rooms.export(roomId) without handling errors, but
RoomManager.export() throws when the room isn’t ready or initialization failed. As a result,
clients can get an unstructured HTTP 500 for a normal “not ready / failed” state instead of a
4xx/409-style response.

Code

demos/mcp-collaboration/room-server/src/rooms.ts[R52-56]

+  app.get('/rooms/:roomId/download', async (request, reply) => {
+    const roomId = readRoomId(request, reply);
+    if (!roomId) return;
+    const outputPath = await rooms.export(roomId);
+    if (!outputPath) return reply.code(404).send({ error: 'Room not found' });

Relevance

⭐⭐⭐ High

They favor explicit error propagation/handling over unstructured 500s (export error handling
accepted in PR #3641).

PR-#3641
PR-#3404

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The download handler directly awaits rooms.export() with no error handling, while
RoomManager.export() explicitly throws when the editor is null (not ready or failed), which
Fastify will surface as a 500.

demos/mcp-collaboration/room-server/src/rooms.ts[52-60]
demos/mcp-collaboration/room-server/src/room-manager.ts[48-53]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GET /rooms/:roomId/download` can return 500 when a room is not ready (or failed to initialize) because `RoomManager.export()` throws and the route doesn’t catch/map the error.

## Issue Context
- Unknown room is already mapped to 404 (`export()` returns `null`).
- “Not ready” and “init failed” are currently represented as `room.editor === null` and an optional `room.error`, but they are thrown as generic errors.

## Fix Focus Areas
- demos/mcp-collaboration/room-server/src/rooms.ts[52-60]
- demos/mcp-collaboration/room-server/src/room-manager.ts[48-57]

### Suggested fix
Option A (route-level mapping):
- Wrap `rooms.export(roomId)` in try/catch.
- If the error indicates not-ready, return `409` (or `503`) with JSON `{ error: 'Document not ready' }`.
- If initialization failed, return `422`/`500` with `{ error: roomError }`.

Option B (API-level typing):
- Change `RoomManager.export()` to return a discriminated result (e.g., `{ ok: false, code: 'NOT_READY' | 'FAILED', error?: string }`) and map to status codes in the route.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. IPv6 localhost rejected ✗ Dismissed 🐞 Bug ≡ Correctness

Description

isLocalHostHeader() compares URL(...).hostname to '[::1]', but URL.hostname normalizes IPv6
loopback to '::1', so requests to http://[::1]:PORT/mcp are incorrectly rejected with 403. This
breaks a common localhost access pattern on IPv6-preferred systems/clients.

Code

demos/mcp-collaboration/mcp-server/src/http-server.ts[R178-183]

+function isLocalHostHeader(hostHeader: string | undefined): boolean {
+  if (!hostHeader) return false;
+  try {
+    const hostname = new URL(`http://${hostHeader}`).hostname;
+    return hostname === '127.0.0.1' || hostname === 'localhost' || hostname === '[::1]';
+  } catch {

Relevance

⭐⭐ Medium

No direct precedent for IPv6 Host header allowlists; only general URL normalization fixes seen
accepted.

PR-#2319
PR-#3404

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The allowlist compares to '[::1]', but the parsed hostname is taken from URL(...).hostname,
which returns the normalized hostname without brackets for IPv6, so the check fails for loopback
IPv6 host headers.

demos/mcp-collaboration/mcp-server/src/http-server.ts[178-183]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The MCP demo HTTP server’s localhost allowlist incorrectly checks for `"[::1]"` instead of the normalized hostname `"::1"`, causing IPv6 loopback clients to be blocked.

## Issue Context
`new URL('http://[::1]:8091').hostname` yields `"::1"` (without brackets). The allowlist should match normalized output.

## Fix Focus Areas
- demos/mcp-collaboration/mcp-server/src/http-server.ts[178-186]

### Suggested fix
Update the check to accept `"::1"` (and optionally keep `"[::1]"` defensively), e.g.:
- `hostname === '::1'` (preferred)
- or normalize by stripping `[`/`]` before comparison.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Upload buffers whole file ✗ Dismissed 🐞 Bug ➹ Performance

Description

The room start endpoint buffers multipart file uploads into chunks and then Buffer.concat()s
them before writing, which temporarily double-allocates memory (chunks + concatenated buffer). With
the 50MB limit, concurrent uploads can create significant memory pressure and may destabilize the
demo server.

Code

demos/mcp-collaboration/room-server/src/rooms.ts[R30-35]

+        if (part.type === 'file' && part.fieldname === 'file') {
+          await mkdir(UPLOAD_DIR, { recursive: true });
+          sourcePath = join(UPLOAD_DIR, `${roomId}.docx`);
+          const chunks: Buffer[] = [];
+          for await (const chunk of part.file) chunks.push(Buffer.from(chunk));
+          await writeFile(sourcePath, Buffer.concat(chunks));

Relevance

⭐⭐ Medium

No repo history found about streaming multipart uploads vs Buffer.concat buffering in demos/servers.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code explicitly accumulates all chunks in an array and then concatenates them into a single
buffer before writing, guaranteeing full buffering (and a second allocation during concat).

demos/mcp-collaboration/room-server/src/rooms.ts[16-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The multipart upload handler reads the entire uploaded DOCX into memory and then concatenates it, causing temporary double-buffering and high memory usage under concurrency.

## Issue Context
There is already a 50MB multipart limit, but the implementation still buffers the entire file before writing.

## Fix Focus Areas
- demos/mcp-collaboration/room-server/src/rooms.ts[17-36]

### Suggested fix
- Replace the `chunks` + `Buffer.concat()` approach with streaming to a file:
 - `const out = createWriteStream(sourcePath)`
 - `await pipeline(part.file, out)` (node:stream/promises)
- Keep the multipart `fileSize` limit in place as a guardrail.
- Ensure the file stream is closed/cleaned up on errors.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. HMR unload handler leak ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

EditorWorkspace registers an anonymous beforeunload listener at module load, which cannot be
removed and can be duplicated across applicable Vite HMR module re-evaluations. This is primarily a
dev-time leak (stale listeners/closures and potentially extra cleanup work), not a production
navigation issue.

Code

demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[R45-55]

+// Clean up on full page unload (not HMR)
+if (typeof window !== 'undefined') {
+  window.addEventListener('beforeunload', () => {
+    if (cached) {
+      cached.provider.disconnect();
+      cached.provider.destroy();
+      cached.ydoc.destroy();
+      cached = null;
+    }
+  });
+}

Relevance

⭐⭐⭐ High

Team often fixes leaked/duplicated listeners and missing teardown (listener-growth fix accepted in
PR #3353).

PR-#3353
PR-#2924
PR-#3091

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The beforeunload handler is installed at module scope with an inline function and no corresponding
removal path, which is exactly the pattern that can accumulate across HMR re-evaluations.

demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[45-55]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A module-scope `beforeunload` listener is registered with an inline anonymous callback; it can’t be removed and may accumulate across certain HMR updates.

## Issue Context
This is mainly a developer-experience issue during Vite HMR, where modules can be re-evaluated and old listeners may remain.

## Fix Focus Areas
- demos/mcp-collaboration/client/src/components/editor/editor-workspace.tsx[45-55]

### Suggested fix
- Define a named/stable handler function and register it once.
- If using Vite HMR, add `import.meta.hot?.dispose(() => window.removeEventListener('beforeunload', handler))`.
- Optionally also persist/cleanup `cached` via `import.meta.hot.data` so the “survives HMR” comment matches behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fcb4dde16e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[qodo-code-review]

Code review by qodo was updated up to the latest commit 961697c

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 961697ca31

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[alex-alecu]

@caio-pizzol FYI https://github.com/qodo-ai/qodo-cover is no longer maintained:

2025-06-15
⚠️ This repository is no longer maintained. Please fork it if you wish to continue development or use it in your own projects.