fix: relic refinement drop rates #796
Conversation
WalkthroughThe PR expands Parser.addRelics to accept drop-rate data and enrich relic rewards using drops: it derives a dropPlaceName from relic names, filters matching drops, merges drop-based chances into each reward (deduplicated, first-match), updates call sites, and adds a test validating relic drop distributions by refinement level. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Relics as Relics Data
participant Drops as Drops Data
participant Rewards as Final Rewards
Note over Parser,Relics: New flow: enrich relic rewards with drops
Parser->>Relics: receive relics (with rewards metadata)
Parser->>Drops: receive drops array
Parser->>Parser: derive dropPlaceName from relic.name
Parser->>Drops: filter dropsForRelic (place == dropPlaceName)
Parser->>Parser: map baseRewards -> for each reward:
alt matching drop found
Parser->>Parser: merge drop.chance into reward (first-match, dedupe)
else no match
Parser->>Parser: keep original reward metadata
end
Parser->>Rewards: assign enriched rewards back to item
Note right of Rewards: rewards now include drops-aware chances
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
build/parser.mjs (2)
1096-1107: Consider adding observability for drops data merge outcomes.The current implementation silently falls back to original reward data when no matching drop is found. This could mask issues with drops API data availability or formatting. Consider adding logging or metrics to track merge success/failure rates.
Example approach (add after line 1107):
// Log merge statistics for observability const mergedCount = relicItem.rewards.filter(r => dropsForRelic.some(d => d.item === r.item?.name) ).length; if (mergedCount === 0 && dropsForRelic.length > 0) { console.warn(`No rewards merged for ${name}: format mismatch between drops and relics data`); }
1096-1098: Document the expected relic name format and drops API key format.The string manipulation logic assumes specific naming patterns for relics (ending with refinement level) and constructs the drops API lookup key accordingly. Consider adding a comment documenting the expected formats to aid future maintenance.
Example documentation:
// Build the correct "place" name for drops API lookup // Expected relic name formats: // - "Lith A1 Intact" -> drops key: "Lith A1 Relic" // - "Lith A1 Exceptional" -> drops key: "Lith A1 Relic (Exceptional)" // - "Lith A1 Radiant" -> drops key: "Lith A1 Relic (Radiant)" const dropPlaceName = name.endsWith('Intact') ? name.replace(/\sIntact$/, ' Relic') : name.replace(/\s(\w+)$/, ' Relic ($1)');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
build/parser.mjs(3 hunks)data/warnings.json(1 hunks)test/index.spec.mjs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: SlayerOrnstein
Repo: WFCD/warframe-items PR: 663
File: config/masterableCategories.json:1-2
Timestamp: 2024-12-03T23:47:24.677Z
Learning: In the 'warframe-items' repository, when suggesting changes to regex patterns in 'config/masterableCategories.json', ensure that added patterns only include items relevant to the current PR context, and avoid adding items not present in the context.
📚 Learning: 2025-03-29T18:52:41.800Z
Learnt from: TobiTenno
Repo: WFCD/warframe-items PR: 714
File: build/parser.mjs:451-469
Timestamp: 2025-03-29T18:52:41.800Z
Learning: Wiki data in the warframe-items builder should not have null checks as it's critical for the build process - if wiki fetch fails, the build should fail to prevent propagating incorrect data downstream.
Applied to files:
data/warnings.json
🧬 Code graph analysis (1)
build/parser.mjs (1)
build/scraper.mjs (1)
data(94-94)
🔇 Additional comments (3)
data/warnings.json (1)
9-9: LGTM!The expanded
missingWikiThumblist provides more comprehensive tracking of items missing wiki thumbnails. This is consistent with the broader data completeness improvements in this PR.build/parser.mjs (1)
1074-1074: Function signature update is correctly propagated across all call sites.The search found one call site at line 202 that correctly passes all three arguments including the
dropsparameter:this.addRelics(result, data.relics, data.drops). The function definition at line 1074 matches the expected signature with all three parameters. No updates are needed.test/index.spec.mjs (1)
368-410: Verify that known relic bugs are tracked in your issue tracker.The test is well-structured with clear documentation of workarounds for known data issues. However, I was unable to complete automated verification of whether Axi S19 (empty rewards) and Meso D1 (duplicate item) bugs are tracked in your issue system. Please manually confirm these data quality issues are documented in your tracker.
BUSTheid
changed the title
BUSTheid
force-pushed
the
fix/relic-refinement-drop-rates
branch
from
c6bd6d4 to
5649f24
Compare
|
I've cleaned up the commit history with a force push. The PR now has 2 commits (code changes + rebuilt data), fixing the regression I accidentally created when trying to fetch Meso D1 Relic (which reported identical values for both |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
build/parser.mjs (1)
1072-1074: Function signature update looks good.The addition of the
dropsparameter with corresponding JSDoc enables refinement-specific drop rate processing.Optionally, you could make the JSDoc more specific by adding the type annotation:
- * @param {Array<DropRate>} drops drop rate data for refinement-specific chances + * @param {Array<DropRate>} drops drop rate data for refinement-specific chances(Though the description is already clear enough.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
build/parser.mjs(3 hunks)data/warnings.json(1 hunks)test/index.spec.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/index.spec.mjs
- data/warnings.json
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SlayerOrnstein
Repo: WFCD/warframe-items PR: 663
File: config/masterableCategories.json:1-2
Timestamp: 2024-12-03T23:47:24.677Z
Learning: In the 'warframe-items' repository, when suggesting changes to regex patterns in 'config/masterableCategories.json', ensure that added patterns only include items relevant to the current PR context, and avoid adding items not present in the context.
🔇 Additional comments (2)
build/parser.mjs (2)
202-202: LGTM! Drops data correctly passed to addRelics.The addition of
data.dropsas the third argument enables refinement-specific drop rate integration as intended.
1089-1118: Well-implemented drops integration with proper edge case handling.The implementation correctly integrates refinement-specific drop rates from the Drops API while preserving reward metadata from the relics package. Key strengths:
- dropPlaceName construction (lines 1093-1096) correctly maps relic names to Drops API format for all refinement levels
- Null safety (line 1105) properly addresses the previous review concern about
reward.item.namewith a clean guard clause approach- Duplicate handling (lines 1102-1117) uses
usedDropIndicesSet to ensure each drop is matched only once when the same item appears multiple times- Graceful fallbacks handle missing drops data (line 1099) and unmatched rewards (line 1116)
|
@BUSTheid thanks! approving shortly, just waiting for the checks to pass! |
TobiTenno
added
Type: Bug
Scope: Data
|
Build got cloudflare-gated lol |
|
Yeah, if you could drop your new data updates off of it, I'll merge once it doesn't have conflicts (new conflicts are coming from master branch generating new data) and I'll test it locally. |
Head branch was pushed to by a user without write access
BUSTheid
force-pushed
the
fix/relic-refinement-drop-rates
branch
2 times, most recently
from
c79fcde to
e9aa5f8
Compare
Previously, all relic refinement levels (Intact, Exceptional, Flawless, Radiant) were getting identical drop rates because the `addRelics` function only used data from `@wfcd/relics` package, which contains only base (Intact) variants. Now fetches refinement-specific drop rates from the drops API and correctly matches them to rewards, including handling for duplicate item names in a single relic (e.g., Meso D1). Adds test validating distribution pattern across all refinement levels. Fixes WFCD#602
BUSTheid
force-pushed
the
fix/relic-refinement-drop-rates
branch
from
e9aa5f8 to
26d58a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
build/parser.mjs (2)
1089-1096: Consider the effectiveness of Set-based deduplication.Line 1091 uses
Array.from(new Set(...))to deduplicate rewards. SinceSetcompares by object reference, not deep equality, andflat()creates new object instances from each relic's rewards array, the deduplication may not work as intended—identical reward objects from different relics would not be deduplicated.If deduplication by value is required, consider using a helper like
dedupe(imported at line 18) or a manual deduplication strategy. If the current behavior is intentional (keeping all reward instances), theSetwrapper adds no value and could be removed for clarity.- const baseRewards = Array.from(new Set(related.map((relic) => relic.rewards).flat())); + const baseRewards = related.map((relic) => relic.rewards).flat();
1104-1117: Good null safety and duplicate handling; consider adding verification for unmatched rewards.The null safety check on line 1105 (
!reward.item?.name) correctly addresses the past review concern. TheusedDropIndicestracking provides a solid first-match strategy for handling duplicate item names across rewards.However, rewards that don't match any drop silently fall back to their original data (line 1116). While preserving metadata is intentional, consider adding optional verification (e.g., logging or a warning) when rewards remain unmatched, especially during development, to detect potential data inconsistencies between the relics package and the drops API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
build/parser.mjs(3 hunks)test/index.spec.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/index.spec.mjs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SlayerOrnstein
Repo: WFCD/warframe-items PR: 663
File: config/masterableCategories.json:1-2
Timestamp: 2024-12-03T23:47:24.677Z
Learning: In the 'warframe-items' repository, when suggesting changes to regex patterns in 'config/masterableCategories.json', ensure that added patterns only include items relevant to the current PR context, and avoid adding items not present in the context.
🔇 Additional comments (2)
build/parser.mjs (2)
202-202: LGTM!The call site correctly passes the drops data to support refinement-specific drop rates.
1072-1074: LGTM!The function signature and JSDoc are correctly updated to accept drops data.
|
Force-pushed without the data rebuilt commit and rebased on the latest master |
|
🎉 This PR is included in version 1.1270.887 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
@BUSTheid there we go, out it went |
3 participants
What did you fix?
fixes #602
All relic refinement levels (Intact, Exceptional, Flawless, Radiant) were showing identical drop rates (e.g. 2% for rare items) instead of their refinement-specific rates.
The bug was in the
addRelicsfunction which was using reward data from@wfcd/relicspackage (which only contains Intact/base drop rates) instead of the drops API (which has accurate refinement-specific data).Reproduction steps
npm install && npm run build -- --forcedata/json/Relics.jsonEvidence/screenshot/link to line
addRelicsConsiderations
Summary by CodeRabbit
New Features
Bug Fixes
Tests