fix: reduce duplicate image downloads#908
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe build pipeline is restructured to load persisted warnings, deduplicate image filenames through a new pass, and download images per-category while updating warnings incrementally. Image naming logic is simplified: hard-coded filenames for specific items and textureLocation-based derivation for others, replacing prior hash-based collision detection. A new test suite validates image file existence, size, and parseability. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
SlayerOrnstein
force-pushed
the
simple-image-name
branch
from
82ac6b3 to
d785728
Compare
SlayerOrnstein
force-pushed
the
simple-image-name
branch
2 times, most recently
from
845d0ee to
1db7259
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build/build.ts (1)
184-188:⚠️ Potential issue | 🔴 CriticalKeep the image-name reconciliation pass even when the manifest hash is unchanged.
saveImage()now mutatesitem.imageNamefor duplicate hashes and filename conflicts, but Line 188 returns before any of that runs when only non-manifest data changed.saveJson()then re-emits the parser's raw names, so duplicate items can point at files that were never written. Let the loop run and rely on the per-itemcached?.hash !== hashcheck to skip actual downloads.Proposed fix
async saveImages(items: Item[], manifest: ImageManifest): Promise<void> { - // No need to go through every item if the manifest didn't change. I'm - // guessing the `fileTime` key in each element works more or less like a - // hash, so any change to that changes the hash of the full thing. - if (!hashManager.hasChanged('Manifest')) return; const bar = new Progress('Fetching Images', items.length);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/build.ts` around lines 184 - 188, The early return in saveImages prevents the image-name reconciliation that saveImage performs, so remove or alter the "if (!hashManager.hasChanged('Manifest')) return;" logic in saveImages to always iterate items; instead, only skip the actual download per-item by relying on the existing per-item check (cached?.hash !== hash) inside saveImage. Ensure saveImages still calls saveImage for every Item to allow mutation of item.imageName (resolving duplicate hashes/filename conflicts) while letting saveImage decide whether to download based on cached?.hash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build/build.ts`:
- Around line 257-268: The collision check currently treats any existing file as
a collision (using existsSync(filePath)), which can rename the current item's
own previous output and break later hash checks; modify the condition so we only
treat it as a collision when the on-disk file is owned by a different item—e.g.
check ownership via cached?.uniqueName or the processed[hash] mapping instead of
existsSync alone: only enter the rename branch when existsSync(filePath) &&
cached?.uniqueName !== item.uniqueName (or processed[hash] !== item.imageName),
leaving the file untouched if the existing file belongs to the same uniqueName,
and ensure the subsequent cached/hash logic still prevents unnecessary writes
when the hash matches.
In `@build/parser.ts`:
- Around line 575-588: In addImageName(), normalize backslashes on
image.textureLocation (same as saveImage() uses) before splitting: const
imageStub = image.textureLocation.replace(/\\/g, '/'); then derive uniqueName =
imageStub.split('!')[0] and treat an empty string as invalid (if (!uniqueName) {
warnings.missingImage.push(item.name); return; }). Next get imageName =
uniqueName.split('/').reverse()[0] and reject empty imageName similarly. After
calling sanitize(imageName), validate the sanitized result is non-empty before
assigning item.imageName = sanitized; otherwise push to warnings.missingImage
and return. Reference: addImageName(), image.textureLocation, saveImage(),
sanitize(), warnings.missingImage, item.imageName.
---
Outside diff comments:
In `@build/build.ts`:
- Around line 184-188: The early return in saveImages prevents the image-name
reconciliation that saveImage performs, so remove or alter the "if
(!hashManager.hasChanged('Manifest')) return;" logic in saveImages to always
iterate items; instead, only skip the actual download per-item by relying on the
existing per-item check (cached?.hash !== hash) inside saveImage. Ensure
saveImages still calls saveImage for every Item to allow mutation of
item.imageName (resolving duplicate hashes/filename conflicts) while letting
saveImage decide whether to download based on cached?.hash.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8d4d278-bf7b-4e7c-9653-5f4058e22dd8
⛔ Files ignored due to path filters (296)
data/img/10YearAnniversaryLoginSongItemStoreIcon.pngis excluded by!**/*.pngdata/img/13angTV.pngis excluded by!**/*.pngdata/img/1999Aoi.pngis excluded by!**/*.pngdata/img/1999BoybandPoster.pngis excluded by!**/*.pngdata/img/1999ComicCoverPoster.pngis excluded by!**/*.pngdata/img/1999CommunityARGEmblem.pngis excluded by!**/*.pngdata/img/1999Drippy.pngis excluded by!**/*.pngdata/img/1999EntHybridPistol.pngis excluded by!**/*.pngdata/img/1999FlareBandPoster.pngis excluded by!**/*.pngdata/img/1999InfShotgunWeapon.pngis excluded by!**/*.pngdata/img/1999InfSporePistolWeapon.pngis excluded by!**/*.pngdata/img/1999OnlyneWallPoster.pngis excluded by!**/*.pngdata/img/1999Pizza.pngis excluded by!**/*.pngdata/img/1999PrologueQuestKeyChain.pngis excluded by!**/*.pngdata/img/1999QuestKeyChain.pngis excluded by!**/*.pngdata/img/1999ResourceCommonA.pngis excluded by!**/*.pngdata/img/1999ResourceCommonAContainer.pngis excluded by!**/*.pngdata/img/1999ResourceCommonB.pngis excluded by!**/*.pngdata/img/1999ResourceCommonBContainer.pngis excluded by!**/*.pngdata/img/1999ResourceDefense.pngis excluded by!**/*.pngdata/img/1999ResourceRareA.pngis excluded by!**/*.pngdata/img/1999ResourceRareAContainer.pngis excluded by!**/*.pngdata/img/1999ResourceUncommonA.pngis excluded by!**/*.pngdata/img/1999ResourceUncommonAContainer.pngis excluded by!**/*.pngdata/img/1999ResourceUncommonB.pngis excluded by!**/*.pngdata/img/1999ResourceUncommonBContainer.pngis excluded by!**/*.pngdata/img/1999TankHoodOrnament.pngis excluded by!**/*.pngdata/img/1999TrophyBronze.pngis excluded by!**/*.pngdata/img/1999Y2KEarB.pngis excluded by!**/*.pngdata/img/1999Y2KEarC.pngis excluded by!**/*.pngdata/img/1999Y2KEyeA.pngis excluded by!**/*.pngdata/img/1999Y2KEyeB.pngis excluded by!**/*.pngdata/img/1999Y2KEyeC.pngis excluded by!**/*.pngdata/img/2020ZerOGlyph.pngis excluded by!**/*.pngdata/img/2025GCX.pngis excluded by!**/*.pngdata/img/4MORI4N.pngis excluded by!**/*.pngdata/img/7thAnniversaryPoster.pngis excluded by!**/*.pngdata/img/8thAnniversaryPoster.pngis excluded by!**/*.pngdata/img/A.pngis excluded by!**/*.pngdata/img/AGGP.pngis excluded by!**/*.pngdata/img/AHCommonPickup.pngis excluded by!**/*.pngdata/img/AHR.pngis excluded by!**/*.pngdata/img/AHRarePickup.pngis excluded by!**/*.pngdata/img/AHUncommonPickup.pngis excluded by!**/*.pngdata/img/AK47Weapon.pngis excluded by!**/*.pngdata/img/AVReceiver.pngis excluded by!**/*.pngdata/img/AbacusSmall.pngis excluded by!**/*.pngdata/img/AbberaPrime.pngis excluded by!**/*.pngdata/img/AbilityBlock.jpgis excluded by!**/*.jpgdata/img/AbilityDurationMod.jpgis excluded by!**/*.jpgdata/img/AbilityEfficiencyMod.jpgis excluded by!**/*.jpgdata/img/AbilityRangeMod.jpgis excluded by!**/*.jpgdata/img/AbilityStrengthMod.jpgis excluded by!**/*.jpgdata/img/Abrasys.pngis excluded by!**/*.pngdata/img/AbyssOfDagathLoginSongItemStoreIcon.pngis excluded by!**/*.pngdata/img/AcceltraDeluxeIISkin.pngis excluded by!**/*.pngdata/img/AcceltraDeluxeSkin.pngis excluded by!**/*.pngdata/img/AcceltraPrime.pngis excluded by!**/*.pngdata/img/AccessibleGamer.pngis excluded by!**/*.pngdata/img/AccuracyWhileAiming.jpgis excluded by!**/*.jpgdata/img/AckAndBrunt.pngis excluded by!**/*.pngdata/img/AckAndBruntConclave.pngis excluded by!**/*.pngdata/img/AckAndBruntDayOfTheDead.pngis excluded by!**/*.pngdata/img/AckAndBruntNigtwave.pngis excluded by!**/*.pngdata/img/AckBruntIncarnonAdapter.pngis excluded by!**/*.pngdata/img/AckBruntNightwatchMod.jpgis excluded by!**/*.jpgdata/img/AcolyteSynpai.pngis excluded by!**/*.pngdata/img/Acrid.jpgis excluded by!**/*.jpgdata/img/Acrid.pngis excluded by!**/*.pngdata/img/ActivateDeimosRequiemTotem.pngis excluded by!**/*.pngdata/img/Actuator.pngis excluded by!**/*.pngdata/img/AdelfosSelene.pngis excluded by!**/*.pngdata/img/AdikDarkCero.pngis excluded by!**/*.pngdata/img/AdmiralBahroo.pngis excluded by!**/*.pngdata/img/AdrenalStim.pngis excluded by!**/*.pngdata/img/AdultHeadARemastered.pngis excluded by!**/*.pngdata/img/AdultHeadBRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadCRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadDRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadERemastered.pngis excluded by!**/*.pngdata/img/AdultHeadFRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleA.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleB.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleC.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleD.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleE.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleF.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleG.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleH.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleI.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleJ.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleK.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleL.pngis excluded by!**/*.pngdata/img/AdultHeadFemaleM.pngis excluded by!**/*.pngdata/img/AdultHeadGRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadHRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadIRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadJRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadKRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadLRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadMRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadMaleA.pngis excluded by!**/*.pngdata/img/AdultHeadMaleB.pngis excluded by!**/*.pngdata/img/AdultHeadMaleC.pngis excluded by!**/*.pngdata/img/AdultHeadMaleD.pngis excluded by!**/*.pngdata/img/AdultHeadMaleE.pngis excluded by!**/*.pngdata/img/AdultHeadMaleF.pngis excluded by!**/*.pngdata/img/AdultHeadMaleG.pngis excluded by!**/*.pngdata/img/AdultHeadMaleH.pngis excluded by!**/*.pngdata/img/AdultHeadMaleI.pngis excluded by!**/*.pngdata/img/AdultHeadMaleJ.pngis excluded by!**/*.pngdata/img/AdultHeadMaleK.pngis excluded by!**/*.pngdata/img/AdultHeadMaleL.pngis excluded by!**/*.pngdata/img/AdultHeadMaleM.pngis excluded by!**/*.pngdata/img/AdultHeadNRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadORemastered.pngis excluded by!**/*.pngdata/img/AdultHeadPRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadQRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadRRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadSRemastered.pngis excluded by!**/*.pngdata/img/AdultHeadTRemastered.pngis excluded by!**/*.pngdata/img/AdultOperatorAgile.pngis excluded by!**/*.pngdata/img/AdultOperatorNoble.pngis excluded by!**/*.pngdata/img/AeonKnight.pngis excluded by!**/*.pngdata/img/AesopYOLIANGlyph.pngis excluded by!**/*.pngdata/img/Afuris.pngis excluded by!**/*.pngdata/img/AfurisForestCamoSkin.pngis excluded by!**/*.pngdata/img/AfurisPrime.pngis excluded by!**/*.pngdata/img/Agkuza.pngis excluded by!**/*.pngdata/img/AimGlide.pngis excluded by!**/*.pngdata/img/AirHockey.pngis excluded by!**/*.pngdata/img/AirSlideBoost.jpgis excluded by!**/*.jpgdata/img/AirSupportCarpetBomb.pngis excluded by!**/*.pngdata/img/AirSupportGrineer.pngis excluded by!**/*.pngdata/img/AirSupportLiset.pngis excluded by!**/*.pngdata/img/AirSupportMantis.pngis excluded by!**/*.pngdata/img/AirSupportNightwave.pngis excluded by!**/*.pngdata/img/AirSupportRareInstinct.pngis excluded by!**/*.pngdata/img/AirSupportSentryTurret.pngis excluded by!**/*.pngdata/img/Ajingom.pngis excluded by!**/*.pngdata/img/AkSura.pngis excluded by!**/*.pngdata/img/Akariayataka.pngis excluded by!**/*.pngdata/img/AkariusPrime.pngis excluded by!**/*.pngdata/img/Akbolto.pngis excluded by!**/*.pngdata/img/AkboltoOrmolu.pngis excluded by!**/*.pngdata/img/AkboltoPrime.pngis excluded by!**/*.pngdata/img/Akbronco.pngis excluded by!**/*.pngdata/img/AkbroncoPrime.pngis excluded by!**/*.pngdata/img/AkbroncoPrimeViralMod.jpgis excluded by!**/*.jpgdata/img/Akjagara.pngis excluded by!**/*.pngdata/img/AkjagaraIridosSkin.pngis excluded by!**/*.pngdata/img/AkjagaraPrime.pngis excluded by!**/*.pngdata/img/Aklato.pngis excluded by!**/*.pngdata/img/AklatoConclaveSkin.pngis excluded by!**/*.pngdata/img/AklatoDayOfTheDead.pngis excluded by!**/*.pngdata/img/AklatoKintsugi.pngis excluded by!**/*.pngdata/img/AklatoNocturne.pngis excluded by!**/*.pngdata/img/AklatoRixtyMOLSkin.pngis excluded by!**/*.pngdata/img/Aklex.pngis excluded by!**/*.pngdata/img/AklexConclaveSkin.pngis excluded by!**/*.pngdata/img/AklexPrime.pngis excluded by!**/*.pngdata/img/Akmagnus.pngis excluded by!**/*.pngdata/img/AkmagnusDakila.pngis excluded by!**/*.pngdata/img/AkmagnusHiveLight.pngis excluded by!**/*.pngdata/img/AkmagnusObsidian.pngis excluded by!**/*.pngdata/img/AkmagnusPrime.pngis excluded by!**/*.pngdata/img/Akrabu.pngis excluded by!**/*.pngdata/img/AkrabuPrime.pngis excluded by!**/*.pngdata/img/Aksomati.pngis excluded by!**/*.pngdata/img/AksomatiPrime.pngis excluded by!**/*.pngdata/img/Akstileto.pngis excluded by!**/*.pngdata/img/AkstiletoPrime.pngis excluded by!**/*.pngdata/img/AkstilettoConclaveSkin.pngis excluded by!**/*.pngdata/img/AkstilettoPrimeAmmoEfficiencyMod.jpgis excluded by!**/*.jpgdata/img/Akvasto.pngis excluded by!**/*.pngdata/img/AkvastoConclaveSkin.pngis excluded by!**/*.pngdata/img/AkvastoDayOfTheDeadSkin.pngis excluded by!**/*.pngdata/img/AkvastoForestCamoSkin.pngis excluded by!**/*.pngdata/img/AkvastoPrime.pngis excluded by!**/*.pngdata/img/AkvastoVoidSkin.pngis excluded by!**/*.pngdata/img/Akzani.pngis excluded by!**/*.pngdata/img/AladV.pngis excluded by!**/*.pngdata/img/AlainLove.pngis excluded by!**/*.pngdata/img/Albrecht2in1Display.pngis excluded by!**/*.pngdata/img/AlbrechtHatCommunityGlyph.pngis excluded by!**/*.pngdata/img/AlbrechtPortrait.pngis excluded by!**/*.pngdata/img/AlchemistAgile.pngis excluded by!**/*.pngdata/img/AlchemistDistill.pngis excluded by!**/*.pngdata/img/AlchemistNoble.pngis excluded by!**/*.pngdata/img/AlchemistRush.pngis excluded by!**/*.pngdata/img/AlchemistSerpent.pngis excluded by!**/*.pngdata/img/AlchemistTransmuteAugmentCard.jpgis excluded by!**/*.jpgdata/img/AlchemistTransmuter.pngis excluded by!**/*.pngdata/img/Alertium.pngis excluded by!**/*.pngdata/img/AlexanderDario.pngis excluded by!**/*.pngdata/img/AlexandraLive.pngis excluded by!**/*.pngdata/img/AlloyPlate.pngis excluded by!**/*.pngdata/img/AlternoxDeluxeSkin.pngis excluded by!**/*.pngdata/img/AlternoxPrime.pngis excluded by!**/*.pngdata/img/Althani.pngis excluded by!**/*.pngdata/img/Altra.pngis excluded by!**/*.pngdata/img/AltraPrime.pngis excluded by!**/*.pngdata/img/Alyekk.pngis excluded by!**/*.pngdata/img/AmalgamArgonak.jpgis excluded by!**/*.jpgdata/img/AmalgamDaikyu.jpgis excluded by!**/*.jpgdata/img/AmalgamEventBadge.pngis excluded by!**/*.pngdata/img/AmalgamFurax.jpgis excluded by!**/*.jpgdata/img/AmalgamJavlok.jpgis excluded by!**/*.jpgdata/img/AmalgamRipkas.jpgis excluded by!**/*.jpgdata/img/AmarExilusMod.jpgis excluded by!**/*.jpgdata/img/AmarHeader.pngis excluded by!**/*.pngdata/img/AmarMeleeMod.jpgis excluded by!**/*.jpgdata/img/AmarWarframeMod.jpgis excluded by!**/*.jpgdata/img/AmaruChromaAvatarBright.pngis excluded by!**/*.pngdata/img/AmaruChromaAvatarDark.pngis excluded by!**/*.pngdata/img/AmazingBuriGlyph.pngis excluded by!**/*.pngdata/img/AmazonOni.pngis excluded by!**/*.pngdata/img/Ambulas.pngis excluded by!**/*.pngdata/img/AmbulasDataFragment.pngis excluded by!**/*.pngdata/img/AmbulasEventBadge.pngis excluded by!**/*.pngdata/img/AmbulasEvent_e.pngis excluded by!**/*.pngdata/img/Amesha.pngis excluded by!**/*.pngdata/img/AmethystAntitoxin.pngis excluded by!**/*.pngdata/img/AmirAccoladeGlyph.pngis excluded by!**/*.pngdata/img/AmirPixelGlyph.pngis excluded by!**/*.pngdata/img/AmirValentineGlyph.pngis excluded by!**/*.pngdata/img/Amphis.pngis excluded by!**/*.pngdata/img/Amphor.pngis excluded by!**/*.pngdata/img/Amprex.pngis excluded by!**/*.pngdata/img/AmprexDayofTheDeadSkin.pngis excluded by!**/*.pngdata/img/AnJetCat.pngis excluded by!**/*.pngdata/img/AnaviIvy.pngis excluded by!**/*.pngdata/img/AngelsOfTheZarimanQuestKeychain.pngis excluded by!**/*.pngdata/img/AngelsOfTheZarimanSongItemStoreIcon.pngis excluded by!**/*.pngdata/img/AngryIceberg.pngis excluded by!**/*.pngdata/img/AngryUnicorn.pngis excluded by!**/*.pngdata/img/Angstrum.pngis excluded by!**/*.pngdata/img/AngstrumConclave.pngis excluded by!**/*.pngdata/img/AngstrumDayOfTheDead.pngis excluded by!**/*.pngdata/img/AngstrumIncarnonAdapter.pngis excluded by!**/*.pngdata/img/AnimaAlt2Helmet.pngis excluded by!**/*.pngdata/img/AnimaAspect.pngis excluded by!**/*.pngdata/img/AnimalInstincts.jpgis excluded by!**/*.jpgdata/img/AnimalLure.pngis excluded by!**/*.pngdata/img/AnimalTagBolarolaCommon.pngis excluded by!**/*.pngdata/img/AnimalTagBolarolaRare.pngis excluded by!**/*.pngdata/img/AnimalTagBolarolaUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagCondrocCommon.pngis excluded by!**/*.pngdata/img/AnimalTagCondrocRare.pngis excluded by!**/*.pngdata/img/AnimalTagCondrocUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagHorrasqueCommon.pngis excluded by!**/*.pngdata/img/AnimalTagHorrasqueRare.pngis excluded by!**/*.pngdata/img/AnimalTagHorrasqueUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedCritterCommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedCritterRare.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedCritterUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedKDriveCommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedKDriveRare.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedKDriveUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedMaggotCommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedMaggotRare.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedMaggotUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedMergooCommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedMergooRare.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedMergooUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedPredatorCommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedPredatorRare.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedPredatorUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedThwompCommon.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedThwompRare.pngis excluded by!**/*.pngdata/img/AnimalTagInfestedThwompUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagKuakaCommon.pngis excluded by!**/*.pngdata/img/AnimalTagKuakaRare.pngis excluded by!**/*.pngdata/img/AnimalTagKuakaUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagKubrodonCommon.pngis excluded by!**/*.pngdata/img/AnimalTagKubrodonRare.pngis excluded by!**/*.pngdata/img/AnimalTagKubrodonUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagMergooCommon.pngis excluded by!**/*.pngdata/img/AnimalTagMergooRare.pngis excluded by!**/*.pngdata/img/AnimalTagMergooUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagPobbersCommon.pngis excluded by!**/*.pngdata/img/AnimalTagPobbersRare.pngis excluded by!**/*.pngdata/img/AnimalTagPobbersUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagSawgawCommon.pngis excluded by!**/*.pngdata/img/AnimalTagSawgawRare.pngis excluded by!**/*.pngdata/img/AnimalTagSawgawUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagStoverCommon.pngis excluded by!**/*.pngdata/img/AnimalTagStoverRare.pngis excluded by!**/*.pngdata/img/AnimalTagStoverUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagVampireKavatCommon.pngis excluded by!**/*.pngdata/img/AnimalTagVampireKavatRare.pngis excluded by!**/*.pngdata/img/AnimalTagVampireKavatUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagVirminkCommon.pngis excluded by!**/*.pngdata/img/AnimalTagVirminkRare.pngis excluded by!**/*.pngdata/img/AnimalTagVirminkUncommon.pngis excluded by!**/*.pngdata/img/AnimalTagZongroCommon.pngis excluded by!**/*.png
📒 Files selected for processing (4)
build/build.tsbuild/parser.tsdata/cache/.export.jsondata/cache/.images.json
SlayerOrnstein
force-pushed
the
simple-image-name
branch
from
720d881 to
f220eac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
build/build.ts (1)
257-264:⚠️ Potential issue | 🟠 MajorCollision check still triggers for current item's own previous output.
The
existsSync(filePath)check at line 258 fires when any file with that name exists, including the current item's output from a previous run. On incremental builds where the hash hasn't changed:
processed[hash]is empty (fresh run state) → continueexistsSync("texture.png")istrue(from previous run)item.imageNamerenamed to"ItemName.png"cached?.hash === hash→ download skipped"ItemName.png"never created, but JSON points to itTo only rename when a different item owns the file, add an ownership check:
- if (existsSync(filePath)) { + if (existsSync(filePath) && cached?.uniqueName !== item.uniqueName) {This skips the rename when the existing file belongs to the same item, allowing the subsequent cache check to correctly prevent unnecessary re-downloads while keeping
item.imageNamevalid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/build.ts` around lines 257 - 264, The collision check renames the file whenever it exists, even if the existing file is the same item's previous output; update the existsSync(filePath) branch to detect ownership first (use processed which maps hash->imageName): compute ownerHash = Object.entries(processed).find(([, name]) => name === item.imageName)?.[0] and only perform the rename when ownerHash is undefined or ownerHash !== hash (i.e., a different item owns that filename); otherwise skip renaming so cached?.hash === hash can correctly skip re-download and item.imageName stays valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build/build.ts`:
- Around line 120-127: The sort method currently contains a leftover debug
console.log call that prints the Item when a.name is falsy; remove that
console.log(a) from the sort(a: Item, b: Item): number function so the
comparator only uses a.name.localeCompare and falls back to
a.uniqueName.localeCompare when names are equal (no other logic changes needed).
---
Duplicate comments:
In `@build/build.ts`:
- Around line 257-264: The collision check renames the file whenever it exists,
even if the existing file is the same item's previous output; update the
existsSync(filePath) branch to detect ownership first (use processed which maps
hash->imageName): compute ownerHash = Object.entries(processed).find(([, name])
=> name === item.imageName)?.[0] and only perform the rename when ownerHash is
undefined or ownerHash !== hash (i.e., a different item owns that filename);
otherwise skip renaming so cached?.hash === hash can correctly skip re-download
and item.imageName stays valid.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 866b2b98-7580-4ef3-876c-56c5afc1d8ea
📒 Files selected for processing (3)
build/build.tsbuild/parser.tstest/utilities/find.spec.mjs
✅ Files skipped from review due to trivial changes (1)
- test/utilities/find.spec.mjs
AyAyEm
reviewed
Apr 15, 2026
Member
AyAyEm
left a comment
AyAyEm
left a comment
There was a problem hiding this comment.
Couldn't find the conflict name resolution for these items:
Octavia.png 2
Mesa.png 2
Ember.png 2
Frost.png 2
Gara.png 2
Mirage.png 2
Mag.png 2
Rhino.png 2
InarosPrime.png 2
EliteSanctuaryOnslaught.png 2
FireExtinguisher.png 2
GenericComponent.png 2
MerulinaPrime.png 2
GenericArchwingSystems.png 2
Amphis.png 2
Imperator.png 2
GenericGear.png 2
AccuracyWhileAiming.jpg 3
CritChanceWhileAiming.jpg 3
CritDamageWhileAiming.jpg 3
FireRateWhileAiming.jpg 3
StatusChanceWhileAiming.jpg 3
WeaponFireIterationsSPMod.jpg 3
WeaponStatusChanceSPMod.jpg 3
WeaponWeakpointCriticalChanceMod.jpg 2
EquinoxAgile.png 2
EquinoxNoble.png 2
AmalgamEventBadge.png 2
GarudaDeluxe.png 2
HydroidDeluxe.png 2
RhinoDeluxe.png 2
Rank10Seeker.png 2
TobiTenno
reviewed
Apr 16, 2026
Member
|
@SlayerOrnstein i'm totally down for this, but the tests are failing... |
Member
Author
|
Yeah just been picking at the code a bit. Was redoing it so that |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/index.spec.mjs (1)
422-443: Tests overlap: "should have the image stored on disk" already checkssize > 0.The first test (lines 422-433) checks both existence and size. The second test (lines 435-443) duplicates the size check. Consider consolidating or removing the redundant test.
♻️ Suggested consolidation
it('should have the image stored on disk', () => { for (const { imageName, uniqueName } of imageNames) { if(imageName === 'missing.png') continue; const imagePath = path.join('./data/img/', imageName); const nameInfo = `in { uniqueName: ${uniqueName}, imageName: ${imageName} }`; - let exists = existsSync(imagePath); - assert(exists, `${imageName} should exist ${nameInfo}`); - if (exists) { - const { size } = statSync(imagePath, () => {}); - assert(size > 0, `size should be greater than zero ${nameInfo}`); - } + assert(existsSync(imagePath), `${imageName} should exist ${nameInfo}`); + const { size } = statSync(imagePath); + assert(size > 0, `size should be greater than zero ${nameInfo}`); } }); - it('size should be greater than 0', () => { - for (const { imageName, uniqueName } of imageNames) { - if(imageName === 'missing.png') continue; - const imagePath = path.join('./data/img/', imageName); - const nameInfo = `in { uniqueName: ${uniqueName}, imageName: ${imageName} }`; - const { size } = statSync(imagePath, () => {}); - assert(size > 0, `size should be greater than zero ${nameInfo}`); - } - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.spec.mjs` around lines 422 - 443, The second test "size should be greater than 0" duplicates the size assertion already performed in "should have the image stored on disk"; remove the redundant test or merge its assertions into the first test: keep the loop over imageNames (skipping 'missing.png'), verify file existence with existsSync and then check size > 0 using statSync in the same test (symbols: tests named "should have the image stored on disk" and "size should be greater than 0", imageNames array, path.join('./data/img/', imageName), existsSync, statSync, assert).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build/build.ts`:
- Line 386: The comparator passed to group.sort is indexing priorityMap with
Item objects (a and b) which yields undefined; change it to index using the
Item's category string property (e.g., use a.category or the actual category
field used on Item) so the comparison becomes (priorityMap[a.category] ??
Infinity) - (priorityMap[b.category] ?? Infinity); update the group.sort call
accordingly to reference the correct category key from the Item objects to
restore priority-based ordering and deduplication.
In `@test/index.spec.mjs`:
- Line 440: The test is calling fs.statSync with a callback (const { size } =
statSync(imagePath, () => {})), but statSync is synchronous and accepts no
callback; remove the callback and call statSync(imagePath) (or switch to an
async stat/fs.promises.stat if you intended async behavior) so that the
destructuring const { size } = statSync(imagePath) works correctly; update the
reference in the test where statSync and imagePath are used.
- Line 430: The call to statSync incorrectly passes a callback
(statSync(imagePath, () => {})); remove the second argument so the synchronous
fs.statSync is used properly: call statSync with only the path (referenced
symbol: statSync and variable imagePath) and keep extracting { size } from its
returned Stats object.
---
Nitpick comments:
In `@test/index.spec.mjs`:
- Around line 422-443: The second test "size should be greater than 0"
duplicates the size assertion already performed in "should have the image stored
on disk"; remove the redundant test or merge its assertions into the first test:
keep the loop over imageNames (skipping 'missing.png'), verify file existence
with existsSync and then check size > 0 using statSync in the same test
(symbols: tests named "should have the image stored on disk" and "size should be
greater than 0", imageNames array, path.join('./data/img/', imageName),
existsSync, statSync, assert).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bdff327-626f-40d8-acd2-2c67182a3ac6
⛔ Files ignored due to path filters (2)
data/img/arcane.pngis excluded by!**/*.pngdata/img/blueprint.pngis excluded by!**/*.png
📒 Files selected for processing (2)
build/build.tstest/index.spec.mjs
SlayerOrnstein
force-pushed
the
simple-image-name
branch
from
6052745 to
add9edb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build/build.ts`:
- Around line 181-182: The code currently uses history: string[] to record seen
hashes which causes duplicates to return without adopting an existing written
filename and also marks a hash seen before the write succeeds; change history to
a Map<string,string> (or object) mapping hash -> successful filename, only
insert into that map after the file is written successfully, and when
encountering an already-seen hash lookup and return the mapped filename so later
resources reuse the first successful filename (update logic around the block
that reads/writes imageName and where history is pushed).
- Line 66: The catch currently wraps a parent image and its nested
components/abilities so when a nested asset fails we only set the parent to
missing.png and then restore parsed.warnings.failedImage =
[...warnings.failedImage], which overwrites and drops newly-detected failures;
instead scope failures to the exact asset that threw, push that asset's
identifier onto parsed.warnings.failedImage (or merge warnings.failedImage with
parsed.warnings.failedImage) at the point of the catch, and ensure the code that
sets the parent to "missing.png" does not short-circuit processing of other
nested assets so all individual failures get persisted for retry.
- Line 43: Wrap the call to readJson in a try/catch and default to an empty
Warnings shape when the file is missing: call readJson<Warnings>(...) inside a
try block, assign its result to the warnings variable, and in the catch check
for a "file not found" error (e.g., ENOENT) or otherwise fallback to an empty
object of type Warnings; update any code that uses the warnings variable
accordingly so build continues when warnings.json is absent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dd56305-f7f8-4c66-b277-f981b0fd0692
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.github/workflows/build.yaml.github/workflows/release.yaml.github/workflows/static.yamlREADME.mdbuild/build.tsdata/cache/.export.jsondata/json/All.jsondata/json/Arch-Gun.jsondata/json/Arch-Melee.jsondata/json/Melee.jsondata/json/Misc.jsondata/json/Pets.jsondata/json/Primary.jsondata/json/Relics.jsondata/json/Resources.jsondata/json/Secondary.jsondata/json/SentinelWeapons.jsondata/json/Skins.jsondata/json/Warframes.jsondata/json/i18n.jsonpackage.json
✅ Files skipped from review due to trivial changes (6)
- package.json
- .github/workflows/build.yaml
- .github/workflows/release.yaml
- .github/workflows/static.yaml
- README.md
- data/cache/.export.json
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
build/build.ts (4)
43-43:⚠️ Potential issue | 🟠 MajorHandle a missing
warnings.jsonhere.Line 43 makes the build depend on a previously generated artifact. On a clean checkout, this can fail before any work starts. Please fall back to an empty
Warningsobject instead of hard-failing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/build.ts` at line 43, The build currently crashes if warnings.json is missing; wrap the readJson<Warnings>(new URL('../data/warnings.json', import.meta.url)) call in a try/catch and, on failure (or specifically if the error indicates file-not-found), set warnings to an empty Warnings object (e.g., {} as Warnings) instead of throwing; update the code that references warnings to expect this fallback.
187-205:⚠️ Potential issue | 🟠 MajorCatch failures per asset, not per parent item.
Lines 188-205 wrap the parent image plus every component and ability together. If one nested download throws, the parent gets
missing.png, the remaining nested assets are skipped, and the failing asset never lands inwarnings.failedImage, so it is lost on the next run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/build.ts` around lines 187 - 205, The current try/catch around the whole parent/item block causes one nested failure to mark the entire parent as missing and skip remaining assets; instead move error handling to each call to saveImage so failures are caught per asset: wrap the calls to this.saveImage(item,...), each this.saveImage(component,...), and each this.saveImage(ability...) in their own try/catch; in each catch push the specific asset name into warnings.failedImage (and only push to warnings.missingImage or set item.imageName = 'missing.png' for the parent item when the parent image itself fails), and continue processing other components/abilities so no nested asset is skipped.
242-246:⚠️ Potential issue | 🔴 CriticalOnly mark an image as processed after a successful write.
Lines 245-246 add
imageBase.textureLocationtohistorybefore the file is downloaded and written. If that first attempt fails, later items sharing the same resource return early and keep animageNamethat was never written to disk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/build.ts` around lines 242 - 246, The code prematurely marks imageBase.textureLocation as processed (history.push) before the download/write completes; change the flow so you only add imageBase.textureLocation to history after the file is successfully written (or write+verification succeeds) and ensure the early return that checks history (the if that includes history.includes(imageBase.textureLocation)) remains but only short-circuits after a successful previous write; update the logic around the handling of item.name ('Blueprint'/'Arcane') and the write routine so that history.push(imageBase.textureLocation) is moved to the post-write success path (or called in the write-success callback/promise resolution).
134-136:⚠️ Potential issue | 🟡 MinorRemove the leftover debug log.
Line 135 still prints the whole item during sorting when
a.nameis falsy. That adds noisy build output without affecting behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/build.ts` around lines 134 - 136, The sort comparator function sort(a: Item, b: Item) contains a leftover debug console.log(a) when a.name is falsy; remove that console.log and, if needed, handle missing names deterministically (e.g., treat empty/null names consistently before calling a.name.localeCompare(b.name)) so the comparator doesn't produce noisy build output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build/build.ts`:
- Around line 368-372: dedupImageNames is deriving image identity from
textureLocation by blindly splitting on '!' whereas saveImage() falls back to
the fileTime/MD5 identity when no '!hash' is present; update dedupImageNames
(the manifest.reduce that builds hashMap and the collision-resolution logic
around the block handling entries at the later collision code path) to reuse the
exact same identity logic as saveImage(): if textureLocation contains a '!' use
the suffix after '!' as the identity, otherwise compute/derive the fallback
identity (fileTime or MD5 fallback used by saveImage()) and use that for hashMap
lookup and for dedup checks so two items missing '!hash' are treated distinctly
unless their fallback identity matches. Ensure you reference and reuse the same
helper or logic that saveImage() uses for generating the fallback identity to
keep behavior consistent.
- Around line 404-410: The current fallback rename for collisions uses only
item.category causing different items in the same category to get identical
names; update the renaming logic in the loop that references seen, group,
mainItem, hashMap, imageName and item.imageName so the fallback includes an
item-specific discriminator (for example append a deterministic short token
derived from item.uniqueName or an index/sequence) before the extension — e.g.
compute a safe shortId from item.uniqueName (or use the loop index) and build
item.imageName as `${stem}${item.category}-${shortId}${ext}` (ensure the shortId
is filesystem-safe and deterministic so repeated runs are stable).
---
Duplicate comments:
In `@build/build.ts`:
- Line 43: The build currently crashes if warnings.json is missing; wrap the
readJson<Warnings>(new URL('../data/warnings.json', import.meta.url)) call in a
try/catch and, on failure (or specifically if the error indicates
file-not-found), set warnings to an empty Warnings object (e.g., {} as Warnings)
instead of throwing; update the code that references warnings to expect this
fallback.
- Around line 187-205: The current try/catch around the whole parent/item block
causes one nested failure to mark the entire parent as missing and skip
remaining assets; instead move error handling to each call to saveImage so
failures are caught per asset: wrap the calls to this.saveImage(item,...), each
this.saveImage(component,...), and each this.saveImage(ability...) in their own
try/catch; in each catch push the specific asset name into warnings.failedImage
(and only push to warnings.missingImage or set item.imageName = 'missing.png'
for the parent item when the parent image itself fails), and continue processing
other components/abilities so no nested asset is skipped.
- Around line 242-246: The code prematurely marks imageBase.textureLocation as
processed (history.push) before the download/write completes; change the flow so
you only add imageBase.textureLocation to history after the file is successfully
written (or write+verification succeeds) and ensure the early return that checks
history (the if that includes history.includes(imageBase.textureLocation))
remains but only short-circuits after a successful previous write; update the
logic around the handling of item.name ('Blueprint'/'Arcane') and the write
routine so that history.push(imageBase.textureLocation) is moved to the
post-write success path (or called in the write-success callback/promise
resolution).
- Around line 134-136: The sort comparator function sort(a: Item, b: Item)
contains a leftover debug console.log(a) when a.name is falsy; remove that
console.log and, if needed, handle missing names deterministically (e.g., treat
empty/null names consistently before calling a.name.localeCompare(b.name)) so
the comparator doesn't produce noisy build output.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3a9439e-58b7-43cc-8f8f-ac92297ffbfa
⛔ Files ignored due to path filters (2)
data/img/arcane.pngis excluded by!**/*.pngdata/img/blueprint.pngis excluded by!**/*.png
📒 Files selected for processing (4)
build/build.tsbuild/parser.tstest/index.spec.mjstest/utilities/find.spec.mjs
✅ Files skipped from review due to trivial changes (1)
- test/utilities/find.spec.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/index.spec.mjs
TobiTenno
approved these changes
Apr 28, 2026
SlayerOrnstein
force-pushed
the
simple-image-name
branch
from
247f768 to
d54b990
Compare
Signed-off-by: SlayerOrnstein <6075693+SlayerOrnstein@users.noreply.github.com>
Signed-off-by: SlayerOrnstein <6075693+SlayerOrnstein@users.noreply.github.com>
Signed-off-by: SlayerOrnstein <6075693+SlayerOrnstein@users.noreply.github.com>
SlayerOrnstein
force-pushed
the
simple-image-name
branch
from
d54b990 to
71137e2
Compare
Signed-off-by: SlayerOrnstein <6075693+SlayerOrnstein@users.noreply.github.com>
Signed-off-by: SlayerOrnstein <6075693+SlayerOrnstein@users.noreply.github.com>
Signed-off-by: SlayerOrnstein <6075693+SlayerOrnstein@users.noreply.github.com>
|
@SlayerOrnstein @TobiTenno can a new build be committed or will the bot handle it? The images are now broken until the newly merged PR takes effect? |
Member
Author
|
@ketoweb fixing it now |
Collaborator
|
🎉 This PR is included in version 1.1274.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
5 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.
What did you fix?
Reduce duplication by leaning more on the image manifest and hashes.
The idea is to just use the internal name of the image resource as the image name to reduce complexity in
addImageName. This also allowssaveImageto be smarter about reusing image names when a duplicate hash is found in thetextureLocation(hashes being the same means the image is the same, even if the filenames are different). This also allowssaveImageto properly handle situations where the filename is the same but the resource is different as is the case withEmberand her monotone glyph also being calledEmber.pngin these cases theItemname is used instead.Any updates or last minute changes by
saveImagewill also update the correspondingItemand push writing the json till after the images are downloaded.Reproduction steps
Evidence/screenshot/link to line
Considerations
Summary by CodeRabbit
Bug Fixes
Tests