-
Notifications
You must be signed in to change notification settings - Fork 62
fix: add missing images #830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughCache hash values are cleared in two JSON files: Manifest.hash in the export cache and a specific item's hash in the images cache. No structural changes occur; only hash fields are reset to empty strings. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
|
It seems there's a bunch of images missing i'm building it now, up to 168 |
|
There's more problems with the images than i thought, there might need one or two prs to fix related issues. |
4863f7a to
b91ea24
Compare
| import nodePath from 'path'; | ||
| import { resolve } from 'node:path'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, you should import join, not use a differently scoped path, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure it'll stay like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also importing join is not that good as join by itself doesnt have much meaning. Join arrays, strings, objects, everything? path.join is better because you know what it's joining.
| // 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; | ||
| // if (!hashManager.hasChanged('Manifest')) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should definitely not be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree but for testing purposes it's better this way, im using the draft as a way to fast commit the changes im making even if theyre not going to be there at the end
|
|
||
| this.updateCache(item, cached, hash, isComponent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't seem like this actually does anything. if you'd like to explain your intention, feel free, but none of the params are modified by moving this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sometimes the cache is updated before saving the file, then image is missing in the next runs. for some reason sharp generates an error on some images some of the time, so only updating the cache when it's saved to disk is better
|
This draft is still in design phase, so no need for the code be compliant with lint or to be well designed. Remember this is basically me testing around on what we can do about the problem. I could very much discard all changes. |
What did you fix?
Add missing images
Considerations
For some reason when building it also update many entries of itemCount and parents in the All.json file. We should probably change the code to check if all the entries in the cache are in fact stored in disk.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.