fix: dice regex matches bare D1/D0 in sourcebook text (#1126)#1389
Open
0xguy07 wants to merge 1 commit into
Open
fix: dice regex matches bare D1/D0 in sourcebook text (#1126)#13890xguy07 wants to merge 1 commit into
0xguy07 wants to merge 1 commit into
Conversation
Restrict the bare-die form `dN`/`DN` to N=2-9 or N with two-or-more digits. This excludes `D0`/`D1`, which are not real dice and produced false positives on sourcebook map labels such as the "doors: A2, B1, C1, D1" sentence reported in issue kakaroto#1126 (Vecna: Eve of Ruin). Explicit-count forms (`1d1`, `2d0`, ...) are unchanged so existing behavior is preserved. Adds test/dice_regex.test.js with 25 cases covering the bug, regressions for standard dice (1d20, 2d6+3, d100, d2, d20ro<3, d6min2, +5, mixed), and embedded-word non-matches. Run with `npm test`. AI assistance: regex change and tests drafted with Claude (Opus 4.7); all behavior verified by the bundled test script against current master. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author
|
@dmportella I ensured I used the correct template this time. I just want to assist in fixing some of the issues which are on the list. We use this extension a lot during our games, and I want to ensure I can help in the ways that I can regarding ensuring that I contribute. |
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
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.
Summary
Restrict the bare-die
dN/DNform inDICE_REGEXPto N=2–9 or N with two-or-more digits, so impossible bare dice likeD0/D1no longer hijack sourcebook map labels (e.g.A2, B1, C1, D1in Vecna: Eve of Ruin). Explicit-count forms (1d1,2d0, …) are unchanged. A standalonenpm testscript is added covering the bug and existing-behavior regressions.Type of change
What was AI-assisted: The regex change in
src/common/utils.jsand the test cases intest/dice_regex.test.jswere drafted with Claude (Anthropic, Opus 4.7). I (the PR author) reviewed every change, ran the bundled test script, and confirmed the behavior on the issue's reproducer sentence and on standard dice expressions. No build/CI tooling, documentation, or unrelated code was modified by AI.This is a resubmission of #1375 — the prior PR was closed after @dmportella asked about AI use; this one discloses it upfront and adds the test coverage that the prior thread noted regex changes need.
Motivation & context
DICE_REGEXPaccepts\d*[dD]\d+, which matches bareD0andD1. Neither is a real die (a 0-sided die is undefined and a 1-sided die has no roll variance), but both occur as map/door labels in published sourcebooks, producing acustom20.pngicon where the original label should be (issue (Minor) Replacement of D1 false positive #1126 screenshot).What changed?
src/common/utils.js: tightened the bare-die alternative inDICE_REGEXPfrom\d*[dD]\d+to\d+[dD]\d+|[dD](?:[2-9]|\d{2,}). Applied in both the primary dice group and the trailing-extra-dice modifier group so compound expressions like1d8+1d6+3still parse. Comment added inline explaining the rationale.test/dice_regex.test.js(new): 25-case standalone Node script. LoadsDICE_REGEXPdirectly fromsrc/common/utils.jsat runtime (no bundler needed) and exits non-zero on any failure.package.json: added"test": "node test/dice_regex.test.js".How to test
git fetch && git checkout fix/d1-d0-false-positive-1126npm test— should print25/25 passed.npm run build:chrome/:firefox), open the Vecna ritual chamber page, scroll to the doors sentence —D1should render as text, not a Beyond20 icon. Sanity check a known-good page (e.g. any monster stat block with1d20/2d6+3) and confirm dice buttons still appear.Reviewer notes
D0andD1are excluded. The original issue thread notedD2also collides with map labels, butd2is a legitimate (if uncommon) die, so excluding it would regress real dice notation. This PR fixes the impossible-die cases and leaves theD2+ambiguity as a known trade-off (worth a follow-up if a context-aware approach is wanted).