feat(markdown): render inline {@link Target} references as links#5
Conversation
Consolidated PR Review
PR SummaryTeaches the markdown pipeline to rewrite inline JSDoc/TSDoc Merge Readiness — BLOCKED ❌The PR is a clean, well-tested, almost entirely additive feature and is better than
1. Backward Compatibility — SAFE ✅Purely additive at every public surface. The sole signature change is a new optional parameter; all renames are module-private; the one behavioral delta to existing rendering fixes a latent invalid-HTML bug.
2. Code Quality — GOOD ✅Cohesive, well-decomposed new code with a thorough test matrix that mirrors the description's behaviour table. Both regex-state concerns were checked and come out safe; findings are minor.
3. Architecture — GOOD ✅Cleanly extends the existing pipeline. The new module is self-contained and the route-URL convention is centralized in
4. Security — SAFE ✅No regression introduced. The pipeline already runs
5. Observability — GOOD ✅Appropriate for a build-time + client-side docs generator. No new logging, but consistent with the target branch and project conventions, and no previously-surfaced error is newly swallowed.
6. Performance — FAIR
|
| Dimension | Verdict |
|---|---|
| Backward Compatibility | SAFE |
| Code Quality | GOOD |
| Architecture | GOOD |
| Security | SAFE |
| Observability | GOOD |
| Performance | FAIR |
| Lime Platform Issues | SAFE |
| Merge Readiness | BLOCKED |
Top Recommendations
- [High from Performance] Fix the ReDoS in
markdown-inline-links.ts:6-7, 41-42before merge. The unbounded whitespace + unterminated-brace path is the trigger. Concrete options: require the closing brace to follow within a bounded distance, make the optional label group non-empty / anchor its leading whitespace, or cap target length. Add a regression test with an unterminated{@link X+ long whitespace string asserting it returns promptly. - [Low from Code Quality / Architecture] While fixing the regexes, factor out the shared label-alternation tail so
URL_LINK_PATTERNandINLINE_LINK_PATTERNcannot drift, and consider building one from the other. - [Low from Code Quality / Observability] Make the integration-test skip visible (
markdown-example-integration.spec.ts:467) — a one-line note/warning on skip, or ensure CI buildskompendium.jsonbefore the test run, so the "real registry" coverage isn't silently absent. - [Low from Architecture] Optionally centralize the
#/type/${target}/#/component/${target}/route-shape so it isn't duplicated betweencreateLinkResolverandmarkdown-typelinks.ts:createLinkNode.
🤖 6-agent review via 6-agent-review skill. The single blocker is narrow and cheaply fixable; everything else is a net improvement over lime.
Addressed the 6-agent reviewThanks for the thorough pass. Summary of what changed and what I left as-is. Fixed the blocker: ReDoS in the
|
Consolidated PR Review (re-review @
|
| Dimension | Verdict |
|---|---|
| Backward Compatibility | SAFE |
| Code Quality | GOOD |
| Architecture | GOOD |
| Security | SAFE |
| Observability | GOOD |
| Performance | FAIR |
| Lime Platform Issues | SAFE |
| Merge Readiness | BLOCKED |
Top Recommendations
- [High from Performance] Eliminate the residual O(n²) in both
{@link}regexes (markdown-inline-links.ts:13, 15). The adjacent overlapping quantifiers([^\s}|]+)([^}]*)are the cause; require a delimiter after the target (e.g.\{@link\s+([^\s}|]+)(?=[\s|}])([^}]*)\}) or restructure so the target and tail can't redistribute a long run. Crucially, add a regression test that uses a long unbroken non-whitespace token (e.g.'{@link ' + 'a'.repeat(50000)), not spaces — the current 50k-space test passes without touching this path. - [Low from Backward Compatibility] Add a test (and decide intended behavior) for a URL
{@link https://…}written inside inline/fenced code — the pre-pass currently rewrites it rather than passing it through. - [Low from Code Quality] Add a test for the
{@link X |}empty-pipe-label case to lock in the (improved) bare-reference semantics the rewrite introduced. - [Low from Architecture] Add a one-line comment in
collectSkippableCode(markdown-typelinks.ts) noting that anchor-nested<code>is skipped becauseinlineLinksemits that shape — making the implicit cross-phase contract self-documenting.
🤖 6-agent re-review via 6-agent-review skill. The whitespace ReDoS and the other first-round items are addressed; the single remaining blocker is a narrower residual of the same backtracking class, reproduced above, with a small fix.
|
Thanks for the careful re-review — you were right that the first fix was incomplete. Fixed the residual backtracking cliff (commit
|
| input length | old regex | new regex |
|---|---|---|
| 50,000 chars | ~1,040 ms | 0.19 ms |
| 200,000 chars | ~16 s (per review) | 0.53 ms |
URL pattern ({@link http://a…), 50,000 chars: ~1,030 ms → 0.15 ms. Now linear.
Tests
Added long-unbroken-token regression tests ('a'.repeat(50000), not spaces) for both the mdast plugin path and the normalizeInlineLinkUrls URL pre-pass, asserting prompt return (<100 ms) + no rewrite. The existing space-based tests stay. Full suite: 126 passed, 6 skipped (the skips are the registry-gated integration suite). ESLint + prettier clean.
Also addressed (the Low items)
- Empty-pipe
{@link X |}: added a test locking in the bare-reference semantics the rewrite introduced (identifier wrapped in code, never a literal|). - URL
{@link}inside code: added two tests documenting this as a known, pre-existing gap — the URL pre-pass runs over raw text before parsing, so it can't distinguish a code span from prose and rewrites the URL even inside inline/fenced code. Non-URL references in code are still correctly passed through. Left as-is and documented rather than "fixed", since making the string pre-pass code-aware would mean reimplementing the parser's code-span detection; the input is developer-authored docs and this trade-off was already deliberate. - Cross-phase contract comment: added a comment in
collectSkippableCodeexplaining anchor-nested<code>is skipped becauseinlineLinksemits thatlink > inlineCodeshape.
All four changes are in the single fixup 65dba7b, since they all belong to the feature commit's files.
Consolidated PR Review (3rd pass @
|
| Dimension | Verdict |
|---|---|
| Backward Compatibility | SAFE |
| Code Quality | GOOD |
| Architecture | GOOD |
| Security | SAFE |
| Observability | GOOD |
| Performance | GOOD |
| Lime Platform Issues | SAFE |
| Merge Readiness | MERGE WITH CAVEATS |
Top Recommendations
- [Low from Performance] Optional belt-and-suspenders for the residual many-near-misses quadratic (
markdown-inline-links.ts:16, 18): bound the tail so a missing terminator can't drive an O(n²) multi-start scan — e.g. stop the tail at newlines ([^}\n]*, which also matches how{@link}is used on single lines) or cap target/tail length. Add a regression test using many{@linknear-misses with no}. Not a blocker; realistic docs never trigger it and any}defuses it. - [Low from Architecture] Optionally note in the
collectSkippableCodecomment that the anchor-nested-<code>skip is intentionally broader than theinlineLinkscontract (it suppresses re-linking for any<code>inside any<a>). - [Low from Code Quality] Consider hardening the wall-clock ReDoS timing assertions against CI starvation (e.g. a more generous bound, or measuring relative to a control) — low priority.
- [Low from Observability] If desired, surface a one-time dev
console.warnwhendocs.componentsis absent so a missing registry doesn't silently turn every component{@link}into inline code — optional for a build-time tool.
🤖 6-agent re-review via 6-agent-review skill. The ReDoS blocker from the prior two passes is resolved and verified; remaining items are all [Low] and do not gate merge.
|
Thanks for the third pass. With no blockers, I went through the four Addressed1. Residual many-near-misses quadratic (Performance) — fixed and verified linear. Measured (raw
Before scales O(n²) (4x the time per 2x the length); after scales linearly. Added three regression tests (single-line, multi-line, URL pre-pass) that fail fast against this shape, and re-confirmed the full behaviour matrix and all existing tests still pass. No real reference has a >1000-char label or crosses a line, so this is behaviour-neutral for actual docs. 2. Anchor-nested- Deliberately skipped3. Hardening the wall-clock timing assertions (Code Quality). The existing 4. Dev Verification
Single fixup commit |
Consolidated PR Review (4th pass @
|
| Dimension | Verdict |
|---|---|
| Backward Compatibility | SAFE |
| Code Quality | GOOD |
| Architecture | GOOD |
| Security | SAFE |
| Observability | GOOD |
| Performance | SAFE |
| Lime Platform Issues | SAFE |
| Merge Readiness | READY TO MERGE |
Top Recommendations
No actionable items remain. The blocker from passes 1–2 and the residual from pass 3 are resolved and verified. The only open notes are optional/cosmetic and need not gate merge:
- [Low from Security, pre-existing] Independent of this PR, the markdown pipeline has no sanitizer; if
kompendiummarkdown ever accepts untrusted input, addrehype-sanitize. Not introduced here. - [Low from Architecture, cosmetic] The double-add to the skip
SetincollectSkippableCodeis harmless; no change needed.
🤖 6-agent re-review via 6-agent-review skill. Clean across all dimensions — recommend merge.
|
Thanks for the thorough 4th pass — agreed, this is ready to merge. I'm not making any further changes. On the two remaining cosmetic notes, I confirm neither warrants action:
No new commits, tip unchanged. |
Fixed the failing CI and Lint checksTwo real failures (both, fittingly, were called out as
Local The Block Autosquash Commits check is expected to stay red by design — it gates merge until the |
When an inline `<code>` element already sits inside an `<a>` (for example, from a markdown link like `[label `Foo`](url)`), the typeLinks pass would wrap the type token in another anchor and produce a nested `<a>`, which is invalid HTML and confuses navigation. Generalises the existing `<pre><code>` skip-set into a more general "skippable code" set that also captures `<code>` inside any anchor ancestor, and leaves those code elements untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JSDoc/TSDoc `{@link Target}` references in component descriptions
previously surfaced in the generated docs as literal text. Adds a remark
plugin that rewrites them into mdast link nodes during the markdown
pipeline, supporting the standard syntactic forms:
{@link Target}
{@link Target Display text}
{@link Target | Display text}
{@link https://example.com | Display text}
Targets are resolved against the project's known types and component
tags (a new component registry mirrors the existing type registry).
Bare-identifier references — those without an explicit display label —
are wrapped in `<code>` so they visually match how type names render
elsewhere in the docs. Unresolved targets still render as inline code
rather than dangling syntax, and absolute URLs link directly.
References inside inline code (``{@link Foo}``) and fenced code blocks
are intentionally left untouched so the syntax itself can be documented.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a dedicated `kompendium-example-inline-links` example component
attached to `kompendium-markdown` so the rendered docs page shows each
inline `{@link …}` syntactic form alongside its source. The integration
test renders the example against the actual built `kompendium.json`
registry to keep the showcase honest as the docs build evolves.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ee32ee1 to
8d14c39
Compare
|
Excellent work @Kiarokh! |
Fork-side pair of upstream PR jgroth#181 by @Kiarokh. Same
main-based branch, so the identical change lands on both sides and the dailyupstream sync merges it as a no-op once upstream accepts.
Note
Per
FORK.md, this PR will show as "out-of-date with the base branch"permanently and by design (
limecarries the fork delta). Do not usethe Update branch button.
Summary
JSDoc/TSDoc
{@link Target}references inside component descriptionspreviously surfaced in the generated docs as literal text — readers saw
{@link Rule}verbatim with nothing clickable. This teaches the markdownpipeline to rewrite those references into proper links, the same way IDEs
interpret them for cmd-click navigation.
Three atomic commits:
fix(markdown): skip type-linking inside anchor ancestors— prerequisitebug fix: a
<code>inside an existing<a>would otherwise get wrapped in asecond
<a>(invalid nested anchors).feat(markdown): render inline {@link Target} references as links— aremark plugin that rewrites
{@link …}(the three TSDoc forms + absoluteURLs) into link nodes, resolving targets against known types and component
tags. Unresolved targets render as inline code; references inside inline/
fenced code are left untouched.
docs(markdown): add example demonstrating inline @link references— akompendium-example-inline-linksshowcase plus an integration test pinningthe rendered HTML against the built
kompendium.jsonregistry.Verification (on the
limemerge result)npm run lint:src,npm run build, andnpm test(119 passing / 6 skipped)all green against
lime+ this branch.Authored by @Kiarokh — see the upstream PR for full description and the
behaviour matrix.