Skip to content

fix: versatile weapon quick-roll picks d8 button correctly (#1388)#1390

Open
0xguy07 wants to merge 1 commit into
kakaroto:masterfrom
0xguy07:fix/versatile-quick-roll-1388
Open

fix: versatile weapon quick-roll picks d8 button correctly (#1388)#1390
0xguy07 wants to merge 1 commit into
kakaroto:masterfrom
0xguy07:fix/versatile-quick-roll-1388

Conversation

@0xguy07

@0xguy07 0xguy07 commented Jun 5, 2026

Copy link
Copy Markdown

Summary

Replace a fragile action.previousElementSibling !== null check in the quick-roll wire-up with a scoped lookup inside .ddbc-combat-attack__damage. The previous check incorrectly set force_versatile = true for the d8 button on a versatile weapon whenever DDB rendered any label/icon node ahead of the first dice container, so clicking the d8 rolled 1d10 instead of 1d8 (issue #1388).

Type of change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • ♻️ Refactor (no functional change)
  • ⚡ Performance improvement
  • 🧪 Tests (adds or updates tests)
  • 📝 Documentation
  • 🔧 Build/CI
  • 🚨 Breaking change

⚠️ AI usage disclosure (required)

  • I did not use AI for this PR.
  • I did use AI for this PR (describe below).

What was AI-assisted: Root-cause analysis from the issue's console trace and the closest() / querySelectorAll() rewrite were drafted with Claude (Anthropic, Opus 4.7). I (the PR author) reviewed the patch and the surrounding activateQuickRolls / rollItem flow before pushing. No test file is included because this code only runs against the live DDB DOM.

Motivation & context

  • Related issue: versatile weapon always rolls higher dice #1388
  • Reporter's console trace shows Executing panel : b20-item-pane false true trueforce_versatile = true was passed even though the user clicked the d8. That flag flows into rollItem where force_versatile = true forces versatile_choice = "two" (longsword 1d10), producing the reported Damage to crits : 1d10+7.

What changed?

  • src/dndbeyond/content-scripts/character.js (activateQuickRolls): when wiring up actions_damage, find each action's nearest .ddbc-combat-attack__damage ancestor, count the .integrated-dice__container children, and set isVersatile = true only when this action is the last of more-than-one container. Behavior matches the original intent ("2-handed/versatile is the second dice container") but is no longer affected by any label/icon/wrapper nodes DDB renders alongside the first container.

How to test

  1. git fetch && git checkout fix/versatile-quick-roll-1388
  2. npm run build:chrome (or :firefox) and load the unpacked build.
  3. On a character with a versatile weapon (Longsword, Quarterstaff, Spear, Warhammer, etc.):
    • Hover the d8 damage die in the action bar → confirm the rolled damage is 1d8 + mods, not 1d10.
    • Hover the d10 damage die → confirm the rolled damage is 1d10 + mods (2-handed).
  4. On a non-versatile weapon (Shortsword, Greatsword, Dagger…): confirm the single damage button still rolls correctly.
  5. With quick-rolls disabled in settings: confirm dice still roll as before (this path is gated on settings["quick-rolls"] and is unchanged).

Reviewer notes

  • DOM hypothesis caveat: I do not have access to a live DDB session for verification. The fix preserves the original branch behavior (length > 1 && last container is equivalent to previousElementSibling !== null whenever the dice containers are the only children) so it should be a no-op on DOM shapes where the prior code worked, and only changes behavior on shapes that previously broke. If you have a quick reproducer or DOM snippet handy, it would be useful to confirm.
  • Scope intentionally minimal: only the four lines wiring actions_damage are touched. rollItem / force_versatile semantics are unchanged.

…1388)

Replace the `action.previousElementSibling !== null` test with a scoped
lookup of `.integrated-dice__container` elements within the same
`.ddbc-combat-attack__damage` area. Only set `force_versatile = true`
when this action is the last of more-than-one container — i.e. the
2-handed/versatile variant — regardless of any sibling label or icon
nodes DDB now renders ahead of the first container.

Before, on a longsword the d8 button got `force_versatile=true` whenever
DDB rendered any node ahead of it in the damage area; that flag flows
into rollItem and forces `versatile_choice="two"`, rolling 1d10 instead
of 1d8. Console trace from the report: `Executing panel : b20-item-pane
false true true` (force_versatile=true) → `Damage to crits : 1d10+7`.

AI assistance: root-cause analysis and the closest()/querySelectorAll()
rewrite drafted with Claude (Opus 4.7). DOM shape is inferred from the
reporter's console output, not verified against a live DDB session.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant