-
-
Notifications
You must be signed in to change notification settings - Fork 370
refactor(core): split comments-plugin into focused modules #135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9abb1b0
c10b573
a34ea3d
498b48f
0d72d5c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@open-slide/core': patch | ||
| --- | ||
|
|
||
| Refactor the dev-server Vite plugins: extract pure logic out of the monolithic `comments-plugin` and `files-plugin` into per-domain modules (`editing/`, `files/`, `http/`), and consolidate every `/__*` HTTP endpoint into a single `api-plugin` whose route handlers live under `vite/routes/` (one file per endpoint group with a manifest comment up top). No public API change. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { b64urlDecode, b64urlEncode, parseMarkers } from './comments.ts'; | ||
|
|
||
| describe('b64url encoding', () => { | ||
| it('round-trips arbitrary unicode strings', () => { | ||
| const samples = ['hello', '안녕하세요', '🎉🎊', 'a/b+c=d', JSON.stringify({ note: 'hi' })]; | ||
| for (const s of samples) { | ||
| expect(b64urlDecode(b64urlEncode(s))).toBe(s); | ||
| } | ||
| }); | ||
|
|
||
| it('produces url-safe output (no +, /, or =)', () => { | ||
| const encoded = b64urlEncode('subject?with/lots+of==special chars'); | ||
| expect(encoded).not.toMatch(/[+/=]/); | ||
| }); | ||
|
|
||
| it('decodes the empty string', () => { | ||
| expect(b64urlDecode('')).toBe(''); | ||
| }); | ||
| }); | ||
|
|
||
| describe('parseMarkers', () => { | ||
| it('returns no comments when the source has no markers', () => { | ||
| expect(parseMarkers('const a = 1;\nexport default [];\n')).toEqual([]); | ||
| }); | ||
|
|
||
| it('extracts a single marker with its line number and decoded note', () => { | ||
| const payload = b64urlEncode(JSON.stringify({ note: 'tighten this' })); | ||
| const ts = '2026-04-25T00:00:00.000Z'; | ||
| const id = 'c-deadbeef'; | ||
| const source = [ | ||
| 'export default [() => (', | ||
| ' <div>', | ||
| ` {/* @slide-comment id="${id}" ts="${ts}" text="${payload}" */}`, | ||
| ' hi', | ||
| ' </div>', | ||
| ')];', | ||
| '', | ||
| ].join('\n'); | ||
|
|
||
| const comments = parseMarkers(source); | ||
| expect(comments).toEqual([{ id, line: 3, ts, note: 'tighten this', hint: undefined }]); | ||
| }); | ||
|
|
||
| it('extracts a hint when the marker payload includes one', () => { | ||
| const payload = b64urlEncode(JSON.stringify({ note: 'fix', hint: 'h1' })); | ||
| const source = `{/* @slide-comment id="c-12345678" ts="2026-04-25T00:00:00.000Z" text="${payload}" */}`; | ||
| const [c] = parseMarkers(source); | ||
| expect(c.hint).toBe('h1'); | ||
| expect(c.note).toBe('fix'); | ||
| }); | ||
|
|
||
| it('skips markers whose payload is malformed', () => { | ||
| const source = | ||
| '{/* @slide-comment id="c-12345678" ts="2026-04-25T00:00:00.000Z" text="not_json" */}'; | ||
| expect(parseMarkers(source)).toEqual([]); | ||
| }); | ||
|
|
||
| it('extracts multiple markers from different lines', () => { | ||
| const p1 = b64urlEncode(JSON.stringify({ note: 'one' })); | ||
| const p2 = b64urlEncode(JSON.stringify({ note: 'two' })); | ||
| const source = [ | ||
| `{/* @slide-comment id="c-aaaaaaaa" ts="2026-04-25T00:00:00.000Z" text="${p1}" */}`, | ||
| 'const x = 1;', | ||
| `{/* @slide-comment id="c-bbbbbbbb" ts="2026-04-25T00:00:00.000Z" text="${p2}" */}`, | ||
| ].join('\n'); | ||
|
|
||
| const comments = parseMarkers(source); | ||
| expect(comments.map((c) => c.note)).toEqual(['one', 'two']); | ||
| expect(comments.map((c) => c.line)).toEqual([1, 3]); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,137 @@ | ||||||||||||||||||||||||||||||||||||
| import { randomUUID } from 'node:crypto'; | ||||||||||||||||||||||||||||||||||||
| import * as t from '@babel/types'; | ||||||||||||||||||||||||||||||||||||
| import { parseSource, walkJsx } from './babel-walk.ts'; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const MARKER_RE = | ||||||||||||||||||||||||||||||||||||
| /\{\/\*\s*@slide-comment\s+id="(c-[a-f0-9]+)"\s+ts="([^"]+)"\s+text="([A-Za-z0-9_-]+={0,2})"\s*\*\/\}/g; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| export type Comment = { id: string; line: number; ts: string; note: string; hint?: string }; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| export function b64urlEncode(s: string): string { | ||||||||||||||||||||||||||||||||||||
| return Buffer.from(s, 'utf8') | ||||||||||||||||||||||||||||||||||||
| .toString('base64') | ||||||||||||||||||||||||||||||||||||
| .replace(/\+/g, '-') | ||||||||||||||||||||||||||||||||||||
| .replace(/\//g, '_') | ||||||||||||||||||||||||||||||||||||
| .replace(/=+$/, ''); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| export function b64urlDecode(s: string): string { | ||||||||||||||||||||||||||||||||||||
| const pad = s.length % 4 === 0 ? '' : '='.repeat(4 - (s.length % 4)); | ||||||||||||||||||||||||||||||||||||
| return Buffer.from(s.replace(/-/g, '+').replace(/_/g, '/') + pad, 'base64').toString('utf8'); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| 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 {} | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return comments; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| export function newCommentId(): string { | ||||||||||||||||||||||||||||||||||||
| return `c-${randomUUID().replace(/-/g, '').slice(0, 8)}`; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| 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*\\*\\/\\}`, | ||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+44
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Escape or validate At Line [46], 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
Suggested change
🧰 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. (regexp-from-variable) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // We always splice the marker as the first child of a JSX container. | ||||||||||||||||||||||||||||||||||||
| // A JSX-comment-like token outside JSX context (e.g. as the body of | ||||||||||||||||||||||||||||||||||||
| // `() => ( <Foo/> )`) is parsed as an empty object literal and breaks | ||||||||||||||||||||||||||||||||||||
| // the surrounding expression. | ||||||||||||||||||||||||||||||||||||
| export type InsertionPlan = { offset: number; indent: string }; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| function lineToOffset(source: string, line: number): number { | ||||||||||||||||||||||||||||||||||||
| let off = 0; | ||||||||||||||||||||||||||||||||||||
| for (let l = 1; l < line; l++) { | ||||||||||||||||||||||||||||||||||||
| const nl = source.indexOf('\n', off); | ||||||||||||||||||||||||||||||||||||
| if (nl === -1) return source.length; | ||||||||||||||||||||||||||||||||||||
| off = nl + 1; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return off; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| function lineIndent(source: string, lineNumber: number): string { | ||||||||||||||||||||||||||||||||||||
| const start = lineToOffset(source, lineNumber); | ||||||||||||||||||||||||||||||||||||
| const m = source.slice(start, start + 200).match(/^[ \t]*/); | ||||||||||||||||||||||||||||||||||||
| return m?.[0] ?? ''; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| type JsxContainer = t.JSXElement | t.JSXFragment; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| function findJsxAncestors(ast: t.Node, line: number, column: number): JsxContainer[] { | ||||||||||||||||||||||||||||||||||||
| const hits: { node: JsxContainer; size: number }[] = []; | ||||||||||||||||||||||||||||||||||||
| walkJsx(ast, (n) => { | ||||||||||||||||||||||||||||||||||||
| if (!n.loc || (!t.isJSXElement(n) && !t.isJSXFragment(n))) return; | ||||||||||||||||||||||||||||||||||||
| const s = n.loc.start; | ||||||||||||||||||||||||||||||||||||
| const e = n.loc.end; | ||||||||||||||||||||||||||||||||||||
| const afterStart = line > s.line || (line === s.line && column >= s.column); | ||||||||||||||||||||||||||||||||||||
| const beforeEnd = line < e.line || (line === e.line && column < e.column); | ||||||||||||||||||||||||||||||||||||
| if (afterStart && beforeEnd) { | ||||||||||||||||||||||||||||||||||||
| hits.push({ node: n, size: (n.end ?? 0) - (n.start ?? 0) }); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||
| hits.sort((a, b) => a.size - b.size); | ||||||||||||||||||||||||||||||||||||
| return hits.map((h) => h.node); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| function planInsertion(source: string, target: JsxContainer): InsertionPlan | null { | ||||||||||||||||||||||||||||||||||||
| if (t.isJSXFragment(target)) { | ||||||||||||||||||||||||||||||||||||
| const opening = target.openingFragment; | ||||||||||||||||||||||||||||||||||||
| const startLine = target.loc?.start.line ?? 1; | ||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||
| offset: opening.end ?? 0, | ||||||||||||||||||||||||||||||||||||
| indent: `${lineIndent(source, startLine)} `, | ||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| if (t.isJSXElement(target)) { | ||||||||||||||||||||||||||||||||||||
| const opening = target.openingElement; | ||||||||||||||||||||||||||||||||||||
| if (opening.selfClosing) return null; | ||||||||||||||||||||||||||||||||||||
| const startLine = target.loc?.start.line ?? 1; | ||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||
| offset: opening.end ?? 0, | ||||||||||||||||||||||||||||||||||||
| indent: `${lineIndent(source, startLine)} `, | ||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Walk innermost → outermost looking for the first JSX container we | ||||||||||||||||||||||||||||||||||||
| // can insert *inside* (not self-closing). Self-closing elements like | ||||||||||||||||||||||||||||||||||||
| // `<img/>` get hoisted to their nearest non-self-closing ancestor. | ||||||||||||||||||||||||||||||||||||
| export function findInsertion( | ||||||||||||||||||||||||||||||||||||
| source: string, | ||||||||||||||||||||||||||||||||||||
| line: number, | ||||||||||||||||||||||||||||||||||||
| column: number | undefined, | ||||||||||||||||||||||||||||||||||||
| ): InsertionPlan | null { | ||||||||||||||||||||||||||||||||||||
| const ast = parseSource(source); | ||||||||||||||||||||||||||||||||||||
| if (!ast) return null; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const col = column ?? 0; | ||||||||||||||||||||||||||||||||||||
| const ancestors = findJsxAncestors(ast, line, col); | ||||||||||||||||||||||||||||||||||||
| for (const node of ancestors) { | ||||||||||||||||||||||||||||||||||||
| const plan = planInsertion(source, node); | ||||||||||||||||||||||||||||||||||||
| if (plan) return plan; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| export function offsetToLine(source: string, offset: number): number { | ||||||||||||||||||||||||||||||||||||
| let line = 1; | ||||||||||||||||||||||||||||||||||||
| for (let i = 0; i < offset && i < source.length; i++) { | ||||||||||||||||||||||||||||||||||||
| if (source[i] === '\n') line++; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return line; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle multiple markers on the same line.
At Line [29],
execruns 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
🧰 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