fix(rich-markdown-editor): make drag-reorder of an image actually move it, on real browser payloads (#5617)

* fix(rich-markdown-editor): make drag-reorder of an image actually move it, on real browser payloads Reported on latest staging (deploy verified via CodePipeline): dragging an image duplicates it instead of moving it, and a click with a few px of hand jitter — which the draggable <img> turns into a native drag+drop-on-self — destroys the selection and duplicates too, reading as "I can't select this image anymore". Reproduced in real Chromium with real mouse input (micro-drag becomes dragstart, never click) and with the real drag payload shape. Two compounding root causes, both empirically pinned: - TipTap's node-view dragstart bypasses ProseMirror's drag serialization entirely (verified in @tiptap/core source: onDragStart only sets a drag image and NodeSelects the node — no PM text/html, no view.dragging). What the drop actually carries is the BROWSER's native enrichment: an image File plus text/html whose <img src> is the ABSOLUTE rendered URL. - Both hosted-image recognizers (extractEmbeddedFileRef and isInlineRouteSrc) reject absolute URLs, so the #5573 skip-check never matched on real drags: the drop fell into the upload branch (duplicate; original never moves). Falling through to PM instead would be no better: with view.dragging unset its default drop PARSES the html into a copy — persisting the display-layer src that share/export tracking don't recognize — and never deletes the original. Fix, at the mechanism level: - Normalize clipboard/dataTransfer srcs origin-relative before comparing (toSameOriginPath), keyed off window.location.origin deliberately rather than getBaseUrl(): the browser serializes against the origin the page is ACTUALLY viewed on, which legitimately diverges from the configured NEXT_PUBLIC_APP_URL (localhost dev, previews, apex-vs-www). Cross-origin srcs are never treated as ours. Applied to isInlineRouteSrc, hasHostedImageHtml, and findHostedImageAttrs (the paste-clone path had the same absolute-URL gap for browser-native "Copy Image"). - handleDrop performs the internal move itself when the drop's html references the currently-selected image node (htmlReferencesSrc — TipTap's dragstart guarantees that selection): same delete → map → insert shape as ProseMirror's own move, ending NodeSelected. Drop-on-self is a no-op that keeps the ring — which is what a jittery click now resolves to. Empirical before/after (real-Chromium harness driving the real editor + engine): pre-fix the drag leaves the original in place and uploads a duplicate; post-fix the node moves exactly once, nothing uploads, and the moved image stays selected. Paste-clone verified for both relative (PM copy) and absolute (native Copy Image) payloads. 604 unit tests pass including 11 new ones for the origin-aware helpers. * fix(rich-markdown-editor): no-op invalid drop points, match external-image identity by absolute URL Greptile round 1, both real: - dropPoint can return null (no valid insertion point); the raw coords.pos fallback could make tr.insert throw — PM's own null-fallback is only safe because it uses the forgiving replaceRangeWith. A null drop point is now a handled no-op: the node stays put, still selected. - A doc image with a cross-origin src (README badge, CDN image) failed the same-origin identity check, so drag-reordering IT still fell into the duplicate path. htmlReferencesSrc now compares full ABSOLUTE URLs — identity is the question there, not hosted-by-us membership — while the hosted-recognition helpers stay same-origin-scoped. New regression test fails pre-fix. Also folded the remaining inline comments into the handleDrop TSDoc and gave IMG_SRC_RE / INLINE_ROUTE_QUERY_KEYS proper TSDoc (production diff is now TSDoc-only).
2026-09-24 15:45:35 +08:00 · 2026-07-11 20:21:51 -07:00
parent a2f868c397
commit f477faab4b
3 changed files with 252 additions and 28 deletions
@@ -14,8 +14,10 @@ import {
  extractImgSrcs,
  findHostedImageAttrs,
  hasHostedImageHtml,
+  htmlReferencesSrc,
  isInlineRouteSrc,
  shouldSkipFileUpload,
+  toSameOriginPath,
 } from './image-paste'

 // jsdom lacks `elementFromPoint`; the Placeholder extension's viewport tracking calls it on mount.
@@ -288,3 +290,97 @@ describe('findHostedImageAttrs', () => {
    expect(originalAlt).toBe('photo')
  })
 })
+
+describe('origin-aware src normalization (browser-native drag/copy enrichment uses ABSOLUTE urls)', () => {
+  const ORIGIN = 'https://www.staging.sim.ai'
+
+  it('toSameOriginPath: relative passes through; same-origin absolute → path+query; cross-origin → null', () => {
+    expect(toSameOriginPath('/api/files/view/wf_a', ORIGIN)).toBe('/api/files/view/wf_a')
+    expect(toSameOriginPath(`${ORIGIN}/api/workspaces/ws-1/files/inline?fileId=wf_a`, ORIGIN)).toBe(
+      '/api/workspaces/ws-1/files/inline?fileId=wf_a'
+    )
+    expect(toSameOriginPath('https://evil.example.com/api/files/view/wf_a', ORIGIN)).toBeNull()
+  })
+
+  it('isInlineRouteSrc accepts the same-origin ABSOLUTE inline route (the real dragged-img src on staging)', () => {
+    expect(isInlineRouteSrc(`${ORIGIN}/api/workspaces/ws-1/files/inline?fileId=wf_a`, ORIGIN)).toBe(
+      true
+    )
+    expect(
+      isInlineRouteSrc(
+        'https://evil.example.com/api/workspaces/ws-1/files/inline?fileId=wf_a',
+        ORIGIN
+      )
+    ).toBe(false)
+  })
+
+  it('hasHostedImageHtml matches absolute same-origin srcs and rejects cross-origin ones', () => {
+    const isHosted = (src: string) => extractEmbeddedFileRef(src) !== null
+    expect(hasHostedImageHtml(`<img src="${ORIGIN}/api/files/view/wf_a">`, isHosted, ORIGIN)).toBe(
+      true
+    )
+    expect(
+      hasHostedImageHtml(
+        `<img src="${ORIGIN}/api/workspaces/ws-1/files/inline?fileId=wf_a">`,
+        isHosted,
+        ORIGIN
+      )
+    ).toBe(true)
+    expect(
+      hasHostedImageHtml(
+        '<img src="https://evil.example.com/api/files/view/wf_a">',
+        isHosted,
+        ORIGIN
+      )
+    ).toBe(false)
+  })
+
+  it('findHostedImageAttrs matches when the clipboard html carries the absolute rendered url', () => {
+    const ws = createWorkspaceFileContentSource('ws-1')
+    const editor = new Editor({
+      extensions: createMarkdownEditorExtensions({ placeholder: '' }),
+      content: {
+        type: 'doc',
+        content: [{ type: 'image', attrs: { src: '/api/files/view/wf_abc', alt: 'photo' } }],
+      },
+    })
+    const absolute = `${ORIGIN}/api/workspaces/ws-1/files/inline?fileId=wf_abc`
+    const match = findHostedImageAttrs(editor.state.doc, [absolute], ws.resolveImageSrc, ORIGIN)
+    expect(match?.src).toBe('/api/files/view/wf_abc')
+  })
+})
+
+describe('htmlReferencesSrc (the "this drop is MY dragged image" check)', () => {
+  const ORIGIN = 'https://www.staging.sim.ai'
+  const RESOLVED = '/api/workspaces/ws-1/files/inline?fileId=wf_a'
+
+  it('true when the html img src is the absolute form of the resolved src', () => {
+    expect(htmlReferencesSrc(`<img src="${ORIGIN}${RESOLVED}">`, RESOLVED, ORIGIN)).toBe(true)
+  })
+
+  it('true for the relative form too (ProseMirror-serialized html)', () => {
+    expect(htmlReferencesSrc(`<img src="${RESOLVED}">`, RESOLVED, ORIGIN)).toBe(true)
+  })
+
+  // Identity must work for cross-origin srcs too: a doc's image may legitimately point at a CDN or
+  // badge URL, and dragging THAT node to reorder it must still match — under the previous
+  // same-origin-path comparison it fell into the duplicate-upload branch instead.
+  it('matches an external (cross-origin) image against its own exact absolute url', () => {
+    const EXTERNAL = 'https://cdn.example.com/badge.svg'
+    expect(htmlReferencesSrc(`<img src="${EXTERNAL}">`, EXTERNAL, ORIGIN)).toBe(true)
+    expect(
+      htmlReferencesSrc('<img src="https://cdn.example.com/other.svg">', EXTERNAL, ORIGIN)
+    ).toBe(false)
+  })
+
+  it('false for a different image, empty html, missing resolved src, or cross-origin', () => {
+    expect(htmlReferencesSrc(`<img src="${ORIGIN}/api/other?fileId=wf_b">`, RESOLVED, ORIGIN)).toBe(
+      false
+    )
+    expect(htmlReferencesSrc('', RESOLVED, ORIGIN)).toBe(false)
+    expect(htmlReferencesSrc(`<img src="${ORIGIN}${RESOLVED}">`, undefined, ORIGIN)).toBe(false)
+    expect(
+      htmlReferencesSrc(`<img src="https://evil.example.com${RESOLVED}">`, RESOLVED, ORIGIN)
+    ).toBe(false)
+  })
+})
@@ -13,27 +13,63 @@ export function extractImageFiles(transfer: DataTransfer | null): File[] {
    .filter((file): file is File => file !== null)
 }

-// `src` may be double-quoted, single-quoted, or (validly) unquoted per the HTML spec — the browser's
-// own clipboard serialization always quotes it, but other producers of `text/html` are not obligated
-// to.
+/**
+ * Matches `<img>` `src` attribute values: double-quoted, single-quoted, or (validly) unquoted per
+ * the HTML spec — the browser's own clipboard serialization always quotes it, but other producers
+ * of `text/html` are not obligated to.
+ */
 const IMG_SRC_RE = /<img\b[^>]*\bsrc\s*=\s*(?:"([^"]*)"|'([^']*)'|([^\s"'>]+))/gi
+
+/** Query params under which the inline route addresses a workspace file. */
 const INLINE_ROUTE_QUERY_KEYS = new Set(['key', 'fileId'])

+/**
+ * The page's own origin — clipboard/dataTransfer srcs on any other origin are never ours.
+ * Deliberately `window.location.origin`, NOT `getBaseUrl()`: the browser serializes a dragged/copied
+ * `<img>`'s URL against the origin the page is ACTUALLY being viewed on, which can legitimately
+ * differ from the configured `NEXT_PUBLIC_APP_URL` (localhost dev against a shared env, preview
+ * deploys, apex-vs-www) — comparing against the configured URL would silently fail on any such
+ * origin, which is the exact bug class this normalization exists to fix.
+ */
+function runtimeOrigin(): string {
+  return typeof window === 'undefined' ? '' : window.location.origin
+}
+
+/**
+ * Normalizes a clipboard/dataTransfer img `src` to an origin-relative `pathname?query`, or `null`
+ * when it belongs to a different origin. The browser's NATIVE drag/copy enrichment (dragging or
+ * "Copy Image" on a rendered `<img>`) serializes the ABSOLUTE resolved URL —
+ * `https://host/api/workspaces/…/inline?…` — while everything the app compares against (persisted
+ * refs, `resolveImageSrc` output) is origin-relative, so both must be brought into the same space
+ * before comparing. A cross-origin src must never be treated as ours.
+ */
+export function toSameOriginPath(src: string, origin = runtimeOrigin()): string | null {
+  try {
+    const base = origin || 'http://placeholder'
+    const parsed = new URL(src, base)
+    if (parsed.origin !== base) return null
+    return parsed.pathname + parsed.search
+  } catch {
+    return null
+  }
+}
+
 /**
 * True for the *display-layer* inline route `resolveImageSrc` (see `use-file-content-source.tsx`)
 * rewrites an embed to — workspace-scoped `/api/workspaces/{workspaceId}/files/inline?key=…`/`?fileId=…`
 * or public-share-scoped `/api/files/public/{token}/inline?key=…`/`?fileId=…`. This is the shape
 * actually rendered into `<img src>`, and so what a same-page copy's `text/html` clipboard payload
- * actually contains — NOT the raw stored reference `extractEmbeddedFileRef` (in
- * `@/lib/uploads/utils/embedded-image-ref`) recognizes, which only matches the persisted `src` before
- * that rewrite. Checked separately from (rather than folded into) `extractEmbeddedFileRef` since that
- * helper is shared with server-side authorization/export code operating on persisted content, where
- * this display-only shape should never legitimately appear.
+ * actually contains (absolute — see {@link toSameOriginPath}) — NOT the raw stored reference
+ * `extractEmbeddedFileRef` (in `@/lib/uploads/utils/embedded-image-ref`) recognizes, which only
+ * matches the persisted `src` before that rewrite. Checked separately from (rather than folded into)
+ * `extractEmbeddedFileRef` since that helper is shared with server-side authorization/export code
+ * operating on persisted content, where this display-only shape should never legitimately appear.
 */
-export function isInlineRouteSrc(src: string): boolean {
+export function isInlineRouteSrc(src: string, origin = runtimeOrigin()): boolean {
+  const path = toSameOriginPath(src, origin)
+  if (path === null) return false
  try {
-    const parsed = new URL(src, 'http://placeholder')
-    if (parsed.origin !== 'http://placeholder') return false
+    const parsed = new URL(path, 'http://placeholder')
    if (!parsed.pathname.endsWith('/inline')) return false
    for (const key of parsed.searchParams.keys()) {
      if (INLINE_ROUTE_QUERY_KEYS.has(key)) return true
@@ -62,9 +98,53 @@ export function extractImgSrcs(html: string): string[] {
 * select it) makes the browser put BOTH `text/html` (the real serialized node, with its real hosted
 * `src`) AND a synthesized image `File` onto the clipboard — the same "drag a web image out" behavior
 * that {@link extractImageFiles} alone can't tell apart from a genuinely new external image paste.
+ * Srcs are normalized origin-relative first ({@link toSameOriginPath}): ProseMirror's own clipboard
+ * serialization writes the persisted relative src, but the BROWSER's native enrichment writes the
+ * absolute resolved URL.
 */
-export function hasHostedImageHtml(html: string, isHostedRef: (src: string) => boolean): boolean {
-  return extractImgSrcs(html).some((src) => isHostedRef(src) || isInlineRouteSrc(src))
+export function hasHostedImageHtml(
+  html: string,
+  isHostedRef: (src: string) => boolean,
+  origin = runtimeOrigin()
+): boolean {
+  return extractImgSrcs(html).some((src) => {
+    const path = toSameOriginPath(src, origin)
+    return path !== null && (isHostedRef(path) || isInlineRouteSrc(path, origin))
+  })
+}
+
+/** Resolves `src` to a full absolute URL against `origin`, or `null` when unparseable. */
+function toAbsoluteUrl(src: string, origin: string): string | null {
+  try {
+    return new URL(src, origin || 'http://placeholder').href
+  } catch {
+    return null
+  }
+}
+
+/**
+ * True when `html` contains an `<img>` whose src resolves to the same ABSOLUTE URL as
+ * `resolvedSrc` (a `resolveImageSrc` output for a node already in this document). This is the
+ * "that drop is MY dragged image" check for internal drag-reorder: TipTap's node-view dragstart
+ * bypasses ProseMirror's serialization entirely (no PM `text/html`, no `view.dragging`) but
+ * NodeSelects the dragged image, and the browser's native enrichment carries the absolute rendered
+ * URL of exactly that node.
+ *
+ * Deliberately compares full absolute URLs rather than same-origin paths ({@link toSameOriginPath}):
+ * identity is the question here, not hosted-by-us membership — a doc's image may legitimately have a
+ * cross-origin `src` (a README badge, a CDN image), and dragging THAT node to reorder it must match
+ * too, or it falls into the duplicate-upload path. Relative and absolute spellings of the same URL
+ * still compare equal because both sides resolve against the page origin.
+ */
+export function htmlReferencesSrc(
+  html: string,
+  resolvedSrc: string | undefined,
+  origin = runtimeOrigin()
+): boolean {
+  if (!html || !resolvedSrc) return false
+  const target = toAbsoluteUrl(resolvedSrc, origin)
+  if (target === null) return false
+  return extractImgSrcs(html).some((src) => toAbsoluteUrl(src, origin) === target)
 }

 /**
@@ -108,20 +188,26 @@ interface DescendantsDoc {
 * the clipboard/dataTransfer `html`, whose `src` is `resolveImageSrc`'s rewritten *display* URL, not the
 * real persisted one. Inserting a node built from that display URL would bake it into the document,
 * which public share/export/referenced-by-doc tracking don't recognize (they only match the persisted
- * shape) — this lookup avoids ever constructing such a node in the first place.
+ * shape) — this lookup avoids ever constructing such a node in the first place. Both sides of the
+ * comparison are normalized origin-relative ({@link toSameOriginPath}): the clipboard html may carry
+ * the browser's absolute URLs.
 */
 export function findHostedImageAttrs(
  doc: DescendantsDoc,
  targetSrcs: string[],
-  resolveImageSrc: (src: string | undefined) => string | undefined
+  resolveImageSrc: (src: string | undefined) => string | undefined,
+  origin = runtimeOrigin()
 ): Record<string, unknown> | null {
-  const targets = new Set(targetSrcs)
+  const targets = new Set(
+    targetSrcs.map((src) => toSameOriginPath(src, origin)).filter((p): p is string => p !== null)
+  )
  let found: Record<string, unknown> | null = null
  doc.descendants((node) => {
    if (found) return false
    if (node.type.name === 'image') {
      const resolved = resolveImageSrc(node.attrs.src as string | undefined)
-      if (resolved && targets.has(resolved)) {
+      const resolvedPath = resolved ? toSameOriginPath(resolved, origin) : null
+      if (resolvedPath && targets.has(resolvedPath)) {
        found = { ...node.attrs }
        return false
      }
@@ -3,6 +3,9 @@
 import { memo, useEffect, useRef, useState } from 'react'
 import { cn, toast } from '@sim/emcn'
 import type { JSONContent } from '@tiptap/core'
+import { Fragment, Slice } from '@tiptap/pm/model'
+import { NodeSelection } from '@tiptap/pm/state'
+import { dropPoint } from '@tiptap/pm/transform'
 import type { Editor } from '@tiptap/react'
 import { EditorContent, useEditor } from '@tiptap/react'
 import { useRouter } from 'next/navigation'
@@ -19,6 +22,7 @@ import {
  extractImageFiles,
  extractImgSrcs,
  findHostedImageAttrs,
+  htmlReferencesSrc,
  shouldSkipFileUpload,
 } from './image-paste'
 import {
@@ -216,6 +220,8 @@ export function LoadedRichMarkdownEditor({
  const uploadFile = useUploadWorkspaceFile()
  const editorInstanceRef = useRef<Editor | null>(null)
  const source = useFileContentSource()
+  const resolveImageSrcRef = useRef(source.resolveImageSrc)
+  resolveImageSrcRef.current = source.resolveImageSrc

  /**
   * The `/Image` slash command opens this hidden picker; `pendingImagePosRef` holds the caret position
@@ -367,22 +373,58 @@ export function LoadedRichMarkdownEditor({
       * the browser doesn't navigate away from the editor; internal text drags carry no files and fall
       * through to the default behavior.
       *
-       * Dragging an existing image node to reorder it is also an internal drag, but the browser's
-       * native drag-and-drop synthesizes an image `File` into `event.dataTransfer` for a dragged `<img>`
-       * (the same mechanism that lets a user drag a web image out to their desktop) — indistinguishable
-       * from a real external drop by `dataTransfer` files/items alone. When the accompanying `text/html`
-       * shows it's a same-page drag of an already-hosted image, bail out and let ProseMirror's own
-       * default move logic run — it relocates the actual existing node object (`dragging.node`), never
-       * re-parsing html, so (unlike the paste case above) there's no display-vs-persisted src risk here.
-       * Checked from `html`, not `view.dragging`, so a stale `dragging` flag (ProseMirror clears it up
-       * to ~50ms late via `dragend` when a prior internal drag was dropped outside this view) can never
-       * suppress the plain-file swallow guard below for an unrelated drop that happens to land in that
-       * window — this only reacts to what THIS specific drop's `html` actually contains.
+       * Drag-REORDER of an image node is the deceptive case. TipTap's node-view dragstart bypasses
+       * ProseMirror's own drag serialization entirely — no PM `text/html`, no `view.dragging` — but it
+       * DOES NodeSelect the dragged image; what the drop carries instead is the browser's native
+       * enrichment for a dragged `<img>`: an image `File` plus `text/html` whose src is the ABSOLUTE
+       * rendered URL of that exact node. So when the drop's html points at the currently-selected image
+       * node ({@link htmlReferencesSrc}), this drop IS that node being moved, and the move must be
+       * performed here: uploading would duplicate it (the original never moves), and falling through to
+       * ProseMirror is no better — with `view.dragging` unset its default drop PARSES the html into a
+       * copy (persisting the display-layer src, which share/export tracking don't recognize) and never
+       * deletes the original. The gate accepts at most one file (not exactly one): some drag transports
+       * (e.g. CDP-driven input) carry the html alone, and a genuinely external drop can never reference
+       * the currently-selected node's own resolved src.
+       *
+       * The move itself is the same shape as ProseMirror's own: compute the drop point on the
+       * pre-delete doc, delete the source, map the insert position through that delete. A null
+       * `dropPoint` (no valid insertion point) is a handled no-op — the node stays put, still
+       * selected — never a raw-position fallback, which `tr.insert` could throw on (PM's own null
+       * fallback is only safe because it uses the forgiving `replaceRangeWith`).
+       *
+       * PM-serialized drags (a text selection spanning an image, dragged from a textblock) still reach
+       * the `shouldSkipFileUpload` bail below: PM set `view.dragging` for those itself, so its default
+       * move logic is correct there.
       */
      handleDrop: (view, event) => {
        if (!view.editable) return false
        const images = extractImageFiles(event.dataTransfer)
        const html = event.dataTransfer?.getData('text/html') ?? ''
+        const { selection } = view.state
+        if (
+          images.length <= 1 &&
+          selection instanceof NodeSelection &&
+          selection.node.type.name === 'image' &&
+          htmlReferencesSrc(html, resolveImageSrcRef.current(selection.node.attrs.src))
+        ) {
+          event.preventDefault()
+          const coords = view.posAtCoords({ left: event.clientX, top: event.clientY })
+          if (!coords) return true
+          const node = selection.node
+          const tr = view.state.tr
+          const insertPos = dropPoint(
+            view.state.doc,
+            coords.pos,
+            new Slice(Fragment.from(node), 0, 0)
+          )
+          if (insertPos === null) return true
+          tr.delete(selection.from, selection.to)
+          const mapped = tr.mapping.map(insertPos)
+          tr.insert(mapped, node)
+          tr.setSelection(NodeSelection.create(tr.doc, mapped))
+          view.dispatch(tr.scrollIntoView())
+          return true
+        }
        if (shouldSkipFileUpload(images, html, (src) => extractEmbeddedFileRef(src) !== null)) {
          return false
        }