ronantakizawa/codereview-bench huggingface dataset:
Repository: ueberdosis/tiptap
File: packages/core/src/commands/splitBlock.ts (TypeScript)

Code before:

  if (selection instanceof NodeSelection && selection.node.isBlock) {
    if (!$from.parentOffset || !canSplit(doc, $from.pos)) {
      return false
    }

    if (dispatch) {
      if (keepMarks) {
        ensureMarks(state, editor.extensionManager.splittableMarks)
      }

      tr.split($from.pos).scrollIntoView()
    }

    return true
  }

  if (!$from.parent.isBlock) {
    return false
  }

  const atEnd = $to.parentOffset === $to.parent.content.size

  if (selection instanceof TextSelection) {
    tr.deleteSelection()
  }

  const deflt = $from.depth === 0
    ? undefined
    : defaultBlockAt($from.node(-1).contentMatchAt($from.indexAfter(-1)))

  let types = atEnd && deflt
    ? [
      {
        type: deflt,
        attrs: newAttributes,
      },
    ]
    : undefined

  let can = canSplit(tr.doc, tr.mapping.map($from.pos), 1, types)

  if (
    !types
        && !can
        && canSplit(tr.doc, tr.mapping.map($from.pos), 1, deflt ? [{ type: deflt }] : undefined)
  ) {
    can = true
    types = deflt
      ? [
        {

Review comment (bug):
Sorry this is not correct, it should only ever manipulate the transaction if `dispatch` is true. This would break the behavior of commands being allowed to fail silently

Code after:

  if (selection instanceof NodeSelection && selection.node.isBlock) {
    if (!$from.parentOffset || !canSplit(doc, $from.pos)) {
      return false
    }

    if (dispatch) {
      if (keepMarks) {
        ensureMarks(state, editor.extensionManager.splittableMarks)
      }

      tr.split($from.pos).scrollIntoView()
    }

    return true
  }

  if (!$from.parent.isBlock) {
    return false
  }

  const atEnd = $to.parentOffset === $to.parent.content.size

  const deflt = $from.depth === 0
    ? undefined
    : defaultBlockAt($from.node(-1).contentMatchAt($from.indexAfter(-1)))

  let types = atEnd && deflt
    ? [
      {
        type: deflt,
        attrs: newAttributes,
      },
    ]
    : undefined

  let can = canSplit(tr.doc, tr.mapping.map($from.pos), 1, types)

  if (
    !types
      && !can
      && canSplit(tr.doc, tr.mapping.map($from.pos), 1, deflt ? [{ type: deflt }] : undefined)
  ) {
    can = true
    types = deflt
      ? [
        {
          type: deflt,
          attrs: newAttributes,
        },
      ]
Repository: ueberdosis/tiptap
File: packages/extension-table/src/utilities/colStyle.ts (TypeScript)

Code before:
export function getColStyleDeclaration(minWidth: number, width: number | undefined): [string, string] {
  if (width) {
    // apply the stored width unless it is below the configured minimum cell width
    return ['width', `${Math.max(width, minWidth)}px`]
  }

  // set the minimum with on the column if it has no stored width
  return ['min-width', `${minWidth}px`]

}

export function getColStyle(minWidth: number, width: number): string {
  return getColStyleDeclaration(minWidth, width).join(': ')
}


Review comment (performance):
This is abstracting over a couple of if statements, I'm not sure it warrants the complexity it adds.

I would honestly prefer for there to be a single function here, either stick to the string version or the array version, both just adds complexity and indirection.

Code after:
export function getColStyleDeclaration(minWidth: number, width: number | undefined): [string, string] {
  if (width) {
    // apply the stored width unless it is below the configured minimum cell width
    return ['width', `${Math.max(width, minWidth)}px`]
  }

  // set the minimum with on the column if it has no stored width
  return ['min-width', `${minWidth}px`]

}
Repository: ueberdosis/tiptap
File: packages/vue-3/src/Editor.ts (TypeScript)

Code before:

  get state() {
    return this.reactiveState ? this.reactiveState.value : this.view.state
  }

  get storage() {
    return this.reactiveExtensionStorage ? this.reactiveExtensionStorage.value : super.storage
  }

  /**
   * Register a ProseMirror plugin.
   */
  public registerPlugin(
    plugin: Plugin,
    handlePlugins?: (newPlugin: Plugin, plugins: Plugin[]) => Plugin[],
  ): void {
    const plugins = isFunction(handlePlugins)
      ? handlePlugins(plugin, [...this.state.plugins])
      : [...this.state.plugins, plugin]

    const state = this.state.reconfigure({ plugins })

    this.view.updateState(state)

    if (this.reactiveState) {
      this.reactiveState.value = state
    }
  }

  /**
   * Unregister a ProseMirror plugin.
   */
  public unregisterPlugin(nameOrPluginKey: string | PluginKey): void {
    if (this.isDestroyed) {
      return
    }

    // @ts-ignore
    const name = typeof nameOrPluginKey === 'string' ? `${nameOrPluginKey}$` : nameOrPluginKey.key

    const state = this.state.reconfigure({
      // @ts-ignore
      plugins: this.state.plugins.filter(plugin => !plugin.key.startsWith(name)),
    })

    this.view.updateState(state)

    if (this.reactiveState) {
      this.reactiveState.value = state
    }
  }

Review comment (refactor):
This is the only real difference from what it was doing before no? Why do we need to duplicate what the editor class is doing?

The same was done for the unregistering, so I'd like to understand better what the actual problem was

Code after:

  get state() {
    return this.reactiveState ? this.reactiveState.value : this.view.state
  }

  get storage() {
    return this.reactiveExtensionStorage ? this.reactiveExtensionStorage.value : super.storage
  }

  /**
   * Register a ProseMirror plugin.
   */
  public registerPlugin(
    plugin: Plugin,
    handlePlugins?: (newPlugin: Plugin, plugins: Plugin[]) => Plugin[],
  ): EditorState {
    const nextState = super.registerPlugin(plugin, handlePlugins)

    if (this.reactiveState) {
      this.reactiveState.value = nextState
    }

    return nextState
  }

  /**
   * Unregister a ProseMirror plugin.
   */
  public unregisterPlugin(nameOrPluginKey: string | PluginKey): EditorState | undefined {
    const nextState = super.unregisterPlugin(nameOrPluginKey)

    if (this.reactiveState && nextState) {
      this.reactiveState.value = nextState
    }

    return nextState
  }
}
Repository: ueberdosis/tiptap
File: demos/src/Marks/Link/React/index.spec.js (JavaScript)

Code before:
      .find('a[href="https://example2.com/foobar"]')
      .should('contain', 'https://example2.com/foobar')

    cy.get('.tiptap')
      .find('a[href="http://example3.com/foobar"]')
      .should('contain', 'http://example3.com/foobar')
  })

  it('should not allow links with disallowed protocols', () => {
    const disallowedProtocols = ['ftp://example.com', 'file:///example.txt', 'mailto:test@example.com']

    disallowedProtocols.forEach(url => {
      cy.get('.tiptap').then(([{ editor }]) => {
        editor.commands.setContent(`<p><a href="${url}">Example Text</a></p>`)
        expect(editor.getHTML()).to.not.include(`<a href="${url}">`)
      })
    })
  })

  it('should not allow links with disallowed domains', () => {
    const disallowedDomains = ['https://example-phishing.com', 'https://malicious-site.net']

    disallowedDomains.forEach(url => {
      cy.get('.tiptap').then(([{ editor }]) => {
        editor.commands.setContent(`<p><a href="${url}">Example Text</a></p>`)
        expect(editor.getHTML()).to.not.include(`<a href="${url}">`)
      })
    })
  })

  it('should not auto-link a URL from a disallowed domain', () => {
    cy.get('.tiptap').type('https://example-phishing.com ') // disallowed domain
    cy.get('.tiptap').should('not.have.descendants', 'a')
    cy.get('.tiptap').should('contain.text', 'https://example-phishing.com')
  })
})


Review comment (suggestion):
same here
```suggestion
        expect(editor.getHTML()).to.not.include(url)
```

Code after:
      .find('a[href="https://example2.com/foobar"]')
      .should('contain', 'https://example2.com/foobar')

    cy.get('.tiptap')
      .find('a[href="http://example3.com/foobar"]')
      .should('contain', 'http://example3.com/foobar')
  })

  it('should not allow links with disallowed protocols', () => {
    const disallowedProtocols = ['ftp://example.com', 'file:///example.txt', 'mailto:test@example.com']

    disallowedProtocols.forEach(url => {
      cy.get('.tiptap').then(([{ editor }]) => {
        editor.commands.setContent(`<p><a href="${url}">Example Text</a></p>`)
        expect(editor.getHTML()).to.not.include(url)
      })
    })
  })

  it('should not allow links with disallowed domains', () => {
    const disallowedDomains = ['https://example-phishing.com', 'https://malicious-site.net']

    disallowedDomains.forEach(url => {
      cy.get('.tiptap').then(([{ editor }]) => {
        editor.commands.setContent(`<p><a href="${url}">Example Text</a></p>`)
        expect(editor.getHTML()).to.not.include(url)
      })
    })
  })

  it('should not auto-link a URL from a disallowed domain', () => {
    cy.get('.tiptap').type('https://example-phishing.com ') // disallowed domain
    cy.get('.tiptap').should('not.have.descendants', 'a')
    cy.get('.tiptap').should('contain.text', 'https://example-phishing.com')
  })
})
Repository: ueberdosis/tiptap
File: packages/extension-youtube/src/utils.ts (TypeScript)

Code before:

export const getYoutubeEmbedUrl = (nocookie?: boolean) => {
  return nocookie ? 'https://www.youtube-nocookie.com/embed/' : 'https://www.youtube.com/embed/'
}

export const getEmbedUrlFromYoutubeUrl = (options: GetEmbedUrlOptions) => {
  const {
    url,
    allowFullscreen,
    autoplay,
    ccLanguage,
    ccLoadPolicy,
    controls,
    disableKBcontrols,
    enableIFrameApi,
    endTime,
    interfaceLanguage,
    ivLoadPolicy,
    loop,
    modestBranding,
    nocookie,
    origin,
    playlist,
    progressBarColor,
    startAt,
    sameChannelRel,
  } = options

  if (!isValidYoutubeUrl(url)) {
    return null
  }

  // if is already an embed url, return it
  if (url.includes('/embed/')) {
    return url
  }

  // if is a youtu.be url, get the id after the /
  if (url.includes('youtu.be')) {
    const id = url.split('/').pop()

    if (!id) {
      return null
    }
    return `${getYoutubeEmbedUrl(nocookie)}${id}`
  }

  const videoIdRegex = /(?:v=|shorts\/)([-\w]+)/gm
  const matches = videoIdRegex.exec(url)

  if (!matches || !matches[1]) {

Review comment (suggestion):
I'd call this `rel: boolean` or `relatedVideos: boolean` and allow to pass `rel={Number(rel)}` so the user can customize it and would be following more the youtube documentation for the embedded player parameters.

Code after:

export const getYoutubeEmbedUrl = (nocookie?: boolean) => {
  return nocookie ? 'https://www.youtube-nocookie.com/embed/' : 'https://www.youtube.com/embed/'
}

export const getEmbedUrlFromYoutubeUrl = (options: GetEmbedUrlOptions) => {
  const {
    url,
    allowFullscreen,
    autoplay,
    ccLanguage,
    ccLoadPolicy,
    controls,
    disableKBcontrols,
    enableIFrameApi,
    endTime,
    interfaceLanguage,
    ivLoadPolicy,
    loop,
    modestBranding,
    nocookie,
    origin,
    playlist,
    progressBarColor,
    startAt,
    rel,
  } = options

  if (!isValidYoutubeUrl(url)) {
    return null
  }

  // if is already an embed url, return it
  if (url.includes('/embed/')) {
    return url
  }

  // if is a youtu.be url, get the id after the /
  if (url.includes('youtu.be')) {
    const id = url.split('/').pop()

    if (!id) {
      return null
    }
    return `${getYoutubeEmbedUrl(nocookie)}${id}`
  }

  const videoIdRegex = /(?:v=|shorts\/)([-\w]+)/gm
  const matches = videoIdRegex.exec(url)

  if (!matches || !matches[1]) {
Repository: ueberdosis/tiptap
File: packages/react/src/EditorContent.tsx (TypeScript)

Code before:
            return prevState
          })

          // Unsubscribe to previous content component
          if (this.unsubscribeToContentComponent) {
            this.unsubscribeToContentComponent()
          }
        })
      }

      editor.createNodeViews()

      this.initialized = true
    }
  }

  componentWillUnmount() {
    const editor = this.props.editor as EditorWithContentComponent | null

    if (!editor) {
      return
    }

    this.initialized = false

    if (!editor.isDestroyed) {
      editor.view.setProps({
        nodeViews: {},
      })
    }

    if (this.unsubscribeToContentComponent) {
      this.unsubscribeToContentComponent()
    }

    editor.contentComponent = null

    if (!editor.options.element?.firstChild) {
      return
    }

    // TODO using the new editor.mount method might allow us to remove this
    const newElement = document.createElement('div')

    newElement.append(...editor.options.element.childNodes)

    editor.setOptions({
      element: newElement,
    })
  }


Review comment (suggestion):
I wonder if, instead of handling the error with a try-catch block, an extra check could be performed on the editor view to determine if the `setProps` method can be called.

For example, we could check this property: https://prosemirror.net/docs/ref/#view.EditorView.isDestroyed

But the solution of the try-catch block works too.

Code after:
            return prevState
          })

          // Unsubscribe to previous content component
          if (this.unsubscribeToContentComponent) {
            this.unsubscribeToContentComponent()
          }
        })
      }

      editor.createNodeViews()

      this.initialized = true
    }
  }

  componentWillUnmount() {
    const editor = this.props.editor as EditorWithContentComponent | null

    if (!editor) {
      return
    }

    this.initialized = false

    try {
      if (!editor.isDestroyed) {
        editor.view.setProps({
          nodeViews: {},
        })
      }
    } catch {
      // ignore error as we can't set props on a non-existing view
    }

    if (this.unsubscribeToContentComponent) {
      this.unsubscribeToContentComponent()
    }

    editor.contentComponent = null

    if (!editor.options.element?.firstChild) {
      return
    }

    // TODO using the new editor.mount method might allow us to remove this
    const newElement = document.createElement('div')

    newElement.append(...editor.options.element.childNodes)

    editor.setOptions({
Repository: ueberdosis/tiptap
File: packages/extension-drag-handle-react/src/DragHandle.tsx (TypeScript)

Code before:
import {
  DragHandlePlugin,
  dragHandlePluginDefaultKey,
  DragHandlePluginProps,
} from '@tiptap/extension-drag-handle'
import { Node } from '@tiptap/pm/model'
import { Plugin } from '@tiptap/pm/state'
import { Editor } from '@tiptap/react'
import React, {
  ReactNode, useEffect, useRef, useState,
} from 'react'

type Optional<T, K extends keyof T> = Pick<Partial<T>, K> & Omit<T, K>;

export type DragHandleProps = Omit<Optional<DragHandlePluginProps, 'pluginKey'>, 'element'> & {
  className?: string;
  onNodeChange?: (data: { node: Node | null; editor: Editor; pos: number }) => void;
  children: ReactNode;

  /**
   * Tippy.js options for the drag handle tooltip.
   *
   * **IMPORTANT**: Make sure to memorize this object - otherwise the object
   * will cause the drag handle to be re-initialized on every render breaking it.
   */
  tippyOptions?: DragHandlePluginProps['tippyOptions'];
};

export const DragHandle = (props: DragHandleProps) => {
  const {
    className = 'drag-handle',
    children,
    editor,
    pluginKey = dragHandlePluginDefaultKey,
    onNodeChange,
    tippyOptions,
  } = props
  const [element, setElement] = useState<HTMLDivElement | null>(null)
  const plugin = useRef<Plugin | null>(null)

  useEffect(() => {
    if (!element) {
      return () => {
        plugin.current = null
      }
    }

    if (editor.isDestroyed) {

Review comment (suggestion):

```suggestion
   * **IMPORTANT**: Make sure to memoize this object - otherwise the object
```

Code after:
import {
  DragHandlePlugin,
  dragHandlePluginDefaultKey,
  DragHandlePluginProps,
} from '@tiptap/extension-drag-handle'
import { Node } from '@tiptap/pm/model'
import { Plugin } from '@tiptap/pm/state'
import { Editor } from '@tiptap/react'
import React, {
  ReactNode, useEffect, useRef, useState,
} from 'react'

type Optional<T, K extends keyof T> = Pick<Partial<T>, K> & Omit<T, K>;

export type DragHandleProps = Omit<Optional<DragHandlePluginProps, 'pluginKey'>, 'element'> & {
  className?: string;
  onNodeChange?: (data: { node: Node | null; editor: Editor; pos: number }) => void;
  children: ReactNode;

  /**
   * Tippy.js options for the drag handle tooltip.
   *
   * **IMPORTANT**: Make sure to memoize this object - otherwise the object
   * will cause the drag handle to be re-initialized on every render breaking it.
   */
  tippyOptions?: DragHandlePluginProps['tippyOptions'];
};

export const DragHandle = (props: DragHandleProps) => {
  const {
    className = 'drag-handle',
    children,
    editor,
    pluginKey = dragHandlePluginDefaultKey,
    onNodeChange,
    tippyOptions,
  } = props
  const [element, setElement] = useState<HTMLDivElement | null>(null)
  const plugin = useRef<Plugin | null>(null)

  useEffect(() => {
    if (!element) {
      return () => {
        plugin.current = null
      }
    }

    if (editor.isDestroyed) {
Repository: ueberdosis/tiptap
File: packages/core/src/MarkView.ts (TypeScript)

Code before:
import { isAndroid, isiOS } from './utilities/index.js'

export function updateMarkViewAttributes(checkMark: Mark, editor: Editor, attrs: Record<string, any> = {}): void {
  const { state } = editor
  const { doc, tr } = state
  const thisMark = checkMark

  doc.descendants((node, pos) => {
    const from = tr.mapping.map(pos)
    const to = tr.mapping.map(pos) + node.nodeSize
    let foundMark: Mark | null = null

    // find the mark on the current node
    node.marks.forEach(mark => {
      if (mark !== thisMark) {
        return false
      }

      foundMark = mark
    })

    if (!foundMark) {
      return
    }

    // now lets check if we need to update given the attributes
    let needsUpdate = false
    Object.keys(attrs).forEach(k => {
      if (attrs[k] !== foundMark!.attrs[k]) {
        needsUpdate = true
      }
    })

    if (needsUpdate) {
      const updatedMark = checkMark.type.create({
        ...checkMark.attrs,
        ...attrs,
      })

      tr.removeMark(from, to, checkMark.type)
      tr.addMark(from, to, updatedMark)
    }
  })

  if (tr.docChanged) {
    editor.view.dispatch(tr)
  }
}

export class MarkView<Component, Options extends MarkViewRendererOptions = MarkViewRendererOptions> {
  component: Component

Review comment (suggestion):
```suggestion
    // check if we need to update given the attributes
```

Code after:
import { isAndroid, isiOS } from './utilities/index.js'

export function updateMarkViewAttributes(checkMark: Mark, editor: Editor, attrs: Record<string, any> = {}): void {
  const { state } = editor
  const { doc, tr } = state
  const thisMark = checkMark

  doc.descendants((node, pos) => {
    const from = tr.mapping.map(pos)
    const to = tr.mapping.map(pos) + node.nodeSize
    let foundMark: Mark | null = null

    // find the mark on the current node
    node.marks.forEach(mark => {
      if (mark !== thisMark) {
        return false
      }

      foundMark = mark
    })

    if (!foundMark) {
      return
    }

    // check if we need to update given the attributes
    let needsUpdate = false
    Object.keys(attrs).forEach(k => {
      if (attrs[k] !== foundMark!.attrs[k]) {
        needsUpdate = true
      }
    })

    if (needsUpdate) {
      const updatedMark = checkMark.type.create({
        ...checkMark.attrs,
        ...attrs,
      })

      tr.removeMark(from, to, checkMark.type)
      tr.addMark(from, to, updatedMark)
    }
  })

  if (tr.docChanged) {
    editor.view.dispatch(tr)
  }
}

export class MarkView<Component, Options extends MarkViewRendererOptions = MarkViewRendererOptions> {
  component: Component
Repository: ueberdosis/tiptap
File: packages/extension-drag-handle/src/drag-handle-plugin.ts (TypeScript)

Code before:
          // Do not hide open popup on mouseleave.
          if (locked) {
            return false
          }

          // If e.target is not inside the wrapper, hide.
          if (e.target && !wrapper.contains(e.relatedTarget as HTMLElement)) {
            popup?.hide()

            currentNode = null
            currentNodePos = -1

            onNodeChange?.({ editor, node: null, pos: -1 })
          }

          return false
        },

        mousemove(view, e) {
          // Do not continue if popup is not initialized or open.
          if (!element || !popup || locked) {
            return false
          }

          const nodeData = findElementNextToCoords({
            x: e.clientX,
            y: e.clientY,
            direction: 'right',
            editor,
          })

          // Skip if there is no node next to coords
          if (!nodeData.resultElement) {
            return false
          }

          let domNode = nodeData.resultElement as HTMLElement

          domNode = getOuterDomNode(view, domNode)

          // Skip if domNode is editor dom.
          if (domNode === view.dom) {
            return false
          }

          // We only want `Element`.
          if (domNode?.nodeType !== 1) {
            return false
          }

          const domNodePos = view.posAtDOM(domNode, 0)

Review comment (suggestion):
This is introducing a hefty problem with blocking other keyhandlers. Fix is already open - will be included in the next release

Code after:

          onNodeChange?.({ editor, node: currentNode, pos: currentNodePos })

          // Update Tippys getReferenceClientRect since domNode might have changed.
          popup.setProps({
            getReferenceClientRect: () => (domNode as Element).getBoundingClientRect(),
          })
        },

        // TODO: Kills even on hot reload
        destroy() {
          popup?.destroy()

          if (element) {
            removeNode(wrapper)
          }
        },
      }
    },

    props: {
      handleDOMEvents: {
        keydown(view) {
          if (popup && popup.state.isVisible && view.hasFocus()) {
            popup.hide()
            return true
          }

          return false
        },

        mouseleave(_view, e) {
          // Do not hide open popup on mouseleave.
          if (locked) {
            return false
          }

          // If e.target is not inside the wrapper, hide.
          if (e.target && !wrapper.contains(e.relatedTarget as HTMLElement)) {
            popup?.hide()

            currentNode = null
            currentNodePos = -1

            onNodeChange?.({ editor, node: null, pos: -1 })
          }

          return false
        },

        mousemove(view, e) {