Skip to content

Conversation

@larbish
Copy link
Contributor

@larbish larbish commented Nov 20, 2025

No description provided.

@vercel
Copy link

vercel bot commented Nov 20, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
content-studio Ready Ready Preview Comment Nov 21, 2025 10:38pm

Comment on lines +22 to +27
const props = defineProps({
draftItem: {
type: Object as PropType<DraftItem>,
required: true,
},
})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component receives a :read-only prop from its parent (ContentEditor.vue, line 70) but does not define this prop in its props definition. This will cause Vue warnings about unknown props and the prop value will be ignored.

View Details
📝 Patch Details
diff --git a/src/app/src/components/content/ContentEditorTipTap.vue b/src/app/src/components/content/ContentEditorTipTap.vue
index 56b1fc6..ca4d458 100644
--- a/src/app/src/components/content/ContentEditorTipTap.vue
+++ b/src/app/src/components/content/ContentEditorTipTap.vue
@@ -24,6 +24,11 @@ const props = defineProps({
     type: Object as PropType<DraftItem>,
     required: true,
   },
+  readOnly: {
+    type: Boolean,
+    required: false,
+    default: false,
+  },
 })
 
 const document = defineModel<DatabasePageItem>()
@@ -84,6 +89,10 @@ async function setEditorJSON(document: DatabasePageItem) {
 
 // TipTap to Markdown
 watch(tiptapJSON, async (json) => {
+  if (props.readOnly) {
+    return
+  }
+
   const mdc = await tiptapToMDC(json!)
 
   const updatedDocument: DatabasePageItem = {

Analysis

Missing readOnly prop in ContentEditorTipTap component

What fails: ContentEditor.vue passes :read-only="readOnly" prop to ContentEditorTipTap.vue (line 70), but ContentEditorTipTap does not define this prop in its props definition (lines 22-27). This causes the readonly state to be ignored by the TipTap editor, allowing users to edit content even when the parent component intends for the editor to be in read-only mode.

How to reproduce:

  1. Open ContentEditor.vue and navigate to a read-only context (where readOnly prop is true)
  2. The ContentEditorTipTap component is rendered with :read-only="true"
  3. Attempt to edit content in the TipTap editor
  4. Expected: Editor should be disabled and prevent editing
  5. Actual: Editor allows editing because the prop is not defined, so the readonly state is never checked

Result: The TipTap editor ignores the readonly state because:

  • The prop is passed by parent but not declared in ContentEditorTipTap
  • Without the prop declaration, props.readOnly is inaccessible in the component
  • The watcher that converts TipTap JSON to markdown updates the document regardless of readonly state
  • Contrast with ContentEditorCode.vue which properly defines and uses the readOnly prop (lines 13-19, 58)

Expected behavior: Per TipTap Editor documentation, the editor supports editable configuration to disable editing. The component should:

  1. Accept and define the readOnly prop
  2. Respect it by preventing document updates when readonly=true
  3. Match the pattern already implemented in ContentEditorCode.vue

Comment on lines +34 to +37
export function omit(obj: Record<string, unknown>, keys: string | string[]) {
return Object.fromEntries(Object.entries(obj)
.filter(([key]) => !keys.includes(key)))
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The omit function's type signature declares it accepts keys: string | string[] but the implementation only works correctly with string[]. If a plain string is passed, the .includes() check treats it as a substring search instead of an array element lookup.

View Details
📝 Patch Details
diff --git a/src/app/src/utils/object.ts b/src/app/src/utils/object.ts
index dff109d..fa210e3 100644
--- a/src/app/src/utils/object.ts
+++ b/src/app/src/utils/object.ts
@@ -32,6 +32,7 @@ export function isEmpty(obj: Array<unknown> | Record<string, unknown> | string |
 }
 
 export function omit(obj: Record<string, unknown>, keys: string | string[]) {
+  const keyArray = Array.isArray(keys) ? keys : [keys]
   return Object.fromEntries(Object.entries(obj)
-    .filter(([key]) => !keys.includes(key)))
+    .filter(([key]) => !keyArray.includes(key)))
 }
diff --git a/src/module/src/runtime/utils/object.ts b/src/module/src/runtime/utils/object.ts
index f264ed1..76bbd25 100644
--- a/src/module/src/runtime/utils/object.ts
+++ b/src/module/src/runtime/utils/object.ts
@@ -1,11 +1,13 @@
 export const omit = (obj: Record<string, unknown>, keys: string | string[]) => {
+  const keyArray = Array.isArray(keys) ? keys : [keys]
   return Object.fromEntries(Object.entries(obj)
-    .filter(([key]) => !keys.includes(key)))
+    .filter(([key]) => !keyArray.includes(key)))
 }
 
 export const pick = (obj: Record<string, unknown>, keys: string | string[]) => {
+  const keyArray = Array.isArray(keys) ? keys : [keys]
   return Object.fromEntries(Object.entries(obj)
-    .filter(([key]) => keys.includes(key)))
+    .filter(([key]) => keyArray.includes(key)))
 }
 
 export function doObjectsMatch(base: Record<string, unknown>, target: Record<string, unknown>) {

Analysis

Type signature mismatch in omit() and pick() functions allows substring matching instead of key matching

What fails: The omit() and pick() functions in both src/app/src/utils/object.ts and src/module/src/runtime/utils/object.ts declare type signature keys: string | string[] but only correctly handle string[]. When a string is passed, .includes() performs substring matching on the string instead of element matching on an array.

How to reproduce:

// With fixed implementation:
function omit(obj, keys) {
  const keyArray = Array.isArray(keys) ? keys : [keys]
  return Object.fromEntries(Object.entries(obj)
    .filter(([key]) => !keyArray.includes(key)))
}

// Before fix - passing string:
omit({a: 1, ab: 2}, 'ab')  
// Returns: {} (incorrect)
// 'ab'.includes('a') = true → filters out 'a'
// 'ab'.includes('ab') = true → filters out 'ab'

// After fix - passing string:
omit({a: 1, ab: 2}, 'ab')  
// Returns: {a: 1} (correct)

Result: Before fix, passing a string causes incorrect filtering due to substring matching. For example, 'ab'.includes('a') returns true (substring found), causing the key 'a' to be filtered out even though it shouldn't be. After fix, both string and array parameters work identically and correctly.

Expected: The type signature promises keys: string | string[] - the implementation should handle both types correctly. This fix converts the string to an array internally while maintaining backward compatibility with existing array usage and fixing the latent bug.

Files modified:

  • src/app/src/utils/object.ts - Fixed omit() function
  • src/module/src/runtime/utils/object.ts - Fixed both omit() and pick() functions

Comment on lines +20 to +34
watch(() => props.editor, (editor) => {
if (!editor) return
const updateUrl = () => {
const { href } = editor.getAttributes('link')
url.value = href || ''
}
updateUrl()
editor.on('selectionUpdate', updateUrl)
onBeforeUnmount(() => {
editor.off('selectionUpdate', updateUrl)
})
}, { immediate: true })
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onBeforeUnmount lifecycle hook is being called inside a watcher callback, which causes it to be registered every time the watcher fires instead of just once. This can lead to memory leaks and improper cleanup of event listeners.

View Details
📝 Patch Details
diff --git a/src/app/src/components/editor/EditorLinkPopover.vue b/src/app/src/components/editor/EditorLinkPopover.vue
index 460f135..d71936d 100644
--- a/src/app/src/components/editor/EditorLinkPopover.vue
+++ b/src/app/src/components/editor/EditorLinkPopover.vue
@@ -10,6 +10,9 @@ const props = defineProps<{
 const open = ref(false)
 const url = ref('')
 
+const updateUrl = ref<(() => void) | null>(null)
+let currentEditor: Editor | null = null
+
 const active = computed(() => props.editor.isActive('link'))
 const disabled = computed(() => {
   if (!props.editor.isEditable) return true
@@ -18,21 +21,31 @@ const disabled = computed(() => {
 })
 
 watch(() => props.editor, (editor) => {
-  if (!editor) return
+  if (currentEditor && updateUrl.value) {
+    currentEditor.off('selectionUpdate', updateUrl.value)
+  }
+
+  if (!editor) {
+    currentEditor = null
+    return
+  }
 
-  const updateUrl = () => {
+  currentEditor = editor
+  updateUrl.value = () => {
     const { href } = editor.getAttributes('link')
     url.value = href || ''
   }
 
-  updateUrl()
-  editor.on('selectionUpdate', updateUrl)
-
-  onBeforeUnmount(() => {
-    editor.off('selectionUpdate', updateUrl)
-  })
+  updateUrl.value()
+  editor.on('selectionUpdate', updateUrl.value)
 }, { immediate: true })
 
+onBeforeUnmount(() => {
+  if (currentEditor && updateUrl.value) {
+    currentEditor.off('selectionUpdate', updateUrl.value)
+  }
+})
+
 watch(active, (isActive) => {
   if (isActive && props.autoOpen) {
     open.value = true

Analysis

Multiple onBeforeUnmount registrations in EditorLinkPopover watcher causes memory leaks

What fails: The onBeforeUnmount() lifecycle hook is called inside the watch() callback (line 31), registering a new cleanup callback each time the watcher fires instead of once during setup. When the editor prop changes multiple times, multiple cleanup callbacks accumulate and multiple event listeners can remain registered, causing memory leaks.

How to reproduce:

  1. Mount EditorLinkPopover component with an editor prop
  2. Change the editor prop value three times (simulating different editor instances)
  3. Unmount the component
  4. Expected: One cleanup callback removes one listener
  5. Actual: Three cleanup callbacks fire, one for each onBeforeUnmount registration

Result: Each watcher callback registers a new cleanup function via onBeforeUnmount(). If the editor prop changes 3 times, 3 cleanup callbacks are registered and executed on unmount, all trying to remove listeners from the same (most recent) editor. Previous editors may retain dangling listeners. According to Vue 3 Composition API documentation, lifecycle hooks must be called synchronously during component setup, not inside watchers which execute later.

Expected: Cleanup should happen exactly once during component unmount via a single onBeforeUnmount() hook registered at setup level. When the watched prop changes, old listeners should be manually cleaned up in the watcher before registering new ones, following the pattern described in the Vue 3 lifecycle guide.

Verification: Vue 3's injectHook function (used by onBeforeUnmount) registers lifecycle callbacks to the component instance. Each call to onBeforeUnmount() adds a new callback to the instance's hook array. When this occurs inside a watcher callback that fires multiple times, multiple callbacks accumulate, all executing during unmount.

Comment on lines +107 to +117
return {
type: 'element',
tag: 'p',
children: [
{
type: 'text',
value: 'XXX',
},
],
props: {},
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback handler for unknown TipTap node types returns a hardcoded placeholder 'XXX', which will corrupt document content if encountered.

View Details
📝 Patch Details
diff --git a/src/app/src/utils/tiptap/tiptapToMdc.ts b/src/app/src/utils/tiptap/tiptapToMdc.ts
index 48b6e1d..4f9fb08 100644
--- a/src/app/src/utils/tiptap/tiptapToMdc.ts
+++ b/src/app/src/utils/tiptap/tiptapToMdc.ts
@@ -101,19 +101,25 @@ export function tiptapNodeToMDC(node: JSONContent): MDCRoot | MDCNode | MDCNode[
     return tiptapToMDCMap[node.type!](node)
   }
 
-  // fallback to unknown node
-  // TODO: all unknown nodes should be handled
+  // fallback to unknown node type
+  console.warn(`[tiptapToMDC] Unknown node type: "${node.type}". Node will be rendered as a generic element.`, node)
 
+  // For unknown nodes, try to preserve content if it exists
+  if (node.content && node.content.length > 0) {
+    return {
+      type: 'element',
+      tag: 'div',
+      children: node.content.flatMap(tiptapNodeToMDC),
+      props: { 'data-unknown-node-type': node.type },
+    }
+  }
+
+  // For leaf nodes with no content, create an empty element
   return {
     type: 'element',
-    tag: 'p',
-    children: [
-      {
-        type: 'text',
-        value: 'XXX',
-      },
-    ],
-    props: {},
+    tag: 'div',
+    children: [],
+    props: { 'data-unknown-node-type': node.type },
   }
 }
 

Analysis

TipTap node type fallback returns hardcoded 'XXX' placeholder, corrupting document content

What fails: The tiptapNodeToMDC() function in src/app/src/utils/tiptap/tiptapToMdc.ts (lines 107-117) returns a hardcoded 'XXX' placeholder when encountering unknown TipTap node types not defined in tiptapToMDCMap. This causes document content corruption when custom extensions or unmapped node types are used.

How to reproduce:

  1. Open the editor in the content editor component
  2. Click the "image" toolbar button which inserts a type: 'image-picker' node (defined in src/app/src/utils/tiptap/extensions/image-picker.ts)
  3. The node is processed by tiptapToMDC() on editor changes (line 83 in ContentEditorTipTap.vue)
  4. Since 'image-picker' is not in tiptapToMDCMap, the fallback handler is triggered
  5. Document will contain literal "XXX" text instead of proper node content

Result: When an 'image-picker' node (or any unmapped custom node type) is converted from TipTap to MDC format, the fallback returns: { type: 'element', tag: 'p', children: [{ type: 'text', value: 'XXX' }], props: {} }

Expected: Unknown node types should be handled gracefully:

  • Content should be preserved, not replaced with a placeholder
  • A warning should be logged to help developers identify missing node type handlers
  • The node should be rendered as a generic element (div) with metadata about the unknown type, rather than corrupting content with hardcoded text

Fixed by:

  • Replacing the hardcoded 'XXX' with proper content preservation logic
  • Adding console.warn() to help developers identify unmapped node types
  • Using 'div' element with 'data-unknown-node-type' attribute for unknown nodes
  • Recursively processing content from unknown nodes to preserve data

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants