Conversation
egorikftp
changed the title
WalkthroughSVG manipulation code was moved from the parser component to a new SDK module: the 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 (2)
sdk/utils/svg/src/main/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifier.kt (1)
57-60: Consider using structured logging instead ofprintln.Using
printlnin library code can pollute stdout unexpectedly. Consider usingSystem.errat minimum, or better yet, a logging abstraction if available in the project.♻️ Suggested improvement
} catch (e: Exception) { - println("Failed to parse SVG for modification: ${e.message}") + System.err.println("Failed to parse SVG for modification: ${e.message}") svgContent }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/utils/svg/src/main/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifier.kt` around lines 57 - 60, Replace the println call in the catch block inside SvgDomModifier (the catch (e: Exception) { ... } that currently prints "Failed to parse SVG for modification: ${e.message}") with a structured logging call or at minimum System.err; use the project's logging abstraction if available (or java.util.logging/SLF4J) to log the error and include the exception (e) so stacktrace is preserved, while returning svgContent as before.sdk/utils/svg/src/test/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifierTest.kt (1)
14-14: Consider updating test method names to reflect the renamed API.Several test methods still reference
modifySvg(e.g., lines 14, 110, 134, 149, 164, 175) while the actual method is nowmodify. This is purely cosmetic but would improve consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/utils/svg/src/test/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifierTest.kt` at line 14, Update the test method names that still reference the old API name `modifySvg` to match the renamed API `modify` (e.g., rename test methods like `modifySvg updates root element attribute` and other tests mentioning `modifySvg` to `modify updates root element attribute` etc.), ensuring all occurrences in SvgDomModifierTest (test names and any display strings) consistently use `modify` so names reflect the current API; keep the test bodies unchanged, only rename the test method identifiers/descriptive names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/utils/svg/build.gradle.kts`:
- Around line 7-8: Remove the unused Android build tools dependency by deleting
the implementation(libs.android.build.tools) entry from the dependencies block;
the module only uses standard JDK XML APIs so keep the remaining
testImplementation(libs.bundles.kmp.test) line and ensure no other references to
libs.android.build.tools exist in the file or build scripts.
---
Nitpick comments:
In
`@sdk/utils/svg/src/main/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifier.kt`:
- Around line 57-60: Replace the println call in the catch block inside
SvgDomModifier (the catch (e: Exception) { ... } that currently prints "Failed
to parse SVG for modification: ${e.message}") with a structured logging call or
at minimum System.err; use the project's logging abstraction if available (or
java.util.logging/SLF4J) to log the error and include the exception (e) so
stacktrace is preserved, while returning svgContent as before.
In
`@sdk/utils/svg/src/test/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifierTest.kt`:
- Line 14: Update the test method names that still reference the old API name
`modifySvg` to match the renamed API `modify` (e.g., rename test methods like
`modifySvg updates root element attribute` and other tests mentioning
`modifySvg` to `modify updates root element attribute` etc.), ensuring all
occurrences in SvgDomModifierTest (test names and any display strings)
consistently use `modify` so names reflect the current API; keep the test bodies
unchanged, only rename the test method identifiers/descriptive names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3d01e045-0e2a-4a4f-b345-3a83c71cf99c
📒 Files selected for processing (9)
components/parser/jvm/svg/api/svg.apicomponents/parser/jvm/svg/build.gradle.ktssdk/utils/svg/api/svg.apisdk/utils/svg/build.gradle.ktssdk/utils/svg/src/main/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifier.ktsdk/utils/svg/src/test/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifierTest.ktsettings.gradle.ktstools/idea-plugin/build.gradle.ktstools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/domain/SvgSizeCustomizer.kt
💤 Files with no reviewable changes (2)
- components/parser/jvm/svg/build.gradle.kts
- components/parser/jvm/svg/api/svg.api
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)
sdk/utils/svg/src/main/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifier.kt (1)
35-59:⚠️ Potential issue | 🟠 MajorDon’t swallow exceptions thrown by the caller’s modification lambda.
modifications(svgElement)currently runs inside the broadcatch, so any bug in caller code is converted into a silent “return original SVG”. That makes failures very hard to spot and can produce incorrect output without any signal.💡 Suggested fix
- return try { + val document = try { val factory = DocumentBuilderFactory.newInstance() factory.apply { setFeature("http://apache.org/xml/features/disallow-doctype-decl", true) setFeature("http://xml.org/sax/features/external-general-entities", false) setFeature("http://xml.org/sax/features/external-parameter-entities", false) } val builder = factory.newDocumentBuilder() - val document = builder.parse(InputSource(StringReader(svgContent))) - - val svgElement = document.documentElement - modifications(svgElement) - - val transformerFactory = TransformerFactory.newInstance().apply { - setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "") - setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "") - } - val transformer = transformerFactory.newTransformer() - transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes") - val writer = StringWriter() - transformer.transform(DOMSource(document), StreamResult(writer)) - writer.toString() + builder.parse(InputSource(StringReader(svgContent))) } catch (e: Exception) { println("Failed to parse SVG for modification: ${e.message}") - svgContent + return svgContent } + + val svgElement = document.documentElement + modifications(svgElement) + + return try { + val transformerFactory = TransformerFactory.newInstance().apply { + setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "") + setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "") + } + val transformer = transformerFactory.newTransformer() + transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes") + val writer = StringWriter() + transformer.transform(DOMSource(document), StreamResult(writer)) + writer.toString() + } catch (e: Exception) { + println("Failed to serialize modified SVG: ${e.message}") + svgContent + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/utils/svg/src/main/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifier.kt` around lines 35 - 59, The current try/catch in SvgDomModifier.kt swallows exceptions from the caller-provided modifications(svgElement) lambda causing silent failures; change the flow so that parsing and transformation are caught separately while allowing exceptions from modifications(svgElement) to propagate (or at least be rethrown after logging). Concretely, perform document parsing and obtaining svgElement inside the try block, then call modifications(svgElement) outside that broad catch (or catch and rethrow the caught modification exception), and keep only the IO/XML-specific operations (parsing/transforming) inside a narrow try that returns svgContent on parse/transform failures; ensure you reference the existing modifications(svgElement), DocumentBuilderFactory/newDocumentBuilder/parse, and TransformerFactory/transform symbols when relocating or narrowing the catch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/utils/svg/api/svg.api`:
- Around line 1-5: The public API was renamed
(sdk.utils.svg.SvgDomModifier.modify replaces
parser.jvm.svg.SvgManipulator.modifySvg) which breaks consumers; restore a
deprecated forwarding shim by adding a class named parser.jvm.svg.SvgManipulator
with the original public method modifySvg having the original signature that
simply delegates to sdk.utils.svg.SvgDomModifier.INSTANCE.modify (preserve
parameter types and return type), annotate the old class/method as `@Deprecated`
and include a KDoc/JavaDoc note pointing to the new
sdk.utils.svg.SvgDomModifier.modify to guide users.
---
Outside diff comments:
In
`@sdk/utils/svg/src/main/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifier.kt`:
- Around line 35-59: The current try/catch in SvgDomModifier.kt swallows
exceptions from the caller-provided modifications(svgElement) lambda causing
silent failures; change the flow so that parsing and transformation are caught
separately while allowing exceptions from modifications(svgElement) to propagate
(or at least be rethrown after logging). Concretely, perform document parsing
and obtaining svgElement inside the try block, then call
modifications(svgElement) outside that broad catch (or catch and rethrow the
caught modification exception), and keep only the IO/XML-specific operations
(parsing/transforming) inside a narrow try that returns svgContent on
parse/transform failures; ensure you reference the existing
modifications(svgElement), DocumentBuilderFactory/newDocumentBuilder/parse, and
TransformerFactory/transform symbols when relocating or narrowing the catch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ff6178b1-97af-47ad-ac69-ed3ffb95ebe5
📒 Files selected for processing (9)
components/parser/jvm/svg/api/svg.apicomponents/parser/jvm/svg/build.gradle.ktssdk/utils/svg/api/svg.apisdk/utils/svg/build.gradle.ktssdk/utils/svg/src/main/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifier.ktsdk/utils/svg/src/test/kotlin/io/github/composegears/valkyrie/sdk/utils/svg/SvgDomModifierTest.ktsettings.gradle.ktstools/idea-plugin/build.gradle.ktstools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/common/domain/SvgSizeCustomizer.kt
💤 Files with no reviewable changes (2)
- components/parser/jvm/svg/api/svg.api
- components/parser/jvm/svg/build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (3)
- tools/idea-plugin/build.gradle.kts
- sdk/utils/svg/build.gradle.kts
- settings.gradle.kts
t-regbs
approved these changes
Mar 9, 2026
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
2 participants
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.
📝 Changelog
If this PR introduces user-facing changes, please update the relevant Unreleased section in changelogs: