Fix stretched icons #2316
Fix stretched icons #2316
Conversation
WalkthroughAdds exported async utility Changes
Sequence Diagram(s)sequenceDiagram
participant UI as RadialMenu / EventsDisplay
participant Util as Utils.getSvgAspectRatio
participant Cache as Cache (on function)
participant Net as network/fetch
UI->>Util: getSvgAspectRatio(src)
Util->>Cache: lookup(src)
alt cache hit
Cache-->>Util: ratio
else cache miss
Util->>Net: fetch(src) with timeout
Net-->>Util: svg text / error
alt fetched SVG
Util->>Util: parse viewBox or width/height -> compute ratio
Util->>Cache: store(src, ratio or null)
else fetch/parse failed
Util->>Cache: store(src, null)
end
end
Util-->>UI: ratio or null
UI->>UI: if ratio != 1 -> compute w/h and x/y -> update <img> attributes (center)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/client/Utils.ts (2)
262-262: Consider supporting single-quoted viewBox attributes.The regex
/viewBox="([^"]+)"/ionly matches double-quoted attributes. While double quotes are standard, some SVG files use single quotes (viewBox='...').If needed, update the regex to handle both:
- const vbMatch = text.match(/viewBox="([^"]+)"/i); + const vbMatch = text.match(/viewBox=["']([^"']+)["']/i);Similarly for width/height attributes at lines 276-277.
250-251: Function-level cache is unconventional but acceptable.Storing the cache on the function object itself (
self.svgAspectRatioCache) is an unconventional pattern in TypeScript. A module-levelMaporWeakMapwould be more idiomatic, but this approach works and avoids polluting the module scope.If you prefer a more conventional approach:
+const svgAspectRatioCache = new Map<string, number | null>(); + export async function getSvgAspectRatio(src: string): Promise<number | null> { - const self = getSvgAspectRatio as any; - self.svgAspectRatioCache ??= new Map(); - - const cached = self.svgAspectRatioCache.get(src); + const cached = svgAspectRatioCache.get(src); if (cached !== undefined) return cached;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/Utils.ts(1 hunks)src/client/graphics/layers/EventsDisplay.ts(3 hunks)src/client/graphics/layers/RadialMenu.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/client/graphics/layers/RadialMenu.ts (1)
src/client/Utils.ts (1)
getSvgAspectRatio(249-294)
src/client/graphics/layers/EventsDisplay.ts (1)
src/client/Utils.ts (1)
getSvgAspectRatio(249-294)
🪛 GitHub Check: 🔍 ESLint
src/client/graphics/layers/EventsDisplay.ts
[failure] 912-912:
'width' is assigned a value but never used.
🔇 Additional comments (3)
src/client/Utils.ts (1)
253-254: Caching null results prevents retry on transient failures.When
getSvgAspectRatioreturnsnulldue to a fetch error or parse failure, thatnullis not cached (line 254 checkscached !== undefined, and the function only caches when it successfully computes a ratio). This means transient failures will cause repeated fetches.However, if a malformed SVG is fetched successfully but produces
null(lines 288-293), the function will re-fetch it on every call. Consider whether cachingnullafter successful fetch but failed parse would improve performance.src/client/graphics/layers/RadialMenu.ts (2)
555-572: Icons may briefly display stretched before aspect ratio loads.The implementation creates the image with
iconSize × iconSizedimensions (lines 549-553), then asynchronously fetches the aspect ratio and adjusts dimensions (lines 555-572). If the aspect ratio is not yet cached, users may see a brief stretched icon before the correction applies.This is likely acceptable since:
- Subsequent loads will use the cached value
- The visual glitch is brief and self-correcting
However, if this is noticeable, consider pre-loading aspect ratios for known icons during
init(), similar to the suggestion for EventsDisplay.ts.
546-572: Async icon sizing logic is correctly implemented.The use of
.then()to handle the async aspect ratio adjustment is appropriate here. The icon is created with default dimensions and then updated once the aspect ratio is available. This pattern works well for this use case.
|
I was able to remove the async aspect ratio completely in EventDisplay.ts simply by not defining the class w-X in the buttons, and letting CSS do the hard work of finding the width. This approach won't work for RadialMenu.ts, because we need to find the largest side in the case. It could technically be hard coded to just use width too, as both the non square icons have a positive aspect ratio (they are wider than taller), but this seems quite short sighted. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/graphics/layers/EventsDisplay.ts(2 hunks)src/client/graphics/layers/RadialMenu.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/RadialMenu.ts
🔇 Additional comments (1)
src/client/graphics/layers/EventsDisplay.ts (1)
954-961: Good consolidation of duplicate code.The use of
renderToggleButtonsuccessfully eliminates repetitive inline HTML for the five category filter buttons. This makes the code cleaner and easier to maintain.Note: This approval is for the consolidation approach itself. See comments above regarding the dynamic Tailwind class issue and missing aspect ratio logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/client/Utils.ts (2)
249-254: Consider adding a cache size limit.The cache grows unbounded, which could accumulate memory if the app uses many unique SVG URLs over time. For a game with a fixed set of icons this is likely fine, but consider adding an LRU eviction policy or size limit if the icon set is dynamic or user-generated.
Example approach using a simple size limit:
const MAX_CACHE_SIZE = 100; const cache = self.svgAspectRatioCache; if (cache.size >= MAX_CACHE_SIZE) { const firstKey = cache.keys().next().value; cache.delete(firstKey); } cache.set(src, ratio);
292-294: Consider logging errors during development.Silent error handling could make it harder to debug issues with SVG files (CORS problems, malformed URLs, invalid SVG format). Consider adding a console.warn for development builds to help identify problems.
} catch (e) { - // fetch may fail due to CORS or non-SVG.. + // fetch may fail due to CORS or non-SVG + if (process.env.NODE_ENV === 'development') { + console.warn(`Failed to get SVG aspect ratio for ${src}:`, e); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/Utils.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: 🧪 CI
src/client/Utils.ts
[warning] 1-1: Code style issues found in the above file. Run 'npx prettier --write' to fix.
🔇 Additional comments (1)
src/client/Utils.ts (1)
256-262: Timeout implementation looks good.The AbortController with timeout correctly addresses the previous review concern about hanging requests. The cleanup with
clearTimeoutis handled properly.
|
Thanks for noticing the bug |
|
Looks like you may need to run |
Lavodan
dismissed stale reviews from evanpelle and coderabbitai[bot]
via
f9a2e45
|
Sorry for the format issue again >.< |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/Utils.ts (1)
281-293: Fallback parsing handles typical SVG width/height formats.The
parseNumhelper correctly strips units (e.g., "px") and the validation catches any malformed values. The implementation works well for standard SVG files.Optional improvement: Check the response
Content-Typeheader to confirm it's an SVG before parsing, which would provide clearer errors if non-SVG URLs are passed.Optional: Add Content-Type validation
const resp = await fetch(src, { signal: controller.signal }); clearTimeout(timeoutId); if (!resp.ok) throw new Error(`Fetch failed: ${resp.status}`); + const contentType = resp.headers.get('content-type'); + if (contentType && !contentType.includes('svg')) { + throw new Error('Not an SVG file'); + } const text = await resp.text();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/Utils.ts(1 hunks)
🔇 Additional comments (4)
src/client/Utils.ts (4)
249-254: Clean cache pattern that matches the codebase style.The function-scoped cache approach is consistent with
translateTextabove and provides good performance for repeated lookups of the same SVG. The early return keeps the code clean.
256-262: Timeout implementation looks solid.The AbortController pattern with a 5-second timeout prevents hanging requests and the timeout is properly cleared after completion. This addresses the reliability concern from previous reviews.
264-279: ViewBox parsing correctly handles SVG spec variations.The regex split pattern
/[\s,]+/properly supports both space and comma separators per the SVG specification, and the validation ensures positive numeric dimensions before computing the ratio.
295-300: Error handling supports graceful degradation.Returning
nullon any fetch or parse failure allows callers to apply fallback styling when aspect ratios can't be determined, which aligns with the progressive enhancement approach for icon rendering.
Good shout! |
4 participants
Description:
Fixes #2257 (well, a part of it)
This PR implements a new async helper function in client Utils.ts ( getSvgAspectRatio), which caches and then retrieves aspect ratios for Svg files. (I am not married to it being in Utils.ts, I just couldn't find a better place to put it, and it seemed like something that should be accessible to many files).
It then implements this helper in RadialMenu.ts to fix incorrect aspect ratios. It uses the default value for the smaller side, and the computed value for the larger side.
Before fixing the stretching in EventsDisplay.ts, this PR first consolidates the hard coded html for toggle buttons into a helper function (renderToggleButton). Afterwards, it adds width calculation to this helper function using getSvgAspectRatio.
Potential flaws
Screenshots
Before
After
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Lavodan