Skip to content

FIX: Move ] label-escape into MarkdownEscaper so image-in-link works#41

Merged
gschlager merged 1 commit into
mainfrom
followup/escape-link-label-in-escaper
May 12, 2026
Merged

FIX: Move ] label-escape into MarkdownEscaper so image-in-link works#41
gschlager merged 1 commit into
mainfrom
followup/escape-link-label-in-escaper

Conversation

@gschlager
Copy link
Copy Markdown
Member

Summary

Follow-up to #40. The earlier fix escaped ] at the bracket-emitting site in UrlTag/EmailTag with an unconditional text.gsub("]") { "\\]" }. That regressed nested markdown that legitimately produces unescaped ] — most concretely an image inside a link:

<a href=""><img src=""/></a>

ImageTag renders the image as ![](src), and the tag-level gsub turned the label into [![\](src)](href) — Discourse renders that as literal text.

The escape rule is contextual to the text content, not to how it gets wrapped. This PR moves the decision to where escaping already happens:

  • MarkdownEscaper#escape grows an in_link_label: kwarg. When set and the result contains ], it post-processes the result to escape every ]. The non-label hot path is unchanged — one boolean check.
  • Renderer#render_text detects an Url/Email ancestor via has_parent? and passes the flag through. Only plain Text nodes flow through this — nested tag output (ImageTag, UploadTag, etc.) doesn't touch render_text, so its structural ] is preserved.
  • UrlTag and EmailTag go back to plain [#{text}](#{href}) wrappers.

Performance

Benchmarked against main with YJIT on Ruby 3.4. Hot path (non-label) is within noise:

Input main this PR
Plain text 1.82M i/s 1.79M i/s
Text with specials 245k i/s 246k i/s
Text with brackets 264k i/s 264k i/s

The label path's result.include?("]") fast-path is locked in by a spec asserting the same string instance is returned when no ] is present.

Test plan

  • bundle exec rspec — 3117 examples, 0 failures
  • bin/mutant run on changed subjects — 309/309 mutations killed (100% coverage)
  • bin/lint — no offenses
  • Benchmark (bench_escaper.rb) — no hot-path regression vs main

Related

The previous fix (#40) escaped ] in UrlTag/EmailTag with an
unconditional `text.gsub("]") { "\\]" }`. That regressed nested
markdown that legitimately produces unescaped ] — for example
an image inside a link:

    <a><img src="…"/></a>

renders the image as `![](src)`, and the tag-level gsub turned the
label into `[![\](src)](href)` — broken.

The escape rule is contextual to the text content, not to how it
gets wrapped. Move the decision to where escaping already happens:

- MarkdownEscaper#escape grows an `in_link_label:` kwarg. When set
  and the result contains ], it post-processes the result to escape
  every ]. The non-label hot path is unchanged (one boolean check).
- Renderer#render_text detects an Url/Email ancestor via
  has_parent? and passes the flag through. Only plain Text nodes
  flow through this — nested tag output (ImageTag, UploadTag, etc.)
  doesn't touch render_text, so its structural ] is preserved.
- UrlTag and EmailTag go back to a plain "[#{text}](#{href})"
  wrapper.

Performance: benchmarked the hot path (non-label) against main at
1.79–1.84M i/s for plain text and ~245k i/s for text with specials
— no measurable regression. The label path's no-`]` fast-return
preserves the input string instance (locked in by a spec).
@gschlager gschlager merged commit a8945bc into main May 12, 2026
8 checks passed
@gschlager gschlager deleted the followup/escape-link-label-in-escaper branch May 12, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant