refactor(core): split comments-plugin into focused modules#135
Merged
Conversation
Break the 1.8k-line comments-plugin.ts into single-purpose files so each module owns one concern, and unify the HTTP endpoints it served behind one Vite plugin: - comments.ts: marker parsing, b64url helpers, JSX-insertion planning - edit-ops.ts: applyEdit + splice builders + asset-import planning - revert-asset.ts: applyRevertAsset, findAssetUsages, placeholder helpers - api-plugin.ts: single plugin registering /__comments and /__edit/* - babel-walk.ts: gains a shared parseSource helper - Tests split into comments/edit-ops/revert-asset/request-guard files Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR refactors the monolithic Vite dev-server plugin infrastructure into focused, testable modules organized by domain. Comment marker handling, asset validation, folder management, and slide operations are split into separate utility modules with comprehensive tests. All HTTP endpoints are consolidated under a single API plugin that delegates to dedicated route modules in ChangesVite plugin modularization and endpoint consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/split-comments-plugin.md:
- Line 5: Replace the implementation-detailed changeset line with a single
present-tense, user-facing one-liner such as "Split the comments Vite plugin
into focused modules." Keep it short and direct (no rationale or PR details);
mention nothing about internal files like comments.ts, edit-ops.ts,
revert-asset.ts, or api-plugin.ts nor the dev-server endpoints (/__comments,
/__edit) in the summary itself.
In `@packages/core/src/vite/api-plugin.ts`:
- Around line 39-53: readBody currently rejects JSON parse errors which end up
being treated as internal server errors by the surrounding catch blocks; change
readBody to wrap JSON.parse failures in a distinct BadRequestError (or attach
status = 400 to the thrown error) so callers can distinguish client errors, and
update the request handlers that call readBody (the API request handling logic
in this module that currently responds with 500) to check for error.status ===
400 or instanceof BadRequestError and return a 400 response for malformed JSON
while preserving 500 for other errors.
In `@packages/core/src/vite/revert-asset.ts`:
- Around line 79-92: The current logic in revert-asset.ts
(valueImport/node/lastSpec with t.isImportSpecifier and
t.isImportDefaultSpecifier) incorrectly falls through for namespace imports
(import * as ...) and side-effect imports (import '`@open-slide/core`') and tries
to insert before the `from`, producing invalid syntax; update the branch so that
when node.specifiers is empty or contains a NamespaceSpecifier you do NOT
compute insertAt before `from` but instead return an edit that adds a separate
import statement (e.g., a new line like import { ImagePlaceholder } from
'`@open-slide/core`';) inserted near the top of the file or immediately after the
existing import node, rather than modifying the existing import token; reference
valueImport, node, lastSpec, and t.isImportSpecifier/t.isImportDefaultSpecifier
to locate and change the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aaef72dd-3ace-4673-8bbd-5d4b1beb903f
📒 Files selected for processing (12)
.changeset/split-comments-plugin.mdpackages/core/src/vite/api-plugin.tspackages/core/src/vite/babel-walk.tspackages/core/src/vite/comments.test.tspackages/core/src/vite/comments.tspackages/core/src/vite/config.tspackages/core/src/vite/edit-ops.test.tspackages/core/src/vite/edit-ops.tspackages/core/src/vite/files-plugin.tspackages/core/src/vite/request-guard.test.tspackages/core/src/vite/revert-asset.test.tspackages/core/src/vite/revert-asset.ts
| '@open-slide/core': patch | ||
| --- | ||
|
|
||
| Split the comments Vite plugin into focused modules: marker helpers (`comments.ts`), source-edit operations (`edit-ops.ts`), asset revert (`revert-asset.ts`), and a unified `api-plugin.ts` that registers the `/__comments` and `/__edit` dev-server endpoints in one place. |
There was a problem hiding this comment.
Shorten the changeset summary to a user-facing one-liner.
Line 5 is too implementation-detailed for the changeset format; keep it brief and user-facing.
✏️ Suggested replacement
-Split the comments Vite plugin into focused modules: marker helpers (`comments.ts`), source-edit operations (`edit-ops.ts`), asset revert (`revert-asset.ts`), and a unified `api-plugin.ts` that registers the `/__comments` and `/__edit` dev-server endpoints in one place.
+Refactor internal Vite comments/edit plugin code into focused modules without behavior changes.As per coding guidelines, ".changeset/*.md: Changeset descriptions must be short and direct: one line, present-tense, describing what changed from a user's perspective. No paragraphs, no rationale, no 'this PR…'."
📝 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.
Suggested change
| Split the comments Vite plugin into focused modules: marker helpers (`comments.ts`), source-edit operations (`edit-ops.ts`), asset revert (`revert-asset.ts`), and a unified `api-plugin.ts` that registers the `/__comments` and `/__edit` dev-server endpoints in one place. | |
| Refactor internal Vite comments/edit plugin code into focused modules without behavior changes. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/split-comments-plugin.md at line 5, Replace the
implementation-detailed changeset line with a single present-tense, user-facing
one-liner such as "Split the comments Vite plugin into focused modules." Keep it
short and direct (no rationale or PR details); mention nothing about internal
files like comments.ts, edit-ops.ts, revert-asset.ts, or api-plugin.ts nor the
dev-server endpoints (/__comments, /__edit) in the summary itself.
Comment on lines
+39
to
+53
| async function readBody(req: Connect.IncomingMessage): Promise<unknown> { | ||
| return await new Promise((resolve, reject) => { | ||
| const chunks: Buffer[] = []; | ||
| req.on('data', (c: Buffer) => chunks.push(c)); | ||
| req.on('end', () => { | ||
| const raw = Buffer.concat(chunks).toString('utf8'); | ||
| if (!raw) return resolve({}); | ||
| try { | ||
| resolve(JSON.parse(raw)); | ||
| } catch (e) { | ||
| reject(e); | ||
| } | ||
| }); | ||
| req.on('error', reject); | ||
| }); |
There was a problem hiding this comment.
Return 400 for malformed JSON bodies instead of 500.
readBody rejects JSON parse failures (Line 46), but both outer catch blocks always emit 500 (Line 178 and Line 270). Bad client payloads should be reported as 400.
💡 Suggested fix
+class HttpError extends Error {
+ constructor(
+ public readonly status: number,
+ message: string,
+ ) {
+ super(message);
+ }
+}
+
async function readBody(req: Connect.IncomingMessage): Promise<unknown> {
return await new Promise((resolve, reject) => {
const chunks: Buffer[] = [];
req.on('data', (c: Buffer) => chunks.push(c));
req.on('end', () => {
const raw = Buffer.concat(chunks).toString('utf8');
if (!raw) return resolve({});
try {
resolve(JSON.parse(raw));
- } catch (e) {
- reject(e);
+ } catch {
+ reject(new HttpError(400, 'invalid JSON body'));
}
});
req.on('error', reject);
});
}
@@
- } catch (err) {
- json(res, 500, { error: String((err as Error).message ?? err) });
+ } catch (err) {
+ const status = err instanceof HttpError ? err.status : 500;
+ json(res, status, { error: String((err as Error).message ?? err) });
}
});
@@
- } catch (err) {
- json(res, 500, { error: String((err as Error).message ?? err) });
+ } catch (err) {
+ const status = err instanceof HttpError ? err.status : 500;
+ json(res, status, { error: String((err as Error).message ?? err) });
}
});Also applies to: 178-180, 270-272
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/vite/api-plugin.ts` around lines 39 - 53, readBody
currently rejects JSON parse errors which end up being treated as internal
server errors by the surrounding catch blocks; change readBody to wrap
JSON.parse failures in a distinct BadRequestError (or attach status = 400 to the
thrown error) so callers can distinguish client errors, and update the request
handlers that call readBody (the API request handling logic in this module that
currently responds with 500) to check for error.status === 400 or instanceof
BadRequestError and return a 400 response for malformed JSON while preserving
500 for other errors.
Comment on lines
+79
to
+92
| if (valueImport) { | ||
| const node = valueImport.node; | ||
| const lastSpec = node.specifiers[node.specifiers.length - 1]; | ||
| if (lastSpec && t.isImportSpecifier(lastSpec)) { | ||
| const insertAt = lastSpec.end ?? 0; | ||
| return { from: insertAt, to: insertAt, text: ', ImagePlaceholder' }; | ||
| } | ||
| if (lastSpec && t.isImportDefaultSpecifier(lastSpec)) { | ||
| const insertAt = lastSpec.end ?? 0; | ||
| return { from: insertAt, to: insertAt, text: ', { ImagePlaceholder }' }; | ||
| } | ||
| const insertAt = (node.source.start ?? 0) - 'from '.length; | ||
| return { from: insertAt, to: insertAt, text: '{ ImagePlaceholder } ' }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect current import-augmentation branches:"
rg -n -C3 "planEnsureImagePlaceholderImport|ImportDefaultSpecifier|ImportSpecifier|ImportNamespaceSpecifier|node\\.source\\.start" packages/core/src/vite/revert-asset.ts
echo
echo "Check test coverage for namespace and side-effect `@open-slide/core` imports:"
rg -n -C2 "import \\* as .* from '`@open-slide/core`'|^import '`@open-slide/core`'" packages/core/src/vite/revert-asset.test.ts || trueRepository: 1weiho/open-slide
Length of output: 2157
Handle namespace and side-effect @open-slide/core imports safely.
Lines 82–91 check for named and default imports, but fall through to line 90 for namespace imports (import * as core) and side-effect imports (import '@open-slide/core'). Line 90 inserts before the from keyword, producing invalid syntax and causing the revert to fail with "edit would produce invalid source".
Add a separate import statement instead of modifying the existing import:
Suggested fix
if (lastSpec && t.isImportDefaultSpecifier(lastSpec)) {
const insertAt = lastSpec.end ?? 0;
return { from: insertAt, to: insertAt, text: ', { ImagePlaceholder }' };
}
- const insertAt = (node.source.start ?? 0) - 'from '.length;
- return { from: insertAt, to: insertAt, text: '{ ImagePlaceholder } ' };
+ const insertAt = node.end ?? 0;
+ return {
+ from: insertAt,
+ to: insertAt,
+ text: "\nimport { ImagePlaceholder } from '`@open-slide/core`';",
+ };📝 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.
Suggested change
| if (valueImport) { | |
| const node = valueImport.node; | |
| const lastSpec = node.specifiers[node.specifiers.length - 1]; | |
| if (lastSpec && t.isImportSpecifier(lastSpec)) { | |
| const insertAt = lastSpec.end ?? 0; | |
| return { from: insertAt, to: insertAt, text: ', ImagePlaceholder' }; | |
| } | |
| if (lastSpec && t.isImportDefaultSpecifier(lastSpec)) { | |
| const insertAt = lastSpec.end ?? 0; | |
| return { from: insertAt, to: insertAt, text: ', { ImagePlaceholder }' }; | |
| } | |
| const insertAt = (node.source.start ?? 0) - 'from '.length; | |
| return { from: insertAt, to: insertAt, text: '{ ImagePlaceholder } ' }; | |
| } | |
| if (valueImport) { | |
| const node = valueImport.node; | |
| const lastSpec = node.specifiers[node.specifiers.length - 1]; | |
| if (lastSpec && t.isImportSpecifier(lastSpec)) { | |
| const insertAt = lastSpec.end ?? 0; | |
| return { from: insertAt, to: insertAt, text: ', ImagePlaceholder' }; | |
| } | |
| if (lastSpec && t.isImportDefaultSpecifier(lastSpec)) { | |
| const insertAt = lastSpec.end ?? 0; | |
| return { from: insertAt, to: insertAt, text: ', { ImagePlaceholder }' }; | |
| } | |
| const insertAt = node.end ?? 0; | |
| return { | |
| from: insertAt, | |
| to: insertAt, | |
| text: "\nimport { ImagePlaceholder } from '`@open-slide/core`';", | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/vite/revert-asset.ts` around lines 79 - 92, The current
logic in revert-asset.ts (valueImport/node/lastSpec with t.isImportSpecifier and
t.isImportDefaultSpecifier) incorrectly falls through for namespace imports
(import * as ...) and side-effect imports (import '`@open-slide/core`') and tries
to insert before the `from`, producing invalid syntax; update the branch so that
when node.specifiers is empty or contains a NamespaceSpecifier you do NOT
compute insertAt before `from` but instead return an edit that adds a separate
import statement (e.g., a new line like import { ImagePlaceholder } from
'`@open-slide/core`';) inserted near the top of the file or immediately after the
existing import node, rather than modifying the existing import token; reference
valueImport, node, lastSpec, and t.isImportSpecifier/t.isImportDefaultSpecifier
to locate and change the logic.
3 tasks
Break the 1.2k-line files-plugin.ts into single-purpose files and fold its four HTTP endpoints + watchers into the unified api-plugin so all dev-server APIs live in one place: - slide-ops.ts: source-editing helpers (meta.title, page/notes reorder, remove, duplicate) + slide path resolvers - assets.ts: mime lookup, asset-name validation, scoped path resolution - folders.ts: manifest read/write + folder name/icon validators - api-plugin.ts: gains /__slides, /__assets, /__svgl, /__folders middlewares and the asset/folders watcher - Tests split into slide-ops, assets, and folders Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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)
packages/core/src/vite/api-plugin.ts (1)
634-646:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe oversize-upload path can stall indefinitely.
When
req.destroy()is called without an error argument, Node.js does not emit anerrorevent and does not emit anendevent (sinceendrepresents a natural EOF, not abrupt destruction). The promise only waits for these two events, so it will never settle, and the413response at line 647 will never be reached. The request handler stalls, leaving the client hanging.Pass an error to
destroy()or add acloseevent listener to ensure the promise resolves:Example fix
req.on('data', (c: Buffer) => { total += c.length; if (total > ASSET_MAX_BYTES) { oversized = true; req.destroy(new Error('payload too large')); return; } chunks.push(c); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/vite/api-plugin.ts` around lines 634 - 646, The read-promise can hang when req.destroy() is called without an error because neither 'end' nor 'error' will fire; in the Promise that wraps req.on('data')/('end')/('error') (the block that tracks total, chunks, oversized and compares to ASSET_MAX_BYTES), change the oversize branch to either pass an Error into req.destroy (e.g. req.destroy(new Error('payload too large')) so the 'error' handler runs and the promise rejects) or add a req.on('close', ...) handler that settles the promise when the socket is aborted; update the Promise handlers accordingly so the flow that sends the 413 response can proceed.
♻️ Duplicate comments (1)
packages/core/src/vite/api-plugin.ts (1)
70-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn
400for malformed JSON bodies.
readBody()turnsJSON.parse()failures into thrown errors, and these generic catch blocks still map them to500. That makes bad client payloads look like server failures across every JSON-backed endpoint here.Also applies to: 249-250, 341-342, 495-496, 708-709, 868-869
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/vite/api-plugin.ts` around lines 70 - 80, readBody currently rejects generic parse errors from JSON.parse which get treated as 500s; modify readBody's JSON.parse catch to reject a typed error that signals a client payload problem (e.g., create and reject a new Error('Malformed JSON body') with a status or statusCode property set to 400, or reject a custom object like { name: 'BadRequest', message: 'Malformed JSON body', status: 400 }) so callers can map it to HTTP 400; update the JSON.parse catch in readBody (and analogous catches at the other noted locations) to use this 400-status error shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/vite/slide-ops.ts`:
- Around line 14-23: rmSlideDir currently returns true when fs.rm succeeds even
if the directory didn't exist (fs.rm with force ignores ENOENT); update
rmSlideDir (referencing function rmSlideDir, SLIDE_ID_RE and the fs.rm call) to
first verify the resolved dir exists (e.g., fs.stat or fs.access) and only call
fs.rm if it exists, or catch the error from fs.rm and treat ENOENT as a failure
by returning false; ensure the path safety check (dir.startsWith(slidesRoot +
path.sep)) remains and that the function returns false when the slide folder is
absent.
- Around line 56-76: The brace-counting loop that finds the meta object (using
metaStart, eqIdx, openBrace and the depth/closeBrace loop) is not syntax-aware
and will break on braces inside strings/comments/template literals; replace this
ad-hoc loop with a syntax-aware approach — either parse the module with a real
JS/TS parser (e.g., acorn/TypeScript AST) and locate the ExportNamedDeclaration
/ VariableDeclarator for the identifier "meta" to get its exact object range, or
implement a minimal state machine that skips over single/double-quoted strings,
template literals (including ${...}), and // and /* */ comments while counting
braces so closeBrace is determined correctly. Ensure the change uses the same
metaStart/eqIdx/openBrace anchors to preserve behavior where appropriate.
---
Outside diff comments:
In `@packages/core/src/vite/api-plugin.ts`:
- Around line 634-646: The read-promise can hang when req.destroy() is called
without an error because neither 'end' nor 'error' will fire; in the Promise
that wraps req.on('data')/('end')/('error') (the block that tracks total,
chunks, oversized and compares to ASSET_MAX_BYTES), change the oversize branch
to either pass an Error into req.destroy (e.g. req.destroy(new Error('payload
too large')) so the 'error' handler runs and the promise rejects) or add a
req.on('close', ...) handler that settles the promise when the socket is
aborted; update the Promise handlers accordingly so the flow that sends the 413
response can proceed.
---
Duplicate comments:
In `@packages/core/src/vite/api-plugin.ts`:
- Around line 70-80: readBody currently rejects generic parse errors from
JSON.parse which get treated as 500s; modify readBody's JSON.parse catch to
reject a typed error that signals a client payload problem (e.g., create and
reject a new Error('Malformed JSON body') with a status or statusCode property
set to 400, or reject a custom object like { name: 'BadRequest', message:
'Malformed JSON body', status: 400 }) so callers can map it to HTTP 400; update
the JSON.parse catch in readBody (and analogous catches at the other noted
locations) to use this 400-status error shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28196337-b989-4846-a8e5-4f55e3c8eb50
📒 Files selected for processing (9)
.changeset/split-files-plugin.mdpackages/core/src/vite/api-plugin.tspackages/core/src/vite/assets.test.tspackages/core/src/vite/assets.tspackages/core/src/vite/config.tspackages/core/src/vite/folders.test.tspackages/core/src/vite/folders.tspackages/core/src/vite/slide-ops.test.tspackages/core/src/vite/slide-ops.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/split-files-plugin.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/vite/config.ts
Comment on lines
+14
to
+23
| export async function rmSlideDir(slidesRoot: string, slideId: string): Promise<boolean> { | ||
| if (!SLIDE_ID_RE.test(slideId)) return false; | ||
| const dir = path.resolve(slidesRoot, slideId); | ||
| if (!dir.startsWith(slidesRoot + path.sep)) return false; | ||
| try { | ||
| await fs.rm(dir, { recursive: true, force: true }); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
rmSlideDir() reports success for missing slides.
fs.rm(dir, { recursive: true, force: true }) also succeeds on ENOENT, so this helper returns true even when the slide directory never existed. The DELETE /__slides/:id route then answers 200 and rewrites the folder manifest for a nonexistent slide.
💡 Suggested fix
export async function rmSlideDir(slidesRoot: string, slideId: string): Promise<boolean> {
if (!SLIDE_ID_RE.test(slideId)) return false;
const dir = path.resolve(slidesRoot, slideId);
if (!dir.startsWith(slidesRoot + path.sep)) return false;
try {
- await fs.rm(dir, { recursive: true, force: true });
+ await fs.access(dir);
+ await fs.rm(dir, { recursive: true });
return true;
- } catch {
+ } catch (err) {
+ if ((err as NodeJS.ErrnoException).code === 'ENOENT') return false;
return false;
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/vite/slide-ops.ts` around lines 14 - 23, rmSlideDir
currently returns true when fs.rm succeeds even if the directory didn't exist
(fs.rm with force ignores ENOENT); update rmSlideDir (referencing function
rmSlideDir, SLIDE_ID_RE and the fs.rm call) to first verify the resolved dir
exists (e.g., fs.stat or fs.access) and only call fs.rm if it exists, or catch
the error from fs.rm and treat ENOENT as a failure by returning false; ensure
the path safety check (dir.startsWith(slidesRoot + path.sep)) remains and that
the function returns false when the slide folder is absent.
Comment on lines
+56
to
+76
| const metaStart = source.search(/export\s+const\s+meta\b/); | ||
| if (metaStart !== -1) { | ||
| const eqIdx = source.indexOf('=', metaStart); | ||
| if (eqIdx === -1) return null; | ||
| const openBrace = source.indexOf('{', eqIdx); | ||
| if (openBrace === -1) return null; | ||
|
|
||
| let depth = 0; | ||
| let closeBrace = -1; | ||
| for (let i = openBrace; i < source.length; i++) { | ||
| const ch = source[i]; | ||
| if (ch === '{') depth++; | ||
| else if (ch === '}') { | ||
| depth--; | ||
| if (depth === 0) { | ||
| closeBrace = i; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (closeBrace === -1) return null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat packages/core/src/vite/slide-ops.tsRepository: 1weiho/open-slide
Length of output: 14653
The meta rewrite loop is not syntax-aware and corrupts valid input.
The brace-counting loop (lines 56–76) treats { and } inside strings, comments, and template literals as structural delimiters. A valid module like export const meta = { description: '}', title: 'x' } will terminate early at the } inside the string literal, truncating the meta object and losing subsequent properties.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/vite/slide-ops.ts` around lines 56 - 76, The brace-counting
loop that finds the meta object (using metaStart, eqIdx, openBrace and the
depth/closeBrace loop) is not syntax-aware and will break on braces inside
strings/comments/template literals; replace this ad-hoc loop with a syntax-aware
approach — either parse the module with a real JS/TS parser (e.g.,
acorn/TypeScript AST) and locate the ExportNamedDeclaration / VariableDeclarator
for the identifier "meta" to get its exact object range, or implement a minimal
state machine that skips over single/double-quoted strings, template literals
(including ${...}), and // and /* */ comments while counting braces so
closeBrace is determined correctly. Ensure the change uses the same
metaStart/eqIdx/openBrace anchors to preserve behavior where appropriate.
…les/, http/ After the comments/files-plugin splits, 60% of src/vite/ was pure logic with no Vite dependency, just consumed by Vite plugins. Move those out so the directory name matches what's actually inside: - editing/ — comments, edit-ops, revert-asset, slide-ops, babel-walk - files/ — assets, folders - http/ — request-guard - vite/ — only Vite plugins + config Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/src/editing/slide-ops.ts (1)
39-52: ⚡ Quick winTrim WHAT-style docblocks to match the TS/JS comment policy.
Most of these blocks explain implementation steps rather than hidden constraints. Keeping only surprising invariants/workarounds would better align with repo standards.
As per coding guidelines, "
**/*.{ts,tsx,js,jsx}: Default to writing no comments. Only add one when the WHY is non-obvious — a hidden constraint, a subtle invariant, a workaround for a specific bug, or behavior that would surprise a reader."Also applies to: 138-150, 241-253, 277-284, 338-344
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/editing/slide-ops.ts` around lines 39 - 52, The top-of-file WHAT-style docblock in packages/core/src/editing/slide-ops.ts (the explanatory block describing the "Rewrite (or insert) the `title` field..." strategy) should be shortened to a WHY-style comment: remove step-by-step implementation details and keep only any surprising invariants or workarounds that a future maintainer must know (e.g., safety conditions like returning null when the file shape is unexpected). Apply the same trimming to the other WHAT-style blocks referenced (around lines 138-150, 241-253, 277-284, 338-344), leaving concise comments that explain only non-obvious constraints or special-case behavior for functions/methods in this file (such as the function that finds `export const meta`, the brace-matching logic, or the `export default` fallback).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/editing/comments.ts`:
- Around line 26-30: The loop over source lines uses a single
MARKER_RE.exec(line) call so multiple markers on the same line are ignored;
change the logic in the loop that contains MARKER_RE and m to run exec
repeatedly for each line (e.g., reset MARKER_RE.lastIndex = 0 then use a while
loop like while ((m = MARKER_RE.exec(line)) !== null) to process every match)
and move the existing per-match handling inside that loop so each marker on the
same line is handled.
- Around line 44-47: The markerDeleteRegex function interpolates id directly
into a RegExp, allowing a malicious or malformed id to change regex behavior;
escape or validate id before building the RegExp in markerDeleteRegex: either
validate that id matches an allowed safe pattern (e.g., alphanumerics, hyphen,
underscore) and throw on invalid values, or run id through a regex-escape helper
that backslashes all special regex metacharacters before interpolation, then
construct the RegExp with the escaped value so the pattern is safe.
In `@packages/core/src/editing/edit-ops.ts`:
- Around line 90-101: collectTopLevelIdentifiers currently only adds import
bindings causing planAssetImport to possibly pick names that collide with
top-level const/function/class declarations; update collectTopLevelIdentifiers
to also scan ast.program.body for top-level VariableDeclaration,
FunctionDeclaration, and ClassDeclaration nodes, extract simple identifiers from
VariableDeclarator ids (skip/desugar destructuring), add those names to the
names Set, and remove the outdated comment about source-level scanning so the
function's behavior matches its documentation; reference
collectTopLevelIdentifiers and planAssetImport to ensure the new checks prevent
identifier collisions.
---
Nitpick comments:
In `@packages/core/src/editing/slide-ops.ts`:
- Around line 39-52: The top-of-file WHAT-style docblock in
packages/core/src/editing/slide-ops.ts (the explanatory block describing the
"Rewrite (or insert) the `title` field..." strategy) should be shortened to a
WHY-style comment: remove step-by-step implementation details and keep only any
surprising invariants or workarounds that a future maintainer must know (e.g.,
safety conditions like returning null when the file shape is unexpected). Apply
the same trimming to the other WHAT-style blocks referenced (around lines
138-150, 241-253, 277-284, 338-344), leaving concise comments that explain only
non-obvious constraints or special-case behavior for functions/methods in this
file (such as the function that finds `export const meta`, the brace-matching
logic, or the `export default` fallback).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3134702-a83c-4678-896e-08cce96ddcaa
📒 Files selected for processing (19)
packages/core/src/editing/babel-walk.tspackages/core/src/editing/comments.test.tspackages/core/src/editing/comments.tspackages/core/src/editing/edit-ops.test.tspackages/core/src/editing/edit-ops.tspackages/core/src/editing/revert-asset.test.tspackages/core/src/editing/revert-asset.tspackages/core/src/editing/slide-ops.test.tspackages/core/src/editing/slide-ops.tspackages/core/src/files/assets.test.tspackages/core/src/files/assets.tspackages/core/src/files/folders.test.tspackages/core/src/files/folders.tspackages/core/src/http/request-guard.test.tspackages/core/src/http/request-guard.tspackages/core/src/vite/api-plugin.tspackages/core/src/vite/design-plugin.tspackages/core/src/vite/loc-tags-plugin.tspackages/core/src/vite/notes-plugin.ts
✅ Files skipped from review due to trivial changes (4)
- packages/core/src/files/assets.test.ts
- packages/core/src/vite/loc-tags-plugin.ts
- packages/core/src/vite/design-plugin.ts
- packages/core/src/editing/revert-asset.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/vite/api-plugin.ts
Comment on lines
+26
to
+30
| for (let i = 0; i < lines.length; i++) { | ||
| const line = lines[i]; | ||
| MARKER_RE.lastIndex = 0; | ||
| const m = MARKER_RE.exec(line); | ||
| if (!m) continue; |
There was a problem hiding this comment.
Handle multiple markers on the same line.
At Line [29], exec runs once, so additional markers on the same source line are ignored.
Suggested fix
export function parseMarkers(source: string): Comment[] {
const comments: Comment[] = [];
const lines = source.split('\n');
for (let i = 0; i < lines.length; i++) {
const line = lines[i];
MARKER_RE.lastIndex = 0;
- const m = MARKER_RE.exec(line);
- if (!m) continue;
- const [, id, ts, textB64] = m;
- try {
- const payload = JSON.parse(b64urlDecode(textB64)) as { note: string; hint?: string };
- comments.push({ id, line: i + 1, ts, note: payload.note, hint: payload.hint });
- } catch {}
+ let m: RegExpExecArray | null;
+ while ((m = MARKER_RE.exec(line)) !== null) {
+ const [, id, ts, textB64] = m;
+ try {
+ const payload = JSON.parse(b64urlDecode(textB64)) as { note: string; hint?: string };
+ comments.push({ id, line: i + 1, ts, note: payload.note, hint: payload.hint });
+ } catch {}
+ }
}
return comments;
}📝 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.
Suggested change
| for (let i = 0; i < lines.length; i++) { | |
| const line = lines[i]; | |
| MARKER_RE.lastIndex = 0; | |
| const m = MARKER_RE.exec(line); | |
| if (!m) continue; | |
| export function parseMarkers(source: string): Comment[] { | |
| const comments: Comment[] = []; | |
| const lines = source.split('\n'); | |
| for (let i = 0; i < lines.length; i++) { | |
| const line = lines[i]; | |
| MARKER_RE.lastIndex = 0; | |
| let m: RegExpExecArray | null; | |
| while ((m = MARKER_RE.exec(line)) !== null) { | |
| const [, id, ts, textB64] = m; | |
| try { | |
| const payload = JSON.parse(b64urlDecode(textB64)) as { note: string; hint?: string }; | |
| comments.push({ id, line: i + 1, ts, note: payload.note, hint: payload.hint }); | |
| } catch {} | |
| } | |
| } | |
| return comments; | |
| } |
🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 29-29: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/editing/comments.ts` around lines 26 - 30, The loop over
source lines uses a single MARKER_RE.exec(line) call so multiple markers on the
same line are ignored; change the logic in the loop that contains MARKER_RE and
m to run exec repeatedly for each line (e.g., reset MARKER_RE.lastIndex = 0 then
use a while loop like while ((m = MARKER_RE.exec(line)) !== null) to process
every match) and move the existing per-match handling inside that loop so each
marker on the same line is handled.
Comment on lines
+44
to
+47
| export function markerDeleteRegex(id: string): RegExp { | ||
| return new RegExp( | ||
| `\\{\\/\\*\\s*@slide-comment\\s+id="${id}"\\s+ts="[^"]+"\\s+text="[A-Za-z0-9_\\-]+={0,2}"\\s*\\*\\/\\}`, | ||
| ); |
There was a problem hiding this comment.
Escape or validate id before building the deletion regex.
At Line [46], id is interpolated directly into new RegExp(...). A crafted ID can change regex behavior or cause regex errors.
Suggested fix
+function escapeRegExp(value: string): string {
+ return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+}
+
export function markerDeleteRegex(id: string): RegExp {
+ if (!/^c-[a-f0-9]{8}$/.test(id)) {
+ throw new Error('invalid comment id');
+ }
+ const safeId = escapeRegExp(id);
return new RegExp(
- `\\{\\/\\*\\s*`@slide-comment`\\s+id="${id}"\\s+ts="[^"]+"\\s+text="[A-Za-z0-9_\\-]+={0,2}"\\s*\\*\\/\\}`,
+ `\\{\\/\\*\\s*`@slide-comment`\\s+id="${safeId}"\\s+ts="[^"]+"\\s+text="[A-Za-z0-9_\\-]+={0,2}"\\s*\\*\\/\\}`,
);
}📝 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.
Suggested change
| export function markerDeleteRegex(id: string): RegExp { | |
| return new RegExp( | |
| `\\{\\/\\*\\s*@slide-comment\\s+id="${id}"\\s+ts="[^"]+"\\s+text="[A-Za-z0-9_\\-]+={0,2}"\\s*\\*\\/\\}`, | |
| ); | |
| function escapeRegExp(value: string): string { | |
| return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| } | |
| export function markerDeleteRegex(id: string): RegExp { | |
| if (!/^c-[a-f0-9]{8}$/.test(id)) { | |
| throw new Error('invalid comment id'); | |
| } | |
| const safeId = escapeRegExp(id); | |
| return new RegExp( | |
| `\\{\\/\\*\\s*`@slide-comment`\\s+id="${safeId}"\\s+ts="[^"]+"\\s+text="[A-Za-z0-9_\\-]+={0,2}"\\s*\\*\\/\\}`, | |
| ); | |
| } |
🧰 Tools
🪛 ast-grep (0.42.2)
[warning] 44-46: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
\\{\\/\\*\\s*@slide-comment\\s+id="${id}"\\s+ts="[^"]+"\\s+text="[A-Za-z0-9_\\-]+={0,2}"\\s*\\*\\/\\},
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/editing/comments.ts` around lines 44 - 47, The
markerDeleteRegex function interpolates id directly into a RegExp, allowing a
malicious or malformed id to change regex behavior; escape or validate id before
building the RegExp in markerDeleteRegex: either validate that id matches an
allowed safe pattern (e.g., alphanumerics, hyphen, underscore) and throw on
invalid values, or run id through a regex-escape helper that backslashes all
special regex metacharacters before interpolation, then construct the RegExp
with the escaped value so the pattern is safe.
Comment on lines
+90
to
+101
| function collectTopLevelIdentifiers(ast: t.File): Set<string> { | ||
| // Only need to avoid colliding with anything resolvable by JSX — | ||
| // import bindings cover the common case. Local consts/lets are | ||
| // handled by source-level identifier scanning below. | ||
| const names = new Set<string>(); | ||
| for (const imp of findImports(ast)) { | ||
| if (imp.defaultIdent) names.add(imp.defaultIdent); | ||
| for (const spec of imp.node.specifiers) { | ||
| if (!t.isImportDefaultSpecifier(spec)) names.add(spec.local.name); | ||
| } | ||
| } | ||
| return names; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current implementation scope and whether tests cover collision cases.
rg -n -C3 'function collectTopLevelIdentifiers|findImports\(ast\)|safeAssetIdentifier\(' packages/core/src/editing/edit-ops.ts
rg -n -C2 'safeAssetIdentifier|set-attr-asset|replace-placeholder-with-image' packages/core/src/editing/edit-ops.test.tsRepository: 1weiho/open-slide
Length of output: 5274
🏁 Script executed:
# Check if collectTopLevelIdentifiers is used elsewhere
rg -n 'collectTopLevelIdentifiers' packages/core/src/editing/
# Search for any "source-level" or "identifier scanning" logic in the codebase
rg -n 'source.*level|identifier.*scan' packages/core/src/editing/edit-ops.ts
# Check test integration scenarios with actual source code containing top-level declarations
rg -A10 'set-attr-asset.*assetPath' packages/core/src/editing/edit-ops.test.ts | head -50Repository: 1weiho/open-slide
Length of output: 2071
🏁 Script executed:
# Check for destructuring patterns in test cases and existing variable declarations
rg -n 'VariableDeclaration|FunctionDeclaration|ClassDeclaration' packages/core/src/editing/edit-ops.ts
# Look at actual test source code to see if destructuring is used
rg -B5 -A5 'const src = \[' packages/core/src/editing/edit-ops.test.ts | head -60
# Check if any exports or other statement types create bindings
rg -n 't\.is' packages/core/src/editing/edit-ops.ts | grep -i 'declaration\|binding\|export'Repository: 1weiho/open-slide
Length of output: 2998
🏁 Script executed:
# Check if destructuring patterns exist in the test sources and how common they are
rg -n 'const\s*\{|const\s*\[' packages/core/src/editing/edit-ops.test.ts | head -20
# Look at how the code handles destructuring in other contexts
rg -B2 -A2 'ObjectPattern|ArrayPattern' packages/core/src/editing/edit-ops.ts
# Verify if exported declarations create top-level bindings that need to be tracked
rg -n 'export\s*(const|function|class)' packages/core/src/editing/edit-ops.test.ts | head -10Repository: 1weiho/open-slide
Length of output: 671
Include non-import top-level bindings in collectTopLevelIdentifiers.
Currently only imports are tracked, so planAssetImport can generate an identifier that collides with existing top-level const/function/class declarations and fail edit application. Also, the outdated comment about "source-level identifier scanning" should be removed.
Suggested fix
function collectTopLevelIdentifiers(ast: t.File): Set<string> {
- // Only need to avoid colliding with anything resolvable by JSX —
- // import bindings cover the common case. Local consts/lets are
- // handled by source-level identifier scanning below.
const names = new Set<string>();
for (const imp of findImports(ast)) {
if (imp.defaultIdent) names.add(imp.defaultIdent);
for (const spec of imp.node.specifiers) {
if (!t.isImportDefaultSpecifier(spec)) names.add(spec.local.name);
}
}
+ for (const stmt of ast.program.body) {
+ if (t.isVariableDeclaration(stmt)) {
+ for (const decl of stmt.declarations) {
+ if (t.isIdentifier(decl.id)) names.add(decl.id.name);
+ }
+ } else if (t.isFunctionDeclaration(stmt) && stmt.id) {
+ names.add(stmt.id.name);
+ } else if (t.isClassDeclaration(stmt) && stmt.id) {
+ names.add(stmt.id.name);
+ }
+ }
return names;
}Note: This fix handles simple declarations but skips destructured variables (e.g., const { x } = ...); consider using a helper to extract identifiers from destructuring patterns if needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/editing/edit-ops.ts` around lines 90 - 101,
collectTopLevelIdentifiers currently only adds import bindings causing
planAssetImport to possibly pick names that collide with top-level
const/function/class declarations; update collectTopLevelIdentifiers to also
scan ast.program.body for top-level VariableDeclaration, FunctionDeclaration,
and ClassDeclaration nodes, extract simple identifiers from VariableDeclarator
ids (skip/desugar destructuring), add those names to the names Set, and remove
the outdated comment about source-level scanning so the function's behavior
matches its documentation; reference collectTopLevelIdentifiers and
planAssetImport to ensure the new checks prevent identifier collisions.
Owner
Author
|
@coderabbitai Fresh review of this refactor - any breaking changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the package's public entry point and any relevant export files to check for breaking changes. 🏁 Script executed: Length of output: 2515 🏁 Script executed: Length of output: 822 🏁 Script executed: Length of output: 1040
Public API check
Internal changes that are not breaking
VerdictThe |
api-plugin.ts was 750+ lines of inline middleware handlers; scanning "all endpoints" meant scrolling through every body. Move each prefix group into its own routes/*.ts with a register*(server, ctx) function. Each route file leads with a manifest comment of its endpoints, and apiPlugin itself becomes a 30-line dispatcher whose body is literally the list of route groups. - routes/context.ts ApiContext + readBody/json/resolveSlideEntryPath - routes/watchers.ts manifest + asset HMR pings - routes/edit.ts /__edit, /__edit/revert-asset, /__edit/batch - routes/comments.ts /__comments (list/add/delete) - routes/slides.ts /__slides/:id (PATCH/DELETE), reorder, pages/:i - routes/assets.ts /__assets/:scope CRUD + /usages - routes/svgl.ts /__svgl/search, /__svgl/svg - routes/folders.ts /__folders (list/create/assign/patch/delete) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/vite/routes/context.ts (1)
30-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMalformed JSON currently escalates to 500 instead of 400.
Line 39 rethrows raw parse errors; callers’ generic
catchpaths then emit 500. Invalid JSON is a client error and should return 400 consistently.💡 Minimal fix direction
+export class BadRequestError extends Error { + readonly status = 400; + constructor(message: string) { + super(message); + } +} + export async function readBody(req: Connect.IncomingMessage): Promise<unknown> { return await new Promise((resolve, reject) => { @@ try { resolve(JSON.parse(raw)); - } catch (e) { - reject(e); + } catch { + reject(new BadRequestError('invalid JSON body')); }Then in route-level
catchblocks, mapBadRequestErrorto400and keep500for everything else.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/vite/routes/context.ts` around lines 30 - 41, readBody currently rejects raw JSON parse errors causing upstream handlers to return 500; change the JSON parse failure path in readBody to reject/throw a typed client error (e.g. create or import and use BadRequestError) with a clear message and underlying parse message (e.g. "Invalid JSON body: <parse message>") instead of reject(e), and update route-level catch blocks to detect BadRequestError and return HTTP 400 while preserving 500 for other errors; target symbols: readBody and BadRequestError (or your app's equivalent client error class).
🧹 Nitpick comments (1)
packages/core/src/vite/api-plugin.ts (1)
13-15: ⚡ Quick winTrim module-header style comments and keep only hidden constraints.
This block documents WHAT the module does and navigation guidance; that should live in docs/tests, not inline route plumbing.
As per coding guidelines
**/*.{ts,tsx,js,jsx}: Default to writing no comments. Only add one when the WHY is non-obvious ... don't explain WHAT the code does ... don't write module-header descriptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/vite/api-plugin.ts` around lines 13 - 15, Remove the module-header style comment block at the top of api-plugin.ts that explains WHAT the module does and navigation guidance (the multi-line comment referencing routes/), leaving only any hidden constraints or machine/metadata comments (e.g., eslint-disable, `@internal`, TODOs) that are required for tooling; ensure no explanatory "what this file does" prose remains and only keep a brief WHY comment if there is a non-obvious constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/vite/routes/assets.ts`:
- Around line 164-177: The promise that accumulates request chunks in the upload
handler can hang when req.destroy() is called because it only listens for 'end'
and 'error'; update the Promise in the code that reads the request (the
anonymous async block that awaits new Promise<void>(...)) to also listen for the
'close' event and settle the promise (reject or resolve) when 'close' fires, and
ensure you remove all attached listeners ('data','end','error','close') in each
terminal path to avoid memory leaks; locate the Promise around req.on('data'...)
/ req.on('end'...) and add a 'close' handler that rejects or resolves
consistently with the oversized logic.
---
Duplicate comments:
In `@packages/core/src/vite/routes/context.ts`:
- Around line 30-41: readBody currently rejects raw JSON parse errors causing
upstream handlers to return 500; change the JSON parse failure path in readBody
to reject/throw a typed client error (e.g. create or import and use
BadRequestError) with a clear message and underlying parse message (e.g.
"Invalid JSON body: <parse message>") instead of reject(e), and update
route-level catch blocks to detect BadRequestError and return HTTP 400 while
preserving 500 for other errors; target symbols: readBody and BadRequestError
(or your app's equivalent client error class).
---
Nitpick comments:
In `@packages/core/src/vite/api-plugin.ts`:
- Around line 13-15: Remove the module-header style comment block at the top of
api-plugin.ts that explains WHAT the module does and navigation guidance (the
multi-line comment referencing routes/), leaving only any hidden constraints or
machine/metadata comments (e.g., eslint-disable, `@internal`, TODOs) that are
required for tooling; ensure no explanatory "what this file does" prose remains
and only keep a brief WHY comment if there is a non-obvious constraint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ce0c042-8769-4560-965b-629acb36d40d
📒 Files selected for processing (9)
packages/core/src/vite/api-plugin.tspackages/core/src/vite/routes/assets.tspackages/core/src/vite/routes/comments.tspackages/core/src/vite/routes/context.tspackages/core/src/vite/routes/edit.tspackages/core/src/vite/routes/folders.tspackages/core/src/vite/routes/slides.tspackages/core/src/vite/routes/svgl.tspackages/core/src/vite/routes/watchers.ts
Comment on lines
+164
to
+177
| await new Promise<void>((resolve, reject) => { | ||
| req.on('data', (c: Buffer) => { | ||
| total += c.length; | ||
| if (total > ASSET_MAX_BYTES) { | ||
| oversized = true; | ||
| req.destroy(); | ||
| return; | ||
| } | ||
| chunks.push(c); | ||
| }); | ||
| req.on('end', () => resolve()); | ||
| req.on('error', reject); | ||
| }); | ||
| if (oversized) return json(res, 413, { error: 'file too large' }); |
There was a problem hiding this comment.
Promise may hang indefinitely when req.destroy() is called.
When an oversized upload is detected (line 167-169), req.destroy() is called but the Promise only listens for 'end' and 'error' events. Node.js Readable.destroy() emits 'close' (not 'end' or 'error') when no error argument is passed, so the Promise never settles and the request hangs.
Proposed fix: listen for 'close' event
await new Promise<void>((resolve, reject) => {
req.on('data', (c: Buffer) => {
total += c.length;
if (total > ASSET_MAX_BYTES) {
oversized = true;
req.destroy();
return;
}
chunks.push(c);
});
req.on('end', () => resolve());
req.on('error', reject);
+ req.on('close', () => resolve());
});📝 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.
Suggested change
| await new Promise<void>((resolve, reject) => { | |
| req.on('data', (c: Buffer) => { | |
| total += c.length; | |
| if (total > ASSET_MAX_BYTES) { | |
| oversized = true; | |
| req.destroy(); | |
| return; | |
| } | |
| chunks.push(c); | |
| }); | |
| req.on('end', () => resolve()); | |
| req.on('error', reject); | |
| }); | |
| if (oversized) return json(res, 413, { error: 'file too large' }); | |
| await new Promise<void>((resolve, reject) => { | |
| req.on('data', (c: Buffer) => { | |
| total += c.length; | |
| if (total > ASSET_MAX_BYTES) { | |
| oversized = true; | |
| req.destroy(); | |
| return; | |
| } | |
| chunks.push(c); | |
| }); | |
| req.on('end', () => resolve()); | |
| req.on('error', reject); | |
| req.on('close', () => resolve()); | |
| }); | |
| if (oversized) return json(res, 413, { error: 'file too large' }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/vite/routes/assets.ts` around lines 164 - 177, The promise
that accumulates request chunks in the upload handler can hang when
req.destroy() is called because it only listens for 'end' and 'error'; update
the Promise in the code that reads the request (the anonymous async block that
awaits new Promise<void>(...)) to also listen for the 'close' event and settle
the promise (reject or resolve) when 'close' fires, and ensure you remove all
attached listeners ('data','end','error','close') in each terminal path to avoid
memory leaks; locate the Promise around req.on('data'...) / req.on('end'...) and
add a 'close' handler that rejects or resolves consistently with the oversized
logic.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
This was referenced May 23, 2026
This was referenced Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
packages/core/src/vite/comments-plugin.tshad grown to 1.8k lines covering comment markers, generic JSX source edits, asset revert, and several dev-server HTTP endpoints. This refactor splits it into single-purpose modules and consolidates the HTTP endpoints behind one Vite plugin — no behavior changes, just structure.New files (under
packages/core/src/vite/)comments.ts— marker parsing, b64url helpers, JSX-insertion planningedit-ops.ts—applyEdit+ splice builders + asset-import planning +safeAssetIdentifierrevert-asset.ts—applyRevertAsset,findAssetUsages, placeholder helpersapi-plugin.ts— single Vite plugin registering/__comments/*and/__edit/*middlewares with sharedreadBody/json/resolveSlidePathhelpersTouched
babel-walk.ts— gains a sharedparseSourcehelper used by all three new logic modulesconfig.ts— swapscommentsPlugin→apiPluginfiles-plugin.ts— importsfindAssetUsagesfrom./revert-asset.tscomments.test.ts,edit-ops.test.ts,revert-asset.test.ts,request-guard.test.tscomments-plugin.tsandcomments-plugin.test.tsNotes for the reviewer
apps/web/lib/layout.shared.tsx<img>andrequest-guard.tsoptional-chain suggestion) are not new and not touched here./__commentsand/__editendpoint groups are unified inapi-plugin.ts. The other plugins (notes-plugin,files-plugin,design-plugin, …) still own their own middlewares — happy to fold those intoapi-plugin.tsin a follow-up if desired.patchsince there's no public API change.Test plan
pnpm typecheckclean across the graphpnpm test— 207 tests pass (12 files), including the four new split test filespnpm check— no errors (2 pre-existing warnings unchanged)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Refactor