fix: resolve code review issues — generation counter, XSS sanitizer, ARIA, zone refs, and editor-operations type safety

packages/editor/src/components/Editor.vue

@@ -26,11 +26,11 @@ import {
   splitMarkdownIntoBlocks,
 } from "@/lib/block-parser"
 import {
+  moveBlock,
   deleteIfEmpty as opDeleteIfEmpty,
   indentBlock as opIndentBlock,
   insertBlock as opInsertBlock,
   mergePrevious as opMergePrevious,
-  moveBlock,
   outdentBlock as opOutdentBlock,
   splitBlocks as opSplitBlocks,
 } from "@/lib/editor-operations"
@@ -76,18 +76,16 @@ const themeCSSVars = computed(() => {
   return themeToCSSVars(resolved)
 })
 
-// Tracks whether an internal content update is in flight. When the watcher
-// fires for a content.value change that originated from onBlockContentChange
-// (not from an external source), it resets and skips re-parsing.
-// Using a boolean avoids the counter-desync problem: if Vue collapses N
-// writes to content.value into one watcher invocation, resetting to false
-// on every skip handles any number of batched internal updates.
-// Edge case: if two onBlockContentChange calls fire in separate microtask
-// turns (the first's nextTick clears the flag before the second's watcher
-// fires), the boolean would prematurely reset. This doesn't happen in the
-// current call graph (all internal changes are synchronous), but is noted
-// for future refactoring.
-let internalUpdatePending = false
+/**
+ * Generation counter disambiguates internal content updates from external
+ * ones. onBlockContentChange increments it synchronously before setting
+ * content.value; the watcher compares it against lastContentGeneration.
+ * If Vue collapses N writes into one watcher invocation, the generational
+ * gap is still detected. Unlike a boolean, this survives interleaved async
+ * calls (e.g. resolveAsset then nextTick then another content change).
+ */
+let contentGeneration = 0
+let lastContentGeneration = 0
 
 const blocks = ref<EditorBlockData[]>([])
 
@@ -97,7 +95,9 @@ let announceTimer: ReturnType<typeof setTimeout> | null = null
 function announce(msg: string) {
   liveAnnouncement.value = msg
   if (announceTimer) clearTimeout(announceTimer)
-  announceTimer = setTimeout(() => { liveAnnouncement.value = "" }, 2000)
+  announceTimer = setTimeout(() => {
+    liveAnnouncement.value = ""
+  }, 2000)
 }
 
 const registry = useFocusRegistry()
@@ -105,14 +105,29 @@ const registry = useFocusRegistry()
 const log = createLogger("Editor", props.debug)
 
 const blockRefs = new Map<string, { setContent: (md: string) => void }>()
-const zoneRefs: HTMLElement[] = []
+const zoneRefs = new Map<number, HTMLElement>()
 
+/**
+ * Focus the insertion zone at the given index. Used for keyboard
+ * navigation when the cursor exits the first or last block.
+ */
 function focusZone(index: number) {
-  const zone = zoneRefs[index]
+  const zone = zoneRefs.get(index)
   if (!zone) return
   zone.focus()
 }
 
+/**
+ * Push content into a block's ProseMirror view, but only if the block
+ * still exists in the model. Without this guard, an async content
+ * change that resolves after the block is removed could schedule a
+ * doc replacement in a component about to be unmounted.
+ */
+function syncBlockContent(id: string, content: string) {
+  if (!blocks.value.some((b) => b.id === id)) return
+  blockRefs.get(id)?.setContent(content)
+}
+
 // ── Drag-to-reorder ────────────────────────────────────────────────
 
 const dragState = ref<{
@@ -167,8 +182,7 @@ function onDropOnZone(zoneIndex: number) {
 
   // Zone i = insert before block i.  In post-removal coordinates:
   // if fromIndex < zoneIndex, the zone shifts left by 1.
-  const targetIndex =
-    state.fromIndex < zoneIndex ? zoneIndex - 1 : zoneIndex
+  const targetIndex = state.fromIndex < zoneIndex ? zoneIndex - 1 : zoneIndex
 
   if (state.fromIndex === targetIndex) {
     dragState.value = null
@@ -211,7 +225,7 @@ function applyOperation(op: EditorOperation): void {
       const block = blocks.value[op.index]
       if (!block) return
       block.content = op.beforeContent
-      blockRefs.get(block.id)?.setContent(op.beforeContent)
+      syncBlockContent(block.id, op.beforeContent)
       blocks.value.splice(op.index + 1, 0, {
         id: op.splitBlockId,
         depth: op.depth,
@@ -224,7 +238,7 @@ function applyOperation(op: EditorOperation): void {
       const prev = blocks.value[op.index - 1]
       if (prev) {
         prev.content = op.prevContent + op.appendedContent
-        blockRefs.get(prev.id)?.setContent(prev.content)
+        syncBlockContent(prev.id, prev.content)
       }
       blocks.value.splice(op.index, 1)
       // Cursor lands at the join point: PM's 1-indexed position after
@@ -273,10 +287,12 @@ function applyOperation(op: EditorOperation): void {
       break
     }
     case "set-block-content": {
-      const setBlock = blocks.value.find((b: EditorBlockData) => b.id === op.blockId)
+      const setBlock = blocks.value.find(
+        (b: EditorBlockData) => b.id === op.blockId,
+      )
       if (setBlock) {
         setBlock.content = op.newContent
-        blockRefs.get(setBlock.id)?.setContent(op.newContent)
+        syncBlockContent(setBlock.id, op.newContent)
       }
       if (setBlock) focusInfo = { id: setBlock.id, pos: "end" }
       break
@@ -350,8 +366,8 @@ function parseContent(md: string | undefined) {
 watch(
   () => content.value,
   (val) => {
-    if (internalUpdatePending) {
-      internalUpdatePending = false
+    if (contentGeneration !== lastContentGeneration) {
+      lastContentGeneration = contentGeneration
       return
     }
     history.clear()
@@ -364,10 +380,10 @@ watch(
 
 // Serialize and emit when any block's content changes.
 function onBlockContentChange() {
-  internalUpdatePending = true
+  contentGeneration++
   content.value = serializeBlocks(blocks.value)
   nextTick(() => {
-    internalUpdatePending = false
+    lastContentGeneration = contentGeneration
   })
 }
 
@@ -487,7 +503,7 @@ function onMergePrevious(index: number) {
   // Directly update the prev block's PM + model so the debounced watch
   // (which fires ~50ms later) sees currentContent === newContent and is a
   // no-op — preventing a cursor-resetting doc replacement.
-  blockRefs.get(prev.id)?.setContent(result.mergedContent)
+  syncBlockContent(prev.id, result.mergedContent)
 
   // Defensively ensure the result block's content matches mergedContent,
   // even if mergePrevious's implementation evolves. Without this, the new
@@ -755,7 +771,7 @@ defineExpose({
     <div class="editor-body" @dragover.prevent>
       <template v-for="(block, i) in blocks" :key="block.id">
         <EditorInsertionZone
-        :ref="(el: any) => { if (el) zoneRefs[i] = el; else delete zoneRefs[i] }"
+        :ref="(el: any) => { if (el) zoneRefs.set(i, el); else zoneRefs.delete(i) }"
         :class="dropZoneIndex === i ? 'bg-(--ui-primary) rounded' : ''"
         @activate="(content: string) => onZoneActivate(i, content)"
         @focus="onZoneFocus(i)"
@@ -822,7 +838,7 @@ defineExpose({
       </div>
     </template>
     <EditorInsertionZone
-      :ref="(el: any) => { if (el) zoneRefs[blocks.length] = el; else delete zoneRefs[blocks.length] }"
+      :ref="(el: any) => { if (el) zoneRefs.set(blocks.length, el); else zoneRefs.delete(blocks.length) }"
       :class="dropZoneIndex === blocks.length ? 'bg-(--ui-primary) rounded' : ''"
       @activate="(content: string) => onZoneActivate(blocks.length, content)"
       @focus="onZoneFocus(blocks.length)"

packages/editor/src/components/EditorBlock.vue

@@ -11,9 +11,9 @@ import {
 import { EditorView } from "prosemirror-view"
 import {
   computed,
+  nextTick,
   onMounted,
   onUnmounted,
-  nextTick,
   ref,
   useTemplateRef,
   watch,
@@ -89,8 +89,10 @@ const blockAriaLabel = computed(() => {
   if (firstLine.startsWith("> [") && firstLine.endsWith("]")) return "Callout"
   if (firstLine.startsWith(">")) return "Blockquote"
   if (firstLine.startsWith("|")) return "Table"
-  if (/^#{1,6}\s/.test(firstLine)) return `Heading ${firstLine.match(/^#+/)?.[0]?.length ?? 0}`
-  if (/^(TODO|DOING|DONE|LATER|NOW|WAITING|CANCELLED)\b/.test(firstLine)) return "Task"
+  if (/^#{1,6}\s/.test(firstLine))
+    return `Heading ${firstLine.match(/^#+/)?.[0]?.length ?? 0}`
+  if (/^(TODO|DOING|DONE|LATER|NOW|WAITING|CANCELLED)\b/.test(firstLine))
+    return "Task"
   if (/^\d+[.)]\s/.test(firstLine)) return "Ordered list"
   if (/^[-*+]\s/.test(firstLine)) return "Unordered list"
   return "Paragraph"
@@ -142,12 +144,14 @@ const emit = defineEmits<{
   focus: [payload: { view: EditorView; handlers: FormattingHandlers }]
   blur: []
   "selection-change": [payload: { from: number; to: number; empty: boolean }]
-  "content-change-op": [payload: {
-    blockId: string
-    previousContent: string
-    newContent: string
-    timestamp: number
-  }]
+  "content-change-op": [
+    payload: {
+      blockId: string
+      previousContent: string
+      newContent: string
+      timestamp: number
+    },
+  ]
   error: [payload: { code: string; message?: string; blockId?: string }]
   "page-ref-clicked": [payload: { pageName: string; alias?: string }]
   "block-ref-clicked": [payload: { blockId: string }]
@@ -679,18 +683,15 @@ onMounted(() => {
           const domTarget = event.target as HTMLElement | null
           const pageRefEl = domTarget?.closest?.(".md-page-ref")
           if (pageRefEl instanceof HTMLElement) {
-            const pageName =
-              pageRefEl.getAttribute("data-page-name") ?? ""
-            const alias =
-              pageRefEl.getAttribute("data-alias") ?? undefined
+            const pageName = pageRefEl.getAttribute("data-page-name") ?? ""
+            const alias = pageRefEl.getAttribute("data-alias") ?? undefined
             event.preventDefault()
             emit("page-ref-clicked", { pageName, alias })
             return true
           }
           const blockRefEl = domTarget?.closest?.(".md-block-ref")
           if (blockRefEl instanceof HTMLElement) {
-            const blockId =
-              blockRefEl.getAttribute("data-block-id") ?? ""
+            const blockId = blockRefEl.getAttribute("data-block-id") ?? ""
             event.preventDefault()
             emit("block-ref-clicked", { blockId })
             return true
@@ -707,6 +708,8 @@ onMounted(() => {
       },
     })
 
+    view.dom.setAttribute("aria-label", blockAriaLabel.value)
+
     if (props.focused && view) {
       view.dispatch(view.state.tr.setMeta("decorations:focus", true))
       focusAt(props.cursorPosition)
@@ -798,6 +801,13 @@ watch(blockAriaLabel, (label) => {
 })
 
 defineExpose({
+  /**
+   * Live ProseMirror EditorView reference.
+   *
+   * ⚠️ Dispatch via this view directly bypasses the operation history
+   * (undo/redo). Prefer setContent() or emit("change") for programmatic
+   * updates. Only use raw dispatch when you know history is irrelevant.
+   */
   get view() {
     return view
   },

packages/editor/src/lib/__tests__/safe-image-src.test.ts

@@ -0,0 +1,46 @@
+import { describe, expect, it } from "vitest"
+import { safeImageSrc } from "@/lib/safe-image-src"
+
+describe("safeImageSrc", () => {
+  it("allows http URLs", () => {
+    expect(safeImageSrc("http://example.com/photo.jpg")).toBe(
+      "http://example.com/photo.jpg",
+    )
+  })
+
+  it("allows https URLs", () => {
+    expect(safeImageSrc("https://cdn.example.com/images/hero.webp")).toBe(
+      "https://cdn.example.com/images/hero.webp",
+    )
+  })
+
+  it("allows data:image URIs", () => {
+    const uri =
+      "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk"
+    expect(safeImageSrc(uri)).toBe(uri)
+  })
+
+  it("rejects javascript: URLs", () => {
+    expect(safeImageSrc("javascript:alert(1)")).toBeNull()
+  })
+
+  it("rejects data:text URIs", () => {
+    expect(safeImageSrc("data:text/html,<script>alert(1)</script>")).toBeNull()
+  })
+
+  it("rejects file: URLs", () => {
+    expect(safeImageSrc("file:///etc/passwd")).toBeNull()
+  })
+
+  it("rejects empty string", () => {
+    expect(safeImageSrc("")).toBeNull()
+  })
+
+  it("rejects protocol-relative URL without scheme", () => {
+    expect(safeImageSrc("//evil.com/xss.svg")).toBeNull()
+  })
+
+  it("rejects blob: URLs", () => {
+    expect(safeImageSrc("blob:https://example.com/uuid")).toBeNull()
+  })
+})

packages/editor/src/lib/editor-operations.ts

@@ -1,4 +1,4 @@
-import type { EditorBlockData } from "@/lib/block-parser"
+import { type EditorBlockData, generateBlockId } from "@/lib/block-parser"
 
 export interface SplitResult {
   blocks: EditorBlockData[]
@@ -30,11 +30,20 @@ export function splitBlocks(
   index: number,
   before: string,
   after: string,
-  idFn: () => string = generateId,
+  idFn: () => string = generateBlockId,
 ): SplitResult {
-  if (index < 0 || index >= blocks.length) return { blocks: [...blocks], newBlock: null }
-  const newBlocks = blocks.map((b, i) => (i === index ? { ...b, content: before } : { ...b }))
-  const newBlock: EditorBlockData = { id: idFn(), depth: newBlocks[index]!.depth, content: after }
+  if (index < 0 || index >= blocks.length)
+    return { blocks: [...blocks], newBlock: null }
+  const block = blocks[index]
+  if (!block) return { blocks: [...blocks], newBlock: null }
+  const newBlock: EditorBlockData = {
+    id: idFn(),
+    depth: block.depth,
+    content: after,
+  }
+  const newBlocks = blocks.map((b, i) =>
+    i === index ? { ...b, content: before } : { ...b },
+  )
   newBlocks.splice(index + 1, 0, newBlock)
   return { blocks: newBlocks, newBlock }
 }
@@ -42,8 +51,15 @@ export function splitBlocks(
 /**
  * Merge block at `index` into its predecessor.
  */
-export function mergePrevious(blocks: EditorBlockData[], index: number): MergeResult {
-  const result: MergeResult = { blocks: [...blocks], mergedContent: "", joinPos: 0 }
+export function mergePrevious(
+  blocks: EditorBlockData[],
+  index: number,
+): MergeResult {
+  const result: MergeResult = {
+    blocks: [...blocks],
+    mergedContent: "",
+    joinPos: 0,
+  }
   if (index <= 0 || index >= blocks.length) return result
   const current = blocks[index]
   const prev = blocks[index - 1]
@@ -52,7 +68,9 @@ export function mergePrevious(blocks: EditorBlockData[], index: number): MergeRe
   const prevContent = prev.content
   const appendedContent = current.content
   const joinPos = prevContent.length > 0 ? prevContent.length + 1 : 0
-  const mergedContent = prevContent ? `${prevContent}${appendedContent}` : appendedContent
+  const mergedContent = prevContent
+    ? `${prevContent}${appendedContent}`
+    : appendedContent
 
   const newBlocks = blocks.map((b, i) => {
     if (i === index - 1) return { ...b, content: mergedContent }
@@ -66,15 +84,18 @@ export function mergePrevious(blocks: EditorBlockData[], index: number): MergeRe
  * Delete the block at `index`. Returns the ID of the block to focus next.
  * Does nothing if there is only one block.
  */
-export function deleteIfEmpty(blocks: EditorBlockData[], index: number): DeleteResult {
+export function deleteIfEmpty(
+  blocks: EditorBlockData[],
+  index: number,
+): DeleteResult {
   const result: DeleteResult = { blocks: [...blocks], nextId: null }
   if (blocks.length <= 1) return result
   if (index < 0 || index >= blocks.length) return result
 
   const nextId =
     index < blocks.length - 1
-      ? blocks[index + 1]?.id ?? null
-      : blocks[index - 1]?.id ?? null
+      ? (blocks[index + 1]?.id ?? null)
+      : (blocks[index - 1]?.id ?? null)
 
   const newBlocks = blocks.filter((_, i) => i !== index)
   return { blocks: newBlocks, nextId }
@@ -84,10 +105,15 @@ export function deleteIfEmpty(blocks: EditorBlockData[], index: number): DeleteR
  * Indent the block at `index` and its children (blocks at higher depth
  * immediately following it).
  */
-export function indentBlock(blocks: EditorBlockData[], index: number): IndentResult {
-  if (index < 0 || index >= blocks.length) return { blocks: [...blocks], childIndices: [] }
+export function indentBlock(
+  blocks: EditorBlockData[],
+  index: number,
+): IndentResult {
+  if (index < 0 || index >= blocks.length)
+    return { blocks: [...blocks], childIndices: [] }
   const block = blocks[index]
-  if (!block || block.depth >= 10) return { blocks: [...blocks], childIndices: [] }
+  if (!block || block.depth >= 10)
+    return { blocks: [...blocks], childIndices: [] }
 
   const parentDepth = block.depth
   const childIndices: number[] = []
@@ -110,10 +136,15 @@ export function indentBlock(blocks: EditorBlockData[], index: number): IndentRes
 /**
  * Outdent the block at `index` and its children.
  */
-export function outdentBlock(blocks: EditorBlockData[], index: number): IndentResult {
-  if (index < 0 || index >= blocks.length) return { blocks: [...blocks], childIndices: [] }
+export function outdentBlock(
+  blocks: EditorBlockData[],
+  index: number,
+): IndentResult {
+  if (index < 0 || index >= blocks.length)
+    return { blocks: [...blocks], childIndices: [] }
   const block = blocks[index]
-  if (!block || block.depth <= 0) return { blocks: [...blocks], childIndices: [] }
+  if (!block || block.depth <= 0)
+    return { blocks: [...blocks], childIndices: [] }
 
   const parentDepth = block.depth
   const childIndices: number[] = []
@@ -141,7 +172,7 @@ export function insertBlock(
   index: number,
   content: string,
   depth: number,
-  idFn: () => string = generateId,
+  idFn: () => string = generateBlockId,
 ): { blocks: EditorBlockData[]; newBlock: EditorBlockData } {
   const newBlock: EditorBlockData = { id: idFn(), depth, content }
   const newBlocks = [...blocks]
@@ -157,7 +188,9 @@ export function setBlockContent(
   blockId: string,
   newContent: string,
 ): EditorBlockData[] {
-  return blocks.map((b) => (b.id === blockId ? { ...b, content: newContent } : { ...b }))
+  return blocks.map((b) =>
+    b.id === blockId ? { ...b, content: newContent } : { ...b },
+  )
 }
 
 /**

packages/editor/src/lib/safe-image-src.ts

@@ -0,0 +1,18 @@
+const ALLOWED_PROTOCOLS = /^(https?:\/\/|data:image\/)/
+
+/**
+ * Validates an image URL against allowed schemes.
+ *
+ * Accepts `http://`, `https://`, and `data:image/*` URIs. Rejects
+ * everything else (`javascript:`, `data:text/`, `file:`, `blob:`, etc.)
+ * to prevent XSS via unsafe `src` attributes in rendered HTML widgets.
+ *
+ * @param url - The raw URL from user-supplied markdown or drop/paste.
+ * @returns The URL unchanged if allowed, or `null` if rejected.
+ */
+export function safeImageSrc(url: string): string | null {
+  if (ALLOWED_PROTOCOLS.test(url)) {
+    return url
+  }
+  return null
+}