fix(icons): cache one source per icon, retry rate limits, stop panicking - #390
Merged
Merged
Conversation
Three faults, one cause each.
**One download per colour and per size.** The disk entry was named
`{slug}-{color}-{w}x{h}.svg` because the Iconify URL baked colour and size into
the SVG. A cache could hold 21 copies of `lucide:sparkles`, and changing only a
colour sent an icon that rendered fine offline back to the network. The source
is now fetched once per `prefix:name`, cached under `{slug}.svg`, and recoloured
and resized locally — Lucide draws with `stroke="currentColor"`, so recolouring
is a substitution, and resizing rewrites the root `width`/`height` while leaving
the `viewBox` alone. Done without a regex crate: the attribute rewrite is a
short scan of the root tag.
**No retry.** A single `GET`, so a `429` from `api.iconify.design` under
parallel renders lost the icon. Four attempts with exponential backoff, and only
for `429` and `5xx` — a `404` is not going to resolve on the fourth try, and
retrying it would make a typo take four times as long to report.
**`render` panicked where `still` and `sheet` warned.** The panic was not an
oversight: `an_unresolvable_icon_must_fail_the_preload_not_be_swallowed` exists
to enforce it, and says "must panic (or otherwise hard-fail)". So the severity
stays and the mechanism changes. `prefetch_icons` returns a named
`IconsUnresolved` error, the encode paths propagate it, and `still` and `sheet`
now preload too, so all three fail the same way with the same message and a
clean exit code instead of a backtrace. The studio warns and keeps its preview
alive, which is the one caller that must not die. `validate` stays offline.
$ rustmotion still -f missing-icon.json --time 0 -o out.png
Error: 1 icon(s) could not be loaded — checked the disk cache at
~/.cache/rustmotion/icons and the network, both failed:
- 'lucide:no-such-icon-xyz' (color #2A2F6B, target 84x84px):
Failed to fetch icon: http status: 404
[exit 1]
`disk_cache_is_keyed_by_icon_color_and_size` asserted the first fault as a
guarantee — "different colors must not share a cache file". It is replaced by
its opposite rather than worked around.
Closes #365
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
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.
Closes #365. Refs #388.
Six of the ten studies in the issue lost icons while rendering in parallel; most ended up inlining the Lucide sources by hand.
One download per colour and per size
The disk entry was
{slug}-{color}-{w}x{h}.svg, because the Iconify URL baked colour and size into the SVG. A cache could hold 21 copies oflucide:sparkles, and changing only a colour sent an icon that rendered fine offline back to the network.The source is now fetched once per
prefix:nameand recoloured and resized locally. Lucide draws withstroke="currentColor", so the recolour is a substitution; the resize rewrites the rootwidth/heightand leaves theviewBoxalone, which is the glyph's own coordinate space.No regex crate was added for this — the attribute rewrite is a short scan of the root tag.
No retry
A single
GET. Four attempts with exponential backoff now, and deliberately only for429and5xx:A
404is not going to resolve on the fourth try, and retrying it would make a typo take four times as long to report.The panic was deliberate, so the severity stays and the mechanism changes
I first downgraded it to a warning to match
stillandsheet. That broke an existing test:That is explicit evidence of intent, and it settles which of the issue's two options to take — it offers "either all fail with a named error, or all warn", and the codebase had already chosen. "or otherwise hard-fail" is the part that leaves the mechanism open, and a panic is the wrong one: it prints a backtrace-style message for what is user input, and cannot be handled by a caller.
So
prefetch_iconsreturns a namedIconsUnresolvederror:renderstillsheetvalidateThe studio is the one caller that must not die on a bad icon, so it warns and continues — which is also why returning an error beats panicking: the caller decides.
A test that asserted the bug
disk_cache_is_keyed_by_icon_color_and_size— "different colors must not share a cache file" — encoded the first fault as a guarantee. It is replaced by its opposite (disk_cache_is_keyed_by_the_icon_alone) rather than worked around, and the two other tests that built paths with the old helper were migrated.Verification
Eight tests over the cache and rewrite, plus the rewritten preload test.
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace(1590) all clean.Not done
The issue also suggests
validate --offlineand arustmotion icons prefetchsubcommand. Neither is here: the first needs a policy decision about whatvalidateis allowed to touch (#320 is about it not reaching the network), and the second is a new command rather than a fix. Worth their own issues if wanted.Written comment-free, per the codebase-wide rule from #345.