Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (8)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughAdds a new colored icon asset 🚥 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. Review rate limit: 6/8 reviews remaining, refill in 10 minutes and 25 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
sdk/compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/compose/icons/colored/EvaLogo.kt (1)
23-38: Consider dropping the fully transparent off-viewport path.This first path is rendered with
fillAlpha = 0fand its coordinates lie outside the22 × 17viewport (from(-1, -4)to(23, 20)), so it has no visual effect but still contributes parsing/rendering work every time the vector is built. If it isn't serving as an intentional padding/bounds hint, consider removing it to keep the vector minimal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/compose/icons/colored/EvaLogo.kt` around lines 23 - 38, Remove the fully transparent, off-viewport rectangle path in EvaLogo.kt that starts with path(fill = SolidColor(Color.White), fillAlpha = 0f, ...) and uses coordinates from (-1, -4) to (23, 20); this path provides no visible output and only costs rendering/parsing time—delete that entire path block (or guard it behind a clearly named flag like includePaddingBounds if you need it for intentional padding) so the remaining vector paths define the icon minimally.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt (1)
131-137: Nit: parameter order differs from siblingInfoCardcalls.All other
InfoCardinvocations above order arguments asonClick, icon, tint, title, description. Heretitleis placed beforetint. Behaviorally identical (named args), but aligning the order improves readability and diff-friendliness.Proposed tweak
InfoCard( onClick = { onClick(Eva) }, icon = ValkyrieIcons.Colored.EvaLogo, - title = stringResource("web.import.selector.eva.title"), tint = Color.Unspecified, + title = stringResource("web.import.selector.eva.title"), description = stringResource("web.import.selector.eva.description"), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt` around lines 131 - 137, Reorder the named parameters in the InfoCard call for the Eva option so they match the sibling calls' argument order: place onClick, icon, tint, title, description (i.e., move tint before title) in the InfoCard invocation (method/component name: InfoCard; parameter names: onClick, icon, tint, title, description) to improve readability and diff consistency.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/eva/data/EvaRepository.kt (1)
44-46: Consider pinning the eva-icons CDN version.Using
eva-icons@latestmeans any upstream release can silently change the font, CSS, or SVG contents (and the codepoint map cached in-memory). For reproducible imports, consider pinning to a known-good version (e.g.eva-icons@1.1.3) — same as whatever strategy the other providers (Lucide/Bootstrap) are using. Non-blocking if the rest of the web-import flow already standardizes on@latest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/eva/data/EvaRepository.kt` around lines 44 - 46, The constants CDN_BASE, CSS_URL and FONT_URL in EvaRepository.kt currently use "eva-icons@latest" which risks unpredictable upstream changes; change CDN_BASE to a pinned version like "https://cdn.jsdelivr.net/npm/eva-icons@1.1.3" (or the specific version used by other providers) so CSS_URL and FONT_URL resolve to a fixed, reproducible asset; update CDN_BASE only and ensure any related caching or in-memory codepoint map consumers (e.g., functions that read CSS_URL/FONT_URL) continue to work with the pinned version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@sdk/compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/compose/icons/colored/EvaLogo.kt`:
- Around line 23-38: Remove the fully transparent, off-viewport rectangle path
in EvaLogo.kt that starts with path(fill = SolidColor(Color.White), fillAlpha =
0f, ...) and uses coordinates from (-1, -4) to (23, 20); this path provides no
visible output and only costs rendering/parsing time—delete that entire path
block (or guard it behind a clearly named flag like includePaddingBounds if you
need it for intentional padding) so the remaining vector paths define the icon
minimally.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/eva/data/EvaRepository.kt`:
- Around line 44-46: The constants CDN_BASE, CSS_URL and FONT_URL in
EvaRepository.kt currently use "eva-icons@latest" which risks unpredictable
upstream changes; change CDN_BASE to a pinned version like
"https://cdn.jsdelivr.net/npm/eva-icons@1.1.3" (or the specific version used by
other providers) so CSS_URL and FONT_URL resolve to a fixed, reproducible asset;
update CDN_BASE only and ensure any related caching or in-memory codepoint map
consumers (e.g., functions that read CSS_URL/FONT_URL) continue to work with the
pinned version.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt`:
- Around line 131-137: Reorder the named parameters in the InfoCard call for the
Eva option so they match the sibling calls' argument order: place onClick, icon,
tint, title, description (i.e., move tint before title) in the InfoCard
invocation (method/component name: InfoCard; parameter names: onClick, icon,
tint, title, description) to improve readability and diff consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 74b22fc7-83cf-4fa8-af1a-8694c56ad487
📒 Files selected for processing (17)
sdk/compose/icons/api/icons.apisdk/compose/icons/api/icons.klib.apisdk/compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/compose/icons/colored/EvaLogo.kttools/idea-plugin/CHANGELOG.mdtools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/service/PersistentSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/settings/InMemorySettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportFlow.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/eva/EvaImportScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/eva/data/EvaCodepointParser.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/eva/data/EvaRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/eva/di/EvaModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/eva/domain/EvaUseCase.kttools/idea-plugin/src/main/resources/messages/Valkyrie.propertiestools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/eva/data/EvaCodepointParserTest.kttools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/eva/data/EvaSvgPathResolverTest.kttools/idea-plugin/src/test/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/standard/eva/domain/EvaIconNameTest.kt
egorikftp
force-pushed
the
feature/eva-icons
branch
from
ca96b77 to
c65bb75
Compare
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.
Screen.Recording.2026-04-18.at.20.03.49.mov
📝 Changelog
If this PR introduces user-facing changes, please update the relevant Unreleased section in changelogs: