ronantakizawa/codereview-bench huggingface dataset:
Repository: trueadm/ripple
File: packages/ripple/src/runtime/internal/client/portal.js (JavaScript)

Code before:
  render(() => {
    if (target === (target = props.target)) return;
    if (children === (children = props.children)) return;

    if (b !== null) {
      destroy_block(b);
    }

    domStart = domEnd = null;

    anchor = create_text();
    /** @type {Element} */ (target).append(anchor);

    const cleanup_events = handle_root_events(/** @type {Element} */ (target));

    b = branch(() => /** @type {(anchor: Node) => void} */ (children)(/** @type {Text} */ (anchor)));

    if (b && b.s) {
      domStart = b.s.start;
      domEnd = b.s.end;
    }

    return () => {
      cleanup_events();
      /** @type {Text} */ (anchor).remove();
      if(domStart !== null && domEnd !== null) {
        remove_block_dom(domStart, domEnd);
      }
    };
  });
}


Review comment (suggestion):
seems like these can also be undefined.


```suggestion
      if(domStart != null && domEnd != null) {
```

Code after:

	render(() => {
		if (target === (target = props.target)) return;
		if (children === (children = props.children)) return;

		if (b !== null) {
			destroy_block(b);
		}

		if (anchor !== null) {
			anchor.remove();
		}

		dom_start = dom_end = null;

		anchor = create_text();
		/** @type {Element} */ (target).append(anchor);

		const cleanup_events = handle_root_events(/** @type {Element} */ (target));

		var block = /** @type {Block} */ (active_block);

		b = branch(() => {
			if (typeof children === 'function') {
				children(/** @type {Text} */ (anchor), {}, block);
			}
		});

		dom_start = b?.s?.start;
		dom_end = b?.s?.end;

		return () => {
			cleanup_events();
			/** @type {Text} */ (anchor).remove();
			if (dom_start && dom_end) {
				remove_block_dom(dom_start, dom_end);
			}
		};
	});
}
Repository: trueadm/ripple
File: packages/ripple/src/runtime/internal/client/portal.js (JavaScript)

Code before:
    if (b !== null) {
      destroy_block(b);
    }

    if (anchor !== null) {
      anchor.remove();
    }

    domStart = domEnd = null;

    anchor = create_text();
    /** @type {Element} */ (target).append(anchor);

    const cleanup_events = handle_root_events(/** @type {Element} */ (target));

    // @ts-ignore
    b = branch(() =>  children(anchor));

    domStart = b?.s?.start;
    domEnd = b?.s?.end;

    return () => {
      cleanup_events();
      /** @type {Text} */ (anchor).remove();
      if(domStart && domEnd) {
        remove_block_dom(domStart, domEnd);
      }
    };
  });
}


Review comment (suggestion):
```suggestion
        remove_block_dom(dom_start, dom_end);

Code after:

		if (b !== null) {
			destroy_block(b);
		}

		if (anchor !== null) {
			anchor.remove();
		}

		dom_start = dom_end = null;

		anchor = create_text();
		/** @type {Element} */ (target).append(anchor);

		const cleanup_events = handle_root_events(/** @type {Element} */ (target));

		var block = /** @type {Block} */ (active_block);

		b = branch(() => {
			if (typeof children === 'function') {
				children(/** @type {Text} */ (anchor), {}, block);
			}
		});

		dom_start = b?.s?.start;
		dom_end = b?.s?.end;

		return () => {
			cleanup_events();
			/** @type {Text} */ (anchor).remove();
			if (dom_start && dom_end) {
				remove_block_dom(dom_start, dom_end);
			}
		};
	});
}
Repository: trueadm/ripple
File: packages/ripple/src/runtime/internal/client/portal.js (JavaScript)

Code before:
  /** @type {Node | null} */
  var dom_start = null;
  /** @type {Node | null} */
  var dom_end = null;

  render(() => {
    if (target === (target = props.target)) return;
    if (children === (children = props.children)) return;

    if (b !== null) {
      destroy_block(b);
    }

    if (anchor !== null) {
      anchor.remove();
    }

    dom_start = dom_end = null;

    anchor = create_text();
    /** @type {Element} */ (target).append(anchor);

    const cleanup_events = handle_root_events(/** @type {Element} */ (target));

    // @ts-ignore
    b = branch(() =>  children(anchor));

    dom_start = b?.s?.start;
    dom_end = b?.s?.end;

    return () => {
      cleanup_events();
      /** @type {Text} */ (anchor).remove();
      if(dom_start && dom_end) {
        remove_block_dom(dom_start, dom_end);
      }
    };
  });
}


Review comment (bug):
I know the issue now, this is missing `props` and `block`, here `props` is `{}` and `block` is `active_block` (you'll need to cast it to `Block`) as we know its active here

It was a bug in the original implementation too

Code after:
	var anchor = null;
	/** @type {Node | null} */
	var dom_start = null;
	/** @type {Node | null} */
	var dom_end = null;

	render(() => {
		if (target === (target = props.target)) return;
		if (children === (children = props.children)) return;

		if (b !== null) {
			destroy_block(b);
		}

		if (anchor !== null) {
			anchor.remove();
		}

		dom_start = dom_end = null;

		anchor = create_text();
		/** @type {Element} */ (target).append(anchor);

		const cleanup_events = handle_root_events(/** @type {Element} */ (target));

		var block = /** @type {Block} */ (active_block);

		b = branch(() => {
			if (typeof children === 'function') {
				children(/** @type {Text} */ (anchor), {}, block);
			}
		});

		dom_start = b?.s?.start;
		dom_end = b?.s?.end;

		return () => {
			cleanup_events();
			/** @type {Text} */ (anchor).remove();
			if (dom_start && dom_end) {
				remove_block_dom(dom_start, dom_end);
			}
		};
	});
}
Repository: trueadm/ripple
File: packages/ripple/src/runtime/index-server.js (JavaScript)

Code before:
/** @import { Component, Derived, Tracked } from '#server' */

import { DERIVED, TRACKED, UNINITIALIZED } from './internal/client/constants.js';
import { is_tracked_object } from './internal/client/utils.js';
import { active_component } from './internal/server/index.js';

export { Context } from './internal/server/context.js';

export function effect() {
	// NO-OP
}

var empty_get_set = { get: undefined, set: undefined };

/**
 *
 * @param {any} v
 * @param {Function} [get]
 * @param {Function} [set]
 * @returns {Tracked | Derived}
 */
export function track(v, get, set) {
	var is_tracked = is_tracked_object(v);

	if (is_tracked) {
		return v;
	}

	if (typeof v === 'function') {
		return {
			a: get || set ? { get, set } : empty_get_set,
			co: active_component,
			f: TRACKED | DERIVED,
			fn: v,
			v: UNINITIALIZED,
		};
	}

	return {
		a: get || set ? { get, set } : empty_get_set,
		f: TRACKED,
		v,
	};
}

Review comment (suggestion):
not sure if we want a public version of `get` where we'd call `safe_scope` to make sure the api is being called within the ripple context.  Otherwise, I don't see anywhere where there'd an exception thrown, so the calls will still succeed.

Code after:
/** @import { Component, Derived, Tracked } from '#server' */

import { DERIVED, TRACKED, UNINITIALIZED } from './internal/client/constants.js';
import { is_tracked_object } from './internal/client/utils.js';
import { active_component } from './internal/server/index.js';

export { Context } from './internal/server/context.js';

export function effect() {
	// NO-OP
}

var empty_get_set = { get: undefined, set: undefined };

/**
 * @param {Derived | Tracked} tracked
 * @returns {any}
 */
export function get(tracked) {
	if (!is_tracked_object(tracked)) {
		return tracked;
	}

	var g = tracked.a.get;

	return g ? g(tracked.v) : tracked.v;
}

/**
 * @param {Tracked} tracked
 * @param {any} value
 */
export function set(tracked, value) {
	var s = tracked.a.set;

	tracked.v = s ? s(value, tracked.v) : value;
}

/**
 *
 * @param {any} v
 * @param {Function} [get]
 * @param {Function} [set]
 * @returns {Tracked | Derived}
Repository: trueadm/ripple
File: packages/ripple/src/compiler/phases/3-transform/client/index.js (JavaScript)

Code before:
		);
	}
}

function visit_title_element(node, context) {
	const normalized = normalize_children(node.children, context);
	const content = normalized[0];

	const metadata = { tracking: false, await: false };
	const result = context.visit(content, { ...context.state, metadata }).expression;

	if (metadata.tracking) {
		context.state.init.push(
			b.stmt(
				b.call(
					'_$_.render',
					b.thunk(b.block([b.assignment('=', b.id('_$_.document.title'), result)])),
				),
			),
		);
	} else {
		context.state.init.push(b.stmt(b.assignment('=', b.id('_$_.document.title'), result)));
	}
}

function mark_scoped_css_recursively(node) {
	if (!node || typeof node !== 'object') return;
	if (node.type === 'Element') {
		node.metadata.scoped = true;
	}
	if (Array.isArray(node.children)) {
		for (const child of node.children) mark_scoped_css_recursively(child);
	}
	if (Array.isArray(node.body)) {
		for (const child of node.body) mark_scoped_css_recursively(child);
	}
};

const visitors = {
	_: function set_scope(node, { next, state }) {
		const scope = state.scopes.get(node);

		if (scope && scope !== state.scope) {
			return next({ ...state, scope });
		} else {
			return next();
		}
	},

	Identifier(node, context) {
		const parent = /** @type {Node} */ (context.path.at(-1));

Review comment (suggestion):
We never want to visit children, because the children might get replaced during the transformation process. Instead we just want to apply this during the Element visitor. We can look up the through the parents, and see if the component has `state.component.css` using `context.path`.

Code after:
		);
	}
}

function visit_title_element(node, context) {
	const normalized = normalize_children(node.children, context);
	const content = normalized[0];

	const metadata = { tracking: false, await: false };
	const result = context.visit(content, { ...context.state, metadata }).expression;

	if (metadata.tracking) {
		context.state.init.push(
			b.stmt(
				b.call(
					'_$_.render',
					b.thunk(b.block([b.assignment('=', b.id('_$_.document.title'), result)])),
				),
			),
		);
	} else {
		context.state.init.push(b.stmt(b.assignment('=', b.id('_$_.document.title'), result)));
	}
}

/**
 * @param {string} name
 * @param {any} context
 * @returns {string}
 */
function import_from_ripple_if_needed(name, context) {
	const alias = context.state.ripple_user_imports?.get?.(name);

	if (!alias && !context.state.imports.has(`import { ${name} } from 'ripple'`)) {
		context.state.imports.add(`import { ${name} } from 'ripple'`);
	}

	return alias ?? name;
}

const visitors = {
	_: function set_scope(node, { next, state }) {
		const scope = state.scopes.get(node);

		if (scope && scope !== state.scope) {
			return next({ ...state, scope });
		} else {
			return next();
		}
	},
Repository: trueadm/ripple
File: packages/prettier-plugin/src/index.test.js (JavaScript)

Code before:
  <div>{x}</div>
}`;
				const expected = `component Test() {
  let x = 0;
  if (x === 0) x = 1;
  else x = 2;
  <div>{x}</div>
}`;

				const result = await format(input, { singleQuote: true });
				expect(result).toBeWithNewline(expected);
			});

			it('should format nested if statements with non-block bodies', async () => {
				const input = `component Test() {
  let x = 0;
  if (x === 0) if (x === 1) x = 2; else x = 3;
  <div>{x}</div>
}`;
				const expected = `component Test() {
  let x = 0;
  if (x === 0) if (x === 1) x = 2;
  else x = 3;
  <div>{x}</div>
}`;

				const result = await format(input, { singleQuote: true });
				expect(result).toBeWithNewline(expected);
			});
		});
	});
});


Review comment (suggestion):
@copilot this is not correct, it should be:

```jsx
component Test() {
  let x = 0;
  if (x === 0)
    if (x === 1) x = 2;
    else x = 3;
  <div>{x}</div>;
}
```

Code after:
  <div>{x}</div>
}`;
				const expected = `component Test() {
  let x = 0;
  if (x === 0) x = 1;
  else x = 2;
  <div>{x}</div>
}`;

				const result = await format(input, { singleQuote: true });
				expect(result).toBeWithNewline(expected);
			});

			it('should format nested if statements with non-block bodies', async () => {
				const input = `component Test() {
  let x = 0;
  if (x === 0) if (x === 1) x = 2; else x = 3;
  <div>{x}</div>
}`;
				const expected = `component Test() {
  let x = 0;
  if (x === 0)
    if (x === 1) x = 2;
    else x = 3;
  <div>{x}</div>
}`;

				const result = await format(input, { singleQuote: true });
				expect(result).toBeWithNewline(expected);
			});
		});
	});
});
Repository: trueadm/ripple
File: packages/ripple/src/compiler/phases/2-analyze/index.js (JavaScript)

Code before:
						'Cannot have a `children` prop on an element',
						state.analysis.module.filename,
						attribute,
						context.state.loose ? context.state.analysis.errors : undefined,
					);
				}
			}
		}

		return {
			...node,
			children: node.children.map((child) => visit(child)),
		};
	},

	Text(node, context) {
		mark_control_flow_has_template(context.path);

		if (
			is_children_template_expression(
				/** @type {AST.Expression} */ (node.expression),
				context.state,
			)
		) {
			error(
				'`children` cannot be rendered using text interpolation. Use `<@children />` instead.',
				context.state.analysis.module.filename,
				node.expression,
				context.state.loose ? context.state.analysis.errors : undefined,
			);
		}

		context.next();
	},

	AwaitExpression(node, context) {
		const parent_block = get_parent_block_node(context);

		if (is_inside_component(context)) {
			if (context.state.metadata?.await === false) {
				context.state.metadata.await = true;
			}

			if (
				parent_block !== null &&
				parent_block?.type !== 'Component' &&
				!context.state.ancestor_server_block &&
				!(
					parent_block.type === 'TryStatement' &&
					parent_block.pending &&
					is_inside_try_block(parent_block, context)

Review comment (suggestion):
you need to use just `<children />`

`@` is only for dynamic components, e.g. via trackSplit

and change the errors messages in tests to reflect

Code after:
						'Cannot have a `children` prop on an element',
						state.analysis.module.filename,
						attribute,
						context.state.loose ? context.state.analysis.errors : undefined,
					);
				}
			}
		}

		return {
			...node,
			children: node.children.map((child) => visit(child)),
		};
	},

	Text(node, context) {
		mark_control_flow_has_template(context.path);

		if (
			is_children_template_expression(
				/** @type {AST.Expression} */ (node.expression),
				context.state,
			)
		) {
			error(
				'`children` cannot be rendered using text interpolation. Use `<children />` instead.',
				context.state.analysis.module.filename,
				node.expression,
				context.state.loose ? context.state.analysis.errors : undefined,
			);
		}

		context.next();
	},

	AwaitExpression(node, context) {
		const parent_block = get_parent_block_node(context);

		if (is_inside_component(context)) {
			if (context.state.metadata?.await === false) {
				context.state.metadata.await = true;
			}

			if (
				parent_block !== null &&
				parent_block?.type !== 'Component' &&
				!context.state.ancestor_server_block &&
				!(
					parent_block.type === 'TryStatement' &&
					parent_block.pending &&
					is_inside_try_block(parent_block, context)
Repository: ueberdosis/tiptap
File: packages/core/src/utilities/mergeAttributes.ts (TypeScript)

Code before:

          mergedAttributes[key] = [...existingClasses, ...insertClasses].join(' ')
        } else if (key === 'style') {
          const newStyles: string[] = value ? value.split(';') : []
          const existingStyles: string[] = mergedAttributes[key] ? mergedAttributes[key].split(';') : []

          const newStyleProperties = newStyles.map(newStyle => newStyle.split(':'))
          const existingStyleProperties = existingStyles.map(existingStyle => existingStyle.split(':'))

          const newStyleKeys = newStyleProperties.map(property => property[0])

          const updatedExistingStyleProperties = existingStyleProperties.filter(
            existingStyleProperty => !newStyleKeys.includes(existingStyleProperty[0]),
          )

          const existingStyleAttributes = updatedExistingStyleProperties.map(
            updatedExistingStyleProperty => updatedExistingStyleProperty.join(':'),
          ).join('; ')

          const newStyleAttributes = newStyleProperties.map(
            valueStyleProperty => valueStyleProperty.join(':'),
          ).join('; ')

          const mergedStyleAttributes = [existingStyleAttributes, newStyleAttributes].filter(attribute => !!attribute)

          mergedAttributes[key] = mergedStyleAttributes.join('; ')
        } else {
          mergedAttributes[key] = value
        }
      })

      return mergedAttributes
    }, {})
}


Review comment (suggestion):
There has to be an easier way to do this, couldn't you just roll this up into a Map or an object (acting as `Record<AttributeName, AttributeValue>`)

Also would be great to have tests to understand the desired behaviors

Code after:

          mergedAttributes[key] = [...existingClasses, ...insertClasses].join(' ')
        } else if (key === 'style') {
          const newStyles: string[] = value ? value.split(';').map((style: string) => style.trim()).filter(Boolean) : []
          const existingStyles: string[] = mergedAttributes[key] ? mergedAttributes[key].split(';').map((style: string) => style.trim()).filter(Boolean) : []

          const styleMap = new Map<string, string>()

          existingStyles.forEach(style => {
            const [property, val] = style.split(':').map(part => part.trim())

            styleMap.set(property, val)
          })

          newStyles.forEach(style => {
            const [property, val] = style.split(':').map(part => part.trim())

            styleMap.set(property, val)
          })

          mergedAttributes[key] = Array.from(styleMap.entries()).map(([property, val]) => `${property}: ${val}`).join('; ')
        } else {
          mergedAttributes[key] = value
        }
      })

      return mergedAttributes
    }, {})
}
Repository: ueberdosis/tiptap
File: packages/react/src/useEditor.ts (TypeScript)

Code before:
  useEffect(() => {
    let isMounted = true

    const instance = new Editor(options)

    setEditor(instance)

    instance.on('transaction', () => {
      requestAnimationFrame(() => {
        requestAnimationFrame(() => {
          if (isMounted) {
            forceUpdate()
          }
        })
      })
    })

    return () => {
      isMounted = false
    }
  }, deps)

  useEffect(() => {
    return () => {
      editor?.destroy()
    }
  }, [editor])

  return editor
}


Review comment (bug):
@svenadlung Won't this new logic lead to memory leaks (and potentially other negative side effects)? As of this PR, whenever `deps` change, a `new Editor()` is created. However, this code now only `destroy`s the `editor` on _final_ unmount. So it will _not_ be calling `destroy` on any intermediary `editor`s that are created as `deps` change.

Maybe this can be fixed by moving the `destroy()` call to within the cleanup callback for the `useEffect` above that creates the `editor`. Something like

```ts
  useEffect(() => {
    let isMounted = true

    editorRef.current = new Editor(options)

    editorRef.current.on('transaction', () => {
      requestAnimationFrame(() => {
        requestAnimationFrame(() => {
          if (isMounted) {
            forceUpdate({})
          }
        })
      })
    })
    return () => {
      isMounted = false
      editorRef.current?.destroy();
    }
  }, deps)
```

Similarly, using `editorRef.current` as a dependency for `useEffect` for changing event handlers will not work (see explanation here https://stackoverflow.com/a/60476525/4543977). This means that if the editor changes as `deps` change, it will seemingly _not_ be updating any of the callbacks on the new editor instance. So each new `editor` instance will not be set up correctly.

---

I see the comment in the linked issue https://github.com/ueberdosis/tiptap/issues/4482#issuecomment-1820825154 referencing https://github.com/ueberdosis/tiptap/commit/6984ea11d79c059c55f045038e7758a34872a3f2 as being problematic. Seems that "Destroy editor in safe" PR https://github.com/ueberdosis/tiptap/pull/4000 was potentially the incorrect solution to its bug, and I think similarly this is a bandaid to attempt to fix a new bug that PR introduced, but unfortunately will probably introduce new bugs itself. 😕  (For what it's worth, I am still on 2.0.3 and do not experience the `[Bug]: Collaboration Cursor flickering` issue.)

Code after:

  useEffect(() => {
    let isMounted = true

    editorRef.current = new Editor(options)

    editorRef.current.on('transaction', () => {
      requestAnimationFrame(() => {
        requestAnimationFrame(() => {
          if (isMounted) {
            forceUpdate({})
          }
        })
      })
    })

    return () => {
      isMounted = false
    }
  }, deps)

  useEffect(() => {
    return () => {
      return editorRef.current?.destroy()
    }
  }, [])

  return editorRef.current
}
Repository: ueberdosis/tiptap
File: packages/extension-task-item/src/task-item.ts (TypeScript)

Code before:
  name: 'taskItem',

  addOptions() {
    return {
      nested: false,
      HTMLAttributes: {},
      taskListTypeName: 'taskList',
    }
  },

  content() {
    return this.options.nested ? 'paragraph block*' : 'paragraph+'
  },

  defining: true,

  addAttributes() {
    return {
      checked: {
        default: false,
        keepOnSplit: false,
        parseHTML: element => {
          const dataChecked = element.getAttribute('data-checked')

          return dataChecked != null && dataChecked !== 'false'
        },
        renderHTML: attributes => ({
          'data-checked': attributes.checked,
        }),
      },
    }
  },

  parseHTML() {
    return [
      {
        tag: `li[data-type="${this.name}"]`,
        priority: 51,
      },
    ]
  },

  renderHTML({ node, HTMLAttributes }) {
    return [
      'li',
      mergeAttributes(this.options.HTMLAttributes, HTMLAttributes, {
        'data-type': this.name,
      }),
      [
        'label',
        [

Review comment (suggestion):
I think I would do `element.getAttribute('data-checked') === '' || element.getAttribute('data-checked') === 'true'` To be explicit about what we accept

Code after:
  name: 'taskItem',

  addOptions() {
    return {
      nested: false,
      HTMLAttributes: {},
      taskListTypeName: 'taskList',
    }
  },

  content() {
    return this.options.nested ? 'paragraph block*' : 'paragraph+'
  },

  defining: true,

  addAttributes() {
    return {
      checked: {
        default: false,
        keepOnSplit: false,
        parseHTML: element => {
          const dataChecked = element.getAttribute('data-checked')

          return dataChecked == null || dataChecked === 'true'
        },
        renderHTML: attributes => ({
          'data-checked': attributes.checked,
        }),
      },
    }
  },

  parseHTML() {
    return [
      {
        tag: `li[data-type="${this.name}"]`,
        priority: 51,
      },
    ]
  },

  renderHTML({ node, HTMLAttributes }) {
    return [
      'li',
      mergeAttributes(this.options.HTMLAttributes, HTMLAttributes, {
        'data-type': this.name,
      }),
      [
        'label',
        [