feat: store richtext data as HTML rather than a Prosemirror tree by tefkah · Pull Request #1239 · knowledgefutures/pubplatform

tefkah · 2025-05-12T15:59:23Z

Issue(s) Resolved

High-level Explanation of PR

Alternatively/in addition, i have a triplet of videos.

just goes over the text of this PR (optional)

summary.mp4

shows the serialization/deserialization working with the editor (optional)

demo.mp4

shows/goes over the changes to the context editor (recommended. gets cut off slightly at the end)

editor-stuff.mp4

This PR changes how we store RichText: rather than storing the full ProseMirror tree, we first convert it to HTML and only store that.

Some reasons to do this:

Easier to convert format: rather than having to create $N$ prosemirror-to-X tools, we only need to use the (native) DOMSerializer, and then are able to create (much easier) $N-1$ (the 1 being HTML) html-to-X tools. This could integrate better with our existing rehype/remark converters.
Much easier to display a "pretty good" version of RichText. Rather than any clients (like the site builder) needing to ship and implement some prosemirror-to-html functionality, we can "just" do a quick __dangerouslySetInnerHTML to get 85% of the way there
Much easier to index for fulltext search! kind of trivial to remove all the <> from HTML and index that, much harder to do so for a json blob.
html is cool, and prosemirror is cringe

Schema

The RichText schema is now just a plain String schema. I wanted to add some regex maybe but decided against it, as we could still handle "" as valid HTML.

FTS

Previously the prosemirror tree was indexed for full text search extremely naively. I wanted to make the body more searchable, so we now do something like

on update/creation, check if pubvalue is RichText
if so, remove all <.*>, and index the remainder
now only the content of the body gets searched

This could be improved by weighting eg headings and non-headings differently, which is something we should do in the future. That way results will be ranked higher if eg ponies occurs in an h1 rather than in a span tag.

Where does the conversion happen

This was the most difficult part to figure out for me and I've gone back and forth quite a few times on where exactly the stored HTML should be converted to ProseMirror and vice versa when the created ProseMirror tree from the editor should be converted back to HTML again to be stored. At first I had this all on the client, then I had it on server and then I had it on the client again.

Finally, I decided that the conversion from ProseMirror to HTML should happen on the server again, but that this conversion only happens on pages/actions dealing with the ContextEditor (ie the PubEditor). Everywhere else treats a RichText field as HTML.

Without trying to go on a tangent, the reasoning is roughly

only the things that need to think about prosemirror think about it
converting ProseMirror to HTML on the client can be slow, especially if we were to do that on onChange. so a clean "ContextEditor converts back and forth between HTML and ProseMirror` doesn't really cut it

Hacks

(see 3rd video for demo/some explanation)

Although previously this was meant as an optimization in the case that the Context Editor was converting between HTML and ProseMirror, I kind of left it in because it does make the Editor faster.
Instead of tracking the state of the Editor in the form by doing field.onChange, we now instead expose a ref which retrieves the value of the Context Editor when it is needed: on submission time of the form.

This is kind of evil and not at all React-y, but it does make the editor much faster, at least twice as fast for big documents.

I think we should keep it even though it kind of goes against our React-y model, as we can still treat everything inside of the ContextEditor React-y. One reason I think why this is justified is because the Context Editor is not really a controlled component, but with the onChange hook it kind of makes it seem like it is, ie we don't pass the field.value back into the ProseMirror Editor. The Context Editor controls its own state anyway and we just need a way to get that state out, because we can't manipulate it outside of the Context Editor and expect it to stay the same anyway.

Now this might be a problem in the future and we might want to revert it when dealing with updates to transcluded values, eg the ContextAtoms. Maybe we do want to update the value of a transcluded starter:abstract in the Editor when we change the starter:abstract value in the form field above. But we can burn that bridge when we get to it!

Test Plan

Go to legacy
CMD+K Ponies, go to the one pub that pubs up
Observe the beautiful html content
Create a new pub
Try to cram as many different nodes in the richtext body as you can
Save + reload
hopefully they stay the same!

Screenshots (if applicable)

Notes

validation

…client

github-actions · 2025-05-12T16:14:49Z

PR Preview Action v1.6.1
Preview removed because the pull request was closed.
2025-05-29 18:30 UTC

tefkah · 2025-05-12T17:27:50Z

basically this allows for much better indexing of html by first stripping all the tags essentially.
this could be improved by creating to sets to index: headings and body, and weighting the headings more heavily in the search algo. i didn't do that for now as that's kinda overkill.

tefkah · 2025-05-12T17:31:28Z

 	],
-	toDOM: () => ["math-inline", { class: "math-node" }, 0],
+	toDOM: (node: Node) => renderMath(node, "math-inline"),
 } satisfies NodeSpec;


im very happy that this works: basically math is now stored as MathML! this would maybe prove to be a problem, but luckily katex stores the raw "input" as an <annotation> node, which we can use to still easily parse the HTML (katex does not provide any mathml-to-katex functionality).

we could also still allow maybe raw katex as the body, that would make importing custom html a bit easier, eg

<math-display> \int_a^b x^2dx </math-display>

tefkah · 2025-05-12T17:32:05Z

+	// this is not nice, i would like to avoid manually calling `document.createElement`
+	// as we now need to keep track of setting `global.document` when this is called on the server
+	// forturnately, we only really server render the HTML when importing Legacy text content
+	// could be easily solved if `toDOM` was passed the document object, but ProseMirror does not seem to intend providing this
+	// https://discuss.prosemirror.net/t/getting-a-hold-of-the-document-used-by-prosemirror-from-todom/8392
+	if (!global.document) {
+		throw new Error(
+			"document not found. Trying to serialize math in a non-browser environment. To do this, set `global.document` to eg a `JSDOM` document before serializing."
+		);
+	}


this is very annoying, if you know of a better way to have prosemirror convert a raw html string to a node let me know!

tefkah · 2025-05-12T17:32:37Z


-export function getJsonSchemaByCoreSchemaType(coreSchemaType: CoreSchemaType, config?: unknown) {
-	switch (coreSchemaType) {
-		case CoreSchemaType.Boolean:
-			return Boolean;
-		case CoreSchemaType.DateTime:
-			return DateTime;
-		case CoreSchemaType.Email:
-			return Email;
-		case CoreSchemaType.FileUpload:
-			return FileUpload;
-		case CoreSchemaType.MemberId:
-			return MemberId;
-		case CoreSchemaType.Null:
-			return Null;
-		case CoreSchemaType.Number:
-			return Number;
-		case CoreSchemaType.NumericArray:
-			return getNumericArrayWithMinMax(config);
-		case CoreSchemaType.RichText:
-			return RichText;
-		case CoreSchemaType.String:
-			return String;
-		case CoreSchemaType.StringArray:
-			return getStringArrayWithMinMax(config);
-		case CoreSchemaType.URL:
-			return URL;
-		case CoreSchemaType.Vector3:
-			return Vector3;
-		default:
-			const _exhaustiveCheck: never = coreSchemaType;
-			return _exhaustiveCheck;
-	}
+const schemaMap = {
+	[CoreSchemaType.Boolean]: () => Boolean,
+	[CoreSchemaType.DateTime]: () => DateTime,
+	[CoreSchemaType.Email]: () => Email,
+	[CoreSchemaType.FileUpload]: () => FileUpload,
+	[CoreSchemaType.MemberId]: () => MemberId,
+	[CoreSchemaType.Null]: () => Null,
+	[CoreSchemaType.Number]: () => Number,
+	[CoreSchemaType.NumericArray]: (config) => getNumericArrayWithMinMax(config),
+	[CoreSchemaType.RichText]: () => RichText,
+	[CoreSchemaType.String]: () => String,
+	[CoreSchemaType.StringArray]: (config) => getStringArrayWithMinMax(config),
+	[CoreSchemaType.URL]: () => URL,
+	[CoreSchemaType.Vector3]: () => Vector3,
+} as const satisfies Record<CoreSchemaType, (config?: unknown) => (typeof Schemas)[CoreSchemaType]>;
+
+export function getJsonSchemaByCoreSchemaType<C extends CoreSchemaType>(
+	coreSchemaType: C,
+	config?: unknown
+) {
+	return schemaMap[coreSchemaType](config) as ReturnType<(typeof schemaMap)[C]>;
 }


this just provides better type info

tefkah · 2025-05-12T17:33:11Z

+export function ShowMore({
+	children,
+	className,
+	variant,
+	showMoreText = "Show more",
+	showLessText = "Show less",
+	animate = true,
+	...props
+}: ShowMoreProps) {
+	const [expanded, setExpanded] = useState(false);
+
+	const toggleExpanded = () => setExpanded((prev) => !prev);
+
+	return (
+		<div className={cn("w-fit", className)} {...props}>
+			<div
+				className={cn(
+					showMoreVariants({ variant }),
+					expanded && "max-h-full",
+					animate && "transition-[max-height] duration-300 ease-in-out",
+					!expanded &&
+						"after:absolute after:bottom-0 after:left-0 after:h-16 after:w-full after:bg-gradient-to-t after:from-background after:to-transparent"
+				)}
+			>
+				{children}
+			</div>
+			<div className="mt-2 flex justify-center">
+				<Button
+					variant="ghost"
+					size="sm"
+					onClick={toggleExpanded}
+					className="flex items-center gap-1"
+				>
+					{expanded ? (
+						<>
+							{showLessText}
+							<ChevronUp size={16} />
+						</>
+					) : (
+						<>
+							{showMoreText}
+							<ChevronDown size={16} />
+						</>
+					)}
+				</Button>
+			</div>
+		</div>
+	);
+}


todo: ideally this does not show "show more" if the thing is smaller than the max size

also this should maybe be a proper <details><summary> element for accessibilty

tefkah · 2025-05-12T17:34:24Z

this is this article, which is one of the more complex ones we have JOTE
ideally we would have a nice large body that tests every feature of the editor

https://journal.trialanderror.org/pub/jointregeneration/release/6

tefkah · 2025-05-22T13:22:00Z

 			}
+			const value = getter.current?.getCurrentState();
+
+			valuesPayload[slug] = value && serializeProseMirrorDoc(value.doc);


we could also just convert it to HTML here, let me know your opinion on that! that way, the server action does not need to be able to handle prosemirror at all, and we can maybe simplify the validation code somewhat

I'm all for converting it on the client! That should take some responsibility/load off the server.

tefkah · 2025-05-22T13:23:22Z

+import { htmlToProsemirrorServer, prosemirrorToHTMLServer } from "../editor/serialize-server";
+
 const validateAgainstContextEditorSchema = (value: unknown) => {
 	try {
+		if (typeof value === "string") {
+			const node = htmlToProsemirrorServer(value);
+
+			node.check();
+			// return renderNodeToHTML(node);
+			return { success: true, value };
+		}
+
 		const node = baseSchema.nodeFromJSON(value);
+
+		// TODO: reenable this
 		node.check();
-		return true;
-	} catch {
-		return false;
+
+		const html = prosemirrorToHTMLServer(node);
+
+		return { success: true, value: html };
+	} catch (e) {
+		return { success: false, error: e };
 	}
 };


this is somewhat of a big change, as it turns the validation into parsing + coercion. i don't think this is incorrect per se, but it's quite a large change.

this is not necessary per se anymore: we could, on form submission, make the client convert the prosemirror tree to html instead. let me know your opinion here!

I'm happy either way. I definitely like the idea of the client doing the ProseMirror->HTML conversion if that would take load off of the server. But I'm okay with this too.

tefkah · 2025-05-22T13:24:35Z

+CREATE OR REPLACE FUNCTION strip_html_tags(text_with_tags text)
+RETURNS text AS $$
+BEGIN
+    RETURN regexp_replace(
+        regexp_replace(
+            regexp_replace(
+                text_with_tags,
+                '<[^>]+>', -- removes HTML tags
+                ' ',
+                'gi'
+            ),
+            '&[^;]+;', -- removes HTML entities
+            ' ',
+            'gi'
+        ),
+        '\s+', -- collapse multiple spaces
+        ' ',
+        'g'
+    );
+END;
+$$ LANGUAGE plpgsql IMMUTABLE;


immutable means its a pure function. postgres allegedly does some stuff with that info

tefkah · 2025-05-22T13:25:13Z

i think i can legally include this, bc the license is CC BY. might need to include a header that does the attribution

tefkah · 2025-05-22T13:26:42Z

+	// this is slightly evil and should not be taken as an example of good api design
+	// it basically allows you to retrieve the value of the editor from the parent component
+	// whenever you want, rather than having to do it through `onChange` (which would also cause a re-render)
+	// this makes (some) sense in this case bc the editor is almost fully uncontrolled:
+	// you cannot pass in an `EditorState` that holds the actual value
+	useImperativeHandle(
+		props.getterRef,
+		() => ({
+			getCurrentState: () => editorState,
+		}),
+		[editorState]
+	);
+


this is nifty, but a bit hacky. as i mention in the body, we might want to make the context editor more of a controlled component in the future in order to sync transluded pubfields, eg if i change abstract:title in the form i might expect it to change automatically here. we should first figure out transclusion tho

tefkah · 2025-05-22T13:27:31Z

+	let toBeProcessedContent =
+		node instanceof Node ? node.content : Node.fromJSON(baseSchema, node).content;
+
+	const fragment = DOMSerializer.fromSchema(baseSchema).serializeFragment(toBeProcessedContent, {
+		document: doc,
+	});
+
+	const container = doc.createElement("div");
+	container.appendChild(fragment);


no sure if there's a more elegant way to do this

From about ten minutes of searching, no, it doesn't seem like there's a much better way.

tefkah · 2025-05-22T13:28:16Z

+export function ShowMore({
+	children,
+	className,
+	variant,
+	showMoreText = "Show more",
+	showLessText = "Show less",
+	animate = true,
+	...props
+}: ShowMoreProps) {
+	const [expanded, setExpanded] = useState(false);
+
+	const toggleExpanded = () => setExpanded((prev) => !prev);
+
+	return (
+		<div className={cn("w-fit", className)} {...props}>
+			<div
+				className={cn(
+					showMoreVariants({ variant }),
+					expanded && "max-h-full",
+					animate && "transition-[max-height] duration-300 ease-in-out",
+					!expanded &&
+						"after:absolute after:bottom-0 after:left-0 after:h-16 after:w-full after:bg-gradient-to-t after:from-background after:to-transparent"
+				)}
+			>
+				{children}
+			</div>
+			<div className="mt-2 flex justify-center">
+				<Button
+					variant="ghost"
+					size="sm"
+					onClick={toggleExpanded}
+					className="flex items-center gap-1"
+				>
+					{expanded ? (
+						<>
+							{showLessText}
+							<ChevronUp size={16} />
+						</>
+					) : (
+						<>
+							{showMoreText}
+							<ChevronDown size={16} />
+						</>
+					)}
+				</Button>
+			</div>
+		</div>
+	);
+}


also this should maybe be a proper <details><summary> element for accessibilty

Minio's latest client (https://github.com/minio/minio/releases/tag/RELEASE.2025-05-24T17-08-30Z) had a breaking change which made the old `mc config add` no longer work

3mcd

Awesome work man. No big requests, just a question or two!

3mcd · 2025-05-27T16:49:08Z

+	 * Refs which are able to retrieve the current state of the context editor
+	 * on demand, rather than having the form state manage the value (which is slower)
+	 */
+	contextEditorGetters: ContextEditorGetters;


Good comments!

3mcd · 2025-05-27T16:54:58Z

 			}
+			const value = getter.current?.getCurrentState();
+
+			valuesPayload[slug] = value && serializeProseMirrorDoc(value.doc);


I'm all for converting it on the client! That should take some responsibility/load off the server.

3mcd · 2025-05-27T16:56:04Z

+			const isNew = !v.valueId;
+			const isChanged = v.valueId && !isEqualWith(arrayDefaults[v.valueId], v);
+			return isNew || isChanged;
+		});


I like how you break this up into multiple statements instead of cramming it into a single, harder to read ternary!

3mcd · 2025-05-27T16:58:05Z

+	opts?: {
+		toJson?: boolean;
+	}
+): T => {


I gotta say, I'm a fan of using type aliases even for function with small signatures. While this isn't difficult to read, it does take extra time to scan/parse for me personally. I'd much rather both the generic and opts signatures be lifted into a type alias above. Feel free to ignore -- just my personal preference!

bc it's you i changed it <3

but i agree in the case of the generic here: that was kind of hard to read.
for one-of types like the opts here i personally find just inlining it easier to read, but i don't feel that strongly about it!

3mcd · 2025-05-27T16:58:30Z

 	}

-	return q.execute();
+	return autoCache(dbQuery).execute();


3mcd · 2025-05-27T16:59:35Z

+import { htmlToProsemirrorServer, prosemirrorToHTMLServer } from "../editor/serialize-server";
+
 const validateAgainstContextEditorSchema = (value: unknown) => {
 	try {
+		if (typeof value === "string") {
+			const node = htmlToProsemirrorServer(value);
+
+			node.check();
+			// return renderNodeToHTML(node);
+			return { success: true, value };
+		}
+
 		const node = baseSchema.nodeFromJSON(value);
+
+		// TODO: reenable this
 		node.check();
-		return true;
-	} catch {
-		return false;
+
+		const html = prosemirrorToHTMLServer(node);
+
+		return { success: true, value: html };
+	} catch (e) {
+		return { success: false, error: e };
 	}
 };


I'm happy either way. I definitely like the idea of the client doing the ProseMirror->HTML conversion if that would take load off of the server. But I'm okay with this too.

3mcd · 2025-05-27T17:01:06Z

+	let toBeProcessedContent =
+		node instanceof Node ? node.content : Node.fromJSON(baseSchema, node).content;
+
+	const fragment = DOMSerializer.fromSchema(baseSchema).serializeFragment(toBeProcessedContent, {
+		document: doc,
+	});
+
+	const container = doc.createElement("div");
+	container.appendChild(fragment);


From about ten minutes of searching, no, it doesn't seem like there's a much better way.

tefkah added 17 commits May 7, 2025 15:15

feat(serialization): add first pass at serialization

f5d42cb

feat(ui): add new show more component

ce77fcd

feat(serialization): add proper serialization libs

204fa4f

refactor(serialization): parse and convert prosemirror tree during

c33097f

validation

feat(fts|serialization): properly index html

8f49b17

fix(serialization): sort of fix loading

d9f4f20

fix: fix weird redis install issue

e92f261

dev: make legacy seed have useful richtext

550c767

dev: make legacy seed have useful richtext

71439d6

feat(math): serialize math to mathml and back reliably

642bbd1

feat(serialization): find a happy way to store html and serialize on …

9997e8f

…client

chore: remove old validation function, fix test

2b3c8b5

fix: fix the stories

e5bf99f

fix: really fix math, cleanup

b83d855

feat: add migration to clear current richtext values

e2b5978

Merge branch 'main' into tfk/html-richtext

276fffe

chore: meaningless change to trigger ci

6c740c6

tefkah added 4 commits May 12, 2025 18:43

fix: fix fts related type issues

53bb547

fix: fix contexteditor related type issues

162b545

chore: don't format html

0ced401

fix: fix test to work with figures

76e70e8

tefkah added the preview Auto-deploys a preview application label May 12, 2025

3mcd temporarily deployed to gh-654103159-pr-1239 May 12, 2025 17:23 Inactive

tefkah commented May 12, 2025

View reviewed changes

fix: find math display correctly

a3d9614

3mcd temporarily deployed to gh-654103159-pr-1239 May 22, 2025 12:41 Inactive

tefkah commented May 22, 2025

View reviewed changes

tefkah marked this pull request as ready for review May 22, 2025 13:29

tefkah requested review from 3mcd, allisonking and kalilsn and removed request for 3mcd May 22, 2025 13:29

Merge branch 'main' into tfk/html-richtext

22c80b2

3mcd temporarily deployed to gh-654103159-pr-1239 May 27, 2025 08:22 Inactive

fix?: use localhost instead of docker-host for minio init

25f761d

3mcd temporarily deployed to gh-654103159-pr-1239 May 27, 2025 10:05 Inactive

debug: add some logs

3edd460

3mcd temporarily deployed to gh-654103159-pr-1239 May 27, 2025 10:13 Inactive

fix?: upgrade aws?

4b4c059

3mcd temporarily deployed to gh-654103159-pr-1239 May 27, 2025 10:18 Inactive

fix(minio): use new minio client syntax after breaking change

7ced2c4

Minio's latest client (https://github.com/minio/minio/releases/tag/RELEASE.2025-05-24T17-08-30Z) had a breaking change which made the old `mc config add` no longer work

3mcd temporarily deployed to gh-654103159-pr-1239 May 27, 2025 10:54 Inactive

Merge branch 'main' into tfk/html-richtext

243d4ea

3mcd temporarily deployed to gh-654103159-pr-1239 May 27, 2025 13:31 Inactive

chore: merge

9881d52

3mcd temporarily deployed to gh-654103159-pr-1239 May 27, 2025 14:55 Inactive

3mcd approved these changes May 27, 2025

View reviewed changes

tefkah added 3 commits May 29, 2025 19:30

chore: merge

e77d8a5

fix: cave to erics CRAZY demands

0aa25f9

fix: remove unnecessary memo

71984ef

3mcd had a problem deploying to gh-654103159-pr-1239 May 29, 2025 18:18 Error

tefkah merged commit b5f6f5d into main May 29, 2025
17 checks passed

tefkah deleted the tfk/html-richtext branch May 29, 2025 18:30

Conversation

tefkah commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue(s) Resolved

High-level Explanation of PR

Schema

FTS

Where does the conversion happen

Hacks

Test Plan

Screenshots (if applicable)

Notes

Uh oh!

github-actions Bot commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

3mcd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tefkah commented May 12, 2025 •

edited

Loading

github-actions Bot commented May 12, 2025 •

edited

Loading