Popover error message #2739

Golddirio · 2025-07-28T13:08:12Z

What is the purpose of this pull request?

Fixes #2688

Overview of changes:
Popover error messages are placed in the content position which is consistent right now.

Anything you'd like to highlight/discuss:
Now the error messages are shown below:

Testing instructions:
NA

Proposed commit message: (wrap lines at 72 characters)
Fix the inconsistency in positions of popover error messages

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

Major (when you make incompatible API changes)
Minor (when you add functionality in a backward compatible manner)
Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

what was the old feature that was made obsolete

any replacement feature (if any), and

how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

Copilot

Pull Request Overview

This PR fixes the inconsistency in positions of popover error messages by modifying how error messages are displayed in the Popover component and how they are generated in the include panel processor.

Adds local error handling and display functionality to the Popover Vue component
Replaces error node creation with inline error slots in the popover content area
Introduces error validation and DOM error capturing mechanisms

Reviewed Changes

Copilot reviewed 2 out of 66 changed files in this pull request and generated 6 comments.

File	Description
packages/vue-components/src/Popover.vue	Adds local error state, validation methods, and inline error message display
packages/core/src/html/includePanelProcessor.ts	Replaces error node creation with inline error slots for consistent positioning

Copilot · 2025-08-03T15:35:14Z

packages/vue-components/src/Popover.vue

+    },
+    isValidUrl(url) {
+      try {
+        (() => new URL(url))();


The immediately invoked function expression (IIFE) pattern (() => new URL(url))() is unnecessarily complex. Replace with new URL(url) directly since the constructor already throws on invalid URLs.

Suggested change

(() => new URL(url))();

new URL(url);

Copilot · 2025-08-03T15:35:14Z

packages/vue-components/src/Popover.vue

+          this.localError = errorEl.textContent;
+          errorEl.remove();
+        }
+      });


Direct DOM querying with document.querySelectorAll breaks Vue's component encapsulation and reactive data flow. Consider using Vue's ref system or emitting events to parent components instead of global DOM manipulation.

Suggested change

});

// Check for error messages within this component only

const errorEl = this.$refs.popoverError;

if (errorEl && (errorEl.textContent.includes('popover') || errorEl.textContent.includes('src'))) {

// Optionally, further check if the error is related to this popover

if (errorEl.textContent.includes(this.src) || errorEl.textContent.includes(this.header)) {

this.localError = errorEl.textContent;

}

}

Copilot · 2025-08-03T15:35:15Z

packages/vue-components/src/Popover.vue

+        if (errorEl.textContent.includes(this.src)
+            || errorEl.textContent.includes(this.header)) {
+          this.localError = errorEl.textContent;
+          errorEl.remove();


Directly removing DOM elements with errorEl.remove() can cause memory leaks and break Vue's reactivity. If error elements need to be removed, use Vue's conditional rendering with v-if instead.

Suggested change

errorEl.remove();

// Instead of removing the element, hide it to avoid breaking Vue's reactivity

errorEl.style.display = 'none';

Copilot · 2025-08-03T15:35:15Z

packages/vue-components/src/Popover.vue

    }
+
+    .popover-error-message {
+        color: #f00;


[nitpick] The hardcoded color value #f00 is a magic number. Consider using a CSS custom property or a semantic color variable from your design system for better maintainability.

Suggested change

color: #f00;

color: var(--popover-error-color, #f00);

Copilot · 2025-08-03T15:35:15Z

packages/core/src/html/includePanelProcessor.ts

+        {
+          type: 'tag',
+          name: 'div',
+          attribs: { style: 'color: red;' },


[nitpick] Inline styles with hardcoded colors should use CSS classes instead for consistency and maintainability. Consider using a CSS class like 'error-message' that can be styled centrally.

Suggested change

attribs: { style: 'color: red;' },

attribs: { class: 'error-message' },

Copilot · 2025-08-03T15:35:15Z

packages/core/src/html/includePanelProcessor.ts

-    cheerio(node).replaceWith(createErrorNode(node, error));
+    const errorMsg
+      = 'URLs are not allowed in the \'src\' attribute.<br>'
+      + `File: ${context.cwf}<br>`


Using HTML <br> tags in error messages couples the error content with presentation. Consider using \n for line breaks and let the display layer handle formatting, or use an array of message parts.

Suggested change

+ `File: ${context.cwf}<br>`

= 'URLs are not allowed in the \'src\' attribute.\n'

+ `File: ${context.cwf}\n`

gerteck · 2025-08-04T06:09:33Z

Hi @Golddirio,

just a preliminary test, will give a more detailed PR review. I noticed that the popover error message no longer shows for URLs in your branch, see:

gerteck

Hi @Golddirio , thank you for your PR!

Firstly, just a small nit, do try to keep your PR clean by not committing the font files unless there are any specific font changes, and for the frontend bundles as well. Link.

I believe you are on the right track, do feel free to clarify any comments.

I would suggest refining the processing logic and minimizing any changes to the vue component to avoid introducing any unnecessary coupling between the two. Ideally, the vue popover component should not need to be too aware of how the src attribute processing logic functions.

Think about why in one case, the error is generated at the location of the popover, and in the other case, it is not. There might already be a way to generate any similar error at the location of the popover then without modifying the logic or introducing if-else clauses in the vue component, with some slight tweaking.

I think with some refinement, you could achieve a simpler and more elegant fix. Great effort!

As a stretch goal, you could even consider adding a few testcases to verify that the popover appears at the desired location in the HTML dom.

gerteck · 2025-08-04T07:32:54Z

packages/core/src/html/includePanelProcessor.ts

+    node.children = createPopoverInlineErrorSlot(errorMsg);
    return context;
  }



Perhaps it would be helpful to understand how the original implementation displays the error message in-place without touching the logic in the separate vue UI components

gerteck · 2025-08-04T07:42:33Z

packages/vue-components/src/Popover.vue

+    },
+    findAndHandleGlobalErrors() {
+      // Look for error messages that might have been injected elsewhere
+      const popoverErrors = Array.from(document.querySelectorAll('.popover-error'))


Just curious, where do the elements with class popover-error originate from? I can't seem to find where such elements would be injected or modified upstream

gerteck · 2025-08-04T07:50:25Z

packages/vue-components/src/Popover.vue

      type: String,
      default: 'top',
    },
+    src: {


Should the popover expect this attribute?

markbind/packages/core/src/html/includePanelProcessor.ts

Lines 398 to 404 in f681976

const attributeSlotElement: NodeOrText[] = createSlotTemplateNode('content', actualContent);

node.children = node.children ? attributeSlotElement.concat(node.children) : attributeSlotElement;

delete node.attribs.src;

return childContext;

}

Our processPopoverSrc upstream function deletes this attribute, would it be better that the vue component remains unaware of a potential src attribute?

AgentHagu

Hi @Golddirio, thanks for the contribution. I'd just like to follow up on @gerteck's comments.

Think about why in one case, the error is generated at the location of the popover, and in the other case, it is not. There might already be a way to generate any similar error at the location of the popover then without modifying the logic or introducing if-else clauses in the vue component, with some slight tweaking.

Just as gerteck mentioned, look at the original code to see why there is differing behavior between error locations for different errors. In particular, after some investigation there seems to be some strange behavior between cheerio's replaceWith function and our createErrorNode function.

createErrorNode actually doesn't "create" a new node, it mutates the given element into an error node.
This leads to an issue when used with replaceWith, since we are accidentally replacing a node with itself. From my own testing, this often leads to strange behavior like placing the node at the end or deleting other nodes.

With this knowledge, think about a way to standardize error location behavior. Good luck!

Golddirio added 2 commits July 28, 2025 20:33

Fix inconsistency of popover error message position

4ba3872

Fix inconsistency of popover error message position

c4a1a0f

Golddirio marked this pull request as ready for review July 28, 2025 13:10

damithc requested review from Copilot, gerteck and AgentHagu and removed request for gerteck August 3, 2025 15:34

Copilot AI reviewed Aug 3, 2025

View reviewed changes

gerteck requested changes Aug 4, 2025

View reviewed changes

AgentHagu reviewed Aug 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Popover error message #2739

Popover error message #2739

Uh oh!

Golddirio commented Jul 28, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 3, 2025

Uh oh!

Copilot AI Aug 3, 2025

Uh oh!

Copilot AI Aug 3, 2025

Uh oh!

Copilot AI Aug 3, 2025

Uh oh!

Copilot AI Aug 3, 2025

Uh oh!

Copilot AI Aug 3, 2025

Uh oh!

gerteck commented Aug 4, 2025

Uh oh!

gerteck left a comment •

edited

Loading

Uh oh!

gerteck Aug 4, 2025

Uh oh!

gerteck Aug 4, 2025

Uh oh!

gerteck Aug 4, 2025

Uh oh!

AgentHagu left a comment

Uh oh!

Uh oh!

-      });
+      // Check for error messages within this component only
+      const errorEl = this.$refs.popoverError;
+      if (errorEl && (errorEl.textContent.includes('popover') || errorEl.textContent.includes('src'))) {
+        // Optionally, further check if the error is related to this popover
+        if (errorEl.textContent.includes(this.src) || errorEl.textContent.includes(this.header)) {
+          this.localError = errorEl.textContent;
+        }
+      }

	errorEl.remove();
	// Instead of removing the element, hide it to avoid breaking Vue's reactivity
	errorEl.style.display = 'none';

	attribs: { style: 'color: red;' },
	attribs: { class: 'error-message' },

	+ `File: ${context.cwf}<br>`
	= 'URLs are not allowed in the \'src\' attribute.\n'
	+ `File: ${context.cwf}\n`

	const attributeSlotElement: NodeOrText[] = createSlotTemplateNode('content', actualContent);
	node.children = node.children ? attributeSlotElement.concat(node.children) : attributeSlotElement;

	delete node.attribs.src;

	return childContext;
	}

Popover error message #2739

Are you sure you want to change the base?

Popover error message #2739

Uh oh!

Conversation

Golddirio commented Jul 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

gerteck commented Aug 4, 2025

Uh oh!

gerteck left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerteck Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

gerteck Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

gerteck Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

AgentHagu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gerteck left a comment •

edited

Loading