refactor(data): replace setVersion with loadData and establish fetch boundary#153
Merged
refactor(data): replace setVersion with loadData and establish fetch boundary#153
Conversation
…n, and locale warnings
… and improve orchestration
Move DEFAULT_LOCALE into shared constants and update app modules to import it from one place Adjust fetch-icons, gen-sitemap, and bench-node to use the current CBNData constructor
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/ADR-007_data_loading_orchestration.md`:
- Line 85: The ADR's claim that the public data.loadData() API does not change
is incorrect; update the document to state that data.setVersion(...) was
replaced by a new signature data.loadData(version, locale, activeMods), and
describe the new parameters and caller responsibilities (version, locale,
activeMods) and any behavioral differences from the old setVersion flow;
reference the symbols data.setVersion and data.loadData in the correction so
readers can map the change.
In `@src/data-loader.ts`:
- Around line 163-170: The returned dataJson is being asserted to
LoadedRawDataset["dataJson"] without runtime validation, which can mask missing
fields (e.g., raw.dataJson.build_number) and cause downstream failures; update
the fetch completion code (use dataResult/dataJson in src/data-loader.ts) to
perform explicit runtime schema validation (for example using zod/AJV or a
custom validator) on dataResult.value and only then assign/return it as
LoadedRawDataset["dataJson"]; alternatively, if you prefer not to add a schema
library, add explicit existence/type guards for required fields (at least data
and build_number) before the type assertion and document that callers rely on
validated data so accesses like raw.dataJson.build_number are safe.
In `@src/data.mods.test.ts`:
- Line 38: The test uses await expect(data.loadData(...)).resolves.not.toThrow()
which misapplies a function-execution matcher to an already-unwrapped promise
result; replace each occurrence of .resolves.not.toThrow() for loadData(...)
with an explicit assertion such as await expect(data.loadData("latest", "en",
[])).resolves.toBe(true) (or simply await data.loadData("latest", "en", []) if
you only want to ensure it doesn't reject) and update the other three instances
(the same pattern at the other failing assertions) so they assert the resolved
boolean value rather than using not.toThrow().
In `@src/data.ts`:
- Around line 2449-2500: Wrap the existing try/finally block with a
try/catch/finally (or insert a catch between the existing try and finally) that
captures any thrown errors from loadRawDataset, parseModsJson, the CBNData
constructor, etc., by calling Sentry.captureException(err) before re-throwing
the error; keep the existing finally behavior (the generationToken check and
loadProgressStore.set(null)) unchanged and re-throw the caught error so original
fail-fast behavior is preserved.
- Around line 303-312: The catch block in the CBNData constructor that handles
failures from applyLocaleJson currently logs to console and falls back to
resetI18n("en") but does not report the error to telemetry; update the catch to
call Sentry.captureException(e) (or the project's Sentry wrapper) in addition to
the console.warn and fallback so the failure is recorded, keeping the existing
resetI18n("en") and this.locale = "en" behavior; locate the code around the
CBNData constructor where applyLocaleJson(...) is invoked and add the
Sentry.captureException call inside that catch clause.
In `@src/i18n/game-locale.ts`:
- Around line 85-88: The function signature for applyLocaleJson has a redundant
union type for pinyinJSON; change the parameter type from "unknown | null" to
just "unknown" in the applyLocaleJson declaration so pinyinJSON uses a single
unknown type (update any matching overloads or callers if necessary), keeping
the function name applyLocaleJson as the locator for the change.
In `@src/ModSelector.svelte`:
- Around line 50-55: The current assignment to mods uses an unsafe cast `as
ModInfo[]` which can hide null/undefined if a rawModsJson entry has a missing or
malformed info; remove the cast and instead add a runtime type guard that
filters out null/undefined and narrows the type to ModInfo (e.g. change the
pipeline that reads Object.values(rawModsJson).map(({ info }) => info) so you
call .filter((info): info is ModInfo => info != null && typeof info ===
'object') and then apply the existing `.filter((modInfo) => !modInfo.core)` or
combine both checks; ensure the variable `mods` (and any use of `$derived`) is
typed from that filtered result rather than using `as ModInfo[]`.
In `@src/tile-data.test.ts`:
- Around line 56-57: The test fixture `fakeData` still uses old snake_case keys
(e.g., active_mods, raw_mods_json) causing invalid CBNData defaults after you
renamed fields; update the fixture and all other occurrences (e.g., the
instances around the other reported spots) to use the new camelCase keys
`activeMods` and `rawModsJSON` so the baseline data matches the renamed model
and tests exercise real behavior. Locate and replace the legacy keys inside the
`fakeData` object and any other test fixtures or inline objects in this file
(and the other listed occurrences) to ensure the test data shape matches the
`CBNData`/model fields. Ensure no other snake_case variants remain in this
file's fixtures so overrides using `activeMods`/`rawModsJSON` are trusted.
In `@tsconfig.node.json`:
- Line 4: Remove the redundant "noEmit": true setting from tsconfig.node.json
since it is already inherited from the base tsconfig; edit tsconfig.node.json to
delete the "noEmit" option so the file relies on the base configuration and
avoids duplicate compiler option declarations.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3aeadf1c-647f-45b7-8e46-ac8bc5d27113
📒 Files selected for processing (102)
.github/workflows/ci.ymlAGENTS.mdDEVELOPMENT.mddocs/adr/ADR-007_data_loading_orchestration.mddocs/adr/README.mddocs/architecture.mdscripts/bench-node.tsscripts/fetch-icons.tsscripts/gen-sitemap.tssrc/App.sveltesrc/Catalog.sveltesrc/ModSelector.sveltesrc/ModSelector.test.tssrc/PageMeta.sveltesrc/PageMeta.test.tssrc/RenderErrorFallback.sveltesrc/Thing.sveltesrc/constants.tssrc/data-loader.test.tssrc/data-loader.tssrc/data.flatten.test.tssrc/data.mods.test.tssrc/data.test-helpers.tssrc/data.test.tssrc/data.tssrc/i18n/game-locale.tssrc/i18n/transifex-static.tssrc/i18n/ui-locale.tssrc/mod-provenance.test.tssrc/mods.test.tssrc/monster-policy.test.tssrc/navigation.svelte.tssrc/routing.svelte.tssrc/routing.test.tssrc/schema.test.tssrc/search-engine.tssrc/search-state.test.tssrc/search.test.tssrc/testRender.tssrc/testRenderMods.tssrc/tile-data.test.tssrc/tile-data.tssrc/types/Achievement.sveltesrc/types/AmmunitionType.sveltesrc/types/Bionic.sveltesrc/types/BonusContainer.sveltesrc/types/Construction.sveltesrc/types/Construction.test.tssrc/types/ConstructionGroup.sveltesrc/types/Fault.sveltesrc/types/Flag.sveltesrc/types/Furniture.sveltesrc/types/Furniture.test.tssrc/types/Item.sveltesrc/types/ItemAction.sveltesrc/types/ItemLink.sveltesrc/types/MartialArt.sveltesrc/types/MartialArtRequirements.sveltesrc/types/Material.sveltesrc/types/Monster.sveltesrc/types/Mutation.sveltesrc/types/MutationCategory.sveltesrc/types/MutationList.sveltesrc/types/MutationType.sveltesrc/types/OvermapSpecial.sveltesrc/types/Recipe.sveltesrc/types/Skill.sveltesrc/types/Technique.sveltesrc/types/Terrain.sveltesrc/types/Terrain.test.tssrc/types/ToolQuality.sveltesrc/types/Trap.test.tssrc/types/UsageDescription.sveltesrc/types/Vehicle.sveltesrc/types/VehiclePart.sveltesrc/types/Vitamin.sveltesrc/types/WeaponCategory.sveltesrc/types/item/AmmoInfo.sveltesrc/types/item/ArmorInfo.sveltesrc/types/item/CompatibleItems.sveltesrc/types/item/ComponentOf.sveltesrc/types/item/ConstructionByproduct.sveltesrc/types/item/DisarmTrap.test.tssrc/types/item/Disassembly.sveltesrc/types/item/DissectedFrom.sveltesrc/types/item/DissectedFrom.test.tssrc/types/item/GunAmmoInfo.sveltesrc/types/item/GunInfo.sveltesrc/types/item/HarvestedFrom.sveltesrc/types/item/HarvestedTo.sveltesrc/types/item/MagazineInfo.sveltesrc/types/item/MeleeInfo.sveltesrc/types/item/MilledFrom.sveltesrc/types/item/OvermapAppearance.sveltesrc/types/item/Recipes.sveltesrc/types/item/RequirementDataTools.sveltesrc/types/item/Salvaged.sveltesrc/types/item/SmokedFrom.sveltesrc/types/item/SpawnedIn.test.tssrc/types/item/spawnLocations.test.tstsconfig.jsontsconfig.node.json
💤 Files with no reviewable changes (2)
- src/routing.test.ts
- src/mod-provenance.test.ts
… data mods tests Simplify test cases by directly awaiting `data.loadData` calls.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/data-loader.ts`:
- Around line 65-74: The onload handler's truthy check for xhr.response can
incorrectly reject valid falsy primitives (0, false, ""), so update the check in
the xhr.onload callback to accept any non-null/undefined response (e.g., use
xhr.response !== null && xhr.response !== undefined) before calling
resolve(xhr.response); keep the existing 404 and status-range logic and the
handleError("Status") path unchanged so only the falsy-value handling around
xhr.response is fixed.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b6845a6-6427-4d0e-bedf-2ed8fc1d496b
📒 Files selected for processing (12)
docs/adr/ADR-007_data_loading_orchestration.mdscripts/fetch-fixtures.tsscripts/fetch-icons.tsscripts/gen-sitemap.tssrc/constants.tssrc/data-loader.test.tssrc/data-loader.tssrc/data.mods.test.tssrc/data.tssrc/i18n/game-locale.tssrc/tile-data.test.tstsconfig.node.json
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
1 participant
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
This PR refactors game-data loading around a clearer architectural boundary.
The core change is the one described in ADR-007:
src/data-loader.tsnow owns raw transport and fetch orchestration, whilesrc/data.tskeeps domain assembly, locale fallback, mod parsing/merging, cancellation, and store publication.Along the way, this branch also:
data.setVersion()withdata.loadData()src/i18n/game-locale.tsWhy
Before this refactor,
src/data.tswas carrying too many responsibilities at once:all.json, locale, pinyin, and mod-catalog fetchesCBNDataThat made the main loading path harder to reason about than the problem really is.
ADR-007 captures the simpler truth:
all.jsonis the only required assetall_mods.jsonare optional enrichmentsall.jsondownloadWhat Changed
Data-loading architecture
data.setVersion(...)withdata.loadData(version, locale, activeMods).loadRawDataset(...)as the single public entry point insrc/data-loader.ts.loadRawDataset(...)now fetchesall.json, locale JSON, zh pinyin JSON, andall_mods.jsonin parallel viaPromise.allSettled.src/data.tsnow consumes the settled raw asset bundle and remains responsible for:all_mods.jsonCBNDataLoading behavior
all.json.undefined.Mod metadata flow
ensureModsLoaded()path.ModSelectornow derives its catalog directly fromrawModsJsonand no longer has a separate loading/error state.Locale/runtime cleanup
src/i18n/gettext.tstosrc/i18n/game-locale.tsto make its role explicit.src/constants.ts.Docs and tests
src/data-loader.test.ts.docs/architecture.mdandDEVELOPMENT.md.What's Improved
loadData()is materially shorter and easier to reason about.Compromises / Trade-offs
all_mods.json, even if the user never opens the mod selector or activates mods.Promise.allSettledcan start optional requests that later turn out not to matter ifall.jsonfails.all.jsondominates perceived load time.Behavior Changes
User-visible
uk_UAcan resolve toukwhen that is the real available locale.all.jsonfails, the app fails immediately instead of waiting through retry attempts.Internal / reviewer-relevant
data.setVersion()was removed in favor ofdata.loadData().data.ensureModsLoaded()was removed; lazy mod metadata loading no longer exists.src/i18n/gettext.tswas renamed tosrc/i18n/game-locale.ts.CBNDatanow consistently carriesactiveMods/rawModsJSONdata from initial load rather than from a later enrichment step.Reviewer Notes / Potential Triggers
Removed lazy mod loading
This is intentional.
Previously, mod metadata could arrive later through
ensureModsLoaded(), which created a second loading model for the same screen and forcedModSelectorto carry loading/error UI states. This branch pays a small extra request up front to make the data model simpler and the selector deterministic.Removed retries from game-data loading
This is also intentional and follows ADR-007.
The relevant assets are static CDN-hosted JSON files. Retrying them in-app mostly adds delay and duplicate traffic rather than meaningful resilience. The new contract is:
all.json) fails fastOptional mod fetch errors are now quieter
This is the main semantic compromise in the branch.
Fetch failures for
all_mods.jsonno longer block the main dataset load, even for non-404 transport errors. A successful but invalidall_mods.jsonpayload still throws during parsing, so we are not masking schema corruption after a successful response.Critical Path For Manual Testing
Default, manual selection,Reset, andApplyin the mod selector and confirm the URL and selected mod set stay consistent after reload.ukand confirm translations apply.zh_CNand confirm the pinyin supplement path does not break loading.all_mods.jsonto fail and confirm the main app still loads while mod selection gracefully degrades.Verification