fix(markdown): stop inline badge image overlap (SVG / unknown intrinsic size)#643
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:
WalkthroughInline image sizing now uses a transformer-provided placeholderConfig that returns badge defaults for unspecified intrinsic sizes or a density-/container-aware, aspect-ratio-preserving placeholder capped by a max height; block-level images use a shared clipped full-width modifier with a 600.dp max height and clickable applied there when linked. ChangesImage Placeholder & Block Modifier
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownImageTransformer.kt (1)
130-145: ⚡ Quick winAlign the height-cap docs with the actual constant.
The KDoc says “height capped at 200sp”, but the implementation caps at
MAX_INLINE_HEIGHT_PX = 240f(240px). Please update wording so future tuning/debugging uses the right unit/value.Proposed doc-only fix
- * height capped at 200sp so a stray hero image doesn't dominate. + * height capped at 240px so a stray hero image doesn't dominate.- // Cap inline image height when intrinsic size IS known so a + // Cap inline image height (in px) when intrinsic size IS known so a // stray hero image inside a paragraph can't blow up the line.Also applies to: 222-224
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownImageTransformer.kt` around lines 130 - 145, The KDoc above the MarkdownImageTransformer logic incorrectly states “height capped at 200sp” while the implementation uses MAX_INLINE_HEIGHT_PX = 240f (240px); update the KDoc to match the actual constant/value and unit (e.g., “height capped at 240px” or clarify the px vs sp unit) so documentation aligns with the code (search for MAX_INLINE_HEIGHT_PX and the KDoc block in MarkdownImageTransformer.kt and edit the comment text accordingly, including the other occurrence around lines 222-224).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownImageTransformer.kt`:
- Around line 130-145: The KDoc above the MarkdownImageTransformer logic
incorrectly states “height capped at 200sp” while the implementation uses
MAX_INLINE_HEIGHT_PX = 240f (240px); update the KDoc to match the actual
constant/value and unit (e.g., “height capped at 240px” or clarify the px vs sp
unit) so documentation aligns with the code (search for MAX_INLINE_HEIGHT_PX and
the KDoc block in MarkdownImageTransformer.kt and edit the comment text
accordingly, including the other occurrence around lines 222-224).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c84d19b6-2133-4d88-acad-f50c435f74d9
📒 Files selected for processing (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownImageTransformer.kt
Greptile SummaryThis PR fixes inline badge overlap in markdown README rendering by combining two strategies: a pre-processing pass (
Confidence Score: 4/5Safe to merge for the badge-overlap fix; one path needs attention before inline raster images in paragraphs behave correctly. The placeholderConfig caps all inline-image placeholder heights at BADGE_MAX_HEIGHT_DP (40 dp), but raster inline images embedded in a text paragraph have their modifier capped at RASTER_MAX_HEIGHT_DP (320 dp). The 8x discrepancy means a raster image genuinely inline in a paragraph will overflow its reserved text-flow slot. The badge-overlap fix itself is sound and block-level images are unaffected. MarkdownImageTransformer.kt — the placeholderConfig known-size branch uses the badge height constant as a universal cap, which does not match the raster modifier cap. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Raw Markdown] --> B[applyThemeAwareImages]
B --> C[separateAdjacentImageLinks]
C --> D{Badge row on separate line?}
D -- Yes --> E[Blank line inserted]
D -- No --> F[Line passed through]
E --> G[splitMarkdownIntoChunks]
F --> G
G --> H[MarkdownText renderer]
H --> I{Image node type}
I -- Block-level --> J[LinkAwareMarkdownImage]
I -- Inline in paragraph --> K[InlineContent slot]
J --> L[blockModifier fillMaxWidth + 600dp + clip]
K --> M[placeholderConfig cap 40dp]
K --> Q[inlineModifier via ImageData]
Q --> R{looksLikeBadge?}
R -- Yes --> S[heightIn 40dp / widthIn 220dp]
R -- No --> T[heightIn 320dp / widthIn 480dp]
Reviews (7): Last reviewed commit: "fix(markdown): unify placeholder + image..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/markdown/GithubStoreMarkdownComponents.kt (1)
78-85: ⚡ Quick winExtract
blockModifierto top-level constant.Since
blockModifieris a fixed configuration that doesn't depend on composable state, extracting it to a top-level constant would clarify intent and avoid recreating it on every recomposition.♻️ Proposed refactor
Move the modifier to top-level, just after the imports:
import org.intellij.markdown.ast.ASTNode +private val blockImageModifier = androidx.compose.ui.Modifier + .fillMaxWidth() + .heightIn(max = 600.dp) + .clipToBounds() + // Plain (non-@Composable) factory so callers can wrap in `remember(isDark)`Then use it in the function:
- // Block-level images get their own layout modifier (fillMaxWidth + - // height cap + clip). The transformer's `imageData.modifier` is - // intentionally empty so inline-content rendering stays bounded by - // the shared `Placeholder` slot — see kdoc on `MarkdownImageTransformer`. - val blockModifier = androidx.compose.ui.Modifier - .fillMaxWidth() - .heightIn(max = 600.dp) - .clipToBounds() - if (outerHref != null) { val uriHandler = LocalUriHandler.current Image( painter = imageData.painter, contentDescription = imageData.contentDescription, - modifier = blockModifier.clickable { + modifier = blockImageModifier.clickable {} else { Image( painter = imageData.painter, contentDescription = imageData.contentDescription, - modifier = blockModifier, + modifier = blockImageModifier,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/markdown/GithubStoreMarkdownComponents.kt` around lines 78 - 85, The local val blockModifier inside GithubStoreMarkdownComponents.kt should be extracted to a top-level constant (e.g., BLOCK_IMAGE_MODIFIER) so it isn’t recreated on every recomposition; create a top-level immutable val immediately after the imports with the same Modifier chain (.fillMaxWidth().heightIn(max = 600.dp).clipToBounds()) and replace the local blockModifier reference in the function with that new constant (ensure the symbol name matches usages and keep MarkdownImageTransformer behavior unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/markdown/GithubStoreMarkdownComponents.kt`:
- Around line 78-85: The local val blockModifier inside
GithubStoreMarkdownComponents.kt should be extracted to a top-level constant
(e.g., BLOCK_IMAGE_MODIFIER) so it isn’t recreated on every recomposition;
create a top-level immutable val immediately after the imports with the same
Modifier chain (.fillMaxWidth().heightIn(max = 600.dp).clipToBounds()) and
replace the local blockModifier reference in the function with that new constant
(ensure the symbol name matches usages and keep MarkdownImageTransformer
behavior unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 266fe02f-b9cf-4f00-9589-3cf6c20b476e
📒 Files selected for processing (2)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/markdown/GithubStoreMarkdownComponents.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownImageTransformer.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownImageTransformer.kt
… as belt-and-braces
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownImageTransformer.kt`:
- Around line 181-186: The current image sizing computes targetWidth from
intrinsicImageSize and then clamps targetHeight, which can break the aspect
ratio; in MarkdownImageTransformer (the code that computes
targetWidth/targetHeight) recompute width after applying the max-height clamp:
compute ratio = intrinsicImageSize.height /
intrinsicImageSize.width.coerceAtLeast(1f), compute tentative targetWidth =
minOf(intrinsicImageSize.width, containerWidthPx), compute targetHeight =
(tentativeTargetWidth * ratio).coerceAtMost(MAX_INLINE_HEIGHT_PX), then if
targetHeight was clamped recalc targetWidth = targetHeight / ratio (or simply
set targetWidth = minOf(tentativeTargetWidth, targetHeight / ratio) ) so the
returned values (targetWidth.toSp().value and targetHeight.toSp().value)
preserve the intrinsic aspect ratio and avoid excess horizontal space.
- Line 129: The placeholder reserves up to MAX_INLINE_HEIGHT_PX (240 px) but the
rendered image uses INLINE_MAX_HEIGHT_DP (40 dp), causing mismatched heights on
high-density screens; update placeholderConfig() to convert MAX_INLINE_HEIGHT_PX
into dp at runtime using the current Density (e.g., LocalDensity.current) and
use that dp value so both the placeholder and the image modifier =
Modifier.heightIn(max = INLINE_MAX_HEIGHT_DP.dp) use the same visual max height;
ensure you reference INLINE_MAX_HEIGHT_DP and MAX_INLINE_HEIGHT_PX and perform
the conversion once (px -> dp) and pass the dp into the placeholder
configuration.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31dbf706-604c-483f-bf59-916b9911aa87
📒 Files selected for processing (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownImageTransformer.kt
| // Derive both bounds from a single dp constant so the | ||
| // placeholder cap, the image-modifier cap, and the unit | ||
| // (dp, not raw px) all stay in lock-step across screen | ||
| // densities. Mixing px here with dp in the image | ||
| // modifier caused a ~7× mismatch on 3× density screens. | ||
| val maxHeightPx = BADGE_MAX_HEIGHT_DP.dp.toPx() | ||
| val targetHeight = rawHeight.coerceAtMost(maxHeightPx) | ||
| // Preserve aspect ratio: when height was clamped, scale | ||
| // the width down by the same factor so the placeholder | ||
| // box matches the actual rendered image shape — avoids | ||
| // excess horizontal whitespace and badge re-wrapping. | ||
| val targetWidth = if (rawHeight > maxHeightPx && ratio > 0f) { | ||
| targetHeight / ratio | ||
| } else { | ||
| initialWidth | ||
| } | ||
| targetWidth.toSp().value to targetHeight.toSp().value |
There was a problem hiding this comment.
placeholderConfig always caps targetHeight at BADGE_MAX_HEIGHT_DP.dp.toPx() (40 dp) for every image with a known intrinsic size. But the inlineModifier for raster images uses RASTER_MAX_HEIGHT_DP = 320 dp. When a raster image appears genuinely inline within a paragraph — Some text  more text — the text-flow slot is reserved at 40 dp while the image's own modifier allows up to 320 dp, so the image overflows the paragraph's placeholder vertically. The fix is to use RASTER_MAX_HEIGHT_DP.dp.toPx() as the cap in placeholderConfig (matching the largest possible modifier cap), which still produces a well-bounded slot for badge images since their modifier independently limits them to 40 dp anyway.
There was a problem hiding this comment.
rainxchzed has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
rainxchzed has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Summary
Stop the badge-row overlap seen on README headers (
[](url) [](url)). The previous behavior inherited the lib's default per-paragraphplaceholderConfigofSize(0, 180sp)whenever the FIRST image had no intrinsic size (typical for SVG before Coil decodes), so every following badge in the sameMarkdownTextran with the same 180sp tall slot and collided horizontally.Fix
Override
ImageTransformer.placeholderConfig:120sp × 32sp) so badges tile cleanly on a single line.Centerso adjacent badges align on a shared baseline.Block-level images (rendered via our custom
LinkAwareMarkdownImage) are unaffected — they already cap atheightIn(max = 600.dp).Test plan
kamgurgul/cpu-infoREADME. Badge row at the top tiles horizontally without overlap.Summary by CodeRabbit