Skip to content

FEATURE: Parse BBCode natively with markdown-it rules instead of BBob - #165

Open
Alteras1 wants to merge 12 commits into
mainfrom
dev/integration
Open

Alteras1 wants to merge 12 commits into
mainfrom
dev/integration

Conversation

@Alteras1

Copy link
Copy Markdown
Collaborator

Previously, BBob rendered the whole post to HTML before markdown-it ran, so bbcode broke core markdown and other Discourse features when mixed with them (headings, lists, polls, math, [details], emphasis across tags), and unclosed or mis-nested tags lost content.

This change parses bbcode as block, inline and core rules in the same markdown-it pass as core, so the two nest inside each other and render as XenForo did (every newline a <br>, mis-nested tags closed with their parent). It also removes BBob and keeps <template> CSS/scripts and spoilers out of excerpts, emails and search.

@Mondrethos

Copy link
Copy Markdown
Contributor

Ran a comparison against #164 using a larger local xF sample. This genuinely replaces BBob with native markdown-it rules, and the mixed-Markdown improvements are real. The measurements show a modest overall speed improvement, rather than a substantially lighter payload, with several regressions worth fixing before merge.

Tested revisions:

Corpus and measurements

10,000 sampled xF posts + nine known complex reference posts, totaling 15.8 MB of message text. The sample used 64 bounded windows spread across the xf_post table in the SQL dump, with deterministic selection within each window. This is byte-stratified sampling, not a uniform random sample of every post.

Each revision ran three times, sequentially with alternating order, through Discourse's actual JS cooking pipeline and sanitizer, with the same installed plugin features and real Ruby helper callbacks. Each run used an isolated MiniRacer/V8 context and a fresh Markdown engine per message. Timings exclude Rails startup, Ruby PrettyText.cleanup, post-bake processing, and browser layout. Hardware: Ryzen 9 7950X / Linux x64.

Measurement #164 / BBob #165 / native
Parser JS, identically minified 35,513 bytes 35,923 bytes
Same parser payload, gzip 12,169 bytes 11,808 bytes
Whole corpus, median total rendering time 15.13 s 13.48 s
34 posts ≥50 kB, median combined rendering time 1.19 s 1.59 s
Cyberkill reference post, median rendering time 59.8 ms 148.8 ms
Retained V8 heap after corpus + explicit GC, median 10.93 MiB 11.00 MiB

Payload comparison uses the same Rails AMD transformation and Terser 5.51.2 compression/mangling, then gzip level 9. It includes the parser and its Markdown registration, excludes shared Discourse code/source maps, and is not a full production asset-download measurement. Heap numbers are retained-heap samples, not peak RSS.

That is 11% less total rendering time, 3% fewer gzipped parser bytes, and essentially unchanged retained heap. The large-post subset took 34% longer, and the public Cyberkill reference post took 2.49× as long.

Both revisions completed all 30,027 corpus renders each without exceptions. That does not establish content or visual equivalence for every historical post.

Reproduced regressions

1. Accordion delimiters inside inline code discard content

[accordion]{slide=Title}Before `{/slide}` after{/slide}[/accordion]

#164 preserves Before {/slide} after, with the delimiter displayed as code. #165 renders only Before and the opening backtick: the rest of the slide is discarded.

findTop treats the delimiter inside the code span as the section close. Confirmed through cooking and browser DOM/visual inspection. sections.js:123–126

2. Unmatched-tag scanning scales quadratically

Reproducer: "[b]x".repeat(N).

Five measurements per size produced these median rendering times:

Unmatched tags #164 #165
1,000 3.2 ms 47.2 ms
2,000 6.7 ms 177.1 ms
4,000 9.7 ms 687.1 ms

The 4,000-tag input is only 16 kB. Doubling the input approaches quadrupling native rendering time. Each distinct opener gets a separate findClose cache key and scans the remaining source again. scanner.js:111–159

3. Separate inline-code examples incorrectly suppress intervening BBCode

`[plain]`
[color=red]red[/color]
`[/plain]`

#164 displays the two code examples and formats the intervening text red. #165 leaves [color=red]red[/color] literal. The same problem occurs with displayed [code] / [/code] delimiters.

Literal BBCode regions are indexed before inline-code spans, so these inactive delimiters become one false literal region. Confirmed with both single-line and multiline variants. scanner.js:303–338

4. Nested [nobr] emits a visible line break

[b]x [nobr]a
b[/nobr][/b]

#164 renders x ab without a break; #165 inserts <br> between a and b. The nested tokens are hardened before nobr metadata is attached, while only the softbreak renderer checks that metadata. Confirmed in cooked HTML and the browser. bbcode-native.js:663–670

5. Named block icons retain desktop sizing on mobile

Reproducer: [block=dice]Example body[/block] at a 390px viewport.

The exact PR stylesheet, compiled with Sass, gives the default block background-size: 100% 20px, 50px, but the named dice block gets 100% 20px, auto. Its margin still shrinks to 57px, so the desktop-sized icon overlaps the content area.

The named variant's more-specific background shorthand overrides the mobile size/position longhands. Confirmed with Chromium computed styles and a screenshot. block.scss:63–70

6. Nesting repair treats an escaped opener as active

\[b][i]x[/b] y[/i]

The escaped [b] is literal. #164 keeps x[/b] y inside the italic span; #165 ends italics after x, because the repair pass nevertheless treats that escaped opening tag as active. scanner.js:170–193

Improvements and intentional differences

The native version successfully handles:

Separately, the global removal of paragraph elements and disabling of indented Markdown code also affect ordinary posts containing no custom BBCode. Those are documented design decisions, not accidental regressions, but they materially change #164's ordinary-Markdown contract and should be explicitly agreed on.

The direction looks worthwhile, but I would fix the content-loss/literal-context cases, worst-case scanning, nested nobr, and mobile CSS before merging. Removing BBob alone does not make the replacement substantially lighter.

No xF database restore/import, Discourse post writes, or rebake was performed. The sampled corpus stays private locally; only synthetic reproducers and already-public reference links are included here. Browser checks displayed isolated renderer output and the compiled stylesheet, not a deployment of this branch to the live composer.

@Mondrethos

Copy link
Copy Markdown
Contributor

Re-reviewed the updated branch and DESIGN.md. This is a substantial improvement: all six original exact reproducers now pass, and the previous large-post performance regression is reversed. I would still request changes for the remaining worst-case path and four new correctness regressions below.

Tested revisions:

Status of the previous findings

Previous finding Updated result
Accordion delimiter inside inline code discards content Content and code span preserved
Unmatched "[b]x".repeat(N) scales quadratically Fixed for this input; 4,000 tags fell from 641 ms to 21 ms in the fresh old/new comparison
Separate [plain] / [code] examples suppress intervening formatting Intervening text correctly renders red
Nested [nobr] emits a visible break No visible break
Named block icons retain desktop sizing on mobile Both default and named blocks use the correct 50px icon size
Escaped opener participates in nesting repair Escaped opener remains literal; italics extend correctly

Verified through the actual cooking pipeline, with browser checks for rendered output and mobile CSS. The broader worst-case scanning issue remains, despite the specific [b] fix.

Remaining findings

1. High: malformed-input performance still violates the stated 500,000-character goal

For "[code]x".repeat(N), five-run median rendering times were:

Repetitions Updated #165
1,000 74 ms
2,000 278 ms
4,000 1,052 ms

Doubling still approaches quadrupling.

More decisively, "[code]".repeat(83333) — 499,998 characters — hit the 25-second JavaScript timeout. #164 completed that input in approximately 5 ms.

Attribution matters: disabling this plugin reproduces most of the [code] cost. The dominant problem is Discourse's fallback parser, which the BBob preprocessing previously avoided, not the new tagPairs index itself. Nevertheless, the replacement exposes that regression and does not meet its documented input-size guarantee.

The plugin's literal-region scanner also still searches the remaining suffix repeatedly for unmatched literal tags. For "[plain]x".repeat(N), 4,000 / 8,000 / 16,000 repetitions took approximately 45 / 140 / 476 ms. Worst-case coverage needs to include these paths, not just unmatched [b].

2. Medium: a new early return breaks valid containers inside Markdown quotes

> [div]
> `x
>
> [/div]
> `

The previous #165 revision renders a <div> inside the blockquote. The updated revision displays [div] and [/div] literally.

The new early return assumes removing blockquote markers cannot expose a close. It can: removing > creates a blank line, invalidating a previously inferred code span and making [/div] active.

Retain the stripped-text fallback when Markdown transforms the line context.

3. Medium: an escaped backtick can now disable an accordion

[accordion]{slide=Title}a \`{/slide} x`[/accordion]

There is exactly one backslash before the first backtick. The previous #165 revision recognizes the slide. The updated revision leaves the entire accordion literal.

The literal-range scanner treats the escaped backtick as a code-span opener, hiding the only {/slide}. The newly added literal-range check in section scanning exposes this defect.

Honor Markdown escapes when indexing code spans, while retaining the protection for genuine inline-code examples.

4. Medium: keyboard activation of links inside inline spoilers is broken

[inlinespoiler][url=https://example.com]open link[/url][/inlinespoiler]

Reveal the spoiler, focus the link, and press Enter.

The new keydown handler catches the link's bubbling event, prevents navigation, and closes the spoiler instead.

Confirmed in Chromium with the actual old/new decorators on isolated markup: the old version follows the link; the updated version cancels the event. Separately confirmed that the actual cooker emits the anchor inside the inline spoiler.

Only handle keyboard toggling when the spoiler itself is the event target, not an interactive descendant.

5. Medium: valid fractional animation keyframes are silently dropped

[animation=fade]
[keyframe=.5%]opacity:0;[/keyframe]
[keyframe=100%]opacity:1;[/keyframe]
[/animation]

The new selector allowlist rejects .5%, although it is valid CSS.

The previous #165 cooker emits both frames, and Chromium recognizes 0.5% and 100%. The updated cooker emits only 100%.

Accept leading-dot decimal percentages without weakening the selector-injection protection.

Updated performance comparison

Reused the original 10,000 sampled xF posts plus nine reference posts, totaling 15.8 MB of message text. As in the prior review, this is a byte-stratified sample using 64 bounded windows across the SQL dump, not a uniform random sample of all posts.

The figures below use three clean sequential runs per revision with alternating order, through Discourse's actual JS cooking pipeline and sanitizer, with the same installed plugin features and real Ruby helper callbacks. Each run used an isolated MiniRacer/V8 context and a fresh Markdown engine per message. Hardware: Ryzen 9 7950X / Linux x64.

Measurement #164 / BBob Updated #165
Whole corpus, median rendering time 17.09 s 14.30 s
34 posts ≥50 kB, median combined rendering time 1.30 s 1.00 s
Cyberkill reference post, median rendering time 61.5 ms 41.6 ms
Parser JS, identically minified 35,513 bytes 38,659 bytes
Same payload, gzip level 9 12,157 bytes 12,724 bytes
Retained V8 heap after corpus + explicit GC, median 11.03 MiB 11.35 MiB

That is approximately 16% faster overall, 24% faster on large posts, and 32% faster on Cyberkill. The previous large-post performance regression is reversed.

The parser is now approximately 9% larger minified and 5% larger gzipped than #164, not lighter. Payloads were remeasured together using the same Rails AMD transformation, Terser 5.51.2 compression/mangling, and gzip level 9. This includes the parser and its Markdown registration, excludes shared Discourse code/source maps, and is not a full production asset-download measurement.

Both revisions completed all 30,027 measured corpus renders each without exceptions. That does not establish content or visual equivalence across the corpus.

Timings exclude Rails startup, Ruby PrettyText.cleanup, post-bake processing, and browser layout. Heap figures are retained-heap samples, not peak RSS. Browser checks used isolated renderer output, the compiled stylesheet, and the actual decorators, not a deployment of this branch to the live composer.

Design decisions

DESIGN.md usefully distinguishes intended behavior from defects. It still explicitly leaves owner agreement open for removing paragraph elements and disabling indented Markdown code. Those affect ordinary posts too and remain a separate decision, not an accidental regression.

The worst-case linearity and 500,000-character safety claims need correction until the remaining paths are addressed.

Overall: the six targeted fixes and the performance improvement are real. I would address the five findings above before merging.

No xF database restore/import, Discourse post writes, or rebake was performed. The sampled corpus stays private locally; the reproducers above are synthetic.

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