diff --git a/.changeset/inline-image-clipboard-range.md b/.changeset/inline-image-clipboard-range.md new file mode 100644 index 00000000..404bceee --- /dev/null +++ b/.changeset/inline-image-clipboard-range.md @@ -0,0 +1,5 @@ +--- +"@inkeep/open-knowledge": patch +--- + +Fixed copying prose that contains an inline image with a relative or otherwise non-portable URL. The clipboard HTML previously duplicated the paragraph's leading text inside the source-fallback span and silently dropped the image markdown; the fallback now carries the image's own markdown (`![alt](path)`), so pasting into other apps preserves the full sentence and the image reference. diff --git a/packages/app/src/editor/clipboard/serialize.inline-image-range.test.ts b/packages/app/src/editor/clipboard/serialize.inline-image-range.test.ts new file mode 100644 index 00000000..a08ae1c4 --- /dev/null +++ b/packages/app/src/editor/clipboard/serialize.inline-image-range.test.ts @@ -0,0 +1,295 @@ +/** + * Inline-image source-fallback range contract: position resolution must + * target the image atom's own PM range. + * + * tiptap always renders an outer `.react-renderer.node-image` span around + * a React node view; fixtures that omit it exercise a topology production + * never produces. These tests model the real production topology: + * + * .ProseMirror > p + * > span.react-renderer.node-image + * > span[data-node-view-wrapper data-clipboard-inline-leaf] + * > span[data-rmiz] > span[data-rmiz-content] > img + * + * with a leading text-node sibling in the paragraph (`Some prose with an + * ![alt](./x.jpg) image.`), and pin the contract stated in + * `ImageInlineZoomView.tsx` and `findDescriptorRoot`'s doc comment: + * position resolution for an inline-leaf-wrapped PM atom must target the + * atom node's own range (direct `posAtDOM(, 0)` path — no descriptor + * misroute), and any offset handed to `posAtDOM` counts childNodes (text + * nodes included), never an element-only index. + * + * `buildWalkerEnv` is intentionally private; the env is captured by + * stubbing the walker entry point via `mock.module` and driving + * `serializeFragment` — the same public wiring production uses. + * + * The registered mock DELEGATES to the real implementation except inside + * an explicit capture window, so it stays behavior-transparent to every + * other test file in the process. Bun's `mock.module` registry is + * process-global and never truly un-mocks (oven-sh/bun#12823), and an + * `afterAll` re-register's factory can observe the already-mocked + * namespace — a restore built from a namespace spread leaked the stub to + * later files on some runner file orders. Delegation avoids restore + * semantics entirely; `realWalkLiveDom` is captured before the mock so it + * is immune to in-place namespace mutation. + */ + +import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test'; +import type { JSONContent } from '@tiptap/core'; +import type { Fragment } from '@tiptap/pm/model'; +import { Schema } from '@tiptap/pm/model'; +import type { EditorView } from '@tiptap/pm/view'; +import type { SerializeResult, WalkerEnv } from './clipboard-walker.ts'; + +const actualWalker = await import('./clipboard-walker.ts'); +const realWalkLiveDom = actualWalker.walkLiveDomToInlineStyledFragment; + +let capturedEnv: WalkerEnv | null = null; +let captureActive = false; + +mock.module('./clipboard-walker.ts', () => ({ + ...actualWalker, + walkLiveDomToInlineStyledFragment: (slice: unknown, view: unknown, env: WalkerEnv) => { + if (captureActive) { + capturedEnv = env; + return { childNodes: [] }; + } + // biome-ignore lint/suspicious/noExplicitAny: pass-through to the real implementation + return realWalkLiveDom(slice as any, view as any, env); + }, +})); + +// Imported AFTER the module mock so `serializeFragment`'s walker tier hits +// the delegating mock above. +const { createClipboardHtmlSerializer, findDescriptorRoot } = await import('./serialize.ts'); + +// --------------------------------------------------------------------------- +// Fake live-DOM elements. bun-test has no DOM; these cover exactly the +// surface the code under test touches (`classList.contains`, +// `hasAttribute`, `parentElement`, `childNodes`) with STABLE +// object identity so `parentElement ===` and `childNodes.indexOf(...)` work. +// --------------------------------------------------------------------------- + +interface FakeEl { + classes: Set; + attrs: Set; + parentElement: FakeEl | null; + children: FakeEl[]; + childNodes: unknown[]; + classList: { contains: (c: string) => boolean }; + hasAttribute: (a: string) => boolean; +} + +function el(opts?: { classes?: string[]; attrs?: string[] }): FakeEl { + const classes = new Set(opts?.classes ?? []); + const attrs = new Set(opts?.attrs ?? []); + const node: FakeEl = { + classes, + attrs, + parentElement: null, + children: [], + childNodes: [], + classList: { contains: (c: string) => classes.has(c) }, + hasAttribute: (a: string) => attrs.has(a), + }; + return node; +} + +function chain(...els: FakeEl[]): void { + for (let i = 1; i < els.length; i++) { + els[i].parentElement = els[i - 1]; + } +} + +const fakeTextNode = () => ({ nodeType: 3 }); + +/** + * Real production topology around ImageInlineZoom's inline ``, with + * the leading/trailing text runs as paragraph siblings. Returns the + * elements the tests interrogate. + */ +function buildInlineZoomTopology() { + const proseMirror = el({ classes: ['ProseMirror'] }); + const para = el(); + const reactRenderer = el({ classes: ['react-renderer', 'node-image'] }); + const inlineLeafWrapper = el({ + attrs: ['data-node-view-wrapper', 'data-clipboard-inline-leaf'], + }); + const rmiz = el({ attrs: ['data-rmiz'] }); + const rmizContent = el({ attrs: ['data-rmiz-content'] }); + const img = el(); + chain(proseMirror, para, reactRenderer, inlineLeafWrapper, rmiz, rmizContent, img); + para.children = [reactRenderer]; + para.childNodes = [fakeTextNode(), reactRenderer, fakeTextNode()]; + return { para, reactRenderer, img }; +} + +// --------------------------------------------------------------------------- +// PM document mirroring the inline-image-in-prose seed: `Some prose with an ![alt](./x.jpg) image.` +// --------------------------------------------------------------------------- + +const inlineImageSchema = new Schema({ + nodes: { + doc: { content: 'block+' }, + paragraph: { + group: 'block', + content: 'inline*', + toDOM: () => ['p', 0], + parseDOM: [{ tag: 'p' }], + }, + image: { + group: 'inline', + inline: true, + atom: true, + attrs: { src: { default: '' }, alt: { default: '' } }, + toDOM: (node) => ['img', { src: node.attrs.src, alt: node.attrs.alt }], + parseDOM: [{ tag: 'img' }], + }, + text: { group: 'inline' }, + }, +}); + +const LEADING_TEXT = 'Some prose with an '; +const TRAILING_TEXT = ' image.'; + +function buildDoc() { + return inlineImageSchema.node('doc', null, [ + inlineImageSchema.node('paragraph', null, [ + inlineImageSchema.text(LEADING_TEXT), + inlineImageSchema.node('image', { src: './x.jpg', alt: 'alt' }), + inlineImageSchema.text(TRAILING_TEXT), + ]), + ]); +} + +// Paragraph content starts at 1; the image atom sits after the 19-char +// leading text run. +const IMAGE_POS = 1 + LEADING_TEXT.length; + +/** + * posAtDOM faithful to ProseMirror's documented contract: the offset + * argument counts CHILDNODES of the passed node (text nodes included). + * For the paragraph, childNodes map to PM inline children 1:1. + */ +function fakePosAtDOM(para: FakeEl, img: FakeEl, doc: ReturnType) { + const paraNode = doc.child(0); + return (node: unknown, offset: number, _bias?: number): number => { + if (node === img) return IMAGE_POS; + if (node === para) { + let pos = 1; + for (let i = 0; i < offset; i++) { + pos += paraNode.child(i).nodeSize; + } + return pos; + } + throw new RangeError('fakePosAtDOM: element not in fake mapping'); + }; +} + +/** + * Markdown manager double that discriminates WHICH PM node reached + * serialization: an image node emits its markdown source; text emits the + * raw text. The assertion below is on which node was resolved, so a wrong + * range surfaces as wrong bytes, never a false green. + */ +function discriminatingMdManager() { + const serializeJson = (json: JSONContent): string => { + if (json.type === 'image') { + return `![${json.attrs?.alt ?? ''}](${json.attrs?.src ?? ''})`; + } + if (json.type === 'text') return json.text ?? ''; + return (json.content ?? []).map(serializeJson).join(''); + }; + return { + serialize: (json: JSONContent) => serializeJson(json), + parse: () => ({ type: 'doc', content: [] }), + }; +} + +function captureEnv(para: FakeEl, img: FakeEl): WalkerEnv { + capturedEnv = null; + captureActive = true; + const doc = buildDoc(); + const view = { + posAtDOM: fakePosAtDOM(para, img, doc), + state: { + schema: inlineImageSchema, + doc, + selection: { + from: 0, + to: doc.content.size, + content: () => doc.slice(0, doc.content.size), + }, + }, + } as unknown as EditorView; + const handle = createClipboardHtmlSerializer({ + // biome-ignore lint/suspicious/noExplicitAny: markdown-manager double + mdManager: discriminatingMdManager() as any, + }); + handle.setView(view); + try { + handle.serializer.serializeFragment({ firstChild: null } as unknown as Fragment, undefined, { + appendChild: () => {}, + } as unknown as DocumentFragment); + } finally { + captureActive = false; + } + if (!capturedEnv) throw new Error('walker env was not captured'); + return capturedEnv; +} + +let origWarn: typeof console.warn; +beforeEach(() => { + origWarn = console.warn; + console.warn = () => {}; +}); +afterEach(() => { + console.warn = origWarn; +}); + +describe('QA-005 contract — inline-image source-fallback range resolution', () => { + test('findDescriptorRoot: real ImageInlineZoom topology (outer .react-renderer.node-image present) is NOT a descriptor', () => { + // tiptap always renders a `.react-renderer.node-` span around a + // React node view — the inline-leaf opt-out must neutralize the WHOLE + // wrapper stack of the same node view, not just the annotated + // NodeViewWrapper, so the direct `posAtDOM(, 0)` path is taken. + const { img } = buildInlineZoomTopology(); + expect(findDescriptorRoot(img as unknown as Element)).toBeNull(); + }); + + test('serializeElementMarkdown resolves the inline image atom, not the paragraph leading text run', () => { + const { para, img } = buildInlineZoomTopology(); + const env = captureEnv(para, img); + const result = env.serializeElementMarkdown?.(img as unknown as Element) as SerializeResult; + expect(result.kind).toBe('ok'); + if (result.kind === 'ok') { + expect(result.markdown).toBe('![alt](./x.jpg)'); + } + }); + + test('descriptor path with a leading text sibling passes a childNodes offset to posAtDOM (genuine descriptor mid-paragraph)', () => { + // Same paragraph shape, but the wrapper stack is a GENUINE descriptor + // (no `data-clipboard-inline-leaf`), so the descriptor-parent branch + // is the correct route. `mathInline` is the inline descriptor that + // actually sits mid-paragraph between text nodes (`MathInlineView`, + // NodeViewWrapper as="span"). The paragraph's childNodes are + // [#text, descriptor, #text]; an element-only index (0) addresses the + // leading text run. The offset fed to posAtDOM must count childNodes, + // so the resolved range is the descriptor's own node. + const proseMirror = el({ classes: ['ProseMirror'] }); + const para = el(); + const reactRenderer = el({ classes: ['react-renderer', 'node-mathInline'] }); + const innerWrapper = el({ attrs: ['data-node-view-wrapper'] }); + const leaf = el(); + chain(proseMirror, para, reactRenderer, innerWrapper, leaf); + para.children = [reactRenderer]; + para.childNodes = [fakeTextNode(), reactRenderer, fakeTextNode()]; + + const env = captureEnv(para, leaf); + const result = env.serializeElementMarkdown?.(leaf as unknown as Element) as SerializeResult; + expect(result.kind).toBe('ok'); + if (result.kind === 'ok') { + expect(result.markdown).toBe('![alt](./x.jpg)'); + } + }); +}); diff --git a/packages/app/src/editor/clipboard/serialize.test.ts b/packages/app/src/editor/clipboard/serialize.test.ts index 9fbf5c27..795fd163 100644 --- a/packages/app/src/editor/clipboard/serialize.test.ts +++ b/packages/app/src/editor/clipboard/serialize.test.ts @@ -336,14 +336,23 @@ function chainDescriptorEls(...els: FakeDescriptorElement[]): FakeDescriptorElem return els[els.length - 1]; } +// Memoized so repeated `parentElement` walks return the SAME wrapper +// object — real DOM elements have stable identity, and the climb's +// same-node-view containment check compares by identity. +const descriptorWrappers = new WeakMap(); + function wrapDescriptor(el: FakeDescriptorElement): Element { - return { + const existing = descriptorWrappers.get(el); + if (existing) return existing; + const wrapper = { classList: { contains: (c: string) => el.classes.has(c) }, hasAttribute: (a: string) => el.attrs.has(a), get parentElement() { return el.parentElement === null ? null : wrapDescriptor(el.parentElement); }, } as unknown as Element; + descriptorWrappers.set(el, wrapper); + return wrapper; } describe('findDescriptorRoot — outermost-wrapper selection', () => { @@ -421,45 +430,63 @@ describe('findDescriptorRoot — outermost-wrapper selection', () => { expect(root).toBeNull(); }); - test('(f) wrappers carrying `data-clipboard-inline-leaf` are skipped (ImageInlineZoom opt-out)', () => { + test("(f) wrappers carrying `data-clipboard-inline-leaf` are skipped, including the same node view's outer .react-renderer (ImageInlineZoom opt-out)", () => { // `ImageInlineZoom` wraps inline `` in `` so - // click-to-enlarge works mid-prose. The wrapper carries - // `data-node-view-wrapper` (tiptap stamps it on every NodeViewWrapper) - // — without the opt-out, `findDescriptorRoot` would match the wrapper - // and route clipboard serialization through the descriptor-parent - // codepath (`posAtDOM(

, idx, -1)`), which has different mark- - // interaction semantics than the direct `posAtDOM(, 0)` path the - // bare PM image node used. The opt-out preserves the pre- - // existing clipboard behavior with zero risk surface. A regression - // that drops the skip would silently re-introduce the routing change. + // click-to-enlarge works mid-prose. tiptap renders the node view as + // `.react-renderer.node-image > [data-node-view-wrapper ...]` — the + // opt-out must neutralize the WHOLE wrapper stack of that node view, + // not just the annotated wrapper: the outer `.react-renderer` belongs + // to the same node view and matching it would route clipboard + // serialization through the descriptor-parent codepath + // (`posAtDOM(

, idx, -1)`) instead of the direct + // `posAtDOM(, 0)` path the bare PM image node uses. This models + // the real production topology. const proseMirror = makeDescriptorEl({ classes: ['ProseMirror'] }); const para = makeDescriptorEl(); + const outerReactRenderer = makeDescriptorEl({ classes: ['react-renderer', 'node-image'] }); const inlineLeafWrapper = makeDescriptorEl({ attrs: ['data-node-view-wrapper', 'data-clipboard-inline-leaf'], }); const img = makeDescriptorEl(); - const live = chainDescriptorEls(proseMirror, para, inlineLeafWrapper, img); + const live = chainDescriptorEls(proseMirror, para, outerReactRenderer, inlineLeafWrapper, img); expect(findDescriptorRoot(wrapDescriptor(live))).toBeNull(); }); - test('(g) opt-out is wrapper-local — a real descriptor BEYOND the inline-leaf wrapper still matches (defense against accidental no-op for nested cases)', () => { - // Pin that `data-clipboard-inline-leaf` only skips THAT wrapper, not - // the rest of the climb. If a future schema nests `ImageInlineZoom` - // inside a block descriptor (hypothetical, but the walker shouldn't - // care), the outer block descriptor must still be found. + test("(g) opt-out neutralizes only the same node view's stack — a genuine descriptor ABOVE it still matches", () => { + // Pin that the opt-out skips exactly the inline-leaf node view's own + // wrapper pair (`.react-renderer` + annotated NodeViewWrapper), not + // the rest of the climb. A genuine enclosing descriptor always + // interposes its OWN NodeViewWrapper between its `.react-renderer` + // and nested content, so if a future schema nests `ImageInlineZoom` + // inside a block descriptor, the outer descriptor must still be found. const proseMirror = makeDescriptorEl({ classes: ['ProseMirror'] }); const outerReactRenderer = makeDescriptorEl({ classes: ['react-renderer'] }); + const outerWrapper = makeDescriptorEl({ + attrs: ['data-node-view-wrapper', 'data-jsx-component'], + }); + const innerReactRenderer = makeDescriptorEl({ classes: ['react-renderer', 'node-image'] }); const inlineLeafWrapper = makeDescriptorEl({ attrs: ['data-node-view-wrapper', 'data-clipboard-inline-leaf'], }); const img = makeDescriptorEl(); - const live = chainDescriptorEls(proseMirror, outerReactRenderer, inlineLeafWrapper, img); + const live = chainDescriptorEls( + proseMirror, + outerReactRenderer, + outerWrapper, + innerReactRenderer, + inlineLeafWrapper, + img, + ); const root = findDescriptorRoot(wrapDescriptor(live)); expect(root).not.toBeNull(); - expect(root?.classList.contains('react-renderer')).toBe(true); + // Positive: the OUTERMOST genuine descriptor root is returned. + expect(root).toBe(wrapDescriptor(outerReactRenderer)); + // Negative: the inline node view's own wrapper stack was neutralized. + expect(root).not.toBe(wrapDescriptor(innerReactRenderer)); + expect(root).not.toBe(wrapDescriptor(inlineLeafWrapper)); }); }); diff --git a/packages/app/src/editor/clipboard/serialize.ts b/packages/app/src/editor/clipboard/serialize.ts index 5abdfeb4..39fe6583 100644 --- a/packages/app/src/editor/clipboard/serialize.ts +++ b/packages/app/src/editor/clipboard/serialize.ts @@ -488,6 +488,13 @@ function sliceToMarkdown(slice: Slice, schema: Schema, mdManager: MarkdownManage * mark-interaction semantics than the direct `posAtDOM(, 0)` path * the bare PM image node uses. Skipping these wrappers preserves * the direct-leaf clipboard behavior while still mounting the Zoom UI. + * The opt-out neutralizes the node view's WHOLE wrapper stack: tiptap + * always renders a `.react-renderer` element immediately around the + * NodeViewWrapper, so when the annotated wrapper is skipped, its direct + * `.react-renderer` parent (the same node view's outer wrapper) is + * skipped too. A genuine enclosing descriptor always interposes its own + * NodeViewWrapper between its `.react-renderer` and any nested content, + * so nested descriptors above the stack still match. * * Exported only for unit-test reach; the production caller is * `buildWalkerEnv` below. @@ -495,6 +502,7 @@ function sliceToMarkdown(slice: Slice, schema: Schema, mdManager: MarkdownManage export function findDescriptorRoot(live: Element): Element | null { let descriptorRoot: Element | null = null; let cur: Element | null = live; + let optedOutWrapper: Element | null = null; while (cur && !cur.classList.contains('ProseMirror')) { // Opt-out: live-editor render wrappers around bare inline PM atoms // (e.g., `ImageInlineZoom`'s `` wrap). The PM node IS the leaf @@ -502,6 +510,17 @@ export function findDescriptorRoot(live: Element): Element | null { // `data-node-view-wrapper` on the NodeViewWrapper. Skip these so // `posAtDOM(, 0)` stays the resolution path. if (cur.hasAttribute('data-clipboard-inline-leaf')) { + optedOutWrapper = cur; + cur = cur.parentElement; + continue; + } + // tiptap always renders a `.react-renderer` immediately around the + // same node view's NodeViewWrapper. When that wrapper opted out, its + // DIRECT `.react-renderer` parent belongs to the same node view and + // must be neutralized too — a genuine enclosing descriptor always + // interposes its own NodeViewWrapper, so this containment check + // cannot swallow real nested descriptors. + if (optedOutWrapper?.parentElement === cur && cur.classList.contains('react-renderer')) { cur = cur.parentElement; continue; } @@ -567,7 +586,10 @@ function buildWalkerEnv(view: EditorView, mdManager: MarkdownManager): WalkerEnv try { const parent = descriptorRoot?.parentElement; if (parent && descriptorRoot) { - const idx = Array.from(parent.children).indexOf(descriptorRoot); + // posAtDOM's offset counts CHILDNODES (text nodes included), not + // element children — an element-only index misaddresses any + // descriptor with a non-element preceding sibling. + const idx = Array.prototype.indexOf.call(parent.childNodes, descriptorRoot); pos = view.posAtDOM(parent, idx, -1); } else { pos = view.posAtDOM(live, 0); diff --git a/packages/app/src/editor/extensions/ImageInlineZoomView.tsx b/packages/app/src/editor/extensions/ImageInlineZoomView.tsx index bcb3df6a..9ac4cca2 100644 --- a/packages/app/src/editor/extensions/ImageInlineZoomView.tsx +++ b/packages/app/src/editor/extensions/ImageInlineZoomView.tsx @@ -17,7 +17,9 @@ * `findDescriptorRoot` (see `clipboard/serialize.ts`): the PM node IS * the bare `` atom, not a descriptor, so position resolution must * stay on the direct-leaf `posAtDOM(, 0)` path that the un-wrapped - * image used. + * image used. The opt-out neutralizes this node view's whole wrapper + * stack: the outer `.react-renderer` span tiptap renders directly around + * this wrapper is skipped too. * * Doc-relative `src` is resolved against the document's folder here, the * same render-time normalization the block path applies in diff --git a/packages/app/tests/stress/clipboard-relative-url-source-fallback.e2e.ts b/packages/app/tests/stress/clipboard-relative-url-source-fallback.e2e.ts index d7477d94..6f797631 100644 --- a/packages/app/tests/stress/clipboard-relative-url-source-fallback.e2e.ts +++ b/packages/app/tests/stress/clipboard-relative-url-source-fallback.e2e.ts @@ -143,6 +143,47 @@ test.describe('FR-2 walker URL classifier — WYSIWYG cross-app source-fallback' expect(captured.html).not.toContain('src="./x.jpg"'); }); + test('QA-005b inline image adjacent to marks resolves the image range and keeps the marks', async ({ + page, + baseURL, + }) => { + // Behavioral pin for the direct-path resolution when the image sits + // inside marked text. The mark-interaction semantics of the direct + // `posAtDOM(, 0)` path (vs the descriptor-parent path's -1 bias) + // only matter when marks surround the image, so this seed puts the + // image inside a strong run with prose on both sides. + await fetch(`${baseURL}/api/agent-write-md`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + docName, + markdown: 'Lead **bold ![alt](./y.png) tail** end.\n', + position: 'replace', + }), + }); + await expect(async () => { + expect(await getYText(page)).toContain('![alt](./y.png)'); + }).toPass({ timeout: 5_000 }); + await page.click('.ProseMirror:not(.composer-prosemirror)'); + + const captured = await simulateCopyAndRead(page, 'wysiwyg'); + + // text/plain is canonical markdown; the serializer splits the strong + // run around the image atom, so assert the pieces rather than exact + // bytes (that emission path is untouched here). + expect(captured.plain).toContain('![alt](./y.png)'); + expect(captured.plain).toContain('bold'); + expect(captured.plain).toContain('tail'); + // The fallback span carries the image's own markdown, not a duplicate + // of the surrounding text run. + expect(captured.html).toContain(''); + expect(captured.html).toContain('![alt](./y.png)'); + expect(captured.html).not.toMatch(/[^<]*bold/); + // The strong mark survives around the swap. + expect(captured.html).toMatch(/<(strong|b)[\s>]/); + expect(captured.html).not.toContain('src="./y.png"'); + }); + test('QA-009 all-portable selection: walker passes through unchanged (regression check)', async ({ page, baseURL,