[Plugin] Add Lucide Icons Web Import Feature#781
Conversation
...in/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt
Show resolved
Hide resolved
|
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:
WalkthroughThis pull request adds Lucide icon import functionality to the IDE plugin alongside supporting infrastructure. It introduces a new SVG manipulation utility (SvgManipulator) for DOM-based SVG modifications with security features. The Lucide feature comprises domain models, a repository for fetching icons and font data, a use case layer, a view model for state management, UI components for browsing and customizing icons, and integration with dependency injection. A LucideLogo icon is added to the compose icons library. Network infrastructure is centralized into a new NetworkModule for shared HTTP client and JSON configuration. Tests are added for SVG manipulation, and resource strings are updated for UI display. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (7)
components/parser/jvm/svg/src/test/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulatorTest.kt (2)
13-78: Good coverage of root and recursive updates; only minor brittleness riskThese tests do a nice job validating
modifySvgon the root element andupdateAttributeRecursivelyacross nested structures, including counting occurrences to assert full coverage. The only minor concern is reliance on exact serialized substrings (e.g.,stroke-width="3"and full<path .../>), which could become brittle if the XML serializer ever changes quoting or spacing behavior.You might consider, in the future, parsing
modifiedSvgback into a DOM for these specific checks (e.g., count elements with a given attribute/value) instead of relying on raw string splits, to make the tests more resilient to formatting changes.
133-212: Nice edge‑case coverage for quoting, spacing, parse failures, and empty valuesThe tests for mixed quote styles, extra spaces around
=, parse failures returning the original string, structure preservation, and empty attribute values together give good assurance around real‑world SVG quirks. Only small enhancement you might consider is an extra test whereupdateAttributeRecursively/updateAttributeConditionallytarget an attribute that doesn’t exist anywhere, to assert clean no‑op behavior.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideConfig.kt (1)
3-21: Confirm uniqueness ofLucideIcon.nameacross categories
iconsByNameis built viagridItems.values.flatten().associateBy { it.name }, so if an icon name appears in multiple category lists, the last one wins and earlier entries are silently dropped. That’s fine ifnameis guaranteed unique globally; otherwise it can hide duplicates in a non-obvious way.Consider either:
- documenting the uniqueness guarantee on
LucideIcon.name, or- guarding against duplicates (e.g., logging, or choosing a deterministic winner).
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt (1)
68-97: Revisit stroke-width customization guard and fix locale-dependent hex formattingTwo smaller points in
applySvgCustomizations/toHexString:
Stroke-width guard and absolute stroke width
The update is gated only on
strokeWidthdiffering from the default:if (settings.strokeWidth != DEFAULT_STROKE_WIDTH.toFloat()) { SvgManipulator.updateAttributeRecursively( attributeName = ATTR_STROKE_WIDTH, newValue = settings.adjustedStrokeWidth().toString(), ) }If
absoluteStrokeWidthis meant to “maintain stroke width when scaling” (per the UI text) then toggling that flag or changingsizewhile leavingstrokeWidthat the default will not adjuststroke-widthat all. You may want the condition to also considerabsoluteStrokeWidth/size, e.g. “apply whenever settings differ from the default configuration” rather than only when the slider value changes.Locale-stable hex formatting
Detekt correctly flags:
return String.format("#%06X", 0xFFFFFF and argb)as using the implicit default locale. To avoid locale-dependent behavior, specify an explicit locale:
+import java.util.Locale
...
return String.format("#%06X", 0xFFFFFF and argb)
return String.format(Locale.ROOT, "#%06X", 0xFFFFFF and argb)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt (1)
61-73: Use locale-stable formatting for the stroke width labelThe label uses:
text = "Stroke Width: ${String.format("%.1f", settings.strokeWidth)}",
String.formatwithout an explicitLocalecan behave differently under non-English locales. For predictable UI text, prefer an explicit locale, e.g.:+import java.util.Locale ... - Text( - text = "Stroke Width: ${String.format(\"%.1f\", settings.strokeWidth)}", + Text( + text = "Stroke Width: ${String.format(Locale.ROOT, \"%.1f\", settings.strokeWidth)}", style = MaterialTheme.typography.labelMedium, )This keeps the numeric formatting stable regardless of the user’s system locale.
components/parser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt (1)
46-49: Replaceprintlnwith proper logging.Using
printlnfor error reporting is not appropriate for a library component, especially within an IntelliJ plugin context. Consider using a logging framework consistent with the rest of the codebase (e.g.,com.intellij.openapi.diagnostic.Loggeras used inLucideViewModel).+import com.intellij.openapi.diagnostic.Logger + object SvgManipulator { + private val LOG = Logger.getInstance(SvgManipulator::class.java) // ... in modifySvg: } catch (e: Exception) { - println("Failed to parse SVG for modification: ${e.message}") + LOG.warn("Failed to parse SVG for modification", e) svgContent }tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kt (1)
191-203: Unchecked cast and fallback logic concern.The unchecked cast
item.icon as LucideIconat line 192 will throwClassCastExceptionifIconItemever contains a non-LucideIcon type. Consider using a type-safe approach.The fallback logic (lines 196-203) that searches for any matching icon name prefix seems like a workaround for settings changes invalidating cache keys. This works but couples the UI to cache key implementation details.
Consider using a safer cast pattern:
is IconItem<*> -> { - val lucideIcon = item.icon as LucideIcon + val lucideIcon = item.icon as? LucideIcon ?: return@items val iconCacheKey = getIconCacheKey(lucideIcon.name, state.settings)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
components/parser/jvm/svg/build.gradle.kts(1 hunks)components/parser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt(1 hunks)components/parser/jvm/svg/src/test/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulatorTest.kt(1 hunks)compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/icons/colored/LucideLogo.kt(1 hunks)tools/idea-plugin/build.gradle.kts(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportFlow.kt(2 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt(6 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideMetadata.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/di/LucideModule.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/Category.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideConfig.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideGridItem.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideIcon.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideSettings.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideTopActions.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/util/LruCache.kt(1 hunks)tools/idea-plugin/src/main/resources/messages/Valkyrie.properties(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-07T20:07:49.753Z
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 750
File: tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt:71-85
Timestamp: 2025-12-07T20:07:49.753Z
Learning: In the Valkyrie Gradle plugin (Kotlin), the `useFlatPackage` flag in `IconPackExtension` is only applicable when nested packs are configured. For single icon packs (without nested packs), the flag is intentionally not propagated to `ImageVectorGeneratorConfig` as there is no package hierarchy to flatten.
Applied to files:
compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/icons/colored/LucideLogo.kt
📚 Learning: 2025-10-21T20:55:27.073Z
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 651
File: tools/idea-plugin/build.gradle.kts:147-175
Timestamp: 2025-10-21T20:55:27.073Z
Learning: In Gradle Kotlin DSL (.gradle.kts) scripts, the types `org.gradle.api.artifacts.ArtifactCollection` and `org.gradle.api.artifacts.component.ModuleComponentIdentifier` are implicitly available and do not require explicit import statements.
Applied to files:
components/parser/jvm/svg/build.gradle.kts
🧬 Code graph analysis (6)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt (2)
compose/ui/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/ui/InfoCard.kt (1)
InfoCard(24-78)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/util/ValkyrieStrings.kt (1)
stringResource(15-21)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportScreenComponents.kt (1)
IconLoadingPlaceholder(219-230)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt (3)
compose/core/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/core/layout/Row.kt (1)
CenterVerticalRow(10-20)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/foundation/IconButton.kt (1)
IconButton(15-36)compose/core/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/core/layout/Spacer.kt (2)
HorizontalSpacer(23-29)VerticalSpacer(15-21)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideGridItem.kt (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportScreenComponents.kt (1)
CategoryHeader(106-118)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideTopActions.kt (2)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportTopActions.kt (1)
WebImportTopActions(46-131)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/CategoriesDropdown.kt (1)
CategoriesDropdown(12-29)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt (1)
loadConfig(69-92)
🪛 detekt (1.23.8)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt
[warning] 96-96: String.format("#%06X", 0xFFFFFF and argb) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (36)
components/parser/jvm/svg/build.gradle.kts (1)
10-11: ✓ Appropriate test dependencies for SVG manipulation tests.The addition of
kotlin.testandassertkwithtestImplementationscope is correct and aligns with best practices for Kotlin testing. These will support the new SvgManipulatorTest test suite.tools/idea-plugin/build.gradle.kts (1)
25-25: ✓ SVG parser module dependency correctly scoped and positioned.The addition of the SVG parser module is necessary for the Lucide icon feature's SVG manipulation capabilities. The placement among other component dependencies is consistent with the module's organizational structure.
components/parser/jvm/svg/src/test/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulatorTest.kt (3)
3-11: Test setup and assertion choices look solidUsing
assertkalongsidekotlin.testis consistent and keeps assertions readable; imports and test class scaffolding are clean with no obvious redundancy or misuse.
79-107: Conditional attribute update test is precise and effectiveThe conditional stroke update test clearly validates that only matching
currentValueentries are updated while others remain untouched, and the explicit counts for green vs red strokes give strong confidence in behavior across root and children.
214-249: End‑to‑end Lucide‑style scenario nicely validates combined behaviorThe “complex real‑world Lucide icon manipulation” test is a strong integration-style check: it exercises root attribute changes, recursive stroke‑width updates, and conditional stroke color replacement together and asserts both positive and negative conditions. This should catch most regressions in the manipulator relevant to the Lucide import flow.
tools/idea-plugin/src/main/resources/messages/Valkyrie.properties (1)
46-47: LGTM!The resource keys follow the existing naming convention and the description is clear and informative.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideSettings.kt (3)
11-14: LGTM!The validation ranges are appropriate for SVG stroke width and icon size, with clear error messages.
16-17: LGTM!The
isModifiedproperty correctly detects when any setting differs from its default value.
19-25: LGTM!The
adjustedStrokeWidthcalculation correctly scales the stroke width to maintain visual consistency across different icon sizes whenabsoluteStrokeWidthis enabled.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportFlow.kt (1)
9-9: LGTM!The Lucide import screen is correctly integrated into the navigation flow, following the same pattern as the existing Material Symbols import.
Also applies to: 21-21
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/Category.kt (1)
3-9: LGTM!The
Categorydata class is straightforward and theAllconstant provides a useful default for filtering.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideGridItem.kt (1)
10-13: LGTM!The extension function efficiently transforms categorized icons into a flat grid structure suitable for UI rendering.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt (4)
22-22: LGTM!The imports are correctly added to support the new Lucide icon provider option.
Also applies to: 29-30
40-43: LGTM!The Lucide navigation mapping follows the same pattern as the existing Google Material Symbols option.
67-67: LGTM!The Lucide info card is consistently implemented following the same pattern as the existing Google Material card, and the vertical spacing improves the layout.
Also applies to: 76-82
91-94: LGTM!The
Lucideenum member is correctly added to represent the new icon provider option.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideIcon.kt (1)
3-8: LGTM!The
LucideIcondata class provides a clear and appropriate structure for representing Lucide icons with their metadata.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/di/LucideModule.kt (3)
15-20: LGTM!The JSON configuration is appropriate for parsing external API responses, providing resilience against schema changes.
22-31: LGTM!The HTTP client is properly configured with a reasonable timeout and content negotiation for JSON responses.
33-44: LGTM!The dependency injection wiring is clean and follows good separation of concerns, exposing only the use case publicly while keeping infrastructure details private.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideTopActions.kt (1)
9-30: LucideTopActions wrapper looks clean and idiomaticThinly composing
WebImportTopActionswithCategoriesDropdownkeeps Lucide-specific wiring isolated and reuses the common UI nicely. No issues from a correctness or UX standpoint.compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/icons/colored/LucideLogo.kt (1)
12-56: LucideLogo vector and lazy caching look goodThe lazy
_LucideLogobacking field and builder setup are consistent with typical icon definitions; paths and sizes look coherent. No changes needed.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideMetadata.kt (1)
6-16: LucideIconMetadata structure aligns with repository usageThe metadata model is minimal and focused on what the repository actually needs (tags/categories, optional schema/contributors). Looks good as a serialization DTO.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/util/LruCache.kt (1)
9-40: LRU cache and thread-safe wrapper are straightforward and appropriateThe LRU implementation using a
LinkedHashMapwith access order and simple eviction, plus the synchronized wrapper for multi-threaded use, is clear and sufficient for the icon/SVG caching use cases in this PR. No changes needed.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt (1)
41-52: The currentloadIconListis safe with the actual Lucidetags.jsonstructureThe review assumes
tags.jsonmight contain non-array fields like$schema, but the actualtags.jsonpublished in lucide-static is a simple map of icon names to tag string arrays:{ "tag": ["label", "badge", "ticket"], "mail": ["email", "message", "inbox"] }Metadata like
$schemaandcontributorsare in separate per-icon JSON files, not intags.json. The current code will not crash on the actual endpoint.However, if future-proofing against API changes is desired, adding a type guard is reasonable:
- tagsJson.entries.map { (iconName, tagsArray) -> - val tags = tagsArray.jsonArray.map { it.jsonPrimitive.content } + tagsJson.entries.mapNotNull { (iconName, element) -> + if (element !is JsonArray) return@mapNotNull null + val tags = element.map { it.jsonPrimitive.content } iconName to LucideIconMetadata( tags = tags, categories = emptyList(), ) }This is optional defensive programming rather than a necessary fix for current data.
Likely an incorrect or invalid review comment.
components/parser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt (1)
59-75: LGTM: Recursive attribute update functions.The recursive traversal logic is correct and handles type checking properly. For typical SVG icons, the nesting depth is shallow enough that stack overflow is not a practical concern.
Also applies to: 85-105
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kt (2)
51-85: LGTM: Screen setup and event handling.The navigation setup, ViewModel integration, and event flow are well-structured. The use of
LaunchedEffectto collect events and navigate appropriately follows idiomatic Compose patterns.
103-128: LGTM: AnimatedContent state handling.Using
contentKeyto map states to stable keys for animation transitions is a good pattern that prevents unnecessary re-compositions.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt (3)
26-58: LGTM: Category keyword mapping with priorities.The priority-based categorization system is well-designed. The documentation clearly explains how priorities work and provides helpful examples. The keyword list covers major icon categories comprehensively.
103-120: LGTM: Category inference algorithm.The priority-based matching with name boost is a sensible approach. The
-2boost for name matches ensures that icon names take precedence over tags when determining categories.Note: When the same keyword matches both name and tags, it will appear twice in
allMatcheswith different priorities. This is harmless sinceminByOrNullcorrectly selects the name match (with the boosted/lower priority).
69-92: LGTM: loadConfig implementation.The config loading logic is clean: loads metadata, maps to domain models, extracts and sorts categories, groups icons, and creates the config. The use of
Category.Allas a prefix to the sorted categories list is intuitive for UI display.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (5)
182-205: Stale state reference after mutex release.The
statecaptured at line 183 becomes stale after the mutex is released at line 205. The network call and parsing at lines 207-214 usestate.settingsfrom this stale reference, which could differ from the current settings ifupdateSettingswas called concurrently.For display purposes this may be acceptable (the icon will be re-fetched on next settings change), but it's worth noting for correctness.
Verify whether settings changes during an in-flight load should abort or continue with stale settings. If abort is preferred, check for cancellation or settings match before
parseAndCacheIcon.
83-98: LGTM: downloadIcon implementation.The job cancellation pattern ensures only one download is active at a time, which is appropriate for the download-and-navigate flow.
128-149: LGTM: Settings update with job cancellation.The pattern of canceling all in-flight icon load jobs and re-parsing with new settings is correct. The use of
iconsByNamefor efficient lookup is good.
266-290: LGTM: Grid item filtering logic.The filtering implementation correctly handles category filtering and search queries across name, displayName, and tags. The early return for blank search queries is a good optimization.
300-327: LGTM: State and event sealed interfaces.The sealed interface hierarchy is well-defined. Using
@Stableannotations for Compose is appropriate for these state classes.
...ser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt
Show resolved
Hide resolved
...rc/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt
Show resolved
Hide resolved
...in/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideIconMetadata.kt (1)
10-15: Consider removing redundant@SerialNameannotations.The
@SerialNameannotations forcontributors,tags, andcategoriesare redundant since the property names already match the JSON keys. Only theschemafield requires it due to the special$character.Apply this diff to simplify:
- @SerialName("contributors") val contributors: List<String> = emptyList(), - @SerialName("tags") val tags: List<String>, - @SerialName("categories") val categories: List<String>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/parser/jvm/svg/api/svg.api(1 hunks)components/parser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt(1 hunks)compose/icons/api/icons.api(1 hunks)compose/icons/api/icons.klib.api(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideIconMetadata.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (3)
compose/icons/api/icons.api (1)
30-32: LucideLogo API entry is consistent and additive-onlyThe new
LucideLogoKt.getLucideLogo(ValkyrieIcons$Colored): ImageVectorentry cleanly mirrorsgetGoogleMaterialLogo, keeps ordering with other colored logo accessors, and is purely additive, so it should be binary/source compatible. No issues from the API-surface perspective.compose/icons/api/icons.klib.api (1)
21-22: LGTM! LucideLogo API addition follows existing pattern.The new LucideLogo symbol and its getter are correctly structured, consistently following the pattern of GoogleMaterialLogo and ValkyrieLogo. This auto-generated ABI dump properly reflects the new colored icon addition for the Lucide integration feature.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideIconMetadata.kt (1)
6-16: No action required - the implementation is correct.The data class safely requires
tagsandcategoriesbecause the repository code atLucideRepository.ktalways supplies these values when instantiatingLucideIconMetadata:tagsis extracted from the JSON array andcategoriesis explicitly set toemptyList(). Both fields are guaranteed to have values before the object is created.Likely an incorrect or invalid review comment.
...ser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt
Outdated
Show resolved
Hide resolved
...ser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt
Show resolved
Hide resolved
.../src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/util/LruCache.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
compose/icons/api/icons.klib.api (1)
19-20: Inconsistent API pattern for colored icon.
FlagMxis missing the@io.github.composegears.valkyrie.compose.icons.ValkyrieIcons.Coloredreceiver annotation that other colored icons use (seeGoogleMaterialLogoat line 21,LucideLogoat line 23, andValkyrieLogoat line 25). The declaration shows{}FlagMx[0]but should show@io.github.composegears.valkyrie.compose.icons.ValkyrieIcons.Colored{}FlagMx[0]to match the pattern. This is the same inconsistency observed in the JVM API dump.
🧹 Nitpick comments (3)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt (1)
94-97: Prefer explicit locale or Kotlin's format for hex strings.The static analysis tool flags
String.formatwithout explicit locale. While locale doesn't affect hex formatting, using Kotlin's native formatting is more idiomatic and avoids the warning.private fun Color.toHexString(): String { val argb = this.toArgb() - return String.format("#%06X", 0xFFFFFF and argb) + return "#%06X".format(0xFFFFFF and argb) }tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (2)
138-140: Brittle cache key parsing relies on internal format.Extracting icon names using
substringBefore("-")tightly couples this code to the cache key format defined inbuildIconCacheKey(). If the format changes (e.g., icon names containing hyphens), this logic breaks silently.Consider extracting icon names from the
config.iconsByNamekeys or maintaining a separate mapping:val currentState = lucideRecord.value.safeAs<LucideState.Success>() ?: return@launch -val loadedIconNames = currentState.loadedIcons.keys - .map { cacheKey -> cacheKey.substringBefore("-") } - .toSet() +// Get icons that have any loaded state (regardless of settings) +val loadedIconNames = currentState.loadedIcons.keys + .mapNotNull { cacheKey -> + // Match cache key format: "iconName-strokeWidth-size-absoluteStroke-color" + cacheKey.split("-").firstOrNull()?.takeIf { it in currentState.config.iconsByName } + } + .toSet()Or better yet, track loaded icon names separately to avoid parsing altogether.
184-222: Job tracking by icon name may cause cancellations across different settings.The
iconLoadJobsmap is keyed byicon.name(line 220), but the cache and state usecacheKeywhich includes settings. If the same icon is requested with different settings before the first request completes, the second request will overwrite and cancel the first job.While
updateSettings()(line 133) cancels all jobs anyway, this design could cause unexpected behavior if jobs are launched with varying settings outside of settings updates.Consider whether jobs should be keyed by
cacheKeyinstead oficon.namefor consistency:-private val iconLoadJobs = mutableMapOf<String, Job>() +private val iconLoadJobs = mutableMapOf<String, Job>() // Key: cacheKey fun loadIconForDisplay(icon: LucideIcon) { val job = viewModelScope.launch { val state = lucideRecord.value.safeAs<LucideState.Success>() ?: return@launch val cacheKey = buildIconCacheKey(icon.name, state.settings) // ... existing logic ... } - iconLoadJobs[icon.name] = job - job.invokeOnCompletion { iconLoadJobs.remove(icon.name) } + iconLoadJobs[cacheKey] = job + job.invokeOnCompletion { iconLoadJobs.remove(cacheKey) } }This would require similar updates to
reParseIconFromRepository()(line 174-175).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
compose/icons/api/icons.api(1 hunks)compose/icons/api/icons.klib.api(1 hunks)gradle/libs.versions.toml(1 hunks)tools/idea-plugin/build.gradle.kts(2 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt(1 hunks)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/idea-plugin/build.gradle.kts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-07T20:07:49.753Z
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 750
File: tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt:71-85
Timestamp: 2025-12-07T20:07:49.753Z
Learning: In the Valkyrie Gradle plugin (Kotlin), the `useFlatPackage` flag in `IconPackExtension` is only applicable when nested packs are configured. For single icon packs (without nested packs), the flag is intentionally not propagated to `ImageVectorGeneratorConfig` as there is no package hierarchy to flatten.
Applied to files:
compose/icons/api/icons.klib.apicompose/icons/api/icons.api
📚 Learning: 2025-10-21T20:55:27.073Z
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 651
File: tools/idea-plugin/build.gradle.kts:147-175
Timestamp: 2025-10-21T20:55:27.073Z
Learning: In Gradle Kotlin DSL (.gradle.kts) scripts, the types `org.gradle.api.artifacts.ArtifactCollection` and `org.gradle.api.artifacts.component.ModuleComponentIdentifier` are implicitly available and do not require explicit import statements.
Applied to files:
gradle/libs.versions.toml
🧬 Code graph analysis (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt (1)
loadConfig(69-92)
🪛 detekt (1.23.8)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt
[warning] 96-96: String.format("#%06X", 0xFFFFFF and argb) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🔇 Additional comments (4)
gradle/libs.versions.toml (2)
14-14: Library entry looks good.The androidx-collection library entry is correctly formatted and appropriate for the LRU cache use case described in the PR. The syntax follows Gradle version catalog conventions, and the placement is correct.
10-10: Version 1.5.0 is current and secure.No known vulnerabilities found for androidx.collection:collection 1.5.0. The version is the latest stable release and compatible with JDK 17. Appropriate for the LRU cache use case.
compose/icons/api/icons.api (1)
34-36: LGTM!The
LucideLogoAPI correctly follows the established pattern for colored icons with theValkyrieIcons$Coloredreceiver parameter.compose/icons/api/icons.klib.api (1)
23-24: LGTM!The
LucideLogoAPI correctly follows the established pattern for colored icons with the@io.github.composegears.valkyrie.compose.icons.ValkyrieIcons.Coloredreceiver annotation.
|
@t-regbs Also, as an idea, is it possible to extract the feature into a separate Gradle module? (can be discussed on chat, non critical for this PR). I'm not sure about the UI part, but all the network and conversion logic can potentially be isolated. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/di/MaterialSymbolsModule.kt (1)
4-5: Inconsistent import usage.Line 5 has a direct static import of
NetworkModule.httpClient, but line 11 creates a local referencenetwork = NetworkModulewhich is used fornetwork.httpClienton line 15. However, line 20 still uses the barehttpClientfrom the static import. While functionally equivalent, this is inconsistent.Consider removing the static import and using
network.httpClientconsistently:🔎 Proposed fix
import com.composegears.leviathan.Leviathan import io.github.composegears.valkyrie.ui.screen.webimport.common.di.NetworkModule -import io.github.composegears.valkyrie.ui.screen.webimport.common.di.NetworkModule.httpClient import io.github.composegears.valkyrie.ui.screen.webimport.material.data.config.MaterialSymbolsConfigRepositoryAnd on line 20:
private val materialFontRepository by instanceOf { - MaterialFontRepository(httpClient = inject(httpClient)) + MaterialFontRepository(httpClient = inject(network.httpClient)) }tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/di/NetworkModule.kt (1)
21-30: Consider addingkeepAlive = trueand connect timeout for the HttpClient.The
jsonbinding useskeepAlive = true, buthttpClientdoes not. WithoutkeepAlive, the HttpClient instance may be recreated when dependencies are re-injected. SinceHttpClientholds connection pools and resources that should be explicitly closed, this could lead to resource leaks.Additionally, only
requestTimeoutMillisis configured. Consider addingconnectTimeoutMillisto prevent indefinite hangs when the remote server is unresponsive during connection establishment.🔎 Proposed fix
- val httpClient by instanceOf { + val httpClient by instanceOf(keepAlive = true) { HttpClient(OkHttp) { install(HttpTimeout) { requestTimeoutMillis = 30.seconds.inWholeMilliseconds + connectTimeoutMillis = 10.seconds.inWholeMilliseconds } install(ContentNegotiation) { json(inject(json)) } } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/di/NetworkModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/di/LucideModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/di/MaterialSymbolsModule.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/di/LucideModule.kt (1)
8-23: LGTM!Clean module structure that follows the same DI patterns as MaterialSymbolsModule. Properly wires the Lucide dependencies using the shared NetworkModule for httpClient and json.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/di/NetworkModule.kt (1)
14-19: LGTM on Json configuration.Using
keepAlive = truefor the sharedJsoninstance is appropriate, and the configuration (ignoreUnknownKeys,isLenient) is sensible for consuming external APIs where schema changes may occur.
egorikftp
force-pushed
the
feature/lucide-web-import
branch
from
0fcb595 to
aa5fe89
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 (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/di/MaterialSymbolsModule.kt (1)
13-21: Use consistent pattern for referencing NetworkModule dependencies.The code mixes two different patterns for accessing
httpClient:
- Lines 15-16:
inject(network.httpClient)andinject(network.json)(via thenetworkreference)- Line 20:
inject(httpClient)(via direct import at line 5)This inconsistency makes the code harder to read and maintain. Choose one pattern and apply it uniformly.
🔎 Recommended fix (use network reference consistently)
Remove the direct import at line 5:
import com.composegears.leviathan.Leviathan import io.github.composegears.valkyrie.ui.screen.webimport.common.di.NetworkModule -import io.github.composegears.valkyrie.ui.screen.webimport.common.di.NetworkModule.httpClient import io.github.composegears.valkyrie.ui.screen.webimport.material.data.config.MaterialSymbolsConfigRepositoryUpdate line 20 to use the network reference:
private val materialFontRepository by instanceOf { - MaterialFontRepository(httpClient = inject(httpClient)) + MaterialFontRepository(httpClient = inject(network.httpClient)) }
🧹 Nitpick comments (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/di/NetworkModule.kt (1)
23-25: Consider adding connect and socket timeout configurations.The current configuration only sets
requestTimeoutMillis(30 seconds), but doesn't explicitly configure connect or socket timeouts. For better control over network operations and to avoid hanging connections, consider adding:🔎 Proposed enhancement
install(HttpTimeout) { requestTimeoutMillis = 30.seconds.inWholeMilliseconds + connectTimeoutMillis = 10.seconds.inWholeMilliseconds + socketTimeoutMillis = 30.seconds.inWholeMilliseconds }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/di/NetworkModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/di/LucideModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/di/MaterialSymbolsModule.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/di/LucideModule.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (ubuntu-latest)
|
@t-regbs I found some issue Screen.Recording.2025-12-23.at.14.43.20.mov |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt (1)
63-63: Use explicit locale for number formatting.The
String.formatcall uses the implicit default locale, which can produce inconsistent decimal separators across different locales (e.g., "1.5" vs "1,5"). For a numeric display in a code/technical context, consider using an explicit locale.As per static analysis hint.
🔎 Proposed fix using explicit locale
- text = "Stroke width: ${String.format("%.1f", settings.strokeWidth)}", + text = "Stroke width: ${"%.1f".format(java.util.Locale.US, settings.strokeWidth)}",Alternatively, for a more locale-aware user experience, you could use
Locale.getDefault()explicitly to show intent, or leverage platform-specific number formatters if consistency with system locale is desired.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/Slider.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt
🧰 Additional context used
🧬 Code graph analysis (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt (4)
compose/core/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/core/layout/Row.kt (1)
CenterVerticalRow(10-20)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/foundation/IconButton.kt (1)
IconButton(15-36)compose/core/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/core/layout/Spacer.kt (2)
HorizontalSpacer(23-29)VerticalSpacer(15-21)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/Slider.kt (1)
NoStopIndicatorSlider(12-44)
🪛 detekt (1.23.8)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt
[warning] 63-63: String.format("%.1f", settings.strokeWidth) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (4)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/Slider.kt (1)
12-44: Implementation looks good.The custom slider correctly disables stop indicators and tick marks through the custom track implementation. The color customization and parameter delegation are clean.
Note: This uses
ExperimentalMaterial3Api, which may change in future Material3 releases. Additionally, per the PR discussion, the team plans to migrate from Material3 to Jewel, which will require reimplementing this component.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt (3)
26-53: LGTM! Clean component structure and reset logic.The function signature follows Compose conventions, and the top bar implementation is well-structured. The reset mechanism correctly restores default settings and is appropriately enabled only when modifications exist.
78-90: LGTM! Size control implementation is correct.The pixel size slider range and step calculation are appropriate, and the float-to-int conversion using
roundToInt()properly handles the discrete pixel values.
94-113: LGTM! Well-structured switch control with clear labeling.The absolute stroke width toggle is properly implemented with descriptive text that helps users understand the feature's purpose.
|
@egorikftp ah nice catch, I will look at only reparsing the icons when the user is done dragging the sliders. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/Slider.kt (3)
14-22: Consider adding anenabledparameter.The standard Material3
Slidersupports anenabledparameter for disabling user interaction. Currently, theTrackis hardcoded toenabled = true(line 39), preventing this slider from being disabled. Adding anenabledparameter would improve flexibility.🔎 Proposed refactor to add enabled parameter
@Composable fun NoStopIndicatorSlider( value: Float, onValueChange: (Float) -> Unit, valueRange: ClosedFloatingPointRange<Float>, steps: Int, modifier: Modifier = Modifier, + enabled: Boolean = true, onValueChangeFinished: (() -> Unit)? = null, interactionSource: MutableInteractionSource = remember { MutableInteractionSource() }, ) { val colors = SliderDefaults.colors().copy( inactiveTrackColor = MaterialTheme.colorScheme.primary.copy(alpha = 0.3f), ) Slider( modifier = modifier, value = value, onValueChange = onValueChange, onValueChangeFinished = onValueChangeFinished, interactionSource = interactionSource, + enabled = enabled, colors = colors, valueRange = valueRange, steps = steps, track = { sliderState -> SliderDefaults.Track( colors = colors, - enabled = true, + enabled = enabled, sliderState = sliderState, drawStopIndicator = null, drawTick = { _, _ -> }, ) }, ) }Also applies to: 39-39
13-22: Consider adding KDoc documentation.Adding documentation would clarify the purpose of this custom slider (removing stop indicators and tick marks) and describe its parameters, improving maintainability.
23-25: Consider making the inactive track alpha configurable.The inactive track alpha is hardcoded to
0.3f. While this creates consistent styling, exposing it as an optional parameter would provide additional flexibility for different visual requirements.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideSettings.kt (2)
21-25: Consider adding KDoc to clarify purpose.The logic correctly identifies when settings differ from defaults. Adding a brief KDoc comment would help clarify its use case (e.g., determining when to show a "reset" button in the UI).
📝 Suggested documentation
+ /** + * Returns true if any setting has been modified from its default value. + * Useful for UI state management (e.g., showing reset/restore options). + */ val isModified: Boolean get() = color != Color.Unspecified || strokeWidth != DEFAULT_STROKE_WIDTH || size != DEFAULT_SIZE || absoluteStrokeWidth
27-33: Consider adding documentation to explain the absoluteStrokeWidth scaling behavior.The
adjustedStrokeWidth()function correctly scales stroke values whenabsoluteStrokeWidthis true, extending the output range to 0.25–6.0 (from the validated input range of 0.5–4.0). This is intentional and works correctly withSvgManipulator.updateAttributeRecursively(), which converts the value to a string and sets it as an SVG attribute without issues.Adding KDoc would clarify the non-obvious scaling logic:
+ /** + * Calculates the stroke width to apply to the SVG. + * + * When [absoluteStrokeWidth] is true, scales the stroke width to maintain + * visual consistency across different icon sizes. For example, a 2px stroke + * at 24px size would become 1px at 48px size to maintain the same visual thickness. + * + * @return The stroke width value to use in SVG manipulation. + * May exceed the validated 0.5-4.0 range when [absoluteStrokeWidth] is true. + */ fun adjustedStrokeWidth(): Float {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/Slider.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (3)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/Slider.kt (1)
12-12: ExperimentalMaterial3Api carries real stability risk.The
@OptIn(ExperimentalMaterial3Api::class)annotation marks experimental Material3 APIs with no long-term stability guarantee. Compose Material3 has had breaking changes in experimental surfaces across recent releases (e.g., removal of ExperimentalMaterial3ExpressiveApi and ExperimentalMaterial3ComponentOverrideApi, reworked pull-to-refresh in 1.4→1.5), so expect potential maintenance burden when upgrading Material3 versions. Consider pinning to a tested compose-material3 version and review release notes before upgrading.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideSettings.kt (2)
5-14: LGTM - Clean data class structure.The data class design is well-structured with sensible defaults and reusable constants in the companion object.
16-19: LGTM - Validation addresses UI bounds issue.The validation ranges are appropriate, and the size constraint (16..48) correctly addresses the UI bounds issue mentioned in the PR objectives where the previous max of 96px exceeded the icon card bounds.
t-regbs
force-pushed
the
feature/lucide-web-import
branch
from
2f75841 to
699d531
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideGridItem.kt:
- Around line 10-14: The toGridItems extension on Map<Category,
List<LucideIcon>> relies on insertion order and can produce non-deterministic
category ordering; modify the function to sort categories by name before mapping
by calling toSortedMap(compareBy { it.name }) (i.e., on the Map in toGridItems)
and then flatMap to produce CategoryHeader(category.title) and IconItem entries
so ordering matches Material Symbols' deterministic alphabetical order.
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt:
- Around line 53-54: iconLoadJobs is a plain mutableMapOf accessed from multiple
coroutines and can race; replace it with a thread-safe map (e.g.,
java.util.concurrent.ConcurrentHashMap or Collections.synchronizedMap) and
update the declaration in LucideViewModel to use that concurrent map for all
reads/writes; ensure places that modify iconLoadJobs (the usages around
iconLoadJob and the methods that call put/remove/containsKey) continue to use
atomic ConcurrentHashMap methods so no additional coroutine synchronization
(Mutex) is required, or alternatively guard all accesses with a single Mutex if
you prefer explicit coroutine-safe locking.
- Around line 86-101: downloadIcon lacks error handling so network/parsing
failures are swallowed; wrap the download/apply sequence (calls to
lucideUseCase.getRawSvg and lucideUseCase.applyCustomizations inside
downloadIcon) in a runCatching or try/catch, cancel/replace iconLoadJob as
before, and on success emit LucideEvent.IconDownloaded with svgContent and
IconNameFormatter.format(icon.displayName) as you do now, but on failure emit or
log a failure event (e.g., a LucideEvent.IconDownloadFailed or use _events.emit
with an error event) including the exception message so callers can react and
failures are visible.
🧹 Nitpick comments (9)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/Slider.kt (1)
12-46: Clean implementation of a customized slider.The use of
@OptIn(ExperimentalMaterial3Api::class)is appropriate—the Slider track customization remains experimental as of early 2026. The approach to disable stop indicators (drawStopIndicator = null) and ticks (drawTick = { _, _ -> }) is correct.Minor note:
enabled = trueis hardcoded in the Track. If this component is reused in contexts requiring a disabled state, consider exposing anenabledparameter. For the current use case in LucideCustomization, this is acceptable.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt (1)
42-45: Consider differentiating the error state visually.The
Errorstate is grouped withLoadingandnull, showing the same shimmer placeholder. Users won't know if an icon failed to load vs. still loading. Consider showing an error indicator or implementing a retry mechanism on tap.components/parser/jvm/svg/src/test/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulatorTest.kt (1)
74-77: Assertion may be fragile due to XML serialization differences.The exact string match
<path d=\"M10,10h4v4h-4z\"/>assumes the XML serializer outputs a self-closing tag with that exact format. Different XML serializers or configurations might output<path d="M10,10h4v4h-4z"></path>or add whitespace. Consider using a more flexible assertion.More robust assertion
- assertThat(modifiedSvg).contains("<path d=\"M10,10h4v4h-4z\"/>") + assertThat(modifiedSvg).contains("d=\"M10,10h4v4h-4z\"") + // Verify path element exists without checking exact serialization formattools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt (1)
116-120: Consider using a constant for the General fallback category.Line 119 creates a new
Categoryinstance for the "General" fallback. IfCategory.Allexists as a constant (used at line 90), consider whether aCategory.Generalconstant should also exist for consistency and to avoid creating duplicate instances.Use a constant for General category
- return bestMatch?.toCategory() ?: Category(id = "general", title = "General") + return bestMatch?.toCategory() ?: Category.GeneralAnd in the
Categorycompanion object:val General = Category(id = "general", title = "General")tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt (3)
41-52: Consider safer JSON parsing.The direct cast
as JsonObjecton line 43 will throwClassCastExceptionif the API response format changes unexpectedly. While this is acceptable for a known API schema, consider usingjsonObjectproperty accessor for slightly safer handling.- val tagsJson = json.parseToJsonElement(response.bodyAsText()) as JsonObject + val tagsJson = json.parseToJsonElement(response.bodyAsText()).jsonObject
54-66: Potential duplicate downloads due to TOCTOU race.The mutex is released between the cache check and download, allowing multiple coroutines to concurrently miss the cache and download the same icon. While functionally correct, this wastes bandwidth.
Consider holding the lock while checking and registering intent to load, or using a per-icon loading state pattern:
♻️ Suggested fix using double-check pattern
suspend fun getRawSvg(iconName: String): String = withContext(Dispatchers.IO) { - cacheMutex.withLock { - rawSvgCache[iconName] - } ?: run { - val url = "$UNPKG_BASE/icons/$iconName.svg" - val downloaded = httpClient.get(url).bodyAsText() - - cacheMutex.withLock { - rawSvgCache.put(iconName, downloaded) + cacheMutex.withLock { + rawSvgCache[iconName]?.let { return@withContext it } + } + + val url = "$UNPKG_BASE/icons/$iconName.svg" + val downloaded = httpClient.get(url).bodyAsText() + + cacheMutex.withLock { + // Double-check: another coroutine may have cached it + rawSvgCache[iconName] ?: run { + rawSvgCache.put(iconName, downloaded) + downloaded } - downloaded } }
94-97: Use explicit locale for consistent hex formatting.Per static analysis hint,
String.formatuses the implicit default locale. While hex formatting is unlikely to vary by locale, usingLocale.ROOTensures consistent output.♻️ Suggested fix
+import java.util.Locale + private fun Color.toHexString(): String { val argb = this.toArgb() - return String.format("#%06X", 0xFFFFFF and argb) + return String.format(Locale.ROOT, "#%06X", 0xFFFFFF and argb) }tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (2)
186-208: Stale state check inside mutex block.Line 191 checks
state.loadedIconswhich was captured on line 186, but this check happens inside the mutex after potentially waiting. Another coroutine may have updatedloadedIconsin the meantime.♻️ Suggested fix
fun loadIconForDisplay(icon: LucideIcon) { val job = viewModelScope.launch { - val state = lucideRecord.value.safeAs<LucideState.Success>() ?: return@launch - val cacheKey = buildIconCacheKey(icon.name, state.settings) + val initialState = lucideRecord.value.safeAs<LucideState.Success>() ?: return@launch + val cacheKey = buildIconCacheKey(icon.name, initialState.settings) iconLoadMutex.withLock { - // Skip if already loaded successfully or currently loading - val currentState = state.loadedIcons[cacheKey] + // Re-check current state after acquiring lock + val currentState = lucideRecord.value.safeAs<LucideState.Success>() + ?.loadedIcons?.get(cacheKey) if (currentState is IconLoadState.Success || currentState is IconLoadState.Loading) { return@launch }
256-258: Consider removing redundant public wrapper.
getIconCacheKeysimply delegates tobuildIconCacheKey. Unless external callers specifically need this, consider makingbuildIconCacheKeyinternal or exposing it directly.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
components/parser/jvm/svg/api/svg.apicomponents/parser/jvm/svg/build.gradle.ktscomponents/parser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.ktcomponents/parser/jvm/svg/src/test/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulatorTest.ktcompose/icons/api/icons.apicompose/icons/api/icons.klib.apicompose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/icons/colored/LucideLogo.ktgradle/libs.versions.tomltools/idea-plugin/build.gradle.ktstools/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/common/di/NetworkModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/Slider.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideIconMetadata.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/di/LucideModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/Category.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideConfig.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideGridItem.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideIcon.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideTopActions.kt
🚧 Files skipped from review as they are similar to previous changes (12)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/di/NetworkModule.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideConfig.kt
- compose/icons/api/icons.klib.api
- components/parser/jvm/svg/build.gradle.kts
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideSettings.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideIconMetadata.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportFlow.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideIcon.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/Category.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt
- components/parser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-21T20:55:27.073Z
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 651
File: tools/idea-plugin/build.gradle.kts:147-175
Timestamp: 2025-10-21T20:55:27.073Z
Learning: In Gradle Kotlin DSL (.gradle.kts) scripts, the types `org.gradle.api.artifacts.ArtifactCollection` and `org.gradle.api.artifacts.component.ModuleComponentIdentifier` are implicitly available and do not require explicit import statements.
Applied to files:
tools/idea-plugin/build.gradle.ktsgradle/libs.versions.toml
📚 Learning: 2025-12-07T20:07:49.753Z
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 750
File: tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt:71-85
Timestamp: 2025-12-07T20:07:49.753Z
Learning: In the Valkyrie Gradle plugin (Kotlin), the `useFlatPackage` flag in `IconPackExtension` is only applicable when nested packs are configured. For single icon packs (without nested packs), the flag is intentionally not propagated to `ImageVectorGeneratorConfig` as there is no package hierarchy to flatten.
Applied to files:
compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/icons/colored/LucideLogo.ktcompose/icons/api/icons.apitools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt
📚 Learning: 2026-01-01T18:09:32.917Z
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 801
File: gradle/libs.versions.toml:14-14
Timestamp: 2026-01-01T18:09:32.917Z
Learning: In the Valkyrie project (ComposeGears/Valkyrie), pin compose-ui-tooling-preview to version 1.10.0 in gradle/libs.versions.toml even if runtime Compose is 1.8.2. This is required because 1.10.0+ provides unified KMP preview annotation support needed for IntelliJ IDEA 2025.3+ preview functionality. Update the entry for compose-ui-tooling-preview to 1.10.0 and add a brief note in repository docs explaining the rationale.
Applied to files:
gradle/libs.versions.toml
🧬 Code graph analysis (4)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideTopActions.kt (2)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportTopActions.kt (1)
WebImportTopActions(46-131)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/CategoriesDropdown.kt (1)
CategoriesDropdown(12-29)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportScreenComponents.kt (1)
IconLoadingPlaceholder(260-271)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kt (7)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/foundation/TopAppBar.kt (2)
TopAppBar(34-48)BackAction(63-76)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportScreenComponents.kt (5)
LoadingContent(46-66)ErrorContent(71-122)EmptyContent(127-142)IconGrid(169-187)IconCard(199-252)compose/core/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/core/animation/RememberShimmer.kt (1)
rememberShimmer(32-50)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideTopActions.kt (1)
LucideTopActions(9-31)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (1)
getIconCacheKey(256-258)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt (1)
LucideIconDisplay(23-53)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt (1)
LucideCustomization(31-130)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt (1)
loadConfig(69-92)
🪛 detekt (1.23.8)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt
[warning] 96-96: String.format("#%06X", 0xFFFFFF and argb) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (20)
compose/icons/api/icons.api (1)
30-33: LGTM!The new
LucideLogoKtAPI entry follows the established pattern for colored icons, consistent withGoogleMaterialLogoKtandPluginIconKt. The alphabetical ordering is correct.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideTopActions.kt (1)
9-31: LGTM!Clean and well-structured composable that properly delegates to the shared
WebImportTopActionscomponent while injecting Lucide-specific category dropdown. The implementation:
- Follows Compose conventions with
modifieras the last parameter with a default value.- Correctly wires all callbacks through to the underlying components.
- Uses a consistent pattern with the existing
CategoriesDropdowngeneric component.tools/idea-plugin/build.gradle.kts (2)
30-30: LGTM!The SVG parser dependency addition aligns with the new
SvgManipulatorutility introduced for DOM-based SVG manipulation.
70-70: LGTM!The AndroidX Collection dependency provides the
LruCacheimplementation used byLucideRepositoryfor caching SVG strings (300 entries as per PR description).gradle/libs.versions.toml (1)
10-14: Approved. Version 1.5.0 is the latest stable release and is properly declared and actively used intools/idea-plugin/build.gradle.ktswithLruCacheimports across the codebase.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt (1)
23-52: LGTM!The component correctly uses
rememberUpdatedStateto capture the latest callback andLaunchedEffectkeyed oniconCacheKeyto trigger icon loading when settings change. The structure is clean and follows Compose best practices.compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/icons/colored/LucideLogo.kt (1)
12-56: LGTM!The
LucideLogoImageVector follows the standard lazy initialization pattern used throughout Compose icon libraries. The dual-path stroked design correctly represents the Lucide logo with appropriate colors and stroke attributes.components/parser/jvm/svg/api/svg.api (1)
1-6: LGTM!The
SvgManipulatorAPI surface is well-defined with a singleton pattern appropriate for a stateless utility. The three methods (modifySvg,updateAttributeRecursively,updateAttributeConditionally) provide a clean interface for DOM-based SVG manipulation.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kt (1)
51-85: LGTM!The navigation destination setup is well-structured with proper event collection via
LaunchedEffectand clean state hoisting. ThesaveableViewModelpattern ensures state survival across configuration changes.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/di/LucideModule.kt (1)
8-23: LGTM!The DI module correctly shares the
httpClientandjsoninstances fromNetworkModuleas suggested in the PR review comments. The module properly encapsulateslucideRepositoryas a private implementation detail while exposing onlylucideUseCaseas the public API.components/parser/jvm/svg/src/test/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulatorTest.kt (1)
11-249: Comprehensive test coverage.The test suite thoroughly covers the
SvgManipulatorfunctionality including recursive updates, conditional updates, nested elements, edge cases (empty attributes, parse failures), and a real-world Lucide icon scenario. Well-structured tests with clear naming.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt (2)
66-92: LGTM!The
loadConfigfunction cleanly maps metadata to domain models, groups icons by category, and constructs the configuration. The category inference with priority-based keyword matching is well-documented and handles edge cases appropriately.
26-58: Well-documented category keyword system.The priority-based keyword mapping with clear documentation and examples makes the categorization logic maintainable. The tiered priorities (1→high specificity, 5→low specificity) provide sensible defaults for icon classification.
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt (2)
20-39: LGTM: Clean repository setup with appropriate caching.The use of
LruCachewith mutex protection and well-organized constants is a solid foundation.
68-92: LGTM: Clean conditional SVG customization.The logic appropriately applies only changed attributes and delegates to
SvgManipulatorfor DOM manipulation.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (5)
60-84: LGTM: Proper initialization with state recovery.The init block correctly avoids redundant loading when state is already
Success, and error handling is appropriate.
224-254: LGTM: Clean parsing and error handling.Both methods properly synchronize cache updates and maintain consistent state.
269-293: LGTM: Comprehensive filtering logic.The filter correctly handles category selection and search across name, display name, and tags with case-insensitive matching.
295-300: LGTM: Useful state update helper.This pattern simplifies state mutations. Note that concurrent calls could race, but this is acceptable for UI state management.
303-330: LGTM: Well-structured state definitions.The sealed interfaces provide clear state modeling, and the
@Stableannotation is appropriate for Compose state.
...in/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideGridItem.kt
Outdated
Show resolved
Hide resolved
...main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kt
Outdated
Show resolved
Hide resolved
...rc/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt
Outdated
Show resolved
Hide resolved
...rc/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt
Show resolved
Hide resolved
...rc/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt
Outdated
Show resolved
Hide resolved
t-regbs
force-pushed
the
feature/lucide-web-import
branch
from
699d531 to
2f75841
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt:
- Around line 41-52: The loadIconList function performs an unsafe cast and lacks
HTTP error handling; wrap the HTTP request and JSON parsing in a try/catch,
check the HTTP response status before parsing (e.g., ensure status is 200/OK),
parse the body with json.parseToJsonElement(...) and verify the result is a
JsonObject (if not, handle gracefully by logging and returning an empty list or
propagating a meaningful exception), and handle any JSON parsing exceptions;
update references in this function (loadIconList) to use these guards so you
don’t cast to JsonObject blindly and you handle non-OK responses/timeouts
appropriately.
🧹 Nitpick comments (9)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/di/MaterialSymbolsModule.kt (1)
5-5: Inconsistent access pattern forhttpClient.
materialSymbolsConfigRepositoryusesnetwork.httpClient(line 15), whilematerialFontRepositoryuses the direct importhttpClient(line 20). For consistency within this module, use thenetworkinstance for both repositories and remove the unused direct import.Suggested fix
Remove the direct import at line 5:
import com.composegears.leviathan.Leviathan import io.github.composegears.valkyrie.ui.screen.webimport.common.di.NetworkModule -import io.github.composegears.valkyrie.ui.screen.webimport.common.di.NetworkModule.httpClient import io.github.composegears.valkyrie.ui.screen.webimport.material.data.config.MaterialSymbolsConfigRepositoryUpdate
materialFontRepositoryto usenetwork.httpClient:private val materialFontRepository by instanceOf { - MaterialFontRepository(httpClient = inject(httpClient)) + MaterialFontRepository(httpClient = inject(network.httpClient)) }Also applies to: 19-21
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt (1)
47-56: Consider differentiating error state from loading.Error and Loading states both show
IconLoadingPlaceholder, making them visually indistinguishable. Users won't know if an icon failed to load versus still loading.Consider showing an error indicator or adding retry capability for failed icons.
♻️ Optional: Add visual distinction for error state
when (iconLoadState) { - null, IconLoadState.Loading, IconLoadState.Error -> { + null, IconLoadState.Loading -> { IconLoadingPlaceholder(shimmer = shimmer) } + IconLoadState.Error -> { + Icon( + imageVector = ValkyrieIcons.Outlined.BrokenImage, // or similar error icon + contentDescription = "Failed to load ${icon.name}", + tint = MaterialTheme.colorScheme.error, + ) + } is IconLoadState.Success -> Icon(tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt (2)
38-39: Local state may become stale if settings change externally.The local
strokeWidthandsizestates are initialized once fromsettingsusingremember. Ifsettingschanges from an external source (e.g., after navigation or state restoration), these local values won't update.Consider using
LaunchedEffectto sync or userememberUpdatedState:♻️ Proposed fix
- var strokeWidth by remember { mutableFloatStateOf(settings.strokeWidth) } - var size by remember { mutableIntStateOf(settings.size) } + var strokeWidth by remember(settings.strokeWidth) { mutableFloatStateOf(settings.strokeWidth) } + var size by remember(settings.size) { mutableIntStateOf(settings.size) }
73-73: Locale-dependent decimal formatting.
String.format("%.1f", strokeWidth)uses the default locale, which may produce unexpected results (e.g., comma instead of period as decimal separator in some locales).Consider using
Locale.USorLocale.ROOTfor consistent formatting:♻️ Proposed fix
- text = "Stroke width: ${String.format("%.1f", strokeWidth)}", + text = "Stroke width: ${String.format(java.util.Locale.ROOT, "%.1f", strokeWidth)}",components/parser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt (1)
33-59: Good security practices for XML parsing.The XXE (XML External Entity) prevention measures are properly implemented:
disallow-doctype-declset to true- External general and parameter entities disabled
ACCESS_EXTERNAL_DTDandACCESS_EXTERNAL_STYLESHEETrestricted on TransformerFactoryOne minor issue: Line 56 uses
printlnfor error logging. Consider removing the print statement or using a proper logger for consistency with the rest of the codebase.♻️ Optional: Remove println or use proper logging
} catch (e: Exception) { - println("Failed to parse SVG for modification: ${e.message}") svgContent }tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kt (1)
192-193: Unchecked cast could be made safer.The cast
item.icon as LucideIconassumes allIconIteminstances in the grid containLucideIcon. While this should be true in this context, consider a safer approach:♻️ Safer type handling
is IconItem<*> -> { - val lucideIcon = item.icon as LucideIcon + val lucideIcon = item.icon as? LucideIcon ?: return@itemstools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt (1)
94-97: Use explicit locale for hex color formatting.As flagged by static analysis,
String.formatuses the default locale implicitly. While hex formatting is typically locale-independent, it's best to be explicit:♻️ Proposed fix
private fun Color.toHexString(): String { val argb = this.toArgb() - return String.format("#%06X", 0xFFFFFF and argb) + return String.format(java.util.Locale.ROOT, "#%06X", 0xFFFFFF and argb) }tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (2)
213-260: State capture may become stale during async execution.In
loadIconForDisplay, thestateis captured at line 215, but the coroutine executes asynchronously. If settings change during execution,state.settingsat line 246 will be stale.Additionally, lines 254-257 compute the cache key multiple times with potentially different values:
♻️ Proposed fix - capture settings early and reuse
fun loadIconForDisplay(icon: LucideIcon): Job { + val currentState = lucideRecord.value.safeAs<LucideState.Success>() ?: return Job() + val settings = currentState.settings + val cacheKey = buildIconCacheKey(icon.name, settings) + val job = viewModelScope.launch { - val state = lucideRecord.value.safeAs<LucideState.Success>() ?: return@launch - val cacheKey = buildIconCacheKey(icon.name, state.settings) - iconLoadJobs[cacheKey]?.cancel() val mutex = iconLoadMutexes.computeIfAbsent(cacheKey) { Mutex() } mutex.withLock { - val currentState = state.loadedIcons[cacheKey] - if (currentState is IconLoadState.Success || currentState is IconLoadState.Loading) { + val loadState = lucideRecord.value.safeAs<LucideState.Success>()?.loadedIcons?.get(cacheKey) + if (loadState is IconLoadState.Success || loadState is IconLoadState.Loading) { return@launch } // ... rest of the logic } runCatching { val rawSvg = lucideUseCase.getRawSvg(icon.name) - val customizedSvg = lucideUseCase.applyCustomizations(rawSvg, state.settings) + val customizedSvg = lucideUseCase.applyCustomizations(rawSvg, settings) parseAndCacheIcon(icon.name, customizedSvg, cacheKey) }.onFailure { error -> handleIconParseError(icon.name, cacheKey, error) } } - iconLoadJobs[buildIconCacheKey(icon.name, lucideRecord.value.safeAs<LucideState.Success>()?.settings ?: LucideSettings())] = job + iconLoadJobs[cacheKey] = job job.invokeOnCompletion { - iconLoadJobs.remove(buildIconCacheKey(icon.name, lucideRecord.value.safeAs<LucideState.Success>()?.settings ?: LucideSettings())) + iconLoadJobs.remove(cacheKey) } return job }
339-344:updateSuccesshas potential race condition.The read-check-write pattern on
lucideRecord.valueis not atomic. Concurrent calls could overwrite each other's changes.In practice, most calls are on
Dispatchers.DefaultorviewModelScopewhich may serialize access, but consider using a mutex or atomic update pattern for safety:♻️ Thread-safe update pattern
+ private val stateMutex = Mutex() + - private inline fun updateSuccess(crossinline transform: (LucideState.Success) -> LucideState.Success) { + private suspend inline fun updateSuccess(crossinline transform: (LucideState.Success) -> LucideState.Success) { + stateMutex.withLock { val current = lucideRecord.value if (current is LucideState.Success) { lucideRecord.value = transform(current) } + } }Note: This would require updating callers to use coroutine context.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
components/parser/jvm/svg/api/svg.apicomponents/parser/jvm/svg/build.gradle.ktscomponents/parser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.ktcomponents/parser/jvm/svg/src/test/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulatorTest.ktcompose/icons/api/icons.apicompose/icons/api/icons.klib.apicompose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/icons/colored/LucideLogo.ktgradle/libs.versions.tomltools/idea-plugin/build.gradle.ktstools/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/common/di/NetworkModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/Slider.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideIconMetadata.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/di/LucideModule.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/Category.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideConfig.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideGridItem.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideIcon.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideSettings.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideTopActions.kttools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/di/MaterialSymbolsModule.kttools/idea-plugin/src/main/resources/messages/Valkyrie.properties
🚧 Files skipped from review as they are similar to previous changes (11)
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportFlow.kt
- compose/icons/api/icons.api
- tools/idea-plugin/src/main/resources/messages/Valkyrie.properties
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/di/NetworkModule.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideSettings.kt
- compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/icons/colored/LucideLogo.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideIcon.kt
- tools/idea-plugin/build.gradle.kts
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideIconMetadata.kt
- tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideGridItem.kt
- gradle/libs.versions.toml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-07T20:07:49.753Z
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 750
File: tools/gradle-plugin/src/main/kotlin/io/github/composegears/valkyrie/gradle/internal/task/GenerateImageVectorsTask.kt:71-85
Timestamp: 2025-12-07T20:07:49.753Z
Learning: In the Valkyrie Gradle plugin (Kotlin), the `useFlatPackage` flag in `IconPackExtension` is only applicable when nested packs are configured. For single icon packs (without nested packs), the flag is intentionally not propagated to `ImageVectorGeneratorConfig` as there is no package hierarchy to flatten.
Applied to files:
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt
📚 Learning: 2026-01-01T18:09:41.901Z
Learnt from: egorikftp
Repo: ComposeGears/Valkyrie PR: 801
File: gradle/libs.versions.toml:14-14
Timestamp: 2026-01-01T18:09:41.901Z
Learning: In the Valkyrie project (ComposeGears/Valkyrie), compose-ui-tooling-preview must use version 1.10.0 (even though runtime Compose is 1.8.2) because 1.10.0+ provides unified KMP preview annotation support required for IntelliJ IDEA 2025.3+ preview functionality.
Applied to files:
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt
🧬 Code graph analysis (5)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideTopActions.kt (2)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportTopActions.kt (1)
WebImportTopActions(46-131)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/CategoriesDropdown.kt (1)
CategoriesDropdown(12-29)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/WebImportScreenComponents.kt (1)
IconLoadingPlaceholder(219-230)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt (4)
compose/core/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/core/layout/Row.kt (1)
CenterVerticalRow(10-20)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/foundation/IconButton.kt (1)
IconButton(15-36)compose/core/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/core/layout/Spacer.kt (2)
HorizontalSpacer(23-29)VerticalSpacer(15-21)tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/Slider.kt (1)
NoStopIndicatorSlider(12-46)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt (1)
compose/ui/src/commonMain/kotlin/io/github/composegears/valkyrie/compose/ui/InfoCard.kt (1)
InfoCard(24-78)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt (1)
loadConfig(69-92)
🪛 detekt (1.23.8)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt
[warning] 96-96: String.format("#%06X", 0xFFFFFF and argb) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🔇 Additional comments (27)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/material/di/MaterialSymbolsModule.kt (1)
22-27: LGTM!The public exposure of
materialSymbolsConfigUseCaseis appropriate for this DI module, and the wiring is correct.components/parser/jvm/svg/build.gradle.kts (1)
9-10: LGTM!The test dependency addition is appropriate for supporting the new
SvgManipulatorTesttests introduced in this PR.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/Category.kt (1)
3-10: LGTM!Clean domain model design. Using a data class makes it suitable as a map key (for
gridItems: Map<Category, List<LucideIcon>>), and the companion object pattern for the staticAllinstance is idiomatic Kotlin.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/common/ui/Slider.kt (2)
36-44: LGTM!The custom
Trackconfiguration correctly disables the stop indicator and tick marks, providing a cleaner slider appearance for the Lucide customization UI.
21-21: The current implementation is correct and aligns with official Jetpack Compose best practices. Usingremember { MutableInteractionSource() }as a default parameter value for a composable is the recommended pattern, not an anti-pattern. The alternative suggested in the original review—usingnullas a sentinel value to lazily initialize withrememberinside the function body—is explicitly discouraged by the official Compose API guidelines as it produces inconsistent behavior across recompositions. No changes are needed.compose/icons/api/icons.klib.api (1)
21-22: LGTM!The new
LucideLogoAPI entry follows the established pattern for colored icons in the ValkyrieIcons API surface.components/parser/jvm/svg/api/svg.api (1)
1-6: LGTM!The
SvgManipulatorAPI is well-structured with clear separation of concerns:
modifySvgfor high-level SVG string manipulation via callbackupdateAttributeRecursivelyandupdateAttributeConditionallyfor DOM-level operationsNote that the public API exposes
org.w3c.dom.Element, which couples consumers to the W3C DOM API. This appears intentional given themodifySvgpattern allows callback-based manipulation without requiring direct Element handling by most callers.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt (3)
34-47: LGTM! Clean navigation routing.The when expression follows the existing pattern for GoogleMaterialSymbols and correctly routes Lucide to LucideImportScreen.
91-94: LGTM!The enum extension is straightforward.
67-82: LGTM! Consistent UI pattern.Good use of
verticalArrangement = Arrangement.spacedBy(16.dp)instead of manual spacers—this is cleaner and ensures consistent spacing. The new Lucide InfoCard follows the same structure as the Google Material card. Resource strings for the Lucide card are properly defined in the messages bundle.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/di/LucideModule.kt (1)
8-23: LGTM! Clean DI wiring.Good approach sharing
httpClientandjsonviaNetworkModuleas suggested in the PR comments. The repository is appropriately kept private while exposing only the use case.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideIconDisplay.kt (1)
35-41: LGTM! Proper resource cleanup.Good use of
DisposableEffectwith job cancellation on dispose to prevent leaked coroutines when the composable leaves composition or the cache key changes.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideTopActions.kt (1)
9-31: LGTM!Clean wrapper that follows Compose conventions and properly delegates to existing components. The
categoryName = { it.title }mapping is simple and effective.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/model/LucideConfig.kt (1)
9-19: Remove this concern — icon names in Lucide are globally unique.
associateBy { it.name }doesn't risk losing data. Each icon is created once from the Lucide repository (which has globally unique icon names) and grouped into exactly one category viagroupBy { it.category }. No duplicates exist in the flattened result.components/parser/jvm/svg/src/test/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulatorTest.kt (1)
1-250: Comprehensive test coverage for SvgManipulator.The test suite provides thorough coverage including:
- Root and recursive attribute updates
- Conditional attribute matching
- Nested element handling
- Various SVG attribute formats (quotes, spaces)
- Error handling for invalid input
- Real-world Lucide icon manipulation scenario
The tests are well-structured and validate the key behaviors of the
SvgManipulatorutility.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/ui/LucideCustomization.kt (1)
41-129: Well-structured customization panel.The UI layout is clean with proper use of layout helpers (
CenterVerticalRow,VerticalSpacer, etc.). The pattern of updating settings only ononValueChangeFinishedis a good optimization to avoid excessive re-parsing during slider drags, as discussed in the PR comments.components/parser/jvm/svg/src/main/kotlin/io/github/composegears/valkyrie/parser/jvm/svg/SvgManipulator.kt (1)
68-114: Clean recursive attribute update implementation.Both
updateAttributeRecursivelyandupdateAttributeConditionallyare well-implemented with clear separation of concerns. The recursive traversal handles all child elements correctly.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt (3)
26-64: Well-documented category keyword system.The priority-based keyword mapping is clearly documented with helpful examples. The structure allows for extensibility when adding new categories.
103-120: Category inference logic is solid.The priority-based matching with name match boosting (-2) effectively prioritizes direct name matches over tag matches. The fallback to "General" category handles unmatched icons gracefully.
Minor note: negative priorities (from the -2 boost) work correctly with
minByOrNull, but consider documenting this behavior or using a separate scoring mechanism for clarity.
66-133: Clean use case design.The
LucideUseCasefollows good separation of concerns by delegating data operations to the repository while handling business logic (category inference, display name formatting) internally.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kt (2)
52-86: Well-structured navigation destination.The screen properly:
- Uses
saveableViewModelfor state persistence across configuration changes- Handles events via
LaunchedEffectwith proper coroutine scoping- Delegates UI rendering to a separate composable for testability
133-237: Clean icons content implementation.The
IconsContentcomposable:
- Uses
rememberShimmerfor loading placeholders- Implements proper grid item spanning (headers vs icons)
- Includes smart fallback to latest successful state during settings changes for smoother UX
- Integrates customization panel via
SidePanelGood use of
pointerInputto clear focus when tapping outside input fields.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt (2)
54-66: Potential redundant fetches under concurrent access.The mutex only protects individual cache operations, not the entire fetch-then-cache sequence. Two concurrent calls for the same uncached icon could both proceed to fetch. This is benign (just redundant network calls) but could be optimized if needed.
The current implementation is acceptable for this use case since:
- Icons are typically loaded sequentially from grid visibility
- Duplicate fetches only waste bandwidth, not cause data issues
68-92: SVG customization logic is well-implemented.The customization applies modifications only when settings differ from defaults, avoiding unnecessary DOM manipulations. The conditional color update using
updateAttributeConditionallycorrectly targets onlycurrentColorvalues.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (3)
150-179: Settings update logic handles cache invalidation well.The
updateSettingsfunction properly:
- Cancels ongoing load jobs
- Clears old cache entries
- Re-parses previously loaded icons with new settings
This ensures consistent rendering when settings change.
299-311: Cache key design is appropriate.Using
||as a delimiter is a reasonable choice since it's unlikely to appear in icon names. The key includes all relevant settings for proper cache invalidation.
1-92: Solid ViewModel architecture.The ViewModel effectively manages:
- Saveable state via tiamat's
recordOf- Debounced search with
Flow.debounce- LRU caching for parsed
ImageVectorinstances- Per-icon mutexes for concurrent load protection
Good use of
ConcurrentHashMapfor thread-safe job and mutex tracking.
...n/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt
Show resolved
Hide resolved
…r against xxe attacks
# Conflicts: # compose/icons/api/icons.klib.api # gradle/libs.versions.toml # tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt`:
- Around line 76-83: Remove the extra horizontal padding applied to the Lucide
InfoCard so its layout matches the Google Material InfoCard: in
WebImportSelectorScreen.kt locate the InfoCard invocation that calls onClick = {
onClick(Lucide) } and remove the explicit Modifier.padding(horizontal = 16.dp)
(leave the base Modifier or no modifier as the sibling card uses) so both
InfoCard instances rely on the Column's Modifier.padding(16.dp).
...c/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt
Show resolved
Hide resolved
egorikftp
force-pushed
the
feature/lucide-web-import
branch
from
92b4f9f to
c887aea
Compare
|
@t-regbs I checked the current performance, and it's not ideal. 🙂 I conducted a small investigation and found a way to do it similarly to Material Symbols: using a TTF font. Url for font file: https://unpkg.com/lucide-static@latest/font/lucide.ttf |
egorikftp
force-pushed
the
feature/lucide-web-import
branch
from
c887aea to
9a0defb
Compare
Ah nice one! I didn't realise there was a TTF font for it, I will refactor to mirror the material symbols implementation. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt`:
- Around line 104-122: The downloadFont() failure handler currently replaces the
whole lucideRecord with LucideState.Error, discarding existing Success data;
instead, in the onFailure block keep the current success state and only clear or
leave fontByteArray as null: log the error with LOG.error(...) and call
updateSuccess { it.copy(fontByteArray = null) } (do not assign
lucideRecord.value = LucideState.Error), so existing config/grid items remain
available; reference lucideUseCase.loadFontBytes(), fontCache, updateSuccess,
fontByteArray, lucideRecord, and LucideState.Error when making the change.
🧹 Nitpick comments (4)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideImportScreen.kt (1)
202-274: Duplicated grid item rendering logic between font-null and font-loaded branches.The
items(...)block withkey,span, and thewhen(item)dispatch is nearly identical in the two branches (lines 203–225 and 238–272). Consider extracting the shared grid scaffolding into a helper that takes a composable lambda for the icon content slot.♻️ Sketch of extracted helper
+@Composable +private fun LucideIconGrid( + gridItems: List<GridItem>, + lazyGridState: LazyGridState, + iconContent: `@Composable` (LucideIcon) -> Unit, +) { + IconGrid(state = lazyGridState) { + items( + items = gridItems, + key = { it.id }, + span = { item -> + when (item) { + is CategoryHeader -> GridItemSpan(maxLineSpan) + is IconItem<*> -> GridItemSpan(1) + } + }, + ) { item -> + when (item) { + is CategoryHeader -> CategoryHeader(title = item.categoryName) + is IconItem<*> -> iconContent(item.icon as LucideIcon) + } + } + } +}tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/data/LucideRepository.kt (1)
21-25:@latestURLs skip version pinning — deliberate trade-off to note.Using
@latestfor both UNPKG and jsDelivr means the plugin always gets the newest icons, but a breaking change inlucide-static's format (e.g., tags.json structure change, CSS class naming change) would silently break the plugin. Consider pinning to a known version and updating it periodically, or at minimum documenting this choice.tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/domain/LucideUseCase.kt (1)
70-96: Icons without matching codepoints are silently filtered out.Line 75 silently skips icons whose name doesn't appear in the codepoints map. A mismatch between
tags.jsonandlucide.css(e.g., a newly added icon present in tags but not yet in the font) would cause icons to vanish without any diagnostic signal. Consider logging the count of skipped icons at debug level so discrepancies are discoverable.💡 Sketch
val icons = iconMetadataList.mapNotNull { (name, metadata) -> val codepoint = codepoints[name] ?: return@mapNotNull null // ... } + + val skipped = iconMetadataList.size - icons.size + if (skipped > 0) { + // Use a logger if available in this layer + println("LucideUseCase: $skipped icons skipped (no matching codepoint)") + }tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt (1)
55-61: Double-read oflucideRecord.valuein init — minor code smell.The
whenreadslucideRecord.valueonce, then the innerifreads it again and performs an unsafe cast. While safe here (init is single-threaded), capturing the value once is cleaner and avoids the redundant read + cast.♻️ Proposed fix
init { - when (lucideRecord.value) { - is LucideState.Success -> if ((lucideRecord.value as LucideState.Success).fontByteArray == null) { - downloadFont() - } + val initialState = lucideRecord.value + when { + initialState is LucideState.Success && initialState.fontByteArray == null -> downloadFont() + initialState !is LucideState.Success -> loadConfig() - else -> loadConfig() }
...rc/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt
Show resolved
Hide resolved
|
@egorikftp Can you have another look? |
...rc/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/lucide/LucideViewModel.kt
Outdated
Show resolved
Hide resolved
|
@t-regbs By the way HUGE thanks, cool job |
egorikftp
force-pushed
the
feature/lucide-web-import
branch
from
b4c3feb to
99438c1
Compare
|
@t-regbs now super cool, thanks for quick implementation. Added a few commits to clean up code |
…lucide and material
# Conflicts: # compose/icons/api/icons.api # compose/icons/api/icons.klib.api # sdk/compose/icons/src/commonMain/kotlin/io/github/composegears/valkyrie/sdk/compose/icons/colored/LucideLogo.kt # tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/webimport/WebImportSelectorScreen.kt
egorikftp
force-pushed
the
feature/lucide-web-import
branch
from
9613e85 to
f1ca375
Compare
3 participants
What did I do?
PR addresses #766 , adding support for importing icons from Lucide. Matches existing material symbols functionality so users can now browse, customize, and import Lucide icons directly within the IDE plugin.
Screen.Recording.2025-12-15.at.16.34.46.mov
How did I do it?
Key Components
LucideConfig
Data model with built-in indexing for performance
LucideRepository
Handles data fetching and SVG customization:
HTTP Layer:
Caching:
LucideUseCase
Contains business logic for icon categorization and customization:
Category Inference:
Priority-based keyword matching name keywords > tag keywords
58 predefined keywords across 15+ categories
Name matches get priority boost -2 over tag matches
Falls back to "General" category if no matches
LucideViewModel
Manages state, caching, and concurrency for the import flow:
SvgManipulator
A robust utility for DOM-based SVG manipulation that handles attribute modifications reliably.