[kumascript] Display error on invalid Live Sample

[karlhorky]

Summary

Originally reported in mdn/kumascript#1214, which was just recently closed by @schalkneethling

Hi there! First of all, thanks for all of the work that is done as part of MDN and the Learn Web Development documentation. It is an invaluable resource for developers of all skill levels.

Live samples can be invalidated if the matching id does not exist (see @SphinxKnight's tweet):

Basically the "live sample" system uses the id of the headings and is case sensitive aaaand the last edit on the page changed those headings.

As mentioned in my tweet, it would be awesome if an error was displayed when an invalid live sample was saved:

Would be cool if these types of errors wouldn't be possible

One solution proposed by @SphinxKnight was to edit macros/EmbedLiveSample.ejs and macros/LiveSampleURL.ejs:

I guess a realistic solution could be an edit on https://github.com/mdn/kumascript/blob/master/macros/LiveSampleURL.ejs or https://github.com/mdn/kumascript/blob/master/macros/EmbedLiveSample.ejs to check for the existence of the section/id.

Although not completely preventing these errors from happening, this would at least be a step in the right direction.

---

Better, more complex solution: Validate live samples as they are saved and do not allow saving an invalid document state.

Although, as Julien G mentioned:

This would only display an alert on the saved page but I'm afraid that solution 1 (checking beforehand) is "too far" regarding the current state of Kuma (the underlying backend for MDN)

URL

https://developer.mozilla.org/en-US/docs/Learn

Reproduction steps

1. Create and deploy an invalid live sample
2. It's broken without any notice to the author or user

Expected behavior

Some notice should be displayed during authorship / usage of the website

Actual behavior

It's broken without any notice to the author or user

Device

Other (specify below)

Browser

Other (specify below)

Browser version

Other version (specify below)

Operating system

Other (specify below)

Screenshot

No response

Anything else?

Not browser or operating-system specific

Validations

• I have read the Community Participation Guidelines.
• I have verified that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• I have checked that this is a concrete bug. For Q&A open a GitHub Discussion.

[wbamberg]

I think this is a good idea. For your "better solution", we could run it as a CI check in mdn/content - we already do this for various things like uncompressed image files.

One issue is that technically the ID argument is now optional in live samples, although there are some unresolved issues with this (#5005, #5006) so it's not widely used. But we could at least validate the places where the ID argument is used.

@OnkarRuikar , you've done a lot of stuff in this area lately, wdyt?

[OnkarRuikar]

The issue mentioned in OP is no longer valid because, now if the id is not found then the live sample generator goes one level up and collects all css, html, and js code blocks.
But this automatic collection of code blocks some times grabs unwanted code as well. This has generated a lot of silent errors on the content pages. Noticeable ones have been fixed recently. But I believe many are still lingering.

There other issues related to the macro. To be on the same page here are some issues:

## Example 1

### HTML

```html
<h1>one</h1>
```

Works fine shows "one".
{{EmbedLiveSample('Old title', '100', '100')}}

---------------------------------------------------------------------

## Examples 2

```html
<h1>2</h1>
```

This shows "2" but "two" is ignored
{{EmbedLiveSample('b', '100', '100')}}

### HTML

```html
<h1>two</h1>
```

---------------------------------------------------------------------

## Examples 3

### Html

```html
<h1>three</h1>
```

This shows "one".
{{EmbedLiveSample('html', '100', '100')}}

---------------------------------------------------------------------

## Example 4

```js
let let a = 3;
```
Logs syntax error in user's browser.
{{EmbedLiveSample('Example 4', '100', '100')}}

To reproduce the issues, paste this entire section in a content page.
If you know/find any then let us know.

---

@karlhorky

Better, more complex solution: Validate live samples as they are saved and do not allow saving an invalid document state.

For contributors using git, we do not have git pre-commit hooks in mdn/content repo. It is not as strict as mdn/yari.
For contributors using GitHub, we have PR Companion that tells us errors found in modified files. For example, mdn/content#17365 (comment) look for the macro related flaws listed under macros: sections. The reviewers are supposed to look at the preview URLs and check the flaws before merging the PRs.

---

@wbamberg

But we could at least validate the places where the ID argument is used.

How about we add flaw cheeking for these issues? ID checking can be done in the macros. And for syntax errors we can have dedicated flaw checker in mdn/yari/tree/main/build/flaws

[karlhorky]

For contributors using GitHub, we have PR Companion that tells us errors found in modified files. For example, mdn/content#17365 (comment) look for the macro related flaws listed under macros: sections. The reviewers are supposed to look at the preview URLs and check the flaws before merging the PRs.

Seems also fine to me 👍 (although I'm not experienced with the codebase / processes)

[caugner]

@karlhorky I believe the original issue has been fixed by #2490/#3973/#4940 and potentially other PRs, so I'm marking this issue as not planned (aka "can't repro").

@OnkarRuikar Feel free to open an issue for the edge cases you may have found, even though we would really only act if it impacts content/translated-content. I had a quick look, but didn't see what's wrong.

FYI The algorithm for extracting the live samples can be found here, if you would like to dig:

yari/kumascript/src/api/util.js

Lines 280 to 320 in 6d493e4

	extractLiveSampleObject(iframeID) {
	const sectionID = iframeID.substr("frame_".length);
	if (hasHeading(this.$, sectionID)) {
	const result = Object.create(null);
	const sample = this.getSection(sectionID);
	// We have to wrap the collection of elements from the section
	// we've just acquired because we're going to search among all
	// descendants and we want to include the elements themselves
	// as well as their descendants.
	const $ = cheerio.load(`<div>${this.$.html(sample)}</div>`);
	for (const part of LIVE_SAMPLE_PARTS) {
	const src = $(
	`.${part}, pre[class*="brush:${part}"], pre[class*="${part};"]`
	)
	.map((i, element) => $(element).text())
	.get()
	.join("\n");
	// The string replacements below have been carried forward from Kuma:
	// * Bugzilla 819999: &nbsp; gets decoded to \xa0, which trips up CSS.
	// * Bugzilla 1284781: &nbsp; is incorrectly parsed on embed sample.
	result[part] = src ? src.replace(/\u00a0/g, " ") : null;
	}
	if (!LIVE_SAMPLE_PARTS.some((part) => result[part])) {
	throw new KumascriptError(
	`unable to find any live code samples for "${sectionID}" within ${this.pathDescription}`
	);
	}
	return result;
	} else {
	// We're here because we can't find the sectionID, so instead we're going
	// to find the live-sample iframe by its id (iframeID, NOT sectionID), and
	// then collect the closest blocks of code for the live sample.
	const result = collectClosestCode(findSectionStart(this.$, iframeID));
	if (!result) {
	throw new KumascriptError(
	`unable to find any live code samples for "${sectionID}" within ${this.pathDescription}`
	);
	}
	return result;
	}
	}

yari/kumascript/src/api/util.js

Lines 120 to 140 in 6d493e4

	function collectClosestCode($start) {
	const $el = findTopLevelParent($start);
	for (const $level of collectLevels($el)) {
	const pairs = LIVE_SAMPLE_PARTS.map((part) => {
	const selector = `.${part}, pre[class*="brush:${part}"], pre[class*="${part};"]`;
	const $filtered = $level.find(selector).add($level.filter(selector));
	return [
	part,
	$filtered
	.map((i, element) => cheerio.load(element).text())
	.get()
	.join("\n"),
	];
	});
	if (pairs.some(([, code]) => !!code)) {
	$start.prop("title", $level.first(":header").text());
	return Object.fromEntries(pairs);
	}
	}
	return null;
	}