feat(markdown): native SVG decoder + theme-aware images + browser UA#618
Conversation
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.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (21)
WalkthroughThis PR adds native SVG rendering and theme-aware image handling to the GitHub Store app. It integrates Coil 3 for image loading with SVG support, implements a utility to process GitHub's ChangesSVG and Theme-Aware Image Rendering
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 docstrings
🧪 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 |
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 (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownImageTransformer.kt (1)
35-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle protocol-relative URLs before scheme validation.
Lines 44-47 currently reject
//...image links, which are still encountered in markdown content. Normalize them tohttps://first so they can render.Suggested patch
- val normalizedLink = + val normalizedLink = if (link.contains("github.com") && link.contains("/blob/")) { link .replace("github.com", "raw.githubusercontent.com") .replace("/blob/", "/") } else { link } + val requestLink = + if (normalizedLink.startsWith("//")) { + "https:$normalizedLink" + } else { + normalizedLink + } - if (!normalizedLink.startsWith("http://") && - !normalizedLink.startsWith("https://") && - !normalizedLink.startsWith("data:") + if (!requestLink.startsWith("http://") && + !requestLink.startsWith("https://") && + !requestLink.startsWith("data:") ) { return null } @@ - .data(normalizedLink) + .data(requestLink)Also applies to: 53-55
🤖 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 35 - 49, The scheme validation currently rejects protocol-relative URLs like "//host/path"; update the normalization flow in MarkdownImageTransformer (where normalizedLink is computed) to detect links starting with "//" and prepend "https:" before performing the github->raw conversion and the subsequent startsWith checks, and apply the same adjustment to the second validation block around lines 53-55 so both checks accept protocol-relative URLs after normalization.
🤖 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
`@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/util/applyThemeAwareImages.kt`:
- Around line 14-27: The current replacement returns "" for mismatched themed
images which can leave a leftover linking wrapper like [](...); update
the replacement logic in applyThemeAwareImages (the processed.replace call and
its regex/replacement) to detect and remove an optional surrounding markdown
link when dropping an inner image: either extend the Regex to optionally capture
a surrounding "[ ... ](...)" wrapper and return an empty string for the whole
wrapper, or in the lambda inspect characters/substring around the matched span
(using processed and match.range) to remove the enclosing "[...](...)" if
present; ensure you still strip the fragment and rebuild the image URL for kept
matches (using match.groupValues[1..4] / the same alt/urlBase/trailing
variables).
---
Outside diff comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownImageTransformer.kt`:
- Around line 35-49: The scheme validation currently rejects protocol-relative
URLs like "//host/path"; update the normalization flow in
MarkdownImageTransformer (where normalizedLink is computed) to detect links
starting with "//" and prepend "https:" before performing the github->raw
conversion and the subsequent startsWith checks, and apply the same adjustment
to the second validation block around lines 53-55 so both checks accept
protocol-relative URLs after normalization.
🪄 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: 82d53420-8dfc-4ee8-8a40-4276897016d3
📒 Files selected for processing (21)
composeApp/build.gradle.ktscomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/Main.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/util/applyThemeAwareImages.ktcore/presentation/src/commonMain/composeResources/files/whatsnew/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ar/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/bn/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/es/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/fr/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/hi/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/it/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ja/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ko/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/pl/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ru/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/tr/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/zh-CN/18.jsonfeature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/utils/preprocessMarkdown.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/About.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/utils/MarkdownImageTransformer.ktgradle/libs.versions.toml
| processed = processed.replace( | ||
| Regex("""!\[([^\]]*)\]\(([^)]*?)#gh-(dark|light)-mode-only([^)]*)\)"""), | ||
| ) { match -> | ||
| val mode = match.groupValues[3] | ||
| if ((isDark && mode == "light") || (!isDark && mode == "dark")) { | ||
| "" // drop entirely; alt text would just be noise here | ||
| } else { | ||
| // Strip the fragment so the URL is clean for Coil's cache key. | ||
| val alt = match.groupValues[1] | ||
| val urlBase = match.groupValues[2] | ||
| val trailing = match.groupValues[4] | ||
| "" | ||
| } | ||
| } |
There was a problem hiding this comment.
Removing mismatched themed images can leave empty markdown links.
At Line 19, returning "" for mismatched variants can turn linked images into [](...) artifacts when the image is wrapped in a link.
Suggested patch
processed = processed.replace(
Regex("""!\[([^\]]*)\]\(([^)]*?)`#gh-`(dark|light)-mode-only([^)]*)\)"""),
) { match ->
@@
}
}
+
+ // Cleanup empty markdown links left after dropping mismatched linked images.
+ processed = processed.replace(Regex("""\[\s*\]\(([^)]+)\)"""), "")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| processed = processed.replace( | |
| Regex("""!\[([^\]]*)\]\(([^)]*?)#gh-(dark|light)-mode-only([^)]*)\)"""), | |
| ) { match -> | |
| val mode = match.groupValues[3] | |
| if ((isDark && mode == "light") || (!isDark && mode == "dark")) { | |
| "" // drop entirely; alt text would just be noise here | |
| } else { | |
| // Strip the fragment so the URL is clean for Coil's cache key. | |
| val alt = match.groupValues[1] | |
| val urlBase = match.groupValues[2] | |
| val trailing = match.groupValues[4] | |
| "" | |
| } | |
| } | |
| processed = processed.replace( | |
| Regex("""!\[([^\]]*)\]\(([^)]*?)`#gh-`(dark|light)-mode-only([^)]*)\)"""), | |
| ) { match -> | |
| val mode = match.groupValues[3] | |
| if ((isDark && mode == "light") || (!isDark && mode == "dark")) { | |
| "" // drop entirely; alt text would just be noise here | |
| } else { | |
| // Strip the fragment so the URL is clean for Coil's cache key. | |
| val alt = match.groupValues[1] | |
| val urlBase = match.groupValues[2] | |
| val trailing = match.groupValues[4] | |
| "" | |
| } | |
| } | |
| // Cleanup empty markdown links left after dropping mismatched linked images. | |
| processed = processed.replace(Regex("""\[\s*\]\(([^)]+)\)"""), "") |
🤖 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
`@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/util/applyThemeAwareImages.kt`
around lines 14 - 27, The current replacement returns "" for mismatched themed
images which can leave a leftover linking wrapper like [](...); update
the replacement logic in applyThemeAwareImages (the processed.replace call and
its regex/replacement) to detect and remove an optional surrounding markdown
link when dropping an inner image: either extend the Regex to optionally capture
a surrounding "[ ... ](...)" wrapper and return an empty string for the whole
wrapper, or in the lambda inspect characters/substring around the matched span
(using processed and match.range) to remove the enclosing "[...](...)" if
present; ensure you still strip the fragment and rebuild the image URL for kept
matches (using match.groupValues[1..4] / the same alt/urlBase/trailing
variables).
c7b9259 to
f6b9e0e
Compare
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.
Test plan
Summary by CodeRabbit
New Features
Improved