feat: add Obsidian-style embeds and wiki-link display text - #301
Conversation
jannis-baum
left a comment
There was a problem hiding this comment.
Hello, thanks for opening the PR! I finally had some time to look at it now.
Have you thoroughly tested all the features you are proposing? My first step with feature PRs like these is to look at the markdown-additional.md test file which you added examples to that do not exist in this repo, i.e. it is not testable. Once I created some of the files you are referencing there I found a few issues:
- the sizing only works for images, if you e.g. add it to an embedded Markdown viewer or embedded video and it won't resize because your CSS overwrites it
- embedding a PDF doesn't work and Vivify instead makes the browser download it; fixing this might be a bigger and separate topic so unless there is a very simple and clean solution it may be better to explicitly not support it yet
- there may be more, please test everything yourself
Overall the idea of the rendering test files is that they work as they are without needing to add other files. We explicitly do not want to add big files like images or videos to this repo to keep it small, those files will have to go into my assets repo, you can open a PR there as well to add videos and images you reference from here. A PDF can be small enough to be feasible to add directly to this repo, I would propose e.g. exporting a small, simple Vivify-rendered document (see the guide) to PDF and adding it so you can reference it from your test file.
Once you have made the rendering test document showcase all your features without requiring files that not everyone will have, please thoroughly test it and make sure everything you implemented works to the best of your knowledge, and that you have understood the code and would have implemented it like this yourself as well.
A common problem with vibecoding is that models love to implement things they should just take an existing library function for. This creates a messy codebase, unnecessary maintenance overhead, and bugs. Following are some notes from Claude on this in your PR.
1. Reuse: escAttr reimplements md.utils.escapeHtml — and does it incompletely
embeds.ts hand-rolls:
function escAttr(value: string): string {
return value.replace(/["&]/g, (char) => (char === '"' ? '"' : '&'));
}markdown-it already exposes md.utils.escapeHtml (escapes & < > "), which is the canonical helper — markdown-it's own docs use it. The from-scratch version omits < and >. With html: true this isn't a new XSS surface (an author can already write raw HTML — see mermaid.ts, which doesn't escape at all), so it's not a security regression, but it's exactly the "don't reimplement a library function" case you called out. Pass md into renderEmbedHtml and use md.utils.escapeHtml.
2. Minimality: several bits of dead/redundant code
VIDEO_MIME/AUDIO_MIMEmaps: every extension inVIDEO_EXTS/AUDIO_EXTShas a corresponding MIME entry, so theconst typeAttr = type ? … : ''fallback branch is unreachable. And for a single<source>, the browser sniffs the type from the extension anyway — Obsidian doesn't emit these. ~20 lines that could go, or the maps kept but the dead ternary dropped.escAttris applied towidth/height, which are guaranteed\d+byparseSize's regex — escaping digits is a no-op.parseSizereturnsheight: match[2] ?? undefined—match[2]is alreadyundefinedwhen the group doesn't match, so?? undefinedis redundant.
3. Duplication with wiki-links.ts (convention/DRY)
embeds.ts re-implements the [[/]] scanning loop, the | pipe-split, and the extension detection that already live in wiki-links.ts. They've diverged too: wiki-links.ts uses pbasename(x).indexOf('.') > -1 to detect an extension, while embeds.ts uses pextname(x). Not wrong today, but two copies of the same intent drifting apart. A small shared helper (bracket-scan + target/pipe parse) would cut both files down and keep them in sync.
Relatedly, a minor convention point (not a blocker): the sibling wiki-links.ts builds output via markdown-it's token API (state.push('link_open') + attrSet), which auto-escapes; embeds.ts switches to raw html_inline strings. Raw-HTML is precedented here (mermaid.ts, front-matter.ts, dot.ts), so it's acceptable — but at least the image case could use an image token with attrSet to match the neighbour and get escaping for free.
This PR adds support for:
This was vibecoded and mainly added because I wanted the feature to be available for everyone to use.
Tests and documentation are included.