fix(core): detect showLineNumbers anywhere in the meta string - #275
Conversation
`showLineNumbers` was detected with a reversed-string regex:
/srebmuNeniLwohs(?!(.*)(\/))/.test(reverseString(meta))
The negative lookahead was meant to ignore `/showLineNumbers/` used as a
highlight word, but it actually rejected `showLineNumbers` whenever any
`/` appeared before it in the meta. Since highlight tokens such as
`/age/#v` contain slashes, `showLineNumbers` placed after them was
silently ignored — so it only worked at the start of the meta string.
Replace the reverse-string hack with a token-boundary match that is
position-independent and captures the optional start-at number directly:
meta.match(/(?:^|\s)showLineNumbers(?:\{(\d+)\})?(?=\s|$)/)
Bounding on whitespace/string-ends still excludes `/showLineNumbers/`
(slash-delimited), so the highlight-word cases keep working. The
`reverseString` import is now unused in this module and removed.
Added a fixture covering `showLineNumbers` (and `showLineNumbers{N}`)
after highlight tokens; all existing snapshots are unchanged.
Closes rehype-pretty#204
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
commit: |
PR reviewThe detection fixes option ordering but still mistakes text inside a valid highlight annotation for a standalone option. The replacement remains compatible with older Safari because it does not use lookbehind. Bugs (1)1. 🟠 Exclude showLineNumbers inside delimited annotationsLocation: const showLineNumbersMatch = meta.match(
/(?:^|\s)showLineNumbers(?:\{(\d+)\})?(?=\s|$)/,
);Whitespace boundaries do not prove that the match is outside a slash- or quote-delimited annotation. Multi-word character highlights may legitimately contain whitespace. Failure scenario: With metadata Fix: Tokenize the metadata or reject matches whose indices fall inside the existing parsed annotation spans. Keep the solution lookbehind-free for pre-Safari 16.4 compatibility, and add this multi-word case to the fixture. Docs (1)1. 🟠 Add a patch changesetLocation: // Detect `showLineNumbers` ... as a standalone meta tokenThis fixes public metadata behavior but currently produces no package version bump. Failure scenario: The fix merges without being included in the next published version. Fix: Add a changeset declaring VerdictApprove after nits - the remaining false positive is narrow but should be corrected before merging when practical. 🤖 Review generated with Codex |
Whitespace boundaries alone do not prove the token is an option: a character highlight may legitimately span whitespace, so `/foo showLineNumbers bar/` highlighted the text correctly but also gave the `<code>` a spurious `data-line-numbers`. Move the detection into `getShowLineNumbers`, which skips matches overlapping the spans already claimed by the parsed `charsMatches` annotations. Only lookahead is used, so this stays parseable on Safari < 16.4. Fixture gains the multi-word case both on its own and alongside a real `showLineNumbers` option; the latter keeps line numbers and highlights both occurrences. Also adds the patch changeset.
PR reviewThe detection rewrite is correct and the annotation-exclusion follow-up closes the gap raised in the previous round; nothing here is merge-blocking. Two narrower metadata forms that used to enable line numbers silently stop doing so, and that tightening is not mentioned in the changeset. Docs (1)1. 🟠 Two previously-accepted metadata forms silently stop enabling line numbersLocation: const showLineNumbersRegex = /(?:^|\s)(showLineNumbers(?:\{(\d+)\})?)(?=\s|$)/g;The old reversed-string regex matched Rendering the same fixture on
The last two rows are the tightening. Both were almost certainly unintended on Failure scenario: A user upgrades a patch version, line numbers disappear from a code block, and the changeset only mentions detecting Fix: Add a sentence to Tests (1)1. 🟡 The exclusion fixture only covers slash-delimited annotationsLocation: ```js /foo showLineNumbers bar/
const value = 'foo showLineNumbers bar';
```
Failure scenario: A future change to the Fix: Add two blocks to the fixture: one using VerdictApprove after nits - the regex and span arithmetic are correct and lookbehind-free; only the undocumented tightening and two fixture gaps remain. 🤖 Review generated with Claude Code |
|
Addressed the review. Detection moved into The fixture gains the multi-word case, plus Patch changeset added. |
Summary
Closes #204.
showLineNumbersonly worked when it was the first token in the metastring. Placed after any highlight token (e.g.
/age/#v), it wassilently ignored:
Root cause
Detection used a reversed-string regex (
packages/core/src/index.ts):srebmuNeniLwohsisshowLineNumbersreversed. The negative lookaheadwas intended to ignore
/showLineNumbers/when used as a highlightword, but it actually fails whenever any
/precedesshowLineNumbersin the meta. Highlight tokens like/age/#vcontainslashes, so
showLineNumbersafter them never matched — the behavior wasposition-dependent by accident.
Fix
Match
showLineNumbers(optionallyshowLineNumbers{N}) as a standalone,whitespace-bounded token, wherever it appears, and read the start-at
number directly instead of reversing substrings:
Bounding on whitespace / string-ends still excludes the slash-delimited
/showLineNumbers/highlight word, so those cases keep working. ThereverseStringimport is now unused in this module and was removed.Behavior
showLineNumbers /const//const/ showLineNumbers/age/#v showLineNumbers{5}counter-set: line 4)showLineNumbers/showLineNumbers/(highlight word)/anything?showLineNumbers/Tests
showLineNumbersAfterHighlightfixture coveringshowLineNumbersand
showLineNumbers{N}after highlight tokens.showLineNumbersfixture that asserts/showLineNumbers/and/anything?showLineNumbers/do not enable line numbers — confirmingno regression.
typecheckandbiome checkpass; full suite green.