fix(repl-sdk): correct the whitespace escape in formatDefaultId - #2215
Merged
NullVoxPopuli merged 2 commits intoAug 15, 2026
Merged
NullVoxPopuli merged 2 commits into
NullVoxPopuli merged 2 commits into
Conversation
`/\\s+/g` matches a literal backslash followed by one or more `s`, not whitespace. The intent reads as collapsing runs of whitespace before kebab-casing the heading text. In practice this is a no-op: `kebabCase` already splits on whitespace, so every realistic heading slugs identically either way. The one input that differs is a heading containing a literal `\s`: "a \s b" before: a-b after: a-s-b Happy to close this in favour of dropping the `replaceAll` entirely, since `kebabCase` makes it redundant — I went with the minimal change.
|
|
Two cases for the escape fix: - runs of whitespace in a heading collapse to a single `-` in the id (passes either way, since kebabCase also splits on whitespace -- pins the intent so a future refactor of formatDefaultId cannot quietly change it) - a literal `\s` in a heading is text, not whitespace The second is the actual regression test. With the old `/\\s+/g`, `## a \s b` produced `id="a-b"` -- the `s` was consumed as part of the match.
NullVoxPopuli
approved these changes
Aug 14, 2026
NullVoxPopuli
marked this pull request as ready for review
August 14, 2026 23:59
Contributor
Footnotes
|
Merged
gitKrystan
added a commit
to gitKrystan/limber
that referenced
this pull request
Aug 15, 2026
@gitKrystan asked whether the whitespace normalization was needed at all with github-slugger. It wasn't -- and it was making us diverge from GitHub. Checked against rehype-slug, which is the github-slugger reference: ## Hello World -> id="hello----world" (does NOT collapse) ## Hello *there* -> id="hello-there" (no separator inserted) ## `setupMirage` and more -> id="setupmirage-and-more" So the actual defect was `extractText` joining children with a space, adding one the markdown never had: `## Hello *there*` extracted as `'Hello there'` and slugged as `hello--there`. Joining with '' fixes that at the source. Collapsing author-written runs on top of it was wrong -- GitHub keeps those. Drops normalizeText and the test asserting collapsing (which came from NullVoxPopuli#2215, back when kebabCase was the target). Adds two tests pinning parity with the rehype-slug results above.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft — tiny one, happy to close if you'd rather handle it differently.
formatDefaultIdinpackages/repl-sdk/src/compilers/markdown/heading-id.jsreads:/\\s+/gmatches a literal backslash followed by one or mores, not whitespace. The intent looks like collapsing runs of whitespace before kebab-casing.Mostly a no-op.
kebabCasealready splits on whitespace, so realistic headings slug identically either way. The observable difference is a heading containing a literal\s, where thesgot eaten:Tests
Two cases in
parse.test.ts:-— passes either way, pinning the intent so a future refactor offormatDefaultIdcan't quietly change it\sis text, not whitespace — the actual regression testI verified the second genuinely fails without the fix (
expected '<h2 id="a-b">a \s b</h2>' to contain 'id="a-s-b"') rather than just passing alongside it.Full
repl-sdksuite is unchanged otherwise: 1 test file (compilers.ember.onUnhandled.test.ts) fails to load andlint:typesreports 3 errors, both identical on cleanmainin my checkout.No README change — this doesn't alter documented behavior.
Alternative
Two reasonable outcomes and I don't have a strong preference:
replaceAllentirely, sincekebabCasemakes it redundant. (Note this would change the\scase again, toa-s-bas well — same result, one fewer moving part.)Note
Related but independent: #2216 adds a configurable heading slugger, which needs whitespace actually collapsed — a space-to-dash slugger emits
hello--therewithout it. It normalizes at a different call site, so the two don't conflict and either can merge first.🤖 Drafted by Claude (Opus 5) for @gitKrystan, while tracking down why heading anchors differed between our docs site and GitHub (soxhub/auditboard-frontend#42085).