fix: add unobfuscated version awareness to MappingService#7
Conversation
MappingService.getMappings() and lookupMapping() now detect unobfuscated Minecraft versions (26.1+) and throw clear, actionable errors instead of failing with cryptic "Intermediary mappings not available" messages. The RemapService already handled this correctly, but tools like find_mapping and the mappings resource endpoint called MappingService directly, bypassing the check. Bumps version to 1.1.0 for npm publish — v1.0.0 on npm predates the unobfuscated version support added in PR #4.
|
@codex Review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for unobfuscated Minecraft versions (26.1+) by updating the mapping service to throw descriptive errors when mappings are missing. It includes new regression tests and updates the package version to 1.1.0. Reviewer feedback recommends optimizing test assertions with combined regex, utilizing parameterized tests to minimize duplication, and using single template literals for multi-line error strings to enhance code readability.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84171e1019
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Move unobfuscated check after cache hits in getMappings() to avoid network-gating cached mapping paths (Codex P1) - Extract throwIfUnobfuscated() helper called only before downloads - Remove redundant lookupMapping() guard — getMappings() handles it (Codex P1) - Combine double await-expect assertions into single regex (Gemini) - Use it.each for parameterized version-manager regression tests (Gemini) - Collapse string concatenation into single template literals (Gemini)
|
@gemini-code-assist Review |
|
@codex Review |
There was a problem hiding this comment.
Code Review
This pull request implements explicit handling for unobfuscated Minecraft versions (26.1+), ensuring that attempts to retrieve non-existent mapping files result in clear, actionable error messages. The changes include updates to the MappingService, a new throwIfUnobfuscated helper, and comprehensive regression tests, alongside a package rename and version bump to 1.1.0. Feedback suggests refactoring the getMappings method to unify control flow and eliminate duplicate logic, which would improve maintainability and adhere to DRY principles.
There was a problem hiding this comment.
Code Review
This pull request introduces handling for unobfuscated Minecraft versions (26.1+), ensuring that the mapping service provides clear, actionable error messages when mapping files are unavailable. It includes regression tests for specific snapshots and updates the package name and version. A review comment suggests refactoring the getMappings method in src/services/mapping-service.ts to reduce code duplication between the mojmap and general mapping download logic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 771015c9a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Extract getCachedMapping() and startDownload() to eliminate duplicate cache/lock/download logic between mojmap and other mapping types - Recheck dedupe lock after async throwIfUnobfuscated() to prevent concurrent callers from starting parallel downloads - Fix template literal concatenation lint warning
1 participant
Summary
Closes #5
Context
The RemapService already handled unobfuscated versions correctly (PR #4), but
MappingServicewas called directly by tools likefind_mapping,compare_versions, and the mappings resource endpoint — bypassing the check. Additionally, npm users on v1.0.0 never received the PR #4 fix at all.Test plan
getMappings(intermediary)on unobfuscated version → actionable error mentioning mojmapgetMappings(yarn)on unobfuscated version → actionable error mentioning mojmapgetMappings(mojmap)on unobfuscated version → actionable error explaining JAR is already in Mojang nameslookupMapping()on unobfuscated version → actionable errorlookupMapping()on unobfuscated version → identity (no files needed)isVersionUnobfuscatedreturns true for 26.1-snapshot-10, 26.1-snapshot-11, 26.1-rc-3find_mappingtool returns actionable error for unobfuscated versions🤖 Generated with Claude Code