feat: dynamically size thumbnail to fill remaining space#162
feat: dynamically size thumbnail to fill remaining space#162
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:
📝 WalkthroughWalkthroughBackground image rendering was refactored to use dynamic layout: horizontal padding increased, text block height is measured via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BackgroundImage
participant TextHeight
participant Canvas
Caller->>BackgroundImage: backgroundImage(props)
Note right of BackgroundImage: compute maxWidth, prepare title/lines/thumb
BackgroundImage->>TextHeight: textHeight(context, maxWidth, title, lines)
TextHeight-->>BackgroundImage: modTextHeight
BackgroundImage->>Canvas: draw background
BackgroundImage->>Canvas: draw thumbnail at tracked position (using modTextHeight)
BackgroundImage->>Canvas: draw mod name at computed position
BackgroundImage->>Canvas: wrapText & draw description lines
BackgroundImage-->>Caller: return final image
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/utils.ts (1)
214-232: Consider parameterizing or synchronizing line spacing.
The function currently uses a hardcoded bottom line spacing of 15, while in other parts of the code (e.g.,src/drawers.ts), there's a separate line spacing of 15 as well. It might be beneficial to unify these spacing values to avoid mismatches or to consider parameterizing them, so that future adjustments don’t require scattered changes.src/drawers.ts (2)
17-17: Correct the spelling of “horizontalPad.”
Please consider renaminghorizantalPadtohorizontalPadfor clarity and consistency.-export const horizantalPad = 8; +export const horizontalPad = 8;
167-187: Unify text spacing between measurement and drawing.
You’re adding manual line spacing of 15 here, but the total text height is derived elsewhere using ascent and descent metrics (plus a constant). Consider consolidating the spacing logic to keep the visual spacing consistent and reduce potential discrepancies between measured and rendered text spacing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/drawers.ts(2 hunks)src/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/drawers.ts (1)
src/utils.ts (4)
modDescription(121-138)textHeight(215-232)textColor(179-183)wrapText(140-159)
🔇 Additional comments (2)
src/drawers.ts (2)
13-13: Looks good.
ImportingtextHeightis consistent with its usage. No concerns here.
151-155:Details
❓ Verification inconclusive
Ensure large text doesn’t cause layout problems.
modTextHeightcan potentially become very large for lengthy descriptions, which could shrink or overlap content unexpectedly. Double-check how large text is handled upstream to confirm that it won’t disrupt your layout.
🏁 Script executed:
#!/bin/bash # Searching for calls to `backgroundImage` to see if there's a limit on description sizes rg -A 3 $'backgroundImage'Length of output: 801
Verify that extremely long mod descriptions are safely handled.
The calculation ofmodTextHeightinsrc/drawers.ts(lines 151–155) may yield very large values when processing lengthy descriptions, which risks shrinking or overlapping content in the layout. Our search for references tobackgroundImagein bothsrc/generator.tsandsrc/drawers.tsdid not reveal any explicit limits or safeguards on the description’s size upstream. Please verify that thetextHeightlogic and any associated layout mechanisms (especially wherebackgroundImageis used) adequately constrain or adjust for oversized text to maintain a robust design.
SlayerOrnstein
changed the title
SlayerOrnstein
force-pushed
the
feat-dynamic-thumb
branch
from
5b6f634 to
ab6c52c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/drawers.ts (2)
4-13:⚠️ Potential issue | 🔴 Critical
textHeightis missing from the imports — will fail to compile.
textHeightis called at line 153 but is not imported from'./utils.js'.🐛 Proposed fix
import { fetchModPiece, fetchPolarity, flip, getTier, modDescription, modRarityMap, textColor, + textHeight, wrapText, } from './utils.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawers.ts` around lines 4 - 13, The file is missing the textHeight import from ./utils.js which causes a compile error where textHeight is used (around line where textHeight(...) is called); update the import list at the top (the existing import that brings in fetchModPiece, fetchPolarity, flip, getTier, modDescription, modRarityMap, textColor, wrapText) to also import textHeight from './utils.js' so the symbol is available to the function that measures text height.
216-240:⚠️ Potential issue | 🔴 CriticalOld static rendering code not removed — root cause of all four CI failures and double-draws mod name + description.
The new dynamic rendering logic (lines 167–188) supersedes this block entirely. Leaving it in redeclares
description(line 224),lines(line 227), andmaxWidth(line 233) in the same function scope, causing all four biome lint errors. It also drawsmod.namea second time at the fixed positioncanvas.height * 0.52and re-renders the description starting atcanvas.height * 0.56, producing overlapping artifacts.🐛 Proposed fix — remove the stale block
- context.fillStyle = textColor(tier); - - context.textAlign = 'center'; - context.textBaseline = 'middle'; - context.font = '400 16px "Roboto"'; - - context.fillText(mod.name, canvas.width * 0.5, canvas.height * 0.52); - - const description = modDescription(mod.description, mod.levelStats, rank ?? 0); - if (description && description.length > 0) { - const x = canvas.width * 0.5; - const lines = description.split('\n'); - - context.font = '12px "Roboto"'; - let start = canvas.height * 0.56; - - lines.forEach((line) => { - const maxWidth = background.width * 0.8; - const texts = wrapText(context, line, maxWidth); - texts.forEach((text) => { - context.fillText(text, x, start, maxWidth); - start += 15; - }); - }); - } - return loadImage(await canvas.encode('png'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawers.ts` around lines 216 - 240, This block duplicates the new dynamic rendering and redeclares variables (description, lines, maxWidth) and causes double-drawing of the mod name and description; remove the entire stale static rendering block starting at the context.fillStyle = textColor(tier); ... through the closing brace that handles description rendering so only the new dynamic rendering (lines ~167–188) remains, ensuring you keep references to modDescription(...) and wrapText(...) only in the new code path and do not re-declare description/lines/maxWidth in the same function scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/drawers.ts`:
- Line 153: textHeight is being called before the canvas font is set so
measurements use the default font; fix by setting the description/body font on
the canvas context before computing modTextHeight (i.e., move or add the
context.font assignment used for descriptions so it runs prior to the call to
textHeight), ensuring the measured height matches the rendered description;
reference functions/variables: textHeight, modTextHeight, and the context.font
assignment that currently sets "400 16px Roboto" (adjust to the description font
used when rendering).
---
Outside diff comments:
In `@src/drawers.ts`:
- Around line 4-13: The file is missing the textHeight import from ./utils.js
which causes a compile error where textHeight is used (around line where
textHeight(...) is called); update the import list at the top (the existing
import that brings in fetchModPiece, fetchPolarity, flip, getTier,
modDescription, modRarityMap, textColor, wrapText) to also import textHeight
from './utils.js' so the symbol is available to the function that measures text
height.
- Around line 216-240: This block duplicates the new dynamic rendering and
redeclares variables (description, lines, maxWidth) and causes double-drawing of
the mod name and description; remove the entire stale static rendering block
starting at the context.fillStyle = textColor(tier); ... through the closing
brace that handles description rendering so only the new dynamic rendering
(lines ~167–188) remains, ensuring you keep references to modDescription(...)
and wrapText(...) only in the new code path and do not re-declare
description/lines/maxWidth in the same function scope.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/drawers.tssrc/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils.ts
SlayerOrnstein
changed the title
SlayerOrnstein
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/drawers.ts (1)
151-154:⚠️ Potential issue | 🟠 Major
textHeightstill called beforecontext.fontis set — measurements use the default"10px sans-serif".
context.fontis first assigned at line 171 ('400 16px "Roboto"') and 178 ('12px "Roboto"'). AllmeasureTextcalls insidetextHeighttherefore operate on the default font, underestimatingmodTextHeightand causingthumbHeightto be larger than expected, which can push description text behind the thumbnail.🔧 Minimal fix — set body font before the call
+ context.font = '12px "Roboto"'; const modTextHeight = textHeight(context, maxWidth, mod.name, lines);For a fully accurate title measurement,
textHeightinsrc/utils.tscould accept a separatetitleFontparameter and set it on the context before measuring the title, but that is a broader change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawers.ts` around lines 151 - 154, The measured mod text height is computed before the canvas font is set, so textHeight(context, ...) uses the default font and underestimates heights; fix by ensuring context.font is set to the intended body/title font before calling textHeight (or update textHeight to accept a font parameter and set context.font inside it) — locate the call to textHeight in this file (modTextHeight = textHeight(context, maxWidth, mod.name, lines)) and either set context.font = '400 16px "Roboto"' (or the appropriate font used later) immediately before that call or change textHeight signature to accept a titleFont and set context.font there prior to measurements.
🧹 Nitpick comments (2)
src/drawers.ts (2)
179-188:lineSpacing = 15in the rendering loop doesn't matchtextHeight's per-line metric.
textHeight(src/utils.ts:194) accumulatesactualBoundingBoxAscent + actualBoundingBoxDescentper wrapped line (≈ 11–13 px for 12 px Roboto), while the rendering loop below advancesstartby a fixedlineSpacing = 15. The mismatch grows with line count: for an N-line description the measured space is roughlyN×12 + 15px, but the rendered space isN×15px. The two happen to meet near 5 lines, so for typical short Warframe mod descriptions this leaves only a small visual gap rather than an overlap. Aligning the per-line accumulation intextHeightto use the samelineSpacingconstant would make the two functions consistent.♻️ Option — use a shared lineSpacing constant
In
src/utils.ts:+ export const LINE_SPACING = 15; export const textHeight = (context: SKRSContext2D, maxWidth: number, title: string, lines?: string[]): number => { - const bottomLineSpacing = 15; + const bottomLineSpacing = LINE_SPACING; const titleMetrics = context.measureText(title); let height = titleMetrics.actualBoundingBoxAscent + titleMetrics.actualBoundingBoxDescent; if (lines) { lines.forEach((line) => { const text = wrapText(context, line, maxWidth); text.forEach((t) => { - const metrics = context.measureText(t); - height += metrics.actualBoundingBoxAscent + metrics.actualBoundingBoxDescent; + height += LINE_SPACING; }); }); } return height + bottomLineSpacing; };In
src/drawers.ts:+ import { ..., LINE_SPACING, ... } from './utils.js'; ... - const lineSpacing = 15; + const lineSpacing = LINE_SPACING;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawers.ts` around lines 179 - 188, The rendering loop in drawText uses a hardcoded lineSpacing = 15 which diverges from the per-line metric calculated by textHeight (function textHeight in src/utils.ts); make the two consistent by extracting a shared lineSpacing constant (or exported function/constant) and use it in both drawers.ts (where start is advanced after context.fillText in the wrapText loop) and in textHeight so per-line accumulation matches the actual render step; update references to the variable in the wrapText usage and textHeight implementation to import/use that shared constant.
151-160:maxWidthandthumbWidthuse different formulas — only coincidentally equal at W = 160.
maxWidth = background.width * 0.9whilethumbWidth = canvas.width - horizantalPad * 2. They diverge for any background image width other than 160 px, so text wrapping width and thumbnail width won't match for other frame sizes.♻️ Unify the formulas
- const maxWidth = background.width * 0.9; + const maxWidth = canvas.width - horizantalPad * 2; const description = modDescription(mod.description, mod.levelStats, rank ?? 0); const lines = description?.split('\n'); const modTextHeight = textHeight(context, maxWidth, mod.name, lines); let position = canvas.height * 0.17; if (mod.imageName || image) { const thumb = await loadImage(image ?? `https://cdn.warframestat.us/img/${mod.imageName}`); - const thumbWidth = canvas.width - horizantalPad * 2; + const thumbWidth = maxWidth;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawers.ts` around lines 151 - 160, The text wrapping width and thumbnail width are calculated differently which causes mismatch; make them use the same formula by computing a single maxWidth based on the rendered canvas frame and horizontal padding (e.g., replace maxWidth = background.width * 0.9 with maxWidth = canvas.width - horizantalPad * 2) and then set thumbWidth = maxWidth (remove the separate canvas.width expression). Update uses of maxWidth (e.g., in textHeight and when sizing the thumb before loadImage) so both text wrapping and thumbnail sizing reference the same variable (maxWidth/thumbWidth) — changes affect maxWidth, thumbWidth, textHeight(context, ...), and the image sizing logic around loadImage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/drawers.ts`:
- Around line 151-154: The measured mod text height is computed before the
canvas font is set, so textHeight(context, ...) uses the default font and
underestimates heights; fix by ensuring context.font is set to the intended
body/title font before calling textHeight (or update textHeight to accept a font
parameter and set context.font inside it) — locate the call to textHeight in
this file (modTextHeight = textHeight(context, maxWidth, mod.name, lines)) and
either set context.font = '400 16px "Roboto"' (or the appropriate font used
later) immediately before that call or change textHeight signature to accept a
titleFont and set context.font there prior to measurements.
---
Nitpick comments:
In `@src/drawers.ts`:
- Around line 179-188: The rendering loop in drawText uses a hardcoded
lineSpacing = 15 which diverges from the per-line metric calculated by
textHeight (function textHeight in src/utils.ts); make the two consistent by
extracting a shared lineSpacing constant (or exported function/constant) and use
it in both drawers.ts (where start is advanced after context.fillText in the
wrapText loop) and in textHeight so per-line accumulation matches the actual
render step; update references to the variable in the wrapText usage and
textHeight implementation to import/use that shared constant.
- Around line 151-160: The text wrapping width and thumbnail width are
calculated differently which causes mismatch; make them use the same formula by
computing a single maxWidth based on the rendered canvas frame and horizontal
padding (e.g., replace maxWidth = background.width * 0.9 with maxWidth =
canvas.width - horizantalPad * 2) and then set thumbWidth = maxWidth (remove the
separate canvas.width expression). Update uses of maxWidth (e.g., in textHeight
and when sizing the thumb before loadImage) so both text wrapping and thumbnail
sizing reference the same variable (maxWidth/thumbWidth) — changes affect
maxWidth, thumbWidth, textHeight(context, ...), and the image sizing logic
around loadImage.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
assets/readme/Afterburn.pngis excluded by!**/*.pngassets/readme/Archgun_Riven_Mod.pngis excluded by!**/*.pngassets/readme/Primed_Flow.pngis excluded by!**/*.pngassets/readme/Steel_Charge.pngis excluded by!**/*.pngassets/readme/Vitality.pngis excluded by!**/*.png
📒 Files selected for processing (2)
README.mdsrc/drawers.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
2 participants
What did you fix?
Noticed this is how it is done in-game plus it gets rid of some of the dead space for one liners
Reproduction steps
Evidence/screenshot/link to line
Considerations
Summary by CodeRabbit
New Features
Refactor
Documentation