sheet/feat/ageofficial - #112
Merged
Merged
Conversation
Implements the legacy sheet migration adapter (src/utility/legacyAdapter.ts), converting a flat Roll20 attributes blob (scalars + repeating_* rows) into the Beacon SDK stores. Feature: - Import modal with per-section checkboxes (Abilities, Character Stats, Biography, Game Settings, Talents & Powers, Ability Focuses, Spells/Arcana, Equipment & Weapons, Currency, Name) and Append/Overwrite modes. Overwrite clears only the selected sections; a guard prevents a non-legacy/empty blob from blanking the sheet. - Slide-out Import tab with a game-system-specific masked icon, gated behind a new "Allow Import" setting (default off) in the Game System row. - Robust parsing: mixed money formats, split/ranged attacks -> inventory weapons, base-speed, xp from the structured section, casters auto-show Arcana. healthMax/magicMax are left at default (missing from the Roll20 export). Also: - AbilitiesView: derive the display from a computed instead of a one-time snapshot ref so external store changes (imports) reflect immediately. - vitest.config: add the @ alias and exclude nested .worktrees so the unit suite can run. - Add legacyAdapter.spec.ts covering the flat import, section selection, overwrite clearing, money/range parsing, and the empty-blob guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add legacy Roll20 character import to the AGE sheet
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds Roll20 legacy character import support with selectable append or overwrite behavior, persistent import settings, and a modal interface. It also makes ability-score rendering reactive and adds Vitest coverage and configuration updates. ChangesLegacy character import
Reactive ability scores
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AppVue
participant importLegacyCharacter
participant PiniaStores
AppVue->>importLegacyCharacter: selected sections and import mode
importLegacyCharacter->>PiniaStores: clear selected overwrite targets
importLegacyCharacter->>PiniaStores: write mapped legacy data
importLegacyCharacter-->>AppVue: completion and change report
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
ageofficial/src/utility/legacyAdapter.spec.ts (2)
110-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an assertion for the imported specialization quality level.
The fixture sets
"specialization1-degree": "Novice"at Line 25, andimportLegacySpecializationlowercases the degree before comparing it. Line 147 checks only the item name. AssertqualityLevelas well so the case-normalization path stays covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ageofficial/src/utility/legacyAdapter.spec.ts` around lines 110 - 167, Update the specialization assertion in the “imports Bjordson (warrior)” test to locate the imported “Berserker” quality and also assert its qualityLevel is the normalized lowercase value “novice”, preserving the existing name assertion.
100-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the console spies after each test.
vi.spyOnreplaces the globalconsolemethods and this suite never restores them. The replacements persist for the rest of the worker process, so an unrelated spec file can lose console output or inherit call counts. Add anafterEachthat callsvi.restoreAllMocks(), or setrestoreMocks: trueinageofficial/vitest.config.ts.♻️ Proposed fix
-import { describe, it, expect, beforeEach, vi } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";vi.spyOn(console, "info").mockImplementation(() => {}); }); + + afterEach(() => { + vi.restoreAllMocks(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ageofficial/src/utility/legacyAdapter.spec.ts` around lines 100 - 108, Add cleanup for the console spies created in the legacy adapter suite’s beforeEach by adding an afterEach that calls vi.restoreAllMocks(). Keep the existing setup unchanged and ensure each test restores the global console methods and mock state.ageofficial/src/utility/legacyAdapter.ts (3)
295-300: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTalent degree parsing accepts fewer formats than the specialization parser.
Lines 299-300 compare
talent.talentdegreeonly to the strings"2"and"3".importLegacySpecializationat Lines 229-234 also accepts the wordsexpertandmaster, and normalizes case. Legacy talent rows use the same mixed conventions, so a talent stored as"Expert"or numeric2imports asnovice.Extract a shared degree-to-quality-level helper and use it in both places.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ageofficial/src/utility/legacyAdapter.ts` around lines 295 - 300, Extract a shared degree-to-quality-level helper that normalizes case and maps both numeric and textual values for novice, expert, and master. Replace the local parsing in the talents loop and the corresponding logic in importLegacySpecialization with this helper, preserving novice as the fallback for unrecognized values.
698-714: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the
sectionsparameter asImportSectionKey[].Line 701 declares
sections?: string[]and Line 714 casts toImportSectionKey[]. The cast hides typos at every call site, andapplySectionsilently ignores unknown keys. Declare the parameter asImportSectionKey[]so the compiler validates the caller inApp.vue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ageofficial/src/utility/legacyAdapter.ts` around lines 698 - 714, Update the importLegacyCharacter function’s sections parameter from string[] to ImportSectionKey[], and remove the downstream cast when deriving selected. Preserve the existing default-section behavior while allowing TypeScript to validate section keys at all call sites.
716-726: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTwo full store serializations run on every import for the debug report only.
Lines 717 and 723 each call
sheet.dehydrateStore(), and Line 724 deep-diffs the result. The only consumer is the console report. Also,console.warnat Line 707 andconsole.infoat Line 725 are ungated. Skip the snapshot, the diff, and the logs whenlogModeis off.As per coding guidelines: "Production
console.logcalls must be gated behind alogModeflag or removed before merging; thedevRelaystub is an allowed exception."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ageofficial/src/utility/legacyAdapter.ts` around lines 716 - 726, Gate the import debug-report path on logMode: only call sheet.dehydrateStore before and after applying sections, diffStates, and logImportReport when logging is enabled. Also guard the nearby console.warn and completion console.info calls with the same logMode condition, while leaving the import behavior unchanged when logging is disabled.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ageofficial/src/App.vue`:
- Around line 221-271: Enhance the import modal anchored by importModalOpen with
dialog semantics: add role="dialog", aria-modal="true", and a stable
aria-labelledby reference connecting the modal to its h3 heading. Implement
open-time focus on the modal, trap Tab/Shift+Tab within it, close on Escape, and
restore focus to the triggering element after cancelImport or confirmImport
closes the dialog.
In `@ageofficial/src/utility/legacyAdapter.ts`:
- Around line 602-621: Update the bio branch of clearSection so it does not
clear bio.gender, since importLegacyBioFlat writes legacy gender to bio.sex
only. Remove the bio.gender reset while preserving all other bio-field resets
and quality cleanup.
- Around line 502-533: Gate all diagnostic output in legacyAdapter.ts behind the
existing logMode flag by adding one shared guarded logging helper and routing
every console call through it. In
ageofficial/src/utility/legacyAdapter.ts:71-108, update loadLegacyAbilityScores,
loadLegacyCharacterDetails, and loadLegacyGroupings to gate or remove their
inspectors, including the raw attribute dump; in :502-533, guard logImportReport
and its group, warnings, tables, and logs; in :716-726, gate console.warn and
console.info and only run sheet.dehydrateStore() and diffStates when logMode is
enabled, since they only support the report.
- Around line 623-625: Update the overwrite handling for the "settings" case in
clearSection and the importLegacySettingsFlat flow so showArcana is explicitly
reset to false before applying imported settings, allowing non-caster legacy
characters to hide the Arcana section.
- Around line 278-289: Update the money-row handling in legacyAdapter’s money
iteration to accumulate amounts per denomination rather than overwrite
inventory.cash[key], preserving existing cash in append mode. In overwrite mode,
ensure clearSection("currency") resets all denominations before processing,
matching the existing reset behavior referenced near the currency-clearing
logic.
- Around line 180-186: Update the speed assignment in the legacy adapter to
write char.speed only when base-speed is present, preserving existing values
during append imports instead of defaulting to 10. Also convert the optional
attributes.character?.character?.xp value to the expected numeric type before
assigning char.xp, while leaving it unchanged when absent.
- Around line 383-398: Update the weapon classification in the attacks mapping
around parseRange so ranged is derived from the parsed shortRange value rather
than raw atk.range truthiness. Preserve the existing parseRange results and set
weaponType to Ranged only when shortRange indicates a valid range; otherwise
classify the weapon as Melee.
In `@ageofficial/src/views/SettingsView.vue`:
- Around line 88-94: Update the Allow Import checkbox markup in SettingsView by
adding a unique id to the input bound to settings.allowImport and assigning the
visible “Allow Import” text to a label with a matching for attribute. Preserve
the existing toggle styling and binding.
---
Nitpick comments:
In `@ageofficial/src/utility/legacyAdapter.spec.ts`:
- Around line 110-167: Update the specialization assertion in the “imports
Bjordson (warrior)” test to locate the imported “Berserker” quality and also
assert its qualityLevel is the normalized lowercase value “novice”, preserving
the existing name assertion.
- Around line 100-108: Add cleanup for the console spies created in the legacy
adapter suite’s beforeEach by adding an afterEach that calls
vi.restoreAllMocks(). Keep the existing setup unchanged and ensure each test
restores the global console methods and mock state.
In `@ageofficial/src/utility/legacyAdapter.ts`:
- Around line 295-300: Extract a shared degree-to-quality-level helper that
normalizes case and maps both numeric and textual values for novice, expert, and
master. Replace the local parsing in the talents loop and the corresponding
logic in importLegacySpecialization with this helper, preserving novice as the
fallback for unrecognized values.
- Around line 698-714: Update the importLegacyCharacter function’s sections
parameter from string[] to ImportSectionKey[], and remove the downstream cast
when deriving selected. Preserve the existing default-section behavior while
allowing TypeScript to validate section keys at all call sites.
- Around line 716-726: Gate the import debug-report path on logMode: only call
sheet.dehydrateStore before and after applying sections, diffStates, and
logImportReport when logging is enabled. Also guard the nearby console.warn and
completion console.info calls with the same logMode condition, while leaving the
import behavior unchanged when logging is disabled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d911d0b-571e-479c-afc7-0e5009d9900b
📒 Files selected for processing (7)
ageofficial/src/App.vueageofficial/src/components/abilities/AbilitiesView.vueageofficial/src/sheet/stores/settings/settingsStore.tsageofficial/src/utility/legacyAdapter.spec.tsageofficial/src/utility/legacyAdapter.tsageofficial/src/views/SettingsView.vueageofficial/vitest.config.ts
BronsonHall
previously approved these changes
Aug 11, 2026
Contributor
|
Hey @donbwhite Would you mind going through and either addressing or resolving these coderabbit comments? |
BronsonHall
added
the
blocked
Legacy import (src/utility/legacyAdapter.ts): - Write char.speed only when base-speed is present; keep existing value on append instead of clobbering it with the default 10. - Convert legacy xp through toInt so char.xp is always numeric. - Accumulate money rows per denomination instead of overwriting, so repeated rows and append mode add up. - Classify weapons Ranged/Melee from the parsed shortRange, not raw range truthiness (placeholder "0"/"-" no longer read as ranged). - Gate all diagnostic console output behind a logMode flag via shared helpers, and skip the before/after dehydrate + diff work when logging is off. - Don't clear bio.gender in overwrite mode; the import maps legacy gender to bio.sex only, so clearing gender would delete a user value nothing rewrites. - Reset settings.showArcana to false in overwrite mode so a non-caster legacy character hides the Arcana section. Import modal (src/App.vue): - Add role="dialog", aria-modal, aria-labelledby, initial focus, a Tab focus trap, Escape-to-close, and focus restoration to the triggering element. Settings (src/views/SettingsView.vue): - Associate the "Allow Import" text with its checkbox via id/for. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address CodeRabbit review on PR Roll20#112
Contributor
Author
|
@BronsonHall Updated the PR |
BronsonHall
approved these changes
Aug 18, 2026
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.
Submission Checklist
Changes / Description (optional)
Add legacy Roll20 character import to the AGE sheet
Summary
Adds a legacy character importer to the AGE (Adventure Game Engine) sheet.
It reads the flat Roll20 attribute blob from an old-format character
(
repeating_*rows + top-level scalars) and maps it into the Beacon SDKPinia stores, so players can migrate existing characters onto the new sheet.
Implemented in
src/utility/legacyAdapter.ts, driven by a new Import modal inApp.vue, and gated behind a new Allow Import setting.What it does
Import UI
mostly off-screen and slides out on hover/keyboard-focus so it doesn't
interfere with play. The icon is a CSS mask/background image that themes per
game system (Fantasy AGE / Blue Rose vs. Modern AGE / Expanse), with a
screen-reader-only label and an "Import" tooltip.
row), default off — opt-in per character.
per-section checkboxes: Name, Abilities, Character Stats, Biography,
Game Settings, Talents & Powers, Ability Focuses, Spells (Arcana),
Equipment & Weapons, Currency.
(clear the selected sections first, then import). Overwrite only clears the
sections the user checked.
Mapping (
legacyAdapter.ts)groups, xp, level), biography (ancestry, class, background, physical,
history, goals), and game settings (game system, caster → show Arcana).
specializations, and ancestry/class → quality items.
(a legacy attack implies possession of the weapon, so it becomes an
inventory weapon).
(
"18g","GP"+"6", reversed"10"+"G"), split/ranged attack ranges(
"4 / 6","16 yards"), and hyphenated repeating-section names.Safety
data, so an empty/not-yet-loaded character can never blank the sheet.
existing
updateIdcheck prevents the re-hydrate loop from clobbering animport.
Other changes
AbilitiesView.vue: the ability display was built from a one-timesnapshot
ref(only refreshed when the abilities modal closed), so externalstore changes never showed. Changed to a
computed— a latent reactivitybug that the importer surfaced.
vitest.config.ts: added the@alias (needed to unit-test modules thatuse it) and excluded nested
.worktreesso the unit suite runs cleanly.settingsStore.ts: new persistedallowImportflag (defaultfalse).Testing
src/utility/legacyAdapter.spec.ts— 8 tests covering the flat import(warrior + mage sample characters), section selection, overwrite clearing,
messy money/range parsing, and the empty-blob guard. All pass.
npm run type-checkis clean for the changed files.Known limitations / follow-ups
characters. Modern AGE / Expanse / Blue Rose legacy field names are untested
and may import partially.
the player enters the real max after import.
rows) imports verbatim.
languages, appearance, and computed aggregates (armor penalty/shield bonus)are dropped.
settingsStore.test.tsasserts an outdateddehydrate()shapeand remains red (unrelated to this feature).
🤖 Generated with Claude Code
Summary by CodeRabbit